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 <[email protected]> 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-a21a5c850...@gmail.com?part=13
