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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 03, 2015 at 01:31:34PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Tuesday 03 February 2015 11:30:11 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.
> 
> (Thinking out loud)
> 
> In principle this looks good, but I find the naming pretty confusing. The 
> drm_crtc_vblank_* functions now have get, put, on, off and reset variants. The 
> fact that on is supposed to be called both at runtime and an init time while 
> off is replaced by reset at init time breaks the symmetry between on and off. 
> What would you think of a drm_crtc_vblank_init() function instead, used at 
> init time only, and that would take an enabled boolean argument ?
> 
> On the other hand, calling drm_crtc_vblank_on() at init time also makes sense, 
> as even if the CRTC is active the vblank interrupt should be off then, and an 
> explicit call to the function means "turn the vblank interrupt on". I'm thus 
> not totally opposed to keeping that as-is. Wouldn't it then be better to 
> modify the core to default to off, and let drivers call drm_crtc_vblank_on() 
> explicitly if the default isn't correct ? I think I like this solution better, 
> and it could be conditioned by a driver flag if we don't want to audit all 
> drivers for possible breakages.

Yeah, that's been my thinking with sticking with on and only replacing the
off: For _on the environment exactly matches what we do when enabling the
crtc in a modeset:
- the pipe is actually running
- we want vblanks to start working

Totally different for _off, which assumes:
- pipe is still on
- vblanks should stop running and state should be saved
- any pending
Wheras _reset just disallows vblanks.

Long-term I wonder whether a drm_crtc_vblank_init would be useful, which:
- uses dev->mode_config.num_crtcs to set up the right amount of vblank
  pipes.
- calls _reset directly
- in the future might even store the vblank state in the drm_crtc.

But before we can do that we need to split the vblank code into ums legacy
paths using int pipe and kms paths dealing in struct drm_crtc *. We're not
there yet.

> 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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
> 
> Typo: difference compared 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);
> > 
> >  	/* 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);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx