[Letux-kernel] [PATCH 00/20] A bunch of JZ4730 fixups for letux-kernel

[H. Nikolaus Schaller]

Hi Paul,

> Am 11.01.2021 um 22:59 schrieb Paul Boddie <paul at boddie.org.uk>:
> 
> On Monday, 11 January 2021 21:35:03 CET H. Nikolaus Schaller wrote:
>> Am 11.01.2021 um 19:50 schrieb Paul Boddie <paul at boddie.org.uk>:
>>> 
>>> I don't think I ever found out what the "correct" solution was for this.
>> 
>> Well, for drivers which have a ".compatible" entry they can apparently set
>> it in their get_modes function. The panel_simple driver is some mixture.
>> If the panel is explicitly listed in the tables, there is a bus format.
>> 
>> But the fallback was forgotten to somehow provide one. The same is for
>> the connector-type. IMHO both should also be defined in the DT.
> 
> It is incredible how complicated this is for what is effectively a table of 
> values associated with the panel itself. I seem to remember investigating what 
> device tree did support, and I think the following is still relevant:
> 
> ----
> 
> I now see that there is code in the driver to read timings for non-DPI panels 
> (panel_simple_parse_panel_timing_node). However, I don't see a way of 
> specifying a generic "panel-simple" and have the driver activated on that 
> basis. None of the Chunghwa panels in the file are comparable to the one used 
> by the Mipsbook.
> 
> The panel_dpi_probe function reads the timings plus width and height from the 
> device tree. In contrast, panel_simple_parse_panel_timing_node only reads the 
> timings. Both functions supposedly calculate the appropriate bus flags. I have 
> no idea at the moment how the appropriate bus format is obtained. For this 
> panel, we need to end up with MEDIA_BUS_FMT_RGB565_1X16, I think.
> 
> ----

Yes, exactl what I did :)

I have asked the lists (maybe I should have asked Laurent Pinchard as well).
In the worst case they say that a full DT support is missing. Then we can
still explicitly add the panel to the panel-simple table.

> 
> [...]
> 
>> IMHO doing it where I have hacked the panel_simple is indeed the right
>> place - because the table based initialization does it there as well -
>> but it should not be a constant in code.
> 
> I should take a look at this, but I now cannot find the details of it in your 
> messages.

I have placed

	desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;

in panel_dpi_probe(). This is where all desc components are set up from DT - except
the bus format. Or in other words it must go to 

> 
> [...]
> 
>> I have checked what is going on and there is another discrepancy. In
>> ingenic_drm_plane_config() the fourcc is DRM_FORMAT_RGB888 (no idea where
>> this comes from - not from the panel driver where I define
>> MEDIA_BUS_FMT_RGB565_1X16) which sets the BPP field to 0x05. The jz4730
>> does not permit such a value for this register. Only the jz4740 and later
>> SoC version. Otherwise they are almost compatible, at least I have not seen
>> another incompatibility.
> 
> The JZ4730 supports at most 16bpp displays.

Yes, and that is the reason why the jz4740-lcd does not work. It tries to
create an 18bpp setup...

> There may well be other 
> differences, but I haven't got round to documenting the LCD controller in the 
> JZ4730 just yet.
> 
>> The other issue (which may be the reason for the atomic timeout) is that
>> LCDCMD0 seems not to be initialized properly. Therefore the DMA probably
>> gets stuck and no SOF/EOF interrupts occur.
> 
> There should be a descriptor set up which then populates LCDCMD0 when the DMA 
> is activated. It is interesting that the LCD controller supports descriptor-
> based transfers whereas the general DMA controller in the JZ4730 does not.

Well, usually such SoC are glued together from separate VHDL IP building blocks.
So they may have had one for the LCDC with descriptors and a different for the
general DMA.

> 
> (I have been investigating the DMA controller and have demonstrated it working 
> under L4Re, so I will now try and apply the lessons to the Linux driver.)

That will be great!

> 
>> At the moment I wouldn't mind if the framebuffer is not displayed properly
>> (e.g. shows false colors, distorted or whatever) but the LCDC should scan
>> the panel from the framebuffer (something should blink or change if I type)
>> and there should be no warnings and errors during boot...
>> 
>> It seems as if we are close to get this hacked... Then we can look for a
>> general solution that does not disturb e.g. jz4780.
> 
> Indeed.
> 
>> PS: I get an idea where the wrong DRM_FORMAT_RGB888 comes from. There is
>> a new format table since v5.11-rc1 which defines the formats every Ingenic
>> SoC understands. And I have taken the same table as for the jz4740.
>> If user-space or the /dev/fb0 driver can access this table it assumes
>> that the jz4730 also understands DRM_FORMAT_XRGB8888. I'll try to add
>> a specific jz4730 format table later...
> 
> The JZ4730 supports 16bpp as RGB565 and, maybe only for passive displays, 
> RGB555 (with the top bit unused). Something comparable to these are defined in 
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c alongside a 32bpp format:
> 
> static const u32 ingenic_drm_primary_formats[] = {
>        DRM_FORMAT_XRGB1555,
>        DRM_FORMAT_RGB565,
>        DRM_FORMAT_XRGB8888,
> };

This table has been split up into SoC specific ones by an upstream patch
in 5.11-rc1. It now looks like:

https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/gpu/drm/ingenic/ingenic-drm-drv.c#L1169

I have added the following table and made it active by compatible = ingenic,jz4730-lcd:

static const u32 jz4730_formats[] = {
	DRM_FORMAT_XRGB1555,
	DRM_FORMAT_RGB565,
};

I don't know if the DRM_FORMAT_C8, could be useful.

So my latest status is:
* I had to add two NULL pointer issues in cleanup code
* now I get 

[    0.372035] ingenic-drm 13050000.lcd: [drm] requested bpp 32, scaled depth down to 16
[    0.452211] ingenic-drm 13050000.lcd: [drm] *ERROR* fbdev: Failed to setup generic emulation (ret=-22)

I could pinpoint this to drm_mode_legacy_fb_format()

https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/gpu/drm/drm_fourcc.c#L46

There is no case for bpp=32 and depth=16.

I just want to try to add a case 16: fmt = DRM_FORMAT_RGB565; and let's see...

BR,
Nikolaus