[PATCH 09/10] MCDE: Add build files and bus

[PATCH 09/10] MCDE: Add build files and bus

[Marcus LORENTZON]

Hi Arnd,
Comments inline.

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: den 12 november 2010 17:23
> To: linux-arm-kernel at lists.infradead.org
> Cc: Jimmy RUBIN; linux-fbdev at vger.kernel.org; linux-
> media at vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
> Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
> 
> On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > This patch adds support for the MCDE, Memory-to-display controller,
> > found in the ST-Ericsson ux500 products.
> >
> > This patch adds the necessary build files for MCDE and the bus that
> > all displays are connected to.
> >
> 
> Can you explain why you need a bus for this?

The idea of the bus is to make it easier to add new panel/display support using the normal device/driver model. Boards add devices at init, and drivers are selected in config. If we were to model the "real physical" bus structure it would be very complex, hard to use. We use our own bus, but it's really a virtual bus for adding display devices and drivers to MCDE host. Is there any "generic virtual bus type" we should use instead of our own type? If you have another idea of how to add display devices and drivers without MCDE code modifications, please let us know. A virtual bus just give us a nice framework to do this.

> With the code you currently have, there is only a single driver
> associated
> with this bus type, and also just a single device that gets registered
> here!

And yes, currently there's only one single driver. But there's a lot more coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And once all different u8500 boards are pushed, there will be multiple boards registering devices with different setups. But one thing at a time.

> >+static int __init mcde_subsystem_init(void)
> >+{
> >+       int ret;
> >+       pr_info("MCDE subsystem init begin\n");
> >+
> >+       /* MCDE module init sequence */
> >+       ret = mcde_init();
> >+       if (ret)
> >+               return ret;
> >+       ret = mcde_display_init();
> >+       if (ret)
> >+               goto mcde_display_failed;
> >+       ret = mcde_dss_init();
> >+       if (ret)
> >+               goto mcde_dss_failed;
> >+       ret = mcde_fb_init();
> >+       if (ret)
> >+               goto mcde_fb_failed;
> >+       pr_info("MCDE subsystem init done\n");
> >+
> >+       return 0;
> >+mcde_fb_failed:
> >+       mcde_dss_exit();
> >+mcde_dss_failed:
> >+       mcde_display_exit();
> >+mcde_display_failed:
> >+       mcde_exit();
> >+       return ret;
> >+}
> 
> Splitting up the module into four sub-modules and then initializing
> everything from one place indicates that something is done wrong
> on a global scale.
> 
> If you indeed need a bus, that should be a separate module that gets
> loaded first and then has the other modules build on top of.

Yes, that's the general idea. We don't completely understand the correct way of making sure how one module gets loaded before another, without selecting init level, like the fs_initcall below you commented on. I guess most of our submodules should be initialized during subsys_init. But we have not found how to specify the module init order inside subsys init level. Maybe you can shed some light on this? Makefile order seems like a fragile way of defining init order dependencies.
Do you think static init calls from machine subsys init are a better solution?

> I'm not sure how the other parts layer on top of one another, can you
> provide some more insight?

+----------------------------+
| mcde_fb/mcde_kms/mcde_v4l2 |
+---------------+------------+
|    mcde_dss   |
+   +-----------+
|   | disp drvs |
+---+-----------+
|    mcde hw    |
+---------------+
|      HW       |
+---------------+

> From what I understood so far, you have a single multi-channel display
> controller (mcde_hw.c) that drives the hardware.
> Each controller can have multiple frame buffers attached to it, which
> in turn can have multiple displays attached to each of them, but your
> current configuration only has one of each, right?

Correct, channels A/B (crtcs) can have two blended "framebuffers" plus background color, channels C0/C1 can have one framebuffer.

> Right now you have a single top-level bus device for the displays,
> maybe that can be integrated into the controller so the displays are
> properly rooted below the hardware that drives them.

Not sure I understand 100%. Routing is independent of bus structure. Routing could change dynamically during runtime reconfiguration using future KMS for example. Bus is only for connecting display devices with drivers. Of course we could have one bus per port/connector. But then the code would be more complex and we would end up with one bus/device/driver per connector (or in some rare cases 2-4 on DBI/DSI connectors).

> The frame buffer device also looks weird. Right now you only seem
> to have a single frame buffer registered to a driver in the same
> module. Is that frame buffer not dependent on a controller?

MCDE framebuffers are only depending on MCDE DSS. DSS is the API that will be used by all user space APIs so that we don't have to duplicate the common code. We are planning mcde_kms and mcde_v4l2 drivers on top of MCDE DSS(=Display Sub System).

> >+#ifdef MODULE
> >+module_init(mcde_subsystem_init);
> >+#else
> >+fs_initcall(mcde_subsystem_init);
> >+#endif
> 
> This is not a file system ;-)

Yes, we know :)

Any comments on how to control the init order of multiple modules with init order dependencies are very welcome.

/BR
/Marcus

[PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

On Thursday 25 November 2010, Marcus LORENTZON wrote:
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> > On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > > This patch adds support for the MCDE, Memory-to-display controller,
> > > found in the ST-Ericsson ux500 products.
> > >
> > > This patch adds the necessary build files for MCDE and the bus that
> > > all displays are connected to.
> > >
> > 
> > Can you explain why you need a bus for this?
> 
> The idea of the bus is to make it easier to add new panel/display support
> using the normal device/driver model. Boards add devices at init, and
> drivers are selected in config. If we were to model the "real physical"
> bus structure it would be very complex, hard to use. We use our own bus,
> but it's really a virtual bus for adding display devices and drivers to 
> MCDE host. Is there any "generic virtual bus type" we should use instead
> of our own type? If you have another idea of how to add display devices
> and drivers without MCDE code modifications, please let us know. A virtual
> bus just give us a nice framework to do this.

All devices that you cannot probe by asking hardware or firmware are normally
considered platform devices. Then again, a platform device is usally
identified by its resources, i.e. MMIO addresses and interrupts, which
I guess your display does not have.

> > With the code you currently have, there is only a single driver
> > associated
> > with this bus type, and also just a single device that gets registered
> > here!
> 
> And yes, currently there's only one single driver. But there's a lot more
> coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And once
> all different u8500 boards are pushed, there will be multiple boards
> registering devices with different setups. But one thing at a time.

Your approach is making it hard to review the code in context. Adding a
device driver that uses existing infrastructure is usually straightforward,
but adding infrastructure is a hard thing and needs to be done with great
care, especially if you add infrastructure before it actually is needed.

Staging it in a way that adds all the display drivers later than the
base driver is a good idea, but it would be helpful to also add the
infrastructure at the later stage. Maybe you can try to simplify the
code for now by hardcoding the single display and remove the dynamic
registration. You still have the the code, so once the base code looks
good for inclusion, we can talk about it in the context of adding
dynamic display support back in, possibly in exactly the way you are
proposing now, but perhaps in an entirely different way if we come up
with a better solution.

On a more abstract level, when you want to add large chunks of code
to the kernel, you often cannot find anyone to review and understand
all of it at once. Splitting a subsystem into ten patches by file
level rarely helps, so instead you should ideally have a series of
patches that each add working code that enable more features.

This is of course more work for you, especially if you did not consider
it while writing the code in the first place. Still, you should
always try hard to make code easy to review as much as possible,
because you need to work with reviewers both to get code in and to
make sure you make the quality ends up as good as possible.

> > >+static int __init mcde_subsystem_init(void)
> > >+{
> > >+       int ret;
> > >+       pr_info("MCDE subsystem init begin\n");
> > >+
> > >+       /* MCDE module init sequence */
> > >+       ret = mcde_init();
> > >+       if (ret)
> > >+               return ret;
> > >+       ret = mcde_display_init();
> > >+       if (ret)
> > >+               goto mcde_display_failed;
> > >+       ret = mcde_dss_init();
> > >+       if (ret)
> > >+               goto mcde_dss_failed;
> > >+       ret = mcde_fb_init();
> > >+       if (ret)
> > >+               goto mcde_fb_failed;
> > >+       pr_info("MCDE subsystem init done\n");
> > >+
> > >+       return 0;
> > >+mcde_fb_failed:
> > >+       mcde_dss_exit();
> > >+mcde_dss_failed:
> > >+       mcde_display_exit();
> > >+mcde_display_failed:
> > >+       mcde_exit();
> > >+       return ret;
> > >+}
> > 
> > Splitting up the module into four sub-modules and then initializing
> > everything from one place indicates that something is done wrong
> > on a global scale.
> > 
> > If you indeed need a bus, that should be a separate module that gets
> > loaded first and then has the other modules build on top of.
> 
> Yes, that's the general idea. We don't completely understand the correct
> way of making sure how one module gets loaded before another, without
> selecting init level, like the fs_initcall below you commented on. I
> guess most of our submodules should be initialized during subsys_init.
> But we have not found how to specify the module init order inside subsys
> init level. Maybe you can shed some light on this? Makefile order seems
> like a fragile way of defining init order dependencies.
> Do you think static init calls from machine subsys init are a better solution?

In general, the idea with loadable modules is that you can only load
them in initialization order because of the symbol dependencies: The bus
gets loaded first and it exports symbols used by the device drivers,
so a device driver can be sure that the bus is initialized entirely.

For the case where all modules are built-in, you can rely in link-order
in the Makefile, e.g.

obj-$(CONFIG_FOO_BASE)		+= foo_base.o
obj-$(CONFIG_FOO_SPECIFIC)	+= foo_specific.o # this comes after foo_base

> > I'm not sure how the other parts layer on top of one another, can you
> > provide some more insight?
> 
> +----------------------------+
> | mcde_fb/mcde_kms/mcde_v4l2 |
> +---------------+------------+
> |    mcde_dss   |
> +   +-----------+
> |   | disp drvs |
> +---+-----------+
> |    mcde hw    |
> +---------------+
> |      HW       |
> +---------------+

Ok. One problem with this is that once you have a multitude of
display drivers, you can no longer layer them below mcde_dss.

Having the kms/fb/v4l2 drivers on top definitely makes sense, so
these should all be able to be standalone loadable modules.
I do not understand why you have a v4l2 driver at all, or why
you need both fb and kms drivers, but that is probably because
of my ignorance of display device drivers.

> > From what I understood so far, you have a single multi-channel display
> > controller (mcde_hw.c) that drives the hardware.
> > Each controller can have multiple frame buffers attached to it, which
> > in turn can have multiple displays attached to each of them, but your
> > current configuration only has one of each, right?
> 
> Correct, channels A/B (crtcs) can have two blended "framebuffers" plus
> background color, channels C0/C1 can have one framebuffer.

We might still be talking about different things here, not sure.
 
> > Right now you have a single top-level bus device for the displays,
> > maybe that can be integrated into the controller so the displays are
> > properly rooted below the hardware that drives them.
> 
> Not sure I understand 100%. Routing is independent of bus structure.
> Routing could change dynamically during runtime reconfiguration using
> future KMS for example. Bus is only for connecting display devices
> with drivers. Of course we could have one bus per port/connector.
> But then the code would be more complex and we would end up with one
> bus/device/driver per connector (or in some rare cases 2-4 on DBI/DSI
> connectors).

I mean 'root' as in the root of your device tree, where the tree
is modeled after the logical layout of your hardware.

Looking at the representation in sysfs, you should probably aim
for something like

/sys/devices/axi/axi0/mcde_controller
				/chnlA
					/dspl_crtc0
						/fb0
						/fb1
						/v4l_0
					/dspl_dbi0
						/fb2
						/v4l_1
				/chnlB
					/dspl_ctrc1
						/fb3
				/chnlC
					/dspl_lcd0
						/fb4
						/v4l_2

Not sure if that is close to what your hardware would really
look like. My point is that all the objects that you are
dealing with as a device driver should be represented hierarchically
according to how you probe them.

Assuming the structure above is correct and you cannot probe
any of this by looking at registers, you would put a description
of it into the a data structure (ideally a flattened device tree
or a section of one) and let the probing happen:

* The axi core registers an mcde controller as device axi0.
* udev matches the device and loads the mcde hw driver from
  user space
* the hw driver creates a device for each channel, and passes
  the channel specific configuration data to the channel device
* the dss driver gets loaded through udev and matches all the
  channels
* the dss driver creates the display devices below each channel,
  according to the configuration data it got passed.
* The various display drivers get loaded through udev as needed
  and match the display devices
* Each display device driver initializes the display and creates
  the high-level devices (fb and v4l) as needed.
* Your fb and v4l highlevel drivers get loaded through udev and
  bind to the devices, creating the user space device nodes
  through their subsystems.

Now this would be the most complex scenerio that hopefully is
not really needed, but I guess it illustrates the concept.
I would guess that you can actually reduce this significantly
if you do not actually need all the indirections.

Some parts could also get simpler if you change the layering,
e.g. by making the v4l and fb drivers library code and having
the display drivers call them, rather than have the display
drivers create the devices that get passed to upper drivers.

> > The frame buffer device also looks weird. Right now you only seem
> > to have a single frame buffer registered to a driver in the same
> > module. Is that frame buffer not dependent on a controller?
> 
> MCDE framebuffers are only depending on MCDE DSS. DSS is the API that
> will be used by all user space APIs so that we don't have to duplicate
> the common code. We are planning mcde_kms and mcde_v4l2 drivers on top
> of MCDE DSS(=Display Sub System).

My impression was that you don't need a frame buffer driver if you have
a kms driver, is this wrong?

What does the v4l2 driver do? In my simple world, displays are for output
and v4l is for input, so I must have missed something here.

	Arnd

[PATCH 09/10] MCDE: Add build files and bus

[Marcus LORENTZON]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: den 25 november 2010 17:48
> To: Marcus LORENTZON
> Cc: linux-arm-kernel at lists.infradead.org; Jimmy RUBIN; linux-
> media at vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
> Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
>
> On Thursday 25 November 2010, Marcus LORENTZON wrote:
> > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > > On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > > > This patch adds support for the MCDE, Memory-to-display
> controller,
> > > > found in the ST-Ericsson ux500 products.
> > > >
> > > > This patch adds the necessary build files for MCDE and the bus
> that
> > > > all displays are connected to.
> > > >
> > >
> > > Can you explain why you need a bus for this?
> >
> > The idea of the bus is to make it easier to add new panel/display
> support
> > using the normal device/driver model. Boards add devices at init, and
> > drivers are selected in config. If we were to model the "real
> physical"
> > bus structure it would be very complex, hard to use. We use our own
> bus,
> > but it's really a virtual bus for adding display devices and drivers
> to
> > MCDE host. Is there any "generic virtual bus type" we should use
> instead
> > of our own type? If you have another idea of how to add display
> devices
> > and drivers without MCDE code modifications, please let us know. A
> virtual
> > bus just give us a nice framework to do this.
>
> All devices that you cannot probe by asking hardware or firmware are
> normally
> considered platform devices. Then again, a platform device is usally
> identified by its resources, i.e. MMIO addresses and interrupts, which
> I guess your display does not have.

Then we might be on right track to model them as devices on a platform bus. Since most displays/panels can't be "plug-n-play" probed, instead devices has to be statically declared in board-xx.c files in mach-ux500 folder. Or is platform bus a "singleton"? Or can we define a new platform bus device?
Displays like HDMI TV-sets are not considered for device/driver in mcde. Instead there will be a hdmi-chip-device/driver on the mcde bus. So all devices and drivers on this bus are static.

> > > With the code you currently have, there is only a single driver
> > > associated
> > > with this bus type, and also just a single device that gets
> registered
> > > here!
> >
> > And yes, currently there's only one single driver. But there's a lot
> more
> > coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And
> once
> > all different u8500 boards are pushed, there will be multiple boards
> > registering devices with different setups. But one thing at a time.
>
> Your approach is making it hard to review the code in context. Adding a
> device driver that uses existing infrastructure is usually
> straightforward,
> but adding infrastructure is a hard thing and needs to be done with
> great
> care, especially if you add infrastructure before it actually is
> needed.
>
> Staging it in a way that adds all the display drivers later than the
> base driver is a good idea, but it would be helpful to also add the
> infrastructure at the later stage. Maybe you can try to simplify the
> code for now by hardcoding the single display and remove the dynamic
> registration. You still have the the code, so once the base code looks
> good for inclusion, we can talk about it in the context of adding
> dynamic display support back in, possibly in exactly the way you are
> proposing now, but perhaps in an entirely different way if we come up
> with a better solution.

What about starting with MCDE HW, which is the core HW driver doing all real work? And then continue with the infrastructure + some displays + drivers ...
Only problem is that we then have a driver that can't be used from user space, because I don't think I can find anyone with enough time to write a display driver + framebuffer on top of mcde_hw (which is what the existing code does).

> On a more abstract level, when you want to add large chunks of code
> to the kernel, you often cannot find anyone to review and understand
> all of it at once. Splitting a subsystem into ten patches by file
> level rarely helps, so instead you should ideally have a series of
> patches that each add working code that enable more features.
>
> This is of course more work for you, especially if you did not consider
> it while writing the code in the first place. Still, you should
> always try hard to make code easy to review as much as possible,
> because you need to work with reviewers both to get code in and to
> make sure you make the quality ends up as good as possible.
>
> > > >+static int __init mcde_subsystem_init(void)
> > > >+{
> > > >+       int ret;
> > > >+       pr_info("MCDE subsystem init begin\n");
> > > >+
> > > >+       /* MCDE module init sequence */
> > > >+       ret = mcde_init();
> > > >+       if (ret)
> > > >+               return ret;
> > > >+       ret = mcde_display_init();
> > > >+       if (ret)
> > > >+               goto mcde_display_failed;
> > > >+       ret = mcde_dss_init();
> > > >+       if (ret)
> > > >+               goto mcde_dss_failed;
> > > >+       ret = mcde_fb_init();
> > > >+       if (ret)
> > > >+               goto mcde_fb_failed;
> > > >+       pr_info("MCDE subsystem init done\n");
> > > >+
> > > >+       return 0;
> > > >+mcde_fb_failed:
> > > >+       mcde_dss_exit();
> > > >+mcde_dss_failed:
> > > >+       mcde_display_exit();
> > > >+mcde_display_failed:
> > > >+       mcde_exit();
> > > >+       return ret;
> > > >+}
> > >
> > > Splitting up the module into four sub-modules and then initializing
> > > everything from one place indicates that something is done wrong
> > > on a global scale.
> > >
> > > If you indeed need a bus, that should be a separate module that
> gets
> > > loaded first and then has the other modules build on top of.
> >
> > Yes, that's the general idea. We don't completely understand the
> correct
> > way of making sure how one module gets loaded before another, without
> > selecting init level, like the fs_initcall below you commented on. I
> > guess most of our submodules should be initialized during
> subsys_init.
> > But we have not found how to specify the module init order inside
> subsys
> > init level. Maybe you can shed some light on this? Makefile order
> seems
> > like a fragile way of defining init order dependencies.
> > Do you think static init calls from machine subsys init are a better
> solution?
>
> In general, the idea with loadable modules is that you can only load
> them in initialization order because of the symbol dependencies: The
> bus
> gets loaded first and it exports symbols used by the device drivers,
> so a device driver can be sure that the bus is initialized entirely.
>
> For the case where all modules are built-in, you can rely in link-order
> in the Makefile, e.g.
>
> obj-$(CONFIG_FOO_BASE)                += foo_base.o
> obj-$(CONFIG_FOO_SPECIFIC)    += foo_specific.o # this comes after
> foo_base

Ok, we will do this for the mcde stuff. How do we handle stuff that span different kernel folders? Like drivers/misc and drivers/video/mcde etc. We can't just change the order of top level makefiles, that would break other drivers I guess.

> > > I'm not sure how the other parts layer on top of one another, can
> you
> > > provide some more insight?
> >
> > +----------------------------+
> > | mcde_fb/mcde_kms/mcde_v4l2 |
> > +---------------+------------+
> > |    mcde_dss   |
> > +   +-----------+
> > |   | disp drvs |
> > +---+-----------+
> > |    mcde hw    |
> > +---------------+
> > |      HW       |
> > +---------------+
>
> Ok. One problem with this is that once you have a multitude of
> display drivers, you can no longer layer them below mcde_dss.

Not sure what you mean, we have plenty of drivers and devices already. Maybe I should try to clarify picture.

DSS give access to all display devices probed on the virtual mcde dss bus, or platform bus with specific type of devices if you like. All calls to DSS operate on a display device, like create an overlay(=framebuffer), request an update, set power mode, etc. All calls to DSS related to display itself and not only framebuffer scanout, will be passed on to the display driver of the display device in question. All calls DSS only related to overlays, like buffer pointers, position, rotation etc is handled directly by DSS calling mcde_hw.
You could see mcde_hw as a physical level driver and mcde_dss closer to a logical driver, delegating display specific decisions to the display driver. Another analogy is mcde_hw is host driver and display drivers are client device drivers. And DSS is a collection of logic to manage the interaction between host and client devices.

> Having the kms/fb/v4l2 drivers on top definitely makes sense, so
> these should all be able to be standalone loadable modules.
> I do not understand why you have a v4l2 driver at all, or why
> you need both fb and kms drivers, but that is probably because
> of my ignorance of display device drivers.

All APIs have to be provided, these are user space API requirements. KMS has a generic FB implementation. But most of KMS is modeled by desktop/PC graphics cards. And while we might squeeze MCDE in to look like a PC card, it might also just make things more complex and restrict us to do things not possible in PC architecture. Alex Deucher noted in a previous post that we also have the option of implementing the KMS ioctls. We are looking at both options. And having our own framebuffer driver might make sense since it is a very basic driver, and it will allow us to easily extend support for things like partial updates for display panels with on board memory. These panels with memory (like DSI command mode displays) is one of the reasons why KMS is not the perfect match. Since we want to expose features available for these types of displays.

> > > From what I understood so far, you have a single multi-channel
> display
> > > controller (mcde_hw.c) that drives the hardware.
> > > Each controller can have multiple frame buffers attached to it,
> which
> > > in turn can have multiple displays attached to each of them, but
> your
> > > current configuration only has one of each, right?
> >
> > Correct, channels A/B (crtcs) can have two blended "framebuffers"
> plus
> > background color, channels C0/C1 can have one framebuffer.
>
> We might still be talking about different things here, not sure.

In short,
KMS connector = MCDE port
KMS encoder = MCDE channel
KMS crtc = MCDE overlay

> > > Right now you have a single top-level bus device for the displays,
> > > maybe that can be integrated into the controller so the displays
> are
> > > properly rooted below the hardware that drives them.
> >
> > Not sure I understand 100%. Routing is independent of bus structure.
> > Routing could change dynamically during runtime reconfiguration using
> > future KMS for example. Bus is only for connecting display devices
> > with drivers. Of course we could have one bus per port/connector.
> > But then the code would be more complex and we would end up with one
> > bus/device/driver per connector (or in some rare cases 2-4 on DBI/DSI
> > connectors).
>
> I mean 'root' as in the root of your device tree, where the tree
> is modeled after the logical layout of your hardware.
>
> Looking at the representation in sysfs, you should probably aim
> for something like
>
> /sys/devices/axi/axi0/mcde_controller
>                               /chnlA
>                                       /dspl_crtc0
>                                               /fb0
>                                               /fb1
>                                               /v4l_0
>                                       /dspl_dbi0
>                                               /fb2
>                                               /v4l_1
>                               /chnlB
>                                       /dspl_ctrc1
>                                               /fb3
>                               /chnlC
>                                       /dspl_lcd0
>                                               /fb4
>                                               /v4l_2
>
> Not sure if that is close to what your hardware would really
> look like. My point is that all the objects that you are
> dealing with as a device driver should be represented hierarchically
> according to how you probe them.

Yes, mcde_bus should be connected to mcde, this is a bug. The display drivers will placed in this bus, since this is where they are probed like platform devices, by name (unless driver can do MIPI standard probing or something). Framebuffers/V4L2 overlay devices can't be put in same hierarchy, since they have multiple "parents" in case the same framebuffer is cloned to multiple displays for example. But I think I understand your more general point of sysfs representing the "real" probe hierarchy. And this is something we will look at.

> Assuming the structure above is correct and you cannot probe
> any of this by looking at registers, you would put a description
> of it into the a data structure (ideally a flattened device tree
> or a section of one) and let the probing happen:
>
> * The axi core registers an mcde controller as device axi0.
> * udev matches the device and loads the mcde hw driver from
>   user space

We are trying to avoid dynamic driver loading and udev for platform devices to be able to show application graphics within a few seconds after boot.

> * the hw driver creates a device for each channel, and passes
>   the channel specific configuration data to the channel device

We have to migrate displays in runtime between different channels (different use cases and different channel features), we don't want to model displays as probed beneath the channel. Maybe the port/connector could be a device. But that code is so small, so it might just add complexity to visualize sysfs hierarchy. What do you think?

> * the dss driver gets loaded through udev and matches all the
>   channels
> * the dss driver creates the display devices below each channel,
>   according to the configuration data it got passed.

"All" display devices need static platform_data from mach-ux500/board-xx.c. This is why we have the bus, to bind display dev and driver.

> * The various display drivers get loaded through udev as needed
>   and match the display devices
> * Each display device driver initializes the display and creates
>   the high-level devices (fb and v4l) as needed.

This is setup by board/product specific code. Display drivers just enable use of the HW, not defining how the displays are used from user space.

> * Your fb and v4l highlevel drivers get loaded through udev and
>   bind to the devices, creating the user space device nodes
>   through their subsystems.
>
> Now this would be the most complex scenerio that hopefully is
> not really needed, but I guess it illustrates the concept.
> I would guess that you can actually reduce this significantly
> if you do not actually need all the indirections.
>
> Some parts could also get simpler if you change the layering,
> e.g. by making the v4l and fb drivers library code and having
> the display drivers call them, rather than have the display
> drivers create the devices that get passed to upper drivers.

Devices are static from mach-ux500/board-xx. And v4l2/fb setup is board/product specific and could change dynamically.

> > > The frame buffer device also looks weird. Right now you only seem
> > > to have a single frame buffer registered to a driver in the same
> > > module. Is that frame buffer not dependent on a controller?
> >
> > MCDE framebuffers are only depending on MCDE DSS. DSS is the API that
> > will be used by all user space APIs so that we don't have to
> duplicate
> > the common code. We are planning mcde_kms and mcde_v4l2 drivers on
> top
> > of MCDE DSS(=Display Sub System).
>
> My impression was that you don't need a frame buffer driver if you have
> a kms driver, is this wrong?

No, see above. Just that we have mcde dss to support multiple user space apis by customer request. Then doing our own fb on top of that is very simple and adds flexibility.

> What does the v4l2 driver do? In my simple world, displays are for
> output
> and v4l is for input, so I must have missed something here.

Currently nothing, since it is not finished. But the idea (and requirement) is that normal graphics will use framebuffer and video/camera overlays will use v4l2 overlays. Both using same mcde channel and display. Some users might also configure their board to use two framebuffers instead. Or maybe only use KMS etc ...

Thanks for all your experience sharing and feedback!

/BR
/Marcus

[PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

[dri people: please have a look at the KMS discussion way below]

On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote:
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > Sent: den 25 november 2010 17:48
> > To: Marcus LORENTZON
> > Cc: linux-arm-kernel at lists.infradead.org; Jimmy RUBIN; linux-
> > media at vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
> > Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
> >
> > On Thursday 25 November 2010, Marcus LORENTZON wrote:
> > > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > > > On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > > > > This patch adds support for the MCDE, Memory-to-display
> > controller,
> > > > > found in the ST-Ericsson ux500 products.
> > > > >

[note: please configure your email client properly so it keeps
proper attribution of text and and does not rewrap the citations
incorrectly. Wrap your own text after 70 characters]

> >
> > All devices that you cannot probe by asking hardware or firmware are
> > normally
> > considered platform devices. Then again, a platform device is usally
> > identified by its resources, i.e. MMIO addresses and interrupts, which
> > I guess your display does not have.
> 
> Then we might be on right track to model them as devices on a
> platform bus. Since most displays/panels can't be "plug-n-play"
> probed, instead devices has to be statically declared in
> board-xx.c files in mach-ux500 folder. Or is platform bus a
> "singleton"? Or can we define a new platform bus device?
> Displays like HDMI TV-sets are not considered for device/driver
> in mcde. Instead there will be a hdmi-chip-device/driver on the
> mcde bus. So all devices and drivers on this bus are static.

I think I need to clarify to things:

* When I talk about a bus, I mean 'struct bus_type', which identifies
  all devices with a uniform bus interface to their parent device
  (think: PCI, USB, I2C). You seem to think of a bus as a specific
  instance of that bus type, i.e. the device that is the parent of
  all the connected devices. If you have only one instance of a bus
  in any system, and they are all using the same driver, do not add
  a bus_type for it.
  A good reason to add a bus_type would be e.g. if the "display"
  driver uses interfaces to the dss that are common among multiple
  dss drivers from different vendors, but the actual display drivers
  are identical. This does not seem to be the case.

* When you say that the devices are static, I hope you do not mean
  static in the C language sense. We used to allow devices to be
  declared as "static struct" and registered using
  platform_device_register (or other bus specific functions). This
  is no longer valid and we are removing the existing users, do not
  add new ones. When creating a platform device, use
  platform_device_register_simple or platform_device_register_resndata.

I'm not sure what you mean with drivers being static. Predefining
the association between displays and drivers in per-machine files is
fine, but since this is really board specific, it would be better
to eventually do this through data passed from the boot loader, so
you don't have to have a machine file for every combination of displays
that is in the field.

> > Staging it in a way that adds all the display drivers later than the
> > base driver is a good idea, but it would be helpful to also add the
> > infrastructure at the later stage. Maybe you can try to simplify the
> > code for now by hardcoding the single display and remove the dynamic
> > registration. You still have the the code, so once the base code looks
> > good for inclusion, we can talk about it in the context of adding
> > dynamic display support back in, possibly in exactly the way you are
> > proposing now, but perhaps in an entirely different way if we come up
> > with a better solution.
> 
> What about starting with MCDE HW, which is the core HW driver doing
> all real work? And then continue with the infrastructure + some displays
> + drivers ...

This is already the order in which you submitted them, I don't see a
difference here. I was not asking to delay any of the code, just to put
them in a logical order.

> Only problem is that we then have a driver that can't be used from user
> space, because I don't think I can find anyone with enough time to write
> a display driver + framebuffer on top of mcde_hw (which is what the
> existing code does).

Well, developer time does not appear to be one of your problems, you
already wasted tons of it by developing a huge chunk of code that isn't
going anywhere because you wrote it without consulting the upstream
community ;-)

There is no need to develop anything from scratch here, you already have
the code you want to end up with. What I would do here is to start with
a single git commit that adds the full driver. Then take out bits you
don't absolutely need to keep the driver from showing text on your
screen (not necessarily in this order):

* Take out display drivers one by one, until there is only one left.
  Do a git commit after each driver
* Take out the register definitions that are not actually used in your
  code
* Remove the infrastructure for dynamic displays and hardcode the one
  you use
* Take out the frame buffer code
* Take out the infrastructure for multiple user-interfaces, hardcoding KMS
  to the DSS
* Anything else you don't absolutely need.

Finally, you should end up with a very lean driver that only does a
single thing and only works on one very specific board. Remove that, too,
in a final commit. Now use git to reverse the patch order and you have
a nice series that you can use for patch submission, one feature at a
time. Then we can discuss the individual merits of each patch.

In the future, best plan for how you want to submit the code while
you're writing it, instead of as an afterthought. Quite often, the
first patch to submit is also one of the early stages of the driver,
so there is no need to wait for the big picture before you start
submitting. This way, we can work out conceptual mistakes early on,
saving a lot of your time, and the reviewer's time as well.

> > For the case where all modules are built-in, you can rely in link-order
> > in the Makefile, e.g.
> >
> > obj-$(CONFIG_FOO_BASE)                += foo_base.o
> > obj-$(CONFIG_FOO_SPECIFIC)    += foo_specific.o # this comes after
> > foo_base
> 
> Ok, we will do this for the mcde stuff. How do we handle stuff that span 
> different kernel folders? Like drivers/misc and drivers/video/mcde etc.
> We can't just change the order of top level makefiles, that would break 
> other drivers I guess.

Right, you have to find a different solution for those. Most importantly,
a module in one directory should not have intimate knowledge of data
structures in a different module in another directory.

In your example, drivers/misc is probably wrong anyway. Try ignoring this
problem at first by forcing all the drivers loadable modules, which will
naturally fix the initialization order. When you still have link order
problems by building all the drivers into the kernel after this, we can
have another look to find the least ugly solution.

> > > > I'm not sure how the other parts layer on top of one another, can
> > you
> > > > provide some more insight?
> > >
> > > +----------------------------+
> > > | mcde_fb/mcde_kms/mcde_v4l2 |
> > > +---------------+------------+
> > > |    mcde_dss   |
> > > +   +-----------+
> > > |   | disp drvs |
> > > +---+-----------+
> > > |    mcde hw    |
> > > +---------------+
> > > |      HW       |
> > > +---------------+
> >
> > Ok. One problem with this is that once you have a multitude of
> > display drivers, you can no longer layer them below mcde_dss.
> 
> Not sure what you mean, we have plenty of drivers and devices already.
> Maybe I should try to clarify picture.

I mean the layering of loadable modules: you cannot make a high-level
module link against multiple low-level modules that export the
same interface. If you have multiple modules that implement the same
interface like you diplay drivers, they need to be on top!

> DSS give access to all display devices probed on the virtual mcde
> dss bus, or platform bus with specific type of devices if you like.
> All calls to DSS operate on a display device, like create an
> overlay(=framebuffer), request an update, set power mode, etc.
> All calls to DSS related to display itself and not only framebuffer
> scanout, will be passed on to the display driver of the display
> device in question. All calls DSS only related to overlays, like
> buffer pointers, position, rotation etc is handled directly by DSS 
> calling mcde_hw.
>
> You could see mcde_hw as a physical level driver and mcde_dss closer
> to a logical driver, delegating display specific decisions to the
> display driver. Another analogy is mcde_hw is host driver and display
> drivers are client device drivers. And DSS is a collection of logic
> to manage the interaction between host and client devices.

The way you describe it, I would picture it differently:

+----------+ +----+-----+-----+ +-------+
| mcde_hw  | | fb | kms | v4l | | displ |
+----+----------------------------------+
| HW |            mcde_dss              |
+----+----------------------------------+

In this model, the dss is the core module that everything else
links to. The hw driver talks to the actual hardware and to the
dss. The three front-ends only talk to the dss, but not to the
individual display drivers or to the hw code directly (i.e. they
don't use their exported symbols or internal data structures.
The display drivers only talk to the dss, but not to the front-ends
or the hw drivers.

Would this be a correct representation of your modules?

> > Having the kms/fb/v4l2 drivers on top definitely makes sense, so
> > these should all be able to be standalone loadable modules.
> > I do not understand why you have a v4l2 driver at all, or why
> > you need both fb and kms drivers, but that is probably because
> > of my ignorance of display device drivers.
> 
> All APIs have to be provided, these are user space API requirements.
> KMS has a generic FB implementation. But most of KMS is modeled by
> desktop/PC graphics cards. And while we might squeeze MCDE in to look
> like a PC card, it might also just make things more complex and
> restrict us to do things not possible in PC architecture.

Ok, so you have identified a flaw with the existing KMS code. You should
most certainly not try to make your driver fit into the flawed model by
making it look like a PC. Instead, you are encouraged to fix the problems
with KMS to make sure it can also meet your requirements. The reason
why it doesn't do that today is that all the existing users are PC
hardware and we don't build infrastructure that we expect to be used
in the future but don't need yet. It would be incorrect anyway.

Can you describe the shortcomings of the KSM code? I've added the dri-devel
list to Cc, to get the attention of the right people.

> Alex Deucher noted in a previous post that we also have the option of
> implementing the KMS ioctls. We are looking at both options. And having
> our own framebuffer driver might make sense since it is a very basic
> driver, and it will allow us to easily extend support for things like
> partial updates for display panels with on board memory. These panels
> with memory (like DSI command mode displays) is one of the reasons why
> KMS is not the perfect match. Since we want to expose features available
> for these types of displays.

Ok.

> > > > From what I understood so far, you have a single multi-channel
> > display
> > > > controller (mcde_hw.c) that drives the hardware.
> > > > Each controller can have multiple frame buffers attached to it,
> > which
> > > > in turn can have multiple displays attached to each of them, but
> > your
> > > > current configuration only has one of each, right?
> > >
> > > Correct, channels A/B (crtcs) can have two blended "framebuffers"
> > plus
> > > background color, channels C0/C1 can have one framebuffer.
> >
> > We might still be talking about different things here, not sure.
> 
> In short,
> KMS connector = MCDE port
> KMS encoder = MCDE channel
> KMS crtc = MCDE overlay

Any chance you could change the identifiers in the code for this
without confusing other people?

> > Looking at the representation in sysfs, you should probably aim
> > for something like
> >
> > /sys/devices/axi/axi0/mcde_controller
> >                               /chnlA
> >                                       /dspl_crtc0
> >                                               /fb0
> >                                               /fb1
> >                                               /v4l_0
> >                                       /dspl_dbi0
> >                                               /fb2
> >                                               /v4l_1
> >                               /chnlB
> >                                       /dspl_ctrc1
> >                                               /fb3
> >                               /chnlC
> >                                       /dspl_lcd0
> >                                               /fb4
> >                                               /v4l_2
> >
> > Not sure if that is close to what your hardware would really
> > look like. My point is that all the objects that you are
> > dealing with as a device driver should be represented hierarchically
> > according to how you probe them.
> 
> Yes, mcde_bus should be connected to mcde, this is a bug. The display
> drivers will placed in this bus, since this is where they are probed
> like platform devices, by name (unless driver can do MIPI standard
> probing or something). Framebuffers/V4L2 overlay devices can't be
> put in same hierarchy, since they have multiple "parents" in case
> the same framebuffer is cloned to multiple displays for example.
> But I think I understand your more general point of sysfs representing
> the "real" probe hierarchy. And this is something we will look at.

Ok. If your frame buffers are not children of the displays, they should
however be children of the controller:

.../mcde_controller/
	/chnlA/
		/displ_crtc0
		/displ_dbi0
	/chnlB/
		dspl_crtc1
	/fb0
	/fb1
	/fb2
	/v4l_0
	/v4l_1

Does this fit better?

> > Assuming the structure above is correct and you cannot probe
> > any of this by looking at registers, you would put a description
> > of it into the a data structure (ideally a flattened device tree
> > or a section of one) and let the probing happen:
> >
> > * The axi core registers an mcde controller as device axi0.
> > * udev matches the device and loads the mcde hw driver from
> >   user space
> 
> We are trying to avoid dynamic driver loading and udev for platform
> devices to be able to show application graphics within a few seconds
> after boot.

That is fine, you don't need to do that for products. However, it
is valuable to be able to do it and to think of it in this way.
When you are able to have everything modular, it is much easier to
spot layering violations and you can much easier define the object
life time rules.

Also, for the general case of building a cross-platform kernel,
you want to be able to use modules for everything. Remember that
we are targetting a single kernel binary that can run on multiple
SoC families, potentially with hundreds of different boards.

> > * the hw driver creates a device for each channel, and passes
> >   the channel specific configuration data to the channel device
> 
> We have to migrate displays in runtime between different channels
> (different use cases and different channel features), we don't want
> to model displays as probed beneath the channel. Maybe the
> port/connector could be a device. But that code is so small, so it
> might just add complexity to visualize sysfs hierarchy.
> What do you think?

This makes it pretty obvious that the channel should not be a
device, but rather something internal to the dss or hw module.

What is the relation between a port/connector and a display?
If it's 1:1, it should be the same device.

> > * the dss driver gets loaded through udev and matches all the
> >   channels
> > * the dss driver creates the display devices below each channel,
> >   according to the configuration data it got passed.
> 
> "All" display devices need static platform_data from
> mach-ux500/board-xx.c. This is why we have the bus,
> to bind display dev and driver.

You don't need to instantiate the device from the board though,
just provide the data. When you add the display specific data
to the dss data, the dss can create the display devices:

static struct mcde_display_data mcde_displays[2] = {
{
	...
}, {
	...
},
};

static struct mcde_dss_data {
	int num_displays;
	struct mcde_display_data *displays;
} my_dss = {
	.num_displays = 2,
	.displays = &mcde_displays;
};

The mcde_dss probe function then takes the dss_data and iterates
the displays, creating a new child device for each.

> > * The various display drivers get loaded through udev as needed
> >   and match the display devices
> > * Each display device driver initializes the display and creates
> >   the high-level devices (fb and v4l) as needed.
> 
> This is setup by board/product specific code. Display drivers
> just enable use of the HW, not defining how the displays are
> used from user space.

Right, this also gets obsolete, since as you said an fb cannot be
the child of a display.
 
> > * Your fb and v4l highlevel drivers get loaded through udev and
> >   bind to the devices, creating the user space device nodes
> >   through their subsystems.
> >
> > Now this would be the most complex scenerio that hopefully is
> > not really needed, but I guess it illustrates the concept.
> > I would guess that you can actually reduce this significantly
> > if you do not actually need all the indirections.
> >
> > Some parts could also get simpler if you change the layering,
> > e.g. by making the v4l and fb drivers library code and having
> > the display drivers call them, rather than have the display
> > drivers create the devices that get passed to upper drivers.
> 
> Devices are static from mach-ux500/board-xx. And v4l2/fb setup
> is board/product specific and could change dynamically.

Not sure how the fb setup can be both board specific and dynamic.
If it's statically defined per board, it should be part of the
dss data, and dss can then create the fb devices. If it's completely
dynamic, it gets created through user space interaction anyway.
 
> > > > The frame buffer device also looks weird. Right now you only seem
> > > > to have a single frame buffer registered to a driver in the same
> > > > module. Is that frame buffer not dependent on a controller?
> > >
> > > MCDE framebuffers are only depending on MCDE DSS. DSS is the API that
> > > will be used by all user space APIs so that we don't have to
> > duplicate
> > > the common code. We are planning mcde_kms and mcde_v4l2 drivers on
> > top
> > > of MCDE DSS(=Display Sub System).
> >
> > My impression was that you don't need a frame buffer driver if you have
> > a kms driver, is this wrong?
> 
> No, see above. Just that we have mcde dss to support multiple user
> space apis by customer request. Then doing our own fb on top of
> that is very simple and adds flexibility.

This sounds like an odd thing for a customer to ask for ;-)

In my experience customers want to solve specific problems like
everyone else, they have little interest in adding complexity
for the sake of it. Is there something wrong with one of the
interfaces? If so, it would be better to fix that than to add
an indirection to allow more of them!

> > What does the v4l2 driver do? In my simple world, displays are for
> > output
> > and v4l is for input, so I must have missed something here.
> 
> Currently nothing, since it is not finished. But the idea (and
> requirement) is that normal graphics will use framebuffer and
> video/camera overlays will use v4l2 overlays. Both using same
> mcde channel and display. Some users might also configure their
> board to use two framebuffers instead. Or maybe only use KMS etc ...

I still don't understand, sorry for being slow. Why does a camera
use a display?

	Arnd

[PATCH 09/10] MCDE: Add build files and bus

[Linus Walleij]

2010/11/26 Arnd Bergmann <arnd@arndb.de>:

> * When you say that the devices are static, I hope you do not mean
> ?static in the C language sense. We used to allow devices to be
> ?declared as "static struct" and registered using
> ?platform_device_register (or other bus specific functions). This
> ?is no longer valid and we are removing the existing users, do not
> ?add new ones. When creating a platform device, use
> ?platform_device_register_simple or platform_device_register_resndata.

Is this part of the generic ARM runtime multi-platform kernel
and device trees shebang?

The Ux500 still isn't in that sector, it needs extensive rewriting
of arch/arm/mach-ux500 to be done first, so as to support e.g.
U8500 and U5500 with a single kernel image.

Trying to skin that cat that as part of this review is a bit too
much to ask IMO, I'd rather have the author of this driver
adapt to whatever platform data registration mechanism is
in place for the merge window. Else it needs fixing as part
of a bigger endavour to root out compile-time platform
configuration.

Yours
Linus Walleij

[PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

On Tuesday 30 November 2010, Linus Walleij wrote:
> 2010/11/26 Arnd Bergmann <arnd@arndb.de>:
> 
> > * When you say that the devices are static, I hope you do not mean
> >  static in the C language sense. We used to allow devices to be
> >  declared as "static struct" and registered using
> >  platform_device_register (or other bus specific functions). This
> >  is no longer valid and we are removing the existing users, do not
> >  add new ones. When creating a platform device, use
> >  platform_device_register_simple or platform_device_register_resndata.
> 
> Is this part of the generic ARM runtime multi-platform kernel
> and device trees shebang?
> 
> The Ux500 still isn't in that sector, it needs extensive rewriting
> of arch/arm/mach-ux500 to be done first, so as to support e.g.
> U8500 and U5500 with a single kernel image.
> 
> Trying to skin that cat that as part of this review is a bit too
> much to ask IMO, I'd rather have the author of this driver
> adapt to whatever platform data registration mechanism is
> in place for the merge window. Else it needs fixing as part
> of a bigger endavour to root out compile-time platform
> configuration.

The 'no static devices' rule is something that Greg brought up
at the embedded developer session during PlumbersConf this year,
I wasn't aware of the problem before that either.

It is not related to the multi-platform kernel work and it's
not ARM specific.

Maybe Greg can give a short explanation of the impact of this.
AFAIR, static device definitions still work, but there are
plans to remove that capability in the future.

	Arnd

[PATCH 09/10] MCDE: Add build files and bus

[Greg KH]

On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote:
> On Tuesday 30 November 2010, Linus Walleij wrote:
> > 2010/11/26 Arnd Bergmann <arnd@arndb.de>:
> > 
> > > * When you say that the devices are static, I hope you do not mean
> > >  static in the C language sense. We used to allow devices to be
> > >  declared as "static struct" and registered using
> > >  platform_device_register (or other bus specific functions). This
> > >  is no longer valid and we are removing the existing users, do not
> > >  add new ones. When creating a platform device, use
> > >  platform_device_register_simple or platform_device_register_resndata.
> > 
> > Is this part of the generic ARM runtime multi-platform kernel
> > and device trees shebang?
> > 
> > The Ux500 still isn't in that sector, it needs extensive rewriting
> > of arch/arm/mach-ux500 to be done first, so as to support e.g.
> > U8500 and U5500 with a single kernel image.
> > 
> > Trying to skin that cat that as part of this review is a bit too
> > much to ask IMO, I'd rather have the author of this driver
> > adapt to whatever platform data registration mechanism is
> > in place for the merge window. Else it needs fixing as part
> > of a bigger endavour to root out compile-time platform
> > configuration.
> 
> The 'no static devices' rule is something that Greg brought up
> at the embedded developer session during PlumbersConf this year,
> I wasn't aware of the problem before that either.
> 
> It is not related to the multi-platform kernel work and it's
> not ARM specific.
> 
> Maybe Greg can give a short explanation of the impact of this.
> AFAIR, static device definitions still work, but there are
> plans to remove that capability in the future.

That is exactly correct.

A struct device is a dynamically referenced thing, and as such, should
be dynamically created and it will be automatically destroyed when it
needs to when everyone is finished with it.  By making a struct device
static, that kind of defeats the whole purpose of reference counting the
thing, not to mention making freeing the object when finished a bit
difficult :)

thanks,

greg k-h

[PATCH 09/10] MCDE: Add build files and bus

[Russell King - ARM Linux]

On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote:
> On Tuesday 30 November 2010, Linus Walleij wrote:
> > 2010/11/26 Arnd Bergmann <arnd@arndb.de>:
> > 
> > > * When you say that the devices are static, I hope you do not mean
> > >  static in the C language sense. We used to allow devices to be
> > >  declared as "static struct" and registered using
> > >  platform_device_register (or other bus specific functions). This
> > >  is no longer valid and we are removing the existing users, do not
> > >  add new ones. When creating a platform device, use
> > >  platform_device_register_simple or platform_device_register_resndata.
> > 
> > Is this part of the generic ARM runtime multi-platform kernel
> > and device trees shebang?
> > 
> > The Ux500 still isn't in that sector, it needs extensive rewriting
> > of arch/arm/mach-ux500 to be done first, so as to support e.g.
> > U8500 and U5500 with a single kernel image.
> > 
> > Trying to skin that cat that as part of this review is a bit too
> > much to ask IMO, I'd rather have the author of this driver
> > adapt to whatever platform data registration mechanism is
> > in place for the merge window. Else it needs fixing as part
> > of a bigger endavour to root out compile-time platform
> > configuration.
> 
> The 'no static devices' rule is something that Greg brought up
> at the embedded developer session during PlumbersConf this year,
> I wasn't aware of the problem before that either.
> 
> It is not related to the multi-platform kernel work and it's
> not ARM specific.
> 
> Maybe Greg can give a short explanation of the impact of this.
> AFAIR, static device definitions still work, but there are
> plans to remove that capability in the future.

There's lots of static devices, not only platform devices, in the ARM
tree.  It's going to be a hell of a lot of work to fix this all up
properly.

I hope that the capability for static devices won't disappear until
the huge pile of work on ARM has been completed.

[PATCH 09/10] MCDE: Add build files and bus

[Greg KH]

On Tue, Nov 30, 2010 at 06:40:49PM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote:
> > On Tuesday 30 November 2010, Linus Walleij wrote:
> > > 2010/11/26 Arnd Bergmann <arnd@arndb.de>:
> > > 
> > > > * When you say that the devices are static, I hope you do not mean
> > > >  static in the C language sense. We used to allow devices to be
> > > >  declared as "static struct" and registered using
> > > >  platform_device_register (or other bus specific functions). This
> > > >  is no longer valid and we are removing the existing users, do not
> > > >  add new ones. When creating a platform device, use
> > > >  platform_device_register_simple or platform_device_register_resndata.
> > > 
> > > Is this part of the generic ARM runtime multi-platform kernel
> > > and device trees shebang?
> > > 
> > > The Ux500 still isn't in that sector, it needs extensive rewriting
> > > of arch/arm/mach-ux500 to be done first, so as to support e.g.
> > > U8500 and U5500 with a single kernel image.
> > > 
> > > Trying to skin that cat that as part of this review is a bit too
> > > much to ask IMO, I'd rather have the author of this driver
> > > adapt to whatever platform data registration mechanism is
> > > in place for the merge window. Else it needs fixing as part
> > > of a bigger endavour to root out compile-time platform
> > > configuration.
> > 
> > The 'no static devices' rule is something that Greg brought up
> > at the embedded developer session during PlumbersConf this year,
> > I wasn't aware of the problem before that either.
> > 
> > It is not related to the multi-platform kernel work and it's
> > not ARM specific.
> > 
> > Maybe Greg can give a short explanation of the impact of this.
> > AFAIR, static device definitions still work, but there are
> > plans to remove that capability in the future.
> 
> There's lots of static devices, not only platform devices, in the ARM
> tree.  It's going to be a hell of a lot of work to fix this all up
> properly.

I agree, it's been abused for many years this way :(

> I hope that the capability for static devices won't disappear until
> the huge pile of work on ARM has been completed.

Don't worry, it will not.

thanks,

greg k-h

[PATCH 09/10] MCDE: Add build files and bus

[Russell King - ARM Linux]

On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote:
> On Tue, Nov 30, 2010 at 06:40:49PM +0000, Russell King - ARM Linux wrote:
> > There's lots of static devices, not only platform devices, in the ARM
> > tree.  It's going to be a hell of a lot of work to fix this all up
> > properly.
> 
> I agree, it's been abused for many years this way :(

I don't agree that it is abuse - it was something explicitly allowed by
the original device model design by Patrick, with the condition that
such a device was never unregistered.  That's exactly the way we treat
these devices.

What I'm slightly concerned about is that this is going to needlessly
bloat the kernel - we're going to have to find some other way to store
this information, and create devices from that - which means additional
code to do the creation, and data structures for it to create these from.
There will be additional wastage from kmalloc as kmalloc doesn't allocate
just the size you ask for, but normally a power of two which will contain
the size.

That could potentially mean that as the device structure is 216 bytes,
kmalloc will use the 256 byte allocation size, which means a wastage of
40 bytes per device structure.  On top of that goes the size of
resources with the allocation slop on top for that, and then there's
another allocation for the platform data.

Has anyone considered these implications before making this choice?

[PATCH 09/10] MCDE: Add build files and bus

[Greg KH]

On Tue, Nov 30, 2010 at 10:05:50PM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote:
> > On Tue, Nov 30, 2010 at 06:40:49PM +0000, Russell King - ARM Linux wrote:
> > > There's lots of static devices, not only platform devices, in the ARM
> > > tree.  It's going to be a hell of a lot of work to fix this all up
> > > properly.
> > 
> > I agree, it's been abused for many years this way :(
> 
> I don't agree that it is abuse - it was something explicitly allowed by
> the original device model design by Patrick, with the condition that
> such a device was never unregistered.  That's exactly the way we treat
> these devices.

I understand Pat allowed this, I just don't agree that it's the correct
thing to do :)

-mm had a patch for a long time that would throw up warnings if you ever
did this for x86 so that arch should be clean of this issue by now.

> What I'm slightly concerned about is that this is going to needlessly
> bloat the kernel - we're going to have to find some other way to store
> this information, and create devices from that - which means additional
> code to do the creation, and data structures for it to create these from.
> There will be additional wastage from kmalloc as kmalloc doesn't allocate
> just the size you ask for, but normally a power of two which will contain
> the size.
> 
> That could potentially mean that as the device structure is 216 bytes,
> kmalloc will use the 256 byte allocation size, which means a wastage of
> 40 bytes per device structure.  On top of that goes the size of
> resources with the allocation slop on top for that, and then there's
> another allocation for the platform data.
> 
> Has anyone considered these implications before making this choice?

Yes, I have, which is one reason I haven't done this type of change yet.
I need to figure out a way to not drasticly increase the size and still
make it easy and simple for the platform and driver write their code.

It's a work in progress, but wherever possible, I encourage people to
not make 'struct device' static.

thanks,

greg k-h

[PATCH 09/10] MCDE: Add build files and bus

[Russell King - ARM Linux]

On Tue, Nov 30, 2010 at 03:05:33PM -0800, Greg KH wrote:
> On Tue, Nov 30, 2010 at 10:05:50PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote:
> > > On Tue, Nov 30, 2010 at 06:40:49PM +0000, Russell King - ARM Linux wrote:
> > > > There's lots of static devices, not only platform devices, in the ARM
> > > > tree.  It's going to be a hell of a lot of work to fix this all up
> > > > properly.
> > > 
> > > I agree, it's been abused for many years this way :(
> > 
> > I don't agree that it is abuse - it was something explicitly allowed by
> > the original device model design by Patrick, with the condition that
> > such a device was never unregistered.  That's exactly the way we treat
> > these devices.
> 
> I understand Pat allowed this, I just don't agree that it's the correct
> thing to do :)
> 
> -mm had a patch for a long time that would throw up warnings if you ever
> did this for x86 so that arch should be clean of this issue by now.
> 
> > What I'm slightly concerned about is that this is going to needlessly
> > bloat the kernel - we're going to have to find some other way to store
> > this information, and create devices from that - which means additional
> > code to do the creation, and data structures for it to create these from.
> > There will be additional wastage from kmalloc as kmalloc doesn't allocate
> > just the size you ask for, but normally a power of two which will contain
> > the size.
> > 
> > That could potentially mean that as the device structure is 216 bytes,
> > kmalloc will use the 256 byte allocation size, which means a wastage of
> > 40 bytes per device structure.  On top of that goes the size of
> > resources with the allocation slop on top for that, and then there's
> > another allocation for the platform data.
> > 
> > Has anyone considered these implications before making this choice?
> 
> Yes, I have, which is one reason I haven't done this type of change yet.
> I need to figure out a way to not drasticly increase the size and still
> make it easy and simple for the platform and driver write their code.
> 
> It's a work in progress, but wherever possible, I encourage people to
> not make 'struct device' static.

Right, so saying to ARM developers that they can't submit code which
adds new static device structures is rather problematical then, and
effectively brings a section of kernel development to a complete
standstill - it means no support for additional ARM platforms until
this issue is resolved.  (This "condition" was mentioned by Arnd
earlier in this thread, and was put in such a way that it was now
a hard and fast rule.)

I feel it would be better to allow the current situation to continue.
If we start telling people that they can't use statically declared
devices without first having an alternative, we'll end up with people
inventing their own individual - and different - solutions to this
problem, which could actually make the problem harder to resolve in
the longer term.

[PATCH 09/10] MCDE: Add build files and bus

[Greg KH]

On Tue, Nov 30, 2010 at 11:42:15PM +0000, Russell King - ARM Linux wrote:
> > It's a work in progress, but wherever possible, I encourage people to
> > not make 'struct device' static.
> 
> Right, so saying to ARM developers that they can't submit code which
> adds new static device structures is rather problematical then, and
> effectively brings a section of kernel development to a complete
> standstill - it means no support for additional ARM platforms until
> this issue is resolved.  (This "condition" was mentioned by Arnd
> earlier in this thread, and was put in such a way that it was now
> a hard and fast rule.)

Sorry, I didn't mean for that to be mentioned that way at all, as I know
the issues that are keeping this from happening.

> I feel it would be better to allow the current situation to continue.
> If we start telling people that they can't use statically declared
> devices without first having an alternative, we'll end up with people
> inventing their own individual - and different - solutions to this
> problem, which could actually make the problem harder to resolve in
> the longer term.

Ok, but again, I do encourage, wherever possible, that people do not
statically create a 'struct device'.

thanks,

greg k-h

[PATCH 09/10] MCDE: Add build files and bus

[Peter Stuge]

Russell King - ARM Linux wrote:
> I feel it would be better to allow the current situation to continue.

I think this misses the point, and is somewhat redundant; I think
everyone knows that it is easiest to never change anything. But
then nothing improves.


> If we start telling people that they can't use statically declared
> devices without first having an alternative,

I would consider it early warning that the way things have been done
before will go away. And I would thus write drivers in a way that
demonstrates and includes that understanding.

The same problem exists in hardware products needing any kind of
longish lifetime. Deal with evolving components by having clean
and simple interfaces, and by not relying on a particular interface
very deep on either side of the edge. Simple I think.


//Peter

[PATCH 09/10] MCDE: Add build files and bus

[Russell King - ARM Linux]

On Wed, Dec 01, 2010 at 01:53:39PM +0100, Peter Stuge wrote:
> Russell King - ARM Linux wrote:
> > I feel it would be better to allow the current situation to continue.
> 
> I think this misses the point, and is somewhat redundant; I think
> everyone knows that it is easiest to never change anything. But
> then nothing improves.
> 
> 
> > If we start telling people that they can't use statically declared
> > devices without first having an alternative,
> 
> I would consider it early warning that the way things have been done
> before will go away. And I would thus write drivers in a way that
> demonstrates and includes that understanding.

Clearly you haven't understood my point.

If we go down the route you suggest, we're going to end up with
everyone doing something different, which will then need to be cleaned
up once the proper solution is in place.  That could easily become
much more work than just waiting for the proper solution.

What is easier - to fix all instances which statically declare, or
to fix all instances which statically declare _and_ all the bodged up
alternative solutions?

[PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

On Wednesday 01 December 2010, Russell King - ARM Linux wrote:
> Right, so saying to ARM developers that they can't submit code which
> adds new static device structures is rather problematical then, and
> effectively brings a section of kernel development to a complete
> standstill - it means no support for additional ARM platforms until
> this issue is resolved.  (This "condition" was mentioned by Arnd
> earlier in this thread, and was put in such a way that it was now
> a hard and fast rule.)

At the embedded developer BoF in Cambridge,MA we discussed this
problem quite a bit, and my impression there was that it is a hard
rule indeed, so I said this to the best of my knowledge.

> I feel it would be better to allow the current situation to continue.
> If we start telling people that they can't use statically declared
> devices without first having an alternative, we'll end up with people
> inventing their own individual - and different - solutions to this
> problem, which could actually make the problem harder to resolve in
> the longer term.

Yes, that makes sense. We should probably start thinking about the
migration plan though. There does not seem to be a shortage of
alternatives for registering new platform devices already and once
we can get to agree on how we want it done, we can start mandating
that new drivers do it that way, while we phase out some of the
other ones.

Among the architectures that use platform devices extensively, the
various ways to register them I could find are:

* static platform_register_device:
  arm, avr32, frv, mips, m32r, sparc, sh and xtensa

* static platform_add_devices:
  arm, blackfin, m68knommu, mips, sh

* dynamic platform_device_register_simple:
  m68k, mips, powerpc, sh and x86

* dynamic platform_device_alloc/platform_device_add:
  arm, avr32, mips, powerpc, lots of drivers

* dynamic of_platform_bus_probe:
  powerpc, microblaze

* dynamic platform_device_register_resndata:
  not currently used

I was under the impression that platform_device_register_resndata is
the function that was actually meant to solve this, but there are
exactly zero users of this, except for the
platform_device_register_simple wrapper. The fact that it's currently
not used probably means either that nobody heard about it or that
the interface is lacking in some way and is actually useless for
replacing the static definitions.

	Arnd

[PATCH 09/10] MCDE: Add build files and bus

[Dave Airlie]

>
>> > Having the kms/fb/v4l2 drivers on top definitely makes sense, so
>> > these should all be able to be standalone loadable modules.
>> > I do not understand why you have a v4l2 driver at all, or why
>> > you need both fb and kms drivers, but that is probably because
>> > of my ignorance of display device drivers.
>>
>> All APIs have to be provided, these are user space API requirements.
>> KMS has a generic FB implementation. But most of KMS is modeled by
>> desktop/PC graphics cards. And while we might squeeze MCDE in to look
>> like a PC card, it might also just make things more complex and
>> restrict us to do things not possible in PC architecture.
>
> Ok, so you have identified a flaw with the existing KMS code. You should
> most certainly not try to make your driver fit into the flawed model by
> making it look like a PC. Instead, you are encouraged to fix the problems
> with KMS to make sure it can also meet your requirements. The reason
> why it doesn't do that today is that all the existing users are PC
> hardware and we don't build infrastructure that we expect to be used
> in the future but don't need yet. It would be incorrect anyway.
>
> Can you describe the shortcomings of the KSM code? I've added the dri-devel
> list to Cc, to get the attention of the right people.

I'm not sure I've a full understanding of what this bus is all about,
but I can't see
why it can't fit inside KMS, with maybe a V4L bolted on. The whole
point of KMS is to provide a consistent userspace
interface for describing the graphics hardware in enough detail that
userspace can use it, but without giving it all the gorey details.

So we've reduced the interface to crtc/encoder/connectors as the base
level objects at the interface, internally drivers can and do have
extra layers, but usually no need to show this to userspace.

KMS at the moment doesn't really handle dynamic hotplug of new crtcs
connectors etc, but I'm not sure that is needed here.

It sounds like you just have some embedded building blocks you want to
put together on a design by design basis, please correct me if I'm
wrong.

Dave.


>
>> Alex Deucher noted in a previous post that we also have the option of
>> implementing the KMS ioctls. We are looking at both options. And having
>> our own framebuffer driver might make sense since it is a very basic
>> driver, and it will allow us to easily extend support for things like
>> partial updates for display panels with on board memory. These panels
>> with memory (like DSI command mode displays) is one of the reasons why
>> KMS is not the perfect match. Since we want to expose features available
>> for these types of displays.
>
> Ok.
>
>> > > > From what I understood so far, you have a single multi-channel
>> > display
>> > > > controller (mcde_hw.c) that drives the hardware.
>> > > > Each controller can have multiple frame buffers attached to it,
>> > which
>> > > > in turn can have multiple displays attached to each of them, but
>> > your
>> > > > current configuration only has one of each, right?
>> > >
>> > > Correct, channels A/B (crtcs) can have two blended "framebuffers"
>> > plus
>> > > background color, channels C0/C1 can have one framebuffer.
>> >
>> > We might still be talking about different things here, not sure.
>>
>> In short,
>> KMS connector = MCDE port
>> KMS encoder = MCDE channel
>> KMS crtc = MCDE overlay
>
> Any chance you could change the identifiers in the code for this
> without confusing other people?
>
>> > Looking at the representation in sysfs, you should probably aim
>> > for something like
>> >
>> > /sys/devices/axi/axi0/mcde_controller
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /chnlA
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_crtc0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb1
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /v4l_0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_dbi0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb2
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /v4l_1
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /chnlB
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_ctrc1
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb3
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /chnlC
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_lcd0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb4
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /v4l_2
>> >
>> > Not sure if that is close to what your hardware would really
>> > look like. My point is that all the objects that you are
>> > dealing with as a device driver should be represented hierarchically
>> > according to how you probe them.
>>
>> Yes, mcde_bus should be connected to mcde, this is a bug. The display
>> drivers will placed in this bus, since this is where they are probed
>> like platform devices, by name (unless driver can do MIPI standard
>> probing or something). Framebuffers/V4L2 overlay devices can't be
>> put in same hierarchy, since they have multiple "parents" in case
>> the same framebuffer is cloned to multiple displays for example.
>> But I think I understand your more general point of sysfs representing
>> the "real" probe hierarchy. And this is something we will look at.
>
> Ok. If your frame buffers are not children of the displays, they should
> however be children of the controller:
>
> .../mcde_controller/
> ? ? ? ?/chnlA/
> ? ? ? ? ? ? ? ?/displ_crtc0
> ? ? ? ? ? ? ? ?/displ_dbi0
> ? ? ? ?/chnlB/
> ? ? ? ? ? ? ? ?dspl_crtc1
> ? ? ? ?/fb0
> ? ? ? ?/fb1
> ? ? ? ?/fb2
> ? ? ? ?/v4l_0
> ? ? ? ?/v4l_1
>
> Does this fit better?
>
>> > Assuming the structure above is correct and you cannot probe
>> > any of this by looking at registers, you would put a description
>> > of it into the a data structure (ideally a flattened device tree
>> > or a section of one) and let the probing happen:
>> >
>> > * The axi core registers an mcde controller as device axi0.
>> > * udev matches the device and loads the mcde hw driver from
>> > ? user space
>>
>> We are trying to avoid dynamic driver loading and udev for platform
>> devices to be able to show application graphics within a few seconds
>> after boot.
>
> That is fine, you don't need to do that for products. However, it
> is valuable to be able to do it and to think of it in this way.
> When you are able to have everything modular, it is much easier to
> spot layering violations and you can much easier define the object
> life time rules.
>
> Also, for the general case of building a cross-platform kernel,
> you want to be able to use modules for everything. Remember that
> we are targetting a single kernel binary that can run on multiple
> SoC families, potentially with hundreds of different boards.
>
>> > * the hw driver creates a device for each channel, and passes
>> > ? the channel specific configuration data to the channel device
>>
>> We have to migrate displays in runtime between different channels
>> (different use cases and different channel features), we don't want
>> to model displays as probed beneath the channel. Maybe the
>> port/connector could be a device. But that code is so small, so it
>> might just add complexity to visualize sysfs hierarchy.
>> What do you think?
>
> This makes it pretty obvious that the channel should not be a
> device, but rather something internal to the dss or hw module.
>
> What is the relation between a port/connector and a display?
> If it's 1:1, it should be the same device.
>
>> > * the dss driver gets loaded through udev and matches all the
>> > ? channels
>> > * the dss driver creates the display devices below each channel,
>> > ? according to the configuration data it got passed.
>>
>> "All" display devices need static platform_data from
>> mach-ux500/board-xx.c. This is why we have the bus,
>> to bind display dev and driver.
>
> You don't need to instantiate the device from the board though,
> just provide the data. When you add the display specific data
> to the dss data, the dss can create the display devices:
>
> static struct mcde_display_data mcde_displays[2] = {
> {
> ? ? ? ?...
> }, {
> ? ? ? ?...
> },
> };
>
> static struct mcde_dss_data {
> ? ? ? ?int num_displays;
> ? ? ? ?struct mcde_display_data *displays;
> } my_dss = {
> ? ? ? ?.num_displays = 2,
> ? ? ? ?.displays = &mcde_displays;
> };
>
> The mcde_dss probe function then takes the dss_data and iterates
> the displays, creating a new child device for each.
>
>> > * The various display drivers get loaded through udev as needed
>> > ? and match the display devices
>> > * Each display device driver initializes the display and creates
>> > ? the high-level devices (fb and v4l) as needed.
>>
>> This is setup by board/product specific code. Display drivers
>> just enable use of the HW, not defining how the displays are
>> used from user space.
>
> Right, this also gets obsolete, since as you said an fb cannot be
> the child of a display.
>
>> > * Your fb and v4l highlevel drivers get loaded through udev and
>> > ? bind to the devices, creating the user space device nodes
>> > ? through their subsystems.
>> >
>> > Now this would be the most complex scenerio that hopefully is
>> > not really needed, but I guess it illustrates the concept.
>> > I would guess that you can actually reduce this significantly
>> > if you do not actually need all the indirections.
>> >
>> > Some parts could also get simpler if you change the layering,
>> > e.g. by making the v4l and fb drivers library code and having
>> > the display drivers call them, rather than have the display
>> > drivers create the devices that get passed to upper drivers.
>>
>> Devices are static from mach-ux500/board-xx. And v4l2/fb setup
>> is board/product specific and could change dynamically.
>
> Not sure how the fb setup can be both board specific and dynamic.
> If it's statically defined per board, it should be part of the
> dss data, and dss can then create the fb devices. If it's completely
> dynamic, it gets created through user space interaction anyway.
>
>> > > > The frame buffer device also looks weird. Right now you only seem
>> > > > to have a single frame buffer registered to a driver in the same
>> > > > module. Is that frame buffer not dependent on a controller?
>> > >
>> > > MCDE framebuffers are only depending on MCDE DSS. DSS is the API that
>> > > will be used by all user space APIs so that we don't have to
>> > duplicate
>> > > the common code. We are planning mcde_kms and mcde_v4l2 drivers on
>> > top
>> > > of MCDE DSS(=Display Sub System).
>> >
>> > My impression was that you don't need a frame buffer driver if you have
>> > a kms driver, is this wrong?
>>
>> No, see above. Just that we have mcde dss to support multiple user
>> space apis by customer request. Then doing our own fb on top of
>> that is very simple and adds flexibility.
>
> This sounds like an odd thing for a customer to ask for ;-)
>
> In my experience customers want to solve specific problems like
> everyone else, they have little interest in adding complexity
> for the sake of it. Is there something wrong with one of the
> interfaces? If so, it would be better to fix that than to add
> an indirection to allow more of them!
>
>> > What does the v4l2 driver do? In my simple world, displays are for
>> > output
>> > and v4l is for input, so I must have missed something here.
>>
>> Currently nothing, since it is not finished. But the idea (and
>> requirement) is that normal graphics will use framebuffer and
>> video/camera overlays will use v4l2 overlays. Both using same
>> mcde channel and display. Some users might also configure their
>> board to use two framebuffers instead. Or maybe only use KMS etc ...
>
> I still don't understand, sorry for being slow. Why does a camera
> use a display?
>
> ? ? ? ?Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>

[PATCH 09/10] MCDE: Add build files and bus

[Alex Deucher]

On Fri, Nov 26, 2010 at 6:24 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> [dri people: please have a look at the KMS discussion way below]
>
> On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote:
>> > -----Original Message-----
>> > From: Arnd Bergmann [mailto:arnd at arndb.de]
>> > Sent: den 25 november 2010 17:48
>> > To: Marcus LORENTZON
>> > Cc: linux-arm-kernel at lists.infradead.org; Jimmy RUBIN; linux-
>> > media at vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
>> > Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
>> >
>> > On Thursday 25 November 2010, Marcus LORENTZON wrote:
>> > > From: Arnd Bergmann [mailto:arnd at arndb.de]
>> > > > On Wednesday 10 November 2010, Jimmy Rubin wrote:
>> > > > > This patch adds support for the MCDE, Memory-to-display
>> > controller,
>> > > > > found in the ST-Ericsson ux500 products.
>> > > > >
>
> [note: please configure your email client properly so it keeps
> proper attribution of text and and does not rewrap the citations
> incorrectly. Wrap your own text after 70 characters]
>
>> >
>> > All devices that you cannot probe by asking hardware or firmware are
>> > normally
>> > considered platform devices. Then again, a platform device is usally
>> > identified by its resources, i.e. MMIO addresses and interrupts, which
>> > I guess your display does not have.
>>
>> Then we might be on right track to model them as devices on a
>> platform bus. Since most displays/panels can't be "plug-n-play"
>> probed, instead devices has to be statically declared in
>> board-xx.c files in mach-ux500 folder. Or is platform bus a
>> "singleton"? Or can we define a new platform bus device?
>> Displays like HDMI TV-sets are not considered for device/driver
>> in mcde. Instead there will be a hdmi-chip-device/driver on the
>> mcde bus. So all devices and drivers on this bus are static.
>
> I think I need to clarify to things:
>
> * When I talk about a bus, I mean 'struct bus_type', which identifies
> ?all devices with a uniform bus interface to their parent device
> ?(think: PCI, USB, I2C). You seem to think of a bus as a specific
> ?instance of that bus type, i.e. the device that is the parent of
> ?all the connected devices. If you have only one instance of a bus
> ?in any system, and they are all using the same driver, do not add
> ?a bus_type for it.
> ?A good reason to add a bus_type would be e.g. if the "display"
> ?driver uses interfaces to the dss that are common among multiple
> ?dss drivers from different vendors, but the actual display drivers
> ?are identical. This does not seem to be the case.
>
> * When you say that the devices are static, I hope you do not mean
> ?static in the C language sense. We used to allow devices to be
> ?declared as "static struct" and registered using
> ?platform_device_register (or other bus specific functions). This
> ?is no longer valid and we are removing the existing users, do not
> ?add new ones. When creating a platform device, use
> ?platform_device_register_simple or platform_device_register_resndata.
>
> I'm not sure what you mean with drivers being static. Predefining
> the association between displays and drivers in per-machine files is
> fine, but since this is really board specific, it would be better
> to eventually do this through data passed from the boot loader, so
> you don't have to have a machine file for every combination of displays
> that is in the field.
>
>> > Staging it in a way that adds all the display drivers later than the
>> > base driver is a good idea, but it would be helpful to also add the
>> > infrastructure at the later stage. Maybe you can try to simplify the
>> > code for now by hardcoding the single display and remove the dynamic
>> > registration. You still have the the code, so once the base code looks
>> > good for inclusion, we can talk about it in the context of adding
>> > dynamic display support back in, possibly in exactly the way you are
>> > proposing now, but perhaps in an entirely different way if we come up
>> > with a better solution.
>>
>> What about starting with MCDE HW, which is the core HW driver doing
>> all real work? And then continue with the infrastructure + some displays
>> + drivers ...
>
> This is already the order in which you submitted them, I don't see a
> difference here. I was not asking to delay any of the code, just to put
> them in a logical order.
>
>> Only problem is that we then have a driver that can't be used from user
>> space, because I don't think I can find anyone with enough time to write
>> a display driver + framebuffer on top of mcde_hw (which is what the
>> existing code does).
>
> Well, developer time does not appear to be one of your problems, you
> already wasted tons of it by developing a huge chunk of code that isn't
> going anywhere because you wrote it without consulting the upstream
> community ;-)
>
> There is no need to develop anything from scratch here, you already have
> the code you want to end up with. What I would do here is to start with
> a single git commit that adds the full driver. Then take out bits you
> don't absolutely need to keep the driver from showing text on your
> screen (not necessarily in this order):
>
> * Take out display drivers one by one, until there is only one left.
> ?Do a git commit after each driver
> * Take out the register definitions that are not actually used in your
> ?code
> * Remove the infrastructure for dynamic displays and hardcode the one
> ?you use
> * Take out the frame buffer code
> * Take out the infrastructure for multiple user-interfaces, hardcoding KMS
> ?to the DSS
> * Anything else you don't absolutely need.
>
> Finally, you should end up with a very lean driver that only does a
> single thing and only works on one very specific board. Remove that, too,
> in a final commit. Now use git to reverse the patch order and you have
> a nice series that you can use for patch submission, one feature at a
> time. Then we can discuss the individual merits of each patch.
>
> In the future, best plan for how you want to submit the code while
> you're writing it, instead of as an afterthought. Quite often, the
> first patch to submit is also one of the early stages of the driver,
> so there is no need to wait for the big picture before you start
> submitting. This way, we can work out conceptual mistakes early on,
> saving a lot of your time, and the reviewer's time as well.
>
>> > For the case where all modules are built-in, you can rely in link-order
>> > in the Makefile, e.g.
>> >
>> > obj-$(CONFIG_FOO_BASE) ? ? ? ? ? ? ? ?+= foo_base.o
>> > obj-$(CONFIG_FOO_SPECIFIC) ? ?+= foo_specific.o # this comes after
>> > foo_base
>>
>> Ok, we will do this for the mcde stuff. How do we handle stuff that span
>> different kernel folders? Like drivers/misc and drivers/video/mcde etc.
>> We can't just change the order of top level makefiles, that would break
>> other drivers I guess.
>
> Right, you have to find a different solution for those. Most importantly,
> a module in one directory should not have intimate knowledge of data
> structures in a different module in another directory.
>
> In your example, drivers/misc is probably wrong anyway. Try ignoring this
> problem at first by forcing all the drivers loadable modules, which will
> naturally fix the initialization order. When you still have link order
> problems by building all the drivers into the kernel after this, we can
> have another look to find the least ugly solution.
>
>> > > > I'm not sure how the other parts layer on top of one another, can
>> > you
>> > > > provide some more insight?
>> > >
>> > > +----------------------------+
>> > > | mcde_fb/mcde_kms/mcde_v4l2 |
>> > > +---------------+------------+
>> > > | ? ?mcde_dss ? |
>> > > + ? +-----------+
>> > > | ? | disp drvs |
>> > > +---+-----------+
>> > > | ? ?mcde hw ? ?|
>> > > +---------------+
>> > > | ? ? ?HW ? ? ? |
>> > > +---------------+
>> >
>> > Ok. One problem with this is that once you have a multitude of
>> > display drivers, you can no longer layer them below mcde_dss.
>>
>> Not sure what you mean, we have plenty of drivers and devices already.
>> Maybe I should try to clarify picture.
>
> I mean the layering of loadable modules: you cannot make a high-level
> module link against multiple low-level modules that export the
> same interface. If you have multiple modules that implement the same
> interface like you diplay drivers, they need to be on top!
>
>> DSS give access to all display devices probed on the virtual mcde
>> dss bus, or platform bus with specific type of devices if you like.
>> All calls to DSS operate on a display device, like create an
>> overlay(=framebuffer), request an update, set power mode, etc.
>> All calls to DSS related to display itself and not only framebuffer
>> scanout, will be passed on to the display driver of the display
>> device in question. All calls DSS only related to overlays, like
>> buffer pointers, position, rotation etc is handled directly by DSS
>> calling mcde_hw.
>>
>> You could see mcde_hw as a physical level driver and mcde_dss closer
>> to a logical driver, delegating display specific decisions to the
>> display driver. Another analogy is mcde_hw is host driver and display
>> drivers are client device drivers. And DSS is a collection of logic
>> to manage the interaction between host and client devices.
>
> The way you describe it, I would picture it differently:
>
> +----------+ +----+-----+-----+ +-------+
> | mcde_hw ?| | fb | kms | v4l | | displ |
> +----+----------------------------------+
> | HW | ? ? ? ? ? ?mcde_dss ? ? ? ? ? ? ?|
> +----+----------------------------------+
>
> In this model, the dss is the core module that everything else
> links to. The hw driver talks to the actual hardware and to the
> dss. The three front-ends only talk to the dss, but not to the
> individual display drivers or to the hw code directly (i.e. they
> don't use their exported symbols or internal data structures.
> The display drivers only talk to the dss, but not to the front-ends
> or the hw drivers.
>
> Would this be a correct representation of your modules?
>
>> > Having the kms/fb/v4l2 drivers on top definitely makes sense, so
>> > these should all be able to be standalone loadable modules.
>> > I do not understand why you have a v4l2 driver at all, or why
>> > you need both fb and kms drivers, but that is probably because
>> > of my ignorance of display device drivers.
>>
>> All APIs have to be provided, these are user space API requirements.
>> KMS has a generic FB implementation. But most of KMS is modeled by
>> desktop/PC graphics cards. And while we might squeeze MCDE in to look
>> like a PC card, it might also just make things more complex and
>> restrict us to do things not possible in PC architecture.
>
> Ok, so you have identified a flaw with the existing KMS code. You should
> most certainly not try to make your driver fit into the flawed model by
> making it look like a PC. Instead, you are encouraged to fix the problems
> with KMS to make sure it can also meet your requirements. The reason
> why it doesn't do that today is that all the existing users are PC
> hardware and we don't build infrastructure that we expect to be used
> in the future but don't need yet. It would be incorrect anyway.
>
> Can you describe the shortcomings of the KSM code? I've added the dri-devel
> list to Cc, to get the attention of the right people.

This doesn't seem that different from the graphics chips we support
with kms.  I don't think it would require much work to use KMS.  One
thing we considered, but never ended up implementing was a generic
overlay API for KMS.  Most PC hardware still has overlays, but we
don't use them much any more on the desktop side.  It may be
worthwhile to design an appropriate API for them for these type of
hardware.

To elaborate on the current KMS design, we have:
crtcs - the display controller.  these map to the timing generators
and scanout hardware
encoders - the hw that takes the bitstream from the display controller
and converts it to the appropriate format for the connected display.
connector - the physical interface that a display attaches to (VGA,
LVDS, eDP, HDMI-A, etc.)

Modern PC hardware is pretty complex.  I've blogged about some of the
recent radeon display hardware:
http://www.botchco.com/agd5f/?p=51
Moreover, each oem designs different boards with vastly different
display configurations.  It gets more complex with things like
advanced color management and DP (DisplayPort) 1.2 that introduces
things like daisy-chaining monitors and tunnelling USB and audio over
DP.


>
>> Alex Deucher noted in a previous post that we also have the option of
>> implementing the KMS ioctls. We are looking at both options. And having
>> our own framebuffer driver might make sense since it is a very basic
>> driver, and it will allow us to easily extend support for things like
>> partial updates for display panels with on board memory. These panels
>> with memory (like DSI command mode displays) is one of the reasons why
>> KMS is not the perfect match. Since we want to expose features available
>> for these types of displays.
>
> Ok.
>
>> > > > From what I understood so far, you have a single multi-channel
>> > display
>> > > > controller (mcde_hw.c) that drives the hardware.
>> > > > Each controller can have multiple frame buffers attached to it,
>> > which
>> > > > in turn can have multiple displays attached to each of them, but
>> > your
>> > > > current configuration only has one of each, right?
>> > >
>> > > Correct, channels A/B (crtcs) can have two blended "framebuffers"
>> > plus
>> > > background color, channels C0/C1 can have one framebuffer.
>> >
>> > We might still be talking about different things here, not sure.
>>
>> In short,
>> KMS connector = MCDE port
>> KMS encoder = MCDE channel
>> KMS crtc = MCDE overlay
>
> Any chance you could change the identifiers in the code for this
> without confusing other people?
>
>> > Looking at the representation in sysfs, you should probably aim
>> > for something like
>> >
>> > /sys/devices/axi/axi0/mcde_controller
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /chnlA
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_crtc0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb1
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /v4l_0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_dbi0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb2
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /v4l_1
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /chnlB
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_ctrc1
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb3
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /chnlC
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /dspl_lcd0
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /fb4
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /v4l_2
>> >
>> > Not sure if that is close to what your hardware would really
>> > look like. My point is that all the objects that you are
>> > dealing with as a device driver should be represented hierarchically
>> > according to how you probe them.
>>
>> Yes, mcde_bus should be connected to mcde, this is a bug. The display
>> drivers will placed in this bus, since this is where they are probed
>> like platform devices, by name (unless driver can do MIPI standard
>> probing or something). Framebuffers/V4L2 overlay devices can't be
>> put in same hierarchy, since they have multiple "parents" in case
>> the same framebuffer is cloned to multiple displays for example.
>> But I think I understand your more general point of sysfs representing
>> the "real" probe hierarchy. And this is something we will look at.
>
> Ok. If your frame buffers are not children of the displays, they should
> however be children of the controller:
>
> .../mcde_controller/
> ? ? ? ?/chnlA/
> ? ? ? ? ? ? ? ?/displ_crtc0
> ? ? ? ? ? ? ? ?/displ_dbi0
> ? ? ? ?/chnlB/
> ? ? ? ? ? ? ? ?dspl_crtc1
> ? ? ? ?/fb0
> ? ? ? ?/fb1
> ? ? ? ?/fb2
> ? ? ? ?/v4l_0
> ? ? ? ?/v4l_1
>
> Does this fit better?
>
>> > Assuming the structure above is correct and you cannot probe
>> > any of this by looking at registers, you would put a description
>> > of it into the a data structure (ideally a flattened device tree
>> > or a section of one) and let the probing happen:
>> >
>> > * The axi core registers an mcde controller as device axi0.
>> > * udev matches the device and loads the mcde hw driver from
>> > ? user space
>>
>> We are trying to avoid dynamic driver loading and udev for platform
>> devices to be able to show application graphics within a few seconds
>> after boot.
>
> That is fine, you don't need to do that for products. However, it
> is valuable to be able to do it and to think of it in this way.
> When you are able to have everything modular, it is much easier to
> spot layering violations and you can much easier define the object
> life time rules.
>
> Also, for the general case of building a cross-platform kernel,
> you want to be able to use modules for everything. Remember that
> we are targetting a single kernel binary that can run on multiple
> SoC families, potentially with hundreds of different boards.
>
>> > * the hw driver creates a device for each channel, and passes
>> > ? the channel specific configuration data to the channel device
>>
>> We have to migrate displays in runtime between different channels
>> (different use cases and different channel features), we don't want
>> to model displays as probed beneath the channel. Maybe the
>> port/connector could be a device. But that code is so small, so it
>> might just add complexity to visualize sysfs hierarchy.
>> What do you think?
>
> This makes it pretty obvious that the channel should not be a
> device, but rather something internal to the dss or hw module.
>
> What is the relation between a port/connector and a display?
> If it's 1:1, it should be the same device.
>
>> > * the dss driver gets loaded through udev and matches all the
>> > ? channels
>> > * the dss driver creates the display devices below each channel,
>> > ? according to the configuration data it got passed.
>>
>> "All" display devices need static platform_data from
>> mach-ux500/board-xx.c. This is why we have the bus,
>> to bind display dev and driver.
>
> You don't need to instantiate the device from the board though,
> just provide the data. When you add the display specific data
> to the dss data, the dss can create the display devices:
>
> static struct mcde_display_data mcde_displays[2] = {
> {
> ? ? ? ?...
> }, {
> ? ? ? ?...
> },
> };
>
> static struct mcde_dss_data {
> ? ? ? ?int num_displays;
> ? ? ? ?struct mcde_display_data *displays;
> } my_dss = {
> ? ? ? ?.num_displays = 2,
> ? ? ? ?.displays = &mcde_displays;
> };
>
> The mcde_dss probe function then takes the dss_data and iterates
> the displays, creating a new child device for each.
>
>> > * The various display drivers get loaded through udev as needed
>> > ? and match the display devices
>> > * Each display device driver initializes the display and creates
>> > ? the high-level devices (fb and v4l) as needed.
>>
>> This is setup by board/product specific code. Display drivers
>> just enable use of the HW, not defining how the displays are
>> used from user space.
>
> Right, this also gets obsolete, since as you said an fb cannot be
> the child of a display.
>
>> > * Your fb and v4l highlevel drivers get loaded through udev and
>> > ? bind to the devices, creating the user space device nodes
>> > ? through their subsystems.
>> >
>> > Now this would be the most complex scenerio that hopefully is
>> > not really needed, but I guess it illustrates the concept.
>> > I would guess that you can actually reduce this significantly
>> > if you do not actually need all the indirections.
>> >
>> > Some parts could also get simpler if you change the layering,
>> > e.g. by making the v4l and fb drivers library code and having
>> > the display drivers call them, rather than have the display
>> > drivers create the devices that get passed to upper drivers.
>>
>> Devices are static from mach-ux500/board-xx. And v4l2/fb setup
>> is board/product specific and could change dynamically.
>
> Not sure how the fb setup can be both board specific and dynamic.
> If it's statically defined per board, it should be part of the
> dss data, and dss can then create the fb devices. If it's completely
> dynamic, it gets created through user space interaction anyway.
>
>> > > > The frame buffer device also looks weird. Right now you only seem
>> > > > to have a single frame buffer registered to a driver in the same
>> > > > module. Is that frame buffer not dependent on a controller?
>> > >
>> > > MCDE framebuffers are only depending on MCDE DSS. DSS is the API that
>> > > will be used by all user space APIs so that we don't have to
>> > duplicate
>> > > the common code. We are planning mcde_kms and mcde_v4l2 drivers on
>> > top
>> > > of MCDE DSS(=Display Sub System).
>> >
>> > My impression was that you don't need a frame buffer driver if you have
>> > a kms driver, is this wrong?
>>
>> No, see above. Just that we have mcde dss to support multiple user
>> space apis by customer request. Then doing our own fb on top of
>> that is very simple and adds flexibility.
>
> This sounds like an odd thing for a customer to ask for ;-)
>
> In my experience customers want to solve specific problems like
> everyone else, they have little interest in adding complexity
> for the sake of it. Is there something wrong with one of the
> interfaces? If so, it would be better to fix that than to add
> an indirection to allow more of them!
>
>> > What does the v4l2 driver do? In my simple world, displays are for
>> > output
>> > and v4l is for input, so I must have missed something here.
>>
>> Currently nothing, since it is not finished. But the idea (and
>> requirement) is that normal graphics will use framebuffer and
>> video/camera overlays will use v4l2 overlays. Both using same
>> mcde channel and display. Some users might also configure their
>> board to use two framebuffers instead. Or maybe only use KMS etc ...
>
> I still don't understand, sorry for being slow. Why does a camera
> use a display?
>
> ? ? ? ?Arnd
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH 09/10] MCDE: Add build files and bus

[Daniel Vetter]

On Sat, Dec 04, 2010 at 04:34:22PM -0500, Alex Deucher wrote:
> This doesn't seem that different from the graphics chips we support
> with kms.  I don't think it would require much work to use KMS.  One
> thing we considered, but never ended up implementing was a generic
> overlay API for KMS.  Most PC hardware still has overlays, but we
> don't use them much any more on the desktop side.  It may be
> worthwhile to design an appropriate API for them for these type of
> hardware.

Just fyi about a generic overlay api: I've looked a bit into this when
doing the intel overlay support and I think adding special overlay crtcs
that can be attached real crtcs gives a nice clean api. We could the reuse
the existing framebuffer/pageflipping api to get the buffers to the
overlay engine.

The real pain starts when we want format discovery from userspace with all
the alignement/size/layout constrains. Add in tiling support and its
almost impossible to do in a generic way. But also for kms userspace needs
to know these constrains (implemented for generic use in libkms). I favor
such an approach for overlays, too (if it ever happens) - i.e. a few
helpers in libkms that allocate an appropriate buffer for a given format
and size and returns the buffer, strides and offsets for the different
planes.

-Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 09/10] MCDE: Add build files and bus

[Marcus Lorentzon]

On 11/26/2010 12:24 PM, Arnd Bergmann wrote:
> [dri people: please have a look at the KMS discussion way below]
>
> On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote:
>    
>>> -----Original Message-----
>>> From: Arnd Bergmann [mailto:arnd at arndb.de]
>>> Sent: den 25 november 2010 17:48
>>> To: Marcus LORENTZON
>>> Cc: linux-arm-kernel at lists.infradead.org; Jimmy RUBIN; linux-
>>> media at vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
>>> Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
>>>
>>> On Thursday 25 November 2010, Marcus LORENTZON wrote:
>>>        
>>>> From: Arnd Bergmann [mailto:arnd at arndb.de]
>>>>          
>>>>> On Wednesday 10 November 2010, Jimmy Rubin wrote:
>>>>>            
>>>>>> This patch adds support for the MCDE, Memory-to-display
>>>>>>              
>>> controller,
>>>        
>>>>>> found in the ST-Ericsson ux500 products.
>>>>>>
>>>>>>              
> [note: please configure your email client properly so it keeps
> proper attribution of text and and does not rewrap the citations
> incorrectly. Wrap your own text after 70 characters]
>    
I'm now using Thunderbird, please let me know if it's better than my 
previous webmail client, neither have many features for reply formatting.
>>> All devices that you cannot probe by asking hardware or firmware are
>>> normally
>>> considered platform devices. Then again, a platform device is usally
>>> identified by its resources, i.e. MMIO addresses and interrupts, which
>>> I guess your display does not have.
>>>        
>> Then we might be on right track to model them as devices on a
>> platform bus. Since most displays/panels can't be "plug-n-play"
>> probed, instead devices has to be statically declared in
>> board-xx.c files in mach-ux500 folder. Or is platform bus a
>> "singleton"? Or can we define a new platform bus device?
>> Displays like HDMI TV-sets are not considered for device/driver
>> in mcde. Instead there will be a hdmi-chip-device/driver on the
>> mcde bus. So all devices and drivers on this bus are static.
>>      
> I think I need to clarify to things:
>
> * When I talk about a bus, I mean 'struct bus_type', which identifies
>    all devices with a uniform bus interface to their parent device
>    (think: PCI, USB, I2C). You seem to think of a bus as a specific
>    instance of that bus type, i.e. the device that is the parent of
>    all the connected devices. If you have only one instance of a bus
>    in any system, and they are all using the same driver, do not add
>    a bus_type for it.
>    A good reason to add a bus_type would be e.g. if the "display"
>    driver uses interfaces to the dss that are common among multiple
>    dss drivers from different vendors, but the actual display drivers
>    are identical. This does not seem to be the case.
>    
Correct, I refer to the device, not type or driver. I used a bus type 
since it allowed me to setup a default implementation for each driver 
callback. So all drivers get generic implementation by default, and 
override when that is not enough. Meybe you have a better way of getting 
the same behavior.
> * When you say that the devices are static, I hope you do not mean
>    static in the C language sense. We used to allow devices to be
>    declared as "static struct" and registered using
>    platform_device_register (or other bus specific functions). This
>    is no longer valid and we are removing the existing users, do not
>    add new ones. When creating a platform device, use
>    platform_device_register_simple or platform_device_register_resndata.
>
> I'm not sure what you mean with drivers being static. Predefining
> the association between displays and drivers in per-machine files is
> fine, but since this is really board specific, it would be better
> to eventually do this through data passed from the boot loader, so
> you don't have to have a machine file for every combination of displays
> that is in the field.
>    
I guess you have read the ARM vs static platform_devices. But, yes, I 
mean in the c-language static sense. I will adopt to whatever Russel 
King says is The right way in ARM SoCs.
>>> Staging it in a way that adds all the display drivers later than the
>>> base driver is a good idea, but it would be helpful to also add the
>>> infrastructure at the later stage. Maybe you can try to simplify the
>>> code for now by hardcoding the single display and remove the dynamic
>>> registration. You still have the the code, so once the base code looks
>>> good for inclusion, we can talk about it in the context of adding
>>> dynamic display support back in, possibly in exactly the way you are
>>> proposing now, but perhaps in an entirely different way if we come up
>>> with a better solution.
>>>        
>> What about starting with MCDE HW, which is the core HW driver doing
>> all real work? And then continue with the infrastructure + some displays
>> + drivers ...
> This is already the order in which you submitted them, I don't see a
> difference here. I was not asking to delay any of the code, just to put
> them in a logical order.
>    
We are now taking a step back and start "all over". We were almost as 
fresh on this HW block as you are now when we started implementing the 
driver earlier this year. I think all of us benefit from now having a 
better understanding of customer requirements and the HW itself, there 
are some nice quirks ;). Anyway, we will restart the patches and RFC 
only the MCDE HW part of the driver, implementing basic fb support for 
one display board as you suggested initially. It's a nice step towards 
making the patches easier to review and give us some time to prepare the 
DSS stuff. That remake was done today, so I think the patch will be sent 
out soon. (I'm going on vacation for 3 weeks btw).
>> Only problem is that we then have a driver that can't be used from user
>> space, because I don't think I can find anyone with enough time to write
>> a display driver + framebuffer on top of mcde_hw (which is what the
>> existing code does).
>>      
> Well, developer time does not appear to be one of your problems, you
> already wasted tons of it by developing a huge chunk of code that isn't
> going anywhere because you wrote it without consulting the upstream
> community ;-)
>    
Hope not, we have learned a lot, and we are now ready for a second 
refactoring of the driver. Now that most of the features needed are in 
place. Allowing us also to remove any driver code/feature that was never 
needed.
> There is no need to develop anything from scratch here, you already have
> the code you want to end up with. What I would do here is to start with
> a single git commit that adds the full driver. Then take out bits you
> don't absolutely need to keep the driver from showing text on your
> screen (not necessarily in this order):
>
> * Take out display drivers one by one, until there is only one left.
>    Do a git commit after each driver
> * Take out the register definitions that are not actually used in your
>    code
> * Remove the infrastructure for dynamic displays and hardcode the one
>    you use
> * Take out the frame buffer code
> * Take out the infrastructure for multiple user-interfaces, hardcoding KMS
>    to the DSS
> * Anything else you don't absolutely need.
>
> Finally, you should end up with a very lean driver that only does a
> single thing and only works on one very specific board. Remove that, too,
> in a final commit. Now use git to reverse the patch order and you have
> a nice series that you can use for patch submission, one feature at a
> time. Then we can discuss the individual merits of each patch.
>
> In the future, best plan for how you want to submit the code while
> you're writing it, instead of as an afterthought. Quite often, the
> first patch to submit is also one of the early stages of the driver,
> so there is no need to wait for the big picture before you start
> submitting. This way, we can work out conceptual mistakes early on,
> saving a lot of your time, and the reviewer's time as well.
>    
This is how we will try to work now that we know how the HW works. I you 
feel we are not, please let me know :).
>>> For the case where all modules are built-in, you can rely in link-order
>>> in the Makefile, e.g.
>>>
>>> obj-$(CONFIG_FOO_BASE)                += foo_base.o
>>> obj-$(CONFIG_FOO_SPECIFIC)    += foo_specific.o # this comes after
>>> foo_base
>>>        
>> Ok, we will do this for the mcde stuff. How do we handle stuff that span
>> different kernel folders? Like drivers/misc and drivers/video/mcde etc.
>> We can't just change the order of top level makefiles, that would break
>> other drivers I guess.
>>      
> Right, you have to find a different solution for those. Most importantly,
> a module in one directory should not have intimate knowledge of data
> structures in a different module in another directory.
>
> In your example, drivers/misc is probably wrong anyway. Try ignoring this
> problem at first by forcing all the drivers loadable modules, which will
> naturally fix the initialization order. When you still have link order
> problems by building all the drivers into the kernel after this, we can
> have another look to find the least ugly solution.
>    

Relying on per folder load order might solve most of the ordering 
issues. Will do!
>>>>> I'm not sure how the other parts layer on top of one another, can
>>>>>            
>>> you
>>>        
>>>>> provide some more insight?
>>>>>            
>>>> +----------------------------+
>>>> | mcde_fb/mcde_kms/mcde_v4l2 |
>>>> +---------------+------------+
>>>> |    mcde_dss   |
>>>> +   +-----------+
>>>> |   | disp drvs |
>>>> +---+-----------+
>>>> |    mcde hw    |
>>>> +---------------+
>>>> |      HW       |
>>>> +---------------+
>>>>          
>>> Ok. One problem with this is that once you have a multitude of
>>> display drivers, you can no longer layer them below mcde_dss.
>>>        
>> Not sure what you mean, we have plenty of drivers and devices already.
>> Maybe I should try to clarify picture.
>>      
> I mean the layering of loadable modules: you cannot make a high-level
> module link against multiple low-level modules that export the
> same interface. If you have multiple modules that implement the same
> interface like you diplay drivers, they need to be on top!
>    
I don't think we do. The layers are very strict. If you found some code 
not following the layering rules please let me know and we will fix it.
>> DSS give access to all display devices probed on the virtual mcde
>> dss bus, or platform bus with specific type of devices if you like.
>> All calls to DSS operate on a display device, like create an
>> overlay(=framebuffer), request an update, set power mode, etc.
>> All calls to DSS related to display itself and not only framebuffer
>> scanout, will be passed on to the display driver of the display
>> device in question. All calls DSS only related to overlays, like
>> buffer pointers, position, rotation etc is handled directly by DSS
>> calling mcde_hw.
>>
>> You could see mcde_hw as a physical level driver and mcde_dss closer
>> to a logical driver, delegating display specific decisions to the
>> display driver. Another analogy is mcde_hw is host driver and display
>> drivers are client device drivers. And DSS is a collection of logic
>> to manage the interaction between host and client devices.
>>      
> The way you describe it, I would picture it differently:
>
> +----------+ +----+-----+-----+ +-------+
> | mcde_hw  | | fb | kms | v4l | | displ |
> +----+----------------------------------+
> | HW |            mcde_dss              |
> +----+----------------------------------+
>
> In this model, the dss is the core module that everything else
> links to. The hw driver talks to the actual hardware and to the
> dss. The three front-ends only talk to the dss, but not to the
> individual display drivers or to the hw code directly (i.e. they
> don't use their exported symbols or internal data structures.
> The display drivers only talk to the dss, but not to the front-ends
> or the hw drivers.
>
> Would this be a correct representation of your modules?
>    
Hmm, mcde_hw does not link to dss. It should be FB->DSS->Display 
driver->MCDE_HW->HW IO (+ DSS->MCDE_HW). My picture is how code should 
be used. Anything else you find in code is a violation of that layering.
>>> Having the kms/fb/v4l2 drivers on top definitely makes sense, so
>>> these should all be able to be standalone loadable modules.
>>> I do not understand why you have a v4l2 driver at all, or why
>>> you need both fb and kms drivers, but that is probably because
>>> of my ignorance of display device drivers.
>>>        
>> All APIs have to be provided, these are user space API requirements.
>> KMS has a generic FB implementation. But most of KMS is modeled by
>> desktop/PC graphics cards. And while we might squeeze MCDE in to look
>> like a PC card, it might also just make things more complex and
>> restrict us to do things not possible in PC architecture.
>>      
> Ok, so you have identified a flaw with the existing KMS code. You should
> most certainly not try to make your driver fit into the flawed model by
> making it look like a PC. Instead, you are encouraged to fix the problems
> with KMS to make sure it can also meet your requirements. The reason
> why it doesn't do that today is that all the existing users are PC
> hardware and we don't build infrastructure that we expect to be used
> in the future but don't need yet. It would be incorrect anyway.
>
> Can you describe the shortcomings of the KSM code? I've added the dri-devel
> list to Cc, to get the attention of the right people.
>    
I will start this work early next year. MCDE DSS refactoring will take 
KMS into account. Some of the _possible_ short comings (I must say I 
have not looked into this in any detail yet):
- 3D HW is bundled with display HW. Makes it harder for us to use 
different 3D HW with same display HW or the other way around. I would 
like KMS and "DRM3D" to be more separated. We get DRM 3D drivers from IP 
vendors, but we still have to expose our own KMS DRM device. The other 
"issue" is the usual, 3D vendors don't upstream their drivers. Which 
means we have to integrate with drivers not in mainline kernel ... and 
we still want to open all our drivers, even if some external IPs don't.
- GEM user space buffer API has a security model and IPC sharing not 
compatible (at first glance and after short discussion with Chris 
Wilson) with Android (binder fdup) or for protecting buffers from the 
user. As I understand it correctly, GEM master, once client 
authenticated, you have access to all buffers.
- Partial updates, overlay support and pushing any buffer to scanout. 
Some might be possible with the latest ioctls in KMS, will look at this.

But as I said, I have not had time to look at this yet. Framebuffer was 
just so much easier to implement and the only customer requirement.
>> Alex Deucher noted in a previous post that we also have the option of
>> implementing the KMS ioctls. We are looking at both options. And having
>> our own framebuffer driver might make sense since it is a very basic
>> driver, and it will allow us to easily extend support for things like
>> partial updates for display panels with on board memory. These panels
>> with memory (like DSI command mode displays) is one of the reasons why
>> KMS is not the perfect match. Since we want to expose features available
>> for these types of displays.
>>      
> Ok.
>    
>>>>>  From what I understood so far, you have a single multi-channel
>>>>>            
>>> display
>>>        
>>>>> controller (mcde_hw.c) that drives the hardware.
>>>>> Each controller can have multiple frame buffers attached to it,
>>>>>            
>>> which
>>>        
>>>>> in turn can have multiple displays attached to each of them, but
>>>>>            
>>> your
>>>        
>>>>> current configuration only has one of each, right?
>>>>>            
>>>> Correct, channels A/B (crtcs) can have two blended "framebuffers"
>>>>          
>>> plus
>>>        
>>>> background color, channels C0/C1 can have one framebuffer.
>>>>          
>>> We might still be talking about different things here, not sure.
>>>        
>> In short,
>> KMS connector = MCDE port
>> KMS encoder = MCDE channel
>> KMS crtc = MCDE overlay
>>      
> Any chance you could change the identifiers in the code for this
> without confusing other people?
>
>    
I will see, but if it's not exactly the same it might confuse even more. 
But I'll definitely look at it.
>>> Looking at the representation in sysfs, you should probably aim
>>> for something like
>>>
>>> /sys/devices/axi/axi0/mcde_controller
>>>                                /chnlA
>>>                                        /dspl_crtc0
>>>                                                /fb0
>>>                                                /fb1
>>>                                                /v4l_0
>>>                                        /dspl_dbi0
>>>                                                /fb2
>>>                                                /v4l_1
>>>                                /chnlB
>>>                                        /dspl_ctrc1
>>>                                                /fb3
>>>                                /chnlC
>>>                                        /dspl_lcd0
>>>                                                /fb4
>>>                                                /v4l_2
>>>
>>> Not sure if that is close to what your hardware would really
>>> look like. My point is that all the objects that you are
>>> dealing with as a device driver should be represented hierarchically
>>> according to how you probe them.
>>>        
>> Yes, mcde_bus should be connected to mcde, this is a bug. The display
>> drivers will placed in this bus, since this is where they are probed
>> like platform devices, by name (unless driver can do MIPI standard
>> probing or something). Framebuffers/V4L2 overlay devices can't be
>> put in same hierarchy, since they have multiple "parents" in case
>> the same framebuffer is cloned to multiple displays for example.
>> But I think I understand your more general point of sysfs representing
>> the "real" probe hierarchy. And this is something we will look at.
>>      
> Ok. If your frame buffers are not children of the displays, they should
> however be children of the controller:
>
> .../mcde_controller/
>          /chnlA/
>                  /displ_crtc0
>                  /displ_dbi0
>          /chnlB/
>                  dspl_crtc1
>          /fb0
>          /fb1
>          /fb2
>          /v4l_0
>          /v4l_1
>
> Does this fit better?
>
>    
Maybe, will try to find a better structure for relations. Not something 
I've considered before. But I see your point.
BTW. Can this hierarchy be changed in runtime? When for example one 
display move from one channel to another. There's a lot of muxing going 
on in the HW and that is hard to visualize in a static tree structure. A 
flat structure might be better then.
>>> Assuming the structure above is correct and you cannot probe
>>> any of this by looking at registers, you would put a description
>>> of it into the a data structure (ideally a flattened device tree
>>> or a section of one) and let the probing happen:
>>>
>>> * The axi core registers an mcde controller as device axi0.
>>> * udev matches the device and loads the mcde hw driver from
>>>    user space
>>>        
>> We are trying to avoid dynamic driver loading and udev for platform
>> devices to be able to show application graphics within a few seconds
>> after boot.
>>      
> That is fine, you don't need to do that for products. However, it
> is valuable to be able to do it and to think of it in this way.
> When you are able to have everything modular, it is much easier to
> spot layering violations and you can much easier define the object
> life time rules.
>
> Also, for the general case of building a cross-platform kernel,
> you want to be able to use modules for everything. Remember that
> we are targetting a single kernel binary that can run on multiple
> SoC families, potentially with hundreds of different boards.
>
>    
Initially the driver was developed as a module since it's easier during 
development. I will do my best to enable this feature again.
>>> * the hw driver creates a device for each channel, and passes
>>>    the channel specific configuration data to the channel device
>>>        
>> We have to migrate displays in runtime between different channels
>> (different use cases and different channel features), we don't want
>> to model displays as probed beneath the channel. Maybe the
>> port/connector could be a device. But that code is so small, so it
>> might just add complexity to visualize sysfs hierarchy.
>> What do you think?
>>      
> This makes it pretty obvious that the channel should not be a
> device, but rather something internal to the dss or hw module.
>
>    
And that is the way it is now.
> What is the relation between a port/connector and a display?
> If it's 1:1, it should be the same device.
>
>    
A port is product specific display device data. Just a structure used to 
describe the MCDE<->Display/panel physical connection. The display 
device resource is you like. Port data describe the SoC-wires-display 
connection. Where are the display platform device struct describe the on 
SoC display configuration. Like initial color depth, what MCDE 
channel/encoder to use etc.
>>> * the dss driver gets loaded through udev and matches all the
>>>    channels
>>> * the dss driver creates the display devices below each channel,
>>>    according to the configuration data it got passed.
>>>        
>> "All" display devices need static platform_data from
>> mach-ux500/board-xx.c. This is why we have the bus,
>> to bind display dev and driver.
>>      
> You don't need to instantiate the device from the board though,
> just provide the data. When you add the display specific data
> to the dss data, the dss can create the display devices:
>
> static struct mcde_display_data mcde_displays[2] = {
> {
>          ...
> }, {
>          ...
> },
> };
>
> static struct mcde_dss_data {
>          int num_displays;
>          struct mcde_display_data *displays;
> } my_dss = {
>          .num_displays = 2,
>          .displays =&mcde_displays;
> };
>
> The mcde_dss probe function then takes the dss_data and iterates
> the displays, creating a new child device for each.
>
>    
To me this is exactly the same as the static devices we now have. Same 
amount of static data. And if you don't register the device, I don't see 
the difference. I will follow the ARM discussions on c-static platform 
devices and adopt.
>>> * The various display drivers get loaded through udev as needed
>>>    and match the display devices
>>> * Each display device driver initializes the display and creates
>>>    the high-level devices (fb and v4l) as needed.
>>>        
>> This is setup by board/product specific code. Display drivers
>> just enable use of the HW, not defining how the displays are
>> used from user space.
>>      
> Right, this also gets obsolete, since as you said an fb cannot be
> the child of a display.
>
>    
>>> * Your fb and v4l highlevel drivers get loaded through udev and
>>>    bind to the devices, creating the user space device nodes
>>>    through their subsystems.
>>>
>>> Now this would be the most complex scenerio that hopefully is
>>> not really needed, but I guess it illustrates the concept.
>>> I would guess that you can actually reduce this significantly
>>> if you do not actually need all the indirections.
>>>
>>> Some parts could also get simpler if you change the layering,
>>> e.g. by making the v4l and fb drivers library code and having
>>> the display drivers call them, rather than have the display
>>> drivers create the devices that get passed to upper drivers.
>>>        
>> Devices are static from mach-ux500/board-xx. And v4l2/fb setup
>> is board/product specific and could change dynamically.
>>      
> Not sure how the fb setup can be both board specific and dynamic.
> If it's statically defined per board, it should be part of the
> dss data, and dss can then create the fb devices. If it's completely
> dynamic, it gets created through user space interaction anyway.
>
>    
The default is setup dynamically by static calls in board init code. 
User space will then be able to change this config. This is one of the 
features that is not heavily used and might get removed. Like Multihead 
framebuffers or framebuffer cloning to multiple displays. This might be 
controlled using KMS instead once adopted.
>>>>> The frame buffer device also looks weird. Right now you only seem
>>>>> to have a single frame buffer registered to a driver in the same
>>>>> module. Is that frame buffer not dependent on a controller?
>>>>>            
>>>> MCDE framebuffers are only depending on MCDE DSS. DSS is the API that
>>>> will be used by all user space APIs so that we don't have to
>>>>          
>>> duplicate
>>>        
>>>> the common code. We are planning mcde_kms and mcde_v4l2 drivers on
>>>>          
>>> top
>>>        
>>>> of MCDE DSS(=Display Sub System).
>>>>          
>>> My impression was that you don't need a frame buffer driver if you have
>>> a kms driver, is this wrong?
>>>        
>> No, see above. Just that we have mcde dss to support multiple user
>> space apis by customer request. Then doing our own fb on top of
>> that is very simple and adds flexibility.
>>      
> This sounds like an odd thing for a customer to ask for ;-)
>
> In my experience customers want to solve specific problems like
> everyone else, they have little interest in adding complexity
> for the sake of it. Is there something wrong with one of the
> interfaces? If so, it would be better to fix that than to add
> an indirection to allow more of them!
>
>    
Ok, different customers use different platforms that have different 
requirements. Read MeeGo vs. Android.
>>> What does the v4l2 driver do? In my simple world, displays are for
>>> output
>>> and v4l is for input, so I must have missed something here.
>>>        
>> Currently nothing, since it is not finished. But the idea (and
>> requirement) is that normal graphics will use framebuffer and
>> video/camera overlays will use v4l2 overlays. Both using same
>> mcde channel and display. Some users might also configure their
>> board to use two framebuffers instead. Or maybe only use KMS etc ...
>>      
> I still don't understand, sorry for being slow. Why does a camera
> use a display?
>    
Sorry, camera _application_ use V4L2 overlays for pushing YUV camera 
preview or video buffers to screen composition in MCDE. V4L2 have output 
devices too, it's not only for capturing, even if that is what most 
desktops use it for.

/Marcus

[PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

On Thursday 16 December 2010 19:26:37 Marcus Lorentzon wrote:
> On 11/26/2010 12:24 PM, Arnd Bergmann wrote:
> > [note: please configure your email client properly so it keeps
> > proper attribution of text and and does not rewrap the citations
> > incorrectly. Wrap your own text after 70 characters]
> >    
> I'm now using Thunderbird, please let me know if it's better than my 
> previous webmail client, neither have many features for reply formatting.

Much better now, just remember to leave empty lines around your replies
and to trim the lines that you are not replying to.

> > * When I talk about a bus, I mean 'struct bus_type', which identifies
> >    all devices with a uniform bus interface to their parent device
> >    (think: PCI, USB, I2C). You seem to think of a bus as a specific
> >    instance of that bus type, i.e. the device that is the parent of
> >    all the connected devices. If you have only one instance of a bus
> >    in any system, and they are all using the same driver, do not add
> >    a bus_type for it.
> >    A good reason to add a bus_type would be e.g. if the "display"
> >    driver uses interfaces to the dss that are common among multiple
> >    dss drivers from different vendors, but the actual display drivers
> >    are identical. This does not seem to be the case.
> >    
> Correct, I refer to the device, not type or driver. I used a bus type 
> since it allowed me to setup a default implementation for each driver 
> callback. So all drivers get generic implementation by default, and 
> override when that is not enough. Meybe you have a better way of getting 
> the same behavior.

One solution that I like is to write a module with the common code as
a library, exporting all the default actions. The specific drivers
can then fill their operations structure by listing the defaults
or by providing their own functions to replace them, which in turn
can call the default functions. This is e.g. what libata does.

> > * When you say that the devices are static, I hope you do not mean
> >    static in the C language sense. We used to allow devices to be
> >    declared as "static struct" and registered using
> >    platform_device_register (or other bus specific functions). This
> >    is no longer valid and we are removing the existing users, do not
> >    add new ones. When creating a platform device, use
> >    platform_device_register_simple or platform_device_register_resndata.
> >
> > I'm not sure what you mean with drivers being static. Predefining
> > the association between displays and drivers in per-machine files is
> > fine, but since this is really board specific, it would be better
> > to eventually do this through data passed from the boot loader, so
> > you don't have to have a machine file for every combination of displays
> > that is in the field.
> >    
> I guess you have read the ARM vs static platform_devices. But, yes, I 
> mean in the c-language static sense. I will adopt to whatever Russel 
> King says is The right way in ARM SoCs.

Fair enough. We will have to fix it some day so Greg can go on with
his plan to disallow static devices, but for now I'm not going to
stop you. I would use platform_device_register_simple anyway, but feel
free to do whatever fits your need here.

> We are now taking a step back and start "all over". We were almost as 
> fresh on this HW block as you are now when we started implementing the 
> driver earlier this year. I think all of us benefit from now having a 
> better understanding of customer requirements and the HW itself, there 
> are some nice quirks ;). Anyway, we will restart the patches and RFC 
> only the MCDE HW part of the driver, implementing basic fb support for 
> one display board as you suggested initially. It's a nice step towards 
> making the patches easier to review and give us some time to prepare the 
> DSS stuff. That remake was done today, so I think the patch will be sent 
> out soon. (I'm going on vacation for 3 weeks btw).

Ok, sounds great! I'm also starting a 3 week vacation, but will be at the
Linaro sprint in Dallas.

My feeling now, after understanding about it some more, is that it would
actually be better to start with a KMS implementation instead of a classic
frame buffer. Ideally you wouldn't even need the frame buffer driver or
the multiplexing between the two then, but still get all the benefits
from the new KMS infrastructure.

> > In the future, best plan for how you want to submit the code while
> > you're writing it, instead of as an afterthought. Quite often, the
> > first patch to submit is also one of the early stages of the driver,
> > so there is no need to wait for the big picture before you start
> > submitting. This way, we can work out conceptual mistakes early on,
> > saving a lot of your time, and the reviewer's time as well.
> >    
> This is how we will try to work now that we know how the HW works.

Ok, cool!

> >> DSS give access to all display devices probed on the virtual mcde
> >> dss bus, or platform bus with specific type of devices if you like.
> >> All calls to DSS operate on a display device, like create an
> >> overlay(=framebuffer), request an update, set power mode, etc.
> >> All calls to DSS related to display itself and not only framebuffer
> >> scanout, will be passed on to the display driver of the display
> >> device in question. All calls DSS only related to overlays, like
> >> buffer pointers, position, rotation etc is handled directly by DSS
> >> calling mcde_hw.
> >>
> >> You could see mcde_hw as a physical level driver and mcde_dss closer
> >> to a logical driver, delegating display specific decisions to the
> >> display driver. Another analogy is mcde_hw is host driver and display
> >> drivers are client device drivers. And DSS is a collection of logic
> >> to manage the interaction between host and client devices.
> >>      
> > The way you describe it, I would picture it differently:
> >
> > +----------+ +----+-----+-----+ +-------+
> > | mcde_hw  | | fb | kms | v4l | | displ |
> > +----+----------------------------------+
> > | HW |            mcde_dss              |
> > +----+----------------------------------+
> >
> > In this model, the dss is the core module that everything else
> > links to. The hw driver talks to the actual hardware and to the
> > dss. The three front-ends only talk to the dss, but not to the
> > individual display drivers or to the hw code directly (i.e. they
> > don't use their exported symbols or internal data structures.
> > The display drivers only talk to the dss, but not to the front-ends
> > or the hw drivers.
> >
> > Would this be a correct representation of your modules?
> >    
> Hmm, mcde_hw does not link to dss. It should be FB->DSS->Display 
> driver->MCDE_HW->HW IO (+ DSS->MCDE_HW). My picture is how code should 
> be used. Anything else you find in code is a violation of that layering.

I don't think it makes any sense to have the DSS sit on top of the
display drivers, since that means it has to know about all of them
and loading the DSS module would implicitly have to load all the
display modules below it, even for the displays that are not present.

Moreover, I don't yet see the reason for the split between mcde_hw and
dss. If dss is the only user of the hardware module (aside from stuff
using dss), and dss is written against the hw module as a low-level
implementation, they can simply be the same module.

> > Can you describe the shortcomings of the KSM code? I've added the dri-devel
> > list to Cc, to get the attention of the right people.
> >    
> I will start this work early next year. MCDE DSS refactoring will take 
> KMS into account. Some of the _possible_ short comings (I must say I 
> have not looked into this in any detail yet):
> - 3D HW is bundled with display HW. Makes it harder for us to use 
> different 3D HW with same display HW or the other way around. I would 
> like KMS and "DRM3D" to be more separated. We get DRM 3D drivers from IP 
> vendors, but we still have to expose our own KMS DRM device.

Ok. I'd have to look into this in more detail myself to see how
severe this is, or how to solve it. The problem seems obvious
enough that you should see no resistance to a patch for this.

> The other "issue" is the usual, 3D vendors don't upstream their drivers.
> Which means we have to integrate with drivers not in mainline kernel ...
> and we still want to open all our drivers, even if some external IPs
> don't.

This will be a lot tougher for you. External modules are generally
not accepted as a reason for designing code one way vs. another.
Whatever the solution is, you have to convince people that it would
still make sense if all drivers were part of the kernel itself.
Bonus points to you if you define it in a way that forces the 3d driver
people to put their code under the GPL in order to work with yours ;-)

> - GEM user space buffer API has a security model and IPC sharing not 
> compatible (at first glance and after short discussion with Chris 
> Wilson) with Android (binder fdup) or for protecting buffers from the 
> user. As I understand it correctly, GEM master, once client 
> authenticated, you have access to all buffers.

I have no idea what this means, but I trust that you and others
can come up with a solution.

> - Partial updates, overlay support and pushing any buffer to scanout. 
> Some might be possible with the latest ioctls in KMS, will look at this.

Remember that with ioctls, you can always add new ones if you need
them, though you cannot remove or change them in incompatible ways.

If you need the ioctl commands to do something they can't do today,
try defining new commands in a way that will also work with future
extensions without making the interface more complex than what you
need to do. It takes some experience to get this right and the first
versions will probably get rejected, but that doesn't mean people
are opposed to extending the interface.

> But as I said, I have not had time to look at this yet. Framebuffer was 
> just so much easier to implement and the only customer requirement.

Yes.

> > Ok. If your frame buffers are not children of the displays, they should
> > however be children of the controller:
> >
> > .../mcde_controller/
> >          /chnlA/
> >                  /displ_crtc0
> >                  /displ_dbi0
> >          /chnlB/
> >                  dspl_crtc1
> >          /fb0
> >          /fb1
> >          /fb2
> >          /v4l_0
> >          /v4l_1
> >
> > Does this fit better?
> >
> >    
> Maybe, will try to find a better structure for relations. Not something 
> I've considered before. But I see your point.
> BTW. Can this hierarchy be changed in runtime? When for example one 
> display move from one channel to another. There's a lot of muxing going 
> on in the HW and that is hard to visualize in a static tree structure. A 
> flat structure might be better then.

It can change at runtime in theory, but that's highly discouraged
because it tends to break user space programs working with the
path names.

Using a flatter structure indeed sounds better in that case,
showing only the displays.

> > What is the relation between a port/connector and a display?
> > If it's 1:1, it should be the same device.
> >
> >    
> A port is product specific display device data. Just a structure used to 
> describe the MCDE<->Display/panel physical connection. The display 
> device resource is you like. Port data describe the SoC-wires-display 
> connection. Where are the display platform device struct describe the on 
> SoC display configuration. Like initial color depth, what MCDE 
> channel/encoder to use etc.

It still sounds to me like it only needs to be one device for
the display then. The device can have properties for the wires and
for the settings, but since it's a one-to-one relationship, I would
represent it as a single object in the device tree.

> >>> * the dss driver gets loaded through udev and matches all the
> >>>    channels
> >>> * the dss driver creates the display devices below each channel,
> >>>    according to the configuration data it got passed.
> >>>        
> >> "All" display devices need static platform_data from
> >> mach-ux500/board-xx.c. This is why we have the bus,
> >> to bind display dev and driver.
> >>      
> > You don't need to instantiate the device from the board though,
> > just provide the data. When you add the display specific data
> > to the dss data, the dss can create the display devices:
> >
> > static struct mcde_display_data mcde_displays[2] = {
> > {
> >          ...
> > }, {
> >          ...
> > },
> > };
> >
> > static struct mcde_dss_data {
> >          int num_displays;
> >          struct mcde_display_data *displays;
> > } my_dss = {
> >          .num_displays = 2,
> >          .displays =&mcde_displays;
> > };
> >
> > The mcde_dss probe function then takes the dss_data and iterates
> > the displays, creating a new child device for each.
> >
> >    
> To me this is exactly the same as the static devices we now have. Same 
> amount of static data. And if you don't register the device, I don't see 
> the difference. I will follow the ARM discussions on c-static platform 
> devices and adopt.

There is a problem in the object life time rules if you instantiate
all the devices at boot time: It means that the devices lower in the
hierarchy can get used before the parent devices are fully initialized.

You can do the main mcde device as a static platform device if you
insist, but registering a hierarchy of static platform devices
is asking for trouble.

> >> Devices are static from mach-ux500/board-xx. And v4l2/fb setup
> >> is board/product specific and could change dynamically.
> >>      
> > Not sure how the fb setup can be both board specific and dynamic.
> > If it's statically defined per board, it should be part of the
> > dss data, and dss can then create the fb devices. If it's completely
> > dynamic, it gets created through user space interaction anyway.
> >
> >    
> The default is setup dynamically by static calls in board init code. 
> User space will then be able to change this config. This is one of the 
> features that is not heavily used and might get removed. Like Multihead 
> framebuffers or framebuffer cloning to multiple displays. This might be 
> controlled using KMS instead once adopted.

Ok, makes sense.

> >> No, see above. Just that we have mcde dss to support multiple user
> >> space apis by customer request. Then doing our own fb on top of
> >> that is very simple and adds flexibility.
> >>      
> > This sounds like an odd thing for a customer to ask for ;-)
> >
> > In my experience customers want to solve specific problems like
> > everyone else, they have little interest in adding complexity
> > for the sake of it. Is there something wrong with one of the
> > interfaces? If so, it would be better to fix that than to add
> > an indirection to allow more of them!
> >
> >    
> Ok, different customers use different platforms that have different 
> requirements. Read MeeGo vs. Android.

I see. This needs to be solved more generally though, since everyone
has the same requirements. If we conclude that we should do everything
with KMS infrastructure, we should also make sure that it works
for all the relevant users. That might be something worth discussing
in the Linaro graphics workgroup as well.

> >>> What does the v4l2 driver do? In my simple world, displays are for
> >>> output
> >>> and v4l is for input, so I must have missed something here.
> >>>        
> >> Currently nothing, since it is not finished. But the idea (and
> >> requirement) is that normal graphics will use framebuffer and
> >> video/camera overlays will use v4l2 overlays. Both using same
> >> mcde channel and display. Some users might also configure their
> >> board to use two framebuffers instead. Or maybe only use KMS etc ...
> >>      
> > I still don't understand, sorry for being slow. Why does a camera
> > use a display?
> >    
> Sorry, camera _application_ use V4L2 overlays for pushing YUV camera 
> preview or video buffers to screen composition in MCDE. V4L2 have output 
> devices too, it's not only for capturing, even if that is what most 
> desktops use it for.

Ok, I'm starting to remember this from the 90's when I used bttv on the
console framebuffer ;-).

Could you simply define a v4l overlay device for every display device,
even if you might not want to use it?
That might simplify the setup considerably.

	Arnd

[PATCH 09/10] MCDE: Add build files and bus

[Marcus Lorentzon]

On 12/17/2010 12:22 PM, Arnd Bergmann wrote:
>>> * When I talk about a bus, I mean 'struct bus_type', which identifies
>>>     all devices with a uniform bus interface to their parent device
>>>     (think: PCI, USB, I2C). You seem to think of a bus as a specific
>>>     instance of that bus type, i.e. the device that is the parent of
>>>     all the connected devices. If you have only one instance of a bus
>>>     in any system, and they are all using the same driver, do not add
>>>     a bus_type for it.
>>>     A good reason to add a bus_type would be e.g. if the "display"
>>>     driver uses interfaces to the dss that are common among multiple
>>>     dss drivers from different vendors, but the actual display drivers
>>>     are identical. This does not seem to be the case.
>>>
>>>        
>> Correct, I refer to the device, not type or driver. I used a bus type
>> since it allowed me to setup a default implementation for each driver
>> callback. So all drivers get generic implementation by default, and
>> override when that is not enough. Meybe you have a better way of getting
>> the same behavior.
>>      
> One solution that I like is to write a module with the common code as
> a library, exporting all the default actions. The specific drivers
> can then fill their operations structure by listing the defaults
> or by providing their own functions to replace them, which in turn
> can call the default functions. This is e.g. what libata does.
>
>    
Will do.
>
>> We are now taking a step back and start "all over". We were almost as
>> fresh on this HW block as you are now when we started implementing the
>> driver earlier this year. I think all of us benefit from now having a
>> better understanding of customer requirements and the HW itself, there
>> are some nice quirks ;). Anyway, we will restart the patches and RFC
>> only the MCDE HW part of the driver, implementing basic fb support for
>> one display board as you suggested initially. It's a nice step towards
>> making the patches easier to review and give us some time to prepare the
>> DSS stuff. That remake was done today, so I think the patch will be sent
>> out soon. (I'm going on vacation for 3 weeks btw).
>>      
> Ok, sounds great! I'm also starting a 3 week vacation, but will be at the
> Linaro sprint in Dallas.
>
> My feeling now, after understanding about it some more, is that it would
> actually be better to start with a KMS implementation instead of a classic
> frame buffer. Ideally you wouldn't even need the frame buffer driver or
> the multiplexing between the two then, but still get all the benefits
> from the new KMS infrastructure.
>
>    
I will look at it, we might still post a fb->mcde_hw first though, since 
it's so little work.
>
>>>> DSS give access to all display devices probed on the virtual mcde
>>>> dss bus, or platform bus with specific type of devices if you like.
>>>> All calls to DSS operate on a display device, like create an
>>>> overlay(=framebuffer), request an update, set power mode, etc.
>>>> All calls to DSS related to display itself and not only framebuffer
>>>> scanout, will be passed on to the display driver of the display
>>>> device in question. All calls DSS only related to overlays, like
>>>> buffer pointers, position, rotation etc is handled directly by DSS
>>>> calling mcde_hw.
>>>>
>>>> You could see mcde_hw as a physical level driver and mcde_dss closer
>>>> to a logical driver, delegating display specific decisions to the
>>>> display driver. Another analogy is mcde_hw is host driver and display
>>>> drivers are client device drivers. And DSS is a collection of logic
>>>> to manage the interaction between host and client devices.
>>>>
>>>>          
>>> The way you describe it, I would picture it differently:
>>>
>>> +----------+ +----+-----+-----+ +-------+
>>> | mcde_hw  | | fb | kms | v4l | | displ |
>>> +----+----------------------------------+
>>> | HW |            mcde_dss              |
>>> +----+----------------------------------+
>>>
>>> In this model, the dss is the core module that everything else
>>> links to. The hw driver talks to the actual hardware and to the
>>> dss. The three front-ends only talk to the dss, but not to the
>>> individual display drivers or to the hw code directly (i.e. they
>>> don't use their exported symbols or internal data structures.
>>> The display drivers only talk to the dss, but not to the front-ends
>>> or the hw drivers.
>>>
>>> Would this be a correct representation of your modules?
>>>
>>>        
>> Hmm, mcde_hw does not link to dss. It should be FB->DSS->Display
>> driver->MCDE_HW->HW IO (+ DSS->MCDE_HW). My picture is how code should
>> be used. Anything else you find in code is a violation of that layering.
>>      
> I don't think it makes any sense to have the DSS sit on top of the
> display drivers, since that means it has to know about all of them
> and loading the DSS module would implicitly have to load all the
> display modules below it, even for the displays that are not present.
>
>    
DSS does not have a static dependency on display drivers. DSS is just a 
"convenience library" for handling the correct display driver call 
sequences, instead of each user (fbdev/KMS/V4L2) having to duplicate 
this code.

> Moreover, I don't yet see the reason for the split between mcde_hw and
> dss. If dss is the only user of the hardware module (aside from stuff
> using dss), and dss is written against the hw module as a low-level
> implementation, they can simply be the same module.
>
>    
They are the same module, just split into two files.
>
>> The other "issue" is the usual, 3D vendors don't upstream their drivers.
>> Which means we have to integrate with drivers not in mainline kernel ...
>> and we still want to open all our drivers, even if some external IPs
>> don't.
>>      
> This will be a lot tougher for you. External modules are generally
> not accepted as a reason for designing code one way vs. another.
> Whatever the solution is, you have to convince people that it would
> still make sense if all drivers were part of the kernel itself.
> Bonus points to you if you define it in a way that forces the 3d driver
> people to put their code under the GPL in order to work with yours ;-)
>
>    
I see this as a side effect of DRM putting a dependency between 3D HW 
and KMS HW driver. In most embedded systems, these two are no more 
coupled than any other HW block on the SoC. So by "fixing" this 
_possible_ flaw. I see no reason why a KMS driver can't stand on it's 
own. There's no reason not to support display in the kernel just because 
there's no 3D HW driver, right?
>
>>>>> What does the v4l2 driver do? In my simple world, displays are for
>>>>> output
>>>>> and v4l is for input, so I must have missed something here.
>>>>>
>>>>>            
>>>> Currently nothing, since it is not finished. But the idea (and
>>>> requirement) is that normal graphics will use framebuffer and
>>>> video/camera overlays will use v4l2 overlays. Both using same
>>>> mcde channel and display. Some users might also configure their
>>>> board to use two framebuffers instead. Or maybe only use KMS etc ...
>>>>
>>>>          
>>> I still don't understand, sorry for being slow. Why does a camera
>>> use a display?
>>>
>>>        
>> Sorry, camera _application_ use V4L2 overlays for pushing YUV camera
>> preview or video buffers to screen composition in MCDE. V4L2 have output
>> devices too, it's not only for capturing, even if that is what most
>> desktops use it for.
>>      
> Ok, I'm starting to remember this from the 90's when I used bttv on the
> console framebuffer ;-).
>
> Could you simply define a v4l overlay device for every display device,
> even if you might not want to use it?
> That might simplify the setup considerably.
>    
Sure, but that is currently up to board init code. Just as for frame 
buffers, mcde_fb_create(display, ...), we will have a 
"createV4L2device(display, ...)".

/BR
/Marcus

[PATCH 09/10] MCDE: Add build files and bus

[Rob Clark]

On Sun, Dec 5, 2010 at 5:28 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Dec 04, 2010 at 04:34:22PM -0500, Alex Deucher wrote:
>> This doesn't seem that different from the graphics chips we support
>> with kms. ?I don't think it would require much work to use KMS. ?One
>> thing we considered, but never ended up implementing was a generic
>> overlay API for KMS. ?Most PC hardware still has overlays, but we
>> don't use them much any more on the desktop side. ?It may be
>> worthwhile to design an appropriate API for them for these type of
>> hardware.
>
> Just fyi about a generic overlay api: I've looked a bit into this when
> doing the intel overlay support and I think adding special overlay crtcs
> that can be attached real crtcs gives a nice clean api. We could the reuse
> the existing framebuffer/pageflipping api to get the buffers to the
> overlay engine.

btw, has there been any further thought/discussion on this topic..
I've been experimenting with a drm driver interface on the OMAP SoC.
It is working well now for framebuffer type usage (mode setting,
virtual framebuffer spanning multiple diplays, and those types of
xrandr things)..  the next step that I've started thinking about is
overlay (or underlay.. the z-order is flexible) support..

I was thinking in a similar direction (ie. a special, or maybe not so
special, sort of crtc) and came across this thread, so I thought I'd
resurrect the topic.

In our case, most of the CRTCs in our driver could be used either with
(A)RGB buffers as a traditional framebuffer, or with a few different
formats of YUV as video under/overlays.  So if you had one display
attached, you might only use one CRTC for traditional GUI/gfx layer,
and the rest are available for video.  If you had two displays, then
you'd steal one of the video CRTCs and use it for the gfx layer on the
second display.  And so on.

Rough thinking:
+ add some 'caps' to the CRTC to indicate whether it can handle YUV,
ARGB, scaling
+ add an x/y offset relative to the encoder (as opposed to the
existing x/y offset relative to the framebuffer)
+ add a z-order parameter

Not sure about intel hw if it is supporting clip-rects.. if so, maybe
need to add something about that.  In our case we jut put the video
behind the gfx layer and use the alpha channel in the gfx framebuffer
to clip/blend rather than using clip-rects.


> The real pain starts when we want format discovery from userspace with all
> the alignement/size/layout constrains. Add in tiling support and its
> almost impossible to do in a generic way. But also for kms userspace needs
> to know these constrains (implemented for generic use in libkms). I favor
> such an approach for overlays, too (if it ever happens) - i.e. a few
> helpers in libkms that allocate an appropriate buffer for a given format
> and size and returns the buffer, strides and offsets for the different
> planes.

hmm, I guess I know about the OMAP display subsystem, and it's overlay
formats/capabilities.. but not enough about other hw to say anything
intelligent here.  But I guess even if we ignore the format of the
data in the buffer, at least the APIs to setup/attach overlay CRTCs at
various positions could maybe be something we can start with as a
first step.  At least standardizing this part seems like a good first
step.  But I'm definitely interested if someone has some ideas.

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH 09/10] MCDE: Add build files and bus

[Marcus Lorentzon]

On 03/12/2011 04:59 PM, Rob Clark wrote:
> On Sun, Dec 5, 2010 at 5:28 AM, Daniel Vetter<daniel@ffwll.ch>  wrote:
>    
>> On Sat, Dec 04, 2010 at 04:34:22PM -0500, Alex Deucher wrote:
>>      
>>> This doesn't seem that different from the graphics chips we support
>>> with kms.  I don't think it would require much work to use KMS.  One
>>> thing we considered, but never ended up implementing was a generic
>>> overlay API for KMS.  Most PC hardware still has overlays, but we
>>> don't use them much any more on the desktop side.  It may be
>>> worthwhile to design an appropriate API for them for these type of
>>> hardware.
>>>        
>> Just fyi about a generic overlay api: I've looked a bit into this when
>> doing the intel overlay support and I think adding special overlay crtcs
>> that can be attached real crtcs gives a nice clean api. We could the reuse
>> the existing framebuffer/pageflipping api to get the buffers to the
>> overlay engine.
>>      
> btw, has there been any further thought/discussion on this topic..
> I've been experimenting with a drm driver interface on the OMAP SoC.
> It is working well now for framebuffer type usage (mode setting,
> virtual framebuffer spanning multiple diplays, and those types of
> xrandr things)..  the next step that I've started thinking about is
> overlay (or underlay.. the z-order is flexible) support..
>
> I was thinking in a similar direction (ie. a special, or maybe not so
> special, sort of crtc) and came across this thread, so I thought I'd
> resurrect the topic.
>
> In our case, most of the CRTCs in our driver could be used either with
> (A)RGB buffers as a traditional framebuffer, or with a few different
> formats of YUV as video under/overlays.  So if you had one display
> attached, you might only use one CRTC for traditional GUI/gfx layer,
> and the rest are available for video.  If you had two displays, then
> you'd steal one of the video CRTCs and use it for the gfx layer on the
> second display.  And so on.
>
>    
We have similar HW and are also interested in finding some common ground 
for overlays in KMS. Just as you describe, we have no hard connection 
between a CRTC and output. Instead we only have overlays. Normal gfx use 
case is then of course one of these overlays dedicated to one display. 
And when adding video overlays, we also prefer YUV "underlays" with 
fullscreen ARGB gfx on top.

The problem with mapping this to the CRTCs in KMS today, is that there 
is no differentiation between framebuffer width/height and crt 
width/height. And of course YUV formats and fb position etc are missing.

One advantage of the set CRTC ioctl is that all information needed to 
switch mode is contained in one atomic set mode ioctl. So we have to 
think about if we want a new more advanced set crtc including overlay 
config. Or if we want to split mode setup into several requests. And 
then we must decide if multiple setup ioctls will need some type of 
"commit" to get the atomic mode switch we have today. For example I 
don't want to have to do a set_crtc enabling blending without overlay 
being setup. It should be just as glitch free as KMS is today.
> Rough thinking:
> + add some 'caps' to the CRTC to indicate whether it can handle YUV,
> ARGB, scaling
> + add an x/y offset relative to the encoder (as opposed to the
> existing x/y offset relative to the framebuffer)
> + add a z-order parameter
>
>    
Exactly what I would like to have. Especially the caps for scaling, 
since we have one HW that can't do scaling.
> Not sure about intel hw if it is supporting clip-rects.. if so, maybe
> need to add something about that.  In our case we jut put the video
> behind the gfx layer and use the alpha channel in the gfx framebuffer
> to clip/blend rather than using clip-rects.
>
>    
If this is common ground, I would like to have one clip rect per 
CRTC/overlay to enable framebuffers larger than overlay viewport. That 
makes it easier to reuse a large buffer for multiple 
overlays/framebuffers without having to stress memory management driver. 
But this is just a "nice to have" feature. Maybe this can be mapped to 
stride/start address mappings on HW without clip rect. But that will 
probably include alignment requirements on position and size.
>> The real pain starts when we want format discovery from userspace with all
>> the alignement/size/layout constrains. Add in tiling support and its
>> almost impossible to do in a generic way. But also for kms userspace needs
>> to know these constrains (implemented for generic use in libkms). I favor
>> such an approach for overlays, too (if it ever happens) - i.e. a few
>> helpers in libkms that allocate an appropriate buffer for a given format
>> and size and returns the buffer, strides and offsets for the different
>> planes.
>>      
> hmm, I guess I know about the OMAP display subsystem, and it's overlay
> formats/capabilities.. but not enough about other hw to say anything
> intelligent here.  But I guess even if we ignore the format of the
> data in the buffer, at least the APIs to setup/attach overlay CRTCs at
> various positions could maybe be something we can start with as a
> first step.  At least standardizing this part seems like a good first
> step.  But I'm definitely interested if someone has some ideas.
>
>    

Yes, so we could try and find some common ground and add support for 
that. But still enable drivers to extend that with the features where we 
find no common ground. Just as GEM doesn't provide allocation ioctl, 
only free.

And in the end we have to see if the common ground is enough to actually 
build an application on. If not, there's not much use for a partial 
common API. Maybe that's why there's no overlay API in KMS tiday?

Maybe vendor libkms can be used to fill in the gaps?

/BR
/Marcus

[PATCH 09/10] MCDE: Add build files and bus

[Rob Clark]

