| Message ID | 1353935954-13763-7-git-send-email-tbergstrom@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 26, 2012 at 03:19:12PM +0200, Terje Bergstrom wrote: > From: Arto Merilainen <amerilainen@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 <amerilainen@nvidia.com> > Signed-off-by: Terje Bergstrom <tbergstrom@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
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
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
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
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
On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergström <tbergstrom@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 above&beyond 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
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
On Wed, Dec 5, 2012 at 1:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Dec 5, 2012 at 12:47 PM, Terje Bergström <tbergstrom@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 above&beyond > 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
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 <tbergstrom@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 above&beyond > 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
On Wed, Dec 5, 2012 at 1:22 PM, Thierry Reding <thierry.reding@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
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.reding@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
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
On Wed, Dec 5, 2012 at 2:28 PM, Thierry Reding <thierry.reding@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
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.reding@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
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
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
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
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
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
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.
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
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
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.
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
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
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 0xffffffff. But that is still something that can be handled inside tegradrm without involving the host1x driver. Terje
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.
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
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
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.
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
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.
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
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
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
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
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, ...)
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.
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
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
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.
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
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
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.
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
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.
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
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
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", + dev_err(&pdev->dev, "failed to unregister tegra_drm client: %d\n", err); return err; } diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 4a306c2..cba2d1d 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -24,44 +24,137 @@ #define DRIVER_MINOR 0 #define DRIVER_PATCHLEVEL 0 -#ifdef CONFIG_DRM_TEGRA_IOMMU -#define TEGRA_DRM_IOMMU_BASE_ADDR 0x20000000 -#define TEGRA_DRM_IOMMU_SIZE 0x10000000 -#endif +static LIST_HEAD(tegra_drm_subdrv_list); +static LIST_HEAD(tegra_drm_subdrv_required); -static int tegra_drm_load(struct drm_device *drm, unsigned long flags) +struct tegra_drm_client_entry { + struct device_node *np; + struct list_head list; +}; + +static int tegra_drm_add_client(struct device_node *np) +{ + struct tegra_drm_client_entry *client; + + client = kzalloc(sizeof(*client), GFP_KERNEL); + if (!client) + return -ENOMEM; + + INIT_LIST_HEAD(&client->list); + client->np = of_node_get(np); + + list_add_tail(&client->list, &tegra_drm_subdrv_required); + + return 0; +} + +static int tegra_drm_parse_dt(void) { - struct device *dev = drm->dev; - struct host1x *host1x; + static const char * const compat[] = { + "nvidia,tegra20-dc", + "nvidia,tegra20-hdmi", + "nvidia,tegra20-tvo", + "nvidia,tegra20-dsi", + "nvidia,tegra30-dc", + "nvidia,tegra30-hdmi", + "nvidia,tegra30-tvo", + "nvidia,tegra30-dsi" + }; + unsigned int i; int err; + struct device *dev; - host1x = dev_get_drvdata(dev); - drm->dev_private = host1x; - host1x->drm = drm; + /* host1x is parent of all devices */ + dev = bus_find_device_by_name(&platform_bus_type, NULL, "host1x"); + if (!dev) + return -ENODEV; - drm_mode_config_init(drm); + /* find devices that are available and add them into the 'required' + * list */ + for (i = 0; i < ARRAY_SIZE(compat); i++) { + struct device_node *np; - err = host1x_drm_init(host1x, drm); - if (err < 0) - return err; + for_each_child_of_node(dev->of_node, np) { + if (of_device_is_compatible(np, compat[i]) && + of_device_is_available(np)) { + err = tegra_drm_add_client(np); + if (err < 0) + return err; + } + } + } -#ifdef CONFIG_DRM_TEGRA_IOMMU - host1x->dim = arm_iommu_create_mapping(&platform_bus_type, - TEGRA_DRM_IOMMU_BASE_ADDR, - TEGRA_DRM_IOMMU_SIZE, 0); - if (IS_ERR_OR_NULL(host1x->dim)) { - dev_err(dev, "%s: Create iommu mapping failed: %ld\n", __func__, - PTR_ERR(host1x->dim)); - return PTR_ERR(host1x->dim); + return 0; +} + +int tegra_drm_register_client(struct tegra_drm_client *client) +{ + struct tegra_drm_client_entry *drm, *tmp; + int err; + + list_add_tail(&client->list, &tegra_drm_subdrv_list); + + /* remove this device from 'required' list */ + list_for_each_entry_safe(drm, tmp, &tegra_drm_subdrv_required, list) + if (drm->np == client->dev->of_node) + list_del(&drm->list); + + /* if all required devices are found, register drm device */ + if (list_empty(&tegra_drm_subdrv_required)) { + struct platform_device *pdev = to_platform_device(client->dev); + + err = drm_platform_init(&tegra_drm_driver, pdev); + if (err < 0) { + dev_err(client->dev, "drm_platform_init(): %d\n", err); + return err; + } } - err = arm_iommu_attach_device(drm->dev, host1x->dim); - if (err < 0) { - dev_err(dev, "%s: Attach iommu device failed: %d\n", __func__, - err); - return err; + return 0; +} + +int tegra_drm_unregister_client(struct tegra_drm_client *client) +{ + list_for_each_entry(client, &tegra_drm_subdrv_list, list) { + + struct platform_device *pdev = to_platform_device(client->dev); + + if (client->ops && client->ops->drm_exit) { + int err = client->ops->drm_exit(client); + if (err < 0) { + dev_err(client->dev, + "DRM cleanup failed for %s: %d\n", + dev_name(client->dev), err); + return err; + } + } + + /* if this is the last device, unregister the drm driver */ + if (client->list.next == &tegra_drm_subdrv_list) + drm_platform_exit(&tegra_drm_driver, pdev); + + list_del_init(&client->list); + } + + return 0; +} + +static int tegra_drm_load(struct drm_device *drm, unsigned long flags) +{ + struct tegra_drm_client *client; + int err; + + drm_mode_config_init(drm); + + list_for_each_entry(client, &tegra_drm_subdrv_list, list) { + if (client->ops && client->ops->drm_init) { + int err = client->ops->drm_init(client, drm); + if (err < 0) { + dev_dbg(drm->dev, "drm_init() failed for %s: %d\n", + dev_name(client->dev), err); + } + } } -#endif err = tegra_drm_fb_init(drm); if (err < 0) @@ -74,18 +167,9 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags) static int tegra_drm_unload(struct drm_device *drm) { -#ifdef CONFIG_DRM_TEGRA_IOMMU - struct host1x *host1x = dev_get_drvdata(drm->dev); -#endif - drm_kms_helper_poll_fini(drm); tegra_drm_fb_exit(drm); -#ifdef CONFIG_DRM_TEGRA_IOMMU - if (host1x->dim) - arm_iommu_release_mapping(host1x->dim); -#endif - drm_mode_config_cleanup(drm); return 0; @@ -98,10 +182,55 @@ static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp) static void tegra_drm_lastclose(struct drm_device *drm) { - struct host1x *host1x = drm->dev_private; + tegra_drm_fb_restore(drm); +} - drm_fbdev_cma_restore_mode(host1x->fbdev); +static int __init tegra_drm_init(void) +{ + int err; + + tegra_drm_parse_dt(); + + err = platform_driver_register(&tegra_dc_driver); + if (err < 0) + return err; + + err = platform_driver_register(&tegra_hdmi_driver); + if (err < 0) + goto unregister_dc; + + err = platform_driver_register(&tegra_tvo_driver); + if (err < 0) + goto unregister_hdmi; + + err = platform_driver_register(&tegra_dsi_driver); + if (err < 0) + goto unregister_tvo; + + return 0; + +unregister_tvo: + platform_driver_unregister(&tegra_tvo_driver); +unregister_hdmi: + platform_driver_unregister(&tegra_hdmi_driver); +unregister_dc: + platform_driver_unregister(&tegra_dc_driver); + return err; } +module_init(tegra_drm_init); + +static void __exit tegra_drm_exit(void) +{ + platform_driver_unregister(&tegra_dsi_driver); + platform_driver_unregister(&tegra_tvo_driver); + platform_driver_unregister(&tegra_hdmi_driver); + platform_driver_unregister(&tegra_dc_driver); +} +module_exit(tegra_drm_exit); + +MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>"); +MODULE_DESCRIPTION("NVIDIA Tegra DRM driver"); +MODULE_LICENSE("GPL"); static struct drm_ioctl_desc tegra_drm_ioctls[] = { }; diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index c7079ff..b2f9f10 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -31,59 +31,31 @@ static inline struct tegra_framebuffer *to_tegra_fb(struct drm_framebuffer *fb) return container_of(fb, struct tegra_framebuffer, base); } -struct host1x { - struct drm_device *drm; - struct device *dev; - void __iomem *regs; - struct clk *clk; - int syncpt; - int irq; - - struct mutex drm_clients_lock; - struct list_head drm_clients; - struct list_head drm_active; - - struct mutex clients_lock; - struct list_head clients; +struct tegra_drm_client; - struct drm_fbdev_cma *fbdev; - struct tegra_framebuffer fb; - -#ifdef CONFIG_DRM_TEGRA_IOMMU - struct dma_iommu_mapping *dim; -#endif -}; - -struct host1x_client; - -struct host1x_client_ops { - int (*drm_init)(struct host1x_client *client, struct drm_device *drm); - int (*drm_exit)(struct host1x_client *client); +struct tegra_drm_client_ops { + int (*drm_init)(struct tegra_drm_client *client, + struct drm_device *drm); + int (*drm_exit)(struct tegra_drm_client *client); }; -struct host1x_client { - struct host1x *host1x; +struct tegra_drm_client { struct device *dev; - const struct host1x_client_ops *ops; + const struct tegra_drm_client_ops *ops; struct list_head list; -}; -extern int host1x_drm_init(struct host1x *host1x, struct drm_device *drm); -extern int host1x_drm_exit(struct host1x *host1x); +}; -extern int host1x_register_client(struct host1x *host1x, - struct host1x_client *client); -extern int host1x_unregister_client(struct host1x *host1x, - struct host1x_client *client); +extern int tegra_drm_register_client(struct tegra_drm_client *client); +extern int tegra_drm_unregister_client(struct tegra_drm_client *client); struct tegra_output; struct tegra_dc { - struct host1x_client client; + struct tegra_drm_client client; - struct host1x *host1x; struct device *dev; struct drm_crtc base; @@ -103,7 +75,8 @@ struct tegra_dc { struct dentry *debugfs; }; -static inline struct tegra_dc *host1x_client_to_dc(struct host1x_client *client) +static inline struct tegra_dc *tegra_drm_client_to_dc( + struct tegra_drm_client *client) { return container_of(client, struct tegra_dc, client); } @@ -246,8 +219,8 @@ extern struct vm_operations_struct tegra_gem_vm_ops; /* from fb.c */ extern int tegra_drm_fb_init(struct drm_device *drm); extern void tegra_drm_fb_exit(struct drm_device *drm); +extern void tegra_drm_fb_restore(struct drm_device *drm); -extern struct platform_driver tegra_host1x_driver; extern struct platform_driver tegra_hdmi_driver; extern struct platform_driver tegra_tvo_driver; extern struct platform_driver tegra_dsi_driver; diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c index 156b3753..4f4c709 100644 --- a/drivers/gpu/drm/tegra/dsi.c +++ b/drivers/gpu/drm/tegra/dsi.c @@ -15,7 +15,7 @@ #include "drm.h" struct tegra_dsi { - struct host1x_client client; + struct tegra_drm_client client; struct tegra_output output; void __iomem *regs; @@ -23,7 +23,7 @@ struct tegra_dsi { }; static inline struct tegra_dsi * -host1x_client_to_dsi(struct host1x_client *client) +tegra_drm_client_to_dsi(struct tegra_drm_client *client) { return container_of(client, struct tegra_dsi, client); } @@ -59,10 +59,10 @@ static const struct tegra_output_ops dsi_ops = { .disable = tegra_output_dsi_disable, }; -static int tegra_dsi_drm_init(struct host1x_client *client, +static int tegra_dsi_drm_init(struct tegra_drm_client *client, struct drm_device *drm) { - struct tegra_dsi *dsi = host1x_client_to_dsi(client); + struct tegra_dsi *dsi = tegra_drm_client_to_dsi(client); int err; dsi->output.type = TEGRA_OUTPUT_DSI; @@ -78,9 +78,9 @@ static int tegra_dsi_drm_init(struct host1x_client *client, return 0; } -static int tegra_dsi_drm_exit(struct host1x_client *client) +static int tegra_dsi_drm_exit(struct tegra_drm_client *client) { - struct tegra_dsi *dsi = host1x_client_to_dsi(client); + struct tegra_dsi *dsi = tegra_drm_client_to_dsi(client); int err; err = tegra_output_exit(&dsi->output); @@ -92,14 +92,13 @@ static int tegra_dsi_drm_exit(struct host1x_client *client) return 0; } -static const struct host1x_client_ops dsi_client_ops = { +static const struct tegra_drm_client_ops dsi_client_ops = { .drm_init = tegra_dsi_drm_init, .drm_exit = tegra_dsi_drm_exit, }; static int tegra_dsi_probe(struct platform_device *pdev) { - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_dsi *dsi; struct resource *regs; int err; @@ -126,9 +125,9 @@ static int tegra_dsi_probe(struct platform_device *pdev) INIT_LIST_HEAD(&dsi->client.list); dsi->client.dev = &pdev->dev; - err = host1x_register_client(host1x, &dsi->client); + err = tegra_drm_register_client(&dsi->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; } @@ -140,13 +139,12 @@ static int tegra_dsi_probe(struct platform_device *pdev) static int tegra_dsi_remove(struct platform_device *pdev) { - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_dsi *dsi = platform_get_drvdata(pdev); int err; - err = host1x_unregister_client(host1x, &dsi->client); + err = tegra_drm_unregister_client(&dsi->client); if (err < 0) { - dev_err(&pdev->dev, "failed to unregister host1x client: %d\n", + dev_err(&pdev->dev, "failed to unregister tegra drm client: %d\n", err); return err; } diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c index 97993c6..d6f44fa 100644 --- a/drivers/gpu/drm/tegra/fb.c +++ b/drivers/gpu/drm/tegra/fb.c @@ -9,11 +9,11 @@ #include "drm.h" +static struct drm_fbdev_cma *tegra_fbdev; + static void tegra_drm_fb_output_poll_changed(struct drm_device *drm) { - struct host1x *host1x = drm->dev_private; - - drm_fbdev_cma_hotplug_event(host1x->fbdev); + drm_fbdev_cma_hotplug_event(tegra_fbdev); } static const struct drm_mode_config_funcs tegra_drm_mode_funcs = { @@ -23,9 +23,6 @@ static const struct drm_mode_config_funcs tegra_drm_mode_funcs = { int tegra_drm_fb_init(struct drm_device *drm) { - struct host1x *host1x = drm->dev_private; - struct drm_fbdev_cma *fbdev; - drm->mode_config.min_width = 0; drm->mode_config.min_height = 0; @@ -34,23 +31,24 @@ int tegra_drm_fb_init(struct drm_device *drm) drm->mode_config.funcs = &tegra_drm_mode_funcs; - fbdev = drm_fbdev_cma_init(drm, 32, drm->mode_config.num_crtc, + tegra_fbdev = drm_fbdev_cma_init(drm, 32, drm->mode_config.num_crtc, drm->mode_config.num_connector); - if (IS_ERR(fbdev)) - return PTR_ERR(fbdev); + if (IS_ERR(tegra_fbdev)) + return PTR_ERR(tegra_fbdev); #ifndef CONFIG_FRAMEBUFFER_CONSOLE - drm_fbdev_cma_restore_mode(fbdev); + drm_fbdev_cma_restore_mode(tegra_fbdev); #endif - host1x->fbdev = fbdev; - return 0; } void tegra_drm_fb_exit(struct drm_device *drm) { - struct host1x *host1x = drm->dev_private; + drm_fbdev_cma_fini(tegra_fbdev); +} - drm_fbdev_cma_fini(host1x->fbdev); +void tegra_drm_fb_restore(struct drm_device *drm) +{ + drm_fbdev_cma_restore_mode(tegra_fbdev); } diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index 58f55dc..b2b8e58 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -22,7 +22,7 @@ #include "dc.h" struct tegra_hdmi { - struct host1x_client client; + struct tegra_drm_client client; struct tegra_output output; struct device *dev; @@ -46,7 +46,7 @@ struct tegra_hdmi { }; static inline struct tegra_hdmi * -host1x_client_to_hdmi(struct host1x_client *client) +tegra_drm_client_to_hdmi(struct tegra_drm_client *client) { return container_of(client, struct tegra_hdmi, client); } @@ -1152,10 +1152,10 @@ static int tegra_hdmi_debugfs_exit(struct tegra_hdmi *hdmi) return 0; } -static int tegra_hdmi_drm_init(struct host1x_client *client, +static int tegra_hdmi_drm_init(struct tegra_drm_client *client, struct drm_device *drm) { - struct tegra_hdmi *hdmi = host1x_client_to_hdmi(client); + struct tegra_hdmi *hdmi = tegra_drm_client_to_hdmi(client); int err; hdmi->output.type = TEGRA_OUTPUT_HDMI; @@ -1177,9 +1177,9 @@ static int tegra_hdmi_drm_init(struct host1x_client *client, return 0; } -static int tegra_hdmi_drm_exit(struct host1x_client *client) +static int tegra_hdmi_drm_exit(struct tegra_drm_client *client) { - struct tegra_hdmi *hdmi = host1x_client_to_hdmi(client); + struct tegra_hdmi *hdmi = tegra_drm_client_to_hdmi(client); int err; if (IS_ENABLED(CONFIG_DEBUG_FS)) { @@ -1204,14 +1204,13 @@ static int tegra_hdmi_drm_exit(struct host1x_client *client) return 0; } -static const struct host1x_client_ops hdmi_client_ops = { +static const struct tegra_drm_client_ops hdmi_client_ops = { .drm_init = tegra_hdmi_drm_init, .drm_exit = tegra_hdmi_drm_exit, }; static int tegra_hdmi_probe(struct platform_device *pdev) { - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_hdmi *hdmi; struct resource *regs; int err; @@ -1286,9 +1285,9 @@ static int tegra_hdmi_probe(struct platform_device *pdev) INIT_LIST_HEAD(&hdmi->client.list); hdmi->client.dev = &pdev->dev; - err = host1x_register_client(host1x, &hdmi->client); + err = tegra_drm_register_client(&hdmi->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; } @@ -1300,13 +1299,12 @@ static int tegra_hdmi_probe(struct platform_device *pdev) static int tegra_hdmi_remove(struct platform_device *pdev) { - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_hdmi *hdmi = platform_get_drvdata(pdev); int err; - err = host1x_unregister_client(host1x, &hdmi->client); + err = tegra_drm_unregister_client(&hdmi->client); if (err < 0) { - dev_err(&pdev->dev, "failed to unregister host1x client: %d\n", + dev_err(&pdev->dev, "failed to unregister tegra drm client: %d\n", err); return err; } diff --git a/drivers/gpu/drm/tegra/host1x.c b/drivers/gpu/drm/tegra/host1x.c deleted file mode 100644 index f9d3a84..0000000 --- a/drivers/gpu/drm/tegra/host1x.c +++ /dev/null @@ -1,343 +0,0 @@ -/* - * Copyright (C) 2012 Avionic Design GmbH - * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 as - * published by the Free Software Foundation. - */ - -#include <linux/clk.h> -#include <linux/err.h> -#include <linux/module.h> -#include <linux/of.h> -#include <linux/platform_device.h> - -#include "drm.h" - -struct host1x_drm_client { - struct host1x_client *client; - struct device_node *np; - struct list_head list; -}; - -static int host1x_add_drm_client(struct host1x *host1x, struct device_node *np) -{ - struct host1x_drm_client *client; - - client = kzalloc(sizeof(*client), GFP_KERNEL); - if (!client) - return -ENOMEM; - - INIT_LIST_HEAD(&client->list); - client->np = of_node_get(np); - - list_add_tail(&client->list, &host1x->drm_clients); - - return 0; -} - -static int host1x_activate_drm_client(struct host1x *host1x, - struct host1x_drm_client *drm, - struct host1x_client *client) -{ - mutex_lock(&host1x->drm_clients_lock); - list_del_init(&drm->list); - list_add_tail(&drm->list, &host1x->drm_active); - drm->client = client; - mutex_unlock(&host1x->drm_clients_lock); - - return 0; -} - -static int host1x_remove_drm_client(struct host1x *host1x, - struct host1x_drm_client *client) -{ - mutex_lock(&host1x->drm_clients_lock); - list_del_init(&client->list); - mutex_unlock(&host1x->drm_clients_lock); - - of_node_put(client->np); - kfree(client); - - return 0; -} - -static int host1x_parse_dt(struct host1x *host1x) -{ - static const char * const compat[] = { - "nvidia,tegra20-dc", - "nvidia,tegra20-hdmi", - "nvidia,tegra20-tvo", - "nvidia,tegra20-dsi", - "nvidia,tegra30-dc", - "nvidia,tegra30-hdmi", - "nvidia,tegra30-tvo", - "nvidia,tegra30-dsi" - }; - unsigned int i; - int err; - - for (i = 0; i < ARRAY_SIZE(compat); i++) { - struct device_node *np; - - for_each_child_of_node(host1x->dev->of_node, np) { - if (of_device_is_compatible(np, compat[i]) && - of_device_is_available(np)) { - err = host1x_add_drm_client(host1x, np); - if (err < 0) - return err; - } - } - } - - return 0; -} - -static int tegra_host1x_probe(struct platform_device *pdev) -{ - struct host1x *host1x; - struct resource *regs; - int err; - - host1x = devm_kzalloc(&pdev->dev, sizeof(*host1x), GFP_KERNEL); - if (!host1x) - return -ENOMEM; - - mutex_init(&host1x->drm_clients_lock); - INIT_LIST_HEAD(&host1x->drm_clients); - INIT_LIST_HEAD(&host1x->drm_active); - mutex_init(&host1x->clients_lock); - INIT_LIST_HEAD(&host1x->clients); - host1x->dev = &pdev->dev; - - err = host1x_parse_dt(host1x); - if (err < 0) { - dev_err(&pdev->dev, "failed to parse DT: %d\n", err); - return err; - } - - host1x->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(host1x->clk)) - return PTR_ERR(host1x->clk); - - err = clk_prepare_enable(host1x->clk); - if (err < 0) - return err; - - regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!regs) { - err = -ENXIO; - goto err; - } - - err = platform_get_irq(pdev, 0); - if (err < 0) - goto err; - - host1x->syncpt = err; - - err = platform_get_irq(pdev, 1); - if (err < 0) - goto err; - - host1x->irq = err; - - host1x->regs = devm_request_and_ioremap(&pdev->dev, regs); - if (!host1x->regs) { - err = -EADDRNOTAVAIL; - goto err; - } - - platform_set_drvdata(pdev, host1x); - - return 0; - -err: - clk_disable_unprepare(host1x->clk); - return err; -} - -static int tegra_host1x_remove(struct platform_device *pdev) -{ - struct host1x *host1x = platform_get_drvdata(pdev); - - clk_disable_unprepare(host1x->clk); - - return 0; -} - -int host1x_drm_init(struct host1x *host1x, struct drm_device *drm) -{ - struct host1x_client *client; - - mutex_lock(&host1x->clients_lock); - - list_for_each_entry(client, &host1x->clients, list) { - if (client->ops && client->ops->drm_init) { - int err = client->ops->drm_init(client, drm); - if (err < 0) { - dev_err(host1x->dev, - "DRM setup failed for %s: %d\n", - dev_name(client->dev), err); - return err; - } - } - } - - mutex_unlock(&host1x->clients_lock); - - return 0; -} - -int host1x_drm_exit(struct host1x *host1x) -{ - struct platform_device *pdev = to_platform_device(host1x->dev); - struct host1x_client *client; - - if (!host1x->drm) - return 0; - - mutex_lock(&host1x->clients_lock); - - list_for_each_entry_reverse(client, &host1x->clients, list) { - if (client->ops && client->ops->drm_exit) { - int err = client->ops->drm_exit(client); - if (err < 0) { - dev_err(host1x->dev, - "DRM cleanup failed for %s: %d\n", - dev_name(client->dev), err); - return err; - } - } - } - - mutex_unlock(&host1x->clients_lock); - - drm_platform_exit(&tegra_drm_driver, pdev); - host1x->drm = NULL; - - return 0; -} - -int host1x_register_client(struct host1x *host1x, struct host1x_client *client) -{ - struct host1x_drm_client *drm, *tmp; - int err; - - mutex_lock(&host1x->clients_lock); - list_add_tail(&client->list, &host1x->clients); - mutex_unlock(&host1x->clients_lock); - - list_for_each_entry_safe(drm, tmp, &host1x->drm_clients, list) - if (drm->np == client->dev->of_node) - host1x_activate_drm_client(host1x, drm, client); - - if (list_empty(&host1x->drm_clients)) { - struct platform_device *pdev = to_platform_device(host1x->dev); - - err = drm_platform_init(&tegra_drm_driver, pdev); - if (err < 0) { - dev_err(host1x->dev, "drm_platform_init(): %d\n", err); - return err; - } - } - - return 0; -} - -int host1x_unregister_client(struct host1x *host1x, - struct host1x_client *client) -{ - struct host1x_drm_client *drm, *tmp; - int err; - - list_for_each_entry_safe(drm, tmp, &host1x->drm_active, list) { - if (drm->client == client) { - err = host1x_drm_exit(host1x); - if (err < 0) { - dev_err(host1x->dev, "host1x_drm_exit(): %d\n", - err); - return err; - } - - host1x_remove_drm_client(host1x, drm); - break; - } - } - - mutex_lock(&host1x->clients_lock); - list_del_init(&client->list); - mutex_unlock(&host1x->clients_lock); - - return 0; -} - -static struct of_device_id tegra_host1x_of_match[] = { - { .compatible = "nvidia,tegra20-host1x", }, - { .compatible = "nvidia,tegra30-host1x", }, - { }, -}; -MODULE_DEVICE_TABLE(of, tegra_host1x_of_match); - -struct platform_driver tegra_host1x_driver = { - .driver = { - .name = "tegra-host1x", - .owner = THIS_MODULE, - .of_match_table = tegra_host1x_of_match, - }, - .probe = tegra_host1x_probe, - .remove = tegra_host1x_remove, -}; - -static int __init tegra_host1x_init(void) -{ - int err; - - err = platform_driver_register(&tegra_host1x_driver); - if (err < 0) - return err; - - err = platform_driver_register(&tegra_dc_driver); - if (err < 0) - goto unregister_host1x; - - err = platform_driver_register(&tegra_hdmi_driver); - if (err < 0) - goto unregister_dc; - - err = platform_driver_register(&tegra_tvo_driver); - if (err < 0) - goto unregister_hdmi; - - err = platform_driver_register(&tegra_dsi_driver); - if (err < 0) - goto unregister_tvo; - - return 0; - -unregister_tvo: - platform_driver_unregister(&tegra_tvo_driver); -unregister_hdmi: - platform_driver_unregister(&tegra_hdmi_driver); -unregister_dc: - platform_driver_unregister(&tegra_dc_driver); -unregister_host1x: - platform_driver_unregister(&tegra_host1x_driver); - return err; -} -module_init(tegra_host1x_init); - -static void __exit tegra_host1x_exit(void) -{ - platform_driver_unregister(&tegra_dsi_driver); - platform_driver_unregister(&tegra_tvo_driver); - platform_driver_unregister(&tegra_hdmi_driver); - platform_driver_unregister(&tegra_dc_driver); - platform_driver_unregister(&tegra_host1x_driver); -} -module_exit(tegra_host1x_exit); - -MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>"); -MODULE_DESCRIPTION("NVIDIA Tegra DRM driver"); -MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/tegra/tvo.c b/drivers/gpu/drm/tegra/tvo.c index a67bd28..01ac356 100644 --- a/drivers/gpu/drm/tegra/tvo.c +++ b/drivers/gpu/drm/tegra/tvo.c @@ -15,7 +15,7 @@ #include "drm.h" struct tegra_tvo { - struct host1x_client client; + struct tegra_drm_client client; struct tegra_output output; void __iomem *regs; @@ -24,7 +24,7 @@ struct tegra_tvo { }; static inline struct tegra_tvo * -host1x_client_to_tvo(struct host1x_client *client) +tegra_drm_client_to_tvo(struct tegra_drm_client *client) { return container_of(client, struct tegra_tvo, client); } @@ -60,10 +60,10 @@ static const struct tegra_output_ops tvo_ops = { .disable = tegra_output_tvo_disable, }; -static int tegra_tvo_drm_init(struct host1x_client *client, +static int tegra_tvo_drm_init(struct tegra_drm_client *client, struct drm_device *drm) { - struct tegra_tvo *tvo = host1x_client_to_tvo(client); + struct tegra_tvo *tvo = tegra_drm_client_to_tvo(client); int err; tvo->output.type = TEGRA_OUTPUT_TVO; @@ -79,9 +79,9 @@ static int tegra_tvo_drm_init(struct host1x_client *client, return 0; } -static int tegra_tvo_drm_exit(struct host1x_client *client) +static int tegra_tvo_drm_exit(struct tegra_drm_client *client) { - struct tegra_tvo *tvo = host1x_client_to_tvo(client); + struct tegra_tvo *tvo = tegra_drm_client_to_tvo(client); int err; err = tegra_output_exit(&tvo->output); @@ -93,14 +93,13 @@ static int tegra_tvo_drm_exit(struct host1x_client *client) return err; } -static const struct host1x_client_ops tvo_client_ops = { +static const struct tegra_drm_client_ops tvo_client_ops = { .drm_init = tegra_tvo_drm_init, .drm_exit = tegra_tvo_drm_exit, }; static int tegra_tvo_probe(struct platform_device *pdev) { - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_tvo *tvo; struct resource *regs; int err; @@ -120,22 +119,25 @@ static int tegra_tvo_probe(struct platform_device *pdev) return -ENXIO; err = platform_get_irq(pdev, 0); - if (err < 0) + if (err < 0) { + dev_err(&pdev->dev, "failed to get tvo irq\n"); return err; - + } tvo->irq = err; tvo->regs = devm_request_and_ioremap(&pdev->dev, regs); - if (!tvo->regs) + if (!tvo->regs) { + dev_err(&pdev->dev, "failed to request tvo regs\n"); return -EADDRNOTAVAIL; + } tvo->client.ops = &tvo_client_ops; INIT_LIST_HEAD(&tvo->client.list); tvo->client.dev = &pdev->dev; - err = host1x_register_client(host1x, &tvo->client); + err = tegra_drm_register_client(&tvo->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; } @@ -147,13 +149,12 @@ static int tegra_tvo_probe(struct platform_device *pdev) static int tegra_tvo_remove(struct platform_device *pdev) { - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_tvo *tvo = platform_get_drvdata(pdev); int err; - err = host1x_unregister_client(host1x, &tvo->client); + err = tegra_drm_unregister_client(&tvo->client); if (err < 0) { - dev_err(&pdev->dev, "failed to unregister host1x client: %d\n", + dev_err(&pdev->dev, "failed to unregister tegra drm client: %d\n", err); return err; }