[PATCH 1/2] drm/msm: Enable THP for GEM buffers

[PATCH 1/2] drm/msm: Enable THP for GEM buffers

[Rob Clark]

More than 2x speedup on darktable benchmark which was bottlenecked on
page allocation/pinning.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 73d99bde26f1..8129e7d229d9 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -58,9 +58,29 @@ static bool separate_gpu_kms;
 MODULE_PARM_DESC(separate_gpu_drm, "Use separate DRM device for the GPU (0=single DRM device for both GPU and display (default), 1=two DRM devices)");
 module_param(separate_gpu_kms, bool, 0400);
 
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+static bool transparent_hugepage = true;
+module_param(transparent_hugepage, bool, 0400);
+MODULE_PARM_DESC(transparent_hugepage, "Use a dedicated tmpfs mount point with Transparent Hugepage enabled (true = default)");
+#endif
+
 DECLARE_FAULT_ATTR(fail_gem_alloc);
 DECLARE_FAULT_ATTR(fail_gem_iova);
 
+static void msm_gem_thp_init(struct drm_device *dev)
+{
+	int err;
+
+	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !transparent_hugepage)
+		return;
+
+	err = drm_gem_huge_mnt_create(dev, "within_size");
+	if (drm_gem_get_huge_mnt(dev))
+		drm_info(dev, "Using Transparent Hugepage\n");
+	else if (err)
+		drm_warn(dev, "Can't use Transparent Hugepage (%d)\n", err);
+}
+
 bool msm_gpu_no_components(void)
 {
 	return separate_gpu_kms;
@@ -178,6 +198,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv,
 	if (priv->kms_init)
 		msm_drm_kms_post_init(dev);
 
+	msm_gem_thp_init(ddev);
+
 	return 0;
 
 err_msm_uninit:
-- 
2.55.0

[PATCH 2/2] drm/msm/gem: Add modparam to disable shrinker blocking

[Rob Clark]

Normally if we are under enough memory pressure, the shrinker will
eventually start waiting for BOs to become idle.  In some very latency
sensitive use-cases this is undesirable.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_shrinker.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index 3514d5c84989..83ee032cb21d 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -19,6 +19,10 @@ static bool enable_eviction = true;
 MODULE_PARM_DESC(enable_eviction, "Enable swappable GEM buffers");
 module_param(enable_eviction, bool, 0600);
 
+static bool eviction_can_block = true;
+MODULE_PARM_DESC(eviction_can_block, "Enable blocking for GEM buffer to become idle for eviction");
+module_param(eviction_can_block, bool, 0600);
+
 static bool can_swap(void)
 {
 	return enable_eviction && get_nr_swap_pages() > 0;
@@ -26,6 +30,8 @@ static bool can_swap(void)
 
 static bool can_block(struct shrink_control *sc)
 {
+	if (!eviction_can_block)
+		return false;
 	return (sc->gfp_mask & __GFP_DIRECT_RECLAIM) ||
 	       (current_is_kswapd() && (sc->gfp_mask & __GFP_KSWAPD_RECLAIM));
 }
-- 
2.55.0

Re: [PATCH 2/2] drm/msm/gem: Add modparam to disable shrinker blocking

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The shrinker scan function `msm_gem_shrinker_scan` loses count of freed objects and aborts prematurely by returning `SHRINK_STOP` when all scanned objects are successfully freed.
--

commit 1da508dba68cbcbae647ca4f3177e66e5a19e756
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Add modparam to disable shrinker blocking

This commit adds a module parameter, eviction_can_block, to disable
blocking for GEM buffers to become idle during shrinker eviction. This
helps prevent the shrinker from waiting on idle BOs during high memory
pressure, which is undesirable in latency-sensitive use-cases.

> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index 9d2788f79acee..db57609046d1b 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -19,6 +19,10 @@ static bool enable_eviction = true;
>  MODULE_PARM_DESC(enable_eviction, "Enable swappable GEM buffers");
>  module_param(enable_eviction, bool, 0600);
>  
> +static bool eviction_can_block = true;
> +MODULE_PARM_DESC(eviction_can_block, "Enable blocking for GEM buffer to become idle for eviction");
> +module_param(eviction_can_block, bool, 0600);
> +
>  static bool can_swap(void)
>  {
>  	return enable_eviction && get_nr_swap_pages() > 0;
> @@ -26,6 +30,8 @@ static bool can_swap(void)
>  
>  static bool can_block(struct shrink_control *sc)
>  {
> +	if (!eviction_can_block)
> +		return false;

[Severity: High]
This is a pre-existing issue, but I noticed the shrinker scan function can
lose count of freed objects and abort prematurely.

When the VM calls msm_gem_shrinker_scan(), it loops over LRU stages calling
drm_gem_lru_scan():

drivers/gpu/drm/msm/msm_gem_shrinker.c:msm_gem_shrinker_scan() {
    ...
    for (unsigned i = 0; (nr > 0) && (i < ARRAY_SIZE(stages)); i++) {
        ...
        stages[i].freed =
            drm_gem_lru_scan(priv->dev, stages[i].lru, nr,
                     &stages[i].remaining,
                     stages[i].shrink,
                     NULL);
        nr -= stages[i].freed;
        freed += stages[i].freed;
        remaining += stages[i].remaining;
    }
    ...
    return (freed > 0 && remaining > 0) ? freed : SHRINK_STOP;
}

In drm_gem_lru_scan(), the remaining parameter is only populated with objects
that failed to be shrunk:

drivers/gpu/drm/drm_gem.c:drm_gem_lru_scan() {
    ...
        if (!ww_mutex_trylock(&obj->resv->lock, ticket)) {
            *remaining += obj->size >> PAGE_SHIFT;
            goto tail;
        }
    ...
}

If all scanned objects in a batch are successfully freed without encountering
a locked object, remaining will be 0. The return expression
(freed > 0 && remaining > 0) ? freed : SHRINK_STOP; then evaluates to
SHRINK_STOP instead of the number of freed items.

Does this prematurely abort the core memory management shrinker
(do_shrink_slab) and under-report the amount of reclaimed memory,
potentially degrading memory reclaim efficiency and triggering unnecessary
OOM kills?

>  	return (sc->gfp_mask & __GFP_DIRECT_RECLAIM) ||
>  	       (current_is_kswapd() && (sc->gfp_mask & __GFP_KSWAPD_RECLAIM));
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903183725.15835-1-robin.clark@oss.qualcomm.com?part=2

Re: [PATCH 1/2] drm/msm: Enable THP for GEM buffers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] THP initialization (`msm_gem_thp_init`) occurs after the DRM device is exposed to userspace via `drm_dev_register`, creating a data race and allowing premature access.
- [Low] The module parameter `transparent_hugepage` is defined inside an `#ifdef CONFIG_TRANSPARENT_HUGEPAGE` block but accessed unconditionally in `msm_gem_thp_init()`, causing a compilation error when the config is disabled.
--

commit 46b2b6fe3a31a0213c502585828f1edd5794c935
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Enable THP for GEM buffers

This commit introduces a dedicated tmpfs mount point for Transparent
Hugepage support in MSM GEM buffers to improve page allocation performance.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index db1b655dd055b..33b9f495af020 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
[ ... ]
> @@ -58,9 +58,29 @@ static bool separate_gpu_kms;
>  MODULE_PARM_DESC(separate_gpu_drm, "Use separate DRM device for the GPU (0=single DRM device for both GPU and display (default), 1=two DRM devices)");
>  module_param(separate_gpu_kms, bool, 0400);
>  
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static bool transparent_hugepage = true;
> +module_param(transparent_hugepage, bool, 0400);
> +MODULE_PARM_DESC(transparent_hugepage, "Use a dedicated tmpfs mount point with Transparent Hugepage enabled (true = default)");
> +#endif
> +
>  DECLARE_FAULT_ATTR(fail_gem_alloc);
>  DECLARE_FAULT_ATTR(fail_gem_iova);
>  
> +static void msm_gem_thp_init(struct drm_device *dev)
> +{
> +	int err;
> +
> +	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !transparent_hugepage)

[Severity: Low]
Will this cause a build failure when CONFIG_TRANSPARENT_HUGEPAGE is disabled?
The transparent_hugepage module parameter is only defined inside the ifdef block
just above, but the C compiler will still evaluate it unconditionally here in
msm_gem_thp_init(), resulting in an undeclared identifier error.

> +		return;
> +
> +	err = drm_gem_huge_mnt_create(dev, "within_size");
> +	if (drm_gem_get_huge_mnt(dev))
> +		drm_info(dev, "Using Transparent Hugepage\n");
> +	else if (err)
> +		drm_warn(dev, "Can't use Transparent Hugepage (%d)\n", err);
> +}
[ ... ]
> @@ -178,6 +198,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv,
>  	if (priv->kms_init)
>  		msm_drm_kms_post_init(dev);
>  
> +	msm_gem_thp_init(ddev);

[Severity: High]
Is it safe to initialize the THP mount point here?

Looking earlier in msm_drm_init(), drm_dev_register(ddev, 0) is called before
this point. Because the device node is already exposed to userspace, a process
could open it immediately after registration. 

If userspace starts allocating GEM objects before msm_gem_thp_init() completes,
could this race condition cause the allocations to silently fall back to non-THP
shmem, or potentially lead to a crash if a partially written pointer is read?

> +
>  	return 0;
>  
>  err_msm_uninit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903183725.15835-1-robin.clark@oss.qualcomm.com?part=1