Re: [PATCH v9 2/2] drm: Send per-connector hotplug events

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Friday, 22 May 2026 19:58:26 Central European Summer Time Daniel Stone wrote:
> On Fri, 22 May 2026 at 14:06, Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> > On Thursday, 21 May 2026 17:00:17 Central European Summer Time Daniel Stone wrote:
> > > On Wed, 22 Apr 2026 at 19:24, Nicolas Frattaroli
> > > <nicolas.frattaroli@collabora.com> wrote:
> > > > +       /* Pick up any changes detected by the probe functions. */
> > > > +       if (dev->mode_config.delayed_event) {
> > >
> > > Not your doing, as it was just the same before, but doesn't this read
> > > need to be protected by the mode_config mutex?
> >
> > It looks like the fuzzy meaning of the mode_config.mutex came to
> > bite here. It is set to true with the lock held in
> > drm_helper_probe_single_connector_modes(), but set to false in this
> > function without the lock, and always read without the lock
> > (drm_kms_helper_poll_enable(), reschedule_output_poll_work()). Some
> > of these calls happen in paths where it might be inconvenient to take
> > a lock this coarse.
> >
> > Specifically, drm_helper_probe_single_connector_modes() has this
> > comment right before setting delayed_event to true:
> >
> >         /*
> >          * The hotplug event code might call into the fb
> >          * helpers, and so expects that we do not hold any
> >          * locks. Fire up the poll struct instead, it will
> >          * disable itself again.
> >          */
> >
> > So there was a problem with accessing parts under a lock before,
> > so if we're reintroducing the coarse lock wherever the boolean is
> > accessed we might be going backwards here.
> 
> Yeah, that makes sense - those are definitely called from an IRQ
> context so it wouldn't be safe to take the mode_config mutex.
> 
> I wonder if the safest path which guarantees eventual consistency is
> to change the !trylock(mode_config) { goto out } path in
> output_poll_execute() to jump directly to the schedule_delayed_work()?
> That way we could read and modify delayed_event under the mode_config
> mutex; the if (changed) path will not be taken if it wasn't possible
> to take the mode_config mutex.

If the drm_kms_helper_poll module parameter is set to false (i.e. the
non-default setting), with my patch we'd then also need to think about
the behavior if delayed_event == true in that case, since it also goes
to `out` without taking the lock.

I think I need to ponder this particular orb some more. I suspect it'd
be fine to move the delayed_event clearing + sending to before the
mutex unlock, since failing to lock the mutex will just make it repoll
and try again in 0.1 seconds. The event would only be lost if the
another delayed_event arrives before it's processed.

I think Ville's on the money here in that the users of delayed_event
might want to call a drm_client_dev_hotplug that does the work
scheduling instead, but that might be a bigger thing to work towards.

Kind regards,
Nicolas Frattaroli

> 
> Cheers,
> Daniel
>