Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Lucas Stach <l.stach@pengutronix.de>]

Am Mittwoch, dem 08.12.2021 um 09:13 -0600 schrieb Adam Ford:
> On Wed, Dec 8, 2021 at 7:36 AM Benjamin Gaignard
> <benjamin.gaignard@collabora.com> wrote:
> > 
> > 
> > Le 08/12/2021 à 11:32, Lucas Stach a écrit :
> > > Am Mittwoch, dem 08.12.2021 um 10:32 +0100 schrieb Benjamin Gaignard:
> > > > Le 07/12/2021 à 02:54, Adam Ford a écrit :
> > > > 
> > > > > Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
> > > > > reality, it's two IP blocks called G1 and G2.  There is initialization
> > > > > code in VPU code to pull some clocks, resets and other features which
> > > > > has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
> > > > > similar method can be used to make the VPU codec's operate as
> > > > > stand-alone cores without having to know the details of each other
> > > > > or the quirks unique to the i.MX8MQ, so the remaining code can be
> > > > > left more generic.
> > > > > 
> > > > > This series was started by Lucas Stach with one by Benjamin Gaignard.
> > > > > Most patches have been modified slightly by me.  It's in an RFC state
> > > > > because I wasn't sure how to best handle the signatures and wasn't sure
> > > > > if I could base it off the branch I did.
> > > > > 
> > > > > Since the g-streamer and media trees are in a constant state of
> > > > > change, this series is based on
> > > > > 
> > > > > git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
> > > > > 
> > > > > The downstream code from NXP shows the G1 and G2 clocks running
> > > > > at 600MHz, but between the TRM and the datasheet, there is some
> > > > > discrepancy.  Because the NXP reference code used 600MHz, that is
> > > > > what was chosen here.  Users who need to adjust their G1 and G2
> > > > > clocks can do so in their board files.
> > > > Hi Adam,
> > > > 
> > > > Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
> > > > In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.
> > 
> > The results for Fluster HEVC are 77/147 so no regressions :-)
> > 
> > Regards,
> > Benjamin
> > 
> > > > The max supported G2 clock frequency is 660MHz but needs a higher
> > > > voltage. The maximum supported  frequency at the default 0.9V is
> > > > 550MHz. We should not configure the clocks for the higher than that, as
> > > > long as there is no support in the VPU driver to scale the voltage
> > > > along with the frequency. Same as with the GPU we should stick to base
> > > > frequency levels for the nominal operating mode.
> 
> Lucas,
> 
> After reviewing the comments from Benjamin, I re-ran the VP9 tests
> with the G2 running at 300MHz,and the number of passing VP9 tests
> increased to 148 from 144 with an increase of time to 250.502 secs
> from 162.665 secs.
> 
> While the datasheet reads that the G2 can run faster, the i.MX 8M
> Dual/8M QuadLite/8M Quad Applications Processors Reference Manual,
> Rev. 3.1, 06/2021, table 5-1  shows the VPU_G2_CLK_ROOT has a max
> clock of 300MHz.  I might be inclined to agree with Benjamin on the
> 300MHz and let people who want to push their hardware overwrite the
> default clocks since it increases functionality.
> 
I'm quite surprised that the G2 codec should have such a much lower max
frequency compared with the G1. While the table from the RM hasn't been
obviously incorrect for any other clock, I'm still inclined to believe
the frequencies stated in the datasheet.

> I wonder if someone from NXP can comment
> 
Yea, not sure how to proceed here. 300MHz is the safe bet, but we are
leaving quite some performance on the table if the datasheet is
correct. Without some help from NXP it's probably hard to validate 
which max frequency statement is correct. It doesn't really help that
the NXP downstream kernel seems to drive the G2 at 600MHz, which is
neither the 550MHz nominal mode max, nor the 660MHz overdrive mode max.

Regards,
Lucas