* [PATCH 0/3] drm/etnaviv: IOMMU related fixes @ 2021-08-26 12:10 Michael Walle 2021-08-26 12:10 ` [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE Michael Walle ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:10 UTC (permalink / raw) To: etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter, Michael Walle This patch series fixes usage of the etnaviv driver with GPUs behind a IOMMU. It was tested on a NXP LS1028A SoC. Together with Lucas' MMU patches [1] there are not more (GPU internal) MMU nor (system) IOMMU faults on the LS1028A. [1] https://lists.freedesktop.org/archives/etnaviv/2021-August/003682.html Michael Walle (3): drm/etnaviv: use PLATFORM_DEVID_NONE drm/etnaviv: fix dma configuration of the virtual device drm/etnaviv: use a 32 bit mask as coherent DMA mask drivers/gpu/drm/etnaviv/etnaviv_drv.c | 41 ++++++++++++++++++++------- 1 file changed, 31 insertions(+), 10 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE 2021-08-26 12:10 [PATCH 0/3] drm/etnaviv: IOMMU related fixes Michael Walle @ 2021-08-26 12:10 ` Michael Walle 2021-08-26 12:59 ` Christian Gmeiner 2021-08-26 12:10 ` [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device Michael Walle 2021-08-26 12:10 ` [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask Michael Walle 2 siblings, 1 reply; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:10 UTC (permalink / raw) To: etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter, Michael Walle There is already a macro for the magic value. Use it. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 7dcc6392792d..2509b3e85709 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -653,7 +653,7 @@ static int __init etnaviv_init(void) if (!of_device_is_available(np)) continue; - pdev = platform_device_alloc("etnaviv", -1); + pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE); if (!pdev) { ret = -ENOMEM; of_node_put(np); -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE 2021-08-26 12:10 ` [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE Michael Walle @ 2021-08-26 12:59 ` Christian Gmeiner 0 siblings, 0 replies; 13+ messages in thread From: Christian Gmeiner @ 2021-08-26 12:59 UTC (permalink / raw) To: Michael Walle Cc: The etnaviv authors, DRI mailing list, LKML, Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, David Airlie, Daniel Vetter Am Do., 26. Aug. 2021 um 14:10 Uhr schrieb Michael Walle <michael@walle.cc>: > > There is already a macro for the magic value. Use it. > > Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com> I will wait for v2 for the rest of the changes to review. > --- > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index 7dcc6392792d..2509b3e85709 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > @@ -653,7 +653,7 @@ static int __init etnaviv_init(void) > if (!of_device_is_available(np)) > continue; > > - pdev = platform_device_alloc("etnaviv", -1); > + pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE); > if (!pdev) { > ret = -ENOMEM; > of_node_put(np); > -- > 2.30.2 > -- greets -- Christian Gmeiner, MSc https://christian-gmeiner.info/privacypolicy ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device 2021-08-26 12:10 [PATCH 0/3] drm/etnaviv: IOMMU related fixes Michael Walle 2021-08-26 12:10 ` [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE Michael Walle @ 2021-08-26 12:10 ` Michael Walle 2021-08-26 12:14 ` Russell King (Oracle) 2021-08-26 15:00 ` Robin Murphy 2021-08-26 12:10 ` [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask Michael Walle 2 siblings, 2 replies; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:10 UTC (permalink / raw) To: etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter, Michael Walle The DMA configuration of the virtual device is inherited from the first actual etnaviv device. Unfortunately, this doesn't work with an IOMMU: [ 5.191008] Failed to set up IOMMU for device (null); retaining platform DMA ops This is because there is no associated iommu_group with the device. The group is set in iommu_group_add_device() which is eventually called by device_add() via the platform bus: device_add() blocking_notifier_call_chain() iommu_bus_notifier() iommu_probe_device() __iommu_probe_device() iommu_group_get_for_dev() iommu_group_add_device() Move of_dma_configure() into the probe function, which is called after device_add(). Normally, the platform code will already call it itself if .of_node is set. Unfortunately, this isn't the case here. Also move the dma mask assignemnts to probe() to keep all DMA related settings together. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 2509b3e85709..ff6425f6ebad 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -589,6 +589,7 @@ static int compare_str(struct device *dev, void *data) static int etnaviv_pdev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct device_node *first_node = NULL; struct component_match *match = NULL; if (!dev->platform_data) { @@ -598,6 +599,9 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) if (!of_device_is_available(core_node)) continue; + if (!first_node) + first_node = core_node; + drm_of_component_match_add(&pdev->dev, &match, compare_of, core_node); } @@ -609,6 +613,17 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) component_match_add(dev, &match, compare_str, names[i]); } + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; + + /* + * Apply the same DMA configuration to the virtual etnaviv + * device as the GPU we found. This assumes that all Vivante + * GPUs in the system share the same DMA constraints. + */ + if (first_node) + of_dma_configure(&pdev->dev, first_node, true); + return component_master_add_with_match(dev, &etnaviv_master_ops, match); } @@ -659,15 +674,6 @@ static int __init etnaviv_init(void) of_node_put(np); goto unregister_platform_driver; } - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; - - /* - * Apply the same DMA configuration to the virtual etnaviv - * device as the GPU we found. This assumes that all Vivante - * GPUs in the system share the same DMA constraints. - */ - of_dma_configure(&pdev->dev, np, true); ret = platform_device_add(pdev); if (ret) { -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device 2021-08-26 12:10 ` [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device Michael Walle @ 2021-08-26 12:14 ` Russell King (Oracle) 2021-08-26 12:17 ` Michael Walle 2021-08-26 15:00 ` Robin Murphy 1 sibling, 1 reply; 13+ messages in thread From: Russell King (Oracle) @ 2021-08-26 12:14 UTC (permalink / raw) To: Michael Walle Cc: etnaviv, dri-devel, linux-kernel, Lukas F . Hartmann, Marek Vasut, Lucas Stach, Christian Gmeiner, David Airlie, Daniel Vetter On Thu, Aug 26, 2021 at 02:10:05PM +0200, Michael Walle wrote: > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; Please use dma_coerce_mask_and_coherent() here instead. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device 2021-08-26 12:14 ` Russell King (Oracle) @ 2021-08-26 12:17 ` Michael Walle 0 siblings, 0 replies; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:17 UTC (permalink / raw) To: Russell King (Oracle) Cc: etnaviv, dri-devel, linux-kernel, Lukas F . Hartmann, Marek Vasut, Lucas Stach, Christian Gmeiner, David Airlie, Daniel Vetter Am 2021-08-26 14:14, schrieb Russell King (Oracle): > On Thu, Aug 26, 2021 at 02:10:05PM +0200, Michael Walle wrote: >> + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); >> + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > Please use dma_coerce_mask_and_coherent() here instead. It will be removed altogether in patch 3/3. I just moved the current code around here. I could drop the code moving in 2/3. I wasn't sure whats best. -michael ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device 2021-08-26 12:10 ` [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device Michael Walle 2021-08-26 12:14 ` Russell King (Oracle) @ 2021-08-26 15:00 ` Robin Murphy 2021-08-26 15:17 ` Lucas Stach 1 sibling, 1 reply; 13+ messages in thread From: Robin Murphy @ 2021-08-26 15:00 UTC (permalink / raw) To: Michael Walle, etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter On 2021-08-26 13:10, Michael Walle wrote: > The DMA configuration of the virtual device is inherited from the first > actual etnaviv device. Unfortunately, this doesn't work with an IOMMU: > > [ 5.191008] Failed to set up IOMMU for device (null); retaining platform DMA ops > > This is because there is no associated iommu_group with the device. The > group is set in iommu_group_add_device() which is eventually called by > device_add() via the platform bus: > device_add() > blocking_notifier_call_chain() > iommu_bus_notifier() > iommu_probe_device() > __iommu_probe_device() > iommu_group_get_for_dev() > iommu_group_add_device() > > Move of_dma_configure() into the probe function, which is called after > device_add(). Normally, the platform code will already call it itself > if .of_node is set. Unfortunately, this isn't the case here. > > Also move the dma mask assignemnts to probe() to keep all DMA related > settings together. I assume the driver must already keep track of the real GPU platform device in order to map registers, request interrupts, etc. correctly - can't it also correctly use that device for DMA API calls and avoid the need for these shenanigans altogether? FYI, IOMMU configuration is really supposed to *only* run at add_device() time as above - the fact that it's currently hooked in to be retriggered by of_dma_configure() on DT platforms actually turns out to lead to various issues within the IOMMU API, and the plan to change that is slowly climbing up my to-do list. Robin. > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index 2509b3e85709..ff6425f6ebad 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > @@ -589,6 +589,7 @@ static int compare_str(struct device *dev, void *data) > static int etnaviv_pdev_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct device_node *first_node = NULL; > struct component_match *match = NULL; > > if (!dev->platform_data) { > @@ -598,6 +599,9 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) > if (!of_device_is_available(core_node)) > continue; > > + if (!first_node) > + first_node = core_node; > + > drm_of_component_match_add(&pdev->dev, &match, > compare_of, core_node); > } > @@ -609,6 +613,17 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) > component_match_add(dev, &match, compare_str, names[i]); > } > > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > + > + /* > + * Apply the same DMA configuration to the virtual etnaviv > + * device as the GPU we found. This assumes that all Vivante > + * GPUs in the system share the same DMA constraints. > + */ > + if (first_node) > + of_dma_configure(&pdev->dev, first_node, true); > + > return component_master_add_with_match(dev, &etnaviv_master_ops, match); > } > > @@ -659,15 +674,6 @@ static int __init etnaviv_init(void) > of_node_put(np); > goto unregister_platform_driver; > } > - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > - > - /* > - * Apply the same DMA configuration to the virtual etnaviv > - * device as the GPU we found. This assumes that all Vivante > - * GPUs in the system share the same DMA constraints. > - */ > - of_dma_configure(&pdev->dev, np, true); > > ret = platform_device_add(pdev); > if (ret) { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device 2021-08-26 15:00 ` Robin Murphy @ 2021-08-26 15:17 ` Lucas Stach 2021-08-26 20:15 ` Robin Murphy 0 siblings, 1 reply; 13+ messages in thread From: Lucas Stach @ 2021-08-26 15:17 UTC (permalink / raw) To: Robin Murphy, Michael Walle, etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter Am Donnerstag, dem 26.08.2021 um 16:00 +0100 schrieb Robin Murphy: > On 2021-08-26 13:10, Michael Walle wrote: > > The DMA configuration of the virtual device is inherited from the first > > actual etnaviv device. Unfortunately, this doesn't work with an IOMMU: > > > > [ 5.191008] Failed to set up IOMMU for device (null); retaining platform DMA ops > > > > This is because there is no associated iommu_group with the device. The > > group is set in iommu_group_add_device() which is eventually called by > > device_add() via the platform bus: > > device_add() > > blocking_notifier_call_chain() > > iommu_bus_notifier() > > iommu_probe_device() > > __iommu_probe_device() > > iommu_group_get_for_dev() > > iommu_group_add_device() > > > > Move of_dma_configure() into the probe function, which is called after > > device_add(). Normally, the platform code will already call it itself > > if .of_node is set. Unfortunately, this isn't the case here. > > > > Also move the dma mask assignemnts to probe() to keep all DMA related > > settings together. > > I assume the driver must already keep track of the real GPU platform > device in order to map registers, request interrupts, etc. correctly - > can't it also correctly use that device for DMA API calls and avoid the > need for these shenanigans altogether? > Not without a bigger rework. There's still quite a bit of midlayer issues in DRM, where dma-buf imports are dma-mapped and cached via the virtual DRM device instead of the real GPU device. Also etnaviv is able to coalesce multiple Vivante GPUs in a single system under one virtual DRM device, which is used on i.MX6 where the 2D and 3D GPUs are separate peripherals, but have the same DMA constraints. Effectively we would need to handle N devices for the dma-mapping in a lot of places instead of only dealing with the one virtual DRM device. It would probably be the right thing to anyways, but it's not something that can be changed short-term. I'm also not yet sure about the performance implications, as we might run into some cache maintenance bottlenecks if we dma synchronize buffers to multiple real device instead of doing it a single time with the virtual DRM device. I know, I know, this has a lot of assumptions baked in that could fall apart if someone builds a SoC with multiple Vivante GPUs that have differing DMA constraints, but up until now hardware designers have not been *that* crazy, fortunately. Regards, Lucas > FYI, IOMMU configuration is really supposed to *only* run at > add_device() time as above - the fact that it's currently hooked in to > be retriggered by of_dma_configure() on DT platforms actually turns out > to lead to various issues within the IOMMU API, and the plan to change > that is slowly climbing up my to-do list. > > Robin. > > > Signed-off-by: Michael Walle <michael@walle.cc> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 24 +++++++++++++++--------- > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > index 2509b3e85709..ff6425f6ebad 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > @@ -589,6 +589,7 @@ static int compare_str(struct device *dev, void *data) > > static int etnaviv_pdev_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > + struct device_node *first_node = NULL; > > struct component_match *match = NULL; > > > > if (!dev->platform_data) { > > @@ -598,6 +599,9 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) > > if (!of_device_is_available(core_node)) > > continue; > > > > + if (!first_node) > > + first_node = core_node; > > + > > drm_of_component_match_add(&pdev->dev, &match, > > compare_of, core_node); > > } > > @@ -609,6 +613,17 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) > > component_match_add(dev, &match, compare_str, names[i]); > > } > > > > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > + > > + /* > > + * Apply the same DMA configuration to the virtual etnaviv > > + * device as the GPU we found. This assumes that all Vivante > > + * GPUs in the system share the same DMA constraints. > > + */ > > + if (first_node) > > + of_dma_configure(&pdev->dev, first_node, true); > > + > > return component_master_add_with_match(dev, &etnaviv_master_ops, match); > > } > > > > @@ -659,15 +674,6 @@ static int __init etnaviv_init(void) > > of_node_put(np); > > goto unregister_platform_driver; > > } > > - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > - > > - /* > > - * Apply the same DMA configuration to the virtual etnaviv > > - * device as the GPU we found. This assumes that all Vivante > > - * GPUs in the system share the same DMA constraints. > > - */ > > - of_dma_configure(&pdev->dev, np, true); > > > > ret = platform_device_add(pdev); > > if (ret) { > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device 2021-08-26 15:17 ` Lucas Stach @ 2021-08-26 20:15 ` Robin Murphy 0 siblings, 0 replies; 13+ messages in thread From: Robin Murphy @ 2021-08-26 20:15 UTC (permalink / raw) To: Lucas Stach, Michael Walle, etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter On 2021-08-26 16:17, Lucas Stach wrote: > Am Donnerstag, dem 26.08.2021 um 16:00 +0100 schrieb Robin Murphy: >> On 2021-08-26 13:10, Michael Walle wrote: >>> The DMA configuration of the virtual device is inherited from the first >>> actual etnaviv device. Unfortunately, this doesn't work with an IOMMU: >>> >>> [ 5.191008] Failed to set up IOMMU for device (null); retaining platform DMA ops >>> >>> This is because there is no associated iommu_group with the device. The >>> group is set in iommu_group_add_device() which is eventually called by >>> device_add() via the platform bus: >>> device_add() >>> blocking_notifier_call_chain() >>> iommu_bus_notifier() >>> iommu_probe_device() >>> __iommu_probe_device() >>> iommu_group_get_for_dev() >>> iommu_group_add_device() >>> >>> Move of_dma_configure() into the probe function, which is called after >>> device_add(). Normally, the platform code will already call it itself >>> if .of_node is set. Unfortunately, this isn't the case here. >>> >>> Also move the dma mask assignemnts to probe() to keep all DMA related >>> settings together. >> >> I assume the driver must already keep track of the real GPU platform >> device in order to map registers, request interrupts, etc. correctly - >> can't it also correctly use that device for DMA API calls and avoid the >> need for these shenanigans altogether? >> > Not without a bigger rework. There's still quite a bit of midlayer > issues in DRM, where dma-buf imports are dma-mapped and cached via the > virtual DRM device instead of the real GPU device. Also etnaviv is able > to coalesce multiple Vivante GPUs in a single system under one virtual > DRM device, which is used on i.MX6 where the 2D and 3D GPUs are > separate peripherals, but have the same DMA constraints. Sure, I wouldn't expect it to be trivial to fix properly, but I wanted to point out that this is essentially a hack, relying on an implicit side-effect of of_dma_configure() which is already slated for removal. As such, I for one am not going to be too sympathetic if it stops working in future. Furthermore, even today it doesn't work in general - it might be OK for LS1028A with a single GPU block behind an SMMU, but as soon as you have multiple GPU blocks with distinct SMMU StreamIDs, or behind different IOMMU instances, then you're stuffed again. Although in fact I think it's also broken even for LS1028A, since AFAICS there's no guarantee that the relevant SMMU instance will actually be probed, or the SMMU driver even loaded, when etnaviv_pdev_probe() runs. > Effectively we would need to handle N devices for the dma-mapping in a > lot of places instead of only dealing with the one virtual DRM device. > It would probably be the right thing to anyways, but it's not something > that can be changed short-term. I'm also not yet sure about the > performance implications, as we might run into some cache maintenance > bottlenecks if we dma synchronize buffers to multiple real device > instead of doing it a single time with the virtual DRM device. I know, > I know, this has a lot of assumptions baked in that could fall apart if > someone builds a SoC with multiple Vivante GPUs that have differing DMA > constraints, but up until now hardware designers have not been *that* > crazy, fortunately. I'm not too familiar with the component stuff, but would it be viable to just have etnaviv_gpu_platform_probe() set up the first GPU which comes along as the master component and fundamental DRM device, then treat any subsequent ones as subcomponents as before? That would at least stand to be more robust in terms of obviating the of_dma_configure() hack (only actual bus code should ever be calling that), even if it won't do anything for the multiple IOMMU mapping or differing DMA constraints problems. Thanks, Robin. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask 2021-08-26 12:10 [PATCH 0/3] drm/etnaviv: IOMMU related fixes Michael Walle 2021-08-26 12:10 ` [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE Michael Walle 2021-08-26 12:10 ` [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device Michael Walle @ 2021-08-26 12:10 ` Michael Walle 2021-08-26 12:19 ` Russell King (Oracle) 2021-08-26 12:43 ` Michael Walle 2 siblings, 2 replies; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:10 UTC (permalink / raw) To: etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter, Michael Walle The STLB and the first command buffer (which is used to set up the TLBs) has a 32 bit size restriction in hardware. There seems to be no way to specify addresses larger than 32 bit. Keep it simple and restict the addresses to the lower 4 GiB range for all coherent DMA memory allocations. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index ff6425f6ebad..0b756ecb1bc2 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -613,8 +613,23 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) component_match_add(dev, &match, compare_str, names[i]); } - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; + /* + * PTA and MTLB can have 40 bit base addresses, but + * unfortunately, an entry in the MTLB can only point to a + * 32 bit base address of a STLB. Moreover, to initialize the + * MMU we need a command buffer with a 32 bit address because + * without an MMU there is only an indentity mapping between + * the internal 32 bit addresses and the bus addresses. + * + * To make things easy, we set the dma_coherent_mask to 32 + * bit to make sure we are allocating the command buffers and + * TLBs in the lower 4 GiB address space. + */ + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) || + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { + dev_dbg(&pdev->dev, "No suitable DMA available\n"); + return -ENODEV; + } /* * Apply the same DMA configuration to the virtual etnaviv -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask 2021-08-26 12:10 ` [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask Michael Walle @ 2021-08-26 12:19 ` Russell King (Oracle) 2021-08-26 12:25 ` Michael Walle 2021-08-26 12:43 ` Michael Walle 1 sibling, 1 reply; 13+ messages in thread From: Russell King (Oracle) @ 2021-08-26 12:19 UTC (permalink / raw) To: Michael Walle Cc: etnaviv, dri-devel, linux-kernel, Lukas F . Hartmann, Marek Vasut, Lucas Stach, Christian Gmeiner, David Airlie, Daniel Vetter On Thu, Aug 26, 2021 at 02:10:06PM +0200, Michael Walle wrote: > - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > + /* > + * PTA and MTLB can have 40 bit base addresses, but > + * unfortunately, an entry in the MTLB can only point to a > + * 32 bit base address of a STLB. Moreover, to initialize the > + * MMU we need a command buffer with a 32 bit address because > + * without an MMU there is only an indentity mapping between > + * the internal 32 bit addresses and the bus addresses. > + * > + * To make things easy, we set the dma_coherent_mask to 32 > + * bit to make sure we are allocating the command buffers and > + * TLBs in the lower 4 GiB address space. > + */ > + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) || > + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { > + dev_dbg(&pdev->dev, "No suitable DMA available\n"); > + return -ENODEV; > + } This makes no sense. In the previous patch, you initialised pdev->dev.dma_mask ot point at the coherent mask, implying that it wasn't already set - for which dma_coerce_mask_and_coherent() should be used. Now you're just calling dma_set_mask(), which will fail if pdev->dev.dma_mask hasn't already been set to point at something. If it's already been initialised to point at something, then you shouldn't be overwriting it in the driver, and you should've used dma_set_mask_and_coherent() in your previous patch. Confused. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask 2021-08-26 12:19 ` Russell King (Oracle) @ 2021-08-26 12:25 ` Michael Walle 0 siblings, 0 replies; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:25 UTC (permalink / raw) To: Russell King (Oracle) Cc: etnaviv, dri-devel, linux-kernel, Lukas F . Hartmann, Marek Vasut, Lucas Stach, Christian Gmeiner, David Airlie, Daniel Vetter Am 2021-08-26 14:19, schrieb Russell King (Oracle): > On Thu, Aug 26, 2021 at 02:10:06PM +0200, Michael Walle wrote: >> - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); >> - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; >> + /* >> + * PTA and MTLB can have 40 bit base addresses, but >> + * unfortunately, an entry in the MTLB can only point to a >> + * 32 bit base address of a STLB. Moreover, to initialize the >> + * MMU we need a command buffer with a 32 bit address because >> + * without an MMU there is only an indentity mapping between >> + * the internal 32 bit addresses and the bus addresses. >> + * >> + * To make things easy, we set the dma_coherent_mask to 32 >> + * bit to make sure we are allocating the command buffers and >> + * TLBs in the lower 4 GiB address space. >> + */ >> + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) || >> + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { >> + dev_dbg(&pdev->dev, "No suitable DMA available\n"); >> + return -ENODEV; >> + } > > This makes no sense. In the previous patch, you initialised > pdev->dev.dma_mask ot point at the coherent mask, implying that > it wasn't already set - for which dma_coerce_mask_and_coherent() > should be used. Now you're just calling dma_set_mask(), which will > fail if pdev->dev.dma_mask hasn't already been set to point at > something. > > If it's already been initialised to point at something, then you > shouldn't be overwriting it in the driver, and you should've used > dma_set_mask_and_coherent() in your previous patch. > > Confused. Mh, I see that moving these two lines was a bad idea. See commit message in patch 2/3: > Also move the dma mask assignemnts to probe() to keep all DMA related > settings together. The actual fix in patch 2/3 is the move of the of_dma_configure() not the dma_mask assignments. -michael ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask 2021-08-26 12:10 ` [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask Michael Walle 2021-08-26 12:19 ` Russell King (Oracle) @ 2021-08-26 12:43 ` Michael Walle 1 sibling, 0 replies; 13+ messages in thread From: Michael Walle @ 2021-08-26 12:43 UTC (permalink / raw) To: etnaviv, dri-devel, linux-kernel Cc: Lukas F . Hartmann, Marek Vasut, Lucas Stach, Russell King, Christian Gmeiner, David Airlie, Daniel Vetter Am 2021-08-26 14:10, schrieb Michael Walle: > The STLB and the first command buffer (which is used to set up the > TLBs) > has a 32 bit size restriction in hardware. There seems to be no way to > specify addresses larger than 32 bit. Keep it simple and restict the > addresses to the lower 4 GiB range for all coherent DMA memory > allocations. > > Signed-off-by: Michael Walle <michael@walle.cc> Suggested-by: Lucas Stach <l.stach@pengutronix.de> is missing here. sorry, will add it in the next version. -michael ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-08-26 20:15 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-26 12:10 [PATCH 0/3] drm/etnaviv: IOMMU related fixes Michael Walle 2021-08-26 12:10 ` [PATCH 1/3] drm/etnaviv: use PLATFORM_DEVID_NONE Michael Walle 2021-08-26 12:59 ` Christian Gmeiner 2021-08-26 12:10 ` [PATCH 2/3] drm/etnaviv: fix dma configuration of the virtual device Michael Walle 2021-08-26 12:14 ` Russell King (Oracle) 2021-08-26 12:17 ` Michael Walle 2021-08-26 15:00 ` Robin Murphy 2021-08-26 15:17 ` Lucas Stach 2021-08-26 20:15 ` Robin Murphy 2021-08-26 12:10 ` [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask Michael Walle 2021-08-26 12:19 ` Russell King (Oracle) 2021-08-26 12:25 ` Michael Walle 2021-08-26 12:43 ` Michael Walle
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).