[PATCH 0/8] drm: bridge: Add support for static image formats

[PATCH 0/8] drm: bridge: Add support for static image formats

[Jacopo Mondi]

Hello DRM list,
  cc media-list for the mbus format extension
  cc renesas-soc and devicetree for Eagle DTS patch

This series adds support for static image formats to DRM bridges, mimicking
what display_info.bus_formats represents for DRM connectors.

The main use case of this series is the R-Car DU LVDS encoder. This component
can output LVDS streams compatible with JEIDA or SPWG specification, and so far
it has only been possible to decide which one to output if the next component
in the DRM pipeline was a panel, equipped with a DRM connector where to inspect
the accepted input image format from.

With the introduction of the transparent THC63LVD1024 LVDS decoder driver, the
next component in the pipeline is now a bridge, and the DU LVDS encoder needs
to inspect which media bus image formats it accepts and set its LVDS output
mode accordingly.

The series implements -static- image format supports for bridges. As a result
of the discussion on Peter Rosin's patch series:
[PATCH v2 0/5] allow override of bus format in bridges
https://lkml.org/lkml/2018/3/26/610
my understanding is that the accepted image formats can be 'dynamic' (or
'atomic') if depends on the configured DRM mode, or 'static' as in the
THC63LVD1024 case, where an external pin configuration decides which LVDS
mapping mode the encoder accepts (which implies it comes from DT, as in Peter's
use case).

For dynamic formats Daniel already suggested a possible implementation:
https://lkml.org/lkml/2018/3/28/57
while for static image formats I am proposing an implementation that
copies what we have for connectors at the moment.

One more detail: the DU LVDS encoder supports 'mirroring' of LVDS modes, and
that was handled through a set of flags (DRM_BUS_FLAG_DATA_*) defined for
connectors only, and added for that specific purpose, if I got this right.
Instead of replicating the same flags for bridges, or moving them to some shared
header which I had trouble to identify, I have introduced the _LE version of
mbus formats used to describe LVDS streams. While 'little endian' is not
*technically* exact for those mirrored formats, it is my opinion they represent
a good description anyhow of the reversed component ordering.

As a result, the connector specific DRM_BUS_FLAG_DATA_* have been removed as
all their user have been ported to use the new _LE formats.

The series depends on THC63LVD1024 support, implemented by the following
in-review series:
 [PATCH v9 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge
 [PATCH v3 0/5] V3M-Eagle HDMI output enablement

available for the interested at:
git://jmondi.org/linux lvds-bridge/linus-master/v9-eagle-v3

Thanks for comments
   j

Jacopo Mondi (8):
  drm: bridge: Add support for static image formats
  dt-bindings: display: bridge: thc63lvd1024: Add lvds map property
  drm: bridge: thc63lvd1024: Add support for LVDS mode map
  arm64: dts: renesas: eagle: Add thc63 LVDS map
  media: Add LE version of RGB LVDS formats
  drm: rcar-du: rcar-lvds: Add bridge format support
  drm: panel: Use _LE LVDS formats for data mirroring
  drm: connector: Remove DRM_BUS_FLAG_DATA_* flags

 .../bindings/display/bridge/thine,thc63lvd1024.txt |   3 +
 Documentation/media/uapi/v4l/subdev-formats.rst    | 174 +++++++++++++++++++++
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c              |  41 +++++
 drivers/gpu/drm/drm_bridge.c                       |  30 ++++
 drivers/gpu/drm/panel/panel-lvds.c                 |  21 +--
 drivers/gpu/drm/rcar-du/rcar_lvds.c                |  64 +++++---
 include/drm/drm_bridge.h                           |   8 +
 include/drm/drm_connector.h                        |   4 -
 include/uapi/linux/media-bus-format.h              |   5 +-
 10 files changed, 317 insertions(+), 34 deletions(-)

--
2.7.4

[PATCH 1/8] drm: bridge: Add support for static image formats

[Jacopo Mondi]

Add support for storing image format information in DRM bridges with
associated helper function.

This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
is for connectors.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  8 ++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe..e2ad098 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 }
 
 /**
+ * drm_bridge_set_bus_formats() - set bridge supported image formats
+ * @bridge: the bridge to set image formats in
+ * @formats: array of MEDIA_BUS_FMT\_ supported image formats
+ * @num_formats: number of elements in the @formats array
+ *
+ * Store a list of supported image formats in a bridge.
+ * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for
+ * a full list of available formats.
+ */
+int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *formats,
+			       unsigned int num_formats)
+{
+	u32 *fmts;
+
+	if (!formats || !num_formats)
+		return -EINVAL;
+
+	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
+	if (!fmts)
+		return -ENOMEM;
+
+	kfree(bridge->bus_formats);
+	bridge->bus_formats = fmts;
+	bridge->num_bus_formats = num_formats;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_bridge_set_bus_formats);
+
+/**
  * DOC: bridge callbacks
  *
  * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec..6b3648c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,6 +258,9 @@ struct drm_bridge_timings {
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
  * @of_node: device node pointer to the bridge
+ * @bus_formats: wire image formats. Array of @num_bus_formats MEDIA_BUS_FMT\_
+ * elements
+ * @num_bus_formats: size of @bus_formats array
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -271,6 +274,9 @@ struct drm_bridge {
 #ifdef CONFIG_OF
 	struct device_node *of_node;
 #endif
+	const u32 *bus_formats;
+	unsigned int num_bus_formats;
+
 	struct list_head list;
 	const struct drm_bridge_timings *timings;
 
@@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
 			struct drm_display_mode *adjusted_mode);
 void drm_bridge_pre_enable(struct drm_bridge *bridge);
 void drm_bridge_enable(struct drm_bridge *bridge);
+int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
+			       unsigned int num_fmts);
 
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
-- 
2.7.4

[PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Jacopo Mondi]

The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
modes, selectable by means of an external pin.

Describe the LVDS mode map through a newly defined mandatory property in
device tree bindings.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
index 37f0c04..0937595 100644
--- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -12,6 +12,8 @@ Required properties:
 - compatible: Shall be "thine,thc63lvd1024"
 - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
   PPL and digital circuitry
+- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be <1>
+  for mapping mode 1, <0> for mapping mode 2
 
 Optional properties:
 - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
@@ -36,6 +38,7 @@ Example:
 
 		vcc-supply = <&reg_lvds_vcc>;
 		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
+		thine,map = <1>;
 
 		ports {
 			#address-cells = <1>;
-- 
2.7.4

[PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Jacopo Mondi]

The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
modes, selectable by means of an external pin.

Add support for configurable LVDS input mapping modes, using the newly
introduced support for bridge input image formats.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 48527f8..a3071a1 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -10,9 +10,15 @@
 #include <drm/drm_panel.h>
 
 #include <linux/gpio/consumer.h>
+#include <linux/of.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 
+enum thc63_lvds_mapping_mode {
+	THC63_LVDS_MAP_MODE2,
+	THC63_LVDS_MAP_MODE1,
+};
+
 enum thc63_ports {
 	THC63_LVDS_IN0,
 	THC63_LVDS_IN1,
@@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
 	return 0;
 }
 
+static int thc63_set_bus_fmt(struct thc63_dev *thc63)
+{
+	u32 bus_fmt;
+	u32 map;
+	int ret;
+
+	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
+	if (ret) {
+		dev_err(thc63->dev,
+			"Unable to parse property \"thine,map\": %d\n", ret);
+		return ret;
+	}
+
+	switch (map) {
+	case THC63_LVDS_MAP_MODE1:
+		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
+		break;
+	case THC63_LVDS_MAP_MODE2:
+		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
+		break;
+	default:
+		dev_err(thc63->dev,
+			"Invalid value for property \"thine,map\": %u\n", map);
+		return -EINVAL;
+	}
+
+	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
+
+	return 0;
+}
+
 static int thc63_gpio_init(struct thc63_dev *thc63)
 {
 	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
@@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = thc63_set_bus_fmt(thc63);
+	if (ret)
+		return ret;
+
 	thc63->bridge.driver_private = thc63;
 	thc63->bridge.of_node = pdev->dev.of_node;
 	thc63->bridge.funcs = &thc63_bridge_func;
-- 
2.7.4

[PATCH 4/8] arm64: dts: renesas: eagle: Add thc63 LVDS map

[Jacopo Mondi]

Add LVDS map mode description property to THC63LVD1024 LVDS decoder in
R-Car V3M-Eagle board device tree.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index ebfbb51..2609fa3 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -56,6 +56,7 @@
 		compatible = "thine,thc63lvd1024";
 
 		vcc-supply = <&d3p3>;
+		thine,map = <1>;
 
 		ports {
 			#address-cells = <1>;
-- 
2.7.4

[PATCH 5/8] media: Add LE version of RGB LVDS formats

[Jacopo Mondi]

Some LVDS controller can output swapped versions of LVDS RGB formats.
Define and document them in the list of supported media bus formats

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/media/uapi/v4l/subdev-formats.rst | 174 ++++++++++++++++++++++++
 include/uapi/linux/media-bus-format.h           |   5 +-
 2 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst
index 9fcabe7..9a5263c 100644
--- a/Documentation/media/uapi/v4l/subdev-formats.rst
+++ b/Documentation/media/uapi/v4l/subdev-formats.rst
@@ -1669,6 +1669,64 @@ JEIDA defined bit mapping will be named
       - b\ :sub:`2`
       - g\ :sub:`1`
       - r\ :sub:`0`
+    * .. _MEDIA-BUS-FMT-RGB666-1X7X3-SPWG_LE:
+
+      - MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE
+      - 0x101b
+      - 0
+      -
+      -
+      - b\ :sub:`2`
+      - g\ :sub:`1`
+      - r\ :sub:`0`
+    * -
+      -
+      - 1
+      -
+      -
+      - b\ :sub:`3`
+      - g\ :sub:`2`
+      - r\ :sub:`1`
+    * -
+      -
+      - 2
+      -
+      -
+      - b\ :sub:`4`
+      - g\ :sub:`3`
+      - r\ :sub:`2`
+    * -
+      -
+      - 3
+      -
+      -
+      - b\ :sub:`5`
+      - g\ :sub:`4`
+      - r\ :sub:`3`
+    * -
+      -
+      - 4
+      -
+      -
+      - d
+      - g\ :sub:`5`
+      - r\ :sub:`4`
+    * -
+      -
+      - 5
+      -
+      -
+      - d
+      - b\ :sub:`0`
+      - r\ :sub:`5`
+    * -
+      -
+      - 6
+      -
+      -
+      - d
+      - b\ :sub:`1`
+      - g\ :sub:`0`
     * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG:
 
       - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG
@@ -1727,6 +1785,64 @@ JEIDA defined bit mapping will be named
       - b\ :sub:`2`
       - g\ :sub:`1`
       - r\ :sub:`0`
+    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG_LE:
+
+      - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE
+      - 0x101c
+      - 0
+      -
+      - r\ :sub:`6`
+      - b\ :sub:`2`
+      - g\ :sub:`1`
+      - r\ :sub:`0`
+    * -
+      -
+      - 1
+      -
+      - r\ :sub:`7`
+      - b\ :sub:`3`
+      - g\ :sub:`2`
+      - r\ :sub:`1`
+    * -
+      -
+      - 2
+      -
+      - g\ :sub:`6`
+      - b\ :sub:`4`
+      - g\ :sub:`3`
+      - r\ :sub:`2`
+    * -
+      -
+      - 3
+      -
+      - g\ :sub:`7`
+      - b\ :sub:`5`
+      - g\ :sub:`4`
+      - r\ :sub:`3`
+    * -
+      -
+      - 4
+      -
+      - b\ :sub:`6`
+      - d
+      - g\ :sub:`5`
+      - r\ :sub:`4`
+    * -
+      -
+      - 5
+      -
+      - b\ :sub:`7`
+      - d
+      - b\ :sub:`0`
+      - r\ :sub:`5`
+    * -
+      -
+      - 6
+      -
+      - d
+      - d
+      - b\ :sub:`1`
+      - g\ :sub:`0`
     * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA:
 
       - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA
@@ -1785,6 +1901,64 @@ JEIDA defined bit mapping will be named
       - b\ :sub:`4`
       - g\ :sub:`3`
       - r\ :sub:`2`
+    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA_LE:
+
+      - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE
+      - 0x101d
+      - 0
+      -
+      - r\ :sub:`0`
+      - b\ :sub:`4`
+      - g\ :sub:`3`
+      - r\ :sub:`2`
+    * -
+      -
+      - 1
+      -
+      - r\ :sub:`1`
+      - b\ :sub:`5`
+      - g\ :sub:`4`
+      - r\ :sub:`3`
+    * -
+      -
+      - 2
+      -
+      - g\ :sub:`0`
+      - b\ :sub:`6`
+      - g\ :sub:`5`
+      - r\ :sub:`4`
+    * -
+      -
+      - 3
+      -
+      - g\ :sub:`1`
+      - b\ :sub:`7`
+      - g\ :sub:`6`
+      - r\ :sub:`5`
+    * -
+      -
+      - 4
+      -
+      - b\ :sub:`0`
+      - d
+      - g\ :sub:`7`
+      - r\ :sub:`6`
+    * -
+      -
+      - 5
+      -
+      - b\ :sub:`1`
+      - d
+      - b\ :sub:`2`
+      - r\ :sub:`7`
+    * -
+      -
+      - 6
+      -
+      - d
+      - d
+      - b\ :sub:`3`
+      - g\ :sub:`2`
 
 .. raw:: latex
 
diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
index 9e35117..5bea7c0 100644
--- a/include/uapi/linux/media-bus-format.h
+++ b/include/uapi/linux/media-bus-format.h
@@ -34,7 +34,7 @@
 
 #define MEDIA_BUS_FMT_FIXED			0x0001
 
-/* RGB - next is	0x101b */
+/* RGB - next is	0x101f */
 #define MEDIA_BUS_FMT_RGB444_1X12		0x1016
 #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE	0x1001
 #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE	0x1002
@@ -49,13 +49,16 @@
 #define MEDIA_BUS_FMT_RBG888_1X24		0x100e
 #define MEDIA_BUS_FMT_RGB666_1X24_CPADHI	0x1015
 #define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG		0x1010
+#define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE	0x101b
 #define MEDIA_BUS_FMT_BGR888_1X24		0x1013
 #define MEDIA_BUS_FMT_GBR888_1X24		0x1014
 #define MEDIA_BUS_FMT_RGB888_1X24		0x100a
 #define MEDIA_BUS_FMT_RGB888_2X12_BE		0x100b
 #define MEDIA_BUS_FMT_RGB888_2X12_LE		0x100c
 #define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG		0x1011
+#define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE	0x101c
 #define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA	0x1012
+#define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE	0x101d
 #define MEDIA_BUS_FMT_ARGB8888_1X32		0x100d
 #define MEDIA_BUS_FMT_RGB888_1X32_PADHI		0x100f
 #define MEDIA_BUS_FMT_RGB101010_1X30		0x1018
-- 
2.7.4

[PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Jacopo Mondi]

With the introduction of static input image format enumeration in DRM
bridges, add support to retrieve the format in rcar-lvds LVDS encoder
from both panel or bridge, to set the desired LVDS mode.

Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
format, as it is only defined for drm connectors, but use the newly
introduced _LE version of LVDS mbus image formats.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bb..2fa875f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
 	return true;
 }
 
-static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
+static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
+						  unsigned int *bus_fmt)
 {
 	struct drm_display_info *info = &lvds->connector.display_info;
-	enum rcar_lvds_mode mode;
-
-	/*
-	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
-	 * are supported.
-	 */
-	if (!lvds->panel)
-		return;
 
 	if (!info->num_bus_formats || !info->bus_formats) {
 		dev_err(lvds->dev, "no LVDS bus format reported\n");
-		return;
+		return -EINVAL;
+	}
+
+	*bus_fmt = info->bus_formats[0];
+
+	return 0;
+}
+
+static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
+					       unsigned int *bus_fmt)
+{
+	if (!lvds->next_bridge->num_bus_formats ||
+	    !lvds->next_bridge->bus_formats) {
+		dev_err(lvds->dev, "no LVDS bus format reported\n");
+		return -EINVAL;
 	}
 
-	switch (info->bus_formats[0]) {
+	*bus_fmt = lvds->next_bridge->bus_formats[0];
+
+	return 0;
+}
+
+static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
+{
+	unsigned int bus_fmt;
+	int ret;
+
+	if (lvds->panel)
+		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
+	else
+		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
+	if (ret)
+		return;
+
+	switch (bus_fmt) {
+	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
+	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
+		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
 	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
 	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
-		mode = RCAR_LVDS_MODE_JEIDA;
+		lvds->mode = RCAR_LVDS_MODE_JEIDA;
 		break;
+
+	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
+		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
 	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
-		mode = RCAR_LVDS_MODE_VESA;
+		lvds->mode = RCAR_LVDS_MODE_VESA;
 		break;
 	default:
 		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
-			info->bus_formats[0]);
-		return;
+			bus_fmt);
 	}
-
-	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
-		mode |= RCAR_LVDS_MODE_MIRROR;
-
-	lvds->mode = mode;
 }
 
 static void rcar_lvds_mode_set(struct drm_bridge *bridge,
-- 
2.7.4

[PATCH 7/8] drm: panel: Use _LE LVDS formats for data mirroring

[Jacopo Mondi]

As now both bridges and panels report supported image formats,
use the newly introduced _LE version of LVDS media bus formats in place
of the DRM_BUS_FLAG_DATA_ flags defined in drm_connector.h

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/panel/panel-lvds.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index 5185819..ac03eab 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -37,7 +37,6 @@ struct panel_lvds {
 	unsigned int height;
 	struct videomode video_mode;
 	unsigned int bus_format;
-	bool data_mirror;
 
 	struct backlight_device *backlight;
 	struct regulator *supply;
@@ -129,9 +128,6 @@ static int panel_lvds_get_modes(struct drm_panel *panel)
 	connector->display_info.height_mm = lvds->height;
 	drm_display_info_set_bus_formats(&connector->display_info,
 					 &lvds->bus_format, 1);
-	connector->display_info.bus_flags = lvds->data_mirror
-					  ? DRM_BUS_FLAG_DATA_LSB_TO_MSB
-					  : DRM_BUS_FLAG_DATA_MSB_TO_LSB;
 
 	return 1;
 }
@@ -149,6 +145,7 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
 	struct device_node *np = lvds->dev->of_node;
 	struct display_timing timing;
 	const char *mapping;
+	bool data_mirror;
 	int ret;
 
 	ret = of_get_display_timing(np, "panel-timing", &timing);
@@ -179,20 +176,26 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
 		return -ENODEV;
 	}
 
+	data_mirror = of_property_read_bool(np, "data-mirror");
+
 	if (!strcmp(mapping, "jeida-18")) {
-		lvds->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
+		lvds->bus_format = data_mirror ?
+				   MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE :
+				   MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
 	} else if (!strcmp(mapping, "jeida-24")) {
-		lvds->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
+		lvds->bus_format = data_mirror ?
+				   MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE :
+				   MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
 	} else if (!strcmp(mapping, "vesa-24")) {
-		lvds->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
+		lvds->bus_format = data_mirror ?
+				   MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE :
+				   MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
 	} else {
 		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
 			np, "data-mapping");
 		return -EINVAL;
 	}
 
-	lvds->data_mirror = of_property_read_bool(np, "data-mirror");
-
 	return 0;
 }
 
-- 
2.7.4

[PATCH 8/8] drm: connector: Remove DRM_BUS_FLAG_DATA_* flags

[Jacopo Mondi]

DRM_BUS_FLAG_DATA_* flags, defined in drm_connector.h header file are
used to swap ordering of LVDS RGB format to accommodate DRM objects
that need to handle LVDS components ordering.

Now that the only 2 users of DRM_BUS_FLAG_DATA_* flags have been ported
to use the newly introduced MEDIA_BUS_FMT_RGB888_1X7X*_LE media bus
formats, remove them.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/drm/drm_connector.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 675cc3f..9e0d6d5 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -286,10 +286,6 @@ struct drm_display_info {
 #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
 /* drive data on neg. edge */
 #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
-/* data is transmitted MSB to LSB on the bus */
-#define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
-/* data is transmitted LSB to MSB on the bus */
-#define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
 
 	/**
 	 * @bus_flags: Additional information (like pixel signal polarity) for
-- 
2.7.4

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> Add support for storing image format information in DRM bridges with
> associated helper function.
> 
> This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> is for connectors.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  8 ++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe..e2ad098 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  }
>  
>  /**
> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> + * @bridge: the bridge to set image formats in
> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> + * @num_formats: number of elements in the @formats array
> + *
> + * Store a list of supported image formats in a bridge.
> + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for
> + * a full list of available formats.
> + */
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *formats,
> +			       unsigned int num_formats)
> +{
> +	u32 *fmts;
> +
> +	if (!formats || !num_formats)
> +		return -EINVAL;

I see no compelling reason to forbid restoring the number of reported
input formats to zero? I can't think of a use right now of course, but it
seems a bit odd all the same.

Cheers,
Peter

> +
> +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> +	if (!fmts)
> +		return -ENOMEM;
> +
> +	kfree(bridge->bus_formats);
> +	bridge->bus_formats = fmts;
> +	bridge->num_bus_formats = num_formats;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> +
> +/**
>   * DOC: bridge callbacks
>   *
>   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec..6b3648c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
>   * @of_node: device node pointer to the bridge
> + * @bus_formats: wire image formats. Array of @num_bus_formats MEDIA_BUS_FMT\_
> + * elements
> + * @num_bus_formats: size of @bus_formats array
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> @@ -271,6 +274,9 @@ struct drm_bridge {
>  #ifdef CONFIG_OF
>  	struct device_node *of_node;
>  #endif
> +	const u32 *bus_formats;
> +	unsigned int num_bus_formats;
> +
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
>  
> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>  			struct drm_display_mode *adjusted_mode);
>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>  void drm_bridge_enable(struct drm_bridge *bridge);
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> +			       unsigned int num_fmts);
>  
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> 

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> modes, selectable by means of an external pin.
> 
> Describe the LVDS mode map through a newly defined mandatory property in
> device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> index 37f0c04..0937595 100644
> --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -12,6 +12,8 @@ Required properties:
>  - compatible: Shall be "thine,thc63lvd1024"
>  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
>    PPL and digital circuitry
> +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be <1>
> +  for mapping mode 1, <0> for mapping mode 2

Since the MAP pin is an input pin, I would expect there to be an optional gpio
specifier like thine,map-gpios so that the driver can set it according to
the value given in thine,map in case the HW has a line from some gpio output
to the MAP pin (instead of hardwired hi/low which seem to be your thinking).

Cheers,
Peter

>  
>  Optional properties:
>  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> @@ -36,6 +38,7 @@ Example:
>  
>  		vcc-supply = <&reg_lvds_vcc>;
>  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +		thine,map = <1>;
>  
>  		ports {
>  			#address-cells = <1>;
> 

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> modes, selectable by means of an external pin.
> 
> Add support for configurable LVDS input mapping modes, using the newly
> introduced support for bridge input image formats.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index 48527f8..a3071a1 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -10,9 +10,15 @@
>  #include <drm/drm_panel.h>
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/of.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
>  
> +enum thc63_lvds_mapping_mode {
> +	THC63_LVDS_MAP_MODE2,
> +	THC63_LVDS_MAP_MODE1,
> +};
> +
>  enum thc63_ports {
>  	THC63_LVDS_IN0,
>  	THC63_LVDS_IN1,
> @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>  	return 0;
>  }
>  
> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> +{
> +	u32 bus_fmt;
> +	u32 map;
> +	int ret;
> +
> +	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
> +	if (ret) {
> +		dev_err(thc63->dev,
> +			"Unable to parse property \"thine,map\": %d\n", ret);
> +		return ret;
> +	}
> +
> +	switch (map) {
> +	case THC63_LVDS_MAP_MODE1:
> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> +		break;
> +	case THC63_LVDS_MAP_MODE2:
> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;

Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
or rgb101010/1x7x5, no?

Cheers,
Peter

> +		break;
> +	default:
> +		dev_err(thc63->dev,
> +			"Invalid value for property \"thine,map\": %u\n", map);
> +		return -EINVAL;
> +	}
> +
> +	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
> +
> +	return 0;
> +}
> +
>  static int thc63_gpio_init(struct thc63_dev *thc63)
>  {
>  	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = thc63_set_bus_fmt(thc63);
> +	if (ret)
> +		return ret;
> +
>  	thc63->bridge.driver_private = thc63;
>  	thc63->bridge.of_node = pdev->dev.of_node;
>  	thc63->bridge.funcs = &thc63_bridge_func;
> 

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> With the introduction of static input image format enumeration in DRM
> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> from both panel or bridge, to set the desired LVDS mode.
> 
> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> format, as it is only defined for drm connectors, but use the newly
> introduced _LE version of LVDS mbus image formats.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3d2d3bb..2fa875f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
>  	return true;
>  }
>  
> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
> +						  unsigned int *bus_fmt)
>  {
>  	struct drm_display_info *info = &lvds->connector.display_info;
> -	enum rcar_lvds_mode mode;
> -
> -	/*
> -	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
> -	 * are supported.
> -	 */
> -	if (!lvds->panel)
> -		return;
>  
>  	if (!info->num_bus_formats || !info->bus_formats) {
>  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> -		return;
> +		return -EINVAL;
> +	}
> +
> +	*bus_fmt = info->bus_formats[0];
> +
> +	return 0;
> +}
> +
> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
> +					       unsigned int *bus_fmt)
> +{
> +	if (!lvds->next_bridge->num_bus_formats ||
> +	    !lvds->next_bridge->bus_formats) {
> +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> +		return -EINVAL;
>  	}
>  
> -	switch (info->bus_formats[0]) {
> +	*bus_fmt = lvds->next_bridge->bus_formats[0];

What makes the first reported format the best choice?

> +
> +	return 0;
> +}
> +
> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> +{
> +	unsigned int bus_fmt;
> +	int ret;
> +
> +	if (lvds->panel)
> +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> +	else
> +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);

What if no bridge reports any format, shouldn't the connector be examined
then?

> +	if (ret)
> +		return;
> +
> +	switch (bus_fmt) {
> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
>  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
>  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> -		mode = RCAR_LVDS_MODE_JEIDA;
> +		lvds->mode = RCAR_LVDS_MODE_JEIDA;

This is b0rken, first the mirror bit is ORed into some unknown preexisting
value, then the code falls through (without any fall through comment, btw)
and forcibly sets the mode, thus discarding the mirror bit which was
carefully ORed in.

>  		break;
> +
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
>  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> -		mode = RCAR_LVDS_MODE_VESA;
> +		lvds->mode = RCAR_LVDS_MODE_VESA;

Dito.

Cheers,
Peter

>  		break;
>  	default:
>  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> -			info->bus_formats[0]);
> -		return;
> +			bus_fmt);
>  	}
> -
> -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> -		mode |= RCAR_LVDS_MODE_MIRROR;
> -
> -	lvds->mode = mode;
>  }
>  
>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> 

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5044 bytes --]

Hi Peter,
   thanks for looking into this

On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > With the introduction of static input image format enumeration in DRM
> > bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> > from both panel or bridge, to set the desired LVDS mode.
> >
> > Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> > format, as it is only defined for drm connectors, but use the newly
> > introduced _LE version of LVDS mbus image formats.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
> >  1 file changed, 44 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3d2d3bb..2fa875f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> >  	return true;
> >  }
> >
> > -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> > +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
> > +						  unsigned int *bus_fmt)
> >  {
> >  	struct drm_display_info *info = &lvds->connector.display_info;
> > -	enum rcar_lvds_mode mode;
> > -
> > -	/*
> > -	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
> > -	 * are supported.
> > -	 */
> > -	if (!lvds->panel)
> > -		return;
> >
> >  	if (!info->num_bus_formats || !info->bus_formats) {
> >  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> > -		return;
> > +		return -EINVAL;
> > +	}
> > +
> > +	*bus_fmt = info->bus_formats[0];
> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
> > +					       unsigned int *bus_fmt)
> > +{
> > +	if (!lvds->next_bridge->num_bus_formats ||
> > +	    !lvds->next_bridge->bus_formats) {
> > +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> > +		return -EINVAL;
> >  	}
> >
> > -	switch (info->bus_formats[0]) {
> > +	*bus_fmt = lvds->next_bridge->bus_formats[0];
>
> What makes the first reported format the best choice?

It already was the selection 'policy' in place in this driver before
introducing bridge formats. As you can see from the switch I have here
removed, the first format was selected even when only the format
reported by the connector was inspected.

And, anyway, as DRM lacks a format negotiation API, there is no way to
tell a bridge/panel "use this format instead of this other one" (which
makes me wonders why more formats can be reported, but the
bus_formats[] helpers for connectors allow that, so I thought it made
sense to do the same for bridges).

>
> > +
> > +	return 0;
> > +}
> > +
> > +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> > +{
> > +	unsigned int bus_fmt;
> > +	int ret;
> > +
> > +	if (lvds->panel)
> > +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> > +	else
> > +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
>
> What if no bridge reports any format, shouldn't the connector be examined
> then?

There is no fallback selection policy at the moment as you can see, or
either, as it was before, the LVDS mode is not set for the rcar_lvds
component  if it's not reported by the next element in the pipeline (and I
should probably return 0, not an error here in that case).

The connector associated with a panel is only inspected if it's next in the
pipeline.

>
> > +	if (ret)
> > +		return;
> > +
> > +	switch (bus_fmt) {
> > +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> > +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> > +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> >  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> > -		mode = RCAR_LVDS_MODE_JEIDA;
> > +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
>
> This is b0rken, first the mirror bit is ORed into some unknown preexisting
> value, then the code falls through (without any fall through comment, btw)
> and forcibly sets the mode, thus discarding the mirror bit which was
> carefully ORed in.

You are correct, the second assignment should have been an |= and not
a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
*lvds' is kzalloc-ed in probe function.
>
> >  		break;
> > +
> > +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> > +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> > -		mode = RCAR_LVDS_MODE_VESA;
> > +		lvds->mode = RCAR_LVDS_MODE_VESA;
>
> Dito.
>
> Cheers,
> Peter
>
> >  		break;
> >  	default:
> >  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> > -			info->bus_formats[0]);
> > -		return;
> > +			bus_fmt);
> >  	}
> > -
> > -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> > -		mode |= RCAR_LVDS_MODE_MIRROR;
> > -
> > -	lvds->mode = mode;
> >  }
> >
> >  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 5044 bytes --]

Hi Peter,
   thanks for looking into this

On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > With the introduction of static input image format enumeration in DRM
> > bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> > from both panel or bridge, to set the desired LVDS mode.
> >
> > Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> > format, as it is only defined for drm connectors, but use the newly
> > introduced _LE version of LVDS mbus image formats.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
> >  1 file changed, 44 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3d2d3bb..2fa875f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> >  	return true;
> >  }
> >
> > -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> > +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
> > +						  unsigned int *bus_fmt)
> >  {
> >  	struct drm_display_info *info = &lvds->connector.display_info;
> > -	enum rcar_lvds_mode mode;
> > -
> > -	/*
> > -	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
> > -	 * are supported.
> > -	 */
> > -	if (!lvds->panel)
> > -		return;
> >
> >  	if (!info->num_bus_formats || !info->bus_formats) {
> >  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> > -		return;
> > +		return -EINVAL;
> > +	}
> > +
> > +	*bus_fmt = info->bus_formats[0];
> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
> > +					       unsigned int *bus_fmt)
> > +{
> > +	if (!lvds->next_bridge->num_bus_formats ||
> > +	    !lvds->next_bridge->bus_formats) {
> > +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> > +		return -EINVAL;
> >  	}
> >
> > -	switch (info->bus_formats[0]) {
> > +	*bus_fmt = lvds->next_bridge->bus_formats[0];
>
> What makes the first reported format the best choice?

It already was the selection 'policy' in place in this driver before
introducing bridge formats. As you can see from the switch I have here
removed, the first format was selected even when only the format
reported by the connector was inspected.

And, anyway, as DRM lacks a format negotiation API, there is no way to
tell a bridge/panel "use this format instead of this other one" (which
makes me wonders why more formats can be reported, but the
bus_formats[] helpers for connectors allow that, so I thought it made
sense to do the same for bridges).

>
> > +
> > +	return 0;
> > +}
> > +
> > +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> > +{
> > +	unsigned int bus_fmt;
> > +	int ret;
> > +
> > +	if (lvds->panel)
> > +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> > +	else
> > +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
>
> What if no bridge reports any format, shouldn't the connector be examined
> then?

There is no fallback selection policy at the moment as you can see, or
either, as it was before, the LVDS mode is not set for the rcar_lvds
component  if it's not reported by the next element in the pipeline (and I
should probably return 0, not an error here in that case).

The connector associated with a panel is only inspected if it's next in the
pipeline.

>
> > +	if (ret)
> > +		return;
> > +
> > +	switch (bus_fmt) {
> > +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> > +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> > +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> >  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> > -		mode = RCAR_LVDS_MODE_JEIDA;
> > +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
>
> This is b0rken, first the mirror bit is ORed into some unknown preexisting
> value, then the code falls through (without any fall through comment, btw)
> and forcibly sets the mode, thus discarding the mirror bit which was
> carefully ORed in.

You are correct, the second assignment should have been an |= and not
a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
*lvds' is kzalloc-ed in probe function.
>
> >  		break;
> > +
> > +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> > +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> > -		mode = RCAR_LVDS_MODE_VESA;
> > +		lvds->mode = RCAR_LVDS_MODE_VESA;
>
> Dito.
>
> Cheers,
> Peter
>
> >  		break;
> >  	default:
> >  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> > -			info->bus_formats[0]);
> > -		return;
> > +			bus_fmt);
> >  	}
> > -
> > -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> > -		mode |= RCAR_LVDS_MODE_MIRROR;
> > -
> > -	lvds->mode = mode;
> >  }
> >
> >  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> >
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

Hi Peter,
   thanks for commenting,

On Sun, Apr 22, 2018 at 10:02:41PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> > modes, selectable by means of an external pin.
> >
> > Describe the LVDS mode map through a newly defined mandatory property in
> > device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > index 37f0c04..0937595 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -12,6 +12,8 @@ Required properties:
> >  - compatible: Shall be "thine,thc63lvd1024"
> >  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> >    PPL and digital circuitry
> > +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be <1>
> > +  for mapping mode 1, <0> for mapping mode 2
>
> Since the MAP pin is an input pin, I would expect there to be an optional gpio
> specifier like thine,map-gpios so that the driver can set it according to
> the value given in thine,map in case the HW has a line from some gpio output
> to the MAP pin (instead of hardwired hi/low which seem to be your thinking).

I see... As the only use case I had has the pin tied to vcc, I
thought about making it a binary property, and I wonder in how many
cases the chip 'MAP' pin would actually be GPIO controlled input and
not an hardwired one instead. I don't see the LVDS mapping mode to be
changed at runtime, but you are right, who knows....

Do you think we can add an options 'thine,map-gpios' property along
to the proposed ones?

thanks
   j

>
> Cheers,
> Peter
>
> >
> >  Optional properties:
> >  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> > @@ -36,6 +38,7 @@ Example:
> >
> >  		vcc-supply = <&reg_lvds_vcc>;
> >  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> > +		thine,map = <1>;
> >
> >  		ports {
> >  			#address-cells = <1>;
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 2353 bytes --]

Hi Peter,
   thanks for commenting,

On Sun, Apr 22, 2018 at 10:02:41PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> > modes, selectable by means of an external pin.
> >
> > Describe the LVDS mode map through a newly defined mandatory property in
> > device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > index 37f0c04..0937595 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -12,6 +12,8 @@ Required properties:
> >  - compatible: Shall be "thine,thc63lvd1024"
> >  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> >    PPL and digital circuitry
> > +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be <1>
> > +  for mapping mode 1, <0> for mapping mode 2
>
> Since the MAP pin is an input pin, I would expect there to be an optional gpio
> specifier like thine,map-gpios so that the driver can set it according to
> the value given in thine,map in case the HW has a line from some gpio output
> to the MAP pin (instead of hardwired hi/low which seem to be your thinking).

I see... As the only use case I had has the pin tied to vcc, I
thought about making it a binary property, and I wonder in how many
cases the chip 'MAP' pin would actually be GPIO controlled input and
not an hardwired one instead. I don't see the LVDS mapping mode to be
changed at runtime, but you are right, who knows....

Do you think we can add an options 'thine,map-gpios' property along
to the proposed ones?

thanks
   j

>
> Cheers,
> Peter
>
> >
> >  Optional properties:
> >  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> > @@ -36,6 +38,7 @@ Example:
> >
> >  		vcc-supply = <&reg_lvds_vcc>;
> >  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> > +		thine,map = <1>;
> >
> >  		ports {
> >  			#address-cells = <1>;
> >
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]

Hi Peter,

On Sun, Apr 22, 2018 at 10:02:51PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> > modes, selectable by means of an external pin.
> >
> > Add support for configurable LVDS input mapping modes, using the newly
> > introduced support for bridge input image formats.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > index 48527f8..a3071a1 100644
> > --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -10,9 +10,15 @@
> >  #include <drm/drm_panel.h>
> >
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/of.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/regulator/consumer.h>
> >
> > +enum thc63_lvds_mapping_mode {
> > +	THC63_LVDS_MAP_MODE2,
> > +	THC63_LVDS_MAP_MODE1,
> > +};
> > +
> >  enum thc63_ports {
> >  	THC63_LVDS_IN0,
> >  	THC63_LVDS_IN1,
> > @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> >  	return 0;
> >  }
> >
> > +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> > +{
> > +	u32 bus_fmt;
> > +	u32 map;
> > +	int ret;
> > +
> > +	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
> > +	if (ret) {
> > +		dev_err(thc63->dev,
> > +			"Unable to parse property \"thine,map\": %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	switch (map) {
> > +	case THC63_LVDS_MAP_MODE1:
> > +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> > +		break;
> > +	case THC63_LVDS_MAP_MODE2:
> > +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
>
> Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
> or rgb101010/1x7x5, no?

I should combine the 'map' pin input mode property with the 'bus_width' one
to find that out probably.

Thanks
   j


>
> Cheers,
> Peter
>
> > +		break;
> > +	default:
> > +		dev_err(thc63->dev,
> > +			"Invalid value for property \"thine,map\": %u\n", map);
> > +		return -EINVAL;
> > +	}
> > +
> > +	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
> > +
> > +	return 0;
> > +}
> > +
> >  static int thc63_gpio_init(struct thc63_dev *thc63)
> >  {
> >  	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> > @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = thc63_set_bus_fmt(thc63);
> > +	if (ret)
> > +		return ret;
> > +
> >  	thc63->bridge.driver_private = thc63;
> >  	thc63->bridge.of_node = pdev->dev.of_node;
> >  	thc63->bridge.funcs = &thc63_bridge_func;
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Peter Rosin]

On 2018-04-23 09:28, jacopo mondi wrote:
> Hi Peter,
>    thanks for looking into this
> 
> On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
>> On 2018-04-19 11:31, Jacopo Mondi wrote:
>>> With the introduction of static input image format enumeration in DRM
>>> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
>>> from both panel or bridge, to set the desired LVDS mode.
>>>
>>> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
>>> format, as it is only defined for drm connectors, but use the newly
>>> introduced _LE version of LVDS mbus image formats.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
>>>  1 file changed, 44 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>>> index 3d2d3bb..2fa875f 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>>> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
>>>  	return true;
>>>  }
>>>
>>> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
>>> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
>>> +						  unsigned int *bus_fmt)
>>>  {
>>>  	struct drm_display_info *info = &lvds->connector.display_info;
>>> -	enum rcar_lvds_mode mode;
>>> -
>>> -	/*
>>> -	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
>>> -	 * are supported.
>>> -	 */
>>> -	if (!lvds->panel)
>>> -		return;
>>>
>>>  	if (!info->num_bus_formats || !info->bus_formats) {
>>>  		dev_err(lvds->dev, "no LVDS bus format reported\n");
>>> -		return;
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	*bus_fmt = info->bus_formats[0];
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
>>> +					       unsigned int *bus_fmt)
>>> +{
>>> +	if (!lvds->next_bridge->num_bus_formats ||
>>> +	    !lvds->next_bridge->bus_formats) {
>>> +		dev_err(lvds->dev, "no LVDS bus format reported\n");
>>> +		return -EINVAL;
>>>  	}
>>>
>>> -	switch (info->bus_formats[0]) {
>>> +	*bus_fmt = lvds->next_bridge->bus_formats[0];
>>
>> What makes the first reported format the best choice?
> 
> It already was the selection 'policy' in place in this driver before
> introducing bridge formats. As you can see from the switch I have here
> removed, the first format was selected even when only the format
> reported by the connector was inspected.

Well, *if* some bridge/panel do support more than one format, and your
driver depends on it being the first reported format, then I can easily
see that some other driver also requires its expected format to be first.
Then we might end up in a war over what format should be reported as the
first so that this multi-input bridge/panel could be used by both drivers.

> And, anyway, as DRM lacks a format negotiation API, there is no way to
> tell a bridge/panel "use this format instead of this other one" (which
> makes me wonders why more formats can be reported, but the
> bus_formats[] helpers for connectors allow that, so I thought it made
> sense to do the same for bridges).

Since there is no way to negotiate, I would assume that the other end
really does support all reported formats (in some automagical way). To
me, the only sensible approach is to loop over the formats and see if
*any* of them fits, and assume that something else<tm> deals with the
details.

>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
>>> +{
>>> +	unsigned int bus_fmt;
>>> +	int ret;
>>> +
>>> +	if (lvds->panel)
>>> +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
>>> +	else
>>> +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
>>
>> What if no bridge reports any format, shouldn't the connector be examined
>> then?
> 
> There is no fallback selection policy at the moment as you can see, or
> either, as it was before, the LVDS mode is not set for the rcar_lvds
> component  if it's not reported by the next element in the pipeline (and I
> should probably return 0, not an error here in that case).
> 
> The connector associated with a panel is only inspected if it's next in the
> pipeline.

But by not going to the connector for the case where no bridge in the
pipeline has any info on the format, you effectively demand that at
least some bridge in the pipeline should report supported input
format(s). That excludes a lot of existing bridge combinations from
being used. Or do you see it as a requirement that bridges must
report their supported input formats? Perhaps only when used with
this driver?

>>
>>> +	if (ret)
>>> +		return;
>>> +
>>> +	switch (bus_fmt) {
>>> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
>>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
>>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
>>>  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
>>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
>>> -		mode = RCAR_LVDS_MODE_JEIDA;
>>> +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
>>
>> This is b0rken, first the mirror bit is ORed into some unknown preexisting
>> value, then the code falls through (without any fall through comment, btw)
>> and forcibly sets the mode, thus discarding the mirror bit which was
>> carefully ORed in.
> 
> You are correct, the second assignment should have been an |= and not
> a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
> *lvds' is kzalloc-ed in probe function.

The code would be clearer if you explicitly zeroed the mode in this
function. Or do you rely on this function to not clobber other bits?
In that case apply some bit-mask.

Cheers,
Peter

>>
>>>  		break;
>>> +
>>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
>>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
>>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
>>> -		mode = RCAR_LVDS_MODE_VESA;
>>> +		lvds->mode = RCAR_LVDS_MODE_VESA;
>>
>> Dito.
>>
>> Cheers,
>> Peter
>>
>>>  		break;
>>>  	default:
>>>  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
>>> -			info->bus_formats[0]);
>>> -		return;
>>> +			bus_fmt);
>>>  	}
>>> -
>>> -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
>>> -		mode |= RCAR_LVDS_MODE_MIRROR;
>>> -
>>> -	lvds->mode = mode;
>>>  }
>>>
>>>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
>>>
>>

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 7621 bytes --]

Hi Peter,

On Mon, Apr 23, 2018 at 09:59:22AM +0200, Peter Rosin wrote:
> On 2018-04-23 09:28, jacopo mondi wrote:
> > Hi Peter,
> >    thanks for looking into this
> >
> > On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
> >> On 2018-04-19 11:31, Jacopo Mondi wrote:
> >>> With the introduction of static input image format enumeration in DRM
> >>> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> >>> from both panel or bridge, to set the desired LVDS mode.
> >>>
> >>> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> >>> format, as it is only defined for drm connectors, but use the newly
> >>> introduced _LE version of LVDS mbus image formats.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
> >>>  1 file changed, 44 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>> index 3d2d3bb..2fa875f 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> >>>  	return true;
> >>>  }
> >>>
> >>> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
> >>> +						  unsigned int *bus_fmt)
> >>>  {
> >>>  	struct drm_display_info *info = &lvds->connector.display_info;
> >>> -	enum rcar_lvds_mode mode;
> >>> -
> >>> -	/*
> >>> -	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
> >>> -	 * are supported.
> >>> -	 */
> >>> -	if (!lvds->panel)
> >>> -		return;
> >>>
> >>>  	if (!info->num_bus_formats || !info->bus_formats) {
> >>>  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>> -		return;
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	*bus_fmt = info->bus_formats[0];
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
> >>> +					       unsigned int *bus_fmt)
> >>> +{
> >>> +	if (!lvds->next_bridge->num_bus_formats ||
> >>> +	    !lvds->next_bridge->bus_formats) {
> >>> +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>> +		return -EINVAL;
> >>>  	}
> >>>
> >>> -	switch (info->bus_formats[0]) {
> >>> +	*bus_fmt = lvds->next_bridge->bus_formats[0];
> >>
> >> What makes the first reported format the best choice?
> >
> > It already was the selection 'policy' in place in this driver before
> > introducing bridge formats. As you can see from the switch I have here
> > removed, the first format was selected even when only the format
> > reported by the connector was inspected.
>
> Well, *if* some bridge/panel do support more than one format, and your
> driver depends on it being the first reported format, then I can easily
> see that some other driver also requires its expected format to be first.
> Then we might end up in a war over what format should be reported as the
> first so that this multi-input bridge/panel could be used by both drivers.
>
> > And, anyway, as DRM lacks a format negotiation API, there is no way to
> > tell a bridge/panel "use this format instead of this other one" (which
> > makes me wonders why more formats can be reported, but the
> > bus_formats[] helpers for connectors allow that, so I thought it made
> > sense to do the same for bridges).
>
> Since there is no way to negotiate, I would assume that the other end
> really does support all reported formats (in some automagical way). To
> me, the only sensible approach is to loop over the formats and see if
> *any* of them fits, and assume that something else<tm> deals with the
> details.
>

I see. I agree looping may be a better way of handling this
actually...

> >>
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>> +{
> >>> +	unsigned int bus_fmt;
> >>> +	int ret;
> >>> +
> >>> +	if (lvds->panel)
> >>> +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> >>> +	else
> >>> +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
> >>
> >> What if no bridge reports any format, shouldn't the connector be examined
> >> then?
> >
> > There is no fallback selection policy at the moment as you can see, or
> > either, as it was before, the LVDS mode is not set for the rcar_lvds
> > component  if it's not reported by the next element in the pipeline (and I
> > should probably return 0, not an error here in that case).
> >
> > The connector associated with a panel is only inspected if it's next in the
> > pipeline.
>
> But by not going to the connector for the case where no bridge in the
> pipeline has any info on the format, you effectively demand that at
> least some bridge in the pipeline should report supported input
> format(s). That excludes a lot of existing bridge combinations from
> being used. Or do you see it as a requirement that bridges must
> report their supported input formats? Perhaps only when used with
> this driver?

That's a point that should be discussed imho.

Daniel in his reply to your original series suggested format should be
reported by bridges when they get registered [1]. I have not enforced that
though, as it requires all mainline bridge drivers (which are not that
many -yet-) to register a format before bridge_add(). If there is
consensus on this, all other drivers should be updated (I'm not even
sure it is possible for all of them honestly).

Anyway, the only thing I'm sure is that I don't want to make this
driver 'special', I would prefer that in case format is not there,
we'll ignore the LVDS mode configuration as it was before this series.

Thanks
  j


[1] https://lkml.org/lkml/2018/4/4/50
>
> >>
> >>> +	if (ret)
> >>> +		return;
> >>> +
> >>> +	switch (bus_fmt) {
> >>> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> >>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> >>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> >>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> >>> -		mode = RCAR_LVDS_MODE_JEIDA;
> >>> +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
> >>
> >> This is b0rken, first the mirror bit is ORed into some unknown preexisting
> >> value, then the code falls through (without any fall through comment, btw)
> >> and forcibly sets the mode, thus discarding the mirror bit which was
> >> carefully ORed in.
> >
> > You are correct, the second assignment should have been an |= and not
> > a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
> > *lvds' is kzalloc-ed in probe function.
>
> The code would be clearer if you explicitly zeroed the mode in this
> function. Or do you rely on this function to not clobber other bits?
> In that case apply some bit-mask.
>
> Cheers,
> Peter
>
> >>
> >>>  		break;
> >>> +
> >>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> >>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> >>> -		mode = RCAR_LVDS_MODE_VESA;
> >>> +		lvds->mode = RCAR_LVDS_MODE_VESA;
> >>
> >> Dito.
> >>
> >> Cheers,
> >> Peter
> >>
> >>>  		break;
> >>>  	default:
> >>>  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> >>> -			info->bus_formats[0]);
> >>> -		return;
> >>> +			bus_fmt);
> >>>  	}
> >>> -
> >>> -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> >>> -		mode |= RCAR_LVDS_MODE_MIRROR;
> >>> -
> >>> -	lvds->mode = mode;
> >>>  }
> >>>
> >>>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> >>>
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 7621 bytes --]

Hi Peter,

On Mon, Apr 23, 2018 at 09:59:22AM +0200, Peter Rosin wrote:
> On 2018-04-23 09:28, jacopo mondi wrote:
> > Hi Peter,
> >    thanks for looking into this
> >
> > On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
> >> On 2018-04-19 11:31, Jacopo Mondi wrote:
> >>> With the introduction of static input image format enumeration in DRM
> >>> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> >>> from both panel or bridge, to set the desired LVDS mode.
> >>>
> >>> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> >>> format, as it is only defined for drm connectors, but use the newly
> >>> introduced _LE version of LVDS mbus image formats.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++++++++------------
> >>>  1 file changed, 44 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>> index 3d2d3bb..2fa875f 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> >>>  	return true;
> >>>  }
> >>>
> >>> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds *lvds,
> >>> +						  unsigned int *bus_fmt)
> >>>  {
> >>>  	struct drm_display_info *info = &lvds->connector.display_info;
> >>> -	enum rcar_lvds_mode mode;
> >>> -
> >>> -	/*
> >>> -	 * There is no API yet to retrieve LVDS mode from a bridge, only panels
> >>> -	 * are supported.
> >>> -	 */
> >>> -	if (!lvds->panel)
> >>> -		return;
> >>>
> >>>  	if (!info->num_bus_formats || !info->bus_formats) {
> >>>  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>> -		return;
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	*bus_fmt = info->bus_formats[0];
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds *lvds,
> >>> +					       unsigned int *bus_fmt)
> >>> +{
> >>> +	if (!lvds->next_bridge->num_bus_formats ||
> >>> +	    !lvds->next_bridge->bus_formats) {
> >>> +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>> +		return -EINVAL;
> >>>  	}
> >>>
> >>> -	switch (info->bus_formats[0]) {
> >>> +	*bus_fmt = lvds->next_bridge->bus_formats[0];
> >>
> >> What makes the first reported format the best choice?
> >
> > It already was the selection 'policy' in place in this driver before
> > introducing bridge formats. As you can see from the switch I have here
> > removed, the first format was selected even when only the format
> > reported by the connector was inspected.
>
> Well, *if* some bridge/panel do support more than one format, and your
> driver depends on it being the first reported format, then I can easily
> see that some other driver also requires its expected format to be first.
> Then we might end up in a war over what format should be reported as the
> first so that this multi-input bridge/panel could be used by both drivers.
>
> > And, anyway, as DRM lacks a format negotiation API, there is no way to
> > tell a bridge/panel "use this format instead of this other one" (which
> > makes me wonders why more formats can be reported, but the
> > bus_formats[] helpers for connectors allow that, so I thought it made
> > sense to do the same for bridges).
>
> Since there is no way to negotiate, I would assume that the other end
> really does support all reported formats (in some automagical way). To
> me, the only sensible approach is to loop over the formats and see if
> *any* of them fits, and assume that something else<tm> deals with the
> details.
>

I see. I agree looping may be a better way of handling this
actually...

> >>
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>> +{
> >>> +	unsigned int bus_fmt;
> >>> +	int ret;
> >>> +
> >>> +	if (lvds->panel)
> >>> +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> >>> +	else
> >>> +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
> >>
> >> What if no bridge reports any format, shouldn't the connector be examined
> >> then?
> >
> > There is no fallback selection policy at the moment as you can see, or
> > either, as it was before, the LVDS mode is not set for the rcar_lvds
> > component  if it's not reported by the next element in the pipeline (and I
> > should probably return 0, not an error here in that case).
> >
> > The connector associated with a panel is only inspected if it's next in the
> > pipeline.
>
> But by not going to the connector for the case where no bridge in the
> pipeline has any info on the format, you effectively demand that at
> least some bridge in the pipeline should report supported input
> format(s). That excludes a lot of existing bridge combinations from
> being used. Or do you see it as a requirement that bridges must
> report their supported input formats? Perhaps only when used with
> this driver?

That's a point that should be discussed imho.

Daniel in his reply to your original series suggested format should be
reported by bridges when they get registered [1]. I have not enforced that
though, as it requires all mainline bridge drivers (which are not that
many -yet-) to register a format before bridge_add(). If there is
consensus on this, all other drivers should be updated (I'm not even
sure it is possible for all of them honestly).

Anyway, the only thing I'm sure is that I don't want to make this
driver 'special', I would prefer that in case format is not there,
we'll ignore the LVDS mode configuration as it was before this series.

Thanks
  j


[1] https://lkml.org/lkml/2018/4/4/50
>
> >>
> >>> +	if (ret)
> >>> +		return;
> >>> +
> >>> +	switch (bus_fmt) {
> >>> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> >>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> >>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> >>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> >>> -		mode = RCAR_LVDS_MODE_JEIDA;
> >>> +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
> >>
> >> This is b0rken, first the mirror bit is ORed into some unknown preexisting
> >> value, then the code falls through (without any fall through comment, btw)
> >> and forcibly sets the mode, thus discarding the mirror bit which was
> >> carefully ORed in.
> >
> > You are correct, the second assignment should have been an |= and not
> > a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
> > *lvds' is kzalloc-ed in probe function.
>
> The code would be clearer if you explicitly zeroed the mode in this
> function. Or do you rely on this function to not clobber other bits?
> In that case apply some bit-mask.
>
> Cheers,
> Peter
>
> >>
> >>>  		break;
> >>> +
> >>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> >>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> >>> -		mode = RCAR_LVDS_MODE_VESA;
> >>> +		lvds->mode = RCAR_LVDS_MODE_VESA;
> >>
> >> Dito.
> >>
> >> Cheers,
> >> Peter
> >>
> >>>  		break;
> >>>  	default:
> >>>  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> >>> -			info->bus_formats[0]);
> >>> -		return;
> >>> +			bus_fmt);
> >>>  	}
> >>> -
> >>> -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> >>> -		mode |= RCAR_LVDS_MODE_MIRROR;
> >>> -
> >>> -	lvds->mode = mode;
> >>>  }
> >>>
> >>>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> >>>
> >>
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> Add support for storing image format information in DRM bridges with
> associated helper function.
> 
> This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> is for connectors.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  8 ++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe..e2ad098 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  }
> 
>  /**
> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> + * @bridge: the bridge to set image formats in
> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats

Why the \ (here and below) ?

> + * @num_formats: number of elements in the @formats array
> + *
> + * Store a list of supported image formats in a bridge.
> + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h
> for
> + * a full list of available formats.
> + */
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> *formats,
> +			       unsigned int num_formats)
> +{
> +	u32 *fmts;
> +
> +	if (!formats || !num_formats)
> +		return -EINVAL;
> +
> +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);

