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

[Thierry Reding]

On Mon, Dec 03, 2012 at 12:20:30PM -0700, Stephen Warren wrote:
> On 11/30/2012 03:38 AM, Thierry Reding wrote:
> > On Fri, Nov 30, 2012 at 10:56:39AM +0200, Terje BergstrÃm wrote:
> >> On 29.11.2012 13:47, Thierry Reding wrote:
> >>> On Thu, Nov 29, 2012 at 12:21:04PM +0200, Terje BergstrÃm
> >>> wrote:
> >>>> Tegra20 and Tegra30 are compatible, but future chips are not.
> >>>> I was hoping we would be ready in upstream kernel for future
> >>>> chips.
> >>> 
> >>> I think we should ignore that problem for now. Generally
> >>> planning for any possible combination of incompatibilities
> >>> leads to overgeneralized designs that require precisely these
> >>> kinds of indirections.
> >>> 
> >>> Once some documentation for Tegra 40 materializes we can start
> >>> thinking about how to encapsulate the incompatible code.
> >> 
> >> I think here our perspectives differ a lot. That is natural
> >> considering the company I work for and company you work for, so
> >> let's try to sync the perspective.
> >> 
> >> In my reality, whatever is in market is old news and I barely
> >> work on them anymore. Upstreaming activity is the exception. 90%
> >> of my time is spent dealing with future chips which I know cannot
> >> be handled without this split to logical and physical driver
> >> parts.
> >> 
> >> For you, Tegra2 and Tegra3 are the reality.
> > 
> > To be fair, Tegra2 and Tegra3 are the reality for *everyone*
> > *outside* NVIDIA.
> > 
> > It's great that you spend most of your time working with future
> > chips, but unless you submit the code for inclusion or review
> > nobody upstream needs to be concerned about the implications. Most
> > people don't have time to waste so we naturally try to keep the
> > maintenance burden to a minimum.
> > 
> > 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.
> 
> I think there's room for letting Terje's complete knowledge of future
> chips guide the design of the current code that's sent upstream.
> Certainly we shouldn't add a ton of unnecessary abstraction layers
> right now that aren't needed for Tegra20/30, but if there's some
> decision that doesn't affect the bloat, opaqueness, ... of the current
> code but one choice is better for future development without serious
> negatives for the current code, it's pretty reasonable to make that
> decision rather than the other.

The original point was that the current design stashes every function of
host1x into an ops structure and you have to go through those ops to get
at the functionality. I can understand the need to add an ops structure
to cope with incompatibilities between versions, but as you say there
should to be a reason for them being introduced. If such reasons exists,
then I think they at least warrant a comment somewhere.

Furthermore this is usually best handled by wrapping the ops accesses in
a public API, so that the ops structure can be hidden within the driver.
For example, submitting a job to a channel should have a public API such
as:

	int host1x_channel_submit(struct host1x_channel *channel,
				  struct host1x_job *job)
	{
		...
	}

An initial implementation would just add the code into this function. If
it turns out some future version requires special incantations to submit
a job, only then should we introduce an ops structure, with only the one
function:

	struct host1x_channel_ops {
		int (*submit)(struct host1x_channel *channel,
			      struct host1x_job *job);
	};

But since only the public API above has been used, access to the special
implementation can be hidden from the user. So the public function could
be modified in this way:

	int host1x_channel_submit(struct hostx1_channel *channel,
				  struct host1x_job *job)
	{
		if (channel->ops && channel->ops->submit)
			return channel->ops->submit(channel, job);

		...
	}

And then you have two choices: either you keep the code for previous
generations after the if block or you provide a separate ops structure
for older generations as well and handle them via the same code path.

One other thing that such a design can help with is refactoring common
code or parameterizing code. Maybe newer generations are not compatible
but can easily be made to work with existing code by introducing a
variable such as register stride or something.

What's really difficult to follow is if an ops structure is accessed via
some global macro. It also breaks encapsulation because you have a
global ops structure. That may even work fine for now, but it will break
once you have more than a single host1x in a system. I know this will
never happen, but all of a sudden it happens anyway and the code breaks.
Doing this right isn't very hard and it will lead to a better design and
is less likely to break at some point.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature