| 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: <padhzkj5astckiltvxkcs4xq335crrhf2m6bvii6cezyoyfypq@t535fgjwqzqg>
Date: Mon, 17 Feb 2025 21:36:35 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Maxime Ripard <mripard@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
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>,
Douglas Anderson <dianders@...omium.org>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 31/37] drm/bridge: Provide pointers to the connector
and crtc in bridge state
On Mon, Feb 17, 2025 at 05:41:24PM +0100, Simona Vetter wrote:
> On Thu, Feb 13, 2025 at 03:43:50PM +0100, Maxime Ripard wrote:
> > Now that connectors are no longer necessarily created by the bridges
> > drivers themselves but might be created by drm_bridge_connector, it's
> > pretty hard for bridge drivers to retrieve pointers to the connector and
> > CRTC they are attached to.
> >
> > Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
> > encoder field, and then the drm_encoder crtc field, both of them being
> > deprecated.
>
> Eh, this isn't quite how this works. So unless bridges have become very
> dynamic and gained flexible routing the bridge(->bridge->...)->encoder
> chain is static. And the crtc for an encoder you find by walking the
> connector states in a drm_atomic_state, finding the right one that points
> at your encoder, and then return the ->crtc pointer from that connector
> state.
>
> It's a bit bonkers, but I think it's better to compute this than adding
> more pointers that potentially diverge. Unless there's a grand plan here,
> but then I think we want some safety checks that all the pointers never
> get out of sync here.
Luca is working on bridges hotplug, so connectors are dynamic now. And
at least my humble opinion is that we'd better provide the corresponding
pointers rather than having a lot of boilerplate code in the drivers.
(there are enough drivers which look up connector and/or CRTC) and there
are even more drivers (I think, I haven't actually checked the source
tree) that could have benefited from thaving the connector or CRTC in
the state. Instead they store a pointer to the connector or perform
other fancy tricks.
> -Sima
>
> >
> > And for the connector, since we can have multiple connectors attached to
> > a CRTC, we don't really have a reliable way to get it.
> >
> > Let's provide both pointers in the drm_bridge_state structure so we
> > don't have to follow deprecated, non-atomic, pointers, and be more
> > consistent with the other KMS entities.
> >
> > Signed-off-by: Maxime Ripard <mripard@...nel.org>
> > ---
> > drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++++
> > drivers/gpu/drm/drm_bridge.c | 5 +++++
> > include/drm/drm_atomic.h | 14 ++++++++++++++
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 519228eb109533d2596e899a57b571fa0995824f..66661dca077215b78dffca7bc1712f56d35e3918 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -777,10 +777,15 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
> > * that don't subclass the bridge state.
> > */
> > void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
> > struct drm_bridge_state *state)
> > {
> > + if (state->connector) {
> > + drm_connector_put(state->connector);
> > + state->connector = NULL;
> > + }
> > +
> > kfree(state);
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
> >
> > /**
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index b6d24092674c8fa33d9b6ebab9ece0f91fb8f8ea..db2e9834939217d65720ab7a2f82a9ca3db796b0 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -812,10 +812,15 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
> > bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> > bridge);
> > if (WARN_ON(!bridge_state))
> > return -EINVAL;
> >
> > + bridge_state->crtc = crtc_state->crtc;
> > +
> > + drm_connector_get(conn_state->connector);
> > + bridge_state->connector = conn_state->connector;
> > +
> > if (bridge->funcs->atomic_check) {
> > ret = bridge->funcs->atomic_check(bridge, bridge_state,
> > crtc_state, conn_state);
> > if (ret)
> > return ret;
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 4c673f0698fef6b60f77db980378d5e88e0e250e..293e2538a428bc14013d7fabea57a6b858ed7b47 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -1216,10 +1216,24 @@ struct drm_bridge_state {
> > /**
> > * @bridge: the bridge this state refers to
> > */
> > struct drm_bridge *bridge;
> >
> > + /**
> > + * @crtc: CRTC the bridge is connected to, NULL if disabled.
> > + *
> > + * Do not change this directly.
> > + */
> > + struct drm_crtc *crtc;
> > +
> > + /**
> > + * @connector: The connector the bridge is connected to, NULL if disabled.
> > + *
> > + * Do not change this directly.
> > + */
> > + struct drm_connector *connector;
> > +
> > /**
> > * @input_bus_cfg: input bus configuration
> > */
> > struct drm_bus_cfg input_bus_cfg;
> >
> >
> > --
> > 2.48.0
> >
>
> --
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
With best wishes
Dmitry
Powered by blists - more mailing lists