[PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Imre Deak]

On pe, 2015-02-13 at 08:44 +0100, Daniel Vetter wrote:
> 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.

Oops, I didn't think about checking open too. What I wrote above can be
ignored then.

> 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.

Agreed. Also driver unloading should be fixed by doing the reverse. 

> 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 ...

I meant doing that from intel code; I think it would be more logical as
others pointed out. But the above is functionally correct so this is:
Reviewed-by: Imre Deak <imre.deak at intel.com>

> -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);
> > >  
> > 
> > 
>