[PATCH v3 0/3] Add support for errors recovery in the TI SN65DSI83 bridge driver

[PATCH v3 0/3] Add support for errors recovery in the TI SN65DSI83 bridge driver

[Herve Codina]

Hi,

Usually the TI SN65DSI83 recovers from error by itself but during ESD
tests, we have some cases where the TI SN65DSI83 didn't recover.

In order to handle those cases, this series adds support for a recovery
mechanism.

Compare to the previous iteration, this v3 series:
  - move reset_pipe() from the VC4 HDMI driver to an new atomic helper
  - Use this new helper

Best regards,
Hervé Codina

Changes v2 -> v3
  v2: https://lore.kernel.org/lkml/20241217143216.658461-1-herve.codina@bootlin.com/

  - Patch 1:
    No changes

  - Patch 2 (new in v3)
    Move reset_pipe() from VC4 HDMI driver to a new atomic helper

  - Patch 3
    Use the new drm_atomic_helper_reset_pipe()

  Patch removed in v3
    - Patch 2 in v2
      No more needed

Changes v1 -> v2
  v1: https://lore.kernel.org/lkml/20241024095539.1637280-1-herve.codina@bootlin.com/

  - Patch 1:
    Add 'Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>'
    Add 'Acked-by: Conor Dooley <conor.dooley@microchip.com>'

  - Patch 2 (new patch in v2)
    Introduce drm_atomic_helper_disable_connector()

  - Patch 3 (patch 2 in v1)
    Reset the output path instead of the full pipeline.
    Update and add more information related to the bridge in commit log.

Herve Codina (3):
  dt-bindings: display: bridge: sn65dsi83: Add interrupt
  drm/vc4: Move reset_pipe() to an atomic helper
  drm: bridge: ti-sn65dsi83: Add error recovery mechanism

 .../bindings/display/bridge/ti,sn65dsi83.yaml |   3 +
 drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 147 ++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c           |  41 +++++
 drivers/gpu/drm/vc4/vc4_hdmi.c                |  30 +---
 include/drm/drm_atomic_helper.h               |   2 +
 5 files changed, 194 insertions(+), 29 deletions(-)

-- 
2.47.1

[PATCH v3 1/3] dt-bindings: display: bridge: sn65dsi83: Add interrupt

[Herve Codina]

Both the TI SN65DSI83 and SN65DSI84 bridges have an IRQ pin to signal
errors using interrupt.

This interrupt is not documented in the binding.

Add the missing interrupts property.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index 48a97bb3e2e0..4505d2d83e0d 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -35,6 +35,9 @@ properties:
   vcc-supply:
     description: A 1.8V power supply (see regulator/regulator.yaml).
 
+  interrupts:
+    maxItems: 1
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
2.47.1

[PATCH v3 2/3] drm/vc4: Move reset_pipe() to an atomic helper

[Herve Codina]

reset_pipe() allows to reset the CRTC active outputs.

In order to use it from other drivers without code duplication, move it
to an atomic helper without any functional changes.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_hdmi.c      | 30 +--------------------
 include/drm/drm_atomic_helper.h     |  2 ++
 3 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8ed186ddaeaf..4225b814ea35 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3363,6 +3363,47 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_disable_all);
 
+/**
+ * drm_atomic_helper_reset_pipe - reset the active outputs of a CRTC
+ * @crtc: DRM CRTC
+ * @ctx: lock acquisition context
+ *
+ * Reset the active outputs by indicating that connectors have changed.
+ * This implies a reset of all active components available between the CRTC and
+ * connectors.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_atomic_helper_reset_pipe(struct drm_crtc *crtc,
+				 struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_atomic_state *state;
+	struct drm_crtc_state *crtc_state;
+	int ret;
+
+	state = drm_atomic_state_alloc(crtc->dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = ctx;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto out;
+	}
+
+	crtc_state->connectors_changed = true;
+
+	ret = drm_atomic_commit(state);
+out:
+	drm_atomic_state_put(state);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_reset_pipe);
+
 /**
  * drm_atomic_helper_shutdown - shutdown all CRTC
  * @dev: DRM device
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index e3818c48c9b8..19f6592a8cc5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -269,34 +269,6 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
 static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
 #endif
 
-static int reset_pipe(struct drm_crtc *crtc,
-			struct drm_modeset_acquire_ctx *ctx)
-{
-	struct drm_atomic_state *state;
-	struct drm_crtc_state *crtc_state;
-	int ret;
-
-	state = drm_atomic_state_alloc(crtc->dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = ctx;
-
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state)) {
-		ret = PTR_ERR(crtc_state);
-		goto out;
-	}
-
-	crtc_state->connectors_changed = true;
-
-	ret = drm_atomic_commit(state);
-out:
-	drm_atomic_state_put(state);
-
-	return ret;
-}
-
 static int vc4_hdmi_reset_link(struct drm_connector *connector,
 			       struct drm_modeset_acquire_ctx *ctx)
 {
@@ -375,7 +347,7 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector,
 	 * would be perfectly happy if were to just reconfigure
 	 * the SCDC settings on the fly.
 	 */
-	return reset_pipe(crtc, ctx);
+	return drm_atomic_helper_reset_pipe(crtc, ctx);
 }
 
 static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9aa0a05aa072..56eb6881cfb2 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -139,6 +139,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
 
 int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx);
+int drm_atomic_helper_reset_pipe(struct drm_crtc *crtc,
+				 struct drm_modeset_acquire_ctx *ctx);
 void drm_atomic_helper_shutdown(struct drm_device *dev);
 struct drm_atomic_state *
 drm_atomic_helper_duplicate_state(struct drm_device *dev,
-- 
2.47.1

[PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
from errors by itself. A full restart of the bridge is needed in those
cases to have the bridge output LVDS signals again.

Also, during tests, cases were observed where reading the status of the
bridge was not even possible. Indeed, in those cases, the bridge stops
to acknowledge I2C transactions. Only a full reset of the bridge (power
off/on) brings back the bridge to a functional state.

The TI SN65DSI83 has some error detection capabilities. Introduce an
error recovery mechanism based on this detection.

The errors detected are signaled through an interrupt. On system where
this interrupt is not available, the driver uses a polling monitoring
fallback to check for errors. When an error is present or when reading
the bridge status leads to an I2C failure, the recovery process is
launched.

Restarting the bridge needs to redo the initialization sequence. This
initialization sequence has to be done with the DSI data lanes driven in
LP11 state. In order to do that, the recovery process resets the whole
output path (i.e the path from the encoder to the connector) where the
bridge is located.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 ++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index e6264514bb3f..74bc05647436 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -35,9 +35,12 @@
 #include <linux/of_graph.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/timer.h>
+#include <linux/workqueue.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
@@ -147,6 +150,9 @@ struct sn65dsi83 {
 	struct regulator		*vcc;
 	bool				lvds_dual_link;
 	bool				lvds_dual_link_even_odd_swap;
+	bool				use_irq;
+	struct delayed_work		monitor_work;
+	struct work_struct		reset_work;
 };
 
 static const struct regmap_range sn65dsi83_readable_ranges[] = {
@@ -328,6 +334,111 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
 	return dsi_div - 1;
 }
 
+static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
+{
+	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
+	struct drm_device *dev = sn65dsi83->bridge.dev;
+	struct drm_connector_state *connector_state;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_connector *connector;
+	int err;
+
+	/*
+	 * Reset active outputs of the related CRTC.
+	 *
+	 * This way, drm core will reconfigure each components in the CRTC
+	 * outputs path. In our case, this will force the previous component to
+	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
+	 * bridge.
+	 *
+	 * Keep the lock during the whole operation to be atomic.
+	 */
+
+	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
+
+	state = drm_atomic_helper_duplicate_state(dev, &ctx);
+	if (IS_ERR(state)) {
+		err = PTR_ERR(state);
+		goto unlock;
+	}
+
+	state->acquire_ctx = &ctx;
+
+	connector = drm_atomic_get_old_connector_for_encoder(state,
+							     sn65dsi83->bridge.encoder);
+	if (!connector) {
+		err = -EINVAL;
+		goto unlock;
+	}
+
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state)) {
+		err = PTR_ERR(connector_state);
+		goto unlock;
+	}
+
+	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
+	if (err < 0)
+		goto unlock;
+
+unlock:
+	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
+	if (!IS_ERR(state))
+		drm_atomic_state_put(state);
+	return err;
+}
+
+static void sn65dsi83_reset_work(struct work_struct *ws)
+{
+	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
+	int ret;
+
+	dev_warn(ctx->dev, "reset the pipe\n");
+
+	/* Reset the pipe */
+	ret = sn65dsi83_reset_pipe(ctx);
+	if (ret) {
+		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
+		return;
+	}
+}
+
+static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
+{
+	unsigned int irq_stat;
+	int ret;
+
+	/*
+	 * Schedule a reset in case of:
+	 *  - the bridge doesn't answer
+	 *  - the bridge signals an error
+	 */
+
+	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
+	if (ret || irq_stat)
+		schedule_work(&ctx->reset_work);
+}
+
+static void sn65dsi83_monitor_work(struct work_struct *work)
+{
+	struct sn65dsi83 *ctx = container_of(to_delayed_work(work),
+					     struct sn65dsi83, monitor_work);
+
+	sn65dsi83_handle_errors(ctx);
+
+	schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
+}
+
+static void sn65dsi83_monitor_start(struct sn65dsi83 *ctx)
+{
+	schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
+}
+
+static void sn65dsi83_monitor_stop(struct sn65dsi83 *ctx)
+{
+	cancel_delayed_work_sync(&ctx->monitor_work);
+}
+
 static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 					struct drm_bridge_state *old_bridge_state)
 {
@@ -516,6 +627,15 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	if (pval)
 		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
+
+	if (ctx->use_irq) {
+		/* Enable irq to detect errors */
+		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, REG_IRQ_GLOBAL_IRQ_EN);
+		regmap_write(ctx->regmap, REG_IRQ_EN, 0xff);
+	} else {
+		/* Use the polling task */
+		sn65dsi83_monitor_start(ctx);
+	}
 }
 
 static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
@@ -524,6 +644,15 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	int ret;
 
+	if (ctx->use_irq) {
+		/* Disable irq */
+		regmap_write(ctx->regmap, REG_IRQ_EN, 0x0);
+		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, 0x0);
+	} else {
+		/* Stop the polling task */
+		sn65dsi83_monitor_stop(ctx);
+	}
+
 	/* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */
 	gpiod_set_value_cansleep(ctx->enable_gpio, 0);
 	usleep_range(10000, 11000);
@@ -681,6 +810,14 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
 	return 0;
 }
 
+static irqreturn_t sn65dsi83_irq(int irq, void *data)
+{
+	struct sn65dsi83 *ctx = data;
+
+	sn65dsi83_handle_errors(ctx);
+	return IRQ_HANDLED;
+}
+
 static int sn65dsi83_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -698,6 +835,8 @@ static int sn65dsi83_probe(struct i2c_client *client)
 		return ret;
 
 	ctx->dev = dev;
