Re: [PATCH] drm: Make the decision to keep vblank irq enabled earlier

[Mario Kleiner <mario.kleiner.de@gmail.com>]

On 03/23/2017 02:26 PM, Ville Syrjälä wrote:
> On Thu, Mar 23, 2017 at 07:51:06AM +0000, Chris Wilson wrote:
>> We want to provide the vblank irq shadow for pageflip events as well as
>> vblank queries. Such events are completed within the vblank interrupt
>> handler, and so the current check for disabling the irq will disable it
>> from with the same interrupt as the last pageflip event. If we move the
>> decision on whether to disable the irq (based on there no being no
>> remaining vblank events, i.e. vblank->refcount == 0) to before we signal
>> the events, we will only disable the irq on the interrupt after the last
>> event was signaled. In the normal course of events, this will keep the
>> vblank irq enabled for the entire flip sequence whereas before it would
>> flip-flop around every interrupt.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Michel Dänzer <michel@daenzer.net>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Dave Airlie <airlied@redhat.com>,
>> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 5b77057e91ca..1d6bcee3708f 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1741,6 +1741,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
>>  {
>>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>>  	unsigned long irqflags;
>> +	bool disable_irq;
>>
>>  	if (WARN_ON_ONCE(!dev->num_crtcs))
>>  		return false;
>> @@ -1768,16 +1769,19 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
>>  	spin_unlock(&dev->vblank_time_lock);
>>
>>  	wake_up(&vblank->queue);
>> -	drm_handle_vblank_events(dev, pipe);
>>
>>  	/* With instant-off, we defer disabling the interrupt until after
>> -	 * we finish processing the following vblank. The disable has to
>> -	 * be last (after drm_handle_vblank_events) so that the timestamp
>> -	 * is always accurate.
>> +	 * we finish processing the following vblank after all events have
>> +	 * been signaled. The disable has to be last (after
>> +	 * drm_handle_vblank_events) so that the timestamp is always accurate.
>
> We wouldn't actually do the disable as long there's a reference still
> held, so the timestamp should be fine in that case. And if there aren't
> any references the timestamp shouldn't matter... I think. But it's
> probably more clear to keep to the order you propose here anyway.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>

Looks good to me. As a further optimization, i think we could move the 
vblank_disable_fn() call outside/below the spin_unlock_irqrestore for 
event_lock, as vblank_disable_fn() doesn't need any locks held at call 
time, so slightly reduce event_lock hold time. Don't know if it is worth it.

In any case

Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com>

thanks,
-mario

> Oh, and now that I think about this stuff again, I start to wonder why
> I made the disable actually update the seq/ts. If the interrupt is
> currently enabled the seq/ts should be reasonably uptodate already
> when we do disable the interrupt. Perhaps I was only thinking about
> drm_vblank_off() when I made that change, or I decided that I didn't
> want two different disable codepaths. Anyways, just an idea that
> we might be able to make the vblank irq disable a little cheaper.
>
>>  	 */
>> -	if (dev->vblank_disable_immediate &&
>> -	    drm_vblank_offdelay > 0 &&
>> -	    !atomic_read(&vblank->refcount))
>> +	disable_irq = (dev->vblank_disable_immediate &&
>> +		       drm_vblank_offdelay > 0 &&
>> +		       !atomic_read(&vblank->refcount));
>> +
>> +	drm_handle_vblank_events(dev, pipe);
>> +
>> +	if (disable_irq)
>>  		vblank_disable_fn((unsigned long)vblank);
>>
>>  	spin_unlock_irqrestore(&dev->event_lock, irqflags);
>> --
>> 2.11.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx