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-15 at 01:02 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote:
> > 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 calls intel_psr_exit if PSR is already active.

oh it is already there indeed...
i'm crazy, sorry...

I liked the solution... really simple and straightforward... so simple
that confused me hehe

> 
> > > +	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_work gets rescheduled when vblanks are disabled in
> vlv_psr_src_timing_get
> 
> > >  
> > >  	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 /**/
> > 
> I will fix this and send a new version.

with this fix feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Also I believe with this series merge we would be good to re-enable psr
on VLV/CHV...

more random thoughts below... 

> > > +	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,

here I wondered it would be better to call the vlv_psr_activate
directly, but the work function handle some restrictions so your
solution is better...

> +                                             
> > >  msecs_to_jiffies(100));

just asking myself yet again if we should base this timeout in vblank
time...

one way or another maybe it is good to save it in psr struct to keep it
consistent in case we need to change someday...  but not a requirement
at all, just a random thought...

Thanks,
Rodrigo.

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