+	INIT_WORK(&ctx->reset_work, sn65dsi83_reset_work);
+	INIT_DELAYED_WORK(&ctx->monitor_work, sn65dsi83_monitor_work);
 
 	if (dev->of_node) {
 		model = (enum sn65dsi83_model)(uintptr_t)
@@ -722,6 +861,14 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	if (IS_ERR(ctx->regmap))
 		return dev_err_probe(dev, PTR_ERR(ctx->regmap), "failed to get regmap\n");
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(ctx->dev, client->irq, NULL, sn65dsi83_irq,
+						IRQF_ONESHOT, dev_name(ctx->dev), ctx);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request irq\n");
+		ctx->use_irq = true;
+	}
+
 	dev_set_drvdata(dev, ctx);
 	i2c_set_clientdata(client, ctx);
 
-- 
2.47.1

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Alexander Stein]

Hi Herve,

Am Mittwoch, 8. Januar 2025, 11:19:02 CET schrieb Herve Codina:
> In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
> from errors by itself. A full restart of the bridge is needed in those
> cases to have the bridge output LVDS signals again.
> 
> Also, during tests, cases were observed where reading the status of the
> bridge was not even possible. Indeed, in those cases, the bridge stops
> to acknowledge I2C transactions. Only a full reset of the bridge (power
> off/on) brings back the bridge to a functional state.
> 
> The TI SN65DSI83 has some error detection capabilities. Introduce an
> error recovery mechanism based on this detection.
> 
> The errors detected are signaled through an interrupt. On system where
> this interrupt is not available, the driver uses a polling monitoring
> fallback to check for errors. When an error is present or when reading
> the bridge status leads to an I2C failure, the recovery process is
> launched.
> 
> Restarting the bridge needs to redo the initialization sequence. This
> initialization sequence has to be done with the DSI data lanes driven in
> LP11 state. In order to do that, the recovery process resets the whole
> output path (i.e the path from the encoder to the connector) where the
> bridge is located.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 ++++++++++++++++++++++++++
>  1 file changed, 147 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index e6264514bb3f..74bc05647436 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -35,9 +35,12 @@
>  #include <linux/of_graph.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/timer.h>
> +#include <linux/workqueue.h>
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */

Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?

>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
> @@ -147,6 +150,9 @@ struct sn65dsi83 {
>  	struct regulator		*vcc;
>  	bool				lvds_dual_link;
>  	bool				lvds_dual_link_even_odd_swap;
> +	bool				use_irq;
> +	struct delayed_work		monitor_work;
> +	struct work_struct		reset_work;

Can you please rebase? You are missing commit d2b8c6d549570
("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties")

>  };
>  
>  static const struct regmap_range sn65dsi83_readable_ranges[] = {
> @@ -328,6 +334,111 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
>  	return dsi_div - 1;
>  }
>  
> +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> +{
> +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> +	struct drm_device *dev = sn65dsi83->bridge.dev;
> +	struct drm_connector_state *connector_state;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector *connector;
> +	int err;
> +
> +	/*
> +	 * Reset active outputs of the related CRTC.
> +	 *
> +	 * This way, drm core will reconfigure each components in the CRTC
> +	 * outputs path. In our case, this will force the previous component to
> +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> +	 * bridge.
> +	 *
> +	 * Keep the lock during the whole operation to be atomic.
> +	 */
> +
> +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> +
> +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +	if (IS_ERR(state)) {
> +		err = PTR_ERR(state);
> +		goto unlock;
> +	}
> +
> +	state->acquire_ctx = &ctx;
> +
> +	connector = drm_atomic_get_old_connector_for_encoder(state,
> +							     sn65dsi83->bridge.encoder);
> +	if (!connector) {
> +		err = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	connector_state = drm_atomic_get_connector_state(state, connector);
> +	if (IS_ERR(connector_state)) {
> +		err = PTR_ERR(connector_state);
> +		goto unlock;
> +	}
> +
> +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> +	if (err < 0)
> +		goto unlock;
> +
> +unlock:
> +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> +	if (!IS_ERR(state))
> +		drm_atomic_state_put(state);
> +	return err;
> +}
> +
> +static void sn65dsi83_reset_work(struct work_struct *ws)
> +{
> +	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
> +	int ret;
> +
> +	dev_warn(ctx->dev, "reset the pipe\n");
> +
> +	/* Reset the pipe */
> +	ret = sn65dsi83_reset_pipe(ctx);
> +	if (ret) {
> +		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> +		return;
> +	}
> +}
> +
> +static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
> +{
> +	unsigned int irq_stat;
> +	int ret;
> +
> +	/*
> +	 * Schedule a reset in case of:
> +	 *  - the bridge doesn't answer
> +	 *  - the bridge signals an error
> +	 */
> +
> +	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
> +	if (ret || irq_stat)
> +		schedule_work(&ctx->reset_work);

Shouldn't you clear the error bits as well?

Best regards,
Alexander

> +}
> +
> +static void sn65dsi83_monitor_work(struct work_struct *work)
> +{
> +	struct sn65dsi83 *ctx = container_of(to_delayed_work(work),
> +					     struct sn65dsi83, monitor_work);
> +
> +	sn65dsi83_handle_errors(ctx);
> +
> +	schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
> +}
> +
> +static void sn65dsi83_monitor_start(struct sn65dsi83 *ctx)
> +{
> +	schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
> +}
> +
> +static void sn65dsi83_monitor_stop(struct sn65dsi83 *ctx)
> +{
> +	cancel_delayed_work_sync(&ctx->monitor_work);
> +}
> +
>  static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  					struct drm_bridge_state *old_bridge_state)
>  {
> @@ -516,6 +627,15 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>  	if (pval)
>  		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
> +
> +	if (ctx->use_irq) {
> +		/* Enable irq to detect errors */
> +		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, REG_IRQ_GLOBAL_IRQ_EN);
> +		regmap_write(ctx->regmap, REG_IRQ_EN, 0xff);
> +	} else {
> +		/* Use the polling task */
> +		sn65dsi83_monitor_start(ctx);
> +	}
>  }
>  
>  static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
> @@ -524,6 +644,15 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>  	int ret;
>  
> +	if (ctx->use_irq) {
> +		/* Disable irq */
> +		regmap_write(ctx->regmap, REG_IRQ_EN, 0x0);
> +		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, 0x0);
> +	} else {
> +		/* Stop the polling task */
> +		sn65dsi83_monitor_stop(ctx);
> +	}
> +
>  	/* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */
>  	gpiod_set_value_cansleep(ctx->enable_gpio, 0);
>  	usleep_range(10000, 11000);
> @@ -681,6 +810,14 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
>  	return 0;
>  }
>  
> +static irqreturn_t sn65dsi83_irq(int irq, void *data)
> +{
> +	struct sn65dsi83 *ctx = data;
> +
> +	sn65dsi83_handle_errors(ctx);
> +	return IRQ_HANDLED;
> +}
> +
>  static int sn65dsi83_probe(struct i2c_client *client)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -698,6 +835,8 @@ static int sn65dsi83_probe(struct i2c_client *client)
>  		return ret;
>  
>  	ctx->dev = dev;
> +	INIT_WORK(&ctx->reset_work, sn65dsi83_reset_work);
> +	INIT_DELAYED_WORK(&ctx->monitor_work, sn65dsi83_monitor_work);
>  
>  	if (dev->of_node) {
>  		model = (enum sn65dsi83_model)(uintptr_t)
> @@ -722,6 +861,14 @@ static int sn65dsi83_probe(struct i2c_client *client)
>  	if (IS_ERR(ctx->regmap))
>  		return dev_err_probe(dev, PTR_ERR(ctx->regmap), "failed to get regmap\n");
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(ctx->dev, client->irq, NULL, sn65dsi83_irq,
> +						IRQF_ONESHOT, dev_name(ctx->dev), ctx);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to request irq\n");
> +		ctx->use_irq = true;
> +	}
> +
>  	dev_set_drvdata(dev, ctx);
>  	i2c_set_clientdata(client, ctx);
>  
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

Hi Alexander,

On Wed, 08 Jan 2025 11:54:49 +0100
Alexander Stein <alexander.stein@ew.tq-group.com> wrote:

[...]
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */  
> 
> Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?

Yes indeed. I will change that in the next iteration.

> 
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> > @@ -147,6 +150,9 @@ struct sn65dsi83 {
> >  	struct regulator		*vcc;
> >  	bool				lvds_dual_link;
> >  	bool				lvds_dual_link_even_odd_swap;
> > +	bool				use_irq;
> > +	struct delayed_work		monitor_work;
> > +	struct work_struct		reset_work;  
> 
> Can you please rebase? You are missing commit d2b8c6d549570
> ("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties")

Sure, I will rebase.

[...]
> > +static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
> > +{
> > +	unsigned int irq_stat;
> > +	int ret;
> > +
> > +	/*
> > +	 * Schedule a reset in case of:
> > +	 *  - the bridge doesn't answer
> > +	 *  - the bridge signals an error
> > +	 */
> > +
> > +	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
> > +	if (ret || irq_stat)
> > +		schedule_work(&ctx->reset_work);  
> 
> Shouldn't you clear the error bits as well?

Thanks for pointing that.

I can clear the error bit but further more, I probably need to simply
disable the interrupt.

In some cases, we observed i2c access failure. In that cases clearing error
bits is simply not possible.

To avoid some possible interrupt storms (the chip detect a failure, set its
interrupt line but could be not accessible anymore), the best thing to do
is to disable the interrupt line here, let the reset work to do its job
performing a full reset of the device and re-enabling the interrupt line
when needed, probably in sn65dsi83_atomic_enable().

What do you think about that?

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

Hi Alexander,

On Wed, 8 Jan 2025 18:44:42 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_bridge.h>
> > > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */    
> > 
> > Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?  
> 
> Yes indeed. I will change that in the next iteration.

I tried to add '#include <drm/drm_drv.h>' in include/drm/drm_modeset_lock.h
but it breaks the build in several places.

First, I cannot add it at the begining of drm_modeset_lock.h.
The inclusion path leads to:
  drm_modeset_lock.h
    drm/drm_drv.h
      drm/drm_device.h
        drm/drm_mode_config.h
           struct drm_mode_config definition

The struct drm_mode_config needs the struct drm_modeset_lock defined.
struct drm_modeset_lock is defined in drm_modeset_lock.h.

Even if I don't like to add include files in the middle of a header filer,
I tried to include drm/drm_drv.h just before the DRM_MODESET_LOCK_ALL_BEGIN()
definition in drm_modeset_lock.h.

This also breaks the build in several places. For instance:
  In file included from ./include/drm/drm_modeset_lock.h:162,
                   from ./include/drm/drm_mode_config.h:32,
                   from ./include/drm/drm_device.h:9,
                   from drivers/gpu/drm/drm_dumb_buffers.c:26:
  ./include/drm/drm_drv.h: In function ‘drm_core_check_all_features’:
  ./include/drm/drm_drv.h:522:28: error: invalid use of undefined type ‘const struct drm_device’
    522 |         u32 supported = dev->driver->driver_features & dev->driver_features;
        |                            ^~

I stop here, go back and choose to keep '#include <drm/drm_drv.h>' in ti-sn65dsi83.c

Is that ok for you?

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Alexander Stein]

Hi Herve,

Am Donnerstag, 9. Januar 2025, 11:38:34 CET schrieb Herve Codina:
> Hi Alexander,
> 
> On Wed, 8 Jan 2025 18:44:42 +0100
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > > >  #include <drm/drm_atomic_helper.h>
> > > >  #include <drm/drm_bridge.h>
> > > > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */    
> > > 
> > > Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?  
> > 
> > Yes indeed. I will change that in the next iteration.
> 
> I tried to add '#include <drm/drm_drv.h>' in include/drm/drm_modeset_lock.h
> but it breaks the build in several places.
> 
> First, I cannot add it at the begining of drm_modeset_lock.h.
> The inclusion path leads to:
>   drm_modeset_lock.h
>     drm/drm_drv.h
>       drm/drm_device.h
>         drm/drm_mode_config.h
>            struct drm_mode_config definition
> 
> The struct drm_mode_config needs the struct drm_modeset_lock defined.
> struct drm_modeset_lock is defined in drm_modeset_lock.h.
> 
> Even if I don't like to add include files in the middle of a header filer,
> I tried to include drm/drm_drv.h just before the DRM_MODESET_LOCK_ALL_BEGIN()
> definition in drm_modeset_lock.h.
> 
> This also breaks the build in several places. For instance:
>   In file included from ./include/drm/drm_modeset_lock.h:162,
>                    from ./include/drm/drm_mode_config.h:32,
>                    from ./include/drm/drm_device.h:9,
>                    from drivers/gpu/drm/drm_dumb_buffers.c:26:
>   ./include/drm/drm_drv.h: In function ‘drm_core_check_all_features’:
>   ./include/drm/drm_drv.h:522:28: error: invalid use of undefined type ‘const struct drm_device’
>     522 |         u32 supported = dev->driver->driver_features & dev->driver_features;
>         |                            ^~
> 
> I stop here, go back and choose to keep '#include <drm/drm_drv.h>' in ti-sn65dsi83.c
> 
> Is that ok for you?

Mh, okay. It's up to the DRM maintainer to decide what to do. I just
pointed out it looks weird to me.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Alexander Stein]

