dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: abhinavk@codeaurora.org
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: freedreno <freedreno@lists.freedesktop.org>,
	Jonathan Marek <jonathan@marek.ca>,
	Stephen Boyd <sboyd@kernel.org>,
	"open list:DRM DRIVER FOR MSM ADRENO GPU"
	<linux-arm-msm@vger.kernel.org>,
	"open list:DRM DRIVER FOR MSM ADRENO GPU"
	<dri-devel@lists.freedesktop.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	David Airlie <airlied@linux.ie>, Sean Paul <sean@poorly.run>
Subject: Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct
Date: Tue, 27 Apr 2021 15:11:32 -0700	[thread overview]
Message-ID: <e2d378fb49b344724277f2e65794af46@codeaurora.org> (raw)
In-Reply-To: <CAA8EJpqpHqjWups3_fQfxjJhFXO+Z1Zr0LVwEy+8C-2GMj8oyw@mail.gmail.com>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-04-27 22:11 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-27  0:18 [PATCH v2 0/4] drm/msm: improve register snapshotting Dmitry Baryshkov
2021-04-27  0:18 ` [PATCH v2 1/4] drm/msm: pass dump state as a function argument Dmitry Baryshkov
2021-04-27  0:18 ` [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct Dmitry Baryshkov
2021-04-27 19:19   ` [Freedreno] " abhinavk
2021-04-27 20:29     ` Dmitry Baryshkov
2021-04-27 22:11       ` abhinavk [this message]
2021-04-27  0:18 ` [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size Dmitry Baryshkov
2021-04-27 19:29   ` [Freedreno] " abhinavk
2021-04-27 20:32     ` Dmitry Baryshkov
2021-04-27 22:12       ` abhinavk
2021-04-28  2:47   ` Bjorn Andersson
2021-04-28 13:41     ` Dmitry Baryshkov
2021-04-28 13:59       ` Bjorn Andersson
2021-04-28 14:03         ` Dmitry Baryshkov
2021-04-27  0:18 ` [PATCH v2 4/4] drm/msm/dsi: add DSI PHY registers to snapshot data Dmitry Baryshkov
2021-04-27 22:14   ` abhinavk
2021-04-27 19:10 ` [PATCH v2 0/4] drm/msm: improve register snapshotting abhinavk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e2d378fb49b344724277f2e65794af46@codeaurora.org \
    --to=abhinavk@codeaurora.org \
    --cc=airlied@linux.ie \
    --cc=bjorn.andersson@linaro.org \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=jonathan@marek.ca \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=sboyd@kernel.org \
    --cc=sean@poorly.run \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).