Re: [PATCH 1/2] drm/i915/display: Add MMIO path for double-buffered LUT registers

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

On Tue, Feb 25, 2025 at 11:39:04PM +0530, Chaitanya Kumar Borah wrote:
> >From PTL, LUT registers are made double buffered. This helps us
> to program them in the active region without any concern of tearing.
> This particulary helps in case of displays with high refresh rates
> where vblank periods are shorter.
> 
> This patch makes the following changes
> 
> 	- Adds the macro HAS_DOUBLE_BUFFERED_LUT() to distinguish
> 	  platforms that have double buffered LUT registers.
> 
> 	- Program LUT values in active region through
> 	  intel_pre_update_crtc()
> 
> 	- Disable updating of LUT values during vblank.
> 
> 	- Disable pre-loading of LUT values as they are no longer
> 	  single buffered.
> 
> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c          | 4 ++++
>  drivers/gpu/drm/i915/display/intel_crtc.c           | 4 +++-
>  drivers/gpu/drm/i915/display/intel_display.c        | 6 +++++-
>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>  4 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index cfe14162231d..c3ee34b96c15 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -2022,6 +2022,10 @@ static bool intel_can_preload_luts(struct intel_atomic_state *state,
>  {
>  	const struct intel_crtc_state *old_crtc_state =
>  		intel_atomic_get_old_crtc_state(state, crtc);
> +	struct intel_display *display = to_intel_display(crtc);
> +
> +	if (HAS_DOUBLE_BUFFERED_LUT(display))
> +		return false;
>  
>  	return !old_crtc_state->post_csc_lut &&
>  		!old_crtc_state->pre_csc_lut;
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 5b2603ef2ff7..927f9acf61c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -432,10 +432,12 @@ static void intel_crtc_vblank_work(struct kthread_work *base)
>  	struct intel_crtc_state *crtc_state =
>  		container_of(work, typeof(*crtc_state), vblank_work);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct intel_display *display = to_intel_display(crtc_state);
>  
>  	trace_intel_crtc_vblank_work_start(crtc);
>  
> -	intel_color_load_luts(crtc_state);
> +	if (!HAS_DOUBLE_BUFFERED_LUT(display))

Wrong place. You don't even want to schedule the vblank worker
for this.

> +		intel_color_load_luts(crtc_state);
>  
>  	if (crtc_state->uapi.event) {
>  		spin_lock_irq(&crtc->base.dev->event_lock);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 065fdf6dbb88..919e236a9650 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6879,9 +6879,13 @@ static void intel_pre_update_crtc(struct intel_atomic_state *state,
>  
>  	if (!modeset &&
>  	    intel_crtc_needs_color_update(new_crtc_state) &&
> -	    !new_crtc_state->use_dsb)
> +	    !new_crtc_state->use_dsb) {
>  		intel_color_commit_noarm(NULL, new_crtc_state);
>  
> +		if (HAS_DOUBLE_BUFFERED_LUT(display))
> +			intel_color_load_luts(new_crtc_state);

Explanation missing on the double buffering behaviour of the LUT.
This now assumes that it's not self-arming, and therefore some
other register must be the arming register. Which register is it?

> +	}
> +
>  	if (!new_crtc_state->use_dsb)
>  		intel_crtc_planes_update_noarm(NULL, state, crtc);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index fc33791f02b9..419d0213e412 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -155,6 +155,7 @@ struct intel_display_platforms {
>  #define HAS_DMC(__display)		(DISPLAY_RUNTIME_INFO(__display)->has_dmc)
>  #define HAS_DMC_WAKELOCK(__display)	(DISPLAY_VER(__display) >= 20)
>  #define HAS_DOUBLE_BUFFERED_M_N(__display)	(DISPLAY_VER(__display) >= 9 || (__display)->platform.broadwell)
> +#define HAS_DOUBLE_BUFFERED_LUT(__display)	(DISPLAY_VER(__display) >= 30)
>  #define HAS_DOUBLE_WIDE(__display)	(DISPLAY_VER(__display) < 4)
>  #define HAS_DP_MST(__display)		(DISPLAY_INFO(__display)->has_dp_mst)
>  #define HAS_DP20(__display)		((__display)->platform.dg2 || DISPLAY_VER(__display) >= 14)
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel