Re: [PATCH V10 43/46] drm/amd/display: add 3D LUT colorop

[Alex Hung <alex.hung@amd.com>]

On 8/4/25 13:51, Nícolas F. R. A. Prado wrote:
>> +/* __set_colorop_3dlut - set DRM 3D LUT to DC stream
>> + * @drm_lut3d: user 3D LUT
>> + * @drm_lut3d_size: size of 3D LUT
>> + * @lut3d: DC 3D LUT
>> + *
>> + * Map user 3D LUT data to DC 3D LUT and all necessary bits to
>> program it
>> + * on DCN accordingly.
>> + */
>> +static void __set_colorop_3dlut(const struct drm_color_lut_32
>> *drm_lut3d,
>> +				uint32_t drm_lut3d_size,
>> +				struct dc_3dlut *lut)
>> +{
>> +	if (!drm_lut3d_size) {
>> +		lut->state.bits.initialized = 0;
>> +		return;
>> +	}
> IIUC this means that setting a 3D LUT colorop with BYPASS=0 but not
> passing in a DATA property will result in the 3D LUT being bypassed.
> Meanwhile, in __set_dm_plane_colorop_3x4_matrix() in patch 36
> "drm/amd/display: add 3x4 matrix colorop", when DATA is not set, an
> error code will be bubbled up to atomic_check.
> 
> Given that this API is aimed at being prescriptive, I would expect the
> second case, bubbling up an error to atomic_check, to happen whenever a
> required DATA property is omitted, for all of the colorop types.
> 

Thanks for pointing this out. I will make the following changes to fix it.

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 3f7461cbaccb..d6434472f486 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1626,14 +1626,17 @@ __set_dm_plane_colorop_shaper(struct 
drm_plane_state *plane_state,
   *
   * Map user 3D LUT data to DC 3D LUT and all necessary bits to program it
   * on DCN accordingly.
+ *
+ * Returns:
+ * 0 on success. -EINVAL if drm_lut3d_size is zero.
   */
-static void __set_colorop_3dlut(const struct drm_color_lut32 *drm_lut3d,
+static int __set_colorop_3dlut(const struct drm_color_lut32 *drm_lut3d,
  				uint32_t drm_lut3d_size,
  				struct dc_3dlut *lut)
  {
  	if (!drm_lut3d_size) {
  		lut->state.bits.initialized = 0;
-		return;
+		return -EINVAL;
  	}

  	/* Only supports 17x17x17 3D LUT (12-bit) now */
@@ -1644,6 +1647,7 @@ static void __set_colorop_3dlut(const struct 
drm_color_lut32 *drm_lut3d,
  	__drm_3dlut32_to_dc_3dlut(drm_lut3d, drm_lut3d_size, &lut->lut_3d,
  				   lut->lut_3d.use_tetrahedral_9, 12);

+	return 0;
  }

  static int
@@ -1680,7 +1684,12 @@ __set_dm_plane_colorop_3dlut(struct 
drm_plane_state *plane_state,
  		drm_dbg(dev, "3D LUT colorop with ID: %d\n", colorop->base.id);
  		lut3d = __extract_blob_lut32(colorop_state->data, &lut3d_size);
  		lut3d_size = lut3d != NULL ? lut3d_size : 0;
-		__set_colorop_3dlut(lut3d, lut3d_size, &dc_plane_state->lut3d_func);
+		ret = __set_colorop_3dlut(lut3d, lut3d_size, 
&dc_plane_state->lut3d_func);
+		if (ret) {
+			drm_dbg(dev, "3D LUT colorop with ID: %d has LUT size = %d\n",
+				colorop->base.id, lut3d_size);
+			return ret;
+		}

  		/* 3D LUT requires shaper. If shaper colorop is bypassed, enable 
shaper curve
  		 * with TRANSFER_FUNCTION_LINEAR



> This makes me think it would be good to have a colorop validator helper
> function that could be called from the driver's atomic_check to easily
> do all such checks, such as that DATA is supplied when expected, not
> only to remove the burden on every driver to check this, but also to
> ensure consistency across them all.

I think this is not a critical feature now, and let's have this as a 
future improvement.