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.
