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 <[email protected]> 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/[email protected]?part=6
