Message-ID: <CAJZ5v0geGXHM-yHR-CWN8JremnbnSNFkWJEB+8ZZ=jPbUNy6kA@mail.gmail.com>
Date: Thu, 13 Jun 2024 14:37:09 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Hans de Goede <hdegoede@...hat.com>, Genes Lists <lists@...ience.com>, linux-kernel@...r.kernel.org, 
	mchehab@...nel.org, hverkuil-cisco@...all.nl, 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

On Wed, Jun 12, 2024 at 10:41 PM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Wed, Jun 12, 2024 at 10:31:06PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 12, 2024 at 10:00 PM Laurent Pinchart
> > <laurent.pinchart@...asonboard.com> wrote:
> > >
> > > On Wed, Jun 12, 2024 at 08:50:57PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Jun 12, 2024 at 8:41 PM 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?
> > > >
> > > > 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.
> > >
> > > We have to be careful here. Part of the job of the ACPI methods for
> > > camera objects is to control the camera sensor PMIC and set up the right
> > > voltages (many PMICs have programmable output levels). In many cases
> > > we've seen with the IPU3, broken ACPI support means the methods will try
> > > to do something completely bogus, like accessing a PMIC at an incorrect
> > > I2C address. That's mostly fine, it will result in the camera not being
> > > detected. We could however have broken ACPI implementation that would
> > > program the PMIC to output voltages that would damage the sensor. Users
> > > won't be happy.
> >
> > My point is basically that if that data were also used by Windows,
> > then chances are that breakage of this sort would be caught during
> > Windows validation before shipping the machines and so it wouldn't
> > affect Linux as well.
> >
> > However, if OEMs have no vehicle to validate their systems against,
> > bad things can happen indeed.
> >
> > Also, if an OEM has no incentive to carry out the requisite checks,
> > the result is likely to be invalid data in the platform firmware.
>
> We're exactly on the same page. The only solution [*] I can see for this
> problem is to get the Windows drivers to use the same ACPI data as the
> Linux drivers.

That is long-term, however, and in the meantime something needs to be
done about it too.

Sakari is telling me that the warning on boot triggered by firmware
issues was in a new driver and it has been addressed in 6.10-rc3
already.

This is good, as we don't need to worry about people reporting a
regression because of it any more.

Still, IIUC, the driver simply fails to probe if it doesn't get
correct information from the platform firmware and a quirk needs to be
added to the ACPI enumeration code for the driver to use a different
source of information.

I'm wondering if the driver could be modified to switch over to the
different source of information automatically if the firmware-provided
data don't make any sense to it, after logging an FW_BUG message.  It
could even use the other source of information to sanity-check the
firmware-provided data in principle.  It's all software, so it should
be doable.

> * Another solution would be for OEMs to stop caring about Windows and
> testing their machines with Linux only, essentially reversing the
> current situation. Chances of this happening however seem even tinier
> :-)

Seriously though, we could create a Linux-based utility that would
retrieve all of the relevant information from the firmware using the
existing kernel code and they say "this is what I would do to the
hardware based on this information".  That could help people to do
basic checks if they cared.

Powered by blists - more mailing lists