Re: [Intel-gfx] [PATCH] drm/i915/selftests: ring all doorbells in igt_guc_doorbells
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 1a0f2a39cef9..8382d591c784 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -49,6 +49,7 @@ #define WQ_TYPE_BATCH_BUF (0x1 << WQ_TYPE_SHIFT) #define WQ_TYPE_PSEUDO (0x2 << WQ_TYPE_SHIFT) #define WQ_TYPE_INORDER (0x3 << WQ_TYPE_SHIFT) +#define WQ_TYPE_NOOP (0x4 << WQ_TYPE_SHIFT) I got general question to this ^ defines. Do I correctly see that PSEUDO and BATCH_BUF are not used anywhere? Yes they're unused. I'm assuming when the FW support was first added to i915 we just defined all the cases available in the FW at the time, even if we didn't actually use them. Personally I think it is worth keeping them as having only part of the modes defined would be less clear IMO. Daniele ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/selftests: ring all doorbells in igt_guc_doorbells
@@ -465,15 +468,19 @@ static void guc_wq_item_append(struct
intel_guc_client *client,
/* WQ starts from the page after doorbell / process_desc */
wqi = client->vaddr + wq_off + GUC_DB_SIZE;
- /* Now fill in the 4-word work queue item */
- wqi->header = WQ_TYPE_INORDER |
- (wqi_len << WQ_LEN_SHIFT) |
- (target_engine << WQ_TARGET_SHIFT) |
- WQ_NO_WCFLUSH_WAIT;
- wqi->context_desc = context_desc;
- wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
- GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
- wqi->fence_id = fence_id;
+ if (I915_SELFTEST_ONLY(client->use_nop_wqi)) {
+ wqi->header = WQ_TYPE_NOOP | (wqi_len << WQ_LEN_SHIFT);
Note to self, this is WQ_NOOP + three u32 with 0's
Actually we don't care what's in the 3 u32 following the header. When
WQ_TYPE_NOOP is used GuC will just bump the WQ head based on the
provided length and then move to the next request, so it is going to
jump over them.
Daniele
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/selftests: ring all doorbells in igt_guc_doorbells
Quoting Katarzyna Dec (2018-08-28 13:34:16) > On Mon, Aug 27, 2018 at 03:36:14PM -0700, Daniele Ceraolo Spurio wrote: > > We currently verify that all doorbells can be registerd with GuC and > > HW but don't check that all works as expected after a db ring. > > > > Do a nop ring of all doorbells to make sure we haven't misprogrammed > > any WQ or stage descriptor data. This will also help validating > > upcoming changes in the db programming flow. > > > > Cc: Michel Thierry > > Cc: Michal Wajdeczko > > Signed-off-by: Daniele Ceraolo Spurio [snip] > Selftests are new topic for me, but this one looks fairly simple. I hope > I understand it correctly. > Acked-by: Katarzyna Dec Fixed up the spelling mistake and pushed. Thanks for the review and more testing, -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/selftests: ring all doorbells in igt_guc_doorbells
On Mon, Aug 27, 2018 at 03:36:14PM -0700, Daniele Ceraolo Spurio wrote:
> We currently verify that all doorbells can be registerd with GuC and
> HW but don't check that all works as expected after a db ring.
>
> Do a nop ring of all doorbells to make sure we haven't misprogrammed
> any WQ or stage descriptor data. This will also help validating
> upcoming changes in the db programming flow.
>
> Cc: Michel Thierry
> Cc: Michal Wajdeczko
> Signed-off-by: Daniele Ceraolo Spurio
> ---
> drivers/gpu/drm/i915/intel_guc_fwif.h | 1 +
> drivers/gpu/drm/i915/intel_guc_submission.c | 25 +-
> drivers/gpu/drm/i915/intel_guc_submission.h | 4 +++
> drivers/gpu/drm/i915/selftests/intel_guc.c | 38 +
> 4 files changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
> b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 1a0f2a39cef9..8382d591c784 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -49,6 +49,7 @@
> #define WQ_TYPE_BATCH_BUF (0x1 << WQ_TYPE_SHIFT)
> #define WQ_TYPE_PSEUDO (0x2 << WQ_TYPE_SHIFT)
> #define WQ_TYPE_INORDER(0x3 << WQ_TYPE_SHIFT)
> +#define WQ_TYPE_NOOP (0x4 << WQ_TYPE_SHIFT)
I got general question to this ^ defines. Do I correctly see that PSEUDO and
BATCH_BUF are not
used anywhere?
> #define WQ_TARGET_SHIFT 10
> #define WQ_LEN_SHIFT 16
> #define WQ_NO_WCFLUSH_WAIT (1 << 27)
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 195adbd0ebf7..07b9d313b019 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -456,6 +456,9 @@ static void guc_wq_item_append(struct intel_guc_client
> *client,
>*/
> BUILD_BUG_ON(wqi_size != 16);
>
> + /* We expect the WQ to be active if we're appending items to it */
> + GEM_BUG_ON(desc->wq_status != WQ_STATUS_ACTIVE);
> +
> /* Free space is guaranteed. */
> wq_off = READ_ONCE(desc->tail);
> GEM_BUG_ON(CIRC_SPACE(wq_off, READ_ONCE(desc->head),
> @@ -465,15 +468,19 @@ static void guc_wq_item_append(struct intel_guc_client
> *client,
> /* WQ starts from the page after doorbell / process_desc */
> wqi = client->vaddr + wq_off + GUC_DB_SIZE;
>
> - /* Now fill in the 4-word work queue item */
> - wqi->header = WQ_TYPE_INORDER |
> - (wqi_len << WQ_LEN_SHIFT) |
> - (target_engine << WQ_TARGET_SHIFT) |
> - WQ_NO_WCFLUSH_WAIT;
> - wqi->context_desc = context_desc;
> - wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
> - GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
> - wqi->fence_id = fence_id;
> + if (I915_SELFTEST_ONLY(client->use_nop_wqi)) {
> + wqi->header = WQ_TYPE_NOOP | (wqi_len << WQ_LEN_SHIFT);
> + } else {
> + /* Now fill in the 4-word work queue item */
> + wqi->header = WQ_TYPE_INORDER |
> + (wqi_len << WQ_LEN_SHIFT) |
> + (target_engine << WQ_TARGET_SHIFT) |
> + WQ_NO_WCFLUSH_WAIT;
> + wqi->context_desc = context_desc;
> + wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
> + GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
> + wqi->fence_id = fence_id;
> + }
>
> /* Make the update visible to GuC */
> WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1));
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h
> b/drivers/gpu/drm/i915/intel_guc_submission.h
> index fb081cefef93..169c54568340 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.h
> @@ -28,6 +28,7 @@
> #include
>
> #include "i915_gem.h"
> +#include "i915_selftest.h"
>
> struct drm_i915_private;
>
> @@ -71,6 +72,9 @@ struct intel_guc_client {
> spinlock_t wq_lock;
> /* Per-engine counts of GuC submissions */
> u64 submissions[I915_NUM_ENGINES];
> +
> + /* For testing purposes, use nop WQ items instead of real ones */
> + I915_SELFTEST_DECLARE(bool use_nop_wqi);
> };
>
> int intel_guc_submission_init(struct intel_guc *guc);
> diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c
> b/drivers/gpu/drm/i915/selftests/intel_guc.c
> index 407c98fb9170..3154fd2f625d 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_guc.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
> @@ -65,6 +65,40 @@ static int check_all_doorbells(struct intel_guc *guc)
> return 0;
> }
>
> +static int ring_doorbell_nop(struct intel_guc_client *client)
> +{
> + int err;
> + struct guc_process_desc *desc = __get_process_desc(client);
> +
> + client->use_nop_wqi = true;
> +
> +
Re: [Intel-gfx] [PATCH] drm/i915/selftests: ring all doorbells in igt_guc_doorbells
On 8/27/2018 3:36 PM, Daniele Ceraolo Spurio wrote:
We currently verify that all doorbells can be registerd with GuC and
^registered
HW but don't check that all works as expected after a db ring.
Do a nop ring of all doorbells to make sure we haven't misprogrammed
any WQ or stage descriptor data. This will also help validating
upcoming changes in the db programming flow.
Cc: Michel Thierry
Cc: Michal Wajdeczko
Signed-off-by: Daniele Ceraolo Spurio
---
drivers/gpu/drm/i915/intel_guc_fwif.h | 1 +
drivers/gpu/drm/i915/intel_guc_submission.c | 25 +-
drivers/gpu/drm/i915/intel_guc_submission.h | 4 +++
drivers/gpu/drm/i915/selftests/intel_guc.c | 38 +
4 files changed, 59 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 1a0f2a39cef9..8382d591c784 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -49,6 +49,7 @@
#define WQ_TYPE_BATCH_BUF (0x1 << WQ_TYPE_SHIFT)
#define WQ_TYPE_PSEUDO (0x2 << WQ_TYPE_SHIFT)
#define WQ_TYPE_INORDER (0x3 << WQ_TYPE_SHIFT)
+#define WQ_TYPE_NOOP (0x4 << WQ_TYPE_SHIFT)
#define WQ_TARGET_SHIFT 10
#define WQ_LEN_SHIFT 16
#define WQ_NO_WCFLUSH_WAIT(1 << 27)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 195adbd0ebf7..07b9d313b019 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -456,6 +456,9 @@ static void guc_wq_item_append(struct intel_guc_client
*client,
*/
BUILD_BUG_ON(wqi_size != 16);
+ /* We expect the WQ to be active if we're appending items to it */
+ GEM_BUG_ON(desc->wq_status != WQ_STATUS_ACTIVE);
+
/* Free space is guaranteed. */
wq_off = READ_ONCE(desc->tail);
GEM_BUG_ON(CIRC_SPACE(wq_off, READ_ONCE(desc->head),
@@ -465,15 +468,19 @@ static void guc_wq_item_append(struct intel_guc_client
*client,
/* WQ starts from the page after doorbell / process_desc */
wqi = client->vaddr + wq_off + GUC_DB_SIZE;
- /* Now fill in the 4-word work queue item */
- wqi->header = WQ_TYPE_INORDER |
- (wqi_len << WQ_LEN_SHIFT) |
- (target_engine << WQ_TARGET_SHIFT) |
- WQ_NO_WCFLUSH_WAIT;
- wqi->context_desc = context_desc;
- wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
- GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
- wqi->fence_id = fence_id;
+ if (I915_SELFTEST_ONLY(client->use_nop_wqi)) {
+ wqi->header = WQ_TYPE_NOOP | (wqi_len << WQ_LEN_SHIFT);
Note to self, this is WQ_NOOP + three u32 with 0's
+ } else {
+ /* Now fill in the 4-word work queue item */
+ wqi->header = WQ_TYPE_INORDER |
+ (wqi_len << WQ_LEN_SHIFT) |
+ (target_engine << WQ_TARGET_SHIFT) |
+ WQ_NO_WCFLUSH_WAIT;
+ wqi->context_desc = context_desc;
+ wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
+ GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
+ wqi->fence_id = fence_id;
+ }
/* Make the update visible to GuC */
WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1));
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h
b/drivers/gpu/drm/i915/intel_guc_submission.h
index fb081cefef93..169c54568340 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
@@ -28,6 +28,7 @@
#include
#include "i915_gem.h"
+#include "i915_selftest.h"
struct drm_i915_private;
@@ -71,6 +72,9 @@ struct intel_guc_client {
spinlock_t wq_lock;
/* Per-engine counts of GuC submissions */
u64 submissions[I915_NUM_ENGINES];
+
+ /* For testing purposes, use nop WQ items instead of real ones */
+ I915_SELFTEST_DECLARE(bool use_nop_wqi);
};
int intel_guc_submission_init(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c
b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 407c98fb9170..3154fd2f625d 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -65,6 +65,40 @@ static int check_all_doorbells(struct intel_guc *guc)
return 0;
}
+static int ring_doorbell_nop(struct intel_guc_client *client)
+{
+ int err;
+ struct guc_process_desc *desc = __get_process_desc(client);
+
+ client->use_nop_wqi = true;
+
+ spin_lock_irq(>wq_lock);
+
+ guc_wq_item_append(client, 0, 0, 0, 0);
+ guc_ring_doorbell(client);
+
+ spin_unlock_irq(>wq_lock);
+
+
[Intel-gfx] [PATCH] drm/i915/selftests: ring all doorbells in igt_guc_doorbells
We currently verify that all doorbells can be registerd with GuC and
HW but don't check that all works as expected after a db ring.
Do a nop ring of all doorbells to make sure we haven't misprogrammed
any WQ or stage descriptor data. This will also help validating
upcoming changes in the db programming flow.
Cc: Michel Thierry
Cc: Michal Wajdeczko
Signed-off-by: Daniele Ceraolo Spurio
---
drivers/gpu/drm/i915/intel_guc_fwif.h | 1 +
drivers/gpu/drm/i915/intel_guc_submission.c | 25 +-
drivers/gpu/drm/i915/intel_guc_submission.h | 4 +++
drivers/gpu/drm/i915/selftests/intel_guc.c | 38 +
4 files changed, 59 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 1a0f2a39cef9..8382d591c784 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -49,6 +49,7 @@
#define WQ_TYPE_BATCH_BUF(0x1 << WQ_TYPE_SHIFT)
#define WQ_TYPE_PSEUDO (0x2 << WQ_TYPE_SHIFT)
#define WQ_TYPE_INORDER (0x3 << WQ_TYPE_SHIFT)
+#define WQ_TYPE_NOOP (0x4 << WQ_TYPE_SHIFT)
#define WQ_TARGET_SHIFT10
#define WQ_LEN_SHIFT 16
#define WQ_NO_WCFLUSH_WAIT (1 << 27)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 195adbd0ebf7..07b9d313b019 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -456,6 +456,9 @@ static void guc_wq_item_append(struct intel_guc_client
*client,
*/
BUILD_BUG_ON(wqi_size != 16);
+ /* We expect the WQ to be active if we're appending items to it */
+ GEM_BUG_ON(desc->wq_status != WQ_STATUS_ACTIVE);
+
/* Free space is guaranteed. */
wq_off = READ_ONCE(desc->tail);
GEM_BUG_ON(CIRC_SPACE(wq_off, READ_ONCE(desc->head),
@@ -465,15 +468,19 @@ static void guc_wq_item_append(struct intel_guc_client
*client,
/* WQ starts from the page after doorbell / process_desc */
wqi = client->vaddr + wq_off + GUC_DB_SIZE;
- /* Now fill in the 4-word work queue item */
- wqi->header = WQ_TYPE_INORDER |
- (wqi_len << WQ_LEN_SHIFT) |
- (target_engine << WQ_TARGET_SHIFT) |
- WQ_NO_WCFLUSH_WAIT;
- wqi->context_desc = context_desc;
- wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
- GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
- wqi->fence_id = fence_id;
+ if (I915_SELFTEST_ONLY(client->use_nop_wqi)) {
+ wqi->header = WQ_TYPE_NOOP | (wqi_len << WQ_LEN_SHIFT);
+ } else {
+ /* Now fill in the 4-word work queue item */
+ wqi->header = WQ_TYPE_INORDER |
+ (wqi_len << WQ_LEN_SHIFT) |
+ (target_engine << WQ_TARGET_SHIFT) |
+ WQ_NO_WCFLUSH_WAIT;
+ wqi->context_desc = context_desc;
+ wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
+ GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
+ wqi->fence_id = fence_id;
+ }
/* Make the update visible to GuC */
WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1));
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h
b/drivers/gpu/drm/i915/intel_guc_submission.h
index fb081cefef93..169c54568340 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
@@ -28,6 +28,7 @@
#include
#include "i915_gem.h"
+#include "i915_selftest.h"
struct drm_i915_private;
@@ -71,6 +72,9 @@ struct intel_guc_client {
spinlock_t wq_lock;
/* Per-engine counts of GuC submissions */
u64 submissions[I915_NUM_ENGINES];
+
+ /* For testing purposes, use nop WQ items instead of real ones */
+ I915_SELFTEST_DECLARE(bool use_nop_wqi);
};
int intel_guc_submission_init(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c
b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 407c98fb9170..3154fd2f625d 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -65,6 +65,40 @@ static int check_all_doorbells(struct intel_guc *guc)
return 0;
}
+static int ring_doorbell_nop(struct intel_guc_client *client)
+{
+ int err;
+ struct guc_process_desc *desc = __get_process_desc(client);
+
+ client->use_nop_wqi = true;
+
+ spin_lock_irq(>wq_lock);
+
+ guc_wq_item_append(client, 0, 0, 0, 0);
+ guc_ring_doorbell(client);
+
+ spin_unlock_irq(>wq_lock);
+
+ client->use_nop_wqi = false;
+
+ /* if there are no issues GuC will update the WQ head and keep the
+* WQ in active status
+*/
+ err =
