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: 3720 bytes --]

On Wed, Dec 05, 2012 at 12:10:50PM +0200, Terje Bergström wrote:
> On 05.12.2012 10:33, Thierry Reding wrote:
> > I've been thinking about this some more and came to the conclusion that
> > since we will already have a tight coupling between host1x and tegra-drm
> > we may just as well keep the client registration code in host1x. The way
> > I imagine this to work would be to export a public API from tegra-drm,
> > say tegra_drm_init() and tegra_drm_exit(), that could be called in place
> > of drm_platform_init() and drm_platform_exit() in the current code.
> > 
> > tegra_drm_init() could then be passed the host1x platform device to bind
> > to. The only thing that would need to be done is move the fields in the
> > host1x structure specific to DRM into a separate structure. host1x would
> > have to export host1x_drm_init/exit() which the DRM can invoke to have
> > all DRM clients register to the DRM subsystem.
> > 
> > From a hierarchical point of view this makes sense, with host1x being
> > the parent of all DRM subdevices. It allows us to reuse the current code
> > from tegra-drm that has been tested and works properly even for module
> > unload/reload. We also get to keep the proper encapsulation and the
> > switch to the separate host1x driver will require a much smaller patch.
> > 
> > Does anybody see a disadvantage in this approach?
> 
> I'm a bit confused about the scope. You mention host1x several times,
> but I'm not sure if you mean the file drivers/gpu/drm/tegra/host1x.c or
> the host1x driver. So I might be babbling when I answer this. Could you
> please clarify that?

What I propose is to move the client registration code that is currently
in drivers/gpu/drm/tegra/host1x.c to the host1x driver, which may or may
not end up in drivers/gpu/host1x.

> host1x hardware access must be encapsulated in host1x driver
> (drivers/gpu/host1x if that's the location we prefer). As for the
> management of the list of DRM clients, we proposed the move to drm.c,
> because host1x hardware would anyway be controlled by a different
> driver. Having file called host1x.c in tegradrm didn't sound logical, as
> its not really controlling host1x, and its probe wouldn't be called.

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.

> If your proposal is that we'd move the management of the list of host1x
> devices from tegradrm to host1x driver, we'd have a tight circular
> dependency between two drivers and that's almost always a bad idea. So
> far all ideas have revolved around tegradrm calling host1x, and host1x
> calling a bit of DRM (for CMA, would be fixed in later version) but not
> host1x calling tegradrm.

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.

Thierry

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