| Message ID | 20210427001828.2375555-3-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm/msm: improve register snapshotting | expand |
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
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
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 --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
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(-)