| Message ID | 20250124105625.822459-10-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PSR DSB support | expand |
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, and/or maybe all the PSR stuff happens in the {pre,post}_plane_update() stuff? In which case it shouldn't really matter for the stuff that the DSB does. > > if (!new_crtc_state->use_dsb && !new_crtc_state->dsb_color_vblank) > return; > -- > 2.43.0
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. 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. BR, Jouni Högander > , and/or maybe all the > PSR stuff happens in the {pre,post}_plane_update() stuff? In which > case it shouldn't really matter for the stuff that the DSB does. > > > > > if (!new_crtc_state->use_dsb && !new_crtc_state- > > >dsb_color_vblank) > > return; > > -- > > 2.43.0 >
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. > > 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. BR, Jouni Högander > > BR, > > Jouni Högander > > > , and/or maybe all the > > PSR stuff happens in the {pre,post}_plane_update() stuff? In which > > case it shouldn't really matter for the stuff that the DSB does. > > > > > > > > if (!new_crtc_state->use_dsb && !new_crtc_state- > > > > dsb_color_vblank) > > > return; > > > -- > > > 2.43.0 > > >
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.
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. >
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); if (!new_crtc_state->use_dsb && !new_crtc_state->dsb_color_vblank) return;