Re: [PATCH] Primary plane rotation regressions

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC