| Message ID | 1526061363-30411-1-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 11, 2018 at 06:56:03PM +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 erorrs, > 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 file called debug that contains an agregate of the > errors reported by the Mali DP hardware. > > 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 > > This a morphosis of the patch presented here [1], where the errors > where reported continuously via trace_printk. > > [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167042.html > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> Acked-by: Liviu Dudau <liviu.dudau@arm.com> > --- > drivers/gpu/drm/arm/malidp_drv.c | 61 +++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/arm/malidp_drv.h | 15 ++++++++++ > 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, 122 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > index 8d20faa..70ce19a 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> > @@ -29,6 +30,7 @@ > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_modeset_helper.h> > #include <drm/drm_of.h> > +#include <drm/drm_debugfs.h> > > #include "malidp_drv.h" > #include "malidp_regs.h" > @@ -327,6 +329,62 @@ 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) > +{ > + atomic_set(&error_stats->num_errors, 0); > + atomic_set(&error_stats->last_error_status, 0); > + atomic64_set(&error_stats->last_error_vblank, -1); > +} > + > +void malidp_error(struct malidp_error_stats *error_stats, u32 status, > + u64 vblank) > +{ > + atomic_set(&error_stats->last_error_status, status); > + atomic64_set(&error_stats->last_error_vblank, vblank); > + atomic_inc(&error_stats->num_errors); > +} > + > +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, > + atomic_read(&error_stats->num_errors)); > + seq_printf(m, "[%s] last_error_status : 0x%08x\n", prefix, > + atomic_read(&error_stats->last_error_status)); > + seq_printf(m, "[%s] last_error_vblank : %ld\n", prefix, > + atomic64_read(&error_stats->last_error_vblank)); > +} > + > +static int malidp_show_stats(struct seq_file *m, void *arg) > +{ > + struct drm_info_node *node = (struct drm_info_node *)m->private; > + struct drm_device *drm = node->minor->dev; > + struct malidp_drm *malidp = drm->dev_private; > + > + malidp_error_stats_dump("DE", &malidp->de_errors, m); > + malidp_error_stats_dump("SE", &malidp->se_errors, m); > + return 0; > +} > + > +static struct drm_info_list malidp_debugfs_list[] = { > + { "debug", malidp_show_stats, 0 }, > +}; > + > +static int malidp_debugfs_init(struct drm_minor *minor) > +{ > + struct malidp_drm *malidp = minor->dev->dev_private; > + > + malidp_error_stats_init(&malidp->de_errors); > + malidp_error_stats_init(&malidp->se_errors); > + return drm_debugfs_create_files(malidp_debugfs_list, > + ARRAY_SIZE(malidp_debugfs_list), minor->debugfs_root, minor); > +} > + > +#endif //CONFIG_DEBUG_FS > + > static struct drm_driver malidp_driver = { > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | > DRIVER_PRIME, > @@ -343,6 +401,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..c49056c 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.h > +++ b/drivers/gpu/drm/arm/malidp_drv.h > @@ -18,6 +18,12 @@ > #include <drm/drmP.h> > #include "malidp_hw.h" > > +struct malidp_error_stats { > + atomic_t num_errors; > + atomic_t last_error_status; > + atomic64_t last_error_vblank; > +}; > + > struct malidp_drm { > struct malidp_hw_device *dev; > struct drm_crtc crtc; > @@ -25,6 +31,10 @@ 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; > +#endif > }; > > #define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc) > @@ -62,6 +72,11 @@ 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_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..ec40a44 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->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->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 >
Hi Alex, On Fri, May 11, 2018 at 06:56:03PM +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 erorrs, Being picky: s/erorrs/errors/ >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 s/constrains/constraints/ >end up in AXI or underrun errors. > >Add a new file called debug that contains an agregate of the s/agregate/aggregate/ >errors reported by the Mali DP hardware. > >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 > >This a morphosis of the patch presented here [1], where the errors >where reported continuously via trace_printk. > >[1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167042.html > >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> >--- > drivers/gpu/drm/arm/malidp_drv.c | 61 +++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/arm/malidp_drv.h | 15 ++++++++++ > 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, 122 insertions(+), 7 deletions(-) > >diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c >index 8d20faa..70ce19a 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> >@@ -29,6 +30,7 @@ > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_modeset_helper.h> > #include <drm/drm_of.h> >+#include <drm/drm_debugfs.h> > > #include "malidp_drv.h" > #include "malidp_regs.h" >@@ -327,6 +329,62 @@ 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) >+{ >+ atomic_set(&error_stats->num_errors, 0); >+ atomic_set(&error_stats->last_error_status, 0); >+ atomic64_set(&error_stats->last_error_vblank, -1); >+} >+ >+void malidp_error(struct malidp_error_stats *error_stats, u32 status, >+ u64 vblank) >+{ >+ atomic_set(&error_stats->last_error_status, status); >+ atomic64_set(&error_stats->last_error_vblank, vblank); >+ atomic_inc(&error_stats->num_errors); >+} >+ Do we already have a lock we could use to make sure the status printed is consistent? I know this is "only debug", but it could be annoying if we can't trust that the last_error_status does actually match the last_error_vblank. >+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, >+ atomic_read(&error_stats->num_errors)); >+ seq_printf(m, "[%s] last_error_status : 0x%08x\n", prefix, >+ atomic_read(&error_stats->last_error_status)); >+ seq_printf(m, "[%s] last_error_vblank : %ld\n", prefix, >+ atomic64_read(&error_stats->last_error_vblank)); >+} >+ >+static int malidp_show_stats(struct seq_file *m, void *arg) >+{ >+ struct drm_info_node *node = (struct drm_info_node *)m->private; >+ struct drm_device *drm = node->minor->dev; >+ struct malidp_drm *malidp = drm->dev_private; >+ >+ malidp_error_stats_dump("DE", &malidp->de_errors, m); >+ malidp_error_stats_dump("SE", &malidp->se_errors, m); >+ return 0; >+} >+ >+static struct drm_info_list malidp_debugfs_list[] = { >+ { "debug", malidp_show_stats, 0 }, >+}; I think it would be pretty useful to have a way to reset the counters. Maybe by just writing anything to the file? Thanks, -Brian >+ >+static int malidp_debugfs_init(struct drm_minor *minor) >+{ >+ struct malidp_drm *malidp = minor->dev->dev_private; >+ >+ malidp_error_stats_init(&malidp->de_errors); >+ malidp_error_stats_init(&malidp->se_errors); >+ return drm_debugfs_create_files(malidp_debugfs_list, >+ ARRAY_SIZE(malidp_debugfs_list), minor->debugfs_root, minor); >+} >+ >+#endif //CONFIG_DEBUG_FS >+ > static struct drm_driver malidp_driver = { > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | > DRIVER_PRIME, >@@ -343,6 +401,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..c49056c 100644 >--- a/drivers/gpu/drm/arm/malidp_drv.h >+++ b/drivers/gpu/drm/arm/malidp_drv.h >@@ -18,6 +18,12 @@ > #include <drm/drmP.h> > #include "malidp_hw.h" > >+struct malidp_error_stats { >+ atomic_t num_errors; >+ atomic_t last_error_status; >+ atomic64_t last_error_vblank; >+}; >+ > struct malidp_drm { > struct malidp_hw_device *dev; > struct drm_crtc crtc; >@@ -25,6 +31,10 @@ 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; >+#endif > }; > > #define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc) >@@ -62,6 +72,11 @@ 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_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..ec40a44 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->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->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 >
On Mon, May 14, 2018 at 10:19:27AM +0100, Brian Starkey wrote: > Hi Alex, > > On Fri, May 11, 2018 at 06:56:03PM +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 erorrs, > > Being picky: s/erorrs/errors/ > > >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 > > s/constrains/constraints/ > > >end up in AXI or underrun errors. > > > >Add a new file called debug that contains an agregate of the > > s/agregate/aggregate/ > > >errors reported by the Mali DP hardware. > > > >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 > > > >This a morphosis of the patch presented here [1], where the errors > >where reported continuously via trace_printk. > > > >[1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167042.html > > > >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > >--- > >drivers/gpu/drm/arm/malidp_drv.c | 61 +++++++++++++++++++++++++++++++++++++++ > >drivers/gpu/drm/arm/malidp_drv.h | 15 ++++++++++ > >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, 122 insertions(+), 7 deletions(-) > > > >diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > >index 8d20faa..70ce19a 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> > >@@ -29,6 +30,7 @@ > >#include <drm/drm_gem_framebuffer_helper.h> > >#include <drm/drm_modeset_helper.h> > >#include <drm/drm_of.h> > >+#include <drm/drm_debugfs.h> > > > >#include "malidp_drv.h" > >#include "malidp_regs.h" > >@@ -327,6 +329,62 @@ 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) > >+{ > >+ atomic_set(&error_stats->num_errors, 0); > >+ atomic_set(&error_stats->last_error_status, 0); > >+ atomic64_set(&error_stats->last_error_vblank, -1); > >+} > >+ > >+void malidp_error(struct malidp_error_stats *error_stats, u32 status, > >+ u64 vblank) > >+{ > >+ atomic_set(&error_stats->last_error_status, status); > >+ atomic64_set(&error_stats->last_error_vblank, vblank); > >+ atomic_inc(&error_stats->num_errors); > >+} > >+ > > Do we already have a lock we could use to make sure the status printed > is consistent? I know this is "only debug", but it could be annoying > if we can't trust that the last_error_status does actually match the > last_error_vblank. > No, we don't have any lock. Yes that would be annoying, I will add one. > >+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, > >+ atomic_read(&error_stats->num_errors)); > >+ seq_printf(m, "[%s] last_error_status : 0x%08x\n", prefix, > >+ atomic_read(&error_stats->last_error_status)); > >+ seq_printf(m, "[%s] last_error_vblank : %ld\n", prefix, > >+ atomic64_read(&error_stats->last_error_vblank)); > >+} > >+ > >+static int malidp_show_stats(struct seq_file *m, void *arg) > >+{ > >+ struct drm_info_node *node = (struct drm_info_node *)m->private; > >+ struct drm_device *drm = node->minor->dev; > >+ struct malidp_drm *malidp = drm->dev_private; > >+ > >+ malidp_error_stats_dump("DE", &malidp->de_errors, m); > >+ malidp_error_stats_dump("SE", &malidp->se_errors, m); > >+ return 0; > >+} > >+ > >+static struct drm_info_list malidp_debugfs_list[] = { > >+ { "debug", malidp_show_stats, 0 }, > >+}; > > I think it would be pretty useful to have a way to reset the counters. > Maybe by just writing anything to the file? > drm_debugfs_fops doesn't allow writing. I see no reason using our own debugs_fops, but do you think worth the trouble? > Thanks, > -Brian > > >+ > >+static int malidp_debugfs_init(struct drm_minor *minor) > >+{ > >+ struct malidp_drm *malidp = minor->dev->dev_private; > >+ > >+ malidp_error_stats_init(&malidp->de_errors); > >+ malidp_error_stats_init(&malidp->se_errors); > >+ return drm_debugfs_create_files(malidp_debugfs_list, > >+ ARRAY_SIZE(malidp_debugfs_list), minor->debugfs_root, minor); > >+} > >+ > >+#endif //CONFIG_DEBUG_FS > >+ > >static struct drm_driver malidp_driver = { > > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | > > DRIVER_PRIME, > >@@ -343,6 +401,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..c49056c 100644 > >--- a/drivers/gpu/drm/arm/malidp_drv.h > >+++ b/drivers/gpu/drm/arm/malidp_drv.h > >@@ -18,6 +18,12 @@ > >#include <drm/drmP.h> > >#include "malidp_hw.h" > > > >+struct malidp_error_stats { > >+ atomic_t num_errors; > >+ atomic_t last_error_status; > >+ atomic64_t last_error_vblank; > >+}; > >+ > >struct malidp_drm { > > struct malidp_hw_device *dev; > > struct drm_crtc crtc; > >@@ -25,6 +31,10 @@ 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; > >+#endif > >}; > > > >#define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc) > >@@ -62,6 +72,11 @@ 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_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..ec40a44 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->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->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 > >
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 8d20faa..70ce19a 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> @@ -29,6 +30,7 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_modeset_helper.h> #include <drm/drm_of.h> +#include <drm/drm_debugfs.h> #include "malidp_drv.h" #include "malidp_regs.h" @@ -327,6 +329,62 @@ 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) +{ + atomic_set(&error_stats->num_errors, 0); + atomic_set(&error_stats->last_error_status, 0); + atomic64_set(&error_stats->last_error_vblank, -1); +} + +void malidp_error(struct malidp_error_stats *error_stats, u32 status, + u64 vblank) +{ + atomic_set(&error_stats->last_error_status, status); + atomic64_set(&error_stats->last_error_vblank, vblank); + atomic_inc(&error_stats->num_errors); +} + +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, + atomic_read(&error_stats->num_errors)); + seq_printf(m, "[%s] last_error_status : 0x%08x\n", prefix, + atomic_read(&error_stats->last_error_status)); + seq_printf(m, "[%s] last_error_vblank : %ld\n", prefix, + atomic64_read(&error_stats->last_error_vblank)); +} + +static int malidp_show_stats(struct seq_file *m, void *arg) +{ + struct drm_info_node *node = (struct drm_info_node *)m->private; + struct drm_device *drm = node->minor->dev; + struct malidp_drm *malidp = drm->dev_private; + + malidp_error_stats_dump("DE", &malidp->de_errors, m); + malidp_error_stats_dump("SE", &malidp->se_errors, m); + return 0; +} + +static struct drm_info_list malidp_debugfs_list[] = { + { "debug", malidp_show_stats, 0 }, +}; + +static int malidp_debugfs_init(struct drm_minor *minor) +{ + struct malidp_drm *malidp = minor->dev->dev_private; + + malidp_error_stats_init(&malidp->de_errors); + malidp_error_stats_init(&malidp->se_errors); + return drm_debugfs_create_files(malidp_debugfs_list, + ARRAY_SIZE(malidp_debugfs_list), minor->debugfs_root, minor); +} + +#endif //CONFIG_DEBUG_FS + static struct drm_driver malidp_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_PRIME, @@ -343,6 +401,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..c49056c 100644 --- a/drivers/gpu/drm/arm/malidp_drv.h +++ b/drivers/gpu/drm/arm/malidp_drv.h @@ -18,6 +18,12 @@ #include <drm/drmP.h> #include "malidp_hw.h" +struct malidp_error_stats { + atomic_t num_errors; + atomic_t last_error_status; + atomic64_t last_error_vblank; +}; + struct malidp_drm { struct malidp_hw_device *dev; struct drm_crtc crtc; @@ -25,6 +31,10 @@ 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; +#endif }; #define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc) @@ -62,6 +72,11 @@ 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_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..ec40a44 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->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->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)
Status register contains a lot of bits for reporting internal errors inside Mali DP. Currently, we just silently ignore all of the erorrs, 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 file called debug that contains an agregate of the errors reported by the Mali DP hardware. 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 This a morphosis of the patch presented here [1], where the errors where reported continuously via trace_printk. [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167042.html Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> --- drivers/gpu/drm/arm/malidp_drv.c | 61 +++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/arm/malidp_drv.h | 15 ++++++++++ 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, 122 insertions(+), 7 deletions(-)