Re: [PATCH 7/7] drm/i915/dsb: Convert dewake_scanline to a hw scanline number earlier

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 28 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we switch from out software idea of a scanline
> to the hw's idea of a scanline during the commit phase in
> _intel_dsb_commit(). While that is slightly easier due to
> fastsets fiddling with the timings, we'll also need to
> generate proper hw scanline numbers already when emitting
> DSB scanline wait instructions. So this approach won't
> do in the future. Switch to hw scanline numbers earlier.
>
> Also intel_dsb_dewake_scanline() itself already makes
> some assumptions about VRR that don't take into account
> VRR toggling during fastsets, so technically delaying
> the sw->hw conversion doesn't even help us.
>
> The other reason for delaying the conversion was that we
> are using intel_get_crtc_scanline() during intel_dsb_commit()
> which gives us the current sw scanline. But this is pretty
> low level stuff anyway so just using raw PIPEDSL reads seems
> fine here, and that of course gives us the hw scanline
> directly, reducing the need to do so many conversions.

I'll take your word for the PIPEDSL part,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c    | 16 +++++++++-------
>  drivers/gpu/drm/i915/display/intel_vblank.c |  9 ++++-----
>  drivers/gpu/drm/i915/display/intel_vblank.h |  3 ++-
>  3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 319fbebd7008..63268ed2e53f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -326,14 +326,16 @@ static int intel_dsb_dewake_scanline(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  	unsigned int latency = skl_watermark_max_latency(i915, 0);
> -	int vblank_start;
> +	int vblank_start, dewake_scanline;
>  
>  	if (crtc_state->vrr.enable)
>  		vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
>  	else
>  		vblank_start = intel_mode_vblank_start(adjusted_mode);
>  
> -	return max(0, vblank_start - intel_usecs_to_scanlines(adjusted_mode, latency));
> +	dewake_scanline = max(0, vblank_start - intel_usecs_to_scanlines(adjusted_mode, latency));
> +
> +	return intel_crtc_scanline_to_hw(crtc_state, dewake_scanline);
>  }
>  
>  static u32 dsb_chicken(struct intel_crtc *crtc)
> @@ -376,19 +378,19 @@ static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
>  			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
>  
>  	if (dewake_scanline >= 0) {
> -		int diff, hw_dewake_scanline;
> -
> -		hw_dewake_scanline = intel_crtc_scanline_to_hw(crtc, dewake_scanline);
> +		int diff, position;
>  
>  		intel_de_write_fw(dev_priv, DSB_PMCTRL(pipe, dsb->id),
>  				  DSB_ENABLE_DEWAKE |
> -				  DSB_SCANLINE_FOR_DEWAKE(hw_dewake_scanline));
> +				  DSB_SCANLINE_FOR_DEWAKE(dewake_scanline));
>  
>  		/*
>  		 * Force DEwake immediately if we're already past
>  		 * or close to racing past the target scanline.
>  		 */
> -		diff = dewake_scanline - intel_get_crtc_scanline(crtc);
> +		position = intel_de_read_fw(dev_priv, PIPEDSL(pipe)) & PIPEDSL_LINE_MASK;
> +		diff = dewake_scanline - position;
> +
>  		intel_de_write_fw(dev_priv, DSB_PMCTRL_2(pipe, dsb->id),
>  				  (diff >= 0 && diff < 5 ? DSB_FORCE_DEWAKE : 0) |
>  				  DSB_BLOCK_DEWAKE_EXTENSION);
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index eb80952b0cfd..2e3442fe5a5d 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -281,13 +281,12 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	return (position + vtotal + crtc->scanline_offset) % vtotal;
>  }
>  
> -int intel_crtc_scanline_to_hw(struct intel_crtc *crtc, int scanline)
> +int intel_crtc_scanline_to_hw(const struct intel_crtc_state *crtc_state,
> +			      int scanline)
>  {
> -	const struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(&crtc->base);
> -	const struct drm_display_mode *mode = &vblank->hwmode;
> -	int vtotal = intel_mode_vtotal(mode);
> +	int vtotal = intel_mode_vtotal(&crtc_state->hw.adjusted_mode);
>  
> -	return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> +	return (scanline + vtotal - intel_crtc_scanline_offset(crtc_state)) % vtotal;
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
> index b51ae2c1039e..b5b8bcbf0bf0 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.h
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.h
> @@ -39,6 +39,7 @@ void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);
>  void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
>  void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
>  				      bool vrr_enable);
> -int intel_crtc_scanline_to_hw(struct intel_crtc *crtc, int scanline);
> +int intel_crtc_scanline_to_hw(const struct intel_crtc_state *crtc_state,
> +			      int scanline);
>  
>  #endif /* __INTEL_VBLANK_H__ */

-- 
Jani Nikula, Intel