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

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 8/20/2014 2:20 AM, Matt Roper wrote:
> 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().
>
I understand your points, but I am not sure why the updation of these 
intel_plane member variables was left in the first place. Do you a 
reason why this was not present till now? Am I missing something?

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