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

[PATCH v2 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 v2 series mainly:
  - Rebase patches on top of v6.13-rc3.
  - Reset the output path where the bridge is located instead of the
    full pipeline.
  - Add more information in a commit log.

Best regards,
Hervé Codina

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/atomic-helpers: Introduce drm_atomic_helper_disable_connector()
  drm: bridge: ti-sn65dsi83: Add error recovery mechanism

 .../bindings/display/bridge/ti,sn65dsi83.yaml |   3 +
 drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 142 ++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c           |  67 +++++++++
 include/drm/drm_atomic_helper.h               |   2 +
 4 files changed, 214 insertions(+)

-- 
2.47.0

[PATCH v2 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.0

[PATCH v2 2/3] drm/atomic-helpers: Introduce drm_atomic_helper_disable_connector()

[Herve Codina]

drm_atomic_helper_disable_connector() disables a connector taking care
of disabling the CRTC as well if the disabled connector was the only one
connector connected to the CRTC.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 67 +++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 +
 2 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8ed186ddaeaf..1d691cbb047e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3279,6 +3279,73 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
 }
 EXPORT_SYMBOL(drm_atomic_helper_set_config);
 
+/**
+ * drm_atomic_helper_disable_connector - disable connector
+ * @connector: connector to disable
+ * @ctx: lock acquisition context
+ *
+ * Turn off the given connector by setting its DPMS mode to off.
+ * Also deactivate the CRTC is this connector was the only one connected to the
+ * CRTC.
+ *
+ * Note that if callers haven't already acquired all modeset locks this might
+ * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_atomic_helper_disable_connector(struct drm_connector *connector,
+					struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_connector_state *connector_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_atomic_state *state;
+	struct drm_crtc *crtc;
+	int ret;
+
+	state = drm_atomic_state_alloc(connector->dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = ctx;
+
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state)) {
+		ret = PTR_ERR(connector_state);
+		goto end;
+	}
+
+	crtc = connector_state->crtc;
+
+	ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
+	if (ret < 0)
+		goto end;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto end;
+	}
+
+	if (!crtc_state->connector_mask) {
+		/*
+		 * The only one connector from the crtc has
+		 * been disabled -> Disable the CRTC too
+		 */
+		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
+		if (ret < 0)
+			goto end;
+
+		crtc_state->active = false;
+	}
+
+	ret = drm_atomic_commit(state);
+end:
+	drm_atomic_state_put(state);
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_disable_connector);
+
 /**
  * drm_atomic_helper_disable_all - disable all currently active outputs
  * @dev: DRM device
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9aa0a05aa072..923a354577a3 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -137,6 +137,8 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane,
 int drm_atomic_helper_set_config(struct drm_mode_set *set,
 				 struct drm_modeset_acquire_ctx *ctx);
 
+int drm_atomic_helper_disable_connector(struct drm_connector *connector,
+					struct drm_modeset_acquire_ctx *ctx);
 int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx);
 void drm_atomic_helper_shutdown(struct drm_device *dev);
-- 
2.47.0

[PATCH v2 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 | 142 ++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index e6264514bb3f..f3d66d17f28c 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() need 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,106 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
 	return dsi_div - 1;
 }
 
+static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
+{
+	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
+	struct drm_device *dev = sn65dsi83->bridge.dev;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_connector *connector;
+	int err;
+
+	/*
+	 * Reset components available from the encoder to the connector.
+	 * To do that, we disable then re-enable the connector linked to the
+	 * encoder.
+	 *
+	 * This way, drm core will reconfigure each components. 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;
+	}
+
+	connector = drm_atomic_get_old_connector_for_encoder(state,
+							     sn65dsi83->bridge.encoder);
+	if (!connector) {
+		err = -EINVAL;
+		goto unlock;
+	}
+
+	err = drm_atomic_helper_disable_connector(connector, &ctx);
+	if (err < 0)
+		goto unlock;
+
+	/* Restore original state to re-enable the connector */
+	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
+
+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 drm output\n");
+
+	/* Reset the output */
+	ret = sn65dsi83_reset_drm_output(ctx);
+	if (ret) {
+		dev_err(ctx->dev, "reset drm output 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 +622,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 +639,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 +805,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 +830,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 +856,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.0

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

[Maxime Ripard]

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

On Tue, Dec 17, 2024 at 03:32:15PM +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 | 142 ++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index e6264514bb3f..f3d66d17f28c 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() need 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,106 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
>  	return dsi_div - 1;
>  }
>  
> +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> +{
> +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> +	struct drm_device *dev = sn65dsi83->bridge.dev;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector *connector;
> +	int err;
> +
> +	/*
> +	 * Reset components available from the encoder to the connector.
> +	 * To do that, we disable then re-enable the connector linked to the
> +	 * encoder.
> +	 *
> +	 * This way, drm core will reconfigure each components. 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;
> +	}
> +
> +	connector = drm_atomic_get_old_connector_for_encoder(state,
> +							     sn65dsi83->bridge.encoder);
> +	if (!connector) {
> +		err = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> +	if (err < 0)
> +		goto unlock;
> +
> +	/* Restore original state to re-enable the connector */
> +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> +
> +unlock:
> +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> +	if (!IS_ERR(state))
> +		drm_atomic_state_put(state);
> +	return err;
> +}

In the previous version, we advised to create a generic helper similar
to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().

It looks like you chose a different path. Can you expand why?

Maxime

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

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

[Herve Codina]

Hi Maxime,

On Tue, 17 Dec 2024 18:30:52 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Dec 17, 2024 at 03:32:15PM +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 | 142 ++++++++++++++++++++++++++
> >  1 file changed, 142 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index e6264514bb3f..f3d66d17f28c 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() need 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,106 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> >  	return dsi_div - 1;
> >  }
> >  
> > +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> > +{
> > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_connector *connector;
> > +	int err;
> > +
> > +	/*
> > +	 * Reset components available from the encoder to the connector.
> > +	 * To do that, we disable then re-enable the connector linked to the
> > +	 * encoder.
> > +	 *
> > +	 * This way, drm core will reconfigure each components. 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;
> > +	}
> > +
> > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > +							     sn65dsi83->bridge.encoder);
> > +	if (!connector) {
> > +		err = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> > +	if (err < 0)
> > +		goto unlock;
> > +
> > +	/* Restore original state to re-enable the connector */
> > +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > +
> > +unlock:
> > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> > +	if (!IS_ERR(state))
> > +		drm_atomic_state_put(state);
> > +	return err;
> > +}  
> 
> In the previous version, we advised to create a generic helper similar
> to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().
> 
> It looks like you chose a different path. Can you expand why?
> 

I didn't choose a different path.
I created the drm_atomic_helper_disable_connector(). Maybe it is not enough.

I can move (copy/paste) sn65dsi83_reset_drm_output() to a new helper:
  int drm_atomic_helper_disable_output(struct drm_encoder *encoder)

Is it what you expect?

Also, are operations done in current sn65dsi83_reset_drm_output() correct
in order to reset the output? It works on my system but what is your
feedback on operations performed.

Best regards,
Hervé

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

[Maxime Ripard]

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

On Wed, Dec 18, 2024 at 09:24:07AM +0100, Herve Codina wrote:
> Hi Maxime,
> 
> On Tue, 17 Dec 2024 18:30:52 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Tue, Dec 17, 2024 at 03:32:15PM +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 | 142 ++++++++++++++++++++++++++
> > >  1 file changed, 142 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > index e6264514bb3f..f3d66d17f28c 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() need 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,106 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> > >  	return dsi_div - 1;
> > >  }
> > >  
> > > +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> > > +{
> > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_connector *connector;
> > > +	int err;
> > > +
> > > +	/*
> > > +	 * Reset components available from the encoder to the connector.
> > > +	 * To do that, we disable then re-enable the connector linked to the
> > > +	 * encoder.
> > > +	 *
> > > +	 * This way, drm core will reconfigure each components. 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;
> > > +	}
> > > +
> > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > +							     sn65dsi83->bridge.encoder);
> > > +	if (!connector) {
> > > +		err = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> > > +	if (err < 0)
> > > +		goto unlock;
> > > +
> > > +	/* Restore original state to re-enable the connector */
> > > +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > > +
> > > +unlock:
> > > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> > > +	if (!IS_ERR(state))
> > > +		drm_atomic_state_put(state);
> > > +	return err;
> > > +}  
> > 
> > In the previous version, we advised to create a generic helper similar
> > to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().
> > 
> > It looks like you chose a different path. Can you expand why?
> > 
> 
> I didn't choose a different path.
> I created the drm_atomic_helper_disable_connector(). Maybe it is not enough.

It's not that it's not enough, it's that you're not doing the same
thing, see below.

> I can move (copy/paste) sn65dsi83_reset_drm_output() to a new helper:
>   int drm_atomic_helper_disable_output(struct drm_encoder *encoder)
> 
> Is it what you expect?
> 
> Also, are operations done in current sn65dsi83_reset_drm_output() correct
> in order to reset the output? It works on my system but what is your
> feedback on operations performed.

You don't need any of that. Both the reset_pipe() and
intel_modeset_commit_pipes() functions will flag the connectors as
updated in the commit, and the core will consider that it needs to
disable / enable the encoders and bridges below that CRTC.

See
https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1155
https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1476

So you really only need to convert any of these two functions into a
helper, and it does exactly what you need.

Maxime

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

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

[Herve Codina]

Hi Maxime,

On Wed, 18 Dec 2024 16:54:02 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> > > > +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> > > > +{
> > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > +	struct drm_connector *connector;
> > > > +	int err;
> > > > +
> > > > +	/*
> > > > +	 * Reset components available from the encoder to the connector.
> > > > +	 * To do that, we disable then re-enable the connector linked to the
> > > > +	 * encoder.
> > > > +	 *
> > > > +	 * This way, drm core will reconfigure each components. 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;
> > > > +	}
> > > > +
> > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > +							     sn65dsi83->bridge.encoder);
> > > > +	if (!connector) {
> > > > +		err = -EINVAL;
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> > > > +	if (err < 0)
> > > > +		goto unlock;
> > > > +
> > > > +	/* Restore original state to re-enable the connector */
> > > > +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > > > +
> > > > +unlock:
> > > > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> > > > +	if (!IS_ERR(state))
> > > > +		drm_atomic_state_put(state);
> > > > +	return err;
> > > > +}    
> > > 
> > > In the previous version, we advised to create a generic helper similar
> > > to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().
> > > 
> > > It looks like you chose a different path. Can you expand why?
> > >   
> > 
> > I didn't choose a different path.
> > I created the drm_atomic_helper_disable_connector(). Maybe it is not enough.  
> 
> It's not that it's not enough, it's that you're not doing the same
> thing, see below.
> 
> > I can move (copy/paste) sn65dsi83_reset_drm_output() to a new helper:
> >   int drm_atomic_helper_disable_output(struct drm_encoder *encoder)
> > 
> > Is it what you expect?
> > 
> > Also, are operations done in current sn65dsi83_reset_drm_output() correct
> > in order to reset the output? It works on my system but what is your
> > feedback on operations performed.  
> 
> You don't need any of that. Both the reset_pipe() and
> intel_modeset_commit_pipes() functions will flag the connectors as
> updated in the commit, and the core will consider that it needs to
> disable / enable the encoders and bridges below that CRTC.
> 
> See
> https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1155
> https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1476
> 
> So you really only need to convert any of these two functions into a
> helper, and it does exactly what you need.
> 

I see but if I set crtc_state->connectors_changed = true; as it is done in
reset_pipe(), in my understanding, all outputs will be reset.

I understood during v1 review that I need to reset only one output path, the
path where the bridge is present. In other words, the path from the encoder
attached to the bridge to the related connector and not all connectors
related to all encoders attached to the crtc.

Do you confirm that setting crtc_state->connectors_changed = true is the
correct way to do?

Best regards,
Hervé

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

[Maxime Ripard]

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

On Wed, Dec 18, 2024 at 05:37:28PM +0100, Herve Codina wrote:
> Hi Maxime,
> 
> On Wed, 18 Dec 2024 16:54:02 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > > > > +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> > > > > +{
> > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > +	struct drm_connector *connector;
> > > > > +	int err;
> > > > > +
> > > > > +	/*
> > > > > +	 * Reset components available from the encoder to the connector.
> > > > > +	 * To do that, we disable then re-enable the connector linked to the
> > > > > +	 * encoder.
> > > > > +	 *
> > > > > +	 * This way, drm core will reconfigure each components. 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;
> > > > > +	}
> > > > > +
> > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > +							     sn65dsi83->bridge.encoder);
> > > > > +	if (!connector) {
> > > > > +		err = -EINVAL;
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> > > > > +	if (err < 0)
> > > > > +		goto unlock;
> > > > > +
> > > > > +	/* Restore original state to re-enable the connector */
> > > > > +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > > > > +
> > > > > +unlock:
> > > > > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> > > > > +	if (!IS_ERR(state))
> > > > > +		drm_atomic_state_put(state);
> > > > > +	return err;
> > > > > +}    
> > > > 
> > > > In the previous version, we advised to create a generic helper similar
> > > > to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().
> > > > 
> > > > It looks like you chose a different path. Can you expand why?
> > > >   
> > > 
> > > I didn't choose a different path.
> > > I created the drm_atomic_helper_disable_connector(). Maybe it is not enough.  
> > 
> > It's not that it's not enough, it's that you're not doing the same
> > thing, see below.
> > 
> > > I can move (copy/paste) sn65dsi83_reset_drm_output() to a new helper:
> > >   int drm_atomic_helper_disable_output(struct drm_encoder *encoder)
> > > 
> > > Is it what you expect?
> > > 
> > > Also, are operations done in current sn65dsi83_reset_drm_output() correct
> > > in order to reset the output? It works on my system but what is your
> > > feedback on operations performed.  
> > 
> > You don't need any of that. Both the reset_pipe() and
> > intel_modeset_commit_pipes() functions will flag the connectors as
> > updated in the commit, and the core will consider that it needs to
> > disable / enable the encoders and bridges below that CRTC.
> > 
> > See
> > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1155
> > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1476
> > 
> > So you really only need to convert any of these two functions into a
> > helper, and it does exactly what you need.
> > 
> 
> I see but if I set crtc_state->connectors_changed = true; as it is done in
> reset_pipe(), in my understanding, all outputs will be reset.

Not all outputs, all active outputs connected to that CRTC. If you have
only one encoder connected to that CRTC, which is pretty typical on ARM
platforms, it's equivalent to what you're asking for.

And we should probably shut down the CRTC (and thus all active outputs)
anyway. Some encoders and bridges have internal FIFOs/state machines
that need to be enabled disabled at specific points during the
initialization, and the CRTC is a part of that.

Maxime

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

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

[Herve Codina]

Hi Maxime,

On Thu, 19 Dec 2024 15:01:39 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Wed, Dec 18, 2024 at 05:37:28PM +0100, Herve Codina wrote:
> > Hi Maxime,
> > 
> > On Wed, 18 Dec 2024 16:54:02 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > > > > +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> > > > > > +{
> > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > +	struct drm_connector *connector;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Reset components available from the encoder to the connector.
> > > > > > +	 * To do that, we disable then re-enable the connector linked to the
> > > > > > +	 * encoder.
> > > > > > +	 *
> > > > > > +	 * This way, drm core will reconfigure each components. 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;
> > > > > > +	}
> > > > > > +
> > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > +	if (!connector) {
> > > > > > +		err = -EINVAL;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> > > > > > +	if (err < 0)
> > > > > > +		goto unlock;
> > > > > > +
> > > > > > +	/* Restore original state to re-enable the connector */
> > > > > > +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > > > > > +
> > > > > > +unlock:
> > > > > > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> > > > > > +	if (!IS_ERR(state))
> > > > > > +		drm_atomic_state_put(state);
> > > > > > +	return err;
> > > > > > +}      
> > > > > 
> > > > > In the previous version, we advised to create a generic helper similar
> > > > > to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().
> > > > > 
> > > > > It looks like you chose a different path. Can you expand why?
> > > > >     
> > > > 
> > > > I didn't choose a different path.
> > > > I created the drm_atomic_helper_disable_connector(). Maybe it is not enough.    
> > > 
> > > It's not that it's not enough, it's that you're not doing the same
> > > thing, see below.
> > >   
> > > > I can move (copy/paste) sn65dsi83_reset_drm_output() to a new helper:
> > > >   int drm_atomic_helper_disable_output(struct drm_encoder *encoder)
> > > > 
> > > > Is it what you expect?
> > > > 
> > > > Also, are operations done in current sn65dsi83_reset_drm_output() correct
> > > > in order to reset the output? It works on my system but what is your
> > > > feedback on operations performed.    
> > > 
> > > You don't need any of that. Both the reset_pipe() and
> > > intel_modeset_commit_pipes() functions will flag the connectors as
> > > updated in the commit, and the core will consider that it needs to
> > > disable / enable the encoders and bridges below that CRTC.
> > > 
> > > See
> > > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1155
> > > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1476
> > > 
> > > So you really only need to convert any of these two functions into a
> > > helper, and it does exactly what you need.
> > >   
> > 
> > I see but if I set crtc_state->connectors_changed = true; as it is done in
> > reset_pipe(), in my understanding, all outputs will be reset.  
> 
> Not all outputs, all active outputs connected to that CRTC. If you have
> only one encoder connected to that CRTC, which is pretty typical on ARM
> platforms, it's equivalent to what you're asking for.
> 
> And we should probably shut down the CRTC (and thus all active outputs)
> anyway. Some encoders and bridges have internal FIFOs/state machines
> that need to be enabled disabled at specific points during the
> initialization, and the CRTC is a part of that.
> 

Ok, I will convert vc4 reset_pipe() to an atomic helper and use that one.

Thanks for your feedback and explanation.

Best regards,
Hervé