by Ville Syrjälä

[permalink] [raw]

Subject: Re: [PATCH AUTOSEL for 4.4 016/115] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)

On Thu, Mar 29, 2018 at 05:32:13PM +0100, Ben Hutchings wrote:
> On Sat, 2018-03-03 at 22:30 +0000, Sasha Levin wrote:
> > From: Chris Wilson <[email protected]>
> >
> > [ Upstream commit 608b20506941969ea30d8c08dc9ae02bb87dbf7d ]
>
> I think we need this one too:
>
> commit 75cff0837c14eaf632efabb8d7ab9eec6394d20d
> Author: Chris Wilson <[email protected]>
> Date:???Fri Mar 24 17:30:58 2017 +0000
>
> ????drm: Make the decision to keep vblank irq enabled earlier

Why are we backporting optimizations anyway?

>
> Ben.
>
> > 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 <[email protected]>
> > Cc: Ville Syrj?l? <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: Michel D?nzer <[email protected]>
> > Cc: Laurent Pinchart <[email protected]>
> > Cc: Dave Airlie <[email protected]>,
> > Cc: Mario Kleiner <[email protected]>
> > Reviewed-by: Ville Syrj?l? <[email protected]>
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Link: http://patchwork.freedesktop.org/patch/msgid/[email protected]
> > Signed-off-by: Sasha Levin <[email protected]>
> > ---
> > ?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 8090989185b2..4ddbc49125cd 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1271,9 +1271,9 @@ 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));
> > ? }
> > @@ -1902,6 +1902,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.14.1
> --
> Ben Hutchings
> Software Developer, Codethink Ltd.

--
Ville Syrj?l?
Intel OTC

2018-03-29 18:49:07

by Ben Hutchings

[permalink] [raw]

Subject: Re: [PATCH AUTOSEL for 4.4 016/115] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)

On Thu, 2018-03-29 at 19:42 +0300, Ville Syrjälä wrote:
> On Thu, Mar 29, 2018 at 05:32:13PM +0100, Ben Hutchings wrote:
> > On Sat, 2018-03-03 at 22:30 +0000, Sasha Levin wrote:
> > > > > > From: Chris Wilson <[email protected]>
> > >
> > > [ Upstream commit 608b20506941969ea30d8c08dc9ae02bb87dbf7d ]
> >
> > I think we need this one too:
> >
> > commit 75cff0837c14eaf632efabb8d7ab9eec6394d20d
> > Author: Chris Wilson <[email protected]>
> > Date:   Fri Mar 24 17:30:58 2017 +0000
> >
> >     drm: Make the decision to keep vblank irq enabled earlier
>
> Why are we backporting optimizations anyway?

That's a good question. I don't know the reasoning behind picking the
first commit (608b20506941). I'm only suggesting the second commit
because it appears to be an important fix for the first.

Ben.

> >
> > Ben.
> >
> > > 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 <[email protected]>
> > > Cc: Ville Syrjälä <[email protected]>
> > > Cc: Daniel Vetter <[email protected]>
> > > Cc: Michel Dänzer <[email protected]>
> > > Cc: Laurent Pinchart <[email protected]>
> > > Cc: Dave Airlie <[email protected]>,
> > > Cc: Mario Kleiner <[email protected]>
> > > Reviewed-by: Ville Syrjälä <[email protected]>
> > > Signed-off-by: Daniel Vetter <[email protected]>
> > > Link: http://patchwork.freedesktop.org/patch/msgid/[email protected]
> > > Signed-off-by: Sasha Levin <[email protected]>
> > > ---
> > > 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 8090989185b2..4ddbc49125cd 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -1271,9 +1271,9 @@ 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));
> > > }
> > > @@ -1902,6 +1902,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.14.1
> >
> > --
> > Ben Hutchings
> > Software Developer, Codethink Ltd.
>
>
--
Ben Hutchings
Software Developer, Codethink Ltd.