Re: [PATCH v2] drm/nouveau: add a LED driver for the NVIDIA logo

[Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

2016-08-23 16:06 GMT+02:00 Martin Peres <martin.peres@free.fr>:
> On 23/08/16 11:31, Karol Herbst wrote:
>>
>> maybe it makes sense to expose the SLI LED, too.
>>
>> Regardless of my comments this patch is reviewed-by me.
>
>
> You reviewed the wrong patch, I should have named the re-send v3.
>
> I accidentally sent the v1 patch as a v2 :s
>

yeah, mailman was stupid today. I got your new version way too late.
rby still valid though.

>>
>> 2016-08-23 1:39 GMT+02:00 Martin Peres <martin.peres@free.fr>:
>>>
>>> We received a donation of a Titan which has this useless feature
>>> allowing users to control the brightness of the LED behind the
>>> logo of NVIDIA. In the true spirit of open source, let's expose
>>> that to the users of very expensive cards!
>>>
>>> This patch hooks up this LED/PWM to the LED subsystem which allows
>>> blinking it in sync with cpu/disk/network/whatever activity (heartbeat
>>> is quite nice!). Users may also implement some breathing effect or
>>> morse code support in the userspace if they feel like it.
>>>
>>> Signed-off-by: Martin Peres <martin.peres@free.fr>
>>> ---
>>>   drm/nouveau/Kbuild                          |   1 +
>>>   drm/nouveau/include/nvkm/subdev/bios/gpio.h |   1 +
>>>   drm/nouveau/nouveau_drm.c                   |   7 ++
>>>   drm/nouveau/nouveau_drm.h                   |   3 +
>>>   drm/nouveau/nouveau_led.c                   | 131
>>> ++++++++++++++++++++++++++++
>>>   drm/nouveau/nouveau_led.h                   |  48 ++++++++++
>>>   6 files changed, 191 insertions(+)
>>>   create mode 100644 drm/nouveau/nouveau_led.c
>>>   create mode 100644 drm/nouveau/nouveau_led.h
>>>
>>> diff --git a/drm/nouveau/Kbuild b/drm/nouveau/Kbuild
>>> index 2527bf4..312bca9 100644
>>> --- a/drm/nouveau/Kbuild
>>> +++ b/drm/nouveau/Kbuild
>>> @@ -22,6 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o
>>>   nouveau-y += nouveau_drm.o
>>>   nouveau-y += nouveau_hwmon.o
>>>   nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o
>>> +nouveau-y += nouveau_led.o
>>>   nouveau-y += nouveau_nvif.o
>>>   nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o
>>>   nouveau-y += nouveau_usif.o # userspace <-> nvif
>>> diff --git a/drm/nouveau/include/nvkm/subdev/bios/gpio.h
>>> b/drm/nouveau/include/nvkm/subdev/bios/gpio.h
>>> index a47d46d..b7a54e6 100644
>>> --- a/drm/nouveau/include/nvkm/subdev/bios/gpio.h
>>> +++ b/drm/nouveau/include/nvkm/subdev/bios/gpio.h
>>> @@ -6,6 +6,7 @@ enum dcb_gpio_func_name {
>>>          DCB_GPIO_TVDAC1 = 0x2d,
>>>          DCB_GPIO_FAN = 0x09,
>>>          DCB_GPIO_FAN_SENSE = 0x3d,
>>> +       DCB_GPIO_LOGO_LED_PWM = 0x84,
>>>          DCB_GPIO_UNUSED = 0xff,
>>>          DCB_GPIO_VID0 = 0x04,
>>>          DCB_GPIO_VID1 = 0x05,
>>> diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
>>> index d06877d..dc97401 100644
>>> --- a/drm/nouveau/nouveau_drm.c
>>> +++ b/drm/nouveau/nouveau_drm.c
>>> @@ -49,6 +49,7 @@
>>>   #include "nouveau_ttm.h"
>>>   #include "nouveau_gem.h"
>>>   #include "nouveau_vga.h"
>>> +#include "nouveau_led.h"
>>>   #include "nouveau_hwmon.h"
>>>   #include "nouveau_acpi.h"
>>>   #include "nouveau_bios.h"
>>> @@ -468,6 +469,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned
>>> long flags)
>>>          nouveau_hwmon_init(dev);
>>>          nouveau_accel_init(drm);
>>>          nouveau_fbcon_init(dev);
>>> +       nouveau_led_init(dev);
>>>
>>>          if (nouveau_runtime_pm != 0) {
>>>                  pm_runtime_use_autosuspend(dev->dev);
>>> @@ -499,6 +501,7 @@ nouveau_drm_unload(struct drm_device *dev)
>>>          struct nouveau_drm *drm = nouveau_drm(dev);
>>>
>>>          pm_runtime_get_sync(dev->dev);
>>> +       nouveau_led_fini(dev);
>>>          nouveau_fbcon_fini(dev);
>>>          nouveau_accel_fini(drm);
>>>          nouveau_hwmon_fini(dev);
>>> @@ -550,6 +553,8 @@ nouveau_do_suspend(struct drm_device *dev, bool
>>> runtime)
>>>          struct nouveau_cli *cli;
>>>          int ret;
>>>
>>> +       nouveau_led_suspend(dev);
>>> +
>>>          if (dev->mode_config.num_crtc) {
>>>                  NV_INFO(drm, "suspending console...\n");
>>>                  nouveau_fbcon_set_suspend(dev, 1);
>>> @@ -638,6 +643,8 @@ nouveau_do_resume(struct drm_device *dev, bool
>>> runtime)
>>>                  nouveau_fbcon_set_suspend(dev, 0);
>>>          }
>>>
>>> +       nouveau_led_resume(dev);
>>> +
>>>          return 0;
>>>   }
>>>
>>> diff --git a/drm/nouveau/nouveau_drm.h b/drm/nouveau/nouveau_drm.h
>>> index 5c363ed..b148dcb 100644
>>> --- a/drm/nouveau/nouveau_drm.h
>>> +++ b/drm/nouveau/nouveau_drm.h
>>> @@ -166,6 +166,9 @@ struct nouveau_drm {
>>>          struct nouveau_hwmon *hwmon;
>>>          struct nouveau_debugfs *debugfs;
>>>
>>> +       /* led management */
>>> +       struct nouveau_led *led;
>>> +
>>>          /* display power reference */
>>>          bool have_disp_power_ref;
>>>
>>> diff --git a/drm/nouveau/nouveau_led.c b/drm/nouveau/nouveau_led.c
>>> new file mode 100644
>>> index 0000000..3f23ff6
>>> --- /dev/null
>>> +++ b/drm/nouveau/nouveau_led.c
>>> @@ -0,0 +1,131 @@
>>> +/*
>>> + * Copyright (C) 2016 Martin Peres
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining
>>> + * a copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction, including
>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>> + * distribute, sublicense, and/or sell copies of the Software, and to
>>> + * permit persons to whom the Software is furnished to do so, subject to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> + * next paragraph) shall be included in all copies or substantial
>>> + * portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>>> NONINFRINGEMENT.
>>> + * IN NO EVENT SHALL THE COPYRIGHT OWNER(S) AND/OR ITS SUPPLIERS BE
>>> + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>>> ACTION
>>> + * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
>>> + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + */
>>> +
>>> +/*
>>> + * Authors:
>>> + *  Martin Peres <martin.peres@free.fr>
>>> + */
>>> +
>>> +#include <linux/leds.h>
>>> +
>>> +#include "nouveau_drm.h"
>>> +#include "nouveau_led.h"
>>> +#include <nvkm/subdev/gpio.h>
>>> +
>>> +static enum led_brightness
>>> +nouveau_led_get_brightness(struct led_classdev *led)
>>> +{
>>> +       struct drm_device *drm_dev = container_of(led, struct
>>> nouveau_led, led)->dev;
>>> +       struct nouveau_drm *drm = nouveau_drm(drm_dev);
>>> +       struct nvif_object *device = &drm->device.object;
>>> +       u32 div, duty;
>>> +
>>> +       div =  nvif_rd32(device, 0x61c880) & 0x00ffffff;
>>> +       duty = nvif_rd32(device, 0x61c884) & 0x00ffffff;
>>> +
>>> +       return duty * LED_FULL / div;
>>> +}
>>> +
>>> +static void
>>> +nouveau_led_set_brightness(struct led_classdev *led, enum led_brightness
>>> value)
>>> +{
>>> +       struct drm_device *drm_dev = container_of(led, struct
>>> nouveau_led, led)->dev;
>>> +       struct nouveau_drm *drm = nouveau_drm(drm_dev);
>>> +       struct nvif_object *device = &drm->device.object;
>>> +
>>> +       u32 input_clk = 27e6; /* PDISPLAY.SOR[1].PWM is connected to the
>>> crystal */
>>> +       u32 freq = 100; /* this is what nvidia uses and it should be
>>> good-enough */
>>> +       u32 div, duty;
>>> +
>>> +       div = input_clk / freq;
>>> +       duty = value * div / LED_FULL;
>>> +
>>> +       /* for now, this is safe to directly poke those registers
>>> because:
>>> +        *  - A: nvidia never puts the logo led to any other PWM
>>> controler
>>> +        *       than PDISPLAY.SOR[1].PWM.
>>> +        *  - B: nouveau does not touch these registers anywhere else
>>> +        */
>>> +       nvif_wr32(device, 0x61c880, div);
>>> +       nvif_wr32(device, 0x61c884, 0xc0000000 | duty);
>>
>> I feel like there should be a comment explaining the 0x40000000
>> (connected to crystal). Or maybe we should be in general more strict
>> about using constants, I feel like those hardcoded magic numbers are
>> hard to understand by users.
>
>
> Well, true, but as long as it is well documented in rnndb, I think it is the
> proper way to go. Otherwise, we would need to maintain macros which we tried
> and failed at :s
>

well, not everybody knows where to look for stuff. But this is an
issue unrelated to your patch anyway.

> In this particular case, the comments at the top of the function should give
> all the information.
>
> We need to make the use of lookup known:
>
> $ lookup -a E4 0x61c884 0xc0000000
> PDISPLAY.SOR[0x1].PWM_CTL => { DUTY = 0 | CLOCK_SELECT = UNK1 | TRIGGER }
>
> OK, I forgot to update rnnDB :s

upstream or local? The XML looked fine to me, but maybe I looked at
the wrong thing. nvm.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau