This series adds support to use devcoredump for DPU driver. It introduces the msm_disp_snapshot module which assists in the capturing of register dumps during error scenarios. When a display related error happens, the msm_disp_snapshot module captures all the relevant register dumps along with the snapshot of the drm atomic state and triggers a devcoredump.
changes in v4: - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp - start using a list of blocks to store the hardware block information - cleanup block allocation and freeing logic to simplify it
Abhinav Kumar (3): drm: allow drm_atomic_print_state() to accept any drm_printer drm/msm: add support to take dpu snapshot drm/msm: add disp snapshot points across dpu driver
drivers/gpu/drm/drm_atomic.c | 28 +++- drivers/gpu/drm/drm_atomic_uapi.c | 4 +- drivers/gpu/drm/drm_crtc_internal.h | 4 +- drivers/gpu/drm/msm/Makefile | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 14 +- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 +++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 5 + drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 161 ++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot.h | 167 +++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 +++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 12 ++ drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_display.c | 29 ++++ 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 | 19 +++ drivers/gpu/drm/msm/msm_drv.c | 29 +++- drivers/gpu/drm/msm/msm_drv.h | 2 + drivers/gpu/drm/selftests/test-drm_framebuffer.c | 1 + 23 files changed, 743 insertions(+), 18 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
Currently drm_atomic_print_state() internally allocates and uses a drm_info printer. Allow it to accept any drm_printer type so that the API can be leveraged even for taking drm snapshot.
Rename the drm_atomic_print_state() to drm_atomic_print_new_state() so that it reflects its functionality better.
changes in v4: - none
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Abhinav Kumar abhinavk@codeaurora.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic.c | 28 +++++++++++++++++++----- drivers/gpu/drm/drm_atomic_uapi.c | 4 +++- drivers/gpu/drm/drm_crtc_internal.h | 4 +++- drivers/gpu/drm/selftests/test-drm_framebuffer.c | 1 + 4 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index dda6005..7041a26 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1,6 +1,7 @@ /* * Copyright (C) 2014 Red Hat * Copyright (C) 2014 Intel Corp. + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -1573,9 +1574,20 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, } EXPORT_SYMBOL(__drm_atomic_helper_set_config);
-void drm_atomic_print_state(const struct drm_atomic_state *state) +/** + * drm_atomic_print_new_state - prints drm atomic state + * @state: atomic configuration to check + * @p: drm printer + * + * This functions prints the drm atomic state snapshot using the drm printer + * which is passed to it. This snapshot can be used for debugging purposes. + * + * Note that this function looks into the new state objects and hence its not + * safe to be used after the call to drm_atomic_helper_commit_hw_done(). + */ +void drm_atomic_print_new_state(const struct drm_atomic_state *state, + struct drm_printer *p) { - struct drm_printer p = drm_info_printer(state->dev->dev); struct drm_plane *plane; struct drm_plane_state *plane_state; struct drm_crtc *crtc; @@ -1584,17 +1596,23 @@ void drm_atomic_print_state(const struct drm_atomic_state *state) struct drm_connector_state *connector_state; int i;
+ if (!p) { + DRM_ERROR("invalid drm printer\n"); + return; + } + DRM_DEBUG_ATOMIC("checking %p\n", state);
for_each_new_plane_in_state(state, plane, plane_state, i) - drm_atomic_plane_print_state(&p, plane_state); + drm_atomic_plane_print_state(p, plane_state);
for_each_new_crtc_in_state(state, crtc, crtc_state, i) - drm_atomic_crtc_print_state(&p, crtc_state); + drm_atomic_crtc_print_state(p, crtc_state);
for_each_new_connector_in_state(state, connector, connector_state, i) - drm_atomic_connector_print_state(&p, connector_state); + drm_atomic_connector_print_state(p, connector_state); } +EXPORT_SYMBOL(drm_atomic_print_new_state);
static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p, bool take_locks) diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69..c340a67 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -2,6 +2,7 @@ * Copyright (C) 2014 Red Hat * Copyright (C) 2014 Intel Corp. * Copyright (C) 2018 Intel Corp. + * Copyright (c) 2020, The Linux Foundation. All rights reserved. * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -1321,6 +1322,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences; + struct drm_printer p = drm_info_printer(dev->dev);
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1453,7 +1455,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, ret = drm_atomic_nonblocking_commit(state); } else { if (drm_debug_enabled(DRM_UT_STATE)) - drm_atomic_print_state(state); + drm_atomic_print_new_state(state, &p);
ret = drm_atomic_commit(state); } diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 54d4cf1..1ca51ad 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -5,6 +5,7 @@ * Jesse Barnes jesse.barnes@intel.com * Copyright © 2014 Intel Corporation * Daniel Vetter daniel.vetter@ffwll.ch + * Copyright (c) 2020, The Linux Foundation. All rights reserved. * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -236,7 +237,8 @@ int __drm_atomic_helper_disable_plane(struct drm_plane *plane, int __drm_atomic_helper_set_config(struct drm_mode_set *set, struct drm_atomic_state *state);
-void drm_atomic_print_state(const struct drm_atomic_state *state); +void drm_atomic_print_new_state(const struct drm_atomic_state *state, + struct drm_printer *p);
/* drm_atomic_uapi.c */ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, diff --git a/drivers/gpu/drm/selftests/test-drm_framebuffer.c b/drivers/gpu/drm/selftests/test-drm_framebuffer.c index 789f227..61b44d3 100644 --- a/drivers/gpu/drm/selftests/test-drm_framebuffer.c +++ b/drivers/gpu/drm/selftests/test-drm_framebuffer.c @@ -8,6 +8,7 @@ #include <drm/drm_device.h> #include <drm/drm_mode.h> #include <drm/drm_fourcc.h> +#include <drm/drm_print.h>
#include "../drm_crtc_internal.h"
Add the msm_disp_snapshot module which adds supports to dump dpu registers and capture the drm atomic state which can be used in case of error conditions.
changes in v4: - rename dpu_dbg to msm_disp_snapshot and move it to msm/disp - start using a list of blocks to store the hardware block information - cleanup block allocation and freeing logic to simplify it
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_hw_catalog.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 ++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 5 + drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 161 +++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot.h | 167 ++++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 12 ++ drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_display.c | 29 ++++ 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 | 19 +++ drivers/gpu/drm/msm/msm_drv.c | 29 +++- drivers/gpu/drm/msm/msm_drv.h | 2 + 16 files changed, 682 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 610d630..65d86ce 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -77,6 +77,8 @@ msm-y := \ disp/dpu1/dpu_plane.o \ disp/dpu1/dpu_rm.o \ disp/dpu1/dpu_vbif.o \ + disp/msm_disp_snapshot.o \ + disp/msm_disp_snapshot_util.o \ msm_atomic.o \ msm_atomic_tracepoints.o \ msm_debugfs.o \ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index 4dfd8a2..0f9f0a5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved. +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved. */
#ifndef _DPU_HW_CATALOG_H diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 88e9cc3..7529566 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms) dpu_core_irq_uninstall(dpu_kms); }
+void dpu_kms_mdp_snapshot(struct drm_device *dev) +{ + int i; + struct msm_drm_private *priv; + struct dpu_kms *dpu_kms; + struct dpu_mdss_cfg *cat; + struct dpu_hw_mdp *top; + struct msm_disp_state *disp_state; + char name[SZ_128]; + + priv = dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + disp_state = dpu_kms->disp_state; + + cat = dpu_kms->catalog; + top = dpu_kms->hw_mdp; + + pm_runtime_get_sync(&dpu_kms->pdev->dev); + + /* dump CTL sub-blocks HW regs info */ + for (i = 0; i < cat->ctl_count; i++) { + snprintf(name, SZ_128, "ctl_%d", i); + msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len, + dpu_kms->mmio + cat->ctl[i].base); + } + + /* dump DSPP sub-blocks HW regs info */ + for (i = 0; i < cat->dspp_count; i++) { + snprintf(name, SZ_128, "dspp_%d", i); + msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len, + dpu_kms->mmio + cat->dspp[i].base); + } + + /* dump INTF sub-blocks HW regs info */ + for (i = 0; i < cat->intf_count; i++) { + snprintf(name, SZ_128, "intf_%d", i); + msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len, + dpu_kms->mmio + cat->intf[i].base); + } + + /* dump PP sub-blocks HW regs info */ + for (i = 0; i < cat->pingpong_count; i++) { + snprintf(name, SZ_128, "ping-pong_%d", i); + msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len, + dpu_kms->mmio + cat->pingpong[i].base); + } + + /* dump SSPP sub-blocks HW regs info */ + for (i = 0; i < cat->sspp_count; i++) { + snprintf(name, SZ_128, "sspp_%d", i); + msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len, + dpu_kms->mmio + cat->sspp[i].base); + } + + snprintf(name, SZ_128, "top"); + msm_disp_snapshot_add_block(disp_state, name, top->hw.length, + dpu_kms->mmio + top->hw.blk_off); + + pm_runtime_put_sync(&dpu_kms->pdev->dev); +} + static const struct msm_kms_funcs kms_funcs = { .hw_init = dpu_kms_hw_init, .irq_preinstall = dpu_irq_preinstall, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index d6717d6..74b6ce6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -13,6 +13,7 @@ #include <drm/drm_drv.h>
#include "msm_drv.h" +#include "disp/msm_disp_snapshot.h" #include "msm_kms.h" #include "msm_mmu.h" #include "msm_gem.h" @@ -132,6 +133,8 @@ struct dpu_kms {
struct opp_table *opp_table;
+ struct msm_disp_state *disp_state; + struct dss_module_power mp;
/* reference count bandwidth requests, so we know when we can @@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder); */ u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
+void dpu_kms_mdp_snapshot(struct drm_device *dev); + #endif /* __dpu_kms_H__ */ diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c new file mode 100644 index 0000000..1bdad16 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. + */ + +#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__ + +#include "msm_disp_snapshot.h" + +#ifdef CONFIG_DEV_COREDUMP +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset, + size_t count, void *data, size_t datalen) +{ + struct drm_print_iterator iter; + struct drm_printer p; + struct msm_disp_state *disp_state; + + disp_state = data; + + iter.data = buffer; + iter.offset = 0; + iter.start = offset; + iter.remain = count; + + p = drm_coredump_printer(&iter); + + msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP); + + return count - iter.remain; +} + +static void disp_devcoredump_free(void *data) +{ + struct msm_disp_state *disp_state; + + disp_state = data; + + msm_disp_state_free(disp_state); + + disp_state->coredump_pending = false; +} +#endif /* CONFIG_DEV_COREDUMP */ + +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; + + mutex_lock(&disp_state->mutex); + + msm_disp_snapshot_capture_state(disp_state); + + if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) { + p = drm_info_printer(disp_state->drm_dev->dev); + msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG); + } + + /* + * if devcoredump is not defined free the state immediately + * otherwise it will be freed in the free handler. + */ +#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); +} + +void msm_disp_snapshot_state(struct drm_device *drm_dev) +{ + struct msm_drm_private *priv; + struct dpu_kms *dpu_kms; + struct msm_disp_state *disp_state; + + if (!drm_dev) { + DRM_ERROR("invalid params\n"); + return; + } + + priv = drm_dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + disp_state = dpu_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); +} + +int msm_disp_snapshot_init(struct drm_device *drm_dev) +{ + struct dpu_kms *dpu_kms; + struct msm_drm_private *priv; + struct msm_disp_state *disp_state; + + if (!drm_dev) { + DRM_ERROR("invalid params\n"); + return -EINVAL; + } + + priv = drm_dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + + disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL); + + mutex_init(&disp_state->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)) + DRM_ERROR("failed to create disp state task\n"); + + kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work); + + dpu_kms->disp_state = disp_state; + + return 0; +} + +void msm_disp_snapshot_destroy(struct drm_device *drm_dev) +{ + struct dpu_kms *dpu_kms; + struct msm_drm_private *priv; + struct msm_disp_state *disp_state; + + if (!drm_dev) { + DRM_ERROR("invalid params\n"); + return; + } + + priv = drm_dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + disp_state = dpu_kms->disp_state; + + if (disp_state->dump_worker) + kthread_destroy_worker(disp_state->dump_worker); + + list_del(&disp_state->blocks); + + mutex_destroy(&disp_state->mutex); +} diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h new file mode 100644 index 0000000..0ccb7e7 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h @@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. + */ + +#ifndef MSM_DISP_SNAPSHOT_H_ +#define MSM_DISP_SNAPSHOT_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" + +enum msm_disp_snapshot_dump_flag { + MSM_DISP_SNAPSHOT_IN_LOG = BIT(0), + MSM_DISP_SNAPSHOT_IN_MEM = BIT(1), + MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2), +}; + +#define MSM_DISP_SNAPSHOT_MAX_BLKS 10 + +/* debug option to print the registers in logs */ +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0 + +/* print debug ranges in groups of 4 u32s */ +#define REG_DUMP_ALIGN 16 + +/** + * 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; +}; + +/** + * struct msm_disp_state_block - structure to store each hardware block state + * @name: name of the block + * @drm_dev: handle to the linked list head + * @size: size of the register space of this hardware block + * @state: array holding the register dump of this hardware block + * @base_addr: starting address of this hardware block's register space + */ +struct msm_disp_state_block { + char name[SZ_128]; + struct list_head node; + unsigned int size; + u32 *state; + void __iomem *base_addr; +}; + +/** + * MSM_DISP_SNAPSHOT - trigger to dump the display snapshot + * @drm_dev: handle to drm device + */ +#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev) + +/** + * msm_disp_snapshot_init - initialize global sde debug facilities: evtlog, regdump + * @drm_dev: drm device handle + * + * Returns: 0 or -ERROR + */ +int msm_disp_snapshot_init(struct drm_device *drm_dev); + +/** + * msm_disp_snapshot_destroy - destroy the global sde debug facilities + * @drm_dev: drm device handle + * + * Returns: none + */ +void msm_disp_snapshot_destroy(struct drm_device *drm_dev); + +/** + * msm_disp_snapshot_state - trigger to dump the display snapshot + * @drm_dev: handle to drm device + + * Returns: none + */ +void msm_disp_snapshot_state(struct drm_device *drm_dev); + +/** + * msm_disp_state_get - get the handle to msm_disp_state struct from the drm device + * @drm: handle to drm device + + * Returns: handle to the msm_disp_state struct + */ +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm); + +/** + * msm_disp_state_print - print out the current dpu state + * @disp_state: handle to drm device + * @p: handle to drm printer + * @reg_dump_method: tells whether the state has to be dumped in log or coredump + * + * Returns: none + */ +void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p, + u8 reg_dump_method); + +/** + * msm_disp_snapshot_capture_state - utility to capture atomic state and hw registers + * @disp_state: handle to msm_disp_state struct + + * Returns: none + */ +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 + + * Returns: none + */ +void msm_disp_state_free(struct msm_disp_state *disp_state); + +/** + * msm_disp_snapshot_add_block - add a hardware block with its register dump + * @disp_state: handle to struct msm_disp_state + * @name: name of the hardware block + * @len: size of the register space of the hardware block + * @base_addr: starting address of the register space of the hardware block + * + * Returns: none + */ +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len, + void __iomem *base_addr); + +#endif /* MSM_DISP_SNAPSHOT_H_ */ diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c new file mode 100644 index 0000000..383f225 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. + */ + +#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__ + +#include "msm_disp_snapshot.h" + +static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr, + u32 dump_op, struct drm_printer *p) +{ + u32 len_aligned, len_padded; + u32 x0, x4, x8, xc; + void __iomem *addr; + u32 *dump_addr = NULL; + void __iomem *end_addr; + int i; + bool in_drm_printer = false; + + len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN; + len_padded = len_aligned * REG_DUMP_ALIGN; + + addr = base_addr; + end_addr = base_addr + len; + + if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) || + (dump_op == MSM_DISP_SNAPSHOT_IN_LOG)) + in_drm_printer = true; + + if (in_drm_printer && !p) { + DRM_ERROR("invalid drm printer\n"); + return; + } + + if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg)) + *reg = kzalloc(len_padded, GFP_KERNEL); + + if (*reg) + dump_addr = *reg; + + for (i = 0; i < len_aligned; i++) { + if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) { + x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0; + x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0; + x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0; + xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0; + } + + if (dump_addr) { + if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) { + dump_addr[i * 4] = x0; + dump_addr[i * 4 + 1] = x4; + dump_addr[i * 4 + 2] = x8; + dump_addr[i * 4 + 3] = xc; + } else if (in_drm_printer) { + drm_printf(p, "0x%lx : %08x %08x %08x %08x\n", + (unsigned long)(addr - base_addr), + dump_addr[i * 4], dump_addr[i * 4 + 1], + dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]); + } + } + + addr += REG_DUMP_ALIGN; + } +} + +struct msm_disp_state *msm_disp_state_get(struct drm_device *drm) +{ + struct msm_drm_private *priv; + struct dpu_kms *dpu_kms; + + priv = drm->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + + return dpu_kms->disp_state; +} + +void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p, + u8 reg_dump_method) +{ + struct msm_disp_state_block *block, *tmp; + + if (!p) { + DRM_ERROR("invalid drm printer\n"); + return; + } + + drm_printf(p, "---\n"); + + drm_printf(p, "module: " KBUILD_MODNAME "\n"); + drm_printf(p, "dpu devcoredump\n"); + drm_printf(p, "timestamp %lld\n", ktime_to_ns(state->timestamp)); + + list_for_each_entry_safe(block, tmp, &state->blocks, node) { + drm_printf(p, "====================%s================\n", block->name); + msm_disp_state_dump_regs(&block->state, block->size, block->base_addr, + reg_dump_method, p); + } + + drm_printf(p, "===================dpu drm state================\n"); + + if (state->atomic_state) + drm_atomic_print_new_state(state->atomic_state, p); +} + +static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state) +{ + struct drm_device *ddev; + struct drm_modeset_acquire_ctx ctx; + + disp_state->timestamp = ktime_get(); + + ddev = disp_state->drm_dev; + + drm_modeset_acquire_init(&ctx, 0); + + while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0) + drm_modeset_backoff(&ctx); + + disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev, + &ctx); + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); +} + +void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state) +{ + struct msm_drm_private *priv; + int i; + struct drm_device *drm_dev; + + drm_dev = disp_state->drm_dev; + priv = drm_dev->dev_private; + + msm_disp_capture_atomic_state(disp_state); + + if (priv->dp) + msm_dp_snapshot(priv->dp); + + for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { + if (!priv->dsi[i]) + continue; + + msm_dsi_snapshot(priv->dsi[i]); + } + + dpu_kms_mdp_snapshot(drm_dev); +} + +void msm_disp_state_free(struct msm_disp_state *disp_state) +{ + struct msm_disp_state_block *block, *tmp; + + if (disp_state->atomic_state) { + drm_atomic_state_put(disp_state->atomic_state); + disp_state->atomic_state = NULL; + } + + list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) { + kfree(block->state); + list_del(&block->node); + kfree(block); + } +} + +void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len, + void __iomem *base_addr) +{ + struct msm_disp_state_block *new_blk; + + new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL); + + strscpy(new_blk->name, name, sizeof(new_blk->name)); + INIT_LIST_HEAD(&new_blk->node); + new_blk->size = len; + new_blk->base_addr = base_addr; + msm_disp_state_dump_regs(&new_blk->state, len, base_addr, + MSM_DISP_SNAPSHOT_IN_MEM, NULL); + list_add(&new_blk->node, &disp_state->blocks); +} diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b1a9b1b..ce8d19a 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -62,6 +62,18 @@ struct dp_catalog_private { u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX]; };
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + char name[SZ_128]; + + snprintf(name, SZ_128, "dp_ctrl"); + + msm_disp_snapshot_add_block(disp_state, name, catalog->io->dp_controller.len, + catalog->io->dp_controller.base); +} + static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset) { offset += MSM_DP_CONTROLLER_AUX_OFFSET; diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 176a902..e7e8b13 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -9,6 +9,7 @@ #include <drm/drm_modes.h>
#include "dp_parser.h" +#include "disp/msm_disp_snapshot.h"
/* interrupts */ #define DP_INTR_HPD BIT(0) @@ -71,6 +72,9 @@ struct dp_catalog { u32 audio_data; };
+/* Debug module */ +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state); + /* AUX APIs */ u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog); int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 5a39da6..6670558 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp) dp_display->link->test_video.test_bit_depth); }
+void msm_dp_snapshot(struct msm_dp *dp) +{ + struct dp_display_private *dp_display; + struct drm_device *drm; + struct msm_disp_state *disp_state; + + dp_display = container_of(dp, struct dp_display_private, dp_display); + drm = dp->drm_dev; + disp_state = msm_disp_state_get(drm); + + /* + * if we are reading registers we need the link clocks to be on + * however till DP cable is connected this will not happen as we + * do not know the resolution to power up with. Hence check the + * power_on status before dumping DP registers to avoid crash due + * to unclocked access + */ + mutex_lock(&dp_display->event_mutex); + + if (!dp->power_on) { + mutex_unlock(&dp_display->event_mutex); + return; + } + + dp_catalog_snapshot(dp_display->catalog, disp_state); + + mutex_unlock(&dp_display->event_mutex); +} + static void dp_display_config_hpd(struct dp_display_private *dp) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 6092ba1..4d39373 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -8,6 +8,7 @@
#include "dp_panel.h" #include <sound/hdmi-codec.h> +#include "disp/msm_disp_snapshot.h"
struct msm_dp { struct drm_device *drm_dev; diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 62704885..bccc006 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, return ret; }
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi) +{ + msm_dsi_host_snapshot(msm_dsi->host); +} + diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 7abfeab..bb39403 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -15,6 +15,7 @@ #include <drm/drm_panel.h>
#include "msm_drv.h" +#include "disp/msm_disp_snapshot.h"
#define DSI_0 0 #define DSI_1 1 @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi) return msm_dsi->panel || msm_dsi->external_bridge; }
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi); + struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
/* dsi host */ @@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host); int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host); int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi); int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi); +void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
/* dsi phy */ struct msm_dsi_phy; diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 8a10e43..d9fdc07 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host) +{ + struct msm_dsi_host *msm_host = to_msm_dsi_host(host); + struct drm_device *dev = msm_host->dev; + struct msm_disp_state *disp_state; + char name[SZ_128]; + + disp_state = msm_disp_state_get(dev); + + pm_runtime_get_sync(&msm_host->pdev->dev); + + snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id); + + msm_disp_snapshot_add_block(disp_state, name, msm_iomap_size(msm_host->pdev, "dsi_ctrl"), + msm_host->ctrl_base); + + pm_runtime_put_sync(&msm_host->pdev->dev); +} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index e1104d2..ebf9283 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved. + * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved. * Copyright (C) 2013 Red Hat * Author: Rob Clark robdclark@gmail.com */ @@ -19,6 +19,7 @@ #include <drm/drm_of.h> #include <drm/drm_vblank.h>
+#include "disp/msm_disp_snapshot.h" #include "msm_drv.h" #include "msm_debugfs.h" #include "msm_fence.h" @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, return _msm_ioremap(pdev, name, dbgname, true); }
+unsigned long msm_iomap_size(struct platform_device *pdev, const char *name) +{ + struct resource *res; + + if (name) + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); + else + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + if (!res) { + dev_dbg(&pdev->dev, "failed to get memory resource: %s\n", + name); + return 0; + } + + return resource_size(res); +} + void msm_writel(u32 data, void __iomem *addr) { if (reglog) @@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev) msm_fbdev_free(ddev); #endif
+ msm_disp_snapshot_destroy(ddev); + drm_mode_config_cleanup(ddev);
pm_runtime_get_sync(dev); @@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) if (ret) goto err_msm_uninit;
+ if (get_mdp_ver(pdev) == KMS_DPU) { + ret = msm_disp_snapshot_init(ddev); + if (ret) + DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret); + } + drm_mode_config_reset(ddev);
#ifdef CONFIG_DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 2668941..9c40bac 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); void msm_dp_irq_postinstall(struct msm_dp *dp_display); +void msm_dp_snapshot(struct msm_dp *dp_display);
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
@@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name, const char *dbgname); void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, const char *dbgname); +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name); void msm_writel(u32 data, void __iomem *addr); u32 msm_readl(const void __iomem *addr); void msm_rmw(void __iomem *addr, u32 mask, u32 or);
On 15/04/2021 02:11, Abhinav Kumar wrote:
Add the msm_disp_snapshot module which adds supports to dump dpu registers and capture the drm atomic state which can be used in case of error conditions.
changes in v4:
- rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
- start using a list of blocks to store the hardware block information
- cleanup block allocation and freeing logic to simplify it
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_hw_catalog.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 ++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 5 + drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 161 +++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot.h | 167 ++++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 12 ++ drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_display.c | 29 ++++ 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 | 19 +++ drivers/gpu/drm/msm/msm_drv.c | 29 +++- drivers/gpu/drm/msm/msm_drv.h | 2 +
Generic suggestion: Could you please split this patch, separating core snapshotting functionality, dpu1, dsi and dp changes? This would ease reviewing.
16 files changed, 682 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 610d630..65d86ce 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -77,6 +77,8 @@ msm-y := \ disp/dpu1/dpu_plane.o \ disp/dpu1/dpu_rm.o \ disp/dpu1/dpu_vbif.o \
- disp/msm_disp_snapshot.o \
- disp/msm_disp_snapshot_util.o \
If snapshotting is going to be used by dsi/dp, what about landing it into the top drivers/gpu/drm/msm dir? This way we'd point that it is a generic piece of code shared between all drm/msm drivers.
msm_atomic.o \ msm_atomic_tracepoints.o \ msm_debugfs.o \ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index 4dfd8a2..0f9f0a5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved. +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved. */
#ifndef _DPU_HW_CATALOG_H diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 88e9cc3..7529566 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms) dpu_core_irq_uninstall(dpu_kms); }
+void dpu_kms_mdp_snapshot(struct drm_device *dev) +{
- int i;
- struct msm_drm_private *priv;
- struct dpu_kms *dpu_kms;
- struct dpu_mdss_cfg *cat;
- struct dpu_hw_mdp *top;
- struct msm_disp_state *disp_state;
- char name[SZ_128];
- priv = dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- disp_state = dpu_kms->disp_state;
- cat = dpu_kms->catalog;
- top = dpu_kms->hw_mdp;
- pm_runtime_get_sync(&dpu_kms->pdev->dev);
- /* dump CTL sub-blocks HW regs info */
- for (i = 0; i < cat->ctl_count; i++) {
snprintf(name, SZ_128, "ctl_%d", i);
snprintf(name, sizeof(name), "ctl_%d", i); (the same bellow)
msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,dpu_kms->mmio + cat->ctl[i].base);- }
- /* dump DSPP sub-blocks HW regs info */
- for (i = 0; i < cat->dspp_count; i++) {
snprintf(name, SZ_128, "dspp_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,dpu_kms->mmio + cat->dspp[i].base);- }
- /* dump INTF sub-blocks HW regs info */
- for (i = 0; i < cat->intf_count; i++) {
snprintf(name, SZ_128, "intf_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,dpu_kms->mmio + cat->intf[i].base);- }
- /* dump PP sub-blocks HW regs info */
- for (i = 0; i < cat->pingpong_count; i++) {
snprintf(name, SZ_128, "ping-pong_%d", i);
Just a nitpick: we usually call it pp or pingpong, without dash.
msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,dpu_kms->mmio + cat->pingpong[i].base);- }
- /* dump SSPP sub-blocks HW regs info */
- for (i = 0; i < cat->sspp_count; i++) {
snprintf(name, SZ_128, "sspp_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,dpu_kms->mmio + cat->sspp[i].base);- }
- snprintf(name, SZ_128, "top");
- msm_disp_snapshot_add_block(disp_state, name, top->hw.length,
dpu_kms->mmio + top->hw.blk_off);- pm_runtime_put_sync(&dpu_kms->pdev->dev);
+}
- static const struct msm_kms_funcs kms_funcs = { .hw_init = dpu_kms_hw_init, .irq_preinstall = dpu_irq_preinstall,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index d6717d6..74b6ce6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -13,6 +13,7 @@ #include <drm/drm_drv.h>
#include "msm_drv.h" +#include "disp/msm_disp_snapshot.h" #include "msm_kms.h" #include "msm_mmu.h" #include "msm_gem.h" @@ -132,6 +133,8 @@ struct dpu_kms {
struct opp_table *opp_table;
struct msm_disp_state *disp_state;
struct dss_module_power mp;
/* reference count bandwidth requests, so we know when we can
@@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder); */ u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
+void dpu_kms_mdp_snapshot(struct drm_device *dev);
- #endif /* __dpu_kms_H__ */
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c new file mode 100644 index 0000000..1bdad16 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
- */
+#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
+#include "msm_disp_snapshot.h"
+#ifdef CONFIG_DEV_COREDUMP +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
size_t count, void *data, size_t datalen)+{
- struct drm_print_iterator iter;
- struct drm_printer p;
- struct msm_disp_state *disp_state;
- disp_state = data;
- iter.data = buffer;
- iter.offset = 0;
- iter.start = offset;
- iter.remain = count;
- p = drm_coredump_printer(&iter);
- msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);
- return count - iter.remain;
+}
+static void disp_devcoredump_free(void *data) +{
- struct msm_disp_state *disp_state;
- disp_state = data;
- msm_disp_state_free(disp_state);
- disp_state->coredump_pending = false;
+} +#endif /* CONFIG_DEV_COREDUMP */
+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;
- mutex_lock(&disp_state->mutex);
- msm_disp_snapshot_capture_state(disp_state);
- if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
p = drm_info_printer(disp_state->drm_dev->dev);msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);- }
- /*
* if devcoredump is not defined free the state immediately* otherwise it will be freed in the free handler.*/+#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);
+}
+void msm_disp_snapshot_state(struct drm_device *drm_dev) +{
- struct msm_drm_private *priv;
- struct dpu_kms *dpu_kms;
- struct msm_disp_state *disp_state;
- if (!drm_dev) {
DRM_ERROR("invalid params\n");return;- }
- priv = drm_dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
If we are outside of disp/dpu1, we shouldn't reference dpu_kms internals. Probably this can be changed to use msm_drm_private, msm_kms or msm_mdss?
- disp_state = dpu_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
Where do we handle the timeout? Stale comment or did I miss something?
*/- if (disp_state->coredump_pending) {
DRM_DEBUG("coredump is pending read\n");return;- }
- kthread_queue_work(disp_state->dump_worker,
&disp_state->dump_work);+}
+int msm_disp_snapshot_init(struct drm_device *drm_dev) +{
- struct dpu_kms *dpu_kms;
- struct msm_drm_private *priv;
- struct msm_disp_state *disp_state;
- if (!drm_dev) {
DRM_ERROR("invalid params\n");return -EINVAL;- }
- priv = drm_dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);
- mutex_init(&disp_state->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))
DRM_ERROR("failed to create disp state task\n");- kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
- dpu_kms->disp_state = disp_state;
Judging from this code, I think it should be possible to push disp_state directly into struct msm_drm_private.
- return 0;
+}
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev) +{
- struct dpu_kms *dpu_kms;
- struct msm_drm_private *priv;
- struct msm_disp_state *disp_state;
- if (!drm_dev) {
DRM_ERROR("invalid params\n");return;- }
- priv = drm_dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- disp_state = dpu_kms->disp_state;
- if (disp_state->dump_worker)
kthread_destroy_worker(disp_state->dump_worker);- list_del(&disp_state->blocks);
- mutex_destroy(&disp_state->mutex);
+} diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h new file mode 100644 index 0000000..0ccb7e7 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h @@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
- */
+#ifndef MSM_DISP_SNAPSHOT_H_ +#define MSM_DISP_SNAPSHOT_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"
+enum msm_disp_snapshot_dump_flag {
- MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),
- MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),
- MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),
Are they mutually exclusive or can they be specified in parallel?
+};
+#define MSM_DISP_SNAPSHOT_MAX_BLKS 10
+/* debug option to print the registers in logs */ +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
+/* print debug ranges in groups of 4 u32s */ +#define REG_DUMP_ALIGN 16
+/**
- 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;
+};
+/**
- struct msm_disp_state_block - structure to store each hardware block state
- @name: name of the block
- @drm_dev: handle to the linked list head
- @size: size of the register space of this hardware block
- @state: array holding the register dump of this hardware block
- @base_addr: starting address of this hardware block's register space
- */
+struct msm_disp_state_block {
- char name[SZ_128];
- struct list_head node;
- unsigned int size;
- u32 *state;
- void __iomem *base_addr;
+};
+/**
- MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
- @drm_dev: handle to drm device
- */
+#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)
Why do you need it?
+/**
- msm_disp_snapshot_init - initialize global sde debug facilities: evtlog, regdump
Just a nitpick here and below: there is no sde in the upstream. And no evtlog.
- @drm_dev: drm device handle
- Returns: 0 or -ERROR
- */
+int msm_disp_snapshot_init(struct drm_device *drm_dev);
+/**
- msm_disp_snapshot_destroy - destroy the global sde debug facilities
- @drm_dev: drm device handle
- Returns: none
- */
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
+/**
- msm_disp_snapshot_state - trigger to dump the display snapshot
- @drm_dev: handle to drm device
- Returns: none
- */
+void msm_disp_snapshot_state(struct drm_device *drm_dev);
+/**
- msm_disp_state_get - get the handle to msm_disp_state struct from the drm device
- @drm: handle to drm device
- Returns: handle to the msm_disp_state struct
- */
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
+/**
- msm_disp_state_print - print out the current dpu state
- @disp_state: handle to drm device
- @p: handle to drm printer
- @reg_dump_method: tells whether the state has to be dumped in log or coredump
- Returns: none
- */
+void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p,
u8 reg_dump_method);+/**
- msm_disp_snapshot_capture_state - utility to capture atomic state and hw registers
- @disp_state: handle to msm_disp_state struct
- Returns: none
- */
+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
- Returns: none
- */
+void msm_disp_state_free(struct msm_disp_state *disp_state);
+/**
- msm_disp_snapshot_add_block - add a hardware block with its register dump
- @disp_state: handle to struct msm_disp_state
- @name: name of the hardware block
- @len: size of the register space of the hardware block
- @base_addr: starting address of the register space of the hardware block
- Returns: none
- */
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
void __iomem *base_addr);+#endif /* MSM_DISP_SNAPSHOT_H_ */ diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c new file mode 100644 index 0000000..383f225 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
- */
+#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
+#include "msm_disp_snapshot.h"
+static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr,
u32 dump_op, struct drm_printer *p)+{
- u32 len_aligned, len_padded;
- u32 x0, x4, x8, xc;
- void __iomem *addr;
- u32 *dump_addr = NULL;
- void __iomem *end_addr;
- int i;
- bool in_drm_printer = false;
- len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;
- len_padded = len_aligned * REG_DUMP_ALIGN;
- addr = base_addr;
- end_addr = base_addr + len;
- if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||
(dump_op == MSM_DISP_SNAPSHOT_IN_LOG))in_drm_printer = true;- if (in_drm_printer && !p) {
DRM_ERROR("invalid drm printer\n");return;- }
- if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))
*reg = kzalloc(len_padded, GFP_KERNEL);- if (*reg)
dump_addr = *reg;- for (i = 0; i < len_aligned; i++) {
if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;}if (dump_addr) {if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {dump_addr[i * 4] = x0;dump_addr[i * 4 + 1] = x4;dump_addr[i * 4 + 2] = x8;dump_addr[i * 4 + 3] = xc;} else if (in_drm_printer) {drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",(unsigned long)(addr - base_addr),dump_addr[i * 4], dump_addr[i * 4 + 1],dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);}}addr += REG_DUMP_ALIGN;- }
+}
I'd just split this function into two different cases, IN_MEM and DRM_PRINTER.
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm) +{
- struct msm_drm_private *priv;
- struct dpu_kms *dpu_kms;
- priv = drm->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- return dpu_kms->disp_state;
+}
+void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p,
u8 reg_dump_method)+{
- struct msm_disp_state_block *block, *tmp;
- if (!p) {
DRM_ERROR("invalid drm printer\n");return;- }
- drm_printf(p, "---\n");
- drm_printf(p, "module: " KBUILD_MODNAME "\n");
- drm_printf(p, "dpu devcoredump\n");
- drm_printf(p, "timestamp %lld\n", ktime_to_ns(state->timestamp));
- list_for_each_entry_safe(block, tmp, &state->blocks, node) {
drm_printf(p, "====================%s================\n", block->name);msm_disp_state_dump_regs(&block->state, block->size, block->base_addr,reg_dump_method, p);- }
- drm_printf(p, "===================dpu drm state================\n");
- if (state->atomic_state)
drm_atomic_print_new_state(state->atomic_state, p);+}
+static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state) +{
- struct drm_device *ddev;
- struct drm_modeset_acquire_ctx ctx;
- disp_state->timestamp = ktime_get();
- ddev = disp_state->drm_dev;
- drm_modeset_acquire_init(&ctx, 0);
- while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
drm_modeset_backoff(&ctx);- disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
&ctx);- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
+}
+void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state) +{
- struct msm_drm_private *priv;
- int i;
- struct drm_device *drm_dev;
- drm_dev = disp_state->drm_dev;
- priv = drm_dev->dev_private;
- msm_disp_capture_atomic_state(disp_state);
- if (priv->dp)
msm_dp_snapshot(priv->dp);- for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
if (!priv->dsi[i])continue;msm_dsi_snapshot(priv->dsi[i]);- }
- dpu_kms_mdp_snapshot(drm_dev);
Hmm, I now see why do you need the dpu_kms. Can we have a check for the mdp version here? The rest of the code seems generic enough, so we can at least dump the dsi/dp (and hopefully eDP/HDMI) data for MDP4/5.
Ideally subdrivers should be able to report back to the core, which regions are available, so that we won't have this static code. But this might look like an overkill. Not to mention that it might lead to the not-so-static sequence of dumps. (As you can judge, I quite liked the previous approach, when the drivers registered the dumping regions and then the dumper would go through them.)
Let's have the static config in place with the version check.
+}
+void msm_disp_state_free(struct msm_disp_state *disp_state) +{
- struct msm_disp_state_block *block, *tmp;
- if (disp_state->atomic_state) {
drm_atomic_state_put(disp_state->atomic_state);disp_state->atomic_state = NULL;- }
- list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
kfree(block->state);list_del(&block->node);
I'd swap these two lines, so while the block is in the list, its state is coherent.
kfree(block);- }
+}
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
void __iomem *base_addr)+{
- struct msm_disp_state_block *new_blk;
- new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
- strscpy(new_blk->name, name, sizeof(new_blk->name));
- INIT_LIST_HEAD(&new_blk->node);
- new_blk->size = len;
- new_blk->base_addr = base_addr;
- msm_disp_state_dump_regs(&new_blk->state, len, base_addr,
MSM_DISP_SNAPSHOT_IN_MEM, NULL);- list_add(&new_blk->node, &disp_state->blocks);
+} diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b1a9b1b..ce8d19a 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -62,6 +62,18 @@ struct dp_catalog_private { u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX]; };
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state) +{
- struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);- char name[SZ_128];
- snprintf(name, SZ_128, "dp_ctrl");
- msm_disp_snapshot_add_block(disp_state, name, catalog->io->dp_controller.len,
catalog->io->dp_controller.base);+}
- static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset) { offset += MSM_DP_CONTROLLER_AUX_OFFSET;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 176a902..e7e8b13 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -9,6 +9,7 @@ #include <drm/drm_modes.h>
#include "dp_parser.h" +#include "disp/msm_disp_snapshot.h"
/* interrupts */ #define DP_INTR_HPD BIT(0) @@ -71,6 +72,9 @@ struct dp_catalog { u32 audio_data; };
+/* Debug module */ +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
- /* AUX APIs */ u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog); int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 5a39da6..6670558 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp) dp_display->link->test_video.test_bit_depth); }
+void msm_dp_snapshot(struct msm_dp *dp) +{
- struct dp_display_private *dp_display;
- struct drm_device *drm;
- struct msm_disp_state *disp_state;
- dp_display = container_of(dp, struct dp_display_private, dp_display);
- drm = dp->drm_dev;
- disp_state = msm_disp_state_get(drm);
- /*
* if we are reading registers we need the link clocks to be on* however till DP cable is connected this will not happen as we* do not know the resolution to power up with. Hence check the* power_on status before dumping DP registers to avoid crash due* to unclocked access*/- mutex_lock(&dp_display->event_mutex);
- if (!dp->power_on) {
mutex_unlock(&dp_display->event_mutex);return;- }
- dp_catalog_snapshot(dp_display->catalog, disp_state);
- mutex_unlock(&dp_display->event_mutex);
+}
- static void dp_display_config_hpd(struct dp_display_private *dp) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 6092ba1..4d39373 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -8,6 +8,7 @@
#include "dp_panel.h" #include <sound/hdmi-codec.h> +#include "disp/msm_disp_snapshot.h"
struct msm_dp { struct drm_device *drm_dev; diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 62704885..bccc006 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, return ret; }
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi) +{
- msm_dsi_host_snapshot(msm_dsi->host);
+}
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 7abfeab..bb39403 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -15,6 +15,7 @@ #include <drm/drm_panel.h>
#include "msm_drv.h" +#include "disp/msm_disp_snapshot.h"
#define DSI_0 0 #define DSI_1 1 @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi) return msm_dsi->panel || msm_dsi->external_bridge; }
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
/* dsi host */
@@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host); int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host); int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi); int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi); +void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
/* dsi phy */ struct msm_dsi_phy; diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 8a10e43..d9fdc07 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host) +{
- struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
- struct drm_device *dev = msm_host->dev;
- struct msm_disp_state *disp_state;
- char name[SZ_128];
- disp_state = msm_disp_state_get(dev);
- pm_runtime_get_sync(&msm_host->pdev->dev);
- snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);
- msm_disp_snapshot_add_block(disp_state, name, msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
You can store the size into the msm_host, rather than querrying it on each dump.
msm_host->ctrl_base);- pm_runtime_put_sync(&msm_host->pdev->dev);
+} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index e1104d2..ebf9283 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /*
- Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
*/
- Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved.
- Copyright (C) 2013 Red Hat
- Author: Rob Clark robdclark@gmail.com
@@ -19,6 +19,7 @@ #include <drm/drm_of.h> #include <drm/drm_vblank.h>
+#include "disp/msm_disp_snapshot.h" #include "msm_drv.h" #include "msm_debugfs.h" #include "msm_fence.h" @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, return _msm_ioremap(pdev, name, dbgname, true); }
+unsigned long msm_iomap_size(struct platform_device *pdev, const char *name) +{
- struct resource *res;
- if (name)
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);- else
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);- if (!res) {
dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",name);return 0;- }
- return resource_size(res);
+}
- void msm_writel(u32 data, void __iomem *addr) { if (reglog)
@@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev) msm_fbdev_free(ddev); #endif
msm_disp_snapshot_destroy(ddev);
drm_mode_config_cleanup(ddev);
pm_runtime_get_sync(dev);
@@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) if (ret) goto err_msm_uninit;
- if (get_mdp_ver(pdev) == KMS_DPU) {
ret = msm_disp_snapshot_init(ddev);if (ret)DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);- }
Let's get it out of if (KMS_DPU), even if we do not dump mdp4/mdp5 registers for now.
drm_mode_config_reset(ddev);
#ifdef CONFIG_DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 2668941..9c40bac 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); void msm_dp_irq_postinstall(struct msm_dp *dp_display); +void msm_dp_snapshot(struct msm_dp *dp_display);
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
@@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name, const char *dbgname); void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, const char *dbgname); +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name); void msm_writel(u32 data, void __iomem *addr); u32 msm_readl(const void __iomem *addr); void msm_rmw(void __iomem *addr, u32 mask, u32 or);
Hi Dmitry
Thanks for the review.
On 2021-04-14 17:16, Dmitry Baryshkov wrote:
On 15/04/2021 02:11, Abhinav Kumar wrote:
Add the msm_disp_snapshot module which adds supports to dump dpu registers and capture the drm atomic state which can be used in case of error conditions.
changes in v4:
- rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
- start using a list of blocks to store the hardware block
information
- cleanup block allocation and freeing logic to simplify it
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_hw_catalog.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 ++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 5 + drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 161 +++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot.h | 167 ++++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 12 ++ drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_display.c | 29 ++++ 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 | 19 +++ drivers/gpu/drm/msm/msm_drv.c | 29 +++- drivers/gpu/drm/msm/msm_drv.h | 2 +
Generic suggestion: Could you please split this patch, separating core snapshotting functionality, dpu1, dsi and dp changes? This would ease reviewing.
Core snapshotting functionality does include calling into the individual driver's snapshot function. So the diffstat which you are seeing in the dpu1/dsi and dp are just their snapshotting functions and nothing else.
16 files changed, 682 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 610d630..65d86ce 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -77,6 +77,8 @@ msm-y := \ disp/dpu1/dpu_plane.o \ disp/dpu1/dpu_rm.o \ disp/dpu1/dpu_vbif.o \
- disp/msm_disp_snapshot.o \
- disp/msm_disp_snapshot_util.o \
If snapshotting is going to be used by dsi/dp, what about landing it into the top drivers/gpu/drm/msm dir? This way we'd point that it is a generic piece of code shared between all drm/msm drivers.
I dont have a strong preference of drm/msm Vs drm/msm/disp. I just put them here based on Rob's comment on the prev patchset https://patchwork.freedesktop.org/patch/427690/?series=82942&rev=3
msm_atomic.o \ msm_atomic_tracepoints.o \ msm_debugfs.o \ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index 4dfd8a2..0f9f0a5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved. +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved. */ #ifndef _DPU_HW_CATALOG_H diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 88e9cc3..7529566 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms) dpu_core_irq_uninstall(dpu_kms); } +void dpu_kms_mdp_snapshot(struct drm_device *dev) +{
- int i;
- struct msm_drm_private *priv;
- struct dpu_kms *dpu_kms;
- struct dpu_mdss_cfg *cat;
- struct dpu_hw_mdp *top;
- struct msm_disp_state *disp_state;
- char name[SZ_128];
- priv = dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- disp_state = dpu_kms->disp_state;
- cat = dpu_kms->catalog;
- top = dpu_kms->hw_mdp;
- pm_runtime_get_sync(&dpu_kms->pdev->dev);
- /* dump CTL sub-blocks HW regs info */
- for (i = 0; i < cat->ctl_count; i++) {
snprintf(name, SZ_128, "ctl_%d", i);snprintf(name, sizeof(name), "ctl_%d", i); (the same bellow)
Rob just mentioned to me on IRC to move this snprintf inside msm_disp_snapshot_add_block(). I am planning to do that in the next patchset. With that will fix this too.
msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,dpu_kms->mmio + cat->ctl[i].base);- }
- /* dump DSPP sub-blocks HW regs info */
- for (i = 0; i < cat->dspp_count; i++) {
snprintf(name, SZ_128, "dspp_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,dpu_kms->mmio + cat->dspp[i].base);- }
- /* dump INTF sub-blocks HW regs info */
- for (i = 0; i < cat->intf_count; i++) {
snprintf(name, SZ_128, "intf_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,dpu_kms->mmio + cat->intf[i].base);- }
- /* dump PP sub-blocks HW regs info */
- for (i = 0; i < cat->pingpong_count; i++) {
snprintf(name, SZ_128, "ping-pong_%d", i);Just a nitpick: we usually call it pp or pingpong, without dash.
Sure I will fix this.
msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,dpu_kms->mmio + cat->pingpong[i].base);- }
- /* dump SSPP sub-blocks HW regs info */
- for (i = 0; i < cat->sspp_count; i++) {
snprintf(name, SZ_128, "sspp_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,dpu_kms->mmio + cat->sspp[i].base);- }
- snprintf(name, SZ_128, "top");
- msm_disp_snapshot_add_block(disp_state, name, top->hw.length,
dpu_kms->mmio + top->hw.blk_off);- pm_runtime_put_sync(&dpu_kms->pdev->dev);
+}
- static const struct msm_kms_funcs kms_funcs = { .hw_init = dpu_kms_hw_init, .irq_preinstall = dpu_irq_preinstall,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index d6717d6..74b6ce6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -13,6 +13,7 @@ #include <drm/drm_drv.h> #include "msm_drv.h" +#include "disp/msm_disp_snapshot.h" #include "msm_kms.h" #include "msm_mmu.h" #include "msm_gem.h" @@ -132,6 +133,8 @@ struct dpu_kms { struct opp_table *opp_table;
- struct msm_disp_state *disp_state;
- struct dss_module_power mp; /* reference count bandwidth requests, so we know when we can
@@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder); */ u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name); +void dpu_kms_mdp_snapshot(struct drm_device *dev);
- #endif /* __dpu_kms_H__ */
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c new file mode 100644 index 0000000..1bdad16 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights
reserved.
- */
+#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
+#include "msm_disp_snapshot.h"
+#ifdef CONFIG_DEV_COREDUMP +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
size_t count, void *data, size_t datalen)+{
- struct drm_print_iterator iter;
- struct drm_printer p;
- struct msm_disp_state *disp_state;
- disp_state = data;
- iter.data = buffer;
- iter.offset = 0;
- iter.start = offset;
- iter.remain = count;
- p = drm_coredump_printer(&iter);
- msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);
- return count - iter.remain;
+}
+static void disp_devcoredump_free(void *data) +{
- struct msm_disp_state *disp_state;
- disp_state = data;
- msm_disp_state_free(disp_state);
- disp_state->coredump_pending = false;
+} +#endif /* CONFIG_DEV_COREDUMP */
+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;
- mutex_lock(&disp_state->mutex);
- msm_disp_snapshot_capture_state(disp_state);
- if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
p = drm_info_printer(disp_state->drm_dev->dev);msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);- }
- /*
* if devcoredump is not defined free the state immediately* otherwise it will be freed in the free handler.*/+#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);
+}
+void msm_disp_snapshot_state(struct drm_device *drm_dev) +{
- struct msm_drm_private *priv;
- struct dpu_kms *dpu_kms;
- struct msm_disp_state *disp_state;
- if (!drm_dev) {
DRM_ERROR("invalid params\n");return;- }
- priv = drm_dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
If we are outside of disp/dpu1, we shouldn't reference dpu_kms internals. Probably this can be changed to use msm_drm_private, msm_kms or msm_mdss?
This was originally within dpu1 but based on Rob's comment I moved it to disp. Since this is a debug module I thought its okay to reference it here. Its not modifying anything which will impact the current dpu state. If there is a strong preference for this, sure I can move it to msm_drm_private.
- disp_state = dpu_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 happensWhere do we handle the timeout? Stale comment or did I miss something?
The timeout happens in the devcoredump driver and it calls the free() from there. https://github.com/torvalds/linux/blob/master/drivers/base/devcoredump.c#L30...
*/- if (disp_state->coredump_pending) {
DRM_DEBUG("coredump is pending read\n");return;- }
- kthread_queue_work(disp_state->dump_worker,
&disp_state->dump_work);+}
+int msm_disp_snapshot_init(struct drm_device *drm_dev) +{
- struct dpu_kms *dpu_kms;
- struct msm_drm_private *priv;
- struct msm_disp_state *disp_state;
- if (!drm_dev) {
DRM_ERROR("invalid params\n");return -EINVAL;- }
- priv = drm_dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct
msm_disp_state), GFP_KERNEL);
- mutex_init(&disp_state->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))
DRM_ERROR("failed to create disp state task\n");- kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);
- dpu_kms->disp_state = disp_state;
Judging from this code, I think it should be possible to push disp_state directly into struct msm_drm_private.
Again no strong preference here. I just kept it in dpu_kms based on a discussion with Rob. We can move it to msm_drm_private too if there is a strong preference to do so.
- return 0;
+}
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev) +{
- struct dpu_kms *dpu_kms;
- struct msm_drm_private *priv;
- struct msm_disp_state *disp_state;
- if (!drm_dev) {
DRM_ERROR("invalid params\n");return;- }
- priv = drm_dev->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- disp_state = dpu_kms->disp_state;
- if (disp_state->dump_worker)
kthread_destroy_worker(disp_state->dump_worker);- list_del(&disp_state->blocks);
- mutex_destroy(&disp_state->mutex);
+} diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h new file mode 100644 index 0000000..0ccb7e7 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h @@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights
reserved.
- */
+#ifndef MSM_DISP_SNAPSHOT_H_ +#define MSM_DISP_SNAPSHOT_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"
+enum msm_disp_snapshot_dump_flag {
- MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),
- MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),
- MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),
Are they mutually exclusive or can they be specified in parallel?
They are mutually exclusive
+};
+#define MSM_DISP_SNAPSHOT_MAX_BLKS 10
+/* debug option to print the registers in logs */ +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
+/* print debug ranges in groups of 4 u32s */ +#define REG_DUMP_ALIGN 16
+/**
- 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;
+};
+/**
- struct msm_disp_state_block - structure to store each hardware
block state
- @name: name of the block
- @drm_dev: handle to the linked list head
- @size: size of the register space of this hardware block
- @state: array holding the register dump of this hardware block
- @base_addr: starting address of this hardware block's register
space
- */
+struct msm_disp_state_block {
- char name[SZ_128];
- struct list_head node;
- unsigned int size;
- u32 *state;
- void __iomem *base_addr;
+};
+/**
- MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
- @drm_dev: handle to drm device
- */
+#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)
Why do you need it?
Not really needed now, I can just call into msm_disp_snapshot_state() instead
+/**
- msm_disp_snapshot_init - initialize global sde debug facilities:
evtlog, regdump
Just a nitpick here and below: there is no sde in the upstream. And no evtlog.
Sorry this is a stale comment, I thought I got rid of this, will fix this.
- @drm_dev: drm device handle
- Returns: 0 or -ERROR
- */
+int msm_disp_snapshot_init(struct drm_device *drm_dev);
+/**
- msm_disp_snapshot_destroy - destroy the global sde debug
facilities
- @drm_dev: drm device handle
- Returns: none
- */
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
+/**
- msm_disp_snapshot_state - trigger to dump the display snapshot
- @drm_dev: handle to drm device
- Returns: none
- */
+void msm_disp_snapshot_state(struct drm_device *drm_dev);
+/**
- msm_disp_state_get - get the handle to msm_disp_state struct from
the drm device
- @drm: handle to drm device
- Returns: handle to the msm_disp_state struct
- */
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
+/**
- msm_disp_state_print - print out the current dpu state
- @disp_state: handle to drm device
- @p: handle to drm printer
- @reg_dump_method: tells whether the state has to be dumped in log
or coredump
- Returns: none
- */
+void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p,
u8 reg_dump_method);+/**
- msm_disp_snapshot_capture_state - utility to capture atomic state
and hw registers
- @disp_state: handle to msm_disp_state struct
- Returns: none
- */
+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
- Returns: none
- */
+void msm_disp_state_free(struct msm_disp_state *disp_state);
+/**
- msm_disp_snapshot_add_block - add a hardware block with its
register dump
- @disp_state: handle to struct msm_disp_state
- @name: name of the hardware block
- @len: size of the register space of the hardware block
- @base_addr: starting address of the register space of the
hardware block
- Returns: none
- */
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
void __iomem *base_addr);+#endif /* MSM_DISP_SNAPSHOT_H_ */ diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c new file mode 100644 index 0000000..383f225 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights
reserved.
- */
+#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
+#include "msm_disp_snapshot.h"
+static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr,
u32 dump_op, struct drm_printer *p)+{
- u32 len_aligned, len_padded;
- u32 x0, x4, x8, xc;
- void __iomem *addr;
- u32 *dump_addr = NULL;
- void __iomem *end_addr;
- int i;
- bool in_drm_printer = false;
- len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;
- len_padded = len_aligned * REG_DUMP_ALIGN;
- addr = base_addr;
- end_addr = base_addr + len;
- if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||
(dump_op == MSM_DISP_SNAPSHOT_IN_LOG))in_drm_printer = true;- if (in_drm_printer && !p) {
DRM_ERROR("invalid drm printer\n");return;- }
- if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))
*reg = kzalloc(len_padded, GFP_KERNEL);- if (*reg)
dump_addr = *reg;- for (i = 0; i < len_aligned; i++) {
if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;}if (dump_addr) {if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {dump_addr[i * 4] = x0;dump_addr[i * 4 + 1] = x4;dump_addr[i * 4 + 2] = x8;dump_addr[i * 4 + 3] = xc;} else if (in_drm_printer) {drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",(unsigned long)(addr - base_addr),dump_addr[i * 4], dump_addr[i * 4 + 1],dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);}}addr += REG_DUMP_ALIGN;- }
+}
I'd just split this function into two different cases, IN_MEM and DRM_PRINTER.
I wanted to utilize the common loop here, if it will make it more clear I can just have two functions. But it will duplicate a lot of code.
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm) +{
- struct msm_drm_private *priv;
- struct dpu_kms *dpu_kms;
- priv = drm->dev_private;
- dpu_kms = to_dpu_kms(priv->kms);
- return dpu_kms->disp_state;
+}
+void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p,
u8 reg_dump_method)+{
- struct msm_disp_state_block *block, *tmp;
- if (!p) {
DRM_ERROR("invalid drm printer\n");return;- }
- drm_printf(p, "---\n");
- drm_printf(p, "module: " KBUILD_MODNAME "\n");
- drm_printf(p, "dpu devcoredump\n");
- drm_printf(p, "timestamp %lld\n", ktime_to_ns(state->timestamp));
- list_for_each_entry_safe(block, tmp, &state->blocks, node) {
drm_printf(p, "====================%s================\n",block->name);
msm_disp_state_dump_regs(&block->state, block->size,block->base_addr,
reg_dump_method, p);- }
- drm_printf(p, "===================dpu drm state================\n");
- if (state->atomic_state)
drm_atomic_print_new_state(state->atomic_state, p);+}
+static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state) +{
- struct drm_device *ddev;
- struct drm_modeset_acquire_ctx ctx;
- disp_state->timestamp = ktime_get();
- ddev = disp_state->drm_dev;
- drm_modeset_acquire_init(&ctx, 0);
- while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)
drm_modeset_backoff(&ctx);- disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
&ctx);- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
+}
+void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state) +{
- struct msm_drm_private *priv;
- int i;
- struct drm_device *drm_dev;
- drm_dev = disp_state->drm_dev;
- priv = drm_dev->dev_private;
- msm_disp_capture_atomic_state(disp_state);
- if (priv->dp)
msm_dp_snapshot(priv->dp);- for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
if (!priv->dsi[i])continue;msm_dsi_snapshot(priv->dsi[i]);- }
- dpu_kms_mdp_snapshot(drm_dev);
Hmm, I now see why do you need the dpu_kms. Can we have a check for the mdp version here? The rest of the code seems generic enough, so we can at least dump the dsi/dp (and hopefully eDP/HDMI) data for MDP4/5.
Ideally subdrivers should be able to report back to the core, which regions are available, so that we won't have this static code. But this might look like an overkill. Not to mention that it might lead to the not-so-static sequence of dumps. (As you can judge, I quite liked the previous approach, when the drivers registered the dumping regions and then the dumper would go through them.)
Let's have the static config in place with the version check.
Yes, I can have a DPU check here.
+}
+void msm_disp_state_free(struct msm_disp_state *disp_state) +{
- struct msm_disp_state_block *block, *tmp;
- if (disp_state->atomic_state) {
drm_atomic_state_put(disp_state->atomic_state);disp_state->atomic_state = NULL;- }
- list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
kfree(block->state);list_del(&block->node);I'd swap these two lines, so while the block is in the list, its state is coherent.
Ok, will fix this.
kfree(block);- }
+}
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
void __iomem *base_addr)+{
- struct msm_disp_state_block *new_blk;
- new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);
- strscpy(new_blk->name, name, sizeof(new_blk->name));
- INIT_LIST_HEAD(&new_blk->node);
- new_blk->size = len;
- new_blk->base_addr = base_addr;
- msm_disp_state_dump_regs(&new_blk->state, len, base_addr,
MSM_DISP_SNAPSHOT_IN_MEM, NULL);- list_add(&new_blk->node, &disp_state->blocks);
+} diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b1a9b1b..ce8d19a 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -62,6 +62,18 @@ struct dp_catalog_private { u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX]; }; +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state) +{
- struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);- char name[SZ_128];
- snprintf(name, SZ_128, "dp_ctrl");
- msm_disp_snapshot_add_block(disp_state, name,
catalog->io->dp_controller.len,
catalog->io->dp_controller.base);+}
- static inline u32 dp_read_aux(struct dp_catalog_private *catalog,
u32 offset) { offset += MSM_DP_CONTROLLER_AUX_OFFSET; diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 176a902..e7e8b13 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -9,6 +9,7 @@ #include <drm/drm_modes.h> #include "dp_parser.h" +#include "disp/msm_disp_snapshot.h" /* interrupts */ #define DP_INTR_HPD BIT(0) @@ -71,6 +72,9 @@ struct dp_catalog { u32 audio_data; }; +/* Debug module */ +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
- /* AUX APIs */ u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog); int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 5a39da6..6670558 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp) dp_display->link->test_video.test_bit_depth); } +void msm_dp_snapshot(struct msm_dp *dp) +{
- struct dp_display_private *dp_display;
- struct drm_device *drm;
- struct msm_disp_state *disp_state;
- dp_display = container_of(dp, struct dp_display_private,
dp_display);
- drm = dp->drm_dev;
- disp_state = msm_disp_state_get(drm);
- /*
* if we are reading registers we need the link clocks to be on* however till DP cable is connected this will not happen as we* do not know the resolution to power up with. Hence check the* power_on status before dumping DP registers to avoid crash due* to unclocked access*/- mutex_lock(&dp_display->event_mutex);
- if (!dp->power_on) {
mutex_unlock(&dp_display->event_mutex);return;- }
- dp_catalog_snapshot(dp_display->catalog, disp_state);
- mutex_unlock(&dp_display->event_mutex);
+}
- static void dp_display_config_hpd(struct dp_display_private *dp) { diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h index 6092ba1..4d39373 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -8,6 +8,7 @@ #include "dp_panel.h" #include <sound/hdmi-codec.h> +#include "disp/msm_disp_snapshot.h" struct msm_dp { struct drm_device *drm_dev; diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 62704885..bccc006 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, return ret; } +void msm_dsi_snapshot(struct msm_dsi *msm_dsi) +{
- msm_dsi_host_snapshot(msm_dsi->host);
+}
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 7abfeab..bb39403 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -15,6 +15,7 @@ #include <drm/drm_panel.h> #include "msm_drv.h" +#include "disp/msm_disp_snapshot.h" #define DSI_0 0 #define DSI_1 1 @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi) return msm_dsi->panel || msm_dsi->external_bridge; } +void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
- struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi); /* dsi host */
@@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host); int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host); int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi); int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi); +void msm_dsi_host_snapshot(struct mipi_dsi_host *host); /* dsi phy */ struct msm_dsi_phy; diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 8a10e43..d9fdc07 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host) +{
- struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
- struct drm_device *dev = msm_host->dev;
- struct msm_disp_state *disp_state;
- char name[SZ_128];
- disp_state = msm_disp_state_get(dev);
- pm_runtime_get_sync(&msm_host->pdev->dev);
- snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);
- msm_disp_snapshot_add_block(disp_state, name,
msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
You can store the size into the msm_host, rather than querrying it on each dump.
We need to query the base address anyway so it wont lead to much saving.
msm_host->ctrl_base);- pm_runtime_put_sync(&msm_host->pdev->dev);
+} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index e1104d2..ebf9283 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /*
- Copyright (c) 2016-2018, The Linux Foundation. All rights
reserved.
- Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All
rights reserved.
- Copyright (C) 2013 Red Hat
- Author: Rob Clark robdclark@gmail.com
*/ @@ -19,6 +19,7 @@ #include <drm/drm_of.h> #include <drm/drm_vblank.h> +#include "disp/msm_disp_snapshot.h" #include "msm_drv.h" #include "msm_debugfs.h" #include "msm_fence.h" @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, return _msm_ioremap(pdev, name, dbgname, true); } +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name) +{
- struct resource *res;
- if (name)
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);- else
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);- if (!res) {
dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",name);return 0;- }
- return resource_size(res);
+}
- void msm_writel(u32 data, void __iomem *addr) { if (reglog)
@@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev) msm_fbdev_free(ddev); #endif
- msm_disp_snapshot_destroy(ddev);
- drm_mode_config_cleanup(ddev); pm_runtime_get_sync(dev);
@@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) if (ret) goto err_msm_uninit;
- if (get_mdp_ver(pdev) == KMS_DPU) {
ret = msm_disp_snapshot_init(ddev);if (ret)DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n",ret);
- }
Let's get it out of if (KMS_DPU), even if we do not dump mdp4/mdp5 registers for now.
since its stored inside dpu_kms right now, we need to have this DPU check. If we agree on moving this out of dpu_kms then this check can be relaxed. But another issue is I dont have a non-DPU_KMS board to verify this to see if it will break. So at the moment, I am restricting this to it.
drm_mode_config_reset(ddev); #ifdef CONFIG_DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 2668941..9c40bac 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); void msm_dp_irq_postinstall(struct msm_dp *dp_display); +void msm_dp_snapshot(struct msm_dp *dp_display); void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor); @@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name, const char *dbgname); void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, const char *dbgname); +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name); void msm_writel(u32 data, void __iomem *addr); u32 msm_readl(const void __iomem *addr); void msm_rmw(void __iomem *addr, u32 mask, u32 or);
Thanks
Abhinav
On Wed, Apr 14, 2021 at 5:16 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 15/04/2021 02:11, Abhinav Kumar wrote:
Add the msm_disp_snapshot module which adds supports to dump dpu registers and capture the drm atomic state which can be used in case of error conditions.
changes in v4:
- rename dpu_dbg to msm_disp_snapshot and move it to msm/disp
- start using a list of blocks to store the hardware block information
- cleanup block allocation and freeing logic to simplify it
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_hw_catalog.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 ++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 5 + drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 161 +++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot.h | 167 ++++++++++++++++++++ drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 181 ++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 12 ++ drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_display.c | 29 ++++ 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 | 19 +++ drivers/gpu/drm/msm/msm_drv.c | 29 +++- drivers/gpu/drm/msm/msm_drv.h | 2 +
Generic suggestion: Could you please split this patch, separating core snapshotting functionality, dpu1, dsi and dp changes? This would ease reviewing.
16 files changed, 682 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h create mode 100644 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 610d630..65d86ce 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -77,6 +77,8 @@ msm-y := \ disp/dpu1/dpu_plane.o \ disp/dpu1/dpu_rm.o \ disp/dpu1/dpu_vbif.o \
disp/msm_disp_snapshot.o \disp/msm_disp_snapshot_util.o \If snapshotting is going to be used by dsi/dp, what about landing it into the top drivers/gpu/drm/msm dir? This way we'd point that it is a generic piece of code shared between all drm/msm drivers.
I'm ok with either drm/msm or drm/msm/disp (the latter because it is really about the display related components of drm/msm).. but we should not introduce dependencies in (for ex) drm/msm/dsi on dpu
msm_atomic.o \ msm_atomic_tracepoints.o \ msm_debugfs.o \diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index 4dfd8a2..0f9f0a5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved. +/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved. */
#ifndef _DPU_HW_CATALOG_H diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 88e9cc3..7529566 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -798,6 +798,67 @@ static void dpu_irq_uninstall(struct msm_kms *kms) dpu_core_irq_uninstall(dpu_kms); }
+void dpu_kms_mdp_snapshot(struct drm_device *dev) +{
int i;struct msm_drm_private *priv;struct dpu_kms *dpu_kms;struct dpu_mdss_cfg *cat;struct dpu_hw_mdp *top;struct msm_disp_state *disp_state;char name[SZ_128];priv = dev->dev_private;dpu_kms = to_dpu_kms(priv->kms);disp_state = dpu_kms->disp_state;cat = dpu_kms->catalog;top = dpu_kms->hw_mdp;pm_runtime_get_sync(&dpu_kms->pdev->dev);/* dump CTL sub-blocks HW regs info */for (i = 0; i < cat->ctl_count; i++) {snprintf(name, SZ_128, "ctl_%d", i);snprintf(name, sizeof(name), "ctl_%d", i); (the same bellow)
let's just get rid of all the snprintf and make msm_disp_snapshot_add_block() printf-like
(also, I'm not 100% sure off the top of my head if kernel's sprintf() differs from libc as far as losing terminating null if you overflow, but it would be better to deal with this in one place than N places)
msm_disp_snapshot_add_block(disp_state, name, cat->ctl[i].len,dpu_kms->mmio + cat->ctl[i].base);}/* dump DSPP sub-blocks HW regs info */for (i = 0; i < cat->dspp_count; i++) {snprintf(name, SZ_128, "dspp_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->dspp[i].len,dpu_kms->mmio + cat->dspp[i].base);}/* dump INTF sub-blocks HW regs info */for (i = 0; i < cat->intf_count; i++) {snprintf(name, SZ_128, "intf_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->intf[i].len,dpu_kms->mmio + cat->intf[i].base);}/* dump PP sub-blocks HW regs info */for (i = 0; i < cat->pingpong_count; i++) {snprintf(name, SZ_128, "ping-pong_%d", i);Just a nitpick: we usually call it pp or pingpong, without dash.
msm_disp_snapshot_add_block(disp_state, name, cat->pingpong[i].len,dpu_kms->mmio + cat->pingpong[i].base);}/* dump SSPP sub-blocks HW regs info */for (i = 0; i < cat->sspp_count; i++) {snprintf(name, SZ_128, "sspp_%d", i);msm_disp_snapshot_add_block(disp_state, name, cat->sspp[i].len,dpu_kms->mmio + cat->sspp[i].base);}snprintf(name, SZ_128, "top");msm_disp_snapshot_add_block(disp_state, name, top->hw.length,dpu_kms->mmio + top->hw.blk_off);pm_runtime_put_sync(&dpu_kms->pdev->dev);+}
- static const struct msm_kms_funcs kms_funcs = { .hw_init = dpu_kms_hw_init, .irq_preinstall = dpu_irq_preinstall,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index d6717d6..74b6ce6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -13,6 +13,7 @@ #include <drm/drm_drv.h>
#include "msm_drv.h" +#include "disp/msm_disp_snapshot.h" #include "msm_kms.h" #include "msm_mmu.h" #include "msm_gem.h" @@ -132,6 +133,8 @@ struct dpu_kms {
struct opp_table *opp_table;
struct msm_disp_state *disp_state;struct dss_module_power mp; /* reference count bandwidth requests, so we know when we can@@ -265,4 +268,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder); */ u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
+void dpu_kms_mdp_snapshot(struct drm_device *dev);
- #endif /* __dpu_kms_H__ */
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c new file mode 100644 index 0000000..1bdad16 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
- */
+#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
+#include "msm_disp_snapshot.h"
+#ifdef CONFIG_DEV_COREDUMP +static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
size_t count, void *data, size_t datalen)+{
struct drm_print_iterator iter;struct drm_printer p;struct msm_disp_state *disp_state;disp_state = data;iter.data = buffer;iter.offset = 0;iter.start = offset;iter.remain = count;p = drm_coredump_printer(&iter);msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_COREDUMP);return count - iter.remain;+}
+static void disp_devcoredump_free(void *data) +{
struct msm_disp_state *disp_state;disp_state = data;msm_disp_state_free(disp_state);disp_state->coredump_pending = false;+} +#endif /* CONFIG_DEV_COREDUMP */
+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;mutex_lock(&disp_state->mutex);msm_disp_snapshot_capture_state(disp_state);if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {p = drm_info_printer(disp_state->drm_dev->dev);msm_disp_state_print(disp_state, &p, MSM_DISP_SNAPSHOT_IN_LOG);}/** if devcoredump is not defined free the state immediately* otherwise it will be freed in the free handler.*/+#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);+}
+void msm_disp_snapshot_state(struct drm_device *drm_dev) +{
struct msm_drm_private *priv;struct dpu_kms *dpu_kms;struct msm_disp_state *disp_state;if (!drm_dev) {DRM_ERROR("invalid params\n");return;}priv = drm_dev->dev_private;dpu_kms = to_dpu_kms(priv->kms);If we are outside of disp/dpu1, we shouldn't reference dpu_kms internals. Probably this can be changed to use msm_drm_private, msm_kms or msm_mdss?
agreed
disp_state = dpu_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 happensWhere do we handle the timeout? Stale comment or did I miss something?
*/if (disp_state->coredump_pending) {DRM_DEBUG("coredump is pending read\n");return;}kthread_queue_work(disp_state->dump_worker,&disp_state->dump_work);+}
+int msm_disp_snapshot_init(struct drm_device *drm_dev) +{
struct dpu_kms *dpu_kms;struct msm_drm_private *priv;struct msm_disp_state *disp_state;if (!drm_dev) {DRM_ERROR("invalid params\n");return -EINVAL;}priv = drm_dev->dev_private;dpu_kms = to_dpu_kms(priv->kms);disp_state = devm_kzalloc(drm_dev->dev, sizeof(struct msm_disp_state), GFP_KERNEL);mutex_init(&disp_state->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))DRM_ERROR("failed to create disp state task\n");kthread_init_work(&disp_state->dump_work, _msm_disp_snapshot_work);dpu_kms->disp_state = disp_state;Judging from this code, I think it should be possible to push disp_state directly into struct msm_drm_private.
I'd be ok w/ msm_kms since it is display related, but msm_drm_private is ok.. but defn should be higher level than dpu_kms
return 0;+}
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev) +{
struct dpu_kms *dpu_kms;struct msm_drm_private *priv;struct msm_disp_state *disp_state;if (!drm_dev) {DRM_ERROR("invalid params\n");return;}priv = drm_dev->dev_private;dpu_kms = to_dpu_kms(priv->kms);disp_state = dpu_kms->disp_state;if (disp_state->dump_worker)kthread_destroy_worker(disp_state->dump_worker);list_del(&disp_state->blocks);mutex_destroy(&disp_state->mutex);+} diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h new file mode 100644 index 0000000..0ccb7e7 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.h @@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
- */
+#ifndef MSM_DISP_SNAPSHOT_H_ +#define MSM_DISP_SNAPSHOT_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"
+enum msm_disp_snapshot_dump_flag {
MSM_DISP_SNAPSHOT_IN_LOG = BIT(0),MSM_DISP_SNAPSHOT_IN_MEM = BIT(1),MSM_DISP_SNAPSHOT_IN_COREDUMP = BIT(2),Are they mutually exclusive or can they be specified in parallel?
I'd kinda like to see less MSM_DISP_SNAPSHOT_IN_*.. and no MSM_DISP_SNAPSHOT_IN_MEM
Like it is ok for caller requesting snapshot to ask for IN_LOG vs IN_COREDUMP, which is really more of a "which drm_printer do you pick", but it shouldn't be more than that.. both cases should call all the sub-blocks to snapshot to memory, and after that it should be just calling a msm_disp_snapshot_print() helper with an appropriate drm_printer.. with msm_disp_snapshot_print() being mostly an over-glorified hexdump.
So MSM_DISP_SNAPSHOT_IN_MEM should be dropped.. beyond that, if you want a boolean vs bitmask or enum for what should be done with the snapshot after it is collected, I'm not picky..
+};
+#define MSM_DISP_SNAPSHOT_MAX_BLKS 10
+/* debug option to print the registers in logs */ +#define MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE 0
+/* print debug ranges in groups of 4 u32s */ +#define REG_DUMP_ALIGN 16
+/**
- 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;+};
+/**
- struct msm_disp_state_block - structure to store each hardware block state
- @name: name of the block
- @drm_dev: handle to the linked list head
- @size: size of the register space of this hardware block
- @state: array holding the register dump of this hardware block
- @base_addr: starting address of this hardware block's register space
- */
+struct msm_disp_state_block {
char name[SZ_128];struct list_head node;unsigned int size;u32 *state;void __iomem *base_addr;+};
+/**
- MSM_DISP_SNAPSHOT - trigger to dump the display snapshot
- @drm_dev: handle to drm device
- */
+#define MSM_DISP_SNAPSHOT(drm_dev) msm_disp_snapshot_state(drm_dev)
Why do you need it?
+/**
- msm_disp_snapshot_init - initialize global sde debug facilities: evtlog, regdump
Just a nitpick here and below: there is no sde in the upstream. And no evtlog.
- @drm_dev: drm device handle
- Returns: 0 or -ERROR
- */
+int msm_disp_snapshot_init(struct drm_device *drm_dev);
+/**
- msm_disp_snapshot_destroy - destroy the global sde debug facilities
- @drm_dev: drm device handle
- Returns: none
- */
+void msm_disp_snapshot_destroy(struct drm_device *drm_dev);
+/**
- msm_disp_snapshot_state - trigger to dump the display snapshot
- @drm_dev: handle to drm device
- Returns: none
- */
+void msm_disp_snapshot_state(struct drm_device *drm_dev);
+/**
- msm_disp_state_get - get the handle to msm_disp_state struct from the drm device
- @drm: handle to drm device
- Returns: handle to the msm_disp_state struct
- */
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm);
+/**
- msm_disp_state_print - print out the current dpu state
- @disp_state: handle to drm device
- @p: handle to drm printer
- @reg_dump_method: tells whether the state has to be dumped in log or coredump
- Returns: none
- */
+void msm_disp_state_print(struct msm_disp_state *disp_state, struct drm_printer *p,
u8 reg_dump_method);+/**
- msm_disp_snapshot_capture_state - utility to capture atomic state and hw registers
- @disp_state: handle to msm_disp_state struct
- Returns: none
- */
+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
- Returns: none
- */
+void msm_disp_state_free(struct msm_disp_state *disp_state);
+/**
- msm_disp_snapshot_add_block - add a hardware block with its register dump
- @disp_state: handle to struct msm_disp_state
- @name: name of the hardware block
- @len: size of the register space of the hardware block
- @base_addr: starting address of the register space of the hardware block
- Returns: none
- */
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
void __iomem *base_addr);+#endif /* MSM_DISP_SNAPSHOT_H_ */ diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c new file mode 100644 index 0000000..383f225 --- /dev/null +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
- */
+#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
+#include "msm_disp_snapshot.h"
+static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr,
u32 dump_op, struct drm_printer *p)+{
u32 len_aligned, len_padded;u32 x0, x4, x8, xc;void __iomem *addr;u32 *dump_addr = NULL;void __iomem *end_addr;int i;bool in_drm_printer = false;len_aligned = (len + REG_DUMP_ALIGN - 1) / REG_DUMP_ALIGN;len_padded = len_aligned * REG_DUMP_ALIGN;addr = base_addr;end_addr = base_addr + len;if ((dump_op == MSM_DISP_SNAPSHOT_IN_COREDUMP) ||(dump_op == MSM_DISP_SNAPSHOT_IN_LOG))in_drm_printer = true;if (in_drm_printer && !p) {DRM_ERROR("invalid drm printer\n");return;}if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM && !(*reg))*reg = kzalloc(len_padded, GFP_KERNEL);if (*reg)dump_addr = *reg;for (i = 0; i < len_aligned; i++) {if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;}if (dump_addr) {if (dump_op == MSM_DISP_SNAPSHOT_IN_MEM) {dump_addr[i * 4] = x0;dump_addr[i * 4 + 1] = x4;dump_addr[i * 4 + 2] = x8;dump_addr[i * 4 + 3] = xc;} else if (in_drm_printer) {drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",(unsigned long)(addr - base_addr),dump_addr[i * 4], dump_addr[i * 4 + 1],dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);}}addr += REG_DUMP_ALIGN;}+}
I'd just split this function into two different cases, IN_MEM and DRM_PRINTER.
+struct msm_disp_state *msm_disp_state_get(struct drm_device *drm) +{
struct msm_drm_private *priv;struct dpu_kms *dpu_kms;priv = drm->dev_private;dpu_kms = to_dpu_kms(priv->kms);return dpu_kms->disp_state;+}
+void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p,
u8 reg_dump_method)+{
struct msm_disp_state_block *block, *tmp;if (!p) {DRM_ERROR("invalid drm printer\n");return;}drm_printf(p, "---\n");drm_printf(p, "module: " KBUILD_MODNAME "\n");drm_printf(p, "dpu devcoredump\n");drm_printf(p, "timestamp %lld\n", ktime_to_ns(state->timestamp));list_for_each_entry_safe(block, tmp, &state->blocks, node) {drm_printf(p, "====================%s================\n", block->name);msm_disp_state_dump_regs(&block->state, block->size, block->base_addr,reg_dump_method, p);}drm_printf(p, "===================dpu drm state================\n");if (state->atomic_state)drm_atomic_print_new_state(state->atomic_state, p);+}
+static void msm_disp_capture_atomic_state(struct msm_disp_state *disp_state) +{
struct drm_device *ddev;struct drm_modeset_acquire_ctx ctx;disp_state->timestamp = ktime_get();ddev = disp_state->drm_dev;drm_modeset_acquire_init(&ctx, 0);while (drm_modeset_lock_all_ctx(ddev, &ctx) != 0)drm_modeset_backoff(&ctx);disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,&ctx);drm_modeset_drop_locks(&ctx);drm_modeset_acquire_fini(&ctx);+}
+void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state) +{
struct msm_drm_private *priv;int i;struct drm_device *drm_dev;drm_dev = disp_state->drm_dev;priv = drm_dev->dev_private;msm_disp_capture_atomic_state(disp_state);if (priv->dp)msm_dp_snapshot(priv->dp);for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {if (!priv->dsi[i])continue;msm_dsi_snapshot(priv->dsi[i]);}dpu_kms_mdp_snapshot(drm_dev);Hmm, I now see why do you need the dpu_kms. Can we have a check for the mdp version here? The rest of the code seems generic enough, so we can at least dump the dsi/dp (and hopefully eDP/HDMI) data for MDP4/5.
or instead:
if (kms->funcs->snapshot) kms->funcs->snapshot(kms)
At this level we shouldn't depend on dpu.. just add a kms vfunc.
Ideally subdrivers should be able to report back to the core, which regions are available, so that we won't have this static code. But this might look like an overkill. Not to mention that it might lead to the not-so-static sequence of dumps. (As you can judge, I quite liked the previous approach, when the drivers registered the dumping regions and then the dumper would go through them.)
To be fair, I argued against registration.. at the top level, there are just a few things: hdmi, edp, dp, and dsi's.. so one small function to call the sub-blocks is a lot less code than inventing a registration infrastructure.. the "kms" object is the one place (so far) where we need a virtual method.. let's just add it to kms->funcs
BR, -R
Let's have the static config in place with the version check.
+}
+void msm_disp_state_free(struct msm_disp_state *disp_state) +{
struct msm_disp_state_block *block, *tmp;if (disp_state->atomic_state) {drm_atomic_state_put(disp_state->atomic_state);disp_state->atomic_state = NULL;}list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {kfree(block->state);list_del(&block->node);I'd swap these two lines, so while the block is in the list, its state is coherent.
kfree(block);}+}
+void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, char *name, u32 len,
void __iomem *base_addr)+{
struct msm_disp_state_block *new_blk;new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL);strscpy(new_blk->name, name, sizeof(new_blk->name));INIT_LIST_HEAD(&new_blk->node);new_blk->size = len;new_blk->base_addr = base_addr;msm_disp_state_dump_regs(&new_blk->state, len, base_addr,MSM_DISP_SNAPSHOT_IN_MEM, NULL);list_add(&new_blk->node, &disp_state->blocks);+} diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b1a9b1b..ce8d19a 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -62,6 +62,18 @@ struct dp_catalog_private { u8 aux_lut_cfg_index[PHY_AUX_CFG_MAX]; };
+void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state) +{
struct dp_catalog_private *catalog = container_of(dp_catalog,struct dp_catalog_private, dp_catalog);char name[SZ_128];snprintf(name, SZ_128, "dp_ctrl");msm_disp_snapshot_add_block(disp_state, name, catalog->io->dp_controller.len,catalog->io->dp_controller.base);+}
- static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset) { offset += MSM_DP_CONTROLLER_AUX_OFFSET;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 176a902..e7e8b13 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -9,6 +9,7 @@ #include <drm/drm_modes.h>
#include "dp_parser.h" +#include "disp/msm_disp_snapshot.h"
/* interrupts */ #define DP_INTR_HPD BIT(0) @@ -71,6 +72,9 @@ struct dp_catalog { u32 audio_data; };
+/* Debug module */ +void dp_catalog_snapshot(struct dp_catalog *dp_catalog, struct msm_disp_state *disp_state);
- /* AUX APIs */ u32 dp_catalog_aux_read_data(struct dp_catalog *dp_catalog); int dp_catalog_aux_write_data(struct dp_catalog *dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 5a39da6..6670558 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1009,6 +1009,35 @@ int dp_display_get_test_bpp(struct msm_dp *dp) dp_display->link->test_video.test_bit_depth); }
+void msm_dp_snapshot(struct msm_dp *dp) +{
struct dp_display_private *dp_display;struct drm_device *drm;struct msm_disp_state *disp_state;dp_display = container_of(dp, struct dp_display_private, dp_display);drm = dp->drm_dev;disp_state = msm_disp_state_get(drm);/** if we are reading registers we need the link clocks to be on* however till DP cable is connected this will not happen as we* do not know the resolution to power up with. Hence check the* power_on status before dumping DP registers to avoid crash due* to unclocked access*/mutex_lock(&dp_display->event_mutex);if (!dp->power_on) {mutex_unlock(&dp_display->event_mutex);return;}dp_catalog_snapshot(dp_display->catalog, disp_state);mutex_unlock(&dp_display->event_mutex);+}
- static void dp_display_config_hpd(struct dp_display_private *dp) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 6092ba1..4d39373 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -8,6 +8,7 @@
#include "dp_panel.h" #include <sound/hdmi-codec.h> +#include "disp/msm_disp_snapshot.h"
struct msm_dp { struct drm_device *drm_dev; diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 62704885..bccc006 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -266,3 +266,8 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, return ret; }
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi) +{
msm_dsi_host_snapshot(msm_dsi->host);+}
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 7abfeab..bb39403 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -15,6 +15,7 @@ #include <drm/drm_panel.h>
#include "msm_drv.h" +#include "disp/msm_disp_snapshot.h"
#define DSI_0 0 #define DSI_1 1 @@ -90,6 +91,8 @@ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi) return msm_dsi->panel || msm_dsi->external_bridge; }
+void msm_dsi_snapshot(struct msm_dsi *msm_dsi);
struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi);
/* dsi host */
@@ -146,6 +149,7 @@ int dsi_clk_init_v2(struct msm_dsi_host *msm_host); int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host); int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_dual_dsi); int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_dual_dsi); +void msm_dsi_host_snapshot(struct mipi_dsi_host *host);
/* dsi phy */ struct msm_dsi_phy; diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 8a10e43..d9fdc07 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -2487,3 +2487,22 @@ 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_snapshot(struct mipi_dsi_host *host) +{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);struct drm_device *dev = msm_host->dev;struct msm_disp_state *disp_state;char name[SZ_128];disp_state = msm_disp_state_get(dev);pm_runtime_get_sync(&msm_host->pdev->dev);snprintf(name, SZ_128, "dsi%d_ctrl", msm_host->id);msm_disp_snapshot_add_block(disp_state, name, msm_iomap_size(msm_host->pdev, "dsi_ctrl"),You can store the size into the msm_host, rather than querrying it on each dump.
msm_host->ctrl_base);pm_runtime_put_sync(&msm_host->pdev->dev);+} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index e1104d2..ebf9283 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /*
- Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
*/
- Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved.
- Copyright (C) 2013 Red Hat
- Author: Rob Clark robdclark@gmail.com
@@ -19,6 +19,7 @@ #include <drm/drm_of.h> #include <drm/drm_vblank.h>
+#include "disp/msm_disp_snapshot.h" #include "msm_drv.h" #include "msm_debugfs.h" #include "msm_fence.h" @@ -167,6 +168,24 @@ void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, return _msm_ioremap(pdev, name, dbgname, true); }
+unsigned long msm_iomap_size(struct platform_device *pdev, const char *name) +{
struct resource *res;if (name)res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);elseres = platform_get_resource(pdev, IORESOURCE_MEM, 0);if (!res) {dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",name);return 0;}return resource_size(res);+}
- void msm_writel(u32 data, void __iomem *addr) { if (reglog)
@@ -278,6 +297,8 @@ static int msm_drm_uninit(struct device *dev) msm_fbdev_free(ddev); #endif
msm_disp_snapshot_destroy(ddev);drm_mode_config_cleanup(ddev); pm_runtime_get_sync(dev);@@ -550,6 +571,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) if (ret) goto err_msm_uninit;
if (get_mdp_ver(pdev) == KMS_DPU) {ret = msm_disp_snapshot_init(ddev);if (ret)DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);}Let's get it out of if (KMS_DPU), even if we do not dump mdp4/mdp5 registers for now.
drm_mode_config_reset(ddev);#ifdef CONFIG_DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 2668941..9c40bac 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -367,6 +367,7 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); void msm_dp_irq_postinstall(struct msm_dp *dp_display); +void msm_dp_snapshot(struct msm_dp *dp_display);
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
@@ -450,6 +451,7 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name, const char *dbgname); void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name, const char *dbgname); +unsigned long msm_iomap_size(struct platform_device *pdev, const char *name); void msm_writel(u32 data, void __iomem *addr); u32 msm_readl(const void __iomem *addr); void msm_rmw(void __iomem *addr, u32 mask, u32 or);
-- With best wishes Dmitry
Add snapshot points across dpu driver to trigger dumps when critical errors are hit.
changes in v4: - change the callers to use the new snapshot macro
Signed-off-by: Abhinav Kumar abhinavk@codeaurora.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++++++++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 14 +++++++++----- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8 +++++++- 3 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 8d94205..39b2df9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved. + * Copyright (c) 2014-2018, 2020-2021 The Linux Foundation. All rights reserved. * Copyright (C) 2013 Red Hat * Author: Rob Clark robdclark@gmail.com */ @@ -26,6 +26,7 @@ #include "dpu_crtc.h" #include "dpu_trace.h" #include "dpu_core_irq.h" +#include "disp/msm_disp_snapshot.h"
#define DPU_DEBUG_ENC(e, fmt, ...) DPU_DEBUG("enc%d " fmt,\ (e) ? (e)->base.base.id : -1, ##__VA_ARGS__) @@ -1336,6 +1337,13 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
DPU_ATRACE_BEGIN("encoder_underrun_callback"); atomic_inc(&phy_enc->underrun_cnt); + + /* trigger dump only on the first underrun */ + if (atomic_read(&phy_enc->underrun_cnt) == 1) { + pr_err("triggering dump\n"); + MSM_DISP_SNAPSHOT(drm_enc->dev); + } + trace_dpu_enc_underrun_cb(DRMID(drm_enc), atomic_read(&phy_enc->underrun_cnt)); DPU_ATRACE_END("encoder_underrun_callback"); @@ -1565,19 +1573,23 @@ static void dpu_encoder_helper_hw_reset(struct dpu_encoder_phys *phys_enc) struct dpu_encoder_virt *dpu_enc; struct dpu_hw_ctl *ctl; int rc; + struct drm_encoder *drm_enc;
dpu_enc = to_dpu_encoder_virt(phys_enc->parent); ctl = phys_enc->hw_ctl; + drm_enc = phys_enc->parent;
if (!ctl->ops.reset) return;
- DRM_DEBUG_KMS("id:%u ctl %d reset\n", DRMID(phys_enc->parent), + DRM_DEBUG_KMS("id:%u ctl %d reset\n", DRMID(drm_enc), ctl->idx);
rc = ctl->ops.reset(ctl); - if (rc) + if (rc) { DPU_ERROR_ENC(dpu_enc, "ctl %d reset failure\n", ctl->idx); + MSM_DISP_SNAPSHOT(drm_enc->dev); + }
phys_enc->enable_state = DPU_ENC_ENABLED; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index b2be39b..f587985 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2015-2018 The Linux Foundation. All rights reserved. + * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All rights reserved. */
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__ @@ -11,6 +11,7 @@ #include "dpu_core_irq.h" #include "dpu_formats.h" #include "dpu_trace.h" +#include "disp/msm_disp_snapshot.h"
#define DPU_DEBUG_CMDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \ (e) && (e)->base.parent ? \ @@ -191,10 +192,13 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( to_dpu_encoder_phys_cmd(phys_enc); u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR; bool do_log = false; + struct drm_encoder *drm_enc;
if (!phys_enc->hw_pp) return -EINVAL;
+ drm_enc = phys_enc->parent; + cmd_enc->pp_timeout_report_cnt++; if (cmd_enc->pp_timeout_report_cnt == PP_TIMEOUT_MAX_TRIALS) { frame_event |= DPU_ENCODER_FRAME_EVENT_PANEL_DEAD; @@ -203,7 +207,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( do_log = true; }
- trace_dpu_enc_phys_cmd_pdone_timeout(DRMID(phys_enc->parent), + trace_dpu_enc_phys_cmd_pdone_timeout(DRMID(drm_enc), phys_enc->hw_pp->idx - PINGPONG_0, cmd_enc->pp_timeout_report_cnt, atomic_read(&phys_enc->pending_kickoff_cnt), @@ -212,12 +216,12 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( /* to avoid flooding, only log first time, and "dead" time */ if (do_log) { DRM_ERROR("id:%d pp:%d kickoff timeout %d cnt %d koff_cnt %d\n", - DRMID(phys_enc->parent), + DRMID(drm_enc), phys_enc->hw_pp->idx - PINGPONG_0, phys_enc->hw_ctl->idx - CTL_0, cmd_enc->pp_timeout_report_cnt, atomic_read(&phys_enc->pending_kickoff_cnt)); - + MSM_DISP_SNAPSHOT(drm_enc->dev); dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_RDPTR); }
@@ -228,7 +232,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
if (phys_enc->parent_ops->handle_frame_done) phys_enc->parent_ops->handle_frame_done( - phys_enc->parent, phys_enc, frame_event); + drm_enc, phys_enc, frame_event);
return -ETIMEDOUT; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 0e06b7e..9fdf888 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved. +/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All rights reserved. */
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__ @@ -9,6 +9,7 @@ #include "dpu_core_irq.h" #include "dpu_formats.h" #include "dpu_trace.h" +#include "disp/msm_disp_snapshot.h"
#define DPU_DEBUG_VIDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \ (e) && (e)->parent ? \ @@ -468,6 +469,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) "update pending flush ctl %d intf %d\n", ctl->idx - CTL_0, phys_enc->hw_intf->idx);
+ atomic_set(&phys_enc->underrun_cnt, 0);
/* ctl_flush & timing engine enable will be triggered by framework */ if (phys_enc->enable_state == DPU_ENC_DISABLED) @@ -537,6 +539,9 @@ static void dpu_encoder_phys_vid_prepare_for_kickoff( { struct dpu_hw_ctl *ctl; int rc; + struct drm_encoder *drm_enc; + + drm_enc = phys_enc->parent;
ctl = phys_enc->hw_ctl; if (!ctl->ops.wait_reset_status) @@ -550,6 +555,7 @@ static void dpu_encoder_phys_vid_prepare_for_kickoff( if (rc) { DPU_ERROR_VIDENC(phys_enc, "ctl %d reset failure: %d\n", ctl->idx, rc); + MSM_DISP_SNAPSHOT(drm_enc->dev); dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_VSYNC); } }
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
abhinavk＠codeaurora.org
-
Dmitry Baryshkov -
Rob Clark