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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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