Re: [PATCH 14/14] drm/nouveau/therm: Rework nouveau_therm_create()

[Maarten Maathuis <madman2003-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Mon, May 21, 2012 at 8:30 AM, Ben Skeggs <skeggsb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, May 21, 2012 at 12:15:03AM +0100, Emil Velikov wrote:
>> It contains a few changes mainly targeting the following
>>  * Therm table is present in BIT vbios
>>  * Parse the vbios table before hooking temp_get(), as it contains the therm
>> sensor calibration data
>>  * Add dummy_therm_temp_get() function to prevent multiple null dereff's
>
> I didn't take this patch at all yet.  I'll let Martin put his input into
> this instead.  I didn't really touch the thermal stuff aside from matching
> APIs because he's got an overhaul pending anyway.
>
> Comments on specific pieces inline as they may be useful.
>
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/gpu/drm/nouveau/nouveau_pm.c    |    2 +-
>>  drivers/gpu/drm/nouveau/nouveau_therm.c |   63 ++++++++++++++++++++++++-------
>>  2 files changed, 50 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_pm.c b/drivers/gpu/drm/nouveau/nouveau_pm.c
>> index 9dd34fe..1b4422b 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_pm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_pm.c
>> @@ -693,7 +693,7 @@ nouveau_hwmon_init(struct nouveau_device *ndev)
>>       }
>>
>>       /* if the card can read the fan rpm */
>> -     if (nouveau_gpio_func_valid(ndev, DCB_GPIO_FAN_SENSE)) {
>> +     if (pfan && pfan->sense(pfan) >= 0) {
>>               ret = sysfs_create_group(&dev->pdev->dev.kobj,
>>                                        &hwmon_fan_rpm_attrgroup);
>>               if (ret)
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_therm.c b/drivers/gpu/drm/nouveau/nouveau_therm.c
>> index acf81a9..91095be 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_therm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_therm.c
>> @@ -30,6 +30,12 @@
>>  #include "nouveau_pm.h"
>>  #include "nouveau_therm.h"
>>
>> +static inline int
>> +dummy_therm_temp_get(struct nouveau_therm *ptherm)
>> +{
>> +     return 0;
>> +}
>> +
> I don't really like this, if we can't expose any thermal data I think we
> just shouldn't create a thermal subdev?
>
>>  static int
>>  nv40_sensor_setup(struct nouveau_therm *ptherm)
>>  {
>> @@ -181,7 +187,7 @@ nouveau_therm_vbios_parse(struct nouveau_therm *ptherm, u8 *temp)
>>       temp = temp + headerlen;
>>
>>       /* Read the entries from the table */
>> -     for (i = 0; i < entries; i++) {
>> +     for (i = 0; i < entries; i++, temp += recordlen) {
>>               s16 value = ROM16(temp[1]);
>>
>>               switch (temp[0]) {
>> @@ -228,7 +234,6 @@ nouveau_therm_vbios_parse(struct nouveau_therm *ptherm, u8 *temp)
>>                       ptherm->fan.pwm_freq = value;
>>                       break;
>>               }
>> -             temp += recordlen;
>>       }
>>
>>       nouveau_therm_safety_checks(ptherm);
>> @@ -299,6 +304,12 @@ nouveau_therm_probe_i2c(struct nouveau_device *ndev)
>>                            probe_monitoring_device, NV_I2C_DEFAULT(0));
>>  }
>>
>> +static void
>> +nouveau_ptherm_destroy(struct nouveau_device *ndev, int subdev)
>> +{
>> +// XXX: Undo probe_monitoring_device
>> +}
>> +
>>  int
>>  nouveau_therm_create(struct nouveau_device *ndev, int subdev)
>>  {
>> @@ -307,29 +318,53 @@ nouveau_therm_create(struct nouveau_device *ndev, int subdev)
>>       u8 *temp = NULL;
>>       int ret;
>>
>> -     ret = nouveau_subdev_create(ndev, subdev, "THERM", "thermal", &ptherm);
>> -     if (ret)
>> -             return ret;
>> +     if (bios->type == NVBIOS_BIT) {
>> +             if (bit_table(ndev, 'P', &P))
>> +                     return 0;
> The BIT check isn't necessary, bit_table() will fail if it's not a BIT BIOS.
>
>>
>> -     if (ndev->chipset >= 0x40 && ndev->chipset < 0x84)
>> -             ptherm->temp_get = nv40_therm_temp_get;
>> -     else
>> -     if (ndev->chipset <= 0xd9)
>> -             ptherm->temp_get = nv84_therm_temp_get;
>> -
>> -     if (bit_table(ndev, 'P', &P) == 0) {
>>               if (P.version == 1)
>>                       temp = ROMPTR(ndev, P.data[12]);
>>               else
>>               if (P.version == 2)
>>                       temp = ROMPTR(ndev, P.data[16]);
>> -             else
>> +             else {
>>                       NV_WARN(ndev, "unknown temp for BIT P %d\n", P.version);
>> +             }
>> +     } else {
>> +             return 0;
>> +     }
>>
>> -             nouveau_therm_vbios_parse(ptherm, temp);
>> +     if (!temp) {
>> +             NV_DEBUG(ndev, "temp table pointer invalid\n");
>> +             return 0;
>>       }
>>
>> +     ret = nouveau_subdev_create(ndev, subdev, "THERM", "thermal", &ptherm);
>> +     if (ret)
>> +             return ret;
>> +
>> +     nouveau_therm_vbios_parse(ptherm, temp);
>>       nouveau_therm_probe_i2c(ndev);
>>
>> +     ptherm->base.destroy = nouveau_ptherm_destroy;
>> +     switch (ndev->card_type) {
>> +     case NV_40:
>> +     case NV_50:
>> +     case NV_C0:
>> +     case NV_D0:
>> +     case NV_E0:
>> +             if (ndev->chipset < 0x84) {
>> +                     ptherm->temp_get = nv40_therm_temp_get;
>> +                     break;
>> +             } else
>> +             if (ndev->chipset <= 0xd9) {
>> +                     ptherm->temp_get = nv84_therm_temp_get;
>> +                     break;
>> +             }
>> +     default:
>> +             ptherm->temp_get = dummy_therm_temp_get;
>> +             break;
>> +     }
>> +
>>       return nouveau_subdev_init(ndev, subdev, ret);
>>  }
>> --
>> 1.7.10.2
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Does chipset 0x50 really use the NV40 function?

-- 
Far away from the primal instinct, the song seems to fade away, the
river get wider between your thoughts and the things we do and say.