Re: [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
From: Sean Paul <sean@poorly.run>
Date: 2021-09-17 15:48:39
Also in:
amd-gfx, dri-devel, intel-gfx, linux-arm-msm, linux-renesas-soc, linux-tegra, lkml, nouveau
On Thu, Sep 16, 2021 at 11:15:49PM +0200, Fernando Ramos wrote:
quoted hunk ↗ jump to hunk
As requested in Documentation/gpu/todo.rst, replace driver calls to drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and DRM_MODESET_LOCK_ALL_END() Signed-off-by: Fernando Ramos <redacted> --- drivers/gpu/drm/i915/display/intel_audio.c | 12 +++-- drivers/gpu/drm/i915/display/intel_display.c | 5 ++- .../drm/i915/display/intel_display_debugfs.c | 35 ++++++++++----- drivers/gpu/drm/i915/display/intel_overlay.c | 45 +++++++++---------- drivers/gpu/drm/i915/display/intel_pipe_crc.c | 5 ++- drivers/gpu/drm/i915/i915_drv.c | 12 +++-- 6 files changed, 67 insertions(+), 47 deletions(-)diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index 532237588511..ab6a5a734b95 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c@@ -1214,7 +1214,9 @@ static int i915_audio_component_bind(struct device *i915_kdev, { struct i915_audio_component *acomp = data; struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev); + struct drm_modeset_acquire_ctx ctx; int i; + int ret;
Please move up with i
quoted hunk ↗ jump to hunk
if (drm_WARN_ON(&dev_priv->drm, acomp->base.ops || acomp->base.dev)) return -EEXIST;@@ -1224,14 +1226,14 @@ static int i915_audio_component_bind(struct device *i915_kdev, DL_FLAG_STATELESS))) return -ENOMEM; - drm_modeset_lock_all(&dev_priv->drm); + DRM_MODESET_LOCK_ALL_BEGIN((&dev_priv->drm), ctx, 0, ret); acomp->base.ops = &i915_audio_component_ops; acomp->base.dev = i915_kdev; BUILD_BUG_ON(MAX_PORTS != I915_MAX_PORTS); for (i = 0; i < ARRAY_SIZE(acomp->aud_sample_rate); i++) acomp->aud_sample_rate[i] = 0; dev_priv->audio_component = acomp; - drm_modeset_unlock_all(&dev_priv->drm); + DRM_MODESET_LOCK_ALL_END((&dev_priv->drm), ctx, ret); return 0;
Return ret here
quoted hunk ↗ jump to hunk
}@@ -1241,12 +1243,14 @@ static void i915_audio_component_unbind(struct device *i915_kdev, { struct i915_audio_component *acomp = data; struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev); + struct drm_modeset_acquire_ctx ctx; + int ret; - drm_modeset_lock_all(&dev_priv->drm); + DRM_MODESET_LOCK_ALL_BEGIN((&dev_priv->drm), ctx, 0, ret); acomp->base.ops = NULL; acomp->base.dev = NULL; dev_priv->audio_component = NULL; - drm_modeset_unlock_all(&dev_priv->drm); + DRM_MODESET_LOCK_ALL_END((&dev_priv->drm), ctx, ret); device_link_remove(hda_kdev, i915_kdev);diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 997a16e85c85..dc2e4d89e5aa 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c@@ -12511,6 +12511,7 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915) int intel_modeset_init_nogem(struct drm_i915_private *i915) { struct drm_device *dev = &i915->drm; + struct drm_modeset_acquire_ctx ctx; enum pipe pipe; struct intel_crtc *crtc; int ret;@@ -12562,9 +12563,9 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915) intel_vga_disable(i915); intel_setup_outputs(i915); - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); for_each_intel_crtc(dev, crtc) { struct intel_initial_plane_config plane_config = {};diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index 8fdacb252bb1..d73af228862e 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c@@ -1057,11 +1057,13 @@ static int i915_display_info(struct seq_file *m, void *unused) struct intel_crtc *crtc; struct drm_connector *connector; struct drm_connector_list_iter conn_iter; + struct drm_modeset_acquire_ctx ctx; intel_wakeref_t wakeref; + int ret; wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); seq_printf(m, "CRTC info\n"); seq_printf(m, "---------\n");@@ -1076,7 +1078,7 @@ static int i915_display_info(struct seq_file *m, void *unused) intel_connector_info(m, connector); drm_connector_list_iter_end(&conn_iter); - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);@@ -1087,9 +1089,11 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) { struct drm_i915_private *dev_priv = node_to_i915(m->private); struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; int i; + int ret;
Please move up with i
quoted hunk ↗ jump to hunk
- drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); seq_printf(m, "PLL refclks: non-SSC: %d kHz, SSC: %d kHz\n", dev_priv->dpll.ref_clks.nssc,@@ -1132,7 +1136,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) seq_printf(m, " mg_pll_tdc_coldst_bias: 0x%08x\n", pll->state.hw_state.mg_pll_tdc_coldst_bias); } - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return 0;
Return ret
quoted hunk ↗ jump to hunk
}@@ -1193,13 +1197,15 @@ static int i915_ddb_info(struct seq_file *m, void *unused) { struct drm_i915_private *dev_priv = node_to_i915(m->private); struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; struct skl_ddb_entry *entry; struct intel_crtc *crtc; + int ret; if (DISPLAY_VER(dev_priv) < 9) return -ENODEV; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); seq_printf(m, "%-15s%8s%8s%8s\n", "", "Start", "End", "Size");@@ -1223,7 +1229,7 @@ static int i915_ddb_info(struct seq_file *m, void *unused) entry->end, skl_ddb_entry_size(entry)); } - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return 0;
Return ret
quoted hunk ↗ jump to hunk
}@@ -1303,10 +1309,12 @@ static int i915_drrs_status(struct seq_file *m, void *unused) { struct drm_i915_private *dev_priv = node_to_i915(m->private); struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; struct intel_crtc *crtc; int active_crtc_cnt = 0; + int ret; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); for_each_intel_crtc(dev, crtc) { if (crtc->base.state->active) { active_crtc_cnt++;@@ -1315,7 +1323,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused) drrs_status_per_crtc(m, dev, crtc); } } - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); if (!active_crtc_cnt) seq_puts(m, "No active crtc found\n");@@ -1607,8 +1615,10 @@ static void wm_latency_show(struct seq_file *m, const u16 wm[8]) { struct drm_i915_private *dev_priv = m->private; struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; int level; int num_levels; + int ret; if (IS_CHERRYVIEW(dev_priv)) num_levels = 3;@@ -1619,7 +1629,7 @@ static void wm_latency_show(struct seq_file *m, const u16 wm[8]) else num_levels = ilk_wm_max_level(dev_priv) + 1; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); for (level = 0; level < num_levels; level++) { unsigned int latency = wm[level];@@ -1640,7 +1650,7 @@ static void wm_latency_show(struct seq_file *m, const u16 wm[8]) level, wm[level], latency / 10, latency % 10); } - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); } static int pri_wm_latency_show(struct seq_file *m, void *data)@@ -1724,6 +1734,7 @@ static ssize_t wm_latency_write(struct file *file, const char __user *ubuf, struct seq_file *m = file->private_data; struct drm_i915_private *dev_priv = m->private; struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; u16 new[8] = { 0 }; int num_levels; int level;@@ -1753,12 +1764,12 @@ static ssize_t wm_latency_write(struct file *file, const char __user *ubuf, if (ret != num_levels) return -EINVAL; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); for (level = 0; level < num_levels; level++) wm[level] = new[level]; - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
Check ret here and return an error if it's != 0
quoted hunk ↗ jump to hunk
return len; }diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c index 7e3f5c6ca484..79c6940807a7 100644 --- a/drivers/gpu/drm/i915/display/intel_overlay.c +++ b/drivers/gpu/drm/i915/display/intel_overlay.c@@ -1104,6 +1104,7 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data, struct drm_crtc *drmmode_crtc; struct intel_crtc *crtc; struct drm_i915_gem_object *new_bo; + struct drm_modeset_acquire_ctx ctx; int ret; overlay = dev_priv->overlay;@@ -1112,24 +1113,24 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data, return -ENODEV; } - if (!(params->flags & I915_OVERLAY_ENABLE)) { - drm_modeset_lock_all(dev); - ret = intel_overlay_switch_off(overlay); - drm_modeset_unlock_all(dev); + if (params->flags & I915_OVERLAY_ENABLE) { - return ret; - } + drmmode_crtc = drm_crtc_find(dev, file_priv, params->crtc_id); + if (!drmmode_crtc) + return -ENOENT; + crtc = to_intel_crtc(drmmode_crtc); - drmmode_crtc = drm_crtc_find(dev, file_priv, params->crtc_id); - if (!drmmode_crtc) - return -ENOENT; - crtc = to_intel_crtc(drmmode_crtc); + new_bo = i915_gem_object_lookup(file_priv, params->bo_handle); + if (!new_bo) + return -ENOENT; + } - new_bo = i915_gem_object_lookup(file_priv, params->bo_handle); - if (!new_bo) - return -ENOENT; + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); - drm_modeset_lock_all(dev); + if (!(params->flags & I915_OVERLAY_ENABLE)) { + ret = intel_overlay_switch_off(overlay); + goto out_unlock; + } if (i915_gem_object_is_tiled(new_bo)) { drm_dbg_kms(&dev_priv->drm,@@ -1194,14 +1195,11 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data, if (ret != 0) goto out_unlock; - drm_modeset_unlock_all(dev); - i915_gem_object_put(new_bo); - - return 0; - out_unlock: - drm_modeset_unlock_all(dev); - i915_gem_object_put(new_bo); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); + + if (params->flags & I915_OVERLAY_ENABLE) + i915_gem_object_put(new_bo);
This function refactor is a bit more involved than the s/drm_modeset_lock_all/DRM_MODESET_LOCK_ALL_*/ changes in the rest of the patch. Could you split it out into a separate patch so it's not hidden away?
quoted hunk ↗ jump to hunk
return ret; }@@ -1263,6 +1261,7 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data, { struct drm_intel_overlay_attrs *attrs = data; struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_modeset_acquire_ctx ctx; struct intel_overlay *overlay; int ret;@@ -1272,7 +1271,7 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data, return -ENODEV; } - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); ret = -EINVAL; if (!(attrs->flags & I915_OVERLAY_UPDATE_ATTRS)) {@@ -1329,7 +1328,7 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data, ret = 0; out_unlock: - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; }diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c b/drivers/gpu/drm/i915/display/intel_pipe_crc.c index 8ac263f471be..e50e514e4897 100644 --- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c@@ -76,6 +76,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_i915_private *dev_priv, enum intel_pipe_crc_source *source) { struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; struct intel_encoder *encoder; struct intel_crtc *crtc; struct intel_digital_port *dig_port;@@ -83,7 +84,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_i915_private *dev_priv, *source = INTEL_PIPE_CRC_SOURCE_PIPE; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); for_each_intel_encoder(dev, encoder) { if (!encoder->base.crtc) continue;@@ -120,7 +121,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_i915_private *dev_priv, break; } } - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; }diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 59fb4c710c8c..7a30e2ff2fed 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c@@ -1009,31 +1009,35 @@ static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) static void intel_suspend_encoders(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; struct intel_encoder *encoder; + int ret; if (!HAS_DISPLAY(dev_priv)) return; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); for_each_intel_encoder(dev, encoder) if (encoder->suspend) encoder->suspend(encoder); - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); } static void intel_shutdown_encoders(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm; + struct drm_modeset_acquire_ctx ctx; struct intel_encoder *encoder; + int ret; if (!HAS_DISPLAY(dev_priv)) return; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); for_each_intel_encoder(dev, encoder) if (encoder->shutdown) encoder->shutdown(encoder); - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); } void i915_driver_shutdown(struct drm_i915_private *i915)-- 2.33.0
-- Sean Paul, Software Engineer, Google / Chromium OS