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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Jun 12, 2024 at 06:40:52PM +0000, Sakari Ailus wrote:
> On Wed, Jun 12, 2024 at 08:29:21PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 12, 2024 at 8:21 PM Sakari Ailus wrote:
> > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus wrote:
> > > > > 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?
> 
> 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).

Are you aware of any IPU6-based devices, apart from chromebooks, that
have correct ACPI tables for the camera ?

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

-- 
Regards,

Laurent Pinchart