[PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[Tobias Klausmann]

This fixes wrong temperature outputs e.g. 511°C if the card is asleep.

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
index 9f0dea3f61dc..45d0ec632b5a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
@@ -32,8 +32,10 @@ gp100_temp_get(struct nvkm_therm *therm)
 	u32 inttemp = (tsensor & 0x0001fff8);
 
 	/* device SHADOWed */
-	if (tsensor & 0x40000000)
+	if (tsensor & 0x40000000) {
 		nvkm_trace(subdev, "reading temperature from SHADOWed sensor\n");
+		return -ENODEV;
+	}
 
 	/* device valid */
 	if (tsensor & 0x20000000)
-- 
2.16.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[Rhys Kidd]

[-- Attachment #1.1: Type: text/plain, Size: 1452 bytes --]

On 25 January 2018 at 13:16, Tobias Klausmann <
tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org> wrote:

> This fixes wrong temperature outputs e.g. 511°C if the card is asleep.
>
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
>

LGTM,

Reviewed-by: Rhys Kidd <rhyskidd-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


> ---
>  drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> index 9f0dea3f61dc..45d0ec632b5a 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> @@ -32,8 +32,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>         u32 inttemp = (tsensor & 0x0001fff8);
>
>         /* device SHADOWed */
> -       if (tsensor & 0x40000000)
> +       if (tsensor & 0x40000000) {
>                 nvkm_trace(subdev, "reading temperature from SHADOWed
> sensor\n");
> +               return -ENODEV;
> +       }
>
>         /* device valid */
>         if (tsensor & 0x20000000)
> --
> 2.16.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>

[-- Attachment #1.2: Type: text/html, Size: 2609 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[Karol Herbst]

no, we can't do that. We actually have to prevent this from hwom. The
issue here is, that the reg read returns 0xffffffff and parsing that
is the first step in the first place.

On Thu, Jan 25, 2018 at 7:16 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
> This fixes wrong temperature outputs e.g. 511°C if the card is asleep.
>
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> ---
>  drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> index 9f0dea3f61dc..45d0ec632b5a 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
> @@ -32,8 +32,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>         u32 inttemp = (tsensor & 0x0001fff8);
>
>         /* device SHADOWed */
> -       if (tsensor & 0x40000000)
> +       if (tsensor & 0x40000000) {
>                 nvkm_trace(subdev, "reading temperature from SHADOWed sensor\n");
> +               return -ENODEV;
> +       }
>
>         /* device valid */
>         if (tsensor & 0x20000000)
> --
> 2.16.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[Tobias Klausmann]

Not sure if i understand completely what you intend to say here, with 
this we prevent hwmon from reporting utterly wrong temperature values 
returning an error (we could return -EBUSY or somehting instead, 
granted), yet if the device is shadowed, getting a sane temp value out 
of is seems unlikely to me!

Greetings,

Tobias

On 1/26/18 12:40 PM, Karol Herbst wrote:
> no, we can't do that. We actually have to prevent this from hwom. The
> issue here is, that the reg read returns 0xffffffff and parsing that
> is the first step in the first place.
>
> On Thu, Jan 25, 2018 at 7:16 PM, Tobias Klausmann
> <tobias.johannes.klausmann@mni.thm.de> wrote:
>> This fixes wrong temperature outputs e.g. 511°C if the card is asleep.
>>
>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
>> ---
>>   drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>> index 9f0dea3f61dc..45d0ec632b5a 100644
>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>> @@ -32,8 +32,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>>          u32 inttemp = (tsensor & 0x0001fff8);
>>
>>          /* device SHADOWed */
>> -       if (tsensor & 0x40000000)
>> +       if (tsensor & 0x40000000) {
>>                  nvkm_trace(subdev, "reading temperature from SHADOWed sensor\n");
>> +               return -ENODEV;
>> +       }
>>
>>          /* device valid */
>>          if (tsensor & 0x20000000)
>> --
>> 2.16.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[Karol Herbst]

well I just tried to say, that you are not fixing the issue you think
were fixing. In your case the GPU is powered off and you get garbage
values from any mmio read, so parsing those values is just wrong and
we need to prevent doing anything on the hw whenever it is powered off
directly in hwmon.

On Fri, Jan 26, 2018 at 2:40 PM, Tobias Klausmann
<tobias.johannes.klausmann@mni.thm.de> wrote:
> Not sure if i understand completely what you intend to say here, with this
> we prevent hwmon from reporting utterly wrong temperature values returning
> an error (we could return -EBUSY or somehting instead, granted), yet if the
> device is shadowed, getting a sane temp value out of is seems unlikely to
> me!
>
> Greetings,
>
> Tobias
>
>
> On 1/26/18 12:40 PM, Karol Herbst wrote:
>>
>> no, we can't do that. We actually have to prevent this from hwom. The
>> issue here is, that the reg read returns 0xffffffff and parsing that
>> is the first step in the first place.
>>
>> On Thu, Jan 25, 2018 at 7:16 PM, Tobias Klausmann
>> <tobias.johannes.klausmann@mni.thm.de> wrote:
>>>
>>> This fixes wrong temperature outputs e.g. 511°C if the card is asleep.
>>>
>>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
>>> ---
>>>   drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>> b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>> index 9f0dea3f61dc..45d0ec632b5a 100644
>>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>> @@ -32,8 +32,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>>>          u32 inttemp = (tsensor & 0x0001fff8);
>>>
>>>          /* device SHADOWed */
>>> -       if (tsensor & 0x40000000)
>>> +       if (tsensor & 0x40000000) {
>>>                  nvkm_trace(subdev, "reading temperature from SHADOWed
>>> sensor\n");
>>> +               return -ENODEV;
>>> +       }
>>>
>>>          /* device valid */
>>>          if (tsensor & 0x20000000)
>>> --
>>> 2.16.1
>>>
>>> _______________________________________________
>>> Nouveau mailing list
>>> Nouveau@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau/therm/gp100: Do not report temperature when subdev is shadowed

[Tobias Klausmann]

Well fixing the return of wrong values in this function is reasonable by 
any means, of course not reading the mem in the first place would be 
nice, but deciding this is imho not in the scope of a temp_get function 
but somewhere in the code calling temp_get.

On 1/26/18 3:03 PM, Karol Herbst wrote:
> well I just tried to say, that you are not fixing the issue you think
> were fixing. In your case the GPU is powered off and you get garbage
> values from any mmio read, so parsing those values is just wrong and
> we need to prevent doing anything on the hw whenever it is powered off
> directly in hwmon.
>
> On Fri, Jan 26, 2018 at 2:40 PM, Tobias Klausmann
> <tobias.johannes.klausmann@mni.thm.de> wrote:
>> Not sure if i understand completely what you intend to say here, with this
>> we prevent hwmon from reporting utterly wrong temperature values returning
>> an error (we could return -EBUSY or somehting instead, granted), yet if the
>> device is shadowed, getting a sane temp value out of is seems unlikely to
>> me!
>>
>> Greetings,
>>
>> Tobias
>>
>>
>> On 1/26/18 12:40 PM, Karol Herbst wrote:
>>> no, we can't do that. We actually have to prevent this from hwom. The
>>> issue here is, that the reg read returns 0xffffffff and parsing that
>>> is the first step in the first place.
>>>
>>> On Thu, Jan 25, 2018 at 7:16 PM, Tobias Klausmann
>>> <tobias.johannes.klausmann@mni.thm.de> wrote:
>>>> This fixes wrong temperature outputs e.g. 511°C if the card is asleep.
>>>>
>>>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
>>>> ---
>>>>    drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>>> b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>>> index 9f0dea3f61dc..45d0ec632b5a 100644
>>>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/gp100.c
>>>> @@ -32,8 +32,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>>>>           u32 inttemp = (tsensor & 0x0001fff8);
>>>>
>>>>           /* device SHADOWed */
>>>> -       if (tsensor & 0x40000000)
>>>> +       if (tsensor & 0x40000000) {
>>>>                   nvkm_trace(subdev, "reading temperature from SHADOWed
>>>> sensor\n");
>>>> +               return -ENODEV;
>>>> +       }
>>>>
>>>>           /* device valid */
>>>>           if (tsensor & 0x20000000)
>>>> --
>>>> 2.16.1
>>>>
>>>> _______________________________________________
>>>> Nouveau mailing list
>>>> Nouveau@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau