Re: [PATCH v5 1/3] drm/amdgpu: Allow PASID allocator to store fpriv owner

On 7/8/26 17:27, Srinivasan Shanmugam wrote:
> AMDGPU already has a global PASID xarray used for PASID allocation.
> 
> Allow amdgpu_pasid_alloc() to optionally store the owning DRM
> file-private object directly.
> 
> Initial callers pass NULL. 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
> 
> Clear the stored owner from amdgpu_pasid_free_delayed() before waiting
> for outstanding fences so PASID lookups cannot observe a stale fpriv
> while the PASID itself is pending delayed release.
> 
> v5:
> - Store NULL instead of xa_mk_value(0) for ownerless PASIDs.
> - Simplify owner clearing by unconditionally storing NULL.
> 
> v4:
> - 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]>

Reviewed-by: Christian König <[email protected]>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 69 +++++++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  6 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
>  3 files changed, 71 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 684f40fce73f..d41601aab3c7 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,7 +61,7 @@ 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;
> @@ -68,9 +69,9 @@ int amdgpu_pasid_alloc(unsigned int bits)
>  	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);
> +	r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, fpriv,
> +				XA_LIMIT(1, (1U << bits) - 1),
> +				&amdgpu_pasid_xa_next, GFP_KERNEL);
>  	if (r < 0)
>  		return r;
>  
> @@ -106,6 +107,64 @@ static void amdgpu_pasid_free_cb(struct dma_fence *fence,
>  	kfree(cb);
>  }
>  
> +/**
> + * amdgpu_pasid_clear_owner - Clear the owner associated with a PASID
> + * @pasid: PASID whose owner should be cleared
> + *
> + * Replace the stored owner with NULL while keeping the PASID 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;
> +
> +	if (!pasid)
> +		return;
> +
> +	xa_lock_irqsave(&amdgpu_pasid_xa, flags);
> +	__xa_store(&amdgpu_pasid_xa, pasid, NULL, 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)
> +{
> +	return xa_load(&amdgpu_pasid_xa, pasid);
> +}
> +
>  /**
>   * amdgpu_pasid_free_delayed - free pasid when fences signal
>   *
> @@ -121,6 +180,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..4b55d0d9703a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> @@ -34,6 +34,7 @@
>  #define AMDGPU_NUM_VMID	16
>  
>  struct amdgpu_device;
> +struct amdgpu_fpriv;
>  struct amdgpu_vm;
>  struct amdgpu_ring;
>  struct amdgpu_sync;
> @@ -70,7 +71,10 @@ 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_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(u32 pasid);
>  void amdgpu_pasid_free_delayed(struct dma_resv *resv,
>  			       u32 pasid);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 6b4de2a6c629..0c973a3c73fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1488,7 +1488,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;