Hi Rafael, On Wed, Jun 12, 2024 at 09:07:09PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 12, 2024 at 4:40 PM Laurent Pinchart wrote: > > On Wed, Jun 12, 2024 at 04:30:30PM +0200, Hans de Goede wrote: > > > On 6/12/24 3:06 PM, 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? > > > > > > That is a good question. The ACPI MIPI DISCO specification is an > > > attempt standardize how MIPI cameras and their sensors are described > > > in ACPI. > > > > > > But this is not actually being used by any Windows drivers atm. The windows > > > drivers rely on their own custom ACPI data which gets translated into > > > standard Linux device-properties by: drivers/media/pci/intel/ipu-bridge.c > > > > > > and so far AFAIK there are 0 laptops where there actually is 100% functional > > > ACPI MIPI information. I believe that some work is in place to get correct > > > usable ACPI MIPI information in place in the ACPI tables of some Meteor Lake > > > laptops. But I believe that there too it does not work yet with the BIOS > > > version with which current Windows models are shipping. It is being fixed > > > for systems which have Linux support from the vendor but I suspect that > > > > I think it's shipped in Chrome Books. Sakari can confirm. > > > > > on other models if ACPI MIPI DISCO information is there it will not > > > necessarily be reliable because AFAICT Windows does not actually use it. > > > > > > And TBH this has me worried about camera support for Meteor Lake devices > > > going forward. We really need to have 1 reliable source of truth here and > > > using information which is ignored by Windows does not seem like the best > > > source to use. > > > > As long as the Windows drivers don't use the ACPI data that Linux uses, > > you can be 100% sure it will be wrong. That will never be fixed if Intel > > doesn't address the issue on their side, and effort we would put in > > standardizing that data for machines shipped by Windows OEMs is a waste > > of time in my opinion. Our only option, given Intel's failure, is to > > keep reverse-engineering camera support per machine for the time being > > (sometimes with the help of OEMs). > > So while I kind of agree with you on the technical side (as you can > see from my messages in this thread), there is one thing in the above > paragraph that I need to react to. > > Namely, both I and Sakari are Intel employees. Do you think or are > you suggesting that we are somehow responsible for this failure? > Because I personally don't think like I have anything to do with it. That's a worthy clarification: I blame neither you nor Sakari, quite the contrary. As far as I can tell, both of you have done and are doing your best to fix this kind of issues. Sakari has helped standardizing DisCo for Imaging for instance, which was the best outcome we could realistically hope for in this context. Intel is a large company, and as with any large company, some people end up having to try and fix the mess created by others :-S I'm sorry it's falling on you and Sakari. > What do you mean, specifically, by saying "if Intel doesn't address > the issue on their side"? And who at Intel do I need to talk to about > this? I think you would need to get the Windows and Linux side of the Intel team(s) responsible for camera drivers and ACPI integration, along with a manager who can understand the problem, the bad PR, and get the stakeholders to cooperate and do better in the future. I don't know who that would be though, what I know is that it doesn't seem the issue will solve itself without someone with decision power decides to fix it. -- Regards, Laurent Pinchart
