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

On Tue, May 27, 2014 at 5:59 PM, Daniel Vetter daniel.vetter@ffwll.ch 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@gmail.com Cc: Alex Deucher alexdeucher@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

---

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