Re: [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping

[sashiko-bot@kernel.org]

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 <harry.wentland@amd.com>

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/20260722134607.136293-1-harry.wentland@amd.com?part=7