Re: [PATCH v1] drm/amdgpu: Fix ISP regression issue in kernel v7.0

["Nirujogi, Pratap" <pnirujog@amd.com>]

Hi Mario,

On 3/11/2026 11:29 AM, Nirujogi, Pratap wrote:
> 
> 
> On 3/11/2026 1:26 AM, Mario Limonciello wrote:
>>
>>
>> On 3/10/2026 5:52 PM, Nirujogi, Pratap wrote:
>>>
>>>
>>> On 3/9/2026 5:58 PM, Mario Limonciello wrote:
>>>>
>>>>
>>>> On 3/9/2026 4:50 PM, Pratap Nirujogi wrote:
>>>>> Add NULL pointer checks for dev->type before accessing
>>>>> dev->type->name in ISP genpd add/remove functions to
>>>>> prevent kernel crashes. Also add MODULE_SOFTDEP to ensure
>>>>> ISP driver dependencies are loaded in correct order.
>>>>>
>>>>> The regression was introduced in kernel v7.0 where MFD ISP
>>>>> device enumeration doesn't complete by the time it is added
>>>>> to gendp. The timing of ISP device enumeration has changed
>>>>> because of the changes in registering the device sources in
>>>>> the device hierarchy.
>>>>
>>>> It's a little bit pedantic; but I /think/ there are two different 
>>>> problems here with two different root causes that both happened in 
>>>> 7.0-rc.
>>>>
>>>> As a consequence I think you should have this split out as two 
>>>> separate patches in a series linked to a Fixes tag with the reason 
>>>> for each of them.
>>>>
>>> Hi Mario,
>>>
>>> Few things to clarify before I split into 2 patches and submit v2.
>>>
>>> - Yes, you are correct, the changes in this patch are not directly 
>>> related to the Fixes tag 02c057ddefef mentioned. But since it has the 
>>> dependency on the below patch for automatic modprobe of isp to work, 
>>> I have used the same tag to cover the dependency. My apologies if 
>>> this approach is incorrect and misleading.
>>>
>>> https://lore.kernel.org/all/5986516.DvuYhMxLoT@rafael.j.wysocki/
>>>
>>> - NULL dereferencing issue with dev->type observed on v7.0 is 
>>> specific to this commit 057edc58aa5926d63840c7f30afe0953d3994fa3. As 
>>> the wakeup sources are registered using physical device instead of 
>>> ACPI device, wakeup source device (wakeup14) is added as the first 
>>> child of AMDGPU device; and since its dev->type is not initialized 
>>> properly it has resulted in segfault.
>>
>> Sure this makes sense and is a no brainer to get in.  I would just send
>> this one right now and we can keep noodling on MODULE_SOFTDEP (more
>> comments below).
>>
> thanks, I’ll go ahead and send this one out now and will submit the 
> MODULE_SOFTDEP change separately after the root cause is identified 
> ( more details below ).
> 
>>>
>>> In 6.19-rc4 or earlier versions, this issue was not observed as the 
>>> wakeup source device was never part of AMDGPU children list.
>>>
>>> For the changes in isp_v4_1_1.c, I will use Fixes tag 057edc58aa59 in 
>>> v2.
>>>
>>> - MODULE_SOFTDEP change in amdgpu_drv.c is needed for automatic 
>>> modprobe of isp (and other amdgpu mfd child devices) to work in v7.0. 
>>> But couldn't identify the specific commit in v7.0 that is causing the 
>>> issue. I can confirm it is not because of commit 057edc58aa59 as the 
>>> automatic modprobe doesn't work even on reverting this commit. Can I 
>>> submit this as the fix needed for isp probe to work in v7.0 without 
>>> the fixes tag?
>>>
>>
>> MODULE_SOFTDEP is generally for ordering, but I don't think you have an
>> issue with those modules loading before amdgpu do you?  I'm not really
>> following why the modaliases stopped working and I'm a bit worried that
>> it's papering over a more nuanced issue still.
>>
> yes, this explicit load‑order dependency was not required earlier. I 
> agree that the regression point is not clearly identified. I will 
> identify the bisected commit and submit the patch later.
> 
>> To identify the root cause, it might be helpful to do a bisect, although
>> it's a bit complicated.
>> 1) At any point that has that platform driver conversion you need to
>> either revert or add the change to auxillary
>> 2) At any point that has 057edc58aa5926d63840c7f30afe0953d3994fa3 you
>> need to apply the NULL pointer derf fix
>> 3) At any point that has the changes in linux-media for API adjustments
>> in 7.0 you'll need to pick what version of ISP series to apply.
>>
> I tried #1, #2 and few other combinations, but none of them helped. I 
> will investigate further to identify the commit causing the issue.
>>
I have submitted the patch below to address the automatic modprobe 
issue, with the appropriate Fixes tag added. Please review and share 
your feedback.

https://lore.kernel.org/all/20260318034842.1216536-1-pratap.nirujogi@amd.com/

Thanks,
Pratap

>>> Thanks,
>>> Pratap
>>>
>>>>>
>>>>> Co-developed-by: Bin Du <Bin.Du@amd.com>
>>>>> Fixes: 02c057ddefef ("ACPI: video: Convert the driver to a platform 
>>>>> one")
>>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 +
>>>>>   drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c | 4 ++--
>>>>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/ 
>>>>> drm/amd/amdgpu/amdgpu_drv.c
>>>>> index 95d26f086d545..920595f0d22ca 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>> @@ -3212,3 +3212,4 @@ module_exit(amdgpu_exit);
>>>>>   MODULE_AUTHOR(DRIVER_AUTHOR);
>>>>>   MODULE_DESCRIPTION(DRIVER_DESC);
>>>>>   MODULE_LICENSE("GPL and additional rights");
>>>>> +MODULE_SOFTDEP("post: amd_isp4_capture i2c-designware-amdisp 
>>>>> pinctrl- amdisp");
>>>>> \ No newline at end of file
>>>>> 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 b3590b33cab9e..485ecdec96184 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>>>>> @@ -129,7 +129,7 @@ static int isp_genpd_add_device(struct device 
>>>>> *dev, void *data)
>>>>>       if (!pdev)
>>>>>           return -EINVAL;
>>>>> -    if (!dev->type->name) {
>>>>> +    if (!dev->type || !dev->type->name) {
>>>>>           drm_dbg(&adev->ddev, "Invalid device type to add\n");
>>>>>           goto exit;
>>>>>       }
>>>>> @@ -165,7 +165,7 @@ static int isp_genpd_remove_device(struct 
>>>>> device *dev, void *data)
>>>>>       if (!pdev)
>>>>>           return -EINVAL;
>>>>> -    if (!dev->type->name) {
>>>>> +    if (!dev->type || !dev->type->name) {
>>>>>           drm_dbg(&adev->ddev, "Invalid device type to remove\n");
>>>>>           goto exit;
>>>>>       }
>>>>
>>>>
>>>
>>
>