Re: [igt-dev] [PATCH i-g-t] lib/i915/gem_mman: Add support for GEM_MMAP_OFFSET ioctl

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/02/2020 10:50, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-02-10 10:47:23)
>>
>> On 10/02/2020 10:37, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-02-10 10:31:40)
>>>>
>>>> On 05/12/2019 12:40, Chris Wilson wrote:
>>>>> Quoting Zbigniew Kempczyński (2019-12-05 12:31:13)
>>>>>> With introduction of new kernel ioctl we need to cover this in
>>>>>> the IGT's. Patch adds mmap functions appropriate for this.
>>>>>>
>>>>>> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
>>>>>> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com>
>>>>>> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>
>>>>> Do we really have people outside of gem_mmap_wc.c and gem_mmap_offset.c
>>>>> that care whether their WC/WB mmapping is provided by MMAP_OFFSET_IOCTL
>>>>> or MMAP_IOCTL. They just expect it to work, I would have thought --
>>>>> basically just gem_mmap__device_coherent (and maybe the alternative would
>>>>> be gem_mmap__cache_coherent, don't quote me on that :)
>>>>>
>>>>> It's not a huge deal, I think it falls under overengineering that will
>>>>> simply lead to confusion of "when do I use one and not the other?"
>>>>
>>>> What shall we do with call sites which used to do:
>>>>
>>>>      gem_mmap__gtt()
>>>>      get_set_domain(GTT)
>>>>
>>>> And now do:
>>>>
>>>>      gem_mmap__device_coherent()
>>>>      gem_set_domain(GTT)
>>>>
>>>> ?
>>>
>>> For the large part it's immaterial. The subtly is only when mixing GEM
>>> operations on the same buffer within the same critical section.
>>>
>>>> Sounds like we need a helper to either lets the caller know which
>>>> flavour of mmap was used so correct flushing can be done, or a
>>>> synchronous helper where possible sounds much easier:
>>>>
>>>>      gem_mmap__device_coherent_sync() - does the correct set_domain internally
>>>
>>> gem_mmap__device_coherent_domain() to return the domain and sync to roll
>>> up the common actions into one (though I'm not fond of hiding subtle
>>> details in helpers when the tests are meant to be exercising those
>>> subtle details :(
>>
>> So gem_mmap__device_coherent_domain calls the set_domain, not just
>> returns the needed domain?
> 
> I would say returns the domain, with _sync being the helper that mmaps
> + set-domain in one.

Sounds good to me.

Sreedhar could you sketch this and convert gem_ctx_shared as an example?

Regards,

Tvrtko

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev