From: "Rafael J. Wysocki" <rafael@kernel.org>
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>
Subject: Re: [PATCH 1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes
Date: Thu, 13 Jun 2024 14:37:09 +0200 [thread overview]
Message-ID: <CAJZ5v0geGXHM-yHR-CWN8JremnbnSNFkWJEB+8ZZ=jPbUNy6kA@mail.gmail.com> (raw)
In-Reply-To: <20240612204114.GV28989@pendragon.ideasonboard.com>
On Wed, Jun 12, 2024 at 10:41 PM 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 PM 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 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.
next prev parent reply other threads:[~2024-06-13 12:37 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-27 16:34 6.10-rc1 : crash in mei_csi_probe Genes Lists
2024-05-27 19:57 ` Sakari Ailus
2024-05-27 20:58 ` Genes Lists
2024-05-28 8:44 ` [PATCH 1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes Sakari Ailus
2024-05-28 17:09 ` Hans de Goede
2024-06-06 18:12 ` Hans de Goede
2024-06-07 7:55 ` Rafael J. Wysocki
2024-06-07 9:24 ` Sakari Ailus
2024-06-12 10:08 ` Hans de Goede
2024-06-12 10:25 ` Rafael J. Wysocki
2024-06-12 12:10 ` Sakari Ailus
2024-06-12 12:21 ` Rafael J. Wysocki
2024-06-12 12:26 ` Sakari Ailus
2024-06-12 12:32 ` Rafael J. Wysocki
2024-06-12 12:47 ` Sakari Ailus
2024-06-12 13:06 ` Rafael J. Wysocki
2024-06-12 14:30 ` Hans de Goede
2024-06-12 14:39 ` Laurent Pinchart
2024-06-12 18:24 ` Sakari Ailus
2024-06-12 19:07 ` Rafael J. Wysocki
2024-06-12 20:08 ` Laurent Pinchart
2024-06-12 15:26 ` Rafael J. Wysocki
2024-06-12 18:33 ` Sakari Ailus
2024-06-12 18:41 ` Rafael J. Wysocki
2024-06-12 19:06 ` Sakari Ailus
2024-06-12 19:12 ` Rafael J. Wysocki
2024-06-12 19:17 ` Sakari Ailus
2024-06-12 19:32 ` Rafael J. Wysocki
2024-06-12 20:00 ` Sakari Ailus
2024-06-12 20:17 ` Sakari Ailus
2024-06-13 9:50 ` Rafael J. Wysocki
2024-06-12 18:21 ` Sakari Ailus
2024-06-12 18:29 ` Rafael J. Wysocki
2024-06-12 18:40 ` Sakari Ailus
2024-06-12 18:50 ` Rafael J. Wysocki
2024-06-12 19:11 ` Sakari Ailus
2024-06-12 19:26 ` Rafael J. Wysocki
2024-06-12 20:00 ` Laurent Pinchart
2024-06-12 20:31 ` Rafael J. Wysocki
2024-06-12 20:41 ` Laurent Pinchart
2024-06-13 12:37 ` Rafael J. Wysocki [this message]
2024-06-12 20:01 ` Laurent Pinchart
2024-05-30 0:35 ` Wu, Wentong
2024-06-06 15:39 ` 6.10-rc1 : crash in mei_csi_probe Genes Lists
2024-06-06 15:50 ` Sakari Ailus
2024-06-06 16:17 ` Genes Lists
2024-06-06 17:07 ` Linux regression tracking (Thorsten Leemhuis)
2024-06-07 7:57 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJZ5v0geGXHM-yHR-CWN8JremnbnSNFkWJEB+8ZZ=jPbUNy6kA@mail.gmail.com' \
--to=rafael@kernel.org \
--cc=hdegoede@redhat.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=lists@sapience.com \
--cc=mchehab@kernel.org \
--cc=regressions@lists.linux.dev \
--cc=sakari.ailus@linux.intel.com \
--cc=wentong.wu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).