[PATCH] drm/i915: Check for an invalid seqno before __i915_gem_request_started

[PATCH] drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Chris Wilson]

__i915_gem_request_started() asserts that the seqno is valid, but
i915_spin_request() was not checking before querying whether the request
had started.

Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Fixes: 754c9fd57649 ("drm/i915: Protect the request->global_seqno with the engine->timeline lock")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 6fdfb801bcee..cb4e7f3e279e 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -348,6 +348,9 @@ static inline bool i915_spin_request(const struct drm_i915_gem_request *request,
 	u32 seqno;
 
 	seqno = i915_gem_request_global_seqno(request);
+	if (!seqno)
+		return 0;
+
 	return (__i915_gem_request_started(request, seqno) &&
 		__i915_spin_request(request, seqno, state, timeout_us));
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Michał Winiarski]

On Wed, Mar 08, 2017 at 02:22:38PM +0000, Chris Wilson wrote:
> __i915_gem_request_started() asserts that the seqno is valid, but
> i915_spin_request() was not checking before querying whether the request
> had started.
> 
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> Fixes: 754c9fd57649 ("drm/i915: Protect the request->global_seqno with the engine->timeline lock")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> ---
>  drivers/gpu/drm/i915/i915_gem_request.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 6fdfb801bcee..cb4e7f3e279e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -348,6 +348,9 @@ static inline bool i915_spin_request(const struct drm_i915_gem_request *request,
>  	u32 seqno;
>  
>  	seqno = i915_gem_request_global_seqno(request);
> +	if (!seqno)
> +		return 0;
> +
>  	return (__i915_gem_request_started(request, seqno) &&
>  		__i915_spin_request(request, seqno, state, timeout_us));
>  }
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Tvrtko Ursulin]

On 08/03/2017 14:22, Chris Wilson wrote:
> __i915_gem_request_started() asserts that the seqno is valid, but
> i915_spin_request() was not checking before querying whether the request
> had started.
>
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> Fixes: 754c9fd57649 ("drm/i915: Protect the request->global_seqno with the engine->timeline lock")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 6fdfb801bcee..cb4e7f3e279e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -348,6 +348,9 @@ static inline bool i915_spin_request(const struct drm_i915_gem_request *request,
>  	u32 seqno;
>
>  	seqno = i915_gem_request_global_seqno(request);
> +	if (!seqno)
> +		return 0;
> +
>  	return (__i915_gem_request_started(request, seqno) &&
>  		__i915_spin_request(request, seqno, state, timeout_us));
>  }
>

Looks equivalent to just calling i915_gem_request_started instead of 
__i915_gem_request_started.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Chris Wilson]

On Wed, Mar 08, 2017 at 02:45:16PM +0000, Tvrtko Ursulin wrote:
> 
> On 08/03/2017 14:22, Chris Wilson wrote:
> >__i915_gem_request_started() asserts that the seqno is valid, but
> >i915_spin_request() was not checking before querying whether the request
> >had started.
> >
> >Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> >Fixes: 754c9fd57649 ("drm/i915: Protect the request->global_seqno with the engine->timeline lock")
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Michał Winiarski <michal.winiarski@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_gem_request.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> >index 6fdfb801bcee..cb4e7f3e279e 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_request.h
> >+++ b/drivers/gpu/drm/i915/i915_gem_request.h
> >@@ -348,6 +348,9 @@ static inline bool i915_spin_request(const struct drm_i915_gem_request *request,
> > 	u32 seqno;
> >
> > 	seqno = i915_gem_request_global_seqno(request);
> >+	if (!seqno)
> >+		return 0;
> >+
> > 	return (__i915_gem_request_started(request, seqno) &&
> > 		__i915_spin_request(request, seqno, state, timeout_us));
> > }
> >
> 
> Looks equivalent to just calling i915_gem_request_started instead of
> __i915_gem_request_started.

We want to pass the same seqno onto __i915_spin_request, but yeah, very
similar.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Tvrtko Ursulin]

On 08/03/2017 14:50, Chris Wilson wrote:
> On Wed, Mar 08, 2017 at 02:45:16PM +0000, Tvrtko Ursulin wrote:
>>
>> On 08/03/2017 14:22, Chris Wilson wrote:
>>> __i915_gem_request_started() asserts that the seqno is valid, but
>>> i915_spin_request() was not checking before querying whether the request
>>> had started.
>>>
>>> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
>>> Fixes: 754c9fd57649 ("drm/i915: Protect the request->global_seqno with the engine->timeline lock")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem_request.h | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
>>> index 6fdfb801bcee..cb4e7f3e279e 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_request.h
>>> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
>>> @@ -348,6 +348,9 @@ static inline bool i915_spin_request(const struct drm_i915_gem_request *request,
>>> 	u32 seqno;
>>>
>>> 	seqno = i915_gem_request_global_seqno(request);
>>> +	if (!seqno)
>>> +		return 0;
>>> +
>>> 	return (__i915_gem_request_started(request, seqno) &&
>>> 		__i915_spin_request(request, seqno, state, timeout_us));
>>> }
>>>
>>
>> Looks equivalent to just calling i915_gem_request_started instead of
>> __i915_gem_request_started.
>
> We want to pass the same seqno onto __i915_spin_request, but yeah, very
> similar.

Ah yes, my bad.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Patchwork]

== Series Details ==

Series: drm/i915: Check for an invalid seqno before __i915_gem_request_started
URL   : https://patchwork.freedesktop.org/series/20911/
State : failure

== Summary ==

Series 20911v1 drm/i915: Check for an invalid seqno before __i915_gem_request_started
https://patchwork.freedesktop.org/api/1.0/series/20911/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
        Subgroup basic-batch-kernel-default-wb:
                pass       -> INCOMPLETE (fi-skl-6770hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                fail       -> PASS       (fi-kbl-7500u)
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-bxt-t5700)
                fail       -> DMESG-WARN (fi-kbl-7500u) fdo#100099
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-kbl-7500u)
        Subgroup suspend-read-crc-pipe-b:
                fail       -> PASS       (fi-kbl-7500u)
        Subgroup suspend-read-crc-pipe-c:
                fail       -> PASS       (fi-kbl-7500u) fdo#100044

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100099 https://bugs.freedesktop.org/show_bug.cgi?id=100099
fdo#100044 https://bugs.freedesktop.org/show_bug.cgi?id=100044

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 464s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 615s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 541s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 612s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 502s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 502s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 440s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 441s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 442s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 498s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 490s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 480s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 501s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 596s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 497s
fi-skl-6770hq    total:52   pass:50   dwarn:0   dfail:0   fail:0   skip:1   time: 0s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 556s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 415s

6f8585956c95605b9c797ffde8f65e408b8d1e33 drm-tip: 2017y-03m-08d-14h-47m-28s UTC integration manifest
6080cd1 drm/i915: Check for an invalid seqno before __i915_gem_request_started

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4100/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for drm/i915: Check for an invalid seqno before __i915_gem_request_started

[Chris Wilson]

On Wed, Mar 08, 2017 at 06:22:12PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Check for an invalid seqno before __i915_gem_request_started
> URL   : https://patchwork.freedesktop.org/series/20911/
> State : failure
> 
> == Summary ==
> 
> Series 20911v1 drm/i915: Check for an invalid seqno before __i915_gem_request_started
> https://patchwork.freedesktop.org/api/1.0/series/20911/revisions/1/mbox/
> 
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 fail       -> PASS       (fi-snb-2600) fdo#100007
>         Subgroup basic-batch-kernel-default-wb:
>                 pass       -> INCOMPLETE (fi-skl-6770hq)
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 fail       -> PASS       (fi-kbl-7500u)
>         Subgroup basic-s4-devices:
>                 dmesg-warn -> PASS       (fi-bxt-t5700)
>                 fail       -> DMESG-WARN (fi-kbl-7500u) fdo#100099
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-a:
>                 fail       -> PASS       (fi-kbl-7500u)
>         Subgroup suspend-read-crc-pipe-b:
>                 fail       -> PASS       (fi-kbl-7500u)
>         Subgroup suspend-read-crc-pipe-c:
>                 fail       -> PASS       (fi-kbl-7500u) fdo#100044

More random unrelated fails, thanks for the report & review, pushed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx