| Message ID | 20200901010924.235808-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/display: Ignore IGNORE_PSR2_HW_TRACKING for platforms without sel fetch | expand |
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. > return; > > crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE | > -- > 2.28.0
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 > >
On Mon, Sep 14, 2020 at 07:57:34PM +0000, Souza, Jose wrote: > 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. I see it getting called from intel_crtc_atomic_check(). Confused. Am I missing some other patches? > > > > > > return; > > > > > > crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE | > > > -- > > > 2.28.0 > > > >
On Mon, 2020-09-14 at 23:30 +0300, Ville Syrjälä wrote: > On Mon, Sep 14, 2020 at 07:57:34PM +0000, Souza, Jose wrote: > > 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. > > I see it getting called from intel_crtc_atomic_check(). Confused. > Am I missing some other patches? It is set from intel_psr_disable(), intel_psr_enable() and intel_psr_update() all executed before intel_psr2_sel_fetch_update() intel_enable_ddi() intel_enable_ddi_dp() intel_psr_enable() intel_update_crtc() { if (!modeset) { intel_encoders_update_pipe() encoder->update_pipe() / intel_ddi_update_pipe() intel_ddi_update_pipe_dp() intel_psr_update() } ... skl_update_planes_on_crtc(state, crtc); intel_update_plane() plane->update_plane() / skl_update_plane() skl_program_plane() intel_psr2_sel_fetch_update() } > > > > > return; > > > > > > > > crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE | > > > > -- > > > > 2.28.0 > > > > > > > >
On Mon, Sep 14, 2020 at 08:56:12PM +0000, Souza, Jose wrote: > On Mon, 2020-09-14 at 23:30 +0300, Ville Syrjälä wrote: > > On Mon, Sep 14, 2020 at 07:57:34PM +0000, Souza, Jose wrote: > > > 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. > > > > I see it getting called from intel_crtc_atomic_check(). Confused. > > Am I missing some other patches? > > It is set from intel_psr_disable(), intel_psr_enable() and intel_psr_update() all executed before intel_psr2_sel_fetch_update() > > intel_enable_ddi() > intel_enable_ddi_dp() > intel_psr_enable() > > intel_update_crtc() { > if (!modeset) { > intel_encoders_update_pipe() > encoder->update_pipe() / intel_ddi_update_pipe() > intel_ddi_update_pipe_dp() > intel_psr_update() > } > > ... > > skl_update_planes_on_crtc(state, crtc); > intel_update_plane() > plane->update_plane() / skl_update_plane() > skl_program_plane() > intel_psr2_sel_fetch_update() That's not what I see at all. The only caller I see is intel_crtc_atomic_check(). > } > > > > > > > > > return; > > > > > > > > > > crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE | And if it would be called from there then this part would be kinda bad. We should not mutate the state during the commit phase. > > > > > -- > > > > > 2.28.0 > > > > > > > > > > > >
On Tue, 2020-09-15 at 14:50 +0300, Ville Syrjälä wrote: > On Mon, Sep 14, 2020 at 08:56:12PM +0000, Souza, Jose wrote: > > On Mon, 2020-09-14 at 23:30 +0300, Ville Syrjälä wrote: > > > On Mon, Sep 14, 2020 at 07:57:34PM +0000, Souza, Jose wrote: > > > > 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. > > > > > > I see it getting called from intel_crtc_atomic_check(). Confused. > > > Am I missing some other patches? > > > > It is set from intel_psr_disable(), intel_psr_enable() and intel_psr_update() all executed before intel_psr2_sel_fetch_update() > > > > intel_enable_ddi() > > intel_enable_ddi_dp() > > intel_psr_enable() > > > > intel_update_crtc() { > > if (!modeset) { > > intel_encoders_update_pipe() > > encoder->update_pipe() / intel_ddi_update_pipe() > > intel_ddi_update_pipe_dp() > > intel_psr_update() > > } > > > > ... > > > > skl_update_planes_on_crtc(state, crtc); > > intel_update_plane() > > plane->update_plane() / skl_update_plane() > > skl_program_plane() > > intel_psr2_sel_fetch_update() > > That's not what I see at all. The only caller I see is > intel_crtc_atomic_check(). Messed it with program function. Yeah this will not work, so just fixed the has_psr and has_psr2 to match real state, just sent the new patch. Thanks for the comments. > > > > } > > > > > > > > > > return; > > > > > > > > > > > > crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE | > > And if it would be called from there then this part would be > kinda bad. We should not mutate the state during the commit phase. > > > > > > > -- > > > > > > 2.28.0 > > > > > > > > > > > > > > > > > >
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) return; crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE |
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(-)