* [PATCH] drm/i915/blt: Remove recursive vma->lock
@ 2019-06-18 9:52 Chris Wilson
2019-06-18 9:58 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Chris Wilson @ 2019-06-18 9:52 UTC (permalink / raw)
To: intel-gfx; +Cc: Matthew Auld
As we have already plugged the w->dma into the reservation_object, and
have set ourselves up to automatically signal the request and w->dma on
completion, we do not need to export the rq->fence directly and just use
the w->dma fence.
This avoids having to take the reservation_lock inside the worker which
cross-release lockdep would complain about. :)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
---
Patch based on the i915_active overhaul, but on the face of it should be
safe without. But let's pretend it does need the overhaul as this is one
of the motivations :)
-Chris
---
drivers/gpu/drm/i915/gem/i915_gem_client_blt.c | 10 ++++++----
.../gpu/drm/i915/gem/selftests/i915_gem_client_blt.c | 11 -----------
2 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
index f253ec5765ad..83b5c5d13b93 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
@@ -193,10 +193,12 @@ static void clear_pages_worker(struct work_struct *work)
goto out_request;
}
- /* XXX: more feverish nightmares await */
- i915_vma_lock(vma);
- err = i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE);
- i915_vma_unlock(vma);
+ /*
+ * w->dma is already exported via (vma|obj)->resv we need only
+ * keep track of the GPU activity within this vma/request, and
+ * propagate the signal from the request to w->dma.
+ */
+ err = i915_active_ref(&vma->active, rq->fence.context, &rq->fence);
if (err)
goto out_request;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
index f3a5eb807c1c..855481252bda 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
@@ -63,17 +63,6 @@ static int igt_client_fill(void *arg)
if (err)
goto err_unpin;
- /*
- * XXX: For now do the wait without the object resv lock to
- * ensure we don't deadlock.
- */
- err = i915_gem_object_wait(obj,
- I915_WAIT_INTERRUPTIBLE |
- I915_WAIT_ALL,
- MAX_SCHEDULE_TIMEOUT);
- if (err)
- goto err_unpin;
-
i915_gem_object_lock(obj);
err = i915_gem_object_set_to_cpu_domain(obj, false);
i915_gem_object_unlock(obj);
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915/blt: Remove recursive vma->lock
2019-06-18 9:52 [PATCH] drm/i915/blt: Remove recursive vma->lock Chris Wilson
@ 2019-06-18 9:58 ` Patchwork
2019-06-18 12:48 ` [PATCH] " Matthew Auld
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2019-06-18 9:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/blt: Remove recursive vma->lock
URL : https://patchwork.freedesktop.org/series/62290/
State : failure
== Summary ==
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
DESCEND objtool
CHK include/generated/compile.h
AR drivers/gpu/drm/i915/built-in.a
CC [M] drivers/gpu/drm/i915/header_test_i915_active_types.o
CC [M] drivers/gpu/drm/i915/header_test_i915_debugfs.o
CC [M] drivers/gpu/drm/i915/header_test_i915_drv.o
CC [M] drivers/gpu/drm/i915/header_test_i915_irq.o
CC [M] drivers/gpu/drm/i915/header_test_i915_params.o
CC [M] drivers/gpu/drm/i915/header_test_i915_priolist_types.o
CC [M] drivers/gpu/drm/i915/header_test_i915_reg.o
CC [M] drivers/gpu/drm/i915/header_test_i915_scheduler_types.o
CC [M] drivers/gpu/drm/i915/header_test_i915_timeline_types.o
CC [M] drivers/gpu/drm/i915/header_test_i915_utils.o
CC [M] drivers/gpu/drm/i915/header_test_intel_csr.o
CC [M] drivers/gpu/drm/i915/header_test_intel_drv.o
CC [M] drivers/gpu/drm/i915/header_test_intel_pm.o
CC [M] drivers/gpu/drm/i915/header_test_intel_runtime_pm.o
CC [M] drivers/gpu/drm/i915/header_test_intel_sideband.o
CC [M] drivers/gpu/drm/i915/header_test_intel_uncore.o
CC [M] drivers/gpu/drm/i915/header_test_intel_wakeref.o
CC [M] drivers/gpu/drm/i915/gem/i915_gem_client_blt.o
drivers/gpu/drm/i915/gem/i915_gem_client_blt.c: In function ‘clear_pages_worker’:
drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:201:57: error: passing argument 3 of ‘i915_active_ref’ from incompatible pointer type [-Werror=incompatible-pointer-types]
err = i915_active_ref(&vma->active, rq->fence.context, &rq->fence);
^
In file included from drivers/gpu/drm/i915/gem/i915_gem_object_types.h:14:0,
from drivers/gpu/drm/i915/gem/i915_gem_object.h:16,
from drivers/gpu/drm/i915/display/intel_frontbuffer.h:27,
from drivers/gpu/drm/i915/i915_drv.h:70,
from drivers/gpu/drm/i915/intel_drv.h:45,
from drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:8:
drivers/gpu/drm/i915/i915_active.h:376:5: note: expected ‘struct i915_request *’ but argument is of type ‘struct dma_fence *’
int i915_active_ref(struct i915_active *ref,
^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:278: recipe for target 'drivers/gpu/drm/i915/gem/i915_gem_client_blt.o' failed
make[4]: *** [drivers/gpu/drm/i915/gem/i915_gem_client_blt.o] Error 1
scripts/Makefile.build:489: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:489: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:489: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1071: recipe for target 'drivers' failed
make: *** [drivers] Error 2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/blt: Remove recursive vma->lock
2019-06-18 9:52 [PATCH] drm/i915/blt: Remove recursive vma->lock Chris Wilson
2019-06-18 9:58 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2019-06-18 12:48 ` Matthew Auld
2019-06-18 17:28 ` kbuild test robot
2019-06-18 18:20 ` kbuild test robot
3 siblings, 0 replies; 5+ messages in thread
From: Matthew Auld @ 2019-06-18 12:48 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel Graphics Development, Matthew Auld
On Tue, 18 Jun 2019 at 10:53, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> As we have already plugged the w->dma into the reservation_object, and
> have set ourselves up to automatically signal the request and w->dma on
> completion, we do not need to export the rq->fence directly and just use
> the w->dma fence.
>
> This avoids having to take the reservation_lock inside the worker which
> cross-release lockdep would complain about. :)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
> Patch based on the i915_active overhaul, but on the face of it should be
> safe without. But let's pretend it does need the overhaul as this is one
> of the motivations :)
> -Chris
> ---
> drivers/gpu/drm/i915/gem/i915_gem_client_blt.c | 10 ++++++----
> .../gpu/drm/i915/gem/selftests/i915_gem_client_blt.c | 11 -----------
> 2 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> index f253ec5765ad..83b5c5d13b93 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> @@ -193,10 +193,12 @@ static void clear_pages_worker(struct work_struct *work)
> goto out_request;
> }
>
> - /* XXX: more feverish nightmares await */
> - i915_vma_lock(vma);
> - err = i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE);
> - i915_vma_unlock(vma);
> + /*
> + * w->dma is already exported via (vma|obj)->resv we need only
> + * keep track of the GPU activity within this vma/request, and
> + * propagate the signal from the request to w->dma.
> + */
> + err = i915_active_ref(&vma->active, rq->fence.context, &rq->fence);
> if (err)
> goto out_request;
Oh, wow ~o~
Yeah, seems to work locally with the extra ref + active count
tracking, so I guess should fit nicely with your i915_active.acquire,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/blt: Remove recursive vma->lock
2019-06-18 9:52 [PATCH] drm/i915/blt: Remove recursive vma->lock Chris Wilson
2019-06-18 9:58 ` ✗ Fi.CI.BAT: failure for " Patchwork
2019-06-18 12:48 ` [PATCH] " Matthew Auld
@ 2019-06-18 17:28 ` kbuild test robot
2019-06-18 18:20 ` kbuild test robot
3 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2019-06-18 17:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all, Matthew Auld
Hi Chris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20190618]
[cannot apply to v5.2-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-blt-Remove-recursive-vma-lock/20190618-194749
base: git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-rc1-7-g2b96cd8-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:201:65: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected struct i915_request *rq @@ got uct i915_request *rq @@
>> drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:201:65: sparse: expected struct i915_request *rq
>> drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:201:65: sparse: got struct dma_fence *
include/linux/reservation.h:220:20: sparse: sparse: dereference of noderef expression
include/linux/reservation.h:220:45: sparse: sparse: dereference of noderef expression
include/linux/reservation.h:220:20: sparse: sparse: dereference of noderef expression
include/linux/reservation.h:220:45: sparse: sparse: dereference of noderef expression
vim +201 drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
153
154 static void clear_pages_worker(struct work_struct *work)
155 {
156 struct clear_pages_work *w = container_of(work, typeof(*w), work);
157 struct drm_i915_private *i915 = w->ce->gem_context->i915;
158 struct drm_i915_gem_object *obj = w->sleeve->obj;
159 struct i915_vma *vma = w->sleeve->vma;
160 struct i915_request *rq;
161 int err = w->dma.error;
162
163 if (unlikely(err))
164 goto out_signal;
165
166 if (obj->cache_dirty) {
167 obj->write_domain = 0;
168 if (i915_gem_object_has_struct_page(obj))
169 drm_clflush_sg(w->sleeve->pages);
170 obj->cache_dirty = false;
171 }
172
173 /* XXX: we need to kill this */
174 mutex_lock(&i915->drm.struct_mutex);
175 err = i915_vma_pin(vma, 0, 0, PIN_USER);
176 if (unlikely(err))
177 goto out_unlock;
178
179 rq = i915_request_create(w->ce);
180 if (IS_ERR(rq)) {
181 err = PTR_ERR(rq);
182 goto out_unpin;
183 }
184
185 /* There's no way the fence has signalled */
186 if (dma_fence_add_callback(&rq->fence, &w->cb,
187 clear_pages_dma_fence_cb))
188 GEM_BUG_ON(1);
189
190 if (w->ce->engine->emit_init_breadcrumb) {
191 err = w->ce->engine->emit_init_breadcrumb(rq);
192 if (unlikely(err))
193 goto out_request;
194 }
195
196 /*
197 * w->dma is already exported via (vma|obj)->resv we need only
198 * keep track of the GPU activity within this vma/request, and
199 * propagate the signal from the request to w->dma.
200 */
> 201 err = i915_active_ref(&vma->active, rq->fence.context, &rq->fence);
202 if (err)
203 goto out_request;
204
205 err = intel_emit_vma_fill_blt(rq, vma, w->value);
206 out_request:
207 if (unlikely(err)) {
208 i915_request_skip(rq, err);
209 err = 0;
210 }
211
212 i915_request_add(rq);
213 out_unpin:
214 i915_vma_unpin(vma);
215 out_unlock:
216 mutex_unlock(&i915->drm.struct_mutex);
217 out_signal:
218 if (unlikely(err)) {
219 dma_fence_set_error(&w->dma, err);
220 dma_fence_signal(&w->dma);
221 dma_fence_put(&w->dma);
222 }
223 }
224
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/blt: Remove recursive vma->lock
2019-06-18 9:52 [PATCH] drm/i915/blt: Remove recursive vma->lock Chris Wilson
` (2 preceding siblings ...)
2019-06-18 17:28 ` kbuild test robot
@ 2019-06-18 18:20 ` kbuild test robot
3 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2019-06-18 18:20 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all, Matthew Auld
[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]
Hi Chris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20190618]
[cannot apply to v5.2-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-blt-Remove-recursive-vma-lock/20190618-194749
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/gem/i915_gem_client_blt.c: In function 'clear_pages_worker':
>> drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:201:57: error: passing argument 3 of 'i915_active_ref' from incompatible pointer type [-Werror=incompatible-pointer-types]
err = i915_active_ref(&vma->active, rq->fence.context, &rq->fence);
^
In file included from drivers/gpu/drm/i915/i915_timeline.h:30:0,
from drivers/gpu/drm/i915/gt/intel_engine.h:17,
from drivers/gpu/drm/i915/i915_drv.h:67,
from drivers/gpu/drm/i915/intel_drv.h:45,
from drivers/gpu/drm/i915/gem/i915_gem_client_blt.c:8:
drivers/gpu/drm/i915/i915_active.h:376:5: note: expected 'struct i915_request *' but argument is of type 'struct dma_fence *'
int i915_active_ref(struct i915_active *ref,
^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/i915_active_ref +201 drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
153
154 static void clear_pages_worker(struct work_struct *work)
155 {
156 struct clear_pages_work *w = container_of(work, typeof(*w), work);
157 struct drm_i915_private *i915 = w->ce->gem_context->i915;
158 struct drm_i915_gem_object *obj = w->sleeve->obj;
159 struct i915_vma *vma = w->sleeve->vma;
160 struct i915_request *rq;
161 int err = w->dma.error;
162
163 if (unlikely(err))
164 goto out_signal;
165
166 if (obj->cache_dirty) {
167 obj->write_domain = 0;
168 if (i915_gem_object_has_struct_page(obj))
169 drm_clflush_sg(w->sleeve->pages);
170 obj->cache_dirty = false;
171 }
172
173 /* XXX: we need to kill this */
174 mutex_lock(&i915->drm.struct_mutex);
175 err = i915_vma_pin(vma, 0, 0, PIN_USER);
176 if (unlikely(err))
177 goto out_unlock;
178
179 rq = i915_request_create(w->ce);
180 if (IS_ERR(rq)) {
181 err = PTR_ERR(rq);
182 goto out_unpin;
183 }
184
185 /* There's no way the fence has signalled */
186 if (dma_fence_add_callback(&rq->fence, &w->cb,
187 clear_pages_dma_fence_cb))
188 GEM_BUG_ON(1);
189
190 if (w->ce->engine->emit_init_breadcrumb) {
191 err = w->ce->engine->emit_init_breadcrumb(rq);
192 if (unlikely(err))
193 goto out_request;
194 }
195
196 /*
197 * w->dma is already exported via (vma|obj)->resv we need only
198 * keep track of the GPU activity within this vma/request, and
199 * propagate the signal from the request to w->dma.
200 */
> 201 err = i915_active_ref(&vma->active, rq->fence.context, &rq->fence);
202 if (err)
203 goto out_request;
204
205 err = intel_emit_vma_fill_blt(rq, vma, w->value);
206 out_request:
207 if (unlikely(err)) {
208 i915_request_skip(rq, err);
209 err = 0;
210 }
211
212 i915_request_add(rq);
213 out_unpin:
214 i915_vma_unpin(vma);
215 out_unlock:
216 mutex_unlock(&i915->drm.struct_mutex);
217 out_signal:
218 if (unlikely(err)) {
219 dma_fence_set_error(&w->dma, err);
220 dma_fence_signal(&w->dma);
221 dma_fence_put(&w->dma);
222 }
223 }
224
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42816 bytes --]
[-- Attachment #3: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-18 18:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-18 9:52 [PATCH] drm/i915/blt: Remove recursive vma->lock Chris Wilson
2019-06-18 9:58 ` ✗ Fi.CI.BAT: failure for " Patchwork
2019-06-18 12:48 ` [PATCH] " Matthew Auld
2019-06-18 17:28 ` kbuild test robot
2019-06-18 18:20 ` kbuild test robot
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.