linux-kernel - Re: [PATCH] drm: bridge: fsl-ldb: fixup mode on freq mismatch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <897b3787-8246-4509-94a1-129488297150@denx.de>
Date: Sat, 7 Dec 2024 12:30:24 +0100
From: Marek Vasut <marex@...x.de>
To: Nikolaus Voss <nv@...n.de>
Cc: Liu Ying <victor.liu@....nxp.com>,
 Alexander Stein <alexander.stein@...tq-group.com>,
 Liu Ying <victor.liu@....com>, Luca Ceresoli <luca.ceresoli@...tlin.com>,
 Fabio Estevam <festevam@...x.de>, Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 nikolaus.voss@...g-streit.com, miquel.raynal@...tlin.com
Subject: Re: [PATCH] drm: bridge: fsl-ldb: fixup mode on freq mismatch

On 12/4/24 11:55 AM, Nikolaus Voss wrote:
> Hi Marek,

Hi,

>>>>>>> I doubt that pixel clock tree cannot find appropriate division 
>>>>>>> ratios
>>>>>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>>>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster 
>>>>>>> than
>>>>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>>>>>> folder between "ldb" clock and pixel clock can be met. That means 
>>>>>>> the
>>>>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>>>>
>>>>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and 
>>>>>>> pixel
>>>>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>>>>
>>>>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>>>>>> frequency to 506.8 MHz")
>>>>>>
>>>>>> I probably could. The point of my patch is you don't have to know in
>>>>>> advance which LVDS panel is connected, and you don't have to 
>>>>>> calculate
>>>>>> the base PLL clock by hand and store it in the device tree.
>>>>>>
>>>>>> In my test system, I have three different LVDS panels with EDID 
>>>>>> EEPROM,
>>>>>> none of which worked with the stock driver, but all work with this
>>>>>> patch.
>>>>>> With slightly adapted pixel clocks though.
>>>>>
>>>>> If each of the three LVDS panels has only one display mode, you may
>>>>> assign the PLL clock rates in DT overlays for the panels.
>>>> I temporarily agree.
>>>>
>>>> I also currently use DTOs for various panels including their PLL
>>>> setting, but in the end, I think/hope the work of Miquel and co. is
>>>> going to make that PLL setting part unnecessary.
>>>
>>> That is exactly what my patch is about. I want to use one DT for all
>>> panels
>>
>> Right
>>
>>> and store the panel's timing in EDID EEPROM.
>> Oh, that is a new one. Does the EDID EEPROM store the entirety of
>> 'struct display_timing {}' somehow , or is that a custom format ?
> 
> Well, sort of ;-). VESA has taken care of this 30 years ago
> (https://en.wikipedia.org/wiki/Extended_Display_Identification_Data).
> 
> DRM handles this with drm_get_edid() and siblings, e.g. :

EDID can not encode all the information in struct display_timing {} , or 
can it ?

I think what you would be missing are bus_flags , bus_format and 
possibly the single/dual link and channel (odd/even) mapping, won't you ?

> @@ -86,16 +92,36 @@ static int panel_lvds_get_modes(struct drm_panel 
> *panel,
>   {
>          struct panel_lvds *lvds = to_panel_lvds(panel);
>          struct drm_display_mode *mode;
> +       int num = 0;
> +
> +       /* probe EDID if a DDC bus is available */
> +       if (lvds->ddc) {
> +               pm_runtime_get_sync(lvds->dev);
> +
> +               if (!lvds->edid)
> +                       lvds->edid = drm_get_edid(connector, lvds->ddc);
> +
> +               if (lvds->edid)
> +                       num += drm_add_edid_modes(connector, lvds->edid);
> +
> +               pm_runtime_mark_last_busy(lvds->dev);
> +               pm_runtime_put_autosuspend(lvds->dev);
> +       }
> 
> panel-simple.c does that in mainline, I added it to panel-lvds.c.
> The kernel subdir tools/edid has some code to generate the EEPROM data
> from timings and flags.
> 
> We keep the DDC EEPROM on a small adapter glued to to back of the panel
> so we can replace the usually short-lived panel with a successor.
OK