Simon Ser
2021-Feb-05 21:00 UTC
[Nouveau] [PATCH v2] nouveau/dispnv50: add cursor pitch check
The hardware needs a FB which is packed. Add checks to make sure
this is the case.
While at it, add debug logs for the existing checks. This allows
user-space to more easily figure out why a configuration is
rejected.
v2:
- Use drm_format_info instead of hardcoding bytes-per-pixel (Ilia)
- Remove unnecessary size check (Ilia)
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 | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
index 54fbd6fe751d..00f09c9a8d15 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
@@ -30,6 +30,7 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_plane_helper.h>
+#include <drm/drm_fourcc.h>
bool
curs507a_space(struct nv50_wndw *wndw)
@@ -99,6 +100,8 @@ 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;
+ struct drm_framebuffer *fb = asyw->state.fb;
int ret;
ret = drm_atomic_helper_check_plane_state(&asyw->state,
&asyh->state,
@@ -109,12 +112,26 @@ 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.pitch[0] != asyw->image.w * fb->format->cpp[0]) {
+ 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)
+ if (ret) {
+ drm_dbg_atomic(dev,
+ "Invalid cursor image size: unsupported size %dx%d",
+ asyw->image.w, asyw->image.h);
return ret;
+ }
return head->func->curs_format(head, asyw, asyh);
}
--
2.30.0
Lyude Paul
2021-Feb-05 21:18 UTC
[Nouveau] [PATCH v2] nouveau/dispnv50: add cursor pitch check
On Fri, 2021-02-05 at 22:00 +0100, Simon Ser wrote:> The hardware needs a FB which is packed. Add checks to make sure > this is the case. > > While at it, add debug logs for the existing checks. This allows > user-space to more easily figure out why a configuration is > rejected. > > v2: > - Use drm_format_info instead of hardcoding bytes-per-pixel (Ilia) > - Remove unnecessary size check (Ilia) > > 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 | 21 +++++++++++++++++++-- > ?1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > index 54fbd6fe751d..00f09c9a8d15 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > @@ -30,6 +30,7 @@ > ? > ?#include <drm/drm_atomic_helper.h> > ?#include <drm/drm_plane_helper.h> > +#include <drm/drm_fourcc.h> > ? > ?bool > ?curs507a_space(struct nv50_wndw *wndw) > @@ -99,6 +100,8 @@ 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; > +???????struct drm_framebuffer *fb = asyw->state.fb; > ????????int ret; > ? > ????????ret = drm_atomic_helper_check_plane_state(&asyw->state, &asyh->state, > @@ -109,12 +112,26 @@ 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.pitch[0] != asyw->image.w * fb->format->cpp[0]) { > +???????????????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) > +???????if (ret) { > +???????????????drm_dbg_atomic(dev, > +????????????????????????????? "Invalid cursor image size: unsupported size > %dx%d", > +????????????????????????????? asyw->image.w, asyw->image.h); > ????????????????return ret; > +???????}Oh-some things I forgot to mention: - You're missing some newlines in your debug messages - Also, NV_ATOMIC() is a thing With those two fixed: Reviewed-by: Lyude Paul <lyude at redhat.com>> ? > ????????return head->func->curs_format(head, asyw, asyh); > ?}-- 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!
Ilia Mirkin
2021-Feb-05 21:50 UTC
[Nouveau] [PATCH v2] nouveau/dispnv50: add cursor pitch check
On Fri, Feb 5, 2021 at 4:00 PM Simon Ser <contact at emersion.fr> wrote:> > The hardware needs a FB which is packed. Add checks to make sure > this is the case. > > While at it, add debug logs for the existing checks. This allows > user-space to more easily figure out why a configuration is > rejected. > > v2: > - Use drm_format_info instead of hardcoding bytes-per-pixel (Ilia) > - Remove unnecessary size check (Ilia) > > 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 | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > index 54fbd6fe751d..00f09c9a8d15 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c > @@ -30,6 +30,7 @@ > > #include <drm/drm_atomic_helper.h> > #include <drm/drm_plane_helper.h> > +#include <drm/drm_fourcc.h> > > bool > curs507a_space(struct nv50_wndw *wndw) > @@ -99,6 +100,8 @@ 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; > + struct drm_framebuffer *fb = asyw->state.fb; > int ret; > > ret = drm_atomic_helper_check_plane_state(&asyw->state, &asyh->state, > @@ -109,12 +112,26 @@ 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.pitch[0] != asyw->image.w * fb->format->cpp[0]) { > + 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) > + if (ret) { > + drm_dbg_atomic(dev, > + "Invalid cursor image size: unsupported size %dx%d", > + asyw->image.w, asyw->image.h); > return ret; > + } > > return head->func->curs_format(head, asyw, asyh);While you're at it, maybe give this one some love too? Cheers, -ilia