[PATCH] drm/probe-helper: Sort and set crtcinfo before dropping the modeset locks

[PATCH] drm/probe-helper: Sort and set crtcinfo before dropping the modeset locks

[Hugo Sales]

Currently, drm_helper_probe_single_connector_modes() holds
connection_mutex while it probes and prunes connector->modes, then drops
it before sorting the list and filling in the crtc_* fields of each
mode. The caller still holds mode_config.mutex at that point, but
drm_atomic_check_only() never takes it. The only lock it holds on the
connector is connection_mutex, which drm_atomic_get_connector_state()
takes for it

amdgpu walks connector->modes from its atomic_check, in
create_stream_for_sink() and amdgpu_dm_get_highest_refresh_rate_mode(),
holding only the locks of the atomic acquire context. list_sort()
NULL-terminates the list while it runs, so that walk can follow a NULL
next pointer or read a mode whose crtc_* fields are being rewritten

Sort the list and set crtcinfo before dropping the locks, as the
function already does when it probes and prunes the modes. As far as I
can tell, the unlock ended up in front of the sort when
commit 6c5ed5ae353c ("drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.")
first took connection_mutex here, and that commit gives no reason for
the order

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 856b40fdaac4a..2085d93651b21 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -703,11 +703,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	}
 
 exit:
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-
-	if (list_empty(&connector->modes))
-		return 0;
+	if (list_empty(&connector->modes)) {
+		count = 0;
+		goto unlock;
+	}
 
 	drm_mode_sort(&connector->modes);
 
@@ -720,6 +719,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 			    DRM_MODE_ARG(mode));
 	}
 
+unlock:
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
 	return count;
 }
 EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
-- 
2.56.0

Re: [PATCH] drm/probe-helper: Sort and set crtcinfo before dropping the modeset locks

[Ville Syrjälä]

On Sat, Oct 31, 2026 at 04:19:52PM +0000, Hugo Sales wrote:
> Currently, drm_helper_probe_single_connector_modes() holds
> connection_mutex while it probes and prunes connector->modes, then drops
> it before sorting the list and filling in the crtc_* fields of each
> mode. The caller still holds mode_config.mutex at that point, but
> drm_atomic_check_only() never takes it. The only lock it holds on the
> connector is connection_mutex, which drm_atomic_get_connector_state()
> takes for it
> 
> amdgpu walks connector->modes from its atomic_check, in

That is just wrong. connector->modes is protected by mode_config.mutex

The fact that amdgpu apparently uses scaling for external 
displays is a very odd as well. No other driver does that for
external displays. That said, I've thought about adding that for
i915/xe, but I think it should be done via some uapi so that
userspace retains control over the actual display timings.
I'd really prefer to do it that way for laptop panels as well,
but the old uapi is already carved in stone there so can't
change it :(

> create_stream_for_sink() and amdgpu_dm_get_highest_refresh_rate_mode(),
> holding only the locks of the atomic acquire context. list_sort()
> NULL-terminates the list while it runs, so that walk can follow a NULL
> next pointer or read a mode whose crtc_* fields are being rewritten
> 
> Sort the list and set crtcinfo before dropping the locks, as the
> function already does when it probes and prunes the modes. As far as I
> can tell, the unlock ended up in front of the sort when
> commit 6c5ed5ae353c ("drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.")
> first took connection_mutex here, and that commit gives no reason for
> the order
> 
> Signed-off-by: Hugo Sales <hugo@hsal.es>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 856b40fdaac4a..2085d93651b21 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -703,11 +703,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  	}
>  
>  exit:
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -
> -	if (list_empty(&connector->modes))
> -		return 0;
> +	if (list_empty(&connector->modes)) {
> +		count = 0;
> +		goto unlock;
> +	}
>  
>  	drm_mode_sort(&connector->modes);
>  
> @@ -720,6 +719,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  			    DRM_MODE_ARG(mode));
>  	}
>  
> +unlock:
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);

I suppose we aren't moving all that much work under the lock
here, so maybe fine. Though I kinda hate that we even have
to hold the lock across the mode validation. But as long as
we're looking at the connector state in connector.mode_valid()
I suppose there is no choice :/

Any new locking rules would need to be properly documented.

And regardless of changing the locking rules or not, someone
should probably sprinkle lockdep asserts to bunch of places
where connector->modes is consulted. Perhaps even introduce
custom list iterator macros that include the assert...

> +
>  	return count;
>  }
>  EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
> -- 
> 2.56.0

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/probe-helper: Sort and set crtcinfo before dropping the modeset locks

[Hugo Sales]

On 11/02/2026 12:05 PM WET Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Sat, Oct 31, 2026 at 04:19:52PM +0000, Hugo Sales wrote:
> > amdgpu walks connector->modes from its atomic_check, in
>
> That is just wrong. connector->modes is protected by mode_config.mutex

You're right, I should have fixed the real issue rather than work around
it. Please drop this one

I'll try to fix it in amdgpu instead. atomic_check can't take
mode_config.mutex there, so amdgpu would pick the mode it needs while
probing and keep its own copy, like it already does for freesync_vid_base

> And regardless of changing the locking rules or not, someone
> should probably sprinkle lockdep asserts to bunch of places
> where connector->modes is consulted. Perhaps even introduce
> custom list iterator macros that include the assert...

Makes sense. amdgpu would trip them today, so I'll look at that once
the amdgpu side is fixed

Thanks for the review,
Hugo

Re: [PATCH] drm/probe-helper: Sort and set crtcinfo before dropping the modeset locks

[Ville Syrjälä]

On Mon, Nov 02, 2026 at 03:26:42PM +0000, Hugo Sales wrote:
> On 11/02/2026 12:05 PM WET Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Sat, Oct 31, 2026 at 04:19:52PM +0000, Hugo Sales wrote:
> > > amdgpu walks connector->modes from its atomic_check, in
> >
> > That is just wrong. connector->modes is protected by mode_config.mutex
> 
> You're right, I should have fixed the real issue rather than work around
> it. Please drop this one
> 
> I'll try to fix it in amdgpu instead. atomic_check can't take
> mode_config.mutex there, so amdgpu would pick the mode it needs while
> probing and keep its own copy, like it already does for freesync_vid_base

Yeah, that should be doable.

> 
> > And regardless of changing the locking rules or not, someone
> > should probably sprinkle lockdep asserts to bunch of places
> > where connector->modes is consulted. Perhaps even introduce
> > custom list iterator macros that include the assert...
> 
> Makes sense. amdgpu would trip them today, so I'll look at that once
> the amdgpu side is fixed

Sounds good.

-- 
Ville Syrjälä
Intel