Re: [PATCH] drm: mali-dp: Add debugfs file for reporting internal errors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alex,

On Fri, May 11, 2018 at 06:56:03PM +0100, Alexandru Gheorghe wrote:
Status register contains a lot of bits for reporting internal errors
inside Mali DP. Currently, we just silently ignore all of the erorrs,

Being picky: s/erorrs/errors/

that doesn't help when we are investigating different bugs, especially
on the FPGA models which have a lot of constrains, so we could easily

s/constrains/constraints/

end up in AXI or underrun errors.

Add a new file called debug that contains an agregate of the

s/agregate/aggregate/

errors reported by the Mali DP hardware.

E.g:
[root@alarm ~]# cat /sys/kernel/debug/dri/1/debug
[DE] num_errors : 167
[DE] last_error_status  : 0x00000001
[DE] last_error_vblank : 385
[SE] num_errors : 3
[SE] last_error_status  : 0x00e23001
[SE] last_error_vblank : 201

This a morphosis of the patch presented here [1], where the errors
where reported continuously via trace_printk.

[1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167042.html

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@xxxxxxx>
---
drivers/gpu/drm/arm/malidp_drv.c  | 61 +++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/arm/malidp_drv.h  | 15 ++++++++++
drivers/gpu/drm/arm/malidp_hw.c   | 46 ++++++++++++++++++++++++-----
drivers/gpu/drm/arm/malidp_hw.h   |  1 +
drivers/gpu/drm/arm/malidp_regs.h |  6 ++++
5 files changed, 122 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 8d20faa..70ce19a 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -17,6 +17,7 @@
#include <linux/of_graph.h>
#include <linux/of_reserved_mem.h>
#include <linux/pm_runtime.h>
+#include <linux/debugfs.h>

#include <drm/drmP.h>
#include <drm/drm_atomic.h>
@@ -29,6 +30,7 @@
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_modeset_helper.h>
#include <drm/drm_of.h>
+#include <drm/drm_debugfs.h>

#include "malidp_drv.h"
#include "malidp_regs.h"
@@ -327,6 +329,62 @@ static int malidp_dumb_create(struct drm_file *file_priv,
	return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
}

+#ifdef CONFIG_DEBUG_FS
+
+static void malidp_error_stats_init(struct malidp_error_stats *error_stats)
+{
+	atomic_set(&error_stats->num_errors, 0);
+	atomic_set(&error_stats->last_error_status, 0);
+	atomic64_set(&error_stats->last_error_vblank, -1);
+}
+
+void malidp_error(struct malidp_error_stats *error_stats, u32 status,
+		  u64 vblank)
+{
+	atomic_set(&error_stats->last_error_status, status);
+	atomic64_set(&error_stats->last_error_vblank, vblank);
+	atomic_inc(&error_stats->num_errors);
+}
+

Do we already have a lock we could use to make sure the status printed
is consistent? I know this is "only debug", but it could be annoying
if we can't trust that the last_error_status does actually match the
last_error_vblank.

+void malidp_error_stats_dump(const char *prefix,
+			     struct malidp_error_stats *error_stats,
+			     struct seq_file *m)
+{
+	seq_printf(m, "[%s] num_errors : %d\n", prefix,
+		   atomic_read(&error_stats->num_errors));
+	seq_printf(m, "[%s] last_error_status  : 0x%08x\n", prefix,
+		   atomic_read(&error_stats->last_error_status));
+	seq_printf(m, "[%s] last_error_vblank : %ld\n", prefix,
+		   atomic64_read(&error_stats->last_error_vblank));
+}
+
+static int malidp_show_stats(struct seq_file *m, void *arg)
+{
+	struct drm_info_node *node = (struct drm_info_node *)m->private;
+	struct drm_device *drm = node->minor->dev;
+	struct malidp_drm *malidp = drm->dev_private;
+
+	malidp_error_stats_dump("DE", &malidp->de_errors, m);
+	malidp_error_stats_dump("SE", &malidp->se_errors, m);
+	return 0;
+}
+
+static struct drm_info_list malidp_debugfs_list[] = {
+	{ "debug", malidp_show_stats, 0 },
+};

I think it would be pretty useful to have a way to reset the counters.
Maybe by just writing anything to the file?

Thanks,
-Brian

+
+static int malidp_debugfs_init(struct drm_minor *minor)
+{
+	struct malidp_drm *malidp = minor->dev->dev_private;
+
+	malidp_error_stats_init(&malidp->de_errors);
+	malidp_error_stats_init(&malidp->se_errors);
+	return drm_debugfs_create_files(malidp_debugfs_list,
+		ARRAY_SIZE(malidp_debugfs_list), minor->debugfs_root, minor);
+}
+
+#endif //CONFIG_DEBUG_FS
+
static struct drm_driver malidp_driver = {
	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
			   DRIVER_PRIME,
@@ -343,6 +401,9 @@ static struct drm_driver malidp_driver = {
	.gem_prime_vmap = drm_gem_cma_prime_vmap,
	.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
	.gem_prime_mmap = drm_gem_cma_prime_mmap,
+#ifdef CONFIG_DEBUG_FS
+	.debugfs_init = malidp_debugfs_init,
+#endif
	.fops = &fops,
	.name = "mali-dp",
	.desc = "ARM Mali Display Processor driver",
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index c70989b..c49056c 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -18,6 +18,12 @@
#include <drm/drmP.h>
#include "malidp_hw.h"

+struct malidp_error_stats {
+	atomic_t num_errors;
+	atomic_t last_error_status;
+	atomic64_t last_error_vblank;
+};
+
struct malidp_drm {
	struct malidp_hw_device *dev;
	struct drm_crtc crtc;
@@ -25,6 +31,10 @@ struct malidp_drm {
	struct drm_pending_vblank_event *event;
	atomic_t config_valid;
	u32 core_id;
+#ifdef CONFIG_DEBUG_FS
+	struct malidp_error_stats de_errors;
+	struct malidp_error_stats se_errors;
+#endif
};

#define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc)
@@ -62,6 +72,11 @@ struct malidp_crtc_state {
int malidp_de_planes_init(struct drm_device *drm);
int malidp_crtc_init(struct drm_device *drm);

+#ifdef CONFIG_DEBUG_FS
+void malidp_error(struct malidp_error_stats *error_stats, u32 status,
+		  u64 vblank);
+#endif
+
/* often used combination of rotational bits */
#define MALIDP_ROTATED_MASK	(DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index d789b46..ec40a44 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -632,10 +632,16 @@ const struct malidp_hw malidp_device[MALIDP_MAX_DEVICES] = {
					    MALIDP500_DE_IRQ_VSYNC |
					    MALIDP500_DE_IRQ_GLOBAL,
				.vsync_irq = MALIDP500_DE_IRQ_VSYNC,
+				.err_mask = MALIDP_DE_IRQ_UNDERRUN |
+					    MALIDP500_DE_IRQ_AXI_ERR |
+					    MALIDP500_DE_IRQ_SATURATION,
			},
			.se_irq_map = {
				.irq_mask = MALIDP500_SE_IRQ_CONF_MODE,
				.vsync_irq = 0,
+				.err_mask = MALIDP500_SE_IRQ_INIT_BUSY |
+					    MALIDP500_SE_IRQ_AXI_ERROR |
+					    MALIDP500_SE_IRQ_OVERRUN,
			},
			.dc_irq_map = {
				.irq_mask = MALIDP500_DE_IRQ_CONF_VALID,
@@ -669,10 +675,15 @@ const struct malidp_hw malidp_device[MALIDP_MAX_DEVICES] = {
				.irq_mask = MALIDP_DE_IRQ_UNDERRUN |
					    MALIDP550_DE_IRQ_VSYNC,
				.vsync_irq = MALIDP550_DE_IRQ_VSYNC,
+				.err_mask = MALIDP_DE_IRQ_UNDERRUN |
+					    MALIDP550_DE_IRQ_SATURATION |
+					    MALIDP550_DE_IRQ_AXI_ERR,
			},
			.se_irq_map = {
-				.irq_mask = MALIDP550_SE_IRQ_EOW |
-					    MALIDP550_SE_IRQ_AXI_ERR,
+				.irq_mask = MALIDP550_SE_IRQ_EOW,
+				.err_mask  = MALIDP550_SE_IRQ_AXI_ERR |
+					     MALIDP550_SE_IRQ_OVR |
+					     MALIDP550_SE_IRQ_IBSY,
			},
			.dc_irq_map = {
				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
@@ -707,10 +718,20 @@ const struct malidp_hw malidp_device[MALIDP_MAX_DEVICES] = {
					    MALIDP650_DE_IRQ_DRIFT |
					    MALIDP550_DE_IRQ_VSYNC,
				.vsync_irq = MALIDP550_DE_IRQ_VSYNC,
+				.err_mask = MALIDP_DE_IRQ_UNDERRUN |
+					    MALIDP650_DE_IRQ_DRIFT |
+					    MALIDP550_DE_IRQ_SATURATION |
+					    MALIDP550_DE_IRQ_AXI_ERR |
+					    MALIDP650_DE_IRQ_ACEV1 |
+					    MALIDP650_DE_IRQ_ACEV2 |
+					    MALIDP650_DE_IRQ_ACEG |
+					    MALIDP650_DE_IRQ_AXIEP,
			},
			.se_irq_map = {
-				.irq_mask = MALIDP550_SE_IRQ_EOW |
-					    MALIDP550_SE_IRQ_AXI_ERR,
+				.irq_mask = MALIDP550_SE_IRQ_EOW,
+				.err_mask = MALIDP550_SE_IRQ_AXI_ERR |
+					    MALIDP550_SE_IRQ_OVR |
+					    MALIDP550_SE_IRQ_IBSY,
			},
			.dc_irq_map = {
				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
@@ -799,10 +820,17 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
		return ret;

	mask = malidp_hw_read(hwdev, MALIDP_REG_MASKIRQ);
-	status &= mask;
+	/* keep the status of the enabled interrupts, plus the error bits */
+	status &= (mask | de->err_mask);
	if ((status & de->vsync_irq) && malidp->crtc.enabled)
		drm_crtc_handle_vblank(&malidp->crtc);

+#ifdef CONFIG_DEBUG_FS
+	if (status & de->err_mask) {
+		malidp_error(&malidp->de_errors, status,
+			     drm_crtc_vblank_count(&malidp->crtc));
+	}
+#endif
	malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, status);

	return (ret == IRQ_NONE) ? IRQ_HANDLED : ret;
@@ -878,11 +906,15 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
		return IRQ_NONE;

	status = malidp_hw_read(hwdev, hw->map.se_base + MALIDP_REG_STATUS);
-	if (!(status & se->irq_mask))
+	if (!(status & (se->irq_mask | se->err_mask)))
		return IRQ_NONE;

+#ifdef CONFIG_DEBUG_FS
+	if (status & se->err_mask)
+		malidp_error(&malidp->se_errors, status,
+			     drm_crtc_vblank_count(&malidp->crtc));
+#endif
	mask = malidp_hw_read(hwdev, hw->map.se_base + MALIDP_REG_MASKIRQ);
-	status = malidp_hw_read(hwdev, hw->map.se_base + MALIDP_REG_STATUS);
	status &= mask;
	/* ToDo: status decoding and firing up of VSYNC and page flip events */

diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index b5dd6c7..b45f99f 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -52,6 +52,7 @@ struct malidp_format_id {
struct malidp_irq_map {
	u32 irq_mask;		/* mask of IRQs that can be enabled in the block */
	u32 vsync_irq;		/* IRQ bit used for signaling during VSYNC */
+	u32 err_mask;		/* mask of bits that represent errors */
};

struct malidp_layer {
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index 149024f..7c37390 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -53,6 +53,8 @@
#define MALIDP550_DE_IRQ_AXI_ERR		(1 << 16)
#define MALIDP550_SE_IRQ_EOW			(1 << 0)
#define MALIDP550_SE_IRQ_AXI_ERR		(1 << 16)
+#define MALIDP550_SE_IRQ_OVR			(1 << 17)
+#define MALIDP550_SE_IRQ_IBSY			(1 << 18)
#define MALIDP550_DC_IRQ_CONF_VALID		(1 << 0)
#define MALIDP550_DC_IRQ_CONF_MODE		(1 << 4)
#define MALIDP550_DC_IRQ_CONF_ACTIVE		(1 << 16)
@@ -60,6 +62,10 @@
#define MALIDP550_DC_IRQ_SE			(1 << 24)

#define MALIDP650_DE_IRQ_DRIFT			(1 << 4)
+#define MALIDP650_DE_IRQ_ACEV1			(1 << 17)
+#define MALIDP650_DE_IRQ_ACEV2			(1 << 18)
+#define MALIDP650_DE_IRQ_ACEG			(1 << 19)
+#define MALIDP650_DE_IRQ_AXIEP			(1 << 28)

/* bit masks that are common between products */
#define   MALIDP_CFG_VALID		(1 << 0)
--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux