[PATCH] gpu: host1x: Skip reset assert on Tegra186

[PATCH] gpu: host1x: Skip reset assert on Tegra186

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

On Tegra186, other software components may rely on the kernel to
keep Host1x operational even during suspend. As such, as a quirk,
skip asserting Host1x's reset on Tegra186.

We don't need to keep the clocks enabled, as BPMP ensures the clock
stays on while Host1x is being used. On newer SoC's, the reset line
is inaccessible, so there is no need for the quirk.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/host1x/dev.c | 15 +++++++++------
 drivers/gpu/host1x/dev.h |  1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 42fd504abbcd..89983d7d73ca 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -169,6 +169,7 @@ static const struct host1x_info host1x06_info = {
 	.num_sid_entries = ARRAY_SIZE(tegra186_sid_table),
 	.sid_table = tegra186_sid_table,
 	.reserve_vblank_syncpts = false,
+	.skip_reset_assert = true,
 };
 
 static const struct host1x_sid_entry tegra194_sid_table[] = {
@@ -680,13 +681,15 @@ static int __maybe_unused host1x_runtime_suspend(struct device *dev)
 	host1x_intr_stop(host);
 	host1x_syncpt_save(host);
 
-	err = reset_control_bulk_assert(host->nresets, host->resets);
-	if (err) {
-		dev_err(dev, "failed to assert reset: %d\n", err);
-		goto resume_host1x;
-	}
+	if (!host->info->skip_reset_assert) {
+		err = reset_control_bulk_assert(host->nresets, host->resets);
+		if (err) {
+			dev_err(dev, "failed to assert reset: %d\n", err);
+			goto resume_host1x;
+		}
 
-	usleep_range(1000, 2000);
+		usleep_range(1000, 2000);
+	}
 
 	clk_disable_unprepare(host->clk);
 	reset_control_bulk_release(host->nresets, host->resets);
diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
index c8e302de7625..9c13e71a31ff 100644
--- a/drivers/gpu/host1x/dev.h
+++ b/drivers/gpu/host1x/dev.h
@@ -116,6 +116,7 @@ struct host1x_info {
 	 * the display driver disables VBLANK increments.
 	 */
 	bool reserve_vblank_syncpts;
+	bool skip_reset_assert;
 };
 
 struct host1x {
-- 
2.42.0

Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186

[Jon Hunter]

On 14/02/2024 11:40, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
> 
> On Tegra186, other software components may rely on the kernel to
> keep Host1x operational even during suspend. As such, as a quirk,
> skip asserting Host1x's reset on Tegra186.
> 
> We don't need to keep the clocks enabled, as BPMP ensures the clock
> stays on while Host1x is being used. On newer SoC's, the reset line
> is inaccessible, so there is no need for the quirk.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>


We should add the fixes tag ...

Fixes: b7c00cdf6df5 ("gpu: host1x: Enable system suspend callbacks")

... because this fixes a suspend regression on Tegra186.

Thierry, would you be able to add the fixes-tag and send out as a fix 
for v6.8? Otherwise ...

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic

Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Wed Feb 14, 2024 at 12:40 PM CET, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
>
> On Tegra186, other software components may rely on the kernel to
> keep Host1x operational even during suspend. As such, as a quirk,
> skip asserting Host1x's reset on Tegra186.

This all sounds a bit vague. What other software components rely on the
kernel to keep host1x operational during suspend? And why do they do so?
Why is this not a problem elsewhere?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186

[Mikko Perttunen]

On 2/16/24 19:02, Thierry Reding wrote:
> On Wed Feb 14, 2024 at 12:40 PM CET, Mikko Perttunen wrote:
>> From: Mikko Perttunen <mperttunen@nvidia.com>
>>
>> On Tegra186, other software components may rely on the kernel to
>> keep Host1x operational even during suspend. As such, as a quirk,
>> skip asserting Host1x's reset on Tegra186.
> 
> This all sounds a bit vague. What other software components rely on the
> kernel to keep host1x operational during suspend? And why do they do so?
> Why is this not a problem elsewhere?

My assumption is that it's due to a secure world application accessing 
NVDEC or display engines during suspend or resume. This happening 
without kernel knowledge is a bad thing, but it's hard to change at this 
point.

The reset line (CAR vs BPMP vs non-accessible reset line), and the 
secure application code programming this stuff is slightly different in 
every chip generation, which is where I think the differences happen.

Mikko

> 
> Thierry

Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

On Mon Feb 19, 2024 at 3:18 AM CET, Mikko Perttunen wrote:
> On 2/16/24 19:02, Thierry Reding wrote:
> > On Wed Feb 14, 2024 at 12:40 PM CET, Mikko Perttunen wrote:
> >> From: Mikko Perttunen <mperttunen@nvidia.com>
> >>
> >> On Tegra186, other software components may rely on the kernel to
> >> keep Host1x operational even during suspend. As such, as a quirk,
> >> skip asserting Host1x's reset on Tegra186.
> > 
> > This all sounds a bit vague. What other software components rely on the
> > kernel to keep host1x operational during suspend? And why do they do so?
> > Why is this not a problem elsewhere?
>
> My assumption is that it's due to a secure world application accessing 
> NVDEC or display engines during suspend or resume. This happening 
> without kernel knowledge is a bad thing, but it's hard to change at this 
> point.
>
> The reset line (CAR vs BPMP vs non-accessible reset line), and the 
> secure application code programming this stuff is slightly different in 
> every chip generation, which is where I think the differences happen.

*sigh*

I guess it is what it is. Please add a bit more background information
to the commit message and also a comment for the skip_reset field so
that people (including myself) will remember down the road why this
exists.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] gpu: host1x: Skip reset assert on Tegra186

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

On Tegra186, secure world applications may need to access host1x
during suspend/resume, and rely on the kernel to keep Host1x out
of reset during the suspend cycle. As such, as a quirk,
skip asserting Host1x's reset on Tegra186.

We don't need to keep the clocks enabled, as BPMP ensures the clock
stays on while Host1x is being used. On newer SoC's, the reset line
is inaccessible, so there is no need for the quirk.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/host1x/dev.c | 15 +++++++++------
 drivers/gpu/host1x/dev.h |  6 ++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 42fd504abbcd..89983d7d73ca 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -169,6 +169,7 @@ static const struct host1x_info host1x06_info = {
 	.num_sid_entries = ARRAY_SIZE(tegra186_sid_table),
 	.sid_table = tegra186_sid_table,
 	.reserve_vblank_syncpts = false,
+	.skip_reset_assert = true,
 };
 
 static const struct host1x_sid_entry tegra194_sid_table[] = {
@@ -680,13 +681,15 @@ static int __maybe_unused host1x_runtime_suspend(struct device *dev)
 	host1x_intr_stop(host);
 	host1x_syncpt_save(host);
 
-	err = reset_control_bulk_assert(host->nresets, host->resets);
-	if (err) {
-		dev_err(dev, "failed to assert reset: %d\n", err);
-		goto resume_host1x;
-	}
+	if (!host->info->skip_reset_assert) {
+		err = reset_control_bulk_assert(host->nresets, host->resets);
+		if (err) {
+			dev_err(dev, "failed to assert reset: %d\n", err);
+			goto resume_host1x;
+		}
 
-	usleep_range(1000, 2000);
+		usleep_range(1000, 2000);
+	}
 
 	clk_disable_unprepare(host->clk);
 	reset_control_bulk_release(host->nresets, host->resets);
diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
index c8e302de7625..6143c2a61d70 100644
--- a/drivers/gpu/host1x/dev.h
+++ b/drivers/gpu/host1x/dev.h
@@ -116,6 +116,12 @@ struct host1x_info {
 	 * the display driver disables VBLANK increments.
 	 */
 	bool reserve_vblank_syncpts;
+	/*
+	 * On T186, secure world applications may require access to host1x
+	 * during suspend/resume. To allow this, we need to leave host1x
+	 * not in reset.
+	 */
+	bool skip_reset_assert;
 };
 
 struct host1x {
-- 
2.42.0

Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

On Thu Feb 22, 2024 at 2:05 AM CET, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
>
> On Tegra186, secure world applications may need to access host1x
> during suspend/resume, and rely on the kernel to keep Host1x out
> of reset during the suspend cycle. As such, as a quirk,
> skip asserting Host1x's reset on Tegra186.
>
> We don't need to keep the clocks enabled, as BPMP ensures the clock
> stays on while Host1x is being used. On newer SoC's, the reset line
> is inaccessible, so there is no need for the quirk.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/gpu/host1x/dev.c | 15 +++++++++------
>  drivers/gpu/host1x/dev.h |  6 ++++++
>  2 files changed, 15 insertions(+), 6 deletions(-)

Applied to drm-misc-fixes, though I added the Fixes: tag that Jon
mentioned in reply to v1 of this as well as his Reviewed-by and
Tested-by as well, since this is pretty much the same patch except
for the comments.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186

[Mikko Perttunen]

On 2/23/24 03:06, Thierry Reding wrote:
> On Thu Feb 22, 2024 at 2:05 AM CET, Mikko Perttunen wrote:
>> From: Mikko Perttunen <mperttunen@nvidia.com>
>>
>> On Tegra186, secure world applications may need to access host1x
>> during suspend/resume, and rely on the kernel to keep Host1x out
>> of reset during the suspend cycle. As such, as a quirk,
>> skip asserting Host1x's reset on Tegra186.
>>
>> We don't need to keep the clocks enabled, as BPMP ensures the clock
>> stays on while Host1x is being used. On newer SoC's, the reset line
>> is inaccessible, so there is no need for the quirk.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>   drivers/gpu/host1x/dev.c | 15 +++++++++------
>>   drivers/gpu/host1x/dev.h |  6 ++++++
>>   2 files changed, 15 insertions(+), 6 deletions(-)
> 
> Applied to drm-misc-fixes, though I added the Fixes: tag that Jon
> mentioned in reply to v1 of this as well as his Reviewed-by and
> Tested-by as well, since this is pretty much the same patch except
> for the comments.

Thanks. Sorry for messing it up :p

Mikko

> 
> Thanks,
> Thierry