Message-ID: <CAHCN7xKreZmJDyght97eSwdv6v8BgZfkzApM85x=PrR1PJFCDQ@mail.gmail.com>
Date: Tue, 1 Apr 2025 08:59:32 -0500
From: Adam Ford <aford173@...il.com>
To: Dominique Martinet <dominique.martinet@...ark-techno.com>
Cc: Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
	Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>, 
	Frieder Schrempf <frieder.schrempf@...tron.de>, Marco Felsch <m.felsch@...gutronix.de>, 
	Lucas Stach <l.stach@...gutronix.de>, linux-phy@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Makoto Sato <makoto.sato@...ark-techno.com>
Subject: Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate
 instead LUT

On Mon, Mar 31, 2025 at 4:59 PM Dominique Martinet
<dominique.martinet@...ark-techno.com> wrote:
>
>
> Adam Ford wrote on Mon, Mar 31, 2025 at 08:45:26AM -0500:
> > If Dominique won't have time, I can try to clean this up a bit.  I was
> > not liking the names either, but I was trying to keep them small.
> > I'll default to this convention more in the future, based on your
> > feedback.
>
> I've been chasing weird suspend bugs on our platform (another soc) for
> the best of the month so it'd be greatly appreciated, sorry.
>
> Feel free to replace our patch with anything equivalent
>
> Adam Ford wrote on Mon, Mar 31, 2025 at 08:43:38AM -0500:
> > > For this particular rate check, I believe that if phy_clk_round_rate()
> > > rejected the frequency then phy_clk_set_rate() cannot be called at all
> > > with the said value (at least on this particular setup going through the
> > > imx8mp-hdmi-tx bridge validating frequencies first before allowing
> > > modes), not that it'd hurt to recheck.
> >
> > I believe that is true.  I considered adding it, but when I put debug
> > code in to trace what was happening, it was being rejected, in one
> > place, so the other didn't need to.  If the general consensus is to
> > have it in both places, I can add it.
>
> Yes I think it will make the intent more clear, if we're going to share
> some code or make it more consistent might as well go all the way.
>
> > > In general though I agree these are doing pretty much the same thing,
> > > with clk_round_rate() throwing away the pms values and there's more
> > > duplication than ideal...
> >
> > I've been working on creating some caching to determine the best
> > values for the PHY and remember them, so the work doesn't have to be
> > done again if the next call uses the same clock rate.  I'm not quite
> > ready to submit it, because I need to rebase it on Linux-Next with
> > some of the other updates requested by Uwe.  My updates also remove
> > the look-up table completely and use an algorithm to determine the
> > fractional divider values - thanks to Frieder's python code that I
> > ported to C.  I experimented quite a bit with which values have more
> > impact and reorganized his nested for-loops to keep track of how many
> > iterations are done and also measuring the time it takes to do the
> > calculations, so the code doesn't really look like his as much as one
> > would think.
> >
> > The downside with my updates is that running 'modetest'  on the 4K
> > monitor that I use has so many entries, the time it takes to calculate
> > all the values for the monitor takes a second or two longer than
> > before, because searching the LUT is quick and doing a series of
> > for-loops to find the nominal values is more time consuming.  We could
> > potentially keep the LUT and only use this new calculation if the
> > entry isn't in the LUT.  I am not generally a fan of LUT's if the
> > values can be calculated, but I can also see the advantage of speed.
>
> Hmm, tough question... I don't think we want displays to show up a few
> seconds later, we regularily get mails from customers asking how to get
> a logo to show up faster on display (I think there may be some variant
> of uboot that supports imx8mp hdmi? but we don't have that), so while I
> see the appeal of not having an hardcoded look up table I'm not sure we
> would appreciate that compromise.

I only notice the delay when I run modetest to get a list of available
resolutions since it's trying every resolution to see if it can
achieve the desired clock rate.

If I run modetest to set the resolution, it's not really noticeable
because it's really only trying

>
> I think it'd be great to just have a way of checking the values in the
> LUT are correct though (either statically from Frieder's python code as
> I started doing or through a CONFIG_WHATEVER_CHECK_LUT config item
> that'd check once at boot, although that's probably overkill); I started

As a compromise, I could leave the LUT, and fall back to the manual
calculations to both speed up the modetest look up, but to also
maximize the ability to sync new resolutions.  Even with what I have,
it's not perfect.  I skipped a few variables since my testing showed
they didn't have a huge impact and adding them increased the number of
calculations to determine the resolution.

> checking from the python code but they weren't computed with the same
> algorithm so some values end up being different even if theend result is
> the same frequency and I stopped halfway...
>
> > > That's quite rude of me given I just sent the patch, but we probably
> > > won't have time to rework this until mid-april after some urgent work
> > > ends (this has actually been waiting for testing for 3 months
> > > already...)
> > > If this doesn't bother anyone else we can wait for a v2 then, but
> >
> > If you want, I can submit my stuff as an RFC to give it a try and
> > solicit feedback.
>
> Happy to test/review anything you send, but I make no promise on delays

No worries. I am just glad this driver is getting some love and enhancements.

adam
> :-)
>
> Cheers,
> --
> Dominique
>
>

Powered by blists - more mailing lists