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

[PATCH v4 0/4] 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 v4 series:
  - rebase on top of v6.14-rc1
  - disable/re-enable irq in the failure detection and reset process
  - Split the patch moving reset_pipe() from VC4 to a new helper
  - Rename the new helper to drm_atomic_helper_reset_crtc()
  - Remove state duplication and use bridge.encoder->crtc

Best regards,
Hervé Codina

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

  - Patch 1:
    No changes

  - Patch 2 and 3 (patch 2 in v3):
    Rename the helper to drm_atomic_helper_reset_crtc()
    Split the patch available in v3 in two patches.

  - Patch 4 (patch 3 in v3):
    Take into account commit d2b8c6d549570 ("drm/bridge: ti-sn65dsi83:
    Add ti,lvds-vod-swing optional properties"), available in v6.14-rc1.
    Disable irq when a fault is detected and re-enable it after the pipe
    reset.
    Remove state duplication and use bridge.encoder->crtc directly

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 (4):
  dt-bindings: display: bridge: sn65dsi83: Add interrupt
  drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()
  drm/vc4: hdmi: Use drm_atomic_helper_reset_crtc()
  drm: bridge: ti-sn65dsi83: Add error recovery mechanism

 .../bindings/display/bridge/ti,sn65dsi83.yaml |   3 +
 drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 131 ++++++++++++++++++
 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, 178 insertions(+), 29 deletions(-)

-- 
2.47.1

[PATCH v4 1/4] 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 bad6f5c81b06..9b5f3f3eab19 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 v4 2/4] drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()

[Herve Codina]

drm_atomic_helper_reset_crtc() allows to reset the CRTC active outputs.

This resets all active components available between the CRTC and
connectors.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8ed186ddaeaf..cac807df8a86 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_crtc - 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_crtc(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_crtc);
+
 /**
  * drm_atomic_helper_shutdown - shutdown all CRTC
  * @dev: DRM device
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9aa0a05aa072..53382fe93537 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_crtc(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 v4 3/4] drm/vc4: hdmi: Use drm_atomic_helper_reset_crtc()

[Herve Codina]

The current code uses a the reset_pipe() local function to reset the
CRTC outputs.

drm_atomic_helper_reset_crtc() has been introduced recently and it
performs exact same operations.

In order to avoid code duplication, use the new helper instead of the
local function.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 47d9ada98430..e878eddc9c3f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -270,34 +270,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)
 {
@@ -376,7 +348,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_crtc(crtc, ctx);
 }
 
 static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
-- 
2.47.1

[PATCH v4 4/4] 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 | 131 ++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index d6814b065897..856fdd774c8c 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>
@@ -159,6 +162,9 @@ struct sn65dsi83 {
 	bool				lvds_dual_link_even_odd_swap;
 	int				lvds_vod_swing_conf[2];
 	int				lvds_term_conf[2];
+	int				irq;
+	struct delayed_work		monitor_work;
+	struct work_struct		reset_work;
 };
 
 static const struct regmap_range sn65dsi83_readable_ranges[] = {
@@ -370,6 +376,95 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
 	return dsi_div - 1;
 }
 
+static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
+{
+	struct drm_device *dev = sn65dsi83->bridge.dev;
+	struct drm_modeset_acquire_ctx ctx;
+	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);
+
+	err = drm_atomic_helper_reset_crtc(sn65dsi83->bridge.encoder->crtc, &ctx);
+
+	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
+
+	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;
+	}
+	if (ctx->irq)
+		enable_irq(ctx->irq);
+}
+
+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) {
+		/*
+		 * IRQ acknowledged is not always possible (the bridge can be in
+		 * a state where it doesn't answer anymore). To prevent an
+		 * interrupt storm, disable interrupt. The interrupt will be
+		 * after the reset.
+		 */
+		if (ctx->irq)
+			disable_irq_nosync(ctx->irq);
+
+		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)
 {
@@ -562,6 +657,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->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,
@@ -570,6 +674,15 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	int ret;
 
+	if (ctx->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);
@@ -820,6 +933,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);
@@ -837,6 +958,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)
@@ -861,6 +984,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) {
+		ctx->irq = client->irq;
+		ret = devm_request_threaded_irq(ctx->dev, ctx->irq, NULL, sn65dsi83_irq,
+						IRQF_ONESHOT, dev_name(ctx->dev), ctx);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request irq\n");
+	}
+
 	dev_set_drvdata(dev, ctx);
 	i2c_set_clientdata(client, ctx);
 
-- 
2.47.1

Re: [PATCH v4 2/4] drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()

[Dmitry Baryshkov]

On Mon, Feb 03, 2025 at 03:58:21PM +0100, Herve Codina wrote:
> drm_atomic_helper_reset_crtc() allows to reset the CRTC active outputs.
> 
> This resets all active components available between the CRTC and
> connectors.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 43 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v4 0/4] Add support for errors recovery in the TI SN65DSI83 bridge driver

[Herve Codina]

Hi all,

Oops, this series doesn't apply on top of v6.14-rc1. My bad, sorry about
that.

Please ignore this series. I will send soon a new iteration fixed.

Apologies,
Hervé

On Mon,  3 Feb 2025 15:58:19 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> 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 v4 series:
>   - rebase on top of v6.14-rc1
>   - disable/re-enable irq in the failure detection and reset process
>   - Split the patch moving reset_pipe() from VC4 to a new helper
>   - Rename the new helper to drm_atomic_helper_reset_crtc()
>   - Remove state duplication and use bridge.encoder->crtc
> 
> Best regards,
> Hervé Codina
> 
> Changes v3 -> v4
>   v3: https://lore.kernel.org/lkml/20250108101907.410456-1-herve.codina@bootlin.com/
> 
>   - Patch 1:
>     No changes
> 
>   - Patch 2 and 3 (patch 2 in v3):
>     Rename the helper to drm_atomic_helper_reset_crtc()
>     Split the patch available in v3 in two patches.
> 
>   - Patch 4 (patch 3 in v3):
>     Take into account commit d2b8c6d549570 ("drm/bridge: ti-sn65dsi83:
>     Add ti,lvds-vod-swing optional properties"), available in v6.14-rc1.
>     Disable irq when a fault is detected and re-enable it after the pipe
>     reset.
>     Remove state duplication and use bridge.encoder->crtc directly
> 
> 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 (4):
>   dt-bindings: display: bridge: sn65dsi83: Add interrupt
>   drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()
>   drm/vc4: hdmi: Use drm_atomic_helper_reset_crtc()
>   drm: bridge: ti-sn65dsi83: Add error recovery mechanism
> 
>  .../bindings/display/bridge/ti,sn65dsi83.yaml |   3 +
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 131 ++++++++++++++++++
>  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, 178 insertions(+), 29 deletions(-)
> 

Re: [PATCH v4 3/4] drm/vc4: hdmi: Use drm_atomic_helper_reset_crtc()

[Dmitry Baryshkov]

On Mon, Feb 03, 2025 at 03:58:22PM +0100, Herve Codina wrote:
> The current code uses a the reset_pipe() local function to reset the
> CRTC outputs.
> 
> drm_atomic_helper_reset_crtc() has been introduced recently and it
> performs exact same operations.
> 
> In order to avoid code duplication, use the new helper instead of the
> local function.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 30 +-----------------------------
>  1 file changed, 1 insertion(+), 29 deletions(-)
> 

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v4 2/4] drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()

[Herve Codina]

Hi Dmitry,

On Mon, 3 Feb 2025 17:56:33 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, Feb 03, 2025 at 03:58:21PM +0100, Herve Codina wrote:
> > drm_atomic_helper_reset_crtc() allows to reset the CRTC active outputs.
> > 
> > This resets all active components available between the CRTC and
> > connectors.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  2 ++
> >  2 files changed, 43 insertions(+)
> >   
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

I messed me up in this v4. It doesn't apply on top of v6.14-rc1.

I already sent a v5 to fix that.

Can you add your tag on the v5?

Of course, I will add them myself if a v6 is needed.

I am sorry about my mistake.
Apologies,
Hervé


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

Re: [PATCH v4 3/4] drm/vc4: hdmi: Use drm_atomic_helper_reset_crtc()

[Herve Codina]

Hi Dmitry,

On Mon, 3 Feb 2025 17:56:46 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, Feb 03, 2025 at 03:58:22PM +0100, Herve Codina wrote:
> > The current code uses a the reset_pipe() local function to reset the
> > CRTC outputs.
> > 
> > drm_atomic_helper_reset_crtc() has been introduced recently and it
> > performs exact same operations.
> > 
> > In order to avoid code duplication, use the new helper instead of the
> > local function.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 30 +-----------------------------
> >  1 file changed, 1 insertion(+), 29 deletions(-)
> >   
> 
> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

I messed me up in this v4. It doesn't apply on top of v6.14-rc1.

I already sent a v5 to fix that.

Can you add your tag on the v5?

Of course, I will add them myself if a v6 is needed.

I am sorry about my mistake.

Apologies,
Hervé

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

Re: [PATCH v4 2/4] drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()

[Simona Vetter]

On Mon, Feb 03, 2025 at 03:58:21PM +0100, Herve Codina wrote:
> drm_atomic_helper_reset_crtc() allows to reset the CRTC active outputs.
> 
> This resets all active components available between the CRTC and
> connectors.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8ed186ddaeaf..cac807df8a86 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_crtc - 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.

I think you definitely want a

	Note: This relies on resetting &drm_crtc_state.connectors_changed.
	For drivers which optimize out unnecessary modesets this will
	result in a no-op commit, achieving nothing.

> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_atomic_helper_reset_crtc(struct drm_crtc *crtc,
> +				 struct drm_modeset_acquire_ctx *ctx)

So this is pretty close to DP drivers doing link-retraining when
reconnecting a cable. Would be really nice if that could also be rolled
out there where it fits, and maybe augment the documentation accordingly
so that dp helpers point at this?

Either way would be good to extend the kerneldoc a bit to explain what
this is good for. Either way.

Acked-by: Simona Vetter <simona.vetter@ffwll.ch>

Cheers, Sima
> +{
> +	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_crtc);
> +
>  /**
>   * drm_atomic_helper_shutdown - shutdown all CRTC
>   * @dev: DRM device
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 9aa0a05aa072..53382fe93537 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_crtc(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
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 2/4] drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()

[Maxime Ripard]

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

On Wed, Feb 19, 2025 at 02:41:48PM +0100, Simona Vetter wrote:
> On Mon, Feb 03, 2025 at 03:58:21PM +0100, Herve Codina wrote:
> > drm_atomic_helper_reset_crtc() allows to reset the CRTC active outputs.
> > 
> > This resets all active components available between the CRTC and
> > connectors.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  2 ++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8ed186ddaeaf..cac807df8a86 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_crtc - 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.
> 
> I think you definitely want a
> 
> 	Note: This relies on resetting &drm_crtc_state.connectors_changed.
> 	For drivers which optimize out unnecessary modesets this will
> 	result in a no-op commit, achieving nothing.
> 
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code on failure.
> > + */
> > +int drm_atomic_helper_reset_crtc(struct drm_crtc *crtc,
> > +				 struct drm_modeset_acquire_ctx *ctx)
> 
> So this is pretty close to DP drivers doing link-retraining when
> reconnecting a cable. Would be really nice if that could also be rolled
> out there where it fits, and maybe augment the documentation accordingly
> so that dp helpers point at this?

Given that the patch is already merged, Herve, could you send a
subsequent patch fixing up the doc at least?

Thanks,
Maxime

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

Re: [PATCH v4 2/4] drm/atomic-helper: Introduce drm_atomic_helper_reset_crtc()

[Herve Codina]

Hi Maxime,

On Thu, 20 Feb 2025 11:44:37 +0100
Maxime Ripard <mripard@kernel.org> wrote:

...
> 
> Given that the patch is already merged, Herve, could you send a
> subsequent patch fixing up the doc at least?

Did the patch:
  https://lore.kernel.org/all/20250220140406.593314-1-herve.codina@bootlin.com/

Best regards,
Hervé