[PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

dma_map_sg() expects a DMA domain. However, the drm devices
have been traditionally using unmanaged iommu domain which
is non-dma type. Using dma mapping APIs with that domain is bad.

Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}()
to do the cache maintenance.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Suggested-by: Tomasz Figa <tfiga@chromium.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Jordan Crouse <jcrouse@codeaurora.org> 
Cc: Sean Paul <seanpaul@chromium.org>
---

Changes since v1:
 - Addressed Jordan's and Tomasz's comments for
   - moving sg dma addresses preparation out of the coditional
     check to the main path so we do it irrespective of whether
     the buffer is cached or uncached.
   - Enhance the comment to explain this dma addresses prepartion.

 drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 00c795ced02c..1811ac23a31c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
 		struct drm_device *dev = obj->dev;
 		struct page **p;
 		int npages = obj->size >> PAGE_SHIFT;
+		struct scatterlist *s;
+		int i;
 
 		if (use_pages(obj))
 			p = drm_gem_get_pages(obj);
@@ -104,12 +106,21 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			return ptr;
 		}
 
-		/* For non-cached buffers, ensure the new pages are clean
+		/*
+		 * dma_sync_sg_*() flush the physical pages, so point
+		 * sg->dma_address to the physical ones for the right behavior.
+		 */
+		for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
+			sg_dma_address(s) = sg_phys(s);
+
+		/*
+		 * For non-cached buffers, ensure the new pages are clean
 		 * because display controller, GPU, etc. are not coherent:
 		 */
-		if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-			dma_map_sg(dev->dev, msm_obj->sgt->sgl,
-					msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+		if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
+			dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
+					       msm_obj->sgt->nents,
+					       DMA_TO_DEVICE);
 	}
 
 	return msm_obj->pages;
@@ -133,14 +144,16 @@ static void put_pages(struct drm_gem_object *obj)
 
 	if (msm_obj->pages) {
 		if (msm_obj->sgt) {
-			/* For non-cached buffers, ensure the new
+			/*
+			 * For non-cached buffers, ensure the new
 			 * pages are clean because display controller,
 			 * GPU, etc. are not coherent:
 			 */
-			if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-				dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
-					     msm_obj->sgt->nents,
-					     DMA_BIDIRECTIONAL);
+			if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
+				dma_sync_sg_for_cpu(obj->dev->dev,
+						    msm_obj->sgt->sgl,
+						    msm_obj->sgt->nents,
+						    DMA_FROM_DEVICE);
 
 			sg_free_table(msm_obj->sgt);
 			kfree(msm_obj->sgt);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

dma_map_sg() expects a DMA domain. However, the drm devices
have been traditionally using unmanaged iommu domain which
is non-dma type. Using dma mapping APIs with that domain is bad.

Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}()
to do the cache maintenance.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Suggested-by: Tomasz Figa <tfiga@chromium.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Jordan Crouse <jcrouse@codeaurora.org> 
Cc: Sean Paul <seanpaul@chromium.org>
---

Changes since v1:
 - Addressed Jordan's and Tomasz's comments for
   - moving sg dma addresses preparation out of the coditional
     check to the main path so we do it irrespective of whether
     the buffer is cached or uncached.
   - Enhance the comment to explain this dma addresses prepartion.

 drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 00c795ced02c..1811ac23a31c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
 		struct drm_device *dev = obj->dev;
 		struct page **p;
 		int npages = obj->size >> PAGE_SHIFT;
+		struct scatterlist *s;
+		int i;
 
 		if (use_pages(obj))
 			p = drm_gem_get_pages(obj);
@@ -104,12 +106,21 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			return ptr;
 		}
 
-		/* For non-cached buffers, ensure the new pages are clean
+		/*
+		 * dma_sync_sg_*() flush the physical pages, so point
+		 * sg->dma_address to the physical ones for the right behavior.
+		 */
+		for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
+			sg_dma_address(s) = sg_phys(s);
+
+		/*
+		 * For non-cached buffers, ensure the new pages are clean
 		 * because display controller, GPU, etc. are not coherent:
 		 */
-		if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-			dma_map_sg(dev->dev, msm_obj->sgt->sgl,
-					msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+		if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
+			dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
+					       msm_obj->sgt->nents,
+					       DMA_TO_DEVICE);
 	}
 
 	return msm_obj->pages;
