Re: [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel

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

On 15/04/16 12:12, Dave Gordon wrote:
> On 15/04/2016 11:04, Tvrtko Ursulin wrote:
>>
>> On 14/04/16 18:19, Dave Gordon wrote:
>>> With the new i915_gem_obj_pin_map() interface, it makes sense to keep
>>> GuC objects (which are always pinned in memory and in the GGTT anyway)
>>> mapped into kernel address space, rather than mapping and unmapping them
>>> on each access.
>>>
>>> This preliminary patch sets up the pin-and-map for all GuC-specific
>>> objects, and updates the various setup/shutdown functions to use these
>>> long-term mappings rather than doing their own kmap_atomic() calls.
>>>
>>> Cc: <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 37
>>> +++++++++++-------------------
>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>   2 files changed, 14 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index da86bdb..f80f577 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc
>>> *guc,
>>>                     struct i915_guc_client *client)
>>>   {
>>>       struct guc_doorbell_info *doorbell;
>>> -    void *base;
>>>
>>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>>> 0));
>>> -    doorbell = base + client->doorbell_offset;
>>> +    doorbell = client->client_base + client->doorbell_offset;
>>>
>>> -    doorbell->db_status = 1;
>>> +    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>>       doorbell->cookie = 0;
>>> -
>>> -    kunmap_atomic(base);
>>>   }
>>>
>>>   static int guc_ring_doorbell(struct i915_guc_client *gc)
>>> @@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct
>>> intel_guc *guc,
>>>   {
>>>       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>>       struct guc_doorbell_info *doorbell;
>>> -    void *base;
>>>       i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>>>       int value;
>>>
>>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>>> 0));
>>> -    doorbell = base + client->doorbell_offset;
>>> -
>>> -    doorbell->db_status = 0;
>>> +    doorbell = client->client_base + client->doorbell_offset;
>>
>> Not 100% sure of the object lifetimes in GuC, but would it be even
>> simpler to store a pointer to struct struct guc_doorbell_info as
>> guc->doorbell ? There aren't that many call sites true, but kind of
>> looks logical at least from the outside.
>
> Well probably, but that would be a separate patch. This is just dealing
> with eliminating the repeated kmap/unmap calls.

Okay, just thought it may be easier to do it at once since it looked 
really straightforward, like same amount of work and cleaner end result.

> Also, maybe this helps remind people that these are actually parts of
> the same object. There's just one allocated, but it encompasses the
> process descriptor and the doorbell in the first page, and the workqueue
> in the second and third.

I don't think anyone from the outside would care since it is all 
internal guc code so it is free to define its rules with a nice comment 
in the relevant structure definition.

>>> -    kunmap_atomic(base);
>>> +    doorbell->db_status = GUC_DOORBELL_DISABLED;
>>>
>>>       I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>>>
>>> @@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc
>>> *guc,
>>>                      struct i915_guc_client *client)
>>>   {
>>>       struct guc_process_desc *desc;
>>> -    void *base;
>>>
>>> -    base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
>>> 0));
>>> -    desc = base + client->proc_desc_offset;
>>> +    desc = client->client_base + client->proc_desc_offset;
>>
>> And the same maybe for this?
>>
>>>       memset(desc, 0, sizeof(*desc));
>>>
>>> @@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc
>>> *guc,
>>>       desc->wq_size_bytes = client->wq_size;
>>>       desc->wq_status = WQ_STATUS_ACTIVE;
>>>       desc->priority = client->priority;
>>> -
>>> -    kunmap_atomic(base);
>>>   }
>>>
>>>   /*
>>> @@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
>>>    * This is a wrapper to create a gem obj. In order to use it inside
>>> GuC, the
>>>    * object needs to be pinned lifetime. Also we must pin it to gtt
>>> space other
>>>    * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
>>> + * The object is also pinned & mapped into kernel address space.
>>>    *
>>>    * Return:    A drm_i915_gem_object if successful, otherwise NULL.
>>>    */
>>> @@ -620,13 +609,14 @@ static struct drm_i915_gem_object
>>> *gem_allocate_guc_obj(struct drm_device *dev,
>>>       if (!obj)
>>>           return NULL;
>>>
>>> -    if (i915_gem_object_get_pages(obj)) {
>>> +    if (i915_gem_object_pin_map(obj) == NULL) {
>>
>> This should be IS_ERR check.
>
> OK, will update.
>
>>> drm_gem_object_unreference(&obj->base);
>>>           return NULL;
>>>       }
>>>
>>>       if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
>>>               PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
>>> +        i915_gem_object_unpin_map(obj);
>>>           drm_gem_object_unreference(&obj->base);
>>>           return NULL;
>>>       }
>>> @@ -649,6 +639,8 @@ static void gem_release_guc_obj(struct
>>> drm_i915_gem_object *obj)
>>>       if (i915_gem_obj_is_pinned(obj))
>>>           i915_gem_object_ggtt_unpin(obj);
>>>
>>> +    i915_gem_object_unpin_map(obj);
>>> +
>>>       drm_gem_object_unreference(&obj->base);
>>>   }
>>>
>>> @@ -729,6 +721,8 @@ static struct i915_guc_client
>>> *guc_client_alloc(struct drm_device *dev,
>>>           goto err;
>>>
>>>       client->client_obj = obj;
>>> +    client->client_base = obj->mapping;
>>
>> It think outside code should not access obj->mapping directly but use
>> what i915_gem_object_pin_map has returned.
>
> No, that would be quite inconvenient. You shouldn't need to hold
> auxiliary information about an allocated object when you can get that
> information directly from the object itself.

You can not unless you break the API layer. obj->mapping is IMHO private 
to GEM and GuC should not touch it. Even must not IMHO.

> Also, the function that does the pin-and-map doesn't have access to the
> structure where the address is going to be cached, it just returns the
> allocated-pinned-and-mapped object.

That sounds like a local issue which can be worked around by storing it 
in the appropriate GuC data structure, no?

> OTOH I have no objection to wrapping it an accessor function/macro.
>
> void *i914_gem_object_mapped_addr(object) ?
>
> returning NULL if object is not mapped?

I don't feel strongly either way. Question for Chris I suppose.

In this particular case I would look to avoid the need for it by storing 
the address in my own data structure(s), if they have constructors and 
destructors which start and end with i915_gem_object_pin_map/unmap 
respectively.

>>> +    WARN_ON(!client->client_base);
>>
>> And this has already been handled at the i915_gem_object_pin_map call
>> site so I don't think it serves any purpose.
>
> In case the obj->mapping *wasn't* the same value that was returned from
> pin-and-map and checked.

Ah guard against touching forbidden parts. :)

Regards,

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