On Tue, Jan 10, 2017 at 05:10:50PM +0000, Chris Wilson wrote:
> 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.
I thought I also added that this case is a driver bug to the kernel-docs,
but I failed. Driver needs to ensure this is all ordered reasonably well
(which it is, if you only call it from suspend/resume callbacks). I'll
respin with that fixed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
