[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Tiffany Lin]

vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
But in dma_sync_sg_for_device, it use lengths of each SG entries
before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
SGs until dma length > dma seg bundary. sgt->nents will less than
sgt->orig_nents. Using SG entries after dma_map_sg_attrs
in vb2_dc_prepare will make some SGs are not sync to device.
After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
sync data to device twice. Device randomly get incorrect data because
some SGs are not sync to device. Change to use number of SG entries
before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.

Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 2397ceb..c5d00bd 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
 	if (!sgt || buf->db_attach)
 		return;
 
-	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
 }
 
 static void vb2_dc_finish(void *buf_priv)
@@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
 	if (!sgt || buf->db_attach)
 		return;
 
-	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
 }
 
 /*********************************************/
-- 
1.8.1.1.dirty

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Hans Verkuil]

Hi Tiffany!

On 21-09-15 14:26, Tiffany Lin wrote:
> vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> But in dma_sync_sg_for_device, it use lengths of each SG entries
> before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> SGs until dma length > dma seg bundary. sgt->nents will less than
> sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> in vb2_dc_prepare will make some SGs are not sync to device.
> After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> sync data to device twice. Device randomly get incorrect data because
> some SGs are not sync to device. Change to use number of SG entries
> before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> 
> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 2397ceb..c5d00bd 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
>  	if (!sgt || buf->db_attach)
>  		return;
>  
> -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>  }
>  
>  static void vb2_dc_finish(void *buf_priv)
> @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
>  	if (!sgt || buf->db_attach)
>  		return;
>  
> -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>  }

I don't really understand it. I am assuming that this happens on an arm and that
the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.

Now, as I understand it (and my understanding may very well be flawed!) the map_sg
function concatenates SG entries if possible, so it may return fewer entries. But
the dma_sync_sg functions use those updated SG entries, so the full buffer should
be covered by this. Using orig_nents will actually sync parts of the buffer twice!
The first nents entries already cover the full buffer so any remaining entries up
to orig_nents will just duplicate parts of the buffer.

So this patch makes no sense in the current code.

If I understand your log text correctly this patch goes on top of Sakari Ailus' vb2
sync patch series. So if it wasn't needed before, but it is needed after his patch
series, then the problem is in that patch series.

In any case, I need some help understanding this patch.

And *if* this patch is correct, then the same thing should likely be done for
videobuf2-dma-sg.c.

Regards,

	Hans

>  
>  /*********************************************/
> 

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[tiffany lin]

Hi Hans,

On Mon, 2015-09-21 at 15:13 +0200, Hans Verkuil wrote:
> Hi Tiffany!
> 
> On 21-09-15 14:26, Tiffany Lin wrote:
> > vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> > But in dma_sync_sg_for_device, it use lengths of each SG entries
> > before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> > SGs until dma length > dma seg bundary. sgt->nents will less than
> > sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> > in vb2_dc_prepare will make some SGs are not sync to device.
> > After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> > sync data to device twice. Device randomly get incorrect data because
> > some SGs are not sync to device. Change to use number of SG entries
> > before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> > 
> > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > index 2397ceb..c5d00bd 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
> >  	if (!sgt || buf->db_attach)
> >  		return;
> >  
> > -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> >  }
> >  
> >  static void vb2_dc_finish(void *buf_priv)
> > @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
> >  	if (!sgt || buf->db_attach)
> >  		return;
> >  
> > -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> >  }
> 
> I don't really understand it. I am assuming that this happens on an arm and that
> the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
> arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.
> 

We are using __iommu_* implemented in "arch/arm64/mm/dma-mapping.c" in
review patch
http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013898.html
Without patch "[media] vb2: use dma_map_sg_attrs to prevent unnecessary
sync", vb2 will sync data to device twice.
One is from "dma_map_sg" in "vb2_dc_get_userptr", the other is from
"dma_sync_sg_for_device" in "vb2_dc_prepare". dma_map_sg use orig_nents,
and dma_sync_sg_for_device use nents."

We do not run in 32bits mode, but check "arm_dma_sync_sg_for_device" in
"arch/arm/mm/dma-mapping.c", 
ops->sync_single_for_device(dev, sg_dma_address(s), s->length, dir);
It looks like has same issue.

> Now, as I understand it (and my understanding may very well be flawed!) the map_sg
> function concatenates SG entries if possible, so it may return fewer entries. But
> the dma_sync_sg functions use those updated SG entries, so the full buffer should
> be covered by this. Using orig_nents will actually sync parts of the buffer twice!
> The first nents entries already cover the full buffer so any remaining entries up
> to orig_nents will just duplicate parts of the buffer.
> 
I found that in __iommu_sync_sg_for_device, it use sg->length , do not
cover full buffer.
By adding log in " __iommu_sync_sg_for_device" without patch "[media]
vb2: use dma_map_sg_attrs to prevent unnecessary sync", we could see
total synced size are different between called from dma_map_sg and
dma_sync_sg_for_device.
__iommu_sync_sg_for_device called from dma_sync_sg_for_device got
updated SG entries number but it use un-updated sg length.
After using "DMA_ATTR_SKIP_CPU_SYNC" to skip sync in vb2_dc_get_userptr,
we got some part of the buffer not sync.

> So this patch makes no sense in the current code.
> 
> If I understand your log text correctly this patch goes on top of Sakari Ailus' vb2
> sync patch series. So if it wasn't needed before, but it is needed after his patch
> series, then the problem is in that patch series.
> 
This patch goes on top of these two patchs
https://www.mail-archive.com/linux-media%40vger.kernel.org/msg82143.html
http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013898.html


> In any case, I need some help understanding this patch.
> 
> And *if* this patch is correct, then the same thing should likely be done for
> videobuf2-dma-sg.c.
> 
Yes, if this patch correct, same thing should be done for
videobuf2-dma-sg.c
> Regards,
> 
> 	Hans
> 
> >  
> >  /*********************************************/
> > 

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Sakari Ailus]

Hi Tiffany,

On Tue, Sep 22, 2015 at 06:19:25PM +0800, tiffany lin wrote:
> Hi Hans,
> 
> On Mon, 2015-09-21 at 15:13 +0200, Hans Verkuil wrote:
> > Hi Tiffany!
> > 
> > On 21-09-15 14:26, Tiffany Lin wrote:
> > > vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> > > But in dma_sync_sg_for_device, it use lengths of each SG entries
> > > before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> > > SGs until dma length > dma seg bundary. sgt->nents will less than
> > > sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> > > in vb2_dc_prepare will make some SGs are not sync to device.
> > > After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> > > sync data to device twice. Device randomly get incorrect data because
> > > some SGs are not sync to device. Change to use number of SG entries
> > > before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> > > 
> > > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > > ---
> > >  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > index 2397ceb..c5d00bd 100644
> > > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
> > >  	if (!sgt || buf->db_attach)
> > >  		return;
> > >  
> > > -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > > +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> > >  }
> > >  
> > >  static void vb2_dc_finish(void *buf_priv)
> > > @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
> > >  	if (!sgt || buf->db_attach)
> > >  		return;
> > >  
> > > -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > > +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> > >  }
> > 
> > I don't really understand it. I am assuming that this happens on an arm and that
> > the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
> > arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.
> > 
> 
> We are using __iommu_* implemented in "arch/arm64/mm/dma-mapping.c" in
> review patch
> http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013898.html
> Without patch "[media] vb2: use dma_map_sg_attrs to prevent unnecessary
> sync", vb2 will sync data to device twice.
> One is from "dma_map_sg" in "vb2_dc_get_userptr", the other is from
> "dma_sync_sg_for_device" in "vb2_dc_prepare". dma_map_sg use orig_nents,
> and dma_sync_sg_for_device use nents."
> 
> We do not run in 32bits mode, but check "arm_dma_sync_sg_for_device" in
> "arch/arm/mm/dma-mapping.c", 
> ops->sync_single_for_device(dev, sg_dma_address(s), s->length, dir);
> It looks like has same issue.
> 
> > Now, as I understand it (and my understanding may very well be flawed!) the map_sg
> > function concatenates SG entries if possible, so it may return fewer entries. But
> > the dma_sync_sg functions use those updated SG entries, so the full buffer should
> > be covered by this. Using orig_nents will actually sync parts of the buffer twice!
> > The first nents entries already cover the full buffer so any remaining entries up
> > to orig_nents will just duplicate parts of the buffer.
> > 
> I found that in __iommu_sync_sg_for_device, it use sg->length , do not
> cover full buffer.
> By adding log in " __iommu_sync_sg_for_device" without patch "[media]
> vb2: use dma_map_sg_attrs to prevent unnecessary sync", we could see
> total synced size are different between called from dma_map_sg and
> dma_sync_sg_for_device.

I had the same question Hans did, but I still fail to understand where in
the code things are going wrong the way you described at the moment ---
after dma_map_sg() there are nents entries in the scatterlist. But.
sg_dma_len() should be used instead of the length field to get the size of
the entry. If something is wrong, then it's this AFAICT.

Could you try whether changing this fixes it?

> __iommu_sync_sg_for_device called from dma_sync_sg_for_device got
> updated SG entries number but it use un-updated sg length.
> After using "DMA_ATTR_SKIP_CPU_SYNC" to skip sync in vb2_dc_get_userptr,
> we got some part of the buffer not sync.
> 
> > So this patch makes no sense in the current code.
> > 
> > If I understand your log text correctly this patch goes on top of Sakari Ailus' vb2
> > sync patch series. So if it wasn't needed before, but it is needed after his patch
> > series, then the problem is in that patch series.
> > 
> This patch goes on top of these two patchs
> https://www.mail-archive.com/linux-media%40vger.kernel.org/msg82143.html

This patch has been merged long time ago.

> http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013898.html
> 
> 
> > In any case, I need some help understanding this patch.
> > 
> > And *if* this patch is correct, then the same thing should likely be done for
> > videobuf2-dma-sg.c.
> > 
> Yes, if this patch correct, same thing should be done for
> videobuf2-dma-sg.c
> > Regards,
> > 
> > 	Hans
> > 
> > >  
> > >  /*********************************************/
> > > 
> 
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[tiffany lin]

Hi Sakari,

On Tue, 2015-09-22 at 15:07 +0300, Sakari Ailus wrote:
> Hi Tiffany,
> 
> On Tue, Sep 22, 2015 at 06:19:25PM +0800, tiffany lin wrote:
> > Hi Hans,
> > 
> > On Mon, 2015-09-21 at 15:13 +0200, Hans Verkuil wrote:
> > > Hi Tiffany!
> > > 
> > > On 21-09-15 14:26, Tiffany Lin wrote:
> > > > vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> > > > But in dma_sync_sg_for_device, it use lengths of each SG entries
> > > > before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> > > > SGs until dma length > dma seg bundary. sgt->nents will less than
> > > > sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> > > > in vb2_dc_prepare will make some SGs are not sync to device.
> > > > After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> > > > sync data to device twice. Device randomly get incorrect data because
> > > > some SGs are not sync to device. Change to use number of SG entries
> > > > before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> > > > 
> > > > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > > > ---
> > > >  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > > index 2397ceb..c5d00bd 100644
> > > > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > > > @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
> > > >  	if (!sgt || buf->db_attach)
> > > >  		return;
> > > >  
> > > > -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > > > +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> > > >  }
> > > >  
> > > >  static void vb2_dc_finish(void *buf_priv)
> > > > @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
> > > >  	if (!sgt || buf->db_attach)
> > > >  		return;
> > > >  
> > > > -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > > > +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> > > >  }
> > > 
> > > I don't really understand it. I am assuming that this happens on an arm and that
> > > the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
> > > arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.
> > > 
> > 
> > We are using __iommu_* implemented in "arch/arm64/mm/dma-mapping.c" in
> > review patch
> > http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013898.html
> > Without patch "[media] vb2: use dma_map_sg_attrs to prevent unnecessary
> > sync", vb2 will sync data to device twice.
> > One is from "dma_map_sg" in "vb2_dc_get_userptr", the other is from
> > "dma_sync_sg_for_device" in "vb2_dc_prepare". dma_map_sg use orig_nents,
> > and dma_sync_sg_for_device use nents."
> > 
> > We do not run in 32bits mode, but check "arm_dma_sync_sg_for_device" in
> > "arch/arm/mm/dma-mapping.c", 
> > ops->sync_single_for_device(dev, sg_dma_address(s), s->length, dir);
> > It looks like has same issue.
> > 
> > > Now, as I understand it (and my understanding may very well be flawed!) the map_sg
> > > function concatenates SG entries if possible, so it may return fewer entries. But
> > > the dma_sync_sg functions use those updated SG entries, so the full buffer should
> > > be covered by this. Using orig_nents will actually sync parts of the buffer twice!
> > > The first nents entries already cover the full buffer so any remaining entries up
> > > to orig_nents will just duplicate parts of the buffer.
> > > 
> > I found that in __iommu_sync_sg_for_device, it use sg->length , do not
> > cover full buffer.
> > By adding log in " __iommu_sync_sg_for_device" without patch "[media]
> > vb2: use dma_map_sg_attrs to prevent unnecessary sync", we could see
> > total synced size are different between called from dma_map_sg and
> > dma_sync_sg_for_device.
> 
> I had the same question Hans did, but I still fail to understand where in
> the code things are going wrong the way you described at the moment ---
> after dma_map_sg() there are nents entries in the scatterlist. But.
> sg_dma_len() should be used instead of the length field to get the size of
> the entry. If something is wrong, then it's this AFAICT.
> 
> Could you try whether changing this fixes it?
> 
Do you mean try to change to use sg_dma_len in
__iommu_sync_sg_for_device?

I tried to change using sg_dam_len() in __iommu_sync_sg_for_device
	for_each_sg(sgl, sg, nelems, i) {
		//__dma_map_area(sg_virt(sg), sg->length, dir);
		__dma_map_area(sg_virt(sg), sg_dma_len(sg), dir);
	}

I still see the issue. Probably there are some other issue in
iommu_dma_map_sg.

The __iommu_sync_sg_for_device could be called before and after
iommu_dma_map_sg called.
I am not sure whether there is any side effect change it to
sg_dma_len(sg)

> > __iommu_sync_sg_for_device called from dma_sync_sg_for_device got
> > updated SG entries number but it use un-updated sg length.
> > After using "DMA_ATTR_SKIP_CPU_SYNC" to skip sync in vb2_dc_get_userptr,
> > we got some part of the buffer not sync.
> > 
> > > So this patch makes no sense in the current code.
> > > 
> > > If I understand your log text correctly this patch goes on top of Sakari Ailus' vb2
> > > sync patch series. So if it wasn't needed before, but it is needed after his patch
> > > series, then the problem is in that patch series.
> > > 
> > This patch goes on top of these two patchs
> > https://www.mail-archive.com/linux-media%40vger.kernel.org/msg82143.html
> 
> This patch has been merged long time ago.
> 
> > http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013898.html
> > 
> > 
> > > In any case, I need some help understanding this patch.
> > > 
> > > And *if* this patch is correct, then the same thing should likely be done for
> > > videobuf2-dma-sg.c.
> > > 
> > Yes, if this patch correct, same thing should be done for
> > videobuf2-dma-sg.c
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > >  
> > > >  /*********************************************/
> > > > 
> > 
> > 
> 

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Robin Murphy]

Hi Hans,

On 21/09/15 14:13, Hans Verkuil wrote:
> Hi Tiffany!
>
> On 21-09-15 14:26, Tiffany Lin wrote:
>> vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
>> But in dma_sync_sg_for_device, it use lengths of each SG entries
>> before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
>> SGs until dma length > dma seg bundary. sgt->nents will less than
>> sgt->orig_nents. Using SG entries after dma_map_sg_attrs
>> in vb2_dc_prepare will make some SGs are not sync to device.
>> After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
>> sync data to device twice. Device randomly get incorrect data because
>> some SGs are not sync to device. Change to use number of SG entries
>> before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
>>
>> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
>> ---
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> index 2397ceb..c5d00bd 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
>>   	if (!sgt || buf->db_attach)
>>   		return;
>>
>> -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>> +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>>   }
>>
>>   static void vb2_dc_finish(void *buf_priv)
>> @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
>>   	if (!sgt || buf->db_attach)
>>   		return;
>>
>> -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>> +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>>   }
>
> I don't really understand it. I am assuming that this happens on an arm and that
> the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
> arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.
>
> Now, as I understand it (and my understanding may very well be flawed!) the map_sg
> function concatenates SG entries if possible, so it may return fewer entries. But
> the dma_sync_sg functions use those updated SG entries, so the full buffer should
> be covered by this. Using orig_nents will actually sync parts of the buffer twice!
> The first nents entries already cover the full buffer so any remaining entries up
> to orig_nents will just duplicate parts of the buffer.

As Documentation/DMA-API.txt says, the parameters to dma_sync_sg_* must 
be the same as those originally passed into dma_map_sg. The segments are 
only merged *from the point of view of the device*: if I have a 
scatterlist of two discontiguous 4K segments, I can remap them with an 
IOMMU so the device sees them as a single 8K buffer, and tell it as 
such. If on the other hand I want to do maintenance from the CPU side 
(i.e. any DMA API call), then those DMA addresses mean nothing and I can 
only operate on the CPU addresses of the underlying pages, which are 
still very much discontiguous in the linear map; ergo I still need to 
iterate over the original entries.

Whilst I can't claim much familiarity with v4l itself, from a brief look 
over the existing code this patch does look to be doing the right thing.

Robin.

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Sakari Ailus]

Hi Robin,

On Tue, Sep 22, 2015 at 04:37:17PM +0100, Robin Murphy wrote:
> Hi Hans,
> 
> On 21/09/15 14:13, Hans Verkuil wrote:
> >Hi Tiffany!
> >
> >On 21-09-15 14:26, Tiffany Lin wrote:
> >>vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> >>But in dma_sync_sg_for_device, it use lengths of each SG entries
> >>before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> >>SGs until dma length > dma seg bundary. sgt->nents will less than
> >>sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> >>in vb2_dc_prepare will make some SGs are not sync to device.
> >>After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> >>sync data to device twice. Device randomly get incorrect data because
> >>some SGs are not sync to device. Change to use number of SG entries
> >>before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> >>
> >>Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> >>---
> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >>index 2397ceb..c5d00bd 100644
> >>--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >>+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >>@@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
> >>  	if (!sgt || buf->db_attach)
> >>  		return;
> >>
> >>-	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >>+	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> >>  }
> >>
> >>  static void vb2_dc_finish(void *buf_priv)
> >>@@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
> >>  	if (!sgt || buf->db_attach)
> >>  		return;
> >>
> >>-	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >>+	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> >>  }
> >
> >I don't really understand it. I am assuming that this happens on an arm and that
> >the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
> >arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.
> >
> >Now, as I understand it (and my understanding may very well be flawed!) the map_sg
> >function concatenates SG entries if possible, so it may return fewer entries. But
> >the dma_sync_sg functions use those updated SG entries, so the full buffer should
> >be covered by this. Using orig_nents will actually sync parts of the buffer twice!
> >The first nents entries already cover the full buffer so any remaining entries up
> >to orig_nents will just duplicate parts of the buffer.
> 
> As Documentation/DMA-API.txt says, the parameters to dma_sync_sg_* must be
> the same as those originally passed into dma_map_sg. The segments are only
> merged *from the point of view of the device*: if I have a scatterlist of
> two discontiguous 4K segments, I can remap them with an IOMMU so the device
> sees them as a single 8K buffer, and tell it as such. If on the other hand I
> want to do maintenance from the CPU side (i.e. any DMA API call), then those
> DMA addresses mean nothing and I can only operate on the CPU addresses of
> the underlying pages, which are still very much discontiguous in the linear
> map; ergo I still need to iterate over the original entries.
> 
> Whilst I can't claim much familiarity with v4l itself, from a brief look
> over the existing code this patch does look to be doing the right thing.

Thanks for the explanation. I wonder if this is the only place where we have
this issue. :-I

I think this might have been partly caused by the unfortunately named field
(nents) in struct sg_table. I wrote a small patch for this (plus a fix for a
few other things as well):


>From 8928d76db4d45c2b4a16ff90de6695bc88e19779 Mon Sep 17 00:00:00 2001
From: Sakari Ailus <sakari.ailus@iki.fi>
Date: Tue, 22 Sep 2015 23:30:30 +0300
Subject: [PATCH 1/1] Documentation: DMA API: Be more explicit that nelems is
 always the same

The nelems argument to the DMA API functions operating on scatterlists is
always the same. The documentation used different argument names and the
matter was not mentioned in Documentation/DMA-API-HOWTO.txt at all. Fix
these.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/DMA-API-HOWTO.txt | 5 +++++
 Documentation/DMA-API.txt       | 9 +++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/DMA-API-HOWTO.txt b/Documentation/DMA-API-HOWTO.txt
index 55b70b9..d69b3fc 100644
--- a/Documentation/DMA-API-HOWTO.txt
+++ b/Documentation/DMA-API-HOWTO.txt
@@ -681,6 +681,11 @@ or:
 
 as appropriate.
 
+PLEASE NOTE:  The 'nents' argument to dma_sync_sg_for_cpu() and
+	      dma_sync_sg_for_device() must be the same passed to
+	      dma_map_sg(). It is _NOT_ the count returned by
+	      dma_map_sg().
+
 After the last DMA transfer call one of the DMA unmap routines
 dma_unmap_{single,sg}(). If you don't touch the data from the first
 dma_map_*() call till dma_unmap_*(), then you don't have to call the
diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index edccacd..8c832f0 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -340,14 +340,15 @@ accessed sg->address and sg->length as shown above.
 
 	void
 	dma_unmap_sg(struct device *dev, struct scatterlist *sg,
-		int nhwentries, enum dma_data_direction direction)
+		int nents, enum dma_data_direction direction)
 
 Unmap the previously mapped scatter/gather list.  All the parameters
 must be the same as those and passed in to the scatter/gather mapping
 API.
 
 Note: <nents> must be the number you passed in, *not* the number of
-DMA address entries returned.
+DMA address entries returned. In struct sg_table this is the field
+called orig_nents.
 
 void
 dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
@@ -356,10 +357,10 @@ void
 dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle, size_t size,
 			   enum dma_data_direction direction)
 void
-dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int nelems,
+dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int nents,
 		    enum dma_data_direction direction)
 void
-dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, int nelems,
+dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, int nents,
 		       enum dma_data_direction direction)
 
 Synchronise a single contiguous or scatter/gather mapping for the CPU

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Daniel Kurtz]

Hi Sakari,

On Wed, Sep 23, 2015 at 4:37 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Robin,
>
> On Tue, Sep 22, 2015 at 04:37:17PM +0100, Robin Murphy wrote:
>> Hi Hans,
>>
>> On 21/09/15 14:13, Hans Verkuil wrote:
>> >Hi Tiffany!
>> >
>> >On 21-09-15 14:26, Tiffany Lin wrote:
>> >>vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
>> >>But in dma_sync_sg_for_device, it use lengths of each SG entries
>> >>before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
>> >>SGs until dma length > dma seg bundary. sgt->nents will less than
>> >>sgt->orig_nents. Using SG entries after dma_map_sg_attrs
>> >>in vb2_dc_prepare will make some SGs are not sync to device.
>> >>After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
>> >>sync data to device twice. Device randomly get incorrect data because
>> >>some SGs are not sync to device. Change to use number of SG entries
>> >>before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
>> >>
>> >>Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
>> >>---
>> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >>diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> >>index 2397ceb..c5d00bd 100644
>> >>--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> >>+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> >>@@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
>> >>    if (!sgt || buf->db_attach)
>> >>            return;
>> >>
>> >>-   dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>> >>+   dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>> >>  }
>> >>
>> >>  static void vb2_dc_finish(void *buf_priv)
>> >>@@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
>> >>    if (!sgt || buf->db_attach)
>> >>            return;
>> >>
>> >>-   dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>> >>+   dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>> >>  }
>> >
>> >I don't really understand it. I am assuming that this happens on an arm and that
>> >the dma_map_sg_attrs and dma_sync_sg_* functions used are arm_iommu_map_sg() and
>> >arm_iommu_sync_sg_* as implemented in arch/arm/mm/dma-mapping.c.
>> >
>> >Now, as I understand it (and my understanding may very well be flawed!) the map_sg
>> >function concatenates SG entries if possible, so it may return fewer entries. But
>> >the dma_sync_sg functions use those updated SG entries, so the full buffer should
>> >be covered by this. Using orig_nents will actually sync parts of the buffer twice!
>> >The first nents entries already cover the full buffer so any remaining entries up
>> >to orig_nents will just duplicate parts of the buffer.
>>
>> As Documentation/DMA-API.txt says, the parameters to dma_sync_sg_* must be
>> the same as those originally passed into dma_map_sg. The segments are only
>> merged *from the point of view of the device*: if I have a scatterlist of
>> two discontiguous 4K segments, I can remap them with an IOMMU so the device
>> sees them as a single 8K buffer, and tell it as such. If on the other hand I
>> want to do maintenance from the CPU side (i.e. any DMA API call), then those
>> DMA addresses mean nothing and I can only operate on the CPU addresses of
>> the underlying pages, which are still very much discontiguous in the linear
>> map; ergo I still need to iterate over the original entries.
>>
>> Whilst I can't claim much familiarity with v4l itself, from a brief look
>> over the existing code this patch does look to be doing the right thing.
>
> Thanks for the explanation. I wonder if this is the only place where we have
> this issue. :-I
>
> I think this might have been partly caused by the unfortunately named field
> (nents) in struct sg_table. I wrote a small patch for this (plus a fix for a
> few other things as well):
>
>
> From 8928d76db4d45c2b4a16ff90de6695bc88e19779 Mon Sep 17 00:00:00 2001
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Date: Tue, 22 Sep 2015 23:30:30 +0300
> Subject: [PATCH 1/1] Documentation: DMA API: Be more explicit that nelems is
>  always the same
>
> The nelems argument to the DMA API functions operating on scatterlists is
> always the same. The documentation used different argument names and the
> matter was not mentioned in Documentation/DMA-API-HOWTO.txt at all. Fix
> these.


Thanks for this patch!  I was very confused by this as well.
Can you also fix the following incorrect comments in arch/arm/mm/dma-mapping.c :

1694 /**
1695  * arm_iommu_sync_sg_for_cpu
1696  * @dev: valid struct device pointer
1697  * @sg: list of buffers
1698  * @nents: number of buffers to map (returned from dma_map_sg)
1699  * @dir: DMA transfer direction (same as was passed to dma_map_sg)
1700  */
1701 void arm_iommu_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,

1712 /**
1713  * arm_iommu_sync_sg_for_device
1714  * @dev: valid struct device pointer
1715  * @sg: list of buffers
1716  * @nents: number of buffers to map (returned from dma_map_sg)
1717  * @dir: DMA transfer direction (same as was passed to dma_map_sg)
1718  */
1719 void arm_iommu_sync_sg_for_device(struct device *dev, struct
scatterlist *sg,

In both cases @nents should be

  * @nents: number of buffers to sync (same as was passed to dma_map_sg)


Thanks!
-Dan

>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/DMA-API-HOWTO.txt | 5 +++++
>  Documentation/DMA-API.txt       | 9 +++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/DMA-API-HOWTO.txt b/Documentation/DMA-API-HOWTO.txt
> index 55b70b9..d69b3fc 100644
> --- a/Documentation/DMA-API-HOWTO.txt
> +++ b/Documentation/DMA-API-HOWTO.txt
> @@ -681,6 +681,11 @@ or:
>
>  as appropriate.
>
> +PLEASE NOTE:  The 'nents' argument to dma_sync_sg_for_cpu() and
> +             dma_sync_sg_for_device() must be the same passed to
> +             dma_map_sg(). It is _NOT_ the count returned by
> +             dma_map_sg().
> +
>  After the last DMA transfer call one of the DMA unmap routines
>  dma_unmap_{single,sg}(). If you don't touch the data from the first
>  dma_map_*() call till dma_unmap_*(), then you don't have to call the
> diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
> index edccacd..8c832f0 100644
> --- a/Documentation/DMA-API.txt
> +++ b/Documentation/DMA-API.txt
> @@ -340,14 +340,15 @@ accessed sg->address and sg->length as shown above.
>
>         void
>         dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> -               int nhwentries, enum dma_data_direction direction)
> +               int nents, enum dma_data_direction direction)
>
>  Unmap the previously mapped scatter/gather list.  All the parameters
>  must be the same as those and passed in to the scatter/gather mapping
>  API.
>
>  Note: <nents> must be the number you passed in, *not* the number of
> -DMA address entries returned.
> +DMA address entries returned. In struct sg_table this is the field
> +called orig_nents.
>
>  void
>  dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
> @@ -356,10 +357,10 @@ void
>  dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle, size_t size,
>                            enum dma_data_direction direction)
>  void
> -dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int nelems,
> +dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int nents,
>                     enum dma_data_direction direction)
>  void
> -dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, int nelems,
> +dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, int nents,
>                        enum dma_data_direction direction)
>
>  Synchronise a single contiguous or scatter/gather mapping for the CPU
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ailus at iki.fi     XMPP: sailus at retiisi.org.uk
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Sakari Ailus]

Hi Tiffany,

(Robin and Hans cc'd.)

On Mon, Sep 21, 2015 at 08:26:11PM +0800, Tiffany Lin wrote:
> vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> But in dma_sync_sg_for_device, it use lengths of each SG entries
> before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> SGs until dma length > dma seg bundary. sgt->nents will less than
> sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> in vb2_dc_prepare will make some SGs are not sync to device.
> After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> sync data to device twice. Device randomly get incorrect data because
> some SGs are not sync to device. Change to use number of SG entries
> before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> 
> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 2397ceb..c5d00bd 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
>  	if (!sgt || buf->db_attach)
>  		return;
>  
> -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>  }
>  
>  static void vb2_dc_finish(void *buf_priv)
> @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
>  	if (!sgt || buf->db_attach)
>  		return;
>  
> -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>  }
>  
>  /*********************************************/

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Could you post a similar patch for videobuf2-dma-sg as well, please? This
should probably go to stable (since when?).

videobuf-dma-sg appears to be broken, too, but the fix is more changes
than one or two lines.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Hans Verkuil]

Resent, hopefully without html this time.

On September 22, 2015 11:10:15 PM GMT+02:00, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>Hi Tiffany,
>
>(Robin and Hans cc'd.)
>
>On Mon, Sep 21, 2015 at 08:26:11PM +0800, Tiffany Lin wrote:
>> vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
>> But in dma_sync_sg_for_device, it use lengths of each SG entries
>> before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
>> SGs until dma length > dma seg bundary. sgt->nents will less than
>> sgt->orig_nents. Using SG entries after dma_map_sg_attrs
>> in vb2_dc_prepare will make some SGs are not sync to device.
>> After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
>> sync data to device twice. Device randomly get incorrect data because
>> some SGs are not sync to device. Change to use number of SG entries
>> before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
>> 
>> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> index 2397ceb..c5d00bd 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
>>  	if (!sgt || buf->db_attach)
>>  		return;
>>  
>> -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents,
>buf->dma_dir);
>> +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
>buf->dma_dir);
>>  }
>>  
>>  static void vb2_dc_finish(void *buf_priv)
>> @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
>>  	if (!sgt || buf->db_attach)
>>  		return;
>>  
>> -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>> +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents,
>buf->dma_dir);
>>  }
>>  
>>  /*********************************************/
>
>Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
>Could you post a similar patch for videobuf2-dma-sg as well, please?
>This
>should probably go to stable (since when?).
>
>videobuf-dma-sg appears to be broken, too, but the fix is more changes
>than one or two lines.
>
>-- 
>Kind regards,
>
>Sakari Ailus
>e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

Sakari, can you take a careful look at the vb2 code? If I remember correctly, the nents field receives the result of the map_sg function. I have no idea if that's correct.

BTW, don't spend too much time on vb1, nobody cares about that old framework, and vb1 drivers are rarely used on arm platforms. 

Regards,

Hans 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[RESEND PATCH] media: vb2: Fix vb2_dc_prepare do not correct sync data to device

[Sakari Ailus]

Hi Hans,

On Wed, Sep 23, 2015 at 10:40:56AM +0200, Hans Verkuil wrote:
> Resent, hopefully without html this time.
> 
> On September 22, 2015 11:10:15 PM GMT+02:00, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >Hi Tiffany,
> >
> >(Robin and Hans cc'd.)
> >
> >On Mon, Sep 21, 2015 at 08:26:11PM +0800, Tiffany Lin wrote:
> >> vb2_dc_prepare use the number of SG entries dma_map_sg_attrs return.
> >> But in dma_sync_sg_for_device, it use lengths of each SG entries
> >> before dma_map_sg_attrs. dma_map_sg_attrs will concatenate
> >> SGs until dma length > dma seg bundary. sgt->nents will less than
> >> sgt->orig_nents. Using SG entries after dma_map_sg_attrs
> >> in vb2_dc_prepare will make some SGs are not sync to device.
> >> After add DMA_ATTR_SKIP_CPU_SYNC in vb2_dc_get_userptr to remove
> >> sync data to device twice. Device randomly get incorrect data because
> >> some SGs are not sync to device. Change to use number of SG entries
> >> before dma_map_sg_attrs in vb2_dc_prepare to prevent this issue.
> >> 
> >> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> >> ---
> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> index 2397ceb..c5d00bd 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -100,7 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
> >>  	if (!sgt || buf->db_attach)
> >>  		return;
> >>  
> >> -	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents,
> >buf->dma_dir);
> >> +	dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
> >buf->dma_dir);
> >>  }
> >>  
> >>  static void vb2_dc_finish(void *buf_priv)
> >> @@ -112,7 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
> >>  	if (!sgt || buf->db_attach)
> >>  		return;
> >>  
> >> -	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >> +	dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents,
> >buf->dma_dir);
> >>  }
> >>  
> >>  /*********************************************/
> >
> >Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> >Could you post a similar patch for videobuf2-dma-sg as well, please?
> >This
> >should probably go to stable (since when?).
> >
> >videobuf-dma-sg appears to be broken, too, but the fix is more changes
> >than one or two lines.
> >
> >-- 
> >Kind regards,
> >
> >Sakari Ailus
> >e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk
> 
> Sakari, can you take a careful look at the vb2 code? If I remember
> correctly, the nents field receives the result of the map_sg function. I
> have no idea if that's correct.

As far as I can tell, it is. According to a comment in the definition of
struct sg_table in include/linux/scatterlist.h, this is the number of
*mapped* entries in the table. Although a number of drivers construct the
table by themselves use nents only, __sg_alloc_table() assigns the same
number to both. The videobuf2 bug appears to be one of its kind --- I
checked the other users of struct sg_table for the purpose.
drivers/spi/spi.c has the same pattern except that it does not involve
syncing the cache.

There could be other users of dma_map_sg() that get this wrong though.

Perhaps the comment on the sg_table shouldn't be added to the documentation
as most of the users appear to be using it differently, even if it appears
to be in a conflict with the intended usage.

As far as I understand, what we need a similar fix for dma-sg allocator.

> 
> BTW, don't spend too much time on vb1, nobody cares about that old
> framework, and vb1 drivers are rarely used on arm platforms.

In that case the wrong number of sglist entries is also passed to
dma_unmap_sg(). Although in most cases it still works. I think the BTTV
driver is using it, for instance.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk