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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

> 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx