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

[Thierry Reding]

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()).

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

>         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.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240925/0d466f1e/attachment.sig>