Re: [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Build Breakage / Function Prototype Mismatch
- [High] Logic Error overwriting correctly calculated color_space with SRGB during commit_tail
--

--- Patch [9]: [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail ---
commit 7fa51e59a15d532394f2e3959287098fefb48b8d
Author: Harry Wentland <harry.wentland@amd.com>

drm/amd/display: Check actual state during commit_tail

Previously we checked the plane_state->state's plane_color_pipeline
client config when determining whether fill_plane_color_attributes
should run, i.e., whether we had a plane color pipeline and should
use that code for handling fixed matrix properties, or whether we
should look at legacy plane encoding and range.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e7080880b2211..4f07fe5688262 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3008,7 +3008,8 @@ STATIC_IFN_KUNIT bool modereset_required(struct drm_crtc_state *crtc_state)
>  EXPORT_IF_KUNIT(modereset_required);
>  
>  STATIC_IFN_KUNIT int
> -fill_plane_color_attributes(const struct drm_plane_state *plane_state,
> +fill_plane_color_attributes(struct drm_atomic_commit *state,
> +			    const struct drm_plane_state *plane_state,
>  			    const enum surface_pixel_format format,
>  			    enum dc_color_space *color_space)
>  {

[Severity: High]
Will this break the build when CONFIG_DRM_AMD_DC_KUNIT_TEST is enabled?

The old prototype remains in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:

    int fill_plane_color_attributes(const struct drm_plane_state *plane_state,
                                    const enum surface_pixel_format format,
                                    enum dc_color_space *color_space);

And the KUnit tests still call the function with 3 arguments:

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c:dm_test_fill_color_attr_rgb_format() {
    ...
    ret = fill_plane_color_attributes(&plane_state,
                                      SURFACE_PIXEL_FORMAT_GRPH_ARGB8888,
                                      &color_space);
    ...
}

> @@ -3017,7 +3018,7 @@ fill_plane_color_attributes(const struct drm_plane_state *plane_state,
>  	*color_space = COLOR_SPACE_SRGB;
>  
>  	/* Ignore properties when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set */
> -	if (plane_state->state && plane_state->state->plane_color_pipeline)
> +	if (state && state->plane_color_pipeline)
>  		return 0;

[Severity: High]
Could this unintentionally overwrite the correctly calculated color_space with
COLOR_SPACE_SRGB during commit_tail?

Because *color_space is initialized to COLOR_SPACE_SRGB before the early
return, when the color pipeline is active, fill_plane_color_attributes() sets
plane_info->color_space to COLOR_SPACE_SRGB.

Later, DC core's copy_surface_update_to_plane() unconditionally copies this
value to surface->color_space:

drivers/gpu/drm/amd/display/dc/core/dc.c:copy_surface_update_to_plane() {
    ...
    if (srf_update->plane_info) {
        surface->color_space =
                srf_update->plane_info->color_space;
    ...
}

Can this clobber the fixed matrix value calculated during atomic check,
causing YUV-to-RGB conversion to be bypassed for YUV framebuffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=9