Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]