Re: [RFC PATCH v2] media: renesas: vsp1: Add VSPD underrun detection & tracing

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Eugeniu,

Thank you for the patch, and sorry for the delay.

On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote:
> A barely noticeable (especially if hardly reproducible) display flicker
> may not be the biggest concern in the development environment. However,
> an automotive OEM will not only notice it, but will also be haunted by
> its phenomenon/nature till it is understood in the greatest detail and
> ultimately eradicated, to avoid impairing user experience.
> 
> Troubleshooting the above without the right tools becomes a nightmare.

Having spent lots of time working in userspace recently, I can't agree
more.

> Since VSPD underruns may indeed cause [1] display flicker, we believe
> that having a minimal/lightweight support for detecting and logging
> such events would be extremely beneficial. Obviously, this only applies
> to VSP2 modules having an interface to DU (i.e. not mem2mem).
> 
> This implementation is heavily inspired by Koji Matsuoka's work [2-3],
> but has been refactored to hopefully become production/mainline-friendly
> (the original feature is intended for the development environment only).
> 
> [1] https://lore.kernel.org/linux-renesas-soc/20220421161259.GA2660@lxhi-065
> [2] https://github.com/renesas-rcar/linux-bsp/commit/3469001c3098
>     ("v4l: vsp1: Add underrun hung-up workaround")
> [3] https://github.com/renesas-rcar/linux-bsp/commit/12ea79975a10
>     ("v4l: vsp1: Add underrun debug messege option")
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Cc: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> 
> v2:
>  - Limited the functionality to R-Car Gen3 only, since there is no
>    evidence/reports of similar underruns on Gen2 and since the original
>    implementation from Matsuoka-san was limited to Gen3 as well. The Gen2
>    functionality should stay unaltered after this patch.
> 
>  - Used the DTS full node name (e.g. vsp@fea20000, vsp@fea28000, etc) as
>    criteria for computing the VSPD index (vspd_id). This keeps the VSPD
>    index in sync with R-Car Gen3 HW SoC manual and prevents the VSPD
>    index going haywire when the VSPD devices undergo bind/unbind
>    (credits to Michael Rodin for the finding). Using the DTS full name
>    is inline with recent implementations from Morimoto-san, e.g. in v5.0
>    commit beed78aeeb1021 ("ASoC: rsnd: move HDMI information from ssi.c
>    to core.c"), hence should be mainline-friendly/compliant.
> 
>  - The patch has been tested on M3-ES3.0-Salvator-XS. The underruns have
>    been generated by simply plugging and unplugging the HDMI cable (4
>    times) from/to HDMI0_OUT port. The results are visible below. The
>    pace of underrun interrupts is very low (1 IRQ per HDMI connect).
>    No risk is foreseen for UND IRQ storms or for printk pollute.
> 
>    [   90.679901] vsp1 fea28000.vsp: Underrun 1 at VSPD1 LIF0
>    [  259.570720] vsp1 fea28000.vsp: Underrun 2 at VSPD1 LIF0
>    [  312.944974] vsp1 fea28000.vsp: Underrun 3 at VSPD1 LIF0
>    [  338.844490] vsp1 fea28000.vsp: Underrun 4 at VSPD1 LIF0
> 
>    root@rcar-gen3:~# cat /sys/module/vsp1/parameters/vspd_underrun
>    0,4,0,0
> 
> v1:
>  - VSPD_MAX_NUM (4) is based on Table 32.4 in R-Car3 User's HW Manual.
> 
>  - The 'vspd_underrun' array is not fully populated, since plenty of
>    SoCs have less than 4 VSPDs (3M/V3H 1 VSPD, H3N/D3/M3N/E3 2 VSPD).
>    However, it is arguably the necessary trade-off to avoid:
>     * unnecessarily complicated data structures
>     * unnecessarily complicated user interfaces
>     * kmemleak-prone dynamic allocation
> 
>  - The /sys/module/vsp1/parameters/vspd_underrun interface is chosen
>    to avoid custom sysfs/debugfs interfaces (e.g. certain users may
>    disable debugfs to achieve smallest possible image size).
> 
>    $ cat /sys/module/vsp1/parameters/vspd_underrun
>    0,0,0,0
> 
>  - The 'vspd_underrun' module parameter is chosen as RO, to prevent
>    users tampering with it and reporting inaccurate values to the
>    developers/maintainers.
> 
>    $ ls -al /sys/module/vsp1/parameters/vspd_underrun
>    -r--r--r-- 1 root root 4096 Apr 25 18:00 /sys/module/vsp1/parameters/vspd_underrun
> 
>  - The actual order of the 'vspd_underrun' elements reflects the VSPD
>    order in 'renesas,vsps = <&vspdX 0>, <&vspdY 0>, <&vspdZ 0>';
> 
>  - dev_warn_ratelimited is chosen to prevent any potential printk storms
>    (hence pollution of dmesg) from the interrupt context. If this does
>    not address the concerns fully, dev_warn_once() could be used with
>    guaranteed minimal impact (but also minimal logging unfortunately).
> 
>  - 100 chars per line since v5.7 commit
>    bdc48fa11e46f8 ("checkpatch/coding-style: deprecate 80-column warning")
> 
>  - Any comments hugely appreciated. If possible, I kindly ask for some
>    testing on R-Car Gen2 since I do not own any Gen2 boards.
>    Alternatively, the feature could be easily disabled for Gen2 VSPDs.
> ---
>  drivers/media/platform/renesas/vsp1/vsp1.h    |  1 +
>  .../media/platform/renesas/vsp1/vsp1_drv.c    | 31 ++++++++++++++++++-
>  .../media/platform/renesas/vsp1/vsp1_regs.h   |  2 ++
>  .../media/platform/renesas/vsp1/vsp1_wpf.c    |  7 +++--
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
> index 37cf33c7e6ca..df8154267e10 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1.h
> @@ -75,6 +75,7 @@ struct vsp1_device {
>  	struct device *dev;
>  	const struct vsp1_device_info *info;
>  	u32 version;
> +	int vspd_id;
>  
>  	void __iomem *mmio;
>  	struct rcar_fcp_device *fcp;
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> index 502c7d9d6890..d9ae8059463d 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> @@ -38,13 +38,19 @@
>  #include "vsp1_uif.h"
>  #include "vsp1_video.h"
>  
> +#define VSPD_MAX_NUM	4
> +
> +static int vspd_underrun[VSPD_MAX_NUM];
> +module_param_array(vspd_underrun, int, NULL, 0444);
> +MODULE_PARM_DESC(vspd_underrun, "VSPD underrun counter");

Module parameters are not meant to convey information back to userspace.
This should be done through either a debugfs file or a sysfs file. Given
the debugging nature of this feature, I'd recommend the former.

> +
>  /* -----------------------------------------------------------------------------
>   * Interrupt Handling
>   */
>  
>  static irqreturn_t vsp1_irq_handler(int irq, void *data)
>  {
> -	u32 mask = VI6_WPF_IRQ_STA_DFE | VI6_WPF_IRQ_STA_FRE;
> +	u32 mask = VI6_WPF_IRQ_STA_DFE | VI6_WPF_IRQ_STA_FRE | VI6_WPF_IRQ_STA_UND;
>  	struct vsp1_device *vsp1 = data;
>  	irqreturn_t ret = IRQ_NONE;
>  	unsigned int i;
> @@ -63,6 +69,17 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)
>  			vsp1_pipeline_frame_end(wpf->entity.pipe);
>  			ret = IRQ_HANDLED;
>  		}
> +
> +		if (status & VI6_WPF_IRQ_STA_UND) {
> +			int id = vsp1->vspd_id;
> +
> +			if (id >= 0 && id < VSPD_MAX_NUM) {
> +				++vspd_underrun[id];
> +				dev_warn_ratelimited(vsp1->dev, "Underrun %d at VSPD%d LIF%d\n",
> +						     vspd_underrun[id], id, i);
> +			}
> +			ret = IRQ_HANDLED;
> +		}
>  	}
>  
>  	return ret;
> @@ -900,6 +917,18 @@ static int vsp1_probe(struct platform_device *pdev)
>  		goto done;
>  	}
>  
> +	if (strstr(of_node_full_name(vsp1->dev->of_node), "vsp@fea20000")) {
> +		vsp1->vspd_id = 0;
> +	} else if (strstr(of_node_full_name(vsp1->dev->of_node), "vsp@fea28000")) {
> +		vsp1->vspd_id = 1;
> +	} else if (strstr(of_node_full_name(vsp1->dev->of_node), "vsp@fea30000")) {
> +		vsp1->vspd_id = 2;
> +	} else if (strstr(of_node_full_name(vsp1->dev->of_node), "vsp@fea38000")) {
> +		vsp1->vspd_id = 3;
> +	} else {
> +		vsp1->vspd_id = -1;
> +	}

You can drop the curly braces.

Usage of addresses will make this very SoC-specific. With debugfs you
can create one directory per VSP instance, which will scale better.

> +
>  done:
>  	if (ret) {
>  		pm_runtime_disable(&pdev->dev);
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> index fae7286eb01e..632c43bb4cbd 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> @@ -32,10 +32,12 @@
>  #define VI6_STATUS_SYS_ACT(n)		BIT((n) + 8)
>  
>  #define VI6_WPF_IRQ_ENB(n)		(0x0048 + (n) * 12)
> +#define VI6_WPF_IRQ_ENB_UNDE		BIT(16)
>  #define VI6_WPF_IRQ_ENB_DFEE		BIT(1)
>  #define VI6_WPF_IRQ_ENB_FREE		BIT(0)
>  
>  #define VI6_WPF_IRQ_STA(n)		(0x004c + (n) * 12)
> +#define VI6_WPF_IRQ_STA_UND		BIT(16)
>  #define VI6_WPF_IRQ_STA_DFE		BIT(1)
>  #define VI6_WPF_IRQ_STA_FRE		BIT(0)
>  
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> index 94e91d7bb56c..205a8e51f574 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> @@ -266,6 +266,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	unsigned int i;
>  	u32 outfmt = 0;
>  	u32 srcrpf = 0;
> +	u32 irq_enb = VI6_WPF_IRQ_ENB_DFEE;
>  	int ret;
>  
>  	sink_format = vsp1_entity_get_pad_format(&wpf->entity,
> @@ -340,9 +341,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
>  
>  	/* Enable interrupts. */
> +	if (vsp1->info->gen == 3)
> +		irq_enb |= VI6_WPF_IRQ_ENB_UNDE;
> +
>  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> -	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> -			   VI6_WPF_IRQ_ENB_DFEE);
> +	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index), irq_enb);
>  
>  	/*
>  	 * Configure writeback for display pipelines (the wpf writeback flag is

-- 
Regards,

Laurent Pinchart