[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 at 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 at emersion.fr>
> > Cc: Lyude Paul <lyude at redhat.com>
> > Cc: Ben Skeggs <bskeggs at redhat.com>
> > Cc: Ilia Mirkin <imirkin at 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!