Re: [Intel-gfx] [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Fernando Ramos]

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

Re: [Intel-gfx] [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

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