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.w...@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
