[PATCH 0/5] drm/etnaviv: Ignore MC bit when checking for runtime suspend

[PATCH 0/5] drm/etnaviv: Ignore MC bit when checking for runtime suspend

[Guido Günther]

At least GC7000 fails to enter runtime suspend for long periods of time since
the MC becomes busy again even when the FE is idle. The rest of the series
makes detecting similar issues easier to debug in the future by checking
all known bits in debugfs and also warning in the EBUSY case.

Tested on GC7000 with a reduced runtime delay of 50ms. Patches are
against next-20200226.

Thanks to Lucas Stach for pointing me in the right direction.

Guido Günther (5):
  drm/etnaviv: Fix typo in comment
  drm/etnaviv: Update idle bits
  drm/etnaviv: Consider all kwnown idle bits in debugfs
  drm/etnaviv: Ignore MC when checking runtime suspend idleness
  drm/etnaviv: Warn when GPU doesn't idle fast enough

 drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 26 ++++++++++++++++++++++----
 drivers/gpu/drm/etnaviv/state_hi.xml.h |  7 +++++++
 2 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.23.0

[PATCH 1/5] drm/etnaviv: Fix typo in comment

[Guido Günther]

Use 'is' instead of 'it' so it becomes a valid sentence and
spell 'resetting' correctly.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 80b99acea1c4..873d9103164d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -506,7 +506,7 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
 		/* read idle register. */
 		idle = gpu_read(gpu, VIVS_HI_IDLE_STATE);
 
-		/* try reseting again if FE it not idle */
+		/* try resetting again if FE is not idle */
 		if ((idle & VIVS_HI_IDLE_STATE_FE) == 0) {
 			dev_dbg(gpu->dev, "FE is not idle\n");
 			continue;
-- 
2.23.0

[PATCH 2/5] drm/etnaviv: Update idle bits

[Guido Günther]

Update the state HI and common header from rnndb commit
commit 19280a95a (rnndb: Update idle bits)

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/etnaviv/state_hi.xml.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/state_hi.xml.h b/drivers/gpu/drm/etnaviv/state_hi.xml.h
index 41d8da2b6f4f..8fe4598395f1 100644
--- a/drivers/gpu/drm/etnaviv/state_hi.xml.h
+++ b/drivers/gpu/drm/etnaviv/state_hi.xml.h
@@ -81,6 +81,13 @@ DEALINGS IN THE SOFTWARE.
 #define VIVS_HI_IDLE_STATE_IM					0x00000200
 #define VIVS_HI_IDLE_STATE_FP					0x00000400
 #define VIVS_HI_IDLE_STATE_TS					0x00000800
+#define VIVS_HI_IDLE_STATE_BL					0x00001000
+#define VIVS_HI_IDLE_STATE_ASYNCFE				0x00002000
+#define VIVS_HI_IDLE_STATE_MC					0x00004000
+#define VIVS_HI_IDLE_STATE_PPA					0x00008000
+#define VIVS_HI_IDLE_STATE_WD					0x00010000
+#define VIVS_HI_IDLE_STATE_NN					0x00020000
+#define VIVS_HI_IDLE_STATE_TP					0x00040000
 #define VIVS_HI_IDLE_STATE_AXI_LP				0x80000000
 
 #define VIVS_HI_AXI_CONFIG					0x00000008
-- 
2.23.0

[PATCH 3/5] drm/etnaviv: Consider all kwnown idle bits in debugfs

[Guido Günther]

We were missing out on some bits the vendor kernel driver knows about.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 873d9103164d..187de610b325 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -930,6 +930,20 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m)
 		seq_puts(m, "\t FP is not idle\n");
 	if ((idle & VIVS_HI_IDLE_STATE_TS) == 0)
 		seq_puts(m, "\t TS is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_BL) == 0)
+		seq_puts(m, "\t BL is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_ASYNCFE) == 0)
+		seq_puts(m, "\t ASYNCFE is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_MC) == 0)
+		seq_puts(m, "\t MC is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_PPA) == 0)
+		seq_puts(m, "\t PPA is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_WD) == 0)
+		seq_puts(m, "\t WD is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_NN) == 0)
+		seq_puts(m, "\t NN is not idle\n");
+	if ((idle & VIVS_HI_IDLE_STATE_TP) == 0)
+		seq_puts(m, "\t TP is not idle\n");
 	if (idle & VIVS_HI_IDLE_STATE_AXI_LP)
 		seq_puts(m, "\t AXI low power mode\n");
 
-- 
2.23.0

[PATCH 4/5] drm/etnaviv: Ignore MC when checking runtime suspend idleness

[Guido Günther]

Without that runtime suspend is often blocked due to
etnaviv_gpu_rpm_suspend() returning -EBUSY since the FE seems to trigger
the MC in its idle loop.

Ignoring the MC bit makes the GPU suspend as expected. This was tested
on GC7000.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 187de610b325..da24e433f82a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1820,8 +1820,9 @@ static int etnaviv_gpu_rpm_suspend(struct device *dev)
 	if (atomic_read(&gpu->sched.hw_rq_count))
 		return -EBUSY;
 
-	/* Check whether the hardware (except FE) is idle */
-	mask = gpu->idle_mask & ~VIVS_HI_IDLE_STATE_FE;
+	/* Check whether the hardware (except FE and MC) is idle */
+	mask = gpu->idle_mask & ~(VIVS_HI_IDLE_STATE_FE |
+				  VIVS_HI_IDLE_STATE_MC);
 	idle = gpu_read(gpu, VIVS_HI_IDLE_STATE) & mask;
 	if (idle != mask)
 		return -EBUSY;
-- 
2.23.0

[PATCH 5/5] drm/etnaviv: Warn when GPU doesn't idle fast enough

[Guido Günther]

If the GPU isn't idle after signalling pm_runtime_mark_last_busy() plus
waiting for the autosuspend delay there's likely something wrong with
the way we check idleness so warn about that.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index da24e433f82a..4fd16b8f8a7a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1824,8 +1824,11 @@ static int etnaviv_gpu_rpm_suspend(struct device *dev)
 	mask = gpu->idle_mask & ~(VIVS_HI_IDLE_STATE_FE |
 				  VIVS_HI_IDLE_STATE_MC);
 	idle = gpu_read(gpu, VIVS_HI_IDLE_STATE) & mask;
-	if (idle != mask)
+	if (idle != mask) {
+		dev_warn_ratelimited(dev, "GPU not yet idle, mask: 0x%08x\n",
+				     idle);
 		return -EBUSY;
+	}
 
 	return etnaviv_gpu_hw_suspend(gpu);
 }
-- 
2.23.0

Re: [PATCH 0/5] drm/etnaviv: Ignore MC bit when checking for runtime suspend

[Lucas Stach]

On Mo, 2020-03-02 at 20:13 +0100, Guido Günther wrote:
> At least GC7000 fails to enter runtime suspend for long periods of time since
> the MC becomes busy again even when the FE is idle. The rest of the series
> makes detecting similar issues easier to debug in the future by checking
> all known bits in debugfs and also warning in the EBUSY case.

Thanks, series applied to etnaviv/next.

> Tested on GC7000 with a reduced runtime delay of 50ms. Patches are
> against next-20200226.

I've already wondered if 200ms is too long, 50ms sounds more
reasonable. Do you have any numbers on the power draw on the i.MX8M
with idle GPU, vs. being fully power gated? 

Regards,
Lucas

> Thanks to Lucas Stach for pointing me in the right direction.
> 
> Guido Günther (5):
>   drm/etnaviv: Fix typo in comment
>   drm/etnaviv: Update idle bits
>   drm/etnaviv: Consider all kwnown idle bits in debugfs
>   drm/etnaviv: Ignore MC when checking runtime suspend idleness
>   drm/etnaviv: Warn when GPU doesn't idle fast enough
> 
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 26 ++++++++++++++++++++++----
>  drivers/gpu/drm/etnaviv/state_hi.xml.h |  7 +++++++
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 

Re: [PATCH 0/5] drm/etnaviv: Ignore MC bit when checking for runtime suspend

[Guido Günther]

Hi Lucas,
On Tue, Mar 03, 2020 at 12:55:04PM +0100, Lucas Stach wrote:
> On Mo, 2020-03-02 at 20:13 +0100, Guido Günther wrote:
> > At least GC7000 fails to enter runtime suspend for long periods of time since
> > the MC becomes busy again even when the FE is idle. The rest of the series
> > makes detecting similar issues easier to debug in the future by checking
> > all known bits in debugfs and also warning in the EBUSY case.
> 
> Thanks, series applied to etnaviv/next.

Thanks!

> 
> > Tested on GC7000 with a reduced runtime delay of 50ms. Patches are
> > against next-20200226.
> 
> I've already wondered if 200ms is too long, 50ms sounds more
> reasonable. Do you have any numbers on the power draw on the i.MX8M
> with idle GPU, vs. being fully power gated?

I don't have good numbers yet but i'll post here once i do.
Cheers,
 -- Guido

> 
> Regards,
> Lucas
> 
> > Thanks to Lucas Stach for pointing me in the right direction.
> > 
> > Guido Günther (5):
> >   drm/etnaviv: Fix typo in comment
> >   drm/etnaviv: Update idle bits
> >   drm/etnaviv: Consider all kwnown idle bits in debugfs
> >   drm/etnaviv: Ignore MC when checking runtime suspend idleness
> >   drm/etnaviv: Warn when GPU doesn't idle fast enough
> > 
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 26 ++++++++++++++++++++++----
> >  drivers/gpu/drm/etnaviv/state_hi.xml.h |  7 +++++++
> >  2 files changed, 29 insertions(+), 4 deletions(-)
> > 
> 

Re: [PATCH 0/5] drm/etnaviv: Ignore MC bit when checking for runtime suspend

[Guido Günther]

Hi,
On Tue, Mar 03, 2020 at 12:55:04PM +0100, Lucas Stach wrote:
> On Mo, 2020-03-02 at 20:13 +0100, Guido Günther wrote:
> > At least GC7000 fails to enter runtime suspend for long periods of time since
> > the MC becomes busy again even when the FE is idle. The rest of the series
> > makes detecting similar issues easier to debug in the future by checking
> > all known bits in debugfs and also warning in the EBUSY case.
> 
> Thanks, series applied to etnaviv/next.
> 
> > Tested on GC7000 with a reduced runtime delay of 50ms. Patches are
> > against next-20200226.
> 
> I've already wondered if 200ms is too long, 50ms sounds more
> reasonable. Do you have any numbers on the power draw on the i.MX8M
> with idle GPU, vs. being fully power gated?

The difference is at least 250mW. It makes a huge difference over here.
We hit
https://lore.kernel.org/dri-devel/20200614064601.7872-1-navid.emamdoost@gmail.com/
recently and you notice instantly when that happens when looking at the
SoC temperature.

Cheers,
 -- Guido
> 
> Regards,
> Lucas
> 
> > Thanks to Lucas Stach for pointing me in the right direction.
> > 
> > Guido Günther (5):
> >   drm/etnaviv: Fix typo in comment
> >   drm/etnaviv: Update idle bits
> >   drm/etnaviv: Consider all kwnown idle bits in debugfs
> >   drm/etnaviv: Ignore MC when checking runtime suspend idleness
> >   drm/etnaviv: Warn when GPU doesn't idle fast enough
> > 
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 26 ++++++++++++++++++++++----
> >  drivers/gpu/drm/etnaviv/state_hi.xml.h |  7 +++++++
> >  2 files changed, 29 insertions(+), 4 deletions(-)
> > 
>