| Message ID | 1424956829-22892-7-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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.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. > > 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
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.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. > > > > 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.
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;
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. + */ + 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;
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(+)