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:
> > It was only needed to protect the connector_list walking, see
> >
> > commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> > Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Date: Thu Jul 9 23:44:26 2015 +0200
> >
> > drm/probe-helper: Grab mode_config.mutex in poll_init/enable
> >
> > Unfortunately the commit message of that patch fails to mention that
> > the new locking check was for the connector_list.
> >
> > But that requirement disappeared in
> >
> > commit c36a3254f7857f1ad9badbe3578ccc92be541a8e
> > Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Date: Thu Dec 15 16:58:43 2016 +0100
> >
> > drm: Convert all helpers to drm_connector_list_iter
> >
> > and so we can drop this again.
> >
> > This fixes a locking inversion on nouveau, where the rpm code needs to
> > re-enable. But in other places the rpm_get() calls are nested within
> > the big modeset locks.
> >
> > While at it, also improve the kerneldoc for these two functions a
> > notch.
> >
> > Cc: Dave Airlie <airlied@xxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_probe_helper.c | 41 ++++++++++--------------------------
> > drivers/gpu/drm/i915/intel_hotplug.c | 4 ++--
> > include/drm/drm_crtc_helper.h | 1 -
> > 3 files changed, 13 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 060211ac74a1..7c70f2a52250 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -115,25 +115,24 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> >
> > #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > /**
> > - * drm_kms_helper_poll_enable_locked - re-enable output polling.
> > + * drm_kms_helper_poll_enable - re-enable output polling.
> > * @dev: drm_device
> > *
> > - * This function re-enables the output polling work without
> > - * locking the mode_config mutex.
> > + * This function re-enables the output polling work, after it has been
> > + * temporarily disabled using drm_kms_helper_poll_disable(), for example over
> > + * suspend/resume.
> > *
> > - * This is like drm_kms_helper_poll_enable() however it is to be
> > - * called from a context where the mode_config mutex is locked
> > - * already.
> > + * Drivers can call this helper from their device resume implementation. It is
> > + * an error to call this when the output polling support has not yet been set
> > + * up.
> > */
> > -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?
> if (!mutex_trylock(&dev->mode_config.mutex)) {
>
> The connector list is locked, but the other reads are all racy
> (connector->polled, delayed_event). Those races seem intrinsic and handled
> by e.g. intel_hotplug.c.
>
> Since the locking here was only covering the connector list and that now
> has its own lock,
> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Thanks for the review, I think I'll wait for Dave to supply bugzilla link
or confirmation it solves his lockdep issue before merging.
-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
