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

[Nikolaus Voss]

Hi Marek,

On 07.12.2024 12:30, Marek Vasut wrote:
> On 12/4/24 11:55 AM, Nikolaus Voss 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. :
>
> 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 ?

Yes, that's right. I use the vendor block for bus_flags and bus_format
now, but that's not standard and not portable of course.

My first idea was to store the DT overlay in the display EEPROM but
a standard 1k EEPROM is too small for that.

--
Nikolaus Voss