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

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

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?

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

-- 
Ville Syrjälä
Intel