From: Max Krummenacher <[email protected]>
Commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") added
support for defining a panel from device tree provided data.
However support for setting the bus format is missing, so that with
the current implementation a 'panel-dpi' panel can only be used
if the driver of the display interface connected can cope with a
missing bus_format.
This patch series defines the new property bus-format and adds it to
the panel-dpi implementation.
Check initial discussions [1] and [2].
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Changes in v3:
- New commit to allow for additional port node properties
- Changed the V4L number space as suggested by Rob Herring
- Set constraints for bus-format as suggested by Rob Herring, used
the range reserved for RGB formats.
- Editorial changes as suggested by Rob Herring
- Moved the bus-format property under the port/endpoint node as
suggested by Rob Herring
Changes in v2:
- Fix errors found by dt_binding_check
- New commit allowing bus-format property for derived startek yaml
- Fix errors found by dt_binding_check
Max Krummenacher (4):
dt-bindings: display: panel-common: allow for additional port node
properties
dt-bindings: display: add new bus-format property for panel-dpi
dt-bindings: display: startek,startek-kd050c: allow bus-format
property
drm/panel: simple: add bus-format support for panel-dpi
.../bindings/display/panel/panel-common.yaml | 2 +-
.../bindings/display/panel/panel-dpi.yaml | 26 +++++++++-
.../display/panel/startek,startek-kd050c.yaml | 1 +
drivers/gpu/drm/panel/panel-simple.c | 49 +++++++++++++++++++
.../dt-bindings/display/dt-media-bus-format.h | 23 +++++++++
5 files changed, 99 insertions(+), 2 deletions(-)
create mode 100644 include/dt-bindings/display/dt-media-bus-format.h
--
2.20.1
From: Max Krummenacher <[email protected]>
The property is used to set the enum bus_format and infer the bpc
for a panel defined by 'panel-dpi'.
This specifies how the panel is connected to the display interface.
Signed-off-by: Max Krummenacher <[email protected]>
---
Changes in v3:
- Changed the V4L number space as suggested by Rob Herring
- Set constraints for bus-format as suggested by Rob Herring, used
the range reserved for RGB formats.
- Editorial changes as suggested by Rob Herring
- Moved the bus-format property under the port/endpoint node as
suggested by Rob Herring
Changes in v2:
- Fix errors found by dt_binding_check
.../bindings/display/panel/panel-dpi.yaml | 26 ++++++++++++++++++-
.../dt-bindings/display/dt-media-bus-format.h | 23 ++++++++++++++++
2 files changed, 48 insertions(+), 1 deletion(-)
create mode 100644 include/dt-bindings/display/dt-media-bus-format.h
diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
index dae0676b5c6e..52f5db03b6a8 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
@@ -26,7 +26,28 @@ properties:
height-mm: true
label: true
panel-timing: true
- port: true
+
+ port:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ description:
+ Input port node, receives the panel data.
+
+ properties:
+ endpoint:
+ $ref: /schemas/graph.yaml#/$defs/endpoint-base
+
+ properties:
+ bus-format:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0x1001
+ maximum: 0x1fff
+ description: |
+ Describes how the display panel is connected to the display interface.
+ Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
+ The mapping between the color/significance of the panel lines to the
+ parallel data lines are defined in:
+ https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
+
power-supply: true
reset-gpios: true
width-mm: true
@@ -39,6 +60,8 @@ additionalProperties: false
examples:
- |
+ #include <dt-bindings/display/dt-media-bus-format.h>
+
panel {
compatible = "startek,startek-kd050c", "panel-dpi";
label = "osddisplay";
@@ -47,6 +70,7 @@ examples:
port {
lcd_in: endpoint {
+ bus-format = <DT_MEDIA_BUS_FMT_RGB888_1X24>;
remote-endpoint = <&dpi_out>;
};
};
diff --git a/include/dt-bindings/display/dt-media-bus-format.h b/include/dt-bindings/display/dt-media-bus-format.h
new file mode 100644
index 000000000000..a032d9724ed4
--- /dev/null
+++ b/include/dt-bindings/display/dt-media-bus-format.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
+/*
+ * Copyright 2022 Max Krummenacher <[email protected]>
+ */
+
+#ifndef __DT_BINDINGS_DT_MEDIA_BUS_FORMAT_H
+#define __DT_BINDINGS_DT_MEDIA_BUS_FORMAT_H
+
+/*
+ * Attention: Keep these macro names in sync with
+ * include/uapi/linux/media-bus-format.h
+ */
+
+#define DT_MEDIA_BUS_FMT_RGB565_1X16 0x1017
+#define DT_MEDIA_BUS_FMT_RGB666_1X18 0x1009
+#define DT_MEDIA_BUS_FMT_RBG888_1X24 0x100e
+#define DT_MEDIA_BUS_FMT_RGB666_1X24_CPADHI 0x1015
+#define DT_MEDIA_BUS_FMT_BGR888_1X24 0x1013
+#define DT_MEDIA_BUS_FMT_GBR888_1X24 0x1014
+#define DT_MEDIA_BUS_FMT_RGB888_1X24 0x100a
+#define DT_MEDIA_BUS_FMT_RGB888_1X32_PADHI 0x100f
+
+#endif /* __DT_BINDINGS_DT_MEDIA_BUS_FORMAT_H */
--
2.20.1
From: Max Krummenacher <[email protected]>
Evaluate the device tree bus-format property to set bus_format for
a 'panel-dpi' panel. Additionally infer the bpc value from the
given bus-format.
Valid values for bus-format are found in:
<include/dt-bindings/display/dt-media-bus-format.h>
This completes the addition of panel-dpi to completely specify
a panel-simple panel from the device tree.
Signed-off-by: Max Krummenacher <[email protected]>
---
Changes in v3:
- Moved the bus-format property under the port/endpoint node as
suggested by Rob Herring
Changes in v2:
- Fix errors found by dt_binding_check
drivers/gpu/drm/panel/panel-simple.c | 49 ++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 4a2e580a2f7b..f1a457f1069e 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -21,9 +21,11 @@
* DEALINGS IN THE SOFTWARE.
*/
+#include <dt-bindings/display/dt-media-bus-format.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/module.h>
+#include <linux/of_graph.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
@@ -449,10 +451,12 @@ static int panel_dpi_probe(struct device *dev,
struct panel_simple *panel)
{
struct display_timing *timing;
+ struct device_node *endpoint;
const struct device_node *np;
struct panel_desc *desc;
unsigned int bus_flags;
struct videomode vm;
+ u32 bus_format;
int ret;
np = dev->of_node;
@@ -477,6 +481,51 @@ static int panel_dpi_probe(struct device *dev,
of_property_read_u32(np, "width-mm", &desc->size.width);
of_property_read_u32(np, "height-mm", &desc->size.height);
+ endpoint = of_graph_get_endpoint_by_regs(np, -1, -1);
+ if (endpoint &&
+ !of_property_read_u32(endpoint, "bus-format", &bus_format)) {
+ /* infer bpc from bus-format */
+ switch (bus_format) {
+ case DT_MEDIA_BUS_FMT_RGB565_1X16:
+ desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
+ desc->bpc = 6;
+ break;
+ case DT_MEDIA_BUS_FMT_RGB666_1X18:
+ desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+ desc->bpc = 6;
+ break;
+ case DT_MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
+ desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+ desc->bpc = 6;
+ break;
+ case DT_MEDIA_BUS_FMT_BGR888_1X24:
+ desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
+ desc->bpc = 8;
+ break;
+ case DT_MEDIA_BUS_FMT_GBR888_1X24:
+ desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
+ desc->bpc = 8;
+ break;
+ case DT_MEDIA_BUS_FMT_RBG888_1X24:
+ desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
+ desc->bpc = 8;
+ break;
+ case DT_MEDIA_BUS_FMT_RGB888_1X24:
+ desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+ desc->bpc = 8;
+ break;
+ case DT_MEDIA_BUS_FMT_RGB888_1X32_PADHI:
+ desc->bus_format = MEDIA_BUS_FMT_RGB888_1X32_PADHI;
+ desc->bpc = 8;
+ break;
+ default:
+ dev_err(dev, "%pOF: unknown bus-format property\n", np);
+ return -EINVAL;
+ }
+ }
+
+ of_node_put(endpoint);
+
/* Extract bus_flags from display_timing */
bus_flags = 0;
vm.flags = timing->flags;
--
2.20.1
From: Max Krummenacher <[email protected]>
Allow to specify the optional bus-format property newly added to
panel-dpi.
Signed-off-by: Max Krummenacher <[email protected]>
---
(no changes since v2)
Changes in v2:
- New commit allowing bus-format property for derived startek yaml
.../bindings/display/panel/startek,startek-kd050c.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/display/panel/startek,startek-kd050c.yaml b/Documentation/devicetree/bindings/display/panel/startek,startek-kd050c.yaml
index fd668640afd1..05306713044e 100644
--- a/Documentation/devicetree/bindings/display/panel/startek,startek-kd050c.yaml
+++ b/Documentation/devicetree/bindings/display/panel/startek,startek-kd050c.yaml
@@ -19,6 +19,7 @@ properties:
- {} # panel-dpi, but not listed here to avoid false select
backlight: true
+ bus-format: true
enable-gpios: true
height-mm: true
label: true
--
2.20.1
From: Max Krummenacher <[email protected]>
Allow bindings which reference panel-common.yaml to add additional
properties under the port node.
I.e. 'panel-dpi' needs to add a new property to 'port/endpoint'.
Signed-off-by: Max Krummenacher <[email protected]>
---
Changes in v3:
- New commit to allow for additional port node properties
.../devicetree/bindings/display/panel/panel-common.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
index 5b38dc89cb21..ff8dc07ef3b5 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
@@ -68,7 +68,7 @@ properties:
# Connectivity
port:
- $ref: /schemas/graph.yaml#/properties/port
+ $ref: /schemas/graph.yaml#/$defs/port-base
ddc-i2c-bus:
$ref: /schemas/types.yaml#/definitions/phandle
--
2.20.1
On Tue, Jun 28, 2022 at 08:18:35PM +0200, Max Krummenacher wrote:
> From: Max Krummenacher <[email protected]>
>
> Allow bindings which reference panel-common.yaml to add additional
> properties under the port node.
> I.e. 'panel-dpi' needs to add a new property to 'port/endpoint'.
>
> Signed-off-by: Max Krummenacher <[email protected]>
>
> ---
>
> Changes in v3:
> - New commit to allow for additional port node properties
>
> .../devicetree/bindings/display/panel/panel-common.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index 5b38dc89cb21..ff8dc07ef3b5 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -68,7 +68,7 @@ properties:
>
> # Connectivity
> port:
> - $ref: /schemas/graph.yaml#/properties/port
> + $ref: /schemas/graph.yaml#/$defs/port-base
This will allow extra properties for everyone using this. That means
either bus-format needs to go in here (so that it is the only extra
property allowed) or we should remove 'port' here and push this into all
the users.
But we should reach agreement on bus-format before doing anything.
Rob
On 28-Jun-22 23:48, Max Krummenacher wrote:
> From: Max Krummenacher <[email protected]>
>
> Evaluate the device tree bus-format property to set bus_format for
> a 'panel-dpi' panel. Additionally infer the bpc value from the
> given bus-format.
>
> Valid values for bus-format are found in:
> <include/dt-bindings/display/dt-media-bus-format.h>
>
> This completes the addition of panel-dpi to completely specify
> a panel-simple panel from the device tree.
>
> Signed-off-by: Max Krummenacher <[email protected]>
>
> ---
>
> Changes in v3:
> - Moved the bus-format property under the port/endpoint node as
> suggested by Rob Herring
>
> Changes in v2:
> - Fix errors found by dt_binding_check
>
> drivers/gpu/drm/panel/panel-simple.c | 49 ++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 4a2e580a2f7b..f1a457f1069e 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -21,9 +21,11 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> +#include <dt-bindings/display/dt-media-bus-format.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/module.h>
> +#include <linux/of_graph.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> @@ -449,10 +451,12 @@ static int panel_dpi_probe(struct device *dev,
> struct panel_simple *panel)
> {
> struct display_timing *timing;
> + struct device_node *endpoint;
> const struct device_node *np;
> struct panel_desc *desc;
> unsigned int bus_flags;
> struct videomode vm;
> + u32 bus_format;
> int ret;
>
> np = dev->of_node;
> @@ -477,6 +481,51 @@ static int panel_dpi_probe(struct device *dev,
> of_property_read_u32(np, "width-mm", &desc->size.width);
> of_property_read_u32(np, "height-mm", &desc->size.height);
>
> + endpoint = of_graph_get_endpoint_by_regs(np, -1, -1);
> + if (endpoint &&
> + !of_property_read_u32(endpoint, "bus-format", &bus_format)) {
> + /* infer bpc from bus-format */
> + switch (bus_format) {
> + case DT_MEDIA_BUS_FMT_RGB565_1X16:
> + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> + desc->bpc = 6;
> + break;
> + case DT_MEDIA_BUS_FMT_RGB666_1X18:
> + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> + desc->bpc = 6;
> + break;
> + case DT_MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> + desc->bpc = 6;
> + break;
> + case DT_MEDIA_BUS_FMT_BGR888_1X24:
> + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> + desc->bpc = 8;
> + break;
> + case DT_MEDIA_BUS_FMT_GBR888_1X24:
> + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> + desc->bpc = 8;
> + break;
> + case DT_MEDIA_BUS_FMT_RBG888_1X24:
> + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> + desc->bpc = 8;
> + break;
> + case DT_MEDIA_BUS_FMT_RGB888_1X24:
> + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> + desc->bpc = 8;
> + break;
> + case DT_MEDIA_BUS_FMT_RGB888_1X32_PADHI:
> + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X32_PADHI;
> + desc->bpc = 8;
> + break;
> + default:
> + dev_err(dev, "%pOF: unknown bus-format property\n", np);
> + return -EINVAL;
> + }
> + }
> +
> + of_node_put(endpoint);
> +
> /* Extract bus_flags from display_timing */
> bus_flags = 0;
> vm.flags = timing->flags;
I understand that it is important to add a bus-format property for dumb
dpi-panels, and I agree with the implementation in the patch-set.
However,
I do not yet fully understand Rob's comments on the dt-bindings side of
patch set (patch 1/4) and what consequences it may cause if that remains
unresolved.
Given that the bus-format property gets added, I do not see any concern
with the panel-simple driver patch.
Reviewed-by: Aradhya Bhatia <[email protected]>
Regards
Aradhya
On 6/28/22 20:18, Max Krummenacher wrote:
Hello Max,
[...]
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> index dae0676b5c6e..52f5db03b6a8 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> @@ -26,7 +26,28 @@ properties:
> height-mm: true
> label: true
> panel-timing: true
> - port: true
> +
> + port:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + description:
> + Input port node, receives the panel data.
> +
> + properties:
> + endpoint:
> + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +
> + properties:
> + bus-format:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0x1001
> + maximum: 0x1fff
> + description: |
> + Describes how the display panel is connected to the display interface.
> + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> + The mapping between the color/significance of the panel lines to the
> + parallel data lines are defined in:
> + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
I am not sure whether I should re-open this discussion, but I still
wonder, wouldn't it be better to describe the DPI bus color channel
ordering (RGB, BGR, ...), width of each color channel in bits, pixel
format (RGB, YUV, ...) instead of using specific constants for the
entire format ?
Hi Aradhya
Thanks for the review.
Rob's comment requires changes to panel-common.yaml in order for this to get in.
I think I know what needs to be done.
However, as there is no agreement on bus-format in the first place there is no
point in sorting that out now.
Regards
Max
On Wed, Aug 3, 2022 at 8:45 AM Aradhya Bhatia <[email protected]> wrote:
>
>
>
> On 28-Jun-22 23:48, Max Krummenacher wrote:
> > From: Max Krummenacher <[email protected]>
> >
> > Evaluate the device tree bus-format property to set bus_format for
> > a 'panel-dpi' panel. Additionally infer the bpc value from the
> > given bus-format.
> >
> > Valid values for bus-format are found in:
> > <include/dt-bindings/display/dt-media-bus-format.h>
> >
> > This completes the addition of panel-dpi to completely specify
> > a panel-simple panel from the device tree.
> >
> > Signed-off-by: Max Krummenacher <[email protected]>
> >
> > ---
> >
> > Changes in v3:
> > - Moved the bus-format property under the port/endpoint node as
> > suggested by Rob Herring
> >
> > Changes in v2:
> > - Fix errors found by dt_binding_check
> >
> > drivers/gpu/drm/panel/panel-simple.c | 49 ++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 4a2e580a2f7b..f1a457f1069e 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -21,9 +21,11 @@
> > * DEALINGS IN THE SOFTWARE.
> > */
> >
> > +#include <dt-bindings/display/dt-media-bus-format.h>
> > #include <linux/delay.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/module.h>
> > +#include <linux/of_graph.h>
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > @@ -449,10 +451,12 @@ static int panel_dpi_probe(struct device *dev,
> > struct panel_simple *panel)
> > {
> > struct display_timing *timing;
> > + struct device_node *endpoint;
> > const struct device_node *np;
> > struct panel_desc *desc;
> > unsigned int bus_flags;
> > struct videomode vm;
> > + u32 bus_format;
> > int ret;
> >
> > np = dev->of_node;
> > @@ -477,6 +481,51 @@ static int panel_dpi_probe(struct device *dev,
> > of_property_read_u32(np, "width-mm", &desc->size.width);
> > of_property_read_u32(np, "height-mm", &desc->size.height);
> >
> > + endpoint = of_graph_get_endpoint_by_regs(np, -1, -1);
> > + if (endpoint &&
> > + !of_property_read_u32(endpoint, "bus-format", &bus_format)) {
> > + /* infer bpc from bus-format */
> > + switch (bus_format) {
> > + case DT_MEDIA_BUS_FMT_RGB565_1X16:
> > + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > + desc->bpc = 6;
> > + break;
> > + case DT_MEDIA_BUS_FMT_RGB666_1X18:
> > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > + desc->bpc = 6;
> > + break;
> > + case DT_MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > + desc->bpc = 6;
> > + break;
> > + case DT_MEDIA_BUS_FMT_BGR888_1X24:
> > + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > + desc->bpc = 8;
> > + break;
> > + case DT_MEDIA_BUS_FMT_GBR888_1X24:
> > + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > + desc->bpc = 8;
> > + break;
> > + case DT_MEDIA_BUS_FMT_RBG888_1X24:
> > + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > + desc->bpc = 8;
> > + break;
> > + case DT_MEDIA_BUS_FMT_RGB888_1X24:
> > + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > + desc->bpc = 8;
> > + break;
> > + case DT_MEDIA_BUS_FMT_RGB888_1X32_PADHI:
> > + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X32_PADHI;
> > + desc->bpc = 8;
> > + break;
> > + default:
> > + dev_err(dev, "%pOF: unknown bus-format property\n", np);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + of_node_put(endpoint);
> > +
> > /* Extract bus_flags from display_timing */
> > bus_flags = 0;
> > vm.flags = timing->flags;
>
> I understand that it is important to add a bus-format property for dumb
> dpi-panels, and I agree with the implementation in the patch-set.
>
> However,
> I do not yet fully understand Rob's comments on the dt-bindings side of
> patch set (patch 1/4) and what consequences it may cause if that remains
> unresolved.
>
> Given that the bus-format property gets added, I do not see any concern
> with the panel-simple driver patch.
>
>
> Reviewed-by: Aradhya Bhatia <[email protected]>
>
>
> Regards
> Aradhya
Hi Marek
On Wed, Aug 3, 2022 at 10:21 AM Marek Vasut <[email protected]> wrote:
>
> On 6/28/22 20:18, Max Krummenacher wrote:
>
> Hello Max,
>
> [...]
>
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > index dae0676b5c6e..52f5db03b6a8 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > @@ -26,7 +26,28 @@ properties:
> > height-mm: true
> > label: true
> > panel-timing: true
> > - port: true
> > +
> > + port:
> > + $ref: /schemas/graph.yaml#/$defs/port-base
> > + description:
> > + Input port node, receives the panel data.
> > +
> > + properties:
> > + endpoint:
> > + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +
> > + properties:
> > + bus-format:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x1001
> > + maximum: 0x1fff
> > + description: |
> > + Describes how the display panel is connected to the display interface.
> > + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> > + The mapping between the color/significance of the panel lines to the
> > + parallel data lines are defined in:
> > + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
>
> I am not sure whether I should re-open this discussion, but I still
> wonder, wouldn't it be better to describe the DPI bus color channel
> ordering (RGB, BGR, ...), width of each color channel in bits, pixel
> format (RGB, YUV, ...) instead of using specific constants for the
> entire format ?
From a system view it would be hard to define that structure which
will catch any and all future requirements. Assume that there will be
3D panels and they will need an additional depth field. Or in
in addition to RGB data there will be a fourth color component. Or
whatever the panel manufacturers might come up with...
Or consider the Tegra 30 example I brought up in this thread. Tegras can
output RGB666 for 18bit panels, and then use the next 8 bits to extend
to 24bit (Maybe RGB666RGB222 ?).
https://lore.kernel.org/all/[email protected]/
If such requirements pop up the enumeration can be extended with a new
value without changing the binding in any way, with a structured
approach this will require changed bindings, maybe even with issues
in backward compatibility.
From an implementation perspective for Linux the busformat in code is
currently an enumeration. So one would have to take the device tree
structured busformat and run it through a potentially complicated
function to get to the Linux busformat enumeration value. The final
consumer has no advantage over what is there today.
IMHO a change away from one enumeration value to a structured approach
creates some drawbacks without any obvious advantages.
Comments, other views on that?
Regards
Max
On 8/8/22 15:56, Max Krummenacher wrote:
> Hi Marek
Hello Max,
[...]
>>> + properties:
>>> + bus-format:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + minimum: 0x1001
>>> + maximum: 0x1fff
>>> + description: |
>>> + Describes how the display panel is connected to the display interface.
>>> + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
>>> + The mapping between the color/significance of the panel lines to the
>>> + parallel data lines are defined in:
>>> + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
>>
>> I am not sure whether I should re-open this discussion, but I still
>> wonder, wouldn't it be better to describe the DPI bus color channel
>> ordering (RGB, BGR, ...), width of each color channel in bits, pixel
>> format (RGB, YUV, ...) instead of using specific constants for the
>> entire format ?
>
>>From a system view it would be hard to define that structure which
> will catch any and all future requirements. Assume that there will be
> 3D panels and they will need an additional depth field.
You can very much say you have panels which require Y/U/V color channels
instead of R/G/B , and then just add more color channels as needed. But
that -- color channel, their order, offset, bit width, can be described
rather easily, something like:
color-channel-names = "R", "G", "B";
color-channel-width = <8 8 8>;
color-channel-shift = <16 8 0>;
> Or in
> in addition to RGB data there will be a fourth color component. Or
> whatever the panel manufacturers might come up with...
> Or consider the Tegra 30 example I brought up in this thread. Tegras can
> output RGB666 for 18bit panels, and then use the next 8 bits to extend
> to 24bit (Maybe RGB666RGB222 ?).
I think there are two options here:
1) Look at 'data-lanes' property on DSI ? Both the DSI host and DSI
device define the 'data-lanes' property per endpoint and they might
not be the same.
But with DPI, the better option might be:
2) Implement something like LVDS codec, some sort of transparent DPI
bridge driver which can be defined in DT and represent the "glue"
wiring adapter between the mismatched DPI source and sink formats.
> https://lore.kernel.org/all/[email protected]/
> If such requirements pop up the enumeration can be extended with a new
> value without changing the binding in any way, with a structured
> approach this will require changed bindings, maybe even with issues
> in backward compatibility.
If we have 2) which would describe how the DPI wires were connected,
like "channel R got shifted by two bits, bottom two bits got replicated,
etc.", then maybe we can avoid introducing new non-standard formats
altogether ?
>>From an implementation perspective for Linux the busformat in code is
> currently an enumeration. So one would have to take the device tree
> structured busformat and run it through a potentially complicated
> function to get to the Linux busformat enumeration value. The final
> consumer has no advantage over what is there today.
>
> IMHO a change away from one enumeration value to a structured approach
> creates some drawbacks without any obvious advantages.
>
> Comments, other views on that?
See above.
Hello Max, Marek, Dave et al.
On Tue, Jun 28, 2022 at 08:18:36PM +0200, Max Krummenacher wrote:
> From: Max Krummenacher <[email protected]>
>
> The property is used to set the enum bus_format and infer the bpc
> for a panel defined by 'panel-dpi'.
> This specifies how the panel is connected to the display interface.
>
> Signed-off-by: Max Krummenacher <[email protected]>
>
<snip>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> index dae0676b5c6e..52f5db03b6a8 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> @@ -26,7 +26,28 @@ properties:
> height-mm: true
> label: true
> panel-timing: true
> - port: true
> +
> + port:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + description:
> + Input port node, receives the panel data.
> +
> + properties:
> + endpoint:
> + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +
> + properties:
> + bus-format:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0x1001
> + maximum: 0x1fff
> + description: |
> + Describes how the display panel is connected to the display interface.
> + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> + The mapping between the color/significance of the panel lines to the
> + parallel data lines are defined in:
> + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
> +
Last month I had the chance to talk in person about this topic with
Dave, Marek and Max in Dublin.
My understanding is that this change is addressing a general need, Dave
confirmed me they have a downstream patch for raspberrypi [1].
From what I could tell the only concern is about the actual encoding of
this `bus-format` property.
I am personally convinced that a simple enum is the way to go, I think
that Marek proposal is adding complexity and not flexibility (from my
understanding Dave is on the same page, just correct me if I
misunderstood you).
The current proposal is already encoding the exact bit placing as
described in Documentation/userspace-api/media/v4l/subdev-formats.rst [2],
this enumeration can be extended to address any future needs
and I would not invent a new one to define the exact same
things (and using the same enum was also suggested by Rob).
Marek: you told me that you had some concern about some valid use case
not covered by this solution, would you mind explaining why that would
not be covered with an addition on this enumeration?
Any other opinion on this topic? How can we move this forward?
Francesco
[1] https://github.com/raspberrypi/linux/commit/8e43f1898191b43aa7ed6e6ca3a4cd28709af86d
[2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html
Hi Francesco
On Thu, 13 Oct 2022 at 13:58, Francesco Dolcini <[email protected]> wrote:
>
> Hello Max, Marek, Dave et al.
>
> On Tue, Jun 28, 2022 at 08:18:36PM +0200, Max Krummenacher wrote:
> > From: Max Krummenacher <[email protected]>
> >
> > The property is used to set the enum bus_format and infer the bpc
> > for a panel defined by 'panel-dpi'.
> > This specifies how the panel is connected to the display interface.
> >
> > Signed-off-by: Max Krummenacher <[email protected]>
> >
>
> <snip>
>
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > index dae0676b5c6e..52f5db03b6a8 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > @@ -26,7 +26,28 @@ properties:
> > height-mm: true
> > label: true
> > panel-timing: true
> > - port: true
> > +
> > + port:
> > + $ref: /schemas/graph.yaml#/$defs/port-base
> > + description:
> > + Input port node, receives the panel data.
> > +
> > + properties:
> > + endpoint:
> > + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +
> > + properties:
> > + bus-format:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x1001
> > + maximum: 0x1fff
> > + description: |
> > + Describes how the display panel is connected to the display interface.
> > + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> > + The mapping between the color/significance of the panel lines to the
> > + parallel data lines are defined in:
> > + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
> > +
>
> Last month I had the chance to talk in person about this topic with
> Dave, Marek and Max in Dublin.
>
> My understanding is that this change is addressing a general need, Dave
> confirmed me they have a downstream patch for raspberrypi [1].
>
> From what I could tell the only concern is about the actual encoding of
> this `bus-format` property.
>
> I am personally convinced that a simple enum is the way to go, I think
> that Marek proposal is adding complexity and not flexibility (from my
> understanding Dave is on the same page, just correct me if I
> misunderstood you).
Yes I agree with you here.
This binding is for the panel, and currently the only path to pass the
panel mode to the DPI transmitter is one or more MEDIA_BUS_FMT_* enums
in struct drm_display_info *bus_formats.
Looking at Marek's comment over DSI and data-lanes, yes both source
and sink could advertise a data-lanes property to cover the condition
where they aren't wired up in a 1:1 fashion. Reality is that most
drivers don't support reordering the lanes - looking at the bindings,
only one (msm) documents the use of data-lanes on the host side.
rcar_mipi_dsi looks at the number of lanes specified only, and then
checks that the number requested by the device is <= the number
configured.
As I see it, the comparison here is that this "bus-format" property is
the equivalent of the data-lanes on the sink, and is the desired
number of lanes value passed from sink to source (one integer, not a
mapping).
If the source can reorder the lanes, then that is a property of the
source. This binding is for the sink, and so isn't a reasonable
comparison. It also doesn't have to be called "bus-format" on the
source, and can take a totally different form.
I'll admit that I know data-lane configuration more from CSI2, but
within V4L2 it is the node that can support reordering that should
have the lanes in a non-incrementing order, and that is normally the
SoC rather than the sensor. The same would seem to apply here - it's
the SoC that can remap the signals, not the panel.
It could be argued that for DPI the panel should only advertise the
panel's bit depth for each channel, not the padding. The panel is
generic and could handle any wiring/padding options, and it isn't
necessarily a simple 16/18/24/32 bit bus representation, just a
collection of N wires.
Padding and wiring is a function of the DPI transmitter / SoC, or
potentially an interconnect node between the two.
> The current proposal is already encoding the exact bit placing as
> described in Documentation/userspace-api/media/v4l/subdev-formats.rst [2],
> this enumeration can be extended to address any future needs
> and I would not invent a new one to define the exact same
> things (and using the same enum was also suggested by Rob).
>
> Marek: you told me that you had some concern about some valid use case
> not covered by this solution, would you mind explaining why that would
> not be covered with an addition on this enumeration?
All the MEDIA_BUS_FMT_* enums are explicitly defined with regard to
the colour component bit positions. Should someone be in the position
of needing to implement a YUV or similar DPI display, converting these
enums into the relevant new structure will be straightforward, so
backwards compatibility can be achieved easily.
Dave
> Any other opinion on this topic? How can we move this forward?
>
> Francesco
>
> [1] https://github.com/raspberrypi/linux/commit/8e43f1898191b43aa7ed6e6ca3a4cd28709af86d
> [2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html
>
Hello,
On Fri, Oct 14, 2022 at 03:08:49PM +0100, Dave Stevenson wrote:
> On Thu, 13 Oct 2022 at 13:58, Francesco Dolcini wrote:
> > On Tue, Jun 28, 2022 at 08:18:36PM +0200, Max Krummenacher wrote:
> > > From: Max Krummenacher <[email protected]>
> > >
> > > The property is used to set the enum bus_format and infer the bpc
> > > for a panel defined by 'panel-dpi'.
> > > This specifies how the panel is connected to the display interface.
> > >
> > > Signed-off-by: Max Krummenacher <[email protected]>
> > >
> >
> > <snip>
> >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > index dae0676b5c6e..52f5db03b6a8 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > @@ -26,7 +26,28 @@ properties:
> > > height-mm: true
> > > label: true
> > > panel-timing: true
> > > - port: true
> > > +
> > > + port:
> > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > + description:
> > > + Input port node, receives the panel data.
> > > +
> > > + properties:
> > > + endpoint:
> > > + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > > +
> > > + properties:
> > > + bus-format:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + minimum: 0x1001
> > > + maximum: 0x1fff
> > > + description: |
> > > + Describes how the display panel is connected to the display interface.
> > > + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> > > + The mapping between the color/significance of the panel lines to the
> > > + parallel data lines are defined in:
> > > + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
> > > +
> >
> > Last month I had the chance to talk in person about this topic with
> > Dave, Marek and Max in Dublin.
> >
> > My understanding is that this change is addressing a general need, Dave
> > confirmed me they have a downstream patch for raspberrypi [1].
> >
> > From what I could tell the only concern is about the actual encoding of
> > this `bus-format` property.
> >
> > I am personally convinced that a simple enum is the way to go, I think
> > that Marek proposal is adding complexity and not flexibility (from my
> > understanding Dave is on the same page, just correct me if I
> > misunderstood you).
>
> Yes I agree with you here.
>
> This binding is for the panel, and currently the only path to pass the
> panel mode to the DPI transmitter is one or more MEDIA_BUS_FMT_* enums
> in struct drm_display_info *bus_formats.
>
> Looking at Marek's comment over DSI and data-lanes, yes both source
> and sink could advertise a data-lanes property to cover the condition
> where they aren't wired up in a 1:1 fashion. Reality is that most
> drivers don't support reordering the lanes - looking at the bindings,
> only one (msm) documents the use of data-lanes on the host side.
> rcar_mipi_dsi looks at the number of lanes specified only, and then
> checks that the number requested by the device is <= the number
> configured.
>
> As I see it, the comparison here is that this "bus-format" property is
> the equivalent of the data-lanes on the sink, and is the desired
> number of lanes value passed from sink to source (one integer, not a
> mapping).
> If the source can reorder the lanes, then that is a property of the
> source. This binding is for the sink, and so isn't a reasonable
> comparison. It also doesn't have to be called "bus-format" on the
> source, and can take a totally different form.
> I'll admit that I know data-lane configuration more from CSI2, but
> within V4L2 it is the node that can support reordering that should
> have the lanes in a non-incrementing order, and that is normally the
> SoC rather than the sensor. The same would seem to apply here - it's
> the SoC that can remap the signals, not the panel.
>
> It could be argued that for DPI the panel should only advertise the
> panel's bit depth for each channel, not the padding. The panel is
> generic and could handle any wiring/padding options, and it isn't
> necessarily a simple 16/18/24/32 bit bus representation, just a
> collection of N wires.
> Padding and wiring is a function of the DPI transmitter / SoC, or
> potentially an interconnect node between the two.
Sooo... I'm not sure where to start :-)
I think the trouble when describing the connection between a source and
a sink in DT is that none of the source or sink is an ideal place to
describe properties of the connection.
For DSI we have it relatively easy, as we only have to describe the
number of lanes that are routed on the board and possibly how the lanes
are rearranged. The former is a value that is common between the source
and the sink, that's the easiest case, it can be specified in both DT
nodes. The latter is a bit more complicated, and was solved by allowing
specifying lane reordering on both the source and the sink. As there is
typically only one of the two components that will support lane
reordering (if any), DTs will usually specify a 1:1 mapping on one side,
and possibly reorder on the other side. If both the source and the sink
support reordering, setting data-lanes = <1 2> on both sides would lead
to a different configuration than data-lanes = <2 1>, but both would
work the same (I'm not sure why anyone would want the latter though).
There may thus be multiple ways to describe a working setup, but that's
fine, the complexity is manageable, and any hardware configuration can
be described.
The nice thing with DSI is that the actual data format doesn't depend on
the board configuration (provided of course that enough lanes are
available to sustain the required bandwidth). For DPI, things can be
more difficult. In the test below, "format" refers to how data bits are
mapped to hardware lines, similarly in concept to the media bus codes.
I see three different cases at the hardware level:
- Either or both the sink or the source support a single format. This
means that the side that supports multiple formats will always use the
same format. If data lines are rearranged, the format output by the
source may not match the format received by the sink, but the hardware
configuration of both the sink and the source is effectively fixed to
system-specific values.
- Both the sink and the source support multiple formats, but only one
combination of formats is possible with how the data lines are routed.
This case is very similar to the previous one at the hardware level,
only one configuration is possible.
- Both the sink and the source support multiple formats, and multiple
format combinations can lead to working configurations. This isn't an
uncommon case, there are DPI panels with 24 data lines that can
support both RGB666 and RGB888.
At the software level, there are also multiple options:
- Both sides could specify the device configuration in DT, using media
bus codes or any other set of standard or device-specific properties.
As this would specify a single configuration, it would map quite fine
to the first two hardware cases. Each driver would read its own
properties and configure the device accordingly. There would be no
need for communication between the drivers at runtime in this case.
This could also support the third hardware case, but would limit it to
one of the supported configurations, without allowing the other ones
to be selected at runtime.
This scheme is similar to data-lanes, in the sense that each side
reads its own hardcoded configuration from DT. It does however differ
in that the data format gets hardcoded as well, unlike DSI where the
data formats needs to be communicated at runtime between the drivers.
As, like DSI, it requires both sides to specify their hardware
configuration in DT, interoperability between sources and sinks would
require all DT bindings for all DPI devices to adhere to this. They
may not have to specify their configuration using the same set of
properties, but they would all need to specify it in DT. This would
thus, I think, lead to a dead end for the third hardware case.
- The two sides could communicate at runtime to dynamically negotiate
their configuration. Some form of runtime configuration is required to
fully support the third hardware case, and it could also support the
other two cases.
The trouble here, beside how to express the required data in DT, is
how that communication would be handled. Let's consider a case where
data lines are "remapped":
- The display controller that has a D[23:0] output bus
- The panel that has a D[17:0] bus
- The data lines connections from the display controller to the panel
are D[23:18] -> D[17:12], D[15:10] -> D[11:6], D[7:2] -> D[5:0],
with the display controller's D[17:16], D[9:8] and D[1:0] outputs
unconnected
- The panel only supports RGB666
- The display controller supports both RGB888 and RGB666, and outputs
RGB666 as 00RRRRRR00GGGGGG00BBBBBB
This means that the only possibly configuration is the display
controller outputting RGB888 and the panel receiving RGB666. If we
expressed that as media bus codes in DT, the panel would would
communicate its RGB666 input format to the display controller, which
wouldn't know that it would have to output RGB888.
Of course, in this particular example, only one hardware configuration
is possible, so we could support it by specifying the media bus code
in both DT nodes, but that won't scale to cases where multiple
configurations are possible.
The easy optin is to consider that most use cases are in the first two
hardware categories, specify the media bus code in DT on both sides, and
consider that support for the third category can be added later. I'm
worried that we would then corner ourselves, as explained above, because
this scheme requires all devices involved to specify their hardcoded
configuration in DT. Will there then be a path forward that wouldn't
break the DT ABI ? Even if there was, it would mean that all driver
would then need to support two sets of DT properties, leading to a
painful transition period on the driver side.
> > The current proposal is already encoding the exact bit placing as
> > described in Documentation/userspace-api/media/v4l/subdev-formats.rst [2],
> > this enumeration can be extended to address any future needs
> > and I would not invent a new one to define the exact same
> > things (and using the same enum was also suggested by Rob).
> >
> > Marek: you told me that you had some concern about some valid use case
> > not covered by this solution, would you mind explaining why that would
> > not be covered with an addition on this enumeration?
>
> All the MEDIA_BUS_FMT_* enums are explicitly defined with regard to
> the colour component bit positions. Should someone be in the position
> of needing to implement a YUV or similar DPI display, converting these
> enums into the relevant new structure will be straightforward, so
> backwards compatibility can be achieved easily.
>
> > Any other opinion on this topic? How can we move this forward?
> >
> > Francesco
> >
> > [1] https://github.com/raspberrypi/linux/commit/8e43f1898191b43aa7ed6e6ca3a4cd28709af86d
> > [2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html
--
Regards,
Laurent Pinchart
Hi Laurent
On Sun, Oct 16, 2022 at 3:33 AM Laurent Pinchart
<[email protected]> wrote:
>
> Hello,
>
> On Fri, Oct 14, 2022 at 03:08:49PM +0100, Dave Stevenson wrote:
> > On Thu, 13 Oct 2022 at 13:58, Francesco Dolcini wrote:
> > > On Tue, Jun 28, 2022 at 08:18:36PM +0200, Max Krummenacher wrote:
> > > > From: Max Krummenacher <[email protected]>
> > > >
> > > > The property is used to set the enum bus_format and infer the bpc
> > > > for a panel defined by 'panel-dpi'.
> > > > This specifies how the panel is connected to the display interface.
> > > >
> > > > Signed-off-by: Max Krummenacher <[email protected]>
> > > >
> > >
> > > <snip>
> > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > > index dae0676b5c6e..52f5db03b6a8 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > > @@ -26,7 +26,28 @@ properties:
> > > > height-mm: true
> > > > label: true
> > > > panel-timing: true
> > > > - port: true
> > > > +
> > > > + port:
> > > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > > + description:
> > > > + Input port node, receives the panel data.
> > > > +
> > > > + properties:
> > > > + endpoint:
> > > > + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > > > +
> > > > + properties:
> > > > + bus-format:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + minimum: 0x1001
> > > > + maximum: 0x1fff
> > > > + description: |
> > > > + Describes how the display panel is connected to the display interface.
> > > > + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> > > > + The mapping between the color/significance of the panel lines to the
> > > > + parallel data lines are defined in:
> > > > + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
> > > > +
> > >
> > > Last month I had the chance to talk in person about this topic with
> > > Dave, Marek and Max in Dublin.
> > >
> > > My understanding is that this change is addressing a general need, Dave
> > > confirmed me they have a downstream patch for raspberrypi [1].
> > >
> > > From what I could tell the only concern is about the actual encoding of
> > > this `bus-format` property.
> > >
> > > I am personally convinced that a simple enum is the way to go, I think
> > > that Marek proposal is adding complexity and not flexibility (from my
> > > understanding Dave is on the same page, just correct me if I
> > > misunderstood you).
> >
> > Yes I agree with you here.
> >
> > This binding is for the panel, and currently the only path to pass the
> > panel mode to the DPI transmitter is one or more MEDIA_BUS_FMT_* enums
> > in struct drm_display_info *bus_formats.
> >
> > Looking at Marek's comment over DSI and data-lanes, yes both source
> > and sink could advertise a data-lanes property to cover the condition
> > where they aren't wired up in a 1:1 fashion. Reality is that most
> > drivers don't support reordering the lanes - looking at the bindings,
> > only one (msm) documents the use of data-lanes on the host side.
> > rcar_mipi_dsi looks at the number of lanes specified only, and then
> > checks that the number requested by the device is <= the number
> > configured.
> >
> > As I see it, the comparison here is that this "bus-format" property is
> > the equivalent of the data-lanes on the sink, and is the desired
> > number of lanes value passed from sink to source (one integer, not a
> > mapping).
> > If the source can reorder the lanes, then that is a property of the
> > source. This binding is for the sink, and so isn't a reasonable
> > comparison. It also doesn't have to be called "bus-format" on the
> > source, and can take a totally different form.
> > I'll admit that I know data-lane configuration more from CSI2, but
> > within V4L2 it is the node that can support reordering that should
> > have the lanes in a non-incrementing order, and that is normally the
> > SoC rather than the sensor. The same would seem to apply here - it's
> > the SoC that can remap the signals, not the panel.
> >
> > It could be argued that for DPI the panel should only advertise the
> > panel's bit depth for each channel, not the padding. The panel is
> > generic and could handle any wiring/padding options, and it isn't
> > necessarily a simple 16/18/24/32 bit bus representation, just a
> > collection of N wires.
> > Padding and wiring is a function of the DPI transmitter / SoC, or
> > potentially an interconnect node between the two.
>
> Sooo... I'm not sure where to start :-)
>
> I think the trouble when describing the connection between a source and
> a sink in DT is that none of the source or sink is an ideal place to
> describe properties of the connection.
>
> For DSI we have it relatively easy, as we only have to describe the
> number of lanes that are routed on the board and possibly how the lanes
> are rearranged. The former is a value that is common between the source
> and the sink, that's the easiest case, it can be specified in both DT
> nodes. The latter is a bit more complicated, and was solved by allowing
> specifying lane reordering on both the source and the sink. As there is
> typically only one of the two components that will support lane
> reordering (if any), DTs will usually specify a 1:1 mapping on one side,
> and possibly reorder on the other side. If both the source and the sink
> support reordering, setting data-lanes = <1 2> on both sides would lead
> to a different configuration than data-lanes = <2 1>, but both would
> work the same (I'm not sure why anyone would want the latter though).
> There may thus be multiple ways to describe a working setup, but that's
> fine, the complexity is manageable, and any hardware configuration can
> be described.
>
> The nice thing with DSI is that the actual data format doesn't depend on
> the board configuration (provided of course that enough lanes are
> available to sustain the required bandwidth). For DPI, things can be
> more difficult. In the test below, "format" refers to how data bits are
> mapped to hardware lines, similarly in concept to the media bus codes.
>
> I see three different cases at the hardware level:
>
> - Either or both the sink or the source support a single format. This
> means that the side that supports multiple formats will always use the
> same format. If data lines are rearranged, the format output by the
> source may not match the format received by the sink, but the hardware
> configuration of both the sink and the source is effectively fixed to
> system-specific values.
>
> - Both the sink and the source support multiple formats, but only one
> combination of formats is possible with how the data lines are routed.
> This case is very similar to the previous one at the hardware level,
> only one configuration is possible.
>
> - Both the sink and the source support multiple formats, and multiple
> format combinations can lead to working configurations. This isn't an
> uncommon case, there are DPI panels with 24 data lines that can
> support both RGB666 and RGB888.
I see a panel-dpi compatible panel to be a fixed format only.
I.e. The panel itself supports one and only one format. It is the
hardware designers' choice how to connect the panel. I agree that at
a HW level it is possible to connect a panel that supports RGB888 to
18 data lanes in a RGB666 configuration and whatever other combination
you can think of, however once the HW is set the format is fixed.
If a panel can change its format by e.g. strapping pins it is still
in the hardware designers hand to set the format.
Should the panel be able to adapt its format then one would either
need a communication channel from the panel to the source which
transport the information what format the panel decided to use or,
more likely, the panel-dpi driver would need to set the used
format by some communication (GPIO/I2C/SPI...). However both cases
are outside of what panel-simple or panel-dpi can do. Those would
be the panels which need and merit their specific drivers.
>
> At the software level, there are also multiple options:
>
> - Both sides could specify the device configuration in DT, using media
> bus codes or any other set of standard or device-specific properties.
> As this would specify a single configuration, it would map quite fine
> to the first two hardware cases. Each driver would read its own
> properties and configure the device accordingly. There would be no
> need for communication between the drivers at runtime in this case.
In my view of things that format specifies the connection between
the display controller and the panel. I.e. how is data represented
between the two endpoints.
If the controller outputs its data in one format and the panel
expects it in a different format you will unlikely display what
is intended.
So at least for the panel-dpi case it makes more sense that only
one side (the side which has it fixed, i.e. the panel) sets it.
>
> This could also support the third hardware case, but would limit it to
> one of the supported configurations, without allowing the other ones
> to be selected at runtime.
>
> This scheme is similar to data-lanes, in the sense that each side
> reads its own hardcoded configuration from DT. It does however differ
> in that the data format gets hardcoded as well, unlike DSI where the
> data formats needs to be communicated at runtime between the drivers.
> As, like DSI, it requires both sides to specify their hardware
> configuration in DT, interoperability between sources and sinks would
> require all DT bindings for all DPI devices to adhere to this. They
> may not have to specify their configuration using the same set of
> properties, but they would all need to specify it in DT. This would
> thus, I think, lead to a dead end for the third hardware case.
If source and sink would be able to negotiate one of many possible
formats at runtime this can no longer be handled by a panel-simple
provided driver. It would also no longer be a DT responsibility.
I.e. it would go into (optional?) API functionality between the source
and sink.
The HW as described here has the panel connected per
MEDIA_BUS_FMT_RGB666_1X24-CPAD
LO (which does not exist and would be
described as MEDIA_BUS_FMT_RGB888_1X24) and the display controller
supports MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI.
So the display controller would choose MEDIA_BUS_FMT_RGB888_1X24 for
its configuration.
And again, it's the HW designers responsibility to connect the panel
in a way compatible with what the particular display controller can
produce.
So I still see the solution that a panel-dpi in the DT specifies
the format in its endpoint and the display controller does read it
from there as a way forward.
Max
>
> The easy optin is to consider that most use cases are in the first two
> hardware categories, specify the media bus code in DT on both sides, and
> consider that support for the third category can be added later. I'm
> worried that we would then corner ourselves, as explained above, because
> this scheme requires all devices involved to specify their hardcoded
> configuration in DT. Will there then be a path forward that wouldn't
> break the DT ABI ? Even if there was, it would mean that all driver
> would then need to support two sets of DT properties, leading to a
> painful transition period on the driver side.
>
> > > The current proposal is already encoding the exact bit placing as
> > > described in Documentation/userspace-api/media/v4l/subdev-formats.rst [2],
> > > this enumeration can be extended to address any future needs
> > > and I would not invent a new one to define the exact same
> > > things (and using the same enum was also suggested by Rob).
> > >
> > > Marek: you told me that you had some concern about some valid use case
> > > not covered by this solution, would you mind explaining why that would
> > > not be covered with an addition on this enumeration?
> >
> > All the MEDIA_BUS_FMT_* enums are explicitly defined with regard to
> > the colour component bit positions. Should someone be in the position
> > of needing to implement a YUV or similar DPI display, converting these
> > enums into the relevant new structure will be straightforward, so
> > backwards compatibility can be achieved easily.
> >
> > > Any other opinion on this topic? How can we move this forward?
> > >
> > > Francesco
> > >
> > > [1] https://github.com/raspberrypi/linux/commit/8e43f1898191b43aa7ed6e6ca3a4cd28709af86d
> > > [2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html
>
> --
> Regards,
>
> Laurent Pinchart
Hi Laurent
On Sun, 16 Oct 2022 at 02:33, Laurent Pinchart
<[email protected]> wrote:
>
> Hello,
>
> On Fri, Oct 14, 2022 at 03:08:49PM +0100, Dave Stevenson wrote:
> > On Thu, 13 Oct 2022 at 13:58, Francesco Dolcini wrote:
> > > On Tue, Jun 28, 2022 at 08:18:36PM +0200, Max Krummenacher wrote:
> > > > From: Max Krummenacher <[email protected]>
> > > >
> > > > The property is used to set the enum bus_format and infer the bpc
> > > > for a panel defined by 'panel-dpi'.
> > > > This specifies how the panel is connected to the display interface.
> > > >
> > > > Signed-off-by: Max Krummenacher <[email protected]>
> > > >
> > >
> > > <snip>
> > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > > index dae0676b5c6e..52f5db03b6a8 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > > > @@ -26,7 +26,28 @@ properties:
> > > > height-mm: true
> > > > label: true
> > > > panel-timing: true
> > > > - port: true
> > > > +
> > > > + port:
> > > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > > + description:
> > > > + Input port node, receives the panel data.
> > > > +
> > > > + properties:
> > > > + endpoint:
> > > > + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > > > +
> > > > + properties:
> > > > + bus-format:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + minimum: 0x1001
> > > > + maximum: 0x1fff
> > > > + description: |
> > > > + Describes how the display panel is connected to the display interface.
> > > > + Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> > > > + The mapping between the color/significance of the panel lines to the
> > > > + parallel data lines are defined in:
> > > > + https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
> > > > +
> > >
> > > Last month I had the chance to talk in person about this topic with
> > > Dave, Marek and Max in Dublin.
> > >
> > > My understanding is that this change is addressing a general need, Dave
> > > confirmed me they have a downstream patch for raspberrypi [1].
> > >
> > > From what I could tell the only concern is about the actual encoding of
> > > this `bus-format` property.
> > >
> > > I am personally convinced that a simple enum is the way to go, I think
> > > that Marek proposal is adding complexity and not flexibility (from my
> > > understanding Dave is on the same page, just correct me if I
> > > misunderstood you).
> >
> > Yes I agree with you here.
> >
> > This binding is for the panel, and currently the only path to pass the
> > panel mode to the DPI transmitter is one or more MEDIA_BUS_FMT_* enums
> > in struct drm_display_info *bus_formats.
> >
> > Looking at Marek's comment over DSI and data-lanes, yes both source
> > and sink could advertise a data-lanes property to cover the condition
> > where they aren't wired up in a 1:1 fashion. Reality is that most
> > drivers don't support reordering the lanes - looking at the bindings,
> > only one (msm) documents the use of data-lanes on the host side.
> > rcar_mipi_dsi looks at the number of lanes specified only, and then
> > checks that the number requested by the device is <= the number
> > configured.
> >
> > As I see it, the comparison here is that this "bus-format" property is
> > the equivalent of the data-lanes on the sink, and is the desired
> > number of lanes value passed from sink to source (one integer, not a
> > mapping).
> > If the source can reorder the lanes, then that is a property of the
> > source. This binding is for the sink, and so isn't a reasonable
> > comparison. It also doesn't have to be called "bus-format" on the
> > source, and can take a totally different form.
> > I'll admit that I know data-lane configuration more from CSI2, but
> > within V4L2 it is the node that can support reordering that should
> > have the lanes in a non-incrementing order, and that is normally the
> > SoC rather than the sensor. The same would seem to apply here - it's
> > the SoC that can remap the signals, not the panel.
> >
> > It could be argued that for DPI the panel should only advertise the
> > panel's bit depth for each channel, not the padding. The panel is
> > generic and could handle any wiring/padding options, and it isn't
> > necessarily a simple 16/18/24/32 bit bus representation, just a
> > collection of N wires.
> > Padding and wiring is a function of the DPI transmitter / SoC, or
> > potentially an interconnect node between the two.
>
> Sooo... I'm not sure where to start :-)
>
> I think the trouble when describing the connection between a source and
> a sink in DT is that none of the source or sink is an ideal place to
> describe properties of the connection.
>
> For DSI we have it relatively easy, as we only have to describe the
> number of lanes that are routed on the board and possibly how the lanes
> are rearranged. The former is a value that is common between the source
> and the sink, that's the easiest case, it can be specified in both DT
> nodes. The latter is a bit more complicated, and was solved by allowing
> specifying lane reordering on both the source and the sink. As there is
> typically only one of the two components that will support lane
> reordering (if any), DTs will usually specify a 1:1 mapping on one side,
> and possibly reorder on the other side. If both the source and the sink
> support reordering, setting data-lanes = <1 2> on both sides would lead
> to a different configuration than data-lanes = <2 1>, but both would
> work the same (I'm not sure why anyone would want the latter though).
> There may thus be multiple ways to describe a working setup, but that's
> fine, the complexity is manageable, and any hardware configuration can
> be described.
>
> The nice thing with DSI is that the actual data format doesn't depend on
> the board configuration (provided of course that enough lanes are
> available to sustain the required bandwidth). For DPI, things can be
> more difficult. In the test below, "format" refers to how data bits are
> mapped to hardware lines, similarly in concept to the media bus codes.
>
> I see three different cases at the hardware level:
>
> - Either or both the sink or the source support a single format. This
> means that the side that supports multiple formats will always use the
> same format. If data lines are rearranged, the format output by the
> source may not match the format received by the sink, but the hardware
> configuration of both the sink and the source is effectively fixed to
> system-specific values.
>
> - Both the sink and the source support multiple formats, but only one
> combination of formats is possible with how the data lines are routed.
> This case is very similar to the previous one at the hardware level,
> only one configuration is possible.
>
> - Both the sink and the source support multiple formats, and multiple
> format combinations can lead to working configurations. This isn't an
> uncommon case, there are DPI panels with 24 data lines that can
> support both RGB666 and RGB888.
>
> At the software level, there are also multiple options:
>
> - Both sides could specify the device configuration in DT, using media
> bus codes or any other set of standard or device-specific properties.
> As this would specify a single configuration, it would map quite fine
> to the first two hardware cases. Each driver would read its own
> properties and configure the device accordingly. There would be no
> need for communication between the drivers at runtime in this case.
>
> This could also support the third hardware case, but would limit it to
> one of the supported configurations, without allowing the other ones
> to be selected at runtime.
>
> This scheme is similar to data-lanes, in the sense that each side
> reads its own hardcoded configuration from DT. It does however differ
> in that the data format gets hardcoded as well, unlike DSI where the
> data formats needs to be communicated at runtime between the drivers.
> As, like DSI, it requires both sides to specify their hardware
> configuration in DT, interoperability between sources and sinks would
> require all DT bindings for all DPI devices to adhere to this. They
> may not have to specify their configuration using the same set of
> properties, but they would all need to specify it in DT. This would
> thus, I think, lead to a dead end for the third hardware case.
>
> - The two sides could communicate at runtime to dynamically negotiate
> their configuration. Some form of runtime configuration is required to
> fully support the third hardware case, and it could also support the
> other two cases.
>
> The trouble here, beside how to express the required data in DT, is
> how that communication would be handled. Let's consider a case where
> data lines are "remapped":
>
> - The display controller that has a D[23:0] output bus
> - The panel that has a D[17:0] bus
> - The data lines connections from the display controller to the panel
> are D[23:18] -> D[17:12], D[15:10] -> D[11:6], D[7:2] -> D[5:0],
> with the display controller's D[17:16], D[9:8] and D[1:0] outputs
> unconnected
> - The panel only supports RGB666
> - The display controller supports both RGB888 and RGB666, and outputs
> RGB666 as 00RRRRRR00GGGGGG00BBBBBB
Note that you're also effectively advocating for dropping all BGRxxx
variants from panel definitions, as well as the CPAD variants. All of
those would be handled in the wiring description. Not an issue, just
worth flagging.
> This means that the only possibly configuration is the display
> controller outputting RGB888 and the panel receiving RGB666. If we
> expressed that as media bus codes in DT, the panel would would
> communicate its RGB666 input format to the display controller, which
> wouldn't know that it would have to output RGB888.
>
> Of course, in this particular example, only one hardware configuration
> is possible, so we could support it by specifying the media bus code
> in both DT nodes, but that won't scale to cases where multiple
> configurations are possible.
I'll agree that the existing framework is broken in many cases, but
it's broken for ALL panel drivers, not just panel-dpi.
Any of the current drivers with specific compatible strings that
provide two or more bus formats have no mechanism for negotiating or
being told which has been chosen.
Any mapping that isn't 1:1 can't be represented.
This binding change for dpi-panel is just to bring it up to the same
level of brokenness as all other panels.
Yes it's DT ABI, but almost all the issues raised are about other brokenness.
> The easy optin is to consider that most use cases are in the first two
> hardware categories, specify the media bus code in DT on both sides, and
> consider that support for the third category can be added later. I'm
> worried that we would then corner ourselves, as explained above, because
> this scheme requires all devices involved to specify their hardcoded
> configuration in DT.
>
> Will there then be a path forward that wouldn't
> break the DT ABI ? Even if there was, it would mean that all driver
> would then need to support two sets of DT properties, leading to a
> painful transition period on the driver side.
Very few displays need to read their format from DT - dpi-panel is the
only one as it allows a level of configuration from DT.
If the panel driver has a defined compatible string, then the
format(s) is/are in the code and not DT.
Once you get beyond the one entry in DT, it's an internal kernel API
and therefore can be reworked at a later date.
If and when someone needs to extend DPI transmitters to read DT for
simple mappings, then create and use a helper function to do the
parsing. Currently the helper would return a MEDIA_BUS_FMT_ code, but
internally it can have gone through the correct sets of hoops to
either read DT, follow the bridge chain with
atomic_get_input_bus_fmts, or otherwise find the panel to get a
format. (This assumes that there is one true and correct method for
getting the format which is implemented by all drivers).
Should there be a situation further down the road where there is a
need to extend that further, then a new helper function needs to be
added that is able to read either DT API and convert between them. The
existing helper will also need updating to support conversion from the
new DT API.
Converting from a "bus-format" to a more complete description for a
new driver should be trivial with a lookup table. Converting from a
new DT API to a single mbus format should be possible (reverse the
previous lookup table), but you have to accept that there are new DT
configurations that can not be expressed by mbus format, and therefore
that the helper function can return an error.
DPI transmitter drivers can be converted to the new helper as
required, or potentially if they can't support arbitrary routing then
they stay on the old helper forever as it will work both new and old
bindings.
Does this one patch for one driver really back ourselves into a corner
that can't be handled?
The one case I can think of as potentially an issue is if *bus_formats
is removed totally from struct drm_display_info to be replaced by
something else more generic. Under that situation you may need to
match up an existing DT file using bus-format for a padded or BGR
format with a brave new world where the panel advertises a DPI
descriptor that is normally 1:1 and the DPI transmitter has the
mapping. Even then I think is is possible as the DPI transmitter won't
have a mapping in the old DT. It will then be looking at the
configuration provided by the panel which will be one of the standard
table lookups for an MEDIA_BUS_FMT_xxx_, and that gets us back to the
same configuration as before.
Thanks.
Dave
> > > The current proposal is already encoding the exact bit placing as
> > > described in Documentation/userspace-api/media/v4l/subdev-formats.rst [2],
> > > this enumeration can be extended to address any future needs
> > > and I would not invent a new one to define the exact same
> > > things (and using the same enum was also suggested by Rob).
> > >
> > > Marek: you told me that you had some concern about some valid use case
> > > not covered by this solution, would you mind explaining why that would
> > > not be covered with an addition on this enumeration?
> >
> > All the MEDIA_BUS_FMT_* enums are explicitly defined with regard to
> > the colour component bit positions. Should someone be in the position
> > of needing to implement a YUV or similar DPI display, converting these
> > enums into the relevant new structure will be straightforward, so
> > backwards compatibility can be achieved easily.
> >
> > > Any other opinion on this topic? How can we move this forward?
> > >
> > > Francesco
> > >
> > > [1] https://github.com/raspberrypi/linux/commit/8e43f1898191b43aa7ed6e6ca3a4cd28709af86d
> > > [2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html
>
> --
> Regards,
>
> Laurent Pinchart