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

[Marek Vasut <marex@denx.de>]

On 10/9/24 11:55 AM, Isaac Scott wrote:
> On Tue, 2024-10-08 at 23:48 +0200, Marek Vasut wrote:
>> On 10/8/24 12:07 PM, Isaac Scott wrote:
>>> On Mon, 2024-10-07 at 20:06 +0200, Marek Vasut wrote:
>>>> On 10/7/24 7:01 PM, Isaac Scott wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote:
>>>>>> On 6/24/24 11:19 AM, Alexander Stein wrote:
>>>>>>> Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek
>>>>>>> Vasut:
>>>>>>>> In case an upstream bridge modified the required clock
>>>>>>>> frequency
>>>>>>>> in its .atomic_check callback by setting
>>>>>>>> adjusted_mode.clock
>>>>>>>> ,
>>>>>>>> make sure that clock frequency is generated by the
>>>>>>>> LCDIFv3
>>>>>>>> block.
>>>>>>>>
>>>>>>>> This is useful e.g. when LCDIFv3 feeds DSIM which feeds
>>>>>>>> TC358767
>>>>>>>> with (e)DP output, where the TC358767 expects precise
>>>>>>>> timing
>>>>>>>> on
>>>>>>>> its input side, the precise timing must be generated by
>>>>>>>> the
>>>>>>>> LCDIF.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>
>>>>>>> With the other rc358767 patches in place, this does the
>>>>>>> trick.
>>>>>>> Reviewed-by: Alexander Stein
>>>>>>> <alexander.stein@ew.tq-group.com>
>>>>>>
>>>>>> I'll pick this up next week if there is no objection.
>>>>>
>>>>> Unfortunately, this has caused a regression that is present in
>>>>> v6.12-
>>>>> rc1 on the i.MX8MP PHYTEC Pollux using the
>>>>> arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts.
>>>>> The
>>>>> display is the edt,etml1010g3dra panel, as per the upstream
>>>>> dts. We
>>>>> bisected to this commit, and reverting this change fixed the
>>>>> screen.
>>>>>
>>>>> We then tried to retest this on top of v6.12-rc2, and found we
>>>>> also
>>>>> had
>>>>> to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155
>>>>> ("clk:
>>>>> imx:
>>>>> clk-imx8mp: Allow media_disp pixel clock reconfigure parent
>>>>> rate")
>>>>> alongside this. Reverting these two commits makes the display
>>>>> work
>>>>> again at -rc2.
>>>>>
>>>>> Do you have any suggestions on anything we might be missing on
>>>>> our
>>>>> end?
>>>>> Please let me know if there's anything you'd like me to test as
>>>>> I'm
>>>>> not
>>>>> sure what the underlying fault was here.
>>>> I believe what is going on is that the LCDIF cannot configure its
>>>> upstream clock because something else is already using those
>>>> clock
>>>> and
>>>> it set those clock to a specific frequency. LCDIF is now trying
>>>> to
>>>> configure those clock to match the LVDS panel, and it fails, so
>>>> it
>>>> tries
>>>> to set some approximate clock and that is not good enough for the
>>>> LVDS
>>>> panel.
>>>>
>>>> Can you share dump of /sys/kernel/debug/clk/clk_summary on
>>>> failing
>>>> and
>>>> working system ? You might see the difference around the "video"
>>>> clock.
>>>>
>>>> (I have seen this behavior before, the fix was usually a matter
>>>> of
>>>> moving one of the LCDIFs to another upstream clock like PLL3, so
>>>> it
>>>> can
>>>> pick well matching output clock instead of some horrid
>>>> approximation
>>>> which then drives the panel likely out of specification)
>>>
>>> Hi Marek,
>>>
>>> Please find attached the clk_summary for v6.12-rc2 before and after
>>> the
>>> reversion (the one after the reversion is 6.12-
>>> rc2_summary_postfix).
>> Thank you, this helped greatly.
>>
>> I believe I know why it used to kind-of work for you, but I'm afraid
>> this used to work by sheer chance and it does not really work
>> correctly
>> for the panel you use, even if the panel likely does show the correct
>> content. But, there is a way to make it work properly for the panel
>> you use.
>>
>> First of all, the pixel clock never really matched the panel-simple.c
>> pixel clock for the edt_etml1010g3dra_timing:
>>
>> $ grep '\<media_disp2_pix\>' 6.12-rc2_summary_postfix
>>     media_disp2_pix  1  1  0  74250000 ...
>>                               ^^^^^^^^
>>
>> $ grep -A 1 edt_etml1010g3dra_timing drivers/gpu/drm/panel/panel-
>> simple.c
>> static const struct display_timing edt_etml1010g3dra_timing = {
>>           .pixelclock = { 66300000, 72400000, 78900000 },
>>                                     ^^^^^^^^
>>
>> The pixel clock are within tolerance, but there is a discrepancy
>> 74250000 != 72400000 .
>>
>> Since commit 94e6197dadc9 ("arm64: dts: imx8mp: Add LCDIF2 & LDB
>> nodes")
>> the IMX8MP_VIDEO_PLL1_OUT is set to a very specific frequency of
>> 1039500000 Hz, which tidily divides by 2 to 519750000 Hz (which is
>> your
>> LVDS serializer frequency) and divides by 7 to 74250000 Hz which is
>> your
>> LCDIF pixel clock.
>>
>> 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>;
>> };
>>
>> Can you please test whether this works and the pixel clock are
>> accurate
>> in /sys/kernel/debug/clk/clk_summary ?
> 
> Interestingly, after making the change you suggested to imx8mp-
> phyboard-pollux-rdk.dts before the two reversions, the display now
> seems to work. Please see below for the relevant section of the new
> clk_summary referring to media_disp2_pix:
> 
> video_pll1_ref_sel               1       1        0        24000000
> 0          0     50000      Y      deviceless
> no_connection_id
>         video_pll1                    1       1        0
> 506800000   0          0     50000      Y         deviceless
> no_connection_
>            video_pll1_bypass          1       1        0
> 506800000   0          0     50000      Y            deviceless
> no_connecti
>               video_pll1_out          2       2        0
> 506800000   0          0     50000      Y               deviceless
> no_conne
>                  media_ldb            1       1        0
> 506800000   0          0     50000      Y                  deviceless
> no_co
>                     media_ldb_root_clk 1       1        0
> 506800000   0          0     50000      Y
> 32ec0000.blk-ctrl:bridge@5c     l
>                                                                        
> deviceless                      no
>                  media_disp2_pix      1       1        0        72400000
> 0          0     50000      Y                  deviceless
> no_co
>                     media_disp2_pix_root_clk 1       1        0
> 72400000    0          0     50000      Y
> 32e90000.display-controller
>                                                                        
> 32ec0000.blk-ctrl               di
>                                                                        
> deviceless                      no
>                  media_disp1_pix      0       0        0
> 506800000   0          0     50000      N                  deviceless
> no_co
>                     media_disp1_pix_root_clk 0       0        0
> 506800000   0          0     50000      N
> 32ec0000.blk-ctrl
>                                                                        
> deviceless                      no
>                  media_mipi_phy1_ref  0       0        0        23036364
> 0          0     50000      N                  deviceless
> no_co
>                     media_mipi_phy1_ref_root 0       0        0
> 23036364    0          0     50000      N
> 32ec0000.blk-ctrl
> 
> The media_disp2_pix clock now seems to be correct at 724000000 after
> your changes.

Do you want to submit the DT patch with correct Fixes: tag ? :)