linux-kernel - Re: [PATCH v2 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241219-unnatural-terrestrial-aardwark-d1da17@houat>
Date: Thu, 19 Dec 2024 15:01:39 +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 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.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)