[Intel-gfx] [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Daniel Vetter]

On Thu, Feb 12, 2015 at 11:56:50PM +0200, Imre Deak wrote:
> On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > At driver load we need to tell the vblank code about the state of the
> > pipes, so that the logic around reject vblank_get when the pipe is off
> > works correctly.
> > 
> > Thus far i915 used drm_vblank_off, but one of the side-effects of it
> > is that it also saves the vblank counter. And for that it calls down
> > into the ->get_vblank_counter hook. Which isn't really a good idea
> > when the pipe is off for a few reasons:
> > - With runtime pm the register might not respond.
> > - If the pipe is off some datastructures might not be around or
> >   unitialized.
> > 
> > The later is what blew up on gen3: We look at intel_crtc->config to
> > compute the vblank counter, and for a disabled pipe at boot-up that's
> > just not there. Thus far this was papered over by a check for
> > intel_crtc->active, but I want to get rid of that (since it's fairly
> > race, vblank hooks are called from all kinds of places).
> > 
> > So prep for that by adding a _reset functions which only does what we
> > really need to be done at driver load: Mark the vblank pipe as off,
> > but don't do any vblank counter saving or event flushing - neither of
> > that is required.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> >  include/drm/drmP.h                   |  1 +
> >  3 files changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 75647e7f012b..1e5fb1b994d7 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
> >  EXPORT_SYMBOL(drm_crtc_vblank_off);
> >  
> >  /**
> > + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> > + * @crtc: CRTC in question
> > + *
> > + * Drivers can use this function to reset the vblank state to off at load time.
> > + * Drivers should use this together with the drm_crtc_vblank_off() and
> > + * drm_crtc_vblank_on() functions. The diffrence comparet to
> > + * drm_crtc_vblank_off() is that this function doesn't save the vblank counter
> > + * and hence doesn't need to call any driver hooks.
> > + */
> > +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> > +{
> > +	struct drm_device *dev = drm_crtc->dev;
> > +	unsigned long irqflags;
> > +	int crtc = drm_crtc_index(drm_crtc);
> > +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> > +
> > +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > +	/*
> > +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> > +	 * interrupt by bumping the refcount.
> > +	 */
> > +	if (!vblank->inmodeset) {
> > +		atomic_inc(&vblank->refcount);
> > +		vblank->inmodeset = 1;
> > +	}
> > +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > +
> > +	WARN_ON(!list_empty(&dev->vblank_event_list));
> > +}
> > +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> > +
> > +/**
> >   * drm_vblank_on - enable vblank events on a CRTC
> >   * @dev: DRM device
> >   * @crtc: CRTC in question
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 423ef959264d..f8871a184747 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  	/* restore vblank interrupts to correct state */
> >  	if (crtc->active) {
> >  		update_scanline_offset(crtc);
> > -		drm_vblank_on(dev, crtc->pipe);
> > +		drm_crtc_vblank_on(&crtc->base);
> >  	} else
> > -		drm_vblank_off(dev, crtc->pipe);
> > +		drm_crtc_vblank_reset(&crtc->base);
> 
> Since DRM_IOCTL_WAIT_VBLANK is an unlocked ioctl it could trigger the
> WARN in drm_crtc_vblank_reset() if the ioctl is called during driver
> loading. I know it's a corner case and that probably other ioctls are
> already broken in this regard, but we could try not to make things
> worse. One way to that would be to call drm_crtc_vblank_reset()
> unconditionally as Ville suggested, but before enabling irqs.

You can't open the drm file until driver load completes, drm_global_mutex
ensures that. Which is totally not how it's supposed to be done (correct
way is to delay registering the dev node until it's all loaded), but until
we've completely ripped out UMS we can't switch over.

Unconditionally calling vblank_reset would break all the other drivers not
using vblank_on/off yet. And I don't want to audit all of them ...
-Daniel

> 
> >  
> >  	/* We need to sanitize the plane -> pipe mapping first because this will
> >  	 * disable the crtc (and hence change the state) if it is wrong. Note
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index e928625a9da0..54c6ea1e5866 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
> >  extern void drm_vblank_off(struct drm_device *dev, int crtc);
> >  extern void drm_vblank_on(struct drm_device *dev, int crtc);
> >  extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
> > +extern void drm_crtc_vblank_reset(struct drm_crtc *crtc);
> >  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >  extern void drm_vblank_cleanup(struct drm_device *dev);
> >  
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch