[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

The DRM bus flags convey additional information on pixel data on
the bus. All current available bus flags might be of interest for
a bridge. Remove the sampling_edge field and use bus_flags.

In the case at hand a dumb VGA bridge needs a specific data enable
polarity (DRM_BUS_FLAG_DE_LOW).

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
 include/drm/drm_bridge.h              | 11 +++++------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9b706789a341..7a5c24967115 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
  */
 static const struct drm_bridge_timings default_dac_timings = {
 	/* Timing specifications, datasheet page 7 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	.setup_time_ps = 500,
 	.hold_time_ps = 1500,
 };
@@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8134_dac_timings = {
 	/* From timing diagram, datasheet page 9 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 12 */
 	.setup_time_ps = 3000,
 	/* I guess this means latched input */
@@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8135_dac_timings = {
 	/* From timing diagram, datasheet page 14 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 16 */
 	.setup_time_ps = 2000,
 	.hold_time_ps = 500,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd850747ce54..85d4b51eae19 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -244,14 +244,13 @@ struct drm_bridge_funcs {
  */
 struct drm_bridge_timings {
 	/**
-	 * @sampling_edge:
+	 * @bus_flags:
 	 *
-	 * Tells whether the bridge samples the digital input signal
-	 * from the display engine on the positive or negative edge of the
-	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
-	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
+	 * Tells what additional settings for the pixel data on the bus
+	 * this bridge requires (like pixel signal polarity). See also
+	 * &drm_display_info->bus_flags.
 	 */
-	u32 sampling_edge;
+	u32 bus_flags;
 	/**
 	 * @setup_time_ps:
 	 *
-- 
2.18.0

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

The DRM bus flags convey additional information on pixel data on
the bus. All current available bus flags might be of interest for
a bridge. Remove the sampling_edge field and use bus_flags.

In the case at hand a dumb VGA bridge needs a specific data enable
polarity (DRM_BUS_FLAG_DE_LOW).

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
 include/drm/drm_bridge.h              | 11 +++++------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9b706789a341..7a5c24967115 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
  */
 static const struct drm_bridge_timings default_dac_timings = {
 	/* Timing specifications, datasheet page 7 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	.setup_time_ps = 500,
 	.hold_time_ps = 1500,
 };
@@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8134_dac_timings = {
 	/* From timing diagram, datasheet page 9 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 12 */
 	.setup_time_ps = 3000,
 	/* I guess this means latched input */
@@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8135_dac_timings = {
 	/* From timing diagram, datasheet page 14 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 16 */
 	.setup_time_ps = 2000,
 	.hold_time_ps = 500,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd850747ce54..85d4b51eae19 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -244,14 +244,13 @@ struct drm_bridge_funcs {
  */
 struct drm_bridge_timings {
 	/**
-	 * @sampling_edge:
+	 * @bus_flags:
 	 *
-	 * Tells whether the bridge samples the digital input signal
-	 * from the display engine on the positive or negative edge of the
-	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
-	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
+	 * Tells what additional settings for the pixel data on the bus
+	 * this bridge requires (like pixel signal polarity). See also
+	 * &drm_display_info->bus_flags.
 	 */
-	u32 sampling_edge;
+	u32 bus_flags;
 	/**
 	 * @setup_time_ps:
 	 *
-- 
2.18.0

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

The DRM bus flags convey additional information on pixel data on
the bus. All current available bus flags might be of interest for
a bridge. Remove the sampling_edge field and use bus_flags.

In the case at hand a dumb VGA bridge needs a specific data enable
polarity (DRM_BUS_FLAG_DE_LOW).

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
 include/drm/drm_bridge.h              | 11 +++++------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9b706789a341..7a5c24967115 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
  */
 static const struct drm_bridge_timings default_dac_timings = {
 	/* Timing specifications, datasheet page 7 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	.setup_time_ps = 500,
 	.hold_time_ps = 1500,
 };
@@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8134_dac_timings = {
 	/* From timing diagram, datasheet page 9 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 12 */
 	.setup_time_ps = 3000,
 	/* I guess this means latched input */
@@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8135_dac_timings = {
 	/* From timing diagram, datasheet page 14 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 16 */
 	.setup_time_ps = 2000,
 	.hold_time_ps = 500,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd850747ce54..85d4b51eae19 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -244,14 +244,13 @@ struct drm_bridge_funcs {
  */
 struct drm_bridge_timings {
 	/**
-	 * @sampling_edge:
+	 * @bus_flags:
 	 *
-	 * Tells whether the bridge samples the digital input signal
-	 * from the display engine on the positive or negative edge of the
-	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
-	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
+	 * Tells what additional settings for the pixel data on the bus
+	 * this bridge requires (like pixel signal polarity). See also
+	 * &drm_display_info->bus_flags.
 	 */
-	u32 sampling_edge;
+	u32 bus_flags;
 	/**
 	 * @setup_time_ps:
 	 *
-- 
2.18.0

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

[PATCH 2/6] drm/bridge: allow to specify data-enable polarity

[Stefan Agner]

Support boards with a passive RGB to VGA bridge which require a low
active data-enable polarity.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Alternatively a new dt binding could be introduced for dumb VGA bridges
requiring low active data enable... However, also other polarities might
need a specific polarity, hence this generic approach might be better.

 drivers/gpu/drm/bridge/dumb-vga-dac.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 7a5c24967115..2b8c3d629f2f 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -179,6 +179,7 @@ static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
 static int dumb_vga_probe(struct platform_device *pdev)
 {
 	struct dumb_vga *vga;
+	u32 de;
 
 	vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
 	if (!vga)
@@ -194,6 +195,23 @@ static int dumb_vga_probe(struct platform_device *pdev)
 		dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret);
 	}
 
+	vga->bridge.funcs = &dumb_vga_bridge_funcs;
+	vga->bridge.of_node = pdev->dev.of_node;
+	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
+
+	if (!vga->bridge.timings &&
+	    !of_property_read_u32(pdev->dev.of_node, "de-active", &de)) {
+		struct drm_bridge_timings *timings;
+
+		timings = devm_kzalloc(&pdev->dev, sizeof(*timings), GFP_KERNEL);
+		if (!timings)
+			return -ENOMEM;
+
+		timings->bus_flags = de ? DRM_BUS_FLAG_DE_HIGH :
+					DRM_BUS_FLAG_DE_LOW;
+		vga->bridge.timings = timings;
+	}
+
 	vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
 	if (IS_ERR(vga->ddc)) {
 		if (PTR_ERR(vga->ddc) == -ENODEV) {
@@ -205,10 +223,6 @@ 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 = of_device_get_match_data(&pdev->dev);
-
 	drm_bridge_add(&vga->bridge);
 
 	return 0;
-- 
2.18.0

[PATCH 2/6] drm/bridge: allow to specify data-enable polarity

[Stefan Agner]

Support boards with a passive RGB to VGA bridge which require a low
active data-enable polarity.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Alternatively a new dt binding could be introduced for dumb VGA bridges
requiring low active data enable... However, also other polarities might
need a specific polarity, hence this generic approach might be better.

 drivers/gpu/drm/bridge/dumb-vga-dac.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 7a5c24967115..2b8c3d629f2f 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -179,6 +179,7 @@ static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
 static int dumb_vga_probe(struct platform_device *pdev)
 {
 	struct dumb_vga *vga;
+	u32 de;
 
 	vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
 	if (!vga)
@@ -194,6 +195,23 @@ static int dumb_vga_probe(struct platform_device *pdev)
 		dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret);
 	}
 
+	vga->bridge.funcs = &dumb_vga_bridge_funcs;
+	vga->bridge.of_node = pdev->dev.of_node;
+	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
+
+	if (!vga->bridge.timings &&
+	    !of_property_read_u32(pdev->dev.of_node, "de-active", &de)) {
+		struct drm_bridge_timings *timings;
+
+		timings = devm_kzalloc(&pdev->dev, sizeof(*timings), GFP_KERNEL);
+		if (!timings)
+			return -ENOMEM;
+
+		timings->bus_flags = de ? DRM_BUS_FLAG_DE_HIGH :
+					DRM_BUS_FLAG_DE_LOW;
+		vga->bridge.timings = timings;
+	}
+
 	vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
 	if (IS_ERR(vga->ddc)) {
 		if (PTR_ERR(vga->ddc) == -ENODEV) {
@@ -205,10 +223,6 @@ 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 = of_device_get_match_data(&pdev->dev);
-
 	drm_bridge_add(&vga->bridge);
 
 	return 0;
-- 
2.18.0

[PATCH 2/6] drm/bridge: allow to specify data-enable polarity

[Stefan Agner]

Support boards with a passive RGB to VGA bridge which require a low
active data-enable polarity.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Alternatively a new dt binding could be introduced for dumb VGA bridges
requiring low active data enable... However, also other polarities might
need a specific polarity, hence this generic approach might be better.

 drivers/gpu/drm/bridge/dumb-vga-dac.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 7a5c24967115..2b8c3d629f2f 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -179,6 +179,7 @@ static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
 static int dumb_vga_probe(struct platform_device *pdev)
 {
 	struct dumb_vga *vga;
+	u32 de;
 
 	vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
 	if (!vga)
@@ -194,6 +195,23 @@ static int dumb_vga_probe(struct platform_device *pdev)
 		dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret);
 	}
 
+	vga->bridge.funcs = &dumb_vga_bridge_funcs;
+	vga->bridge.of_node = pdev->dev.of_node;
+	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
+
+	if (!vga->bridge.timings &&
+	    !of_property_read_u32(pdev->dev.of_node, "de-active", &de)) {
+		struct drm_bridge_timings *timings;
+
+		timings = devm_kzalloc(&pdev->dev, sizeof(*timings), GFP_KERNEL);
+		if (!timings)
+			return -ENOMEM;
+
+		timings->bus_flags = de ? DRM_BUS_FLAG_DE_HIGH :
+					DRM_BUS_FLAG_DE_LOW;
+		vga->bridge.timings = timings;
+	}
+
 	vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
 	if (IS_ERR(vga->ddc)) {
 		if (PTR_ERR(vga->ddc) == -ENODEV) {
@@ -205,10 +223,6 @@ 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 = of_device_get_match_data(&pdev->dev);
-
 	drm_bridge_add(&vga->bridge);
 
 	return 0;
-- 
2.18.0

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

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

Allow to specify the data-enable polarity required by a dumb VGA
DAC converting parallel RGB to VGA.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
index 164cbb15f04c..adbd2ca0af2f 100644
--- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
@@ -18,6 +18,7 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
 Optional properties:
 - vdd-supply: Power supply for DAC
+- de-active: data-enable pulse is active low/high/ignored
 
 Example
 -------
-- 
2.18.0

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

Allow to specify the data-enable polarity required by a dumb VGA
DAC converting parallel RGB to VGA.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
index 164cbb15f04c..adbd2ca0af2f 100644
--- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
@@ -18,6 +18,7 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
 Optional properties:
 - vdd-supply: Power supply for DAC
+- de-active: data-enable pulse is active low/high/ignored
 
 Example
 -------
-- 
2.18.0

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

Allow to specify the data-enable polarity required by a dumb VGA
DAC converting parallel RGB to VGA.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
index 164cbb15f04c..adbd2ca0af2f 100644
--- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
@@ -18,6 +18,7 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
 Optional properties:
 - vdd-supply: Power supply for DAC
+- de-active: data-enable pulse is active low/high/ignored
 
 Example
 -------
-- 
2.18.0

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

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
> Allow to specify the data-enable polarity required by a dumb VGA
> DAC converting parallel RGB to VGA.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
> 164cbb15f04c..adbd2ca0af2f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> @@ -18,6 +18,7 @@ graph bindings specified in
> Documentation/devicetree/bindings/graph.txt.
> 
>  Optional properties:
>  - vdd-supply: Power supply for DAC
> +- de-active: data-enable pulse is active low/high/ignored

Which value corresponds to low, high and ignored ?

>  Example
>  -------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
> Allow to specify the data-enable polarity required by a dumb VGA
> DAC converting parallel RGB to VGA.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
> 164cbb15f04c..adbd2ca0af2f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> @@ -18,6 +18,7 @@ graph bindings specified in
> Documentation/devicetree/bindings/graph.txt.
> 
>  Optional properties:
>  - vdd-supply: Power supply for DAC
> +- de-active: data-enable pulse is active low/high/ignored

Which value corresponds to low, high and ignored ?

>  Example
>  -------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
> Allow to specify the data-enable polarity required by a dumb VGA
> DAC converting parallel RGB to VGA.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
> 164cbb15f04c..adbd2ca0af2f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> @@ -18,6 +18,7 @@ graph bindings specified in
> Documentation/devicetree/bindings/graph.txt.
> 
>  Optional properties:
>  - vdd-supply: Power supply for DAC
> +- de-active: data-enable pulse is active low/high/ignored

Which value corresponds to low, high and ignored ?

>  Example
>  -------

-- 
Regards,

Laurent Pinchart



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

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

On 05.09.2018 00:07, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
>> Allow to specify the data-enable polarity required by a dumb VGA
>> DAC converting parallel RGB to VGA.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
>> 164cbb15f04c..adbd2ca0af2f 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> @@ -18,6 +18,7 @@ graph bindings specified in
>> Documentation/devicetree/bindings/graph.txt.
>>
>>  Optional properties:
>>  - vdd-supply: Power supply for DAC
>> +- de-active: data-enable pulse is active low/high/ignored
> 
> Which value corresponds to low, high and ignored ?
> 

The wording is taken from
Documentation/devicetree/bindings/display/panel/display-timing.txt. But
I agree, not very useful.

0 is low active, 1 is high active, and none is using driver defaults.

How about:
- de-active: data-enable pulse is 0=active low/1=active high

--
Stefan

>>  Example
>>  -------

Re: [PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

On 05.09.2018 00:07, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
>> Allow to specify the data-enable polarity required by a dumb VGA
>> DAC converting parallel RGB to VGA.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
>> 164cbb15f04c..adbd2ca0af2f 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> @@ -18,6 +18,7 @@ graph bindings specified in
>> Documentation/devicetree/bindings/graph.txt.
>>
>>  Optional properties:
>>  - vdd-supply: Power supply for DAC
>> +- de-active: data-enable pulse is active low/high/ignored
> 
> Which value corresponds to low, high and ignored ?
> 

The wording is taken from
Documentation/devicetree/bindings/display/panel/display-timing.txt. But
I agree, not very useful.

0 is low active, 1 is high active, and none is using driver defaults.

How about:
- de-active: data-enable pulse is 0=active low/1=active high

--
Stefan

>>  Example
>>  -------

Re: [PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

On 05.09.2018 00:07, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
>> Allow to specify the data-enable polarity required by a dumb VGA
>> DAC converting parallel RGB to VGA.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
>> 164cbb15f04c..adbd2ca0af2f 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> @@ -18,6 +18,7 @@ graph bindings specified in
>> Documentation/devicetree/bindings/graph.txt.
>>
>>  Optional properties:
>>  - vdd-supply: Power supply for DAC
>> +- de-active: data-enable pulse is active low/high/ignored
> 
> Which value corresponds to low, high and ignored ?
> 

The wording is taken from
Documentation/devicetree/bindings/display/panel/display-timing.txt. But
I agree, not very useful.

0 is low active, 1 is high active, and none is using driver defaults.

How about:
- de-active: data-enable pulse is 0=active low/1=active high

--
Stefan

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

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Laurent Pinchart]

Hi Stefan,

On Wednesday, 5 September 2018 21:10:08 EEST Stefan Agner wrote:
> On 05.09.2018 00:07, Laurent Pinchart wrote:
> > On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
> >> Allow to specify the data-enable polarity required by a dumb VGA
> >> DAC converting parallel RGB to VGA.
> >> 
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
> >> 164cbb15f04c..adbd2ca0af2f 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> @@ -18,6 +18,7 @@ graph bindings specified in
> >> Documentation/devicetree/bindings/graph.txt.
> >> 
> >>  Optional properties:
> >>  - vdd-supply: Power supply for DAC
> >> 
> >> +- de-active: data-enable pulse is active low/high/ignored
> > 
> > Which value corresponds to low, high and ignored ?
> 
> The wording is taken from
> Documentation/devicetree/bindings/display/panel/display-timing.txt. But
> I agree, not very useful.
> 
> 0 is low active, 1 is high active, and none is using driver defaults.
> 
> How about:
> - de-active: data-enable pulse is 0=active low/1=active high

The data enable signal isn't really a pulse. I would word this as

- de-active: Polarity of the data enable signal. 0 for active low, 1 for 
active high, unset for system-specific defaults.

> >>  Example
> >>  -------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Laurent Pinchart]

Hi Stefan,

On Wednesday, 5 September 2018 21:10:08 EEST Stefan Agner wrote:
> On 05.09.2018 00:07, Laurent Pinchart wrote:
> > On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
> >> Allow to specify the data-enable polarity required by a dumb VGA
> >> DAC converting parallel RGB to VGA.
> >> 
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
> >> 164cbb15f04c..adbd2ca0af2f 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> @@ -18,6 +18,7 @@ graph bindings specified in
> >> Documentation/devicetree/bindings/graph.txt.
> >> 
> >>  Optional properties:
> >>  - vdd-supply: Power supply for DAC
> >> 
> >> +- de-active: data-enable pulse is active low/high/ignored
> > 
> > Which value corresponds to low, high and ignored ?
> 
> The wording is taken from
> Documentation/devicetree/bindings/display/panel/display-timing.txt. But
> I agree, not very useful.
> 
> 0 is low active, 1 is high active, and none is using driver defaults.
> 
> How about:
> - de-active: data-enable pulse is 0=active low/1=active high

The data enable signal isn't really a pulse. I would word this as

- de-active: Polarity of the data enable signal. 0 for active low, 1 for 
active high, unset for system-specific defaults.

> >>  Example
> >>  -------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Laurent Pinchart]

Hi Stefan,

On Wednesday, 5 September 2018 21:10:08 EEST Stefan Agner wrote:
> On 05.09.2018 00:07, Laurent Pinchart wrote:
> > On Wednesday, 5 September 2018 08:21:10 EEST Stefan Agner wrote:
> >> Allow to specify the data-enable polarity required by a dumb VGA
> >> DAC converting parallel RGB to VGA.
> >> 
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index
> >> 164cbb15f04c..adbd2ca0af2f 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> >> @@ -18,6 +18,7 @@ graph bindings specified in
> >> Documentation/devicetree/bindings/graph.txt.
> >> 
> >>  Optional properties:
> >>  - vdd-supply: Power supply for DAC
> >> 
> >> +- de-active: data-enable pulse is active low/high/ignored
> > 
> > Which value corresponds to low, high and ignored ?
> 
> The wording is taken from
> Documentation/devicetree/bindings/display/panel/display-timing.txt. But
> I agree, not very useful.
> 
> 0 is low active, 1 is high active, and none is using driver defaults.
> 
> How about:
> - de-active: data-enable pulse is 0=active low/1=active high

The data enable signal isn't really a pulse. I would word this as

- de-active: Polarity of the data enable signal. 0 for active low, 1 for 
active high, unset for system-specific defaults.

> >>  Example
> >>  -------

-- 
Regards,

Laurent Pinchart



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

[PATCH 4/6] drm/imx: support handling bridge timings bus flags

[Stefan Agner]

A bridge might require specific settings for the pixel data on
the bus. Copy the bus flags from the bridge timings if a bridge
is in use.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/imx/parallel-display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index aefd04e18f93..ee5840cdb9ab 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -239,6 +239,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret && ret != -ENODEV)
 		return ret;
 
+	if (imxpd->bridge) {
+		const struct drm_bridge_timings *btimings = imxpd->bridge->timings;
+
+		if (btimings)
+			imxpd->bus_flags = btimings->bus_flags;
+	}
+
 	imxpd->dev = dev;
 
 	ret = imx_pd_register(drm, imxpd);
-- 
2.18.0

[PATCH 4/6] drm/imx: support handling bridge timings bus flags

[Stefan Agner]

A bridge might require specific settings for the pixel data on
the bus. Copy the bus flags from the bridge timings if a bridge
is in use.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/imx/parallel-display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index aefd04e18f93..ee5840cdb9ab 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -239,6 +239,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret && ret != -ENODEV)
 		return ret;
 
+	if (imxpd->bridge) {
+		const struct drm_bridge_timings *btimings = imxpd->bridge->timings;
+
+		if (btimings)
+			imxpd->bus_flags = btimings->bus_flags;
+	}
+
 	imxpd->dev = dev;
 
 	ret = imx_pd_register(drm, imxpd);
-- 
2.18.0

[PATCH 4/6] drm/imx: support handling bridge timings bus flags

[Stefan Agner]

A bridge might require specific settings for the pixel data on
the bus. Copy the bus flags from the bridge timings if a bridge
is in use.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/imx/parallel-display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index aefd04e18f93..ee5840cdb9ab 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -239,6 +239,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret && ret != -ENODEV)
 		return ret;
 
+	if (imxpd->bridge) {
+		const struct drm_bridge_timings *btimings = imxpd->bridge->timings;
+
+		if (btimings)
+			imxpd->bus_flags = btimings->bus_flags;
+	}
+
 	imxpd->dev = dev;
 
 	ret = imx_pd_register(drm, imxpd);
-- 
2.18.0

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

[PATCH 5/6] ARM: dts: imx6qdl-apalis: add VGA support

[Stefan Agner]

The Apalis iMX6 modules use a simple DAC using resistor ladders
to convert parallel RGB to VGA. Add support for this output using
the dump VGA driver.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx6q-apalis-eval.dts | 28 +++++++++++++
 arch/arm/boot/dts/imx6qdl-apalis.dtsi   | 52 +++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-apalis-eval.dts b/arch/arm/boot/dts/imx6q-apalis-eval.dts
index 707ac9a46115..ae07b3a105ef 100644
--- a/arch/arm/boot/dts/imx6q-apalis-eval.dts
+++ b/arch/arm/boot/dts/imx6q-apalis-eval.dts
@@ -140,6 +140,16 @@
 		regulator-max-microvolt = <3300000>;
 		regulator-always-on;
 	};
+
+	vga {
+		compatible = "vga-connector";
+
+		port {
+			vga_con_in: endpoint {
+				remote-endpoint = <&vga_bridge_out>;
+			};
+		};
+	};
 };
 
 &backlight {
@@ -281,6 +291,24 @@
 	status = "okay";
 };
 
+&vgabridge {
+	status = "okay";
+
+	ports {
+		port at 1 {
+			reg = <1>;
+
+			vga_bridge_out: endpoint {
+				remote-endpoint = <&vga_con_in>;
+			};
+		};
+	};
+};
+
+&vgadisplay {
+	status = "okay";
+};
+
 &iomuxc {
 	/*
 	 * Mux the Apalis GPIOs
diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index 05f07ea3e8c8..e8d0407e3e18 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -138,6 +138,54 @@
 		spdif-out;
 		status = "disabled";
 	};
+
+	vgabridge: bridge {
+		compatible = "dumb-vga-dac";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		de-active = <0>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port at 0 {
+				reg = <0>;
+
+				vga_bridge_in: endpoint {
+					remote-endpoint = <&vga_display_out>;
+				};
+			};
+		};
+	};
+
+	vgadisplay: display at di0 {
+		compatible = "fsl,imx-parallel-display";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interface-pix-fmt = "rgb565";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ipu2_vdac>;
+		status = "disabled";
+
+		port at 0 {
+			reg = <0>;
+
+			vga_display_in: endpoint {
+				remote-endpoint = <&ipu2_di0_disp0>;
+			};
+		};
+
+		port at 1 {
+			reg = <1>;
+
+			vga_display_out: endpoint {
+				remote-endpoint = <&vga_bridge_in>;
+			};
+		};
+	};
+
 };
 
 &audmux {
@@ -373,6 +421,10 @@
 	status = "disabled";
 };
 
+&ipu2_di0_disp0 {
+	remote-endpoint = <&vga_display_in>;
+};
+
 &pwm1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_pwm1>;
-- 
2.18.0

[PATCH 5/6] ARM: dts: imx6qdl-apalis: add VGA support

[Stefan Agner]

The Apalis iMX6 modules use a simple DAC using resistor ladders
to convert parallel RGB to VGA. Add support for this output using
the dump VGA driver.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx6q-apalis-eval.dts | 28 +++++++++++++
 arch/arm/boot/dts/imx6qdl-apalis.dtsi   | 52 +++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-apalis-eval.dts b/arch/arm/boot/dts/imx6q-apalis-eval.dts
index 707ac9a46115..ae07b3a105ef 100644
--- a/arch/arm/boot/dts/imx6q-apalis-eval.dts
+++ b/arch/arm/boot/dts/imx6q-apalis-eval.dts
@@ -140,6 +140,16 @@
 		regulator-max-microvolt = <3300000>;
 		regulator-always-on;
 	};
+
+	vga {
+		compatible = "vga-connector";
+
+		port {
+			vga_con_in: endpoint {
+				remote-endpoint = <&vga_bridge_out>;
+			};
+		};
+	};
 };
 
 &backlight {
@@ -281,6 +291,24 @@
 	status = "okay";
 };
 
+&vgabridge {
+	status = "okay";
+
+	ports {
+		port@1 {
+			reg = <1>;
+
+			vga_bridge_out: endpoint {
+				remote-endpoint = <&vga_con_in>;
+			};
+		};
+	};
+};
+
+&vgadisplay {
+	status = "okay";
+};
+
 &iomuxc {
 	/*
 	 * Mux the Apalis GPIOs
diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index 05f07ea3e8c8..e8d0407e3e18 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -138,6 +138,54 @@
 		spdif-out;
 		status = "disabled";
 	};
+
+	vgabridge: bridge {
+		compatible = "dumb-vga-dac";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		de-active = <0>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				vga_bridge_in: endpoint {
+					remote-endpoint = <&vga_display_out>;
+				};
+			};
+		};
+	};
+
+	vgadisplay: display@di0 {
+		compatible = "fsl,imx-parallel-display";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interface-pix-fmt = "rgb565";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ipu2_vdac>;
+		status = "disabled";
+
+		port@0 {
+			reg = <0>;
+
+			vga_display_in: endpoint {
+				remote-endpoint = <&ipu2_di0_disp0>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			vga_display_out: endpoint {
+				remote-endpoint = <&vga_bridge_in>;
+			};
+		};
+	};
+
 };
 
 &audmux {
@@ -373,6 +421,10 @@
 	status = "disabled";
 };
 
+&ipu2_di0_disp0 {
+	remote-endpoint = <&vga_display_in>;
+};
+
 &pwm1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_pwm1>;
-- 
2.18.0

[PATCH 5/6] ARM: dts: imx6qdl-apalis: add VGA support

[Stefan Agner]

The Apalis iMX6 modules use a simple DAC using resistor ladders
to convert parallel RGB to VGA. Add support for this output using
the dump VGA driver.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx6q-apalis-eval.dts | 28 +++++++++++++
 arch/arm/boot/dts/imx6qdl-apalis.dtsi   | 52 +++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-apalis-eval.dts b/arch/arm/boot/dts/imx6q-apalis-eval.dts
index 707ac9a46115..ae07b3a105ef 100644
--- a/arch/arm/boot/dts/imx6q-apalis-eval.dts
+++ b/arch/arm/boot/dts/imx6q-apalis-eval.dts
@@ -140,6 +140,16 @@
 		regulator-max-microvolt = <3300000>;
 		regulator-always-on;
 	};
+
+	vga {
+		compatible = "vga-connector";
+
+		port {
+			vga_con_in: endpoint {
+				remote-endpoint = <&vga_bridge_out>;
+			};
+		};
+	};
 };
 
 &backlight {
@@ -281,6 +291,24 @@
 	status = "okay";
 };
 
+&vgabridge {
+	status = "okay";
+
+	ports {
+		port@1 {
+			reg = <1>;
+
+			vga_bridge_out: endpoint {
+				remote-endpoint = <&vga_con_in>;
+			};
+		};
+	};
+};
+
+&vgadisplay {
+	status = "okay";
+};
+
 &iomuxc {
 	/*
 	 * Mux the Apalis GPIOs
diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index 05f07ea3e8c8..e8d0407e3e18 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -138,6 +138,54 @@
 		spdif-out;
 		status = "disabled";
 	};
+
+	vgabridge: bridge {
+		compatible = "dumb-vga-dac";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		de-active = <0>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				vga_bridge_in: endpoint {
+					remote-endpoint = <&vga_display_out>;
+				};
+			};
+		};
+	};
+
+	vgadisplay: display@di0 {
+		compatible = "fsl,imx-parallel-display";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interface-pix-fmt = "rgb565";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ipu2_vdac>;
+		status = "disabled";
+
+		port@0 {
+			reg = <0>;
+
+			vga_display_in: endpoint {
+				remote-endpoint = <&ipu2_di0_disp0>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			vga_display_out: endpoint {
+				remote-endpoint = <&vga_bridge_in>;
+			};
+		};
+	};
+
 };
 
 &audmux {
@@ -373,6 +421,10 @@
 	status = "disabled";
 };
 
+&ipu2_di0_disp0 {
+	remote-endpoint = <&vga_display_in>;
+};
+
 &pwm1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_pwm1>;
-- 
2.18.0

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

[PATCH 6/6] ARM: dts: imx6qdl-apalis: add GPIO I2C node for DDC

[Stefan Agner]

Currently, the DDC signals are controlled by the DWC HDMI I2C
master to be used for HDMI (DVI on the Apalis Evaluation Board).
However, the Apalis Evaluation board also allows to route the Apalis
DDC I2C to the LVDS or the VGA connector. By default, the signal
is routed to DVI (HDMI), and therefor the current default settings
are sensible.

To ease customization and to prepare for carrier boards with other
needs, add a GPIO I2C DDC node.

E.g. to reroute the Apalis DDC to the VGA connector on the Apalis
Evaluation Board, the following changes can be used:

vga {
	...
	ddc-i2c-bus = <&i2cddc>;
};

&hdmi {
	/delete-property/ pinctrl-0;
};

&i2cddc {
	status = "okay";
};

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx6qdl-apalis.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index e8d0407e3e18..8340391796df 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -61,6 +61,18 @@
 		status = "disabled";
 	};
 
+	/* DDC_I2C: I2C2_SDA/SCL on MXM3 205/207 */
+	i2cddc: i2c at 0 {
+		compatible = "i2c-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i2c_ddc>;
+		gpios = <&gpio3 16 GPIO_ACTIVE_HIGH /* sda */
+			 &gpio2 30 GPIO_ACTIVE_HIGH /* scl */
+			>;
+		i2c-gpio,delay-us = <2>;	/* ~100 kHz */
+		status = "disabled";
+	};
+
 	reg_module_3v3: regulator-module-3v3 {
 		compatible = "regulator-fixed";
 		regulator-name = "+V3.3";
@@ -688,6 +700,14 @@
 		>;
 	};
 
+	pinctrl_i2c_ddc: gpioi2cddcgrp {
+		fsl,pins = <
+			/* DDC bitbang */
+			MX6QDL_PAD_EIM_EB2__GPIO2_IO30 0x1b0b0
+			MX6QDL_PAD_EIM_D16__GPIO3_IO16 0x1b0b0
+		>;
+	};
+
 	pinctrl_i2c1: i2c1grp {
 		fsl,pins = <
 			MX6QDL_PAD_CSI0_DAT8__I2C1_SDA 0x4001b8b1
-- 
2.18.0

[PATCH 6/6] ARM: dts: imx6qdl-apalis: add GPIO I2C node for DDC

[Stefan Agner]

Currently, the DDC signals are controlled by the DWC HDMI I2C
master to be used for HDMI (DVI on the Apalis Evaluation Board).
However, the Apalis Evaluation board also allows to route the Apalis
DDC I2C to the LVDS or the VGA connector. By default, the signal
is routed to DVI (HDMI), and therefor the current default settings
are sensible.

To ease customization and to prepare for carrier boards with other
needs, add a GPIO I2C DDC node.

E.g. to reroute the Apalis DDC to the VGA connector on the Apalis
Evaluation Board, the following changes can be used:

vga {
	...
	ddc-i2c-bus = <&i2cddc>;
};

&hdmi {
	/delete-property/ pinctrl-0;
};

&i2cddc {
	status = "okay";
};

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx6qdl-apalis.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index e8d0407e3e18..8340391796df 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -61,6 +61,18 @@
 		status = "disabled";
 	};
 
+	/* DDC_I2C: I2C2_SDA/SCL on MXM3 205/207 */
+	i2cddc: i2c@0 {
+		compatible = "i2c-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i2c_ddc>;
+		gpios = <&gpio3 16 GPIO_ACTIVE_HIGH /* sda */
+			 &gpio2 30 GPIO_ACTIVE_HIGH /* scl */
+			>;
+		i2c-gpio,delay-us = <2>;	/* ~100 kHz */
+		status = "disabled";
+	};
+
 	reg_module_3v3: regulator-module-3v3 {
 		compatible = "regulator-fixed";
 		regulator-name = "+V3.3";
@@ -688,6 +700,14 @@
 		>;
 	};
 
+	pinctrl_i2c_ddc: gpioi2cddcgrp {
+		fsl,pins = <
+			/* DDC bitbang */
+			MX6QDL_PAD_EIM_EB2__GPIO2_IO30 0x1b0b0
+			MX6QDL_PAD_EIM_D16__GPIO3_IO16 0x1b0b0
+		>;
+	};
+
 	pinctrl_i2c1: i2c1grp {
 		fsl,pins = <
 			MX6QDL_PAD_CSI0_DAT8__I2C1_SDA 0x4001b8b1
-- 
2.18.0

[PATCH 6/6] ARM: dts: imx6qdl-apalis: add GPIO I2C node for DDC

[Stefan Agner]

Currently, the DDC signals are controlled by the DWC HDMI I2C
master to be used for HDMI (DVI on the Apalis Evaluation Board).
However, the Apalis Evaluation board also allows to route the Apalis
DDC I2C to the LVDS or the VGA connector. By default, the signal
is routed to DVI (HDMI), and therefor the current default settings
are sensible.

To ease customization and to prepare for carrier boards with other
needs, add a GPIO I2C DDC node.

E.g. to reroute the Apalis DDC to the VGA connector on the Apalis
Evaluation Board, the following changes can be used:

vga {
	...
	ddc-i2c-bus = <&i2cddc>;
};

&hdmi {
	/delete-property/ pinctrl-0;
};

&i2cddc {
	status = "okay";
};

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx6qdl-apalis.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index e8d0407e3e18..8340391796df 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -61,6 +61,18 @@
 		status = "disabled";
 	};
 
+	/* DDC_I2C: I2C2_SDA/SCL on MXM3 205/207 */
+	i2cddc: i2c@0 {
+		compatible = "i2c-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i2c_ddc>;
+		gpios = <&gpio3 16 GPIO_ACTIVE_HIGH /* sda */
+			 &gpio2 30 GPIO_ACTIVE_HIGH /* scl */
+			>;
+		i2c-gpio,delay-us = <2>;	/* ~100 kHz */
+		status = "disabled";
+	};
+
 	reg_module_3v3: regulator-module-3v3 {
 		compatible = "regulator-fixed";
 		regulator-name = "+V3.3";
@@ -688,6 +700,14 @@
 		>;
 	};
 
+	pinctrl_i2c_ddc: gpioi2cddcgrp {
+		fsl,pins = <
+			/* DDC bitbang */
+			MX6QDL_PAD_EIM_EB2__GPIO2_IO30 0x1b0b0
+			MX6QDL_PAD_EIM_D16__GPIO3_IO16 0x1b0b0
+		>;
+	};
+
 	pinctrl_i2c1: i2c1grp {
 		fsl,pins = <
 			MX6QDL_PAD_CSI0_DAT8__I2C1_SDA 0x4001b8b1
-- 
2.18.0

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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
> The DRM bus flags convey additional information on pixel data on
> the bus. All current available bus flags might be of interest for
> a bridge. Remove the sampling_edge field and use bus_flags.
> 
> In the case at hand a dumb VGA bridge needs a specific data enable
> polarity (DRM_BUS_FLAG_DE_LOW).
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
>  include/drm/drm_bridge.h              | 11 +++++------
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
> 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
> */
>  static const struct drm_bridge_timings default_dac_timings = {
>  	/* Timing specifications, datasheet page 7 */
> -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>  	.setup_time_ps = 500,
>  	.hold_time_ps = 1500,
>  };
> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
> default_dac_timings = { */
>  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
>  	/* From timing diagram, datasheet page 9 */
> -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>  	/* From datasheet, page 12 */
>  	.setup_time_ps = 3000,
>  	/* I guess this means latched input */
> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
> ti_ths8134_dac_timings = { */
>  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
>  	/* From timing diagram, datasheet page 14 */
> -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>  	/* From datasheet, page 16 */
>  	.setup_time_ps = 2000,
>  	.hold_time_ps = 500,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bd850747ce54..85d4b51eae19 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
>   */
>  struct drm_bridge_timings {
>  	/**
> -	 * @sampling_edge:
> +	 * @bus_flags:
>  	 *
> -	 * Tells whether the bridge samples the digital input signal
> -	 * from the display engine on the positive or negative edge of the
> -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
> -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
> +	 * Tells what additional settings for the pixel data on the bus
> +	 * this bridge requires (like pixel signal polarity). See also
> +	 * &drm_display_info->bus_flags.
>  	 */
> -	u32 sampling_edge;
> +	u32 bus_flags;

While I'm not opposed to extending the bridge structure to allow specifying 
other flags, I think we're losing information here. The sampling_edge field 
makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified on 
which clock edge signals are sampled. bus_flags could be interpreted 
differently, for instance as specifying on which clock edge signals are output 
on the other side of the bridge.

We could easily fix this by specifying that the bus_flags field refers to the 
input side of the bridge. We could also rename the field to input_bus_flags. 
The rename could be delayed until needed, but that would result in yet another 
change to all bridge drivers, so we may want to do it now as your patch 
touches all the drivers already. Other options might also be possible.

>  	/**
>  	 * @setup_time_ps:
>  	 *

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
> The DRM bus flags convey additional information on pixel data on
> the bus. All current available bus flags might be of interest for
> a bridge. Remove the sampling_edge field and use bus_flags.
> 
> In the case at hand a dumb VGA bridge needs a specific data enable
> polarity (DRM_BUS_FLAG_DE_LOW).
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
>  include/drm/drm_bridge.h              | 11 +++++------
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
> 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
> */
>  static const struct drm_bridge_timings default_dac_timings = {
>  	/* Timing specifications, datasheet page 7 */
> -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>  	.setup_time_ps = 500,
>  	.hold_time_ps = 1500,
>  };
> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
> default_dac_timings = { */
>  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
>  	/* From timing diagram, datasheet page 9 */
> -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>  	/* From datasheet, page 12 */
>  	.setup_time_ps = 3000,
>  	/* I guess this means latched input */
> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
> ti_ths8134_dac_timings = { */
>  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
>  	/* From timing diagram, datasheet page 14 */
> -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>  	/* From datasheet, page 16 */
>  	.setup_time_ps = 2000,
>  	.hold_time_ps = 500,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bd850747ce54..85d4b51eae19 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
>   */
>  struct drm_bridge_timings {
>  	/**
> -	 * @sampling_edge:
> +	 * @bus_flags:
>  	 *
> -	 * Tells whether the bridge samples the digital input signal
> -	 * from the display engine on the positive or negative edge of the
> -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
> -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
> +	 * Tells what additional settings for the pixel data on the bus
> +	 * this bridge requires (like pixel signal polarity). See also
> +	 * &drm_display_info->bus_flags.
>  	 */
> -	u32 sampling_edge;
> +	u32 bus_flags;

While I'm not opposed to extending the bridge structure to allow specifying 
other flags, I think we're losing information here. The sampling_edge field 
makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified on 
which clock edge signals are sampled. bus_flags could be interpreted 
differently, for instance as specifying on which clock edge signals are output 
on the other side of the bridge.

We could easily fix this by specifying that the bus_flags field refers to the 
input side of the bridge. We could also rename the field to input_bus_flags. 
The rename could be delayed until needed, but that would result in yet another 
change to all bridge drivers, so we may want to do it now as your patch 
touches all the drivers already. Other options might also be possible.

>  	/**
>  	 * @setup_time_ps:
>  	 *

-- 
Regards,

Laurent Pinchart

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Wednesday, 5 September 2018 10:06:28 EEST Laurent Pinchart wrote:
> On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
> > The DRM bus flags convey additional information on pixel data on
> > the bus. All current available bus flags might be of interest for
> > a bridge. Remove the sampling_edge field and use bus_flags.
> > 
> > In the case at hand a dumb VGA bridge needs a specific data enable
> > polarity (DRM_BUS_FLAG_DE_LOW).
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > ---
> > 
> >  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
> >  include/drm/drm_bridge.h              | 11 +++++------
> >  2 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
> > 100644
> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
> > *pdev) */
> > 
> >  static const struct drm_bridge_timings default_dac_timings = {
> >  
> >  	/* Timing specifications, datasheet page 7 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	.setup_time_ps = 500,
> >  	.hold_time_ps = 1500,
> >  
> >  };
> > 
> > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
> > default_dac_timings = { */
> > 
> >  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> >  
> >  	/* From timing diagram, datasheet page 9 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	/* From datasheet, page 12 */
> >  	.setup_time_ps = 3000,
> >  	/* I guess this means latched input */
> > 
> > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
> > ti_ths8134_dac_timings = { */
> > 
> >  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> >  
> >  	/* From timing diagram, datasheet page 14 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	/* From datasheet, page 16 */
> >  	.setup_time_ps = 2000,
> >  	.hold_time_ps = 500,
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index bd850747ce54..85d4b51eae19 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
> > 
> >   */
> >  
> >  struct drm_bridge_timings {
> >  
> >  	/**
> > 
> > -	 * @sampling_edge:
> > 
> > +	 * @bus_flags:
> >  	 *
> > 
> > -	 * Tells whether the bridge samples the digital input signal
> > -	 * from the display engine on the positive or negative edge of the
> > -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
> > -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
> > +	 * Tells what additional settings for the pixel data on the bus
> > +	 * this bridge requires (like pixel signal polarity). See also
> > +	 * &drm_display_info->bus_flags.
> > 
> >  	 */
> > 
> > -	u32 sampling_edge;
> > +	u32 bus_flags;
> 
> While I'm not opposed to extending the bridge structure to allow specifying
> other flags, I think we're losing information here. The sampling_edge field
> makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified
> on which clock edge signals are sampled. bus_flags could be interpreted
> differently, for instance as specifying on which clock edge signals are
> output on the other side of the bridge.
> 
> We could easily fix this by specifying that the bus_flags field refers to
> the input side of the bridge. We could also rename the field to
> input_bus_flags. The rename could be delayed until needed, but that would
> result in yet another change to all bridge drivers, so we may want to do it
> now as your patch touches all the drivers already. Other options might also
> be possible.

And I should of course make sure to wake up before reviewing patches. 
Obviously only one driver is currently affected by the rename. More will use 
the flags later though, so the argument could still hold.

> >  	/**
> >  	
> >  	 * @setup_time_ps:
> >  	 *

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Wednesday, 5 September 2018 10:06:28 EEST Laurent Pinchart wrote:
> On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
> > The DRM bus flags convey additional information on pixel data on
> > the bus. All current available bus flags might be of interest for
> > a bridge. Remove the sampling_edge field and use bus_flags.
> > 
> > In the case at hand a dumb VGA bridge needs a specific data enable
> > polarity (DRM_BUS_FLAG_DE_LOW).
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > ---
> > 
> >  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
> >  include/drm/drm_bridge.h              | 11 +++++------
> >  2 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
> > 100644
> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
> > *pdev) */
> > 
> >  static const struct drm_bridge_timings default_dac_timings = {
> >  
> >  	/* Timing specifications, datasheet page 7 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	.setup_time_ps = 500,
> >  	.hold_time_ps = 1500,
> >  
> >  };
> > 
> > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
> > default_dac_timings = { */
> > 
> >  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> >  
> >  	/* From timing diagram, datasheet page 9 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	/* From datasheet, page 12 */
> >  	.setup_time_ps = 3000,
> >  	/* I guess this means latched input */
> > 
> > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
> > ti_ths8134_dac_timings = { */
> > 
> >  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> >  
> >  	/* From timing diagram, datasheet page 14 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	/* From datasheet, page 16 */
> >  	.setup_time_ps = 2000,
> >  	.hold_time_ps = 500,
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index bd850747ce54..85d4b51eae19 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
> > 
> >   */
> >  
> >  struct drm_bridge_timings {
> >  
> >  	/**
> > 
> > -	 * @sampling_edge:
> > 
> > +	 * @bus_flags:
> >  	 *
> > 
> > -	 * Tells whether the bridge samples the digital input signal
> > -	 * from the display engine on the positive or negative edge of the
> > -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
> > -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
> > +	 * Tells what additional settings for the pixel data on the bus
> > +	 * this bridge requires (like pixel signal polarity). See also
> > +	 * &drm_display_info->bus_flags.
> > 
> >  	 */
> > 
> > -	u32 sampling_edge;
> > +	u32 bus_flags;
> 
> While I'm not opposed to extending the bridge structure to allow specifying
> other flags, I think we're losing information here. The sampling_edge field
> makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified
> on which clock edge signals are sampled. bus_flags could be interpreted
> differently, for instance as specifying on which clock edge signals are
> output on the other side of the bridge.
> 
> We could easily fix this by specifying that the bus_flags field refers to
> the input side of the bridge. We could also rename the field to
> input_bus_flags. The rename could be delayed until needed, but that would
> result in yet another change to all bridge drivers, so we may want to do it
> now as your patch touches all the drivers already. Other options might also
> be possible.

And I should of course make sure to wake up before reviewing patches. 
Obviously only one driver is currently affected by the rename. More will use 
the flags later though, so the argument could still hold.

> >  	/**
> >  	
> >  	 * @setup_time_ps:
> >  	 *

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Wednesday, 5 September 2018 10:06:28 EEST Laurent Pinchart wrote:
> On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
> > The DRM bus flags convey additional information on pixel data on
> > the bus. All current available bus flags might be of interest for
> > a bridge. Remove the sampling_edge field and use bus_flags.
> > 
> > In the case at hand a dumb VGA bridge needs a specific data enable
> > polarity (DRM_BUS_FLAG_DE_LOW).
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > ---
> > 
> >  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
> >  include/drm/drm_bridge.h              | 11 +++++------
> >  2 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
> > 100644
> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
> > *pdev) */
> > 
> >  static const struct drm_bridge_timings default_dac_timings = {
> >  
> >  	/* Timing specifications, datasheet page 7 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	.setup_time_ps = 500,
> >  	.hold_time_ps = 1500,
> >  
> >  };
> > 
> > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
> > default_dac_timings = { */
> > 
> >  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> >  
> >  	/* From timing diagram, datasheet page 9 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	/* From datasheet, page 12 */
> >  	.setup_time_ps = 3000,
> >  	/* I guess this means latched input */
> > 
> > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
> > ti_ths8134_dac_timings = { */
> > 
> >  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> >  
> >  	/* From timing diagram, datasheet page 14 */
> > 
> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> > 
> >  	/* From datasheet, page 16 */
> >  	.setup_time_ps = 2000,
> >  	.hold_time_ps = 500,
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index bd850747ce54..85d4b51eae19 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
> > 
> >   */
> >  
> >  struct drm_bridge_timings {
> >  
> >  	/**
> > 
> > -	 * @sampling_edge:
> > 
> > +	 * @bus_flags:
> >  	 *
> > 
> > -	 * Tells whether the bridge samples the digital input signal
> > -	 * from the display engine on the positive or negative edge of the
> > -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
> > -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
> > +	 * Tells what additional settings for the pixel data on the bus
> > +	 * this bridge requires (like pixel signal polarity). See also
> > +	 * &drm_display_info->bus_flags.
> > 
> >  	 */
> > 
> > -	u32 sampling_edge;
> > +	u32 bus_flags;
> 
> While I'm not opposed to extending the bridge structure to allow specifying
> other flags, I think we're losing information here. The sampling_edge field
> makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified
> on which clock edge signals are sampled. bus_flags could be interpreted
> differently, for instance as specifying on which clock edge signals are
> output on the other side of the bridge.
> 
> We could easily fix this by specifying that the bus_flags field refers to
> the input side of the bridge. We could also rename the field to
> input_bus_flags. The rename could be delayed until needed, but that would
> result in yet another change to all bridge drivers, so we may want to do it
> now as your patch touches all the drivers already. Other options might also
> be possible.

And I should of course make sure to wake up before reviewing patches. 
Obviously only one driver is currently affected by the rename. More will use 
the flags later though, so the argument could still hold.

> >  	/**
> >  	
> >  	 * @setup_time_ps:
> >  	 *

-- 
Regards,

Laurent Pinchart



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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 05.09.2018 00:44, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Wednesday, 5 September 2018 10:06:28 EEST Laurent Pinchart wrote:
>> On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
>> > The DRM bus flags convey additional information on pixel data on
>> > the bus. All current available bus flags might be of interest for
>> > a bridge. Remove the sampling_edge field and use bus_flags.
>> >
>> > In the case at hand a dumb VGA bridge needs a specific data enable
>> > polarity (DRM_BUS_FLAG_DE_LOW).
>> >
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>> > ---
>> >
>> >  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
>> >  include/drm/drm_bridge.h              | 11 +++++------
>> >  2 files changed, 8 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
>> > 100644
>> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
>> > *pdev) */
>> >
>> >  static const struct drm_bridge_timings default_dac_timings = {
>> >
>> >  	/* Timing specifications, datasheet page 7 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	.setup_time_ps = 500,
>> >  	.hold_time_ps = 1500,
>> >
>> >  };
>> >
>> > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
>> > default_dac_timings = { */
>> >
>> >  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
>> >
>> >  	/* From timing diagram, datasheet page 9 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	/* From datasheet, page 12 */
>> >  	.setup_time_ps = 3000,
>> >  	/* I guess this means latched input */
>> >
>> > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
>> > ti_ths8134_dac_timings = { */
>> >
>> >  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
>> >
>> >  	/* From timing diagram, datasheet page 14 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	/* From datasheet, page 16 */
>> >  	.setup_time_ps = 2000,
>> >  	.hold_time_ps = 500,
>> >
>> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> > index bd850747ce54..85d4b51eae19 100644
>> > --- a/include/drm/drm_bridge.h
>> > +++ b/include/drm/drm_bridge.h
>> > @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
>> >
>> >   */
>> >
>> >  struct drm_bridge_timings {
>> >
>> >  	/**
>> >
>> > -	 * @sampling_edge:
>> >
>> > +	 * @bus_flags:
>> >  	 *
>> >
>> > -	 * Tells whether the bridge samples the digital input signal
>> > -	 * from the display engine on the positive or negative edge of the
>> > -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
>> > -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
>> > +	 * Tells what additional settings for the pixel data on the bus
>> > +	 * this bridge requires (like pixel signal polarity). See also
>> > +	 * &drm_display_info->bus_flags.
>> >
>> >  	 */
>> >
>> > -	u32 sampling_edge;
>> > +	u32 bus_flags;
>>
>> While I'm not opposed to extending the bridge structure to allow specifying
>> other flags, I think we're losing information here. The sampling_edge field
>> makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified
>> on which clock edge signals are sampled. bus_flags could be interpreted
>> differently, for instance as specifying on which clock edge signals are
>> output on the other side of the bridge.

Good point! I actually really don't like that we use the same flags here
but from a different perspective. Especially since the flags defines
document things differently:

/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)

Using the opposite perspective would also need translation in crtc
drivers... So far no driver uses sampling_edge.

I would prefer if we always use the meaning as documented by the flags.

I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
DRM_BUS_FLAG_PIXDATA_NEGEDGE.

Linus Walleij, you added sampling edge, any thoughts?


>>
>> We could easily fix this by specifying that the bus_flags field refers to
>> the input side of the bridge. We could also rename the field to
>> input_bus_flags. The rename could be delayed until needed, but that would
>> result in yet another change to all bridge drivers, so we may want to do it
>> now as your patch touches all the drivers already. Other options might also
>> be possible.

Naming the field input_bus_flags seems very sensible. How about:

struct drm_bridge_timings {
	/**
	 * @input_bus_flags:
	 *
	 * Specifies the settings for the pixel data on the input
	 * bus of this bridge (like pixel signal polarity). Note the
	 * flags are typically controller centric! See also
	 * &drm_display_info->bus_flags.
	 */
	u32 input_bus_flags;

> 
> And I should of course make sure to wake up before reviewing patches. 
> Obviously only one driver is currently affected by the rename. More will use 
> the flags later though, so the argument could still hold.

It is only one bridge driver making use of bridge timings. As far as I
can see currently no driver actually makes use of the bridge timings
sampling_edge currently...

But yes, agreed, we should make a sensible choice now to avoid churn
down the line.

--
Stefan

> 
>> >  	/**
>> >
>> >  	 * @setup_time_ps:
>> >  	 *

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 05.09.2018 00:44, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Wednesday, 5 September 2018 10:06:28 EEST Laurent Pinchart wrote:
>> On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
>> > The DRM bus flags convey additional information on pixel data on
>> > the bus. All current available bus flags might be of interest for
>> > a bridge. Remove the sampling_edge field and use bus_flags.
>> >
>> > In the case at hand a dumb VGA bridge needs a specific data enable
>> > polarity (DRM_BUS_FLAG_DE_LOW).
>> >
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>> > ---
>> >
>> >  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
>> >  include/drm/drm_bridge.h              | 11 +++++------
>> >  2 files changed, 8 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
>> > 100644
>> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
>> > *pdev) */
>> >
>> >  static const struct drm_bridge_timings default_dac_timings = {
>> >
>> >  	/* Timing specifications, datasheet page 7 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	.setup_time_ps = 500,
>> >  	.hold_time_ps = 1500,
>> >
>> >  };
>> >
>> > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
>> > default_dac_timings = { */
>> >
>> >  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
>> >
>> >  	/* From timing diagram, datasheet page 9 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	/* From datasheet, page 12 */
>> >  	.setup_time_ps = 3000,
>> >  	/* I guess this means latched input */
>> >
>> > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
>> > ti_ths8134_dac_timings = { */
>> >
>> >  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
>> >
>> >  	/* From timing diagram, datasheet page 14 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	/* From datasheet, page 16 */
>> >  	.setup_time_ps = 2000,
>> >  	.hold_time_ps = 500,
>> >
>> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> > index bd850747ce54..85d4b51eae19 100644
>> > --- a/include/drm/drm_bridge.h
>> > +++ b/include/drm/drm_bridge.h
>> > @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
>> >
>> >   */
>> >
>> >  struct drm_bridge_timings {
>> >
>> >  	/**
>> >
>> > -	 * @sampling_edge:
>> >
>> > +	 * @bus_flags:
>> >  	 *
>> >
>> > -	 * Tells whether the bridge samples the digital input signal
>> > -	 * from the display engine on the positive or negative edge of the
>> > -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
>> > -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
>> > +	 * Tells what additional settings for the pixel data on the bus
>> > +	 * this bridge requires (like pixel signal polarity). See also
>> > +	 * &drm_display_info->bus_flags.
>> >
>> >  	 */
>> >
>> > -	u32 sampling_edge;
>> > +	u32 bus_flags;
>>
>> While I'm not opposed to extending the bridge structure to allow specifying
>> other flags, I think we're losing information here. The sampling_edge field
>> makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified
>> on which clock edge signals are sampled. bus_flags could be interpreted
>> differently, for instance as specifying on which clock edge signals are
>> output on the other side of the bridge.

Good point! I actually really don't like that we use the same flags here
but from a different perspective. Especially since the flags defines
document things differently:

/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)

Using the opposite perspective would also need translation in crtc
drivers... So far no driver uses sampling_edge.

I would prefer if we always use the meaning as documented by the flags.

I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
DRM_BUS_FLAG_PIXDATA_NEGEDGE.

Linus Walleij, you added sampling edge, any thoughts?


>>
>> We could easily fix this by specifying that the bus_flags field refers to
>> the input side of the bridge. We could also rename the field to
>> input_bus_flags. The rename could be delayed until needed, but that would
>> result in yet another change to all bridge drivers, so we may want to do it
>> now as your patch touches all the drivers already. Other options might also
>> be possible.

Naming the field input_bus_flags seems very sensible. How about:

struct drm_bridge_timings {
	/**
	 * @input_bus_flags:
	 *
	 * Specifies the settings for the pixel data on the input
	 * bus of this bridge (like pixel signal polarity). Note the
	 * flags are typically controller centric! See also
	 * &drm_display_info->bus_flags.
	 */
	u32 input_bus_flags;

> 
> And I should of course make sure to wake up before reviewing patches. 
> Obviously only one driver is currently affected by the rename. More will use 
> the flags later though, so the argument could still hold.

It is only one bridge driver making use of bridge timings. As far as I
can see currently no driver actually makes use of the bridge timings
sampling_edge currently...

But yes, agreed, we should make a sensible choice now to avoid churn
down the line.

--
Stefan

> 
>> >  	/**
>> >
>> >  	 * @setup_time_ps:
>> >  	 *

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 05.09.2018 00:44, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Wednesday, 5 September 2018 10:06:28 EEST Laurent Pinchart wrote:
>> On Wednesday, 5 September 2018 08:21:08 EEST Stefan Agner wrote:
>> > The DRM bus flags convey additional information on pixel data on
>> > the bus. All current available bus flags might be of interest for
>> > a bridge. Remove the sampling_edge field and use bus_flags.
>> >
>> > In the case at hand a dumb VGA bridge needs a specific data enable
>> > polarity (DRM_BUS_FLAG_DE_LOW).
>> >
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>> > ---
>> >
>> >  drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
>> >  include/drm/drm_bridge.h              | 11 +++++------
>> >  2 files changed, 8 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7a5c24967115
>> > 100644
>> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
>> > *pdev) */
>> >
>> >  static const struct drm_bridge_timings default_dac_timings = {
>> >
>> >  	/* Timing specifications, datasheet page 7 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	.setup_time_ps = 500,
>> >  	.hold_time_ps = 1500,
>> >
>> >  };
>> >
>> > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
>> > default_dac_timings = { */
>> >
>> >  static const struct drm_bridge_timings ti_ths8134_dac_timings = {
>> >
>> >  	/* From timing diagram, datasheet page 9 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	/* From datasheet, page 12 */
>> >  	.setup_time_ps = 3000,
>> >  	/* I guess this means latched input */
>> >
>> > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
>> > ti_ths8134_dac_timings = { */
>> >
>> >  static const struct drm_bridge_timings ti_ths8135_dac_timings = {
>> >
>> >  	/* From timing diagram, datasheet page 14 */
>> >
>> > -	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>> >
>> >  	/* From datasheet, page 16 */
>> >  	.setup_time_ps = 2000,
>> >  	.hold_time_ps = 500,
>> >
>> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> > index bd850747ce54..85d4b51eae19 100644
>> > --- a/include/drm/drm_bridge.h
>> > +++ b/include/drm/drm_bridge.h
>> > @@ -244,14 +244,13 @@ struct drm_bridge_funcs {
>> >
>> >   */
>> >
>> >  struct drm_bridge_timings {
>> >
>> >  	/**
>> >
>> > -	 * @sampling_edge:
>> >
>> > +	 * @bus_flags:
>> >  	 *
>> >
>> > -	 * Tells whether the bridge samples the digital input signal
>> > -	 * from the display engine on the positive or negative edge of the
>> > -	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
>> > -	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
>> > +	 * Tells what additional settings for the pixel data on the bus
>> > +	 * this bridge requires (like pixel signal polarity). See also
>> > +	 * &drm_display_info->bus_flags.
>> >
>> >  	 */
>> >
>> > -	u32 sampling_edge;
>> > +	u32 bus_flags;
>>
>> While I'm not opposed to extending the bridge structure to allow specifying
>> other flags, I think we're losing information here. The sampling_edge field
>> makes it clear that the DRM_BUS_FLAG_PIXDATA_(NEG|POS)EDGE flags specified
>> on which clock edge signals are sampled. bus_flags could be interpreted
>> differently, for instance as specifying on which clock edge signals are
>> output on the other side of the bridge.

Good point! I actually really don't like that we use the same flags here
but from a different perspective. Especially since the flags defines
document things differently:

/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)

Using the opposite perspective would also need translation in crtc
drivers... So far no driver uses sampling_edge.

I would prefer if we always use the meaning as documented by the flags.

I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
DRM_BUS_FLAG_PIXDATA_NEGEDGE.

Linus Walleij, you added sampling edge, any thoughts?


>>
>> We could easily fix this by specifying that the bus_flags field refers to
>> the input side of the bridge. We could also rename the field to
>> input_bus_flags. The rename could be delayed until needed, but that would
>> result in yet another change to all bridge drivers, so we may want to do it
>> now as your patch touches all the drivers already. Other options might also
>> be possible.

Naming the field input_bus_flags seems very sensible. How about:

struct drm_bridge_timings {
	/**
	 * @input_bus_flags:
	 *
	 * Specifies the settings for the pixel data on the input
	 * bus of this bridge (like pixel signal polarity). Note the
	 * flags are typically controller centric! See also
	 * &drm_display_info->bus_flags.
	 */
	u32 input_bus_flags;

> 
> And I should of course make sure to wake up before reviewing patches. 
> Obviously only one driver is currently affected by the rename. More will use 
> the flags later though, so the argument could still hold.

It is only one bridge driver making use of bridge timings. As far as I
can see currently no driver actually makes use of the bridge timings
sampling_edge currently...

But yes, agreed, we should make a sensible choice now to avoid churn
down the line.

--
Stefan

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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Linus Walleij]

On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> On 05.09.2018 00:44, Laurent Pinchart wrote:

> Good point! I actually really don't like that we use the same flags here
> but from a different perspective. Especially since the flags defines
> document things differently:
>
> /* drive data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> /* drive data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)

Maybe a stupid comment from my side, but can't we just change the
documentation to match the usecases?

/* Trigger pixel data latch on positive edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)

> Using the opposite perspective would also need translation in crtc
> drivers... So far no driver uses sampling_edge.
>
> I would prefer if we always use the meaning as documented by the flags.
>
> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>
> Linus Walleij, you added sampling edge, any thoughts?

I just thought it was generally useful to have triggering edge encoded
into the bridge as it makes it clear that this edge is something
that is a delayed version of the driving edge which is subject to
clock skew caused by the speed of electrons in silicon and
copper and slew rate caused by parasitic capacitance.

Yours,
Linus Walleij

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Linus Walleij]

On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> On 05.09.2018 00:44, Laurent Pinchart wrote:

> Good point! I actually really don't like that we use the same flags here
> but from a different perspective. Especially since the flags defines
> document things differently:
>
> /* drive data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> /* drive data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)

Maybe a stupid comment from my side, but can't we just change the
documentation to match the usecases?

/* Trigger pixel data latch on positive edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)

> Using the opposite perspective would also need translation in crtc
> drivers... So far no driver uses sampling_edge.
>
> I would prefer if we always use the meaning as documented by the flags.
>
> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>
> Linus Walleij, you added sampling edge, any thoughts?

I just thought it was generally useful to have triggering edge encoded
into the bridge as it makes it clear that this edge is something
that is a delayed version of the driving edge which is subject to
clock skew caused by the speed of electrons in silicon and
copper and slew rate caused by parasitic capacitance.

Yours,
Linus Walleij

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Linus Walleij]

On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> On 05.09.2018 00:44, Laurent Pinchart wrote:

> Good point! I actually really don't like that we use the same flags here
> but from a different perspective. Especially since the flags defines
> document things differently:
>
> /* drive data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> /* drive data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)

Maybe a stupid comment from my side, but can't we just change the
documentation to match the usecases?

/* Trigger pixel data latch on positive edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)

> Using the opposite perspective would also need translation in crtc
> drivers... So far no driver uses sampling_edge.
>
> I would prefer if we always use the meaning as documented by the flags.
>
> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>
> Linus Walleij, you added sampling edge, any thoughts?

I just thought it was generally useful to have triggering edge encoded
into the bridge as it makes it clear that this edge is something
that is a delayed version of the driving edge which is subject to
clock skew caused by the speed of electrons in silicon and
copper and slew rate caused by parasitic capacitance.

Yours,
Linus Walleij

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Linus,

On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> > On 05.09.2018 00:44, Laurent Pinchart wrote:
> > 
> > Good point! I actually really don't like that we use the same flags here
> > but from a different perspective. Especially since the flags defines
> > document things differently:
> > 
> > /* drive data on pos. edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > /* drive data on neg. edge */
> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> 
> Maybe a stupid comment from my side, but can't we just change the
> documentation to match the usecases?
> 
> /* Trigger pixel data latch on positive edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)

The flags are used for the drm_connector bus_flags field, and really mean 
driving on the positive/negative edges. We this can't change their meaning 
like this.

> > Using the opposite perspective would also need translation in crtc
> > drivers... So far no driver uses sampling_edge.
> > 
> > I would prefer if we always use the meaning as documented by the flags.
> > 
> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> > 
> > Linus Walleij, you added sampling edge, any thoughts?
> 
> I just thought it was generally useful to have triggering edge encoded
> into the bridge as it makes it clear that this edge is something
> that is a delayed version of the driving edge which is subject to
> clock skew caused by the speed of electrons in silicon and
> copper and slew rate caused by parasitic capacitance.

I agree that we need both the driving and sampling edge. In many case they 
will be opposite, and providing some kind of appropriate defaults in APIs is 
fine by me, but we need a way to specify both when needed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Linus,

On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> > On 05.09.2018 00:44, Laurent Pinchart wrote:
> > 
> > Good point! I actually really don't like that we use the same flags here
> > but from a different perspective. Especially since the flags defines
> > document things differently:
> > 
> > /* drive data on pos. edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > /* drive data on neg. edge */
> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> 
> Maybe a stupid comment from my side, but can't we just change the
> documentation to match the usecases?
> 
> /* Trigger pixel data latch on positive edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)

The flags are used for the drm_connector bus_flags field, and really mean 
driving on the positive/negative edges. We this can't change their meaning 
like this.

> > Using the opposite perspective would also need translation in crtc
> > drivers... So far no driver uses sampling_edge.
> > 
> > I would prefer if we always use the meaning as documented by the flags.
> > 
> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> > 
> > Linus Walleij, you added sampling edge, any thoughts?
> 
> I just thought it was generally useful to have triggering edge encoded
> into the bridge as it makes it clear that this edge is something
> that is a delayed version of the driving edge which is subject to
> clock skew caused by the speed of electrons in silicon and
> copper and slew rate caused by parasitic capacitance.

I agree that we need both the driving and sampling edge. In many case they 
will be opposite, and providing some kind of appropriate defaults in APIs is 
fine by me, but we need a way to specify both when needed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Linus,

On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> > On 05.09.2018 00:44, Laurent Pinchart wrote:
> > 
> > Good point! I actually really don't like that we use the same flags here
> > but from a different perspective. Especially since the flags defines
> > document things differently:
> > 
> > /* drive data on pos. edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > /* drive data on neg. edge */
> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> 
> Maybe a stupid comment from my side, but can't we just change the
> documentation to match the usecases?
> 
> /* Trigger pixel data latch on positive edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)

The flags are used for the drm_connector bus_flags field, and really mean 
driving on the positive/negative edges. We this can't change their meaning 
like this.

> > Using the opposite perspective would also need translation in crtc
> > drivers... So far no driver uses sampling_edge.
> > 
> > I would prefer if we always use the meaning as documented by the flags.
> > 
> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> > 
> > Linus Walleij, you added sampling edge, any thoughts?
> 
> I just thought it was generally useful to have triggering edge encoded
> into the bridge as it makes it clear that this edge is something
> that is a delayed version of the driving edge which is subject to
> clock skew caused by the speed of electrons in silicon and
> copper and slew rate caused by parasitic capacitance.

I agree that we need both the driving and sampling edge. In many case they 
will be opposite, and providing some kind of appropriate defaults in APIs is 
fine by me, but we need a way to specify both when needed.

-- 
Regards,

Laurent Pinchart

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 05:25, Laurent Pinchart wrote:
> Hi Linus,
> 
> On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
>> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> > On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >
>> > Good point! I actually really don't like that we use the same flags here
>> > but from a different perspective. Especially since the flags defines
>> > document things differently:
>> >
>> > /* drive data on pos. edge */
>> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> > /* drive data on neg. edge */
>> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>>
>> Maybe a stupid comment from my side, but can't we just change the
>> documentation to match the usecases?
>>
>> /* Trigger pixel data latch on positive edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> 
> The flags are used for the drm_connector bus_flags field, and really mean 
> driving on the positive/negative edges. We this can't change their meaning 
> like this.
> 
>> > Using the opposite perspective would also need translation in crtc
>> > drivers... So far no driver uses sampling_edge.
>> >
>> > I would prefer if we always use the meaning as documented by the flags.
>> >
>> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >
>> > Linus Walleij, you added sampling edge, any thoughts?
>>
>> I just thought it was generally useful to have triggering edge encoded
>> into the bridge as it makes it clear that this edge is something
>> that is a delayed version of the driving edge which is subject to
>> clock skew caused by the speed of electrons in silicon and
>> copper and slew rate caused by parasitic capacitance.
> 
> I agree that we need both the driving and sampling edge. In many case they 
> will be opposite, and providing some kind of appropriate defaults in APIs is 
> fine by me, but we need a way to specify both when needed.

We do have pixel clock flags for displays, but also they are actually
controller oriented:

https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.h#L15

I guess having different flags to denote driving and sampling edge
independently would be ideal. But then, is there really a use case where
it wouldn't be the exact opposite?

The other bus flags are actually fine as is. I suggest to just stick
with the bus flags as we have them now, at least for now.

Alternatively, we could provide "consumer/bridge" oriented flags which
use the same bit and just are the opposite of the controller oriented
flags, e.g.:

/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
/* sample data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
/* sample data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

--
Stefan

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 05:25, Laurent Pinchart wrote:
> Hi Linus,
> 
> On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
>> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> > On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >
>> > Good point! I actually really don't like that we use the same flags here
>> > but from a different perspective. Especially since the flags defines
>> > document things differently:
>> >
>> > /* drive data on pos. edge */
>> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> > /* drive data on neg. edge */
>> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>>
>> Maybe a stupid comment from my side, but can't we just change the
>> documentation to match the usecases?
>>
>> /* Trigger pixel data latch on positive edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> 
> The flags are used for the drm_connector bus_flags field, and really mean 
> driving on the positive/negative edges. We this can't change their meaning 
> like this.
> 
>> > Using the opposite perspective would also need translation in crtc
>> > drivers... So far no driver uses sampling_edge.
>> >
>> > I would prefer if we always use the meaning as documented by the flags.
>> >
>> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >
>> > Linus Walleij, you added sampling edge, any thoughts?
>>
>> I just thought it was generally useful to have triggering edge encoded
>> into the bridge as it makes it clear that this edge is something
>> that is a delayed version of the driving edge which is subject to
>> clock skew caused by the speed of electrons in silicon and
>> copper and slew rate caused by parasitic capacitance.
> 
> I agree that we need both the driving and sampling edge. In many case they 
> will be opposite, and providing some kind of appropriate defaults in APIs is 
> fine by me, but we need a way to specify both when needed.

We do have pixel clock flags for displays, but also they are actually
controller oriented:

https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.h#L15

I guess having different flags to denote driving and sampling edge
independently would be ideal. But then, is there really a use case where
it wouldn't be the exact opposite?

The other bus flags are actually fine as is. I suggest to just stick
with the bus flags as we have them now, at least for now.

Alternatively, we could provide "consumer/bridge" oriented flags which
use the same bit and just are the opposite of the controller oriented
flags, e.g.:

/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
/* sample data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
/* sample data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

--
Stefan

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 05:25, Laurent Pinchart wrote:
> Hi Linus,
> 
> On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
>> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> > On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >
>> > Good point! I actually really don't like that we use the same flags here
>> > but from a different perspective. Especially since the flags defines
>> > document things differently:
>> >
>> > /* drive data on pos. edge */
>> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> > /* drive data on neg. edge */
>> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>>
>> Maybe a stupid comment from my side, but can't we just change the
>> documentation to match the usecases?
>>
>> /* Trigger pixel data latch on positive edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> 
> The flags are used for the drm_connector bus_flags field, and really mean 
> driving on the positive/negative edges. We this can't change their meaning 
> like this.
> 
>> > Using the opposite perspective would also need translation in crtc
>> > drivers... So far no driver uses sampling_edge.
>> >
>> > I would prefer if we always use the meaning as documented by the flags.
>> >
>> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >
>> > Linus Walleij, you added sampling edge, any thoughts?
>>
>> I just thought it was generally useful to have triggering edge encoded
>> into the bridge as it makes it clear that this edge is something
>> that is a delayed version of the driving edge which is subject to
>> clock skew caused by the speed of electrons in silicon and
>> copper and slew rate caused by parasitic capacitance.
> 
> I agree that we need both the driving and sampling edge. In many case they 
> will be opposite, and providing some kind of appropriate defaults in APIs is 
> fine by me, but we need a way to specify both when needed.

We do have pixel clock flags for displays, but also they are actually
controller oriented:

https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.h#L15

I guess having different flags to denote driving and sampling edge
independently would be ideal. But then, is there really a use case where
it wouldn't be the exact opposite?

The other bus flags are actually fine as is. I suggest to just stick
with the bus flags as we have them now, at least for now.

Alternatively, we could provide "consumer/bridge" oriented flags which
use the same bit and just are the opposite of the controller oriented
flags, e.g.:

/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
/* sample data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
/* sample data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

--
Stefan



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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 19:48:51 EEST Stefan Agner wrote:
> On 06.09.2018 05:25, Laurent Pinchart wrote:
> > On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
> >> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >> > On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> > 
> >> > Good point! I actually really don't like that we use the same flags
> >> > here
> >> > but from a different perspective. Especially since the flags defines
> >> > document things differently:
> >> > 
> >> > /* drive data on pos. edge */
> >> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> > /* drive data on neg. edge */
> >> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> >> 
> >> Maybe a stupid comment from my side, but can't we just change the
> >> documentation to match the usecases?
> >> 
> >> /* Trigger pixel data latch on positive edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> > The flags are used for the drm_connector bus_flags field, and really mean
> > driving on the positive/negative edges. We this can't change their meaning
> > like this.
> > 
> >> > Using the opposite perspective would also need translation in crtc
> >> > drivers... So far no driver uses sampling_edge.
> >> > 
> >> > I would prefer if we always use the meaning as documented by the flags.
> >> > 
> >> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> > 
> >> > Linus Walleij, you added sampling edge, any thoughts?
> >> 
> >> I just thought it was generally useful to have triggering edge encoded
> >> into the bridge as it makes it clear that this edge is something
> >> that is a delayed version of the driving edge which is subject to
> >> clock skew caused by the speed of electrons in silicon and
> >> copper and slew rate caused by parasitic capacitance.
> > 
> > I agree that we need both the driving and sampling edge. In many case they
> > will be opposite, and providing some kind of appropriate defaults in APIs
> > is fine by me, but we need a way to specify both when needed.
> 
> We do have pixel clock flags for displays, but also they are actually
> controller oriented:
> 
> https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.
> h#L15
> 
> I guess having different flags to denote driving and sampling edge
> independently would be ideal. But then, is there really a use case where
> it wouldn't be the exact opposite?

Yes, for instance when the transmission delay for clock and data signals is 
different, you may want to sample on the same edge used by the transmitter to 
latch the output. Linus had that case, which prompted him to add the sampling 
edge field.

> The other bus flags are actually fine as is. I suggest to just stick
> with the bus flags as we have them now, at least for now.
> 
> Alternatively, we could provide "consumer/bridge" oriented flags which
> use the same bit and just are the opposite of the controller oriented
> flags, e.g.:
> 
> /* drive data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> /* drive data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> /* sample data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
> /* sample data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 19:48:51 EEST Stefan Agner wrote:
> On 06.09.2018 05:25, Laurent Pinchart wrote:
> > On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
> >> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >> > On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> > 
> >> > Good point! I actually really don't like that we use the same flags
> >> > here
> >> > but from a different perspective. Especially since the flags defines
> >> > document things differently:
> >> > 
> >> > /* drive data on pos. edge */
> >> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> > /* drive data on neg. edge */
> >> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> >> 
> >> Maybe a stupid comment from my side, but can't we just change the
> >> documentation to match the usecases?
> >> 
> >> /* Trigger pixel data latch on positive edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> > The flags are used for the drm_connector bus_flags field, and really mean
> > driving on the positive/negative edges. We this can't change their meaning
> > like this.
> > 
> >> > Using the opposite perspective would also need translation in crtc
> >> > drivers... So far no driver uses sampling_edge.
> >> > 
> >> > I would prefer if we always use the meaning as documented by the flags.
> >> > 
> >> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> > 
> >> > Linus Walleij, you added sampling edge, any thoughts?
> >> 
> >> I just thought it was generally useful to have triggering edge encoded
> >> into the bridge as it makes it clear that this edge is something
> >> that is a delayed version of the driving edge which is subject to
> >> clock skew caused by the speed of electrons in silicon and
> >> copper and slew rate caused by parasitic capacitance.
> > 
> > I agree that we need both the driving and sampling edge. In many case they
> > will be opposite, and providing some kind of appropriate defaults in APIs
> > is fine by me, but we need a way to specify both when needed.
> 
> We do have pixel clock flags for displays, but also they are actually
> controller oriented:
> 
> https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.
> h#L15
> 
> I guess having different flags to denote driving and sampling edge
> independently would be ideal. But then, is there really a use case where
> it wouldn't be the exact opposite?

Yes, for instance when the transmission delay for clock and data signals is 
different, you may want to sample on the same edge used by the transmitter to 
latch the output. Linus had that case, which prompted him to add the sampling 
edge field.

> The other bus flags are actually fine as is. I suggest to just stick
> with the bus flags as we have them now, at least for now.
> 
> Alternatively, we could provide "consumer/bridge" oriented flags which
> use the same bit and just are the opposite of the controller oriented
> flags, e.g.:
> 
> /* drive data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> /* drive data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> /* sample data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
> /* sample data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 19:48:51 EEST Stefan Agner wrote:
> On 06.09.2018 05:25, Laurent Pinchart wrote:
> > On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
> >> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >> > On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> > 
> >> > Good point! I actually really don't like that we use the same flags
> >> > here
> >> > but from a different perspective. Especially since the flags defines
> >> > document things differently:
> >> > 
> >> > /* drive data on pos. edge */
> >> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> > /* drive data on neg. edge */
> >> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> >> 
> >> Maybe a stupid comment from my side, but can't we just change the
> >> documentation to match the usecases?
> >> 
> >> /* Trigger pixel data latch on positive edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> > The flags are used for the drm_connector bus_flags field, and really mean
> > driving on the positive/negative edges. We this can't change their meaning
> > like this.
> > 
> >> > Using the opposite perspective would also need translation in crtc
> >> > drivers... So far no driver uses sampling_edge.
> >> > 
> >> > I would prefer if we always use the meaning as documented by the flags.
> >> > 
> >> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> > 
> >> > Linus Walleij, you added sampling edge, any thoughts?
> >> 
> >> I just thought it was generally useful to have triggering edge encoded
> >> into the bridge as it makes it clear that this edge is something
> >> that is a delayed version of the driving edge which is subject to
> >> clock skew caused by the speed of electrons in silicon and
> >> copper and slew rate caused by parasitic capacitance.
> > 
> > I agree that we need both the driving and sampling edge. In many case they
> > will be opposite, and providing some kind of appropriate defaults in APIs
> > is fine by me, but we need a way to specify both when needed.
> 
> We do have pixel clock flags for displays, but also they are actually
> controller oriented:
> 
> https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.
> h#L15
> 
> I guess having different flags to denote driving and sampling edge
> independently would be ideal. But then, is there really a use case where
> it wouldn't be the exact opposite?

Yes, for instance when the transmission delay for clock and data signals is 
different, you may want to sample on the same edge used by the transmitter to 
latch the output. Linus had that case, which prompted him to add the sampling 
edge field.

> The other bus flags are actually fine as is. I suggest to just stick
> with the bus flags as we have them now, at least for now.
> 
> Alternatively, we could provide "consumer/bridge" oriented flags which
> use the same bit and just are the opposite of the controller oriented
> flags, e.g.:
> 
> /* drive data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
> /* drive data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
> /* sample data on neg. edge */
> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
> /* sample data on pos. edge */
> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

-- 
Regards,

Laurent Pinchart

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 09:54, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 19:48:51 EEST Stefan Agner wrote:
>> On 06.09.2018 05:25, Laurent Pinchart wrote:
>> > On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
>> >> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> >> > On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >> >
>> >> > Good point! I actually really don't like that we use the same flags
>> >> > here
>> >> > but from a different perspective. Especially since the flags defines
>> >> > document things differently:
>> >> >
>> >> > /* drive data on pos. edge */
>> >> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >> > /* drive data on neg. edge */
>> >> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>> >>
>> >> Maybe a stupid comment from my side, but can't we just change the
>> >> documentation to match the usecases?
>> >>
>> >> /* Trigger pixel data latch on positive edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >
>> > The flags are used for the drm_connector bus_flags field, and really mean
>> > driving on the positive/negative edges. We this can't change their meaning
>> > like this.
>> >
>> >> > Using the opposite perspective would also need translation in crtc
>> >> > drivers... So far no driver uses sampling_edge.
>> >> >
>> >> > I would prefer if we always use the meaning as documented by the flags.
>> >> >
>> >> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> >> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >> >
>> >> > Linus Walleij, you added sampling edge, any thoughts?
>> >>
>> >> I just thought it was generally useful to have triggering edge encoded
>> >> into the bridge as it makes it clear that this edge is something
>> >> that is a delayed version of the driving edge which is subject to
>> >> clock skew caused by the speed of electrons in silicon and
>> >> copper and slew rate caused by parasitic capacitance.
>> >
>> > I agree that we need both the driving and sampling edge. In many case they
>> > will be opposite, and providing some kind of appropriate defaults in APIs
>> > is fine by me, but we need a way to specify both when needed.
>>
>> We do have pixel clock flags for displays, but also they are actually
>> controller oriented:
>>
>> https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.
>> h#L15
>>
>> I guess having different flags to denote driving and sampling edge
>> independently would be ideal. But then, is there really a use case where
>> it wouldn't be the exact opposite?
> 
> Yes, for instance when the transmission delay for clock and data signals is 
> different, you may want to sample on the same edge used by the transmitter to 
> latch the output. Linus had that case, which prompted him to add the sampling 
> edge field.
> 

Bu in that case the controller does not know what it actually should do
with the sampling information alone either...

Where is that code handling that today? sample_edge has been added, but
I only see setup_time_ps being used in pl111_display.c.

I'm not a hardware guy, but according to my rough calculation a pixel
clock signal at 100MHz would need to be 1.5m off compared to the data
signals to skew by a half period! Length matching is usually only
necessary at really high speed signals. Is that really what is going on
here?

Why not just work around by specifying "the wrong" edge...?

--
Stefan

>> The other bus flags are actually fine as is. I suggest to just stick
>> with the bus flags as we have them now, at least for now.
>>
>> Alternatively, we could provide "consumer/bridge" oriented flags which
>> use the same bit and just are the opposite of the controller oriented
>> flags, e.g.:
>>
>> /* drive data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
>> /* drive data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
>> /* sample data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
>> /* sample data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 09:54, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 19:48:51 EEST Stefan Agner wrote:
>> On 06.09.2018 05:25, Laurent Pinchart wrote:
>> > On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
>> >> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> >> > On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >> >
>> >> > Good point! I actually really don't like that we use the same flags
>> >> > here
>> >> > but from a different perspective. Especially since the flags defines
>> >> > document things differently:
>> >> >
>> >> > /* drive data on pos. edge */
>> >> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >> > /* drive data on neg. edge */
>> >> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>> >>
>> >> Maybe a stupid comment from my side, but can't we just change the
>> >> documentation to match the usecases?
>> >>
>> >> /* Trigger pixel data latch on positive edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >
>> > The flags are used for the drm_connector bus_flags field, and really mean
>> > driving on the positive/negative edges. We this can't change their meaning
>> > like this.
>> >
>> >> > Using the opposite perspective would also need translation in crtc
>> >> > drivers... So far no driver uses sampling_edge.
>> >> >
>> >> > I would prefer if we always use the meaning as documented by the flags.
>> >> >
>> >> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> >> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >> >
>> >> > Linus Walleij, you added sampling edge, any thoughts?
>> >>
>> >> I just thought it was generally useful to have triggering edge encoded
>> >> into the bridge as it makes it clear that this edge is something
>> >> that is a delayed version of the driving edge which is subject to
>> >> clock skew caused by the speed of electrons in silicon and
>> >> copper and slew rate caused by parasitic capacitance.
>> >
>> > I agree that we need both the driving and sampling edge. In many case they
>> > will be opposite, and providing some kind of appropriate defaults in APIs
>> > is fine by me, but we need a way to specify both when needed.
>>
>> We do have pixel clock flags for displays, but also they are actually
>> controller oriented:
>>
>> https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.
>> h#L15
>>
>> I guess having different flags to denote driving and sampling edge
>> independently would be ideal. But then, is there really a use case where
>> it wouldn't be the exact opposite?
> 
> Yes, for instance when the transmission delay for clock and data signals is 
> different, you may want to sample on the same edge used by the transmitter to 
> latch the output. Linus had that case, which prompted him to add the sampling 
> edge field.
> 

Bu in that case the controller does not know what it actually should do
with the sampling information alone either...

Where is that code handling that today? sample_edge has been added, but
I only see setup_time_ps being used in pl111_display.c.

I'm not a hardware guy, but according to my rough calculation a pixel
clock signal at 100MHz would need to be 1.5m off compared to the data
signals to skew by a half period! Length matching is usually only
necessary at really high speed signals. Is that really what is going on
here?

Why not just work around by specifying "the wrong" edge...?

--
Stefan

>> The other bus flags are actually fine as is. I suggest to just stick
>> with the bus flags as we have them now, at least for now.
>>
>> Alternatively, we could provide "consumer/bridge" oriented flags which
>> use the same bit and just are the opposite of the controller oriented
>> flags, e.g.:
>>
>> /* drive data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
>> /* drive data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
>> /* sample data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
>> /* sample data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 09:54, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 19:48:51 EEST Stefan Agner wrote:
>> On 06.09.2018 05:25, Laurent Pinchart wrote:
>> > On Thursday, 6 September 2018 14:07:41 EEST Linus Walleij wrote:
>> >> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> >> > On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >> >
>> >> > Good point! I actually really don't like that we use the same flags
>> >> > here
>> >> > but from a different perspective. Especially since the flags defines
>> >> > document things differently:
>> >> >
>> >> > /* drive data on pos. edge */
>> >> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >> > /* drive data on neg. edge */
>> >> > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>> >>
>> >> Maybe a stupid comment from my side, but can't we just change the
>> >> documentation to match the usecases?
>> >>
>> >> /* Trigger pixel data latch on positive edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >
>> > The flags are used for the drm_connector bus_flags field, and really mean
>> > driving on the positive/negative edges. We this can't change their meaning
>> > like this.
>> >
>> >> > Using the opposite perspective would also need translation in crtc
>> >> > drivers... So far no driver uses sampling_edge.
>> >> >
>> >> > I would prefer if we always use the meaning as documented by the flags.
>> >> >
>> >> > I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> >> > DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >> >
>> >> > Linus Walleij, you added sampling edge, any thoughts?
>> >>
>> >> I just thought it was generally useful to have triggering edge encoded
>> >> into the bridge as it makes it clear that this edge is something
>> >> that is a delayed version of the driving edge which is subject to
>> >> clock skew caused by the speed of electrons in silicon and
>> >> copper and slew rate caused by parasitic capacitance.
>> >
>> > I agree that we need both the driving and sampling edge. In many case they
>> > will be opposite, and providing some kind of appropriate defaults in APIs
>> > is fine by me, but we need a way to specify both when needed.
>>
>> We do have pixel clock flags for displays, but also they are actually
>> controller oriented:
>>
>> https://elixir.bootlin.com/linux/latest/source/include/video/display_timing.
>> h#L15
>>
>> I guess having different flags to denote driving and sampling edge
>> independently would be ideal. But then, is there really a use case where
>> it wouldn't be the exact opposite?
> 
> Yes, for instance when the transmission delay for clock and data signals is 
> different, you may want to sample on the same edge used by the transmitter to 
> latch the output. Linus had that case, which prompted him to add the sampling 
> edge field.
> 

Bu in that case the controller does not know what it actually should do
with the sampling information alone either...

Where is that code handling that today? sample_edge has been added, but
I only see setup_time_ps being used in pl111_display.c.

I'm not a hardware guy, but according to my rough calculation a pixel
clock signal at 100MHz would need to be 1.5m off compared to the data
signals to skew by a half period! Length matching is usually only
necessary at really high speed signals. Is that really what is going on
here?

Why not just work around by specifying "the wrong" edge...?

--
Stefan

>> The other bus flags are actually fine as is. I suggest to just stick
>> with the bus flags as we have them now, at least for now.
>>
>> Alternatively, we could provide "consumer/bridge" oriented flags which
>> use the same bit and just are the opposite of the controller oriented
>> flags, e.g.:
>>
>> /* drive data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)
>> /* drive data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE	(1<<3)
>> /* sample data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE	(1<<2)
>> /* sample data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE	(1<<3)

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 04:07, Linus Walleij wrote:
> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> On 05.09.2018 00:44, Laurent Pinchart wrote:
> 
>> Good point! I actually really don't like that we use the same flags here
>> but from a different perspective. Especially since the flags defines
>> document things differently:
>>
>> /* drive data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> /* drive data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> 
> Maybe a stupid comment from my side, but can't we just change the
> documentation to match the usecases?
> 
> /* Trigger pixel data latch on positive edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> 
>> Using the opposite perspective would also need translation in crtc
>> drivers... So far no driver uses sampling_edge.
>>
>> I would prefer if we always use the meaning as documented by the flags.
>>
>> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>>
>> Linus Walleij, you added sampling edge, any thoughts?
> 
> I just thought it was generally useful to have triggering edge encoded
> into the bridge as it makes it clear that this edge is something
> that is a delayed version of the driving edge which is subject to
> clock skew caused by the speed of electrons in silicon and
> copper and slew rate caused by parasitic capacitance.

Ok, I read a bit up on the history of bridge timing, especially:
https://www.spinics.net/lists/dri-devel/msg155618.html

IMHO, this got overengineered. For displays we do not need all that
setup/sample delay timing information, and much longer cables are in
use. So why is all that needed for bridges?

For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
clearly states:
Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.

So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
should be used, which makes the pl111 driver setting TIM2_IPC: 
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

> DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it 
> doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE 
> would match the hardware, but would break display for some modes, depending on 
> the display clock frequency as the internal 8.5ns output delay applied to a 
> falling clock edge would fall right into the 1.7ns setup + hold time window of 
> the ADV7123 around the rising edge. I can't test this right now as I don't 
> have local access to boards using the ADV7123, but from a quick calculation 
> that ignores the PCB transmission delay modes with frequencies between 57MHz 
> and 71MHz could break if the data was output on the falling edge of the clock.

If clocks vs. data signal are really that much off on R-Car DU, then
parallel displays must have the very same issue...

Are you sure that only the clock signal has an output delay? And that
this output delay is a fixed value, clock independent?

Typically, delays apply to all signals equally, and often are clock
frequency dependent...

Without really looking at the signals, I would not jump to conclusions
here! I am pretty sure that driving on negative edge works just as well.

--
Stefan

> 
> Yours,
> Linus Walleij

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 04:07, Linus Walleij wrote:
> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> On 05.09.2018 00:44, Laurent Pinchart wrote:
> 
>> Good point! I actually really don't like that we use the same flags here
>> but from a different perspective. Especially since the flags defines
>> document things differently:
>>
>> /* drive data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> /* drive data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> 
> Maybe a stupid comment from my side, but can't we just change the
> documentation to match the usecases?
> 
> /* Trigger pixel data latch on positive edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> 
>> Using the opposite perspective would also need translation in crtc
>> drivers... So far no driver uses sampling_edge.
>>
>> I would prefer if we always use the meaning as documented by the flags.
>>
>> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>>
>> Linus Walleij, you added sampling edge, any thoughts?
> 
> I just thought it was generally useful to have triggering edge encoded
> into the bridge as it makes it clear that this edge is something
> that is a delayed version of the driving edge which is subject to
> clock skew caused by the speed of electrons in silicon and
> copper and slew rate caused by parasitic capacitance.

Ok, I read a bit up on the history of bridge timing, especially:
https://www.spinics.net/lists/dri-devel/msg155618.html

IMHO, this got overengineered. For displays we do not need all that
setup/sample delay timing information, and much longer cables are in
use. So why is all that needed for bridges?

For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
clearly states:
Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.

So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
should be used, which makes the pl111 driver setting TIM2_IPC: 
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

> DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it 
> doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE 
> would match the hardware, but would break display for some modes, depending on 
> the display clock frequency as the internal 8.5ns output delay applied to a 
> falling clock edge would fall right into the 1.7ns setup + hold time window of 
> the ADV7123 around the rising edge. I can't test this right now as I don't 
> have local access to boards using the ADV7123, but from a quick calculation 
> that ignores the PCB transmission delay modes with frequencies between 57MHz 
> and 71MHz could break if the data was output on the falling edge of the clock.

If clocks vs. data signal are really that much off on R-Car DU, then
parallel displays must have the very same issue...

Are you sure that only the clock signal has an output delay? And that
this output delay is a fixed value, clock independent?

Typically, delays apply to all signals equally, and often are clock
frequency dependent...

Without really looking at the signals, I would not jump to conclusions
here! I am pretty sure that driving on negative edge works just as well.

--
Stefan

> 
> Yours,
> Linus Walleij

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 06.09.2018 04:07, Linus Walleij wrote:
> On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> On 05.09.2018 00:44, Laurent Pinchart wrote:
> 
>> Good point! I actually really don't like that we use the same flags here
>> but from a different perspective. Especially since the flags defines
>> document things differently:
>>
>> /* drive data on pos. edge */
>> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> /* drive data on neg. edge */
>> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> 
> Maybe a stupid comment from my side, but can't we just change the
> documentation to match the usecases?
> 
> /* Trigger pixel data latch on positive edge */
> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> 
>> Using the opposite perspective would also need translation in crtc
>> drivers... So far no driver uses sampling_edge.
>>
>> I would prefer if we always use the meaning as documented by the flags.
>>
>> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>>
>> Linus Walleij, you added sampling edge, any thoughts?
> 
> I just thought it was generally useful to have triggering edge encoded
> into the bridge as it makes it clear that this edge is something
> that is a delayed version of the driving edge which is subject to
> clock skew caused by the speed of electrons in silicon and
> copper and slew rate caused by parasitic capacitance.

Ok, I read a bit up on the history of bridge timing, especially:
https://www.spinics.net/lists/dri-devel/msg155618.html

IMHO, this got overengineered. For displays we do not need all that
setup/sample delay timing information, and much longer cables are in
use. So why is all that needed for bridges?

For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
clearly states:
Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.

So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
should be used, which makes the pl111 driver setting TIM2_IPC: 
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

> DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it 
> doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE 
> would match the hardware, but would break display for some modes, depending on 
> the display clock frequency as the internal 8.5ns output delay applied to a 
> falling clock edge would fall right into the 1.7ns setup + hold time window of 
> the ADV7123 around the rising edge. I can't test this right now as I don't 
> have local access to boards using the ADV7123, but from a quick calculation 
> that ignores the PCB transmission delay modes with frequencies between 57MHz 
> and 71MHz could break if the data was output on the falling edge of the clock.

If clocks vs. data signal are really that much off on R-Car DU, then
parallel displays must have the very same issue...

Are you sure that only the clock signal has an output delay? And that
this output delay is a fixed value, clock independent?

Typically, delays apply to all signals equally, and often are clock
frequency dependent...

Without really looking at the signals, I would not jump to conclusions
here! I am pretty sure that driving on negative edge works just as well.

--
Stefan

> 
> Yours,
> Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Linus Walleij]

On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:

> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
>
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?

I also avoided the overhead of creating this abstraction initially.

But after doing it I have this Stockholm syndrome that I start
liking what Laurent told me to do.

> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

That is easy to say, but if I just set that up in code, even with a good
comment it is hard for the next reader to understand what is going
on. The central question will be, why does PL111 need to do this
but not R-Car though they are using the same bridge?

So this elaborate model gives a better transfer of abstract concepts
to whoever needs to touch that code next. The code is not just
logic, but also our map of the world and the documentation of our
problem space.

Donald Knuth has this idea about literate programming which even
turns the documentation/implementation process around. We are
not there, not even remotely, but IMO the more complex the problem.
the more we need to convey our thinking, not just our solution.

Yours,
Linus Walleij

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Linus Walleij]

On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:

> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
>
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?

I also avoided the overhead of creating this abstraction initially.

But after doing it I have this Stockholm syndrome that I start
liking what Laurent told me to do.

> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

That is easy to say, but if I just set that up in code, even with a good
comment it is hard for the next reader to understand what is going
on. The central question will be, why does PL111 need to do this
but not R-Car though they are using the same bridge?

So this elaborate model gives a better transfer of abstract concepts
to whoever needs to touch that code next. The code is not just
logic, but also our map of the world and the documentation of our
problem space.

Donald Knuth has this idea about literate programming which even
turns the documentation/implementation process around. We are
not there, not even remotely, but IMO the more complex the problem.
the more we need to convey our thinking, not just our solution.

Yours,
Linus Walleij

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Linus Walleij]

On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:

> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
>
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?

I also avoided the overhead of creating this abstraction initially.

But after doing it I have this Stockholm syndrome that I start
liking what Laurent told me to do.

> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

That is easy to say, but if I just set that up in code, even with a good
comment it is hard for the next reader to understand what is going
on. The central question will be, why does PL111 need to do this
but not R-Car though they are using the same bridge?

So this elaborate model gives a better transfer of abstract concepts
to whoever needs to touch that code next. The code is not just
logic, but also our map of the world and the documentation of our
problem space.

Donald Knuth has this idea about literate programming which even
turns the documentation/implementation process around. We are
not there, not even remotely, but IMO the more complex the problem.
the more we need to convey our thinking, not just our solution.

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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 07.09.2018 00:10, Linus Walleij wrote:
> On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:
> 
>> Ok, I read a bit up on the history of bridge timing, especially:
>> https://www.spinics.net/lists/dri-devel/msg155618.html
>>
>> IMHO, this got overengineered. For displays we do not need all that
>> setup/sample delay timing information, and much longer cables are in
>> use. So why is all that needed for bridges?
> 
> I also avoided the overhead of creating this abstraction initially.
> 
> But after doing it I have this Stockholm syndrome that I start
> liking what Laurent told me to do.
> 
>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html
> 
> That is easy to say, but if I just set that up in code, even with a good
> comment it is hard for the next reader to understand what is going
> on. The central question will be, why does PL111 need to do this
> but not R-Car though they are using the same bridge?

That is something I have experienced before. Depending on drive
strength, and I guess sampling delay etc, it might sample just late
enough that signals are stable and no issues are really visible on the
screen. In those cases different cabling, changes in drive strength, or
even production variations might suddenly show artefacts though... My
point being, just because it supposedly works, does not mean that it is
right.

I did introduce the bus flags a while ago:
https://patchwork.kernel.org/patch/8254801/
https://patchwork.kernel.org/patch/8254821/

And then had to debug quite some controller and display issues around
pixel clock polarity:
https://patchwork.kernel.org/patch/8117181/
https://patchwork.kernel.org/patch/9465485/

E.g. in that last case, the pixel clock polarity flags were interpreted
wrong on both controller and display side. Hence this worked fine for
the case already upstream, but it did not work in my case...

In the end, best thing is to look on the bus. Parallel display signals
are still rather slow, and usually rather easy to scope.

PL111 needs to do this because its standard setting is driving on rising
edge. However, most displays as well as that bridge sample data on
rising edge.

I guess R-Car DU drives on negative edge by default, but can't say for
sure without hardware/data sheet.

As far as I can tell the R-Car DU driver also does not use the
DRM_BUS_FLAG_PIXDATA_*EDGE flags currently.



> 
> So this elaborate model gives a better transfer of abstract concepts
> to whoever needs to touch that code next. The code is not just
> logic, but also our map of the world and the documentation of our
> problem space.
> 
> Donald Knuth has this idea about literate programming which even
> turns the documentation/implementation process around. We are
> not there, not even remotely, but IMO the more complex the problem.
> the more we need to convey our thinking, not just our solution.

Fully agree.

My argument here is, that the map of the world (code) and the problem
are not aligned.

--
Stefan

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 07.09.2018 00:10, Linus Walleij wrote:
> On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:
> 
>> Ok, I read a bit up on the history of bridge timing, especially:
>> https://www.spinics.net/lists/dri-devel/msg155618.html
>>
>> IMHO, this got overengineered. For displays we do not need all that
>> setup/sample delay timing information, and much longer cables are in
>> use. So why is all that needed for bridges?
> 
> I also avoided the overhead of creating this abstraction initially.
> 
> But after doing it I have this Stockholm syndrome that I start
> liking what Laurent told me to do.
> 
>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html
> 
> That is easy to say, but if I just set that up in code, even with a good
> comment it is hard for the next reader to understand what is going
> on. The central question will be, why does PL111 need to do this
> but not R-Car though they are using the same bridge?

That is something I have experienced before. Depending on drive
strength, and I guess sampling delay etc, it might sample just late
enough that signals are stable and no issues are really visible on the
screen. In those cases different cabling, changes in drive strength, or
even production variations might suddenly show artefacts though... My
point being, just because it supposedly works, does not mean that it is
right.

I did introduce the bus flags a while ago:
https://patchwork.kernel.org/patch/8254801/
https://patchwork.kernel.org/patch/8254821/

And then had to debug quite some controller and display issues around
pixel clock polarity:
https://patchwork.kernel.org/patch/8117181/
https://patchwork.kernel.org/patch/9465485/

E.g. in that last case, the pixel clock polarity flags were interpreted
wrong on both controller and display side. Hence this worked fine for
the case already upstream, but it did not work in my case...

In the end, best thing is to look on the bus. Parallel display signals
are still rather slow, and usually rather easy to scope.

PL111 needs to do this because its standard setting is driving on rising
edge. However, most displays as well as that bridge sample data on
rising edge.

I guess R-Car DU drives on negative edge by default, but can't say for
sure without hardware/data sheet.

As far as I can tell the R-Car DU driver also does not use the
DRM_BUS_FLAG_PIXDATA_*EDGE flags currently.



> 
> So this elaborate model gives a better transfer of abstract concepts
> to whoever needs to touch that code next. The code is not just
> logic, but also our map of the world and the documentation of our
> problem space.
> 
> Donald Knuth has this idea about literate programming which even
> turns the documentation/implementation process around. We are
> not there, not even remotely, but IMO the more complex the problem.
> the more we need to convey our thinking, not just our solution.

Fully agree.

My argument here is, that the map of the world (code) and the problem
are not aligned.

--
Stefan

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 07.09.2018 00:10, Linus Walleij wrote:
> On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:
> 
>> Ok, I read a bit up on the history of bridge timing, especially:
>> https://www.spinics.net/lists/dri-devel/msg155618.html
>>
>> IMHO, this got overengineered. For displays we do not need all that
>> setup/sample delay timing information, and much longer cables are in
>> use. So why is all that needed for bridges?
> 
> I also avoided the overhead of creating this abstraction initially.
> 
> But after doing it I have this Stockholm syndrome that I start
> liking what Laurent told me to do.
> 
>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html
> 
> That is easy to say, but if I just set that up in code, even with a good
> comment it is hard for the next reader to understand what is going
> on. The central question will be, why does PL111 need to do this
> but not R-Car though they are using the same bridge?

That is something I have experienced before. Depending on drive
strength, and I guess sampling delay etc, it might sample just late
enough that signals are stable and no issues are really visible on the
screen. In those cases different cabling, changes in drive strength, or
even production variations might suddenly show artefacts though... My
point being, just because it supposedly works, does not mean that it is
right.

I did introduce the bus flags a while ago:
https://patchwork.kernel.org/patch/8254801/
https://patchwork.kernel.org/patch/8254821/

And then had to debug quite some controller and display issues around
pixel clock polarity:
https://patchwork.kernel.org/patch/8117181/
https://patchwork.kernel.org/patch/9465485/

E.g. in that last case, the pixel clock polarity flags were interpreted
wrong on both controller and display side. Hence this worked fine for
the case already upstream, but it did not work in my case...

In the end, best thing is to look on the bus. Parallel display signals
are still rather slow, and usually rather easy to scope.

PL111 needs to do this because its standard setting is driving on rising
edge. However, most displays as well as that bridge sample data on
rising edge.

I guess R-Car DU drives on negative edge by default, but can't say for
sure without hardware/data sheet.

As far as I can tell the R-Car DU driver also does not use the
DRM_BUS_FLAG_PIXDATA_*EDGE flags currently.



> 
> So this elaborate model gives a better transfer of abstract concepts
> to whoever needs to touch that code next. The code is not just
> logic, but also our map of the world and the documentation of our
> problem space.
> 
> Donald Knuth has this idea about literate programming which even
> turns the documentation/implementation process around. We are
> not there, not even remotely, but IMO the more complex the problem.
> the more we need to convey our thinking, not just our solution.

Fully agree.

My argument here is, that the map of the world (code) and the problem
are not aligned.

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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Friday, 7 September 2018 21:25:40 EEST Stefan Agner wrote:
> On 07.09.2018 00:10, Linus Walleij wrote:
> > On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:
> >> Ok, I read a bit up on the history of bridge timing, especially:
> >> https://www.spinics.net/lists/dri-devel/msg155618.html
> >> 
> >> IMHO, this got overengineered. For displays we do not need all that
> >> setup/sample delay timing information, and much longer cables are in
> >> use. So why is all that needed for bridges?
> > 
> > I also avoided the overhead of creating this abstraction initially.
> > 
> > But after doing it I have this Stockholm syndrome that I start
> > liking what Laurent told me to do.
> > 
> >> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> >> clearly states:
> >> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> >> 
> >> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> >> should be used, which makes the pl111 driver setting TIM2_IPC:
> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/inde
> >> x.html
> > 
> > That is easy to say, but if I just set that up in code, even with a good
> > comment it is hard for the next reader to understand what is going
> > on. The central question will be, why does PL111 need to do this
> > but not R-Car though they are using the same bridge?
> 
> That is something I have experienced before. Depending on drive
> strength, and I guess sampling delay etc, it might sample just late
> enough that signals are stable and no issues are really visible on the
> screen. In those cases different cabling, changes in drive strength, or
> even production variations might suddenly show artefacts though... My
> point being, just because it supposedly works, does not mean that it is
> right.
> 
> I did introduce the bus flags a while ago:
> https://patchwork.kernel.org/patch/8254801/
> https://patchwork.kernel.org/patch/8254821/
> 
> And then had to debug quite some controller and display issues around
> pixel clock polarity:
> https://patchwork.kernel.org/patch/8117181/
> https://patchwork.kernel.org/patch/9465485/
> 
> E.g. in that last case, the pixel clock polarity flags were interpreted
> wrong on both controller and display side. Hence this worked fine for
> the case already upstream, but it did not work in my case...
> 
> In the end, best thing is to look on the bus. Parallel display signals
> are still rather slow, and usually rather easy to scope.
> 
> PL111 needs to do this because its standard setting is driving on rising
> edge. However, most displays as well as that bridge sample data on
> rising edge.
> 
> I guess R-Car DU drives on negative edge by default, but can't say for
> sure without hardware/data sheet.

No, it drives on the rising edge by default.

> As far as I can tell the R-Car DU driver also does not use the
> DRM_BUS_FLAG_PIXDATA_*EDGE flags currently.

That's because the hardware default happens to work. As soon as I'll need to 
support a system that requires the other edge, I'll have to add the 
corresponding logic to the driver. So far it hasn't been needed, so I haven't 
bothered.

> > So this elaborate model gives a better transfer of abstract concepts
> > to whoever needs to touch that code next. The code is not just
> > logic, but also our map of the world and the documentation of our
> > problem space.
> > 
> > Donald Knuth has this idea about literate programming which even
> > turns the documentation/implementation process around. We are
> > not there, not even remotely, but IMO the more complex the problem.
> > the more we need to convey our thinking, not just our solution.
> 
> Fully agree.
> 
> My argument here is, that the map of the world (code) and the problem
> are not aligned.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Friday, 7 September 2018 21:25:40 EEST Stefan Agner wrote:
> On 07.09.2018 00:10, Linus Walleij wrote:
> > On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:
> >> Ok, I read a bit up on the history of bridge timing, especially:
> >> https://www.spinics.net/lists/dri-devel/msg155618.html
> >> 
> >> IMHO, this got overengineered. For displays we do not need all that
> >> setup/sample delay timing information, and much longer cables are in
> >> use. So why is all that needed for bridges?
> > 
> > I also avoided the overhead of creating this abstraction initially.
> > 
> > But after doing it I have this Stockholm syndrome that I start
> > liking what Laurent told me to do.
> > 
> >> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> >> clearly states:
> >> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> >> 
> >> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> >> should be used, which makes the pl111 driver setting TIM2_IPC:
> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/inde
> >> x.html
> > 
> > That is easy to say, but if I just set that up in code, even with a good
> > comment it is hard for the next reader to understand what is going
> > on. The central question will be, why does PL111 need to do this
> > but not R-Car though they are using the same bridge?
> 
> That is something I have experienced before. Depending on drive
> strength, and I guess sampling delay etc, it might sample just late
> enough that signals are stable and no issues are really visible on the
> screen. In those cases different cabling, changes in drive strength, or
> even production variations might suddenly show artefacts though... My
> point being, just because it supposedly works, does not mean that it is
> right.
> 
> I did introduce the bus flags a while ago:
> https://patchwork.kernel.org/patch/8254801/
> https://patchwork.kernel.org/patch/8254821/
> 
> And then had to debug quite some controller and display issues around
> pixel clock polarity:
> https://patchwork.kernel.org/patch/8117181/
> https://patchwork.kernel.org/patch/9465485/
> 
> E.g. in that last case, the pixel clock polarity flags were interpreted
> wrong on both controller and display side. Hence this worked fine for
> the case already upstream, but it did not work in my case...
> 
> In the end, best thing is to look on the bus. Parallel display signals
> are still rather slow, and usually rather easy to scope.
> 
> PL111 needs to do this because its standard setting is driving on rising
> edge. However, most displays as well as that bridge sample data on
> rising edge.
> 
> I guess R-Car DU drives on negative edge by default, but can't say for
> sure without hardware/data sheet.

No, it drives on the rising edge by default.

> As far as I can tell the R-Car DU driver also does not use the
> DRM_BUS_FLAG_PIXDATA_*EDGE flags currently.

That's because the hardware default happens to work. As soon as I'll need to 
support a system that requires the other edge, I'll have to add the 
corresponding logic to the driver. So far it hasn't been needed, so I haven't 
bothered.

> > So this elaborate model gives a better transfer of abstract concepts
> > to whoever needs to touch that code next. The code is not just
> > logic, but also our map of the world and the documentation of our
> > problem space.
> > 
> > Donald Knuth has this idea about literate programming which even
> > turns the documentation/implementation process around. We are
> > not there, not even remotely, but IMO the more complex the problem.
> > the more we need to convey our thinking, not just our solution.
> 
> Fully agree.
> 
> My argument here is, that the map of the world (code) and the problem
> are not aligned.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Friday, 7 September 2018 21:25:40 EEST Stefan Agner wrote:
> On 07.09.2018 00:10, Linus Walleij wrote:
> > On Thu, Sep 6, 2018 at 10:25 PM Stefan Agner <stefan@agner.ch> wrote:
> >> Ok, I read a bit up on the history of bridge timing, especially:
> >> https://www.spinics.net/lists/dri-devel/msg155618.html
> >> 
> >> IMHO, this got overengineered. For displays we do not need all that
> >> setup/sample delay timing information, and much longer cables are in
> >> use. So why is all that needed for bridges?
> > 
> > I also avoided the overhead of creating this abstraction initially.
> > 
> > But after doing it I have this Stockholm syndrome that I start
> > liking what Laurent told me to do.
> > 
> >> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> >> clearly states:
> >> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> >> 
> >> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> >> should be used, which makes the pl111 driver setting TIM2_IPC:
> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/inde
> >> x.html
> > 
> > That is easy to say, but if I just set that up in code, even with a good
> > comment it is hard for the next reader to understand what is going
> > on. The central question will be, why does PL111 need to do this
> > but not R-Car though they are using the same bridge?
> 
> That is something I have experienced before. Depending on drive
> strength, and I guess sampling delay etc, it might sample just late
> enough that signals are stable and no issues are really visible on the
> screen. In those cases different cabling, changes in drive strength, or
> even production variations might suddenly show artefacts though... My
> point being, just because it supposedly works, does not mean that it is
> right.
> 
> I did introduce the bus flags a while ago:
> https://patchwork.kernel.org/patch/8254801/
> https://patchwork.kernel.org/patch/8254821/
> 
> And then had to debug quite some controller and display issues around
> pixel clock polarity:
> https://patchwork.kernel.org/patch/8117181/
> https://patchwork.kernel.org/patch/9465485/
> 
> E.g. in that last case, the pixel clock polarity flags were interpreted
> wrong on both controller and display side. Hence this worked fine for
> the case already upstream, but it did not work in my case...
> 
> In the end, best thing is to look on the bus. Parallel display signals
> are still rather slow, and usually rather easy to scope.
> 
> PL111 needs to do this because its standard setting is driving on rising
> edge. However, most displays as well as that bridge sample data on
> rising edge.
> 
> I guess R-Car DU drives on negative edge by default, but can't say for
> sure without hardware/data sheet.

No, it drives on the rising edge by default.

> As far as I can tell the R-Car DU driver also does not use the
> DRM_BUS_FLAG_PIXDATA_*EDGE flags currently.

That's because the hardware default happens to work. As soon as I'll need to 
support a system that requires the other edge, I'll have to add the 
corresponding logic to the driver. So far it hasn't been needed, so I haven't 
bothered.

> > So this elaborate model gives a better transfer of abstract concepts
> > to whoever needs to touch that code next. The code is not just
> > logic, but also our map of the world and the documentation of our
> > problem space.
> > 
> > Donald Knuth has this idea about literate programming which even
> > turns the documentation/implementation process around. We are
> > not there, not even remotely, but IMO the more complex the problem.
> > the more we need to convey our thinking, not just our solution.
> 
> Fully agree.
> 
> My argument here is, that the map of the world (code) and the problem
> are not aligned.

-- 
Regards,

Laurent Pinchart



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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> On 06.09.2018 04:07, Linus Walleij wrote:
> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> 
> >> Good point! I actually really don't like that we use the same flags here
> >> but from a different perspective. Especially since the flags defines
> >> document things differently:
> >> 
> >> /* drive data on pos. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> /* drive data on neg. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > 
> > Maybe a stupid comment from my side, but can't we just change the
> > documentation to match the usecases?
> > 
> > /* Trigger pixel data latch on positive edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> >> Using the opposite perspective would also need translation in crtc
> >> drivers... So far no driver uses sampling_edge.
> >> 
> >> I would prefer if we always use the meaning as documented by the flags.
> >> 
> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> 
> >> Linus Walleij, you added sampling edge, any thoughts?
> > 
> > I just thought it was generally useful to have triggering edge encoded
> > into the bridge as it makes it clear that this edge is something
> > that is a delayed version of the driving edge which is subject to
> > clock skew caused by the speed of electrons in silicon and
> > copper and slew rate caused by parasitic capacitance.
> 
> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
> 
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?
> 
> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> 
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
> tml
> 
> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > would match the hardware, but would break display for some modes,
> > depending on the display clock frequency as the internal 8.5ns output
> > delay applied to a falling clock edge would fall right into the 1.7ns
> > setup + hold time window of the ADV7123 around the rising edge. I can't
> > test this right now as I don't have local access to boards using the
> > ADV7123, but from a quick calculation that ignores the PCB transmission
> > delay modes with frequencies between 57MHz and 71MHz could break if the
> > data was output on the falling edge of the clock.
> 
> If clocks vs. data signal are really that much off on R-Car DU, then
> parallel displays must have the very same issue...
> 
> Are you sure that only the clock signal has an output delay? And that
> this output delay is a fixed value, clock independent?
> 
> Typically, delays apply to all signals equally, and often are clock
> frequency dependent...
> 
> Without really looking at the signals, I would not jump to conclusions
> here! I am pretty sure that driving on negative edge works just as well.

I've tested Linus' original patch and it broke display on R-Car, so, no, it 
doesn't work :-)

The R-Car display engine delays the clock internally (in some cases that delay 
is even configurable, and that's not uncommon in display controllers). We 
really need all this information, and I believe we need it for panels too, not 
just for bridges. The fact that we managed to get away without adding it to 
panels is likely due to the large number of panels out there, which makes it 
less likely that the same panel gets used by different systems in mainline 
with different clock delays. I expect that some panel drivers report the wrong 
clock edge to make things work on the board they were tested with, and I 
expect we'll eventually need to add the same information for panels too.

So please don't remove this useful API, otherwise you'll break my board, and I 
won't be happy.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> On 06.09.2018 04:07, Linus Walleij wrote:
> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> 
> >> Good point! I actually really don't like that we use the same flags here
> >> but from a different perspective. Especially since the flags defines
> >> document things differently:
> >> 
> >> /* drive data on pos. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> /* drive data on neg. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > 
> > Maybe a stupid comment from my side, but can't we just change the
> > documentation to match the usecases?
> > 
> > /* Trigger pixel data latch on positive edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> >> Using the opposite perspective would also need translation in crtc
> >> drivers... So far no driver uses sampling_edge.
> >> 
> >> I would prefer if we always use the meaning as documented by the flags.
> >> 
> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> 
> >> Linus Walleij, you added sampling edge, any thoughts?
> > 
> > I just thought it was generally useful to have triggering edge encoded
> > into the bridge as it makes it clear that this edge is something
> > that is a delayed version of the driving edge which is subject to
> > clock skew caused by the speed of electrons in silicon and
> > copper and slew rate caused by parasitic capacitance.
> 
> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
> 
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?
> 
> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> 
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
> tml
> 
> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > would match the hardware, but would break display for some modes,
> > depending on the display clock frequency as the internal 8.5ns output
> > delay applied to a falling clock edge would fall right into the 1.7ns
> > setup + hold time window of the ADV7123 around the rising edge. I can't
> > test this right now as I don't have local access to boards using the
> > ADV7123, but from a quick calculation that ignores the PCB transmission
> > delay modes with frequencies between 57MHz and 71MHz could break if the
> > data was output on the falling edge of the clock.
> 
> If clocks vs. data signal are really that much off on R-Car DU, then
> parallel displays must have the very same issue...
> 
> Are you sure that only the clock signal has an output delay? And that
> this output delay is a fixed value, clock independent?
> 
> Typically, delays apply to all signals equally, and often are clock
> frequency dependent...
> 
> Without really looking at the signals, I would not jump to conclusions
> here! I am pretty sure that driving on negative edge works just as well.

I've tested Linus' original patch and it broke display on R-Car, so, no, it 
doesn't work :-)

The R-Car display engine delays the clock internally (in some cases that delay 
is even configurable, and that's not uncommon in display controllers). We 
really need all this information, and I believe we need it for panels too, not 
just for bridges. The fact that we managed to get away without adding it to 
panels is likely due to the large number of panels out there, which makes it 
less likely that the same panel gets used by different systems in mainline 
with different clock delays. I expect that some panel drivers report the wrong 
clock edge to make things work on the board they were tested with, and I 
expect we'll eventually need to add the same information for panels too.

So please don't remove this useful API, otherwise you'll break my board, and I 
won't be happy.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> On 06.09.2018 04:07, Linus Walleij wrote:
> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> 
> >> Good point! I actually really don't like that we use the same flags here
> >> but from a different perspective. Especially since the flags defines
> >> document things differently:
> >> 
> >> /* drive data on pos. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> /* drive data on neg. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > 
> > Maybe a stupid comment from my side, but can't we just change the
> > documentation to match the usecases?
> > 
> > /* Trigger pixel data latch on positive edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> >> Using the opposite perspective would also need translation in crtc
> >> drivers... So far no driver uses sampling_edge.
> >> 
> >> I would prefer if we always use the meaning as documented by the flags.
> >> 
> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> 
> >> Linus Walleij, you added sampling edge, any thoughts?
> > 
> > I just thought it was generally useful to have triggering edge encoded
> > into the bridge as it makes it clear that this edge is something
> > that is a delayed version of the driving edge which is subject to
> > clock skew caused by the speed of electrons in silicon and
> > copper and slew rate caused by parasitic capacitance.
> 
> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
> 
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?
> 
> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> 
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
> tml
> 
> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > would match the hardware, but would break display for some modes,
> > depending on the display clock frequency as the internal 8.5ns output
> > delay applied to a falling clock edge would fall right into the 1.7ns
> > setup + hold time window of the ADV7123 around the rising edge. I can't
> > test this right now as I don't have local access to boards using the
> > ADV7123, but from a quick calculation that ignores the PCB transmission
> > delay modes with frequencies between 57MHz and 71MHz could break if the
> > data was output on the falling edge of the clock.
> 
> If clocks vs. data signal are really that much off on R-Car DU, then
> parallel displays must have the very same issue...
> 
> Are you sure that only the clock signal has an output delay? And that
> this output delay is a fixed value, clock independent?
> 
> Typically, delays apply to all signals equally, and often are clock
> frequency dependent...
> 
> Without really looking at the signals, I would not jump to conclusions
> here! I am pretty sure that driving on negative edge works just as well.

I've tested Linus' original patch and it broke display on R-Car, so, no, it 
doesn't work :-)

The R-Car display engine delays the clock internally (in some cases that delay 
is even configurable, and that's not uncommon in display controllers). We 
really need all this information, and I believe we need it for panels too, not 
just for bridges. The fact that we managed to get away without adding it to 
panels is likely due to the large number of panels out there, which makes it 
less likely that the same panel gets used by different systems in mainline 
with different clock delays. I expect that some panel drivers report the wrong 
clock edge to make things work on the board they were tested with, and I 
expect we'll eventually need to add the same information for panels too.

So please don't remove this useful API, otherwise you'll break my board, and I 
won't be happy.

-- 
Regards,

Laurent Pinchart



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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

On Friday, 14 September 2018 12:49:40 EEST Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> > On 06.09.2018 04:07, Linus Walleij wrote:
> > > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> > >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> > >> 
> > >> Good point! I actually really don't like that we use the same flags
> > >> here
> > >> but from a different perspective. Especially since the flags defines
> > >> document things differently:
> > >> 
> > >> /* drive data on pos. edge */
> > >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > >> /* drive data on neg. edge */
> > >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > > 
> > > Maybe a stupid comment from my side, but can't we just change the
> > > documentation to match the usecases?
> > > 
> > > /* Trigger pixel data latch on positive edge */
> > > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > > 
> > >> Using the opposite perspective would also need translation in crtc
> > >> drivers... So far no driver uses sampling_edge.
> > >> 
> > >> I would prefer if we always use the meaning as documented by the flags.
> > >> 
> > >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> > >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> > >> 
> > >> Linus Walleij, you added sampling edge, any thoughts?
> > > 
> > > I just thought it was generally useful to have triggering edge encoded
> > > into the bridge as it makes it clear that this edge is something
> > > that is a delayed version of the driving edge which is subject to
> > > clock skew caused by the speed of electrons in silicon and
> > > copper and slew rate caused by parasitic capacitance.
> > 
> > Ok, I read a bit up on the history of bridge timing, especially:
> > https://www.spinics.net/lists/dri-devel/msg155618.html
> > 
> > IMHO, this got overengineered. For displays we do not need all that
> > setup/sample delay timing information, and much longer cables are in
> > use. So why is all that needed for bridges?
> > 
> > For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> > clearly states:
> > Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> > 
> > So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > should be used, which makes the pl111 driver setting TIM2_IPC:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index
> > .h tml
> > 
> > > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > > doesn't match how the ADV7123 operates. Using
> > > DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > > would match the hardware, but would break display for some modes,
> > > depending on the display clock frequency as the internal 8.5ns output
> > > delay applied to a falling clock edge would fall right into the 1.7ns
> > > setup + hold time window of the ADV7123 around the rising edge. I can't
> > > test this right now as I don't have local access to boards using the
> > > ADV7123, but from a quick calculation that ignores the PCB transmission
> > > delay modes with frequencies between 57MHz and 71MHz could break if the
> > > data was output on the falling edge of the clock.
> > 
> > If clocks vs. data signal are really that much off on R-Car DU, then
> > parallel displays must have the very same issue...
> > 
> > Are you sure that only the clock signal has an output delay? And that
> > this output delay is a fixed value, clock independent?
> > 
> > Typically, delays apply to all signals equally, and often are clock
> > frequency dependent...
> > 
> > Without really looking at the signals, I would not jump to conclusions
> > here! I am pretty sure that driving on negative edge works just as well.
> 
> I've tested Linus' original patch and it broke display on R-Car, so, no, it
> doesn't work :-)
> 
> The R-Car display engine delays the clock internally (in some cases that
> delay is even configurable, and that's not uncommon in display
> controllers). We really need all this information, and I believe we need it
> for panels too, not just for bridges. The fact that we managed to get away
> without adding it to panels is likely due to the large number of panels out
> there, which makes it less likely that the same panel gets used by
> different systems in mainline with different clock delays. I expect that
> some panel drivers report the wrong clock edge to make things work on the
> board they were tested with, and I expect we'll eventually need to add the
> same information for panels too.
> 
> So please don't remove this useful API, otherwise you'll break my board, and
> I won't be happy.

Or, to be precise, the board won't break now, but as soon as I need to 
implement support for configuring the output clock edge, it will break as the 
ADV7123 driver won't give me the right information to make the right choice.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

On Friday, 14 September 2018 12:49:40 EEST Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> > On 06.09.2018 04:07, Linus Walleij wrote:
> > > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> > >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> > >> 
> > >> Good point! I actually really don't like that we use the same flags
> > >> here
> > >> but from a different perspective. Especially since the flags defines
> > >> document things differently:
> > >> 
> > >> /* drive data on pos. edge */
> > >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > >> /* drive data on neg. edge */
> > >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > > 
> > > Maybe a stupid comment from my side, but can't we just change the
> > > documentation to match the usecases?
> > > 
> > > /* Trigger pixel data latch on positive edge */
> > > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > > 
> > >> Using the opposite perspective would also need translation in crtc
> > >> drivers... So far no driver uses sampling_edge.
> > >> 
> > >> I would prefer if we always use the meaning as documented by the flags.
> > >> 
> > >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> > >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> > >> 
> > >> Linus Walleij, you added sampling edge, any thoughts?
> > > 
> > > I just thought it was generally useful to have triggering edge encoded
> > > into the bridge as it makes it clear that this edge is something
> > > that is a delayed version of the driving edge which is subject to
> > > clock skew caused by the speed of electrons in silicon and
> > > copper and slew rate caused by parasitic capacitance.
> > 
> > Ok, I read a bit up on the history of bridge timing, especially:
> > https://www.spinics.net/lists/dri-devel/msg155618.html
> > 
> > IMHO, this got overengineered. For displays we do not need all that
> > setup/sample delay timing information, and much longer cables are in
> > use. So why is all that needed for bridges?
> > 
> > For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> > clearly states:
> > Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> > 
> > So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > should be used, which makes the pl111 driver setting TIM2_IPC:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index
> > .h tml
> > 
> > > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > > doesn't match how the ADV7123 operates. Using
> > > DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > > would match the hardware, but would break display for some modes,
> > > depending on the display clock frequency as the internal 8.5ns output
> > > delay applied to a falling clock edge would fall right into the 1.7ns
> > > setup + hold time window of the ADV7123 around the rising edge. I can't
> > > test this right now as I don't have local access to boards using the
> > > ADV7123, but from a quick calculation that ignores the PCB transmission
> > > delay modes with frequencies between 57MHz and 71MHz could break if the
> > > data was output on the falling edge of the clock.
> > 
> > If clocks vs. data signal are really that much off on R-Car DU, then
> > parallel displays must have the very same issue...
> > 
> > Are you sure that only the clock signal has an output delay? And that
> > this output delay is a fixed value, clock independent?
> > 
> > Typically, delays apply to all signals equally, and often are clock
> > frequency dependent...
> > 
> > Without really looking at the signals, I would not jump to conclusions
> > here! I am pretty sure that driving on negative edge works just as well.
> 
> I've tested Linus' original patch and it broke display on R-Car, so, no, it
> doesn't work :-)
> 
> The R-Car display engine delays the clock internally (in some cases that
> delay is even configurable, and that's not uncommon in display
> controllers). We really need all this information, and I believe we need it
> for panels too, not just for bridges. The fact that we managed to get away
> without adding it to panels is likely due to the large number of panels out
> there, which makes it less likely that the same panel gets used by
> different systems in mainline with different clock delays. I expect that
> some panel drivers report the wrong clock edge to make things work on the
> board they were tested with, and I expect we'll eventually need to add the
> same information for panels too.
> 
> So please don't remove this useful API, otherwise you'll break my board, and
> I won't be happy.

Or, to be precise, the board won't break now, but as soon as I need to 
implement support for configuring the output clock edge, it will break as the 
ADV7123 driver won't give me the right information to make the right choice.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

On Friday, 14 September 2018 12:49:40 EEST Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> > On 06.09.2018 04:07, Linus Walleij wrote:
> > > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
> > >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> > >> 
> > >> Good point! I actually really don't like that we use the same flags
> > >> here
> > >> but from a different perspective. Especially since the flags defines
> > >> document things differently:
> > >> 
> > >> /* drive data on pos. edge */
> > >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > >> /* drive data on neg. edge */
> > >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > > 
> > > Maybe a stupid comment from my side, but can't we just change the
> > > documentation to match the usecases?
> > > 
> > > /* Trigger pixel data latch on positive edge */
> > > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > > 
> > >> Using the opposite perspective would also need translation in crtc
> > >> drivers... So far no driver uses sampling_edge.
> > >> 
> > >> I would prefer if we always use the meaning as documented by the flags.
> > >> 
> > >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> > >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> > >> 
> > >> Linus Walleij, you added sampling edge, any thoughts?
> > > 
> > > I just thought it was generally useful to have triggering edge encoded
> > > into the bridge as it makes it clear that this edge is something
> > > that is a delayed version of the driving edge which is subject to
> > > clock skew caused by the speed of electrons in silicon and
> > > copper and slew rate caused by parasitic capacitance.
> > 
> > Ok, I read a bit up on the history of bridge timing, especially:
> > https://www.spinics.net/lists/dri-devel/msg155618.html
> > 
> > IMHO, this got overengineered. For displays we do not need all that
> > setup/sample delay timing information, and much longer cables are in
> > use. So why is all that needed for bridges?
> > 
> > For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> > clearly states:
> > Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> > 
> > So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > should be used, which makes the pl111 driver setting TIM2_IPC:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index
> > .h tml
> > 
> > > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > > doesn't match how the ADV7123 operates. Using
> > > DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > > would match the hardware, but would break display for some modes,
> > > depending on the display clock frequency as the internal 8.5ns output
> > > delay applied to a falling clock edge would fall right into the 1.7ns
> > > setup + hold time window of the ADV7123 around the rising edge. I can't
> > > test this right now as I don't have local access to boards using the
> > > ADV7123, but from a quick calculation that ignores the PCB transmission
> > > delay modes with frequencies between 57MHz and 71MHz could break if the
> > > data was output on the falling edge of the clock.
> > 
> > If clocks vs. data signal are really that much off on R-Car DU, then
> > parallel displays must have the very same issue...
> > 
> > Are you sure that only the clock signal has an output delay? And that
> > this output delay is a fixed value, clock independent?
> > 
> > Typically, delays apply to all signals equally, and often are clock
> > frequency dependent...
> > 
> > Without really looking at the signals, I would not jump to conclusions
> > here! I am pretty sure that driving on negative edge works just as well.
> 
> I've tested Linus' original patch and it broke display on R-Car, so, no, it
> doesn't work :-)
> 
> The R-Car display engine delays the clock internally (in some cases that
> delay is even configurable, and that's not uncommon in display
> controllers). We really need all this information, and I believe we need it
> for panels too, not just for bridges. The fact that we managed to get away
> without adding it to panels is likely due to the large number of panels out
> there, which makes it less likely that the same panel gets used by
> different systems in mainline with different clock delays. I expect that
> some panel drivers report the wrong clock edge to make things work on the
> board they were tested with, and I expect we'll eventually need to add the
> same information for panels too.
> 
> So please don't remove this useful API, otherwise you'll break my board, and
> I won't be happy.

Or, to be precise, the board won't break now, but as soon as I need to 
implement support for configuring the output clock edge, it will break as the 
ADV7123 driver won't give me the right information to make the right choice.

-- 
Regards,

Laurent Pinchart



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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 14.09.2018 02:49, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
>> On 06.09.2018 04:07, Linus Walleij wrote:
>> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >>
>> >> Good point! I actually really don't like that we use the same flags here
>> >> but from a different perspective. Especially since the flags defines
>> >> document things differently:
>> >>
>> >> /* drive data on pos. edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >> /* drive data on neg. edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>> >
>> > Maybe a stupid comment from my side, but can't we just change the
>> > documentation to match the usecases?
>> >
>> > /* Trigger pixel data latch on positive edge */
>> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >
>> >> Using the opposite perspective would also need translation in crtc
>> >> drivers... So far no driver uses sampling_edge.
>> >>
>> >> I would prefer if we always use the meaning as documented by the flags.
>> >>
>> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >>
>> >> Linus Walleij, you added sampling edge, any thoughts?
>> >
>> > I just thought it was generally useful to have triggering edge encoded
>> > into the bridge as it makes it clear that this edge is something
>> > that is a delayed version of the driving edge which is subject to
>> > clock skew caused by the speed of electrons in silicon and
>> > copper and slew rate caused by parasitic capacitance.
>>
>> Ok, I read a bit up on the history of bridge timing, especially:
>> https://www.spinics.net/lists/dri-devel/msg155618.html
>>
>> IMHO, this got overengineered. For displays we do not need all that
>> setup/sample delay timing information, and much longer cables are in
>> use. So why is all that needed for bridges?
>>
>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
>> tml
>>
>> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
>> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> > would match the hardware, but would break display for some modes,
>> > depending on the display clock frequency as the internal 8.5ns output
>> > delay applied to a falling clock edge would fall right into the 1.7ns
>> > setup + hold time window of the ADV7123 around the rising edge. I can't
>> > test this right now as I don't have local access to boards using the
>> > ADV7123, but from a quick calculation that ignores the PCB transmission
>> > delay modes with frequencies between 57MHz and 71MHz could break if the
>> > data was output on the falling edge of the clock.
>>
>> If clocks vs. data signal are really that much off on R-Car DU, then
>> parallel displays must have the very same issue...
>>
>> Are you sure that only the clock signal has an output delay? And that
>> this output delay is a fixed value, clock independent?
>>
>> Typically, delays apply to all signals equally, and often are clock
>> frequency dependent...
>>
>> Without really looking at the signals, I would not jump to conclusions
>> here! I am pretty sure that driving on negative edge works just as well.
> 
> I've tested Linus' original patch and it broke display on R-Car, so, no, it 
> doesn't work :-)
> 

How can that be, R-Car does not use struct bridge timings or DRM_BUS_FLAG_PIXDATA_*EDGE bus_flags?

Which version exactly did you test? (in v4 you claimed that you did not have access to hardware at that point..)

> The R-Car display engine delays the clock internally (in some cases that delay 
> is even configurable, and that's not uncommon in display controllers). We 
> really need all this information, and I believe we need it for panels too, not 
> just for bridges. The fact that we managed to get away without adding it to 
> panels is likely due to the large number of panels out there, which makes it 
> less likely that the same panel gets used by different systems in mainline 
> with different clock delays. I expect that some panel drivers report the wrong 
> clock edge to make things work on the board they were tested with, and I 
> expect we'll eventually need to add the same information for panels too.

I used a bunch of parallel displays which are upstream (EDT, KEO, TPK) all of them on several different SoCs with mainline drivers, namely mxsfb, drm-mxsfb, FSL DCU, Tegra (RGB). I had several cases where data have been driven on the wrong edge, hooking up oscilloscope
and fix the driver helped in all cases...

As long as there is no case at hand I am not convinced that this is the case.

> 
> So please don't remove this useful API, otherwise you'll break my board, and I 
> won't be happy.

How does it break your board? Again, R-Car does not use struct bridge timings or DRM_BUS_FLAG_PIXDATA_*EDGE bus_flags.

>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

Linus confirmed me in person that simply driving on the correct edge works for him.

Btw the code which is currently in used in pl111 is just a heuristic:

		if (btimings && btimings->setup_time_ps >= 3000)
			tim2 ^= TIM2_IPC;

The code will not invert the pixel clock for THS8135 (since setup time is 2000).

But the THS8135 clearly samples on positive edge!

The above code also does not take the clock into account.

This is much more likely to work for other bridges too:

		if (btimings && btimings->input_bus_flags &
		    DRM_BUS_FLAG_PIXDATA_NEGEDGE)
			tim2 |= TIM2_IPC;


--
Stefan

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 14.09.2018 02:49, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
>> On 06.09.2018 04:07, Linus Walleij wrote:
>> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >>
>> >> Good point! I actually really don't like that we use the same flags here
>> >> but from a different perspective. Especially since the flags defines
>> >> document things differently:
>> >>
>> >> /* drive data on pos. edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >> /* drive data on neg. edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>> >
>> > Maybe a stupid comment from my side, but can't we just change the
>> > documentation to match the usecases?
>> >
>> > /* Trigger pixel data latch on positive edge */
>> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >
>> >> Using the opposite perspective would also need translation in crtc
>> >> drivers... So far no driver uses sampling_edge.
>> >>
>> >> I would prefer if we always use the meaning as documented by the flags.
>> >>
>> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >>
>> >> Linus Walleij, you added sampling edge, any thoughts?
>> >
>> > I just thought it was generally useful to have triggering edge encoded
>> > into the bridge as it makes it clear that this edge is something
>> > that is a delayed version of the driving edge which is subject to
>> > clock skew caused by the speed of electrons in silicon and
>> > copper and slew rate caused by parasitic capacitance.
>>
>> Ok, I read a bit up on the history of bridge timing, especially:
>> https://www.spinics.net/lists/dri-devel/msg155618.html
>>
>> IMHO, this got overengineered. For displays we do not need all that
>> setup/sample delay timing information, and much longer cables are in
>> use. So why is all that needed for bridges?
>>
>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
>> tml
>>
>> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
>> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> > would match the hardware, but would break display for some modes,
>> > depending on the display clock frequency as the internal 8.5ns output
>> > delay applied to a falling clock edge would fall right into the 1.7ns
>> > setup + hold time window of the ADV7123 around the rising edge. I can't
>> > test this right now as I don't have local access to boards using the
>> > ADV7123, but from a quick calculation that ignores the PCB transmission
>> > delay modes with frequencies between 57MHz and 71MHz could break if the
>> > data was output on the falling edge of the clock.
>>
>> If clocks vs. data signal are really that much off on R-Car DU, then
>> parallel displays must have the very same issue...
>>
>> Are you sure that only the clock signal has an output delay? And that
>> this output delay is a fixed value, clock independent?
>>
>> Typically, delays apply to all signals equally, and often are clock
>> frequency dependent...
>>
>> Without really looking at the signals, I would not jump to conclusions
>> here! I am pretty sure that driving on negative edge works just as well.
> 
> I've tested Linus' original patch and it broke display on R-Car, so, no, it 
> doesn't work :-)
> 

How can that be, R-Car does not use struct bridge timings or DRM_BUS_FLAG_PIXDATA_*EDGE bus_flags?

Which version exactly did you test? (in v4 you claimed that you did not have access to hardware at that point..)

> The R-Car display engine delays the clock internally (in some cases that delay 
> is even configurable, and that's not uncommon in display controllers). We 
> really need all this information, and I believe we need it for panels too, not 
> just for bridges. The fact that we managed to get away without adding it to 
> panels is likely due to the large number of panels out there, which makes it 
> less likely that the same panel gets used by different systems in mainline 
> with different clock delays. I expect that some panel drivers report the wrong 
> clock edge to make things work on the board they were tested with, and I 
> expect we'll eventually need to add the same information for panels too.

I used a bunch of parallel displays which are upstream (EDT, KEO, TPK) all of them on several different SoCs with mainline drivers, namely mxsfb, drm-mxsfb, FSL DCU, Tegra (RGB). I had several cases where data have been driven on the wrong edge, hooking up oscilloscope
and fix the driver helped in all cases...

As long as there is no case at hand I am not convinced that this is the case.

> 
> So please don't remove this useful API, otherwise you'll break my board, and I 
> won't be happy.

How does it break your board? Again, R-Car does not use struct bridge timings or DRM_BUS_FLAG_PIXDATA_*EDGE bus_flags.

>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

Linus confirmed me in person that simply driving on the correct edge works for him.

Btw the code which is currently in used in pl111 is just a heuristic:

		if (btimings && btimings->setup_time_ps >= 3000)
			tim2 ^= TIM2_IPC;

The code will not invert the pixel clock for THS8135 (since setup time is 2000).

But the THS8135 clearly samples on positive edge!

The above code also does not take the clock into account.

This is much more likely to work for other bridges too:

		if (btimings && btimings->input_bus_flags &
		    DRM_BUS_FLAG_PIXDATA_NEGEDGE)
			tim2 |= TIM2_IPC;


--
Stefan

Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

On 14.09.2018 02:49, Laurent Pinchart wrote:
> Hi Stefan,
> 
> On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
>> On 06.09.2018 04:07, Linus Walleij wrote:
>> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@agner.ch> wrote:
>> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
>> >>
>> >> Good point! I actually really don't like that we use the same flags here
>> >> but from a different perspective. Especially since the flags defines
>> >> document things differently:
>> >>
>> >> /* drive data on pos. edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >> /* drive data on neg. edge */
>> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
>> >
>> > Maybe a stupid comment from my side, but can't we just change the
>> > documentation to match the usecases?
>> >
>> > /* Trigger pixel data latch on positive edge */
>> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
>> >
>> >> Using the opposite perspective would also need translation in crtc
>> >> drivers... So far no driver uses sampling_edge.
>> >>
>> >> I would prefer if we always use the meaning as documented by the flags.
>> >>
>> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
>> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
>> >>
>> >> Linus Walleij, you added sampling edge, any thoughts?
>> >
>> > I just thought it was generally useful to have triggering edge encoded
>> > into the bridge as it makes it clear that this edge is something
>> > that is a delayed version of the driving edge which is subject to
>> > clock skew caused by the speed of electrons in silicon and
>> > copper and slew rate caused by parasitic capacitance.
>>
>> Ok, I read a bit up on the history of bridge timing, especially:
>> https://www.spinics.net/lists/dri-devel/msg155618.html
>>
>> IMHO, this got overengineered. For displays we do not need all that
>> setup/sample delay timing information, and much longer cables are in
>> use. So why is all that needed for bridges?
>>
>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
>> tml
>>
>> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
>> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> > would match the hardware, but would break display for some modes,
>> > depending on the display clock frequency as the internal 8.5ns output
>> > delay applied to a falling clock edge would fall right into the 1.7ns
>> > setup + hold time window of the ADV7123 around the rising edge. I can't
>> > test this right now as I don't have local access to boards using the
>> > ADV7123, but from a quick calculation that ignores the PCB transmission
>> > delay modes with frequencies between 57MHz and 71MHz could break if the
>> > data was output on the falling edge of the clock.
>>
>> If clocks vs. data signal are really that much off on R-Car DU, then
>> parallel displays must have the very same issue...
>>
>> Are you sure that only the clock signal has an output delay? And that
>> this output delay is a fixed value, clock independent?
>>
>> Typically, delays apply to all signals equally, and often are clock
>> frequency dependent...
>>
>> Without really looking at the signals, I would not jump to conclusions
>> here! I am pretty sure that driving on negative edge works just as well.
> 
> I've tested Linus' original patch and it broke display on R-Car, so, no, it 
> doesn't work :-)
> 

How can that be, R-Car does not use struct bridge timings or DRM_BUS_FLAG_PIXDATA_*EDGE bus_flags?

Which version exactly did you test? (in v4 you claimed that you did not have access to hardware at that point..)

> The R-Car display engine delays the clock internally (in some cases that delay 
> is even configurable, and that's not uncommon in display controllers). We 
> really need all this information, and I believe we need it for panels too, not 
> just for bridges. The fact that we managed to get away without adding it to 
> panels is likely due to the large number of panels out there, which makes it 
> less likely that the same panel gets used by different systems in mainline 
> with different clock delays. I expect that some panel drivers report the wrong 
> clock edge to make things work on the board they were tested with, and I 
> expect we'll eventually need to add the same information for panels too.

I used a bunch of parallel displays which are upstream (EDT, KEO, TPK) all of them on several different SoCs with mainline drivers, namely mxsfb, drm-mxsfb, FSL DCU, Tegra (RGB). I had several cases where data have been driven on the wrong edge, hooking up oscilloscope
and fix the driver helped in all cases...

As long as there is no case at hand I am not convinced that this is the case.

> 
> So please don't remove this useful API, otherwise you'll break my board, and I 
> won't be happy.

How does it break your board? Again, R-Car does not use struct bridge timings or DRM_BUS_FLAG_PIXDATA_*EDGE bus_flags.

>> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
>> clearly states:
>> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
>>
>> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
>> should be used, which makes the pl111 driver setting TIM2_IPC:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.html

Linus confirmed me in person that simply driving on the correct edge works for him.

Btw the code which is currently in used in pl111 is just a heuristic:

		if (btimings && btimings->setup_time_ps >= 3000)
			tim2 ^= TIM2_IPC;

The code will not invert the pixel clock for THS8135 (since setup time is 2000).

But the THS8135 clearly samples on positive edge!

The above code also does not take the clock into account.

This is much more likely to work for other bridges too:

		if (btimings && btimings->input_bus_flags &
		    DRM_BUS_FLAG_PIXDATA_NEGEDGE)
			tim2 |= TIM2_IPC;


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

[PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Stefan Agner]

The DRM bus flags convey additional information on pixel data on
the bus. All current available bus flags might be of interest for
a bridge. Remove the sampling_edge field and use bus_flags.

In the case at hand a dumb VGA bridge needs a specific data enable
polarity (DRM_BUS_FLAG_DE_LOW).

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
 include/drm/drm_bridge.h              | 11 +++++------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9b706789a341..7a5c24967115 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
  */
 static const struct drm_bridge_timings default_dac_timings = {
 	/* Timing specifications, datasheet page 7 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	.setup_time_ps = 500,
 	.hold_time_ps = 1500,
 };
@@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8134_dac_timings = {
 	/* From timing diagram, datasheet page 9 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 12 */
 	.setup_time_ps = 3000,
 	/* I guess this means latched input */
@@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8135_dac_timings = {
 	/* From timing diagram, datasheet page 14 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
 	/* From datasheet, page 16 */
 	.setup_time_ps = 2000,
 	.hold_time_ps = 500,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd850747ce54..85d4b51eae19 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -244,14 +244,13 @@ struct drm_bridge_funcs {
  */
 struct drm_bridge_timings {
 	/**
-	 * @sampling_edge:
+	 * @bus_flags:
 	 *
-	 * Tells whether the bridge samples the digital input signal
-	 * from the display engine on the positive or negative edge of the
-	 * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE
-	 * bitwise flags from the DRM connector (bit 2 and 3 valid).
+	 * Tells what additional settings for the pixel data on the bus
+	 * this bridge requires (like pixel signal polarity). See also
+	 * &drm_display_info->bus_flags.
 	 */
-	u32 sampling_edge;
+	u32 bus_flags;
 	/**
 	 * @setup_time_ps:
 	 *
-- 
2.18.0

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

[PATCH 3/6] dt-bindings: display: add data-enable polarity property

[Stefan Agner]

Allow to specify the data-enable polarity required by a dumb VGA
DAC converting parallel RGB to VGA.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 .../devicetree/bindings/display/bridge/dumb-vga-dac.txt          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
index 164cbb15f04c..adbd2ca0af2f 100644
--- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
@@ -18,6 +18,7 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
 Optional properties:
 - vdd-supply: Power supply for DAC
+- de-active: data-enable pulse is active low/high/ignored
 
 Example
 -------
-- 
2.18.0

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