diff mbox series

[v2,2/4] drm/msm: make msm_disp_state transient data struct

Message ID 20210427001828.2375555-3-dmitry.baryshkov@linaro.org
State New
Headers show
Series drm/msm: improve register snapshotting | expand

Commit Message

Dmitry Baryshkov April 27, 2021, 12:18 a.m. UTC
Instead of allocating snapshotting structure at the driver probe time
and later handling concurrent access, actual state, etc, make
msm_disp_state transient struct. Allocate one when snapshotting happens
and free it after coredump data is read by userspace.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
 drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
 4 files changed, 37 insertions(+), 77 deletions(-)

-- 
2.30.2

Comments

Abhinav Kumar April 27, 2021, 7:19 p.m. UTC | #1
Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Instead of allocating snapshotting structure at the driver probe time

> and later handling concurrent access, actual state, etc, make

> msm_disp_state transient struct. Allocate one when snapshotting happens

> and free it after coredump data is read by userspace.

> 

Can you please check my previous comment on coredump_pending?

https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa3509f@codeaurora.org/T/#u

That helps to serialize read/write of coredump.

Rest of the changes on this one look fine to me.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---

>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------

>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--

>  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-

>  drivers/gpu/drm/msm/msm_kms.h                 |  6 +-

>  4 files changed, 37 insertions(+), 77 deletions(-)

> 

> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c

> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c

> index 70fd5a1fe13e..a4a7cb06bc87 100644

> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c

> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c

> @@ -7,8 +7,7 @@

> 

>  #include "msm_disp_snapshot.h"

> 

> -#ifdef CONFIG_DEV_COREDUMP

> -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,

> +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,

> loff_t offset,

>  		size_t count, void *data, size_t datalen)

>  {

>  	struct drm_print_iterator iter;

> @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,

> loff_t offset,

>  	return count - iter.remain;

>  }

> 

> -static void disp_devcoredump_free(void *data)

> +static void _msm_disp_snapshot_work(struct kthread_work *work)

>  {

> +	struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);

> +	struct drm_device *drm_dev = kms->dev;

>  	struct msm_disp_state *disp_state;

> +	struct drm_printer p;

> 

> -	disp_state = data;

> -

> -	msm_disp_state_free(disp_state);

> +	disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);

> +	if (!disp_state)

> +		return;

> 

> -	disp_state->coredump_pending = false;

> -}

> -#endif /* CONFIG_DEV_COREDUMP */

> +	disp_state->dev = drm_dev->dev;

> +	disp_state->drm_dev = drm_dev;

> 

> -static void _msm_disp_snapshot_work(struct kthread_work *work)

> -{

> -	struct msm_disp_state *disp_state = container_of(work, struct

> msm_disp_state, dump_work);

> -	struct drm_printer p;

> +	INIT_LIST_HEAD(&disp_state->blocks);

> 

> -	mutex_lock(&disp_state->mutex);

> +	/* Serialize dumping here */

> +	mutex_lock(&kms->dump_mutex);

> 

>  	msm_disp_snapshot_capture_state(disp_state);

> 

> +	mutex_unlock(&kms->dump_mutex);

> +

>  	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {

>  		p = drm_info_printer(disp_state->drm_dev->dev);

>  		msm_disp_state_print(disp_state, &p);

>  	}

> 

>  	/*

> -	 * if devcoredump is not defined free the state immediately

> -	 * otherwise it will be freed in the free handler.

> +	 * If COREDUMP is disabled, the stub will call the free function.

> +	 * If there is a codedump pending for the device, the dev_coredumpm()

> +	 * will also free new coredump state.

>  	 */

> -#ifdef CONFIG_DEV_COREDUMP

>  	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, 

> GFP_KERNEL,

> -			disp_devcoredump_read, disp_devcoredump_free);

> -	disp_state->coredump_pending = true;

> -#else

> -	msm_disp_state_free(disp_state);

> -#endif

> -

> -	mutex_unlock(&disp_state->mutex);

> +			disp_devcoredump_read, msm_disp_state_free);

>  }

> 

>  void msm_disp_snapshot_state(struct drm_device *drm_dev)

>  {

>  	struct msm_drm_private *priv;

>  	struct msm_kms *kms;

> -	struct msm_disp_state *disp_state;

> 

>  	if (!drm_dev) {

>  		DRM_ERROR("invalid params\n");

> @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device 

> *drm_dev)

> 

>  	priv = drm_dev->dev_private;

>  	kms = priv->kms;

> -	disp_state = kms->disp_state;

> -

> -	if (!disp_state) {

> -		DRM_ERROR("invalid params\n");

> -		return;

> -	}

> 

> -	/*

> -	 * if there is a coredump pending return immediately till dump

> -	 * if read by userspace or timeout happens

> -	 */

> -	if (disp_state->coredump_pending) {

> -		DRM_DEBUG("coredump is pending read\n");

> -		return;

> -	}

> -

> -	kthread_queue_work(disp_state->dump_worker,

> -			&disp_state->dump_work);

> +	kthread_queue_work(kms->dump_worker, &kms->dump_work);

>  }

> 

>  int msm_disp_snapshot_init(struct drm_device *drm_dev)

>  {

>  	struct msm_drm_private *priv;

> -	struct msm_disp_state *disp_state;

>  	struct msm_kms *kms;

> 

>  	if (!drm_dev) {

> @@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device 

> *drm_dev)

>  	priv = drm_dev->dev_private;

>  	kms = priv->kms;

> 

> -	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct

> msm_disp_state), GFP_KERNEL);

> -

> -	mutex_init(&disp_state->mutex);

> +	mutex_init(&kms->dump_mutex);

> 

> -	disp_state->dev = drm_dev->dev;

> -	disp_state->drm_dev = drm_dev;

> -

> -	INIT_LIST_HEAD(&disp_state->blocks);

> -

> -	disp_state->dump_worker = kthread_create_worker(0, "%s", 

> "disp_snapshot");

> -	if (IS_ERR(disp_state->dump_worker))

> +	kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");

> +	if (IS_ERR(kms->dump_worker))

>  		DRM_ERROR("failed to create disp state task\n");

> 

> -	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);

> -

> -	kms->disp_state = disp_state;

> +	kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);

> 

>  	return 0;

>  }

> @@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device 

> *drm_dev)

>  {

>  	struct msm_kms *kms;

>  	struct msm_drm_private *priv;

> -	struct msm_disp_state *disp_state;

> 

>  	if (!drm_dev) {

>  		DRM_ERROR("invalid params\n");

> @@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device 

> *drm_dev)

> 

>  	priv = drm_dev->dev_private;

>  	kms = priv->kms;

> -	disp_state = kms->disp_state;

> -

> -	if (disp_state->dump_worker)

> -		kthread_destroy_worker(disp_state->dump_worker);

> 

> -	list_del(&disp_state->blocks);

> +	if (kms->dump_worker)

> +		kthread_destroy_worker(kms->dump_worker);

> 

> -	mutex_destroy(&disp_state->mutex);

> +	mutex_destroy(&kms->dump_mutex);

>  }

> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h

> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h

> index 32f52799a1ba..c6174a366095 100644

> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h

> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h

> @@ -41,26 +41,17 @@

>   * struct msm_disp_state - structure to store current dpu state

>   * @dev: device pointer

>   * @drm_dev: drm device pointer

> - * @mutex: mutex to serialize access to serialze dumps, debugfs access

> - * @coredump_pending: coredump is pending read from userspace

>   * @atomic_state: atomic state duplicated at the time of the error

> - * @dump_worker: kworker thread which runs the dump work

> - * @dump_work: kwork which dumps the registers and drm state

>   * @timestamp: timestamp at which the coredump was captured

>   */

>  struct msm_disp_state {

>  	struct device *dev;

>  	struct drm_device *drm_dev;

> -	struct mutex mutex;

> -

> -	bool coredump_pending;

> 

>  	struct list_head blocks;

> 

>  	struct drm_atomic_state *atomic_state;

> 

> -	struct kthread_worker *dump_worker;

> -	struct kthread_work dump_work;

>  	ktime_t timestamp;

>  };

> 

> @@ -123,11 +114,11 @@ void msm_disp_snapshot_capture_state(struct

> msm_disp_state *disp_state);

> 

>  /**

>   * msm_disp_state_free - free the memory after the coredump has been 

> read

> - * @disp_state:	    handle to struct msm_disp_state

> + * @data:	    handle to struct msm_disp_state

> 

>   * Returns: none

>   */

> -void msm_disp_state_free(struct msm_disp_state *disp_state);

> +void msm_disp_state_free(void *data);

> 

>  /**

>   * msm_disp_snapshot_add_block - add a hardware block with its 

> register dump

> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c

> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c

> index ca6632550337..cabe15190ec1 100644

> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c

> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c

> @@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct

> msm_disp_state *disp_state)

>  	msm_disp_capture_atomic_state(disp_state);

>  }

> 

> -void msm_disp_state_free(struct msm_disp_state *disp_state)

> +void msm_disp_state_free(void *data)

>  {

> +	struct msm_disp_state *disp_state = data;

>  	struct msm_disp_state_block *block, *tmp;

> 

>  	if (disp_state->atomic_state) {

> @@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state 

> *disp_state)

>  		kfree(block->state);

>  		kfree(block);

>  	}

> +

> +	kfree(disp_state);

>  }

> 

>  void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, 

> u32 len,

> diff --git a/drivers/gpu/drm/msm/msm_kms.h 

> b/drivers/gpu/drm/msm/msm_kms.h

> index 146dcab123f4..086a2d59b8c8 100644

> --- a/drivers/gpu/drm/msm/msm_kms.h

> +++ b/drivers/gpu/drm/msm/msm_kms.h

> @@ -156,8 +156,10 @@ struct msm_kms {

>  	/* mapper-id used to request GEM buffer mapped for scanout: */

>  	struct msm_gem_address_space *aspace;

> 

> -	/* handle to disp snapshot state */

> -	struct msm_disp_state *disp_state;

> +	/* disp snapshot support */

> +	struct kthread_worker *dump_worker;

> +	struct kthread_work dump_work;

> +	struct mutex dump_mutex;

> 

>  	/*

>  	 * For async commit, where ->flush_commit() and later happens
Dmitry Baryshkov April 27, 2021, 8:29 p.m. UTC | #2
On Tue, 27 Apr 2021 at 22:19, <abhinavk@codeaurora.org> wrote:
>
> Hi Dmitry
>
> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> > Instead of allocating snapshotting structure at the driver probe time
> > and later handling concurrent access, actual state, etc, make
> > msm_disp_state transient struct. Allocate one when snapshotting happens
> > and free it after coredump data is read by userspace.
> >
> Can you please check my previous comment on coredump_pending?
>
> https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa3509f@codeaurora.org/T/#u
>
> That helps to serialize read/write of coredump.

Regarding the serialization of read/write. As the disp_state becomes
the transient object, the need for such serialization vanishes:
 - Before the snapshot is complete, the object is not linked outside
of msm_disp_snapshot functions.
 - When the snapshot is complete, it becomes linked from the codedump
subsystem. After this it is only read by the coredump, nobody will
write to it.
 - Next snapshot will allocate a new disp_state structure.
 - If dev_coredumpm is called when the previous snapshot is still
referenced (the device exists), the new snapshot is silently freed.

Thus there is no need to sync the read/write operations. They are now
naturally serialized.

>
> Rest of the changes on this one look fine to me.
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
> >  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
> >  drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
> >  4 files changed, 37 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > index 70fd5a1fe13e..a4a7cb06bc87 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > @@ -7,8 +7,7 @@
> >
> >  #include "msm_disp_snapshot.h"
> >
> > -#ifdef CONFIG_DEV_COREDUMP
> > -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> > +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
> > loff_t offset,
> >               size_t count, void *data, size_t datalen)
> >  {
> >       struct drm_print_iterator iter;
> > @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
> > loff_t offset,
> >       return count - iter.remain;
> >  }
> >
> > -static void disp_devcoredump_free(void *data)
> > +static void _msm_disp_snapshot_work(struct kthread_work *work)
> >  {
> > +     struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
> > +     struct drm_device *drm_dev = kms->dev;
> >       struct msm_disp_state *disp_state;
> > +     struct drm_printer p;
> >
> > -     disp_state = data;
> > -
> > -     msm_disp_state_free(disp_state);
> > +     disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> > +     if (!disp_state)
> > +             return;
> >
> > -     disp_state->coredump_pending = false;
> > -}
> > -#endif /* CONFIG_DEV_COREDUMP */
> > +     disp_state->dev = drm_dev->dev;
> > +     disp_state->drm_dev = drm_dev;
> >
> > -static void _msm_disp_snapshot_work(struct kthread_work *work)
> > -{
> > -     struct msm_disp_state *disp_state = container_of(work, struct
> > msm_disp_state, dump_work);
> > -     struct drm_printer p;
> > +     INIT_LIST_HEAD(&disp_state->blocks);
> >
> > -     mutex_lock(&disp_state->mutex);
> > +     /* Serialize dumping here */
> > +     mutex_lock(&kms->dump_mutex);
> >
> >       msm_disp_snapshot_capture_state(disp_state);
> >
> > +     mutex_unlock(&kms->dump_mutex);
> > +
> >       if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
> >               p = drm_info_printer(disp_state->drm_dev->dev);
> >               msm_disp_state_print(disp_state, &p);
> >       }
> >
> >       /*
> > -      * if devcoredump is not defined free the state immediately
> > -      * otherwise it will be freed in the free handler.
> > +      * If COREDUMP is disabled, the stub will call the free function.
> > +      * If there is a codedump pending for the device, the dev_coredumpm()
> > +      * will also free new coredump state.
> >        */
> > -#ifdef CONFIG_DEV_COREDUMP
> >       dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0,
> > GFP_KERNEL,
> > -                     disp_devcoredump_read, disp_devcoredump_free);
> > -     disp_state->coredump_pending = true;
> > -#else
> > -     msm_disp_state_free(disp_state);
> > -#endif
> > -
> > -     mutex_unlock(&disp_state->mutex);
> > +                     disp_devcoredump_read, msm_disp_state_free);
> >  }
> >
> >  void msm_disp_snapshot_state(struct drm_device *drm_dev)
> >  {
> >       struct msm_drm_private *priv;
> >       struct msm_kms *kms;
> > -     struct msm_disp_state *disp_state;
> >
> >       if (!drm_dev) {
> >               DRM_ERROR("invalid params\n");
> > @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device
> > *drm_dev)
> >
> >       priv = drm_dev->dev_private;
> >       kms = priv->kms;
> > -     disp_state = kms->disp_state;
> > -
> > -     if (!disp_state) {
> > -             DRM_ERROR("invalid params\n");
> > -             return;
> > -     }
> >
> > -     /*
> > -      * if there is a coredump pending return immediately till dump
> > -      * if read by userspace or timeout happens
> > -      */
> > -     if (disp_state->coredump_pending) {
> > -             DRM_DEBUG("coredump is pending read\n");
> > -             return;
> > -     }
> > -
> > -     kthread_queue_work(disp_state->dump_worker,
> > -                     &disp_state->dump_work);
> > +     kthread_queue_work(kms->dump_worker, &kms->dump_work);
> >  }
> >
> >  int msm_disp_snapshot_init(struct drm_device *drm_dev)
> >  {
> >       struct msm_drm_private *priv;
> > -     struct msm_disp_state *disp_state;
> >       struct msm_kms *kms;
> >
> >       if (!drm_dev) {
> > @@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device
> > *drm_dev)
> >       priv = drm_dev->dev_private;
> >       kms = priv->kms;
> >
> > -     disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct
> > msm_disp_state), GFP_KERNEL);
> > -
> > -     mutex_init(&disp_state->mutex);
> > +     mutex_init(&kms->dump_mutex);
> >
> > -     disp_state->dev = drm_dev->dev;
> > -     disp_state->drm_dev = drm_dev;
> > -
> > -     INIT_LIST_HEAD(&disp_state->blocks);
> > -
> > -     disp_state->dump_worker = kthread_create_worker(0, "%s",
> > "disp_snapshot");
> > -     if (IS_ERR(disp_state->dump_worker))
> > +     kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
> > +     if (IS_ERR(kms->dump_worker))
> >               DRM_ERROR("failed to create disp state task\n");
> >
> > -     kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
> > -
> > -     kms->disp_state = disp_state;
> > +     kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
> >
> >       return 0;
> >  }
> > @@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device
> > *drm_dev)
> >  {
> >       struct msm_kms *kms;
> >       struct msm_drm_private *priv;
> > -     struct msm_disp_state *disp_state;
> >
> >       if (!drm_dev) {
> >               DRM_ERROR("invalid params\n");
> > @@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device
> > *drm_dev)
> >
> >       priv = drm_dev->dev_private;
> >       kms = priv->kms;
> > -     disp_state = kms->disp_state;
> > -
> > -     if (disp_state->dump_worker)
> > -             kthread_destroy_worker(disp_state->dump_worker);
> >
> > -     list_del(&disp_state->blocks);
> > +     if (kms->dump_worker)
> > +             kthread_destroy_worker(kms->dump_worker);
> >
> > -     mutex_destroy(&disp_state->mutex);
> > +     mutex_destroy(&kms->dump_mutex);
> >  }
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > index 32f52799a1ba..c6174a366095 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
> > @@ -41,26 +41,17 @@
> >   * struct msm_disp_state - structure to store current dpu state
> >   * @dev: device pointer
> >   * @drm_dev: drm device pointer
> > - * @mutex: mutex to serialize access to serialze dumps, debugfs access
> > - * @coredump_pending: coredump is pending read from userspace
> >   * @atomic_state: atomic state duplicated at the time of the error
> > - * @dump_worker: kworker thread which runs the dump work
> > - * @dump_work: kwork which dumps the registers and drm state
> >   * @timestamp: timestamp at which the coredump was captured
> >   */
> >  struct msm_disp_state {
> >       struct device *dev;
> >       struct drm_device *drm_dev;
> > -     struct mutex mutex;
> > -
> > -     bool coredump_pending;
> >
> >       struct list_head blocks;
> >
> >       struct drm_atomic_state *atomic_state;
> >
> > -     struct kthread_worker *dump_worker;
> > -     struct kthread_work dump_work;
> >       ktime_t timestamp;
> >  };
> >
> > @@ -123,11 +114,11 @@ void msm_disp_snapshot_capture_state(struct
> > msm_disp_state *disp_state);
> >
> >  /**
> >   * msm_disp_state_free - free the memory after the coredump has been
> > read
> > - * @disp_state:          handle to struct msm_disp_state
> > + * @data:        handle to struct msm_disp_state
> >
> >   * Returns: none
> >   */
> > -void msm_disp_state_free(struct msm_disp_state *disp_state);
> > +void msm_disp_state_free(void *data);
> >
> >  /**
> >   * msm_disp_snapshot_add_block - add a hardware block with its
> > register dump
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > index ca6632550337..cabe15190ec1 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> > @@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct
> > msm_disp_state *disp_state)
> >       msm_disp_capture_atomic_state(disp_state);
> >  }
> >
> > -void msm_disp_state_free(struct msm_disp_state *disp_state)
> > +void msm_disp_state_free(void *data)
> >  {
> > +     struct msm_disp_state *disp_state = data;
> >       struct msm_disp_state_block *block, *tmp;
> >
> >       if (disp_state->atomic_state) {
> > @@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state
> > *disp_state)
> >               kfree(block->state);
> >               kfree(block);
> >       }
> > +
> > +     kfree(disp_state);
> >  }
> >
> >  void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state,
> > u32 len,
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h
> > b/drivers/gpu/drm/msm/msm_kms.h
> > index 146dcab123f4..086a2d59b8c8 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -156,8 +156,10 @@ struct msm_kms {
> >       /* mapper-id used to request GEM buffer mapped for scanout: */
> >       struct msm_gem_address_space *aspace;
> >
> > -     /* handle to disp snapshot state */
> > -     struct msm_disp_state *disp_state;
> > +     /* disp snapshot support */
> > +     struct kthread_worker *dump_worker;
> > +     struct kthread_work dump_work;
> > +     struct mutex dump_mutex;
> >
> >       /*
> >        * For async commit, where ->flush_commit() and later happens
Abhinav Kumar April 27, 2021, 10:11 p.m. UTC | #3
On 2021-04-27 13:29, Dmitry Baryshkov wrote:
> On Tue, 27 Apr 2021 at 22:19, <abhinavk@codeaurora.org> wrote:
>> 
>> Hi Dmitry
>> 
>> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
>> > Instead of allocating snapshotting structure at the driver probe time
>> > and later handling concurrent access, actual state, etc, make
>> > msm_disp_state transient struct. Allocate one when snapshotting happens
>> > and free it after coredump data is read by userspace.
>> >
>> Can you please check my previous comment on coredump_pending?
>> 
>> https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa3509f@codeaurora.org/T/#u
>> 
>> That helps to serialize read/write of coredump.
> 
> Regarding the serialization of read/write. As the disp_state becomes
> the transient object, the need for such serialization vanishes:
>  - Before the snapshot is complete, the object is not linked outside
> of msm_disp_snapshot functions.
>  - When the snapshot is complete, it becomes linked from the codedump
> subsystem. After this it is only read by the coredump, nobody will
> write to it.
>  - Next snapshot will allocate a new disp_state structure.
>  - If dev_coredumpm is called when the previous snapshot is still
> referenced (the device exists), the new snapshot is silently freed.
> 
> Thus there is no need to sync the read/write operations. They are now
> naturally serialized.
Alright, just make sure to validate the robustness of this using the 
method mentioned earlier.
Apart from that,

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> 
>> 
>> Rest of the changes on this one look fine to me.
>> 
>> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++++++-------------
>> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
>> >  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
>> >  drivers/gpu/drm/msm/msm_kms.h                 |  6 +-
>> >  4 files changed, 37 insertions(+), 77 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > index 70fd5a1fe13e..a4a7cb06bc87 100644
>> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
>> > @@ -7,8 +7,7 @@
>> >
>> >  #include "msm_disp_snapshot.h"
>> >
>> > -#ifdef CONFIG_DEV_COREDUMP
>> > -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
>> > +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
>> > loff_t offset,
>> >               size_t count, void *data, size_t datalen)
>> >  {
>> >       struct drm_print_iterator iter;
>> > @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
>> > loff_t offset,
>> >       return count - iter.remain;
>> >  }
>> >
>> > -static void disp_devcoredump_free(void *data)
>> > +static void _msm_disp_snapshot_work(struct kthread_work *work)
>> >  {
>> > +     struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
>> > +     struct drm_device *drm_dev = kms->dev;
>> >       struct msm_disp_state *disp_state;
>> > +     struct drm_printer p;
>> >
>> > -     disp_state = data;
>> > -
>> > -     msm_disp_state_free(disp_state);
>> > +     disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
>> > +     if (!disp_state)
>> > +             return;
>> >
>> > -     disp_state->coredump_pending = false;
>> > -}
>> > -#endif /* CONFIG_DEV_COREDUMP */
>> > +     disp_state->dev = drm_dev->dev;
>> > +     disp_state->drm_dev = drm_dev;
>> >
>> > -static void _msm_disp_snapshot_work(struct kthread_work *work)
>> > -{
>> > -     struct msm_disp_state *disp_state = container_of(work, struct
>> > msm_disp_state, dump_work);
>> > -     struct drm_printer p;
>> > +     INIT_LIST_HEAD(&disp_state->blocks);
>> >
>> > -     mutex_lock(&disp_state->mutex);
>> > +     /* Serialize dumping here */
>> > +     mutex_lock(&kms->dump_mutex);
>> >
>> >       msm_disp_snapshot_capture_state(disp_state);
>> >
>> > +     mutex_unlock(&kms->dump_mutex);
>> > +
>> >       if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
>> >               p = drm_info_printer(disp_state->drm_dev->dev);
>> >               msm_disp_state_print(disp_state, &p);
>> >       }
>> >
>> >       /*
>> > -      * if devcoredump is not defined free the state immediately
>> > -      * otherwise it will be freed in the free handler.
>> > +      * If COREDUMP is disabled, the stub will call the free function.
>> > +      * If there is a codedump pending for the device, the dev_coredumpm()
>> > +      * will also free new coredump state.
>> >        */
>> > -#ifdef CONFIG_DEV_COREDUMP
>> >       dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0,
>> > GFP_KERNEL,
>> > -                     disp_devcoredump_read, disp_devcoredump_free);
>> > -     disp_state->coredump_pending = true;
>> > -#else
>> > -     msm_disp_state_free(disp_state);
>> > -#endif
>> > -
>> > -     mutex_unlock(&disp_state->mutex);
>> > +                     disp_devcoredump_read, msm_disp_state_free);
>> >  }
>> >
>> >  void msm_disp_snapshot_state(struct drm_device *drm_dev)
>> >  {
>> >       struct msm_drm_private *priv;
>> >       struct msm_kms *kms;
>> > -     struct msm_disp_state *disp_state;
>> >
>> >       if (!drm_dev) {
>> >               DRM_ERROR("invalid params\n");
>> > @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device
>> > *drm_dev)
>> >
>> >       priv = drm_dev->dev_private;
>> >       kms = priv->kms;
>> > -     disp_state = kms->disp_state;
>> > -
>> > -     if (!disp_state) {
>> > -             DRM_ERROR("invalid params\n");
>> > -             return;
>> > -     }
>> >
>> > -     /*
>> > -      * if there is a coredump pending return immediately till dump
>> > -      * if read by userspace or timeout happens
>> > -      */
>> > -     if (disp_state->coredump_pending) {
>> > -             DRM_DEBUG("coredump is pending read\n");
>> > -             return;
>> > -     }
>> > -
>> > -     kthread_queue_work(disp_state->dump_worker,
>> > -                     &disp_state->dump_work);
>> > +     kthread_queue_work(kms->dump_worker, &kms->dump_work);
>> >  }
>> >
>> >  int msm_disp_snapshot_init(struct drm_device *drm_dev)
>> >  {
>> >       struct msm_drm_private *priv;
>> > -     struct msm_disp_state *disp_state;
>> >       struct msm_kms *kms;
>> >
>> >       if (!drm_dev) {
>> > @@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device
>> > *drm_dev)
>> >       priv = drm_dev->dev_private;
>> >       kms = priv->kms;
>> >
>> > -     disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct
>> > msm_disp_state), GFP_KERNEL);
>> > -
>> > -     mutex_init(&disp_state->mutex);
>> > +     mutex_init(&kms->dump_mutex);
>> >
>> > -     disp_state->dev = drm_dev->dev;
>> > -     disp_state->drm_dev = drm_dev;
>> > -
>> > -     INIT_LIST_HEAD(&disp_state->blocks);
>> > -
>> > -     disp_state->dump_worker = kthread_create_worker(0, "%s",
>> > "disp_snapshot");
>> > -     if (IS_ERR(disp_state->dump_worker))
>> > +     kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
>> > +     if (IS_ERR(kms->dump_worker))
>> >               DRM_ERROR("failed to create disp state task\n");
>> >
>> > -     kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
>> > -
>> > -     kms->disp_state = disp_state;
>> > +     kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
>> >
>> >       return 0;
>> >  }
>> > @@ -141,7 +109,6 @@ void msm_disp_snapshot_destroy(struct drm_device
>> > *drm_dev)
>> >  {
>> >       struct msm_kms *kms;
>> >       struct msm_drm_private *priv;
>> > -     struct msm_disp_state *disp_state;
>> >
>> >       if (!drm_dev) {
>> >               DRM_ERROR("invalid params\n");
>> > @@ -150,12 +117,9 @@ void msm_disp_snapshot_destroy(struct drm_device
>> > *drm_dev)
>> >
>> >       priv = drm_dev->dev_private;
>> >       kms = priv->kms;
>> > -     disp_state = kms->disp_state;
>> > -
>> > -     if (disp_state->dump_worker)
>> > -             kthread_destroy_worker(disp_state->dump_worker);
>> >
>> > -     list_del(&disp_state->blocks);
>> > +     if (kms->dump_worker)
>> > +             kthread_destroy_worker(kms->dump_worker);
>> >
>> > -     mutex_destroy(&disp_state->mutex);
>> > +     mutex_destroy(&kms->dump_mutex);
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> > index 32f52799a1ba..c6174a366095 100644
>> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
>> > @@ -41,26 +41,17 @@
>> >   * struct msm_disp_state - structure to store current dpu state
>> >   * @dev: device pointer
>> >   * @drm_dev: drm device pointer
>> > - * @mutex: mutex to serialize access to serialze dumps, debugfs access
>> > - * @coredump_pending: coredump is pending read from userspace
>> >   * @atomic_state: atomic state duplicated at the time of the error
>> > - * @dump_worker: kworker thread which runs the dump work
>> > - * @dump_work: kwork which dumps the registers and drm state
>> >   * @timestamp: timestamp at which the coredump was captured
>> >   */
>> >  struct msm_disp_state {
>> >       struct device *dev;
>> >       struct drm_device *drm_dev;
>> > -     struct mutex mutex;
>> > -
>> > -     bool coredump_pending;
>> >
>> >       struct list_head blocks;
>> >
>> >       struct drm_atomic_state *atomic_state;
>> >
>> > -     struct kthread_worker *dump_worker;
>> > -     struct kthread_work dump_work;
>> >       ktime_t timestamp;
>> >  };
>> >
>> > @@ -123,11 +114,11 @@ void msm_disp_snapshot_capture_state(struct
>> > msm_disp_state *disp_state);
>> >
>> >  /**
>> >   * msm_disp_state_free - free the memory after the coredump has been
>> > read
>> > - * @disp_state:          handle to struct msm_disp_state
>> > + * @data:        handle to struct msm_disp_state
>> >
>> >   * Returns: none
>> >   */
>> > -void msm_disp_state_free(struct msm_disp_state *disp_state);
>> > +void msm_disp_state_free(void *data);
>> >
>> >  /**
>> >   * msm_disp_snapshot_add_block - add a hardware block with its
>> > register dump
>> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> > index ca6632550337..cabe15190ec1 100644
>> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
>> > @@ -142,8 +142,9 @@ void msm_disp_snapshot_capture_state(struct
>> > msm_disp_state *disp_state)
>> >       msm_disp_capture_atomic_state(disp_state);
>> >  }
>> >
>> > -void msm_disp_state_free(struct msm_disp_state *disp_state)
>> > +void msm_disp_state_free(void *data)
>> >  {
>> > +     struct msm_disp_state *disp_state = data;
>> >       struct msm_disp_state_block *block, *tmp;
>> >
>> >       if (disp_state->atomic_state) {
>> > @@ -156,6 +157,8 @@ void msm_disp_state_free(struct msm_disp_state
>> > *disp_state)
>> >               kfree(block->state);
>> >               kfree(block);
>> >       }
>> > +
>> > +     kfree(disp_state);
>> >  }
>> >
>> >  void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state,
>> > u32 len,
>> > diff --git a/drivers/gpu/drm/msm/msm_kms.h
>> > b/drivers/gpu/drm/msm/msm_kms.h
>> > index 146dcab123f4..086a2d59b8c8 100644
>> > --- a/drivers/gpu/drm/msm/msm_kms.h
>> > +++ b/drivers/gpu/drm/msm/msm_kms.h
>> > @@ -156,8 +156,10 @@ struct msm_kms {
>> >       /* mapper-id used to request GEM buffer mapped for scanout: */
>> >       struct msm_gem_address_space *aspace;
>> >
>> > -     /* handle to disp snapshot state */
>> > -     struct msm_disp_state *disp_state;
>> > +     /* disp snapshot support */
>> > +     struct kthread_worker *dump_worker;
>> > +     struct kthread_work dump_work;
>> > +     struct mutex dump_mutex;
>> >
>> >       /*
>> >        * For async commit, where ->flush_commit() and later happens
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index 70fd5a1fe13e..a4a7cb06bc87 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -7,8 +7,7 @@ 
 
 #include "msm_disp_snapshot.h"
 
-#ifdef CONFIG_DEV_COREDUMP
-static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
+static ssize_t __maybe_unused disp_devcoredump_read(char *buffer, loff_t offset,
 		size_t count, void *data, size_t datalen)
 {
 	struct drm_print_iterator iter;
@@ -29,52 +28,47 @@  static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
 	return count - iter.remain;
 }
 
-static void disp_devcoredump_free(void *data)
+static void _msm_disp_snapshot_work(struct kthread_work *work)
 {
+	struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
+	struct drm_device *drm_dev = kms->dev;
 	struct msm_disp_state *disp_state;
+	struct drm_printer p;
 
-	disp_state = data;
-
-	msm_disp_state_free(disp_state);
+	disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
+	if (!disp_state)
+		return;
 
-	disp_state->coredump_pending = false;
-}
-#endif /* CONFIG_DEV_COREDUMP */
+	disp_state->dev = drm_dev->dev;
+	disp_state->drm_dev = drm_dev;
 
-static void _msm_disp_snapshot_work(struct kthread_work *work)
-{
-	struct msm_disp_state *disp_state = container_of(work, struct msm_disp_state, dump_work);
-	struct drm_printer p;
+	INIT_LIST_HEAD(&disp_state->blocks);
 
-	mutex_lock(&disp_state->mutex);
+	/* Serialize dumping here */
+	mutex_lock(&kms->dump_mutex);
 
 	msm_disp_snapshot_capture_state(disp_state);
 
+	mutex_unlock(&kms->dump_mutex);
+
 	if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
 		p = drm_info_printer(disp_state->drm_dev->dev);
 		msm_disp_state_print(disp_state, &p);
 	}
 
 	/*
-	 * if devcoredump is not defined free the state immediately
-	 * otherwise it will be freed in the free handler.
+	 * If COREDUMP is disabled, the stub will call the free function.
+	 * If there is a codedump pending for the device, the dev_coredumpm()
+	 * will also free new coredump state.
 	 */
-#ifdef CONFIG_DEV_COREDUMP
 	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, GFP_KERNEL,
-			disp_devcoredump_read, disp_devcoredump_free);
-	disp_state->coredump_pending = true;
-#else
-	msm_disp_state_free(disp_state);
-#endif
-
-	mutex_unlock(&disp_state->mutex);
+			disp_devcoredump_read, msm_disp_state_free);
 }
 
 void msm_disp_snapshot_state(struct drm_device *drm_dev)
 {
 	struct msm_drm_private *priv;
 	struct msm_kms *kms;
-	struct msm_disp_state *disp_state;
 
 	if (!drm_dev) {
 		DRM_ERROR("invalid params\n");
@@ -83,30 +77,13 @@  void msm_disp_snapshot_state(struct drm_device *drm_dev)
 
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
-	disp_state = kms->disp_state;
-
-	if (!disp_state) {
-		DRM_ERROR("invalid params\n");
-		return;
-	}
 
-	/*
-	 * if there is a coredump pending return immediately till dump
-	 * if read by userspace or timeout happens
-	 */
-	if (disp_state->coredump_pending) {
-		DRM_DEBUG("coredump is pending read\n");
-		return;
-	}
-
-	kthread_queue_work(disp_state->dump_worker,
-			&disp_state->dump_work);
+	kthread_queue_work(kms->dump_worker, &kms->dump_work);
 }
 
 int msm_disp_snapshot_init(struct drm_device *drm_dev)
 {
 	struct msm_drm_private *priv;
-	struct msm_disp_state *disp_state;
 	struct msm_kms *kms;
 
 	if (!drm_dev) {
@@ -117,22 +94,13 @@  int msm_disp_snapshot_init(struct drm_device *drm_dev)
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
 
-	disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);
-
-	mutex_init(&disp_state->mutex);
+	mutex_init(&kms->dump_mutex);
 
-	disp_state->dev = drm_dev->dev;
-	disp_state->drm_dev = drm_dev;
-
-	INIT_LIST_HEAD(&disp_state->blocks);
-
-	disp_state->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
-	if (IS_ERR(disp_state->dump_worker))
+	kms->dump_worker = kthread_create_worker(0, "%s", "disp_snapshot");
+	if (IS_ERR(kms->dump_worker))
 		DRM_ERROR("failed to create disp state task\n");
 
-	kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
-
-	kms->disp_state = disp_state;
+	kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
 
 	return 0;
 }
@@ -141,7 +109,6 @@  void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
 {
 	struct msm_kms *kms;
 	struct msm_drm_private *priv;
-	struct msm_disp_state *disp_state;
 
 	if (!drm_dev) {
 		DRM_ERROR("invalid params\n");
@@ -150,12 +117,9 @@  void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
 
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
-	disp_state = kms->disp_state;
-
-	if (disp_state->dump_worker)
-		kthread_destroy_worker(disp_state->dump_worker);
 
-	list_del(&disp_state->blocks);
+	if (kms->dump_worker)
+		kthread_destroy_worker(kms->dump_worker);
 
-	mutex_destroy(&disp_state->mutex);
+	mutex_destroy(&kms->dump_mutex);
 }
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
index 32f52799a1ba..c6174a366095 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h
@@ -41,26 +41,17 @@ 
  * struct msm_disp_state - structure to store current dpu state
  * @dev: device pointer
  * @drm_dev: drm device pointer
- * @mutex: mutex to serialize access to serialze dumps, debugfs access
- * @coredump_pending: coredump is pending read from userspace
  * @atomic_state: atomic state duplicated at the time of the error
- * @dump_worker: kworker thread which runs the dump work
- * @dump_work: kwork which dumps the registers and drm state
  * @timestamp: timestamp at which the coredump was captured
  */
 struct msm_disp_state {
 	struct device *dev;
 	struct drm_device *drm_dev;
-	struct mutex mutex;
-
-	bool coredump_pending;
 
 	struct list_head blocks;
 
 	struct drm_atomic_state *atomic_state;
 
-	struct kthread_worker *dump_worker;
-	struct kthread_work dump_work;
 	ktime_t timestamp;
 };
 
@@ -123,11 +114,11 @@  void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state);
 
 /**
  * msm_disp_state_free - free the memory after the coredump has been read
- * @disp_state:	    handle to struct msm_disp_state
+ * @data:	    handle to struct msm_disp_state
 
  * Returns: none
  */
-void msm_disp_state_free(struct msm_disp_state *disp_state);
+void msm_disp_state_free(void *data);
 
 /**
  * msm_disp_snapshot_add_block - add a hardware block with its register dump
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index ca6632550337..cabe15190ec1 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -142,8 +142,9 @@  void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	msm_disp_capture_atomic_state(disp_state);
 }
 
-void msm_disp_state_free(struct msm_disp_state *disp_state)
+void msm_disp_state_free(void *data)
 {
+	struct msm_disp_state *disp_state = data;
 	struct msm_disp_state_block *block, *tmp;
 
 	if (disp_state->atomic_state) {
@@ -156,6 +157,8 @@  void msm_disp_state_free(struct msm_disp_state *disp_state)
 		kfree(block->state);
 		kfree(block);
 	}
+
+	kfree(disp_state);
 }
 
 void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 146dcab123f4..086a2d59b8c8 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -156,8 +156,10 @@  struct msm_kms {
 	/* mapper-id used to request GEM buffer mapped for scanout: */
 	struct msm_gem_address_space *aspace;
 
-	/* handle to disp snapshot state */
-	struct msm_disp_state *disp_state;
+	/* disp snapshot support */
+	struct kthread_worker *dump_worker;
+	struct kthread_work dump_work;
+	struct mutex dump_mutex;
 
 	/*
 	 * For async commit, where ->flush_commit() and later happens