On Sun, Sep 21, 2014 at 07:20:09PM +0100, Chris Wilson wrote:
> If you attempt to use xrandr --rotation inverted at the moment, the
> kernel disables the output when attempting to update the plane
> rotation. This is because the primary plane src/dst rectangle is never
> initialised and so it attempts to restore a 0x0 sized plane.
>
> There is also a lack of debug trace through the new plane KMS functions,
> and a lack of error propagation.
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +++--
> drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++------
> 2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index af0ee7b60979..6f56b19b244a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2471,9 +2471,10 @@ static void intel_crtc_info(struct seq_file *m, struct intel_crtc *intel_crtc)
> struct intel_encoder *intel_encoder;
>
> if (crtc->primary->fb)
> - seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d\n",
> + seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d, rotation %x\n",
> crtc->primary->fb->base.id, crtc->x, crtc->y,
> - crtc->primary->fb->width, crtc->primary->fb->height);
> + crtc->primary->fb->width, crtc->primary->fb->height,
> + to_intel_plane(crtc->primary)->rotation);
> else
> seq_puts(m, "\tprimary plane disabled\n");
> for_each_encoder_on_crtc(dev, crtc, intel_encoder)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 22fc782449ee..1774b1941bbe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7361,6 +7361,17 @@ static void ironlake_get_plane_config(struct intel_crtc *crtc,
> crtc->base.primary->fb->width = ((val >> 16) & 0xfff) + 1;
> crtc->base.primary->fb->height = ((val >> 0) & 0xfff) + 1;
>
> + to_intel_plane(crtc->base.primary)->src_x = 0;
> + to_intel_plane(crtc->base.primary)->src_y = 0;
> + to_intel_plane(crtc->base.primary)->src_w = crtc->base.primary->fb->width;
> + to_intel_plane(crtc->base.primary)->src_h = crtc->base.primary->fb->height;
> +
> + /* XXX from offset */
> + to_intel_plane(crtc->base.primary)->crtc_x = 0;
> + to_intel_plane(crtc->base.primary)->crtc_y = 0;
> + to_intel_plane(crtc->base.primary)->crtc_w = crtc->base.primary->fb->width;
> + to_intel_plane(crtc->base.primary)->crtc_h = crtc->base.primary->fb->height;
I'm thinking that for now these would be better placed in
intel_modeset_readout_hw_state() where we read out the primary plane
enabled state as well, and there you could use pipe_src_w/h.
Also the src coords need <<16.
.get_plane_config() should really be renamed to .get_primary_plane_fb()
or something...
> +
> val = I915_READ(DSPSTRIDE(pipe));
> crtc->base.primary->fb->pitches[0] = val & 0xffffffc0;
>
> @@ -11497,6 +11508,8 @@ intel_primary_plane_disable(struct drm_plane *plane)
> if (!plane->fb)
> return 0;
>
> + DRM_DEBUG_KMS("CRTC:%d\n", plane->crtc->base.id);
> +
> BUG_ON(!plane->crtc);
>
> intel_crtc = to_intel_crtc(plane->crtc);
> @@ -11559,6 +11572,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
> struct drm_rect *src = &state->src;
> int ret;
>
> + DRM_DEBUG_KMS("CRTC:%d visible?=%d\n", crtc->base.id, state->visible);
> +
> intel_crtc_wait_for_pending_flips(crtc);
>
> /*
> @@ -11646,6 +11661,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int ret;
>
> + DRM_DEBUG_KMS("src=(%d, %d)x(%d, %d), crtc=(%d, %d)x(%d, %d), crtc active?=%d\n",
> + src_x, src_y, src_w, src_h, crtc_x, crtc_y, crtc_w, crtc_h,
> + intel_crtc->active);
I don't think I like having this much debug spew from plane operations.
So if you want to add these things I'd prefer DRM_DEBUG().
Also the src coords are 16.16 here, and I'd prefer we use the same fake
X geometry notation that's used in __setplane_internal() and
drm_rect_debug_print().
> +
> state.crtc = crtc;
> state.fb = fb;
>
> @@ -11670,12 +11689,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> state.orig_dst = state.dst;
>
> ret = intel_check_primary_plane(plane, &state);
> - if (ret)
> - return ret;
> -
> - intel_commit_primary_plane(plane, &state);
> -
> - return 0;
> + if (ret == 0)
> + ret = intel_commit_primary_plane(plane, &state);
> + return ret;
Just 'return intel_commit_primary_plane()' ? Matches the sprite code
which is a plus since we'll probably want to unify this stuff even
more.
> }
>
> /* Common destruction function for both primary and cursor planes */
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
