Nouveau - Sep 2019 - [PATCH 1/7] Revert "ACPI / OSI: Add OEM _OSI string to enable dGPU direct output"

On Thu, 15 Aug 2019 at 07:31, Karol Herbst <kherbst at redhat.com>
wrote:
>
> This reverts commit 28586a51eea666d5531bcaef2f68e4abbd87242c.
>
> The original commit message didn't even make sense. AMD _does_ support
it and
> it works with Nouveau as well.
>
> Also what was the issue being solved here? No references to any bugs and
not
> even explaining any issue at all isn't the way we do things.
>
> And even if it means a muxed design, then the fix is to make it work inside
the
> driver, not adding some hacky workaround through ACPI tricks.
>
> And what out of tree drivers do or do not support we don't care one bit
anyway.
>
I think the reverts should be merged via Rafael's tree as the original
patches went in via there, and we should get them in asap.

Acked-by: Dave Airlie <airlied at redhat.com>
Dave.

> -----Original Message-----
> From: linux-acpi-owner at vger.kernel.org <linux-acpi-owner at
vger.kernel.org> On
> Behalf Of Dave Airlie
> Sent: Wednesday, August 14, 2019 5:48 PM
> To: Karol Herbst
> Cc: LKML; Linux ACPI; dri-devel; nouveau; Rafael J . Wysocki; Alex Hung;
Ben
> Skeggs; Dave Airlie
> Subject: Re: [Nouveau] [PATCH 1/7] Revert "ACPI / OSI: Add OEM _OSI
string to
> enable dGPU direct output"
> 
> On Thu, 15 Aug 2019 at 07:31, Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > This reverts commit 28586a51eea666d5531bcaef2f68e4abbd87242c.
> >
> > The original commit message didn't even make sense. AMD _does_
support it and
> > it works with Nouveau as well.
> >
> > Also what was the issue being solved here? No references to any bugs
and not
> > even explaining any issue at all isn't the way we do things.
> >
> > And even if it means a muxed design, then the fix is to make it work
inside the
> > driver, not adding some hacky workaround through ACPI tricks.
> >
> > And what out of tree drivers do or do not support we don't care
one bit anyway.
> >
> 
> I think the reverts should be merged via Rafael's tree as the original
> patches went in via there, and we should get them in asap.
> 
> Acked-by: Dave Airlie <airlied at redhat.com>
> Dave.
There are definitely going to be regressions on machines in the field with the
in tree drivers by reverting this.  I think we should have an answer for all of
those
before this revert is accepted.

Regarding systems with Intel+NVIDIA, we'll have to work with partners to
collect
some information on the impact of reverting this.

When this is used on a system with Intel+AMD the ASL configures AMD GPU to use
"Hybrid Graphics" when on Windows and "Power Express" and
"Switchable Graphics"
when on Linux.

I feel we need a knob and/or DMI detection to affect the changes that the ASL
normally performs.

On Thu, Aug 15, 2019 at 3:56 PM <Mario.Limonciello at dell.com>
wrote:
>
> > -----Original Message-----
> > From: linux-acpi-owner at vger.kernel.org <linux-acpi-owner at
vger.kernel.org> On
> > Behalf Of Dave Airlie
> > Sent: Wednesday, August 14, 2019 5:48 PM
> > To: Karol Herbst
> > Cc: LKML; Linux ACPI; dri-devel; nouveau; Rafael J . Wysocki; Alex
Hung; Ben
> > Skeggs; Dave Airlie
> > Subject: Re: [Nouveau] [PATCH 1/7] Revert "ACPI / OSI: Add OEM
_OSI string to
> > enable dGPU direct output"
> >
> > On Thu, 15 Aug 2019 at 07:31, Karol Herbst <kherbst at
redhat.com> wrote:
> > >
> > > This reverts commit 28586a51eea666d5531bcaef2f68e4abbd87242c.
> > >
> > > The original commit message didn't even make sense. AMD
_does_ support it and
> > > it works with Nouveau as well.
> > >
> > > Also what was the issue being solved here? No references to any
bugs and not
> > > even explaining any issue at all isn't the way we do things.
> > >
> > > And even if it means a muxed design, then the fix is to make it
work inside the
> > > driver, not adding some hacky workaround through ACPI tricks.
> > >
> > > And what out of tree drivers do or do not support we don't
care one bit anyway.
> > >
> >
> > I think the reverts should be merged via Rafael's tree as the
original
> > patches went in via there, and we should get them in asap.
> >
> > Acked-by: Dave Airlie <airlied at redhat.com>
> > Dave.
>
> There are definitely going to be regressions on machines in the field with
the
> in tree drivers by reverting this.  I think we should have an answer for
all of those
> before this revert is accepted.
>
> Regarding systems with Intel+NVIDIA, we'll have to work with partners
to collect
> some information on the impact of reverting this.
>
> When this is used on a system with Intel+AMD the ASL configures AMD GPU to
use
> "Hybrid Graphics" when on Windows and "Power Express"
and "Switchable Graphics"
> when on Linux.
and what's exactly the difference between those? And what's the actual
issue here?

We already have the PRIME offloading in place and if that's not
enough, we should work on extending it, not adding some ACPI based
workarounds, because that's exactly how that looks like.

Also, was this discussed with anybody involved in the drm subsystem?
>
> I feel we need a knob and/or DMI detection to affect the changes that the
ASL
> normally performs.
Why do we have to do that on a firmware level at all?

On Thu, Aug 15, 2019 at 12:47 AM Dave Airlie <airlied at gmail.com>
wrote:
>
> On Thu, 15 Aug 2019 at 07:31, Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > This reverts commit 28586a51eea666d5531bcaef2f68e4abbd87242c.
> >
> > The original commit message didn't even make sense. AMD _does_
support it and
> > it works with Nouveau as well.
> >
> > Also what was the issue being solved here? No references to any bugs
and not
> > even explaining any issue at all isn't the way we do things.
> >
> > And even if it means a muxed design, then the fix is to make it work
inside the
> > driver, not adding some hacky workaround through ACPI tricks.
> >
> > And what out of tree drivers do or do not support we don't care
one bit anyway.
> >
>
> I think the reverts should be merged via Rafael's tree as the original
> patches went in via there, and we should get them in asap.
+1
> Acked-by: Dave Airlie <airlied at redhat.com>
Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Also fully agreeing with Karol's reply further down, if this doesn't
work we need to improve the drivers, not pile stuff on top in some
ACPI hacks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

On Thursday, August 15, 2019 12:47:35 AM CEST Dave Airlie
wrote:
> On Thu, 15 Aug 2019 at 07:31, Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > This reverts commit 28586a51eea666d5531bcaef2f68e4abbd87242c.
> >
> > The original commit message didn't even make sense. AMD _does_
support it and
> > it works with Nouveau as well.
> >
> > Also what was the issue being solved here? No references to any bugs
and not
> > even explaining any issue at all isn't the way we do things.
> >
> > And even if it means a muxed design, then the fix is to make it work
inside the
> > driver, not adding some hacky workaround through ACPI tricks.
> >
> > And what out of tree drivers do or do not support we don't care
one bit anyway.
> >
> 
> I think the reverts should be merged via Rafael's tree as the original
> patches went in via there, and we should get them in asap.
> 
> Acked-by: Dave Airlie <airlied at redhat.com>
The _OSI strings are to be dropped when all of the needed support is there in
drivers, so they should go away along with the requisite driver changes.

I'm all for dropping then when that's the case, so please feel free to
add ACKs
from me to the patches in question at that point.

Cheers,
Rafael

is there any update on the testing with my patches? On the hardware I
had access to those patches helped, but I can't know if it also helped
on the hardware for which those workarounds where actually added.

On Mon, Aug 19, 2019 at 11:52 AM Rafael J. Wysocki <rjw at rjwysocki.net>
wrote:
>
> On Thursday, August 15, 2019 12:47:35 AM CEST Dave Airlie wrote:
> > On Thu, 15 Aug 2019 at 07:31, Karol Herbst <kherbst at
redhat.com> wrote:
> > >
> > > This reverts commit 28586a51eea666d5531bcaef2f68e4abbd87242c.
> > >
> > > The original commit message didn't even make sense. AMD
_does_ support it and
> > > it works with Nouveau as well.
> > >
> > > Also what was the issue being solved here? No references to any
bugs and not
> > > even explaining any issue at all isn't the way we do things.
> > >
> > > And even if it means a muxed design, then the fix is to make it
work inside the
> > > driver, not adding some hacky workaround through ACPI tricks.
> > >
> > > And what out of tree drivers do or do not support we don't
care one bit anyway.
> > >
> >
> > I think the reverts should be merged via Rafael's tree as the
original
> > patches went in via there, and we should get them in asap.
> >
> > Acked-by: Dave Airlie <airlied at redhat.com>
>
> The _OSI strings are to be dropped when all of the needed support is there
in
> drivers, so they should go away along with the requisite driver changes.
>
that goes beside the point. firmware level workarounds for GPU driver
issues were pushed without consulting with upstream GPU developers.
That's something which shouldn't have happened in the first place. And
yes, I am personally annoyed by the fact, that people know about
issues, but instead of contacting the proper persons and working on a
proper fix, we end up with stupid firmware level workarounds. I can't
see why we ever would have wanted such workarounds in the first place.

And I would be much happier if the next time something like that comes
up, that the drm mailing list will be contacted as well or somebody
involved.

We could have also just disable the feature inside the driver (and
probably we should have done that a long time ago, so that is
essentially our fault, but still....)
> I'm all for dropping then when that's the case, so please feel free
to add ACKs
> from me to the patches in question at that point.
>
> Cheers,
> Rafael
>
>
>