Nouveau - Nov 2017 - [RFC PATCH] gr: did you try turning it off and on again.

Fixes secure boot on my gp107. No idea why. Otherwise the GPU enters
complete lockdown after starting the gpccs and fecs with the LS images
loaded.

Signed-off-by: Karol Herbst <kherbst at redhat.com>
---
 drm/nouveau/nvkm/engine/gr/gf100.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c
b/drm/nouveau/nvkm/engine/gr/gf100.c
index 2f8dc107..322d9fa6 100644
--- a/drm/nouveau/nvkm/engine/gr/gf100.c
+++ b/drm/nouveau/nvkm/engine/gr/gf100.c
@@ -1731,8 +1731,15 @@ gf100_gr_init_(struct nvkm_gr *base)
 {
 	struct gf100_gr *gr = gf100_gr(base);
 	struct nvkm_subdev *subdev = &base->engine.subdev;
+	struct nvkm_device *device = subdev->device;
 	u32 ret;
 
+	/* did you try turning it off and on again? Apparently we need this
+	 * on pascal, otherwise secboot will just fail.
+	 */
+	nvkm_mask(device, 0x200, 0x1000, 0x0000);
+	nvkm_mask(device, 0x200, 0x1000, 0x1000);
+
 	nvkm_pmu_pgob(gr->base.engine.subdev.device->pmu, false);
 
 	ret = nvkm_falcon_get(gr->fecs, subdev);
-- 
2.14.3

Hi,

comments inline

On 11/28/17 2:11 PM, Karol Herbst wrote:
> Fixes secure boot on my gp107. No idea why. Otherwise the GPU enters
> complete lockdown after starting the gpccs and fecs with the LS images
> loaded.
>
> Signed-off-by: Karol Herbst <kherbst at redhat.com>
> ---
>   drm/nouveau/nvkm/engine/gr/gf100.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c
b/drm/nouveau/nvkm/engine/gr/gf100.c
> index 2f8dc107..322d9fa6 100644
> --- a/drm/nouveau/nvkm/engine/gr/gf100.c
> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c
> @@ -1731,8 +1731,15 @@ gf100_gr_init_(struct nvkm_gr *base)
>   {
>   	struct gf100_gr *gr = gf100_gr(base);
>   	struct nvkm_subdev *subdev = &base->engine.subdev;
> +	struct nvkm_device *device = subdev->device;
>   	u32 ret;
>   
> +	/* did you try turning it off and on again? Apparently we need this
> +	 * on pascal, otherwise secboot will just fail.

The comments about the off and on looks silly, at least put it in 
quotation marks, or rewrite it, e.g. "Apparently we need to turn it off 
and on for the pascal generation, otherwise secboot will just fail."
> +	 */
> +	nvkm_mask(device, 0x200, 0x1000, 0x0000);
> +	nvkm_mask(device, 0x200, 0x1000, 0x1000);
> +

It is needed with pascal, but does it harm other generations calling 
this init? Maybe guard it against exectution on maxwell


Greetings,

Tobias

>   	nvkm_pmu_pgob(gr->base.engine.subdev.device->pmu, false);
>   
>   	ret = nvkm_falcon_get(gr->fecs, subdev);

On Tue, Nov 28, 2017 at 4:36 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:
> Hi,
>
> comments inline
>
> On 11/28/17 2:11 PM, Karol Herbst wrote:
>>
>> Fixes secure boot on my gp107. No idea why. Otherwise the GPU enters
>> complete lockdown after starting the gpccs and fecs with the LS images
>> loaded.
>>
>> Signed-off-by: Karol Herbst <kherbst at redhat.com>
>> ---
>>   drm/nouveau/nvkm/engine/gr/gf100.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c
>> b/drm/nouveau/nvkm/engine/gr/gf100.c
>> index 2f8dc107..322d9fa6 100644
>> --- a/drm/nouveau/nvkm/engine/gr/gf100.c
>> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c
>> @@ -1731,8 +1731,15 @@ gf100_gr_init_(struct nvkm_gr *base)
>>   {
>>         struct gf100_gr *gr = gf100_gr(base);
>>         struct nvkm_subdev *subdev = &base->engine.subdev;
>> +       struct nvkm_device *device = subdev->device;
>>         u32 ret;
>>   +     /* did you try turning it off and on again? Apparently we need
>> this
>> +        * on pascal, otherwise secboot will just fail.
>
>
>
> The comments about the off and on looks silly, at least put it in quotation
> marks, or rewrite it, e.g. "Apparently we need to turn it off and on
for the
> pascal generation, otherwise secboot will just fail."
>
right... I wasn't entirely serious about this one though. Just a
little annoyed, that this was just a painful issue, because it just
doesn't make any sense at all in the end. And because this is
involving secure boot here, who knows how many bugs of this kind are
still there?
>> +        */
>> +       nvkm_mask(device, 0x200, 0x1000, 0x0000);
>> +       nvkm_mask(device, 0x200, 0x1000, 0x1000);
>> +
>
>
>
> It is needed with pascal, but does it harm other generations calling this
> init? Maybe guard it against exectution on maxwell
>
Maybe, maybe not? I think it is fairly safe, but I kind of hoped
somebody with more knowledge about these kind of things would respond
and tells us this has to be done or maybe not.
>
> Greetings,
>
> Tobias
>
>
>
>>         nvkm_pmu_pgob(gr->base.engine.subdev.device->pmu, false);
>>         ret = nvkm_falcon_get(gr->fecs, subdev);