[PATCH] drm/omap: check plane size

[Laurent Pinchart]

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 at 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