Re: [PATCH] drm: mali-dp: Log internal errors

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

 



Hi Alex,

Thanks for the patch, it is quite useful. I have some small changes to
suggest:

On Thu, Feb 22, 2018 at 04:02:37PM +0000, Alexandru Gheorghe wrote:
> From: Alexandru Gheorghe <Alexandru-Cosmin.Gheorghe@xxxxxxx>
> 
> Status register contains a lot of bits for reporting internal errors
> inside mali-dp. Currently, we just silently ignore all of the erorrs,

mali-dp is the driver, I think you are actually talking about Mali DP
hardware here, so worth making it "inside Mali DP."

> 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
> end up in AXI or underrun errors.
> 
> Add a new KConfig option called CONFIG_DRM_MALI_DISPLAY_DEBUG, which
> enables the logging of the errors present in the status register.
> 
> Errors will be reported in  /sys/kernel/debug/tracing/trace,
> still not noisy enough, but better than silently ignoring them.
> 
> E.g:
> <idle>-0     [000] d.h1  2387.240042: malidp_de_irq: error occurred at vblank 596 DE_STATUS is 0x00000001
> <idle>-0     [000] d.h1  2387.256703: malidp_de_irq: error occurred at vblank 597 DE_STATUS is 0x00000001
> <idle>-0     [000] d.h1  2387.273458: malidp_de_irq: error occurred at vblank 598 DE_STATUS is 0x00000001
> <idle>-0     [000] d.h1  2387.290035: malidp_de_irq: error occurred at vblank 599 DE_STATUS is 0x00000001
> 
> In addition to that, I removed the setting of MALIDP550_SE_IRQ_AXI_ERR
> bit in the irq_mask, since that bit doesn't exist.
> 
> Signed-off-by: Alexandru Gheorghe <Alexandru-Cosmin.Gheorghe@xxxxxxx>
> ---
>  drivers/gpu/drm/arm/Kconfig       | 10 +++++++++
>  drivers/gpu/drm/arm/malidp_hw.c   | 45 ++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/arm/malidp_hw.h   |  1 +
>  drivers/gpu/drm/arm/malidp_regs.h |  6 ++++++
>  4 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
> index 9a18e1b..c1e6fc8 100644
> --- a/drivers/gpu/drm/arm/Kconfig
> +++ b/drivers/gpu/drm/arm/Kconfig
> @@ -40,3 +40,13 @@ config DRM_MALI_DISPLAY
>  	  of the hardware.
>  
>  	  If compiled as a module it will be called mali-dp.
> +
> +config DRM_MALI_DISPLAY_DEBUG
> +	bool "Enable mali display debugging"

s/mali display/Mali DP/

> +	default n
> +	depends on DRM_MALI_DISPLAY
> +	select FTRACE
> +	help
> +	  Enable this option to log internal errors that happened during
> +	  processing of a frame. Errors will be reported in
> +	  /sys/kernel/debug/tracing/trace.
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index 2bfb542..7d3b2e2 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,
> @@ -793,10 +814,15 @@ 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)
>  		drm_crtc_handle_vblank(&malidp->crtc);
> -
> +#ifdef CONFIG_DRM_MALI_DISPLAY_DEBUG
> +	if (status & de->err_mask)
> +		trace_printk("error occurred at vblank %llu DE_STATUS is 0x%08X\n",
> +			     drm_crtc_vblank_count(&malidp->crtc), status);
> +#endif
>  	malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, status);
>  
>  	return (ret == IRQ_NONE) ? IRQ_HANDLED : ret;
> @@ -874,9 +900,12 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
>  	status = malidp_hw_read(hwdev, hw->map.se_base + MALIDP_REG_STATUS);
>  	if (!(status & se->irq_mask))
>  		return IRQ_NONE;
> -
> +#ifdef CONFIG_DRM_MALI_DISPLAY_DEBUG
> +	if (status & se->err_mask)
> +		trace_printk("error occurred at vblank %llu SE_STATUS is 0x%08X\n",
> +			     drm_crtc_vblank_count(&malidp->crtc), status);
> +#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 b0690eb..909b76e 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 in status register 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 2039f85..ec53811 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
> 

With those changes, it looks good to me.

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

Best regards,
Liviu


-- 
====================
| 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