Re: [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix state of PSR2 sub features

From: "Souza, Jose" <jose.souza@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix state of PSR2 sub features
Date: Mon, 14 Sep 2020 19:57:34 +0000	[thread overview]
Message-ID: <f0b80d8f63f2b69ff6694b5a6bf55b7f7fd032b2.camel@intel.com> (raw)
In-Reply-To: <20200914142406.GN6112@intel.com>

On Mon, 2020-09-14 at 17:24 +0300, Ville Syrjälä wrote:
> On Mon, Aug 31, 2020 at 06:09:22PM -0700, José Roberto de Souza wrote:
> > In case PSR2 is disabled by debugfs dc3co_enabled and
> > psr2_sel_fetch_enabled were still being set causing some code paths
> > to be executed were it should not.
> > We have tests for PSR1 and PSR2 so keep those features disabled when
> > PSR1 is active but PSR2 is supported is important.
> > 
> > Cc: Gwan-gyeong Mun <
> > gwan-gyeong.mun@intel.com
> > >
> > Cc: Ville Syrjälä <
> > ville.syrjala@linux.intel.com
> > >
> > Signed-off-by: José Roberto de Souza <
> > jose.souza@intel.com
> > >
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 4e09ae61d4aa..6698d0209879 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -962,12 +962,14 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
> >  	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> >  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->uapi.crtc)->pipe;
> > -	dev_priv->psr.dc3co_enabled = !!crtc_state->dc3co_exitline;
> > +	dev_priv->psr.dc3co_enabled = !!crtc_state->dc3co_exitline &&
> > +				      dev_priv->psr.psr2_enabled;
> >  	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> >  	/* DC5/DC6 requires at least 6 idle frames */
> >  	val = usecs_to_jiffies(intel_get_frame_time_us(crtc_state) * 6);
> >  	dev_priv->psr.dc3co_exit_delay = val;
> > -	dev_priv->psr.psr2_sel_fetch_enabled = crtc_state->enable_psr2_sel_fetch;
> > +	dev_priv->psr.psr2_sel_fetch_enabled = crtc_state->enable_psr2_sel_fetch &&
> > +					       dev_priv->psr.psr2_enabled;
> >  
> >  	/*
> >  	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
> > @@ -1178,7 +1180,7 @@ void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_st
> >  	struct i915_psr *psr = &dev_priv->psr;
> >  
> >  	if (!HAS_PSR2_SEL_FETCH(dev_priv) ||
> > -	    !crtc_state->enable_psr2_sel_fetch)
> > +	    !dev_priv->psr.psr2_sel_fetch_enabled)
> >  		return;
> >  
> >  	intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(psr->transcoder),
> > @@ -1189,8 +1191,9 @@ void intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> >  				 struct intel_crtc *crtc)
> >  {
> >  	struct intel_crtc_state *crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  
> > -	if (!crtc_state->enable_psr2_sel_fetch)
> > +	if (!dev_priv->psr.psr2_sel_fetch_enabled)
> 
> This looks rather sketchy. AFAICS this gets called during atomic_check()
> so looking at stuff outside the crtc state is very suspicious.

This is called after the functions that change the PSR state so no issues, also we can't really on information in CRTC state, as PSR is only enabled
if supported by state, i915 PSR parameter and PSR debug fs value.

> 
> >  		return;
> >  
> >  	crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE |
> > -- 
> > 2.28.0
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx