On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergström wrote:
On 05.12.2012 13:13, Thierry Reding wrote:
[...]
Oh well, at the time nobody from NVIDIA was involved so I wrote that code in preparation for proper host1x support that I thought I would have to add myself at some point. I'm more than glad that I don't have to do this all by myself. However the patch proposed in this series breaks a number of requirements such as proper encapsulation, which I already mentioned in more detail in another mail.
Hmm, I'm not sure if I remember that you refer to by the proper encapsulation. Is that the fact that we bind DRM to a sub-client?
Yes, but there's more. For instance I went to great lengths to make sure there's no global data whatsoever. So all the data is bound to the host1x device in the current code. I know many other drivers like to take a shortcut and just put these things into global variables but I didn't want to.
The problem that this solves is that the DRM driver needs to be bound to a specific platform device. None of the DRM subdevices are suitable because they are only part of the whole DRM device. I think that host1x is the only device that fits here.
Note that this is only an administrative problem. It shouldn't interfere with the way host1x works. The goal is that the DRM device is registered at the proper hierarchical location.
The circular dependency is indeed a problem, though. Quite frankly I have no idea how to solve this. However the approach taken in the current patch will break several other requirements as I already explained.
The problem with doing drm_platform_init() with host1x device as parameter is that drm_get_platform_dev() will take control of drvdata. We'd need to put host1x specific struct host1x pointer to some other place and I'm not sure what that place could be.
Not anymore. I submitted a patch so that it no longer does that. The patch was merged about a month and a half ago.
You're right in that binding to a sub-device is not a nice way. DRM framework just needs a "struct device" to bind to. exynos seems to solve this by introducing a virtual device and bind to that. I'm not sure if this is the best way, but worth considering?
That was discussed a few months back already and nobody seemed to like the idea. In fact it was as a result of that discussion that Stephen brought up the idea to register the DRM driver from a central host1x driver (it may also have been part of a discussion on IRC, I don't remember exactly).
At the time I spent some time on a patch that introduced drm_soc_init() to solve this by creating a dummy struct device and registering the driver on top of that. But I abandoned it in favour of fixing the DRM platform support code. The approach also didn't provide for the proper encapsulation.
Thierry