Re: [PATCH 1/2] drm/i915: Updating plane parameters for primary plane in setplane

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Aug 19, 2014 at 11:56:42AM +0530, sonika.jindal@intel.com wrote:
> From: Sonika Jindal <sonika.jindal@intel.com>
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9b578e..1b0e403 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11528,6 +11528,21 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
>  		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
>  	};
> +	const struct {
> +		int crtc_x, crtc_y;
> +		unsigned int crtc_w, crtc_h;
> +		uint32_t src_x, src_y, src_w, src_h;
> +	} orig = {
> +		.crtc_x = crtc_x,
> +		.crtc_y = crtc_y,
> +		.crtc_w = crtc_w,
> +		.crtc_h = crtc_h,
> +		.src_x = src_x,
> +		.src_y = src_y,
> +		.src_w = src_w,
> +		.src_h = src_h,
> +	};
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	bool visible;
>  	int ret;
>  
> @@ -11604,6 +11619,15 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  
>  		return 0;
>  	}
> +	intel_plane->crtc_x = orig.crtc_x;
> +	intel_plane->crtc_y = orig.crtc_y;
> +	intel_plane->crtc_w = orig.crtc_w;
> +	intel_plane->crtc_h = orig.crtc_h;
> +	intel_plane->src_x = orig.src_x;
> +	intel_plane->src_y = orig.src_y;
> +	intel_plane->src_w = orig.src_w;
> +	intel_plane->src_h = orig.src_h;
> +	intel_plane->obj = obj;
>  
>  	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
>  	if (ret)

I haven't been following all of this thread carefully, but wouldn't you
want to update the intel_plane fields after the point where the function
can no longer fail?  E.g., if I try to setplane() to a new location
while I have a pageflip pending, my request will fail and the plane
position won't update, yet you're still going to be updating the
internal state tracking variables here.  Then if you do an
intel_plane_restore() later you're going to see the plane jump
unexpectedly.

On the other hand, what about the case where the setplane succeeds, but
the plane isn't visible (either because it's fully clipped or because
your crtc isn't active right now).  Those are other paths through the
intel_primary_plane_setplane() function that don't seem to be updating
the intel_plane fields, even though it seems like you'd want to so that
your plane doesn't snap back to an old position on a later
intel_plane_restore().

Sorry if I'm overlooking something obvious here; I haven't been keeping
up with this whole email thread.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795