[PATCH v2] of: Add videomode helper

[PATCH v2] of: Add videomode helper

[Sascha Hauer]

This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

changes since v1:
- use hyphens instead of underscores for property names

 .../devicetree/bindings/video/displaymode          |   40 ++++++++
 drivers/of/Kconfig                                 |    5 +
 drivers/of/Makefile                                |    1 +
 drivers/of/of_videomode.c                          |  108 ++++++++++++++++++++
 include/linux/of_videomode.h                       |   19 ++++
 5 files changed, 173 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/displaymode
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h

diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 0000000..43cc17d
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+=========
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
+   in pixels
+   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
+   lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree representation
+corresponds to the one used by the Linux Framebuffer framework described here in
+Documentation/fb/framebuffer.txt. This representation has been chosen because it's
+the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
+framework the devicetree has the clock in Hz instead of ps.
+
+Example:
+
+	display@0 {
+		/* 1920x1080p24 */
+		clock = <52000000>;
+		xres = <1920>;
+		yres = <1080>;
+		left-margin = <25>;
+		right-margin = <25>;
+		hsync-len = <25>;
+		lower-margin = <2>;
+		upper-margin = <2>;
+		vsync-len = <2>;
+		hsync-active-high;
+	};
+
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a3acaa3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_VIDEOMODE
+	def_bool y
+	help
+	  helper to parse videomodes from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..80e6db3 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_VIDEOMODE)	+= of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 0000000..50d3bd2
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,108 @@
+/*
+ * OF helpers for parsing display modes
+ *
+ * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/export.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+		struct fb_videomode *fbmode)
+{
+	int ret = 0;
+	u32 left_margin, xres, right_margin, hsync_len;
+	u32 upper_margin, yres, lower_margin, vsync_len;
+	u32 width_mm = 0, height_mm = 0;
+	u32 clock;
+	bool hah = false, vah = false, interlaced = false, doublescan = false;
+
+	if (!np)
+		return -EINVAL;
+
+	ret |= of_property_read_u32(np, "left-margin", &left_margin);
+	ret |= of_property_read_u32(np, "xres", &xres);
+	ret |= of_property_read_u32(np, "right-margin", &right_margin);
+	ret |= of_property_read_u32(np, "hsync-len", &hsync_len);
+	ret |= of_property_read_u32(np, "upper-margin", &upper_margin);
+	ret |= of_property_read_u32(np, "yres", &yres);
+	ret |= of_property_read_u32(np, "lower-margin", &lower_margin);
+	ret |= of_property_read_u32(np, "vsync-len", &vsync_len);
+	ret |= of_property_read_u32(np, "clock", &clock);
+	if (ret)
+		return -EINVAL;
+
+	of_property_read_u32(np, "width-mm", &width_mm);
+	of_property_read_u32(np, "height-mm", &height_mm);
+
+	hah = of_property_read_bool(np, "hsync-active-high");
+	vah = of_property_read_bool(np, "vsync-active-high");
+	interlaced = of_property_read_bool(np, "interlaced");
+	doublescan = of_property_read_bool(np, "doublescan");
+
+	if (dmode) {
+		memset(dmode, 0, sizeof(*dmode));
+
+		dmode->hdisplay = xres;
+		dmode->hsync_start = xres + right_margin;
+		dmode->hsync_end = xres + right_margin + hsync_len;
+		dmode->htotal = xres + right_margin + hsync_len + left_margin;
+
+		dmode->vdisplay = yres;
+		dmode->vsync_start = yres + lower_margin;
+		dmode->vsync_end = yres + lower_margin + vsync_len;
+		dmode->vtotal = yres + lower_margin + vsync_len + upper_margin;
+
+		dmode->width_mm = width_mm;
+		dmode->height_mm = height_mm;
+
+		dmode->clock = clock / 1000;
+
+		if (hah)
+			dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+		else
+			dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+		if (vah)
+			dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+		else
+			dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+		if (interlaced)
+			dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+		if (doublescan)
+			dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+
+		drm_mode_set_name(dmode);
+	}
+
+	if (fbmode) {
+		memset(fbmode, 0, sizeof(*fbmode));
+
+		fbmode->xres = xres;
+		fbmode->left_margin = left_margin;
+		fbmode->right_margin = right_margin;
+		fbmode->hsync_len = hsync_len;
+
+		fbmode->yres = yres;
+		fbmode->upper_margin = upper_margin;
+		fbmode->lower_margin = lower_margin;
+		fbmode->vsync_len = vsync_len;
+
+		fbmode->pixclock = KHZ2PICOS(clock / 1000);
+
+		if (hah)
+			fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+		if (vah)
+			fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+		if (interlaced)
+			fbmode->vmode |= FB_VMODE_INTERLACED;
+		if (doublescan)
+			fbmode->vmode |= FB_VMODE_DOUBLE;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_video_mode);
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..a988429
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * OF helpers for videomodes.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+struct device_node;
+struct fb_videomode;
+struct drm_display_mode;
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+		struct fb_videomode *fbmode);
+
+#endif /* __LINUX_OF_VIDEOMODE_H */
-- 
1.7.10

Re: [PATCH v2] of: Add videomode helper

[Laurent Pinchart]

Hi Sascha,

Thanks for the patch.

On Wednesday 04 July 2012 09:56:35 Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the
> devicetree. The videomode can be either converted to a struct
> drm_display_mode or a struct fb_videomode.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> changes since v1:
> - use hyphens instead of underscores for property names
> 
>  .../devicetree/bindings/video/displaymode          |   40 ++++++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_videomode.c                          |  108 +++++++++++++++++
>  include/linux/of_videomode.h                       |   19 ++++
>  5 files changed, 173 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/displaymode
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/displaymode
> b/Documentation/devicetree/bindings/video/displaymode new file mode 100644
> index 0000000..43cc17d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,40 @@
> +videomode bindings
> +=========
> +
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing
> parameters +   in pixels
> +   upper-margin, lower-margin, vsync-len: Vertical display timing
> parameters in +   lines
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm

I've always had mixed feelings about the physical display dimension being part 
of the display mode. Those are properties of the panel/display instead of the 
mode. Storing them as part of the mode can be convenient, but we then run into 
consistency issues (developers have to remember in which display mode 
instances the values are available, and in which instances they're set to 0 
for instance). If we want to clean this up, this patch would be a good 
occasion.

> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree
> representation +corresponds to the one used by the Linux Framebuffer
> framework described here in +Documentation/fb/framebuffer.txt. This
> representation has been chosen because it's +the only format which does not
> allow for inconsistent parameters.Unlike the Framebuffer +framework the
> devicetree has the clock in Hz instead of ps.
> +
> +Example:
> +
> +	display@0 {
> +		/* 1920x1080p24 */
> +		clock = <52000000>;
> +		xres = <1920>;
> +		yres = <1080>;
> +		left-margin = <25>;
> +		right-margin = <25>;
> +		hsync-len = <25>;
> +		lower-margin = <2>;
> +		upper-margin = <2>;
> +		vsync-len = <2>;
> +		hsync-active-high;
> +	};
> +

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] of: Add videomode helper

[Sascha Hauer]

On Thu, Jul 05, 2012 at 04:08:07PM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> Thanks for the patch.
> 
> > +++ b/Documentation/devicetree/bindings/video/displaymode
> > @@ -0,0 +1,40 @@
> > +videomode bindings
> > +=========
> > +
> > +Required properties:
> > + - xres, yres: Display resolution
> > + - left-margin, right-margin, hsync-len: Horizontal Display timing
> > parameters +   in pixels
> > +   upper-margin, lower-margin, vsync-len: Vertical display timing
> > parameters in +   lines
> > + - clock: displayclock in Hz
> > +
> > +Optional properties:
> > + - width-mm, height-mm: Display dimensions in mm
> 
> I've always had mixed feelings about the physical display dimension being part 
> of the display mode. Those are properties of the panel/display instead of the 
> mode. Storing them as part of the mode can be convenient, but we then run into 
> consistency issues (developers have to remember in which display mode 
> instances the values are available, and in which instances they're set to 0 
> for instance). If we want to clean this up, this patch would be a good 
> occasion.

This sounds like a display node with one or more node subnodes, like:

display {
	width_mm = <>;
	height_mm = <>;
	mode {
		xres = <>;
		yres = <>;
		...
	};
};

Is that what you mean or are you thinking of something else?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] of: Add videomode helper

[Laurent Pinchart]

Hi Sascha,

Sorry for the late reply.

On Thursday 05 July 2012 18:50:29 Sascha Hauer wrote:
> On Thu, Jul 05, 2012 at 04:08:07PM +0200, Laurent Pinchart wrote:
> > > +++ b/Documentation/devicetree/bindings/video/displaymode
> > > @@ -0,0 +1,40 @@
> > > +videomode bindings
> > > +=========
> > > +
> > > +Required properties:
> > > + - xres, yres: Display resolution
> > > + - left-margin, right-margin, hsync-len: Horizontal Display timing
> > > parameters +   in pixels
> > > +   upper-margin, lower-margin, vsync-len: Vertical display timing
> > > parameters in +   lines
> > > + - clock: displayclock in Hz
> > > +
> > > +Optional properties:
> > > + - width-mm, height-mm: Display dimensions in mm
> > 
> > I've always had mixed feelings about the physical display dimension being
> > part of the display mode. Those are properties of the panel/display
> > instead of the mode. Storing them as part of the mode can be convenient,
> > but we then run into consistency issues (developers have to remember in
> > which display mode instances the values are available, and in which
> > instances they're set to 0 for instance). If we want to clean this up,
> > this patch would be a good occasion.
> 
> This sounds like a display node with one or more node subnodes, like:
> 
> display {
> 	width_mm = <>;
> 	height_mm = <>;
> 	mode {
> 		xres = <>;
> 		yres = <>;
> 		...
> 	};
> };
> 
> Is that what you mean or are you thinking of something else?

Yes, that's exactly what I meant.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] of: Add videomode helper

[Rob Herring]

On 07/04/2012 02:56 AM, Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> changes since v1:
> - use hyphens instead of underscores for property names
> 
>  .../devicetree/bindings/video/displaymode          |   40 ++++++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_videomode.c                          |  108 ++++++++++++++++++++
>  include/linux/of_videomode.h                       |   19 ++++
>  5 files changed, 173 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/displaymode
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..43cc17d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,40 @@
> +videomode bindings
> +=========
> +
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one used by the Linux Framebuffer framework described here in
> +Documentation/fb/framebuffer.txt. This representation has been chosen because it's
> +the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
> +framework the devicetree has the clock in Hz instead of ps.

This implies you are putting linux settings into DT rather than
describing the h/w. I'm not saying the binding is wrong, but documenting
it this way makes it seem so.

One important piece missing (and IIRC linux doesn't really support) is
defining the pixel format of the interface.

> +Example:
> +
> +	display@0 {
> +		/* 1920x1080p24 */
> +		clock = <52000000>;

Should this use the clock binding? You probably need both constraints
and clock binding though.

Often you don't know the frequency up front and/or have limited control
of the frequency (i.e. integer dividers). Then you have to adjust the
margins to get the desired refresh rate. To do that, you need to know
the ranges of values a panel can support. Perhaps you just assume you
can increase the right-margin and lower-margins as I think you will hit
pixel clock frequency max before any limit on margins.

Rob

> +		xres = <1920>;
> +		yres = <1080>;
> +		left-margin = <25>;
> +		right-margin = <25>;
> +		hsync-len = <25>;
> +		lower-margin = <2>;
> +		upper-margin = <2>;
> +		vsync-len = <2>;
> +		hsync-active-high;
> +	};
> +
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..a3acaa3 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,9 @@ config OF_MTD
>  	depends on MTD
>  	def_bool y
>  
> +config OF_VIDEOMODE
> +	def_bool y
> +	help
> +	  helper to parse videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..80e6db3 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
>  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>  obj-$(CONFIG_OF_MTD)	+= of_mtd.o
> +obj-$(CONFIG_OF_VIDEOMODE)	+= of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..50d3bd2
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,108 @@
> +/*
> + * OF helpers for parsing display modes
> + *
> + * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/export.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +
> +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
> +		struct fb_videomode *fbmode)
> +{
> +	int ret = 0;
> +	u32 left_margin, xres, right_margin, hsync_len;
> +	u32 upper_margin, yres, lower_margin, vsync_len;
> +	u32 width_mm = 0, height_mm = 0;
> +	u32 clock;
> +	bool hah = false, vah = false, interlaced = false, doublescan = false;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	ret |= of_property_read_u32(np, "left-margin", &left_margin);
> +	ret |= of_property_read_u32(np, "xres", &xres);
> +	ret |= of_property_read_u32(np, "right-margin", &right_margin);
> +	ret |= of_property_read_u32(np, "hsync-len", &hsync_len);
> +	ret |= of_property_read_u32(np, "upper-margin", &upper_margin);
> +	ret |= of_property_read_u32(np, "yres", &yres);
> +	ret |= of_property_read_u32(np, "lower-margin", &lower_margin);
> +	ret |= of_property_read_u32(np, "vsync-len", &vsync_len);
> +	ret |= of_property_read_u32(np, "clock", &clock);
> +	if (ret)
> +		return -EINVAL;
> +
> +	of_property_read_u32(np, "width-mm", &width_mm);
> +	of_property_read_u32(np, "height-mm", &height_mm);
> +
> +	hah = of_property_read_bool(np, "hsync-active-high");
> +	vah = of_property_read_bool(np, "vsync-active-high");
> +	interlaced = of_property_read_bool(np, "interlaced");
> +	doublescan = of_property_read_bool(np, "doublescan");
> +
> +	if (dmode) {
> +		memset(dmode, 0, sizeof(*dmode));
> +
> +		dmode->hdisplay = xres;
> +		dmode->hsync_start = xres + right_margin;
> +		dmode->hsync_end = xres + right_margin + hsync_len;
> +		dmode->htotal = xres + right_margin + hsync_len + left_margin;
> +
> +		dmode->vdisplay = yres;
> +		dmode->vsync_start = yres + lower_margin;
> +		dmode->vsync_end = yres + lower_margin + vsync_len;
> +		dmode->vtotal = yres + lower_margin + vsync_len + upper_margin;
> +
> +		dmode->width_mm = width_mm;
> +		dmode->height_mm = height_mm;
> +
> +		dmode->clock = clock / 1000;
> +
> +		if (hah)
> +			dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> +		else
> +			dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> +		if (vah)
> +			dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> +		else
> +			dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> +		if (interlaced)
> +			dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> +		if (doublescan)
> +			dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> +
> +		drm_mode_set_name(dmode);
> +	}
> +
> +	if (fbmode) {
> +		memset(fbmode, 0, sizeof(*fbmode));
> +
> +		fbmode->xres = xres;
> +		fbmode->left_margin = left_margin;
> +		fbmode->right_margin = right_margin;
> +		fbmode->hsync_len = hsync_len;
> +
> +		fbmode->yres = yres;
> +		fbmode->upper_margin = upper_margin;
> +		fbmode->lower_margin = lower_margin;
> +		fbmode->vsync_len = vsync_len;
> +
> +		fbmode->pixclock = KHZ2PICOS(clock / 1000);
> +
> +		if (hah)
> +			fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +		if (vah)
> +			fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +		if (interlaced)
> +			fbmode->vmode |= FB_VMODE_INTERLACED;
> +		if (doublescan)
> +			fbmode->vmode |= FB_VMODE_DOUBLE;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_video_mode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..a988429
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * OF helpers for videomodes.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +struct device_node;
> +struct fb_videomode;
> +struct drm_display_mode;
> +
> +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
> +		struct fb_videomode *fbmode);
> +
> +#endif /* __LINUX_OF_VIDEOMODE_H */
> 

Re: [PATCH v2] of: Add videomode helper

[Sascha Hauer]

On Thu, Jul 05, 2012 at 09:51:39AM -0500, Rob Herring wrote:
> On 07/04/2012 02:56 AM, Sascha Hauer wrote:
> > +
> > +There are different ways of describing a display mode. The devicetree representation
> > +corresponds to the one used by the Linux Framebuffer framework described here in
> > +Documentation/fb/framebuffer.txt. This representation has been chosen because it's
> > +the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
> > +framework the devicetree has the clock in Hz instead of ps.
> 
> This implies you are putting linux settings into DT rather than
> describing the h/w. I'm not saying the binding is wrong, but documenting
> it this way makes it seem so.

The major reason to use these values was that they do not allow for
inconsistent values (as opposed to for example with hsync_start which you
would have to check for hsync_start >= xres).
I could rephrase this if it looks too much like modelled-after-Linux
instead of modelled-after-hardware.

> 
> One important piece missing (and IIRC linux doesn't really support) is
> defining the pixel format of the interface.

I could use this aswell. I think this can be specified as additional
properties later, right? I'm afraid this needs a lot of discussion so
we should delay this to the next round.

> 
> > +Example:
> > +
> > +	display@0 {
> > +		/* 1920x1080p24 */
> > +		clock = <52000000>;
> 
> Should this use the clock binding? You probably need both constraints
> and clock binding though.

Is the clock binding suitable for this? Here we are not interested where
the clock comes from, but instead which range is allowed.

> 
> Often you don't know the frequency up front and/or have limited control
> of the frequency (i.e. integer dividers). Then you have to adjust the
> margins to get the desired refresh rate. To do that, you need to know
> the ranges of values a panel can support. Perhaps you just assume you
> can increase the right-margin and lower-margins as I think you will hit
> pixel clock frequency max before any limit on margins.

Most datasheets specify min,typ,max triplets. We could do this instead
of using single fixed values for the margins:

	left_margin = <0 10 40>;

Right now we have nothing in the kernel that could handle this, but
getting the interface to the devicetree right seems indeed important.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] of: Add videomode helper

[Stephen Warren]

On 07/05/2012 08:51 AM, Rob Herring wrote:
> On 07/04/2012 02:56 AM, Sascha Hauer wrote:
>> This patch adds a helper function for parsing videomodes from the devicetree.
>> The videomode can be either converted to a struct drm_display_mode or a
>> struct fb_videomode.

>> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode

>> +Example:
>> +
>> +     display@0 {
>> +             /* 1920x1080p24 */
>> +             clock = <52000000>;
> 
> Should this use the clock binding? You probably need both constraints
> and clock binding though.

I don't think the clock binding is appropriate here. This binding
specifies a desired video mode, and should specify just the expected
clock frequency for that mode. Perhaps any tolerance imposed by the
specification used to calculate the mode timing could be specified too,
but the allowed tolerance (for a mode to be recognized by the dispaly)
is more driven by the receiving device than the transmitting device.

The clock bindings should come into play in the display controller that
sends a signal in that display mode. Something like:

mode: hd1080p {
    clock = <52000000>;
    xres = <1920>;
    yres = <1080>;
    ....
};

display-controller {
    pixel-clock-source = <&clk ...>; // common clock bindings here
    default-mode = <&mode>;

> Often you don't know the frequency up front and/or have limited control
> of the frequency (i.e. integer dividers). Then you have to adjust the
> margins to get the desired refresh rate. To do that, you need to know
> the ranges of values a panel can support. Perhaps you just assume you
> can increase the right-margin and lower-margins as I think you will hit
> pixel clock frequency max before any limit on margins.

I think this is more usually dealt with in HW design and matching
components.

The PLL/... feeding the display controller is going to have some known
specifications that imply which pixel clocks it can generate. HW
designers will pick a panel that accepts a clock the display controller
can generate. The driver for the display controller will just generate
as near of a pixel clock as it can to the rate specified in the mode
definition. I believe it'd be unusual for a display driver to start
fiddling with front-back porch (or margin) widths to munge the timing;
that kind of thing probably worked OK with analog displays, but with
digital displays where each pixel is clocked even in the margins, I
think that would cause more problems than it solved.

Similarly for external displays, the display controller will just pick
the nearest clock it can. If it can't generate a close enough clock, it
will just refuse to set the mode. In fact, a display controller driver
would typically validate this when the set of legal modes was enumerated
when initially detecting the display, so user-space typically wouldn't
even request invalid modes.

Re: [PATCH v2] of: Add videomode helper

[Stephen Warren]

On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.

> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode

> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> +   lines

Perhaps bike-shedding, but...

For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:

hactive, hsync-len, hfront-porch, hback-porch?

> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one used by the Linux Framebuffer framework described here in
> +Documentation/fb/framebuffer.txt. This representation has been chosen because it's
> +the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
> +framework the devicetree has the clock in Hz instead of ps.

As Rob mentioned, I think there shouldn't be any mention of Linux FB here.

> +
> +Example:
> +
> +	display@0 {

This node appears to describe a video mode, not a display, hence the
node name seems wrong.

Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.

So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):

disp: display {
    width-mm = <...>;
    height-mm = <...>;
    modes {
        mode@0 {
		/* 1920x1080p24 */
		clock = <52000000>;
		xres = <1920>;
		yres = <1080>;
		left-margin = <25>;
		right-margin = <25>;
		hsync-len = <25>;
		lower-margin = <2>;
		upper-margin = <2>;
		vsync-len = <2>;
		hsync-active-high;
        };
        mode@1 {
        };
    };
};

display-connector {
    display = <&disp>;
    // interface-specific properties such as pixel format here
};

Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a
few simple properties into a DT, but EDIDs have the following advantages:

a) They're already standardized and very common.

b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.

c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.

d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.

e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.

Re: [PATCH v2] of: Add videomode helper

[Sascha Hauer]

Hi Stephen,

On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
> On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> > This patch adds a helper function for parsing videomodes from the devicetree.
> > The videomode can be either converted to a struct drm_display_mode or a
> > struct fb_videomode.
> 
> > diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> 
> > +Required properties:
> > + - xres, yres: Display resolution
> > + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> > +   in pixels
> > +   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> > +   lines
> 
> Perhaps bike-shedding, but...
> 
> For the margin property names, wouldn't it be better to use terminology
> more commonly used for video timings rather than Linux FB naming. In
> other words naming like:
> 
> hactive, hsync-len, hfront-porch, hback-porch?

Can do. Just to make sure:

hactive = xres
hsync-len = hsync-len
hfront-porch = right-margin
hback-porch = left-margin

?

> 
> This node appears to describe a video mode, not a display, hence the
> node name seems wrong.
> 
> Many displays can support multiple different video modes. As mentioned
> elsewhere, properties like display width/height are properties of the
> display not the mode.
> 
> So, I might expect something more like the following (various overhead
> properties like reg/#address-cells etc. elided for simplicity):
> 
> disp: display {
>     width-mm = <...>;
>     height-mm = <...>;
>     modes {
>         mode@0 {
> 		/* 1920x1080p24 */
> 		clock = <52000000>;
> 		xres = <1920>;
> 		yres = <1080>;
> 		left-margin = <25>;
> 		right-margin = <25>;
> 		hsync-len = <25>;
> 		lower-margin = <2>;
> 		upper-margin = <2>;
> 		vsync-len = <2>;
> 		hsync-active-high;
>         };
>         mode@1 {
>         };
>     };
> };

Ok, we can do this.

> 
> display-connector {
>     display = <&disp>;
>     // interface-specific properties such as pixel format here
> };
> 
> Finally, have you considered just using an EDID instead of creating
> something custom? I know that creating an EDID is harder than writing a
> few simple properties into a DT, but EDIDs have the following advantages:

I have considered using EDID and I also tried it. It's painful. There
are no (open) tools available for creating EDID. That's something we
could change of course. Then when generating a devicetree there is
always an extra step involved creating the EDID blob. Once the EDID
blob is in devicetree it is not parsable anymore by mere humans, so
to see what we've got there is another tool involved to generate a
readable form again.

> 
> a) They're already standardized and very common.

Indeed, that's a big plus for EDID. I have no intention of removing EDID
data from the devicetree. There are situations where EDID is handy, for
example when you get EDID data provided by your vendor.

> 
> b) They allow other information such as a display's HDMI audio
> capabilities to be represented, which can then feed into an ALSA driver.
> 
> c) The few LCD panel datasheets I've seen actually quote their
> specification as an EDID already, so deriving the EDID may actually be easy.
> 
> d) People familiar with displays are almost certainly familiar with
> EDID's mode representation. There are many ways of representing display
> modes (sync position vs. porch widths, htotal specified rather than
> specifying all the components and hence htotal being calculated etc.).
> Not everyone will be familiar with all representations. Conversion
> errors are less likely if the target is EDID's familiar format.

You seem to think about a different class of displays for which EDID
indeed is a better way to handle. What I have to deal with here mostly
are dumb displays which:

- can only handle their native resolution
- Have no audio capabilities at all
- come with a datasheet which specify a min/typ/max triplet for
  xres,hsync,..., often enough they are scanned to pdf from some previously
  printed paper.

These displays are very common on embedded devices, probably that's the
reason I did not even think about the possibility that a single display
might have different modes.

> 
> e) You'll end up with exactly the same data as if you have a DDC-based
> external monitor rather than an internal panel, so you end up getting to
> a common path in display handling code much more quickly.

All we have in our display driver currently is:

	edidp = of_get_property(np, "edid", &imxpd->edid_len);
	if (edidp) {
		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
	} else {
		ret = of_get_video_mode(np, &imxpd->mode, NULL);
		if (!ret)
			imxpd->mode_valid = 1;
	}

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] of: Add videomode helper

[Stephen Warren]

On 08/03/2012 01:38 AM, Sascha Hauer wrote:
> Hi Stephen,
> 
> On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
>> On 07/04/2012 01:56 AM, Sascha Hauer wrote:
>>> This patch adds a helper function for parsing videomodes from the devicetree.
>>> The videomode can be either converted to a struct drm_display_mode or a
>>> struct fb_videomode.
>>
>>> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
>>
>>> +Required properties:
>>> + - xres, yres: Display resolution
>>> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
>>> +   in pixels
>>> +   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
>>> +   lines
>>
>> Perhaps bike-shedding, but...
>>
>> For the margin property names, wouldn't it be better to use terminology
>> more commonly used for video timings rather than Linux FB naming. In
>> other words naming like:
>>
>> hactive, hsync-len, hfront-porch, hback-porch?
> 
> Can do. Just to make sure:
> 
> hactive = xres
> hsync-len = hsync-len
> hfront-porch = right-margin
> hback-porch = left-margin

I believe so yes.

>> a) They're already standardized and very common.
> 
> Indeed, that's a big plus for EDID. I have no intention of removing EDID
> data from the devicetree. There are situations where EDID is handy, for
> example when you get EDID data provided by your vendor.
> 
>>
>> b) They allow other information such as a display's HDMI audio
>> capabilities to be represented, which can then feed into an ALSA driver.
>>
>> c) The few LCD panel datasheets I've seen actually quote their
>> specification as an EDID already, so deriving the EDID may actually be easy.
>>
>> d) People familiar with displays are almost certainly familiar with
>> EDID's mode representation. There are many ways of representing display
>> modes (sync position vs. porch widths, htotal specified rather than
>> specifying all the components and hence htotal being calculated etc.).
>> Not everyone will be familiar with all representations. Conversion
>> errors are less likely if the target is EDID's familiar format.
> 
> You seem to think about a different class of displays for which EDID
> indeed is a better way to handle. What I have to deal with here mostly
> are dumb displays which:
> 
> - can only handle their native resolution
> - Have no audio capabilities at all
> - come with a datasheet which specify a min/typ/max triplet for
>   xres,hsync,..., often enough they are scanned to pdf from some previously
>   printed paper.
> 
> These displays are very common on embedded devices, probably that's the
> reason I did not even think about the possibility that a single display
> might have different modes.

That's true, but as I mentioned, there are at least some dumb panels
(both I've seen recently) whose specification provides the EDID. I don't
know how common that is though, I must admit.

>> e) You'll end up with exactly the same data as if you have a DDC-based
>> external monitor rather than an internal panel, so you end up getting to
>> a common path in display handling code much more quickly.
> 
> All we have in our display driver currently is:
> 
> 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
> 	if (edidp) {
> 		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> 	} else {
> 		ret = of_get_video_mode(np, &imxpd->mode, NULL);
> 		if (!ret)
> 			imxpd->mode_valid = 1;
> 	}

Presumably there's more to it though; something later checks
imxpd->mode_valid and if false, parses the EDID and sets up imxpd->mode,
etc.

Re: [PATCH v2] of: Add videomode helper

[Laurent Pinchart]

Hi Sascha,

On Friday 03 August 2012 09:38:44 Sascha Hauer wrote:
> On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
> > On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> > > This patch adds a helper function for parsing videomodes from the
> > > devicetree. The videomode can be either converted to a struct
> > > drm_display_mode or a struct fb_videomode.
> > > 
> > > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > > b/Documentation/devicetree/bindings/video/displaymode
> > > 
> > > +Required properties:
> > > + - xres, yres: Display resolution
> > > + - left-margin, right-margin, hsync-len: Horizontal Display timing
> > > parameters +   in pixels
> > > +   upper-margin, lower-margin, vsync-len: Vertical display timing
> > > parameters in +   lines
> > 
> > Perhaps bike-shedding, but...
> > 
> > For the margin property names, wouldn't it be better to use terminology
> > more commonly used for video timings rather than Linux FB naming. In
> > other words naming like:
> > 
> > hactive, hsync-len, hfront-porch, hback-porch?
> 
> Can do. Just to make sure:
> 
> hactive = xres
> hsync-len = hsync-len
> hfront-porch = right-margin
> hback-porch = left-margin

That's correct. On the vertical direction, vfront-porch = lower-margin and 
vback-porch = upper-margin.

> 
> ?
> 
> > This node appears to describe a video mode, not a display, hence the
> > node name seems wrong.
> > 
> > Many displays can support multiple different video modes. As mentioned
> > elsewhere, properties like display width/height are properties of the
> > display not the mode.
> > 
> > So, I might expect something more like the following (various overhead
> > properties like reg/#address-cells etc. elided for simplicity):
> > 
> > disp: display {
> > 
> >     width-mm = <...>;
> >     height-mm = <...>;
> >     modes {
> >     
> >         mode@0 {
> > 		
> > 		/* 1920x1080p24 */
> > 		clock = <52000000>;
> > 		xres = <1920>;
> > 		yres = <1080>;
> > 		left-margin = <25>;
> > 		right-margin = <25>;
> > 		hsync-len = <25>;
> > 		lower-margin = <2>;
> > 		upper-margin = <2>;
> > 		vsync-len = <2>;
> > 		hsync-active-high;
> > 		
> >         };
> >         mode@1 {
> >         };
> >     
> >     };
> > 
> > };
> 
> Ok, we can do this.
> 
> > display-connector {
> > 
> >     display = <&disp>;
> >     // interface-specific properties such as pixel format here
> > 
> > };
> > 
> > Finally, have you considered just using an EDID instead of creating
> > something custom? I know that creating an EDID is harder than writing a
> 
> > few simple properties into a DT, but EDIDs have the following advantages:
> I have considered using EDID and I also tried it. It's painful. There
> are no (open) tools available for creating EDID. That's something we
> could change of course. Then when generating a devicetree there is
> always an extra step involved creating the EDID blob. Once the EDID
> blob is in devicetree it is not parsable anymore by mere humans, so
> to see what we've got there is another tool involved to generate a
> readable form again.
> 
> > a) They're already standardized and very common.
> 
> Indeed, that's a big plus for EDID. I have no intention of removing EDID
> data from the devicetree. There are situations where EDID is handy, for
> example when you get EDID data provided by your vendor.
> 
> > b) They allow other information such as a display's HDMI audio
> > capabilities to be represented, which can then feed into an ALSA driver.
> > 
> > c) The few LCD panel datasheets I've seen actually quote their
> > specification as an EDID already, so deriving the EDID may actually be
> > easy.
> > 
> > d) People familiar with displays are almost certainly familiar with
> > EDID's mode representation. There are many ways of representing display
> > modes (sync position vs. porch widths, htotal specified rather than
> > specifying all the components and hence htotal being calculated etc.).
> > Not everyone will be familiar with all representations. Conversion
> > errors are less likely if the target is EDID's familiar format.
> 
> You seem to think about a different class of displays for which EDID
> indeed is a better way to handle. What I have to deal with here mostly
> are dumb displays which:
> 
> - can only handle their native resolution
> - Have no audio capabilities at all
> - come with a datasheet which specify a min/typ/max triplet for
>   xres,hsync,..., often enough they are scanned to pdf from some previously
>   printed paper.
> 
> These displays are very common on embedded devices, probably that's the
> reason I did not even think about the possibility that a single display
> might have different modes.
> 
> > e) You'll end up with exactly the same data as if you have a DDC-based
> > external monitor rather than an internal panel, so you end up getting to
> > a common path in display handling code much more quickly.
> 
> All we have in our display driver currently is:
> 
> 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
> 	if (edidp) {
> 		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> 	} else {
> 		ret = of_get_video_mode(np, &imxpd->mode, NULL);
> 		if (!ret)
> 			imxpd->mode_valid = 1;
> 	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] of: Add videomode helper

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

On Wed, 2012-07-04 at 09:56 +0200, Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.

I have more or less the same generic comment for this as for the power
sequences series discussed: this would add panel specific information
into DT data, instead of the driver handling it. But, as also discussed
in the thread, there are differing opinions on which way is better.

> +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
> +		struct fb_videomode *fbmode);

From caller's point of view I think it'd make more sense to have
separate functions to get drm mode or fb mode. I don't think there's a
case where the caller would want both.

Then again, even better would be to have a common video mode struct used
by both fb and drm... But I think that's been on the todo list of
Laurent for a long time already =).

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] of: Add videomode helper

[Sascha Hauer]

On Thu, Sep 13, 2012 at 01:54:07PM +0300, Tomi Valkeinen wrote:
> On Wed, 2012-07-04 at 09:56 +0200, Sascha Hauer wrote:
> > This patch adds a helper function for parsing videomodes from the devicetree.
> > The videomode can be either converted to a struct drm_display_mode or a
> > struct fb_videomode.
> 
> I have more or less the same generic comment for this as for the power
> sequences series discussed: this would add panel specific information
> into DT data, instead of the driver handling it. But, as also discussed
> in the thread, there are differing opinions on which way is better.

With the panel timings I think the approach of moving them into DT is
the best we can do. There are so many displays out there, patching the
kernel each time a customer comes with some new display is no fun.

> 
> > +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
> > +		struct fb_videomode *fbmode);
> 
> From caller's point of view I think it'd make more sense to have
> separate functions to get drm mode or fb mode. I don't think there's a
> case where the caller would want both.

Ok, that makes sense.

> 
> Then again, even better would be to have a common video mode struct used
> by both fb and drm... But I think that's been on the todo list of
> Laurent for a long time already =).

Yes, indeed. We should go into that direction. We already realized that
we want to have ranges instead of fixed values for the timings.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] of: Add videomode helper

[Laurent Pinchart]

Hi Sascha,

On Thursday 13 September 2012 13:19:54 Sascha Hauer wrote:
> On Thu, Sep 13, 2012 at 01:54:07PM +0300, Tomi Valkeinen wrote:
> > On Wed, 2012-07-04 at 09:56 +0200, Sascha Hauer wrote:
> > > This patch adds a helper function for parsing videomodes from the
> > > devicetree. The videomode can be either converted to a struct
> > > drm_display_mode or a struct fb_videomode.
> > 
> > I have more or less the same generic comment for this as for the power
> > sequences series discussed: this would add panel specific information
> > into DT data, instead of the driver handling it. But, as also discussed
> > in the thread, there are differing opinions on which way is better.
> 
> With the panel timings I think the approach of moving them into DT is
> the best we can do. There are so many displays out there, patching the
> kernel each time a customer comes with some new display is no fun.

For generic panels, sure, but for panels that require a specific driver (for 
instance because the panel implements vendor-specific comments) the mode(s) 
could be hardcoded in the panel driver.

> > > +int of_get_video_mode(struct device_node *np, struct drm_display_mode
> > > *dmode,
> > > +		struct fb_videomode *fbmode);
> > 
> > From caller's point of view I think it'd make more sense to have
> > separate functions to get drm mode or fb mode. I don't think there's a
> > case where the caller would want both.
> 
> Ok, that makes sense.
> 
> > Then again, even better would be to have a common video mode struct used
> > by both fb and drm... But I think that's been on the todo list of
> > Laurent for a long time already =).
> 
> Yes, indeed. We should go into that direction. We already realized that
> we want to have ranges instead of fixed values for the timings.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] of: Add videomode helper

[Guennadi Liakhovetski]

Hi Sascha

On Wed, 4 Jul 2012, Sascha Hauer wrote:

> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> changes since v1:
> - use hyphens instead of underscores for property names
> 
>  .../devicetree/bindings/video/displaymode          |   40 ++++++++
>  drivers/of/Kconfig                                 |    5 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/of_videomode.c                          |  108 ++++++++++++++++++++
>  include/linux/of_videomode.h                       |   19 ++++
>  5 files changed, 173 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/displaymode
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..43cc17d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,40 @@
> +videomode bindings
> +=========
> +
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high

How about

+ - hsync-active: Hsync pulse polarity: 1 for high, 0 for low

and similar for vsync-active? Which would then become

> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one used by the Linux Framebuffer framework described here in
> +Documentation/fb/framebuffer.txt. This representation has been chosen because it's
> +the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
> +framework the devicetree has the clock in Hz instead of ps.
> +
> +Example:
> +
> +	display@0 {
> +		/* 1920x1080p24 */
> +		clock = <52000000>;
> +		xres = <1920>;
> +		yres = <1080>;
> +		left-margin = <25>;
> +		right-margin = <25>;
> +		hsync-len = <25>;
> +		lower-margin = <2>;
> +		upper-margin = <2>;
> +		vsync-len = <2>;
> +		hsync-active-high;

+		hsync-active = <1>;

> +	};
> +

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] of: Add videomode helper

[Sascha Hauer]

Hi Guennadi,

On Wed, Jul 11, 2012 at 10:34:54AM +0200, Guennadi Liakhovetski wrote:
> Hi Sascha
> 
> > +
> > +Optional properties:
> > + - width-mm, height-mm: Display dimensions in mm
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> 
> How about
> 
> + - hsync-active: Hsync pulse polarity: 1 for high, 0 for low

I am unsure if it's good to mix this with the other bool flags like:

> 
> > + - interlaced (bool): This is an interlaced mode
> > + - doublescan (bool): This is a doublescan mode

Which behave differently. 'interlaced' will be evaluated as true if
the property is present, no matter which value it has. This might
lead to confusion.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] of: Add videomode helper

[Guennadi Liakhovetski]

On Wed, 11 Jul 2012, Sascha Hauer wrote:

> Hi Guennadi,
> 
> On Wed, Jul 11, 2012 at 10:34:54AM +0200, Guennadi Liakhovetski wrote:
> > Hi Sascha
> > 
> > > +
> > > +Optional properties:
> > > + - width-mm, height-mm: Display dimensions in mm
> > > + - hsync-active-high (bool): Hsync pulse is active high
> > > + - vsync-active-high (bool): Vsync pulse is active high
> > 
> > How about
> > 
> > + - hsync-active: Hsync pulse polarity: 1 for high, 0 for low
> 
> I am unsure if it's good to mix this with the other bool flags like:
> 
> > 
> > > + - interlaced (bool): This is an interlaced mode
> > > + - doublescan (bool): This is a doublescan mode
> 
> Which behave differently. 'interlaced' will be evaluated as true if
> the property is present, no matter which value it has. This might
> lead to confusion.

I don't feel strongly either way either. I don't think it'd be confusing - 
you have integer properties there too, and the logic in this case is not 
"active high - yes or now," but rather "active level - logical 1 (high) or 
0 (low)." But as I said - that was just an idea, unless someone has strong 
arguments either way - you're the original author, it's your call:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/