Re: [PATCH 6/7] Revert "drm/amd/display: Implement zpos property"

[Melissa Wen]

On 07/26, Aurabindo Pillai wrote:
> This reverts commit 6c8ff1683d30024c8cff137d30b740a405cc084e.
> 
> This patch causes IGT test case 'kms_atomic@plane-immutable-zpos' to
> fail on AMDGPU hardware.
> 
> Cc: Joshua Ashton <jos...@froggi.es>
> Signed-off-by: Nicholas Choi <nicholas.c...@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 2198df96ed6f..8eeca160d434 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1468,15 +1468,6 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager 
> *dm,
>               drm_plane_create_blend_mode_property(plane, blend_caps);
>       }
>  
> -     if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> -             drm_plane_create_zpos_immutable_property(plane, 0);
> -     } else if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
> -             unsigned int zpos = 1 + drm_plane_index(plane);
> -             drm_plane_create_zpos_property(plane, zpos, 1, 254);
> -     } else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> -             drm_plane_create_zpos_immutable_property(plane, 255);
> -     }

Hi Jay and Nicholas,

I'm examining this regression and, looking at the test code, I consider
this failure is caused by an incorrect assumption in the IGT test in
which any zpos values must be in the normalized range of 0 and N planes
per CRTC.

        for (int k = 0; k < n_planes; k++) {
                int zpos;
                igt_plane_t *temp;

                temp = &display->pipes[pipe->pipe].planes[k];

                if (!igt_plane_has_prop(temp, IGT_PLANE_ZPOS))
                        continue;

                zpos = igt_plane_get_prop(temp, IGT_PLANE_ZPOS);

                igt_assert_lt(zpos, n_planes);  // test case fails here

                plane_ptr[zpos] = temp;
        }


I didn't find anything in the DRM documentation that imposes this
behavior. Also, the plane composition in the test is working correctly
with this patch and without this likely-misleading assert. In addition,
enabling zpos property increases the coverage of
`kms_atomic@plane-immutable-zpos` test (previously this part of the test
was just bypassed), so it's not a regression per se. Therefore, I'm
inclined to send a fix to IGT, instead of implementing a behavior that
fit the test but may not fit well AMD display machinery.

But first I wonder if you guys find any other test or visual check that
fail with this feature?

I checked other IGT KMS plane tests and AMD MPO tests (in `amd_plane`)
and results are preserved. If there are no other issues besides IGT
plane-immutable-zpos, I'll proceed with sending the change to IGT.

Thanks,

Melissa

> -
>       if (plane->type == DRM_PLANE_TYPE_PRIMARY &&
>           plane_cap &&
>           (plane_cap->pixel_format_support.nv12 ||
> -- 
> 2.41.0
> 

signature.asc
Description: PGP signature