drm/amdgpu: limit DMA size to PAGE_SIZE for scatter-gather buffers

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Signed-off-by Sinan Kaya <[email protected]>

Quoted 4 lines

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

Quoted 5 lines

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Signed-off-by Sinan Kaya <[email protected]>

Sinan, I guess Christian's suggestion is to add amdgpu_device_init function like here:

8<-------

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +2 -0

index 0e798b3..9b96771 100644
@@ -2339,6 +2339,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
         /* init the mode config */
         drm_mode_config_init(adev->ddev);
 
+       dma_set_max_seg_size(adev->dev, PAGE_SIZE);
+
         r = amdgpu_device_ip_init(adev);
         if (r) {
                 /* failed in exclusive mode due to timeout */

8<-------

After that, we don't do it in each generation.

Thanks,
Ray
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Signed-off-by Sinan Kaya <[email protected]>

---
  drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 1 +
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 1 +
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 1 +
  4 files changed, 4 insertions(+), 1 deletion(-)

drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c +1 -1

index 8e28270..1b031eb 100644
@@ -851,7 +851,7 @@ static int gmc_v6_0_sw_init(void *handle)
  		pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
  		dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
  	}
-
+	dma_set_max_seg_size(adev->dev, PAGE_SIZE);
  	r = gmc_v6_0_init_microcode(adev);
  	if (r) {
  		dev_err(adev->dev, "Failed to load mc firmware!\n");

drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c +1 -0

index 86e9d682..0a4b2cc1 100644
@@ -999,6 +999,7 @@ static int gmc_v7_0_sw_init(void *handle)
  		pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
  		pr_warn("amdgpu: No coherent DMA available\n");
  	}
+	dma_set_max_seg_size(adev->dev, PAGE_SIZE);
 
  	r = gmc_v7_0_init_microcode(adev);
  	if (r) {

drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c +1 -0

index 9a813d8..b171529 100644
@@ -1096,6 +1096,7 @@ static int gmc_v8_0_sw_init(void *handle)
  		pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
  		pr_warn("amdgpu: No coherent DMA available\n");
  	}
+	dma_set_max_seg_size(adev->dev, PAGE_SIZE);
 
  	r = gmc_v8_0_init_microcode(adev);
  	if (r) {

drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +0 -0

index 3b7e7af..36e658ab 100644

Quoted 3 lines · Sinan Kaya

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

So why not fix said code? It's clearly not a real hardware limitation, and the map_sg() APIs have potentially returned fewer than nents since forever, so there's really no excuse.

Quoted 4 lines

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Disagree; this is a dodgy hack, since you'll now end up passing scatterlists into dma_map_sg() which already violate max_seg_size to begin with, and I think a conscientious DMA API implementation would be at rights to fail the mapping for that reason (I know arm64 happens not to, but that was a deliberate design decision to make my life easier at the time).

As a short-term fix, at least do something like what i915 does and constrain the table allocation to the desired segment size as well, so things remain self-consistent. But still never claim that faking a hardware constraint as a workaround for a driver shortcoming is "the right thing to do" ;)

Robin.

Quoted 1 line

Signed-off-by Sinan Kaya <[email protected]>

Quoted 9 lines · Robin Murphy

Quoted 4 lines · Sinan Kaya

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

So why not fix said code? It's clearly not a real hardware limitation, and the map_sg() APIs have potentially returned fewer than nents since forever, so there's really no excuse.

Sure, I'll take a better fix if there is one.

Quoted 12 lines

Quoted 4 lines

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Disagree; this is a dodgy hack, since you'll now end up passing scatterlists into dma_map_sg() which already violate max_seg_size to begin with, and I think a conscientious DMA API implementation would be at rights to fail the mapping for that reason (I know arm64 happens not to, but that was a deliberate design decision to make my life easier at the time).

Quoted 5 lines

As a short-term fix, at least do something like what i915 does and constrain the table allocation to the desired segment size as well, so things remain self-consistent. But still never claim that faking a hardware constraint as a workaround for a driver shortcoming is "the right thing to do" ;)

You are asking for something like this from here, right?

https://elixir.bootlin.com/linux/v4.16.1/source/drivers/gpu/drm/i915/i915_gem_dmabuf.c#L58

Quoted 3 lines · Sinan Kaya

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

So why not fix said code? It's clearly not a real hardware limitation, and the map_sg() APIs have potentially returned fewer than nents since forever, so there's really no excuse.

Quoted 3 lines

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Disagree; this is a dodgy hack, since you'll now end up passing scatterlists into dma_map_sg() which already violate max_seg_size to begin with, and I think a conscientious DMA API implementation would be at rights to fail the mapping for that reason (I know arm64 happens not to, but that was a deliberate design decision to make my life easier at the time).

Quoted 7 lines · Sinan Kaya

Quoted 3 lines

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

So why not fix said code? It's clearly not a real hardware limitation, and the map_sg() APIs have potentially returned fewer than nents since forever, so there's really no excuse.

Yes, relying on dma_map_sg returning the same number of entries as passed it is completely bogus.

Quoted 9 lines

Quoted 4 lines

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Disagree; this is a dodgy hack, since you'll now end up passing scatterlists into dma_map_sg() which already violate max_seg_size to begin with, and I think a conscientious DMA API implementation would be at rights to fail the mapping for that reason (I know arm64 happens not to, but that was a deliberate design decision to make my life easier at the time).

Agreed.

Am 12.04.2018 um 08:26 schrieb Christoph Hellwig:

Quoted 10 lines · Robin Murphy

Quoted 7 lines · Sinan Kaya

Quoted 3 lines

Code is expecing to observe the same number of buffers returned from dma_map_sg() function compared to sg_alloc_table_from_pages(). This doesn't hold true universally especially for systems with IOMMU.

So why not fix said code? It's clearly not a real hardware limitation, and the map_sg() APIs have potentially returned fewer than nents since forever, so there's really no excuse.

Yes, relying on dma_map_sg returning the same number of entries as passed it is completely bogus.

I agree that the common DRM functions should be able to deal with both, but we should let the driver side decide if it wants multiple addresses combined or not.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

IOMMU driver tries to combine buffers into a single DMA address as much as it can. The right thing is to tell the DMA layer how much combining IOMMU can do.

Quoted 6 lines

Quoted 5 lines

Disagree; this is a dodgy hack, since you'll now end up passing scatterlists into dma_map_sg() which already violate max_seg_size to begin with, and I think a conscientious DMA API implementation would be at rights to fail the mapping for that reason (I know arm64 happens not to, but that was a deliberate design decision to make my life easier at the time).

Agreed.

Sounds like my understanding of max_seg_size is incorrect, what exactly should that describe?