| Message ID | 1f7c304a79b8b8dd5d4716786cae7502a0cc31f5.1649684782.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/tegra: Stop using iommu_present() | expand |
On 4/11/22 16:46, Robin Murphy wrote: > Refactor the confusing logic to make it both clearer and more robust. If > the host1x parent device does have an IOMMU domain then iommu_present() > is redundantly true, while otherwise for the 32-bit DMA mask case it > still doesn't say whether the IOMMU driver actually knows about the DRM > device or not. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > v2: Fix logic for older SoCs and clarify. > > drivers/gpu/drm/tegra/drm.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index 9464f522e257..4f2bdab31064 100644 > --- a/drivers/gpu/drm/tegra/drm.c > +++ b/drivers/gpu/drm/tegra/drm.c > @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) > struct host1x *host1x = dev_get_drvdata(dev->dev.parent); > struct iommu_domain *domain; > > + /* For starters, this is moot if no IOMMU is available */ > + if (!device_iommu_mapped(&dev->dev)) > + return false; > + > + /* > + * Tegra20 and Tegra30 don't support addressing memory beyond the > + * 32-bit boundary, so the regular GATHER opcodes will always be > + * sufficient and whether or not the host1x is attached to an IOMMU > + * doesn't matter. > + */ > + if (host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32)) > + return true; > + > /* > * If the Tegra DRM clients are backed by an IOMMU, push buffers are > * likely to be allocated beyond the 32-bit boundary if sufficient > @@ -1122,14 +1135,13 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) > domain = iommu_get_domain_for_dev(dev->dev.parent); > > /* > - * Tegra20 and Tegra30 don't support addressing memory beyond the > - * 32-bit boundary, so the regular GATHER opcodes will always be > - * sufficient and whether or not the host1x is attached to an IOMMU > - * doesn't matter. > + * At the moment, the exact type of domain doesn't actually matter. > + * Only for 64-bit kernels might this be a managed DMA API domain, and > + * then only on newer SoCs using arm-smmu, since tegra-smmu doesn't > + * support default domains at all, and since those SoCs are the same > + * ones with extended GATHER support, even if it's a passthrough domain > + * it can still work out OK. > */ > - if (!domain && host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32)) > - return true; > - > return domain != NULL; > } > > @@ -1149,7 +1161,7 @@ static int host1x_drm_probe(struct host1x_device *dev) > goto put; > } > > - if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) { > + if (host1x_drm_wants_iommu(dev)) { > tegra->domain = iommu_domain_alloc(&platform_bus_type); > if (!tegra->domain) { > err = -ENOMEM; Robin, thank you for the updated version. The patch looks okay to me. Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> A bit later I'll also will give it a test, just to be sure because we had problems with that function in the past.
On 4/11/22 16:46, Robin Murphy wrote: > @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) > struct host1x *host1x = dev_get_drvdata(dev->dev.parent); > struct iommu_domain *domain; > > + /* For starters, this is moot if no IOMMU is available */ > + if (!device_iommu_mapped(&dev->dev)) > + return false; Unfortunately this returns false on T30 with enabled IOMMU because we don't use IOMMU for Host1x on T30 [1] to optimize performance. We can't change it until we will update drivers to support Host1x-dedicated buffers. [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/host1x/dev.c#L258
On 2022-05-04 01:52, Dmitry Osipenko wrote: > On 4/11/22 16:46, Robin Murphy wrote: >> @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) >> struct host1x *host1x = dev_get_drvdata(dev->dev.parent); >> struct iommu_domain *domain; >> >> + /* For starters, this is moot if no IOMMU is available */ >> + if (!device_iommu_mapped(&dev->dev)) >> + return false; > > Unfortunately this returns false on T30 with enabled IOMMU because we > don't use IOMMU for Host1x on T30 [1] to optimize performance. We can't > change it until we will update drivers to support Host1x-dedicated buffers. Huh, so is dev->dev here not the DRM device? If it is, and device_iommu_mapped() returns false, then the later iommu_attach_group() call is going to fail anyway, so there's not much point allocating a domain. If it's not, then what the heck is host1x_drm_wants_iommu() actually testing for? In the not-too-distant future we'll need to pass an appropriate IOMMU client device to iommu_domain_alloc() as well, so the sooner we can get this code straight the better. Thanks, Robin. > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/host1x/dev.c#L258 >
On 5/4/22 14:52, Robin Murphy wrote: > On 2022-05-04 01:52, Dmitry Osipenko wrote: >> On 4/11/22 16:46, Robin Murphy wrote: >>> @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct >>> host1x_device *dev) >>> struct host1x *host1x = dev_get_drvdata(dev->dev.parent); >>> struct iommu_domain *domain; >>> + /* For starters, this is moot if no IOMMU is available */ >>> + if (!device_iommu_mapped(&dev->dev)) >>> + return false; >> >> Unfortunately this returns false on T30 with enabled IOMMU because we >> don't use IOMMU for Host1x on T30 [1] to optimize performance. We can't >> change it until we will update drivers to support Host1x-dedicated >> buffers. > > Huh, so is dev->dev here not the DRM device? If it is, and > device_iommu_mapped() returns false, then the later iommu_attach_group() > call is going to fail anyway, so there's not much point allocating a > domain. If it's not, then what the heck is host1x_drm_wants_iommu() > actually testing for? The dev->dev is the host1x device and it's the DRM device. The iommu_attach_group() is called for the DRM sub-devices (clients in the Tegra driver), which are the devices sitting on the host1x bus. There is no single GPU device on Tegra, instead it's composed of independent GPU engines and display controllers that are connected to the host1x bus. Host1x also has channel DMA engines that are used by DRM driver. We don't have dedicated devices for the host1x DMA, there is single host1x driver that manages host1x bus and DMA.
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 9464f522e257..4f2bdab31064 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) struct host1x *host1x = dev_get_drvdata(dev->dev.parent); struct iommu_domain *domain; + /* For starters, this is moot if no IOMMU is available */ + if (!device_iommu_mapped(&dev->dev)) + return false; + + /* + * Tegra20 and Tegra30 don't support addressing memory beyond the + * 32-bit boundary, so the regular GATHER opcodes will always be + * sufficient and whether or not the host1x is attached to an IOMMU + * doesn't matter. + */ + if (host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32)) + return true; + /* * If the Tegra DRM clients are backed by an IOMMU, push buffers are * likely to be allocated beyond the 32-bit boundary if sufficient @@ -1122,14 +1135,13 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) domain = iommu_get_domain_for_dev(dev->dev.parent); /* - * Tegra20 and Tegra30 don't support addressing memory beyond the - * 32-bit boundary, so the regular GATHER opcodes will always be - * sufficient and whether or not the host1x is attached to an IOMMU - * doesn't matter. + * At the moment, the exact type of domain doesn't actually matter. + * Only for 64-bit kernels might this be a managed DMA API domain, and + * then only on newer SoCs using arm-smmu, since tegra-smmu doesn't + * support default domains at all, and since those SoCs are the same + * ones with extended GATHER support, even if it's a passthrough domain + * it can still work out OK. */ - if (!domain && host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32)) - return true; - return domain != NULL; } @@ -1149,7 +1161,7 @@ static int host1x_drm_probe(struct host1x_device *dev) goto put; } - if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) { + if (host1x_drm_wants_iommu(dev)) { tegra->domain = iommu_domain_alloc(&platform_bus_type); if (!tegra->domain) { err = -ENOMEM;
Refactor the confusing logic to make it both clearer and more robust. If the host1x parent device does have an IOMMU domain then iommu_present() is redundantly true, while otherwise for the 32-bit DMA mask case it still doesn't say whether the IOMMU driver actually knows about the DRM device or not. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- v2: Fix logic for older SoCs and clarify. drivers/gpu/drm/tegra/drm.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)