[Intel-gfx] [PATCH] drm/i915/psr: Chase psr.enabled only under the psr.lock

[Chris Wilson]

Quoting Souza, Jose (2018-04-06 19:12:27)
> On Thu, 2018-04-05 at 12:49 +0100, Chris Wilson wrote:
> > Inside the psr work function, we want to wait for PSR to idle first
> > and
> > wish to do so without blocking the normal modeset path, so we do so
> > without holding the PSR lock. However, we first have to find which
> > pipe
> > PSR was enabled on, which requires chasing into the PSR struct and
> > requires locking to prevent intel_psr_disable() from concurrently
> > setting our pointer to NULL.
> > 
> > Fixes: 995d30477496 ("drm/i915: VLV/CHV PSR Software timer mode")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Durgadoss R <durgadoss.r at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: <stable at vger.kernel.org> # v4.0+
> 
> Feel free to add:
> Reviewed-by: Jose Roberto de Souza <jose.souza at intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 82 +++++++++++++++++++++---------
> > ----------
> >  1 file changed, 44 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2d53f7398a6d..69a5b276f4d8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -775,53 +775,59 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >       cancel_delayed_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > -static void intel_psr_work(struct work_struct *work)
> > +static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> >  {
> > -     struct drm_i915_private *dev_priv =
> > -             container_of(work, typeof(*dev_priv),
> > psr.work.work);
> > -     struct intel_dp *intel_dp = dev_priv->psr.enabled;
> > -     struct drm_crtc *crtc = dp_to_dig_port(intel_dp)-
> > >base.base.crtc;
> > -     enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > +     struct intel_dp *intel_dp;
> 
> nitpick: Why not already set it?
> struct intel_dp *intel_dp = dev_priv->psr.enabled;

I have a very strong personal preference for coupling logic together and
not splitting it up into an early init and later test. It certainly
helps against falling into the trap of using the variable inside the init
block before you test.

> > +     i915_reg_t reg;
> > +     u32 mask;
> > +     int err;
> > +
> > +     intel_dp = dev_priv->psr.enabled;
> > +     if (!intel_dp)
> > +             return false;
> >  
> > -     /* We have to make sure PSR is ready for re-enable
> > -      * otherwise it keeps disabled until next full
> > enable/disable cycle.
> > -      * PSR might take some time to get fully disabled
> > -      * and be ready for re-enable.
> > -      */
> >       if (HAS_DDI(dev_priv)) {
> 
> 
> nitpick: While on that you could replace this for:
> 
> if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) {

That I leave to others ;) Though I would suggest making the branch
around the
	VLV_PSRSTAT(pipe)
explicit as that is only applicable to a subset, and easier to extend as
we typically end up with
	if (GEN >= 10) {
	} else if (bxt_special_case_1) {
	} else if (GEN >= 8) {
	} else if (vlv_or_chv_special_case_2) {
	} else if (GEN >= 6) {
	} else {
		MISSING_CASE();
	}
or whatever is required.
-Chris