Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The newly added Fixed Matrix (FM) colorop is unhandled by the AMD atomic check/commit logic, breaking the entire AMD color pipeline feature. --
--- Patch [6]: [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline --- commit d87e1fc27fd29f8b37f2f841d69915be8eea3f32 Author: Harry Wentland <[email protected]> drm/amd/display: Add fixed matrix colorop to color pipeline This commit inserts a fixed matrix colorop as the first operation in the amdgpu color pipeline, before the DEGAM 1D curve. This enables userspace to select YUV-to-RGB conversion via the color pipeline for YCbCr framebuffers. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c > index 056a76b88f438..1ed12cdce3172 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c [ ... ] > @@ -73,6 +81,23 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, > struct drm_plane *plane > > memset(ops, 0, sizeof(ops)); > > + /* Fixed Matrix (YUV to RGB) */ > + ops[i] = kzalloc_obj(*ops[0]); > + if (!ops[i]) { > + ret = -ENOMEM; > + goto cleanup; > + } > + > + ret = drm_plane_colorop_fixed_matrix_init(dev, ops[i], plane, > &dm_colorop_funcs, > + amdgpu_dm_supported_fm, > + > DRM_COLOROP_FLAG_ALLOW_BYPASS); > + if (ret) > + goto cleanup; > + > + list->type = ops[i]->base.id; > + > + i++; [Severity: High] Will this cause atomic checks to unconditionally fail for the color pipeline? Because the fixed matrix colorop is now the first operation in the pipeline, userspace will pass it in plane_state->color_pipeline. However, looking at amdgpu_dm_plane_set_colorop_properties(), it expects the first operation to be the DEGAM TF: amdgpu_dm_color.c:amdgpu_dm_plane_set_colorop_properties() { ... /* 1D Curve - DEGAM TF */ if (!colorop) return -EINVAL; ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state, colorop); if (ret) return ret; ... } When this passes the new fixed matrix colorop to __set_dm_plane_colorop_degamma(), it ultimately calls __set_colorop_in_tf_1d_curve() which has a strict type check: amdgpu_dm_color.c:__set_colorop_in_tf_1d_curve() { ... if (colorop->type != DRM_COLOROP_1D_CURVE) return -EINVAL; ... } Since the first colorop is now DRM_COLOROP_FIXED_MATRIX, this type check will fail and return -EINVAL. Does amdgpu_dm_plane_set_colorop_properties() need to be updated to parse and handle the new fixed matrix colorop before processing the DEGAM TF? > + > /* 1D curve - DEGAM TF */ > ops[i] = kzalloc_obj(*ops[0]); > if (!ops[i]) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
