diff mbox

[2/2] drm/i915: Avoid decomposing a signal-on-any fence-array

Message ID 20170217183536.26100-2-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Feb. 17, 2017, 6:35 p.m. UTC
The code currently assumes that all fence arrays it sees are the normal
signal-on-all variety, and decomposes the array into its individual
fences so that it can extract the native i915 fences. If the fence array
is using signal-on-any, we should not decompose as we must not wait on
them all, just the first in *that* set.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Joonas Lahtinen Feb. 20, 2017, 11:32 a.m. UTC | #1
On pe, 2017-02-17 at 18:35 +0000, Chris Wilson wrote:
> The code currently assumes that all fence arrays it sees are the normal
> signal-on-all variety, and decomposes the array into its individual
> fences so that it can extract the native i915 fences. If the fence array
> is using signal-on-any, we should not decompose as we must not wait on
> them all, just the first in *that* set.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

<SNIP>

> 
> @@ -696,7 +696,8 @@ i915_gem_request_await_dma_fence(struct drm_i915_gem_request *req,
>  	if (dma_fence_is_i915(fence))
>  		return i915_gem_request_await_request(req, to_request(fence));
>  
> -	if (!dma_fence_is_array(fence)) {
> +	if (!dma_fence_is_array(fence) ||
> +	    test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &fence->flags)) {

Smells like a helper function? While that helper is finding the way
upstream;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
Chris Wilson Feb. 20, 2017, 12:03 p.m. UTC | #2
On Mon, Feb 20, 2017 at 01:32:42PM +0200, Joonas Lahtinen wrote:
> On pe, 2017-02-17 at 18:35 +0000, Chris Wilson wrote:
> > The code currently assumes that all fence arrays it sees are the normal
> > signal-on-all variety, and decomposes the array into its individual
> > fences so that it can extract the native i915 fences. If the fence array
> > is using signal-on-any, we should not decompose as we must not wait on
> > them all, just the first in *that* set.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> <SNIP>
> 
> > 
> > @@ -696,7 +696,8 @@ i915_gem_request_await_dma_fence(struct drm_i915_gem_request *req,
> >  	if (dma_fence_is_i915(fence))
> >  		return i915_gem_request_await_request(req, to_request(fence));
> >  
> > -	if (!dma_fence_is_array(fence)) {
> > +	if (!dma_fence_is_array(fence) ||
> > +	    test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &fence->flags)) {
> 
> Smells like a helper function? While that helper is finding the way
> upstream;

Blurgh.

enum dma_fence_array_signal_mode {
        DMA_FENCE_ARRAY_SIGNAL_ON_ALL = 0,
        DMA_FENCE_ARRAY_SIGNAL_ON_ANY,
};

enum dma_fence_array_signal_mode
dma_fence_array_get_signaling_mode(struct dma_fence_array *array)
{       
        return test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &array->base.flags) ?
                DMA_FENCE_ARRAY_SIGNAL_ON_ANY : DMA_FENCE_ARRAY_SIGNAL_ON_ALL;
}

if (!dma_fence_is_array(fence) ||
    dma_fence_array_get_signaling_mode(to_dma_fence_array(fence)) == DMA_FENCE_ARRAY_SIGNAL_ON_ANY)

Hmm. Not happy with that yet.
-Chris
Maarten Lankhorst Feb. 20, 2017, 12:12 p.m. UTC | #3
Op 20-02-17 om 13:03 schreef Chris Wilson:
> On Mon, Feb 20, 2017 at 01:32:42PM +0200, Joonas Lahtinen wrote:
>> On pe, 2017-02-17 at 18:35 +0000, Chris Wilson wrote:
>>> The code currently assumes that all fence arrays it sees are the normal
>>> signal-on-all variety, and decomposes the array into its individual
>>> fences so that it can extract the native i915 fences. If the fence array
>>> is using signal-on-any, we should not decompose as we must not wait on
>>> them all, just the first in *that* set.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> <SNIP>
>>
>>> @@ -696,7 +696,8 @@ i915_gem_request_await_dma_fence(struct drm_i915_gem_request *req,
>>>  	if (dma_fence_is_i915(fence))
>>>  		return i915_gem_request_await_request(req, to_request(fence));
>>>  
>>> -	if (!dma_fence_is_array(fence)) {
>>> +	if (!dma_fence_is_array(fence) ||
>>> +	    test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &fence->flags)) {
>> Smells like a helper function? While that helper is finding the way
>> upstream;
> Blurgh.
>
> enum dma_fence_array_signal_mode {
>         DMA_FENCE_ARRAY_SIGNAL_ON_ALL = 0,
>         DMA_FENCE_ARRAY_SIGNAL_ON_ANY,
> };
>
> enum dma_fence_array_signal_mode
> dma_fence_array_get_signaling_mode(struct dma_fence_array *array)
> {       
>         return test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &array->base.flags) ?
>                 DMA_FENCE_ARRAY_SIGNAL_ON_ANY : DMA_FENCE_ARRAY_SIGNAL_ON_ALL;
> }
>
> if (!dma_fence_is_array(fence) ||
>     dma_fence_array_get_signaling_mode(to_dma_fence_array(fence)) == DMA_FENCE_ARRAY_SIGNAL_ON_ANY)
>
> Hmm. Not happy with that yet.
> -Chris
>
bool dma_fence_array_signal_on_any with a WARN_ON(!dma_fence_is_array(fence))?
Joonas Lahtinen Feb. 21, 2017, 8:53 a.m. UTC | #4
On ma, 2017-02-20 at 13:12 +0100, Maarten Lankhorst wrote:
> Op 20-02-17 om 13:03 schreef Chris Wilson:
> > 
> > On Mon, Feb 20, 2017 at 01:32:42PM +0200, Joonas Lahtinen wrote:
> > > 
> > > Smells like a helper function? While that helper is finding the way
> > > upstream;
> > Blurgh.
> > 
> > enum dma_fence_array_signal_mode {
> >         DMA_FENCE_ARRAY_SIGNAL_ON_ALL = 0,
> >         DMA_FENCE_ARRAY_SIGNAL_ON_ANY,
> > };
> > 
> > enum dma_fence_array_signal_mode
> > dma_fence_array_get_signaling_mode(struct dma_fence_array *array)
> > {       
> >         return test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &array->base.flags) ?
> >                 DMA_FENCE_ARRAY_SIGNAL_ON_ANY : DMA_FENCE_ARRAY_SIGNAL_ON_ALL;
> > }
> > 
> > if (!dma_fence_is_array(fence) ||
> >     dma_fence_array_get_signaling_mode(to_dma_fence_array(fence)) == DMA_FENCE_ARRAY_SIGNAL_ON_ANY)
> > 
> > Hmm. Not happy with that yet.
> > -Chris
> > 
> bool dma_fence_array_signal_on_any with a WARN_ON(!dma_fence_is_array(fence))?

I was also thinking along these lines.

Regards, Joonas
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 2f6cfa47dc61..2ab96c35cc5e 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -696,7 +696,8 @@  i915_gem_request_await_dma_fence(struct drm_i915_gem_request *req,
 	if (dma_fence_is_i915(fence))
 		return i915_gem_request_await_request(req, to_request(fence));
 
-	if (!dma_fence_is_array(fence)) {
+	if (!dma_fence_is_array(fence) ||
+	    test_bit(DMA_FENCE_ARRAY_SIGNAL_ANY, &fence->flags)) {
 		ret = i915_sw_fence_await_dma_fence(&req->submit,
 						    fence, I915_FENCE_TIMEOUT,
 						    GFP_KERNEL);