[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c5ab63da-21ec-4c0d-8ecc-3745943d806f@denx.de>
Date: Thu, 21 Nov 2024 03:45:57 +0100
From: Marek Vasut <marex@...x.de>
To: Ying Liu <victor.liu@....com>, "imx@...ts.linux.dev"
<imx@...ts.linux.dev>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Cc: "shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>, "robh@...nel.org"
<robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will@...nel.org" <will@...nel.org>,
"abelvesa@...nel.org" <abelvesa@...nel.org>, Peng Fan <peng.fan@....com>,
"mturquette@...libre.com" <mturquette@...libre.com>,
"sboyd@...nel.org" <sboyd@...nel.org>,
"andrzej.hajda@...el.com" <andrzej.hajda@...el.com>,
"neil.armstrong@...aro.org" <neil.armstrong@...aro.org>,
"rfoss@...nel.org" <rfoss@...nel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
"jonas@...boo.se" <jonas@...boo.se>,
"jernej.skrabec@...il.com" <jernej.skrabec@...il.com>,
"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
"mripard@...nel.org" <mripard@...nel.org>,
"tzimmermann@...e.de" <tzimmermann@...e.de>,
"airlied@...il.com" <airlied@...il.com>, "simona@...ll.ch"
<simona@...ll.ch>, "quic_bjorande@...cinc.com" <quic_bjorande@...cinc.com>,
"geert+renesas@...der.be" <geert+renesas@...der.be>,
"dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>,
"arnd@...db.de" <arnd@...db.de>,
"nfraprado@...labora.com" <nfraprado@...labora.com>,
Luca Ceresoli <luca.ceresoli@...tlin.com>,
Miquel Raynal <miquel.raynal@...tlin.com>
Subject: Re: [PATCH v7 2/7] Revert "clk: imx: clk-imx8mp: Allow media_disp
pixel clock reconfigure parent rate"
On 11/20/24 7:38 AM, Ying Liu wrote:
[...]
>>> If the DP monitors support typical video modes like 1080p60 with
>>> 148.5MHz pixel clock rate, I assume these typical video modes work
>>> still ok with this patch at least. Please help confirm this, since if the
>>> alternative solution(*) doesn't stand, we would know those video
>>> modes still work ok with my solution(fixed PLL rate).
>>
>> They do not work with the fixed PLL setting.
>
> Why? Did you assign a sensible fixed PLL rate in DT?
Whatever was in imx8mp.dtsi does not really work for all the panels.
Please keep in mind that the use case I have does not include only
1920x1080 "standard" panels, but also other resolutions.
> Can you please compare clk_summary output for the failing cases
> before and after this patch is applied? I assume that if you use
> the fixed PLL rate same to the rate which works before this patch is
> applied, the typical video modes still just work after this patch is
> applied.
I'm afraid I do not need to support only typical video modes, but also
the other "atypical" modes.
[...]
>> One really nasty way I can think of is -- use find_node_by_compatible(),
>> look up all the relevant DT nodes, parse their clock properties, and
>> check whether they all point to the Video PLL or not.
>
> That's nasty. It looks even more nasty when considering the fact that
> i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF
> needs the nasty check, because i.MX93 SoC embeds only one LCDIF.
The check can be skipped based on compatible string.
I agree it is nasty, but it is a start. Are there better ideas ?
>> Maybe the clock subsystem has a better way, like list "neighbor"
>> consumers of some specific parent clock or something like that.
>
> What will imx-lcdif DRM look like by using this way? Get the ancestor PLL
> clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks
> (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the
> PLL clock in a string array and find media_disp1_pix + media_disp2_pix
> in it?
>
> Doesn't look nice, either.
One other option came to my mind -- place a virtual clock between the
Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock
driver do the clock rate negotiation in some .round_rate callback. That
is also nasty, but it is another idea. If there is a clock specifically
implemented to negotiate best upstream clock rate for all of its
consumers, and it is aware of the consumer behavior details and
requirements, maybe that could work ?
>> [...]
>>
>>>> Can something like (*) above be implemented instead, so both Shared
>> and
>>>> separate PLLs would be supported ? That should solve both of our use
>>>> cases, right ?
>>>
>>> I don't see any clear way to implement something like(*).
>>>
>>> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
>>> DRM instance? Would it be too intrusive?
>>
>> Yes, and I think unnecessary, one can simply traverse and parse the DT
>> to determine the clock assignment?
>
> Yes, people can traverse and parse DT, but it's nasty.
>
> In addition, one may argue that now that CLK_SET_RATE_PARENT flag
> is set for the pixel clocks, all potential video modes read from EDID
> should be supported when only either LVDS display pipeline or MIPI DSI
> display pipeline is active in the shared PLL case. This requires one
> single DRM instance to detect single or dual active display pipelines
> dynamically, hence this single DRM instance becomes necessary.
Would single virtual clock which do the frequency negotiation between
multiple DRM consumers work too ?
I do not have much to add to the points below.
Powered by blists - more mailing lists