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

Dave Gordon
2015-04-21T18:41:04Z

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;
> +}