Re: [PATCH v2 4/7] drivers/base: Add interface framework

From: Andrzej Hajda
Date: Tue May 20 2014 - 05:28:17 EST

Next message: Stephane Eranian: "Re: [PATCH] perf: Fix mux_interval hrtimer wreckage"
Previous message: Michal Malý: "Re: [PATCH v4 01/24] input: Add ff-memless-next module"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Thierry, Greg,

On 05/15/2014 10:53 AM, Thierry Reding wrote:
> On Tue, May 13, 2014 at 05:32:15PM -0700, Greg Kroah-Hartman wrote:
>> On Tue, May 13, 2014 at 07:57:13PM +0200, Daniel Vetter wrote:
>>> On Tue, May 13, 2014 at 05:30:47PM +0200, Thierry Reding wrote:
>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@xxxxxxxxxxxxxxxx>
>>>>
>>>> Some drivers, such as graphics drivers in the DRM subsystem, do not have
>>>> a real device that they can bind to. They are often composed of several
>>>> devices, each having their own driver. The master/component framework
>>>> can be used in these situations to collect the devices pertaining to one
>>>> logical device, wait until all of them have registered and then bind
>>>> them all at once.
>>>>
>>>> For some situations this is only a partial solution. An implementation
>>>> of a master still needs to be registered with the system somehow. Many
>>>> drivers currently resort to creating a dummy device that a driver can
>>>> bind to and register the master against. This is problematic since it
>>>> requires (and presumes) knowledge about the system within drivers.
>>>>
>>>> Furthermore there are setups where a suitable device already exists, but
>>>> is already bound to a driver. For example, on Tegra the following device
>>>> tree extract (simplified) represents the host1x device along with child
>>>> devices:
>>>>
>>>> host1x {
>>>> display-controller {
>>>> ...
>>>> };
>>>>
>>>> display-controller {
>>>> ...
>>>> };
>>>>
>>>> hdmi {
>>>> ...
>>>> };
>>>>
>>>> dsi {
>>>> ...
>>>> };
>>>>
>>>> csi {
>>>> ...
>>>> };
>>>>
>>>> video-input {
>>>> ...
>>>> };
>>>> };
>>>>
>>>> Each of the child devices is in turn a client of host1x, in that it can
>>>> request resources (command stream DMA channels and syncpoints) from it.
>>>> To implement the DMA channel and syncpoint infrastructure, host1x comes
>>>> with its own driver. Children are implemented in separate drivers. In
>>>> Linux this set of devices would be exposed by DRM and V4L2 drivers.
>>>>
>>>> However, neither the DRM nor the V4L2 drivers have a single device that
>>>> they can bind to. The DRM device is composed of the display controllers
>>>> and the various output devices, whereas the V4L2 device is composed of
>>>> one or more video input devices.
>>>>
>>>> This patch introduces the concept of an interface and drivers that can
>>>> bind to a given interface. An interface can be exposed by any device,
>>>> and interface drivers can bind to these interfaces. Multiple drivers can
>>>> bind against a single interface. When a device is removed, interfaces
>>>> exposed by it will be removed as well, thereby removing the drivers that
>>>> were bound to those interfaces.
>>>>
>>>> In the example above, the host1x device would expose the "tegra-host1x"
>>>> interface. DRM and V4L2 drivers can then bind to that interface and
>>>> instantiate the respective subsystem objects from there.
>>>>
>>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@xxxxxxxxxxxxxxxx>
>>>> ---
>>>> Note that I'd like to merge this through the Tegra DRM tree so that the
>>>> changes to the Tegra DRM driver later in this series can be merged at
>>>> the same time and are not delayed for another release cycle.
>>>>
>>>> In particular that means that I'm looking for an Acked-by from Greg.
>>>>
>>>> drivers/base/Makefile | 2 +-
>>>> drivers/base/interface.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/interface.h | 40 ++++++++++
>>>> 3 files changed, 227 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/base/interface.c
>>>> create mode 100644 include/linux/interface.h
>>>
>>> Hm, this interface stuff smells like bus drivers light. Should we instead
>>> have a pile of helpers to make creating new buses with match methods more
>>> trivial? There's a fairly big pile of small use-cases where this might be
>>> useful. In your case here all the host1x children would sit on a host1x
>>> bus. Admittedly I didn't look into the details.
>>
>> I have no problem adding such "bus-light" functions, to make it easier
>> to create and implement a bus in the driver core, as I know it's really
>> heavy. That's been on my "todo" list for over a decade now...

I have posted some times ago RFC for interface_tracker framework [1].
It seems with interface_tracker you can achieve similar functionality
and it seems to be more generic.

[1]: https://lkml.org/lkml/2014/4/30/345

Two small things should be added to interface_tracker framework:
- matching objects using string comparison,
- before notifier unregistration call notifier to inform that observed
interface will disappear for him.

Greg, this is another use case for interface_tracker framework.

To show how it could be achieved I present pseudo patch which converts
tegra drivers to interface_tracker. Obviously I have not tested it.

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 11d0deb..79fcb43 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -728,27 +728,27 @@ static const struct component_master_ops
tegra_drm_master_ops = {
.unbind = tegra_drm_unbind,
};

