Re: [Intel-gfx] [PATCH v2] drm/i915: Remove redundant i915_request_await_object in blit clears
>-Original Message-
>From: Tvrtko Ursulin
>Sent: Monday, June 15, 2020 12:16 PM
>To: Ruhl, Michael J ; Intel-
>g...@lists.freedesktop.org
>Cc: Ursulin, Tvrtko ; Auld, Matthew
>; Chris Wilson
>Subject: Re: [PATCH v2] drm/i915: Remove redundant
>i915_request_await_object in blit clears
>
>
>On 15/06/2020 17:11, Ruhl, Michael J wrote:
>>> -Original Message-
>>> From: Tvrtko Ursulin
>>> Sent: Monday, June 15, 2020 11:15 AM
>>> To: Intel-gfx@lists.freedesktop.org
>>> Cc: Ursulin, Tvrtko ; Auld, Matthew
>>> ; Chris Wilson ;
>Ruhl,
>>> Michael J
>>> Subject: [PATCH v2] drm/i915: Remove redundant
>i915_request_await_object
>>> in blit clears
>>>
>>> From: Tvrtko Ursulin
>>>
>>> One i915_request_await_object is enough and we keep the one under the
>>> object lock so it is final.
>>>
>>> At the same time move async clflushing setup under the same locked
>>> section and consolidate common code into a helper function.
>>>
>>> v2:
>>> * Emit initial breadcrumbs after aways are set up. (Chris)
>>>
>>> Signed-off-by: Tvrtko Ursulin
>>> Cc: Matthew Auld
>>> Cc: Chris Wilson
>>> Cc: Michael J. Ruhl
>>> ---
>>> .../gpu/drm/i915/gem/i915_gem_object_blt.c| 52 ---
>>> 1 file changed, 21 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>>> b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>>> index f457d7130491..bfdb32d46877 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>>> @@ -126,6 +126,17 @@ void intel_emit_vma_release(struct intel_context
>>> *ce, struct i915_vma *vma)
>>> intel_engine_pm_put(ce->engine);
>>> }
>>>
>>> +static int
>>> +move_obj_to_gpu(struct drm_i915_gem_object *obj,
>>
>> I am not understanding the name of this function.
>>
>> How is the object moved to the gpu? Is clflush a move? Or is
>> it that it is moving to the gpu domain?
>>
>> What about:
>>
>> obj_flush_and_wait()
>>
>> or just:
>>
>> flush_and_wait()
>>
>> ?
>>
>> Or am I missing something? 😊
>
>Yes, the fact I have renamed the existing move_to_gpu to move_obj_to_gpu
>while moving it up in the file and so risked falling victim to bike
>shedding now. :D
Ok.
Code path makes sense to me.
Reviewed-by: Michael J. Ruhl
M
>
>Regards,
>
>Tvrtko
>
>>
>> Mike
>>
>>> + struct i915_request *rq,
>>> + bool write)
>>> +{
>>> + if (obj->cache_dirty & ~obj->cache_coherent)
>>> + i915_gem_clflush_object(obj, 0);
>>> +
>>> + return i915_request_await_object(rq, obj, write);
>>> +}
>>> +
>>> int i915_gem_object_fill_blt(struct drm_i915_gem_object *obj,
>>> struct intel_context *ce,
>>> u32 value)
>>> @@ -143,12 +154,6 @@ int i915_gem_object_fill_blt(struct
>>> drm_i915_gem_object *obj,
>>> if (unlikely(err))
>>> return err;
>>>
>>> - if (obj->cache_dirty & ~obj->cache_coherent) {
>>> - i915_gem_object_lock(obj);
>>> - i915_gem_clflush_object(obj, 0);
>>> - i915_gem_object_unlock(obj);
>>> - }
>>> -
>>> batch = intel_emit_vma_fill_blt(ce, vma, value);
>>> if (IS_ERR(batch)) {
>>> err = PTR_ERR(batch);
>>> @@ -165,27 +170,22 @@ int i915_gem_object_fill_blt(struct
>>> drm_i915_gem_object *obj,
>>> if (unlikely(err))
>>> goto out_request;
>>>
>>> - err = i915_request_await_object(rq, obj, true);
>>> - if (unlikely(err))
>>> - goto out_request;
>>> -
>>> - if (ce->engine->emit_init_breadcrumb) {
>>> - err = ce->engine->emit_init_breadcrumb(rq);
>>> - if (unlikely(err))
>>> - goto out_request;
>>> - }
>>> -
>>> i915_vma_lock(vma);
>>> - err = i915_request_await_object(rq, vma->obj, true);
>>> + err = move_obj_to_gpu(vma->obj, rq, true);
>>> if (err == 0)
>>> err = i915_vma_move_to_active(vma, rq,
>>> EXEC_OBJECT_WRITE);
>>> i915_vma_unlock(vma);
>>> if (unlikely(err))
>>> goto out_request;
>>>
>>> - err = ce->engine->emit_bb_start(rq,
>>> - batch->node.start, batch->node.size,
>>> - 0);
>>> + if (ce->engine->emit_init_breadcrumb)
>>> + err = ce->engine->emit_init_breadcrumb(rq);
>>> +
>>> + if (likely(!err))
>>> + err = ce->engine->emit_bb_start(rq,
>>> + batch->node.start,
>>> + batch->node.size,
>>> + 0);
>>> out_request:
>>> if (unlikely(err))
>>> i915_request_set_error_once(rq, err);
>>> @@ -317,16 +317,6 @@ struct i915_vma *intel_emit_vma_copy_blt(struct
>>> intel_context *ce,
>>> return ERR_PTR(err);
>>> }
>>>
>>> -static int move_to_gpu(struct i915_vma *vma, struct i915_request *rq,
>bool
>>> write)
>>> -{
>>> - struct drm_i915_gem_object *obj = vma->obj;
>>> -
>>> - if (obj->cache_d
Re: [Intel-gfx] [PATCH v2] drm/i915: Remove redundant i915_request_await_object in blit clears
Quoting Tvrtko Ursulin (2020-06-15 16:14:49) > From: Tvrtko Ursulin > > One i915_request_await_object is enough and we keep the one under the > object lock so it is final. > > At the same time move async clflushing setup under the same locked > section and consolidate common code into a helper function. > > v2: > * Emit initial breadcrumbs after aways are set up. (Chris) s/aways/awaits/ > Signed-off-by: Tvrtko Ursulin > Cc: Matthew Auld > Cc: Chris Wilson > Cc: Michael J. Ruhl > --- > .../gpu/drm/i915/gem/i915_gem_object_blt.c| 52 --- > 1 file changed, 21 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c > b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c > index f457d7130491..bfdb32d46877 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c > @@ -126,6 +126,17 @@ void intel_emit_vma_release(struct intel_context *ce, > struct i915_vma *vma) > intel_engine_pm_put(ce->engine); > } > > +static int > +move_obj_to_gpu(struct drm_i915_gem_object *obj, > + struct i915_request *rq, > + bool write) * shrug, I prefer to think in terms of vma, but even vma are unlikely to be the final form here. Reviewed-by: Chris Wilson -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Remove redundant i915_request_await_object in blit clears
On 15/06/2020 17:11, Ruhl, Michael J wrote:
-Original Message-
From: Tvrtko Ursulin
Sent: Monday, June 15, 2020 11:15 AM
To: Intel-gfx@lists.freedesktop.org
Cc: Ursulin, Tvrtko ; Auld, Matthew
; Chris Wilson ; Ruhl,
Michael J
Subject: [PATCH v2] drm/i915: Remove redundant i915_request_await_object
in blit clears
From: Tvrtko Ursulin
One i915_request_await_object is enough and we keep the one under the
object lock so it is final.
At the same time move async clflushing setup under the same locked
section and consolidate common code into a helper function.
v2:
* Emit initial breadcrumbs after aways are set up. (Chris)
Signed-off-by: Tvrtko Ursulin
Cc: Matthew Auld
Cc: Chris Wilson
Cc: Michael J. Ruhl
---
.../gpu/drm/i915/gem/i915_gem_object_blt.c| 52 ---
1 file changed, 21 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
index f457d7130491..bfdb32d46877 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
@@ -126,6 +126,17 @@ void intel_emit_vma_release(struct intel_context
*ce, struct i915_vma *vma)
intel_engine_pm_put(ce->engine);
}
+static int
+move_obj_to_gpu(struct drm_i915_gem_object *obj,
I am not understanding the name of this function.
How is the object moved to the gpu? Is clflush a move? Or is
it that it is moving to the gpu domain?
What about:
obj_flush_and_wait()
or just:
flush_and_wait()
?
Or am I missing something? 😊
Yes, the fact I have renamed the existing move_to_gpu to move_obj_to_gpu
while moving it up in the file and so risked falling victim to bike
shedding now. :D
Regards,
Tvrtko
Mike
+ struct i915_request *rq,
+ bool write)
+{
+ if (obj->cache_dirty & ~obj->cache_coherent)
+ i915_gem_clflush_object(obj, 0);
+
+ return i915_request_await_object(rq, obj, write);
+}
+
int i915_gem_object_fill_blt(struct drm_i915_gem_object *obj,
struct intel_context *ce,
u32 value)
@@ -143,12 +154,6 @@ int i915_gem_object_fill_blt(struct
drm_i915_gem_object *obj,
if (unlikely(err))
return err;
- if (obj->cache_dirty & ~obj->cache_coherent) {
- i915_gem_object_lock(obj);
- i915_gem_clflush_object(obj, 0);
- i915_gem_object_unlock(obj);
- }
-
batch = intel_emit_vma_fill_blt(ce, vma, value);
if (IS_ERR(batch)) {
err = PTR_ERR(batch);
@@ -165,27 +170,22 @@ int i915_gem_object_fill_blt(struct
drm_i915_gem_object *obj,
if (unlikely(err))
goto out_request;
- err = i915_request_await_object(rq, obj, true);
- if (unlikely(err))
- goto out_request;
-
- if (ce->engine->emit_init_breadcrumb) {
- err = ce->engine->emit_init_breadcrumb(rq);
- if (unlikely(err))
- goto out_request;
- }
-
i915_vma_lock(vma);
- err = i915_request_await_object(rq, vma->obj, true);
+ err = move_obj_to_gpu(vma->obj, rq, true);
if (err == 0)
err = i915_vma_move_to_active(vma, rq,
EXEC_OBJECT_WRITE);
i915_vma_unlock(vma);
if (unlikely(err))
goto out_request;
- err = ce->engine->emit_bb_start(rq,
- batch->node.start, batch->node.size,
- 0);
+ if (ce->engine->emit_init_breadcrumb)
+ err = ce->engine->emit_init_breadcrumb(rq);
+
+ if (likely(!err))
+ err = ce->engine->emit_bb_start(rq,
+ batch->node.start,
+ batch->node.size,
+ 0);
out_request:
if (unlikely(err))
i915_request_set_error_once(rq, err);
@@ -317,16 +317,6 @@ struct i915_vma *intel_emit_vma_copy_blt(struct
intel_context *ce,
return ERR_PTR(err);
}
-static int move_to_gpu(struct i915_vma *vma, struct i915_request *rq, bool
write)
-{
- struct drm_i915_gem_object *obj = vma->obj;
-
- if (obj->cache_dirty & ~obj->cache_coherent)
- i915_gem_clflush_object(obj, 0);
-
- return i915_request_await_object(rq, obj, write);
-}
-
int i915_gem_object_copy_blt(struct drm_i915_gem_object *src,
struct drm_i915_gem_object *dst,
struct intel_context *ce)
@@ -375,7 +365,7 @@ int i915_gem_object_copy_blt(struct
drm_i915_gem_object *src,
goto out_request;
for (i = 0; i < ARRAY_SIZE(vma); i++) {
- err = move_to_gpu(vma[i], rq, i);
+ err = move_obj_to_gpu(vma[i]->obj, rq, i);
if (unlikely(err))
goto out_unlock;
Re: [Intel-gfx] [PATCH v2] drm/i915: Remove redundant i915_request_await_object in blit clears
>-Original Message-
>From: Tvrtko Ursulin
>Sent: Monday, June 15, 2020 11:15 AM
>To: Intel-gfx@lists.freedesktop.org
>Cc: Ursulin, Tvrtko ; Auld, Matthew
>; Chris Wilson ; Ruhl,
>Michael J
>Subject: [PATCH v2] drm/i915: Remove redundant i915_request_await_object
>in blit clears
>
>From: Tvrtko Ursulin
>
>One i915_request_await_object is enough and we keep the one under the
>object lock so it is final.
>
>At the same time move async clflushing setup under the same locked
>section and consolidate common code into a helper function.
>
>v2:
> * Emit initial breadcrumbs after aways are set up. (Chris)
>
>Signed-off-by: Tvrtko Ursulin
>Cc: Matthew Auld
>Cc: Chris Wilson
>Cc: Michael J. Ruhl
>---
> .../gpu/drm/i915/gem/i915_gem_object_blt.c| 52 ---
> 1 file changed, 21 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>index f457d7130491..bfdb32d46877 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
>@@ -126,6 +126,17 @@ void intel_emit_vma_release(struct intel_context
>*ce, struct i915_vma *vma)
> intel_engine_pm_put(ce->engine);
> }
>
>+static int
>+move_obj_to_gpu(struct drm_i915_gem_object *obj,
I am not understanding the name of this function.
How is the object moved to the gpu? Is clflush a move? Or is
it that it is moving to the gpu domain?
What about:
obj_flush_and_wait()
or just:
flush_and_wait()
?
Or am I missing something? 😊
Mike
>+ struct i915_request *rq,
>+ bool write)
>+{
>+ if (obj->cache_dirty & ~obj->cache_coherent)
>+ i915_gem_clflush_object(obj, 0);
>+
>+ return i915_request_await_object(rq, obj, write);
>+}
>+
> int i915_gem_object_fill_blt(struct drm_i915_gem_object *obj,
>struct intel_context *ce,
>u32 value)
>@@ -143,12 +154,6 @@ int i915_gem_object_fill_blt(struct
>drm_i915_gem_object *obj,
> if (unlikely(err))
> return err;
>
>- if (obj->cache_dirty & ~obj->cache_coherent) {
>- i915_gem_object_lock(obj);
>- i915_gem_clflush_object(obj, 0);
>- i915_gem_object_unlock(obj);
>- }
>-
> batch = intel_emit_vma_fill_blt(ce, vma, value);
> if (IS_ERR(batch)) {
> err = PTR_ERR(batch);
>@@ -165,27 +170,22 @@ int i915_gem_object_fill_blt(struct
>drm_i915_gem_object *obj,
> if (unlikely(err))
> goto out_request;
>
>- err = i915_request_await_object(rq, obj, true);
>- if (unlikely(err))
>- goto out_request;
>-
>- if (ce->engine->emit_init_breadcrumb) {
>- err = ce->engine->emit_init_breadcrumb(rq);
>- if (unlikely(err))
>- goto out_request;
>- }
>-
> i915_vma_lock(vma);
>- err = i915_request_await_object(rq, vma->obj, true);
>+ err = move_obj_to_gpu(vma->obj, rq, true);
> if (err == 0)
> err = i915_vma_move_to_active(vma, rq,
>EXEC_OBJECT_WRITE);
> i915_vma_unlock(vma);
> if (unlikely(err))
> goto out_request;
>
>- err = ce->engine->emit_bb_start(rq,
>- batch->node.start, batch->node.size,
>- 0);
>+ if (ce->engine->emit_init_breadcrumb)
>+ err = ce->engine->emit_init_breadcrumb(rq);
>+
>+ if (likely(!err))
>+ err = ce->engine->emit_bb_start(rq,
>+ batch->node.start,
>+ batch->node.size,
>+ 0);
> out_request:
> if (unlikely(err))
> i915_request_set_error_once(rq, err);
>@@ -317,16 +317,6 @@ struct i915_vma *intel_emit_vma_copy_blt(struct
>intel_context *ce,
> return ERR_PTR(err);
> }
>
>-static int move_to_gpu(struct i915_vma *vma, struct i915_request *rq, bool
>write)
>-{
>- struct drm_i915_gem_object *obj = vma->obj;
>-
>- if (obj->cache_dirty & ~obj->cache_coherent)
>- i915_gem_clflush_object(obj, 0);
>-
>- return i915_request_await_object(rq, obj, write);
>-}
>-
> int i915_gem_object_copy_blt(struct drm_i915_gem_object *src,
>struct drm_i915_gem_object *dst,
>struct intel_context *ce)
>@@ -375,7 +365,7 @@ int i915_gem_object_copy_blt(struct
>drm_i915_gem_object *src,
> goto out_request;
>
> for (i = 0; i < ARRAY_SIZE(vma); i++) {
>- err = move_to_gpu(vma[i], rq, i);
>+ err = move_obj_to_gpu(vma[i]->obj, rq, i);
> if (unlikely(err))
> goto out_unlock;
> }
>--
>2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.
[Intel-gfx] [PATCH v2] drm/i915: Remove redundant i915_request_await_object in blit clears
From: Tvrtko Ursulin
One i915_request_await_object is enough and we keep the one under the
object lock so it is final.
At the same time move async clflushing setup under the same locked
section and consolidate common code into a helper function.
v2:
* Emit initial breadcrumbs after aways are set up. (Chris)
Signed-off-by: Tvrtko Ursulin
Cc: Matthew Auld
Cc: Chris Wilson
Cc: Michael J. Ruhl
---
.../gpu/drm/i915/gem/i915_gem_object_blt.c| 52 ---
1 file changed, 21 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
index f457d7130491..bfdb32d46877 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
@@ -126,6 +126,17 @@ void intel_emit_vma_release(struct intel_context *ce,
struct i915_vma *vma)
intel_engine_pm_put(ce->engine);
}
+static int
+move_obj_to_gpu(struct drm_i915_gem_object *obj,
+ struct i915_request *rq,
+ bool write)
+{
+ if (obj->cache_dirty & ~obj->cache_coherent)
+ i915_gem_clflush_object(obj, 0);
+
+ return i915_request_await_object(rq, obj, write);
+}
+
int i915_gem_object_fill_blt(struct drm_i915_gem_object *obj,
struct intel_context *ce,
u32 value)
@@ -143,12 +154,6 @@ int i915_gem_object_fill_blt(struct drm_i915_gem_object
*obj,
if (unlikely(err))
return err;
- if (obj->cache_dirty & ~obj->cache_coherent) {
- i915_gem_object_lock(obj);
- i915_gem_clflush_object(obj, 0);
- i915_gem_object_unlock(obj);
- }
-
batch = intel_emit_vma_fill_blt(ce, vma, value);
if (IS_ERR(batch)) {
err = PTR_ERR(batch);
@@ -165,27 +170,22 @@ int i915_gem_object_fill_blt(struct drm_i915_gem_object
*obj,
if (unlikely(err))
goto out_request;
- err = i915_request_await_object(rq, obj, true);
- if (unlikely(err))
- goto out_request;
-
- if (ce->engine->emit_init_breadcrumb) {
- err = ce->engine->emit_init_breadcrumb(rq);
- if (unlikely(err))
- goto out_request;
- }
-
i915_vma_lock(vma);
- err = i915_request_await_object(rq, vma->obj, true);
+ err = move_obj_to_gpu(vma->obj, rq, true);
if (err == 0)
err = i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE);
i915_vma_unlock(vma);
if (unlikely(err))
goto out_request;
- err = ce->engine->emit_bb_start(rq,
- batch->node.start, batch->node.size,
- 0);
+ if (ce->engine->emit_init_breadcrumb)
+ err = ce->engine->emit_init_breadcrumb(rq);
+
+ if (likely(!err))
+ err = ce->engine->emit_bb_start(rq,
+ batch->node.start,
+ batch->node.size,
+ 0);
out_request:
if (unlikely(err))
i915_request_set_error_once(rq, err);
@@ -317,16 +317,6 @@ struct i915_vma *intel_emit_vma_copy_blt(struct
intel_context *ce,
return ERR_PTR(err);
}
-static int move_to_gpu(struct i915_vma *vma, struct i915_request *rq, bool
write)
-{
- struct drm_i915_gem_object *obj = vma->obj;
-
- if (obj->cache_dirty & ~obj->cache_coherent)
- i915_gem_clflush_object(obj, 0);
-
- return i915_request_await_object(rq, obj, write);
-}
-
int i915_gem_object_copy_blt(struct drm_i915_gem_object *src,
struct drm_i915_gem_object *dst,
struct intel_context *ce)
@@ -375,7 +365,7 @@ int i915_gem_object_copy_blt(struct drm_i915_gem_object
*src,
goto out_request;
for (i = 0; i < ARRAY_SIZE(vma); i++) {
- err = move_to_gpu(vma[i], rq, i);
+ err = move_obj_to_gpu(vma[i]->obj, rq, i);
if (unlikely(err))
goto out_unlock;
}
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
