Add panel-dsi for device-tree side configuration of generic DSI panels

[timonsku]

This adds an analog to the already existing panel-dpi to panel-simple which allows for timing configuration in device-tree for simple DSI panels.
In addition this also allows configuration of the DSI parameters in device tree.
I tried to be as consistent as possible with how panel-dpi was implemented.
Code was tested with a 840x480 panel driven by an externally configured bridge IC.

This addition was discussed with @6by9 over in raspberrypi/firmware#1819 (comment)
The main motivation is with not needing to run custom kernels for simple DSI devices that require nothing but an entry in panel-simple for the timings and DSI config flags.
This becomes very cumbersome when a large amount of displays need to be maintained. Essentially the exact same reasoning that the panel-dpi patch had for the initial implementation for this type of feature in panel-simple but expanding it to also be possible for DSI panels.

Any feedback would be welcome.

[6by9]

Referencing https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml#L58-L60, you ought to be supporting a reg property to set the DSI virtual channel.

[6by9]

Random whitespace change

[6by9]

Still present

[aBUGSworstnightmare-rpi]

May I ask which bridge device this is needed/intended for?
for a MIPI-to-DPI bridge (as used on the official 7in) a "panel-dpi" compatible is there already

		panel_generic: __overlay__  {
			compatible = "panel-dpi";

			width-mm = <154>;
			height-mm = <83>;
			bus-format = <0x1009>;

			timing: panel-timing {
				clock-frequency = <29500000>;
				hactive = <800>;
				hfront-porch = <24>;
				hsync-len = <72>;
				hback-porch = <96>;
				hsync-active = <1>;
				vactive = <480>;
				vfront-porch = <3>;
				vsync-len = <10>;
				vback-porch = <7>;
				vsync-active = <1>;

				de-active = <1>;
				pixelclk-active = <1>;
			};
		};

for a MIPI-to-LVDS bridge one needs to enable 'Generic LVDS panel driver' in order to use "panel-lvds" compatible from DT
i.e.

		panel: panel {
				compatible = "panel-lvds";

				backlight = <&backlight_lvds>;

				/* Physical dimensions of active area */
				width-mm = <217>;
				height-mm = <136>;

				data-mapping = "vesa-24";

				panel-timing {
					clock-frequency = <71100000>;
					hactive = <1280>;
					hfront-porch = <160>;	
					hsync-len = <48>;
					hback-porch = <48>;
					
					vactive = <800>;
					vfront-porch = <23>;
					vsync-len = <6>;					
					vback-porch = <8>;
				};

Bridge input timing will have to be calculated by the driver based on the required output (in most cases) and other parameters (i.e. no data-lanes can be configured via overrides).

so, what is the real target here? Having the possibility for adding 'dumb panels' (panels which just require a suitable timing on the DSI interface)? But that would not be in-line with

raspberrypi/firmware#1819
I'm currently working on product for Adafruit that would support different panels via a bridge IC like it is used in the official touchscreen.

Would be glad if you can share some more details so others might be able to support.

[timonsku]

@aBUGSworstnightmare-rpi
There are multiple things where this would be needed. The use case mentioned in the firmware issue is one of them but does not really relate to the IC itself, that could be any bridge really but yes its a DSI to DPI bridge IC in that instance but might also change down the line which one specifically or new products with different bridges being added where its desirable to keep software compatibility so that the process for the end user looks the same, so the bootstrapping would happen externally and the requirement to the host would only be to send a suitable DSI stream.
A fixed port configuration would not be a great solution as different applications have different needs in terms of DSI bus flags.

"Native" panels without a bridge IC are also a target. The wish is really only to be able to define what one can define in a panel_desc_dsi but doing that at runtime without needing to get a patch merged or distribute a kernel fork.
There are potentially dozens or even hundreds of different panels in the future and defining them all in the driver is just not sustainable, again exact same motivation as with the original pane-dpi patch that enabled DT timings but wanting control over the DSI bits too.

[aBUGSworstnightmare-rpi]

"Native" panels without a bridge IC are also a target.

Hmm .. still don't get that!
A 'native' DSI for me is a command mode display. Find a typical power-on-sequence example from the image linked here (https://drive.google.com/file/d/1Vi3UZlcxU89LIjbEj2nI8zCIRfpN5BTa/view?usp=share_link)

You can't fire this display up with a panel-simple-dsi compatible; you will have to use panel-dpi-cm. That's pretty much the same for all native DSI displays which autoload TCON register settings from NVM; one still has to send 0x51/0x53/0x55 before you can switch the display on (0x29) and exit sleep mode (0x11) for normal operation.
Looking at the sequence, a 'dumb' display simply needs 'Host Display Data transfer'

[6by9]

Native DSI can be either command mode or video mode.
I've never encountered a command mode panel in reality and have no idea how well (or not) they'll behave. When someone actually finds one, I'll worry about it.

Whilst I wouldn't advocate it, the Waveshare DSI panels could be considered native DSI panels because the MCU handles all the initialisation. The regulator and backlight side could be handled by a regulator driver similar to rpi_panel_attiny_regulator, which leaves the panel side as a simple DSI device. That is a valid use case in my book.

I would advocate that bridge chips are configured from Linux rather than hidden behind an MCU. At least that way there is visibility over what the hardware is actually doing, and if a compatibility issue is observed with a future chip then it can be addressed. Most bridge chip drivers should be configured automatically by the upstream panel device, so other than a bit of DT it adds no overhead.

[timonsku]

@aBUGSworstnightmare-rpi see the panels in the dsi_of_match array for examples of DSI video mode panels that do no require initialization.
And what 6by9 described, sometimes its useful to init externally for either systems reasons or because the init sequence is inexplicably under NDA. In those cases you can turn them into simple panels.

@6by9 generally agree with the sentiment about bridges! It gets a bit more complicated in some specific dev board style applications as I found out as the needs break with some assumptions in existing drivers or guidelines of how drivers should behave. That is a very niche thing though where I feel re-using generic infrastructure might be the nicer approach.

[ladyada]

(subscribing myself by commenting)

[pelwell]

That works - there's also a Subscribe button in the controls on the right at the top of the page (it probably says Unsubscribe now).

[pelwell]

Can you stop force-pushing your branch unless you are actively changing your code? GitHub is pretty good at rebasing when we want to merge, and every update invalidates the previous build check results.

[timonsku]

sorry, there were commits that were not supposed to go into this branch that I wanted to roll back. Not going to happen again.

[arturo182]

Just wanted to add that I tested this PR on the CM4 with a custom carrier I'm working on, and it works great! I no longer have to patch panel-simple.c to add my custom panel :)

[ladyada]

@pelwell ive also had much success with fun displays connecting through the ICN6211 (with a microcontroller doing the i2c/spi configuration during power-up) - can you approve the workflow so it could get reviewed before the next kernel release is made? :)

[pelwell]

Is this actually ready? I see that the Draft status has been removed (GitHub doesn't give notifications about that, so a comment is always a good idea), but some of the comments on the PR have been acknowledged but not acted on.

[6by9]

Sorry, still unaddressed issues.

There needs to be 2 commits - one device tree binding, and one driver change. The third "fixup" patch touches both binding and driver.

Checkpatch complaint as there is no commit description.

How about a 3rd commit that adds an overlay using this, along similar lines to https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/vc4-kms-dpi-generic-overlay.dts or vc4-kms-dpi-panel-overlay.dts

For those adding panels using this driver change, I would like to see it done in a similar way to https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/vc4-kms-dpi-panel-overlay.dts, where there is one overlay that uses an override to select the relevant panel. The timing and config can be put in that overlay.

[6by9]

Still present

[timonsku]

Thanks for the input @6by9 !
Regarding splitting binding and driver patch. I thought the initial commit of a new binding should be split into its own commit (which I did).
So what you are saying I should always keep changes to bindings completely separated too? Should I then revert the latest (third) commit and split it into two (making a total of 4 excluding upcoming fixes for the other things you addressed)

For the requested overlay, you mean this as a practical example for how to use this?

[6by9]

Thanks for the input @6by9 ! Regarding splitting binding and driver patch. I thought the initial commit of a new binding should be split into its own commit (which I did). So what you are saying I should always keep changes to bindings completely separated too? Should I then revert the latest (third) commit and split it into two (making a total of 4 excluding upcoming fixes for the other things you addressed)

With a very few exceptions, individual commits should never cross subsystem boundaries. For mainline it causes grief over which tree it should be committed through.

We also don't want your development history here - you've been asked to make changes as the original driver has issues, therefore please amend the existing commits as they've gone nowhere except your local branch.

For the requested overlay, you mean this as a practical example for how to use this?

Yes please.

[timonsku]

Is this actually ready? I see that the Draft status has been removed (GitHub doesn't give notifications about that, so a comment is always a good idea), but some of the comments on the PR have been acknowledged but not acted on.

Sorry I just realized that my added comments where part of making a review and did not show up publicly as I hadn't submitted the review until now.

[timonsku]

Sorry for the force push but not aware of any other way to make a clean history rewrite/amend.
I think I covered all the outstanding change requests. A third commit added for an example overlay.

[6by9]

Force pushing is the correct way to update the branch backing a PR.

Thank you - the general implementation is far cleaner.

A number of checkpatch errors.

Please give a full description in the commit text. Titles should always start with the subsystem (do git log on the file if in doubt, and copy all the other commits).

I was hoping that the overlay would actually be compiled and useful, with similar overrides to https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/vc4-kms-dpi-generic-overlay.dts

[timonsku]

Ah yes sorry about the missing description you mentioned that before, will run checkpatch on the actual patch. I only ran it on the source files.

@ Overlay, my bad I understood that way different. More as a template for new overlays, not to be used as is.
But yes that makes sense, so the intention is full configuration with overrides so that panels don't even necessarily have to have an overlay installed? (e.g. same function as the dpi overlay)

[timonsku]

@6by9 do you have a suggestion for how to represent the mode stringarray with overrides?
As I understand there is currently no direct support for array types in the overrides syntax and I'm not aware of any devicetree "native" way of appending values to an array with overlays (in which case I could use fragments).
The only way I currently see is having a fragment for every possible combination but that is not practical.

[pelwell]

We can do arrays of cells, but not arrays of strings. Exactly what change do you want the overlay to make?

[6by9]

@6by9 do you have a suggestion for how to represent the mode stringarray with overrides? As I understand there is currently no direct support for array types in the overrides syntax and I'm not aware of any devicetree "native" way of appending values to an array with overlays (in which case I could use fragments). The only way I currently see is having a fragment for every possible combination but that is not practical.

• Seeing as we don't really support command mode (I've never seen a command mode panel in the flesh), you can assume MODE_VIDEO is set. Command mode panels would be supported through panel-dsi-cm anyway, and not panel-simple.
• MODE_LPM is redundant as this is the simple panel-dsi that sends no commands.
• HS_PKT_END_ALIGNED is a Mediatek special.
• And actually the vc4 driver currently doesn't look at the other flags, so all of them can be ignored.

So just set MODE_VIDEO and have done with it.

If we add support for the extra flags, then a vc4-kms-dsi-panel-overlay.dts overlay would allow those who wish to define their panel by name can do support that level of configuration, but the generic overlay won't support it. Overlays don't go upstream, so it's not an issue for that.

[timonsku]

That's fair enough, good with me. I don't see a big need for more than video_mode either. Most other options are usually just optimizations anyway.

[timonsku]

@6by9 Please see updated commits. checkpatch should be happy except for license header which does not seem to be wanted for overlays.
I enabled the I2C interface in the overlay as well, while nothing technically needs it it seems sensible for a generic overlay to allow access in case there is anything attached that you might want to talk to from userspace.

The only thing unaddressed is the location of the data-lanes property. See my comments on the last review.
If you feel like things should move then I'll revise that.
If not I think things should hopefully have no more form issues. Thanks for bearing with me there.

[6by9]

I enabled the I2C interface in the overlay as well, while nothing technically needs it it seems sensible for a generic overlay to allow access in case there is anything attached that you might want to talk to from userspace.

If you don't need it in the overlay, don't enable it.

i2c0mux will claim GPIOs 0&1 which may be used for other purposes. Your overlay would be incompatible with that for no good reason.
There's already dtparam=i2c_vc=on to enable it via a 1-liner, or you can use dtoverlay=i2c0,pins44_45 if you want just the single I2C port with no mux.

The only thing unaddressed is the location of the data-lanes property. See my comments on the last review. If you feel like things should move then I'll revise that. If not I think things should hopefully have no more form issues. Thanks for bearing with me there.

data-lanes is a standard property, and it goes in the endpoint as covered by https://www.kernel.org/doc/Documentation/devicetree/bindings/media/video-interfaces.yaml

If you have a bridge or similar DSI device, then you have an input port and an output port, each with their embedded endpoint. The number of lanes is property of the port/endpoint, not the overall device.
Whilst you only have 1 port, the property remains under the endpoint for consistency.

[timonsku]

i2c0mux will claim GPIOs 0&1 which may be used for other purposes. Your overlay would be incompatible with that for no good reason. There's already dtparam=i2c_vc=on to enable it via a 1-liner, or you can use dtoverlay=i2c0,pins44_45 if you want just the single I2C port with no mux.

That is a fair point, will revert.

If you have a bridge or similar DSI device, then you have an input port and an output port, each with their embedded endpoint. The number of lanes is property of the port/endpoint, not the overall device. Whilst you only have 1 port, the property remains under the endpoint for consistency.

Okay yea, the disconnect to the other bus properties feels a bit odd but fair enough if there is precedence for having lanes defined in the endpoint. Will revise.

[timonsku]

Pushed the change. I'm a bit uncertain if the bindings are defined correctly this way but given that there should only ever be a single endpoint this feels like the correct approach to me.
I'm relying on the address for port and endpoint to default to 0, afaik they don't need to/shouldn't be defined specifically if the expectation is that there can't be multiple instances?

[6by9]

All overlays must have an entry in the README file documenting the overlay and the associated overrides. In this case most of it can be copied from vc4-kms-dpi-generic.

Icing on the cake would have been to include configuration of a backlight node (as vc4-kms-dpi-generic does), but I'm not going to worry about that one now.

[6by9]

Not needed as this is the default already present in the panel definition.

[pelwell]

I'm OK with having parameters that select values that are also the defaults. It can take some of the guesswork out of reading a config file.

[timonsku]

That was sort of my thinking, a lot of people who would use the generic overlay might never see the dts file and just follow a guide. Being explicit in the parameters seems like it would avoid potential confusion.

[timonsku]

I'm with you for the backlight but I'm not sure what the best options for that are. Unlike for dpi you are not guaranteed to have a connection to the 40pin header and you might not wanna touch that with a default if you don't know if there isn't likely some form of HAT on there.
With Pi5 we now also have these two AUX IO but I'm not sure what your plans are for that and if they support PWM?
Happy to do a second PR later on when got hardware that could test that.

Have pushed the requested changes except for leaving RGB888 in as an option. If there is disagreement let me know.

[6by9]

Makefile and weird rebase fixes required.

For vc4-kms-dpi-generic the backlight is disabled by default, so you could have done the same here. If the panel implementation has suitable backlight control then the wiring is sorted via overrides.
(For those designing hardware, an I2C controlled backlight makes most sense - TI look to have a number of options in the LP855x range).
Leave it for now - this has rumbled on long enough.

[timonsku]

oh yea sorry, divergent feature branch, looks clean to me now. Makefile fixed.

And yea agree, I2C backlight probably the best for now and what we've been using.

[6by9]

Pi5+ ?

[timonsku]

As in for Pi5 and "going forward" assuming that a Pi6 would keep with the 4 channels.

[6by9]

Nothing obviously wrong other than the Pi5+ comment.

CI pipeline running, so will just check whether that picks anything up, but otherwise I think I'm happy. Thanks for following through on this.

[timonsku]

Glad to hear and thanks for the guidance!
Should I remove the comment for 3 and 4 channel option before you pull it in?

E: Hm why is the dtoverlaycheck unhappy about the Readme? Don't quite understand the error output.

[6by9]

dtoverlaycheck ensures:

• README is in alphabetical order. vc4-kms-dsi-generic doesn't come between vc4-kms-dpi-generic and vc4-kms-dpi-hyperpixel2r
• All parameters are documented, and all documented parameters exist. The overlay uses XXX_lane whilst the documentation says XXX-lane. Hyphen vs underscore. Hyphens are preferred.

If you could update the overlay to use hyphens, correct the alphabetical order, and drop the erroneous + from 5B+, then I think we're there.

[timonsku]

oh yea, thats what I get for typing it out..
dsi0 was indeed also not in the readme. Pushed fix.

[pelwell]

Yay - thanks for persevering and getting this over the line.