[RFC PATCH 0/6] Common preempt fences and semantics

Thu Nov 14 08:38:43 UTC 2024

Am 13.11.24 um 16:34 schrieb Matthew Brost:
>>>>>> Now the ordering works inherently in dma-resv and the scheduler. e.g. No
>>>>>> need to track the last completion fences explictly in preempt fences.
>>>>> I really don't think that this is a good approach. Explicitly keeping the
>>>>> last completion fence in the pre-emption fence is basically a must have as
>>>>> far as I can see.
>>>>>
>>>>> The approach you take here looks like a really ugly hack to me.
>>>>>
>>>> Well, I have to disagree; it seems like a pretty solid, common design.
>> What you basically do is to move the responsibility to signal fences in the
>> right order from the provider of the fences to the consumer of it.
>>
> Are there not ordering rules already built into dma-resv? This is just
> extending those preempt fences.

Well, the usage flags are to distinct which fences should be queried for 
which use case.

The order the fence are used and returned is completely undetermined. 
E.g. currently you can get READ, WRITE fences all mixed together.

> I can maybe buy some of this argument, as if a random yahoo enables
> signaling a preempt fence without properly going through dma-resv or the
> scheduler, then yes, that could break things. But don't do that. In Xe,
> we have exactly two places where these can be triggered: in the TTM BO
> move vfunc (which the scheduler handles) and in the MMU invalidation
> path (dma-resv).

Yeah, but the purpose of all this is that the dma-resv object is 
shareable between device drivers.

That one device driver comes up with a new requirement is certainly 
possible, but then we need to make sure that all others can live with 
that as well.

Just saying that we limit our scope to just the requirements of one 
driver because other are never supposed to see this fence is a recipe 
for problems.

> I would expect all drivers and users of dma-resv and the scheduler with
> preempt fences to use the proper interfaces to signal preempt fences,
> rather than bypassing this thus ensuring the common rules for preempt
> fences are adhered to.

Waiting for fences in any order is part of the design and a requirement 
by some consumers.

See nouveau_fence_sync() for an example of what is usually done, radeon 
has similar requirements.

But those approaches are here to for optimization purposes only and not 
for correctness.

That a driver says "My fences must be waited on first A, then B" is 
something completely new.

>> Since we have tons of consumers of that stuff this is not even remotely a
>> defensive design.
> I can consider other designs, but I do think larger community input may
> be required, as you mentioned there are several consumers of this code.

Each fence is an independent object without dependencies on anything 
else. Imposing some order on consumers of dma_fences is clearly going 
against that.

>>>> Anyway, I think I have this more or less working. I want to run this by
>>>> the Mesa team a bit to ensure I haven't missed anything, and will likely
>>>> post something shortly after.
>>>>
>>>> We can discuss this more after I post and perhaps solicit other
>>>> opinions, weighing the pros and cons of the approaches here. I do think
>>>> they function roughly the same, so something commonly agreed upon would
>>>> be good. Sharing a bit of code, if possible, is always a plus too.
>> Well to make it clear that will never ever get a green light from my side as
>> DMA-buf maintainer. What you suggest here is extremely fragile.
>>
> It is unfortunate that this is your position, and I do feel it is a bit
> premature to suggest as much. I didn’t know being a maintainer was akin
> to being a dictator. As I’ve said multiple times, I feel this warrants a
> bit more discussion with more stakeholders. If this is unacceptable,
> sure, I can change it.

I'm sorry when you feel like that, it's really not my intention to 
dictate anything. I have probably over-reacted.

It's just to me suggesting this solution is so far of that I can't 
really understand how you came up with that.

I perfectly understand that you must have some reason for it, I just 
don't see it.

Maybe we should concentrate on understanding those reasoning first 
instead of discussing a possible solution.

>> Why not simply wait for the pending completion fences as dependency for
>> signaling preemption fences?
>>
>> That should work for all drivers and is trivial to implement as far as I can
>> see.
> Again, this is hard to understand without a clear picture of what AMD is
> doing. I pointed out a dma-fencing problem in the code on the list, and
> the response was, "Well, we have some magic ordering rules that make it
> safe." That might be true, but if you annotated your code, lockdep would
> complain. Anything that cannot be annotated is a non-starter for me.
> This makes me nervous that, in fact, it is not as trivial as you
> suggest, nor is the design as sound as you believe.

I'm pretty sure that the code is not even remotely bug free. The design 
and code has been under development for the last 16 month or so and is 
now published bit by bit.

We completely missed both during internal review as well as testing that 
lockdep should complain about it and I'm actually not sure why it doesn't.

The full code should land in amd-staging-drm-next in the next few 
days/weeks, I can give you a detailed date later today.

> Anyways, I'll still likely post a common solution with annotations. If
> it is rejected, so be it, and I will rework it.

Well that sounds great. But let us discuss what this solution looks like 
instead of jumping ahead and implementing something.

Regards,
Christian.

>
> In spirit of open development here is my code in a public branch:
>
> Kernel:https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-umd-submission/-/tree/v2-11-13-24?ref_type=heads
> IGT:https://gitlab.freedesktop.org/mbrost/igt-gpu-tools-umd-submission/-/tree/umd_submission.v2?ref_type=heads
>
> Matt
>
>> Regards,
>> Christian.
>>
>>>> Matt
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20241114/e7f065de/attachment.htm>