| Message ID | 1381947912-11741-3-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thierry, Thank you for the patch. On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote: > Add a driver for simple panels. Such panels can have a regulator that > provides the supply voltage and a separate GPIO to enable the panel. > Optionally the panels can have a backlight associated with them so it > can be enabled or disabled according to the panel's power management > mode. > > Support is added for three panels: An AU Optronics 10.1" WSVGA, a > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD > panel. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > .../devicetree/bindings/panel/auo,b101aw03.txt | 7 + > .../bindings/panel/chunghwa,claa101wb03.txt | 7 + > .../bindings/panel/panasonic,vvx10f004b00.txt | 7 + > .../devicetree/bindings/panel/simple-panel.txt | 21 ++ > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/panel/Kconfig | 13 + > drivers/gpu/drm/panel/Makefile | 1 + > drivers/gpu/drm/panel/panel-simple.c | 335 ++++++++++++++++++ > 8 files changed, 392 insertions(+) > create mode 100644 Documentation/devicetree/bindings/panel/auo,b101aw03.txt > create mode 100644 > Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt create > mode 100644 > Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt create > mode 100644 Documentation/devicetree/bindings/panel/simple-panel.txt create > mode 100644 drivers/gpu/drm/panel/Makefile > create mode 100644 drivers/gpu/drm/panel/panel-simple.c > > diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt > b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt new file mode > 100644 > index 0000000..72e088a > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt > @@ -0,0 +1,7 @@ > +AU Optronics Corporation 10.1" WSVGA TFT LCD panel > + > +Required properties: > +- compatible: should be "auo,b101aw03" > + > +This binding is compatible with the simple-panel binding, which is > specified +in simple-panel.txt in this directory. > diff --git > a/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt > b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt new file > mode 100644 > index 0000000..0ab2c05 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt > @@ -0,0 +1,7 @@ > +Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel > + > +Required properties: > +- compatible: should be "chunghwa,claa101wb03" > + > +This binding is compatible with the simple-panel binding, which is > specified +in simple-panel.txt in this directory. > diff --git > a/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt > b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt new > file mode 100644 > index 0000000..d328b03 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt > @@ -0,0 +1,7 @@ > +Panasonic Corporation 10.1" WUXGA TFT LCD panel > + > +Required properties: > +- compatible: should be "panasonic,vvx10f004b00" > + > +This binding is compatible with the simple-panel binding, which is > specified +in simple-panel.txt in this directory. > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode > 100644 > index 0000000..1341bbf > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt > @@ -0,0 +1,21 @@ > +Simple display panel > + > +Required properties: > +- power-supply: regulator to provide the supply voltage > + > +Optional properties: > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing > +- enable-gpios: GPIO pin to enable or disable the panel > +- backlight: phandle of the backlight device attached to the panel > + > +Example: > + > + panel: panel { > + compatible = "cptt,claa101wb01"; > + ddc-i2c-bus = <&panelddc>; > + > + power-supply = <&vdd_pnl_reg>; > + enable-gpios = <&gpio 90 0>; > + > + backlight = <&backlight>; > + }; My biggest concern here is that this will not be compatible with the CDF DT bindings. They're not complete yet, but they will require connections between entities to be described in DT, in a way very similar (or actually identical) to the V4L2 DT bindings, documented in Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a look at that ? Please ignore all optional properties beside remote-endpoint, as they're V4L2 specific. I also plan to specify video bus parameters in DT for CDF, but this hasn't been finalized yet. > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 81363a9..a9859e0 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -58,3 +58,4 @@ obj-$(CONFIG_DRM_QXL) += qxl/ > obj-$(CONFIG_DRM_MSM) += msm/ > obj-$(CONFIG_DRM_TEGRA) += tegra/ > obj-y += i2c/ > +obj-y += panel/ > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index 2ddd5bd..843087b 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -2,3 +2,16 @@ menuconfig DRM_PANEL > bool "DRM panel support" > help > Panel registration and lookup framework. > + > +if DRM_PANEL > + > +config DRM_PANEL_SIMPLE > + bool "support for simple panels" > + depends on OF > + help > + DRM panel driver for dumb panels that need at most a regulator and > + a GPIO to be powered up. Optionally a backlight can be attached so > + that it can be automatically turned off when the panel goes into a > + low power state. > + > +endif > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > new file mode 100644 > index 0000000..af9dfa2 > --- /dev/null > +++ b/drivers/gpu/drm/panel/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o > diff --git a/drivers/gpu/drm/panel/panel-simple.c > b/drivers/gpu/drm/panel/panel-simple.c new file mode 100644 > index 0000000..def3e75 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -0,0 +1,335 @@ [snip] > +/* > + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the > "Software"), > + * to deal in the Software without restriction, including without > limitation > + * the rights to use, copy, modify, merge, publish, distribute, sub > license, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the > + * next paragraph) shall be included in all copies or substantial portions > + * of the Software. This contradicts MODULE_LICENSE("GPL v2") below. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > + > +#include <linux/backlight.h> > +#include <linux/gpio.h> > +#include <linux/module.h> > +#include <linux/of_gpio.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +#include <drm/drm_crtc.h> > +#include <drm/drm_panel.h> > + > +struct panel_desc { > + const struct drm_display_mode *modes; > + unsigned int num_modes; > + > + struct { > + unsigned int width; > + unsigned int height; > + } size; > +}; > + > +/* TODO: convert to gpiod_*() API once it's been merged */ > +#define GPIO_ACTIVE_LOW (1 << 0) Why can't you just #include <dt-bindings/gpio/gpio.h> ? > + > +struct panel_simple { > + struct drm_panel base; > + bool enabled; > + > + const struct panel_desc *desc; > + > + struct backlight_device *backlight; > + struct regulator *supply; > + > + unsigned long enable_gpio_flags; > + int enable_gpio; > +}; > + > +static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) > +{ > + return container_of(panel, struct panel_simple, base); > +} > + > +static void panel_simple_disable(struct drm_panel *panel) > +{ > + struct panel_simple *p = to_panel_simple(panel); > + > + if (!p->enabled) > + return; > + > + if (p->backlight) { > + p->backlight->props.power = FB_BLANK_POWERDOWN; At some point we should decouple the backlight API from the fbdev API. That's somewhere on my to-do list after completing CDF (I'd be happy to trade CDF for that task ;-)). > + backlight_update_status(p->backlight); > + } > + > + if (gpio_is_valid(p->enable_gpio)) { > + if (p->enable_gpio_flags & GPIO_ACTIVE_LOW) > + gpio_set_value(p->enable_gpio, 1); > + else > + gpio_set_value(p->enable_gpio, 0); > + } > + > + regulator_disable(p->supply); > + p->enabled = false; > +} > + > +static void panel_simple_enable(struct drm_panel *panel) > +{ > + struct panel_simple *p = to_panel_simple(panel); > + int err; > + > + if (p->enabled) > + return; > + > + err = regulator_enable(p->supply); > + if (err < 0) > + dev_err(panel->dev, "failed to enable supply: %d\n", err); Is that really a non-fatal error ? Shouldn't the enable operation return an int ? > + > + if (gpio_is_valid(p->enable_gpio)) { > + if (p->enable_gpio_flags & GPIO_ACTIVE_LOW) > + gpio_set_value(p->enable_gpio, 0); > + else > + gpio_set_value(p->enable_gpio, 1); > + } > + > + if (p->backlight) { > + p->backlight->props.power = FB_BLANK_UNBLANK; > + backlight_update_status(p->backlight); > + } > + > + p->enabled = true; > +} > + > +static int panel_simple_get_modes(struct drm_panel *panel) > +{ > + struct panel_simple *p = to_panel_simple(panel); > + struct drm_display_mode *mode; > + unsigned int i; > + > + for (i = 0; i < p->desc->num_modes; i++) { > + mode = drm_mode_duplicate(panel->drm, &p->desc->modes[i]); > + if (!mode) > + return -ENOMEM; > + > + drm_mode_set_name(mode); > + > + drm_mode_probed_add(panel->connector, mode); > + } > + > + return p->desc->num_modes; > +} > + > +static const struct drm_panel_funcs panel_simple_funcs = { > + .disable = panel_simple_disable, > + .enable = panel_simple_enable, > + .get_modes = panel_simple_get_modes, > +}; > + > +static const struct drm_display_mode auo_b101aw03_mode = { > + .clock = 51450, > + .hdisplay = 1024, > + .hsync_start = 1024 + 156, > + .hsync_end = 1024 + 156 + 8, > + .htotal = 1024 + 156 + 8 + 156, > + .vdisplay = 600, > + .vsync_start = 600 + 16, > + .vsync_end = 600 + 16 + 6, > + .vtotal = 600 + 16 + 6 + 16, > + .vrefresh = 60, > +}; Instead of hardcoding the modes in the driver, which would then require to be updated for every new simple panel model (and we know there are lots of them), why don't you specify the modes in the panel DT node ? The simple panel driver would then become much more generic. It would also allow board designers to tweak h/v sync timings depending on the system requirements. > + > +static const struct panel_desc auo_b101aw03 = { > + .modes = &auo_b101aw03_mode, > + .num_modes = 1, > + .size = { > + .width = 223, > + .height = 125, > + }, > +}; > + > +static const struct drm_display_mode chunghwa_claa101wb01_mode = { > + .clock = 69300, > + .hdisplay = 1366, > + .hsync_start = 1366 + 48, > + .hsync_end = 1366 + 48 + 32, > + .htotal = 1366 + 48 + 32 + 20, > + .vdisplay = 768, > + .vsync_start = 768 + 16, > + .vsync_end = 768 + 16 + 8, > + .vtotal = 768 + 16 + 8 + 16, > + .vrefresh = 60, > +}; > + > +static const struct panel_desc chunghwa_claa101wb01 = { > + .modes = &chunghwa_claa101wb01_mode, > + .num_modes = 1, > + .size = { > + .width = 223, > + .height = 125, > + }, > +}; > + > +static const struct drm_display_mode panasonic_vvx10f004b00_mode = { > + .clock = 154700, > + .hdisplay = 1920, > + .hsync_start = 1920 + 154, > + .hsync_end = 1920 + 154 + 16, > + .htotal = 1920 + 154 + 16 + 32, > + .vdisplay = 1200, > + .vsync_start = 1200 + 17, > + .vsync_end = 1200 + 17 + 2, > + .vtotal = 1200 + 17 + 2 + 16, > + .vrefresh = 60, > +}; > + > +static const struct panel_desc panasonic_vvx10f004b00 = { > + .modes = &panasonic_vvx10f004b00_mode, > + .num_modes = 1, > + .size = { > + .width = 217, > + .height = 136, > + }, > +}; > + > +static const struct of_device_id panel_simple_of_match[] = { > + { > + .compatible = "auo,b101aw03", > + .data = &auo_b101aw03, > + }, { > + .compatible = "chunghwa,claa101wb01", > + .data = &chunghwa_claa101wb01 > + }, { > + .compatible = "panasonic,vvx10f004b00", > + .data = &panasonic_vvx10f004b00 > + }, { > + /* sentinel */ > + } > +}; > +MODULE_DEVICE_TABLE(of, panel_simple_of_match); > + > +static int panel_simple_probe(struct platform_device *pdev) > +{ > + const struct of_device_id *id; > + struct device_node *backlight; > + struct panel_simple *panel; > + enum of_gpio_flags flags; > + int err; > + > + panel = devm_kzalloc(&pdev->dev, sizeof(*panel), GFP_KERNEL); > + if (!panel) > + return -ENOMEM; > + > + id = of_match_node(panel_simple_of_match, pdev->dev.of_node); > + if (!id) > + return -ENODEV; > + > + panel->desc = id->data; > + panel->enabled = false; > + > + panel->supply = devm_regulator_get(&pdev->dev, "power"); > + if (IS_ERR(panel->supply)) > + return PTR_ERR(panel->supply); > + > + panel->enable_gpio = of_get_named_gpio_flags(pdev->dev.of_node, > + "enable-gpios", 0, > + &flags); > + if (gpio_is_valid(panel->enable_gpio)) { > + unsigned int value; > + > + if (flags & OF_GPIO_ACTIVE_LOW) > + panel->enable_gpio_flags |= GPIO_ACTIVE_LOW; > + > + err = gpio_request(panel->enable_gpio, "enable"); > + if (err < 0) { > + dev_err(&pdev->dev, "failed to request GPIO#%u: %d\n", > + panel->enable_gpio, err); > + return err; > + } > + > + value = (panel->enable_gpio_flags & GPIO_ACTIVE_LOW) != 0; > + > + err = gpio_direction_output(panel->enable_gpio, value); > + if (err < 0) { > + dev_err(&pdev->dev, "failed to setup GPIO%u: %d\n", > + panel->enable_gpio, err); > + goto free_gpio; > + } > + } > + > + backlight = of_parse_phandle(pdev->dev.of_node, "backlight", 0); > + if (backlight) { > + panel->backlight = of_find_backlight_by_node(backlight); > + if (!panel->backlight) { > + err = -EPROBE_DEFER; > + goto free_gpio; > + } > + > + of_node_put(backlight); > + } > + > + drm_panel_init(&panel->base); > + panel->base.dev = &pdev->dev; > + panel->base.funcs = &panel_simple_funcs; > + > + err = drm_panel_add(&panel->base); > + if (err < 0) > + goto free_gpio; > + > + platform_set_drvdata(pdev, panel); > + > + return 0; > + > +free_gpio: > + if (gpio_is_valid(panel->enable_gpio)) > + gpio_free(panel->enable_gpio); > + > + return err; > +} > + > +static int panel_simple_remove(struct platform_device *pdev) > +{ > + struct panel_simple *panel = platform_get_drvdata(pdev); > + > + drm_panel_detach(&panel->base); > + drm_panel_remove(&panel->base); > + > + panel_simple_disable(&panel->base); > + > + if (panel->backlight) > + put_device(&panel->backlight->dev); > + > + if (gpio_is_valid(panel->enable_gpio)) > + gpio_free(panel->enable_gpio); > + > + regulator_disable(panel->supply); > + > + return 0; > +} > + > +static struct platform_driver panel_simple_driver = { > + .driver = { > + .name = "panel-simple", > + .owner = THIS_MODULE, > + .of_match_table = panel_simple_of_match, > + }, > + .probe = panel_simple_probe, > + .remove = panel_simple_remove, > +}; > +module_platform_driver(panel_simple_driver); > + > +MODULE_DESCRIPTION("DRM Driver for Simple Panels"); > +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); > +MODULE_LICENSE("GPL v2");
> > My biggest concern here is that this will not be compatible with the CDF DT > bindings. They're not complete yet, but they will require connections between > entities to be described in DT, in a way very similar (or actually identical) > to the V4L2 DT bindings, documented in > Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a > look at that ? Please ignore all optional properties beside remote-endpoint, > as they're V4L2 specific. > > I also plan to specify video bus parameters in DT for CDF, but this hasn't > been finalized yet. > While I understand this, I don't see why CDF can't enhance these bindings if it has requirements > than they have without disturbing the panel ones, is DT really that inflexible? It seems that have a simple description for basic panels like Thierry wants to support, that can be enhanced for the other cases in the future should suffice, I really don't like blocking stuff that makes things work on the chance of something that isn't upstream yet, its sets a bad precedent, its also breaks the perfect is the enemy of good rule Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 17/10/13 11:28, Dave Airlie wrote: >> >> My biggest concern here is that this will not be compatible with the CDF DT >> bindings. They're not complete yet, but they will require connections between >> entities to be described in DT, in a way very similar (or actually identical) >> to the V4L2 DT bindings, documented in >> Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a >> look at that ? Please ignore all optional properties beside remote-endpoint, >> as they're V4L2 specific. >> >> I also plan to specify video bus parameters in DT for CDF, but this hasn't >> been finalized yet. >> > > While I understand this, I don't see why CDF can't enhance these > bindings if it has > requirements > than they have without disturbing the panel ones, > > is DT really that inflexible? > > It seems that have a simple description for basic panels like Thierry wants > to support, that can be enhanced for the other cases in the future should > suffice, I really don't like blocking stuff that makes things work on the chance > of something that isn't upstream yet, its sets a bad precedent, its also breaks > the perfect is the enemy of good rule Just my opinion, but yes, DT is that inflexible. DT bindings are like a syscall. Once they are there, you need to support them forever. That support can, of course, include some kind of DT data converters or such, that try to convert old bindings to new bindings at kernel boot. In practice even such converter may be enough, if some important piece of data in the new bindings is missing, and this data was implicit in a driver. In that case the conversion, or parts of it, must be done later, in that specific driver. Even that may be difficult, if the piece of data should actually be handled by some other driver. In that case there's probably need for some kind of global look-up table or such, so that the drivers can work around the problem of missing information. I've been working with DT bindings for OMAP display subsystem for something like 1.5 years. Still not merged, as they are not perfect, and I'm scared to push them in non-perfect form, as that could result in some kind of DT-binding-converter-hell described above. Tomi
Adding the dri-devel mailing list on Cc, since this discussion is outgrowing its original intent. On Thu, Oct 17, 2013 at 02:47:52AM +0200, Laurent Pinchart wrote: > Hi Thierry, > > Thank you for the patch. > > On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote: > > Add a driver for simple panels. Such panels can have a regulator that > > provides the supply voltage and a separate GPIO to enable the panel. > > Optionally the panels can have a backlight associated with them so it > > can be enabled or disabled according to the panel's power management > > mode. > > > > Support is added for three panels: An AU Optronics 10.1" WSVGA, a > > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD > > panel. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > .../devicetree/bindings/panel/auo,b101aw03.txt | 7 + > > .../bindings/panel/chunghwa,claa101wb03.txt | 7 + > > .../bindings/panel/panasonic,vvx10f004b00.txt | 7 + > > .../devicetree/bindings/panel/simple-panel.txt | 21 ++ > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/panel/Kconfig | 13 + > > drivers/gpu/drm/panel/Makefile | 1 + > > drivers/gpu/drm/panel/panel-simple.c | 335 ++++++++++++++++++ > > 8 files changed, 392 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/panel/auo,b101aw03.txt > > create mode 100644 > > Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt create > > mode 100644 > > Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt create > > mode 100644 Documentation/devicetree/bindings/panel/simple-panel.txt create > > mode 100644 drivers/gpu/drm/panel/Makefile > > create mode 100644 drivers/gpu/drm/panel/panel-simple.c > > > > diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt > > b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt new file mode > > 100644 > > index 0000000..72e088a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt > > @@ -0,0 +1,7 @@ > > +AU Optronics Corporation 10.1" WSVGA TFT LCD panel > > + > > +Required properties: > > +- compatible: should be "auo,b101aw03" > > + > > +This binding is compatible with the simple-panel binding, which is > > specified +in simple-panel.txt in this directory. > > diff --git > > a/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt > > b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt new file > > mode 100644 > > index 0000000..0ab2c05 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt > > @@ -0,0 +1,7 @@ > > +Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel > > + > > +Required properties: > > +- compatible: should be "chunghwa,claa101wb03" > > + > > +This binding is compatible with the simple-panel binding, which is > > specified +in simple-panel.txt in this directory. > > diff --git > > a/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt > > b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt new > > file mode 100644 > > index 0000000..d328b03 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt > > @@ -0,0 +1,7 @@ > > +Panasonic Corporation 10.1" WUXGA TFT LCD panel > > + > > +Required properties: > > +- compatible: should be "panasonic,vvx10f004b00" > > + > > +This binding is compatible with the simple-panel binding, which is > > specified +in simple-panel.txt in this directory. > > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt > > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode > > 100644 > > index 0000000..1341bbf > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt > > @@ -0,0 +1,21 @@ > > +Simple display panel > > + > > +Required properties: > > +- power-supply: regulator to provide the supply voltage > > + > > +Optional properties: > > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing > > +- enable-gpios: GPIO pin to enable or disable the panel > > +- backlight: phandle of the backlight device attached to the panel > > + > > +Example: > > + > > + panel: panel { > > + compatible = "cptt,claa101wb01"; > > + ddc-i2c-bus = <&panelddc>; > > + > > + power-supply = <&vdd_pnl_reg>; > > + enable-gpios = <&gpio 90 0>; > > + > > + backlight = <&backlight>; > > + }; > > My biggest concern here is that this will not be compatible with the CDF DT > bindings. They're not complete yet, but they will require connections between > entities to be described in DT, in a way very similar (or actually identical) > to the V4L2 DT bindings, documented in > Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a > look at that ? Please ignore all optional properties beside remote-endpoint, > as they're V4L2 specific. Okay, so if I understand correctly, translating those bindings to panel nodes would look somewhat like this: dc: display-controller { ports { port@0 { remote-endpoint = <&panel>; }; }; }; panel: panel { ports { port@0 { remote-endpoint = <&dc>; }; }; }; The above leaves out any of the other, non-relevant properties. Does that sound about right? That seems like an overly complex amount of data to describe just a simple panel where the only connection between it and the display hardware is the data bus and I was sort of expecting the CDF to provide some sort of shortcut for setups where the connection is 1:1 with no means to perform any configuration of the bus. > I also plan to specify video bus parameters in DT for CDF, but this hasn't > been finalized yet. That effectively blocks any of the code that I've been working on until CDF has been merged. It's been discussed for over a year now, and there has even been significant pushback on using CDF in DRM, so even if CDF is eventually merged, that will still leave DRM with no way to turn on a simple panel. I keep wondering why we absolutely must have compatibility between CDF and this simple panel binding. DT content is supposed to concern itself with the description of hardware only. What about the following isn't an accurate description of the panel hardware? panel: panel { compatible = "cptt,claa101wb01"; power-supply = <&vdd_pnl_reg>; enable-gpios = <&gpio 90 0>; backlight = <&backlight>; }; dsi-controller { panel = <&panel>; }; Note how the above is also not incompatible with anything that the CDF (to be) mandates, so it could easily be extended with any nodes or properties that CDF needs to work properly without breaking backwards- compatibility. > > diff --git a/drivers/gpu/drm/panel/panel-simple.c [...] > > +/* > > + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the > > "Software"), > > + * to deal in the Software without restriction, including without > > limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sub > > license, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the > > + * next paragraph) shall be included in all copies or substantial portions > > + * of the Software. > > This contradicts MODULE_LICENSE("GPL v2") below. True, I'll fix that up. > > +/* TODO: convert to gpiod_*() API once it's been merged */ > > +#define GPIO_ACTIVE_LOW (1 << 0) > > Why can't you just #include <dt-bindings/gpio/gpio.h> ? This is supposed to be something completely driver internal. Keeping it here make that more explicit. It shouldn't matter much anyway, since by now these patches have about 0 chance of making it into 3.13, so I'll just convert them to gpiod. > > + backlight_update_status(p->backlight); > > + } > > + > > + if (gpio_is_valid(p->enable_gpio)) { > > + if (p->enable_gpio_flags & GPIO_ACTIVE_LOW) > > + gpio_set_value(p->enable_gpio, 1); > > + else > > + gpio_set_value(p->enable_gpio, 0); > > + } > > + > > + regulator_disable(p->supply); > > + p->enabled = false; > > +} > > + > > +static void panel_simple_enable(struct drm_panel *panel) > > +{ > > + struct panel_simple *p = to_panel_simple(panel); > > + int err; > > + > > + if (p->enabled) > > + return; > > + > > + err = regulator_enable(p->supply); > > + if (err < 0) > > + dev_err(panel->dev, "failed to enable supply: %d\n", err); > > Is that really a non-fatal error ? Shouldn't the enable operation return an > int ? There's no way to propagate this in DRM, so why go through the trouble of returning the error? Furthermore, there's nothing that the caller could do to remedy the situation anyway. > > +static const struct drm_display_mode auo_b101aw03_mode = { > > + .clock = 51450, > > + .hdisplay = 1024, > > + .hsync_start = 1024 + 156, > > + .hsync_end = 1024 + 156 + 8, > > + .htotal = 1024 + 156 + 8 + 156, > > + .vdisplay = 600, > > + .vsync_start = 600 + 16, > > + .vsync_end = 600 + 16 + 6, > > + .vtotal = 600 + 16 + 6 + 16, > > + .vrefresh = 60, > > +}; > > Instead of hardcoding the modes in the driver, which would then require to be > updated for every new simple panel model (and we know there are lots of them), > why don't you specify the modes in the panel DT node ? The simple panel driver > would then become much more generic. It would also allow board designers to > tweak h/v sync timings depending on the system requirements. Sigh... we keep second-guessing ourselves. Back at the time when power sequences were designed (and NAKed at the last minute), we all decided that the right thing to do would be to use specific compatible values for individual panels, because that would allow us to encode the power sequencing within the driver. And when we already have the panel model encoded in the compatible value, we might just as well encode the mode within the driver for that panel. Otherwise we'll have to keep adding the same mode timings for every board that uses the same panel. I do agree though that it might be useful to tweak the mode in case the default one doesn't work. How about we provide a means to override the mode encoded in the driver using one specified in the DT? That's obviously a backwards-compatible change, so it could be added if or when it becomes necessary. Thierry
On Thu, Oct 17, 2013 at 11:49:52AM +0300, Tomi Valkeinen wrote: > Hi, > > On 17/10/13 11:28, Dave Airlie wrote: > >> > >> My biggest concern here is that this will not be compatible with the CDF DT > >> bindings. They're not complete yet, but they will require connections between > >> entities to be described in DT, in a way very similar (or actually identical) > >> to the V4L2 DT bindings, documented in > >> Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a > >> look at that ? Please ignore all optional properties beside remote-endpoint, > >> as they're V4L2 specific. > >> > >> I also plan to specify video bus parameters in DT for CDF, but this hasn't > >> been finalized yet. > >> > > > > While I understand this, I don't see why CDF can't enhance these > > bindings if it has > > requirements > than they have without disturbing the panel ones, > > > > is DT really that inflexible? > > > > It seems that have a simple description for basic panels like Thierry wants > > to support, that can be enhanced for the other cases in the future should > > suffice, I really don't like blocking stuff that makes things work on the chance > > of something that isn't upstream yet, its sets a bad precedent, its also breaks > > the perfect is the enemy of good rule > > Just my opinion, but yes, DT is that inflexible. DT bindings are like a > syscall. Once they are there, you need to support them forever. That > support can, of course, include some kind of DT data converters or such, > that try to convert old bindings to new bindings at kernel boot. That's not entirely true. DT bindings are allowed to change, the only restriction being that they don't break backwards compatibility. So if there's ever a need to support new functionality, be it in the form of new nodes or properties to support something like CDF, that's not a problem as long as the code continues to work with existing bindings. > In practice even such converter may be enough, if some important piece > of data in the new bindings is missing, and this data was implicit in a > driver. In that case the conversion, or parts of it, must be done later, > in that specific driver. > > Even that may be difficult, if the piece of data should actually be > handled by some other driver. In that case there's probably need for > some kind of global look-up table or such, so that the drivers can work > around the problem of missing information. > > I've been working with DT bindings for OMAP display subsystem for > something like 1.5 years. Still not merged, as they are not perfect, and > I'm scared to push them in non-perfect form, as that could result in > some kind of DT-binding-converter-hell described above. Am I the only one refusing to accept that we can't provide even the most basic functionality simply because we haven't been able to come up with the ultimately "perfect" DT binding yet? By definition it's not very likely that any binding will ever be perfect. I don't think we're doing ourselves any good by letting DT actively hinder us in merging any of these features. I would personally prefer having to support several bindings rather than not be able to provide the functionality at all. For crying out loud, we can use the 3D engine to render to a framebuffer but we can't look at the result because we can't make the bloody panel light up! Seriously!? Thierry
On 17/10/13 12:16, Thierry Reding wrote: > On Thu, Oct 17, 2013 at 11:49:52AM +0300, Tomi Valkeinen wrote: >> Just my opinion, but yes, DT is that inflexible. DT bindings are like a >> syscall. Once they are there, you need to support them forever. That >> support can, of course, include some kind of DT data converters or such, >> that try to convert old bindings to new bindings at kernel boot. > > That's not entirely true. DT bindings are allowed to change, the only > restriction being that they don't break backwards compatibility. So if True, but afaik the same goes for syscalls. But yes, it's probably easier to add, say, new properties to DT bindings than adding new flags to a syscall. > Am I the only one refusing to accept that we can't provide even the most > basic functionality simply because we haven't been able to come up with > the ultimately "perfect" DT binding yet? By definition it's not very > likely that any binding will ever be perfect. With "perfect" I meant without any obvious features missing. I agree that DT bindings will never be perfect, but I at least would like them to be good enough to cover all the cases we know of. > I don't think we're doing ourselves any good by letting DT actively > hinder us in merging any of these features. I would personally prefer > having to support several bindings rather than not be able to provide > the functionality at all. I'd say the driver writer is of course free to create basic bindings for the device in question as long as the bindings are sane, and he agrees to later create any needed converters. I don't see any problem with having a driver supporting several versions of the bindings. But I don't think it's fair to push a driver with basic bindings, knowing that the bindings won't be enough in the future, and leave all the converter-mess to other people (not saying that's the case here). As a fbdev maintainer, I'm ok with adding DT bindings to device specific fb drivers, as those can be just left as they are. Even when CDF is merged, the specific fb drivers just work with the old bindings. If the driver maintainer wants to use CDF, it's up to him to create any needed binding-converters. But a generic driver for panels is more problematic, as that one should be maintained and refreshed to new features like CDF. If such was proposed to be merged to fbdev, I'd be very reluctant to merge it if there were any concerns about it. I guess one option there would be to implement a new driver for the same panel devices, one that supports CDF, and leave the old one as is. Not so nice option, though. > For crying out loud, we can use the 3D engine to render to a framebuffer > but we can't look at the result because we can't make the bloody panel > light up! Seriously!? Ah, but do you have DT bindings for the 3D engine yet, which represent how different internal blocks in the 3D engine are connected? If not, better start designing it! Then the problem is solved, you won't even have working 3D engine. ;) But seriously speaking, I'm with you. I don't get this DT stuff. And the funny thing is, DT should just represent the hardware, it doesn't matter what kind of SW drivers there are, so in a sense talking about DT problems with a SW framework like CDF is a bit silly. How hard can it be to describe the hardware properly? Very, obviously =). Tomi
On 17/10/13 11:53, Thierry Reding wrote: > I keep wondering why we absolutely must have compatibility between CDF > and this simple panel binding. DT content is supposed to concern itself > with the description of hardware only. What about the following isn't an > accurate description of the panel hardware? > > panel: panel { > compatible = "cptt,claa101wb01"; > > power-supply = <&vdd_pnl_reg>; > enable-gpios = <&gpio 90 0>; > > backlight = <&backlight>; > }; > > dsi-controller { > panel = <&panel>; > }; That's quite similar to how my current out-of-mainline OMAP DSS DT bindings work. The difference is that I have a reference from the panel node to the video source (dsi-controller), instead of the other way around. I just find it more natural. It works the same way as, say, regulators, gpios, etc. However, one thing unclear is where the panel node should be. You seem to have a DSI panel here. If the panel is truly not controlled in any way, maybe having the panel as a normal platform device is ok. But if the DSI panel is controlled with DSI messages, should it be a child of the dsi-controller node, the same way i2c peripherals are children of i2c master? >>> +static const struct drm_display_mode auo_b101aw03_mode = { >>> + .clock = 51450, >>> + .hdisplay = 1024, >>> + .hsync_start = 1024 + 156, >>> + .hsync_end = 1024 + 156 + 8, >>> + .htotal = 1024 + 156 + 8 + 156, >>> + .vdisplay = 600, >>> + .vsync_start = 600 + 16, >>> + .vsync_end = 600 + 16 + 6, >>> + .vtotal = 600 + 16 + 6 + 16, >>> + .vrefresh = 60, >>> +}; >> >> Instead of hardcoding the modes in the driver, which would then require to be >> updated for every new simple panel model (and we know there are lots of them), >> why don't you specify the modes in the panel DT node ? The simple panel driver >> would then become much more generic. It would also allow board designers to >> tweak h/v sync timings depending on the system requirements. > > Sigh... we keep second-guessing ourselves. Back at the time when power > sequences were designed (and NAKed at the last minute), we all decided > that the right thing to do would be to use specific compatible values > for individual panels, because that would allow us to encode the power > sequencing within the driver. And when we already have the panel model > encoded in the compatible value, we might just as well encode the mode > within the driver for that panel. Otherwise we'll have to keep adding > the same mode timings for every board that uses the same panel. Oh, I didn't feel "we all decided that the right thing..." =). > I do agree though that it might be useful to tweak the mode in case the > default one doesn't work. How about we provide a means to override the > mode encoded in the driver using one specified in the DT? That's > obviously a backwards-compatible change, so it could be added if or when > it becomes necessary. This sounds good to me. Although maybe it'd be good to have the driver compatible with something like "generic-panel", so that you could have: compatible = "foo,specific-panel", "generic-panel"; and if there's no need for any power/gpio setup for your board, you may skip adding "foo,specific-panel" support to the panel driver. Later, somebody else may need to implement fine grained power/gpio support for "foo,specific-panel", and it would just work. Maybe that would help us with the problem of adding support in the driver for a hundred different simple panels each used only once on a specific board. Tomi
On Thu, Oct 17, 2013 at 12:52:40PM +0300, Tomi Valkeinen wrote: > On 17/10/13 12:16, Thierry Reding wrote: > > On Thu, Oct 17, 2013 at 11:49:52AM +0300, Tomi Valkeinen wrote: > > >> Just my opinion, but yes, DT is that inflexible. DT bindings are like a > >> syscall. Once they are there, you need to support them forever. That > >> support can, of course, include some kind of DT data converters or such, > >> that try to convert old bindings to new bindings at kernel boot. > > > > That's not entirely true. DT bindings are allowed to change, the only > > restriction being that they don't break backwards compatibility. So if > > True, but afaik the same goes for syscalls. But yes, it's probably > easier to add, say, new properties to DT bindings than adding new flags > to a syscall. > > > Am I the only one refusing to accept that we can't provide even the most > > basic functionality simply because we haven't been able to come up with > > the ultimately "perfect" DT binding yet? By definition it's not very > > likely that any binding will ever be perfect. > > With "perfect" I meant without any obvious features missing. I agree > that DT bindings will never be perfect, but I at least would like them > to be good enough to cover all the cases we know of. As far as the simple panel device tree binding is concerned, it covers all the cases I know of. I can't seriously be expected to do any better than that. These panels are dumb. They only come with a power supply that needs to be switched on and an additional GPIO to enable it once powered up. All three panels just work with that set of properties. So anything that doesn't can arguably be covered by some other, not-so- simple binding. > > I don't think we're doing ourselves any good by letting DT actively > > hinder us in merging any of these features. I would personally prefer > > having to support several bindings rather than not be able to provide > > the functionality at all. > > I'd say the driver writer is of course free to create basic bindings for > the device in question as long as the bindings are sane, and he agrees > to later create any needed converters. I don't see any problem with > having a driver supporting several versions of the bindings. > > But I don't think it's fair to push a driver with basic bindings, > knowing that the bindings won't be enough in the future, and leave all > the converter-mess to other people (not saying that's the case here). > > As a fbdev maintainer, I'm ok with adding DT bindings to device specific > fb drivers, as those can be just left as they are. Even when CDF is > merged, the specific fb drivers just work with the old bindings. If the > driver maintainer wants to use CDF, it's up to him to create any needed > binding-converters. > > But a generic driver for panels is more problematic, as that one should > be maintained and refreshed to new features like CDF. If such was > proposed to be merged to fbdev, I'd be very reluctant to merge it if > there were any concerns about it. I'm still missing the point here. For one, I don't see any reason that this driver would ever need to gain CDF support. At the same time, CDF could easily be supported by just adding a few required properties and it should be easy to support any non-CDF cases just as well to remain backwards-compatible. > I guess one option there would be to implement a new driver for the same > panel devices, one that supports CDF, and leave the old one as is. Not > so nice option, though. As I said, anything that really needs a CDF binding to work likely isn't "simple" anymore, therefore a separate driver can easily be justified. > > For crying out loud, we can use the 3D engine to render to a framebuffer > > but we can't look at the result because we can't make the bloody panel > > light up! Seriously!? > > Ah, but do you have DT bindings for the 3D engine yet, which represent > how different internal blocks in the 3D engine are connected? If not, > better start designing it! Then the problem is solved, you won't even > have working 3D engine. ;) > > But seriously speaking, I'm with you. I don't get this DT stuff. > > And the funny thing is, DT should just represent the hardware, it > doesn't matter what kind of SW drivers there are, so in a sense talking > about DT problems with a SW framework like CDF is a bit silly. How hard > can it be to describe the hardware properly? > > Very, obviously =). Indeed. The fundamental point here seems to be that we don't represent things in DT which we don't need. In the same way that we don't add device nodes for enumerable devices, why would we need to explicitly add information about connections between devices that cannot be controlled anyway. If the board connects an RGB/LVDS output to a panel directly, it is only required that the output knows what it is connected to because there is no other way besides DT to obtain any information about the panel. Therefore a unidirectional connection is more than enough. Using that the output can access the panel and query it for information such as the mode timings as well as switch it on or off as required. Thierry
Hi Thierry, On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: > Adding the dri-devel mailing list on Cc, since this discussion is > outgrowing its original intent. > > On Thu, Oct 17, 2013 at 02:47:52AM +0200, Laurent Pinchart wrote: > > Hi Thierry, > > > > Thank you for the patch. > > > > On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote: > > > Add a driver for simple panels. Such panels can have a regulator that > > > provides the supply voltage and a separate GPIO to enable the panel. > > > Optionally the panels can have a backlight associated with them so it > > > can be enabled or disabled according to the panel's power management > > > mode. > > > > > > Support is added for three panels: An AU Optronics 10.1" WSVGA, a > > > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD > > > panel. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > > > > .../devicetree/bindings/panel/auo,b101aw03.txt | 7 + > > > .../bindings/panel/chunghwa,claa101wb03.txt | 7 + > > > .../bindings/panel/panasonic,vvx10f004b00.txt | 7 + > > > .../devicetree/bindings/panel/simple-panel.txt | 21 ++ > > > drivers/gpu/drm/Makefile | 1 + > > > drivers/gpu/drm/panel/Kconfig | 13 + > > > drivers/gpu/drm/panel/Makefile | 1 + > > > drivers/gpu/drm/panel/panel-simple.c | 335 ++++++++++++++ > > > 8 files changed, 392 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/panel/auo,b101aw03.txt > > > create mode 100644 > > > Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt > > > create mode 100644 > > > Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt > > > create mode 100644 > > > Documentation/devicetree/bindings/panel/simple-panel.txt > > > create mode 100644 drivers/gpu/drm/panel/Makefile > > > create mode 100644 drivers/gpu/drm/panel/panel-simple.c [snip] > > > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt > > > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode > > > 100644 > > > index 0000000..1341bbf > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt > > > @@ -0,0 +1,21 @@ > > > +Simple display panel > > > + > > > +Required properties: > > > +- power-supply: regulator to provide the supply voltage > > > + > > > +Optional properties: > > > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing > > > +- enable-gpios: GPIO pin to enable or disable the panel > > > +- backlight: phandle of the backlight device attached to the panel > > > + > > > +Example: > > > + > > > + panel: panel { > > > + compatible = "cptt,claa101wb01"; > > > + ddc-i2c-bus = <&panelddc>; > > > + > > > + power-supply = <&vdd_pnl_reg>; > > > + enable-gpios = <&gpio 90 0>; > > > + > > > + backlight = <&backlight>; > > > + }; > > > > My biggest concern here is that this will not be compatible with the CDF > > DT bindings. They're not complete yet, but they will require connections > > between entities to be described in DT, in a way very similar (or > > actually identical) to the V4L2 DT bindings, documented in > > Documentation/devicetree/bindings/media/video-interfaces.txt. Could you > > have a look at that ? Please ignore all optional properties beside > > remote-endpoint, as they're V4L2 specific. > > Okay, so if I understand correctly, translating those bindings to panel > nodes would look somewhat like this: > > dc: display-controller { > ports { > port@0 { > remote-endpoint = <&panel>; > }; > }; > }; > > panel: panel { > ports { > port@0 { > remote-endpoint = <&dc>; > }; > }; > }; > > The above leaves out any of the other, non-relevant properties. Does > that sound about right? Yes it does. > That seems like an overly complex amount of data to describe just a simple > panel where the only connection between it and the display hardware is the > data bus and I was sort of expecting the CDF to provide some sort of > shortcut for setups where the connection is 1:1 with no means to perform any > configuration of the bus. CDF needs to model video pipelines that can be more complex than just a display controller with a single output connected to a panel with a single input. For that reason the DT bindings need to describe connections in a generic enough way. The above DT fragment might be slightly more complex than one would expect when thinking about the really simple case only, but it's nowhere near overly complex in my opinion. Please note that, when a device has as single port, the ports node can be omitted, and the port doesn't need to be numbered. You would then end up with dc: display-controller { port { remote-endpoint = <&panel>; }; }; panel: panel { port { remote-endpoint = <&dc>; }; }; I don't think there's a way to simplify it further. > > I also plan to specify video bus parameters in DT for CDF, but this hasn't > > been finalized yet. > > That effectively blocks any of the code that I've been working on until > CDF has been merged. It's been discussed for over a year now, and there > has even been significant pushback on using CDF in DRM, so even if CDF > is eventually merged, that will still leave DRM with no way to turn on > a simple panel. I'm probably even more concerned about the long time all this took and will still take than you are, sincerely. I'm obviously to blame for not being able to explain the problems and solutions correctly, as I see no way why people would push back if they realized that the other solutions that have been proposed, far from being future-proof, won't even be present-proof. We would be shooting ourselves in the foot, but what can I do when too many people want to do so ? I'll still keep trying of course, but not for years.I have yet to see someone proposing a viable solution to share drivers between DRM and V4L2, which is a real pressing requirement, partly due to DT. Regarding the use of CDF in DRM, please note that it will be optional for drivers to switch to the new framework. Nothing should hopefully require existing drivers to be converted, drivers that want to make use of CDF to support panels will be welcome to do so, but they big DRM drivers for desktop GPUs won't be affected. > I keep wondering why we absolutely must have compatibility between CDF > and this simple panel binding. DT content is supposed to concern itself > with the description of hardware only. What about the following isn't an > accurate description of the panel hardware? > > panel: panel { > compatible = "cptt,claa101wb01"; > > power-supply = <&vdd_pnl_reg>; > enable-gpios = <&gpio 90 0>; > > backlight = <&backlight>; > }; > > dsi-controller { > panel = <&panel>; > }; > > Note how the above is also not incompatible with anything that the CDF > (to be) mandates, so it could easily be extended with any nodes or > properties that CDF needs to work properly without breaking backwards- > compatibility. As mentioned above, CDF needs to describe more complex pipelines. To make the driver life as simple as possible the core code will handle the pipeline description in the DT bindings, and it does require it to be generic enough. You can of course describe connections in various ways depending on the hardware, which would make the DT-related code needlessly overcomplex. One of the issue with the above description is that there's no way to go from the panel to the display controller, which might be a show stopper in the future when code requires that. A workaround would be to look through the whole DT for the reverse link, which would be pretty inefficient. > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c [...] > > > +static void panel_simple_enable(struct drm_panel *panel) > > > +{ > > > + struct panel_simple *p = to_panel_simple(panel); > > > + int err; > > > + > > > + if (p->enabled) > > > + return; > > > + > > > + err = regulator_enable(p->supply); > > > + if (err < 0) > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err); > > > > Is that really a non-fatal error ? Shouldn't the enable operation return > > an int ? > > There's no way to propagate this in DRM, so why go through the trouble > of returning the error? Furthermore, there's nothing that the caller > could do to remedy the situation anyway. That's a DRM issue, which could be fixed. While the caller can't remedy the situation, it should at least report the error to the application instead of silently ignoring it. > > > +static const struct drm_display_mode auo_b101aw03_mode = { > > > + .clock = 51450, > > > + .hdisplay = 1024, > > > + .hsync_start = 1024 + 156, > > > + .hsync_end = 1024 + 156 + 8, > > > + .htotal = 1024 + 156 + 8 + 156, > > > + .vdisplay = 600, > > > + .vsync_start = 600 + 16, > > > + .vsync_end = 600 + 16 + 6, > > > + .vtotal = 600 + 16 + 6 + 16, > > > + .vrefresh = 60, > > > +}; > > > > Instead of hardcoding the modes in the driver, which would then require to > > be updated for every new simple panel model (and we know there are lots > > of them), why don't you specify the modes in the panel DT node ? The > > simple panel driver would then become much more generic. It would also > > allow board designers to tweak h/v sync timings depending on the system > > requirements. > > Sigh... we keep second-guessing ourselves. Back at the time when power > sequences were designed (and NAKed at the last minute), we all decided > that the right thing to do would be to use specific compatible values > for individual panels, because that would allow us to encode the power > sequencing within the driver. And when we already have the panel model > encoded in the compatible value, we might just as well encode the mode > within the driver for that panel. Otherwise we'll have to keep adding > the same mode timings for every board that uses the same panel. > > I do agree though that it might be useful to tweak the mode in case the > default one doesn't work. How about we provide a means to override the > mode encoded in the driver using one specified in the DT? That's > obviously a backwards-compatible change, so it could be added if or when > it becomes necessary. I share Tomi's point of view here. Your three panels use the same power sequence, so I believe we should have a generic panel compatible string that would use modes described in DT for the common case. Only if a panel required something more complex which can't (or which could, but won't for any reason) accurately be described in DT would you need to extend the driver.
On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote: > On 17/10/13 11:53, Thierry Reding wrote: > > > I keep wondering why we absolutely must have compatibility between CDF > > and this simple panel binding. DT content is supposed to concern itself > > with the description of hardware only. What about the following isn't an > > accurate description of the panel hardware? > > > > panel: panel { > > compatible = "cptt,claa101wb01"; > > > > power-supply = <&vdd_pnl_reg>; > > enable-gpios = <&gpio 90 0>; > > > > backlight = <&backlight>; > > }; > > > > dsi-controller { > > panel = <&panel>; > > }; > > That's quite similar to how my current out-of-mainline OMAP DSS DT > bindings work. The difference is that I have a reference from the panel > node to the video source (dsi-controller), instead of the other way > around. I just find it more natural. It works the same way as, say, > regulators, gpios, etc. I suppose that depends on the way you look at it. In the above proposal I consider the output (DSI controller) to use the panel as a resource that provides a certain set of services (query mode timings, provide a way to enable or disable the panel). Similarly the panel uses the backlight as a resource that provides a certain set of services (such as changing the brightness). The above also follows the natural order of control. The panel has no way to control the DSI output. However, it is the output that controls when a panel is required and turn it on as appropriate. > However, one thing unclear is where the panel node should be. You seem > to have a DSI panel here. If the panel is truly not controlled in any > way, maybe having the panel as a normal platform device is ok. But if > the DSI panel is controlled with DSI messages, should it be a child of > the dsi-controller node, the same way i2c peripherals are children of > i2c master? That's one possibility. In this particular case it's not even necessary to use any of DSIs control methods to talk to the panel. The panel only receives the data stream and "just works". Even if the panel were to be controlled via DSI, I think keeping it as a separate device node is equally valid. It's really boils down to what I already said above. The output uses the panel as a resource, similar to how other devices might use a GPIO controller to use a GPIO pin, or an interrupt controller to request an IRQ. > >>> +static const struct drm_display_mode auo_b101aw03_mode = { > >>> + .clock = 51450, > >>> + .hdisplay = 1024, > >>> + .hsync_start = 1024 + 156, > >>> + .hsync_end = 1024 + 156 + 8, > >>> + .htotal = 1024 + 156 + 8 + 156, > >>> + .vdisplay = 600, > >>> + .vsync_start = 600 + 16, > >>> + .vsync_end = 600 + 16 + 6, > >>> + .vtotal = 600 + 16 + 6 + 16, > >>> + .vrefresh = 60, > >>> +}; > >> > >> Instead of hardcoding the modes in the driver, which would then require to be > >> updated for every new simple panel model (and we know there are lots of them), > >> why don't you specify the modes in the panel DT node ? The simple panel driver > >> would then become much more generic. It would also allow board designers to > >> tweak h/v sync timings depending on the system requirements. > > > > Sigh... we keep second-guessing ourselves. Back at the time when power > > sequences were designed (and NAKed at the last minute), we all decided > > that the right thing to do would be to use specific compatible values > > for individual panels, because that would allow us to encode the power > > sequencing within the driver. And when we already have the panel model > > encoded in the compatible value, we might just as well encode the mode > > within the driver for that panel. Otherwise we'll have to keep adding > > the same mode timings for every board that uses the same panel. > > Oh, I didn't feel "we all decided that the right thing..." =). > > > I do agree though that it might be useful to tweak the mode in case the > > default one doesn't work. How about we provide a means to override the > > mode encoded in the driver using one specified in the DT? That's > > obviously a backwards-compatible change, so it could be added if or when > > it becomes necessary. > > This sounds good to me. > > Although maybe it'd be good to have the driver compatible with something > like "generic-panel", so that you could have: > > compatible = "foo,specific-panel", "generic-panel"; > > and if there's no need for any power/gpio setup for your board, you may > skip adding "foo,specific-panel" support to the panel driver. Later, > somebody else may need to implement fine grained power/gpio support for > "foo,specific-panel", and it would just work. > > Maybe that would help us with the problem of adding support in the > driver for a hundred different simple panels each used only once on a > specific board. Sure, that all sounds like reasonable additions. All of the suggestions are even backwards-compatible and hence can be added when needed without breaking compatibility with existing users of the binding. Thierry
Hi Thierry, On Thursday 17 October 2013 12:48:57 Thierry Reding wrote: > On Thu, Oct 17, 2013 at 12:52:40PM +0300, Tomi Valkeinen wrote: > > On 17/10/13 12:16, Thierry Reding wrote: > > > On Thu, Oct 17, 2013 at 11:49:52AM +0300, Tomi Valkeinen wrote: > > >> Just my opinion, but yes, DT is that inflexible. DT bindings are like a > > >> syscall. Once they are there, you need to support them forever. That > > >> support can, of course, include some kind of DT data converters or > > >> such, that try to convert old bindings to new bindings at kernel boot. > > > > > > That's not entirely true. DT bindings are allowed to change, the only > > > restriction being that they don't break backwards compatibility. So if > > > > True, but afaik the same goes for syscalls. But yes, it's probably > > easier to add, say, new properties to DT bindings than adding new flags > > to a syscall. > > > > > Am I the only one refusing to accept that we can't provide even the most > > > basic functionality simply because we haven't been able to come up with > > > the ultimately "perfect" DT binding yet? By definition it's not very > > > likely that any binding will ever be perfect. > > > > With "perfect" I meant without any obvious features missing. I agree > > that DT bindings will never be perfect, but I at least would like them > > to be good enough to cover all the cases we know of. > > As far as the simple panel device tree binding is concerned, it covers > all the cases I know of. I can't seriously be expected to do any better > than that. These panels are dumb. They only come with a power supply > that needs to be switched on and an additional GPIO to enable it once > powered up. All three panels just work with that set of properties. So > anything that doesn't can arguably be covered by some other, not-so- > simple binding. > > > > I don't think we're doing ourselves any good by letting DT actively > > > hinder us in merging any of these features. I would personally prefer > > > having to support several bindings rather than not be able to provide > > > the functionality at all. > > > > I'd say the driver writer is of course free to create basic bindings for > > the device in question as long as the bindings are sane, and he agrees > > to later create any needed converters. I don't see any problem with > > having a driver supporting several versions of the bindings. > > > > But I don't think it's fair to push a driver with basic bindings, > > knowing that the bindings won't be enough in the future, and leave all > > the converter-mess to other people (not saying that's the case here). > > > > As a fbdev maintainer, I'm ok with adding DT bindings to device specific > > fb drivers, as those can be just left as they are. Even when CDF is > > merged, the specific fb drivers just work with the old bindings. If the > > driver maintainer wants to use CDF, it's up to him to create any needed > > binding-converters. > > > > But a generic driver for panels is more problematic, as that one should > > be maintained and refreshed to new features like CDF. If such was > > proposed to be merged to fbdev, I'd be very reluctant to merge it if > > there were any concerns about it. > > I'm still missing the point here. For one, I don't see any reason that > this driver would ever need to gain CDF support. At the same time, CDF > could easily be supported by just adding a few required properties and > it should be easy to support any non-CDF cases just as well to remain > backwards-compatible. > > > I guess one option there would be to implement a new driver for the same > > panel devices, one that supports CDF, and leave the old one as is. Not > > so nice option, though. > > As I said, anything that really needs a CDF binding to work likely isn't > "simple" anymore, therefore a separate driver can easily be justified. The system as a whole would be more complex, but the panel could be the same. We can't have two drivers for the same piece of hardware in the DT world, as there will be a single compatible string and no way to choose between the drivers (unlike the board code world that could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" and live happily with that ever after). > > > For crying out loud, we can use the 3D engine to render to a framebuffer > > > but we can't look at the result because we can't make the bloody panel > > > light up! Seriously!? > > > > Ah, but do you have DT bindings for the 3D engine yet, which represent > > how different internal blocks in the 3D engine are connected? If not, > > better start designing it! Then the problem is solved, you won't even > > have working 3D engine. ;) > > > > But seriously speaking, I'm with you. I don't get this DT stuff. > > > > And the funny thing is, DT should just represent the hardware, it > > doesn't matter what kind of SW drivers there are, so in a sense talking > > about DT problems with a SW framework like CDF is a bit silly. How hard > > can it be to describe the hardware properly? > > > > Very, obviously =). > > Indeed. The fundamental point here seems to be that we don't represent > things in DT which we don't need. In the same way that we don't add device > nodes for enumerable devices, why would we need to explicitly add > information about connections between devices that cannot be controlled > anyway. If the board connects an RGB/LVDS output to a panel directly, it is > only required that the output knows what it is connected to because there is > no other way besides DT to obtain any information about the panel. Therefore > a unidirectional connection is more than enough. Using that the output can > access the panel and query it for information such as the mode timings as > well as switch it on or off as required. Please note that the idea of putting the modes in DT in no way implies that the device controller should parse the modes from the panel node. It must instead query the panel at runtime through whatever API is provided. Going directly to the DT data would be a layering violation. The reason why I propose to add modes to the panel DT node is to make the geenric panel driver more generic and simpler.
Hi Thierry, On Thursday 17 October 2013 13:05:18 Thierry Reding wrote: > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote: > > On 17/10/13 11:53, Thierry Reding wrote: > > > I keep wondering why we absolutely must have compatibility between CDF > > > and this simple panel binding. DT content is supposed to concern itself > > > with the description of hardware only. What about the following isn't an > > > accurate description of the panel hardware? > > > > > > panel: panel { > > > compatible = "cptt,claa101wb01"; > > > > > > power-supply = <&vdd_pnl_reg>; > > > enable-gpios = <&gpio 90 0>; > > > > > > backlight = <&backlight>; > > > }; > > > > > > dsi-controller { > > > panel = <&panel>; > > > }; > > > > That's quite similar to how my current out-of-mainline OMAP DSS DT > > bindings work. The difference is that I have a reference from the panel > > node to the video source (dsi-controller), instead of the other way > > around. I just find it more natural. It works the same way as, say, > > regulators, gpios, etc. > > I suppose that depends on the way you look at it. In the above proposal > I consider the output (DSI controller) to use the panel as a resource > that provides a certain set of services (query mode timings, provide a > way to enable or disable the panel). Similarly the panel uses the > backlight as a resource that provides a certain set of services (such as > changing the brightness). > > The above also follows the natural order of control. The panel has no > way to control the DSI output. However, it is the output that controls > when a panel is required and turn it on as appropriate. I'm no DSI expert, but I know enough about it to be sure that Tomi will disagree. DSI panels can have complex power sequences that require the input stream to be finely controlled (for instance it might require a clock without video frames for some time, switch a GPIO or regulator, send a command to the panel, and then only get video frames). For that reason all developers I've talked to who have an in-depth knowledge of DSI and DSI panels told me that the panel needs to control the video bus, and request the video source to start/stop the video stream. > > However, one thing unclear is where the panel node should be. You seem > > to have a DSI panel here. If the panel is truly not controlled in any > > way, maybe having the panel as a normal platform device is ok. But if > > the DSI panel is controlled with DSI messages, should it be a child of > > the dsi-controller node, the same way i2c peripherals are children of > > i2c master? > > That's one possibility. In this particular case it's not even necessary > to use any of DSIs control methods to talk to the panel. The panel only > receives the data stream and "just works". > > Even if the panel were to be controlled via DSI, I think keeping it as > a separate device node is equally valid. It's really boils down to what > I already said above. The output uses the panel as a resource, similar > to how other devices might use a GPIO controller to use a GPIO pin, or > an interrupt controller to request an IRQ. Except that the display controller can also provides resources to the panel. For DSI panels that support DSI commands, the control bus is provided by the display controller. Much like an I2C device must be a child of its I2C controller node, the DSI panel device should then be a child of the display controller node. > > >>> +static const struct drm_display_mode auo_b101aw03_mode = { > > >>> + .clock = 51450, > > >>> + .hdisplay = 1024, > > >>> + .hsync_start = 1024 + 156, > > >>> + .hsync_end = 1024 + 156 + 8, > > >>> + .htotal = 1024 + 156 + 8 + 156, > > >>> + .vdisplay = 600, > > >>> + .vsync_start = 600 + 16, > > >>> + .vsync_end = 600 + 16 + 6, > > >>> + .vtotal = 600 + 16 + 6 + 16, > > >>> + .vrefresh = 60, > > >>> +}; > > >> > > >> Instead of hardcoding the modes in the driver, which would then require > > >> to be updated for every new simple panel model (and we know there are > > >> lots of them), why don't you specify the modes in the panel DT node ? > > >> The simple panel driver would then become much more generic. It would > > >> also allow board designers to tweak h/v sync timings depending on the > > >> system requirements. > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when power > > > sequences were designed (and NAKed at the last minute), we all decided > > > that the right thing to do would be to use specific compatible values > > > for individual panels, because that would allow us to encode the power > > > sequencing within the driver. And when we already have the panel model > > > encoded in the compatible value, we might just as well encode the mode > > > within the driver for that panel. Otherwise we'll have to keep adding > > > the same mode timings for every board that uses the same panel. > > > > Oh, I didn't feel "we all decided that the right thing..." =). > > > > > I do agree though that it might be useful to tweak the mode in case the > > > default one doesn't work. How about we provide a means to override the > > > mode encoded in the driver using one specified in the DT? That's > > > obviously a backwards-compatible change, so it could be added if or when > > > it becomes necessary. > > > > This sounds good to me. > > > > Although maybe it'd be good to have the driver compatible with something > > like "generic-panel", so that you could have: > > > > compatible = "foo,specific-panel", "generic-panel"; > > > > and if there's no need for any power/gpio setup for your board, you may > > skip adding "foo,specific-panel" support to the panel driver. Later, > > somebody else may need to implement fine grained power/gpio support for > > "foo,specific-panel", and it would just work. > > > > Maybe that would help us with the problem of adding support in the > > driver for a hundred different simple panels each used only once on a > > specific board. > > Sure, that all sounds like reasonable additions. All of the suggestions > are even backwards-compatible and hence can be added when needed without > breaking compatibility with existing users of the binding. What's wrong with moving the three hardcoded modes we already have in the driver to DT before pushing this to mainline ?
On 17/10/13 13:48, Thierry Reding wrote: > As far as the simple panel device tree binding is concerned, it covers > all the cases I know of. I can't seriously be expected to do any better > than that. These panels are dumb. They only come with a power supply > that needs to be switched on and an additional GPIO to enable it once > powered up. All three panels just work with that set of properties. So > anything that doesn't can arguably be covered by some other, not-so- > simple binding. Agreed. I thought the DT bindings were missing information about the video path, but I didn't realize you have a reference from the video output to the panel. However, I think there's usually a bit more information needed than what you describe above. All the dump panels I know have requirements on the sequences related to the video signal, powers and gpios, and delays between those. I guess the most common enable scenario is something like: - enable video signal - enable power - wait n msecs - set GPIO For some panels, the enable GPIO might actually be a reset GPIO, not enable. Some could want the video signal only be enabled after the enable/reset is done. Anyway, I think what you describe fits most of the panels, so no need to worry about the possible ones I described. I just wanted to point out that there may be very dump panels that are still different. > I'm still missing the point here. For one, I don't see any reason that > this driver would ever need to gain CDF support. At the same time, CDF > could easily be supported by just adding a few required properties and > it should be easy to support any non-CDF cases just as well to remain > backwards-compatible. Well, I guess the point here is that if you have a SoC display controller driver and panel drivers, and you want to support complex panels using CDF, you need to implement CDF support into your SoC display controller driver in addition to adding CDF support to the panel driver. And, you still want to be able to use the simple panels. So either you support both CDF panels and non-CDF panels with your SoC dispc, which could be a bit messy, or you add CDF support to all panel drivers you use. >> I guess one option there would be to implement a new driver for the same >> panel devices, one that supports CDF, and leave the old one as is. Not >> so nice option, though. > > As I said, anything that really needs a CDF binding to work likely isn't > "simple" anymore, therefore a separate driver can easily be justified. I think it's rarely the case that a panel specifically needs CDF. It's more that CDF offers a common framework for display components, making it easy to make them independent of the platform used. And if you make your SoC support CDF, you are able to use all the CDF drivers already implemented. That said, I don't see anything technically preventing from having support for both CDF and non-CDF panels. It's just more complex, and they cannot be mixed freely. For example, if I have CDF support in my SoC driver and a panel driver, I can easily add a CDF encoder driver in between. But if the panel driver is a custom one, then I need a custom encoder driver there also. > Indeed. The fundamental point here seems to be that we don't represent > things in DT which we don't need. In the same way that we don't add Surely we should represent the things about the HW we don't need now, if we know they might be needed in the future? (But that's not the case here, see below) > device nodes for enumerable devices, why would we need to explicitly add > information about connections between devices that cannot be controlled > anyway. If the board connects an RGB/LVDS output to a panel directly, it > is only required that the output knows what it is connected to because > there is no other way besides DT to obtain any information about the > panel. Therefore a unidirectional connection is more than enough. Using > that the output can access the panel and query it for information such > as the mode timings as well as switch it on or off as required. Right, in that case, you do have all the necessary information in the DT. I don't think it really matters if the link in DT is from the SoC to the panel, vice versa, or both ways. It still describes the same thing. Even with unidirectional link you can always find the reverse link, you just need to do more work going throught the DT data. Tomi
On 17/10/13 14:02, Laurent Pinchart wrote: >> Okay, so if I understand correctly, translating those bindings to panel >> nodes would look somewhat like this: >> >> dc: display-controller { >> ports { >> port@0 { >> remote-endpoint = <&panel>; >> }; >> }; >> }; >> >> panel: panel { >> ports { >> port@0 { >> remote-endpoint = <&dc>; >> }; >> }; >> }; >> >> The above leaves out any of the other, non-relevant properties. Does >> that sound about right? > > Yes it does. It does? Shouldn't it be something like: panel { ports { port@0 { endpoint@0 { remote = <&dc>; }; }; }; }; And simplified: panel { port { endpoint@0 { remote = <&dc>; }; }; }; You do need a node for the endpoint, a remote-endpoint property is not enough. > Please note that, when a device has as single port, the ports node can be > omitted, and the port doesn't need to be numbered. You would then end up with > > dc: display-controller { > port { > remote-endpoint = <&panel>; > }; > }; > > panel: panel { > port { > remote-endpoint = <&dc>; > }; > }; > > I don't think there's a way to simplify it further. I'm not sure if there's a specific need for the port or endpoint nodes in cases like the above. Even if we have common properties describing the endpoint, I guess they could just be in the parent node. panel { remote = <&dc>; common-video-property = <asd>; }; The above would imply one port and one endpoint. Would that work? If we had a function like parse_endpoint(node), we could just point it to either a real endpoint node, or to the device's node. Tomi
On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote: > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: [...] > CDF needs to model video pipelines that can be more complex than just a > display controller with a single output connected to a panel with a single > input. For that reason the DT bindings need to describe connections in a > generic enough way. The above DT fragment might be slightly more complex than > one would expect when thinking about the really simple case only, but it's > nowhere near overly complex in my opinion. > > Please note that, when a device has as single port, the ports node can be > omitted, and the port doesn't need to be numbered. You would then end up with > > dc: display-controller { > port { > remote-endpoint = <&panel>; > }; > }; > > panel: panel { > port { > remote-endpoint = <&dc>; > }; > }; > > I don't think there's a way to simplify it further. That binding looks completely inconsistent to me. If we can write it either using a ports node with port@X subnodes, in my opinion we can equally well just use a unidirectional link for the case where we don't need the link back from the panel to the output. Where's the difference? Consider for example the case where an LVDS panel is connected to a simple LVDS output. For what possible reason would the panel need to access the output? > > > I also plan to specify video bus parameters in DT for CDF, but this hasn't > > > been finalized yet. > > > > That effectively blocks any of the code that I've been working on until > > CDF has been merged. It's been discussed for over a year now, and there > > has even been significant pushback on using CDF in DRM, so even if CDF > > is eventually merged, that will still leave DRM with no way to turn on > > a simple panel. > > I'm probably even more concerned about the long time all this took and will > still take than you are, sincerely. I'm obviously to blame for not being able > to explain the problems and solutions correctly, as I see no way why people > would push back if they realized that the other solutions that have been > proposed, far from being future-proof, won't even be present-proof. We would > be shooting ourselves in the foot, but what can I do when too many people want > to do so ? I'm curious why you think this current proposal isn't present-proof. All three panels that I've tested with work with this proposal currently and I'm convinced that many more setups can be covered using something as simple as this. > I'll still keep trying of course, but not for years.I have yet to > see someone proposing a viable solution to share drivers between DRM and V4L2, > which is a real pressing requirement, partly due to DT. Yeah... I still don't get that side of the argument. Why exactly do we need to share the drivers again? If we really need to support display output within V4L2, shouldn't we instead improve interoperability between the two and use DRM/KMS exclusively for the output part? Then the problem of having to share drivers between subsystems goes away and we'll actually be using the right subsystem for the right job at hand. Of course none of that is relevant to the topic of DT bindings, which is what we should probably be concentrating on. Nobody's been able to come up with a valid reason why the proposed binding is broken. It works for the tested devices (likely many more as well) and I don't forsee why it can't be future-proof. Why would it suddenly need to change? And even if it needed to change, why would the changes be backwards-incompatible rather than simply additional features that don't impact existing users? > Regarding the use of CDF in DRM, please note that it will be optional for > drivers to switch to the new framework. Nothing should hopefully require > existing drivers to be converted, drivers that want to make use of CDF to > support panels will be welcome to do so, but they big DRM drivers for desktop > GPUs won't be affected. I'm not at all opposed to the idea of CDF or the problems it tries to address. I don't think it necessarily should be a separate framework for reasons explained earlier. But even if it were to end up in a separate framework there's absolutely nothing preventing us from adding a DRM panel driver that hooks into CDF as a "backend" of sorts for panels that require something more complex than the simple-panel binding. > > I keep wondering why we absolutely must have compatibility between CDF > > and this simple panel binding. DT content is supposed to concern itself > > with the description of hardware only. What about the following isn't an > > accurate description of the panel hardware? > > > > panel: panel { > > compatible = "cptt,claa101wb01"; > > > > power-supply = <&vdd_pnl_reg>; > > enable-gpios = <&gpio 90 0>; > > > > backlight = <&backlight>; > > }; > > > > dsi-controller { > > panel = <&panel>; > > }; > > > > Note how the above is also not incompatible with anything that the CDF > > (to be) mandates, so it could easily be extended with any nodes or > > properties that CDF needs to work properly without breaking backwards- > > compatibility. > > As mentioned above, CDF needs to describe more complex pipelines. To make the > driver life as simple as possible the core code will handle the pipeline > description in the DT bindings, and it does require it to be generic enough. > You can of course describe connections in various ways depending on the > hardware, which would make the DT-related code needlessly overcomplex. One of > the issue with the above description is that there's no way to go from the > panel to the display controller, which might be a show stopper in the future > when code requires that. A workaround would be to look through the whole DT > for the reverse link, which would be pretty inefficient. But that's precisely the point. Why would we need to go back from the panel to the display controller? Especially for dumb panels that can't or don't have to be configured in any way. Even if they needed some sort of setup, why can't that be done from the display controller/output. Even given a setup where a DSI controller needs to write some registers in a panel upon initialization, I don't see why the reverse connection needs to be described. The fact alone that an output dereferences a panel's phandle should be enough to connect both of them and have any panel driver use the DSI controller that it's been attached to for the programming. That's very much analogous to how I2C works. There are no connections back to the I2C master from the slaves. Yet each I2C client driver manages to use the services provided by the I2C master to perform transactions on the I2C bus. In a similar way the DSI controller is the bus master that talks to DSI panels. DSI panels don't actively talk to the DSI controller. > > > > +static void panel_simple_enable(struct drm_panel *panel) > > > > +{ > > > > + struct panel_simple *p = to_panel_simple(panel); > > > > + int err; > > > > + > > > > + if (p->enabled) > > > > + return; > > > > + > > > > + err = regulator_enable(p->supply); > > > > + if (err < 0) > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err); > > > > > > Is that really a non-fatal error ? Shouldn't the enable operation return > > > an int ? > > > > There's no way to propagate this in DRM, so why go through the trouble > > of returning the error? Furthermore, there's nothing that the caller > > could do to remedy the situation anyway. > > That's a DRM issue, which could be fixed. While the caller can't remedy the > situation, it should at least report the error to the application instead of > silently ignoring it. Perhaps. It's not really relevant to the discussion and can always be fixed in a subsequent patch. > > > > +static const struct drm_display_mode auo_b101aw03_mode = { > > > > + .clock = 51450, > > > > + .hdisplay = 1024, > > > > + .hsync_start = 1024 + 156, > > > > + .hsync_end = 1024 + 156 + 8, > > > > + .htotal = 1024 + 156 + 8 + 156, > > > > + .vdisplay = 600, > > > > + .vsync_start = 600 + 16, > > > > + .vsync_end = 600 + 16 + 6, > > > > + .vtotal = 600 + 16 + 6 + 16, > > > > + .vrefresh = 60, > > > > +}; > > > > > > Instead of hardcoding the modes in the driver, which would then require to > > > be updated for every new simple panel model (and we know there are lots > > > of them), why don't you specify the modes in the panel DT node ? The > > > simple panel driver would then become much more generic. It would also > > > allow board designers to tweak h/v sync timings depending on the system > > > requirements. > > > > Sigh... we keep second-guessing ourselves. Back at the time when power > > sequences were designed (and NAKed at the last minute), we all decided > > that the right thing to do would be to use specific compatible values > > for individual panels, because that would allow us to encode the power > > sequencing within the driver. And when we already have the panel model > > encoded in the compatible value, we might just as well encode the mode > > within the driver for that panel. Otherwise we'll have to keep adding > > the same mode timings for every board that uses the same panel. > > > > I do agree though that it might be useful to tweak the mode in case the > > default one doesn't work. How about we provide a means to override the > > mode encoded in the driver using one specified in the DT? That's > > obviously a backwards-compatible change, so it could be added if or when > > it becomes necessary. > > I share Tomi's point of view here. Your three panels use the same power > sequence, so I believe we should have a generic panel compatible string that > would use modes described in DT for the common case. Only if a panel required > something more complex which can't (or which could, but won't for any reason) > accurately be described in DT would you need to extend the driver. I don't see the point quite frankly. You seem to be assuming that every panel will only be used in a single board. However what you're proposing will require the mode timings to be repeated in every board's DT file, if the same panel is ever used on more than a single board. Thierry
On 17/10/13 14:05, Thierry Reding wrote: >> That's quite similar to how my current out-of-mainline OMAP DSS DT >> bindings work. The difference is that I have a reference from the panel >> node to the video source (dsi-controller), instead of the other way >> around. I just find it more natural. It works the same way as, say, >> regulators, gpios, etc. > > I suppose that depends on the way you look at it. In the above proposal > I consider the output (DSI controller) to use the panel as a resource > that provides a certain set of services (query mode timings, provide a > way to enable or disable the panel). Similarly the panel uses the > backlight as a resource that provides a certain set of services (such as > changing the brightness). > > The above also follows the natural order of control. The panel has no > way to control the DSI output. However, it is the output that controls > when a panel is required and turn it on as appropriate. I've discussed this issue to death with other people, and for some reason I still see this the other way =). I see the panel using the DSI controller as a resource. The panel driver uses the DSI controller to send the video data to the panel device. Compare this to, say, i2c client driver, which uses i2c master to send data to the i2c device. In the OMAP DSS driver model, the panel driver is in control. The panel driver tells the DSI controller which kind of video timings to use, or which bus speed to use, or whether to use low-power or high-speed mode, when to enable the DSI clock or DSI video stream, etc. With a simple panel the model you describe works usually fine. The reason I went with the different model is that we had rather complex display peripherals, that needed more specific control of the bus. A simple example could be the enable sequence. I presume in your model the DSI controller's enable would be maybe something like: configure_dsi_timings(panel->get_timings()); enable_dsi_output(); panel->enable(); What if the DSI peripheral requires an enable sequence of, say: - enable DSI clock - use LP mode - send some config messages - re-configure DSI clock to higher speed - send some more config messages - enable HS mode - send some more config messages - enable DSI video stream It gets a bit difficult to manage that kind of setup in a generic way in the DSI controller driver. As I see it, the DSI peripheral driver has to be in control of the nuances. The same goes for other video buses also, but DSI is perhaps one of the most complex ones to support. Tomi
Hi Tomi, On Thursday 17 October 2013 14:35:49 Tomi Valkeinen wrote: > On 17/10/13 14:02, Laurent Pinchart wrote: > >> Okay, so if I understand correctly, translating those bindings to panel > >> > >> nodes would look somewhat like this: > >> dc: display-controller { > >> ports { > >> port@0 { > >> remote-endpoint = <&panel>; > >> }; > >> }; > >> }; > >> > >> panel: panel { > >> ports { > >> port@0 { > >> remote-endpoint = <&dc>; > >> }; > >> }; > >> }; > >> > >> The above leaves out any of the other, non-relevant properties. Does > >> that sound about right? > > > > Yes it does. > > It does? > > Shouldn't it be something like: > > panel { > ports { > port@0 { > endpoint@0 { > remote = <&dc>; > }; > }; > }; > }; > > And simplified: > > panel { > port { > endpoint@0 { > remote = <&dc>; > }; > }; > }; > > You do need a node for the endpoint, a remote-endpoint property is not > enough. My bad, you'r absolutely right. More sleep is needed. (And while we're at it, the remote-endpoint properties must point to an endpoint, not the device DT node. > > Please note that, when a device has as single port, the ports node can be > > omitted, and the port doesn't need to be numbered. You would then end up > > with> > > dc: display-controller { > > port { > > remote-endpoint = <&panel>; > > }; > > }; > > > > panel: panel { > > port { > > remote-endpoint = <&dc>; > > }; > > }; > > > > I don't think there's a way to simplify it further. > > I'm not sure if there's a specific need for the port or endpoint nodes > in cases like the above. Even if we have common properties describing > the endpoint, I guess they could just be in the parent node. > > panel { > remote = <&dc>; > common-video-property = <asd>; > }; > > The above would imply one port and one endpoint. Would that work? If we > had a function like parse_endpoint(node), we could just point it to > either a real endpoint node, or to the device's node. You reference the display controller here, not a specific display controller output. Don't most display controllers have several outputs ?
On 17/10/13 14:51, Laurent Pinchart wrote: >> I'm not sure if there's a specific need for the port or endpoint nodes >> in cases like the above. Even if we have common properties describing >> the endpoint, I guess they could just be in the parent node. >> >> panel { >> remote = <&dc>; >> common-video-property = <asd>; >> }; >> >> The above would imply one port and one endpoint. Would that work? If we >> had a function like parse_endpoint(node), we could just point it to >> either a real endpoint node, or to the device's node. > > You reference the display controller here, not a specific display controller > output. Don't most display controllers have several outputs ? Sure. Then the display controller could have more verbose description. But the panel could still have what I wrote above, except the 'remote' property would point to a real endpoint node inside the dispc node, not to the dispc node. This would, of course, need some extra code to handle the different cases, but just from DT point of view, I think all the relevant information is there. Tomi
On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Thursday 17 October 2013 13:05:18 Thierry Reding wrote: > > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote: > > > On 17/10/13 11:53, Thierry Reding wrote: > > > > I keep wondering why we absolutely must have compatibility between CDF > > > > and this simple panel binding. DT content is supposed to concern itself > > > > with the description of hardware only. What about the following isn't an > > > > accurate description of the panel hardware? > > > > > > > > panel: panel { > > > > compatible = "cptt,claa101wb01"; > > > > > > > > power-supply = <&vdd_pnl_reg>; > > > > enable-gpios = <&gpio 90 0>; > > > > > > > > backlight = <&backlight>; > > > > }; > > > > > > > > dsi-controller { > > > > panel = <&panel>; > > > > }; > > > > > > That's quite similar to how my current out-of-mainline OMAP DSS DT > > > bindings work. The difference is that I have a reference from the panel > > > node to the video source (dsi-controller), instead of the other way > > > around. I just find it more natural. It works the same way as, say, > > > regulators, gpios, etc. > > > > I suppose that depends on the way you look at it. In the above proposal > > I consider the output (DSI controller) to use the panel as a resource > > that provides a certain set of services (query mode timings, provide a > > way to enable or disable the panel). Similarly the panel uses the > > backlight as a resource that provides a certain set of services (such as > > changing the brightness). > > > > The above also follows the natural order of control. The panel has no > > way to control the DSI output. However, it is the output that controls > > when a panel is required and turn it on as appropriate. > > I'm no DSI expert, but I know enough about it to be sure that Tomi will > disagree. DSI panels can have complex power sequences that require the input > stream to be finely controlled (for instance it might require a clock without > video frames for some time, switch a GPIO or regulator, send a command to the > panel, and then only get video frames). For that reason all developers I've > talked to who have an in-depth knowledge of DSI and DSI panels told me that > the panel needs to control the video bus, and request the video source to > start/stop the video stream. Oh, I'm very well aware of the various flavours of funkiness that DSI panels come in. But it's wrong to say that the panel needs to control the video bus. There's simply no way that a panel can actually do that. It is true, however, that in order to make this work in a maintainable fashion, the DSI panel *driver* may need to control the DSI bus. That's an entirely different story. Again, that's exactly how I2C works as well. I2C slaves, by definition, do not control the I2C bus. However, I2C client drivers can do so by using the (master) services provided by the I2C controller. So for DSI what we could easily do is provide a DSI "adapter" to the panel when it is attached to the DSI controller. If the DSI adapter implements all of the required services, then the DSI panel driver is easily capable of handling all the required special quirks. I'm Cc'ing Rob and Daniel, both of whom have been working on this lately. Actually I'm not sure if Daniel was directly involved, but at least somebody from Intel's team was working on it. > > > However, one thing unclear is where the panel node should be. You seem > > > to have a DSI panel here. If the panel is truly not controlled in any > > > way, maybe having the panel as a normal platform device is ok. But if > > > the DSI panel is controlled with DSI messages, should it be a child of > > > the dsi-controller node, the same way i2c peripherals are children of > > > i2c master? > > > > That's one possibility. In this particular case it's not even necessary > > to use any of DSIs control methods to talk to the panel. The panel only > > receives the data stream and "just works". > > > > Even if the panel were to be controlled via DSI, I think keeping it as > > a separate device node is equally valid. It's really boils down to what > > I already said above. The output uses the panel as a resource, similar > > to how other devices might use a GPIO controller to use a GPIO pin, or > > an interrupt controller to request an IRQ. > > Except that the display controller can also provides resources to the panel. > For DSI panels that support DSI commands, the control bus is provided by the > display controller. Much like an I2C device must be a child of its I2C > controller node, the DSI panel device should then be a child of the display > controller node. I don't think resources is the right term here. The DSI controller provides services. That's traditionally been handled in DT by handing out phandles to the service providers. That said I see why it would make sense to make a DSI panel the child of the DSI controller node, so we agree at least partially. I don't like it all that much because it makes handling of panels inconsistent across various types of output and therefore makes it more complicated to support it drivers, but I'm sure I can live with it if somebody wants to enforce that. > > > > I do agree though that it might be useful to tweak the mode in case the > > > > default one doesn't work. How about we provide a means to override the > > > > mode encoded in the driver using one specified in the DT? That's > > > > obviously a backwards-compatible change, so it could be added if or when > > > > it becomes necessary. > > > > > > This sounds good to me. > > > > > > Although maybe it'd be good to have the driver compatible with something > > > like "generic-panel", so that you could have: > > > > > > compatible = "foo,specific-panel", "generic-panel"; > > > > > > and if there's no need for any power/gpio setup for your board, you may > > > skip adding "foo,specific-panel" support to the panel driver. Later, > > > somebody else may need to implement fine grained power/gpio support for > > > "foo,specific-panel", and it would just work. > > > > > > Maybe that would help us with the problem of adding support in the > > > driver for a hundred different simple panels each used only once on a > > > specific board. > > > > Sure, that all sounds like reasonable additions. All of the suggestions > > are even backwards-compatible and hence can be added when needed without > > breaking compatibility with existing users of the binding. > > What's wrong with moving the three hardcoded modes we already have in the > driver to DT before pushing this to mainline ? I think I've answered that in a different subthread. Thierry
On Thu, Oct 17, 2013 at 02:50:31PM +0300, Tomi Valkeinen wrote: > On 17/10/13 14:05, Thierry Reding wrote: > > >> That's quite similar to how my current out-of-mainline OMAP DSS DT > >> bindings work. The difference is that I have a reference from the panel > >> node to the video source (dsi-controller), instead of the other way > >> around. I just find it more natural. It works the same way as, say, > >> regulators, gpios, etc. > > > > I suppose that depends on the way you look at it. In the above proposal > > I consider the output (DSI controller) to use the panel as a resource > > that provides a certain set of services (query mode timings, provide a > > way to enable or disable the panel). Similarly the panel uses the > > backlight as a resource that provides a certain set of services (such as > > changing the brightness). > > > > The above also follows the natural order of control. The panel has no > > way to control the DSI output. However, it is the output that controls > > when a panel is required and turn it on as appropriate. > > I've discussed this issue to death with other people, and for some > reason I still see this the other way =). > > I see the panel using the DSI controller as a resource. The panel driver > uses the DSI controller to send the video data to the panel device. > Compare this to, say, i2c client driver, which uses i2c master to send > data to the i2c device. > > In the OMAP DSS driver model, the panel driver is in control. The panel > driver tells the DSI controller which kind of video timings to use, or > which bus speed to use, or whether to use low-power or high-speed mode, > when to enable the DSI clock or DSI video stream, etc. > > With a simple panel the model you describe works usually fine. The > reason I went with the different model is that we had rather complex > display peripherals, that needed more specific control of the bus. > > A simple example could be the enable sequence. I presume in your model > the DSI controller's enable would be maybe something like: > > configure_dsi_timings(panel->get_timings()); > enable_dsi_output(); > panel->enable(); > > What if the DSI peripheral requires an enable sequence of, say: > > - enable DSI clock > - use LP mode > - send some config messages > - re-configure DSI clock to higher speed > - send some more config messages > - enable HS mode > - send some more config messages > - enable DSI video stream > > It gets a bit difficult to manage that kind of setup in a generic way in > the DSI controller driver. As I see it, the DSI peripheral driver has to > be in control of the nuances. The same goes for other video buses also, > but DSI is perhaps one of the most complex ones to support. I've briefly gone over this in another subthread, but here it is again for the sake of completeness. The difference in perception I think comes from the fact that the DSI panel *driver* may need to control the DSI bus. However, DT describes the hardware in terms of devices. And from that point of view it is still the DSI _control_ler that _control_s the panel. There's no electrical way that the panel can take possession of the bus. Well, there's BTA, but even that happens under supervision of the DSI controller. Thierry
On Thursday 17 October 2013 13:41:40 Thierry Reding wrote: > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote: > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: > [...] > > > CDF needs to model video pipelines that can be more complex than just a > > display controller with a single output connected to a panel with a single > > input. For that reason the DT bindings need to describe connections in a > > generic enough way. The above DT fragment might be slightly more complex > > than one would expect when thinking about the really simple case only, > > but it's nowhere near overly complex in my opinion. > > > > Please note that, when a device has as single port, the ports node can be > > omitted, and the port doesn't need to be numbered. You would then end up > > with> > > dc: display-controller { > > port { > > remote-endpoint = <&panel>; > > }; > > }; > > > > panel: panel { > > port { > > remote-endpoint = <&dc>; > > }; > > }; > > > > I don't think there's a way to simplify it further. > > That binding looks completely inconsistent to me. If we can write it either > using a ports node with port@X subnodes, in my opinion we can equally well > just use a unidirectional link for the case where we don't need the link > back from the panel to the output. Where's the difference? > > Consider for example the case where an LVDS panel is connected to a simple > LVDS output. For what possible reason would the panel need to access the > output? The point is that the video pipeline must be described in DT. Having a per- device way to represent connections would be a nightmare to support from an implementation point of view, hence the need for a generic way to describe them. > > > > I also plan to specify video bus parameters in DT for CDF, but this > > > > hasn't been finalized yet. > > > > > > That effectively blocks any of the code that I've been working on until > > > CDF has been merged. It's been discussed for over a year now, and there > > > has even been significant pushback on using CDF in DRM, so even if CDF > > > is eventually merged, that will still leave DRM with no way to turn on > > > a simple panel. > > > > I'm probably even more concerned about the long time all this took and > > will still take than you are, sincerely. I'm obviously to blame for not > > being able to explain the problems and solutions correctly, as I see no > > way why people would push back if they realized that the other solutions > > that have been proposed, far from being future-proof, won't even be > > present-proof. We would be shooting ourselves in the foot, but what can I > > do when too many people want to do so ? > > I'm curious why you think this current proposal isn't present-proof. All > three panels that I've tested with work with this proposal currently and > I'm convinced that many more setups can be covered using something as > simple as this. My apologies for not being clear enough. I wasn't implying that your proposal isn't present-proof, I was talking about the counter-proposals that have been made in reaction to CDF. > > I'll still keep trying of course, but not for years.I have yet to > > see someone proposing a viable solution to share drivers between DRM and > > V4L2, which is a real pressing requirement, partly due to DT. > > Yeah... I still don't get that side of the argument. Why exactly do we need > to share the drivers again? Think about a scaler IP in a SoC or FPGA, with one instance used in a camera capture pipeline, supported by a V4L2 driver, and one instance used in a video output pipeline, supported by a DRM driver. We want a single driver for that IP core. Same story for video encoders. > If we really need to support display output within V4L2, shouldn't we > instead improve interoperability between the two and use DRM/KMS exclusively > for the output part? Then the problem of having to share drivers between > subsystems goes away and we'll actually be using the right subsystem for the > right job at hand. In the long term we definitely should improve interoperability between the two. I'd go further than that, having one API for video capture and one API for video output doesn't make much sense, except for historical reasons. > Of course none of that is relevant to the topic of DT bindings, which is > what we should probably be concentrating on. It actually is. Given that DT bindings must describe hardware, the scaler (or encoder, or other entity) would use the same bindings regardless of the Linux subsystem that will be involved. We thus need to make sure the bindings will be usable on both sides. > Nobody's been able to come up with a valid reason why the proposed binding > is broken. It works for the tested devices (likely many more as well) and I > don't forsee why it can't be future-proof. Why would it suddenly need to > change? And even if it needed to change, why would the changes be backwards- > incompatible rather than simply additional features that don't impact > existing users? > > > Regarding the use of CDF in DRM, please note that it will be optional for > > drivers to switch to the new framework. Nothing should hopefully require > > existing drivers to be converted, drivers that want to make use of CDF to > > support panels will be welcome to do so, but they big DRM drivers for > > desktop GPUs won't be affected. > > I'm not at all opposed to the idea of CDF or the problems it tries to > address. I don't think it necessarily should be a separate framework for > reasons explained earlier. But even if it were to end up in a separate > framework there's absolutely nothing preventing us from adding a DRM panel > driver that hooks into CDF as a "backend" of sorts for panels that require > something more complex than the simple-panel binding. Once again it's not about panel having complex needs, but more about using simple panels in complex pipelines. I'm fine with the drm_panel infrastructure, what I would like is DT bindings that describe connections in a more future-proof way. The rest is fine. > > > I keep wondering why we absolutely must have compatibility between CDF > > > and this simple panel binding. DT content is supposed to concern itself > > > with the description of hardware only. What about the following isn't an > > > accurate description of the panel hardware? > > > > > > panel: panel { > > > compatible = "cptt,claa101wb01"; > > > > > > power-supply = <&vdd_pnl_reg>; > > > enable-gpios = <&gpio 90 0>; > > > > > > backlight = <&backlight>; > > > }; > > > > > > dsi-controller { > > > panel = <&panel>; > > > }; > > > > > > Note how the above is also not incompatible with anything that the CDF > > > (to be) mandates, so it could easily be extended with any nodes or > > > properties that CDF needs to work properly without breaking backwards- > > > compatibility. > > > > As mentioned above, CDF needs to describe more complex pipelines. To make > > the driver life as simple as possible the core code will handle the > > pipeline description in the DT bindings, and it does require it to be > > generic enough. You can of course describe connections in various ways > > depending on the hardware, which would make the DT-related code > > needlessly overcomplex. One of the issue with the above description is > > that there's no way to go from the panel to the display controller, which > > might be a show stopper in the future when code requires that. A > > workaround would be to look through the whole DT for the reverse link, > > which would be pretty inefficient. > > But that's precisely the point. Why would we need to go back from the panel > to the display controller? Especially for dumb panels that can't or don't > have to be configured in any way. Even if they needed some sort of setup, > why can't that be done from the display controller/output. > > Even given a setup where a DSI controller needs to write some registers in a > panel upon initialization, I don't see why the reverse connection needs to > be described. The fact alone that an output dereferences a panel's phandle > should be enough to connect both of them and have any panel driver use the > DSI controller that it's been attached to for the programming. > > That's very much analogous to how I2C works. There are no connections back > to the I2C master from the slaves. Yet each I2C client driver manages to use > the services provided by the I2C master to perform transactions on the I2C > bus. In a similar way the DSI controller is the bus master that talks to DSI > panels. DSI panels don't actively talk to the DSI controller. It's all about modeling video pipeline graphs in DT. To be able to walk the graph we need to describe connections. Not having bidirectional information means that we restrict the points at which we can start walking the graph, potentially making our life much more difficult in the future. What's so wrong about adding a port node and link information to the panel DT node ? It describe what's there: the panel has one input, why not make that explicit ? > > > > > +static void panel_simple_enable(struct drm_panel *panel) > > > > > +{ > > > > > + struct panel_simple *p = to_panel_simple(panel); > > > > > + int err; > > > > > + > > > > > + if (p->enabled) > > > > > + return; > > > > > + > > > > > + err = regulator_enable(p->supply); > > > > > + if (err < 0) > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err); > > > > > > > > Is that really a non-fatal error ? Shouldn't the enable operation > > > > return an int ? > > > > > > There's no way to propagate this in DRM, so why go through the trouble > > > of returning the error? Furthermore, there's nothing that the caller > > > could do to remedy the situation anyway. > > > > That's a DRM issue, which could be fixed. While the caller can't remedy > > the situation, it should at least report the error to the application > > instead of silently ignoring it. > > Perhaps. It's not really relevant to the discussion and can always be > fixed in a subsequent patch. Most things can be fixed by a subsequent patch, that's not a good enough reason not to address the known problems before pushing the code to mainline (to clarify my point of view, there's no need to fix DRM to use the return value before pushing this patch set to mainline, but I'd like a v2 with an int return value). > > > > > +static const struct drm_display_mode auo_b101aw03_mode = { > > > > > + .clock = 51450, > > > > > + .hdisplay = 1024, > > > > > + .hsync_start = 1024 + 156, > > > > > + .hsync_end = 1024 + 156 + 8, > > > > > + .htotal = 1024 + 156 + 8 + 156, > > > > > + .vdisplay = 600, > > > > > + .vsync_start = 600 + 16, > > > > > + .vsync_end = 600 + 16 + 6, > > > > > + .vtotal = 600 + 16 + 6 + 16, > > > > > + .vrefresh = 60, > > > > > +}; > > > > > > > > Instead of hardcoding the modes in the driver, which would then > > > > require to be updated for every new simple panel model (and we know > > > > there are lots of them), why don't you specify the modes in the panel > > > > DT node ? The simple panel driver would then become much more generic. > > > > It would also allow board designers to tweak h/v sync timings > > > > depending on the system requirements. > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when power > > > sequences were designed (and NAKed at the last minute), we all decided > > > that the right thing to do would be to use specific compatible values > > > for individual panels, because that would allow us to encode the power > > > sequencing within the driver. And when we already have the panel model > > > encoded in the compatible value, we might just as well encode the mode > > > within the driver for that panel. Otherwise we'll have to keep adding > > > the same mode timings for every board that uses the same panel. > > > > > > I do agree though that it might be useful to tweak the mode in case the > > > default one doesn't work. How about we provide a means to override the > > > mode encoded in the driver using one specified in the DT? That's > > > obviously a backwards-compatible change, so it could be added if or when > > > it becomes necessary. > > > > I share Tomi's point of view here. Your three panels use the same power > > sequence, so I believe we should have a generic panel compatible string > > that would use modes described in DT for the common case. Only if a panel > > required something more complex which can't (or which could, but won't > > for any reason) accurately be described in DT would you need to extend > > the driver. > > I don't see the point quite frankly. You seem to be assuming that every > panel will only be used in a single board. No, but in practice that's often the case, at least for boards with .dts files in the mainline kernel. > However what you're proposing will require the mode timings to be repeated > in every board's DT file, if the same panel is ever used on more than a > single board. Is that a problem ? You could #include a .dtsi for the panel that would specify the video mode if you want to avoid multiple copies.
Hi Tomi, On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote: > On 17/10/13 14:51, Laurent Pinchart wrote: > >> I'm not sure if there's a specific need for the port or endpoint nodes > >> in cases like the above. Even if we have common properties describing > >> the endpoint, I guess they could just be in the parent node. > >> > >> panel { > >> remote = <&dc>; > >> common-video-property = <asd>; > >> }; > >> > >> The above would imply one port and one endpoint. Would that work? If we > >> had a function like parse_endpoint(node), we could just point it to > >> either a real endpoint node, or to the device's node. > > > > You reference the display controller here, not a specific display > > controller output. Don't most display controllers have several outputs ? > > Sure. Then the display controller could have more verbose description. > But the panel could still have what I wrote above, except the 'remote' > property would point to a real endpoint node inside the dispc node, not > to the dispc node. > > This would, of course, need some extra code to handle the different > cases, but just from DT point of view, I think all the relevant > information is there. There's many ways to describe the same information in DT. While we could have DT bindings that use different descriptions for different devices and still support all of them in our code, why should we opt for that option that will make the implementation much more complex when we can describe connections in a simple and generic way ?
Hi Thierry, On Thursday 17 October 2013 14:06:47 Thierry Reding wrote: > On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote: > > On Thursday 17 October 2013 13:05:18 Thierry Reding wrote: > > > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote: > > > > On 17/10/13 11:53, Thierry Reding wrote: > > > > > I keep wondering why we absolutely must have compatibility between > > > > > CDF and this simple panel binding. DT content is supposed to concern > > > > > itself with the description of hardware only. What about the > > > > > following isn't an accurate description of the panel hardware? > > > > > > > > > > panel: panel { > > > > > compatible = "cptt,claa101wb01"; > > > > > > > > > > power-supply = <&vdd_pnl_reg>; > > > > > enable-gpios = <&gpio 90 0>; > > > > > > > > > > backlight = <&backlight>; > > > > > }; > > > > > > > > > > dsi-controller { > > > > > panel = <&panel>; > > > > > }; > > > > > > > > That's quite similar to how my current out-of-mainline OMAP DSS DT > > > > bindings work. The difference is that I have a reference from the > > > > panel node to the video source (dsi-controller), instead of the other > > > > way around. I just find it more natural. It works the same way as, > > > > say, regulators, gpios, etc. > > > > > > I suppose that depends on the way you look at it. In the above proposal > > > I consider the output (DSI controller) to use the panel as a resource > > > that provides a certain set of services (query mode timings, provide a > > > way to enable or disable the panel). Similarly the panel uses the > > > backlight as a resource that provides a certain set of services (such as > > > changing the brightness). > > > > > > The above also follows the natural order of control. The panel has no > > > way to control the DSI output. However, it is the output that controls > > > when a panel is required and turn it on as appropriate. > > > > I'm no DSI expert, but I know enough about it to be sure that Tomi will > > disagree. DSI panels can have complex power sequences that require the > > input stream to be finely controlled (for instance it might require a > > clock without video frames for some time, switch a GPIO or regulator, > > send a command to the panel, and then only get video frames). For that > > reason all developers I've talked to who have an in-depth knowledge of > > DSI and DSI panels told me that the panel needs to control the video bus, > > and request the video source to start/stop the video stream. > > Oh, I'm very well aware of the various flavours of funkiness that DSI > panels come in. But it's wrong to say that the panel needs to control > the video bus. There's simply no way that a panel can actually do that. > It is true, however, that in order to make this work in a maintainable > fashion, the DSI panel *driver* may need to control the DSI bus. That's > an entirely different story. Sure, but I don't think that's really related to the DT bindings. We don't have to model every electrical signal in a detailed way that matches the direction of the electrons flow :-) What we need to model is a connection between a display controller and a panel (possibly with a direction). What I'd like to do is to express that link in a way that can also express more complex pipeline topologies. I don't want to make it overly complex, I had hoped that my DT bindings proposal would be a good approach as it's both generic and still pretty simple. > Again, that's exactly how I2C works as well. I2C slaves, by definition, > do not control the I2C bus. However, I2C client drivers can do so by > using the (master) services provided by the I2C controller. > > So for DSI what we could easily do is provide a DSI "adapter" to the > panel when it is attached to the DSI controller. If the DSI adapter > implements all of the required services, then the DSI panel driver is > easily capable of handling all the required special quirks. > > I'm Cc'ing Rob and Daniel, both of whom have been working on this > lately. Actually I'm not sure if Daniel was directly involved, but at > least somebody from Intel's team was working on it. > > > > > However, one thing unclear is where the panel node should be. You seem > > > > to have a DSI panel here. If the panel is truly not controlled in any > > > > way, maybe having the panel as a normal platform device is ok. But if > > > > the DSI panel is controlled with DSI messages, should it be a child of > > > > the dsi-controller node, the same way i2c peripherals are children of > > > > i2c master? > > > > > > That's one possibility. In this particular case it's not even necessary > > > to use any of DSIs control methods to talk to the panel. The panel only > > > receives the data stream and "just works". > > > > > > Even if the panel were to be controlled via DSI, I think keeping it as > > > a separate device node is equally valid. It's really boils down to what > > > I already said above. The output uses the panel as a resource, similar > > > to how other devices might use a GPIO controller to use a GPIO pin, or > > > an interrupt controller to request an IRQ. > > > > Except that the display controller can also provides resources to the > > panel. For DSI panels that support DSI commands, the control bus is > > provided by the display controller. Much like an I2C device must be a > > child of its I2C controller node, the DSI panel device should then be a > > child of the display controller node. > > I don't think resources is the right term here. The DSI controller > provides services. That's traditionally been handled in DT by handing > out phandles to the service providers. > > That said I see why it would make sense to make a DSI panel the child of > the DSI controller node, so we agree at least partially. I don't like it > all that much because it makes handling of panels inconsistent across > various types of output and therefore makes it more complicated to > support it drivers, but I'm sure I can live with it if somebody wants to > enforce that. > > > > > > I do agree though that it might be useful to tweak the mode in case > > > > > the default one doesn't work. How about we provide a means to > > > > > override the mode encoded in the driver using one specified in the > > > > > DT? That's obviously a backwards-compatible change, so it could be > > > > > added if or when it becomes necessary. > > > > > > > > This sounds good to me. > > > > > > > > Although maybe it'd be good to have the driver compatible with > > > > something like "generic-panel", so that you could have: > > > > > > > > compatible = "foo,specific-panel", "generic-panel"; > > > > > > > > and if there's no need for any power/gpio setup for your board, you > > > > may skip adding "foo,specific-panel" support to the panel driver. > > > > Later, somebody else may need to implement fine grained power/gpio > > > > support for "foo,specific-panel", and it would just work. > > > > > > > > Maybe that would help us with the problem of adding support in the > > > > driver for a hundred different simple panels each used only once on a > > > > specific board. > > > > > > Sure, that all sounds like reasonable additions. All of the suggestions > > > are even backwards-compatible and hence can be added when needed without > > > breaking compatibility with existing users of the binding. > > > > What's wrong with moving the three hardcoded modes we already have in the > > driver to DT before pushing this to mainline ? > > I think I've answered that in a different subthread. Then I might not have understood your answer :-)
On 17/10/13 15:17, Laurent Pinchart wrote: > Hi Tomi, > > On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote: >> On 17/10/13 14:51, Laurent Pinchart wrote: >>>> I'm not sure if there's a specific need for the port or endpoint nodes >>>> in cases like the above. Even if we have common properties describing >>>> the endpoint, I guess they could just be in the parent node. >>>> >>>> panel { >>>> remote = <&dc>; >>>> common-video-property = <asd>; >>>> }; >>>> >>>> The above would imply one port and one endpoint. Would that work? If we >>>> had a function like parse_endpoint(node), we could just point it to >>>> either a real endpoint node, or to the device's node. >>> >>> You reference the display controller here, not a specific display >>> controller output. Don't most display controllers have several outputs ? >> >> Sure. Then the display controller could have more verbose description. >> But the panel could still have what I wrote above, except the 'remote' >> property would point to a real endpoint node inside the dispc node, not >> to the dispc node. >> >> This would, of course, need some extra code to handle the different >> cases, but just from DT point of view, I think all the relevant >> information is there. > > There's many ways to describe the same information in DT. While we could have > DT bindings that use different descriptions for different devices and still > support all of them in our code, why should we opt for that option that will > make the implementation much more complex when we can describe connections in > a simple and generic way ? My suggestion was simple and generic. I'm not proposing per-device custom bindings. My point was, if we can describe the connections as I described above, which to me sounds possible, we can simplify the panel DT data for 99.9% of the cases. To me, the first of these looks much nicer: panel { remote = <&remote-endpoint>; common-video-property = <asd>; }; panel { port { endpoint { remote = <&remote-endpoint>; common-video-property = <asd>; }; }; }; If that can be supported in the SW by adding complexity to a few functions, and it covers practically all the panels, isn't it worth it? Note that I have not tried this, so I don't know if there are issues. It's just a thought. Even if there's need for a endpoint node, perhaps the port node can be made optional. Tomi
On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote: > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote: > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote: > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: > > [...] > > > > > CDF needs to model video pipelines that can be more complex than just a > > > display controller with a single output connected to a panel with a single > > > input. For that reason the DT bindings need to describe connections in a > > > generic enough way. The above DT fragment might be slightly more complex > > > than one would expect when thinking about the really simple case only, > > > but it's nowhere near overly complex in my opinion. > > > > > > Please note that, when a device has as single port, the ports node can be > > > omitted, and the port doesn't need to be numbered. You would then end up > > > with> > > > dc: display-controller { > > > port { > > > remote-endpoint = <&panel>; > > > }; > > > }; > > > > > > panel: panel { > > > port { > > > remote-endpoint = <&dc>; > > > }; > > > }; > > > > > > I don't think there's a way to simplify it further. > > > > That binding looks completely inconsistent to me. If we can write it either > > using a ports node with port@X subnodes, in my opinion we can equally well > > just use a unidirectional link for the case where we don't need the link > > back from the panel to the output. Where's the difference? > > > > Consider for example the case where an LVDS panel is connected to a simple > > LVDS output. For what possible reason would the panel need to access the > > output? > > The point is that the video pipeline must be described in DT. Having a per- > device way to represent connections would be a nightmare to support from an > implementation point of view, hence the need for a generic way to describe > them. Okay, so we're back to the need to describe pipelines in DT. At the risk of sounding selfish: I don't care about pipelines. I just want us to settle on a way to represent a dumb panel in DT so that it can be enabled when it needs to. Furthermore I don't have any hardware that exhibits any of these "advanced" features, so I'm totally unqualified to work on any of this. Can we please try to be a little pragmatic here and solve one problem at a time? I am aware that solving this for panels may require some amount of foresight, but let's not try to solve everything at once at the risk of not getting anything done at all. > > > I'll still keep trying of course, but not for years.I have yet to > > > see someone proposing a viable solution to share drivers between DRM and > > > V4L2, which is a real pressing requirement, partly due to DT. > > > > Yeah... I still don't get that side of the argument. Why exactly do we need > > to share the drivers again? > > Think about a scaler IP in a SoC or FPGA, with one instance used in a camera > capture pipeline, supported by a V4L2 driver, and one instance used in a video > output pipeline, supported by a DRM driver. We want a single driver for that > IP core. Same story for video encoders. Yes, I see. This doesn't immediately concern the simple panel problem that I'm trying to solve, so perhaps we can have a separate discussion about it. Perhaps this would more easily be solved by providing some sort of helper library for the lower level control of the device and wrapping that with V4L2 or DRM APIs. > > If we really need to support display output within V4L2, shouldn't we > > instead improve interoperability between the two and use DRM/KMS exclusively > > for the output part? Then the problem of having to share drivers between > > subsystems goes away and we'll actually be using the right subsystem for the > > right job at hand. > > In the long term we definitely should improve interoperability between the > two. I'd go further than that, having one API for video capture and one API > for video output doesn't make much sense, except for historical reasons. Having two separate subsystems has worked out pretty well so far. In my opinion having strong boundaries between subsystems helps with design. > > Of course none of that is relevant to the topic of DT bindings, which is > > what we should probably be concentrating on. > > It actually is. Given that DT bindings must describe hardware, the scaler (or > encoder, or other entity) would use the same bindings regardless of the Linux > subsystem that will be involved. We thus need to make sure the bindings will > be usable on both sides. Perhaps a single driver could expose both interfaces? > > I'm not at all opposed to the idea of CDF or the problems it tries to > > address. I don't think it necessarily should be a separate framework for > > reasons explained earlier. But even if it were to end up in a separate > > framework there's absolutely nothing preventing us from adding a DRM panel > > driver that hooks into CDF as a "backend" of sorts for panels that require > > something more complex than the simple-panel binding. > > Once again it's not about panel having complex needs, but more about using > simple panels in complex pipelines. I'm fine with the drm_panel > infrastructure, what I would like is DT bindings that describe connections in > a more future-proof way. The rest is fine. And I've already said elsewhere that the bindings in their current form are easily extensible to cater for the needs of CDF. > > But that's precisely the point. Why would we need to go back from the panel > > to the display controller? Especially for dumb panels that can't or don't > > have to be configured in any way. Even if they needed some sort of setup, > > why can't that be done from the display controller/output. > > > > Even given a setup where a DSI controller needs to write some registers in a > > panel upon initialization, I don't see why the reverse connection needs to > > be described. The fact alone that an output dereferences a panel's phandle > > should be enough to connect both of them and have any panel driver use the > > DSI controller that it's been attached to for the programming. > > > > That's very much analogous to how I2C works. There are no connections back > > to the I2C master from the slaves. Yet each I2C client driver manages to use > > the services provided by the I2C master to perform transactions on the I2C > > bus. In a similar way the DSI controller is the bus master that talks to DSI > > panels. DSI panels don't actively talk to the DSI controller. > > It's all about modeling video pipeline graphs in DT. To be able to walk the > graph we need to describe connections. Not having bidirectional information > means that we restrict the points at which we can start walking the graph, > potentially making our life much more difficult in the future. > > What's so wrong about adding a port node and link information to the panel DT > node ? It describe what's there: the panel has one input, why not make that > explicit ? What's wrong with it is that there's no way to verify the soundness of the design by means of a full implementation because we're missing the majority of the pieces. Just putting the nodes into DT to provide some imaginary future-proofness isn't helpful either. Without any code that actually uses them they are useless. And again, why should we add them right away (while not needed) when they can easily be added in a backwards-compatible manner at some later point when there's actually any use for them and they can actually be tested in some larger framework? > > > > > > +static void panel_simple_enable(struct drm_panel *panel) > > > > > > +{ > > > > > > + struct panel_simple *p = to_panel_simple(panel); > > > > > > + int err; > > > > > > + > > > > > > + if (p->enabled) > > > > > > + return; > > > > > > + > > > > > > + err = regulator_enable(p->supply); > > > > > > + if (err < 0) > > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err); > > > > > > > > > > Is that really a non-fatal error ? Shouldn't the enable operation > > > > > return an int ? > > > > > > > > There's no way to propagate this in DRM, so why go through the trouble > > > > of returning the error? Furthermore, there's nothing that the caller > > > > could do to remedy the situation anyway. > > > > > > That's a DRM issue, which could be fixed. While the caller can't remedy > > > the situation, it should at least report the error to the application > > > instead of silently ignoring it. > > > > Perhaps. It's not really relevant to the discussion and can always be > > fixed in a subsequent patch. > > Most things can be fixed by a subsequent patch, that's not a good enough > reason not to address the known problems before pushing the code to mainline > (to clarify my point of view, there's no need to fix DRM to use the return > value before pushing this patch set to mainline, but I'd like a v2 with an int > return value). Why? What's the use of returning an error if you know up front that it can't be used? This should be changed if or when we "fix" DRM to propagate errors. > > > > > Instead of hardcoding the modes in the driver, which would then > > > > > require to be updated for every new simple panel model (and we know > > > > > there are lots of them), why don't you specify the modes in the panel > > > > > DT node ? The simple panel driver would then become much more generic. > > > > > It would also allow board designers to tweak h/v sync timings > > > > > depending on the system requirements. > > > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when power > > > > sequences were designed (and NAKed at the last minute), we all decided > > > > that the right thing to do would be to use specific compatible values > > > > for individual panels, because that would allow us to encode the power > > > > sequencing within the driver. And when we already have the panel model > > > > encoded in the compatible value, we might just as well encode the mode > > > > within the driver for that panel. Otherwise we'll have to keep adding > > > > the same mode timings for every board that uses the same panel. > > > > > > > > I do agree though that it might be useful to tweak the mode in case the > > > > default one doesn't work. How about we provide a means to override the > > > > mode encoded in the driver using one specified in the DT? That's > > > > obviously a backwards-compatible change, so it could be added if or when > > > > it becomes necessary. > > > > > > I share Tomi's point of view here. Your three panels use the same power > > > sequence, so I believe we should have a generic panel compatible string > > > that would use modes described in DT for the common case. Only if a panel > > > required something more complex which can't (or which could, but won't > > > for any reason) accurately be described in DT would you need to extend > > > the driver. > > > > I don't see the point quite frankly. You seem to be assuming that every > > panel will only be used in a single board. > > No, but in practice that's often the case, at least for boards with .dts files > in the mainline kernel. > > > However what you're proposing will require the mode timings to be repeated > > in every board's DT file, if the same panel is ever used on more than a > > single board. > > Is that a problem ? You could #include a .dtsi for the panel that would > specify the video mode if you want to avoid multiple copies. Yes, I don't think it's desirable to duplicate data needlessly in DT. It also makes the binding more difficult to use. If I know that the panel is one supported by the simple-panel binding, I can just go and add the right compatible value without having to bother looking up the mode timings and duplicating them. That way DT writers get to concern themselves only with the variable data. Thierry
On Thu, Oct 17, 2013 at 02:23:32PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Thursday 17 October 2013 14:06:47 Thierry Reding wrote: > > On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote: > > > On Thursday 17 October 2013 13:05:18 Thierry Reding wrote: > > > > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote: > > > > > On 17/10/13 11:53, Thierry Reding wrote: > > > > > > I keep wondering why we absolutely must have compatibility between > > > > > > CDF and this simple panel binding. DT content is supposed to concern > > > > > > itself with the description of hardware only. What about the > > > > > > following isn't an accurate description of the panel hardware? > > > > > > > > > > > > panel: panel { > > > > > > compatible = "cptt,claa101wb01"; > > > > > > > > > > > > power-supply = <&vdd_pnl_reg>; > > > > > > enable-gpios = <&gpio 90 0>; > > > > > > > > > > > > backlight = <&backlight>; > > > > > > }; > > > > > > > > > > > > dsi-controller { > > > > > > panel = <&panel>; > > > > > > }; > > > > > > > > > > That's quite similar to how my current out-of-mainline OMAP DSS DT > > > > > bindings work. The difference is that I have a reference from the > > > > > panel node to the video source (dsi-controller), instead of the other > > > > > way around. I just find it more natural. It works the same way as, > > > > > say, regulators, gpios, etc. > > > > > > > > I suppose that depends on the way you look at it. In the above proposal > > > > I consider the output (DSI controller) to use the panel as a resource > > > > that provides a certain set of services (query mode timings, provide a > > > > way to enable or disable the panel). Similarly the panel uses the > > > > backlight as a resource that provides a certain set of services (such as > > > > changing the brightness). > > > > > > > > The above also follows the natural order of control. The panel has no > > > > way to control the DSI output. However, it is the output that controls > > > > when a panel is required and turn it on as appropriate. > > > > > > I'm no DSI expert, but I know enough about it to be sure that Tomi will > > > disagree. DSI panels can have complex power sequences that require the > > > input stream to be finely controlled (for instance it might require a > > > clock without video frames for some time, switch a GPIO or regulator, > > > send a command to the panel, and then only get video frames). For that > > > reason all developers I've talked to who have an in-depth knowledge of > > > DSI and DSI panels told me that the panel needs to control the video bus, > > > and request the video source to start/stop the video stream. > > > > Oh, I'm very well aware of the various flavours of funkiness that DSI > > panels come in. But it's wrong to say that the panel needs to control > > the video bus. There's simply no way that a panel can actually do that. > > It is true, however, that in order to make this work in a maintainable > > fashion, the DSI panel *driver* may need to control the DSI bus. That's > > an entirely different story. > > Sure, but I don't think that's really related to the DT bindings. We don't > have to model every electrical signal in a detailed way that matches the > direction of the electrons flow :-) What we need to model is a connection > between a display controller and a panel (possibly with a direction). What I'd > like to do is to express that link in a way that can also express more complex > pipeline topologies. I don't want to make it overly complex, I had hoped that > my DT bindings proposal would be a good approach as it's both generic and > still pretty simple. I get that, and for what it's worth I do think that your proposal looks simple enough and if it can solve any of the problem you're facing with CDF, then that's great. But I don't think we should force inclusion of these properties on every panel, even if it doesn't use any of the graph functionality. Is there any problem with making them optional? Thierry
On 10/17/2013 02:32 PM, Tomi Valkeinen wrote: > On 17/10/13 15:17, Laurent Pinchart wrote: >> On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote: >>> On 17/10/13 14:51, Laurent Pinchart wrote: >>>>> I'm not sure if there's a specific need for the port or endpoint nodes >>>>> in cases like the above. Even if we have common properties describing >>>>> the endpoint, I guess they could just be in the parent node. >>>>> >>>>> panel { >>>>> remote =<&dc>; >>>>> common-video-property =<asd>; >>>>> }; >>>>> >>>>> The above would imply one port and one endpoint. Would that work? If we >>>>> had a function like parse_endpoint(node), we could just point it to >>>>> either a real endpoint node, or to the device's node. >>>> >>>> You reference the display controller here, not a specific display >>>> controller output. Don't most display controllers have several outputs ? >>> >>> Sure. Then the display controller could have more verbose description. >>> But the panel could still have what I wrote above, except the 'remote' >>> property would point to a real endpoint node inside the dispc node, not >>> to the dispc node. >>> >>> This would, of course, need some extra code to handle the different >>> cases, but just from DT point of view, I think all the relevant >>> information is there. >> >> There's many ways to describe the same information in DT. While we could have >> DT bindings that use different descriptions for different devices and still >> support all of them in our code, why should we opt for that option that will >> make the implementation much more complex when we can describe connections in >> a simple and generic way ? > > My suggestion was simple and generic. I'm not proposing per-device > custom bindings. > > My point was, if we can describe the connections as I described above, > which to me sounds possible, we can simplify the panel DT data for 99.9% > of the cases. > > To me, the first of these looks much nicer: > > panel { > remote =<&remote-endpoint>; > common-video-property =<asd>; > }; > > panel { > port { > endpoint { > remote =<&remote-endpoint>; > common-video-property =<asd>; > }; > }; > }; > > If that can be supported in the SW by adding complexity to a few > functions, and it covers practically all the panels, isn't it worth it? > > Note that I have not tried this, so I don't know if there are issues. > It's just a thought. Even if there's need for a endpoint node, perhaps > the port node can be made optional. Each endpoint node was originally supposed to contain the port configuration properties specific to a single remote device. So there would be more than one endpoint sub-mode of a port node only if, e.g. a display controller could output data through a single port to more than one panel (not necessarily simultaneously). If it is known in the display subsystems there is no use case where single data output (port) is being switched between (connected to) multiple data sinks we could probably omit the endpoint nodes in order to simplify the bindings a bit. We could have something like: dispc { disp_port: port { remote-port = <&p_port>; common-video-property = <>; }; }; panel { p_port: port { remote-port = <&disp_port>; common-video-property = <>; }; }; Alternatively we could say, that for only one endpoint node needed such an endpoint node can be omitted and the common video properties can be specified directly in the port node. This would require introducing 'remote-port' phandle property, as remote-endpoint is supposed to point to an endpoint, not a port node. Regarding the reverse link phandle, I was wondering if it shouldn't be made optional. We would have to think twice about such a change though, since making an optional property back mandatory wouldn't be possible. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/17/2013 09:49 AM, Tomi Valkeinen wrote: > Hi, > > On 17/10/13 11:28, Dave Airlie wrote: >>> >>> My biggest concern here is that this will not be compatible >>> with the CDF DT bindings. They're not complete yet, but they >>> will require connections between entities to be described in >>> DT, in a way very similar (or actually identical) to the V4L2 >>> DT bindings, documented in >>> Documentation/devicetree/bindings/media/video-interfaces.txt. >>> Could you have a look at that ? Please ignore all optional >>> properties beside remote-endpoint, as they're V4L2 specific. >>> >>> I also plan to specify video bus parameters in DT for CDF, but >>> this hasn't been finalized yet. >>> >> >> While I understand this, I don't see why CDF can't enhance these >> bindings if it has requirements > than they have without >> disturbing the panel ones, >> >> is DT really that inflexible? >> >> It seems that have a simple description for basic panels like >> Thierry wants to support, that can be enhanced for the other >> cases in the future should suffice, I really don't like blocking >> stuff that makes things work on the chance of something that >> isn't upstream yet, its sets a bad precedent, its also breaks the >> perfect is the enemy of good rule > > Just my opinion, but yes, DT is that inflexible. I don't really agree. A specific DT binding is supposed to be an ABI; something that can only be changed in a backwards-compatible way (perhaps only in a forwards-compatible way too). However, I see no reason you can't have a simple DT binding now, and create a more complex DT binding later. Both can exist (be defined). Now, because DT is an ABI, we can't remove kernel support for the old binding. > DT bindings are like a syscall. Once they are there, you need to > support them forever. That support can, of course, include some > kind of DT data converters or such, that try to convert old > bindings to new bindings at kernel boot. I don't think we need a converter for that; just leave in both the old and new parser/driver and treat them as different things. There's no reason that a new kernel must provide new features when given an old DT written to the old binding, and hence a new kernel can just treat the old DT with the old code. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/17/2013 12:07 PM, Laurent Pinchart wrote: ... >> As I said, anything that really needs a CDF binding to work >> likely isn't "simple" anymore, therefore a separate driver can >> easily be justified. > > The system as a whole would be more complex, but the panel could be > the same. We can't have two drivers for the same piece of hardware > in the DT world, as there will be a single compatible string and no > way to choose between the drivers (unlike the board code world that > could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > and live happily with that ever after). That's not true. We can certainly define two different compatible values for a piece of HW if we have to. We can easily control whether they are handled by the same or different drivers in the OS. Now, we should try to avoid this, because then that means that the original binding wasn't fully describing the HW. However, at least in the case of these simple LCD panels, it's hard to see that there is anything more than what's already in Thierry's binding. Remember, the binding is a description of the HW, not any Linux-internal details of how e.g. a CDF or DRM subsystem is supposed to use the HW; that had better be embodied into the driver or subsystem code, which aren't ABIs, and hence can be easily modified/enhanced. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomi, On Thursday 17 October 2013 15:32:29 Tomi Valkeinen wrote: > On 17/10/13 15:17, Laurent Pinchart wrote: > > On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote: > >> On 17/10/13 14:51, Laurent Pinchart wrote: > >>>> I'm not sure if there's a specific need for the port or endpoint nodes > >>>> in cases like the above. Even if we have common properties describing > >>>> the endpoint, I guess they could just be in the parent node. > >>>> > >>>> panel { > >>>> remote = <&dc>; > >>>> common-video-property = <asd>; > >>>> }; > >>>> > >>>> The above would imply one port and one endpoint. Would that work? If we > >>>> had a function like parse_endpoint(node), we could just point it to > >>>> either a real endpoint node, or to the device's node. > >>> > >>> You reference the display controller here, not a specific display > >>> controller output. Don't most display controllers have several outputs ? > >> > >> Sure. Then the display controller could have more verbose description. > >> But the panel could still have what I wrote above, except the 'remote' > >> property would point to a real endpoint node inside the dispc node, not > >> to the dispc node. > >> > >> This would, of course, need some extra code to handle the different > >> cases, but just from DT point of view, I think all the relevant > >> information is there. > > > > There's many ways to describe the same information in DT. While we could > > have DT bindings that use different descriptions for different devices > > and still support all of them in our code, why should we opt for that > > option that will make the implementation much more complex when we can > > describe connections in a simple and generic way ? > > My suggestion was simple and generic. I'm not proposing per-device > custom bindings. > > My point was, if we can describe the connections as I described above, > which to me sounds possible, we can simplify the panel DT data for 99.9% > of the cases. > > To me, the first of these looks much nicer: > > panel { > remote = <&remote-endpoint>; > common-video-property = <asd>; > }; > > panel { > port { > endpoint { > remote = <&remote-endpoint>; > common-video-property = <asd>; > }; > }; > }; Please note that the common video properties would be in the panel node, not in the endpoint node (unless you have specific requirements to do so, which isn't the common case). > If that can be supported in the SW by adding complexity to a few functions, > and it covers practically all the panels, isn't it worth it? > > Note that I have not tried this, so I don't know if there are issues. > It's just a thought. Even if there's need for a endpoint node, perhaps > the port node can be made optional. It can be worth it, as long as we make sure that simplified bindings cover the needs of the generic code. We could assume that, if the port subnode isn't present, the device will have a single port, with a single endpoint. However, isn't the number of endpoints a system property rather than a device property ? If a port of a device is connected to two remote ports it will require two endpoints. We could select the simplified or full bindings based on the system topology though. I've CC'ed Sylwester Nawrocki and Guennadi Liakhovetski, the V4L2 DT bindings authors, as well as the linux-media list, to get their opinion on this.
On 24/10/13 13:40, Laurent Pinchart wrote: >> panel { >> remote = <&remote-endpoint>; >> common-video-property = <asd>; >> }; >> >> panel { >> port { >> endpoint { >> remote = <&remote-endpoint>; >> common-video-property = <asd>; >> }; >> }; >> }; > > Please note that the common video properties would be in the panel node, not > in the endpoint node (unless you have specific requirements to do so, which > isn't the common case). Hmm, well, the panel driver must look for its properties either in the panel node, or in the endpoint node (I guess it could look them from both, but that doesn't sound good). If you write the panel driver, and in all your cases the properties work fine in the panel node, does that mean they'll work fine with everybody? I guess there are different kinds of properties. Something like a regulator is obviously property of the panel. But anything related to the video itself, like DPI's bus width, or perhaps even something like "orientation" if the panel supports such, could need to be in the endpoint node. But yes, I understand what you mean. With "common-video-property" I meant common properties like DPI bus width. >> If that can be supported in the SW by adding complexity to a few functions, >> and it covers practically all the panels, isn't it worth it? >> >> Note that I have not tried this, so I don't know if there are issues. >> It's just a thought. Even if there's need for a endpoint node, perhaps >> the port node can be made optional. > > It can be worth it, as long as we make sure that simplified bindings cover the > needs of the generic code. > > We could assume that, if the port subnode isn't present, the device will have > a single port, with a single endpoint. However, isn't the number of endpoints Right. > a system property rather than a device property ? If a port of a device is Yes. > connected to two remote ports it will require two endpoints. We could select > the simplified or full bindings based on the system topology though. The drivers should not know about simplified/normal bindings. They should use CDF DT helper functions to get the port and endpoint information. The helper functions would do the assuming. Tomi
Hi Thierry, On Thursday 17 October 2013 14:46:20 Thierry Reding wrote: > On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote: > > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote: > > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote: > > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: [snip] > > The point is that the video pipeline must be described in DT. Having a > > per-device way to represent connections would be a nightmare to support > > from an implementation point of view, hence the need for a generic way to > > describe them. > > Okay, so we're back to the need to describe pipelines in DT. At the risk > of sounding selfish: I don't care about pipelines. I just want us to > settle on a way to represent a dumb panel in DT so that it can be > enabled when it needs to. Furthermore I don't have any hardware that > exhibits any of these "advanced" features, so I'm totally unqualified to > work on any of this. > > Can we please try to be a little pragmatic here and solve one problem at > a time? I am aware that solving this for panels may require some amount > of foresight, but let's not try to solve everything at once at the risk > of not getting anything done at all. I won't force you to care about pipelines, and I don't think this is selfish at all :-) What I would like to ensure is that whatever bindings we come up with, they will not preclude us from adding pipeline support when needed (as I'm pretty sure it will be needed at some point). > > > > I'll still keep trying of course, but not for years.I have yet to > > > > see someone proposing a viable solution to share drivers between DRM > > > > and V4L2, which is a real pressing requirement, partly due to DT. > > > > > > Yeah... I still don't get that side of the argument. Why exactly do we > > > need to share the drivers again? > > > > Think about a scaler IP in a SoC or FPGA, with one instance used in a > > camera capture pipeline, supported by a V4L2 driver, and one instance > > used in a video output pipeline, supported by a DRM driver. We want a > > single driver for that IP core. Same story for video encoders. > > Yes, I see. This doesn't immediately concern the simple panel problem that > I'm trying to solve, so perhaps we can have a separate discussion about it. > Perhaps this would more easily be solved by providing some sort of helper > library for the lower level control of the device and wrapping that with > V4L2 or DRM APIs. That's more or less the idea of CDF ;-) > > > If we really need to support display output within V4L2, shouldn't we > > > instead improve interoperability between the two and use DRM/KMS > > > exclusively for the output part? Then the problem of having to share > > > drivers between subsystems goes away and we'll actually be using the > > > right subsystem for the right job at hand. > > > > In the long term we definitely should improve interoperability between the > > two. I'd go further than that, having one API for video capture and one > > API for video output doesn't make much sense, except for historical > > reasons. > > Having two separate subsystems has worked out pretty well so far. In my > opinion having strong boundaries between subsystems helps with design. I'm not saying we've made a mistake. A unified subsystem wouldn't have got it right in the first place either anyway. My point is that the boundary has collapsed on the hardware side, so I believe the software side will need to follow somehow. > > > Of course none of that is relevant to the topic of DT bindings, which is > > > what we should probably be concentrating on. > > > > It actually is. Given that DT bindings must describe hardware, the scaler > > (or encoder, or other entity) would use the same bindings regardless of > > the Linux subsystem that will be involved. We thus need to make sure the > > bindings will be usable on both sides. > > Perhaps a single driver could expose both interfaces? That's one idea that has been proposed. I believe it would be simpler to standardize a common API and have translation layers specific to V4L and KMS in the core, instead of pushing that translation to all panel/bridges/encoders/whatever drivers. > > > I'm not at all opposed to the idea of CDF or the problems it tries to > > > address. I don't think it necessarily should be a separate framework for > > > reasons explained earlier. But even if it were to end up in a separate > > > framework there's absolutely nothing preventing us from adding a DRM > > > panel driver that hooks into CDF as a "backend" of sorts for panels that > > > require something more complex than the simple-panel binding. > > > > Once again it's not about panel having complex needs, but more about using > > simple panels in complex pipelines. I'm fine with the drm_panel > > infrastructure, what I would like is DT bindings that describe connections > > in a more future-proof way. The rest is fine. > > And I've already said elsewhere that the bindings in their current form > are easily extensible to cater for the needs of CDF. The simple panel bindings do not include any connection information, so we could add that later when needed without having to deprecate, remove or repurpose existing properties. The simple panel driver would need to be extended, which isn't much of a problem (except that extending it with CDF support might require changes to the users of the simple panel driver, which I believe won't be happily accepted, but that's a different issue). My concern is also on the other side. In the patches you've sent the tegra driver uses a custom nvidia,panel property to reference the panel. That would of course not be CDF-compatible, but there's no way around that at the moment if we don't want to keep development of all ARM KMS drivers stalled for the next 6 months. It boils down to the question of whether DT should be a stable ABI, and I'm increasingly tempted to say that I don't care. I want to solve issues we have on the display side, the firmware interface isn't my main concern. > > > But that's precisely the point. Why would we need to go back from the > > > panel to the display controller? Especially for dumb panels that can't > > > or don't have to be configured in any way. Even if they needed some sort > > > of setup, why can't that be done from the display controller/output. > > > > > > Even given a setup where a DSI controller needs to write some registers > > > in a panel upon initialization, I don't see why the reverse connection > > > needs to be described. The fact alone that an output dereferences a > > > panel's phandle should be enough to connect both of them and have any > > > panel driver use the DSI controller that it's been attached to for the > > > programming. > > > > > > That's very much analogous to how I2C works. There are no connections > > > back to the I2C master from the slaves. Yet each I2C client driver > > > manages to use the services provided by the I2C master to perform > > > transactions on the I2C bus. In a similar way the DSI controller is the > > > bus master that talks to DSI panels. DSI panels don't actively talk to > > > the DSI controller. > > > > It's all about modeling video pipeline graphs in DT. To be able to walk > > the graph we need to describe connections. Not having bidirectional > > information means that we restrict the points at which we can start > > walking the graph, potentially making our life much more difficult in the > > future. > > > > What's so wrong about adding a port node and link information to the panel > > DT node ? It describe what's there: the panel has one input, why not make > > that explicit ? > > What's wrong with it is that there's no way to verify the soundness of > the design by means of a full implementation because we're missing the > majority of the pieces. Just putting the nodes into DT to provide some > imaginary future-proofness isn't helpful either. Without any code that > actually uses them they are useless. > > And again, why should we add them right away (while not needed) when > they can easily be added in a backwards-compatible manner at some later > point when there's actually any use for them and they can actually be > tested in some larger framework? It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to solve DT backward compatibility issue. However, as mentioned above, this shouldn't be a show stopper. I'm thus fine with the way the proposed bindings describe (or rather don't describe) the connection. However, I will then expect your support in the future to implement the "easy" extension of the bindings to support CDF. De we have a deal ? ;-) > > > > > > > +static void panel_simple_enable(struct drm_panel *panel) > > > > > > > +{ > > > > > > > + struct panel_simple *p = to_panel_simple(panel); > > > > > > > + int err; > > > > > > > + > > > > > > > + if (p->enabled) > > > > > > > + return; > > > > > > > + > > > > > > > + err = regulator_enable(p->supply); > > > > > > > + if (err < 0) > > > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err); > > > > > > > > > > > > Is that really a non-fatal error ? Shouldn't the enable operation > > > > > > return an int ? > > > > > > > > > > There's no way to propagate this in DRM, so why go through the > > > > > trouble of returning the error? Furthermore, there's nothing that > > > > > the caller could do to remedy the situation anyway. > > > > > > > > That's a DRM issue, which could be fixed. While the caller can't > > > > remedy the situation, it should at least report the error to the > > > > application instead of silently ignoring it. > > > > > > Perhaps. It's not really relevant to the discussion and can always be > > > fixed in a subsequent patch. > > > > Most things can be fixed by a subsequent patch, that's not a good enough > > reason not to address the known problems before pushing the code to > > mainline (to clarify my point of view, there's no need to fix DRM to use > > the return value before pushing this patch set to mainline, but I'd like > > a v2 with an int return value). > > Why? What's the use of returning an error if you know up front that it > can't be used? This should be changed if or when we "fix" DRM to propagate > errors. Because not doing so now will require us to change (potentially) lots of panel drivers at that time. It's much easier to have each panel driver developer implement the required code in his/her driver than having a single developer refactoring the code later and have to touch all drivers. If your concern is that the error paths won't be testable at the moment, you could easily already add a WARN_ON() to the caller to catch problems. > > > > > > Instead of hardcoding the modes in the driver, which would then > > > > > > require to be updated for every new simple panel model (and we > > > > > > know there are lots of them), why don't you specify the modes in > > > > > > the panel DT node ? The simple panel driver would then become much > > > > > > more generic. It would also allow board designers to tweak h/v > > > > > > sync timings depending on the system requirements. > > > > > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when > > > > > power sequences were designed (and NAKed at the last minute), we all > > > > > decided that the right thing to do would be to use specific > > > > > compatible values for individual panels, because that would allow us > > > > > to encode the power sequencing within the driver. And when we > > > > > already have the panel model encoded in the compatible value, we > > > > > might just as well encode the mode within the driver for that panel. > > > > > Otherwise we'll have to keep adding the same mode timings for every > > > > > board that uses the same panel. > > > > > > > > > > I do agree though that it might be useful to tweak the mode in case > > > > > the default one doesn't work. How about we provide a means to > > > > > override the mode encoded in the driver using one specified in the > > > > > DT? That's obviously a backwards-compatible change, so it could be > > > > > added if or when it becomes necessary. > > > > > > > > I share Tomi's point of view here. Your three panels use the same > > > > power sequence, so I believe we should have a generic panel compatible > > > > string that would use modes described in DT for the common case. Only > > > > if a panel required something more complex which can't (or which > > > > could, but won't for any reason) accurately be described in DT would > > > > you need to extend the driver. > > > > > > I don't see the point quite frankly. You seem to be assuming that every > > > panel will only be used in a single board. > > > > No, but in practice that's often the case, at least for boards with .dts > > files in the mainline kernel. > > > > > However what you're proposing will require the mode timings to be > > > repeated in every board's DT file, if the same panel is ever used on > > > more than a single board. > > > > Is that a problem ? You could #include a .dtsi for the panel that would > > specify the video mode if you want to avoid multiple copies. > > Yes, I don't think it's desirable to duplicate data needlessly in DT. It > also makes the binding more difficult to use. If I know that the panel > is one supported by the simple-panel binding, I can just go and add the > right compatible value without having to bother looking up the mode > timings and duplicating them. That way DT writers get to concern > themselves only with the variable data. I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this. As most panels will use standard timings, Hans proposed adding a timings property that would reference the standard timings the panel uses (this could be a string or integer defined in include/dt-bindings/...). In most case DT would just have a single property to select the timings, and only in more complex cases where the system designer wants to use custom timings would the timings need to be manually defined.
Hi Stephen, On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > ... > > >> As I said, anything that really needs a CDF binding to work > >> likely isn't "simple" anymore, therefore a separate driver can > >> easily be justified. > > > > The system as a whole would be more complex, but the panel could be > > the same. We can't have two drivers for the same piece of hardware > > in the DT world, as there will be a single compatible string and no > > way to choose between the drivers (unlike the board code world that > > could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > > and live happily with that ever after). > > That's not true. We can certainly define two different compatible > values for a piece of HW if we have to. We can easily control whether > they are handled by the same or different drivers in the OS. From an implementation point of view, sure. But from a conceptual point of view, that would make the DT bindings pretty Linux-specific, with a description of what the operating system should do instead of a description of what the hardware looks like. My understanding is that we've tried pretty hard in the past not to open that Pandora's box. The case I'm mostly concerned about would be two different compatibility strings to select whether the device should be handled by a KMS or V4L driver. I don't think that's a good idea. > Now, we should try to avoid this, because then that means that the > original binding wasn't fully describing the HW. However, at least in > the case of these simple LCD panels, it's hard to see that there is > anything more than what's already in Thierry's binding. Remember, the > binding is a description of the HW, not any Linux-internal details of > how e.g. a CDF or DRM subsystem is supposed to use the HW; that had > better be embodied into the driver or subsystem code, which aren't > ABIs, and hence can be easily modified/enhanced.
On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Thursday 17 October 2013 14:46:20 Thierry Reding wrote: > > On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote: > > > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote: > > > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote: > > > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: > > [snip] > > > > The point is that the video pipeline must be described in DT. Having a > > > per-device way to represent connections would be a nightmare to support > > > from an implementation point of view, hence the need for a generic way to > > > describe them. > > > > Okay, so we're back to the need to describe pipelines in DT. At the risk > > of sounding selfish: I don't care about pipelines. I just want us to > > settle on a way to represent a dumb panel in DT so that it can be > > enabled when it needs to. Furthermore I don't have any hardware that > > exhibits any of these "advanced" features, so I'm totally unqualified to > > work on any of this. > > > > Can we please try to be a little pragmatic here and solve one problem at > > a time? I am aware that solving this for panels may require some amount > > of foresight, but let's not try to solve everything at once at the risk > > of not getting anything done at all. > > I won't force you to care about pipelines, and I don't think this is selfish > at all :-) > > What I would like to ensure is that whatever bindings we come up with, they > will not preclude us from adding pipeline support when needed (as I'm pretty > sure it will be needed at some point). Sure. I think I've explained elsewhere that I consider this easily possible. Other people have said the same thing. If all we do is add properties to a binding, then drivers written for an old binding will have no problem continuing to work. I also think that we're mostly being too scared. If ever the next device comes around that requires explicit pipeline support to work, how likely is it to have the exact same display as prior devices? > > > > I'm not at all opposed to the idea of CDF or the problems it tries to > > > > address. I don't think it necessarily should be a separate framework for > > > > reasons explained earlier. But even if it were to end up in a separate > > > > framework there's absolutely nothing preventing us from adding a DRM > > > > panel driver that hooks into CDF as a "backend" of sorts for panels that > > > > require something more complex than the simple-panel binding. > > > > > > Once again it's not about panel having complex needs, but more about using > > > simple panels in complex pipelines. I'm fine with the drm_panel > > > infrastructure, what I would like is DT bindings that describe connections > > > in a more future-proof way. The rest is fine. > > > > And I've already said elsewhere that the bindings in their current form > > are easily extensible to cater for the needs of CDF. > > The simple panel bindings do not include any connection information, so we > could add that later when needed without having to deprecate, remove or > repurpose existing properties. Yes, I agree. > The simple panel driver would need to be extended, which isn't much of a > problem (except that extending it with CDF support might require changes > to the users of the simple panel driver, which I believe won't be happily > accepted, but that's a different issue). That might be true. But that's just the way kernel development works. We sometimes require major rework of APIs and we're actually pretty good at pulling that off, so I don't worry too much about it. Also, being the original author of the driver makes me the maintainer, and if it should prove to be necessary I'm absolutely willing to add CDF support. > My concern is also on the other side. In the patches you've sent the tegra > driver uses a custom nvidia,panel property to reference the panel. That would > of course not be CDF-compatible, but there's no way around that at the moment > if we don't want to keep development of all ARM KMS drivers stalled for the > next 6 months. Well, the nvidia,panel property is part of the display output and it is an optional property. So the driver already needs to cope with it being absent. If the display output driver is ever extended with CDF support then we can just go and use CDF if CDF-specific properties exist, or we fall back to nvidia,panel if that doesn't exist. > It boils down to the question of whether DT should be a stable ABI, and > I'm increasingly tempted to say that I don't care. I want to solve > issues we have on the display side, the firmware interface isn't my main > concern. I wholeheartedly agree. In my opinion it is much more useful to come up with a solution that works now, rather than discussing and arguing things to death and not get anything done. If that means that I'll have to maintain additional code, then so be it. > > > > But that's precisely the point. Why would we need to go back from the > > > > panel to the display controller? Especially for dumb panels that can't > > > > or don't have to be configured in any way. Even if they needed some sort > > > > of setup, why can't that be done from the display controller/output. > > > > > > > > Even given a setup where a DSI controller needs to write some registers > > > > in a panel upon initialization, I don't see why the reverse connection > > > > needs to be described. The fact alone that an output dereferences a > > > > panel's phandle should be enough to connect both of them and have any > > > > panel driver use the DSI controller that it's been attached to for the > > > > programming. > > > > > > > > That's very much analogous to how I2C works. There are no connections > > > > back to the I2C master from the slaves. Yet each I2C client driver > > > > manages to use the services provided by the I2C master to perform > > > > transactions on the I2C bus. In a similar way the DSI controller is the > > > > bus master that talks to DSI panels. DSI panels don't actively talk to > > > > the DSI controller. > > > > > > It's all about modeling video pipeline graphs in DT. To be able to walk > > > the graph we need to describe connections. Not having bidirectional > > > information means that we restrict the points at which we can start > > > walking the graph, potentially making our life much more difficult in the > > > future. > > > > > > What's so wrong about adding a port node and link information to the panel > > > DT node ? It describe what's there: the panel has one input, why not make > > > that explicit ? > > > > What's wrong with it is that there's no way to verify the soundness of > > the design by means of a full implementation because we're missing the > > majority of the pieces. Just putting the nodes into DT to provide some > > imaginary future-proofness isn't helpful either. Without any code that > > actually uses them they are useless. > > > > And again, why should we add them right away (while not needed) when > > they can easily be added in a backwards-compatible manner at some later > > point when there's actually any use for them and they can actually be > > tested in some larger framework? > > It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to > solve DT backward compatibility issue. However, as mentioned above, this > shouldn't be a show stopper. I'm thus fine with the way the proposed bindings > describe (or rather don't describe) the connection. However, I will then > expect your support in the future to implement the "easy" extension of the > bindings to support CDF. Again, I don't expect that to ever happen. But if it ever happens anyway, then... > Do we have a deal ? ;-) Yes, we do. > > > > > > > > +static void panel_simple_enable(struct drm_panel *panel) > > > > > > > > +{ > > > > > > > > + struct panel_simple *p = to_panel_simple(panel); > > > > > > > > + int err; > > > > > > > > + > > > > > > > > + if (p->enabled) > > > > > > > > + return; > > > > > > > > + > > > > > > > > + err = regulator_enable(p->supply); > > > > > > > > + if (err < 0) > > > > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", > err); > > > > > > > > > > > > > > Is that really a non-fatal error ? Shouldn't the enable operation > > > > > > > return an int ? > > > > > > > > > > > > There's no way to propagate this in DRM, so why go through the > > > > > > trouble of returning the error? Furthermore, there's nothing that > > > > > > the caller could do to remedy the situation anyway. > > > > > > > > > > That's a DRM issue, which could be fixed. While the caller can't > > > > > remedy the situation, it should at least report the error to the > > > > > application instead of silently ignoring it. > > > > > > > > Perhaps. It's not really relevant to the discussion and can always be > > > > fixed in a subsequent patch. > > > > > > Most things can be fixed by a subsequent patch, that's not a good enough > > > reason not to address the known problems before pushing the code to > > > mainline (to clarify my point of view, there's no need to fix DRM to use > > > the return value before pushing this patch set to mainline, but I'd like > > > a v2 with an int return value). > > > > Why? What's the use of returning an error if you know up front that it > > can't be used? This should be changed if or when we "fix" DRM to propagate > > errors. > > Because not doing so now will require us to change (potentially) lots of panel > drivers at that time. It's much easier to have each panel driver developer > implement the required code in his/her driver than having a single developer > refactoring the code later and have to touch all drivers. If your concern is > that the error paths won't be testable at the moment, you could easily already > add a WARN_ON() to the caller to catch problems. I don't mind fixing potentially many drivers. The conversion is pretty mechanical and therefore easy. We even have tools such as coccinelle to help with it. But we've probably spent more time arguing the point than it would take to make this simple change, so I'll just go and change the return value to an int and return an error. Perhaps if I'm in the mood I'll even write up a patch to propagate the error further. > > > > > > > Instead of hardcoding the modes in the driver, which would then > > > > > > > require to be updated for every new simple panel model (and we > > > > > > > know there are lots of them), why don't you specify the modes in > > > > > > > the panel DT node ? The simple panel driver would then become much > > > > > > > more generic. It would also allow board designers to tweak h/v > > > > > > > sync timings depending on the system requirements. > > > > > > > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when > > > > > > power sequences were designed (and NAKed at the last minute), we all > > > > > > decided that the right thing to do would be to use specific > > > > > > compatible values for individual panels, because that would allow us > > > > > > to encode the power sequencing within the driver. And when we > > > > > > already have the panel model encoded in the compatible value, we > > > > > > might just as well encode the mode within the driver for that panel. > > > > > > Otherwise we'll have to keep adding the same mode timings for every > > > > > > board that uses the same panel. > > > > > > > > > > > > I do agree though that it might be useful to tweak the mode in case > > > > > > the default one doesn't work. How about we provide a means to > > > > > > override the mode encoded in the driver using one specified in the > > > > > > DT? That's obviously a backwards-compatible change, so it could be > > > > > > added if or when it becomes necessary. > > > > > > > > > > I share Tomi's point of view here. Your three panels use the same > > > > > power sequence, so I believe we should have a generic panel compatible > > > > > string that would use modes described in DT for the common case. Only > > > > > if a panel required something more complex which can't (or which > > > > > could, but won't for any reason) accurately be described in DT would > > > > > you need to extend the driver. > > > > > > > > I don't see the point quite frankly. You seem to be assuming that every > > > > panel will only be used in a single board. > > > > > > No, but in practice that's often the case, at least for boards with .dts > > > files in the mainline kernel. > > > > > > > However what you're proposing will require the mode timings to be > > > > repeated in every board's DT file, if the same panel is ever used on > > > > more than a single board. > > > > > > Is that a problem ? You could #include a .dtsi for the panel that would > > > specify the video mode if you want to avoid multiple copies. > > > > Yes, I don't think it's desirable to duplicate data needlessly in DT. It > > also makes the binding more difficult to use. If I know that the panel > > is one supported by the simple-panel binding, I can just go and add the > > right compatible value without having to bother looking up the mode > > timings and duplicating them. That way DT writers get to concern > > themselves only with the variable data. > > I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this. > As most panels will use standard timings, Hans proposed adding a timings > property that would reference the standard timings the panel uses (this could > be a string or integer defined in include/dt-bindings/...). In most case DT > would just have a single property to select the timings, and only in more > complex cases where the system designer wants to use custom timings would the > timings need to be manually defined. We can do the same thing within the kernel. We already have a list of standard EDID/HDMI/CEA display modes, so we could similarly add a new list of common display panel modes and make each driver reference that instead of defining a custom one. And that still enables us to add a property that would allow DT writers to override the display mode if they need to. Thierry
On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > Hi Stephen, > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: >> ... >> >>>> As I said, anything that really needs a CDF binding to work >>>> likely isn't "simple" anymore, therefore a separate driver can >>>> easily be justified. >>> >>> The system as a whole would be more complex, but the panel could be >>> the same. We can't have two drivers for the same piece of hardware >>> in the DT world, as there will be a single compatible string and no >>> way to choose between the drivers (unlike the board code world that >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" >>> and live happily with that ever after). >> >> That's not true. We can certainly define two different compatible >> values for a piece of HW if we have to. We can easily control whether >> they are handled by the same or different drivers in the OS. > > From an implementation point of view, sure. But from a conceptual point of > view, that would make the DT bindings pretty Linux-specific, with a > description of what the operating system should do instead of a description of > what the hardware looks like. My understanding is that we've tried pretty hard > in the past not to open that Pandora's box. > > The case I'm mostly concerned about would be two different compatibility > strings to select whether the device should be handled by a KMS or V4L driver. > I don't think that's a good idea. I wouldn't think of the two compatible values as selecting different specific Linux drivers, but rather they simply describe the HW in different levels of detail. The fact that if we know a certain level of detail about the HW means that Linux can and does create a KMS driver rather than a V4L2 driver seems like a detail that's completely hidden inside the OS. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote: > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > Hi Stephen, > > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > >> ... > >> > >>>> As I said, anything that really needs a CDF binding to work > >>>> likely isn't "simple" anymore, therefore a separate driver can > >>>> easily be justified. > >>> > >>> The system as a whole would be more complex, but the panel could be > >>> the same. We can't have two drivers for the same piece of hardware > >>> in the DT world, as there will be a single compatible string and no > >>> way to choose between the drivers (unlike the board code world that > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > >>> and live happily with that ever after). > >> > >> That's not true. We can certainly define two different compatible > >> values for a piece of HW if we have to. We can easily control whether > >> they are handled by the same or different drivers in the OS. > > > > From an implementation point of view, sure. But from a conceptual point of > > view, that would make the DT bindings pretty Linux-specific, with a > > description of what the operating system should do instead of a description of > > what the hardware looks like. My understanding is that we've tried pretty hard > > in the past not to open that Pandora's box. > > > > The case I'm mostly concerned about would be two different compatibility > > strings to select whether the device should be handled by a KMS or V4L driver. > > I don't think that's a good idea. > > I wouldn't think of the two compatible values as selecting different > specific Linux drivers, but rather they simply describe the HW in > different levels of detail. The fact that if we know a certain level of > detail about the HW means that Linux can and does create a KMS driver > rather than a V4L2 driver seems like a detail that's completely hidden > inside the OS. I've had a somewhat similar idea the other day but couldn't really put it into words. Interestingly someone else mentioned a similar concept in a different thread which I think describes what I had in mind as well. I was wondering if we couldn't use two compatible values to denote two interfaces that the device implements. Something along the lines of: compatible = "vendor,block-name", "encoder"; So a driver could primarily match on "vendor,block-name", but at the same time it could use the additional information of being required to implement "encoder" to expose an additonal interface. I suppose that perhaps something like a device_type property could be used for that as well, and that might even be the more correct thing to do. We already do something similar to make GPIO controllers expose an interrupt chip by adding an interrupt-controller property. We also use the gpio-controller property to mark a device node as exposing the GPIO interface for that matter. So if a HW block can actually implement two different interfaces, each of them being optional, then there should be ways to represent that in DT as well. We already do that for "simpler" HW blocks, so there's no reason we shouldn't be able to do the same with multimedia components. If it's really an encoder, though, the problem might be different, though, since the interface (at a hardware or functional level if you will) remains the same. But I think in that case it's something that needs to be figured out internally by the OS. In my opinion, if we are in a situation where we have two different drivers in two subsystems for the same device, then we're doing something wrong and it should be fixed at that level, not by quirking the DT into making a decision for us. Thierry
On 10/24/2013 12:52 PM, Tomi Valkeinen wrote: > On 24/10/13 13:40, Laurent Pinchart wrote: > >>> panel { >>> remote =<&remote-endpoint>; >>> common-video-property =<asd>; >>> }; >>> >>> panel { >>> port { >>> endpoint { >>> remote =<&remote-endpoint>; >>> common-video-property =<asd>; >>> }; >>> }; >>> }; >> >> Please note that the common video properties would be in the panel node, not >> in the endpoint node (unless you have specific requirements to do so, which >> isn't the common case). > > Hmm, well, the panel driver must look for its properties either in the > panel node, or in the endpoint node (I guess it could look them from > both, but that doesn't sound good). Presumably the OS could be searching for port node and any endpoint node inside it first. If that's not found then it could be parsing the panel node. Please note that a port node may be required even if there is only one port, when there are multiple physical bus interfaces, e.g. at an LCD controller and only one of them is used. The reg property would select the physical bus interface. I wonder if a property like #video-port or #video-endpoint could be used to indicate that a node contains video bus properties. Probably it's too late to introduce it now and make it a required property for the endpoint nodes or nodes containing the common video properties. > If you write the panel driver, and in all your cases the properties work > fine in the panel node, does that mean they'll work fine with everybody? It's likely not safe to assume so. In V4L data bus properties are specified a both the receiver and the transmitter endpoint nodes separately. > I guess there are different kinds of properties. Something like a > regulator is obviously property of the panel. But anything related to > the video itself, like DPI's bus width, or perhaps even something like > "orientation" if the panel supports such, could need to be in the > endpoint node. If we use port/endpoint nodes it all seems clear, the video bus properties are put in an endpoint node. But since we are considering a simplified binding all the properties would be placed in the panel or display controller node. > But yes, I understand what you mean. With "common-video-property" I > meant common properties like DPI bus width. > >>> If that can be supported in the SW by adding complexity to a few functions, >>> and it covers practically all the panels, isn't it worth it? >>> >>> Note that I have not tried this, so I don't know if there are issues. >>> It's just a thought. Even if there's need for a endpoint node, perhaps >>> the port node can be made optional. >> >> It can be worth it, as long as we make sure that simplified bindings cover the >> needs of the generic code. >> >> We could assume that, if the port subnode isn't present, the device will have >> a single port, with a single endpoint. However, isn't the number of endpoints > > Right. > >> a system property rather than a device property ? If a port of a device is > > Yes. > >> connected to two remote ports it will require two endpoints. We could select >> the simplified or full bindings based on the system topology though. Yes, I guess it's all about the system topology. Any simplified binding would work only for very simple configuration like single-output LCD controller with single panel attached to it. > The drivers should not know about simplified/normal bindings. They > should use CDF DT helper functions to get the port and endpoint > information. The helper functions would do the assuming. Yes, anyway all the parsing is supposed to be done within the helpers. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thierry, On Thursday 24 October 2013 13:48:24 Thierry Reding wrote: > On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote: > > On Thursday 17 October 2013 14:46:20 Thierry Reding wrote: > > > On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote: > > > > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote: > > > > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote: > > > > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote: > > [snip] > > > > > > The point is that the video pipeline must be described in DT. Having a > > > > per-device way to represent connections would be a nightmare to > > > > support from an implementation point of view, hence the need for a > > > > generic way to describe them. > > > > > > Okay, so we're back to the need to describe pipelines in DT. At the risk > > > of sounding selfish: I don't care about pipelines. I just want us to > > > settle on a way to represent a dumb panel in DT so that it can be > > > enabled when it needs to. Furthermore I don't have any hardware that > > > exhibits any of these "advanced" features, so I'm totally unqualified to > > > work on any of this. > > > > > > Can we please try to be a little pragmatic here and solve one problem at > > > a time? I am aware that solving this for panels may require some amount > > > of foresight, but let's not try to solve everything at once at the risk > > > of not getting anything done at all. > > > > I won't force you to care about pipelines, and I don't think this is > > selfish at all :-) > > > > What I would like to ensure is that whatever bindings we come up with, > > they will not preclude us from adding pipeline support when needed (as I'm > > pretty sure it will be needed at some point). > > Sure. I think I've explained elsewhere that I consider this easily > possible. Other people have said the same thing. If all we do is add > properties to a binding, then drivers written for an old binding will > have no problem continuing to work. > > I also think that we're mostly being too scared. If ever the next device > comes around that requires explicit pipeline support to work, how likely > is it to have the exact same display as prior devices? The exact same panel, probably low, but a panel supported by the same driver using the same DT bindings (such as the generic simple panel driver), quite high. > > > > > I'm not at all opposed to the idea of CDF or the problems it tries > > > > > to address. I don't think it necessarily should be a separate > > > > > framework for reasons explained earlier. But even if it were to end > > > > > up in a separate framework there's absolutely nothing preventing us > > > > > from adding a DRM panel driver that hooks into CDF as a "backend" of > > > > > sorts for panels that require something more complex than the > > > > > simple-panel binding. > > > > > > > > Once again it's not about panel having complex needs, but more about > > > > using simple panels in complex pipelines. I'm fine with the drm_panel > > > > infrastructure, what I would like is DT bindings that describe > > > > connections in a more future-proof way. The rest is fine. > > > > > > And I've already said elsewhere that the bindings in their current form > > > are easily extensible to cater for the needs of CDF. > > > > The simple panel bindings do not include any connection information, so we > > could add that later when needed without having to deprecate, remove or > > repurpose existing properties. > > Yes, I agree. > > > The simple panel driver would need to be extended, which isn't much of a > > problem (except that extending it with CDF support might require changes > > to the users of the simple panel driver, which I believe won't be happily > > accepted, but that's a different issue). > > That might be true. But that's just the way kernel development works. We > sometimes require major rework of APIs and we're actually pretty good at > pulling that off, so I don't worry too much about it. > > Also, being the original author of the driver makes me the maintainer, > and if it should prove to be necessary I'm absolutely willing to add CDF > support. Much appreciated :-) > > My concern is also on the other side. In the patches you've sent the tegra > > driver uses a custom nvidia,panel property to reference the panel. That > > would of course not be CDF-compatible, but there's no way around that at > > the moment if we don't want to keep development of all ARM KMS drivers > > stalled for the next 6 months. > > Well, the nvidia,panel property is part of the display output and it is > an optional property. So the driver already needs to cope with it being > absent. If the display output driver is ever extended with CDF support > then we can just go and use CDF if CDF-specific properties exist, or we > fall back to nvidia,panel if that doesn't exist. > > > It boils down to the question of whether DT should be a stable ABI, and > > I'm increasingly tempted to say that I don't care. I want to solve > > issues we have on the display side, the firmware interface isn't my main > > concern. > > I wholeheartedly agree. In my opinion it is much more useful to come up > with a solution that works now, rather than discussing and arguing > things to death and not get anything done. If that means that I'll have > to maintain additional code, then so be it. > > > > > > But that's precisely the point. Why would we need to go back from > > > > > the panel to the display controller? Especially for dumb panels that > > > > > can't or don't have to be configured in any way. Even if they needed > > > > > some sort of setup, why can't that be done from the display > > > > > controller/output. > > > > > > > > > > Even given a setup where a DSI controller needs to write some > > > > > registers in a panel upon initialization, I don't see why the > > > > > reverse connection needs to be described. The fact alone that an > > > > > output dereferences a panel's phandle should be enough to connect > > > > > both of them and have any panel driver use the DSI controller that > > > > > it's been attached to for the programming. > > > > > > > > > > That's very much analogous to how I2C works. There are no > > > > > connections back to the I2C master from the slaves. Yet each I2C > > > > > client driver manages to use the services provided by the I2C master > > > > > to perform transactions on the I2C bus. In a similar way the DSI > > > > > controller is the bus master that talks to DSI panels. DSI panels > > > > > don't actively talk to the DSI controller. > > > > > > > > It's all about modeling video pipeline graphs in DT. To be able to > > > > walk the graph we need to describe connections. Not having > > > > bidirectional information means that we restrict the points at which > > > > we can start walking the graph, potentially making our life much more > > > > difficult in the future. > > > > > > > > What's so wrong about adding a port node and link information to the > > > > panel DT node ? It describe what's there: the panel has one input, why > > > > not make that explicit ? > > > > > > What's wrong with it is that there's no way to verify the soundness of > > > the design by means of a full implementation because we're missing the > > > majority of the pieces. Just putting the nodes into DT to provide some > > > imaginary future-proofness isn't helpful either. Without any code that > > > actually uses them they are useless. > > > > > > And again, why should we add them right away (while not needed) when > > > they can easily be added in a backwards-compatible manner at some later > > > point when there's actually any use for them and they can actually be > > > tested in some larger framework? > > > > It's the "easily" part I'm not sure about. I doubt we'll ever have any > > easy to solve DT backward compatibility issue. However, as mentioned > > above, this shouldn't be a show stopper. I'm thus fine with the way the > > proposed bindings describe (or rather don't describe) the connection. > > However, I will then expect your support in the future to implement the > > "easy" extension of the bindings to support CDF. > > Again, I don't expect that to ever happen. But if it ever happens anyway, > then... > > > Do we have a deal ? ;-) > > Yes, we do. Great! > > > > > > > > > +static void panel_simple_enable(struct drm_panel *panel) > > > > > > > > > +{ > > > > > > > > > + struct panel_simple *p = to_panel_simple(panel); > > > > > > > > > + int err; > > > > > > > > > + > > > > > > > > > + if (p->enabled) > > > > > > > > > + return; > > > > > > > > > + > > > > > > > > > + err = regulator_enable(p->supply); > > > > > > > > > + if (err < 0) > > > > > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", > > > > err); > > > > > > > > > > Is that really a non-fatal error ? Shouldn't the enable > > > > > > > > operation return an int ? > > > > > > > > > > > > > > There's no way to propagate this in DRM, so why go through the > > > > > > > trouble of returning the error? Furthermore, there's nothing > > > > > > > that the caller could do to remedy the situation anyway. > > > > > > > > > > > > That's a DRM issue, which could be fixed. While the caller can't > > > > > > remedy the situation, it should at least report the error to the > > > > > > application instead of silently ignoring it. > > > > > > > > > > Perhaps. It's not really relevant to the discussion and can always > > > > > be fixed in a subsequent patch. > > > > > > > > Most things can be fixed by a subsequent patch, that's not a good > > > > enough reason not to address the known problems before pushing the > > > > code to mainline (to clarify my point of view, there's no need to fix > > > > DRM to use the return value before pushing this patch set to mainline, > > > > but I'd like a v2 with an int return value). > > > > > > Why? What's the use of returning an error if you know up front that it > > > can't be used? This should be changed if or when we "fix" DRM to > > > propagate errors. > > > > Because not doing so now will require us to change (potentially) lots of > > panel drivers at that time. It's much easier to have each panel driver > > developer implement the required code in his/her driver than having a > > single developer refactoring the code later and have to touch all > > drivers. If your concern is that the error paths won't be testable at the > > moment, you could easily already add a WARN_ON() to the caller to catch > > problems. > > I don't mind fixing potentially many drivers. The conversion is pretty > mechanical and therefore easy. We even have tools such as coccinelle to > help with it. But we've probably spent more time arguing the point than > it would take to make this simple change, so I'll just go and change the > return value to an int and return an error. Thank you. > Perhaps if I'm in the mood I'll even write up a patch to propagate the > error further. > > > > > > > > > Instead of hardcoding the modes in the driver, which would > > > > > > > > then require to be updated for every new simple panel model > > > > > > > > (and we know there are lots of them), why don't you specify > > > > > > > > the modes in the panel DT node ? The simple panel driver would > > > > > > > > then become much more generic. It would also allow board > > > > > > > > designers to tweak h/v sync timings depending on the system > > > > > > > > requirements. > > > > > > > > > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when > > > > > > > power sequences were designed (and NAKed at the last minute), we > > > > > > > all decided that the right thing to do would be to use specific > > > > > > > compatible values for individual panels, because that would > > > > > > > allow us to encode the power sequencing within the driver. And > > > > > > > when we already have the panel model encoded in the compatible > > > > > > > value, we might just as well encode the mode within the driver > > > > > > > for that panel. Otherwise we'll have to keep adding the same > > > > > > > mode timings for every board that uses the same panel. > > > > > > > > > > > > > > I do agree though that it might be useful to tweak the mode in > > > > > > > case the default one doesn't work. How about we provide a means > > > > > > > to override the mode encoded in the driver using one specified > > > > > > > in the DT? That's obviously a backwards-compatible change, so it > > > > > > > could be added if or when it becomes necessary. > > > > > > > > > > > > I share Tomi's point of view here. Your three panels use the same > > > > > > power sequence, so I believe we should have a generic panel > > > > > > compatible string that would use modes described in DT for the > > > > > > common case. Only if a panel required something more complex which > > > > > > can't (or which could, but won't for any reason) accurately be > > > > > > described in DT would you need to extend the driver. > > > > > > > > > > I don't see the point quite frankly. You seem to be assuming that > > > > > every panel will only be used in a single board. > > > > > > > > No, but in practice that's often the case, at least for boards with > > > > .dts files in the mainline kernel. > > > > > > > > > However what you're proposing will require the mode timings to be > > > > > repeated in every board's DT file, if the same panel is ever used on > > > > > more than a single board. > > > > > > > > Is that a problem ? You could #include a .dtsi for the panel that > > > > would specify the video mode if you want to avoid multiple copies. > > > > > > Yes, I don't think it's desirable to duplicate data needlessly in DT. It > > > also makes the binding more difficult to use. If I know that the panel > > > is one supported by the simple-panel binding, I can just go and add the > > > right compatible value without having to bother looking up the mode > > > timings and duplicating them. That way DT writers get to concern > > > themselves only with the variable data. > > > > I've had a quick chat with Dave Airlie and Hans de Goede yesterday about > > this. As most panels will use standard timings, Hans proposed adding a > > timings property that would reference the standard timings the panel uses > > (this could be a string or integer defined in include/dt-bindings/...). > > In most case DT would just have a single property to select the timings, > > and only in more complex cases where the system designer wants to use > > custom timings would the timings need to be manually defined. > > We can do the same thing within the kernel. We already have a list of > standard EDID/HDMI/CEA display modes, so we could similarly add a new > list of common display panel modes and make each driver reference that > instead of defining a custom one. Sure. My point is that I would like to avoid adding zillions of compatible properties to the driver, when we could use a single property in the DT bindings that would specify the timings instead. This would lower the amount of changes made to the simple panel driver, while keeping DT simple enough (at least in my opinion). > And that still enables us to add a property that would allow DT writers > to override the display mode if they need to.
Hi Stephen, On Thursday 24 October 2013 23:06:18 Stephen Warren wrote: > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > >> ... > >> > >>>> As I said, anything that really needs a CDF binding to work > >>>> likely isn't "simple" anymore, therefore a separate driver can > >>>> easily be justified. > >>> > >>> The system as a whole would be more complex, but the panel could be > >>> the same. We can't have two drivers for the same piece of hardware > >>> in the DT world, as there will be a single compatible string and no > >>> way to choose between the drivers (unlike the board code world that > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > >>> and live happily with that ever after). > >> > >> That's not true. We can certainly define two different compatible > >> values for a piece of HW if we have to. We can easily control whether > >> they are handled by the same or different drivers in the OS. > > > > From an implementation point of view, sure. But from a conceptual point of > > view, that would make the DT bindings pretty Linux-specific, with a > > description of what the operating system should do instead of a > > description of what the hardware looks like. My understanding is that > > we've tried pretty hard in the past not to open that Pandora's box. > > > > The case I'm mostly concerned about would be two different compatibility > > strings to select whether the device should be handled by a KMS or V4L > > driver. I don't think that's a good idea. > > I wouldn't think of the two compatible values as selecting different > specific Linux drivers, but rather they simply describe the HW in > different levels of detail. The fact that if we know a certain level of > detail about the HW means that Linux can and does create a KMS driver > rather than a V4L2 driver seems like a detail that's completely hidden > inside the OS. I expect the same level of details to be needed on both the KMS and V4L sides. Taking the example of the ADV7511 HDMI transmitter, the only change in the DT bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l" and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and -kms to different names wouldn't fundamentally change the problem.
On Fri, Oct 25, 2013 at 01:33:11PM +0200, Laurent Pinchart wrote: > Hi Stephen, > > On Thursday 24 October 2013 23:06:18 Stephen Warren wrote: > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > > >> ... > > >> > > >>>> As I said, anything that really needs a CDF binding to work > > >>>> likely isn't "simple" anymore, therefore a separate driver can > > >>>> easily be justified. > > >>> > > >>> The system as a whole would be more complex, but the panel could be > > >>> the same. We can't have two drivers for the same piece of hardware > > >>> in the DT world, as there will be a single compatible string and no > > >>> way to choose between the drivers (unlike the board code world that > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > > >>> and live happily with that ever after). > > >> > > >> That's not true. We can certainly define two different compatible > > >> values for a piece of HW if we have to. We can easily control whether > > >> they are handled by the same or different drivers in the OS. > > > > > > From an implementation point of view, sure. But from a conceptual point of > > > view, that would make the DT bindings pretty Linux-specific, with a > > > description of what the operating system should do instead of a > > > description of what the hardware looks like. My understanding is that > > > we've tried pretty hard in the past not to open that Pandora's box. > > > > > > The case I'm mostly concerned about would be two different compatibility > > > strings to select whether the device should be handled by a KMS or V4L > > > driver. I don't think that's a good idea. > > > > I wouldn't think of the two compatible values as selecting different > > specific Linux drivers, but rather they simply describe the HW in > > different levels of detail. The fact that if we know a certain level of > > detail about the HW means that Linux can and does create a KMS driver > > rather than a V4L2 driver seems like a detail that's completely hidden > > inside the OS. > > I expect the same level of details to be needed on both the KMS and V4L sides. > Taking the example of the ADV7511 HDMI transmitter, the only change in the DT > bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l" > and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and > -kms to different names wouldn't fundamentally change the problem. I think that we're doing something fundamentally wrong if we use the same device (with the same functionality) in two different subsystems. If the device is used to display something, shouldn't we be moving the driver to DRM? Thierry
Hi Thierry, On Friday 25 October 2013 10:13:15 Thierry Reding wrote: > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote: > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > > >> ... > > >> > > >>>> As I said, anything that really needs a CDF binding to work > > >>>> likely isn't "simple" anymore, therefore a separate driver can > > >>>> easily be justified. > > >>> > > >>> The system as a whole would be more complex, but the panel could be > > >>> the same. We can't have two drivers for the same piece of hardware > > >>> in the DT world, as there will be a single compatible string and no > > >>> way to choose between the drivers (unlike the board code world that > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > > >>> and live happily with that ever after). > > >> > > >> That's not true. We can certainly define two different compatible > > >> values for a piece of HW if we have to. We can easily control whether > > >> they are handled by the same or different drivers in the OS. > > > > > > From an implementation point of view, sure. But from a conceptual point > > > of view, that would make the DT bindings pretty Linux-specific, with a > > > description of what the operating system should do instead of a > > > description of what the hardware looks like. My understanding is that > > > we've tried pretty hard in the past not to open that Pandora's box. > > > > > > The case I'm mostly concerned about would be two different compatibility > > > strings to select whether the device should be handled by a KMS or V4L > > > driver. I don't think that's a good idea. > > > > I wouldn't think of the two compatible values as selecting different > > specific Linux drivers, but rather they simply describe the HW in > > different levels of detail. The fact that if we know a certain level of > > detail about the HW means that Linux can and does create a KMS driver > > rather than a V4L2 driver seems like a detail that's completely hidden > > inside the OS. > > I've had a somewhat similar idea the other day but couldn't really put > it into words. Interestingly someone else mentioned a similar concept in > a different thread which I think describes what I had in mind as well. > > I was wondering if we couldn't use two compatible values to denote two > interfaces that the device implements. Something along the lines of: > > compatible = "vendor,block-name", "encoder"; > > So a driver could primarily match on "vendor,block-name", but at the > same time it could use the additional information of being required to > implement "encoder" to expose an additonal interface. Let's take the hardware architecture described in http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) as an example. The green blocks are part of the capture pipeline and handled by the V4L subsystem. The blue blocks are part of the display pipeline and handled by the KMS subsystem. One ADV7511 HDMI transmitter instance need to be controlled by a KMS driver and the second one by a V4L driver. The two instances are identical, so their DT nodes will show no difference if we stick to a hardware description only. There would then be no way to bind to different drivers. > I suppose that perhaps something like a device_type property could be > used for that as well, and that might even be the more correct thing to > do. > > We already do something similar to make GPIO controllers expose an > interrupt chip by adding an interrupt-controller property. We also use > the gpio-controller property to mark a device node as exposing the GPIO > interface for that matter. > > So if a HW block can actually implement two different interfaces, each > of them being optional, then there should be ways to represent that in > DT as well. We already do that for "simpler" HW blocks, so there's no > reason we shouldn't be able to do the same with multimedia components. > > If it's really an encoder, though, the problem might be different, > though, since the interface (at a hardware or functional level if you > will) remains the same. But I think in that case it's something that > needs to be figured out internally by the OS. In my opinion, if we are > in a situation where we have two different drivers in two subsystems for > the same device, then we're doing something wrong and it should be fixed > at that level, not by quirking the DT into making a decision for us. I tend to agree, and I'd like to be able to share drivers between V4L and KMS. This is way down the road though.
Hi Thierry, On Friday 25 October 2013 14:29:26 Thierry Reding wrote: > On Fri, Oct 25, 2013 at 01:33:11PM +0200, Laurent Pinchart wrote: > > On Thursday 24 October 2013 23:06:18 Stephen Warren wrote: > > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > > > >> ... > > > >> > > > >>>> As I said, anything that really needs a CDF binding to work > > > >>>> likely isn't "simple" anymore, therefore a separate driver can > > > >>>> easily be justified. > > > >>> > > > >>> The system as a whole would be more complex, but the panel could be > > > >>> the same. We can't have two drivers for the same piece of hardware > > > >>> in the DT world, as there will be a single compatible string and no > > > >>> way to choose between the drivers (unlike the board code world that > > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > > > >>> and live happily with that ever after). > > > >> > > > >> That's not true. We can certainly define two different compatible > > > >> values for a piece of HW if we have to. We can easily control whether > > > >> they are handled by the same or different drivers in the OS. > > > > > > > > From an implementation point of view, sure. But from a conceptual > > > > point of view, that would make the DT bindings pretty Linux-specific, > > > > with a description of what the operating system should do instead of a > > > > description of what the hardware looks like. My understanding is that > > > > we've tried pretty hard in the past not to open that Pandora's box. > > > > > > > > The case I'm mostly concerned about would be two different > > > > compatibility strings to select whether the device should be handled > > > > by a KMS or V4L driver. I don't think that's a good idea. > > > > > > I wouldn't think of the two compatible values as selecting different > > > specific Linux drivers, but rather they simply describe the HW in > > > different levels of detail. The fact that if we know a certain level of > > > detail about the HW means that Linux can and does create a KMS driver > > > rather than a V4L2 driver seems like a detail that's completely hidden > > > inside the OS. > > > > I expect the same level of details to be needed on both the KMS and V4L > > sides. Taking the example of the ADV7511 HDMI transmitter, the only > > change in the DT bindings between KMS and V4L would be the compatible > > string. "adi,adv7511-v4l" and "adi,adv7511-kms" is an option that I don't > > really like. Renaming -v4l and -kms to different names wouldn't > > fundamentally change the problem. > > I think that we're doing something fundamentally wrong if we use the > same device (with the same functionality) in two different subsystems. > If the device is used to display something, shouldn't we be moving the > driver to DRM? Let's discuss this in reply to the e-mail from this thread in which I mention the ADV7511 example.
On Fri, Oct 25, 2013 at 03:47:21PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Friday 25 October 2013 10:13:15 Thierry Reding wrote: > > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote: > > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > > > >> ... > > > >> > > > >>>> As I said, anything that really needs a CDF binding to work > > > >>>> likely isn't "simple" anymore, therefore a separate driver can > > > >>>> easily be justified. > > > >>> > > > >>> The system as a whole would be more complex, but the panel could be > > > >>> the same. We can't have two drivers for the same piece of hardware > > > >>> in the DT world, as there will be a single compatible string and no > > > >>> way to choose between the drivers (unlike the board code world that > > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm" > > > >>> and live happily with that ever after). > > > >> > > > >> That's not true. We can certainly define two different compatible > > > >> values for a piece of HW if we have to. We can easily control whether > > > >> they are handled by the same or different drivers in the OS. > > > > > > > > From an implementation point of view, sure. But from a conceptual point > > > > of view, that would make the DT bindings pretty Linux-specific, with a > > > > description of what the operating system should do instead of a > > > > description of what the hardware looks like. My understanding is that > > > > we've tried pretty hard in the past not to open that Pandora's box. > > > > > > > > The case I'm mostly concerned about would be two different compatibility > > > > strings to select whether the device should be handled by a KMS or V4L > > > > driver. I don't think that's a good idea. > > > > > > I wouldn't think of the two compatible values as selecting different > > > specific Linux drivers, but rather they simply describe the HW in > > > different levels of detail. The fact that if we know a certain level of > > > detail about the HW means that Linux can and does create a KMS driver > > > rather than a V4L2 driver seems like a detail that's completely hidden > > > inside the OS. > > > > I've had a somewhat similar idea the other day but couldn't really put > > it into words. Interestingly someone else mentioned a similar concept in > > a different thread which I think describes what I had in mind as well. > > > > I was wondering if we couldn't use two compatible values to denote two > > interfaces that the device implements. Something along the lines of: > > > > compatible = "vendor,block-name", "encoder"; > > > > So a driver could primarily match on "vendor,block-name", but at the > > same time it could use the additional information of being required to > > implement "encoder" to expose an additonal interface. > > Let's take the hardware architecture described in > http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) as > an example. The green blocks are part of the capture pipeline and handled by > the V4L subsystem. The blue blocks are part of the display pipeline and > handled by the KMS subsystem. One ADV7511 HDMI transmitter instance need to be > controlled by a KMS driver and the second one by a V4L driver. > > The two instances are identical, so their DT nodes will show no difference if > we stick to a hardware description only. There would then be no way to bind to > different drivers. I don't quite see why some of the green parts couldn't be part of the display (KMS) pipeline. I thought I remember you say something about implementing deep pipelining support in one of the more recent talks. This sounds exactly like a case where this would be useful. Obviously as long as that work isn't finished that won't help you, but I think that providing a way to pass a video stream object from V4L2 to DRM/KMS would be a more proper solution to this problem. > > I suppose that perhaps something like a device_type property could be > > used for that as well, and that might even be the more correct thing to > > do. > > > > We already do something similar to make GPIO controllers expose an > > interrupt chip by adding an interrupt-controller property. We also use > > the gpio-controller property to mark a device node as exposing the GPIO > > interface for that matter. > > > > So if a HW block can actually implement two different interfaces, each > > of them being optional, then there should be ways to represent that in > > DT as well. We already do that for "simpler" HW blocks, so there's no > > reason we shouldn't be able to do the same with multimedia components. > > > > If it's really an encoder, though, the problem might be different, > > though, since the interface (at a hardware or functional level if you > > will) remains the same. But I think in that case it's something that > > needs to be figured out internally by the OS. In my opinion, if we are > > in a situation where we have two different drivers in two subsystems for > > the same device, then we're doing something wrong and it should be fixed > > at that level, not by quirking the DT into making a decision for us. > > I tend to agree, and I'd like to be able to share drivers between V4L and KMS. > This is way down the road though. My point is that I don't see a need for sharing the drivers if we can make both V4L2 and DRM/KMS interoperate well enough to cover such use cases. Why share the drivers if we can make it work by sharing the data? Thierry
Hi Thierry, On Friday 25 October 2013 16:10:30 Thierry Reding wrote: > On Fri, Oct 25, 2013 at 03:47:21PM +0200, Laurent Pinchart wrote: > > On Friday 25 October 2013 10:13:15 Thierry Reding wrote: > > > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote: > > > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote: > > > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote: > > > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote: > > > > >> ... > > > > >> > > > > >>>> As I said, anything that really needs a CDF binding to work > > > > >>>> likely isn't "simple" anymore, therefore a separate driver can > > > > >>>> easily be justified. > > > > >>> > > > > >>> The system as a whole would be more complex, but the panel could > > > > >>> be the same. We can't have two drivers for the same piece of > > > > >>> hardware in the DT world, as there will be a single compatible > > > > >>> string and no way to choose between the drivers (unlike the board > > > > >>> code world that could set device names to "foo- encoder-v4l2" or > > > > >>> "foo-encoder-drm" and live happily with that ever after). > > > > >> > > > > >> That's not true. We can certainly define two different compatible > > > > >> values for a piece of HW if we have to. We can easily control > > > > >> whether they are handled by the same or different drivers in the > > > > >> OS. > > > > > > > > > > From an implementation point of view, sure. But from a conceptual > > > > > point of view, that would make the DT bindings pretty Linux- > > > > > specific, with a description of what the operating system should do > > > > > instead of a description of what the hardware looks like. My > > > > > understanding is that we've tried pretty hard in the past not to > > > > > open that Pandora's box. > > > > > > > > > > The case I'm mostly concerned about would be two different > > > > > compatibility strings to select whether the device should be handled > > > > > by a KMS or V4L driver. I don't think that's a good idea. > > > > > > > > I wouldn't think of the two compatible values as selecting different > > > > specific Linux drivers, but rather they simply describe the HW in > > > > different levels of detail. The fact that if we know a certain level > > > > of detail about the HW means that Linux can and does create a KMS > > > > driver rather than a V4L2 driver seems like a detail that's completely > > > > hidden inside the OS. > > > > > > I've had a somewhat similar idea the other day but couldn't really put > > > it into words. Interestingly someone else mentioned a similar concept in > > > a different thread which I think describes what I had in mind as well. > > > > > > I was wondering if we couldn't use two compatible values to denote two > > > > > > interfaces that the device implements. Something along the lines of: > > > compatible = "vendor,block-name", "encoder"; > > > > > > So a driver could primarily match on "vendor,block-name", but at the > > > same time it could use the additional information of being required to > > > implement "encoder" to expose an additonal interface. > > > > Let's take the hardware architecture described in > > http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) > > as an example. The green blocks are part of the capture pipeline and > > handled by the V4L subsystem. The blue blocks are part of the display > > pipeline and handled by the KMS subsystem. One ADV7511 HDMI transmitter > > instance need to be controlled by a KMS driver and the second one by a > > V4L driver. > > > > The two instances are identical, so their DT nodes will show no difference > > if we stick to a hardware description only. There would then be no way to > > bind to different drivers. > > I don't quite see why some of the green parts couldn't be part of the > display (KMS) pipeline. Because there might be no blue pipeline in the device, just the green one. The green pipeline is a video capture pipeline and happens to have an HDMI output with a deep pipeline only, with no frame buffer. We can't create a KMS driver for that, as there's no frame buffer and no CRTC. > I thought I remember you say something about implementing deep pipelining > support in one of the more recent talks. This sounds exactly like a case > where this would be useful. > > Obviously as long as that work isn't finished that won't help you, but I > think that providing a way to pass a video stream object from V4L2 to > DRM/KMS would be a more proper solution to this problem. We need that as well, but it won't solve the problem when no KMS device is present. > > > I suppose that perhaps something like a device_type property could be > > > used for that as well, and that might even be the more correct thing to > > > do. > > > > > > We already do something similar to make GPIO controllers expose an > > > interrupt chip by adding an interrupt-controller property. We also use > > > the gpio-controller property to mark a device node as exposing the GPIO > > > interface for that matter. > > > > > > So if a HW block can actually implement two different interfaces, each > > > of them being optional, then there should be ways to represent that in > > > DT as well. We already do that for "simpler" HW blocks, so there's no > > > reason we shouldn't be able to do the same with multimedia components. > > > > > > If it's really an encoder, though, the problem might be different, > > > though, since the interface (at a hardware or functional level if you > > > will) remains the same. But I think in that case it's something that > > > needs to be figured out internally by the OS. In my opinion, if we are > > > in a situation where we have two different drivers in two subsystems for > > > the same device, then we're doing something wrong and it should be fixed > > > at that level, not by quirking the DT into making a decision for us. > > > > I tend to agree, and I'd like to be able to share drivers between V4L and > > KMS. This is way down the road though. > > My point is that I don't see a need for sharing the drivers if we can > make both V4L2 and DRM/KMS interoperate well enough to cover such use > cases. > > Why share the drivers if we can make it work by sharing the data? Would you like to try and merge the two subsystems ? :-)
On 10/25/2013 12:33 PM, Laurent Pinchart wrote: > On Thursday 24 October 2013 23:06:18 Stephen Warren wrote: >> On 10/24/2013 12:20 PM, Laurent Pinchart wrote: ... >>> The case I'm mostly concerned about would be two different compatibility >>> strings to select whether the device should be handled by a KMS or V4L >>> driver. I don't think that's a good idea. >> >> I wouldn't think of the two compatible values as selecting different >> specific Linux drivers, but rather they simply describe the HW in >> different levels of detail. The fact that if we know a certain level of >> detail about the HW means that Linux can and does create a KMS driver >> rather than a V4L2 driver seems like a detail that's completely hidden >> inside the OS. > > I expect the same level of details to be needed on both the KMS and V4L sides. > Taking the example of the ADV7511 HDMI transmitter, the only change in the DT > bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l" > and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and > -kms to different names wouldn't fundamentally change the problem. Yes, two compatible values such as "adi,adv7511-v4l" and "adi,adv7511-kms" sounds like a bad idea. Rather, shouldn't we have just "adi,adv7511", and the driver for that should register itself in a way that allows either/both the top-level DRM or top-level V4L drivers to find it, and make use of its services. There are plenty of drivers in the kernel already that export services through two different subsystems, e.g. a GPIO HW module that registers as both a struct gpio_chip and a struct irq_chip, or both a struct gpio_chip and a pinctrl device. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt new file mode 100644 index 0000000..72e088a --- /dev/null +++ b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt @@ -0,0 +1,7 @@ +AU Optronics Corporation 10.1" WSVGA TFT LCD panel + +Required properties: +- compatible: should be "auo,b101aw03" + +This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt new file mode 100644 index 0000000..0ab2c05 --- /dev/null +++ b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt @@ -0,0 +1,7 @@ +Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel + +Required properties: +- compatible: should be "chunghwa,claa101wb03" + +This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt new file mode 100644 index 0000000..d328b03 --- /dev/null +++ b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt @@ -0,0 +1,7 @@ +Panasonic Corporation 10.1" WUXGA TFT LCD panel + +Required properties: +- compatible: should be "panasonic,vvx10f004b00" + +This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode 100644 index 0000000..1341bbf --- /dev/null +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt @@ -0,0 +1,21 @@ +Simple display panel + +Required properties: +- power-supply: regulator to provide the supply voltage + +Optional properties: +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing +- enable-gpios: GPIO pin to enable or disable the panel +- backlight: phandle of the backlight device attached to the panel + +Example: + + panel: panel { + compatible = "cptt,claa101wb01"; + ddc-i2c-bus = <&panelddc>; + + power-supply = <&vdd_pnl_reg>; + enable-gpios = <&gpio 90 0>; + + backlight = <&backlight>; + }; diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 81363a9..a9859e0 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -58,3 +58,4 @@ obj-$(CONFIG_DRM_QXL) += qxl/ obj-$(CONFIG_DRM_MSM) += msm/ obj-$(CONFIG_DRM_TEGRA) += tegra/ obj-y += i2c/ +obj-y += panel/ diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 2ddd5bd..843087b 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -2,3 +2,16 @@ menuconfig DRM_PANEL bool "DRM panel support" help Panel registration and lookup framework. + +if DRM_PANEL + +config DRM_PANEL_SIMPLE + bool "support for simple panels" + depends on OF + help + DRM panel driver for dumb panels that need at most a regulator and + a GPIO to be powered up. Optionally a backlight can be attached so + that it can be automatically turned off when the panel goes into a + low power state. + +endif diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile new file mode 100644 index 0000000..af9dfa2 --- /dev/null +++ b/drivers/gpu/drm/panel/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c new file mode 100644 index 0000000..def3e75 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -0,0 +1,335 @@ +/* + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sub license, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the + * next paragraph) shall be included in all copies or substantial portions + * of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + */ + +#include <linux/backlight.h> +#include <linux/gpio.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> + +#include <drm/drm_crtc.h> +#include <drm/drm_panel.h> + +struct panel_desc { + const struct drm_display_mode *modes; + unsigned int num_modes; + + struct { + unsigned int width; + unsigned int height; + } size; +}; + +/* TODO: convert to gpiod_*() API once it's been merged */ +#define GPIO_ACTIVE_LOW (1 << 0) + +struct panel_simple { + struct drm_panel base; + bool enabled; + + const struct panel_desc *desc; + + struct backlight_device *backlight; + struct regulator *supply; + + unsigned long enable_gpio_flags; + int enable_gpio; +}; + +static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) +{ + return container_of(panel, struct panel_simple, base); +} + +static void panel_simple_disable(struct drm_panel *panel) +{ + struct panel_simple *p = to_panel_simple(panel); + + if (!p->enabled) + return; + + if (p->backlight) { + p->backlight->props.power = FB_BLANK_POWERDOWN; + backlight_update_status(p->backlight); + } + + if (gpio_is_valid(p->enable_gpio)) { + if (p->enable_gpio_flags & GPIO_ACTIVE_LOW) + gpio_set_value(p->enable_gpio, 1); + else + gpio_set_value(p->enable_gpio, 0); + } + + regulator_disable(p->supply); + p->enabled = false; +} + +static void panel_simple_enable(struct drm_panel *panel) +{ + struct panel_simple *p = to_panel_simple(panel); + int err; + + if (p->enabled) + return; + + err = regulator_enable(p->supply); + if (err < 0) + dev_err(panel->dev, "failed to enable supply: %d\n", err); + + if (gpio_is_valid(p->enable_gpio)) { + if (p->enable_gpio_flags & GPIO_ACTIVE_LOW) + gpio_set_value(p->enable_gpio, 0); + else + gpio_set_value(p->enable_gpio, 1); + } + + if (p->backlight) { + p->backlight->props.power = FB_BLANK_UNBLANK; + backlight_update_status(p->backlight); + } + + p->enabled = true; +} + +static int panel_simple_get_modes(struct drm_panel *panel) +{ + struct panel_simple *p = to_panel_simple(panel); + struct drm_display_mode *mode; + unsigned int i; + + for (i = 0; i < p->desc->num_modes; i++) { + mode = drm_mode_duplicate(panel->drm, &p->desc->modes[i]); + if (!mode) + return -ENOMEM; + + drm_mode_set_name(mode); + + drm_mode_probed_add(panel->connector, mode); + } + + return p->desc->num_modes; +} + +static const struct drm_panel_funcs panel_simple_funcs = { + .disable = panel_simple_disable, + .enable = panel_simple_enable, + .get_modes = panel_simple_get_modes, +}; + +static const struct drm_display_mode auo_b101aw03_mode = { + .clock = 51450, + .hdisplay = 1024, + .hsync_start = 1024 + 156, + .hsync_end = 1024 + 156 + 8, + .htotal = 1024 + 156 + 8 + 156, + .vdisplay = 600, + .vsync_start = 600 + 16, + .vsync_end = 600 + 16 + 6, + .vtotal = 600 + 16 + 6 + 16, + .vrefresh = 60, +}; + +static const struct panel_desc auo_b101aw03 = { + .modes = &auo_b101aw03_mode, + .num_modes = 1, + .size = { + .width = 223, + .height = 125, + }, +}; + +static const struct drm_display_mode chunghwa_claa101wb01_mode = { + .clock = 69300, + .hdisplay = 1366, + .hsync_start = 1366 + 48, + .hsync_end = 1366 + 48 + 32, + .htotal = 1366 + 48 + 32 + 20, + .vdisplay = 768, + .vsync_start = 768 + 16, + .vsync_end = 768 + 16 + 8, + .vtotal = 768 + 16 + 8 + 16, + .vrefresh = 60, +}; + +static const struct panel_desc chunghwa_claa101wb01 = { + .modes = &chunghwa_claa101wb01_mode, + .num_modes = 1, + .size = { + .width = 223, + .height = 125, + }, +}; + +static const struct drm_display_mode panasonic_vvx10f004b00_mode = { + .clock = 154700, + .hdisplay = 1920, + .hsync_start = 1920 + 154, + .hsync_end = 1920 + 154 + 16, + .htotal = 1920 + 154 + 16 + 32, + .vdisplay = 1200, + .vsync_start = 1200 + 17, + .vsync_end = 1200 + 17 + 2, + .vtotal = 1200 + 17 + 2 + 16, + .vrefresh = 60, +}; + +static const struct panel_desc panasonic_vvx10f004b00 = { + .modes = &panasonic_vvx10f004b00_mode, + .num_modes = 1, + .size = { + .width = 217, + .height = 136, + }, +}; + +static const struct of_device_id panel_simple_of_match[] = { + { + .compatible = "auo,b101aw03", + .data = &auo_b101aw03, + }, { + .compatible = "chunghwa,claa101wb01", + .data = &chunghwa_claa101wb01 + }, { + .compatible = "panasonic,vvx10f004b00", + .data = &panasonic_vvx10f004b00 + }, { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(of, panel_simple_of_match); + +static int panel_simple_probe(struct platform_device *pdev) +{ + const struct of_device_id *id; + struct device_node *backlight; + struct panel_simple *panel; + enum of_gpio_flags flags; + int err; + + panel = devm_kzalloc(&pdev->dev, sizeof(*panel), GFP_KERNEL); + if (!panel) + return -ENOMEM; + + id = of_match_node(panel_simple_of_match, pdev->dev.of_node); + if (!id) + return -ENODEV; + + panel->desc = id->data; + panel->enabled = false; + + panel->supply = devm_regulator_get(&pdev->dev, "power"); + if (IS_ERR(panel->supply)) + return PTR_ERR(panel->supply); + + panel->enable_gpio = of_get_named_gpio_flags(pdev->dev.of_node, + "enable-gpios", 0, + &flags); + if (gpio_is_valid(panel->enable_gpio)) { + unsigned int value; + + if (flags & OF_GPIO_ACTIVE_LOW) + panel->enable_gpio_flags |= GPIO_ACTIVE_LOW; + + err = gpio_request(panel->enable_gpio, "enable"); + if (err < 0) { + dev_err(&pdev->dev, "failed to request GPIO#%u: %d\n", + panel->enable_gpio, err); + return err; + } + + value = (panel->enable_gpio_flags & GPIO_ACTIVE_LOW) != 0; + + err = gpio_direction_output(panel->enable_gpio, value); + if (err < 0) { + dev_err(&pdev->dev, "failed to setup GPIO%u: %d\n", + panel->enable_gpio, err); + goto free_gpio; + } + } + + backlight = of_parse_phandle(pdev->dev.of_node, "backlight", 0); + if (backlight) { + panel->backlight = of_find_backlight_by_node(backlight); + if (!panel->backlight) { + err = -EPROBE_DEFER; + goto free_gpio; + } + + of_node_put(backlight); + } + + drm_panel_init(&panel->base); + panel->base.dev = &pdev->dev; + panel->base.funcs = &panel_simple_funcs; + + err = drm_panel_add(&panel->base); + if (err < 0) + goto free_gpio; + + platform_set_drvdata(pdev, panel); + + return 0; + +free_gpio: + if (gpio_is_valid(panel->enable_gpio)) + gpio_free(panel->enable_gpio); + + return err; +} + +static int panel_simple_remove(struct platform_device *pdev) +{ + struct panel_simple *panel = platform_get_drvdata(pdev); + + drm_panel_detach(&panel->base); + drm_panel_remove(&panel->base); + + panel_simple_disable(&panel->base); + + if (panel->backlight) + put_device(&panel->backlight->dev); + + if (gpio_is_valid(panel->enable_gpio)) + gpio_free(panel->enable_gpio); + + regulator_disable(panel->supply); + + return 0; +} + +static struct platform_driver panel_simple_driver = { + .driver = { + .name = "panel-simple", + .owner = THIS_MODULE, + .of_match_table = panel_simple_of_match, + }, + .probe = panel_simple_probe, + .remove = panel_simple_remove, +}; +module_platform_driver(panel_simple_driver); + +MODULE_DESCRIPTION("DRM Driver for Simple Panels"); +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); +MODULE_LICENSE("GPL v2");
Add a driver for simple panels. Such panels can have a regulator that provides the supply voltage and a separate GPIO to enable the panel. Optionally the panels can have a backlight associated with them so it can be enabled or disabled according to the panel's power management mode. Support is added for three panels: An AU Optronics 10.1" WSVGA, a Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD panel. Signed-off-by: Thierry Reding <treding@nvidia.com> --- .../devicetree/bindings/panel/auo,b101aw03.txt | 7 + .../bindings/panel/chunghwa,claa101wb03.txt | 7 + .../bindings/panel/panasonic,vvx10f004b00.txt | 7 + .../devicetree/bindings/panel/simple-panel.txt | 21 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/panel/Kconfig | 13 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-simple.c | 335 +++++++++++++++++++++ 8 files changed, 392 insertions(+) create mode 100644 Documentation/devicetree/bindings/panel/auo,b101aw03.txt create mode 100644 Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt create mode 100644 Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt create mode 100644 Documentation/devicetree/bindings/panel/simple-panel.txt create mode 100644 drivers/gpu/drm/panel/Makefile create mode 100644 drivers/gpu/drm/panel/panel-simple.c