Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] `vkms_plane_atomic_update` unintentionally disables the fixed matrix colorop during simple page flips. - [Medium] `drm_plane_colorop_fixed_matrix_init` uses `-BIT(DRM_COLOROP_FM_COUNT)` which zero-extends on 32-bit platforms, failing to validate bits 32-63 of the `u64` `supported_fm` mask. - [Medium] `__set_dm_plane_colorop_fixed_matrix` unconditionally sets `full_update = 1`, which can unnecessarily destroy fast page flip performance. - [Low] Mismatch between commit message documentation and code implementation for bypass color space. Pre-existing issues: - [High] `__set_dm_plane_colorop_fixed_matrix` and `amdgpu_dm_update_plane_color_mgmt` silently drop the entire color pipeline on page flips. -- --- Patch [7]: [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping --- commit b2189c95d176aaf780d252979cb22287548aef19 Author: Harry Wentland <[email protected]> drm/amd/display: Implement fixed matrix colorop color space mapping Add __set_dm_plane_colorop_fixed_matrix() which maps the fixed matrix type enum to the appropriate dc_color_space for DC programming. It also updates the plane properties processing to handle the fixed matrix before degamma. > When the fixed matrix is bypassed, color_space is set to > COLOR_SPACE_UNKNOWN. [Severity: Low] This isn't a bug, but there is a mismatch between the documentation and the implementation. The commit message states COLOR_SPACE_UNKNOWN is used when bypassed, but the code sets it to COLOR_SPACE_SRGB. > 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 357c7c5c85cff..62791077ceef8 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 > @@ -1545,6 +1545,61 @@ __set_colorop_in_tf_1d_curve(struct dc_plane_state > *dc_plane_state, > } > EXPORT_IF_KUNIT(__set_colorop_in_tf_1d_curve); > > +static int > +__set_dm_plane_colorop_fixed_matrix(struct drm_plane_state *plane_state, > + struct dc_plane_state *dc_plane_state, > + struct drm_colorop *colorop) > +{ > + struct drm_colorop *old_colorop; > + struct drm_colorop_state *colorop_state = NULL, *new_colorop_state; > + struct drm_atomic_commit *state = plane_state->state; > + int i = 0; > + > + old_colorop = colorop; > + > + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) { > + if (new_colorop_state->colorop == old_colorop) { > + colorop_state = new_colorop_state; > + break; > + } > + } > + > + if (!colorop_state) > + return -EINVAL; [Severity: High] This isn't a regression introduced by this patch, but does returning -EINVAL here cause the entire hardware color pipeline configuration to be silently discarded? When userspace performs a fast page flip without modifying the color pipeline, the DRM core does not add the colorop states to the atomic commit. Since for_each_new_colorop_in_state() only searches the current transaction, it will return -EINVAL if the state is missing. Can this lead to amdgpu_dm_update_plane_color_mgmt() catching this -EINVAL and reverting to legacy DRM color properties on every simple page flip? > + > + if (colorop_state->bypass) { > + dc_plane_state->color_space = COLOR_SPACE_SRGB; > + return 0; > + } > + > + switch (colorop_state->fixed_matrix_type) { > + case DRM_COLOROP_FM_YCBCR601_FULL_RGB: > + dc_plane_state->color_space = COLOR_SPACE_YCBCR601; > + break; [ ... ] > + case DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB: > + dc_plane_state->color_space = COLOR_SPACE_2020_YCBCR_LIMITED; > + break; > + default: > + return -EINVAL; > + } > + > + dc_plane_state->update_bits.full_update = 1; > + > + return 0; > +} [Severity: Medium] Does setting full_update to 1 unconditionally here unnecessarily impact fast page flip performance? If the fixed matrix colorop is explicitly included in the atomic commit, this forces a full update even if the state didn't actually change from the previous commit. Should this only force a full update if the color_space or bypass state actively transitioned, to avoid hardware sequencer overhead and potential stuttering? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
