diff mbox

[PATCHv3,1/8] drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*

Message ID 1384274965-30549-1-git-send-email-denis@eukrea.com
State New
Headers show

Commit Message

Denis Carikli Nov. 12, 2013, 4:49 p.m. UTC 
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

Comments

Russell King - ARM Linux Nov. 12, 2013, 5:04 p.m. UTC | #1 
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.
Eric Benard Nov. 13, 2013, 7:53 a.m. UTC | #2 
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
Philipp Zabel Nov. 13, 2013, 9:05 a.m. UTC | #3 
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
Russell King - ARM Linux Nov. 13, 2013, 9:41 a.m. UTC | #4 
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 mbox

Patch

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>