[PATCH 0/2] Introduce h/vsync-disable properties for ti-sn65dsi83

[PATCH 0/2] Introduce h/vsync-disable properties for ti-sn65dsi83

[A. Zini]

From: Alessandro Zini <alessandro.zini@siemens.com>

This patch series adds support for disabling the generation of h/vsync signals
on the ti-sn65dsi83 bridge.

This is required on some panels which are driven in DE-only mode but do not
ignore sync packets, and instead require them to be low-voltage level or ground.

A discussion (1) on TI's E2E forum confirms that this may be required for some
panels.

(1) https://e2e.ti.com/support/interface-group/interface/f/interface-forum/1475734/sn65dsi84-disable-hsync-and-vsync

Alessandro Zini (2):
  dt-bindings: drm/bridge: ti-sn65dsi83: add h/vsync-disable bindings
  drm/bridge: ti-sn65dsi83: add h/vsync-disable support

 .../bindings/display/bridge/ti,sn65dsi83.yaml    | 12 ++++++++++++
 drivers/gpu/drm/bridge/ti-sn65dsi83.c            | 16 ++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.48.1

[PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: add h/vsync-disable bindings

[A. Zini]

From: Alessandro Zini <alessandro.zini@siemens.com>

Add hsync- and vsync-disable bindings, used to disable the generation of
h/vsync signals.

Signed-off-by: Alessandro Zini <alessandro.zini@siemens.com>
---
 .../bindings/display/bridge/ti,sn65dsi83.yaml        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index 9b5f3f3eab198..ff80876d504ad 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -94,6 +94,18 @@ properties:
       - port@0
       - port@2
 
+  hsync-disable:
+    type: boolean
+    description: |
+      Disable HSYNC generation on the LVDS output by setting the
+      width in pixel clocks of the hsync pulse width to 0.
+
+  vsync-disable:
+    type: boolean
+    description: |
+      Disable VSYNC generation on the LVDS output by setting the
+      length in lines of the vsync pulse width to 0.
+
 required:
   - compatible
   - reg
-- 
2.48.1

Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: add h/vsync-disable bindings

[Dmitry Baryshkov]

On Thu, Mar 06, 2025 at 10:11:32AM +0100, A. Zini wrote:
> From: Alessandro Zini <alessandro.zini@siemens.com>
> 
> Add hsync- and vsync-disable bindings, used to disable the generation of
> h/vsync signals.

Please describe, why this is necessary at all, instead of desribing the
contents of the patch.

> 
> Signed-off-by: Alessandro Zini <alessandro.zini@siemens.com>
> ---
>  .../bindings/display/bridge/ti,sn65dsi83.yaml        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> index 9b5f3f3eab198..ff80876d504ad 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> @@ -94,6 +94,18 @@ properties:
>        - port@0
>        - port@2
>  
> +  hsync-disable:
> +    type: boolean
> +    description: |
> +      Disable HSYNC generation on the LVDS output by setting the
> +      width in pixel clocks of the hsync pulse width to 0.
> +
> +  vsync-disable:
> +    type: boolean
> +    description: |
> +      Disable VSYNC generation on the LVDS output by setting the
> +      length in lines of the vsync pulse width to 0.
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.48.1
> 

-- 
With best wishes
Dmitry

[PATCH 2/2] drm/bridge: ti-sn65dsi83: add h/vsync-disable support

[A. Zini]

From: Alessandro Zini <alessandro.zini@siemens.com>

The h/vsync-disable properties are used to control whether to use or
not h/vsync signals, by configuring their pulse width to zero.

This is required on some panels which are driven in DE-only mode but do
not ignore sync packets, and instead require them to be low-voltage level
or ground.

Signed-off-by: Alessandro Zini <alessandro.zini@siemens.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 95563aa1b450d..c94ea92159402 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -164,6 +164,8 @@ struct sn65dsi83 {
 	int				irq;
 	struct delayed_work		monitor_work;
 	struct work_struct		reset_work;
+	bool				hsync_disable;
+	bool				vsync_disable;
 };
 
 static const struct regmap_range sn65dsi83_readable_ranges[] = {
@@ -604,10 +606,12 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	/* 32 + 1 pixel clock to ensure proper operation */
 	le16val = cpu_to_le16(32 + 1);
 	regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, &le16val, 2);
-	le16val = cpu_to_le16(mode->hsync_end - mode->hsync_start);
+	le16val = cpu_to_le16(ctx->hsync_disable ?
+		0 : mode->hsync_end - mode->hsync_start);
 	regmap_bulk_write(ctx->regmap, REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
 			  &le16val, 2);
-	le16val = cpu_to_le16(mode->vsync_end - mode->vsync_start);
+	le16val = cpu_to_le16(ctx->vsync_disable ?
+		0 : mode->vsync_end - mode->vsync_start);
 	regmap_bulk_write(ctx->regmap, REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
 			  &le16val, 2);
 	regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_BACK_PORCH,
@@ -867,6 +871,14 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 		}
 	}
 
+	ctx->hsync_disable = false;
+	if (of_property_present(dev->of_node, "hsync-disable"))
+		ctx->hsync_disable = true;
+
+	ctx->vsync_disable = false;
+	if (of_property_present(dev->of_node, "vsync-disable"))
+		ctx->vsync_disable = true;
+
 	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0);
 	if (IS_ERR(panel_bridge))
 		return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n");
-- 
2.48.1

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: add h/vsync-disable support

[Dmitry Baryshkov]

On Thu, Mar 06, 2025 at 10:11:33AM +0100, A. Zini wrote:
> From: Alessandro Zini <alessandro.zini@siemens.com>
> 
> The h/vsync-disable properties are used to control whether to use or
> not h/vsync signals, by configuring their pulse width to zero.
> 
> This is required on some panels which are driven in DE-only mode but do
> not ignore sync packets, and instead require them to be low-voltage level
> or ground.

If this is required by 'some panels', then it should be a property of
the panel, not by the bridge itself. Can the panel return the mode with
hsync_end = hsync_start and vsync_enc = vsync_start?

> 
> Signed-off-by: Alessandro Zini <alessandro.zini@siemens.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 95563aa1b450d..c94ea92159402 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -164,6 +164,8 @@ struct sn65dsi83 {
>  	int				irq;
>  	struct delayed_work		monitor_work;
>  	struct work_struct		reset_work;
> +	bool				hsync_disable;
> +	bool				vsync_disable;
>  };
>  
>  static const struct regmap_range sn65dsi83_readable_ranges[] = {
> @@ -604,10 +606,12 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  	/* 32 + 1 pixel clock to ensure proper operation */
>  	le16val = cpu_to_le16(32 + 1);
>  	regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, &le16val, 2);
> -	le16val = cpu_to_le16(mode->hsync_end - mode->hsync_start);
> +	le16val = cpu_to_le16(ctx->hsync_disable ?
> +		0 : mode->hsync_end - mode->hsync_start);
>  	regmap_bulk_write(ctx->regmap, REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
>  			  &le16val, 2);
> -	le16val = cpu_to_le16(mode->vsync_end - mode->vsync_start);
> +	le16val = cpu_to_le16(ctx->vsync_disable ?
> +		0 : mode->vsync_end - mode->vsync_start);
>  	regmap_bulk_write(ctx->regmap, REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
>  			  &le16val, 2);
>  	regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> @@ -867,6 +871,14 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>  		}
>  	}
>  
> +	ctx->hsync_disable = false;
> +	if (of_property_present(dev->of_node, "hsync-disable"))
> +		ctx->hsync_disable = true;
> +
> +	ctx->vsync_disable = false;
> +	if (of_property_present(dev->of_node, "vsync-disable"))
> +		ctx->vsync_disable = true;
> +
>  	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0);
>  	if (IS_ERR(panel_bridge))
>  		return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n");
> -- 
> 2.48.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: add h/vsync-disable support

[Zini, Alessandro]

On Fri, 2025-03-07 at 08:05 +0200, Dmitry Baryshkov wrote:
> On Thu, Mar 06, 2025 at 10:11:33AM +0100, A. Zini wrote:
> > From: Alessandro Zini <alessandro.zini@siemens.com>
> > 
> > The h/vsync-disable properties are used to control whether to use or
> > not h/vsync signals, by configuring their pulse width to zero.
> > 
> > This is required on some panels which are driven in DE-only mode but do
> > not ignore sync packets, and instead require them to be low-voltage level
> > or ground.
> 
> If this is required by 'some panels', then it should be a property of
> the panel, not by the bridge itself.

I got the same, rightful objection also from Rob. I'll answer here to
the both of you with the reasoning behind the submission of this patch.
Actually, I waited for a while before sending this patch, because I
originally had the same opinion. I do still have some difficulties
drawing the line between "this is a panel property" and "this is a
configurable feature of the bridge".

However, I have also prepared a second patch which adds support for
configuring the LVDS near-end termination. Afterward, I found that this
feature has already made its way in recently, so I dropped the patch.
Arguably still, that feature could be seen in the same way as the one
added from this patch, since a panel might require 100 Ohm, while
another 200 Ohm. Likewise, a panel might require h/vsync signals, while
another might require them to be zero/absent.

The TI E2E discussion I have attached to the cover letter of this patch
series eventually made me change my mind. From my point of view, the
discussion implies that avoiding the generation of h/vsync signals is
indeed a (configurable) feature of the bridge, even though not
explicitly documented in its datasheet.

Given the two reasons above, I think this patch would better fit in the
bridge rather than in the panel (which, for context, is driven as a
simple-panel).

> Can the panel return the mode with hsync_end = hsync_start and
> vsync_enc = vsync_start?

I did try to set <h/vsync-len> to zero, which resulted in vsync_end =
vsync_start and hsync_end = hsync_start, while also adjusting the other
blanking properties. I am not sure if this is what you meant.
However, this resulted in an issue along the pipeline, and ultimately
caused drm_atomic_helper_wait_for_vblanks() to timeout.

-- 
Alessandro

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: add h/vsync-disable support

[Sverdlin, Alexander]

Hi Rob, Dmitry,

On Tue, 2025-03-11 at 15:27 +0000, Zini, Alessandro wrote:
> > > The h/vsync-disable properties are used to control whether to use or
> > > not h/vsync signals, by configuring their pulse width to zero.
> > > 
> > > This is required on some panels which are driven in DE-only mode but do
> > > not ignore sync packets, and instead require them to be low-voltage level
> > > or ground.
> > 
> > If this is required by 'some panels', then it should be a property of
> > the panel, not by the bridge itself.
> 
> I got the same, rightful objection also from Rob. I'll answer here to
> the both of you with the reasoning behind the submission of this patch.
> Actually, I waited for a while before sending this patch, because I
> originally had the same opinion. I do still have some difficulties
> drawing the line between "this is a panel property" and "this is a
> configurable feature of the bridge".
> 
> However, I have also prepared a second patch which adds support for
> configuring the LVDS near-end termination. Afterward, I found that this
> feature has already made its way in recently, so I dropped the patch.
> Arguably still, that feature could be seen in the same way as the one
> added from this patch, since a panel might require 100 Ohm, while
> another 200 Ohm. Likewise, a panel might require h/vsync signals, while
> another might require them to be zero/absent.
> 
> The TI E2E discussion I have attached to the cover letter of this patch
> series eventually made me change my mind. From my point of view, the
> discussion implies that avoiding the generation of h/vsync signals is
> indeed a (configurable) feature of the bridge, even though not
> explicitly documented in its datasheet.
> 
> Given the two reasons above, I think this patch would better fit in the
> bridge rather than in the panel (which, for context, is driven as a
> simple-panel).
> 
> > Can the panel return the mode with hsync_end = hsync_start and
> > vsync_enc = vsync_start?
> 
> I did try to set <h/vsync-len> to zero, which resulted in vsync_end =
> vsync_start and hsync_end = hsync_start, while also adjusting the other
> blanking properties. I am not sure if this is what you meant.
> However, this resulted in an issue along the pipeline, and ultimately
> caused drm_atomic_helper_wait_for_vblanks() to timeout.

the problem here is that actually we cannot model this pipeline with
the current Linux state of affairs: the bridge requires hsync/vsync signals
and the panel only works if they are always "0". With the only
solution we've found to set their length to "0" in the bridge itself.

So it's a quirk, not a proper configuration. A proper configuration would
lead to vsync/hsync missing in the whole pipeline and the bridge cannot
sync.

As I understand, currently DRM subsystem only supports different polarities
of the vsync/hsync signals along the pipeline, but not the entirely
different configuration (i.e. present/missing sync pulses for different
parts in the pipeline).

As I undestand, the bridges do not look into the panel properties directly,
but rather there is a common "mode" negotiated, but what to do if we have
different requirements along the pipeline?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH 0/2] Introduce h/vsync-disable properties for ti-sn65dsi83

[Rob Herring]

On Thu, Mar 06, 2025 at 10:11:31AM +0100, A. Zini wrote:
> From: Alessandro Zini <alessandro.zini@siemens.com>
> 
> This patch series adds support for disabling the generation of h/vsync signals
> on the ti-sn65dsi83 bridge.
> 
> This is required on some panels which are driven in DE-only mode but do not
> ignore sync packets, and instead require them to be low-voltage level or ground.
> 
> A discussion (1) on TI's E2E forum confirms that this may be required for some
> panels.

If this is a property of the panel then it should be in the panel's 
endpoint or implicit with implicit being preferred. It should not be 
defined in some bridge binding.

Rob