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

[Daniel Vetter]

On Thu, Jun 18, 2015 at 12:49:55PM +0100, Dave Gordon wrote:
> On 17/06/15 13:02, Daniel Vetter wrote:
> > 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 at 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 at intel.com>
> >>>> Signed-off-by: Dave Gordon <david.s.gordon at 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.
> 
> The usage pattern for the particular case is going to be:
> 	Once-only:
> 		Allocate
> 		Fill
> 	Then each time GuC is (re-)initialised:
> 		Map to GTT
> 		DMA-read from buffer into GuC private memory
> 		Unmap
> 	Only on unload:
> 		Dispose
> 
> So our object is write-once by the CPU (and that's always the first
> operation), thereafter read-occasionally by the GuC's DMA engine.

Yup. The problem is more that on atom platforms the objects aren't
coherent by default and generally you need to do something. Hence we
either have
- an explicit set_caching call to document that this is a gpu object which
  is always coherent (so also on chv/bxt), even when that's a no-op on big
  core
- or wrap everything in set_domain calls, even when those are no-ops too.

If either of those lack, reviews tend to freak out preemptively and the
reptil brain takes over ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch