[PATCH v5 0/4] drm/bridge: ti-sn65dsi86: Basic DP support

[PATCH v5 0/4] drm/bridge: ti-sn65dsi86: Basic DP support

[Tomi Valkeinen]

Hi,

This is possibly v5 of the series. I couldn't find v4, but the change
logs suggested changes to v3, so I presumed there's v4. However, it
might be v4 was never sent.

In any case, this series introduces basic DP support for sn65dsi86. So
far the driver has only supported eDP.

With basic I mean that there's no support for real HPD, although we do
implement detect callback. Link training is only done at
atomic-enable-time, which is not really correct for DP.

However, this series does give us a working display on a DP monitor. Due
to physical restrictions I'm not able to actually test plug/unplug,
changing monitors, and things like that, so the only "officially"
supported scenario is a DP monitor that's always plugged in, although I
think changing monitors should work.

 Tomi

Laurent Pinchart (2):
  drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
  drm/bridge: ti-sn65dsi86: Implement bridge connector operations

Tomi Valkeinen (2):
  drm/bridge: ti-sn65dsi86: check AUX errors better
  drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 77 +++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
sending AUX transfers, which leads to the driver not returning an error.

Add the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 90bbabde1595..ba84215c1511 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -582,6 +582,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		goto exit;
 	}
 
+	if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
+		ret = -EIO;
+		goto exit;
+	}
+
 	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
 		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
 		if (ret)
-- 
2.34.1

[PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

The blanking related registers are 8 bits, so reject any modes
with larger blanking periods.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ba84215c1511..f085a037ff5b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
 	if (mode->clock > 594000)
 		return MODE_CLOCK_HIGH;
 
+	/*
+	 * The blanking related registers are 8 bits, so reject any modes
+	 * with larger blanking periods.
+	 */
+
+	if ((mode->hsync_start - mode->hdisplay) > 0xff)
+		return MODE_HBLANK_WIDE;
+
+	if ((mode->vsync_start - mode->vdisplay) > 0xff)
+		return MODE_VBLANK_WIDE;
+
+	if ((mode->hsync_end - mode->hsync_start) > 0xff)
+		return MODE_HSYNC_WIDE;
+
+	if ((mode->vsync_end - mode->vsync_start) > 0xff)
+		return MODE_VSYNC_WIDE;
+
+	if ((mode->htotal - mode->hsync_end) > 0xff)
+		return MODE_HBLANK_WIDE;
+
+	if ((mode->vtotal - mode->vsync_end) > 0xff)
+		return MODE_VBLANK_WIDE;
+
 	return MODE_OK;
 }
 
-- 
2.34.1

[PATCH v5 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Tomi Valkeinen]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Despite the SN65DSI86 being an eDP bridge, on some systems its output is
routed to a DisplayPort connector. Enable DisplayPort mode when the next
component in the display pipeline is detected as a DisplayPort
connector, and disable eDP features in that case.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reworked to set bridge type based on the next bridge/connector.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
--
Changes since v1/RFC:
 - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
   devm_drm_of_get_bridge"
 - eDP/DP mode determined from the next bridge connector type.

Changes since v2:
 - Remove setting of Standard DP Scrambler Seed. (It's read-only).
 - Prevent setting DP_EDP_CONFIGURATION_SET in
   ti_sn_bridge_atomic_enable()
 - Use Doug's suggested text for disabling ASSR on DP mode.

Changes since v3:
 - Remove ASSR_CONTROL definition

Changes since v4:
 - Refactor code to configure the DP/eDP scrambler in one place.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f085a037ff5b..a6b15ea4e84d 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -92,6 +92,8 @@
 #define SN_DATARATE_CONFIG_REG			0x94
 #define  DP_DATARATE_MASK			GENMASK(7, 5)
 #define  DP_DATARATE(x)				((x) << 5)
+#define SN_TRAINING_SETTING_REG			0x95
+#define  SCRAMBLE_DISABLE			BIT(4)
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
@@ -1075,12 +1077,23 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	/*
 	 * The SN65DSI86 only supports ASSR Display Authentication method and
-	 * this method is enabled by default. An eDP panel must support this
+	 * this method is enabled for eDP panels. An eDP panel must support this
 	 * authentication method. We need to enable this method in the eDP panel
 	 * at DisplayPort address 0x0010A prior to link training.
+	 *
+	 * As only ASSR is supported by SN65DSI86, for full DisplayPort displays
+	 * we need to disable the scrambler.
 	 */
-	drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
-			   DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
+				   DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+
+		regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+				   SCRAMBLE_DISABLE, 0);
+	} else {
+		regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+				   SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
+	}
 
 	bpp = ti_sn_bridge_get_bpp(connector);
 	/* Set the DP output format (18 bpp or 24 bpp) */
@@ -1246,6 +1259,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
+	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
+			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.34.1

[PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Tomi Valkeinen]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Implement the bridge connector-related .get_edid() and .detect()
operations, and report the related bridge capabilities and type.

These ops are only added for DP mode. They should also be used for eDP
mode, but the driver seems to be mostly used for eDP and, according to
the comments, they've had issues with eDP panels and HPD. So better be
safe and only enable them for DP for now.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
Changes since v1:

- The connector .get_modes() operation doesn't rely on EDID anymore,
  __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
  together
 - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS

Changes since v2: [Kieran]
 - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.

Changes since v3: [Kieran]
 - Remove PM calls in ti_sn_bridge_get_edid() and simplify

Changes since v4:
 - Add .detect()
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 +++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index a6b15ea4e84d..dd20624adc70 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -29,6 +29,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_bridge_connector.h>
+#include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
@@ -68,6 +69,7 @@
 #define  BPP_18_RGB				BIT(0)
 #define SN_HPD_DISABLE_REG			0x5C
 #define  HPD_DISABLE				BIT(0)
+#define  HPD_DEBOUNCED_STATE			BIT(4)
 #define SN_GPIO_IO_REG				0x5E
 #define  SN_GPIO_INPUT_SHIFT			4
 #define  SN_GPIO_OUTPUT_SHIFT			0
@@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
 	pm_runtime_put_sync(pdata->dev);
 }
 
+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	int val = 0;
+
+	pm_runtime_get_sync(pdata->dev);
+	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+	pm_runtime_put_autosuspend(pdata->dev);
+
+	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
+					 : connector_status_disconnected;
+}
+
+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
+					  struct drm_connector *connector)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	return drm_get_edid(connector, &pdata->aux.ddc);
+}
+
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
 	.mode_valid = ti_sn_bridge_mode_valid,
+	.get_edid = ti_sn_bridge_get_edid,
+	.detect = ti_sn_bridge_detect,
 	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
 	.atomic_enable = ti_sn_bridge_atomic_enable,
 	.atomic_disable = ti_sn_bridge_atomic_disable,
@@ -1262,6 +1287,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
 			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
+
 	drm_bridge_add(&pdata->bridge);
 
 	ret = ti_sn_attach_host(pdata);
-- 
2.34.1

Re: [PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 24, 2022 at 04:00:31PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
> sending AUX transfers, which leads to the driver not returning an error.
> 
> Add the check.

That looks right.

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

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 90bbabde1595..ba84215c1511 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -582,6 +582,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
>  		goto exit;
>  	}
>  
> +	if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> +		ret = -EIO;
> +		goto exit;
> +	}
> +
>  	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
>  		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
>  		if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 24, 2022 at 04:00:31PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
> sending AUX transfers, which leads to the driver not returning an error.
> 
> Add the check.

That looks right.

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

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 90bbabde1595..ba84215c1511 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -582,6 +582,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
>  		goto exit;
>  	}
>  
> +	if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> +		ret = -EIO;
> +		goto exit;
> +	}
> +
>  	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
>  		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
>  		if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 24, 2022 at 04:00:32PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The blanking related registers are 8 bits, so reject any modes
> with larger blanking periods.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba84215c1511..f085a037ff5b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>  	if (mode->clock > 594000)
>  		return MODE_CLOCK_HIGH;
>  
> +	/*
> +	 * The blanking related registers are 8 bits, so reject any modes

s/blanking register/blanking-related/

> +	 * with larger blanking periods.
> +	 */
> +
> +	if ((mode->hsync_start - mode->hdisplay) > 0xff)
> +		return MODE_HBLANK_WIDE;
> +
> +	if ((mode->vsync_start - mode->vdisplay) > 0xff)
> +		return MODE_VBLANK_WIDE;
> +
> +	if ((mode->hsync_end - mode->hsync_start) > 0xff)
> +		return MODE_HSYNC_WIDE;
> +
> +	if ((mode->vsync_end - mode->vsync_start) > 0xff)
> +		return MODE_VSYNC_WIDE;
> +
> +	if ((mode->htotal - mode->hsync_end) > 0xff)
> +		return MODE_HBLANK_WIDE;
> +
> +	if ((mode->vtotal - mode->vsync_end) > 0xff)
> +		return MODE_VBLANK_WIDE;

You could drop all inner parentheses. Up to you.

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

> +
>  	return MODE_OK;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 24, 2022 at 04:00:32PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The blanking related registers are 8 bits, so reject any modes
> with larger blanking periods.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba84215c1511..f085a037ff5b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>  	if (mode->clock > 594000)
>  		return MODE_CLOCK_HIGH;
>  
> +	/*
> +	 * The blanking related registers are 8 bits, so reject any modes

s/blanking register/blanking-related/

> +	 * with larger blanking periods.
> +	 */
> +
> +	if ((mode->hsync_start - mode->hdisplay) > 0xff)
> +		return MODE_HBLANK_WIDE;
> +
> +	if ((mode->vsync_start - mode->vdisplay) > 0xff)
> +		return MODE_VBLANK_WIDE;
> +
> +	if ((mode->hsync_end - mode->hsync_start) > 0xff)
> +		return MODE_HSYNC_WIDE;
> +
> +	if ((mode->vsync_end - mode->vsync_start) > 0xff)
> +		return MODE_VSYNC_WIDE;
> +
> +	if ((mode->htotal - mode->hsync_end) > 0xff)
> +		return MODE_HBLANK_WIDE;
> +
> +	if ((mode->vtotal - mode->vsync_end) > 0xff)
> +		return MODE_VBLANK_WIDE;

You could drop all inner parentheses. Up to you.

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

> +
>  	return MODE_OK;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Laurent Pinchart]

Hi Tomi,

On Wed, Aug 24, 2022 at 04:00:34PM +0300, Tomi Valkeinen wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Implement the bridge connector-related .get_edid() and .detect()
> operations, and report the related bridge capabilities and type.
> 
> These ops are only added for DP mode. They should also be used for eDP
> mode, but the driver seems to be mostly used for eDP and, according to
> the comments, they've had issues with eDP panels and HPD. So better be
> safe and only enable them for DP for now.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

This patch is missing a third-party review, so I won't apply the series
to my tree but will let you pushing it through drm-misc once we get the
necessary review.

> ---
> Changes since v1:
> 
> - The connector .get_modes() operation doesn't rely on EDID anymore,
>   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
>   together
>  - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> 
> Changes since v2: [Kieran]
>  - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> 
> Changes since v3: [Kieran]
>  - Remove PM calls in ti_sn_bridge_get_edid() and simplify
> 
> Changes since v4:
>  - Add .detect()
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 +++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index a6b15ea4e84d..dd20624adc70 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -29,6 +29,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_bridge_connector.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
> @@ -68,6 +69,7 @@
>  #define  BPP_18_RGB				BIT(0)
>  #define SN_HPD_DISABLE_REG			0x5C
>  #define  HPD_DISABLE				BIT(0)
> +#define  HPD_DEBOUNCED_STATE			BIT(4)
>  #define SN_GPIO_IO_REG				0x5E
>  #define  SN_GPIO_INPUT_SHIFT			4
>  #define  SN_GPIO_OUTPUT_SHIFT			0
> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>  	pm_runtime_put_sync(pdata->dev);
>  }
>  
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +	int val = 0;
> +
> +	pm_runtime_get_sync(pdata->dev);

Would this

	int ret;

	ret = pm_runtime_resume_and_get(pdata->dev);
	if (ret)
		return connector_status_disconnected;

(or possibly connector_status_unknown) help avoiding problems ?

Apart from that, the patch looks good to me.

> +	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +	pm_runtime_put_autosuspend(pdata->dev);
> +
> +	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> +					 : connector_status_disconnected;
> +}
> +
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> +					  struct drm_connector *connector)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> +	return drm_get_edid(connector, &pdata->aux.ddc);
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.attach = ti_sn_bridge_attach,
>  	.detach = ti_sn_bridge_detach,
>  	.mode_valid = ti_sn_bridge_mode_valid,
> +	.get_edid = ti_sn_bridge_get_edid,
> +	.detect = ti_sn_bridge_detect,
>  	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
>  	.atomic_enable = ti_sn_bridge_atomic_enable,
>  	.atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1262,6 +1287,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>  	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
>  			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>  
> +	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> +
>  	drm_bridge_add(&pdata->bridge);
>  
>  	ret = ti_sn_attach_host(pdata);
> -- 
> 2.34.1
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Laurent Pinchart]

Hi Tomi,

On Wed, Aug 24, 2022 at 04:00:34PM +0300, Tomi Valkeinen wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Implement the bridge connector-related .get_edid() and .detect()
> operations, and report the related bridge capabilities and type.
> 
> These ops are only added for DP mode. They should also be used for eDP
> mode, but the driver seems to be mostly used for eDP and, according to
> the comments, they've had issues with eDP panels and HPD. So better be
> safe and only enable them for DP for now.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

This patch is missing a third-party review, so I won't apply the series
to my tree but will let you pushing it through drm-misc once we get the
necessary review.

> ---
> Changes since v1:
> 
> - The connector .get_modes() operation doesn't rely on EDID anymore,
>   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
>   together
>  - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> 
> Changes since v2: [Kieran]
>  - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> 
> Changes since v3: [Kieran]
>  - Remove PM calls in ti_sn_bridge_get_edid() and simplify
> 
> Changes since v4:
>  - Add .detect()
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 +++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index a6b15ea4e84d..dd20624adc70 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -29,6 +29,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_bridge_connector.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
> @@ -68,6 +69,7 @@
>  #define  BPP_18_RGB				BIT(0)
>  #define SN_HPD_DISABLE_REG			0x5C
>  #define  HPD_DISABLE				BIT(0)
> +#define  HPD_DEBOUNCED_STATE			BIT(4)
>  #define SN_GPIO_IO_REG				0x5E
>  #define  SN_GPIO_INPUT_SHIFT			4
>  #define  SN_GPIO_OUTPUT_SHIFT			0
> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>  	pm_runtime_put_sync(pdata->dev);
>  }
>  
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +	int val = 0;
> +
> +	pm_runtime_get_sync(pdata->dev);

Would this

	int ret;

	ret = pm_runtime_resume_and_get(pdata->dev);
	if (ret)
		return connector_status_disconnected;

(or possibly connector_status_unknown) help avoiding problems ?

Apart from that, the patch looks good to me.

> +	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +	pm_runtime_put_autosuspend(pdata->dev);
> +
> +	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> +					 : connector_status_disconnected;
> +}
> +
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> +					  struct drm_connector *connector)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> +	return drm_get_edid(connector, &pdata->aux.ddc);
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.attach = ti_sn_bridge_attach,
>  	.detach = ti_sn_bridge_detach,
>  	.mode_valid = ti_sn_bridge_mode_valid,
> +	.get_edid = ti_sn_bridge_get_edid,
> +	.detect = ti_sn_bridge_detect,
>  	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
>  	.atomic_enable = ti_sn_bridge_atomic_enable,
>  	.atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1262,6 +1287,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>  	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
>  			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>  
> +	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> +
>  	drm_bridge_add(&pdata->bridge);
>  
>  	ret = ti_sn_attach_host(pdata);
> -- 
> 2.34.1
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Robert Foss]

On Sat, 27 Aug 2022 at 03:10, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomi,
>
> On Wed, Aug 24, 2022 at 04:00:34PM +0300, Tomi Valkeinen wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() and .detect()
> > operations, and report the related bridge capabilities and type.
> >
> > These ops are only added for DP mode. They should also be used for eDP
> > mode, but the driver seems to be mostly used for eDP and, according to
> > the comments, they've had issues with eDP panels and HPD. So better be
> > safe and only enable them for DP for now.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> This patch is missing a third-party review, so I won't apply the series
> to my tree but will let you pushing it through drm-misc once we get the
> necessary review.
>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> >   together
> >  - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> >
> > Changes since v2: [Kieran]
> >  - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> >
> > Changes since v3: [Kieran]
> >  - Remove PM calls in ti_sn_bridge_get_edid() and simplify
> >
> > Changes since v4:
> >  - Add .detect()
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 +++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index a6b15ea4e84d..dd20624adc70 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -29,6 +29,7 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_bridge_connector.h>
> > +#include <drm/drm_edid.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> > @@ -68,6 +69,7 @@
> >  #define  BPP_18_RGB                          BIT(0)
> >  #define SN_HPD_DISABLE_REG                   0x5C
> >  #define  HPD_DISABLE                         BIT(0)
> > +#define  HPD_DEBOUNCED_STATE                 BIT(4)
> >  #define SN_GPIO_IO_REG                               0x5E
> >  #define  SN_GPIO_INPUT_SHIFT                 4
> >  #define  SN_GPIO_OUTPUT_SHIFT                        0
> > @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> >       pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > +     struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +     int val = 0;
> > +
> > +     pm_runtime_get_sync(pdata->dev);
>
> Would this
>
>         int ret;
>
>         ret = pm_runtime_resume_and_get(pdata->dev);
>         if (ret)
>                 return connector_status_disconnected;
>
> (or possibly connector_status_unknown) help avoiding problems ?
>
> Apart from that, the patch looks good to me.
>
> > +     regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> > +     pm_runtime_put_autosuspend(pdata->dev);
> > +
> > +     return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > +                                      : connector_status_disconnected;
> > +}
> > +
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > +                                       struct drm_connector *connector)
> > +{
> > +     struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > +     return drm_get_edid(connector, &pdata->aux.ddc);
> > +}
> > +
> >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >       .attach = ti_sn_bridge_attach,
> >       .detach = ti_sn_bridge_detach,
> >       .mode_valid = ti_sn_bridge_mode_valid,
> > +     .get_edid = ti_sn_bridge_get_edid,
> > +     .detect = ti_sn_bridge_detect,
> >       .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> >       .atomic_enable = ti_sn_bridge_atomic_enable,
> >       .atomic_disable = ti_sn_bridge_atomic_disable,
> > @@ -1262,6 +1287,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >       pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> >                          ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >
> > +     if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +             pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> > +
> >       drm_bridge_add(&pdata->bridge);
> >
> >       ret = ti_sn_attach_host(pdata);
> > --
> > 2.34.1

I think this patch looks good, please add my r-b.

@Laurent: I'll let you apply this, if you want the above suggestion to
be included.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Robert Foss]

On Sat, 27 Aug 2022 at 03:10, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomi,
>
> On Wed, Aug 24, 2022 at 04:00:34PM +0300, Tomi Valkeinen wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() and .detect()
> > operations, and report the related bridge capabilities and type.
> >
> > These ops are only added for DP mode. They should also be used for eDP
> > mode, but the driver seems to be mostly used for eDP and, according to
> > the comments, they've had issues with eDP panels and HPD. So better be
> > safe and only enable them for DP for now.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> This patch is missing a third-party review, so I won't apply the series
> to my tree but will let you pushing it through drm-misc once we get the
> necessary review.
>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> >   together
> >  - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> >
> > Changes since v2: [Kieran]
> >  - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> >
> > Changes since v3: [Kieran]
> >  - Remove PM calls in ti_sn_bridge_get_edid() and simplify
> >
> > Changes since v4:
> >  - Add .detect()
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 +++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index a6b15ea4e84d..dd20624adc70 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -29,6 +29,7 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_bridge_connector.h>
> > +#include <drm/drm_edid.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> > @@ -68,6 +69,7 @@
> >  #define  BPP_18_RGB                          BIT(0)
> >  #define SN_HPD_DISABLE_REG                   0x5C
> >  #define  HPD_DISABLE                         BIT(0)
> > +#define  HPD_DEBOUNCED_STATE                 BIT(4)
> >  #define SN_GPIO_IO_REG                               0x5E
> >  #define  SN_GPIO_INPUT_SHIFT                 4
> >  #define  SN_GPIO_OUTPUT_SHIFT                        0
> > @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> >       pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > +     struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +     int val = 0;
> > +
> > +     pm_runtime_get_sync(pdata->dev);
>
> Would this
>
>         int ret;
>
>         ret = pm_runtime_resume_and_get(pdata->dev);
>         if (ret)
>                 return connector_status_disconnected;
>
> (or possibly connector_status_unknown) help avoiding problems ?
>
> Apart from that, the patch looks good to me.
>
> > +     regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> > +     pm_runtime_put_autosuspend(pdata->dev);
> > +
> > +     return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > +                                      : connector_status_disconnected;
> > +}
> > +
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > +                                       struct drm_connector *connector)
> > +{
> > +     struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > +     return drm_get_edid(connector, &pdata->aux.ddc);
> > +}
> > +
> >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >       .attach = ti_sn_bridge_attach,
> >       .detach = ti_sn_bridge_detach,
> >       .mode_valid = ti_sn_bridge_mode_valid,
> > +     .get_edid = ti_sn_bridge_get_edid,
> > +     .detect = ti_sn_bridge_detect,
> >       .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> >       .atomic_enable = ti_sn_bridge_atomic_enable,
> >       .atomic_disable = ti_sn_bridge_atomic_disable,
> > @@ -1262,6 +1287,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >       pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> >                          ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >
> > +     if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +             pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> > +
> >       drm_bridge_add(&pdata->bridge);
> >
> >       ret = ti_sn_attach_host(pdata);
> > --
> > 2.34.1

I think this patch looks good, please add my r-b.

@Laurent: I'll let you apply this, if you want the above suggestion to
be included.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
> sending AUX transfers,

It doesn't? What about a few lines down from where your patch modifies
that reads:

  else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {

That seems like it's checking that bit?


> which leads to the driver not returning an error.

Right that it doesn't return an error. I guess the question is: should
it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
returning an error code? What problem are you fixing?

In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
failures"), at least, we thought that returning "0" and setting the
"reply" was the correct thing to do (though we didn't have any good
setup to test all the error paths).

...and looking through the code at drm_dp_i2c_do_msg(), I see that it
only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.

So I guess I would say:

1. Your patch doesn't seem right to me.

2. If your patch is actually fixing a problem, you should at least
modify it so that it doesn't create dead code (the old handling of
AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.

Naked-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
> sending AUX transfers,

It doesn't? What about a few lines down from where your patch modifies
that reads:

  else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {

That seems like it's checking that bit?


> which leads to the driver not returning an error.

Right that it doesn't return an error. I guess the question is: should
it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
returning an error code? What problem are you fixing?

In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
failures"), at least, we thought that returning "0" and setting the
"reply" was the correct thing to do (though we didn't have any good
setup to test all the error paths).

...and looking through the code at drm_dp_i2c_do_msg(), I see that it
only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.

So I guess I would say:

1. Your patch doesn't seem right to me.

2. If your patch is actually fixing a problem, you should at least
modify it so that it doesn't create dead code (the old handling of
AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.

Naked-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> The blanking related registers are 8 bits, so reject any modes
> with larger blanking periods.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba84215c1511..f085a037ff5b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>         if (mode->clock > 594000)
>                 return MODE_CLOCK_HIGH;
>
> +       /*
> +        * The blanking related registers are 8 bits, so reject any modes
> +        * with larger blanking periods.
> +        */
> +
> +       if ((mode->hsync_start - mode->hdisplay) > 0xff)
> +               return MODE_HBLANK_WIDE;
> +
> +       if ((mode->vsync_start - mode->vdisplay) > 0xff)
> +               return MODE_VBLANK_WIDE;
> +
> +       if ((mode->hsync_end - mode->hsync_start) > 0xff)
> +               return MODE_HSYNC_WIDE;

Please double-check your patch. Reading through
ti_sn_bridge_set_video_timings(), I see "mode->hsync_end -
mode->hsync_start" is allowed to be up to 0x7fff. The datasheet seems
to confirm. If I got that right it means you're rejecting valid modes.

I didn't validate any of your other checks, but at least that one seems wrong.

SInce this already had a Reviewed-by tag, being explicit:

Naked-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> The blanking related registers are 8 bits, so reject any modes
> with larger blanking periods.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba84215c1511..f085a037ff5b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>         if (mode->clock > 594000)
>                 return MODE_CLOCK_HIGH;
>
> +       /*
> +        * The blanking related registers are 8 bits, so reject any modes
> +        * with larger blanking periods.
> +        */
> +
> +       if ((mode->hsync_start - mode->hdisplay) > 0xff)
> +               return MODE_HBLANK_WIDE;
> +
> +       if ((mode->vsync_start - mode->vdisplay) > 0xff)
> +               return MODE_VBLANK_WIDE;
> +
> +       if ((mode->hsync_end - mode->hsync_start) > 0xff)
> +               return MODE_HSYNC_WIDE;

Please double-check your patch. Reading through
ti_sn_bridge_set_video_timings(), I see "mode->hsync_end -
mode->hsync_start" is allowed to be up to 0x7fff. The datasheet seems
to confirm. If I got that right it means you're rejecting valid modes.

I didn't validate any of your other checks, but at least that one seems wrong.

SInce this already had a Reviewed-by tag, being explicit:

Naked-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() and .detect()
> operations, and report the related bridge capabilities and type.
>
> These ops are only added for DP mode. They should also be used for eDP
> mode, but the driver seems to be mostly used for eDP and, according to
> the comments, they've had issues with eDP panels and HPD. So better be
> safe and only enable them for DP for now.

Just to be clear: the "They should also be used for eDP" is not correct.

* The detect() function should be returning whether the display is
physically there. For eDP it is _always_ physically there. Thus for
eDP the _correct_ implementation for detect is to always return true.
Yes, there is a line called HPD for eDP and yes that line is used for
full DisplayPort for detecting a display. For eDP, though, HPD does
not detect the presence of a display. A display is always there.

* For eDP implementing get_edid() is done in the panel so that power
sequencing can be done properly. While it could have been designed
other ways, that's how we ended up in the end. Thus eDP controllers
don't implement get_edid().


> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>         pm_runtime_put_sync(pdata->dev);
>  }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       int val = 0;
> +
> +       pm_runtime_get_sync(pdata->dev);
> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +       pm_runtime_put_autosuspend(pdata->dev);
> +
> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> +                                        : connector_status_disconnected;
> +}

I thought in the end we decided that you _could_ get a hot plug detect
interrupt if you just did a pm_runtime_get_sync() sometime earlier in
the case of DP. Basically you're just saying that if you're DP that
you always powered up. Doing some searches makes me find some
discussion at:

https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com

Specifically, the right answer is: "In general the pm_runtime_get
reference need to go with the IRQ enabling"

In any case, if we want to start with just implementing "detect"
that's OK with me...

Thus with the commit message clarified, feel free to add my Reviewed-by.

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() and .detect()
> operations, and report the related bridge capabilities and type.
>
> These ops are only added for DP mode. They should also be used for eDP
> mode, but the driver seems to be mostly used for eDP and, according to
> the comments, they've had issues with eDP panels and HPD. So better be
> safe and only enable them for DP for now.

Just to be clear: the "They should also be used for eDP" is not correct.

* The detect() function should be returning whether the display is
physically there. For eDP it is _always_ physically there. Thus for
eDP the _correct_ implementation for detect is to always return true.
Yes, there is a line called HPD for eDP and yes that line is used for
full DisplayPort for detecting a display. For eDP, though, HPD does
not detect the presence of a display. A display is always there.

* For eDP implementing get_edid() is done in the panel so that power
sequencing can be done properly. While it could have been designed
other ways, that's how we ended up in the end. Thus eDP controllers
don't implement get_edid().


> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>         pm_runtime_put_sync(pdata->dev);
>  }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       int val = 0;
> +
> +       pm_runtime_get_sync(pdata->dev);
> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +       pm_runtime_put_autosuspend(pdata->dev);
> +
> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> +                                        : connector_status_disconnected;
> +}

I thought in the end we decided that you _could_ get a hot plug detect
interrupt if you just did a pm_runtime_get_sync() sometime earlier in
the case of DP. Basically you're just saying that if you're DP that
you always powered up. Doing some searches makes me find some
discussion at:

https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com

Specifically, the right answer is: "In general the pm_runtime_get
reference need to go with the IRQ enabling"

In any case, if we want to start with just implementing "detect"
that's OK with me...

Thus with the commit message clarified, feel free to add my Reviewed-by.

Re: [PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Tomi Valkeinen]

Hi Doug,

On 29/08/2022 20:23, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> The blanking related registers are 8 bits, so reject any modes
>> with larger blanking periods.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index ba84215c1511..f085a037ff5b 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>>          if (mode->clock > 594000)
>>                  return MODE_CLOCK_HIGH;
>>
>> +       /*
>> +        * The blanking related registers are 8 bits, so reject any modes
>> +        * with larger blanking periods.
>> +        */
>> +
>> +       if ((mode->hsync_start - mode->hdisplay) > 0xff)
>> +               return MODE_HBLANK_WIDE;
>> +
>> +       if ((mode->vsync_start - mode->vdisplay) > 0xff)
>> +               return MODE_VBLANK_WIDE;
>> +
>> +       if ((mode->hsync_end - mode->hsync_start) > 0xff)
>> +               return MODE_HSYNC_WIDE;
> 
> Please double-check your patch. Reading through
> ti_sn_bridge_set_video_timings(), I see "mode->hsync_end -
> mode->hsync_start" is allowed to be up to 0x7fff. The datasheet seems
> to confirm. If I got that right it means you're rejecting valid modes.
> 
> I didn't validate any of your other checks, but at least that one seems wrong.

