| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2ee13136.7f7c.192d74c4778.Coremail.andyshrk@163.com>
Date: Tue, 29 Oct 2024 16:02:33 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Laurent Pinchart" <laurent.pinchart@...asonboard.com>
Cc: "Herve Codina" <herve.codina@...tlin.com>,
"Andrzej Hajda" <andrzej.hajda@...el.com>,
"Neil Armstrong" <neil.armstrong@...aro.org>,
"Robert Foss" <rfoss@...nel.org>, "Jonas Karlman" <jonas@...boo.se>,
"Jernej Skrabec" <jernej.skrabec@...il.com>,
"David Airlie" <airlied@...il.com>,
"Simona Vetter" <simona@...ll.ch>,
"Maarten Lankhorst" <maarten.lankhorst@...ux.intel.com>,
"Maxime Ripard" <mripard@...nel.org>,
"Thomas Zimmermann" <tzimmermann@...e.de>,
"Rob Herring" <robh@...nel.org>,
"Krzysztof Kozlowski" <krzk+dt@...nel.org>,
"Conor Dooley" <conor+dt@...nel.org>, "Marek Vasut" <marex@...x.de>,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org,
"Luca Ceresoli" <luca.ceresoli@...tlin.com>,
"Thomas Petazzoni" <thomas.petazzoni@...tlin.com>
Subject: Re:Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery
mechanism
Hi all，
At 2024-10-28 00:23:50, "Laurent Pinchart" <laurent.pinchart@...asonboard.com> wrote:
>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.codina@...tlin.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 ?
If we could simulate a hotplug（disconnected then connected） event to user space and
let userspace do the disable/enable of the output pipeline, would things be simpler?
>
>> +
>> +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
Powered by blists - more mailing lists