diff mbox series

[1/1] ACPI: scan: Ignore Dell XPS 9320 camera graph port nodes

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

Commit Message

Sakari Ailus May 28, 2024, 8:44 a.m. UTC 
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(+)

Comments

Hans de Goede May 28, 2024, 5:09 p.m. UTC | #1 
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.

Regards,

Hans




> 
> - 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"),
> +		},
> +	},
>  	{ }
>  };
>
Wu, Wentong May 30, 2024, 12:35 a.m. UTC | #2 
> 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
Hans de Goede June 6, 2024, 6:12 p.m. UTC | #3 
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"),
>> +		},
>> +	},
>>  	{ }
>>  };
>>
Rafael J. Wysocki June 7, 2024, 7:55 a.m. UTC | #4 
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.
Sakari Ailus June 7, 2024, 9:24 a.m. UTC | #5 
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.
Hans de Goede June 12, 2024, 10:08 a.m. UTC | #6 
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"),
>>> +		},
>>> +	},
>>>  	{ }
>>>  };
>>>  
>
Rafael J. Wysocki June 12, 2024, 10:25 a.m. UTC | #7 
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!
Sakari Ailus June 12, 2024, 12:10 p.m. UTC | #8 
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.
Rafael J. Wysocki June 12, 2024, 12:21 p.m. UTC | #9 
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?
Sakari Ailus June 12, 2024, 12:26 p.m. UTC | #10 
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.
Rafael J. Wysocki June 12, 2024, 12:32 p.m. UTC | #11 
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?
Sakari Ailus June 12, 2024, 12:47 p.m. UTC | #12 
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.
Rafael J. Wysocki June 12, 2024, 1:06 p.m. UTC | #13 
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?
Hans de Goede June 12, 2024, 2:30 p.m. UTC | #14 
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
Laurent Pinchart June 12, 2024, 2:39 p.m. UTC | #15 
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.
Rafael J. Wysocki June 12, 2024, 3:26 p.m. UTC | #16 
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".
Sakari Ailus June 12, 2024, 6:21 p.m. UTC | #17 
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.
Sakari Ailus June 12, 2024, 6:24 p.m. UTC | #18 
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.
Rafael J. Wysocki June 12, 2024, 6:29 p.m. UTC | #19 
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.
Sakari Ailus June 12, 2024, 6:33 p.m. UTC | #20 
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".
Sakari Ailus June 12, 2024, 6:40 p.m. UTC | #21 
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?
Rafael J. Wysocki June 12, 2024, 6:41 p.m. UTC | #22 
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?
Rafael J. Wysocki June 12, 2024, 6:50 p.m. UTC | #23 
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.
Sakari Ailus June 12, 2024, 7:06 p.m. UTC | #24 
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.
Rafael J. Wysocki June 12, 2024, 7:07 p.m. UTC | #25 
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?
Sakari Ailus June 12, 2024, 7:11 p.m. UTC | #26 
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.
Sakari Ailus June 12, 2024, 7:17 p.m. UTC | #27 
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?
Rafael J. Wysocki June 12, 2024, 7:32 p.m. UTC | #28 
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.
Sakari Ailus June 12, 2024, 8 p.m. UTC | #29 
Hi Rafael,

On Wed, Jun 12, 2024 at 09:32:18PM +0200, Rafael J. Wysocki wrote:
> > 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.

This warning comes from drivers/media/pci/intel/ivsc/mei_csi.c line 681 and
it is related to IVSC (Intel Vision Sensing Controller) present on some
systems with IPU6. The driver is necessary for the camera to work in these
systems but then again not all the necessary drivers were in place before
6.10 so this can't be said to be a regression.

The warning is made less verbose by commit
cc864821c7e8b921ebbfb21b17c92f8b3ea3d7ff (on Linus's tree).
Laurent Pinchart June 12, 2024, 8:08 p.m. UTC | #30 
Hi Rafael,

On Wed, Jun 12, 2024 at 09:07:09PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 12, 2024 at 4:40 PM Laurent Pinchart wrote:
> > On Wed, Jun 12, 2024 at 04:30:30PM +0200, Hans de Goede wrote:
> > > On 6/12/24 3:06 PM, Rafael J. Wysocki wrote:
> > > > On Wed, Jun 12, 2024 at 2:47 PM Sakari Ailus wrote:
> > > >> On Wed, Jun 12, 2024 at 02:32:26PM +0200, Rafael J. Wysocki wrote:
> > > >>>>>>> I just hit the same problem on another Dell laptop. It seems that
> > > >>>>>>> all Dell laptops with IPU6 camera from the Tiger Lake, Alder Lake
> > > >>>>>>> and Raptor Lake generations suffer from this problem.
> > > >>>>>>>
> > > >>>>>>> So instead of playing whack a mole with DMI matches we should
> > > >>>>>>> simply disable ACPI MIPI DISCO support on all Dell laptops
> > > >>>>>>> with those CPUs. I'm preparing a fix for this to replace
> > > >>>>>>> the DMI matching now.
> > > >>>>>>
> > > >>>>>> DisCo for Imaging support shouldn't be dropped on these systems, and this
> > > >>>>>> isn't what your patch does either. Instead the ACPI graph port nodes (as
> > > >>>>>> per Linux specific definitions) are simply dropped, i.e. this isn't related
> > > >>>>>> to DisCo for Imaging at all.
> > > >>>>>
> > > >>>>> So it looks like the changelog of that patch could be improved, right?
> > > >>>>
> > > >>>> Well, yes. The reason the function is in the file is that nearly all camera
> > > >>>> related parsing is located there, not that it would be related to DisCo for
> > > >>>> Imaging as such.
> > > >>>
> > > >>> So IIUC the camera graph port nodes are created by default with the
> > > >>> help of the firmware-supplied information, but if that is defective a
> > > >>> quirk can be added to skip the creation of those ports in which case
> > > >>> they will be created elsewhere.
> > > >>>
> > > >>> Is this correct?
> > > >>
> > > >> Yes.
> > > >
> > > > So it would be good to add a comment to this effect to
> > > > acpi_nondev_subnode_extract() where acpi_graph_ignore_port() is
> > > > called.
> > > >
> > > > And there is a somewhat tangential question that occurred to me: If
> > > > the nodes are created elsewhere when acpi_graph_ignore_port() is true,
> > > > why is it necessary to consult the platform firmware for the
> > > > information on them at all?  Wouldn't it be better to simply always
> > > > create them elsewhere?
> > >
> > > That is a good question. The ACPI MIPI DISCO specification is an
> > > attempt standardize how MIPI cameras and their sensors are described
> > > in ACPI.
> > >
> > > But this is not actually being used by any Windows drivers atm. The windows
> > > drivers rely on their own custom ACPI data which gets translated into
> > > standard Linux device-properties by: drivers/media/pci/intel/ipu-bridge.c
> > >
> > > and so far AFAIK there are 0 laptops where there actually is 100% functional
> > > ACPI MIPI information. I believe that some work is in place to get correct
> > > usable ACPI MIPI information in place in the ACPI tables of some Meteor Lake
> > > laptops. But I believe that there too it does not work yet with the BIOS
> > > version with which current Windows models are shipping. It is being fixed
> > > for systems which have Linux support from the vendor but I suspect that
> >
> > I think it's shipped in Chrome Books. Sakari can confirm.
> >
> > > on other models if ACPI MIPI DISCO information is there it will not
> > > necessarily be reliable because AFAICT Windows does not actually use it.
> > >
> > > And TBH this has me worried about camera support for Meteor Lake devices
> > > going forward. We really need to have 1 reliable source of truth here and
> > > using information which is ignored by Windows does not seem like the best
> > > source to use.
> >
> > As long as the Windows drivers don't use the ACPI data that Linux uses,
> > you can be 100% sure it will be wrong. That will never be fixed if Intel
> > doesn't address the issue on their side, and effort we would put in
> > standardizing that data for machines shipped by Windows OEMs is a waste
> > of time in my opinion. Our only option, given Intel's failure, is to
> > keep reverse-engineering camera support per machine for the time being
> > (sometimes with the help of OEMs).
> 
> So while I kind of agree with you on the technical side (as you can
> see from my messages in this thread), there is one thing in the above
> paragraph that I need to react to.
> 
> Namely, both I and Sakari are Intel employees.  Do you think or are
> you suggesting that we are somehow responsible for this failure?
> Because I personally don't think like I have anything to do with it.

That's a worthy clarification: I blame neither you nor Sakari, quite the
contrary. As far as I can tell, both of you have done and are doing your
best to fix this kind of issues. Sakari has helped standardizing DisCo
for Imaging for instance, which was the best outcome we could
realistically hope for in this context. Intel is a large company, and as
with any large company, some people end up having to try and fix the
mess created by others :-S I'm sorry it's falling on you and Sakari.

> What do you mean, specifically, by saying "if Intel doesn't address
> the issue on their side"?  And who at Intel do I need to talk to about
> this?

I think you would need to get the Windows and Linux side of the Intel
team(s) responsible for camera drivers and ACPI integration, along with
a manager who can understand the problem, the bad PR, and get the
stakeholders to cooperate and do better in the future. I don't know who
that would be though, what I know is that it doesn't seem the issue will
solve itself without someone with decision power decides to fix it.
Sakari Ailus June 12, 2024, 8:17 p.m. UTC | #31 
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.
Rafael J. Wysocki June 12, 2024, 8:31 p.m. UTC | #32 
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.
Rafael J. Wysocki June 13, 2024, 9:50 a.m. UTC | #33 
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.
Rafael J. Wysocki June 13, 2024, 12:37 p.m. UTC | #34 
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 mbox series

Patch

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"),
+		},
+	},
 	{ }
 };