| Message ID | 20230123113750.462144-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | ACPI: video/apple-gmux: Improve apple-gmux backlight detection | expand |
On Mon, Jan 23, 2023 at 01:38:37PM +0100, Hans de Goede wrote: > On 1/23/23 13:09, Lukas Wunner wrote: > > On Mon, Jan 23, 2023 at 12:37:47PM +0100, Hans de Goede wrote: > > > Some apple laptop models have an ACPI device with a HID of APP000B > > > and that device has an IO resource (so it does not describe the new > > > unsupported MMIO based gmux type), but there actually is no gmux > > > in the laptop at all. > > > > > > This patch-series improves the drivers/acpi/video_detect.c so that > > > it no longer tries to use the non present gmux in this case. > > > > > > Note I'm still waiting for testing feedback from the reporter of > > > this problem. But from the logs the problem is clear > > > (the logs show: "apple_gmux: gmux device not present") > > > > Please provide a link to the original report. I would also like to > > know the exact MacBook model used and I would like to see full dmesg > > output as well as an acpidump. > > I only have a report by private email. This does include full dmesg > output and an acpidump. I will forward this to you in a private > email. > > The reporter describes their model as a macbookpro8,1. > > > What you're saying here is that there's a fake APP000B device present > > in DSDT > > Yes that is exactly what I'm saying. That's a 2011 13" MacBook Pro which indeed does not have dual GPUs. I searched for other affected models and this seems to be more common than I thought: MacBookPro5,4 https://pastebin.com/8Xjq7RhS MacBookPro8,1 https://linux-hardware.org/?probe=e513cfbadb&log=dmesg MacBookPro9,2 https://bugzilla.kernel.org/attachment.cgi?id=278961 MacBookPro10,2 https://lkml.org/lkml/2014/9/22/657 MacBookPro11,2 https://forums.fedora-fr.org/viewtopic.php?id=70142 MacBookPro11,4 https://raw.githubusercontent.com/im-0/investigate-card-reader-suspend-problem-on-mbp11.4/master/test-16/dmesg These are 13" and 15" models from the pre-retina and retina era (2009 - 2015). None of them have dual GPUs. (Only a subset of the 15" and 17" models had dual GPUs.) Apple sloppily included a GMUX device on all of them and it wasn't a problem so far because the gmux driver detects non-presence and bails out, but it throws off the new backlight algorithm. This is really sad. :( Please add a Reported-by to your commits as well as the list I've provided above so that we've got a complete record in the git history. Thanks, Lukas
Hi, On 1/23/23 14:49, Lukas Wunner wrote: > On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: >> --- a/include/linux/apple-gmux.h >> +++ b/include/linux/apple-gmux.h > [...] >> +static inline bool apple_gmux_is_indexed(unsigned long iostart) >> +{ >> + u16 val; >> + >> + outb(0xaa, iostart + 0xcc); >> + outb(0x55, iostart + 0xcd); >> + outb(0x00, iostart + 0xce); >> + >> + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); >> + if (val == 0x55aa) >> + return true; >> + >> + return false; >> +} > > Something like this, and especially the large apple_gmux_detect() below, > should not live in a header file. I understand where you are coming from, but these functions really are not that large. > Why can't apple_gmux.ko just export a detection function which is used > both internally and as a helper by the backlight detection? Both the acpi-video code and the apple-gmux code can be built as modules. So this will break if the acpi-video code get builtin and the apple-gmux code does not. This can be worked around in Kconfig by adding something like: depends on APPLE_GMUX || APPLE_GMUX=n to the ACPI_VIDEO Kconfig bits and then cross our fingers we don't get some Kconfig circular dep thing causing things to error out. The whole idea behind the video-detect.c code is that it does as little as possible to figure out which backlight control method to use. It e.g. on purpose does not depend on the GPU drivers to see if native GPU backlight control is available that would bring in a whole lot of dependencies. So the do not depend on other kernel-modules / driver-code is part of the design of video-detect.c (in so far as it was ever designed, since it also very much has organically grown / evolved into its current code). If we forgo this design principle then we evt would end adding similar Kconfig snippets for each backlight device-type which the video-detect code supports this quickly gets unwieldly. And doing this also means that video.ko now starts depending on not just apple-gmux.ko but also on its dependencies, although in this case that would not bring in any extra dependencies. But for ohter types there might very well be significant dependencies. So waying the cons and pros here, as well as trying to be consistent and not add dependencies on other kernel-modules just for detection purposes, I would prefer to keep using the static inline approach for this. Regards, Hans >> >> /** >> - * apple_gmux_present() - detect if gmux is built into the machine >> + * apple_gmux_detect() - detect if gmux is built into the machine >> + * >> + * @pnp_dev: Device to probe or NULL to use the first matching device >> + * @indexed_ret: Returns (by reference) if the gmux is indexed or not >> + * >> + * Detect if a supported gmux device is present by actually probing it. >> + * This avoids the false positives returned on some models by >> + * apple_gmux_present(). >> + * >> + * Return: %true if a supported gmux ACPI device is detected and the kernel >> + * was configured with CONFIG_APPLE_GMUX, %false otherwise. >> + */ >> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) >> +{ >> + u8 ver_major, ver_minor, ver_release; >> + struct resource *res; >> + bool indexed = false; >> + >> + if (!pnp_dev) { >> + struct acpi_device *adev; >> + struct device *dev; >> + >> + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); >> + if (!adev) >> + return false; >> + >> + dev = acpi_get_first_physical_node(adev); >> + if (!dev) >> + return false; >> + >> + pnp_dev = to_pnp_dev(dev); >> + } >> + >> + res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0); >> + if (!res) >> + return false; >> + >> + if (resource_size(res) < GMUX_MIN_IO_LEN) >> + return false; >> + >> + /* >> + * Invalid version information may indicate either that the gmux >> + * device isn't present or that it's a new one that uses indexed io. >> + */ >> + ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR); >> + ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR); >> + ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE); >> + if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { >> + indexed = apple_gmux_is_indexed(res->start); >> + if (!indexed) >> + return false; >> + } >> + >> + if (indexed_ret) >> + *indexed_ret = indexed; >> + >> + return true; >> +} >> + >> +/** >> + * apple_gmux_present() - check if gmux ACPI device is present >> * >> * Drivers may use this to activate quirks specific to dual GPU MacBook Pros >> * and Mac Pros, e.g. for deferred probing, runtime pm and backlight. >> * >> - * Return: %true if gmux is present and the kernel was configured >> + * Return: %true if gmux ACPI device is present and the kernel was configured >> * with CONFIG_APPLE_GMUX, %false otherwise. >> */ >> static inline bool apple_gmux_present(void) >> @@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void) >> return false; >> } >> >> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) >> +{ >> + return false; >> +} >> + >> #endif /* !CONFIG_APPLE_GMUX */ >> >> #endif /* LINUX_APPLE_GMUX_H */ >> -- >> 2.39.0 >
> On 23-Jan-2023, at 5:07 PM, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > Some apple laptop models have an ACPI device with a HID of APP000B > and that device has an IO resource (so it does not describe the new > unsupported MMIO based gmux type), but there actually is no gmux > in the laptop at all. > > This patch-series improves the drivers/acpi/video_detect.c so that > it no longer tries to use the non present gmux in this case. > > Note I'm still waiting for testing feedback from the reporter of > this problem. But from the logs the problem is clear > (the logs show: "apple_gmux: gmux device not present") and > the detection code is not changed, just moved so these patches > should be fine, but they can definitely use a good review. > > Aditya, can you perhaps test this on a model macbook which does > actually use the apple-gmux driver for backlight control > (assuming you have such a model) ? > Hi Hans Since I own a T2 MacBook, I’ll have to apply some out of tree patches as well from https://github.com/Redecorating/linux/commits/bits/010-gmux. I’ve requested the maintainer although he believes they shouldn't break anything on T2 Macs. I’ve still let a kernel compile without the out of tree patches, just with yours and shall send the outputs by tomorrow.
Hi, On 1/23/23 15:23, Lukas Wunner wrote: > On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote: >> On 1/23/23 14:49, Lukas Wunner wrote: >>> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: >>>> --- a/include/linux/apple-gmux.h >>>> +++ b/include/linux/apple-gmux.h >>> [...] >>>> +static inline bool apple_gmux_is_indexed(unsigned long iostart) >>>> +{ >>>> + u16 val; >>>> + >>>> + outb(0xaa, iostart + 0xcc); >>>> + outb(0x55, iostart + 0xcd); >>>> + outb(0x00, iostart + 0xce); >>>> + >>>> + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); >>>> + if (val == 0x55aa) >>>> + return true; >>>> + >>>> + return false; >>>> +} >>> >>> Something like this, and especially the large apple_gmux_detect() below, >>> should not live in a header file. >> >> I understand where you are coming from, but these functions really >> are not that large. >> >>> Why can't apple_gmux.ko just export a detection function which is used >>> both internally and as a helper by the backlight detection? >> >> Both the acpi-video code and the apple-gmux code can be built as >> modules. So this will break if the acpi-video code get builtin >> and the apple-gmux code does not. >> >> This can be worked around in Kconfig by adding something like: >> >> depends on APPLE_GMUX || APPLE_GMUX=n >> >> to the ACPI_VIDEO Kconfig bits and then cross our fingers >> we don't get some Kconfig circular dep thing causing things >> to error out. > > Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is? > > Would making APPLE_GMUX depend on ACPI_VIDEO (instead of > "ACPI_VIDEO=n || ACPI_VIDEO") achieve that? I believe > APPLE_GMUX would then inherit the setting of ACPI_VIDEO? I'm afraid that won't work, make it depend on ACPI_VIDEO would not make it inherit ACPI_VIDEO's setting instead it would be limited to ACPI_VIDEO's setting. So if we make APPLE_GMUX "depends on ACPI_VIDEO" and ACPI_VIDEO=y then APPLE_GMUX can be both Y or M, where as if ACPI_VIDEO=m then APPLE_GMUX can only be m. Note that the APPLE_GMUX Kconfig "ACPI_VIDEO=n || ACPI_VIDEO" bit is obsolete and should be dropped (I have already prepared a patch for this), the apple_gmux code no longer depends on any of the ACPI_VIDEO symbols. Which does make it possible to add a dependency the other way. I just tried the following: --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -210,6 +210,8 @@ config ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE depends on INPUT depends on ACPI_WMI || !X86 + # ACPI_VIDEO uses symbols from APPLE_GMUX if that is enabled + depends on APPLE_GMUX || APPLE_GMUX=n select THERMAL help This driver implements the ACPI Extensions For Display Adapters And that does not cause any circular dep issues it seems, so If we really want to have the detection code inside apple_gmux then we could use the above and have the acpi-video code depend on apple_gmux.ko. I'm not a fan of that though, as mentioned before the intent for the acpi-video code's detection parts is to be as much standalone code as possible. Regards, Hans
Hi, On 1/23/23 16:05, Hans de Goede wrote: > Hi, > > On 1/23/23 15:23, Lukas Wunner wrote: >> On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote: >>> On 1/23/23 14:49, Lukas Wunner wrote: >>>> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: >>>>> --- a/include/linux/apple-gmux.h >>>>> +++ b/include/linux/apple-gmux.h >>>> [...] >>>>> +static inline bool apple_gmux_is_indexed(unsigned long iostart) >>>>> +{ >>>>> + u16 val; >>>>> + >>>>> + outb(0xaa, iostart + 0xcc); >>>>> + outb(0x55, iostart + 0xcd); >>>>> + outb(0x00, iostart + 0xce); >>>>> + >>>>> + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); >>>>> + if (val == 0x55aa) >>>>> + return true; >>>>> + >>>>> + return false; >>>>> +} >>>> >>>> Something like this, and especially the large apple_gmux_detect() below, >>>> should not live in a header file. >>> >>> I understand where you are coming from, but these functions really >>> are not that large. >>> >>>> Why can't apple_gmux.ko just export a detection function which is used >>>> both internally and as a helper by the backlight detection? >>> >>> Both the acpi-video code and the apple-gmux code can be built as >>> modules. So this will break if the acpi-video code get builtin >>> and the apple-gmux code does not. >>> >>> This can be worked around in Kconfig by adding something like: >>> >>> depends on APPLE_GMUX || APPLE_GMUX=n >>> >>> to the ACPI_VIDEO Kconfig bits and then cross our fingers >>> we don't get some Kconfig circular dep thing causing things >>> to error out. >> >> Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is? >> >> Would making APPLE_GMUX depend on ACPI_VIDEO (instead of >> "ACPI_VIDEO=n || ACPI_VIDEO") achieve that? I believe >> APPLE_GMUX would then inherit the setting of ACPI_VIDEO? > > I'm afraid that won't work, make it depend on ACPI_VIDEO would not > make it inherit ACPI_VIDEO's setting instead it would be limited > to ACPI_VIDEO's setting. > > So if we make APPLE_GMUX "depends on ACPI_VIDEO" and ACPI_VIDEO=y > then APPLE_GMUX can be both Y or M, where as if ACPI_VIDEO=m > then APPLE_GMUX can only be m. > > Note that the APPLE_GMUX Kconfig "ACPI_VIDEO=n || ACPI_VIDEO" > bit is obsolete and should be dropped (I have already prepared > a patch for this), the apple_gmux code no longer depends on any > of the ACPI_VIDEO symbols. Which does make it possible to > add a dependency the other way. > > I just tried the following: > > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -210,6 +210,8 @@ config ACPI_VIDEO > depends on BACKLIGHT_CLASS_DEVICE > depends on INPUT > depends on ACPI_WMI || !X86 > + # ACPI_VIDEO uses symbols from APPLE_GMUX if that is enabled > + depends on APPLE_GMUX || APPLE_GMUX=n > select THERMAL > help > This driver implements the ACPI Extensions For Display Adapters > > And that does not cause any circular dep issues it seems And 10 seconds after hitting send I realized that things are not this simple, because a bunch of other Kconfig bits do "select ACPI_VIDEO" and Kconfig bits doing "select FOO" much make sure all the dependencies of FOO are met and we have just added a new dependency... So all those other places then would need something similar. Kconfig is great, but with intra dependencies it really can get quite hairy. So IMHO the (really not that big) static inline detect function really is the best solution here. Regards, Hans
On Mon, Jan 23, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Add a new (static inline) apple_gmux_detect() helper to apple-gmux.h > which can be used for gmux detection instead of apple_gmux_present(). > > The latter is not really reliable since an ACPI device with a HID > of APP000B is present on some devices without a gmux at all, as well > as on devices with a newer (unsupported) MMIO based gmux model. > > This causes apple_gmux_present() to return false-postives on positives > a number of different Apple laptop models. > > This new helper uses the same probing as the actual apple-gmux > driver, so that it does not return false positives. > > To avoid code duplication the gmux_probe() function of the actual > driver is also moved over to using the new apple_gmux_detect() helper. ... > + if (!apple_gmux_detect(pnp, &indexed)) { > + pr_info("gmux device not present\n"); You may start using dev_info(&pnp->dev, ...) if I'm not mistaken. > + return -ENODEV; > + } ... > +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) > +{ > + u8 ver_major, ver_minor, ver_release; > + struct resource *res; > + bool indexed = false; > + > + if (!pnp_dev) { > + struct acpi_device *adev; > + struct device *dev; > + > + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); > + if (!adev) > + return false; > + > + dev = acpi_get_first_physical_node(adev); > + if (!dev) I remember I saw something like this in your tree(?). I hope it's not pending upstream (yet) because of a leak here. Don't forget to call acpi_dev_put() after you finish with adev. Recently I have fixed a bunch of similar issues in ASoC Intel. > + return false; > + > + pnp_dev = to_pnp_dev(dev); > + } > + > + res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0); > + if (!res) > + return false; > + > + if (resource_size(res) < GMUX_MIN_IO_LEN) > + return false; > + > + /* > + * Invalid version information may indicate either that the gmux > + * device isn't present or that it's a new one that uses indexed io. > + */ > + ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR); > + ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR); > + ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE); > + if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { > + indexed = apple_gmux_is_indexed(res->start); > + if (!indexed) > + return false; > + } > + > + if (indexed_ret) > + *indexed_ret = indexed; > + > + return true; > +}