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 <[email protected]>
>> Tested-by: Maud Spierings <[email protected]>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> 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
