[PATCH 0/4 v5] Support bridge timings

[PATCH 0/4 v5] Support bridge timings

[Linus Walleij]

This patch set is in response to Laurent's mail:
https://www.spinics.net/lists/dri-devel/msg155618.html

So in summary:
- The connector is apparently not the right abstraction to carry
  the detailed timings specification between DRI drivers and bridge
  drivers.

- Instead put detailed timing data into the bridge itself as an
  optional information pointer.

- Add fields to specify triggering edge on the clock, setup
  and hold time for the bridge.

- Augment the dumb VGA driver with this data, supporting a few
  ADV and TI variants.

- Augment the PL111 driver to use this data to figure out if it
  needs to clock out display data on the negative edge rather
  than the positive edge because the current clocking out on the
  positive edge obviously partly misses the setup->hold window,
  as can be observed in annoying green flicker.

Linus Walleij (4):
  drm/bridge: Add bindings for TI THS8134
  drm/bridge: Provide a way to embed timing info in bridges
  drm/bridge: Add timing support to dumb VGA DAC
  drm/pl111: Support handling bridge timings

 .../bridge/{ti,ths8135.txt => ti,ths813x.txt}      | 13 +++--
 drivers/gpu/drm/bridge/dumb-vga-dac.c              | 61 ++++++++++++++++++++--
 drivers/gpu/drm/pl111/Kconfig                      |  1 +
 drivers/gpu/drm/pl111/pl111_display.c              | 35 +++++++++++--
 drivers/gpu/drm/pl111/pl111_drv.c                  | 20 +++----
 include/drm/drm_bridge.h                           | 20 +++++++
 6 files changed, 130 insertions(+), 20 deletions(-)
 rename Documentation/devicetree/bindings/display/bridge/{ti,ths8135.txt => ti,ths813x.txt} (69%)

-- 
2.14.3

[PATCH 1/4 v5] drm/bridge: Add bindings for TI THS8134

[Linus Walleij]

This adds device tree bindings for the Texas Instruments
THS8134, THS8134A and THS8134B VGA DACs by extending and
renaming the existing bindings for THS8135.

These DACs are used for the VGA outputs on the ARM reference
designs such as Integrator, Versatile and RealView.

Cc: devicetree at vger.kernel.org
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v5:
- Dropped the "ti,ths813x" as it turns out we need precise info
  about the sub-variant anyways as they all very in timings.
- Refine the THS8134 variants, it turns out ths8134, ths8134a
  and ths8134b are three different variants of ths8134.
ChangeLog v1->v2:
- Introduce specific-to-general compatible string:
  compatible = "ti,ths8134a", "ti,ths813x";
  so drivers can handle the whole family the same way.
- Collected Rob's ACK.
---
 .../display/bridge/{ti,ths8135.txt => ti,ths813x.txt}       | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/display/bridge/{ti,ths8135.txt => ti,ths813x.txt} (69%)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
similarity index 69%
rename from Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
rename to Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
index 6ec1a880ac18..49f155467f00 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
@@ -1,11 +1,16 @@
-THS8135 Video DAC
------------------
+THS8134 and THS8135 Video DAC
+-----------------------------
 
-This is the binding for Texas Instruments THS8135 Video DAC bridge.
+This is the binding for Texas Instruments THS8134, THS8134A, THS8134B and
+THS8135 Video DAC bridge.
 
 Required properties:
 
-- compatible: Must be "ti,ths8135"
+- compatible: Must be one of
+  "ti,ths8134"
+  "ti,ths8134", "ti,ths8134a"
+  "ti,ths8134", "ti,ths8134b"
+  "ti,ths8135"
 
 Required nodes:
 
-- 
2.14.3

[PATCH 1/4 v5] drm/bridge: Add bindings for TI THS8134

[Rob Herring]

On Fri, Dec 15, 2017 at 01:10:44PM +0100, Linus Walleij wrote:
> This adds device tree bindings for the Texas Instruments
> THS8134, THS8134A and THS8134B VGA DACs by extending and
> renaming the existing bindings for THS8135.
> 
> These DACs are used for the VGA outputs on the ARM reference
> designs such as Integrator, Versatile and RealView.
> 
> Cc: devicetree at vger.kernel.org
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v5:
> - Dropped the "ti,ths813x" as it turns out we need precise info
>   about the sub-variant anyways as they all very in timings.
> - Refine the THS8134 variants, it turns out ths8134, ths8134a
>   and ths8134b are three different variants of ths8134.
> ChangeLog v1->v2:
> - Introduce specific-to-general compatible string:
>   compatible = "ti,ths8134a", "ti,ths813x";
>   so drivers can handle the whole family the same way.
> - Collected Rob's ACK.
> ---
>  .../display/bridge/{ti,ths8135.txt => ti,ths813x.txt}       | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>  rename Documentation/devicetree/bindings/display/bridge/{ti,ths8135.txt => ti,ths813x.txt} (69%)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> similarity index 69%
> rename from Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> rename to Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> index 6ec1a880ac18..49f155467f00 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> @@ -1,11 +1,16 @@
> -THS8135 Video DAC
> ------------------
> +THS8134 and THS8135 Video DAC
> +-----------------------------
>  
> -This is the binding for Texas Instruments THS8135 Video DAC bridge.
> +This is the binding for Texas Instruments THS8134, THS8134A, THS8134B and
> +THS8135 Video DAC bridge.
>  
>  Required properties:
>  
> -- compatible: Must be "ti,ths8135"
> +- compatible: Must be one of
> +  "ti,ths8134"
> +  "ti,ths8134", "ti,ths8134a"
> +  "ti,ths8134", "ti,ths8134b"

These should be the opposite order.

> +  "ti,ths8135"
>  
>  Required nodes:
>  
> -- 
> 2.14.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4 v5] drm/bridge: Add bindings for TI THS8134

[Laurent Pinchart]

Hello Linus,

Thank you for the patch.

