[Intel-gfx] [PATCH 8/8] drm/i915: Refactor execbuffer relocation writing

[Chris Wilson]

On Thu, Jun 09, 2016 at 04:31:25PM +0300, Joonas Lahtinen wrote:
> On to, 2016-06-09 at 12:29 +0100, Chris Wilson wrote:
> > With in the introduction of the reloc page cache, we are just one step
> > away from refactoring the relocation write functions into one. Not only
> > does it tidy the code (slightly), but it greatly simplifies the control
> > logic much to gcc's satisfaction.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 289 +++++++++++++++--------------
> >  1 file changed, 150 insertions(+), 139 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 318c71b663f4..bda8ec8b02e6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -34,6 +34,8 @@
> >  #include 
> >  #include 
> >  
> > +#define DBG_USE_CPU_RELOC 0 /* force relocations to use the CPU write path */
> 
> Better connect to the new debug Kconfig menu you introduced?

I don't think so, it's not even intended as a general debug feature.
Just needed to force certain code paths for ease of testing.

That becomes more important later on when we have fewer reasons to use
the cpu relocation path on !llc.


> > -static int
> > -relocate_entry_cpu(struct drm_i915_gem_object *obj,
> > -		   struct drm_i915_gem_relocation_entry *reloc,
> > -		   struct reloc_cache *cache,
> > -		   uint64_t target_offset)
> > +static void *reloc_iomap(struct drm_i915_gem_object *obj,
> > +			 struct reloc_cache *cache,
> > +			 int page)
> >  {
> > -	struct drm_device *dev = obj->base.dev;
> > -	uint32_t page_offset = offset_in_page(reloc->offset);
> > -	uint64_t delta = relocation_target(reloc, target_offset);
> > -	char *vaddr;
> > -	int ret;
> > +	void *vaddr;
> >  
> > -	ret = i915_gem_object_set_to_cpu_domain(obj, true);
> > -	if (ret)
> > -		return ret;
> > +	if (cache->vaddr) {
> > +		io_mapping_unmap_atomic(unmask_page(cache->vaddr));
> > +	} else {
> > +		struct i915_vma *vma;
> > +		int ret;
> >  
> > -	vaddr = reloc_kmap(obj, cache, reloc->offset >> PAGE_SHIFT);
> > -	*(uint32_t *)(vaddr + page_offset) = lower_32_bits(delta);
> > +		if (use_cpu_reloc(obj))
> > +			return NULL;
> >  
> > -	if (INTEL_GEN(dev) >= 8) {
> > -		page_offset += sizeof(uint32_t);
> > -		if (page_offset == PAGE_SIZE) {
> > -			vaddr = reloc_kmap(obj, cache, cache->page + 1);
> > -			page_offset = 0;
> > -		}
> > -		*(uint32_t *)(vaddr + page_offset) = upper_32_bits(delta);
> > -	}
> > +		vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> > +					       PIN_MAPPABLE | PIN_NONBLOCK);
> > +		if (IS_ERR(vma))
> > +			return NULL;
> >  
> > -	return 0;
> > -}
> 
> Ugh, did you simultaneously move and rename functions. This is rather
> hard to follow from this point on...

No, it's a bunch of function removals. I have a feeling it is going to
look ugly in diff no matter what, unless diff stops trying to relate
uncorresponding code.
-Chirs

-- 
Chris Wilson, Intel Open Source Technology Centre