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

[Nicolas Frattaroli]

On Sunday, 19 April 2026 03:02:29 Central European Summer Time Dmitry Baryshkov wrote:
> On Wed, Apr 15, 2026 at 07:59:40PM +0200, Nicolas Frattaroli wrote:
> > From: Marius Vlad <marius.vlad@xxxxxxxxxxxxx>
> > 
> > Use the new pending_hp member of drm_connector to always send
> > per-connector hotplug events for those connectors that need it, rather
> > than sending a global event, or only an event for one connector.
> > 
> > On the HPD (Hot Plug Detect) path this change notifies all connectors,
> > rather than just first changed connector.
> 
> I'd like to point out that there are enough places were the code calls
> drm_kms_helper_hotplug_event() directly. Some of them are ridiculous and
> can easily be sorted out (e.g. bridges or single connector drivers,
> where we know exactly for which connector or bridge to report the HPD
> event).

I'll see if I can fix those easy cases up in the next revision.

> Others might be not that obvious. So, if we are to promise to
> userspace to always deliver connector-based events, there is more work
> to be done (and it probably makes a nice TODO item).

I agree. I'll need to reword the message to not claim it "always"
sends per-connector events; the existence of that statement is to
be blamed on me since I wrote that.

> > 
> > The polling path is changed to no longer send a connector-less hotplug
> > event, but similarly send a hotplug event for each changed connector.
> > 
> > Signed-off-by: Marius Vlad <marius.vlad@xxxxxxxxxxxxx>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@xxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index f8cbd6713960..5c39f27ada1d 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -860,8 +860,14 @@ static void output_poll_execute(struct work_struct *work)
> >  	mutex_unlock(&dev->mode_config.mutex);
> >  
> >  out:
> > -	if (changed)
> > -		drm_kms_helper_hotplug_event(dev);
> > +	if (changed) {
> > +		drm_connector_list_iter_begin(dev, &conn_iter);
> > +		drm_for_each_connector_iter(connector, &conn_iter) {
> > +			if (connector->pending_hp)
> > +				drm_kms_helper_connector_hotplug_event(connector);
> > +		}
> > +		drm_connector_list_iter_end(&conn_iter);
> > +	}
> >  
> >  	if (repoll)
> >  		schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
> > @@ -1124,10 +1130,16 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
> >  	drm_connector_list_iter_end(&conn_iter);
> >  	mutex_unlock(&dev->mode_config.mutex);
> >  
> > -	if (changed == 1)
> > +	if (changed == 1) {
> >  		drm_kms_helper_connector_hotplug_event(first_changed_connector);
> > -	else if (changed > 0)
> > -		drm_kms_helper_hotplug_event(dev);
> > +	} else if (changed > 0) {
> > +		drm_connector_list_iter_begin(dev, &conn_iter);
> > +		drm_for_each_connector_iter(connector, &conn_iter) {
> > +			if (connector->pending_hp)
> > +				drm_kms_helper_connector_hotplug_event(connector);
> 
> So, instead of sending one HPD event, we can now send a set of events in
> a very short time, potentially triggering undesired events in the
> userspace. At least this change of te behaviour must be documented in
> the commit message and in the cover letter.

Will do.

> > +		}
> > +		drm_connector_list_iter_end(&conn_iter);
> 
> Should this still be executed under the mode_config mutex?

Hmmm, can the number of connectors change while we don't hold it here?
I'll dig into this, if the mutex_unlock miraculously jumps down a few
lines in the next revision then you know what the answer is. :)

> > +	}
> >  
> >  	if (first_changed_connector)
> >  		drm_connector_put(first_changed_connector);
> 
> With the code in place, is there a point in keeping a special case for a
> single connector change?

Only as a tiny fast path, but realistically speaking, no. I will get rid
of this, it's only confusing.

> > 
> 
>