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

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Fri, 2025-01-24 at 14:32 +0200, Ville Syrjälä wrote:
> 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_sta
> > > > > te,
> > > > > 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.

Ok, based on this I will drop patches 8 and 9.

BR,

Jouni Högander

> 
> > > 
> > > 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.
>