Re: [PATCH v2 3/5] drm/i915/guc: don't spinwait if the GuC's workqueue is full

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/05/16 16:17, Dave Gordon wrote:
> On 29/04/16 16:45, Tvrtko Ursulin wrote:
>>
>> One late comment:
>>
>> On 27/04/16 19:03, Dave Gordon wrote:
>>> Rather than wait to see whether more space becomes available in the GuC
>>> submission workqueue, we can just return -EAGAIN and let the caller try
>>> again in a little while. This gets rid of an uninterruptable sleep in
>>> the polling code :)
>>>
>>> We'll also add a counter to the GuC client statistics, to see how often
>>> we find the WQ full.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c        |  1 +
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 16 +++++-----------
>>>   drivers/gpu/drm/i915/intel_guc.h           |  8 ++++----
>>>   3 files changed, 10 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 8b8d6f0..1024947 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2509,6 +2509,7 @@ static void i915_guc_client_info(struct seq_file
>>> *m,
>>>       seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
>>>           client->wq_size, client->wq_offset, client->wq_tail);
>>>
>>> +    seq_printf(m, "\tWork queue full: %u\n", client->no_wq_space);
>>>       seq_printf(m, "\tFailed to queue: %u\n", client->q_fail);
>>>       seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
>>>       seq_printf(m, "\tLast submission result: %d\n", client->retcode);
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index 66af5ce..6626eff 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -453,27 +453,21 @@ static void guc_fini_ctx_desc(struct intel_guc
>>> *guc,
>>>
>>>   int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>>>   {
>>> -    const size_t size = sizeof(struct guc_wq_item);
>>> +    const size_t wqi_size = sizeof(struct guc_wq_item);
>>>       struct i915_guc_client *gc = request->i915->guc.execbuf_client;
>>>       struct guc_process_desc *desc;
>>> -    int ret = -ETIMEDOUT, timeout_counter = 200;
>>>
>>>       if (!gc)
>>>           return 0;
>>>
>>>       desc = gc->client_base + gc->proc_desc_offset;
>>>
>>> -    while (timeout_counter-- > 0) {
>>> -        if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
>>> -            ret = 0;
>>> -            break;
>>> -        }
>>> +    if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= wqi_size)
>>> +        return 0;
>>>
>>> -        if (timeout_counter)
>>> -            usleep_range(1000, 2000);
>>> -    };
>>> +    gc->no_wq_space += 1;
>>>
>>> -    return ret;
>>> +    return -EAGAIN;
>>>   }
>>>
>>>   static int guc_add_workqueue_item(struct i915_guc_client *gc,
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>>> b/drivers/gpu/drm/i915/intel_guc.h
>>> index b37c731..436f2d6 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>> @@ -73,10 +73,10 @@ struct i915_guc_client {
>>>
>>>       /* GuC submission statistics & status */
>>>       uint64_t submissions[GUC_MAX_ENGINES_NUM];
>>> -    uint32_t q_fail;
>>> -    uint32_t b_fail;
>>> -    int retcode;
>>> -    int spare;            /* pad to 32 DWords        */
>>> +    uint32_t no_wq_space;        /* Space pre-check failed    */
>>> +    uint32_t q_fail;        /* Failed to queue (MBZ)    */
>>> +    uint32_t b_fail;        /* Doorbell failure (MBZ)    */
>>
>> Why MBZ? It is not all used in this context so this will just confuse
>> people.
>
> MBZ => Must Be Zero. As in, we can't really deal with the events that
> cause these counters to be incremented, so if they're nonzero, something
> is broken and the driver may or may not recover :(
>
> If the call protocol is changed, the MBZ variables may go away entirely.

My objection is that when someone sees MBZ they'll wrongly think this 
structure is shared with the hardware. Since it is just a software 
counter MBZ is a confusing marker to use.

Instead these fields should probably just be unsigned ints with comments 
saying something to the effect of what you wrote above.

Regards,

Tvrtko



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