[PATCH v1] drm/amd/amdgpu: Add GPIO resources required for amdisp

[Nirujogi, Pratap]

Hi Mario,

On 5/15/2025 4:05 PM, Mario Limonciello wrote:
> On 5/15/2025 2:54 PM, Pratap Nirujogi wrote:
>> ISP is a child device to GFX, and its device specific information
>> is not available in ACPI. Adding the 2 GPIO resources required for
>> ISP_v4_1_1 in amdgpu_isp driver.
>>
>> - GPIO 0 to allow sensor driver to enable and disable sensor module.
>> - GPIO 85 to allow ISP driver to enable and disable ISP RGB streaming 
>> mode.
>>
>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi at amd.com>
>> ---
>> Changes v0 -> v1:
>>
>> * Add amdgpu_acpi_get_isp4_dev_hid() utility to retrieve isp4
>> platform device hid.
>>
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h      |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 29 ++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c  | 35 ++++++++++++++++++++++++
>>   3 files changed, 66 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/ 
>> amd/amdgpu/amdgpu.h
>> index cc26cf1bd843..b63ceead2499 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1657,10 +1657,12 @@ static inline void 
>> amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_cap
>>   bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev);
>>   bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev);
>>   void amdgpu_choose_low_power_state(struct amdgpu_device *adev);
>> +int amdgpu_acpi_get_isp4_dev_hid(char **hid);
>>   #else
>>   static inline bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device 
>> *adev) { return false; }
>>   static inline bool amdgpu_acpi_is_s3_active(struct amdgpu_device 
>> *adev) { return false; }
>>   static inline void amdgpu_choose_low_power_state(struct 
>> amdgpu_device *adev) { }
>> +static int amdgpu_acpi_get_isp4_dev_hid(char **hid) { }
>>   #endif
>>   void amdgpu_register_gpu_instance(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/ 
>> drm/amd/amdgpu/amdgpu_acpi.c
>> index b7f8f2ff143d..5e04f4b7d0ea 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> @@ -1551,4 +1551,33 @@ void amdgpu_choose_low_power_state(struct 
>> amdgpu_device *adev)
>>           adev->in_s3 = true;
>>   }
>> +static const struct acpi_device_id isp_sensor_ids[] = {
>> +    { "OMNI5C10" },
>> +    { }
>> +};
>> +
>> +static int isp_match_acpi_device_ids(struct device *dev, const void 
>> *data)
>> +{
>> +    return acpi_match_device(data, dev) ? 1 : 0;
>> +}
>> +
>> +int amdgpu_acpi_get_isp4_dev_hid(char **hid)
> 
> Looking over this signature two comments:
> 
> 1) To help avoid any risk of mistake in the future I think it's a good 
> idea to pass in the size as a second argument and then use that in 
> strscpy() below.
> 
> 2) Does this really need to be a pointer to a pointer?  The way that 
> you're using it I think you just want a single character pointer, 
> especially if you use my suggestion below of on stack memory instead.
> 
Instead of pointer to a pointer and passing size param, I will switch to 
pointer to array to simplify the implementation.

>> +{
>> +    struct acpi_device *acpi_pdev;
>> +    struct device *pdev;
>> +
>> +    pdev = bus_find_device(&platform_bus_type, NULL, isp_sensor_ids,
>> +                   isp_match_acpi_device_ids);
>> +    if (!pdev)
>> +        return -EINVAL;
>> +
>> +    acpi_pdev = ACPI_COMPANION(pdev);
>> +    if (acpi_pdev)
>> +        strscpy(*hid, acpi_device_hid(acpi_pdev), ACPI_ID_LEN);
>> +
>> +    put_device(pdev);
>> +
>> +    return 0;
>> +}
>> +
>>   #endif /* CONFIG_SUSPEND */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c b/drivers/gpu/ 
>> drm/amd/amdgpu/isp_v4_1_1.c
>> index 69dd92f6e86d..1bb927428847 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>> @@ -25,6 +25,7 @@
>>    *
>>    */
>> +#include <linux/gpio/machine.h>
>>   #include "amdgpu.h"
>>   #include "isp_v4_1_1.h"
>> @@ -39,15 +40,49 @@ static const unsigned int 
>> isp_4_1_1_int_srcid[MAX_ISP411_INT_SRC] = {
>>       ISP_4_1__SRCID__ISP_RINGBUFFER_WPT16
>>   };
>> +static struct gpiod_lookup_table isp_gpio_table = {
>> +    .dev_id = "amd_isp_capture",
>> +    .table = {
>> +        GPIO_LOOKUP("AMDI0030:00", 85, "enable_isp", GPIO_ACTIVE_HIGH),
>> +        { }
>> +    },
>> +};
>> +
>> +static struct gpiod_lookup_table isp_sensor_gpio_table = {
>> +    .dev_id = "i2c-ov05c10",
>> +    .table = {
>> +        GPIO_LOOKUP("amdisp-pinctrl", 0, "enable", GPIO_ACTIVE_HIGH),
>> +        { }
>> +    },
>> +};
>> +
>>   static int isp_v4_1_1_hw_init(struct amdgpu_isp *isp)
>>   {
>>       struct amdgpu_device *adev = isp->adev;
>>       int idx, int_idx, num_res, r;
>> +    char *isp_dev_uid;
>>       u64 isp_base;
>>       if (adev->rmmio_size == 0 || adev->rmmio_size < 0x5289)
>>           return -EINVAL;
>> +    isp_dev_uid = devm_kzalloc(adev->dev, ACPI_ID_LEN, GFP_KERNEL);
> 
> Normally I like device managed resources, but I'd say this is a case 
> that device managed resources are overkill.  This is only needed for hw 
> init, and any device missing this ACPI ID is going to have wasted memory 
> from the device managed allocation.
> 
> ACPI_ID_LEN is 16 bytes right?  We don't care about the use of this 
> after the function goes out of scope AFAICT.
> 
> Why not just put a 16 byte variable on the stack as part of hw_init() in 
> this function and write and compare with that?
> 
yes, I agree, having a local 16 bytes array is good enough and need not 
allocate using devm_kzalloc.

>> +    if (!isp_dev_uid)
>> +        return -ENOMEM;
>> +
>> +    r = amdgpu_acpi_get_isp4_dev_hid(&isp_dev_uid);
>> +    if (r) {
>> +        drm_dbg(&adev->ddev, "Invalid isp platform detected");
> 
> Since you have the error code in 'r' I'd say include it in the dynamic 
> debug statement.
> 
sure, will print the error code too in the debug statement.

Will submit v3 addressing the comments shortly.

Thanks,
Pratap

>> +        /* allow GPU init to progress */
>> +        return 0;
>> +    }
>> +
>> +    /* add GPIO resources required for OMNI5C10 sensor */
>> +    if (!strcmp("OMNI5C10", isp_dev_uid)) {
>> +        gpiod_add_lookup_table(&isp_gpio_table);
>> +        gpiod_add_lookup_table(&isp_sensor_gpio_table);
>> +    }
>> +
>>       isp_base = adev->rmmio_base;
>>       isp->isp_cell = kcalloc(3, sizeof(struct mfd_cell), GFP_KERNEL);
>