[PATCH 1/2] drm/msm: dpu: Fix memory leak caused by dropped reference

[PATCH 1/2] drm/msm: dpu: Fix memory leak caused by dropped reference

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

We are currently leaking a drm_crtc_commit struct for every atomic
commit containing plane state. The dpu plane destroy function cleans up
the fb reference manually, but fails to release the commit ref. As a
result, we just keep allocating drm_crtc_commits without ever freeing
them. Fortunately there's a helper function which will clean up all of
our mess at once, so use that.

Thanks to Doug Anderson for reporting the memory leak (and leaving
breadcrumbs from kmemleak!).

Reported-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index fc59a69aa832..f549daf30fe6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1203,9 +1203,7 @@ static void dpu_plane_destroy_state(struct drm_plane *plane,
 
 	pstate = to_dpu_plane_state(state);
 
-	/* remove ref count for frame buffers */
-	if (state->fb)
-		drm_framebuffer_put(state->fb);
+	__drm_atomic_helper_plane_destroy_state(state);
 
 	kfree(pstate);
 }
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 2/2] drm/msm: dpu: Remove checks from dpu_plane_destroy_state()

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

They're not needed.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index f549daf30fe6..4213e7a8e525 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1193,19 +1193,8 @@ static void dpu_plane_destroy(struct drm_plane *plane)
 static void dpu_plane_destroy_state(struct drm_plane *plane,
 		struct drm_plane_state *state)
 {
-	struct dpu_plane_state *pstate;
-
-	if (!plane || !state) {
-		DPU_ERROR("invalid arg(s), plane %d state %d\n",
-				plane != 0, state != 0);
-		return;
-	}
-
-	pstate = to_dpu_plane_state(state);
-
 	__drm_atomic_helper_plane_destroy_state(state);
-
-	kfree(pstate);
+	kfree(to_dpu_plane_state(state));
 }
 
 static struct drm_plane_state *
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/2] drm/msm: dpu: Fix memory leak caused by dropped reference

[Jeykumar Sankaran]

On 2018-10-04 11:09, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> We are currently leaking a drm_crtc_commit struct for every atomic
> commit containing plane state. The dpu plane destroy function cleans up
dpu plane destroy -> dpu_plane_destroy_state
> the fb reference manually, but fails to release the commit ref. As a
> result, we just keep allocating drm_crtc_commits without ever freeing
> them. Fortunately there's a helper function which will clean up all of
> our mess at once, so use that.
> 
> Thanks to Doug Anderson for reporting the memory leak (and leaving
> breadcrumbs from kmemleak!).
> 
> Reported-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
With the nit addressed:

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index fc59a69aa832..f549daf30fe6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1203,9 +1203,7 @@ static void dpu_plane_destroy_state(struct 
> drm_plane
> *plane,
> 
>  	pstate = to_dpu_plane_state(state);
> 
> -	/* remove ref count for frame buffers */
> -	if (state->fb)
> -		drm_framebuffer_put(state->fb);
> +	__drm_atomic_helper_plane_destroy_state(state);
> 
>  	kfree(pstate);
>  }

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 2/2] drm/msm: dpu: Remove checks from dpu_plane_destroy_state()

[Jeykumar Sankaran]

On 2018-10-04 11:09, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> They're not needed.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index f549daf30fe6..4213e7a8e525 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1193,19 +1193,8 @@ static void dpu_plane_destroy(struct drm_plane
> *plane)
>  static void dpu_plane_destroy_state(struct drm_plane *plane,
>  		struct drm_plane_state *state)
>  {
> -	struct dpu_plane_state *pstate;
> -
> -	if (!plane || !state) {
> -		DPU_ERROR("invalid arg(s), plane %d state %d\n",
> -				plane != 0, state != 0);
> -		return;
> -	}
> -
> -	pstate = to_dpu_plane_state(state);
> -
>  	__drm_atomic_helper_plane_destroy_state(state);
> -
> -	kfree(pstate);
> +	kfree(to_dpu_plane_state(state));
>  }
> 
>  static struct drm_plane_state *

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/2] drm/msm: dpu: Fix memory leak caused by dropped reference

[Sean Paul]

On Tue, Oct 09, 2018 at 11:43:25AM -0700, Jeykumar Sankaran wrote:
> On 2018-10-04 11:09, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > We are currently leaking a drm_crtc_commit struct for every atomic
> > commit containing plane state. The dpu plane destroy function cleans up
> dpu plane destroy -> dpu_plane_destroy_state
> > the fb reference manually, but fails to release the commit ref. As a
> > result, we just keep allocating drm_crtc_commits without ever freeing
> > them. Fortunately there's a helper function which will clean up all of
> > our mess at once, so use that.
> > 
> > Thanks to Doug Anderson for reporting the memory leak (and leaving
> > breadcrumbs from kmemleak!).
> > 
> > Reported-by: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> With the nit addressed:

FYI, this already picked up in msm-next last week.

> 
> Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> 
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index fc59a69aa832..f549daf30fe6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -1203,9 +1203,7 @@ static void dpu_plane_destroy_state(struct
> > drm_plane
> > *plane,
> > 
> >  	pstate = to_dpu_plane_state(state);
> > 
> > -	/* remove ref count for frame buffers */
> > -	if (state->fb)
> > -		drm_framebuffer_put(state->fb);
> > +	__drm_atomic_helper_plane_destroy_state(state);
> > 
> >  	kfree(pstate);
> >  }
> 
> -- 
> Jeykumar S

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno