radeon kernel driver not suppressing ACPI_VIDEO_NOTIFY_PROBE events when it should

[Alex Deucher]

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.

Alex

>
> This all assumes though that the problem is that radeon_atif_handler()
> does not return NOTIFY_BAD when the event count being 0 (in other words
> a spurious event). It is also possibly that one of the earlier checks in
> radeon_atif_handler() is failing...
>
> I guess that a first step in debugging this would be to ask the reporter
> to run a kernel with some debugging printk-s added to radeon_atif_handler(),
> to see which code-path in radeon_atif_handler we end up in
> (assuming that radeon_atif_handler() gets called at all).
>
> Any suggestions for other debugging printk-s, before I prepare a Fedora
> kernel for the reporter to test?
>
> Regards,
>
> Hans
>