Received-SPF: pass (google.com: domain of linux-kernel+bounces-213215-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <ZmmT56Cyvb2FCyav@kekkonen.localdomain> <CAJZ5v0hOBggQR_=uA3VuhruQnZihVxHHovpTz4=qfcbiSunsYw@mail.gmail.com>
 <ZmmY3he9vfWVWU3I@kekkonen.localdomain> <CAJZ5v0j7HTfg1wY+B+7vhE6tBKPVHMuu_MsFHjaLK70VS_cNEw@mail.gmail.com>
 <ZmnnFueL-Cgw5Eqp@kekkonen.localdomain> <CAJZ5v0gtK9yusimCOVV2dGkQWDwQ6=r=vfbgC-eE60Cg-5wk_Q@mail.gmail.com>
 <ZmnrtIEla9R24egi@kekkonen.localdomain> <CAJZ5v0hXU62QiXxWfkbiovciNNEk0h49kRdScmz5qONTMDA+4A@mail.gmail.com>
 <20240612200012.GP28989@pendragon.ideasonboard.com> <CAJZ5v0hF+6_RCyP-Rr+ajNNEKe0YenFR8x6wX3dG1Pq+vguTwg@mail.gmail.com>
 <20240612204114.GV28989@pendragon.ideasonboard.com>
In-Reply-To: <20240612204114.GV28989@pendragon.ideasonboard.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Thu, 13 Jun 2024 14:37:09 +0200
Message-ID: <CAJZ5v0geGXHM-yHR-CWN8JremnbnSNFkWJEB+8ZZ=jPbUNy6kA@mail.gmail.com>
Subject: Re: [PATCH 1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, 
	Hans de Goede <hdegoede@redhat.com>, Genes Lists <lists@sapience.com>, linux-kernel@vger.kernel.org, 
	mchehab@kernel.org, hverkuil-cisco@xs4all.nl, wentong.wu@intel.com, 
	linux-media@vger.kernel.org, linux-acpi@vger.kernel.org, 
	"regressions@lists.linux.dev" <regressions@lists.linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 12, 2024 at 10:41=E2=80=AFPM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Jun 12, 2024 at 10:31:06PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 12, 2024 at 10:00=E2=80=AFPM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > On Wed, Jun 12, 2024 at 08:50:57PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Jun 12, 2024 at 8:41=E2=80=AFPM 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=E2=80=AFPM Sakari Ailus wrote:
> > > > > > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki w=
rote:
> > > > > > > > On Wed, Jun 12, 2024 at 2:47=E2=80=AFPM Sakari Ailus wrote:
> > > > > > > > > On Wed, Jun 12, 2024 at 02:32:26PM +0200, Rafael J. Wysoc=
ki wrote:
> > > > > > > > > > > > > > I just hit the same problem on another Dell lap=
top. It seems that
> > > > > > > > > > > > > > all Dell laptops with IPU6 camera from the Tige=
r Lake, Alder Lake
> > > > > > > > > > > > > > and Raptor Lake generations suffer from this pr=
oblem.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So instead of playing whack a mole with DMI mat=
ches we should
> > > > > > > > > > > > > > simply disable ACPI MIPI DISCO support on all D=
ell laptops
> > > > > > > > > > > > > > with those CPUs. I'm preparing a fix for this t=
o 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 AC=
PI graph port nodes (as
> > > > > > > > > > > > > per Linux specific definitions) are simply droppe=
d, 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 b=
e related to DisCo for
> > > > > > > > > > > Imaging as such.
> > > > > > > > > >
> > > > > > > > > > So IIUC the camera graph port nodes are created by defa=
ult 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 t=
o me: If
> > > > > > > > the nodes are created elsewhere when acpi_graph_ignore_port=
() is true,
> > > > > > > > why is it necessary to consult the platform firmware for th=
e
> > > > > > > > information on them at all?  Wouldn't it be better to simpl=
y always
> > > > > > > > create them elsewhere?
> > > > > > >
> > > > > > > Simple answer: for the same reason why in general system spec=
ific
> > > > > > > information comes from ACPI and not from platform data compil=
ed 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 th=
e
> > > > > > kernel and expect the platform firmware to provide the necessar=
y
> > > > > > information, and add quirks for the systems where it is known i=
nvalid.
> > > > > >
> > > > > > 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 bi=
t
> > > > 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 valida=
ted
> > > > by OEMs which mostly happens when the data in question are not need=
ed
> > > > 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 ri=
ght
> > > 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 incorre=
ct
> > > I2C address. That's mostly fine, it will result in the camera not bei=
ng
> > > detected. We could however have broken ACPI implementation that would
> > > program the PMIC to output voltages that would damage the sensor. Use=
rs
> > > 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.