Re: [PATCH 07/22] drm/i915: Refactor execbuffer relocation writing

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2016-08-16 at 11:42 +0100, Chris Wilson wrote:
> @@ -278,6 +283,9 @@ static void eb_destroy(struct eb_vmas *eb)
>  
>  static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
>  {
> +	if (DBG_USE_CPU_RELOC)
> +		return DBG_USE_CPU_RELOC > 0;

If DBG_USE_CPU_RELOC == 0, this path is never taken. So it would have
to be set at -1 to yield false. Unexpected when defining it. 0 and 1 is
the de facto. So drop a comment at defining site.

> +#define KMAP 0x4

At least make a comment that CLFLUSH_AFTER and CLFLUSH_BEFORE are also
in the flag set.

> +
>  static void reloc_cache_fini(struct reloc_cache *cache)
>  {
> +	void *vaddr;
> +
>  	if (!cache->vaddr)
>  		return;
>  
> -	switch (cache->type) {
> -	case KMAP:
> -		kunmap_atomic(cache->vaddr);
> -		break;
> +	vaddr = unmask_page(cache->vaddr);
> +	if (cache->vaddr & KMAP) {
> +		if (cache->vaddr & CLFLUSH_AFTER)
> +			mb();
>  
> -	case IOMAP:
> -		io_mapping_unmap_atomic(cache->vaddr);
> -		break;
> +		kunmap_atomic(vaddr);
> +		i915_gem_object_unpin_pages((struct drm_i915_gem_object *)cache->node.mm);

I'd prefer i915_gem_obj_cleanup_shmem_write() for symmetry or a comment
here.

> +	} else {
> +		io_mapping_unmap_atomic(vaddr);
> +		i915_vma_unpin((struct i915_vma *)cache->node.mm);

This does have a clear counterpart.

> -	clflush_write32(vaddr + page_offset, lower_32_bits(delta));
> +	clflush_write32(vaddr + offset_in_page(offset),
> +			lower_32_bits(target_offset),
> +			cache->vaddr);

unmap_flags(cache->vaddr) for clarity

This could use another set of eyes, the patch is horribly mangled.

But with my eyes, with couple of comments added;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx