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

[Dave Gordon]

On 22/06/15 13:37, Chris Wilson wrote:
> On Mon, Jun 22, 2015 at 12:59:00PM +0100, Dave Gordon wrote:
>> On 19/06/15 09:44, Chris Wilson wrote:
>>> On Thu, Jun 18, 2015 at 07:07:46PM +0100, Dave Gordon wrote:
>>>> 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?
>>>
>>> Either, since the domain will not change whilst you have the lock,
>>> but if you do it before get_pages() you will have a slightly easier
>>> error path.
>>
>> OK, call to i915_gem_object_set_to_cpu_domain(obj, true) added right
>> after the i915_gem_alloc_object(); also, since we now have multiple
>> failure paths where the ability to distinguish them might be useful (and
>> since this function is a public addition to the gem_object repertoire),
>> I've made it return object-or-error-code, with the incomplete-copy case
>> returning ERR_PTR(-EIO).
> 
> I'd stick to only using EIO when we have a GPU failure. Incomplete copy
> is EFAULT.

Done, it now returns -EFAULT on incomplete copy.

>>> Part of the reason why I want a function like this is so that I can
>>> replace it with a stolen object and so need to write the data through a
>>> temporary GGTT mapping. Speak now if you need more flags to the function
>>> to prevent certain classes of objects being created.
>>> -Chris
>>
>> For the GuC, the firmware image is written once by the CPU and
>> thereafter read only by the DMA engine via a GGTT address; other uC
>> devices might have different requirements e.g. the CSR/DMC doesn't have
>> a DMA engine AFAIK and the f/w is transferred to the h/w via MMIO writes
>> by the CPU. The primary reason for storing the image in a GEM object
>> (rather than kmalloc'd space, as the DMC loader does) is to make it
>> pageable; it's needed multiple times, as we have to reload the f/w after
>> reset or on exit from low-power states, but not used the rest of the
>> time. So the existing objects seem a good match.
>>
>> The GuC's pool and log objects, OTOH, must be permanently resident in
>> RAM and permanently mapped via the GGTT above GUC_WOPCM_OFFSET. So for
>> these it would be useful to have an allocator that *didn't* make the
>> object shmfs-backed.
> 
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=3f74a251aa9a3e5c1215226f7857ed53693c563f
> 
> Though I want to use stolen as much as is practically possible, however
> without direct CPU access, stolen is very much an idle fancy.
> -Chris

Yes, that looks useful ...

.Dave.