* [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks
@ 2021-02-05 16:45 Simon Ser
2021-02-05 17:34 ` Ilia Mirkin
0 siblings, 1 reply; 4+ messages in thread
From: Simon Ser @ 2021-02-05 16:45 UTC (permalink / raw)
To: nouveau; +Cc: Ben Skeggs
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks 2021-02-05 16:45 [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks Simon Ser @ 2021-02-05 17:34 ` Ilia Mirkin 2021-02-05 19:32 ` Lyude Paul 2021-02-05 21:00 ` Simon Ser 0 siblings, 2 replies; 4+ messages in thread From: Ilia Mirkin @ 2021-02-05 17:34 UTC (permalink / raw) To: Simon Ser; +Cc: nouveau, Ben Skeggs 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks 2021-02-05 17:34 ` Ilia Mirkin @ 2021-02-05 19:32 ` Lyude Paul 2021-02-05 21:00 ` Simon Ser 1 sibling, 0 replies; 4+ messages in thread From: Lyude Paul @ 2021-02-05 19:32 UTC (permalink / raw) To: Ilia Mirkin, Simon Ser; +Cc: nouveau, Ben Skeggs 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks 2021-02-05 17:34 ` Ilia Mirkin 2021-02-05 19:32 ` Lyude Paul @ 2021-02-05 21:00 ` Simon Ser 1 sibling, 0 replies; 4+ messages in thread From: Simon Ser @ 2021-02-05 21:00 UTC (permalink / raw) To: Ilia Mirkin; +Cc: nouveau, Ben Skeggs 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-05 21:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-05 16:45 [Nouveau] [PATCH] nouveau/dispnv50: add cursor size/pitch checks Simon Ser 2021-02-05 17:34 ` Ilia Mirkin 2021-02-05 19:32 ` Lyude Paul 2021-02-05 21:00 ` Simon Ser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).