On Thu, 14 Apr 2011 10:03:35 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> ... to clarify just how we use it inside the driver. We still need to
> translate through agp_type for interface into the fake AGP driver.

agp_type has some really confusing semantics...

> -	obj->agp_type = AGP_USER_MEMORY;
> +	obj->cache_level = I915_CACHE_NONE;

AGP_USER_MEMORY is indeed uncached, so this is correct.


> +/* XXX kill agp_type! */
> +static uint32_t cache_level_to_agp_type(struct drm_device *dev,
> +					enum i915_cache_level cache_level)

This should be unsigned int to match the users of this value

> +	switch (cache_level) {
> +	case I915_CACHE_LLC_MLC:
> +		if (INTEL_INFO(dev)->gen >= 6)
> +			return AGP_USER_CACHED_MEMORY_LLC_MLC;

I like to see a comment here:

+               /* Fall through ... */

> +	case I915_CACHE_LLC:
> +		return AGP_USER_CACHED_MEMORY;
> +	default:
> +	case I915_CACHE_NONE:
> +		return AGP_USER_MEMORY;
> +	}

>  	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
> +		int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
> +

This should be 'unsigned int' to match the signature of
intel_gtt_insert_sg_entries and intel_gtt_insert_pages.

-- 
keith.packard@intel.com