[Intel-gfx] [PATCH v2 12/19] drm/i915/dsb: Load LUTs using the DSB during vblank

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, June 7, 2023 12:45 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 12/19] drm/i915/dsb: Load LUTs using the DSB
> during vblank
> 
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Loading LUTs with the DSB outside of vblank doesn't really work due to the
> palette anti-collision logic. Apparently the DSB register writes don't get stalled
> like CPU mmio writes do and instead we end up corrupting the LUT entries.
> Disabling the anti-collision logic would allow us to successfully load the LUT
> outside of vblank, but presumably that risks the LUT reads from the scanout
> (temportarily) getting corrupted data from the LUT instead.

Nit: Typo in temporarily.
> 
> The anti-collision logic isn't active during vblank so that is when we can
> successfully load the LUT with the DSB. That is what we want to do anyway to
> avoid tearing.

Doing in vblank should be good, only case I see where we have to be watchful is the
HRR (high refresh rate) cases. We need to be sure, through DSB we will be able to get
this in time, it needs to be fast enough to fit the programming window, else we may
have to have some fallback to MMIO and do in active. Ideally it should work out with
DSB execution, just something to be mindful of.

Change 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_color.c   | 30 ++++++++++++++++----
>  drivers/gpu/drm/i915/display/intel_color.h   |  2 ++
>  drivers/gpu/drm/i915/display/intel_crtc.c    |  4 ++-
>  drivers/gpu/drm/i915/display/intel_display.c |  3 ++
>  4 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 2db9d1d6dadd..077e45372dab 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1660,12 +1660,6 @@ static void icl_load_luts(const struct intel_crtc_state
> *crtc_state)
>  		MISSING_CASE(crtc_state->gamma_mode);
>  		break;
>  	}
> -
> -	if (crtc_state->dsb) {
> -		intel_dsb_finish(crtc_state->dsb);
> -		intel_dsb_commit(crtc_state->dsb, false);
> -		intel_dsb_wait(crtc_state->dsb);
> -	}
>  }
> 
>  static void vlv_load_luts(const struct intel_crtc_state *crtc_state) @@ -1772,6
> +1766,9 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
> {
>  	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> 
> +	if (crtc_state->dsb)
> +		return;
> +
>  	i915->display.funcs.color->load_luts(crtc_state);
>  }
> 
> @@ -1788,6 +1785,9 @@ void intel_color_commit_arm(const struct
> intel_crtc_state *crtc_state)
>  	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> 
>  	i915->display.funcs.color->color_commit_arm(crtc_state);
> +
> +	if (crtc_state->dsb)
> +		intel_dsb_commit(crtc_state->dsb, true);
>  }
> 
>  void intel_color_post_update(const struct intel_crtc_state *crtc_state) @@ -
> 1801,6 +1801,7 @@ void intel_color_post_update(const struct intel_crtc_state
> *crtc_state)  void intel_color_prepare_commit(struct intel_crtc_state
> *crtc_state)  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> 
>  	/* FIXME DSB has issues loading LUTs, disable it for now */
>  	return;
> @@ -1813,6 +1814,12 @@ void intel_color_prepare_commit(struct
> intel_crtc_state *crtc_state)
>  		return;
> 
>  	crtc_state->dsb = intel_dsb_prepare(crtc, 1024);
> +	if (!crtc_state->dsb)
> +		return;
> +
> +	i915->display.funcs.color->load_luts(crtc_state);
> +
> +	intel_dsb_finish(crtc_state->dsb);
>  }
> 
>  void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state) @@ -
> 1824,6 +1831,17 @@ void intel_color_cleanup_commit(struct intel_crtc_state
> *crtc_state)
>  	crtc_state->dsb = NULL;
>  }
> 
> +void intel_color_wait_commit(const struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->dsb)
> +		intel_dsb_wait(crtc_state->dsb);
> +}
> +
> +bool intel_color_uses_dsb(const struct intel_crtc_state *crtc_state) {
> +	return crtc_state->dsb;
> +}
> +
>  static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
> {
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_color.h
> b/drivers/gpu/drm/i915/display/intel_color.h
> index 8002492be709..8ecd36149def 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.h
> +++ b/drivers/gpu/drm/i915/display/intel_color.h
> @@ -19,6 +19,8 @@ void intel_color_crtc_init(struct intel_crtc *crtc);  int
> intel_color_check(struct intel_crtc_state *crtc_state);  void
> intel_color_prepare_commit(struct intel_crtc_state *crtc_state);  void
> intel_color_cleanup_commit(struct intel_crtc_state *crtc_state);
> +bool intel_color_uses_dsb(const struct intel_crtc_state *crtc_state);
> +void intel_color_wait_commit(const struct intel_crtc_state
> +*crtc_state);
>  void intel_color_commit_noarm(const struct intel_crtc_state *crtc_state);  void
> intel_color_commit_arm(const struct intel_crtc_state *crtc_state);  void
> intel_color_post_update(const struct intel_crtc_state *crtc_state); diff --git
> a/drivers/gpu/drm/i915/display/intel_crtc.c
> b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 182c6dd64f47..36c9b590a058 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -24,6 +24,7 @@
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
>  #include "intel_drrs.h"
> +#include "intel_dsb.h"
>  #include "intel_dsi.h"
>  #include "intel_fifo_underrun.h"
>  #include "intel_pipe_crc.h"
> @@ -394,7 +395,8 @@ static bool intel_crtc_needs_vblank_work(const struct
> intel_crtc_state *crtc_sta
>  	return crtc_state->hw.active &&
>  		!intel_crtc_needs_modeset(crtc_state) &&
>  		!crtc_state->preload_luts &&
> -		intel_crtc_needs_color_update(crtc_state);
> +		intel_crtc_needs_color_update(crtc_state) &&
> +		!intel_color_uses_dsb(crtc_state);
>  }
> 
>  static void intel_crtc_vblank_work(struct kthread_work *base) diff --git
> a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index f23dd937c27c..25d6b19e6944 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -78,6 +78,7 @@
>  #include "intel_dpll_mgr.h"
>  #include "intel_dpt.h"
>  #include "intel_drrs.h"
> +#include "intel_dsb.h"
>  #include "intel_dsi.h"
>  #include "intel_dvo.h"
>  #include "intel_fb.h"
> @@ -7056,6 +7057,8 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		if (new_crtc_state->do_async_flip)
>  			intel_crtc_disable_flip_done(state, crtc);
> +
> +		intel_color_wait_commit(new_crtc_state);
>  	}
> 
>  	/*
> --
> 2.39.3