This memory will be leaked when the bridge is destroyed.

> +	if (!fmts)
> +		return -ENOMEM;
> +
> +	kfree(bridge->bus_formats);
> +	bridge->bus_formats = fmts;
> +	bridge->num_bus_formats = num_formats;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> +
> +/**
>   * DOC: bridge callbacks
>   *
>   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec..6b3648c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
>   * @of_node: device node pointer to the bridge
> + * @bus_formats: wire image formats. Array of @num_bus_formats
> MEDIA_BUS_FMT\_
> + * elements
> + * @num_bus_formats: size of @bus_formats array
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> @@ -271,6 +274,9 @@ struct drm_bridge {
>  #ifdef CONFIG_OF
>  	struct device_node *of_node;
>  #endif
> +	const u32 *bus_formats;
> +	unsigned int num_bus_formats;
> +
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> 
> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>  			struct drm_display_mode *adjusted_mode);
>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>  void drm_bridge_enable(struct drm_bridge *bridge);
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> +			       unsigned int num_fmts);
> 
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> Add support for storing image format information in DRM bridges with
> associated helper function.
> 
> This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> is for connectors.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  8 ++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe..e2ad098 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  }
> 
>  /**
> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> + * @bridge: the bridge to set image formats in
> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats

Why the \ (here and below) ?

> + * @num_formats: number of elements in the @formats array
> + *
> + * Store a list of supported image formats in a bridge.
> + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h
> for
> + * a full list of available formats.
> + */
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> *formats,
> +			       unsigned int num_formats)
> +{
> +	u32 *fmts;
> +
> +	if (!formats || !num_formats)
> +		return -EINVAL;
> +
> +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);

This memory will be leaked when the bridge is destroyed.

> +	if (!fmts)
> +		return -ENOMEM;
> +
> +	kfree(bridge->bus_formats);
> +	bridge->bus_formats = fmts;
> +	bridge->num_bus_formats = num_formats;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> +
> +/**
>   * DOC: bridge callbacks
>   *
>   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec..6b3648c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
>   * @of_node: device node pointer to the bridge
> + * @bus_formats: wire image formats. Array of @num_bus_formats
> MEDIA_BUS_FMT\_
> + * elements
> + * @num_bus_formats: size of @bus_formats array
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> @@ -271,6 +274,9 @@ struct drm_bridge {
>  #ifdef CONFIG_OF
>  	struct device_node *of_node;
>  #endif
> +	const u32 *bus_formats;
> +	unsigned int num_bus_formats;
> +
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> 
> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>  			struct drm_display_mode *adjusted_mode);
>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>  void drm_bridge_enable(struct drm_bridge *bridge);
> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> +			       unsigned int num_fmts);
> 
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] arm64: dts: renesas: eagle: Add thc63 LVDS map

[Simon Horman]

On Thu, Apr 19, 2018 at 11:31:05AM +0200, Jacopo Mondi wrote:
> Add LVDS map mode description property to THC63LVD1024 LVDS decoder in
> R-Car V3M-Eagle board device tree.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Hi Jacopo,

it looks like there has been a request to change this binding.
So I have marked this as "Changes Requested". Please repost or otherwise
ping me if that turns out not to be the case.

> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index ebfbb51..2609fa3 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -56,6 +56,7 @@
>  		compatible = "thine,thc63lvd1024";
>  
>  		vcc-supply = <&d3p3>;
> +		thine,map = <1>;
>  
>  		ports {
>  			#address-cells = <1>;
> -- 
> 2.7.4
> 

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Laurent Pinchart]

Hi Jacopo,

On Monday, 23 April 2018 10:35:04 EEST jacopo mondi wrote:
> On Sun, Apr 22, 2018 at 10:02:41PM +0200, Peter Rosin wrote:
> > On 2018-04-19 11:31, Jacopo Mondi wrote:
> >> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> >> modes, selectable by means of an external pin.
> >> 
> >> Describe the LVDS mode map through a newly defined mandatory property in
> >> device tree bindings.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt    | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> index 37f0c04..0937595 100644
> >> ---
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> @@ -12,6 +12,8 @@ Required properties:
> >>  - compatible: Shall be "thine,thc63lvd1024"
> >>  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS
> >>  input,
> >>    PPL and digital circuitry
> >> 
> >> +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall
> >> be <1>
> >> +  for mapping mode 1, <0> for mapping mode 2
> > 
> > Since the MAP pin is an input pin, I would expect there to be an optional
> > gpio specifier like thine,map-gpios so that the driver can set it
> > according to the value given in thine,map in case the HW has a line from
> > some gpio output to the MAP pin (instead of hardwired hi/low which seem
> > to be your thinking).
> 
> I see... As the only use case I had has the pin tied to vcc, I
> thought about making it a binary property, and I wonder in how many
> cases the chip 'MAP' pin would actually be GPIO controlled input and
> not an hardwired one instead. I don't see the LVDS mapping mode to be
> changed at runtime, but you are right, who knows....
> 
> Do you think we can add an options 'thine,map-gpios' property along
> to the proposed ones?

If the MAP pin is connected to an SoC-controlled GPIO for a given platform 
then we will likely need a thine,map-gpios property to control the pin. I 
think we can leave it out for now and add it later if the need arises.

The thine,map property serves a different purpose, it indicates the level of 
the MAP pin for platforms where the pin is hardwired. If we later introduce a 
thine,map-gpios property it should be mutually exclusive with the thine,map 
property. The level of the MAP pin should be then software-controlled, not set 
through DT.

> >>  Optional properties:
> >>  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> >> 
> >> @@ -36,6 +38,7 @@ Example:
> >>  		vcc-supply = <&reg_lvds_vcc>;
> >>  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> >> +		thine,map = <1>;
> >> 
> >>  		ports {
> >>  			#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Laurent Pinchart]

Hi Jacopo,

On Monday, 23 April 2018 10:35:04 EEST jacopo mondi wrote:
> On Sun, Apr 22, 2018 at 10:02:41PM +0200, Peter Rosin wrote:
> > On 2018-04-19 11:31, Jacopo Mondi wrote:
> >> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> >> modes, selectable by means of an external pin.
> >> 
> >> Describe the LVDS mode map through a newly defined mandatory property in
> >> device tree bindings.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt    | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> index 37f0c04..0937595 100644
> >> ---
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> @@ -12,6 +12,8 @@ Required properties:
> >>  - compatible: Shall be "thine,thc63lvd1024"
> >>  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS
> >>  input,
> >>    PPL and digital circuitry
> >> 
> >> +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall
> >> be <1>
> >> +  for mapping mode 1, <0> for mapping mode 2
> > 
> > Since the MAP pin is an input pin, I would expect there to be an optional
> > gpio specifier like thine,map-gpios so that the driver can set it
> > according to the value given in thine,map in case the HW has a line from
> > some gpio output to the MAP pin (instead of hardwired hi/low which seem
> > to be your thinking).
> 
> I see... As the only use case I had has the pin tied to vcc, I
> thought about making it a binary property, and I wonder in how many
> cases the chip 'MAP' pin would actually be GPIO controlled input and
> not an hardwired one instead. I don't see the LVDS mapping mode to be
> changed at runtime, but you are right, who knows....
> 
> Do you think we can add an options 'thine,map-gpios' property along
> to the proposed ones?

If the MAP pin is connected to an SoC-controlled GPIO for a given platform 
then we will likely need a thine,map-gpios property to control the pin. I 
think we can leave it out for now and add it later if the need arises.

The thine,map property serves a different purpose, it indicates the level of 
the MAP pin for platforms where the pin is hardwired. If we later introduce a 
thine,map-gpios property it should be mutually exclusive with the thine,map 
property. The level of the MAP pin should be then software-controlled, not set 
through DT.

> >>  Optional properties:
> >>  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> >> 
> >> @@ -36,6 +38,7 @@ Example:
> >>  		vcc-supply = <&reg_lvds_vcc>;
> >>  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> >> +		thine,map = <1>;
> >> 
> >>  		ports {
> >>  			#address-cells = <1>;

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:03 EEST Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> modes, selectable by means of an external pin.
> 
> Describe the LVDS mode map through a newly defined mandatory property in
> device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3
> +++ 1 file changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> index 37f0c04..0937595 100644
> ---
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> +++
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -12,6 +12,8 @@ Required properties:
>  - compatible: Shall be "thine,thc63lvd1024"
>  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> PPL and digital circuitry
> +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be
> <1>
> +  for mapping mode 1, <0> for mapping mode 2

That's sounds like an odd mapping. I suppose you have modeled it based on the 
state of the MAP pin instead of the mode number (MAP low means mode 2, MAP 
high means mode 1). To avoid confusing readers I would write it as

- thine,map: level of the MAP pin that selects the LVDS mapping mode. Shall be
  <0> for low level (mapping mode 2) or <1> for high level (mapping mode 1).

Apart from that this patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  Optional properties:
>  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> @@ -36,6 +38,7 @@ Example:
> 
>  		vcc-supply = <&reg_lvds_vcc>;
>  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +		thine,map = <1>;
> 
>  		ports {
>  			#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:03 EEST Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> modes, selectable by means of an external pin.
> 
> Describe the LVDS mode map through a newly defined mandatory property in
> device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3
> +++ 1 file changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> index 37f0c04..0937595 100644
> ---
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> +++
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -12,6 +12,8 @@ Required properties:
>  - compatible: Shall be "thine,thc63lvd1024"
>  - vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> PPL and digital circuitry
> +- thine,map: LVDS mapping mode selection signal, pin name "MAP". Shall be
> <1>
> +  for mapping mode 1, <0> for mapping mode 2

That's sounds like an odd mapping. I suppose you have modeled it based on the 
state of the MAP pin instead of the mode number (MAP low means mode 2, MAP 
high means mode 1). To avoid confusing readers I would write it as

- thine,map: level of the MAP pin that selects the LVDS mapping mode. Shall be
  <0> for low level (mapping mode 2) or <1> for high level (mapping mode 1).

Apart from that this patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  Optional properties:
>  - powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> @@ -36,6 +38,7 @@ Example:
> 
>  		vcc-supply = <&reg_lvds_vcc>;
>  		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +		thine,map = <1>;
> 
>  		ports {
>  			#address-cells = <1>;

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:04 EEST Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> modes, selectable by means of an external pin.
> 
> Add support for configurable LVDS input mapping modes, using the newly
> introduced support for bridge input image formats.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c index 48527f8..a3071a1 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -10,9 +10,15 @@
>  #include <drm/drm_panel.h>
> 
>  #include <linux/gpio/consumer.h>
> +#include <linux/of.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
> 
> +enum thc63_lvds_mapping_mode {
> +	THC63_LVDS_MAP_MODE2,
> +	THC63_LVDS_MAP_MODE1,
> +};
> +
>  enum thc63_ports {
>  	THC63_LVDS_IN0,
>  	THC63_LVDS_IN1,
> @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>  	return 0;
>  }
> 
> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> +{
> +	u32 bus_fmt;
> +	u32 map;
> +	int ret;
> +
> +	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
> +	if (ret) {
> +		dev_err(thc63->dev,
> +			"Unable to parse property \"thine,map\": %d\n", ret);
> +		return ret;

Given that the property is currently not defined, I think you should select a 
default instead of returning an error to preserve backward compatibility. You 
can print a warning message to get the DT updated, maybe something like

	u32 map = 0;
	...

	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
	if (ret)
		dev_warn(thc63->dev,
			 "Unable to parse thine,map property (%d), defaulting to 0\n",
			 ret);

> +	}
> +
> +	switch (map) {
> +	case THC63_LVDS_MAP_MODE1:
> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> +		break;
> +	case THC63_LVDS_MAP_MODE2:
> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> +		break;
> +	default:
> +		dev_err(thc63->dev,
> +			"Invalid value for property \"thine,map\": %u\n", map);
> +		return -EINVAL;

This is fin as invalid property values should be rejected.

> +	}
> +
> +	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
> +
> +	return 0;
> +}
> +
>  static int thc63_gpio_init(struct thc63_dev *thc63)
>  {
>  	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
> 
> +	ret = thc63_set_bus_fmt(thc63);
> +	if (ret)
> +		return ret;
> +

Or you could move the code to thc63_parse_dt() as you're parsing DT.

>  	thc63->bridge.driver_private = thc63;
>  	thc63->bridge.of_node = pdev->dev.of_node;
>  	thc63->bridge.funcs = &thc63_bridge_func;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:04 EEST Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> modes, selectable by means of an external pin.
> 
> Add support for configurable LVDS input mapping modes, using the newly
> introduced support for bridge input image formats.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c index 48527f8..a3071a1 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -10,9 +10,15 @@
>  #include <drm/drm_panel.h>
> 
>  #include <linux/gpio/consumer.h>
> +#include <linux/of.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
> 
> +enum thc63_lvds_mapping_mode {
> +	THC63_LVDS_MAP_MODE2,
> +	THC63_LVDS_MAP_MODE1,
> +};
> +
>  enum thc63_ports {
>  	THC63_LVDS_IN0,
>  	THC63_LVDS_IN1,
> @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>  	return 0;
>  }
> 
> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> +{
> +	u32 bus_fmt;
> +	u32 map;
> +	int ret;
> +
> +	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
> +	if (ret) {
> +		dev_err(thc63->dev,
> +			"Unable to parse property \"thine,map\": %d\n", ret);
> +		return ret;

Given that the property is currently not defined, I think you should select a 
default instead of returning an error to preserve backward compatibility. You 
can print a warning message to get the DT updated, maybe something like

	u32 map = 0;
	...

	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
	if (ret)
		dev_warn(thc63->dev,
			 "Unable to parse thine,map property (%d), defaulting to 0\n",
			 ret);

> +	}
> +
> +	switch (map) {
> +	case THC63_LVDS_MAP_MODE1:
> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> +		break;
> +	case THC63_LVDS_MAP_MODE2:
> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> +		break;
> +	default:
> +		dev_err(thc63->dev,
> +			"Invalid value for property \"thine,map\": %u\n", map);
> +		return -EINVAL;

This is fin as invalid property values should be rejected.

> +	}
> +
> +	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
> +
> +	return 0;
> +}
> +
>  static int thc63_gpio_init(struct thc63_dev *thc63)
>  {
>  	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
> 
> +	ret = thc63_set_bus_fmt(thc63);
> +	if (ret)
> +		return ret;
> +

Or you could move the code to thc63_parse_dt() as you're parsing DT.

>  	thc63->bridge.driver_private = thc63;
>  	thc63->bridge.of_node = pdev->dev.of_node;
>  	thc63->bridge.funcs = &thc63_bridge_func;

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Laurent Pinchart]

Hi Jacopo,

On Monday, 23 April 2018 10:41:56 EEST jacopo mondi wrote:
> On Sun, Apr 22, 2018 at 10:02:51PM +0200, Peter Rosin wrote:
> > On 2018-04-19 11:31, Jacopo Mondi wrote:
> >> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> >> modes, selectable by means of an external pin.
> >> 
> >> Add support for configurable LVDS input mapping modes, using the newly
> >> introduced support for bridge input image formats.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 ++++++++++++++++++++++++++++
> >>  1 file changed, 41 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> b/drivers/gpu/drm/bridge/thc63lvd1024.c index 48527f8..a3071a1 100644
> >> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c

[snip]

> >> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> >> +{
> >> +	u32 bus_fmt;
> >> +	u32 map;
> >> +	int ret;
> >> +
> >> +	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
> >> +	if (ret) {
> >> +		dev_err(thc63->dev,
> >> +			"Unable to parse property \"thine,map\": %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	switch (map) {
> >> +	case THC63_LVDS_MAP_MODE1:
> >> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> >> +		break;
> >> +	case THC63_LVDS_MAP_MODE2:
> >> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> > 
> > Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
> > or rgb101010/1x7x5, no?
> 
> I should combine the 'map' pin input mode property with the 'bus_width' one
> to find that out probably.

Yes, but that could also be left for later, when the need to support those 
formats arise, especially given that include/uapi/linux/media-bus-format.h has 
no 1x7x5 formats yet.

> >> +		break;
> >> +	default:
> >> +		dev_err(thc63->dev,
> >> +			"Invalid value for property \"thine,map\": %u\n", map);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
> >> +
> >> +	return 0;
> >> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Laurent Pinchart]

Hi Jacopo,

On Monday, 23 April 2018 10:41:56 EEST jacopo mondi wrote:
> On Sun, Apr 22, 2018 at 10:02:51PM +0200, Peter Rosin wrote:
> > On 2018-04-19 11:31, Jacopo Mondi wrote:
> >> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> >> modes, selectable by means of an external pin.
> >> 
> >> Add support for configurable LVDS input mapping modes, using the newly
> >> introduced support for bridge input image formats.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 ++++++++++++++++++++++++++++
> >>  1 file changed, 41 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> b/drivers/gpu/drm/bridge/thc63lvd1024.c index 48527f8..a3071a1 100644
> >> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c

[snip]

> >> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> >> +{
> >> +	u32 bus_fmt;
> >> +	u32 map;
> >> +	int ret;
> >> +
> >> +	ret = of_property_read_u32(thc63->dev->of_node, "thine,map", &map);
> >> +	if (ret) {
> >> +		dev_err(thc63->dev,
> >> +			"Unable to parse property \"thine,map\": %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	switch (map) {
> >> +	case THC63_LVDS_MAP_MODE1:
> >> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> >> +		break;
> >> +	case THC63_LVDS_MAP_MODE2:
> >> +		bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> > 
> > Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
> > or rgb101010/1x7x5, no?
> 
> I should combine the 'map' pin input mode property with the 'bus_width' one
> to find that out probably.

Yes, but that could also be left for later, when the need to support those 
formats arise, especially given that include/uapi/linux/media-bus-format.h has 
no 1x7x5 formats yet.

> >> +		break;
> >> +	default:
> >> +		dev_err(thc63->dev,
> >> +			"Invalid value for property \"thine,map\": %u\n", map);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	drm_bridge_set_bus_formats(&thc63->bridge, &bus_fmt, 1);
> >> +
> >> +	return 0;
> >> +}

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Laurent Pinchart]

Hello,

On Monday, 23 April 2018 11:47:28 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 09:59:22AM +0200, Peter Rosin wrote:
> > On 2018-04-23 09:28, jacopo mondi wrote:
> >> On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
> >>> On 2018-04-19 11:31, Jacopo Mondi wrote:
> >>>> With the introduction of static input image format enumeration in DRM
> >>>> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> >>>> from both panel or bridge, to set the desired LVDS mode.
> >>>> 
> >>>> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> >>>> format, as it is only defined for drm connectors, but use the newly
> >>>> introduced _LE version of LVDS mbus image formats.
> >>>> 
> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++---------
> >>>>  1 file changed, 44 insertions(+), 20 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>>> b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 3d2d3bb..2fa875f 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>>> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct
> >>>> drm_bridge *bridge,
> >>>>  	return true;
> >>>>  }
> >>>> 
> >>>> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>>> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds
> >>>> *lvds,
> >>>> +						  unsigned int *bus_fmt)
> >>>>  {
> >>>>  	struct drm_display_info *info = &lvds->connector.display_info;
> >>>> -	enum rcar_lvds_mode mode;
> >>>> -
> >>>> -	/*
> >>>> -	 * There is no API yet to retrieve LVDS mode from a bridge, only
> >>>> panels
> >>>> -	 * are supported.
> >>>> -	 */
> >>>> -	if (!lvds->panel)
> >>>> -		return;
> >>>> 
> >>>>  	if (!info->num_bus_formats || !info->bus_formats) {
> >>>>  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>>> -		return;
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	*bus_fmt = info->bus_formats[0];
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds
> >>>> *lvds,
> >>>> +					       unsigned int *bus_fmt)
> >>>> +{
> >>>> +	if (!lvds->next_bridge->num_bus_formats ||
> >>>> +	    !lvds->next_bridge->bus_formats) {
> >>>> +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>>> +		return -EINVAL;
> >>>>  	}
> >>>> 
> >>>> -	switch (info->bus_formats[0]) {
> >>>> +	*bus_fmt = lvds->next_bridge->bus_formats[0];
> >>> 
> >>> What makes the first reported format the best choice?
> >> 
> >> It already was the selection 'policy' in place in this driver before
> >> introducing bridge formats. As you can see from the switch I have here
> >> removed, the first format was selected even when only the format
> >> reported by the connector was inspected.
> > 
> > Well, *if* some bridge/panel do support more than one format, and your
> > driver depends on it being the first reported format, then I can easily
> > see that some other driver also requires its expected format to be first.
> > Then we might end up in a war over what format should be reported as the
> > first so that this multi-input bridge/panel could be used by both drivers.

In that case we would have to implement a format negotiation procedure for 
bridges. It won't work transparently anyway, if a bridge supports multiple 
formats, one will need to be selected, and the bridge hardware will need to be 
configured accordingly.

I don't think this is a problem for now. The bridge we're using reports a 
single format, as it supports a single format at the hardware level. If an 
LVDS decoder accepting multiple formats later needs to be supported I'd be 
happy to participate in the design of a bridge format negotiation procedure 
and API.

> >> And, anyway, as DRM lacks a format negotiation API, there is no way to
> >> tell a bridge/panel "use this format instead of this other one" (which
> >> makes me wonders why more formats can be reported, but the
> >> bus_formats[] helpers for connectors allow that, so I thought it made
> >> sense to do the same for bridges).
> > 
> > Since there is no way to negotiate, I would assume that the other end
> > really does support all reported formats (in some automagical way).

In practice it does, because it supports a single format. Selection is thus 
pretty easy :-)

> > To me, the only sensible approach is to loop over the formats and see if
> > *any* of them fits, and assume that something else<tm> deals with the
> > details.
> 
> I see. I agree looping may be a better way of handling this actually...

I don't think so, for the reasons explained above. If the bridge supports 
multiple incompatible formats we will need to select one of them and configure 
the bridge hardware appropriately. There is no magic autoselection, so looping 
won't help, we will need more than that. Using the first first is thus good 
enough here. Let's not forget that this patch is against the internal LVDS 
encoder driver, so we're guaranteed that the next bridge in the chain accepts 
LVDS. This limits the cases that need to be supported for now.

> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>>> +{
> >>>> +	unsigned int bus_fmt;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (lvds->panel)
> >>>> +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> >>>> +	else
> >>>> +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
> >>> 
> >>> What if no bridge reports any format, shouldn't the connector be
> >>> examined then?
> >> 
> >> There is no fallback selection policy at the moment as you can see, or
> >> either, as it was before, the LVDS mode is not set for the rcar_lvds
> >> component  if it's not reported by the next element in the pipeline (and
> >> I should probably return 0, not an error here in that case).
> >> 
> >> The connector associated with a panel is only inspected if it's next in
> >> the pipeline.
> > 
> > But by not going to the connector for the case where no bridge in the
> > pipeline has any info on the format, you effectively demand that at
> > least some bridge in the pipeline should report supported input
> > format(s). That excludes a lot of existing bridge combinations from
> > being used. Or do you see it as a requirement that bridges must
> > report their supported input formats? Perhaps only when used with
> > this driver?

We demand that the next component in the chain, bridge or panel, reports the 
LVDS mode it requires through the bus format API. I don't think that's an 
issue, if we later need to use a different bridge that doesn't comply with 
this requirement (is there even other LVDS decoder bridge drivers in mainline 
?) then we'll implement bus format support in that bridge driver. It will not 
break any existing platform.

Looking at the connector won't help as the connector will likely not report an 
LVDS bus format if there's an LVDS decoder bridge in the chain.

However, given that the Lager board currently uses a DT hack and connects the 
DU LVDS output to an HDMI encoder without modeling the on-board LVDS decoder 
in DT, we need to ensure that backward compatibility is preserved by not 
returning a fatal error if the next bridge doesn't report bus formats.

> That's a point that should be discussed imho.
> 
> Daniel in his reply to your original series suggested format should be
> reported by bridges when they get registered [1]. I have not enforced that
> though, as it requires all mainline bridge drivers (which are not that
> many -yet-) to register a format before bridge_add(). If there is
> consensus on this, all other drivers should be updated (I'm not even
> sure it is possible for all of them honestly).

I wouldn't make it mandatory yet, because I'm also not sure if all bridges 
accept a single input format.

> Anyway, the only thing I'm sure is that I don't want to make this
> driver 'special', I would prefer that in case format is not there,
> we'll ignore the LVDS mode configuration as it was before this series.

Yes, that seems the best option to me. You can print a warning message to 
ensure that DT and/or bridge drivers gets updated, but we should default to 
the same LVDS mode as before.

> [1] https://lkml.org/lkml/2018/4/4/50
> 
> >>>> +	if (ret)
> >>>> +		return;
> >>>> +
> >>>> +	switch (bus_fmt) {
> >>>> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> >>>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> >>>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>> 
> >>>>  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> >>>> 
> >>>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> >>>> -		mode = RCAR_LVDS_MODE_JEIDA;
> >>>> +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
> >>> 
> >>> This is b0rken, first the mirror bit is ORed into some unknown
> >>> preexisting value, then the code falls through (without any fall
> >>> through comment, btw) and forcibly sets the mode, thus discarding the
> >>> mirror bit which was carefully ORed in.
> >> 
> >> You are correct, the second assignment should have been an |= and not
> >> a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
> >> *lvds' is kzalloc-ed in probe function.
> > 
> > The code would be clearer if you explicitly zeroed the mode in this
> > function. Or do you rely on this function to not clobber other bits?
> > In that case apply some bit-mask.
> >
> >>>>  		break;
> >>>> +
> >>>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> >>>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>> 
> >>>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> >>>> -		mode = RCAR_LVDS_MODE_VESA;
> >>>> +		lvds->mode = RCAR_LVDS_MODE_VESA;
> >>> 
> >>> Dito.
> >>> 
> >>>>  		break;
> >>>>  	
> >>>>  	default:
> >>>>  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> >>>> -			info->bus_formats[0]);
> >>>> -		return;
> >>>> +			bus_fmt);
> >>>>  	}
> >>>> -
> >>>> -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> >>>> -		mode |= RCAR_LVDS_MODE_MIRROR;
> >>>> -
> >>>> -	lvds->mode = mode;
> >>>>  }
> >>>>  
> >>>>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] drm: rcar-du: rcar-lvds: Add bridge format support

[Laurent Pinchart]

Hello,

On Monday, 23 April 2018 11:47:28 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 09:59:22AM +0200, Peter Rosin wrote:
> > On 2018-04-23 09:28, jacopo mondi wrote:
> >> On Sun, Apr 22, 2018 at 10:08:21PM +0200, Peter Rosin wrote:
> >>> On 2018-04-19 11:31, Jacopo Mondi wrote:
> >>>> With the introduction of static input image format enumeration in DRM
> >>>> bridges, add support to retrieve the format in rcar-lvds LVDS encoder
> >>>> from both panel or bridge, to set the desired LVDS mode.
> >>>> 
> >>>> Do not rely on 'DRM_BUS_FLAG_DATA_LSB_TO_MSB' flag to mirror the LVDS
> >>>> format, as it is only defined for drm connectors, but use the newly
> >>>> introduced _LE version of LVDS mbus image formats.
> >>>> 
> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 64 +++++++++++++++++++---------
> >>>>  1 file changed, 44 insertions(+), 20 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>>> b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 3d2d3bb..2fa875f 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> >>>> @@ -280,41 +280,65 @@ static bool rcar_lvds_mode_fixup(struct
> >>>> drm_bridge *bridge,
> >>>>  	return true;
> >>>>  }
> >>>> 
> >>>> -static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>>> +static int rcar_lvds_get_lvds_mode_from_connector(struct rcar_lvds
> >>>> *lvds,
> >>>> +						  unsigned int *bus_fmt)
> >>>>  {
> >>>>  	struct drm_display_info *info = &lvds->connector.display_info;
> >>>> -	enum rcar_lvds_mode mode;
> >>>> -
> >>>> -	/*
> >>>> -	 * There is no API yet to retrieve LVDS mode from a bridge, only
> >>>> panels
> >>>> -	 * are supported.
> >>>> -	 */
> >>>> -	if (!lvds->panel)
> >>>> -		return;
> >>>> 
> >>>>  	if (!info->num_bus_formats || !info->bus_formats) {
> >>>>  		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>>> -		return;
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	*bus_fmt = info->bus_formats[0];
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rcar_lvds_get_lvds_mode_from_bridge(struct rcar_lvds
> >>>> *lvds,
> >>>> +					       unsigned int *bus_fmt)
> >>>> +{
> >>>> +	if (!lvds->next_bridge->num_bus_formats ||
> >>>> +	    !lvds->next_bridge->bus_formats) {
> >>>> +		dev_err(lvds->dev, "no LVDS bus format reported\n");
> >>>> +		return -EINVAL;
> >>>>  	}
> >>>> 
> >>>> -	switch (info->bus_formats[0]) {
> >>>> +	*bus_fmt = lvds->next_bridge->bus_formats[0];
> >>> 
> >>> What makes the first reported format the best choice?
> >> 
> >> It already was the selection 'policy' in place in this driver before
> >> introducing bridge formats. As you can see from the switch I have here
> >> removed, the first format was selected even when only the format
> >> reported by the connector was inspected.
> > 
> > Well, *if* some bridge/panel do support more than one format, and your
> > driver depends on it being the first reported format, then I can easily
> > see that some other driver also requires its expected format to be first.
> > Then we might end up in a war over what format should be reported as the
> > first so that this multi-input bridge/panel could be used by both drivers.

In that case we would have to implement a format negotiation procedure for 
bridges. It won't work transparently anyway, if a bridge supports multiple 
formats, one will need to be selected, and the bridge hardware will need to be 
configured accordingly.

I don't think this is a problem for now. The bridge we're using reports a 
single format, as it supports a single format at the hardware level. If an 
LVDS decoder accepting multiple formats later needs to be supported I'd be 
happy to participate in the design of a bridge format negotiation procedure 
and API.

> >> And, anyway, as DRM lacks a format negotiation API, there is no way to
> >> tell a bridge/panel "use this format instead of this other one" (which
> >> makes me wonders why more formats can be reported, but the
> >> bus_formats[] helpers for connectors allow that, so I thought it made
> >> sense to do the same for bridges).
> > 
> > Since there is no way to negotiate, I would assume that the other end
> > really does support all reported formats (in some automagical way).

In practice it does, because it supports a single format. Selection is thus 
pretty easy :-)

> > To me, the only sensible approach is to loop over the formats and see if
> > *any* of them fits, and assume that something else<tm> deals with the
> > details.
> 
> I see. I agree looping may be a better way of handling this actually...

I don't think so, for the reasons explained above. If the bridge supports 
multiple incompatible formats we will need to select one of them and configure 
the bridge hardware appropriately. There is no magic autoselection, so looping 
won't help, we will need more than that. Using the first first is thus good 
enough here. Let's not forget that this patch is against the internal LVDS 
encoder driver, so we're guaranteed that the next bridge in the chain accepts 
LVDS. This limits the cases that need to be supported for now.

> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static void rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds)
> >>>> +{
> >>>> +	unsigned int bus_fmt;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (lvds->panel)
> >>>> +		ret = rcar_lvds_get_lvds_mode_from_connector(lvds, &bus_fmt);
> >>>> +	else
> >>>> +		ret = rcar_lvds_get_lvds_mode_from_bridge(lvds, &bus_fmt);
> >>> 
> >>> What if no bridge reports any format, shouldn't the connector be
> >>> examined then?
> >> 
> >> There is no fallback selection policy at the moment as you can see, or
> >> either, as it was before, the LVDS mode is not set for the rcar_lvds
> >> component  if it's not reported by the next element in the pipeline (and
> >> I should probably return 0, not an error here in that case).
> >> 
> >> The connector associated with a panel is only inspected if it's next in
> >> the pipeline.
> > 
> > But by not going to the connector for the case where no bridge in the
> > pipeline has any info on the format, you effectively demand that at
> > least some bridge in the pipeline should report supported input
> > format(s). That excludes a lot of existing bridge combinations from
> > being used. Or do you see it as a requirement that bridges must
> > report their supported input formats? Perhaps only when used with
> > this driver?

We demand that the next component in the chain, bridge or panel, reports the 
LVDS mode it requires through the bus format API. I don't think that's an 
issue, if we later need to use a different bridge that doesn't comply with 
this requirement (is there even other LVDS decoder bridge drivers in mainline 
?) then we'll implement bus format support in that bridge driver. It will not 
break any existing platform.

Looking at the connector won't help as the connector will likely not report an 
LVDS bus format if there's an LVDS decoder bridge in the chain.

However, given that the Lager board currently uses a DT hack and connects the 
DU LVDS output to an HDMI encoder without modeling the on-board LVDS decoder 
in DT, we need to ensure that backward compatibility is preserved by not 
returning a fatal error if the next bridge doesn't report bus formats.

> That's a point that should be discussed imho.
> 
> Daniel in his reply to your original series suggested format should be
> reported by bridges when they get registered [1]. I have not enforced that
> though, as it requires all mainline bridge drivers (which are not that
> many -yet-) to register a format before bridge_add(). If there is
> consensus on this, all other drivers should be updated (I'm not even
> sure it is possible for all of them honestly).

I wouldn't make it mandatory yet, because I'm also not sure if all bridges 
accept a single input format.

> Anyway, the only thing I'm sure is that I don't want to make this
> driver 'special', I would prefer that in case format is not there,
> we'll ignore the LVDS mode configuration as it was before this series.

Yes, that seems the best option to me. You can print a warning message to 
ensure that DT and/or bridge drivers gets updated, but we should default to 
the same LVDS mode as before.

> [1] https://lkml.org/lkml/2018/4/4/50
> 
> >>>> +	if (ret)
> >>>> +		return;
> >>>> +
> >>>> +	switch (bus_fmt) {
> >>>> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE:
> >>>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE:
> >>>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>> 
> >>>>  	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> >>>> 
> >>>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> >>>> -		mode = RCAR_LVDS_MODE_JEIDA;
> >>>> +		lvds->mode = RCAR_LVDS_MODE_JEIDA;
> >>> 
> >>> This is b0rken, first the mirror bit is ORed into some unknown
> >>> preexisting value, then the code falls through (without any fall
> >>> through comment, btw) and forcibly sets the mode, thus discarding the
> >>> mirror bit which was carefully ORed in.
> >> 
> >> You are correct, the second assignment should have been an |= and not
> >> a plain assignment. The variable is 0ed though, as 'struct rcar_lvds
> >> *lvds' is kzalloc-ed in probe function.
> > 
> > The code would be clearer if you explicitly zeroed the mode in this
> > function. Or do you rely on this function to not clobber other bits?
> > In that case apply some bit-mask.
> >
> >>>>  		break;
> >>>> +
> >>>> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE:
> >>>> +		lvds->mode |= RCAR_LVDS_MODE_MIRROR;
> >>>> 
> >>>>  	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> >>>> -		mode = RCAR_LVDS_MODE_VESA;
> >>>> +		lvds->mode = RCAR_LVDS_MODE_VESA;
> >>> 
> >>> Dito.
> >>> 
> >>>>  		break;
> >>>>  	
> >>>>  	default:
> >>>>  		dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> >>>> -			info->bus_formats[0]);
> >>>> -		return;
> >>>> +			bus_fmt);
> >>>>  	}
> >>>> -
> >>>> -	if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> >>>> -		mode |= RCAR_LVDS_MODE_MIRROR;
> >>>> -
> >>>> -	lvds->mode = mode;
> >>>>  }
> >>>>  
> >>>>  static void rcar_lvds_mode_set(struct drm_bridge *bridge,

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] media: Add LE version of RGB LVDS formats

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:06 EEST Jacopo Mondi wrote:
> Some LVDS controller can output swapped versions of LVDS RGB formats.
> Define and document them in the list of supported media bus formats

I wouldn't introduce those new formats as we don't need them. As a general 
rule we would like to have at least one user for any new format added to the 
API.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/media/uapi/v4l/subdev-formats.rst | 174 +++++++++++++++++++++
>  include/uapi/linux/media-bus-format.h           |   5 +-
>  2 files changed, 178 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst
> b/Documentation/media/uapi/v4l/subdev-formats.rst index 9fcabe7..9a5263c
> 100644
> --- a/Documentation/media/uapi/v4l/subdev-formats.rst
> +++ b/Documentation/media/uapi/v4l/subdev-formats.rst
> @@ -1669,6 +1669,64 @@ JEIDA defined bit mapping will be named
>        - b\ :sub:`2`
>        - g\ :sub:`1`
>        - r\ :sub:`0`
> +    * .. _MEDIA-BUS-FMT-RGB666-1X7X3-SPWG_LE:
> +
> +      - MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE
> +      - 0x101b
> +      - 0
> +      -
> +      -
> +      - b\ :sub:`2`
> +      - g\ :sub:`1`
> +      - r\ :sub:`0`
> +    * -
> +      -
> +      - 1
> +      -
> +      -
> +      - b\ :sub:`3`
> +      - g\ :sub:`2`
> +      - r\ :sub:`1`
> +    * -
> +      -
> +      - 2
> +      -
> +      -
> +      - b\ :sub:`4`
> +      - g\ :sub:`3`
> +      - r\ :sub:`2`
> +    * -
> +      -
> +      - 3
> +      -
> +      -
> +      - b\ :sub:`5`
> +      - g\ :sub:`4`
> +      - r\ :sub:`3`
> +    * -
> +      -
> +      - 4
> +      -
> +      -
> +      - d
> +      - g\ :sub:`5`
> +      - r\ :sub:`4`
> +    * -
> +      -
> +      - 5
> +      -
> +      -
> +      - d
> +      - b\ :sub:`0`
> +      - r\ :sub:`5`
> +    * -
> +      -
> +      - 6
> +      -
> +      -
> +      - d
> +      - b\ :sub:`1`
> +      - g\ :sub:`0`
>      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG:
> 
>        - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG
> @@ -1727,6 +1785,64 @@ JEIDA defined bit mapping will be named
>        - b\ :sub:`2`
>        - g\ :sub:`1`
>        - r\ :sub:`0`
> +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG_LE:
> +
> +      - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE
> +      - 0x101c
> +      - 0
> +      -
> +      - r\ :sub:`6`
> +      - b\ :sub:`2`
> +      - g\ :sub:`1`
> +      - r\ :sub:`0`
> +    * -
> +      -
> +      - 1
> +      -
> +      - r\ :sub:`7`
> +      - b\ :sub:`3`
> +      - g\ :sub:`2`
> +      - r\ :sub:`1`
> +    * -
> +      -
> +      - 2
> +      -
> +      - g\ :sub:`6`
> +      - b\ :sub:`4`
> +      - g\ :sub:`3`
> +      - r\ :sub:`2`
> +    * -
> +      -
> +      - 3
> +      -
> +      - g\ :sub:`7`
> +      - b\ :sub:`5`
> +      - g\ :sub:`4`
> +      - r\ :sub:`3`
> +    * -
> +      -
> +      - 4
> +      -
> +      - b\ :sub:`6`
> +      - d
> +      - g\ :sub:`5`
> +      - r\ :sub:`4`
> +    * -
> +      -
> +      - 5
> +      -
> +      - b\ :sub:`7`
> +      - d
> +      - b\ :sub:`0`
> +      - r\ :sub:`5`
> +    * -
> +      -
> +      - 6
> +      -
> +      - d
> +      - d
> +      - b\ :sub:`1`
> +      - g\ :sub:`0`
>      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA:
> 
>        - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA
> @@ -1785,6 +1901,64 @@ JEIDA defined bit mapping will be named
>        - b\ :sub:`4`
>        - g\ :sub:`3`
>        - r\ :sub:`2`
> +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA_LE:
> +
> +      - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE
> +      - 0x101d
> +      - 0
> +      -
> +      - r\ :sub:`0`
> +      - b\ :sub:`4`
> +      - g\ :sub:`3`
> +      - r\ :sub:`2`
> +    * -
> +      -
> +      - 1
> +      -
> +      - r\ :sub:`1`
> +      - b\ :sub:`5`
> +      - g\ :sub:`4`
> +      - r\ :sub:`3`
> +    * -
> +      -
> +      - 2
> +      -
> +      - g\ :sub:`0`
> +      - b\ :sub:`6`
> +      - g\ :sub:`5`
> +      - r\ :sub:`4`
> +    * -
> +      -
> +      - 3
> +      -
> +      - g\ :sub:`1`
> +      - b\ :sub:`7`
> +      - g\ :sub:`6`
> +      - r\ :sub:`5`
> +    * -
> +      -
> +      - 4
> +      -
> +      - b\ :sub:`0`
> +      - d
> +      - g\ :sub:`7`
> +      - r\ :sub:`6`
> +    * -
> +      -
> +      - 5
> +      -
> +      - b\ :sub:`1`
> +      - d
> +      - b\ :sub:`2`
> +      - r\ :sub:`7`
> +    * -
> +      -
> +      - 6
> +      -
> +      - d
> +      - d
> +      - b\ :sub:`3`
> +      - g\ :sub:`2`
> 
>  .. raw:: latex
> 
> diff --git a/include/uapi/linux/media-bus-format.h
> b/include/uapi/linux/media-bus-format.h index 9e35117..5bea7c0 100644
> --- a/include/uapi/linux/media-bus-format.h
> +++ b/include/uapi/linux/media-bus-format.h
> @@ -34,7 +34,7 @@
> 
>  #define MEDIA_BUS_FMT_FIXED			0x0001
> 
> -/* RGB - next is	0x101b */
> +/* RGB - next is	0x101f */
>  #define MEDIA_BUS_FMT_RGB444_1X12		0x1016
>  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE	0x1001
>  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE	0x1002
> @@ -49,13 +49,16 @@
>  #define MEDIA_BUS_FMT_RBG888_1X24		0x100e
>  #define MEDIA_BUS_FMT_RGB666_1X24_CPADHI	0x1015
>  #define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG		0x1010
> +#define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE	0x101b
>  #define MEDIA_BUS_FMT_BGR888_1X24		0x1013
>  #define MEDIA_BUS_FMT_GBR888_1X24		0x1014
>  #define MEDIA_BUS_FMT_RGB888_1X24		0x100a
>  #define MEDIA_BUS_FMT_RGB888_2X12_BE		0x100b
>  #define MEDIA_BUS_FMT_RGB888_2X12_LE		0x100c
>  #define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG		0x1011
> +#define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE	0x101c
>  #define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA	0x1012
> +#define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE	0x101d
>  #define MEDIA_BUS_FMT_ARGB8888_1X32		0x100d
>  #define MEDIA_BUS_FMT_RGB888_1X32_PADHI		0x100f
>  #define MEDIA_BUS_FMT_RGB101010_1X30		0x1018

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/8] media: Add LE version of RGB LVDS formats

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:06 EEST Jacopo Mondi wrote:
> Some LVDS controller can output swapped versions of LVDS RGB formats.
> Define and document them in the list of supported media bus formats

I wouldn't introduce those new formats as we don't need them. As a general 
rule we would like to have at least one user for any new format added to the 
API.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/media/uapi/v4l/subdev-formats.rst | 174 +++++++++++++++++++++
>  include/uapi/linux/media-bus-format.h           |   5 +-
>  2 files changed, 178 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst
> b/Documentation/media/uapi/v4l/subdev-formats.rst index 9fcabe7..9a5263c
> 100644
> --- a/Documentation/media/uapi/v4l/subdev-formats.rst
> +++ b/Documentation/media/uapi/v4l/subdev-formats.rst
> @@ -1669,6 +1669,64 @@ JEIDA defined bit mapping will be named
>        - b\ :sub:`2`
>        - g\ :sub:`1`
>        - r\ :sub:`0`
> +    * .. _MEDIA-BUS-FMT-RGB666-1X7X3-SPWG_LE:
> +
> +      - MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE
> +      - 0x101b
> +      - 0
> +      -
> +      -
> +      - b\ :sub:`2`
> +      - g\ :sub:`1`
> +      - r\ :sub:`0`
> +    * -
> +      -
> +      - 1
> +      -
> +      -
> +      - b\ :sub:`3`
> +      - g\ :sub:`2`
> +      - r\ :sub:`1`
> +    * -
> +      -
> +      - 2
> +      -
> +      -
> +      - b\ :sub:`4`
> +      - g\ :sub:`3`
> +      - r\ :sub:`2`
> +    * -
> +      -
> +      - 3
> +      -
> +      -
> +      - b\ :sub:`5`
> +      - g\ :sub:`4`
> +      - r\ :sub:`3`
> +    * -
> +      -
> +      - 4
> +      -
> +      -
> +      - d
> +      - g\ :sub:`5`
> +      - r\ :sub:`4`
> +    * -
> +      -
> +      - 5
> +      -
> +      -
> +      - d
> +      - b\ :sub:`0`
> +      - r\ :sub:`5`
> +    * -
> +      -
> +      - 6
> +      -
> +      -
> +      - d
> +      - b\ :sub:`1`
> +      - g\ :sub:`0`
>      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG:
> 
>        - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG
> @@ -1727,6 +1785,64 @@ JEIDA defined bit mapping will be named
>        - b\ :sub:`2`
>        - g\ :sub:`1`
>        - r\ :sub:`0`
> +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG_LE:
> +
> +      - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE
> +      - 0x101c
> +      - 0
> +      -
> +      - r\ :sub:`6`
> +      - b\ :sub:`2`
> +      - g\ :sub:`1`
> +      - r\ :sub:`0`
> +    * -
> +      -
> +      - 1
> +      -
> +      - r\ :sub:`7`
> +      - b\ :sub:`3`
> +      - g\ :sub:`2`
> +      - r\ :sub:`1`
> +    * -
> +      -
> +      - 2
> +      -
> +      - g\ :sub:`6`
> +      - b\ :sub:`4`
> +      - g\ :sub:`3`
> +      - r\ :sub:`2`
> +    * -
> +      -
> +      - 3
> +      -
> +      - g\ :sub:`7`
> +      - b\ :sub:`5`
> +      - g\ :sub:`4`
> +      - r\ :sub:`3`
> +    * -
> +      -
> +      - 4
> +      -
> +      - b\ :sub:`6`
> +      - d
> +      - g\ :sub:`5`
> +      - r\ :sub:`4`
> +    * -
> +      -
> +      - 5
> +      -
> +      - b\ :sub:`7`
> +      - d
> +      - b\ :sub:`0`
> +      - r\ :sub:`5`
> +    * -
> +      -
> +      - 6
> +      -
> +      - d
> +      - d
> +      - b\ :sub:`1`
> +      - g\ :sub:`0`
>      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA:
> 
>        - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA
> @@ -1785,6 +1901,64 @@ JEIDA defined bit mapping will be named
>        - b\ :sub:`4`
>        - g\ :sub:`3`
>        - r\ :sub:`2`
> +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA_LE:
> +
> +      - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE
> +      - 0x101d
> +      - 0
> +      -
> +      - r\ :sub:`0`
> +      - b\ :sub:`4`
> +      - g\ :sub:`3`
> +      - r\ :sub:`2`
> +    * -
> +      -
> +      - 1
> +      -
> +      - r\ :sub:`1`
> +      - b\ :sub:`5`
> +      - g\ :sub:`4`
> +      - r\ :sub:`3`
> +    * -
> +      -
> +      - 2
> +      -
> +      - g\ :sub:`0`
> +      - b\ :sub:`6`
> +      - g\ :sub:`5`
> +      - r\ :sub:`4`
> +    * -
> +      -
> +      - 3
> +      -
> +      - g\ :sub:`1`
> +      - b\ :sub:`7`
> +      - g\ :sub:`6`
> +      - r\ :sub:`5`
> +    * -
> +      -
> +      - 4
> +      -
> +      - b\ :sub:`0`
> +      - d
> +      - g\ :sub:`7`
> +      - r\ :sub:`6`
> +    * -
> +      -
> +      - 5
> +      -
> +      - b\ :sub:`1`
> +      - d
> +      - b\ :sub:`2`
> +      - r\ :sub:`7`
> +    * -
> +      -
> +      - 6
> +      -
> +      - d
> +      - d
> +      - b\ :sub:`3`
> +      - g\ :sub:`2`
> 
>  .. raw:: latex
> 
> diff --git a/include/uapi/linux/media-bus-format.h
> b/include/uapi/linux/media-bus-format.h index 9e35117..5bea7c0 100644
> --- a/include/uapi/linux/media-bus-format.h
> +++ b/include/uapi/linux/media-bus-format.h
> @@ -34,7 +34,7 @@
> 
>  #define MEDIA_BUS_FMT_FIXED			0x0001
> 
> -/* RGB - next is	0x101b */
> +/* RGB - next is	0x101f */
>  #define MEDIA_BUS_FMT_RGB444_1X12		0x1016
>  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE	0x1001
>  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE	0x1002
> @@ -49,13 +49,16 @@
>  #define MEDIA_BUS_FMT_RBG888_1X24		0x100e
>  #define MEDIA_BUS_FMT_RGB666_1X24_CPADHI	0x1015
>  #define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG		0x1010
> +#define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE	0x101b
>  #define MEDIA_BUS_FMT_BGR888_1X24		0x1013
>  #define MEDIA_BUS_FMT_GBR888_1X24		0x1014
>  #define MEDIA_BUS_FMT_RGB888_1X24		0x100a
>  #define MEDIA_BUS_FMT_RGB888_2X12_BE		0x100b
>  #define MEDIA_BUS_FMT_RGB888_2X12_LE		0x100c
>  #define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG		0x1011
> +#define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE	0x101c
>  #define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA	0x1012
> +#define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE	0x101d
>  #define MEDIA_BUS_FMT_ARGB8888_1X32		0x100d
>  #define MEDIA_BUS_FMT_RGB888_1X32_PADHI		0x100f
>  #define MEDIA_BUS_FMT_RGB101010_1X30		0x1018

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] arm64: dts: renesas: eagle: Add thc63 LVDS map

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:05 EEST Jacopo Mondi wrote:
> Add LVDS map mode description property to THC63LVD1024 LVDS decoder in
> R-Car V3M-Eagle board device tree.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index ebfbb51..2609fa3
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -56,6 +56,7 @@
>  		compatible = "thine,thc63lvd1024";
> 
>  		vcc-supply = <&d3p3>;
> +		thine,map = <1>;
> 
>  		ports {
>  			#address-cells = <1>;


-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/8] arm64: dts: renesas: eagle: Add thc63 LVDS map

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:05 EEST Jacopo Mondi wrote:
> Add LVDS map mode description property to THC63LVD1024 LVDS decoder in
> R-Car V3M-Eagle board device tree.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index ebfbb51..2609fa3
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -56,6 +56,7 @@
>  		compatible = "thine,thc63lvd1024";
> 
>  		vcc-supply = <&d3p3>;
> +		thine,map = <1>;
> 
>  		ports {
>  			#address-cells = <1>;


-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm: connector: Remove DRM_BUS_FLAG_DATA_* flags

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:09 EEST Jacopo Mondi wrote:
> DRM_BUS_FLAG_DATA_* flags, defined in drm_connector.h header file are
> used to swap ordering of LVDS RGB format to accommodate DRM objects
> that need to handle LVDS components ordering.
> 
> Now that the only 2 users of DRM_BUS_FLAG_DATA_* flags have been ported
> to use the newly introduced MEDIA_BUS_FMT_RGB888_1X7X*_LE media bus
> formats, remove them.

I'm not opposed to this (despite my review of patch 5/8), but I think the _LE 
suffix isn't the right name for the new formats. _BE and _LE relate to byte 
swapping, while here you really need to describe full mirroring. Maybe a 
_MIRROR variant would be more appropriate ?

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/drm/drm_connector.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 675cc3f..9e0d6d5 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -286,10 +286,6 @@ struct drm_display_info {
>  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
>  /* drive data on neg. edge */
>  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> -/* data is transmitted MSB to LSB on the bus */
> -#define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
> -/* data is transmitted LSB to MSB on the bus */
> -#define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> 
>  	/**
>  	 * @bus_flags: Additional information (like pixel signal polarity) for


-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] drm: connector: Remove DRM_BUS_FLAG_DATA_* flags

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:09 EEST Jacopo Mondi wrote:
> DRM_BUS_FLAG_DATA_* flags, defined in drm_connector.h header file are
> used to swap ordering of LVDS RGB format to accommodate DRM objects
> that need to handle LVDS components ordering.
> 
> Now that the only 2 users of DRM_BUS_FLAG_DATA_* flags have been ported
> to use the newly introduced MEDIA_BUS_FMT_RGB888_1X7X*_LE media bus
> formats, remove them.

I'm not opposed to this (despite my review of patch 5/8), but I think the _LE 
suffix isn't the right name for the new formats. _BE and _LE relate to byte 
swapping, while here you really need to describe full mirroring. Maybe a 
_MIRROR variant would be more appropriate ?

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/drm/drm_connector.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 675cc3f..9e0d6d5 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -286,10 +286,6 @@ struct drm_display_info {
>  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
>  /* drive data on neg. edge */
>  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> -/* data is transmitted MSB to LSB on the bus */
> -#define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
> -/* data is transmitted LSB to MSB on the bus */
> -#define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> 
>  	/**
>  	 * @bus_flags: Additional information (like pixel signal polarity) for


-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] media: Add LE version of RGB LVDS formats

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 6886 bytes --]

HI Laurent,

On Mon, Apr 23, 2018 at 04:06:01PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:06 EEST Jacopo Mondi wrote:
> > Some LVDS controller can output swapped versions of LVDS RGB formats.
> > Define and document them in the list of supported media bus formats
>
> I wouldn't introduce those new formats as we don't need them. As a general
> rule we would like to have at least one user for any new format added to the
> API.

I was about to point you to patch [8/8], as the newly introduced
formats allow replacing the DRM_BUS_FLAG_DATA_ flags, defined in
drm_connector.h and which I struggled to find a more appropiate place
where to move them. Or I either duplicate them for bridges, but I
prefer not to, or we remove them, and defining some dedicated formats,
seems more natural to me...

I'll reply to your comment on [8/8] on the format names and other
details.

Thanks
   j

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/media/uapi/v4l/subdev-formats.rst | 174 +++++++++++++++++++++
> >  include/uapi/linux/media-bus-format.h           |   5 +-
> >  2 files changed, 178 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst
> > b/Documentation/media/uapi/v4l/subdev-formats.rst index 9fcabe7..9a5263c
> > 100644
> > --- a/Documentation/media/uapi/v4l/subdev-formats.rst
> > +++ b/Documentation/media/uapi/v4l/subdev-formats.rst
> > @@ -1669,6 +1669,64 @@ JEIDA defined bit mapping will be named
> >        - b\ :sub:`2`
> >        - g\ :sub:`1`
> >        - r\ :sub:`0`
> > +    * .. _MEDIA-BUS-FMT-RGB666-1X7X3-SPWG_LE:
> > +
> > +      - MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE
> > +      - 0x101b
> > +      - 0
> > +      -
> > +      -
> > +      - b\ :sub:`2`
> > +      - g\ :sub:`1`
> > +      - r\ :sub:`0`
> > +    * -
> > +      -
> > +      - 1
> > +      -
> > +      -
> > +      - b\ :sub:`3`
> > +      - g\ :sub:`2`
> > +      - r\ :sub:`1`
> > +    * -
> > +      -
> > +      - 2
> > +      -
> > +      -
> > +      - b\ :sub:`4`
> > +      - g\ :sub:`3`
> > +      - r\ :sub:`2`
> > +    * -
> > +      -
> > +      - 3
> > +      -
> > +      -
> > +      - b\ :sub:`5`
> > +      - g\ :sub:`4`
> > +      - r\ :sub:`3`
> > +    * -
> > +      -
> > +      - 4
> > +      -
> > +      -
> > +      - d
> > +      - g\ :sub:`5`
> > +      - r\ :sub:`4`
> > +    * -
> > +      -
> > +      - 5
> > +      -
> > +      -
> > +      - d
> > +      - b\ :sub:`0`
> > +      - r\ :sub:`5`
> > +    * -
> > +      -
> > +      - 6
> > +      -
> > +      -
> > +      - d
> > +      - b\ :sub:`1`
> > +      - g\ :sub:`0`
> >      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG:
> >
> >        - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG
> > @@ -1727,6 +1785,64 @@ JEIDA defined bit mapping will be named
> >        - b\ :sub:`2`
> >        - g\ :sub:`1`
> >        - r\ :sub:`0`
> > +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG_LE:
> > +
> > +      - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE
> > +      - 0x101c
> > +      - 0
> > +      -
> > +      - r\ :sub:`6`
> > +      - b\ :sub:`2`
> > +      - g\ :sub:`1`
> > +      - r\ :sub:`0`
> > +    * -
> > +      -
> > +      - 1
> > +      -
> > +      - r\ :sub:`7`
> > +      - b\ :sub:`3`
> > +      - g\ :sub:`2`
> > +      - r\ :sub:`1`
> > +    * -
> > +      -
> > +      - 2
> > +      -
> > +      - g\ :sub:`6`
> > +      - b\ :sub:`4`
> > +      - g\ :sub:`3`
> > +      - r\ :sub:`2`
> > +    * -
> > +      -
> > +      - 3
> > +      -
> > +      - g\ :sub:`7`
> > +      - b\ :sub:`5`
> > +      - g\ :sub:`4`
> > +      - r\ :sub:`3`
> > +    * -
> > +      -
> > +      - 4
> > +      -
> > +      - b\ :sub:`6`
> > +      - d
> > +      - g\ :sub:`5`
> > +      - r\ :sub:`4`
> > +    * -
> > +      -
> > +      - 5
> > +      -
> > +      - b\ :sub:`7`
> > +      - d
> > +      - b\ :sub:`0`
> > +      - r\ :sub:`5`
> > +    * -
> > +      -
> > +      - 6
> > +      -
> > +      - d
> > +      - d
> > +      - b\ :sub:`1`
> > +      - g\ :sub:`0`
> >      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA:
> >
> >        - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA
> > @@ -1785,6 +1901,64 @@ JEIDA defined bit mapping will be named
> >        - b\ :sub:`4`
> >        - g\ :sub:`3`
> >        - r\ :sub:`2`
> > +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA_LE:
> > +
> > +      - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE
> > +      - 0x101d
> > +      - 0
> > +      -
> > +      - r\ :sub:`0`
> > +      - b\ :sub:`4`
> > +      - g\ :sub:`3`
> > +      - r\ :sub:`2`
> > +    * -
> > +      -
> > +      - 1
> > +      -
> > +      - r\ :sub:`1`
> > +      - b\ :sub:`5`
> > +      - g\ :sub:`4`
> > +      - r\ :sub:`3`
> > +    * -
> > +      -
> > +      - 2
> > +      -
> > +      - g\ :sub:`0`
> > +      - b\ :sub:`6`
> > +      - g\ :sub:`5`
> > +      - r\ :sub:`4`
> > +    * -
> > +      -
> > +      - 3
> > +      -
> > +      - g\ :sub:`1`
> > +      - b\ :sub:`7`
> > +      - g\ :sub:`6`
> > +      - r\ :sub:`5`
> > +    * -
> > +      -
> > +      - 4
> > +      -
> > +      - b\ :sub:`0`
> > +      - d
> > +      - g\ :sub:`7`
> > +      - r\ :sub:`6`
> > +    * -
> > +      -
> > +      - 5
> > +      -
> > +      - b\ :sub:`1`
> > +      - d
> > +      - b\ :sub:`2`
> > +      - r\ :sub:`7`
> > +    * -
> > +      -
> > +      - 6
> > +      -
> > +      - d
> > +      - d
> > +      - b\ :sub:`3`
> > +      - g\ :sub:`2`
> >
> >  .. raw:: latex
> >
> > diff --git a/include/uapi/linux/media-bus-format.h
> > b/include/uapi/linux/media-bus-format.h index 9e35117..5bea7c0 100644
> > --- a/include/uapi/linux/media-bus-format.h
> > +++ b/include/uapi/linux/media-bus-format.h
> > @@ -34,7 +34,7 @@
> >
> >  #define MEDIA_BUS_FMT_FIXED			0x0001
> >
> > -/* RGB - next is	0x101b */
> > +/* RGB - next is	0x101f */
> >  #define MEDIA_BUS_FMT_RGB444_1X12		0x1016
> >  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE	0x1001
> >  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE	0x1002
> > @@ -49,13 +49,16 @@
> >  #define MEDIA_BUS_FMT_RBG888_1X24		0x100e
> >  #define MEDIA_BUS_FMT_RGB666_1X24_CPADHI	0x1015
> >  #define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG		0x1010
> > +#define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE	0x101b
> >  #define MEDIA_BUS_FMT_BGR888_1X24		0x1013
> >  #define MEDIA_BUS_FMT_GBR888_1X24		0x1014
> >  #define MEDIA_BUS_FMT_RGB888_1X24		0x100a
> >  #define MEDIA_BUS_FMT_RGB888_2X12_BE		0x100b
> >  #define MEDIA_BUS_FMT_RGB888_2X12_LE		0x100c
> >  #define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG		0x1011
> > +#define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE	0x101c
> >  #define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA	0x1012
> > +#define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE	0x101d
> >  #define MEDIA_BUS_FMT_ARGB8888_1X32		0x100d
> >  #define MEDIA_BUS_FMT_RGB888_1X32_PADHI		0x100f
> >  #define MEDIA_BUS_FMT_RGB101010_1X30		0x1018
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/8] media: Add LE version of RGB LVDS formats

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 6886 bytes --]

HI Laurent,

On Mon, Apr 23, 2018 at 04:06:01PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:06 EEST Jacopo Mondi wrote:
> > Some LVDS controller can output swapped versions of LVDS RGB formats.
> > Define and document them in the list of supported media bus formats
>
> I wouldn't introduce those new formats as we don't need them. As a general
> rule we would like to have at least one user for any new format added to the
> API.

I was about to point you to patch [8/8], as the newly introduced
formats allow replacing the DRM_BUS_FLAG_DATA_ flags, defined in
drm_connector.h and which I struggled to find a more appropiate place
where to move them. Or I either duplicate them for bridges, but I
prefer not to, or we remove them, and defining some dedicated formats,
seems more natural to me...

I'll reply to your comment on [8/8] on the format names and other
details.

Thanks
   j

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/media/uapi/v4l/subdev-formats.rst | 174 +++++++++++++++++++++
> >  include/uapi/linux/media-bus-format.h           |   5 +-
> >  2 files changed, 178 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst
> > b/Documentation/media/uapi/v4l/subdev-formats.rst index 9fcabe7..9a5263c
> > 100644
> > --- a/Documentation/media/uapi/v4l/subdev-formats.rst
> > +++ b/Documentation/media/uapi/v4l/subdev-formats.rst
> > @@ -1669,6 +1669,64 @@ JEIDA defined bit mapping will be named
> >        - b\ :sub:`2`
> >        - g\ :sub:`1`
> >        - r\ :sub:`0`
> > +    * .. _MEDIA-BUS-FMT-RGB666-1X7X3-SPWG_LE:
> > +
> > +      - MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE
> > +      - 0x101b
> > +      - 0
> > +      -
> > +      -
> > +      - b\ :sub:`2`
> > +      - g\ :sub:`1`
> > +      - r\ :sub:`0`
> > +    * -
> > +      -
> > +      - 1
> > +      -
> > +      -
> > +      - b\ :sub:`3`
> > +      - g\ :sub:`2`
> > +      - r\ :sub:`1`
> > +    * -
> > +      -
> > +      - 2
> > +      -
> > +      -
> > +      - b\ :sub:`4`
> > +      - g\ :sub:`3`
> > +      - r\ :sub:`2`
> > +    * -
> > +      -
> > +      - 3
> > +      -
> > +      -
> > +      - b\ :sub:`5`
> > +      - g\ :sub:`4`
> > +      - r\ :sub:`3`
> > +    * -
> > +      -
> > +      - 4
> > +      -
> > +      -
> > +      - d
> > +      - g\ :sub:`5`
> > +      - r\ :sub:`4`
> > +    * -
> > +      -
> > +      - 5
> > +      -
> > +      -
> > +      - d
> > +      - b\ :sub:`0`
> > +      - r\ :sub:`5`
> > +    * -
> > +      -
> > +      - 6
> > +      -
> > +      -
> > +      - d
> > +      - b\ :sub:`1`
> > +      - g\ :sub:`0`
> >      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG:
> >
> >        - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG
> > @@ -1727,6 +1785,64 @@ JEIDA defined bit mapping will be named
> >        - b\ :sub:`2`
> >        - g\ :sub:`1`
> >        - r\ :sub:`0`
> > +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-SPWG_LE:
> > +
> > +      - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE
> > +      - 0x101c
> > +      - 0
> > +      -
> > +      - r\ :sub:`6`
> > +      - b\ :sub:`2`
> > +      - g\ :sub:`1`
> > +      - r\ :sub:`0`
> > +    * -
> > +      -
> > +      - 1
> > +      -
> > +      - r\ :sub:`7`
> > +      - b\ :sub:`3`
> > +      - g\ :sub:`2`
> > +      - r\ :sub:`1`
> > +    * -
> > +      -
> > +      - 2
> > +      -
> > +      - g\ :sub:`6`
> > +      - b\ :sub:`4`
> > +      - g\ :sub:`3`
> > +      - r\ :sub:`2`
> > +    * -
> > +      -
> > +      - 3
> > +      -
> > +      - g\ :sub:`7`
> > +      - b\ :sub:`5`
> > +      - g\ :sub:`4`
> > +      - r\ :sub:`3`
> > +    * -
> > +      -
> > +      - 4
> > +      -
> > +      - b\ :sub:`6`
> > +      - d
> > +      - g\ :sub:`5`
> > +      - r\ :sub:`4`
> > +    * -
> > +      -
> > +      - 5
> > +      -
> > +      - b\ :sub:`7`
> > +      - d
> > +      - b\ :sub:`0`
> > +      - r\ :sub:`5`
> > +    * -
> > +      -
> > +      - 6
> > +      -
> > +      - d
> > +      - d
> > +      - b\ :sub:`1`
> > +      - g\ :sub:`0`
> >      * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA:
> >
> >        - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA
> > @@ -1785,6 +1901,64 @@ JEIDA defined bit mapping will be named
> >        - b\ :sub:`4`
> >        - g\ :sub:`3`
> >        - r\ :sub:`2`
> > +    * .. _MEDIA-BUS-FMT-RGB888-1X7X4-JEIDA_LE:
> > +
> > +      - MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE
> > +      - 0x101d
> > +      - 0
> > +      -
> > +      - r\ :sub:`0`
> > +      - b\ :sub:`4`
> > +      - g\ :sub:`3`
> > +      - r\ :sub:`2`
> > +    * -
> > +      -
> > +      - 1
> > +      -
> > +      - r\ :sub:`1`
> > +      - b\ :sub:`5`
> > +      - g\ :sub:`4`
> > +      - r\ :sub:`3`
> > +    * -
> > +      -
> > +      - 2
> > +      -
> > +      - g\ :sub:`0`
> > +      - b\ :sub:`6`
> > +      - g\ :sub:`5`
> > +      - r\ :sub:`4`
> > +    * -
> > +      -
> > +      - 3
> > +      -
> > +      - g\ :sub:`1`
> > +      - b\ :sub:`7`
> > +      - g\ :sub:`6`
> > +      - r\ :sub:`5`
> > +    * -
> > +      -
> > +      - 4
> > +      -
> > +      - b\ :sub:`0`
> > +      - d
> > +      - g\ :sub:`7`
> > +      - r\ :sub:`6`
> > +    * -
> > +      -
> > +      - 5
> > +      -
> > +      - b\ :sub:`1`
> > +      - d
> > +      - b\ :sub:`2`
> > +      - r\ :sub:`7`
> > +    * -
> > +      -
> > +      - 6
> > +      -
> > +      - d
> > +      - d
> > +      - b\ :sub:`3`
> > +      - g\ :sub:`2`
> >
> >  .. raw:: latex
> >
> > diff --git a/include/uapi/linux/media-bus-format.h
> > b/include/uapi/linux/media-bus-format.h index 9e35117..5bea7c0 100644
> > --- a/include/uapi/linux/media-bus-format.h
> > +++ b/include/uapi/linux/media-bus-format.h
> > @@ -34,7 +34,7 @@
> >
> >  #define MEDIA_BUS_FMT_FIXED			0x0001
> >
> > -/* RGB - next is	0x101b */
> > +/* RGB - next is	0x101f */
> >  #define MEDIA_BUS_FMT_RGB444_1X12		0x1016
> >  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE	0x1001
> >  #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE	0x1002
> > @@ -49,13 +49,16 @@
> >  #define MEDIA_BUS_FMT_RBG888_1X24		0x100e
> >  #define MEDIA_BUS_FMT_RGB666_1X24_CPADHI	0x1015
> >  #define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG		0x1010
> > +#define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG_LE	0x101b
> >  #define MEDIA_BUS_FMT_BGR888_1X24		0x1013
> >  #define MEDIA_BUS_FMT_GBR888_1X24		0x1014
> >  #define MEDIA_BUS_FMT_RGB888_1X24		0x100a
> >  #define MEDIA_BUS_FMT_RGB888_2X12_BE		0x100b
> >  #define MEDIA_BUS_FMT_RGB888_2X12_LE		0x100c
> >  #define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG		0x1011
> > +#define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG_LE	0x101c
> >  #define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA	0x1012
> > +#define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA_LE	0x101d
> >  #define MEDIA_BUS_FMT_ARGB8888_1X32		0x100d
> >  #define MEDIA_BUS_FMT_RGB888_1X32_PADHI		0x100f
> >  #define MEDIA_BUS_FMT_RGB101010_1X30		0x1018
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm: connector: Remove DRM_BUS_FLAG_DATA_* flags

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

Hi Laurent,

On Tue, Apr 24, 2018 at 12:03:04AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:09 EEST Jacopo Mondi wrote:
> > DRM_BUS_FLAG_DATA_* flags, defined in drm_connector.h header file are
> > used to swap ordering of LVDS RGB format to accommodate DRM objects
> > that need to handle LVDS components ordering.
> >
> > Now that the only 2 users of DRM_BUS_FLAG_DATA_* flags have been ported
> > to use the newly introduced MEDIA_BUS_FMT_RGB888_1X7X*_LE media bus
> > formats, remove them.
>
> I'm not opposed to this (despite my review of patch 5/8), but I think the _LE
> suffix isn't the right name for the new formats. _BE and _LE relate to byte
> swapping, while here you really need to describe full mirroring. Maybe a
> _MIRROR variant would be more appropriate ?

As I anticipated in the cover letter, I agree the BE/LE distinction
does not apply well for LVDS formats. I chose to use _LE anyhow as
there are no other format variants defined in media-bus-format.h

I'm open to use either of those suffixes btw, what presses me is to
get rid of those flags only defined in drm_connector.h

thanks
   j

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/drm/drm_connector.h | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 675cc3f..9e0d6d5 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -286,10 +286,6 @@ struct drm_display_info {
> >  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> >  /* drive data on neg. edge */
> >  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> > -/* data is transmitted MSB to LSB on the bus */
> > -#define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
> > -/* data is transmitted LSB to MSB on the bus */
> > -#define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> >
> >  	/**
> >  	 * @bus_flags: Additional information (like pixel signal polarity) for
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 8/8] drm: connector: Remove DRM_BUS_FLAG_DATA_* flags

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 2043 bytes --]

Hi Laurent,

On Tue, Apr 24, 2018 at 12:03:04AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:09 EEST Jacopo Mondi wrote:
> > DRM_BUS_FLAG_DATA_* flags, defined in drm_connector.h header file are
> > used to swap ordering of LVDS RGB format to accommodate DRM objects
> > that need to handle LVDS components ordering.
> >
> > Now that the only 2 users of DRM_BUS_FLAG_DATA_* flags have been ported
> > to use the newly introduced MEDIA_BUS_FMT_RGB888_1X7X*_LE media bus
> > formats, remove them.
>
> I'm not opposed to this (despite my review of patch 5/8), but I think the _LE
> suffix isn't the right name for the new formats. _BE and _LE relate to byte
> swapping, while here you really need to describe full mirroring. Maybe a
> _MIRROR variant would be more appropriate ?

As I anticipated in the cover letter, I agree the BE/LE distinction
does not apply well for LVDS formats. I chose to use _LE anyhow as
there are no other format variants defined in media-bus-format.h

I'm open to use either of those suffixes btw, what presses me is to
get rid of those flags only defined in drm_connector.h

thanks
   j

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/drm/drm_connector.h | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 675cc3f..9e0d6d5 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -286,10 +286,6 @@ struct drm_display_info {
> >  #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> >  /* drive data on neg. edge */
> >  #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> > -/* data is transmitted MSB to LSB on the bus */
> > -#define DRM_BUS_FLAG_DATA_MSB_TO_LSB	(1<<4)
> > -/* data is transmitted LSB to MSB on the bus */
> > -#define DRM_BUS_FLAG_DATA_LSB_TO_MSB	(1<<5)
> >
> >  	/**
> >  	 * @bus_flags: Additional information (like pixel signal polarity) for
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Rob Herring]

On Thu, Apr 19, 2018 at 11:31:03AM +0200, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> modes, selectable by means of an external pin.
> 
> Describe the LVDS mode map through a newly defined mandatory property in
> device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3 +++
>  1 file changed, 3 insertions(+)

+1 for what Laurent said.

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/8] dt-bindings: display: bridge: thc63lvd1024: Add lvds map property

[Rob Herring]

On Thu, Apr 19, 2018 at 11:31:03AM +0200, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different input mapping
> modes, selectable by means of an external pin.
> 
> Describe the LVDS mode map through a newly defined mandatory property in
> device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/display/bridge/thine,thc63lvd1024.txt          | 3 +++
>  1 file changed, 3 insertions(+)

+1 for what Laurent said.

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4069 bytes --]

Hi Laurent,

On Mon, Apr 23, 2018 at 12:27:39PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> > Add support for storing image format information in DRM bridges with
> > associated helper function.
> >
> > This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> > is for connectors.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  8 ++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe..e2ad098 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  }
> >
> >  /**
> > + * drm_bridge_set_bus_formats() - set bridge supported image formats
> > + * @bridge: the bridge to set image formats in
> > + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
>
> Why the \ (here and below) ?

mmm, I can't tell where I made that up from... Will change with
MEDIA_BUS_FMT_*

>
> > + * @num_formats: number of elements in the @formats array
> > + *
> > + * Store a list of supported image formats in a bridge.
> > + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h
> > for
> > + * a full list of available formats.
> > + */
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> > *formats,
> > +			       unsigned int num_formats)
> > +{
> > +	u32 *fmts;
> > +
> > +	if (!formats || !num_formats)
> > +		return -EINVAL;
> > +
> > +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
>
> This memory will be leaked when the bridge is destroyed.

Right. I'm afraid this may open a pandora box, but, ehm, where is the
bridge objects lifetime managed? The best I can think of is to use the
resource managed version of kmemdup, associating that memory to
the drm_device device object. That means the memory will be freed at
DRM pipeline teardown time only if I'm not wrong. Can a bridge be
destroyed before that?

Thanks
   j

>
> > +	if (!fmts)
> > +		return -ENOMEM;
> > +
> > +	kfree(bridge->bus_formats);
> > +	bridge->bus_formats = fmts;
> > +	bridge->num_bus_formats = num_formats;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> > +
> > +/**
> >   * DOC: bridge callbacks
> >   *
> >   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec..6b3648c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >   * @encoder: encoder to which this bridge is connected
> >   * @next: the next bridge in the encoder chain
> >   * @of_node: device node pointer to the bridge
> > + * @bus_formats: wire image formats. Array of @num_bus_formats
> > MEDIA_BUS_FMT\_
> > + * elements
> > + * @num_bus_formats: size of @bus_formats array
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > @@ -271,6 +274,9 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> >  	struct device_node *of_node;
> >  #endif
> > +	const u32 *bus_formats;
> > +	unsigned int num_bus_formats;
> > +
> >  	struct list_head list;
> >  	const struct drm_bridge_timings *timings;
> >
> > @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >  			struct drm_display_mode *adjusted_mode);
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> > +			       unsigned int num_fmts);
> >
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 4069 bytes --]

Hi Laurent,

On Mon, Apr 23, 2018 at 12:27:39PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> > Add support for storing image format information in DRM bridges with
> > associated helper function.
> >
> > This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> > is for connectors.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  8 ++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe..e2ad098 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  }
> >
> >  /**
> > + * drm_bridge_set_bus_formats() - set bridge supported image formats
> > + * @bridge: the bridge to set image formats in
> > + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
>
> Why the \ (here and below) ?

mmm, I can't tell where I made that up from... Will change with
MEDIA_BUS_FMT_*

>
> > + * @num_formats: number of elements in the @formats array
> > + *
> > + * Store a list of supported image formats in a bridge.
> > + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h
> > for
> > + * a full list of available formats.
> > + */
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> > *formats,
> > +			       unsigned int num_formats)
> > +{
> > +	u32 *fmts;
> > +
> > +	if (!formats || !num_formats)
> > +		return -EINVAL;
> > +
> > +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
>
> This memory will be leaked when the bridge is destroyed.

Right. I'm afraid this may open a pandora box, but, ehm, where is the
bridge objects lifetime managed? The best I can think of is to use the
resource managed version of kmemdup, associating that memory to
the drm_device device object. That means the memory will be freed at
DRM pipeline teardown time only if I'm not wrong. Can a bridge be
destroyed before that?

Thanks
   j

>
> > +	if (!fmts)
> > +		return -ENOMEM;
> > +
> > +	kfree(bridge->bus_formats);
> > +	bridge->bus_formats = fmts;
> > +	bridge->num_bus_formats = num_formats;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> > +
> > +/**
> >   * DOC: bridge callbacks
> >   *
> >   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec..6b3648c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >   * @encoder: encoder to which this bridge is connected
> >   * @next: the next bridge in the encoder chain
> >   * @of_node: device node pointer to the bridge
> > + * @bus_formats: wire image formats. Array of @num_bus_formats
> > MEDIA_BUS_FMT\_
> > + * elements
> > + * @num_bus_formats: size of @bus_formats array
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > @@ -271,6 +274,9 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> >  	struct device_node *of_node;
> >  #endif
> > +	const u32 *bus_formats;
> > +	unsigned int num_bus_formats;
> > +
> >  	struct list_head list;
> >  	const struct drm_bridge_timings *timings;
> >
> > @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >  			struct drm_display_mode *adjusted_mode);
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> > +			       unsigned int num_fmts);
> >
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

Hi Peter,

On Sun, Apr 22, 2018 at 10:02:23PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > Add support for storing image format information in DRM bridges with
> > associated helper function.
> >
> > This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> > is for connectors.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  8 ++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe..e2ad098 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  }
> >
> >  /**
> > + * drm_bridge_set_bus_formats() - set bridge supported image formats
> > + * @bridge: the bridge to set image formats in
> > + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> > + * @num_formats: number of elements in the @formats array
> > + *
> > + * Store a list of supported image formats in a bridge.
> > + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for
> > + * a full list of available formats.
> > + */
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *formats,
> > +			       unsigned int num_formats)
> > +{
> > +	u32 *fmts;
> > +
> > +	if (!formats || !num_formats)
> > +		return -EINVAL;
>
> I see no compelling reason to forbid restoring the number of reported
> input formats to zero? I can't think of a use right now of course, but it
> seems a bit odd all the same.

It is, you're right. Will fix in v2 and will allow bridges to just
restore formats to 0 (as it is done for drm_connectors, by the way)

Thanks
   j
>
> Cheers,
> Peter
>
> > +
> > +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> > +	if (!fmts)
> > +		return -ENOMEM;
> > +
> > +	kfree(bridge->bus_formats);
> > +	bridge->bus_formats = fmts;
> > +	bridge->num_bus_formats = num_formats;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> > +
> > +/**
> >   * DOC: bridge callbacks
> >   *
> >   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec..6b3648c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >   * @encoder: encoder to which this bridge is connected
> >   * @next: the next bridge in the encoder chain
> >   * @of_node: device node pointer to the bridge
> > + * @bus_formats: wire image formats. Array of @num_bus_formats MEDIA_BUS_FMT\_
> > + * elements
> > + * @num_bus_formats: size of @bus_formats array
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > @@ -271,6 +274,9 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> >  	struct device_node *of_node;
> >  #endif
> > +	const u32 *bus_formats;
> > +	unsigned int num_bus_formats;
> > +
> >  	struct list_head list;
> >  	const struct drm_bridge_timings *timings;
> >
> > @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >  			struct drm_display_mode *adjusted_mode);
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> > +			       unsigned int num_fmts);
> >
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 3742 bytes --]

Hi Peter,

On Sun, Apr 22, 2018 at 10:02:23PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > Add support for storing image format information in DRM bridges with
> > associated helper function.
> >
> > This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> > is for connectors.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  8 ++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe..e2ad098 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  }
> >
> >  /**
> > + * drm_bridge_set_bus_formats() - set bridge supported image formats
> > + * @bridge: the bridge to set image formats in
> > + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> > + * @num_formats: number of elements in the @formats array
> > + *
> > + * Store a list of supported image formats in a bridge.
> > + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for
> > + * a full list of available formats.
> > + */
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *formats,
> > +			       unsigned int num_formats)
> > +{
> > +	u32 *fmts;
> > +
> > +	if (!formats || !num_formats)
> > +		return -EINVAL;
>
> I see no compelling reason to forbid restoring the number of reported
> input formats to zero? I can't think of a use right now of course, but it
> seems a bit odd all the same.

It is, you're right. Will fix in v2 and will allow bridges to just
restore formats to 0 (as it is done for drm_connectors, by the way)

Thanks
   j
>
> Cheers,
> Peter
>
> > +
> > +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> > +	if (!fmts)
> > +		return -ENOMEM;
> > +
> > +	kfree(bridge->bus_formats);
> > +	bridge->bus_formats = fmts;
> > +	bridge->num_bus_formats = num_formats;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> > +
> > +/**
> >   * DOC: bridge callbacks
> >   *
> >   * The &drm_bridge_funcs ops are populated by the bridge driver. The DRM
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec..6b3648c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >   * @encoder: encoder to which this bridge is connected
> >   * @next: the next bridge in the encoder chain
> >   * @of_node: device node pointer to the bridge
> > + * @bus_formats: wire image formats. Array of @num_bus_formats MEDIA_BUS_FMT\_
> > + * elements
> > + * @num_bus_formats: size of @bus_formats array
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > @@ -271,6 +274,9 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> >  	struct device_node *of_node;
> >  #endif
> > +	const u32 *bus_formats;
> > +	unsigned int num_bus_formats;
> > +
> >  	struct list_head list;
> >  	const struct drm_bridge_timings *timings;
> >
> > @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >  			struct drm_display_mode *adjusted_mode);
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> > +			       unsigned int num_fmts);
> >
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Laurent Pinchart]

Hi Jacopo,

On Thursday, 26 April 2018 21:40:56 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 12:27:39PM +0300, Laurent Pinchart wrote:
> > On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> >> Add support for storing image format information in DRM bridges with
> >> associated helper function.
> >> 
> >> This patch replicates for bridges what
> >> 'drm_display_info_set_bus_formats()'
> >> is for connectors.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >>  include/drm/drm_bridge.h     |  8 ++++++++
> >>  2 files changed, 38 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 1638bfe..e2ad098 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>  }
> >>  
> >>  /**
> >> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> >> + * @bridge: the bridge to set image formats in
> >> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> > 
> > Why the \ (here and below) ?
> 
> mmm, I can't tell where I made that up from... Will change with
> MEDIA_BUS_FMT_*
> 
> >> + * @num_formats: number of elements in the @formats array
> >> + *
> >> + * Store a list of supported image formats in a bridge.
> >> + * See MEDIA_BUS_FMT_* definitions in
> >> include/uapi/linux/media-bus-format.h for
> >> + * a full list of available formats.
> >> + */
> >> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> >> *formats,
> >> +			       unsigned int num_formats)
> >> +{
> >> +	u32 *fmts;
> >> +
> >> +	if (!formats || !num_formats)
> >> +		return -EINVAL;
> >> +
> >> +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> > 
> > This memory will be leaked when the bridge is destroyed.
> 
> Right. I'm afraid this may open a pandora box, but, ehm, where is the
> bridge objects lifetime managed? The best I can think of is to use the
> resource managed version of kmemdup, associating that memory to
> the drm_device device object. That means the memory will be freed at
> DRM pipeline teardown time only if I'm not wrong. Can a bridge be
> destroyed before that?

The lifetime of the bridge is independent from the lifetime of the drm_device, 
so that won't work. It looks like we need to add a bridge cleanup operation 
:-/ You're right, it opens pandora's box.

My recommendation is to add a .release() operation to the bridge ops 
structure, and to implement a drm_bridge_cleanup() function that frees the 
bus_formats memory. Then, a drm_bridge_release() function can wrap the 
release() ops, and that should be called from the bridge driver .remove() 
handler. Or even butter, call the drm_bridge_release() function 
drm_bridge_put(), to pave the way for a reference-count-based implementation.

> >> +	if (!fmts)
> >> +		return -ENOMEM;
> >> +
> >> +	kfree(bridge->bus_formats);
> >> +	bridge->bus_formats = fmts;
> >> +	bridge->num_bus_formats = num_formats;
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> >> +
> >> +/**
> >>   * DOC: bridge callbacks
> >>   *
> >>   * The &drm_bridge_funcs ops are populated by the bridge driver. The
> >>   DRM
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index 3270fec..6b3648c 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >>   * @encoder: encoder to which this bridge is connected
> >>   * @next: the next bridge in the encoder chain
> >>   * @of_node: device node pointer to the bridge
> >> + * @bus_formats: wire image formats. Array of @num_bus_formats
> >> MEDIA_BUS_FMT\_
> >> + * elements
> >> + * @num_bus_formats: size of @bus_formats array
> >>   * @list: to keep track of all added bridges
> >>   * @timings: the timing specification for the bridge, if any (may
> >>   * be NULL)
> >> @@ -271,6 +274,9 @@ struct drm_bridge {
> >>  #ifdef CONFIG_OF
> >>  	struct device_node *of_node;
> >>  #endif
> >> +	const u32 *bus_formats;
> >> +	unsigned int num_bus_formats;
> >> +
> >>  	struct list_head list;
> >>  	const struct drm_bridge_timings *timings;
> >> 
> >> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >>  			struct drm_display_mode *adjusted_mode);
> >>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >>  void drm_bridge_enable(struct drm_bridge *bridge);
> >> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> >> *fmts,
> >> +			       unsigned int num_fmts);
> >> 
> >>  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Laurent Pinchart]

Hi Jacopo,

On Thursday, 26 April 2018 21:40:56 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 12:27:39PM +0300, Laurent Pinchart wrote:
> > On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> >> Add support for storing image format information in DRM bridges with
> >> associated helper function.
> >> 
> >> This patch replicates for bridges what
> >> 'drm_display_info_set_bus_formats()'
> >> is for connectors.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >>  include/drm/drm_bridge.h     |  8 ++++++++
> >>  2 files changed, 38 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 1638bfe..e2ad098 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>  }
> >>  
> >>  /**
> >> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> >> + * @bridge: the bridge to set image formats in
> >> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> > 
> > Why the \ (here and below) ?
> 
> mmm, I can't tell where I made that up from... Will change with
> MEDIA_BUS_FMT_*
> 
> >> + * @num_formats: number of elements in the @formats array
> >> + *
> >> + * Store a list of supported image formats in a bridge.
> >> + * See MEDIA_BUS_FMT_* definitions in
> >> include/uapi/linux/media-bus-format.h for
> >> + * a full list of available formats.
> >> + */
> >> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> >> *formats,
> >> +			       unsigned int num_formats)
> >> +{
> >> +	u32 *fmts;
> >> +
> >> +	if (!formats || !num_formats)
> >> +		return -EINVAL;
> >> +
> >> +	fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> > 
> > This memory will be leaked when the bridge is destroyed.
> 
> Right. I'm afraid this may open a pandora box, but, ehm, where is the
> bridge objects lifetime managed? The best I can think of is to use the
> resource managed version of kmemdup, associating that memory to
> the drm_device device object. That means the memory will be freed at
> DRM pipeline teardown time only if I'm not wrong. Can a bridge be
> destroyed before that?

The lifetime of the bridge is independent from the lifetime of the drm_device, 
so that won't work. It looks like we need to add a bridge cleanup operation 
:-/ You're right, it opens pandora's box.

My recommendation is to add a .release() operation to the bridge ops 
structure, and to implement a drm_bridge_cleanup() function that frees the 
bus_formats memory. Then, a drm_bridge_release() function can wrap the 
release() ops, and that should be called from the bridge driver .remove() 
handler. Or even butter, call the drm_bridge_release() function 
drm_bridge_put(), to pave the way for a reference-count-based implementation.

> >> +	if (!fmts)
> >> +		return -ENOMEM;
> >> +
> >> +	kfree(bridge->bus_formats);
> >> +	bridge->bus_formats = fmts;
> >> +	bridge->num_bus_formats = num_formats;
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> >> +
> >> +/**
> >>   * DOC: bridge callbacks
> >>   *
> >>   * The &drm_bridge_funcs ops are populated by the bridge driver. The
> >>   DRM
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index 3270fec..6b3648c 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >>   * @encoder: encoder to which this bridge is connected
> >>   * @next: the next bridge in the encoder chain
> >>   * @of_node: device node pointer to the bridge
> >> + * @bus_formats: wire image formats. Array of @num_bus_formats
> >> MEDIA_BUS_FMT\_
> >> + * elements
> >> + * @num_bus_formats: size of @bus_formats array
> >>   * @list: to keep track of all added bridges
> >>   * @timings: the timing specification for the bridge, if any (may
> >>   * be NULL)
> >> @@ -271,6 +274,9 @@ struct drm_bridge {
> >>  #ifdef CONFIG_OF
> >>  	struct device_node *of_node;
> >>  #endif
> >> +	const u32 *bus_formats;
> >> +	unsigned int num_bus_formats;
> >> +
> >>  	struct list_head list;
> >>  	const struct drm_bridge_timings *timings;
> >> 
> >> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >>  			struct drm_display_mode *adjusted_mode);
> >>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >>  void drm_bridge_enable(struct drm_bridge *bridge);
> >> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> >> *fmts,
> >> +			       unsigned int num_fmts);
> >> 
> >>  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel