[PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Marek Vasut]

On 10/21/24 1:48 PM, Maxime Ripard wrote:
> On Sun, Oct 20, 2024 at 04:49:29AM +0200, Marek Vasut wrote:
>> On 10/19/24 11:49 PM, Kieran Bingham wrote:
>>> Quoting Marek Vasut (2024-10-12 21:37:59)
>>>> On 10/11/24 5:10 AM, Liu Ying wrote:
>>>>
>>>> Hi,
>>>>
>>>>>>>> This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is still in the imx8mp.dtsi . Therefore, to make your panel work at the correct desired pixel clock frequency instead of some random one inherited from imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the problem and is the correct fix:
>>>>>>>>
>>>>>>>> &media_blk_ctrl {
>>>>>>>>        // 506800000 = 72400000 * 7 (for single-link LVDS, this is enough)
>>>>>>>>        // there is no need to multiply the clock by * 2
>>>>>>>>        assigned-clock-rates = <500000000>, <200000000>, <0>, <0>, <500000000>, <506800000>;
>>>>>>>
>>>>>>> This assigns "video_pll1" clock rate to 506.8MHz which is currently not
>>>>>>> listed in imx_pll1443x_tbl[].
>>>>>>
>>>>>> Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those should produce accurate clock.
>>>>>
>>>>> Ack.
>>>>>
>>>>>>
>>>>>>> Does the below patch[1] fix the regression issue? It explicitly sets
>>>>>>> the clock frequency of the panel timing to 74.25MHz.
>>>>>>>
>>>>>>> [1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1
>>>>>> That patch is wrong, there is an existing entry for this panel in panel-simple.c which is correct and precise, please do not add that kind of imprecise duplicate timings into DT.
>>>>>
>>>>> At least the patch[1] is legitimate now to override the display
>>>>> timing of the panel because the override mode is something
>>>>> panel-simple.c supports.
>>>>
>>>> It may be possible to override the mode, but why would this be the
>>>> desired if the panel-simple.c already contains valid accurate timings
>>>> for this particular panel ?
>>>
>>> I'm confused a little here. Why is it that setting the panel timings in
>>> the DT as per [1] make the display work, while the panel timeings in
>>> panel-simple alone are not enough?
>>>
>>> Is there some difference in code path for 'how' the panel timings are
>>> set as to whether they will apply fully or not ?
>> Because [1] sets inaccurate pixel clock of 74.25 MHz, which can be divided
>> down from random default Video PLL setting of 1039.5 MHz set in imx8mp.dtsi
>> media_blk_ctrl , 1039.5 / 74.25 = 14 .
>>
>> The panel-simple pixel clock are 72.4 MHz, to achieve that accurately, it is
>> necessary to reconfigure the Video PLL frequency to 506.8 MHz , which
>> LCDIFv3 can do, but LDB can not, hence it has to be done in DT for now.
> 
> That the clock driver is broken and thus should be fixed through the DT is a
> bit backward, don't you think?

See these two patches, they might address that part for next cycle:

clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate
drm: bridge: ldb: Configure LDB clock in .mode_set

For this cycle, fixing up the frequency that is already incorrectly set 
in imx8mp.dtsi in board DT is the least impact approach, see

arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz

> AFAIU, the clock can't reach the ideal pixel clock panel-simple has. Do
> you have the datasheet for that panel?

No

> If so, using display_timings and shortening/extending the blanking
> timings to match the clock that can be provided seems like a more robust
> solution.
The PLL has to be configured correctly, see the two patches listed above.