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 ? -- Regards, Laurent Pinchart
