On 10/09/2024, 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 <ma...@denx.de>
>>>>>>
>>>>>> With the other rc358767 patches in place, this does the trick.
>>>>>> Reviewed-by: Alexander Stein <alexander.st...@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>;
This assigns "video_pll1" clock rate to 506.8MHz which is currently not
listed in imx_pll1443x_tbl[].
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
> };
>
> Can you please test whether this works and the pixel clock are accurate in
> /sys/kernel/debug/clk/clk_summary ?
>
> Now ... as for the LVDS serializer clock ... that is more complicated.
>
> The LCDIF driver does its .atomic_enable first , configures the pixel clock
> (and the Video PLL now) and enables the clock. The LVDS LDB serializer driver
> does its .atomic_enable second and cannot reconfigure the clock anymore, the
> Video PLL is stuck at /7 rate set by LCDIF driver and won't budge because the
> clock are already enabled. I'm currently trying to figure out if this can be
> improved somehow, but I believe that would be material for next release.
>
--
Regards,
Liu Ying
