| Message ID | 20200826063316.23486-15-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | DRM: fix struct sg_table nents vs. orig_nents misuse | expand |
On 2020-08-26 07:32, Marek Szyprowski wrote: > 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 :) Robin. > * > * - buffers mapped through the TILER when dma_addr_cnt is not zero, in > * which case the DMA address points to the TILER aperture > @@ -1279,7 +1279,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, > union omap_gem_size gsize; > > /* Without a DMM only physically contiguous buffers can be supported. */ > - if (sgt->orig_nents != 1 && !priv->has_dmm) > + if (sgt->nents != 1 && !priv->has_dmm) > return ERR_PTR(-EINVAL); > > gsize.bytes = PAGE_ALIGN(size); > @@ -1293,7 +1293,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, > > omap_obj->sgt = sgt; > > - if (sgt->orig_nents == 1) { > + if (sgt->nents == 1) { > omap_obj->dma_addr = sg_dma_address(sgt->sgl); > } else { > /* Create pages list from sgt */ > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 01/09/2020 22:33, Robin Murphy wrote: > On 2020-08-26 07:32, Marek Szyprowski wrote: >> 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? Tomi
Hi Tomi, On 02.09.2020 10:00, Tomi Valkeinen wrote: > On 01/09/2020 22:33, Robin Murphy wrote: >> On 2020-08-26 07:32, Marek Szyprowski wrote: >>> 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. If you don't like it, we can drop this patch, because it won't change the way the driver works. Best regards
Hi again, On 04.09.2020 14:06, Marek Szyprowski wrote: > Hi Tomi, > > On 02.09.2020 10:00, Tomi Valkeinen wrote: >> On 01/09/2020 22:33, Robin Murphy wrote: >>> On 2020-08-26 07:32, Marek Szyprowski wrote: >>>> 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
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) * * - buffers mapped through the TILER when dma_addr_cnt is not zero, in * which case the DMA address points to the TILER aperture @@ -1279,7 +1279,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, union omap_gem_size gsize; /* Without a DMM only physically contiguous buffers can be supported. */ - if (sgt->orig_nents != 1 && !priv->has_dmm) + if (sgt->nents != 1 && !priv->has_dmm) return ERR_PTR(-EINVAL); gsize.bytes = PAGE_ALIGN(size); @@ -1293,7 +1293,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, omap_obj->sgt = sgt; - if (sgt->orig_nents == 1) { + if (sgt->nents == 1) { omap_obj->dma_addr = sg_dma_address(sgt->sgl); } else { /* Create pages list from sgt */
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(-) -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel