Re: [PATCH v2] drm/tegra: Stop using iommu_present()
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)) >>> + 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. -- Best regards, Dmitry ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
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)) + 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
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)) > + 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 -- Best regards, Dmitry ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
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
> ---
>
> 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))
> + 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(_bus_type)) {
> + if (host1x_drm_wants_iommu(dev)) {
> tegra->domain = iommu_domain_alloc(_bus_type);
> if (!tegra->domain) {
> err = -ENOMEM;
Robin, thank you for the updated version. The patch looks okay to me.
Reviewed-by: Dmitry Osipenko
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] drm/tegra: Stop using iommu_present()
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
---
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))
+ 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(_bus_type)) {
+ if (host1x_drm_wants_iommu(dev)) {
tegra->domain = iommu_domain_alloc(_bus_type);
if (!tegra->domain) {
err = -ENOMEM;
--
2.28.0.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
