[PATCH v2] drm/nouveau/kms/nv50-: Correct size checks for cursors

[PATCH v2] drm/nouveau/kms/nv50-: Correct size checks for cursors

[Lyude Paul]

Found this while trying to make some changes to the kms_cursor_crc test.
curs507a_acquire checks that the width and height of the cursor framebuffer
are equal (asyw->image.{w,h}). This isn't entirely correct though, as the
height of the cursor can be larger than the size of the cursor, as long as
the width is the same as the cursor size and there's no framebuffer offset.

Note that I'm not entirely sure why this wasn't previously breaking
kms_cursor_crc tests - they all set up cursors with the height being one
pixel larger than the actual size of the cursor. But this seems to fix
things, and the code before was definitely incorrect - so it's not really
worth looking into further imho.

Changes since v1:
* Don't use crtc_w everywhere for determining cursor layout, just use fb
  size again
* Change check so that we only check that the w/h of the cursor plane is
  the same, the width of the scanout surface is the same as the framebuffer
  width, and that there's no offset being used for the cursor surface.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@mupuf.org>
Cc: Jeremy Cline <jcline@redhat.com>
---
 drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
index 54fbd6fe751d..00e19fd959ea 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
@@ -98,6 +98,7 @@ static int
 curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
 		 struct nv50_head_atom *asyh)
 {
+	struct nouveau_drm *drm = nouveau_drm(wndw->plane.dev);
 	struct nv50_head *head = nv50_head(asyw->state.crtc);
 	int ret;
 
@@ -109,8 +110,20 @@ curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
 	if (ret || !asyh->curs.visible)
 		return ret;
 
-	if (asyw->image.w != asyw->image.h)
+	if (asyw->state.crtc_w != asyw->state.crtc_h) {
+		NV_ATOMIC(drm, "Plane width/height must be equal for cursors\n");
 		return -EINVAL;
+	}
+
+	if (asyw->image.w != asyw->state.crtc_w) {
+		NV_ATOMIC(drm, "Plane width must be equal to fb width for cursors (height can be larger though)\n");
+		return -EINVAL;
+	}
+
+	if (asyw->state.src_x || asyw->state.src_y) {
+		NV_ATOMIC(drm, "Cursor planes do not support framebuffer offsets\n");
+		return -EINVAL;
+	}
 
 	ret = head->func->curs_layout(head, asyw, asyh);
 	if (ret)
-- 
2.29.2

Re: [PATCH v2] drm/nouveau/kms/nv50-: Correct size checks for cursors

[Ben Skeggs]

On Fri, 19 Mar 2021 at 09:04, Lyude Paul <lyude@redhat.com> wrote:
>
> Found this while trying to make some changes to the kms_cursor_crc test.
> curs507a_acquire checks that the width and height of the cursor framebuffer
> are equal (asyw->image.{w,h}). This isn't entirely correct though, as the
> height of the cursor can be larger than the size of the cursor, as long as
> the width is the same as the cursor size and there's no framebuffer offset.
>
> Note that I'm not entirely sure why this wasn't previously breaking
> kms_cursor_crc tests - they all set up cursors with the height being one
> pixel larger than the actual size of the cursor. But this seems to fix
> things, and the code before was definitely incorrect - so it's not really
> worth looking into further imho.
>
> Changes since v1:
> * Don't use crtc_w everywhere for determining cursor layout, just use fb
>   size again
> * Change check so that we only check that the w/h of the cursor plane is
>   the same, the width of the scanout surface is the same as the framebuffer
>   width, and that there's no offset being used for the cursor surface.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Martin Peres <martin.peres@mupuf.org>
> Cc: Jeremy Cline <jcline@redhat.com>
Thanks Lyude!

> ---
>  drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> index 54fbd6fe751d..00e19fd959ea 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> @@ -98,6 +98,7 @@ static int
>  curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
>                  struct nv50_head_atom *asyh)
>  {
> +       struct nouveau_drm *drm = nouveau_drm(wndw->plane.dev);
>         struct nv50_head *head = nv50_head(asyw->state.crtc);
>         int ret;
>
> @@ -109,8 +110,20 @@ curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
>         if (ret || !asyh->curs.visible)
>                 return ret;
>
> -       if (asyw->image.w != asyw->image.h)
> +       if (asyw->state.crtc_w != asyw->state.crtc_h) {
> +               NV_ATOMIC(drm, "Plane width/height must be equal for cursors\n");
>                 return -EINVAL;
> +       }
> +
> +       if (asyw->image.w != asyw->state.crtc_w) {
> +               NV_ATOMIC(drm, "Plane width must be equal to fb width for cursors (height can be larger though)\n");
> +               return -EINVAL;
> +       }
> +
> +       if (asyw->state.src_x || asyw->state.src_y) {
> +               NV_ATOMIC(drm, "Cursor planes do not support framebuffer offsets\n");
> +               return -EINVAL;
> +       }
>
>         ret = head->func->curs_layout(head, asyw, asyh);
>         if (ret)
> --
> 2.29.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel