Re: [PATCH v5] drm/amdgpu: Take IOMMU remapping into account for p2p checks

[Felix Kuehling <felix.kuehling@amd.com>]

On 2024-08-16 3:29, Rahul Jain wrote:
> when trying to enable p2p the amdgpu_device_is_peer_accessible()
> checks the condition where address_mask overlaps the aper_base
> and hence returns 0, due to which the p2p disables for this platform
> 
> IOMMU should remap the BAR addresses so the device can access
> them. Hence check if peer_adev is remapping DMA
> 
> v5: (Felix, Alex)
> - fixing comment as per Alex feedback
> - refactor code as per Felix
> 
> v4: (Alex)
> - fix the comment and description
> 
> v3:
> - remove iommu_remap variable
> 
> v2: (Alex)
> - Fix as per review comments
> - add new function amdgpu_device_check_iommu_remap to check if iommu
>   remap
> 
> Signed-off-by: Rahul Jain <Rahul.Jain@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44 +++++++++++++++++-----
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a6b8d0ba4758..e03b3357ae09 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3952,6 +3952,25 @@ static void amdgpu_device_check_iommu_direct_map(struct amdgpu_device *adev)
>  		adev->ram_is_direct_mapped = true;
>  }
>  
> +/**
> + * amdgpu_device_check_iommu_remap - Check if DMA remapping is enabled.
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * return if IOMMU remapping bar address
> + */
> +static bool amdgpu_device_check_iommu_remap(struct amdgpu_device *adev)
> +{
> +	struct iommu_domain *domain;
> +
> +	domain = iommu_get_domain_for_dev(adev->dev);
> +	if (domain && (domain->type == IOMMU_DOMAIN_DMA ||
> +		domain->type ==	IOMMU_DOMAIN_DMA_FQ))
> +		return true;
> +
> +	return false;
> +}
> +
>  static const struct attribute *amdgpu_dev_attributes[] = {
>  	&dev_attr_pcie_replay_count.attr,
>  	NULL
> @@ -6127,21 +6146,26 @@ bool amdgpu_device_is_peer_accessible(struct amdgpu_device *adev,
>  				      struct amdgpu_device *peer_adev)
>  {
>  #ifdef CONFIG_HSA_AMD_P2P
> -	uint64_t address_mask = peer_adev->dev->dma_mask ?
> -		~*peer_adev->dev->dma_mask : ~((1ULL << 32) - 1);
> -	resource_size_t aper_limit =
> -		adev->gmc.aper_base + adev->gmc.aper_size - 1;
>  	bool p2p_access =
>  		!adev->gmc.xgmi.connected_to_cpu &&
>  		!(pci_p2pdma_distance(adev->pdev, peer_adev->dev, false) < 0);
>  
> -	return pcie_p2p && p2p_access && (adev->gmc.visible_vram_size &&
> -		adev->gmc.real_vram_size == adev->gmc.visible_vram_size &&
> -		!(adev->gmc.aper_base & address_mask ||
> -		  aper_limit & address_mask));
> -#else
> -	return false;
> +	bool is_large_bar = adev->gmc.visible_vram_size &&
> +		adev->gmc.real_vram_size == adev->gmc.visible_vram_size;
> +	bool p2p_addressable = amdgpu_device_check_iommu_remap(peer_adev);
> +
> +	if (!p2p_addressable) {
> +		uint64_t address_mask = peer_adev->dev->dma_mask ?
> +			~*peer_adev->dev->dma_mask : ~((1ULL << 32) - 1);
> +		resource_size_t aper_limit =
> +			adev->gmc.aper_base + adev->gmc.aper_size - 1;
> +
> +		p2p_addressable = !(adev->gmc.aper_base & address_mask ||
> +				     aper_limit & address_mask);
> +	}
> +	return is_large_bar && p2p_access && p2p_addressable;
>  #endif
> +	return false;

You changed the #else into a #endif. Logically that's OK, but it may cause a compiler warning about unreachable code because the program can never reach the "return false;" statement when CONFIG_HSA_AMD_P2P is enabled. It's probably safer to leave the #else to make sure this compiles without warnings on current and future compilers.

With that fixed, this patch is
Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>

>  }
>  
>  int amdgpu_device_baco_enter(struct drm_device *dev)