Re: [PATCH 01/11] drm/i915: Add support for mapping an object page by page

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: ankitprasad.r.sharma@intel.com, intel-gfx@lists.freedesktop.org
Cc: akash.goel@intel.com, shashidhar.hiremath@intel.com
Subject: Re: [PATCH 01/11] drm/i915: Add support for mapping an object page by page
Date: Thu, 14 Jan 2016 10:32:11 +0000	[thread overview]
Message-ID: <5697792B.7030503@linux.intel.com> (raw)
In-Reply-To: <1452752207-30382-2-git-send-email-ankitprasad.r.sharma@intel.com>

On 14/01/16 06:16, ankitprasad.r.sharma@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> Introduced a new vm specfic callback insert_page() to program a single pte in
> ggtt or ppgtt. This allows us to map a single page in to the mappable aperture
> space. This can be iterated over to access the whole object by using space as
> meagre as page size.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/char/agp/intel-gtt.c        |  9 +++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 49 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |  5 ++++
>   include/drm/intel-gtt.h             |  3 +++
>   4 files changed, 66 insertions(+)
>
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 1341a94..7c68576 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -838,6 +838,15 @@ static bool i830_check_flags(unsigned int flags)
>   	return false;
>   }
>
> +void intel_gtt_insert_page(dma_addr_t addr,
> +			   unsigned int pg,
> +			   unsigned int flags)
> +{
> +	intel_private.driver->write_entry(addr, pg, flags);
> +	wmb();
> +}
> +EXPORT_SYMBOL(intel_gtt_insert_page);
> +
>   void intel_gtt_insert_sg_entries(struct sg_table *st,
>   				 unsigned int pg_start,
>   				 unsigned int flags)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 56f4f2e..48bd6a4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2339,6 +2339,23 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
>   #endif
>   }
>
> +static void gen8_ggtt_insert_page(struct i915_address_space *vm,
> +				  dma_addr_t addr,
> +				  uint64_t offset,
> +				  enum i915_cache_level level,
> +				  u32 unused)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> +	gen8_pte_t __iomem *pte =
> +		(gen8_pte_t __iomem *)dev_priv->gtt.gsm +
> +		(offset >> PAGE_SHIFT);
> +
> +	gen8_set_pte(pte, gen8_pte_encode(addr, level, true));
> +	wmb();
> +
> +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> +}
> +
>   static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>   				     struct sg_table *st,
>   				     uint64_t start,
> @@ -2410,6 +2427,23 @@ static void gen8_ggtt_insert_entries__BKL(struct i915_address_space *vm,
>   	stop_machine(gen8_ggtt_insert_entries__cb, &arg, NULL);
>   }
>
> +static void gen6_ggtt_insert_page(struct i915_address_space *vm,
> +				  dma_addr_t addr,
> +				  uint64_t offset,
> +				  enum i915_cache_level level,
> +				  u32 flags)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> +	gen6_pte_t __iomem *pte =
> +		(gen6_pte_t __iomem *)dev_priv->gtt.gsm +
> +		(offset >> PAGE_SHIFT);
> +
> +	iowrite32(vm->pte_encode(addr, level, true, flags), pte);
> +	wmb();
> +
> +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> +}
> +
>   /*
>    * Binds an object into the global gtt with the specified cache level. The object
>    * will be accessible to the GPU via commands whose operands reference offsets
> @@ -2521,6 +2555,18 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
>   	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
>   }
>
> +static void i915_ggtt_insert_page(struct i915_address_space *vm,
> +				  dma_addr_t addr,
> +				  uint64_t offset,
> +				  enum i915_cache_level cache_level,
> +				  u32 unused)
> +{
> +	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
> +		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
> +
> +	intel_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags);
> +}
> +
>   static void i915_ggtt_insert_entries(struct i915_address_space *vm,
>   				     struct sg_table *pages,
>   				     uint64_t start,
> @@ -3050,6 +3096,7 @@ static int gen8_gmch_probe(struct drm_device *dev,
>   	ret = ggtt_probe_common(dev, gtt_size);
>
>   	dev_priv->gtt.base.clear_range = gen8_ggtt_clear_range;
> +	dev_priv->gtt.base.insert_page = gen8_ggtt_insert_page;
>   	dev_priv->gtt.base.insert_entries = gen8_ggtt_insert_entries;
>   	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
>   	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
> @@ -3095,6 +3142,7 @@ static int gen6_gmch_probe(struct drm_device *dev,
>   	ret = ggtt_probe_common(dev, gtt_size);
>
>   	dev_priv->gtt.base.clear_range = gen6_ggtt_clear_range;
> +	dev_priv->gtt.base.insert_page = gen6_ggtt_insert_page;
>   	dev_priv->gtt.base.insert_entries = gen6_ggtt_insert_entries;
>   	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
>   	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
> @@ -3129,6 +3177,7 @@ static int i915_gmch_probe(struct drm_device *dev,
>   	intel_gtt_get(gtt_total, stolen, mappable_base, mappable_end);
>
>   	dev_priv->gtt.do_idle_maps = needs_idle_maps(dev_priv->dev);
> +	dev_priv->gtt.base.insert_page = i915_ggtt_insert_page;
>   	dev_priv->gtt.base.insert_entries = i915_ggtt_insert_entries;
>   	dev_priv->gtt.base.clear_range = i915_ggtt_clear_range;
>   	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index b448ad8..5f86596 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -317,6 +317,11 @@ struct i915_address_space {
>   			    uint64_t start,
>   			    uint64_t length,
>   			    bool use_scratch);
> +	void (*insert_page)(struct i915_address_space *vm,
> +			    dma_addr_t addr,
> +			    uint64_t offset,
> +			    enum i915_cache_level cache_level,
> +			    u32 flags);
>   	void (*insert_entries)(struct i915_address_space *vm,
>   			       struct sg_table *st,
>   			       uint64_t start,

Why not extend the current API to support start page offset and number 
of pages? Could default to full object like today if zero. Eg:

  void (*insert_entries)(struct i915_address_space *vm,
			struct sg_table *st,
+			unsigned page_offset,
+			unsigned num_pages,
			uint64_t start,
			enum i915_cache_level cache_level,
			u32 flags);

That way we would not have two functions for effectively the same thing 
operating on different type of input parameters.

If extending insert_entries is not preferable, then still we could add 
another compatible one, like insert_entries_range or something, and then 
both could share the same underlying implementation for less code.

Like this, this patch already does not match current codebase - see 
assert_rpm_atomic_begin in insert_entries.

Also if API between the two was compatible there would be no need for 
i915_gem_object_get_dma_address() and i915_gem_object_get_page() could 
be used instead.

Regards,

Tvrtko

> diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> index 9e9bddaa5..f49edec 100644
> --- a/include/drm/intel-gtt.h
> +++ b/include/drm/intel-gtt.h
> @@ -13,6 +13,9 @@ void intel_gmch_remove(void);
>   bool intel_enable_gtt(void);
>
>   void intel_gtt_chipset_flush(void);
> +void intel_gtt_insert_page(dma_addr_t addr,
> +			   unsigned int pg,
> +			   unsigned int flags);
>   void intel_gtt_insert_sg_entries(struct sg_table *st,
>   				 unsigned int pg_start,
>   				 unsigned int flags);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx