[PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use DSB_WAIT_FOR_VBLANK

Manna, Animesh animesh.manna at intel.com
Fri Jul 5 15:58:32 UTC 2024



> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Tuesday, June 25, 2024 12:40 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use
> DSB_WAIT_FOR_VBLANK
> 
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Allow intel_dsb_chain() to start the chained DSB
> at start of the undelaye vblank. This is slightly
> more involved than simply setting the bit as we
> must use the DEwake mechanism to eliminate pkgC
> latency.
> 
> And DSB_ENABLE_DEWAKE itself is problematic in that
> it allows us to configure just a single scanline,
> and if the current scanline is already past that
> DSB_ENABLE_DEWAKE won't do anything, rendering the
> whole thing moot.
> 
> The current workaround involves checking the pipe's current
> scanline with the CPU, and if it looks like we're about to
> miss the configured DEwake scanline we set DSB_FORCE_DEWAKE
> to immediately assert DEwake. This is somewhat racy since the
> hardware is making progress all the while we're checking it on
> the CPU.
> 
> We can make things less racy by chaining two DSBs and handling
> the DSB_FORCE_DEWAKE stuff entirely without CPU involvement:
> 1. CPU starts the first DSB immediately
> 2. First DSB configures the second DSB, including its dewake_scanline
> 3. First DSB starts the second w/ DSB_WAIT_FOR_VBLANK
> 4. First DSB asserts DSB_FORCE_DEWAKE
> 5. First DSB waits until we're outside the dewake_scanline-vblank_start
>    window
> 6. First DSB deasserts DSB_FORCE_DEWAKE
> 
> That will guarantee that the we are fully awake when the second
> DSB starts to actually execute.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 43 +++++++++++++++++++++---
>  drivers/gpu/drm/i915/display/intel_dsb.h |  3 +-
>  2 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 4c0519c41f16..cf710f0bf430 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -130,8 +130,8 @@ static int dsb_vtotal(struct intel_atomic_state *state,
>  		return intel_mode_vtotal(&crtc_state->hw.adjusted_mode);
>  }
> 
> -static int dsb_dewake_scanline(struct intel_atomic_state *state,
> -			       struct intel_crtc *crtc)
> +static int dsb_dewake_scanline_start(struct intel_atomic_state *state,
> +				     struct intel_crtc *crtc)
>  {
>  	const struct intel_crtc_state *crtc_state =
> pre_commit_crtc_state(state, crtc);
>  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> @@ -141,6 +141,14 @@ static int dsb_dewake_scanline(struct
> intel_atomic_state *state,
>  		intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode,
> latency);
>  }
> 
> +static int dsb_dewake_scanline_end(struct intel_atomic_state *state,
> +				   struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *crtc_state =
> pre_commit_crtc_state(state, crtc);
> +
> +	return intel_mode_vdisplay(&crtc_state->hw.adjusted_mode);
> +}
> +
>  static int dsb_scanline_to_hw(struct intel_atomic_state *state,
>  			      struct intel_crtc *crtc, int scanline)
>  {
> @@ -529,19 +537,44 @@ static void _intel_dsb_chain(struct
> intel_atomic_state *state,
>  			    dsb_error_int_status(display) |
> DSB_PROG_INT_STATUS |
>  			    dsb_error_int_en(display));
> 
> +	if (ctrl & DSB_WAIT_FOR_VBLANK) {
> +		int dewake_scanline = dsb_dewake_scanline_start(state,
> crtc);
> +		int hw_dewake_scanline = dsb_scanline_to_hw(state, crtc,
> dewake_scanline);
> +
> +		intel_dsb_reg_write(dsb, DSB_PMCTRL(pipe, chained_dsb-
> >id),
> +				    DSB_ENABLE_DEWAKE |
> +
> DSB_SCANLINE_FOR_DEWAKE(hw_dewake_scanline));
> +	}
> +
>  	intel_dsb_reg_write(dsb, DSB_HEAD(pipe, chained_dsb->id),
>  			    intel_dsb_buffer_ggtt_offset(&chained_dsb-
> >dsb_buf));
> 
>  	intel_dsb_reg_write(dsb, DSB_TAIL(pipe, chained_dsb->id),
>  			    intel_dsb_buffer_ggtt_offset(&chained_dsb-
> >dsb_buf) + tail);
> +
> +	if (ctrl & DSB_WAIT_FOR_VBLANK) {
> +		/*
> +		 * Keep DEwake alive via the first DSB, in
> +		 * case we're already past dewake_scanline,
> +		 * and thus DSB_ENABLE_DEWAKE on the second
> +		 * DSB won't do its job.
> +		 */
> +		intel_dsb_reg_write_masked(dsb, DSB_PMCTRL_2(pipe, dsb-
> >id),
> +					   DSB_FORCE_DEWAKE,
> DSB_FORCE_DEWAKE);
> +
> +		intel_dsb_wait_scanline_out(state, dsb,
> +					    dsb_dewake_scanline_start(state,
> crtc),
> +					    dsb_dewake_scanline_end(state,
> crtc));
> +	}
>  }
> 
>  void intel_dsb_chain(struct intel_atomic_state *state,
>  		     struct intel_dsb *dsb,
> -		     struct intel_dsb *chained_dsb)
> +		     struct intel_dsb *chained_dsb,
> +		     bool wait_for_vblank)
>  {
>  	_intel_dsb_chain(state, dsb, chained_dsb,
> -			 0);
> +			 wait_for_vblank ? DSB_WAIT_FOR_VBLANK : 0);

As per commit description and current implementation always need DSB_WAIT_FOR_VBLANK. Just wondering is there any scenario where will pass false through wait_for_vblank flag to  intel_dsb_chain()? If no can we drop the wait_for_vblank flag?

Regards,
Animesh
>  }
> 
>  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
> @@ -699,7 +732,7 @@ struct intel_dsb *intel_dsb_prepare(struct
> intel_atomic_state *state,
> 
>  	dsb->chicken = dsb_chicken(state, crtc);
>  	dsb->hw_dewake_scanline =
> -		dsb_scanline_to_hw(state, crtc, dsb_dewake_scanline(state,
> crtc));
> +		dsb_scanline_to_hw(state, crtc,
> dsb_dewake_scanline_start(state, crtc));
> 
>  	return dsb;
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h
> b/drivers/gpu/drm/i915/display/intel_dsb.h
> index e59fd7da0fc0..c352c12aa59f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -47,7 +47,8 @@ void intel_dsb_wait_scanline_out(struct
> intel_atomic_state *state,
>  				 int lower, int upper);
>  void intel_dsb_chain(struct intel_atomic_state *state,
>  		     struct intel_dsb *dsb,
> -		     struct intel_dsb *chained_dsb);
> +		     struct intel_dsb *chained_dsb,
> +		     bool wait_for_vblank);
> 
>  void intel_dsb_commit(struct intel_dsb *dsb,
>  		      bool wait_for_vblank);
> --
> 2.44.2



More information about the Intel-gfx mailing list