* [PATCH 0/3] more dma-buf lockdep priming
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development; +Cc: Daniel Vetter
Hi all,
While looking at (dynamic) dma-buf issues and ideas I've spotted a bit
more room for locking down the cross-driver dma_resv rules.
Only functional fallout is a tiny patch for msm, assuming I didn't botch
anything in the auditing of all relevant code.
Comments, review and testing very much appreciated, as usual.
Cheers, Daniel
Daniel Vetter (3):
drm/modeset: Prime modeset lock vs dma_resv
dma-resv: Also prime acquire ctx for lockdep
drm/msm: Don't init ww_mutec acquire ctx before needed
drivers/dma-buf/dma-resv.c | 8 +++++++-
drivers/gpu/drm/drm_mode_config.c | 28 ++++++++++++++++++++++++++++
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
3 files changed, 36 insertions(+), 2 deletions(-)
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 0/3] more dma-buf lockdep priming
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development; +Cc: Daniel Vetter
Hi all,
While looking at (dynamic) dma-buf issues and ideas I've spotted a bit
more room for locking down the cross-driver dma_resv rules.
Only functional fallout is a tiny patch for msm, assuming I didn't botch
anything in the auditing of all relevant code.
Comments, review and testing very much appreciated, as usual.
Cheers, Daniel
Daniel Vetter (3):
drm/modeset: Prime modeset lock vs dma_resv
dma-resv: Also prime acquire ctx for lockdep
drm/msm: Don't init ww_mutec acquire ctx before needed
drivers/dma-buf/dma-resv.c | 8 +++++++-
drivers/gpu/drm/drm_mode_config.c | 28 ++++++++++++++++++++++++++++
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
3 files changed, 36 insertions(+), 2 deletions(-)
--
2.24.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Intel-gfx] [PATCH 0/3] more dma-buf lockdep priming
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development; +Cc: Daniel Vetter
Hi all,
While looking at (dynamic) dma-buf issues and ideas I've spotted a bit
more room for locking down the cross-driver dma_resv rules.
Only functional fallout is a tiny patch for msm, assuming I didn't botch
anything in the auditing of all relevant code.
Comments, review and testing very much appreciated, as usual.
Cheers, Daniel
Daniel Vetter (3):
drm/modeset: Prime modeset lock vs dma_resv
dma-resv: Also prime acquire ctx for lockdep
drm/msm: Don't init ww_mutec acquire ctx before needed
drivers/dma-buf/dma-resv.c | 8 +++++++-
drivers/gpu/drm/drm_mode_config.c | 28 ++++++++++++++++++++++++++++
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
3 files changed, 36 insertions(+), 2 deletions(-)
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Daniel Vetter, Daniel Vetter, Christian König
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(+)
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);
--
2.24.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Intel-gfx] [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Daniel Vetter, Daniel Vetter, Christian König
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(+)
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);
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
2019-11-19 21:08 ` Daniel Vetter
(?)
(?)
@ 2019-11-19 21:08 ` Daniel Vetter
-1 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Daniel Vetter, Maarten Lankhorst, Chris Wilson,
Christian König, Sumit Semwal, linux-media, linaro-mm-sig,
Huang Rui, Eric Anholt, Ben Skeggs, Alex Deucher, Rob Herring,
Lucas Stach, Russell King, Christian Gmeiner, Rob Clark,
Sean Paul, Daniel Vetter
Semnatically it really doesn't matter where we grab the ticket. But
since the ticket is a fake lockdep lock, it matters for lockdep
validation purposes.
This means stuff like grabbing a ticket and then doing
copy_from/to_user isn't allowed anymore. This is a changed compared to
the current ttm fault handler, which doesn't bother with having a full
reservation. Since I'm looking into fixing the TODO entry in
ttm_mem_evict_wait_busy() I think that'll have to change sooner or
later anyway, better get started. A bit more context on why I'm
looking into this: For backwards compat with existing i915 gem code I
think we'll have to do full slowpath locking in the i915 equivalent of
the eviction code. And with dynamic dma-buf that will leak across
drivers, so another thing we need to standardize and make sure it's
done the same way everyway.
Unfortunately this means another full audit of all drivers:
- gem helpers: acquire_init is done right before taking locks, so no
problem. Same for acquire_fini and unlocking, which means nothing
that's not already covered by the dma_resv_lock rules will be caught
with this extension here to the acquire_ctx.
- etnaviv: An absolute massive amount of code is run between the
acquire_init and the first lock acquisition in submit_lock_objects.
But nothing that would touch user memory and could cause a fault.
Furthermore nothing that uses the ticket, so even if I missed
something, it would be easy to fix by pushing the acquire_init right
before the first use. Similar on the unlock/acquire_fini side.
- i915: Right now (and this will likely change a lot rsn) the acquire
ctx and actual locks are right next to each another. No problem.
- msm has a problem: submit_create calls acquire_init, but then
submit_lookup_objects() has a bunch of copy_from_user to do the
object lookups. That's the only thing before submit_lock_objects
call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
does not have this issue since it copies all the userspace structs
earlier. submit_cleanup does not have any such issues.
With the prep patch to pull out the acquire_ctx and reorder it msm
is going to be safe too.
- nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
Similar on the acquire_fini/ttm_bo_unreserve side.
- ttm execbuf utils: acquire context and locking are even in the same
functions here (one function to reserve everything, the other to
unreserve), so all good.
- vc4: Another case where acquire context and locking are handled in
the same functions (one function to lock everything, the other to
unlock).
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: Huang Rui <ray.huang@amd.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/dma-buf/dma-resv.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index d3c760e19991..079e38fde33a 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
static void __init dma_resv_lockdep(void)
{
struct mm_struct *mm = mm_alloc();
+ struct ww_acquire_ctx ctx;
struct dma_resv obj;
+ int ret;
if (!mm)
return;
@@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
dma_resv_init(&obj);
down_read(&mm->mmap_sem);
- ww_mutex_lock(&obj.lock, NULL);
+ ww_acquire_init(&ctx, &reservation_ww_class);
+ ret = dma_resv_lock(&obj, &ctx);
+ if (ret == -EDEADLK)
+ dma_resv_lock_slow(&obj, &ctx);
fs_reclaim_acquire(GFP_KERNEL);
fs_reclaim_release(GFP_KERNEL);
ww_mutex_unlock(&obj.lock);
+ ww_acquire_fini(&ctx);
up_read(&mm->mmap_sem);
mmput(mm);
--
2.24.0
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Rob Herring, Daniel Vetter, Russell King, Christian König,
linaro-mm-sig, Eric Anholt, Huang Rui, Ben Skeggs, Lucas Stach,
Alex Deucher, Daniel Vetter, Sumit Semwal, linux-media
Semnatically it really doesn't matter where we grab the ticket. But
since the ticket is a fake lockdep lock, it matters for lockdep
validation purposes.
This means stuff like grabbing a ticket and then doing
copy_from/to_user isn't allowed anymore. This is a changed compared to
the current ttm fault handler, which doesn't bother with having a full
reservation. Since I'm looking into fixing the TODO entry in
ttm_mem_evict_wait_busy() I think that'll have to change sooner or
later anyway, better get started. A bit more context on why I'm
looking into this: For backwards compat with existing i915 gem code I
think we'll have to do full slowpath locking in the i915 equivalent of
the eviction code. And with dynamic dma-buf that will leak across
drivers, so another thing we need to standardize and make sure it's
done the same way everyway.
Unfortunately this means another full audit of all drivers:
- gem helpers: acquire_init is done right before taking locks, so no
problem. Same for acquire_fini and unlocking, which means nothing
that's not already covered by the dma_resv_lock rules will be caught
with this extension here to the acquire_ctx.
- etnaviv: An absolute massive amount of code is run between the
acquire_init and the first lock acquisition in submit_lock_objects.
But nothing that would touch user memory and could cause a fault.
Furthermore nothing that uses the ticket, so even if I missed
something, it would be easy to fix by pushing the acquire_init right
before the first use. Similar on the unlock/acquire_fini side.
- i915: Right now (and this will likely change a lot rsn) the acquire
ctx and actual locks are right next to each another. No problem.
- msm has a problem: submit_create calls acquire_init, but then
submit_lookup_objects() has a bunch of copy_from_user to do the
object lookups. That's the only thing before submit_lock_objects
call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
does not have this issue since it copies all the userspace structs
earlier. submit_cleanup does not have any such issues.
With the prep patch to pull out the acquire_ctx and reorder it msm
is going to be safe too.
- nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
Similar on the acquire_fini/ttm_bo_unreserve side.
- ttm execbuf utils: acquire context and locking are even in the same
functions here (one function to reserve everything, the other to
unreserve), so all good.
- vc4: Another case where acquire context and locking are handled in
the same functions (one function to lock everything, the other to
unlock).
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: Huang Rui <ray.huang@amd.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/dma-buf/dma-resv.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index d3c760e19991..079e38fde33a 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
static void __init dma_resv_lockdep(void)
{
struct mm_struct *mm = mm_alloc();
+ struct ww_acquire_ctx ctx;
struct dma_resv obj;
+ int ret;
if (!mm)
return;
@@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
dma_resv_init(&obj);
down_read(&mm->mmap_sem);
- ww_mutex_lock(&obj.lock, NULL);
+ ww_acquire_init(&ctx, &reservation_ww_class);
+ ret = dma_resv_lock(&obj, &ctx);
+ if (ret == -EDEADLK)
+ dma_resv_lock_slow(&obj, &ctx);
fs_reclaim_acquire(GFP_KERNEL);
fs_reclaim_release(GFP_KERNEL);
ww_mutex_unlock(&obj.lock);
+ ww_acquire_fini(&ctx);
up_read(&mm->mmap_sem);
mmput(mm);
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Daniel Vetter, Russell King, Christian König, linaro-mm-sig,
Huang Rui, Ben Skeggs, Alex Deucher, Daniel Vetter, Sean Paul,
linux-media
Semnatically it really doesn't matter where we grab the ticket. But
since the ticket is a fake lockdep lock, it matters for lockdep
validation purposes.
This means stuff like grabbing a ticket and then doing
copy_from/to_user isn't allowed anymore. This is a changed compared to
the current ttm fault handler, which doesn't bother with having a full
reservation. Since I'm looking into fixing the TODO entry in
ttm_mem_evict_wait_busy() I think that'll have to change sooner or
later anyway, better get started. A bit more context on why I'm
looking into this: For backwards compat with existing i915 gem code I
think we'll have to do full slowpath locking in the i915 equivalent of
the eviction code. And with dynamic dma-buf that will leak across
drivers, so another thing we need to standardize and make sure it's
done the same way everyway.
Unfortunately this means another full audit of all drivers:
- gem helpers: acquire_init is done right before taking locks, so no
problem. Same for acquire_fini and unlocking, which means nothing
that's not already covered by the dma_resv_lock rules will be caught
with this extension here to the acquire_ctx.
- etnaviv: An absolute massive amount of code is run between the
acquire_init and the first lock acquisition in submit_lock_objects.
But nothing that would touch user memory and could cause a fault.
Furthermore nothing that uses the ticket, so even if I missed
something, it would be easy to fix by pushing the acquire_init right
before the first use. Similar on the unlock/acquire_fini side.
- i915: Right now (and this will likely change a lot rsn) the acquire
ctx and actual locks are right next to each another. No problem.
- msm has a problem: submit_create calls acquire_init, but then
submit_lookup_objects() has a bunch of copy_from_user to do the
object lookups. That's the only thing before submit_lock_objects
call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
does not have this issue since it copies all the userspace structs
earlier. submit_cleanup does not have any such issues.
With the prep patch to pull out the acquire_ctx and reorder it msm
is going to be safe too.
- nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
Similar on the acquire_fini/ttm_bo_unreserve side.
- ttm execbuf utils: acquire context and locking are even in the same
functions here (one function to reserve everything, the other to
unreserve), so all good.
- vc4: Another case where acquire context and locking are handled in
the same functions (one function to lock everything, the other to
unlock).
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: Huang Rui <ray.huang@amd.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/dma-buf/dma-resv.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index d3c760e19991..079e38fde33a 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
static void __init dma_resv_lockdep(void)
{
struct mm_struct *mm = mm_alloc();
+ struct ww_acquire_ctx ctx;
struct dma_resv obj;
+ int ret;
if (!mm)
return;
@@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
dma_resv_init(&obj);
down_read(&mm->mmap_sem);
- ww_mutex_lock(&obj.lock, NULL);
+ ww_acquire_init(&ctx, &reservation_ww_class);
+ ret = dma_resv_lock(&obj, &ctx);
+ if (ret == -EDEADLK)
+ dma_resv_lock_slow(&obj, &ctx);
fs_reclaim_acquire(GFP_KERNEL);
fs_reclaim_release(GFP_KERNEL);
ww_mutex_unlock(&obj.lock);
+ ww_acquire_fini(&ctx);
up_read(&mm->mmap_sem);
mmput(mm);
--
2.24.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Intel-gfx] [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Rob Herring, Daniel Vetter, Russell King, Christian König,
linaro-mm-sig, Eric Anholt, Huang Rui, Ben Skeggs, Lucas Stach,
Alex Deucher, Daniel Vetter, Sumit Semwal, linux-media
Semnatically it really doesn't matter where we grab the ticket. But
since the ticket is a fake lockdep lock, it matters for lockdep
validation purposes.
This means stuff like grabbing a ticket and then doing
copy_from/to_user isn't allowed anymore. This is a changed compared to
the current ttm fault handler, which doesn't bother with having a full
reservation. Since I'm looking into fixing the TODO entry in
ttm_mem_evict_wait_busy() I think that'll have to change sooner or
later anyway, better get started. A bit more context on why I'm
looking into this: For backwards compat with existing i915 gem code I
think we'll have to do full slowpath locking in the i915 equivalent of
the eviction code. And with dynamic dma-buf that will leak across
drivers, so another thing we need to standardize and make sure it's
done the same way everyway.
Unfortunately this means another full audit of all drivers:
- gem helpers: acquire_init is done right before taking locks, so no
problem. Same for acquire_fini and unlocking, which means nothing
that's not already covered by the dma_resv_lock rules will be caught
with this extension here to the acquire_ctx.
- etnaviv: An absolute massive amount of code is run between the
acquire_init and the first lock acquisition in submit_lock_objects.
But nothing that would touch user memory and could cause a fault.
Furthermore nothing that uses the ticket, so even if I missed
something, it would be easy to fix by pushing the acquire_init right
before the first use. Similar on the unlock/acquire_fini side.
- i915: Right now (and this will likely change a lot rsn) the acquire
ctx and actual locks are right next to each another. No problem.
- msm has a problem: submit_create calls acquire_init, but then
submit_lookup_objects() has a bunch of copy_from_user to do the
object lookups. That's the only thing before submit_lock_objects
call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
does not have this issue since it copies all the userspace structs
earlier. submit_cleanup does not have any such issues.
With the prep patch to pull out the acquire_ctx and reorder it msm
is going to be safe too.
- nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
Similar on the acquire_fini/ttm_bo_unreserve side.
- ttm execbuf utils: acquire context and locking are even in the same
functions here (one function to reserve everything, the other to
unreserve), so all good.
- vc4: Another case where acquire context and locking are handled in
the same functions (one function to lock everything, the other to
unlock).
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: Huang Rui <ray.huang@amd.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/dma-buf/dma-resv.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index d3c760e19991..079e38fde33a 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
static void __init dma_resv_lockdep(void)
{
struct mm_struct *mm = mm_alloc();
+ struct ww_acquire_ctx ctx;
struct dma_resv obj;
+ int ret;
if (!mm)
return;
@@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
dma_resv_init(&obj);
down_read(&mm->mmap_sem);
- ww_mutex_lock(&obj.lock, NULL);
+ ww_acquire_init(&ctx, &reservation_ww_class);
+ ret = dma_resv_lock(&obj, &ctx);
+ if (ret == -EDEADLK)
+ dma_resv_lock_slow(&obj, &ctx);
fs_reclaim_acquire(GFP_KERNEL);
fs_reclaim_release(GFP_KERNEL);
ww_mutex_unlock(&obj.lock);
+ ww_acquire_fini(&ctx);
up_read(&mm->mmap_sem);
mmput(mm);
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
2019-11-19 21:08 ` Daniel Vetter
(?)
@ 2019-11-19 21:08 ` Daniel Vetter
-1 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Daniel Vetter, Daniel Vetter, Rob Clark, Sean Paul,
linux-arm-msm, freedreno
For locking semantics it really doesn't matter when we grab the
ticket. But for lockdep validation it does: the acquire ctx is a fake
lockdep. Since other drivers might want to do a full multi-lock dance
in their fault-handler, not just lock a single dma_resv. Therefore we
must init the acquire_ctx only after we've done all the copy_*_user or
anything else that might trigger a pagefault. For msm this means we
need to move it past submit_lookup_objects.
Aside: Why is msm still using struct_mutex, it seems to be using
dma_resv_lock for general buffer state protection?
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index fb1fdd7fa3ef..126b2f62bfe7 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
INIT_LIST_HEAD(&submit->node);
INIT_LIST_HEAD(&submit->bo_list);
- ww_acquire_init(&submit->ticket, &reservation_ww_class);
return submit;
}
@@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
+ ww_acquire_init(&submit->ticket, &reservation_ww_class);
ret = submit_lock_objects(submit);
if (ret)
goto out;
--
2.24.0
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Sean Paul, Daniel Vetter, linux-arm-msm, Daniel Vetter, freedreno
For locking semantics it really doesn't matter when we grab the
ticket. But for lockdep validation it does: the acquire ctx is a fake
lockdep. Since other drivers might want to do a full multi-lock dance
in their fault-handler, not just lock a single dma_resv. Therefore we
must init the acquire_ctx only after we've done all the copy_*_user or
anything else that might trigger a pagefault. For msm this means we
need to move it past submit_lookup_objects.
Aside: Why is msm still using struct_mutex, it seems to be using
dma_resv_lock for general buffer state protection?
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index fb1fdd7fa3ef..126b2f62bfe7 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
INIT_LIST_HEAD(&submit->node);
INIT_LIST_HEAD(&submit->bo_list);
- ww_acquire_init(&submit->ticket, &reservation_ww_class);
return submit;
}
@@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
+ ww_acquire_init(&submit->ticket, &reservation_ww_class);
ret = submit_lock_objects(submit);
if (ret)
goto out;
--
2.24.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-19 21:08 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw)
To: Intel Graphics Development, DRI Development
Cc: Daniel Vetter, linux-arm-msm, Daniel Vetter, freedreno
For locking semantics it really doesn't matter when we grab the
ticket. But for lockdep validation it does: the acquire ctx is a fake
lockdep. Since other drivers might want to do a full multi-lock dance
in their fault-handler, not just lock a single dma_resv. Therefore we
must init the acquire_ctx only after we've done all the copy_*_user or
anything else that might trigger a pagefault. For msm this means we
need to move it past submit_lookup_objects.
Aside: Why is msm still using struct_mutex, it seems to be using
dma_resv_lock for general buffer state protection?
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index fb1fdd7fa3ef..126b2f62bfe7 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
INIT_LIST_HEAD(&submit->node);
INIT_LIST_HEAD(&submit->bo_list);
- ww_acquire_init(&submit->ticket, &reservation_ww_class);
return submit;
}
@@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
+ ww_acquire_init(&submit->ticket, &reservation_ww_class);
ret = submit_lock_objects(submit);
if (ret)
goto out;
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 49+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for more dma-buf lockdep priming
@ 2019-11-19 22:31 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-19 22:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming
URL : https://patchwork.freedesktop.org/series/69695/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
877e4edf1ff5 drm/modeset: Prime modeset lock vs dma_resv
-:68: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 0 errors, 1 warnings, 0 checks, 40 lines checked
50bc03ff3da7 dma-resv: Also prime acquire ctx for lockdep
-:110: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 0 errors, 1 warnings, 0 checks, 24 lines checked
59aa67eff613 drm/msm: Don't init ww_mutec acquire ctx before needed
-:42: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 0 errors, 1 warnings, 0 checks, 14 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for more dma-buf lockdep priming
@ 2019-11-19 22:31 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-19 22:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming
URL : https://patchwork.freedesktop.org/series/69695/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
877e4edf1ff5 drm/modeset: Prime modeset lock vs dma_resv
-:68: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 0 errors, 1 warnings, 0 checks, 40 lines checked
50bc03ff3da7 dma-resv: Also prime acquire ctx for lockdep
-:110: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 0 errors, 1 warnings, 0 checks, 24 lines checked
59aa67eff613 drm/msm: Don't init ww_mutec acquire ctx before needed
-:42: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 0 errors, 1 warnings, 0 checks, 14 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* ✓ Fi.CI.BAT: success for more dma-buf lockdep priming
@ 2019-11-19 22:53 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-19 22:53 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming
URL : https://patchwork.freedesktop.org/series/69695/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7380 -> Patchwork_15335
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/index.html
Known issues
------------
Here are the changes found in Patchwork_15335 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_pm_rpm@module-reload:
- fi-kbl-guc: [PASS][1] -> [SKIP][2] ([fdo#109271])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live_blt:
- fi-hsw-peppy: [PASS][3] -> [DMESG-FAIL][4] ([fdo#112147])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-hsw-peppy/igt@i915_selftest@live_blt.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-hsw-peppy/igt@i915_selftest@live_blt.html
* igt@i915_selftest@live_gt_heartbeat:
- fi-kbl-guc: [PASS][5] -> [DMESG-FAIL][6] ([fdo#112096])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-kbl-guc/igt@i915_selftest@live_gt_heartbeat.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-kbl-guc/igt@i915_selftest@live_gt_heartbeat.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [PASS][7] -> [FAIL][8] ([fdo#111045] / [fdo#111096])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
#### Possible fixes ####
* igt@i915_module_load@reload-with-fault-injection:
- fi-bsw-nick: [INCOMPLETE][9] ([fdo#105876]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html
* igt@i915_pm_rpm@module-reload:
- fi-skl-6770hq: [FAIL][11] ([fdo#108511]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live_blt:
- fi-bsw-n3050: [DMESG-FAIL][13] ([fdo#112176]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-bsw-n3050/igt@i915_selftest@live_blt.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-bsw-n3050/igt@i915_selftest@live_blt.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#105876]: https://bugs.freedesktop.org/show_bug.cgi?id=105876
[fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109964]: https://bugs.freedesktop.org/show_bug.cgi?id=109964
[fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
[fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
[fdo#112096]: https://bugs.freedesktop.org/show_bug.cgi?id=112096
[fdo#112147]: https://bugs.freedesktop.org/show_bug.cgi?id=112147
[fdo#112176]: https://bugs.freedesktop.org/show_bug.cgi?id=112176
[fdo#112298]: https://bugs.freedesktop.org/show_bug.cgi?id=112298
Participating hosts (49 -> 43)
------------------------------
Missing (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_7380 -> Patchwork_15335
CI-20190529: 20190529
CI_DRM_7380: 7b0a3189ead776ce02426186e8d63cff190a453e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5295: 9211e4794e40135d797e6d056d6d8d40076acb92 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_15335: 59aa67eff61389d8a898bbf74432bfaddbc44554 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
59aa67eff613 drm/msm: Don't init ww_mutec acquire ctx before needed
50bc03ff3da7 dma-resv: Also prime acquire ctx for lockdep
877e4edf1ff5 drm/modeset: Prime modeset lock vs dma_resv
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for more dma-buf lockdep priming
@ 2019-11-19 22:53 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-19 22:53 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming
URL : https://patchwork.freedesktop.org/series/69695/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7380 -> Patchwork_15335
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/index.html
Known issues
------------
Here are the changes found in Patchwork_15335 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_pm_rpm@module-reload:
- fi-kbl-guc: [PASS][1] -> [SKIP][2] ([fdo#109271])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live_blt:
- fi-hsw-peppy: [PASS][3] -> [DMESG-FAIL][4] ([fdo#112147])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-hsw-peppy/igt@i915_selftest@live_blt.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-hsw-peppy/igt@i915_selftest@live_blt.html
* igt@i915_selftest@live_gt_heartbeat:
- fi-kbl-guc: [PASS][5] -> [DMESG-FAIL][6] ([fdo#112096])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-kbl-guc/igt@i915_selftest@live_gt_heartbeat.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-kbl-guc/igt@i915_selftest@live_gt_heartbeat.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [PASS][7] -> [FAIL][8] ([fdo#111045] / [fdo#111096])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
#### Possible fixes ####
* igt@i915_module_load@reload-with-fault-injection:
- fi-bsw-nick: [INCOMPLETE][9] ([fdo#105876]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html
* igt@i915_pm_rpm@module-reload:
- fi-skl-6770hq: [FAIL][11] ([fdo#108511]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live_blt:
- fi-bsw-n3050: [DMESG-FAIL][13] ([fdo#112176]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/fi-bsw-n3050/igt@i915_selftest@live_blt.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/fi-bsw-n3050/igt@i915_selftest@live_blt.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#105876]: https://bugs.freedesktop.org/show_bug.cgi?id=105876
[fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109964]: https://bugs.freedesktop.org/show_bug.cgi?id=109964
[fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
[fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
[fdo#112096]: https://bugs.freedesktop.org/show_bug.cgi?id=112096
[fdo#112147]: https://bugs.freedesktop.org/show_bug.cgi?id=112147
[fdo#112176]: https://bugs.freedesktop.org/show_bug.cgi?id=112176
[fdo#112298]: https://bugs.freedesktop.org/show_bug.cgi?id=112298
Participating hosts (49 -> 43)
------------------------------
Missing (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_7380 -> Patchwork_15335
CI-20190529: 20190529
CI_DRM_7380: 7b0a3189ead776ce02426186e8d63cff190a453e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5295: 9211e4794e40135d797e6d056d6d8d40076acb92 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_15335: 59aa67eff61389d8a898bbf74432bfaddbc44554 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
59aa67eff613 drm/msm: Don't init ww_mutec acquire ctx before needed
50bc03ff3da7 dma-resv: Also prime acquire ctx for lockdep
877e4edf1ff5 drm/modeset: Prime modeset lock vs dma_resv
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 2:07 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-20 2:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, DRI Development, Daniel Vetter,
Sean Paul, linux-arm-msm, freedreno
On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
seems reasonable.. but maybe a comment would be useful to prevent
future-me from "cleaning-this-up" back to the way it was
with that, r-b
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
only because I (or anyone else) hasn't had time to revisit the
struct_mutex usage since we moved to per-object-locks.. the downside,
I suppose, of kernel generally working ok and this not being a big
enough fire to bubble up to the top of my todo list
BR,
-R
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..126b2f62bfe7 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 2:07 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-20 2:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Sean Paul, Intel Graphics Development, DRI Development,
linux-arm-msm, Daniel Vetter, freedreno
On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
seems reasonable.. but maybe a comment would be useful to prevent
future-me from "cleaning-this-up" back to the way it was
with that, r-b
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
only because I (or anyone else) hasn't had time to revisit the
struct_mutex usage since we moved to per-object-locks.. the downside,
I suppose, of kernel generally working ok and this not being a big
enough fire to bubble up to the top of my todo list
BR,
-R
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..126b2f62bfe7 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> --
> 2.24.0
>
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 2:07 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-20 2:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Sean Paul, Intel Graphics Development, DRI Development,
linux-arm-msm, Daniel Vetter, freedreno
On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
seems reasonable.. but maybe a comment would be useful to prevent
future-me from "cleaning-this-up" back to the way it was
with that, r-b
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
only because I (or anyone else) hasn't had time to revisit the
struct_mutex usage since we moved to per-object-locks.. the downside,
I suppose, of kernel generally working ok and this not being a big
enough fire to bubble up to the top of my todo list
BR,
-R
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..126b2f62bfe7 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> --
> 2.24.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 2:07 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-20 2:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, DRI Development, linux-arm-msm,
Daniel Vetter, freedreno
On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
seems reasonable.. but maybe a comment would be useful to prevent
future-me from "cleaning-this-up" back to the way it was
with that, r-b
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
only because I (or anyone else) hasn't had time to revisit the
struct_mutex usage since we moved to per-object-locks.. the downside,
I suppose, of kernel generally working ok and this not being a big
enough fire to bubble up to the top of my todo list
BR,
-R
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..126b2f62bfe7 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> --
> 2.24.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-20 8:34 ` Christian König
0 siblings, 0 replies; 49+ messages in thread
From: Christian König @ 2019-11-20 8:34 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development, DRI Development; +Cc: Daniel Vetter
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);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-20 8:34 ` Christian König
0 siblings, 0 replies; 49+ messages in thread
From: Christian König @ 2019-11-20 8:34 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development, DRI Development; +Cc: Daniel Vetter
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);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-20 8:34 ` Christian König
0 siblings, 0 replies; 49+ messages in thread
From: Christian König @ 2019-11-20 8:34 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development, DRI Development; +Cc: Daniel Vetter
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);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 10:22 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:22 UTC (permalink / raw)
To: Rob Clark
Cc: Intel Graphics Development, DRI Development, Daniel Vetter,
Sean Paul, linux-arm-msm, freedreno
On Wed, Nov 20, 2019 at 3:07 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
>
> seems reasonable.. but maybe a comment would be useful to prevent
> future-me from "cleaning-this-up" back to the way it was
I'll add a comment.
> with that, r-b
Well I spotted a bug for the error handling, I'll need to respin.
-Daniel
>
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
>
> only because I (or anyone else) hasn't had time to revisit the
> struct_mutex usage since we moved to per-object-locks.. the downside,
> I suppose, of kernel generally working ok and this not being a big
> enough fire to bubble up to the top of my todo list
>
> BR,
> -R
>
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..126b2f62bfe7 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 10:22 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:22 UTC (permalink / raw)
To: Rob Clark
Cc: Sean Paul, Intel Graphics Development, DRI Development,
linux-arm-msm, Daniel Vetter, freedreno
On Wed, Nov 20, 2019 at 3:07 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
>
> seems reasonable.. but maybe a comment would be useful to prevent
> future-me from "cleaning-this-up" back to the way it was
I'll add a comment.
> with that, r-b
Well I spotted a bug for the error handling, I'll need to respin.
-Daniel
>
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
>
> only because I (or anyone else) hasn't had time to revisit the
> struct_mutex usage since we moved to per-object-locks.. the downside,
> I suppose, of kernel generally working ok and this not being a big
> enough fire to bubble up to the top of my todo list
>
> BR,
> -R
>
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..126b2f62bfe7 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 10:22 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:22 UTC (permalink / raw)
To: Rob Clark
Cc: Sean Paul, Intel Graphics Development, DRI Development,
linux-arm-msm, Daniel Vetter, freedreno
On Wed, Nov 20, 2019 at 3:07 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
>
> seems reasonable.. but maybe a comment would be useful to prevent
> future-me from "cleaning-this-up" back to the way it was
I'll add a comment.
> with that, r-b
Well I spotted a bug for the error handling, I'll need to respin.
-Daniel
>
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
>
> only because I (or anyone else) hasn't had time to revisit the
> struct_mutex usage since we moved to per-object-locks.. the downside,
> I suppose, of kernel generally working ok and this not being a big
> enough fire to bubble up to the top of my todo list
>
> BR,
> -R
>
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..126b2f62bfe7 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 10:22 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:22 UTC (permalink / raw)
To: Rob Clark
Cc: Intel Graphics Development, DRI Development, linux-arm-msm,
Daniel Vetter, freedreno
On Wed, Nov 20, 2019 at 3:07 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Nov 19, 2019 at 1:08 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
>
> seems reasonable.. but maybe a comment would be useful to prevent
> future-me from "cleaning-this-up" back to the way it was
I'll add a comment.
> with that, r-b
Well I spotted a bug for the error handling, I'll need to respin.
-Daniel
>
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
>
> only because I (or anyone else) hasn't had time to revisit the
> struct_mutex usage since we moved to per-object-locks.. the downside,
> I suppose, of kernel generally working ok and this not being a big
> enough fire to bubble up to the top of my todo list
>
> BR,
> -R
>
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..126b2f62bfe7 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -489,6 +488,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-20 10:55 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:55 UTC (permalink / raw)
To: Christian König
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Daniel Vetter
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);
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv
@ 2019-11-20 10:55 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:55 UTC (permalink / raw)
To: Christian König
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Daniel Vetter
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);
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
2019-11-19 21:08 ` Daniel Vetter
(?)
@ 2019-11-20 10:56 ` Daniel Vetter
-1 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:56 UTC (permalink / raw)
To: DRI Development
Cc: Intel Graphics Development, Daniel Vetter, Daniel Vetter,
Rob Clark, Sean Paul, linux-arm-msm, freedreno
For locking semantics it really doesn't matter when we grab the
ticket. But for lockdep validation it does: the acquire ctx is a fake
lockdep. Since other drivers might want to do a full multi-lock dance
in their fault-handler, not just lock a single dma_resv. Therefore we
must init the acquire_ctx only after we've done all the copy_*_user or
anything else that might trigger a pagefault. For msm this means we
need to move it past submit_lookup_objects.
Aside: Why is msm still using struct_mutex, it seems to be using
dma_resv_lock for general buffer state protection?
v2:
- Add comment to explain why the ww ticket setup is separate (Rob)
- Fix up error handling, we need to make sure we don't call
ww_acquire_fini without _init.
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index fb1fdd7fa3ef..385d4965a8d0 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
INIT_LIST_HEAD(&submit->node);
INIT_LIST_HEAD(&submit->bo_list);
- ww_acquire_init(&submit->ticket, &reservation_ww_class);
return submit;
}
@@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
list_del_init(&msm_obj->submit_entry);
drm_gem_object_put(&msm_obj->base);
}
-
- ww_acquire_fini(&submit->ticket);
}
int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
@@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
struct msm_ringbuffer *ring;
int out_fence_fd = -1;
struct pid *pid = get_pid(task_pid(current));
+ bool has_ww_ticket = false;
unsigned i;
int ret, submitid;
if (!gpu)
@@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
+ /* copy_*_user while holding a ww ticket upsets lockdep */
+ ww_acquire_init(&submit->ticket, &reservation_ww_class);
+ has_ww_ticket = true;
ret = submit_lock_objects(submit);
if (ret)
goto out;
@@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
out:
submit_cleanup(submit);
+ if (has_ww_ticket)
+ ww_acquire_fini(&submit->ticket);
if (ret)
msm_gem_submit_free(submit);
out_unlock:
--
2.24.0
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 10:56 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:56 UTC (permalink / raw)
To: DRI Development
Cc: freedreno, Daniel Vetter, Intel Graphics Development,
linux-arm-msm, Daniel Vetter, Sean Paul
For locking semantics it really doesn't matter when we grab the
ticket. But for lockdep validation it does: the acquire ctx is a fake
lockdep. Since other drivers might want to do a full multi-lock dance
in their fault-handler, not just lock a single dma_resv. Therefore we
must init the acquire_ctx only after we've done all the copy_*_user or
anything else that might trigger a pagefault. For msm this means we
need to move it past submit_lookup_objects.
Aside: Why is msm still using struct_mutex, it seems to be using
dma_resv_lock for general buffer state protection?
v2:
- Add comment to explain why the ww ticket setup is separate (Rob)
- Fix up error handling, we need to make sure we don't call
ww_acquire_fini without _init.
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index fb1fdd7fa3ef..385d4965a8d0 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
INIT_LIST_HEAD(&submit->node);
INIT_LIST_HEAD(&submit->bo_list);
- ww_acquire_init(&submit->ticket, &reservation_ww_class);
return submit;
}
@@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
list_del_init(&msm_obj->submit_entry);
drm_gem_object_put(&msm_obj->base);
}
-
- ww_acquire_fini(&submit->ticket);
}
int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
@@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
struct msm_ringbuffer *ring;
int out_fence_fd = -1;
struct pid *pid = get_pid(task_pid(current));
+ bool has_ww_ticket = false;
unsigned i;
int ret, submitid;
if (!gpu)
@@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
+ /* copy_*_user while holding a ww ticket upsets lockdep */
+ ww_acquire_init(&submit->ticket, &reservation_ww_class);
+ has_ww_ticket = true;
ret = submit_lock_objects(submit);
if (ret)
goto out;
@@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
out:
submit_cleanup(submit);
+ if (has_ww_ticket)
+ ww_acquire_fini(&submit->ticket);
if (ret)
msm_gem_submit_free(submit);
out_unlock:
--
2.24.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Intel-gfx] [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-20 10:56 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-20 10:56 UTC (permalink / raw)
To: DRI Development
Cc: freedreno, Daniel Vetter, Intel Graphics Development,
linux-arm-msm, Daniel Vetter
For locking semantics it really doesn't matter when we grab the
ticket. But for lockdep validation it does: the acquire ctx is a fake
lockdep. Since other drivers might want to do a full multi-lock dance
in their fault-handler, not just lock a single dma_resv. Therefore we
must init the acquire_ctx only after we've done all the copy_*_user or
anything else that might trigger a pagefault. For msm this means we
need to move it past submit_lookup_objects.
Aside: Why is msm still using struct_mutex, it seems to be using
dma_resv_lock for general buffer state protection?
v2:
- Add comment to explain why the ww ticket setup is separate (Rob)
- Fix up error handling, we need to make sure we don't call
ww_acquire_fini without _init.
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index fb1fdd7fa3ef..385d4965a8d0 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
INIT_LIST_HEAD(&submit->node);
INIT_LIST_HEAD(&submit->bo_list);
- ww_acquire_init(&submit->ticket, &reservation_ww_class);
return submit;
}
@@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
list_del_init(&msm_obj->submit_entry);
drm_gem_object_put(&msm_obj->base);
}
-
- ww_acquire_fini(&submit->ticket);
}
int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
@@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
struct msm_ringbuffer *ring;
int out_fence_fd = -1;
struct pid *pid = get_pid(task_pid(current));
+ bool has_ww_ticket = false;
unsigned i;
int ret, submitid;
if (!gpu)
@@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
+ /* copy_*_user while holding a ww ticket upsets lockdep */
+ ww_acquire_init(&submit->ticket, &reservation_ww_class);
+ has_ww_ticket = true;
ret = submit_lock_objects(submit);
if (ret)
goto out;
@@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
out:
submit_cleanup(submit);
+ if (has_ww_ticket)
+ ww_acquire_fini(&submit->ticket);
if (ret)
msm_gem_submit_free(submit);
out_unlock:
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
2019-11-19 21:08 ` Daniel Vetter
(?)
@ 2019-11-20 11:30 ` Christian König
-1 siblings, 0 replies; 49+ messages in thread
From: Christian König @ 2019-11-20 11:30 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development, DRI Development
Cc: Maarten Lankhorst, Chris Wilson, Sumit Semwal, linux-media,
linaro-mm-sig, Huang Rui, Eric Anholt, Ben Skeggs, Alex Deucher,
Rob Herring, Lucas Stach, Russell King, Christian Gmeiner,
Rob Clark, Sean Paul, Daniel Vetter
Am 19.11.19 um 22:08 schrieb Daniel Vetter:
> Semnatically it really doesn't matter where we grab the ticket. But
> since the ticket is a fake lockdep lock, it matters for lockdep
> validation purposes.
>
> This means stuff like grabbing a ticket and then doing
> copy_from/to_user isn't allowed anymore. This is a changed compared to
> the current ttm fault handler, which doesn't bother with having a full
> reservation. Since I'm looking into fixing the TODO entry in
> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
> later anyway, better get started. A bit more context on why I'm
> looking into this: For backwards compat with existing i915 gem code I
> think we'll have to do full slowpath locking in the i915 equivalent of
> the eviction code. And with dynamic dma-buf that will leak across
> drivers, so another thing we need to standardize and make sure it's
> done the same way everyway.
>
> Unfortunately this means another full audit of all drivers:
>
> - gem helpers: acquire_init is done right before taking locks, so no
> problem. Same for acquire_fini and unlocking, which means nothing
> that's not already covered by the dma_resv_lock rules will be caught
> with this extension here to the acquire_ctx.
>
> - etnaviv: An absolute massive amount of code is run between the
> acquire_init and the first lock acquisition in submit_lock_objects.
> But nothing that would touch user memory and could cause a fault.
> Furthermore nothing that uses the ticket, so even if I missed
> something, it would be easy to fix by pushing the acquire_init right
> before the first use. Similar on the unlock/acquire_fini side.
>
> - i915: Right now (and this will likely change a lot rsn) the acquire
> ctx and actual locks are right next to each another. No problem.
>
> - msm has a problem: submit_create calls acquire_init, but then
> submit_lookup_objects() has a bunch of copy_from_user to do the
> object lookups. That's the only thing before submit_lock_objects
> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
> does not have this issue since it copies all the userspace structs
> earlier. submit_cleanup does not have any such issues.
>
> With the prep patch to pull out the acquire_ctx and reorder it msm
> is going to be safe too.
>
> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
> Similar on the acquire_fini/ttm_bo_unreserve side.
>
> - ttm execbuf utils: acquire context and locking are even in the same
> functions here (one function to reserve everything, the other to
> unreserve), so all good.
>
> - vc4: Another case where acquire context and locking are handled in
> the same functions (one function to lock everything, the other to
> unlock).
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/dma-buf/dma-resv.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index d3c760e19991..079e38fde33a 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
> static void __init dma_resv_lockdep(void)
> {
> struct mm_struct *mm = mm_alloc();
> + struct ww_acquire_ctx ctx;
> struct dma_resv obj;
> + int ret;
>
> if (!mm)
> return;
> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
> dma_resv_init(&obj);
>
> down_read(&mm->mmap_sem);
> - ww_mutex_lock(&obj.lock, NULL);
> + ww_acquire_init(&ctx, &reservation_ww_class);
> + ret = dma_resv_lock(&obj, &ctx);
> + if (ret == -EDEADLK)
> + dma_resv_lock_slow(&obj, &ctx);
> fs_reclaim_acquire(GFP_KERNEL);
> fs_reclaim_release(GFP_KERNEL);
> ww_mutex_unlock(&obj.lock);
> + ww_acquire_fini(&ctx);
> up_read(&mm->mmap_sem);
>
> mmput(mm);
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-20 11:30 ` Christian König
0 siblings, 0 replies; 49+ messages in thread
From: Christian König @ 2019-11-20 11:30 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development, DRI Development
Cc: linaro-mm-sig, Huang Rui, Ben Skeggs, Russell King, Alex Deucher,
Daniel Vetter, Sean Paul, linux-media
Am 19.11.19 um 22:08 schrieb Daniel Vetter:
> Semnatically it really doesn't matter where we grab the ticket. But
> since the ticket is a fake lockdep lock, it matters for lockdep
> validation purposes.
>
> This means stuff like grabbing a ticket and then doing
> copy_from/to_user isn't allowed anymore. This is a changed compared to
> the current ttm fault handler, which doesn't bother with having a full
> reservation. Since I'm looking into fixing the TODO entry in
> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
> later anyway, better get started. A bit more context on why I'm
> looking into this: For backwards compat with existing i915 gem code I
> think we'll have to do full slowpath locking in the i915 equivalent of
> the eviction code. And with dynamic dma-buf that will leak across
> drivers, so another thing we need to standardize and make sure it's
> done the same way everyway.
>
> Unfortunately this means another full audit of all drivers:
>
> - gem helpers: acquire_init is done right before taking locks, so no
> problem. Same for acquire_fini and unlocking, which means nothing
> that's not already covered by the dma_resv_lock rules will be caught
> with this extension here to the acquire_ctx.
>
> - etnaviv: An absolute massive amount of code is run between the
> acquire_init and the first lock acquisition in submit_lock_objects.
> But nothing that would touch user memory and could cause a fault.
> Furthermore nothing that uses the ticket, so even if I missed
> something, it would be easy to fix by pushing the acquire_init right
> before the first use. Similar on the unlock/acquire_fini side.
>
> - i915: Right now (and this will likely change a lot rsn) the acquire
> ctx and actual locks are right next to each another. No problem.
>
> - msm has a problem: submit_create calls acquire_init, but then
> submit_lookup_objects() has a bunch of copy_from_user to do the
> object lookups. That's the only thing before submit_lock_objects
> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
> does not have this issue since it copies all the userspace structs
> earlier. submit_cleanup does not have any such issues.
>
> With the prep patch to pull out the acquire_ctx and reorder it msm
> is going to be safe too.
>
> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
> Similar on the acquire_fini/ttm_bo_unreserve side.
>
> - ttm execbuf utils: acquire context and locking are even in the same
> functions here (one function to reserve everything, the other to
> unreserve), so all good.
>
> - vc4: Another case where acquire context and locking are handled in
> the same functions (one function to lock everything, the other to
> unlock).
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/dma-buf/dma-resv.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index d3c760e19991..079e38fde33a 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
> static void __init dma_resv_lockdep(void)
> {
> struct mm_struct *mm = mm_alloc();
> + struct ww_acquire_ctx ctx;
> struct dma_resv obj;
> + int ret;
>
> if (!mm)
> return;
> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
> dma_resv_init(&obj);
>
> down_read(&mm->mmap_sem);
> - ww_mutex_lock(&obj.lock, NULL);
> + ww_acquire_init(&ctx, &reservation_ww_class);
> + ret = dma_resv_lock(&obj, &ctx);
> + if (ret == -EDEADLK)
> + dma_resv_lock_slow(&obj, &ctx);
> fs_reclaim_acquire(GFP_KERNEL);
> fs_reclaim_release(GFP_KERNEL);
> ww_mutex_unlock(&obj.lock);
> + ww_acquire_fini(&ctx);
> up_read(&mm->mmap_sem);
>
> mmput(mm);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-20 11:30 ` Christian König
0 siblings, 0 replies; 49+ messages in thread
From: Christian König @ 2019-11-20 11:30 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development, DRI Development
Cc: Rob Herring, linaro-mm-sig, Eric Anholt, Huang Rui, Ben Skeggs,
Russell King, Alex Deucher, Daniel Vetter, Lucas Stach,
Sumit Semwal, linux-media
Am 19.11.19 um 22:08 schrieb Daniel Vetter:
> Semnatically it really doesn't matter where we grab the ticket. But
> since the ticket is a fake lockdep lock, it matters for lockdep
> validation purposes.
>
> This means stuff like grabbing a ticket and then doing
> copy_from/to_user isn't allowed anymore. This is a changed compared to
> the current ttm fault handler, which doesn't bother with having a full
> reservation. Since I'm looking into fixing the TODO entry in
> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
> later anyway, better get started. A bit more context on why I'm
> looking into this: For backwards compat with existing i915 gem code I
> think we'll have to do full slowpath locking in the i915 equivalent of
> the eviction code. And with dynamic dma-buf that will leak across
> drivers, so another thing we need to standardize and make sure it's
> done the same way everyway.
>
> Unfortunately this means another full audit of all drivers:
>
> - gem helpers: acquire_init is done right before taking locks, so no
> problem. Same for acquire_fini and unlocking, which means nothing
> that's not already covered by the dma_resv_lock rules will be caught
> with this extension here to the acquire_ctx.
>
> - etnaviv: An absolute massive amount of code is run between the
> acquire_init and the first lock acquisition in submit_lock_objects.
> But nothing that would touch user memory and could cause a fault.
> Furthermore nothing that uses the ticket, so even if I missed
> something, it would be easy to fix by pushing the acquire_init right
> before the first use. Similar on the unlock/acquire_fini side.
>
> - i915: Right now (and this will likely change a lot rsn) the acquire
> ctx and actual locks are right next to each another. No problem.
>
> - msm has a problem: submit_create calls acquire_init, but then
> submit_lookup_objects() has a bunch of copy_from_user to do the
> object lookups. That's the only thing before submit_lock_objects
> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
> does not have this issue since it copies all the userspace structs
> earlier. submit_cleanup does not have any such issues.
>
> With the prep patch to pull out the acquire_ctx and reorder it msm
> is going to be safe too.
>
> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
> Similar on the acquire_fini/ttm_bo_unreserve side.
>
> - ttm execbuf utils: acquire context and locking are even in the same
> functions here (one function to reserve everything, the other to
> unreserve), so all good.
>
> - vc4: Another case where acquire context and locking are handled in
> the same functions (one function to lock everything, the other to
> unlock).
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/dma-buf/dma-resv.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index d3c760e19991..079e38fde33a 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
> static void __init dma_resv_lockdep(void)
> {
> struct mm_struct *mm = mm_alloc();
> + struct ww_acquire_ctx ctx;
> struct dma_resv obj;
> + int ret;
>
> if (!mm)
> return;
> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
> dma_resv_init(&obj);
>
> down_read(&mm->mmap_sem);
> - ww_mutex_lock(&obj.lock, NULL);
> + ww_acquire_init(&ctx, &reservation_ww_class);
> + ret = dma_resv_lock(&obj, &ctx);
> + if (ret == -EDEADLK)
> + dma_resv_lock_slow(&obj, &ctx);
> fs_reclaim_acquire(GFP_KERNEL);
> fs_reclaim_release(GFP_KERNEL);
> ww_mutex_unlock(&obj.lock);
> + ww_acquire_fini(&ctx);
> up_read(&mm->mmap_sem);
>
> mmput(mm);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* ✓ Fi.CI.IGT: success for more dma-buf lockdep priming
@ 2019-11-20 13:55 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-20 13:55 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming
URL : https://patchwork.freedesktop.org/series/69695/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7380_full -> Patchwork_15335_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_15335_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_isolation@rcs0-s3:
- shard-kbl: [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +5 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-kbl1/igt@gem_ctx_isolation@rcs0-s3.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-kbl6/igt@gem_ctx_isolation@rcs0-s3.html
* igt@gem_ctx_persistence@vcs1-mixed-process:
- shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#109276] / [fdo#112080]) +4 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb2/igt@gem_ctx_persistence@vcs1-mixed-process.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb3/igt@gem_ctx_persistence@vcs1-mixed-process.html
* igt@gem_eio@hibernate:
- shard-tglb: [PASS][5] -> [INCOMPLETE][6] ([fdo#111832])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb9/igt@gem_eio@hibernate.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb3/igt@gem_eio@hibernate.html
* igt@gem_eio@in-flight-suspend:
- shard-tglb: [PASS][7] -> [INCOMPLETE][8] ([fdo#111832] / [fdo#111850] / [fdo#112081])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb6/igt@gem_eio@in-flight-suspend.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb8/igt@gem_eio@in-flight-suspend.html
* igt@gem_exec_schedule@in-order-bsd:
- shard-iclb: [PASS][9] -> [SKIP][10] ([fdo#112146]) +5 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@gem_exec_schedule@in-order-bsd.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@gem_exec_schedule@in-order-bsd.html
* igt@gem_exec_schedule@preempt-queue-bsd1:
- shard-iclb: [PASS][11] -> [SKIP][12] ([fdo#109276]) +20 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd1.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb6/igt@gem_exec_schedule@preempt-queue-bsd1.html
* igt@gem_exec_schedule@wide-render:
- shard-iclb: [PASS][13] -> [INCOMPLETE][14] ([fdo#107713] / [fdo#110338 ])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb6/igt@gem_exec_schedule@wide-render.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb7/igt@gem_exec_schedule@wide-render.html
* igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
- shard-hsw: [PASS][15] -> [DMESG-WARN][16] ([fdo#111870]) +2 similar issues
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw4/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw6/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
* igt@gem_userptr_blits@sync-unmap-cycles:
- shard-snb: [PASS][17] -> [DMESG-WARN][18] ([fdo#111870]) +1 similar issue
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-snb7/igt@gem_userptr_blits@sync-unmap-cycles.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-snb1/igt@gem_userptr_blits@sync-unmap-cycles.html
* igt@i915_pm_dc@dc5-dpms:
- shard-iclb: [PASS][19] -> [FAIL][20] ([fdo#111795 ])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb1/igt@i915_pm_dc@dc5-dpms.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb3/igt@i915_pm_dc@dc5-dpms.html
* igt@i915_selftest@live_execlists:
- shard-kbl: [PASS][21] -> [INCOMPLETE][22] ([fdo#103665] / [fdo#112259])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-kbl4/igt@i915_selftest@live_execlists.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-kbl1/igt@i915_selftest@live_execlists.html
* igt@i915_selftest@live_hangcheck:
- shard-hsw: [PASS][23] -> [DMESG-FAIL][24] ([fdo#111991])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw2/igt@i915_selftest@live_hangcheck.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw1/igt@i915_selftest@live_hangcheck.html
* igt@i915_suspend@sysfs-reader:
- shard-tglb: [PASS][25] -> [INCOMPLETE][26] ([fdo#111832] / [fdo#111850]) +1 similar issue
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb9/igt@i915_suspend@sysfs-reader.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb4/igt@i915_suspend@sysfs-reader.html
* igt@kms_color@pipe-b-ctm-blue-to-red:
- shard-skl: [PASS][27] -> [DMESG-WARN][28] ([fdo#106107])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl2/igt@kms_color@pipe-b-ctm-blue-to-red.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl9/igt@kms_color@pipe-b-ctm-blue-to-red.html
* igt@kms_cursor_crc@pipe-c-cursor-suspend:
- shard-apl: [PASS][29] -> [DMESG-WARN][30] ([fdo#108566])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-apl4/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-apl4/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
- shard-iclb: [PASS][31] -> [FAIL][32] ([fdo#103167]) +6 similar issues
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-rte:
- shard-tglb: [PASS][33] -> [FAIL][34] ([fdo#103167]) +2 similar issues
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- shard-skl: [PASS][35] -> [INCOMPLETE][36] ([fdo#104108])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl5/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: [PASS][37] -> [FAIL][38] ([fdo#108145]) +1 similar issue
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: [PASS][39] -> [FAIL][40] ([fdo#108145] / [fdo#110403])
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
* igt@kms_psr@no_drrs:
- shard-iclb: [PASS][41] -> [FAIL][42] ([fdo#108341])
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@kms_psr@no_drrs.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_sprite_blt:
- shard-iclb: [PASS][43] -> [SKIP][44] ([fdo#109441]) +2 similar issues
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb8/igt@kms_psr@psr2_sprite_blt.html
* igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
- shard-glk: [PASS][45] -> [DMESG-FAIL][46] ([fdo#105763] / [fdo#106538])
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-glk7/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-glk1/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html
* igt@kms_rotation_crc@sprite-rotation-90-pos-100-0:
- shard-iclb: [PASS][47] -> [INCOMPLETE][48] ([fdo#107713] / [fdo#110026])
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb6/igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb7/igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.html
* igt@kms_setmode@basic:
- shard-apl: [PASS][49] -> [FAIL][50] ([fdo#99912])
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-apl7/igt@kms_setmode@basic.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-apl2/igt@kms_setmode@basic.html
* igt@perf_pmu@busy-accuracy-98-vcs1:
- shard-iclb: [PASS][51] -> [SKIP][52] ([fdo#112080]) +11 similar issues
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb4/igt@perf_pmu@busy-accuracy-98-vcs1.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb6/igt@perf_pmu@busy-accuracy-98-vcs1.html
#### Possible fixes ####
* igt@gem_ctx_isolation@vecs0-s3:
- shard-tglb: [INCOMPLETE][53] ([fdo#111832]) -> [PASS][54]
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb4/igt@gem_ctx_isolation@vecs0-s3.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb3/igt@gem_ctx_isolation@vecs0-s3.html
* igt@gem_ctx_switch@vcs1:
- shard-iclb: [SKIP][55] ([fdo#112080]) -> [PASS][56] +13 similar issues
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@gem_ctx_switch@vcs1.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@gem_ctx_switch@vcs1.html
* igt@gem_eio@suspend:
- shard-tglb: [INCOMPLETE][57] ([fdo#111850]) -> [PASS][58] +2 similar issues
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb3/igt@gem_eio@suspend.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb1/igt@gem_eio@suspend.html
* igt@gem_exec_async@concurrent-writes-bsd:
- shard-iclb: [SKIP][59] ([fdo#112146]) -> [PASS][60] +5 similar issues
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb2/igt@gem_exec_async@concurrent-writes-bsd.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb3/igt@gem_exec_async@concurrent-writes-bsd.html
* igt@gem_exec_suspend@basic-s3:
- shard-tglb: [INCOMPLETE][61] ([fdo#111736] / [fdo#111850]) -> [PASS][62]
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb4/igt@gem_exec_suspend@basic-s3.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb2/igt@gem_exec_suspend@basic-s3.html
* igt@gem_softpin@evict-active-interruptible:
- shard-tglb: [TIMEOUT][63] ([fdo#112126]) -> [PASS][64]
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb1/igt@gem_softpin@evict-active-interruptible.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@gem_softpin@evict-active-interruptible.html
* igt@gem_userptr_blits@dmabuf-unsync:
- shard-hsw: [DMESG-WARN][65] ([fdo#111870]) -> [PASS][66]
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw8/igt@gem_userptr_blits@dmabuf-unsync.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw4/igt@gem_userptr_blits@dmabuf-unsync.html
* igt@kms_cursor_crc@pipe-a-cursor-suspend:
- shard-skl: [INCOMPLETE][67] ([fdo#110741]) -> [PASS][68]
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl10/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl5/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
* igt@kms_cursor_crc@pipe-b-cursor-suspend:
- shard-apl: [DMESG-WARN][69] ([fdo#108566]) -> [PASS][70] +4 similar issues
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-apl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-apl7/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
* igt@kms_cursor_edge_walk@pipe-a-128x128-right-edge:
- shard-tglb: [INCOMPLETE][71] -> [PASS][72]
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb1/igt@kms_cursor_edge_walk@pipe-a-128x128-right-edge.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@kms_cursor_edge_walk@pipe-a-128x128-right-edge.html
* igt@kms_cursor_legacy@cursora-vs-flipa-atomic:
- shard-skl: [DMESG-WARN][73] ([fdo#106107]) -> [PASS][74]
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl5/igt@kms_cursor_legacy@cursora-vs-flipa-atomic.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl4/igt@kms_cursor_legacy@cursora-vs-flipa-atomic.html
* igt@kms_flip@2x-flip-vs-suspend:
- shard-hsw: [INCOMPLETE][75] ([fdo#103540]) -> [PASS][76]
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw8/igt@kms_flip@2x-flip-vs-suspend.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw2/igt@kms_flip@2x-flip-vs-suspend.html
* igt@kms_flip@flip-vs-expired-vblank:
- shard-skl: [FAIL][77] ([fdo#105363]) -> [PASS][78]
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl6/igt@kms_flip@flip-vs-expired-vblank.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl3/igt@kms_flip@flip-vs-expired-vblank.html
* igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt:
- shard-iclb: [FAIL][79] ([fdo#103167]) -> [PASS][80] +3 similar issues
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt.html
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt.html
* igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
- shard-tglb: [FAIL][81] ([fdo#103167]) -> [PASS][82]
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb7/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb9/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc:
- shard-tglb: [TIMEOUT][83] ([fdo#112168]) -> [PASS][84]
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc.html
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc.html
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: [FAIL][85] ([fdo#108145] / [fdo#110403]) -> [PASS][86]
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
* igt@kms_plane_lowres@pipe-a-tiling-x:
- shard-iclb: [FAIL][87] ([fdo#103166]) -> [PASS][88]
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html
* igt@kms_psr@psr2_primary_mmap_cpu:
- shard-iclb: [SKIP][89] ([fdo#109441]) -> [PASS][90] +2 similar issues
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb7/igt@kms_psr@psr2_primary_mmap_cpu.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
* igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
- shard-tglb: [INCOMPLETE][91] ([fdo#111832] / [fdo#111850]) -> [PASS][92] +2 similar issues
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb4/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-kbl: [DMESG-WARN][93] ([fdo#108566]) -> [PASS][94] +2 similar issues
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-kbl4/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
* igt@prime_vgem@fence-wait-bsd2:
- shard-iclb: [SKIP][95] ([fdo#109276]) -> [PASS][96] +14 similar issues
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@prime_vgem@fence-wait-bsd2.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@prime_vgem@fence-wait-bsd2.html
#### Warnings ####
* igt@kms_atomic_transition@6x-modeset-transitions-nonblocking:
- shard-tglb: [SKIP][97] ([fdo#112016 ] / [fdo#112021 ]) -> [SKIP][98] ([fdo#112021 ])
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb2/igt@kms_atomic_transition@6x-modeset-transitions-nonblocking.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb9/igt@kms_atomic_transition@6x-modeset-transitions-nonblocking.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
[fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#110026]: https://bugs.freedesktop.org/show_bug.cgi?id=110026
[fdo#110338 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110338
[fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
[fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741
[fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
[fdo#111795 ]: https://bugs.freedesktop.org/show_bu
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for more dma-buf lockdep priming
@ 2019-11-20 13:55 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-20 13:55 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming
URL : https://patchwork.freedesktop.org/series/69695/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7380_full -> Patchwork_15335_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_15335_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_isolation@rcs0-s3:
- shard-kbl: [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +5 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-kbl1/igt@gem_ctx_isolation@rcs0-s3.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-kbl6/igt@gem_ctx_isolation@rcs0-s3.html
* igt@gem_ctx_persistence@vcs1-mixed-process:
- shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#109276] / [fdo#112080]) +4 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb2/igt@gem_ctx_persistence@vcs1-mixed-process.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb3/igt@gem_ctx_persistence@vcs1-mixed-process.html
* igt@gem_eio@hibernate:
- shard-tglb: [PASS][5] -> [INCOMPLETE][6] ([fdo#111832])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb9/igt@gem_eio@hibernate.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb3/igt@gem_eio@hibernate.html
* igt@gem_eio@in-flight-suspend:
- shard-tglb: [PASS][7] -> [INCOMPLETE][8] ([fdo#111832] / [fdo#111850] / [fdo#112081])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb6/igt@gem_eio@in-flight-suspend.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb8/igt@gem_eio@in-flight-suspend.html
* igt@gem_exec_schedule@in-order-bsd:
- shard-iclb: [PASS][9] -> [SKIP][10] ([fdo#112146]) +5 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@gem_exec_schedule@in-order-bsd.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@gem_exec_schedule@in-order-bsd.html
* igt@gem_exec_schedule@preempt-queue-bsd1:
- shard-iclb: [PASS][11] -> [SKIP][12] ([fdo#109276]) +20 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd1.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb6/igt@gem_exec_schedule@preempt-queue-bsd1.html
* igt@gem_exec_schedule@wide-render:
- shard-iclb: [PASS][13] -> [INCOMPLETE][14] ([fdo#107713] / [fdo#110338 ])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb6/igt@gem_exec_schedule@wide-render.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb7/igt@gem_exec_schedule@wide-render.html
* igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
- shard-hsw: [PASS][15] -> [DMESG-WARN][16] ([fdo#111870]) +2 similar issues
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw4/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw6/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
* igt@gem_userptr_blits@sync-unmap-cycles:
- shard-snb: [PASS][17] -> [DMESG-WARN][18] ([fdo#111870]) +1 similar issue
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-snb7/igt@gem_userptr_blits@sync-unmap-cycles.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-snb1/igt@gem_userptr_blits@sync-unmap-cycles.html
* igt@i915_pm_dc@dc5-dpms:
- shard-iclb: [PASS][19] -> [FAIL][20] ([fdo#111795 ])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb1/igt@i915_pm_dc@dc5-dpms.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb3/igt@i915_pm_dc@dc5-dpms.html
* igt@i915_selftest@live_execlists:
- shard-kbl: [PASS][21] -> [INCOMPLETE][22] ([fdo#103665] / [fdo#112259])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-kbl4/igt@i915_selftest@live_execlists.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-kbl1/igt@i915_selftest@live_execlists.html
* igt@i915_selftest@live_hangcheck:
- shard-hsw: [PASS][23] -> [DMESG-FAIL][24] ([fdo#111991])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw2/igt@i915_selftest@live_hangcheck.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw1/igt@i915_selftest@live_hangcheck.html
* igt@i915_suspend@sysfs-reader:
- shard-tglb: [PASS][25] -> [INCOMPLETE][26] ([fdo#111832] / [fdo#111850]) +1 similar issue
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb9/igt@i915_suspend@sysfs-reader.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb4/igt@i915_suspend@sysfs-reader.html
* igt@kms_color@pipe-b-ctm-blue-to-red:
- shard-skl: [PASS][27] -> [DMESG-WARN][28] ([fdo#106107])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl2/igt@kms_color@pipe-b-ctm-blue-to-red.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl9/igt@kms_color@pipe-b-ctm-blue-to-red.html
* igt@kms_cursor_crc@pipe-c-cursor-suspend:
- shard-apl: [PASS][29] -> [DMESG-WARN][30] ([fdo#108566])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-apl4/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-apl4/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
- shard-iclb: [PASS][31] -> [FAIL][32] ([fdo#103167]) +6 similar issues
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-rte:
- shard-tglb: [PASS][33] -> [FAIL][34] ([fdo#103167]) +2 similar issues
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- shard-skl: [PASS][35] -> [INCOMPLETE][36] ([fdo#104108])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl5/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: [PASS][37] -> [FAIL][38] ([fdo#108145]) +1 similar issue
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: [PASS][39] -> [FAIL][40] ([fdo#108145] / [fdo#110403])
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
* igt@kms_psr@no_drrs:
- shard-iclb: [PASS][41] -> [FAIL][42] ([fdo#108341])
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@kms_psr@no_drrs.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_sprite_blt:
- shard-iclb: [PASS][43] -> [SKIP][44] ([fdo#109441]) +2 similar issues
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb8/igt@kms_psr@psr2_sprite_blt.html
* igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
- shard-glk: [PASS][45] -> [DMESG-FAIL][46] ([fdo#105763] / [fdo#106538])
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-glk7/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-glk1/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html
* igt@kms_rotation_crc@sprite-rotation-90-pos-100-0:
- shard-iclb: [PASS][47] -> [INCOMPLETE][48] ([fdo#107713] / [fdo#110026])
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb6/igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb7/igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.html
* igt@kms_setmode@basic:
- shard-apl: [PASS][49] -> [FAIL][50] ([fdo#99912])
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-apl7/igt@kms_setmode@basic.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-apl2/igt@kms_setmode@basic.html
* igt@perf_pmu@busy-accuracy-98-vcs1:
- shard-iclb: [PASS][51] -> [SKIP][52] ([fdo#112080]) +11 similar issues
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb4/igt@perf_pmu@busy-accuracy-98-vcs1.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb6/igt@perf_pmu@busy-accuracy-98-vcs1.html
#### Possible fixes ####
* igt@gem_ctx_isolation@vecs0-s3:
- shard-tglb: [INCOMPLETE][53] ([fdo#111832]) -> [PASS][54]
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb4/igt@gem_ctx_isolation@vecs0-s3.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb3/igt@gem_ctx_isolation@vecs0-s3.html
* igt@gem_ctx_switch@vcs1:
- shard-iclb: [SKIP][55] ([fdo#112080]) -> [PASS][56] +13 similar issues
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@gem_ctx_switch@vcs1.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@gem_ctx_switch@vcs1.html
* igt@gem_eio@suspend:
- shard-tglb: [INCOMPLETE][57] ([fdo#111850]) -> [PASS][58] +2 similar issues
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb3/igt@gem_eio@suspend.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb1/igt@gem_eio@suspend.html
* igt@gem_exec_async@concurrent-writes-bsd:
- shard-iclb: [SKIP][59] ([fdo#112146]) -> [PASS][60] +5 similar issues
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb2/igt@gem_exec_async@concurrent-writes-bsd.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb3/igt@gem_exec_async@concurrent-writes-bsd.html
* igt@gem_exec_suspend@basic-s3:
- shard-tglb: [INCOMPLETE][61] ([fdo#111736] / [fdo#111850]) -> [PASS][62]
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb4/igt@gem_exec_suspend@basic-s3.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb2/igt@gem_exec_suspend@basic-s3.html
* igt@gem_softpin@evict-active-interruptible:
- shard-tglb: [TIMEOUT][63] ([fdo#112126]) -> [PASS][64]
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb1/igt@gem_softpin@evict-active-interruptible.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@gem_softpin@evict-active-interruptible.html
* igt@gem_userptr_blits@dmabuf-unsync:
- shard-hsw: [DMESG-WARN][65] ([fdo#111870]) -> [PASS][66]
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw8/igt@gem_userptr_blits@dmabuf-unsync.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw4/igt@gem_userptr_blits@dmabuf-unsync.html
* igt@kms_cursor_crc@pipe-a-cursor-suspend:
- shard-skl: [INCOMPLETE][67] ([fdo#110741]) -> [PASS][68]
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl10/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl5/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
* igt@kms_cursor_crc@pipe-b-cursor-suspend:
- shard-apl: [DMESG-WARN][69] ([fdo#108566]) -> [PASS][70] +4 similar issues
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-apl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-apl7/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
* igt@kms_cursor_edge_walk@pipe-a-128x128-right-edge:
- shard-tglb: [INCOMPLETE][71] -> [PASS][72]
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb1/igt@kms_cursor_edge_walk@pipe-a-128x128-right-edge.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@kms_cursor_edge_walk@pipe-a-128x128-right-edge.html
* igt@kms_cursor_legacy@cursora-vs-flipa-atomic:
- shard-skl: [DMESG-WARN][73] ([fdo#106107]) -> [PASS][74]
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl5/igt@kms_cursor_legacy@cursora-vs-flipa-atomic.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl4/igt@kms_cursor_legacy@cursora-vs-flipa-atomic.html
* igt@kms_flip@2x-flip-vs-suspend:
- shard-hsw: [INCOMPLETE][75] ([fdo#103540]) -> [PASS][76]
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-hsw8/igt@kms_flip@2x-flip-vs-suspend.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-hsw2/igt@kms_flip@2x-flip-vs-suspend.html
* igt@kms_flip@flip-vs-expired-vblank:
- shard-skl: [FAIL][77] ([fdo#105363]) -> [PASS][78]
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl6/igt@kms_flip@flip-vs-expired-vblank.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl3/igt@kms_flip@flip-vs-expired-vblank.html
* igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt:
- shard-iclb: [FAIL][79] ([fdo#103167]) -> [PASS][80] +3 similar issues
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt.html
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt.html
* igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
- shard-tglb: [FAIL][81] ([fdo#103167]) -> [PASS][82]
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb7/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb9/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc:
- shard-tglb: [TIMEOUT][83] ([fdo#112168]) -> [PASS][84]
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc.html
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc.html
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: [FAIL][85] ([fdo#108145] / [fdo#110403]) -> [PASS][86]
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
* igt@kms_plane_lowres@pipe-a-tiling-x:
- shard-iclb: [FAIL][87] ([fdo#103166]) -> [PASS][88]
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html
* igt@kms_psr@psr2_primary_mmap_cpu:
- shard-iclb: [SKIP][89] ([fdo#109441]) -> [PASS][90] +2 similar issues
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb7/igt@kms_psr@psr2_primary_mmap_cpu.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
* igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
- shard-tglb: [INCOMPLETE][91] ([fdo#111832] / [fdo#111850]) -> [PASS][92] +2 similar issues
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb4/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb5/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-kbl: [DMESG-WARN][93] ([fdo#108566]) -> [PASS][94] +2 similar issues
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-kbl4/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
* igt@prime_vgem@fence-wait-bsd2:
- shard-iclb: [SKIP][95] ([fdo#109276]) -> [PASS][96] +14 similar issues
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-iclb3/igt@prime_vgem@fence-wait-bsd2.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-iclb1/igt@prime_vgem@fence-wait-bsd2.html
#### Warnings ####
* igt@kms_atomic_transition@6x-modeset-transitions-nonblocking:
- shard-tglb: [SKIP][97] ([fdo#112016 ] / [fdo#112021 ]) -> [SKIP][98] ([fdo#112021 ])
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7380/shard-tglb2/igt@kms_atomic_transition@6x-modeset-transitions-nonblocking.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/shard-tglb9/igt@kms_atomic_transition@6x-modeset-transitions-nonblocking.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
[fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#110026]: https://bugs.freedesktop.org/show_bug.cgi?id=110026
[fdo#110338 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110338
[fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
[fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741
[fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
[fdo#111795 ]: https://bugs.freedesktop.org/show_bu
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15335/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
2019-11-20 11:30 ` Christian König
(?)
@ 2019-11-20 13:56 ` Maarten Lankhorst
-1 siblings, 0 replies; 49+ messages in thread
From: Maarten Lankhorst @ 2019-11-20 13:56 UTC (permalink / raw)
To: Christian König, Daniel Vetter, Intel Graphics Development,
DRI Development
Cc: Chris Wilson, Sumit Semwal, linux-media, linaro-mm-sig,
Huang Rui, Eric Anholt, Ben Skeggs, Alex Deucher, Rob Herring,
Lucas Stach, Russell King, Christian Gmeiner, Rob Clark,
Sean Paul, Daniel Vetter
Op 20-11-2019 om 12:30 schreef Christian König:
> Am 19.11.19 um 22:08 schrieb Daniel Vetter:
>> Semnatically it really doesn't matter where we grab the ticket. But
>> since the ticket is a fake lockdep lock, it matters for lockdep
>> validation purposes.
>>
>> This means stuff like grabbing a ticket and then doing
>> copy_from/to_user isn't allowed anymore. This is a changed compared to
>> the current ttm fault handler, which doesn't bother with having a full
>> reservation. Since I'm looking into fixing the TODO entry in
>> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
>> later anyway, better get started. A bit more context on why I'm
>> looking into this: For backwards compat with existing i915 gem code I
>> think we'll have to do full slowpath locking in the i915 equivalent of
>> the eviction code. And with dynamic dma-buf that will leak across
>> drivers, so another thing we need to standardize and make sure it's
>> done the same way everyway.
>>
>> Unfortunately this means another full audit of all drivers:
>>
>> - gem helpers: acquire_init is done right before taking locks, so no
>> problem. Same for acquire_fini and unlocking, which means nothing
>> that's not already covered by the dma_resv_lock rules will be caught
>> with this extension here to the acquire_ctx.
>>
>> - etnaviv: An absolute massive amount of code is run between the
>> acquire_init and the first lock acquisition in submit_lock_objects.
>> But nothing that would touch user memory and could cause a fault.
>> Furthermore nothing that uses the ticket, so even if I missed
>> something, it would be easy to fix by pushing the acquire_init right
>> before the first use. Similar on the unlock/acquire_fini side.
>>
>> - i915: Right now (and this will likely change a lot rsn) the acquire
>> ctx and actual locks are right next to each another. No problem.
>>
>> - msm has a problem: submit_create calls acquire_init, but then
>> submit_lookup_objects() has a bunch of copy_from_user to do the
>> object lookups. That's the only thing before submit_lock_objects
>> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
>> does not have this issue since it copies all the userspace structs
>> earlier. submit_cleanup does not have any such issues.
>>
>> With the prep patch to pull out the acquire_ctx and reorder it msm
>> is going to be safe too.
>>
>> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
>> Similar on the acquire_fini/ttm_bo_unreserve side.
>>
>> - ttm execbuf utils: acquire context and locking are even in the same
>> functions here (one function to reserve everything, the other to
>> unreserve), so all good.
>>
>> - vc4: Another case where acquire context and locking are handled in
>> the same functions (one function to lock everything, the other to
>> unlock).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: linux-media@vger.kernel.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Cc: Huang Rui <ray.huang@amd.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Sean Paul <sean@poorly.run>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Acked-by: Christian König <christian.koenig@amd.com>
>
>> ---
>> drivers/dma-buf/dma-resv.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>> index d3c760e19991..079e38fde33a 100644
>> --- a/drivers/dma-buf/dma-resv.c
>> +++ b/drivers/dma-buf/dma-resv.c
>> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
>> static void __init dma_resv_lockdep(void)
>> {
>> struct mm_struct *mm = mm_alloc();
>> + struct ww_acquire_ctx ctx;
>> struct dma_resv obj;
>> + int ret;
>> if (!mm)
>> return;
>> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
>> dma_resv_init(&obj);
>> down_read(&mm->mmap_sem);
>> - ww_mutex_lock(&obj.lock, NULL);
>> + ww_acquire_init(&ctx, &reservation_ww_class);
>> + ret = dma_resv_lock(&obj, &ctx);
>> + if (ret == -EDEADLK)
>> + dma_resv_lock_slow(&obj, &ctx);
>> fs_reclaim_acquire(GFP_KERNEL);
>> fs_reclaim_release(GFP_KERNEL);
>> ww_mutex_unlock(&obj.lock);
>> + ww_acquire_fini(&ctx);
>> up_read(&mm->mmap_sem);
>>
>> mmput(mm);
>
For whole series:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
typo in patch 3 btw :)
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-20 13:56 ` Maarten Lankhorst
0 siblings, 0 replies; 49+ messages in thread
From: Maarten Lankhorst @ 2019-11-20 13:56 UTC (permalink / raw)
To: Christian König, Daniel Vetter, Intel Graphics Development,
DRI Development
Cc: linaro-mm-sig, Huang Rui, Ben Skeggs, Russell King, Alex Deucher,
Daniel Vetter, Sean Paul, linux-media
Op 20-11-2019 om 12:30 schreef Christian König:
> Am 19.11.19 um 22:08 schrieb Daniel Vetter:
>> Semnatically it really doesn't matter where we grab the ticket. But
>> since the ticket is a fake lockdep lock, it matters for lockdep
>> validation purposes.
>>
>> This means stuff like grabbing a ticket and then doing
>> copy_from/to_user isn't allowed anymore. This is a changed compared to
>> the current ttm fault handler, which doesn't bother with having a full
>> reservation. Since I'm looking into fixing the TODO entry in
>> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
>> later anyway, better get started. A bit more context on why I'm
>> looking into this: For backwards compat with existing i915 gem code I
>> think we'll have to do full slowpath locking in the i915 equivalent of
>> the eviction code. And with dynamic dma-buf that will leak across
>> drivers, so another thing we need to standardize and make sure it's
>> done the same way everyway.
>>
>> Unfortunately this means another full audit of all drivers:
>>
>> - gem helpers: acquire_init is done right before taking locks, so no
>> problem. Same for acquire_fini and unlocking, which means nothing
>> that's not already covered by the dma_resv_lock rules will be caught
>> with this extension here to the acquire_ctx.
>>
>> - etnaviv: An absolute massive amount of code is run between the
>> acquire_init and the first lock acquisition in submit_lock_objects.
>> But nothing that would touch user memory and could cause a fault.
>> Furthermore nothing that uses the ticket, so even if I missed
>> something, it would be easy to fix by pushing the acquire_init right
>> before the first use. Similar on the unlock/acquire_fini side.
>>
>> - i915: Right now (and this will likely change a lot rsn) the acquire
>> ctx and actual locks are right next to each another. No problem.
>>
>> - msm has a problem: submit_create calls acquire_init, but then
>> submit_lookup_objects() has a bunch of copy_from_user to do the
>> object lookups. That's the only thing before submit_lock_objects
>> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
>> does not have this issue since it copies all the userspace structs
>> earlier. submit_cleanup does not have any such issues.
>>
>> With the prep patch to pull out the acquire_ctx and reorder it msm
>> is going to be safe too.
>>
>> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
>> Similar on the acquire_fini/ttm_bo_unreserve side.
>>
>> - ttm execbuf utils: acquire context and locking are even in the same
>> functions here (one function to reserve everything, the other to
>> unreserve), so all good.
>>
>> - vc4: Another case where acquire context and locking are handled in
>> the same functions (one function to lock everything, the other to
>> unlock).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: linux-media@vger.kernel.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Cc: Huang Rui <ray.huang@amd.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Sean Paul <sean@poorly.run>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Acked-by: Christian König <christian.koenig@amd.com>
>
>> ---
>> drivers/dma-buf/dma-resv.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>> index d3c760e19991..079e38fde33a 100644
>> --- a/drivers/dma-buf/dma-resv.c
>> +++ b/drivers/dma-buf/dma-resv.c
>> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
>> static void __init dma_resv_lockdep(void)
>> {
>> struct mm_struct *mm = mm_alloc();
>> + struct ww_acquire_ctx ctx;
>> struct dma_resv obj;
>> + int ret;
>> if (!mm)
>> return;
>> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
>> dma_resv_init(&obj);
>> down_read(&mm->mmap_sem);
>> - ww_mutex_lock(&obj.lock, NULL);
>> + ww_acquire_init(&ctx, &reservation_ww_class);
>> + ret = dma_resv_lock(&obj, &ctx);
>> + if (ret == -EDEADLK)
>> + dma_resv_lock_slow(&obj, &ctx);
>> fs_reclaim_acquire(GFP_KERNEL);
>> fs_reclaim_release(GFP_KERNEL);
>> ww_mutex_unlock(&obj.lock);
>> + ww_acquire_fini(&ctx);
>> up_read(&mm->mmap_sem);
>>
>> mmput(mm);
>
For whole series:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
typo in patch 3 btw :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep
@ 2019-11-20 13:56 ` Maarten Lankhorst
0 siblings, 0 replies; 49+ messages in thread
From: Maarten Lankhorst @ 2019-11-20 13:56 UTC (permalink / raw)
To: Christian König, Daniel Vetter, Intel Graphics Development,
DRI Development
Cc: Rob Herring, linaro-mm-sig, Eric Anholt, Huang Rui, Ben Skeggs,
Russell King, Alex Deucher, Daniel Vetter, Lucas Stach,
Sumit Semwal, linux-media
Op 20-11-2019 om 12:30 schreef Christian König:
> Am 19.11.19 um 22:08 schrieb Daniel Vetter:
>> Semnatically it really doesn't matter where we grab the ticket. But
>> since the ticket is a fake lockdep lock, it matters for lockdep
>> validation purposes.
>>
>> This means stuff like grabbing a ticket and then doing
>> copy_from/to_user isn't allowed anymore. This is a changed compared to
>> the current ttm fault handler, which doesn't bother with having a full
>> reservation. Since I'm looking into fixing the TODO entry in
>> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
>> later anyway, better get started. A bit more context on why I'm
>> looking into this: For backwards compat with existing i915 gem code I
>> think we'll have to do full slowpath locking in the i915 equivalent of
>> the eviction code. And with dynamic dma-buf that will leak across
>> drivers, so another thing we need to standardize and make sure it's
>> done the same way everyway.
>>
>> Unfortunately this means another full audit of all drivers:
>>
>> - gem helpers: acquire_init is done right before taking locks, so no
>> problem. Same for acquire_fini and unlocking, which means nothing
>> that's not already covered by the dma_resv_lock rules will be caught
>> with this extension here to the acquire_ctx.
>>
>> - etnaviv: An absolute massive amount of code is run between the
>> acquire_init and the first lock acquisition in submit_lock_objects.
>> But nothing that would touch user memory and could cause a fault.
>> Furthermore nothing that uses the ticket, so even if I missed
>> something, it would be easy to fix by pushing the acquire_init right
>> before the first use. Similar on the unlock/acquire_fini side.
>>
>> - i915: Right now (and this will likely change a lot rsn) the acquire
>> ctx and actual locks are right next to each another. No problem.
>>
>> - msm has a problem: submit_create calls acquire_init, but then
>> submit_lookup_objects() has a bunch of copy_from_user to do the
>> object lookups. That's the only thing before submit_lock_objects
>> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
>> does not have this issue since it copies all the userspace structs
>> earlier. submit_cleanup does not have any such issues.
>>
>> With the prep patch to pull out the acquire_ctx and reorder it msm
>> is going to be safe too.
>>
>> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
>> Similar on the acquire_fini/ttm_bo_unreserve side.
>>
>> - ttm execbuf utils: acquire context and locking are even in the same
>> functions here (one function to reserve everything, the other to
>> unreserve), so all good.
>>
>> - vc4: Another case where acquire context and locking are handled in
>> the same functions (one function to lock everything, the other to
>> unlock).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: linux-media@vger.kernel.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Cc: Huang Rui <ray.huang@amd.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Sean Paul <sean@poorly.run>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Acked-by: Christian König <christian.koenig@amd.com>
>
>> ---
>> drivers/dma-buf/dma-resv.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>> index d3c760e19991..079e38fde33a 100644
>> --- a/drivers/dma-buf/dma-resv.c
>> +++ b/drivers/dma-buf/dma-resv.c
>> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
>> static void __init dma_resv_lockdep(void)
>> {
>> struct mm_struct *mm = mm_alloc();
>> + struct ww_acquire_ctx ctx;
>> struct dma_resv obj;
>> + int ret;
>> if (!mm)
>> return;
>> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
>> dma_resv_init(&obj);
>> down_read(&mm->mmap_sem);
>> - ww_mutex_lock(&obj.lock, NULL);
>> + ww_acquire_init(&ctx, &reservation_ww_class);
>> + ret = dma_resv_lock(&obj, &ctx);
>> + if (ret == -EDEADLK)
>> + dma_resv_lock_slow(&obj, &ctx);
>> fs_reclaim_acquire(GFP_KERNEL);
>> fs_reclaim_release(GFP_KERNEL);
>> ww_mutex_unlock(&obj.lock);
>> + ww_acquire_fini(&ctx);
>> up_read(&mm->mmap_sem);
>>
>> mmput(mm);
>
For whole series:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
typo in patch 3 btw :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* ✗ Fi.CI.BUILD: failure for more dma-buf lockdep priming (rev2)
@ 2019-11-20 15:31 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-20 15:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming (rev2)
URL : https://patchwork.freedesktop.org/series/69695/
State : failure
== Summary ==
Applying: drm/modeset: Prime modeset lock vs dma_resv
Applying: dma-resv: Also prime acquire ctx for lockdep
error: sha1 information is lacking or useless (drivers/dma-buf/dma-resv.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0002 dma-resv: Also prime acquire ctx for lockdep
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for more dma-buf lockdep priming (rev2)
@ 2019-11-20 15:31 ` Patchwork
0 siblings, 0 replies; 49+ messages in thread
From: Patchwork @ 2019-11-20 15:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: more dma-buf lockdep priming (rev2)
URL : https://patchwork.freedesktop.org/series/69695/
State : failure
== Summary ==
Applying: drm/modeset: Prime modeset lock vs dma_resv
Applying: dma-resv: Also prime acquire ctx for lockdep
error: sha1 information is lacking or useless (drivers/dma-buf/dma-resv.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0002 dma-resv: Also prime acquire ctx for lockdep
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-21 0:24 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-21 0:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, Daniel Vetter,
Sean Paul, linux-arm-msm, freedreno
On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
>
> v2:
> - Add comment to explain why the ww ticket setup is separate (Rob)
> - Fix up error handling, we need to make sure we don't call
> ww_acquire_fini without _init.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
found a few minutes to take this for a spin and seems fine.. t-b && r-b
BR,
-R
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..385d4965a8d0 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> list_del_init(&msm_obj->submit_entry);
> drm_gem_object_put(&msm_obj->base);
> }
> -
> - ww_acquire_fini(&submit->ticket);
> }
>
> int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> struct msm_ringbuffer *ring;
> int out_fence_fd = -1;
> struct pid *pid = get_pid(task_pid(current));
> + bool has_ww_ticket = false;
> unsigned i;
> int ret, submitid;
> if (!gpu)
> @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + /* copy_*_user while holding a ww ticket upsets lockdep */
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> + has_ww_ticket = true;
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>
> out:
> submit_cleanup(submit);
> + if (has_ww_ticket)
> + ww_acquire_fini(&submit->ticket);
> if (ret)
> msm_gem_submit_free(submit);
> out_unlock:
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-21 0:24 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-21 0:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: Sean Paul, Intel Graphics Development, DRI Development,
linux-arm-msm, Daniel Vetter, freedreno
On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
>
> v2:
> - Add comment to explain why the ww ticket setup is separate (Rob)
> - Fix up error handling, we need to make sure we don't call
> ww_acquire_fini without _init.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
found a few minutes to take this for a spin and seems fine.. t-b && r-b
BR,
-R
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..385d4965a8d0 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> list_del_init(&msm_obj->submit_entry);
> drm_gem_object_put(&msm_obj->base);
> }
> -
> - ww_acquire_fini(&submit->ticket);
> }
>
> int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> struct msm_ringbuffer *ring;
> int out_fence_fd = -1;
> struct pid *pid = get_pid(task_pid(current));
> + bool has_ww_ticket = false;
> unsigned i;
> int ret, submitid;
> if (!gpu)
> @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + /* copy_*_user while holding a ww ticket upsets lockdep */
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> + has_ww_ticket = true;
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>
> out:
> submit_cleanup(submit);
> + if (has_ww_ticket)
> + ww_acquire_fini(&submit->ticket);
> if (ret)
> msm_gem_submit_free(submit);
> out_unlock:
> --
> 2.24.0
>
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-21 0:24 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-21 0:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: Sean Paul, Intel Graphics Development, DRI Development,
linux-arm-msm, Daniel Vetter, freedreno
On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
>
> v2:
> - Add comment to explain why the ww ticket setup is separate (Rob)
> - Fix up error handling, we need to make sure we don't call
> ww_acquire_fini without _init.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
found a few minutes to take this for a spin and seems fine.. t-b && r-b
BR,
-R
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..385d4965a8d0 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> list_del_init(&msm_obj->submit_entry);
> drm_gem_object_put(&msm_obj->base);
> }
> -
> - ww_acquire_fini(&submit->ticket);
> }
>
> int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> struct msm_ringbuffer *ring;
> int out_fence_fd = -1;
> struct pid *pid = get_pid(task_pid(current));
> + bool has_ww_ticket = false;
> unsigned i;
> int ret, submitid;
> if (!gpu)
> @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + /* copy_*_user while holding a ww ticket upsets lockdep */
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> + has_ww_ticket = true;
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>
> out:
> submit_cleanup(submit);
> + if (has_ww_ticket)
> + ww_acquire_fini(&submit->ticket);
> if (ret)
> msm_gem_submit_free(submit);
> out_unlock:
> --
> 2.24.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-21 0:24 ` Rob Clark
0 siblings, 0 replies; 49+ messages in thread
From: Rob Clark @ 2019-11-21 0:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, DRI Development, linux-arm-msm,
Daniel Vetter, freedreno
On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> For locking semantics it really doesn't matter when we grab the
> ticket. But for lockdep validation it does: the acquire ctx is a fake
> lockdep. Since other drivers might want to do a full multi-lock dance
> in their fault-handler, not just lock a single dma_resv. Therefore we
> must init the acquire_ctx only after we've done all the copy_*_user or
> anything else that might trigger a pagefault. For msm this means we
> need to move it past submit_lookup_objects.
>
> Aside: Why is msm still using struct_mutex, it seems to be using
> dma_resv_lock for general buffer state protection?
>
> v2:
> - Add comment to explain why the ww ticket setup is separate (Rob)
> - Fix up error handling, we need to make sure we don't call
> ww_acquire_fini without _init.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
found a few minutes to take this for a spin and seems fine.. t-b && r-b
BR,
-R
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fb1fdd7fa3ef..385d4965a8d0 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
> INIT_LIST_HEAD(&submit->node);
> INIT_LIST_HEAD(&submit->bo_list);
> - ww_acquire_init(&submit->ticket, &reservation_ww_class);
>
> return submit;
> }
> @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> list_del_init(&msm_obj->submit_entry);
> drm_gem_object_put(&msm_obj->base);
> }
> -
> - ww_acquire_fini(&submit->ticket);
> }
>
> int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> struct msm_ringbuffer *ring;
> int out_fence_fd = -1;
> struct pid *pid = get_pid(task_pid(current));
> + bool has_ww_ticket = false;
> unsigned i;
> int ret, submitid;
> if (!gpu)
> @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> + /* copy_*_user while holding a ww ticket upsets lockdep */
> + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> + has_ww_ticket = true;
> ret = submit_lock_objects(submit);
> if (ret)
> goto out;
> @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>
> out:
> submit_cleanup(submit);
> + if (has_ww_ticket)
> + ww_acquire_fini(&submit->ticket);
> if (ret)
> msm_gem_submit_free(submit);
> out_unlock:
> --
> 2.24.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
2019-11-21 0:24 ` Rob Clark
(?)
@ 2019-11-21 10:03 ` Daniel Vetter
-1 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-21 10:03 UTC (permalink / raw)
To: Rob Clark
Cc: Daniel Vetter, DRI Development, Intel Graphics Development,
Daniel Vetter, Sean Paul, linux-arm-msm, freedreno
On Wed, Nov 20, 2019 at 04:24:48PM -0800, Rob Clark wrote:
> On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
> >
> > v2:
> > - Add comment to explain why the ww ticket setup is separate (Rob)
> > - Fix up error handling, we need to make sure we don't call
> > ww_acquire_fini without _init.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
>
> found a few minutes to take this for a spin and seems fine.. t-b && r-b
Thanks for taking this for a spin, entire series applied.
-Daniel
>
> BR,
> -R
>
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..385d4965a8d0 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> > list_del_init(&msm_obj->submit_entry);
> > drm_gem_object_put(&msm_obj->base);
> > }
> > -
> > - ww_acquire_fini(&submit->ticket);
> > }
> >
> > int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > struct msm_ringbuffer *ring;
> > int out_fence_fd = -1;
> > struct pid *pid = get_pid(task_pid(current));
> > + bool has_ww_ticket = false;
> > unsigned i;
> > int ret, submitid;
> > if (!gpu)
> > @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + /* copy_*_user while holding a ww ticket upsets lockdep */
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > + has_ww_ticket = true;
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >
> > out:
> > submit_cleanup(submit);
> > + if (has_ww_ticket)
> > + ww_acquire_fini(&submit->ticket);
> > if (ret)
> > msm_gem_submit_free(submit);
> > out_unlock:
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-21 10:03 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-21 10:03 UTC (permalink / raw)
To: Rob Clark
Cc: freedreno, linux-arm-msm, Intel Graphics Development,
DRI Development, Daniel Vetter, Daniel Vetter, Sean Paul
On Wed, Nov 20, 2019 at 04:24:48PM -0800, Rob Clark wrote:
> On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
> >
> > v2:
> > - Add comment to explain why the ww ticket setup is separate (Rob)
> > - Fix up error handling, we need to make sure we don't call
> > ww_acquire_fini without _init.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
>
> found a few minutes to take this for a spin and seems fine.. t-b && r-b
Thanks for taking this for a spin, entire series applied.
-Daniel
>
> BR,
> -R
>
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..385d4965a8d0 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> > list_del_init(&msm_obj->submit_entry);
> > drm_gem_object_put(&msm_obj->base);
> > }
> > -
> > - ww_acquire_fini(&submit->ticket);
> > }
> >
> > int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > struct msm_ringbuffer *ring;
> > int out_fence_fd = -1;
> > struct pid *pid = get_pid(task_pid(current));
> > + bool has_ww_ticket = false;
> > unsigned i;
> > int ret, submitid;
> > if (!gpu)
> > @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + /* copy_*_user while holding a ww ticket upsets lockdep */
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > + has_ww_ticket = true;
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >
> > out:
> > submit_cleanup(submit);
> > + if (has_ww_ticket)
> > + ww_acquire_fini(&submit->ticket);
> > if (ret)
> > msm_gem_submit_free(submit);
> > out_unlock:
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/msm: Don't init ww_mutec acquire ctx before needed
@ 2019-11-21 10:03 ` Daniel Vetter
0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2019-11-21 10:03 UTC (permalink / raw)
To: Rob Clark
Cc: freedreno, linux-arm-msm, Intel Graphics Development,
DRI Development, Daniel Vetter, Daniel Vetter
On Wed, Nov 20, 2019 at 04:24:48PM -0800, Rob Clark wrote:
> On Wed, Nov 20, 2019 at 2:56 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > For locking semantics it really doesn't matter when we grab the
> > ticket. But for lockdep validation it does: the acquire ctx is a fake
> > lockdep. Since other drivers might want to do a full multi-lock dance
> > in their fault-handler, not just lock a single dma_resv. Therefore we
> > must init the acquire_ctx only after we've done all the copy_*_user or
> > anything else that might trigger a pagefault. For msm this means we
> > need to move it past submit_lookup_objects.
> >
> > Aside: Why is msm still using struct_mutex, it seems to be using
> > dma_resv_lock for general buffer state protection?
> >
> > v2:
> > - Add comment to explain why the ww ticket setup is separate (Rob)
> > - Fix up error handling, we need to make sure we don't call
> > ww_acquire_fini without _init.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
>
> found a few minutes to take this for a spin and seems fine.. t-b && r-b
Thanks for taking this for a spin, entire series applied.
-Daniel
>
> BR,
> -R
>
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index fb1fdd7fa3ef..385d4965a8d0 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -54,7 +54,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
> >
> > INIT_LIST_HEAD(&submit->node);
> > INIT_LIST_HEAD(&submit->bo_list);
> > - ww_acquire_init(&submit->ticket, &reservation_ww_class);
> >
> > return submit;
> > }
> > @@ -390,8 +389,6 @@ static void submit_cleanup(struct msm_gem_submit *submit)
> > list_del_init(&msm_obj->submit_entry);
> > drm_gem_object_put(&msm_obj->base);
> > }
> > -
> > - ww_acquire_fini(&submit->ticket);
> > }
> >
> > int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > @@ -408,6 +405,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > struct msm_ringbuffer *ring;
> > int out_fence_fd = -1;
> > struct pid *pid = get_pid(task_pid(current));
> > + bool has_ww_ticket = false;
> > unsigned i;
> > int ret, submitid;
> > if (!gpu)
> > @@ -489,6 +487,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > if (ret)
> > goto out;
> >
> > + /* copy_*_user while holding a ww ticket upsets lockdep */
> > + ww_acquire_init(&submit->ticket, &reservation_ww_class);
> > + has_ww_ticket = true;
> > ret = submit_lock_objects(submit);
> > if (ret)
> > goto out;
> > @@ -588,6 +589,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >
> > out:
> > submit_cleanup(submit);
> > + if (has_ww_ticket)
> > + ww_acquire_fini(&submit->ticket);
> > if (ret)
> > msm_gem_submit_free(submit);
> > out_unlock:
> > --
> > 2.24.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2019-11-21 10:04 UTC | newest]
Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 21:08 [PATCH 0/3] more dma-buf lockdep priming Daniel Vetter
2019-11-19 21:08 ` [Intel-gfx] " Daniel Vetter
2019-11-19 21:08 ` Daniel Vetter
2019-11-19 21:08 ` [PATCH 1/3] drm/modeset: Prime modeset lock vs dma_resv Daniel Vetter
2019-11-19 21:08 ` [Intel-gfx] " Daniel Vetter
2019-11-20 8:34 ` Christian König
2019-11-20 8:34 ` [Intel-gfx] " Christian König
2019-11-20 8:34 ` Christian König
2019-11-20 10:55 ` Daniel Vetter
2019-11-20 10:55 ` [Intel-gfx] " Daniel Vetter
2019-11-19 21:08 ` [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep Daniel Vetter
2019-11-19 21:08 ` [Intel-gfx] " Daniel Vetter
2019-11-19 21:08 ` Daniel Vetter
2019-11-19 21:08 ` Daniel Vetter
2019-11-20 11:30 ` Christian König
2019-11-20 11:30 ` [Intel-gfx] " Christian König
2019-11-20 11:30 ` Christian König
2019-11-20 13:56 ` Maarten Lankhorst
2019-11-20 13:56 ` [Intel-gfx] " Maarten Lankhorst
2019-11-20 13:56 ` Maarten Lankhorst
2019-11-19 21:08 ` [PATCH 3/3] drm/msm: Don't init ww_mutec acquire ctx before needed Daniel Vetter
2019-11-19 21:08 ` [Intel-gfx] " Daniel Vetter
2019-11-19 21:08 ` Daniel Vetter
2019-11-20 2:07 ` Rob Clark
2019-11-20 2:07 ` [Intel-gfx] " Rob Clark
2019-11-20 2:07 ` Rob Clark
2019-11-20 2:07 ` Rob Clark
2019-11-20 10:22 ` Daniel Vetter
2019-11-20 10:22 ` [Intel-gfx] " Daniel Vetter
2019-11-20 10:22 ` Daniel Vetter
2019-11-20 10:22 ` Daniel Vetter
2019-11-20 10:56 ` [PATCH] " Daniel Vetter
2019-11-20 10:56 ` [Intel-gfx] " Daniel Vetter
2019-11-20 10:56 ` Daniel Vetter
2019-11-21 0:24 ` Rob Clark
2019-11-21 0:24 ` [Intel-gfx] " Rob Clark
2019-11-21 0:24 ` Rob Clark
2019-11-21 0:24 ` Rob Clark
2019-11-21 10:03 ` Daniel Vetter
2019-11-21 10:03 ` [Intel-gfx] " Daniel Vetter
2019-11-21 10:03 ` Daniel Vetter
2019-11-19 22:31 ` ✗ Fi.CI.CHECKPATCH: warning for more dma-buf lockdep priming Patchwork
2019-11-19 22:31 ` [Intel-gfx] " Patchwork
2019-11-19 22:53 ` ✓ Fi.CI.BAT: success " Patchwork
2019-11-19 22:53 ` [Intel-gfx] " Patchwork
2019-11-20 13:55 ` ✓ Fi.CI.IGT: " Patchwork
2019-11-20 13:55 ` [Intel-gfx] " Patchwork
2019-11-20 15:31 ` ✗ Fi.CI.BUILD: failure for more dma-buf lockdep priming (rev2) Patchwork
2019-11-20 15:31 ` [Intel-gfx] " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.