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

[Daniel Vetter]

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

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

> +	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;
> -- 
> 2.3.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch