staging: android: ion: Set the length of the DMA sg entries in buffer
PATCH 1/1 staging: android: ion: Set the length of the DMA sg entries in buffer
From: Liviu Dudau <[email protected]>
ion_buffer_create() will allocate a buffer and then create a DMA mapping for it, but it forgot to set the length of the page entries.
--- drivers/staging/android/ion/ion.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/android/ion/ion.c +3 -1
@@ -251,8 +251,10 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,* memory coming from the heaps is ready for dma, ie if it has a * cached mapping that mapping has been invalidated */ -for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) +for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) {sg_dma_address (sg) =sg_phys (sg); +sg_dma_len (sg) = sg->length; + }mutex_lock (&dev->buffer_lock);ion_buffer_add (dev, buffer);mutex_unlock (&dev->buffer_lock);
Signature · 1 line
2.1.4
Quoted 18 lines · Jon Medhurst (Tixy)
From: Liviu Dudau <[email protected]>
ion_buffer_create()will allocate a buffer and then create a DMA mapping for it, but it forgot to set the length of the page entries.Signed-off-by Liviu Dudau <[email protected]>Signed-off-by Jon Medhurst <[email protected]>--- drivers/staging/android/ion/ion.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/staging/android/ion/ion.c +0
-0@@ -251,8 +251,10 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,* memory coming from the heaps is ready for dma, ie if it has a
drivers/staging/android/ion/ion.c static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, · line 260
Quoted 10 lines
drivers/staging/android/ion/ion.c +3
-1* cached mapping that mapping has been invalidated */ -for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) +for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) {sg_dma_address (sg) =sg_phys (sg); +sg_dma_len (sg) = sg->length; + }mutex_lock (&dev->buffer_lock);ion_buffer_add (dev, buffer);mutex_unlock (&dev->buffer_lock);
So Ion is really doing it wrong by setting the sg_dma_address manually as the comment above notes. Ion has moved away from sg_dma_len though (see 06e0dcaeb4fd72a010a1f5ad0c03abd8e0a58ef9). This isn't technically a mapping as well. What's broken by not having sg_dma_len set?
Thanks,
Laura
Quoted 26 lines · Laura Abbott
Quoted 25 lines · Jon Medhurst (Tixy)
From: Liviu Dudau <[email protected]>
ion_buffer_create()will allocate a buffer and then create a DMA mapping for it, but it forgot to set the length of the page entries.Signed-off-by Liviu Dudau <[email protected]>Signed-off-by Jon Medhurst <[email protected]>--- drivers/staging/android/ion/ion.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/staging/android/ion/ion.c +3
-1@@ -251,8 +251,10 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,* memory coming from the heaps is ready for dma, ie if it has a * cached mapping that mapping has been invalidated */ -for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) +for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) {sg_dma_address (sg) =sg_phys (sg); +sg_dma_len (sg) = sg->length; + }mutex_lock (&dev->buffer_lock);
Quoted 8 lines
drivers/staging/android/ion/ion.c static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, · line 260
Quoted 3 lines
drivers/staging/android/ion/ion.c +0
-0ion_buffer_add (dev, buffer);mutex_unlock (&dev->buffer_lock);So Ion is really doing it wrong by setting the
sg_dma_addressmanually as the comment above notes. Ion has moved away fromsg_dma_lenthough (see06e0dcaeb4fd72a010a1f5ad0c03abd8e0a58ef9). This isn't technically a mapping as well. What's broken by not havingsg_dma_lenset?
I fear this could end up being embarrassing...
What's broken is that the out-of-tree kernel driver for ARM's Mali GPU is getting passed a dma_buf corresponding to the ION buffer. It is then calling dma_buf_map_attachment [1] on that and then parsing the resultant scatter-gather list to get the physical pages so it can pass them to the GPU hardware. In the process, it is using sg_dma_len() to get the length, which is garbage for ION buffers if ion_buffer_create() doesn't set it.
Now, I just tried making the Mali driver use sg->length rather than sg_dma_len() and, unsurprisingly, that also fixes the problem. So, my questions would be...
Is it acceptable for a driver getting a dma_buf to parse the scatter-gather list for that by had?
If so, should it use ->length or sg_dma_len() to get the length of each element?
If sg_dma_len() is correct or acceptable then it seems to me that the ION code should set that length. Especially as the comment in the code implies it's faking a call to map_sg and grepping the kernel tree for real implementations of that functionality seems to show the dma_address getting set.
As you can probably tell, I feel I may be on shaky ground. This is because I don't fully understanding the code and suspecting both the ION and GPU code is rather dodgy (and possibly the bits in between :-)
Signature · 1 line
Tixy
Laura Abbott
Acked-byQuoted 64 lines · Jon Medhurst (Tixy)
Quoted 26 lines · Laura Abbott
Quoted 25 lines · Jon Medhurst (Tixy)
From: Liviu Dudau <[email protected]>
ion_buffer_create()will allocate a buffer and then create a DMA mapping for it, but it forgot to set the length of the page entries.Signed-off-by Liviu Dudau <[email protected]>Signed-off-by Jon Medhurst <[email protected]>--- drivers/staging/android/ion/ion.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/staging/android/ion/ion.c +3
-1@@ -251,8 +251,10 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,* memory coming from the heaps is ready for dma, ie if it has a * cached mapping that mapping has been invalidated */ -for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) +for_each_sg (buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) {sg_dma_address (sg) =sg_phys (sg); +sg_dma_len (sg) = sg->length; + }mutex_lock (&dev->buffer_lock);Quoted 8 lines
drivers/staging/android/ion/ion.c static struct ion_buffer *ion_buffer_create(struct ion_heap *heap, · line 260
Quoted 3 lines
drivers/staging/android/ion/ion.c +0
-0ion_buffer_add (dev, buffer);mutex_unlock (&dev->buffer_lock);So Ion is really doing it wrong by setting the
sg_dma_addressmanually as the comment above notes. Ion has moved away fromsg_dma_lenthough (see06e0dcaeb4fd72a010a1f5ad0c03abd8e0a58ef9). This isn't technically a mapping as well. What's broken by not havingsg_dma_lenset?I fear this could end up being embarrassing...
What's broken is that the out-of-tree kernel driver for ARM's Mali GPU is getting passed a
dma_bufcorresponding to the ION buffer. It is then callingdma_buf_map_attachment[1] on that and then parsing the resultant scatter-gather list to get the physical pages so it can pass them to the GPU hardware. In the process, it is usingsg_dma_len()to get the length, which is garbage for ION buffers ifion_buffer_create()doesn't set it.Now, I just tried making the Mali driver use
sg->lengthrather thansg_dma_len()and, unsurprisingly, that also fixes the problem. So, my questions would be...Is it acceptable for a driver getting a
dma_bufto parse the scatter-gather list for that by had?If so, should it use ->length or
sg_dma_len()to get the length of each element?If
sg_dma_len()is correct or acceptable then it seems to me that the ION code should set that length. Especially as the comment in the code implies it's faking a call tomap_sgand grepping the kernel tree for real implementations of that functionality seems to show thedma_addressgetting set.
Quoted 4 lines
As you can probably tell, I feel I may be on shaky ground. This is because I don't fully understanding the code and suspecting both the ION and GPU code is rather dodgy (and possibly the bits in between :-)
I blame the Ion code completely. I remember hitting a similar problem with other out of tree drivers. The solution then was to have drivers switch to using sg->length instead of sg_dma_len given the state of that tree. For the Mali driver, if it is ever going to be backed by an IOMMU you will need to use sg_dma_len so I think at least that part of your code is correct.
Thinking about it some, I'm okay with the patch going in. I thought there was some reason why the out of tree code from before didn't just do this hack but I can't remember it. It may have been an out of tree use case. This does go well with Ion's behavior of pretending to do DMA mapping. More out of tree users can plead their case if it breaks.
Quoted 1 line · Laura Abbott
On 01/21/2016 12:19 PM, Jon Medhurst (Tixy) wrote:
[...]
Quoted 18 lines
Quoted 10 lines
If
sg_dma_len()is correct or acceptable then it seems to me that the ION code should set that length. Especially as the comment in the code implies it's faking a call tomap_sgand grepping the kernel tree for real implementations of that functionality seems to show thedma_addressgetting set.As you can probably tell, I feel I may be on shaky ground. This is because I don't fully understanding the code and suspecting both the ION and GPU code is rather dodgy (and possibly the bits in between :-)
I blame the Ion code completely. I remember hitting a similar problem with other out of tree drivers. The solution then was to have drivers switch to using
sg->lengthinstead ofsg_dma_lengiven the state of that tree. For the Mali driver, if it is ever going to be backed by an IOMMU you will need to usesg_dma_lenso I think at least that part of your code is correct.
Quoted 5 lines
Thinking about it some, I'm okay with the patch going in. I thought there was some reason why the out of tree code from before didn't just do this hack but I can't remember it. It may have been an out of tree use case. This does go well with Ion's behavior of pretending to do DMA mapping. More out of tree users can plead their case if it breaks.
Well, the $subject patch is copying the value of 'length' into 'dma_length', and if dma_length was previously uninitialised then I don't see that it can really cause additional problems for ION users. (I hate the way I've been using 'if' a lot so I'm going to spend some time educating myself.)
Quoted 1 line
Acked-by Laura Abbott <[email protected]>
Thanks
Signature · 1 line
Tixy