On Friday, 15 December 2017 14:10:44 EET Linus Walleij wrote:
> This adds device tree bindings for the Texas Instruments
> THS8134, THS8134A and THS8134B VGA DACs by extending and
> renaming the existing bindings for THS8135.
> 
> These DACs are used for the VGA outputs on the ARM reference
> designs such as Integrator, Versatile and RealView.
> 
> Cc: devicetree at vger.kernel.org
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v5:
> - Dropped the "ti,ths813x" as it turns out we need precise info
>   about the sub-variant anyways as they all very in timings.
> - Refine the THS8134 variants, it turns out ths8134, ths8134a
>   and ths8134b are three different variants of ths8134.
> ChangeLog v1->v2:
> - Introduce specific-to-general compatible string:
>   compatible = "ti,ths8134a", "ti,ths813x";
>   so drivers can handle the whole family the same way.
> - Collected Rob's ACK.
> ---
>  .../display/bridge/{ti,ths8135.txt => ti,ths813x.txt}       | 13 +++++++--
>  1 file changed, 9 insertions(+), 4 deletions(-)
>  rename Documentation/devicetree/bindings/display/bridge/{ti,ths8135.txt =>
> ti,ths813x.txt} (69%)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> similarity index 69%
> rename from Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> rename to Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> index 6ec1a880ac18..49f155467f00 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> @@ -1,11 +1,16 @@
> -THS8135 Video DAC
> ------------------
> +THS8134 and THS8135 Video DAC
> +-----------------------------
> 
> -This is the binding for Texas Instruments THS8135 Video DAC bridge.
> +This is the binding for Texas Instruments THS8134, THS8134A, THS8134B and
> +THS8135 Video DAC bridge.

There's more than one no, so s/bridge/bridges/. Or just s/DAC bridge/DACs/ as 
bridge refers to the software implementation.

With this and Rob's comment about the compatible string ordering 
("ti,ths8134[ab]", "ti,ths8134" instead of "ti,ths8134[ab]", "ti,ths8134"),

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

>  Required properties:
> 
> -- compatible: Must be "ti,ths8135"
> +- compatible: Must be one of
> +  "ti,ths8134"
> +  "ti,ths8134", "ti,ths8134a"
> +  "ti,ths8134", "ti,ths8134b"
> +  "ti,ths8135"
> 
>  Required nodes:


-- 
Regards,

Laurent Pinchart

[PATCH 2/4 v5] drm/bridge: Provide a way to embed timing info in bridges

[Linus Walleij]

After some discussion and failed patch sets trying to convey
the right timing information between the display engine and
a bridge using the connector, I try instead to use an optional
timing information container in the bridge itself, so that
display engines can retrieve it from any bridge and use it to
determine how to drive outputs.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog ->v5:
- New patch
---
 include/drm/drm_bridge.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..3bf34f7c90d4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -30,6 +30,7 @@
 
 struct drm_bridge;
 struct drm_panel;
+struct drm_bridge_timings;
 
 /**
  * struct drm_bridge_funcs - drm_bridge control functions
@@ -222,6 +223,22 @@ struct drm_bridge_funcs {
 	void (*enable)(struct drm_bridge *bridge);
 };
 
+/**
+ * struct drm_bridge_timings - timing information for the bridge
+ * @sampling_edge: whether the bridge samples the digital input signal from the
+ * display engine on the positive or negative edge of the clock - false means
+ * negative edge, true means positive edge.
+ * @setup_time_ps: the time in picoseconds the input data lines must be stable
+ * before the clock edge
+ * @hold_time_ps: the time in picoseconds taken for the bridge to sample the
+ * input signal after the rising or falling edge
+ */
+struct drm_bridge_timings {
+	bool sampling_edge;
+	u32 setup_time_ps;
+	u32 hold_time_ps;
+};
+
 /**
  * struct drm_bridge - central DRM bridge control structure
  * @dev: DRM device this bridge belongs to
@@ -229,6 +246,8 @@ struct drm_bridge_funcs {
  * @next: the next bridge in the encoder chain
  * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
+ * @timings: the timing specification for the bridge, if any (may
+ * be NULL)
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -240,6 +259,7 @@ struct drm_bridge {
 	struct device_node *of_node;
 #endif
 	struct list_head list;
+	const struct drm_bridge_timings *timings;
 
 	const struct drm_bridge_funcs *funcs;
 	void *driver_private;
-- 
2.14.3

[PATCH 2/4 v5] drm/bridge: Provide a way to embed timing info in bridges

[Laurent Pinchart]

Hello Linus,

Thank you for the patch.

On Friday, 15 December 2017 14:10:45 EET Linus Walleij wrote:
> After some discussion and failed patch sets trying to convey
> the right timing information between the display engine and
> a bridge using the connector, I try instead to use an optional
> timing information container in the bridge itself, so that
> display engines can retrieve it from any bridge and use it to
> determine how to drive outputs.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog ->v5:
> - New patch
> ---
>  include/drm/drm_bridge.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 682d01ba920c..3bf34f7c90d4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -30,6 +30,7 @@
> 
>  struct drm_bridge;
>  struct drm_panel;
> +struct drm_bridge_timings;

Could you keep these alphabetically sorted ?

> 
>  /**
>   * struct drm_bridge_funcs - drm_bridge control functions
> @@ -222,6 +223,22 @@ struct drm_bridge_funcs {
>  	void (*enable)(struct drm_bridge *bridge);
>  };
> 
> +/**
> + * struct drm_bridge_timings - timing information for the bridge
> + * @sampling_edge: whether the bridge samples the digital input signal from
> the
> + * display engine on the positive or negative edge of the clock - false
> means
> + * negative edge, true means positive edge.

Would it be clearer to reuse the DRM_BUS_FLAG_PIXDATA_*EDGE flags here ?

> + * @setup_time_ps: the time in picoseconds the input data lines must be
> stable
> + * before the clock edge
> + * @hold_time_ps: the time in picoseconds taken for the bridge to sample
> the
> + * input signal after the rising or falling edge

I'd write s/rising or falling edge/clock edge/ to be consistent with the 
setup_time_ps description (and because that's also clearer in my opinion). If 
you want to explicitly tell which clock edge, you could write "the clock 
sampling edge" for both.

The rest of the patch looks good to me.

> + */
> +struct drm_bridge_timings {
> +	bool sampling_edge;
> +	u32 setup_time_ps;
> +	u32 hold_time_ps;
> +};
> +
>  /**
>   * struct drm_bridge - central DRM bridge control structure
>   * @dev: DRM device this bridge belongs to
> @@ -229,6 +246,8 @@ struct drm_bridge_funcs {
>   * @next: the next bridge in the encoder chain
>   * @of_node: device node pointer to the bridge
>   * @list: to keep track of all added bridges
> + * @timings: the timing specification for the bridge, if any (may
> + * be NULL)
>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -240,6 +259,7 @@ struct drm_bridge {
>  	struct device_node *of_node;
>  #endif
>  	struct list_head list;
> +	const struct drm_bridge_timings *timings;
> 
>  	const struct drm_bridge_funcs *funcs;
>  	void *driver_private;

-- 
Regards,

Laurent Pinchart

[PATCH 3/4 v5] drm/bridge: Add timing support to dumb VGA DAC

[Linus Walleij]

This extends the dumb VGA DAC bridge to handle the THS8134A
and THS8134B VGA DACs in addition to those already handled.

We assign the proper timing data to the pointer inside the
bridge struct so display controllers that need to align their
timings to the bridge can pick it up and work from there.

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v4->v5:
- Rewrite the support using the new concept of defining
  fine-granular sampling (setup+hold) timing definitions
  stored in the bridge timings struct.
ChangeLog v3->v4:
- Actually have the code syntactically correct and compiling :(
  (Kconfig mistake.)
  (...)
  AS      usr/initramfs_data.o
  AR      usr/built-in.o
  CC      drivers/gpu/drm/bridge/dumb-vga-dac.o
  AR      drivers/gpu/drm/bridge/built-in.o
  AR      drivers/gpu/drm/built-in.o
  AR      drivers/gpu/built-in.o
  AR      drivers/built-in.o
  (...)
ChangeLog v2->v3:
- Move const specifier.
- Cut one line of code assigning bus flags.
- Preserve the "ti,ths8135" compatible for elder device trees.
ChangeLog v1->v2:
- Alphabetize includes
- Use a u32 with the bus polarity flags and just encode the
  polarity using the DRM define directly.
- Rename vendor_data to vendor_info.
- Simplify assignment of the flag as it is just a simple
  u32 now.
- Probe all TI variants on the "ti,ths813x" wildcard for now,
  we only need to know that the device is in this family to
  set the clock edge flag right.
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 61 +++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index de5e7dee7ad6..34788783a90f 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 
@@ -176,11 +177,13 @@ static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
 static int dumb_vga_probe(struct platform_device *pdev)
 {
 	struct dumb_vga *vga;
+	const struct drm_bridge_timings *timings;
 
 	vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
 	if (!vga)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, vga);
+	timings = of_device_get_match_data(&pdev->dev);
 
 	vga->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
 	if (IS_ERR(vga->vdd)) {
@@ -204,6 +207,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
 
 	vga->bridge.funcs = &dumb_vga_bridge_funcs;
 	vga->bridge.of_node = pdev->dev.of_node;
+	vga->bridge.timings = timings;
 
 	drm_bridge_add(&vga->bridge);
 
@@ -222,10 +226,61 @@ static int dumb_vga_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/*
+ * We assume the ADV7123 DAC is the "default" for historical reasons
+ * Information taken from the ADV7123 datasheet, revision D.
+ * NOTE: the ADV7123EP seems to have other timings and need a new timings
+ * set if used.
+ */
+static const struct drm_bridge_timings default_dac_timings = {
+	/* Timing specifications, datasheet page 7 */
+	.sampling_edge = true,
+	.setup_time_ps = 500,
+	.hold_time_ps = 1500,
+};
+
+/*
+ * Information taken from the THS8134, THS8134A, THS8134B datasheet named
+ * "SLVS205D", dated May 1990, revised March 2000.
+ */
+static const struct drm_bridge_timings ti_ths8134_dac_timings = {
+	/* From timing diagram, datasheet page 9 */
+	.sampling_edge = true,
+	/* From datasheet, page 12 */
+	.setup_time_ps = 3000,
+	/* I guess this means latched input */
+	.hold_time_ps = 0,
+};
+
+/*
+ * Information taken from the THS8135 datasheet named "SLAS343B", dated
+ * May 2001, revised April 2013.
+ */
+static const struct drm_bridge_timings ti_ths8135_dac_timings = {
+	/* From timing diagram, datasheet page 14 */
+	.sampling_edge = true,
+	/* From datasheet, page 16 */
+	.setup_time_ps = 2000,
+	.hold_time_ps = 500,
+};
+
 static const struct of_device_id dumb_vga_match[] = {
-	{ .compatible = "dumb-vga-dac" },
-	{ .compatible = "adi,adv7123" },
-	{ .compatible = "ti,ths8135" },
+	{
+		.compatible = "dumb-vga-dac",
+		.data = &default_dac_timings,
+	},
+	{
+		.compatible = "adi,adv7123",
+		.data = &default_dac_timings,
+	},
+	{
+		.compatible = "ti,ths8135",
+		.data = &ti_ths8135_dac_timings,
+	},
+	{
+		.compatible = "ti,ths8134",
+		.data = &ti_ths8134_dac_timings,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, dumb_vga_match);
-- 
2.14.3

[PATCH 3/4 v5] drm/bridge: Add timing support to dumb VGA DAC

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Friday, 15 December 2017 14:10:46 EET Linus Walleij wrote:
> This extends the dumb VGA DAC bridge to handle the THS8134A
> and THS8134B VGA DACs in addition to those already handled.
> 
> We assign the proper timing data to the pointer inside the
> bridge struct so display controllers that need to align their
> timings to the bridge can pick it up and work from there.
> 
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v4->v5:
> - Rewrite the support using the new concept of defining
>   fine-granular sampling (setup+hold) timing definitions
>   stored in the bridge timings struct.
> ChangeLog v3->v4:
> - Actually have the code syntactically correct and compiling :(
>   (Kconfig mistake.)
>   (...)
>   AS      usr/initramfs_data.o
>   AR      usr/built-in.o
>   CC      drivers/gpu/drm/bridge/dumb-vga-dac.o
>   AR      drivers/gpu/drm/bridge/built-in.o
>   AR      drivers/gpu/drm/built-in.o
>   AR      drivers/gpu/built-in.o
>   AR      drivers/built-in.o
>   (...)
> ChangeLog v2->v3:
> - Move const specifier.
> - Cut one line of code assigning bus flags.
> - Preserve the "ti,ths8135" compatible for elder device trees.
> ChangeLog v1->v2:
> - Alphabetize includes
> - Use a u32 with the bus polarity flags and just encode the
>   polarity using the DRM define directly.
> - Rename vendor_data to vendor_info.
> - Simplify assignment of the flag as it is just a simple
>   u32 now.
> - Probe all TI variants on the "ti,ths813x" wildcard for now,
>   we only need to know that the device is in this family to
>   set the clock edge flag right.
> ---
>  drivers/gpu/drm/bridge/dumb-vga-dac.c | 61 ++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index de5e7dee7ad6..34788783a90f
> 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -11,6 +11,7 @@
>   */
> 
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
> 
> @@ -176,11 +177,13 @@ static struct i2c_adapter
> *dumb_vga_retrieve_ddc(struct device *dev) static int dumb_vga_probe(struct
> platform_device *pdev)
>  {
>  	struct dumb_vga *vga;
> +	const struct drm_bridge_timings *timings;
> 
>  	vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
>  	if (!vga)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, vga);
> +	timings = of_device_get_match_data(&pdev->dev);
> 
>  	vga->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
>  	if (IS_ERR(vga->vdd)) {
> @@ -204,6 +207,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
> 
>  	vga->bridge.funcs = &dumb_vga_bridge_funcs;
>  	vga->bridge.of_node = pdev->dev.of_node;
> +	vga->bridge.timings = timings;

Do you need the intermediate timings variable ?

>  	drm_bridge_add(&vga->bridge);
> 
> @@ -222,10 +226,61 @@ static int dumb_vga_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> +/*
> + * We assume the ADV7123 DAC is the "default" for historical reasons
> + * Information taken from the ADV7123 datasheet, revision D.
> + * NOTE: the ADV7123EP seems to have other timings and need a new timings
> + * set if used.
> + */
> +static const struct drm_bridge_timings default_dac_timings = {
> +	/* Timing specifications, datasheet page 7 */
> +	.sampling_edge = true,
> +	.setup_time_ps = 500,
> +	.hold_time_ps = 1500,
> +};

You know what's lovely ? The setup time depends on the power supply voltage 
:-) Let's use 500ps for now, that's a conservative value that will work for 
both 5V and 3.3V. If anyone needs to lower it to 200ps later, they can always 
implement support for voltage-dependent timings.

> +/*
> + * Information taken from the THS8134, THS8134A, THS8134B datasheet named
> + * "SLVS205D", dated May 1990, revised March 2000.
> + */
> +static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> +	/* From timing diagram, datasheet page 9 */
> +	.sampling_edge = true,
> +	/* From datasheet, page 12 */
> +	.setup_time_ps = 3000,
> +	/* I guess this means latched input */
> +	.hold_time_ps = 0,
> +};
> +
> +/*
> + * Information taken from the THS8135 datasheet named "SLAS343B", dated
> + * May 2001, revised April 2013.
> + */
> +static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> +	/* From timing diagram, datasheet page 14 */
> +	.sampling_edge = true,
> +	/* From datasheet, page 16 */
> +	.setup_time_ps = 2000,
> +	.hold_time_ps = 500,
> +};
> +
>  static const struct of_device_id dumb_vga_match[] = {
> -	{ .compatible = "dumb-vga-dac" },
> -	{ .compatible = "adi,adv7123" },
> -	{ .compatible = "ti,ths8135" },
> +	{
> +		.compatible = "dumb-vga-dac",
> +		.data = &default_dac_timings,

Shouldn't we leave this NULL for dumb VGA DACs ? They're made of passive 
components and don't sample the signal, so there's no real timings that we can 
report.

Apart from that,

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

> +	},
> +	{
> +		.compatible = "adi,adv7123",
> +		.data = &default_dac_timings,
> +	},
> +	{
> +		.compatible = "ti,ths8135",
> +		.data = &ti_ths8135_dac_timings,
> +	},
> +	{
> +		.compatible = "ti,ths8134",
> +		.data = &ti_ths8134_dac_timings,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dumb_vga_match);


-- 
Regards,

Laurent Pinchart

[PATCH 4/4 v5] drm/pl111: Support handling bridge timings

[Linus Walleij]

If the bridge has a too strict setup time for the incoming
signals, we may not be fast enough and then we need to
compensate by outputting the signal on the inverse clock
edge so it is for sure stable when the bridge samples it.

Since bridges in difference to panels does not expose their
connectors, make the connector optional in the display
setup code.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v4->v5:
- Use the new bridge timings setup method.
---
 drivers/gpu/drm/pl111/Kconfig         |  1 +
 drivers/gpu/drm/pl111/pl111_display.c | 35 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/pl111/pl111_drv.c     | 20 +++++++++++---------
 3 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig
index e5e2abd66491..82cb3e60ddc8 100644
--- a/drivers/gpu/drm/pl111/Kconfig
+++ b/drivers/gpu/drm/pl111/Kconfig
@@ -8,6 +8,7 @@ config DRM_PL111
 	select DRM_GEM_CMA_HELPER
 	select DRM_BRIDGE
 	select DRM_PANEL_BRIDGE
+	select DRM_DUMB_VGA_DAC
 	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
 	help
 	  Choose this option for DRM support for the PL111 CLCD controller.
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index 06c4bf756b69..7fe4040aea46 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -94,6 +94,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
 	const struct drm_display_mode *mode = &cstate->mode;
 	struct drm_framebuffer *fb = plane->state->fb;
 	struct drm_connector *connector = priv->connector;
+	struct drm_bridge *bridge = priv->bridge;
 	u32 cntl;
 	u32 ppl, hsw, hfp, hbp;
 	u32 lpp, vsw, vfp, vbp;
@@ -143,11 +144,37 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 		tim2 |= TIM2_IVS;
 
-	if (connector->display_info.bus_flags & DRM_BUS_FLAG_DE_LOW)
-		tim2 |= TIM2_IOE;
+	if (connector) {
+		if (connector->display_info.bus_flags & DRM_BUS_FLAG_DE_LOW)
+			tim2 |= TIM2_IOE;
 
-	if (connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
-		tim2 |= TIM2_IPC;
+		if (connector->display_info.bus_flags &
+		    DRM_BUS_FLAG_PIXDATA_NEGEDGE)
+			tim2 |= TIM2_IPC;
+	}
+
+	if (bridge) {
+		const struct drm_bridge_timings *btimings = bridge->timings;
+
+		/*
+		 * Here is when things get really fun. Sometimes the bridge
+		 * timings are such that the signal out from PL11x is not
+		 * stable before the receiving bridge (such as a dumb VGA DAC
+		 * or similar) samples it. If that happens, we compensate by
+		 * the only method we have: output the data on the opposite
+		 * edge of the clock so it is for sure stable when it gets
+		 * sampled.
+		 *
+		 * The PL111 manual does not contain proper timining diagrams
+		 * or data for these details, but we know from experiments
+		 * that the setup time is more than 3000 picoseconds (3 ns).
+		 * If we have a bridge that requires the signal to be stable
+		 * earlier than 3000 ps before the clock pulse, we have to
+		 * output the data on the opposite edge to avoid flicker.
+		 */
+		if (btimings && btimings->setup_time_ps >= 3000)
+			tim2 ^= TIM2_IPC;
+	}
 
 	tim2 |= cpl << 16;
 	writel(tim2, priv->regs + CLCD_TIM2);
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 201d57d5cb54..101a9c7db6ff 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -107,11 +107,17 @@ static int pl111_modeset_init(struct drm_device *dev)
 			ret = PTR_ERR(bridge);
 			goto out_config;
 		}
-		/*
-		 * TODO: when we are using a different bridge than a panel
-		 * (such as a dumb VGA connector) we need to devise a different
-		 * method to get the connector out of the bridge.
-		 */
+	} else if (bridge) {
+		dev_info(dev->dev, "Using non-panel bridge\n");
+	} else {
+		dev_err(dev->dev, "No bridge, exiting\n");
+		return -ENODEV;
+	}
+
+	priv->bridge = bridge;
+	if (panel) {
+		priv->panel = panel;
+		priv->connector = panel->connector;
 	}
 
 	ret = pl111_display_init(dev);
