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

Submitted by Vivek Gautam on Nov. 20, 2018, 9:54 a.m.

Details

Message ID 20181120095437.29820-1-vivek.gautam@codeaurora.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Vivek Gautam Nov. 20, 2018, 9:54 a.m.
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>
---

Tested on an MTP sdm845:
https://github.com/vivekgautam1/linux/tree/v4.19/sdm845-mtp-display-working

 drivers/gpu/drm/msm/msm_gem.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 00c795ced02c..d7a7af610803 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);
@@ -107,9 +109,19 @@  static struct page **get_pages(struct drm_gem_object *obj)
 		/* 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)) {
+			/*
+			 * Fake up the SG table so that dma_sync_sg_*()
+			 * can be used to flush the pages associated with it.
+			 */
+			for_each_sg(msm_obj->sgt->sgl, s,
+				    msm_obj->sgt->nents, i)
+				sg_dma_address(s) = sg_phys(s);
+
+			dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
+					       msm_obj->sgt->nents,
+					       DMA_TO_DEVICE);
+		}
 	}
 
 	return msm_obj->pages;
@@ -137,10 +149,11 @@  static void put_pages(struct drm_gem_object *obj)
 			 * 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);

Comments

On Tue, Nov 20, 2018 at 03:24:37PM +0530, Vivek Gautam 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>
> ---
> 
> Tested on an MTP sdm845:
> https://github.com/vivekgautam1/linux/tree/v4.19/sdm845-mtp-display-working
> 
>  drivers/gpu/drm/msm/msm_gem.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 00c795ced02c..d7a7af610803 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);
> @@ -107,9 +109,19 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  		/* 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)) {
> +			/*
> +			 * Fake up the SG table so that dma_sync_sg_*()
> +			 * can be used to flush the pages associated with it.
> +			 */

We aren't really faking.  The table is real, we are just slightly abusing the
sg_dma_address() which makes this comment a bit misleading. Instead I would
probably say something like:

/* dma_sync_sg_* flushes pages using sg_dma_address() so point it at the
 * physical page for the right behavior */

Or something like that.

> +			for_each_sg(msm_obj->sgt->sgl, s,
> +				    msm_obj->sgt->nents, i)
> +				sg_dma_address(s) = sg_phys(s);
> +

I'm wondering - wouldn't we want to do this association for cached buffers to so
we could sync them correctly in cpu_prep and cpu_fini?  Maybe it wouldn't hurt
to put this association in the main path (obviously the sync should stay inside
the conditional for uncached buffers).

> +			dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl,
> +					       msm_obj->sgt->nents,
> +					       DMA_TO_DEVICE);
> +		}
>  	}
>  
>  	return msm_obj->pages;
> @@ -137,10 +149,11 @@ static void put_pages(struct drm_gem_object *obj)
>  			 * 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);
Hi Jordan, Vivek,

On Wed, Nov 21, 2018 at 12:41 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> On Tue, Nov 20, 2018 at 03:24:37PM +0530, Vivek Gautam 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>
> > ---
> >
> > Tested on an MTP sdm845:
> > https://github.com/vivekgautam1/linux/tree/v4.19/sdm845-mtp-display-working
> >
> >  drivers/gpu/drm/msm/msm_gem.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > index 00c795ced02c..d7a7af610803 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);
> > @@ -107,9 +109,19 @@ static struct page **get_pages(struct drm_gem_object *obj)
> >               /* 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)) {
> > +                     /*
> > +                      * Fake up the SG table so that dma_sync_sg_*()
> > +                      * can be used to flush the pages associated with it.
> > +                      */
>
> We aren't really faking.  The table is real, we are just slightly abusing the
> sg_dma_address() which makes this comment a bit misleading. Instead I would
> probably say something like:
>
> /* dma_sync_sg_* flushes pages using sg_dma_address() so point it at the
>  * physical page for the right behavior */
>
> Or something like that.
>

It's actually quite complicated, but I agree that the comment isn't
very precise. The cases are as follows:
- arm64 iommu_dma_ops use sg_phys()
https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm64/mm/dma-mapping.c#L599
- swiotlb_dma_ops used on arm64 if no IOMMU is available use
sg->dma_address directly:
https://elixir.bootlin.com/linux/v4.20-rc3/source/kernel/dma/swiotlb.c#L832
- arm_dma_ops use sg_dma_address():
https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1130
- arm iommu_ops use sg_page():
https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1869

Sounds like a mess...

> > +                     for_each_sg(msm_obj->sgt->sgl, s,
> > +                                 msm_obj->sgt->nents, i)
> > +                             sg_dma_address(s) = sg_phys(s);
> > +
>
> I'm wondering - wouldn't we want to do this association for cached buffers to so
> we could sync them correctly in cpu_prep and cpu_fini?  Maybe it wouldn't hurt
> to put this association in the main path (obviously the sync should stay inside
> the conditional for uncached buffers).
>

I guess it wouldn't hurt indeed. Note that cpu_prep/fini seem to be
missing the sync call currently.

P.S. Jordan, not sure if it's my Gmail or your email client, but your
message had all the recipients in a Reply-to header, except you, so
pressing Reply to all in my case led to a message that didn't have you
in recipients anymore...

Best regards,
Tomasz
Hi Tomasz, Jordan,


On 11/21/2018 9:18 AM, Tomasz Figa wrote:
> Hi Jordan, Vivek,
>
> On Wed, Nov 21, 2018 at 12:41 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>> On Tue, Nov 20, 2018 at 03:24:37PM +0530, Vivek Gautam 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>
>>> ---
>>>
>>> Tested on an MTP sdm845:
>>> https://github.com/vivekgautam1/linux/tree/v4.19/sdm845-mtp-display-working
>>>
>>>   drivers/gpu/drm/msm/msm_gem.c | 27 ++++++++++++++++++++-------
>>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>>> index 00c795ced02c..d7a7af610803 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);
>>> @@ -107,9 +109,19 @@ static struct page **get_pages(struct drm_gem_object *obj)
>>>                /* 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)) {
>>> +                     /*
>>> +                      * Fake up the SG table so that dma_sync_sg_*()
>>> +                      * can be used to flush the pages associated with it.
>>> +                      */
>> We aren't really faking.  The table is real, we are just slightly abusing the
>> sg_dma_address() which makes this comment a bit misleading. Instead I would
>> probably say something like:
>>
>> /* dma_sync_sg_* flushes pages using sg_dma_address() so point it at the
>>   * physical page for the right behavior */
>>
>> Or something like that.
>>
> It's actually quite complicated, but I agree that the comment isn't
> very precise. The cases are as follows:
> - arm64 iommu_dma_ops use sg_phys()
> https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm64/mm/dma-mapping.c#L599
> - swiotlb_dma_ops used on arm64 if no IOMMU is available use
> sg->dma_address directly:
> https://elixir.bootlin.com/linux/v4.20-rc3/source/kernel/dma/swiotlb.c#L832
> - arm_dma_ops use sg_dma_address():
> https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1130
> - arm iommu_ops use sg_page():
> https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1869
>
> Sounds like a mess...
Thanks for the review.

Technically with the below assignment we address all of the above. How 
about an even
simpler version of the suggested comment:

/* dma_sync_sg_* flushes physical pages, so point sg->dma_address to
  * the physical one 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 wondering - wouldn't we want to do this association for cached buffers to so
>> we could sync them correctly in cpu_prep and cpu_fini?  Maybe it wouldn't hurt
>> to put this association in the main path (obviously the sync should stay inside
>> the conditional for uncached buffers).
>>

Sure, I will move this out of the conditional check.

> I guess it wouldn't hurt indeed. Note that cpu_prep/fini seem to be
> missing the sync call currently.

I can't say I understand the usage of cpu_prep and cpu_fini(). But I can add
the necessary support if you can point me in the right direction.
Thanks

Best regards
Vivek
>
> P.S. Jordan, not sure if it's my Gmail or your email client, but your
> message had all the recipients in a Reply-to header, except you, so
> pressing Reply to all in my case led to a message that didn't have you
> in recipients anymore...
>
> Best regards,
> Tomasz
On Thu, Nov 22, 2018 at 03:37:54PM +0530, Vivek Gautam wrote:
> Hi Tomasz, Jordan,
> 
> 
> On 11/21/2018 9:18 AM, Tomasz Figa wrote:
 
> >
> >>>+                     for_each_sg(msm_obj->sgt->sgl, s,
> >>>+                                 msm_obj->sgt->nents, i)
> >>>+                             sg_dma_address(s) = sg_phys(s);
> >>>+
> >>I'm wondering - wouldn't we want to do this association for cached buffers to so
> >>we could sync them correctly in cpu_prep and cpu_fini?  Maybe it wouldn't hurt
> >>to put this association in the main path (obviously the sync should stay inside
> >>the conditional for uncached buffers).
> >>
> 
> Sure, I will move this out of the conditional check.
> 
> >I guess it wouldn't hurt indeed. Note that cpu_prep/fini seem to be
> >missing the sync call currently.
> 
> I can't say I understand the usage of cpu_prep and cpu_fini(). But I can add
> the necessary support if you can point me in the right direction.

Not needed for this iteration. We don't have support in those functions for
cached buffers right now so continuing to not support it wouldn't hurt.

Jordan