Indeed, I misread the spec. The pulse width registers are 15 bits. The 
front and back porch are 8 bits.

Thanks!

  Tomi

Re: [PATCH v5 2/4] drm/bridge: ti-sn65dsi86: Reject modes with too large blanking

[Tomi Valkeinen]

Hi Doug,

On 29/08/2022 20:23, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> The blanking related registers are 8 bits, so reject any modes
>> with larger blanking periods.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index ba84215c1511..f085a037ff5b 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -752,6 +752,29 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>>          if (mode->clock > 594000)
>>                  return MODE_CLOCK_HIGH;
>>
>> +       /*
>> +        * The blanking related registers are 8 bits, so reject any modes
>> +        * with larger blanking periods.
>> +        */
>> +
>> +       if ((mode->hsync_start - mode->hdisplay) > 0xff)
>> +               return MODE_HBLANK_WIDE;
>> +
>> +       if ((mode->vsync_start - mode->vdisplay) > 0xff)
>> +               return MODE_VBLANK_WIDE;
>> +
>> +       if ((mode->hsync_end - mode->hsync_start) > 0xff)
>> +               return MODE_HSYNC_WIDE;
> 
> Please double-check your patch. Reading through
> ti_sn_bridge_set_video_timings(), I see "mode->hsync_end -
> mode->hsync_start" is allowed to be up to 0x7fff. The datasheet seems
> to confirm. If I got that right it means you're rejecting valid modes.
> 
> I didn't validate any of your other checks, but at least that one seems wrong.

Indeed, I misread the spec. The pulse width registers are 15 bits. The 
front and back porch are 8 bits.

Thanks!

  Tomi

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Tomi Valkeinen]

Hi,

On 29/08/2022 20:38, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> Implement the bridge connector-related .get_edid() and .detect()
>> operations, and report the related bridge capabilities and type.
>>
>> These ops are only added for DP mode. They should also be used for eDP
>> mode, but the driver seems to be mostly used for eDP and, according to
>> the comments, they've had issues with eDP panels and HPD. So better be
>> safe and only enable them for DP for now.
> 
> Just to be clear: the "They should also be used for eDP" is not correct.
> 
> * The detect() function should be returning whether the display is
> physically there. For eDP it is _always_ physically there. Thus for

Really? I thought detect() is the polling counter-part of HPD interrupt. 
What is the point of returning true from detect() if the display is 
there, but cannot be used?

> eDP the _correct_ implementation for detect is to always return true.
> Yes, there is a line called HPD for eDP and yes that line is used for
> full DisplayPort for detecting a display. For eDP, though, HPD does
> not detect the presence of a display. A display is always there.

But for eDP it still signals the actual availability of the display, 
similarly to DP, doesn't it? You can't communicate with the monitor or 
read the EDID until you get the HPD.

> * For eDP implementing get_edid() is done in the panel so that power
> sequencing can be done properly. While it could have been designed
> other ways, that's how we ended up in the end. Thus eDP controllers
> don't implement get_edid().

Ok. I guess eDP panels do what they want and the drivers cannot rely on 
the HPD.

Or is the whole point here that because eDP panel drivers deal with the 
panel quirks, the get_edid() and also detect (if any) is handled by the 
eDP panel driver, and thus the bridge should not implement get_edid() 
nor detect() for eDP?

>> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>>          pm_runtime_put_sync(pdata->dev);
>>   }
>>
>> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>> +{
>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +       int val = 0;
>> +
>> +       pm_runtime_get_sync(pdata->dev);
>> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>> +       pm_runtime_put_autosuspend(pdata->dev);
>> +
>> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>> +                                        : connector_status_disconnected;
>> +}
> 
> I thought in the end we decided that you _could_ get a hot plug detect
> interrupt if you just did a pm_runtime_get_sync() sometime earlier in
> the case of DP. Basically you're just saying that if you're DP that
> you always powered up. Doing some searches makes me find some
> discussion at:
> 
> https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com
> 
> Specifically, the right answer is: "In general the pm_runtime_get
> reference need to go with the IRQ enabling"
> 
> In any case, if we want to start with just implementing "detect"
> that's OK with me...

Yes, I have the HPD interrupt working in my branch, kind of. The problem 
is that with the HPD interrupt I encountered issues (even if the monitor 
was always connected): every now and then the dsi86 does not display 
anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I 
couldn't figure out the problem. All the registers on the DSI source and 
DSI sink side looked identical, so it hints to some kind of race issue, 
which might well be there even with polling, but just doesn't seem to 
trigger.

To make things worse, the board in question is a remote board and I 
can't actually test the HPD, i.e. plugging in and out the cable, 
changing the monitors, powering up/down the monitors, etc.

On top of that, a few years back I had a lot of problems working on 
Cadence DP controller, dealing with all kinds of corner case race issues 
with DP HPD and trying to comply with the DP spec, which made me realize 
that DP is just really complex.

So, I thought it's better if I just try to get a minimum version working 
so that we can have a picture on a monitor, without even trying to claim 
real HPD support.

  Tomi

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Tomi Valkeinen]

Hi,

On 29/08/2022 20:38, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> Implement the bridge connector-related .get_edid() and .detect()
>> operations, and report the related bridge capabilities and type.
>>
>> These ops are only added for DP mode. They should also be used for eDP
>> mode, but the driver seems to be mostly used for eDP and, according to
>> the comments, they've had issues with eDP panels and HPD. So better be
>> safe and only enable them for DP for now.
> 
> Just to be clear: the "They should also be used for eDP" is not correct.
> 
> * The detect() function should be returning whether the display is
> physically there. For eDP it is _always_ physically there. Thus for

Really? I thought detect() is the polling counter-part of HPD interrupt. 
What is the point of returning true from detect() if the display is 
there, but cannot be used?

> eDP the _correct_ implementation for detect is to always return true.
> Yes, there is a line called HPD for eDP and yes that line is used for
> full DisplayPort for detecting a display. For eDP, though, HPD does
> not detect the presence of a display. A display is always there.

But for eDP it still signals the actual availability of the display, 
similarly to DP, doesn't it? You can't communicate with the monitor or 
read the EDID until you get the HPD.

> * For eDP implementing get_edid() is done in the panel so that power
> sequencing can be done properly. While it could have been designed
> other ways, that's how we ended up in the end. Thus eDP controllers
> don't implement get_edid().

Ok. I guess eDP panels do what they want and the drivers cannot rely on 
the HPD.

Or is the whole point here that because eDP panel drivers deal with the 
panel quirks, the get_edid() and also detect (if any) is handled by the 
eDP panel driver, and thus the bridge should not implement get_edid() 
nor detect() for eDP?

>> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>>          pm_runtime_put_sync(pdata->dev);
>>   }
>>
>> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>> +{
>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +       int val = 0;
>> +
>> +       pm_runtime_get_sync(pdata->dev);
>> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>> +       pm_runtime_put_autosuspend(pdata->dev);
>> +
>> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>> +                                        : connector_status_disconnected;
>> +}
> 
> I thought in the end we decided that you _could_ get a hot plug detect
> interrupt if you just did a pm_runtime_get_sync() sometime earlier in
> the case of DP. Basically you're just saying that if you're DP that
> you always powered up. Doing some searches makes me find some
> discussion at:
> 
> https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com
> 
> Specifically, the right answer is: "In general the pm_runtime_get
> reference need to go with the IRQ enabling"
> 
> In any case, if we want to start with just implementing "detect"
> that's OK with me...

Yes, I have the HPD interrupt working in my branch, kind of. The problem 
is that with the HPD interrupt I encountered issues (even if the monitor 
was always connected): every now and then the dsi86 does not display 
anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I 
couldn't figure out the problem. All the registers on the DSI source and 
DSI sink side looked identical, so it hints to some kind of race issue, 
which might well be there even with polling, but just doesn't seem to 
trigger.

To make things worse, the board in question is a remote board and I 
can't actually test the HPD, i.e. plugging in and out the cable, 
changing the monitors, powering up/down the monitors, etc.

On top of that, a few years back I had a lot of problems working on 
Cadence DP controller, dealing with all kinds of corner case race issues 
with DP HPD and trying to comply with the DP spec, which made me realize 
that DP is just really complex.

So, I thought it's better if I just try to get a minimum version working 
so that we can have a picture on a monitor, without even trying to claim 
real HPD support.

  Tomi

Re: [PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Tomi Valkeinen]

Hi,

On 29/08/2022 20:15, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
>> sending AUX transfers,
> 
> It doesn't? What about a few lines down from where your patch modifies
> that reads:
> 
>    else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> 
> That seems like it's checking that bit?

You're right, the patch is obviously broken.

>> which leads to the driver not returning an error.
> 
> Right that it doesn't return an error. I guess the question is: should
> it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
> DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
> returning an error code? What problem are you fixing?

I encountered a problem where the monitor was not replying properly and 
the driver was just getting AUX_IRQ_STATUS_NAT_I2C_FAIL errors, but the 
drm_dp_dpcd_read functions were not returning an error and the driver 
didn't understand that none of the transactions are actually going through.

Of course, now that I try I'm unable to reproduce the situation, I can 
only see AUX_IRQ_STATUS_AUX_RPLY_TOUT when something is not right, never 
AUX_IRQ_STATUS_NAT_I2C_FAIL.

Looking at the code, AUX_IRQ_STATUS_NAT_I2C_FAIL sets the len to 0, so 
that should cause a failure when the driver compares the return value of 
drm_dp_dpcd_read to the expected number of bytes. So I have trouble 
understanding the behavior I saw.

I did have WIP IRQ code in my branch at that time, though, and I'm now 
kind of suspecting that code, as it also somehow triggers the DSI RX 
issues I mention in the other mail.

> In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
> failures"), at least, we thought that returning "0" and setting the
> "reply" was the correct thing to do (though we didn't have any good
> setup to test all the error paths).
> 
> ...and looking through the code at drm_dp_i2c_do_msg(), I see that it
> only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.
> 
> So I guess I would say:
> 
> 1. Your patch doesn't seem right to me.
> 
> 2. If your patch is actually fixing a problem, you should at least
> modify it so that it doesn't create dead code (the old handling of
> AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.
> 
> Naked-by: Douglas Anderson <dianders@chromium.org>

I'll drop this patch.

  Tomi

Re: [PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Tomi Valkeinen]

Hi,

On 29/08/2022 20:15, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
>> sending AUX transfers,
> 
> It doesn't? What about a few lines down from where your patch modifies
> that reads:
> 
>    else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> 
> That seems like it's checking that bit?

You're right, the patch is obviously broken.

>> which leads to the driver not returning an error.
> 
> Right that it doesn't return an error. I guess the question is: should
> it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
> DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
> returning an error code? What problem are you fixing?

I encountered a problem where the monitor was not replying properly and 
the driver was just getting AUX_IRQ_STATUS_NAT_I2C_FAIL errors, but the 
drm_dp_dpcd_read functions were not returning an error and the driver 
didn't understand that none of the transactions are actually going through.

Of course, now that I try I'm unable to reproduce the situation, I can 
only see AUX_IRQ_STATUS_AUX_RPLY_TOUT when something is not right, never 
AUX_IRQ_STATUS_NAT_I2C_FAIL.

Looking at the code, AUX_IRQ_STATUS_NAT_I2C_FAIL sets the len to 0, so 
that should cause a failure when the driver compares the return value of 
drm_dp_dpcd_read to the expected number of bytes. So I have trouble 
understanding the behavior I saw.

I did have WIP IRQ code in my branch at that time, though, and I'm now 
kind of suspecting that code, as it also somehow triggers the DSI RX 
issues I mention in the other mail.

> In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
> failures"), at least, we thought that returning "0" and setting the
> "reply" was the correct thing to do (though we didn't have any good
> setup to test all the error paths).
> 
> ...and looking through the code at drm_dp_i2c_do_msg(), I see that it
> only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.
> 
> So I guess I would say:
> 
> 1. Your patch doesn't seem right to me.
> 
> 2. If your patch is actually fixing a problem, you should at least
> modify it so that it doesn't create dead code (the old handling of
> AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.
> 
> Naked-by: Douglas Anderson <dianders@chromium.org>

I'll drop this patch.

  Tomi

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Tue, Aug 30, 2022 at 2:00 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> On 29/08/2022 20:38, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> >> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>
> >> Implement the bridge connector-related .get_edid() and .detect()
> >> operations, and report the related bridge capabilities and type.
> >>
> >> These ops are only added for DP mode. They should also be used for eDP
> >> mode, but the driver seems to be mostly used for eDP and, according to
> >> the comments, they've had issues with eDP panels and HPD. So better be
> >> safe and only enable them for DP for now.
> >
> > Just to be clear: the "They should also be used for eDP" is not correct.
> >
> > * The detect() function should be returning whether the display is
> > physically there. For eDP it is _always_ physically there. Thus for
>
> Really? I thought detect() is the polling counter-part of HPD interrupt.
> What is the point of returning true from detect() if the display is
> there, but cannot be used?

The critical thing to realize is that for eDP the "HPD" signal does
not mean "hot plug detect". Worst. Name. Ever. The HPD signal in eDP
should be read as "panel IRQ" or "panel ready" or "panel attention" or
something. Anything but "hot plug detect". eDP is not hot plugged. You
can't take your laptop and, while it's on, pop the panel out and put
in another. It's simply not designed for it.

Specifically, eDP is _designed_ such that when the panel is turned off
the system should remove power to the panel. ...and when you remove
power to the panel then HPD goes low. Yet the panel is still there.
How do you know? You simply assume that since this is an eDP port that
it has a panel attached to it. You power it on and you use the "HPD"
signal (AKA "panel ready") to tell when it's finished powering on.

This is like every other non-hot-pluggable device in your system. If
your board has an audio codec then you just know it's there. You power
it on, wait a fixed amount of time for it to boot (or maybe wait until
it asserts a GPIO that says it's ready) and then you use it. That
i2c-controlled trackpad? Same thing. Your eMMC chip? You assume it's
there and power it up.


> > eDP the _correct_ implementation for detect is to always return true.
> > Yes, there is a line called HPD for eDP and yes that line is used for
> > full DisplayPort for detecting a display. For eDP, though, HPD does
> > not detect the presence of a display. A display is always there.
>
> But for eDP it still signals the actual availability of the display,
> similarly to DP, doesn't it? You can't communicate with the monitor or
> read the EDID until you get the HPD.

It signals that the display has finished booting, _not_ whether the
display is there. The display is always there.

There are simply two concepts:
1. Is a display there?
2. Can I talk to the display?

I assert that the way that "detect" is used in the DRM core is for #1.

In theory one could try to conflate the two. Everyone keeps trying
until they think about it more. Probably because the signal is named
HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
In any case, here lies dragons. Specifically if you conflate these two
concepts then when do you know to provide power to the display?
Remember, you can't detect the display until it's powered. ...but why
would you power it if you thought it wasn't there? You could power it
once at bootup, but then if someone turns the display off how will you
ever know that you can power it back on? It'll look like the display
was removed...


> > * For eDP implementing get_edid() is done in the panel so that power
> > sequencing can be done properly. While it could have been designed
> > other ways, that's how we ended up in the end. Thus eDP controllers
> > don't implement get_edid().
>
> Ok. I guess eDP panels do what they want and the drivers cannot rely on
> the HPD.
>
> Or is the whole point here that because eDP panel drivers deal with the
> panel quirks, the get_edid() and also detect (if any) is handled by the
> eDP panel driver, and thus the bridge should not implement get_edid()
> nor detect() for eDP?

It gets down to making sure things are powered. If the eDP controller
implements get_edid() then the eDP controller needs to know how to
power on the panel in response to that get_edid(). Remember, this is
eDP and we have to _always_ say the panel is there even when HPD
hasn't been asserted. See the above rant^H^H^H^H explanation. While
it's possible to have the eDP controller call down the bridge chain to
power the panel temporarily for get_edid() (early patches of mine did
that), in the end we decided it made more sense to have this driven by
the panel driver.


> >> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> >>          pm_runtime_put_sync(pdata->dev);
> >>   }
> >>
> >> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> >> +{
> >> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >> +       int val = 0;
> >> +
> >> +       pm_runtime_get_sync(pdata->dev);
> >> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> >> +       pm_runtime_put_autosuspend(pdata->dev);
> >> +
> >> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> >> +                                        : connector_status_disconnected;
> >> +}
> >
> > I thought in the end we decided that you _could_ get a hot plug detect
> > interrupt if you just did a pm_runtime_get_sync() sometime earlier in
> > the case of DP. Basically you're just saying that if you're DP that
> > you always powered up. Doing some searches makes me find some
> > discussion at:
> >
> > https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com
> >
> > Specifically, the right answer is: "In general the pm_runtime_get
> > reference need to go with the IRQ enabling"
> >
> > In any case, if we want to start with just implementing "detect"
> > that's OK with me...
>
> Yes, I have the HPD interrupt working in my branch, kind of. The problem
> is that with the HPD interrupt I encountered issues (even if the monitor
> was always connected): every now and then the dsi86 does not display
> anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I
> couldn't figure out the problem. All the registers on the DSI source and
> DSI sink side looked identical, so it hints to some kind of race issue,
> which might well be there even with polling, but just doesn't seem to
> trigger.
>
> To make things worse, the board in question is a remote board and I
> can't actually test the HPD, i.e. plugging in and out the cable,
> changing the monitors, powering up/down the monitors, etc.
>
> On top of that, a few years back I had a lot of problems working on
> Cadence DP controller, dealing with all kinds of corner case race issues
> with DP HPD and trying to comply with the DP spec, which made me realize
> that DP is just really complex.
>
> So, I thought it's better if I just try to get a minimum version working
> so that we can have a picture on a monitor, without even trying to claim
> real HPD support.

Weird. OK. I guess we can see if someone later comes along and tries
to implement interrupt support. :-)

-Doug

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Tue, Aug 30, 2022 at 2:00 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> On 29/08/2022 20:38, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> >> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>
> >> Implement the bridge connector-related .get_edid() and .detect()
> >> operations, and report the related bridge capabilities and type.
> >>
> >> These ops are only added for DP mode. They should also be used for eDP
> >> mode, but the driver seems to be mostly used for eDP and, according to
> >> the comments, they've had issues with eDP panels and HPD. So better be
> >> safe and only enable them for DP for now.
> >
> > Just to be clear: the "They should also be used for eDP" is not correct.
> >
> > * The detect() function should be returning whether the display is
> > physically there. For eDP it is _always_ physically there. Thus for
>
> Really? I thought detect() is the polling counter-part of HPD interrupt.
> What is the point of returning true from detect() if the display is
> there, but cannot be used?

The critical thing to realize is that for eDP the "HPD" signal does
not mean "hot plug detect". Worst. Name. Ever. The HPD signal in eDP
should be read as "panel IRQ" or "panel ready" or "panel attention" or
something. Anything but "hot plug detect". eDP is not hot plugged. You
can't take your laptop and, while it's on, pop the panel out and put
in another. It's simply not designed for it.

Specifically, eDP is _designed_ such that when the panel is turned off
the system should remove power to the panel. ...and when you remove
power to the panel then HPD goes low. Yet the panel is still there.
How do you know? You simply assume that since this is an eDP port that
it has a panel attached to it. You power it on and you use the "HPD"
signal (AKA "panel ready") to tell when it's finished powering on.

This is like every other non-hot-pluggable device in your system. If
your board has an audio codec then you just know it's there. You power
it on, wait a fixed amount of time for it to boot (or maybe wait until
it asserts a GPIO that says it's ready) and then you use it. That
i2c-controlled trackpad? Same thing. Your eMMC chip? You assume it's
there and power it up.


> > eDP the _correct_ implementation for detect is to always return true.
> > Yes, there is a line called HPD for eDP and yes that line is used for
> > full DisplayPort for detecting a display. For eDP, though, HPD does
> > not detect the presence of a display. A display is always there.
>
> But for eDP it still signals the actual availability of the display,
> similarly to DP, doesn't it? You can't communicate with the monitor or
> read the EDID until you get the HPD.

It signals that the display has finished booting, _not_ whether the
display is there. The display is always there.

There are simply two concepts:
1. Is a display there?
2. Can I talk to the display?

I assert that the way that "detect" is used in the DRM core is for #1.

In theory one could try to conflate the two. Everyone keeps trying
until they think about it more. Probably because the signal is named
HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
In any case, here lies dragons. Specifically if you conflate these two
concepts then when do you know to provide power to the display?
Remember, you can't detect the display until it's powered. ...but why
would you power it if you thought it wasn't there? You could power it
once at bootup, but then if someone turns the display off how will you
ever know that you can power it back on? It'll look like the display
was removed...


> > * For eDP implementing get_edid() is done in the panel so that power
> > sequencing can be done properly. While it could have been designed
> > other ways, that's how we ended up in the end. Thus eDP controllers
> > don't implement get_edid().
>
> Ok. I guess eDP panels do what they want and the drivers cannot rely on
> the HPD.
>
> Or is the whole point here that because eDP panel drivers deal with the
> panel quirks, the get_edid() and also detect (if any) is handled by the
> eDP panel driver, and thus the bridge should not implement get_edid()
> nor detect() for eDP?

It gets down to making sure things are powered. If the eDP controller
implements get_edid() then the eDP controller needs to know how to
power on the panel in response to that get_edid(). Remember, this is
eDP and we have to _always_ say the panel is there even when HPD
hasn't been asserted. See the above rant^H^H^H^H explanation. While
it's possible to have the eDP controller call down the bridge chain to
power the panel temporarily for get_edid() (early patches of mine did
that), in the end we decided it made more sense to have this driven by
the panel driver.


> >> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> >>          pm_runtime_put_sync(pdata->dev);
> >>   }
> >>
> >> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> >> +{
> >> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >> +       int val = 0;
> >> +
> >> +       pm_runtime_get_sync(pdata->dev);
> >> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> >> +       pm_runtime_put_autosuspend(pdata->dev);
> >> +
> >> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> >> +                                        : connector_status_disconnected;
> >> +}
> >
> > I thought in the end we decided that you _could_ get a hot plug detect
> > interrupt if you just did a pm_runtime_get_sync() sometime earlier in
> > the case of DP. Basically you're just saying that if you're DP that
> > you always powered up. Doing some searches makes me find some
> > discussion at:
> >
> > https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com
> >
> > Specifically, the right answer is: "In general the pm_runtime_get
> > reference need to go with the IRQ enabling"
> >
> > In any case, if we want to start with just implementing "detect"
> > that's OK with me...
>
> Yes, I have the HPD interrupt working in my branch, kind of. The problem
> is that with the HPD interrupt I encountered issues (even if the monitor
> was always connected): every now and then the dsi86 does not display
> anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I
> couldn't figure out the problem. All the registers on the DSI source and
> DSI sink side looked identical, so it hints to some kind of race issue,
> which might well be there even with polling, but just doesn't seem to
> trigger.
>
> To make things worse, the board in question is a remote board and I
> can't actually test the HPD, i.e. plugging in and out the cable,
> changing the monitors, powering up/down the monitors, etc.
>
> On top of that, a few years back I had a lot of problems working on
> Cadence DP controller, dealing with all kinds of corner case race issues
> with DP HPD and trying to comply with the DP spec, which made me realize
> that DP is just really complex.
>
> So, I thought it's better if I just try to get a minimum version working
> so that we can have a picture on a monitor, without even trying to claim
> real HPD support.

Weird. OK. I guess we can see if someone later comes along and tries
to implement interrupt support. :-)

-Doug

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Tomi Valkeinen]

Hi,

On 30/08/2022 17:55, Doug Anderson wrote:
> Hi,
> 
> On Tue, Aug 30, 2022 at 2:00 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> Hi,
>>
>> On 29/08/2022 20:38, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>
>>>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>>
>>>> Implement the bridge connector-related .get_edid() and .detect()
>>>> operations, and report the related bridge capabilities and type.
>>>>
>>>> These ops are only added for DP mode. They should also be used for eDP
>>>> mode, but the driver seems to be mostly used for eDP and, according to
>>>> the comments, they've had issues with eDP panels and HPD. So better be
>>>> safe and only enable them for DP for now.
>>>
>>> Just to be clear: the "They should also be used for eDP" is not correct.
>>>
>>> * The detect() function should be returning whether the display is
>>> physically there. For eDP it is _always_ physically there. Thus for
>>
>> Really? I thought detect() is the polling counter-part of HPD interrupt.
>> What is the point of returning true from detect() if the display is
>> there, but cannot be used?
> 
> The critical thing to realize is that for eDP the "HPD" signal does
> not mean "hot plug detect". Worst. Name. Ever. The HPD signal in eDP
> should be read as "panel IRQ" or "panel ready" or "panel attention" or
> something. Anything but "hot plug detect". eDP is not hot plugged. You
> can't take your laptop and, while it's on, pop the panel out and put
> in another. It's simply not designed for it.

Well, I have to agree that the name is not the best possible. But the 
name is understandable, because of legacy, and in any case I don't 
really see it matters. Unless you go hot-plugging your eDP panel because 
it has HPD ;).

DP has IRQ HPD pulses and "real" HPD pulses. I think we can ignore the 
IRQ pulses in this discussion. The source should have the means to 
distinguish those two.

> Specifically, eDP is _designed_ such that when the panel is turned off
> the system should remove power to the panel. ...and when you remove
> power to the panel then HPD goes low. Yet the panel is still there.
> How do you know? You simply assume that since this is an eDP port that
> it has a panel attached to it. You power it on and you use the "HPD"
> signal (AKA "panel ready") to tell when it's finished powering on.

Yes, I agree.

> This is like every other non-hot-pluggable device in your system. If
> your board has an audio codec then you just know it's there. You power
> it on, wait a fixed amount of time for it to boot (or maybe wait until
> it asserts a GPIO that says it's ready) and then you use it. That
> i2c-controlled trackpad? Same thing. Your eMMC chip? You assume it's
> there and power it up.

Yep.

> 
>>> eDP the _correct_ implementation for detect is to always return true.
>>> Yes, there is a line called HPD for eDP and yes that line is used for
>>> full DisplayPort for detecting a display. For eDP, though, HPD does
>>> not detect the presence of a display. A display is always there.
>>
>> But for eDP it still signals the actual availability of the display,
>> similarly to DP, doesn't it? You can't communicate with the monitor or
>> read the EDID until you get the HPD.
> 
> It signals that the display has finished booting, _not_ whether the
> display is there. The display is always there.
> 
> There are simply two concepts:
> 1. Is a display there?
> 2. Can I talk to the display?
> 
> I assert that the way that "detect" is used in the DRM core is for #1.

Why is that? Can you point to any specific piece of code?

I didn't look it closely, but I believe in my testing I saw that the 
framework expects to be able to read EDID after detect() reports that 
the display is connected. And if EDID read fails, then you get only the 
default modes, even if the display was ready very soon afterwards. If so 
that hints more towards 2.

> In theory one could try to conflate the two. Everyone keeps trying

I agree here, they are not the same.

> until they think about it more. Probably because the signal is named
> HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
> In any case, here lies dragons. Specifically if you conflate these two
> concepts then when do you know to provide power to the display?
> Remember, you can't detect the display until it's powered. ...but why
> would you power it if you thought it wasn't there? You could power it
> once at bootup, but then if someone turns the display off how will you
> ever know that you can power it back on? It'll look like the display
> was removed...

But here's my question: if detect() tells whether the display is 
physically there, why do we need it?

If the display is not hot-pluggable, then, as you say, it's always 
there, and detect() is unnecessary. The panel driver always assumes the 
panel is there and will power it up. So detect is not really needed.

If the display is hot-pluggable, then we don't need to know if the 
display is physically there, but not ready. We need to know if it's 
ready. So detect is not needed, or rather, it doesn't do what is needed.

The above system feels a bit broken in my opinion. If, on the other 
hand, detect() is the polling counter-part of HPD, i.e. it tells if the 
display is ready, those two different cases converge. For not 
hot-pluggable displays the panel driver knows the panel is always there 
(without detect()), and will power it up. For hot-pluggable devices the 
user must connect the display and press the power button. In both cases 
HPD will then go high and detect() tells that the display is ready.

Of course, for eDP the HPD is optional, so without HPD the panel driver 
just needs to wait a known amount of time until reporting that the panel 
is ready (after maybe doing an AUX read).

>>> * For eDP implementing get_edid() is done in the panel so that power
>>> sequencing can be done properly. While it could have been designed
>>> other ways, that's how we ended up in the end. Thus eDP controllers
>>> don't implement get_edid().
>>
>> Ok. I guess eDP panels do what they want and the drivers cannot rely on
>> the HPD.
>>
>> Or is the whole point here that because eDP panel drivers deal with the
>> panel quirks, the get_edid() and also detect (if any) is handled by the
>> eDP panel driver, and thus the bridge should not implement get_edid()
>> nor detect() for eDP?
> 
> It gets down to making sure things are powered. If the eDP controller
> implements get_edid() then the eDP controller needs to know how to
> power on the panel in response to that get_edid(). Remember, this is
> eDP and we have to _always_ say the panel is there even when HPD
> hasn't been asserted. See the above rant^H^H^H^H explanation. While
> it's possible to have the eDP controller call down the bridge chain to
> power the panel temporarily for get_edid() (early patches of mine did
> that), in the end we decided it made more sense to have this driven by
> the panel driver.

I agree here, the panel driver has to drive the process. That's actually 
how I designed the old omapfb display subsystem (well, DP didn't exist 
then), everything originated from the display driver, not the crtc side.

However, my argument is that someone, be it the display or the source 
driver, should offer detect() and get_edid(), and afaics it makes sense 
for detect() to report whether the display is ready or not (usually HPD 
if it is connected, but could be via some other means).

However, I have to say this is perhaps sidetracking this patch =). I can 
drop the comment in question from the description as it's somewhat 
irrelevant wrt. this patch.

>>>> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>>>>           pm_runtime_put_sync(pdata->dev);
>>>>    }
>>>>
>>>> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>>> +{
>>>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>>> +       int val = 0;
>>>> +
>>>> +       pm_runtime_get_sync(pdata->dev);
>>>> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>>>> +       pm_runtime_put_autosuspend(pdata->dev);
>>>> +
>>>> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>>> +                                        : connector_status_disconnected;
>>>> +}
>>>
>>> I thought in the end we decided that you _could_ get a hot plug detect
>>> interrupt if you just did a pm_runtime_get_sync() sometime earlier in
>>> the case of DP. Basically you're just saying that if you're DP that
>>> you always powered up. Doing some searches makes me find some
>>> discussion at:
>>>
>>> https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com
>>>
>>> Specifically, the right answer is: "In general the pm_runtime_get
>>> reference need to go with the IRQ enabling"
>>>
>>> In any case, if we want to start with just implementing "detect"
>>> that's OK with me...
>>
>> Yes, I have the HPD interrupt working in my branch, kind of. The problem
>> is that with the HPD interrupt I encountered issues (even if the monitor
>> was always connected): every now and then the dsi86 does not display
>> anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I
>> couldn't figure out the problem. All the registers on the DSI source and
>> DSI sink side looked identical, so it hints to some kind of race issue,
>> which might well be there even with polling, but just doesn't seem to
>> trigger.
>>
>> To make things worse, the board in question is a remote board and I
>> can't actually test the HPD, i.e. plugging in and out the cable,
>> changing the monitors, powering up/down the monitors, etc.
>>
>> On top of that, a few years back I had a lot of problems working on
>> Cadence DP controller, dealing with all kinds of corner case race issues
>> with DP HPD and trying to comply with the DP spec, which made me realize
>> that DP is just really complex.
>>
>> So, I thought it's better if I just try to get a minimum version working
>> so that we can have a picture on a monitor, without even trying to claim
>> real HPD support.
> 
> Weird. OK. I guess we can see if someone later comes along and tries
> to implement interrupt support. :-)

I could actually add the HPD IRQ patch on top, as an RFC, for that 
future "someone". Or perhaps someone notices a similar stupid mistake in 
that patch as I made in this series and the problem gets solved =).

  Tomi

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Tomi Valkeinen]

Hi,

On 30/08/2022 17:55, Doug Anderson wrote:
> Hi,
> 
> On Tue, Aug 30, 2022 at 2:00 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> Hi,
>>
>> On 29/08/2022 20:38, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>
>>>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>>
>>>> Implement the bridge connector-related .get_edid() and .detect()
>>>> operations, and report the related bridge capabilities and type.
>>>>
>>>> These ops are only added for DP mode. They should also be used for eDP
>>>> mode, but the driver seems to be mostly used for eDP and, according to
>>>> the comments, they've had issues with eDP panels and HPD. So better be
>>>> safe and only enable them for DP for now.
>>>
>>> Just to be clear: the "They should also be used for eDP" is not correct.
>>>
>>> * The detect() function should be returning whether the display is
>>> physically there. For eDP it is _always_ physically there. Thus for
>>
>> Really? I thought detect() is the polling counter-part of HPD interrupt.
>> What is the point of returning true from detect() if the display is
>> there, but cannot be used?
> 
> The critical thing to realize is that for eDP the "HPD" signal does
> not mean "hot plug detect". Worst. Name. Ever. The HPD signal in eDP
> should be read as "panel IRQ" or "panel ready" or "panel attention" or
> something. Anything but "hot plug detect". eDP is not hot plugged. You
> can't take your laptop and, while it's on, pop the panel out and put
> in another. It's simply not designed for it.

Well, I have to agree that the name is not the best possible. But the 
name is understandable, because of legacy, and in any case I don't 
really see it matters. Unless you go hot-plugging your eDP panel because 
it has HPD ;).

DP has IRQ HPD pulses and "real" HPD pulses. I think we can ignore the 
IRQ pulses in this discussion. The source should have the means to 
distinguish those two.

> Specifically, eDP is _designed_ such that when the panel is turned off
> the system should remove power to the panel. ...and when you remove
> power to the panel then HPD goes low. Yet the panel is still there.
> How do you know? You simply assume that since this is an eDP port that
> it has a panel attached to it. You power it on and you use the "HPD"
> signal (AKA "panel ready") to tell when it's finished powering on.

Yes, I agree.

> This is like every other non-hot-pluggable device in your system. If
> your board has an audio codec then you just know it's there. You power
> it on, wait a fixed amount of time for it to boot (or maybe wait until
> it asserts a GPIO that says it's ready) and then you use it. That
> i2c-controlled trackpad? Same thing. Your eMMC chip? You assume it's
> there and power it up.

Yep.

> 
>>> eDP the _correct_ implementation for detect is to always return true.
>>> Yes, there is a line called HPD for eDP and yes that line is used for
>>> full DisplayPort for detecting a display. For eDP, though, HPD does
>>> not detect the presence of a display. A display is always there.
>>
>> But for eDP it still signals the actual availability of the display,
>> similarly to DP, doesn't it? You can't communicate with the monitor or
>> read the EDID until you get the HPD.
> 
> It signals that the display has finished booting, _not_ whether the
> display is there. The display is always there.
> 
> There are simply two concepts:
> 1. Is a display there?
> 2. Can I talk to the display?
> 
> I assert that the way that "detect" is used in the DRM core is for #1.

Why is that? Can you point to any specific piece of code?

I didn't look it closely, but I believe in my testing I saw that the 
framework expects to be able to read EDID after detect() reports that 
the display is connected. And if EDID read fails, then you get only the 
default modes, even if the display was ready very soon afterwards. If so 
that hints more towards 2.

> In theory one could try to conflate the two. Everyone keeps trying

I agree here, they are not the same.

> until they think about it more. Probably because the signal is named
> HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
> In any case, here lies dragons. Specifically if you conflate these two
> concepts then when do you know to provide power to the display?
> Remember, you can't detect the display until it's powered. ...but why
> would you power it if you thought it wasn't there? You could power it
> once at bootup, but then if someone turns the display off how will you
> ever know that you can power it back on? It'll look like the display
> was removed...

But here's my question: if detect() tells whether the display is 
physically there, why do we need it?

If the display is not hot-pluggable, then, as you say, it's always 
there, and detect() is unnecessary. The panel driver always assumes the 
panel is there and will power it up. So detect is not really needed.

If the display is hot-pluggable, then we don't need to know if the 
display is physically there, but not ready. We need to know if it's 
ready. So detect is not needed, or rather, it doesn't do what is needed.

The above system feels a bit broken in my opinion. If, on the other 
hand, detect() is the polling counter-part of HPD, i.e. it tells if the 
display is ready, those two different cases converge. For not 
hot-pluggable displays the panel driver knows the panel is always there 
(without detect()), and will power it up. For hot-pluggable devices the 
user must connect the display and press the power button. In both cases 
HPD will then go high and detect() tells that the display is ready.

Of course, for eDP the HPD is optional, so without HPD the panel driver 
just needs to wait a known amount of time until reporting that the panel 
is ready (after maybe doing an AUX read).

>>> * For eDP implementing get_edid() is done in the panel so that power
>>> sequencing can be done properly. While it could have been designed
>>> other ways, that's how we ended up in the end. Thus eDP controllers
>>> don't implement get_edid().
>>
>> Ok. I guess eDP panels do what they want and the drivers cannot rely on
>> the HPD.
>>
>> Or is the whole point here that because eDP panel drivers deal with the
>> panel quirks, the get_edid() and also detect (if any) is handled by the
>> eDP panel driver, and thus the bridge should not implement get_edid()
>> nor detect() for eDP?
> 
> It gets down to making sure things are powered. If the eDP controller
> implements get_edid() then the eDP controller needs to know how to
> power on the panel in response to that get_edid(). Remember, this is
> eDP and we have to _always_ say the panel is there even when HPD
> hasn't been asserted. See the above rant^H^H^H^H explanation. While
> it's possible to have the eDP controller call down the bridge chain to
> power the panel temporarily for get_edid() (early patches of mine did
> that), in the end we decided it made more sense to have this driven by
> the panel driver.

I agree here, the panel driver has to drive the process. That's actually 
how I designed the old omapfb display subsystem (well, DP didn't exist 
then), everything originated from the display driver, not the crtc side.

However, my argument is that someone, be it the display or the source 
driver, should offer detect() and get_edid(), and afaics it makes sense 
for detect() to report whether the display is ready or not (usually HPD 
if it is connected, but could be via some other means).

However, I have to say this is perhaps sidetracking this patch =). I can 
drop the comment in question from the description as it's somewhat 
irrelevant wrt. this patch.

