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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
>
> ...
>
> > 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
Attachment:
signature.asc
Description: PGP signature
