Re: [PATCH 06/21] drm/omap: check CRTC color format earlier
Hi Tomi,
On Monday 02 March 2015 11:55:48 Tomi Valkeinen wrote:
On 27/02/15 14:07, 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.valkei...@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 ;-)
Laurent is working on universal planes and atomic modesetting for
omapdrm. In fact, I think universal planes is already done in his WIP
branch.
So, if this check is already done with universal planes (if I understood
correctly), I'm fine with dropping this patch.
Yes, I've implemented universal plane support. I'm currently rebasing your
patches on top of my bug fixes (excluding the more intrusive atomic update
rework). I'll push the resulting branch shortly and will send the patches for
review.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/21] drm/omap: check CRTC color format earlier
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.valkei...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/21] drm/omap: check CRTC color format earlier
On 27/02/15 14:07, 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.valkei...@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 ;-)
Laurent is working on universal planes and atomic modesetting for
omapdrm. In fact, I think universal planes is already done in his WIP
branch.
So, if this check is already done with universal planes (if I understood
correctly), I'm fine with dropping this patch.
Tomi
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 06/21] drm/omap: check CRTC color format earlier
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.valkei...@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-de...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
