[1/3] drm/modeset: Prime modeset lock vs dma_resv
diff mbox series

Message ID 20191119210844.16947-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • more dma-buf lockdep priming
Related show

Commit Message

Daniel Vetter Nov. 19, 2019, 9:08 p.m. UTC
It's kinda really hard to get this wrong on a driver with both display
and dma_resv locking. But who ever knows, so better to make sure that
really all drivers nest these the same way.

For actual lock semantics the acquire context nesting doesn't matter.
But to teach lockdep what's going on with ww_mutex the acquire ctx is
a fake lockdep lock, hence from a lockdep pov it does matter. That's
why I figured better to include it.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_mode_config.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Christian König Nov. 20, 2019, 8:34 a.m. UTC | #1
Am 19.11.19 um 22:08 schrieb Daniel Vetter:
> It's kinda really hard to get this wrong on a driver with both display
> and dma_resv locking. But who ever knows, so better to make sure that
> really all drivers nest these the same way.
>
> For actual lock semantics the acquire context nesting doesn't matter.
> But to teach lockdep what's going on with ww_mutex the acquire ctx is
> a fake lockdep lock, hence from a lockdep pov it does matter. That's
> why I figured better to include it.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Why not add another __init function like we did for dma_resv? That 
looked rather clean to me.

Either why feel free to add an Acked-by: Christian König 
<christian.koenig@amd.com> to the patch.

Regards,
Christian.

> ---
>   drivers/gpu/drm/drm_mode_config.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 3b570a404933..08e6eff6a179 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -27,6 +27,7 @@
>   #include <drm/drm_file.h>
>   #include <drm/drm_mode_config.h>
>   #include <drm/drm_print.h>
> +#include <linux/dma-resv.h>
>   
>   #include "drm_crtc_internal.h"
>   #include "drm_internal.h"
> @@ -415,6 +416,33 @@ void drm_mode_config_init(struct drm_device *dev)
>   	dev->mode_config.num_crtc = 0;
>   	dev->mode_config.num_encoder = 0;
>   	dev->mode_config.num_total_plane = 0;
> +
> +	if (IS_ENABLED(CONFIG_LOCKDEP)) {
> +		struct drm_modeset_acquire_ctx modeset_ctx;
> +		struct ww_acquire_ctx resv_ctx;
> +		struct dma_resv resv;
> +		int ret;
> +
> +		dma_resv_init(&resv);
> +
> +		drm_modeset_acquire_init(&modeset_ctx, 0);
> +		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> +				       &modeset_ctx);
> +		if (ret == -EDEADLK)
> +			ret = drm_modeset_backoff(&modeset_ctx);
> +
> +		ww_acquire_init(&resv_ctx, &reservation_ww_class);
> +		ret = dma_resv_lock(&resv, &resv_ctx);
> +		if (ret == -EDEADLK)
> +			dma_resv_lock_slow(&resv, &resv_ctx);
> +
> +		dma_resv_unlock(&resv);
> +		ww_acquire_fini(&resv_ctx);
> +
> +		drm_modeset_drop_locks(&modeset_ctx);
> +		drm_modeset_acquire_fini(&modeset_ctx);
> +		dma_resv_fini(&resv);
> +	}
>   }
>   EXPORT_SYMBOL(drm_mode_config_init);
>
Daniel Vetter Nov. 20, 2019, 10:55 a.m. UTC | #2
On Wed, Nov 20, 2019 at 09:34:03AM +0100, Christian König wrote:
> Am 19.11.19 um 22:08 schrieb Daniel Vetter:
> > It's kinda really hard to get this wrong on a driver with both display
> > and dma_resv locking. But who ever knows, so better to make sure that
> > really all drivers nest these the same way.
> > 
> > For actual lock semantics the acquire context nesting doesn't matter.
> > But to teach lockdep what's going on with ww_mutex the acquire ctx is
> > a fake lockdep lock, hence from a lockdep pov it does matter. That's
> > why I figured better to include it.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Why not add another __init function like we did for dma_resv? That looked
> rather clean to me.

Hm, I've only done that because callers of dma_resv_init where holding
lots of locks already (ttm ghost objects). At least in i915 we try to do
all lockdep priming as close as possible to the mutex_init calls, so it's
all together. Since often there's no separate obj_init function, and you
need to use the same mutex_init to make sure you have the same static
lockdep key.

No strong opinion here from me, just wanted to explain why I'm biased to
this way of doing things.

> Either why feel free to add an Acked-by: Christian König
> <christian.koenig@amd.com> to the patch.

Thanks. Can you pls also look at patch 2, at least from a ttm/amdgpu pov?

Cheers, Daniel

> 
> Regards,
> Christian.
> 
> > ---
> >   drivers/gpu/drm/drm_mode_config.c | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 3b570a404933..08e6eff6a179 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -27,6 +27,7 @@
> >   #include <drm/drm_file.h>
> >   #include <drm/drm_mode_config.h>
> >   #include <drm/drm_print.h>
> > +#include <linux/dma-resv.h>
> >   #include "drm_crtc_internal.h"
> >   #include "drm_internal.h"
> > @@ -415,6 +416,33 @@ void drm_mode_config_init(struct drm_device *dev)
> >   	dev->mode_config.num_crtc = 0;
> >   	dev->mode_config.num_encoder = 0;
> >   	dev->mode_config.num_total_plane = 0;
> > +
> > +	if (IS_ENABLED(CONFIG_LOCKDEP)) {
> > +		struct drm_modeset_acquire_ctx modeset_ctx;
> > +		struct ww_acquire_ctx resv_ctx;
> > +		struct dma_resv resv;
> > +		int ret;
> > +
> > +		dma_resv_init(&resv);
> > +
> > +		drm_modeset_acquire_init(&modeset_ctx, 0);
> > +		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> > +				       &modeset_ctx);
> > +		if (ret == -EDEADLK)
> > +			ret = drm_modeset_backoff(&modeset_ctx);
> > +
> > +		ww_acquire_init(&resv_ctx, &reservation_ww_class);
> > +		ret = dma_resv_lock(&resv, &resv_ctx);
> > +		if (ret == -EDEADLK)
> > +			dma_resv_lock_slow(&resv, &resv_ctx);
> > +
> > +		dma_resv_unlock(&resv);
> > +		ww_acquire_fini(&resv_ctx);
> > +
> > +		drm_modeset_drop_locks(&modeset_ctx);
> > +		drm_modeset_acquire_fini(&modeset_ctx);
> > +		dma_resv_fini(&resv);
> > +	}
> >   }
> >   EXPORT_SYMBOL(drm_mode_config_init);
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 3b570a404933..08e6eff6a179 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -27,6 +27,7 @@ 
 #include <drm/drm_file.h>
 #include <drm/drm_mode_config.h>
 #include <drm/drm_print.h>
+#include <linux/dma-resv.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -415,6 +416,33 @@  void drm_mode_config_init(struct drm_device *dev)
 	dev->mode_config.num_crtc = 0;
 	dev->mode_config.num_encoder = 0;
 	dev->mode_config.num_total_plane = 0;
+
+	if (IS_ENABLED(CONFIG_LOCKDEP)) {
+		struct drm_modeset_acquire_ctx modeset_ctx;
+		struct ww_acquire_ctx resv_ctx;
+		struct dma_resv resv;
+		int ret;
+
+		dma_resv_init(&resv);
+
+		drm_modeset_acquire_init(&modeset_ctx, 0);
+		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
+				       &modeset_ctx);
+		if (ret == -EDEADLK)
+			ret = drm_modeset_backoff(&modeset_ctx);
+
+		ww_acquire_init(&resv_ctx, &reservation_ww_class);
+		ret = dma_resv_lock(&resv, &resv_ctx);
+		if (ret == -EDEADLK)
+			dma_resv_lock_slow(&resv, &resv_ctx);
+
+		dma_resv_unlock(&resv);
+		ww_acquire_fini(&resv_ctx);
+
+		drm_modeset_drop_locks(&modeset_ctx);
+		drm_modeset_acquire_fini(&modeset_ctx);
+		dma_resv_fini(&resv);
+	}
 }
 EXPORT_SYMBOL(drm_mode_config_init);