Re: [PATCH v3 3/3] drm/i915/guc: local optimisations and updating comments

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

On 19/04/16 17:38, Dave Gordon wrote:
> On 19/04/16 16:08, Tvrtko Ursulin wrote:
>>
>> On 19/04/16 12:45, Dave Gordon wrote:
>>> Tidying up guc_init_proc_desc() and adding commentary to the client
>>> structure after the recent change in GuC page mapping strategy.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 38
>>> ++++++++++++++----------------
>>>   drivers/gpu/drm/i915/intel_guc.h           | 23 ++++++++++++++++++
>>>   2 files changed, 41 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index 135f094..5bbb13b 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -358,12 +358,14 @@ static void guc_init_proc_desc(struct intel_guc
>>> *guc,
>>>   static void guc_init_ctx_desc(struct intel_guc *guc,
>>>                     struct i915_guc_client *client)
>>>   {
>>> +    struct drm_i915_gem_object *client_obj = client->client_obj;
>>>       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>>       struct intel_engine_cs *engine;
>>>       struct intel_context *ctx = client->owner;
>>>       struct guc_context_desc desc;
>>>       struct sg_table *sg;
>>>       enum intel_engine_id id;
>>> +    u32 gfx_addr;
>>>
>>>       memset(&desc, 0, sizeof(desc));
>>>
>>> @@ -392,16 +394,17 @@ static void guc_init_ctx_desc(struct intel_guc
>>> *guc,
>>>           lrc->context_desc = (u32)ctx_desc;
>>>
>>>           /* The state page is after PPHWSP */
>>> -        lrc->ring_lcra = i915_gem_obj_ggtt_offset(obj) +
>>> -                LRC_STATE_PN * PAGE_SIZE;
>>> +        gfx_addr = i915_gem_obj_ggtt_offset(obj);
>>> +        lrc->ring_lcra = gfx_addr + LRC_STATE_PN * PAGE_SIZE;
>>>           lrc->context_id = (client->ctx_index <<
>>> GUC_ELC_CTXID_OFFSET) |
>>>                   (engine->guc_id << GUC_ELC_ENGINE_OFFSET);
>>>
>>>           obj = ctx->engine[id].ringbuf->obj;
>>> +        gfx_addr = i915_gem_obj_ggtt_offset(obj);
>>>
>>> -        lrc->ring_begin = i915_gem_obj_ggtt_offset(obj);
>>> -        lrc->ring_end = lrc->ring_begin + obj->base.size - 1;
>>> -        lrc->ring_next_free_location = lrc->ring_begin;
>>> +        lrc->ring_begin = gfx_addr;
>>> +        lrc->ring_end = gfx_addr + obj->base.size - 1;
>>> +        lrc->ring_next_free_location = gfx_addr;
>>>           lrc->ring_current_tail_pointer_value = 0;
>>>
>>>           desc.engines_used |= (1 << engine->guc_id);
>>> @@ -410,22 +413,17 @@ static void guc_init_ctx_desc(struct intel_guc
>>> *guc,
>>>       WARN_ON(desc.engines_used == 0);
>>>
>>>       /*
>>> -     * The CPU address is only needed at certain points, so
>>> kmap_atomic on
>>> -     * demand instead of storing it in the ctx descriptor.
>>> -     * XXX: May make debug easier to have it mapped
>>> +     * The doorbell, process descriptor, and workqueue are all parts
>>> +     * of the client object, which the GuC will reference via the GGTT
>>>        */
>>> -    desc.db_trigger_cpu = 0;
>>> -    desc.db_trigger_uk = client->doorbell_offset +
>>> -        i915_gem_obj_ggtt_offset(client->client_obj);
>>> -    desc.db_trigger_phy = client->doorbell_offset +
>>> -        sg_dma_address(client->client_obj->pages->sgl);
>>> -
>>> -    desc.process_desc = client->proc_desc_offset +
>>> -        i915_gem_obj_ggtt_offset(client->client_obj);
>>> -
>>> -    desc.wq_addr = client->wq_offset +
>>> -        i915_gem_obj_ggtt_offset(client->client_obj);
>>> -
>>> +    gfx_addr = i915_gem_obj_ggtt_offset(client_obj);
>>> +    desc.db_trigger_phy = sg_dma_address(client_obj->pages->sgl) +
>>> +                client->doorbell_offset;
>>> +    desc.db_trigger_cpu = (uintptr_t)client->client_base +
>>> +                client->doorbell_offset;
>>
>> This changed from zero to the address. Was it buggy before?
>
> It's not actually used, AFAICT, but we might as well set it to the
> correct value anyway - this data is shared with the GuC, so I'd like the
> values to be as complete and correct as possible, in case the GuC
> firmware ever starts making use of them. And maybe we could use this for
> consistency checking if we ever found mysterious page faults in the
> doorbell code. But really it's just to show that these three fields all
> describe the same thing, namely where to ring the doorbell, in physical
> (DMA), kernel, and GGTT address spaces.

Got it, in that case:

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