Re: [PATCH v4 1/4] drm/vc4: Report HVS underrun errors

From: Eric Anholt
Date: Wed Feb 06 2019 - 18:51:39 EST

Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> writes:

> From: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
>
> Add a debugfs entry and helper for reporting HVS underrun errors as
> well as helpers for masking and unmasking the underrun interrupts.
> Add an IRQ handler and initial IRQ configuration.
> Rework related register definitions to take the channel number.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/vc4/vc4_debugfs.c | 1 +
> drivers/gpu/drm/vc4/vc4_drv.h | 10 ++++
> drivers/gpu/drm/vc4/vc4_hvs.c | 92 +++++++++++++++++++++++++++++++
> drivers/gpu/drm/vc4/vc4_kms.c | 4 ++
> drivers/gpu/drm/vc4/vc4_regs.h | 49 +++++-----------
> 5 files changed, 121 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
> index 7a0003de71ab..64021bcba041 100644
> --- a/drivers/gpu/drm/vc4/vc4_debugfs.c
> +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
> @@ -23,6 +23,7 @@ static const struct drm_info_list vc4_debugfs_list[] = {
> {"vec_regs", vc4_vec_debugfs_regs, 0},
> {"txp_regs", vc4_txp_debugfs_regs, 0},
> {"hvs_regs", vc4_hvs_debugfs_regs, 0},
> + {"hvs_underrun", vc4_hvs_debugfs_underrun, 0},
> {"crtc0_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)0},
> {"crtc1_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)1},
> {"crtc2_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)2},
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 2c635f001c71..b34da7b6ee6b 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -185,6 +185,13 @@ struct vc4_dev {
> /* Bitmask of the current bin_alloc used for overflow memory. */
> uint32_t bin_alloc_overflow;
>
> + /* Incremented when an underrun error happened after an atomic commit.
> + * This is particularly useful to detect when a specific modeset is too
> + * demanding in term of memory or HVS bandwidth which is hard to guess
> + * at atomic check time.
> + */
> + atomic_t underrun;
> +
> struct work_struct overflow_mem_work;
>
> int power_refcount;
> @@ -773,6 +780,9 @@ void vc4_irq_reset(struct drm_device *dev);
> extern struct platform_driver vc4_hvs_driver;
> void vc4_hvs_dump_state(struct drm_device *dev);
> int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused);
> +int vc4_hvs_debugfs_underrun(struct seq_file *m, void *unused);
> +void vc4_hvs_unmask_underrun(struct drm_device *dev);
> +void vc4_hvs_mask_underrun(struct drm_device *dev);
>
> /* vc4_kms.c */
> int vc4_kms_load(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 5d8c749c9749..d5bc3bcd3e51 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -22,6 +22,7 @@
> * each CRTC.
> */
>
> +#include <drm/drm_atomic_helper.h>
> #include <linux/component.h>
> #include "vc4_drv.h"
> #include "vc4_regs.h"
> @@ -102,6 +103,18 @@ int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused)
>
> return 0;
> }
> +
> +int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data)
> +{
> + struct drm_info_node *node = m->private;
> + struct drm_device *dev = node->minor->dev;
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> + struct drm_printer p = drm_seq_file_printer(m);
> +
> + drm_printf(&p, "%d\n", atomic_read(&vc4->underrun));
> +
> + return 0;
> +}
> #endif
>
> /* The filter kernel is composed of dwords each containing 3 9-bit
> @@ -166,6 +179,67 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
> return 0;
> }
>
> +void vc4_hvs_mask_underrun(struct drm_device *dev)
> +{
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> + u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> +
> + dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
> + SCALER_DISPCTRL_DSPEISLUR(1) |
> + SCALER_DISPCTRL_DSPEISLUR(2));
> +
> + HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> +}
> +
> +void vc4_hvs_unmask_underrun(struct drm_device *dev)
> +{
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> + u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> +
> + dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
> + SCALER_DISPCTRL_DSPEISLUR(1) |
> + SCALER_DISPCTRL_DSPEISLUR(2);
> +
> + HVS_WRITE(SCALER_DISPSTAT,
> + SCALER_DISPSTAT_EUFLOW(0) |
> + SCALER_DISPSTAT_EUFLOW(1) |
> + SCALER_DISPSTAT_EUFLOW(2));
> + HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> +}
> +
> +static void vc4_hvs_report_underrun(struct drm_device *dev)
> +{
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> +
> + atomic_inc(&vc4->underrun);
> + DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
> +}
> +
> +static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
> +{
> + struct drm_device *dev = data;
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> + irqreturn_t irqret = IRQ_NONE;
> + u32 status;
> +
> + status = HVS_READ(SCALER_DISPSTAT);
> +
> + if (status & (SCALER_DISPSTAT_EUFLOW(0) |
> + SCALER_DISPSTAT_EUFLOW(1) |
> + SCALER_DISPSTAT_EUFLOW(2))) {
> + vc4_hvs_mask_underrun(dev);
> + vc4_hvs_report_underrun(dev);
> +
> + irqret = IRQ_HANDLED;
> + }
> +
> + HVS_WRITE(SCALER_DISPSTAT, SCALER_DISPSTAT_IRQMASK(0) |
> + SCALER_DISPSTAT_IRQMASK(1) |
> + SCALER_DISPSTAT_IRQMASK(2));
> +
> + return irqret;
> +}
> +
> static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -219,15 +293,33 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> dispctrl = HVS_READ(SCALER_DISPCTRL);
>
> dispctrl |= SCALER_DISPCTRL_ENABLE;
> + dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) | SCALER_DISPCTRL_DISPEIRQ(0) |
> + SCALER_DISPCTRL_DSPEISLUR(1) | SCALER_DISPCTRL_DISPEIRQ(1) |
> + SCALER_DISPCTRL_DSPEISLUR(2) | SCALER_DISPCTRL_DISPEIRQ(2);
>
> /* Set DSP3 (PV1) to use HVS channel 2, which would otherwise
> * be unused.
> */
> dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> + dispctrl &= ~(SCALER_DISPCTRL_DMAEIRQ |
> + SCALER_DISPCTRL_SLVWREIRQ |
> + SCALER_DISPCTRL_SLVRDEIRQ |
> + SCALER_DISPCTRL_DSPEIEOF(0) |
> + SCALER_DISPCTRL_DSPEIEOF(1) |
> + SCALER_DISPCTRL_DSPEIEOF(2) |
> + SCALER_DISPCTRL_DSPEIEOLN(0) |
> + SCALER_DISPCTRL_DSPEIEOLN(1) |
> + SCALER_DISPCTRL_DSPEIEOLN(2) |
> + SCALER_DISPCTRL_SCLEIRQ);
> dispctrl |= VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
>
> HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>
> + ret = devm_request_irq(dev, platform_get_irq(pdev, 0),
> + vc4_hvs_irq_handler, 0, "vc4 hvs", drm);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 91b8c72ff361..3c87fbcd0b17 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -139,6 +139,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
> struct drm_device *dev = state->dev;
> struct vc4_dev *vc4 = to_vc4_dev(dev);
>
> + vc4_hvs_mask_underrun(dev);
> +
> drm_atomic_helper_wait_for_fences(dev, state, false);
>
> drm_atomic_helper_wait_for_dependencies(state);
> @@ -155,6 +157,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>
> drm_atomic_helper_commit_hw_done(state);
>
> + vc4_hvs_unmask_underrun(dev);
> +
> drm_atomic_helper_wait_for_flip_done(dev, state);
>
> drm_atomic_helper_cleanup_planes(dev, state);

I think the mask/unmask in here could race against another atomic_commit
happening on another CRTC in parallel. I guess maybe we should mask off
interrupts on the specific channel being modified.

However, given that we're just talking about a debugfs entry and
user-space testing tool, I'm fine with this as-is.

Other than my concern for patch #4, patch 1-3 are:

Reviewed-by: Eric Anholt <eric@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature