Re: [PATCH RFC v2 13/14] drm/msm: gate lpac behind modparam

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: broken enable_lpac default and gating logic in msm_gpu_init()
- [High] drm/msm: unfreed queue in msm_submitqueue_create() when lpac_rb is NULL
- [Low] drm/msm: missing kernel-parameters.txt documentation for enable_lpac
--

commit 1335ae6013ec105c8c18047306da8e49457285e5
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

drm/msm: gate lpac behind modparam

This commit adds the enable_lpac module parameter in adreno_device.c and
checks it in msm_gpu_init() before creating the LPAC ringbuffer.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 6e2333466d7f1..51781a2e8a00e 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -30,6 +30,10 @@ static bool skip_gpu;
>  MODULE_PARM_DESC(no_gpu, "Disable GPU driver register (0=enable GPU driver register (default), 1=skip GPU driver register");
>  module_param(skip_gpu, bool, 0400);
>  
> +int enable_lpac = 0;
> +MODULE_PARM_DESC(enable_lpac, "Enable LPAC (A7xx+ only) (1=on , 0=disable, -1=auto (default))");
> +module_param(enable_lpac, int, 0600);

[Severity: Low]
Should this new module parameter also be documented in
Documentation/admin-guide/kernel-parameters.txt?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 4244d9324cda7..f6927c797fa5c 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c

[ ... ]

> @@ -1012,7 +1013,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		memptrs_iova += sizeof(struct msm_rbmemptrs);
>  	}
>  
> -	if (config->has_lpac) {
> +	if (config->has_lpac && enable_lpac != 0) {

[Severity: High]
Does the default value and tri-state logic for enable_lpac match the
documented behavior?

In drivers/gpu/drm/msm/adreno/adreno_device.c, enable_lpac is initialized
to 0, whereas MODULE_PARM_DESC() documents -1 (auto) as the default:

    int enable_lpac = 0;
    MODULE_PARM_DESC(enable_lpac, "Enable LPAC (A7xx+ only) (1=on , 0=disable, -1=auto (default))");

Because enable_lpac defaults to 0 instead of -1, LPAC remains disabled by
default even on targets that set ADRENO_QUIRK_LPAC (such as gen70900 in
the subsequent patch "drm/msm: enable LPAC on gen70900").

In addition, adreno_gpu_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c
computes has_lpac solely from ADRENO_QUIRK_LPAC:

    adreno_gpu_config.has_lpac = !!(config->info->quirks & ADRENO_QUIRK_LPAC);

and msm_gpu_init() requires both config->has_lpac and enable_lpac != 0.
Unlike enable_preemption in a6xx_gpu_init(), this does not distinguish
between 1 (force on) and -1 (auto based on ADRENO_QUIRK_LPAC), so passing
msm.enable_lpac=1 cannot enable LPAC on an A7xx+ GPU unless
ADRENO_QUIRK_LPAC is already set in the catalog.

Furthermore, because enable_lpac is only checked when creating
gpu->lpac_rb in msm_gpu_init(), other LPAC paths remain out of sync with
the parameter:

1. msm_gpu_init() still allocates memptrs using config->has_lpac:

    memptrs = msm_gem_kernel_new(drm,
        sizeof(struct msm_rbmemptrs) * (nr_rings + config->has_lpac),

2. adreno_hw_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c still
configures the LPAC SMMU aperture based on ADRENO_QUIRK_LPAC rather than
whether LPAC is enabled:

    if (info->quirks & ADRENO_QUIRK_LPAC) {
        ret = qcom_scm_set_gpu_smmu_lpac_aperture(1);
        if (ret)
            DRM_DEV_ERROR(gpu->dev->dev, "unable to set lpac SMMU aperture: %d\n", ret);
    }

Should enable_lpac be initialized to -1 and evaluated when setting
config->has_lpac (and checked consistently in adreno_hw_init())?

>  		gpu->lpac_rb = msm_ringbuffer_new(gpu, nr_rings, memptrs, memptrs_iova);

[Severity: High]
When enable_lpac is 0, msm_gpu_init() leaves gpu->lpac_rb as NULL. Can
this cause msm_submitqueue_create() to leak the queue allocation when
userspace requests an LPAC submitqueue?

In drivers/gpu/drm/msm/msm_submitqueue.c, msm_ioctl_submitqueue_new() ->
msm_submitqueue_create() allocates queue before checking whether ring
(priv->gpu->lpac_rb) is NULL:

    queue = kzalloc_obj(*queue);

    if (flags & MSM_SUBMITQUEUE_LPAC) {
        ring_nr = priv->gpu->nr_rings;
        ring = priv->gpu->lpac_rb;
    } else {
        ring = priv->gpu->rb[ring_nr];
    }

    if (!ring)
        return -EINVAL;

When priv->gpu->lpac_rb is NULL and userspace passes MSM_SUBMITQUEUE_LPAC
to DRM_IOCTL_MSM_SUBMITQUEUE_NEW, msm_submitqueue_create() returns -EINVAL
without freeing queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850961@gmail.com?part=13