Nouveau - Nov 2014 - [Bug 86539] New: [NVAC] Trying to register nv_backlight after NV96 did it

https://bugs.freedesktop.org/show_bug.cgi?id=86539

            Bug ID: 86539
           Summary: [NVAC] Trying to register nv_backlight after NV96 did
                    it
           Product: xorg
           Version: git
          Hardware: x86-64 (AMD64)
                OS: Linux (All)
            Status: NEW
          Severity: normal
          Priority: medium
         Component: Driver/nouveau
          Assignee: nouveau at lists.freedesktop.org
          Reporter: pierre.morrow at free.fr
        QA Contact: xorg-team at lists.x.org

Created attachment 109822
  --> https://bugs.freedesktop.org/attachment.cgi?id=109822&action=edit
dmesg

Hardware:
* 9600M GT (NV96)
* 9400M (NVAC)

Tested on
http://cgit.freedesktop.org/~darktama/nouveau/commit/?id=1a9d15cb98ec30dfba43bbe65022f22d978ea9fc

Options: nouveau.noaccel=1 (otherwise falling into bug 86537)

When using the NV96 card to drive the display, the NVAC still tries to register
nv_backlight even though it isn't driving a display and the NV96 already
registered it.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20141121/adb3b9d3/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #1 from Pierre Moreau <pierre.morrow at free.fr> ---
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20141216/81ebbf2d/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

Pierre Moreau <pierre.morrow at free.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109822|0                           |1
        is obsolete|                            |

--- Comment #2 from Pierre Moreau <pierre.morrow at free.fr> ---
Created attachment 121231
  --> https://bugs.freedesktop.org/attachment.cgi?id=121231&action=edit
G96 as main, config=NvForcePost=1, debug=PDISP=debug, drm.debug=0xf

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160123/f1786401/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #3 from Pierre Moreau <pierre.morrow at free.fr> ---
Created attachment 121232
  --> https://bugs.freedesktop.org/attachment.cgi?id=121232&action=edit
G96 as main, debug=PDISP=debug, drm.debug=0xf

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160123/ba820b57/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #4 from Pierre Moreau <pierre.morrow at free.fr> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160123/14b905e2/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

Pierre Moreau <pierre.morrow at free.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121231|G96 as main,                |dmesg: G96 as main,
        description|config=NvForcePost=1,       |config=NvForcePost=1,
                   |debug=PDISP=debug,          |debug=PDISP=debug,
                   |drm.debug=0xf               |drm.debug=0xf

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160123/c882410d/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

Pierre Moreau <pierre.morrow at free.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121232|G96 as main,                |dmesg: G96 as main,
        description|debug=PDISP=debug,          |debug=PDISP=trace,
                   |drm.debug=0xf               |drm.debug=0xf

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160123/5d8a0eb3/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #5 from Hans de Goede <jwrdegoede at fedoraproject.org> ---
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160124/4c1ea824/attachment-0001.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #6 from Lukas Wunner <lukas at wunner.de> ---
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160124/cdec77ec/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #7 from Hans de Goede <jwrdegoede at fedoraproject.org> ---
(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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160126/34770484/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #8 from Pierre Moreau <pierre.morrow at free.fr> ---
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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160126/98d17b11/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

Lukas Wunner <lukas at wunner.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lukas at wunner.de

--- Comment #9 from Lukas Wunner <lukas at wunner.de> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160126/23b9c7b0/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #10 from Pierre Moreau <pierre.morrow at free.fr> ---
Created attachment 122019
  --> https://bugs.freedesktop.org/attachment.cgi?id=122019&action=edit
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20160229/be791a60/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

--- Comment #11 from Hans de Goede <jwrdegoede at fedoraproject.org> ---
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20160229/02865c6d/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

Pierre Moreau <pierre.morrow at free.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122019|0                           |1
        is obsolete|                            |

--- Comment #12 from Pierre Moreau <pierre.morrow at free.fr> ---
Created attachment 122083
  --> https://bugs.freedesktop.org/attachment.cgi?id=122083&action=edit
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20160302/7176ea63/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=86539

Pierre Moreau <pierre.morrow at free.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from Pierre Moreau <pierre.morrow at free.fr> ---
Fixed by db1a0ae21461afa4bc435651a6dd55e0e6ef4a8b.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20170312/b5c7d3c3/attachment.html>