[PATCH] drm/i915/guc: Do not use wait_for_atomic in host2guc_action

[PATCH] drm/i915/guc: Do not use wait_for_atomic in host2guc_action

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

host2guc_action does not appear to be called from atomic context
so a more polite wait_for macro should be used. Especially since
the timeout is 10ms.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reported-by: Imre Deak <imre.deak@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 355b6475e74c..fcc7982f8141 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -98,7 +98,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
 	I915_WRITE(HOST2GUC_INTERRUPT, HOST2GUC_TRIGGER);
 
 	/* No HOST2GUC command should take longer than 10ms */
-	ret = wait_for_atomic(host2guc_action_response(dev_priv, &status), 10);
+	ret = wait_for(host2guc_action_response(dev_priv, &status), 10);
 	if (status != GUC2HOST_STATUS_SUCCESS) {
 		/*
 		 * Either the GuC explicitly returned an error (which
-- 
1.9.1

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

✗ Ro.CI.BAT: failure for drm/i915/guc: Do not use wait_for_atomic in host2guc_action

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Do not use wait_for_atomic in host2guc_action
URL   : https://patchwork.freedesktop.org/series/9233/
State : failure

== Summary ==

Series 9233v1 drm/i915/guc: Do not use wait_for_atomic in host2guc_action
http://patchwork.freedesktop.org/api/1.0/series/9233/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-c:
                pass       -> SKIP       (fi-skl-i5-6260u)
        Subgroup suspend-read-crc-pipe-b:
                skip       -> DMESG-WARN (ro-bdw-i7-5557U)
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)

fi-hsw-i7-4770k  total:203  pass:182  dwarn:0   dfail:0   fail:2   skip:18 
fi-skl-i5-6260u  total:229  pass:201  dwarn:0   dfail:0   fail:2   skip:26 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:4   dfail:1   fail:2   skip:20 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:2   dfail:1   fail:2   skip:22 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:176  dwarn:0   dfail:1   fail:3   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1321/

3a50b42 drm-intel-nightly: 2016y-06m-28d-11h-06m-59s UTC integration manifest
937b35d drm/i915/guc: Do not use wait_for_atomic in host2guc_action

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

Re: [PATCH] drm/i915/guc: Do not use wait_for_atomic in host2guc_action

[Dave Gordon]

On 28/06/16 15:30, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> host2guc_action does not appear to be called from atomic context
> so a more polite wait_for macro should be used. Especially since
> the timeout is 10ms.

Maybe. However we don't really want to sleep if the action takes only a 
few microseconds, which some of them do. Maybe we need an optimistic 
spin for a few uS to handle the quick commands followed by a sleeping 
wait in the cases where it takes long enough that we don't mind the 
extra wakeup latency?

.Dave.

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reported-by: Imre Deak <imre.deak@intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 355b6475e74c..fcc7982f8141 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -98,7 +98,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
>   	I915_WRITE(HOST2GUC_INTERRUPT, HOST2GUC_TRIGGER);
>
>   	/* No HOST2GUC command should take longer than 10ms */
> -	ret = wait_for_atomic(host2guc_action_response(dev_priv, &status), 10);
> +	ret = wait_for(host2guc_action_response(dev_priv, &status), 10);
>   	if (status != GUC2HOST_STATUS_SUCCESS) {
>   		/*
>   		 * Either the GuC explicitly returned an error (which
>

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

Re: [PATCH] drm/i915/guc: Do not use wait_for_atomic in host2guc_action

[Tvrtko Ursulin]

On 28/06/16 16:50, Dave Gordon wrote:
> On 28/06/16 15:30, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> host2guc_action does not appear to be called from atomic context
>> so a more polite wait_for macro should be used. Especially since
>> the timeout is 10ms.
>
> Maybe. However we don't really want to sleep if the action takes only a
> few microseconds, which some of them do. Maybe we need an optimistic
> spin for a few uS to handle the quick commands followed by a sleeping
> wait in the cases where it takes long enough that we don't mind the
> extra wakeup latency?

I believe Mika is working on smart waits similar to what you describe. 
Adaptive back-off etc.

In the meantime how about calling _wait_for directly and setting the 
re-check period to something you think is more appropriate for the GuC?

Shouldn't be less than 10us since usleep_range recommends against it.

Regards,

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