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

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Friday 13 February 2015 21:03:42 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.
> 
> v2: Clarify the code flow slightly as suggested by Ville.
> 
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Reviewed-by: Imre Deak <imre.deak at intel.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 |  6 +++---
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 36 insertions(+), 3 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

s/diffrence comparet/difference compared/

With that fixed,

Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> + * 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..8dcf6b512236
> 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13294,11 +13294,11 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc) I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
> 
>  	/* restore vblank interrupts to correct state */
> +	drm_crtc_vblank_reset(&crtc->base);
>  	if (crtc->active) {
>  		update_scanline_offset(crtc);
> -		drm_vblank_on(dev, crtc->pipe);
> -	} else
> -		drm_vblank_off(dev, crtc->pipe);
> +		drm_crtc_vblank_on(&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