From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag Date: Tue, 29 Jul 2014 19:57:55 +0200 Message-ID: <20140729175755.GT4747@phenom.ffwll.local> References: <20140526130228.GH14357@phenom.ffwll.local> <1401114408-23475-1-git-send-email-ville.syrjala@linux.intel.com> <20140620004124.GG18027@intel.com> <20140729173155.GZ27580@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <20140729173155.GZ27580@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Tue, Jul 29, 2014 at 08:31:55PM +0300, Ville Syrj=E4l=E4 wrote: > On Thu, Jun 19, 2014 at 05:41:24PM -0700, Matt Roper wrote: > > On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com= wrote: > > > From: Ville Syrj=E4l=E4 > > > = > > > Add a flag to drm_device which will cause the vblank code to bypass t= he > > > disable timer and always disable the vblank interrupt immediately when > > > the last reference is dropped. > > > = > > > Signed-off-by: Ville Syrj=E4l=E4 > > > --- > > > drivers/gpu/drm/drm_irq.c | 6 +++--- > > > include/drm/drmP.h | 10 ++++++++++ > > > 2 files changed, 13 insertions(+), 3 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > > index 20a4855..b008803 100644 > > > --- a/drivers/gpu/drm/drm_irq.c > > > +++ b/drivers/gpu/drm/drm_irq.c > > > @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int= crtc) > > > = > > > /* Last user schedules interrupt disable */ > > > if (atomic_dec_and_test(&vblank->refcount)) { > > > - if (drm_vblank_offdelay > 0) > > > + if (dev->vblank_disable_immediate || drm_vblank_offdelay =3D=3D 0) > > > + vblank_disable_fn((unsigned long)vblank); > > > + else if (drm_vblank_offdelay > 0) > > > mod_timer(&vblank->disable_timer, > > > jiffies + ((drm_vblank_offdelay * HZ)/1000)); > > > - else if (drm_vblank_offdelay =3D=3D 0) > > > - vblank_disable_fn((unsigned long)vblank); > > > } > > > } > > > EXPORT_SYMBOL(drm_vblank_put); > > = > > Would it be better if we just squashed this device flag logic back into > > patch 7, but kept the drm_vblank_offdelay =3D=3D 0 meaning of "never > > disable?" I can see there being people who might already use this when > > debugging new and potentially flaky hardware platforms who would be > > surprised by the change in behavior. So basically something like: > > = > > if (drm_vblank_offdelay =3D=3D 0) > > return > > else if (dev->vblank_disable_immediate) > > vblank_disable_fn((unsigned long)vblank); > > else > > mod_timer(...); > = > I'm not sure I want drm_vblank_offdelay affecting drivers that have > vblank_disable_immediate set since it's a global variable. If there > are two devices on the system where one has > vblank_disable_immediate=3D=3Dtrue and the other doesn't, the user > might want to keep vblank interrupts enabled on the crappy device > all time by frobbing drm_vblank_offdelay. > = > I agree that changing the meaning of drm_vblank_offdelay=3D0 is a bit > questionable, but reversing the meaning so that '=3D=3D0' meas 'never dis= able' > and '<0' means 'disable immediately' seemed a bit weird for my taste. But > I suppose I could make that change if people think it's better. Or maybe > just forget about the 'disable immediately' behaviour when > vblank_disable_immediate=3D=3Dfalse? > = > > I'd also suggest adding a comment in drm_stub.c to indicate that > > drm_vblank_offdelay gets ignored for drivers that set > > vblank_disable_immediate. > = > Good idea. Yeah, I think that's good enough. There really shouldn't be any need for drivers which support immediate vblank disabling to ever keep it on for a while. Presuming the immediate disable actually works ofc. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch