On Fri, May 22, 2026 at 06:58:26PM +0100, Daniel Stone wrote: > On Fri, 22 May 2026 at 14:06, Nicolas Frattaroli > <[email protected]> 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 > > > <[email protected]> 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
