| Message ID | 1651584010-10156-1-git-send-email-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [RFC,v2] media: renesas: vsp1: Add VSPD underrun detection & tracing | expand |
Dear Laurent, Dear Kieran, Dear community, On Di, Mai 03, 2022 at 03:20:10 +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. > > 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> > --- I apologize for another friendly reminder, but is there any chance to get a bit of attention from your side regarding this patch, which is intended to make our life easier in production? I hope the RFC tag does not convey the lack of confidence, importance and/or time spent for implementation and testing on our side. This v2 version fixed all review comments and issues which popped up during internal testing, so I am very hopeful and looking forward to your precious feedback. We have also made aware Renesas Japan that this patch is important to us, hoping that potentially there is another communication bridge between them and Renesas OSS community. Best regards, Eugeniu Rosca
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
Hello Laurent, On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > Troubleshooting the above without the right tools becomes a nightmare. > > Having spent lots of time working in userspace recently, I can't agree > more. Thanks for the feedback and for endorsing the utility of this patch. > > +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. It is a bit unfortunate that we have to go the debugFS route, since I recall at least one Customer in the past, who disabled the debugFS in the end product, since it was the only available means to meet the stringent automotive requirements (w.r.t. KNL binary size). Anybody who has no choice but to disable debugFS will consequently not be able to take advantage of this patch in the production/release software. If there is no alternative, then for sure I can go this way. However, before submitting PATCH v3, would you consider SYSFS viable too, if keeping the module param is totally unacceptable? I was hoping to keep the number of external dependencies to the bare minimum, hence the initial choice of module param. Looking forward to your final suggestion/preference. > > + 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. Fine. > > Usage of addresses will make this very SoC-specific. With debugfs you > can create one directory per VSP instance, which will scale better. Since VSP underruns are only relevant to the VSP devices with an interface to DU, we originally skipped any mem2mem VSP devices when exposing the underrun info to the user. Do I understand your feedback correctly that you would prefer to expose the mem2mem VSP devices along with the VSPD devices (even if the former will never experience a display underrun/flicker), for the sake of 1) skipping the address filtering in the C code and for the sake of 2) making the list of VSP instances in sysfs/debugFS less HW specific? > -- > Regards, > > Laurent Pinchart Thanks again for the feedback and bear with me if the PATCH v3 is coming a bit late due to the long-awaited vacation looming on the horizon. BR, Eugeniu.
Hi Eugeniu, On Tue, Jun 28, 2022 at 09:05:34PM +0200, Eugeniu Rosca wrote: > On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > > > Troubleshooting the above without the right tools becomes a nightmare. > > > > Having spent lots of time working in userspace recently, I can't agree > > more. > > Thanks for the feedback and for endorsing the utility of this patch. > > > > +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. > > It is a bit unfortunate that we have to go the debugFS route, since I > recall at least one Customer in the past, who disabled the debugFS in > the end product, since it was the only available means to meet the > stringent automotive requirements (w.r.t. KNL binary size). Anybody > who has no choice but to disable debugFS will consequently not be able > to take advantage of this patch in the production/release software. debugfs isn't meant to be enabled in production, so if you need a solution for production environment, it's not an option indeed. > If there is no alternative, then for sure I can go this way. > > However, before submitting PATCH v3, would you consider SYSFS viable > too, if keeping the module param is totally unacceptable? > > I was hoping to keep the number of external dependencies to the bare > minimum, hence the initial choice of module param. Looking forward to > your final suggestion/preference. sysfs would be my next recommendation. I don't think a Linux system can meaningfully run without sysfs, so it shouldn't be an issue dependency-wise. > > > + 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. > > Fine. > > > Usage of addresses will make this very SoC-specific. With debugfs you > > can create one directory per VSP instance, which will scale better. > > Since VSP underruns are only relevant to the VSP devices with an > interface to DU, we originally skipped any mem2mem VSP devices when > exposing the underrun info to the user. > > Do I understand your feedback correctly that you would prefer to expose > the mem2mem VSP devices along with the VSPD devices (even if the former > will never experience a display underrun/flicker), for the sake of > 1) skipping the address filtering in the C code and for the sake of > 2) making the list of VSP instances in sysfs/debugFS less HW specific? You don't have to expose this in every VSP device, but the addresses above are specific to the VSPD integration in particular R-Car SoCs. There could be other R-Car SoCs that have their VSPD at different addresses, either existing devices, or future devices. The whole point of describing the SoC integration in the device tree is to free drivers from having to hardcode addresses, which makes them more portable across different SoCs. I'd like to preserve that. Using sysfs will solve the issue. > > -- > > Regards, > > > > Laurent Pinchart > > Thanks again for the feedback and bear with me if the PATCH v3 is coming > a bit late due to the long-awaited vacation looming on the horizon. I'm the one who should apologize for delays, I'm trully ashamed by how long it took me to reply to your v2. Please rest assured that your work is truly appreciated.
Hi Laurent, On Tue, Jun 28, 2022 at 9:53 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tue, Jun 28, 2022 at 09:05:34PM +0200, Eugeniu Rosca wrote: > > On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > > > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > > > > > Troubleshooting the above without the right tools becomes a nightmare. > > > > > > Having spent lots of time working in userspace recently, I can't agree > > > more. > > > > Thanks for the feedback and for endorsing the utility of this patch. > > > > > > +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. > > > > It is a bit unfortunate that we have to go the debugFS route, since I > > recall at least one Customer in the past, who disabled the debugFS in > > the end product, since it was the only available means to meet the > > stringent automotive requirements (w.r.t. KNL binary size). Anybody > > who has no choice but to disable debugFS will consequently not be able > > to take advantage of this patch in the production/release software. > > debugfs isn't meant to be enabled in production, so if you need a > solution for production environment, it's not an option indeed. > > > If there is no alternative, then for sure I can go this way. > > > > However, before submitting PATCH v3, would you consider SYSFS viable > > too, if keeping the module param is totally unacceptable? > > > > I was hoping to keep the number of external dependencies to the bare > > minimum, hence the initial choice of module param. Looking forward to > > your final suggestion/preference. > > sysfs would be my next recommendation. I don't think a Linux system can > meaningfully run without sysfs, so it shouldn't be an issue > dependency-wise. Indeed, you can add a device attribute. But as that is not a debug feature, the attribute must be documented, and becomes ABI. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Tue, Jun 28, 2022 at 10:08:28PM +0200, Geert Uytterhoeven wrote: > On Tue, Jun 28, 2022 at 9:53 PM Laurent Pinchart wrote: > > On Tue, Jun 28, 2022 at 09:05:34PM +0200, Eugeniu Rosca wrote: > > > On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > > > > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > > > > > > > Troubleshooting the above without the right tools becomes a nightmare. > > > > > > > > Having spent lots of time working in userspace recently, I can't agree > > > > more. > > > > > > Thanks for the feedback and for endorsing the utility of this patch. > > > > > > > > +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. > > > > > > It is a bit unfortunate that we have to go the debugFS route, since I > > > recall at least one Customer in the past, who disabled the debugFS in > > > the end product, since it was the only available means to meet the > > > stringent automotive requirements (w.r.t. KNL binary size). Anybody > > > who has no choice but to disable debugFS will consequently not be able > > > to take advantage of this patch in the production/release software. > > > > debugfs isn't meant to be enabled in production, so if you need a > > solution for production environment, it's not an option indeed. > > > > > If there is no alternative, then for sure I can go this way. > > > > > > However, before submitting PATCH v3, would you consider SYSFS viable > > > too, if keeping the module param is totally unacceptable? > > > > > > I was hoping to keep the number of external dependencies to the bare > > > minimum, hence the initial choice of module param. Looking forward to > > > your final suggestion/preference. > > > > sysfs would be my next recommendation. I don't think a Linux system can > > meaningfully run without sysfs, so it shouldn't be an issue > > dependency-wise. > > Indeed, you can add a device attribute. > But as that is not a debug feature, the attribute must be documented, > and becomes ABI. Thanks for the comment, that's correct
Dear Laurent, dear Geert, On Di, Jun 28, 2022 at 11:11:51 +0300, Laurent Pinchart wrote: > On Tue, Jun 28, 2022 at 10:08:28PM +0200, Geert Uytterhoeven wrote: > > On Tue, Jun 28, 2022 at 9:53 PM Laurent Pinchart wrote: > > > On Tue, Jun 28, 2022 at 09:05:34PM +0200, Eugeniu Rosca wrote: > > > > On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > > > > > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > > > > > > > > > Troubleshooting the above without the right tools becomes a nightmare. > > > > > > > > > > Having spent lots of time working in userspace recently, I can't agree > > > > > more. > > > > > > > > Thanks for the feedback and for endorsing the utility of this patch. > > > > > > > > > > +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. > > > > > > > > It is a bit unfortunate that we have to go the debugFS route, since I > > > > recall at least one Customer in the past, who disabled the debugFS in > > > > the end product, since it was the only available means to meet the > > > > stringent automotive requirements (w.r.t. KNL binary size). Anybody > > > > who has no choice but to disable debugFS will consequently not be able > > > > to take advantage of this patch in the production/release software. > > > > > > debugfs isn't meant to be enabled in production, so if you need a > > > solution for production environment, it's not an option indeed. > > > > > > > If there is no alternative, then for sure I can go this way. > > > > > > > > However, before submitting PATCH v3, would you consider SYSFS viable > > > > too, if keeping the module param is totally unacceptable? > > > > > > > > I was hoping to keep the number of external dependencies to the bare > > > > minimum, hence the initial choice of module param. Looking forward to > > > > your final suggestion/preference. > > > > > > sysfs would be my next recommendation. I don't think a Linux system can > > > meaningfully run without sysfs, so it shouldn't be an issue > > > dependency-wise. > > > > Indeed, you can add a device attribute. > > But as that is not a debug feature, the attribute must be documented, > > and becomes ABI. > > Thanks for the comment, that's correct Thanks for the precious and insightful comments. I will try to get them resolved in PATCH v3, to the best of my ability. BR, Eugeniu
Hi all, Quoting Laurent Pinchart (2022-06-28 21:11:51) > Hi Geert, > > On Tue, Jun 28, 2022 at 10:08:28PM +0200, Geert Uytterhoeven wrote: > > On Tue, Jun 28, 2022 at 9:53 PM Laurent Pinchart wrote: > > > On Tue, Jun 28, 2022 at 09:05:34PM +0200, Eugeniu Rosca wrote: > > > > On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > > > > > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > > > > > > > > > Troubleshooting the above without the right tools becomes a nightmare. > > > > > > > > > > Having spent lots of time working in userspace recently, I can't agree > > > > > more. > > > > > > > > Thanks for the feedback and for endorsing the utility of this patch. > > > > > > > > > > +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. > > > > > > > > It is a bit unfortunate that we have to go the debugFS route, since I > > > > recall at least one Customer in the past, who disabled the debugFS in > > > > the end product, since it was the only available means to meet the > > > > stringent automotive requirements (w.r.t. KNL binary size). Anybody > > > > who has no choice but to disable debugFS will consequently not be able > > > > to take advantage of this patch in the production/release software. > > > > > > debugfs isn't meant to be enabled in production, so if you need a > > > solution for production environment, it's not an option indeed. I have an out of tree patch set that I've kept around since I started working on VSP1 that adds a debugfs entry for VSPd so that I can extract information/stats when debugging. Seems like I should probably have shared that in the past, so I'll do so now. https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/ Branch: kbingham/vsp1/debugfs I'll post to the linux-media mailing list too separately. Certainly for the patches I have, shouldn't be in sysfs they provide debug of registers and decoding and aren't expected to be an ABI. I also added an underrun interrupt warning, but I think your implementation keeping a count is valid too, though anytime I've seen an underrun - that's been the 'end' - and the whole device has stalled. Have you actually seen it occur, and continue? > > > > If there is no alternative, then for sure I can go this way. > > > > > > > > However, before submitting PATCH v3, would you consider SYSFS viable > > > > too, if keeping the module param is totally unacceptable? > > > > > > > > I was hoping to keep the number of external dependencies to the bare > > > > minimum, hence the initial choice of module param. Looking forward to > > > > your final suggestion/preference. > > > > > > sysfs would be my next recommendation. I don't think a Linux system can > > > meaningfully run without sysfs, so it shouldn't be an issue > > > dependency-wise. > > > > Indeed, you can add a device attribute. > > But as that is not a debug feature, the attribute must be documented, > > and becomes ABI. > > Thanks for the comment, that's correct If we end up with a sysfs interface, I might like to see other frame counters added too I think. And that's my only worry about using sysfs for this ... it would become the defacto place to add debug info, rather than 'debugfs'. Not a direct objection, but a worry. But perhaps exposing frame counters and basic device stats through sysfs is a win anyway. -- Kieran > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Wed, Jun 29, 2022 at 11:35:24AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2022-06-28 21:11:51) > > On Tue, Jun 28, 2022 at 10:08:28PM +0200, Geert Uytterhoeven wrote: > > > On Tue, Jun 28, 2022 at 9:53 PM Laurent Pinchart wrote: > > > > On Tue, Jun 28, 2022 at 09:05:34PM +0200, Eugeniu Rosca wrote: > > > > > On So, Jun 26, 2022 at 09:46:42 +0300, Laurent Pinchart wrote: > > > > > > On Tue, May 03, 2022 at 03:20:10PM +0200, Eugeniu Rosca wrote: > > > > > > > > > > > > > > Troubleshooting the above without the right tools becomes a nightmare. > > > > > > > > > > > > Having spent lots of time working in userspace recently, I can't agree > > > > > > more. > > > > > > > > > > Thanks for the feedback and for endorsing the utility of this patch. > > > > > > > > > > > > +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. > > > > > > > > > > It is a bit unfortunate that we have to go the debugFS route, since I > > > > > recall at least one Customer in the past, who disabled the debugFS in > > > > > the end product, since it was the only available means to meet the > > > > > stringent automotive requirements (w.r.t. KNL binary size). Anybody > > > > > who has no choice but to disable debugFS will consequently not be able > > > > > to take advantage of this patch in the production/release software. > > > > > > > > debugfs isn't meant to be enabled in production, so if you need a > > > > solution for production environment, it's not an option indeed. > > I have an out of tree patch set that I've kept around since I started > working on VSP1 that adds a debugfs entry for VSPd so that I can extract > information/stats when debugging. > > Seems like I should probably have shared that in the past, so I'll do so > now. > > https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/ > Branch: kbingham/vsp1/debugfs > > > I'll post to the linux-media mailing list too separately. > > Certainly for the patches I have, shouldn't be in sysfs they provide > debug of registers and decoding and aren't expected to be an ABI. > > I also added an underrun interrupt warning, but I think your > implementation keeping a count is valid too, though anytime I've seen an > underrun - that's been the 'end' - and the whole device has stalled. > > Have you actually seen it occur, and continue? > > > > > > > If there is no alternative, then for sure I can go this way. > > > > > > > > > > However, before submitting PATCH v3, would you consider SYSFS viable > > > > > too, if keeping the module param is totally unacceptable? > > > > > > > > > > I was hoping to keep the number of external dependencies to the bare > > > > > minimum, hence the initial choice of module param. Looking forward to > > > > > your final suggestion/preference. > > > > > > > > sysfs would be my next recommendation. I don't think a Linux system can > > > > meaningfully run without sysfs, so it shouldn't be an issue > > > > dependency-wise. > > > > > > Indeed, you can add a device attribute. > > > But as that is not a debug feature, the attribute must be documented, > > > and becomes ABI. > > > > Thanks for the comment, that's correct > > If we end up with a sysfs interface, I might like to see other frame > counters added too I think. And that's my only worry about using sysfs > for this ... it would become the defacto place to add debug info, rather > than 'debugfs'. > > Not a direct objection, but a worry. But perhaps exposing frame counters > and basic device stats through sysfs is a win anyway. I've been thinking some more about this. We should separate debugging features, which should be exposed through debugfs, from production monitoring features, which need a different API (and have a stable ABI). sysfs is an interesting option, but I'm wondering in this case if userspace would also need the ability to receive notifications in case of errors. This isn't something the sysfs provides, polling would be required in that case, which isn't ideal. Are there other standard device monitoring APIs that we could use ?
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"); + /* ----------------------------------------------------------------------------- * 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; + } + 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
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. 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(-)