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

[Nikolaus Voss <nv@vosn.de>]

Hi Marek,

On 04.12.2024 00:40, Marek Vasut wrote:
> On 12/3/24 8:20 AM, Nikolaus Voss wrote:
>> On 03.12.2024 04:12, Marek Vasut wrote:
>>> On 12/3/24 3:22 AM, Liu Ying wrote:
>>> 
>>> [...]
>>> 
>>>>>> 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. :

@@ -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.

-- 
Nikolaus Voss