The backlight intensity cannot be controlled using the special Powerbook function keys and neither through Gnome's brightness applet. I don't know how, but the function keys work fine when running the nv driver with Debian stock Linux kernel 2.6.32-5-powerpc (which uses nvidiafb, in case that is important). The Gnome applet, though, doesn't work either; Gnome, however, shows a nice overlay indicating that the brightness changed when the fn-keys are pressed. I ran radeontool as described in bug #29685 and took dumps of the registers while regulating the brightness under nv, in case thats helpful: fn-key brightness level 0 (no backlight): 0x0060081c: 0x86066050 0x000010f0: 0x00000535 fn-key brightness levels 1-8: 0x0060081c: 0x86066051 or 0x86066055 (bit 0x4 flips randomly) 0x000010f0: 0x82450535, 0x82900535, 0x82db0535, 0x83290535, 0x83740535, 0x83bf0535, 0x840d0535, 0x84550535 for levels 1-8, respectively dpms standby at max. brightness (xset dpms force standby): 0x0060081c: 0x86066050 0x000010f0: 0x04550535 I played a bit with radeontool, and bits 0x0fff0000 of 0x10f0 seem continous. The values can actually be set a bit higher (and lower) than level 8 (or level 1). The maximum brightness here is reached at ca. 0x84c00535. The display will also be in maximum brightness when bit 31 of 0x10f0 is not set (as long as bit 1 of 0x60081c is set, otherwise the backlight turns off). The machine is a PowerBook6,2 12", with an nVidia Corporation NV34M [GeForce FX Go5200] [10de:0329] (rev a1). It is running Debian xserver-xorg-video-nouveau 1:0.0.16+git20100825+390f1c8-1 and vanilla Linux 2.6.36. I also filed the related bug #31120 about the backlight not working after hibernation.
This still applies to Debian's 3.0.0-1-powerpc kernel (when the kernel command line prohibits nvidiafb to force nouveau to be used) as does the bug concerning it not being switched back on after hibernation (#31122). Is there any news of a fix or work around for this?
Hi cfr Thank you for trying a recent kernel, but can you please provide a bit more information as per our bugs [1] policy Note that by default if nouveau detects another backlight interface it will not register "ACPI backlight interface available, not registering our own" Thus the brightness control will be left on the hands of the existing interface - ACPI in my case Thanks [1] http://nouveau.freedesktop.org/wiki/Bugs
The problem is that there is no device. The system doesn't recognise the existence of the backlight in some sense. I realise you'd want dmesg output from the 3.1 kernel but I can't provide that right now because it panics on boot. I'll try to get it later.
The powerpc platform has become so old that most current developers do no longer care (and most maintainers have no clue about it...). There is no ACPI on powerpc, so that will not help you. About 3-4 years ago nouveau was working reasonable on powerbooks, since then it has gotten more love from hard-working (paid) RH devs, who's priorities are different than from some of the original group. Not to discredit their work, they come a long way since the original, but much of the newer work breaks the weird powerpc platform. The harddisk of my own model also broke so I also stopped testing things on powerpc. Backlight and dpms on powerpc is done via some registers on the card. You can write directly to them (via radeontool) or make a special driver for it (a patch was posted on lkml, but here is some more recent info about it: http://mac.linux.be/phpBB3/viewtopic.php?f=11&t=365). I remember abstracting the similar code in a separate kernel module which still sits on my powerbook harddisk (it also contained patches against the then current nouveau to make the machine wake from sleep and restore the screen. Perhaps I will get time someday to see how much of that can be salvaged and used with modern kernels, but still there probably only 5 people left that are interested, it has not much priority...).
What do the Fn+X keys do? I assume they just act as a keypress? (And I'm guessing that the nv driver hooks into that somehow... although the only reference to 10f0 that I see is in turning the thing on/off, perhaps it just leaves something untouched that nouveau touches and messes up.) This should be fairly easy to implement... You know what register to control, what the min/max are. I'm guessing it's just 10f0 that matters -- the other register controls whether the CRTC is on/off. It should be easy to stick something into nouveau_backlight.c that deals with your sitation. Instead of keying off of card family, you'd key off of dev->pci_device, like dfp.c:nv04_dfp_update_backlight does. You should give it a shot if you're interested.
No comment after all, did the last comment resolve this?
Created attachment 125547 [details] [review] adds backlight control
Created attachment 125548 [details] [review] adds backlight control
I created a patch for this and it works for me under debian testing on 4.6.4-1-powerpc. I also added a workaround for another bug 31120. So this patch fixes backlight control and backlight after hibernate. I never submitted a patch before. Hopefully this one is ok. I welcome all comments and suggestions.
Nice to see a patch :) However, this patch exposes a backlight even when there is no panel connected. Could you come up with an heuristic to detect if there is or not a backlight? The normal thing to do is to check in the GPIO table of the bios (check it out using envytools's nvbios and nvagetbios). I can assist a little for this.
nvagetbios wasn't able to extract the bios. From what I gathered after searching, openfirmware handles vbios differently (ie: no checksums, smaller vbios, etc). I found a nv34 openfirmware bios someone else dumped and nvbios is unable to decode the GPIO section. However, if the aim is ensuring the device has a backlight, I believe it's already done. In nouveau_backlight_init, it first checks to ensure there's a display on either LVDS or eDP. Can we not reasonably assume that if the gpu has a LVDS or eDP panel attached that it also has a backlight? My init function is a slightly altered version of the existing nv40_backlight_init function which doesn't have a backlight check either.
Created attachment 125561 [details] attachment-18578-0.html You can get vbios once nouveau is loaded from debugfs. It should correspond 1:1 to the of blob as well. On Aug 5, 2016 11:37, <bugzilla-daemon@freedesktop.org> wrote: > *Comment # 11 <https://bugs.freedesktop.org/show_bug.cgi?id=31122#c11> on > bug 31122 <https://bugs.freedesktop.org/show_bug.cgi?id=31122> from A. > Chaud <nvbugz@sparklabs.me> * > > nvagetbios wasn't able to extract the bios. From what I gathered after > searching, openfirmware handles vbios differently (ie: no checksums, smaller > vbios, etc). I found a nv34 openfirmware bios someone else dumped and nvbios > is unable to decode the GPIO section. > > However, if the aim is ensuring the device has a backlight, I believe it's > already done. In nouveau_backlight_init, it first checks to ensure there's a > display on either LVDS or eDP. Can we not reasonably assume that if the gpu > has a LVDS or eDP panel attached that it also has a backlight? My init > function is a slightly altered version of the existing nv40_backlight_init > function which doesn't have a backlight check either. > > ------------------------------ > You are receiving this mail because: > > - You are the assignee for the bug. > > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau > >
Created attachment 125562 [details] [review] backlight control w/ gpio check Thanks for that tip. It was helpful. The rom in debugfs worked in nvbios and this patch has a gpio check. It seems to work but I've no idea whether or not this is correctly implemented. However, I still believe this is unnecessary as the main init function already checks for LVDS and eDP. I copied the GPIO table below for reference: GPIO table at 0x0ac3 version 1.0 GPIO 0: line 1 tag 0x08 [HPD_1] IN GPIO 1: line 2 tag 0x00 [PANEL_BACKLIGHT_ON] OUT param 1 GPIO 2: line 3 tag 0x01 [PANEL_POWER] OUT GPIO 3: line 4 tag 0x0c [TVDAC_0] OUT GPIO 4: line 6 tag 0x13 [???] IN GPIO 5: line 9 tag 0x0a [???] OUT
Comment on attachment 125562 [details] [review] backlight control w/ gpio check Review of attachment 125562 [details] [review]: ----------------------------------------------------------------- Please read the Linux kernel coding style: https://www.kernel.org/doc/Documentation/CodingStyle and adjust your Patch to that (especially the tabs except spaces part). Also there are a few things I really dislike: 1. way too coarse backlight level control. Please consider exposing the full range and remove all the curve handling code. Userspace should handle this _not_ the kernel. It makes the code much cleaner and easier to read. 2. it only works for powerpc. Is there any reason to make it ppc only? more things inline. ::: nouveau_backlight.c.orig @@ +49,5 @@ > + struct nvif_object *device = &drm->device.object; > + int val = (nvif_rd32(device, NV30_PMC_BACKLIGHT) & > + NV30_PMC_BACKLIGHT_MASK) >> 16; > + int i; > + for(i=0;i<sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0]);i++) spaces @@ +54,5 @@ > + { > + if(NV30_BL_CURVE[i] == val) return i; > + } > + > + return sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0]); spaces @@ +66,5 @@ > + struct nvif_object *device = &drm->device.object; > + int val = bd->props.brightness; > + > + nvif_wr32(device, NV30_PMC_BACKLIGHT, > + (NV30_BL_CURVE[val] << 16) + 0x80000535); what does the 0x535 part stands for? Is it to protect against turning off the display by accident? In that case, remove it. @@ +87,5 @@ > + struct backlight_device *bd; > + > + struct dcb_gpio_func func; > + struct nvkm_gpio *gpio = nvxx_gpio(&drm->device); > + int ret = nvkm_gpio_find(gpio, 0,0x00,0xff, &func); spaces @@ +97,5 @@ > + return 0; > + > + memset(&props, 0, sizeof(struct backlight_properties)); > + props.type = BACKLIGHT_RAW; > + props.max_brightness = sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0])-1; spaces
Created attachment 125642 [details] [review] backlight control w/ gpio check Thank you for the comments. This version removes the powerpc ifdef and exposes the full range of brightness values. I also fixed the formatting. Let me know if I missed any of it. Regarding the 0x535. It's a workaround for bug 31120. The lower half of the register gets set to 0x7fff upon waking from suspend which disables the backlight. This changes it back to a working value.
(In reply to A. Chaud from comment #15) > Regarding the 0x535. It's a workaround for bug 31120. The lower half of > the register gets set to 0x7fff upon waking from suspend which disables the > backlight. This changes it back to a working value. There should be a way to save a value on suspend and restore that saved value on resume. BTW, I didn't check whether you're still doing this in the latest patch, but a few things I remember noticing: (a) foo | 0x80000000, not + (b) use ARRAY_SIZE, although I think you got rid of that array anyways
Comment on attachment 125642 [details] [review] backlight control w/ gpio check Review of attachment 125642 [details] [review]: ----------------------------------------------------------------- This looks much better already. There are still a few things: the kernel uses tabs 8 character big. Don't use whitespaces. Other remaining issues inlined. With everything taking care off, you get my rb-by karolherbst@gmail.com ::: nouveau_backlight.c.orig @@ +46,5 @@ > +{ > + struct nouveau_drm *drm = bl_get_data(bd); > + struct nvif_object *device = &drm->device.object; > + int val = (nvif_rd32(device, NV30_PMC_BACKLIGHT) & > + NV30_PMC_BACKLIGHT_MASK) >> 16; indentation, either try to fit it in one line or break the nvif_rd32 command like this: nvif_rd32(arg1, arg2); @@ +64,5 @@ > + int level; > + > + if (val == 0) > + level = 0x00; > + remove empty line here @@ +69,5 @@ > + else > + level = val + NV30_BL_MIN_LEVEL - 1; > + > + nvif_wr32(device, NV30_PMC_BACKLIGHT, > + (level << 16) + 0x80000535); /* workaround for disabled backlight after waking from suspend */ indentation same as above. Also beware the 80 character limit. @@ +101,5 @@ > + return 0; > + > + memset(&props, 0, sizeof(struct backlight_properties)); > + props.type = BACKLIGHT_RAW; > + props.max_brightness = NV30_BL_MAX_LEVEL - NV30_BL_MIN_LEVEL; /* includes an additional 0x00 level (backlight off) */ 80 character limit
Comment on attachment 125642 [details] [review] backlight control w/ gpio check Review of attachment 125642 [details] [review]: ----------------------------------------------------------------- ::: nouveau_backlight.c.orig @@ +69,5 @@ > + else > + level = val + NV30_BL_MIN_LEVEL - 1; > + > + nvif_wr32(device, NV30_PMC_BACKLIGHT, > + (level << 16) + 0x80000535); /* workaround for disabled backlight after waking from suspend */ also like imirkin said: use | instead of + ((level << 16) + 0000535) | 0x80000000
I meant of course reviewed-by, not rb-by
Created attachment 125817 [details] [review] backlight control w/ gpio check I've fixed the formatting. Hopefully this new version is now fully compliant. However I've one question. Is that GPIO check necessary considering the main init function already checks for LVDS and eDP?
Comment on attachment 125817 [details] [review] backlight control w/ gpio check Review of attachment 125817 [details] [review]: ----------------------------------------------------------------- yeah, it is much better now, just a few lines are oddish, comments inlined. With everything fixed you can add this to your patch: Reviewed-by: Karol Herbst <karolherbst@gmail.com> Also please send it to the ML after you are done with the stuff: https://lists.freedesktop.org/mailman/listinfo/nouveau ::: /root/nouveau_backlight.c.orig @@ +47,5 @@ > + struct nouveau_drm *drm = bl_get_data(bd); > + struct nvif_object *device = &drm->device.object; > + int val = (nvif_rd32(device, > + NV30_PMC_BACKLIGHT) & > + NV30_PMC_BACKLIGHT_MASK) >> 16; this line is a bit odd (also contains spaces). Maybe it would be cleaner if you move the rd out of the calculation and do something like that (which looks much cleaner imho): int val = nvif_rd32(device, NV30_PMC_BACKLIGHT); val = (val & NV30_PMC_BACKLIGHT_MASK) >> 16; @@ +68,5 @@ > + level = val + NV30_BL_MIN_LEVEL - 1; > + nvif_wr32(device, > + NV30_PMC_BACKLIGHT, > + ((level << 16) + 0x535) | 0x80000000); > + /* workaround for disabled backlight after waking from suspend */ please move the comment before the code it refers to @@ +99,5 @@ > + > + memset(&props, 0, sizeof(struct backlight_properties)); > + props.type = BACKLIGHT_RAW; > + props.max_brightness = NV30_BL_MAX_LEVEL - NV30_BL_MIN_LEVEL; > + /* includes an additional 0x00 level (backlight off) */ please move the comment before the code it refers to @@ +104,5 @@ > + > + bd = backlight_device_register("nv_backlight", > + connector->kdev, > + drm, > + &nv30_bl_ops, spaces in this line @@ +307,5 @@ > continue; > > switch (device->info.family) { > + case NV_DEVICE_INFO_V0_RANKINE: > + return nv30_backlight_init(connector); both lines have spaces
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/driver/xf86-video-nouveau/issues/10.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.