-static int host1x_interface_bind(struct interface *intf)
-{
- return component_master_add(intf->dev, &tegra_drm_master_ops);
-}
-
-static void host1x_interface_unbind(struct interface *intf)
-{
- component_master_del(intf->dev, &tegra_drm_master_ops);
-}
-
-static struct interface_driver host1x_interface_driver = {
- .name = "nvidia,tegra-host1x",
- .bind = host1x_interface_bind,
- .unbind = host1x_interface_unbind,
-};
+void itb_callback(struct interface_tracker_block *itb, const void *object,
+ unsigned long type, bool on, void *data)
+{
+ struct device *dev = data;
+
+ if (on)
+ component_master_add(dev, &tegra_drm_master_ops);
+ else
+ component_master_del(dev, &tegra_drm_master_ops);
+}
+static struct interface_tracker_block itb = {
+ .callback = itb_callback
+};

static int __init tegra_drm_init(void)
{
int err;

- err = interface_register_driver(&host1x_interface_driver);
+ err = interface_tracker_register("nvidia,tegra-host1x",
+ INTERFACE_TRACKER_TYPE_PARENT_DEV, &itb);
if (err < 0)
return err;

@@ -795,7 +795,8 @@ unregister_dsi:
unregister_dc:
platform_driver_unregister(&tegra_dc_driver);
unregister_host1x:
- interface_unregister_driver(&host1x_interface_driver);
+ interface_tracker_unregister("nvidia,tegra-host1x",
+ INTERFACE_TRACKER_TYPE_PARENT_DEV, &itb);
return err;
}
module_init(tegra_drm_init);
@@ -809,7 +810,8 @@ static void __exit tegra_drm_exit(void)
platform_driver_unregister(&tegra_sor_driver);
platform_driver_unregister(&tegra_dsi_driver);
platform_driver_unregister(&tegra_dc_driver);
- interface_unregister_driver(&host1x_interface_driver);
+ interface_tracker_unregister("nvidia,tegra-host1x",
+ INTERFACE_TRACKER_TYPE_PARENT_DEV, &itb);
}
module_exit(tegra_drm_exit);

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index b92812b..f4455c5 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -175,10 +175,9 @@ static int host1x_probe(struct platform_device *pdev)

host1x_debug_init(host);

- host->interface.name = "nvidia,tegra-host1x";
- host->interface.dev = &pdev->dev;
-
- err = interface_add(&host->interface);
+ err = interface_tracker_ifup("nvidia,tegra-host1x",
+ INTERFACE_TRACKER_TYPE_PARENT_DEV, &pdev->dev);
if (err < 0)
goto fail_deinit_intr;

@@ -197,7 +196,9 @@ static int host1x_remove(struct platform_device *pdev)
{
struct host1x *host = platform_get_drvdata(pdev);

- interface_remove(&host->interface);
+ err = interface_tracker_ifdown("nvidia,tegra-host1x",
+ INTERFACE_TRACKER_TYPE_PARENT_DEV, &pdev->dev);
host1x_intr_deinit(host);
host1x_syncpt_deinit(host);
clk_disable_unprepare(host->clk);

Regards
Andrzej

>
> Greg,
>
> Do you think you could find the time to look at this patch in a little
> more detail? This isn't about creating a light alternative to busses at
> all. It is an attempt to solve a different problem.
>
> Consider the following: you have a collection of hardware devices that
> together can implement functionality in a given subsystem such as DRM or
> V4L2. In many cases all these devices have their own driver and they are
> glued together using the component helpers. This results in a situation
> where people are instantiating dummy devices for the sole purpose of
> getting a driver probed, since all of the existing devices have already
> had a driver bind to them.
>
> Another downside of using dummy devices is that they mess up the device
> hierarchy. All of a sudden you have a situation where the dummy device
> is the logical parent for its aunts and uncles (or siblings).
>
> The solution proposed here is to allow any device to expose an interface
> that interface drivers can bind to. This removes the need for dummy
> devices. As opposed to device drivers, an interface can be bound to by
> multiple drivers. That's a feature that is needed specifically by host1x
> on Tegra since two drivers need to hang off of the host1x device (DRM
> and V4L2), but I can easily imagine this to be useful in other cases.
> Interfaces are exposed explicitly at probe time by the device drivers
> for the devices they drive.
>
> Even though this was designed with the above use-case in mind I can
> imagine this to be useful for other things as well. For example a set of
> generic interfaces could be created and devices (or even whole classes
> of devices) could be made to expose these interfaces. This would cause
> interfaces to be created for each of these devices. That's functionality
> similar to what can be done with notifiers, albeit somewhat more
> structured. That could for example be useful to apply policy to a class
> of devices or collect statistics, etc.
>
> If you think that I'm on a wild-goose chase, please let me know so that
> I don't waste any more time on this.
>
> Thierry
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Stephane Eranian: "Re: [PATCH] perf: Fix mux_interval hrtimer wreckage"
Previous message: Michal Malý: "Re: [PATCH v4 01/24] input: Add ff-memless-next module"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]