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.niruj...@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);
