Re: [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote:
> PSR in CHV, unlike HSW, can get activated even if vblanks interrupts
> are
> enabled. But, the pipe is not expected to generate timings signals
> when PSR is active. Specifically, we do not get vblank interrupts in
> CHV
> if PSR becomes active. This has led to drm_wait_vblank timing out.
> 
> Let's disable PSR using the vblank prepare hook that gets called
> before
> enabling vblank interrupts and keep it disabled until the interrupts
> are
> not needed.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_psr.c | 61
> +++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e34..03f311e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -994,6 +994,7 @@ struct i915_psr {
>  	bool psr2_support;
>  	bool aux_frame_sync;
>  	bool link_standby;
> +	bool vlv_src_timing;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index caaf1e2..77f3d76 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct
> drm_device *dev, unsigned int pipe)
>  	return 0;
>  }
>  
> +static void valleyview_prepare_vblank(struct drm_device *dev,
> unsigned int pipe)
> +{

shouldn't we force psr_exit here? What if vblank is enabled after psr
is already in active mode?

> +	vlv_psr_src_timing_get(dev);
> +}
> +
> +static void valleyview_unprepare_vblank(struct drm_device *dev,
> unsigned int pipe){
> +
> +	vlv_psr_src_timing_put(dev);
> +}
> +
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private
> *dev_priv)
>  		dev->driver->irq_uninstall =
> cherryview_irq_uninstall;
>  		dev->driver->enable_vblank =
> valleyview_enable_vblank;
>  		dev->driver->disable_vblank =
> valleyview_disable_vblank;
> +		dev->driver->prepare_vblank =
> valleyview_prepare_vblank;
> +		dev->driver->unprepare_vblank =
> valleyview_unprepare_vblank;
>  		dev_priv->display.hpd_irq_setup =
> i915_hpd_irq_setup;
>  	} else if (IS_VALLEYVIEW(dev_priv)) {
>  		dev->driver->irq_handler = valleyview_irq_handler;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 9b5f663..e2078fd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev,
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>  				   unsigned frontbuffer_bits);
> +void vlv_psr_src_timing_get(struct drm_device *dev);
> +void vlv_psr_src_timing_put(struct drm_device *dev);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 29a09bf..c95e680 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  
>  		/* Enable PSR on the panel */
>  		vlv_psr_enable_sink(intel_dp);
> +		dev_priv->psr.vlv_src_timing = false;
>  
>  		/* On HSW+ enable_source also means go to PSR
> entry/active
>  		 * state as soon as idle_frame achieved and here
> would be
> @@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct
> *work)
>  	 * The delayed work can race with an invalidate hence we
> need to
>  	 * recheck. Since psr_flush first clears this and then
> reschedules we
>  	 * won't ever miss a flush when bailing out here.
> +	 * Also, do not enable PSR if source is required to generate
> timing
> +	 * signals like vblanks.
>  	 */
> -	if (dev_priv->psr.busy_frontbuffer_bits)
> +	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv
> ->psr.vlv_src_timing)
>  		goto unlock;

I wonder if in this vlv_src_timing case the work should re-schedule
itself... otherwise we have the risk of psr staying disabled forever
right?


>  
>  	intel_psr_activate(intel_dp);
> @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct
> drm_device *dev,
>  }
>  
>  /**
> + * vlv_psr_src_timing_get - src timing generation requested
> + *
> + * CHV does not have HW tracking to trigger PSR exit when VBI are
> enabled nor
> + * does enabling vblank interrupts prevent PSR entry. This function
> is called
> + * before enabling VBI to exit PSR and prevent PSR re-entry until
> vblanks are
> + * disabled again.
> + */
> +void vlv_psr_src_timing_get(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +        if (!dev_priv->psr.enabled) {
> +                mutex_unlock(&dev_priv->psr.lock);
> +                return;
> +	}
> +
> +	//Handle racing with intel_psr_work with this flag

Is this comment permanent? if so you should use /**/

> +	dev_priv->psr.vlv_src_timing = true;
> +
> +	if(dev_priv->psr.active)
> +		intel_psr_exit(dev);
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +}
> +
> +
> +/**
> + * vlv_psr_src_timing_put - src timing generation not required
> + *
> + * CHV does not have HW tracking to trigger PSR exit when VBI are
> enabled nor
> + * does enabling vblank interrupts prevent PSR entry. This function
> is called
> + * when VBI are not required and PSR can be activated.
> + */
> +void vlv_psr_src_timing_put(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (!dev_priv->psr.enabled) {
> +                mutex_unlock(&dev_priv->psr.lock);
> +                return;
> +        }
> +
> +	dev_priv->psr.vlv_src_timing = false;
> +
> +	if (!dev_priv->psr.active)
> +                if (!work_busy(&dev_priv->psr.work.work))
> +			schedule_delayed_work(&dev_priv->psr.work,
> +                                             
>  msecs_to_jiffies(100));
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_invalidate - Invalidade PSR
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx