From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown Date: Sun, 05 Aug 2018 19:23:58 +0100 Message-ID: <861sbcr7sx.wl-marc.zyngier@arm.com> References: <5B20F5F0.9090101@rock-chips.com> <20180805140911.19205-1-vicencb@gmail.com> <20180805175038.1d3a0c5e@why.wild-wind.fr.eu.org> Mime-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Vicente Bergas Cc: "open list:ARM/Rockchip SoC..." , JeffyChen , Robin Murphy , Heiko Stuebner , Tomasz Figa List-Id: linux-rockchip.vger.kernel.org On Sun, 05 Aug 2018 18:38:18 +0100, Vicente Bergas wrote: > > On Sun, Aug 5, 2018 at 6:50 PM, Marc Zyngier wrote: > > Hi Vicente, > > > > On Sun, 5 Aug 2018 16:09:11 +0200 > > Vicente Bergas 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 > >> Suggested-by: Robin Murphy > >> Signed-off-by: Vicente Bergas > >> --- > >> 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.