| Message ID | 1519315357-9245-1-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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.Gheorghe@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.dudau@arm.com> Best regards, Liviu
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" + 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)