| Message ID | 20210916211552.33490-1-greenfoo@u92.eu |
|---|---|
| Headers | show |
| Series | drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible | expand |
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 <greenfoo@u92.eu> > --- > 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 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 <sean@poorly.run> > Signed-off-by: Fernando Ramos <greenfoo@u92.eu> > --- > 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 <sean@poorly.run> > Signed-off-by: Fernando Ramos <greenfoo@u92.eu> > --- > 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:44PM +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 <greenfoo@u92.eu> Reviewed-by: Sean Paul <sean@poorly.run> > --- > drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c > index 7db01904d18d..8ee215ab614e 100644 > --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c > @@ -156,10 +156,12 @@ static int shmob_drm_pm_suspend(struct device *dev) > static int shmob_drm_pm_resume(struct device *dev) > { > struct shmob_drm_device *sdev = dev_get_drvdata(dev); > + struct drm_modeset_acquire_ctx ctx; > + int ret; > > - drm_modeset_lock_all(sdev->ddev); > + DRM_MODESET_LOCK_ALL_BEGIN(sdev->ddev, ctx, 0, ret); > shmob_drm_crtc_resume(&sdev->crtc); > - drm_modeset_unlock_all(sdev->ddev); > + DRM_MODESET_LOCK_ALL_END(sdev->ddev, ctx, ret); > > drm_kms_helper_poll_enable(sdev->ddev); > return 0; > -- > 2.33.0 > -- Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Sep 16, 2021 at 11:15:46PM +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 <greenfoo@u92.eu> > --- > drivers/gpu/drm/omapdrm/omap_fb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c > index 190afc564914..56013c3ef701 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fb.c > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c > @@ -62,13 +62,15 @@ static int omap_framebuffer_dirty(struct drm_framebuffer *fb, > unsigned num_clips) > { > struct drm_crtc *crtc; > + struct drm_modeset_acquire_ctx ctx; > + int ret; > > - drm_modeset_lock_all(fb->dev); > + DRM_MODESET_LOCK_ALL_BEGIN(fb->dev, ctx, 0, ret); > > drm_for_each_crtc(crtc, fb->dev) > omap_crtc_flush(crtc); > > - drm_modeset_unlock_all(fb->dev); > + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret); > > return 0; Return ret here, with that, Reviewed-by: Sean Paul <sean@poorly.run> > } > -- > 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 <greenfoo@u92.eu> > --- > 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:52PM +0200, Fernando Ramos wrote: > The previous commits do exactly what this entry in the TODO file asks > for, thus we can remove it now as it is no longer applicable. Thanks for doing this work! Can we remove drm_modeset_lock_all[_ctx] now? If so, let's queue that up as part of the set. Reviewed-by: Sean Paul <sean@poorly.run> > > Signed-off-by: Fernando Ramos <greenfoo@u92.eu> > --- > Documentation/gpu/todo.rst | 17 ----------------- > Documentation/locking/ww-mutex-design.rst | 2 +- > 2 files changed, 1 insertion(+), 18 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 12e61869939e..6613543955e9 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -353,23 +353,6 @@ converted, except for struct drm_driver.gem_prime_mmap. > > Level: Intermediate > > -Use DRM_MODESET_LOCK_ALL_* helpers instead of boilerplate > ---------------------------------------------------------- > - > -For cases where drivers are attempting to grab the modeset locks with a local > -acquire context. Replace the boilerplate code surrounding > -drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN() and > -DRM_MODESET_LOCK_ALL_END() instead. > - > -This should also be done for all places where drm_modeset_lock_all() is still > -used. > - > -As a reference, take a look at the conversions already completed in drm core. > - > -Contact: Sean Paul, respective driver maintainers > - > -Level: Starter > - > Rename CMA helpers to DMA helpers > --------------------------------- > > diff --git a/Documentation/locking/ww-mutex-design.rst b/Documentation/locking/ww-mutex-design.rst > index 6a4d7319f8f0..6a8f8beb9ec4 100644 > --- a/Documentation/locking/ww-mutex-design.rst > +++ b/Documentation/locking/ww-mutex-design.rst > @@ -60,7 +60,7 @@ Concepts > Compared to normal mutexes two additional concepts/objects show up in the lock > interface for w/w mutexes: > > -Acquire context: To ensure eventual forward progress it is important the a task > +Acquire context: To ensure eventual forward progress it is important that a task > trying to acquire locks doesn't grab a new reservation id, but keeps the one it > acquired when starting the lock acquisition. This ticket is stored in the > acquire context. Furthermore the acquire context keeps track of debugging state > -- > 2.33.0 > -- Sean Paul, Software Engineer, Google / Chromium OS
> > 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?
> > - drm_modeset_unlock_all(fb->dev); > > + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret); > > > > return 0; > > Return ret here. Done!
> > int i, out_width; > > + int ret; > > Please put ret with i & out_width Done! > > - drm_modeset_unlock_all(crtc->dev); > > + DRM_MODESET_LOCK_ALL_END(crtc->dev, ctx, ret); > > > > return 0; > > Return ret here Done!
> Can we remove drm_modeset_lock_all[_ctx] now? If so, let's queue that up as part > of the set. > drm_modeset_lock_all() and drm_modeset_unlock_all() can be removed (I'll do that on v2 of this patch series). drm_modset_lock_all_ctx() is a different story and there are still two places (one in the i915 driver and another one in the amd driver) where they are needed. I would need to understand the code better before trying to remove those :)
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(-)