Hi again,

On 04.09.2020 14:06, Marek Szyprowski wrote:

Hi Tomi,

On 02.09.2020 10:00, Tomi Valkeinen wrote:

quoted

On 01/09/2020 22:33, Robin Murphy wrote:

quoted

On 2020-08-26 07:32, Marek Szyprowski wrote:

quoted

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() 
function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().

struct sg_table is a common structure used for describing a 
non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl 
entry),
as well as the number of scatterlist entries: CPU pages (orig_nents 
entry)
and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and 
orig_nents
entries, calling DMA-mapping functions with a wrong number of 
entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.

Fix the code to refer to proper nents or orig_nents entries. This 
driver
checks for a buffer contiguity in DMA address space, so it should test
sg_table->nents entry.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
   drivers/gpu/drm/omapdrm/omap_gem.c | 6 +++---
   1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 

b/drivers/gpu/drm/omapdrm/omap_gem.c
index ff0c4b0c3fd0..a7a9a0afe2b6 100644

--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c

@@ -48,7 +48,7 @@ struct omap_gem_object {

        *   OMAP_BO_MEM_DMA_API flag set)
        *
        * - buffers imported from dmabuf (with the 
OMAP_BO_MEM_DMABUF flag set)
-     *   if they are physically contiguous (when sgt->orig_nents 
== 1)
+     *   if they are physically contiguous (when sgt->nents == 1)

Hmm, if this really does mean *physically* contiguous - i.e. if 
buffers might be shared between
DMA-translatable and non-DMA-translatable devices - then these 
changes might not be appropriate. If
not and it only actually means DMA-contiguous, then it would be good 
to clarify the comments to that
effect.

Can anyone familiar with omapdrm clarify what exactly the case is 
here? I know that IOMMUs might be
involved to some degree, and I've skimmed the interconnect chapters 
of enough OMAP TRMs to be scared
by the reference to the tiler aperture in the context below :)

DSS (like many other IPs in OMAP) does not have any MMU/PAT, and can 
only use contiguous buffers
(contiguous in the RAM).

There's a special case with TILER (which is not part of DSS but of 
the memory subsystem, but it's
still handled internally by the omapdrm driver), which has a PAT. PAT 
can create a contiguous view
of scattered pages, and DSS can then use this contiguous view ("tiler 
aperture", which to DSS looks
just like normal contiguous memory).

Note that omapdrm does not use dma_map_sg() & co. mentioned in the 
patch description.

If there's no MMU/PAT, is orig_nents always the same as nents? Or can 
we have multiple physically
contiguous pages listed separately in the sgt (so orig_nents > 1) but 
as the pages form one big
contiguous area, nents == 1?

Well, when DMA-mapping API is properly used, the difference between 
nents and orig_nents is only when the scatterlist have been mapped for 
DMA.

For the mentioned case, even if the creator of the buffer used only 
the pages that are consecutive in the physical memory, he is free to 
chose either to set nents/orig_nents to 1 and length to n*PAGE_SIZE or 
set nents/orig_nents to n and length to PAGE_SIZE for each. Then the 
buffer chunks might be merged, but this is done by the DMA-mapping 
code. For your case, without any call to DMA-mapping, you can only 
assume that the buffer is contiguous in physical memory if orig_nents 
is 1.

I've changed the use of nents to orig_nents to make things consistent 
- this code operates only on the unmapped buffers. I want to ensure 
that anyone who will potentially copy this code, won't make the 
nents/orig_nents mistake in the future.

I've just noticed that I've read my patch (the diff) in the reverse 
order, I'm sorry. The omapdrm code is right, this patch should be dropped.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel