On Wed, Dec 03, 2025 at 02:22:10PM +0530, Uma Shankar wrote:
> From: Chaitanya Kumar Borah <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chaitanya Kumar Borah <[email protected]>
> Signed-off-by: Uma Shankar <[email protected]>
> ---
> 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
