| 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: <301e7c8f-5a68-45c4-a18d-60e0545efdf0@collabora.com>
Date: Thu, 15 Jan 2026 01:04:04 +0200
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>,
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>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Sandy Huang <hjc@...k-chips.com>, Heiko Stübner
<heiko@...ech.de>, Andy Yan <andy.yan@...k-chips.com>
Cc: kernel@...labora.com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org,
Diederik de Haas <diederik@...ow-tech.com>,
Maud Spierings <maud_spierings@...mail.com>
Subject: Re: [PATCH v2 1/4] drm/bridge: Add ->detect_ctx hook and
drm_bridge_detect_ctx()
Hi Luca,
On 1/14/26 12:08 PM, Luca Ceresoli wrote:
> Hello Cristian,
>
> On Mon Jan 12, 2026 at 11:26 PM CET, Cristian Ciocaltea wrote:
>> Add an atomic variant of the ->detect callback and a new helper to call
>> the hook while passing an optional drm_modeset_acquire_ctx reference.
>>
>> When both ->detect_ctx and ->detect are defined, the latter is ignored.
>> If acquire_ctx is unset, the function takes care of the locking,
>> while also handling EDEADLK.
>>
>> Tested-by: Diederik de Haas <diederik@...ow-tech.com>
>> Tested-by: Maud Spierings <maud_spierings@...mail.com>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>> drivers/gpu/drm/drm_bridge.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
>> include/drm/drm_bridge.h | 30 +++++++++++++++++++++++
>> 2 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 6dcf8f6d3ecf..0ef12bf98011 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -1344,6 +1344,64 @@ drm_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
>> }
>> EXPORT_SYMBOL_GPL(drm_bridge_detect);
>>
>> +/**
>> + * drm_bridge_detect_ctx - check if anything is attached to the bridge output
>> + * @bridge: bridge control structure
>> + * @connector: attached connector
>> + * @ctx: acquire_ctx, or NULL to let this function handle locking
>> + *
>> + * If the bridge supports output detection, as reported by the
>> + * DRM_BRIDGE_OP_DETECT bridge ops flag, call &drm_bridge_funcs.detect_ctx
>> + * or &drm_bridge_funcs.detect for the bridge and return the connection status.
>> + * Otherwise return connector_status_unknown.
>> + *
>> + * When both @ctx and &drm_bridge_funcs.detect_ctx are not set, this helper
>> + * function is equivalent to drm_bridge_detect() above.
>> + *
>> + * RETURNS:
>> + * The detection status on success, or connector_status_unknown if the bridge
>> + * doesn't support output detection.
>> + * If @ctx is set, it might also return -EDEADLK.
>> + */
>> +int drm_bridge_detect_ctx(struct drm_bridge *bridge,
>> + struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx)
>
> Shouldn't this new function return the same type as detect, i.e. enum
> drm_connector_status?
No, because it might also return an error, as documented in the RETURNS section.
Please also check the comments below.
>
> Otherwise (see below)...
>
>> +{
>> + if (!(bridge->ops & DRM_BRIDGE_OP_DETECT))
>> + return connector_status_unknown;
>> +
>> + if (bridge->funcs->detect_ctx) {
>> + struct drm_modeset_acquire_ctx br_ctx;
>> + int ret;
>> +
>> + if (ctx)
>> + return bridge->funcs->detect_ctx(bridge, connector, ctx);
>> +
>> + drm_modeset_acquire_init(&br_ctx, 0);
>> +retry:
>> + ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex,
>> + &br_ctx);
>> + if (!ret)
>> + ret = bridge->funcs->detect_ctx(bridge, connector, &br_ctx);
>> +
>> + if (ret == -EDEADLK) {
>> + drm_modeset_backoff(&br_ctx);
>> + goto retry;
>> + }
>> +
>> + if (ret < 0)
>> + ret = connector_status_unknown;
>> +
>> + drm_modeset_drop_locks(&br_ctx);
>> + drm_modeset_acquire_fini(&br_ctx);
>> +
>> + return ret;
>> + }
>> +
>> + return bridge->funcs->detect(bridge, connector);
>
> ...here you're converting an enum into an int, which is ok-isk but not
> ideal.
We already have a similar approach with drm_connector_helper_funcs.detect_ctx()
which is expected to return drm_connector_status or the error from
drm_modeset_lock().
>
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -664,6 +664,33 @@ struct drm_bridge_funcs {
>> enum drm_connector_status (*detect)(struct drm_bridge *bridge,
>> struct drm_connector *connector);
>>
>> + /**
>> + * @detect_ctx:
>> + *
>> + * Check if anything is attached to the bridge output.
>> + *
>> + * This callback is optional, if not implemented the bridge will be
>> + * considered as always having a component attached to its output.
>> + * Bridges that implement this callback shall set the
>> + * DRM_BRIDGE_OP_DETECT flag in their &drm_bridge->ops.
>> + *
>> + * This is the atomic version of &drm_bridge_funcs.detect.
>
> I may be missing something, but I'm a bit puzzled by the "atomic" word
> here. For other funcs in this struct there's the old non-atomic func X and
> the new atomic_X func that receives a pointer to struct drm_atomic_state.
>
> Here I think you are using "atomic" with a more generic meaning. Maybe we'd
> better use another wording to not confuse readers?
This is once again consistent with drm_connector_helper_funcs.detect_ctx()
stating:
"This is the atomic version of &drm_connector_funcs.detect."
I'm open for changes, but then we should probably do this across all relevant
funcs, beyond current struct scope.
>> + *
>> + * To avoid races against concurrent connector state updates, the
>> + * helper libraries always call this with ctx set to a valid context,
>> + * and &drm_mode_config.connection_mutex will always be locked with
>> + * the ctx parameter set to this ctx. This allows taking additional
>> + * locks as required.
>> + *
>> + * RETURNS:
>> + *
>> + * &drm_connector_status indicating the bridge output status,
>> + * or the error code returned by drm_modeset_lock(), -EDEADLK.
>> + */
>> + int (*detect_ctx)(struct drm_bridge *bridge,
>> + struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx);
>
> As above, shouldn't this new func return the same type as detect?
Nope, as explained above.
Thanks,
Cristian
Powered by blists - more mailing lists