[PATCHv3 1/8] drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*
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 +, > Russell King - ARM Linux 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
[PATCHv3 1/8] drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*
On Wed, Nov 13, 2013 at 08:53:18AM +0100, Eric B?nard wrote: > Hi Russell, > > Le Tue, 12 Nov 2013 17:04:55 +, > Russell King - ARM Linux 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.
[PATCHv3 1/8] drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*
Hi Russell, Le Tue, 12 Nov 2013 17:04:55 +, Russell King - ARM Linux 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
[PATCHv3 1/8] drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*
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 Cc: driverdev-devel at linuxdriverproject.org Cc: Matt Sealey Cc: David Airlie Cc: Ville Syrjala Cc: dri-devel at lists.freedesktop.org Cc: Fabio Estevam Cc: Sascha Hauer Cc: Shawn Guo Cc: linux-arm-kernel at lists.infradead.org Cc: Eric B?nard Signed-off-by: Denis Carikli --- 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 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 000..07e5259 --- /dev/null +++ b/include/drm/drm_mode.h @@ -0,0 +1,34 @@ +/* + * Copyright 2013 Eukr?a Electromatique + * + * 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 + +/* 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 -- 1.7.9.5
[PATCHv3 1/8] drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*
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.