Re: [PATCH 3/6] drm/i915: Support write only mappings

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

On Mon, 19 Sep 2011 21:25:03 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> When doing a GTT mapping, the pages are not backed until taking a fault.
> If we know that the object is write only, when the fault happens we do not need
> to make the pages coherent with the CPU.  This allows for semi fast prefaults
> to occur in libdrm at map time.
> 
> For the non-GTT case, there isn't much to do except make sure we handle things
> properly we we move an object out of the GTT.
> 
> Finally, modify the set domain IOCTL to actually mark objects as write only.
> Current limitation in that once an object is marked write only, it cannot be
> readable.

I couldn't see any reason for this restriction and it does impose a
significant burden on the userspace cache; any buffer used for
write-only mappings must be purged and can't be reused.

> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   32 ++++++++++++++++++++++++--------
>  1 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d4de7f7..48ea4bc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1047,6 +1047,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  	struct drm_i915_gem_object *obj;
>  	uint32_t read_domains = args->read_domains;
>  	uint32_t write_domain = args->write_domain;
> +	int write_only = 0;
>  	int ret;
>  
>  	if (!(dev->driver->driver_features & DRIVER_GEM))
> @@ -1059,11 +1060,15 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  	if (read_domains & I915_GEM_GPU_DOMAINS)
>  		return -EINVAL;
>  
> -	/* Having something in the write domain implies it's in the read
> -	 * domain, and only that read domain.  Enforce that in the request.
> -	 */
> -	if (write_domain != 0 && read_domains != write_domain)
> +	if (write_domain != 0 && read_domains == 0) {

Neat extension of the existing ioctl. Just a little wary of the
potential for mass foot shootings - but this whole extension is about
giving them a machinegun. 

> +		write_only = 1;
> +	} else if (write_domain != 0 && read_domains != write_domain) {
> +		/* Having something in the write domain implies it's in the read
> +		 * domain, and only that read domain. Enforce that in the
> +		 * request.
> +		 */
>  		return -EINVAL;
> +	}
>  
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
> @@ -1084,6 +1089,13 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  		 */
>  		if (ret == -EINVAL)
>  			ret = 0;
> +	} else if (obj->cpu_write_only && !write_only) {
> +		DRM_ERROR("Not yet supported\n");
> +		ret = -EINVAL;
> +		goto unlock;
> +	} else if (write_only) {
> +		atomic_set(&obj->cpu_write_only, 1);

Atomic? Did you intend for obj->cpu_write_only be atomic_t?

> +		obj->base.read_domains = 0;
>  	} else {
>  		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
>  	}
> @@ -1217,9 +1229,13 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  		if (ret)
>  			goto unlock;
>  
> -		ret = i915_gem_object_set_to_gtt_domain(obj, write);
> -		if (ret)
> -			goto unlock;
> +		if (obj->cpu_write_only) {
> +			i915_gem_object_flush_cpu_write_domain(obj);
> +		} else {
> +			ret = i915_gem_object_set_to_gtt_domain(obj, write);
> +			if (ret)
> +				goto unlock;
> +		}
>  	}
>  
>  	if (obj->tiling_mode == I915_TILING_NONE)
> @@ -1586,7 +1602,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>  		obj->dirty = 0;
>  
>  	for (i = 0; i < page_count; i++) {
> -		if (obj->dirty)
> +		if (obj->dirty || obj->cpu_write_only)

Could this be simplified by marking the bo as dirty upon setting the
object write-only?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre