Overlay issues

Overlay issues

[Cornij, Nikola]

[AMD Public Use]

Hi Simon,

Just to keep you updated, I've reproduced issue '[1] - Overlay plane: position not updated when CRTC_X is negative' on Ubuntu using IGT. Seems to happen only with smaller FBs (so far tried 24x24 and I can repro, but 300x300 is OK).

I'll look into fixing this.

Regards,

Nikola


-----Original Message-----
From: Cornij, Nikola 
Sent: Tuesday, December 8, 2020 4:11 PM
To: Simon Ser <contact@emersion.fr>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH 2/2] drm/amd/display: add cursor pitch check

[AMD Public Use]

Very good, thanks!

I'll take a look at the overlay ones, then.

-----Original Message-----
From: Simon Ser <contact@emersion.fr>
Sent: Tuesday, December 8, 2020 7:00 AM
To: Cornij, Nikola <Nikola.Cornij@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH 2/2] drm/amd/display: add cursor pitch check

Hi Nikola,

On Tuesday, December 8th, 2020 at 2:36 AM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> [AMD Public Use]
>
> Hi Simon,
>
> It looks to me I'm kinda late to the party to look at your questions 
> under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Famd-gfx%2F2020-November%2F056032.html&amp;data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=T5ijUId6nrrKK3R7c62wVllZ1rEhKdmi95Foijvjf5M%3D&amp;reserved=0...
>
> Does the commit below and
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Farchives%2Famd-gfx%2F2020-December%2F057048.html&amp;data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=6GKjptsf0S%2FWzg9CwSnp89EkaQoMxgyq9JvVcVhJIyU%3D&amp;reserved=0 mean the above issue is now on its way to resolution?

Yes, I've figured everything out about the cursor, thanks!

If you have time, another thing I need feedback about is overlay planes. I have some bug reports [1] [2] about corruption when using a small 24x24 buffer near the edges of the screen. However everything works fine with a bigger 256x256 buffer. Is there a minimum buffer size for the overlay plane?

Simon

[1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1385&amp;data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=rXhMSAySLeVbVP7Pw6vw2B0%2BnrNcKqsveeFKRYMWNhE%3D&amp;reserved=0
[2]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1386&amp;data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=gD%2FTWuBAVM8AuEfbY7c6tp%2Byns6I6hd9SihTKrO4so0%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: Overlay issues

[Simon Ser]

On Tuesday, December 15th, 2020 at 5:09 AM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> [AMD Public Use]
>
> Hi Simon,
>
> Just to keep you updated, I've reproduced issue '[1] - Overlay plane:
> position not updated when CRTC_X is negative' on Ubuntu using IGT.
> Seems to happen only with smaller FBs (so far tried 24x24 and I can
> repro, but 300x300 is OK).
>
> I'll look into fixing this.

Thanks for the update and for looking into this! Let me know if I can
help with anything. Nicholas replied on the issue tracker that overlay
planes couldn't have negative positions, so I was thinking of having a
look at the SRC_Y handling (see if we can do the same for SRC_X),
experimenting with FB sizes and SRC_W/H to figure out what's the
overlay max size still triggering the bug. If we really can't emulate
neg SRC_X we should fail atomic commits with negative SRC_X by
returning EINVAL (so that user-space can fall back to not using the
plane in this case).

Simon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Cornij, Nikola]

[AMD Public Use]

Yeah, but... it works with bigger FBs (300x300). So looks like some clipping somewhere works OK until a corner-case is reached.

-----Original Message-----
From: Simon Ser <contact@emersion.fr> 
Sent: Tuesday, December 15, 2020 2:58 AM
To: Cornij, Nikola <Nikola.Cornij@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: Overlay issues

On Tuesday, December 15th, 2020 at 5:09 AM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> [AMD Public Use]
>
> Hi Simon,
>
> Just to keep you updated, I've reproduced issue '[1] - Overlay plane:
> position not updated when CRTC_X is negative' on Ubuntu using IGT.
> Seems to happen only with smaller FBs (so far tried 24x24 and I can 
> repro, but 300x300 is OK).
>
> I'll look into fixing this.

Thanks for the update and for looking into this! Let me know if I can help with anything. Nicholas replied on the issue tracker that overlay planes couldn't have negative positions, so I was thinking of having a look at the SRC_Y handling (see if we can do the same for SRC_X), experimenting with FB sizes and SRC_W/H to figure out what's the overlay max size still triggering the bug. If we really can't emulate neg SRC_X we should fail atomic commits with negative SRC_X by returning EINVAL (so that user-space can fall back to not using the plane in this case).

Simon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Cornij, Nikola]

[AMD Public Use]

Hi Simon,

Another interim update: so far to me it looks like this is an issue if there's fewer than 24 pixels left on the screen when moving the FB outside of the left edge (e.g. with 300x300 FB size, it repros with X = -280). When this happens, what looks like a boundary condition in our driver is hit and destination rectangle update is skipped.

I need to do some more digging to fully understand why is this condition in place and how to avoid it.

Regards,

Nikola

-----Original Message-----
From: Cornij, Nikola 
Sent: Tuesday, December 15, 2020 11:37 AM
To: Simon Ser <contact@emersion.fr>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: Overlay issues

[AMD Public Use]

Yeah, but... it works with bigger FBs (300x300). So looks like some clipping somewhere works OK until a corner-case is reached.

-----Original Message-----
From: Simon Ser <contact@emersion.fr>
Sent: Tuesday, December 15, 2020 2:58 AM
To: Cornij, Nikola <Nikola.Cornij@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: Overlay issues

On Tuesday, December 15th, 2020 at 5:09 AM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> [AMD Public Use]
>
> Hi Simon,
>
> Just to keep you updated, I've reproduced issue '[1] - Overlay plane:
> position not updated when CRTC_X is negative' on Ubuntu using IGT.
> Seems to happen only with smaller FBs (so far tried 24x24 and I can 
> repro, but 300x300 is OK).
>
> I'll look into fixing this.

Thanks for the update and for looking into this! Let me know if I can help with anything. Nicholas replied on the issue tracker that overlay planes couldn't have negative positions, so I was thinking of having a look at the SRC_Y handling (see if we can do the same for SRC_X), experimenting with FB sizes and SRC_W/H to figure out what's the overlay max size still triggering the bug. If we really can't emulate neg SRC_X we should fail atomic commits with negative SRC_X by returning EINVAL (so that user-space can fall back to not using the plane in this case).

Simon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Simon Ser]

Hi,

On Wednesday, December 23rd, 2020 at 6:48 AM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> Another interim update: so far to me it looks like this is an issue if there's
> fewer than 24 pixels left on the screen when moving the FB outside of the left
> edge (e.g. with 300x300 FB size, it repros with X = -280). When this happens,
> what looks like a boundary condition in our driver is hit and destination
> rectangle update is skipped.
>
> I need to do some more digging to fully understand why is this condition in
> place and how to avoid it.

Did you have the chance to continue working on this?

Thanks,

Simon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Cornij, Nikola]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

[AMD Official Use Only - Approved for External Use]

Hi Simon,

Yes, I did have a chance to wrap this up, indeed :-)

It turned out this and other similar setup was hitting a legit HW limitation. I added a patch (please see attached) that'd fail this config at validation time. The patch has been merged for upstreaming at the beginning of February time-frame, not sure if it made it to the public repo by now.

Please let me know if you need more info on this.

Regards,

Nikola


-----Original Message-----
From: Simon Ser <contact@emersion.fr> 
Sent: Friday, February 19, 2021 4:22 PM
To: Cornij, Nikola <Nikola.Cornij@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: Overlay issues

Hi,

On Wednesday, December 23rd, 2020 at 6:48 AM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> Another interim update: so far to me it looks like this is an issue if 
> there's fewer than 24 pixels left on the screen when moving the FB 
> outside of the left edge (e.g. with 300x300 FB size, it repros with X 
> = -280). When this happens, what looks like a boundary condition in 
> our driver is hit and destination rectangle update is skipped.
>
> I need to do some more digging to fully understand why is this 
> condition in place and how to avoid it.

Did you have the chance to continue working on this?

Thanks,

Simon

[-- Attachment #2: 0001-drm-amd-display-Reject-too-small-viewport-size-when-.patch --]
[-- Type: application/octet-stream, Size: 4164 bytes --]

From ba73ebd0cdae05dcf0f6195f05f1cda6f059e9ec Mon Sep 17 00:00:00 2001
From: Nikola Cornij <nikola.cornij@amd.com>
Date: Thu, 21 Jan 2021 22:35:54 -0500
Subject: [PATCH] drm/amd/display: Reject too small viewport size when
 validating plane

[why]
Overlay won't move to a new positon if viewport size is smaller than
what can be handled. It'd either disappear or stay at the old
position. This condition is for example hit if overlay is moved too
much outside of left or top edge of the screen, but it applies to
any non-cursor plane type.

[how]
Reject this contidion at validation time. This gives the calling
level a chance to handle this gracefully and avoid inconsistent
behaivor.

Signed-off-by: Nikola Cornij <nikola.cornij@amd.com>
Reviewed-by: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
Acked-by: Anson Jacob <Anson.Jacob@amd.com>
Tested-by: Daniel Wheeler <daniel.wheeler@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 ++++++++++++++++++-
 .../gpu/drm/amd/display/dc/core/dc_resource.c |  4 +--
 drivers/gpu/drm/amd/display/dc/dc.h           |  1 +
 3 files changed, 29 insertions(+), 3 deletions(-)

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 e11139ca41d4..626a8cc92d65 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6496,8 +6496,33 @@ static int dm_plane_helper_check_state(struct drm_plane_state *state,
 	int min_scale = 0;
 	int max_scale = INT_MAX;
 
-	/* Plane enabled? Get min/max allowed scaling factors from plane caps. */
+	/* Plane enabled? Validate viewport and get scaling factors from plane caps. */
 	if (fb && state->crtc) {
+		/* Validate viewport to cover the case when only the position changes */
+		if (state->plane->type != DRM_PLANE_TYPE_CURSOR) {
+			int viewport_width = state->crtc_w;
+			int viewport_height = state->crtc_h;
+
+			if (state->crtc_x < 0)
+				viewport_width += state->crtc_x;
+			else if (state->crtc_x + state->crtc_w > new_crtc_state->mode.crtc_hdisplay)
+				viewport_width = new_crtc_state->mode.crtc_hdisplay - state->crtc_x;
+
+			if (state->crtc_y < 0)
+				viewport_height += state->crtc_y;
+			else if (state->crtc_y + state->crtc_h > new_crtc_state->mode.crtc_vdisplay)
+				viewport_height = new_crtc_state->mode.crtc_vdisplay - state->crtc_y;
+
+			/* If completely outside of screen, viewport_width and/or viewport_height will be negative,
+			 * which is still OK to satisfy the condition below, thereby also covering these cases
+			 * (when plane is completely outside of screen).
+			 * x2 for width is because of pipe-split.
+			 */
+			if (viewport_width < MIN_VIEWPORT_SIZE*2 || viewport_height < MIN_VIEWPORT_SIZE)
+				return -EINVAL;
+		}
+
+		/* Get min/max allowed scaling factors from plane caps. */
 		get_min_max_dc_plane_scaling(state->crtc->dev, fb,
 					     &min_downscale, &max_upscale);
 		/*
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
index 68b65a090d17..0c26c2ade782 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
@@ -1153,8 +1153,8 @@ bool resource_build_scaling_params(struct pipe_ctx *pipe_ctx)
 
 	calculate_viewport(pipe_ctx);
 
-	if (pipe_ctx->plane_res.scl_data.viewport.height < 12 ||
-		pipe_ctx->plane_res.scl_data.viewport.width < 12) {
+	if (pipe_ctx->plane_res.scl_data.viewport.height < MIN_VIEWPORT_SIZE ||
+		pipe_ctx->plane_res.scl_data.viewport.width < MIN_VIEWPORT_SIZE) {
 		if (store_h_border_left) {
 			restore_border_left_from_dst(pipe_ctx,
 				store_h_border_left);
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index d5105924c9e0..be9adddfac96 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -48,6 +48,7 @@
 #define MAX_PLANES 6
 #define MAX_STREAMS 6
 #define MAX_SINKS_PER_LINK 4
+#define MIN_VIEWPORT_SIZE 12
 
 /*******************************************************************************
  * Display Core Interfaces
-- 
2.25.1


[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Simon Ser]

On Monday, February 22nd, 2021 at 9:59 PM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> [AMD Official Use Only - Approved for External Use]
>
> Hi Simon,
>
> Yes, I did have a chance to wrap this up, indeed :-)
>
> It turned out this and other similar setup was hitting a legit HW
> limitation. I added a patch (please see attached) that'd fail this
> config at validation time. The patch has been merged for upstreaming
> at the beginning of February time-frame, not sure if it made it to
> the public repo by now.

Excellent! I don't see the patch in agd5f/drm-next yet, but it'll
probably show up soon.

Thanks for fixing this!

One small nit: if possible, add some drm_dbg_atomic() calls explaining
the error right before returning -EINVAL. This is very valuable when
debugging user-space applications and trying to figure out why the
kernel rejects an atomic commit.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Cornij, Nikola]

[AMD Official Use Only - Approved for External Use]

Thanks for the suggestion, Simon.

Improved the patch, now in the process of getting it reviewed internally. It'll be posted for upstream thereafter.


-----Original Message-----
From: Simon Ser <contact@emersion.fr> 
Sent: Monday, February 22, 2021 5:31 PM
To: Cornij, Nikola <Nikola.Cornij@amd.com>
Cc: Alex Deucher <alexdeucher@gmail.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: RE: Overlay issues

On Monday, February 22nd, 2021 at 9:59 PM, Cornij, Nikola <Nikola.Cornij@amd.com> wrote:

> [AMD Official Use Only - Approved for External Use]
>
> Hi Simon,
>
> Yes, I did have a chance to wrap this up, indeed :-)
>
> It turned out this and other similar setup was hitting a legit HW 
> limitation. I added a patch (please see attached) that'd fail this 
> config at validation time. The patch has been merged for upstreaming 
> at the beginning of February time-frame, not sure if it made it to the 
> public repo by now.

Excellent! I don't see the patch in agd5f/drm-next yet, but it'll probably show up soon.

Thanks for fixing this!

One small nit: if possible, add some drm_dbg_atomic() calls explaining the error right before returning -EINVAL. This is very valuable when debugging user-space applications and trying to figure out why the kernel rejects an atomic commit.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: Overlay issues

[Simon Ser]

> Improved the patch, now in the process of getting it reviewed
> internally. It'll be posted for upstream thereafter.

Thanks, perfect!
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx