From: Ludovic Desroches <[email protected]> [ Upstream commit 81af99cbd9e4f238011af811d544fff75641fc25 ]
If there is a plane state to destroy when doing a plane reset, destroy it using the atmel_hlcdc_plane_destroy_state() function. So we call __drm_atomic_helper_plane_destroy_state() and avoid code duplication. Signed-off-by: Ludovic Desroches <[email protected]> Reviewed-by: Manikandan Muralidharan <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Manikandan Muralidharan <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: ## Analysis of the Commit ### Commit Message Analysis The commit message says: "destroy properly the plane state in the reset callback" — the key word is **properly**. This suggests the previous destruction was **improper**, meaning there was a bug in the old code. ### Code Change Analysis Let me examine the old vs new `atmel_hlcdc_plane_reset()` function carefully. **Old code (being removed):** ```c if (p->state) { state = drm_plane_state_to_atmel_hlcdc_plane_state(p->state); if (state->base.fb) drm_framebuffer_put(state->base.fb); kfree(state); p->state = NULL; } ``` **New code (replacement):** ```c if (p->state) { atmel_hlcdc_plane_atomic_destroy_state(p, p->state); p->state = NULL; } ``` **What `atmel_hlcdc_plane_atomic_destroy_state()` does that the old inline code did NOT:** ```c static void atmel_hlcdc_plane_atomic_destroy_state(...) { // 1. Frees DMA descriptors from the DMA pool for (i = 0; i < ARRAY_SIZE(state->dscrs); i++) { dma_pool_free(dc->dscrpool, state->dscrs[i], state->dscrs[i]->self); } // 2. Puts the framebuffer reference if (s->fb) drm_framebuffer_put(s->fb); // 3. Frees the state kfree(state); } ``` ### Bug Identified: DMA Descriptor Resource Leak The old reset code was **missing the DMA pool free** for the descriptors (`state->dscrs[i]`). Every time `atmel_hlcdc_plane_reset()` was called with an existing plane state, the DMA descriptors allocated from `dc->dscrpool` were **leaked** — they were never returned to the DMA pool. This is a **real resource leak**. DMA pool memory is coherent DMA memory, a finite and precious resource, especially on embedded ARM systems where this Atmel HLCDC display controller runs. DMA pool exhaustion can lead to allocation failures and display malfunction. Additionally, the old code was also **missing the `__drm_atomic_helper_plane_destroy_state()` call** that the proper destroy function would provide (though in this driver the destroy function doesn't call it either, it does its own fb put + kfree). However, the DMA descriptor leak is the primary bug. ### Additional Change: CSC Initialization The new reset function also adds: ```c if (plane->layer.desc->layout.csc) dc->desc->ops->lcdc_csc_init(plane, plane->layer.desc); ``` This is a **new addition** — CSC (Color Space Conversion) initialization during plane reset. This is arguably a feature addition or a separate fix for missing initialization. This adds a bit of risk since it's not purely fixing the leak but also changing initialization behavior. ### Scope and Risk Assessment - **Files changed:** 1 file - **Net change:** The function is reorganized, and the core fix (using proper destroy) is straightforward - **Risk:** LOW for the resource leak fix. MEDIUM for the CSC init addition — but CSC init during reset is standard practice and the code only runs on Atmel HLCDC hardware - **Subsystem:** DRM driver for a specific ARM SoC (Atmel/Microchip SAM9/SAMA5) — limited blast radius ### Stable Criteria Assessment 1. **Obviously correct:** Yes — the DMA descriptors were being leaked; they must be freed via the proper destroy path 2. **Fixes a real bug:** Yes — DMA pool memory leak on plane reset 3. **Important issue:** Moderate — resource leak in a display driver; on embedded systems with limited DMA pool memory, this could eventually cause display failures 4. **Small and contained:** Yes — one file, one function reorganized 5. **No new features:** The CSC init addition is borderline — it's initialization that was arguably missing, but it does add new behavior 6. **Applies cleanly:** Would need to verify, but it's self-contained ### Concerns 1. The CSC initialization addition goes beyond the pure leak fix and introduces new behavior. This makes it slightly riskier than a pure fix. 2. The commit message doesn't emphasize the DMA descriptor leak — it's described as avoiding "code duplication," which undersells the actual bug being fixed. This is a classic case of a bug fix disguised as cleanup. 3. No `Fixes:` tag or syzbot report, but the bug is clearly visible in the code. ### Verdict The core of this commit fixes a **real DMA pool memory leak** — DMA descriptors were not being freed when the plane state was destroyed during reset. This is a legitimate resource leak bug fix. The change is small, contained to a single driver file, and the fix approach (reusing the existing proper destroy function) is obviously correct. The CSC initialization addition adds some risk but is limited to specific hardware and is part of proper plane reset behavior. On balance, the resource leak fix makes this worth backporting despite the small additional CSC change. **YES** .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index 92132be9823f1..0ffec44c6d317 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -1155,32 +1155,6 @@ static int atmel_hlcdc_plane_alloc_dscrs(struct drm_plane *p, return -ENOMEM; } -static void atmel_hlcdc_plane_reset(struct drm_plane *p) -{ - struct atmel_hlcdc_plane_state *state; - - if (p->state) { - state = drm_plane_state_to_atmel_hlcdc_plane_state(p->state); - - if (state->base.fb) - drm_framebuffer_put(state->base.fb); - - kfree(state); - p->state = NULL; - } - - state = kzalloc(sizeof(*state), GFP_KERNEL); - if (state) { - if (atmel_hlcdc_plane_alloc_dscrs(p, state)) { - kfree(state); - drm_err(p->dev, - "Failed to allocate initial plane state\n"); - return; - } - __drm_atomic_helper_plane_reset(p, &state->base); - } -} - static struct drm_plane_state * atmel_hlcdc_plane_atomic_duplicate_state(struct drm_plane *p) { @@ -1222,6 +1196,32 @@ static void atmel_hlcdc_plane_atomic_destroy_state(struct drm_plane *p, kfree(state); } +static void atmel_hlcdc_plane_reset(struct drm_plane *p) +{ + struct atmel_hlcdc_plane_state *state; + struct atmel_hlcdc_dc *dc = p->dev->dev_private; + struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p); + + if (p->state) { + atmel_hlcdc_plane_atomic_destroy_state(p, p->state); + p->state = NULL; + } + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (state) { + if (atmel_hlcdc_plane_alloc_dscrs(p, state)) { + kfree(state); + drm_err(p->dev, + "Failed to allocate initial plane state\n"); + return; + } + __drm_atomic_helper_plane_reset(p, &state->base); + } + + if (plane->layer.desc->layout.csc) + dc->desc->ops->lcdc_csc_init(plane, plane->layer.desc); +} + static const struct drm_plane_funcs layer_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, -- 2.51.0
