[Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks

[Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks

[Simon Ser]

The hardware needs a FB which is packed and not too large. Add
checks to make sure this is the case.

While at it, add a debug log for the existing check. This allows
user-space to more easily figure out why a configuration is
rejected.

This commit depends on "drm/nouveau/kms/nv50-: Report max cursor
size to userspace", otherwise mode_config.cursor_{width,height}
is zero.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
---
 drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 22 ++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
index 54fbd6fe751d..9a401751c56d 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
@@ -99,6 +99,7 @@ curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
 		 struct nv50_head_atom *asyh)
 {
 	struct nv50_head *head = nv50_head(asyw->state.crtc);
+	struct drm_device *dev = head->base.base.dev;
 	int ret;
 
 	ret = drm_atomic_helper_check_plane_state(&asyw->state, &asyh->state,
@@ -109,8 +110,27 @@ 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->image.w != asyw->image.h) {
+		drm_dbg_atomic(dev,
+			       "Invalid cursor image size: width (%d) must match height (%d)",
+			       asyw->image.w, asyw->image.h);
 		return -EINVAL;
+	}
+	if (asyw->image.w > dev->mode_config.cursor_width ||
+	    asyw->image.h > dev->mode_config.cursor_height) {
+		drm_dbg_atomic(dev,
+			       "Invalid cursor image size: too large (%dx%d > %dx%d)",
+			       asyw->image.w, asyw->image.h,
+			       dev->mode_config.cursor_width,
+			       dev->mode_config.cursor_height);
+		return -EINVAL;
+	}
+	if (asyw->image.pitch[0] != asyw->image.w * 4) {
+		drm_dbg_atomic(dev,
+			       "Invalid cursor image pitch: image must be packed (pitch = %d, width = %d)",
+			       asyw->image.pitch[0], asyw->image.w);
+		return -EINVAL;
+	}
 
 	ret = head->func->curs_layout(head, asyw, asyh);
 	if (ret)
-- 
2.30.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks

[Ilia Mirkin]

On Fri, Feb 5, 2021 at 11:45 AM Simon Ser <contact@emersion.fr> wrote:
>
> The hardware needs a FB which is packed and not too large. Add
> checks to make sure this is the case.
>
> While at it, add a debug log for the existing check. This allows
> user-space to more easily figure out why a configuration is
> rejected.
>
> This commit depends on "drm/nouveau/kms/nv50-: Report max cursor
> size to userspace", otherwise mode_config.cursor_{width,height}
> is zero.
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> ---
>  drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 22 ++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> index 54fbd6fe751d..9a401751c56d 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> @@ -99,6 +99,7 @@ curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
>                  struct nv50_head_atom *asyh)
>  {
>         struct nv50_head *head = nv50_head(asyw->state.crtc);
> +       struct drm_device *dev = head->base.base.dev;
>         int ret;
>
>         ret = drm_atomic_helper_check_plane_state(&asyw->state, &asyh->state,
> @@ -109,8 +110,27 @@ 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->image.w != asyw->image.h) {
> +               drm_dbg_atomic(dev,
> +                              "Invalid cursor image size: width (%d) must match height (%d)",
> +                              asyw->image.w, asyw->image.h);
>                 return -EINVAL;
> +       }
> +       if (asyw->image.w > dev->mode_config.cursor_width ||
> +           asyw->image.h > dev->mode_config.cursor_height) {
> +               drm_dbg_atomic(dev,
> +                              "Invalid cursor image size: too large (%dx%d > %dx%d)",
> +                              asyw->image.w, asyw->image.h,
> +                              dev->mode_config.cursor_width,
> +                              dev->mode_config.cursor_height);
> +               return -EINVAL;
> +       }
> +       if (asyw->image.pitch[0] != asyw->image.w * 4) {

Rather than hard-coding to 4, make this look at the format (or cpp,
which should be available somewhere too I think). (Yeah, currently we
only expose RGBA8, but we should also be doing RGB5A1.)

> +               drm_dbg_atomic(dev,
> +                              "Invalid cursor image pitch: image must be packed (pitch = %d, width = %d)",
> +                              asyw->image.pitch[0], asyw->image.w);
> +               return -EINVAL;
> +       }
>
>         ret = head->func->curs_layout(head, asyw, asyh);

And this will fail due to the width/height not being supported, right?

  -ilia
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks

[Lyude Paul]

On Fri, 2021-02-05 at 12:34 -0500, Ilia Mirkin wrote:
> On Fri, Feb 5, 2021 at 11:45 AM Simon Ser <contact@emersion.fr> wrote:
> > 
> > The hardware needs a FB which is packed and not too large. Add
> > checks to make sure this is the case.
> > 
> > While at it, add a debug log for the existing check. This allows
> > user-space to more easily figure out why a configuration is
> > rejected.
> > 
> > This commit depends on "drm/nouveau/kms/nv50-: Report max cursor
> > size to userspace", otherwise mode_config.cursor_{width,height}
> > is zero.
> > 
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > ---
> >  drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 22 ++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> > b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> > index 54fbd6fe751d..9a401751c56d 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> > @@ -99,6 +99,7 @@ curs507a_acquire(struct nv50_wndw *wndw, struct
> > nv50_wndw_atom *asyw,
> >                  struct nv50_head_atom *asyh)
> >  {
> >         struct nv50_head *head = nv50_head(asyw->state.crtc);
> > +       struct drm_device *dev = head->base.base.dev;
> >         int ret;
> > 
> >         ret = drm_atomic_helper_check_plane_state(&asyw->state, &asyh-
> > >state,
> > @@ -109,8 +110,27 @@ 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->image.w != asyw->image.h) {
> > +               drm_dbg_atomic(dev,
> > +                              "Invalid cursor image size: width (%d) must
> > match height (%d)",
> > +                              asyw->image.w, asyw->image.h);
> >                 return -EINVAL;
> > +       }
> > +       if (asyw->image.w > dev->mode_config.cursor_width ||
> > +           asyw->image.h > dev->mode_config.cursor_height) {
> > +               drm_dbg_atomic(dev,
> > +                              "Invalid cursor image size: too large (%dx%d
> > > %dx%d)",
> > +                              asyw->image.w, asyw->image.h,
> > +                              dev->mode_config.cursor_width,
> > +                              dev->mode_config.cursor_height);
> > +               return -EINVAL;
> > +       }
> > +       if (asyw->image.pitch[0] != asyw->image.w * 4) {
> 
> Rather than hard-coding to 4, make this look at the format (or cpp,
> which should be available somewhere too I think). (Yeah, currently we
> only expose RGBA8, but we should also be doing RGB5A1.)

FWIW, required pitch for cursors (fun fact - it's actually different then the
pitch requirements for any other kind of surface used with evo!) in A1G5G5B5 is
* 2, and the * 4 you have here is correct for A8R8G8B8.

Just a note, I did actually try to enable RGB5A1 at least on pascal when working
on igt with nouveau, I'm not sure why but something appears to be broken with
that (iirc the RM throws an exception, and unfortunately I think it's one of the
few exceptions I don't actually have any secret docs for), just changing the
cursor layout isn't enough. Or maybe I screwed something silly up somewhere, but
seeing as I tried quite a few times I'd hope not :).

Anyway, the rest of this looks great

> 
> > +               drm_dbg_atomic(dev,
> > +                              "Invalid cursor image pitch: image must be
> > packed (pitch = %d, width = %d)",
> > +                              asyw->image.pitch[0], asyw->image.w);
> > +               return -EINVAL;
> > +       }
> > 
> >         ret = head->func->curs_layout(head, asyw, asyh);
> 
> And this will fail due to the width/height not being supported, right?
> 
>   -ilia
> 

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks

[Simon Ser]

On Friday, February 5th, 2021 at 6:34 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:

> > +       if (asyw->image.pitch[0] != asyw->image.w * 4) {
>
> Rather than hard-coding to 4, make this look at the format (or cpp,
> which should be available somewhere too I think). (Yeah, currently we
> only expose RGBA8, but we should also be doing RGB5A1.)

Makes sense.

> > +               drm_dbg_atomic(dev,
> > +                              "Invalid cursor image pitch: image must be packed (pitch = %d, width = %d)",
> > +                              asyw->image.pitch[0], asyw->image.w);
> > +               return -EINVAL;
> > +       }
> >
> >         ret = head->func->curs_layout(head, asyw, asyh);
>
> And this will fail due to the width/height not being supported, right?

Oh right, this function will perform size checks, and is better than the one
I added above because it actually checks that the combination is supported.
Will remove the one above in v2.

Thanks for the comments!
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau