| Message ID | 1384274965-30549-1-git-send-email-denis@eukrea.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 12, 2013 at 05:49:18PM +0100, Denis Carikli wrote: > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index fc2adb6..586c12f 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -537,6 +537,15 @@ int drm_display_mode_from_videomode(const struct videomode *vm, > dmode->flags |= DRM_MODE_FLAG_DBLSCAN; > if (vm->flags & DISPLAY_FLAGS_DOUBLECLK) > dmode->flags |= DRM_MODE_FLAG_DBLCLK; > + if (vm->flags & DISPLAY_FLAGS_DE_LOW) > + dmode->flags |= DRM_MODE_FLAG_DE_LOW; > + if (vm->flags & DISPLAY_FLAGS_DE_HIGH) > + dmode->flags |= DRM_MODE_FLAG_DE_HIGH; > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_POSEDGE; > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_NEGEDGE; > + I'm still not convinced that these should be exposed in *any* way to userspace - I'm with Ville Syrjälä on this point. Yes, you've moved their definition out of a uapi header file, but they're still leaking out of kernel space via calls (and with Xorg, they'll leak into the DisplayMode structures within the X server.) Now, here's the thing... The polarity of the display enable signal. That's a property of the connected device right? It doesn't change with respect to the displayed mode unlike the hsync/vsync signals. If that's true, it should not be here. Same goes for the pixel clock edge. If it's a property of the connected device and doesn't have a dependence on the displayed mode, then it should not be in the DRM mode structure.
Hi Russell, Le Tue, 12 Nov 2013 17:04:55 +0000, Russell King - ARM Linux <linux@arm.linux.org.uk> a écrit : > On Tue, Nov 12, 2013 at 05:49:18PM +0100, Denis Carikli wrote: > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > > index fc2adb6..586c12f 100644 > > --- a/drivers/gpu/drm/drm_modes.c > > +++ b/drivers/gpu/drm/drm_modes.c > > @@ -537,6 +537,15 @@ int drm_display_mode_from_videomode(const struct videomode *vm, > > dmode->flags |= DRM_MODE_FLAG_DBLSCAN; > > if (vm->flags & DISPLAY_FLAGS_DOUBLECLK) > > dmode->flags |= DRM_MODE_FLAG_DBLCLK; > > + if (vm->flags & DISPLAY_FLAGS_DE_LOW) > > + dmode->flags |= DRM_MODE_FLAG_DE_LOW; > > + if (vm->flags & DISPLAY_FLAGS_DE_HIGH) > > + dmode->flags |= DRM_MODE_FLAG_DE_HIGH; > > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) > > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_POSEDGE; > > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_NEGEDGE; > > + > > I'm still not convinced that these should be exposed in *any* way to > userspace - I'm with Ville Syrjälä on this point. > > Yes, you've moved their definition out of a uapi header file, but > they're still leaking out of kernel space via calls (and with Xorg, > they'll leak into the DisplayMode structures within the X server.) > > Now, here's the thing... The polarity of the display enable signal. > That's a property of the connected device right? It doesn't change > with respect to the displayed mode unlike the hsync/vsync signals. > If that's true, it should not be here. > > Same goes for the pixel clock edge. If it's a property of the > connected device and doesn't have a dependence on the displayed > mode, then it should not be in the DRM mode structure. What would be the right way to configure these settings without exposing them to userspace ? As explained in my answer to Fabio, these settings are currently hardcoded into ipuv3-crtc and we need to configure them to support more TFT panels using the IPUV3 Parallel Display Interface. Thanks Eric
Hi Eric, Am Mittwoch, den 13.11.2013, 08:53 +0100 schrieb Eric Bénard: > Hi Russell, > > Le Tue, 12 Nov 2013 17:04:55 +0000, > Russell King - ARM Linux <linux@arm.linux.org.uk> a écrit : > > On Tue, Nov 12, 2013 at 05:49:18PM +0100, Denis Carikli wrote: > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > > > index fc2adb6..586c12f 100644 > > > --- a/drivers/gpu/drm/drm_modes.c > > > +++ b/drivers/gpu/drm/drm_modes.c > > > @@ -537,6 +537,15 @@ int drm_display_mode_from_videomode(const struct videomode *vm, > > > dmode->flags |= DRM_MODE_FLAG_DBLSCAN; > > > if (vm->flags & DISPLAY_FLAGS_DOUBLECLK) > > > dmode->flags |= DRM_MODE_FLAG_DBLCLK; > > > + if (vm->flags & DISPLAY_FLAGS_DE_LOW) > > > + dmode->flags |= DRM_MODE_FLAG_DE_LOW; > > > + if (vm->flags & DISPLAY_FLAGS_DE_HIGH) > > > + dmode->flags |= DRM_MODE_FLAG_DE_HIGH; > > > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) > > > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_POSEDGE; > > > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > > > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_NEGEDGE; > > > + > > > > I'm still not convinced that these should be exposed in *any* way to > > userspace - I'm with Ville Syrjälä on this point. > > > > Yes, you've moved their definition out of a uapi header file, but > > they're still leaking out of kernel space via calls (and with Xorg, > > they'll leak into the DisplayMode structures within the X server.) > > > > Now, here's the thing... The polarity of the display enable signal. > > That's a property of the connected device right? It doesn't change > > with respect to the displayed mode unlike the hsync/vsync signals. > > If that's true, it should not be here. > > > > Same goes for the pixel clock edge. If it's a property of the > > connected device and doesn't have a dependence on the displayed > > mode, then it should not be in the DRM mode structure. > > What would be the right way to configure these settings without > exposing them to userspace ? I think as a property of the connected device, this should be obtained from the device tree node of the panel. In the v4l2 style device tree model this could also be made a property of the link (endpoint). > As explained in my answer to Fabio, these settings are currently > hardcoded into ipuv3-crtc and we need to configure them to support more > TFT panels using the IPUV3 Parallel Display Interface. regards Philipp
On Wed, Nov 13, 2013 at 08:53:18AM +0100, Eric Bénard wrote: > Hi Russell, > > Le Tue, 12 Nov 2013 17:04:55 +0000, > Russell King - ARM Linux <linux@arm.linux.org.uk> a écrit : > > On Tue, Nov 12, 2013 at 05:49:18PM +0100, Denis Carikli wrote: > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > > > index fc2adb6..586c12f 100644 > > > --- a/drivers/gpu/drm/drm_modes.c > > > +++ b/drivers/gpu/drm/drm_modes.c > > > @@ -537,6 +537,15 @@ int drm_display_mode_from_videomode(const struct videomode *vm, > > > dmode->flags |= DRM_MODE_FLAG_DBLSCAN; > > > if (vm->flags & DISPLAY_FLAGS_DOUBLECLK) > > > dmode->flags |= DRM_MODE_FLAG_DBLCLK; > > > + if (vm->flags & DISPLAY_FLAGS_DE_LOW) > > > + dmode->flags |= DRM_MODE_FLAG_DE_LOW; > > > + if (vm->flags & DISPLAY_FLAGS_DE_HIGH) > > > + dmode->flags |= DRM_MODE_FLAG_DE_HIGH; > > > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) > > > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_POSEDGE; > > > + if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > > > + dmode->flags |= DRM_MODE_FLAG_PIXDATA_NEGEDGE; > > > + > > > > I'm still not convinced that these should be exposed in *any* way to > > userspace - I'm with Ville Syrjälä on this point. > > > > Yes, you've moved their definition out of a uapi header file, but > > they're still leaking out of kernel space via calls (and with Xorg, > > they'll leak into the DisplayMode structures within the X server.) > > > > Now, here's the thing... The polarity of the display enable signal. > > That's a property of the connected device right? It doesn't change > > with respect to the displayed mode unlike the hsync/vsync signals. > > If that's true, it should not be here. > > > > Same goes for the pixel clock edge. If it's a property of the > > connected device and doesn't have a dependence on the displayed > > mode, then it should not be in the DRM mode structure. > > What would be the right way to configure these settings without > exposing them to userspace ? > > As explained in my answer to Fabio, these settings are currently > hardcoded into ipuv3-crtc and we need to configure them to support more > TFT panels using the IPUV3 Parallel Display Interface. First, realise that what you're doing is configuring components within the IMX driver "suite" so what you need to do is communicate your requirements _only_ from parallel-display.c to ipuv3-crtc.c. There's already infrastructure in imx-drm for the display bridges to communicate various information to the CRTC layer - there's already the encoder type, and the "pins" for hsync/vsync being communicated via imx_drm_panel_format*(). This is really no different IMHO.
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index fc2adb6..586c12f 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -537,6 +537,15 @@ int drm_display_mode_from_videomode(const struct videomode *vm, dmode->flags |= DRM_MODE_FLAG_DBLSCAN; if (vm->flags & DISPLAY_FLAGS_DOUBLECLK) dmode->flags |= DRM_MODE_FLAG_DBLCLK; + if (vm->flags & DISPLAY_FLAGS_DE_LOW) + dmode->flags |= DRM_MODE_FLAG_DE_LOW; + if (vm->flags & DISPLAY_FLAGS_DE_HIGH) + dmode->flags |= DRM_MODE_FLAG_DE_HIGH; + if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) + dmode->flags |= DRM_MODE_FLAG_PIXDATA_POSEDGE; + if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) + dmode->flags |= DRM_MODE_FLAG_PIXDATA_NEGEDGE; + drm_mode_set_name(dmode); return 0; diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h new file mode 100644 index 0000000..07e5259 --- /dev/null +++ b/include/drm/drm_mode.h @@ -0,0 +1,34 @@ +/* + * Copyright 2013 Eukréa Electromatique <denis@eukrea.com> + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#ifndef _DRM_MODE_H +#define _DRM_MODE_H + +#include <uapi/drm/drm_mode.h> + +/* DISPLAY_FLAGS DRM counterpart */ +#define DRM_MODE_FLAG_DE_HIGH (1<<22) +#define DRM_MODE_FLAG_DE_LOW (1<<23) +#define DRM_MODE_FLAG_PIXDATA_POSEDGE (1<<24) +#define DRM_MODE_FLAG_PIXDATA_NEGEDGE (1<<25) + +#endif diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 28acbaf..5681897 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -24,8 +24,8 @@ * IN THE SOFTWARE. */ -#ifndef _DRM_MODE_H -#define _DRM_MODE_H +#ifndef _UAPI_DRM_MODE_H +#define _UAPI_DRM_MODE_H #include <linux/types.h>
Without that fix, drivers using the drm_display_mode_from_videomode function will not be able to get certain information because some DISPLAY_FLAGS_* have no corresponding DRM_MODE_FLAG_*. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: driverdev-devel@linuxdriverproject.org Cc: Matt Sealey <neko@bakuhatsu.net> Cc: David Airlie <airlied@linux.ie> Cc: Ville Syrjala <syrjala@sci.fi> Cc: dri-devel@lists.freedesktop.org Cc: Fabio Estevam <fabio.estevam@freescale.com> Cc: Sascha Hauer <kernel@pengutronix.de> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: linux-arm-kernel@lists.infradead.org Cc: Eric Bénard <eric@eukrea.com> Signed-off-by: Denis Carikli <denis@eukrea.com> --- ChangeLog v2->v3: - Added some interested people in the Cc list. - Removed a duplicated Cc. - Changed the flags names to match the one in DISPLAY_FLAGS. - Moved the flags out of the userspace headers. - Updated the rest of the code accordingly. --- drivers/gpu/drm/drm_modes.c | 9 +++++++++ include/drm/drm_mode.h | 34 ++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_mode.h | 4 ++-- 3 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 include/drm/drm_mode.h