Bug 31122

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.