On 12 July 2017 at 03:47, Marek Olšák <maraeo at gmail.com> wrote: > On Tue, Jul 11, 2017 at 11:20 AM, Dave Airlie <airlied at gmail.com> wrote: >> On 11 July 2017 at 18:36, Christian König <deathsimple at vodafone.de> wrote: >>> Am 11.07.2017 um 08:49 schrieb Dave Airlie: >>>> >>>> On 7 July 2017 at 19:07, Christian König <deathsimple at vodafone.de> wrote: >>>>> >>>>> Hi Dave, >>>>> >>>>> on first glance that looks rather good to me, but there is one things I >>>>> don't really like and I strongly think Marek will absolutely agree on >>>>> that: >>>>> When we add a new CS function then let's get ride of all this >>>>> abstraction! >>>>> >>>>> The new function should get an amdgpu_device_handle and a list of chunks >>>>> to >>>>> submit, nothing else. >>>>> >>>>> When then provide helper functions to generate the chunks out of the >>>>> existing amdgpu_context_handle and amdgpu_bo_list_handle. >>>>> >>>>> That should be perfectly sufficient and extensible for future additions >>>>> as >>>>> well. >>>> >>>> Sounds tempting, but it a bit messier than it looks once I started >>>> digging into it. >>>> >>>> The main things I ran up against is the context sequence mutex protecting >>>> the >>>> kernel submissions per context which would be tricky to figure out why >>>> that is >>>> required (should we be submitting from different contexts on different >>>> threads?) >>> >>> >>> The sequence lock is just to keep last_seq up to date and last_seq just >>> exists because of amdgpu_cs_signal_semaphore. >>> >>> We want to get ride of that, so you can drop support for this altogether in >>> the new IOCTL. >>> >>>> I'd prefer to land this then refactor a new interface, I do wonder if >>>> maybe Marek >>>> would prefer just doing this all in Mesa and avoiding these APIs a bit >>>> more :-) >>>> >>>> Once I get the syncobjs in I might look at internally refactoring the >>>> code a bit more, >>>> then a new API. >>> >>> >>> Actually I wanted to propose just to remove the old semaphore API, it was >>> never used by Mesa or any other open source user. >> >> radv uses it right now until we have syncobjs. >> >> So it should hang around. > > Dave, this may sound outrageous, but think about it: What would happen > if we removed the old semaphore API? Users would have to get a new > RADV after getting new libdrm_amdgpu. Is that really that big of a > deal? It happens with LLVM all the time and we've managed to cope with > that just fine. Cope is the word, I don't think adopting unstable library APIs is an option after you've provided stable library APIs. LLVM is an outlier and totally the opposite of where we'd want to head. I've considered not bothering using kernel syncobjs for non-shared semaphores for a while, so we don't require a new kernel just to provide a feature we already provide. Dave.
