On Wed, Jun 12, 2024 at 10:41 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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.
