Re: [PATCH] drm/i915/gem/mman: only allow WC for lmem

[Matthew Auld <matthew.auld@intel.com>]

On 02/06/2021 13:00, Thomas Hellström wrote:
> Hi,
> 
> On 6/2/21 11:36 AM, Matthew Auld wrote:
>> For dgfx where we now have lmem and ttm, we can only support single mmap
>> mode for the lifetime of the object, and for lmem objects this should be
>> WC, so reject all other mapping modes for mmap_offset, including if the
>> object can be placed in both smem and lmem.
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++++
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gem/i915_gem_object.h |  4 ++++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index fd1c9714f8d8..32f88f236771 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -689,6 +689,10 @@ __assign_mmap_offset(struct drm_file *file,
>>           goto out;
>>       }
>> +    if (mmap_type != I915_MMAP_TYPE_WC &&
>> +        i915_gem_object_placements_contain_type(obj, 
>> INTEL_MEMORY_LOCAL))
>> +        return -ENODEV;
>> +
> 
> I think we will also have the restriction that any other objects on DGFX 
> can only be mapped cached? At least that's what the TTM code is 
> implementing currently.

Yeah, with this patch the caching mode should now at least be consistent 
for lmem objects, for smem we still need more patches.

> 
> 
>>       mmo = mmap_offset_attach(obj, mmap_type, file);
>>       if (IS_ERR(mmo)) {
>>           err = PTR_ERR(mmo);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 2be6109d0093..d4b0da8ed969 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -403,6 +403,28 @@ int i915_gem_object_read_from_page(struct 
>> drm_i915_gem_object *obj, u64 offset,
>>       return 0;
>>   }
>> +/**
>> + * i915_gem_object_placements_contain_type - Check whether the object 
>> can be
>> + * placed at certain memory type
>> + * @obj: Pointer to the object
>> + * @type: The memory type to check
>> + *
>> + * Return: True if the object can be placed in @type. False otherwise.
>> + */
>> +bool i915_gem_object_placements_contain_type(struct 
>> drm_i915_gem_object *obj,
>> +                         enum intel_memory_type type)
>> +{
>> +    unsigned int i;
>> +
>> +    /* TODO: consider maybe storing as a mask when doing 
>> gem_create_ext */
>> +    for (i = 0; i < obj->mm.n_placements; i++) {
>> +        if (obj->mm.placements[i]->type == type)
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
> 
> Do we need something for the in-kernel mappings as well? Or just return 
> a mapping with the only allowed caching mode?

For lmem everything should already be WC for in-kernel mappings. For 
everything else which uses pin_map() we will need to default to cached. 
I guess just add a different helper for this? We should probably also 
adjust the obj->cache_level for dg1.

> 
> /Thomas
> 
>