linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rob Clark <robdclark@gmail.com>
To: Abhinav Kumar <abhinavk@codeaurora.org>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	Sean Paul <seanpaul@chromium.org>,
	Stephen Boyd <swboyd@chromium.org>,
	nganji@codeaurora.org, aravindh@codeaurora.org,
	Kuogee Hsieh <khsieh@codeaurora.org>,
	Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH v3 2/3] drm/msm/dpu: add support to dump dpu registers
Date: Fri, 9 Apr 2021 13:38:42 -0700	[thread overview]
Message-ID: <CAF6AEGt+-XmP-diTwVWoSRNM0u3ehY8ZkeHDdK3ZbVzUcmxX4A@mail.gmail.com> (raw)
In-Reply-To: <1617935317-15571-3-git-send-email-abhinavk@codeaurora.org>

On Thu, Apr 8, 2021 at 7:28 PM Abhinav Kumar <abhinavk@codeaurora.org> wrote:
>
> Add the dpu_dbg module which adds supports to dump dpu registers
> which can be used in case of error conditions.
>
> changes in v3:
>  - Get rid of registration mechanism for sub-modules and instead get
>    this information from the dpu catalog itself
>  - Get rid of global dpu_dbg struct and instead store it in dpu_kms
>  - delegate the power management of the sub-modules to the resp drivers
>  - refactor and remove the linked list logic and simplify it to have
>    just an array
>
> Change-Id: Ide975ecf5d7952ae44daaa6eb611e27d09630be5
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/Makefile                   |   2 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c        | 221 +++++++++++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h        | 200 +++++++++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c   | 257 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |   2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        |  86 +++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h        |   5 +
>  drivers/gpu/drm/msm/dp/dp_catalog.c            |  10 +
>  drivers/gpu/drm/msm/dp/dp_catalog.h            |   5 +
>  drivers/gpu/drm/msm/dp/dp_display.c            |  37 ++++
>  drivers/gpu/drm/msm/dp/dp_display.h            |   1 +
>  drivers/gpu/drm/msm/dsi/dsi.c                  |   5 +
>  drivers/gpu/drm/msm/dsi/dsi.h                  |   4 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c             |  25 +++
>  drivers/gpu/drm/msm/msm_drv.c                  |  29 ++-
>  drivers/gpu/drm/msm/msm_drv.h                  |   2 +
>  16 files changed, 889 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c
>

[snip]

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
> new file mode 100644
> index 0000000..302205a
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
> @@ -0,0 +1,200 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef DPU_DBG_H_
> +#define DPU_DBG_H_
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_device.h>
> +#include "../../../drm_crtc_internal.h"
> +#include <drm/drm_print.h>
> +#include <drm/drm_atomic.h>
> +#include <linux/debugfs.h>
> +#include <linux/list.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/ktime.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +#include <linux/dma-buf.h>
> +#include <linux/slab.h>
> +#include <linux/list_sort.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/kthread.h>
> +#include <linux/devcoredump.h>
> +#include <stdarg.h>
> +#include "dpu_hw_catalog.h"
> +#include "dpu_kms.h"
> +#include "dsi.h"
> +
> +#define DPU_DBG_DUMP_DATA_LIMITER (NULL)
> +
> +enum dpu_dbg_dump_flag {
> +       DPU_DBG_DUMP_IN_LOG = BIT(0),
> +       DPU_DBG_DUMP_IN_MEM = BIT(1),
> +       DPU_DBG_DUMP_IN_COREDUMP = BIT(2),
> +};

overall, I like this better, but..

I'm not completely convinced about the need for
DUMP_IN_LOG/DUMP_IN_MEM.. we haven't really needed it on the GPU side
of things, and the only case I can think of where it might be useful
is if you can't boot far enough to get to some minimal userspace..
once you have at least some minimal userspace, you can just pull out
and clear the devcore dump via sysfs.  That said, if state snapshot
and printing were better separated it would just take a few lines of
code to use a different drm_printer to print the state snapshot to
dmesg.

[snip]

> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index ab281cb..d1675ee 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -2489,3 +2489,28 @@ struct drm_bridge *msm_dsi_host_get_bridge(struct mipi_dsi_host *host)
>
>         return of_drm_find_bridge(msm_host->device_node);
>  }
> +
> +void msm_dsi_host_dump_regs(struct mipi_dsi_host *host)
> +{
> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +       struct drm_device *dev = msm_host->dev;
> +       struct dpu_dbg_base *dpu_dbg;
> +
> +       dpu_dbg = dpu_dbg_get(dev);
> +
> +       if (dpu_dbg_is_drm_printer_needed(dpu_dbg) &&
> +                       !dpu_dbg->dpu_dbg_printer) {
> +               pr_err("invalid drm printer\n");
> +               return;
> +       }

for example, here ^^^

why should the other blocks even care?  All they should know is that
they've been asked to snapshot their state..

> +       if (dpu_dbg->reg_dump_method == DPU_DBG_DUMP_IN_MEM)
> +               pm_runtime_get_sync(&msm_host->pdev->dev);
> +
> +       dpu_dbg_dump_regs(&dpu_dbg->dsi_ctrl_regs[msm_host->id],
> +                       msm_iomap_size(msm_host->pdev, "dsi_ctrl"), msm_host->ctrl_base,
> +                       dpu_dbg->reg_dump_method, dpu_dbg->dpu_dbg_printer);
> +
> +       if (dpu_dbg->reg_dump_method == DPU_DBG_DUMP_IN_MEM)
> +               pm_runtime_put_sync(&msm_host->pdev->dev);
> +}

I'm thinking something more along the lines of (in drm/msm/disp so it
is a little less weird for dsi/dp/etc to be #include'ing header with
structs):

struct msm_disp_state {
    .. maybe a bit of generic information like kernel version, time, etc ...

   struct list_head blocks;  /* list of msm_disp_state_block */
};

struct msm_disp_state_block {
   const char *name;
   struct list_head node;  /* node in msm_disp_state::blocks */
   unsigned size;
   uint32_t state[];
};

struct msm_disp_state * msm_disp_snapshot_state(struct drm_device *dev)
{
   struct msm_disp_state *state = ...;

   if (priv->kms)
      priv->kms->snapshot(priv->kms, state);

   for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
      msm_dsi_snapshot(priv->dsi[i], state);

   if (priv->dp)
      msm_dp_snapshot(priv->dp, state);

   return state;
}

... maybe add a helper that can be used everywhere to allocate and
append a new state block.. Ie. pass it state/name/len/iomem

void msm_disp_state_free(struct msm_disp_state *state)
{
   list_for_each_entry_safe (block, ...)
     kfree(block);
   kfree(state)
}

void msm_disp_state_print(struct drm_disp_state *state, drm_printer *p)
{
   .. print generic state and loop over state->blocks printing them ..
}

... if you do need to support printing to dmesg, you could re-use
msm_disp_state_print() with an drm_info printer in just a few lines of
code, rather than spreading the logic far and wide.

BR,
-R

  parent reply	other threads:[~2021-04-09 20:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-09  2:28 [PATCH v3 0/3] Add devcoredump support for DPU Abhinav Kumar
2021-04-09  2:28 ` [PATCH v3 1/3] drm: allow drm_atomic_print_state() to accept any drm_printer Abhinav Kumar
2021-04-09  2:28 ` [PATCH v3 2/3] drm/msm/dpu: add support to dump dpu registers Abhinav Kumar
2021-04-09  6:41   ` kernel test robot
2021-04-09 20:38   ` Rob Clark [this message]
2021-04-09 21:20     ` abhinavk
2021-04-09  2:28 ` [PATCH v3 3/3] drm/msm/dpu: add dpu_dbg points across dpu driver Abhinav Kumar

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=CAF6AEGt+-XmP-diTwVWoSRNM0u3ehY8ZkeHDdK3ZbVzUcmxX4A@mail.gmail.com \
    --to=robdclark@gmail.com \
    --cc=abhinavk@codeaurora.org \
    --cc=aravindh@codeaurora.org \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=khsieh@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=nganji@codeaurora.org \
    --cc=seanpaul@chromium.org \
    --cc=swboyd@chromium.org \
    /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).