| Message ID | 20240528084413.2624435-1-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes | expand |
> From: Sakari Ailus <sakari.ailus@linux.intel.com> > > Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS They -> The > is buggy, just like it is for Dell XPS 9315. The corresponding software nodes > are created by the ipu-bridge. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Wentong Wu <wentong.wu@intel.com> > --- > Hi, > > Could you test this and see whether it fixes the warning? > > The camera might work with this change, too. > > - Sakari > > drivers/acpi/mipi-disco-img.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/acpi/mipi-disco-img.c b/drivers/acpi/mipi-disco-img.c > index d05413a0672a..bf9a5cee32ac 100644 > --- a/drivers/acpi/mipi-disco-img.c > +++ b/drivers/acpi/mipi-disco-img.c > @@ -732,6 +732,12 @@ static const struct dmi_system_id > dmi_ignore_port_nodes[] = { > DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS > 9315"), > }, > }, > + { > + .matches = { > + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS > 9320"), > + }, > + }, > { } > }; > > -- > 2.39.2
Hi, +To: Rafael since this was Cc-ed to linux-acpi but never send to Rafael directly. Rafael this fixes a crash in 6.10-rc1 for some users and is necessary to make the cameras work on the Dell XPS 13 plus 9320 . On 5/28/24 7:09 PM, Hans de Goede wrote: > Hi Sakari, > > On 5/28/24 10:44 AM, Sakari Ailus wrote: >> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS >> is buggy, just like it is for Dell XPS 9315. The corresponding software >> nodes are created by the ipu-bridge. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> Hi, >> >> Could you test this and see whether it fixes the warning? >> >> The camera might work with this change, too. > > Thank you I just received a Dell XPS 13 plus 9320 myself to use > for VSC testing and I can confirm that with this patch 6.10.0-rc1 > works, including giving a picture with the libcamera software ISP + > 3 small libcamera patches. I forgot to add: Tested-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans >> drivers/acpi/mipi-disco-img.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/acpi/mipi-disco-img.c b/drivers/acpi/mipi-disco-img.c >> index d05413a0672a..bf9a5cee32ac 100644 >> --- a/drivers/acpi/mipi-disco-img.c >> +++ b/drivers/acpi/mipi-disco-img.c >> @@ -732,6 +732,12 @@ static const struct dmi_system_id dmi_ignore_port_nodes[] = { >> DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"), >> }, >> }, >> + { >> + .matches = { >> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9320"), >> + }, >> + }, >> { } >> }; >>
On Thu, Jun 6, 2024 at 8:12 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > +To: Rafael since this was Cc-ed to linux-acpi but never send > to Rafael directly. > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > to make the cameras work on the Dell XPS 13 plus 9320 . > > On 5/28/24 7:09 PM, Hans de Goede wrote: > > Hi Sakari, > > > > On 5/28/24 10:44 AM, Sakari Ailus wrote: > >> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > >> is buggy, just like it is for Dell XPS 9315. The corresponding software > >> nodes are created by the ipu-bridge. > >> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> --- > >> Hi, > >> > >> Could you test this and see whether it fixes the warning? > >> > >> The camera might work with this change, too. > > > > Thank you I just received a Dell XPS 13 plus 9320 myself to use > > for VSC testing and I can confirm that with this patch 6.10.0-rc1 > > works, including giving a picture with the libcamera software ISP + > > 3 small libcamera patches. > > I forgot to add: > > Tested-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Applied as 6.10-rc material. I've also added Reported-by and Closes tags to this, but I'm not sure which commit exactly is fixed by it, so the Fixes tag is missing.
Hi Rafael, On Fri, Jun 07, 2024 at 09:55:44AM +0200, Rafael J. Wysocki wrote: > On Thu, Jun 6, 2024 at 8:12 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi, > > > > +To: Rafael since this was Cc-ed to linux-acpi but never send > > to Rafael directly. > > > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > > to make the cameras work on the Dell XPS 13 plus 9320 . > > > > On 5/28/24 7:09 PM, Hans de Goede wrote: > > > Hi Sakari, > > > > > > On 5/28/24 10:44 AM, Sakari Ailus wrote: > > >> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > > >> is buggy, just like it is for Dell XPS 9315. The corresponding software > > >> nodes are created by the ipu-bridge. > > >> > > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > >> --- > > >> Hi, > > >> > > >> Could you test this and see whether it fixes the warning? > > >> > > >> The camera might work with this change, too. > > > > > > Thank you I just received a Dell XPS 13 plus 9320 myself to use > > > for VSC testing and I can confirm that with this patch 6.10.0-rc1 > > > works, including giving a picture with the libcamera software ISP + > > > 3 small libcamera patches. > > > > I forgot to add: > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Applied as 6.10-rc material. Thanks! > > I've also added Reported-by and Closes tags to this, but I'm not sure > which commit exactly is fixed by it, so the Fixes tag is missing. That's fine. We don't know which systems have faulty camera graph in DSDT so these are added as they're found.
Hi, On 6/6/24 8:12 PM, Hans de Goede wrote: > Hi, > > +To: Rafael since this was Cc-ed to linux-acpi but never send > to Rafael directly. > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > to make the cameras work on the Dell XPS 13 plus 9320 . > > On 5/28/24 7:09 PM, Hans de Goede wrote: >> Hi Sakari, >> >> On 5/28/24 10:44 AM, Sakari Ailus wrote: >>> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS >>> is buggy, just like it is for Dell XPS 9315. The corresponding software >>> nodes are created by the ipu-bridge. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> Hi, >>> >>> Could you test this and see whether it fixes the warning? >>> >>> The camera might work with this change, too. >> >> Thank you I just received a Dell XPS 13 plus 9320 myself to use >> for VSC testing and I can confirm that with this patch 6.10.0-rc1 >> works, including giving a picture with the libcamera software ISP + >> 3 small libcamera patches. > > I forgot to add: > > Tested-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> 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. Rafael, please drop this patch, my more generic fix will replace it and backporting will be easier without having the intermediate fix in the middle. Regards, Hans >>> drivers/acpi/mipi-disco-img.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/acpi/mipi-disco-img.c b/drivers/acpi/mipi-disco-img.c >>> index d05413a0672a..bf9a5cee32ac 100644 >>> --- a/drivers/acpi/mipi-disco-img.c >>> +++ b/drivers/acpi/mipi-disco-img.c >>> @@ -732,6 +732,12 @@ static const struct dmi_system_id dmi_ignore_port_nodes[] = { >>> DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"), >>> }, >>> }, >>> + { >>> + .matches = { >>> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >>> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9320"), >>> + }, >>> + }, >>> { } >>> }; >>> >
On Wed, Jun 12, 2024 at 12:08 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 6/6/24 8:12 PM, Hans de Goede wrote: > > Hi, > > > > +To: Rafael since this was Cc-ed to linux-acpi but never send > > to Rafael directly. > > > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > > to make the cameras work on the Dell XPS 13 plus 9320 . > > > > On 5/28/24 7:09 PM, Hans de Goede wrote: > >> Hi Sakari, > >> > >> On 5/28/24 10:44 AM, Sakari Ailus wrote: > >>> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > >>> is buggy, just like it is for Dell XPS 9315. The corresponding software > >>> nodes are created by the ipu-bridge. > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> Hi, > >>> > >>> Could you test this and see whether it fixes the warning? > >>> > >>> The camera might work with this change, too. > >> > >> Thank you I just received a Dell XPS 13 plus 9320 myself to use > >> for VSC testing and I can confirm that with this patch 6.10.0-rc1 > >> works, including giving a picture with the libcamera software ISP + > >> 3 small libcamera patches. > > > > I forgot to add: > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > 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. > > Rafael, please drop this patch, my more generic fix will replace it > and backporting will be easier without having the intermediate fix > in the middle. Dropping, thanks!
Hi Hans, Just read this discussion, too... On Wed, Jun 12, 2024 at 12:08:49PM +0200, Hans de Goede wrote: > Hi, > > On 6/6/24 8:12 PM, Hans de Goede wrote: > > Hi, > > > > +To: Rafael since this was Cc-ed to linux-acpi but never send > > to Rafael directly. > > > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > > to make the cameras work on the Dell XPS 13 plus 9320 . > > > > On 5/28/24 7:09 PM, Hans de Goede wrote: > >> Hi Sakari, > >> > >> On 5/28/24 10:44 AM, Sakari Ailus wrote: > >>> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > >>> is buggy, just like it is for Dell XPS 9315. The corresponding software > >>> nodes are created by the ipu-bridge. > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> Hi, > >>> > >>> Could you test this and see whether it fixes the warning? > >>> > >>> The camera might work with this change, too. > >> > >> Thank you I just received a Dell XPS 13 plus 9320 myself to use > >> for VSC testing and I can confirm that with this patch 6.10.0-rc1 > >> works, including giving a picture with the libcamera software ISP + > >> 3 small libcamera patches. > > > > I forgot to add: > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > 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.
On Wed, Jun 12, 2024 at 2:10 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Hans, > > Just read this discussion, too... > > On Wed, Jun 12, 2024 at 12:08:49PM +0200, Hans de Goede wrote: > > Hi, > > > > On 6/6/24 8:12 PM, Hans de Goede wrote: > > > Hi, > > > > > > +To: Rafael since this was Cc-ed to linux-acpi but never send > > > to Rafael directly. > > > > > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > > > to make the cameras work on the Dell XPS 13 plus 9320 . > > > > > > On 5/28/24 7:09 PM, Hans de Goede wrote: > > >> Hi Sakari, > > >> > > >> On 5/28/24 10:44 AM, Sakari Ailus wrote: > > >>> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > > >>> is buggy, just like it is for Dell XPS 9315. The corresponding software > > >>> nodes are created by the ipu-bridge. > > >>> > > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > >>> --- > > >>> Hi, > > >>> > > >>> Could you test this and see whether it fixes the warning? > > >>> > > >>> The camera might work with this change, too. > > >> > > >> Thank you I just received a Dell XPS 13 plus 9320 myself to use > > >> for VSC testing and I can confirm that with this patch 6.10.0-rc1 > > >> works, including giving a picture with the libcamera software ISP + > > >> 3 small libcamera patches. > > > > > > I forgot to add: > > > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > > 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?
Hi Rafael, On Wed, Jun 12, 2024 at 02:21:51PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 12, 2024 at 2:10 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Hans, > > > > Just read this discussion, too... > > > > On Wed, Jun 12, 2024 at 12:08:49PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 6/6/24 8:12 PM, Hans de Goede wrote: > > > > Hi, > > > > > > > > +To: Rafael since this was Cc-ed to linux-acpi but never send > > > > to Rafael directly. > > > > > > > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > > > > to make the cameras work on the Dell XPS 13 plus 9320 . > > > > > > > > On 5/28/24 7:09 PM, Hans de Goede wrote: > > > >> Hi Sakari, > > > >> > > > >> On 5/28/24 10:44 AM, Sakari Ailus wrote: > > > >>> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > > > >>> is buggy, just like it is for Dell XPS 9315. The corresponding software > > > >>> nodes are created by the ipu-bridge. > > > >>> > > > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > >>> --- > > > >>> Hi, > > > >>> > > > >>> Could you test this and see whether it fixes the warning? > > > >>> > > > >>> The camera might work with this change, too. > > > >> > > > >> Thank you I just received a Dell XPS 13 plus 9320 myself to use > > > >> for VSC testing and I can confirm that with this patch 6.10.0-rc1 > > > >> works, including giving a picture with the libcamera software ISP + > > > >> 3 small libcamera patches. > > > > > > > > I forgot to add: > > > > > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > > > > 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.
Hi Sakari, On Wed, Jun 12, 2024 at 2:26 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Jun 12, 2024 at 02:21:51PM +0200, Rafael J. Wysocki wrote: > > On Wed, Jun 12, 2024 at 2:10 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Hans, > > > > > > Just read this discussion, too... > > > > > > On Wed, Jun 12, 2024 at 12:08:49PM +0200, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 6/6/24 8:12 PM, Hans de Goede wrote: > > > > > Hi, > > > > > > > > > > +To: Rafael since this was Cc-ed to linux-acpi but never send > > > > > to Rafael directly. > > > > > > > > > > Rafael this fixes a crash in 6.10-rc1 for some users and is necessary > > > > > to make the cameras work on the Dell XPS 13 plus 9320 . > > > > > > > > > > On 5/28/24 7:09 PM, Hans de Goede wrote: > > > > >> Hi Sakari, > > > > >> > > > > >> On 5/28/24 10:44 AM, Sakari Ailus wrote: > > > > >>> Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS > > > > >>> is buggy, just like it is for Dell XPS 9315. The corresponding software > > > > >>> nodes are created by the ipu-bridge. > > > > >>> > > > > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > >>> --- > > > > >>> Hi, > > > > >>> > > > > >>> Could you test this and see whether it fixes the warning? > > > > >>> > > > > >>> The camera might work with this change, too. > > > > >> > > > > >> Thank you I just received a Dell XPS 13 plus 9320 myself to use > > > > >> for VSC testing and I can confirm that with this patch 6.10.0-rc1 > > > > >> works, including giving a picture with the libcamera software ISP + > > > > >> 3 small libcamera patches. > > > > > > > > > > I forgot to add: > > > > > > > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > > > > > > 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?
Hi Rafael, 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.
Hi Sakari, On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > 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?
Hi, On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > Hi Sakari, > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: >> >> Hi Rafael, >> >> 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 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. Sakari I know you have been pushing for MIPI camera descriptions under ACPI to move to a standardized format and I can see how that is a good thing, but atm it seems to mainly cause things to break and before the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, since the information used by the ipu-bridge code does seem to be correct. Regards, Hans
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). > Sakari I know you have been pushing for MIPI camera descriptions under > ACPI to move to a standardized format and I can see how that is a good > thing, but atm it seems to mainly cause things to break and before > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > since the information used by the ipu-bridge code does seem to be correct.
Hi, On Wed, Jun 12, 2024 at 4:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > > Hi Sakari, > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > >> > >> Hi Rafael, > >> > >> 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 > 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. > > Sakari I know you have been pushing for MIPI camera descriptions under > ACPI to move to a standardized format and I can see how that is a good > thing, but atm it seems to mainly cause things to break and before > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > since the information used by the ipu-bridge code does seem to be correct. Well, if Windows doesn't use this information, it is almost guaranteed to be garbage. So maybe it would be better to make acpi_graph_ignore_port() return true by default and false only when the information is known to be valid. IOW, whitelist things instead of adding blacklist entries in perpetuum. And hopefully we'll eventually get to the point at which we are able to say "whitelist everything from now on".
Hi Rafael, On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote: > Hi Sakari, > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > 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. On laptops shipped with Windows some additional information is also available from ACPI via custom objects but a lot of information is just hard coded into the IPU bridge as well as the INT3472 driver.
Hi Laurent, On Wed, Jun 12, 2024 at 05:39:56PM +0300, 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. Not yet but I'd expect that some time in the future. The Chromebooks use Linux specific definitions that pre-date DisCo for Imaging but are still relatively close to that.
Hi Sakari, On Wed, Jun 12, 2024 at 8:21 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote: > > Hi Sakari, > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > 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? > On laptops shipped with Windows some additional information is also available > from ACPI via custom objects but a lot of information is just hard coded into > the IPU bridge as well as the INT3472 driver. Well, that's how it goes.
Hi Rafael, On Wed, Jun 12, 2024 at 05:26:46PM +0200, Rafael J. Wysocki wrote: > Hi, > > On Wed, Jun 12, 2024 at 4:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi, > > > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > > > Hi Sakari, > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > >> > > >> Hi Rafael, > > >> > > >> 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 > > 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. > > > > Sakari I know you have been pushing for MIPI camera descriptions under > > ACPI to move to a standardized format and I can see how that is a good > > thing, but atm it seems to mainly cause things to break and before > > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > > since the information used by the ipu-bridge code does seem to be correct. > > Well, if Windows doesn't use this information, it is almost guaranteed > to be garbage. No ACPI DSDT in production systems uses DisCo for Imaging as of now at least to my knowledge. > > So maybe it would be better to make acpi_graph_ignore_port() return > true by default and false only when the information is known to be > valid. IOW, whitelist things instead of adding blacklist entries in > perpetuum. What could be gained from this? > > And hopefully we'll eventually get to the point at which we are able > to say "whitelist everything from now on".
Hi Rafael, On Wed, Jun 12, 2024 at 08:29:21PM +0200, Rafael J. Wysocki wrote: > Hi Sakari, > > On Wed, Jun 12, 2024 at 8:21 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote: > > > Hi Sakari, > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Hi Rafael, > > > > > > > > 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? We can support systems that contain correct DSDT description of cameras without platform data. I was, until recently, only aware of Dell XPS 9315 that has incorrect camera description and that based on recent findings seems to extend to other Dell systems with IPU6 (Hans's patches have the details). Still this is not a reason to break systems that have correct camera description and expect the users to report them so they can be listed as such. > > > On laptops shipped with Windows some additional information is also available > > from ACPI via custom objects but a lot of information is just hard coded into > > the IPU bridge as well as the INT3472 driver. > > Well, that's how it goes. Yes, but is it desirable?
Hi Sakari, On Wed, Jun 12, 2024 at 8:33 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Jun 12, 2024 at 05:26:46PM +0200, Rafael J. Wysocki wrote: > > Hi, > > > > On Wed, Jun 12, 2024 at 4:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > Hi, > > > > > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > > > > Hi Sakari, > > > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > >> > > > >> Hi Rafael, > > > >> > > > >> 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 > > > 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. > > > > > > Sakari I know you have been pushing for MIPI camera descriptions under > > > ACPI to move to a standardized format and I can see how that is a good > > > thing, but atm it seems to mainly cause things to break and before > > > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > > > since the information used by the ipu-bridge code does seem to be correct. > > > > Well, if Windows doesn't use this information, it is almost guaranteed > > to be garbage. > > No ACPI DSDT in production systems uses DisCo for Imaging as of now at > least to my knowledge. > > > > > So maybe it would be better to make acpi_graph_ignore_port() return > > true by default and false only when the information is known to be > > valid. IOW, whitelist things instead of adding blacklist entries in > > perpetuum. > > What could be gained from this? Generally speaking, fewer headaches for people trying to support Linux on Intel client platforms. Every system that needs to be put into dmi_ignore_port_nodes[] means a boot problem for someone that needs to be addressed. And after the Hans' patch at https://patchwork.kernel.org/project/linux-acpi/patch/20240612104220.22219-1-hdegoede@redhat.com/ we would effectively get very close to that point anyway. Are the ACPI tables on MTL and beyond going to be fixed? If not, we'll probably need to add MTL to the list of platform IDs and so on. In what way is this better?
Hi Sakari, On Wed, Jun 12, 2024 at 8:41 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Jun 12, 2024 at 08:29:21PM +0200, Rafael J. Wysocki wrote: > > Hi Sakari, > > > > On Wed, Jun 12, 2024 at 8:21 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote: > > > > Hi Sakari, > > > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > Hi Rafael, > > > > > > > > > > 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. Which unfortunately is related to whether or not Windows uses those data. > We can support systems that contain correct DSDT description of cameras > without platform data. I was, until recently, only aware of Dell XPS 9315 > that has incorrect camera description and that based on recent findings > seems to extend to other Dell systems with IPU6 (Hans's patches have the > details). > > Still this is not a reason to break systems that have correct camera > description and expect the users to report them so they can be listed as > such. Well, what do you mean by "break". I thought that platform data needed to support them were built into the kernel, weren't they? > > > > > On laptops shipped with Windows some additional information is also available > > > from ACPI via custom objects but a lot of information is just hard coded into > > > the IPU bridge as well as the INT3472 driver. > > > > Well, that's how it goes. > > Yes, but is it desirable? No, it is not desirable, but the way to address it is to convince the Windows people to stop doing this and use standard-defined data from the ACPI tables instead. It cannot be addressed by Linux unilaterally trying to do the right thing, because there are OEMs who don't care about Linux.
Hi Rafael, On Wed, Jun 12, 2024 at 08:41:43PM +0200, Rafael J. Wysocki wrote: > Hi Sakari, > > On Wed, Jun 12, 2024 at 8:33 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > On Wed, Jun 12, 2024 at 05:26:46PM +0200, Rafael J. Wysocki wrote: > > > Hi, > > > > > > On Wed, Jun 12, 2024 at 4:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > > > Hi, > > > > > > > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > > > > > Hi Sakari, > > > > > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > > > <sakari.ailus@linux.intel.com> wrote: > > > > >> > > > > >> Hi Rafael, > > > > >> > > > > >> 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 > > > > 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. > > > > > > > > Sakari I know you have been pushing for MIPI camera descriptions under > > > > ACPI to move to a standardized format and I can see how that is a good > > > > thing, but atm it seems to mainly cause things to break and before > > > > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > > > > since the information used by the ipu-bridge code does seem to be correct. > > > > > > Well, if Windows doesn't use this information, it is almost guaranteed > > > to be garbage. > > > > No ACPI DSDT in production systems uses DisCo for Imaging as of now at > > least to my knowledge. > > > > > > > > So maybe it would be better to make acpi_graph_ignore_port() return > > > true by default and false only when the information is known to be > > > valid. IOW, whitelist things instead of adding blacklist entries in > > > perpetuum. > > > > What could be gained from this? > > Generally speaking, fewer headaches for people trying to support Linux > on Intel client platforms. I don't think that is the case here. I'd like to reiterate that none of the issues there have been so far (including with Dell laptops) have been related to DisCo for Imaging. > > Every system that needs to be put into dmi_ignore_port_nodes[] means a > boot problem for someone that needs to be addressed. > > And after the Hans' patch at > > https://patchwork.kernel.org/project/linux-acpi/patch/20240612104220.22219-1-hdegoede@redhat.com/ > > we would effectively get very close to that point anyway. Dell systems only, and of a limited range. > > Are the ACPI tables on MTL and beyond going to be fixed? If not, > we'll probably need to add MTL to the list of platform IDs and so on. > In what way is this better? This will very probably take place post-MTL.
Hi Laurent, On Wed, Jun 12, 2024 at 4:40 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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. 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?
Hi Rafael, On Wed, Jun 12, 2024 at 08:50:57PM +0200, Rafael J. Wysocki wrote: > Hi Sakari, > > On Wed, Jun 12, 2024 at 8:41 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > On Wed, Jun 12, 2024 at 08:29:21PM +0200, Rafael J. Wysocki wrote: > > > Hi Sakari, > > > > > > On Wed, Jun 12, 2024 at 8:21 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Hi Rafael, > > > > > > > > On Wed, Jun 12, 2024 at 03:06:53PM +0200, Rafael J. Wysocki wrote: > > > > > Hi Sakari, > > > > > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > > > Hi Rafael, > > > > > > > > > > > > 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. > > Which unfortunately is related to whether or not Windows uses those data. > > > We can support systems that contain correct DSDT description of cameras > > without platform data. I was, until recently, only aware of Dell XPS 9315 > > that has incorrect camera description and that based on recent findings > > seems to extend to other Dell systems with IPU6 (Hans's patches have the > > details). > > > > Still this is not a reason to break systems that have correct camera > > description and expect the users to report them so they can be listed as > > such. > > Well, what do you mean by "break". I thought that platform data > needed to support them were built into the kernel, weren't they? If you add a whitelist for systems where the port aren't just dropped, that is bound to break camera on a lot of current systems that depend on the said port nodes. > > > > > > > > On laptops shipped with Windows some additional information is also available > > > > from ACPI via custom objects but a lot of information is just hard coded into > > > > the IPU bridge as well as the INT3472 driver. > > > > > > Well, that's how it goes. > > > > Yes, but is it desirable? > > No, it is not desirable, but the way to address it is to convince the > Windows people to stop doing this and use standard-defined data from > the ACPI tables instead. It cannot be addressed by Linux unilaterally > trying to do the right thing, because there are OEMs who don't care > about Linux. I don't disagree with that as such but it's not really the matter here, is it? Historically, some systems were amended with special "Linux support" which I believe is what these Dell systems have. That was done because the IPU bridge driver did not exist yet. I frankly don't think it was ever tested on Linux either.
Hi Rafael, On Wed, Jun 12, 2024 at 09:12:59PM +0200, Rafael J. Wysocki wrote: > Hi Sakari, > > On Wed, Jun 12, 2024 at 9:07 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > On Wed, Jun 12, 2024 at 08:41:43PM +0200, Rafael J. Wysocki wrote: > > > Hi Sakari, > > > > > > On Wed, Jun 12, 2024 at 8:33 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Hi Rafael, > > > > > > > > On Wed, Jun 12, 2024 at 05:26:46PM +0200, Rafael J. Wysocki wrote: > > > > > Hi, > > > > > > > > > > On Wed, Jun 12, 2024 at 4:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > > > > > > > Hi Sakari, > > > > > > > > > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > >> > > > > > > >> Hi Rafael, > > > > > > >> > > > > > > >> 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 > > > > > > 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. > > > > > > > > > > > > Sakari I know you have been pushing for MIPI camera descriptions under > > > > > > ACPI to move to a standardized format and I can see how that is a good > > > > > > thing, but atm it seems to mainly cause things to break and before > > > > > > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > > > > > > since the information used by the ipu-bridge code does seem to be correct. > > > > > > > > > > Well, if Windows doesn't use this information, it is almost guaranteed > > > > > to be garbage. > > > > > > > > No ACPI DSDT in production systems uses DisCo for Imaging as of now at > > > > least to my knowledge. > > > > > > > > > > > > > > So maybe it would be better to make acpi_graph_ignore_port() return > > > > > true by default and false only when the information is known to be > > > > > valid. IOW, whitelist things instead of adding blacklist entries in > > > > > perpetuum. > > > > > > > > What could be gained from this? > > > > > > Generally speaking, fewer headaches for people trying to support Linux > > > on Intel client platforms. > > > > I don't think that is the case here. > > > > I'd like to reiterate that none of the issues there have been so far > > (including with Dell laptops) have been related to DisCo for Imaging. > > Well, they were (or are) related to firmware issues that cause systems > to fail to boot if triggered until they get blacklisted in > acpi_graph_ignore_port(). This is the first time I hear about a boot failure due to incorrect camera description (on production systems). Could you point me to where this has happened?
Hi Sakari, On Wed, Jun 12, 2024 at 9:17 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Jun 12, 2024 at 09:12:59PM +0200, Rafael J. Wysocki wrote: > > Hi Sakari, > > > > On Wed, Jun 12, 2024 at 9:07 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > On Wed, Jun 12, 2024 at 08:41:43PM +0200, Rafael J. Wysocki wrote: > > > > Hi Sakari, > > > > > > > > On Wed, Jun 12, 2024 at 8:33 PM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > Hi Rafael, > > > > > > > > > > On Wed, Jun 12, 2024 at 05:26:46PM +0200, Rafael J. Wysocki wrote: > > > > > > Hi, > > > > > > > > > > > > On Wed, Jun 12, 2024 at 4:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote: > > > > > > > > Hi Sakari, > > > > > > > > > > > > > > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus > > > > > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > >> > > > > > > > >> Hi Rafael, > > > > > > > >> > > > > > > > >> 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 > > > > > > > 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. > > > > > > > > > > > > > > Sakari I know you have been pushing for MIPI camera descriptions under > > > > > > > ACPI to move to a standardized format and I can see how that is a good > > > > > > > thing, but atm it seems to mainly cause things to break and before > > > > > > > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > > > > > > > since the information used by the ipu-bridge code does seem to be correct. > > > > > > > > > > > > Well, if Windows doesn't use this information, it is almost guaranteed > > > > > > to be garbage. > > > > > > > > > > No ACPI DSDT in production systems uses DisCo for Imaging as of now at > > > > > least to my knowledge. > > > > > > > > > > > > > > > > > So maybe it would be better to make acpi_graph_ignore_port() return > > > > > > true by default and false only when the information is known to be > > > > > > valid. IOW, whitelist things instead of adding blacklist entries in > > > > > > perpetuum. > > > > > > > > > > What could be gained from this? > > > > > > > > Generally speaking, fewer headaches for people trying to support Linux > > > > on Intel client platforms. > > > > > > I don't think that is the case here. > > > > > > I'd like to reiterate that none of the issues there have been so far > > > (including with Dell laptops) have been related to DisCo for Imaging. > > > > Well, they were (or are) related to firmware issues that cause systems > > to fail to boot if triggered until they get blacklisted in > > acpi_graph_ignore_port(). > > This is the first time I hear about a boot failure due to incorrect camera > description (on production systems). Could you point me to where this has > happened? https://lore.kernel.org/lkml/8afe9391b96ff3e1c60e624c1b8a3b2bd5039560.camel@sapience.com/ or is it not a boot failure? If so, apologies for misunderstanding. Looks serious enough to me though.
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. And now that I wrote that, maybe that's what we should hope for, a major recall of machines from Dell or Lenovo, whose financial cost would give an incentive to fixing this mess in the future... *sigh* > Which unfortunately is related to whether or not Windows uses those data. > > > We can support systems that contain correct DSDT description of cameras > > without platform data. I was, until recently, only aware of Dell XPS 9315 > > that has incorrect camera description and that based on recent findings > > seems to extend to other Dell systems with IPU6 (Hans's patches have the > > details). > > > > Still this is not a reason to break systems that have correct camera > > description and expect the users to report them so they can be listed as > > such. > > Well, what do you mean by "break". I thought that platform data > needed to support them were built into the kernel, weren't they? > > > > > On laptops shipped with Windows some additional information is also available > > > > from ACPI via custom objects but a lot of information is just hard coded into > > > > the IPU bridge as well as the INT3472 driver. > > > > > > Well, that's how it goes. > > > > Yes, but is it desirable? > > No, it is not desirable, but the way to address it is to convince the > Windows people to stop doing this and use standard-defined data from > the ACPI tables instead. It cannot be addressed by Linux unilaterally > trying to do the right thing, because there are OEMs who don't care > about Linux.
On Wed, Jun 12, 2024 at 06:40:52PM +0000, 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? > > We can support systems that contain correct DSDT description of cameras > without platform data. I was, until recently, only aware of Dell XPS 9315 > that has incorrect camera description and that based on recent findings > seems to extend to other Dell systems with IPU6 (Hans's patches have the > details). Are you aware of any IPU6-based devices, apart from chromebooks, that have correct ACPI tables for the camera ? > Still this is not a reason to break systems that have correct camera > description and expect the users to report them so they can be listed as > such. > > > > On laptops shipped with Windows some additional information is also available > > > from ACPI via custom objects but a lot of information is just hard coded into > > > the IPU bridge as well as the INT3472 driver. > > > > Well, that's how it goes. > > Yes, but is it desirable?
Hi Hans, On Wed, Jun 12, 2024 at 04:30:30PM +0200, Hans de Goede wrote: > Sakari I know you have been pushing for MIPI camera descriptions under > ACPI to move to a standardized format and I can see how that is a good > thing, but atm it seems to mainly cause things to break and before > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > since the information used by the ipu-bridge code does seem to be correct. Support for capturing from cameras on IPU6 systems (IPU6 ISYS driver and IPU bridge changes) was upstreamed for 6.10, with some drivers such as IVSC (four of them) and IVSC related IPU bridge changes merged for 6.8 already. We can't guarantee the continued functioning of downstream drivers in cases where new upstream drivers for the same devices get merged to the kernel, often with different APIs. You know that as well as I do. In other words, there was no regression with respect to the upstream kernel.
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. * 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 :-)
Hi Sakari, On Wed, Jun 12, 2024 at 10:17 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Hans, > > On Wed, Jun 12, 2024 at 04:30:30PM +0200, Hans de Goede wrote: > > Sakari I know you have been pushing for MIPI camera descriptions under > > ACPI to move to a standardized format and I can see how that is a good > > thing, but atm it seems to mainly cause things to break and before > > the ACPI MIPI DISCO support landed in 6.8 we did not have these issues, > > since the information used by the ipu-bridge code does seem to be correct. > > Support for capturing from cameras on IPU6 systems (IPU6 ISYS driver and > IPU bridge changes) was upstreamed for 6.10, with some drivers such as IVSC > (four of them) and IVSC related IPU bridge changes merged for 6.8 already. > > We can't guarantee the continued functioning of downstream drivers in cases > where new upstream drivers for the same devices get merged to the kernel, > often with different APIs. You know that as well as I do. > > In other words, there was no regression with respect to the upstream > kernel. Users' opinions on this may differ I suppose. If a user sees a new kernel warning on boot, they will easily count it as a regression, and with panic_on_warn this becomes a full-fledged kernel crash. This is bad, even though it may be coming from a new driver strictly speaking.
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.
diff --git a/drivers/acpi/mipi-disco-img.c b/drivers/acpi/mipi-disco-img.c index d05413a0672a..bf9a5cee32ac 100644 --- a/drivers/acpi/mipi-disco-img.c +++ b/drivers/acpi/mipi-disco-img.c @@ -732,6 +732,12 @@ static const struct dmi_system_id dmi_ignore_port_nodes[] = { DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"), }, }, + { + .matches = { + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9320"), + }, + }, { } };
Ignore camera related graph port nodes on Dell XPS 9320. They data in BIOS is buggy, just like it is for Dell XPS 9315. The corresponding software nodes are created by the ipu-bridge. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Hi, Could you test this and see whether it fixes the warning? The camera might work with this change, too. - Sakari drivers/acpi/mipi-disco-img.c | 6 ++++++ 1 file changed, 6 insertions(+)