Re: [RFC v2 1/8] video: tegra: Add nvhost driver

[Thierry Reding]

On Sat, Dec 01, 2012 at 01:31:02PM +0200, Terje BergstrÃm wrote:
> On 30.11.2012 12:38, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > The above implies that when you submit code it shouldn't contain pieces
> > that prepare for possible future extensions which may or may not be
> > submitted (the exception being if such changes are part of a series
> > where subsequent patches actually use them). The outcome is that the
> > amount of cruft in the mainline kernel is kept to a minimum. And that's
> > a very good thing.
> 
> We're now talking about actually a separation of logical and physical
> driver. I can't see why that's a bad thing. Especially considering that
> it's standard practice in well written drivers. Let's try to find a
> technical clean solution instead of debating politics. The latter should
> never be part of Linux kernel reviews.

I don't know where you see politics in what I said. All I'm saying is
that we shouldn't be making things needlessly complex. In my experience
the technically cleanest solution is usually the one with the least
complexity.

> > Okay, so what you're sayingCan here is that a huge number of people haven't
> > been wise in using the preprocessor for register definitions all these
> > years. That's a pretty bold statement. Now I obviously haven't looked at
> > every single line in the kernel, but I have never come across this usage
> > for static inline functions used for this. So, to be honest, I don't
> > think this is really up for discussion. Of course if you come up with an
> > example where this is done in a similar way I could be persuaded
> > otherwise.
> 
> We must've talked about a bit different things. For pure register defs,
> I can accommodate changing to #defines. We'd lose the code coverage
> analysis, though, but if the parentheses are a make-or-break question to
> upstreaming, I can change.
> 
> I was thinking of definitions like this:
> 
> static inline u32 host1x_sync_cfpeek_ctrl_cfpeek_addr_f(u32 v)
> {
> 	return (v & 0x1ff) << 0;
> }
> 
> versus
> 
> #define host1x_sync_cfpeek_ctrl_cfpeek_addr_f(v) ((v) >> 16) & 0x3ff
> 
> Both of these produce the same machine code and have same usage, but the
> latter has type checking and code coverage analysis and the former is
> (in my eyes) clearer. In both of these cases the usage is like this:
> 
> writel(host1x_sync_cfpeek_ctrl_cfpeek_ena_f(1)
> 		| host1x_sync_cfpeek_ctrl_cfpeek_channr_f(chid)
> 		| host1x_sync_cfpeek_ctrl_cfpeek_addr_f(rd_ptr),
> 	m->sync_aperture + host1x_sync_cfpeek_ctrl_r());

Again there's no precedent for doing this with static inline functions.
You can do the same with macros. Type checking isn't an issue in these
cases since we're talking about bitfields for which no proper type
exists.

Two other things about the examples above: the definitions should be all
caps and it would be nice if they could be made a bit shorter.

> > But I don't see how that's relevant here. Let me quote what you said
> > originally:
> > 
> >> This is actually the only interface to read the max value to user space,
> >> which can be useful for doing some comparisons that take wrapping into
> >> account. But we could just add IOCTLs and remove the sysfs entries.
> > 
> > To me that sounded like it was only used for debugging purposes. If you
> > actually need to access this from a userspace driver then, as opposed to
> > what I said earlier, this should be handled by some IOCTL.
> 
> There's a use for production code to know both the max and min, but I
> think we can just scope that use out from this patch sest.
> 
> User space can use these two for checking if one of their fences has
> already passed by comparing if the current value is between min and
> fence, taking wrapping into account. In these cases user space can f.ex.
> leave a host1x wait out from a command stream.

But you already have extra code in the kernel to patch out expired sync-
points. Is it really worth the added effort to burden userspace with
this? If so I still think some kind of generic IOCTL to retrieve
information about a syncpoint would be better than a sysfs interface.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature