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
