Re: [PATCH v3 33/37] drm/i915: cpu-map based dumb buffers

Chris Wilson
2019-08-10T11:44:45Z

Quoting Matthew Auld (2019-08-09 23:26:39)
> From: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>
> If there is no aperture we can't use map_gtt to map dumb buffers, so we
> need a cpu-map based path to do it. We prefer map_gtt on platforms that
> do have aperture.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 +++++++++++++++++-
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 +
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> 4 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 304ea578fd30..4fe83e31c1b3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -500,7 +500,8 @@ static void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
> list_for_each_entry(mmo, &obj->mmap_offsets, offset) {
> if (mmo->mmap_type == I915_MMAP_TYPE_OFFSET_WC ||
> mmo->mmap_type == I915_MMAP_TYPE_OFFSET_WB ||
> - mmo->mmap_type == I915_MMAP_TYPE_OFFSET_UC)
> + mmo->mmap_type == I915_MMAP_TYPE_OFFSET_UC ||
> + mmo->mmap_type == I915_MMAP_TYPE_DUMB_WC)

Why even special case it???

Even at three logical-ors I suggest you start encoding flags into
mmap_type.

In this case, why isn't it something like

if (has_node(&mmo->vma_node))
> drm_vma_node_unmap(&mmo->vma_node,
> obj->base.dev->anon_inode->i_mapping);

> }
> @@ -602,6 +603,19 @@ __assign_gem_object_mmap_data(struct drm_file *file,
> return ret;
> }
>
> +int
> +i915_gem_mmap_dumb(struct drm_file *file,
> + struct drm_device *dev,
> + u32 handle,
> + u64 *offset)
> +{
> + struct drm_i915_private *i915 = dev->dev_private;
> + enum i915_mmap_type mmap_type = HAS_MAPPABLE_APERTURE(i915) ?
> + I915_MMAP_TYPE_GTT : I915_MMAP_TYPE_DUMB_WC;

It's a linear buffer, you can always do WC now that you can return an
offset for WC.

> +
> + return __assign_gem_object_mmap_data(file, handle, mmap_type, offset);
> +}
> +
> /**
> * i915_gem_mmap_gtt_ioctl - prepare an object for GTT mmap'ing
> * @dev: DRM device
> @@ -714,6 +728,7 @@ static void set_vmdata_mmap_offset(struct i915_mmap_offset *mmo, struct vm_area_
> {
> switch (mmo->mmap_type) {
> case I915_MMAP_TYPE_OFFSET_WC:
> + case I915_MMAP_TYPE_DUMB_WC:
> vma->vm_page_prot =
> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> break;
> @@ -801,6 +816,7 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> case I915_MMAP_TYPE_OFFSET_WC:
> case I915_MMAP_TYPE_OFFSET_WB:
> case I915_MMAP_TYPE_OFFSET_UC:
> + case I915_MMAP_TYPE_DUMB_WC:

Yup, that was quite redundant.