Re: [PATCH] drm/modeset-lock: Take the modeset BKL for legacy drivers

[Alex Deucher <alexdeucher@gmail.com>]

On Fri, Aug 14, 2020 at 5:38 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> This fell off in the conversion in
>
> commit 9bcaa3fe58ab7559e71df798bcff6e0795158695
> Author: Michal Orzel <michalorzel.eng@gmail.com>
> Date:   Tue Apr 28 19:10:04 2020 +0200
>
>     drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
>
> but it's caught by the drm_warn_on_modeset_not_all_locked() that the
> legacy modeset code uses. Since this is the bkl and it's unclear
> what's all protected, play it safe and grab it again for legacy
> drivers.
>
> Unfortunately this means we need to sprinkle a few more #includes
> around.
>
> Also we need to add the drm_device as a parameter to the _END macro.
>
> Finally remove the mute_lock() from setcrtc, since that's now done by
> the macro.
>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1224
> Fixes: 9bcaa3fe58ab ("drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers")
> Cc: Michal Orzel <michalorzel.eng@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> --
> Patch compiles but otherwise untested, and I'll go on vacations now
> for 2 weeks. Alex, can you pls take care of this?

Looks good to me.
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Also confirmed to fix the issue.  I'll push to drm-misc.

Thanks!

Alex

>
> Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 7 ++++---
>  drivers/gpu/drm/drm_color_mgmt.c    | 2 +-
>  drivers/gpu/drm/drm_crtc.c          | 4 +---
>  drivers/gpu/drm/drm_mode_object.c   | 4 ++--
>  drivers/gpu/drm/drm_plane.c         | 2 +-
>  include/drm/drm_modeset_lock.h      | 9 +++++++--
>  6 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f67ee513a7cc..7515a40b2056 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_self_refresh_helper.h>
> @@ -3109,7 +3110,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
>         if (ret)
>                 DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret);
>
> -       DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_shutdown);
>
> @@ -3249,7 +3250,7 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
>         }
>
>  unlock:
> -       DRM_MODESET_LOCK_ALL_END(ctx, err);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
>         if (err)
>                 return ERR_PTR(err);
>
> @@ -3330,7 +3331,7 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>
>         err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
>
> -       DRM_MODESET_LOCK_ALL_END(ctx, err);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
>         drm_atomic_state_put(state);
>
>         return err;
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index c93123ff7c21..138ff34b31db 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -294,7 +294,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>                                      crtc->gamma_size, &ctx);
>
>  out:
> -       DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>         return ret;
>
>  }
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 283bcc4362ca..aecdd7ea26dc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -588,7 +588,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>         if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id))
>                 return -EACCES;
>
> -       mutex_lock(&crtc->dev->mode_config.mutex);
>         DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
>                                    DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
>
> @@ -756,8 +755,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>         fb = NULL;
>         mode = NULL;
>
> -       DRM_MODESET_LOCK_ALL_END(ctx, ret);
> -       mutex_unlock(&crtc->dev->mode_config.mutex);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
>         return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 901b078abf40..db05f386a709 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -428,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  out_unref:
>         drm_mode_object_put(obj);
>  out:
> -       DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>         return ret;
>  }
>
> @@ -470,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>                 break;
>         }
>         drm_property_change_valid_put(prop, ref);
> -       DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +       DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
>         return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index b7b90b3a2e38..affe1cfed009 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -792,7 +792,7 @@ static int setplane_internal(struct drm_plane *plane,
>                                           crtc_x, crtc_y, crtc_w, crtc_h,
>                                           src_x, src_y, src_w, src_h, &ctx);
>
> -       DRM_MODESET_LOCK_ALL_END(ctx, ret);
> +       DRM_MODESET_LOCK_ALL_END(plane->dev, ctx, ret);
>
>         return ret;
>  }
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 4fc9a43ac45a..aafd07388eb7 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -164,6 +164,8 @@ int drm_modeset_lock_all_ctx(struct drm_device *dev,
>   * is 0, so no error checking is necessary
>   */
>  #define DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, flags, ret)               \
> +       if (!drm_drv_uses_atomic_modeset(dev))                          \
> +               mutex_lock(&dev->mode_config.mutex);                    \
>         drm_modeset_acquire_init(&ctx, flags);                          \
>  modeset_lock_retry:                                                    \
>         ret = drm_modeset_lock_all_ctx(dev, &ctx);                      \
> @@ -172,6 +174,7 @@ modeset_lock_retry:                                                 \
>
>  /**
>   * DRM_MODESET_LOCK_ALL_END - Helper to release and cleanup modeset locks
> + * @dev: drm device
>   * @ctx: local modeset acquire context, will be dereferenced
>   * @ret: local ret/err/etc variable to track error status
>   *
> @@ -188,7 +191,7 @@ modeset_lock_retry:                                                 \
>   * to that failure. In both of these cases the code between BEGIN/END will not
>   * be run, so the failure will reflect the inability to grab the locks.
>   */
> -#define DRM_MODESET_LOCK_ALL_END(ctx, ret)                             \
> +#define DRM_MODESET_LOCK_ALL_END(dev, ctx, ret)                                \
>  modeset_lock_fail:                                                     \
>         if (ret == -EDEADLK) {                                          \
>                 ret = drm_modeset_backoff(&ctx);                        \
> @@ -196,6 +199,8 @@ modeset_lock_fail:                                                  \
>                         goto modeset_lock_retry;                        \
>         }                                                               \
>         drm_modeset_drop_locks(&ctx);                                   \
> -       drm_modeset_acquire_fini(&ctx);
> +       drm_modeset_acquire_fini(&ctx);                                 \
> +       if (!drm_drv_uses_atomic_modeset(dev))                          \
> +               mutex_unlock(&dev->mode_config.mutex);
>
>  #endif /* DRM_MODESET_LOCK_H_ */
> --
> 2.28.0
>