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

[Melissa Wen]

On 08/18, Leo Li wrote:
> 
> 
> On 2023-08-18 04:25, Melissa Wen wrote:
> > On 07/26, Aurabindo Pillai wrote:
> > > This reverts commit 6c8ff1683d30024c8cff137d30b740a405cc084e.
> > > 
> > > This patch causes IGT test case 'kms_atomic at plane-immutable-zpos' to
> > > fail on AMDGPU hardware.
> > > 
> > > Cc: Joshua Ashton <joshua at froggi.es>
> > > Signed-off-by: Nicholas Choi <Nicholas.Choi at 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 at 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
> 
> Hi Melissa,
> 
> Thanks for taking a look at the IGT test. Looks like the IGT failures
> are the only concerns, and I agree that it doesn't make sense to require
> zpos to be normalized between 0 and number of planes.

Hi Leo,

Thanks for the feedback.

I've also checked that msm driver creates zpos properties in a similar
way of this commit here, so I sent and applied the IGT test change:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/cb77add45011b129e21f3cb2a4089a73dde56179

With that, could you guys revert this commit reversion?

Thanks,

Melissa

> 
> Thanks,
> Leo
> 
> > 
> > > -
> > >   	if (plane->type == DRM_PLANE_TYPE_PRIMARY &&
> > >   	    plane_cap &&
> > >   	    (plane_cap->pixel_format_support.nv12 ||
> > > -- 
> > > 2.41.0
> > > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20230829/cbbcdcc5/attachment.sig>