2021-09-17 06:59:21

by Fernando Ramos

[permalink] [raw]
Subject: [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

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 <[email protected]>
---
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;

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;
}
@@ -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;

- 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;
}
@@ -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;
}
@@ -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);

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);

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


2021-09-17 15:50:43

by Sean Paul

[permalink] [raw]
Subject: Re: [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

On Thu, Sep 16, 2021 at 11:15:49PM +0200, Fernando Ramos wrote:
> 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 <[email protected]>
> ---
> 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

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

> }
> @@ -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

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

> }
> @@ -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

> }
> @@ -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

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

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

2021-09-18 05:11:32

by Fernando Ramos

[permalink] [raw]
Subject: Re: [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

> > int i;
> > + int ret;
>
> Please move up with i

Done!


> > + DRM_MODESET_LOCK_ALL_END((&dev_priv->drm), ctx, ret);
> >
> > return 0;
>
> Return ret here

Done!


> > + struct drm_modeset_acquire_ctx ctx;
> > int i;
> > + int ret;
>
> Please move up with i

Done!


> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> >
> > return 0;
>
> Return ret

Done!


> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> >
> > return 0;
>
> Return ret

Done!


> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> >
>
> Check ret here and return an error if it's != 0

Done!


> > @@ -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?

Sure, no problem.