[PATCH 1/2] drm/tegra: Set DMA ops

[PATCH 1/2] drm/tegra: Set DMA ops

[Alexandre Courbot]

The current settings leaves the DRM device's dma_ops field NULL, which
makes it use the dummy DMA ops on arm64 and return an error whenever we
try to import a buffer. Call of_dma_configure() with a NULL node (since
the device is not spawn from the device tree) so that
arch_setup_dma_ops() is called and sets the default ioswtlb DMA ops.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index d347188bf8f4..bc0555adecaf 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -9,6 +9,7 @@
 
 #include <linux/host1x.h>
 #include <linux/iommu.h>
+#include <linux/of_device.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -990,6 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
 		return -ENOMEM;
 
 	dev_set_drvdata(&dev->dev, drm);
+	of_dma_configure(drm->dev, NULL);
 
 	err = drm_dev_register(drm, 0);
 	if (err < 0)
-- 
2.7.1

[PATCH 2/2] drm/tegra: Set the DMA mask

[Alexandre Courbot]

The default DMA mask covers a 32 bits address range, but tegradrm can
address more than that. Set the DMA mask to the actual addressable range
to avoid the use of unneeded bounce buffers.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Thierry, I am not absolutely sure whether the size is correct and applies
to all Tegra generations - please let me know if this needs to be
reworked.

 drivers/gpu/drm/tegra/drm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index bc0555adecaf..503fc9e73521 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -992,6 +992,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
 
 	dev_set_drvdata(&dev->dev, drm);
 	of_dma_configure(drm->dev, NULL);
+	dma_set_mask(drm->dev, DMA_BIT_MASK(34));
 
 	err = drm_dev_register(drm, 0);
 	if (err < 0)
-- 
2.7.1

Re: [PATCH 2/2] drm/tegra: Set the DMA mask

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]

On Tue, Feb 23, 2016 at 03:25:54PM +0900, Alexandre Courbot wrote:
> The default DMA mask covers a 32 bits address range, but tegradrm can
> address more than that. Set the DMA mask to the actual addressable range
> to avoid the use of unneeded bounce buffers.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Thierry, I am not absolutely sure whether the size is correct and applies
> to all Tegra generations - please let me know if this needs to be
> reworked.
> 
>  drivers/gpu/drm/tegra/drm.c | 1 +
>  1 file changed, 1 insertion(+)

This kind of depends on whether or not the device is behind an IOMMU. If
it is, then the IOMMU DMA MASK would apply, which can be derived from
the number of address bits that the IOMMU can handle. The SMMU supports
32 address bits on Tegra30 and Tegra114, 34 address bits on more recent
generations.

I think for now it's safer to leave the DMA mask at the default (32 bit)
to avoid the need to distinguish between IOMMU and non-IOMMU devices.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] drm/tegra: Set the DMA mask

[Terje Bergstrom]

On 02/23/2016 08:04 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key  > > On Tue, Feb 23, 2016 at 03:25:54PM +0900, Alexandre Courbot wrote: 
 >> The default DMA mask covers a 32 bits address range, but tegradrm >> 
can address more than that. Set the DMA mask to the actual >> 
addressable range to avoid the use of unneeded bounce buffers. >> >> 
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- Thierry, >> I 
am not absolutely sure whether the size is correct and applies to >> all 
Tegra generations - please let me know if this needs to be >> reworked. 
 >> >> drivers/gpu/drm/tegra/drm.c | 1 + 1 file changed, 1 insertion(+) 
 > > This kind of depends on whether or not the device is behind an 
IOMMU. > If it is, then the IOMMU DMA MASK would apply, which can be 
derived > from the number of address bits that the IOMMU can handle. The 
SMMU > supports 32 address bits on Tegra30 and Tegra114, 34 address bits 
on > more recent generations. > > I think for now it's safer to leave 
the DMA mask at the default (32 > bit) to avoid the need to distinguish 
between IOMMU and non-IOMMU > devices.

The GPUs after Tegra114 can choose per access whether they're using IOMMU
or not. The interface is 34 bits wide, so the physical addresses can be 
34 bits.
IOMMU addresses are limited by Tegra SMMU to 32-bit for gk20a. gm20b can use
34-bit if SMMU is configured to combine four ASIDs together.

Re: [PATCH 2/2] drm/tegra: Set the DMA mask

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On Tue, Feb 23, 2016 at 08:18:26AM -0800, Terje Bergstrom wrote:
> 
> 
> On 02/23/2016 08:04 AM, Thierry Reding wrote:
> >* PGP Signed by an unknown key  > > On Tue, Feb 23, 2016 at 03:25:54PM
> >+0900, Alexandre Courbot wrote:
> >> The default DMA mask covers a 32 bits address range, but tegradrm >> can
> address more than that. Set the DMA mask to the actual >> addressable range
> to avoid the use of unneeded bounce buffers. >> >> Signed-off-by: Alexandre
> Courbot <acourbot@nvidia.com> --- Thierry, >> I am not absolutely sure
> whether the size is correct and applies to >> all Tegra generations - please
> let me know if this needs to be >> reworked. >> >>
> drivers/gpu/drm/tegra/drm.c | 1 + 1 file changed, 1 insertion(+) > > This
> kind of depends on whether or not the device is behind an IOMMU. > If it is,
> then the IOMMU DMA MASK would apply, which can be derived > from the number
> of address bits that the IOMMU can handle. The SMMU > supports 32 address
> bits on Tegra30 and Tegra114, 34 address bits on > more recent generations.
> > > I think for now it's safer to leave the DMA mask at the default (32 >
> bit) to avoid the need to distinguish between IOMMU and non-IOMMU > devices.
> 
> The GPUs after Tegra114 can choose per access whether they're using IOMMU
> or not. The interface is 34 bits wide, so the physical addresses can be 34
> bits.
> IOMMU addresses are limited by Tegra SMMU to 32-bit for gk20a. gm20b can use
> 34-bit if SMMU is configured to combine four ASIDs together.

This particular patch sets up the DMA mask for the display engines. But
yes, most of the above holds true for that case as well, except that as
far as I know there is no mechanism to have the display engines choose
per access, whether or not to use the SMMU.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] drm/tegra: Set the DMA mask

[Alexandre Courbot]

On 02/24/2016 01:04 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Feb 23, 2016 at 03:25:54PM +0900, Alexandre Courbot wrote:
>> The default DMA mask covers a 32 bits address range, but tegradrm can
>> address more than that. Set the DMA mask to the actual addressable range
>> to avoid the use of unneeded bounce buffers.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> Thierry, I am not absolutely sure whether the size is correct and applies
>> to all Tegra generations - please let me know if this needs to be
>> reworked.
>>
>>   drivers/gpu/drm/tegra/drm.c | 1 +
>>   1 file changed, 1 insertion(+)
>
> This kind of depends on whether or not the device is behind an IOMMU.  If
> it is, then the IOMMU DMA MASK would apply, which can be derived from
> the number of address bits that the IOMMU can handle. The SMMU supports
> 32 address bits on Tegra30 and Tegra114, 34 address bits on more recent
> generations.
>
> I think for now it's safer to leave the DMA mask at the default (32 bit)
> to avoid the need to distinguish between IOMMU and non-IOMMU devices.

Leaving it that way makes it (almost) impossible to import buffers on 
TX1. Patch 1 sets the DMA ops to swiotlb, so at least after this we 
actually try to import the buffer. However, any page that is higher than 
the 32 bits range will be bounced. If you are lucky, you won't notice it 
(even though I don't think it is acceptable to bounce data to be 
displayed), but most of the time the swiotlb bounce area will run full 
and import will fail with the following message:

drm drm: swiotlb buffer is full (sz: 294912 bytes)

So we should really try and fix this. The issue is, how do you detect 
whether you are behind a IOMMU? The DCs have an iommus property, but the 
drm device (which does the importing) does not. And when we import a 
buffer into tegradrm, nothing guarantees that it is for display. So far 
I cannot think of a better heuristics than "assume 32 bits on < t124 and 
34 bits afterwards".

Re: [PATCH 1/2] drm/tegra: Set DMA ops

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Tue, Feb 23, 2016 at 03:25:53PM +0900, Alexandre Courbot wrote:
> The current settings leaves the DRM device's dma_ops field NULL, which
> makes it use the dummy DMA ops on arm64 and return an error whenever we
> try to import a buffer. Call of_dma_configure() with a NULL node (since
> the device is not spawn from the device tree) so that
> arch_setup_dma_ops() is called and sets the default ioswtlb DMA ops.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index d347188bf8f4..bc0555adecaf 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/host1x.h>
>  #include <linux/iommu.h>
> +#include <linux/of_device.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -990,6 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
>  		return -ENOMEM;
>  
>  	dev_set_drvdata(&dev->dev, drm);
> +	of_dma_configure(drm->dev, NULL);

Looking at the various pieces, I think this really belongs in
host1x_device_add() (see drivers/gpu/host1x/bus.c) where it can replace
the open-coded setting of DMA and coherent DMA masks. Also why can't we
pass the correct device tree node here? The DRM device is a virtual
device that hangs off the host1x device, so I think it could use the
same device tree node as the host1x device.

Something like the below (untested).

Thierry

--- >8 ---
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index c2e7fba370bb..d46d26a574da 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -17,6 +17,7 @@
 
 #include <linux/host1x.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 
 #include "bus.h"
@@ -393,9 +394,8 @@ static int host1x_device_add(struct host1x *host1x,
 	INIT_LIST_HEAD(&device->list);
 	device->driver = driver;
 
-	device->dev.coherent_dma_mask = host1x->dev->coherent_dma_mask;
-	device->dev.dma_mask = &device->dev.coherent_dma_mask;
 	dev_set_name(&device->dev, "%s", driver->driver.name);
+	of_dma_configure(&device->dev, host1x->dev->of_node);
 	device->dev.release = host1x_device_release;
 	device->dev.bus = &host1x_bus_type;
 	device->dev.parent = host1x->dev;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] drm/tegra: Set DMA ops

[Alexandre Courbot]

On 02/24/2016 12:28 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Feb 23, 2016 at 03:25:53PM +0900, Alexandre Courbot wrote:
>> The current settings leaves the DRM device's dma_ops field NULL, which
>> makes it use the dummy DMA ops on arm64 and return an error whenever we
>> try to import a buffer. Call of_dma_configure() with a NULL node (since
>> the device is not spawn from the device tree) so that
>> arch_setup_dma_ops() is called and sets the default ioswtlb DMA ops.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>   drivers/gpu/drm/tegra/drm.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index d347188bf8f4..bc0555adecaf 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -9,6 +9,7 @@
>>
>>   #include <linux/host1x.h>
>>   #include <linux/iommu.h>
>> +#include <linux/of_device.h>
>>
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>> @@ -990,6 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
>>   		return -ENOMEM;
>>
>>   	dev_set_drvdata(&dev->dev, drm);
>> +	of_dma_configure(drm->dev, NULL);
>
> Looking at the various pieces, I think this really belongs in
> host1x_device_add() (see drivers/gpu/host1x/bus.c) where it can replace
> the open-coded setting of DMA and coherent DMA masks. Also why can't we
> pass the correct device tree node here? The DRM device is a virtual
> device that hangs off the host1x device, so I think it could use the
> same device tree node as the host1x device.
>
> Something like the below (untested).

You're right, that looks like a much better place to do this. 
of_dma_configure() is called at the bus level (platform and PCI), so it 
makes sense to do it from host1x too.