[PATCH] gpu: host1x: Request syncpoint IRQs only during probe

[Jon Hunter]

On 25/09/2024 13:58, Thierry Reding wrote:
> On Tue, Sep 24, 2024 at 07:33:05PM GMT, Jon Hunter wrote:
>>
>> On 06/09/2024 09:38, Jon Hunter wrote:
>>> Hi Mikko,
>>>
>>> On 31/05/2024 08:07, Mikko Perttunen wrote:
>>>> From: Mikko Perttunen <mperttunen at nvidia.com>
>>>>
>>>> Syncpoint IRQs are currently requested in a code path that runs
>>>> during resume. Due to this, we get multiple overlapping registered
>>>> interrupt handlers as host1x is suspended and resumed.
>>>>
>>>> Rearrange interrupt code to only request IRQs during initialization.
>>>>
>>>> Signed-off-by: Mikko Perttunen <mperttunen at nvidia.com>
>>
>> ...
>>
>>> This change is causing a boot regression on Tegra186 with the latest
>>> -next. I have reverted this to confirm that this fixes the problem. I
>>> don't see any crash log but the board appears to just hang.
>>
>>
>> I had a look at this and I was able to fix this by moving where
>> we initialise the interrupts to after the PM runtime enable ...
>>
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
>> index b62e4f0e8130..ff98d4903cac 100644
>> --- a/drivers/gpu/host1x/dev.c
>> +++ b/drivers/gpu/host1x/dev.c
>> @@ -625,12 +625,6 @@ static int host1x_probe(struct platform_device *pdev)
>>                  goto free_contexts;
>>          }
>> -       err = host1x_intr_init(host);
>> -       if (err) {
>> -               dev_err(&pdev->dev, "failed to initialize interrupts\n");
>> -               goto deinit_syncpt;
>> -       }
>> -
>>          pm_runtime_enable(&pdev->dev);
>>          err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
>> @@ -642,6 +636,12 @@ static int host1x_probe(struct platform_device *pdev)
>>          if (err)
>>                  goto pm_disable;
>> +       err = host1x_intr_init(host);
>> +       if (err) {
>> +               dev_err(&pdev->dev, "failed to initialize interrupts\n");
>> +               goto pm_put;
>> +       }
>> +
> 
> I think the reason why this might fail now is because host1x_intr_init()
> ends up writing some registers during the call to the
> host1x_hw_intr_disable_all_syncpt_intrs() function, which would hang or
> crash if the device isn't on (which in turn happens during
> pm_runtime_resume_and_get()).

Yes that is exactly where it hung!

> Not sure exactly why this used to work because prior to Mikko's patch
> because the sequence was the same before.

Previously host1x_intr_init() did not call 
host1x_hw_intr_disable_all_syncpt_intrs().

> 
>>          host1x_debug_init(host);
>>          err = host1x_register(host);
>> @@ -658,14 +658,11 @@ static int host1x_probe(struct platform_device *pdev)
>>          host1x_unregister(host);
>>   deinit_debugfs:
>>          host1x_debug_deinit(host);
>> -
>> +       host1x_intr_deinit(host);
>> +pm_put:
>>          pm_runtime_put_sync_suspend(&pdev->dev);
>>   pm_disable:
>>          pm_runtime_disable(&pdev->dev);
>> -
>> -       host1x_intr_deinit(host);
>> -deinit_syncpt:
>> -       host1x_syncpt_deinit(host);
>>   free_contexts:
>>          host1x_memory_context_list_free(&host->context_list);
>>   free_channels:
>>
>>
>> Thierry, do you want to me to send a fix for the above or do you
>> want to squash with the original (assuming that OK with Mikko)?
> 
> In any case, this looks like a good fix, so please send a proper patch.
> This is merged through drm-misc, so squashing this into the original
> patch is not an option any longer.

Yes will do!

Jon

-- 
nvpublic