Re: [PATCH v3 6/7] drm/amd/display: fix bnld colorop bypass mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Silent failure and invalid hardware state when an active 1D LUT has an invalid data blob size.
- [High] The return value of `__set_input_tf_32()` is ignored, masking potential memory allocation failures.
- [Medium] The validation logic for the 1D LUT colorop incorrectly checks the `curve_1d_type` property, which is unused and logically invalid for LUTs.
--

commit 43bb295360cf4259492eaea0a93a4e70f5f5599c
Author: Melissa Wen <mwen@igalia.com>
Subject: drm/amd/display: fix bnld colorop bypass mode

If userspace set blnd colorop to bypass, AMD driver skips blnd transfer
function configuration. This patch clarifies the bypass mode setup by initially
setting the tf type to bypass.

Fixes: 52da8325996b ("drm/amd/display: Add support for sRGB EOTF in BLND block")

> 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 4651241433296..2d91d88a45742 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
> @@ -1811,6 +1811,8 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
>  	uint32_t blend_size = 0;
>  	int i = 0;
>  
> +	tf->type = TF_TYPE_BYPASS;
> +
>  	/* 1D Curve - BLND TF */

[ ... ]

> 	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_CURVE &&
> 	    (BIT(colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {
> 		drm_dbg(dev, "Blend TF colorop with ID: %d\n", colorop->base.id);
> 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
> 		tf->tf = default_tf = amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
> 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> 		__set_input_tf_32(NULL, tf, blend_lut, blend_size);

[Severity: High]
This is a pre-existing issue, but does this mask potential memory allocation
failures?

The return value of __set_input_tf_32() is ignored here, so if it returns an
error, the atomic commit proceeds with incompletely programmed hardware state.

> 	}
> 
> 	/* 1D Curve - BLND LUT */

[ ... ]

> 	if (!colorop_state->bypass && colorop->type == DRM_COLOROP_1D_LUT &&
> 	    (BIT(colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {

[Severity: Medium]
This isn't a bug introduced by this patch, but does this improperly validate
the 1D LUT state?

The validation logic incorrectly checks the curve_1d_type property, which is
unused and logically invalid for LUTs. The condition passes by coincidence
because 0 corresponds to DRM_COLOROP_1D_CURVE_SRGB_EOTF, which happens to be
supported by the BLND block.

> 		drm_dbg(dev, "Blend LUT colorop with ID: %d\n", colorop->base.id);
> 		tf->type = TF_TYPE_DISTRIBUTED_POINTS;
> 		tf->tf = default_tf;
> 		tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
> 		blend_lut = __extract_blob_lut32(colorop_state->data, &blend_size);
> 		blend_size = blend_lut != NULL ? blend_size : 0;
> 
> 		/* Custom LUT size must be the same as supported size */
> 		if (blend_size == colorop->size)
> 			__set_input_tf_32(NULL, tf, blend_lut, blend_size);

[Severity: High]
This is a pre-existing issue, but could this lead to an invalid hardware
state if the sizes do not match?

If a 1D LUT colorop is active but the user provides an improperly sized or
missing data blob, the setup function __set_input_tf_32() is skipped.

However, tf->type is already set to TF_TYPE_DISTRIBUTED_POINTS and the
function returns success (0) instead of an error. This leaves the transfer
function logically active but with uninitialized point data. A similar issue
appears to exist in __set_dm_plane_colorop_shaper() as well.

> 	}
> 
> 	return 0;
> }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=6