| Message ID | 20191113095144.2981-2-thomas_os@shipmail.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vmwgfx: Use coherent memory if there are dma mapping size restrictions | expand |
Hi, On 11/13/19 3:16 PM, Christoph Hellwig wrote: > On Wed, Nov 13, 2019 at 10:51:43AM +0100, Thomas Hellström (VMware) wrote: >> From: Thomas Hellstrom <thellstrom@vmware.com> >> >> We're gradually moving towards using DMA coherent memory in most >> situations, although TTM interactions with the DMA layers is still a >> work-in-progress. Meanwhile, use coherent memory when there are size >> restrictions meaning that there is a chance that streaming dma mapping >> of large buffer objects may fail. >> Also move DMA mask settings to the vmw_dma_select_mode function, since >> it's important that we set the correct DMA masks before calling the >> dma_max_mapping_size() function. >> >> Cc: Christoph Hellwig <hch@infradead.org> >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> >> Reviewed-by: Brian Paul <brianp@vmware.com> >> --- >> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 31 +++++++---------------------- >> 1 file changed, 7 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c >> index fc0283659c41..1e1de83908fe 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c >> @@ -569,7 +569,10 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv) >> [vmw_dma_map_populate] = "Caching DMA mappings.", >> [vmw_dma_map_bind] = "Giving up DMA mappings early."}; >> >> - if (vmw_force_coherent) >> + (void) dma_set_mask_and_coherent(dev_priv->dev->dev, DMA_BIT_MASK(64)); > Please don't use void cast on returns. Also this genercally can't > fail, so if it fails anyway it is fatal, and you should actually > return an error. OK. Will fix. > >> + if (vmw_force_coherent || >> + dma_max_mapping_size(dev_priv->dev->dev) != SIZE_MAX) > I don't think this is the right check to see if swiotlb would be > used. You probably want to call dma_addressing_limited(). Please > also add a comment explaining the call here. If I understand things correctly, dma_addressing_limited() would always return false on vmwgfx, at least if the "restrict to 44 bit" option is removed. The "odd" modes we want to catch is someone setting swiotlb=force, or someone enabling SEV. In both cases, vmwgfx would break down due to limited DMA space even if streaming DMA was fixed with appropriate sync calls. The same holds for vmw_pvscsi (which we discussed before) where the large queue depth will typically fill the SWIOTLB causing excessive non-fatal error logging. dma_max_mapping_size() currently catch these modes, but best I guess would be dma_swiotlb_forced() function that could be used in cases like this? > > >> if (dev_priv->map_mode != vmw_dma_phys && > AFAIK vmw_dma_phys can't even be set in current mainline and is dead > code. Can you check if I'm missing something? Certainly all three > branches above don't set it. I'll remove that dead code for v2. > >> (sizeof(unsigned long) == 4 || vmw_restrict_dma_mask)) { >> DRM_INFO("Restricting DMA addresses to 44 bits.\n"); >> - return dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(44)); >> + return dma_set_mask_and_coherent(dev_priv->dev->dev, >> + DMA_BIT_MASK(44)); > Can you keep setting the DMA mask together? > > > E.g. make the whole thing something like: > > static int vmw_dma_select_mode(struct vmw_private *dev_priv) > { > if (dma_addressing_limited(dev_priv->dev->dev) || vmw_force_coherent) { > /* > * XXX: what about AMD iommu? virtio-iommu? Also > * swiotlb should be always available where we can > * address more than 32-bit of memory.. > */ > if (!IS_ENABLED(CONFIG_SWIOTLB) && > !IS_ENABLED(CONFIG_INTEL_IOMMU)) > return -EINVAL; The above check is only to see if coherent memory functionality is really compiled in into TTM. We have a patchset that got lost in the last merge window to fix this properly and avoid confusion about this. I'll rebase on that. > dev_priv->map_mode = vmw_dma_alloc_coherent; > } else if (vmw_restrict_iommu) { > dev_priv->map_mode = vmw_dma_map_bind; > } else { > dev_priv->map_mode = vmw_dma_map_populate; > } > > /* > * On 32-bit systems we can only handle 32 bit PFNs. Optionally set > * that restriction also for 64-bit systems. > */ > return dma_set_mask_and_coherent(dev->dev, > (IS_ENABLED(CONFIG_64BIT) && !vmw_restrict_dma_mask) ? > 64 : 44); > } Thanks, Thomas
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index fc0283659c41..1e1de83908fe 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -569,7 +569,10 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv) [vmw_dma_map_populate] = "Caching DMA mappings.", [vmw_dma_map_bind] = "Giving up DMA mappings early."}; - if (vmw_force_coherent) + (void) dma_set_mask_and_coherent(dev_priv->dev->dev, DMA_BIT_MASK(64)); + + if (vmw_force_coherent || + dma_max_mapping_size(dev_priv->dev->dev) != SIZE_MAX) dev_priv->map_mode = vmw_dma_alloc_coherent; else if (vmw_restrict_iommu) dev_priv->map_mode = vmw_dma_map_bind; @@ -582,30 +585,15 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv) return -EINVAL; DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]); - return 0; -} -/** - * vmw_dma_masks - set required page- and dma masks - * - * @dev: Pointer to struct drm-device - * - * With 32-bit we can only handle 32 bit PFNs. Optionally set that - * restriction also for 64-bit systems. - */ -static int vmw_dma_masks(struct vmw_private *dev_priv) -{ - struct drm_device *dev = dev_priv->dev; - int ret = 0; - - ret = dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(64)); if (dev_priv->map_mode != vmw_dma_phys && (sizeof(unsigned long) == 4 || vmw_restrict_dma_mask)) { DRM_INFO("Restricting DMA addresses to 44 bits.\n"); - return dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(44)); + return dma_set_mask_and_coherent(dev_priv->dev->dev, + DMA_BIT_MASK(44)); } - return ret; + return 0; } static int vmw_driver_load(struct drm_device *dev, unsigned long chipset) @@ -674,7 +662,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset) dev_priv->capabilities2 = vmw_read(dev_priv, SVGA_REG_CAP2); } - ret = vmw_dma_select_mode(dev_priv); if (unlikely(ret != 0)) { DRM_INFO("Restricting capabilities due to IOMMU setup.\n"); @@ -746,10 +733,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset) if (dev_priv->capabilities & SVGA_CAP_CAP2_REGISTER) vmw_print_capabilities2(dev_priv->capabilities2); - ret = vmw_dma_masks(dev_priv); - if (unlikely(ret != 0)) - goto out_err0; - dma_set_max_seg_size(dev->dev, min_t(unsigned int, U32_MAX & PAGE_MASK, SCATTERLIST_MAX_SEGMENT));