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@plane-immutable-zpos' to
> > > fail on AMDGPU hardware.
> > >
> > > Cc: Joshua Ashton <joshua@xxxxxxxxx>
> > > Signed-off-by: Nicholas Choi <Nicholas.Choi@xxxxxxx>
> > > ---
> > > 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
>
> 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
> > >
Attachment:
signature.asc
Description: PGP signature
