Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Removing the `enabled` fallback leaves the shaper transfer function in a corrupted state if an invalid 1D LUT size is provided. Pre-existing issues: - [High] Atomic check silently ignores invalid colorop LUT sizes and allocation failures in the blend transfer function. -- commit 7d44b36a8843f15370d407ecab07d171e3f7d506 Author: Melissa Wen <[email protected]> drm/amd/display: make shaper bypass mode cleaner Start shaper tranfer function setup in bypass mode, i.e. tf->type == TF_TYPE_BYPASS and let the helper checks set it to a different mode according to userspace request. > 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 13ad7ac1e35e4..4651241433296 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 > @@ -1648,10 +1648,11 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state > *plane_state, > struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func; > const struct drm_color_lut32 *shaper_lut; > struct drm_device *dev = colorop->dev; > - bool enabled = false; > u32 shaper_size; > int i = 0, ret = 0; > > + tf->type = TF_TYPE_BYPASS; > + > /* 1D Curve - SHAPER TF */ > old_colorop = colorop; > for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) { [ ... ] > @@ -1702,13 +1702,9 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state > *plane_state, > ret = __set_output_tf_32(tf, shaper_lut, shaper_size, > false); > if (ret) > return ret; > - enabled = true; > } > } > > - if (!enabled) > - tf->type = TF_TYPE_BYPASS; > - > return 0; > } [Severity: High] Does removing the enabled fallback leave the shaper transfer function in a corrupted state if an invalid 1D LUT size is provided? If userspace sets bypass = false but provides an invalid LUT size or unsets the data blob (size = 0), the check for shaper_size == colorop->size fails and __set_output_tf_32() is skipped in __set_dm_plane_colorop_shaper(). Since tf->type was already set to TF_TYPE_DISTRIBUTED_POINTS earlier in the function, and we no longer fall back to TF_TYPE_BYPASS when enabled is false, the function returns 0. Could this cause the atomic commit to succeed while programming the hardware with an uninitialized transfer function? [Severity: High] This is a pre-existing issue, but a similar problem exists in __set_dm_plane_colorop_blend(). Does the atomic check silently ignore invalid colorop LUT sizes and allocation failures in the blend transfer function? drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c:__set_dm_plane_colorop_blend() { ... /* 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); } return 0; } If the size check fails, __set_input_tf_32() is skipped entirely, but the function returns 0. Also, the return value of __set_input_tf_32() is ignored, meaning allocation failures are not propagated. Could this allow the hardware to be programmed with an uninitialized or partially initialized transfer function? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
