On Thu, Oct 24, 2024 at 11:55:38AM +0200, 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.
>
> 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, 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 entire
> pipeline.
>
> Signed-off-by: Herve Codina <herve.cod...@bootlin.com>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 128 ++++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 96e829163d87..22975b60e80f 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[] = {
> @@ -321,6 +327,92 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> return dsi_div - 1;
> }
>
> +static int sn65dsi83_reset_pipeline(struct sn65dsi83 *sn65dsi83)
> +{
> + struct drm_device *dev = sn65dsi83->bridge.dev;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + int err;
> +
> + /* Use operation done in drm_atomic_helper_suspend() followed by
> + * operation done in drm_atomic_helper_resume() but without releasing
> + * the lock between suspend()/resume()
> + */
> +
> + 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;
> + }
> +
> + err = drm_atomic_helper_disable_all(dev, &ctx);
> + if (err < 0)
> + goto unlock;
> +
> + drm_mode_config_reset(dev);
> +
> + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
Committing a full atomic state from a bridge driver in an asynchronous
way seems quite uncharted territory, and it worries me. It's also a very
heavyweight, you disable all outputs here, instead of focussing on the
output connected to the bridge. Can you either implement something more
local, resetting the bridge only, or create a core helper to handle this
kind of situation, on a per-output basis ?
> +
> +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 pipeline\n");
> +
> + /* Reset the pipeline */
> + ret = sn65dsi83_reset_pipeline(ctx);
> + if (ret) {
> + dev_err(ctx->dev, "reset pipeline 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)
> {
> @@ -509,6 +601,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,
> @@ -517,6 +618,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);
> @@ -673,6 +783,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);
> @@ -686,6 +804,8 @@ static int sn65dsi83_probe(struct i2c_client *client)
> return -ENOMEM;
>
> 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)
> @@ -710,6 +830,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);
>
--
Regards,
Laurent Pinchart
