Re: [PATCH v4 09/13] drm/i915/display: Don't use DSB if psr mode changing

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 24, 2025 at 12:16:56PM +0000, Hogander, Jouni wrote:
> On Fri, 2025-01-24 at 14:09 +0200, Hogander, Jouni wrote:
> > On Fri, 2025-01-24 at 13:53 +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 12:56:20PM +0200, Jouni Högander wrote:
> > > > Changing PSR mode using DSB is not implemented. Do not use DSB
> > > > when
> > > > PSR
> > > > mode is changing.
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 3ac1cc9ac08a8..a189aa437d972 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -7682,7 +7682,8 @@ static void intel_atomic_dsb_finish(struct
> > > > intel_atomic_state *state,
> > > >  		!new_crtc_state->scaler_state.scaler_users &&
> > > >  		!old_crtc_state->scaler_state.scaler_users &&
> > > >  		!intel_crtc_needs_modeset(new_crtc_state) &&
> > > > -		!intel_crtc_needs_fastset(new_crtc_state);
> > > > +		!intel_crtc_needs_fastset(new_crtc_state) &&
> > > > +		!intel_psr_is_psr_mode_changing(old_crtc_state,
> > > > new_crtc_state);
> > > 
> > > Hmm. Doesn't all that imply a fastset anyway
> > 
> > PSR/PR doesn't imply fastset.

You seem to be checking has_psr, has_sel_update, has_panel_replay,
and enable_psr2_su_region_et, and all of those seem to come from 
from intel_psr_compute_config() which is only called from the
modeset path. Which means it's either going to end up as a full
modeset or fastset.

> > 
> > At the time of enabling PSR/PR crtc_state->has_psr is true at this
> > point, but PSR is not yet enabled. It gets enabled only in 
> > pre_plane_update. Also in hsw_activate_psr2 and
> > dg2_panel_replay_activate we are writing PSR2_MAN_TRK_CTL.
> 
> I said it wrong here: post_plane_update I mean of course.

We don't really care whether PSR is actually active or not.
All we care about is whether it might be active. Or I suppose
technically we wouldn't even have to care about that if we just
always blasted in the extra DSB commands, but since it's easy to
avoid the extra overhead when PSR is not even possible then I
suppose it might be worthwile to check for that.

-- 
Ville Syrjälä
Intel