linux-kernel - Re: [PATCH v3 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] [day] [month] [year] [list]

Message-ID: <20250114135456.5366eb2a@bootlin.com>
Date: Tue, 14 Jan 2025 13:54:56 +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 v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery
 mechanism

Hi Maxime,

On Tue, 14 Jan 2025 08:40:51 +0100
Maxime Ripard <mripard@...nel.org> wrote:

...

> >  
> > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > +{
> > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_connector *connector;
> > +	int err;
> > +
> > +	/*
> > +	 * Reset active outputs of the related CRTC.
> > +	 *
> > +	 * This way, drm core will reconfigure each components in the CRTC
> > +	 * outputs path. 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;
> > +	}  
> 
> No, you must not allocate a new state for this, you need to reuse the
> existing state. You'll find it in bridge->base.state->state.

Thanks for pointing that. I didn't know about bridge->base.state->state.

I will use that if using the state is still relevant (see next comment).

> 
> > +	state->acquire_ctx = &ctx;
> > +
> > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > +							     sn65dsi83->bridge.encoder);
> > +	if (!connector) {
> > +		err = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > +	if (IS_ERR(connector_state)) {
> > +		err = PTR_ERR(connector_state);
> > +		goto unlock;
> > +	}
> > +
> > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > +	if (err < 0)
> > +		goto unlock;  
> 
> And you'll find the crtc in bridge->encoder->crtc.

I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
field available in this structure should not be used by atomic drivers. They
should rely on &drm_connector_state.crtc.

In my case, I have the feeling that I should get the ctrc from the current
state (i.e. bridge->base.state->state) using the sequence provided in this
current patch:
  Retrieve the connector with drm_atomic_get_old_connector_for_encoder()
  Retrieve the connector state with drm_atomic_get_connector_state()

but you pointed out the bridge->encoder->crtc field.

Should I use this field or use the &drm_connector_state.crtc with the drm
connector state retrieved from bridge->base.state->state using the proposed
sequence?

[1] https://elixir.bootlin.com/linux/v6.13-rc1/source/include/drm/drm_encoder.h#L180

Best regards,
Hervé