Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/gem: only allow WB for smem only placements

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

On 28/06/2021 08:41, Thomas Hellström wrote:
> 
> On 6/25/21 2:27 PM, Matthew Auld wrote:
>> We only support single mode and this should be immutable. For smem only
>> placements on DGFX this should be WB. On DG1 everything is snooped,
>> always, and so should be coherent.
>>
>> I915_GEM_DOMAIN_GTT looks like it's for the aperture which is now gone
>> for DGFX, so hopefully can also be safely rejected.
>>
>> 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_domain.c |  7 +++++++
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 10 ++++++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>> index d0c91697bb22..e3459a524e64 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>> @@ -577,6 +577,13 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, 
>> void *data,
>>           goto out_unpin;
>>       }
>> +    if (IS_DGFX(to_i915(obj->base.dev)) && obj->mm.n_placements == 1 &&
>> +        i915_gem_object_placements_contain_type(obj, 
>> INTEL_MEMORY_SYSTEM) &&
>> +        read_domains != I915_GEM_DOMAIN_CPU) {
>> +        err = -EINVAL;
>> +        goto out_unpin;
>> +    }
>> +
>>       if (read_domains & I915_GEM_DOMAIN_WC)
>>           err = i915_gem_object_set_to_wc_domain(obj, write_domain);
>>       else if (read_domains & I915_GEM_DOMAIN_GTT)
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index f3586b36dd53..afc9f3dc38b9 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -673,6 +673,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
>>                enum i915_mmap_type mmap_type,
>>                u64 *offset, struct drm_file *file)
>>   {
>> +    struct drm_i915_private *i915 = to_i915(obj->base.dev);
>>       struct i915_mmap_offset *mmo;
>>       if (i915_gem_object_never_mmap(obj))
>> @@ -697,6 +698,15 @@ __assign_mmap_offset(struct drm_i915_gem_object 
>> *obj,
>>           i915_gem_object_placements_contain_type(obj, 
>> INTEL_MEMORY_LOCAL))
>>           return -ENODEV;
>> +    /*
>> +     * For smem only placements on DGFX we need to default to WB. On DG1
>> +     * everything is snooped always, so should always be coherent.
>> +     */
>> +     if (IS_DGFX(i915) &&
>> +         mmap_type != I915_MMAP_TYPE_WB && obj->mm.n_placements == 1 &&
>> +         i915_gem_object_placements_contain_type(obj, 
>> INTEL_MEMORY_SYSTEM))
>> +        return -ENODEV;
>> +
> 
> Same thing here as in the previous patch.
> 
> Also do we need to modify i915_coherent_map_type() to also include 
> HAS_SNOOP()?
> 
> While we're at it, that "always_coherent" argument to 
> i915_coherent_map_type() appears scary to me and probably needs some 
> documentation. It seems used for page-tables. Is it because we know 
> those are always snooped?

Yeah, it's either because the caller has/will mark the pages as 
coherent(which translates to some special ppGTT bits), or we manually 
flush ourselves. In i915_coherent_map_type() we should account for DG1 
somehow.

Historically I don't think we enabled snooping by default since it's 
considered slow compared to shared LLC. On DG1 this is a different story 
though.

Also the pin_map() interface is pretty much only for kernel internal 
objects, so I don't think we have any users which try to map userspace 
objects with that interface. Ok, except for vm_access it seems, but that 
should hopefully be a simple fix to use the correct caching mode? We can 
maybe add some sanity checking there if someone tries to map a userspace 
object?

For all the other callers of pin_map() which should all be kernel 
internal do we still need to force WB for system memory? By design we 
only support a single mm.mapping there. For lmem we already use WC only.

> 
> /Thomas
> 
> 
>>       mmo = mmap_offset_attach(obj, mmap_type, file);
>>       if (IS_ERR(mmo))
>>           return PTR_ERR(mmo);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx