From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com ([192.55.52.120]:33191 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbeC2QmM (ORCPT ); Thu, 29 Mar 2018 12:42:12 -0400 Date: Thu, 29 Mar 2018 19:42:07 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Ben Hutchings Cc: Sasha Levin , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Chris Wilson , Daniel Vetter , Michel =?iso-8859-1?Q?D=E4nzer?= , Laurent Pinchart , Dave Airlie , Mario Kleiner , Daniel Vetter Subject: Re: [PATCH AUTOSEL for 4.4 016/115] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off) Message-ID: <20180329164207.GY5453@intel.com> References: <20180303223010.27106-1-alexander.levin@microsoft.com> <20180303223010.27106-16-alexander.levin@microsoft.com> <1522341133.2654.22.camel@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1522341133.2654.22.camel@codethink.co.uk> Sender: stable-owner@vger.kernel.org List-ID: 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 > > > > [ Upstream commit 608b20506941969ea30d8c08dc9ae02bb87dbf7d ] > > I think we need this one too: > > commit 75cff0837c14eaf632efabb8d7ab9eec6394d20d > Author: Chris Wilson > 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 > > Cc: Ville Syrj�l� > > Cc: Daniel Vetter > > Cc: Michel D�nzer > > Cc: Laurent Pinchart > > Cc: Dave Airlie , > > Cc: Mario Kleiner > > Reviewed-by: Ville Syrj�l� > > Signed-off-by: Daniel Vetter > > Link: http://patchwork.freedesktop.org/patch/msgid/20170315204027.20160-1-chris@chris-wilson.co.uk > > Signed-off-by: Sasha Levin > > --- > > �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