Hi Andi,
Thx for looking at it.
On 21.10.2022 17:51, Andi Shyti wrote:
Hi Andrzej,
(at first I r-b'ed this patch, but then I wanted to think on some
more "simplification" (if it really simplifies things). Please
read the review in patch 2 first )
This is v1, there is already v2.
I will reply here to your comments with v2 in mind.
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 1cae24349a96fd..80e7fdd5d16427 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -565,10 +565,8 @@ static int make_obj_busy(struct drm_i915_gem_object *obj)
goto err_unpin;
}
- err = i915_request_await_object(rq, vma->obj, true);
- if (err == 0)
- err = i915_vma_move_to_active(vma, rq,
- EXEC_OBJECT_WRITE);
+ err = i915_vma_move_to_active(vma, rq,
+ EXEC_OBJECT_WRITE);
nit: don't need to break the line here.
Corrected in v2.
i915_request_add(rq);
err_unpin:
[...]
@@ -860,9 +854,7 @@ static int read_whitelisted_registers(struct intel_context *ce,
return PTR_ERR(rq);
i915_vma_lock(results);
- err = i915_request_await_object(rq, results->obj, true);
- if (err == 0)
- err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
+ err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
i915_vma_unlock(results);
if (err)
goto err_req;
@@ -944,9 +936,7 @@ static int scrub_whitelisted_registers(struct intel_context *ce)
}
i915_vma_lock(batch);
- err = i915_request_await_object(rq, batch->obj, false);
- if (err == 0)
- err = i915_vma_move_to_active(batch, rq, 0);
+ err = i915_vma_move_to_active(batch, rq, 0);
i915_vma_unlock(batch);
The final risult would be:
i915_vma_lock();
i915_vma_move_to_active()
i915_vma_unlock();
and it's a pattern... as I suggested in patch 2, how about having
an:
i915_vma_move_to_active_unlocked()
There is igt_vma_move_to_active_unlocked in patch 2. Chris suggested to
limit this helper to selftests, as this pattern is specific to selftests
and should not be exposed as 'internal API'.
and...
if (err)
goto err_request;
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index d6fe94cd0fdb61..b49098f045005e 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -570,9 +570,8 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
if (gmadr_bytes == 8)
bb->bb_start_cmd_va[2] = 0;
- ret = i915_vma_move_to_active(bb->vma,
- workload->req,
- 0);
+ ret = _i915_vma_move_to_active(bb->vma, workload->req,
+ &workload->req->fence, 0);
if (ret)
goto err;
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 15816df916c781..19138e99d2fd03 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2015,9 +2015,7 @@ emit_oa_config(struct i915_perf_stream *stream,
goto err_add_request;
}
- err = i915_request_await_object(rq, vma->obj, 0);
- if (!err)
- err = i915_vma_move_to_active(vma, rq, 0);
+ err = i915_vma_move_to_active(vma, rq, 0);
if (err)
goto err_add_request;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index aecd9c64486b27..47ac5bd1ffcce6 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -64,7 +64,11 @@ static inline int __must_check
i915_vma_move_to_active(struct i915_vma *vma, struct i915_request *rq,
unsigned int flags)
{
- return _i915_vma_move_to_active(vma, rq, &rq->fence, flags);
+ int err = i915_request_await_object(rq, vma->obj, flags & EXEC_OBJECT_WRITE);
+
+ if (!err)
+ err = _i915_vma_move_to_active(vma, rq, &rq->fence, flags);
+ return err;
}
... this i915_vma_move_to_active() now it's doing more than just
moving to active but it's also waiting on dma fences, shall we
call it i915_vma_move_to_active_async() or silimar? (I'm not good
at giving names).
I do not feel an expert in this area, but for example
__i915_vma_move_to_active also calls __i915_request_await_bind and then
moves to active (so awaits are there anyway).
In v2 this was handled by putting i915_request_await_object to
_i915_vma_move_to_active and added no_await flag.
Regards
Andrzej
The above would be i915_vma_move_to_active_async_unlocked(). Too
long? More complex?
We would have something like:
i915_vma_move_to_active() /* not used */
i915_vma_move_to_active_unlocked()
i915_vma_move_to_active_async()
i915_vma_move_to_active_async_unlocked()
Anyway as it is looks good, I didn't spot any error in the
conversion:
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Andi
[...]
