[PATCH v4 05/21] drm/i915/dsb: Move the DSB_PMCTRL* reset out of intel_dsb_finish()

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Monday, June 9, 2025 7:41 PM
> To: intel-gfx at lists.freedesktop.org
> Cc: intel-xe at lists.freedesktop.org
> Subject: [PATCH v4 05/21] drm/i915/dsb: Move the DSB_PMCTRL* reset out of
> intel_dsb_finish()
> 
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> When using the flip queue, due to the DMC vs. DSB register corruption problem,
> we must not issue any register writes from the DSB after unhalting the DMC.
> Currently we are doign just that by trying to restore DSB_PMCTRL* back to a

Nit: Typo in "doing"

> sane state from intel_dsb_finish().
> 
> Since the only place left that pokes at DSB_PMCTRL* is intel_dsb_chain() we can
> just do DSB_PMCTRL_2/DSB_FORCE_DEWAKE reset in the same place.
> 
> The DSB_PMCTRL reset is trickier since we'd have to do it from the chained DSB
> itself. But based on my earlier testing DSB_PMCTRL/DSB_ENABLE_DEWAKE
> doesn't actually do anything if the DSB isn't actually enabled, so we can omit the
> reset to keep things a bit simpler. We do need to reset
> DSB_PMCTRL/DSB_ENABLE_DEWAKE before starting the DSB however, in case
> it was left enabled from a previous use.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar at intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 200555a9e94b..6fdd324615e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -629,18 +629,6 @@ void intel_dsb_gosub_finish(struct intel_dsb *dsb)
> 
>  void intel_dsb_finish(struct intel_dsb *dsb)  {
> -	struct intel_crtc *crtc = dsb->crtc;
> -
> -	/*
> -	 * DSB_FORCE_DEWAKE remains active even after DSB is
> -	 * disabled, so make sure to clear it (if set during
> -	 * intel_dsb_commit()). And clear DSB_ENABLE_DEWAKE as
> -	 * well for good measure.
> -	 */
> -	intel_dsb_reg_write(dsb, DSB_PMCTRL(crtc->pipe, dsb->id), 0);
> -	intel_dsb_reg_write_masked(dsb, DSB_PMCTRL_2(crtc->pipe, dsb->id),
> -				   DSB_FORCE_DEWAKE, 0);
> -
>  	intel_dsb_align_tail(dsb);
> 
>  	intel_dsb_buffer_flush_map(&dsb->dsb_buf);
> @@ -781,6 +769,8 @@ static void _intel_dsb_chain(struct intel_atomic_state
> *state,
>  		intel_dsb_reg_write(dsb, DSB_PMCTRL(pipe, chained_dsb->id),
>  				    DSB_ENABLE_DEWAKE |
> 
> DSB_SCANLINE_FOR_DEWAKE(hw_dewake_scanline));
> +	} else {
> +		intel_dsb_reg_write(dsb, DSB_PMCTRL(pipe, chained_dsb->id),
> 0);
>  	}
> 
>  	intel_dsb_reg_write(dsb, DSB_HEAD(pipe, chained_dsb->id), @@ -802,6
> +792,13 @@ static void _intel_dsb_chain(struct intel_atomic_state *state,
>  		intel_dsb_wait_scanline_out(state, dsb,
>  					    dsb_dewake_scanline_start(state,
> crtc),
>  					    dsb_dewake_scanline_end(state,
> crtc));
> +
> +		/*
> +		 * DSB_FORCE_DEWAKE remains active even after DSB is
> +		 * disabled, so make sure to clear it.
> +		 */
> +		intel_dsb_reg_write_masked(dsb, DSB_PMCTRL_2(crtc->pipe,
> dsb->id),
> +					   DSB_FORCE_DEWAKE, 0);
>  	}
>  }
> 
> @@ -857,6 +854,8 @@ void intel_dsb_commit(struct intel_dsb *dsb)
>  			  dsb_error_int_status(display) |
> DSB_PROG_INT_STATUS |
>  			  dsb_error_int_en(display) | DSB_PROG_INT_EN);
> 
> +	intel_de_write_fw(display, DSB_PMCTRL(pipe, dsb->id), 0);
> +
>  	intel_de_write_fw(display, DSB_HEAD(pipe, dsb->id),
>  			  intel_dsb_head(dsb));
> 
> --
> 2.49.0