Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

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

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