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

[Dave Gordon <david.s.gordon@intel.com>]

On 18/04/16 11:25, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 11:15:07AM +0100, 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.
>>
>> v2:
>>      Invent & use accessor function i915_object_mapped_address() rather
>>      than accessing obj->mapping directly (Tvrtko Ursulin)
>>      Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
>>      Added big comment about struct i915_guc_client & usage of the GEM
>>      object that it describes
>>
>> 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_drv.h            | 12 ++++++++++
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
>>   drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
>>   3 files changed, 50 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 85102ad..439f149 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>>   void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
>>
>>   /**
>> + * i915_object_mapped_address - address at which a GEM object is mapped
>> + * @obj - the object (presumably already mapped into kernel address space)
>> + *
>> + * Returns the address at which an object has been mapped by a call to
>> + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
>> + */
>> +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
>> +{
>> +       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
>> +}
>
> No.

The kernel address at which an object is pinned is just as much a 
property of the object as its GGTT address, so should have a similar 
function for retrieving it.

The pages_pin_count check is there because once that goes to zero, the 
object doesn't logically /have/ a memory address, even if 'mapping' is 
still set.

Or was it just the name you objected to?

>> @@ -649,6 +639,9 @@ 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);
>>
>> +	if (i915_object_mapped_address(obj))
>> +		i915_gem_object_unpin_map(obj);
>
> Eh? If you aren't tracking your pin, you can't just randomly free
> someone elses.

The GuC code exclusively owns this object, so it can't be "someone 
else's". The pages_pin count here will always be 1 (at present). This 
was to allow for the possibility that we already chose to release the 
pin-and-map while leaving the object in the GGTT, in which case the 
pages_pin_count will be 0. (We don't do this, at present, but see below).

>>   	drm_gem_object_unreference(&obj->base);
>>   }
>>
>> @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>>   		goto err;
>>
>>   	client->client_obj = obj;
>> +	client->client_base = i915_object_mapped_address(obj);
>> +	WARN_ON(!client->client_base);
>>   	client->wq_offset = GUC_DB_SIZE;
>>   	client->wq_size = GUC_WQ_SIZE;
>>
>> @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
>>   	struct guc_policies *policies;
>>   	struct guc_mmio_reg_state *reg_state;
>>   	struct intel_engine_cs *engine;
>> -	struct page *page;
>>   	u32 size;
>>
>>   	/* The ads obj includes the struct itself and buffers passed to GuC */
>> @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
>>
>>   		guc->ads_obj = obj;
>>   	}
>> -
>> -	page = i915_gem_object_get_page(obj, 0);
>> -	ads = kmap(page);
>> +	ads = i915_object_mapped_address(obj);
>
> You need to explicitly aknowlege that you are using the memory tracked
> by the object otherwise it *will* be removed by the shrinker.
> So what is wrong with using i915_gem_object_pin_map()?
> -Chris

The object is pinned-and-mapped just once, when it's created, and kept 
that way until it's destroyed. Calling pin-and-map here would give it a 
pin count of two, which would be redundant, and require us to 
unpin-and-map it again at the end of the function - which just seems a 
waste of time.

I also considered dropping the original pin-and-map on the ADS object 
once this function has finished with it, but decided it wasn't worth the 
extra work. It has to stay in memory (and in the GGTT) anyway, so it 
would only save a few pages of vmap space. If we did that, the check on 
whether the object was still mapped (in gem_release_guc_obj() above) 
would absolutely be necessary.

.Dave.

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