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@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
