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). > 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. .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
