[PATCH] drm/omap: check plane size

[Ville Syrjälä]

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

> 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