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
