On Wed, Jan 21, 2015 at 9:41 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: > There's a race window (small for hpd, 10s large for polled outputs) > where userspace could sneak in with an unrelated connnector probe > ioctl call and eat the hotplug event (since neither the hpd nor the > poll code see a state change). > > To avoid this, check whether the connector state changes in all other > ->detect calls (in the current helper code that's only probe_single) > and if that's the case, fire off a hotplug event. Note that we can't > directly call the hotplug event handler, since that expects that no > locks are held (due to reentrancy with the fb code to update the kms > console). > > Also, this requires that drivers using the probe_single helper > function set up the poll work. All current drivers do that already, > and with the reworked hpd handling there'll be no downside to > unconditionally setting up the poll work any more. > > v2: Review from Rob Clark > - Don't bail out of the output poll work immediately if it's disabled > to make sure we deliver the delayed hoptplug events. Instead just > jump to the tail. > - Don't scheduel the work when it's not set up. Would be a driver bug > since using probe helpers for anything dynamic without them > initialized makes them all noops. > > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> (v1) > Cc: Rob Clark <robdclark@xxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx> and with the monitor that started all this, plus the addition of: http://lists.freedesktop.org/archives/dri-devel/2015-January/075901.html Tested-by: Rob Clark <robdclark@xxxxxxxxx> > --- > drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++-- > include/drm/drm_crtc.h | 1 + > 2 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 2fbdcca7ca9a..33bf550a1d3f 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect > int count = 0; > int mode_flags = 0; > bool verbose_prune = true; > + enum drm_connector_status old_status; > > WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); > > @@ -121,7 +122,33 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect > if (connector->funcs->force) > connector->funcs->force(connector); > } else { > + old_status = connector->status; > + > connector->status = connector->funcs->detect(connector, true); > + > + /* > + * Normally either the driver's hpd code or the poll loop should > + * pick up any changes and fire the hotplug event. But if > + * userspace sneaks in a probe, we might miss a change. Hence > + * check here, and if anything changed start the hotplug code. > + */ > + if (old_status != connector->status) { > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", > + connector->base.id, > + connector->name, > + old_status, connector->status); > + > + /* > + * 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. > + */ > + dev->mode_config.delayed_event = true; > + if (dev->mode_config.poll_enabled) > + schedule_delayed_work(&dev->mode_config.output_poll_work, > + 0); > + } > } > > /* Re-enable polling in case the global poll config changed. */ > @@ -274,10 +301,14 @@ static void output_poll_execute(struct work_struct *work) > struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work); > struct drm_connector *connector; > enum drm_connector_status old_status; > - bool repoll = false, changed = false; > + bool repoll = false, changed; > + > + /* Pick up any changes detected by the probe functions. */ > + changed = dev->mode_config.delayed_event; > + dev->mode_config.delayed_event = false; > > if (!drm_kms_helper_poll) > - return; > + goto out; > > mutex_lock(&dev->mode_config.mutex); > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > @@ -319,6 +350,7 @@ static void output_poll_execute(struct work_struct *work) > > mutex_unlock(&dev->mode_config.mutex); > > +out: > if (changed) > drm_kms_helper_hotplug_event(dev); > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f444263055c5..65da9fb939a7 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1089,6 +1089,7 @@ struct drm_mode_config { > /* output poll support */ > bool poll_enabled; > bool poll_running; > + bool delayed_event; > struct delayed_work output_poll_work; > > /* pointers to standard properties */ > -- > 2.1.4 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
