[Intel-gfx] [PATCH 08/10] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Chris Wilson]

On Tue, Jan 26, 2016 at 01:13:34AM +0530, ankitprasad.r.sharma at intel.com wrote:
> +static int
> +i915_gem_gtt_copy(struct drm_device *dev,

I still do not like this name as I have no direction information.

> +		   struct drm_i915_gem_object *obj, uint64_t size,
> +		   uint64_t data_offset, uint64_t data_ptr)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mm_node node;
> +	char __user *user_data;
> +	uint64_t remain;
> +	uint64_t offset;
> +	int ret = 0;
> +
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret) {
> +		ret = insert_mappable_node(dev_priv, &node, PAGE_SIZE);
> +		if (ret)
> +			goto out;
> +
> +		ret = i915_gem_object_get_pages(obj);
> +		if (ret) {
> +			remove_mappable_node(&node);
> +			goto out;
> +		}
> +
> +		i915_gem_object_pin_pages(obj);
> +	} else {
> +		node.start = i915_gem_obj_ggtt_offset(obj);
> +		node.allocated = false;
> +	}
> +
> +	ret = i915_gem_object_set_to_gtt_domain(obj, false);
> +	if (ret)
> +		goto out_unpin;
> +
> +	ret = i915_gem_object_put_fence(obj);
> +	if (ret)
> +		goto out_unpin;

This only needs to be done for the whole-obj path. The single-page path
explicitly avoids the fence register.

> +	user_data = to_user_ptr(data_ptr);
> +	remain = size;
> +	offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
> +
> +	mutex_unlock(&dev->struct_mutex);
> +	if (likely(!i915.prefault_disable))
> +		ret = fault_in_multipages_writeable(user_data, remain);

Handle the EFAULT!

> +	while (remain > 0) {
> +		/* Operation in this page
> +		 *
> +		 * page_base = page offset within aperture
> +		 * page_offset = offset within page
> +		 * page_length = bytes to copy for this page
> +		 */
> +		u32 page_base = node.start;
> +		unsigned page_offset = offset_in_page(offset);
> +		unsigned page_length = PAGE_SIZE - page_offset;
> +		page_length = remain < page_length ? remain : page_length;
> +		if (node.allocated) {
> +			wmb();
> +			dev_priv->gtt.base.insert_page(&dev_priv->gtt.base,
> +						       i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
> +						       node.start,
> +						       I915_CACHE_NONE, 0);
> +			wmb();
> +		} else {
> +			page_base += offset & PAGE_MASK;
> +		}
> +		/* This is a slow read/write as it tries to read from
> +		 * and write to user memory which may result into page
> +		 * faults, and so we cannot perform this under struct_mutex.
> +		 */
> +		if (slow_user_access(dev_priv->gtt.mappable, page_base,
> +				     page_offset, user_data,
> +				     page_length, false)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		remain -= page_length;
> +		user_data += page_length;
> +		offset += page_length;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);
> +	WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false));

This is user triggerable. It would take a silly user, but it is
possible, so we can't WARN.

	if (ret == 0 && (obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0)) {
		/* The user has modified the object whilst we tried
		 * reading from it, and we now have no idea what domain
		 * the pages should be in. As we have just been touching
		 * them directly, flush everything back to the GTT
		 * domain.
		 */
		ret = i915_gem_object_set_to_gtt_domain(obj, false);
	}

> @@ -1170,9 +1308,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  	 * pread/pwrite currently are reading and writing from the CPU
>  	 * 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)) {
> +	if (!obj->base.filp || cpu_write_needs_clflush(obj)) {

That's looking much tidier.

>  		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
> @@ -1182,8 +1318,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  	if (ret == -EFAULT || ret == -ENOSPC) {

We don't return ENOSPC any more, so one less complication!

>  		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);
> +		else
> +			ret = -ENODEV;

-- 
Chris Wilson, Intel Open Source Technology Centre