Hi Srini,
On 3/4/26 10:12, SHANMUGAM, SRINIVASAN wrote:
> [Public]
>
> Hi Christian,
>
> Good Morning!
>
> Thank you! for taking the time to review and for your valuable feedback.
>
> Few clarifications as below please:
>
>> -----Original Message-----
>> From: Koenig, Christian <[email protected]>
>> Sent: Monday, March 2, 2026 6:31 PM
>> To: SHANMUGAM, SRINIVASAN <[email protected]>;
>> Deucher, Alexander <[email protected]>
>> Cc: [email protected]; Kasiviswanathan, Harish
>> <[email protected]>; Kuehling, Felix <[email protected]>
>> Subject: Re: [PATCH v3 1/4] drm/amdgpu: Add render-node EVENTFD manager
>> core
>>
>> On 3/2/26 04:02, Srinivasan Shanmugam wrote:
>>> Introduce a per-drm_file eventfd manager to support render-node event
>>> subscriptions.
>>>
>>> The manager is implemented in amdgpu_eventfd.[ch] and is owned by the
>>> drm_file (amdgpu_fpriv). It maps event_id -> eventfd_id object, where
>>> each eventfd_id can have multiple eventfds bound (fan-out).
>>>
>>> The design is IRQ-safe for signaling: IRQ path takes the xarray lock
>>> (irqsave) and signals eventfds while still holding the lock.
>>>
>>> This patch only adds the core manager and wires its lifetime into
>>> open/postclose.
>>>
>>> Cc: Harish Kasiviswanathan <[email protected]>
>>> Cc: Felix Kuehling <[email protected]>
>>> Cc: Alex Deucher <[email protected]>
>>> Suggested-by: Christian König <[email protected]>
>>> Signed-off-by: Srinivasan Shanmugam <[email protected]>
>>> Change-Id: Iac025dcb7c1b4f9ed74ac4190085e1925e2c8e68
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/Makefile | 3 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 5 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_eventfd.c | 321
>>> ++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_eventfd.h |
>> 76 +++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 28 +-
>>> 5 files changed, 430 insertions(+), 3 deletions(-) create mode
>>> 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_eventfd.c
>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_eventfd.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> index 006d49d6b4af..30b1cf3c6cdf 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> @@ -67,7 +67,8 @@ amdgpu-y += amdgpu_device.o amdgpu_reg_access.o
>> amdgpu_doorbell_mgr.o amdgpu_kms
>>> amdgpu_fw_attestation.o amdgpu_securedisplay.o \
>>> amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
>>> amdgpu_ring_mux.o amdgpu_xcp.o amdgpu_seq64.o amdgpu_aca.o
>> amdgpu_dev_coredump.o \
>>> - amdgpu_cper.o amdgpu_userq_fence.o amdgpu_eviction_fence.o
>> amdgpu_ip.o
>>> + amdgpu_cper.o amdgpu_userq_fence.o amdgpu_eviction_fence.o
>> amdgpu_ip.o \
>>> + amdgpu_eventfd.o
>>>
>>> amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 1e71a03c8bba..9e650b3707e3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -44,6 +44,7 @@
>>> #include <linux/hashtable.h>
>>> #include <linux/dma-fence.h>
>>> #include <linux/pci.h>
>>> +#include <linux/xarray.h>
>>>
>>> #include <drm/ttm/ttm_bo.h>
>>> #include <drm/ttm/ttm_placement.h>
>>> @@ -434,6 +435,8 @@ struct amdgpu_flip_work {
>>> bool async;
>>> };
>>>
>>> +struct amdgpu_eventfd_mgr;
>>> +
>>> /*
>>> * file private structure
>>> */
>>> @@ -453,6 +456,8 @@ struct amdgpu_fpriv {
>>>
>>> /** GPU partition selection */
>>> uint32_t xcp_id;
>>> +
>>> + struct amdgpu_eventfd_mgr *eventfd_mgr;
>>
>> If possible we should embed that structure here instead of having a pointer.
>
> I understand you prefer embedding struct amdgpu_eventfd_mgr in amdgpu_fpriv.
>
> In my current series the USERQ fence driver holds a ref to the manager, so
> mgr can outlive fpriv.
>
> Question: Do you want to keep mgr separately allocated + kref (mgr may
> outlive fpriv), or should I restrict mgr lifetime to fpriv to allow embedding?
Well that is something you need to figure out :)
The rational for the longer lifetime of the fence_drv is that when you share
fences their lifetime is determined by a reference count. And those fences then
reference the fence driver.
But for the eventfd manager the eventfds don't reference the eventfd manager.
So it should be sufficient that on cleanup you deregister all eventfds, then
eventually wait for all ongoing interrupts and that's it.
A reference count and with that not embedding the eventfd_mgr into fpriv would
only be necessary if the lifetime of eventfd_mgr is defined by anything else
than the lifetime of fpriv.
Regards,
Christian.
...
>>> +
>>> + spin_lock(&id->lock);
>>> + {
>>> + struct amdgpu_eventfd_entry *it;
>>> +
>>> + hlist_for_each_entry(it, &id->entries, hnode) {
>>> + if (it->eventfd == eventfd) {
>>
>> Absolutely clear NAK to stuff like that! You must compare the ctx instead.
>>
>> Apart from that is it problematic to bind the same fd to and event multiple
>> times?
>
> If the same (event_id, ctx) is bound again, should bind be a no-op returning
> 0, or should it return -EEXIST?
>
> I
>> mean it doesn't make to much sense but it also doesn't hurt us in any way.
>
> Ack. I’ll stop storing and comparing the eventfd integer and instead use
> eventfd_ctx *ctx as the binding identity.
> For duplicate binds of the same (event_id, ctx), I plan to make bind
> idempotent (ignore duplicates and keep a single entry).
> For unbind, I’ll resolve the fd to ctx and remove the matching (event_id,
> ctx) entry.
> May I kno please, if this correct?
That sounds like something we can do, yes. But you need to document that
behavior in the uAPI header.
>
>
> And other questions please:
>
> 1. Signal count API:
>
> My tree seems to have eventfd_signal(ctx) only (no count arg).
>
> Question: For older kernels, is it acceptable to loop signal count times, or
> should we treat count as 1?
I don't see an use case for count != 1. What exactly is the background here?
>
> 2. IRQ lifetime:
>
> In IRQ paths where I copy mgr pointer, I’ll take a kref get/put around the
> signal to avoid raw pointer lifetime issues.
Well questionable idea, this means that the mgr can also be destroyed from IRQ
context.
>
> Question: OK with doing amdgpu_eventfd_mgr_get/put() in IRQ before/after
> signaling?
It would be better to just hold the appropriate lock while you work with the
manager and not have reference counting, but as I wrote above that is up to you.
> 3. One clarification regarding lifetime:
> In the current design the eventfd manager and subscriptions are tied to the
> drm_file (fpriv), and longer-lived producers like the USERQ fence driver hold
> a kref to the manager if needed.
> Does that match the expected model, or would you prefer moving the
> subscriptions under VM lifetime instead?
I think the lifetime should be tied to the fpriv and not use get/put reference.
Especially the fence_drv should not signal eventfds, but rather just the pure
interrupt handler.
Regards,
Christian.
>
> Thanks in advance!
>
> Best regards,
> Srini
>
>>
>> I'm running out of time to further review the patches, but I think that
>> already gives
>> you quite a bit of todo.
>>
>> Regards,
>> Christian.
>>
