[Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Apr 27 20:29:05 UTC 2021
On Tue, 27 Apr 2021 at 22:19, <abhinavk at 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 at 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
--
With best wishes
Dmitry
More information about the dri-devel
mailing list