[Intel-gfx] [PATCH 04/12] drm: Add drm_vblank_on()

[Daniel Vetter]

On Wed, May 21, 2014 at 01:32:35PM +0200, Thierry Reding wrote:
> On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > drm_vblank_off() will turn off vblank interrupts, but as long as the
> > refcount is elevated drm_vblank_get() will not re-enable them. This
> > is a problem is someone is holding a vblank reference while a modeset is
> > happening, and the driver requires vblank interrupt to work during that
> > time.
> > 
> > Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
> > re-enabled vblank interrupts if the refcount is already elevated. This
> > will allow drivers to choose the specific places in the modeset sequence
> > at which vblank interrupts get disabled and enabled.
> > 
> > Testcase: igt/kms_flip/*-vs-suspend
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > [danvet: Add Testcase tag for the igt I've written.]
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_display.c |  8 ++++
> >  include/drm/drmP.h                   |  1 +
> >  3 files changed, 62 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 13d671ed3421..dd786d84daab 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> >  }
> >  
> >  /**
> > + * drm_vblank_enable - enable the vblank interrupt on a CRTC
> > + * @dev: DRM device
> > + * @crtc: CRTC in question
> > + */
> 
> Perhaps the kernel-doc here should contain some of what's described in
> the commit message? Also a "Return:" section would be useful here to
> specify what's an error and what isn't.
> 
> > +static int drm_vblank_enable(struct drm_device *dev, int crtc)
> 
> On second thought, since this is a local function, my comments above
> apply to drm_vblank_on() below rather than drm_vblank_enable().

Yeah, we don't pull this into the kerneldoc, only functions exported to
drivers. Follow-up patch should (hopefully) sufficiently pimp the
kerneldoc for drm_vblank_on. If not please complain.

> 
> > +{
> > +	int ret = 0;
> > +
> > +	assert_spin_locked(&dev->vbl_lock);
> > +
> > +	spin_lock(&dev->vblank_time_lock);
> > +
> > +	if (!dev->vblank[crtc].enabled) {
> > +		/* Enable vblank irqs under vblank_time_lock protection.
> > +		 * All vblank count & timestamp updates are held off
> > +		 * until we are done reinitializing master counter and
> > +		 * timestamps. Filtercode in drm_handle_vblank() will
> > +		 * prevent double-accounting of same vblank interval.
> > +		 */
> 
> Coding style:
> 
> 	/*
> 	 * Enable...
> 	 */
> 
> ?

Yeah, will polish.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> 
> Perhaps split off the i915 changes into a separate patch?

Yeah, should have. But I've got fed up with the conflicts this topic
branch caused so pulled it in. I'll apply your review feedback as
follow-up patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch