radeon kernel driver not suppressing ACPI_VIDEO_NOTIFY_PROBE events when it should

[Hans de Goede]

Hi,

On 1/6/21 9:38 PM, Alex Deucher wrote:
> On Wed, Jan 6, 2021 at 3:04 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/6/21 8:33 PM, Alex Deucher wrote:
>>> On Wed, Jan 6, 2021 at 1:10 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 1/6/21 6:07 PM, Alex Deucher wrote:
>>>>> On Wed, Jan 6, 2021 at 11:25 AM Hans de Goede <hdegoede at redhat.com> wrote:
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> I get Cc-ed on all Fedora kernel bugs and this one stood out to me:
>>>>>>
>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1911763
>>>>>>
>>>>>> Since I've done a lot of work on the acpi-video code I thought I should
>>>>>> take a look. I've managed to help the user with a kernel-commandline
>>>>>> option which stops video.ko (the acpi-video kernel module) from emitting
>>>>>> key-press events for ACPI_VIDEO_NOTIFY_PROBE events.
>>>>>>
>>>>>> This is on a Dell Vostro laptop with i915/radeon hybrid gfx.
>>>>>>
>>>>>> I was thinking about adding a DMI quirk for this, but from the brief time
>>>>>> that I worked on nouveau (and specifically hybrid gfx setups) I know that
>>>>>> these events get fired on hybrid gfx setups when the discrete GPU is
>>>>>> powered down and something happens which requires the discrete GPUs drivers
>>>>>> attention, like an external monitor being plugged into a connector handled
>>>>>> by the dGPU (note that is not the case here).
>>>>>>
>>>>>> So I took a quick look at the radeon code and the radeon_atif_handler()
>>>>>> function from drivers/gpu/drm/radeon/radeon_acpi.c. When successful that
>>>>>> returns NOTIFY_BAD which suppresses the key-press.
>>>>>>
>>>>>> But in various cases it returns NOTIFY_DONE instead which does not
>>>>>> suppress the key-press event. So I think that the spurious key-press events
>>>>>> which the user is seeing should be avoided by this function returning
>>>>>> NOTIFY_BAD.
>>>>>>
>>>>>> Specifically I'm wondering if we should not return
>>>>>> NOTIFY_BAD when count == 0?   I guess this can cause problems if there
>>>>>> are multiple GPUs, but we could check if the acpi-event is for the
>>>>>> pci-device the radeon driver is bound to. This would require changing the
>>>>>> acpi-notify code to also pass the acpi_device pointer as part of the
>>>>>> acpi_bus_event but that should not be a problem.
>>>>>>
>>>>>
>>>>> For A+A PX/HG systems, we'd want the notifications for both the dGPU
>>>>> and the APU since some of the events are relevant to one or the other.
>>>>> ATIF_DGPU_DISPLAY_EVENT is only relevant to the dGPU, while
>>>>> ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST would be possibly relevant to
>>>>> both (if there was a mux), but mainly the APU.
>>>>> ATIF_SYSTEM_POWER_SOURCE_CHANGE_REQUEST would be relevant to both.
>>>>> The other events have extended bits to determine which GPU the event
>>>>> is targeted at.
>>>>
>>>> Right, but AFAIK on hybrid systems there are 2 ACPI video-bus devices,
>>>> one for each of the iGPU and dGPU which is why I suggested passing
>>>> the video-bus acpi_device as extra data in acpi_bus_event and then
>>>> radeon_atif_handler() could check if the acpi_device is the companion
>>>> device of the GPU. This assumes that events for GPU# will also
>>>> originate from (through an ACPI ASL notify call) the ACPI video-bus
>>>> which belongs to that GPU.
>>>
>>> That's not the case.  For PX/HG systems, ATIF is in the iGPU's
>>> namespace, on dGPU only systems, ATIF is in the dGPU's namespace.
>>
>> That assumes and AMD iGPU + AMD dGPU I believe ?  The system on
>> which the spurious ACPI_VIDEO_NOTIFY_PROBE events lead to spurious
>> KEY_SWITCHVIDEOMODE key-presses being reported uses an Intel iGPU
>> with an AMD dGPU. I don't have any hybrid gfx systems available for
>> testing atm, but I believe that in this case there will be 2 ACPI
>> video-busses, one for each GPU.
> 
> I think the ATIF method will be on the iGPU regardless of whether it's
> intel or AMD.

Ok.

>> Note I'm not saying that that means that checking the originating
>> ACPI device is the companion of the GPUs PCI-device is the solution
>> here. But so far all I've heard from you is that that is not the
>> solution, without you offering any alternative ideas / possible
>> solutions to try for filtering out these spurious key-presses.
> 
> Sorry, I'm not really an ACPI expert.  I think returning NOTIFY_BAD is
> fine for this specific case, but I don't know if it will break other
> platforms.

Yes, I'm worried too that it might break other platforms, so that
option is of the table then.

> That said, I don't recall seeing any other similar bugs,
> so maybe this is something specific to this particular laptop.

Ok, the acpi_video.c code already has the option to suppress
key-press reporting based on either a cmdline option, or
a DMI quirk and the reporter of the issue has already confirmed that
the kernel cmdline option works around this. So I will submit a patch
for acpi_video.c to add a DMI quirk for this then. This seems more
of a workaround then a real solution, but it looks like this is
the best which we can do atm.

Regards,

Hans