Hi Herve,

Am Mittwoch, 8. Januar 2025, 18:44:42 CET schrieb Herve Codina:
> Hi Alexander,
> 
> On Wed, 08 Jan 2025 11:54:49 +0100
> Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
> 
> [...]
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_bridge.h>
> > > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */  
> > 
> > Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?
> 
> Yes indeed. I will change that in the next iteration.
> 
> > 
> > >  #include <drm/drm_mipi_dsi.h>
> > >  #include <drm/drm_of.h>
> > >  #include <drm/drm_panel.h>
> > > @@ -147,6 +150,9 @@ struct sn65dsi83 {
> > >  	struct regulator		*vcc;
> > >  	bool				lvds_dual_link;
> > >  	bool				lvds_dual_link_even_odd_swap;
> > > +	bool				use_irq;
> > > +	struct delayed_work		monitor_work;
> > > +	struct work_struct		reset_work;  
> > 
> > Can you please rebase? You are missing commit d2b8c6d549570
> > ("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties")
> 
> Sure, I will rebase.
> 
> [...]
> > > +static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
> > > +{
> > > +	unsigned int irq_stat;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Schedule a reset in case of:
> > > +	 *  - the bridge doesn't answer
> > > +	 *  - the bridge signals an error
> > > +	 */
> > > +
> > > +	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
> > > +	if (ret || irq_stat)
> > > +		schedule_work(&ctx->reset_work);  
> > 
> > Shouldn't you clear the error bits as well?
> 
> Thanks for pointing that.
> 
> I can clear the error bit but further more, I probably need to simply
> disable the interrupt.
> 
> In some cases, we observed i2c access failure. In that cases clearing error
> bits is simply not possible.
> 
> To avoid some possible interrupt storms (the chip detect a failure, set its
> interrupt line but could be not accessible anymore), the best thing to do
> is to disable the interrupt line here, let the reset work to do its job
> performing a full reset of the device and re-enabling the interrupt line
> when needed, probably in sn65dsi83_atomic_enable().
> 
> What do you think about that?

As I read the datasheet this is a active-high level interrupt, so as
long as some enabled IRQs are pending the signal will stay high.
There are 3 notes in section 9.1.3. IRQ usage that in various situations
IRQ bits may be set/pending and have to be cleared.
At least clear the interrupts before enabling it again to be on the
safe side.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

On Thu, 09 Jan 2025 11:49:13 +0100
Alexander Stein <alexander.stein@ew.tq-group.com> wrote:

> Hi Herve,
> 
> Am Mittwoch, 8. Januar 2025, 18:44:42 CET schrieb Herve Codina:
> > Hi Alexander,
> > 
> > On Wed, 08 Jan 2025 11:54:49 +0100
> > Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
> > 
> > [...]  
> > > >  #include <drm/drm_atomic_helper.h>
> > > >  #include <drm/drm_bridge.h>
> > > > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */    
> > > 
> > > Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?  
> > 
> > Yes indeed. I will change that in the next iteration.
> >   
> > >   
> > > >  #include <drm/drm_mipi_dsi.h>
> > > >  #include <drm/drm_of.h>
> > > >  #include <drm/drm_panel.h>
> > > > @@ -147,6 +150,9 @@ struct sn65dsi83 {
> > > >  	struct regulator		*vcc;
> > > >  	bool				lvds_dual_link;
> > > >  	bool				lvds_dual_link_even_odd_swap;
> > > > +	bool				use_irq;
> > > > +	struct delayed_work		monitor_work;
> > > > +	struct work_struct		reset_work;    
> > > 
> > > Can you please rebase? You are missing commit d2b8c6d549570
> > > ("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties")  
> > 
> > Sure, I will rebase.
> > 
> > [...]  
> > > > +static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
> > > > +{
> > > > +	unsigned int irq_stat;
> > > > +	int ret;
> > > > +
> > > > +	/*
> > > > +	 * Schedule a reset in case of:
> > > > +	 *  - the bridge doesn't answer
> > > > +	 *  - the bridge signals an error
> > > > +	 */
> > > > +
> > > > +	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
> > > > +	if (ret || irq_stat)
> > > > +		schedule_work(&ctx->reset_work);    
> > > 
> > > Shouldn't you clear the error bits as well?  
> > 
> > Thanks for pointing that.
> > 
> > I can clear the error bit but further more, I probably need to simply
> > disable the interrupt.
> > 
> > In some cases, we observed i2c access failure. In that cases clearing error
> > bits is simply not possible.
> > 
> > To avoid some possible interrupt storms (the chip detect a failure, set its
> > interrupt line but could be not accessible anymore), the best thing to do
> > is to disable the interrupt line here, let the reset work to do its job
> > performing a full reset of the device and re-enabling the interrupt line
> > when needed, probably in sn65dsi83_atomic_enable().
> > 
> > What do you think about that?  
> 
> As I read the datasheet this is a active-high level interrupt, so as
> long as some enabled IRQs are pending the signal will stay high.
> There are 3 notes in section 9.1.3. IRQ usage that in various situations
> IRQ bits may be set/pending and have to be cleared.
> At least clear the interrupts before enabling it again to be on the
> safe side.

Ok, I finally disable the irq just before the schedule_work(&ctx->reset_work)
call and re-enable it after the sn65dsi83_reset_pipe(ctx) call done
in sn65dsi83_reset_work().

During sn65dsi83_reset_pipe(), sn65dsi83_atomic_disable() and
sn65dsi83_atomic_enable() are called and so interrupts are cleared.

This modification will be part of the next iteration.

Best regards,
Hervé

Re: [PATCH v3 2/3] drm/vc4: Move reset_pipe() to an atomic helper

[Maxime Ripard]

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

Hi,

On Wed, Jan 08, 2025 at 11:19:01AM +0100, Herve Codina wrote:
> reset_pipe() allows to reset the CRTC active outputs.
> 
> In order to use it from other drivers without code duplication, move it
> to an atomic helper without any functional changes.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_hdmi.c      | 30 +--------------------
>  include/drm/drm_atomic_helper.h     |  2 ++
>  3 files changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8ed186ddaeaf..4225b814ea35 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3363,6 +3363,47 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>  
> +/**
> + * drm_atomic_helper_reset_pipe - reset the active outputs of a CRTC
> + * @crtc: DRM CRTC
> + * @ctx: lock acquisition context
> + *
> + * Reset the active outputs by indicating that connectors have changed.
> + * This implies a reset of all active components available between the CRTC and
> + * connectors.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_atomic_helper_reset_pipe(struct drm_crtc *crtc,
> +				 struct drm_modeset_acquire_ctx *ctx)

We should probably name it reset_crtc to make it more consistent with
the other helpers.

> +{
> +	struct drm_atomic_state *state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret;
> +
> +	state = drm_atomic_state_alloc(crtc->dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = ctx;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		goto out;
> +	}
> +
> +	crtc_state->connectors_changed = true;
> +
> +	ret = drm_atomic_commit(state);
> +out:
> +	drm_atomic_state_put(state);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_reset_pipe);
> +
>  /**
>   * drm_atomic_helper_shutdown - shutdown all CRTC
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index e3818c48c9b8..19f6592a8cc5 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -269,34 +269,6 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
>  static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
>  #endif
>  
> -static int reset_pipe(struct drm_crtc *crtc,
> -			struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_atomic_state *state;
> -	struct drm_crtc_state *crtc_state;
> -	int ret;
> -
> -	state = drm_atomic_state_alloc(crtc->dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> -	state->acquire_ctx = ctx;
> -
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state)) {
> -		ret = PTR_ERR(crtc_state);
> -		goto out;
> -	}
> -
> -	crtc_state->connectors_changed = true;
> -
> -	ret = drm_atomic_commit(state);
> -out:
> -	drm_atomic_state_put(state);
> -
> -	return ret;
> -}
> -
>  static int vc4_hdmi_reset_link(struct drm_connector *connector,
>  			       struct drm_modeset_acquire_ctx *ctx)
>  {
> @@ -375,7 +347,7 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector,
>  	 * would be perfectly happy if were to just reconfigure
>  	 * the SCDC settings on the fly.
>  	 */
> -	return reset_pipe(crtc, ctx);
> +	return drm_atomic_helper_reset_pipe(crtc, ctx);
>  }

And that part should be in another patch

Maxime

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

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Maxime Ripard]

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

On Wed, Jan 08, 2025 at 11:19:02AM +0100, Herve Codina wrote:
> In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
> from errors by itself. A full restart of the bridge is needed in those
> cases to have the bridge output LVDS signals again.
> 
> Also, during tests, cases were observed where reading the status of the
> bridge was not even possible. Indeed, in those cases, the bridge stops
> to acknowledge I2C transactions. Only a full reset of the bridge (power
> off/on) brings back the bridge to a functional state.
> 
> The TI SN65DSI83 has some error detection capabilities. Introduce an
> error recovery mechanism based on this detection.
> 
> The errors detected are signaled through an interrupt. On system where
> this interrupt is not available, the driver uses a polling monitoring
> fallback to check for errors. When an error is present or when reading
> the bridge status leads to an I2C failure, the recovery process is
> launched.
> 
> Restarting the bridge needs to redo the initialization sequence. This
> initialization sequence has to be done with the DSI data lanes driven in
> LP11 state. In order to do that, the recovery process resets the whole
> output path (i.e the path from the encoder to the connector) where the
> bridge is located.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 ++++++++++++++++++++++++++
>  1 file changed, 147 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index e6264514bb3f..74bc05647436 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -35,9 +35,12 @@
>  #include <linux/of_graph.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/timer.h>
> +#include <linux/workqueue.h>
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
> @@ -147,6 +150,9 @@ struct sn65dsi83 {
>  	struct regulator		*vcc;
>  	bool				lvds_dual_link;
>  	bool				lvds_dual_link_even_odd_swap;
> +	bool				use_irq;
> +	struct delayed_work		monitor_work;
> +	struct work_struct		reset_work;
>  };
>  
>  static const struct regmap_range sn65dsi83_readable_ranges[] = {
> @@ -328,6 +334,111 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
>  	return dsi_div - 1;
>  }
>  
> +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> +{
> +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> +	struct drm_device *dev = sn65dsi83->bridge.dev;
> +	struct drm_connector_state *connector_state;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector *connector;
> +	int err;
> +
> +	/*
> +	 * Reset active outputs of the related CRTC.
> +	 *
> +	 * This way, drm core will reconfigure each components in the CRTC
> +	 * outputs path. In our case, this will force the previous component to
> +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> +	 * bridge.
> +	 *
> +	 * Keep the lock during the whole operation to be atomic.
> +	 */
> +
> +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> +
> +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +	if (IS_ERR(state)) {
> +		err = PTR_ERR(state);
> +		goto unlock;
> +	}

No, you must not allocate a new state for this, you need to reuse the
existing state. You'll find it in bridge->base.state->state.

> +	state->acquire_ctx = &ctx;
> +
> +	connector = drm_atomic_get_old_connector_for_encoder(state,
> +							     sn65dsi83->bridge.encoder);
> +	if (!connector) {
> +		err = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	connector_state = drm_atomic_get_connector_state(state, connector);
> +	if (IS_ERR(connector_state)) {
> +		err = PTR_ERR(connector_state);
> +		goto unlock;
> +	}
> +
> +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> +	if (err < 0)
> +		goto unlock;

And you'll find the crtc in bridge->encoder->crtc.

Maxime

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

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

Hi Maxime,

On Tue, 14 Jan 2025 08:40:51 +0100
Maxime Ripard <mripard@kernel.org> wrote:

...

> >  
> > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > +{
> > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_connector *connector;
> > +	int err;
> > +
> > +	/*
> > +	 * Reset active outputs of the related CRTC.
> > +	 *
> > +	 * This way, drm core will reconfigure each components in the CRTC
> > +	 * outputs path. In our case, this will force the previous component to
> > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > +	 * bridge.
> > +	 *
> > +	 * Keep the lock during the whole operation to be atomic.
> > +	 */
> > +
> > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > +
> > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > +	if (IS_ERR(state)) {
> > +		err = PTR_ERR(state);
> > +		goto unlock;
> > +	}  
> 
> No, you must not allocate a new state for this, you need to reuse the
> existing state. You'll find it in bridge->base.state->state.

Thanks for pointing that. I didn't know about bridge->base.state->state.

I will use that if using the state is still relevant (see next comment).

> 
> > +	state->acquire_ctx = &ctx;
> > +
> > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > +							     sn65dsi83->bridge.encoder);
> > +	if (!connector) {
> > +		err = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > +	if (IS_ERR(connector_state)) {
> > +		err = PTR_ERR(connector_state);
> > +		goto unlock;
> > +	}
> > +
> > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > +	if (err < 0)
> > +		goto unlock;  
> 
> And you'll find the crtc in bridge->encoder->crtc.

I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
field available in this structure should not be used by atomic drivers. They
should rely on &drm_connector_state.crtc.

In my case, I have the feeling that I should get the ctrc from the current
state (i.e. bridge->base.state->state) using the sequence provided in this
current patch:
  Retrieve the connector with drm_atomic_get_old_connector_for_encoder()
  Retrieve the connector state with drm_atomic_get_connector_state()

but you pointed out the bridge->encoder->crtc field.

Should I use this field or use the &drm_connector_state.crtc with the drm
connector state retrieved from bridge->base.state->state using the proposed
sequence?

[1] https://elixir.bootlin.com/linux/v6.13-rc1/source/include/drm/drm_encoder.h#L180

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

Hi Maxime,

On Tue, 14 Jan 2025 13:54:56 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Maxime,
> 
> On Tue, 14 Jan 2025 08:40:51 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> ...
> 
> > >  
> > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > +{
> > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > +	struct drm_connector_state *connector_state;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_connector *connector;
> > > +	int err;
> > > +
> > > +	/*
> > > +	 * Reset active outputs of the related CRTC.
> > > +	 *
> > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > +	 * outputs path. In our case, this will force the previous component to
> > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > +	 * bridge.
> > > +	 *
> > > +	 * Keep the lock during the whole operation to be atomic.
> > > +	 */
> > > +
> > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > +
> > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > +	if (IS_ERR(state)) {
> > > +		err = PTR_ERR(state);
> > > +		goto unlock;
> > > +	}    
> > 
> > No, you must not allocate a new state for this, you need to reuse the
> > existing state. You'll find it in bridge->base.state->state.  
> 
> Thanks for pointing that. I didn't know about bridge->base.state->state.
> 
> I will use that if using the state is still relevant (see next comment).
> 
> >   
> > > +	state->acquire_ctx = &ctx;
> > > +
> > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > +							     sn65dsi83->bridge.encoder);
> > > +	if (!connector) {
> > > +		err = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > +	if (IS_ERR(connector_state)) {
> > > +		err = PTR_ERR(connector_state);
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > +	if (err < 0)
> > > +		goto unlock;    
> > 
> > And you'll find the crtc in bridge->encoder->crtc.  
> 
> I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> field available in this structure should not be used by atomic drivers. They
> should rely on &drm_connector_state.crtc.
> 
> In my case, I have the feeling that I should get the ctrc from the current
> state (i.e. bridge->base.state->state) using the sequence provided in this
> current patch:
>   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()
>   Retrieve the connector state with drm_atomic_get_connector_state()
> 
> but you pointed out the bridge->encoder->crtc field.
> 
> Should I use this field or use the &drm_connector_state.crtc with the drm
> connector state retrieved from bridge->base.state->state using the proposed
> sequence?
> 
> [1] https://elixir.bootlin.com/linux/v6.13-rc1/source/include/drm/drm_encoder.h#L180
> 

I did some test and I cannot use bridge->base.state->state to retrieve the
CRTC from the current state because this last state field is not set in my
case (i.e. NULL pointer).

So, in the next iteration, I will use directly bridge->encoder->crtc to get
the CRTC.

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Maxime Ripard]

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

On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:
> Hi Maxime,
> 
> On Tue, 14 Jan 2025 08:40:51 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> ...
> 
> > >  
> > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > +{
> > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > +	struct drm_connector_state *connector_state;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_connector *connector;
> > > +	int err;
> > > +
> > > +	/*
> > > +	 * Reset active outputs of the related CRTC.
> > > +	 *
> > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > +	 * outputs path. In our case, this will force the previous component to
> > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > +	 * bridge.
> > > +	 *
> > > +	 * Keep the lock during the whole operation to be atomic.
> > > +	 */
> > > +
> > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > +
> > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > +	if (IS_ERR(state)) {
> > > +		err = PTR_ERR(state);
> > > +		goto unlock;
> > > +	}  
> > 
> > No, you must not allocate a new state for this, you need to reuse the
> > existing state. You'll find it in bridge->base.state->state.
> 
> Thanks for pointing that. I didn't know about bridge->base.state->state.
> 
> I will use that if using the state is still relevant (see next comment).
> 
> > 
> > > +	state->acquire_ctx = &ctx;
> > > +
> > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > +							     sn65dsi83->bridge.encoder);
> > > +	if (!connector) {
> > > +		err = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > +	if (IS_ERR(connector_state)) {
> > > +		err = PTR_ERR(connector_state);
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > +	if (err < 0)
> > > +		goto unlock;  
> > 
> > And you'll find the crtc in bridge->encoder->crtc.
> 
> I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> field available in this structure should not be used by atomic drivers. They
> should rely on &drm_connector_state.crtc.

You're right, it's deprecated but used by most bridges anyway.

I made a series of changes after reviewing your series to address some
issues with the current bridge API, most notably

https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/

> In my case, I have the feeling that I should get the ctrc from the current
> state (i.e. bridge->base.state->state) using the sequence provided in this
> current patch:
>   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()

Retrieving the old connector makes no sense though. It's the connector
that was formerly associated with your encoder. It might work, it might
not, it's not what you're looking for.

>   Retrieve the connector state with drm_atomic_get_connector_state()

drm_atomic_get_connector_state will allocate and pull the connector
state into the drm_atomic_state, even if it wasn't part of it before, so
it's not great. And you don't need it in the first place, you only need
the current active CRTC.

> but you pointed out the bridge->encoder->crtc field.
>
> Should I use this field or use the &drm_connector_state.crtc with the drm
> connector state retrieved from bridge->base.state->state using the proposed
> sequence?

Having access to the connector isn't really easy either. Hopefully that
patch above should help there.

Maxime

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

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

Hi Maxime,

On Thu, 16 Jan 2025 09:38:45 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:
> > Hi Maxime,
> > 
> > On Tue, 14 Jan 2025 08:40:51 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> > 
> > ...
> >   
> > > >  
> > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > +{
> > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > +	struct drm_connector_state *connector_state;
> > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > +	struct drm_connector *connector;
> > > > +	int err;
> > > > +
> > > > +	/*
> > > > +	 * Reset active outputs of the related CRTC.
> > > > +	 *
> > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > +	 * outputs path. In our case, this will force the previous component to
> > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > +	 * bridge.
> > > > +	 *
> > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > +	 */
> > > > +
> > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > +
> > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > +	if (IS_ERR(state)) {
> > > > +		err = PTR_ERR(state);
> > > > +		goto unlock;
> > > > +	}    
> > > 
> > > No, you must not allocate a new state for this, you need to reuse the
> > > existing state. You'll find it in bridge->base.state->state.  
> > 
> > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > 
> > I will use that if using the state is still relevant (see next comment).
> >   
> > >   
> > > > +	state->acquire_ctx = &ctx;
> > > > +
> > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > +							     sn65dsi83->bridge.encoder);
> > > > +	if (!connector) {
> > > > +		err = -EINVAL;
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > +	if (IS_ERR(connector_state)) {
> > > > +		err = PTR_ERR(connector_state);
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > +	if (err < 0)
> > > > +		goto unlock;    
> > > 
> > > And you'll find the crtc in bridge->encoder->crtc.  
> > 
> > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > field available in this structure should not be used by atomic drivers. They
> > should rely on &drm_connector_state.crtc.  
> 
> You're right, it's deprecated but used by most bridges anyway.
> 
> I made a series of changes after reviewing your series to address some
> issues with the current bridge API, most notably
> 
> https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/

Thanks for pointing that, indeed, it clarify many things!

> 
> > In my case, I have the feeling that I should get the ctrc from the current
> > state (i.e. bridge->base.state->state) using the sequence provided in this
> > current patch:
> >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()  
> 
> Retrieving the old connector makes no sense though. It's the connector
> that was formerly associated with your encoder. It might work, it might
> not, it's not what you're looking for.
> 
> >   Retrieve the connector state with drm_atomic_get_connector_state()  
> 
> drm_atomic_get_connector_state will allocate and pull the connector
> state into the drm_atomic_state, even if it wasn't part of it before, so
> it's not great. And you don't need it in the first place, you only need
> the current active CRTC.

Yes, I agree with that, I only need the active CRTC.

I tried to get the current atomic_state from:
  1) bridge->base.state->state
  2) drm_bridge_state->base.state

In both cases, it is NULL. Looking at Sima's reply in your series
explained that:
  https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/

If I understood correctly those pointers are explicitly cleared.

So, with all of that, either:
  a) I wait for your series to be applied in order to use your the crtc field from
     drm_bridge_state added by:
       https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
  b) I use the old school bridge->encoder->crtc for the moment

Do you mind if I use the bridge->encoder->crtc way for the next iteration of
my series?

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Maxime Ripard]

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

Hi,

On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:
> Hi Maxime,
> 
> On Thu, 16 Jan 2025 09:38:45 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:
> > > Hi Maxime,
> > > 
> > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > > 
> > > ...
> > >   
> > > > >  
> > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > +{
> > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > +	struct drm_connector_state *connector_state;
> > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > +	struct drm_connector *connector;
> > > > > +	int err;
> > > > > +
> > > > > +	/*
> > > > > +	 * Reset active outputs of the related CRTC.
> > > > > +	 *
> > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > +	 * outputs path. In our case, this will force the previous component to
> > > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > > +	 * bridge.
> > > > > +	 *
> > > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > > +	 */
> > > > > +
> > > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > +
> > > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > +	if (IS_ERR(state)) {
> > > > > +		err = PTR_ERR(state);
> > > > > +		goto unlock;
> > > > > +	}    
> > > > 
> > > > No, you must not allocate a new state for this, you need to reuse the
> > > > existing state. You'll find it in bridge->base.state->state.  
> > > 
> > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > 
> > > I will use that if using the state is still relevant (see next comment).
> > >   
> > > >   
> > > > > +	state->acquire_ctx = &ctx;
> > > > > +
> > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > +							     sn65dsi83->bridge.encoder);
> > > > > +	if (!connector) {
> > > > > +		err = -EINVAL;
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > +	if (IS_ERR(connector_state)) {
> > > > > +		err = PTR_ERR(connector_state);
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > +	if (err < 0)
> > > > > +		goto unlock;    
> > > > 
> > > > And you'll find the crtc in bridge->encoder->crtc.  
> > > 
> > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > field available in this structure should not be used by atomic drivers. They
> > > should rely on &drm_connector_state.crtc.  
> > 
> > You're right, it's deprecated but used by most bridges anyway.
> > 
> > I made a series of changes after reviewing your series to address some
> > issues with the current bridge API, most notably
> > 
> > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/
> 
> Thanks for pointing that, indeed, it clarify many things!
> 
> > 
> > > In my case, I have the feeling that I should get the ctrc from the current
> > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > current patch:
> > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()  
> > 
> > Retrieving the old connector makes no sense though. It's the connector
> > that was formerly associated with your encoder. It might work, it might
> > not, it's not what you're looking for.
> > 
> > >   Retrieve the connector state with drm_atomic_get_connector_state()  
> > 
> > drm_atomic_get_connector_state will allocate and pull the connector
> > state into the drm_atomic_state, even if it wasn't part of it before, so
> > it's not great. And you don't need it in the first place, you only need
> > the current active CRTC.
> 
> Yes, I agree with that, I only need the active CRTC.
> 
> I tried to get the current atomic_state from:
>   1) bridge->base.state->state
>   2) drm_bridge_state->base.state
> 
> In both cases, it is NULL. Looking at Sima's reply in your series
> explained that:
>   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> 
> If I understood correctly those pointers are explicitly cleared.
> 
> So, with all of that, either:
>   a) I wait for your series to be applied in order to use your the crtc field from
>      drm_bridge_state added by:
>        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
>   b) I use the old school bridge->encoder->crtc for the moment
> 
> Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> my series?

Yeah, it makes sense.

Still, it would be great if you could test my series on your setup and see if it helps :)

Maxime

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

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

On Tue, 4 Feb 2025 16:17:10 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:
> > Hi Maxime,
> > 
> > On Thu, 16 Jan 2025 09:38:45 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:  
> > > > Hi Maxime,
> > > > 
> > > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > 
> > > > ...
> > > >     
> > > > > >  
> > > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > > +{
> > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > +	struct drm_connector_state *connector_state;
> > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > +	struct drm_connector *connector;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Reset active outputs of the related CRTC.
> > > > > > +	 *
> > > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > > +	 * outputs path. In our case, this will force the previous component to
> > > > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > > > +	 * bridge.
> > > > > > +	 *
> > > > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > > > +	 */
> > > > > > +
> > > > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > +
> > > > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > +	if (IS_ERR(state)) {
> > > > > > +		err = PTR_ERR(state);
> > > > > > +		goto unlock;
> > > > > > +	}      
> > > > > 
> > > > > No, you must not allocate a new state for this, you need to reuse the
> > > > > existing state. You'll find it in bridge->base.state->state.    
> > > > 
> > > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > > 
> > > > I will use that if using the state is still relevant (see next comment).
> > > >     
> > > > >     
> > > > > > +	state->acquire_ctx = &ctx;
> > > > > > +
> > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > +	if (!connector) {
> > > > > > +		err = -EINVAL;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > +		err = PTR_ERR(connector_state);
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > > +	if (err < 0)
> > > > > > +		goto unlock;      
> > > > > 
> > > > > And you'll find the crtc in bridge->encoder->crtc.    
> > > > 
> > > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > > field available in this structure should not be used by atomic drivers. They
> > > > should rely on &drm_connector_state.crtc.    
> > > 
> > > You're right, it's deprecated but used by most bridges anyway.
> > > 
> > > I made a series of changes after reviewing your series to address some
> > > issues with the current bridge API, most notably
> > > 
> > > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/  
> > 
> > Thanks for pointing that, indeed, it clarify many things!
> >   
> > >   
> > > > In my case, I have the feeling that I should get the ctrc from the current
> > > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > > current patch:
> > > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()    
> > > 
> > > Retrieving the old connector makes no sense though. It's the connector
> > > that was formerly associated with your encoder. It might work, it might
> > > not, it's not what you're looking for.
> > >   
> > > >   Retrieve the connector state with drm_atomic_get_connector_state()    
> > > 
> > > drm_atomic_get_connector_state will allocate and pull the connector
> > > state into the drm_atomic_state, even if it wasn't part of it before, so
> > > it's not great. And you don't need it in the first place, you only need
> > > the current active CRTC.  
> > 
> > Yes, I agree with that, I only need the active CRTC.
> > 
> > I tried to get the current atomic_state from:
> >   1) bridge->base.state->state
> >   2) drm_bridge_state->base.state
> > 
> > In both cases, it is NULL. Looking at Sima's reply in your series
> > explained that:
> >   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> > 
> > If I understood correctly those pointers are explicitly cleared.
> > 
> > So, with all of that, either:
> >   a) I wait for your series to be applied in order to use your the crtc field from
> >      drm_bridge_state added by:
> >        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
> >   b) I use the old school bridge->encoder->crtc for the moment
> > 
> > Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> > my series?  
> 
> Yeah, it makes sense.

I already send a wrong v4 (sorry) and a correct v5 with modifications in
this way :)

> 
> Still, it would be great if you could test my series on your setup and see if it helps :)

Of course, I can test updated version of your series.

I already try to get the current atomic_state exactly the same way as you do
in your series and the pointer is NULL in my case.

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Maxime Ripard]

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

On Tue, Feb 04, 2025 at 04:34:04PM +0100, Herve Codina wrote:
> On Tue, 4 Feb 2025 16:17:10 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > Hi,
> > 
> > On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:
> > > Hi Maxime,
> > > 
> > > On Thu, 16 Jan 2025 09:38:45 +0100
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >   
> > > > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:  
> > > > > Hi Maxime,
> > > > > 
> > > > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > > 
> > > > > ...
> > > > >     
> > > > > > >  
> > > > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > > > +{
> > > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > > +	struct drm_connector_state *connector_state;
> > > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > > +	struct drm_connector *connector;
> > > > > > > +	int err;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Reset active outputs of the related CRTC.
> > > > > > > +	 *
> > > > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > > > +	 * outputs path. In our case, this will force the previous component to
> > > > > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > > > > +	 * bridge.
> > > > > > > +	 *
> > > > > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > > > > +	 */
> > > > > > > +
> > > > > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > > +
> > > > > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > > +	if (IS_ERR(state)) {
> > > > > > > +		err = PTR_ERR(state);
> > > > > > > +		goto unlock;
> > > > > > > +	}      
> > > > > > 
> > > > > > No, you must not allocate a new state for this, you need to reuse the
> > > > > > existing state. You'll find it in bridge->base.state->state.    
> > > > > 
> > > > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > > > 
> > > > > I will use that if using the state is still relevant (see next comment).
> > > > >     
> > > > > >     
> > > > > > > +	state->acquire_ctx = &ctx;
> > > > > > > +
> > > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > > +	if (!connector) {
> > > > > > > +		err = -EINVAL;
> > > > > > > +		goto unlock;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > > +		err = PTR_ERR(connector_state);
> > > > > > > +		goto unlock;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > > > +	if (err < 0)
> > > > > > > +		goto unlock;      
> > > > > > 
> > > > > > And you'll find the crtc in bridge->encoder->crtc.    
> > > > > 
> > > > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > > > field available in this structure should not be used by atomic drivers. They
> > > > > should rely on &drm_connector_state.crtc.    
> > > > 
> > > > You're right, it's deprecated but used by most bridges anyway.
> > > > 
> > > > I made a series of changes after reviewing your series to address some
> > > > issues with the current bridge API, most notably
> > > > 
> > > > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/  
> > > 
> > > Thanks for pointing that, indeed, it clarify many things!
> > >   
> > > >   
> > > > > In my case, I have the feeling that I should get the ctrc from the current
> > > > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > > > current patch:
> > > > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()    
> > > > 
> > > > Retrieving the old connector makes no sense though. It's the connector
> > > > that was formerly associated with your encoder. It might work, it might
> > > > not, it's not what you're looking for.
> > > >   
> > > > >   Retrieve the connector state with drm_atomic_get_connector_state()    
> > > > 
> > > > drm_atomic_get_connector_state will allocate and pull the connector
> > > > state into the drm_atomic_state, even if it wasn't part of it before, so
> > > > it's not great. And you don't need it in the first place, you only need
> > > > the current active CRTC.  
> > > 
> > > Yes, I agree with that, I only need the active CRTC.
> > > 
> > > I tried to get the current atomic_state from:
> > >   1) bridge->base.state->state
> > >   2) drm_bridge_state->base.state
> > > 
> > > In both cases, it is NULL. Looking at Sima's reply in your series
> > > explained that:
> > >   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> > > 
> > > If I understood correctly those pointers are explicitly cleared.
> > > 
> > > So, with all of that, either:
> > >   a) I wait for your series to be applied in order to use your the crtc field from
> > >      drm_bridge_state added by:
> > >        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
> > >   b) I use the old school bridge->encoder->crtc for the moment
> > > 
> > > Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> > > my series?  
> > 
> > Yeah, it makes sense.
> 
> I already send a wrong v4 (sorry) and a correct v5 with modifications in
> this way :)
> 
> > 
> > Still, it would be great if you could test my series on your setup and see if it helps :)
> 
> Of course, I can test updated version of your series.
> 
> I already try to get the current atomic_state exactly the same way as you do
> in your series and the pointer is NULL in my case.

I sent a second version today, let me know if it works.

Maxime

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

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

On Tue, 4 Feb 2025 18:11:01 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Feb 04, 2025 at 04:34:04PM +0100, Herve Codina wrote:
> > On Tue, 4 Feb 2025 16:17:10 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > Hi,
> > > 
> > > On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:  
> > > > Hi Maxime,
> > > > 
> > > > On Thu, 16 Jan 2025 09:38:45 +0100
> > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > >     
> > > > > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:    
> > > > > > Hi Maxime,
> > > > > > 
> > > > > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > 
> > > > > > ...
> > > > > >       
> > > > > > > >  
> > > > > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > > > > +{
> > > > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > > > +	struct drm_connector_state *connector_state;
> > > > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > > > +	struct drm_connector *connector;
> > > > > > > > +	int err;
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * Reset active outputs of the related CRTC.
> > > > > > > > +	 *
> > > > > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > > > > +	 * outputs path. In our case, this will force the previous component to
> > > > > > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > > > > > +	 * bridge.
> > > > > > > > +	 *
> > > > > > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > > > > > +	 */
> > > > > > > > +
> > > > > > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > > > +
> > > > > > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > > > +	if (IS_ERR(state)) {
> > > > > > > > +		err = PTR_ERR(state);
> > > > > > > > +		goto unlock;
> > > > > > > > +	}        
> > > > > > > 
> > > > > > > No, you must not allocate a new state for this, you need to reuse the
> > > > > > > existing state. You'll find it in bridge->base.state->state.      
> > > > > > 
> > > > > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > > > > 
> > > > > > I will use that if using the state is still relevant (see next comment).
> > > > > >       
> > > > > > >       
> > > > > > > > +	state->acquire_ctx = &ctx;
> > > > > > > > +
> > > > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > > > +	if (!connector) {
> > > > > > > > +		err = -EINVAL;
> > > > > > > > +		goto unlock;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > > > +		err = PTR_ERR(connector_state);
> > > > > > > > +		goto unlock;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > > > > +	if (err < 0)
> > > > > > > > +		goto unlock;        
> > > > > > > 
> > > > > > > And you'll find the crtc in bridge->encoder->crtc.      
> > > > > > 
> > > > > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > > > > field available in this structure should not be used by atomic drivers. They
> > > > > > should rely on &drm_connector_state.crtc.      
> > > > > 
> > > > > You're right, it's deprecated but used by most bridges anyway.
> > > > > 
> > > > > I made a series of changes after reviewing your series to address some
> > > > > issues with the current bridge API, most notably
> > > > > 
> > > > > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/    
> > > > 
> > > > Thanks for pointing that, indeed, it clarify many things!
> > > >     
> > > > >     
> > > > > > In my case, I have the feeling that I should get the ctrc from the current
> > > > > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > > > > current patch:
> > > > > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()      
> > > > > 
> > > > > Retrieving the old connector makes no sense though. It's the connector
> > > > > that was formerly associated with your encoder. It might work, it might
> > > > > not, it's not what you're looking for.
> > > > >     
> > > > > >   Retrieve the connector state with drm_atomic_get_connector_state()      
> > > > > 
> > > > > drm_atomic_get_connector_state will allocate and pull the connector
> > > > > state into the drm_atomic_state, even if it wasn't part of it before, so
> > > > > it's not great. And you don't need it in the first place, you only need
> > > > > the current active CRTC.    
> > > > 
> > > > Yes, I agree with that, I only need the active CRTC.
> > > > 
> > > > I tried to get the current atomic_state from:
> > > >   1) bridge->base.state->state
> > > >   2) drm_bridge_state->base.state
> > > > 
> > > > In both cases, it is NULL. Looking at Sima's reply in your series
> > > > explained that:
> > > >   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> > > > 
> > > > If I understood correctly those pointers are explicitly cleared.
> > > > 
> > > > So, with all of that, either:
> > > >   a) I wait for your series to be applied in order to use your the crtc field from
> > > >      drm_bridge_state added by:
> > > >        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
> > > >   b) I use the old school bridge->encoder->crtc for the moment
> > > > 
> > > > Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> > > > my series?    
> > > 
> > > Yeah, it makes sense.  
> > 
> > I already send a wrong v4 (sorry) and a correct v5 with modifications in
> > this way :)
> >   
> > > 
> > > Still, it would be great if you could test my series on your setup and see if it helps :)  
> > 
> > Of course, I can test updated version of your series.
> > 
> > I already try to get the current atomic_state exactly the same way as you do
> > in your series and the pointer is NULL in my case.  
> 
> I sent a second version today, let me know if it works.
> 

Tried your v2 series and...:
   # modetest -s 39:1920x1080
   trying to open device 'i915'...failed
   ...
   trying to open device 'imx-lcdif'...done
   [   28.310476] ------------[ cut here ]------------
   [   28.310494] WARNING: CPU: 3 PID: 449 at drivers/gpu/drm/drm_bridge.c:943 drm_atomic_bridge_chain_check+0x24c/0x310 [drm]
setting mode 1920x1080-60.00Hz on[   28.326058] Modules linked in: fsl_ldb imx8mp_interconnect imx_interconnect imx_cpufreq_dt imx8mm_thermal lm75 tmp103 rtc_snvs leds_pca963x snvs_pwrkey rtc_rs5c372 pwm_imx27 st_pressure_spi st_sensors_spi regmap_spi gpio_charger st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer kfifo_buf st_sensors led_bl panel_simple opt3001 iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi dwmac_imx stmmac_platform stmmac pcs_xpcs phylink samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi fsl_imx8_ddr_perf exc3000 caam ti_sn65dsi83 error hotplug_bridge pwm_bl drm_display_helper drm_kms_helper drm drm_panel_orientation_quirks backlight gehc_sunh_connector ltc2497 ltc2497_core
   [   28.391264] CPU: 3 UID: 0 PID: 449 Comm: modetest Not tainted 6.14.0-rc1+ #18
   [   28.398404] Hardware name: GE HealthCare Supernova Patient Hub v1 (DT)
   [   28.404933] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
   [   28.411896] pc : drm_atomic_bridge_chain_check+0x24c/0x310 [drm]
   [   28.417940] lr : drm_atomic_bridge_chain_check+0x134/0x310 [drm]
   [   28.423983] sp : ffff8000823eb860
   [   28.427299] x29: ffff8000823eb860 x28: ffff000000ac1e00 x27: ffff00007b27ea18
   [   28.434445] x26: ffff00007b27ea90 x25: ffff00007b106170 x24: ffff00007a5d6ed8
   [   28.441587] x23: ffff00007bb4b8a0 x22: ffff00007b27e800 x21: ffff00007b27ea00
   [   28.448732] x20: 0000000000000000 x19: ffff00007b106008 x18: 0000000000000000
   [   28.455876] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
   [   28.463021] x14: 0000000000000000 x13: ffff00007a7c4ec0 x12: ffff000006dcb440
   [   28.470165] x11: ffff00007a7c4ec0 x10: ffff000079ee5200 x9 : ffff8000798e1aec
   [   28.477311] x8 : ffff00007a63f190 x7 : 0000000000000000 x6 : ffff800079954110
   [   28.484455] x5 : 0000000000000000 x4 : 0000000000000020 x3 : ffff000079a12d80
   [   28.491601] x2 : ffff00007b107000 x1 : ffff00007b106008 x0 : 0000000000000000
   [   28.498747] Call trace:
   [   28.501195]  drm_atomic_bridge_chain_check+0x24c/0x310 [drm] (P)
   [   28.507241]  drm_atomic_helper_check_modeset+0xa1c/0xca0 [drm_kms_helper]
   [   28.514046]  drm_atomic_helper_check+0x28/0xb8 [drm_kms_helper]
   [   28.519980]  drm_atomic_check_only+0x4bc/0x990 [drm]
   [   28.524982]  drm_atomic_commit+0x50/0xd8 [drm]
   [   28.529463]  drm_atomic_helper_set_config+0xe4/0x128 [drm_kms_helper]
   [   28.535919]  drm_mode_setcrtc+0x1cc/0x7b0 [drm]
   [   28.540486]  drm_ioctl_kernel+0xc0/0x140 [drm]
   [   28.544965]  drm_ioctl+0x210/0x4e8 [drm]
   [   28.548926]  __arm64_sys_ioctl+0xa4/0xe8
   [   28.552860]  invoke_syscall+0x50/0x120
   [   28.556619]  el0_svc_common.constprop.0+0x48/0xf8
   [   28.561330]  do_el0_svc+0x28/0x40
   [   28.564653]  el0_svc+0x30/0xd0
   [   28.567713]  el0t_64_sync_handler+0x144/0x168
   [   28.572075]  el0t_64_sync+0x198/0x1a0
   [   28.575746] ---[ end trace 0000000000000000 ]---
    connectors 39, crtc 36
   failed to set mode: Function not implemented


Without your series applied, modetest -s works correctly.

Due to the failure, I couldn't test your drm_bridge_get_current_state() to
see if I can retrieve the drm_bridge_state from my sn65dsi83_reset_pipe()
function.

Also, I have some local commits related to Luca's work about DRM bridge
hot-pluggin stuff on my test branch.

It will not be easy for me to test your series without them as my TI
sn65dsi83 bridge is behind a connector and I need some Luca's modification
to have a functional system. Sorry about that.

Hope that the kernel WARN log provided here will ring you a bell.

Feel free to ask for more information if needed.

Best regards,
Hervé

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Maxime Ripard]

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

Hi,

On Tue, Feb 04, 2025 at 07:52:40PM +0100, Herve Codina wrote:
> On Tue, 4 Feb 2025 18:11:01 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Tue, Feb 04, 2025 at 04:34:04PM +0100, Herve Codina wrote:
> > > On Tue, 4 Feb 2025 16:17:10 +0100
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:  
> > > > > Hi Maxime,
> > > > > 
> > > > > On Thu, 16 Jan 2025 09:38:45 +0100
> > > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > >     
> > > > > > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:    
> > > > > > > Hi Maxime,
> > > > > > > 
> > > > > > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > > > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > > 
> > > > > > > ...
> > > > > > >       
> > > > > > > > >  
> > > > > > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > > > > > +{
> > > > > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > > > > +	struct drm_connector_state *connector_state;
> > > > > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > > > > +	struct drm_connector *connector;
> > > > > > > > > +	int err;
> > > > > > > > > +
> > > > > > > > > +	/*
> > > > > > > > > +	 * Reset active outputs of the related CRTC.
> > > > > > > > > +	 *
> > > > > > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > > > > > +	 * outputs path. In our case, this will force the previous component to
> > > > > > > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > > > > > > +	 * bridge.
> > > > > > > > > +	 *
> > > > > > > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > > > > > > +	 */
> > > > > > > > > +
> > > > > > > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > > > > +
> > > > > > > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > > > > +	if (IS_ERR(state)) {
> > > > > > > > > +		err = PTR_ERR(state);
> > > > > > > > > +		goto unlock;
> > > > > > > > > +	}        
> > > > > > > > 
> > > > > > > > No, you must not allocate a new state for this, you need to reuse the
> > > > > > > > existing state. You'll find it in bridge->base.state->state.      
> > > > > > > 
> > > > > > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > > > > > 
> > > > > > > I will use that if using the state is still relevant (see next comment).
> > > > > > >       
> > > > > > > >       
> > > > > > > > > +	state->acquire_ctx = &ctx;
> > > > > > > > > +
> > > > > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > > > > +	if (!connector) {
> > > > > > > > > +		err = -EINVAL;
> > > > > > > > > +		goto unlock;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > > > > +		err = PTR_ERR(connector_state);
> > > > > > > > > +		goto unlock;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > > > > > +	if (err < 0)
> > > > > > > > > +		goto unlock;        
> > > > > > > > 
> > > > > > > > And you'll find the crtc in bridge->encoder->crtc.      
> > > > > > > 
> > > > > > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > > > > > field available in this structure should not be used by atomic drivers. They
> > > > > > > should rely on &drm_connector_state.crtc.      
> > > > > > 
> > > > > > You're right, it's deprecated but used by most bridges anyway.
> > > > > > 
> > > > > > I made a series of changes after reviewing your series to address some
> > > > > > issues with the current bridge API, most notably
> > > > > > 
> > > > > > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/    
> > > > > 
> > > > > Thanks for pointing that, indeed, it clarify many things!
> > > > >     
> > > > > >     
> > > > > > > In my case, I have the feeling that I should get the ctrc from the current
> > > > > > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > > > > > current patch:
> > > > > > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()      
> > > > > > 
> > > > > > Retrieving the old connector makes no sense though. It's the connector
> > > > > > that was formerly associated with your encoder. It might work, it might
> > > > > > not, it's not what you're looking for.
> > > > > >     
> > > > > > >   Retrieve the connector state with drm_atomic_get_connector_state()      
> > > > > > 
> > > > > > drm_atomic_get_connector_state will allocate and pull the connector
> > > > > > state into the drm_atomic_state, even if it wasn't part of it before, so
> > > > > > it's not great. And you don't need it in the first place, you only need
> > > > > > the current active CRTC.    
> > > > > 
> > > > > Yes, I agree with that, I only need the active CRTC.
> > > > > 
> > > > > I tried to get the current atomic_state from:
> > > > >   1) bridge->base.state->state
> > > > >   2) drm_bridge_state->base.state
> > > > > 
> > > > > In both cases, it is NULL. Looking at Sima's reply in your series
> > > > > explained that:
> > > > >   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> > > > > 
> > > > > If I understood correctly those pointers are explicitly cleared.
> > > > > 
> > > > > So, with all of that, either:
> > > > >   a) I wait for your series to be applied in order to use your the crtc field from
> > > > >      drm_bridge_state added by:
> > > > >        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
> > > > >   b) I use the old school bridge->encoder->crtc for the moment
> > > > > 
> > > > > Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> > > > > my series?    
> > > > 
> > > > Yeah, it makes sense.  
> > > 
> > > I already send a wrong v4 (sorry) and a correct v5 with modifications in
> > > this way :)
> > >   
> > > > 
> > > > Still, it would be great if you could test my series on your setup and see if it helps :)  
> > > 
> > > Of course, I can test updated version of your series.
> > > 
> > > I already try to get the current atomic_state exactly the same way as you do
> > > in your series and the pointer is NULL in my case.  
> > 
> > I sent a second version today, let me know if it works.
> > 
> 
> Tried your v2 series and...:
>    # modetest -s 39:1920x1080
>    trying to open device 'i915'...failed
>    ...
>    trying to open device 'imx-lcdif'...done
>    [   28.310476] ------------[ cut here ]------------
>    [   28.310494] WARNING: CPU: 3 PID: 449 at drivers/gpu/drm/drm_bridge.c:943 drm_atomic_bridge_chain_check+0x24c/0x310 [drm]
> setting mode 1920x1080-60.00Hz on[   28.326058] Modules linked in: fsl_ldb imx8mp_interconnect imx_interconnect imx_cpufreq_dt imx8mm_thermal lm75 tmp103 rtc_snvs leds_pca963x snvs_pwrkey rtc_rs5c372 pwm_imx27 st_pressure_spi st_sensors_spi regmap_spi gpio_charger st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer kfifo_buf st_sensors led_bl panel_simple opt3001 iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi dwmac_imx stmmac_platform stmmac pcs_xpcs phylink samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi fsl_imx8_ddr_perf exc3000 caam ti_sn65dsi83 error hotplug_bridge pwm_bl drm_display_helper drm_kms_helper drm drm_panel_orientation_quirks backlight gehc_sunh_connector ltc2497 ltc2497_core
>    [   28.391264] CPU: 3 UID: 0 PID: 449 Comm: modetest Not tainted 6.14.0-rc1+ #18
>    [   28.398404] Hardware name: GE HealthCare Supernova Patient Hub v1 (DT)
>    [   28.404933] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>    [   28.411896] pc : drm_atomic_bridge_chain_check+0x24c/0x310 [drm]
>    [   28.417940] lr : drm_atomic_bridge_chain_check+0x134/0x310 [drm]
>    [   28.423983] sp : ffff8000823eb860
>    [   28.427299] x29: ffff8000823eb860 x28: ffff000000ac1e00 x27: ffff00007b27ea18
>    [   28.434445] x26: ffff00007b27ea90 x25: ffff00007b106170 x24: ffff00007a5d6ed8
>    [   28.441587] x23: ffff00007bb4b8a0 x22: ffff00007b27e800 x21: ffff00007b27ea00
>    [   28.448732] x20: 0000000000000000 x19: ffff00007b106008 x18: 0000000000000000
>    [   28.455876] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>    [   28.463021] x14: 0000000000000000 x13: ffff00007a7c4ec0 x12: ffff000006dcb440
>    [   28.470165] x11: ffff00007a7c4ec0 x10: ffff000079ee5200 x9 : ffff8000798e1aec
>    [   28.477311] x8 : ffff00007a63f190 x7 : 0000000000000000 x6 : ffff800079954110
>    [   28.484455] x5 : 0000000000000000 x4 : 0000000000000020 x3 : ffff000079a12d80
>    [   28.491601] x2 : ffff00007b107000 x1 : ffff00007b106008 x0 : 0000000000000000
>    [   28.498747] Call trace:
>    [   28.501195]  drm_atomic_bridge_chain_check+0x24c/0x310 [drm] (P)
>    [   28.507241]  drm_atomic_helper_check_modeset+0xa1c/0xca0 [drm_kms_helper]
>    [   28.514046]  drm_atomic_helper_check+0x28/0xb8 [drm_kms_helper]
>    [   28.519980]  drm_atomic_check_only+0x4bc/0x990 [drm]
>    [   28.524982]  drm_atomic_commit+0x50/0xd8 [drm]
>    [   28.529463]  drm_atomic_helper_set_config+0xe4/0x128 [drm_kms_helper]
>    [   28.535919]  drm_mode_setcrtc+0x1cc/0x7b0 [drm]
>    [   28.540486]  drm_ioctl_kernel+0xc0/0x140 [drm]
>    [   28.544965]  drm_ioctl+0x210/0x4e8 [drm]
>    [   28.548926]  __arm64_sys_ioctl+0xa4/0xe8
>    [   28.552860]  invoke_syscall+0x50/0x120
>    [   28.556619]  el0_svc_common.constprop.0+0x48/0xf8
>    [   28.561330]  do_el0_svc+0x28/0x40
>    [   28.564653]  el0_svc+0x30/0xd0
>    [   28.567713]  el0t_64_sync_handler+0x144/0x168
>    [   28.572075]  el0t_64_sync+0x198/0x1a0
>    [   28.575746] ---[ end trace 0000000000000000 ]---
>     connectors 39, crtc 36
>    failed to set mode: Function not implemented
> 
> 
> Without your series applied, modetest -s works correctly.
> 
> Due to the failure, I couldn't test your drm_bridge_get_current_state() to
> see if I can retrieve the drm_bridge_state from my sn65dsi83_reset_pipe()
> function.
> 
> Also, I have some local commits related to Luca's work about DRM bridge
> hot-pluggin stuff on my test branch.
> 
> It will not be easy for me to test your series without them as my TI
> sn65dsi83 bridge is behind a connector and I need some Luca's modification
> to have a functional system. Sorry about that.
>
> Hope that the kernel WARN log provided here will ring you a bell.

Not really. I can't figure out where in drm_atomic_bridge_chain_check we
have that warning.

Does $CROSS_COMPILE-addr2line -e vmlinux
drm_atomic_bridge_chain_check+0x24c/0x310 report anything?

Maxime

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

Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina]

Hi Maxime,

On Wed, 19 Feb 2025 10:07:39 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Tue, Feb 04, 2025 at 07:52:40PM +0100, Herve Codina wrote:
> > On Tue, 4 Feb 2025 18:11:01 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> > 
> > > On Tue, Feb 04, 2025 at 04:34:04PM +0100, Herve Codina wrote:
> > > > On Tue, 4 Feb 2025 16:17:10 +0100
> > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > >   
> > > > > Hi,
> > > > > 
> > > > > On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:  
> > > > > > Hi Maxime,
> > > > > > 
> > > > > > On Thu, 16 Jan 2025 09:38:45 +0100
> > > > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > > >     
> > > > > > > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:    
> > > > > > > > Hi Maxime,
> > > > > > > > 
> > > > > > > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > > > > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > > > 
> > > > > > > > ...
> > > > > > > >       
> > > > > > > > > >  
> > > > > > > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > > > > > > +{
> > > > > > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > > > > > +	struct drm_connector_state *connector_state;
> > > > > > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > > > > > +	struct drm_connector *connector;
> > > > > > > > > > +	int err;
> > > > > > > > > > +
> > > > > > > > > > +	/*
> > > > > > > > > > +	 * Reset active outputs of the related CRTC.
> > > > > > > > > > +	 *
> > > > > > > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > > > > > > +	 * outputs path. In our case, this will force the previous component to
> > > > > > > > > > +	 * go back in LP11 mode and so allow the reconfiguration of SN64DSI83
> > > > > > > > > > +	 * bridge.
> > > > > > > > > > +	 *
> > > > > > > > > > +	 * Keep the lock during the whole operation to be atomic.
> > > > > > > > > > +	 */
> > > > > > > > > > +
> > > > > > > > > > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > > > > > +
> > > > > > > > > > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > > > > > +	if (IS_ERR(state)) {
> > > > > > > > > > +		err = PTR_ERR(state);
> > > > > > > > > > +		goto unlock;
> > > > > > > > > > +	}        
> > > > > > > > > 
> > > > > > > > > No, you must not allocate a new state for this, you need to reuse the
> > > > > > > > > existing state. You'll find it in bridge->base.state->state.      
> > > > > > > > 
> > > > > > > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > > > > > > 
> > > > > > > > I will use that if using the state is still relevant (see next comment).
> > > > > > > >       
> > > > > > > > >       
> > > > > > > > > > +	state->acquire_ctx = &ctx;
> > > > > > > > > > +
> > > > > > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > > > > > +	if (!connector) {
> > > > > > > > > > +		err = -EINVAL;
> > > > > > > > > > +		goto unlock;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > > > > > +		err = PTR_ERR(connector_state);
> > > > > > > > > > +		goto unlock;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > > > > > > +	if (err < 0)
> > > > > > > > > > +		goto unlock;        
> > > > > > > > > 
> > > > > > > > > And you'll find the crtc in bridge->encoder->crtc.      
> > > > > > > > 
> > > > > > > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > > > > > > field available in this structure should not be used by atomic drivers. They
> > > > > > > > should rely on &drm_connector_state.crtc.      
> > > > > > > 
> > > > > > > You're right, it's deprecated but used by most bridges anyway.
> > > > > > > 
> > > > > > > I made a series of changes after reviewing your series to address some
> > > > > > > issues with the current bridge API, most notably
> > > > > > > 
> > > > > > > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/    
> > > > > > 
> > > > > > Thanks for pointing that, indeed, it clarify many things!
> > > > > >     
> > > > > > >     
> > > > > > > > In my case, I have the feeling that I should get the ctrc from the current
> > > > > > > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > > > > > > current patch:
> > > > > > > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()      
> > > > > > > 
> > > > > > > Retrieving the old connector makes no sense though. It's the connector
> > > > > > > that was formerly associated with your encoder. It might work, it might
> > > > > > > not, it's not what you're looking for.
> > > > > > >     
> > > > > > > >   Retrieve the connector state with drm_atomic_get_connector_state()      
> > > > > > > 
> > > > > > > drm_atomic_get_connector_state will allocate and pull the connector
> > > > > > > state into the drm_atomic_state, even if it wasn't part of it before, so
> > > > > > > it's not great. And you don't need it in the first place, you only need
> > > > > > > the current active CRTC.    
> > > > > > 
> > > > > > Yes, I agree with that, I only need the active CRTC.
> > > > > > 
> > > > > > I tried to get the current atomic_state from:
> > > > > >   1) bridge->base.state->state
> > > > > >   2) drm_bridge_state->base.state
> > > > > > 
> > > > > > In both cases, it is NULL. Looking at Sima's reply in your series
> > > > > > explained that:
> > > > > >   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> > > > > > 
> > > > > > If I understood correctly those pointers are explicitly cleared.
> > > > > > 
> > > > > > So, with all of that, either:
> > > > > >   a) I wait for your series to be applied in order to use your the crtc field from
> > > > > >      drm_bridge_state added by:
> > > > > >        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org/#t
> > > > > >   b) I use the old school bridge->encoder->crtc for the moment
> > > > > > 
> > > > > > Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> > > > > > my series?    
> > > > > 
> > > > > Yeah, it makes sense.  
> > > > 
> > > > I already send a wrong v4 (sorry) and a correct v5 with modifications in
> > > > this way :)
> > > >   
> > > > > 
> > > > > Still, it would be great if you could test my series on your setup and see if it helps :)  
> > > > 
> > > > Of course, I can test updated version of your series.
> > > > 
> > > > I already try to get the current atomic_state exactly the same way as you do
> > > > in your series and the pointer is NULL in my case.  
> > > 
> > > I sent a second version today, let me know if it works.
> > > 
> > 
> > Tried your v2 series and...:
> >    # modetest -s 39:1920x1080
> >    trying to open device 'i915'...failed
> >    ...
> >    trying to open device 'imx-lcdif'...done
> >    [   28.310476] ------------[ cut here ]------------
> >    [   28.310494] WARNING: CPU: 3 PID: 449 at drivers/gpu/drm/drm_bridge.c:943 drm_atomic_bridge_chain_check+0x24c/0x310 [drm]
> > setting mode 1920x1080-60.00Hz on[   28.326058] Modules linked in: fsl_ldb imx8mp_interconnect imx_interconnect imx_cpufreq_dt imx8mm_thermal lm75 tmp103 rtc_snvs leds_pca963x snvs_pwrkey rtc_rs5c372 pwm_imx27 st_pressure_spi st_sensors_spi regmap_spi gpio_charger st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer kfifo_buf st_sensors led_bl panel_simple opt3001 iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi dwmac_imx stmmac_platform stmmac pcs_xpcs phylink samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi fsl_imx8_ddr_perf exc3000 caam ti_sn65dsi83 error hotplug_bridge pwm_bl drm_display_helper drm_kms_helper drm drm_panel_orientation_quirks backlight gehc_sunh_connector ltc2497 ltc2497_core
> >    [   28.391264] CPU: 3 UID: 0 PID: 449 Comm: modetest Not tainted 6.14.0-rc1+ #18
> >    [   28.398404] Hardware name: Xxxxxxxxxx
> >    [   28.404933] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >    [   28.411896] pc : drm_atomic_bridge_chain_check+0x24c/0x310 [drm]
> >    [   28.417940] lr : drm_atomic_bridge_chain_check+0x134/0x310 [drm]
> >    [   28.423983] sp : ffff8000823eb860
> >    [   28.427299] x29: ffff8000823eb860 x28: ffff000000ac1e00 x27: ffff00007b27ea18
> >    [   28.434445] x26: ffff00007b27ea90 x25: ffff00007b106170 x24: ffff00007a5d6ed8
> >    [   28.441587] x23: ffff00007bb4b8a0 x22: ffff00007b27e800 x21: ffff00007b27ea00
> >    [   28.448732] x20: 0000000000000000 x19: ffff00007b106008 x18: 0000000000000000
> >    [   28.455876] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> >    [   28.463021] x14: 0000000000000000 x13: ffff00007a7c4ec0 x12: ffff000006dcb440
> >    [   28.470165] x11: ffff00007a7c4ec0 x10: ffff000079ee5200 x9 : ffff8000798e1aec
> >    [   28.477311] x8 : ffff00007a63f190 x7 : 0000000000000000 x6 : ffff800079954110
> >    [   28.484455] x5 : 0000000000000000 x4 : 0000000000000020 x3 : ffff000079a12d80
> >    [   28.491601] x2 : ffff00007b107000 x1 : ffff00007b106008 x0 : 0000000000000000
> >    [   28.498747] Call trace:
> >    [   28.501195]  drm_atomic_bridge_chain_check+0x24c/0x310 [drm] (P)
> >    [   28.507241]  drm_atomic_helper_check_modeset+0xa1c/0xca0 [drm_kms_helper]
> >    [   28.514046]  drm_atomic_helper_check+0x28/0xb8 [drm_kms_helper]
> >    [   28.519980]  drm_atomic_check_only+0x4bc/0x990 [drm]
> >    [   28.524982]  drm_atomic_commit+0x50/0xd8 [drm]
> >    [   28.529463]  drm_atomic_helper_set_config+0xe4/0x128 [drm_kms_helper]
> >    [   28.535919]  drm_mode_setcrtc+0x1cc/0x7b0 [drm]
> >    [   28.540486]  drm_ioctl_kernel+0xc0/0x140 [drm]
> >    [   28.544965]  drm_ioctl+0x210/0x4e8 [drm]
> >    [   28.548926]  __arm64_sys_ioctl+0xa4/0xe8
> >    [   28.552860]  invoke_syscall+0x50/0x120
> >    [   28.556619]  el0_svc_common.constprop.0+0x48/0xf8
> >    [   28.561330]  do_el0_svc+0x28/0x40
> >    [   28.564653]  el0_svc+0x30/0xd0
> >    [   28.567713]  el0t_64_sync_handler+0x144/0x168
> >    [   28.572075]  el0t_64_sync+0x198/0x1a0
> >    [   28.575746] ---[ end trace 0000000000000000 ]---
> >     connectors 39, crtc 36
> >    failed to set mode: Function not implemented
> > 
> > 
> > Without your series applied, modetest -s works correctly.
> > 
> > Due to the failure, I couldn't test your drm_bridge_get_current_state() to
> > see if I can retrieve the drm_bridge_state from my sn65dsi83_reset_pipe()
> > function.
> > 
> > Also, I have some local commits related to Luca's work about DRM bridge
> > hot-pluggin stuff on my test branch.
> > 
> > It will not be easy for me to test your series without them as my TI
> > sn65dsi83 bridge is behind a connector and I need some Luca's modification
> > to have a functional system. Sorry about that.
> >
> > Hope that the kernel WARN log provided here will ring you a bell.
> 
> Not really. I can't figure out where in drm_atomic_bridge_chain_check we
> have that warning.
> 
> Does $CROSS_COMPILE-addr2line -e vmlinux
> drm_atomic_bridge_chain_check+0x24c/0x310 report anything?
> 

addr2line doesn't give any information but the WARN() triggered is the
following (code available in your series):
   --- 8< ---
   static int drm_atomic_bridge_check(struct drm_bridge *bridge,
   				   struct drm_crtc_state *crtc_state,
   				   struct drm_connector_state *conn_state)
   {
   	struct drm_bridge_state *bridge_state;
   	int ret;
   
   	bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
   						       bridge);
   	if (WARN_ON(!bridge_state))
   		return -EINVAL;
   --- 8< ---

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com