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
changes since v1:
- don't move the former dma_mask setup code around in patch 2/3. Will
avoid any confusion.
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
There is already a macro for the magic value. Use it.
Signed-off-by: Michael Walle <[email protected]>
Reviewed-by: Christian Gmeiner <[email protected]>
---
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
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.
Please note, that platform_device_alloc() will initialize dev->dma_mask
to point to pdev->platform_dma_mask, thus dma_set_mask() will work as
expected.
While at it, move the dma_mask setup code to the of_dma_configure() to
keep all the DMA setup code next to each other.
Suggested-by: Lucas Stach <[email protected]>
Signed-off-by: Michael Walle <[email protected]>
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 54eb653ca295..0b756ecb1bc2 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
component_match_add(dev, &match, compare_str, names[i]);
}
+ /*
+ * 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
* device as the GPU we found. This assumes that all Vivante
@@ -671,8 +689,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;
ret = platform_device_add(pdev);
if (ret) {
--
2.30.2
Am 2021-09-07 18:49, schrieb 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
>
> changes since v1:
> - don't move the former dma_mask setup code around in patch 2/3. Will
> avoid any confusion.
>
> 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(-)
ping? :)
-michael
Hi Michael,
Am Dienstag, dem 07.09.2021 um 18:49 +0200 schrieb 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
>
> changes since v1:
> - don't move the former dma_mask setup code around in patch 2/3. Will
> avoid any confusion.
>
> 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(-)
>
Thanks for the patches! I applied them to my etnaviv/next branch.
Regards,
Lucas
Sorry I missed this earlier...
On 2021-09-07 17:49, Michael Walle wrote:
> 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.
>
> Please note, that platform_device_alloc() will initialize dev->dma_mask
> to point to pdev->platform_dma_mask, thus dma_set_mask() will work as
> expected.
>
> While at it, move the dma_mask setup code to the of_dma_configure() to
> keep all the DMA setup code next to each other.
>
> Suggested-by: Lucas Stach <[email protected]>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 54eb653ca295..0b756ecb1bc2 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
> component_match_add(dev, &match, compare_str, names[i]);
> }
>
> + /*
> + * 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))) {
Since AFAICS you're not changing the default dma_mask pointer to point
to some storage other than the coherent mask, the dma_set_mask() call
effectively does nothing and both masks will end up reading back as 32 bits.
Robin.
> + dev_dbg(&pdev->dev, "No suitable DMA available\n");
> + return -ENODEV;
> + }
> +
> /*
> * Apply the same DMA configuration to the virtual etnaviv
> * device as the GPU we found. This assumes that all Vivante
> @@ -671,8 +689,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;
>
> ret = platform_device_add(pdev);
> if (ret) {
>
Hi Robin,
Am Mittwoch, dem 01.12.2021 um 12:50 +0000 schrieb Robin Murphy:
> Sorry I missed this earlier...
>
> On 2021-09-07 17:49, Michael Walle wrote:
> > 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.
> >
> > Please note, that platform_device_alloc() will initialize dev->dma_mask
> > to point to pdev->platform_dma_mask, thus dma_set_mask() will work as
> > expected.
> >
> > While at it, move the dma_mask setup code to the of_dma_configure() to
> > keep all the DMA setup code next to each other.
> >
> > Suggested-by: Lucas Stach <[email protected]>
> > Signed-off-by: Michael Walle <[email protected]>
> > ---
> > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++++++++++++++++++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > index 54eb653ca295..0b756ecb1bc2 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > @@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
> > component_match_add(dev, &match, compare_str, names[i]);
> > }
> >
> > + /*
> > + * 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))) {
>
> Since AFAICS you're not changing the default dma_mask pointer to point
> to some storage other than the coherent mask, the dma_set_mask() call
> effectively does nothing and both masks will end up reading back as 32 bits.
>
From what I can see the dma_mask has allocated storage in the platform
device and does not point to the coherent dma mask, see
setup_pdev_dma_masks().
Regards,
Lucas
> Robin.
>
> > + dev_dbg(&pdev->dev, "No suitable DMA available\n");
> > + return -ENODEV;
> > + }
> > +
> > /*
> > * Apply the same DMA configuration to the virtual etnaviv
> > * device as the GPU we found. This assumes that all Vivante
> > @@ -671,8 +689,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;
> >
> > ret = platform_device_add(pdev);
> > if (ret) {
> >
On 2021-12-01 13:41, Lucas Stach wrote:
> Hi Robin,
>
> Am Mittwoch, dem 01.12.2021 um 12:50 +0000 schrieb Robin Murphy:
>> Sorry I missed this earlier...
>>
>> On 2021-09-07 17:49, Michael Walle wrote:
>>> 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.
>>>
>>> Please note, that platform_device_alloc() will initialize dev->dma_mask
>>> to point to pdev->platform_dma_mask, thus dma_set_mask() will work as
>>> expected.
>>>
>>> While at it, move the dma_mask setup code to the of_dma_configure() to
>>> keep all the DMA setup code next to each other.
>>>
>>> Suggested-by: Lucas Stach <[email protected]>
>>> Signed-off-by: Michael Walle <[email protected]>
>>> ---
>>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++++++++++++++++++--
>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> index 54eb653ca295..0b756ecb1bc2 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> @@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>>> component_match_add(dev, &match, compare_str, names[i]);
>>> }
>>>
>>> + /*
>>> + * 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))) {
>>
>> Since AFAICS you're not changing the default dma_mask pointer to point
>> to some storage other than the coherent mask, the dma_set_mask() call
>> effectively does nothing and both masks will end up reading back as 32 bits.
>>
> From what I can see the dma_mask has allocated storage in the platform
> device and does not point to the coherent dma mask, see
> setup_pdev_dma_masks().
Urgh, apologies for the confusion - seems I had one of those mental
short-circuits and was utterly convinced that that's what the platform
device setup did, but of course it's only the fallback case in
of_dma_configure(). Sorry for the noise!
Cheers,
Robin.