| Message ID | 20211015163336.95188-6-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: add per-connector hotplug events | expand |
On Fri, Oct 15, 2021 at 04:33:45PM +0000, Simon Ser wrote: > If an hotplug event only updates a single connector, use > drm_kms_helper_connector_hotplug_event instead of > drm_kms_helper_hotplug_event. > > Signed-off-by: Simon Ser <contact@emersion.fr> > --- > drivers/gpu/drm/drm_probe_helper.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 3aef3b188c99..6049dc92324b 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -927,7 +927,7 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); > */ > bool drm_helper_hpd_irq_event(struct drm_device *dev) > { > - struct drm_connector *connector; > + struct drm_connector *connector, *changed_connector = NULL; > struct drm_connector_list_iter conn_iter; > bool changed = false; > > @@ -941,15 +941,27 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) > continue; > > - if (check_connector_changed(connector)) > + if (check_connector_changed(connector)) { > + if (changed) { > + if (changed_connector) > + drm_connector_put(changed_connector); > + changed_connector = NULL; > + } else { > + drm_connector_get(connector); > + changed_connector = connector; > + } > + > changed = true; So many things that "changed". Would it not be simpler to just grab the first changed connector always, and count how many there were in total? > + } > } > drm_connector_list_iter_end(&conn_iter); > mutex_unlock(&dev->mode_config.mutex); > > - if (changed) { > + if (changed_connector) { > + drm_kms_helper_connector_hotplug_event(changed_connector); > + drm_connector_put(changed_connector); > + } else if (changed) { > drm_kms_helper_hotplug_event(dev); > - DRM_DEBUG_KMS("Sent hotplug event\n"); > } > > return changed; > -- > 2.33.1 >
Hi Simon, On Fri, Oct 15, 2021 at 04:33:45PM +0000, Simon Ser wrote: > If an hotplug event only updates a single connector, use > drm_kms_helper_connector_hotplug_event instead of > drm_kms_helper_hotplug_event. > > Signed-off-by: Simon Ser <contact@emersion.fr> > --- > drivers/gpu/drm/drm_probe_helper.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 3aef3b188c99..6049dc92324b 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -927,7 +927,7 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); > */ > bool drm_helper_hpd_irq_event(struct drm_device *dev) > { > - struct drm_connector *connector; > + struct drm_connector *connector, *changed_connector = NULL; > struct drm_connector_list_iter conn_iter; > bool changed = false; > > @@ -941,15 +941,27 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) > continue; > > - if (check_connector_changed(connector)) > + if (check_connector_changed(connector)) { > + if (changed) { > + if (changed_connector) > + drm_connector_put(changed_connector); > + changed_connector = NULL; > + } else { > + drm_connector_get(connector); > + changed_connector = connector; > + } This code is a little confusing to read. In case we have only one connector with a change we hit the else part. What we really want to find out is if we have one or more connectors with a change. We could do something like: struct drm_connector *changed_connector = NULL; int changes = 0; ... /* Find if we have one or more changed connectors */ drm_for_each_connector_iter(connector, &conn_iter) { if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue; if (check_connector_changed(connector)) { if (!changes) { changed_connector = connector; drm_connector_get(changed_connector); } changes++; } } drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex); if (changes == 1) drm_kms_helper_connector_hotplug_event(changed_connector); else if (changes > 1) drm_kms_helper_hotplug_event(dev); if (changed_connector) drm_connector_put(changed_connector); Maybe the only reason why I think this is better is bc I wrote it?!? Sam
Hi Simon, On Fri, Oct 15, 2021 at 04:33:45PM +0000, Simon Ser wrote: > If an hotplug event only updates a single connector, use > drm_kms_helper_connector_hotplug_event instead of > drm_kms_helper_hotplug_event. > > Signed-off-by: Simon Ser <contact@emersion.fr> I guess we'd also need to update drm_connector_helper_hpd_irq_event ? Maxime
On Friday, October 15th, 2021 at 21:41, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > So many things that "changed". Would it not be simpler to just grab the > first changed connector always, and count how many there were in total? Indeed, sounds much better. Will do that in the next version.
On Monday, October 18th, 2021 at 10:15, Maxime Ripard <maxime@cerno.tech> wrote:
> I guess we'd also need to update drm_connector_helper_hpd_irq_event ?
Good catch! IIRC this function didn't exist when I first wrote the
patchset.
On Friday, October 15th, 2021 at 22:03, Sam Ravnborg <sam@ravnborg.org> wrote: > This code is a little confusing to read. > > In case we have only one connector with a change we hit the else part. > What we really want to find out is if we have one or more connectors > with a change. > We could do something like: > > struct drm_connector *changed_connector = NULL; > int changes = 0; > > > ... > > /* Find if we have one or more changed connectors */ > drm_for_each_connector_iter(connector, &conn_iter) { > if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) > continue; > > if (check_connector_changed(connector)) { > if (!changes) { > changed_connector = connector; > drm_connector_get(changed_connector); > } > > changes++; > } > } > drm_connector_list_iter_end(&conn_iter); > mutex_unlock(&dev->mode_config.mutex); > > if (changes == 1) > drm_kms_helper_connector_hotplug_event(changed_connector); > else if (changes > 1) > drm_kms_helper_hotplug_event(dev); > > if (changed_connector) > drm_connector_put(changed_connector); > > > Maybe the only reason why I think this is better is bc I wrote it?!? Ah, it's not just you, this version is much better. Thanks for the suggestion, will do that in the next version!
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 3aef3b188c99..6049dc92324b 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -927,7 +927,7 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); */ bool drm_helper_hpd_irq_event(struct drm_device *dev) { - struct drm_connector *connector; + struct drm_connector *connector, *changed_connector = NULL; struct drm_connector_list_iter conn_iter; bool changed = false; @@ -941,15 +941,27 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue; - if (check_connector_changed(connector)) + if (check_connector_changed(connector)) { + if (changed) { + if (changed_connector) + drm_connector_put(changed_connector); + changed_connector = NULL; + } else { + drm_connector_get(connector); + changed_connector = connector; + } + changed = true; + } } drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex); - if (changed) { + if (changed_connector) { + drm_kms_helper_connector_hotplug_event(changed_connector); + drm_connector_put(changed_connector); + } else if (changed) { drm_kms_helper_hotplug_event(dev); - DRM_DEBUG_KMS("Sent hotplug event\n"); } return changed;
If an hotplug event only updates a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event. Signed-off-by: Simon Ser <contact@emersion.fr> --- drivers/gpu/drm/drm_probe_helper.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)