Re: [PATCH 06/21] drm/omap: check CRTC color format earlier

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Friday 27 February 2015 13:07:57 Daniel Vetter wrote:
> On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote:
> > When setting a color format to a DRM plane, the DRM core checks whether
> > the format is supported by the HW. However, it seems that when setting
> > the color format of a CRTC (i.e. a root plane), there's no checking
> > done. This causes omapdrm to configure omapdss with the bad color
> > format, which omapdss then rejects.
> > 
> > While the above is ok, the error message is a bit confusing, and the
> > configuring of omapdss happens asynchronously from the ioctl that does
> > the color format change.
> > 
> > This patch adds a color format check to omap_plane_mode_set() which
> > causes the ioctl to return an error for invalid color format. This means
> > that the userspace will get to know of the wrong setting, instead of the
> > current behavior where the error is not seen by the userspace.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> > b/drivers/gpu/drm/omapdrm/omap_plane.c index 1f4f2b866379..bedd6f7af0f1
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> > @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
> > 
> >  {
> >  
> >  	struct omap_plane *omap_plane = to_omap_plane(plane);
> >  	struct omap_drm_window *win = &omap_plane->win;
> > 
> > +	int i;
> > +
> > +	/*
> > +	 * Check whether this plane supports the fb pixel format.
> > +	 * I don't think this should really be needed, but it looks like the
> > +	 * drm core only checks the format for planes, not for the crtc. So
> > +	 * when setting the format for crtc, without this check we would
> > +	 * get an error later when trying to program the color format into 
the
> > +	 * HW.
> > +	 */
> 
> I think we should add a format check to the setcrtc ioctl if crtc->primary
> is set. Atm the code is in __setplane_internal but could easily be shared
> - there's already a copy in drm_atomi.c.

I'll submit a patch.

> Omapdrm wouldn't benefit from this yet since it doesn't support universal
> planes. But adding universal planes should be on your todo anyway ;-)

Soon, soon :-)

> > +	for (i = 0; i < plane->format_count; i++)
> > +		if (fb->pixel_format == plane->format_types[i])
> > +			break;
> > +	if (i == plane->format_count) {
> > +		DBG("Invalid pixel format %s",
> > +			      drm_get_format_name(fb->pixel_format));
> > +		return -EINVAL;
> > +	}
> > 
> >  	win->crtc_x = crtc_x;
> >  	win->crtc_y = crtc_y;

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel