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

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

On Fri, May 22, 2026 at 06:58:26PM +0100, 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.

I've been cursing at this delayed_event hack a couple of times.
Seems to me that the correct solution would be to make
drm_client_dev_hotplug() schedule its own work and do the actual
work there instead of this direct call we have right now.

-- 
Ville Syrjälä
Intel