Hi,
(CC'ing the linux-media mailing list, as video modes are of interest there as
well)
On Wednesday 19 September 2012 12:19:22 Tomi Valkeinen wrote:
> On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> > ---
> >
> > Hi!
> >
> > changes since v3:
> > - print error messages
> > - free alloced memory
> > - general cleanup
> >
> > Regards
> > Steffen
> >
> > .../devicetree/bindings/video/displaymode | 74 +++++
> > drivers/of/Kconfig | 5 +
> > drivers/of/Makefile | 1 +
> > drivers/of/of_videomode.c | 283 +++++++++++++++
> > include/linux/of_videomode.h | 56 ++++
> > 5 files changed, 419 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..990ca52
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/displaymode
> > @@ -0,0 +1,74 @@
> > +videomode bindings
> > +==================
> > +
> > +Required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > parameters
> > + in pixels
> > + vfront-porch, vback-porch, 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 commonly found in datasheets for displays.
> > +The description of the display and its mode is split in two parts: first
> > the display
> > +properties like size in mm and (optionally) multiple subnodes with the
> > supported modes.
> > +
> > +Example:
> > +
> > + display@0 {
> > + width-mm = <800>;
> > + height-mm = <480>;
> > + modes {
> > + mode0: mode@0 {
> > + /* 1920x1080p24 */
> > + clock = <52000000>;
> > + hactive = <1920>;
> > + vactive = <1080>;
> > + hfront-porch = <25>;
> > + hback-porch = <25>;
> > + hsync-len = <25>;
> > + vback-porch = <2>;
> > + vfront-porch = <2>;
> > + vsync-len = <2>;
> > + hsync-active-high;
> > + };
> > + };
> > + };
> > +
> > +Every property also supports the use of ranges, so the commonly used
> > datasheet +description with <min typ max>-tuples can be used.
> > +
> > +Example:
> > +
> > + mode1: mode@1 {
> > + /* 1920x1080p24 */
> > + clock = <148500000>;
> > + hactive = <1920>;
> > + vactive = <1080>;
> > + hsync-len = <0 44 60>;
> > + hfront-porch = <80 88 95>;
> > + hback-porch = <100 148 160>;
> > + vfront-porch = <0 4 6>;
> > + vback-porch = <0 36 50>;
> > + vsync-len = <0 5 6>;
> > + };
> > +
> > +The videomode can be linked to a connector via phandles. The connector
> > has to
> > +support the display- and default-mode-property to link to and select a
> > mode.
> > +
> > +Example:
> > +
> > + hdmi@00120000 {
> > + status = "okay";
> > + display = <&benq>;
> > + default-mode = <&mode1>;
> > + };
> > +
> > 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..52bfc74
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,283 @@
> > +/*
> > + * OF helpers for parsing display modes
> > + *
> > + * Copyright (c) 2012 Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>, Pengutronix
> > + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>,
> > Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +#include <linux/of.h>
> > +#include <linux/fb.h>
> > +#include <linux/export.h>
> > +#include <linux/slab.h>
> > +#include <drm/drmP.h>
> > +#include <drm/drm_crtc.h>
> > +#include <linux/of_videomode.h>
> > +
> > +static u32 of_video_get_value(struct mode_property *prop)
> > +{
> > + return (prop->min >= prop->typ) ? prop->min : prop->typ;
> > +}
>
> Why is this needed? If the prop->min is higher than prop->typ, isn't
> that an error in the DT data, and should be rejected when parsing?
>
> > +
> > +/* read property into new mode_property */
> > +static int of_video_parse_property(struct device_node *np, char *name,
> > + struct mode_property *result)
> > +{
> > + struct property *prop;
> > + int length;
> > + int cells;
> > + int ret;
> > +
> > + prop = of_find_property(np, name, &length);
> > + if (!prop) {
> > + pr_err("%s: could not find property %s\n", __func__,
> > + name);
> > + return -EINVAL;
> > + }
> > +
> > + cells = length / sizeof(u32);
> > +
> > + memset(result, 0, sizeof(*result));
> > +
> > + ret = of_property_read_u32_array(np, name, &result->min, cells);
> > +
> > + return ret;
>
> Ah, I guess this is the reason for the of_video_get_value... Wouldn't it
> be cleaner to be more explicit here? If there's one cell, it's the
> typical value, otherwise if there are 3 cells, they are min/typ/max,
> else it's an error.
I agree. We should flag errors and fail instead of trying to fix them
silently. That's the only way to make sure that device tree authors will get
it right (or at least less wrong). We might need to accept wrong DT data later
to support buggy devices found in the wild, but we should not start that way.
> And I think the above also trashes memory if there happens to be 4+ cells.
>
> > +}
> > +
> > +static int of_video_free(struct display *disp)
> > +{
> > + int i;
> > +
> > + for (i=0; i<disp->num_modes; i++)
Spaces around = and < please.
> > + kfree(disp->modes[i]);
> > + kfree(disp->modes);
> > +
> > + return 0;
> > +}
> > +
> > +int videomode_to_display_mode(struct display *disp, struct
> > drm_display_mode *dmode, int index)
Shouldn't this use drm_mode_modeinfo instead of drm_display_mode ?
> > +{
> > + struct videomode *vm;
> > +
> > + memset(dmode, 0, sizeof(*dmode));
> > +
> > + if (index > disp->num_modes) {
> > + pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > disp->num_modes); + return -EINVAL;
> > + }
> > +
> > + vm = disp->modes[index];
> > +
> > + dmode->hdisplay = of_video_get_value(&vm->hactive);
> > + dmode->hsync_start = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch);
You could replace of_video_get_value(&vm->hactive) with dmode->hdisplay here,
and similarly below (hsync_end = hsync_start + of_video_get_value(&vm-
>hsync_len), ...). Beside shortening lines, it would save calls to
of_video_get_value().
> > + dmode->hsync_end = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch)
> > + + of_video_get_value(&vm->hsync_len);
> > + dmode->htotal = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch)
> > + + of_video_get_value(&vm->hsync_len) +
> > of_video_get_value(&vm->hback_porch);
> > + dmode->vdisplay = of_video_get_value(&vm->vactive);
> > + dmode->vsync_start = of_video_get_value(&vm->vactive) +
> > of_video_get_value(&vm->vfront_porch); + dmode->vsync_end =
> > of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> > + + of_video_get_value(&vm->vsync_len);
> > + dmode->vtotal = of_video_get_value(&vm->vactive) +
> > of_video_get_value(&vm->vfront_porch) +
> > + of_video_get_value(&vm->vsync_len) +
> > of_video_get_value(&vm->vback_porch); +
> > + dmode->width_mm = disp->width_mm;
> > + dmode->height_mm = disp->height_mm;
> > +
> > + dmode->clock = of_video_get_value(&vm->clock) / 1000;
> > +
> > + if (vm->hah)
> > + dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> > + else
> > + dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> > + if (vm->vah)
> > + dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> > + else
> > + dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> > + if (vm->interlaced)
> > + dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> > + if (vm->doublescan)
> > + dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> > +
> > + drm_mode_set_name(dmode);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> > +
> > +int videomode_to_fb_mode(struct display *disp, struct fb_videomode
> > *fbmode, int index)
disp is only used to retrieve the mode, you could pass a struct videomode
instead of disp and index.
Thinking about it, I would make videomode_to_display_mode take a struct
videomode as well. The only reason it needs struct display is to get the
display physical dimensions. Those are only used in very specific cases in the
DRM subsystem, I don't think they should be copied to every struct
drm_mode_modeinfo.
> > +{
> > + struct videomode *vm;
> > +
> > + memset(fbmode, 0, sizeof(*fbmode));
> > +
> > + if (index > disp->num_modes) {
> > + pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > disp->num_modes);
> > + return -EINVAL;
> > + }
> > +
> > + vm = disp->modes[index];
> > +
> > + fbmode->xres = of_video_get_value(&vm->hactive);
> > + fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> > + fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> > + fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> > +
> > + fbmode->yres = of_video_get_value(&vm->vactive);
> > + fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> > + fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> > + fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> > +
> > + fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> > +
> > + if (vm->hah)
> > + fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > + if (vm->vah)
> > + fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > + if (vm->interlaced)
> > + fbmode->vmode |= FB_VMODE_INTERLACED;
> > + if (vm->doublescan)
> > + fbmode->vmode |= FB_VMODE_DOUBLE;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> > +
> > +int of_get_video_modes(struct device_node *np, struct display *disp)
> > +{
> > + struct device_node *display_np;
> > + struct device_node *mode_np;
> > + struct device_node *modes_np;
> > + char *default_mode;
> > +
> > + int ret = 0;
> > +
> > + memset(disp, 0, sizeof(*disp));
> > + disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > +
> > + if (!np) {
> > + pr_err("%s: no node provided\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + display_np = of_parse_phandle(np, "display", 0);
> > +
> > + if (!display_np) {
> > + pr_err("%s: could not find display node\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> > + of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> > +
> > + mode_np = of_parse_phandle(np, "default-mode", 0);
> > +
> > + if (!mode_np) {
> > + pr_info("%s: no default-mode specified.\n", __func__);
> > + mode_np = of_find_node_by_name(np, "mode");
> > + }
> > +
> > + if (!mode_np) {
> > + pr_err("%s: could not find any mode specification\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + default_mode = (char *)mode_np->full_name;
> > +
> > + modes_np = of_find_node_by_name(np, "modes");
> > + for_each_child_of_node(modes_np, mode_np) {
> > + struct videomode *vm;
> > +
> > + vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > + disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*),
> > GFP_KERNEL);
> > +
> > + ret |= of_video_parse_property(mode_np, "hback-porch",
> > &vm->hback_porch);
> > + ret |= of_video_parse_property(mode_np, "hfront-porch",
> > &vm->hfront_porch);
> > + ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> > + ret |= of_video_parse_property(mode_np, "hsync-len",
> > &vm->hsync_len);
> > + ret |= of_video_parse_property(mode_np, "vback-porch",
> > &vm->vback_porch);
> > + ret |= of_video_parse_property(mode_np, "vfront-porch",
> > &vm->vfront_porch);
> > + ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> > + ret |= of_video_parse_property(mode_np, "vsync-len",
> > &vm->vsync_len);
> > + ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> > +
> > + if (ret)
> > + return -EINVAL;
> > +
> > + vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> > + vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> > + vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> > + vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> > +
> > + if (strcmp(default_mode,mode_np->full_name) == 0)
> > + disp->default_mode = disp->num_modes;
> > +
> > + disp->modes[disp->num_modes] = vm;
> > + disp->num_modes++;
> > + }
> > + of_node_put(display_np);
> > +
> > + pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> > + disp->num_modes, disp->default_mode + 1);
> > +
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_video_modes);
> > +
> > +int of_video_mode_exists(struct device_node *np)
> > +{
> > + struct device_node *display_np;
> > + struct device_node *mode_np;
> > +
> > + if (!np)
> > + return -EINVAL;
> > +
> > + display_np = of_parse_phandle(np, "display", 0);
> > +
> > + if (!display_np)
> > + return -EINVAL;
> > +
> > + mode_np = of_parse_phandle(np, "default-mode", 0);
> > +
> > + if (mode_np)
> > + return 0;
> > +
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> > +
> > +int of_get_display_mode(struct device_node *np, struct drm_display_mode
> > *dmode, int index)
> > +{
> > + struct display disp;
> > +
> > + of_get_video_modes(np, &disp);
> > +
> > + if (index == OF_MODE_SELECTION)
> > + index = disp.default_mode;
> > + if (dmode)
> > + videomode_to_display_mode(&disp, dmode, index);
> > +
> > + of_video_free(&disp);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_display_mode);
> > +
> > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode
> > *fbmode, int index)
> > +{
> > + struct display disp;
> > +
> > + of_get_video_modes(np, &disp);
> > +
> > + if (index == OF_MODE_SELECTION)
> > + index = disp.default_mode;
> > + if (fbmode)
> > + videomode_to_fb_mode(&disp, fbmode, index);
> > +
> > + of_video_free(&disp);
> > +
> > + return 0;
>
> This and of_get_display_mode() do not handle errors from
> of_get_video_modes() nor from videomode_to_xxx_mode(). And I don't see a
> reason for the if (fbmode) check (and the same for dmode), as there's no
> reason to call these functions except to get the video modes.
>
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
What are those two functions used for ? Aren't drivers expected to parse the
modes into a struct display and then operate on that structure, instead of
getting individual modes from the DT node ?
> > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > new file mode 100644
> > index 0000000..5571ce3
> > --- /dev/null
> > +++ b/include/linux/of_videomode.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright 2012 Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> > + *
> > + * OF helpers for videomodes.
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_OF_VIDEOMODE_H
> > +#define __LINUX_OF_VIDEOMODE_H
> > +
> > +#define OF_MODE_SELECTION -1
> > +
> > +struct mode_property {
> > + u32 min;
> > + u32 typ;
> > + u32 max;
> > +};
> > +
> > +struct display {
> > + u32 width_mm;
> > + u32 height_mm;
> > + struct videomode **modes;
> > + int default_mode;
> > + int num_modes;
default_mode and num_modes are non-negative, what about using an unsigned int
type for them ?
> > +};
> > +
> > +/* describe videomode in terms of hardware parameters */
> > +struct videomode {
> > + struct mode_property hback_porch;
> > + struct mode_property hfront_porch;
> > + struct mode_property hactive;
> > + struct mode_property hsync_len;
> > +
> > + struct mode_property vback_porch;
> > + struct mode_property vfront_porch;
> > + struct mode_property vactive;
> > + struct mode_property vsync_len;
> > +
> > + struct mode_property clock;
> > +
> > + bool hah;
> > + bool vah;
> > + bool interlaced;
> > + bool doublescan;
> > +};
>
> I think the names display and videomode are a bit too generic here, and
> could clash with names from kernel drivers. Also, the videomode is not
> really a videomode, but a "template" (or something) for videomode. A
> real videomode doesn't have min & max values, only the actual value.
>
> Perhaps these should be prefixed with "of_"? Then again, they are not
> really of specific either...
If feel this is an important topic.
A generic video mode structure is definitely needed, as well as helper
functions to convert between the new structure and existing video mode
structures (struct fb_videomode, struct drm_mode_modeinfo and struct
v4l2_bt_timings). The structure and the helper functions should be generic, as
the goal is to gradually replace subsystem-specific video mode structures
where possible (userspace APIs will still need to keep the old structures). We
thus need to drop the dependency on OF for everything but the DT parsing code.
For that reason an of_ prefix wouldn't be a good idea. As the goal is to
create a truly generic video mode structure, a generic name is a good idea (I
might prefer struct video_mode instead of struct videomode, but that's just
bikeshedding).
We might need two video mode structures, one to represent a video mode, and
one to represent a range of video modes. I don't have enough experience with
video modes to have a really strong opinion on this, having a single structure
to represent both could be useful as well. This particular topic needs to be
discussed.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
