[RFC,V2,0/2] arm64: imx8mm: Enable Hantro VPUs

  The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
to be related to the video decoders used on the i.MX8MQ, but because of
how the Mini handles the power domains, the VPU driver does not need to
handle all the functions, nor does it support the post-processor,
so a new compatible flag is required.

With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
it's unclear to me if that's an acceptable alternative.

At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
results from Fluster. However, the G2 VPU appears to fail most tests.

./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
Ran 90/135 tests successfully               in 76.431 secs

 ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
Ran 55/61 tests successfully               in 21.454 secs

./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
Ran 0/303 tests successfully               in 20.016 secs

Each day seems to show more and more G2 submissions, and gstreamer seems to be 
still working on the VP9, so I am not sure if I should drop G2 as well.


Adam Ford (2):
  media: hantro: Add support for i.MX8M Mini
  arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2

 arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
 drivers/staging/media/hantro/hantro_drv.c   |  2 +
 drivers/staging/media/hantro/hantro_hw.h    |  2 +
 drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
 4 files changed, 102 insertions(+)
  

Le 01/12/2021 à 02:33, Adam Ford a écrit :
> The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> to be related to the video decoders used on the i.MX8MQ, but because of
> how the Mini handles the power domains, the VPU driver does not need to
> handle all the functions, nor does it support the post-processor,
> so a new compatible flag is required.
>
> With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> it's unclear to me if that's an acceptable alternative.
>
> At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> results from Fluster. However, the G2 VPU appears to fail most tests.
>
> ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> Ran 90/135 tests successfully               in 76.431 secs
>
>   ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> Ran 55/61 tests successfully               in 21.454 secs
>
> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> Ran 0/303 tests successfully               in 20.016 secs
>
> Each day seems to show more and more G2 submissions, and gstreamer seems to be
> still working on the VP9, so I am not sure if I should drop G2 as well.

I think it is going in the good direction.
I'm trying to do the same on IMX6MQ but still have hang issue on G2.

Regards,
Benjamin

>
> Adam Ford (2):
>    media: hantro: Add support for i.MX8M Mini
>    arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
>
>   arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
>   drivers/staging/media/hantro/hantro_drv.c   |  2 +
>   drivers/staging/media/hantro/hantro_hw.h    |  2 +
>   drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
>   4 files changed, 102 insertions(+)
>
  

On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
>
> The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> to be related to the video decoders used on the i.MX8MQ, but because of
> how the Mini handles the power domains, the VPU driver does not need to
> handle all the functions, nor does it support the post-processor,
> so a new compatible flag is required.
>
> With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> it's unclear to me if that's an acceptable alternative.
>
> At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> results from Fluster. However, the G2 VPU appears to fail most tests.
>
> ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> Ran 90/135 tests successfully               in 76.431 secs
>
>  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> Ran 55/61 tests successfully               in 21.454 secs
>
> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> Ran 0/303 tests successfully               in 20.016 secs
>
> Each day seems to show more and more G2 submissions, and gstreamer seems to be
> still working on the VP9, so I am not sure if I should drop G2 as well.
>
>
> Adam Ford (2):
>   media: hantro: Add support for i.MX8M Mini
>   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
>
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
>  drivers/staging/media/hantro/hantro_drv.c   |  2 +
>  drivers/staging/media/hantro/hantro_hw.h    |  2 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
>  4 files changed, 102 insertions(+)
>

Adam,

That's for the patches!

I tested just this series on top of v5.16-rc3 on an
imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
is called for VPUMIX pd :
while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
...
[  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
[  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain

I added prints in imx_pgc_power_{up,down} and
imx8m_blk_ctrl_power_{on,off} to get some more context
...
Ran 55/61 tests successfully               in 8.685 secs
 17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
********************************************************************************
********************
Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
Using 4 parallel job(s)
********************************************************************************
********************

[TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
ESULT
----------------------------------------------------------------------
[ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
[ 1023.119669] imx_pgc_power_up vpumix
[ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
[ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain

While this wouldn't be an issue with this series it does indicate we
still have something racy in blk-ctrl. Can you reproduce this (and if
not what kernel are you based on)? Perhaps you or Lucas have some
ideas?

Best regards,

Tim
  

Hi Tim,

Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > 
> > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > to be related to the video decoders used on the i.MX8MQ, but because of
> > how the Mini handles the power domains, the VPU driver does not need to
> > handle all the functions, nor does it support the post-processor,
> > so a new compatible flag is required.
> > 
> > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > it's unclear to me if that's an acceptable alternative.
> > 
> > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > results from Fluster. However, the G2 VPU appears to fail most tests.
> > 
> > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > Ran 90/135 tests successfully               in 76.431 secs
> > 
> >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > Ran 55/61 tests successfully               in 21.454 secs
> > 
> > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > Ran 0/303 tests successfully               in 20.016 secs
> > 
> > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > still working on the VP9, so I am not sure if I should drop G2 as well.
> > 
> > 
> > Adam Ford (2):
> >   media: hantro: Add support for i.MX8M Mini
> >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > 
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> >  4 files changed, 102 insertions(+)
> > 
> 
> Adam,
> 
> That's for the patches!
> 
> I tested just this series on top of v5.16-rc3 on an
> imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> is called for VPUMIX pd :
> while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> ...
> [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> 
> I added prints in imx_pgc_power_{up,down} and
> imx8m_blk_ctrl_power_{on,off} to get some more context
> ...
> Ran 55/61 tests successfully               in 8.685 secs
>  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> ********************************************************************************
> ********************
> Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> Using 4 parallel job(s)
> ********************************************************************************
> ********************
> 
> [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> ESULT
> ----------------------------------------------------------------------
> [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> [ 1023.119669] imx_pgc_power_up vpumix
> [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> 
> While this wouldn't be an issue with this series it does indicate we
> still have something racy in blk-ctrl. Can you reproduce this (and if
> not what kernel are you based on)? Perhaps you or Lucas have some
> ideas?
> 
Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
domains" applied when running those tests? It has only recently been
picked up by Shawn and may have an influence on the bus domain
behavior.

Regards,
Lucas
  

On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Tim,
>
> Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > how the Mini handles the power domains, the VPU driver does not need to
> > > handle all the functions, nor does it support the post-processor,
> > > so a new compatible flag is required.
> > >
> > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > it's unclear to me if that's an acceptable alternative.
> > >
> > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > >
> > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > Ran 90/135 tests successfully               in 76.431 secs
> > >
> > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > Ran 55/61 tests successfully               in 21.454 secs
> > >
> > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > Ran 0/303 tests successfully               in 20.016 secs
> > >
> > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > >
> > >
> > > Adam Ford (2):
> > >   media: hantro: Add support for i.MX8M Mini
> > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > >
> > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > >  4 files changed, 102 insertions(+)
> > >
> >
> > Adam,
> >
> > That's for the patches!
> >
> > I tested just this series on top of v5.16-rc3 on an
> > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > is called for VPUMIX pd :
> > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > ...
> > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> >
> > I added prints in imx_pgc_power_{up,down} and
> > imx8m_blk_ctrl_power_{on,off} to get some more context
> > ...
> > Ran 55/61 tests successfully               in 8.685 secs
> >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > ********************************************************************************
> > ********************
> > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > Using 4 parallel job(s)
> > ********************************************************************************
> > ********************
> >
> > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > ESULT
> > ----------------------------------------------------------------------
> > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > [ 1023.119669] imx_pgc_power_up vpumix
> > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> >
> > While this wouldn't be an issue with this series it does indicate we
> > still have something racy in blk-ctrl. Can you reproduce this (and if
> > not what kernel are you based on)? Perhaps you or Lucas have some
> > ideas?
> >
> Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> domains" applied when running those tests? It has only recently been
> picked up by Shawn and may have an influence on the bus domain
> behavior.
>

Lucas,

Good point. I did have that originally before I started pruning down
to the bare minimum to reproduce the issue.

I added it back and now I have the following:
arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
media: hantro: Add support for i.MX8M Mini
soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
soc: imx: gpcv2: Synchronously suspend MIX domains
Linux 5.16-rc3

Here's the latest with that patch:
...
[VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
vp80-00-comprehensive-007 ... Success
[  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
[  316.636908] imx_pgc_power_down vpu-g1
[  316.640983] imx_pgc_power_down vpumix
[  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
[  316.761360] imx_pgc_power_up vpumix
[  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
[  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
^^^ hang

I believe there is some sort of simple test I can do to power the gpu
up/down to test as well but not clear what that is.

Tim
  

On Wed, Dec 1, 2021 at 11:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Tim,
>
> Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > how the Mini handles the power domains, the VPU driver does not need to
> > > handle all the functions, nor does it support the post-processor,
> > > so a new compatible flag is required.
> > >
> > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > it's unclear to me if that's an acceptable alternative.
> > >
> > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > >
> > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > Ran 90/135 tests successfully               in 76.431 secs
> > >
> > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > Ran 55/61 tests successfully               in 21.454 secs
> > >
> > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > Ran 0/303 tests successfully               in 20.016 secs
> > >
> > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > >
> > >
> > > Adam Ford (2):
> > >   media: hantro: Add support for i.MX8M Mini
> > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > >
> > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > >  4 files changed, 102 insertions(+)
> > >
> >
> > Adam,
> >
> > That's for the patches!
> >
> > I tested just this series on top of v5.16-rc3 on an
> > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > is called for VPUMIX pd :
> > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > ...
> > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> >
> > I added prints in imx_pgc_power_{up,down} and
> > imx8m_blk_ctrl_power_{on,off} to get some more context
> > ...
> > Ran 55/61 tests successfully               in 8.685 secs
> >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > ********************************************************************************
> > ********************
> > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > Using 4 parallel job(s)
> > ********************************************************************************
> > ********************
> >
> > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > ESULT
> > ----------------------------------------------------------------------
> > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > [ 1023.119669] imx_pgc_power_up vpumix
> > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> >
> > While this wouldn't be an issue with this series it does indicate we
> > still have something racy in blk-ctrl. Can you reproduce this (and if
> > not what kernel are you based on)? Perhaps you or Lucas have some
> > ideas?

i have not seen an issue with my implementation, but used
media-staging [1] for the last attempt to try to get as much of the
latest hantro driver integration, although the VP9 stuff isn't quite
ready yet on the g2-VPU

[1] - https://git.linuxtv.org/media_stage.git/log/drivers/staging/media/hantro

> >
> Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> domains" applied when running those tests? It has only recently been
> picked up by Shawn and may have an influence on the bus domain
> behavior.

I didn't know about this one either, so I'll try it.

When I was attempting to read registers from the H1 vpu, I had to set
"keep_clocks = true" for the H1 power domain or it would hang.  If the
patch Lucas suggests doesn't work, you could try keeing the G1 or G2
clocks on.  I believe it's already set for the vpumix, but the G1, G2
and H1 clocks are not touched by the vpumix, just the
IMX8MM_CLK_VPU_DEC_ROOT.

>
> Regards,
> Lucas
>
  

Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Hi Tim,
> > 
> > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > 
> > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > handle all the functions, nor does it support the post-processor,
> > > > so a new compatible flag is required.
> > > > 
> > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > it's unclear to me if that's an acceptable alternative.
> > > > 
> > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > 
> > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > 
> > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > 
> > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > 
> > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > 
> > > > 
> > > > Adam Ford (2):
> > > >   media: hantro: Add support for i.MX8M Mini
> > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > 
> > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > >  4 files changed, 102 insertions(+)
> > > > 
> > > 
> > > Adam,
> > > 
> > > That's for the patches!
> > > 
> > > I tested just this series on top of v5.16-rc3 on an
> > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > is called for VPUMIX pd :
> > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > ...
> > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > 
> > > I added prints in imx_pgc_power_{up,down} and
> > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > ...
> > > Ran 55/61 tests successfully               in 8.685 secs
> > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > ********************************************************************************
> > > ********************
> > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > Using 4 parallel job(s)
> > > ********************************************************************************
> > > ********************
> > > 
> > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > ESULT
> > > ----------------------------------------------------------------------
> > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > [ 1023.119669] imx_pgc_power_up vpumix
> > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > 
> > > While this wouldn't be an issue with this series it does indicate we
> > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > ideas?
> > > 
> > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > domains" applied when running those tests? It has only recently been
> > picked up by Shawn and may have an influence on the bus domain
> > behavior.
> > 
> 
> Lucas,
> 
> Good point. I did have that originally before I started pruning down
> to the bare minimum to reproduce the issue.
> 
> I added it back and now I have the following:
> arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> media: hantro: Add support for i.MX8M Mini
> soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> soc: imx: gpcv2: Synchronously suspend MIX domains
> Linux 5.16-rc3
> 
> Here's the latest with that patch:
> ...
> [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> vp80-00-comprehensive-007 ... Success
> [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> [  316.636908] imx_pgc_power_down vpu-g1
> [  316.640983] imx_pgc_power_down vpumix
> [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> [  316.761360] imx_pgc_power_up vpumix
> [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> ^^^ hang

Hm, I wonder if there's some broken error handling here somewhere, as a
failure to power up a domain shouldn't lead to a hang.

However, that doesn't explain why the PGC isn't completing the request.
Can you try to extend the timeout some more. Even though I think that
1msec should already be generous. Can you dump the content of the
GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
registers, when the failure condition is hit?

Regards,
Lucas
  

On Wed, Dec 1, 2021 at 12:37 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> > On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Hi Tim,
> > >
> > > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > > handle all the functions, nor does it support the post-processor,
> > > > > so a new compatible flag is required.
> > > > >
> > > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > > it's unclear to me if that's an acceptable alternative.
> > > > >
> > > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > >
> > > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > >
> > > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > >
> > > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > >
> > > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > >
> > > > >
> > > > > Adam Ford (2):
> > > > >   media: hantro: Add support for i.MX8M Mini
> > > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > >
> > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > > >  4 files changed, 102 insertions(+)
> > > > >
> > > >
> > > > Adam,
> > > >
> > > > That's for the patches!
> > > >
> > > > I tested just this series on top of v5.16-rc3 on an
> > > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > > is called for VPUMIX pd :
> > > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > > ...
> > > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > >
> > > > I added prints in imx_pgc_power_{up,down} and
> > > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > > ...
> > > > Ran 55/61 tests successfully               in 8.685 secs
> > > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > > ********************************************************************************
> > > > ********************
> > > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > > Using 4 parallel job(s)
> > > > ********************************************************************************
> > > > ********************
> > > >
> > > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > > ESULT
> > > > ----------------------------------------------------------------------
> > > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > > [ 1023.119669] imx_pgc_power_up vpumix
> > > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > >
> > > > While this wouldn't be an issue with this series it does indicate we
> > > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > > ideas?
> > > >
> > > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > > domains" applied when running those tests? It has only recently been
> > > picked up by Shawn and may have an influence on the bus domain
> > > behavior.
> > >
> >
> > Lucas,
> >
> > Good point. I did have that originally before I started pruning down
> > to the bare minimum to reproduce the issue.
> >
> > I added it back and now I have the following:
> > arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > media: hantro: Add support for i.MX8M Mini
> > soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> > soc: imx: gpcv2: Synchronously suspend MIX domains
> > Linux 5.16-rc3
> >
> > Here's the latest with that patch:
> > ...
> > [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> > vp80-00-comprehensive-007 ... Success
> > [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> > [  316.636908] imx_pgc_power_down vpu-g1
> > [  316.640983] imx_pgc_power_down vpumix
> > [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> > [  316.761360] imx_pgc_power_up vpumix
> > [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> > [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > ^^^ hang
>
> Hm, I wonder if there's some broken error handling here somewhere, as a
> failure to power up a domain shouldn't lead to a hang.
>
> However, that doesn't explain why the PGC isn't completing the request.
> Can you try to extend the timeout some more. Even though I think that
> 1msec should already be generous. Can you dump the content of the
> GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
> registers, when the failure condition is hit?

I submitted a patch [1]  to enable the commented-out if statement
which waits for the handshake if the gpc domain was invoked by the
blk-ctrl or we knew if the bus clock was operational.

I am not 100% certain it can work as-is with the vpumix, but based on
what I've seen from my testing, it's not hanging or causing errors.

[1] - https://lore.kernel.org/linux-arm-kernel/20211120194900.1309914-1-aford173@gmail.com/T/

I didn't have it applied to my latest RFC for the G1 and G2 because I
had not noticed a change in behavior one way or the other with that
patch.

adam
>
> Regards,
> Lucas
>
  

Am Mittwoch, dem 01.12.2021 um 12:52 -0600 schrieb Adam Ford:
> On Wed, Dec 1, 2021 at 12:37 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> > > On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > 
> > > > Hi Tim,
> > > > 
> > > > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > > > 
> > > > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > > > handle all the functions, nor does it support the post-processor,
> > > > > > so a new compatible flag is required.
> > > > > > 
> > > > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > > > it's unclear to me if that's an acceptable alternative.
> > > > > > 
> > > > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > > > 
> > > > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > > > 
> > > > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > > > 
> > > > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > > > 
> > > > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > > > 
> > > > > > 
> > > > > > Adam Ford (2):
> > > > > >   media: hantro: Add support for i.MX8M Mini
> > > > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > > > 
> > > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > > > >  4 files changed, 102 insertions(+)
> > > > > > 
> > > > > 
> > > > > Adam,
> > > > > 
> > > > > That's for the patches!
> > > > > 
> > > > > I tested just this series on top of v5.16-rc3 on an
> > > > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > > > is called for VPUMIX pd :
> > > > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > > > ...
> > > > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > > 
> > > > > I added prints in imx_pgc_power_{up,down} and
> > > > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > > > ...
> > > > > Ran 55/61 tests successfully               in 8.685 secs
> > > > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > > > ********************************************************************************
> > > > > ********************
> > > > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > > > Using 4 parallel job(s)
> > > > > ********************************************************************************
> > > > > ********************
> > > > > 
> > > > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > > > ESULT
> > > > > ----------------------------------------------------------------------
> > > > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > > > [ 1023.119669] imx_pgc_power_up vpumix
> > > > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > > 
> > > > > While this wouldn't be an issue with this series it does indicate we
> > > > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > > > ideas?
> > > > > 
> > > > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > > > domains" applied when running those tests? It has only recently been
> > > > picked up by Shawn and may have an influence on the bus domain
> > > > behavior.
> > > > 
> > > 
> > > Lucas,
> > > 
> > > Good point. I did have that originally before I started pruning down
> > > to the bare minimum to reproduce the issue.
> > > 
> > > I added it back and now I have the following:
> > > arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > media: hantro: Add support for i.MX8M Mini
> > > soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> > > soc: imx: gpcv2: Synchronously suspend MIX domains
> > > Linux 5.16-rc3
> > > 
> > > Here's the latest with that patch:
> > > ...
> > > [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> > > vp80-00-comprehensive-007 ... Success
> > > [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> > > [  316.636908] imx_pgc_power_down vpu-g1
> > > [  316.640983] imx_pgc_power_down vpumix
> > > [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> > > [  316.761360] imx_pgc_power_up vpumix
> > > [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > ^^^ hang
> > 
> > Hm, I wonder if there's some broken error handling here somewhere, as a
> > failure to power up a domain shouldn't lead to a hang.
> > 
> > However, that doesn't explain why the PGC isn't completing the request.
> > Can you try to extend the timeout some more. Even though I think that
> > 1msec should already be generous. Can you dump the content of the
> > GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
> > registers, when the failure condition is hit?
> 
> I submitted a patch [1]  to enable the commented-out if statement
> which waits for the handshake if the gpc domain was invoked by the
> blk-ctrl or we knew if the bus clock was operational.
> 
> I am not 100% certain it can work as-is with the vpumix, but based on
> what I've seen from my testing, it's not hanging or causing errors.
> 
> [1] - https://lore.kernel.org/linux-arm-kernel/20211120194900.1309914-1-aford173@gmail.com/T/
> 
> I didn't have it applied to my latest RFC for the G1 and G2 because I
> had not noticed a change in behavior one way or the other with that
> patch.

That's not going to work with all the MIX domains. The handshake
requires some clocks to be enabled in the blk-ctrl (the secondary clock
gates in the blk-ctrl) to work properly. This is only done by the blk-
ctrl driver _after_ the GPC bus domain is powered up, so you can not
wait for the handshake to complete inside the GPC power up routine.

Regards,
Lucas
  

On Wed, Dec 1, 2021 at 1:04 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Mittwoch, dem 01.12.2021 um 12:52 -0600 schrieb Adam Ford:
> > On Wed, Dec 1, 2021 at 12:37 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> > > > On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > >
> > > > > Hi Tim,
> > > > >
> > > > > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > > > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > > > >
> > > > > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > > > > handle all the functions, nor does it support the post-processor,
> > > > > > > so a new compatible flag is required.
> > > > > > >
> > > > > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > > > > it's unclear to me if that's an acceptable alternative.
> > > > > > >
> > > > > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > > > >
> > > > > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > > > >
> > > > > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > > > >
> > > > > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > > > >
> > > > > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > > > >
> > > > > > >
> > > > > > > Adam Ford (2):
> > > > > > >   media: hantro: Add support for i.MX8M Mini
> > > > > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > > > >
> > > > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > > > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > > > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > > > > >  4 files changed, 102 insertions(+)
> > > > > > >
> > > > > >
> > > > > > Adam,
> > > > > >
> > > > > > That's for the patches!
> > > > > >
> > > > > > I tested just this series on top of v5.16-rc3 on an
> > > > > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > > > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > > > > is called for VPUMIX pd :
> > > > > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > > > > ...
> > > > > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > > >
> > > > > > I added prints in imx_pgc_power_{up,down} and
> > > > > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > > > > ...
> > > > > > Ran 55/61 tests successfully               in 8.685 secs
> > > > > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > > > > ********************************************************************************
> > > > > > ********************
> > > > > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > > > > Using 4 parallel job(s)
> > > > > > ********************************************************************************
> > > > > > ********************
> > > > > >
> > > > > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > > > > ESULT
> > > > > > ----------------------------------------------------------------------
> > > > > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > > > > [ 1023.119669] imx_pgc_power_up vpumix
> > > > > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > > >
> > > > > > While this wouldn't be an issue with this series it does indicate we
> > > > > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > > > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > > > > ideas?
> > > > > >
> > > > > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > > > > domains" applied when running those tests? It has only recently been
> > > > > picked up by Shawn and may have an influence on the bus domain
> > > > > behavior.
> > > > >
> > > >
> > > > Lucas,
> > > >
> > > > Good point. I did have that originally before I started pruning down
> > > > to the bare minimum to reproduce the issue.
> > > >
> > > > I added it back and now I have the following:
> > > > arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > media: hantro: Add support for i.MX8M Mini
> > > > soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> > > > soc: imx: gpcv2: Synchronously suspend MIX domains
> > > > Linux 5.16-rc3
> > > >
> > > > Here's the latest with that patch:
> > > > ...
> > > > [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> > > > vp80-00-comprehensive-007 ... Success
> > > > [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> > > > [  316.636908] imx_pgc_power_down vpu-g1
> > > > [  316.640983] imx_pgc_power_down vpumix
> > > > [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> > > > [  316.761360] imx_pgc_power_up vpumix
> > > > [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > ^^^ hang
> > >
> > > Hm, I wonder if there's some broken error handling here somewhere, as a
> > > failure to power up a domain shouldn't lead to a hang.
> > >
> > > However, that doesn't explain why the PGC isn't completing the request.
> > > Can you try to extend the timeout some more. Even though I think that
> > > 1msec should already be generous. Can you dump the content of the
> > > GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
> > > registers, when the failure condition is hit?
> >
> > I submitted a patch [1]  to enable the commented-out if statement
> > which waits for the handshake if the gpc domain was invoked by the
> > blk-ctrl or we knew if the bus clock was operational.
> >
> > I am not 100% certain it can work as-is with the vpumix, but based on
> > what I've seen from my testing, it's not hanging or causing errors.
> >
> > [1] - https://lore.kernel.org/linux-arm-kernel/20211120194900.1309914-1-aford173@gmail.com/T/
> >
> > I didn't have it applied to my latest RFC for the G1 and G2 because I
> > had not noticed a change in behavior one way or the other with that
> > patch.
>
> That's not going to work with all the MIX domains. The handshake
> requires some clocks to be enabled in the blk-ctrl (the secondary clock
> gates in the blk-ctrl) to work properly. This is only done by the blk-
> ctrl driver _after_ the GPC bus domain is powered up, so you can not
> wait for the handshake to complete inside the GPC power up routine.

I wasn't exactly sure how the handshake worked.  What your saying makes sense.
Will you NAK my patch so it doesn't accidentally get applied.

adam
>
> Regards,
> Lucas
>
  

On Wed, Dec 1, 2021 at 10:37 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> > On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Hi Tim,
> > >
> > > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > > handle all the functions, nor does it support the post-processor,
> > > > > so a new compatible flag is required.
> > > > >
> > > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > > it's unclear to me if that's an acceptable alternative.
> > > > >
> > > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > >
> > > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > >
> > > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > >
> > > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > >
> > > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > >
> > > > >
> > > > > Adam Ford (2):
> > > > >   media: hantro: Add support for i.MX8M Mini
> > > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > >
> > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > > >  4 files changed, 102 insertions(+)
> > > > >
> > > >
> > > > Adam,
> > > >
> > > > That's for the patches!
> > > >
> > > > I tested just this series on top of v5.16-rc3 on an
> > > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > > is called for VPUMIX pd :
> > > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > > ...
> > > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > >
> > > > I added prints in imx_pgc_power_{up,down} and
> > > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > > ...
> > > > Ran 55/61 tests successfully               in 8.685 secs
> > > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > > ********************************************************************************
> > > > ********************
> > > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > > Using 4 parallel job(s)
> > > > ********************************************************************************
> > > > ********************
> > > >
> > > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > > ESULT
> > > > ----------------------------------------------------------------------
> > > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > > [ 1023.119669] imx_pgc_power_up vpumix
> > > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > >
> > > > While this wouldn't be an issue with this series it does indicate we
> > > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > > ideas?
> > > >
> > > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > > domains" applied when running those tests? It has only recently been
> > > picked up by Shawn and may have an influence on the bus domain
> > > behavior.
> > >
> >
> > Lucas,
> >
> > Good point. I did have that originally before I started pruning down
> > to the bare minimum to reproduce the issue.
> >
> > I added it back and now I have the following:
> > arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > media: hantro: Add support for i.MX8M Mini
> > soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> > soc: imx: gpcv2: Synchronously suspend MIX domains
> > Linux 5.16-rc3
> >
> > Here's the latest with that patch:
> > ...
> > [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> > vp80-00-comprehensive-007 ... Success
> > [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> > [  316.636908] imx_pgc_power_down vpu-g1
> > [  316.640983] imx_pgc_power_down vpumix
> > [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> > [  316.761360] imx_pgc_power_up vpumix
> > [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> > [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > ^^^ hang
>
> Hm, I wonder if there's some broken error handling here somewhere, as a
> failure to power up a domain shouldn't lead to a hang.
>
> However, that doesn't explain why the PGC isn't completing the request.
> Can you try to extend the timeout some more. Even though I think that
> 1msec should already be generous. Can you dump the content of the
> GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
> registers, when the failure condition is hit?
>

Adam,

Adding keep_clocks=true to VPUG1/VPUG2 domains did not help

Lucas,

I bumped the regmap_read_poll_timeout timeouts from 1m to 100ms and
still saw the same issue.

Here's some added debugging to show the regs:
[  648.037903] imx8m_blk_ctrl_power_on vpublk-g1
[  648.042346] imx_pgc_power_up vpumix
[  648.146178] imx-pgc imx-pgc-domain.6: imx_pgc_power_up: failed to command PGC
[  648.153355] imx-pgc imx-pgc-domain.6: GPC_PU_PGC_SW_PUP_REQ(0x0f8)=0x00000100
[  648.162339] imx-pgc imx-pgc-domain.6:
GPC_A53_PU_PGC_PUP_STATUS0(0x14c)=0x00000000
[  648.169988] imx-pgc imx-pgc-domain.6:
GPC_A53_PU_PGC_PUP_STATUS1(0x150)=0x00000000
[  648.177618] imx-pgc imx-pgc-domain.6:
GPC_A53_PU_PGC_PUP_STATUS2(0x154)=0x00000000
[  648.185281] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 8176380b02e6..8124a3434655 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -67,6 +67,9 @@

 #define GPC_PU_PGC_SW_PUP_REQ          0x0f8
 #define GPC_PU_PGC_SW_PDN_REQ          0x104
+#define GPC_A53_PU_PGC_PUP_STATUS0     0x14c
+#define GPC_A53_PU_PGC_PUP_STATUS1     0x150
+#define GPC_A53_PU_PGC_PUP_STATUS2     0x154

 #define IMX7_USB_HSIC_PHY_SW_Pxx_REQ           BIT(4)
 #define IMX7_USB_OTG2_PHY_SW_Pxx_REQ           BIT(3)
@@ -224,6 +227,7 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
        u32 reg_val, pgc;
        int ret;

+printk("%s %s\n", __func__, genpd->name);
        ret = pm_runtime_get_sync(domain->dev);
        if (ret < 0) {
                pm_runtime_put_noidle(domain->dev);
@@ -258,9 +262,17 @@ static int imx_pgc_power_up(struct
generic_pm_domain *genpd)
                ret = regmap_read_poll_timeout(domain->regmap,
                                               GPC_PU_PGC_SW_PUP_REQ, reg_val,
                                               !(reg_val & domain->bits.pxx),
-                                              0, USEC_PER_MSEC);
+                                              0, 100 * USEC_PER_MSEC);
                if (ret) {
-                       dev_err(domain->dev, "failed to command PGC\n");
+                       dev_err(domain->dev, "%s: failed to command
PGC\n", __func__);
+                       if (!regmap_read(domain->regmap,
GPC_PU_PGC_SW_PUP_REQ, &reg_val))
+                               dev_err(domain->dev,
"GPC_PU_PGC_SW_PUP_REQ(0x%03x)=0x%08x\n", GPC_PU_PGC_SW_PUP_REQ,
reg_val);
+                       if (!regmap_read(domain->regmap,
GPC_A53_PU_PGC_PUP_STATUS0, &reg_val))
+                               dev_err(domain->dev,
"GPC_A53_PU_PGC_PUP_STATUS0(0x%03x)=0x%08x\n",
GPC_A53_PU_PGC_PUP_STATUS0, reg_val);
+                       if (!regmap_read(domain->regmap,
GPC_A53_PU_PGC_PUP_STATUS1, &reg_val))
+                               dev_err(domain->dev,
"GPC_A53_PU_PGC_PUP_STATUS1(0x%03x)=0x%08x\n",
GPC_A53_PU_PGC_PUP_STATUS1, reg_val);
+                       if (!regmap_read(domain->regmap,
GPC_A53_PU_PGC_PUP_STATUS2, &reg_val))
+                               dev_err(domain->dev,
"GPC_A53_PU_PGC_PUP_STATUS2(0x%03x)=0x%08x\n",
GPC_A53_PU_PGC_PUP_STATUS2, reg_val);
                        goto out_clk_disable;
                }

@@ -318,6 +330,7 @@ static int imx_pgc_power_down(struct
generic_pm_domain *genpd)
        u32 reg_val, pgc;
        int ret;

+printk("%s %s\n", __func__, genpd->name);
        /* Enable reset clocks for all devices in the domain */
        if (!domain->keep_clocks) {
                ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
@@ -335,7 +348,7 @@ static int imx_pgc_power_down(struct
generic_pm_domain *genpd)
                ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
                                               reg_val,
                                               !(reg_val & domain->bits.hskack),
-                                              0, USEC_PER_MSEC);
+                                              0, 100 * USEC_PER_MSEC);
                if (ret) {
                        dev_err(domain->dev, "failed to power down ADB400\n");
                        goto out_clk_disable;
@@ -359,9 +372,9 @@ static int imx_pgc_power_down(struct
generic_pm_domain *genpd)
                ret = regmap_read_poll_timeout(domain->regmap,
                                               GPC_PU_PGC_SW_PDN_REQ, reg_val,
                                               !(reg_val & domain->bits.pxx),
-                                              0, USEC_PER_MSEC);
+                                              0, 100 * USEC_PER_MSEC);
                if (ret) {
-                       dev_err(domain->dev, "failed to command PGC\n");
+                       dev_err(domain->dev, "%s: failed to command
PGC\n", __func__);
                        goto out_clk_disable;
                }
        }
@@ -712,6 +725,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
                        .map = IMX8MM_VPUG1_A53_DOMAIN,
                },
                .pgc   = BIT(IMX8MM_PGC_VPUG1),
+               .keep_clocks = true,
        },

        [IMX8MM_POWER_DOMAIN_VPUG2] = {
@@ -723,6 +737,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
                        .map = IMX8MM_VPUG2_A53_DOMAIN,
                },
                .pgc   = BIT(IMX8MM_PGC_VPUG2),
+               .keep_clocks = true,
        },

        [IMX8MM_POWER_DOMAIN_VPUH1] = {
diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 519b3651d1d9..028f38d45892 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -68,6 +68,7 @@ static int imx8m_blk_ctrl_power_on(struct
generic_pm_domain *genpd)
        struct imx8m_blk_ctrl *bc = domain->bc;
        int ret;

+printk("%s %s\n", __func__, genpd->name);
        /* make sure bus domain is awake */
        ret = pm_runtime_get_sync(bc->bus_power_dev);
        if (ret < 0) {
@@ -119,6 +120,7 @@ static int imx8m_blk_ctrl_power_off(struct
generic_pm_domain *genpd)
        const struct imx8m_blk_ctrl_domain_data *data = domain->data;
        struct imx8m_blk_ctrl *bc = domain->bc;

+printk("%s %s\n", __func__, genpd->name);
        /* put devices into reset and disable clocks */
        regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
        regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);

Tim
  

On Wed, Dec 1, 2021 at 2:04 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Wed, Dec 1, 2021 at 10:37 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> > > On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > >
> > > > Hi Tim,
> > > >
> > > > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > > >
> > > > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > > > handle all the functions, nor does it support the post-processor,
> > > > > > so a new compatible flag is required.
> > > > > >
> > > > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > > > it's unclear to me if that's an acceptable alternative.
> > > > > >
> > > > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > > >
> > > > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > > >
> > > > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > > >
> > > > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > > >
> > > > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > > >
> > > > > >
> > > > > > Adam Ford (2):
> > > > > >   media: hantro: Add support for i.MX8M Mini
> > > > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > > >
> > > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > > > >  4 files changed, 102 insertions(+)
> > > > > >
> > > > >
> > > > > Adam,
> > > > >
> > > > > That's for the patches!
> > > > >
> > > > > I tested just this series on top of v5.16-rc3 on an
> > > > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > > > is called for VPUMIX pd :
> > > > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > > > ...
> > > > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > >
> > > > > I added prints in imx_pgc_power_{up,down} and
> > > > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > > > ...
> > > > > Ran 55/61 tests successfully               in 8.685 secs
> > > > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > > > ********************************************************************************
> > > > > ********************
> > > > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > > > Using 4 parallel job(s)
> > > > > ********************************************************************************
> > > > > ********************
> > > > >
> > > > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > > > ESULT
> > > > > ----------------------------------------------------------------------
> > > > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > > > [ 1023.119669] imx_pgc_power_up vpumix
> > > > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > >
> > > > > While this wouldn't be an issue with this series it does indicate we
> > > > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > > > ideas?
> > > > >
> > > > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > > > domains" applied when running those tests? It has only recently been
> > > > picked up by Shawn and may have an influence on the bus domain
> > > > behavior.
> > > >
> > >
> > > Lucas,
> > >
> > > Good point. I did have that originally before I started pruning down
> > > to the bare minimum to reproduce the issue.
> > >
> > > I added it back and now I have the following:
> > > arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > media: hantro: Add support for i.MX8M Mini
> > > soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> > > soc: imx: gpcv2: Synchronously suspend MIX domains
> > > Linux 5.16-rc3
> > >
> > > Here's the latest with that patch:
> > > ...
> > > [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> > > vp80-00-comprehensive-007 ... Success
> > > [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> > > [  316.636908] imx_pgc_power_down vpu-g1
> > > [  316.640983] imx_pgc_power_down vpumix
> > > [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> > > [  316.761360] imx_pgc_power_up vpumix
> > > [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > ^^^ hang
> >
> > Hm, I wonder if there's some broken error handling here somewhere, as a
> > failure to power up a domain shouldn't lead to a hang.
> >
> > However, that doesn't explain why the PGC isn't completing the request.
> > Can you try to extend the timeout some more. Even though I think that
> > 1msec should already be generous. Can you dump the content of the
> > GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
> > registers, when the failure condition is hit?
> >

I haven't been able to repeat your findings on G1, but when testing
VP9 on the G2 decoder, I get the following:

[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-07-frame_parallel.webm                   ... Success
[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-07-frame_parallel-1.webm                 ... Success
[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-08-tile_1x4_frame_parallel.webm          ... Timeout
[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-08-tile_1x2.webm                         ... Timeout
[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-02-size-lf-1920x1080.webm                ... Timeout
[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-08-tile_1x2_frame_parallel.webm          ... Timeout
[  192.971101] cma: cma_alloc: reserved: alloc failed, req-size: 3548
pages, ret: -12
[  192.979748] hantro-vpu 38310000.video-codec: dma alloc of size
14532608 failed
[  192.988683] cma: cma_alloc: reserved: alloc failed, req-size: 938
pages, ret: -12
[VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
vp90-2-08-tile_1x8.webm                         ... Error
[  195.296712] imx-pgc imx-pgc-domain.6: failed to command PGC
[  195.302396] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain

At some point the G2 times out, starts to choke on memory, then the
power domain fails, and then the system hangs.  I have a heartbeat
GPIO running so I know if the kernel is alive or not.  The heartbeat
stops, so I know it's locked up tightly.
I guess the good news is that the G2 decoder is able to run 100-ish
successful tests before it falls down.

Either way, I'll post a RFC V3 for the Hantro stuff, but I wonder if
the order of operations on the power-domain powerdown is an issue.

adam

>
> Adam,
>
> Adding keep_clocks=true to VPUG1/VPUG2 domains did not help
>
> Lucas,
>
> I bumped the regmap_read_poll_timeout timeouts from 1m to 100ms and
> still saw the same issue.
>
> Here's some added debugging to show the regs:
> [  648.037903] imx8m_blk_ctrl_power_on vpublk-g1
> [  648.042346] imx_pgc_power_up vpumix
> [  648.146178] imx-pgc imx-pgc-domain.6: imx_pgc_power_up: failed to command PGC
> [  648.153355] imx-pgc imx-pgc-domain.6: GPC_PU_PGC_SW_PUP_REQ(0x0f8)=0x00000100
> [  648.162339] imx-pgc imx-pgc-domain.6:
> GPC_A53_PU_PGC_PUP_STATUS0(0x14c)=0x00000000
> [  648.169988] imx-pgc imx-pgc-domain.6:
> GPC_A53_PU_PGC_PUP_STATUS1(0x150)=0x00000000
> [  648.177618] imx-pgc imx-pgc-domain.6:
> GPC_A53_PU_PGC_PUP_STATUS2(0x154)=0x00000000
> [  648.185281] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 8176380b02e6..8124a3434655 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -67,6 +67,9 @@
>
>  #define GPC_PU_PGC_SW_PUP_REQ          0x0f8
>  #define GPC_PU_PGC_SW_PDN_REQ          0x104
> +#define GPC_A53_PU_PGC_PUP_STATUS0     0x14c
> +#define GPC_A53_PU_PGC_PUP_STATUS1     0x150
> +#define GPC_A53_PU_PGC_PUP_STATUS2     0x154
>
>  #define IMX7_USB_HSIC_PHY_SW_Pxx_REQ           BIT(4)
>  #define IMX7_USB_OTG2_PHY_SW_Pxx_REQ           BIT(3)
> @@ -224,6 +227,7 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>         u32 reg_val, pgc;
>         int ret;
>
> +printk("%s %s\n", __func__, genpd->name);
>         ret = pm_runtime_get_sync(domain->dev);
>         if (ret < 0) {
>                 pm_runtime_put_noidle(domain->dev);
> @@ -258,9 +262,17 @@ static int imx_pgc_power_up(struct
> generic_pm_domain *genpd)
>                 ret = regmap_read_poll_timeout(domain->regmap,
>                                                GPC_PU_PGC_SW_PUP_REQ, reg_val,
>                                                !(reg_val & domain->bits.pxx),
> -                                              0, USEC_PER_MSEC);
> +                                              0, 100 * USEC_PER_MSEC);
>                 if (ret) {
> -                       dev_err(domain->dev, "failed to command PGC\n");
> +                       dev_err(domain->dev, "%s: failed to command
> PGC\n", __func__);
> +                       if (!regmap_read(domain->regmap,
> GPC_PU_PGC_SW_PUP_REQ, &reg_val))
> +                               dev_err(domain->dev,
> "GPC_PU_PGC_SW_PUP_REQ(0x%03x)=0x%08x\n", GPC_PU_PGC_SW_PUP_REQ,
> reg_val);
> +                       if (!regmap_read(domain->regmap,
> GPC_A53_PU_PGC_PUP_STATUS0, &reg_val))
> +                               dev_err(domain->dev,
> "GPC_A53_PU_PGC_PUP_STATUS0(0x%03x)=0x%08x\n",
> GPC_A53_PU_PGC_PUP_STATUS0, reg_val);
> +                       if (!regmap_read(domain->regmap,
> GPC_A53_PU_PGC_PUP_STATUS1, &reg_val))
> +                               dev_err(domain->dev,
> "GPC_A53_PU_PGC_PUP_STATUS1(0x%03x)=0x%08x\n",
> GPC_A53_PU_PGC_PUP_STATUS1, reg_val);
> +                       if (!regmap_read(domain->regmap,
> GPC_A53_PU_PGC_PUP_STATUS2, &reg_val))
> +                               dev_err(domain->dev,
> "GPC_A53_PU_PGC_PUP_STATUS2(0x%03x)=0x%08x\n",
> GPC_A53_PU_PGC_PUP_STATUS2, reg_val);
>                         goto out_clk_disable;
>                 }
>
> @@ -318,6 +330,7 @@ static int imx_pgc_power_down(struct
> generic_pm_domain *genpd)
>         u32 reg_val, pgc;
>         int ret;
>
> +printk("%s %s\n", __func__, genpd->name);
>         /* Enable reset clocks for all devices in the domain */
>         if (!domain->keep_clocks) {
>                 ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> @@ -335,7 +348,7 @@ static int imx_pgc_power_down(struct
> generic_pm_domain *genpd)
>                 ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
>                                                reg_val,
>                                                !(reg_val & domain->bits.hskack),
> -                                              0, USEC_PER_MSEC);
> +                                              0, 100 * USEC_PER_MSEC);
>                 if (ret) {
>                         dev_err(domain->dev, "failed to power down ADB400\n");
>                         goto out_clk_disable;
> @@ -359,9 +372,9 @@ static int imx_pgc_power_down(struct
> generic_pm_domain *genpd)
>                 ret = regmap_read_poll_timeout(domain->regmap,
>                                                GPC_PU_PGC_SW_PDN_REQ, reg_val,
>                                                !(reg_val & domain->bits.pxx),
> -                                              0, USEC_PER_MSEC);
> +                                              0, 100 * USEC_PER_MSEC);
>                 if (ret) {
> -                       dev_err(domain->dev, "failed to command PGC\n");
> +                       dev_err(domain->dev, "%s: failed to command
> PGC\n", __func__);
>                         goto out_clk_disable;
>                 }
>         }
> @@ -712,6 +725,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
>                         .map = IMX8MM_VPUG1_A53_DOMAIN,
>                 },
>                 .pgc   = BIT(IMX8MM_PGC_VPUG1),
> +               .keep_clocks = true,
>         },
>
>         [IMX8MM_POWER_DOMAIN_VPUG2] = {
> @@ -723,6 +737,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
>                         .map = IMX8MM_VPUG2_A53_DOMAIN,
>                 },
>                 .pgc   = BIT(IMX8MM_PGC_VPUG2),
> +               .keep_clocks = true,
>         },
>
>         [IMX8MM_POWER_DOMAIN_VPUH1] = {
> diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> index 519b3651d1d9..028f38d45892 100644
> --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> @@ -68,6 +68,7 @@ static int imx8m_blk_ctrl_power_on(struct
> generic_pm_domain *genpd)
>         struct imx8m_blk_ctrl *bc = domain->bc;
>         int ret;
>
> +printk("%s %s\n", __func__, genpd->name);
>         /* make sure bus domain is awake */
>         ret = pm_runtime_get_sync(bc->bus_power_dev);
>         if (ret < 0) {
> @@ -119,6 +120,7 @@ static int imx8m_blk_ctrl_power_off(struct
> generic_pm_domain *genpd)
>         const struct imx8m_blk_ctrl_domain_data *data = domain->data;
>         struct imx8m_blk_ctrl *bc = domain->bc;
>
> +printk("%s %s\n", __func__, genpd->name);
>         /* put devices into reset and disable clocks */
>         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
>         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
>
> Tim
  

On Wed, Dec 1, 2021 at 7:07 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Wed, Dec 1, 2021 at 2:04 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Wed, Dec 1, 2021 at 10:37 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Am Mittwoch, dem 01.12.2021 um 10:16 -0800 schrieb Tim Harvey:
> > > > On Wed, Dec 1, 2021 at 9:32 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > >
> > > > > Hi Tim,
> > > > >
> > > > > Am Mittwoch, dem 01.12.2021 um 09:23 -0800 schrieb Tim Harvey:
> > > > > > On Tue, Nov 30, 2021 at 5:33 PM Adam Ford <aford173@gmail.com> wrote:
> > > > > > >
> > > > > > > The i.MX8M has two Hantro video decoders, called G1 and G2 which appear
> > > > > > > to be related to the video decoders used on the i.MX8MQ, but because of
> > > > > > > how the Mini handles the power domains, the VPU driver does not need to
> > > > > > > handle all the functions, nor does it support the post-processor,
> > > > > > > so a new compatible flag is required.
> > > > > > >
> > > > > > > With the suggestion from Hans Verkuil, I was able to get the G2 splat to go away
> > > > > > > with changes to FORCE_MAX_ZONEORDER, but I found I could also set cma=512M, however
> > > > > > > it's unclear to me if that's an acceptable alternative.
> > > > > > >
> > > > > > > At the suggestion of Ezequiel Garcia and Nicolas Dufresne I have some
> > > > > > > results from Fluster. However, the G2 VPU appears to fail most tests.
> > > > > > >
> > > > > > > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > > > > > > Ran 90/135 tests successfully               in 76.431 secs
> > > > > > >
> > > > > > >  ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > > > > > > Ran 55/61 tests successfully               in 21.454 secs
> > > > > > >
> > > > > > > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > > > > > > Ran 0/303 tests successfully               in 20.016 secs
> > > > > > >
> > > > > > > Each day seems to show more and more G2 submissions, and gstreamer seems to be
> > > > > > > still working on the VP9, so I am not sure if I should drop G2 as well.
> > > > > > >
> > > > > > >
> > > > > > > Adam Ford (2):
> > > > > > >   media: hantro: Add support for i.MX8M Mini
> > > > > > >   arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > > > >
> > > > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi   | 41 +++++++++++++++
> > > > > > >  drivers/staging/media/hantro/hantro_drv.c   |  2 +
> > > > > > >  drivers/staging/media/hantro/hantro_hw.h    |  2 +
> > > > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 57 +++++++++++++++++++++
> > > > > > >  4 files changed, 102 insertions(+)
> > > > > > >
> > > > > >
> > > > > > Adam,
> > > > > >
> > > > > > That's for the patches!
> > > > > >
> > > > > > I tested just this series on top of v5.16-rc3 on an
> > > > > > imx8mm-venice-gw73xx-0x and found that if I loop fluster I can end up
> > > > > > getting a hang within 10 to 15 mins or so when imx8m_blk_ctrl_power_on
> > > > > > is called for VPUMIX pd :
> > > > > > while [ 1 ]; do uptime; ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0; done
> > > > > > ...
> > > > > > [  618.838436] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > > [  618.844407] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > > >
> > > > > > I added prints in imx_pgc_power_{up,down} and
> > > > > > imx8m_blk_ctrl_power_{on,off} to get some more context
> > > > > > ...
> > > > > > Ran 55/61 tests successfully               in 8.685 secs
> > > > > >  17:16:34 up 17 min,  0 users,  load average: 3.97, 2.11, 0.93
> > > > > > ********************************************************************************
> > > > > > ********************
> > > > > > Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> > > > > > Using 4 parallel job(s)
> > > > > > ********************************************************************************
> > > > > > ********************
> > > > > >
> > > > > > [TEST SUITE      ] (DECODER                    ) TEST VECTOR               ... R
> > > > > > ESULT
> > > > > > ----------------------------------------------------------------------
> > > > > > [ 1023.114806] imx8m_blk_ctrl_power_on vpublk-g1
> > > > > > [ 1023.119669] imx_pgc_power_up vpumix
> > > > > > [ 1023.124307] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > > > [ 1023.130006] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > > >
> > > > > > While this wouldn't be an issue with this series it does indicate we
> > > > > > still have something racy in blk-ctrl. Can you reproduce this (and if
> > > > > > not what kernel are you based on)? Perhaps you or Lucas have some
> > > > > > ideas?
> > > > > >
> > > > > Did you have "[PATCH] soc: imx: gpcv2: Synchronously suspend MIX
> > > > > domains" applied when running those tests? It has only recently been
> > > > > picked up by Shawn and may have an influence on the bus domain
> > > > > behavior.
> > > > >
> > > >
> > > > Lucas,
> > > >
> > > > Good point. I did have that originally before I started pruning down
> > > > to the bare minimum to reproduce the issue.
> > > >
> > > > I added it back and now I have the following:
> > > > arm64: dts: imx8mm: Enable VPU-G1 and VPU-G2
> > > > media: hantro: Add support for i.MX8M Mini
> > > > soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active
> > > > soc: imx: gpcv2: Synchronously suspend MIX domains
> > > > Linux 5.16-rc3
> > > >
> > > > Here's the latest with that patch:
> > > > ...
> > > > [VP8-TEST-VECTORS] (GStreamer-VP8-V4L2SL-Gst1.0)
> > > > vp80-00-comprehensive-007 ... Success
> > > > [  316.632373] imx8m_blk_ctrl_power_off vpublk-g1
> > > > [  316.636908] imx_pgc_power_down vpu-g1
> > > > [  316.640983] imx_pgc_power_down vpumix
> > > > [  316.756869] imx8m_blk_ctrl_power_on vpublk-g1
> > > > [  316.761360] imx_pgc_power_up vpumix
> > > > [  316.765985] imx-pgc imx-pgc-domain.6: failed to command PGC
> > > > [  316.772743] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> > > > ^^^ hang
> > >
> > > Hm, I wonder if there's some broken error handling here somewhere, as a
> > > failure to power up a domain shouldn't lead to a hang.
> > >
> > > However, that doesn't explain why the PGC isn't completing the request.
> > > Can you try to extend the timeout some more. Even though I think that
> > > 1msec should already be generous. Can you dump the content of the
> > > GPC_PU_PGC_SW_PUP_REQ and GPC_A53_PU_PGC_PUP_STATUSn (all 3 of them)
> > > registers, when the failure condition is hit?
> > >
>
> I haven't been able to repeat your findings on G1, but when testing
> VP9 on the G2 decoder, I get the following:
>
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-07-frame_parallel.webm                   ... Success
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-07-frame_parallel-1.webm                 ... Success
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-08-tile_1x4_frame_parallel.webm          ... Timeout
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-08-tile_1x2.webm                         ... Timeout
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-02-size-lf-1920x1080.webm                ... Timeout
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-08-tile_1x2_frame_parallel.webm          ... Timeout
> [  192.971101] cma: cma_alloc: reserved: alloc failed, req-size: 3548
> pages, ret: -12
> [  192.979748] hantro-vpu 38310000.video-codec: dma alloc of size
> 14532608 failed
> [  192.988683] cma: cma_alloc: reserved: alloc failed, req-size: 938
> pages, ret: -12
> [VP9-TEST-VECTORS] (GStreamer-VP9-V4L2SL-Gst1.0)
> vp90-2-08-tile_1x8.webm                         ... Error
> [  195.296712] imx-pgc imx-pgc-domain.6: failed to command PGC
> [  195.302396] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
>
> At some point the G2 times out, starts to choke on memory, then the
> power domain fails, and then the system hangs.  I have a heartbeat
> GPIO running so I know if the kernel is alive or not.  The heartbeat
> stops, so I know it's locked up tightly.
> I guess the good news is that the G2 decoder is able to run 100-ish
> successful tests before it falls down.
>
> Either way, I'll post a RFC V3 for the Hantro stuff, but I wonder if
> the order of operations on the power-domain powerdown is an issue.
>
> adam
>
> >
> > Adam,
> >
> > Adding keep_clocks=true to VPUG1/VPUG2 domains did not help
> >
> > Lucas,
> >
> > I bumped the regmap_read_poll_timeout timeouts from 1m to 100ms and
> > still saw the same issue.
> >
> > Here's some added debugging to show the regs:
> > [  648.037903] imx8m_blk_ctrl_power_on vpublk-g1
> > [  648.042346] imx_pgc_power_up vpumix
> > [  648.146178] imx-pgc imx-pgc-domain.6: imx_pgc_power_up: failed to command PGC
> > [  648.153355] imx-pgc imx-pgc-domain.6: GPC_PU_PGC_SW_PUP_REQ(0x0f8)=0x00000100
> > [  648.162339] imx-pgc imx-pgc-domain.6:
> > GPC_A53_PU_PGC_PUP_STATUS0(0x14c)=0x00000000
> > [  648.169988] imx-pgc imx-pgc-domain.6:
> > GPC_A53_PU_PGC_PUP_STATUS1(0x150)=0x00000000
> > [  648.177618] imx-pgc imx-pgc-domain.6:
> > GPC_A53_PU_PGC_PUP_STATUS2(0x154)=0x00000000
> > [  648.185281] imx8m-blk-ctrl 38330000.blk-ctrl: failed to power up bus domain
> >

Tim / Lucas,

I was able to run the G2 decoder for 203 seconds using fluster, and it
didn't lock up.  I used a combination of the synchronous suspend, and
I removed the VPU reset reference.
Looking at the NXP power domain controller in both Linux and ATF,
neither appear to be referencing IMX8MQ_RESET_VPU_RESET anywhere.  We
are referencing it in the pgc_vpumix node, so I commented it out.
With those two items changed, I was able to get the G2 operational.
I'm going to post another RFC with the reset in the vpumix removed and
a note to what I am using for a starting point.

Ran 127/303 tests successfully               in 203.873 secs

adam

> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 8176380b02e6..8124a3434655 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -67,6 +67,9 @@
> >
> >  #define GPC_PU_PGC_SW_PUP_REQ          0x0f8
> >  #define GPC_PU_PGC_SW_PDN_REQ          0x104
> > +#define GPC_A53_PU_PGC_PUP_STATUS0     0x14c
> > +#define GPC_A53_PU_PGC_PUP_STATUS1     0x150
> > +#define GPC_A53_PU_PGC_PUP_STATUS2     0x154
> >
> >  #define IMX7_USB_HSIC_PHY_SW_Pxx_REQ           BIT(4)
> >  #define IMX7_USB_OTG2_PHY_SW_Pxx_REQ           BIT(3)
> > @@ -224,6 +227,7 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
> >         u32 reg_val, pgc;
> >         int ret;
> >
> > +printk("%s %s\n", __func__, genpd->name);
> >         ret = pm_runtime_get_sync(domain->dev);
> >         if (ret < 0) {
> >                 pm_runtime_put_noidle(domain->dev);
> > @@ -258,9 +262,17 @@ static int imx_pgc_power_up(struct
> > generic_pm_domain *genpd)
> >                 ret = regmap_read_poll_timeout(domain->regmap,
> >                                                GPC_PU_PGC_SW_PUP_REQ, reg_val,
> >                                                !(reg_val & domain->bits.pxx),
> > -                                              0, USEC_PER_MSEC);
> > +                                              0, 100 * USEC_PER_MSEC);
> >                 if (ret) {
> > -                       dev_err(domain->dev, "failed to command PGC\n");
> > +                       dev_err(domain->dev, "%s: failed to command
> > PGC\n", __func__);
> > +                       if (!regmap_read(domain->regmap,
> > GPC_PU_PGC_SW_PUP_REQ, &reg_val))
> > +                               dev_err(domain->dev,
> > "GPC_PU_PGC_SW_PUP_REQ(0x%03x)=0x%08x\n", GPC_PU_PGC_SW_PUP_REQ,
> > reg_val);
> > +                       if (!regmap_read(domain->regmap,
> > GPC_A53_PU_PGC_PUP_STATUS0, &reg_val))
> > +                               dev_err(domain->dev,
> > "GPC_A53_PU_PGC_PUP_STATUS0(0x%03x)=0x%08x\n",
> > GPC_A53_PU_PGC_PUP_STATUS0, reg_val);
> > +                       if (!regmap_read(domain->regmap,
> > GPC_A53_PU_PGC_PUP_STATUS1, &reg_val))
> > +                               dev_err(domain->dev,
> > "GPC_A53_PU_PGC_PUP_STATUS1(0x%03x)=0x%08x\n",
> > GPC_A53_PU_PGC_PUP_STATUS1, reg_val);
> > +                       if (!regmap_read(domain->regmap,
> > GPC_A53_PU_PGC_PUP_STATUS2, &reg_val))
> > +                               dev_err(domain->dev,
> > "GPC_A53_PU_PGC_PUP_STATUS2(0x%03x)=0x%08x\n",
> > GPC_A53_PU_PGC_PUP_STATUS2, reg_val);
> >                         goto out_clk_disable;
> >                 }
> >
> > @@ -318,6 +330,7 @@ static int imx_pgc_power_down(struct
> > generic_pm_domain *genpd)
> >         u32 reg_val, pgc;
> >         int ret;
> >
> > +printk("%s %s\n", __func__, genpd->name);
> >         /* Enable reset clocks for all devices in the domain */
> >         if (!domain->keep_clocks) {
> >                 ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> > @@ -335,7 +348,7 @@ static int imx_pgc_power_down(struct
> > generic_pm_domain *genpd)
> >                 ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
> >                                                reg_val,
> >                                                !(reg_val & domain->bits.hskack),
> > -                                              0, USEC_PER_MSEC);
> > +                                              0, 100 * USEC_PER_MSEC);
> >                 if (ret) {
> >                         dev_err(domain->dev, "failed to power down ADB400\n");
> >                         goto out_clk_disable;
> > @@ -359,9 +372,9 @@ static int imx_pgc_power_down(struct
> > generic_pm_domain *genpd)
> >                 ret = regmap_read_poll_timeout(domain->regmap,
> >                                                GPC_PU_PGC_SW_PDN_REQ, reg_val,
> >                                                !(reg_val & domain->bits.pxx),
> > -                                              0, USEC_PER_MSEC);
> > +                                              0, 100 * USEC_PER_MSEC);
> >                 if (ret) {
> > -                       dev_err(domain->dev, "failed to command PGC\n");
> > +                       dev_err(domain->dev, "%s: failed to command
> > PGC\n", __func__);
> >                         goto out_clk_disable;
> >                 }
> >         }
> > @@ -712,6 +725,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
> >                         .map = IMX8MM_VPUG1_A53_DOMAIN,
> >                 },
> >                 .pgc   = BIT(IMX8MM_PGC_VPUG1),
> > +               .keep_clocks = true,
> >         },
> >
> >         [IMX8MM_POWER_DOMAIN_VPUG2] = {
> > @@ -723,6 +737,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
> >                         .map = IMX8MM_VPUG2_A53_DOMAIN,
> >                 },
> >                 .pgc   = BIT(IMX8MM_PGC_VPUG2),
> > +               .keep_clocks = true,
> >         },
> >
> >         [IMX8MM_POWER_DOMAIN_VPUH1] = {
> > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > index 519b3651d1d9..028f38d45892 100644
> > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > @@ -68,6 +68,7 @@ static int imx8m_blk_ctrl_power_on(struct
> > generic_pm_domain *genpd)
> >         struct imx8m_blk_ctrl *bc = domain->bc;
> >         int ret;
> >
> > +printk("%s %s\n", __func__, genpd->name);
> >         /* make sure bus domain is awake */
> >         ret = pm_runtime_get_sync(bc->bus_power_dev);
> >         if (ret < 0) {
> > @@ -119,6 +120,7 @@ static int imx8m_blk_ctrl_power_off(struct
> > generic_pm_domain *genpd)
> >         const struct imx8m_blk_ctrl_domain_data *data = domain->data;
> >         struct imx8m_blk_ctrl *bc = domain->bc;
> >
> > +printk("%s %s\n", __func__, genpd->name);
> >         /* put devices into reset and disable clocks */
> >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> >         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> >
> > Tim