Hi Laurent,

On Fri, Aug 17, 2012 at 02:49:39AM +0200, Laurent Pinchart wrote:

+/**

• • panel_get_modes - Get video modes supported by the panel
• • @panel: The panel
• • @modes: Pointer to an array of modes
• • Fill the modes argument with a pointer to an array of video modes. The array
• • is owned by the panel.
• • Return the number of supported modes on success (including 0 if no mode is
• • supported) or a negative error code otherwise.
• */

+int panel_get_modes(struct panel *panel, const struct fb_videomode **modes) +{

• if (!panel->ops || !panel->ops->get_modes)

• return 0;
  

• return panel->ops->get_modes(panel, modes);

+} +EXPORT_SYMBOL_GPL(panel_get_modes);

You have seen my of videomode helper proposal. One result there was that we want to have ranges for the margin/synclen fields. Does it make sense to base this new panel framework on a more sophisticated internal reprentation of the panel parameters? This could then be converted to struct fb_videomode / struct drm_display_mode as needed. This would also make it more suitable for drm drivers which are not interested in struct fb_videomode.

Related to this I suggest to change the API to be able to iterate over the different modes, like:

struct videomode *panel_get_mode(struct panel *panel, int num);

This was we do not have to have an array of modes in memory, which may be a burden for some panel drivers.

Sascha

On Thu, Sep 13, 2012 at 03:40:40AM +0200, Laurent Pinchart wrote:

Hi Sascha,

...

...

+int panel_get_modes(struct panel *panel, const struct fb_videomode **modes) +{

• if (!panel->ops || !panel->ops->get_modes)

• return 0;
  

• return panel->ops->get_modes(panel, modes);

+} +EXPORT_SYMBOL_GPL(panel_get_modes);

You have seen my of videomode helper proposal. One result there was that we want to have ranges for the margin/synclen fields. Does it make sense to base this new panel framework on a more sophisticated internal reprentation of the panel parameters?

I think it does, yes. We need a common video mode structure, and the panel framework should use it. I'll try to rebase my patches on top of your proposal. Have you posted the latest version ?

V2 is the newest version. I'd like to implement ranges for the display timings which then makes for a new common video mode structure, which then could be used by drm and fbdev, probably with helper functions to convert from common videomode to drm/fbdev specific variants.

...

This could then be converted to struct fb_videomode / struct drm_display_mode as needed. This would also make it more suitable for drm drivers which are not interested in struct fb_videomode.

Related to this I suggest to change the API to be able to iterate over the different modes, like:

struct videomode *panel_get_mode(struct panel *panel, int num);

This was we do not have to have an array of modes in memory, which may be a burden for some panel drivers.

I currently have mixed feelings about this. Both approaches have pros and cons. Iterating over the modes would be more complex for drivers that use panels, and would be race-prone if the modes can change at runtime (OK, this isn't supported by the current panel API proposal).

I just remember that the array approach was painful when I worked on an fbdev driver some time ago. There some possible modes came from platform_data, other modes were default modes in the driver and all had to be merged into a single array. I don't remember the situation exactly, but it would have been simpler if it had been a list instead of an array.

Sascha

On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:

+/* -----------------------------------------------------------------------------

• • Bus operations
• */

+void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long cmd) +{

• dev->bus->ops->write_command(dev->bus, cmd);

+} +EXPORT_SYMBOL_GPL(panel_dbi_write_command);

+void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long data) +{

• dev->bus->ops->write_data(dev->bus, data);

+} +EXPORT_SYMBOL_GPL(panel_dbi_write_data);

+unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) +{

• return dev->bus->ops->read_data(dev->bus);

+} +EXPORT_SYMBOL_GPL(panel_dbi_read_data);

I'm not that familiar with how to implement bus drivers, can you describe in pseudo code how the SoC's DBI driver would register these?

I think write/read data functions are a bit limited. Shouldn't they be something like write_data(const u8 *buf, int size) and read_data(u8 *buf, int len)?

Something that's totally missing is configuring the DBI bus. There are a bunch of timing related values that need to be configured. See include/video/omapdss.h struct rfbi_timings. While the struct is OMAP specific, if I recall right most of the values match to the MIPI DBI spec.

And this makes me wonder, you use DBI bus for SYS-80 panel. The busses may look similar in operation, but are they really so similar when you take into account the timings (and perhaps something else, it's been years since I read the MIPI DBI spec)?

Then there's the start_transfer. This is something I'm not sure what is the best way to handle it, but the same problems that I mentioned in the previous post related to enable apply here also. For example, what if the panel needs to be update in two parts? This is done in Nokia N9. From panel's perspective, it'd be best to handle it somewhat like this (although asynchronously, probably):

write_update_area(0, 0, xres, yres / 2); write_memory_start() start_pixel_transfer();

wait_transfer_done();

write_update_area(0, yres / 2, xres, yres / 2); write_memory_start() start_pixel_transfer();

Why I said I'm not sure about this is that it does complicate things, as the actual pixel data often comes from the display subsystem hardware, which should probably be controlled by the display driver.

I think there also needs to be some kind of transfer_done notifier, for both the display driver and the panel driver. Although if the display driver handles starting the actual pixel transfer, then it'll get the transfer_done via whatever interrupt the SoC has.

Also as food for thought, videomode timings does not really make sense for DBI panels, at least when you just consider the DBI side. There's really just the resolution of the display, and then the DBI timings. No pck, syncs, etc. Of course in the end there's the actual panel, which does have these video timings. But often they cannot be configured, and often you don't even know them as the specs don't tell them.

Tomi

On Fri, 2012-08-17 at 12:02 +0200, Laurent Pinchart wrote:

Hi Tomi,

Thank you for the review.

On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote:

...

On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:

...

+/*

---- + * Bus operations

• */

+void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long cmd) +{

• dev->bus->ops->write_command(dev->bus, cmd);

+} +EXPORT_SYMBOL_GPL(panel_dbi_write_command);

+void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long data) +{

• dev->bus->ops->write_data(dev->bus, data);

+} +EXPORT_SYMBOL_GPL(panel_dbi_write_data);

+unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) +{

• return dev->bus->ops->read_data(dev->bus);

+} +EXPORT_SYMBOL_GPL(panel_dbi_read_data);

I'm not that familiar with how to implement bus drivers, can you describe in pseudo code how the SoC's DBI driver would register these?

Sure.

The DBI bus driver first needs to create a panel_dbi_bus_ops instance:

static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = { .write_command = lcdc_dbi_write_command, .write_data = lcdc_dbi_write_data, .read_data = lcdc_dbi_read_data, };

Thanks for the example, I think it cleared up things a bit.

As I mentioned earlier, I really think "panel" is not right here. While the whole framework may be called panel framework, the bus drivers are not panels, and we should support external chips also, which are not panels either.

...

I think write/read data functions are a bit limited. Shouldn't they be something like write_data(const u8 *buf, int size) and read_data(u8 *buf, int len)?

Good point. My hardware doesn't support multi-byte read/write operations directly so I haven't thought about adding those.

OMAP HW doesn't support it either. Well, not quite true, as OMAP's system DMA could be used to write a buffer to the DBI output. But that's really the same as doing the write with a a loop with CPU.

But first, the data type should be byte, not unsigned long. How would you write 8 bits or 16 bits with your API? And second, if the function takes just u8, you'd need lots of calls to do simple writes.

Can your hardware group command + data writes in a single operation ? If so we should expose that at the API level as well.

No it can't. But with DCS that is a common operation, so we could have some helpers to send command + data with one call. Then again, I'd hope to have DCS somehow as a separate library, which would then use DBI/DSI/whatnot to actually send the data.

I'm not quite sure how easy that is because of the differences between the busses.

Is DBI limited to 8-bit data transfers for commands ? Pixels can be transferred 16-bit at a time, commands might as well. While DCS only specifies 8-bit command/data, DBI panels that are not DCS compliant can use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does so).

I have to say I don't remember much about DBI =). Looking at OMAP's driver, which was made for omap2 and hasn't been much updated since, I see that there are 4 modes, 8/9/12/16 bits. I think that defines how many of the parallel data lines are used.

However, I don't think that matters for the panel driver when it wants to send data. The panel driver should just call dbi_write(buf, buf_len), and the dbi driver would send the data in the buffer according to the bus width.

Also note that some chips need to change the bus width on the fly. The chip used on N800 wants configuration to be done with 8-bits, and pixel transfers with 16-bits. Who knows why...

So I think this, and generally most of the configuration, should be somewhat dynamic, so that the panel driver can change them when it needs.

...

Something that's totally missing is configuring the DBI bus. There are a bunch of timing related values that need to be configured. See include/video/omapdss.h struct rfbi_timings. While the struct is OMAP specific, if I recall right most of the values match to the MIPI DBI spec.

I've left that out currently, and thought about passing that information as platform data to the DBI bus driver. That's the easiest solution, but I agree that it's a hack. Panel should expose their timing requirements to the DBI host. API wise that wouldn't be difficult (we only need to add timing information to the panel platform data and add a function to the DBI API to retrieve it), one of challenges might be to express it in a way that's both universal enough and easy to use for DBI bus drivers.

As I pointed above, I think the panel driver shouldn't expose it, but the panel driver should somehow set it. Or at least allowed to change it in some manner. This is actually again, the same problem as with enable and transfer: who controls what's going on.

How I think it should work is something like:

mipi_dbi_set_timings(dbi_dev, mytimings); mipi_dbi_set_bus_width(dbi_dev, 8); mipi_dbi_write(dbi_dev, ...); mipi_dbi_set_bus_width(dbi_dev, 16); start_frame_transfer(dbi_dev, ...);

...

And this makes me wonder, you use DBI bus for SYS-80 panel. The busses may look similar in operation, but are they really so similar when you take into account the timings (and perhaps something else, it's been years since I read the MIPI DBI spec)?

I'll have to check all the details. SYS-80 is similar to DBI-B, but supports a wider bus width of 18 bits. I think the interfaces are similar enough to use a single bus implementation, possibly with quirks and/or options (see SCCB support in I2C for instance, with flags to ignore acks, force a stop bit generation, ...). We would duplicate lots of code if we had two different implementations, and would prevent a DBI panel to be attached to a SYS-80 host and vice-versa (the format is known to work).

Ah ok, if a DBI panel can be connected to SYS-80 output and vice versa, then I agree they are similar enough.

We might just need to provide fake timings. Video mode timings are at the core of display support in all drivers so we can't just get rid of them. The h/v front/back porch and sync won't be used by display drivers for DBI/DSI panels anyway.

Right. But we should probably think if we can, at the panel level, easily separate conventional panels and smart panels. Then this framework wouldn't need to fake the timings, and it'd be up to the higher level to decide if and how to fake them. Then again, this is no biggie. Just thought that at the lowest level it'd be nice to be "correct" and leave faking to upper layers =).

Tomi

On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote:

...

But first, the data type should be byte, not unsigned long. How would you write 8 bits or 16 bits with your API?

u8 and u16 both fit in an unsigned long :-) Please see below.

Ah, I see, so the driver would just discard 24 bits or 16 bits from the ulong. I somehow thought that if you have 8 bit bus, and you call the write with ulong, 4 bytes will be written.

...

Then again, I'd hope to have DCS somehow as a separate library, which would then use DBI/DSI/whatnot to actually send the data.

I'm not quite sure how easy that is because of the differences between the busses.

...

Is DBI limited to 8-bit data transfers for commands ? Pixels can be transferred 16-bit at a time, commands might as well. While DCS only specifies 8-bit command/data, DBI panels that are not DCS compliant can use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does so).

I have to say I don't remember much about DBI =). Looking at OMAP's driver, which was made for omap2 and hasn't been much updated since, I see that there are 4 modes, 8/9/12/16 bits. I think that defines how many of the parallel data lines are used.

SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when transferring instructions and data other than pixels (for pixels the 18-bits bus width can be used to transfer RGB666 in a single clock cycle).

See page 87 of http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.

...

However, I don't think that matters for the panel driver when it wants to send data. The panel driver should just call dbi_write(buf, buf_len), and the dbi driver would send the data in the buffer according to the bus width.

According to the DCS specification, commands and parameters are transferred using 8-bit data. Non-DCS panels can however use wider commands and parameters (all commands and parameters are 16-bits wide for the R61505 for instance).

We can add an API to switch the DBI bus width on the fly. For Renesas hardware this would "just" require shifting bits around to output the 8-bit or 16-bit commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while the DCS specification mentions D7-D0) based on how the panel is connected and on which lines the panel expects data.

As commands can be expressed on either 8 or 16 bits I would use a 16 type for them.

I won't put my head on the block, but I don't think DBI has any restriction on the size of the command. A "command" just means a data transfer while keeping the D/CX line low, and "data" when the line is high. Similar transfers for n bytes can be done in both modes.

For parameters, we can either express everything as u8 * in the DBI bus operations, or use a union similar to i2c_smbus_data

union i2c_smbus_data { __u8 byte; __u16 word; __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */ /* and one more for user-space compatibility */ };

There's no DBI_BLOCK_MAX, so at least identical union won't work. I think it's simplest to have u8 * function as a base, and then a few helpers to write the most common datatypes.

So we could have on the lowest level something like:

dbi_write_command(u8 *buf, size_t size); dbi_write_data(u8 *buf, size_t size);

And possible helpers:

dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t data_size);

dbi_write_dcs(u8 cmd, u8 *data, size_t size);

And variations:

dbi_write_dcs_0(u8 cmd); dbi_write_dcs_1(u8 cmd, u8 data);

etc. So a simple helper to send 16 bits would be:

dbi_write_data(u16 data) { // or are the bytes the other way around... u8 buf[2] = { data & 0xff, (data >> 8) & 0xff }; return dbi_write_data(buf, 2); }

Helper functions would be available to perform 8-bit, 16-bit or n*8 bits transfers.

Would that work for your use cases ?

...

Also note that some chips need to change the bus width on the fly. The chip used on N800 wants configuration to be done with 8-bits, and pixel transfers with 16-bits. Who knows why...

On which data lines is configuration performed ? D7-D0 ?

I guess so, but does it matter? All the bus driver needs to know is how to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit register, and the HW takes the lowest n bits. Do the bits represent the data lines directly on Renesans?

The omap driver actually only implements 8 and 16 bit modes, not the 9 and 12 bit modes. I'm not sure what kind of shifting is needed for those.

...

...

We might just need to provide fake timings. Video mode timings are at the core of display support in all drivers so we can't just get rid of them. The h/v front/back porch and sync won't be used by display drivers for DBI/DSI panels anyway.

Right. But we should probably think if we can, at the panel level, easily separate conventional panels and smart panels. Then this framework wouldn't need to fake the timings, and it'd be up to the higher level to decide if and how to fake them. Then again, this is no biggie. Just thought that at the lowest level it'd be nice to be "correct" and leave faking to upper layers =).

But we would then have two different APIs at the lower level depending on the panel type. I'm not sure that's a good thing.

Different API for what? Why anyway need panel type specific functions. In the panel struct we could just have an union of the different types of parameters for different types of panels.

But if this complicates things, it's not a biggie. Just something that has been in my mind when dealing with smart panels and assigning dummy video timings for them =).

Tomi

Hi Tomi,

Thanks a lot for the review.

On Friday 17 August 2012 11:38:14 Tomi Valkeinen wrote:

On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:

...

I will appreciate all reviews, comments, criticisms, ideas, remarks, ... If

Oookay, where to start... ;)

A few cosmetic/general comments first.

I find the file naming a bit strange. You have panel.c, which is the core framework, panel-dbi.c, which is the DBI bus, panel-r61517.c, which is driver for r61517 panel...

Perhaps something in this direction (in order): panel-core.c, mipi-dbi-bus.c, panel-r61517.c?

That looks good to me. I'll then rename panel_dbi_* to mipi_dbi_*.

And we probably end up with quite a lot of panel drivers, perhaps we should already divide these into separate directories, and then we wouldn't need to prefix each panel with "panel-" at all.

What kind of directory structure do you have in mind ? Panels are already isolated in drivers/video/panel/ so we could already ditch the panel- prefix in drivers.

Would you also create include/video/panel/ ?

---

Should we aim for DT only solution from the start? DT is the direction we are going, and I feel the older platform data stuff would be deprecated soon.

Don't forget about non-ARM architectures :-/ We need panel drivers for SH as well, which doesn't use DT. I don't think that would be a big issue, a DT- compliant solution should be easy to use through board code and platform data as well.

---

Something missing from the intro is how this whole thing should be used. It doesn't help if we know how to turn on the panel, we also need to display something on it =). So I think some kind of diagram/example of how, say, drm would use this thing, and also how the SoC specific DBI bus driver would be done, would clarify things.

Of course. If I had all that information already I would have shared it :-) This is really a first RFC, my goal is to make sure that I'm going in the right direction.

---

We have discussed face to face about the different hardware setups and scenarios that we should support, but I'll list some of them here for others:

1. We need to support chains of external display chips and panels. A

simple example is a chip that takes DSI in, and outputs DPI. In that case we'd have a chain of SoC -> DSI2DPI -> DPI panel.

In final products I think two external devices is the maximum (at least I've never seen three devices in a row), but in theory and in development environments the chain can be arbitrarily long. Also the connections are not necessarily 1-to-1, but a device can take one input while it has two outputs, or a device can take two inputs.

Now, I think two external devices is a must requirement. I'm not sure if supporting more is an important requirement. However, if we support two devices, it could be that it's trivial to change the framework to support n devices.

2. Panels and display chips are all but standard. They very often have

their own sequences how to do things, have bugs, or implement some feature in slightly different way than some other panel. This is why the panel driver should be able to control or define the way things happen.

As an example, Sharp LQ043T1DG01 panel (www.sharpsme.com/download/LQ043T1DG01-SP-072106pdf). It is enabled with the following sequence:

• Enable VCC and AVDD regulators
• Wait min 50ms
• Enable full video stream (pck, syncs, pixels) from SoC
• Wait min 0.5ms
• Set DISP GPIO, which turns on the display panel

Here we could split the enabling of panel to two parts, prepare (in this case starts regulators and waits 50ms) and finish (wait 0.5ms and set DISP GPIO), and the upper layer would start the video stream in between.

I realize this could be done with the PANEL_ENABLE_* levels in your RFC, but I don't think the concepts quite match:

• PANEL_ENABLE_BLANK level is needed for "smart panels", as we need to

configure them and send the initial frame at that operating level. With dummy panels there's really no such level, there's just one enable sequence that is always done right away.

• I find waiting at the beginning of a function very ugly (what are we

waiting for?) and we'd need that when changing the panel to PANEL_ENABLE_ON level.

• It's still limited if the panel is a stranger one (see following

example).

Consider the following theoretical panel enable example, taken to absurd level just to show the general problem:

• Enable regulators
• Enable video stream
• Wait 50ms
• Disable video stream
• Set enable GPIO
• Enable video stream

This one would be rather impossible with the upper layer handling the enabling of the video stream. Thus I see that the panel driver needs to control the sequences, and the Sharp panel driver's enable would look something like:

regulator_enable(...); sleep(); dpi_enable_video(); sleep(); gpip_set(..);

I have to admit I have no better solution to propose at the moment, even if I don't really like making the panel control the video stream. When several devices will be present in the chain all of them might have similar annoying requirements, and my feeling is that the resulting code will be quite messy. At the end of the day the only way to really find out is to write an implementation.

Note that even with this model we still need the PANEL_ENABLE levels you have.

---

I'm not sure I understand the panel unload problem you mentioned. Nobody should have direct references to the panel functions, so there shouldn't be any automatic references that would prevent module unloading. So when the user does rmmod panel-mypanel, the panel driver's remove will be called. It'll unregister itself from the panel framework, which causes notifications and the display driver will stop using the panel. After that nobody has pointers to the panel, and it can safely be unloaded.

It could cause some locking issues, though. First the panel's remove could take a lock, but the remove sequence would cause the display driver to call disable on the panel, which could again try to take the same lock...

We have two possible ways of calling panel operations, either directly (panel-

bus->ops->enable(...)) or indirectly (panel_enable(...)).

The former is what V4L2 currently does with subdevs, and requires display drivers to hold a reference to the panel. The later can do without a direct reference only if we use a global lock, which is something I would like to avoid. A panel-wide lock wouldn't work, as the access function would need to take the lock on a panel instance that can be removed at any time.

Note that this issue is not specific to panels, V4L2 will need a solution as well when V4L2 subdevs will be instantiated from the DT.

Hi Tomi,

On Friday 17 August 2012 14:42:31 Tomi Valkeinen wrote:

On Fri, 2012-08-17 at 13:10 +0200, Laurent Pinchart wrote:

...

What kind of directory structure do you have in mind ? Panels are already isolated in drivers/video/panel/ so we could already ditch the panel- prefix in drivers.

The same directory also contains files for the framework and buses. But perhaps there's no need for additional directories if the amount of non-panel files is small. And you can easily see from the name that they are not panel drivers (e.g. mipi_dbi_bus.c).

I don't expect the directory to contain many non-panel files, so let's keep it as-is for now.

mipi-dbi-bus might not belong to include/video/panel/ though, as it can be used for non-panel devices (at least in theory). The future mipi-dsi-bus certainly will.

...

Would you also create include/video/panel/ ?

Perhaps that would be good. Well, having all the files prefixed with panel- is not bad as such, but just feel extra.

...

...

---

Should we aim for DT only solution from the start? DT is the direction we are going, and I feel the older platform data stuff would be deprecated soon.

Don't forget about non-ARM architectures :-/ We need panel drivers for SH as well, which doesn't use DT. I don't think that would be a big issue, a DT- compliant solution should be easy to use through board code and platform data as well.

I didn't forget about them as I didn't even think about them ;). I somehow had the impression that other architectures would also use DT, sooner or later. I could be mistaken, though.

And true, it's not a big issue to support both DT and non-DT versions, but I've been porting omap stuff for DT and keeping the old platform data stuff also there, and it just feels burdensome. For very simple panels it's easy, but when you've passing lots of parameters the code starts to get longer.

...

...

This one would be rather impossible with the upper layer handling the enabling of the video stream. Thus I see that the panel driver needs to control the sequences, and the Sharp panel driver's enable would look something like:

regulator_enable(...); sleep(); dpi_enable_video(); sleep(); gpip_set(..);

I have to admit I have no better solution to propose at the moment, even if I don't really like making the panel control the video stream. When several devices will be present in the chain all of them might have similar annoying requirements, and my feeling is that the resulting code will be quite messy. At the end of the day the only way to really find out is to write an implementation.

If we have a chain of devices, and each device uses the bus interface from the previous device in the chain, there shouldn't be a problem. In that model each device can handle the task however is best for it.

I think the problems come from the divided control we'll have. I mean, if the panel driver would decide itself what to send to its output, and it would provide the data (think of an i2c device), this would be very simple. And it actually is for things like configuration data etc, but not so for video stream.

Would you be able to send incremental patches on top of v2 to implement the solution you have in mind ? It would be neat if you could also implement mipi- dsi-bus for the OMAP DSS and test the code with a real device :-)

...

...

It could cause some locking issues, though. First the panel's remove could take a lock, but the remove sequence would cause the display driver to call disable on the panel, which could again try to take the same lock...

We have two possible ways of calling panel operations, either directly (panel->bus->ops->enable(...)) or indirectly (panel_enable(...)).

The former is what V4L2 currently does with subdevs, and requires display drivers to hold a reference to the panel. The later can do without a direct reference only if we use a global lock, which is something I would like to

Wouldn't panel_enable() just do the same panel->bus->ops->enable() anyway, and both require a panel reference? I don't see the difference.

Indeed, you're right. I'm not sure what I was thinking about.

...

avoid. A panel-wide lock wouldn't work, as the access function would need to take the lock on a panel instance that can be removed at any time.

Can't this be handled with some kind of get/put refcounting? If there's a ref, it can't be removed.

Trouble will come when the display driver will hold a reference to the panel, and the panel will hold a reference to the display driver (for instance because the display driver provides the DBI/DSI bus, or because it provides a clock used by the panel).

Generally about locks, if we define that panel ops may only be called exclusively, does it simplify things? I think we can make such requirements, as there should be only one display framework that handles the panel. Then we don't need locking for things like enable/disable.

Pushing locking to callers would indeed simplify panel drivers, but we need to make sure we won't need to expose a panel to several callers in the future.

Of course we need to be careful about things where calls come from "outside" the display framework. I guess one such thing is rmmod, but if that causes a notification to the display framework, which again handles locking, it shouldn't be a problem.

Another thing to be careful about is if the panel internally uses irqs, workqueues, sysfs files or such. In that case it needs to handle locking.

Of course panels will need to manage concurrency for their own infrastructure.

Hi Tomi,

On Monday 20 August 2012 14:39:30 Tomi Valkeinen wrote:

On Sat, 2012-08-18 at 03:16 +0200, Laurent Pinchart wrote:

...

Hi Tomi,

mipi-dbi-bus might not belong to include/video/panel/ though, as it can be used for non-panel devices (at least in theory). The future mipi-dsi-bus certainly will.

They are both display busses. So while they could be used for anything, I find it quite unlikely as there are much better alternatives for generic bus needs.

My point is that they could be used for display devices other than panels. This is especially true for DSI, as there are DSI to HDMI converters. Technically speaking that's also true for DBI, as DBI chips convert from DBI to DPI, but we can group both the DBI-to-DPI chip and the panel in a single panel object.

...

Would you be able to send incremental patches on top of v2 to implement the solution you have in mind ? It would be neat if you could also implement mipi- dsi-bus for the OMAP DSS and test the code with a real device :-)

Yes, I'd like to try this out on OMAP, both DBI and DSI. However, I fear it'll be quite complex due to the dependencies all around we have in the current driver. We're working on simplifying things so that it'll be easier to try thing like the panel framework, though, so we're going in the right direction.

If you want the panel framework to support your use cases I'm afraid you will need to work on that ;-)

...

...

Generally about locks, if we define that panel ops may only be called exclusively, does it simplify things? I think we can make such requirements, as there should be only one display framework that handles the panel. Then we don't need locking for things like enable/disable.

Pushing locking to callers would indeed simplify panel drivers, but we need to make sure we won't need to expose a panel to several callers in the future.

I have a feeling that would be a bad idea.

Display related stuff are quite sensitive to any delays, so any extra transactions over, say, DSI bus could cause a noticeable glitch on the screen. I'm not sure what are all the possible ops that a panel can offer, but I think all that affect the display or could cause delays should be handled by one controlling entity (drm or such). The controlling entity needs to handle locking anyway, so in that sense I don't think it's an extra burden for it.

The things that come to my mind that could possibly cause calls to the panel outside drm: debugfs, sysfs, audio, backlight. Of those, I think backlight should go through drm. Audio, no idea. debugfs and sysfs locking needs to be handled by the panel driver, and they are a bit problematic as I guess having them requires full locking.

Hi Tomi,

On Tuesday 21 August 2012 08:49:57 Tomi Valkeinen wrote:

On Tue, 2012-08-21 at 01:29 +0200, Laurent Pinchart wrote:

...

On Monday 20 August 2012 14:39:30 Tomi Valkeinen wrote:

...

On Sat, 2012-08-18 at 03:16 +0200, Laurent Pinchart wrote:

...

Hi Tomi,

mipi-dbi-bus might not belong to include/video/panel/ though, as it can be used for non-panel devices (at least in theory). The future mipi-dsi-bus certainly will.

They are both display busses. So while they could be used for anything, I find it quite unlikely as there are much better alternatives for generic bus needs.

My point is that they could be used for display devices other than panels. This is especially true for DSI, as there are DSI to HDMI converters. Technically speaking that's also true for DBI, as DBI chips convert from DBI to DPI, but we can group both the DBI-to-DPI chip and the panel in a single panel object.

Ah, ok. I thought "panels" would include these buffer/converter chips.

I think we should have one driver for one indivisible hardware entity. So if you've got a panel module that contains DBI receiver, buffer memory and a DPI panel, let's just have one "DBI panel" driver for it.

If we get lots of different panel modules containing the same DBI RX IP, we could have the DBI IP part as a common library, but still have one panel driver per panel module.

Sounds good to me.

But how do you see the case for separate converter/buffer chips? Are they part of the generic panel framework? I guess they kinda have to be. On one side they use the "panel" API control the bus they are connected to, and on the other they offer an API for the connected panel to use the bus they provide.

The DBI/DSI APIs will not be panel-specific (they won't contain any reference to "panel") I'm thus thinking of moving them from drivers/video/panel/ to drivers/video/.

Furthermore, a DSI-to-HDMI converter chip is not a panel, but needs to expose display-related operations in a way similar to panels. I was thus wondering if we shouldn't replace the panel structure with some kind of video entity structure that would expose operations similar to panels. We could then extend that structure with converter-specific operations later. The framework would become a bit more generic, while remaining display-specific.

Did you just mean we should have a separate directory for them, while still part of the same framework, or...?

Hi Laurent,

Basically I agree that we need a common panel framework. I just have some questions: 1. I think we should add color format in videomode - if we use such common video mode structure shared across subsystems. In HDMI, colors are bind with timings tightly. We need a combined videomode with timing and color format together. 2. I think we should add "set_videomode" interface. It helps HDMI monitors to set EDIDs.

Hi Jun,

I've finally been able to resume my work on the panel framework (I hope to post a v2 at the end of the week).

On Thursday 23 August 2012 14:23:01 Jun Nie wrote:

Hi Laurent, Do you plan to add an API to get and parse EDID to mode list?

An API to get the raw EDID data is likely needed. Parsing EDID data in the panel driver and providing the modes to the caller isn't enough, as EDID contains more than just video modes. I'm not sure whether a driver for an EDID-aware panel should parse the EDID data internally and provide both modes and raw EDID data, or only raw EDID data.

video mode is tightly coupled with panel that is capable of hot-plug. Or you are busy on modifying EDID parsing code for sharing it amoung DRM/FB/etc? I see you mentioned this in Mar.

That's needed as well, but -ENOTIME :-S

It is great if you are considering add more info into video mode, such as pixel repeating, 3D timing related parameter.

Please have a look at "[PATCH 2/2 v6] of: add generic videomode description" on dri-devel. There's a proposal for a common video mode structure.

I have some code for CEA modes filtering and 3D parsing, but still tight coupled with FB and with a little hack style.

My HDMI driver is implemented as lcd device as you mentioned here.

But more complex than other lcd devices for a kthread is handling hot-plug/EDID/HDCP/ASoC etc.

I also feel a little weird to add code parsing HDMI audio related

info in fbmod.c in my current implementation, thought it is the only place to handle EDID in kernel. Your panel framework provide a better place to add panel related audio/HDCP code. panel notifier can also trigger hot-plug related feature, such as HDCP start.

That's a good idea. I was wondering whether to put the common EDID parser in drivers/gpu/drm, drivers/video or drivers/media. Putting it wherever the panel framework will be might be a good option as well.

Looking forward to your hot-plug panel patch. Or I can help add it

if you would like me to.

I'll try to post a v2 at the end of the week, but likely without much hot-plug support. Patches and enhancement proposals will be welcome.

On 2012-10-31 15:13, Laurent Pinchart wrote:

...

OMAP SoC

So here's first the SoC specific display nodes. OMAP has a DSS (display subsystem) block, which contains the following elements:

• DISPC (display controller) reads the pixels from memory and outputs

them using specified video timings. DISPC has three outputs, LCD0, LCD1 and TV. These are SoC internal outputs, they do not go outside the SoC.

• DPI gets its data from DISPC's LCD0, and outputs MIPI DPI (parallel

RBG)

• Two independent DSI modules, which get their data from LCD0 or LCD1,

and output MIPI DSI (a serial two-way video bus)

• HDMI, gets data from DISPC's TV output and outputs HDMI

/ { ocp { dss { dispc { dss-lcd0: output@0 { };

		dss-lcd1: output@1 {
		};

		dss-tv: output@2 {
		};
	};

	dpi: dpi {
		video-source = <&dss-lcd0>;
	};

	dsi0: dsi@0 {
		video-source = <&dss-lcd0>;
	};

	dsi1: dsi@1 {
		video-source = <&dss-lcd1>;
	};

	hdmi: hdmi {
		video-source = <&dss-tv>;
	};
};

}; };

I have defined all the relevant nodes, and video-source property is used to point to the source for video data. I also define aliases for the SoC outputs so that panels can use them.

One thing to note is that the video sources for some of the blocks, like DSI, are not hardcoded in the HW, so dsi0 could get its data from LCD0 or LCD1.

What about the source that are hardwired in hardware ? Shouldn't those be hardcoded in the driver instead ?

Even if both the DSI and the DISPC are parts of OMAP DSS, and part of the SoC, they are separate IPs. We should look at them the same way we'd consider chips that are outside the SoC.

So things that are internal to a device can (and I think should) be hardcoded in the driver, but integration details, the connections between the IPs, etc, should be described in the DT data.

Then again, we do have (and need) a driver for the "dss" node in the above DT data. This dss represents dss_core, a "glue" IP that contains the rest of the DSS blocks and muxes and such. It could be argued that this dss_core driver does indeed know the integration details, and thus there's no need to represent them in the DT data.

However, I do think that we should represent the DISPC outputs with generic display entities inside CPF, just like DSI and the panels. And we do need to set the connections between these entities. So the question is, do we get those connections from the DT data, or are they hardcoded in the dss_core driver.

I don't currently have strong opinions on either direction. Both make sense to me. But I think this is SoC driver implementation specific, and the common panel framework doesn't need to force this to either direction. Both should be possible from CPF's point of view.

...

However, I don't think they are usually changed during runtime, and the dss driver cannot change them independently for sure (meaning that some upper layer should tell it how to change the config). Thus I specify sane defaults here, but the board dts files can of course override the video sources.

I'm not sure whether default settings like those really belong to the DT. I'm no expert on that topic though.

I agree. But I also don't have a good solution how the driver would find good choices for these settings...

...

Another thing to note is that we have more outputs from OMAP than we have outputs from DISPC. This means that the same video source is used by multiple sinks (LCD0 used by DPI and DSI0). DPI and DSI0 cannot be used at the same time, obviously.

It might not be really obvious, as I don't see what prevents DPI and DSI0 to be used at the same time :-) Do they share physical pins ?

I think they do, but even if they didn't, there's just one source for two outputs. So if the SoC design is such that the only video source for DPI is LCD0, and the only video source for DSI0 is LCD0, and presuming you can't send the video data to both destinations, then only one of DPI and DSI0 can be enabled at the same time.

Even if the LCD0 could send the pixel stream to both DPI and DSI0, it'd be "interesting", because the original video timings and pixel clock are programmed in the LCD0 output, and thus both DPI and DSI0 get the same timings. If DPI would want to use some other mode, DSI would most likely go nuts.

So my opinion is that we should only allow 1:1 connections between sources and sinks. If a component has multiple outputs, and even if those outputs give the exact same data, it should define multiple sources.

...

And third thing to note, DISPC node defines outputs explicitly, as it has multiple outputs, whereas the external outputs do not as they have only one output. Thus the node's output is implicitly the node itself. So, instead of having:

ds0: dsi@0 { video-source = <&dss-lcd0>; dsi0-out0: output@0 { }; };

I have:

dsi0: dsi@0 { video-source = <&dss-lcd0>; };

What about defining the data sinks instead of the data sources ? I find it more logical for the DSS to get the panel it's connected to than the other way around.

Good question. We look at this from different angles. Is the DSS connected to the panel, or is the panel connected to the DSS? ;)

I see this the same way than, say, regulators. We have a device and a driver for it, and the driver wants to use a hw resource. If the resource is a regulator, the DT data for the device will contain a reference to the regulator. The driver will then get the regulator, and use it to operate the device.

Similarly for the display devices. We have a driver for a, say, i2c device. The DT data for that device will contain a reference to the source for the video data. The driver for the device will then use this reference to enable/disable/etc the video stream (i.e. operate the device).

The regulators don't even really know anything about the users of the regulators. Somebody just "gets" a regulator and enables it. The same should work for display entities also. There's no need for the video source to know anything about the sink.

So looking at a chain like:

DISPC -> DPI -> Panel

I see the component on the right side using the component on its left. And thus it makes sense to me to have references from right to left, i.e. panel has reference to DPI, etc.

Also, it makes sense with DT data files. For example, for omap4 we'll have omap4.dtsi, which describes common omap4 details. This file would contain the omap dss nodes. Then we have omap4-panda.dts, which includes omap4.dtsi. omap4-panda.dts contains data for the displays on this board. Thus we have something like:

omap4.dtsi:

dss { dsi0: dsi@0 { ... }; };

omap4-panda.dts:

/ { panel { video-source = <&dsi0>; }; };

So it comes out quite nicely. If we had the references the other way around, omap4-panda.dts would need to have the panel definition, and also override the dsi0 node from omap4.dtsi, and set the sink to the panel.

I have to say we've had this mind-set with omapdss for a long time, so I may be a bit blind to other alternative. Do you have any practical examples how linking the other way around would be better?

...

Of this I'm a bit unsure. I believe in most cases there's only one output, so it'd be nice to have a shorter representation, but I'm not sure if it's good to handle the cases for single and multiple outputs differently. Or if it's good to mix control and data busses, as, for example, dsi0 can be used as both control and data bus. Having the output defined explicitly would separate the control and data bus nodes.

Simple DPI panel

Here a board has a DPI panel, which is controlled via i2c. Panel nodes are children of the control bus, so in this case we define the panel under i2c2.

&i2c2 { dpi-lcd-panel { video-source = <&dpi>;

}; };

HDMI

OMAP has a HDMI output, but it cannot be connected directly to an HDMI cable. TI uses tpd12s015 chip in its board, which provides ESD, level-shifting and whatnot (I'm an SW guy, google for the chip to read the details =). tpd12s015 has a few GPIOs and powers that need to be controlled, so we need a driver for it.

There's no control bus for the tpd12s015, so it's platform device. Then there's the device for the HDMI monitor, and the DDC lines are connected to OMAP's i2c4, thus the hdmi monitor device is a child of i2c.

/ { hdmi-connector: tpd12s015 { video-source = <&hdmi>; }; };

&i2c4 { hdmi-monitor { video-source = <&hdmi-connector>; }; };

So this implied we would have the following chain ?

DISPC (on SoC) -> HDMI (on SoC) -> TPD12S015 (on board) -> HDMI monitor (off board)

Yes.

Although to be honest, I'm not sure if the TPD12S015 should be part of the chain, or somehow separate. It's so simple, kinda pass-through IP, that it would be nicer to have the HDMI monitor connected to the OMAP HDMI. But this is omap specific question, not really CPF thing.

Should we then have a driver for the HDMI monitor ?

Yes, that is my opinion:

- It would be strange if for some video chains we would have panel devices in the end of the chain, and for some we wouldn't. I guess we'd need some trickery to make them both work the same way if there's no panel devices for some cases.

- While in 99% of the cases we can use a common, simple HDMI monitor driver, there could be HDMI monitors with special features. We could detect this monitor by reading the EDID and if it's this special case, use a specific driver for it instead of the common HDMI driver. This is perhaps not very likely with HDMI, but I could imagine eDP panels with special features.

So I imagine that we could use hot-plug here. The HDMI monitor device would not exist until a HDMI cable is connected. The SoC's HDMI driver (or whatever is before the HDMI monitor in the chain) gets a hotplug interrupt, and it would then add the device, which would then trigger probe of the corresponding HDMI monitor driver.

Actually, thinking about this, what I said in the above paragraph wouldn't work. The SoC's HDMI driver can't know what kind of device to create, unless we have a HDMI bus and HDMI devices. Which, I think, we shouldn't have, as HDMI monitors are usually controlled via i2c, and thus the HDMI monitors should be i2c devices.

So I don't really know how this hotplug would work =). It's just an idea, not a scenario I have at hand.

Tomi

correct some typo. Sorry for this.

2012/10/20 Inki Dae inki.dae@samsung.com:

Hi Laurent. sorry for being late.

2012/8/17 Laurent Pinchart laurent.pinchart@ideasonboard.com:

...

Hi everybody,

While working on DT bindings for the Renesas Mobile SoC display controller (a.k.a. LCDC) I quickly realized that display panel implementation based on board code callbacks would need to be replaced by a driver-based panel framework.

Several driver-based panel support solution already exist in the kernel.

• The LCD device class is implemented in drivers/video/backlight/lcd.c and exposes a kernel API in include/linux/lcd.h. That API is tied to the FBDEV API for historical reason, uses board code callback for reset and power management, and doesn't include support for standard features available in today's "smart panels".

• OMAP2+ based systems use custom panel drivers available in drivers/video/omap2/displays. Those drivers are based on OMAP DSS (display controller) specific APIs.

• Similarly, Exynos based systems use custom panel drivers available in drivers/video/exynos. Only a single driver (s6e8ax0) is currently available. That driver is based on Exynos display controller specific APIs and on the LCD device class API.

I've brought up the issue with Tomi Valkeinen (OMAP DSS maintainer) and Marcus Lorentzon (working on panel support for ST/Linaro), and we agreed that a generic panel framework for display devices is needed. These patches implement a first proof of concept.

One of the main reasons for creating a new panel framework instead of adding missing features to the LCD framework is to avoid being tied to the FBDEV framework. Panels will be used by DRM drivers as well, and their API should thus be subsystem-agnostic. Note that the panel framework used the fb_videomode structure in its API, this will be replaced by a common video mode structure shared across subsystems (there's only so many hours per day).

Panels, as used in these patches, are defined as physical devices combining a matrix of pixels and a controller capable of driving that matrix.

Panel physical devices are registered as children of the control bus the panel controller is connected to (depending on the panel type, we can find platform devices for dummy panels with no control bus, or I2C, SPI, DBI, DSI, ... devices). The generic panel framework matches registered panel devices with panel drivers and call the panel drivers probe method, as done by other device classes in the kernel. The driver probe() method is responsible for instantiating a struct panel instance and registering it with the generic panel framework.

Display drivers are panel consumers. They register a panel notifier with the framework, which then calls the notifier when a matching panel is registered. The reason for this asynchronous mode of operation, compared to how drivers acquire regulator or clock resources, is that the panel can use resources provided by the display driver. For instance a panel can be a child of the DBI or DSI bus controlled by the display device, or use a clock provided by that device. We can't defer the display device probe until the panel is registered and also defer the panel device probe until the display is registered. As most display drivers need to handle output devices hotplug (HDMI monitors for instance), handling panel through a notification system seemed to be the easiest solution.

Note that this brings a different issue after registration, as display and panel drivers would take a reference to each other. Those circular references would make driver unloading impossible. I haven't found a good solution for that problem yet (hence the RFC state of those patches), and I would appreciate your input here. This might also be a hint that the framework design is wrong to start with. I guess I can't get everything right on the first try ;-)

Getting hold of the panel is the most complex part. Once done, display drivers can call abstract operations provided by panel drivers to control the panel operation. These patches implement three of those operations (enable, start transfer and get modes). More operations will be needed, and those three operations will likely get modified during review. Most of the panels on devices I own are dumb panels with no control bus, and are thus not the best candidates to design a framework that needs to take complex panels' needs into account.

In addition to the generic panel core, I've implemented MIPI DBI (Display Bus Interface, a parallel bus for panels that supports read/write transfers of commands and data) bus support, as well as three panel drivers (dummy panels with no control bus, and Renesas R61505- and R61517-based panels, both using DBI as their control bus). Only the dummy panel driver has been tested as I lack hardware for the two other drivers.

I will appreciate all reviews, comments, criticisms, ideas, remarks, ... If you can find a clever way to solve the cyclic references issue described above I'll buy you a beer at the next conference we will both attend. If you think the proposed solution is too complex, or too simple, I'm all ears. I personally already feel that we might need something even more generic to support other kinds of external devices connected to display controllers, such as external DSI to HDMI converters for instance. Some kind of video entity exposing abstract operations like the panels do would make sense, in which case panels would "inherit" from that video entity.

Speaking of conferences, I will attend the KS/LPC in San Diego in a bit more than a week, and would be happy to discuss this topic face to face there.

Laurent Pinchart (5): video: Add generic display panel core video: panel: Add dummy panel support video: panel: Add MIPI DBI bus support video: panel: Add R61505 panel support video: panel: Add R61517 panel support

how about using 'buses' directory instead of 'panel' and adding

s/buses/busses

'panel' under that like below? video/buess: display bus frameworks such as MIPI-DBI/DSI and eDP are placed. video/buess/panel: panel drivers based on display bus-based drivers are placed.

video/busses: display bus frameworks such as MIPI-DBI/DSI and eDP are placed. video/busses/panel: panel drivers based on display bus-based drivers are placed.

I think MIPI-DBI(Display Bus Interface)/DSI(Display Serial Interface) and eDP are the bus interfaces for display controllers such as DISC(OMAP SoC) and FIMC(Exynos SoC).

Thanks, Inki Dae

...

drivers/video/Kconfig | 1 + drivers/video/Makefile | 1 + drivers/video/panel/Kconfig | 37 +++ drivers/video/panel/Makefile | 5 + drivers/video/panel/panel-dbi.c | 217 +++++++++++++++ drivers/video/panel/panel-dummy.c | 103 +++++++ drivers/video/panel/panel-r61505.c | 520 ++++++++++++++++++++++++++++++++++++ drivers/video/panel/panel-r61517.c | 408 ++++++++++++++++++++++++++++ drivers/video/panel/panel.c | 269 +++++++++++++++++++ include/video/panel-dbi.h | 92 +++++++ include/video/panel-dummy.h | 25 ++ include/video/panel-r61505.h | 27 ++ include/video/panel-r61517.h | 28 ++ include/video/panel.h | 111 ++++++++ 14 files changed, 1844 insertions(+), 0 deletions(-) create mode 100644 drivers/video/panel/Kconfig create mode 100644 drivers/video/panel/Makefile create mode 100644 drivers/video/panel/panel-dbi.c create mode 100644 drivers/video/panel/panel-dummy.c create mode 100644 drivers/video/panel/panel-r61505.c create mode 100644 drivers/video/panel/panel-r61517.c create mode 100644 drivers/video/panel/panel.c create mode 100644 include/video/panel-dbi.h create mode 100644 include/video/panel-dummy.h create mode 100644 include/video/panel-r61505.h create mode 100644 include/video/panel-r61517.h create mode 100644 include/video/panel.h

-- Regards,

Laurent Pinchart

-- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html