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

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

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.

-- 
Regards,

Laurent Pinchart