* [PATCH] drm: Fix writeback_job leak when state is check only or check failed
@ 2019-02-21 11:14 james qian wang (Arm Technology China)
2019-02-21 13:56 ` Maarten Lankhorst
0 siblings, 1 reply; 4+ messages in thread
From: james qian wang (Arm Technology China) @ 2019-02-21 11:14 UTC (permalink / raw)
To: Liviu Dudau, airlied@linux.ie, Brian Starkey,
maarten.lankhorst@linux.intel.com, maxime.ripard@bootlin.com,
sean@poorly.run
Cc: Jonathan Chai (Arm Technology China),
dri-devel@lists.freedesktop.org,
Julien Yin (Arm Technology China),
james qian wang (Arm Technology China), nd
The writeback job will not be added to writeback queue if the state is
check only or check failed, to avoid leak, need to cleanup writeback job
in connector_destroy_state if the job existed.
Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++---
2 files changed, 22 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..6a8e414233de 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -28,6 +28,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_plane.h>
#include <drm/drm_connector.h>
+#include <drm/drm_writeback.h>
#include <drm/drm_atomic.h>
#include <drm/drm_device.h>
@@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
void
__drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
{
+ if (state->writeback_job)
+ drm_writeback_cleanup_job(state->writeback_job);
+
if (state->crtc)
drm_connector_put(state->connector);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index c20e6fe00cb3..486121150eaa 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
}
EXPORT_SYMBOL(drm_writeback_queue_job);
+/**
+ * drm_writeback_cleanup_job - cleanup a writeback job
+ * @job: The job to cleanup
+ */
+void drm_writeback_cleanup_job(struct drm_writeback_job *job)
+{
+ if (job->fb)
+ drm_framebuffer_put(job->fb);
+
+ if (job->out_fence)
+ dma_fence_put(job->out_fence);
+
+ kfree(job);
+}
+EXPORT_SYMBOL(drm_writeback_cleanup_job);
+
/*
* @cleanup_work: deferred cleanup of a writeback job
*
@@ -280,11 +296,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
* @wb_connector: The writeback connector whose job is complete
@@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
dma_fence_set_error(job->out_fence, status);
dma_fence_signal(job->out_fence);
dma_fence_put(job->out_fence);
+ job->out_fence = NULL;
}
}
spin_unlock_irqrestore(&wb_connector->job_lock, flags);
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Fix writeback_job leak when state is check only or check failed
2019-02-21 11:14 [PATCH] drm: Fix writeback_job leak when state is check only or check failed james qian wang (Arm Technology China)
@ 2019-02-21 13:56 ` Maarten Lankhorst
2019-02-22 7:39 ` james qian wang (Arm Technology China)
0 siblings, 1 reply; 4+ messages in thread
From: Maarten Lankhorst @ 2019-02-21 13:56 UTC (permalink / raw)
To: james qian wang (Arm Technology China), Liviu Dudau,
airlied@linux.ie, Brian Starkey, maxime.ripard@bootlin.com,
sean@poorly.run
Cc: Julien Yin (Arm Technology China),
Jonathan Chai (Arm Technology China), nd,
dri-devel@lists.freedesktop.org
Hey
Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China):
> The writeback job will not be added to writeback queue if the state is
> check only or check failed, to avoid leak, need to cleanup writeback job
> in connector_destroy_state if the job existed.
>
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success.
~Maarten
> drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
> drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++---
> 2 files changed, 22 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..6a8e414233de 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -28,6 +28,7 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_plane.h>
> #include <drm/drm_connector.h>
> +#include <drm/drm_writeback.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_device.h>
>
> @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
> void
> __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> {
> + if (state->writeback_job)
> + drm_writeback_cleanup_job(state->writeback_job);
> +
> if (state->crtc)
> drm_connector_put(state->connector);
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index c20e6fe00cb3..486121150eaa 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> }
> EXPORT_SYMBOL(drm_writeback_queue_job);
>
> +/**
> + * drm_writeback_cleanup_job - cleanup a writeback job
> + * @job: The job to cleanup
> + */
> +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> +{
> + if (job->fb)
> + drm_framebuffer_put(job->fb);
> +
> + if (job->out_fence)
> + dma_fence_put(job->out_fence);
> +
> + kfree(job);
> +}
> +EXPORT_SYMBOL(drm_writeback_cleanup_job);
> +
> /*
> * @cleanup_work: deferred cleanup of a writeback job
> *
> @@ -280,11 +296,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
> * @wb_connector: The writeback connector whose job is complete
> @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
> dma_fence_set_error(job->out_fence, status);
> dma_fence_signal(job->out_fence);
> dma_fence_put(job->out_fence);
> + job->out_fence = NULL;
> }
> }
> spin_unlock_irqrestore(&wb_connector->job_lock, flags);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Fix writeback_job leak when state is check only or check failed
2019-02-21 13:56 ` Maarten Lankhorst
@ 2019-02-22 7:39 ` james qian wang (Arm Technology China)
2019-02-22 16:06 ` Brian Starkey
0 siblings, 1 reply; 4+ messages in thread
From: james qian wang (Arm Technology China) @ 2019-02-22 7:39 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: airlied@linux.ie, Jonathan Chai (Arm Technology China),
Liviu Dudau, dri-devel@lists.freedesktop.org,
Julien Yin (Arm Technology China), maxime.ripard@bootlin.com, nd,
sean@poorly.run
On Thu, Feb 21, 2019 at 02:56:56PM +0100, Maarten Lankhorst wrote:
> Hey
>
> Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China):
> > The writeback job will not be added to writeback queue if the state is
> > check only or check failed, to avoid leak, need to cleanup writeback job
> > in connector_destroy_state if the job existed.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
>
> Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success.
>
> ~Maarten
Yes, the state->writeback_job need to be set to null once the job has been
queued.
Maybe we can refine the queue_job function and add this set NULL into
it.
void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
struct drm_writeback_job **new_job)
{
struct drm_writeback_job *job = *new_job;
unsigned long flags;
*new_job = NULL;
spin_lock_irqsave(&wb_connector->job_lock, flags);
list_add_tail(&job->list_entry, &wb_connector->job_queue);
spin_unlock_irqrestore(&wb_connector->job_lock, flags);
}
Thanks
James
>
> > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
> > drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++---
> > 2 files changed, 22 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..6a8e414233de 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -28,6 +28,7 @@
> > #include <drm/drm_crtc.h>
> > #include <drm/drm_plane.h>
> > #include <drm/drm_connector.h>
> > +#include <drm/drm_writeback.h>
> > #include <drm/drm_atomic.h>
> > #include <drm/drm_device.h>
> >
> > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
> > void
> > __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> > {
> > + if (state->writeback_job)
> > + drm_writeback_cleanup_job(state->writeback_job);
> > +
> > if (state->crtc)
> > drm_connector_put(state->connector);
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index c20e6fe00cb3..486121150eaa 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> > }
> > EXPORT_SYMBOL(drm_writeback_queue_job);
> >
> > +/**
> > + * drm_writeback_cleanup_job - cleanup a writeback job
> > + * @job: The job to cleanup
> > + */
> > +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> > +{
> > + if (job->fb)
> > + drm_framebuffer_put(job->fb);
> > +
> > + if (job->out_fence)
> > + dma_fence_put(job->out_fence);
> > +
> > + kfree(job);
> > +}
> > +EXPORT_SYMBOL(drm_writeback_cleanup_job);
> > +
> > /*
> > * @cleanup_work: deferred cleanup of a writeback job
> > *
> > @@ -280,11 +296,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
> > * @wb_connector: The writeback connector whose job is complete
> > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
> > dma_fence_set_error(job->out_fence, status);
> > dma_fence_signal(job->out_fence);
> > dma_fence_put(job->out_fence);
> > + job->out_fence = NULL;
> > }
> > }
> > spin_unlock_irqrestore(&wb_connector->job_lock, flags);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Fix writeback_job leak when state is check only or check failed
2019-02-22 7:39 ` james qian wang (Arm Technology China)
@ 2019-02-22 16:06 ` Brian Starkey
0 siblings, 0 replies; 4+ messages in thread
From: Brian Starkey @ 2019-02-22 16:06 UTC (permalink / raw)
To: james qian wang (Arm Technology China)
Cc: airlied@linux.ie, Jonathan Chai (Arm Technology China),
Liviu Dudau, dri-devel@lists.freedesktop.org,
Julien Yin (Arm Technology China), maxime.ripard@bootlin.com, nd,
sean@poorly.run
Hi James,
On Fri, Feb 22, 2019 at 07:39:55AM +0000, james qian wang (Arm Technology China) wrote:
> On Thu, Feb 21, 2019 at 02:56:56PM +0100, Maarten Lankhorst wrote:
> > Hey
> >
> > Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China):
> > > The writeback job will not be added to writeback queue if the state is
> > > check only or check failed, to avoid leak, need to cleanup writeback job
> > > in connector_destroy_state if the job existed.
> > >
> > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> >
> > Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success.
> >
> > ~Maarten
>
> Yes, the state->writeback_job need to be set to null once the job has been
> queued.
> Maybe we can refine the queue_job function and add this set NULL into
> it.
>
Laurent has submitted patches for both of these issues, please check
[PATCH v5 12/19] drm: writeback: Cleanup job ownership handling when queuing job
[PATCH v5 13/19] drm: writeback: Fix leak of writeback job
Also note they will impact the komeda implementation
Thanks,
-Brian
> void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> struct drm_writeback_job **new_job)
> {
> struct drm_writeback_job *job = *new_job;
> unsigned long flags;
>
> *new_job = NULL;
>
> spin_lock_irqsave(&wb_connector->job_lock, flags);
> list_add_tail(&job->list_entry, &wb_connector->job_queue);
> spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> }
>
> Thanks
> James
>
> >
> > > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
> > > drivers/gpu/drm/drm_writeback.c | 21 ++++++++++++++++++---
> > > 2 files changed, 22 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..6a8e414233de 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -28,6 +28,7 @@
> > > #include <drm/drm_crtc.h>
> > > #include <drm/drm_plane.h>
> > > #include <drm/drm_connector.h>
> > > +#include <drm/drm_writeback.h>
> > > #include <drm/drm_atomic.h>
> > > #include <drm/drm_device.h>
> > >
> > > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
> > > void
> > > __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> > > {
> > > + if (state->writeback_job)
> > > + drm_writeback_cleanup_job(state->writeback_job);
> > > +
> > > if (state->crtc)
> > > drm_connector_put(state->connector);
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > index c20e6fe00cb3..486121150eaa 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> > > }
> > > EXPORT_SYMBOL(drm_writeback_queue_job);
> > >
> > > +/**
> > > + * drm_writeback_cleanup_job - cleanup a writeback job
> > > + * @job: The job to cleanup
> > > + */
> > > +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> > > +{
> > > + if (job->fb)
> > > + drm_framebuffer_put(job->fb);
> > > +
> > > + if (job->out_fence)
> > > + dma_fence_put(job->out_fence);
> > > +
> > > + kfree(job);
> > > +}
> > > +EXPORT_SYMBOL(drm_writeback_cleanup_job);
> > > +
> > > /*
> > > * @cleanup_work: deferred cleanup of a writeback job
> > > *
> > > @@ -280,11 +296,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
> > > * @wb_connector: The writeback connector whose job is complete
> > > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
> > > dma_fence_set_error(job->out_fence, status);
> > > dma_fence_signal(job->out_fence);
> > > dma_fence_put(job->out_fence);
> > > + job->out_fence = NULL;
> > > }
> > > }
> > > spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-22 16:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-21 11:14 [PATCH] drm: Fix writeback_job leak when state is check only or check failed james qian wang (Arm Technology China)
2019-02-21 13:56 ` Maarten Lankhorst
2019-02-22 7:39 ` james qian wang (Arm Technology China)
2019-02-22 16:06 ` Brian Starkey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox