drm: Split connection_mutex out of mode_config.mutex (v3)

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.

- 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.

v1: original (only compile tested)

v2: missing mutex_init(), etc (from Rob Clark)

v3: i915 needs more care in the conversion:
- Protect the edp pp logic with the connection_mutex.
- Use connection_mutex in the backlight code due to
  get_pipe_from_connector.
- Use drm_modeset_lock_all in suspend/resume paths.
- Update lock checks in the overlay code.

Cc: Alex Deucher <alexdeucher@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Rob Clark <robdclark@gmail.com>

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5b3e2920c376eda417118882a02b680a1ffa2938..f3b98d4b6f4604247274fffc95da6f5b6f8b926e 100644 (file)
--- 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);
@@ -1793,6 +1798,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);
 
        connector = drm_connector_find(dev, out_resp->connector_id);
        if (!connector) {
@@ -1891,6 +1897,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;
@@ -4640,6 +4647,7 @@ EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
 void drm_mode_config_init(struct drm_device *dev)
 {
        mutex_init(&dev->mode_config.mutex);
+       mutex_init(&dev->mode_config.connection_mutex);
        mutex_init(&dev->mode_config.idr_mutex);
        mutex_init(&dev->mode_config.fb_lock);
        INIT_LIST_HEAD(&dev->mode_config.fb_list);

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 231c9433341fe3732dbb7008cf3900c00941803d..b55d27c872f6cf6d6ea40a6fb8fced50796d6501 100644 (file)
--- 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 085e5550aac501d4dac34749dbe0299d036e1152..7be21781d3bdf82a79973a41cba38fe7abb9b507 100644 (file)
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3304,6 +3304,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 d966afa7ecae66e620cf38a1ec237a6b47b08b68..458d9bf092096b6a3a30fdff777e2fc513fa2745 100644 (file)
--- 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/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 36f4c15f538b9b2a51e4f2997854461eeaf5a26c..169fc2d8c554e85f86adf2cf4f5e0b4b32edc432 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2609,7 +2609,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
 
        *source = INTEL_PIPE_CRC_SOURCE_PIPE;
 
-       mutex_lock(&dev->mode_config.mutex);
+       drm_modeset_lock_all(dev);
        list_for_each_entry(encoder, &dev->mode_config.encoder_list,
                            base.head) {
                if (!encoder->base.crtc)
@@ -2645,7 +2645,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
                        break;
                }
        }
-       mutex_unlock(&dev->mode_config.mutex);
+       drm_modeset_unlock_all(dev);
 
        return ret;
 }

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4619c9e519076828b5449732fa39cea3c363224c..5b5b82c3f57086962bf61d47958bcb0fefb786dd 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -533,13 +533,11 @@ static int i915_drm_freeze(struct drm_device *dev)
                 * Disable CRTCs directly since we want to preserve sw state
                 * for _thaw.
                 */
-               mutex_lock(&dev->mode_config.mutex);
+               drm_modeset_lock_all(dev);
                for_each_crtc(dev, crtc) {
-                       mutex_lock(&crtc->mutex);
                        dev_priv->display.crtc_disable(crtc);
-                       mutex_unlock(&crtc->mutex);
                }
-               mutex_unlock(&dev->mode_config.mutex);
+               drm_modeset_unlock_all(dev);
 
                intel_modeset_suspend_hw(dev);
        }

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e80b2917e3cbd13872cd3842bbc1246b296f4e94..c2976ade823fa1b8e0d3d381e09342c02f90843a 100644 (file)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8323,6 +8323,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
                      connector->base.id, connector->name,
                      encoder->base.id, encoder->name);
 
+       mutex_lock(&dev->mode_config.connection_mutex);
+
        /*
         * Algorithm gets a little messy:
         *
@@ -8365,7 +8367,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);
@@ -8419,6 +8421,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;
 }
 
@@ -8448,6 +8453,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
                }
 
                mutex_unlock(&crtc->mutex);
+               mutex_unlock(&connector->dev->mode_config.connection_mutex);
                return;
        }
 
@@ -8456,6 +8462,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,
@@ -10986,8 +10993,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
 {
        struct drm_encoder *encoder = connector->base.encoder;
+       struct drm_device *dev = connector->base.dev;
 
-       WARN_ON(!mutex_is_locked(&connector->base.dev->mode_config.mutex));
+       WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 
        if (!encoder)
                return INVALID_PIPE;
@@ -11756,9 +11764,9 @@ void intel_modeset_init(struct drm_device *dev)
        /* Just in case the BIOS is doing something questionable. */
        intel_disable_fbc(dev);
 
-       mutex_lock(&dev->mode_config.mutex);
+       drm_modeset_lock_all(dev);
        intel_modeset_setup_hw_state(dev, false);
-       mutex_unlock(&dev->mode_config.mutex);
+       drm_modeset_unlock_all(dev);
 
        for_each_intel_crtc(dev, crtc) {
                if (!crtc->active)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d94f0271e4221d80cc1d8b460ba1b91c8f30d35e..c4d8839212829f9c48bd1f10bc070063c44c6668 100644 (file)
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1154,7 +1154,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
        u32 pp;
        u32 pp_stat_reg, pp_ctrl_reg;
 
-       WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+       WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 
        if (!intel_dp->want_panel_vdd && edp_have_panel_vdd(intel_dp)) {
                struct intel_digital_port *intel_dig_port =
@@ -1191,9 +1191,9 @@ static void edp_panel_vdd_work(struct work_struct *__work)
                                                 struct intel_dp, panel_vdd_work);
        struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
-       mutex_lock(&dev->mode_config.mutex);
+       mutex_lock(&dev->mode_config.connection_mutex);
        edp_panel_vdd_off_sync(intel_dp);
-       mutex_unlock(&dev->mode_config.mutex);
+       mutex_unlock(&dev->mode_config.connection_mutex);
 }
 
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
@@ -3235,6 +3235,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;
 
@@ -3665,9 +3666,9 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
        drm_encoder_cleanup(encoder);
        if (is_edp(intel_dp)) {
                cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-               mutex_lock(&dev->mode_config.mutex);
+               mutex_lock(&dev->mode_config.connection_mutex);
                edp_panel_vdd_off_sync(intel_dp);
-               mutex_unlock(&dev->mode_config.mutex);
+               mutex_unlock(&dev->mode_config.connection_mutex);
        }
        kfree(intel_dig_port);
 }
@@ -4246,9 +4247,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
                drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
                if (is_edp(intel_dp)) {
                        cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-                       mutex_lock(&dev->mode_config.mutex);
+                       mutex_lock(&dev->mode_config.connection_mutex);
                        edp_panel_vdd_off_sync(intel_dp);
-                       mutex_unlock(&dev->mode_config.mutex);
+                       mutex_unlock(&dev->mode_config.connection_mutex);
                }
                drm_sysfs_connector_remove(connector);
                drm_connector_cleanup(connector);

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index acde2945eb8a73e075c2fa60493f4b22e89bb666..b812e9d39f3853c6efeb4c1db0f1bcf6b4a0539e 100644 (file)
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -410,7 +410,7 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
        if (bclp > 255)
                return ASLC_BACKLIGHT_FAILED;
 
-       mutex_lock(&dev->mode_config.mutex);
+       mutex_lock(&dev->mode_config.connection_mutex);
 
        /*
         * Update backlight on all connectors that support backlight (usually
@@ -421,7 +421,7 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
                intel_panel_set_backlight(intel_connector, bclp, 255);
        iowrite32(DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID, &asle->cblv);
 
-       mutex_unlock(&dev->mode_config.mutex);
+       mutex_unlock(&dev->mode_config.connection_mutex);
 
 
        return 0;

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 27eb820d1b19c72e176660fbed14c7746df5752c..e97ea33e01179d4ce521e10dab99bbdbbfd02464 100644 (file)
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -688,7 +688,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
        u32 swidth, swidthsw, sheight, ostride;
 
        BUG_ON(!mutex_is_locked(&dev->struct_mutex));
-       BUG_ON(!mutex_is_locked(&dev->mode_config.mutex));
+       BUG_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
        BUG_ON(!overlay);
 
        ret = intel_overlay_release_old_vid(overlay);
@@ -793,7 +793,7 @@ int intel_overlay_switch_off(struct intel_overlay *overlay)
        int ret;
 
        BUG_ON(!mutex_is_locked(&dev->struct_mutex));
-       BUG_ON(!mutex_is_locked(&dev->mode_config.mutex));
+       BUG_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 
        ret = intel_overlay_recover_from_interrupt(overlay);
        if (ret != 0)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 3fb3f939dca571af624fc2b916a7367c7dbfbbbb..d4d415665475eb74e23d3368d967fe1920387e3b 100644 (file)
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -876,12 +876,12 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
        struct intel_connector *connector = bl_get_data(bd);
        struct drm_device *dev = connector->base.dev;
 
-       mutex_lock(&dev->mode_config.mutex);
+       mutex_lock(&dev->mode_config.connection_mutex);
        DRM_DEBUG_KMS("updating intel_backlight, brightness=%d/%d\n",
                      bd->props.brightness, bd->props.max_brightness);
        intel_panel_set_backlight(connector, bd->props.brightness,
                                  bd->props.max_brightness);
-       mutex_unlock(&dev->mode_config.mutex);
+       mutex_unlock(&dev->mode_config.connection_mutex);
        return 0;
 }
 
@@ -893,9 +893,9 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd)
        int ret;
 
        intel_runtime_pm_get(dev_priv);
-       mutex_lock(&dev->mode_config.mutex);
+       mutex_lock(&dev->mode_config.connection_mutex);
        ret = intel_panel_get_backlight(connector);
-       mutex_unlock(&dev->mode_config.mutex);
+       mutex_unlock(&dev->mode_config.connection_mutex);
        intel_runtime_pm_put(dev_priv);
 
        return ret;

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 8b019602ffe61564dfa979a082e9c6b0afc0a1f2..2a5cacdc344b621fd182d3e8d053bbc0e227ac60 100644 (file)
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -346,6 +346,7 @@ 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) {

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 31d4cf60bae3c4668c8698e69a26b9e58cfeb641..4522f7dce6539b76f712370e83384602d0946959 100644 (file)
--- 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 849cfdccff09fe38925290d2f13cd0baf9b8ecd4..6c295df7b0dffdb976baabca26f38fe1756a35a0 100644 (file)
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -738,6 +738,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 */