| Message ID | 1341388595-30672-1-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > + }; > +
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 */ >
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
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
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/
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
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/
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.
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.
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
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.
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; > }
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.
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
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
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.
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 */
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