[PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[Iskren Chernev]

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;
 
 	/*
-- 
2.29.2

Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[Konrad Dybcio]

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

Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[Alexey Minnekhanov]

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;
>   
>   	/*
> 

Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[Rob Clark]

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
>

Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[Rob Clark]

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

Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout

[Iskren Chernev]

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