On Mon, Mar 14, 2011 at 9:03 AM, Marcus Lorentzon
<marcus.xm.lorentzon@stericsson.com> wrote:
> On 03/12/2011 04:59 PM, Rob Clark wrote:
>>
>> On Sun, Dec 5, 2010 at 5:28 AM, Daniel Vetter<daniel@ffwll.ch> ?wrote:
>>
>>>
>>> On Sat, Dec 04, 2010 at 04:34:22PM -0500, Alex Deucher wrote:
>>>
>>>>
>>>> This doesn't seem that different from the graphics chips we support
>>>> with kms. ?I don't think it would require much work to use KMS. ?One
>>>> thing we considered, but never ended up implementing was a generic
>>>> overlay API for KMS. ?Most PC hardware still has overlays, but we
>>>> don't use them much any more on the desktop side. ?It may be
>>>> worthwhile to design an appropriate API for them for these type of
>>>> hardware.
>>>>
>>>
>>> Just fyi about a generic overlay api: I've looked a bit into this when
>>> doing the intel overlay support and I think adding special overlay crtcs
>>> that can be attached real crtcs gives a nice clean api. We could the
>>> reuse
>>> the existing framebuffer/pageflipping api to get the buffers to the
>>> overlay engine.
>>>
>>
>> btw, has there been any further thought/discussion on this topic..
>> I've been experimenting with a drm driver interface on the OMAP SoC.
>> It is working well now for framebuffer type usage (mode setting,
>> virtual framebuffer spanning multiple diplays, and those types of
>> xrandr things).. ?the next step that I've started thinking about is
>> overlay (or underlay.. the z-order is flexible) support..
>>
>> I was thinking in a similar direction (ie. a special, or maybe not so
>> special, sort of crtc) and came across this thread, so I thought I'd
>> resurrect the topic.
>>
>> In our case, most of the CRTCs in our driver could be used either with
>> (A)RGB buffers as a traditional framebuffer, or with a few different
>> formats of YUV as video under/overlays. ?So if you had one display
>> attached, you might only use one CRTC for traditional GUI/gfx layer,
>> and the rest are available for video. ?If you had two displays, then
>> you'd steal one of the video CRTCs and use it for the gfx layer on the
>> second display. ?And so on.
>>
>>
>
> We have similar HW and are also interested in finding some common ground for
> overlays in KMS. Just as you describe, we have no hard connection between a
> CRTC and output. Instead we only have overlays. Normal gfx use case is then
> of course one of these overlays dedicated to one display. And when adding
> video overlays, we also prefer YUV "underlays" with fullscreen ARGB gfx on
> top.
>
> The problem with mapping this to the CRTCs in KMS today, is that there is no
> differentiation between framebuffer width/height and crt width/height. And
> of course YUV formats and fb position etc are missing.
>
> One advantage of the set CRTC ioctl is that all information needed to switch
> mode is contained in one atomic set mode ioctl. So we have to think about if
> we want a new more advanced set crtc including overlay config. Or if we want
> to split mode setup into several requests. And then we must decide if
> multiple setup ioctls will need some type of "commit" to get the atomic mode
> switch we have today. For example I don't want to have to do a set_crtc
> enabling blending without overlay being setup. It should be just as glitch
> free as KMS is today.
>>
>> Rough thinking:
>> + add some 'caps' to the CRTC to indicate whether it can handle YUV,
>> ARGB, scaling
>> + add an x/y offset relative to the encoder (as opposed to the
>> existing x/y offset relative to the framebuffer)
>> + add a z-order parameter
>>
>>
>
> Exactly what I would like to have. Especially the caps for scaling, since we
> have one HW that can't do scaling.
>>
>> Not sure about intel hw if it is supporting clip-rects.. if so, maybe
>> need to add something about that. ?In our case we jut put the video
>> behind the gfx layer and use the alpha channel in the gfx framebuffer
>> to clip/blend rather than using clip-rects.
>>
>>
>
> If this is common ground, I would like to have one clip rect per
> CRTC/overlay to enable framebuffers larger than overlay viewport. That makes
> it easier to reuse a large buffer for multiple overlays/framebuffers without
> having to stress memory management driver. But this is just a "nice to have"
> feature. Maybe this can be mapped to stride/start address mappings on HW
> without clip rect. But that will probably include alignment requirements on
> position and size.


Good point, I had overlooked that but we do have same requirement for
cropping as well.. although in the crtc we already specify an x/y
offset within the drm_framebuffer that the crtc is attached to.. so I
guess if we have an input width/height (output is implied I guess by
the encoder/connector) then we should be fine for cropping

Although in some cases top/left crop offset could be changing frame by
frame (think use cases like zero-copy video stabilization or pan/scan)
so might be nice to have a way to specify new x/y offset when
flipping.  I guess that would be an extension/change to existing page
flip ioctl.

BR,
-R

>>>
>>> The real pain starts when we want format discovery from userspace with
>>> all
>>> the alignement/size/layout constrains. Add in tiling support and its
>>> almost impossible to do in a generic way. But also for kms userspace
>>> needs
>>> to know these constrains (implemented for generic use in libkms). I favor
>>> such an approach for overlays, too (if it ever happens) - i.e. a few
>>> helpers in libkms that allocate an appropriate buffer for a given format
>>> and size and returns the buffer, strides and offsets for the different
>>> planes.
>>>
>>
>> hmm, I guess I know about the OMAP display subsystem, and it's overlay
>> formats/capabilities.. but not enough about other hw to say anything
>> intelligent here. ?But I guess even if we ignore the format of the
>> data in the buffer, at least the APIs to setup/attach overlay CRTCs at
>> various positions could maybe be something we can start with as a
>> first step. ?At least standardizing this part seems like a good first
>> step. ?But I'm definitely interested if someone has some ideas.
>>
>>
>
> Yes, so we could try and find some common ground and add support for that.
> But still enable drivers to extend that with the features where we find no
> common ground. Just as GEM doesn't provide allocation ioctl, only free.
>
> And in the end we have to see if the common ground is enough to actually
> build an application on. If not, there's not much use for a partial common
> API. Maybe that's why there's no overlay API in KMS tiday?
>
> Maybe vendor libkms can be used to fill in the gaps?
>
> /BR
> /Marcus
>
>

[PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

On Wednesday 10 November 2010, Jimmy Rubin wrote:
> This patch adds support for the MCDE, Memory-to-display controller,
> found in the ST-Ericsson ux500 products.
> 
> This patch adds the necessary build files for MCDE and the bus that
> all displays are connected to.
> 

Can you explain why you need a bus for this?

With the code you currently have, there is only a single driver associated
with this bus type, and also just a single device that gets registered here!

>+static int __init mcde_subsystem_init(void)
>+{
>+       int ret;
>+       pr_info("MCDE subsystem init begin\n");
>+
>+       /* MCDE module init sequence */
>+       ret = mcde_init();
>+       if (ret)
>+               return ret;
>+       ret = mcde_display_init();
>+       if (ret)
>+               goto mcde_display_failed;
>+       ret = mcde_dss_init();
>+       if (ret)
>+               goto mcde_dss_failed;
>+       ret = mcde_fb_init();
>+       if (ret)
>+               goto mcde_fb_failed;
>+       pr_info("MCDE subsystem init done\n");
>+
>+       return 0;
>+mcde_fb_failed:
>+       mcde_dss_exit();
>+mcde_dss_failed:
>+       mcde_display_exit();
>+mcde_display_failed:
>+       mcde_exit();
>+       return ret;
>+}

Splitting up the module into four sub-modules and then initializing
everything from one place indicates that something is done wrong
on a global scale.

If you indeed need a bus, that should be a separate module that gets
loaded first and then has the other modules build on top of.

I'm not sure how the other parts layer on top of one another, can you
provide some more insight?

>From what I understood so far, you have a single multi-channel display
controller (mcde_hw.c) that drives the hardware.
Each controller can have multiple frame buffers attached to it, which
in turn can have multiple displays attached to each of them, but your
current configuration only has one of each, right?

Right now you have a single top-level bus device for the displays,
maybe that can be integrated into the controller so the displays are
properly rooted below the hardware that drives them.

The frame buffer device also looks weird. Right now you only seem
to have a single frame buffer registered to a driver in the same
module. Is that frame buffer not dependent on a controller?

>+#ifdef MODULE
>+module_init(mcde_subsystem_init);
>+#else
>+fs_initcall(mcde_subsystem_init);
>+#endif

This is not a file system ;-)

	Arnd

[PATCH 09/10] MCDE: Add build files and bus

[Jimmy Rubin]

This patch adds support for the MCDE, Memory-to-display controller,
found in the ST-Ericsson ux500 products.

This patch adds the necessary build files for MCDE and the bus that
all displays are connected to.

Signed-off-by: Jimmy Rubin <jimmy.rubin@stericsson.com>
Acked-by: Linus Walleij <linus.walleij.stericsson.com>
---
 drivers/video/Kconfig         |    2 +
 drivers/video/Makefile        |    1 +
 drivers/video/mcde/Kconfig    |   39 ++++++
 drivers/video/mcde/Makefile   |   12 ++
 drivers/video/mcde/mcde_bus.c |  259 +++++++++++++++++++++++++++++++++++++++++
 drivers/video/mcde/mcde_mod.c |   67 +++++++++++
 6 files changed, 380 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/mcde/Kconfig
 create mode 100644 drivers/video/mcde/Makefile
 create mode 100644 drivers/video/mcde/mcde_bus.c
 create mode 100644 drivers/video/mcde/mcde_mod.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 935cdc2..04aecf4 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2260,6 +2260,8 @@ config FB_JZ4740
 source "drivers/video/omap/Kconfig"
 source "drivers/video/omap2/Kconfig"
 
+source "drivers/video/mcde/Kconfig"
+
 source "drivers/video/backlight/Kconfig"
 source "drivers/video/display/Kconfig"
 
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 485e8ed..325cdcc 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -128,6 +128,7 @@ obj-$(CONFIG_FB_SH_MOBILE_HDMI)	  += sh_mobile_hdmi.o
 obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
 obj-$(CONFIG_FB_OMAP)             += omap/
 obj-y                             += omap2/
+obj-$(CONFIG_FB_MCDE)             += mcde/
 obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
 obj-$(CONFIG_FB_CARMINE)          += carminefb.o
 obj-$(CONFIG_FB_MB862XX)	  += mb862xx/
diff --git a/drivers/video/mcde/Kconfig b/drivers/video/mcde/Kconfig
new file mode 100644
index 0000000..5dab37b
--- /dev/null
+++ b/drivers/video/mcde/Kconfig
@@ -0,0 +1,39 @@
+config FB_MCDE
+	tristate "MCDE support"
+	depends on FB
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	select FB_SYS_FOPS
+	---help---
+	  This enables support for MCDE based frame buffer driver.
+
+	  Please read the file <file:Documentation/fb/mcde.txt>
+
+config MCDE_DISPLAY_GENERIC_DSI
+	tristate "Generic display driver"
+	depends on FB_MCDE
+
+config FB_MCDE_DEBUG
+	bool "MCDE debug messages"
+	depends on FB_MCDE
+	---help---
+	  Say Y here if you want the MCDE driver to output debug messages
+
+config FB_MCDE_VDEBUG
+	bool "MCDE verbose debug messages"
+	depends on FB_MCDE_DEBUG
+	---help---
+	  Say Y here if you want the MCDE driver to output more debug messages
+
+config MCDE_FB_AVOID_REALLOC
+	bool "MCDE early allocate framebuffer"
+	default n
+	depends on FB_MCDE
+	---help---
+	  If you say Y here maximum frame buffer size is allocated and
+	  used for all resolutions. If you say N here, the frame buffer is
+	  reallocated when resolution is changed. This reallocation might
+	  fail because of fragmented memory. Note that this memory will
+	  never be deallocated, while the MCDE framebuffer is used.
+
diff --git a/drivers/video/mcde/Makefile b/drivers/video/mcde/Makefile
new file mode 100644
index 0000000..f90979a
--- /dev/null
+++ b/drivers/video/mcde/Makefile
@@ -0,0 +1,12 @@
+
+mcde-objs			:= mcde_mod.o mcde_hw.o mcde_dss.o mcde_display.o mcde_bus.o mcde_fb.o
+obj-$(CONFIG_FB_MCDE)		+= mcde.o
+
+obj-$(CONFIG_MCDE_DISPLAY_GENERIC_DSI)	+= display-generic_dsi.o
+
+ifdef CONFIG_FB_MCDE_DEBUG
+EXTRA_CFLAGS += -DDEBUG
+endif
+ifdef CONFIG_FB_MCDE_VDEBUG
+EXTRA_CFLAGS += -DVERBOSE_DEBUG
+endif
diff --git a/drivers/video/mcde/mcde_bus.c b/drivers/video/mcde/mcde_bus.c
new file mode 100644
index 0000000..bc1f048
--- /dev/null
+++ b/drivers/video/mcde/mcde_bus.c
@@ -0,0 +1,259 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * ST-Ericsson MCDE display bus driver
+ *
+ * Author: Marcus Lorentzon <marcus.xm.lorentzon@stericsson.com>
+ * for ST-Ericsson.
+ *
+ * License terms: GNU General Public License (GPL), version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/notifier.h>
+
+#include <video/mcde/mcde_display.h>
+#include <video/mcde/mcde_dss.h>
+
+#define to_mcde_display_driver(__drv) \
+	container_of((__drv), struct mcde_display_driver, driver)
+
+static BLOCKING_NOTIFIER_HEAD(bus_notifier_list);
+
+static int mcde_drv_suspend(struct device *_dev, pm_message_t state);
+static int mcde_drv_resume(struct device *_dev);
+struct bus_type mcde_bus_type;
+
+static int mcde_suspend_device(struct device *dev, void *data)
+{
+	pm_message_t* state = (pm_message_t *) data;
+	if (dev->driver->suspend)
+		return dev->driver->suspend(dev, *state);
+	return 0;
+}
+
+static int mcde_resume_device(struct device *dev, void *data)
+{
+	if (dev->driver->resume)
+		return dev->driver->resume(dev);
+	return 0;
+}
+
+/* Bus driver */
+
+static int mcde_bus_match(struct device *_dev, struct device_driver *driver)
+{
+	pr_debug("Matching device %s with driver %s\n",
+		dev_name(_dev), driver->name);
+
+	return strncmp(dev_name(_dev), driver->name, strlen(driver->name)) == 0;
+}
+
+static int mcde_bus_suspend(struct device *_dev, pm_message_t state)
+{
+	int ret;
+	ret = bus_for_each_dev(&mcde_bus_type, NULL, &state,
+				mcde_suspend_device);
+	if (ret) {
+		/* TODO Resume all suspended devices */
+		/* mcde_bus_resume(dev); */
+		return ret;
+	}
+	return 0;
+}
+
+static int mcde_bus_resume(struct device *_dev)
+{
+	return bus_for_each_dev(&mcde_bus_type, NULL, NULL, mcde_resume_device);
+}
+
+struct bus_type mcde_bus_type = {
+	.name = "mcde_bus",
+	.match = mcde_bus_match,
+	.suspend = mcde_bus_suspend,
+	.resume = mcde_bus_resume,
+};
+
+static int mcde_drv_probe(struct device *_dev)
+{
+	struct mcde_display_driver *drv = to_mcde_display_driver(_dev->driver);
+	struct mcde_display_device *dev = to_mcde_display_device(_dev);
+
+	return drv->probe(dev);
+}
+
+static int mcde_drv_remove(struct device *_dev)
+{
+	struct mcde_display_driver *drv = to_mcde_display_driver(_dev->driver);
+	struct mcde_display_device *dev = to_mcde_display_device(_dev);
+
+	return drv->remove(dev);
+}
+
+static void mcde_drv_shutdown(struct device *_dev)
+{
+	struct mcde_display_driver *drv = to_mcde_display_driver(_dev->driver);
+	struct mcde_display_device *dev = to_mcde_display_device(_dev);
+
+	drv->shutdown(dev);
+}
+
+static int mcde_drv_suspend(struct device *_dev, pm_message_t state)
+{
+	struct mcde_display_driver *drv = to_mcde_display_driver(_dev->driver);
+	struct mcde_display_device *dev = to_mcde_display_device(_dev);
+
+	return drv->suspend(dev, state);
+}
+
+static int mcde_drv_resume(struct device *_dev)
+{
+	struct mcde_display_driver *drv = to_mcde_display_driver(_dev->driver);
+	struct mcde_display_device *dev = to_mcde_display_device(_dev);
+
+	return drv->resume(dev);
+}
+
+/* Bus device */
+
+static void mcde_bus_release(struct device *dev)
+{
+}
+
+struct device mcde_bus = {
+	.init_name = "mcde_bus",
+	.release  = mcde_bus_release
+};
+
+/* Public bus API */
+
+int mcde_display_driver_register(struct mcde_display_driver *drv)
+{
+	drv->driver.bus = &mcde_bus_type;
+	if (drv->probe)
+		drv->driver.probe = mcde_drv_probe;
+	if (drv->remove)
+		drv->driver.remove = mcde_drv_remove;
+	if (drv->shutdown)
+		drv->driver.shutdown = mcde_drv_shutdown;
+	if (drv->suspend)
+		drv->driver.suspend = mcde_drv_suspend;
+	if (drv->resume)
+		drv->driver.resume = mcde_drv_resume;
+
+	return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL(mcde_display_driver_register);
+
+void mcde_display_driver_unregister(struct mcde_display_driver *drv)
+{
+	driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL(mcde_display_driver_unregister);
+
+static void mcde_display_dev_release(struct device *dev)
+{
+	/* Do nothing */
+}
+
+int mcde_display_device_register(struct mcde_display_device *dev)
+{
+	/* Setup device */
+	if (!dev)
+		return -EINVAL;
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	dev->dev.bus = &mcde_bus_type;
+	if (dev->dev.parent != NULL)
+		dev->dev.parent = &mcde_bus;
+	dev->dev.release = mcde_display_dev_release;
+	if (dev->id != -1)
+		dev_set_name(&dev->dev, "%s.%d", dev->name,  dev->id);
+	else
+		dev_set_name(&dev->dev, dev->name);
+
+	mcde_display_init_device(dev);
+
+	return device_register(&dev->dev);
+}
+EXPORT_SYMBOL(mcde_display_device_register);
+
+void mcde_display_device_unregister(struct mcde_display_device *dev)
+{
+	device_unregister(&dev->dev);
+}
+EXPORT_SYMBOL(mcde_display_device_unregister);
+
+/* Notifications */
+int mcde_dss_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&bus_notifier_list, nb);
+}
+EXPORT_SYMBOL(mcde_dss_register_notifier);
+
+int mcde_dss_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&bus_notifier_list, nb);
+}
+EXPORT_SYMBOL(mcde_dss_unregister_notifier);
+
+static int bus_notify_callback(struct notifier_block *nb,
+	unsigned long event, void *dev)
+{
+	struct mcde_display_device *ddev = to_mcde_display_device(dev);
+
+	if (event == BUS_NOTIFY_BOUND_DRIVER) {
+		ddev->initialized = true;
+		blocking_notifier_call_chain(&bus_notifier_list,
+			MCDE_DSS_EVENT_DISPLAY_REGISTERED, ddev);
+	} else if (event == BUS_NOTIFY_UNBIND_DRIVER) {
+		ddev->initialized = false;
+		blocking_notifier_call_chain(&bus_notifier_list,
+			MCDE_DSS_EVENT_DISPLAY_UNREGISTERED, ddev);
+	}
+	return 0;
+}
+
+struct notifier_block bus_nb = {
+	.notifier_call = bus_notify_callback,
+};
+
+/* Driver init/exit */
+
+int __init mcde_display_init(void)
+{
+	int ret;
+
+	ret = bus_register(&mcde_bus_type);
+	if (ret) {
+		pr_warning("Unable to register bus type\n");
+		return ret;
+	}
+	ret = device_register(&mcde_bus);
+	if (ret) {
+		pr_warning("Unable to register bus device\n");
+		goto no_device_registration;
+	}
+	ret = bus_register_notifier(&mcde_bus_type, &bus_nb);
+	if (ret) {
+		pr_warning("Unable to register bus notifier\n");
+		goto no_bus_notifier;
+	}
+
+	return 0;
+
+no_bus_notifier:
+	device_unregister(&mcde_bus);
+no_device_registration:
+	bus_unregister(&mcde_bus_type);
+	return ret;
+}
+
+void mcde_display_exit(void)
+{
+	bus_unregister_notifier(&mcde_bus_type, &bus_nb);
+	device_unregister(&mcde_bus);
+	bus_unregister(&mcde_bus_type);
+}
diff --git a/drivers/video/mcde/mcde_mod.c b/drivers/video/mcde/mcde_mod.c
new file mode 100644
index 0000000..297857f
--- /dev/null
+++ b/drivers/video/mcde/mcde_mod.c
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * ST-Ericsson MCDE driver
+ *
+ * Author: Marcus Lorentzon <marcus.xm.lorentzon@stericsson.com>
+ * for ST-Ericsson.
+ *
+ * License terms: GNU General Public License (GPL), version 2.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+
+#include <video/mcde/mcde.h>
+#include <video/mcde/mcde_fb.h>
+#include <video/mcde/mcde_dss.h>
+#include <video/mcde/mcde_display.h>
+
+/* Module init */
+
+static int __init mcde_subsystem_init(void)
+{
+	int ret;
+	pr_info("MCDE subsystem init begin\n");
+
+	/* MCDE module init sequence */
+	ret = mcde_init();
+	if (ret)
+		return ret;
+	ret = mcde_display_init();
+	if (ret)
+		goto mcde_display_failed;
+	ret = mcde_dss_init();
+	if (ret)
+		goto mcde_dss_failed;
+	ret = mcde_fb_init();
+	if (ret)
+		goto mcde_fb_failed;
+	pr_info("MCDE subsystem init done\n");
+
+	return 0;
+mcde_fb_failed:
+	mcde_dss_exit();
+mcde_dss_failed:
+	mcde_display_exit();
+mcde_display_failed:
+	mcde_exit();
+	return ret;
+}
+#ifdef MODULE
+module_init(mcde_subsystem_init);
+#else
+fs_initcall(mcde_subsystem_init);
+#endif
+
+static void __exit mcde_module_exit(void)
+{
+	mcde_exit();
+	mcde_display_exit();
+	mcde_dss_exit();
+}
+module_exit(mcde_module_exit);
+
+MODULE_AUTHOR("Marcus Lorentzon <marcus.xm.lorentzon@stericsson.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ST-Ericsson MCDE driver");
+
-- 
1.6.3.3