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@xxxxxxxxxxxxxxx> Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx