Re: [RFC PATCH 02/29] dma-fence: Add dma_fence_user_fence

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]

Am 22.11.24 um 03:35 schrieb Matthew Brost:
> [SNIP]
>>> The flow here would be, a user job needs to wait on external dma-fence
>>> in a syncobj, syncfile, etc..., call the convert dma-fence to user fence
>>> IOCTL before the submission (patch 22, 28 in this series), program the
>>> wait via ring instructions, and then do the user submission. This would
>>> avoid blocking on external dma-fences in the submission path.
>>>
>>> I think this makes sense and having a light weight helper to normalize
>>> this flow across drivers makes a bit sense too.
>> Well we have pretty much the same concept, but all writes are done by the
>> hardware and not go by a round-trip through the CPU.
>>
> Hmm, I'm curious how that works on your end. Doesn't the DMA fence
> signaling have to go through the kernel?

No, we have a protected_fence packet which basically writes the current 
processing status (RPTR) into a location defined by the kernel driver.

So neither the value nor the location of the write can be manipulated by 
userspace.

This way queues can signal each other their status without going through 
a CPU round trip nor writing into a shared memory location. Writing into 
a memory location can probably be done by any hardware, but that usually 
has tons of scheduling implications, e.g. priority inversion etc...

> Yes, of course, in Xe we program seqno writes through the GPU when we
> can, but our bind code currently opportunistically bypasses the GPU.
> Eventually, I think it will become a 100% CPU operation for various
> reasons. Likewise, if a fence is coming from an external process, there
> is no GPU job to write the seqno.

Good point, for that use case the implementation would be useful for us 
as well.

> Of course, we could issue a GPU job to
> write the seqno, but this would add latency. In the case of VM bind, we
> really want to completely decouple that from the GPU for various reasons
> (I can explain why if needed, but it's kind of off-topic).
>
>> We have a read only mapped seq64 area in the kernel reserved part of the VM
>> address space.
>>
>> Through this area the queues can see each others fence progress and we can
>> say things like BO mapping and TLB flush are finished when this seq64
>> increases please suspend further processing until you see that.
>>
>> Could be that this is useful for more than XE, but at least for AMD I
>> currently don't see that.
>>
> Ok, we have no other current users, and if you feel it is better to
> carry this in Xe in a way that it can be moved to the common layer
> later, there’s no issue with that. We have several other components like
> this in Xe that are generic but currently live in Xe.

It's probably overkill for DMA-buf, but maybe we can put that stuff into 
DRM.

Christian.

>
> Matt
>
>> Regards,
>> Christian.
>>
>>> Matt
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> +}
>>>>> +EXPORT_SYMBOL(dma_fence_user_fence_free);
>>>>> +
>>>>> +/**
>>>>> + * dma_fence_user_fence_attach() - Attach user fence to dma-fence
>>>>> + *
>>>>> + * @fence: fence
>>>>> + * @user_fence user fence
>>>>> + * @map: IOSYS map to write seqno to
>>>>> + * @seqno: seqno to write to IOSYS map
>>>>> + *
>>>>> + * Attach a user fence, which is a seqno write to an IOSYS map, to a DMA fence.
>>>>> + * The caller must guarantee that the memory in the IOSYS map doesn't move
>>>>> + * before the fence signals. This is typically done by installing the DMA fence
>>>>> + * into the BO's DMA reservation bookkeeping slot from which the IOSYS was
>>>>> + * derived.
>>>>> + */
>>>>> +void dma_fence_user_fence_attach(struct dma_fence *fence,
>>>>> +				 struct dma_fence_user_fence *user_fence,
>>>>> +				 struct iosys_map *map, u64 seqno)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	user_fence->map = *map;
>>>>> +	user_fence->seqno = seqno;
>>>>> +
>>>>> +	err = dma_fence_add_callback(fence, &user_fence->cb, user_fence_cb);
>>>>> +	if (err == -ENOENT)
>>>>> +		user_fence_cb(NULL, &user_fence->cb);
>>>>> +}
>>>>> +EXPORT_SYMBOL(dma_fence_user_fence_attach);
>>>>> diff --git a/include/linux/dma-fence-user-fence.h b/include/linux/dma-fence-user-fence.h
>>>>> new file mode 100644
>>>>> index 000000000000..8678129c7d56
>>>>> --- /dev/null
>>>>> +++ b/include/linux/dma-fence-user-fence.h
>>>>> @@ -0,0 +1,31 @@
>>>>> +/* SPDX-License-Identifier: MIT */
>>>>> +/*
>>>>> + * Copyright © 2024 Intel Corporation
>>>>> + */
>>>>> +
>>>>> +#ifndef __LINUX_DMA_FENCE_USER_FENCE_H
>>>>> +#define __LINUX_DMA_FENCE_USER_FENCE_H
>>>>> +
>>>>> +#include <linux/dma-fence.h>
>>>>> +#include <linux/iosys-map.h>
>>>>> +
>>>>> +/** struct dma_fence_user_fence - User fence */
>>>>> +struct dma_fence_user_fence {
>>>>> +	/** @cb: dma-fence callback used to attach user fence to dma-fence */
>>>>> +	struct dma_fence_cb cb;
>>>>> +	/** @map: IOSYS map to write seqno to */
>>>>> +	struct iosys_map map;
>>>>> +	/** @seqno: seqno to write to IOSYS map */
>>>>> +	u64 seqno;
>>>>> +};
>>>>> +
>>>>> +struct dma_fence_user_fence *dma_fence_user_fence_alloc(void);
>>>>> +
>>>>> +void dma_fence_user_fence_free(struct dma_fence_user_fence *user_fence);
>>>>> +
>>>>> +void dma_fence_user_fence_attach(struct dma_fence *fence,
>>>>> +				 struct dma_fence_user_fence *user_fence,
>>>>> +				 struct iosys_map *map,
>>>>> +				 u64 seqno);
>>>>> +
>>>>> +#endif

[-- Attachment #2: Type: text/html, Size: 6605 bytes --]