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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 17, 2015 at 08:23:40AM +0100, Dave Gordon wrote:
> On 15/06/15 21:09, Chris Wilson wrote:
> > On Mon, Jun 15, 2015 at 07:36:19PM +0100, Dave Gordon wrote:
> >> From: Alex Dai <yu.dai@intel.com>
> >>
> >> i915_gem_object_write() is a generic function to copy data from a plain
> >> linear buffer to a paged gem object.
> >>
> >> We will need this for the microcontroller firmware loading support code.
> >>
> >> Issue: VIZ-4884
> >> Signed-off-by: Alex Dai <yu.dai@intel.com>
> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h |    2 ++
> >>  drivers/gpu/drm/i915/i915_gem.c |   28 ++++++++++++++++++++++++++++
> >>  2 files changed, 30 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 611fbd8..9094c06 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2713,6 +2713,8 @@ void *i915_gem_object_alloc(struct drm_device *dev);
> >>  void i915_gem_object_free(struct drm_i915_gem_object *obj);
> >>  void i915_gem_object_init(struct drm_i915_gem_object *obj,
> >>  			 const struct drm_i915_gem_object_ops *ops);
> >> +int i915_gem_object_write(struct drm_i915_gem_object *obj,
> >> +			  const void *data, size_t size);
> >>  struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> >>  						  size_t size);
> >>  void i915_init_vm(struct drm_i915_private *dev_priv,
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index be35f04..75d63c2 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -5392,3 +5392,31 @@ 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, 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);
> > 
> > You don't set the object into the CPU domain, or instead manually handle
> > the domain flushing. You don't handle objects that cannot be written
> > directly by the CPU, nor do you handle objects whose representation in
> > memory is not linear.
> > -Chris
> 
> No we don't handle just any random gem object, but we do return an error
> code for any types not supported. However, as we don't really need the
> full generality of writing into a gem object of any type, I will replace
> this function with one that combines the allocation of a new object
> (which will therefore definitely be of the correct type, in the correct
> domain, etc) and filling it with the data to be preserved.

Domain handling is required for all gem objects, and the resulting bugs if
you don't for one-off objects are absolutely no fun to track down.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx