On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> AMDGPU already has a global PASID xarray used by the PASID allocator.
>
> Allow amdgpu_pasid_alloc() to optionally store the owning DRM
> file-private object directly.
>
> Initial callers pass NULL and keep the current dummy allocation marker
> behavior. A later patch in this series passes the DRM file-private
> object for DRM PASIDs.
>
> This prepares for using:
>
> PASID -> fpriv -> VM
>
> instead of:
>
> PASID -> VM
>
> Also clear any stored owner from amdgpu_pasid_free_delayed() before
> waiting for fences, so PASID lookups cannot observe a stale fpriv while
> the PASID number itself is still pending delayed release.
>
> v4: (per Christian)
> - Add fpriv as an optional parameter to amdgpu_pasid_alloc().
> - Drop separate amdgpu_pasid_set_fpriv()/clear_fpriv() helpers.
> - Clear PASID owner from amdgpu_pasid_free_delayed().
>
> Cc: Alex Deucher <[email protected]>
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Srinivasan Shanmugam <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 82 +++++++++++++++++++++++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h | 6 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/mes_v12_1.c | 2 +-
> 4 files changed, 85 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 684f40fce73f..669d0fff8cbc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -52,6 +52,7 @@ struct amdgpu_pasid_cb {
> /**
> * amdgpu_pasid_alloc - Allocate a PASID
> * @bits: Maximum width of the PASID in bits, must be at least 1
> + * @fpriv: optional DRM file-private owner
> *
> * Uses kernel's IDR cyclic allocator (same as PID allocation).
> * Allocates sequentially with automatic wrap-around.
> @@ -60,17 +61,19 @@ struct amdgpu_pasid_cb {
> * Returns %-ENOSPC if no PASID was available. Returns %-ENOMEM on
> * memory allocation failure.
> */
> -int amdgpu_pasid_alloc(unsigned int bits)
> +int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv)
> {
> u32 pasid;
> int r;
> + void *entry;
>
> if (bits == 0)
> return -EINVAL;
>
> - r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, xa_mk_value(0),
> - XA_LIMIT(1, (1U << bits) - 1),
> - &amdgpu_pasid_xa_next, GFP_KERNEL);
> + entry = fpriv ? fpriv : xa_mk_value(0);
That's problematic I think. The xa_mk_value(0) value is not NULL and needs to
be filtered out when somebody looks the fpriv up using the array.
But xa_insert() can handle NULL entries, but I'm not sure if
xa_alloc_cyclic_irq() can do that as well.
> + r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, entry,
> + XA_LIMIT(1, (1U << bits) - 1),
> + &amdgpu_pasid_xa_next, GFP_KERNEL);
> if (r < 0)
> return r;
>
> @@ -78,6 +81,75 @@ int amdgpu_pasid_alloc(unsigned int bits)
> return pasid;
> }
>
> +/**
> + * amdgpu_pasid_clear_owner - Remove the owner associated with a PASID
> + * @pasid: PASID whose owner should be cleared
> + *
> + * Restore a PASID entry back to the allocation marker while keeping the
> + * PASID itself allocated.
> + *
> + * This is used by the delayed PASID free path so that future PASID
> + * lookups cannot resolve a stale DRM file-private object while the PASID
> + * is still waiting for outstanding fences before being released.
> + */
> +static void amdgpu_pasid_clear_owner(u32 pasid)
> +{
> + unsigned long flags;
> + void *entry;
> +
> + if (!pasid)
> + return;
> +
> + xa_lock_irqsave(&amdgpu_pasid_xa, flags);
> + entry = xa_load(&amdgpu_pasid_xa, pasid);
> + if (entry && !xa_is_value(entry))
I think you should just skip this test and use xa_store to overwrite the fpriv
value.
Alternative xa_cmpxchg() could be used to replace the value.
> + __xa_store(&amdgpu_pasid_xa, pasid, xa_mk_value(0),
> + GFP_ATOMIC);
> + xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
> +}
> +
> +/**
> + * amdgpu_pasid_lock - acquire the global PASID xarray lock
> + * @flags: storage for interrupt state
> + *
> + * Acquire the global PASID xarray lock with interrupts disabled.
> + * The saved interrupt state must be passed to
> + * amdgpu_pasid_unlock().
> + */
> +void amdgpu_pasid_lock(unsigned long *flags)
> +{
> + xa_lock_irqsave(&amdgpu_pasid_xa, *flags);
> +}
> +
> +/**
> + * amdgpu_pasid_unlock - release the global PASID xarray lock
> + * @flags: interrupt state returned by amdgpu_pasid_lock()
> + *
> + * Release the global PASID xarray lock and restore the previous
> + * interrupt state.
> + */
> +void amdgpu_pasid_unlock(unsigned long flags)
> +{
> + xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
> +}
> +
> +/**
> + * amdgpu_pasid_get_fpriv_locked - get fpriv from PASID
> + * @pasid: PASID to resolve
> + *
> + * Caller must hold the PASID XA lock.
> + */
> +struct amdgpu_fpriv *amdgpu_pasid_get_fpriv_locked(u32 pasid)
> +{
> + void *entry;
> +
> + entry = xa_load(&amdgpu_pasid_xa, pasid);
> + if (!entry || xa_is_value(entry))
> + return NULL;
> +
> + return entry;
> +}
> +
> /**
> * amdgpu_pasid_free - Free a PASID
> * @pasid: PASID to free
> @@ -121,6 +193,8 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
> struct dma_fence *fence;
> int r;
>
> + amdgpu_pasid_clear_owner(pasid);
> +
> r = dma_resv_get_singleton(resv, DMA_RESV_USAGE_BOOKKEEP, &fence);
> if (r)
> goto fallback;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> index a57919478d3b..c2be6f81d680 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> @@ -38,6 +38,7 @@ struct amdgpu_vm;
> struct amdgpu_ring;
> struct amdgpu_sync;
> struct amdgpu_job;
> +struct amdgpu_fpriv;
>
> struct amdgpu_vmid {
> struct list_head list;
> @@ -70,8 +71,11 @@ struct amdgpu_vmid_mgr {
> bool reserved_vmid;
> };
>
> -int amdgpu_pasid_alloc(unsigned int bits);
> +int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv);
> void amdgpu_pasid_free(u32 pasid);
> +void amdgpu_pasid_lock(unsigned long *flags);
> +void amdgpu_pasid_unlock(unsigned long flags);
> +struct amdgpu_fpriv *amdgpu_pasid_get_fpriv_locked(u32 pasid);
> void amdgpu_pasid_free_delayed(struct dma_resv *resv,
> u32 pasid);
> void amdgpu_pasid_mgr_cleanup(void);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index cacdc99b3ad6..4610d6889e9b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1487,7 +1487,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev,
> struct drm_file *file_priv)
> goto out_suspend;
> }
>
> - pasid = amdgpu_pasid_alloc(16);
> + pasid = amdgpu_pasid_alloc(16, NULL);
> if (pasid < 0) {
> dev_warn(adev->dev, "No more PASIDs available!");
> pasid = 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> index f7d5879c6e44..65b824144a2f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> @@ -2293,7 +2293,7 @@ static int mes_v12_1_self_test(struct amdgpu_device
> *adev, int xcc_id)
> u64 meta_gpu_addr, ctx_gpu_addr;
> int size, i, r, pasid;
>
> - pasid = amdgpu_pasid_alloc(16);
> + pasid = amdgpu_pasid_alloc(16, NULL);
> if (pasid < 0)
> pasid = 0;
>
Please prepare a patch to completely remove the self test for MES v12. We have
already removed the self test for other MES versions because the IGT tests now
take care of that.
Regards,
Christian.
