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.
