On Wed, 2016-06-15 at 21:44 +0000, Vivi, Rodrigo wrote:
> 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@xxxxxxxxx
> > > > >
> > > > ---
> > > > 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@xxxxxxxxx>
>
> 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
> >
Thanks for the review Rodrigo.
Unfortunately, I found some IGT failures because of race conditions, I
will fix that and send another version.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
