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

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 01/15] drm/i915: Add i915_gem_object_write() to i915_gem.c
Date: Thu, 18 Jun 2015 19:07:46 +0100	[thread overview]
Message-ID: <558308F2.6040809@intel.com> (raw)
In-Reply-To: <20150618121034.GL24012@nuc-i3427.alporthouse.com>

On 18/06/15 13:10, Chris Wilson wrote:
> On Thu, Jun 18, 2015 at 12:49:55PM +0100, Dave Gordon wrote:
>> On 17/06/15 13:02, Daniel Vetter wrote:
>>> 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.
>>
>> Is it not the case that the new object returned by
>> i915_gem_alloc_object() is
>> (a) of a type that can be mapped into the GTT, and
>> (b) initially in the CPU domain for both reading and writing?
>>
>> So AFAICS the allocate-and-fill function I'm describing (to appear in
>> next patch series respin) doesn't need any further domain handling.
> 
> A i915_gem_object_create_from_data() is a reasonable addition, and I
> suspect it will make the code a bit more succinct.

I shall adopt this name for it :)

> Whilst your statement is true today, calling set_domain is then a no-op,
> and helps document how you use the object and so reduces the likelihood
> of us introducing bugs in the future.
> -Chris

So here's the new function ... where should the set-to-cpu-domain go?
After the pin_pages and before the sg_copy_from_buffer?

/* Allocate a new GEM object and fill it with the supplied data */
struct drm_i915_gem_object *
i915_gem_object_create_from_data(struct drm_device *dev,
                                 const void *data, size_t size)
{
        struct drm_i915_gem_object *obj;
        struct sg_table *sg;
        size_t bytes;
        int ret;

        obj = i915_gem_alloc_object(dev, round_up(size, PAGE_SIZE));
        if (!obj)
                return NULL;

        ret = i915_gem_object_get_pages(obj);
        if (ret)
                goto fail;

        i915_gem_object_pin_pages(obj);
        sg = obj->pages;
        bytes = sg_copy_from_buffer(sg->sgl, sg->nents, (void *)data, size);
        i915_gem_object_unpin_pages(obj);

        if (WARN_ON(bytes != size)) {
                DRM_ERROR("Incomplete copy, wrote %zu of %zu", bytes, size);
                goto fail;
        }

        return obj;

fail:
        drm_gem_object_unreference(&obj->base);
        return NULL;
}

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