* [PATCH] drm/omap: check plane size @ 2016-04-26 10:16 Tomi Valkeinen 2016-04-27 15:30 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Tomi Valkeinen @ 2016-04-26 10:16 UTC (permalink / raw) To: dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen At the moment we don't check the plane input/output sizes, which can lead to DSS HW errors when invalid values are given from the userspace. Add a check so that the sizes are > 0. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 93ee538a99f5..fa9e5086eb65 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -168,6 +168,12 @@ static int omap_plane_atomic_check(struct drm_plane *plane, if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state); + if (state->src_w == 0 || state->src_h == 0) + return -EINVAL; + + if (state->crtc_w == 0 || state->crtc_h == 0) + return -EINVAL; + if (state->crtc_x < 0 || state->crtc_y < 0) return -EINVAL; -- 2.5.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: check plane size 2016-04-26 10:16 [PATCH] drm/omap: check plane size Tomi Valkeinen @ 2016-04-27 15:30 ` Laurent Pinchart 2016-04-27 17:29 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2016-04-27 15:30 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Daniel Vetter, dri-devel Hi Tomi, (CC'ing Daniel) Thank you for the patch. On Tuesday 26 Apr 2016 13:16:42 Tomi Valkeinen wrote: > At the moment we don't check the plane input/output sizes, which can > lead to DSS HW errors when invalid values are given from the userspace. > > Add a check so that the sizes are > 0. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> I don't think it makes sense to have an empty source or destination rectangle for any driver, not just omapdrm. Shouldn't this check be added to drm_atomic_plane_check() instead ? > --- > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > b/drivers/gpu/drm/omapdrm/omap_plane.c index 93ee538a99f5..fa9e5086eb65 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > @@ -168,6 +168,12 @@ static int omap_plane_atomic_check(struct drm_plane > *plane, if (IS_ERR(crtc_state)) > return PTR_ERR(crtc_state); > > + if (state->src_w == 0 || state->src_h == 0) > + return -EINVAL; > + > + if (state->crtc_w == 0 || state->crtc_h == 0) > + return -EINVAL; > + > if (state->crtc_x < 0 || state->crtc_y < 0) > return -EINVAL; -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: check plane size 2016-04-27 15:30 ` Laurent Pinchart @ 2016-04-27 17:29 ` Ville Syrjälä 2016-04-27 20:02 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2016-04-27 17:29 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel On Wed, Apr 27, 2016 at 06:30:19PM +0300, Laurent Pinchart wrote: > Hi Tomi, > > (CC'ing Daniel) > > Thank you for the patch. > > On Tuesday 26 Apr 2016 13:16:42 Tomi Valkeinen wrote: > > At the moment we don't check the plane input/output sizes, which can > > lead to DSS HW errors when invalid values are given from the userspace. > > > > Add a check so that the sizes are > 0. > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > I don't think it makes sense to have an empty source or destination rectangle > for any driver, not just omapdrm. Shouldn't this check be added to > drm_atomic_plane_check() instead ? It's perfectly legal. Just means you're supposed to turn the plane off. > > > --- > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > > b/drivers/gpu/drm/omapdrm/omap_plane.c index 93ee538a99f5..fa9e5086eb65 > > 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > @@ -168,6 +168,12 @@ static int omap_plane_atomic_check(struct drm_plane > > *plane, if (IS_ERR(crtc_state)) > > return PTR_ERR(crtc_state); > > > > + if (state->src_w == 0 || state->src_h == 0) > > + return -EINVAL; > > + > > + if (state->crtc_w == 0 || state->crtc_h == 0) > > + return -EINVAL; > > + > > if (state->crtc_x < 0 || state->crtc_y < 0) > > return -EINVAL; > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: check plane size 2016-04-27 17:29 ` Ville Syrjälä @ 2016-04-27 20:02 ` Laurent Pinchart 2016-04-28 18:30 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2016-04-27 20:02 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel Hi Ville, On Wednesday 27 Apr 2016 20:29:24 Ville Syrjälä wrote: > On Wed, Apr 27, 2016 at 06:30:19PM +0300, Laurent Pinchart wrote: > > Hi Tomi, > > > > (CC'ing Daniel) > > > > Thank you for the patch. > > > > On Tuesday 26 Apr 2016 13:16:42 Tomi Valkeinen wrote: > > > At the moment we don't check the plane input/output sizes, which can > > > lead to DSS HW errors when invalid values are given from the userspace. > > > > > > Add a check so that the sizes are > 0. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > > I don't think it makes sense to have an empty source or destination > > rectangle for any driver, not just omapdrm. Shouldn't this check be added > > to drm_atomic_plane_check() instead ? > > It's perfectly legal. Just means you're supposed to turn the plane off. Where is that documented ? I thought turning the plane off was done by setting the framebuffer to NULL (in which case the src and crtc coordinates must of course be ignored) ? > > > --- > > > > > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > > > b/drivers/gpu/drm/omapdrm/omap_plane.c index 93ee538a99f5..fa9e5086eb65 > > > 100644 > > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > > @@ -168,6 +168,12 @@ static int omap_plane_atomic_check(struct drm_plane > > > *plane, if (IS_ERR(crtc_state)) > > > > > > return PTR_ERR(crtc_state); > > > > > > + if (state->src_w == 0 || state->src_h == 0) > > > + return -EINVAL; > > > + > > > + if (state->crtc_w == 0 || state->crtc_h == 0) > > > + return -EINVAL; > > > + > > > > > > if (state->crtc_x < 0 || state->crtc_y < 0) > > > > > > return -EINVAL; -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: check plane size 2016-04-27 20:02 ` Laurent Pinchart @ 2016-04-28 18:30 ` Ville Syrjälä 2016-04-28 21:52 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2016-04-28 18:30 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel On Wed, Apr 27, 2016 at 11:02:17PM +0300, Laurent Pinchart wrote: > Hi Ville, > > On Wednesday 27 Apr 2016 20:29:24 Ville Syrjälä wrote: > > On Wed, Apr 27, 2016 at 06:30:19PM +0300, Laurent Pinchart wrote: > > > Hi Tomi, > > > > > > (CC'ing Daniel) > > > > > > Thank you for the patch. > > > > > > On Tuesday 26 Apr 2016 13:16:42 Tomi Valkeinen wrote: > > > > At the moment we don't check the plane input/output sizes, which can > > > > lead to DSS HW errors when invalid values are given from the userspace. > > > > > > > > Add a check so that the sizes are > 0. > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > > > > I don't think it makes sense to have an empty source or destination > > > rectangle for any driver, not just omapdrm. Shouldn't this check be added > > > to drm_atomic_plane_check() instead ? > > > > It's perfectly legal. Just means you're supposed to turn the plane off. > > Where is that documented ? Do we have uapi docs? > I thought turning the plane off was done by setting > the framebuffer to NULL (in which case the src and crtc coordinates must of > course be ignored) ? That's another way. However setting the fb to 0 is a bit different, as then you're not holding a ref on the fb (nor does it get pinned etc.). So eg. if you want to make sure that you really can pin the fb, but want to have the plane disabled for a bit, you could just the clear out the coordinates. Also from an implementation POV it's no different than the plane just getting clipped away entirely, so supporting this way of disabling a plane has no extra cost really. > > > > > --- > > > > > > > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > > > > b/drivers/gpu/drm/omapdrm/omap_plane.c index 93ee538a99f5..fa9e5086eb65 > > > > 100644 > > > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > > > @@ -168,6 +168,12 @@ static int omap_plane_atomic_check(struct drm_plane > > > > *plane, if (IS_ERR(crtc_state)) > > > > > > > > return PTR_ERR(crtc_state); > > > > > > > > + if (state->src_w == 0 || state->src_h == 0) > > > > + return -EINVAL; > > > > + > > > > + if (state->crtc_w == 0 || state->crtc_h == 0) > > > > + return -EINVAL; > > > > + > > > > > > > > if (state->crtc_x < 0 || state->crtc_y < 0) > > > > > > > > return -EINVAL; > > -- > Regards, > > Laurent Pinchart -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: check plane size 2016-04-28 18:30 ` Ville Syrjälä @ 2016-04-28 21:52 ` Laurent Pinchart 2016-04-29 7:03 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2016-04-28 21:52 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel Hi Ville, On Thursday 28 Apr 2016 21:30:18 Ville Syrjälä wrote: > On Wed, Apr 27, 2016 at 11:02:17PM +0300, Laurent Pinchart wrote: > > On Wednesday 27 Apr 2016 20:29:24 Ville Syrjälä wrote: > >> On Wed, Apr 27, 2016 at 06:30:19PM +0300, Laurent Pinchart wrote: > >>> On Tuesday 26 Apr 2016 13:16:42 Tomi Valkeinen wrote: > >>>> At the moment we don't check the plane input/output sizes, which can > >>>> lead to DSS HW errors when invalid values are given from the > >>>> userspace. > >>>> > >>>> Add a check so that the sizes are > 0. > >>>> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >>> > >>> I don't think it makes sense to have an empty source or destination > >>> rectangle for any driver, not just omapdrm. Shouldn't this check be > >>> added to drm_atomic_plane_check() instead ? > >> > >> It's perfectly legal. Just means you're supposed to turn the plane off. > > > > Where is that documented ? > > Do we have uapi docs? My point, really :-) > > I thought turning the plane off was done by setting > > the framebuffer to NULL (in which case the src and crtc coordinates must > > of course be ignored) ? > > That's another way. However setting the fb to 0 is a bit different, as > then you're not holding a ref on the fb (nor does it get pinned etc.). > So eg. if you want to make sure that you really can pin the fb, but > want to have the plane disabled for a bit, you could just the clear out > the coordinates. > > Also from an implementation POV it's no different than the plane just > getting clipped away entirely, so supporting this way of disabling a > plane has no extra cost really. This really needs to be captured in a uapi documentation, this is the first time I hear about that feature, and I'm pretty sure I'm not the only one. We can't expect drivers to implement a consistent behaviour if the expected behaviour isn't documented. > >>>> --- > >>>> > >>>> drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ > >>>> 1 file changed, 6 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > >>>> b/drivers/gpu/drm/omapdrm/omap_plane.c index > >>>> 93ee538a99f5..fa9e5086eb65 > >>>>> 100644 > >>>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c > >>>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > >>>> @@ -168,6 +168,12 @@ static int omap_plane_atomic_check(struct > >>>> drm_plane > >>> *plane, if (IS_ERR(crtc_state)) > >>>> > >>>> return PTR_ERR(crtc_state); > >>>> > >>>> + if (state->src_w == 0 || state->src_h == 0) > >>>> + return -EINVAL; > >>>> + > >>>> + if (state->crtc_w == 0 || state->crtc_h == 0) > >>>> + return -EINVAL; > >>>> + > >>>> > >>>> if (state->crtc_x < 0 || state->crtc_y < 0) > >>>> > >>>> return -EINVAL; -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: check plane size 2016-04-28 21:52 ` Laurent Pinchart @ 2016-04-29 7:03 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2016-04-29 7:03 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Tomi Valkeinen, dri-devel On Thu, Apr 28, 2016 at 11:52 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> > I thought turning the plane off was done by setting >> > the framebuffer to NULL (in which case the src and crtc coordinates must >> > of course be ignored) ? >> >> That's another way. However setting the fb to 0 is a bit different, as >> then you're not holding a ref on the fb (nor does it get pinned etc.). >> So eg. if you want to make sure that you really can pin the fb, but >> want to have the plane disabled for a bit, you could just the clear out >> the coordinates. >> >> Also from an implementation POV it's no different than the plane just >> getting clipped away entirely, so supporting this way of disabling a >> plane has no extra cost really. > > This really needs to be captured in a uapi documentation, this is the first > time I hear about that feature, and I'm pretty sure I'm not the only one. We > can't expect drivers to implement a consistent behaviour if the expected > behaviour isn't documented. I think we also have some room for better helpers. There's one to validate plane state, but it was written back for legacy planes and so takes slightly different arguments than what drm_plane_state provides. Doing an atomic version of that would likely get rid of lots of duplicated boilerplate all over drivers. And that helper does compute bool visible how Ville explained here. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-29 7:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-26 10:16 [PATCH] drm/omap: check plane size Tomi Valkeinen 2016-04-27 15:30 ` Laurent Pinchart 2016-04-27 17:29 ` Ville Syrjälä 2016-04-27 20:02 ` Laurent Pinchart 2016-04-28 18:30 ` Ville Syrjälä 2016-04-28 21:52 ` Laurent Pinchart 2016-04-29 7:03 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.