>>>> @@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
>>>>           pm_runtime_put_sync(pdata->dev);
>>>>    }
>>>>
>>>> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>>> +{
>>>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>>> +       int val = 0;
>>>> +
>>>> +       pm_runtime_get_sync(pdata->dev);
>>>> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>>>> +       pm_runtime_put_autosuspend(pdata->dev);
>>>> +
>>>> +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>>> +                                        : connector_status_disconnected;
>>>> +}
>>>
>>> I thought in the end we decided that you _could_ get a hot plug detect
>>> interrupt if you just did a pm_runtime_get_sync() sometime earlier in
>>> the case of DP. Basically you're just saying that if you're DP that
>>> you always powered up. Doing some searches makes me find some
>>> discussion at:
>>>
>>> https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@ideasonboard.com
>>>
>>> Specifically, the right answer is: "In general the pm_runtime_get
>>> reference need to go with the IRQ enabling"
>>>
>>> In any case, if we want to start with just implementing "detect"
>>> that's OK with me...
>>
>> Yes, I have the HPD interrupt working in my branch, kind of. The problem
>> is that with the HPD interrupt I encountered issues (even if the monitor
>> was always connected): every now and then the dsi86 does not display
>> anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I
>> couldn't figure out the problem. All the registers on the DSI source and
>> DSI sink side looked identical, so it hints to some kind of race issue,
>> which might well be there even with polling, but just doesn't seem to
>> trigger.
>>
>> To make things worse, the board in question is a remote board and I
>> can't actually test the HPD, i.e. plugging in and out the cable,
>> changing the monitors, powering up/down the monitors, etc.
>>
>> On top of that, a few years back I had a lot of problems working on
>> Cadence DP controller, dealing with all kinds of corner case race issues
>> with DP HPD and trying to comply with the DP spec, which made me realize
>> that DP is just really complex.
>>
>> So, I thought it's better if I just try to get a minimum version working
>> so that we can have a picture on a monitor, without even trying to claim
>> real HPD support.
> 
> Weird. OK. I guess we can see if someone later comes along and tries
> to implement interrupt support. :-)

I could actually add the HPD IRQ patch on top, as an RFC, for that 
future "someone". Or perhaps someone notices a similar stupid mistake in 
that patch as I made in this series and the problem gets solved =).

  Tomi

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Tue, Aug 30, 2022 at 9:11 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> >>> eDP the _correct_ implementation for detect is to always return true.
> >>> Yes, there is a line called HPD for eDP and yes that line is used for
> >>> full DisplayPort for detecting a display. For eDP, though, HPD does
> >>> not detect the presence of a display. A display is always there.
> >>
> >> But for eDP it still signals the actual availability of the display,
> >> similarly to DP, doesn't it? You can't communicate with the monitor or
> >> read the EDID until you get the HPD.
> >
> > It signals that the display has finished booting, _not_ whether the
> > display is there. The display is always there.
> >
> > There are simply two concepts:
> > 1. Is a display there?
> > 2. Can I talk to the display?
> >
> > I assert that the way that "detect" is used in the DRM core is for #1.
>
> Why is that? Can you point to any specific piece of code?
>
> I didn't look it closely, but I believe in my testing I saw that the
> framework expects to be able to read EDID after detect() reports that
> the display is connected. And if EDID read fails, then you get only the
> default modes, even if the display was ready very soon afterwards. If so
> that hints more towards 2.

I guess it's mostly the chicken and egg problem. In your model, how
does the panel get turned on? Let's say that the eDP panel is off at
bootup, right? So "HPD" will not be high. detect() will say that
nothing is there. Since nothing is there, nobody will ever try to call
get_edid() nor will they ever try turning on the panel. ...and since
nobody turns on the panel HPD will never be high.

Now let's imagine that there is some rule to turn the panel on once at
bootup. Great, you'll see the panel at bootup. ...but then what
happens when you go through a modeset or suspend/resume or similar?
We'll turn the panel off as part of the modeset, HPD will go low, and
it will look like the panel is gone forever.

This is why detect() has to always say that an eDP panel is present.
If this isn't true the panel will never be turned on because we'll
never know it's there.


> > In theory one could try to conflate the two. Everyone keeps trying
>
> I agree here, they are not the same.
>
> > until they think about it more. Probably because the signal is named
> > HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
> > In any case, here lies dragons. Specifically if you conflate these two
> > concepts then when do you know to provide power to the display?
> > Remember, you can't detect the display until it's powered. ...but why
> > would you power it if you thought it wasn't there? You could power it
> > once at bootup, but then if someone turns the display off how will you
> > ever know that you can power it back on? It'll look like the display
> > was removed...
>
> But here's my question: if detect() tells whether the display is
> physically there, why do we need it?
>
> If the display is not hot-pluggable, then, as you say, it's always
> there, and detect() is unnecessary. The panel driver always assumes the
> panel is there and will power it up. So detect is not really needed.

Right. I conflated these two, sorry. Having detect() unimplemented and
having it always return true are the same thing and the DRM core
treats them the same as far as I'm aware.


> > It gets down to making sure things are powered. If the eDP controller
> > implements get_edid() then the eDP controller needs to know how to
> > power on the panel in response to that get_edid(). Remember, this is
> > eDP and we have to _always_ say the panel is there even when HPD
> > hasn't been asserted. See the above rant^H^H^H^H explanation. While
> > it's possible to have the eDP controller call down the bridge chain to
> > power the panel temporarily for get_edid() (early patches of mine did
> > that), in the end we decided it made more sense to have this driven by
> > the panel driver.
>
> I agree here, the panel driver has to drive the process. That's actually
> how I designed the old omapfb display subsystem (well, DP didn't exist
> then), everything originated from the display driver, not the crtc side.
>
> However, my argument is that someone, be it the display or the source
> driver, should offer detect() and get_edid(),

If you implement get_edid() I believe it won't be the end of the world
because the panel's version will be picked first. However, it feels
clearer to me to not implement it if it's not going to be used / won't
work for eDP.


> and afaics it makes sense
> for detect() to report whether the display is ready or not (usually HPD
> if it is connected, but could be via some other means).

I think detect() is actually harmful for eDP, as per my argument
above. If we detect the panel is "gone" then we'll turn off the power
to the panel. We'll never detect the panel again and we'll never again
have a reason to power the panel on.


> However, I have to say this is perhaps sidetracking this patch =). I can
> drop the comment in question from the description as it's somewhat
> irrelevant wrt. this patch.

Sounds good!

-Doug

Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Tue, Aug 30, 2022 at 9:11 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> >>> eDP the _correct_ implementation for detect is to always return true.
> >>> Yes, there is a line called HPD for eDP and yes that line is used for
> >>> full DisplayPort for detecting a display. For eDP, though, HPD does
> >>> not detect the presence of a display. A display is always there.
> >>
> >> But for eDP it still signals the actual availability of the display,
> >> similarly to DP, doesn't it? You can't communicate with the monitor or
> >> read the EDID until you get the HPD.
> >
> > It signals that the display has finished booting, _not_ whether the
> > display is there. The display is always there.
> >
> > There are simply two concepts:
> > 1. Is a display there?
> > 2. Can I talk to the display?
> >
> > I assert that the way that "detect" is used in the DRM core is for #1.
>
> Why is that? Can you point to any specific piece of code?
>
> I didn't look it closely, but I believe in my testing I saw that the
> framework expects to be able to read EDID after detect() reports that
> the display is connected. And if EDID read fails, then you get only the
> default modes, even if the display was ready very soon afterwards. If so
> that hints more towards 2.

I guess it's mostly the chicken and egg problem. In your model, how
does the panel get turned on? Let's say that the eDP panel is off at
bootup, right? So "HPD" will not be high. detect() will say that
nothing is there. Since nothing is there, nobody will ever try to call
get_edid() nor will they ever try turning on the panel. ...and since
nobody turns on the panel HPD will never be high.

Now let's imagine that there is some rule to turn the panel on once at
bootup. Great, you'll see the panel at bootup. ...but then what
happens when you go through a modeset or suspend/resume or similar?
We'll turn the panel off as part of the modeset, HPD will go low, and
it will look like the panel is gone forever.

This is why detect() has to always say that an eDP panel is present.
If this isn't true the panel will never be turned on because we'll
never know it's there.


> > In theory one could try to conflate the two. Everyone keeps trying
>
> I agree here, they are not the same.
>
> > until they think about it more. Probably because the signal is named
> > HPD and everyone reads that as "hot plug detect". Worst. Name. Ever.
> > In any case, here lies dragons. Specifically if you conflate these two
> > concepts then when do you know to provide power to the display?
> > Remember, you can't detect the display until it's powered. ...but why
> > would you power it if you thought it wasn't there? You could power it
> > once at bootup, but then if someone turns the display off how will you
> > ever know that you can power it back on? It'll look like the display
> > was removed...
>
> But here's my question: if detect() tells whether the display is
> physically there, why do we need it?
>
> If the display is not hot-pluggable, then, as you say, it's always
> there, and detect() is unnecessary. The panel driver always assumes the
> panel is there and will power it up. So detect is not really needed.

Right. I conflated these two, sorry. Having detect() unimplemented and
having it always return true are the same thing and the DRM core
treats them the same as far as I'm aware.


> > It gets down to making sure things are powered. If the eDP controller
> > implements get_edid() then the eDP controller needs to know how to
> > power on the panel in response to that get_edid(). Remember, this is
> > eDP and we have to _always_ say the panel is there even when HPD
> > hasn't been asserted. See the above rant^H^H^H^H explanation. While
> > it's possible to have the eDP controller call down the bridge chain to
> > power the panel temporarily for get_edid() (early patches of mine did
> > that), in the end we decided it made more sense to have this driven by
> > the panel driver.
>
> I agree here, the panel driver has to drive the process. That's actually
> how I designed the old omapfb display subsystem (well, DP didn't exist
> then), everything originated from the display driver, not the crtc side.
>
> However, my argument is that someone, be it the display or the source
> driver, should offer detect() and get_edid(),

If you implement get_edid() I believe it won't be the end of the world
because the panel's version will be picked first. However, it feels
clearer to me to not implement it if it's not going to be used / won't
work for eDP.


> and afaics it makes sense
> for detect() to report whether the display is ready or not (usually HPD
> if it is connected, but could be via some other means).

I think detect() is actually harmful for eDP, as per my argument
above. If we detect the panel is "gone" then we'll turn off the power
to the panel. We'll never detect the panel again and we'll never again
have a reason to power the panel on.


> However, I have to say this is perhaps sidetracking this patch =). I can
> drop the comment in question from the description as it's somewhat
> irrelevant wrt. this patch.

Sounds good!

-Doug