Re: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Dave Gordon]

On 06/05/16 09:55, Tvrtko Ursulin wrote:


On 05/05/16 19:38, Dave Gordon wrote:

On 29/04/2016 16:44, Tvrtko Ursulin wrote:


On 27/04/16 19:03, Dave Gordon wrote:

Mostly little optimisations; for instance, if the driver is correctly
following the submission protocol, the "out of space" condition is
impossible, so the previous runtime WARN_ON() is promoted to a
GEM_BUG_ON() for a more dramatic effect in development and less impact
in end-user systems.

Similarly we can replace other WARN_ON() conditions that don't
relate to
the hardware state with either BUILD_BUG_ON() for compile-time-
detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

Signed-off-by: Dave Gordon 
Cc: Chris Wilson 

---
  drivers/gpu/drm/i915/i915_guc_submission.c | 69
+++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
  3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct
drm_i915_gem_request *request)
  return -EAGAIN;
  }

-static int guc_add_workqueue_item(struct i915_guc_client *gc,
-  struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+   struct drm_i915_gem_request *rq)
  {
+/* wqi_len is in DWords, and does not include the one-word
header */
+const size_t wqi_size = sizeof(struct guc_wq_item);


Again, u32 is correct I think.

Nope, it's a sizeof(), but the compiler will check that it fits in u32
when we convert it to DWords below.


I already wrote in this same thread few days ago this was my oversight.


Yeah, I'm answering messages out of sequence :)


+const u32 wqi_len = wqi_size/sizeof(u32) - 1;
  struct guc_process_desc *desc;
  struct guc_wq_item *wqi;
  void *base;
-u32 tail, wq_len, wq_off, space;
+u32 space, tail, wq_off, wq_page;

  desc = gc->client_base + gc->proc_desc_offset;
+
+/* Space was checked earlier, in i915_guc_wq_check_space()
above */


It may be above in the file, but the two do not call one another so I
recommend saying exactly who called it.

>>>

I don't really mind who called it, as long as it was called sometime
earlier in the request submission protocol -- the callsite may get moved
around a bit. Use cscope(1) or your favourite IDE to find it.


So what does this comment supposed to tell the reader?


It tells the reader that we "cannot" be out of space, because we already 
checked that there was enough. Unless, of course, somebody removed the 
check. The commit message says that, too.



guc_add_workqueue_item does not call i915_guc_wq_check_space so "above"
what? Above in the file? Or maybe *earlier* in the call sequence of
what? Might as well put an useful comment in when you are adding one.


It already has both the words "earlier" (as in, before in time, in 
calling sequence), and "above" (meaning, in textual order, in this 
file). So yes, both "above" AND "earlier". Anyway, this one-liner is 
only here to justify the lack of any check-and-wait code. OTOH I could 
add some kerneldoc elsewhere to explain that the caller must call 
check_space() before calling submit().



  space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-if (WARN_ON(space < sizeof(struct guc_wq_item)))
-return -ENOSPC; /* shouldn't happen */
+GEM_BUG_ON(space < wqi_size);


It is impossible to hit this only because of the struct_mutex guarding
the whole time window from request creation to submission. If in the
future, near or far, that gets fixed, then this will need reworking.

I don't have any better ideas though.

But a WARN_ON and return would be almost as good. Everything is better
than a dead machine one can't ssh into...

So I appeal to make this a WARN_ON and return. Nothing bad would
happen apart from software thinking GPU has hung.

>>

If the driver violates the sequencing of check+submit then it does
indeed require reworking -- that would be a bug. Hence (GEM_)BUG_ON().


This also was clarified in this same thread few days ago and I conceded
the point.



-/* postincrement WQ tail for next time */
-wq_off = gc->wq_tail;
-gc->wq_tail += sizeof(struct guc_wq_item);
-gc->wq_tail &= gc->wq_size - 1;
+/* The GuC firmware wants the tail index in QWords, not bytes */
+tail = rq->tail;


Used to be sampled from rq->ringbuf->tail - are those the same?

>>

It should always have been rq->tail; they're the same at present because
it was copied from ringbuffer as part of the submission process, 

Re: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Tvrtko Ursulin]

On 05/05/16 19:38, Dave Gordon wrote:

On 29/04/2016 16:44, Tvrtko Ursulin wrote:


On 27/04/16 19:03, Dave Gordon wrote:

Mostly little optimisations; for instance, if the driver is correctly
following the submission protocol, the "out of space" condition is
impossible, so the previous runtime WARN_ON() is promoted to a
GEM_BUG_ON() for a more dramatic effect in development and less impact
in end-user systems.

Similarly we can replace other WARN_ON() conditions that don't relate to
the hardware state with either BUILD_BUG_ON() for compile-time-
detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

Signed-off-by: Dave Gordon 
Cc: Chris Wilson 

---
  drivers/gpu/drm/i915/i915_guc_submission.c | 69
+++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
  3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct
drm_i915_gem_request *request)
  return -EAGAIN;
  }

-static int guc_add_workqueue_item(struct i915_guc_client *gc,
-  struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+   struct drm_i915_gem_request *rq)
  {
+/* wqi_len is in DWords, and does not include the one-word
header */
+const size_t wqi_size = sizeof(struct guc_wq_item);


Again, u32 is correct I think.

Nope, it's a sizeof(), but the compiler will check that it fits in u32
when we convert it to DWords below.


I already wrote in this same thread few days ago this was my oversight.




+const u32 wqi_len = wqi_size/sizeof(u32) - 1;
  struct guc_process_desc *desc;
  struct guc_wq_item *wqi;
  void *base;
-u32 tail, wq_len, wq_off, space;
+u32 space, tail, wq_off, wq_page;

  desc = gc->client_base + gc->proc_desc_offset;
+
+/* Space was checked earlier, in i915_guc_wq_check_space() above */


It may be above in the file, but the two do not call one another so I
recommend saying exactly who called it.

I don't really mind who called it, as long as it was called sometime
earlier in the request submission protocol -- the callsite may get moved
around a bit. Use cscope(1) or your favourite IDE to find it.


So what does this comment supposed to tell the reader? 
guc_add_workqueue_item does not call i915_guc_wq_check_space so "above" 
what? Above in the file? Or maybe *earlier* in the call sequence of 
what? Might as well put an useful comment in when you are adding one.





  space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-if (WARN_ON(space < sizeof(struct guc_wq_item)))
-return -ENOSPC; /* shouldn't happen */
+GEM_BUG_ON(space < wqi_size);


It is impossible to hit this only because of the struct_mutex guarding
the whole time window from request creation to submission. If in the
future, near or far, that gets fixed, then this will need reworking.

I don't have any better ideas though.

But a WARN_ON and return would be almost as good. Everything is better
than a dead machine one can't ssh into...

So I appeal to make this a WARN_ON and return. Nothing bad would
happen apart from software thinking GPU has hung.

If the driver violates the sequencing of check+submit then it does
indeed require reworking -- that would be a bug. Hence (GEM_)BUG_ON().


This also was clarified in this same thread few days ago and I conceded 
the point.






-/* postincrement WQ tail for next time */
-wq_off = gc->wq_tail;
-gc->wq_tail += sizeof(struct guc_wq_item);
-gc->wq_tail &= gc->wq_size - 1;
+/* The GuC firmware wants the tail index in QWords, not bytes */
+tail = rq->tail;


Used to be sampled from rq->ringbuf->tail - are those the same?

It should always have been rq->tail; they're the same at present because
it was copied from ringbuffer as part of the submission process, but it
might not be the same with the scheduler & preemption. So let's get it
right before it becomes a mysterious bug.



+GEM_BUG_ON(tail & 7);
+tail >>= 3;
+GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);

  /* For now workqueue item is 4 DWs; workqueue buffer is 2
pages. So we
   * should not have the case where structure wqi is across page,
neither
@@ -495,19 +500,23 @@ static int guc_add_workqueue_item(struct
i915_guc_client *gc,
   * XXX: if not the case, we need save data to a temp wqi and
copy it to
   * workqueue buffer dw by dw.
   */
-WARN_ON(sizeof(struct guc_wq_item) != 16);
-WARN_ON(wq_off & 3);
+BUILD_BUG_ON(wqi_size 

Re: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Dave Gordon]

On 29/04/2016 16:44, Tvrtko Ursulin wrote:


On 27/04/16 19:03, Dave Gordon wrote:

Mostly little optimisations; for instance, if the driver is correctly
following the submission protocol, the "out of space" condition is
impossible, so the previous runtime WARN_ON() is promoted to a
GEM_BUG_ON() for a more dramatic effect in development and less impact
in end-user systems.

Similarly we can replace other WARN_ON() conditions that don't relate to
the hardware state with either BUILD_BUG_ON() for compile-time-
detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

Signed-off-by: Dave Gordon 
Cc: Chris Wilson 

---
  drivers/gpu/drm/i915/i915_guc_submission.c | 69 
+++---

  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
  3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c

index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct 
drm_i915_gem_request *request)

  return -EAGAIN;
  }

-static int guc_add_workqueue_item(struct i915_guc_client *gc,
-  struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+   struct drm_i915_gem_request *rq)
  {
+/* wqi_len is in DWords, and does not include the one-word 
header */

+const size_t wqi_size = sizeof(struct guc_wq_item);


Again, u32 is correct I think.
Nope, it's a sizeof(), but the compiler will check that it fits in u32 
when we convert it to DWords below.



+const u32 wqi_len = wqi_size/sizeof(u32) - 1;
  struct guc_process_desc *desc;
  struct guc_wq_item *wqi;
  void *base;
-u32 tail, wq_len, wq_off, space;
+u32 space, tail, wq_off, wq_page;

  desc = gc->client_base + gc->proc_desc_offset;
+
+/* Space was checked earlier, in i915_guc_wq_check_space() above */


It may be above in the file, but the two do not call one another so I 
recommend saying exactly who called it.
I don't really mind who called it, as long as it was called sometime 
earlier in the request submission protocol -- the callsite may get moved 
around a bit. Use cscope(1) or your favourite IDE to find it.



  space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-if (WARN_ON(space < sizeof(struct guc_wq_item)))
-return -ENOSPC; /* shouldn't happen */
+GEM_BUG_ON(space < wqi_size);


It is impossible to hit this only because of the struct_mutex guarding 
the whole time window from request creation to submission. If in the 
future, near or far, that gets fixed, then this will need reworking.


I don't have any better ideas though.

But a WARN_ON and return would be almost as good. Everything is better 
than a dead machine one can't ssh into...


So I appeal to make this a WARN_ON and return. Nothing bad would 
happen apart from software thinking GPU has hung.
If the driver violates the sequencing of check+submit then it does 
indeed require reworking -- that would be a bug. Hence (GEM_)BUG_ON().




-/* postincrement WQ tail for next time */
-wq_off = gc->wq_tail;
-gc->wq_tail += sizeof(struct guc_wq_item);
-gc->wq_tail &= gc->wq_size - 1;
+/* The GuC firmware wants the tail index in QWords, not bytes */
+tail = rq->tail;


Used to be sampled from rq->ringbuf->tail - are those the same?
It should always have been rq->tail; they're the same at present because 
it was copied from ringbuffer as part of the submission process, but it 
might not be the same with the scheduler & preemption. So let's get it 
right before it becomes a mysterious bug.



+GEM_BUG_ON(tail & 7);
+tail >>= 3;
+GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);

  /* For now workqueue item is 4 DWs; workqueue buffer is 2 
pages. So we
   * should not have the case where structure wqi is across page, 
neither
@@ -495,19 +500,23 @@ static int guc_add_workqueue_item(struct 
i915_guc_client *gc,
   * XXX: if not the case, we need save data to a temp wqi and 
copy it to

   * workqueue buffer dw by dw.
   */
-WARN_ON(sizeof(struct guc_wq_item) != 16);
-WARN_ON(wq_off & 3);
+BUILD_BUG_ON(wqi_size != 16);

-/* wq starts from the page after doorbell / process_desc */
-base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
+/* postincrement WQ tail for next time */
+wq_off = gc->wq_tail;
+gc->wq_tail += wqi_size;
+gc->wq_tail &= gc->wq_size - 1;
+GEM_BUG_ON(wq_off & (wqi_size - 1));


Use to be wq_off & 3, now is wq_off & 15, which one is correct?
The new one 

Re: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Tvrtko Ursulin]

On 29/04/16 17:10, Chris Wilson wrote:

On Fri, Apr 29, 2016 at 04:44:24PM +0100, Tvrtko Ursulin wrote:


On 27/04/16 19:03, Dave Gordon wrote:

Mostly little optimisations; for instance, if the driver is correctly
following the submission protocol, the "out of space" condition is
impossible, so the previous runtime WARN_ON() is promoted to a
GEM_BUG_ON() for a more dramatic effect in development and less impact
in end-user systems.

Similarly we can replace other WARN_ON() conditions that don't relate to
the hardware state with either BUILD_BUG_ON() for compile-time-
detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

Signed-off-by: Dave Gordon 
Cc: Chris Wilson 

---
  drivers/gpu/drm/i915/i915_guc_submission.c | 69 +++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
  3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request 
*request)
return -EAGAIN;
  }

-static int guc_add_workqueue_item(struct i915_guc_client *gc,
- struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+  struct drm_i915_gem_request *rq)
  {
+   /* wqi_len is in DWords, and does not include the one-word header */
+   const size_t wqi_size = sizeof(struct guc_wq_item);


Again, u32 is correct I think.


+   const u32 wqi_len = wqi_size/sizeof(u32) - 1;
struct guc_process_desc *desc;
struct guc_wq_item *wqi;
void *base;
-   u32 tail, wq_len, wq_off, space;
+   u32 space, tail, wq_off, wq_page;

desc = gc->client_base + gc->proc_desc_offset;
+
+   /* Space was checked earlier, in i915_guc_wq_check_space() above */


It may be above in the file, but the two do not call one another so
I recommend saying exactly who called it.


space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-   if (WARN_ON(space < sizeof(struct guc_wq_item)))
-   return -ENOSPC; /* shouldn't happen */
+   GEM_BUG_ON(space < wqi_size);


It is impossible to hit this only because of the struct_mutex
guarding the whole time window from request creation to submission.
If in the future, near or far, that gets fixed, then this will need
reworking.


Request submission will still have to serialised by a "ring" mutex,
from the time we allocate the request to the time we add it to whatever
submission queue. It should still hold that we can pin all the required
resources (ringbuffer, context state, vm page tables, workqueues) up
front and take any errors early and then rely on our preallocation when
submitting the request.


I don't have any better ideas though.

But a WARN_ON and return would be almost as good. Everything is
better than a dead machine one can't ssh into...

So I appeal to make this a WARN_ON and return. Nothing bad would
happen apart from software thinking GPU has hung.


Hence why not make it a bug? If you can't ssh in because the driver died
inside GEM, something is very wrong.


I was sure bugs are kernel panics, looks like I've been running with 
panic on oops for too long. :(


GEM_BUG_ON is ok then.

Regards,

Tvrtko


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

Re: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Chris Wilson]

On Fri, Apr 29, 2016 at 04:44:24PM +0100, Tvrtko Ursulin wrote:
> 
> On 27/04/16 19:03, Dave Gordon wrote:
> >Mostly little optimisations; for instance, if the driver is correctly
> >following the submission protocol, the "out of space" condition is
> >impossible, so the previous runtime WARN_ON() is promoted to a
> >GEM_BUG_ON() for a more dramatic effect in development and less impact
> >in end-user systems.
> >
> >Similarly we can replace other WARN_ON() conditions that don't relate to
> >the hardware state with either BUILD_BUG_ON() for compile-time-
> >detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.
> >
> >With those changes, we can convert it to void, as suggested by Chris
> >Wilson, and update the calling code appropriately.
> >
> >Signed-off-by: Dave Gordon 
> >Cc: Chris Wilson 
> >
> >---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 69 
> > +++---
> >  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
> >  drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
> >  3 files changed, 37 insertions(+), 37 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> >b/drivers/gpu/drm/i915/i915_guc_submission.c
> >index 6626eff..4d2ea84 100644
> >--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct 
> >drm_i915_gem_request *request)
> > return -EAGAIN;
> >  }
> >
> >-static int guc_add_workqueue_item(struct i915_guc_client *gc,
> >-  struct drm_i915_gem_request *rq)
> >+static void guc_add_workqueue_item(struct i915_guc_client *gc,
> >+   struct drm_i915_gem_request *rq)
> >  {
> >+/* wqi_len is in DWords, and does not include the one-word header */
> >+const size_t wqi_size = sizeof(struct guc_wq_item);
> 
> Again, u32 is correct I think.
> 
> >+const u32 wqi_len = wqi_size/sizeof(u32) - 1;
> > struct guc_process_desc *desc;
> > struct guc_wq_item *wqi;
> > void *base;
> >-u32 tail, wq_len, wq_off, space;
> >+u32 space, tail, wq_off, wq_page;
> >
> > desc = gc->client_base + gc->proc_desc_offset;
> >+
> >+/* Space was checked earlier, in i915_guc_wq_check_space() above */
> 
> It may be above in the file, but the two do not call one another so
> I recommend saying exactly who called it.
> 
> > space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
> >-if (WARN_ON(space < sizeof(struct guc_wq_item)))
> >-return -ENOSPC; /* shouldn't happen */
> >+GEM_BUG_ON(space < wqi_size);
> 
> It is impossible to hit this only because of the struct_mutex
> guarding the whole time window from request creation to submission.
> If in the future, near or far, that gets fixed, then this will need
> reworking.

Request submission will still have to serialised by a "ring" mutex, 
from the time we allocate the request to the time we add it to whatever
submission queue. It should still hold that we can pin all the required
resources (ringbuffer, context state, vm page tables, workqueues) up
front and take any errors early and then rely on our preallocation when
submitting the request.

> I don't have any better ideas though.
> 
> But a WARN_ON and return would be almost as good. Everything is
> better than a dead machine one can't ssh into...
> 
> So I appeal to make this a WARN_ON and return. Nothing bad would
> happen apart from software thinking GPU has hung.

Hence why not make it a bug? If you can't ssh in because the driver died
inside GEM, something is very wrong.
-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: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Tvrtko Ursulin]

On 27/04/16 19:03, Dave Gordon wrote:

Mostly little optimisations; for instance, if the driver is correctly
following the submission protocol, the "out of space" condition is
impossible, so the previous runtime WARN_ON() is promoted to a
GEM_BUG_ON() for a more dramatic effect in development and less impact
in end-user systems.

Similarly we can replace other WARN_ON() conditions that don't relate to
the hardware state with either BUILD_BUG_ON() for compile-time-
detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

Signed-off-by: Dave Gordon 
Cc: Chris Wilson 

---
  drivers/gpu/drm/i915/i915_guc_submission.c | 69 +++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
  3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request 
*request)
return -EAGAIN;
  }

-static int guc_add_workqueue_item(struct i915_guc_client *gc,
- struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+  struct drm_i915_gem_request *rq)
  {
+   /* wqi_len is in DWords, and does not include the one-word header */
+   const size_t wqi_size = sizeof(struct guc_wq_item);


Again, u32 is correct I think.


+   const u32 wqi_len = wqi_size/sizeof(u32) - 1;
struct guc_process_desc *desc;
struct guc_wq_item *wqi;
void *base;
-   u32 tail, wq_len, wq_off, space;
+   u32 space, tail, wq_off, wq_page;

desc = gc->client_base + gc->proc_desc_offset;
+
+   /* Space was checked earlier, in i915_guc_wq_check_space() above */


It may be above in the file, but the two do not call one another so I 
recommend saying exactly who called it.



space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-   if (WARN_ON(space < sizeof(struct guc_wq_item)))
-   return -ENOSPC; /* shouldn't happen */
+   GEM_BUG_ON(space < wqi_size);


It is impossible to hit this only because of the struct_mutex guarding 
the whole time window from request creation to submission. If in the 
future, near or far, that gets fixed, then this will need reworking.


I don't have any better ideas though.

But a WARN_ON and return would be almost as good. Everything is better 
than a dead machine one can't ssh into...


So I appeal to make this a WARN_ON and return. Nothing bad would happen 
apart from software thinking GPU has hung.




-   /* postincrement WQ tail for next time */
-   wq_off = gc->wq_tail;
-   gc->wq_tail += sizeof(struct guc_wq_item);
-   gc->wq_tail &= gc->wq_size - 1;
+   /* The GuC firmware wants the tail index in QWords, not bytes */
+   tail = rq->tail;


Used to be sampled from rq->ringbuf->tail - are those the same?


+   GEM_BUG_ON(tail & 7);
+   tail >>= 3;
+   GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);

/* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
 * should not have the case where structure wqi is across page, neither
@@ -495,19 +500,23 @@ static int guc_add_workqueue_item(struct i915_guc_client 
*gc,
 * XXX: if not the case, we need save data to a temp wqi and copy it to
 * workqueue buffer dw by dw.
 */
-   WARN_ON(sizeof(struct guc_wq_item) != 16);
-   WARN_ON(wq_off & 3);
+   BUILD_BUG_ON(wqi_size != 16);

-   /* wq starts from the page after doorbell / process_desc */
-   base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-   (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
+   /* postincrement WQ tail for next time */
+   wq_off = gc->wq_tail;
+   gc->wq_tail += wqi_size;
+   gc->wq_tail &= gc->wq_size - 1;
+   GEM_BUG_ON(wq_off & (wqi_size - 1));


Use to be wq_off & 3, now is wq_off & 15, which one is correct?


+
+   /* WQ starts from the page after doorbell / process_desc */
+   wq_page = (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT;
wq_off &= PAGE_SIZE - 1;
+   base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, wq_page));
wqi = (struct guc_wq_item *)((char *)base + wq_off);

-   /* len does not include the header */
-   wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
+   /* Now fill in the 4-word work queue item */
wqi->header = WQ_TYPE_INORDER |
-   (wq_len << WQ_LEN_SHIFT) |
+   (wqi_len << WQ_LEN_SHIFT) |

[Intel-gfx] [PATCH v2 4/5] drm/i915/guc: rework guc_add_workqueue_item()

[Dave Gordon]

Mostly little optimisations; for instance, if the driver is correctly
following the submission protocol, the "out of space" condition is
impossible, so the previous runtime WARN_ON() is promoted to a
GEM_BUG_ON() for a more dramatic effect in development and less impact
in end-user systems.

Similarly we can replace other WARN_ON() conditions that don't relate to
the hardware state with either BUILD_BUG_ON() for compile-time-
detectable issues, or GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

Signed-off-by: Dave Gordon 
Cc: Chris Wilson 

---
 drivers/gpu/drm/i915/i915_guc_submission.c | 69 +++---
 drivers/gpu/drm/i915/intel_guc.h   |  2 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h  |  3 +-
 3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request 
*request)
return -EAGAIN;
 }
 
-static int guc_add_workqueue_item(struct i915_guc_client *gc,
- struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+  struct drm_i915_gem_request *rq)
 {
+   /* wqi_len is in DWords, and does not include the one-word header */
+   const size_t wqi_size = sizeof(struct guc_wq_item);
+   const u32 wqi_len = wqi_size/sizeof(u32) - 1;
struct guc_process_desc *desc;
struct guc_wq_item *wqi;
void *base;
-   u32 tail, wq_len, wq_off, space;
+   u32 space, tail, wq_off, wq_page;
 
desc = gc->client_base + gc->proc_desc_offset;
+
+   /* Space was checked earlier, in i915_guc_wq_check_space() above */
space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-   if (WARN_ON(space < sizeof(struct guc_wq_item)))
-   return -ENOSPC; /* shouldn't happen */
+   GEM_BUG_ON(space < wqi_size);
 
-   /* postincrement WQ tail for next time */
-   wq_off = gc->wq_tail;
-   gc->wq_tail += sizeof(struct guc_wq_item);
-   gc->wq_tail &= gc->wq_size - 1;
+   /* The GuC firmware wants the tail index in QWords, not bytes */
+   tail = rq->tail;
+   GEM_BUG_ON(tail & 7);
+   tail >>= 3;
+   GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
 
/* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
 * should not have the case where structure wqi is across page, neither
@@ -495,19 +500,23 @@ static int guc_add_workqueue_item(struct i915_guc_client 
*gc,
 * XXX: if not the case, we need save data to a temp wqi and copy it to
 * workqueue buffer dw by dw.
 */
-   WARN_ON(sizeof(struct guc_wq_item) != 16);
-   WARN_ON(wq_off & 3);
+   BUILD_BUG_ON(wqi_size != 16);
 
-   /* wq starts from the page after doorbell / process_desc */
-   base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-   (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
+   /* postincrement WQ tail for next time */
+   wq_off = gc->wq_tail;
+   gc->wq_tail += wqi_size;
+   gc->wq_tail &= gc->wq_size - 1;
+   GEM_BUG_ON(wq_off & (wqi_size - 1));
+
+   /* WQ starts from the page after doorbell / process_desc */
+   wq_page = (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT;
wq_off &= PAGE_SIZE - 1;
+   base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, wq_page));
wqi = (struct guc_wq_item *)((char *)base + wq_off);
 
-   /* len does not include the header */
-   wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
+   /* Now fill in the 4-word work queue item */
wqi->header = WQ_TYPE_INORDER |
-   (wq_len << WQ_LEN_SHIFT) |
+   (wqi_len << WQ_LEN_SHIFT) |
(rq->engine->guc_id << WQ_TARGET_SHIFT) |
WQ_NO_WCFLUSH_WAIT;
 
@@ -515,14 +524,10 @@ static int guc_add_workqueue_item(struct i915_guc_client 
*gc,
wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx,
 rq->engine);
 
-   /* The GuC firmware wants the tail index in QWords, not bytes */
-   tail = rq->ringbuf->tail >> 3;
wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
-   wqi->fence_id = 0; /*XXX: what fence to be here */
+   wqi->fence_id = rq->seqno;
 
kunmap_atomic(base);
-
-   return 0;
 }
 
 /**
@@ -537,26 +542,20 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
unsigned int engine_id = rq->engine->guc_id;
struct intel_guc *guc = >i915->guc;