On Tue, Jan 10, 2017 at 05:34:08PM +0100, Daniel Vetter wrote:
> On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote:
> > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote:
> > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> > > +void drm_kms_helper_poll_enable(struct drm_device *dev)
> > > {
> > > bool poll = false;
> > > struct drm_connector *connector;
> > > struct drm_connector_list_iter conn_iter;
> > > unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
> > >
> > > - WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > > -
> > > if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > > return;
> >
> > Hmm, output_poll_execute() itself is not checking this correctly,
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 84b75709af90..cb3febc6e719 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work)
> > changed = dev->mode_config.delayed_event;
> > dev->mode_config.delayed_event = false;
> >
> > - if (!drm_kms_helper_poll)
> > + if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > goto out;
>
> The cancel_work_sync in poll_disable should make this impossible, but it
> might be a good thing to check this with a WARN_ON? Care to type that
> patch as a follow up please?
Imagine a drm_kms_helper_poll_disable() run concurrently with
drm_kms_helper_poll_enable(). The enable() gets past the is-enabled? check
and begins iterating the list, meanwhile the disable() cancels the work
and turns off the helper. The enable() is unaware of this and so
reschedules the work, and as output_poll_execute() doesn't check for
dev->mode_config.poll_enabled it stays active.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
