| 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: <20241219151705.5c950f24@bootlin.com>
Date: Thu, 19 Dec 2024 15:17:05 +0100
From: Herve Codina <herve.codina@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
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
Hi Maxime,
On Thu, 19 Dec 2024 15:01:39 +0100
Maxime Ripard <mripard@...nel.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@...nel.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é
Powered by blists - more mailing lists