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@xxxxxx> > >>> > >>> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