@@ -125,10 +131,6 @@ static int pl111_modeset_init(struct drm_device *dev)
 	if (ret)
 		return ret;
 
-	priv->bridge = bridge;
-	priv->panel = panel;
-	priv->connector = panel->connector;
-
 	ret = drm_vblank_init(dev, 1);
 	if (ret != 0) {
 		dev_err(dev->dev, "Failed to init vblank\n");
-- 
2.14.3

[PATCH 4/4 v5] drm/pl111: Support handling bridge timings

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Friday, 15 December 2017 14:10:47 EET Linus Walleij wrote:
> If the bridge has a too strict setup time for the incoming
> signals, we may not be fast enough and then we need to
> compensate by outputting the signal on the inverse clock
> edge so it is for sure stable when the bridge samples it.
> 
> Since bridges in difference to panels does not expose their
> connectors, make the connector optional in the display
> setup code.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v4->v5:
> - Use the new bridge timings setup method.
> ---
>  drivers/gpu/drm/pl111/Kconfig         |  1 +
>  drivers/gpu/drm/pl111/pl111_display.c | 35 ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/pl111/pl111_drv.c     | 20 +++++++++++---------
>  3 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig
> index e5e2abd66491..82cb3e60ddc8 100644
> --- a/drivers/gpu/drm/pl111/Kconfig
> +++ b/drivers/gpu/drm/pl111/Kconfig
> @@ -8,6 +8,7 @@ config DRM_PL111
>  	select DRM_GEM_CMA_HELPER
>  	select DRM_BRIDGE
>  	select DRM_PANEL_BRIDGE
> +	select DRM_DUMB_VGA_DAC
>  	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
>  	help
>  	  Choose this option for DRM support for the PL111 CLCD controller.
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c
> b/drivers/gpu/drm/pl111/pl111_display.c index 06c4bf756b69..7fe4040aea46
> 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -94,6 +94,7 @@ static void pl111_display_enable(struct
> drm_simple_display_pipe *pipe, const struct drm_display_mode *mode =
> &cstate->mode;
>  	struct drm_framebuffer *fb = plane->state->fb;
>  	struct drm_connector *connector = priv->connector;
> +	struct drm_bridge *bridge = priv->bridge;
>  	u32 cntl;
>  	u32 ppl, hsw, hfp, hbp;
>  	u32 lpp, vsw, vfp, vbp;
> @@ -143,11 +144,37 @@ static void pl111_display_enable(struct
> drm_simple_display_pipe *pipe, if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>  		tim2 |= TIM2_IVS;
> 
> -	if (connector->display_info.bus_flags & DRM_BUS_FLAG_DE_LOW)
> -		tim2 |= TIM2_IOE;
> +	if (connector) {
> +		if (connector->display_info.bus_flags & DRM_BUS_FLAG_DE_LOW)
> +			tim2 |= TIM2_IOE;
> 
> -	if (connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
> -		tim2 |= TIM2_IPC;
> +		if (connector->display_info.bus_flags &
> +		    DRM_BUS_FLAG_PIXDATA_NEGEDGE)
> +			tim2 |= TIM2_IPC;
> +	}
> +
> +	if (bridge) {
> +		const struct drm_bridge_timings *btimings = bridge->timings;
> +
> +		/*
> +		 * Here is when things get really fun. Sometimes the bridge
> +		 * timings are such that the signal out from PL11x is not
> +		 * stable before the receiving bridge (such as a dumb VGA DAC
> +		 * or similar) samples it. If that happens, we compensate by
> +		 * the only method we have: output the data on the opposite
> +		 * edge of the clock so it is for sure stable when it gets
> +		 * sampled.
> +		 *
> +		 * The PL111 manual does not contain proper timining diagrams
> +		 * or data for these details, but we know from experiments
> +		 * that the setup time is more than 3000 picoseconds (3 ns).
> +		 * If we have a bridge that requires the signal to be stable
> +		 * earlier than 3000 ps before the clock pulse, we have to
> +		 * output the data on the opposite edge to avoid flicker.

This should probably depend on the pixel clock frequency, but if it works for 
you for now, I have no objection.

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

> +		 */
> +		if (btimings && btimings->setup_time_ps >= 3000)
> +			tim2 ^= TIM2_IPC;
> +	}
> 
>  	tim2 |= cpl << 16;
>  	writel(tim2, priv->regs + CLCD_TIM2);
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c
> b/drivers/gpu/drm/pl111/pl111_drv.c index 201d57d5cb54..101a9c7db6ff 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -107,11 +107,17 @@ static int pl111_modeset_init(struct drm_device *dev)
>  			ret = PTR_ERR(bridge);
>  			goto out_config;
>  		}
> -		/*
> -		 * TODO: when we are using a different bridge than a panel
> -		 * (such as a dumb VGA connector) we need to devise a different
> -		 * method to get the connector out of the bridge.
> -		 */
> +	} else if (bridge) {
> +		dev_info(dev->dev, "Using non-panel bridge\n");
> +	} else {
> +		dev_err(dev->dev, "No bridge, exiting\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->bridge = bridge;
> +	if (panel) {
> +		priv->panel = panel;
> +		priv->connector = panel->connector;
>  	}
> 
>  	ret = pl111_display_init(dev);
> @@ -125,10 +131,6 @@ static int pl111_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		return ret;
> 
> -	priv->bridge = bridge;
> -	priv->panel = panel;
> -	priv->connector = panel->connector;
> -
>  	ret = drm_vblank_init(dev, 1);
>  	if (ret != 0) {
>  		dev_err(dev->dev, "Failed to init vblank\n");


-- 
Regards,

Laurent Pinchart

[PATCH 0/4 v5] Support bridge timings

[Linus Walleij]

On Fri, Dec 15, 2017 at 1:10 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> - The connector is apparently not the right abstraction to carry
>   the detailed timings specification between DRI drivers and bridge
>   drivers.
>
> - Instead put detailed timing data into the bridge itself as an
>   optional information pointer.

Notice that this is just my fumbling attempts to deal with the situation.

Laurent made me understand what the actual technical problem was,
how come my pixels were flickering.

Both Laurent and DVetter mentioned that we may need to convey
information between the bridge and the display engine in some
way.

Alternatively I could go and hack on adding this to e.g. drm_display_info
which was used in the previous patch sets by setting the negede flag
in bus_formats.

I don't know. struct drm_display_info is getting a bit heavy as
container of misc settings related to "some kind of display".
The bridge isn't even a display itself, that is on the other side
of it. So using the connector and treating a bridge as "some kind
of display" seems wrong too.

Is there a third way?

I'm just a bit lost.

Suggestions welcome!

Yours,
Linus Walleij

[PATCH 0/4 v5] Support bridge timings

[Daniel Vetter]

On Fri, Dec 15, 2017 at 01:30:24PM +0100, Linus Walleij wrote:
> On Fri, Dec 15, 2017 at 1:10 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > - The connector is apparently not the right abstraction to carry
> >   the detailed timings specification between DRI drivers and bridge
> >   drivers.
> >
> > - Instead put detailed timing data into the bridge itself as an
> >   optional information pointer.
> 
> Notice that this is just my fumbling attempts to deal with the situation.
> 
> Laurent made me understand what the actual technical problem was,
> how come my pixels were flickering.
> 
> Both Laurent and DVetter mentioned that we may need to convey
> information between the bridge and the display engine in some
> way.
> 
> Alternatively I could go and hack on adding this to e.g. drm_display_info
> which was used in the previous patch sets by setting the negede flag
> in bus_formats.
> 
> I don't know. struct drm_display_info is getting a bit heavy as
> container of misc settings related to "some kind of display".
> The bridge isn't even a display itself, that is on the other side
> of it. So using the connector and treating a bridge as "some kind
> of display" seems wrong too.
> 
> Is there a third way?

If you don't plan to nest bridges too deeply, there is. Atm we have 2
modes in drm_crtc_state:

- mode, which is what userspace requested, and what it expects logically
  to be the actual real thing. I.e. timing, resolution and all that that
  userspace can observe (through plane positioning and vblank timestamps)
  should match this mode. For external screens this should also match
  what's physically going over the cable.

- adjusted_mode, which is something entirely undefined and to be used by
  drivers internally. Most drivers use it as the thing that's actually
  transported between the CRTC and the encoder.

There's a few common reasons for adjusted mode to be different:
- integrated panel, and your CRTC has a scaler. In that case the
  userspace-requested mode is what you feed into into the scaler, and the
  adjusted mode is what comes out of your scaler and then goes down the
  wire to the panel.

- your encoder is funky, and e.g. transcodes to the output mode itself,
  but expects that you program the input mode always the same. Usual
  reasons for this are transcoders that always want non-interlaced mode
  (and do the interlacing themselves), if the transcoder has some scaler
  itself (some TV-out transcoders had that), or if it has a strict
  expectation about signalling edges and stuff (and then transcodes the
  signal again). DACs are common doing that.
 
Anyway, sounds like your bridge is of the 2nd kind, so all you have to do
is
- in your bridge->mode_fixup function, adjust the adjusted_mode as needed
- in your pl111 driver, program the adjusted mode, not the originally
  requested mode

adjusted mode is set to be a copy of the requested mode by atomic helpers,
so this should keep working as-is on any other bridge driver.

No idea why I didn't tell you this right away, or maybe I'm missing
something this time around.

> I'm just a bit lost.

Once your un-lost, pls review the docs for drm_crtc_state and the various
mode_fixup functions, to make sure they're clear on how this is supposed
to work. Might need a new overview DOC: comment that ties it all together.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 0/4 v5] Support bridge timings

[Andrzej Hajda]

On 15.12.2017 16:54, Daniel Vetter wrote:
> On Fri, Dec 15, 2017 at 01:30:24PM +0100, Linus Walleij wrote:
>> On Fri, Dec 15, 2017 at 1:10 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>>> - The connector is apparently not the right abstraction to carry
>>>   the detailed timings specification between DRI drivers and bridge
>>>   drivers.
>>>
>>> - Instead put detailed timing data into the bridge itself as an
>>>   optional information pointer.
>> Notice that this is just my fumbling attempts to deal with the situation.
>>
>> Laurent made me understand what the actual technical problem was,
>> how come my pixels were flickering.
>>
>> Both Laurent and DVetter mentioned that we may need to convey
>> information between the bridge and the display engine in some
>> way.
>>
>> Alternatively I could go and hack on adding this to e.g. drm_display_info
>> which was used in the previous patch sets by setting the negede flag
>> in bus_formats.
>>
>> I don't know. struct drm_display_info is getting a bit heavy as
>> container of misc settings related to "some kind of display".
>> The bridge isn't even a display itself, that is on the other side
>> of it. So using the connector and treating a bridge as "some kind
>> of display" seems wrong too.
>>
>> Is there a third way?
> If you don't plan to nest bridges too deeply, there is. Atm we have 2
> modes in drm_crtc_state:
>
> - mode, which is what userspace requested, and what it expects logically
>   to be the actual real thing. I.e. timing, resolution and all that that
>   userspace can observe (through plane positioning and vblank timestamps)
>   should match this mode. For external screens this should also match
>   what's physically going over the cable.
>
> - adjusted_mode, which is something entirely undefined and to be used by
>   drivers internally. Most drivers use it as the thing that's actually
>   transported between the CRTC and the encoder.
>
> There's a few common reasons for adjusted mode to be different:
> - integrated panel, and your CRTC has a scaler. In that case the
>   userspace-requested mode is what you feed into into the scaler, and the
>   adjusted mode is what comes out of your scaler and then goes down the
>   wire to the panel.
>
> - your encoder is funky, and e.g. transcodes to the output mode itself,
>   but expects that you program the input mode always the same. Usual
>   reasons for this are transcoders that always want non-interlaced mode
>   (and do the interlacing themselves), if the transcoder has some scaler
>   itself (some TV-out transcoders had that), or if it has a strict
>   expectation about signalling edges and stuff (and then transcodes the
>   signal again). DACs are common doing that.
>  
> Anyway, sounds like your bridge is of the 2nd kind, so all you have to do
> is
> - in your bridge->mode_fixup function, adjust the adjusted_mode as needed
> - in your pl111 driver, program the adjusted mode, not the originally
>   requested mode
>
> adjusted mode is set to be a copy of the requested mode by atomic helpers,
> so this should keep working as-is on any other bridge driver.
>
> No idea why I didn't tell you this right away, or maybe I'm missing
> something this time around.

Using adjusted_mode will probably help here, fortunately the pipeline is
short in this case and one adjusted_mode per pipeline should be enough.
But there is more fundamental problem here - drm core does not provide a
way to interact between adjacent components of the pipeline. So they are
not able to negotiate/exchange parameters specific for the video link
between them.
adjusted_mode is one per pipeline and will note scale without hacks.
Anyway I remember multiple attempts to somehow workaround this issue,
for example:
- passing DSI specific settings (currently performed via DSI control bus),
- passing info about panel command mode parameters(by adding these
properties to crtc node),
- passing bus flags from panel to encoder/bridge (via drm_display_mode,
nacked if I remember, I do not know if/how it is solved actually).

Another concern is that adjusted_mode used inside drm driver (ie.
between encoder and crtc) is something private per drm driver. In this
case we have bridge which theoretically should work with ANY video
source with compatible video bus: encoder, or another bridge before it.
So adjusted_mode is not private anymore, and will be problematic if two
different bridges/encoders will use it differently.

Regards
Andrzej

>
>> I'm just a bit lost.
> Once your un-lost, pls review the docs for drm_crtc_state and the various
> mode_fixup functions, to make sure they're clear on how this is supposed
> to work. Might need a new overview DOC: comment that ties it all together.
>
> Cheers, Daniel

[PATCH 0/4 v5] Support bridge timings

[Laurent Pinchart]

Hi Andrzej,

On Monday, 18 December 2017 10:43:51 EET Andrzej Hajda wrote:
> On 15.12.2017 16:54, Daniel Vetter wrote:
> > On Fri, Dec 15, 2017 at 01:30:24PM +0100, Linus Walleij wrote:
> >> On Fri, Dec 15, 2017 at 1:10 PM, Linus Walleij <linus.walleij@linaro.org> 
wrote:
> >>> - The connector is apparently not the right abstraction to carry
> >>> 
> >>>   the detailed timings specification between DRI drivers and bridge
> >>>   drivers.
> >>> 
> >>> - Instead put detailed timing data into the bridge itself as an
> >>> 
> >>>   optional information pointer.
> >> 
> >> Notice that this is just my fumbling attempts to deal with the situation.
> >> 
> >> Laurent made me understand what the actual technical problem was,
> >> how come my pixels were flickering.
> >> 
> >> Both Laurent and DVetter mentioned that we may need to convey
> >> information between the bridge and the display engine in some
> >> way.
> >> 
> >> Alternatively I could go and hack on adding this to e.g. drm_display_info
> >> which was used in the previous patch sets by setting the negede flag
> >> in bus_formats.
> >> 
> >> I don't know. struct drm_display_info is getting a bit heavy as
> >> container of misc settings related to "some kind of display".
> >> The bridge isn't even a display itself, that is on the other side
> >> of it. So using the connector and treating a bridge as "some kind
> >> of display" seems wrong too.
> >> 
> >> Is there a third way?
> > 
> > If you don't plan to nest bridges too deeply, there is. Atm we have 2
> > modes in drm_crtc_state:
> > 
> > - mode, which is what userspace requested, and what it expects logically
> >   to be the actual real thing. I.e. timing, resolution and all that that
> >   userspace can observe (through plane positioning and vblank timestamps)
> >   should match this mode. For external screens this should also match
> >   what's physically going over the cable.
> > 
> > - adjusted_mode, which is something entirely undefined and to be used by
> >   drivers internally. Most drivers use it as the thing that's actually
> >   transported between the CRTC and the encoder.
> > 
> > There's a few common reasons for adjusted mode to be different:
> > - integrated panel, and your CRTC has a scaler. In that case the
> >   userspace-requested mode is what you feed into into the scaler, and the
> >   adjusted mode is what comes out of your scaler and then goes down the
> >   wire to the panel.
> > 
> > - your encoder is funky, and e.g. transcodes to the output mode itself,
> >   but expects that you program the input mode always the same. Usual
> >   reasons for this are transcoders that always want non-interlaced mode
> >   (and do the interlacing themselves), if the transcoder has some scaler
> >   itself (some TV-out transcoders had that), or if it has a strict
> >   expectation about signalling edges and stuff (and then transcodes the
> >   signal again). DACs are common doing that.
> > 
> > Anyway, sounds like your bridge is of the 2nd kind, so all you have to do
> > is
> > - in your bridge->mode_fixup function, adjust the adjusted_mode as needed
> > - in your pl111 driver, program the adjusted mode, not the originally
> > 
> >   requested mode
> > 
> > adjusted mode is set to be a copy of the requested mode by atomic helpers,
> > so this should keep working as-is on any other bridge driver.
> > 
> > No idea why I didn't tell you this right away, or maybe I'm missing
> > something this time around.
> 
> Using adjusted_mode will probably help here, fortunately the pipeline is
> short in this case and one adjusted_mode per pipeline should be enough.
> But there is more fundamental problem here - drm core does not provide a
> way to interact between adjacent components of the pipeline. So they are
> not able to negotiate/exchange parameters specific for the video link
> between them.
> adjusted_mode is one per pipeline and will note scale without hacks.
> Anyway I remember multiple attempts to somehow workaround this issue,
> for example:
> - passing DSI specific settings (currently performed via DSI control bus),
> - passing info about panel command mode parameters(by adding these
> properties to crtc node),
> - passing bus flags from panel to encoder/bridge (via drm_display_mode,
> nacked if I remember, I do not know if/how it is solved actually).
> 
> Another concern is that adjusted_mode used inside drm driver (ie.
> between encoder and crtc) is something private per drm driver. In this
> case we have bridge which theoretically should work with ANY video
> source with compatible video bus: encoder, or another bridge before it.
> So adjusted_mode is not private anymore, and will be problematic if two
> different bridges/encoders will use it differently.

I agree completely. I believe we'll need to introduce new bridge operations to 
report and negotiate timings along the pipeline. Ideally the same operations 
would also be implemented by panel drivers for easier handling of the 
pipeline, in which case we'll need a common data structure for both bridges 
and panels. We can however start with bridges and extend that to panels in a 
second step.

I'm currently reworking the bridge and panel code for the omapdrm driver, 
which uses omapdrm-specific drivers not based on the drm_bridge and drm_panel 
API. The goal is to replace them by standard bridge and panel drivers. As part 
of that work I plan to propose new operations for bridges as discussed above. 
I can't tell however when the first version will be ready, so if you want to 
give it a go, feel free to do so.

> >> I'm just a bit lost.
> > 
> > Once your un-lost, pls review the docs for drm_crtc_state and the various
> > mode_fixup functions, to make sure they're clear on how this is supposed
> > to work. Might need a new overview DOC: comment that ties it all together.

-- 
Regards,

Laurent Pinchart

[PATCH 0/4 v5] Support bridge timings

[Laurent Pinchart]

Hi Daniel,

On Friday, 15 December 2017 17:54:15 EET Daniel Vetter wrote:
> On Fri, Dec 15, 2017 at 01:30:24PM +0100, Linus Walleij wrote:
> > On Fri, Dec 15, 2017 at 1:10 PM, Linus Walleij <linus.walleij@linaro.org> 
wrote:
> >> - The connector is apparently not the right abstraction to carry
> >>   the detailed timings specification between DRI drivers and bridge
> >>   drivers.
> >> 
> >> - Instead put detailed timing data into the bridge itself as an
> >>   optional information pointer.
> > 
> > Notice that this is just my fumbling attempts to deal with the situation.
> > 
> > Laurent made me understand what the actual technical problem was,
> > how come my pixels were flickering.
> > 
> > Both Laurent and DVetter mentioned that we may need to convey
> > information between the bridge and the display engine in some
> > way.
> > 
> > Alternatively I could go and hack on adding this to e.g. drm_display_info
> > which was used in the previous patch sets by setting the negede flag
> > in bus_formats.
> > 
> > I don't know. struct drm_display_info is getting a bit heavy as
> > container of misc settings related to "some kind of display".
> > The bridge isn't even a display itself, that is on the other side
> > of it. So using the connector and treating a bridge as "some kind
> > of display" seems wrong too.
> > 
> > Is there a third way?
> 
> If you don't plan to nest bridges too deeply, there is. Atm we have 2
> modes in drm_crtc_state:
> 
> - mode, which is what userspace requested, and what it expects logically
>   to be the actual real thing. I.e. timing, resolution and all that that
>   userspace can observe (through plane positioning and vblank timestamps)
>   should match this mode. For external screens this should also match
>   what's physically going over the cable.
> 
> - adjusted_mode, which is something entirely undefined and to be used by
>   drivers internally. Most drivers use it as the thing that's actually
>   transported between the CRTC and the encoder.
> 
> There's a few common reasons for adjusted mode to be different:
> - integrated panel, and your CRTC has a scaler. In that case the
>   userspace-requested mode is what you feed into into the scaler, and the
>   adjusted mode is what comes out of your scaler and then goes down the
>   wire to the panel.
> 
> - your encoder is funky, and e.g. transcodes to the output mode itself,
>   but expects that you program the input mode always the same. Usual
>   reasons for this are transcoders that always want non-interlaced mode
>   (and do the interlacing themselves), if the transcoder has some scaler
>   itself (some TV-out transcoders had that), or if it has a strict
>   expectation about signalling edges and stuff (and then transcodes the
>   signal again). DACs are common doing that.
> 
> Anyway, sounds like your bridge is of the 2nd kind, so all you have to do
> is
> - in your bridge->mode_fixup function, adjust the adjusted_mode as needed
> - in your pl111 driver, program the adjusted mode, not the originally
>   requested mode
> 
> adjusted mode is set to be a copy of the requested mode by atomic helpers,
> so this should keep working as-is on any other bridge driver.

I don't think that's the right fix.

The problem here is that the display engine has to output data in a way that 
doesn't violate the DAC setup and hold times. Depending on the display engine, 
you can just select the output clock edge, or adjust the phase of the data 
compared to the pixel clock by a fraction of a clock cycle (1/4 is common, 
I've seen smaller steps too). Note that selecting the opposite clock edge is 
simple a 1/2 clock cycle delay.

The delay has to be computed based on the receiver's setup and hold times, but 
also take into account other components on the board (such as buffer or 
voltage shifters, or even inverters) and the PCB delay itself. This 
computation doesn't belong to the bridge driver, especially given than its 
goal is to compute a delay that depends on the display engine's capabilities 
(inverting the clock vs. smaller step delays for instance). For this reason I 
think the bridge driver should expose its timing parameters, and the display 
engine should then decide how to output its data accordingly.

> No idea why I didn't tell you this right away, or maybe I'm missing
> something this time around.
> 
> > I'm just a bit lost.
> 
> Once your un-lost, pls review the docs for drm_crtc_state and the various
> mode_fixup functions, to make sure they're clear on how this is supposed
> to work. Might need a new overview DOC: comment that ties it all together.

-- 
Regards,

Laurent Pinchart