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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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?

/Thomas


>   	mmo = mmap_offset_attach(obj, mmap_type, file);
>   	if (IS_ERR(mmo))
>   		return PTR_ERR(mmo);