Re: [PATCH] platform/x86: amd: Add ISP platform info

[Armin Wolf <W_Armin@gmx.de>]

Am 04.03.25 um 00:14 schrieb Nirujogi, Pratap:

> Hi Hans,
>
> Thanks for your review. Please see the inline comments and let us know
> your insights.
>
> Thanks,
> Pratap
>
>
> On 3/3/2025 8:41 AM, Hans de Goede wrote:
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Hi Pratap,
>>
>> Thank you for your patch.
>>
>> On 28-Feb-25 18:02, Pratap Nirujogi wrote:
>>> Add ov05c i2c boardinfo and GPIO pin info for AMD ISP platform.
>>>
>>> Details of the resources added:
>>>
>>> - Added i2c bus number for AMD ISP platform is 99.
>>> - Added GPIO 85 to allow ISP driver to enable and disable ISP access.
>>> - Added GPIO 0 to allow sensor driver to enable and disable sensor
>>> module.
>>>
>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>> ---
>>>   drivers/platform/x86/amd/Kconfig   | 11 +++++
>>>   drivers/platform/x86/amd/Makefile  |  1 +
>>>   drivers/platform/x86/amd/amd_isp.c | 72
>>> ++++++++++++++++++++++++++++++
>>>   3 files changed, 84 insertions(+)
>>>   create mode 100644 drivers/platform/x86/amd/amd_isp.c
>>>
>>> diff --git a/drivers/platform/x86/amd/Kconfig
>>> b/drivers/platform/x86/amd/Kconfig
>>> index c3e086ea64fc..4b373edd750d 100644
>>> --- a/drivers/platform/x86/amd/Kconfig
>>> +++ b/drivers/platform/x86/amd/Kconfig
>>> @@ -32,3 +32,14 @@ config AMD_WBRF
>>>
>>>          This mechanism will only be activated on platforms that
>>> advertise a
>>>          need for it.
>>> +
>>> +config AMD_ISP_PLATFORM
>>> +     bool "AMD platform with ISP4 that supports Camera sensor device"
>>> +     depends on I2C && X86_64 && AMD_ISP4
>>> +     help
>>> +       For AMD platform that support Image signal processor
>>> generation 4, it
>>> +       is necessary to add platform specific camera sensor module
>>> board info
>>> +       which includes the sensor driver device id and the i2c address.
>>> +
>>> +       If you have a AMD platform that support ISP4 and with a sensor
>>> +       connected to it, say Y here
>>> diff --git a/drivers/platform/x86/amd/Makefile
>>> b/drivers/platform/x86/amd/Makefile
>>> index 56f62fc9c97b..0d89e2d4f7e6 100644
>>> --- a/drivers/platform/x86/amd/Makefile
>>> +++ b/drivers/platform/x86/amd/Makefile
>>> @@ -10,3 +10,4 @@ obj-$(CONFIG_AMD_PMC)               += pmc/
>>>   obj-$(CONFIG_AMD_HSMP)               += hsmp/
>>>   obj-$(CONFIG_AMD_PMF)                += pmf/
>>>   obj-$(CONFIG_AMD_WBRF)               += wbrf.o
>>> +obj-$(CONFIG_AMD_ISP_PLATFORM)       += amd_isp.o
>>> diff --git a/drivers/platform/x86/amd/amd_isp.c
>>> b/drivers/platform/x86/amd/amd_isp.c
>>> new file mode 100644
>>> index 000000000000..751f209e9509
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/amd/amd_isp.c
>>> @@ -0,0 +1,72 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright 2025 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> obtaining a
>>> + * copy of this software and associated documentation files (the
>>> "Software"),
>>> + * to deal in the Software without restriction, including without
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to
>>> whom the
>>> + * Software is furnished to do so, subject to the following
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be
>>> included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
>>> EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>>> DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>> OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
>>> USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#include <linux/init.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/gpio/machine.h>
>>> +
>>> +#define AMDISP_I2C_BUS               99
>>
>> I'm not a fan of using static i2c-bus numbers for this. static bus
>> numbers are
>> something of the past and we typically do not use these on x86 anymore.
>>
>> Using this static number + i2c_register_board_info() also requires
>> this code
>> to be builtin rather then modular which is also undesirable.
>>
>> For a more dynamic way of manually adding i2c-devices see:
>>
>> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/dell/dell-lis3lv02d.c
>>
>>
>> But a better question here is why instantiate the sensor i2c device
>> manually at all.
>>
>> ACPI has a standardized way to describe I2C-clients which tyically
>> is used for all I2C devices on ACPI platforms like I2C touchscreens /
>> touchpads / audio-codecs / accelerometers / etc.
>> I don't see why the camera sensor on AMD platforms is so special that
>> it could not be described in ACPI using an ACPI child-device of the
>> i2c-controller with a ACPI resource (_CRS entry) of the I2cSerialBusV2()
>> type.
>>
>> Likewise the sensor enable GPIO should also be described in the ACPI
>> table as a Gpio type resource in the same _CRS table.
>>
>
> We have to take this approach because ISP is a child to GFX PCI device
> in AMD HW architectures, and since it is not an independent device,
> its device specific configuration (gpio pin ids, i2c-bus number etc.)
> is not registered in ACPI.
>
>> Can you run acpidump -o acpidump.txt on a laptop with this camera
>> sensor and send me the acpidupm.txt offlist ? Please run this
>> on a production hardware laptop model using production firmware.
>>
>
> Please refer the attached acpidump.txt
>
>> I suspect that Windows will also be using the ACPI description
>> for the sensor so we really should figure out what Windows is doing
>> here.
>>
>
> Yes, same ACPI configuration for both Windows and Linux. Similar
> approach followed even on windows to control the isp gpio pins.
>
The OMNI5C10 ACPI device has a _DSM method that supports the GUID f8fd3bff-21b7-4a99-bdc8-c414a3e9453c. Do you know
more about the purpose of this method?

Thanks,
Armin Wolf

>> As Mario mentioned we cannot just assume that the GPIOs +
>> sensor address and model are valid for all laptops. Ideally we should
>> be getting this information from ACPI rather then hardcoding it
>> in the kernel.
>>
>
> Yes, we initially assumed CONFIG_AMD_ISP_PLATFORM=y will be set only
> on the intended platforms, but as that assumption is not valid, the below
> check is added in v2 patch checking the specific ov05c acpi hw id to
> present before running the driver.
>
> /* check for valid platform before configuring isp4 board resources */
>     if (!acpi_dev_found(AMDISP_ACPI_CAM_HID))
>         return -ENODEV;
>
>>> +
>>> +static struct gpiod_lookup_table isp_gpio_table = {
>>> +     .dev_id = "amd_isp_capture",
>>> +     .table = {
>>> +             GPIO_LOOKUP("AMDI0030:00", 85, "enable_isp",
>>> GPIO_ACTIVE_HIGH),
>>> +             { }
>>> +     },
>>> +};
>>
>> This too really should be an Gpio() type ACPI resource on the ACPI
>> device
>> node for the ISP.
>>
>> How/where is this "amd_isp_capture" device created ?
>>
>
> "amd_isp_capture" is the V4L2 ISP driver in this case. The patches for
> ISP driver are yet to be submitted. It will be loaded during AMDGPU
> device probe on the AMD platforms supporting isp4.2 HW. AMDGPU
> reference to trigger the isp device probe:
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c#L108
>
>
>> Regards,
>>
>> Hans
>>
>>
>>> +
>>> +static struct gpiod_lookup_table isp_sensor_gpio_table = {
>>> +     .dev_id = "ov05c",
>>> +     .table = {
>>> +             GPIO_LOOKUP("amdisp-pinctrl", 0, "sensor0_enable",
>>> GPIO_ACTIVE_HIGH),
>>> +             { }
>>> +     },
>>> +};
>>> +
>>> +static struct i2c_board_info sensor_info = {
>>> +     .dev_name = "ov05c",
>>> +     I2C_BOARD_INFO("ov05c", 0x10),
>>> +};
>>> +
>>> +static int __init amd_isp_init(void)
>>> +{
>>> +     int ret;
>>> +
>>> +     gpiod_add_lookup_table(&isp_gpio_table);
>>> +     gpiod_add_lookup_table(&isp_sensor_gpio_table);
>>> +
>>> +     ret = i2c_register_board_info(AMDISP_I2C_BUS, &sensor_info, 1);
>>> +     if (ret)
>>> +             pr_err("%s: cannot register i2c board devices:%s",
>>> +                    __func__, sensor_info.dev_name);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +arch_initcall(amd_isp_init);
>>> +
>>> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>>> +MODULE_DESCRIPTION("AMD ISP Platform parameters");
>>> +MODULE_LICENSE("GPL and additional rights");
>>