* [RFC PATCH v2 1/6] lib/scatterlist.c: Support constructing sgt from page xarray
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
@ 2025-03-26 2:14 ` Adrián Larumbe
2025-04-01 8:28 ` Boris Brezillon
2025-03-26 2:14 ` [RFC PATCH v2 2/6] drm/shmem: Introduce the notion of sparse objects Adrián Larumbe
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Adrián Larumbe @ 2025-03-26 2:14 UTC (permalink / raw)
To: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, Adrián Larumbe, linux-kernel, dri-devel
In preparation for a future commit that will introduce sparse allocation
of pages in DRM shmem, a scatterlist function that knows how to deal
with an xarray collection of memory pages had to be introduced.
Because the new function is identical to the existing one that deals
with a page array, the page_array abstraction is also introduced, which
hides the way pages are retrieved from a collection.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
include/linux/scatterlist.h | 17 ++++
lib/scatterlist.c | 175 +++++++++++++++++++++++++-----------
2 files changed, 142 insertions(+), 50 deletions(-)
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index d836e7440ee8..cffb0cffcda0 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -447,6 +447,11 @@ int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
unsigned int n_pages, unsigned int offset,
unsigned long size,
unsigned int max_segment, gfp_t gfp_mask);
+int sg_alloc_table_from_xarray_segment(struct sg_table *sgt, struct xarray *pages,
+ unsigned int idx, unsigned int n_pages,
+ unsigned int offset, unsigned long size,
+ unsigned int max_segment, gfp_t gfp_mask);
+
/**
* sg_alloc_table_from_pages - Allocate and initialize an sg table from
@@ -478,6 +483,18 @@ static inline int sg_alloc_table_from_pages(struct sg_table *sgt,
size, UINT_MAX, gfp_mask);
}
+static inline int sg_alloc_table_from_xarray(struct sg_table *sgt,
+ struct xarray *pages,
+ unsigned int idx,
+ unsigned int n_pages,
+ unsigned int offset,
+ unsigned long size, gfp_t gfp_mask)
+{
+ return sg_alloc_table_from_xarray_segment(sgt, pages, idx, n_pages, offset,
+ size, UINT_MAX, gfp_mask);
+}
+
+
#ifdef CONFIG_SGL_ALLOC
struct scatterlist *sgl_alloc_order(unsigned long long length,
unsigned int order, bool chainable,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 5bb6b8aff232..08b9ed51324e 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -423,43 +423,53 @@ static bool pages_are_mergeable(struct page *a, struct page *b)
return true;
}
-/**
- * sg_alloc_append_table_from_pages - Allocate and initialize an append sg
- * table from an array of pages
- * @sgt_append: The sg append table to use
- * @pages: Pointer to an array of page pointers
- * @n_pages: Number of pages in the pages array
- * @offset: Offset from start of the first page to the start of a buffer
- * @size: Number of valid bytes in the buffer (after offset)
- * @max_segment: Maximum size of a scatterlist element in bytes
- * @left_pages: Left pages caller have to set after this call
- * @gfp_mask: GFP allocation mask
- *
- * Description:
- * In the first call it allocate and initialize an sg table from a list of
- * pages, else reuse the scatterlist from sgt_append. Contiguous ranges of
- * the pages are squashed into a single scatterlist entry up to the maximum
- * size specified in @max_segment. A user may provide an offset at a start
- * and a size of valid data in a buffer specified by the page array. The
- * returned sg table is released by sg_free_append_table
- *
- * Returns:
- * 0 on success, negative error on failure
- *
- * Notes:
- * If this function returns non-0 (eg failure), the caller must call
- * sg_free_append_table() to cleanup any leftover allocations.
- *
- * In the fist call, sgt_append must by initialized.
- */
-int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
- struct page **pages, unsigned int n_pages, unsigned int offset,
- unsigned long size, unsigned int max_segment,
- unsigned int left_pages, gfp_t gfp_mask)
+struct page_array {
+ union {
+ struct page **array;
+ struct xarray *xarray;
+ };
+
+ struct page * (* const get_page)(struct page_array, unsigned int);
+};
+
+static inline struct page *page_array_get_page(struct page_array a,
+ unsigned int index)
{
- unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
+ return a.array[index];
+}
+
+static inline struct page *page_xarray_get_page(struct page_array a,
+ unsigned int index)
+{
+ return xa_load(a.xarray, index);
+}
+
+#define PAGE_ARRAY(pages) \
+ ((struct page_array) { \
+ .array = pages, \
+ .get_page = page_array_get_page, \
+ })
+
+#define PAGE_XARRAY(pages) \
+ ((struct page_array) { \
+ .xarray = pages, \
+ .get_page = page_xarray_get_page, \
+ })
+
+static inline int
+sg_alloc_append_table_from_page_array(struct sg_append_table *sgt_append,
+ struct page_array pages,
+ unsigned int first_page,
+ unsigned int n_pages,
+ unsigned int offset, unsigned long size,
+ unsigned int max_segment,
+ unsigned int left_pages, gfp_t gfp_mask)
+{
+ unsigned int chunks, seg_len, i, prv_len = 0;
unsigned int added_nents = 0;
struct scatterlist *s = sgt_append->prv;
+ unsigned int cur_pg_index = first_page;
+ unsigned int last_pg_index = first_page + n_pages - 1;
struct page *last_pg;
/*
@@ -475,24 +485,26 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
if (sgt_append->prv) {
unsigned long next_pfn;
+ struct page *page;
if (WARN_ON(offset))
return -EINVAL;
/* Merge contiguous pages into the last SG */
+ page = pages.get_page(pages, cur_pg_index);
prv_len = sgt_append->prv->length;
next_pfn = (sg_phys(sgt_append->prv) + prv_len) / PAGE_SIZE;
- if (page_to_pfn(pages[0]) == next_pfn) {
+ if (page_to_pfn(page) == next_pfn) {
last_pg = pfn_to_page(next_pfn - 1);
- while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
+ while (cur_pg_index <= last_pg_index &&
+ pages_are_mergeable(page, last_pg)) {
if (sgt_append->prv->length + PAGE_SIZE > max_segment)
break;
sgt_append->prv->length += PAGE_SIZE;
- last_pg = pages[0];
- pages++;
- n_pages--;
+ last_pg = page;
+ cur_pg_index++;
}
- if (!n_pages)
+ if (cur_pg_index > last_pg_index)
goto out;
}
}
@@ -500,26 +512,27 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
/* compute number of contiguous chunks */
chunks = 1;
seg_len = 0;
- for (i = 1; i < n_pages; i++) {
+ for (i = cur_pg_index + 1; i <= last_pg_index; i++) {
seg_len += PAGE_SIZE;
if (seg_len >= max_segment ||
- !pages_are_mergeable(pages[i], pages[i - 1])) {
+ !pages_are_mergeable(pages.get_page(pages, i),
+ pages.get_page(pages, i - 1))) {
chunks++;
seg_len = 0;
}
}
/* merging chunks and putting them into the scatterlist */
- cur_page = 0;
for (i = 0; i < chunks; i++) {
unsigned int j, chunk_size;
/* look for the end of the current chunk */
seg_len = 0;
- for (j = cur_page + 1; j < n_pages; j++) {
+ for (j = cur_pg_index + 1; j <= last_pg_index; j++) {
seg_len += PAGE_SIZE;
if (seg_len >= max_segment ||
- !pages_are_mergeable(pages[j], pages[j - 1]))
+ !pages_are_mergeable(pages.get_page(pages, j),
+ pages.get_page(pages, j - 1)))
break;
}
@@ -535,13 +548,13 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
sgt_append->prv->length = prv_len;
return PTR_ERR(s);
}
- chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
- sg_set_page(s, pages[cur_page],
+ chunk_size = ((j - cur_pg_index) << PAGE_SHIFT) - offset;
+ sg_set_page(s, pages.get_page(pages, cur_pg_index),
min_t(unsigned long, size, chunk_size), offset);
added_nents++;
size -= chunk_size;
offset = 0;
- cur_page = j;
+ cur_pg_index = j;
}
sgt_append->sgt.nents += added_nents;
sgt_append->sgt.orig_nents = sgt_append->sgt.nents;
@@ -551,6 +564,46 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
sg_mark_end(s);
return 0;
}
+
+/**
+ * sg_alloc_append_table_from_pages - Allocate and initialize an append sg
+ * table from an array of pages
+ * @sgt_append: The sg append table to use
+ * @pages: Pointer to an array of page pointers
+ * @n_pages: Number of pages in the pages array
+ * @offset: Offset from start of the first page to the start of a buffer
+ * @size: Number of valid bytes in the buffer (after offset)
+ * @max_segment: Maximum size of a scatterlist element in bytes
+ * @left_pages: Left pages caller have to set after this call
+ * @gfp_mask: GFP allocation mask
+ *
+ * Description:
+ * In the first call it allocate and initialize an sg table from a list of
+ * pages, else reuse the scatterlist from sgt_append. Contiguous ranges of
+ * the pages are squashed into a single scatterlist entry up to the maximum
+ * size specified in @max_segment. A user may provide an offset at a start
+ * and a size of valid data in a buffer specified by the page array. The
+ * returned sg table is released by sg_free_append_table
+ *
+ * Returns:
+ * 0 on success, negative error on failure
+ *
+ * Notes:
+ * If this function returns non-0 (eg failure), the caller must call
+ * sg_free_append_table() to cleanup any leftover allocations.
+ *
+ * In the fist call, sgt_append must by initialized.
+ */
+int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
+ struct page **pages, unsigned int n_pages, unsigned int offset,
+ unsigned long size, unsigned int max_segment,
+ unsigned int left_pages, gfp_t gfp_mask)
+{
+ struct page_array parray = PAGE_ARRAY(pages);
+
+ return sg_alloc_append_table_from_page_array(sgt_append, parray, 0, n_pages, offset,
+ size, max_segment, left_pages, gfp_mask);
+}
EXPORT_SYMBOL(sg_alloc_append_table_from_pages);
/**
@@ -582,10 +635,11 @@ int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
gfp_t gfp_mask)
{
struct sg_append_table append = {};
+ struct page_array parray = PAGE_ARRAY(pages);
int err;
- err = sg_alloc_append_table_from_pages(&append, pages, n_pages, offset,
- size, max_segment, 0, gfp_mask);
+ err = sg_alloc_append_table_from_page_array(&append, parray, 0, n_pages, offset,
+ size, max_segment, 0, gfp_mask);
if (err) {
sg_free_append_table(&append);
return err;
@@ -596,6 +650,27 @@ int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
}
EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);
+int sg_alloc_table_from_xarray_segment(struct sg_table *sgt, struct xarray *pages,
+ unsigned int idx, unsigned int n_pages,
+ unsigned int offset, unsigned long size,
+ unsigned int max_segment, gfp_t gfp_mask)
+{
+ struct sg_append_table append = {};
+ struct page_array parray = PAGE_XARRAY(pages);
+ int err;
+
+ err = sg_alloc_append_table_from_page_array(&append, parray, idx, n_pages, offset,
+ size, max_segment, 0, gfp_mask);
+ if (err) {
+ sg_free_append_table(&append);
+ return err;
+ }
+ memcpy(sgt, &append.sgt, sizeof(*sgt));
+ WARN_ON(append.total_nents != sgt->orig_nents);
+ return 0;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_xarray_segment);
+
#ifdef CONFIG_SGL_ALLOC
/**
--
2.48.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 1/6] lib/scatterlist.c: Support constructing sgt from page xarray
2025-03-26 2:14 ` [RFC PATCH v2 1/6] lib/scatterlist.c: Support constructing sgt from page xarray Adrián Larumbe
@ 2025-04-01 8:28 ` Boris Brezillon
0 siblings, 0 replies; 16+ messages in thread
From: Boris Brezillon @ 2025-04-01 8:28 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Steven Price, Liviu Dudau, kernel, linux-kernel, dri-devel
On Wed, 26 Mar 2025 02:14:21 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> In preparation for a future commit that will introduce sparse allocation
> of pages in DRM shmem, a scatterlist function that knows how to deal
> with an xarray collection of memory pages had to be introduced.
>
> Because the new function is identical to the existing one that deals
> with a page array, the page_array abstraction is also introduced, which
> hides the way pages are retrieved from a collection.
Let's see how things look/work with an xarray -> array copy step for sgt
building, and we can consider adding an helper for direct xarray array
to the scatterlist lib later on. The reasoning here is to limit the
number of dependencies on non-DRM stuff at first.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
> include/linux/scatterlist.h | 17 ++++
> lib/scatterlist.c | 175 +++++++++++++++++++++++++-----------
> 2 files changed, 142 insertions(+), 50 deletions(-)
>
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index d836e7440ee8..cffb0cffcda0 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -447,6 +447,11 @@ int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
> unsigned int n_pages, unsigned int offset,
> unsigned long size,
> unsigned int max_segment, gfp_t gfp_mask);
> +int sg_alloc_table_from_xarray_segment(struct sg_table *sgt, struct xarray *pages,
> + unsigned int idx, unsigned int n_pages,
> + unsigned int offset, unsigned long size,
> + unsigned int max_segment, gfp_t gfp_mask);
> +
>
> /**
> * sg_alloc_table_from_pages - Allocate and initialize an sg table from
> @@ -478,6 +483,18 @@ static inline int sg_alloc_table_from_pages(struct sg_table *sgt,
> size, UINT_MAX, gfp_mask);
> }
>
> +static inline int sg_alloc_table_from_xarray(struct sg_table *sgt,
> + struct xarray *pages,
> + unsigned int idx,
> + unsigned int n_pages,
> + unsigned int offset,
> + unsigned long size, gfp_t gfp_mask)
> +{
> + return sg_alloc_table_from_xarray_segment(sgt, pages, idx, n_pages, offset,
> + size, UINT_MAX, gfp_mask);
> +}
> +
> +
> #ifdef CONFIG_SGL_ALLOC
> struct scatterlist *sgl_alloc_order(unsigned long long length,
> unsigned int order, bool chainable,
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index 5bb6b8aff232..08b9ed51324e 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -423,43 +423,53 @@ static bool pages_are_mergeable(struct page *a, struct page *b)
> return true;
> }
>
> -/**
> - * sg_alloc_append_table_from_pages - Allocate and initialize an append sg
> - * table from an array of pages
> - * @sgt_append: The sg append table to use
> - * @pages: Pointer to an array of page pointers
> - * @n_pages: Number of pages in the pages array
> - * @offset: Offset from start of the first page to the start of a buffer
> - * @size: Number of valid bytes in the buffer (after offset)
> - * @max_segment: Maximum size of a scatterlist element in bytes
> - * @left_pages: Left pages caller have to set after this call
> - * @gfp_mask: GFP allocation mask
> - *
> - * Description:
> - * In the first call it allocate and initialize an sg table from a list of
> - * pages, else reuse the scatterlist from sgt_append. Contiguous ranges of
> - * the pages are squashed into a single scatterlist entry up to the maximum
> - * size specified in @max_segment. A user may provide an offset at a start
> - * and a size of valid data in a buffer specified by the page array. The
> - * returned sg table is released by sg_free_append_table
> - *
> - * Returns:
> - * 0 on success, negative error on failure
> - *
> - * Notes:
> - * If this function returns non-0 (eg failure), the caller must call
> - * sg_free_append_table() to cleanup any leftover allocations.
> - *
> - * In the fist call, sgt_append must by initialized.
> - */
> -int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
> - struct page **pages, unsigned int n_pages, unsigned int offset,
> - unsigned long size, unsigned int max_segment,
> - unsigned int left_pages, gfp_t gfp_mask)
> +struct page_array {
> + union {
> + struct page **array;
> + struct xarray *xarray;
> + };
> +
> + struct page * (* const get_page)(struct page_array, unsigned int);
> +};
> +
> +static inline struct page *page_array_get_page(struct page_array a,
> + unsigned int index)
> {
> - unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
> + return a.array[index];
> +}
> +
> +static inline struct page *page_xarray_get_page(struct page_array a,
> + unsigned int index)
> +{
> + return xa_load(a.xarray, index);
> +}
> +
> +#define PAGE_ARRAY(pages) \
> + ((struct page_array) { \
> + .array = pages, \
> + .get_page = page_array_get_page, \
> + })
> +
> +#define PAGE_XARRAY(pages) \
> + ((struct page_array) { \
> + .xarray = pages, \
> + .get_page = page_xarray_get_page, \
> + })
> +
> +static inline int
> +sg_alloc_append_table_from_page_array(struct sg_append_table *sgt_append,
> + struct page_array pages,
> + unsigned int first_page,
> + unsigned int n_pages,
> + unsigned int offset, unsigned long size,
> + unsigned int max_segment,
> + unsigned int left_pages, gfp_t gfp_mask)
> +{
> + unsigned int chunks, seg_len, i, prv_len = 0;
> unsigned int added_nents = 0;
> struct scatterlist *s = sgt_append->prv;
> + unsigned int cur_pg_index = first_page;
> + unsigned int last_pg_index = first_page + n_pages - 1;
> struct page *last_pg;
>
> /*
> @@ -475,24 +485,26 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
>
> if (sgt_append->prv) {
> unsigned long next_pfn;
> + struct page *page;
>
> if (WARN_ON(offset))
> return -EINVAL;
>
> /* Merge contiguous pages into the last SG */
> + page = pages.get_page(pages, cur_pg_index);
> prv_len = sgt_append->prv->length;
> next_pfn = (sg_phys(sgt_append->prv) + prv_len) / PAGE_SIZE;
> - if (page_to_pfn(pages[0]) == next_pfn) {
> + if (page_to_pfn(page) == next_pfn) {
> last_pg = pfn_to_page(next_pfn - 1);
> - while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
> + while (cur_pg_index <= last_pg_index &&
> + pages_are_mergeable(page, last_pg)) {
> if (sgt_append->prv->length + PAGE_SIZE > max_segment)
> break;
> sgt_append->prv->length += PAGE_SIZE;
> - last_pg = pages[0];
> - pages++;
> - n_pages--;
> + last_pg = page;
> + cur_pg_index++;
> }
> - if (!n_pages)
> + if (cur_pg_index > last_pg_index)
> goto out;
> }
> }
> @@ -500,26 +512,27 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
> /* compute number of contiguous chunks */
> chunks = 1;
> seg_len = 0;
> - for (i = 1; i < n_pages; i++) {
> + for (i = cur_pg_index + 1; i <= last_pg_index; i++) {
> seg_len += PAGE_SIZE;
> if (seg_len >= max_segment ||
> - !pages_are_mergeable(pages[i], pages[i - 1])) {
> + !pages_are_mergeable(pages.get_page(pages, i),
> + pages.get_page(pages, i - 1))) {
> chunks++;
> seg_len = 0;
> }
> }
>
> /* merging chunks and putting them into the scatterlist */
> - cur_page = 0;
> for (i = 0; i < chunks; i++) {
> unsigned int j, chunk_size;
>
> /* look for the end of the current chunk */
> seg_len = 0;
> - for (j = cur_page + 1; j < n_pages; j++) {
> + for (j = cur_pg_index + 1; j <= last_pg_index; j++) {
> seg_len += PAGE_SIZE;
> if (seg_len >= max_segment ||
> - !pages_are_mergeable(pages[j], pages[j - 1]))
> + !pages_are_mergeable(pages.get_page(pages, j),
> + pages.get_page(pages, j - 1)))
> break;
> }
>
> @@ -535,13 +548,13 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
> sgt_append->prv->length = prv_len;
> return PTR_ERR(s);
> }
> - chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
> - sg_set_page(s, pages[cur_page],
> + chunk_size = ((j - cur_pg_index) << PAGE_SHIFT) - offset;
> + sg_set_page(s, pages.get_page(pages, cur_pg_index),
> min_t(unsigned long, size, chunk_size), offset);
> added_nents++;
> size -= chunk_size;
> offset = 0;
> - cur_page = j;
> + cur_pg_index = j;
> }
> sgt_append->sgt.nents += added_nents;
> sgt_append->sgt.orig_nents = sgt_append->sgt.nents;
> @@ -551,6 +564,46 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
> sg_mark_end(s);
> return 0;
> }
> +
> +/**
> + * sg_alloc_append_table_from_pages - Allocate and initialize an append sg
> + * table from an array of pages
> + * @sgt_append: The sg append table to use
> + * @pages: Pointer to an array of page pointers
> + * @n_pages: Number of pages in the pages array
> + * @offset: Offset from start of the first page to the start of a buffer
> + * @size: Number of valid bytes in the buffer (after offset)
> + * @max_segment: Maximum size of a scatterlist element in bytes
> + * @left_pages: Left pages caller have to set after this call
> + * @gfp_mask: GFP allocation mask
> + *
> + * Description:
> + * In the first call it allocate and initialize an sg table from a list of
> + * pages, else reuse the scatterlist from sgt_append. Contiguous ranges of
> + * the pages are squashed into a single scatterlist entry up to the maximum
> + * size specified in @max_segment. A user may provide an offset at a start
> + * and a size of valid data in a buffer specified by the page array. The
> + * returned sg table is released by sg_free_append_table
> + *
> + * Returns:
> + * 0 on success, negative error on failure
> + *
> + * Notes:
> + * If this function returns non-0 (eg failure), the caller must call
> + * sg_free_append_table() to cleanup any leftover allocations.
> + *
> + * In the fist call, sgt_append must by initialized.
> + */
> +int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
> + struct page **pages, unsigned int n_pages, unsigned int offset,
> + unsigned long size, unsigned int max_segment,
> + unsigned int left_pages, gfp_t gfp_mask)
> +{
> + struct page_array parray = PAGE_ARRAY(pages);
> +
> + return sg_alloc_append_table_from_page_array(sgt_append, parray, 0, n_pages, offset,
> + size, max_segment, left_pages, gfp_mask);
> +}
> EXPORT_SYMBOL(sg_alloc_append_table_from_pages);
>
> /**
> @@ -582,10 +635,11 @@ int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
> gfp_t gfp_mask)
> {
> struct sg_append_table append = {};
> + struct page_array parray = PAGE_ARRAY(pages);
> int err;
>
> - err = sg_alloc_append_table_from_pages(&append, pages, n_pages, offset,
> - size, max_segment, 0, gfp_mask);
> + err = sg_alloc_append_table_from_page_array(&append, parray, 0, n_pages, offset,
> + size, max_segment, 0, gfp_mask);
> if (err) {
> sg_free_append_table(&append);
> return err;
> @@ -596,6 +650,27 @@ int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
> }
> EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);
>
> +int sg_alloc_table_from_xarray_segment(struct sg_table *sgt, struct xarray *pages,
> + unsigned int idx, unsigned int n_pages,
> + unsigned int offset, unsigned long size,
> + unsigned int max_segment, gfp_t gfp_mask)
> +{
> + struct sg_append_table append = {};
> + struct page_array parray = PAGE_XARRAY(pages);
> + int err;
> +
> + err = sg_alloc_append_table_from_page_array(&append, parray, idx, n_pages, offset,
> + size, max_segment, 0, gfp_mask);
> + if (err) {
> + sg_free_append_table(&append);
> + return err;
> + }
> + memcpy(sgt, &append.sgt, sizeof(*sgt));
> + WARN_ON(append.total_nents != sgt->orig_nents);
> + return 0;
> +}
> +EXPORT_SYMBOL(sg_alloc_table_from_xarray_segment);
> +
> #ifdef CONFIG_SGL_ALLOC
>
> /**
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v2 2/6] drm/shmem: Introduce the notion of sparse objects
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
2025-03-26 2:14 ` [RFC PATCH v2 1/6] lib/scatterlist.c: Support constructing sgt from page xarray Adrián Larumbe
@ 2025-03-26 2:14 ` Adrián Larumbe
2025-04-01 8:45 ` Boris Brezillon
2025-03-26 2:14 ` [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects Adrián Larumbe
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Adrián Larumbe @ 2025-03-26 2:14 UTC (permalink / raw)
To: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, Adrián Larumbe, linux-kernel, dri-devel
Sparse DRM objects will store their backing pages in an xarray, to avoid
the overhead of preallocating a huge struct page pointer array when only
a very small range of indices might be assigned.
For now, only the definition of a sparse object as a union alternative
to a 'dense' object is provided, with functions that exploit it being
part of later commits.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 68 +++++++++++++++++++++++++-
include/drm/drm_gem_shmem_helper.h | 23 ++++++++-
2 files changed, 88 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index d99dee67353a..5f75eb1230f6 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -128,6 +128,31 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
+/**
+ * drm_gem_shmem_create_sparse - Allocate a sparse object with the given size
+ * @dev: DRM device
+ * @size: Size of the sparse object to allocate
+ *
+ * This function creates a sparse shmem GEM object.
+ *
+ * Returns:
+ * A struct drm_gem_shmem_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
+ */
+struct drm_gem_shmem_object *drm_gem_shmem_create_sparse(struct drm_device *dev, size_t size)
+{
+ struct drm_gem_shmem_object *shmem =
+ __drm_gem_shmem_create(dev, size, false, NULL);
+
+ if (!IS_ERR(shmem)) {
+ shmem->sparse = true;
+ xa_init_flags(&shmem->xapages, XA_FLAGS_ALLOC);
+ }
+
+ return shmem;
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_create_sparse);
+
/**
* drm_gem_shmem_create_with_mnt - Allocate an object with the given size in a
* given mountpoint
@@ -173,8 +198,8 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
}
- if (shmem->pages)
- drm_gem_shmem_put_pages(shmem);
+
+ drm_gem_shmem_put_pages(shmem);
drm_WARN_ON(obj->dev, shmem->pages_use_count);
@@ -196,6 +221,12 @@ static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
if (shmem->pages_use_count++ > 0)
return 0;
+ /* We only allow increasing the user count in the case of
+ * sparse shmem objects with some backed pages for now
+ */
+ if (shmem->sparse && xa_empty(&shmem->xapages))
+ return -EINVAL;
+
pages = drm_gem_get_pages(obj);
if (IS_ERR(pages)) {
drm_dbg_kms(obj->dev, "Failed to get pages (%ld)\n",
@@ -231,6 +262,14 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
dma_resv_assert_held(shmem->base.resv);
+ if (!shmem->sparse) {
+ if (!shmem->pages)
+ return;
+ } else {
+ /* Not implemented yet */
+ return;
+ }
+
if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_use_count))
return;
@@ -404,8 +443,15 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
{
struct drm_gem_object *obj = &shmem->base;
+ if (shmem->sparse) {
+ drm_err(obj->dev, "UM unmapping of sparse shmem objects not implemented\n");
+ return;
+ }
+
if (drm_gem_is_imported(obj)) {
dma_buf_vunmap(obj->dma_buf, map);
+ } else if (obj->import_attach) {
+ dma_buf_vunmap(obj->import_attach->dmabuf, map);
} else {
dma_resv_assert_held(shmem->base.resv);
@@ -541,6 +587,12 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
struct page *page;
pgoff_t page_offset;
+ /* TODO: Implement UM mapping of sparse shmem objects */
+ if (drm_WARN_ON(obj->dev, shmem->sparse)) {
+ drm_err(obj->dev, "UM mapping of sparse shmem objects not implemented\n");
+ return VM_FAULT_SIGBUS;
+ }
+
/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
@@ -566,8 +618,14 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma)
struct drm_gem_object *obj = vma->vm_private_data;
struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ /* TODO: Implement UM mapping of sparse shmem objects */
+ if (drm_WARN_ON(obj->dev, shmem->sparse))
+ return;
+
drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));
+ drm_WARN_ON(obj->dev, obj->import_attach);
+
dma_resv_lock(shmem->base.resv, NULL);
/*
@@ -690,6 +748,9 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
+ if (drm_WARN_ON(obj->dev, shmem->sparse))
+ return ERR_PTR(-EINVAL);
+
drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));
return drm_prime_pages_to_sg(obj->dev, shmem->pages, obj->size >> PAGE_SHIFT);
@@ -702,6 +763,9 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
int ret;
struct sg_table *sgt;
+ if (drm_WARN_ON(obj->dev, shmem->sparse))
+ return ERR_PTR(-EINVAL);
+
if (shmem->sgt)
return shmem->sgt;
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index cef5a6b5a4d6..00e47512b30f 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -6,6 +6,7 @@
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/mutex.h>
+#include <linux/xarray.h>
#include <drm/drm_file.h>
#include <drm/drm_gem.h>
@@ -29,7 +30,10 @@ struct drm_gem_shmem_object {
/**
* @pages: Page table
*/
- struct page **pages;
+ union {
+ struct page **pages;
+ struct xarray xapages;
+ };
/**
* @pages_use_count:
@@ -91,12 +95,18 @@ struct drm_gem_shmem_object {
* @map_wc: map object write-combined (instead of using shmem defaults).
*/
bool map_wc : 1;
+
+ /**
+ * @sparse: the object is only partially backed by pages
+ */
+ bool sparse : 1;
};
#define to_drm_gem_shmem_obj(obj) \
container_of(obj, struct drm_gem_shmem_object, base)
struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
+struct drm_gem_shmem_object *drm_gem_shmem_create_sparse(struct drm_device *dev, size_t size);
struct drm_gem_shmem_object *drm_gem_shmem_create_with_mnt(struct drm_device *dev,
size_t size,
struct vfsmount *gemfs);
@@ -210,6 +220,10 @@ static inline struct sg_table *drm_gem_shmem_object_get_sg_table(struct drm_gem_
{
struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ /* Use the specific sparse shmem get_sg_table function instead */
+ if (WARN_ON(shmem->sparse))
+ return ERR_PTR(-EINVAL);
+
return drm_gem_shmem_get_sg_table(shmem);
}
@@ -229,6 +243,10 @@ static inline int drm_gem_shmem_object_vmap(struct drm_gem_object *obj,
{
struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ /* TODO: Implement kernel mapping of sparse shmem objects */
+ if (WARN_ON(shmem->sparse))
+ return -EACCES;
+
return drm_gem_shmem_vmap(shmem, map);
}
@@ -263,6 +281,9 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v
{
struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ if (shmem->sparse)
+ return -EACCES;
+
return drm_gem_shmem_mmap(shmem, vma);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 2/6] drm/shmem: Introduce the notion of sparse objects
2025-03-26 2:14 ` [RFC PATCH v2 2/6] drm/shmem: Introduce the notion of sparse objects Adrián Larumbe
@ 2025-04-01 8:45 ` Boris Brezillon
0 siblings, 0 replies; 16+ messages in thread
From: Boris Brezillon @ 2025-04-01 8:45 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Steven Price, Liviu Dudau, kernel, linux-kernel, dri-devel
On Wed, 26 Mar 2025 02:14:22 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> Sparse DRM objects will store their backing pages in an xarray, to avoid
> the overhead of preallocating a huge struct page pointer array when only
> a very small range of indices might be assigned.
>
> For now, only the definition of a sparse object as a union alternative
> to a 'dense' object is provided, with functions that exploit it being
> part of later commits.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 68 +++++++++++++++++++++++++-
> include/drm/drm_gem_shmem_helper.h | 23 ++++++++-
> 2 files changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index d99dee67353a..5f75eb1230f6 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -128,6 +128,31 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>
> +/**
> + * drm_gem_shmem_create_sparse - Allocate a sparse object with the given size
> + * @dev: DRM device
> + * @size: Size of the sparse object to allocate
> + *
> + * This function creates a sparse shmem GEM object.
> + *
> + * Returns:
> + * A struct drm_gem_shmem_object * on success or an ERR_PTR()-encoded negative
> + * error code on failure.
> + */
> +struct drm_gem_shmem_object *drm_gem_shmem_create_sparse(struct drm_device *dev, size_t size)
> +{
> + struct drm_gem_shmem_object *shmem =
> + __drm_gem_shmem_create(dev, size, false, NULL);
> +
> + if (!IS_ERR(shmem)) {
> + shmem->sparse = true;
> + xa_init_flags(&shmem->xapages, XA_FLAGS_ALLOC);
> + }
> +
> + return shmem;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_create_sparse);
> +
> /**
> * drm_gem_shmem_create_with_mnt - Allocate an object with the given size in a
> * given mountpoint
> @@ -173,8 +198,8 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> }
> - if (shmem->pages)
> - drm_gem_shmem_put_pages(shmem);
> +
> + drm_gem_shmem_put_pages(shmem);
>
> drm_WARN_ON(obj->dev, shmem->pages_use_count);
>
> @@ -196,6 +221,12 @@ static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
> if (shmem->pages_use_count++ > 0)
> return 0;
>
> + /* We only allow increasing the user count in the case of
> + * sparse shmem objects with some backed pages for now
> + */
> + if (shmem->sparse && xa_empty(&shmem->xapages))
> + return -EINVAL;
> +
> pages = drm_gem_get_pages(obj);
> if (IS_ERR(pages)) {
> drm_dbg_kms(obj->dev, "Failed to get pages (%ld)\n",
> @@ -231,6 +262,14 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
>
> dma_resv_assert_held(shmem->base.resv);
>
> + if (!shmem->sparse) {
> + if (!shmem->pages)
> + return;
> + } else {
> + /* Not implemented yet */
> + return;
> + }
> +
> if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_use_count))
> return;
>
> @@ -404,8 +443,15 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
> {
> struct drm_gem_object *obj = &shmem->base;
>
> + if (shmem->sparse) {
> + drm_err(obj->dev, "UM unmapping of sparse shmem objects not implemented\n");
> + return;
> + }
> +
> if (drm_gem_is_imported(obj)) {
> dma_buf_vunmap(obj->dma_buf, map);
> + } else if (obj->import_attach) {
> + dma_buf_vunmap(obj->import_attach->dmabuf, map);
> } else {
> dma_resv_assert_held(shmem->base.resv);
>
> @@ -541,6 +587,12 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> struct page *page;
> pgoff_t page_offset;
>
> + /* TODO: Implement UM mapping of sparse shmem objects */
> + if (drm_WARN_ON(obj->dev, shmem->sparse)) {
> + drm_err(obj->dev, "UM mapping of sparse shmem objects not implemented\n");
> + return VM_FAULT_SIGBUS;
> + }
> +
> /* We don't use vmf->pgoff since that has the fake offset */
> page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>
> @@ -566,8 +618,14 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma)
> struct drm_gem_object *obj = vma->vm_private_data;
> struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>
> + /* TODO: Implement UM mapping of sparse shmem objects */
> + if (drm_WARN_ON(obj->dev, shmem->sparse))
> + return;
> +
> drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));
>
> + drm_WARN_ON(obj->dev, obj->import_attach);
> +
> dma_resv_lock(shmem->base.resv, NULL);
>
> /*
> @@ -690,6 +748,9 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem)
> {
> struct drm_gem_object *obj = &shmem->base;
>
> + if (drm_WARN_ON(obj->dev, shmem->sparse))
> + return ERR_PTR(-EINVAL);
> +
> drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));
>
> return drm_prime_pages_to_sg(obj->dev, shmem->pages, obj->size >> PAGE_SHIFT);
> @@ -702,6 +763,9 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
> int ret;
> struct sg_table *sgt;
>
> + if (drm_WARN_ON(obj->dev, shmem->sparse))
> + return ERR_PTR(-EINVAL);
> +
> if (shmem->sgt)
> return shmem->sgt;
>
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index cef5a6b5a4d6..00e47512b30f 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -6,6 +6,7 @@
> #include <linux/fs.h>
> #include <linux/mm.h>
> #include <linux/mutex.h>
> +#include <linux/xarray.h>
>
> #include <drm/drm_file.h>
> #include <drm/drm_gem.h>
> @@ -29,7 +30,10 @@ struct drm_gem_shmem_object {
> /**
> * @pages: Page table
> */
> - struct page **pages;
> + union {
> + struct page **pages;
> + struct xarray xapages;
> + };
I've played with this code, and I must admit I'm not a huge fan of this
union. It's just super easy to forget an
if (is_xarray) do_this else do_that
in one of the path, and end up accessing the wrong type without even
noticing (or noticing late).
I'd rather go for an drm_gem_shmem_sparse_backing object, and have an
optional pointer to this sparse_backing object in drm_gem_shmem_object.
I actually tried this approach here [1], and it seems to work.
>
> /**
> * @pages_use_count:
> @@ -91,12 +95,18 @@ struct drm_gem_shmem_object {
> * @map_wc: map object write-combined (instead of using shmem defaults).
> */
> bool map_wc : 1;
> +
> + /**
> + * @sparse: the object is only partially backed by pages
> + */
> + bool sparse : 1;
> };
>
> #define to_drm_gem_shmem_obj(obj) \
> container_of(obj, struct drm_gem_shmem_object, base)
>
> struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
> +struct drm_gem_shmem_object *drm_gem_shmem_create_sparse(struct drm_device *dev, size_t size);
> struct drm_gem_shmem_object *drm_gem_shmem_create_with_mnt(struct drm_device *dev,
> size_t size,
> struct vfsmount *gemfs);
> @@ -210,6 +220,10 @@ static inline struct sg_table *drm_gem_shmem_object_get_sg_table(struct drm_gem_
> {
> struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>
> + /* Use the specific sparse shmem get_sg_table function instead */
> + if (WARN_ON(shmem->sparse))
> + return ERR_PTR(-EINVAL);
> +
> return drm_gem_shmem_get_sg_table(shmem);
> }
>
> @@ -229,6 +243,10 @@ static inline int drm_gem_shmem_object_vmap(struct drm_gem_object *obj,
> {
> struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>
> + /* TODO: Implement kernel mapping of sparse shmem objects */
> + if (WARN_ON(shmem->sparse))
> + return -EACCES;
> +
Okay, this is where things start to get messy. Sparse objects support a
subset of the features supported by regular shmem objects, which
not only makes the code error-prone, but makes it hard for people to
guess what a vmap/get_pages/sgt/... implementation should do. I think
we should define it right now, and IMO, the simpler is to just assume
that sparse objects, when operated as regular objects, provide the same
functionality. That is:
- vmap will populate all pages, pin them, and vmap them
- get_pages will populate all pages
- get_sgt will populate all pages and instantiate an sgt covering the
whole GEM
- ...
Of course, this means that sparse objects are pointless when operated as
regular objects, but it makes them safe to use, and if we want to
discourage people to call vmap/get_pages/... on a sparse object, we can
always add a drm_warn() in those places.
> return drm_gem_shmem_vmap(shmem, map);
> }
>
> @@ -263,6 +281,9 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v
> {
> struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>
> + if (shmem->sparse)
> + return -EACCES;
> +
> return drm_gem_shmem_mmap(shmem, vma);
> }
>
[1]https://gitlab.freedesktop.org/panfrost/linux/-/merge_requests/16/diffs?commit_id=ec08f6c7a728bc6bfc8031ab5fd67ffe92453726
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
2025-03-26 2:14 ` [RFC PATCH v2 1/6] lib/scatterlist.c: Support constructing sgt from page xarray Adrián Larumbe
2025-03-26 2:14 ` [RFC PATCH v2 2/6] drm/shmem: Introduce the notion of sparse objects Adrián Larumbe
@ 2025-03-26 2:14 ` Adrián Larumbe
2025-03-26 19:54 ` Dmitry Osipenko
2025-04-01 9:36 ` Boris Brezillon
2025-03-26 2:14 ` [RFC PATCH v2 4/6] drm/panfrost: Use shmem sparse allocation for heap BOs Adrián Larumbe
` (3 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Adrián Larumbe @ 2025-03-26 2:14 UTC (permalink / raw)
To: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, Adrián Larumbe, linux-kernel, dri-devel
Add a new function that lets drivers allocate pages for a subset of the
shmem object's virtual address range, and another function for obtaining
an SG table from those pages, so that memory can be mapped onto an MMU.
Add also a new function for putting the pages of a sparse page array.
The sparse allocation function allowed a gfp argument to allow
allocations other than GFP_KERNEL, in cases where memory allocation can
race with the shrinker's memory reclaim path
There is factorization potential with drm_gem_put_pages and
drm_get_pages_, but it is yet to be decided what this should look like.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
drivers/gpu/drm/drm_gem.c | 117 ++++++++++++++++
drivers/gpu/drm/drm_gem_shmem_helper.c | 182 ++++++++++++++++++++++++-
include/drm/drm_gem.h | 6 +
include/drm/drm_gem_shmem_helper.h | 4 +
4 files changed, 303 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index c6240bab3fa5..fa9b3f01f9ac 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -679,6 +679,123 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
}
EXPORT_SYMBOL(drm_gem_put_pages);
+/**
+ * drm_get_pages_xarray - helper to allocate backing pages for a GEM object
+ * from shmem, and store them in an xarray.
+ * @obj: obj in question
+ * @pa: xarray that holds the backing pages
+ * @page_offset: shmem index of the very first page to allocate
+ * @npages: number of consecutive shmem pages to allocate
+ * @gfp: additional allocation flags
+ *
+ * This reads the page-array of the shmem-backing storage of the given gem
+ * object. The input xarray is where the pages are stored. If a page is not
+ * allocated or swapped-out, this will allocate/swap-in the required pages.
+ * Only the requested range is covered with physical pages.
+ *
+ * Use drm_gem_put_xarray_page_range() to release the same xarray subset of pages.
+ *
+ * This uses the GFP-mask set on the shmem-mapping (see mapping_set_gfp_mask()),
+ * and any further mask bits set in the gfp input parameter.
+ *
+ * This function is only valid on objects initialized with
+ * drm_gem_object_init(), but not for those initialized with
+ * drm_gem_private_object_init() only.
+ */
+int drm_get_pages_xarray(struct drm_gem_object *obj, struct xarray *pa,
+ pgoff_t page_offset, unsigned int npages, gfp_t gfp)
+{
+ struct address_space *mapping;
+ struct page *page;
+ int ret = 0;
+ int i;
+
+ if (WARN_ON(!obj->filp))
+ return -EINVAL;
+
+ /* This is the shared memory object that backs the GEM resource */
+ mapping = obj->filp->f_mapping;
+
+ /* We already BUG_ON() for non-page-aligned sizes in
+ * drm_gem_object_init(), so we should never hit this unless
+ * driver author is doing something really wrong:
+ */
+ WARN_ON((obj->size & (PAGE_SIZE - 1)) != 0);
+
+ mapping = obj->filp->f_mapping;
+ mapping_set_unevictable(mapping);
+
+ for (i = 0; i < npages; i++) {
+ page = shmem_read_mapping_page_gfp(mapping, page_offset + i,
+ mapping_gfp_mask(mapping) | gfp);
+ if (IS_ERR(page)) {
+ ret = PTR_ERR(page);
+ goto err_free_pages;
+ }
+
+ /* Add the page into the xarray */
+ ret = xa_err(xa_store(pa, page_offset + i, page, gfp));
+ if (ret) {
+ put_page(page);
+ goto err_free_pages;
+ }
+ }
+
+ return ret;
+
+err_free_pages:
+ while (--i) {
+ page = xa_erase(pa, page_offset + i);
+ if (drm_WARN_ON(obj->dev, !page))
+ continue;
+ put_page(page);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_get_pages_xarray);
+
+/**
+ * drm_gem_put_xarray_page_range - helper to free some backing pages for a
+ * sparse GEM object
+ * @pa: xarray that holds the backing pages
+ * @idx: xarray index of the first page tof ree
+ * @npages: number of consecutive pages in the xarray to free
+ * @dirty: if true, pages will be marked as dirty
+ * @accessed: if true, the pages will be marked as accessed
+ */
+void drm_gem_put_xarray_page_range(struct xarray *pa, unsigned long idx,
+ unsigned int npages, bool dirty, bool accessed)
+{
+ struct folio_batch fbatch;
+ struct page *page;
+
+ folio_batch_init(&fbatch);
+
+ xa_for_each(pa, idx, page) {
+ struct folio *folio = page_folio(page);
+
+ if (dirty)
+ folio_mark_dirty(folio);
+ if (accessed)
+ folio_mark_accessed(folio);
+
+ /* Undo the reference we took when populating the table */
+ if (!folio_batch_add(&fbatch, folio))
+ drm_gem_check_release_batch(&fbatch);
+
+ xa_erase(pa, idx);
+
+ idx += folio_nr_pages(folio) - 1;
+ }
+
+ if (folio_batch_count(&fbatch))
+ drm_gem_check_release_batch(&fbatch);
+
+ WARN_ON((idx+1) != npages);
+}
+EXPORT_SYMBOL(drm_gem_put_xarray_page_range);
+
static int objects_lookup(struct drm_file *filp, u32 *handle, int count,
struct drm_gem_object **objs)
{
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 5f75eb1230f6..1bf33e5a1c4c 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -174,6 +174,34 @@ struct drm_gem_shmem_object *drm_gem_shmem_create_with_mnt(struct drm_device *de
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_create_with_mnt);
+static void drm_gem_shmem_put_pages_sparse(struct drm_gem_shmem_object *shmem)
+{
+ struct page *page;
+ unsigned long idx;
+
+ if (drm_WARN_ON(shmem->base.dev, !shmem->sparse))
+ return;
+
+ idx = 0;
+ xa_for_each(&shmem->xapages, idx, page) {
+ unsigned long consecutive = 1;
+
+ if (!page)
+ continue;
+
+ while (xa_load(&shmem->xapages, idx + consecutive))
+ consecutive++;
+
+ drm_gem_put_xarray_page_range(&shmem->xapages, idx, consecutive,
+ shmem->pages_mark_dirty_on_put,
+ shmem->pages_mark_accessed_on_put);
+
+ idx += consecutive;
+ }
+
+ drm_WARN_ON(shmem->base.dev, !xa_empty(&shmem->xapages));
+}
+
/**
* drm_gem_shmem_free - Free resources associated with a shmem GEM object
* @shmem: shmem GEM object to free
@@ -266,8 +294,8 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
if (!shmem->pages)
return;
} else {
- /* Not implemented yet */
- return;
+ if (xa_empty(&shmem->xapages))
+ return;
}
if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_use_count))
@@ -281,10 +309,15 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT);
#endif
- drm_gem_put_pages(obj, shmem->pages,
- shmem->pages_mark_dirty_on_put,
- shmem->pages_mark_accessed_on_put);
- shmem->pages = NULL;
+ if (!shmem->sparse) {
+ drm_gem_put_pages(obj, shmem->pages,
+ shmem->pages_mark_dirty_on_put,
+ shmem->pages_mark_accessed_on_put);
+ shmem->pages = NULL;
+ } else {
+ drm_gem_shmem_put_pages_sparse(shmem);
+ xa_destroy(&shmem->xapages);
+ }
}
EXPORT_SYMBOL(drm_gem_shmem_put_pages);
@@ -797,6 +830,103 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
return ERR_PTR(ret);
}
+static int
+drm_gem_shmem_sparse_populate_locked(struct drm_gem_shmem_object *shmem,
+ unsigned int n_pages, pgoff_t page_offset,
+ gfp_t gfp)
+{
+ bool first_alloc;
+ int ret;
+
+ if (!shmem->sparse)
+ return -EINVAL;
+
+ dma_resv_assert_held(shmem->base.resv);
+
+ /* If the mapping exists, then bail out immediately */
+ if (xa_load(&shmem->xapages, page_offset) != NULL)
+ return -EEXIST;
+
+ first_alloc = xa_empty(&shmem->xapages);
+
+ ret = drm_get_pages_xarray(&shmem->base, &shmem->xapages,
+ page_offset, n_pages, gfp);
+ if (ret)
+ return ret;
+
+ if (first_alloc)
+ shmem->pages_use_count = 1;
+
+ return 0;
+}
+
+static struct sg_table *
+drm_gem_shmem_sparse_get_sgt_range(struct drm_gem_shmem_object *shmem,
+ unsigned int n_pages, pgoff_t page_offset,
+ gfp_t gfp)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct sg_table *sgt;
+ int ret;
+
+ if (drm_WARN_ON(obj->dev, !shmem->sparse))
+ return ERR_PTR(-EINVAL);
+
+ /* If the page range wasn't allocated, then bail out immediately */
+ if (xa_load(&shmem->xapages, page_offset) == NULL)
+ return ERR_PTR(-EINVAL);
+
+ sgt = kzalloc(sizeof(*sgt), GFP_NOWAIT);
+ if (!sgt)
+ return ERR_PTR(-ENOMEM);
+
+ ret = sg_alloc_table_from_xarray(sgt, &shmem->xapages, page_offset,
+ n_pages, 0, n_pages * PAGE_SIZE, gfp);
+ if (ret)
+ goto err_free_sgtable;
+
+ ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
+ if (ret)
+ goto err_free_sgtable;
+
+ return sgt;
+
+err_free_sgtable:
+ kfree(sgt);
+ return ERR_PTR(ret);
+}
+
+static struct sg_table *
+drm_gem_shmem_get_sparse_pages_locked(struct drm_gem_shmem_object *shmem,
+ unsigned int n_pages, pgoff_t page_offset,
+ gfp_t gfp)
+{
+ struct sg_table *sgt;
+ int ret;
+
+ if (!shmem->sparse)
+ return ERR_PTR(-EINVAL);
+
+ dma_resv_assert_held(shmem->base.resv);
+
+ ret = drm_gem_shmem_sparse_populate_locked(shmem, n_pages, page_offset, gfp);
+ if (ret)
+ return ERR_PTR(ret);
+
+ sgt = drm_gem_shmem_sparse_get_sgt_range(shmem, n_pages, page_offset, gfp);
+ if (IS_ERR(sgt)) {
+ ret = PTR_ERR(sgt);
+ goto err_free_pages;
+ }
+
+ return sgt;
+
+err_free_pages:
+ drm_gem_put_xarray_page_range(&shmem->xapages, page_offset,
+ n_pages, false, false);
+ return ERR_PTR(ret);
+}
+
/**
* drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a
* scatter/gather table for a shmem GEM object.
@@ -828,6 +958,46 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt);
+/**
+ * drm_gem_shmem_get_sparse_pages_sgt - Pin pages, dma map them, and return a
+ * scatter/gather table for a sparse shmem GEM object.
+ * @shmem: shmem GEM object
+ * @n_pages: number of pages to pin and map
+ * @page_offset: shmem file index of the first page to allocate and map
+ * @gfp: Further allocation flags
+ *
+ * This function conceptually does the same thing as drm_gem_shmem_get_pages_sgt,
+ * but only for a contiguous subset of pages from the underlying shmem file.
+ * The allocation flags allows users to allocate pages with a mask other than
+ * GFP_KERNEL, in cases where it can race with shmem shrinkers.
+ *
+ * Returns:
+ * A pointer to the scatter/gather table of pinned pages or errno on failure.
+ */
+struct sg_table *
+drm_gem_shmem_get_sparse_pages_sgt(struct drm_gem_shmem_object *shmem,
+ unsigned int n_pages, pgoff_t page_offset,
+ gfp_t gfp)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct sg_table *sgt;
+ int ret;
+
+ if (drm_WARN_ON(obj->dev, !shmem->sparse))
+ return ERR_PTR(-EINVAL);
+
+ ret = dma_resv_lock(shmem->base.resv, NULL);
+ if (ret)
+ return ERR_PTR(ret);
+
+ sgt = drm_gem_shmem_get_sparse_pages_locked(shmem, n_pages, page_offset, gfp);
+
+ dma_resv_unlock(shmem->base.resv);
+
+ return sgt;
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sparse_pages_sgt);
+
/**
* drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from
* another driver's scatter/gather table of pinned pages
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 2bf893eabb4b..d8288a119bc3 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -39,6 +39,7 @@
#include <linux/dma-resv.h>
#include <linux/list.h>
#include <linux/mutex.h>
+#include <linux/xarray.h>
#include <drm/drm_vma_manager.h>
@@ -534,6 +535,11 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj);
void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
bool dirty, bool accessed);
+int drm_get_pages_xarray(struct drm_gem_object *obj, struct xarray *pa,
+ pgoff_t page_offset, unsigned int npages, gfp_t gfp);
+void drm_gem_put_xarray_page_range(struct xarray *pa, unsigned long idx,
+ unsigned int npages, bool dirty, bool accessed);
+
void drm_gem_lock(struct drm_gem_object *obj);
void drm_gem_unlock(struct drm_gem_object *obj);
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 00e47512b30f..cbe4548e3ff6 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -138,6 +138,10 @@ void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem);
struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem);
struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem);
+struct sg_table *drm_gem_shmem_get_sparse_pages_sgt(struct drm_gem_shmem_object *shmem,
+ unsigned int n_pages, pgoff_t page_offset,
+ gfp_t gfp);
+
void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
struct drm_printer *p, unsigned int indent);
--
2.48.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects
2025-03-26 2:14 ` [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects Adrián Larumbe
@ 2025-03-26 19:54 ` Dmitry Osipenko
2025-04-01 9:36 ` Boris Brezillon
1 sibling, 0 replies; 16+ messages in thread
From: Dmitry Osipenko @ 2025-03-26 19:54 UTC (permalink / raw)
To: Adrián Larumbe, Andrew Morton, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Boris Brezillon, Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, linux-kernel, dri-devel
On 3/26/25 05:14, Adrián Larumbe wrote:
> +static struct sg_table *
> +drm_gem_shmem_sparse_get_sgt_range(struct drm_gem_shmem_object *shmem,
> + unsigned int n_pages, pgoff_t page_offset,
> + gfp_t gfp)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct sg_table *sgt;
> + int ret;
> +
> + if (drm_WARN_ON(obj->dev, !shmem->sparse))
> + return ERR_PTR(-EINVAL);
> +
> + /* If the page range wasn't allocated, then bail out immediately */
> + if (xa_load(&shmem->xapages, page_offset) == NULL)
> + return ERR_PTR(-EINVAL);
> +
> + sgt = kzalloc(sizeof(*sgt), GFP_NOWAIT);
You likely meant to use the gfp arg here.
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects
2025-03-26 2:14 ` [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects Adrián Larumbe
2025-03-26 19:54 ` Dmitry Osipenko
@ 2025-04-01 9:36 ` Boris Brezillon
1 sibling, 0 replies; 16+ messages in thread
From: Boris Brezillon @ 2025-04-01 9:36 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Steven Price, Liviu Dudau, kernel, linux-kernel, dri-devel
On Wed, 26 Mar 2025 02:14:23 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> Add a new function that lets drivers allocate pages for a subset of the
> shmem object's virtual address range, and another function for obtaining
> an SG table from those pages, so that memory can be mapped onto an MMU.
>
> Add also a new function for putting the pages of a sparse page array.
>
> The sparse allocation function allowed a gfp argument to allow
> allocations other than GFP_KERNEL, in cases where memory allocation can
> race with the shrinker's memory reclaim path
>
> There is factorization potential with drm_gem_put_pages and
> drm_get_pages_, but it is yet to be decided what this should look like.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
> drivers/gpu/drm/drm_gem.c | 117 ++++++++++++++++
> drivers/gpu/drm/drm_gem_shmem_helper.c | 182 ++++++++++++++++++++++++-
> include/drm/drm_gem.h | 6 +
> include/drm/drm_gem_shmem_helper.h | 4 +
Let's split this in two commits: one adding the drm_gem helpers, and
the other one adding support for sparse shmem objects.
> 4 files changed, 303 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index c6240bab3fa5..fa9b3f01f9ac 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -679,6 +679,123 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> }
> EXPORT_SYMBOL(drm_gem_put_pages);
>
> +/**
> + * drm_get_pages_xarray - helper to allocate backing pages for a GEM object
> + * from shmem, and store them in an xarray.
> + * @obj: obj in question
> + * @pa: xarray that holds the backing pages
> + * @page_offset: shmem index of the very first page to allocate
> + * @npages: number of consecutive shmem pages to allocate
> + * @gfp: additional allocation flags
> + *
> + * This reads the page-array of the shmem-backing storage of the given gem
> + * object. The input xarray is where the pages are stored. If a page is not
> + * allocated or swapped-out, this will allocate/swap-in the required pages.
> + * Only the requested range is covered with physical pages.
> + *
> + * Use drm_gem_put_xarray_page_range() to release the same xarray subset of pages.
> + *
> + * This uses the GFP-mask set on the shmem-mapping (see mapping_set_gfp_mask()),
> + * and any further mask bits set in the gfp input parameter.
> + *
> + * This function is only valid on objects initialized with
> + * drm_gem_object_init(), but not for those initialized with
> + * drm_gem_private_object_init() only.
> + */
> +int drm_get_pages_xarray(struct drm_gem_object *obj, struct xarray *pa,
> + pgoff_t page_offset, unsigned int npages, gfp_t gfp)
Can we make the names consistent, like drm_gem_{get,put}_page_range().
Same for the arguments we pass to these helpers (same name, same order
for common arguments).
> +{
> + struct address_space *mapping;
> + struct page *page;
> + int ret = 0;
> + int i;
> +
> + if (WARN_ON(!obj->filp))
> + return -EINVAL;
> +
> + /* This is the shared memory object that backs the GEM resource */
> + mapping = obj->filp->f_mapping;
> +
> + /* We already BUG_ON() for non-page-aligned sizes in
> + * drm_gem_object_init(), so we should never hit this unless
> + * driver author is doing something really wrong:
> + */
> + WARN_ON((obj->size & (PAGE_SIZE - 1)) != 0);
> +
> + mapping = obj->filp->f_mapping;
> + mapping_set_unevictable(mapping);
> +
> + for (i = 0; i < npages; i++) {
> + page = shmem_read_mapping_page_gfp(mapping, page_offset + i,
> + mapping_gfp_mask(mapping) | gfp);
I don't think that works. For instance we need something like:
gfp_t page_gfp = mapping_gfp_constraint(mapping, ~__GFP_RECLAIM);
page_gfp |= __GFP_NORETRY | __GFP_WARN;
for non-blocking allocation. But I'm not even sure we should
let the user wants to modify the mapping flags. How about
we just get a gfp_t page_flag from the caller.
> + if (IS_ERR(page)) {
> + ret = PTR_ERR(page);
> + goto err_free_pages;
> + }
> +
> + /* Add the page into the xarray */
> + ret = xa_err(xa_store(pa, page_offset + i, page, gfp));
We probably don't want the same flags for other allocations,
so I'd recommend take a gfp_t other_gfp argument for anything
that's not a shmem page allocation.
> + if (ret) {
> + put_page(page);
> + goto err_free_pages;
> + }
> + }
> +
> + return ret;
> +
> +err_free_pages:
> + while (--i) {
> + page = xa_erase(pa, page_offset + i);
> + if (drm_WARN_ON(obj->dev, !page))
> + continue;
> + put_page(page);
I'd rather use shmem_read_folio_gfp() with a folio_batch to
release the folios that were acquired in the main loop, like we
do in drm_gem_get_pages(). Actually, I'm not sure we want to
release those pages when we populate a GEM range, because
suddenly you have to check if the page was already present, and
only release it if it's been added in this call. Or you have
to reject requests to populate regions that overlap with already
populated regions.
It's simpler if we define a slightly different semantics for
drm_gem_get_page_range(): in case of failure, the pages already
populated are kept, and the user should call drm_gem_put_page_range()
to release those.
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_get_pages_xarray);
> +
> +/**
> + * drm_gem_put_xarray_page_range - helper to free some backing pages for a
> + * sparse GEM object
> + * @pa: xarray that holds the backing pages
> + * @idx: xarray index of the first page tof ree
> + * @npages: number of consecutive pages in the xarray to free
> + * @dirty: if true, pages will be marked as dirty
> + * @accessed: if true, the pages will be marked as accessed
> + */
> +void drm_gem_put_xarray_page_range(struct xarray *pa, unsigned long idx,
> + unsigned int npages, bool dirty, bool accessed)
void drm_gem_put_page_range(struct drm_gem_object *obj, struct xarray *pa,
pgoff_t page_offset, unsigned int npages)
To be consistent with the getter.
> +{
> + struct folio_batch fbatch;
> + struct page *page;
> +
> + folio_batch_init(&fbatch);
> +
You need to call mapping_clear_unevictable() if you want the folios
you return to be evictable again. This becomes problematic because
evictability is per-GEM, so, you can only flag the mapping as
evictable if no page range remains active.
Luckily, the xarray construct has a concept of marker which we can
use to flag an entry as used, and only when the xarray as a whole
has this marker cleared (!xa_marked(pa, PAGE_USED)), we consider the
mapping as evictable again.
The other option is to consider that all pages are always returned
in one go, but that means we can't do partial reclaim of sparse BOs.
Not sure how much we care about this case though.
> + xa_for_each(pa, idx, page) {
If you only want to drop the pages in the idx:idx+npages, you need
to use xa_for_each_range(), and if you want to drop all pages, you
need to change the function name and drop the idx and pages arguments.
> + struct folio *folio = page_folio(page);
> +
> + if (dirty)
> + folio_mark_dirty(folio);
> + if (accessed)
> + folio_mark_accessed(folio);
> +
> + /* Undo the reference we took when populating the table */
> + if (!folio_batch_add(&fbatch, folio))
> + drm_gem_check_release_batch(&fbatch);
> +
> + xa_erase(pa, idx);
You're leaving pages behind if the folio contains more than one page. Not
a big deal if the intent is to destroy the array, but given it's
not something this function controls, I'd be tempted to not take that
chance.
> +
> + idx += folio_nr_pages(folio) - 1;
> + }
> +
> + if (folio_batch_count(&fbatch))
> + drm_gem_check_release_batch(&fbatch);
> +
> + WARN_ON((idx+1) != npages);
How about we allow unmapping a sparsely populate range instead of
complaining here? This would allow one to call
drm_gem_put_page_range(0, UINT_MAX) to mean "drop all pages".
> +}
> +EXPORT_SYMBOL(drm_gem_put_xarray_page_range);
> +
> static int objects_lookup(struct drm_file *filp, u32 *handle, int count,
> struct drm_gem_object **objs)
> {
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 5f75eb1230f6..1bf33e5a1c4c 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -174,6 +174,34 @@ struct drm_gem_shmem_object *drm_gem_shmem_create_with_mnt(struct drm_device *de
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create_with_mnt);
>
> +static void drm_gem_shmem_put_pages_sparse(struct drm_gem_shmem_object *shmem)
> +{
> + struct page *page;
> + unsigned long idx;
> +
> + if (drm_WARN_ON(shmem->base.dev, !shmem->sparse))
> + return;
> +
> + idx = 0;
> + xa_for_each(&shmem->xapages, idx, page) {
> + unsigned long consecutive = 1;
> +
> + if (!page)
> + continue;
> +
> + while (xa_load(&shmem->xapages, idx + consecutive))
> + consecutive++;
> +
> + drm_gem_put_xarray_page_range(&shmem->xapages, idx, consecutive,
> + shmem->pages_mark_dirty_on_put,
> + shmem->pages_mark_accessed_on_put);
It really feels like something the drm_gem helper should deal with, instead
of having the shmem helper iterate over the page array to figure out holes
to skip.
> +
> + idx += consecutive;
> + }
> +
> + drm_WARN_ON(shmem->base.dev, !xa_empty(&shmem->xapages));
> +}
> +
> /**
> * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> * @shmem: shmem GEM object to free
> @@ -266,8 +294,8 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
> if (!shmem->pages)
> return;
> } else {
> - /* Not implemented yet */
> - return;
> + if (xa_empty(&shmem->xapages))
> + return;
> }
>
> if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_use_count))
> @@ -281,10 +309,15 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
> set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT);
> #endif
>
> - drm_gem_put_pages(obj, shmem->pages,
> - shmem->pages_mark_dirty_on_put,
> - shmem->pages_mark_accessed_on_put);
> - shmem->pages = NULL;
> + if (!shmem->sparse) {
> + drm_gem_put_pages(obj, shmem->pages,
> + shmem->pages_mark_dirty_on_put,
> + shmem->pages_mark_accessed_on_put);
> + shmem->pages = NULL;
> + } else {
> + drm_gem_shmem_put_pages_sparse(shmem);
> + xa_destroy(&shmem->xapages);
> + }
> }
> EXPORT_SYMBOL(drm_gem_shmem_put_pages);
>
> @@ -797,6 +830,103 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
> return ERR_PTR(ret);
> }
>
> +static int
> +drm_gem_shmem_sparse_populate_locked(struct drm_gem_shmem_object *shmem,
> + unsigned int n_pages, pgoff_t page_offset,
> + gfp_t gfp)
> +{
> + bool first_alloc;
> + int ret;
> +
> + if (!shmem->sparse)
> + return -EINVAL;
> +
> + dma_resv_assert_held(shmem->base.resv);
One of the use-case of sparse GEM is to allow alloc-on-fault, where we're
not allowed to block on allocation, but also not allowed to take locks
under which blocking allocations might happen. The GEM resv lock is one
falls in that case, meaning locking around sparse gem population is
something that should be left to the driver, not enforced here.
> +
> + /* If the mapping exists, then bail out immediately */
> + if (xa_load(&shmem->xapages, page_offset) != NULL)
> + return -EEXIST;
> +
> + first_alloc = xa_empty(&shmem->xapages);
> +
> + ret = drm_get_pages_xarray(&shmem->base, &shmem->xapages,
> + page_offset, n_pages, gfp);
> + if (ret)
> + return ret;
> +
> + if (first_alloc)
> + shmem->pages_use_count = 1;
I highly recommend that the sparse_backing objects have their own
refcounting, because of the lock issue I mentioned above, and also
because if we decide that sparse GEMs can be used at regular GEMs,
the sparse GEM use/pin_count won't encode the same thing:
- regular GEM count: number of users of a fully mapped sparse GEM
- sparse GEM count: number of users of the sparse GEM, plus one if
the GEM is also used as a regular GEM, because the regular GEM logic
is also considered a user of the sparse GEM resources
> +
> + return 0;
> +}
> +
> +static struct sg_table *
> +drm_gem_shmem_sparse_get_sgt_range(struct drm_gem_shmem_object *shmem,
> + unsigned int n_pages, pgoff_t page_offset,
> + gfp_t gfp)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct sg_table *sgt;
> + int ret;
> +
> + if (drm_WARN_ON(obj->dev, !shmem->sparse))
> + return ERR_PTR(-EINVAL);
> +
> + /* If the page range wasn't allocated, then bail out immediately */
> + if (xa_load(&shmem->xapages, page_offset) == NULL)
> + return ERR_PTR(-EINVAL);
> +
> + sgt = kzalloc(sizeof(*sgt), GFP_NOWAIT);
> + if (!sgt)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = sg_alloc_table_from_xarray(sgt, &shmem->xapages, page_offset,
> + n_pages, 0, n_pages * PAGE_SIZE, gfp);
> + if (ret)
> + goto err_free_sgtable;
> +
> + ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
> + if (ret)
> + goto err_free_sgtable;
> +
> + return sgt;
> +
> +err_free_sgtable:
> + kfree(sgt);
> + return ERR_PTR(ret);
> +}
> +
> +static struct sg_table *
> +drm_gem_shmem_get_sparse_pages_locked(struct drm_gem_shmem_object *shmem,
> + unsigned int n_pages, pgoff_t page_offset,
> + gfp_t gfp)
> +{
> + struct sg_table *sgt;
> + int ret;
> +
> + if (!shmem->sparse)
> + return ERR_PTR(-EINVAL);
> +
> + dma_resv_assert_held(shmem->base.resv);
Same problem I mentioned for pages exist here, because the
sparse sgt will be needed in a path where we not allowed
to take that lock.
> +
> + ret = drm_gem_shmem_sparse_populate_locked(shmem, n_pages, page_offset, gfp);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + sgt = drm_gem_shmem_sparse_get_sgt_range(shmem, n_pages, page_offset, gfp);
> + if (IS_ERR(sgt)) {
> + ret = PTR_ERR(sgt);
> + goto err_free_pages;
> + }
> +
> + return sgt;
> +
> +err_free_pages:
> + drm_gem_put_xarray_page_range(&shmem->xapages, page_offset,
> + n_pages, false, false);
> + return ERR_PTR(ret);
> +}
I'd be in favor of managing the sgts at the drm_gem_shmem_sparse_backing
level too, because AFAICT, all potential sparse GEM users (panfrost,
panthor and lima) need those, and that's probably good if we can
standardize that too.
> +
> /**
> * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a
> * scatter/gather table for a shmem GEM object.
> @@ -828,6 +958,46 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt);
>
> +/**
> + * drm_gem_shmem_get_sparse_pages_sgt - Pin pages, dma map them, and return a
> + * scatter/gather table for a sparse shmem GEM object.
> + * @shmem: shmem GEM object
> + * @n_pages: number of pages to pin and map
> + * @page_offset: shmem file index of the first page to allocate and map
> + * @gfp: Further allocation flags
> + *
> + * This function conceptually does the same thing as drm_gem_shmem_get_pages_sgt,
> + * but only for a contiguous subset of pages from the underlying shmem file.
> + * The allocation flags allows users to allocate pages with a mask other than
> + * GFP_KERNEL, in cases where it can race with shmem shrinkers.
> + *
> + * Returns:
> + * A pointer to the scatter/gather table of pinned pages or errno on failure.
> + */
> +struct sg_table *
> +drm_gem_shmem_get_sparse_pages_sgt(struct drm_gem_shmem_object *shmem,
> + unsigned int n_pages, pgoff_t page_offset,
> + gfp_t gfp)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct sg_table *sgt;
> + int ret;
> +
> + if (drm_WARN_ON(obj->dev, !shmem->sparse))
> + return ERR_PTR(-EINVAL);
> +
> + ret = dma_resv_lock(shmem->base.resv, NULL);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + sgt = drm_gem_shmem_get_sparse_pages_locked(shmem, n_pages, page_offset, gfp);
> +
> + dma_resv_unlock(shmem->base.resv);
> +
> + return sgt;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sparse_pages_sgt);
> +
> /**
> * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from
> * another driver's scatter/gather table of pinned pages
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 2bf893eabb4b..d8288a119bc3 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -39,6 +39,7 @@
> #include <linux/dma-resv.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> +#include <linux/xarray.h>
>
> #include <drm/drm_vma_manager.h>
>
> @@ -534,6 +535,11 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj);
> void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> bool dirty, bool accessed);
>
> +int drm_get_pages_xarray(struct drm_gem_object *obj, struct xarray *pa,
> + pgoff_t page_offset, unsigned int npages, gfp_t gfp);
> +void drm_gem_put_xarray_page_range(struct xarray *pa, unsigned long idx,
> + unsigned int npages, bool dirty, bool accessed);
> +
> void drm_gem_lock(struct drm_gem_object *obj);
> void drm_gem_unlock(struct drm_gem_object *obj);
>
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 00e47512b30f..cbe4548e3ff6 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -138,6 +138,10 @@ void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem);
> struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem);
> struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem);
>
> +struct sg_table *drm_gem_shmem_get_sparse_pages_sgt(struct drm_gem_shmem_object *shmem,
> + unsigned int n_pages, pgoff_t page_offset,
> + gfp_t gfp);
> +
> void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
> struct drm_printer *p, unsigned int indent);
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v2 4/6] drm/panfrost: Use shmem sparse allocation for heap BOs
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
` (2 preceding siblings ...)
2025-03-26 2:14 ` [RFC PATCH v2 3/6] drm/shmem: Implement sparse allocation of pages for shmem objects Adrián Larumbe
@ 2025-03-26 2:14 ` Adrián Larumbe
2025-03-26 2:14 ` [RFC PATCH v2 5/6] drm/shmem: Add a helper to check object's page backing status Adrián Larumbe
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Adrián Larumbe @ 2025-03-26 2:14 UTC (permalink / raw)
To: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, Adrián Larumbe, linux-kernel, dri-devel
Panfrost heap BOs grow on demand when the GPU triggers a page fault
after accessing an address within the BO's virtual range.
We still store the sgts we get back from the shmem sparse allocation
function, since it was decided management of sparse memory SGTs should
be done by client drivers rather than the shmem subsystem.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
drivers/gpu/drm/panfrost/panfrost_gem.c | 12 ++--
drivers/gpu/drm/panfrost/panfrost_gem.h | 2 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 86 +++++--------------------
3 files changed, 26 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 8e0ff3efede7..0cda2c4e524f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -40,10 +40,10 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
int n_sgt = bo->base.base.size / SZ_2M;
for (i = 0; i < n_sgt; i++) {
- if (bo->sgts[i].sgl) {
- dma_unmap_sgtable(pfdev->dev, &bo->sgts[i],
+ if (bo->sgts[i]) {
+ dma_unmap_sgtable(pfdev->dev, bo->sgts[i],
DMA_BIDIRECTIONAL, 0);
- sg_free_table(&bo->sgts[i]);
+ sg_free_table(bo->sgts[i]);
}
}
kvfree(bo->sgts);
@@ -274,7 +274,11 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
if (flags & PANFROST_BO_HEAP)
size = roundup(size, SZ_2M);
- shmem = drm_gem_shmem_create(dev, size);
+ if (flags & PANFROST_BO_HEAP)
+ shmem = drm_gem_shmem_create_sparse(dev, size);
+ else
+ shmem = drm_gem_shmem_create(dev, size);
+
if (IS_ERR(shmem))
return ERR_CAST(shmem);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 7516b7ecf7fe..2a8d0752011e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -11,7 +11,7 @@ struct panfrost_mmu;
struct panfrost_gem_object {
struct drm_gem_shmem_object base;
- struct sg_table *sgts;
+ struct sg_table **sgts;
/*
* Use a list for now. If searching a mapping ever becomes the
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index b91019cd5acb..de343c4e399a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -441,14 +441,11 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
u64 addr)
{
- int ret, i;
struct panfrost_gem_mapping *bomapping;
struct panfrost_gem_object *bo;
- struct address_space *mapping;
- struct drm_gem_object *obj;
pgoff_t page_offset;
struct sg_table *sgt;
- struct page **pages;
+ int ret = 0;
bomapping = addr_to_mapping(pfdev, as, addr);
if (!bomapping)
@@ -459,94 +456,45 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
dev_WARN(pfdev->dev, "matching BO is not heap type (GPU VA = %llx)",
bomapping->mmnode.start << PAGE_SHIFT);
ret = -EINVAL;
- goto err_bo;
+ goto fault_out;
}
WARN_ON(bomapping->mmu->as != as);
/* Assume 2MB alignment and size multiple */
addr &= ~((u64)SZ_2M - 1);
- page_offset = addr >> PAGE_SHIFT;
- page_offset -= bomapping->mmnode.start;
+ page_offset = (addr >> PAGE_SHIFT) - bomapping->mmnode.start;
- obj = &bo->base.base;
-
- dma_resv_lock(obj->resv, NULL);
-
- if (!bo->base.pages) {
+ if (!bo->sgts) {
bo->sgts = kvmalloc_array(bo->base.base.size / SZ_2M,
- sizeof(struct sg_table), GFP_KERNEL | __GFP_ZERO);
+ sizeof(struct sg_table *), GFP_KERNEL | __GFP_ZERO);
if (!bo->sgts) {
ret = -ENOMEM;
- goto err_unlock;
- }
-
- pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
- sizeof(struct page *), GFP_KERNEL | __GFP_ZERO);
- if (!pages) {
- kvfree(bo->sgts);
- bo->sgts = NULL;
- ret = -ENOMEM;
- goto err_unlock;
- }
- bo->base.pages = pages;
- bo->base.pages_use_count = 1;
- } else {
- pages = bo->base.pages;
- if (pages[page_offset]) {
- /* Pages are already mapped, bail out. */
- goto out;
+ goto fault_out;
}
}
- mapping = bo->base.base.filp->f_mapping;
- mapping_set_unevictable(mapping);
+ sgt = drm_gem_shmem_get_sparse_pages_sgt(&bo->base, NUM_FAULT_PAGES,
+ page_offset, GFP_NOWAIT);
+ if (IS_ERR(sgt)) {
+ if (WARN_ON(PTR_ERR(sgt) != -EEXIST))
+ ret = PTR_ERR(sgt);
+ else
+ ret = 0;
- for (i = page_offset; i < page_offset + NUM_FAULT_PAGES; i++) {
- /* Can happen if the last fault only partially filled this
- * section of the pages array before failing. In that case
- * we skip already filled pages.
- */
- if (pages[i])
- continue;
-
- pages[i] = shmem_read_mapping_page(mapping, i);
- if (IS_ERR(pages[i])) {
- ret = PTR_ERR(pages[i]);
- pages[i] = NULL;
- goto err_unlock;
- }
+ goto fault_out;
}
- sgt = &bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)];
- ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
- NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
- if (ret)
- goto err_unlock;
-
- ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0);
- if (ret)
- goto err_map;
-
mmu_map_sg(pfdev, bomapping->mmu, addr,
IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
+ bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)] = sgt;
+
bomapping->active = true;
bo->heap_rss_size += SZ_2M;
dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
-out:
- dma_resv_unlock(obj->resv);
-
- panfrost_gem_mapping_put(bomapping);
-
- return 0;
-
-err_map:
- sg_free_table(sgt);
-err_unlock:
- dma_resv_unlock(obj->resv);
-err_bo:
+fault_out:
panfrost_gem_mapping_put(bomapping);
return ret;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [RFC PATCH v2 5/6] drm/shmem: Add a helper to check object's page backing status
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
` (3 preceding siblings ...)
2025-03-26 2:14 ` [RFC PATCH v2 4/6] drm/panfrost: Use shmem sparse allocation for heap BOs Adrián Larumbe
@ 2025-03-26 2:14 ` Adrián Larumbe
2025-03-31 7:15 ` Thomas Zimmermann
2025-03-26 2:14 ` [RFC PATCH v2 6/6] drm/panfrost/panthor: Take sparse objects into account for fdinfo Adrián Larumbe
2025-03-31 7:13 ` [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Thomas Zimmermann
6 siblings, 1 reply; 16+ messages in thread
From: Adrián Larumbe @ 2025-03-26 2:14 UTC (permalink / raw)
To: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, Adrián Larumbe, linux-kernel, dri-devel
Provide a helper function that lets shmem API users know whether a given
object is backed by physical pages, or else in the case of a sparse
shmem object, at least one of them is populated.
The obvious user is fdinfo, which needs to know an object's resident
status.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 18 ++++++++++++++++++
include/drm/drm_gem_shmem_helper.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 1bf33e5a1c4c..79ac7c7c953f 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -1033,6 +1033,24 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
+/**
+ * drm_gem_shmem_is_populated - Tell whether the shem object is backed by
+ * at least one page of physical memory
+ * @shmem: shmem GEM object
+ *
+ * Returns:
+ * A boolean, where the 'true' value depends on at least one page being preset
+ * in a sparse object's xarray, or all the shmem file pages for PRIME buffers
+ * and regular shmem objects.
+ */
+bool drm_gem_shmem_is_populated(struct drm_gem_shmem_object *shmem)
+{
+ return (shmem->base.import_attach ||
+ (!shmem->sparse && shmem->pages) ||
+ (shmem->sparse && !xa_empty(&shmem->xapages)));
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_is_populated);
+
MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
MODULE_IMPORT_NS("DMA_BUF");
MODULE_LICENSE("GPL v2");
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index cbe4548e3ff6..60d2b8ef230b 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -302,6 +302,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
struct drm_mode_create_dumb *args);
+bool drm_gem_shmem_is_populated(struct drm_gem_shmem_object *shmem);
+
/**
* DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
*
--
2.48.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 5/6] drm/shmem: Add a helper to check object's page backing status
2025-03-26 2:14 ` [RFC PATCH v2 5/6] drm/shmem: Add a helper to check object's page backing status Adrián Larumbe
@ 2025-03-31 7:15 ` Thomas Zimmermann
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Zimmermann @ 2025-03-31 7:15 UTC (permalink / raw)
To: Adrián Larumbe, Andrew Morton, Maarten Lankhorst,
Maxime Ripard, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, linux-kernel, dri-devel
Hi
Am 26.03.25 um 03:14 schrieb Adrián Larumbe:
> Provide a helper function that lets shmem API users know whether a given
> object is backed by physical pages, or else in the case of a sparse
> shmem object, at least one of them is populated.
>
> The obvious user is fdinfo, which needs to know an object's resident
> status.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 18 ++++++++++++++++++
> include/drm/drm_gem_shmem_helper.h | 2 ++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 1bf33e5a1c4c..79ac7c7c953f 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -1033,6 +1033,24 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>
> +/**
> + * drm_gem_shmem_is_populated - Tell whether the shem object is backed by
> + * at least one page of physical memory
> + * @shmem: shmem GEM object
> + *
> + * Returns:
> + * A boolean, where the 'true' value depends on at least one page being preset
> + * in a sparse object's xarray, or all the shmem file pages for PRIME buffers
> + * and regular shmem objects.
> + */
> +bool drm_gem_shmem_is_populated(struct drm_gem_shmem_object *shmem)
> +{
> + return (shmem->base.import_attach ||
The field import_attach should no longer be tested for detecting
imported buffers. Use drm_gem_is_imported() instead.
Best regards
Thomas
> + (!shmem->sparse && shmem->pages) ||
> + (shmem->sparse && !xa_empty(&shmem->xapages)));
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_is_populated);
> +
> MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
> MODULE_IMPORT_NS("DMA_BUF");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index cbe4548e3ff6..60d2b8ef230b 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -302,6 +302,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
> struct drm_mode_create_dumb *args);
>
> +bool drm_gem_shmem_is_populated(struct drm_gem_shmem_object *shmem);
> +
> /**
> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
> *
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v2 6/6] drm/panfrost/panthor: Take sparse objects into account for fdinfo
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
` (4 preceding siblings ...)
2025-03-26 2:14 ` [RFC PATCH v2 5/6] drm/shmem: Add a helper to check object's page backing status Adrián Larumbe
@ 2025-03-26 2:14 ` Adrián Larumbe
2025-04-01 9:39 ` Boris Brezillon
2025-03-31 7:13 ` [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Thomas Zimmermann
6 siblings, 1 reply; 16+ messages in thread
From: Adrián Larumbe @ 2025-03-26 2:14 UTC (permalink / raw)
To: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, Adrián Larumbe, linux-kernel, dri-devel
Make use of the new shmem helper for deciding whether a GEM object has
backing pages.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
drivers/gpu/drm/panthor/panthor_gem.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 0cda2c4e524f..2c6d73a7b5e5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -200,7 +200,7 @@ static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj
struct panfrost_gem_object *bo = to_panfrost_bo(obj);
enum drm_gem_object_status res = 0;
- if (bo->base.base.import_attach || bo->base.pages)
+ if (drm_gem_shmem_is_populated(&bo->base))
res |= DRM_GEM_OBJECT_RESIDENT;
if (bo->base.madv == PANFROST_MADV_DONTNEED)
diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index 8244a4e6c2a2..48930fe7b398 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -155,7 +155,7 @@ static enum drm_gem_object_status panthor_gem_status(struct drm_gem_object *obj)
struct panthor_gem_object *bo = to_panthor_bo(obj);
enum drm_gem_object_status res = 0;
- if (bo->base.base.import_attach || bo->base.pages)
+ if (drm_gem_shmem_is_populated(&bo->base))
res |= DRM_GEM_OBJECT_RESIDENT;
return res;
--
2.48.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 6/6] drm/panfrost/panthor: Take sparse objects into account for fdinfo
2025-03-26 2:14 ` [RFC PATCH v2 6/6] drm/panfrost/panthor: Take sparse objects into account for fdinfo Adrián Larumbe
@ 2025-04-01 9:39 ` Boris Brezillon
0 siblings, 0 replies; 16+ messages in thread
From: Boris Brezillon @ 2025-04-01 9:39 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Andrew Morton, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Steven Price, Liviu Dudau, kernel, linux-kernel, dri-devel
On Wed, 26 Mar 2025 02:14:26 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> Make use of the new shmem helper for deciding whether a GEM object has
> backing pages.
This should be done in patch 4, otherwise the series is not bisectible.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
> drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
> drivers/gpu/drm/panthor/panthor_gem.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 0cda2c4e524f..2c6d73a7b5e5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -200,7 +200,7 @@ static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj
> struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> enum drm_gem_object_status res = 0;
>
> - if (bo->base.base.import_attach || bo->base.pages)
> + if (drm_gem_shmem_is_populated(&bo->base))
> res |= DRM_GEM_OBJECT_RESIDENT;
>
> if (bo->base.madv == PANFROST_MADV_DONTNEED)
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index 8244a4e6c2a2..48930fe7b398 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -155,7 +155,7 @@ static enum drm_gem_object_status panthor_gem_status(struct drm_gem_object *obj)
> struct panthor_gem_object *bo = to_panthor_bo(obj);
> enum drm_gem_object_status res = 0;
>
> - if (bo->base.base.import_attach || bo->base.pages)
> + if (drm_gem_shmem_is_populated(&bo->base))
> res |= DRM_GEM_OBJECT_RESIDENT;
>
> return res;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations
2025-03-26 2:14 [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Adrián Larumbe
` (5 preceding siblings ...)
2025-03-26 2:14 ` [RFC PATCH v2 6/6] drm/panfrost/panthor: Take sparse objects into account for fdinfo Adrián Larumbe
@ 2025-03-31 7:13 ` Thomas Zimmermann
2025-03-31 8:31 ` Boris Brezillon
6 siblings, 1 reply; 16+ messages in thread
From: Thomas Zimmermann @ 2025-03-31 7:13 UTC (permalink / raw)
To: Adrián Larumbe, Andrew Morton, Maarten Lankhorst,
Maxime Ripard, David Airlie, Simona Vetter, Boris Brezillon,
Rob Herring, Steven Price, Liviu Dudau
Cc: kernel, linux-kernel, dri-devel
Hi
Am 26.03.25 um 03:14 schrieb Adrián Larumbe:
> This patch series is a proposal for implementing sparse page allocations
> for shmem objects. It was initially motivated by a kind of BO managed by
> the Panfrost driver, the tiler heap, which grows on demand every time the
> GPU faults on a virtual address within its drm_mm-managed ranged.
I've looked at panfrost_gem.h and found that the driver's gem structure
has grown quite a bit. It seems to have outgrown gem-shmem already. I
think you should consider pulling a copy of gem-shmem into the driver
and building a dedicated memory manager on top.
Best regards
Thomas
>
> Because keeping a struct page pointer array that can describe the entire
> virtual range is wasteful when only a few backing pages have been
> allocated, at Collabora we thought a sparse allocation approach with
> xarrays was a more efficient choice.
>
> Since sparse and 'dense' DRM shmem objects must be managed slightly
> differently, the API is expanded to allow client drivers to create sparse
> objects and also to expand their page backing range, but everything else
> should remain as transparent as possible and be handled from within the DRM
> shmem system itself.
>
> Discussion of previus revision can be found here:
> https://lore.kernel.org/dri-devel/20250218232552.3450939-1-adrian.larumbe@collabora.com/
>
> Changelog:
> v2:
> - Removed patch with helper for non-blocking shmem page allocations.
> - Moved page_array definitions away from scatterlist interface to hide
> them from consumers.
> - Refactored sg_alloc_append_table_from_pages() so that it now calls
> sg_alloc_append_table_from_page_array() to avoid code duplication.
> - Undid extension of __drm_gem_shmem_create() argument list so that a sparse
> shmem object is now fully defined in a parent function.
> - Moved check for absence of backing pages when putting an object into
> drm_gem_shmem_put_pages()
> - Added explanatory comments above DRM WARN'ings across yet unimplemented
> shmem code paths, like kernel vmap's and UM mappings of sparse objects
> - Created drm_gem helper for doing the actual sparse allocation, to keep
> the interface aligned with the existing one with regular shmem objects.
> - Split the body of drm_gem_shmem_get_sparse_pages_locked() into two separate
> functions, one which performs the actual page allocation, and another
> one that retrieves an sgtable.
> - Expanded the argument list of drm_gem_shmem_get_sparse_pages() and its
> children functions so that it takes an gfp mask, in the even that we would
> want to do non-blocking allocations, for instance like when we wish to
> avoid races with the shrinker memory reclaim path.
> - Created shmem helper that returns whether an shmem object has any backing pages.
>
> TODO:
> The following items need to be worked on, and will be the subject of a v3 of this RFC:
>
> - Handle the special case when some of the pages in a sparse allocation range are
> already present, rather than bailing out immediately.
> - Redefining panfrost_gem_object::sgts into an xarray or perhaps a sg_append_table
> to avoid memory waste in allocating more sgtable pointers than we could need.
> - Deciding on the rules for sparse shmem object's kmaps and UM maps.
>
> Adrián Larumbe (6):
> lib/scatterlist.c: Support constructing sgt from page xarray
> drm/shmem: Introduce the notion of sparse objects
> drm/shmem: Implement sparse allocation of pages for shmem objects
> drm/panfrost: Use shmem sparse allocation for heap BOs
> drm/shmem: Add a helper to check object's page backing status
> drm/panfrost/panthor: Take sparse objects into account for fdinfo
>
> drivers/gpu/drm/drm_gem.c | 117 +++++++++++
> drivers/gpu/drm/drm_gem_shmem_helper.c | 264 +++++++++++++++++++++++-
> drivers/gpu/drm/panfrost/panfrost_gem.c | 14 +-
> drivers/gpu/drm/panfrost/panfrost_gem.h | 2 +-
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 86 ++------
> drivers/gpu/drm/panthor/panthor_gem.c | 2 +-
> include/drm/drm_gem.h | 6 +
> include/drm/drm_gem_shmem_helper.h | 29 ++-
> include/linux/scatterlist.h | 17 ++
> lib/scatterlist.c | 175 +++++++++++-----
> 10 files changed, 579 insertions(+), 133 deletions(-)
>
>
> base-commit: 2f9d51740cc30e0d2c8a23a55b1e20cf2513c250
> --
> 2.48.1
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations
2025-03-31 7:13 ` [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations Thomas Zimmermann
@ 2025-03-31 8:31 ` Boris Brezillon
2025-03-31 9:12 ` Thomas Zimmermann
0 siblings, 1 reply; 16+ messages in thread
From: Boris Brezillon @ 2025-03-31 8:31 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Adrián Larumbe, Andrew Morton, Maarten Lankhorst,
Maxime Ripard, David Airlie, Simona Vetter, Rob Herring,
Steven Price, Liviu Dudau, kernel, linux-kernel, dri-devel
Hi Thomas,
On Mon, 31 Mar 2025 09:13:59 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 26.03.25 um 03:14 schrieb Adrián Larumbe:
> > This patch series is a proposal for implementing sparse page allocations
> > for shmem objects. It was initially motivated by a kind of BO managed by
> > the Panfrost driver, the tiler heap, which grows on demand every time the
> > GPU faults on a virtual address within its drm_mm-managed ranged.
>
> I've looked at panfrost_gem.h and found that the driver's gem structure
> has grown quite a bit. It seems to have outgrown gem-shmem already. I
> think you should consider pulling a copy of gem-shmem into the driver
> and building a dedicated memory manager on top.
Actually, it's not just something we need for panfrost. I plan to use
the same non-blocking allocation mechanism for panthor's heap
chunks/buffers, and lima could use it for its heap buffers too. The
non-blocking page allocation is also something i915 has been
open-coding here [1], and I believe that some of this logic could
(and should IMHO) live in common code rather than each driver coming
with its own solution, thus increasing the risk of bugs/inconsistencies.
That's even more important if we provide a common gem-shmem shrinker
like Dmitry's has been proposing.
Best Regards,
Boris
[1]https://elixir.bootlin.com/linux/v6.13.7/source/drivers/gpu/drm/i915/gem/i915_gem_shmem.c#L89
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v2 0/6] Introduce sparse DRM shmem object allocations
2025-03-31 8:31 ` Boris Brezillon
@ 2025-03-31 9:12 ` Thomas Zimmermann
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Zimmermann @ 2025-03-31 9:12 UTC (permalink / raw)
To: Boris Brezillon
Cc: Adrián Larumbe, Andrew Morton, Maarten Lankhorst,
Maxime Ripard, David Airlie, Simona Vetter, Rob Herring,
Steven Price, Liviu Dudau, kernel, linux-kernel, dri-devel
Hi
Am 31.03.25 um 10:31 schrieb Boris Brezillon:
> Hi Thomas,
>
> On Mon, 31 Mar 2025 09:13:59 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> Hi
>>
>> Am 26.03.25 um 03:14 schrieb Adrián Larumbe:
>>> This patch series is a proposal for implementing sparse page allocations
>>> for shmem objects. It was initially motivated by a kind of BO managed by
>>> the Panfrost driver, the tiler heap, which grows on demand every time the
>>> GPU faults on a virtual address within its drm_mm-managed ranged.
>> I've looked at panfrost_gem.h and found that the driver's gem structure
>> has grown quite a bit. It seems to have outgrown gem-shmem already. I
>> think you should consider pulling a copy of gem-shmem into the driver
>> and building a dedicated memory manager on top.
> Actually, it's not just something we need for panfrost. I plan to use
> the same non-blocking allocation mechanism for panthor's heap
> chunks/buffers, and lima could use it for its heap buffers too. The
> non-blocking page allocation is also something i915 has been
> open-coding here [1], and I believe that some of this logic could
> (and should IMHO) live in common code rather than each driver coming
> with its own solution, thus increasing the risk of bugs/inconsistencies.
> That's even more important if we provide a common gem-shmem shrinker
> like Dmitry's has been proposing.
Thanks for the note.
Best regards
Thomas
>
> Best Regards,
>
> Boris
>
> [1]https://elixir.bootlin.com/linux/v6.13.7/source/drivers/gpu/drm/i915/gem/i915_gem_shmem.c#L89
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 16+ messages in thread