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