Nouveau - Feb 2018 - [Intel-gfx] [PATCH 2/5] drm: Allow determining if current task is output poll worker

On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote:
> Introduce a helper to determine if the current task is an output poll
> worker.
> 
> This allows us to fix a long-standing deadlock in several DRM drivers
> wherein the ->runtime_suspend callback waits for the output poll worker
> to finish and the worker in turn calls a ->detect callback which waits
> for runtime suspend to finish.  The ->detect callback is invoked from
> multiple call sites and waiting for runtime suspend to finish is the
> correct thing to do except if it's executing in the context of the
> worker.
> 
> Cc: Dave Airlie <airlied at redhat.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Lukas Wunner <lukas at wunner.de>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++
>  include/drm/drm_crtc_helper.h      |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 555fbe54d6e2..019881d15ce1 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct
> *work)
>  		schedule_delayed_work(delayed_work,
> DRM_OUTPUT_POLL_PERIOD);
>  }
>  
> +/**
> + * drm_kms_helper_is_poll_worker - is %current task an output poll worker?
> + *
> + * Determine if %current task is an output poll worker.  This can be used
> + * to select distinct code paths for output polling versus other contexts.
> + */
For this, it would be worth explicitly noting in the comments herethat this
should be called by DRM drivers in order to prevent racing with hotplug
polling workers, so that new drivers in the future can avoid implementing this
race condition in their driver.
> +bool drm_kms_helper_is_poll_worker(void)
> +{
> +	struct work_struct *work = current_work();
> +
> +	return work && work->func == output_poll_execute;
> +}
> +EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
> +
>  /**
>   * drm_kms_helper_poll_disable - disable output polling
>   * @dev: drm_device
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index 76e237bd989b..6914633037a5 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -77,5 +77,6 @@ void drm_kms_helper_hotplug_event(struct drm_device
*dev);
>  
>  void drm_kms_helper_poll_disable(struct drm_device *dev);
>  void drm_kms_helper_poll_enable(struct drm_device *dev);
> +bool drm_kms_helper_is_poll_worker(void);
>  
>  #endif
-- 
Cheers,
	Lyude Paul

Quoting Lyude Paul (2018-02-12 17:46:11)
> On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote:
> > Introduce a helper to determine if the current task is an output poll
> > worker.
> > 
> > This allows us to fix a long-standing deadlock in several DRM drivers
> > wherein the ->runtime_suspend callback waits for the output poll
worker
> > to finish and the worker in turn calls a ->detect callback which
waits
> > for runtime suspend to finish.  The ->detect callback is invoked
from
> > multiple call sites and waiting for runtime suspend to finish is the
> > correct thing to do except if it's executing in the context of the
> > worker.
> > 
> > Cc: Dave Airlie <airlied at redhat.com>
> > Cc: Ben Skeggs <bskeggs at redhat.com>
> > Cc: Alex Deucher <alexander.deucher at amd.com>
> > Signed-off-by: Lukas Wunner <lukas at wunner.de>
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++
> >  include/drm/drm_crtc_helper.h      |  1 +
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 555fbe54d6e2..019881d15ce1 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -653,6 +653,20 @@ static void output_poll_execute(struct
work_struct
> > *work)
> >               schedule_delayed_work(delayed_work,
> > DRM_OUTPUT_POLL_PERIOD);
> >  }
> >  
> > +/**
> > + * drm_kms_helper_is_poll_worker - is %current task an output poll
worker?
> > + *
> > + * Determine if %current task is an output poll worker.  This can be
used
> > + * to select distinct code paths for output polling versus other
contexts.
> > + */
> For this, it would be worth explicitly noting in the comments herethat this
> should be called by DRM drivers in order to prevent racing with hotplug
> polling workers, so that new drivers in the future can avoid implementing
this
> race condition in their driver.
> 
> > +bool drm_kms_helper_is_poll_worker(void)
> > +{
> > +     struct work_struct *work = current_work();
> > +
> > +     return work && work->func == output_poll_execute;
What ensures that work is accessible? Does this need rcu_read_lock
protection or more?
-Chris

On Mon, Feb 12, 2018 at 05:50:12PM +0000, Chris Wilson
wrote:
> Quoting Lyude Paul (2018-02-12 17:46:11)
> > On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote:
> > > Introduce a helper to determine if the current task is an output
poll
> > > worker.
> > > 
> > > This allows us to fix a long-standing deadlock in several DRM
drivers
> > > wherein the ->runtime_suspend callback waits for the output
poll worker
> > > to finish and the worker in turn calls a ->detect callback
which waits
> > > for runtime suspend to finish.  The ->detect callback is
invoked from
> > > multiple call sites and waiting for runtime suspend to finish is
the
> > > correct thing to do except if it's executing in the context
of the
> > > worker.
> > > 
> > > Cc: Dave Airlie <airlied at redhat.com>
> > > Cc: Ben Skeggs <bskeggs at redhat.com>
> > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > Signed-off-by: Lukas Wunner <lukas at wunner.de>
> > > ---
> > >  drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++++
> > >  include/drm/drm_crtc_helper.h      |  1 +
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > b/drivers/gpu/drm/drm_probe_helper.c
> > > index 555fbe54d6e2..019881d15ce1 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -653,6 +653,20 @@ static void output_poll_execute(struct
work_struct
> > > *work)
> > >               schedule_delayed_work(delayed_work,
> > > DRM_OUTPUT_POLL_PERIOD);
> > >  }
> > >  
> > > +/**
> > > + * drm_kms_helper_is_poll_worker - is %current task an output
poll worker?
> > > + *
> > > + * Determine if %current task is an output poll worker.  This
can be used
> > > + * to select distinct code paths for output polling versus other
contexts.
> > > + */
> > > +bool drm_kms_helper_is_poll_worker(void)
> > > +{
> > > +     struct work_struct *work = current_work();
> > > +
> > > +     return work && work->func ==
output_poll_execute;
> 
> What ensures that work is accessible? Does this need rcu_read_lock
> protection or more?
The work_struct exists as long this kthread is working on it.
Since this is called by the kthread itself, it is guaranteed to exist.
So there's no protection needed.

Thanks,

Lukas

On Mon, Feb 12, 2018 at 12:46:11PM -0500, Lyude Paul
wrote:
> On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote:
> > Introduce a helper to determine if the current task is an output poll
> > worker.
> > 
> > This allows us to fix a long-standing deadlock in several DRM drivers
> > wherein the ->runtime_suspend callback waits for the output poll
worker
> > to finish and the worker in turn calls a ->detect callback which
waits
> > for runtime suspend to finish.  The ->detect callback is invoked
from
> > multiple call sites and waiting for runtime suspend to finish is the
> > correct thing to do except if it's executing in the context of the
> > worker.
[snip]
> > +/**
> > + * drm_kms_helper_is_poll_worker - is %current task an output poll
worker?
> > + *
> > + * Determine if %current task is an output poll worker.  This can be
used
> > + * to select distinct code paths for output polling versus other
contexts.
> > + */
>
> For this, it would be worth explicitly noting in the comments herethat this
> should be called by DRM drivers in order to prevent racing with hotplug
> polling workers, so that new drivers in the future can avoid implementing
this
> race condition in their driver.
Good point, I've just sent out a v2 to address your comment.  Let me know
if this isn't what you had in mind.  It may also be worth to expand the
DOC section at the top of drm_probe_helper.c to explain the interaction
between polling and runtime suspend in more detail, but I think this is
better done in a separate patch to keep the present patch small and thus
easily backportable to stable.

Thanks a lot for the review,

Lukas