Message-ID: <CAHCN7xJsTo=yRxkqPwvt5hX0UTFvF+gBVOqcjrR2jSq4jKzV3g@mail.gmail.com>
Date: Sat, 23 Nov 2024 13:47:19 -0600
From: Adam Ford <aford173@...il.com>
To: Ying Liu <victor.liu@....com>
Cc: Marek Vasut <marex@...x.de>, "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>, 
	"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 Thu, Nov 21, 2024 at 9:39 PM Ying Liu <victor.liu@....com> wrote:
>
> On 11/22/24, Marek Vasut wrote:
> > 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.
>
> It looks like you are still sticking to the idea of supporting all potentially
> valid video modes by trying to find an "alternative" solution, while
> neglecting that the solution *could be* never working.
>
> >
> > > 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.
>
> If the "alternative" solution doesn't work, we'll end up using the "fixed
> PLL rate" solution.  It that case, some video modes would be filtered
> out as a sacrifice.
>
> >
> > [...]
> >
> > >> 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 ?
>
> No good idea from me.
>
> >
> > >> 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 ?
>
> A mighty virtual clock?  I'm not sure if that would work or not.

>From a power-consumption perspective, it seems to me like running the
clocks at the lowest value instead of setting a really high rate which
divides down would save power.

adam
>
> >
> > >> [...]
> > >>
> > >>>> 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 ?
>
> Not sure if it would work or not, but I'm sure that one single DRM instance
> means atomic check/commit for the display pipelines as a whole, hence
> awareness of active display pipeline number in an atomic way.
>
> >
> > I do not have much to add to the points below.
>
> Regards,
> Liu Ying

Powered by blists - more mailing lists