Hi all,
One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to
"use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this
patch series is about.
You will find two types of changes here:
- Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with
"DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has
already been done in previous commits such as b7ea04d2)
- Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
in the remaining places (as it has already been done in previous commits
such as 57037094)
Most of the changes are straight forward, except for a few cases in the "amd"
and "i915" drivers where some extra dancing was needed to overcome the
limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used
once inside the same function (the reason being that the macro expansion
includes *labels*, and you can not have two labels named the same inside one
function)
Notice that, even after this patch series, some places remain where
"drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present,
all inside drm core (which makes sense), except for two (in "amd" and "i915")
which cannot be replaced due to the way they are being used.
Fernando Ramos (15):
dmr: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
dmr/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
dmr/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
Documentation/gpu/todo.rst | 17 -------
Documentation/locking/ww-mutex-design.rst | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 13 +++--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++----------
.../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 23 +++++----
drivers/gpu/drm/drm_client_modeset.c | 14 +++---
drivers/gpu/drm/drm_crtc_helper.c | 18 ++++---
drivers/gpu/drm/drm_fb_helper.c | 10 ++--
drivers/gpu/drm/drm_framebuffer.c | 6 ++-
drivers/gpu/drm/gma500/psb_device.c | 14 ++++--
drivers/gpu/drm/i915/display/intel_audio.c | 12 +++--
drivers/gpu/drm/i915/display/intel_display.c | 22 +++-----
.../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 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++-
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 10 ++--
drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
drivers/gpu/drm/omapdrm/omap_fb.c | 6 ++-
drivers/gpu/drm/radeon/radeon_device.c | 13 +++--
drivers/gpu/drm/radeon/radeon_dp_mst.c | 7 ++-
drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 ++-
drivers/gpu/drm/tegra/dsi.c | 6 ++-
drivers/gpu/drm/tegra/hdmi.c | 5 +-
drivers/gpu/drm/tegra/sor.c | 10 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 +++--
28 files changed, 222 insertions(+), 180 deletions(-)
--
2.33.0
As requested in Documentation/gpu/todo.rst, replace the boilerplate code
surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
and DRM_MODESET_LOCK_ALL_END()
Signed-off-by: Fernando Ramos <[email protected]>
---
drivers/gpu/drm/drm_client_modeset.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index ced09c7c06f9..5f5184f071ed 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -574,6 +574,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
int num_connectors_detected = 0;
int num_tiled_conns = 0;
struct drm_modeset_acquire_ctx ctx;
+ int err;
if (!drm_drv_uses_atomic_modeset(dev))
return false;
@@ -585,10 +586,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
if (!save_enabled)
return false;
- drm_modeset_acquire_init(&ctx, 0);
-
- while (drm_modeset_lock_all_ctx(dev, &ctx) != 0)
- drm_modeset_backoff(&ctx);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
memcpy(save_enabled, enabled, count);
mask = GENMASK(count - 1, 0);
@@ -743,8 +741,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
ret = false;
}
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
kfree(save_enabled);
return ret;
--
2.33.0
As requested in Documentation/gpu/todo.rst, replace the boilerplate code
surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
and DRM_MODESET_LOCK_ALL_END()
Signed-off-by: Fernando Ramos <[email protected]>
---
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index cabe15190ec1..c83db90b0e02 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -99,20 +99,18 @@ static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state)
{
struct drm_device *ddev;
struct drm_modeset_acquire_ctx ctx;
+ int ret;
disp_state->timestamp = ktime_get();
ddev = disp_state->drm_dev;
- drm_modeset_acquire_init(&ctx, 0);
-
- while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
- drm_modeset_backoff(&ctx);
+ DRM_MODESET_LOCK_ALL_BEGIN(ddev, ctx, 0, ret);
disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
&ctx);
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
+
+ DRM_MODESET_LOCK_ALL_END(ddev, ctx, ret);
}
void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
--
2.33.0
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/vmwgfx/vmwgfx_ioctl.c | 11 +++++++----
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++----
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
index 28af34ab6ed6..7df35c6f1458 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
@@ -28,6 +28,7 @@
#include "vmwgfx_drv.h"
#include "vmwgfx_devcaps.h"
#include <drm/vmwgfx_drm.h>
+#include <drm/drm_drv.h>
#include "vmwgfx_kms.h"
int vmw_getparam_ioctl(struct drm_device *dev, void *data,
@@ -172,6 +173,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
struct drm_vmw_rect __user *clips_ptr;
struct drm_vmw_rect *clips = NULL;
struct drm_framebuffer *fb;
+ struct drm_modeset_acquire_ctx ctx;
struct vmw_framebuffer *vfb;
struct vmw_resource *res;
uint32_t num_clips;
@@ -203,7 +205,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
goto out_no_copy;
}
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
if (!fb) {
@@ -231,7 +233,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
out_no_surface:
drm_framebuffer_put(fb);
out_no_fb:
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
out_no_copy:
kfree(clips);
out_clips:
@@ -250,6 +252,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
struct drm_vmw_rect __user *clips_ptr;
struct drm_vmw_rect *clips = NULL;
struct drm_framebuffer *fb;
+ struct drm_modeset_acquire_ctx ctx;
struct vmw_framebuffer *vfb;
uint32_t num_clips;
int ret;
@@ -280,7 +283,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
goto out_no_copy;
}
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
if (!fb) {
@@ -303,7 +306,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
out_no_ttm_lock:
drm_framebuffer_put(fb);
out_no_fb:
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
out_no_copy:
kfree(clips);
out_clips:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 74fa41909213..268095cb8c84 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -33,6 +33,7 @@
#include <drm/drm_rect.h>
#include <drm/drm_sysfs.h>
#include <drm/drm_vblank.h>
+#include <drm/drm_drv.h>
#include "vmwgfx_kms.h"
@@ -243,15 +244,17 @@ void vmw_kms_legacy_hotspot_clear(struct vmw_private *dev_priv)
struct drm_device *dev = &dev_priv->drm;
struct vmw_display_unit *du;
struct drm_crtc *crtc;
+ struct drm_modeset_acquire_ctx ctx;
+ int ret;
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
drm_for_each_crtc(crtc, dev) {
du = vmw_crtc_to_du(crtc);
du->hotspot_x = 0;
du->hotspot_y = 0;
}
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
}
void vmw_kms_cursor_post_execbuf(struct vmw_private *dev_priv)
@@ -1012,9 +1015,10 @@ static int vmw_framebuffer_bo_dirty(struct drm_framebuffer *framebuffer,
struct vmw_framebuffer_bo *vfbd =
vmw_framebuffer_to_vfbd(framebuffer);
struct drm_clip_rect norect;
+ struct drm_modeset_acquire_ctx ctx;
int ret, increment = 1;
- drm_modeset_lock_all(&dev_priv->drm);
+ DRM_MODESET_LOCK_ALL_BEGIN((&dev_priv->drm), ctx, 0, ret);
if (!num_clips) {
num_clips = 1;
@@ -1040,7 +1044,7 @@ static int vmw_framebuffer_bo_dirty(struct drm_framebuffer *framebuffer,
vmw_cmd_flush(dev_priv, false);
- drm_modeset_unlock_all(&dev_priv->drm);
+ DRM_MODESET_LOCK_ALL_END((&dev_priv->drm), ctx, ret);
return ret;
}
--
2.33.0
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/tegra/dsi.c | 6 ++++--
drivers/gpu/drm/tegra/hdmi.c | 5 +++--
drivers/gpu/drm/tegra/sor.c | 10 ++++++----
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f46d377f0c30..77a496f6a2e9 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -202,10 +202,12 @@ static int tegra_dsi_show_regs(struct seq_file *s, void *data)
struct tegra_dsi *dsi = node->info_ent->data;
struct drm_crtc *crtc = dsi->output.encoder.crtc;
struct drm_device *drm = node->minor->dev;
+ struct drm_modeset_acquire_ctx ctx;
unsigned int i;
int err = 0;
+ int ret;
- drm_modeset_lock_all(drm);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, ret);
if (!crtc || !crtc->state->active) {
err = -EBUSY;
@@ -220,7 +222,7 @@ static int tegra_dsi_show_regs(struct seq_file *s, void *data)
}
unlock:
- drm_modeset_unlock_all(drm);
+ DRM_MODESET_LOCK_ALL_END(drm, ctx, ret);
return err;
}
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index e5d2a4026028..669a2ebb55ae 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1031,10 +1031,11 @@ static int tegra_hdmi_show_regs(struct seq_file *s, void *data)
struct tegra_hdmi *hdmi = node->info_ent->data;
struct drm_crtc *crtc = hdmi->output.encoder.crtc;
struct drm_device *drm = node->minor->dev;
+ struct drm_modeset_acquire_ctx ctx;
unsigned int i;
int err = 0;
- drm_modeset_lock_all(drm);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
if (!crtc || !crtc->state->active) {
err = -EBUSY;
@@ -1049,7 +1050,7 @@ static int tegra_hdmi_show_regs(struct seq_file *s, void *data)
}
unlock:
- drm_modeset_unlock_all(drm);
+ DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
return err;
}
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 0ea320c1092b..323d95eb0cac 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1490,10 +1490,11 @@ static int tegra_sor_show_crc(struct seq_file *s, void *data)
struct tegra_sor *sor = node->info_ent->data;
struct drm_crtc *crtc = sor->output.encoder.crtc;
struct drm_device *drm = node->minor->dev;
+ struct drm_modeset_acquire_ctx ctx;
int err = 0;
u32 value;
- drm_modeset_lock_all(drm);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
if (!crtc || !crtc->state->active) {
err = -EBUSY;
@@ -1522,7 +1523,7 @@ static int tegra_sor_show_crc(struct seq_file *s, void *data)
seq_printf(s, "%08x\n", value);
unlock:
- drm_modeset_unlock_all(drm);
+ DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
return err;
}
@@ -1652,10 +1653,11 @@ static int tegra_sor_show_regs(struct seq_file *s, void *data)
struct tegra_sor *sor = node->info_ent->data;
struct drm_crtc *crtc = sor->output.encoder.crtc;
struct drm_device *drm = node->minor->dev;
+ struct drm_modeset_acquire_ctx ctx;
unsigned int i;
int err = 0;
- drm_modeset_lock_all(drm);
+ DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
if (!crtc || !crtc->state->active) {
err = -EBUSY;
@@ -1670,7 +1672,7 @@ static int tegra_sor_show_regs(struct seq_file *s, void *data)
}
unlock:
- drm_modeset_unlock_all(drm);
+ DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
return err;
}
--
2.33.0
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/gma500/psb_device.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/gma500/psb_device.c b/drivers/gpu/drm/gma500/psb_device.c
index 951725a0f7a3..4e27f65a1f16 100644
--- a/drivers/gpu/drm/gma500/psb_device.c
+++ b/drivers/gpu/drm/gma500/psb_device.c
@@ -8,6 +8,7 @@
#include <linux/backlight.h>
#include <drm/drm.h>
+#include <drm/drm_drv.h>
#include "gma_device.h"
#include "intel_bios.h"
@@ -169,8 +170,10 @@ static int psb_save_display_registers(struct drm_device *dev)
{
struct drm_psb_private *dev_priv = dev->dev_private;
struct drm_crtc *crtc;
+ struct drm_modeset_acquire_ctx ctx;
struct gma_connector *connector;
struct psb_state *regs = &dev_priv->regs.psb;
+ int ret;
/* Display arbitration control + watermarks */
regs->saveDSPARB = PSB_RVDC32(DSPARB);
@@ -183,7 +186,7 @@ static int psb_save_display_registers(struct drm_device *dev)
regs->saveCHICKENBIT = PSB_RVDC32(DSPCHICKENBIT);
/* Save crtc and output state */
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
if (drm_helper_crtc_in_use(crtc))
dev_priv->ops->save_crtc(crtc);
@@ -193,7 +196,8 @@ static int psb_save_display_registers(struct drm_device *dev)
if (connector->save)
connector->save(&connector->base);
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
+
return 0;
}
@@ -207,8 +211,10 @@ static int psb_restore_display_registers(struct drm_device *dev)
{
struct drm_psb_private *dev_priv = dev->dev_private;
struct drm_crtc *crtc;
+ struct drm_modeset_acquire_ctx ctx;
struct gma_connector *connector;
struct psb_state *regs = &dev_priv->regs.psb;
+ int ret;
/* Display arbitration + watermarks */
PSB_WVDC32(regs->saveDSPARB, DSPARB);
@@ -223,7 +229,7 @@ static int psb_restore_display_registers(struct drm_device *dev)
/*make sure VGA plane is off. it initializes to on after reset!*/
PSB_WVDC32(0x80000000, VGACNTRL);
- drm_modeset_lock_all(dev);
+ DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
if (drm_helper_crtc_in_use(crtc))
dev_priv->ops->restore_crtc(crtc);
@@ -232,7 +238,7 @@ static int psb_restore_display_registers(struct drm_device *dev)
if (connector->restore)
connector->restore(&connector->base);
- drm_modeset_unlock_all(dev);
+ DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
return 0;
}
--
2.33.0
On Thu, Sep 16, 2021 at 11:15:40PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
>
With the subject fixed (s/dmr/drm/),
Reviewed-by: Sean Paul <[email protected]>
> Signed-off-by: Fernando Ramos <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> index cabe15190ec1..c83db90b0e02 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> @@ -99,20 +99,18 @@ static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state)
> {
> struct drm_device *ddev;
> struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> disp_state->timestamp = ktime_get();
>
> ddev = disp_state->drm_dev;
>
> - drm_modeset_acquire_init(&ctx, 0);
> -
> - while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
> - drm_modeset_backoff(&ctx);
> + DRM_MODESET_LOCK_ALL_BEGIN(ddev, ctx, 0, ret);
>
> disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
> &ctx);
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
> +
> + DRM_MODESET_LOCK_ALL_END(ddev, ctx, ret);
> }
>
> void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Sep 16, 2021 at 11:15:42PM +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()
>
Reviewed-by: Sean Paul <[email protected]>
> Signed-off-by: Fernando Ramos <[email protected]>
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 +++++++----
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++----
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> index 28af34ab6ed6..7df35c6f1458 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> @@ -28,6 +28,7 @@
> #include "vmwgfx_drv.h"
> #include "vmwgfx_devcaps.h"
> #include <drm/vmwgfx_drm.h>
> +#include <drm/drm_drv.h>
> #include "vmwgfx_kms.h"
>
> int vmw_getparam_ioctl(struct drm_device *dev, void *data,
> @@ -172,6 +173,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
> struct drm_vmw_rect __user *clips_ptr;
> struct drm_vmw_rect *clips = NULL;
> struct drm_framebuffer *fb;
> + struct drm_modeset_acquire_ctx ctx;
> struct vmw_framebuffer *vfb;
> struct vmw_resource *res;
> uint32_t num_clips;
> @@ -203,7 +205,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
> goto out_no_copy;
> }
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
> if (!fb) {
> @@ -231,7 +233,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
> out_no_surface:
> drm_framebuffer_put(fb);
> out_no_fb:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> out_no_copy:
> kfree(clips);
> out_clips:
> @@ -250,6 +252,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
> struct drm_vmw_rect __user *clips_ptr;
> struct drm_vmw_rect *clips = NULL;
> struct drm_framebuffer *fb;
> + struct drm_modeset_acquire_ctx ctx;
> struct vmw_framebuffer *vfb;
> uint32_t num_clips;
> int ret;
> @@ -280,7 +283,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
> goto out_no_copy;
> }
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
> if (!fb) {
> @@ -303,7 +306,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
> out_no_ttm_lock:
> drm_framebuffer_put(fb);
> out_no_fb:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> out_no_copy:
> kfree(clips);
> out_clips:
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 74fa41909213..268095cb8c84 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -33,6 +33,7 @@
> #include <drm/drm_rect.h>
> #include <drm/drm_sysfs.h>
> #include <drm/drm_vblank.h>
> +#include <drm/drm_drv.h>
>
> #include "vmwgfx_kms.h"
>
> @@ -243,15 +244,17 @@ void vmw_kms_legacy_hotspot_clear(struct vmw_private *dev_priv)
> struct drm_device *dev = &dev_priv->drm;
> struct vmw_display_unit *du;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> drm_for_each_crtc(crtc, dev) {
> du = vmw_crtc_to_du(crtc);
>
> du->hotspot_x = 0;
> du->hotspot_y = 0;
> }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> }
>
> void vmw_kms_cursor_post_execbuf(struct vmw_private *dev_priv)
> @@ -1012,9 +1015,10 @@ static int vmw_framebuffer_bo_dirty(struct drm_framebuffer *framebuffer,
> struct vmw_framebuffer_bo *vfbd =
> vmw_framebuffer_to_vfbd(framebuffer);
> struct drm_clip_rect norect;
> + struct drm_modeset_acquire_ctx ctx;
> int ret, increment = 1;
>
> - drm_modeset_lock_all(&dev_priv->drm);
> + DRM_MODESET_LOCK_ALL_BEGIN((&dev_priv->drm), ctx, 0, ret);
>
> if (!num_clips) {
> num_clips = 1;
> @@ -1040,7 +1044,7 @@ static int vmw_framebuffer_bo_dirty(struct drm_framebuffer *framebuffer,
>
> vmw_cmd_flush(dev_priv, false);
>
> - drm_modeset_unlock_all(&dev_priv->drm);
> + DRM_MODESET_LOCK_ALL_END((&dev_priv->drm), ctx, ret);
>
> return ret;
> }
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Sep 16, 2021 at 11:15:43PM +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/tegra/dsi.c | 6 ++++--
> drivers/gpu/drm/tegra/hdmi.c | 5 +++--
> drivers/gpu/drm/tegra/sor.c | 10 ++++++----
> 3 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f46d377f0c30..77a496f6a2e9 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -202,10 +202,12 @@ static int tegra_dsi_show_regs(struct seq_file *s, void *data)
> struct tegra_dsi *dsi = node->info_ent->data;
> struct drm_crtc *crtc = dsi->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> unsigned int i;
> int err = 0;
> + int ret;
You can use err here instead. With that fixed,
Reviewed-by: Sean Paul <[email protected]>
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, ret);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -220,7 +222,7 @@ static int tegra_dsi_show_regs(struct seq_file *s, void *data)
> }
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, ret);
> return err;
> }
>
> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index e5d2a4026028..669a2ebb55ae 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -1031,10 +1031,11 @@ static int tegra_hdmi_show_regs(struct seq_file *s, void *data)
> struct tegra_hdmi *hdmi = node->info_ent->data;
> struct drm_crtc *crtc = hdmi->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> unsigned int i;
> int err = 0;
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -1049,7 +1050,7 @@ static int tegra_hdmi_show_regs(struct seq_file *s, void *data)
> }
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
> return err;
> }
>
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 0ea320c1092b..323d95eb0cac 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -1490,10 +1490,11 @@ static int tegra_sor_show_crc(struct seq_file *s, void *data)
> struct tegra_sor *sor = node->info_ent->data;
> struct drm_crtc *crtc = sor->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> int err = 0;
> u32 value;
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -1522,7 +1523,7 @@ static int tegra_sor_show_crc(struct seq_file *s, void *data)
> seq_printf(s, "%08x\n", value);
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
> return err;
> }
>
> @@ -1652,10 +1653,11 @@ static int tegra_sor_show_regs(struct seq_file *s, void *data)
> struct tegra_sor *sor = node->info_ent->data;
> struct drm_crtc *crtc = sor->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> unsigned int i;
> int err = 0;
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -1670,7 +1672,7 @@ static int tegra_sor_show_regs(struct seq_file *s, void *data)
> }
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
> return err;
> }
>
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Sep 16, 2021 at 11:15:50PM +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/gma500/psb_device.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_device.c b/drivers/gpu/drm/gma500/psb_device.c
> index 951725a0f7a3..4e27f65a1f16 100644
> --- a/drivers/gpu/drm/gma500/psb_device.c
> +++ b/drivers/gpu/drm/gma500/psb_device.c
> @@ -8,6 +8,7 @@
> #include <linux/backlight.h>
>
> #include <drm/drm.h>
> +#include <drm/drm_drv.h>
>
> #include "gma_device.h"
> #include "intel_bios.h"
> @@ -169,8 +170,10 @@ static int psb_save_display_registers(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> struct gma_connector *connector;
> struct psb_state *regs = &dev_priv->regs.psb;
> + int ret;
>
> /* Display arbitration control + watermarks */
> regs->saveDSPARB = PSB_RVDC32(DSPARB);
> @@ -183,7 +186,7 @@ static int psb_save_display_registers(struct drm_device *dev)
> regs->saveCHICKENBIT = PSB_RVDC32(DSPCHICKENBIT);
>
> /* Save crtc and output state */
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> if (drm_helper_crtc_in_use(crtc))
> dev_priv->ops->save_crtc(crtc);
> @@ -193,7 +196,8 @@ static int psb_save_display_registers(struct drm_device *dev)
> if (connector->save)
> connector->save(&connector->base);
>
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> +
> return 0;
Return ret here please
> }
>
> @@ -207,8 +211,10 @@ static int psb_restore_display_registers(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> struct gma_connector *connector;
> struct psb_state *regs = &dev_priv->regs.psb;
> + int ret;
>
> /* Display arbitration + watermarks */
> PSB_WVDC32(regs->saveDSPARB, DSPARB);
> @@ -223,7 +229,7 @@ static int psb_restore_display_registers(struct drm_device *dev)
> /*make sure VGA plane is off. it initializes to on after reset!*/
> PSB_WVDC32(0x80000000, VGACNTRL);
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> if (drm_helper_crtc_in_use(crtc))
> dev_priv->ops->restore_crtc(crtc);
> @@ -232,7 +238,7 @@ static int psb_restore_display_registers(struct drm_device *dev)
> if (connector->restore)
> connector->restore(&connector->base);
>
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return 0;
Here too
> }
>
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Sep 16, 2021 at 11:15:37PM +0200, Fernando Ramos wrote:
> Hi all,
>
> One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to
> "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this
> patch series is about.
>
> You will find two types of changes here:
>
> - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with
> "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has
> already been done in previous commits such as b7ea04d2)
>
> - Replacing "drm_modeset_lock_all()" with "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> in the remaining places (as it has already been done in previous commits
> such as 57037094)
>
> Most of the changes are straight forward, except for a few cases in the "amd"
> and "i915" drivers where some extra dancing was needed to overcome the
> limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used
> once inside the same function (the reason being that the macro expansion
> includes *labels*, and you can not have two labels named the same inside one
> function)
>
> Notice that, even after this patch series, some places remain where
> "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present,
> all inside drm core (which makes sense), except for two (in "amd" and "i915")
> which cannot be replaced due to the way they are being used.
Can we at least replace those with drm_modeset_lock_all_ctx and delete
drm_modeset_lock_all? That would be really nice goal to make sure these
don't spread further.
Otherwise great stuff, I'm trying to volunteer a few reviewers.
-Daniel
>
> Fernando Ramos (15):
> dmr: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> dmr/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> dmr/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
>
> Documentation/gpu/todo.rst | 17 -------
> Documentation/locking/ww-mutex-design.rst | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 13 +++--
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++----------
> .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 23 +++++----
> drivers/gpu/drm/drm_client_modeset.c | 14 +++---
> drivers/gpu/drm/drm_crtc_helper.c | 18 ++++---
> drivers/gpu/drm/drm_fb_helper.c | 10 ++--
> drivers/gpu/drm/drm_framebuffer.c | 6 ++-
> drivers/gpu/drm/gma500/psb_device.c | 14 ++++--
> drivers/gpu/drm/i915/display/intel_audio.c | 12 +++--
> drivers/gpu/drm/i915/display/intel_display.c | 22 +++-----
> .../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 +++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++-
> .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 10 ++--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
> drivers/gpu/drm/omapdrm/omap_fb.c | 6 ++-
> drivers/gpu/drm/radeon/radeon_device.c | 13 +++--
> drivers/gpu/drm/radeon/radeon_dp_mst.c | 7 ++-
> drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 ++-
> drivers/gpu/drm/tegra/dsi.c | 6 ++-
> drivers/gpu/drm/tegra/hdmi.c | 5 +-
> drivers/gpu/drm/tegra/sor.c | 10 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 +++--
> 28 files changed, 222 insertions(+), 180 deletions(-)
>
> --
> 2.33.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Sep 16, 2021 at 11:15:38PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
>
Hi Fernando,
Thank you for your patch. Could you please fix the subject, changing dmr to drm?
> Signed-off-by: Fernando Ramos <[email protected]>
> ---
> drivers/gpu/drm/drm_client_modeset.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index ced09c7c06f9..5f5184f071ed 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -574,6 +574,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> int num_connectors_detected = 0;
> int num_tiled_conns = 0;
> struct drm_modeset_acquire_ctx ctx;
> + int err;
I think you can just reuse 'ret' instead of creating a new variable. That
ensures if the lock fails we return the error from the macros.
Sean
>
> if (!drm_drv_uses_atomic_modeset(dev))
> return false;
> @@ -585,10 +586,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> if (!save_enabled)
> return false;
>
> - drm_modeset_acquire_init(&ctx, 0);
> -
> - while (drm_modeset_lock_all_ctx(dev, &ctx) != 0)
> - drm_modeset_backoff(&ctx);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
>
> memcpy(save_enabled, enabled, count);
> mask = GENMASK(count - 1, 0);
> @@ -743,8 +741,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> ret = false;
> }
>
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
>
> kfree(save_enabled);
> return ret;
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On 21/09/17 05:24PM, Daniel Vetter wrote:
>
> Can we at least replace those with drm_modeset_lock_all_ctx and delete
> drm_modeset_lock_all? That would be really nice goal to make sure these
> don't spread further.
I just checked and turns out no one else is using "drm_modeset_lock_all()"
anymore.
The only reference is the definition of the function itself, which I did not
remove because it was being EXPORT_SYMBOL'ed and I was not sure whether it could
be removed or not (to prevent breaking third party modules maybe?)
The same goes true for its sibling "dmr_modeset_unlock_all()".
But if you give me the green light I'll remove both of them right away :)
>
> Could you please fix the subject, changing dmr to drm?
>
Ups! Sure, I'll fix that. Thanks for noticing.
>
> I think you can just reuse 'ret' instead of creating a new variable. That
> ensures if the lock fails we return the error from the macros.
>
I didn't reuse "ret" because otherwise I would have had to change the prototype
of the function (which currently returns a "bool" instead of an "int").
However I could, for example, check for any error and convert that into "false".
Would that be ok?
> > int err = 0;
> > + int ret;
>
> You can use err here instead.
Done!
Hi Fernando,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-exynos/exynos-drm-next]
[also build test ERROR on tegra-drm/drm/tegra/for-next linus/master v5.15-rc2 next-20210917]
[cannot apply to drm-intel/for-linux-next tegra/for-next drm-tip/drm-tip airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Fernando-Ramos/drm-cleanup-Use-DRM_MODESET_LOCK_ALL_-helpers-where-possible/20210917-051842
base: https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/0da6630f04d8f86f9d3c9a65fe61a6c6d182c87f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Fernando-Ramos/drm-cleanup-Use-DRM_MODESET_LOCK_ALL_-helpers-where-possible/20210917-051842
git checkout 0da6630f04d8f86f9d3c9a65fe61a6c6d182c87f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from include/drm/drm_crtc.h:36,
from include/drm/drm_atomic_helper.h:31,
from drivers/gpu/drm/msm/disp/msm_disp_snapshot.h:9,
from drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c:8:
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c: In function 'msm_disp_capture_atomic_state':
>> include/drm/drm_modeset_lock.h:167:14: error: implicit declaration of function 'drm_drv_uses_atomic_modeset' [-Werror=implicit-function-declaration]
167 | if (!drm_drv_uses_atomic_modeset(dev)) \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c:108:9: note: in expansion of macro 'DRM_MODESET_LOCK_ALL_BEGIN'
108 | DRM_MODESET_LOCK_ALL_BEGIN(ddev, ctx, 0, ret);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/drm_drv_uses_atomic_modeset +167 include/drm/drm_modeset_lock.h
a6a8bb848d5ca4 Daniel Vetter 2014-07-25 138
06eaae46381737 Thierry Reding 2015-12-02 139 int drm_modeset_lock_all_ctx(struct drm_device *dev,
51fd371bbaf940 Rob Clark 2013-11-19 140 struct drm_modeset_acquire_ctx *ctx);
51fd371bbaf940 Rob Clark 2013-11-19 141
b7ea04d299c78b Sean Paul 2018-11-29 142 /**
b7ea04d299c78b Sean Paul 2018-11-29 143 * DRM_MODESET_LOCK_ALL_BEGIN - Helper to acquire modeset locks
b7ea04d299c78b Sean Paul 2018-11-29 144 * @dev: drm device
b7ea04d299c78b Sean Paul 2018-11-29 145 * @ctx: local modeset acquire context, will be dereferenced
b7ea04d299c78b Sean Paul 2018-11-29 146 * @flags: DRM_MODESET_ACQUIRE_* flags to pass to drm_modeset_acquire_init()
b7ea04d299c78b Sean Paul 2018-11-29 147 * @ret: local ret/err/etc variable to track error status
b7ea04d299c78b Sean Paul 2018-11-29 148 *
b7ea04d299c78b Sean Paul 2018-11-29 149 * Use these macros to simplify grabbing all modeset locks using a local
b7ea04d299c78b Sean Paul 2018-11-29 150 * context. This has the advantage of reducing boilerplate, but also properly
b7ea04d299c78b Sean Paul 2018-11-29 151 * checking return values where appropriate.
b7ea04d299c78b Sean Paul 2018-11-29 152 *
b7ea04d299c78b Sean Paul 2018-11-29 153 * Any code run between BEGIN and END will be holding the modeset locks.
b7ea04d299c78b Sean Paul 2018-11-29 154 *
b7ea04d299c78b Sean Paul 2018-11-29 155 * This must be paired with DRM_MODESET_LOCK_ALL_END(). We will jump back and
b7ea04d299c78b Sean Paul 2018-11-29 156 * forth between the labels on deadlock and error conditions.
b7ea04d299c78b Sean Paul 2018-11-29 157 *
b7ea04d299c78b Sean Paul 2018-11-29 158 * Drivers can acquire additional modeset locks. If any lock acquisition
b7ea04d299c78b Sean Paul 2018-11-29 159 * fails, the control flow needs to jump to DRM_MODESET_LOCK_ALL_END() with
b7ea04d299c78b Sean Paul 2018-11-29 160 * the @ret parameter containing the return value of drm_modeset_lock().
b7ea04d299c78b Sean Paul 2018-11-29 161 *
b7ea04d299c78b Sean Paul 2018-11-29 162 * Returns:
b7ea04d299c78b Sean Paul 2018-11-29 163 * The only possible value of ret immediately after DRM_MODESET_LOCK_ALL_BEGIN()
b7ea04d299c78b Sean Paul 2018-11-29 164 * is 0, so no error checking is necessary
b7ea04d299c78b Sean Paul 2018-11-29 165 */
b7ea04d299c78b Sean Paul 2018-11-29 166 #define DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, flags, ret) \
77ef38574beb3e Daniel Vetter 2020-08-14 @167 if (!drm_drv_uses_atomic_modeset(dev)) \
77ef38574beb3e Daniel Vetter 2020-08-14 168 mutex_lock(&dev->mode_config.mutex); \
b7ea04d299c78b Sean Paul 2018-11-29 169 drm_modeset_acquire_init(&ctx, flags); \
b7ea04d299c78b Sean Paul 2018-11-29 170 modeset_lock_retry: \
b7ea04d299c78b Sean Paul 2018-11-29 171 ret = drm_modeset_lock_all_ctx(dev, &ctx); \
b7ea04d299c78b Sean Paul 2018-11-29 172 if (ret) \
b7ea04d299c78b Sean Paul 2018-11-29 173 goto modeset_lock_fail;
b7ea04d299c78b Sean Paul 2018-11-29 174
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 21/09/20 09:54AM, kernel test robot wrote:
>
> [auto build test ERROR on drm-exynos/exynos-drm-next]
> [also build test ERROR on tegra-drm/drm/tegra/for-next linus/master v5.15-rc2 next-20210917]
I forgot to #include <drm/drm_drv.h> for those platforms and didn't notice
because I only tried to build for X86. I'll fix it.
> [cannot apply to drm-intel/for-linux-next tegra/for-next drm-tip/drm-tip airlied/drm-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base'.
I built this patch against drm-next, which currently points to v5.15-rc1.
Should I be targeting a different branch? In any case, as suggested, I'll
remember to use "--base" in the future to make it easier to apply. Thanks for
the hint.
> All errors (new ones prefixed by >>):
>
> In file included from include/drm/drm_crtc.h:36,
> from include/drm/drm_atomic_helper.h:31,
> from drivers/gpu/drm/msm/disp/msm_disp_snapshot.h:9,
> from drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c:8:
> drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c: In function 'msm_disp_capture_atomic_state':
> >> include/drm/drm_modeset_lock.h:167:14: error: implicit declaration of function 'drm_drv_uses_atomic_modeset' [-Werror=implicit-function-declaration]
> 167 | if (!drm_drv_uses_atomic_modeset(dev)) \
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c:108:9: note: in expansion of macro 'DRM_MODESET_LOCK_ALL_BEGIN'
> 108 | DRM_MODESET_LOCK_ALL_BEGIN(ddev, ctx, 0, ret);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
Out of curiosity: The top comment says there were two build errors (one on
exynos and another one on tegra), but there is only one reported bug (on msm).
Is this because the bot only reports the first error found? Is there a link to
a report with each of the build errors on each of the platforms?
Thanks.