| Message ID | 20210102202437.1630365-1-iskren.chernev@gmail.com |
|---|---|
| State | Accepted |
| Commit | 6cefa31e810404dafdfcdb94874146cea11626c2 |
| Headers | show |
| Series | drm/msm: Fix MSM_INFO_GET_IOVA with carveout | expand |
Kind reminder that MSM8974, 8994, 8992 and friends are held back by the lack of IOMMU support upstream. There has been an attempt back in 2014(!) [1], but it was either overlooked or forgotten about ever since. I'd be more than happy to see someone look into this, as I have some other bits (almost) ready for both 8974 and 94, but MMUs aren't something I understand well enough yet. Konrad [1] https://lists.linuxfoundation.org/pipermail/iommu/2014-June/008993.html
I've tested all recent GPU bring-up patches on msm8974pro samsung-klte (a330v2) and with this patch everything is OK. But without this we're getting the following in dmesg while running kmscube (which is rendering nothing except black screen): [ 94.969272] msm fd900000.mdss: [drm:hangcheck_handler [msm]] *ERROR* A330: hangcheck detected gpu lockup rb 0! [ 94.970184] msm fd900000.mdss: [drm:hangcheck_handler [msm]] *ERROR* A330: completed fence: 0 [ 94.970873] msm fd900000.mdss: [drm:hangcheck_handler [msm]] *ERROR* A330: submitted fence: 1 [ 94.971600] msm fd900000.mdss: [drm:recover_worker [msm]] *ERROR* A330: hangcheck recover! [ 94.972329] msm fd900000.mdss: [drm:recover_worker [msm]] *ERROR* A330: offending task: kmscube (kmscube) [ 94.974101] revision: 330 (3.3.0.2) [ 94.974117] rb 0: fence: 0/1 [ 94.974129] rptr: 36 [ 94.974139] rb wptr: 36 [ 94.974148] CP_SCRATCH_REG0: 0 [ 94.974159] CP_SCRATCH_REG1: 0 [ 94.974169] CP_SCRATCH_REG2: 0 [ 94.974178] CP_SCRATCH_REG3: 0 [ 94.974188] CP_SCRATCH_REG4: 0 [ 94.974198] CP_SCRATCH_REG5: 0 [ 94.974208] CP_SCRATCH_REG6: 10 [ 94.974218] CP_SCRATCH_REG7: 12 So indeed partial revert of "if" condition fixes gpu at least on msm8974. Tested-by: Alexey Minnekhanov <alexeymin@postmarketos.org> On 1/2/21 11:24 PM, Iskren Chernev wrote: > The msm_gem_get_iova should be guarded with gpu != NULL and not aspace > != NULL, because aspace is NULL when using vram carveout. > > Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > --- > drivers/gpu/drm/msm/msm_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index c5e61cb3356df..c1953fb079133 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, > struct drm_file *file, struct drm_gem_object *obj, > uint64_t *iova) > { > + struct msm_drm_private *priv = dev->dev_private; > struct msm_file_private *ctx = file->driver_priv; > > - if (!ctx->aspace) > + if (!priv->gpu) > return -EINVAL; > > /* >
On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote: > > The msm_gem_get_iova should be guarded with gpu != NULL and not aspace > != NULL, because aspace is NULL when using vram carveout. > > Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > --- > drivers/gpu/drm/msm/msm_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index c5e61cb3356df..c1953fb079133 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, > struct drm_file *file, struct drm_gem_object *obj, > uint64_t *iova) > { > + struct msm_drm_private *priv = dev->dev_private; > struct msm_file_private *ctx = file->driver_priv; > > - if (!ctx->aspace) > + if (!priv->gpu) > return -EINVAL; Does this actually work? It seems like you would hit a null ptr deref in msm_gem_init_vma().. and in general I think a lot of code paths would be surprised by a null address space, so this seems like a risky idea. Maybe instead we should be creating an address space for the vram carveout? BR, -R > /* > -- > 2.29.2 >
On Thu, Jan 7, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote: > > On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote: > > > > The msm_gem_get_iova should be guarded with gpu != NULL and not aspace > > != NULL, because aspace is NULL when using vram carveout. > > > > Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") > > > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > > --- > > drivers/gpu/drm/msm/msm_drv.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index c5e61cb3356df..c1953fb079133 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, > > struct drm_file *file, struct drm_gem_object *obj, > > uint64_t *iova) > > { > > + struct msm_drm_private *priv = dev->dev_private; > > struct msm_file_private *ctx = file->driver_priv; > > > > - if (!ctx->aspace) > > + if (!priv->gpu) > > return -EINVAL; > > Does this actually work? It seems like you would hit a null ptr deref > in msm_gem_init_vma().. and in general I think a lot of code paths > would be surprised by a null address space, so this seems like a risky > idea. oh, actually, I suppose it is ok, since in the vram carveout case we create the vma up front when the gem obj is created.. (still, it does seem a bit fragile.. and easy for folks testing on devices not using vram carvout to break.. hmm..) BR, -R > Maybe instead we should be creating an address space for the vram carveout? > > BR, > -R > > > > /* > > -- > > 2.29.2 > >
On 1/8/21 12:36 AM, Rob Clark wrote: > On Thu, Jan 7, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote: >> >> On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote: >>> >>> The msm_gem_get_iova should be guarded with gpu != NULL and not aspace >>> != NULL, because aspace is NULL when using vram carveout. >>> >>> Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") >>> >>> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> >>> --- >>> drivers/gpu/drm/msm/msm_drv.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c >>> index c5e61cb3356df..c1953fb079133 100644 >>> --- a/drivers/gpu/drm/msm/msm_drv.c >>> +++ b/drivers/gpu/drm/msm/msm_drv.c >>> @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, >>> struct drm_file *file, struct drm_gem_object *obj, >>> uint64_t *iova) >>> { >>> + struct msm_drm_private *priv = dev->dev_private; >>> struct msm_file_private *ctx = file->driver_priv; >>> >>> - if (!ctx->aspace) >>> + if (!priv->gpu) >>> return -EINVAL; >> >> Does this actually work? It seems like you would hit a null ptr deref >> in msm_gem_init_vma().. and in general I think a lot of code paths >> would be surprised by a null address space, so this seems like a risky >> idea. > > oh, actually, I suppose it is ok, since in the vram carveout case we > create the vma up front when the gem obj is created.. > > (still, it does seem a bit fragile.. and easy for folks testing on > devices not using vram carvout to break.. hmm..) In _msm_gem_new add_vma is called with NULL, so consequently lookup_vma finds it when aspace is NULL. Also, this is how the code was before the "breaking" change, so it should not be worse. I'll be happy to work on refactoring this a bit, but some some documentation about the different gpu/mdp pieces and how they fit together won't hurt. Regards, Iskren > BR, > -R > >> Maybe instead we should be creating an address space for the vram carveout? >> >> BR, >> -R >> >> >>> /* >>> -- >>> 2.29.2 >>>
Hello: This patch was applied to qcom/linux.git (refs/heads/for-next): On Sat, 2 Jan 2021 22:24:37 +0200 you wrote: > The msm_gem_get_iova should be guarded with gpu != NULL and not aspace > != NULL, because aspace is NULL when using vram carveout. > > Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > > [...] Here is the summary with links: - drm/msm: Fix MSM_INFO_GET_IOVA with carveout https://git.kernel.org/qcom/c/6cefa31e8104 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index c5e61cb3356df..c1953fb079133 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, struct drm_file *file, struct drm_gem_object *obj, uint64_t *iova) { + struct msm_drm_private *priv = dev->dev_private; struct msm_file_private *ctx = file->driver_priv; - if (!ctx->aspace) + if (!priv->gpu) return -EINVAL; /*
The msm_gem_get_iova should be guarded with gpu != NULL and not aspace != NULL, because aspace is NULL when using vram carveout. Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> --- drivers/gpu/drm/msm/msm_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)