Re: [PATCH] [RFC] drm: Split connection_mutex out of mode_config.mutex

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 27, 2014 at 5:59 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> After the split-out of crtc locks from the big mode_config.mutex
> there's still two major areas it protects:
> - Various connector probe states, like connector->status, EDID
>   properties, probed mode lists and similar information.
> - The links from connector->encoder and encoder->crtc and other
>   modeset-relevant connector state (e.g. properties which control the
>   panel fitter).
>
> The later is used by modeset operations. But they don't really care
> about the former since it's allowed to e.g. enable a disconnected VGA
> output or with a mode not in the probed list.
>
> Thus far this hasn't been a problem, but for the atomic modeset
> conversion Rob Clark needs to convert all modeset relevant locks into
> w/w locks. This is required because the order of acquisition is
> determined by how userspace supplies the atomic modeset data. This has
> run into troubles in the detect path since the i915 load detect code
> needs _both_ protections offered by the mode_config.mutex: It updates
> probe state and it needs to change the modeset configuration to enable
> the temporary load detect pipe.
>
> The big deal here is that for the probe/detect users of this lock a
> plain mutex fits best, but for atomic modesets we really want a w/w
> mutex. To fix this lets split out a new connection_mutex lock for the
> modeset relevant parts.
>
> For simplicity I've decided to only add one additional lock for all
> connector/encoder links and modeset configuration states. We have
> piles of different modeset objects in addition to those (like bridges
> or panels), so adding per-object locks would be much more effort.
>
> Also, we're guaranteed (at least for now) to do a full modeset if we
> need to acquire this lock. Which means that fine-grained locking is
> fairly irrelevant compared to the amount of time the full modeset will
> take.
>
> I've done a full audit, and there's just a few things that justify
> special focus:
> - Locking in drm_sysfs.c is almost completely absent. We should
>   sprinkle mode_config.connection_mutex over this file a bit, but
>   since it already lacks mode_config.mutex this patch wont make the
>   situation any worse. This is material for a follow-up patch.
>
> - omap has a omap_framebuffer_flush function which walks the
>   connector->encoder->crtc links and is called from many contexts.
>   Some look like they don't acquire mode_config.mutex, so this is
>   already racy. Again fixing this is material for a separate patch.

btw, I guess until someone makes cmd mode dsi panels work properly
with omapdrm, the flush stuff could probably be dropped.  Otherwise it
should learn the ww dance.. mode_config.mutex isn't the right thing
here, the new connection_mutex is what it needs.

otherwise, looks good.. I'll pull it plus ww_mutex stuff (and maybe
few of the less controvercial atomic patches) into an atomic-prep
branch and see what explodes

BR,
-R

> - The radeon hot_plug function to retrain DP links looks at
>   connector->dpms. Currently this happens without any locking, so is
>   already racy. I think radeon_hotplug_work_func should gain
>   mutex_lock/unlock calls for the mode_config.connection_mutex.
>
> - Same applies to i915's intel_dp_hot_plug. But again, this is already
>   racy.
>
> - i915 load_detect code needs to acquire this lock. Which means the
>   w/w dance due to Rob's work will be nicely contained to _just_ this
>   function.
>
> I've added fixme comments everywhere where it looks suspicious but in
> the sysfs code. After a quick irc discussion with Dave Airlie it
> sounds like the lack of locking in there is due to sysfs cleanup fun
> at module unload.
>
> Only compiled tested thus far.
>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Alex Deucher <alexdeucher@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  drivers/gpu/drm/drm_crtc.c                 | 7 +++++++
>  drivers/gpu/drm/drm_crtc_helper.c          | 2 ++
>  drivers/gpu/drm/drm_edid.c                 | 2 ++
>  drivers/gpu/drm/drm_plane_helper.c         | 7 +++++++
>  drivers/gpu/drm/i915/intel_display.c       | 9 ++++++++-
>  drivers/gpu/drm/i915/intel_dp.c            | 1 +
>  drivers/gpu/drm/omapdrm/omap_fb.c          | 2 ++
>  drivers/gpu/drm/radeon/radeon_connectors.c | 1 +
>  include/drm/drm_crtc.h                     | 1 +
>  9 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 34f0bf18d80d..85f15bea88ed 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -54,6 +54,8 @@ void drm_modeset_lock_all(struct drm_device *dev)
>
>         mutex_lock(&dev->mode_config.mutex);
>
> +       mutex_lock(&dev->mode_config.connection_mutex);
> +
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>                 mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
>  }
> @@ -72,6 +74,8 @@ void drm_modeset_unlock_all(struct drm_device *dev)
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>                 mutex_unlock(&crtc->mutex);
>
> +       mutex_unlock(&dev->mode_config.connection_mutex);
> +
>         mutex_unlock(&dev->mode_config.mutex);
>  }
>  EXPORT_SYMBOL(drm_modeset_unlock_all);
> @@ -93,6 +97,7 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev)
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>                 WARN_ON(!mutex_is_locked(&crtc->mutex));
>
> +       WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>  }
>  EXPORT_SYMBOL(drm_warn_on_modeset_not_all_locked);
> @@ -1795,6 +1800,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>         DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>
>         mutex_lock(&dev->mode_config.mutex);
> +       mutex_lock(&dev->mode_config.connection_mutex);
>
>         obj = drm_mode_object_find(dev, out_resp->connector_id,
>                                    DRM_MODE_OBJECT_CONNECTOR);
> @@ -1895,6 +1901,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>         out_resp->count_encoders = encoders_count;
>
>  out:
> +       mutex_unlock(&dev->mode_config.connection_mutex);
>         mutex_unlock(&dev->mode_config.mutex);
>
>         return ret;
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index a8b78e7bde50..422beb5d9c17 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -89,6 +89,8 @@ bool drm_helper_encoder_in_use(struct drm_encoder *encoder)
>         struct drm_device *dev = encoder->dev;
>
>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +       WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
> +
>         list_for_each_entry(connector, &dev->mode_config.connector_list, head)
>                 if (connector->encoder == encoder)
>                         return true;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7a4fd2ed1280..99b9836c8433 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3300,6 +3300,8 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>         struct drm_connector *connector;
>         struct drm_device *dev = encoder->dev;
>
> +       WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +
>         list_for_each_entry(connector, &dev->mode_config.connector_list, head)
>                 if (connector->encoder == encoder && connector->eld[0])
>                         return connector;
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index d966afa7ecae..458d9bf09209 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -54,6 +54,13 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>         struct drm_connector *connector;
>         int count = 0;
>
> +       /*
> +        * Note: Once we change the plane hooks to more fine-grained locking we
> +        * need to grab the connection_mutex here to be able to make these
> +        * checks.
> +        */
> +       WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
> +
>         list_for_each_entry(connector, &dev->mode_config.connector_list, head)
>                 if (connector->encoder && connector->encoder->crtc == crtc) {
>                         if (connector_list != NULL && count < num_connectors)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 883c69a47a99..ea24c602ad6f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8327,6 +8327,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>                       connector->base.id, drm_get_connector_name(connector),
>                       encoder->base.id, drm_get_encoder_name(encoder));
>
> +
> +       mutex_lock(&dev->mode_config.connection_mutex);
>         /*
>          * Algorithm gets a little messy:
>          *
> @@ -8369,7 +8371,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>          */
>         if (!crtc) {
>                 DRM_DEBUG_KMS("no pipe available for load-detect\n");
> -               return false;
> +               goto fail_unlock_connector;
>         }
>
>         mutex_lock(&crtc->mutex);
> @@ -8423,6 +8425,9 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>         else
>                 intel_crtc->new_config = NULL;
>         mutex_unlock(&crtc->mutex);
> +fail_unlock_connector:
> +       mutex_unlock(&dev->mode_config.connection_mutex);
> +
>         return false;
>  }
>
> @@ -8452,6 +8457,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>                 }
>
>                 mutex_unlock(&crtc->mutex);
> +               mutex_unlock(&connector->dev->mode_config.connection_mutex);
>                 return;
>         }
>
> @@ -8460,6 +8466,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>                 connector->funcs->dpms(connector, old->dpms_mode);
>
>         mutex_unlock(&crtc->mutex);
> +       mutex_unlock(&connector->dev->mode_config.connection_mutex);
>  }
>
>  static int i9xx_pll_refclk(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 398d45a6e071..3b888a128a6f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3299,6 +3299,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>         u8 sink_irq_vector;
>         u8 link_status[DP_LINK_STATUS_SIZE];
>
> +       /* FIXME: This access isn't protected by any locks. */
>         if (!intel_encoder->connectors_active)
>                 return;
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 8b019602ffe6..7a02ab6cd3ad 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -346,8 +346,10 @@ void omap_framebuffer_flush(struct drm_framebuffer *fb,
>
>         VERB("flush: %d,%d %dx%d, fb=%p", x, y, w, h, fb);
>
> +       /* FIXME: This is racy - no protection against modeset config changes. */
>         while ((connector = omap_framebuffer_get_next_connector(fb, connector))) {
>                 /* only consider connectors that are part of a chain */
> +
>                 if (connector->encoder && connector->encoder->crtc) {
>                         /* TODO: maybe this should propagate thru the crtc who
>                          * could do the coordinate translation..
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index ea50e0ae7bf7..bcfe5d51ab15 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -48,6 +48,7 @@ void radeon_connector_hotplug(struct drm_connector *connector)
>         radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
>
>         /* if the connector is already off, don't turn it back on */
> +       /* FIXME: This access isn't protected by any locks. */
>         if (connector->dpms != DRM_MODE_DPMS_ON)
>                 return;
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 698d54e27f39..c7a65f309e74 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -723,6 +723,7 @@ struct drm_mode_group {
>   */
>  struct drm_mode_config {
>         struct mutex mutex; /* protects configuration (mode lists etc.) */
> +       struct mutex connection_mutex; /* protects connector->encoder and encoder->crtc links */
>         struct mutex idr_mutex; /* for IDR management */
>         struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
>         /* this is limited to one for now */
> --
> 1.9.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux