| Message ID | 20220608135821.1153346-1-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v3] drm/cma-helper: Describe what a "contiguous chunk" actually means | expand |
On Wed, Jun 08, 2022 at 02:58:21PM +0100, Daniel Thompson wrote: > Since it's inception in 2012 it has been understood that the DRM GEM CMA > helpers do not depend on CMA as the backend allocator. In fact the first > bug fix to ensure the cma-helpers work correctly with an IOMMU backend > appeared in 2014. However currently the documentation for > drm_gem_cma_create() talks about "a contiguous chunk of memory" without > making clear which address space it will be a contiguous part of. > Additionally the CMA introduction is actively misleading because it only > contemplates the CMA backend. > > This matters because when the device accesses the bus through an IOMMU > (and don't use the CMA backend) then the allocated memory is contiguous > only in the IOVA space. This is a significant difference compared to the > CMA backend and the behaviour can be a surprise even to someone who does > a reasonable level of code browsing (but doesn't find all the relevant > function pointers ;-) ). > > Improve the kernel doc comments accordingly. > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > > Notes: > Am I Cc:ing the correct reviewers/maintainers with this patch? There > has been no negative feedback but I've been rebasing and re-posting it > for three kernel cycles now. Do I need to queue it somewhere special or > get it in front of someone specific? Occasionally stuff falls through a few too many cracks, that's all. We have tons of committers for drm-misc (and Lucas is one of them), but sometimes they shy away from pushing themselves and others see the r-b and assume it's already handled, and then it doesn't move :-/ Anyway I pushed it now, thanks for your patch. -Daniel > > Either way... > > This RESEND is unaltered (except for collecting tags) and is rebased on > v5.19-rc1. > > RESEND for v5.18-rc3 > - Unaltered but rebased on v5.18-rc3 > > Changes in v3: > - Rebased on v5.17-rc2 > - Minor improvements to wording. > > Changes in v2: > - Oops. I did a final proof read and accidentally committed these > changes as a seperate patch. This means that v1 contains only > one tenth of the actual patch. This is fixed in v2. Many apologies > for the noise! > > drivers/gpu/drm/drm_gem_cma_helper.c | 39 +++++++++++++++++++++------- > 1 file changed, 29 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c > index f36734c2c9e1..42abee9a0f4f 100644 > --- a/drivers/gpu/drm/drm_gem_cma_helper.c > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c > @@ -26,12 +26,22 @@ > /** > * DOC: cma helpers > * > - * The Contiguous Memory Allocator reserves a pool of memory at early boot > - * that is used to service requests for large blocks of contiguous memory. > + * The DRM GEM/CMA helpers are a means to provide buffer objects that are > + * presented to the device as a contiguous chunk of memory. This is useful > + * for devices that do not support scatter-gather DMA (either directly or > + * by using an intimately attached IOMMU). > * > - * The DRM GEM/CMA helpers use this allocator as a means to provide buffer > - * objects that are physically contiguous in memory. This is useful for > - * display drivers that are unable to map scattered buffers via an IOMMU. > + * Despite the name, the DRM GEM/CMA helpers are not hardwired to use the > + * Contiguous Memory Allocator (CMA). > + * > + * For devices that access the memory bus through an (external) IOMMU then > + * the buffer objects are allocated using a traditional page-based > + * allocator and may be scattered through physical memory. However they > + * are contiguous in the IOVA space so appear contiguous to devices using > + * them. > + * > + * For other devices then the helpers rely on CMA to provide buffer > + * objects that are physically contiguous in memory. > * > * For GEM callback helpers in struct &drm_gem_object functions, see likewise > * named functions with an _object_ infix (e.g., drm_gem_cma_object_vmap() wraps > @@ -111,8 +121,14 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size, bool private) > * @drm: DRM device > * @size: size of the object to allocate > * > - * This function creates a CMA GEM object and allocates a contiguous chunk of > - * memory as backing store. > + * This function creates a CMA GEM object and allocates memory as backing store. > + * The allocated memory will occupy a contiguous chunk of bus address space. > + * > + * For devices that are directly connected to the memory bus then the allocated > + * memory will be physically contiguous. For devices that access through an > + * IOMMU, then the allocated memory is not expected to be physically contiguous > + * because having contiguous IOVAs is sufficient to meet a devices DMA > + * requirements. > * > * Returns: > * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative > @@ -162,9 +178,12 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_create); > * @size: size of the object to allocate > * @handle: return location for the GEM handle > * > - * This function creates a CMA GEM object, allocating a physically contiguous > - * chunk of memory as backing store. The GEM object is then added to the list > - * of object associated with the given file and a handle to it is returned. > + * This function creates a CMA GEM object, allocating a chunk of memory as > + * backing store. The GEM object is then added to the list of object associated > + * with the given file and a handle to it is returned. > + * > + * The allocated memory will occupy a contiguous chunk of bus address space. > + * See drm_gem_cma_create() for more details. > * > * Returns: > * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative > > base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56 > -- > 2.35.1 >
On Wed, Jun 08, 2022 at 05:37:50PM +0200, Daniel Vetter wrote: > On Wed, Jun 08, 2022 at 02:58:21PM +0100, Daniel Thompson wrote: > > Since it's inception in 2012 it has been understood that the DRM GEM CMA > > helpers do not depend on CMA as the backend allocator. In fact the first > > bug fix to ensure the cma-helpers work correctly with an IOMMU backend > > appeared in 2014. However currently the documentation for > > drm_gem_cma_create() talks about "a contiguous chunk of memory" without > > making clear which address space it will be a contiguous part of. > > Additionally the CMA introduction is actively misleading because it only > > contemplates the CMA backend. > > > > This matters because when the device accesses the bus through an IOMMU > > (and don't use the CMA backend) then the allocated memory is contiguous > > only in the IOVA space. This is a significant difference compared to the > > CMA backend and the behaviour can be a surprise even to someone who does > > a reasonable level of code browsing (but doesn't find all the relevant > > function pointers ;-) ). > > > > Improve the kernel doc comments accordingly. > > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > > > Notes: > > Am I Cc:ing the correct reviewers/maintainers with this patch? There > > has been no negative feedback but I've been rebasing and re-posting it > > for three kernel cycles now. Do I need to queue it somewhere special or > > get it in front of someone specific? > > Occasionally stuff falls through a few too many cracks, that's all. We > have tons of committers for drm-misc (and Lucas is one of them), but > sometimes they shy away from pushing themselves and others see the r-b and > assume it's already handled, and then it doesn't move :-/ No worries. Arguably I should have asked this question a little earlier anyway. Thanks for pushing it. Daniel.
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index f36734c2c9e1..42abee9a0f4f 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -26,12 +26,22 @@ /** * DOC: cma helpers * - * The Contiguous Memory Allocator reserves a pool of memory at early boot - * that is used to service requests for large blocks of contiguous memory. + * The DRM GEM/CMA helpers are a means to provide buffer objects that are + * presented to the device as a contiguous chunk of memory. This is useful + * for devices that do not support scatter-gather DMA (either directly or + * by using an intimately attached IOMMU). * - * The DRM GEM/CMA helpers use this allocator as a means to provide buffer - * objects that are physically contiguous in memory. This is useful for - * display drivers that are unable to map scattered buffers via an IOMMU. + * Despite the name, the DRM GEM/CMA helpers are not hardwired to use the + * Contiguous Memory Allocator (CMA). + * + * For devices that access the memory bus through an (external) IOMMU then + * the buffer objects are allocated using a traditional page-based + * allocator and may be scattered through physical memory. However they + * are contiguous in the IOVA space so appear contiguous to devices using + * them. + * + * For other devices then the helpers rely on CMA to provide buffer + * objects that are physically contiguous in memory. * * For GEM callback helpers in struct &drm_gem_object functions, see likewise * named functions with an _object_ infix (e.g., drm_gem_cma_object_vmap() wraps @@ -111,8 +121,14 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size, bool private) * @drm: DRM device * @size: size of the object to allocate * - * This function creates a CMA GEM object and allocates a contiguous chunk of - * memory as backing store. + * This function creates a CMA GEM object and allocates memory as backing store. + * The allocated memory will occupy a contiguous chunk of bus address space. + * + * For devices that are directly connected to the memory bus then the allocated + * memory will be physically contiguous. For devices that access through an + * IOMMU, then the allocated memory is not expected to be physically contiguous + * because having contiguous IOVAs is sufficient to meet a devices DMA + * requirements. * * Returns: * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative @@ -162,9 +178,12 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_create); * @size: size of the object to allocate * @handle: return location for the GEM handle * - * This function creates a CMA GEM object, allocating a physically contiguous - * chunk of memory as backing store. The GEM object is then added to the list - * of object associated with the given file and a handle to it is returned. + * This function creates a CMA GEM object, allocating a chunk of memory as + * backing store. The GEM object is then added to the list of object associated + * with the given file and a handle to it is returned. + * + * The allocated memory will occupy a contiguous chunk of bus address space. + * See drm_gem_cma_create() for more details. * * Returns: * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative