Re: [PATCH 1/3] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)

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

On 03/15/2017 10:00 PM, Ville Syrjälä wrote:
> On Wed, Mar 15, 2017 at 08:40:25PM +0000, Chris Wilson wrote:
>> On vblank instant-off systems, we can get into a situation where the cost
>> of enabling and disabling the vblank IRQ around a drmWaitVblank query
>> dominates. And with the advent of even deeper hardware sleep state,
>> touching registers becomes ever more expensive.  However, we know that if
>> the user wants the current vblank counter, they are also very likely to
>> immediately queue a vblank wait and so we can keep the interrupt around
>> and only turn it off if we have no further vblank requests queued within
>> the interrupt interval.
>>
>> After vblank event delivery, this patch adds a shadow of one vblank where
>> the interrupt is kept alive for the user to query and queue another vblank
>> event. Similarly, if the user is using blocking drmWaitVblanks, the
>> interrupt will be disabled on the IRQ following the wait completion.
>> However, if the user is simply querying the current vblank counter and
>> timestamp, the interrupt will be disabled after every IRQ and the user
>> will enabled it again on the first query following the IRQ.
>>
>> v2: Mario Kleiner -
>> After testing this, one more thing that would make sense is to move
>> the disable block at the end of drm_handle_vblank() instead of at the
>> top.
>>
>> Turns out that if high precision timestaming is disabled or doesn't
>> work for some reason (as can be simulated by echo 0 >
>> /sys/module/drm/parameters/timestamp_precision_usec), then with your
>> delayed disable code at its current place, the vblank counter won't
>> increment anymore at all for instant queries, ie. with your other
>> "instant query" patches. Clients which repeatedly query the counter
>> and wait for it to progress will simply hang, spinning in an endless
>> query loop. There's that comment in vblank_disable_and_save:
>>
>> "* Skip this step if there isn't any high precision timestamp
>>  * available. In that case we can't account for this and just
>>  * hope for the best.
>>  */
>>
>> With the disable happening after leading edge of vblank (== hw counter
>> increment already happened) but before the vblank counter/timestamp
>> handling in drm_handle_vblank, that step is needed to keep the counter
>> progressing, so skipping it is bad.
>>
>> Now without high precision timestamping support, a kms driver must not
>> set dev->vblank_disable_immediate = true, as this would cause problems
>> for clients, so this shouldn't matter, but it would be good to still
>> make this robust against a future kms driver which might have
>> unreliable high precision timestamping, e.g., high precision
>> timestamping that intermittently doesn't work.
>>
>> v3: Patch before coffee needs extra coffee.
>>
>> Testcase: igt/kms_vblank
>> 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>
>
> Yep. This seems like a good idea to me. I just neglected to review it
> last time around (and maybe even before that?) for some reason. Locks
> seem to be taken in the right order, so it at least looks safe to me.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>

Hi,

as a followup to this one, maybe we should move the 
drm_handle_vblank_events(dev, pipe); down, immediately after Chris new 
delayed disable code?

The idea was to avoid lots of redundant enable->disable->enable... calls 
by having some 1 frame delay before disable. This works for pure vblank 
count/ts queries.

But both DRI2 and DRI3/Present use vblank events to trigger a 
pageflip-ioctl at the right target vblank. With the current ordering we 
may dispatch the vblank swap trigger event to the X-Server and drop the 
vblank refcount to zero due to the vblank_put inside 
drm_handle_vblank_events for the dispatched event, then detect in this 
patch that refcount == 0 and disable vblanks, but a few microseconds 
later the server will queue a pageflip ioctl which bumps the refcount 
and reenables vblank irqs, so we have a redundant disable->enable.

Also many kms drivers now use drm_crtc_arm_vblank_event() for pageflip 
completion handling at vblank, the pageflip completion events are also 
dispatched via drm_handle_vblank_events(). After a pageflip completes, 
it makes sense to have this "swap shadow" of 1 full frame, as animations 
would likely queue a new vblank query/event immediately for the next 
animation frame.

-mario

>> ---
>>  drivers/gpu/drm/drm_irq.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 9bdca69f754c..e64b05ea95ea 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1198,9 +1198,9 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>>  	if (atomic_dec_and_test(&vblank->refcount)) {
>>  		if (drm_vblank_offdelay == 0)
>>  			return;
>> -		else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
>> +		else if (drm_vblank_offdelay < 0)
>>  			vblank_disable_fn((unsigned long)vblank);
>> -		else
>> +		else if (!dev->vblank_disable_immediate)
>>  			mod_timer(&vblank->disable_timer,
>>  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
>>  	}
>> @@ -1734,6 +1734,16 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
>>  	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.
>> +	 */
>> +	if (dev->vblank_disable_immediate &&
>> +	    drm_vblank_offdelay > 0 &&
>> +	    !atomic_read(&vblank->refcount))
>> +		vblank_disable_fn((unsigned long)vblank);
>> +
>>  	spin_unlock_irqrestore(&dev->event_lock, irqflags);
>>
>>  	return true;
>> --
>> 2.11.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx