Re: [v8 14/15] drm/i915/color: Add 3D LUT to color pipeline

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

On Wed, Dec 03, 2025 at 02:22:10PM +0530, Uma Shankar wrote:
> From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> 
> Add helpers to program the 3D LUT registers and arm them.
> 
> LUT_3D_READY in LUT_3D_CLT is cleared off by the HW once
> the LUT buffer is loaded into it's internal working RAM.
> So by the time we try to load/commit new values, we expect
> it to be cleared off. If not, log an error and return
> without writing new values. Do it only when writing with MMIO.
> There is no way to read register within DSB execution.
> 
> v2:
> - Add information regarding LUT_3D_READY to commit message (Jani)
> - Log error instead of a drm_warn and return without committing changes
>   if 3DLUT HW is not ready to accept new values.
> - Refactor intel_color_crtc_has_3dlut()
>   Also remove Gen10 check (Suraj)
> v3:
> - Addressed review comments (Suraj)
> 
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c    | 78 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_color.h    |  4 +
>  .../drm/i915/display/intel_color_pipeline.c   | 29 +++++--
>  .../drm/i915/display/intel_color_pipeline.h   |  3 +-
>  .../drm/i915/display/intel_display_limits.h   |  1 +
>  .../drm/i915/display/intel_display_types.h    |  2 +-
>  drivers/gpu/drm/i915/display/intel_plane.c    |  2 +
>  7 files changed, 112 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 08f3b5b47b8e..e7950655434b 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -4062,6 +4062,52 @@ xelpd_plane_load_luts(struct intel_dsb *dsb, const struct intel_plane_state *pla
>  		xelpd_program_plane_post_csc_lut(dsb, plane_state);
>  }
>  
> +static u32 glk_3dlut_10(const struct drm_color_lut32 *color)
> +{
> +	return REG_FIELD_PREP(LUT_3D_DATA_RED_MASK, drm_color_lut32_extract(color->red, 10)) |
> +		REG_FIELD_PREP(LUT_3D_DATA_GREEN_MASK, drm_color_lut32_extract(color->green, 10)) |
> +		REG_FIELD_PREP(LUT_3D_DATA_BLUE_MASK, drm_color_lut32_extract(color->blue, 10));
> +}
> +
> +static void glk_load_lut_3d(struct intel_dsb *dsb,
> +			    struct intel_crtc *crtc,
> +			    const struct drm_property_blob *blob)
> +{
> +	struct intel_display *display = to_intel_display(crtc->base.dev);
> +	const struct drm_color_lut32 *lut = blob->data;
> +	int i, lut_size = drm_color_lut32_size(blob);
> +	enum pipe pipe = crtc->pipe;
> +
> +	if (!dsb && intel_de_read(display, LUT_3D_CTL(pipe)) & LUT_3D_READY) {
> +		drm_err(display->drm, "[CRTC:%d:%s] 3D LUT not ready, not loading LUTs\n",
> +			crtc->base.base.id, crtc->base.name);
> +		return;

Just ran into this while perusing the code...

This check could be implemented exactly like intel_vrr_check_push_sent()
so that it works for both the DSB and non-DSB paths. The 'return' should
just get nuked IMO.

> +void intel_color_plane_commit_arm(struct intel_dsb *dsb,
> +				  const struct intel_plane_state *plane_state)
> +{
> +	struct intel_display *display = to_intel_display(plane_state);
> +	struct intel_crtc *crtc = to_intel_crtc(plane_state->uapi.crtc);
> +
> +	if (crtc && intel_color_crtc_has_3dlut(display, crtc->pipe))
> +		glk_lut_3d_commit(dsb, crtc, !!plane_state->hw.lut_3d);
                                              ^^^^^^^^^^^^

And this looks like a pretty major fail. Why is the 3D LUT stored in
the *plane* state when it's a pipe level thing?

-- 
Ville Syrjälä
Intel