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

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

 



On Tue, May 15, 2018 at 11:18:50AM +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 errors,
> that doesn't help when we are investigating different bugs, especially
> on the FPGA models which have a lot of constraints, so we could easily
> end up in AXI or underrun errors.
> 
> Add a new file called debug that contains an aggregate of the
> errors reported by the Mali DP hardware.

Hi Alex,

Sorry, I thought I had Acked this one already, but I cannot find the
proof, so:

Acked-by: Liviu Dudau <liviu.dudau@xxxxxxx>

Best regards,
Liviu

> 
> 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
> 
> Changes since v2:
> - Add lock to protect the errors stats.
> - Add possibility to reset the error stats by writing anything to the
>   debug file.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@xxxxxxx>
> ---
>  drivers/gpu/drm/arm/malidp_drv.c  | 104 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/arm/malidp_drv.h  |  19 +++++++
>  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, 169 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 8d20faa..8bfeb46 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>
> @@ -327,6 +328,106 @@ 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)
> +{
> +	error_stats->num_errors = 0;
> +	error_stats->last_error_status = 0;
> +	error_stats->last_error_vblank = -1;
> +}
> +
> +void malidp_error(struct malidp_drm *malidp,
> +		  struct malidp_error_stats *error_stats, u32 status,
> +		  u64 vblank)
> +{
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&malidp->errors_lock, irqflags);
> +	error_stats->last_error_status = status;
> +	error_stats->last_error_vblank = vblank;
> +	error_stats->num_errors++;
> +	spin_unlock_irqrestore(&malidp->errors_lock, irqflags);
> +}
> +
> +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,
> +		   error_stats.num_errors);
> +	seq_printf(m, "[%s] last_error_status  : 0x%08x\n", prefix,
> +		   error_stats.last_error_status);
> +	seq_printf(m, "[%s] last_error_vblank : %lld\n", prefix,
> +		   error_stats.last_error_vblank);
> +}
> +
> +static int malidp_show_stats(struct seq_file *m, void *arg)
> +{
> +	struct drm_device *drm = m->private;
> +	struct malidp_drm *malidp = drm->dev_private;
> +	unsigned long irqflags;
> +	struct malidp_error_stats de_errors, se_errors;
> +
> +	spin_lock_irqsave(&malidp->errors_lock, irqflags);
> +	de_errors = malidp->de_errors;
> +	se_errors = malidp->se_errors;
> +	spin_unlock_irqrestore(&malidp->errors_lock, irqflags);
> +	malidp_error_stats_dump("DE", de_errors, m);
> +	malidp_error_stats_dump("SE", se_errors, m);
> +	return 0;
> +}
> +
> +static int malidp_debugfs_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, malidp_show_stats, inode->i_private);
> +}
> +
> +static ssize_t malidp_debugfs_write(struct file *file, const char __user *ubuf,
> +				    size_t len, loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct drm_device *drm = m->private;
> +	struct malidp_drm *malidp = drm->dev_private;
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&malidp->errors_lock, irqflags);
> +	malidp_error_stats_init(&malidp->de_errors);
> +	malidp_error_stats_init(&malidp->se_errors);
> +	spin_unlock_irqrestore(&malidp->errors_lock, irqflags);
> +	return len;
> +}
> +
> +static const struct file_operations malidp_debugfs_fops = {
> +	.owner = THIS_MODULE,
> +	.open = malidp_debugfs_open,
> +	.read = seq_read,
> +	.write = malidp_debugfs_write,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static int malidp_debugfs_init(struct drm_minor *minor)
> +{
> +	struct malidp_drm *malidp = minor->dev->dev_private;
> +	struct dentry *dentry = NULL;
> +
> +	malidp_error_stats_init(&malidp->de_errors);
> +	malidp_error_stats_init(&malidp->se_errors);
> +	spin_lock_init(&malidp->errors_lock);
> +	dentry = debugfs_create_file("debug",
> +				     S_IRUGO | S_IWUSR,
> +				     minor->debugfs_root, minor->dev,
> +				     &malidp_debugfs_fops);
> +	if (!dentry) {
> +		DRM_ERROR("Cannot create debug file\n");
> +		return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +#endif //CONFIG_DEBUG_FS
> +
>  static struct drm_driver malidp_driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
>  			   DRIVER_PRIME,
> @@ -343,6 +444,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..bd907a4 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.h
> +++ b/drivers/gpu/drm/arm/malidp_drv.h
> @@ -15,9 +15,16 @@
>  
>  #include <linux/mutex.h>
>  #include <linux/wait.h>
> +#include <linux/spinlock.h>
>  #include <drm/drmP.h>
>  #include "malidp_hw.h"
>  
> +struct malidp_error_stats {
> +	s32 num_errors;
> +	u32 last_error_status;
> +	s64 last_error_vblank;
> +};
> +
>  struct malidp_drm {
>  	struct malidp_hw_device *dev;
>  	struct drm_crtc crtc;
> @@ -25,6 +32,12 @@ 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;
> +	/* Protects errors stats */
> +	spinlock_t errors_lock;
> +#endif
>  };
>  
>  #define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc)
> @@ -62,6 +75,12 @@ 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_drm *malidp,
> +		  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..c9896ae 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, &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, &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
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
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