| Message ID | 1267307902-31939-2-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Anatolij, [added cc: to devicetree-discuss@lists.ozlabs.org] On Sat, Feb 27, 2010 at 2:58 PM, Anatolij Gustschin <agust@denx.de> wrote: > Framebuffer drivers may want to get panel timing info > from the device tree. This patch adds appropriate support. > Subsequent patch for FSL DIU frame buffer driver makes use > of this functionality. I think this is moving in the right direction, but there needs to debate & review over the binding before committing to anything. Please write a patch that documents the new binding in Documentation/powerpc/dts-bindings. All new bindings should be documented and reviewed on devicetree-discuss before merging any drivers that use them into mainline. From what I can tell by reading your code, I suspect that the binding you've designed will solve your immediate problem, but won't be able to handle anything slightly more complex, but it also looks like the binding has been designed to be generic, usable by any display device. First off, I did a tiny amount of research, and I didn't find any existing OpenFirmware bindings for describing video displays. Otherwise, I'd suggest considering that. From the little bit that I know, it seems that for most video devices (ie. PCs) the video card discovers the capabilities of the screen by reading the monitor's EDID data. However, in your particular case embedded case, a fixed flat panel is attached, and there isn't any EDID data provided. Therefore, you need an alternate method of describing the display capabilities. Rather than designing something entirely new, you may want to consider using the EDID data format directly; or at least cover the same things that EDID describes. The downside to using EDID directly is that it is a packed binary format that isn't parseable by mere mortals; but the data contained in it seems about right. The upside is the kernel already knows what to do with EDID data. Otherwise you risk designing something that won't be useful for anything much outside of your own use case. For example, the binding I see from the code cannot handle a display with multiple output modes. Also, since you're now in the realm of describing a video display, which is separate from the display controller, you should consider describing the display in a separate device tree node. Maybe something like this... video { compatible = "fsl,mpc5121-diu"; display { compatible = "<vendor>,<model>"; edid = [edid-data]; }; }; Cheers, g. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > drivers/video/Kconfig | 5 +++ > drivers/video/Makefile | 1 + > drivers/video/ofmode.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/video/ofmode.h | 6 +++ > 4 files changed, 107 insertions(+), 0 deletions(-) > create mode 100644 drivers/video/ofmode.c > create mode 100644 drivers/video/ofmode.h > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > index 5a5c303..dc1beb0 100644 > --- a/drivers/video/Kconfig > +++ b/drivers/video/Kconfig > @@ -203,6 +203,11 @@ config FB_MACMODES > depends on FB > default n > > +config FB_OF_MODE > + tristate > + depends on FB && OF > + default n > + > config FB_BACKLIGHT > bool > depends on FB > diff --git a/drivers/video/Makefile b/drivers/video/Makefile > index 4ecb30c..c4a912b 100644 > --- a/drivers/video/Makefile > +++ b/drivers/video/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_FB_SVGALIB) += svgalib.o > obj-$(CONFIG_FB_MACMODES) += macmodes.o > obj-$(CONFIG_FB_DDC) += fb_ddc.o > obj-$(CONFIG_FB_DEFERRED_IO) += fb_defio.o > +obj-$(CONFIG_FB_OF_MODE) += ofmode.o > > # Hardware specific drivers go first > obj-$(CONFIG_FB_AMIGA) += amifb.o c2p_planar.o > diff --git a/drivers/video/ofmode.c b/drivers/video/ofmode.c > new file mode 100644 > index 0000000..78caad1 > --- /dev/null > +++ b/drivers/video/ofmode.c > @@ -0,0 +1,95 @@ > +/* > + * Get video mode settings from Flattened Device Tree. > + * > + * Copyright (C) 2010 DENX Software Engineering. > + * Anatolij Gustschin <agust@denx.de> > + * > + * This file is subject to the terms and conditions of the GNU General > + * Public License version 2. See the file COPYING in the main directory > + * of this archive for more details. > + */ > + > +#include <linux/fb.h> > +#include <linux/of.h> > + > +int __devinit of_get_video_mode(struct device_node *np, > + struct fb_info *info) > +{ > + struct fb_videomode dt_mode; > + const u32 *prop; > + unsigned int len; > + > + if (!np || !info) > + return -EINVAL; > + > + prop = of_get_property(np, "width", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.xres = *prop; > + > + prop = of_get_property(np, "height", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.yres = *prop; > + > + prop = of_get_property(np, "depth", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + info->var.bits_per_pixel = *prop; > + > + prop = of_get_property(np, "linebytes", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + info->fix.line_length = *prop; > + > + prop = of_get_property(np, "refresh", &len); > + if (prop && len == sizeof(u32)) > + dt_mode.refresh = *prop; /* optional */ > + > + prop = of_get_property(np, "pixclock", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.pixclock = *prop; > + > + prop = of_get_property(np, "left_margin", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.left_margin = *prop; > + > + prop = of_get_property(np, "right_margin", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.right_margin = *prop; > + > + prop = of_get_property(np, "upper_margin", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.upper_margin = *prop; > + > + prop = of_get_property(np, "lower_margin", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.lower_margin = *prop; > + > + prop = of_get_property(np, "hsync_len", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.hsync_len = *prop; > + > + prop = of_get_property(np, "vsync_len", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.vsync_len = *prop; > + > + prop = of_get_property(np, "sync", &len); > + if (!prop || len != sizeof(u32)) > + goto err; > + dt_mode.sync = *prop; > + > + fb_videomode_to_var(&info->var, &dt_mode); > + > + return 0; > +err: > + pr_err("%s: Can't find expected mode entry\n", np->full_name); > + return -EINVAL; > +} > diff --git a/drivers/video/ofmode.h b/drivers/video/ofmode.h > new file mode 100644 > index 0000000..9a13bec > --- /dev/null > +++ b/drivers/video/ofmode.h > @@ -0,0 +1,6 @@ > +#ifndef _OFMODE_H > +#define _OFMODE_H > + > +extern int __devinit of_get_video_mode(struct device_node *np, > + struct fb_info *info); > +#endif > -- > 1.6.3.3 > >
> > Hi Anatolij, > > [added cc: to devicetree-discuss@lists.ozlabs.org] > > On Sat, Feb 27, 2010 at 2:58 PM, Anatolij Gustschin <agust@denx.de> wrote: > >> > Framebuffer drivers may want to get panel timing info >> > from the device tree. This patch adds appropriate support. >> > Subsequent patch for FSL DIU frame buffer driver makes use >> > of this functionality. >> > > I think this is moving in the right direction, but there needs to > debate & review over the binding before committing to anything. > Please write a patch that documents the new binding in > Documentation/powerpc/dts-bindings. All new bindings should be > documented and reviewed on devicetree-discuss before merging any > drivers that use them into mainline. > > From what I can tell by reading your code, I suspect that the binding > you've designed will solve your immediate problem, but won't be able > to handle anything slightly more complex, but it also looks like the > binding has been designed to be generic, usable by any display device. > > First off, I did a tiny amount of research, and I didn't find any > existing OpenFirmware bindings for describing video displays. > Otherwise, I'd suggest considering that. > > From the little bit that I know, it seems that for most video devices > (ie. PCs) the video card discovers the capabilities of the screen by > reading the monitor's EDID data. However, in your particular case > embedded case, a fixed flat panel is attached, and there isn't any > EDID data provided. Therefore, you need an alternate method of > describing the display capabilities. Rather than designing something > entirely new, you may want to consider using the EDID data format > directly; or at least cover the same things that EDID describes. The > downside to using EDID directly is that it is a packed binary format > that isn't parseable by mere mortals; but the data contained in it > seems about right. The upside is the kernel already knows what to do > with EDID data. > > Otherwise you risk designing something that won't be useful for > anything much outside of your own use case. For example, the binding > I see from the code cannot handle a display with multiple output > modes. > > Also, since you're now in the realm of describing a video display, > which is separate from the display controller, you should consider > describing the display in a separate device tree node. Maybe > something like this... > > video { > compatible = "fsl,mpc5121-diu"; > display { > compatible = "<vendor>,<model>"; > edid = [edid-data]; > }; > }; > As it turns out, I'm doing exactly that - exporting verbatim EDID data as the value of the "edid" property - for the display node on the Via version of the OLPC machine. The kernel driver uses it instead of trying to obtain the EDID data from the monitor, because the builtin OLPC display cannot supply EDID data through the usual hardware interfaces. Mitch > Cheers, > g. > >
On Sun, Feb 28, 2010 at 1:44 AM, Mitch Bradley <wmb@firmworks.com> wrote: > Grant Likely Wrote: >> On Sat, Feb 27, 2010 at 2:58 PM, Anatolij Gustschin <agust@denx.de> wrote: >> Also, since you're now in the realm of describing a video display, >> which is separate from the display controller, you should consider >> describing the display in a separate device tree node. Maybe >> something like this... >> >> video { >> compatible = "fsl,mpc5121-diu"; >> display { >> compatible = "<vendor>,<model>"; >> edid = [edid-data]; >> }; >> }; >> > > > As it turns out, I'm doing exactly that - exporting verbatim EDID data as > the value of the "edid" property - for the display node on the Via version > of the OLPC machine. The kernel driver uses it instead of trying to obtain > the EDID data from the monitor, because the builtin OLPC display cannot > supply EDID data through the usual hardware interfaces. Cool. That sounds sane then. How do you go about generating the EDID data? Is there a tool that can do that? Or did you hand craft it? Cheers, g.
At some stage, Grant wrote: > > First off, I did a tiny amount of research, and I didn't find any > > existing OpenFirmware bindings for describing video displays. > > Otherwise, I'd suggest considering that. There's a binding for framebuffers but it sucks big time :-) It doesn't provide a reliable way for the OS to get to the physical address of the fb, doesn't define outputs and mode setting, doesn't really deal with >8bpp, etc.... On Sat, 2010-02-27 at 22:44 -1000, Mitch Bradley wrote: > As it turns out, I'm doing exactly that - exporting verbatim EDID > data > as the value of the "edid" property - for the display node on the Via > version of the OLPC machine. The kernel driver uses it instead of > trying to obtain the EDID data from the monitor, because the builtin > OLPC display cannot supply EDID data through the usual hardware > interfaces. This is actually a common practice (though EDID is most often in uppercase) on Apple hardware too. It has issues though in the sense that it doesn't carry proper connector information and falls over in many multi-head cases. I think passing the EDID data, when available, is thus the right thing to do indeed, however that doesn't solve two problems: - Where to put that property ? This is a complicated problem and we might argue on a binding for weeks because video cards typically support multiple outputs, etc. etc... I think the best for now is to stick as closely as possible to the existing OF fb binding, and have "display" nodes for each output, which can eventually contain an EDID. We might also want to add a string that indicate the connector type. Specific drivers might want to define additional properties here where it makes sense such as binding of those outputs to CRTCs or such, up to you. - We -also- want a way to specify the "default" mode as set by the firmware, at least on some devices. The EDID gives capabilities, and often for LCDs also the "preferred" mode which is almost always the "default" mode ... but could be different. In order to avoid "flicking", the driver might wants to know what is the currently programmed mode. For that, having split out properties makes sense, though I would like to either prefix them all with "mode," or stick them in a sub-node of the display@. Cheers, Ben.
On Mon, 01 Mar 2010 14:45:20 +1100 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Sat, 2010-02-27 at 22:44 -1000, Mitch Bradley wrote: > > > As it turns out, I'm doing exactly that - exporting verbatim EDID > > data > > as the value of the "edid" property - for the display node on the Via > > version of the OLPC machine. The kernel driver uses it instead of > > trying to obtain the EDID data from the monitor, because the builtin > > OLPC display cannot supply EDID data through the usual hardware > > interfaces. > > This is actually a common practice (though EDID is most often in > uppercase) on Apple hardware too. It has issues though in the sense that > it doesn't carry proper connector information and falls over in many > multi-head cases. > > I think passing the EDID data, when available, is thus the right thing > to do indeed, however that doesn't solve two problems: > > - Where to put that property ? This is a complicated problem and we > might argue on a binding for weeks because video cards typically support > multiple outputs, etc. etc... I think the best for now is to stick as > closely as possible to the existing OF fb binding, and have "display" > nodes for each output, which can eventually contain an EDID. We might > also want to add a string that indicate the connector type. Specific > drivers might want to define additional properties here where it makes > sense such as binding of those outputs to CRTCs or such, up to you. Putting EDID to display node would be really sufficient for LCDs in our case. Other systems might define this additional connector type property. > > - We -also- want a way to specify the "default" mode as set by the > firmware, at least on some devices. The EDID gives capabilities, and > often for LCDs also the "preferred" mode which is almost always the > "default" mode ... but could be different. In order to avoid "flicking", > the driver might wants to know what is the currently programmed mode. > For that, having split out properties makes sense, though I would like > to either prefix them all with "mode," or stick them in a sub-node of > the display@. I would propose defining following properties in the case the programmed mode is different from "default" mode: display@...{ compatible = "<vendor>,<model>" EDID = [edid-data]; current-mode { pixel_clock = <value>; horizontal_active = <value>; horizontal_blank = <value>; vertical_active = <value>; vertical_blank = <value>; horizontal_active = <value>; hsync_offset = <value>; hsync_pulse_width = <value>; vsync_offset = <value>; vsync_pulse_width = <value>; hsync_positive; vsync_positive; } }; The firmware can set the "default" mode using the EDID's preferred Detailed Timing Descriptor data. If on some devices it sets another mode than the preferred mode, then the firmware can insert a "current-mode" sub-node with currently programmed mode. The driver can check for this sub-node and use it's data and if it isn't present, it can use the preferred timing data from EDID. The names of the properties here are actually what Detailed Timing Descriptor in EDID specifies. What do you think? Thanks, Anatolij
On Wed, Apr 28, 2010 at 7:43 AM, Anatolij Gustschin <agust@denx.de> wrote: > On Mon, 01 Mar 2010 14:45:20 +1100 > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > >> On Sat, 2010-02-27 at 22:44 -1000, Mitch Bradley wrote: >> >> > As it turns out, I'm doing exactly that - exporting verbatim EDID >> > data >> > as the value of the "edid" property - for the display node on the Via >> > version of the OLPC machine. The kernel driver uses it instead of >> > trying to obtain the EDID data from the monitor, because the builtin >> > OLPC display cannot supply EDID data through the usual hardware >> > interfaces. >> >> This is actually a common practice (though EDID is most often in >> uppercase) on Apple hardware too. It has issues though in the sense that >> it doesn't carry proper connector information and falls over in many >> multi-head cases. >> >> I think passing the EDID data, when available, is thus the right thing >> to do indeed, however that doesn't solve two problems: >> >> - Where to put that property ? This is a complicated problem and we >> might argue on a binding for weeks because video cards typically support >> multiple outputs, etc. etc... I think the best for now is to stick as >> closely as possible to the existing OF fb binding, and have "display" >> nodes for each output, which can eventually contain an EDID. We might >> also want to add a string that indicate the connector type. Specific >> drivers might want to define additional properties here where it makes >> sense such as binding of those outputs to CRTCs or such, up to you. > > Putting EDID to display node would be really sufficient for LCDs in > our case. Other systems might define this additional connector type > property. > >> >> - We -also- want a way to specify the "default" mode as set by the >> firmware, at least on some devices. The EDID gives capabilities, and >> often for LCDs also the "preferred" mode which is almost always the >> "default" mode ... but could be different. In order to avoid "flicking", >> the driver might wants to know what is the currently programmed mode. >> For that, having split out properties makes sense, though I would like >> to either prefix them all with "mode," or stick them in a sub-node of >> the display@. > > I would propose defining following properties in the case the > programmed mode is different from "default" mode: > > display@...{ > compatible = "<vendor>,<model>" > EDID = [edid-data]; > > current-mode { > pixel_clock = <value>; > horizontal_active = <value>; > horizontal_blank = <value>; > vertical_active = <value>; > vertical_blank = <value>; > horizontal_active = <value>; > hsync_offset = <value>; > hsync_pulse_width = <value>; > vsync_offset = <value>; > vsync_pulse_width = <value>; > hsync_positive; > vsync_positive; > } > }; > > The firmware can set the "default" mode using the EDID's preferred > Detailed Timing Descriptor data. If on some devices it sets another > mode than the preferred mode, then the firmware can insert a > "current-mode" sub-node with currently programmed mode. The driver > can check for this sub-node and use it's data and if it isn't present, > it can use the preferred timing data from EDID. The names of the > properties here are actually what Detailed Timing Descriptor in EDID > specifies. What do you think? If you really want to do that, then I think it is okay. I really don't know enough about the problem space to say whether or not that particular description is a good binding or not, but regardless it should be a video-controller-specific binding. The name of the node should probably be prefixed with "<manufacturer>," and it should be documented along with the display controller's device tree binding. If another controller wants/needs a different binding format (for the current mode), then that is fine too (unless you can make a really good argument that this current-mode binding is perfect and no other layout should ever be required). :-) Cheers, g. > > Thanks, > Anatolij >
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 5a5c303..dc1beb0 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -203,6 +203,11 @@ config FB_MACMODES depends on FB default n +config FB_OF_MODE + tristate + depends on FB && OF + default n + config FB_BACKLIGHT bool depends on FB diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 4ecb30c..c4a912b 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_FB_SVGALIB) += svgalib.o obj-$(CONFIG_FB_MACMODES) += macmodes.o obj-$(CONFIG_FB_DDC) += fb_ddc.o obj-$(CONFIG_FB_DEFERRED_IO) += fb_defio.o +obj-$(CONFIG_FB_OF_MODE) += ofmode.o # Hardware specific drivers go first obj-$(CONFIG_FB_AMIGA) += amifb.o c2p_planar.o diff --git a/drivers/video/ofmode.c b/drivers/video/ofmode.c new file mode 100644 index 0000000..78caad1 --- /dev/null +++ b/drivers/video/ofmode.c @@ -0,0 +1,95 @@ +/* + * Get video mode settings from Flattened Device Tree. + * + * Copyright (C) 2010 DENX Software Engineering. + * Anatolij Gustschin <agust@denx.de> + * + * This file is subject to the terms and conditions of the GNU General + * Public License version 2. See the file COPYING in the main directory + * of this archive for more details. + */ + +#include <linux/fb.h> +#include <linux/of.h> + +int __devinit of_get_video_mode(struct device_node *np, + struct fb_info *info) +{ + struct fb_videomode dt_mode; + const u32 *prop; + unsigned int len; + + if (!np || !info) + return -EINVAL; + + prop = of_get_property(np, "width", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.xres = *prop; + + prop = of_get_property(np, "height", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.yres = *prop; + + prop = of_get_property(np, "depth", &len); + if (!prop || len != sizeof(u32)) + goto err; + info->var.bits_per_pixel = *prop; + + prop = of_get_property(np, "linebytes", &len); + if (!prop || len != sizeof(u32)) + goto err; + info->fix.line_length = *prop; + + prop = of_get_property(np, "refresh", &len); + if (prop && len == sizeof(u32)) + dt_mode.refresh = *prop; /* optional */ + + prop = of_get_property(np, "pixclock", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.pixclock = *prop; + + prop = of_get_property(np, "left_margin", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.left_margin = *prop; + + prop = of_get_property(np, "right_margin", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.right_margin = *prop; + + prop = of_get_property(np, "upper_margin", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.upper_margin = *prop; + + prop = of_get_property(np, "lower_margin", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.lower_margin = *prop; + + prop = of_get_property(np, "hsync_len", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.hsync_len = *prop; + + prop = of_get_property(np, "vsync_len", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.vsync_len = *prop; + + prop = of_get_property(np, "sync", &len); + if (!prop || len != sizeof(u32)) + goto err; + dt_mode.sync = *prop; + + fb_videomode_to_var(&info->var, &dt_mode); + + return 0; +err: + pr_err("%s: Can't find expected mode entry\n", np->full_name); + return -EINVAL; +} diff --git a/drivers/video/ofmode.h b/drivers/video/ofmode.h new file mode 100644 index 0000000..9a13bec --- /dev/null +++ b/drivers/video/ofmode.h @@ -0,0 +1,6 @@ +#ifndef _OFMODE_H +#define _OFMODE_H + +extern int __devinit of_get_video_mode(struct device_node *np, + struct fb_info *info); +#endif
Framebuffer drivers may want to get panel timing info from the device tree. This patch adds appropriate support. Subsequent patch for FSL DIU frame buffer driver makes use of this functionality. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- drivers/video/Kconfig | 5 +++ drivers/video/Makefile | 1 + drivers/video/ofmode.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/video/ofmode.h | 6 +++ 4 files changed, 107 insertions(+), 0 deletions(-) create mode 100644 drivers/video/ofmode.c create mode 100644 drivers/video/ofmode.h