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

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 8167 bytes --]

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> From: Boris Brezillon <boris.brezillon@bootlin.com>
>
> 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@bootlin.com>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  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@anholt.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]