Re: [PATCH 1/3] video: add support for getting video mode from device tree

Re: [PATCH 1/3] video: add support for getting video mode from device tree

[Grant Likely]

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
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/3] video: add support for getting video mode from device tree

[Mitch Bradley]

>
> Hi Anatolij,
>
> [added cc: to devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org]
>
> On Sat, Feb 27, 2010 at 2:58 PM, Anatolij Gustschin <agust-ynQEQJNshbs@public.gmane.org> 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.
>
>   

Re: [PATCH 1/3] video: add support for getting video mode from device tree

[Grant Likely]

On Sun, Feb 28, 2010 at 1:44 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> Grant Likely Wrote:
>> On Sat, Feb 27, 2010 at 2:58 PM, Anatolij Gustschin <agust-ynQEQJNshbs@public.gmane.org> 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.

Re: [PATCH 1/3] video: add support for getting video mode from device tree

[Benjamin Herrenschmidt]

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.

Re: [PATCH 1/3] video: add support for getting video mode from device tree

[Anatolij Gustschin]

On Mon, 01 Mar 2010 14:45:20 +1100
Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.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

Re: [PATCH 1/3] video: add support for getting video mode from device tree

[Grant Likely]

On Wed, Apr 28, 2010 at 7:43 AM, Anatolij Gustschin <agust-ynQEQJNshbs@public.gmane.org> wrote:
> On Mon, 01 Mar 2010 14:45:20 +1100
> Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.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
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.