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

[Terje Bergström]

On 15.01.2013 13:30, Thierry Reding wrote:
> Sorry for not getting back to you on this earlier. I just remembered
> this thread when I saw Terje's latest patch series.
> 
> I agree that having everything in one location will make things a lot
> easier, even if it means we have to add the tegra-drm driver to a new
> location. In the long run I think this will pay off, though.
> 
> That said, I see that Terje has chosen this approach in his latest
> series, so it's all good.

*whew* Thanks Thierry. I'm not entirely sure that drivers/gpu/host1x is
the correct location, though - host1x looks pretty lonely there. If
anybody has a strong opinion about the location, I'm willing to adjust.

Terje

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

[Thierry Reding]

On Fri, Jan 04, 2013 at 01:25:06PM -0700, Stephen Warren wrote:
> On 01/04/2013 03:09 AM, Terje Bergstr?m wrote:
> ...
> > I think we have now two ways to go forward with cons and pros:
> >  1) Keep host1x and tegra-drm as separate driver
> >+ Code almost done
> >- we need dummy device and dummy driver
> >- extra code and API when host1x creates dummy device and its passed
> > to tegra-drm
> 
> Just to play devil's advocate:
> 
> I suspect that's only a few lines of code.
> 
> >- tegra-drm device would need to be a child of host1x device. Having
> > virtual and real devices as host1x children sounds weird.
> 
> And I doubt that would cause problems.
> 
> >  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
> > and whatever other personalities we wish would also be subcomponents of
> > host1x. host1x calls tegra-drm directly to handle preparation for drm
> > initialization. As they're in the same module, circular dependency is ok.
> >+ Simpler conceptually (no dummy device/driver)
> >+ Less code
> >- Proposal doesn't yet exist
> 
> But that said, I agree this approach would be very reasonable; it seems
> to me that host1x really is the main HW behind a DRM driver or a V4L2
> driver or ... As such, it seems quite reasonable for a single struct
> device to exist that represents host1x, and for the driver for that
> device to register both a DRM and a V4L2 driver etc. The code could
> physically be organized into separate modules, and under different
> Kconfig options for configurability etc.
> 
> But either way, I'll let you (Thierry and Terje) work out which way to go.

Sorry for not getting back to you on this earlier. I just remembered
this thread when I saw Terje's latest patch series.

I agree that having everything in one location will make things a lot
easier, even if it means we have to add the tegra-drm driver to a new
location. In the long run I think this will pay off, though.

That said, I see that Terje has chosen this approach in his latest
series, so it's all good.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

On Fri, Jan 04, 2013 at 01:25:06PM -0700, Stephen Warren wrote:
 On 01/04/2013 03:09 AM, Terje Bergström wrote:
 ...
  I think we have now two ways to go forward with cons and pros:
   1) Keep host1x and tegra-drm as separate driver
 + Code almost done
 - we need dummy device and dummy driver
 - extra code and API when host1x creates dummy device and its passed
  to tegra-drm
 
 Just to play devil's advocate:
 
 I suspect that's only a few lines of code.
 
 - tegra-drm device would need to be a child of host1x device. Having
  virtual and real devices as host1x children sounds weird.
 
 And I doubt that would cause problems.
 
   2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
  and whatever other personalities we wish would also be subcomponents of
  host1x. host1x calls tegra-drm directly to handle preparation for drm
  initialization. As they're in the same module, circular dependency is ok.
 + Simpler conceptually (no dummy device/driver)
 + Less code
 - Proposal doesn't yet exist
 
 But that said, I agree this approach would be very reasonable; it seems
 to me that host1x really is the main HW behind a DRM driver or a V4L2
 driver or ... As such, it seems quite reasonable for a single struct
 device to exist that represents host1x, and for the driver for that
 device to register both a DRM and a V4L2 driver etc. The code could
 physically be organized into separate modules, and under different
 Kconfig options for configurability etc.
 
 But either way, I'll let you (Thierry and Terje) work out which way to go.

Sorry for not getting back to you on this earlier. I just remembered
this thread when I saw Terje's latest patch series.

I agree that having everything in one location will make things a lot
easier, even if it means we have to add the tegra-drm driver to a new
location. In the long run I think this will pay off, though.

That said, I see that Terje has chosen this approach in his latest
series, so it's all good.

Thierry


pgpefy1VrkHqU.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 04.01.2013 22:25, Stephen Warren wrote:
> On 01/04/2013 03:09 AM, Terje Bergstr?m wrote:
> ...
>> I think we have now two ways to go forward with cons and pros:
>>  1) Keep host1x and tegra-drm as separate driver
>>+ Code almost done
>>- we need dummy device and dummy driver
>>- extra code and API when host1x creates dummy device and its passed
>> to tegra-drm
> 
> Just to play devil's advocate:
> 
> I suspect that's only a few lines of code.

Yes, that's true. There's some overhead, but there's not too many actual
code lines.

>>- tegra-drm device would need to be a child of host1x device. Having
>> virtual and real devices as host1x children sounds weird.
> 
> And I doubt that would cause problems.

True. It could become a problem if the driver just assumed that all
host1x children are actual hardware, but we could avoid that.

>>  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
>> and whatever other personalities we wish would also be subcomponents of
>> host1x. host1x calls tegra-drm directly to handle preparation for drm
>> initialization. As they're in the same module, circular dependency is ok.
>>+ Simpler conceptually (no dummy device/driver)
>>+ Less code
>>- Proposal doesn't yet exist
> 
> But that said, I agree this approach would be very reasonable; it seems
> to me that host1x really is the main HW behind a DRM driver or a V4L2
> driver or ... As such, it seems quite reasonable for a single struct
> device to exist that represents host1x, and for the driver for that
> device to register both a DRM and a V4L2 driver etc. The code could
> physically be organized into separate modules, and under different
> Kconfig options for configurability etc.
> 
> But either way, I'll let you (Thierry and Terje) work out which way to go.

If we want separate modules, we'd need the dummy device & dummy driver
binding between them. We could also just put them in the same module.
It'd make DRM a requirement to host1x driver, but given the current
structure, I think that'd be reasonable. We could later make it more
configurable if needed. Does this now make tegra-drm and host1x too
dependent on each other? I'm not sure.

I also like the fact that we don't have to export APIs to include, but
we can (if we so choose) keep all tegra-drm-host1x-APIs in local header
files. As we have noted, the two drivers are tightly interconnected,
changing the APIs is easier if we can just work on the same directory
hierarchy.

Terje

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

[Stephen Warren]

On 01/07/2013 01:20 AM, Terje Bergstr?m wrote:
> On 04.01.2013 22:25, Stephen Warren wrote:
>> On 01/04/2013 03:09 AM, Terje Bergstr?m wrote:
>> ...
>>> I think we have now two ways to go forward with cons and pros:
>>>  1) Keep host1x and tegra-drm as separate driver
>>>+ Code almost done
>>>- we need dummy device and dummy driver
>>>- extra code and API when host1x creates dummy device and its passed
>>> to tegra-drm
...
>>>  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
>>> and whatever other personalities we wish would also be subcomponents of
>>> host1x. host1x calls tegra-drm directly to handle preparation for drm
>>> initialization. As they're in the same module, circular dependency is ok.
>>>+ Simpler conceptually (no dummy device/driver)
>>>+ Less code
>>>- Proposal doesn't yet exist
>>
>> But that said, I agree this approach would be very reasonable; it seems
>> to me that host1x really is the main HW behind a DRM driver or a V4L2
>> driver or ... As such, it seems quite reasonable for a single struct
>> device to exist that represents host1x, and for the driver for that
>> device to register both a DRM and a V4L2 driver etc. The code could
>> physically be organized into separate modules, and under different
>> Kconfig options for configurability etc.
>>
>> But either way, I'll let you (Thierry and Terje) work out which way to go.
> 
> If we want separate modules, we'd need the dummy device & dummy driver
> binding between them.

I haven't really thought it through, but I don't think so; I was
thinking separate modules more just to allow linking smaller chunks of
code at once rather than allowing optional functionality via loading (or
not) various modules. Hence, simple function calls between the
files/modules. Still, there may well be no need at all to split it into
separate modules.

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

[Terje Bergström]

On 04.01.2013 22:25, Stephen Warren wrote:
 On 01/04/2013 03:09 AM, Terje Bergström wrote:
 ...
 I think we have now two ways to go forward with cons and pros:
  1) Keep host1x and tegra-drm as separate driver
+ Code almost done
- we need dummy device and dummy driver
- extra code and API when host1x creates dummy device and its passed
 to tegra-drm
 
 Just to play devil's advocate:
 
 I suspect that's only a few lines of code.

Yes, that's true. There's some overhead, but there's not too many actual
code lines.

- tegra-drm device would need to be a child of host1x device. Having
 virtual and real devices as host1x children sounds weird.
 
 And I doubt that would cause problems.

True. It could become a problem if the driver just assumed that all
host1x children are actual hardware, but we could avoid that.

  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
 and whatever other personalities we wish would also be subcomponents of
 host1x. host1x calls tegra-drm directly to handle preparation for drm
 initialization. As they're in the same module, circular dependency is ok.
+ Simpler conceptually (no dummy device/driver)
+ Less code
- Proposal doesn't yet exist
 
 But that said, I agree this approach would be very reasonable; it seems
 to me that host1x really is the main HW behind a DRM driver or a V4L2
 driver or ... As such, it seems quite reasonable for a single struct
 device to exist that represents host1x, and for the driver for that
 device to register both a DRM and a V4L2 driver etc. The code could
 physically be organized into separate modules, and under different
 Kconfig options for configurability etc.
 
 But either way, I'll let you (Thierry and Terje) work out which way to go.

If we want separate modules, we'd need the dummy device  dummy driver
binding between them. We could also just put them in the same module.
It'd make DRM a requirement to host1x driver, but given the current
structure, I think that'd be reasonable. We could later make it more
configurable if needed. Does this now make tegra-drm and host1x too
dependent on each other? I'm not sure.

I also like the fact that we don't have to export APIs to include, but
we can (if we so choose) keep all tegra-drm-host1x-APIs in local header
files. As we have noted, the two drivers are tightly interconnected,
changing the APIs is easier if we can just work on the same directory
hierarchy.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 01/07/2013 01:20 AM, Terje Bergström wrote:
 On 04.01.2013 22:25, Stephen Warren wrote:
 On 01/04/2013 03:09 AM, Terje Bergström wrote:
 ...
 I think we have now two ways to go forward with cons and pros:
  1) Keep host1x and tegra-drm as separate driver
+ Code almost done
- we need dummy device and dummy driver
- extra code and API when host1x creates dummy device and its passed
 to tegra-drm
...
  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
 and whatever other personalities we wish would also be subcomponents of
 host1x. host1x calls tegra-drm directly to handle preparation for drm
 initialization. As they're in the same module, circular dependency is ok.
+ Simpler conceptually (no dummy device/driver)
+ Less code
- Proposal doesn't yet exist

 But that said, I agree this approach would be very reasonable; it seems
 to me that host1x really is the main HW behind a DRM driver or a V4L2
 driver or ... As such, it seems quite reasonable for a single struct
 device to exist that represents host1x, and for the driver for that
 device to register both a DRM and a V4L2 driver etc. The code could
 physically be organized into separate modules, and under different
 Kconfig options for configurability etc.

 But either way, I'll let you (Thierry and Terje) work out which way to go.
 
 If we want separate modules, we'd need the dummy device  dummy driver
 binding between them.

I haven't really thought it through, but I don't think so; I was
thinking separate modules more just to allow linking smaller chunks of
code at once rather than allowing optional functionality via loading (or
not) various modules. Hence, simple function calls between the
files/modules. Still, there may well be no need at all to split it into
separate modules.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 01/04/2013 03:09 AM, Terje Bergstr?m wrote:
...
> I think we have now two ways to go forward with cons and pros:
>  1) Keep host1x and tegra-drm as separate driver
>+ Code almost done
>- we need dummy device and dummy driver
>- extra code and API when host1x creates dummy device and its passed
> to tegra-drm

Just to play devil's advocate:

I suspect that's only a few lines of code.

>- tegra-drm device would need to be a child of host1x device. Having
> virtual and real devices as host1x children sounds weird.

And I doubt that would cause problems.

>  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
> and whatever other personalities we wish would also be subcomponents of
> host1x. host1x calls tegra-drm directly to handle preparation for drm
> initialization. As they're in the same module, circular dependency is ok.
>+ Simpler conceptually (no dummy device/driver)
>+ Less code
>- Proposal doesn't yet exist

But that said, I agree this approach would be very reasonable; it seems
to me that host1x really is the main HW behind a DRM driver or a V4L2
driver or ... As such, it seems quite reasonable for a single struct
device to exist that represents host1x, and for the driver for that
device to register both a DRM and a V4L2 driver etc. The code could
physically be organized into separate modules, and under different
Kconfig options for configurability etc.

But either way, I'll let you (Thierry and Terje) work out which way to go.

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

[Terje Bergström]

On 21.12.2012 23:19, Stephen Warren wrote:
> I see the situation more like:
> 
> * There's host1x hardware.
> 
> * There's a low-level driver just for host1x itself; the host1x driver.
> 
> * There's a high-level driver for the entire host1x complex of devices.
> That is tegradrm. There may be more high-level drivers in the future
> (e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x
> sub-devices liek tegradrm does).
> 
> * The presence of the host1x DT node logically implies that both the two
> drivers in the previous two points should be instantiated.
> 
> * Linux instantiates a single device per DT node.
> 
> * So, it's host1x's responsibility to instantiate the other device(s). I
> think it's reasonable for the host1x driver to know exactly what
> features the host1x HW complex supports; raw host1x access being one,
> and the higher level concept of a tegradrm being another. So, it's
> reasonable for host1x to trigger the instantiation of tegradrm.
> 
> * If DRM core didn't stomp on the device object's drvdata (or whichever
> field it is), I would recommend not creating a dummy device at all, but
> rather having the host1x driver directly implement multiple driver
> interfaces. There are many examples like this already in the kernel,
> e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc.

We had a phone call with Stephen yesterday, and I finally understood why
unbroken chain from DT to tegra-drm is important. The goals are to be
able to modprobe tegra-drm without ill effects, and to auto-load
tegra-drm module. I had been chasing a totally different goal.

Sorry about all the churn.

I think we have now two ways to go forward with cons and pros:
 1) Keep host1x and tegra-drm as separate driver
   + Code almost done
   - we need dummy device and dummy driver
   - extra code and API when host1x creates dummy device and its passed
to tegra-drm
   - tegra-drm device would need to be a child of host1x device. Having
virtual and real devices as host1x children sounds weird.

 2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
and whatever other personalities we wish would also be subcomponents of
host1x. host1x calls tegra-drm directly to handle preparation for drm
initialization. As they're in the same module, circular dependency is ok.
   + Simpler conceptually (no dummy device/driver)
   + Less code
   - Proposal doesn't yet exist

Thierry, what do you think? I'd prefer option 2, as that keeps things
simple and still fulfills the requirements. We probably would redo the
patch "Remove redundant host1x" to actually move drm under host1x, and
adds calls from host1x to parse_dt() directly. We'd probably leave the
code otherwise mostly as it is now, so we'll drop whatever modifications
we've done so far in my proposals. You can later pick up some things
from our proposals if you want, but it's then up to you.

We could also later think about generalizing some of the list management
to belong to host1x, but that'd be a follow-up and we can decide that later.

Terje

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

[Terje Bergström]

On 21.12.2012 23:19, Stephen Warren wrote:
 I see the situation more like:
 
 * There's host1x hardware.
 
 * There's a low-level driver just for host1x itself; the host1x driver.
 
 * There's a high-level driver for the entire host1x complex of devices.
 That is tegradrm. There may be more high-level drivers in the future
 (e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x
 sub-devices liek tegradrm does).
 
 * The presence of the host1x DT node logically implies that both the two
 drivers in the previous two points should be instantiated.
 
 * Linux instantiates a single device per DT node.
 
 * So, it's host1x's responsibility to instantiate the other device(s). I
 think it's reasonable for the host1x driver to know exactly what
 features the host1x HW complex supports; raw host1x access being one,
 and the higher level concept of a tegradrm being another. So, it's
 reasonable for host1x to trigger the instantiation of tegradrm.
 
 * If DRM core didn't stomp on the device object's drvdata (or whichever
 field it is), I would recommend not creating a dummy device at all, but
 rather having the host1x driver directly implement multiple driver
 interfaces. There are many examples like this already in the kernel,
 e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc.

We had a phone call with Stephen yesterday, and I finally understood why
unbroken chain from DT to tegra-drm is important. The goals are to be
able to modprobe tegra-drm without ill effects, and to auto-load
tegra-drm module. I had been chasing a totally different goal.

Sorry about all the churn.

I think we have now two ways to go forward with cons and pros:
 1) Keep host1x and tegra-drm as separate driver
   + Code almost done
   - we need dummy device and dummy driver
   - extra code and API when host1x creates dummy device and its passed
to tegra-drm
   - tegra-drm device would need to be a child of host1x device. Having
virtual and real devices as host1x children sounds weird.

 2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
and whatever other personalities we wish would also be subcomponents of
host1x. host1x calls tegra-drm directly to handle preparation for drm
initialization. As they're in the same module, circular dependency is ok.
   + Simpler conceptually (no dummy device/driver)
   + Less code
   - Proposal doesn't yet exist

Thierry, what do you think? I'd prefer option 2, as that keeps things
simple and still fulfills the requirements. We probably would redo the
patch Remove redundant host1x to actually move drm under host1x, and
adds calls from host1x to parse_dt() directly. We'd probably leave the
code otherwise mostly as it is now, so we'll drop whatever modifications
we've done so far in my proposals. You can later pick up some things
from our proposals if you want, but it's then up to you.

We could also later think about generalizing some of the list management
to belong to host1x, but that'd be a follow-up and we can decide that later.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 01/04/2013 03:09 AM, Terje Bergström wrote:
...
 I think we have now two ways to go forward with cons and pros:
  1) Keep host1x and tegra-drm as separate driver
+ Code almost done
- we need dummy device and dummy driver
- extra code and API when host1x creates dummy device and its passed
 to tegra-drm

Just to play devil's advocate:

I suspect that's only a few lines of code.

- tegra-drm device would need to be a child of host1x device. Having
 virtual and real devices as host1x children sounds weird.

And I doubt that would cause problems.

  2) Merge host1x and tegra-drm into one module. drm is a subcomponent,
 and whatever other personalities we wish would also be subcomponents of
 host1x. host1x calls tegra-drm directly to handle preparation for drm
 initialization. As they're in the same module, circular dependency is ok.
+ Simpler conceptually (no dummy device/driver)
+ Less code
- Proposal doesn't yet exist

But that said, I agree this approach would be very reasonable; it seems
to me that host1x really is the main HW behind a DRM driver or a V4L2
driver or ... As such, it seems quite reasonable for a single struct
device to exist that represents host1x, and for the driver for that
device to register both a DRM and a V4L2 driver etc. The code could
physically be organized into separate modules, and under different
Kconfig options for configurability etc.

But either way, I'll let you (Thierry and Terje) work out which way to go.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 21.12.2012 23:19, Stephen Warren wrote:
> * There's host1x hardware.
> 
> * There's a low-level driver just for host1x itself; the host1x driver.
> 
> * There's a high-level driver for the entire host1x complex of devices.
> That is tegradrm. There may be more high-level drivers in the future
> (e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x
> sub-devices liek tegradrm does).

tegradrm is a driver for a couple of the host1x clients, namely DC and
HDMI, and now adding 2D.

> * The presence of the host1x DT node logically implies that both the two
> drivers in the previous two points should be instantiated.
> 
> * Linux instantiates a single device per DT node.
> 
> * So, it's host1x's responsibility to instantiate the other device(s). I
> think it's reasonable for the host1x driver to know exactly what
> features the host1x HW complex supports; raw host1x access being one,
> and the higher level concept of a tegradrm being another. So, it's
> reasonable for host1x to trigger the instantiation of tegradrm.

tegradrm has drivers for each device that it controls, so tegradrm via
kernel device-driver model instantiates them. host1x driver doesn't need
to do that.

> * If DRM core didn't stomp on the device object's drvdata (or whichever
> field it is), I would recommend not creating a dummy device at all, but
> rather having the host1x driver directly implement multiple driver
> interfaces. There are many examples like this already in the kernel,
> e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc.

This is the architecture that would imply host1x instantiating
everything, and DRM being a subcomponent of host1x driver. But we didn't
choose to go there. We agreed to have tegradrm and host1x drivers separate.

Terje

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

[Stephen Warren]

On 12/21/2012 01:57 AM, Arto Merilainen wrote:
> On 12/20/2012 07:14 PM, Stephen Warren wrote:
>>
>> What's wrong with just having each device ask the host1x (its parent)
>> for a pointer to the (dummy) tegradrm device. That seems extremely
>>
> 
> We are talking about creating a dummy device because:
> - we need to give something for drm_platform_init(),
> - drm related data should be stored somewhere,

Yes to those too, I believe.

> - some data must be passed to all driver probe() functions. This is not
> hw-related data, but just some lists that are used to ensure that all
> drivers have been initialised before calling drm_platform_init().

I haven't really thought through /when/ host1x would create the dummy
device. I guess if tegradrm really must initialize after all the devices
that it uses (rather than using something like deferred probe) then the
above may be true.

> All these issues are purely tegra-drm related and solving them elsewhere
> (in host1x) would be just wrong! host1x would not even use the dummy
> device for anything so creating it there seems bizarre.

I see the situation more like:

* There's host1x hardware.

* There's a low-level driver just for host1x itself; the host1x driver.

* There's a high-level driver for the entire host1x complex of devices.
That is tegradrm. There may be more high-level drivers in the future
(e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x
sub-devices liek tegradrm does).

* The presence of the host1x DT node logically implies that both the two
drivers in the previous two points should be instantiated.

* Linux instantiates a single device per DT node.

* So, it's host1x's responsibility to instantiate the other device(s). I
think it's reasonable for the host1x driver to know exactly what
features the host1x HW complex supports; raw host1x access being one,
and the higher level concept of a tegradrm being another. So, it's
reasonable for host1x to trigger the instantiation of tegradrm.

* If DRM core didn't stomp on the device object's drvdata (or whichever
field it is), I would recommend not creating a dummy device at all, but
rather having the host1x driver directly implement multiple driver
interfaces. There are many examples like this already in the kernel,
e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc.

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

[Arto Merilainen]

On 12/20/2012 07:14 PM, Stephen Warren wrote:
>
> What's wrong with just having each device ask the host1x (its parent)
> for a pointer to the (dummy) tegradrm device. That seems extremely
>

We are talking about creating a dummy device because:
- we need to give something for drm_platform_init(),
- drm related data should be stored somewhere,
- some data must be passed to all driver probe() functions. This is not 
hw-related data, but just some lists that are used to ensure that all 
drivers have been initialised before calling drm_platform_init().

All these issues are purely tegra-drm related and solving them elsewhere 
(in host1x) would be just wrong! host1x would not even use the dummy 
device for anything so creating it there seems bizarre.

- Arto

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

[Terje Bergström]

On 21.12.2012 00:28, Stephen Warren wrote:
> On 12/20/2012 02:34 PM, Terje Bergstr?m wrote:
>> On 20.12.2012 22:30, Thierry Reding wrote:
>>> The problem with your proposed solution is that, even any of Stephen's
>>> valid objections aside, it won't work. Once the tegra-drm module is
>>> unloaded, the driver's data will be left in the current state and the
>>> link to the dummy device will be lost.
>>
>> The dummy device is created by tegradrm's module init, because it's used
> 
> No, the tegradrm driver object might be registered by tegradrm's module
> init, but the dummy tegradrm platform device would need to be
> created/registered by host1x's probe. Otherwise, the device would get
> created even if there was no host1x/... in the system (disabled for some
> reason, multi-SoC kernel, ...)

Oh. I was all the time thinking that dummy device needs to be created by
tegradrm, because it's only used by tegradrm.  But as we're mixing the
responsibilities, we might then just as well go all the way.

Terje

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

[Arto Merilainen]

On 12/20/2012 07:14 PM, Stephen Warren wrote:


What's wrong with just having each device ask the host1x (its parent)
for a pointer to the (dummy) tegradrm device. That seems extremely



We are talking about creating a dummy device because:
- we need to give something for drm_platform_init(),
- drm related data should be stored somewhere,
- some data must be passed to all driver probe() functions. This is not 
hw-related data, but just some lists that are used to ensure that all 
drivers have been initialised before calling drm_platform_init().


All these issues are purely tegra-drm related and solving them elsewhere 
(in host1x) would be just wrong! host1x would not even use the dummy 
device for anything so creating it there seems bizarre.


- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 12/21/2012 01:57 AM, Arto Merilainen wrote:
 On 12/20/2012 07:14 PM, Stephen Warren wrote:

 What's wrong with just having each device ask the host1x (its parent)
 for a pointer to the (dummy) tegradrm device. That seems extremely

 
 We are talking about creating a dummy device because:
 - we need to give something for drm_platform_init(),
 - drm related data should be stored somewhere,

Yes to those too, I believe.

 - some data must be passed to all driver probe() functions. This is not
 hw-related data, but just some lists that are used to ensure that all
 drivers have been initialised before calling drm_platform_init().

I haven't really thought through /when/ host1x would create the dummy
device. I guess if tegradrm really must initialize after all the devices
that it uses (rather than using something like deferred probe) then the
above may be true.

 All these issues are purely tegra-drm related and solving them elsewhere
 (in host1x) would be just wrong! host1x would not even use the dummy
 device for anything so creating it there seems bizarre.

I see the situation more like:

* There's host1x hardware.

* There's a low-level driver just for host1x itself; the host1x driver.

* There's a high-level driver for the entire host1x complex of devices.
That is tegradrm. There may be more high-level drivers in the future
(e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x
sub-devices liek tegradrm does).

* The presence of the host1x DT node logically implies that both the two
drivers in the previous two points should be instantiated.

* Linux instantiates a single device per DT node.

* So, it's host1x's responsibility to instantiate the other device(s). I
think it's reasonable for the host1x driver to know exactly what
features the host1x HW complex supports; raw host1x access being one,
and the higher level concept of a tegradrm being another. So, it's
reasonable for host1x to trigger the instantiation of tegradrm.

* If DRM core didn't stomp on the device object's drvdata (or whichever
field it is), I would recommend not creating a dummy device at all, but
rather having the host1x driver directly implement multiple driver
interfaces. There are many examples like this already in the kernel,
e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 20.12.2012 22:30, Thierry Reding wrote:
> The problem with your proposed solution is that, even any of Stephen's
> valid objections aside, it won't work. Once the tegra-drm module is
> unloaded, the driver's data will be left in the current state and the
> link to the dummy device will be lost.

The dummy device is created by tegradrm's module init, because it's used
only by tegradrm. When tegradrm is unloaded, all the tegradrm devices
and drivers are unloaded and freed, including the dummy one.

> I don't think waiting for things to fail is the right option here. If we
> can make out this problem now, then now is the time to fix it. No
> excuses.

Of course not. If we know of something that fails now, let's not do that.

> And quite frankly given how all the various host1x components are
> interlinked I think having a single function that gets the DRM dummy
> device for DRM-related clients isn't that bad.

I like clear responsibilities. tegradrm is responsible for the DRM
interface, and host1x driver is responsible for host1x. tegradrm owns
its data, and can pass its pointers to the functions that need it.

But fine, I still don't like it, but I'll add host1x_set_tegradrm() and
host1x_get_tegradrm(), which act as setter and getter for tegradrm's
global data. I still think it's a solution to a problem we don't have,
but we've wasted an order of magnitude more time debating it than it
takes to implement.

Terje

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

[Thierry Reding]

On Thu, Dec 20, 2012 at 11:34:26PM +0200, Terje Bergstr?m wrote:
> On 20.12.2012 22:30, Thierry Reding wrote:
> > The problem with your proposed solution is that, even any of Stephen's
> > valid objections aside, it won't work. Once the tegra-drm module is
> > unloaded, the driver's data will be left in the current state and the
> > link to the dummy device will be lost.
> 
> The dummy device is created by tegradrm's module init, because it's used
> only by tegradrm. When tegradrm is unloaded, all the tegradrm devices
> and drivers are unloaded and freed, including the dummy one.

No, the children platform devices of host1x are not freed, so their
driver data will still contain the data set by the previous call to
their driver's .probe() function.

But reading what you said again, you were proposing to set the children
driver data from the tegra-drm dummy device's .probe(). In that case it
would probably work.

> > And quite frankly given how all the various host1x components are
> > interlinked I think having a single function that gets the DRM dummy
> > device for DRM-related clients isn't that bad.
> 
> I like clear responsibilities. tegradrm is responsible for the DRM
> interface, and host1x driver is responsible for host1x. tegradrm owns
> its data, and can pass its pointers to the functions that need it.
> 
> But fine, I still don't like it, but I'll add host1x_set_tegradrm() and
> host1x_get_tegradrm(), which act as setter and getter for tegradrm's
> global data. I still think it's a solution to a problem we don't have,
> but we've wasted an order of magnitude more time debating it than it
> takes to implement.

As Stephen already pointed out, since the host1x driver will already
instantiate the dummy device, there's no point in adding a function to
set the pointer to that device.

Adding a getter should be enough. And it should return a pointer to the
dummy platform devices .dev field. Calling it host1x_get_drm_device()
may make it more obvious about what it returns.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

On Thu, Dec 20, 2012 at 08:01:43PM +0200, Terje Bergstr?m wrote:
> On 20.12.2012 19:55, Stephen Warren wrote:
> > So it's fine for the tegradrm driver to manipulate the tegradrm
> > (virtual) device's drvdata pointer.
> > 
> > It's not fine for tegradrm to manipulate the drvdata pointer of other
> > devices; the host1x children. The drvdata pointer for other devices is
> > reserved for those devices' driver's use only.
> 
> They're all tegradrm drivers, so tegradrm can manipulate its drvdata.
> The other simple way is global pointer. Let's aim now for a simple
> solution, and if later we find out it causes problems, let's fix it then.

The problem with your proposed solution is that, even any of Stephen's
valid objections aside, it won't work. Once the tegra-drm module is
unloaded, the driver's data will be left in the current state and the
link to the dummy device will be lost.

I don't think waiting for things to fail is the right option here. If we
can make out this problem now, then now is the time to fix it. No
excuses.

And quite frankly given how all the various host1x components are
interlinked I think having a single function that gets the DRM dummy
device for DRM-related clients isn't that bad.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Terje Bergström]

On 20.12.2012 19:55, Stephen Warren wrote:
> So it's fine for the tegradrm driver to manipulate the tegradrm
> (virtual) device's drvdata pointer.
> 
> It's not fine for tegradrm to manipulate the drvdata pointer of other
> devices; the host1x children. The drvdata pointer for other devices is
> reserved for those devices' driver's use only.

They're all tegradrm drivers, so tegradrm can manipulate its drvdata.
The other simple way is global pointer. Let's aim now for a simple
solution, and if later we find out it causes problems, let's fix it then.

Terje

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

[Terje Bergström]

On 20.12.2012 19:14, Stephen Warren wrote:
> I'm not sure that sounds right. drvdata is something that a driver
> should manage itself.
> 
> What's wrong with just having each device ask the host1x (its parent)
> for a pointer to the (dummy) tegradrm device. That seems extremely
> simple, and doesn't require abusing existing stuff like drvdata for
> non-standard purposes.

This is tegradrm's own data, and it's tegradrm which accesses the
pointer. So it's entirely something that the driver takes care of itself.

It's simplest if tegradrm takes care of its own data. That way there's
no chance of confusion of ownership or lifecycle of the data. The only
places which needs this access are the probe functions, and adding an
API contract between two components just for these few calls sounds too
much. All code after that anyway access drm_device->dev_private to get
the pointer.

Terje

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

[Stephen Warren]

On 12/20/2012 02:50 PM, Thierry Reding wrote:
> On Thu, Dec 20, 2012 at 11:34:26PM +0200, Terje Bergstr?m wrote:
>> On 20.12.2012 22:30, Thierry Reding wrote:
>>> The problem with your proposed solution is that, even any of
>>> Stephen's valid objections aside, it won't work. Once the
>>> tegra-drm module is unloaded, the driver's data will be left in
>>> the current state and the link to the dummy device will be
>>> lost.
>> 
>> The dummy device is created by tegradrm's module init, because
>> it's used only by tegradrm. When tegradrm is unloaded, all the
>> tegradrm devices and drivers are unloaded and freed, including
>> the dummy one.
> 
> No, the children platform devices of host1x are not freed, so
> their driver data will still contain the data set by the previous
> call to their driver's .probe() function.
> 
> But reading what you said again, you were proposing to set the
> children driver data from the tegra-drm dummy device's .probe(). In
> that case it would probably work.

Many things would work, but since tegradrm isn't (or at least should
not be) the driver for the platform devices which are host1x's
children, it shouldn't be randomly screwing around with those devices'
drvdata fields.

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

[Stephen Warren]

On 12/20/2012 02:34 PM, Terje Bergstr?m wrote:
> On 20.12.2012 22:30, Thierry Reding wrote:
>> The problem with your proposed solution is that, even any of Stephen's
>> valid objections aside, it won't work. Once the tegra-drm module is
>> unloaded, the driver's data will be left in the current state and the
>> link to the dummy device will be lost.
> 
> The dummy device is created by tegradrm's module init, because it's used

No, the tegradrm driver object might be registered by tegradrm's module
init, but the dummy tegradrm platform device would need to be
created/registered by host1x's probe. Otherwise, the device would get
created even if there was no host1x/... in the system (disabled for some
reason, multi-SoC kernel, ...)

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

[Terje Bergström]

On 16.12.2012 14:16, Thierry Reding wrote:
> Okay, so we're back on the topic of using globals. I need to assert
> again that this is not an option. If we were to use globals, then we
> could just as well leave out the dummy device and just do all of that in
> the tegra-drm driver's initialization function.

I found a way of dropping the global in a straightforward way, and
introduce dummy device for drm_platform_init().

I added dummy device and driver, and moved the tegradrm global
(previously called struct host1x *host1x) allocation to happen in the
probe. In addition, probe calls device tree node traversal to do the
tegra_drm_add_client() calls. The dummy device is owner for this global.

I changed the device tree node traversal so that it goes actually
through each host1x child, checks if it's supported by tegradrm, and if
so, sets its drvdata to point to the tegradrm data.

Each probe will add the client with tegra_drm_register_client(), and
that will find the global via dev_get_drvdata(). In the end of probe,
each driver will replace the drvdata with its own data.

I am also setting the coherent_dma_mask for dummy device so that it can
be used with CMA FB helper.

Would this be ok for you? I could send a patchset with this implemented
by tomorrow and let it simmer for 2-3 weeks due to my and everybody
elses' holidays. I'm hoping we would have 2D acceleration in Linux
kernel 3.9.

Terje

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

[Stephen Warren]

On 12/20/2012 10:46 AM, Terje Bergstr?m wrote:
> On 20.12.2012 19:14, Stephen Warren wrote:
>> I'm not sure that sounds right. drvdata is something that a driver
>> should manage itself.
>>
>> What's wrong with just having each device ask the host1x (its parent)
>> for a pointer to the (dummy) tegradrm device. That seems extremely
>> simple, and doesn't require abusing existing stuff like drvdata for
>> non-standard purposes.
> 
> This is tegradrm's own data, and it's tegradrm which accesses the
> pointer. So it's entirely something that the driver takes care of itself.

So it's fine for the tegradrm driver to manipulate the tegradrm
(virtual) device's drvdata pointer.

It's not fine for tegradrm to manipulate the drvdata pointer of other
devices; the host1x children. The drvdata pointer for other devices is
reserved for those devices' driver's use only.

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

[Stephen Warren]

On 12/20/2012 02:17 AM, Terje Bergstr?m wrote:
> On 16.12.2012 14:16, Thierry Reding wrote:
>> Okay, so we're back on the topic of using globals. I need to assert
>> again that this is not an option. If we were to use globals, then we
>> could just as well leave out the dummy device and just do all of that in
>> the tegra-drm driver's initialization function.
> 
> I found a way of dropping the global in a straightforward way, and
> introduce dummy device for drm_platform_init().
> 
> I added dummy device and driver, and moved the tegradrm global
> (previously called struct host1x *host1x) allocation to happen in the
> probe. In addition, probe calls device tree node traversal to do the
> tegra_drm_add_client() calls. The dummy device is owner for this global.
> 
> I changed the device tree node traversal so that it goes actually
> through each host1x child, checks if it's supported by tegradrm, and if
> so, sets its drvdata to point to the tegradrm data.

I'm not sure that sounds right. drvdata is something that a driver
should manage itself.

What's wrong with just having each device ask the host1x (its parent)
for a pointer to the (dummy) tegradrm device. That seems extremely
simple, and doesn't require abusing existing stuff like drvdata for
non-standard purposes.

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

[Terje Bergström]

On 16.12.2012 14:16, Thierry Reding wrote:
 Okay, so we're back on the topic of using globals. I need to assert
 again that this is not an option. If we were to use globals, then we
 could just as well leave out the dummy device and just do all of that in
 the tegra-drm driver's initialization function.

I found a way of dropping the global in a straightforward way, and
introduce dummy device for drm_platform_init().

I added dummy device and driver, and moved the tegradrm global
(previously called struct host1x *host1x) allocation to happen in the
probe. In addition, probe calls device tree node traversal to do the
tegra_drm_add_client() calls. The dummy device is owner for this global.

I changed the device tree node traversal so that it goes actually
through each host1x child, checks if it's supported by tegradrm, and if
so, sets its drvdata to point to the tegradrm data.

Each probe will add the client with tegra_drm_register_client(), and
that will find the global via dev_get_drvdata(). In the end of probe,
each driver will replace the drvdata with its own data.

I am also setting the coherent_dma_mask for dummy device so that it can
be used with CMA FB helper.

Would this be ok for you? I could send a patchset with this implemented
by tomorrow and let it simmer for 2-3 weeks due to my and everybody
elses' holidays. I'm hoping we would have 2D acceleration in Linux
kernel 3.9.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 12/20/2012 02:17 AM, Terje Bergström wrote:
 On 16.12.2012 14:16, Thierry Reding wrote:
 Okay, so we're back on the topic of using globals. I need to assert
 again that this is not an option. If we were to use globals, then we
 could just as well leave out the dummy device and just do all of that in
 the tegra-drm driver's initialization function.
 
 I found a way of dropping the global in a straightforward way, and
 introduce dummy device for drm_platform_init().
 
 I added dummy device and driver, and moved the tegradrm global
 (previously called struct host1x *host1x) allocation to happen in the
 probe. In addition, probe calls device tree node traversal to do the
 tegra_drm_add_client() calls. The dummy device is owner for this global.
 
 I changed the device tree node traversal so that it goes actually
 through each host1x child, checks if it's supported by tegradrm, and if
 so, sets its drvdata to point to the tegradrm data.

I'm not sure that sounds right. drvdata is something that a driver
should manage itself.

What's wrong with just having each device ask the host1x (its parent)
for a pointer to the (dummy) tegradrm device. That seems extremely
simple, and doesn't require abusing existing stuff like drvdata for
non-standard purposes.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 20.12.2012 19:14, Stephen Warren wrote:
 I'm not sure that sounds right. drvdata is something that a driver
 should manage itself.
 
 What's wrong with just having each device ask the host1x (its parent)
 for a pointer to the (dummy) tegradrm device. That seems extremely
 simple, and doesn't require abusing existing stuff like drvdata for
 non-standard purposes.

This is tegradrm's own data, and it's tegradrm which accesses the
pointer. So it's entirely something that the driver takes care of itself.

It's simplest if tegradrm takes care of its own data. That way there's
no chance of confusion of ownership or lifecycle of the data. The only
places which needs this access are the probe functions, and adding an
API contract between two components just for these few calls sounds too
much. All code after that anyway access drm_device-dev_private to get
the pointer.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 20.12.2012 19:55, Stephen Warren wrote:
 So it's fine for the tegradrm driver to manipulate the tegradrm
 (virtual) device's drvdata pointer.
 
 It's not fine for tegradrm to manipulate the drvdata pointer of other
 devices; the host1x children. The drvdata pointer for other devices is
 reserved for those devices' driver's use only.

They're all tegradrm drivers, so tegradrm can manipulate its drvdata.
The other simple way is global pointer. Let's aim now for a simple
solution, and if later we find out it causes problems, let's fix it then.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Thu, Dec 20, 2012 at 08:01:43PM +0200, Terje Bergström wrote:
 On 20.12.2012 19:55, Stephen Warren wrote:
  So it's fine for the tegradrm driver to manipulate the tegradrm
  (virtual) device's drvdata pointer.
  
  It's not fine for tegradrm to manipulate the drvdata pointer of other
  devices; the host1x children. The drvdata pointer for other devices is
  reserved for those devices' driver's use only.
 
 They're all tegradrm drivers, so tegradrm can manipulate its drvdata.
 The other simple way is global pointer. Let's aim now for a simple
 solution, and if later we find out it causes problems, let's fix it then.

The problem with your proposed solution is that, even any of Stephen's
valid objections aside, it won't work. Once the tegra-drm module is
unloaded, the driver's data will be left in the current state and the
link to the dummy device will be lost.

I don't think waiting for things to fail is the right option here. If we
can make out this problem now, then now is the time to fix it. No
excuses.

And quite frankly given how all the various host1x components are
interlinked I think having a single function that gets the DRM dummy
device for DRM-related clients isn't that bad.

Thierry


pgpPMI9tt6ITN.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 20.12.2012 22:30, Thierry Reding wrote:
 The problem with your proposed solution is that, even any of Stephen's
 valid objections aside, it won't work. Once the tegra-drm module is
 unloaded, the driver's data will be left in the current state and the
 link to the dummy device will be lost.

The dummy device is created by tegradrm's module init, because it's used
only by tegradrm. When tegradrm is unloaded, all the tegradrm devices
and drivers are unloaded and freed, including the dummy one.

 I don't think waiting for things to fail is the right option here. If we
 can make out this problem now, then now is the time to fix it. No
 excuses.

Of course not. If we know of something that fails now, let's not do that.

 And quite frankly given how all the various host1x components are
 interlinked I think having a single function that gets the DRM dummy
 device for DRM-related clients isn't that bad.

I like clear responsibilities. tegradrm is responsible for the DRM
interface, and host1x driver is responsible for host1x. tegradrm owns
its data, and can pass its pointers to the functions that need it.

But fine, I still don't like it, but I'll add host1x_set_tegradrm() and
host1x_get_tegradrm(), which act as setter and getter for tegradrm's
global data. I still think it's a solution to a problem we don't have,
but we've wasted an order of magnitude more time debating it than it
takes to implement.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Thu, Dec 20, 2012 at 11:34:26PM +0200, Terje Bergström wrote:
 On 20.12.2012 22:30, Thierry Reding wrote:
  The problem with your proposed solution is that, even any of Stephen's
  valid objections aside, it won't work. Once the tegra-drm module is
  unloaded, the driver's data will be left in the current state and the
  link to the dummy device will be lost.
 
 The dummy device is created by tegradrm's module init, because it's used
 only by tegradrm. When tegradrm is unloaded, all the tegradrm devices
 and drivers are unloaded and freed, including the dummy one.

No, the children platform devices of host1x are not freed, so their
driver data will still contain the data set by the previous call to
their driver's .probe() function.

But reading what you said again, you were proposing to set the children
driver data from the tegra-drm dummy device's .probe(). In that case it
would probably work.

  And quite frankly given how all the various host1x components are
  interlinked I think having a single function that gets the DRM dummy
  device for DRM-related clients isn't that bad.
 
 I like clear responsibilities. tegradrm is responsible for the DRM
 interface, and host1x driver is responsible for host1x. tegradrm owns
 its data, and can pass its pointers to the functions that need it.
 
 But fine, I still don't like it, but I'll add host1x_set_tegradrm() and
 host1x_get_tegradrm(), which act as setter and getter for tegradrm's
 global data. I still think it's a solution to a problem we don't have,
 but we've wasted an order of magnitude more time debating it than it
 takes to implement.

As Stephen already pointed out, since the host1x driver will already
instantiate the dummy device, there's no point in adding a function to
set the pointer to that device.

Adding a getter should be enough. And it should return a pointer to the
dummy platform devices .dev field. Calling it host1x_get_drm_device()
may make it more obvious about what it returns.

Thierry


pgpQaT9J5lEyg.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 12/20/2012 02:34 PM, Terje Bergström wrote:
 On 20.12.2012 22:30, Thierry Reding wrote:
 The problem with your proposed solution is that, even any of Stephen's
 valid objections aside, it won't work. Once the tegra-drm module is
 unloaded, the driver's data will be left in the current state and the
 link to the dummy device will be lost.
 
 The dummy device is created by tegradrm's module init, because it's used

No, the tegradrm driver object might be registered by tegradrm's module
init, but the dummy tegradrm platform device would need to be
created/registered by host1x's probe. Otherwise, the device would get
created even if there was no host1x/... in the system (disabled for some
reason, multi-SoC kernel, ...)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 12/20/2012 02:50 PM, Thierry Reding wrote:
 On Thu, Dec 20, 2012 at 11:34:26PM +0200, Terje Bergström wrote:
 On 20.12.2012 22:30, Thierry Reding wrote:
 The problem with your proposed solution is that, even any of
 Stephen's valid objections aside, it won't work. Once the
 tegra-drm module is unloaded, the driver's data will be left in
 the current state and the link to the dummy device will be
 lost.
 
 The dummy device is created by tegradrm's module init, because
 it's used only by tegradrm. When tegradrm is unloaded, all the
 tegradrm devices and drivers are unloaded and freed, including
 the dummy one.
 
 No, the children platform devices of host1x are not freed, so
 their driver data will still contain the data set by the previous
 call to their driver's .probe() function.
 
 But reading what you said again, you were proposing to set the
 children driver data from the tegra-drm dummy device's .probe(). In
 that case it would probably work.

Many things would work, but since tegradrm isn't (or at least should
not be) the driver for the platform devices which are host1x's
children, it shouldn't be randomly screwing around with those devices'
drvdata fields.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 21.12.2012 00:28, Stephen Warren wrote:
 On 12/20/2012 02:34 PM, Terje Bergström wrote:
 On 20.12.2012 22:30, Thierry Reding wrote:
 The problem with your proposed solution is that, even any of Stephen's
 valid objections aside, it won't work. Once the tegra-drm module is
 unloaded, the driver's data will be left in the current state and the
 link to the dummy device will be lost.

 The dummy device is created by tegradrm's module init, because it's used
 
 No, the tegradrm driver object might be registered by tegradrm's module
 init, but the dummy tegradrm platform device would need to be
 created/registered by host1x's probe. Otherwise, the device would get
 created even if there was no host1x/... in the system (disabled for some
 reason, multi-SoC kernel, ...)

Oh. I was all the time thinking that dummy device needs to be created by
tegradrm, because it's only used by tegradrm.  But as we're mixing the
responsibilities, we might then just as well go all the way.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 17.12.2012 22:55, Stephen Warren wrote:
> On 12/16/2012 09:37 AM, Terje Bergstr?m wrote:
> ...
>> ... Sure we could tell DC to ask its parent
>> (host1x), and call host1x driver with platform_device pointer found that
>> way, and host1x would return a pointer to tegradrm's data. Hanging the
>> data onto host1x driver is just a more complicated way of implementing
>> global data
> 
> No it's not; at that point, the data is no longer global, but specific
> to the driver instance.

We have only one tegradrm, so the advantage is theoretical - the one
driver gets the same pointer in both cases.

If we use static pointer with an accessor function, we can keep the
solution contained to one source code file and the ownership of data is
clear - tegradrm allocates and deallocates the object and is the sole
user. Code is already in the patchset I sent.

Shared responsibility with host1x and tegradrm would work probably
something like this:

tegradrm creates an object, and passes the reference to host1x
(host1x_set_drm_pdata(host1x_platform_dev, object). host1x sets the
pdata to a member in struct host1x. A getter host1x_get_drm_pdata()
allows retrieving the object. DC would call it with
"host1x_get_drm_pdata(to_platform_device(pdev->dev.parent))".

This assumes that tegradrm would keep ownership of the data and free it
before host1x gets unloaded.

To me this sounds like a steep price and I fail to see the advantage.

Dummy device is something that would come then on top of this mechanism.

Terje

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

[Stephen Warren]

On 12/16/2012 09:37 AM, Terje Bergstr?m wrote:
...
> ... Sure we could tell DC to ask its parent
> (host1x), and call host1x driver with platform_device pointer found that
> way, and host1x would return a pointer to tegradrm's data. Hanging the
> data onto host1x driver is just a more complicated way of implementing
> global data

No it's not; at that point, the data is no longer global, but specific
to the driver instance.

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

[Terje Bergström]

On 17.12.2012 22:55, Stephen Warren wrote:
 On 12/16/2012 09:37 AM, Terje Bergström wrote:
 ...
 ... Sure we could tell DC to ask its parent
 (host1x), and call host1x driver with platform_device pointer found that
 way, and host1x would return a pointer to tegradrm's data. Hanging the
 data onto host1x driver is just a more complicated way of implementing
 global data
 
 No it's not; at that point, the data is no longer global, but specific
 to the driver instance.

We have only one tegradrm, so the advantage is theoretical - the one
driver gets the same pointer in both cases.

If we use static pointer with an accessor function, we can keep the
solution contained to one source code file and the ownership of data is
clear - tegradrm allocates and deallocates the object and is the sole
user. Code is already in the patchset I sent.

Shared responsibility with host1x and tegradrm would work probably
something like this:

tegradrm creates an object, and passes the reference to host1x
(host1x_set_drm_pdata(host1x_platform_dev, object). host1x sets the
pdata to a member in struct host1x. A getter host1x_get_drm_pdata()
allows retrieving the object. DC would call it with
host1x_get_drm_pdata(to_platform_device(pdev-dev.parent)).

This assumes that tegradrm would keep ownership of the data and free it
before host1x gets unloaded.

To me this sounds like a steep price and I fail to see the advantage.

Dummy device is something that would come then on top of this mechanism.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 16.12.2012 14:16, Thierry Reding wrote:
> Okay, so we're back on the topic of using globals. I need to assert
> again that this is not an option. If we were to use globals, then we
> could just as well leave out the dummy device and just do all of that in
> the tegra-drm driver's initialization function.
> The whole point of all this is to link the host1x and it's particular
> tegra-drm instance. I will see if I can find the time to implement this
> in the existing driver, so that the host1x code that you want to remove
> registers the tegra-drm dummy device and the tegra-drm devices use the
> accessors as discussed previously to access host1x and the dummy device.
> Once that is implemented, removing the original host1x implementation
> should be much easier since you will only have to keep the dummy device
> instantiation along with the one or two accessor functions.

I'm not sure what you have discussed with Stephen, so I might be missing
the reason why this is a problem that needs to be solved with new
dependency between tegradrm and host1x instead of locally in tegradrm
driver itself.

As far I remember, we had two reasons for discussing the dummy device.
First reason is for DC, HDMI probe calls to find the global data. Second
is giving something to DRM framework's drm_platform_init().

The easiest way to solve the probe problem is just to have a tegradrm
accessor for the global data in the way I proposed in the patchset.
Dummy device doesn't help there, as the dummy device is in no
relationship to DC and HDMI. Sure we could tell DC to ask its parent
(host1x), and call host1x driver with platform_device pointer found that
way, and host1x would return a pointer to tegradrm's data. Hanging the
data onto host1x driver is just a more complicated way of implementing
global data, and it's breaking the responsibility split between host1x
driver and tegradrm. To me, host1x driver is responsible of host1x, and
tegradrm is responsible of the DRM interface and everything related to that.

All other parts of code use drm_device->dev_private for getting the
global data, so the access problem is only for the probe calls.

drm_platform_init() needing a device is another problem.
drm_platform_init() leads to a call to the CMA FB helper. That again
needed the coherent_dma_mask set for the device give to it. I guess that
problem can be solved by just setting the mask to 0x. But that
is still something that can be handled inside tegradrm without involving
the host1x driver.

Terje

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

[Thierry Reding]

On Fri, Dec 14, 2012 at 09:59:11PM +0200, Terje Bergstr?m wrote:
> On 14.12.2012 18:21, Stephen Warren wrote:
> > On 12/13/2012 11:09 PM, Terje Bergstr?m wrote:
> >> They want to get the global data by getting drvdata of their parent.
> > 
> > There's no reason that /has/ to be so; they can get any information from
> > wherever it is, rather than trying to require it to be somewhere it isn't.
> 
> Exactly, this was also my solution.
> 
> >> The dummy device is not their parent - host1x is. There's no
> >> connection between the dummy and the real client devices.
> > 
> > It's quite possible for the client devices to ask their actual parent
> > (host1x) for the tegradrm struct device *, by calling a host1x API, and
> > have host1x implement that API by returning the dummy/virtual device
> > that it created. That should be just 1-2 lines of code to implement in
> > host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED
> > when appropriate.
> 
> My solution was to just have one global, and an getter for the global.
> Instead of drvdata, the pointer is retrieved with the getter. No need
> for dummy device, or passing points between host1x and tegradrm.

Okay, so we're back on the topic of using globals. I need to assert
again that this is not an option. If we were to use globals, then we
could just as well leave out the dummy device and just do all of that in
the tegra-drm driver's initialization function.

The whole point of all this is to link the host1x and it's particular
tegra-drm instance. I will see if I can find the time to implement this
in the existing driver, so that the host1x code that you want to remove
registers the tegra-drm dummy device and the tegra-drm devices use the
accessors as discussed previously to access host1x and the dummy device.
Once that is implemented, removing the original host1x implementation
should be much easier since you will only have to keep the dummy device
instantiation along with the one or two accessor functions.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

On Fri, Dec 14, 2012 at 09:59:11PM +0200, Terje Bergström wrote:
 On 14.12.2012 18:21, Stephen Warren wrote:
  On 12/13/2012 11:09 PM, Terje Bergström wrote:
  They want to get the global data by getting drvdata of their parent.
  
  There's no reason that /has/ to be so; they can get any information from
  wherever it is, rather than trying to require it to be somewhere it isn't.
 
 Exactly, this was also my solution.
 
  The dummy device is not their parent - host1x is. There's no
  connection between the dummy and the real client devices.
  
  It's quite possible for the client devices to ask their actual parent
  (host1x) for the tegradrm struct device *, by calling a host1x API, and
  have host1x implement that API by returning the dummy/virtual device
  that it created. That should be just 1-2 lines of code to implement in
  host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED
  when appropriate.
 
 My solution was to just have one global, and an getter for the global.
 Instead of drvdata, the pointer is retrieved with the getter. No need
 for dummy device, or passing points between host1x and tegradrm.

Okay, so we're back on the topic of using globals. I need to assert
again that this is not an option. If we were to use globals, then we
could just as well leave out the dummy device and just do all of that in
the tegra-drm driver's initialization function.

The whole point of all this is to link the host1x and it's particular
tegra-drm instance. I will see if I can find the time to implement this
in the existing driver, so that the host1x code that you want to remove
registers the tegra-drm dummy device and the tegra-drm devices use the
accessors as discussed previously to access host1x and the dummy device.
Once that is implemented, removing the original host1x implementation
should be much easier since you will only have to keep the dummy device
instantiation along with the one or two accessor functions.

Thierry


pgpGKpIiYTTTM.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 16.12.2012 14:16, Thierry Reding wrote:
 Okay, so we're back on the topic of using globals. I need to assert
 again that this is not an option. If we were to use globals, then we
 could just as well leave out the dummy device and just do all of that in
 the tegra-drm driver's initialization function.
 The whole point of all this is to link the host1x and it's particular
 tegra-drm instance. I will see if I can find the time to implement this
 in the existing driver, so that the host1x code that you want to remove
 registers the tegra-drm dummy device and the tegra-drm devices use the
 accessors as discussed previously to access host1x and the dummy device.
 Once that is implemented, removing the original host1x implementation
 should be much easier since you will only have to keep the dummy device
 instantiation along with the one or two accessor functions.

I'm not sure what you have discussed with Stephen, so I might be missing
the reason why this is a problem that needs to be solved with new
dependency between tegradrm and host1x instead of locally in tegradrm
driver itself.

As far I remember, we had two reasons for discussing the dummy device.
First reason is for DC, HDMI probe calls to find the global data. Second
is giving something to DRM framework's drm_platform_init().

The easiest way to solve the probe problem is just to have a tegradrm
accessor for the global data in the way I proposed in the patchset.
Dummy device doesn't help there, as the dummy device is in no
relationship to DC and HDMI. Sure we could tell DC to ask its parent
(host1x), and call host1x driver with platform_device pointer found that
way, and host1x would return a pointer to tegradrm's data. Hanging the
data onto host1x driver is just a more complicated way of implementing
global data, and it's breaking the responsibility split between host1x
driver and tegradrm. To me, host1x driver is responsible of host1x, and
tegradrm is responsible of the DRM interface and everything related to that.

All other parts of code use drm_device-dev_private for getting the
global data, so the access problem is only for the probe calls.

drm_platform_init() needing a device is another problem.
drm_platform_init() leads to a call to the CMA FB helper. That again
needed the coherent_dma_mask set for the device give to it. I guess that
problem can be solved by just setting the mask to 0x. But that
is still something that can be handled inside tegradrm without involving
the host1x driver.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 14.12.2012 18:21, Stephen Warren wrote:
> On 12/13/2012 11:09 PM, Terje Bergstr?m wrote:
>> They want to get the global data by getting drvdata of their parent.
> 
> There's no reason that /has/ to be so; they can get any information from
> wherever it is, rather than trying to require it to be somewhere it isn't.

Exactly, this was also my solution.

>> The dummy device is not their parent - host1x is. There's no
>> connection between the dummy and the real client devices.
> 
> It's quite possible for the client devices to ask their actual parent
> (host1x) for the tegradrm struct device *, by calling a host1x API, and
> have host1x implement that API by returning the dummy/virtual device
> that it created. That should be just 1-2 lines of code to implement in
> host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED
> when appropriate.

My solution was to just have one global, and an getter for the global.
Instead of drvdata, the pointer is retrieved with the getter. No need
for dummy device, or passing points between host1x and tegradrm.

Terje

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

[Stephen Warren]

On 12/13/2012 11:09 PM, Terje Bergstr?m wrote:
> On 13.12.2012 19:58, Stephen Warren wrote:
>> On 12/13/2012 01:57 AM, Thierry Reding wrote:
>>> After some more discussion with Stephen on IRC we came to the
>>> conclusion that the easiest might be to have tegra-drm call into
>>> host1x with something like:
>>>
>>> void host1x_set_drm_device(struct host1x *host1x, struct device
>>> *dev);
>>
>> If host1x is registering the dummy device that causes tegradrm to be
>> instantiated, then presumably there's no need for the API above, since
>> host1x will already have the struct device * for tegradrm, since it
>> created it?
> 
> I didn't add the dummy device in my latest patch set. I first set out to
> add it, and moved the drm global data to be drvdata of that device. Then
> I noticed that it doesn't actually help at all.
> 
> The critical accesses to the global data are from probes of DC, HDMI,
> etc.

OK

> They want to get the global data by getting drvdata of their parent.

There's no reason that /has/ to be so; they can get any information from
wherever it is, rather than trying to require it to be somewhere it isn't.

> The dummy device is not their parent - host1x is. There's no
> connection between the dummy and the real client devices.

It's quite possible for the client devices to ask their actual parent
(host1x) for the tegradrm struct device *, by calling a host1x API, and
have host1x implement that API by returning the dummy/virtual device
that it created. That should be just 1-2 lines of code to implement in
host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED
when appropriate.

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

[Terje Bergström]

On 13.12.2012 19:58, Stephen Warren wrote:
> On 12/13/2012 01:57 AM, Thierry Reding wrote:
>> After some more discussion with Stephen on IRC we came to the
>> conclusion that the easiest might be to have tegra-drm call into
>> host1x with something like:
>>
>> void host1x_set_drm_device(struct host1x *host1x, struct device
>> *dev);
> 
> If host1x is registering the dummy device that causes tegradrm to be
> instantiated, then presumably there's no need for the API above, since
> host1x will already have the struct device * for tegradrm, since it
> created it?

I didn't add the dummy device in my latest patch set. I first set out to
add it, and moved the drm global data to be drvdata of that device. Then
I noticed that it doesn't actually help at all.

The critical accesses to the global data are from probes of DC, HDMI,
etc. They want to get the global data by getting drvdata of their
parent. The dummy device is not their parent - host1x is. There's no
connection between the dummy and the real client devices. So there's no
logical way for DC and HDMI to find the the dummy device, except perhaps
by searching for "tegradrm" device from platform bus. But then again,
that'd break encapsulation so it's as bad as a global variable - only
with a lot more code to do the same thing.

Accesses after probing can happen via drm->dev_private, so post-probe
we're fine.

Another problem arouse (already mentioned it) when I used the dummy
device to call to drm_platform_init(). It called back into tegradrm,
which calls the CMA FB helper to allocate memory. Unfortunately the
memory is allocated for the dummy device, and it's not initialized to do
do that. I ended up with failed frame buffer allocation. That needs
host1x allocator to fix.

host1x itself shouldn't need any DRM specific calls or callbacks. The
device model already allows traversing through list of host1x children.
The list of drm clients and devices is something that tegradrm needs to
be able to initialize DRM at appropriate time. I also took that into use
for storing the channel and class data. So we should try to keep the
list maintenance as local to tegradrm as we can.

In my latest patch set, I kept the list management inside tegradrm, and
put all DRM global data into struct tegradrm, which is accessed via a
global. I guess global in tegradrm is not as bad as global in host1x,
because one DRM driver can handle multiple devices, so there's no reason
to have multiple tegradrm's trampling on each others data. But if you
can come up with a better solution, I'm all ears.

Terje

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

[Stephen Warren]

On 12/13/2012 11:09 PM, Terje Bergström wrote:
 On 13.12.2012 19:58, Stephen Warren wrote:
 On 12/13/2012 01:57 AM, Thierry Reding wrote:
 After some more discussion with Stephen on IRC we came to the
 conclusion that the easiest might be to have tegra-drm call into
 host1x with something like:

 void host1x_set_drm_device(struct host1x *host1x, struct device
 *dev);

 If host1x is registering the dummy device that causes tegradrm to be
 instantiated, then presumably there's no need for the API above, since
 host1x will already have the struct device * for tegradrm, since it
 created it?
 
 I didn't add the dummy device in my latest patch set. I first set out to
 add it, and moved the drm global data to be drvdata of that device. Then
 I noticed that it doesn't actually help at all.
 
 The critical accesses to the global data are from probes of DC, HDMI,
 etc.

OK

 They want to get the global data by getting drvdata of their parent.

There's no reason that /has/ to be so; they can get any information from
wherever it is, rather than trying to require it to be somewhere it isn't.

 The dummy device is not their parent - host1x is. There's no
 connection between the dummy and the real client devices.

It's quite possible for the client devices to ask their actual parent
(host1x) for the tegradrm struct device *, by calling a host1x API, and
have host1x implement that API by returning the dummy/virtual device
that it created. That should be just 1-2 lines of code to implement in
host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED
when appropriate.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 14.12.2012 18:21, Stephen Warren wrote:
 On 12/13/2012 11:09 PM, Terje Bergström wrote:
 They want to get the global data by getting drvdata of their parent.
 
 There's no reason that /has/ to be so; they can get any information from
 wherever it is, rather than trying to require it to be somewhere it isn't.

Exactly, this was also my solution.

 The dummy device is not their parent - host1x is. There's no
 connection between the dummy and the real client devices.
 
 It's quite possible for the client devices to ask their actual parent
 (host1x) for the tegradrm struct device *, by calling a host1x API, and
 have host1x implement that API by returning the dummy/virtual device
 that it created. That should be just 1-2 lines of code to implement in
 host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED
 when appropriate.

My solution was to just have one global, and an getter for the global.
Instead of drvdata, the pointer is retrieved with the getter. No need
for dummy device, or passing points between host1x and tegradrm.

Terje

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Thu, Dec 13, 2012 at 10:58:55AM -0700, Stephen Warren wrote:
> On 12/13/2012 01:57 AM, Thierry Reding wrote:
> > On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergstr?m wrote:
> >> On 12.12.2012 18:08, Thierry Reding wrote:
> >>> I've briefly discussed this with Stephen on IRC because I
> >>> thought I had remembered him objecting to the idea of adding a
> >>> dummy device just for this purpose. It turns out, however, that
> >>> what he didn't like was to add a dummy node to the DT just to
> >>> make this happen, but he has no (strong) objections to a dummy
> >>> platform device.
> >>> 
> >>> While I'm not very happy about that solution, I've been going
> >>> over it for a week now and haven't come up with any better
> >>> alternative that doesn't have its own disadvantages. So perhaps
> >>> we should go ahead and implement that. For the host1x driver
> >>> this really just means creating a platform device and adding it
> >>> to the system, with some of the fields tweaked to make things
> >>> work.
> >> 
> >> Even the virtual device is not too beautiful. The problem is that
> >> the virtual device is not physical parent for DC, HDMI, etc, so 
> >> dev_get_drvdata(pdev->dev.parent) returns the data from host1x
> >> device, not the virtual device.
> >> 
> >> We'll post with something that goes around this, but it's not
> >> going to be too pretty. Let's try to find the solution once we
> >> get the code out.
> > 
> > After some more discussion with Stephen on IRC we came to the
> > conclusion that the easiest might be to have tegra-drm call into
> > host1x with something like:
> > 
> > void host1x_set_drm_device(struct host1x *host1x, struct device
> > *dev);
> 
> If host1x is registering the dummy device that causes tegradrm to be
> instantiated, then presumably there's no need for the API above, since
> host1x will already have the struct device * for tegradrm, since it
> created it?

Right, that won't be necessary of course. As long as the driver-private
data of the device stays NULL until tegra-drm is ready (has finished
probing) just getting the struct device from the clients and looking at
that should be enough.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Stephen Warren]

On 12/13/2012 01:57 AM, Thierry Reding wrote:
> On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergstr?m wrote:
>> On 12.12.2012 18:08, Thierry Reding wrote:
>>> I've briefly discussed this with Stephen on IRC because I
>>> thought I had remembered him objecting to the idea of adding a
>>> dummy device just for this purpose. It turns out, however, that
>>> what he didn't like was to add a dummy node to the DT just to
>>> make this happen, but he has no (strong) objections to a dummy
>>> platform device.
>>> 
>>> While I'm not very happy about that solution, I've been going
>>> over it for a week now and haven't come up with any better
>>> alternative that doesn't have its own disadvantages. So perhaps
>>> we should go ahead and implement that. For the host1x driver
>>> this really just means creating a platform device and adding it
>>> to the system, with some of the fields tweaked to make things
>>> work.
>> 
>> Even the virtual device is not too beautiful. The problem is that
>> the virtual device is not physical parent for DC, HDMI, etc, so 
>> dev_get_drvdata(pdev->dev.parent) returns the data from host1x
>> device, not the virtual device.
>> 
>> We'll post with something that goes around this, but it's not
>> going to be too pretty. Let's try to find the solution once we
>> get the code out.
> 
> After some more discussion with Stephen on IRC we came to the
> conclusion that the easiest might be to have tegra-drm call into
> host1x with something like:
> 
> void host1x_set_drm_device(struct host1x *host1x, struct device
> *dev);

If host1x is registering the dummy device that causes tegradrm to be
instantiated, then presumably there's no need for the API above, since
host1x will already have the struct device * for tegradrm, since it
created it?

> Once the dummy device has been properly set up and have each client
> use
> 
> struct device *host1x_get_drm_device(struct host1x *host1x);
> 
> to obtain a pointer to the dummy device, such that it can receive
> the driver-private data using dev_get_drvdata() on the returned
> device. As long as tegra-drm hasn't registered with host1x yet, the
> above function could return ERR_PTR(-EPROBE_DEFER), so that
> dependencies are automatically handled. This is required because,
> tegra-drm not being the parent of the child devices, it can be
> guaranteed that it is probed before any of the children.
> 
> That way we should be able to get around any circular
> dependencies, since we only call into host1x from tegra-drm but not
> the other way around.

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

[Terje Bergström]

On 12.12.2012 18:08, Thierry Reding wrote:
> I've briefly discussed this with Stephen on IRC because I thought I had
> remembered him objecting to the idea of adding a dummy device just for
> this purpose. It turns out, however, that what he didn't like was to add
> a dummy node to the DT just to make this happen, but he has no (strong)
> objections to a dummy platform device.
> 
> While I'm not very happy about that solution, I've been going over it
> for a week now and haven't come up with any better alternative that
> doesn't have its own disadvantages. So perhaps we should go ahead and
> implement that. For the host1x driver this really just means creating a
> platform device and adding it to the system, with some of the fields
> tweaked to make things work.

Even the virtual device is not too beautiful. The problem is that the
virtual device is not physical parent for DC, HDMI, etc, so
dev_get_drvdata(pdev->dev.parent) returns the data from host1x device,
not the virtual device.

We'll post with something that goes around this, but it's not going to
be too pretty. Let's try to find the solution once we get the code out.

Terje

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

[Thierry Reding]

On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergstr?m wrote:
> On 12.12.2012 18:08, Thierry Reding wrote:
> > I've briefly discussed this with Stephen on IRC because I thought I had
> > remembered him objecting to the idea of adding a dummy device just for
> > this purpose. It turns out, however, that what he didn't like was to add
> > a dummy node to the DT just to make this happen, but he has no (strong)
> > objections to a dummy platform device.
> > 
> > While I'm not very happy about that solution, I've been going over it
> > for a week now and haven't come up with any better alternative that
> > doesn't have its own disadvantages. So perhaps we should go ahead and
> > implement that. For the host1x driver this really just means creating a
> > platform device and adding it to the system, with some of the fields
> > tweaked to make things work.
> 
> Even the virtual device is not too beautiful. The problem is that the
> virtual device is not physical parent for DC, HDMI, etc, so
> dev_get_drvdata(pdev->dev.parent) returns the data from host1x device,
> not the virtual device.
> 
> We'll post with something that goes around this, but it's not going to
> be too pretty. Let's try to find the solution once we get the code out.

After some more discussion with Stephen on IRC we came to the conclusion
that the easiest might be to have tegra-drm call into host1x with
something like:

void host1x_set_drm_device(struct host1x *host1x, struct device *dev);

Once the dummy device has been properly set up and have each client use

struct device *host1x_get_drm_device(struct host1x *host1x);

to obtain a pointer to the dummy device, such that it can receive the
driver-private data using dev_get_drvdata() on the returned device. As
long as tegra-drm hasn't registered with host1x yet, the above function
could return ERR_PTR(-EPROBE_DEFER), so that dependencies are
automatically handled. This is required because, tegra-drm not being the
parent of the child devices, it can be guaranteed that it is probed
before any of the children.

That way we should be able to get around any circular dependencies,
since we only call into host1x from tegra-drm but not the other way
around.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Terje Bergström]

On 12.12.2012 18:08, Thierry Reding wrote:
 I've briefly discussed this with Stephen on IRC because I thought I had
 remembered him objecting to the idea of adding a dummy device just for
 this purpose. It turns out, however, that what he didn't like was to add
 a dummy node to the DT just to make this happen, but he has no (strong)
 objections to a dummy platform device.
 
 While I'm not very happy about that solution, I've been going over it
 for a week now and haven't come up with any better alternative that
 doesn't have its own disadvantages. So perhaps we should go ahead and
 implement that. For the host1x driver this really just means creating a
 platform device and adding it to the system, with some of the fields
 tweaked to make things work.

Even the virtual device is not too beautiful. The problem is that the
virtual device is not physical parent for DC, HDMI, etc, so
dev_get_drvdata(pdev-dev.parent) returns the data from host1x device,
not the virtual device.

We'll post with something that goes around this, but it's not going to
be too pretty. Let's try to find the solution once we get the code out.

Terje

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergström wrote:
 On 12.12.2012 18:08, Thierry Reding wrote:
  I've briefly discussed this with Stephen on IRC because I thought I had
  remembered him objecting to the idea of adding a dummy device just for
  this purpose. It turns out, however, that what he didn't like was to add
  a dummy node to the DT just to make this happen, but he has no (strong)
  objections to a dummy platform device.
  
  While I'm not very happy about that solution, I've been going over it
  for a week now and haven't come up with any better alternative that
  doesn't have its own disadvantages. So perhaps we should go ahead and
  implement that. For the host1x driver this really just means creating a
  platform device and adding it to the system, with some of the fields
  tweaked to make things work.
 
 Even the virtual device is not too beautiful. The problem is that the
 virtual device is not physical parent for DC, HDMI, etc, so
 dev_get_drvdata(pdev-dev.parent) returns the data from host1x device,
 not the virtual device.
 
 We'll post with something that goes around this, but it's not going to
 be too pretty. Let's try to find the solution once we get the code out.

After some more discussion with Stephen on IRC we came to the conclusion
that the easiest might be to have tegra-drm call into host1x with
something like:

void host1x_set_drm_device(struct host1x *host1x, struct device *dev);

Once the dummy device has been properly set up and have each client use

struct device *host1x_get_drm_device(struct host1x *host1x);

to obtain a pointer to the dummy device, such that it can receive the
driver-private data using dev_get_drvdata() on the returned device. As
long as tegra-drm hasn't registered with host1x yet, the above function
could return ERR_PTR(-EPROBE_DEFER), so that dependencies are
automatically handled. This is required because, tegra-drm not being the
parent of the child devices, it can be guaranteed that it is probed
before any of the children.

That way we should be able to get around any circular dependencies,
since we only call into host1x from tegra-drm but not the other way
around.

Thierry


pgpOFIHZxMouW.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

On 12/13/2012 01:57 AM, Thierry Reding wrote:
 On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergström wrote:
 On 12.12.2012 18:08, Thierry Reding wrote:
 I've briefly discussed this with Stephen on IRC because I
 thought I had remembered him objecting to the idea of adding a
 dummy device just for this purpose. It turns out, however, that
 what he didn't like was to add a dummy node to the DT just to
 make this happen, but he has no (strong) objections to a dummy
 platform device.
 
 While I'm not very happy about that solution, I've been going
 over it for a week now and haven't come up with any better
 alternative that doesn't have its own disadvantages. So perhaps
 we should go ahead and implement that. For the host1x driver
 this really just means creating a platform device and adding it
 to the system, with some of the fields tweaked to make things
 work.
 
 Even the virtual device is not too beautiful. The problem is that
 the virtual device is not physical parent for DC, HDMI, etc, so 
 dev_get_drvdata(pdev-dev.parent) returns the data from host1x
 device, not the virtual device.
 
 We'll post with something that goes around this, but it's not
 going to be too pretty. Let's try to find the solution once we
 get the code out.
 
 After some more discussion with Stephen on IRC we came to the
 conclusion that the easiest might be to have tegra-drm call into
 host1x with something like:
 
 void host1x_set_drm_device(struct host1x *host1x, struct device
 *dev);

If host1x is registering the dummy device that causes tegradrm to be
instantiated, then presumably there's no need for the API above, since
host1x will already have the struct device * for tegradrm, since it
created it?

 Once the dummy device has been properly set up and have each client
 use
 
 struct device *host1x_get_drm_device(struct host1x *host1x);
 
 to obtain a pointer to the dummy device, such that it can receive
 the driver-private data using dev_get_drvdata() on the returned
 device. As long as tegra-drm hasn't registered with host1x yet, the
 above function could return ERR_PTR(-EPROBE_DEFER), so that
 dependencies are automatically handled. This is required because,
 tegra-drm not being the parent of the child devices, it can be
 guaranteed that it is probed before any of the children.
 
 That way we should be able to get around any circular
 dependencies, since we only call into host1x from tegra-drm but not
 the other way around.


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Thu, Dec 13, 2012 at 10:58:55AM -0700, Stephen Warren wrote:
 On 12/13/2012 01:57 AM, Thierry Reding wrote:
  On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergström wrote:
  On 12.12.2012 18:08, Thierry Reding wrote:
  I've briefly discussed this with Stephen on IRC because I
  thought I had remembered him objecting to the idea of adding a
  dummy device just for this purpose. It turns out, however, that
  what he didn't like was to add a dummy node to the DT just to
  make this happen, but he has no (strong) objections to a dummy
  platform device.
  
  While I'm not very happy about that solution, I've been going
  over it for a week now and haven't come up with any better
  alternative that doesn't have its own disadvantages. So perhaps
  we should go ahead and implement that. For the host1x driver
  this really just means creating a platform device and adding it
  to the system, with some of the fields tweaked to make things
  work.
  
  Even the virtual device is not too beautiful. The problem is that
  the virtual device is not physical parent for DC, HDMI, etc, so 
  dev_get_drvdata(pdev-dev.parent) returns the data from host1x
  device, not the virtual device.
  
  We'll post with something that goes around this, but it's not
  going to be too pretty. Let's try to find the solution once we
  get the code out.
  
  After some more discussion with Stephen on IRC we came to the
  conclusion that the easiest might be to have tegra-drm call into
  host1x with something like:
  
  void host1x_set_drm_device(struct host1x *host1x, struct device
  *dev);
 
 If host1x is registering the dummy device that causes tegradrm to be
 instantiated, then presumably there's no need for the API above, since
 host1x will already have the struct device * for tegradrm, since it
 created it?

Right, that won't be necessary of course. As long as the driver-private
data of the device stays NULL until tegra-drm is ready (has finished
probing) just getting the struct device from the clients and looking at
that should be enough.

Thierry


pgp6oBb0ENmZV.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 13.12.2012 19:58, Stephen Warren wrote:
 On 12/13/2012 01:57 AM, Thierry Reding wrote:
 After some more discussion with Stephen on IRC we came to the
 conclusion that the easiest might be to have tegra-drm call into
 host1x with something like:

 void host1x_set_drm_device(struct host1x *host1x, struct device
 *dev);
 
 If host1x is registering the dummy device that causes tegradrm to be
 instantiated, then presumably there's no need for the API above, since
 host1x will already have the struct device * for tegradrm, since it
 created it?

I didn't add the dummy device in my latest patch set. I first set out to
add it, and moved the drm global data to be drvdata of that device. Then
I noticed that it doesn't actually help at all.

The critical accesses to the global data are from probes of DC, HDMI,
etc. They want to get the global data by getting drvdata of their
parent. The dummy device is not their parent - host1x is. There's no
connection between the dummy and the real client devices. So there's no
logical way for DC and HDMI to find the the dummy device, except perhaps
by searching for tegradrm device from platform bus. But then again,
that'd break encapsulation so it's as bad as a global variable - only
with a lot more code to do the same thing.

Accesses after probing can happen via drm-dev_private, so post-probe
we're fine.

Another problem arouse (already mentioned it) when I used the dummy
device to call to drm_platform_init(). It called back into tegradrm,
which calls the CMA FB helper to allocate memory. Unfortunately the
memory is allocated for the dummy device, and it's not initialized to do
do that. I ended up with failed frame buffer allocation. That needs
host1x allocator to fix.

host1x itself shouldn't need any DRM specific calls or callbacks. The
device model already allows traversing through list of host1x children.
The list of drm clients and devices is something that tegradrm needs to
be able to initialize DRM at appropriate time. I also took that into use
for storing the channel and class data. So we should try to keep the
list maintenance as local to tegradrm as we can.

In my latest patch set, I kept the list management inside tegradrm, and
put all DRM global data into struct tegradrm, which is accessed via a
global. I guess global in tegradrm is not as bad as global in host1x,
because one DRM driver can handle multiple devices, so there's no reason
to have multiple tegradrm's trampling on each others data. But if you
can come up with a better solution, I'm all ears.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 12.12.2012 18:08, Thierry Reding wrote:
> I've briefly discussed this with Stephen on IRC because I thought I had
> remembered him objecting to the idea of adding a dummy device just for
> this purpose. It turns out, however, that what he didn't like was to add
> a dummy node to the DT just to make this happen, but he has no (strong)
> objections to a dummy platform device.
> 
> While I'm not very happy about that solution, I've been going over it
> for a week now and haven't come up with any better alternative that
> doesn't have its own disadvantages. So perhaps we should go ahead and
> implement that. For the host1x driver this really just means creating a
> platform device and adding it to the system, with some of the fields
> tweaked to make things work.

This sounds ok. Considering that the DRM driver is a virtual driver,
attaching it to a virtual device sounds ok -- ish.

> Is this something that you can take care of in your patch series? I
> could also implement this on top of the current driver and then all your
> patch series would have to do is remove host1x.c from tegra-drm and
> instantiate the platform device itself.

We're about to post patches for both host1x driver and libdrm. This is
the last big rock unturned, so let me take a stab at this first. I'd
like to get the updated patches out tomorrow or on Friday, so I'll try
to either get it done for that or leave it to the TODO list.

Terje

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

[Thierry Reding]

On Mon, Dec 10, 2012 at 01:42:45PM +0200, Terje Bergstr?m wrote:
> On 05.12.2012 14:04, Thierry Reding wrote:
> > On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergstr?m wrote:
> >> 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.
> 
> I've managed to go through all the other feedback and implement a
> solution to most of them, so now I have some slack to actually think
> about the initialization. Sorry about this, but you (meaning all the
> reviewers) did give us a _lot_ to do. :-) Fortunately, the driver
> actually became a lot better, too.
> 
> Back to the topic of tegradrm init. The root cause of the problem is
> that DRM framework needs some device to assign itself to. The problem is
> that this device doesn't have any physical counterpart, as it's only for
> storing a pointer in DRM framework. Please correct me if this is wrong.
> 
> Moving the client registration to ping pong between DRM and host1x has
> its problems. host1x driver itself has no use for a list of client
> devices. It can just iterate its children in case it needs them. In
> tegradrm, you need a list of devices under tegradrm control, which might
> or might not be the same as list of devices under host1x hardware.
> 
> The solutions that many other DRM drivers seem to employ are the virtual
> devices. Exynos and OMAP drivers do this, as does SH Mobile DRM driver.
> So I think I'd still go this way, as it's the path of minimum
> resistance, least amount of code and most localized change. I know it's
> not ideal, but I'd also not like to get stuck in this.

I've briefly discussed this with Stephen on IRC because I thought I had
remembered him objecting to the idea of adding a dummy device just for
this purpose. It turns out, however, that what he didn't like was to add
a dummy node to the DT just to make this happen, but he has no (strong)
objections to a dummy platform device.

While I'm not very happy about that solution, I've been going over it
for a week now and haven't come up with any better alternative that
doesn't have its own disadvantages. So perhaps we should go ahead and
implement that. For the host1x driver this really just means creating a
platform device and adding it to the system, with some of the fields
tweaked to make things work.

Is this something that you can take care of in your patch series? I
could also implement this on top of the current driver and then all your
patch series would have to do is remove host1x.c from tegra-drm and
instantiate the platform device itself.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

On Mon, Dec 10, 2012 at 01:42:45PM +0200, Terje Bergström wrote:
 On 05.12.2012 14:04, Thierry Reding wrote:
  On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergström wrote:
  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.
 
 I've managed to go through all the other feedback and implement a
 solution to most of them, so now I have some slack to actually think
 about the initialization. Sorry about this, but you (meaning all the
 reviewers) did give us a _lot_ to do. :-) Fortunately, the driver
 actually became a lot better, too.
 
 Back to the topic of tegradrm init. The root cause of the problem is
 that DRM framework needs some device to assign itself to. The problem is
 that this device doesn't have any physical counterpart, as it's only for
 storing a pointer in DRM framework. Please correct me if this is wrong.
 
 Moving the client registration to ping pong between DRM and host1x has
 its problems. host1x driver itself has no use for a list of client
 devices. It can just iterate its children in case it needs them. In
 tegradrm, you need a list of devices under tegradrm control, which might
 or might not be the same as list of devices under host1x hardware.
 
 The solutions that many other DRM drivers seem to employ are the virtual
 devices. Exynos and OMAP drivers do this, as does SH Mobile DRM driver.
 So I think I'd still go this way, as it's the path of minimum
 resistance, least amount of code and most localized change. I know it's
 not ideal, but I'd also not like to get stuck in this.

I've briefly discussed this with Stephen on IRC because I thought I had
remembered him objecting to the idea of adding a dummy device just for
this purpose. It turns out, however, that what he didn't like was to add
a dummy node to the DT just to make this happen, but he has no (strong)
objections to a dummy platform device.

While I'm not very happy about that solution, I've been going over it
for a week now and haven't come up with any better alternative that
doesn't have its own disadvantages. So perhaps we should go ahead and
implement that. For the host1x driver this really just means creating a
platform device and adding it to the system, with some of the fields
tweaked to make things work.

Is this something that you can take care of in your patch series? I
could also implement this on top of the current driver and then all your
patch series would have to do is remove host1x.c from tegra-drm and
instantiate the platform device itself.

Thierry


pgpkr1vZRqwZi.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 05.12.2012 14:04, Thierry Reding wrote:
> On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergstr?m wrote:
>> 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.

I've managed to go through all the other feedback and implement a
solution to most of them, so now I have some slack to actually think
about the initialization. Sorry about this, but you (meaning all the
reviewers) did give us a _lot_ to do. :-) Fortunately, the driver
actually became a lot better, too.

Back to the topic of tegradrm init. The root cause of the problem is
that DRM framework needs some device to assign itself to. The problem is
that this device doesn't have any physical counterpart, as it's only for
storing a pointer in DRM framework. Please correct me if this is wrong.

Moving the client registration to ping pong between DRM and host1x has
its problems. host1x driver itself has no use for a list of client
devices. It can just iterate its children in case it needs them. In
tegradrm, you need a list of devices under tegradrm control, which might
or might not be the same as list of devices under host1x hardware.

The solutions that many other DRM drivers seem to employ are the virtual
devices. Exynos and OMAP drivers do this, as does SH Mobile DRM driver.
So I think I'd still go this way, as it's the path of minimum
resistance, least amount of code and most localized change. I know it's
not ideal, but I'd also not like to get stuck in this.

Terje

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

[Terje Bergström]

On 05.12.2012 14:04, Thierry Reding wrote:
 On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergström wrote:
 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.

I've managed to go through all the other feedback and implement a
solution to most of them, so now I have some slack to actually think
about the initialization. Sorry about this, but you (meaning all the
reviewers) did give us a _lot_ to do. :-) Fortunately, the driver
actually became a lot better, too.

Back to the topic of tegradrm init. The root cause of the problem is
that DRM framework needs some device to assign itself to. The problem is
that this device doesn't have any physical counterpart, as it's only for
storing a pointer in DRM framework. Please correct me if this is wrong.

Moving the client registration to ping pong between DRM and host1x has
its problems. host1x driver itself has no use for a list of client
devices. It can just iterate its children in case it needs them. In
tegradrm, you need a list of devices under tegradrm control, which might
or might not be the same as list of devices under host1x hardware.

The solutions that many other DRM drivers seem to employ are the virtual
devices. Exynos and OMAP drivers do this, as does SH Mobile DRM driver.
So I think I'd still go this way, as it's the path of minimum
resistance, least amount of code and most localized change. I know it's
not ideal, but I'd also not like to get stuck in this.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Wed, Dec 05, 2012 at 05:34:30PM +0100, Daniel Vetter wrote:
 On Wed, Dec 5, 2012 at 2:28 PM, Thierry Reding
 thierry.red...@avionic-design.de wrote:
  Imo that's worse, since now drm manages even more of the driver-hw
  binding process. In my dream world the drm driver just registers a
  normal driver at module load time directly with whatever bus it's
  interested in, and then, from it the bus' -probe callback setups up
  the entire driver, calling down into drm to setup up interfaces to
  userspace (dev node, sysfs, and whatever is required to implement the
  ioctls) and uses the various helper libraries provided. So host1x
  providing a virtual device that tegradrm can match sounds much better
  (if that helps in decoupling from host1x).
 
  Okay, now I see where you're going. You want to replace the various
  drm_*_init() functions with something more fine-grained that does the
  initialization manually in each driver. Is that it? The obvious
  disadvantage is that a lot of code would have to be duplicated, though
  that can presumably be factored out into further helpers if necessary.
 
  On that note, I just noticed that drm_platform_init() actually binds a
  single platform_device to the drm_driver, which isn't going to work very
  well in case there are two devices that want to use the same driver. It
  would be nice to get rid of that limitation as well while at it.
 
 Yeah, it's the lack of generality that irks me, and writing driver
 init code is one giant sequence of setup function calls anyway -
 sprinkling 1-2 more doesn't really matter, but helps a lot if it
 results in the driver being in full control (e.g. if you need to
 reorder due to some special requirement, that's much easier to do then
 than with the current hoop-jumping). But like I've said, a bit a
 bigger fish to fry, just wanted to point you into that direction ...

I have quite a number of things to finish up myself and this sounds like
quite a bit of work.

  Or simply call the tegradrm setup from host1x directly, creating a
  depency on the tegradrm module. When trying to unload host1x it can
  then also call down into tegradrm to tear down the drm device.
  Afterwards you should be able to unload tegradrm without fuzz. And if
  the hard dependcy is an issue for other host1x clients this
  setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.
 
  This is what I originally proposed. However, since tegra-drm will need
  to call functions provided by host1x we have a cyclic dependency.
  Wouldn't that prevent either module from being unloaded?
 
 You could pass down a host1x interface struct with a vtable to
 tegradrm (plus some static inline helpers to make those not a pain to
 call).

That sounds very interesting. It's also in line with what Terje proposed
earlier, making the host1x into a helper library, only the registration
part would remain with host1x. So in this kind of setup, the host1x
driver would initialize tegra-drm with something like:

int tegra_drm_init(struct device *parent, const struct host1x_ops *ops)
{
struct platform_device *pdev = to_platform_device(parent);
int err;

err = drm_platform_init(tegra_drm_driver, pdev);
...
}

The DRM driver's .load() callback would of course have to be passed the
ops pointer. Either that or indeed some kind of custom setup function is
needed instead of calling drm_platform_init().

Maybe I should go and give such an implementation a shot, see where it
ends up.

 The other possibility (and I'm not proud at all of that code)
 which we've used in the intel-ips driver is to use symbol_get at
 runtime - but there the requirement was explicitly that intel-ips
 needs to work on server systems without the drm/i915 driver loaded,
 but still always have the support for interacting with it compiled in
 (to make distros happy). It's all rather awkward though ...

Hehe, indeed. Adding a dummy platform device suddenly doesn't sound that
bad. =)

Thierry


pgpKxGOUZlz0D.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Wed, Dec 05, 2012 at 05:34:30PM +0100, Daniel Vetter wrote:
> On Wed, Dec 5, 2012 at 2:28 PM, Thierry Reding
>  wrote:
> >> Imo that's worse, since now drm manages even more of the driver->hw
> >> binding process. In my dream world the drm driver just registers a
> >> normal driver at module load time directly with whatever bus it's
> >> interested in, and then, from it the bus' ->probe callback setups up
> >> the entire driver, calling down into drm to setup up interfaces to
> >> userspace (dev node, sysfs, and whatever is required to implement the
> >> ioctls) and uses the various helper libraries provided. So host1x
> >> providing a virtual device that tegradrm can match sounds much better
> >> (if that helps in decoupling from host1x).
> >
> > Okay, now I see where you're going. You want to replace the various
> > drm_*_init() functions with something more fine-grained that does the
> > initialization manually in each driver. Is that it? The obvious
> > disadvantage is that a lot of code would have to be duplicated, though
> > that can presumably be factored out into further helpers if necessary.
> >
> > On that note, I just noticed that drm_platform_init() actually binds a
> > single platform_device to the drm_driver, which isn't going to work very
> > well in case there are two devices that want to use the same driver. It
> > would be nice to get rid of that limitation as well while at it.
> 
> Yeah, it's the lack of generality that irks me, and writing driver
> init code is one giant sequence of setup function calls anyway -
> sprinkling 1-2 more doesn't really matter, but helps a lot if it
> results in the driver being in full control (e.g. if you need to
> reorder due to some special requirement, that's much easier to do then
> than with the current hoop-jumping). But like I've said, a bit a
> bigger fish to fry, just wanted to point you into that direction ...

I have quite a number of things to finish up myself and this sounds like
quite a bit of work.

> >> Or simply call the tegradrm setup from host1x directly, creating a
> >> depency on the tegradrm module. When trying to unload host1x it can
> >> then also call down into tegradrm to tear down the drm device.
> >> Afterwards you should be able to unload tegradrm without fuzz. And if
> >> the hard dependcy is an issue for other host1x clients this
> >> setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.
> >
> > This is what I originally proposed. However, since tegra-drm will need
> > to call functions provided by host1x we have a cyclic dependency.
> > Wouldn't that prevent either module from being unloaded?
> 
> You could pass down a host1x interface struct with a vtable to
> tegradrm (plus some static inline helpers to make those not a pain to
> call).

That sounds very interesting. It's also in line with what Terje proposed
earlier, making the host1x into a helper library, only the registration
part would remain with host1x. So in this kind of setup, the host1x
driver would initialize tegra-drm with something like:

int tegra_drm_init(struct device *parent, const struct host1x_ops *ops)
{
struct platform_device *pdev = to_platform_device(parent);
int err;

err = drm_platform_init(_drm_driver, pdev);
...
}

The DRM driver's .load() callback would of course have to be passed the
ops pointer. Either that or indeed some kind of custom setup function is
needed instead of calling drm_platform_init().

Maybe I should go and give such an implementation a shot, see where it
ends up.

> The other possibility (and I'm not proud at all of that code)
> which we've used in the intel-ips driver is to use symbol_get at
> runtime - but there the requirement was explicitly that intel-ips
> needs to work on server systems without the drm/i915 driver loaded,
> but still always have the support for interacting with it compiled in
> (to make distros happy). It's all rather awkward though ...

Hehe, indeed. Adding a dummy platform device suddenly doesn't sound that
bad. =)

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Terje Bergström]

On 05.12.2012 14:04, Thierry Reding wrote:
> On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergstr?m wrote:
> 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.

Sure, that feedback is in the todo list. For some parts I already have a
proposed solution, but for chip ops not yet.

>> 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.

Oops, I must've checked the status from a stale tree. Thanks for that.
In this case there's no technical reason why host1x couldn't act as the
device to register for drm, as drm wouldn't touch host1x device data in
any way.

How about if we treat the host1x driver as utility library, and move the
code for host1x probe altogether to tegradrm/host1x.c? I haven't yet
checked how bad the dependencies between host1x's driver's host1x.c and
the rest of the driver are, so I'm not sure if it's feasible and if
there would be a clear design. Just tossing in ideas.

That would solve the circular dependency problem. I've already committed
to contents of host1x.c being very different in downstream and upstream,
so this change would just emphasize that.

Terje

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 2:28 PM, Thierry Reding
 wrote:
>> Imo that's worse, since now drm manages even more of the driver->hw
>> binding process. In my dream world the drm driver just registers a
>> normal driver at module load time directly with whatever bus it's
>> interested in, and then, from it the bus' ->probe callback setups up
>> the entire driver, calling down into drm to setup up interfaces to
>> userspace (dev node, sysfs, and whatever is required to implement the
>> ioctls) and uses the various helper libraries provided. So host1x
>> providing a virtual device that tegradrm can match sounds much better
>> (if that helps in decoupling from host1x).
>
> Okay, now I see where you're going. You want to replace the various
> drm_*_init() functions with something more fine-grained that does the
> initialization manually in each driver. Is that it? The obvious
> disadvantage is that a lot of code would have to be duplicated, though
> that can presumably be factored out into further helpers if necessary.
>
> On that note, I just noticed that drm_platform_init() actually binds a
> single platform_device to the drm_driver, which isn't going to work very
> well in case there are two devices that want to use the same driver. It
> would be nice to get rid of that limitation as well while at it.

Yeah, it's the lack of generality that irks me, and writing driver
init code is one giant sequence of setup function calls anyway -
sprinkling 1-2 more doesn't really matter, but helps a lot if it
results in the driver being in full control (e.g. if you need to
reorder due to some special requirement, that's much easier to do then
than with the current hoop-jumping). But like I've said, a bit a
bigger fish to fry, just wanted to point you into that direction ...

>> Or simply call the tegradrm setup from host1x directly, creating a
>> depency on the tegradrm module. When trying to unload host1x it can
>> then also call down into tegradrm to tear down the drm device.
>> Afterwards you should be able to unload tegradrm without fuzz. And if
>> the hard dependcy is an issue for other host1x clients this
>> setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.
>
> This is what I originally proposed. However, since tegra-drm will need
> to call functions provided by host1x we have a cyclic dependency.
> Wouldn't that prevent either module from being unloaded?

You could pass down a host1x interface struct with a vtable to
tegradrm (plus some static inline helpers to make those not a pain to
call). The other possibility (and I'm not proud at all of that code)
which we've used in the intel-ips driver is to use symbol_get at
runtime - but there the requirement was explicitly that intel-ips
needs to work on server systems without the drm/i915 driver loaded,
but still always have the support for interacting with it compiled in
(to make distros happy). It's all rather awkward though ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

On Wed, Dec 05, 2012 at 01:31:54PM +0100, Daniel Vetter wrote:
> On Wed, Dec 5, 2012 at 1:22 PM, Thierry Reding
>  wrote:
> > Maybe something more elaborate could help, though. Suppose we add
> > functionality to DRM to instantiate a DRM device. We could call such a
> > function from the host1x driver to add a device which the tegra-drm
> > driver could bind to. This would entail something like a small bus
> > implementation for DRM that would allow matching DRM devices to DRM
> > drivers much like the platform or PCI busses. This could automatically
> > take care of registering the DRM device with the proper parent so that
> > host1x clients can lookup the host1x via dev->parent.
> >
> > Perhaps something as simple as
> >
> > int drm_device_register(struct device *parent, const char *name, 
> > int id);
> >
> > could work. Or something more elaborate such as allocating a device with
> >
> > struct drm_device *drm_device_alloc(const char *name, int id);
> >
> > paired with
> >
> > int drm_device_register(struct drm_device *dev);
> >
> > would be more flexible in that drivers could modify the DRM device in
> > between the two calls.
> 
> Imo that's worse, since now drm manages even more of the driver->hw
> binding process. In my dream world the drm driver just registers a
> normal driver at module load time directly with whatever bus it's
> interested in, and then, from it the bus' ->probe callback setups up
> the entire driver, calling down into drm to setup up interfaces to
> userspace (dev node, sysfs, and whatever is required to implement the
> ioctls) and uses the various helper libraries provided. So host1x
> providing a virtual device that tegradrm can match sounds much better
> (if that helps in decoupling from host1x).

Okay, now I see where you're going. You want to replace the various
drm_*_init() functions with something more fine-grained that does the
initialization manually in each driver. Is that it? The obvious
disadvantage is that a lot of code would have to be duplicated, though
that can presumably be factored out into further helpers if necessary.

On that note, I just noticed that drm_platform_init() actually binds a
single platform_device to the drm_driver, which isn't going to work very
well in case there are two devices that want to use the same driver. It
would be nice to get rid of that limitation as well while at it.

> Or simply call the tegradrm setup from host1x directly, creating a
> depency on the tegradrm module. When trying to unload host1x it can
> then also call down into tegradrm to tear down the drm device.
> Afterwards you should be able to unload tegradrm without fuzz. And if
> the hard dependcy is an issue for other host1x clients this
> setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.

This is what I originally proposed. However, since tegra-drm will need
to call functions provided by host1x we have a cyclic dependency.
Wouldn't that prevent either module from being unloaded?

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Terje Bergström]

On 05.12.2012 13:13, Thierry Reding wrote:
> 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.

Ok.

> 
>> 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.

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?

> 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.

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?

Terje

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 1:22 PM, Thierry Reding
 wrote:
> Maybe something more elaborate could help, though. Suppose we add
> functionality to DRM to instantiate a DRM device. We could call such a
> function from the host1x driver to add a device which the tegra-drm
> driver could bind to. This would entail something like a small bus
> implementation for DRM that would allow matching DRM devices to DRM
> drivers much like the platform or PCI busses. This could automatically
> take care of registering the DRM device with the proper parent so that
> host1x clients can lookup the host1x via dev->parent.
>
> Perhaps something as simple as
>
> int drm_device_register(struct device *parent, const char *name, int 
> id);
>
> could work. Or something more elaborate such as allocating a device with
>
> struct drm_device *drm_device_alloc(const char *name, int id);
>
> paired with
>
> int drm_device_register(struct drm_device *dev);
>
> would be more flexible in that drivers could modify the DRM device in
> between the two calls.

Imo that's worse, since now drm manages even more of the driver->hw
binding process. In my dream world the drm driver just registers a
normal driver at module load time directly with whatever bus it's
interested in, and then, from it the bus' ->probe callback setups up
the entire driver, calling down into drm to setup up interfaces to
userspace (dev node, sysfs, and whatever is required to implement the
ioctls) and uses the various helper libraries provided. So host1x
providing a virtual device that tegradrm can match sounds much better
(if that helps in decoupling from host1x).

Or simply call the tegradrm setup from host1x directly, creating a
depency on the tegradrm module. When trying to unload host1x it can
then also call down into tegradrm to tear down the drm device.
Afterwards you should be able to unload tegradrm without fuzz. And if
the hard dependcy is an issue for other host1x clients this
setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

On Wed, Dec 05, 2012 at 01:03:14PM +0100, Daniel Vetter wrote:
> On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergstr?m  
> wrote:
> > 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?
> 
> Note that I'm not too happy about the fact that drm wants a struct
> device to register a drm device. This all made a lot of sense back in
> the days when drm drivers this this fancy shadow attaching to allow
> drm to use a driver for rendering cooperatively with a fbdev driver.
> Today there's not much reason for that anymore imo, and I'd welcome
> patches to allow drivers to simply register a drm device (and remove
> all the newer registration functions for usb/platform/whatever
> drivers, moving the device handling into drivers). Note that it's a
> bit work, since not-really-required abstraction (which was useful back
> when the drm drivers have been shared with *BSD, but pointless now)
> like the drm irq support needs to be moved away to a pci-dev legacy
> thing only - it doesn't really buy a kms driver anything above
> calling request_irq() itself.
> 
> So feel free to burn this down, I'll be happy to carry wood to the
> pyre in the from of reviews (not much time for more right now ...).

This wouldn't solve the problem at hand, though, since we'd still need
to instantiate the DRM driver somehow. Currently this is triggered
ultimately by the host1x's .probe().

Maybe something more elaborate could help, though. Suppose we add
functionality to DRM to instantiate a DRM device. We could call such a
function from the host1x driver to add a device which the tegra-drm
driver could bind to. This would entail something like a small bus
implementation for DRM that would allow matching DRM devices to DRM
drivers much like the platform or PCI busses. This could automatically
take care of registering the DRM device with the proper parent so that
host1x clients can lookup the host1x via dev->parent.

Perhaps something as simple as

int drm_device_register(struct device *parent, const char *name, int 
id);

could work. Or something more elaborate such as allocating a device with

struct drm_device *drm_device_alloc(const char *name, int id);

paired with

int drm_device_register(struct drm_device *dev);

would be more flexible in that drivers could modify the DRM device in
between the two calls.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 1:03 PM, Daniel Vetter  wrote:
> On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergstr?m  
> wrote:
>> 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?
>
> Note that I'm not too happy about the fact that drm wants a struct
> device to register a drm device. This all made a lot of sense back in
> the days when drm drivers this this fancy shadow attaching to allow
> drm to use a driver for rendering cooperatively with a fbdev driver.
> Today there's not much reason for that anymore imo, and I'd welcome
> patches to allow drivers to simply register a drm device (and remove
> all the newer registration functions for usb/platform/whatever
> drivers, moving the device handling into drivers). Note that it's a
> bit work, since not-really-required abstraction (which was useful back
> when the drm drivers have been shared with *BSD, but pointless now)
> like the drm irq support needs to be moved away to a pci-dev legacy
> thing only - it doesn't really buy a kms driver anything above
> calling request_irq() itself.
>
> So feel free to burn this down, I'll be happy to carry wood to the
> pyre in the from of reviews (not much time for more right now ...).

Part of the reasons I haven't tackled this is that for drm/i915 we
can't use this alone (since we still need to support the shadow attach
for old ums), but I regard it as a smaller part of the general
midlayer/inversion of control problem in the drm driver setup/teardown
sequence. Which all results in ridiculous amounts of races between the
interfaces regsiter in the drm core (dev node, sysfs files, debugfs
files) and the driver itself. Especially module unload is totally
broken.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergstr?m  
wrote:
> 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?

Note that I'm not too happy about the fact that drm wants a struct
device to register a drm device. This all made a lot of sense back in
the days when drm drivers this this fancy shadow attaching to allow
drm to use a driver for rendering cooperatively with a fbdev driver.
Today there's not much reason for that anymore imo, and I'd welcome
patches to allow drivers to simply register a drm device (and remove
all the newer registration functions for usb/platform/whatever
drivers, moving the device handling into drivers). Note that it's a
bit work, since not-really-required abstraction (which was useful back
when the drm drivers have been shared with *BSD, but pointless now)
like the drm irq support needs to be moved away to a pci-dev legacy
thing only - it doesn't really buy a kms driver anything above
calling request_irq() itself.

So feel free to burn this down, I'll be happy to carry wood to the
pyre in the from of reviews (not much time for more right now ...).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Lucas Stach]

Am Mittwoch, den 05.12.2012, 13:47 +0200 schrieb Terje Bergstr?m:
[...]
> 
> > 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.
> 
> 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?
> 
I also think the introduction of a dummy platform device to aggregate
the various DRM devices would be the best way to keep a clean interface
between host1x and the tegradrm parts.

But I haven't thought through how to instantiate such a dummy device
without clobbering the device tree and sprinkling global data all over
the driver. Perhaps the host1x driver could instantiate such a device
and only provide a single API entry point to make this dummy-drm-device
known to it's subdevices. This way we don't have to move all the drm
specific aggregation of devices into host1x and could keep the API a bit
leaner. Obviously this solution would not get around the circular
dependency completely, but would cut down on it a bit.

Regards,
Lucas

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

[Thierry Reding]

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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Terje Bergström]

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?

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.

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.

host1x driver itself has only little use for the list of clients.
Basically we need only a list of channels, and platform devices
associated with channels, to be able to dump host1x channel state.

Mind you, I believe nvhost driver as part of our BSP has had quite many
more hours of runtime than tegradrm. :-)

Terje

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

[Thierry Reding]

On Mon, Nov 26, 2012 at 03:19:12PM +0200, Terje Bergstrom wrote:
> From: Arto Merilainen 
> 
> This patch removes the redundant host1x driver from tegradrm and
> makes necessary bindings to the separate host driver.
> 
> This modification introduces a regression: Because there is no
> general framework for attaching separate devices into the
> same address space, this patch removes the ability to use IOMMU
> in tegradrm.
> 
> Signed-off-by: Arto Merilainen 
> Signed-off-by: Terje Bergstrom 

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.

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

[Thierry Reding]

On Mon, Nov 26, 2012 at 03:19:12PM +0200, Terje Bergstrom wrote:
 From: Arto Merilainen amerilai...@nvidia.com
 
 This patch removes the redundant host1x driver from tegradrm and
 makes necessary bindings to the separate host driver.
 
 This modification introduces a regression: Because there is no
 general framework for attaching separate devices into the
 same address space, this patch removes the ability to use IOMMU
 in tegradrm.
 
 Signed-off-by: Arto Merilainen amerilai...@nvidia.com
 Signed-off-by: Terje Bergstrom tbergst...@nvidia.com

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?

Thierry


pgpNgdQZX5da3.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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


pgp99SlbfRpUI.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 05.12.2012 13:13, Thierry Reding wrote:
 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.

Ok.

 
 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.

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?

 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.

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?

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas Stach]

Am Mittwoch, den 05.12.2012, 13:47 +0200 schrieb Terje Bergström:
[...]
 
  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.
 
 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?
 
I also think the introduction of a dummy platform device to aggregate
the various DRM devices would be the best way to keep a clean interface
between host1x and the tegradrm parts.

But I haven't thought through how to instantiate such a dummy device
without clobbering the device tree and sprinkling global data all over
the driver. Perhaps the host1x driver could instantiate such a device
and only provide a single API entry point to make this dummy-drm-device
known to it's subdevices. This way we don't have to move all the drm
specific aggregation of devices into host1x and could keep the API a bit
leaner. Obviously this solution would not get around the circular
dependency completely, but would cut down on it a bit.

Regards,
Lucas

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergström tbergst...@nvidia.com wrote:
 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?

Note that I'm not too happy about the fact that drm wants a struct
device to register a drm device. This all made a lot of sense back in
the days when drm drivers this this fancy shadow attaching to allow
drm to use a driver for rendering cooperatively with a fbdev driver.
Today there's not much reason for that anymore imo, and I'd welcome
patches to allow drivers to simply register a drm device (and remove
all the newer registration functions for usb/platform/whatever
drivers, moving the device handling into drivers). Note that it's a
bit work, since not-really-required abstraction (which was useful back
when the drm drivers have been shared with *BSD, but pointless now)
like the drm irq support needs to be moved away to a pci-dev legacy
thing only - it doesn't really buy a kms driver anything abovebeyond
calling request_irq() itself.

So feel free to burn this down, I'll be happy to carry wood to the
pyre in the from of reviews (not much time for more right now ...).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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


pgps1NvYpd4Zf.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 1:03 PM, Daniel Vetter dan...@ffwll.ch wrote:
 On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergström tbergst...@nvidia.com 
 wrote:
 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?

 Note that I'm not too happy about the fact that drm wants a struct
 device to register a drm device. This all made a lot of sense back in
 the days when drm drivers this this fancy shadow attaching to allow
 drm to use a driver for rendering cooperatively with a fbdev driver.
 Today there's not much reason for that anymore imo, and I'd welcome
 patches to allow drivers to simply register a drm device (and remove
 all the newer registration functions for usb/platform/whatever
 drivers, moving the device handling into drivers). Note that it's a
 bit work, since not-really-required abstraction (which was useful back
 when the drm drivers have been shared with *BSD, but pointless now)
 like the drm irq support needs to be moved away to a pci-dev legacy
 thing only - it doesn't really buy a kms driver anything abovebeyond
 calling request_irq() itself.

 So feel free to burn this down, I'll be happy to carry wood to the
 pyre in the from of reviews (not much time for more right now ...).

Part of the reasons I haven't tackled this is that for drm/i915 we
can't use this alone (since we still need to support the shadow attach
for old ums), but I regard it as a smaller part of the general
midlayer/inversion of control problem in the drm driver setup/teardown
sequence. Which all results in ridiculous amounts of races between the
interfaces regsiter in the drm core (dev node, sysfs files, debugfs
files) and the driver itself. Especially module unload is totally
broken.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Wed, Dec 05, 2012 at 01:03:14PM +0100, Daniel Vetter wrote:
 On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergström tbergst...@nvidia.com 
 wrote:
  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?
 
 Note that I'm not too happy about the fact that drm wants a struct
 device to register a drm device. This all made a lot of sense back in
 the days when drm drivers this this fancy shadow attaching to allow
 drm to use a driver for rendering cooperatively with a fbdev driver.
 Today there's not much reason for that anymore imo, and I'd welcome
 patches to allow drivers to simply register a drm device (and remove
 all the newer registration functions for usb/platform/whatever
 drivers, moving the device handling into drivers). Note that it's a
 bit work, since not-really-required abstraction (which was useful back
 when the drm drivers have been shared with *BSD, but pointless now)
 like the drm irq support needs to be moved away to a pci-dev legacy
 thing only - it doesn't really buy a kms driver anything abovebeyond
 calling request_irq() itself.
 
 So feel free to burn this down, I'll be happy to carry wood to the
 pyre in the from of reviews (not much time for more right now ...).

This wouldn't solve the problem at hand, though, since we'd still need
to instantiate the DRM driver somehow. Currently this is triggered
ultimately by the host1x's .probe().

Maybe something more elaborate could help, though. Suppose we add
functionality to DRM to instantiate a DRM device. We could call such a
function from the host1x driver to add a device which the tegra-drm
driver could bind to. This would entail something like a small bus
implementation for DRM that would allow matching DRM devices to DRM
drivers much like the platform or PCI busses. This could automatically
take care of registering the DRM device with the proper parent so that
host1x clients can lookup the host1x via dev-parent.

Perhaps something as simple as

int drm_device_register(struct device *parent, const char *name, int 
id);

could work. Or something more elaborate such as allocating a device with

struct drm_device *drm_device_alloc(const char *name, int id);

paired with

int drm_device_register(struct drm_device *dev);

would be more flexible in that drivers could modify the DRM device in
between the two calls.

Thierry


pgp8WXyS8ECfI.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 1:22 PM, Thierry Reding
thierry.red...@avionic-design.de wrote:
 Maybe something more elaborate could help, though. Suppose we add
 functionality to DRM to instantiate a DRM device. We could call such a
 function from the host1x driver to add a device which the tegra-drm
 driver could bind to. This would entail something like a small bus
 implementation for DRM that would allow matching DRM devices to DRM
 drivers much like the platform or PCI busses. This could automatically
 take care of registering the DRM device with the proper parent so that
 host1x clients can lookup the host1x via dev-parent.

 Perhaps something as simple as

 int drm_device_register(struct device *parent, const char *name, int 
 id);

 could work. Or something more elaborate such as allocating a device with

 struct drm_device *drm_device_alloc(const char *name, int id);

 paired with

 int drm_device_register(struct drm_device *dev);

 would be more flexible in that drivers could modify the DRM device in
 between the two calls.

Imo that's worse, since now drm manages even more of the driver-hw
binding process. In my dream world the drm driver just registers a
normal driver at module load time directly with whatever bus it's
interested in, and then, from it the bus' -probe callback setups up
the entire driver, calling down into drm to setup up interfaces to
userspace (dev node, sysfs, and whatever is required to implement the
ioctls) and uses the various helper libraries provided. So host1x
providing a virtual device that tegradrm can match sounds much better
(if that helps in decoupling from host1x).

Or simply call the tegradrm setup from host1x directly, creating a
depency on the tegradrm module. When trying to unload host1x it can
then also call down into tegradrm to tear down the drm device.
Afterwards you should be able to unload tegradrm without fuzz. And if
the hard dependcy is an issue for other host1x clients this
setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

On Wed, Dec 05, 2012 at 01:31:54PM +0100, Daniel Vetter wrote:
 On Wed, Dec 5, 2012 at 1:22 PM, Thierry Reding
 thierry.red...@avionic-design.de wrote:
  Maybe something more elaborate could help, though. Suppose we add
  functionality to DRM to instantiate a DRM device. We could call such a
  function from the host1x driver to add a device which the tegra-drm
  driver could bind to. This would entail something like a small bus
  implementation for DRM that would allow matching DRM devices to DRM
  drivers much like the platform or PCI busses. This could automatically
  take care of registering the DRM device with the proper parent so that
  host1x clients can lookup the host1x via dev-parent.
 
  Perhaps something as simple as
 
  int drm_device_register(struct device *parent, const char *name, 
  int id);
 
  could work. Or something more elaborate such as allocating a device with
 
  struct drm_device *drm_device_alloc(const char *name, int id);
 
  paired with
 
  int drm_device_register(struct drm_device *dev);
 
  would be more flexible in that drivers could modify the DRM device in
  between the two calls.
 
 Imo that's worse, since now drm manages even more of the driver-hw
 binding process. In my dream world the drm driver just registers a
 normal driver at module load time directly with whatever bus it's
 interested in, and then, from it the bus' -probe callback setups up
 the entire driver, calling down into drm to setup up interfaces to
 userspace (dev node, sysfs, and whatever is required to implement the
 ioctls) and uses the various helper libraries provided. So host1x
 providing a virtual device that tegradrm can match sounds much better
 (if that helps in decoupling from host1x).

Okay, now I see where you're going. You want to replace the various
drm_*_init() functions with something more fine-grained that does the
initialization manually in each driver. Is that it? The obvious
disadvantage is that a lot of code would have to be duplicated, though
that can presumably be factored out into further helpers if necessary.

On that note, I just noticed that drm_platform_init() actually binds a
single platform_device to the drm_driver, which isn't going to work very
well in case there are two devices that want to use the same driver. It
would be nice to get rid of that limitation as well while at it.

 Or simply call the tegradrm setup from host1x directly, creating a
 depency on the tegradrm module. When trying to unload host1x it can
 then also call down into tegradrm to tear down the drm device.
 Afterwards you should be able to unload tegradrm without fuzz. And if
 the hard dependcy is an issue for other host1x clients this
 setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.

This is what I originally proposed. However, since tegra-drm will need
to call functions provided by host1x we have a cyclic dependency.
Wouldn't that prevent either module from being unloaded?

Thierry


pgpskmN4vbLCN.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergström]

On 05.12.2012 14:04, Thierry Reding wrote:
 On Wed, Dec 05, 2012 at 01:47:38PM +0200, Terje Bergström wrote:
 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.

Sure, that feedback is in the todo list. For some parts I already have a
proposed solution, but for chip ops not yet.

 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.

Oops, I must've checked the status from a stale tree. Thanks for that.
In this case there's no technical reason why host1x couldn't act as the
device to register for drm, as drm wouldn't touch host1x device data in
any way.

How about if we treat the host1x driver as utility library, and move the
code for host1x probe altogether to tegradrm/host1x.c? I haven't yet
checked how bad the dependencies between host1x's driver's host1x.c and
the rest of the driver are, so I'm not sure if it's feasible and if
there would be a clear design. Just tossing in ideas.

That would solve the circular dependency problem. I've already committed
to contents of host1x.c being very different in downstream and upstream,
so this change would just emphasize that.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Dec 5, 2012 at 2:28 PM, Thierry Reding
thierry.red...@avionic-design.de wrote:
 Imo that's worse, since now drm manages even more of the driver-hw
 binding process. In my dream world the drm driver just registers a
 normal driver at module load time directly with whatever bus it's
 interested in, and then, from it the bus' -probe callback setups up
 the entire driver, calling down into drm to setup up interfaces to
 userspace (dev node, sysfs, and whatever is required to implement the
 ioctls) and uses the various helper libraries provided. So host1x
 providing a virtual device that tegradrm can match sounds much better
 (if that helps in decoupling from host1x).

 Okay, now I see where you're going. You want to replace the various
 drm_*_init() functions with something more fine-grained that does the
 initialization manually in each driver. Is that it? The obvious
 disadvantage is that a lot of code would have to be duplicated, though
 that can presumably be factored out into further helpers if necessary.

 On that note, I just noticed that drm_platform_init() actually binds a
 single platform_device to the drm_driver, which isn't going to work very
 well in case there are two devices that want to use the same driver. It
 would be nice to get rid of that limitation as well while at it.

Yeah, it's the lack of generality that irks me, and writing driver
init code is one giant sequence of setup function calls anyway -
sprinkling 1-2 more doesn't really matter, but helps a lot if it
results in the driver being in full control (e.g. if you need to
reorder due to some special requirement, that's much easier to do then
than with the current hoop-jumping). But like I've said, a bit a
bigger fish to fry, just wanted to point you into that direction ...

 Or simply call the tegradrm setup from host1x directly, creating a
 depency on the tegradrm module. When trying to unload host1x it can
 then also call down into tegradrm to tear down the drm device.
 Afterwards you should be able to unload tegradrm without fuzz. And if
 the hard dependcy is an issue for other host1x clients this
 setup/teardown could be wrapped into an #ifdef CONFIG_TEGRADRM.

 This is what I originally proposed. However, since tegra-drm will need
 to call functions provided by host1x we have a cyclic dependency.
 Wouldn't that prevent either module from being unloaded?

You could pass down a host1x interface struct with a vtable to
tegradrm (plus some static inline helpers to make those not a pain to
call). The other possibility (and I'm not proud at all of that code)
which we've used in the intel-ips driver is to use symbol_get at
runtime - but there the requirement was explicitly that intel-ips
needs to work on server systems without the drm/i915 driver loaded,
but still always have the support for interacting with it compiled in
(to make distros happy). It's all rather awkward though ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Terje Bergstrom]

From: Arto Merilainen 

This patch removes the redundant host1x driver from tegradrm and
makes necessary bindings to the separate host driver.

This modification introduces a regression: Because there is no
general framework for attaching separate devices into the
same address space, this patch removes the ability to use IOMMU
in tegradrm.

Signed-off-by: Arto Merilainen 
Signed-off-by: Terje Bergstrom 
---
 drivers/gpu/drm/tegra/Kconfig  |8 +-
 drivers/gpu/drm/tegra/Makefile |2 +-
 drivers/gpu/drm/tegra/dc.c |   22 +--
 drivers/gpu/drm/tegra/drm.c|  207 +++-
 drivers/gpu/drm/tegra/drm.h|   55 ++-
 drivers/gpu/drm/tegra/dsi.c|   24 ++-
 drivers/gpu/drm/tegra/fb.c |   26 ++-
 drivers/gpu/drm/tegra/hdmi.c   |   24 ++-
 drivers/gpu/drm/tegra/host1x.c |  343 
 drivers/gpu/drm/tegra/tvo.c|   33 ++--
 10 files changed, 246 insertions(+), 498 deletions(-)
 delete mode 100644 drivers/gpu/drm/tegra/host1x.c

diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
index affd741..4a0290e 100644
--- a/drivers/gpu/drm/tegra/Kconfig
+++ b/drivers/gpu/drm/tegra/Kconfig
@@ -1,6 +1,6 @@
 config DRM_TEGRA
tristate "NVIDIA Tegra DRM"
-   depends on DRM && OF && ARCH_TEGRA
+   depends on DRM && OF && ARCH_TEGRA && TEGRA_HOST1X
select DRM_KMS_HELPER
select DRM_GEM_CMA_HELPER
select DRM_KMS_CMA_HELPER
@@ -20,10 +20,4 @@ config DRM_TEGRA_DEBUG
help
  Say yes here to enable debugging support.

-config DRM_TEGRA_IOMMU
-   bool "NVIDIA Tegra DRM IOMMU support"
-   help
- Say yes here to enable the use of the IOMMU to allocate and
- map memory buffers.
-
 endif
diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
index e6e96af..57a334d 100644
--- a/drivers/gpu/drm/tegra/Makefile
+++ b/drivers/gpu/drm/tegra/Makefile
@@ -1,7 +1,7 @@
 ccflags-y := -Iinclude/drm
 ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG

-tegra-drm-y := drm.o fb.o dc.o host1x.o
+tegra-drm-y := drm.o fb.o dc.o
 tegra-drm-y += output.o rgb.o hdmi.o tvo.o dsi.o
 tegra-drm-y += plane.o

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 3a16e93..1779008 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 

 #include 

@@ -673,10 +674,10 @@ static int tegra_dc_debugfs_exit(struct tegra_dc *dc)
return 0;
 }

-static int tegra_dc_drm_init(struct host1x_client *client,
+static int tegra_dc_drm_init(struct tegra_drm_client *client,
 struct drm_device *drm)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;

dc->pipe = drm->mode_config.num_crtc;
@@ -712,9 +713,9 @@ static int tegra_dc_drm_init(struct host1x_client *client,
return 0;
 }

-static int tegra_dc_drm_exit(struct host1x_client *client)
+static int tegra_dc_drm_exit(struct tegra_drm_client *client)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;

devm_free_irq(dc->dev, dc->irq, dc);
@@ -734,14 +735,13 @@ static int tegra_dc_drm_exit(struct host1x_client *client)
return 0;
 }

-static const struct host1x_client_ops dc_client_ops = {
+static const struct tegra_drm_client_ops dc_client_ops = {
.drm_init = tegra_dc_drm_init,
.drm_exit = tegra_dc_drm_exit,
 };

 static int tegra_dc_probe(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
struct resource *regs;
struct tegra_dc *dc;
int err;
@@ -791,13 +791,14 @@ static int tegra_dc_probe(struct platform_device *pdev)
return err;
}

-   err = host1x_register_client(host1x, >client);
+   err = tegra_drm_register_client(>client);
if (err < 0) {
-   dev_err(>dev, "failed to register host1x client: %d\n",
+   dev_err(>dev, "failed to register tegra drm client: %d\n",
err);
return err;
}

+   host1x_busy(pdev);
platform_set_drvdata(pdev, dc);

return 0;
@@ -805,13 +806,12 @@ static int tegra_dc_probe(struct platform_device *pdev)

 static int tegra_dc_remove(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
struct tegra_dc *dc = platform_get_drvdata(pdev);
int err;

-   err = host1x_unregister_client(host1x, >client);
+   err = tegra_drm_unregister_client(>client);
if (err < 0) {
-   dev_err(>dev, "failed to unregister host1x client: %d\n",
+   dev_err(>dev, "failed to unregister tegra_drm client: 
%d\n",
err);
return err;
   

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

[Terje Bergstrom]

From: Arto Merilainen amerilai...@nvidia.com

This patch removes the redundant host1x driver from tegradrm and
makes necessary bindings to the separate host driver.

This modification introduces a regression: Because there is no
general framework for attaching separate devices into the
same address space, this patch removes the ability to use IOMMU
in tegradrm.

Signed-off-by: Arto Merilainen amerilai...@nvidia.com
Signed-off-by: Terje Bergstrom tbergst...@nvidia.com
---
 drivers/gpu/drm/tegra/Kconfig  |8 +-
 drivers/gpu/drm/tegra/Makefile |2 +-
 drivers/gpu/drm/tegra/dc.c |   22 +--
 drivers/gpu/drm/tegra/drm.c|  207 +++-
 drivers/gpu/drm/tegra/drm.h|   55 ++-
 drivers/gpu/drm/tegra/dsi.c|   24 ++-
 drivers/gpu/drm/tegra/fb.c |   26 ++-
 drivers/gpu/drm/tegra/hdmi.c   |   24 ++-
 drivers/gpu/drm/tegra/host1x.c |  343 
 drivers/gpu/drm/tegra/tvo.c|   33 ++--
 10 files changed, 246 insertions(+), 498 deletions(-)
 delete mode 100644 drivers/gpu/drm/tegra/host1x.c

diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
index affd741..4a0290e 100644
--- a/drivers/gpu/drm/tegra/Kconfig
+++ b/drivers/gpu/drm/tegra/Kconfig
@@ -1,6 +1,6 @@
 config DRM_TEGRA
tristate NVIDIA Tegra DRM
-   depends on DRM  OF  ARCH_TEGRA
+   depends on DRM  OF  ARCH_TEGRA  TEGRA_HOST1X
select DRM_KMS_HELPER
select DRM_GEM_CMA_HELPER
select DRM_KMS_CMA_HELPER
@@ -20,10 +20,4 @@ config DRM_TEGRA_DEBUG
help
  Say yes here to enable debugging support.
 
-config DRM_TEGRA_IOMMU
-   bool NVIDIA Tegra DRM IOMMU support
-   help
- Say yes here to enable the use of the IOMMU to allocate and
- map memory buffers.
-
 endif
diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
index e6e96af..57a334d 100644
--- a/drivers/gpu/drm/tegra/Makefile
+++ b/drivers/gpu/drm/tegra/Makefile
@@ -1,7 +1,7 @@
 ccflags-y := -Iinclude/drm
 ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG
 
-tegra-drm-y := drm.o fb.o dc.o host1x.o
+tegra-drm-y := drm.o fb.o dc.o
 tegra-drm-y += output.o rgb.o hdmi.o tvo.o dsi.o
 tegra-drm-y += plane.o
 
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 3a16e93..1779008 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -12,6 +12,7 @@
 #include linux/module.h
 #include linux/of.h
 #include linux/platform_device.h
+#include linux/nvhost.h
 
 #include mach/clk.h
 
@@ -673,10 +674,10 @@ static int tegra_dc_debugfs_exit(struct tegra_dc *dc)
return 0;
 }
 
-static int tegra_dc_drm_init(struct host1x_client *client,
+static int tegra_dc_drm_init(struct tegra_drm_client *client,
 struct drm_device *drm)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;
 
dc-pipe = drm-mode_config.num_crtc;
@@ -712,9 +713,9 @@ static int tegra_dc_drm_init(struct host1x_client *client,
return 0;
 }
 
-static int tegra_dc_drm_exit(struct host1x_client *client)
+static int tegra_dc_drm_exit(struct tegra_drm_client *client)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;
 
devm_free_irq(dc-dev, dc-irq, dc);
@@ -734,14 +735,13 @@ static int tegra_dc_drm_exit(struct host1x_client *client)
return 0;
 }
 
-static const struct host1x_client_ops dc_client_ops = {
+static const struct tegra_drm_client_ops dc_client_ops = {
.drm_init = tegra_dc_drm_init,
.drm_exit = tegra_dc_drm_exit,
 };
 
 static int tegra_dc_probe(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev-dev.parent);
struct resource *regs;
struct tegra_dc *dc;
int err;
@@ -791,13 +791,14 @@ static int tegra_dc_probe(struct platform_device *pdev)
return err;
}
 
-   err = host1x_register_client(host1x, dc-client);
+   err = tegra_drm_register_client(dc-client);
if (err  0) {
-   dev_err(pdev-dev, failed to register host1x client: %d\n,
+   dev_err(pdev-dev, failed to register tegra drm client: %d\n,
err);
return err;
}
 
+   host1x_busy(pdev);
platform_set_drvdata(pdev, dc);
 
return 0;
@@ -805,13 +806,12 @@ static int tegra_dc_probe(struct platform_device *pdev)
 
 static int tegra_dc_remove(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev-dev.parent);
struct tegra_dc *dc = platform_get_drvdata(pdev);
int err;
 
-   err = host1x_unregister_client(host1x, dc-client);
+   err = tegra_drm_unregister_client(dc-client);
if (err  0) {
-   dev_err(pdev-dev, failed to unregister host1x client: %d\n,
+