From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag Date: Tue, 29 Jul 2014 20:31:55 +0300 Message-ID: <20140729173155.GZ27580@intel.com> References: <20140526130228.GH14357@phenom.ffwll.local> <1401114408-23475-1-git-send-email-ville.syrjala@linux.intel.com> <20140620004124.GG18027@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: <20140620004124.GG18027@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Matt Roper Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 w= rote: > > From: Ville Syrj=E4l=E4 > > = > > Add a flag to drm_device which will cause the vblank code to bypass the > > 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 c= rtc) > > = > > /* 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 disab= le' 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. > = > Other than that, patches 1-8, 10, and 11 are > Reviewed-by: Matt Roper > = > I'll finish up reviewing #9 and 12-14 tomorrow when I have some more > time. > = > = > = > Matt > = > = > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > > index 979a498..0944544 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -1117,6 +1117,16 @@ struct drm_device { > > */ > > bool vblank_disable_allowed; > > = > > + /* > > + * If true, vblank interrupt will be disabled immediately when the > > + * refcount drops to zero, as opposed to via the vblank disable > > + * timer. > > + * This can be set to true it the hardware has a working vblank > > + * counter and the driver uses drm_vblank_on() and drm_vblank_off() > > + * appropriately. > > + */ > > + bool vblank_disable_immediate; > > + > > /* array of size num_crtcs */ > > struct drm_vblank_crtc *vblank; > > = > > -- = > > 1.8.5.5 > > = > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > -- = > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- = Ville Syrj=E4l=E4 Intel OTC