[RFC] gr: did you try turning it off and on again.

Submitted by Karol Herbst on Nov. 28, 2017, 1:11 p.m.

Details

Message ID 20171128131142.2913-1-kherbst@redhat.com
State New
Headers show
Series "gr: did you try turning it off and on again." ( rev: 1 ) in Nouveau

Not browsing as part of any series.

Commit Message

Karol Herbst Nov. 28, 2017, 1:11 p.m.
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@redhat.com>
---
 drm/nouveau/nvkm/engine/gr/gf100.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

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);

Comments

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@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@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@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);
On 11/29/2017 01:47 PM, Karol Herbst wrote:
> On Tue, Nov 28, 2017 at 4:36 PM, Tobias Klausmann
> <tobias.johannes.klausmann@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@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.
This should already be happening actually, so it's definitely weird that
adding it there explicitly does anything at all...

Can you mmiotrace nouveau for me?

Ben.

> 
>>
>> Greetings,
>>
>> Tobias
>>
>>
>>
>>>         nvkm_pmu_pgob(gr->base.engine.subdev.device->pmu, false);
>>>         ret = nvkm_falcon_get(gr->fecs, subdev);
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>