Re: [PATCH] drm/i915: Don't wait for vblank for sprite plane flips

[Vijay Purushothaman <vijay.a.purushothaman@intel.com>]

On 6/28/2013 7:54 PM, Ville Syrjälä wrote:
> On Fri, Jun 28, 2013 at 07:45:31PM +0530, Vijay Purushothaman wrote:
>> Since the sprite planes are using synchronized MMIO based flip, no need
>> to wait for vblank. Removing this wait allows us to get a nice
>> performance boost to both 3D & media workloads based on sprite (~60 fps
>> from ~20 fps)
>
> Nak. We can't unpin the buffer until the hardware has finished reading
> from it.

Thanks much for the review feedback. We have removed this check in our 
android production branch so far we have not seen any side effect or 
artifact. Is this a conservative check or do we have any use case which 
will fail without this piece of code?

Apparently the windows driver team have been using MMIO flips for Sprite 
and the windows driver also didn't wait for vblank for such flips all along.

Could you please help me understand this a bit better? This wait reduces 
the perf to ~20 fps and thus prevent us from using sprite for any OGL 
layer mapping in hwcomposer and we are losing significant amount of 
power. For video content playback the problem is not that bad.

>
> The proper fix is to do the unpin asynchronously after the flip has
> completed. That's one part of the bigger atomic pageflip story.
>

Any idea when are we planning to merge this atomic flip in d-i-n-q?

Thanks,
Vijay


>>
>> Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
>> Signed-off-by: Gary Smith <gary.k.smith@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_sprite.c |   14 +-------------
>>   1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 1fa5612..1d14fc0 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -828,20 +828,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>   		intel_disable_primary(crtc);
>>
>>   	/* Unpin old obj after new one is active to avoid ugliness */
>> -	if (old_obj) {
>> -		/*
>> -		 * It's fairly common to simply update the position of
>> -		 * an existing object.  In that case, we don't need to
>> -		 * wait for vblank to avoid ugliness, we only need to
>> -		 * do the pin & ref bookkeeping.
>> -		 */
>> -		if (old_obj != obj) {
>> -			mutex_unlock(&dev->struct_mutex);
>> -			intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
>> -			mutex_lock(&dev->struct_mutex);
>> -		}
>> +	if (old_obj)
>>   		intel_unpin_fb_obj(old_obj);
>> -	}
>>
>>   out_unlock:
>>   	mutex_unlock(&dev->struct_mutex);
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>