Message-ID: <CAJZ5v0iH3QX3TUU0jEuRsPQNSN84R6t_ZJ85-kSZhiedBSds2A@mail.gmail.com>
Date: Wed, 12 Jun 2024 21:26:36 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Hans de Goede <hdegoede@...hat.com>, 
	Genes Lists <lists@...ience.com>, linux-kernel@...r.kernel.org, mchehab@...nel.org, 
	hverkuil-cisco@...all.nl, laurent.pinchart@...asonboard.com, 
	wentong.wu@...el.com, linux-media@...r.kernel.org, linux-acpi@...r.kernel.org, 
	"regressions@...ts.linux.dev" <regressions@...ts.linux.dev>
Subject: Re: [PATCH 1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes

Hi Sakari,

On Wed, Jun 12, 2024 at 9:11 PM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
>
> Hi Rafael,
>
> On Wed, Jun 12, 2024 at 08:50:57PM +0200, Rafael J. Wysocki wrote:
> > Hi Sakari,
> >
> > On Wed, Jun 12, 2024 at 8:41 PM Sakari Ailus
> > <sakari.ailus@...ux.intel.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Wed, Jun 12, 2024 at 08:29:21PM +0200, Rafael J. Wysocki wrote:
> > > > Hi Sakari,
> > > >
> > > > On Wed, Jun 12, 2024 at 8:21 PM Sakari Ailus
> > > > <sakari.ailus@...ux.intel.com> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus
> > > > > > <sakari.ailus@...ux.intel.com> wrote:
> > > > > > >
> > > > > > > Hi Rafael,
> > > > > > >
> > > > > > > On Wed, Jun 12, 2024 at 02:32:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > > I just hit the same problem on another Dell laptop. It seems that
> > > > > > > > > > > > all Dell laptops with IPU6 camera from the Tiger Lake, Alder Lake
> > > > > > > > > > > > and Raptor Lake generations suffer from this problem.
> > > > > > > > > > > >
> > > > > > > > > > > > So instead of playing whack a mole with DMI matches we should
> > > > > > > > > > > > simply disable ACPI MIPI DISCO support on all Dell laptops
> > > > > > > > > > > > with those CPUs. I'm preparing a fix for this to replace
> > > > > > > > > > > > the DMI matching now.
> > > > > > > > > > >
> > > > > > > > > > > DisCo for Imaging support shouldn't be dropped on these systems, and this
> > > > > > > > > > > isn't what your patch does either. Instead the ACPI graph port nodes (as
> > > > > > > > > > > per Linux specific definitions) are simply dropped, i.e. this isn't related
> > > > > > > > > > > to DisCo for Imaging at all.
> > > > > > > > > >
> > > > > > > > > > So it looks like the changelog of that patch could be improved, right?
> > > > > > > > >
> > > > > > > > > Well, yes. The reason the function is in the file is that nearly all camera
> > > > > > > > > related parsing is located there, not that it would be related to DisCo for
> > > > > > > > > Imaging as such.
> > > > > > > >
> > > > > > > > So IIUC the camera graph port nodes are created by default with the
> > > > > > > > help of the firmware-supplied information, but if that is defective a
> > > > > > > > quirk can be added to skip the creation of those ports in which case
> > > > > > > > they will be created elsewhere.
> > > > > > > >
> > > > > > > > Is this correct?
> > > > > > >
> > > > > > > Yes.
> > > > > >
> > > > > > So it would be good to add a comment to this effect to
> > > > > > acpi_nondev_subnode_extract() where acpi_graph_ignore_port() is
> > > > > > called.
> > > > > >
> > > > > > And there is a somewhat tangential question that occurred to me: If
> > > > > > the nodes are created elsewhere when acpi_graph_ignore_port() is true,
> > > > > > why is it necessary to consult the platform firmware for the
> > > > > > information on them at all?  Wouldn't it be better to simply always
> > > > > > create them elsewhere?
> > > > >
> > > > > Simple answer: for the same reason why in general system specific
> > > > > information comes from ACPI and not from platform data compiled into the
> > > > > kernel.
> > > > >
> > > > > Of course this is technically possible but it does not scale.
> > > >
> > > > While I agree in general, in this particular case the platform data
> > > > compiled into the kernel needs to be present anyway, at least
> > > > apparently, in case the data coming from the platform firmware is
> > > > invalid.
> > > >
> > > > So we need to do 3 things: compile in the platform data into the
> > > > kernel and expect the platform firmware to provide the necessary
> > > > information, and add quirks for the systems where it is known invalid.
> > > >
> > > > Isn't this a bit too much?
> > >
> > > Isn't this pretty much how ACPI works currently?
> >
> > No, we don't need to put platform data into the kernel for every bit
> > of information that can be retrieved from the platform firmware via
> > ACPI.
> >
> > The vast majority of information in the ACPI tables is actually
> > correct and if quirks are needed, they usually are limited in scope.
> >
> > Where it breaks is when the ACPI tables are not sufficiently validated
> > by OEMs which mostly happens when the data in question are not needed
> > to pass some sort of certification or admission tests.
> >
> > Which unfortunately is related to whether or not Windows uses those data.
> >
> > > We can support systems that contain correct DSDT description of cameras
> > > without platform data. I was, until recently, only aware of Dell XPS 9315
> > > that has incorrect camera description and that based on recent findings
> > > seems to extend to other Dell systems with IPU6 (Hans's patches have the
> > > details).
> > >
> > > Still this is not a reason to break systems that have correct camera
> > > description and expect the users to report them so they can be listed as
> > > such.
> >
> > Well, what do you mean by "break".  I thought that platform data
> > needed to support them were built into the kernel, weren't they?
>
> If you add a whitelist for systems where the port aren't just dropped, that
> is bound to break camera on a lot of current systems that depend on the
> said port nodes.

I'm not sure I understand the above.

If a blacklist entry is added to acpi_graph_ignore_port(),
acpi_nondev_subnode_extract() will bail out for all of the ports on
the given system regardless of their role etc.  Still, it is expected
that requisite nodes will be added somewhere else and the camera will
be handled because some platform data has been compiled into the
kernel for this purpose.  Or is my understanding incorrect?

If it is correct, the blacklisting mechanism might be replaced with a
whitelisting one where acpi_nondev_subnode_extract() will not bail out
on systems that are known to be good.

> >
> > > >
> > > > > On laptops shipped with Windows some additional information is also available
> > > > > from ACPI via custom objects but a lot of information is just hard coded into
> > > > > the IPU bridge as well as the INT3472 driver.
> > > >
> > > > Well, that's how it goes.
> > >
> > > Yes, but is it desirable?
> >
> > No, it is not desirable, but the way to address it is to convince the
> > Windows people to stop doing this and use standard-defined data from
> > the ACPI tables instead.  It cannot be addressed by Linux unilaterally
> > trying to do the right thing, because there are OEMs who don't care
> > about Linux.
>
> I don't disagree with that as such but it's not really the matter here, is
> it?
>
> Historically, some systems were amended with special "Linux support" which
> I believe is what these Dell systems have. That was done because the IPU
> bridge driver did not exist yet. I frankly don't think it was ever tested
> on Linux either.

OK, so this is a result of the vendor trying to support Linux in a
home-grown way which now is blowing up IIUC.

Or do you mean something else?

Powered by blists - more mailing lists