Re: [PATCH v3 06/10] drm/i915/psr: Allow writing PSR2_MAN_TRK_CTL using DSB

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

On Fri, 2025-01-17 at 21:22 +0200, Ville Syrjälä wrote:
> On Thu, Jan 09, 2025 at 09:31:33AM +0200, Jouni Högander wrote:
> > Allow writing PSR2_MAN_TRK_CTL using DSB by using
> > intel_de_write_dsb. Do
> > not check intel_dp->psr.lock being held when using DSB. This
> > assertion
> > doesn't make sense as in case of using DSB the actual write happens
> > later
> > and we are not taking intel_dp->psr.lock mutex over dsb commit.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |  2 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c     | 16 ++++++++++------
> >  drivers/gpu/drm/i915/display/intel_psr.h     |  4 +++-
> >  3 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 4271da219b41..5a5100f147a6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7089,7 +7089,7 @@ static void commit_pipe_pre_planes(struct
> > intel_atomic_state *state,
> >  			intel_pipe_fastset(old_crtc_state,
> > new_crtc_state);
> >  	}
> >  
> > -	intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
> > +	intel_psr2_program_trans_man_trk_ctl(NULL,
> > new_crtc_state);
> >  
> >  	intel_atomic_update_watermarks(state, crtc);
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 85ecedd3162d..1e99329b70a1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -2330,7 +2330,8 @@ static void intel_psr_force_update(struct
> > intel_dp *intel_dp)
> >  	intel_de_write(display, CURSURFLIVE(display, intel_dp-
> > >psr.pipe), 0);
> >  }
> >  
> > -void intel_psr2_program_trans_man_trk_ctl(const struct
> > intel_crtc_state *crtc_state)
> > +void intel_psr2_program_trans_man_trk_ctl(struct intel_dsb *dsb,
> > +					  const struct
> > intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(crtc_state);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state-
> > >uapi.crtc);
> > @@ -2344,20 +2345,23 @@ void
> > intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state
> > *crtc_st
> >  					     crtc_state-
> > >uapi.encoder_mask) {
> >  		struct intel_dp *intel_dp =
> > enc_to_intel_dp(encoder);
> >  
> > -		lockdep_assert_held(&intel_dp->psr.lock);
> > +		if (!dsb)
> > +			lockdep_assert_held(&intel_dp->psr.lock);
> 
> The question now becomes what exactly that lock is protecting, why
> is that important for the mmio path, and how is it not an issue
> for the DSB path?

I tried to explain my thinking/idea on this in the cover letter:

"
PSR mutex is not locked when performing DSB commit. It is not
necessary as we are currently using DSB only when sending updates
towards panel. I.e. not using it when changing PSR mode. Also
necessary changes are made to use PSR2_MAN_TRK_CTL only in
DSB. Frontbuffer updates and legacy cursor updates are using SFF_CTL
register to perform full frame updates.
"

BR,

Jouni Högander

> > +
> >  		if (DISPLAY_VER(display) < 20 && intel_dp-
> > >psr.psr2_sel_fetch_cff_enabled)
> >  			return;
> >  		break;
> >  	}
> >  
> > -	intel_de_write(display, PSR2_MAN_TRK_CTL(display,
> > cpu_transcoder),
> > -		       crtc_state->psr2_man_track_ctl);
> > +	intel_de_write_dsb(display, dsb,
> > +			   PSR2_MAN_TRK_CTL(display,
> > cpu_transcoder),
> > +			   crtc_state->psr2_man_track_ctl);
> >  
> >  	if (!crtc_state->enable_psr2_su_region_et)
> >  		return;
> >  
> > -	intel_de_write(display, PIPE_SRCSZ_ERLY_TPT(crtc->pipe),
> > -		       crtc_state->pipe_srcsz_early_tpt);
> > +	intel_de_write_dsb(display, dsb, PIPE_SRCSZ_ERLY_TPT(crtc-
> > >pipe),
> > +			   crtc_state->pipe_srcsz_early_tpt);
> >  }
> >  
> >  static void psr2_man_trk_ctl_calc(struct intel_crtc_state
> > *crtc_state,
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index 956be263c09e..fc807817863e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -17,6 +17,7 @@ struct intel_crtc;
> >  struct intel_crtc_state;
> >  struct intel_display;
> >  struct intel_dp;
> > +struct intel_dsb;
> >  struct intel_encoder;
> >  struct intel_plane;
> >  struct intel_plane_state;
> > @@ -55,7 +56,8 @@ void intel_psr_wait_for_idle_locked(const struct
> > intel_crtc_state *new_crtc_stat
> >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> >  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> >  				struct intel_crtc *crtc);
> > -void intel_psr2_program_trans_man_trk_ctl(const struct
> > intel_crtc_state *crtc_state);
> > +void intel_psr2_program_trans_man_trk_ctl(struct intel_dsb *dsb,
> > +					  const struct
> > intel_crtc_state *crtc_state);
> >  void intel_psr_pause(struct intel_dp *intel_dp);
> >  void intel_psr_resume(struct intel_dp *intel_dp);
> >  bool intel_psr_needs_block_dc_vblank(const struct intel_crtc_state
> > *crtc_state);
> > -- 
> > 2.43.0
>