Re: [PATCH 7/9] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Chris Wilson <chris@chris-wilson.co.uk>]

On Mon, Dec 14, 2015 at 11:16:09AM +0530, ankitprasad.r.sharma@intel.com wrote:
> @@ -1150,8 +1252,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  	 * perspective, requiring manual detiling by the client.
>  	 */
>  	if (obj->tiling_mode == I915_TILING_NONE &&
> -	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> -	    cpu_write_needs_clflush(obj)) {
> +	    (!obj->base.filp ||
> +	    (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> +	    cpu_write_needs_clflush(obj)))) {

This is too confusing. Move the write_domain check to needs_clflush
ala:

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8edc56a34caa..fd3c73c8ab77 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -49,6 +49,9 @@ static bool cpu_cache_is_coherent(struct drm_device *dev,
 
 static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
 {
+       if (obj->base.write_domain == I915_GEM_DOMAIN_CPU)
+               return false;
+
        if (!cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
                return true;
 
@@ -1073,7 +1076,6 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
         * perspective, requiring manual detiling by the client.
         */
        if (obj->tiling_mode == I915_TILING_NONE &&
-           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
            cpu_write_needs_clflush(obj)) {
                ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file);
                /* Note that the gtt paths might fail with non-page-backed user
@@ -3159,9 +3161,7 @@ out:
         * object is now coherent at its new cache level (with respect
         * to the access domain).
         */
-       if (obj->cache_dirty &&
-           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
-           cpu_write_needs_clflush(obj)) {
+       if (obj->cache_dirty && cpu_write_needs_clflush(obj)) {
                if (i915_gem_clflush_object(obj, true))
                        i915_gem_chipset_flush(obj->base.dev);
        }


and the negative (tiling mode) test to i915_gem_gtt_pwrite_fast.

Then it reads as

if (obj->base.filp == NULL || cpu_write_needs_clflush(obj))
	ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);

>  		/* Note that the gtt paths might fail with non-page-backed user
>  		 * pointers (e.g. gtt mappings when moving data between
> @@ -1161,7 +1264,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  	if (ret == -EFAULT || ret == -ENOSPC) {
>  		if (obj->phys_handle)
>  			ret = i915_gem_phys_pwrite(obj, args, file);
> -		else
> +		else if (obj->base.filp)
>  			ret = i915_gem_shmem_pwrite(dev, obj, args, file);
Forgot
		else
			ret = -ENODEV;

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx