With the main card begin the 9400M we have:

* 0x61c084 = 0x0   on the 9600M GT;
* 0x61c084 = 0x400 on the 9400M.

so only one registration for nv_backlight.


However, after setting under Mac OS the 9600M GT as the main card we get:

* 0x61c084 = 0x400 on the 9600M GT;
* 0x61c084 = 0x400 on the 9400M.

thus leading to a double registration.


The NVidia driver is always forcing POST which sets 0x61c084 to 0x0 on the 9600M GT, solving the problem. Should we do something similar in Nouveau or try some other ways to go around it?

By the way the nv_backlight is useless on this setup as it does nothing: the backlight is controlled by the mux, and thus by the apple_gmux driver. Would it make sense to hide the nv_backlight in this case?

Created attachment 121231 [details]
dmesg: G96 as main, config=NvForcePost=1, debug=PDISP=debug, drm.debug=0xf

Created attachment 121232 [details]
dmesg: G96 as main, debug=PDISP=trace, drm.debug=0xf

Updated to 4.4, and still getting the same issue (which is still solved by using NvForcePost=1).

I'm not sure how backlight is supposed to be managed in a multi-screen (same backlight value set for all screens attached to a card?) and/or multi-gpu situation (one backlight value per card?), but:

* If each card can have it's own backlight configuration, shouldn't the naming of Nouveau's backlight path (/sys/class/nv_backlight) take that into account, to avoid different cards registering to the same backlight entry point?

* When should a backlight interface be registered? If connectors are found, regardless of if any screens are connected to those? Currently Nouveau iterates over all LVDS and eDP connectors of a device, and will register one if the "brightness value" (NV50_PDISP_SOR_PWM_CTL(i)) is different of 0. So if a device has an LVDS connector and a eDP one, both will try to register a backlight under the same name (nv_backlight) if I understand the code correctly.

Anyone who could weight in, Ben or Hans maybe?

Attached two dmesgs, one without the NvForcePost, the other with. It's interesting to see that both see the connectors the same way, but due to NV50_PDISP_SOR_PWM_CTL having a different value (as outlined in comment #1), one will try to register and additional backlight while the other won't.

Hi Pierre,

(In reply to Pierre Moreau from comment #1)
> With the main card begin the 9400M we have:
> 
> * 0x61c084 = 0x0   on the 9600M GT;
> * 0x61c084 = 0x400 on the 9400M.
> 
> so only one registration for nv_backlight.
> 
> 
> However, after setting under Mac OS the 9600M GT as the main card we get:
> 
> * 0x61c084 = 0x400 on the 9600M GT;
> * 0x61c084 = 0x400 on the 9400M.
> 
> thus leading to a double registration.
> 
> 
> The NVidia driver is always forcing POST which sets 0x61c084 to 0x0 on the
> 9600M GT, solving the problem. Should we do something similar in Nouveau or
> try some other ways to go around it?
> 
> By the way the nv_backlight is useless on this setup as it does nothing: the
> backlight is controlled by the mux, and thus by the apple_gmux driver. Would
> it make sense to hide the nv_backlight in this case?

Ok, so I happen to know a bit about the whole mess which is backlight control on PC-like platforms / laptops.

So as you mention in a later comment, we could fix the immediate problem by making the nv_backlight name numbered, but that is not really a good fix.

Ultimately we should show only one backlight interface under /sys/class/backlight, otherwise we are just punting the problem to userspace, which has exactly 0% chance of correctly which backlight interface it should actually use.

So if, as you say, the apple gmux is really controlling the backlight then the right thing to do would be to not register nv_backlight at all on these machines. Note that the apple gmux driver already has code to get other backlight drivers out of the way:

drivers/platform/x86/apple-gmux.c : gmux_probe() :

        /*
         * The backlight situation on Macs is complicated. If the gmux is
         * present it's the best choice, because it always works for
         * backlight control and supports more levels than other options.
         * Disable the other backlight choices.
         */
        acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
        apple_bl_unregister();

In my rewrite / cleanup of drivers/acpi/video_detect.c I've teached the code to automatically unregister any acpi-video backlight interface when the backlight_type changes away from acpi_backlight_video.

Note that the driver/acpi/acpi_video.c backlight code is not necessarily stopped from registering its backlight by the acpi_video_set_dmi_backlight_type() call, since it may have already registered, so the video_detect.c code will actively unregister it do deal with this (module loading ordering problem).

I've been thinking about patching the native (type=BACKLIGHT_RAW, intel, ati and nouveau kms driver) backlight interface code to call into acpi_video_get_backlight_type() and to not register a backlight interface when that returns a value != acpi_backlight_native.

As said this is something which I've been thinking about currently the "native" drivers will always register a backlight interface, and userspace knows to prefer non native type backlight interfaces over those if a non native type is present. But it would be nice to just not register the native interface (and possibly later on
also deregister them if acpi_video_get_backlight_type()'s return value changes, by e.g. a acpi_video_set_dmi_backlight_type() call from the apple-gmux driver).

Hmm, sorry to correct myself here, but since userspace uses (type=BACKLIGHT_RAW) sysfs backlight entries as a last resort,  that actually means that our main problem is the nv_backlight name clash, since the apple gmux driver has type = BACKLIGHT_PLATFORM, so the nv_backlight interface being present is not a problem persé, as any properly writting userspace code should prefer the apple gmux driver.

So we could add a counter to if we've already registered nv_backlight and name the second and later interface we register nv_backlight%d (we should keep just "nv_backlight" for the first one to not break userspace saved backlight settings, etc.).

This seems like a worthwhile fix in general, since we may hit the same problem elsewhere.

Sorry for the long ramble, I've been fixing backlight problems for 2 years now and it is such a big mess ...

TLDR: The best fix is probably to fix the nv_backlight name clash by naming the second and later backlight interfaces registered nv_backlight%d.

Regards,

Hans

To add to this discussion, radeon has a quirk for the MacBookPro8,1 wherein a backlight is *not* registered since this machine has a gmux built in. See radeon_atom_backlight_init  in atombios_encoders.c:

http://cgit.freedesktop.org/~airlied/linux/tree/drivers/gpu/drm/radeon/atombios_encoders.c?h=drm-next#n189

	/* Mac laptops with multiple GPUs use the gmux driver for backlight
	 * so don't register a backlight device
	 */
	if ((rdev->pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE) &&
	    (rdev->pdev->device == 0x6741))
		return;

The code is wrong though in that it tries to recognize gmux by way of the machine's PCI ID. The MacBookPro8,1 was available with multiple different AMD GPUs and we'd need to hardcode all of their PCI IDs. Also, the MacBookPro 11,5 has an AMD GPU and we'd need to add its PCI ID as well.

A more sensible solution, as Pierre suggests, is to detect the presence of gmux and not register a backlight if gmux is found. So basically a quirk specific to all dual GPU MacBook Pros.

This patch adds a handy function to detect the presence of gmux. Unfortunately noone has reviewed the series it belongs to so far:
http://lists.freedesktop.org/archives/dri-devel/2016-January/098403.html

(In reply to Lukas Wunner from comment #6)
> To add to this discussion, radeon has a quirk for the MacBookPro8,1 wherein
> a backlight is *not* registered since this machine has a gmux built in. See
> radeon_atom_backlight_init  in atombios_encoders.c:
> 
> http://cgit.freedesktop.org/~airlied/linux/tree/drivers/gpu/drm/radeon/
> atombios_encoders.c?h=drm-next#n189
> 
> 	/* Mac laptops with multiple GPUs use the gmux driver for backlight
> 	 * so don't register a backlight device
> 	 */
> 	if ((rdev->pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE) &&
> 	    (rdev->pdev->device == 0x6741))
> 		return;
> 
> The code is wrong though in that it tries to recognize gmux by way of the
> machine's PCI ID. The MacBookPro8,1 was available with multiple different
> AMD GPUs and we'd need to hardcode all of their PCI IDs. Also, the
> MacBookPro 11,5 has an AMD GPU and we'd need to add its PCI ID as well.
> 
> A more sensible solution, as Pierre suggests, is to detect the presence of
> gmux and not register a backlight if gmux is found. So basically a quirk
> specific to all dual GPU MacBook Pros.
> 
> This patch adds a handy function to detect the presence of gmux.
> Unfortunately noone has reviewed the series it belongs to so far:
> http://lists.freedesktop.org/archives/dri-devel/2016-January/098403.html

Hi,

I agree that having an apple_gmux_present() helper is present then the current code in the radeon driver, and we could use that in the nouveau driver too. But this really should not be necessary at all. Things should work fine with the radeon / nouveau driver registering their native / BACKLIGHT_RAW backlight interfaces in sysfs, userspace should prefer the apple_gmux sysfs entry over a raw type sysfs entry.

This is how things work on regular x86 laptops too, which often have both an acpi_video backlight interface and a native one, so they have 2 backlight sysfs interfaces too.

With that said I do agree that not registering a native driver at all in these cases is better. So I think your apple_gmux patch + matching patches in the nouveau / ati drivers are a good idea, but I do wonder if
this is really necessary.

Eitherway we should first solve the namespace clash when nouveau tries to register multiple backlights, that may happen in non apple weird setups too, so is something which we should fix anyways IMHO,
and fixing that before not registering a native backlight when the apple_gmux is present allows us to actually test that fix :)

Pierre, can you whip up a patch for the nouveau_bl name clash, and that test that on your affected machine ? Note I understand if you will delay this till after Fosdem :)

Regards,

Hans

Hello Hans and Lukas,

I should be able to write and test that quick patch before FOSDEM, but, we'll see how it turns out. :-)

I agree with Hans about fixing the naming clash first, but then avoid registering the Nouveau interface if apple_gmux is detected. The apple_gmux detection could fail on those laptops, if ever it gets registered after Nouveau.

Thank you for your comments!

Hi Pierre,

> The apple_gmux detection could fail on those laptops, if ever it gets
> registered after Nouveau.

No worries, apple_gmux_present() checks for presence of gmux' ACPI device, so it works even if its driver isn't loaded.

Created attachment 122019 [details] [review]
Hack for registering multiple backlight interfaces

I hacked a little patch to add a counter to "nv_backlight", so that multiple backlight interfaces won't conflict on registering the same file. If the counter is 0, no number is added to "nv_backlight" to keep the current behaviour.

Hi Pierre,

Thanks for working on this. I'm not sure what the nbs suffix stands for, maybe rename it to
bl_interface_nr ?

Chances are that nv50_backlight_init() may run twice at the same time if 2 cards are being probed simultaneously. So I would make bl_interface_nr an atomic_t and use:

int nr = atomic_inc_return(&bl_interface_nr) - 1;

to increment it and get the _NEW_ value. Since this gives you the new value, you should substract 1 as is done above.

Regards,

Hans

Created attachment 122083 [details] [review]
Allow multiple backlight interfaces for Nouveau

Here is a new version of the patch using the atomic as you suggested Hans.
I'll send it to the ML along with another patch using Lukas' `apple_gmux_present()`, since it has been merged in drm-next.

Fixed by db1a0ae21461afa4bc435651a6dd55e0e6ef4a8b.