Re: [PATCH 1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes

[Sakari Ailus]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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.

> 
> > >
> > > > 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.

-- 
Kind regards,

Sakari Ailus