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

Dave Gordon
2015-04-21T18:46:44Z

On 21/04/15 19:41, Dave Gordon wrote:
> 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" ?

Also a WARNing should have a trailing newline, so that would be
"Incomplete transfer, wrote %zu of %zu bytes\n" !

> 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