Re: [PATCH v3 02/15] drm/i915: Add i915_gem_object_write() to i915_gem.c

[Dave Gordon <david.s.gordon@intel.com>]

On 17/04/15 22:21, yu.dai@intel.com wrote:
> From: "Michael H. Nguyen" <michael.h.nguyen@intel.com>
> 
> i915_gem_object_write() is a generic function to copy data from
> user memory to gem object.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++
>  drivers/gpu/drm/i915/i915_gem.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 40ef672..6e8d106 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2645,6 +2645,9 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
>  void i915_gem_free_object(struct drm_gem_object *obj);
>  void i915_gem_vma_destroy(struct i915_vma *vma);
>  
> +int i915_gem_object_write(struct drm_i915_gem_object *obj,
> +			const void *data, const size_t size);
> +
>  #define PIN_MAPPABLE 0x1
>  #define PIN_NONBLOCK 0x2
>  #define PIN_GLOBAL 0x4
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f7b8766..44154fe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5260,3 +5260,33 @@ bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
>  	return false;
>  }
>  
> +/* Fill the @obj with the @size amount of @data */
> +int i915_gem_object_write(struct drm_i915_gem_object *obj,
> +			const void *data, const size_t size)
> +{
> +	struct sg_table *sg;
> +	size_t bytes;
> +	int ret;
> +
> +	ret = i915_gem_object_get_pages(obj);
> +	if (ret)
> +		return ret;
> +
> +	i915_gem_object_pin_pages(obj);
> +
> +	sg = obj->pages;
> +
> +	bytes = sg_copy_from_buffer(sg->sgl, sg->nents,
> +				    (void *)data, (size_t)size);

The second cast (size_t) is not required; better still, without
that cast the code fits on one line :)

> +	i915_gem_object_unpin_pages(obj);
> +
> +	if (WARN(bytes != size,
> +		 "Failed to upload all data (completed %zu bytes out of %zu total",

There's a mismatched parenthesis in the warning message above.
Also the line is just a bit too long, according to checkpatch :(
Perhaps just "Incomplete transfer, wrote %zu of %zu bytes" ?

The caller is going to issue a message too, if we ever reach
this case, so this code doesn't have to explain too much.

With these fixed,

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

> +		 bytes, size)) {
> +		i915_gem_object_put_pages(obj);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx