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.gheor...@arm.com>
> 
> 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.gheor...@arm.com>
> ---
>  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.du...@arm.com>

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to