Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>]

On Sun, 05 Aug 2018 18:38:18 +0100,
Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Sun, Aug 5, 2018 at 6:50 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi Vicente,
> >
> > On Sun,  5 Aug 2018 16:09:11 +0200
> > Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> As explained by Robin Murphy:
> >> > the IOMMU shutdown disables paging, so if the VOP is still
> >> > scanning out then that will result in whatever IOVAs it was using now going
> >> > straight out onto the bus as physical addresses.
> >>
> >> Suggested-by: JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> index f814d37b1db2..00a06768edb2 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
> >>       return 0;
> >>  }
> >>
> >> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> >> +{
> >> +     struct drm_device *drm = platform_get_drvdata(pdev);
> >> +
> >> +     if (drm)
> >> +             drm_atomic_helper_shutdown(drm);
> >
> > I'm not completely sure this is the right thing to do here. We want
> > shutdown to teardown the whole thing, not just the DRM context.
> >
> > The driver already calls drm_atomic_helper_shutdown as part of the
> > unbind sequence, which looks very much like what we're trying to
> > achieve here.
> 
> OK, but maybe it is calling drm_atomic_helper_shutdown too late
> because this patch makes a difference and does indeed fix the issue.

I'm not saying it doesn't fix your issue. All I'm saying is that we
may want something that is closer to a full remove than just a shallow
DRM teardown. At the moment, unbind is simply *never* called.

> Anyways, please, apply your version if you have reasons to think it
> is better suited to do the task.

That would be for the DRM maintainers to decide. For that particular
subsystem, I'm just a random contributor.

> > I've already posted a patch[1] which calls the .remove handler,
> > ensuring that the whole infrastructure gets torn down at shutdown time.
> 
> That is funny: after months of having reported the issue, we both
> sent a patch to fix it in less than 3 hours difference!

I was hoping you'd do that earlier, but given that 4.18 is just a week
away and that we still don't have a proper fix for this, I've taken
the matter into my own hands. Without this, I cannot reliably use
kexec anymore, which is just the same as not being able to boot at
all.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.