[v5,04/16] drm/atomic: Introduce helper to lookup connector by encoder

Commit Message

Maxime Ripard March 4, 2025, 11:10 a.m. UTC

With the bridges switching over to drm_bridge_connector, the direct
association between a bridge driver and its connector was lost.

This is mitigated for atomic bridge drivers by the fact you can access
the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.

This was also made easier by providing drm_atomic_state directly to all
atomic hooks bridges can implement.

However, bridge drivers don't have a way to access drm_atomic_state
outside of the modeset path, like from the hotplug interrupt path or any
interrupt handler.

Let's introduce a function to retrieve the connector currently assigned
to an encoder, without using drm_atomic_state, to make these drivers'
life easier.

Reviewed-by: Dmitry Baryshkov <[e-mail]>
Co-developed-by: Simona Vetter <[e-mail]>
Signed-off-by: Maxime Ripard <[e-mail]>
---
 drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     |  3 +++
 2 files changed, 48 insertions(+)

Comments

Andy Yan March 5, 2025, 11:55 a.m. UTC | #1

Hi Maxime,

At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>With the bridges switching over to drm_bridge_connector, the direct
>association between a bridge driver and its connector was lost.
>
>This is mitigated for atomic bridge drivers by the fact you can access
>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>
>This was also made easier by providing drm_atomic_state directly to all
>atomic hooks bridges can implement.
>
>However, bridge drivers don't have a way to access drm_atomic_state
>outside of the modeset path, like from the hotplug interrupt path or any
>interrupt handler.
>
>Let's introduce a function to retrieve the connector currently assigned
>to an encoder, without using drm_atomic_state, to make these drivers'
>life easier.
>
>Reviewed-by: Dmitry Baryshkov <[e-mail]>
>Co-developed-by: Simona Vetter <[e-mail]>
>Signed-off-by: Maxime Ripard <[e-mail]>
>---
> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_atomic.h     |  3 +++
> 2 files changed, 48 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> 
> 	return NULL;
> }
> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> 
>+/**
>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>+ * @encoder: The encoder to find the connector of
>+ * @ctx: Modeset locking context
>+ *
>+ * This function finds and returns the connector currently assigned to
>+ * an @encoder.
>+ *
>+ * Returns:
>+ * The connector connected to @encoder, or an error pointer otherwise.
>+ * When the error is EDEADLK, a deadlock has been detected and the
>+ * sequence must be restarted.
>+ */
>+struct drm_connector *
>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>+				     struct drm_modeset_acquire_ctx *ctx)
>+{
>+	struct drm_connector_list_iter conn_iter;
>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>+	struct drm_connector *connector;
>+	struct drm_device *dev = encoder->dev;
>+	int ret;
>+
>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>+	if (ret)
>+		return ERR_PTR(ret);

It seems that this will cause a deadlock when called from a  hotplug handling path,
I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
which will hold connection_mutex first, so the deaklock happens:


drm_helper_probe_detect(struct drm_connector *connector,
                        struct drm_modeset_acquire_ctx *ctx,
                        bool force)
{
        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
        struct drm_device *dev = connector->dev;
        int ret;

        if (!ctx)
                return drm_helper_probe_detect_ctx(connector, force);

        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
        if (ret)
                return ret;

        if (funcs->detect_ctx)
                ret = funcs->detect_ctx(connector, ctx, force);
        else if (connector->funcs->detect)
                ret = connector->funcs->detect(connector, force);
        else
                ret = connector_status_connected;

        if (ret != connector->status)
                connector->epoch_counter += 1;

So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?



[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
>+
>+	drm_connector_list_iter_begin(dev, &conn_iter);
>+	drm_for_each_connector_iter(connector, &conn_iter) {
>+		if (!connector->state)
>+			continue;
>+
>+		if (encoder == connector->state->best_encoder) {
>+			out_connector = connector;
>+			break;
>+		}
>+	}
>+	drm_connector_list_iter_end(&conn_iter);
>+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>+
>+	return out_connector;
>+}
>+EXPORT_SYMBOL(drm_atomic_get_connector_for_encoder);
>+
>+
> /**
>  * drm_atomic_get_old_crtc_for_encoder - Get old crtc for an encoder
>  * @state: Atomic state
>  * @encoder: The encoder to fetch the crtc state for
>  *
>diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>index 4c673f0698fef6b60f77db980378d5e88e0e250e..38636a593c9d98cadda85ccd67326cb152f0dd27 100644
>--- a/include/drm/drm_atomic.h
>+++ b/include/drm/drm_atomic.h
>@@ -623,10 +623,13 @@ struct drm_connector *
> drm_atomic_get_old_connector_for_encoder(const struct drm_atomic_state *state,
> 					 struct drm_encoder *encoder);
> struct drm_connector *
> drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> 					 struct drm_encoder *encoder);
>+struct drm_connector *
>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>+				     struct drm_modeset_acquire_ctx *ctx);
> 
> struct drm_crtc *
> drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
> 					 struct drm_encoder *encoder);
> struct drm_crtc *
>
>-- 
>2.48.1

Maxime Ripard March 5, 2025, 1:19 p.m. UTC | #2

Hi Andy,

On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
> At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> >With the bridges switching over to drm_bridge_connector, the direct
> >association between a bridge driver and its connector was lost.
> >
> >This is mitigated for atomic bridge drivers by the fact you can access
> >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> >
> >This was also made easier by providing drm_atomic_state directly to all
> >atomic hooks bridges can implement.
> >
> >However, bridge drivers don't have a way to access drm_atomic_state
> >outside of the modeset path, like from the hotplug interrupt path or any
> >interrupt handler.
> >
> >Let's introduce a function to retrieve the connector currently assigned
> >to an encoder, without using drm_atomic_state, to make these drivers'
> >life easier.
> >
> >Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >Co-developed-by: Simona Vetter <[e-mail]>
> >Signed-off-by: Maxime Ripard <[e-mail]>
> >---
> > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_atomic.h     |  3 +++
> > 2 files changed, 48 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> >--- a/drivers/gpu/drm/drm_atomic.c
> >+++ b/drivers/gpu/drm/drm_atomic.c
> >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > 
> > 	return NULL;
> > }
> > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > 
> >+/**
> >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> >+ * @encoder: The encoder to find the connector of
> >+ * @ctx: Modeset locking context
> >+ *
> >+ * This function finds and returns the connector currently assigned to
> >+ * an @encoder.
> >+ *
> >+ * Returns:
> >+ * The connector connected to @encoder, or an error pointer otherwise.
> >+ * When the error is EDEADLK, a deadlock has been detected and the
> >+ * sequence must be restarted.
> >+ */
> >+struct drm_connector *
> >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> >+				     struct drm_modeset_acquire_ctx *ctx)
> >+{
> >+	struct drm_connector_list_iter conn_iter;
> >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> >+	struct drm_connector *connector;
> >+	struct drm_device *dev = encoder->dev;
> >+	int ret;
> >+
> >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >+	if (ret)
> >+		return ERR_PTR(ret);
> 
> It seems that this will cause a deadlock when called from a hotplug
> handling path, I have a WIP DP diver[0], which suggested by Dmitry to
> use this API from a &drm_bridge_funcs.detect callback to get the
> connector, as detect is called by drm_helper_probe_detect, which will
> hold connection_mutex first, so the deaklock happens:
>
> drm_helper_probe_detect(struct drm_connector *connector,
>                         struct drm_modeset_acquire_ctx *ctx,
>                         bool force)
> {
>         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>         struct drm_device *dev = connector->dev;
>         int ret;
> 
>         if (!ctx)
>                 return drm_helper_probe_detect_ctx(connector, force);
> 
>         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>         if (ret)
>                 return ret;
> 
>         if (funcs->detect_ctx)
>                 ret = funcs->detect_ctx(connector, ctx, force);
>         else if (connector->funcs->detect)
>                 ret = connector->funcs->detect(connector, force);
>         else
>                 ret = connector_status_connected;
> 
>         if (ret != connector->status)
>                 connector->epoch_counter += 1;
> 
> So I wonder can we let drm_bridge_funcs.detect pass a connector for
> this case ?

Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
fine with reentrancy from the same context, so it should work just fine.

Maxime

Dmitry Baryshkov March 5, 2025, 8:13 p.m. UTC | #3

On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
> Hi Andy,
> 
> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > >With the bridges switching over to drm_bridge_connector, the direct
> > >association between a bridge driver and its connector was lost.
> > >
> > >This is mitigated for atomic bridge drivers by the fact you can access
> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > >
> > >This was also made easier by providing drm_atomic_state directly to all
> > >atomic hooks bridges can implement.
> > >
> > >However, bridge drivers don't have a way to access drm_atomic_state
> > >outside of the modeset path, like from the hotplug interrupt path or any
> > >interrupt handler.
> > >
> > >Let's introduce a function to retrieve the connector currently assigned
> > >to an encoder, without using drm_atomic_state, to make these drivers'
> > >life easier.
> > >
> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > >Co-developed-by: Simona Vetter <[e-mail]>
> > >Signed-off-by: Maxime Ripard <[e-mail]>
> > >---
> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > include/drm/drm_atomic.h     |  3 +++
> > > 2 files changed, 48 insertions(+)
> > >
> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > >--- a/drivers/gpu/drm/drm_atomic.c
> > >+++ b/drivers/gpu/drm/drm_atomic.c
> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > 
> > > 	return NULL;
> > > }
> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > 
> > >+/**
> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > >+ * @encoder: The encoder to find the connector of
> > >+ * @ctx: Modeset locking context
> > >+ *
> > >+ * This function finds and returns the connector currently assigned to
> > >+ * an @encoder.
> > >+ *
> > >+ * Returns:
> > >+ * The connector connected to @encoder, or an error pointer otherwise.
> > >+ * When the error is EDEADLK, a deadlock has been detected and the
> > >+ * sequence must be restarted.
> > >+ */
> > >+struct drm_connector *
> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > >+				     struct drm_modeset_acquire_ctx *ctx)
> > >+{
> > >+	struct drm_connector_list_iter conn_iter;
> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > >+	struct drm_connector *connector;
> > >+	struct drm_device *dev = encoder->dev;
> > >+	int ret;
> > >+
> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > >+	if (ret)
> > >+		return ERR_PTR(ret);
> > 
> > It seems that this will cause a deadlock when called from a hotplug
> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
> > use this API from a &drm_bridge_funcs.detect callback to get the
> > connector, as detect is called by drm_helper_probe_detect, which will
> > hold connection_mutex first, so the deaklock happens:
> >
> > drm_helper_probe_detect(struct drm_connector *connector,
> >                         struct drm_modeset_acquire_ctx *ctx,
> >                         bool force)
> > {
> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> >         struct drm_device *dev = connector->dev;
> >         int ret;
> > 
> >         if (!ctx)
> >                 return drm_helper_probe_detect_ctx(connector, force);
> > 
> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >         if (ret)
> >                 return ret;
> > 
> >         if (funcs->detect_ctx)
> >                 ret = funcs->detect_ctx(connector, ctx, force);
> >         else if (connector->funcs->detect)
> >                 ret = connector->funcs->detect(connector, force);
> >         else
> >                 ret = connector_status_connected;
> > 
> >         if (ret != connector->status)
> >                 connector->epoch_counter += 1;
> > 
> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
> > this case ?
> 
> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
> fine with reentrancy from the same context, so it should work just fine.

Andy, that probably means that you should use .detect_ctx() and pass the
context to drm_atomic_get_connector_for_encoder().

Andy Yan March 6, 2025, 1:16 a.m. UTC | #4

Hi Maxime and Dmitry:

At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
>On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
>> Hi Andy,
>> 
>> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
>> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>> > >With the bridges switching over to drm_bridge_connector, the direct
>> > >association between a bridge driver and its connector was lost.
>> > >
>> > >This is mitigated for atomic bridge drivers by the fact you can access
>> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>> > >
>> > >This was also made easier by providing drm_atomic_state directly to all
>> > >atomic hooks bridges can implement.
>> > >
>> > >However, bridge drivers don't have a way to access drm_atomic_state
>> > >outside of the modeset path, like from the hotplug interrupt path or any
>> > >interrupt handler.
>> > >
>> > >Let's introduce a function to retrieve the connector currently assigned
>> > >to an encoder, without using drm_atomic_state, to make these drivers'
>> > >life easier.
>> > >
>> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
>> > >Co-developed-by: Simona Vetter <[e-mail]>
>> > >Signed-off-by: Maxime Ripard <[e-mail]>
>> > >---
>> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> > > include/drm/drm_atomic.h     |  3 +++
>> > > 2 files changed, 48 insertions(+)
>> > >
>> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>> > >--- a/drivers/gpu/drm/drm_atomic.c
>> > >+++ b/drivers/gpu/drm/drm_atomic.c
>> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>> > > 
>> > > 	return NULL;
>> > > }
>> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>> > > 
>> > >+/**
>> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>> > >+ * @encoder: The encoder to find the connector of
>> > >+ * @ctx: Modeset locking context
>> > >+ *
>> > >+ * This function finds and returns the connector currently assigned to
>> > >+ * an @encoder.
>> > >+ *
>> > >+ * Returns:
>> > >+ * The connector connected to @encoder, or an error pointer otherwise.
>> > >+ * When the error is EDEADLK, a deadlock has been detected and the
>> > >+ * sequence must be restarted.
>> > >+ */
>> > >+struct drm_connector *
>> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>> > >+				     struct drm_modeset_acquire_ctx *ctx)
>> > >+{
>> > >+	struct drm_connector_list_iter conn_iter;
>> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>> > >+	struct drm_connector *connector;
>> > >+	struct drm_device *dev = encoder->dev;
>> > >+	int ret;
>> > >+
>> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> > >+	if (ret)
>> > >+		return ERR_PTR(ret);
>> > 
>> > It seems that this will cause a deadlock when called from a hotplug
>> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
>> > use this API from a &drm_bridge_funcs.detect callback to get the
>> > connector, as detect is called by drm_helper_probe_detect, which will
>> > hold connection_mutex first, so the deaklock happens:
>> >
>> > drm_helper_probe_detect(struct drm_connector *connector,
>> >                         struct drm_modeset_acquire_ctx *ctx,
>> >                         bool force)
>> > {
>> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>> >         struct drm_device *dev = connector->dev;
>> >         int ret;
>> > 
>> >         if (!ctx)
>> >                 return drm_helper_probe_detect_ctx(connector, force);
>> > 
>> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> >         if (ret)
>> >                 return ret;
>> > 
>> >         if (funcs->detect_ctx)
>> >                 ret = funcs->detect_ctx(connector, ctx, force);
>> >         else if (connector->funcs->detect)
>> >                 ret = connector->funcs->detect(connector, force);
>> >         else
>> >                 ret = connector_status_connected;
>> > 
>> >         if (ret != connector->status)
>> >                 connector->epoch_counter += 1;
>> > 
>> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
>> > this case ?
>> 
>> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
>> fine with reentrancy from the same context, so it should work just fine.
>
>Andy, that probably means that you should use .detect_ctx() and pass the
>context to drm_atomic_get_connector_for_encoder().

Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
The call chain is:
 drm_helper_probe_detect 
 --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
--> drm_bridge_funcs.detect(bridge)
The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
The connector got dropped  when connector call it's bridege.detect

So I think the simplest solution is to have drm_bridge_funcs.detect directly pass the connector

>
>-- 
>With best wishes
>Dmitry

Maxime Ripard March 6, 2025, 7:10 a.m. UTC | #5

On Thu, Mar 06, 2025 at 09:16:24AM +0800, Andy Yan wrote:
> 
> Hi Maxime and Dmitry:
> 
> At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
> >On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
> >> Hi Andy,
> >> 
> >> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
> >> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> >> > >With the bridges switching over to drm_bridge_connector, the direct
> >> > >association between a bridge driver and its connector was lost.
> >> > >
> >> > >This is mitigated for atomic bridge drivers by the fact you can access
> >> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> >> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> >> > >
> >> > >This was also made easier by providing drm_atomic_state directly to all
> >> > >atomic hooks bridges can implement.
> >> > >
> >> > >However, bridge drivers don't have a way to access drm_atomic_state
> >> > >outside of the modeset path, like from the hotplug interrupt path or any
> >> > >interrupt handler.
> >> > >
> >> > >Let's introduce a function to retrieve the connector currently assigned
> >> > >to an encoder, without using drm_atomic_state, to make these drivers'
> >> > >life easier.
> >> > >
> >> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >> > >Co-developed-by: Simona Vetter <[e-mail]>
> >> > >Signed-off-by: Maxime Ripard <[e-mail]>
> >> > >---
> >> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >> > > include/drm/drm_atomic.h     |  3 +++
> >> > > 2 files changed, 48 insertions(+)
> >> > >
> >> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> >> > >--- a/drivers/gpu/drm/drm_atomic.c
> >> > >+++ b/drivers/gpu/drm/drm_atomic.c
> >> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> >> > > 
> >> > > 	return NULL;
> >> > > }
> >> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> >> > > 
> >> > >+/**
> >> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> >> > >+ * @encoder: The encoder to find the connector of
> >> > >+ * @ctx: Modeset locking context
> >> > >+ *
> >> > >+ * This function finds and returns the connector currently assigned to
> >> > >+ * an @encoder.
> >> > >+ *
> >> > >+ * Returns:
> >> > >+ * The connector connected to @encoder, or an error pointer otherwise.
> >> > >+ * When the error is EDEADLK, a deadlock has been detected and the
> >> > >+ * sequence must be restarted.
> >> > >+ */
> >> > >+struct drm_connector *
> >> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> >> > >+				     struct drm_modeset_acquire_ctx *ctx)
> >> > >+{
> >> > >+	struct drm_connector_list_iter conn_iter;
> >> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> >> > >+	struct drm_connector *connector;
> >> > >+	struct drm_device *dev = encoder->dev;
> >> > >+	int ret;
> >> > >+
> >> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >> > >+	if (ret)
> >> > >+		return ERR_PTR(ret);
> >> > 
> >> > It seems that this will cause a deadlock when called from a hotplug
> >> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
> >> > use this API from a &drm_bridge_funcs.detect callback to get the
> >> > connector, as detect is called by drm_helper_probe_detect, which will
> >> > hold connection_mutex first, so the deaklock happens:
> >> >
> >> > drm_helper_probe_detect(struct drm_connector *connector,
> >> >                         struct drm_modeset_acquire_ctx *ctx,
> >> >                         bool force)
> >> > {
> >> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> >> >         struct drm_device *dev = connector->dev;
> >> >         int ret;
> >> > 
> >> >         if (!ctx)
> >> >                 return drm_helper_probe_detect_ctx(connector, force);
> >> > 
> >> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >> >         if (ret)
> >> >                 return ret;
> >> > 
> >> >         if (funcs->detect_ctx)
> >> >                 ret = funcs->detect_ctx(connector, ctx, force);
> >> >         else if (connector->funcs->detect)
> >> >                 ret = connector->funcs->detect(connector, force);
> >> >         else
> >> >                 ret = connector_status_connected;
> >> > 
> >> >         if (ret != connector->status)
> >> >                 connector->epoch_counter += 1;
> >> > 
> >> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
> >> > this case ?
> >> 
> >> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
> >> fine with reentrancy from the same context, so it should work just fine.
> >
> >Andy, that probably means that you should use .detect_ctx() and pass the
> >context to drm_atomic_get_connector_for_encoder().
> 
> Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
> The call chain is:
>  drm_helper_probe_detect 
>  --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> --> drm_bridge_funcs.detect(bridge)
> The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
> The connector got dropped  when connector call it's bridege.detect
> 
> So I think the simplest solution is to have drm_bridge_funcs.detect
> directly pass the connector

I don't disagree on principle, but I think a better first step would be
to provide a detect_ctx hook to bridges.

Maxime

Herve Codina March 6, 2025, 8:21 a.m. UTC | #6

Hi Maxime,
Hi Maxime,

On Tue, 04 Mar 2025 12:10:47 +0100
Maxime Ripard <[e-mail]> wrote:

> With the bridges switching over to drm_bridge_connector, the direct
> association between a bridge driver and its connector was lost.
> 
> This is mitigated for atomic bridge drivers by the fact you can access
> the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> 
> This was also made easier by providing drm_atomic_state directly to all
> atomic hooks bridges can implement.
> 
> However, bridge drivers don't have a way to access drm_atomic_state
> outside of the modeset path, like from the hotplug interrupt path or any
> interrupt handler.
> 
> Let's introduce a function to retrieve the connector currently assigned
> to an encoder, without using drm_atomic_state, to make these drivers'
> life easier.
> 
> Reviewed-by: Dmitry Baryshkov <[e-mail]>
> Co-developed-by: Simona Vetter <[e-mail]>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  3 +++
>  2 files changed, 48 insertions(+)
> 

Tested the drm_atomic_get_connector_for_encoder() in the context of the
ti-sn65dsi83 driver recovery process (later modification in this series).

Tested-by: Herve Codina <[e-mail]>

Best regards,
Hervé

Simona Vetter March 6, 2025, 3:41 p.m. UTC | #7

On Thu, Mar 06, 2025 at 08:10:16AM +0100, Maxime Ripard wrote:
> On Thu, Mar 06, 2025 at 09:16:24AM +0800, Andy Yan wrote:
> > 
> > Hi Maxime and Dmitry:
> > 
> > At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
> > >On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
> > >> Hi Andy,
> > >> 
> > >> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
> > >> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > >> > >With the bridges switching over to drm_bridge_connector, the direct
> > >> > >association between a bridge driver and its connector was lost.
> > >> > >
> > >> > >This is mitigated for atomic bridge drivers by the fact you can access
> > >> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > >> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > >> > >
> > >> > >This was also made easier by providing drm_atomic_state directly to all
> > >> > >atomic hooks bridges can implement.
> > >> > >
> > >> > >However, bridge drivers don't have a way to access drm_atomic_state
> > >> > >outside of the modeset path, like from the hotplug interrupt path or any
> > >> > >interrupt handler.
> > >> > >
> > >> > >Let's introduce a function to retrieve the connector currently assigned
> > >> > >to an encoder, without using drm_atomic_state, to make these drivers'
> > >> > >life easier.
> > >> > >
> > >> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > >> > >Co-developed-by: Simona Vetter <[e-mail]>
> > >> > >Signed-off-by: Maxime Ripard <[e-mail]>
> > >> > >---
> > >> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > >> > > include/drm/drm_atomic.h     |  3 +++
> > >> > > 2 files changed, 48 insertions(+)
> > >> > >
> > >> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > >> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > >> > >--- a/drivers/gpu/drm/drm_atomic.c
> > >> > >+++ b/drivers/gpu/drm/drm_atomic.c
> > >> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > >> > > 
> > >> > > 	return NULL;
> > >> > > }
> > >> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > >> > > 
> > >> > >+/**
> > >> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > >> > >+ * @encoder: The encoder to find the connector of
> > >> > >+ * @ctx: Modeset locking context
> > >> > >+ *
> > >> > >+ * This function finds and returns the connector currently assigned to
> > >> > >+ * an @encoder.
> > >> > >+ *
> > >> > >+ * Returns:
> > >> > >+ * The connector connected to @encoder, or an error pointer otherwise.
> > >> > >+ * When the error is EDEADLK, a deadlock has been detected and the
> > >> > >+ * sequence must be restarted.
> > >> > >+ */
> > >> > >+struct drm_connector *
> > >> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > >> > >+				     struct drm_modeset_acquire_ctx *ctx)
> > >> > >+{
> > >> > >+	struct drm_connector_list_iter conn_iter;
> > >> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > >> > >+	struct drm_connector *connector;
> > >> > >+	struct drm_device *dev = encoder->dev;
> > >> > >+	int ret;
> > >> > >+
> > >> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > >> > >+	if (ret)
> > >> > >+		return ERR_PTR(ret);
> > >> > 
> > >> > It seems that this will cause a deadlock when called from a hotplug
> > >> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
> > >> > use this API from a &drm_bridge_funcs.detect callback to get the
> > >> > connector, as detect is called by drm_helper_probe_detect, which will
> > >> > hold connection_mutex first, so the deaklock happens:
> > >> >
> > >> > drm_helper_probe_detect(struct drm_connector *connector,
> > >> >                         struct drm_modeset_acquire_ctx *ctx,
> > >> >                         bool force)
> > >> > {
> > >> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > >> >         struct drm_device *dev = connector->dev;
> > >> >         int ret;
> > >> > 
> > >> >         if (!ctx)
> > >> >                 return drm_helper_probe_detect_ctx(connector, force);
> > >> > 
> > >> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > >> >         if (ret)
> > >> >                 return ret;
> > >> > 
> > >> >         if (funcs->detect_ctx)
> > >> >                 ret = funcs->detect_ctx(connector, ctx, force);
> > >> >         else if (connector->funcs->detect)
> > >> >                 ret = connector->funcs->detect(connector, force);
> > >> >         else
> > >> >                 ret = connector_status_connected;
> > >> > 
> > >> >         if (ret != connector->status)
> > >> >                 connector->epoch_counter += 1;
> > >> > 
> > >> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
> > >> > this case ?
> > >> 
> > >> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
> > >> fine with reentrancy from the same context, so it should work just fine.
> > >
> > >Andy, that probably means that you should use .detect_ctx() and pass the
> > >context to drm_atomic_get_connector_for_encoder().
> > 
> > Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
> > The call chain is:
> >  drm_helper_probe_detect 
> >  --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> > --> drm_bridge_funcs.detect(bridge)
> > The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
> > The connector got dropped  when connector call it's bridege.detect
> > 
> > So I think the simplest solution is to have drm_bridge_funcs.detect
> > directly pass the connector
> 
> I don't disagree on principle, but I think a better first step would be
> to provide a detect_ctx hook to bridges.

Yup. There's other reasons you really want to get at the locking context
in detect callbacks, doing this special case by passing something for
everyone doesn't sound like the right approach to me.
-Sima

Simona Vetter March 6, 2025, 3:44 p.m. UTC | #8

On Tue, Mar 04, 2025 at 12:10:47PM +0100, Maxime Ripard wrote:
> With the bridges switching over to drm_bridge_connector, the direct
> association between a bridge driver and its connector was lost.
> 
> This is mitigated for atomic bridge drivers by the fact you can access
> the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> 
> This was also made easier by providing drm_atomic_state directly to all
> atomic hooks bridges can implement.
> 
> However, bridge drivers don't have a way to access drm_atomic_state
> outside of the modeset path, like from the hotplug interrupt path or any
> interrupt handler.
> 
> Let's introduce a function to retrieve the connector currently assigned
> to an encoder, without using drm_atomic_state, to make these drivers'
> life easier.
> 
> Reviewed-by: Dmitry Baryshkov <[e-mail]>
> Co-developed-by: Simona Vetter <[e-mail]>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  3 +++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>  
>  	return NULL;
>  }
>  EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>  
> +/**
> + * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> + * @encoder: The encoder to find the connector of
> + * @ctx: Modeset locking context
> + *
> + * This function finds and returns the connector currently assigned to
> + * an @encoder.

I think it'd be good to link to the other atomic connector functions like
drm_atomic_get_old/new_connector_for_encoder and explain when to use them.
So also add links to the kerneldoc of these other functions pointing to
here.

- This function is for detect, link repair or anything else that comes
  from the hardware. Or in general, anything that's not in atomic
  commit/check code paths.

- In atomic check/commit code you want to use the other functions.

Otherwise people will have an even harder time finding the right variant
in this maze of look-alikes :-)

With the kerneldoc suitably polished:

Reviewed-by: Simona Vetter <[e-mail]>
Signed-off-by: Simona Vetter <[e-mail]>

> + *
> + * Returns:
> + * The connector connected to @encoder, or an error pointer otherwise.
> + * When the error is EDEADLK, a deadlock has been detected and the
> + * sequence must be restarted.
> + */
> +struct drm_connector *
> +drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> +				     struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> +	struct drm_connector *connector;
> +	struct drm_device *dev = encoder->dev;
> +	int ret;
> +
> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (!connector->state)
> +			continue;
> +
> +		if (encoder == connector->state->best_encoder) {
> +			out_connector = connector;
> +			break;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +	return out_connector;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_connector_for_encoder);
> +
> +
>  /**
>   * drm_atomic_get_old_crtc_for_encoder - Get old crtc for an encoder
>   * @state: Atomic state
>   * @encoder: The encoder to fetch the crtc state for
>   *
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 4c673f0698fef6b60f77db980378d5e88e0e250e..38636a593c9d98cadda85ccd67326cb152f0dd27 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -623,10 +623,13 @@ struct drm_connector *
>  drm_atomic_get_old_connector_for_encoder(const struct drm_atomic_state *state,
>  					 struct drm_encoder *encoder);
>  struct drm_connector *
>  drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>  					 struct drm_encoder *encoder);
> +struct drm_connector *
> +drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> +				     struct drm_modeset_acquire_ctx *ctx);
>  
>  struct drm_crtc *
>  drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
>  					 struct drm_encoder *encoder);
>  struct drm_crtc *
> 
> -- 
> 2.48.1
>

Andy Yan March 7, 2025, 1:08 a.m. UTC | #9

Hi All,

At 2025-03-06 23:41:24, "Simona Vetter" <[e-mail]> wrote:
>On Thu, Mar 06, 2025 at 08:10:16AM +0100, Maxime Ripard wrote:
>> On Thu, Mar 06, 2025 at 09:16:24AM +0800, Andy Yan wrote:
>> > 
>> > Hi Maxime and Dmitry:
>> > 
>> > At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
>> > >On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
>> > >> Hi Andy,
>> > >> 
>> > >> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
>> > >> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>> > >> > >With the bridges switching over to drm_bridge_connector, the direct
>> > >> > >association between a bridge driver and its connector was lost.
>> > >> > >
>> > >> > >This is mitigated for atomic bridge drivers by the fact you can access
>> > >> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>> > >> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>> > >> > >
>> > >> > >This was also made easier by providing drm_atomic_state directly to all
>> > >> > >atomic hooks bridges can implement.
>> > >> > >
>> > >> > >However, bridge drivers don't have a way to access drm_atomic_state
>> > >> > >outside of the modeset path, like from the hotplug interrupt path or any
>> > >> > >interrupt handler.
>> > >> > >
>> > >> > >Let's introduce a function to retrieve the connector currently assigned
>> > >> > >to an encoder, without using drm_atomic_state, to make these drivers'
>> > >> > >life easier.
>> > >> > >
>> > >> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
>> > >> > >Co-developed-by: Simona Vetter <[e-mail]>
>> > >> > >Signed-off-by: Maxime Ripard <[e-mail]>
>> > >> > >---
>> > >> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> > >> > > include/drm/drm_atomic.h     |  3 +++
>> > >> > > 2 files changed, 48 insertions(+)
>> > >> > >
>> > >> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > >> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>> > >> > >--- a/drivers/gpu/drm/drm_atomic.c
>> > >> > >+++ b/drivers/gpu/drm/drm_atomic.c
>> > >> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>> > >> > > 
>> > >> > > 	return NULL;
>> > >> > > }
>> > >> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>> > >> > > 
>> > >> > >+/**
>> > >> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>> > >> > >+ * @encoder: The encoder to find the connector of
>> > >> > >+ * @ctx: Modeset locking context
>> > >> > >+ *
>> > >> > >+ * This function finds and returns the connector currently assigned to
>> > >> > >+ * an @encoder.
>> > >> > >+ *
>> > >> > >+ * Returns:
>> > >> > >+ * The connector connected to @encoder, or an error pointer otherwise.
>> > >> > >+ * When the error is EDEADLK, a deadlock has been detected and the
>> > >> > >+ * sequence must be restarted.
>> > >> > >+ */
>> > >> > >+struct drm_connector *
>> > >> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>> > >> > >+				     struct drm_modeset_acquire_ctx *ctx)
>> > >> > >+{
>> > >> > >+	struct drm_connector_list_iter conn_iter;
>> > >> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>> > >> > >+	struct drm_connector *connector;
>> > >> > >+	struct drm_device *dev = encoder->dev;
>> > >> > >+	int ret;
>> > >> > >+
>> > >> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> > >> > >+	if (ret)
>> > >> > >+		return ERR_PTR(ret);
>> > >> > 
>> > >> > It seems that this will cause a deadlock when called from a hotplug
>> > >> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
>> > >> > use this API from a &drm_bridge_funcs.detect callback to get the
>> > >> > connector, as detect is called by drm_helper_probe_detect, which will
>> > >> > hold connection_mutex first, so the deaklock happens:
>> > >> >
>> > >> > drm_helper_probe_detect(struct drm_connector *connector,
>> > >> >                         struct drm_modeset_acquire_ctx *ctx,
>> > >> >                         bool force)
>> > >> > {
>> > >> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>> > >> >         struct drm_device *dev = connector->dev;
>> > >> >         int ret;
>> > >> > 
>> > >> >         if (!ctx)
>> > >> >                 return drm_helper_probe_detect_ctx(connector, force);
>> > >> > 
>> > >> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> > >> >         if (ret)
>> > >> >                 return ret;
>> > >> > 
>> > >> >         if (funcs->detect_ctx)
>> > >> >                 ret = funcs->detect_ctx(connector, ctx, force);
>> > >> >         else if (connector->funcs->detect)
>> > >> >                 ret = connector->funcs->detect(connector, force);
>> > >> >         else
>> > >> >                 ret = connector_status_connected;
>> > >> > 
>> > >> >         if (ret != connector->status)
>> > >> >                 connector->epoch_counter += 1;
>> > >> > 
>> > >> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
>> > >> > this case ?
>> > >> 
>> > >> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
>> > >> fine with reentrancy from the same context, so it should work just fine.
>> > >
>> > >Andy, that probably means that you should use .detect_ctx() and pass the
>> > >context to drm_atomic_get_connector_for_encoder().
>> > 
>> > Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
>> > The call chain is:
>> >  drm_helper_probe_detect 
>> >  --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>> > --> drm_bridge_funcs.detect(bridge)
>> > The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
>> > The connector got dropped  when connector call it's bridege.detect
>> > 
>> > So I think the simplest solution is to have drm_bridge_funcs.detect
>> > directly pass the connector
>> 
>> I don't disagree on principle, but I think a better first step would be
>> to provide a detect_ctx hook to bridges.
>
>Yup. There's other reasons you really want to get at the locking context
>in detect callbacks, doing this special case by passing something for
>everyone doesn't sound like the right approach to me.

Ok, I will add a detect_ctx  hook for bridge. Thanks for your advice.

Just confirm that can I send this add detect_ctx hook patch alone first? 
I think this patch will be easy to merge,  so it can help my WIP DP driver stay light on dependencies。


>-Sima
>-- 
>Simona Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

Andy Yan March 7, 2025, 7:30 a.m. UTC | #10

Hi All,
At 2025-03-07 09:08:48, "Andy Yan" <[e-mail]> wrote:
>Hi All,
>
>At 2025-03-06 23:41:24, "Simona Vetter" <[e-mail]> wrote:
>>On Thu, Mar 06, 2025 at 08:10:16AM +0100, Maxime Ripard wrote:
>>> On Thu, Mar 06, 2025 at 09:16:24AM +0800, Andy Yan wrote:
>>> > 
>>> > Hi Maxime and Dmitry:
>>> > 
>>> > At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
>>> > >On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
>>> > >> Hi Andy,
>>> > >> 
>>> > >> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
>>> > >> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>>> > >> > >With the bridges switching over to drm_bridge_connector, the direct
>>> > >> > >association between a bridge driver and its connector was lost.
>>> > >> > >
>>> > >> > >This is mitigated for atomic bridge drivers by the fact you can access
>>> > >> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>>> > >> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>>> > >> > >
>>> > >> > >This was also made easier by providing drm_atomic_state directly to all
>>> > >> > >atomic hooks bridges can implement.
>>> > >> > >
>>> > >> > >However, bridge drivers don't have a way to access drm_atomic_state
>>> > >> > >outside of the modeset path, like from the hotplug interrupt path or any
>>> > >> > >interrupt handler.
>>> > >> > >
>>> > >> > >Let's introduce a function to retrieve the connector currently assigned
>>> > >> > >to an encoder, without using drm_atomic_state, to make these drivers'
>>> > >> > >life easier.
>>> > >> > >
>>> > >> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
>>> > >> > >Co-developed-by: Simona Vetter <[e-mail]>
>>> > >> > >Signed-off-by: Maxime Ripard <[e-mail]>
>>> > >> > >---
>>> > >> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>> > >> > > include/drm/drm_atomic.h     |  3 +++
>>> > >> > > 2 files changed, 48 insertions(+)
>>> > >> > >
>>> > >> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> > >> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>>> > >> > >--- a/drivers/gpu/drm/drm_atomic.c
>>> > >> > >+++ b/drivers/gpu/drm/drm_atomic.c
>>> > >> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>>> > >> > > 
>>> > >> > > 	return NULL;
>>> > >> > > }
>>> > >> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>>> > >> > > 
>>> > >> > >+/**
>>> > >> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>>> > >> > >+ * @encoder: The encoder to find the connector of
>>> > >> > >+ * @ctx: Modeset locking context
>>> > >> > >+ *
>>> > >> > >+ * This function finds and returns the connector currently assigned to
>>> > >> > >+ * an @encoder.
>>> > >> > >+ *
>>> > >> > >+ * Returns:
>>> > >> > >+ * The connector connected to @encoder, or an error pointer otherwise.
>>> > >> > >+ * When the error is EDEADLK, a deadlock has been detected and the
>>> > >> > >+ * sequence must be restarted.
>>> > >> > >+ */
>>> > >> > >+struct drm_connector *
>>> > >> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>>> > >> > >+				     struct drm_modeset_acquire_ctx *ctx)
>>> > >> > >+{
>>> > >> > >+	struct drm_connector_list_iter conn_iter;
>>> > >> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>>> > >> > >+	struct drm_connector *connector;
>>> > >> > >+	struct drm_device *dev = encoder->dev;
>>> > >> > >+	int ret;
>>> > >> > >+
>>> > >> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>>> > >> > >+	if (ret)
>>> > >> > >+		return ERR_PTR(ret);
>>> > >> > 
>>> > >> > It seems that this will cause a deadlock when called from a hotplug
>>> > >> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
>>> > >> > use this API from a &drm_bridge_funcs.detect callback to get the
>>> > >> > connector, as detect is called by drm_helper_probe_detect, which will
>>> > >> > hold connection_mutex first, so the deaklock happens:
>>> > >> >
>>> > >> > drm_helper_probe_detect(struct drm_connector *connector,
>>> > >> >                         struct drm_modeset_acquire_ctx *ctx,
>>> > >> >                         bool force)
>>> > >> > {
>>> > >> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>>> > >> >         struct drm_device *dev = connector->dev;
>>> > >> >         int ret;
>>> > >> > 
>>> > >> >         if (!ctx)
>>> > >> >                 return drm_helper_probe_detect_ctx(connector, force);
>>> > >> > 
>>> > >> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>>> > >> >         if (ret)
>>> > >> >                 return ret;
>>> > >> > 
>>> > >> >         if (funcs->detect_ctx)
>>> > >> >                 ret = funcs->detect_ctx(connector, ctx, force);
>>> > >> >         else if (connector->funcs->detect)
>>> > >> >                 ret = connector->funcs->detect(connector, force);
>>> > >> >         else
>>> > >> >                 ret = connector_status_connected;
>>> > >> > 
>>> > >> >         if (ret != connector->status)
>>> > >> >                 connector->epoch_counter += 1;
>>> > >> > 
>>> > >> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
>>> > >> > this case ?
>>> > >> 
>>> > >> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
>>> > >> fine with reentrancy from the same context, so it should work just fine.
>>> > >
>>> > >Andy, that probably means that you should use .detect_ctx() and pass the
>>> > >context to drm_atomic_get_connector_for_encoder().
>>> > 
>>> > Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
>>> > The call chain is:
>>> >  drm_helper_probe_detect 
>>> >  --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>>> > --> drm_bridge_funcs.detect(bridge)
>>> > The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
>>> > The connector got dropped  when connector call it's bridege.detect
>>> > 
>>> > So I think the simplest solution is to have drm_bridge_funcs.detect
>>> > directly pass the connector
>>> 
>>> I don't disagree on principle, but I think a better first step would be
>>> to provide a detect_ctx hook to bridges.
>>
>>Yup. There's other reasons you really want to get at the locking context
>>in detect callbacks, doing this special case by passing something for
>>everyone doesn't sound like the right approach to me.
>
>Ok, I will add a detect_ctx  hook for bridge. Thanks for your advice.
>
>Just confirm that can I send this add detect_ctx hook patch alone first? 
>I think this patch will be easy to merge,  so it can help my WIP DP driver stay light on dependencies。

When I try to add the detect_ctx hook to bridge, I found that there is still a case that there is no ctx to
pass to detect_ctx:

int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
                                            uint32_t maxX, uint32_t maxY)
{
        ...............
        struct drm_modeset_acquire_ctx ctx;

        WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));

        drm_modeset_acquire_init(&ctx, 0);

        drm_dbg_kms(dev, "[CONNECTOR:%d:%s]\n", connector->base.id,
                    connector->name);

retry:
        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
        ...................
       ret = drm_helper_probe_detect(connector, &ctx, true);
       ......................................

     count = drm_helper_probe_get_modes(connector);

Then in drm_bridge_connector_get_modes:

static int drm_bridge_connector_get_modes(struct drm_connector *connector)
{
        struct drm_bridge_connector *bridge_connector =
                to_drm_bridge_connector(connector);
        struct drm_bridge *bridge;

   ...........................................
        /*
         * If display exposes EDID, then we parse that in the normal way to
         * build table of supported modes.
         */
        bridge = bridge_connector->bridge_edid;
        if (bridge)
                return drm_bridge_connector_get_modes_edid(connector, bridge);

static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
                                               struct drm_bridge *bridge)
{
        enum drm_connector_status status;
        const struct drm_edid *drm_edid;
        int n;

        status = drm_bridge_connector_detect(connector, false);

......................

-drm_bridge_connector_detect(struct drm_connector *connector, bool force)
+drm_bridge_connector_detect(struct drm_connector *connector,
+                           struct drm_modeset_acquire_ctx *ctx,
+                           bool force)
 {
        struct drm_bridge_connector *bridge_connector =
                to_drm_bridge_connector(connector);
@@ -186,7 +188,7 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
        enum drm_connector_status status;
 
        if (detect) {
               status = detect->funcs->detect(detect, ctx);

There is still no ctx in this call chain.

So there will be deadlock if I use drm_atomic_get_new_connector_for_encoder to find connector in
my bridge detect_ctx hook.



>
>
>>-Sima
>>-- 
>>Simona Vetter
>>Software Engineer, Intel Corporation
>>http://blog.ffwll.ch

Simona Vetter March 7, 2025, 1:25 p.m. UTC | #11

On Fri, Mar 07, 2025 at 03:30:41PM +0800, Andy Yan wrote:
> 
> Hi All,
> At 2025-03-07 09:08:48, "Andy Yan" <[e-mail]> wrote:
> >Hi All,
> >
> >At 2025-03-06 23:41:24, "Simona Vetter" <[e-mail]> wrote:
> >>On Thu, Mar 06, 2025 at 08:10:16AM +0100, Maxime Ripard wrote:
> >>> On Thu, Mar 06, 2025 at 09:16:24AM +0800, Andy Yan wrote:
> >>> > 
> >>> > Hi Maxime and Dmitry:
> >>> > 
> >>> > At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
> >>> > >On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
> >>> > >> Hi Andy,
> >>> > >> 
> >>> > >> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
> >>> > >> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> >>> > >> > >With the bridges switching over to drm_bridge_connector, the direct
> >>> > >> > >association between a bridge driver and its connector was lost.
> >>> > >> > >
> >>> > >> > >This is mitigated for atomic bridge drivers by the fact you can access
> >>> > >> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> >>> > >> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> >>> > >> > >
> >>> > >> > >This was also made easier by providing drm_atomic_state directly to all
> >>> > >> > >atomic hooks bridges can implement.
> >>> > >> > >
> >>> > >> > >However, bridge drivers don't have a way to access drm_atomic_state
> >>> > >> > >outside of the modeset path, like from the hotplug interrupt path or any
> >>> > >> > >interrupt handler.
> >>> > >> > >
> >>> > >> > >Let's introduce a function to retrieve the connector currently assigned
> >>> > >> > >to an encoder, without using drm_atomic_state, to make these drivers'
> >>> > >> > >life easier.
> >>> > >> > >
> >>> > >> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >>> > >> > >Co-developed-by: Simona Vetter <[e-mail]>
> >>> > >> > >Signed-off-by: Maxime Ripard <[e-mail]>
> >>> > >> > >---
> >>> > >> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >>> > >> > > include/drm/drm_atomic.h     |  3 +++
> >>> > >> > > 2 files changed, 48 insertions(+)
> >>> > >> > >
> >>> > >> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> > >> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> >>> > >> > >--- a/drivers/gpu/drm/drm_atomic.c
> >>> > >> > >+++ b/drivers/gpu/drm/drm_atomic.c
> >>> > >> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> >>> > >> > > 
> >>> > >> > > 	return NULL;
> >>> > >> > > }
> >>> > >> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> >>> > >> > > 
> >>> > >> > >+/**
> >>> > >> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> >>> > >> > >+ * @encoder: The encoder to find the connector of
> >>> > >> > >+ * @ctx: Modeset locking context
> >>> > >> > >+ *
> >>> > >> > >+ * This function finds and returns the connector currently assigned to
> >>> > >> > >+ * an @encoder.
> >>> > >> > >+ *
> >>> > >> > >+ * Returns:
> >>> > >> > >+ * The connector connected to @encoder, or an error pointer otherwise.
> >>> > >> > >+ * When the error is EDEADLK, a deadlock has been detected and the
> >>> > >> > >+ * sequence must be restarted.
> >>> > >> > >+ */
> >>> > >> > >+struct drm_connector *
> >>> > >> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> >>> > >> > >+				     struct drm_modeset_acquire_ctx *ctx)
> >>> > >> > >+{
> >>> > >> > >+	struct drm_connector_list_iter conn_iter;
> >>> > >> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> >>> > >> > >+	struct drm_connector *connector;
> >>> > >> > >+	struct drm_device *dev = encoder->dev;
> >>> > >> > >+	int ret;
> >>> > >> > >+
> >>> > >> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >>> > >> > >+	if (ret)
> >>> > >> > >+		return ERR_PTR(ret);
> >>> > >> > 
> >>> > >> > It seems that this will cause a deadlock when called from a hotplug
> >>> > >> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
> >>> > >> > use this API from a &drm_bridge_funcs.detect callback to get the
> >>> > >> > connector, as detect is called by drm_helper_probe_detect, which will
> >>> > >> > hold connection_mutex first, so the deaklock happens:
> >>> > >> >
> >>> > >> > drm_helper_probe_detect(struct drm_connector *connector,
> >>> > >> >                         struct drm_modeset_acquire_ctx *ctx,
> >>> > >> >                         bool force)
> >>> > >> > {
> >>> > >> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> >>> > >> >         struct drm_device *dev = connector->dev;
> >>> > >> >         int ret;
> >>> > >> > 
> >>> > >> >         if (!ctx)
> >>> > >> >                 return drm_helper_probe_detect_ctx(connector, force);
> >>> > >> > 
> >>> > >> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >>> > >> >         if (ret)
> >>> > >> >                 return ret;
> >>> > >> > 
> >>> > >> >         if (funcs->detect_ctx)
> >>> > >> >                 ret = funcs->detect_ctx(connector, ctx, force);
> >>> > >> >         else if (connector->funcs->detect)
> >>> > >> >                 ret = connector->funcs->detect(connector, force);
> >>> > >> >         else
> >>> > >> >                 ret = connector_status_connected;
> >>> > >> > 
> >>> > >> >         if (ret != connector->status)
> >>> > >> >                 connector->epoch_counter += 1;
> >>> > >> > 
> >>> > >> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
> >>> > >> > this case ?
> >>> > >> 
> >>> > >> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
> >>> > >> fine with reentrancy from the same context, so it should work just fine.
> >>> > >
> >>> > >Andy, that probably means that you should use .detect_ctx() and pass the
> >>> > >context to drm_atomic_get_connector_for_encoder().
> >>> > 
> >>> > Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
> >>> > The call chain is:
> >>> >  drm_helper_probe_detect 
> >>> >  --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> >>> > --> drm_bridge_funcs.detect(bridge)
> >>> > The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
> >>> > The connector got dropped  when connector call it's bridege.detect
> >>> > 
> >>> > So I think the simplest solution is to have drm_bridge_funcs.detect
> >>> > directly pass the connector
> >>> 
> >>> I don't disagree on principle, but I think a better first step would be
> >>> to provide a detect_ctx hook to bridges.
> >>
> >>Yup. There's other reasons you really want to get at the locking context
> >>in detect callbacks, doing this special case by passing something for
> >>everyone doesn't sound like the right approach to me.
> >
> >Ok, I will add a detect_ctx  hook for bridge. Thanks for your advice.
> >
> >Just confirm that can I send this add detect_ctx hook patch alone first? 
> >I think this patch will be easy to merge,  so it can help my WIP DP driver stay light on dependencies。

Yeah I think sending this prep work as a standalone thing is ok,
especially since it looks like there's going to be more work involved.

> When I try to add the detect_ctx hook to bridge, I found that there is still a case that there is no ctx to
> pass to detect_ctx:
> 
> int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                                             uint32_t maxX, uint32_t maxY)
> {
>         ...............
>         struct drm_modeset_acquire_ctx ctx;
> 
>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> 
>         drm_modeset_acquire_init(&ctx, 0);
> 
>         drm_dbg_kms(dev, "[CONNECTOR:%d:%s]\n", connector->base.id,
>                     connector->name);
> 
> retry:
>         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
>         ...................
>        ret = drm_helper_probe_detect(connector, &ctx, true);
>        ......................................
> 
>      count = drm_helper_probe_get_modes(connector);
> 
> Then in drm_bridge_connector_get_modes:
> 
> static int drm_bridge_connector_get_modes(struct drm_connector *connector)
> {
>         struct drm_bridge_connector *bridge_connector =
>                 to_drm_bridge_connector(connector);
>         struct drm_bridge *bridge;
> 
>    ...........................................
>         /*
>          * If display exposes EDID, then we parse that in the normal way to
>          * build table of supported modes.
>          */
>         bridge = bridge_connector->bridge_edid;
>         if (bridge)
>                 return drm_bridge_connector_get_modes_edid(connector, bridge);
> 
> static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
>                                                struct drm_bridge *bridge)
> {
>         enum drm_connector_status status;
>         const struct drm_edid *drm_edid;
>         int n;
> 
>         status = drm_bridge_connector_detect(connector, false);
> 
> ......................
> 
> -drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> +drm_bridge_connector_detect(struct drm_connector *connector,
> +                           struct drm_modeset_acquire_ctx *ctx,
> +                           bool force)
>  {
>         struct drm_bridge_connector *bridge_connector =
>                 to_drm_bridge_connector(connector);
> @@ -186,7 +188,7 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>         enum drm_connector_status status;
>  
>         if (detect) {
>                status = detect->funcs->detect(detect, ctx);
> 
> There is still no ctx in this call chain.
> 
> So there will be deadlock if I use drm_atomic_get_new_connector_for_encoder to find connector in
> my bridge detect_ctx hook.

Hm yeah looks like bridge callchain isn't split up into ->detec and
->get_modes in exactly the same way as connectors. I think the clean
solution would be to pull drm_bridge_connector_detect() out from
drm_bridge_connector_get_modes(), but that might unravel a huge amount of
work. But wiring the ctx through all the get_mode functions might also not
work out great, hence why I'd try moving _detect() first.

Once you have bridge_connector_detect at the same split-up like any other
connectors, it should be possible to add the contect to
drm_bridge_connector_detect and have it everywhere. In general I think
calling drm_bridge_connector_detect with a NULL ctx should be a bug, and
we should catch that with a WARN_ON and bail out. Otherwise the locking is
going to be a complete mess.

And yeah definitely do this prep work as a separate series, I think the
need is clearly established, so we don't need a user right away to justify
this rework.

Cheers, Sima
> 
> 
> 
> >
> >
> >>-Sima
> >>-- 
> >>Simona Vetter
> >>Software Engineer, Intel Corporation
> >>http://blog.ffwll.ch

Andy Yan March 11, 2025, 6:42 a.m. UTC | #12

Hi Simona and Laurent,

At 2025-03-07 21:25:02, "Simona Vetter" <[e-mail]> wrote:
>On Fri, Mar 07, 2025 at 03:30:41PM +0800, Andy Yan wrote:
>> 
>> Hi All,
>> At 2025-03-07 09:08:48, "Andy Yan" <[e-mail]> wrote:
>> >Hi All,
>> >
>> >At 2025-03-06 23:41:24, "Simona Vetter" <[e-mail]> wrote:
>> >>On Thu, Mar 06, 2025 at 08:10:16AM +0100, Maxime Ripard wrote:
>> >>> On Thu, Mar 06, 2025 at 09:16:24AM +0800, Andy Yan wrote:
>> >>> > 
>> >>> > Hi Maxime and Dmitry:
>> >>> > 
>> >>> > At 2025-03-06 04:13:53, "Dmitry Baryshkov" <[e-mail]> wrote:
>> >>> > >On Wed, Mar 05, 2025 at 02:19:36PM +0100, Maxime Ripard wrote:
>> >>> > >> Hi Andy,
>> >>> > >> 
>> >>> > >> On Wed, Mar 05, 2025 at 07:55:19PM +0800, Andy Yan wrote:
>> >>> > >> > At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>> >>> > >> > >With the bridges switching over to drm_bridge_connector, the direct
>> >>> > >> > >association between a bridge driver and its connector was lost.
>> >>> > >> > >
>> >>> > >> > >This is mitigated for atomic bridge drivers by the fact you can access
>> >>> > >> > >the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>> >>> > >> > >drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>> >>> > >> > >
>> >>> > >> > >This was also made easier by providing drm_atomic_state directly to all
>> >>> > >> > >atomic hooks bridges can implement.
>> >>> > >> > >
>> >>> > >> > >However, bridge drivers don't have a way to access drm_atomic_state
>> >>> > >> > >outside of the modeset path, like from the hotplug interrupt path or any
>> >>> > >> > >interrupt handler.
>> >>> > >> > >
>> >>> > >> > >Let's introduce a function to retrieve the connector currently assigned
>> >>> > >> > >to an encoder, without using drm_atomic_state, to make these drivers'
>> >>> > >> > >life easier.
>> >>> > >> > >
>> >>> > >> > >Reviewed-by: Dmitry Baryshkov <[e-mail]>
>> >>> > >> > >Co-developed-by: Simona Vetter <[e-mail]>
>> >>> > >> > >Signed-off-by: Maxime Ripard <[e-mail]>
>> >>> > >> > >---
>> >>> > >> > > drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> >>> > >> > > include/drm/drm_atomic.h     |  3 +++
>> >>> > >> > > 2 files changed, 48 insertions(+)
>> >>> > >> > >
>> >>> > >> > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >>> > >> > >index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>> >>> > >> > >--- a/drivers/gpu/drm/drm_atomic.c
>> >>> > >> > >+++ b/drivers/gpu/drm/drm_atomic.c
>> >>> > >> > >@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>> >>> > >> > > 
>> >>> > >> > > 	return NULL;
>> >>> > >> > > }
>> >>> > >> > > EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>> >>> > >> > > 
>> >>> > >> > >+/**
>> >>> > >> > >+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>> >>> > >> > >+ * @encoder: The encoder to find the connector of
>> >>> > >> > >+ * @ctx: Modeset locking context
>> >>> > >> > >+ *
>> >>> > >> > >+ * This function finds and returns the connector currently assigned to
>> >>> > >> > >+ * an @encoder.
>> >>> > >> > >+ *
>> >>> > >> > >+ * Returns:
>> >>> > >> > >+ * The connector connected to @encoder, or an error pointer otherwise.
>> >>> > >> > >+ * When the error is EDEADLK, a deadlock has been detected and the
>> >>> > >> > >+ * sequence must be restarted.
>> >>> > >> > >+ */
>> >>> > >> > >+struct drm_connector *
>> >>> > >> > >+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>> >>> > >> > >+				     struct drm_modeset_acquire_ctx *ctx)
>> >>> > >> > >+{
>> >>> > >> > >+	struct drm_connector_list_iter conn_iter;
>> >>> > >> > >+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>> >>> > >> > >+	struct drm_connector *connector;
>> >>> > >> > >+	struct drm_device *dev = encoder->dev;
>> >>> > >> > >+	int ret;
>> >>> > >> > >+
>> >>> > >> > >+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> >>> > >> > >+	if (ret)
>> >>> > >> > >+		return ERR_PTR(ret);
>> >>> > >> > 
>> >>> > >> > It seems that this will cause a deadlock when called from a hotplug
>> >>> > >> > handling path, I have a WIP DP diver[0], which suggested by Dmitry to
>> >>> > >> > use this API from a &drm_bridge_funcs.detect callback to get the
>> >>> > >> > connector, as detect is called by drm_helper_probe_detect, which will
>> >>> > >> > hold connection_mutex first, so the deaklock happens:
>> >>> > >> >
>> >>> > >> > drm_helper_probe_detect(struct drm_connector *connector,
>> >>> > >> >                         struct drm_modeset_acquire_ctx *ctx,
>> >>> > >> >                         bool force)
>> >>> > >> > {
>> >>> > >> >         const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>> >>> > >> >         struct drm_device *dev = connector->dev;
>> >>> > >> >         int ret;
>> >>> > >> > 
>> >>> > >> >         if (!ctx)
>> >>> > >> >                 return drm_helper_probe_detect_ctx(connector, force);
>> >>> > >> > 
>> >>> > >> >         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> >>> > >> >         if (ret)
>> >>> > >> >                 return ret;
>> >>> > >> > 
>> >>> > >> >         if (funcs->detect_ctx)
>> >>> > >> >                 ret = funcs->detect_ctx(connector, ctx, force);
>> >>> > >> >         else if (connector->funcs->detect)
>> >>> > >> >                 ret = connector->funcs->detect(connector, force);
>> >>> > >> >         else
>> >>> > >> >                 ret = connector_status_connected;
>> >>> > >> > 
>> >>> > >> >         if (ret != connector->status)
>> >>> > >> >                 connector->epoch_counter += 1;
>> >>> > >> > 
>> >>> > >> > So I wonder can we let drm_bridge_funcs.detect pass a connector for
>> >>> > >> > this case ?
>> >>> > >> 
>> >>> > >> Do you actually see a deadlock occurring? AFAIK, drm_modeset_lock is
>> >>> > >> fine with reentrancy from the same context, so it should work just fine.
>> >>> > >
>> >>> > >Andy, that probably means that you should use .detect_ctx() and pass the
>> >>> > >context to drm_atomic_get_connector_for_encoder().
>> >>> > 
>> >>> > Unfortunately, the drm_bridge_funcs does not have a .detect_ctx()  version .
>> >>> > The call chain is:
>> >>> >  drm_helper_probe_detect 
>> >>> >  --> drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>> >>> > --> drm_bridge_funcs.detect(bridge)
>> >>> > The ctx got dropped when drm_helper_probe_detect call  drm_bridge_connector_detect
>> >>> > The connector got dropped  when connector call it's bridege.detect
>> >>> > 
>> >>> > So I think the simplest solution is to have drm_bridge_funcs.detect
>> >>> > directly pass the connector
>> >>> 
>> >>> I don't disagree on principle, but I think a better first step would be
>> >>> to provide a detect_ctx hook to bridges.
>> >>
>> >>Yup. There's other reasons you really want to get at the locking context
>> >>in detect callbacks, doing this special case by passing something for
>> >>everyone doesn't sound like the right approach to me.
>> >
>> >Ok, I will add a detect_ctx  hook for bridge. Thanks for your advice.
>> >
>> >Just confirm that can I send this add detect_ctx hook patch alone first? 
>> >I think this patch will be easy to merge,  so it can help my WIP DP driver stay light on dependencies。
>
>Yeah I think sending this prep work as a standalone thing is ok,
>especially since it looks like there's going to be more work involved.
>
>> When I try to add the detect_ctx hook to bridge, I found that there is still a case that there is no ctx to
>> pass to detect_ctx:
>> 
>> int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>                                             uint32_t maxX, uint32_t maxY)
>> {
>>         ...............
>>         struct drm_modeset_acquire_ctx ctx;
>> 
>>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>> 
>>         drm_modeset_acquire_init(&ctx, 0);
>> 
>>         drm_dbg_kms(dev, "[CONNECTOR:%d:%s]\n", connector->base.id,
>>                     connector->name);
>> 
>> retry:
>>         ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
>>         ...................
>>        ret = drm_helper_probe_detect(connector, &ctx, true);
>>        ......................................
>> 
>>      count = drm_helper_probe_get_modes(connector);
>> 
>> Then in drm_bridge_connector_get_modes:
>> 
>> static int drm_bridge_connector_get_modes(struct drm_connector *connector)
>> {
>>         struct drm_bridge_connector *bridge_connector =
>>                 to_drm_bridge_connector(connector);
>>         struct drm_bridge *bridge;
>> 
>>    ...........................................
>>         /*
>>          * If display exposes EDID, then we parse that in the normal way to
>>          * build table of supported modes.
>>          */
>>         bridge = bridge_connector->bridge_edid;
>>         if (bridge)
>>                 return drm_bridge_connector_get_modes_edid(connector, bridge);
>> 
>> static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
>>                                                struct drm_bridge *bridge)
>> {
>>         enum drm_connector_status status;
>>         const struct drm_edid *drm_edid;
>>         int n;
>> 
>>         status = drm_bridge_connector_detect(connector, false);
>> 
>> ......................
>> 
>> -drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>> +drm_bridge_connector_detect(struct drm_connector *connector,
>> +                           struct drm_modeset_acquire_ctx *ctx,
>> +                           bool force)
>>  {
>>         struct drm_bridge_connector *bridge_connector =
>>                 to_drm_bridge_connector(connector);
>> @@ -186,7 +188,7 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>>         enum drm_connector_status status;
>>  
>>         if (detect) {
>>                status = detect->funcs->detect(detect, ctx);
>> 
>> There is still no ctx in this call chain.
>> 
>> So there will be deadlock if I use drm_atomic_get_new_connector_for_encoder to find connector in
>> my bridge detect_ctx hook.
>
>Hm yeah looks like bridge callchain isn't split up into ->detec and
>->get_modes in exactly the same way as connectors. I think the clean
>solution would be to pull drm_bridge_connector_detect() out from
>drm_bridge_connector_get_modes(), but that might unravel a huge amount of
>work. But wiring the ctx through all the get_mode functions might also not
>work out great, hence why I'd try moving _detect() first.

Since drm_connector_helper_funcs.get_modes is called by drm_helper_probe_single_connector_modes,
which already called drm_helper_probe_detect to make sure the connector is at connected state, so
Can we directly remove drm_bridge_connector_detect within drm_bridge_connector_get_modes_edid?

>
>Once you have bridge_connector_detect at the same split-up like any other
>connectors, it should be possible to add the contect to
>drm_bridge_connector_detect and have it everywhere. In general I think
>calling drm_bridge_connector_detect with a NULL ctx should be a bug, and
>we should catch that with a WARN_ON and bail out. Otherwise the locking is
>going to be a complete mess.
>
>And yeah definitely do this prep work as a separate series, I think the
>need is clearly established, so we don't need a user right away to justify
>this rework.
>
>Cheers, Sima
>> 
>> 
>> 
>> >
>> >
>> >>-Sima
>> >>-- 
>> >>Simona Vetter
>> >>Software Engineer, Intel Corporation
>> >>http://blog.ffwll.ch
>
>-- 
>Simona Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

Andy Yan March 13, 2025, 8:09 a.m. UTC | #13

Hi Maxime,

At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
>
>
>Hi Maxime,
>
>At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>>With the bridges switching over to drm_bridge_connector, the direct
>>association between a bridge driver and its connector was lost.
>>
>>This is mitigated for atomic bridge drivers by the fact you can access
>>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>>
>>This was also made easier by providing drm_atomic_state directly to all
>>atomic hooks bridges can implement.
>>
>>However, bridge drivers don't have a way to access drm_atomic_state
>>outside of the modeset path, like from the hotplug interrupt path or any
>>interrupt handler.
>>
>>Let's introduce a function to retrieve the connector currently assigned
>>to an encoder, without using drm_atomic_state, to make these drivers'
>>life easier.
>>
>>Reviewed-by: Dmitry Baryshkov <[e-mail]>
>>Co-developed-by: Simona Vetter <[e-mail]>
>>Signed-off-by: Maxime Ripard <[e-mail]>
>>---
>> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> include/drm/drm_atomic.h     |  3 +++
>> 2 files changed, 48 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>>--- a/drivers/gpu/drm/drm_atomic.c
>>+++ b/drivers/gpu/drm/drm_atomic.c
>>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>> 
>> 	return NULL;
>> }
>> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>> 
>>+/**
>>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>>+ * @encoder: The encoder to find the connector of
>>+ * @ctx: Modeset locking context
>>+ *
>>+ * This function finds and returns the connector currently assigned to
>>+ * an @encoder.
>>+ *
>>+ * Returns:
>>+ * The connector connected to @encoder, or an error pointer otherwise.
>>+ * When the error is EDEADLK, a deadlock has been detected and the
>>+ * sequence must be restarted.
>>+ */
>>+struct drm_connector *
>>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>>+				     struct drm_modeset_acquire_ctx *ctx)
>>+{
>>+	struct drm_connector_list_iter conn_iter;
>>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>>+	struct drm_connector *connector;
>>+	struct drm_device *dev = encoder->dev;
>>+	int ret;
>>+
>>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>>+	if (ret)
>>+		return ERR_PTR(ret);
>
>It seems that this will cause a deadlock when called from a  hotplug handling path,
>I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
>&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
>which will hold connection_mutex first, so the deaklock happens:
>
>
>drm_helper_probe_detect(struct drm_connector *connector,
>                        struct drm_modeset_acquire_ctx *ctx,
>                        bool force)
>{
>        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>        struct drm_device *dev = connector->dev;
>        int ret;
>
>        if (!ctx)
>                return drm_helper_probe_detect_ctx(connector, force);
>
>        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>        if (ret)
>                return ret;
>
>        if (funcs->detect_ctx)
>                ret = funcs->detect_ctx(connector, ctx, force);
>        else if (connector->funcs->detect)
>                ret = connector->funcs->detect(connector, force);
>        else
>                ret = connector_status_connected;
>
>        if (ret != connector->status)
>                connector->epoch_counter += 1;
>
>So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
>
>
>
>[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
>>+
>>+	drm_connector_list_iter_begin(dev, &conn_iter);
>>+	drm_for_each_connector_iter(connector, &conn_iter) {
>>+		if (!connector->state)
>>+			continue;
>>+
>>+		if (encoder == connector->state->best_encoder) {
>>+			out_connector = connector;


When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
 maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:

[   52.713030] Invalid return value -22 for connector detection
[   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
0x63c
[   52.714568] Modules linked in:

[   52.724546] Call trace:
[   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
[   52.725319]  drm_mode_getconnector+0x2a4/0x488
[   52.725711]  drm_ioctl_kernel+0xb4/0x11c
[   52.726057]  drm_ioctl+0x22c/0x544
[   52.726358]  __arm64_sys_ioctl+0xac/0xe0
[   52.726706]  invoke_syscall+0x44/0x100
[   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4

This is because  best_encoder is set by set_best_encoder, which is called from
drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
for the first time, the functions mentioned above have not been called yet,
then we can't match the encoder from connector->state->best_encoder for this case.



>>+			break;
>>+		}
>>+	}
>>+	drm_connector_list_iter_end(&conn_iter);
>>+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>+
>>+	return out_connector;
>>+}
>>+EXPORT_SYMBOL(drm_atomic_get_connector_for_encoder);
>>+
>>+
>> /**

Maxime Ripard March 13, 2025, 11:55 a.m. UTC | #14

Hi,

On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> >>With the bridges switching over to drm_bridge_connector, the direct
> >>association between a bridge driver and its connector was lost.
> >>
> >>This is mitigated for atomic bridge drivers by the fact you can access
> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> >>
> >>This was also made easier by providing drm_atomic_state directly to all
> >>atomic hooks bridges can implement.
> >>
> >>However, bridge drivers don't have a way to access drm_atomic_state
> >>outside of the modeset path, like from the hotplug interrupt path or any
> >>interrupt handler.
> >>
> >>Let's introduce a function to retrieve the connector currently assigned
> >>to an encoder, without using drm_atomic_state, to make these drivers'
> >>life easier.
> >>
> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >>Co-developed-by: Simona Vetter <[e-mail]>
> >>Signed-off-by: Maxime Ripard <[e-mail]>
> >>---
> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >> include/drm/drm_atomic.h     |  3 +++
> >> 2 files changed, 48 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> >>--- a/drivers/gpu/drm/drm_atomic.c
> >>+++ b/drivers/gpu/drm/drm_atomic.c
> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> >> 
> >> 	return NULL;
> >> }
> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> >> 
> >>+/**
> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> >>+ * @encoder: The encoder to find the connector of
> >>+ * @ctx: Modeset locking context
> >>+ *
> >>+ * This function finds and returns the connector currently assigned to
> >>+ * an @encoder.
> >>+ *
> >>+ * Returns:
> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> >>+ * sequence must be restarted.
> >>+ */
> >>+struct drm_connector *
> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> >>+				     struct drm_modeset_acquire_ctx *ctx)
> >>+{
> >>+	struct drm_connector_list_iter conn_iter;
> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> >>+	struct drm_connector *connector;
> >>+	struct drm_device *dev = encoder->dev;
> >>+	int ret;
> >>+
> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >>+	if (ret)
> >>+		return ERR_PTR(ret);
> >
> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> >which will hold connection_mutex first, so the deaklock happens:
> >
> >
> >drm_helper_probe_detect(struct drm_connector *connector,
> >                        struct drm_modeset_acquire_ctx *ctx,
> >                        bool force)
> >{
> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> >        struct drm_device *dev = connector->dev;
> >        int ret;
> >
> >        if (!ctx)
> >                return drm_helper_probe_detect_ctx(connector, force);
> >
> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >        if (ret)
> >                return ret;
> >
> >        if (funcs->detect_ctx)
> >                ret = funcs->detect_ctx(connector, ctx, force);
> >        else if (connector->funcs->detect)
> >                ret = connector->funcs->detect(connector, force);
> >        else
> >                ret = connector_status_connected;
> >
> >        if (ret != connector->status)
> >                connector->epoch_counter += 1;
> >
> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> >
> >
> >
> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> >>+
> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> >>+		if (!connector->state)
> >>+			continue;
> >>+
> >>+		if (encoder == connector->state->best_encoder) {
> >>+			out_connector = connector;
> 
> 
> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> 
> [   52.713030] Invalid return value -22 for connector detection
> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> 0x63c
> [   52.714568] Modules linked in:
> 
> [   52.724546] Call trace:
> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> [   52.726057]  drm_ioctl+0x22c/0x544
> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> [   52.726706]  invoke_syscall+0x44/0x100
> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> 
> This is because  best_encoder is set by set_best_encoder, which is called from
> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> for the first time, the functions mentioned above have not been called yet,
> then we can't match the encoder from connector->state->best_encoder for this case.

As far as I'm concerned, it's by design. Encoders and connectors have
1:N relationship, and only once a connector has been enabled it has an
encoder.

If the connector is disabled, there's no associated encoder.

Maxime

Andy Yan March 14, 2025, 12:50 a.m. UTC | #15

Hi Maxime and Simona,


At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
>Hi,
>
>On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
>> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
>> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>> >>With the bridges switching over to drm_bridge_connector, the direct
>> >>association between a bridge driver and its connector was lost.
>> >>
>> >>This is mitigated for atomic bridge drivers by the fact you can access
>> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>> >>
>> >>This was also made easier by providing drm_atomic_state directly to all
>> >>atomic hooks bridges can implement.
>> >>
>> >>However, bridge drivers don't have a way to access drm_atomic_state
>> >>outside of the modeset path, like from the hotplug interrupt path or any
>> >>interrupt handler.
>> >>
>> >>Let's introduce a function to retrieve the connector currently assigned
>> >>to an encoder, without using drm_atomic_state, to make these drivers'
>> >>life easier.
>> >>
>> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
>> >>Co-developed-by: Simona Vetter <[e-mail]>
>> >>Signed-off-by: Maxime Ripard <[e-mail]>
>> >>---
>> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> >> include/drm/drm_atomic.h     |  3 +++
>> >> 2 files changed, 48 insertions(+)
>> >>
>> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>> >>--- a/drivers/gpu/drm/drm_atomic.c
>> >>+++ b/drivers/gpu/drm/drm_atomic.c
>> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>> >> 
>> >> 	return NULL;
>> >> }
>> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>> >> 
>> >>+/**
>> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>> >>+ * @encoder: The encoder to find the connector of
>> >>+ * @ctx: Modeset locking context
>> >>+ *
>> >>+ * This function finds and returns the connector currently assigned to
>> >>+ * an @encoder.
>> >>+ *
>> >>+ * Returns:
>> >>+ * The connector connected to @encoder, or an error pointer otherwise.
>> >>+ * When the error is EDEADLK, a deadlock has been detected and the
>> >>+ * sequence must be restarted.
>> >>+ */
>> >>+struct drm_connector *
>> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>> >>+				     struct drm_modeset_acquire_ctx *ctx)
>> >>+{
>> >>+	struct drm_connector_list_iter conn_iter;
>> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>> >>+	struct drm_connector *connector;
>> >>+	struct drm_device *dev = encoder->dev;
>> >>+	int ret;
>> >>+
>> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> >>+	if (ret)
>> >>+		return ERR_PTR(ret);
>> >
>> >It seems that this will cause a deadlock when called from a  hotplug handling path,
>> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
>> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
>> >which will hold connection_mutex first, so the deaklock happens:
>> >
>> >
>> >drm_helper_probe_detect(struct drm_connector *connector,
>> >                        struct drm_modeset_acquire_ctx *ctx,
>> >                        bool force)
>> >{
>> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>> >        struct drm_device *dev = connector->dev;
>> >        int ret;
>> >
>> >        if (!ctx)
>> >                return drm_helper_probe_detect_ctx(connector, force);
>> >
>> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> >        if (ret)
>> >                return ret;
>> >
>> >        if (funcs->detect_ctx)
>> >                ret = funcs->detect_ctx(connector, ctx, force);
>> >        else if (connector->funcs->detect)
>> >                ret = connector->funcs->detect(connector, force);
>> >        else
>> >                ret = connector_status_connected;
>> >
>> >        if (ret != connector->status)
>> >                connector->epoch_counter += 1;
>> >
>> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
>> >
>> >
>> >
>> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
>> >>+
>> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
>> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
>> >>+		if (!connector->state)
>> >>+			continue;
>> >>+
>> >>+		if (encoder == connector->state->best_encoder) {
>> >>+			out_connector = connector;
>> 
>> 
>> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
>>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
>> 
>> [   52.713030] Invalid return value -22 for connector detection
>> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
>> 0x63c
>> [   52.714568] Modules linked in:
>> 
>> [   52.724546] Call trace:
>> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
>> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
>> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
>> [   52.726057]  drm_ioctl+0x22c/0x544
>> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
>> [   52.726706]  invoke_syscall+0x44/0x100
>> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
>> 
>> This is because  best_encoder is set by set_best_encoder, which is called from
>> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
>> for the first time, the functions mentioned above have not been called yet,
>> then we can't match the encoder from connector->state->best_encoder for this case.
>
>As far as I'm concerned, it's by design. Encoders and connectors have
>1:N relationship, and only once a connector has been enabled it has an
>encoder.
>
>If the connector is disabled, there's no associated encoder.

Does this prove that this API is not suitable for my application scenario: 
Get the connector in the bridge's .detect callback, so this means that I may
still need to modify the bridge's connector callback so that it can pass the connector ?

>
>Maxime

Dmitry Baryshkov March 14, 2025, 5:52 a.m. UTC | #16

On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> 
> Hi Maxime and Simona,
> 
> 
> At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> >Hi,
> >
> >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> >> >>With the bridges switching over to drm_bridge_connector, the direct
> >> >>association between a bridge driver and its connector was lost.
> >> >>
> >> >>This is mitigated for atomic bridge drivers by the fact you can access
> >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> >> >>
> >> >>This was also made easier by providing drm_atomic_state directly to all
> >> >>atomic hooks bridges can implement.
> >> >>
> >> >>However, bridge drivers don't have a way to access drm_atomic_state
> >> >>outside of the modeset path, like from the hotplug interrupt path or any
> >> >>interrupt handler.
> >> >>
> >> >>Let's introduce a function to retrieve the connector currently assigned
> >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> >> >>life easier.
> >> >>
> >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >> >>Co-developed-by: Simona Vetter <[e-mail]>
> >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> >> >>---
> >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >> >> include/drm/drm_atomic.h     |  3 +++
> >> >> 2 files changed, 48 insertions(+)
> >> >>
> >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> >> >>--- a/drivers/gpu/drm/drm_atomic.c
> >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> >> >> 
> >> >> 	return NULL;
> >> >> }
> >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> >> >> 
> >> >>+/**
> >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> >> >>+ * @encoder: The encoder to find the connector of
> >> >>+ * @ctx: Modeset locking context
> >> >>+ *
> >> >>+ * This function finds and returns the connector currently assigned to
> >> >>+ * an @encoder.
> >> >>+ *
> >> >>+ * Returns:
> >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> >> >>+ * sequence must be restarted.
> >> >>+ */
> >> >>+struct drm_connector *
> >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> >> >>+{
> >> >>+	struct drm_connector_list_iter conn_iter;
> >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> >> >>+	struct drm_connector *connector;
> >> >>+	struct drm_device *dev = encoder->dev;
> >> >>+	int ret;
> >> >>+
> >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >> >>+	if (ret)
> >> >>+		return ERR_PTR(ret);
> >> >
> >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> >> >which will hold connection_mutex first, so the deaklock happens:
> >> >
> >> >
> >> >drm_helper_probe_detect(struct drm_connector *connector,
> >> >                        struct drm_modeset_acquire_ctx *ctx,
> >> >                        bool force)
> >> >{
> >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> >> >        struct drm_device *dev = connector->dev;
> >> >        int ret;
> >> >
> >> >        if (!ctx)
> >> >                return drm_helper_probe_detect_ctx(connector, force);
> >> >
> >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >> >        if (ret)
> >> >                return ret;
> >> >
> >> >        if (funcs->detect_ctx)
> >> >                ret = funcs->detect_ctx(connector, ctx, force);
> >> >        else if (connector->funcs->detect)
> >> >                ret = connector->funcs->detect(connector, force);
> >> >        else
> >> >                ret = connector_status_connected;
> >> >
> >> >        if (ret != connector->status)
> >> >                connector->epoch_counter += 1;
> >> >
> >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> >> >
> >> >
> >> >
> >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> >> >>+
> >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> >> >>+		if (!connector->state)
> >> >>+			continue;
> >> >>+
> >> >>+		if (encoder == connector->state->best_encoder) {
> >> >>+			out_connector = connector;
> >> 
> >> 
> >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> >> 
> >> [   52.713030] Invalid return value -22 for connector detection
> >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> >> 0x63c
> >> [   52.714568] Modules linked in:
> >> 
> >> [   52.724546] Call trace:
> >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> >> [   52.726057]  drm_ioctl+0x22c/0x544
> >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> >> [   52.726706]  invoke_syscall+0x44/0x100
> >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> >> 
> >> This is because  best_encoder is set by set_best_encoder, which is called from
> >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> >> for the first time, the functions mentioned above have not been called yet,
> >> then we can't match the encoder from connector->state->best_encoder for this case.
> >
> >As far as I'm concerned, it's by design. Encoders and connectors have
> >1:N relationship, and only once a connector has been enabled it has an
> >encoder.
> >
> >If the connector is disabled, there's no associated encoder.
> 
> Does this prove that this API is not suitable for my application scenario: 
> Get the connector in the bridge's .detect callback, so this means that I may
> still need to modify the bridge's connector callback so that it can pass the connector ?

I'd say, yes, please.

Maxime Ripard March 14, 2025, 7:45 a.m. UTC | #17

On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > 
> > Hi Maxime and Simona,
> > 
> > 
> > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > >Hi,
> > >
> > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > >> >>association between a bridge driver and its connector was lost.
> > >> >>
> > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > >> >>
> > >> >>This was also made easier by providing drm_atomic_state directly to all
> > >> >>atomic hooks bridges can implement.
> > >> >>
> > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > >> >>interrupt handler.
> > >> >>
> > >> >>Let's introduce a function to retrieve the connector currently assigned
> > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > >> >>life easier.
> > >> >>
> > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > >> >>---
> > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > >> >> include/drm/drm_atomic.h     |  3 +++
> > >> >> 2 files changed, 48 insertions(+)
> > >> >>
> > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > >> >> 
> > >> >> 	return NULL;
> > >> >> }
> > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > >> >> 
> > >> >>+/**
> > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > >> >>+ * @encoder: The encoder to find the connector of
> > >> >>+ * @ctx: Modeset locking context
> > >> >>+ *
> > >> >>+ * This function finds and returns the connector currently assigned to
> > >> >>+ * an @encoder.
> > >> >>+ *
> > >> >>+ * Returns:
> > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > >> >>+ * sequence must be restarted.
> > >> >>+ */
> > >> >>+struct drm_connector *
> > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > >> >>+{
> > >> >>+	struct drm_connector_list_iter conn_iter;
> > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > >> >>+	struct drm_connector *connector;
> > >> >>+	struct drm_device *dev = encoder->dev;
> > >> >>+	int ret;
> > >> >>+
> > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > >> >>+	if (ret)
> > >> >>+		return ERR_PTR(ret);
> > >> >
> > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > >> >which will hold connection_mutex first, so the deaklock happens:
> > >> >
> > >> >
> > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > >> >                        bool force)
> > >> >{
> > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > >> >        struct drm_device *dev = connector->dev;
> > >> >        int ret;
> > >> >
> > >> >        if (!ctx)
> > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > >> >
> > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > >> >        if (ret)
> > >> >                return ret;
> > >> >
> > >> >        if (funcs->detect_ctx)
> > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > >> >        else if (connector->funcs->detect)
> > >> >                ret = connector->funcs->detect(connector, force);
> > >> >        else
> > >> >                ret = connector_status_connected;
> > >> >
> > >> >        if (ret != connector->status)
> > >> >                connector->epoch_counter += 1;
> > >> >
> > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > >> >
> > >> >
> > >> >
> > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > >> >>+
> > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > >> >>+		if (!connector->state)
> > >> >>+			continue;
> > >> >>+
> > >> >>+		if (encoder == connector->state->best_encoder) {
> > >> >>+			out_connector = connector;
> > >> 
> > >> 
> > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > >> 
> > >> [   52.713030] Invalid return value -22 for connector detection
> > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > >> 0x63c
> > >> [   52.714568] Modules linked in:
> > >> 
> > >> [   52.724546] Call trace:
> > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > >> [   52.726706]  invoke_syscall+0x44/0x100
> > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > >> 
> > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > >> for the first time, the functions mentioned above have not been called yet,
> > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > >
> > >As far as I'm concerned, it's by design. Encoders and connectors have
> > >1:N relationship, and only once a connector has been enabled it has an
> > >encoder.
> > >
> > >If the connector is disabled, there's no associated encoder.
> > 
> > Does this prove that this API is not suitable for my application scenario: 
> > Get the connector in the bridge's .detect callback, so this means that I may
> > still need to modify the bridge's connector callback so that it can pass the connector ?
> 
> I'd say, yes, please.

And I'd say no :)

There's no reason to deviate from the API other entities have here. It's
just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
completely thought through and it's one of the part where it shows.

We have two alternative solutions: Either the driver creates the
connector itself, since it doesn't seem to use any downstream bridge
anyway, or we need a new bridge helper to find the connector on a bridge
chain.

We have the iterator already, we just need a new accessor to retrieve
the (optional) connector of a bridge, and if there's none, go to the
next bridge and try again.

There's a decent amount of variations that ideally call for tests too,
but it should be pretty simple overall.

Maxime

Dmitry Baryshkov March 14, 2025, 7:59 a.m. UTC | #18

On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > 
> > > Hi Maxime and Simona,
> > > 
> > > 
> > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > >Hi,
> > > >
> > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > >> >>association between a bridge driver and its connector was lost.
> > > >> >>
> > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > >> >>
> > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > >> >>atomic hooks bridges can implement.
> > > >> >>
> > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > >> >>interrupt handler.
> > > >> >>
> > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > >> >>life easier.
> > > >> >>
> > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > >> >>---
> > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > >> >> 2 files changed, 48 insertions(+)
> > > >> >>
> > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > >> >> 
> > > >> >> 	return NULL;
> > > >> >> }
> > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > >> >> 
> > > >> >>+/**
> > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > >> >>+ * @encoder: The encoder to find the connector of
> > > >> >>+ * @ctx: Modeset locking context
> > > >> >>+ *
> > > >> >>+ * This function finds and returns the connector currently assigned to
> > > >> >>+ * an @encoder.
> > > >> >>+ *
> > > >> >>+ * Returns:
> > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > >> >>+ * sequence must be restarted.
> > > >> >>+ */
> > > >> >>+struct drm_connector *
> > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > >> >>+{
> > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > >> >>+	struct drm_connector *connector;
> > > >> >>+	struct drm_device *dev = encoder->dev;
> > > >> >>+	int ret;
> > > >> >>+
> > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > >> >>+	if (ret)
> > > >> >>+		return ERR_PTR(ret);
> > > >> >
> > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > >> >
> > > >> >
> > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > >> >                        bool force)
> > > >> >{
> > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > >> >        struct drm_device *dev = connector->dev;
> > > >> >        int ret;
> > > >> >
> > > >> >        if (!ctx)
> > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > >> >
> > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > >> >        if (ret)
> > > >> >                return ret;
> > > >> >
> > > >> >        if (funcs->detect_ctx)
> > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > >> >        else if (connector->funcs->detect)
> > > >> >                ret = connector->funcs->detect(connector, force);
> > > >> >        else
> > > >> >                ret = connector_status_connected;
> > > >> >
> > > >> >        if (ret != connector->status)
> > > >> >                connector->epoch_counter += 1;
> > > >> >
> > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > >> >
> > > >> >
> > > >> >
> > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > >> >>+
> > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > >> >>+		if (!connector->state)
> > > >> >>+			continue;
> > > >> >>+
> > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > >> >>+			out_connector = connector;
> > > >> 
> > > >> 
> > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > >> 
> > > >> [   52.713030] Invalid return value -22 for connector detection
> > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > >> 0x63c
> > > >> [   52.714568] Modules linked in:
> > > >> 
> > > >> [   52.724546] Call trace:
> > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > >> 
> > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > >> for the first time, the functions mentioned above have not been called yet,
> > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > >
> > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > >1:N relationship, and only once a connector has been enabled it has an
> > > >encoder.
> > > >
> > > >If the connector is disabled, there's no associated encoder.
> > > 
> > > Does this prove that this API is not suitable for my application scenario: 
> > > Get the connector in the bridge's .detect callback, so this means that I may
> > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > 
> > I'd say, yes, please.
> 
> And I'd say no :)

Fair enough :-)

> There's no reason to deviate from the API other entities have here. It's
> just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> completely thought through and it's one of the part where it shows.
> 
> We have two alternative solutions: Either the driver creates the
> connector itself, since it doesn't seem to use any downstream bridge
> anyway, or we need a new bridge helper to find the connector on a bridge
> chain.
> 
> We have the iterator already, we just need a new accessor to retrieve
> the (optional) connector of a bridge, and if there's none, go to the
> next bridge and try again.

The problem is that there is no guarantee that the the created connector
is created for or linked to any bridge. For example, for msm driver I'm
waiting for several series to go in, but after that I plan to work on
moving connector creation to the generic code within the msm driver.

In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
perfectly legit not to have a bridge which has "connector of a bridge".
It is possible to create drm_bridge_connector on the drm_encoder's side
after the drm_bridge_attach() succeeds.

> There's a decent amount of variations that ideally call for tests too,
> but it should be pretty simple overall.

Maxime Ripard March 14, 2025, 5:40 p.m. UTC | #19

On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
> On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > > >Hi,
> > > > >
> > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > > >> >>association between a bridge driver and its connector was lost.
> > > > >> >>
> > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > > >> >>
> > > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > > >> >>atomic hooks bridges can implement.
> > > > >> >>
> > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > > >> >>interrupt handler.
> > > > >> >>
> > > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > > >> >>life easier.
> > > > >> >>
> > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > > >> >>---
> > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > > >> >> 2 files changed, 48 insertions(+)
> > > > >> >>
> > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > > >> >> 
> > > > >> >> 	return NULL;
> > > > >> >> }
> > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > > >> >> 
> > > > >> >>+/**
> > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > > >> >>+ * @encoder: The encoder to find the connector of
> > > > >> >>+ * @ctx: Modeset locking context
> > > > >> >>+ *
> > > > >> >>+ * This function finds and returns the connector currently assigned to
> > > > >> >>+ * an @encoder.
> > > > >> >>+ *
> > > > >> >>+ * Returns:
> > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > > >> >>+ * sequence must be restarted.
> > > > >> >>+ */
> > > > >> >>+struct drm_connector *
> > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > > >> >>+{
> > > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > > >> >>+	struct drm_connector *connector;
> > > > >> >>+	struct drm_device *dev = encoder->dev;
> > > > >> >>+	int ret;
> > > > >> >>+
> > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > >> >>+	if (ret)
> > > > >> >>+		return ERR_PTR(ret);
> > > > >> >
> > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > > >> >
> > > > >> >
> > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > > >> >                        bool force)
> > > > >> >{
> > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > > >> >        struct drm_device *dev = connector->dev;
> > > > >> >        int ret;
> > > > >> >
> > > > >> >        if (!ctx)
> > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > > >> >
> > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > >> >        if (ret)
> > > > >> >                return ret;
> > > > >> >
> > > > >> >        if (funcs->detect_ctx)
> > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > > >> >        else if (connector->funcs->detect)
> > > > >> >                ret = connector->funcs->detect(connector, force);
> > > > >> >        else
> > > > >> >                ret = connector_status_connected;
> > > > >> >
> > > > >> >        if (ret != connector->status)
> > > > >> >                connector->epoch_counter += 1;
> > > > >> >
> > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > > >> >>+
> > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > >> >>+		if (!connector->state)
> > > > >> >>+			continue;
> > > > >> >>+
> > > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > > >> >>+			out_connector = connector;
> > > > >> 
> > > > >> 
> > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > > >> 
> > > > >> [   52.713030] Invalid return value -22 for connector detection
> > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > > >> 0x63c
> > > > >> [   52.714568] Modules linked in:
> > > > >> 
> > > > >> [   52.724546] Call trace:
> > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > > >> 
> > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > > >> for the first time, the functions mentioned above have not been called yet,
> > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > > >
> > > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > > >1:N relationship, and only once a connector has been enabled it has an
> > > > >encoder.
> > > > >
> > > > >If the connector is disabled, there's no associated encoder.
> > > > 
> > > > Does this prove that this API is not suitable for my application scenario: 
> > > > Get the connector in the bridge's .detect callback, so this means that I may
> > > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > > 
> > > I'd say, yes, please.
> > 
> > And I'd say no :)
> 
> Fair enough :-)
> 
> > There's no reason to deviate from the API other entities have here. It's
> > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> > completely thought through and it's one of the part where it shows.
> > 
> > We have two alternative solutions: Either the driver creates the
> > connector itself, since it doesn't seem to use any downstream bridge
> > anyway, or we need a new bridge helper to find the connector on a bridge
> > chain.
> > 
> > We have the iterator already, we just need a new accessor to retrieve
> > the (optional) connector of a bridge, and if there's none, go to the
> > next bridge and try again.
> 
> The problem is that there is no guarantee that the the created connector
> is created for or linked to any bridge. For example, for msm driver I'm
> waiting for several series to go in, but after that I plan to work on
> moving connector creation to the generic code within the msm driver.
> 
> In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
> perfectly legit not to have a bridge which has "connector of a bridge".
> It is possible to create drm_bridge_connector on the drm_encoder's side
> after the drm_bridge_attach() succeeds.

Sure, but then I'd expect detect and get_modes to only be called *after*
that connector has been created, right?

Returning NULL in the case where we don't have a connector (yet?) in
such a case would make total sense to me, just like we return NULL if
the connector is disabled and doesn't have an encoder here.

Maxime

Dmitry Baryshkov March 14, 2025, 6:28 p.m. UTC | #20

On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote:
> On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
> > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > > > >Hi,
> > > > > >
> > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > > > >> >>association between a bridge driver and its connector was lost.
> > > > > >> >>
> > > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > > > >> >>
> > > > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > > > >> >>atomic hooks bridges can implement.
> > > > > >> >>
> > > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > > > >> >>interrupt handler.
> > > > > >> >>
> > > > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > > > >> >>life easier.
> > > > > >> >>
> > > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > > > >> >>---
> > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > > > >> >> 2 files changed, 48 insertions(+)
> > > > > >> >>
> > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > > > >> >> 
> > > > > >> >> 	return NULL;
> > > > > >> >> }
> > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > > > >> >> 
> > > > > >> >>+/**
> > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > > > >> >>+ * @encoder: The encoder to find the connector of
> > > > > >> >>+ * @ctx: Modeset locking context
> > > > > >> >>+ *
> > > > > >> >>+ * This function finds and returns the connector currently assigned to
> > > > > >> >>+ * an @encoder.
> > > > > >> >>+ *
> > > > > >> >>+ * Returns:
> > > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > > > >> >>+ * sequence must be restarted.
> > > > > >> >>+ */
> > > > > >> >>+struct drm_connector *
> > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > > > >> >>+{
> > > > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > > > >> >>+	struct drm_connector *connector;
> > > > > >> >>+	struct drm_device *dev = encoder->dev;
> > > > > >> >>+	int ret;
> > > > > >> >>+
> > > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > >> >>+	if (ret)
> > > > > >> >>+		return ERR_PTR(ret);
> > > > > >> >
> > > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > > > >> >
> > > > > >> >
> > > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > > > >> >                        bool force)
> > > > > >> >{
> > > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > > > >> >        struct drm_device *dev = connector->dev;
> > > > > >> >        int ret;
> > > > > >> >
> > > > > >> >        if (!ctx)
> > > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > > > >> >
> > > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > >> >        if (ret)
> > > > > >> >                return ret;
> > > > > >> >
> > > > > >> >        if (funcs->detect_ctx)
> > > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > > > >> >        else if (connector->funcs->detect)
> > > > > >> >                ret = connector->funcs->detect(connector, force);
> > > > > >> >        else
> > > > > >> >                ret = connector_status_connected;
> > > > > >> >
> > > > > >> >        if (ret != connector->status)
> > > > > >> >                connector->epoch_counter += 1;
> > > > > >> >
> > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > > > >> >
> > > > > >> >
> > > > > >> >
> > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > > > >> >>+
> > > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > >> >>+		if (!connector->state)
> > > > > >> >>+			continue;
> > > > > >> >>+
> > > > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > > > >> >>+			out_connector = connector;
> > > > > >> 
> > > > > >> 
> > > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > > > >> 
> > > > > >> [   52.713030] Invalid return value -22 for connector detection
> > > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > > > >> 0x63c
> > > > > >> [   52.714568] Modules linked in:
> > > > > >> 
> > > > > >> [   52.724546] Call trace:
> > > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > > > >> 
> > > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > > > >> for the first time, the functions mentioned above have not been called yet,
> > > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > > > >
> > > > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > > > >1:N relationship, and only once a connector has been enabled it has an
> > > > > >encoder.
> > > > > >
> > > > > >If the connector is disabled, there's no associated encoder.
> > > > > 
> > > > > Does this prove that this API is not suitable for my application scenario: 
> > > > > Get the connector in the bridge's .detect callback, so this means that I may
> > > > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > > > 
> > > > I'd say, yes, please.
> > > 
> > > And I'd say no :)
> > 
> > Fair enough :-)
> > 
> > > There's no reason to deviate from the API other entities have here. It's
> > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> > > completely thought through and it's one of the part where it shows.
> > > 
> > > We have two alternative solutions: Either the driver creates the
> > > connector itself, since it doesn't seem to use any downstream bridge
> > > anyway, or we need a new bridge helper to find the connector on a bridge
> > > chain.
> > > 
> > > We have the iterator already, we just need a new accessor to retrieve
> > > the (optional) connector of a bridge, and if there's none, go to the
> > > next bridge and try again.
> > 
> > The problem is that there is no guarantee that the the created connector
> > is created for or linked to any bridge. For example, for msm driver I'm
> > waiting for several series to go in, but after that I plan to work on
> > moving connector creation to the generic code within the msm driver.
> > 
> > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
> > perfectly legit not to have a bridge which has "connector of a bridge".
> > It is possible to create drm_bridge_connector on the drm_encoder's side
> > after the drm_bridge_attach() succeeds.
> 
> Sure, but then I'd expect detect and get_modes to only be called *after*
> that connector has been created, right?

Yes. But you can not get the connector by following bridge chain. Well,
unless you include encoder into the chain. If that's what you have had
in mind, then please excuse me, I didn't understand that from the
beginning.

But frankly speaking, I think it might be easier to pass down the
connector to the detect callback (as drm_connector_funcs.detect already
gets the connecor) rather than making bridge drivers go through the
chain to get the value that is already present in the caller function.

(For some other usecases I'd totally agree with you, especially if the
connector isn't already available on the caller side).

> Returning NULL in the case where we don't have a connector (yet?) in
> such a case would make total sense to me, just like we return NULL if
> the connector is disabled and doesn't have an encoder here.
> 
> Maxime

Maxime Ripard March 18, 2025, 3:51 p.m. UTC | #21

On Fri, Mar 14, 2025 at 08:28:22PM +0200, Dmitry Baryshkov wrote:
> On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote:
> > On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
> > > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> > > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > >Hi,
> > > > > > >
> > > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > > > > >> >>association between a bridge driver and its connector was lost.
> > > > > > >> >>
> > > > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > > > > >> >>
> > > > > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > > > > >> >>atomic hooks bridges can implement.
> > > > > > >> >>
> > > > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > > > > >> >>interrupt handler.
> > > > > > >> >>
> > > > > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > > > > >> >>life easier.
> > > > > > >> >>
> > > > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > > > > >> >>---
> > > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > > > > >> >> 2 files changed, 48 insertions(+)
> > > > > > >> >>
> > > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > > > > >> >> 
> > > > > > >> >> 	return NULL;
> > > > > > >> >> }
> > > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > > > > >> >> 
> > > > > > >> >>+/**
> > > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > > > > >> >>+ * @encoder: The encoder to find the connector of
> > > > > > >> >>+ * @ctx: Modeset locking context
> > > > > > >> >>+ *
> > > > > > >> >>+ * This function finds and returns the connector currently assigned to
> > > > > > >> >>+ * an @encoder.
> > > > > > >> >>+ *
> > > > > > >> >>+ * Returns:
> > > > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > > > > >> >>+ * sequence must be restarted.
> > > > > > >> >>+ */
> > > > > > >> >>+struct drm_connector *
> > > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > > > > >> >>+{
> > > > > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > > > > >> >>+	struct drm_connector *connector;
> > > > > > >> >>+	struct drm_device *dev = encoder->dev;
> > > > > > >> >>+	int ret;
> > > > > > >> >>+
> > > > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > >> >>+	if (ret)
> > > > > > >> >>+		return ERR_PTR(ret);
> > > > > > >> >
> > > > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > > > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > > > > >> >
> > > > > > >> >
> > > > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > > > > >> >                        bool force)
> > > > > > >> >{
> > > > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > > > > >> >        struct drm_device *dev = connector->dev;
> > > > > > >> >        int ret;
> > > > > > >> >
> > > > > > >> >        if (!ctx)
> > > > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > > > > >> >
> > > > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > >> >        if (ret)
> > > > > > >> >                return ret;
> > > > > > >> >
> > > > > > >> >        if (funcs->detect_ctx)
> > > > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > > > > >> >        else if (connector->funcs->detect)
> > > > > > >> >                ret = connector->funcs->detect(connector, force);
> > > > > > >> >        else
> > > > > > >> >                ret = connector_status_connected;
> > > > > > >> >
> > > > > > >> >        if (ret != connector->status)
> > > > > > >> >                connector->epoch_counter += 1;
> > > > > > >> >
> > > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > > > > >> >
> > > > > > >> >
> > > > > > >> >
> > > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > > > > >> >>+
> > > > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > > >> >>+		if (!connector->state)
> > > > > > >> >>+			continue;
> > > > > > >> >>+
> > > > > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > > > > >> >>+			out_connector = connector;
> > > > > > >> 
> > > > > > >> 
> > > > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > > > > >> 
> > > > > > >> [   52.713030] Invalid return value -22 for connector detection
> > > > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > > > > >> 0x63c
> > > > > > >> [   52.714568] Modules linked in:
> > > > > > >> 
> > > > > > >> [   52.724546] Call trace:
> > > > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > > > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > > > > >> 
> > > > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > > > > >> for the first time, the functions mentioned above have not been called yet,
> > > > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > > > > >
> > > > > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > > > > >1:N relationship, and only once a connector has been enabled it has an
> > > > > > >encoder.
> > > > > > >
> > > > > > >If the connector is disabled, there's no associated encoder.
> > > > > > 
> > > > > > Does this prove that this API is not suitable for my application scenario: 
> > > > > > Get the connector in the bridge's .detect callback, so this means that I may
> > > > > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > > > > 
> > > > > I'd say, yes, please.
> > > > 
> > > > And I'd say no :)
> > > 
> > > Fair enough :-)
> > > 
> > > > There's no reason to deviate from the API other entities have here. It's
> > > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> > > > completely thought through and it's one of the part where it shows.
> > > > 
> > > > We have two alternative solutions: Either the driver creates the
> > > > connector itself, since it doesn't seem to use any downstream bridge
> > > > anyway, or we need a new bridge helper to find the connector on a bridge
> > > > chain.
> > > > 
> > > > We have the iterator already, we just need a new accessor to retrieve
> > > > the (optional) connector of a bridge, and if there's none, go to the
> > > > next bridge and try again.
> > > 
> > > The problem is that there is no guarantee that the the created connector
> > > is created for or linked to any bridge. For example, for msm driver I'm
> > > waiting for several series to go in, but after that I plan to work on
> > > moving connector creation to the generic code within the msm driver.
> > > 
> > > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
> > > perfectly legit not to have a bridge which has "connector of a bridge".
> > > It is possible to create drm_bridge_connector on the drm_encoder's side
> > > after the drm_bridge_attach() succeeds.
> > 
> > Sure, but then I'd expect detect and get_modes to only be called *after*
> > that connector has been created, right?
> 
> Yes. But you can not get the connector by following bridge chain. Well,
> unless you include encoder into the chain. If that's what you have had
> in mind, then please excuse me, I didn't understand that from the
> beginning.

You can't include the encoder either, because the encoder doesn't have a
connector assigned yet at that time.

However, you can:

  - Store the bridge attach flags in drm_bridge

  - Create a hook that returns the connector a bridge creates, depending
    on the attach flags.

  - Create a helper that iterates over the next bridges until the
    previous hook returns !NULL. If it doesn't find anything, return
    NULL.

AFAIK, it solves all the problems being discussed here, while dealing
with legacy and new-style bridge drivers.

> But frankly speaking, I think it might be easier to pass down the
> connector to the detect callback (as drm_connector_funcs.detect already
> gets the connecor) rather than making bridge drivers go through the
> chain to get the value that is already present in the caller function.
> 
> (For some other usecases I'd totally agree with you, especially if the
> connector isn't already available on the caller side).

Still, we've tried to converge to the same API for all entities, it
feels like a step backward to me.

Maxime

Dmitry Baryshkov March 18, 2025, 7 p.m. UTC | #22

On Tue, Mar 18, 2025 at 04:51:19PM +0100, Maxime Ripard wrote:
> On Fri, Mar 14, 2025 at 08:28:22PM +0200, Dmitry Baryshkov wrote:
> > On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote:
> > > On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
> > > > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> > > > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > > > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > > > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > > >Hi,
> > > > > > > >
> > > > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > > > > > >> >>association between a bridge driver and its connector was lost.
> > > > > > > >> >>
> > > > > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > > > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > > > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > > > > > >> >>
> > > > > > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > > > > > >> >>atomic hooks bridges can implement.
> > > > > > > >> >>
> > > > > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > > > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > > > > > >> >>interrupt handler.
> > > > > > > >> >>
> > > > > > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > > > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > > > > > >> >>life easier.
> > > > > > > >> >>
> > > > > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > > > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > > > > > >> >>---
> > > > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > > > > > >> >> 2 files changed, 48 insertions(+)
> > > > > > > >> >>
> > > > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > > > > > >> >> 
> > > > > > > >> >> 	return NULL;
> > > > > > > >> >> }
> > > > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > > > > > >> >> 
> > > > > > > >> >>+/**
> > > > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > > > > > >> >>+ * @encoder: The encoder to find the connector of
> > > > > > > >> >>+ * @ctx: Modeset locking context
> > > > > > > >> >>+ *
> > > > > > > >> >>+ * This function finds and returns the connector currently assigned to
> > > > > > > >> >>+ * an @encoder.
> > > > > > > >> >>+ *
> > > > > > > >> >>+ * Returns:
> > > > > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > > > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > > > > > >> >>+ * sequence must be restarted.
> > > > > > > >> >>+ */
> > > > > > > >> >>+struct drm_connector *
> > > > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > > > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > > > > > >> >>+{
> > > > > > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > > > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > > > > > >> >>+	struct drm_connector *connector;
> > > > > > > >> >>+	struct drm_device *dev = encoder->dev;
> > > > > > > >> >>+	int ret;
> > > > > > > >> >>+
> > > > > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > > >> >>+	if (ret)
> > > > > > > >> >>+		return ERR_PTR(ret);
> > > > > > > >> >
> > > > > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > > > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > > > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > > > > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > > > > > >> >
> > > > > > > >> >
> > > > > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > > > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > > > > > >> >                        bool force)
> > > > > > > >> >{
> > > > > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > > > > > >> >        struct drm_device *dev = connector->dev;
> > > > > > > >> >        int ret;
> > > > > > > >> >
> > > > > > > >> >        if (!ctx)
> > > > > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > > > > > >> >
> > > > > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > > >> >        if (ret)
> > > > > > > >> >                return ret;
> > > > > > > >> >
> > > > > > > >> >        if (funcs->detect_ctx)
> > > > > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > > > > > >> >        else if (connector->funcs->detect)
> > > > > > > >> >                ret = connector->funcs->detect(connector, force);
> > > > > > > >> >        else
> > > > > > > >> >                ret = connector_status_connected;
> > > > > > > >> >
> > > > > > > >> >        if (ret != connector->status)
> > > > > > > >> >                connector->epoch_counter += 1;
> > > > > > > >> >
> > > > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > > > > > >> >
> > > > > > > >> >
> > > > > > > >> >
> > > > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > > > > > >> >>+
> > > > > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > > > >> >>+		if (!connector->state)
> > > > > > > >> >>+			continue;
> > > > > > > >> >>+
> > > > > > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > > > > > >> >>+			out_connector = connector;
> > > > > > > >> 
> > > > > > > >> 
> > > > > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > > > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > > > > > >> 
> > > > > > > >> [   52.713030] Invalid return value -22 for connector detection
> > > > > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > > > > > >> 0x63c
> > > > > > > >> [   52.714568] Modules linked in:
> > > > > > > >> 
> > > > > > > >> [   52.724546] Call trace:
> > > > > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > > > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > > > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > > > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > > > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > > > > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > > > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > > > > > >> 
> > > > > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > > > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > > > > > >> for the first time, the functions mentioned above have not been called yet,
> > > > > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > > > > > >
> > > > > > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > > > > > >1:N relationship, and only once a connector has been enabled it has an
> > > > > > > >encoder.
> > > > > > > >
> > > > > > > >If the connector is disabled, there's no associated encoder.
> > > > > > > 
> > > > > > > Does this prove that this API is not suitable for my application scenario: 
> > > > > > > Get the connector in the bridge's .detect callback, so this means that I may
> > > > > > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > > > > > 
> > > > > > I'd say, yes, please.
> > > > > 
> > > > > And I'd say no :)
> > > > 
> > > > Fair enough :-)
> > > > 
> > > > > There's no reason to deviate from the API other entities have here. It's
> > > > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> > > > > completely thought through and it's one of the part where it shows.
> > > > > 
> > > > > We have two alternative solutions: Either the driver creates the
> > > > > connector itself, since it doesn't seem to use any downstream bridge
> > > > > anyway, or we need a new bridge helper to find the connector on a bridge
> > > > > chain.
> > > > > 
> > > > > We have the iterator already, we just need a new accessor to retrieve
> > > > > the (optional) connector of a bridge, and if there's none, go to the
> > > > > next bridge and try again.
> > > > 
> > > > The problem is that there is no guarantee that the the created connector
> > > > is created for or linked to any bridge. For example, for msm driver I'm
> > > > waiting for several series to go in, but after that I plan to work on
> > > > moving connector creation to the generic code within the msm driver.
> > > > 
> > > > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
> > > > perfectly legit not to have a bridge which has "connector of a bridge".
> > > > It is possible to create drm_bridge_connector on the drm_encoder's side
> > > > after the drm_bridge_attach() succeeds.
> > > 
> > > Sure, but then I'd expect detect and get_modes to only be called *after*
> > > that connector has been created, right?
> > 
> > Yes. But you can not get the connector by following bridge chain. Well,
> > unless you include encoder into the chain. If that's what you have had
> > in mind, then please excuse me, I didn't understand that from the
> > beginning.
> 
> You can't include the encoder either, because the encoder doesn't have a
> connector assigned yet at that time.
> 
> However, you can:
> 
>   - Store the bridge attach flags in drm_bridge
> 
>   - Create a hook that returns the connector a bridge creates, depending
>     on the attach flags.
> 
>   - Create a helper that iterates over the next bridges until the
>     previous hook returns !NULL. If it doesn't find anything, return
>     NULL.
> 
> AFAIK, it solves all the problems being discussed here, while dealing
> with legacy and new-style bridge drivers.

I'm still fail to understand how does that solve the issue for new-style
bridges. How do we find the created drm_bridge_connector for them?

> 
> > But frankly speaking, I think it might be easier to pass down the
> > connector to the detect callback (as drm_connector_funcs.detect already
> > gets the connecor) rather than making bridge drivers go through the
> > chain to get the value that is already present in the caller function.
> > 
> > (For some other usecases I'd totally agree with you, especially if the
> > connector isn't already available on the caller side).
> 
> Still, we've tried to converge to the same API for all entities, it
> feels like a step backward to me.

I'd argue here a bit. The drm_connector interface has connector here.
drm_bridge is an extension/subpart of the drm_connector, so it would be
logical to extend that interface.

Andy Yan March 19, 2025, 7:21 a.m. UTC | #23

Hello，

At 2025-03-19 03:00:29, "Dmitry Baryshkov" <[e-mail]> wrote:
>On Tue, Mar 18, 2025 at 04:51:19PM +0100, Maxime Ripard wrote:
>> On Fri, Mar 14, 2025 at 08:28:22PM +0200, Dmitry Baryshkov wrote:
>> > On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote:
>> > > On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
>> > > > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
>> > > > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
>> > > > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
>> > > > > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
>> > > > > > > >Hi,
>> > > > > > > >
>> > > > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
>> > > > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
>> > > > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
>> > > > > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
>> > > > > > > >> >>association between a bridge driver and its connector was lost.
>> > > > > > > >> >>
>> > > > > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
>> > > > > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
>> > > > > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
>> > > > > > > >> >>
>> > > > > > > >> >>This was also made easier by providing drm_atomic_state directly to all
>> > > > > > > >> >>atomic hooks bridges can implement.
>> > > > > > > >> >>
>> > > > > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
>> > > > > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
>> > > > > > > >> >>interrupt handler.
>> > > > > > > >> >>
>> > > > > > > >> >>Let's introduce a function to retrieve the connector currently assigned
>> > > > > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
>> > > > > > > >> >>life easier.
>> > > > > > > >> >>
>> > > > > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
>> > > > > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
>> > > > > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
>> > > > > > > >> >>---
>> > > > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>> > > > > > > >> >> include/drm/drm_atomic.h     |  3 +++
>> > > > > > > >> >> 2 files changed, 48 insertions(+)
>> > > > > > > >> >>
>> > > > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > > > > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
>> > > > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
>> > > > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
>> > > > > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
>> > > > > > > >> >> 
>> > > > > > > >> >> 	return NULL;
>> > > > > > > >> >> }
>> > > > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
>> > > > > > > >> >> 
>> > > > > > > >> >>+/**
>> > > > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
>> > > > > > > >> >>+ * @encoder: The encoder to find the connector of
>> > > > > > > >> >>+ * @ctx: Modeset locking context
>> > > > > > > >> >>+ *
>> > > > > > > >> >>+ * This function finds and returns the connector currently assigned to
>> > > > > > > >> >>+ * an @encoder.
>> > > > > > > >> >>+ *
>> > > > > > > >> >>+ * Returns:
>> > > > > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
>> > > > > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
>> > > > > > > >> >>+ * sequence must be restarted.
>> > > > > > > >> >>+ */
>> > > > > > > >> >>+struct drm_connector *
>> > > > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
>> > > > > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
>> > > > > > > >> >>+{
>> > > > > > > >> >>+	struct drm_connector_list_iter conn_iter;
>> > > > > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
>> > > > > > > >> >>+	struct drm_connector *connector;
>> > > > > > > >> >>+	struct drm_device *dev = encoder->dev;
>> > > > > > > >> >>+	int ret;
>> > > > > > > >> >>+
>> > > > > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> > > > > > > >> >>+	if (ret)
>> > > > > > > >> >>+		return ERR_PTR(ret);
>> > > > > > > >> >
>> > > > > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
>> > > > > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
>> > > > > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
>> > > > > > > >> >which will hold connection_mutex first, so the deaklock happens:
>> > > > > > > >> >
>> > > > > > > >> >
>> > > > > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
>> > > > > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
>> > > > > > > >> >                        bool force)
>> > > > > > > >> >{
>> > > > > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>> > > > > > > >> >        struct drm_device *dev = connector->dev;
>> > > > > > > >> >        int ret;
>> > > > > > > >> >
>> > > > > > > >> >        if (!ctx)
>> > > > > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
>> > > > > > > >> >
>> > > > > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>> > > > > > > >> >        if (ret)
>> > > > > > > >> >                return ret;
>> > > > > > > >> >
>> > > > > > > >> >        if (funcs->detect_ctx)
>> > > > > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
>> > > > > > > >> >        else if (connector->funcs->detect)
>> > > > > > > >> >                ret = connector->funcs->detect(connector, force);
>> > > > > > > >> >        else
>> > > > > > > >> >                ret = connector_status_connected;
>> > > > > > > >> >
>> > > > > > > >> >        if (ret != connector->status)
>> > > > > > > >> >                connector->epoch_counter += 1;
>> > > > > > > >> >
>> > > > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
>> > > > > > > >> >
>> > > > > > > >> >
>> > > > > > > >> >
>> > > > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
>> > > > > > > >> >>+
>> > > > > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
>> > > > > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
>> > > > > > > >> >>+		if (!connector->state)
>> > > > > > > >> >>+			continue;
>> > > > > > > >> >>+
>> > > > > > > >> >>+		if (encoder == connector->state->best_encoder) {
>> > > > > > > >> >>+			out_connector = connector;
>> > > > > > > >> 
>> > > > > > > >> 
>> > > > > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
>> > > > > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
>> > > > > > > >> 
>> > > > > > > >> [   52.713030] Invalid return value -22 for connector detection
>> > > > > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
>> > > > > > > >> 0x63c
>> > > > > > > >> [   52.714568] Modules linked in:
>> > > > > > > >> 
>> > > > > > > >> [   52.724546] Call trace:
>> > > > > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
>> > > > > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
>> > > > > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
>> > > > > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
>> > > > > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
>> > > > > > > >> [   52.726706]  invoke_syscall+0x44/0x100
>> > > > > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
>> > > > > > > >> 
>> > > > > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
>> > > > > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
>> > > > > > > >> for the first time, the functions mentioned above have not been called yet,
>> > > > > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
>> > > > > > > >
>> > > > > > > >As far as I'm concerned, it's by design. Encoders and connectors have
>> > > > > > > >1:N relationship, and only once a connector has been enabled it has an
>> > > > > > > >encoder.
>> > > > > > > >
>> > > > > > > >If the connector is disabled, there's no associated encoder.
>> > > > > > > 
>> > > > > > > Does this prove that this API is not suitable for my application scenario: 
>> > > > > > > Get the connector in the bridge's .detect callback, so this means that I may
>> > > > > > > still need to modify the bridge's connector callback so that it can pass the connector ?
>> > > > > > 
>> > > > > > I'd say, yes, please.
>> > > > > 
>> > > > > And I'd say no :)
>> > > > 
>> > > > Fair enough :-)
>> > > > 
>> > > > > There's no reason to deviate from the API other entities have here. It's
>> > > > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
>> > > > > completely thought through and it's one of the part where it shows.
>> > > > > 
>> > > > > We have two alternative solutions: Either the driver creates the
>> > > > > connector itself, since it doesn't seem to use any downstream bridge
>> > > > > anyway, or we need a new bridge helper to find the connector on a bridge
>> > > > > chain.
>> > > > > 
>> > > > > We have the iterator already, we just need a new accessor to retrieve
>> > > > > the (optional) connector of a bridge, and if there's none, go to the
>> > > > > next bridge and try again.
>> > > > 
>> > > > The problem is that there is no guarantee that the the created connector
>> > > > is created for or linked to any bridge. For example, for msm driver I'm
>> > > > waiting for several series to go in, but after that I plan to work on
>> > > > moving connector creation to the generic code within the msm driver.
>> > > > 
>> > > > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
>> > > > perfectly legit not to have a bridge which has "connector of a bridge".
>> > > > It is possible to create drm_bridge_connector on the drm_encoder's side
>> > > > after the drm_bridge_attach() succeeds.
>> > > 
>> > > Sure, but then I'd expect detect and get_modes to only be called *after*
>> > > that connector has been created, right?
>> > 
>> > Yes. But you can not get the connector by following bridge chain. Well,
>> > unless you include encoder into the chain. If that's what you have had
>> > in mind, then please excuse me, I didn't understand that from the
>> > beginning.
>> 
>> You can't include the encoder either, because the encoder doesn't have a
>> connector assigned yet at that time.
>> 
>> However, you can:
>> 
>>   - Store the bridge attach flags in drm_bridge
>> 
>>   - Create a hook that returns the connector a bridge creates, depending
>>     on the attach flags.
>> 
>>   - Create a helper that iterates over the next bridges until the
>>     previous hook returns !NULL. If it doesn't find anything, return
>>     NULL.
>> 
>> AFAIK, it solves all the problems being discussed here, while dealing
>> with legacy and new-style bridge drivers.
>
>I'm still fail to understand how does that solve the issue for new-style
>bridges. How do we find the created drm_bridge_connector for them?
>
>> 
>> > But frankly speaking, I think it might be easier to pass down the
>> > connector to the detect callback (as drm_connector_funcs.detect already
>> > gets the connecor) rather than making bridge drivers go through the
>> > chain to get the value that is already present in the caller function.
>> > 
>> > (For some other usecases I'd totally agree with you, especially if the
>> > connector isn't already available on the caller side).
>> 
>> Still, we've tried to converge to the same API for all entities, it
>> feels like a step backward to me.
>
>I'd argue here a bit. The drm_connector interface has connector here.
>drm_bridge is an extension/subpart of the drm_connector, so it would be
>logical to extend that interface.

Yes, since all the calls to drm_bridge_funcs.detect are initiated by the connecter, 
I think it is reasonable to pass this connecter to the corresponding bridge.
And this can prevent iterates failures caused by various complex  bridge connection logic changes.

>
>
>-- 
>With best wishes
>Dmitry

Maxime Ripard March 21, 2025, 9:46 a.m. UTC | #24

On Tue, Mar 18, 2025 at 09:00:29PM +0200, Dmitry Baryshkov wrote:
> On Tue, Mar 18, 2025 at 04:51:19PM +0100, Maxime Ripard wrote:
> > On Fri, Mar 14, 2025 at 08:28:22PM +0200, Dmitry Baryshkov wrote:
> > > On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote:
> > > > On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
> > > > > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > > > > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > > > > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > > > >Hi,
> > > > > > > > >
> > > > > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > > > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > > > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > > > > > > >> >>association between a bridge driver and its connector was lost.
> > > > > > > > >> >>
> > > > > > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > > > > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > > > > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > > > > > > >> >>
> > > > > > > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > > > > > > >> >>atomic hooks bridges can implement.
> > > > > > > > >> >>
> > > > > > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > > > > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > > > > > > >> >>interrupt handler.
> > > > > > > > >> >>
> > > > > > > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > > > > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > > > > > > >> >>life easier.
> > > > > > > > >> >>
> > > > > > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > > > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > > > > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > > > > > > >> >>---
> > > > > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > > > > > > >> >> 2 files changed, 48 insertions(+)
> > > > > > > > >> >>
> > > > > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > > > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > > > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > > > > > > >> >> 
> > > > > > > > >> >> 	return NULL;
> > > > > > > > >> >> }
> > > > > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > > > > > > >> >> 
> > > > > > > > >> >>+/**
> > > > > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > > > > > > >> >>+ * @encoder: The encoder to find the connector of
> > > > > > > > >> >>+ * @ctx: Modeset locking context
> > > > > > > > >> >>+ *
> > > > > > > > >> >>+ * This function finds and returns the connector currently assigned to
> > > > > > > > >> >>+ * an @encoder.
> > > > > > > > >> >>+ *
> > > > > > > > >> >>+ * Returns:
> > > > > > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > > > > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > > > > > > >> >>+ * sequence must be restarted.
> > > > > > > > >> >>+ */
> > > > > > > > >> >>+struct drm_connector *
> > > > > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > > > > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > > > > > > >> >>+{
> > > > > > > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > > > > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > > > > > > >> >>+	struct drm_connector *connector;
> > > > > > > > >> >>+	struct drm_device *dev = encoder->dev;
> > > > > > > > >> >>+	int ret;
> > > > > > > > >> >>+
> > > > > > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > > > >> >>+	if (ret)
> > > > > > > > >> >>+		return ERR_PTR(ret);
> > > > > > > > >> >
> > > > > > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > > > > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > > > > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > > > > > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > > > > > > >> >
> > > > > > > > >> >
> > > > > > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > > > > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > > > > > > >> >                        bool force)
> > > > > > > > >> >{
> > > > > > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > > > > > > >> >        struct drm_device *dev = connector->dev;
> > > > > > > > >> >        int ret;
> > > > > > > > >> >
> > > > > > > > >> >        if (!ctx)
> > > > > > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > > > > > > >> >
> > > > > > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > > > >> >        if (ret)
> > > > > > > > >> >                return ret;
> > > > > > > > >> >
> > > > > > > > >> >        if (funcs->detect_ctx)
> > > > > > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > > > > > > >> >        else if (connector->funcs->detect)
> > > > > > > > >> >                ret = connector->funcs->detect(connector, force);
> > > > > > > > >> >        else
> > > > > > > > >> >                ret = connector_status_connected;
> > > > > > > > >> >
> > > > > > > > >> >        if (ret != connector->status)
> > > > > > > > >> >                connector->epoch_counter += 1;
> > > > > > > > >> >
> > > > > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > > > > > > >> >
> > > > > > > > >> >
> > > > > > > > >> >
> > > > > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > > > > > > >> >>+
> > > > > > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > > > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > > > > >> >>+		if (!connector->state)
> > > > > > > > >> >>+			continue;
> > > > > > > > >> >>+
> > > > > > > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > > > > > > >> >>+			out_connector = connector;
> > > > > > > > >> 
> > > > > > > > >> 
> > > > > > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > > > > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > > > > > > >> 
> > > > > > > > >> [   52.713030] Invalid return value -22 for connector detection
> > > > > > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > > > > > > >> 0x63c
> > > > > > > > >> [   52.714568] Modules linked in:
> > > > > > > > >> 
> > > > > > > > >> [   52.724546] Call trace:
> > > > > > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > > > > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > > > > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > > > > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > > > > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > > > > > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > > > > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > > > > > > >> 
> > > > > > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > > > > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > > > > > > >> for the first time, the functions mentioned above have not been called yet,
> > > > > > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > > > > > > >
> > > > > > > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > > > > > > >1:N relationship, and only once a connector has been enabled it has an
> > > > > > > > >encoder.
> > > > > > > > >
> > > > > > > > >If the connector is disabled, there's no associated encoder.
> > > > > > > > 
> > > > > > > > Does this prove that this API is not suitable for my application scenario: 
> > > > > > > > Get the connector in the bridge's .detect callback, so this means that I may
> > > > > > > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > > > > > > 
> > > > > > > I'd say, yes, please.
> > > > > > 
> > > > > > And I'd say no :)
> > > > > 
> > > > > Fair enough :-)
> > > > > 
> > > > > > There's no reason to deviate from the API other entities have here. It's
> > > > > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> > > > > > completely thought through and it's one of the part where it shows.
> > > > > > 
> > > > > > We have two alternative solutions: Either the driver creates the
> > > > > > connector itself, since it doesn't seem to use any downstream bridge
> > > > > > anyway, or we need a new bridge helper to find the connector on a bridge
> > > > > > chain.
> > > > > > 
> > > > > > We have the iterator already, we just need a new accessor to retrieve
> > > > > > the (optional) connector of a bridge, and if there's none, go to the
> > > > > > next bridge and try again.
> > > > > 
> > > > > The problem is that there is no guarantee that the the created connector
> > > > > is created for or linked to any bridge. For example, for msm driver I'm
> > > > > waiting for several series to go in, but after that I plan to work on
> > > > > moving connector creation to the generic code within the msm driver.
> > > > > 
> > > > > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
> > > > > perfectly legit not to have a bridge which has "connector of a bridge".
> > > > > It is possible to create drm_bridge_connector on the drm_encoder's side
> > > > > after the drm_bridge_attach() succeeds.
> > > > 
> > > > Sure, but then I'd expect detect and get_modes to only be called *after*
> > > > that connector has been created, right?
> > > 
> > > Yes. But you can not get the connector by following bridge chain. Well,
> > > unless you include encoder into the chain. If that's what you have had
> > > in mind, then please excuse me, I didn't understand that from the
> > > beginning.
> > 
> > You can't include the encoder either, because the encoder doesn't have a
> > connector assigned yet at that time.
> > 
> > However, you can:
> > 
> >   - Store the bridge attach flags in drm_bridge
> > 
> >   - Create a hook that returns the connector a bridge creates, depending
> >     on the attach flags.
> > 
> >   - Create a helper that iterates over the next bridges until the
> >     previous hook returns !NULL. If it doesn't find anything, return
> >     NULL.
> > 
> > AFAIK, it solves all the problems being discussed here, while dealing
> > with legacy and new-style bridge drivers.
> 
> I'm still fail to understand how does that solve the issue for new-style
> bridges. How do we find the created drm_bridge_connector for them?

Sigh, for some reason I was remembering that drm_bridge_connector was a
bridge itself, which it isn't. My bad. But I guess it still applies. If
we make drm_bridge_connector a bridge, then it works, doesn't it?

> > 
> > > But frankly speaking, I think it might be easier to pass down the
> > > connector to the detect callback (as drm_connector_funcs.detect already
> > > gets the connecor) rather than making bridge drivers go through the
> > > chain to get the value that is already present in the caller function.
> > > 
> > > (For some other usecases I'd totally agree with you, especially if the
> > > connector isn't already available on the caller side).
> > 
> > Still, we've tried to converge to the same API for all entities, it
> > feels like a step backward to me.
> 
> I'd argue here a bit. The drm_connector interface has connector here.
> drm_bridge is an extension/subpart of the drm_connector, so it would be
> logical to extend that interface.

The drm_connector interface has the connector because it's a connector.
Just like CRTC atomic_check has a crtc, but you wouldn't pass the crtc
pointer to drm_bridge atomic_check.

I still think it goes against the trend and work we've been doing over
the years. And we should at least *try* something different instead of
just taking the easy way out. Or accepting to duplicate the helpers that
started the discussion, or to create a connector directyl instead of
using drm_bridge_connector for that driver.

Maxime

Dmitry Baryshkov March 23, 2025, 2:22 a.m. UTC | #25

On Fri, Mar 21, 2025 at 10:46:56AM +0100, Maxime Ripard wrote:
> On Tue, Mar 18, 2025 at 09:00:29PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Mar 18, 2025 at 04:51:19PM +0100, Maxime Ripard wrote:
> > > On Fri, Mar 14, 2025 at 08:28:22PM +0200, Dmitry Baryshkov wrote:
> > > > On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote:
> > > > > On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote:
> > > > > > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote:
> > > > > > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov wrote:
> > > > > > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote:
> > > > > > > > > At 2025-03-13 19:55:33, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > > > > >Hi,
> > > > > > > > > >
> > > > > > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan wrote:
> > > > > > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[e-mail]> wrote:
> > > > > > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" <[e-mail]> wrote:
> > > > > > > > > >> >>With the bridges switching over to drm_bridge_connector, the direct
> > > > > > > > > >> >>association between a bridge driver and its connector was lost.
> > > > > > > > > >> >>
> > > > > > > > > >> >>This is mitigated for atomic bridge drivers by the fact you can access
> > > > > > > > > >> >>the encoder, and then call drm_atomic_get_old_connector_for_encoder() or
> > > > > > > > > >> >>drm_atomic_get_new_connector_for_encoder() with drm_atomic_state.
> > > > > > > > > >> >>
> > > > > > > > > >> >>This was also made easier by providing drm_atomic_state directly to all
> > > > > > > > > >> >>atomic hooks bridges can implement.
> > > > > > > > > >> >>
> > > > > > > > > >> >>However, bridge drivers don't have a way to access drm_atomic_state
> > > > > > > > > >> >>outside of the modeset path, like from the hotplug interrupt path or any
> > > > > > > > > >> >>interrupt handler.
> > > > > > > > > >> >>
> > > > > > > > > >> >>Let's introduce a function to retrieve the connector currently assigned
> > > > > > > > > >> >>to an encoder, without using drm_atomic_state, to make these drivers'
> > > > > > > > > >> >>life easier.
> > > > > > > > > >> >>
> > > > > > > > > >> >>Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > > > > > > >> >>Co-developed-by: Simona Vetter <[e-mail]>
> > > > > > > > > >> >>Signed-off-by: Maxime Ripard <[e-mail]>
> > > > > > > > > >> >>---
> > > > > > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >> >> include/drm/drm_atomic.h     |  3 +++
> > > > > > > > > >> >> 2 files changed, 48 insertions(+)
> > > > > > > > > >> >>
> > > > > > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > >> >>index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
> > > > > > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > >> >>@@ -985,10 +985,55 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
> > > > > > > > > >> >> 
> > > > > > > > > >> >> 	return NULL;
> > > > > > > > > >> >> }
> > > > > > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> > > > > > > > > >> >> 
> > > > > > > > > >> >>+/**
> > > > > > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
> > > > > > > > > >> >>+ * @encoder: The encoder to find the connector of
> > > > > > > > > >> >>+ * @ctx: Modeset locking context
> > > > > > > > > >> >>+ *
> > > > > > > > > >> >>+ * This function finds and returns the connector currently assigned to
> > > > > > > > > >> >>+ * an @encoder.
> > > > > > > > > >> >>+ *
> > > > > > > > > >> >>+ * Returns:
> > > > > > > > > >> >>+ * The connector connected to @encoder, or an error pointer otherwise.
> > > > > > > > > >> >>+ * When the error is EDEADLK, a deadlock has been detected and the
> > > > > > > > > >> >>+ * sequence must be restarted.
> > > > > > > > > >> >>+ */
> > > > > > > > > >> >>+struct drm_connector *
> > > > > > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
> > > > > > > > > >> >>+				     struct drm_modeset_acquire_ctx *ctx)
> > > > > > > > > >> >>+{
> > > > > > > > > >> >>+	struct drm_connector_list_iter conn_iter;
> > > > > > > > > >> >>+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
> > > > > > > > > >> >>+	struct drm_connector *connector;
> > > > > > > > > >> >>+	struct drm_device *dev = encoder->dev;
> > > > > > > > > >> >>+	int ret;
> > > > > > > > > >> >>+
> > > > > > > > > >> >>+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > > > > >> >>+	if (ret)
> > > > > > > > > >> >>+		return ERR_PTR(ret);
> > > > > > > > > >> >
> > > > > > > > > >> >It seems that this will cause a deadlock when called from a  hotplug handling path,
> > > > > > > > > >> >I have a WIP DP diver[0],  which suggested by Dmitry to use this API from a 
> > > > > > > > > >> >&drm_bridge_funcs.detect callback to get the connector,  as detect is called by drm_helper_probe_detect,
> > > > > > > > > >> >which will hold connection_mutex first, so the deaklock happens:
> > > > > > > > > >> >
> > > > > > > > > >> >
> > > > > > > > > >> >drm_helper_probe_detect(struct drm_connector *connector,
> > > > > > > > > >> >                        struct drm_modeset_acquire_ctx *ctx,
> > > > > > > > > >> >                        bool force)
> > > > > > > > > >> >{
> > > > > > > > > >> >        const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > > > > > > > > >> >        struct drm_device *dev = connector->dev;
> > > > > > > > > >> >        int ret;
> > > > > > > > > >> >
> > > > > > > > > >> >        if (!ctx)
> > > > > > > > > >> >                return drm_helper_probe_detect_ctx(connector, force);
> > > > > > > > > >> >
> > > > > > > > > >> >        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> > > > > > > > > >> >        if (ret)
> > > > > > > > > >> >                return ret;
> > > > > > > > > >> >
> > > > > > > > > >> >        if (funcs->detect_ctx)
> > > > > > > > > >> >                ret = funcs->detect_ctx(connector, ctx, force);
> > > > > > > > > >> >        else if (connector->funcs->detect)
> > > > > > > > > >> >                ret = connector->funcs->detect(connector, force);
> > > > > > > > > >> >        else
> > > > > > > > > >> >                ret = connector_status_connected;
> > > > > > > > > >> >
> > > > > > > > > >> >        if (ret != connector->status)
> > > > > > > > > >> >                connector->epoch_counter += 1;
> > > > > > > > > >> >
> > > > > > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass a connector for this case ?
> > > > > > > > > >> >
> > > > > > > > > >> >
> > > > > > > > > >> >
> > > > > > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[e-mail]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094
> > > > > > > > > >> >>+
> > > > > > > > > >> >>+	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > > > > > > >> >>+	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > > > > > >> >>+		if (!connector->state)
> > > > > > > > > >> >>+			continue;
> > > > > > > > > >> >>+
> > > > > > > > > >> >>+		if (encoder == connector->state->best_encoder) {
> > > > > > > > > >> >>+			out_connector = connector;
> > > > > > > > > >> 
> > > > > > > > > >> 
> > > > > > > > > >> When try to use this patch in my bridge driver,  I found that the connector->state->best_encoder 
> > > > > > > > > >>  maybe NULL when   drm_bridge_funcs.detect or drm_bridge_funcs.detect_ctx is  called:
> > > > > > > > > >> 
> > > > > > > > > >> [   52.713030] Invalid return value -22 for connector detection
> > > > > > > > > >> [   52.713539] WARNING: CPU: 7 PID: 288 at drivers/gpu/drm/drm_probe_helper.c:602 drm_helper_probe_single_connector_modes+0x5e0/
> > > > > > > > > >> 0x63c
> > > > > > > > > >> [   52.714568] Modules linked in:
> > > > > > > > > >> 
> > > > > > > > > >> [   52.724546] Call trace:
> > > > > > > > > >> [   52.724762]  drm_helper_probe_single_connector_modes+0x5e0/0x63c (P)
> > > > > > > > > >> [   52.725319]  drm_mode_getconnector+0x2a4/0x488
> > > > > > > > > >> [   52.725711]  drm_ioctl_kernel+0xb4/0x11c
> > > > > > > > > >> [   52.726057]  drm_ioctl+0x22c/0x544
> > > > > > > > > >> [   52.726358]  __arm64_sys_ioctl+0xac/0xe0
> > > > > > > > > >> [   52.726706]  invoke_syscall+0x44/0x100
> > > > > > > > > >> [   52.727039]  el0_svc_common.constprop.0+0x3c/0xd4
> > > > > > > > > >> 
> > > > > > > > > >> This is because  best_encoder is set by set_best_encoder, which is called from
> > > > > > > > > >> drm_atomic_helper_check_modeset. When we call drm_mode_getconnector 
> > > > > > > > > >> for the first time, the functions mentioned above have not been called yet,
> > > > > > > > > >> then we can't match the encoder from connector->state->best_encoder for this case.
> > > > > > > > > >
> > > > > > > > > >As far as I'm concerned, it's by design. Encoders and connectors have
> > > > > > > > > >1:N relationship, and only once a connector has been enabled it has an
> > > > > > > > > >encoder.
> > > > > > > > > >
> > > > > > > > > >If the connector is disabled, there's no associated encoder.
> > > > > > > > > 
> > > > > > > > > Does this prove that this API is not suitable for my application scenario: 
> > > > > > > > > Get the connector in the bridge's .detect callback, so this means that I may
> > > > > > > > > still need to modify the bridge's connector callback so that it can pass the connector ?
> > > > > > > > 
> > > > > > > > I'd say, yes, please.
> > > > > > > 
> > > > > > > And I'd say no :)
> > > > > > 
> > > > > > Fair enough :-)
> > > > > > 
> > > > > > > There's no reason to deviate from the API other entities have here. It's
> > > > > > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR hasn't been
> > > > > > > completely thought through and it's one of the part where it shows.
> > > > > > > 
> > > > > > > We have two alternative solutions: Either the driver creates the
> > > > > > > connector itself, since it doesn't seem to use any downstream bridge
> > > > > > > anyway, or we need a new bridge helper to find the connector on a bridge
> > > > > > > chain.
> > > > > > > 
> > > > > > > We have the iterator already, we just need a new accessor to retrieve
> > > > > > > the (optional) connector of a bridge, and if there's none, go to the
> > > > > > > next bridge and try again.
> > > > > > 
> > > > > > The problem is that there is no guarantee that the the created connector
> > > > > > is created for or linked to any bridge. For example, for msm driver I'm
> > > > > > waiting for several series to go in, but after that I plan to work on
> > > > > > moving connector creation to the generic code within the msm driver.
> > > > > > 
> > > > > > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place it is
> > > > > > perfectly legit not to have a bridge which has "connector of a bridge".
> > > > > > It is possible to create drm_bridge_connector on the drm_encoder's side
> > > > > > after the drm_bridge_attach() succeeds.
> > > > > 
> > > > > Sure, but then I'd expect detect and get_modes to only be called *after*
> > > > > that connector has been created, right?
> > > > 
> > > > Yes. But you can not get the connector by following bridge chain. Well,
> > > > unless you include encoder into the chain. If that's what you have had
> > > > in mind, then please excuse me, I didn't understand that from the
> > > > beginning.
> > > 
> > > You can't include the encoder either, because the encoder doesn't have a
> > > connector assigned yet at that time.
> > > 
> > > However, you can:
> > > 
> > >   - Store the bridge attach flags in drm_bridge
> > > 
> > >   - Create a hook that returns the connector a bridge creates, depending
> > >     on the attach flags.
> > > 
> > >   - Create a helper that iterates over the next bridges until the
> > >     previous hook returns !NULL. If it doesn't find anything, return
> > >     NULL.
> > > 
> > > AFAIK, it solves all the problems being discussed here, while dealing
> > > with legacy and new-style bridge drivers.
> > 
> > I'm still fail to understand how does that solve the issue for new-style
> > bridges. How do we find the created drm_bridge_connector for them?
> 
> Sigh, for some reason I was remembering that drm_bridge_connector was a
> bridge itself, which it isn't. My bad. But I guess it still applies. If
> we make drm_bridge_connector a bridge, then it works, doesn't it?

I'd rather not. This would complicate other bridges using
drm_bridge_connector (e.g. ite-it6263, ti-sn65dsi86)

> > > > But frankly speaking, I think it might be easier to pass down the
> > > > connector to the detect callback (as drm_connector_funcs.detect already
> > > > gets the connecor) rather than making bridge drivers go through the
> > > > chain to get the value that is already present in the caller function.
> > > > 
> > > > (For some other usecases I'd totally agree with you, especially if the
> > > > connector isn't already available on the caller side).
> > > 
> > > Still, we've tried to converge to the same API for all entities, it
> > > feels like a step backward to me.
> > 
> > I'd argue here a bit. The drm_connector interface has connector here.
> > drm_bridge is an extension/subpart of the drm_connector, so it would be
> > logical to extend that interface.
> 
> The drm_connector interface has the connector because it's a connector.
> Just like CRTC atomic_check has a crtc, but you wouldn't pass the crtc
> pointer to drm_bridge atomic_check.
> 
> I still think it goes against the trend and work we've been doing over
> the years. And we should at least *try* something different instead of
> just taking the easy way out. Or accepting to duplicate the helpers that
> started the discussion, or to create a connector directyl instead of
> using drm_bridge_connector for that driver.

I think passing drm_connector and drm_bridge matches the pattern started
by edid_read() and several hdmi_audio_*() callbacks. They are receiving
both the bridge and the connector for exactly the same reason - the
callbacks needs both _and_ the connector is well known in the calling
code.

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -985,10 +985,55 @@  drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
 
 	return NULL;
 }
 EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
 
+/**
+ * drm_atomic_get_connector_for_encoder - Get connector currently assigned to an encoder
+ * @encoder: The encoder to find the connector of
+ * @ctx: Modeset locking context
+ *
+ * This function finds and returns the connector currently assigned to
+ * an @encoder.
+ *
+ * Returns:
+ * The connector connected to @encoder, or an error pointer otherwise.
+ * When the error is EDEADLK, a deadlock has been detected and the
+ * sequence must be restarted.
+ */
+struct drm_connector *
+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
+				     struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *out_connector = ERR_PTR(-EINVAL);
+	struct drm_connector *connector;
+	struct drm_device *dev = encoder->dev;
+	int ret;
+
+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
+	drm_connector_list_iter_begin(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		if (!connector->state)
+			continue;
+
+		if (encoder == connector->state->best_encoder) {
+			out_connector = connector;
+			break;
+		}
+	}
+	drm_connector_list_iter_end(&conn_iter);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	return out_connector;
+}
+EXPORT_SYMBOL(drm_atomic_get_connector_for_encoder);
+
+
 /**
  * drm_atomic_get_old_crtc_for_encoder - Get old crtc for an encoder
  * @state: Atomic state
  * @encoder: The encoder to fetch the crtc state for
  *
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 4c673f0698fef6b60f77db980378d5e88e0e250e..38636a593c9d98cadda85ccd67326cb152f0dd27 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -623,10 +623,13 @@  struct drm_connector *
 drm_atomic_get_old_connector_for_encoder(const struct drm_atomic_state *state,
 					 struct drm_encoder *encoder);
 struct drm_connector *
 drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
 					 struct drm_encoder *encoder);
+struct drm_connector *
+drm_atomic_get_connector_for_encoder(const struct drm_encoder *encoder,
+				     struct drm_modeset_acquire_ctx *ctx);
 
 struct drm_crtc *
 drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
 					 struct drm_encoder *encoder);
 struct drm_crtc *