[PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[Christoph Hellwig]

The omapfb platform devices does not have a DMA mask set.  The
traditional arm DMA code ignores, but the generic dma-direct/swiotlb
has stricter checks and thus fails mappings without a DMA mask.
As we use swiotlb for arm with LPAE now, omap needs to catch up
and actually set a DMA mask.

Fixes: ad3c7b18c5b3 ("arm: use swiotlb for bounce buffering on LPAE configs")
Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
Tested-by: "H. Nikolaus Schaller" <hns@goldelico.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 561c4812545b..2c8abf07e617 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -232,6 +232,8 @@ void omap_fbdev_init(struct drm_device *dev)
 	if (!priv->num_pipes)
 		return;
 
+	dma_coerce_mask_and_coherent(dev->dev, DMA_BIT_MASK(32));
+
 	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
 	if (!fbdev)
 		goto fail;
-- 
2.20.1

Re: [PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[Tomi Valkeinen]

Hi,

On 08/08/2019 13:10, Christoph Hellwig wrote:
> The omapfb platform devices does not have a DMA mask set.  The
> traditional arm DMA code ignores, but the generic dma-direct/swiotlb
> has stricter checks and thus fails mappings without a DMA mask.
> As we use swiotlb for arm with LPAE now, omap needs to catch up
> and actually set a DMA mask.
> 
> Fixes: ad3c7b18c5b3 ("arm: use swiotlb for bounce buffering on LPAE configs")
> Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> Tested-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index 561c4812545b..2c8abf07e617 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -232,6 +232,8 @@ void omap_fbdev_init(struct drm_device *dev)
>   	if (!priv->num_pipes)
>   		return;
>   
> +	dma_coerce_mask_and_coherent(dev->dev, DMA_BIT_MASK(32));
> +
>   	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>   	if (!fbdev)
>   		goto fail;
> 

We do call dma_set_coherent_mask() in omapdrm's probe() (in omap_drv.c), 
but apparently that's not enough anymore. Changing that call to 
dma_coerce_mask_and_coherent() removes the WARN. I can create a patch 
for that, or Christoph can respin this one.

I am not too familiar with the dma mask handling, so maybe someone can 
educate:

dma_coerce_mask_and_coherent() overwrites dev->dma_mask. Isn't that a 
bad thing? What if the platform has set dev->dma_mask, and the driver 
overwrites it with its value? Or who is supposed to set dev->dma_mask?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[Christoph Hellwig]

On Fri, Aug 09, 2019 at 09:40:32AM +0300, Tomi Valkeinen wrote:
> We do call dma_set_coherent_mask() in omapdrm's probe() (in omap_drv.c), 
> but apparently that's not enough anymore. Changing that call to 
> dma_coerce_mask_and_coherent() removes the WARN. I can create a patch for 
> that, or Christoph can respin this one.

Oh, yes - that actually is the right thing to do here.  If you already
have it please just send it out.

>
> I am not too familiar with the dma mask handling, so maybe someone can 
> educate:
>
> dma_coerce_mask_and_coherent() overwrites dev->dma_mask. Isn't that a bad 
> thing? What if the platform has set dev->dma_mask, and the driver 
> overwrites it with its value? Or who is supposed to set dev->dma_mask?

->dma_mask is a complete mess.  It is a pointer when it really should
just be a u64, and that means every driver layer has to allocate space
for it.  We don't really do that for platform_devices, as that breaks
horribly assumptions in the usb code.  That is why
dma_coerce_mask_and_coherent exists as a nasty workaround that sets
the dma_mask to the coherent_dma_mask for devices that don't have
space for ->dma_mask allocated, which works as long as the device
doesn't have differnet addressing requirements for both.

I'm actually working to fix that mess up at the moment, but it is going
to take a few cycles until everything falls into place.

Re: [PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[Tomi Valkeinen]

On 09/08/2019 11:07, Christoph Hellwig wrote:
> On Fri, Aug 09, 2019 at 09:40:32AM +0300, Tomi Valkeinen wrote:
>> We do call dma_set_coherent_mask() in omapdrm's probe() (in omap_drv.c),
>> but apparently that's not enough anymore. Changing that call to
>> dma_coerce_mask_and_coherent() removes the WARN. I can create a patch for
>> that, or Christoph can respin this one.
> 
> Oh, yes - that actually is the right thing to do here.  If you already
> have it please just send it out.
> 
>>
>> I am not too familiar with the dma mask handling, so maybe someone can
>> educate:
>>
>> dma_coerce_mask_and_coherent() overwrites dev->dma_mask. Isn't that a bad
>> thing? What if the platform has set dev->dma_mask, and the driver
>> overwrites it with its value? Or who is supposed to set dev->dma_mask?
> 
> ->dma_mask is a complete mess.  It is a pointer when it really should
> just be a u64, and that means every driver layer has to allocate space
> for it.  We don't really do that for platform_devices, as that breaks
> horribly assumptions in the usb code.  That is why
> dma_coerce_mask_and_coherent exists as a nasty workaround that sets
> the dma_mask to the coherent_dma_mask for devices that don't have
> space for ->dma_mask allocated, which works as long as the device
> doesn't have differnet addressing requirements for both.
> 
> I'm actually working to fix that mess up at the moment, but it is going
> to take a few cycles until everything falls into place.

Alright, thanks for the clarification!

Here's my version.

From c258309e36fc86076db155aead03a3900b96c3d4 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Fri, 9 Aug 2019 09:54:49 +0300
Subject: [PATCH] drm/omap: ensure we have a valid dma_mask

The omapdrm driver uses dma_set_coherent_mask(), but that's not enough
anymore when LPAE is enabled.

From Christoph Hellwig <hch@lst.de>:

The traditional arm DMA code ignores, but the generic dma-direct/swiotlb
has stricter checks and thus fails mappings without a DMA mask.  As we
use swiotlb for arm with LPAE now, omapdrm needs to catch up and
actually set a DMA mask.

Change the dma_set_coherent_mask() call to
dma_coerce_mask_and_coherent() so that the dev->dma_mask is also set.

Fixes: ad3c7b18c5b3 ("arm: use swiotlb for bounce buffering on LPAE configs")
Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 288c59dae56a..1bad0a2cc5c6 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -669,7 +669,7 @@ static int pdev_probe(struct platform_device *pdev)
 	if (omapdss_is_initialized() == false)
 		return -EPROBE_DEFER;
 
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to set the DMA mask\n");
 		return ret;




-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[Christoph Hellwig]

On Fri, Aug 09, 2019 at 01:00:38PM +0300, Tomi Valkeinen wrote:
> Alright, thanks for the clarification!
> 
> Here's my version.

Looks god to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH for-5.3] drm/omap: ensure we have a valid dma_mask

[Peter Ujfalusi]

On 09/08/2019 13.00, Tomi Valkeinen wrote:
> Here's my version.
> 
> From c258309e36fc86076db155aead03a3900b96c3d4 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Fri, 9 Aug 2019 09:54:49 +0300
> Subject: [PATCH] drm/omap: ensure we have a valid dma_mask
> 
> The omapdrm driver uses dma_set_coherent_mask(), but that's not enough
> anymore when LPAE is enabled.
> 
> From Christoph Hellwig <hch@lst.de>:
> 
> The traditional arm DMA code ignores, but the generic dma-direct/swiotlb
> has stricter checks and thus fails mappings without a DMA mask.  As we
> use swiotlb for arm with LPAE now, omapdrm needs to catch up and
> actually set a DMA mask.
> 
> Change the dma_set_coherent_mask() call to
> dma_coerce_mask_and_coherent() so that the dev->dma_mask is also set.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> Fixes: ad3c7b18c5b3 ("arm: use swiotlb for bounce buffering on LPAE configs")
> Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 288c59dae56a..1bad0a2cc5c6 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -669,7 +669,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	if (omapdss_is_initialized() == false)
>  		return -EPROBE_DEFER;
>  
> -	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to set the DMA mask\n");
>  		return ret;
> 
> 
> 
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki