Re: [PATCH v5 13/19] drm: writeback: Fix leak of writeback job
On Thu, Feb 21, 2019 at 12:32:06PM +0200, Laurent Pinchart wrote: > Writeback jobs are allocated when the WRITEBACK_FB_ID is set, and > deleted when the jobs complete. This results in both a memory leak of > the job and a leak of the framebuffer if the atomic commit returns > before the job is queued for processing, for instance if the atomic > check fails or if the commit runs in test-only mode. > > Fix this by implementing the drm_writeback_cleanup_job() function and > calling it from __drm_atomic_helper_connector_destroy_state(). As > writeback jobs are removed from the state when they're queued for > processing, any job left in the state when the state gets destroyed > needs to be cleaned up. > > The existing declaration of the drm_writeback_cleanup_job() function > without an implementation hints that this problem was considered, but > never addressed. > > Signed-off-by: Laurent Pinchart Acked-by: Liviu Dudau Best regards, Liviu > --- > drivers/gpu/drm/drm_atomic_state_helper.c | 4 > drivers/gpu/drm/drm_writeback.c | 13 ++--- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > b/drivers/gpu/drm/drm_atomic_state_helper.c > index 4985384e51f6..59ffb6b9c745 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -412,6 +413,9 @@ __drm_atomic_helper_connector_destroy_state(struct > drm_connector_state *state) > > if (state->commit) > drm_crtc_commit_put(state->commit); > + > + if (state->writeback_job) > + drm_writeback_cleanup_job(state->writeback_job); > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index 338b993d7c9f..afb1ae6e0ecb 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -273,6 +273,14 @@ void drm_writeback_queue_job(struct > drm_writeback_connector *wb_connector, > } > EXPORT_SYMBOL(drm_writeback_queue_job); > > +void drm_writeback_cleanup_job(struct drm_writeback_job *job) > +{ > + if (job->fb) > + drm_framebuffer_put(job->fb); > + > + kfree(job); > +} > + > /* > * @cleanup_work: deferred cleanup of a writeback job > * > @@ -285,10 +293,9 @@ static void cleanup_work(struct work_struct *work) > struct drm_writeback_job *job = container_of(work, >struct drm_writeback_job, >cleanup_work); > - drm_framebuffer_put(job->fb); > - kfree(job); > -} > > + drm_writeback_cleanup_job(job); > +} > > /** > * drm_writeback_signal_completion - Signal the completion of a writeback job > -- > Regards, > > Laurent Pinchart > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 13/19] drm: writeback: Fix leak of writeback job
Hi, On Thu, Feb 21, 2019 at 12:32:06PM +0200, Laurent Pinchart wrote: > Writeback jobs are allocated when the WRITEBACK_FB_ID is set, and > deleted when the jobs complete. This results in both a memory leak of > the job and a leak of the framebuffer if the atomic commit returns > before the job is queued for processing, for instance if the atomic > check fails or if the commit runs in test-only mode. > > Fix this by implementing the drm_writeback_cleanup_job() function and > calling it from __drm_atomic_helper_connector_destroy_state(). As > writeback jobs are removed from the state when they're queued for > processing, any job left in the state when the state gets destroyed > needs to be cleaned up. > > The existing declaration of the drm_writeback_cleanup_job() function > without an implementation hints that this problem was considered, but > never addressed. > > Signed-off-by: Laurent Pinchart Thanks for fixing this, it looks like it got dropped in one of the rework/rebases. Reviewed-by: Brian Starkey > --- > drivers/gpu/drm/drm_atomic_state_helper.c | 4 > drivers/gpu/drm/drm_writeback.c | 13 ++--- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > b/drivers/gpu/drm/drm_atomic_state_helper.c > index 4985384e51f6..59ffb6b9c745 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -412,6 +413,9 @@ __drm_atomic_helper_connector_destroy_state(struct > drm_connector_state *state) > > if (state->commit) > drm_crtc_commit_put(state->commit); > + > + if (state->writeback_job) > + drm_writeback_cleanup_job(state->writeback_job); > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index 338b993d7c9f..afb1ae6e0ecb 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -273,6 +273,14 @@ void drm_writeback_queue_job(struct > drm_writeback_connector *wb_connector, > } > EXPORT_SYMBOL(drm_writeback_queue_job); > > +void drm_writeback_cleanup_job(struct drm_writeback_job *job) > +{ > + if (job->fb) > + drm_framebuffer_put(job->fb); > + > + kfree(job); > +} > + > /* > * @cleanup_work: deferred cleanup of a writeback job > * > @@ -285,10 +293,9 @@ static void cleanup_work(struct work_struct *work) > struct drm_writeback_job *job = container_of(work, >struct drm_writeback_job, >cleanup_work); > - drm_framebuffer_put(job->fb); > - kfree(job); > -} > > + drm_writeback_cleanup_job(job); > +} > > /** > * drm_writeback_signal_completion - Signal the completion of a writeback job > -- > Regards, > > Laurent Pinchart > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel