[Intel-gfx] [PATCH 01/12] drm/i915: Add support for mapping an object page by page

[Ankitprasad Sharma]

On Wed, 2016-04-20 at 13:04 +0100, Chris Wilson wrote:
> On Wed, Apr 20, 2016 at 04:47:28PM +0530, ankitprasad.r.sharma at intel.com wrote:
> > From: Chris Wilson <chris at 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.
> > 
> > v2: Added low level rpm assertions to insert_page routines (Chris)
> > 
> > v3: Added POSTING_READ post register write (Tvrtko)
> > 
> > v4: Rebase (Ankit)
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >  drivers/char/agp/intel-gtt.c        |  9 +++++
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 67 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++
> >  include/drm/intel-gtt.h             |  3 ++
> >  4 files changed, 84 insertions(+)
> > 
> > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> > index aef87fd..cea0ae3 100644
> > --- a/drivers/char/agp/intel-gtt.c
> > +++ b/drivers/char/agp/intel-gtt.c
> > @@ -840,6 +840,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 9f165fe..da1819d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2353,6 +2353,29 @@ 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->ggtt.gsm +
> > +		(offset >> PAGE_SHIFT);
> > +	int rpm_atomic_seq;
> > +
> > +	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
> > +
> > +	gen8_set_pte(pte, gen8_pte_encode(addr, level, true));
> > +	wmb();
> > +
> > +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> > +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> 
> Actually, we don't need these at all for the insert-page API (current
> users in pread/pwrite/reloc/gpu-error-capture) as they go through the
> GTT and not the GPU TLB. I would much rather we punt these to the caller
> with a vm->flush() which we can add later. When I say later, we already
> have the GUC pretending to be the GGTT and adding code where it doesn't
> belong...

I was trying to test the stolen memory patches on SKL with the removed
FLSH_CNTL write. Apparently, the insert_page routine does not seem to be
working. As I was verifying if the stolen-backed object is zeroed out
(using i915_gem_object_clear, which makes use of insert_page) on
creation but it failed (I was getting a garbage value instead of zeros).
And on adding this write it works as expected.

I think this write cannot be removed after all.

Thanks,
Ankit
> 
> All we need here is the wmb() to order the PTE write (and flush the WCB)
> with the use afterwards. The caller has to provide the wmb() to order
> its writes before a subsequent PTE change.
> 
> In a few patches time we can also kill the rpm_atomic asserts and the
> dev_priv local.
> -Chris
>