@@ -133,14 +144,16 @@ static void put_pages(struct drm_gem_object *obj)
 
 	if (msm_obj->pages) {
 		if (msm_obj->sgt) {
-			/* For non-cached buffers, ensure the new
+			/*
+			 * For non-cached buffers, ensure the new
 			 * pages are clean because display controller,
 			 * GPU, etc. are not coherent:
 			 */
-			if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-				dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
-					     msm_obj->sgt->nents,
-					     DMA_BIDIRECTIONAL);
+			if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
+				dma_sync_sg_for_cpu(obj->dev->dev,
+						    msm_obj->sgt->sgl,
+						    msm_obj->sgt->nents,
+						    DMA_FROM_DEVICE);
 
 			sg_free_table(msm_obj->sgt);
 			kfree(msm_obj->sgt);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Tomasz Figa]

Hi Vivek,

On Tue, Nov 27, 2018 at 6:37 AM Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
>
> dma_map_sg() expects a DMA domain. However, the drm devices
> have been traditionally using unmanaged iommu domain which
> is non-dma type. Using dma mapping APIs with that domain is bad.
>
> Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}()
> to do the cache maintenance.
>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Suggested-by: Tomasz Figa <tfiga@chromium.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Jordan Crouse <jcrouse@codeaurora.org>
> Cc: Sean Paul <seanpaul@chromium.org>
> ---
>
> Changes since v1:
>  - Addressed Jordan's and Tomasz's comments for
>    - moving sg dma addresses preparation out of the coditional
>      check to the main path so we do it irrespective of whether
>      the buffer is cached or uncached.
>    - Enhance the comment to explain this dma addresses prepartion.
>

Thanks for the patch. Some comments inline.

>  drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 00c795ced02c..1811ac23a31c 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
>                 struct drm_device *dev = obj->dev;
>                 struct page **p;
>                 int npages = obj->size >> PAGE_SHIFT;
> +               struct scatterlist *s;
> +               int i;
>
>                 if (use_pages(obj))
>                         p = drm_gem_get_pages(obj);
> @@ -104,12 +106,21 @@ static struct page **get_pages(struct drm_gem_object *obj)
>                         return ptr;
>                 }
>
> -               /* For non-cached buffers, ensure the new pages are clean
> +               /*
> +                * dma_sync_sg_*() flush the physical pages, so point
> +                * sg->dma_address to the physical ones for the right behavior.

The two halves of the sequence don't really relate to each other. An
sglist has the `page` field for the purpose of pointing to physical
pages. The `dma_address` field is for DMA addresses, which are not
equivalent to physical addresses. I'd rewrite it like this;

/*
 * Some implementations of the DMA mapping ops expect
 * physical addresses of the pages to be stored as DMA
 * addresses of the sglist entries. To work around it,
 * set them here explicitly.
 */

> +                */
> +               for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> +                       sg_dma_address(s) = sg_phys(s);
> +
> +               /*
> +                * For non-cached buffers, ensure the new pages are clean
>                  * because display controller, GPU, etc. are not coherent:
>                  */
> -               if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> -                       dma_map_sg(dev->dev, msm_obj->sgt->sgl,
> -                                       msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
> +               if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
> +                       dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
> +                                              msm_obj->sgt->nents,
> +                                              DMA_TO_DEVICE);

Why changing from DMA_BIDIRECTIONAL?

>         }
>
>         return msm_obj->pages;
> @@ -133,14 +144,16 @@ static void put_pages(struct drm_gem_object *obj)
>
>         if (msm_obj->pages) {
>                 if (msm_obj->sgt) {
> -                       /* For non-cached buffers, ensure the new
> +                       /*
> +                        * For non-cached buffers, ensure the new
>                          * pages are clean because display controller,
>                          * GPU, etc. are not coherent:
>                          */
> -                       if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> -                               dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
> -                                            msm_obj->sgt->nents,
> -                                            DMA_BIDIRECTIONAL);
> +                       if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
> +                               dma_sync_sg_for_cpu(obj->dev->dev,
> +                                                   msm_obj->sgt->sgl,
> +                                                   msm_obj->sgt->nents,
> +                                                   DMA_FROM_DEVICE);

Ditto.

Best regards,
Tomasz
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Tomasz Figa]

Hi Vivek,

On Tue, Nov 27, 2018 at 6:37 AM Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
>
> dma_map_sg() expects a DMA domain. However, the drm devices
> have been traditionally using unmanaged iommu domain which
> is non-dma type. Using dma mapping APIs with that domain is bad.
>
> Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}()
> to do the cache maintenance.
>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Suggested-by: Tomasz Figa <tfiga@chromium.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Jordan Crouse <jcrouse@codeaurora.org>
> Cc: Sean Paul <seanpaul@chromium.org>
> ---
>
> Changes since v1:
>  - Addressed Jordan's and Tomasz's comments for
>    - moving sg dma addresses preparation out of the coditional
>      check to the main path so we do it irrespective of whether
>      the buffer is cached or uncached.
>    - Enhance the comment to explain this dma addresses prepartion.
>

Thanks for the patch. Some comments inline.

>  drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 00c795ced02c..1811ac23a31c 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
>                 struct drm_device *dev = obj->dev;
>                 struct page **p;
>                 int npages = obj->size >> PAGE_SHIFT;
> +               struct scatterlist *s;
> +               int i;
>
>                 if (use_pages(obj))
>                         p = drm_gem_get_pages(obj);
> @@ -104,12 +106,21 @@ static struct page **get_pages(struct drm_gem_object *obj)
>                         return ptr;
>                 }
>
> -               /* For non-cached buffers, ensure the new pages are clean
> +               /*
> +                * dma_sync_sg_*() flush the physical pages, so point
> +                * sg->dma_address to the physical ones for the right behavior.

The two halves of the sequence don't really relate to each other. An
sglist has the `page` field for the purpose of pointing to physical
pages. The `dma_address` field is for DMA addresses, which are not
equivalent to physical addresses. I'd rewrite it like this;

/*
 * Some implementations of the DMA mapping ops expect
 * physical addresses of the pages to be stored as DMA
 * addresses of the sglist entries. To work around it,
 * set them here explicitly.
 */

> +                */
> +               for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> +                       sg_dma_address(s) = sg_phys(s);
> +
> +               /*
> +                * For non-cached buffers, ensure the new pages are clean
>                  * because display controller, GPU, etc. are not coherent:
>                  */
> -               if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> -                       dma_map_sg(dev->dev, msm_obj->sgt->sgl,
> -                                       msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
> +               if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
> +                       dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
> +                                              msm_obj->sgt->nents,
> +                                              DMA_TO_DEVICE);

Why changing from DMA_BIDIRECTIONAL?

>         }
>
>         return msm_obj->pages;
> @@ -133,14 +144,16 @@ static void put_pages(struct drm_gem_object *obj)
>
>         if (msm_obj->pages) {
>                 if (msm_obj->sgt) {
> -                       /* For non-cached buffers, ensure the new
> +                       /*
> +                        * For non-cached buffers, ensure the new
>                          * pages are clean because display controller,
>                          * GPU, etc. are not coherent:
>                          */
> -                       if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> -                               dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
> -                                            msm_obj->sgt->nents,
> -                                            DMA_BIDIRECTIONAL);
> +                       if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
> +                               dma_sync_sg_for_cpu(obj->dev->dev,
> +                                                   msm_obj->sgt->sgl,
> +                                                   msm_obj->sgt->nents,
> +                                                   DMA_FROM_DEVICE);

Ditto.

Best regards,
Tomasz

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Christoph Hellwig]

> +		/*
> +		 * dma_sync_sg_*() flush the physical pages, so point
> +		 * sg->dma_address to the physical ones for the right behavior.
> +		 */
> +		for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> +			sg_dma_address(s) = sg_phys(s);
> +

I'm sorry, but this is completely bogus and not acceptable.

The only place that is allowed to initialize sg_dma_address is
dma_map_sg.  If the default dma ops don't work for your setup we have
major a problem and need to fix the dma api / iommu integration instead
of hacking around it.

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

Hi Christoph ,

On Wed, Nov 28, 2018 at 1:10 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +             /*
> > +              * dma_sync_sg_*() flush the physical pages, so point
> > +              * sg->dma_address to the physical ones for the right behavior.
> > +              */
> > +             for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +                     sg_dma_address(s) = sg_phys(s);
> > +
>
> I'm sorry, but this is completely bogus and not acceptable.
>
> The only place that is allowed to initialize sg_dma_address is
> dma_map_sg.  If the default dma ops don't work for your setup we have
> major a problem and need to fix the dma api / iommu integration instead
> of hacking around it.

Thanks for reviewing this change.
From what I understand the things in drm, we don't use the default
iommu-dma domain
for drm devices. Rather we allocate a new iommu domain, and therefore we can't
use the default dma ops. Hence we need separate dma_sync_sg*() to
flush/invalidate
the cache. For this we need to initialize the sg's addresses.

Please correct me if my understanding is incomplete.

Best regards
Vivek



--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

Hi Christoph ,

On Wed, Nov 28, 2018 at 1:10 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +             /*
> > +              * dma_sync_sg_*() flush the physical pages, so point
> > +              * sg->dma_address to the physical ones for the right behavior.
> > +              */
> > +             for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +                     sg_dma_address(s) = sg_phys(s);
> > +
>
> I'm sorry, but this is completely bogus and not acceptable.
>
> The only place that is allowed to initialize sg_dma_address is
> dma_map_sg.  If the default dma ops don't work for your setup we have
> major a problem and need to fix the dma api / iommu integration instead
> of hacking around it.

Thanks for reviewing this change.
From what I understand the things in drm, we don't use the default
iommu-dma domain
for drm devices. Rather we allocate a new iommu domain, and therefore we can't
use the default dma ops. Hence we need separate dma_sync_sg*() to
flush/invalidate
the cache. For this we need to initialize the sg's addresses.

Please correct me if my understanding is incomplete.

Best regards
Vivek



--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Rob Clark]

On Wed, Nov 28, 2018 at 2:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +             /*
> > +              * dma_sync_sg_*() flush the physical pages, so point
> > +              * sg->dma_address to the physical ones for the right behavior.
> > +              */
> > +             for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +                     sg_dma_address(s) = sg_phys(s);
> > +
>
> I'm sorry, but this is completely bogus and not acceptable.
>
> The only place that is allowed to initialize sg_dma_address is
> dma_map_sg.  If the default dma ops don't work for your setup we have
> major a problem and need to fix the dma api / iommu integration instead
> of hacking around it.

I agree that the dma/iommu integration is very problematic for drm (in
particular, gpu drivers that use the iommu as an gpu mmu)..  Really we
need a way that a driver can opt-out of this, and access the cpu cache
APIs directly, skipping the dma API entirely.  But as it is, we've had
to hack around the dma API.  I'm not really sure this hack is any
worse than abusing dma_(un)map_sg() for doing cache operations.

I probably should have paid more attention and nak'd the dma/iommu
integration before it landed.  But given that now we are stuck in this
situation, while I'm certainly interested if anyone has some ideas
about how to let drivers opt out of the dma/iommu integration and
bypass the dma API layer, I'm ok with replacing a hack with a less-bad
hack.

BR,
-R
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Rob Clark]

On Wed, Nov 28, 2018 at 2:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +             /*
> > +              * dma_sync_sg_*() flush the physical pages, so point
> > +              * sg->dma_address to the physical ones for the right behavior.
> > +              */
> > +             for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +                     sg_dma_address(s) = sg_phys(s);
> > +
>
> I'm sorry, but this is completely bogus and not acceptable.
>
> The only place that is allowed to initialize sg_dma_address is
> dma_map_sg.  If the default dma ops don't work for your setup we have
> major a problem and need to fix the dma api / iommu integration instead
> of hacking around it.

I agree that the dma/iommu integration is very problematic for drm (in
particular, gpu drivers that use the iommu as an gpu mmu)..  Really we
need a way that a driver can opt-out of this, and access the cpu cache
APIs directly, skipping the dma API entirely.  But as it is, we've had
to hack around the dma API.  I'm not really sure this hack is any
worse than abusing dma_(un)map_sg() for doing cache operations.

I probably should have paid more attention and nak'd the dma/iommu
integration before it landed.  But given that now we are stuck in this
situation, while I'm certainly interested if anyone has some ideas
about how to let drivers opt out of the dma/iommu integration and
bypass the dma API layer, I'm ok with replacing a hack with a less-bad
hack.

BR,
-R

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Lukas Wunner]

On Tue, Nov 27, 2018 at 11:39:40PM -0800, Christoph Hellwig wrote:
> > +		/*
> > +		 * dma_sync_sg_*() flush the physical pages, so point
> > +		 * sg->dma_address to the physical ones for the right behavior.
> > +		 */
> > +		for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +			sg_dma_address(s) = sg_phys(s);
> > +
> 
> I'm sorry, but this is completely bogus and not acceptable.
> 
> The only place that is allowed to initialize sg_dma_address is
> dma_map_sg.  If the default dma ops don't work for your setup we have
> major a problem and need to fix the dma api / iommu integration instead
> of hacking around it.

Somewhat related, I recently submitted a patch for the spi-bcm2835.c
driver to overcome a limitation of the DMA engine and the spi0 master
on the Raspberry Pi:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2018-November/008275.html

If a buffer needs to be sent or received which was vmalloc'ed (hence is
not contiguous in physical memory) and starts in the middle of a page
with an offset not a multiple of 4, the buffer could not be transmitted
with DMA so far because the spi0 master requires 4 byte writes to the
FIFO and the DMA engine is incapable of combining multiple sglist
entries of arbitrary length into a continuous stream of 4 byte chunks.

The solution I've found is to transfer the first few bytes of the first
sglist entry with programmed I/O such that the remainder has a length
which is a multiple of 4.  For this to work, I have to massage the first
sglist entry, i.e. I have to add the number of bytes transmitted with
PIO to the DMA address and subtract it from the sglist entry's length.
After the DMA engine has done its job, I undo those changes to the
sglist.

Could you comment on the acceptibility of this approach?  If you do not
deem it acceptable, could you make a suggestion how the dma-mapping API
shall be amended to support such use cases?

(I sure hope the approach *is* acceptable, getting this to work at all
required endless fiddling.  The BCM2835 is in some ways a not so great
SoC and could do with better documentation, to put it mildly.)

(Note: I've since amended the above-linked patch to sync only the modified
cachelines back to memory, not the full first sglist entry.  I've also
switched to the sg_dma_address() and sg_dma_len() syntax instead of
referencing the sglist entry members directly, this appears to be more
common in the tree even for l-values.  I was going to post these changes
as v2 shortly.)

Thanks,

Lukas

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

Hi Tomasz,

On Wed, Nov 28, 2018 at 8:39 AM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Vivek,
>
> On Tue, Nov 27, 2018 at 6:37 AM Vivek Gautam
> <vivek.gautam@codeaurora.org> wrote:
> >
> > dma_map_sg() expects a DMA domain. However, the drm devices
> > have been traditionally using unmanaged iommu domain which
> > is non-dma type. Using dma mapping APIs with that domain is bad.
> >
> > Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}()
> > to do the cache maintenance.
> >
> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Suggested-by: Tomasz Figa <tfiga@chromium.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Jordan Crouse <jcrouse@codeaurora.org>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > ---
> >
> > Changes since v1:
> >  - Addressed Jordan's and Tomasz's comments for
> >    - moving sg dma addresses preparation out of the coditional
> >      check to the main path so we do it irrespective of whether
> >      the buffer is cached or uncached.
> >    - Enhance the comment to explain this dma addresses prepartion.
> >
>
> Thanks for the patch. Some comments inline.
>
> >  drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++---------
> >  1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > index 00c795ced02c..1811ac23a31c 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > @@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
> >                 struct drm_device *dev = obj->dev;
> >                 struct page **p;
> >                 int npages = obj->size >> PAGE_SHIFT;
> > +               struct scatterlist *s;
> > +               int i;
> >
> >                 if (use_pages(obj))
> >                         p = drm_gem_get_pages(obj);
> > @@ -104,12 +106,21 @@ static struct page **get_pages(struct drm_gem_object *obj)
> >                         return ptr;
> >                 }
> >
> > -               /* For non-cached buffers, ensure the new pages are clean
> > +               /*
> > +                * dma_sync_sg_*() flush the physical pages, so point
> > +                * sg->dma_address to the physical ones for the right behavior.
>
> The two halves of the sequence don't really relate to each other. An
> sglist has the `page` field for the purpose of pointing to physical
> pages. The `dma_address` field is for DMA addresses, which are not
> equivalent to physical addresses. I'd rewrite it like this;

I guess I was lenient in using the physical pages and dma address words.

>
> /*
>  * Some implementations of the DMA mapping ops expect
>  * physical addresses of the pages to be stored as DMA
>  * addresses of the sglist entries. To work around it,
>  * set them here explicitly.
>  */

Will update as suggested.

> > +                */
> > +               for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +                       sg_dma_address(s) = sg_phys(s);
> > +
> > +               /*
> > +                * For non-cached buffers, ensure the new pages are clean
> >                  * because display controller, GPU, etc. are not coherent:
> >                  */
> > -               if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> > -                       dma_map_sg(dev->dev, msm_obj->sgt->sgl,
> > -                                       msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
> > +               if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
> > +                       dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
> > +                                              msm_obj->sgt->nents,
> > +                                              DMA_TO_DEVICE);
>
> Why changing from DMA_BIDIRECTIONAL?

Yea, I went back and checked that we wanted to do this for both.
Will keep DMA_BIDIRECTIONAL intact.

>
> >         }
> >
> >         return msm_obj->pages;
> > @@ -133,14 +144,16 @@ static void put_pages(struct drm_gem_object *obj)
> >
> >         if (msm_obj->pages) {
> >                 if (msm_obj->sgt) {
> > -                       /* For non-cached buffers, ensure the new
> > +                       /*
> > +                        * For non-cached buffers, ensure the new
> >                          * pages are clean because display controller,
> >                          * GPU, etc. are not coherent:
> >                          */
> > -                       if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> > -                               dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
> > -                                            msm_obj->sgt->nents,
> > -                                            DMA_BIDIRECTIONAL);
> > +                       if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED))
> > +                               dma_sync_sg_for_cpu(obj->dev->dev,
> > +                                                   msm_obj->sgt->sgl,
> > +                                                   msm_obj->sgt->nents,
> > +                                                   DMA_FROM_DEVICE);
>
> Ditto.

Sure, will change this as well.

Thanks & Regards
Vivek

>
> Best regards,
> Tomasz



--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

On Wed, Nov 28, 2018 at 6:09 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Wed, Nov 28, 2018 at 2:39 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > > +             /*
> > > +              * dma_sync_sg_*() flush the physical pages, so point
> > > +              * sg->dma_address to the physical ones for the right behavior.
> > > +              */
> > > +             for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > > +                     sg_dma_address(s) = sg_phys(s);
> > > +
> >
> > I'm sorry, but this is completely bogus and not acceptable.
> >
> > The only place that is allowed to initialize sg_dma_address is
> > dma_map_sg.  If the default dma ops don't work for your setup we have
> > major a problem and need to fix the dma api / iommu integration instead
> > of hacking around it.
>
> I agree that the dma/iommu integration is very problematic for drm (in
> particular, gpu drivers that use the iommu as an gpu mmu)..  Really we
> need a way that a driver can opt-out of this, and access the cpu cache
> APIs directly, skipping the dma API entirely.  But as it is, we've had
> to hack around the dma API.  I'm not really sure this hack is any
> worse than abusing dma_(un)map_sg() for doing cache operations.
>
> I probably should have paid more attention and nak'd the dma/iommu
> integration before it landed.  But given that now we are stuck in this
> situation, while I'm certainly interested if anyone has some ideas
> about how to let drivers opt out of the dma/iommu integration and
> bypass the dma API layer, I'm ok with replacing a hack with a less-bad
> hack.

May I take it as a positive nod to respin the next version?

Regards
Vivek

>
> BR,
> -R



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Vivek Gautam]

On Wed, Nov 28, 2018 at 6:09 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Wed, Nov 28, 2018 at 2:39 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > > +             /*
> > > +              * dma_sync_sg_*() flush the physical pages, so point
> > > +              * sg->dma_address to the physical ones for the right behavior.
> > > +              */
> > > +             for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > > +                     sg_dma_address(s) = sg_phys(s);
> > > +
> >
> > I'm sorry, but this is completely bogus and not acceptable.
> >
> > The only place that is allowed to initialize sg_dma_address is
> > dma_map_sg.  If the default dma ops don't work for your setup we have
> > major a problem and need to fix the dma api / iommu integration instead
> > of hacking around it.
>
> I agree that the dma/iommu integration is very problematic for drm (in
> particular, gpu drivers that use the iommu as an gpu mmu)..  Really we
> need a way that a driver can opt-out of this, and access the cpu cache
> APIs directly, skipping the dma API entirely.  But as it is, we've had
> to hack around the dma API.  I'm not really sure this hack is any
> worse than abusing dma_(un)map_sg() for doing cache operations.
>
> I probably should have paid more attention and nak'd the dma/iommu
> integration before it landed.  But given that now we are stuck in this
> situation, while I'm certainly interested if anyone has some ideas
> about how to let drivers opt out of the dma/iommu integration and
> bypass the dma API layer, I'm ok with replacing a hack with a less-bad
> hack.

May I take it as a positive nod to respin the next version?

Regards
Vivek

>
> BR,
> -R



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation