| 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: <20241218-armored-fearless-dugong-ccd72d@houat>
Date: Wed, 18 Dec 2024 16:54:02 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Herve Codina <herve.codina@...tlin.com>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 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>,
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,
Louis Chauvet <louis.chauvet@...tlin.com>, Luca Ceresoli <luca.ceresoli@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 3/3] drm: bridge: ti-sn65dsi83: Add error recovery
mechanism
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@...nel.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@...tlin.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
Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)
Powered by blists - more mailing lists