diff mbox series

[1/2] drm/msm/adreno: Fix warn splat for devices without revn

Message ID 20230704163640.6162-1-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/msm/adreno: Fix warn splat for devices without revn | expand

Commit Message

Rob Clark July 4, 2023, 4:36 p.m. UTC 
From: Rob Clark <robdclark@chromium.org>

Recently, a WARN_ON() was introduced to ensure that revn is filled before
adreno_is_aXYZ is called. This however doesn't work very well when revn is
0 by design (such as for A635).

Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set")
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Dmitry Baryshkov July 4, 2023, 5:19 p.m. UTC | #1 
On Tue, 4 Jul 2023 at 19:36, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Recently, a WARN_ON() was introduced to ensure that revn is filled before
> adreno_is_aXYZ is called. This however doesn't work very well when revn is
> 0 by design (such as for A635).
>
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set")
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 65379e4824d9..ef1bcb6b624e 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -149,7 +149,8 @@ bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>
>  static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
>  {
> -       WARN_ON_ONCE(!gpu->revn);
> +       /* revn can be zero, but if not is set at same time as info */
> +       WARN_ON_ONCE(!gpu->info);
>
>         return gpu->revn == revn;
>  }
> @@ -161,14 +162,16 @@ static inline bool adreno_has_gmu_wrapper(const struct adreno_gpu *gpu)
>
>  static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
>  {
> -       WARN_ON_ONCE(!gpu->revn);
> +       /* revn can be zero, but if not is set at same time as info */
> +       WARN_ON_ONCE(!gpu->info);
>
>         return (gpu->revn < 300);

This is then incorrect for a635 / a690 if they have revn at 0.

>  }
>
>  static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
>  {
> -       WARN_ON_ONCE(!gpu->revn);
> +       /* revn can be zero, but if not is set at same time as info */
> +       WARN_ON_ONCE(!gpu->info);
>
>         return (gpu->revn < 210);

And this too.

>  }
> --
> 2.41.0
>
Rob Clark July 5, 2023, 2:42 p.m. UTC | #2 
On Tue, Jul 4, 2023 at 10:20 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 4 Jul 2023 at 19:36, Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Recently, a WARN_ON() was introduced to ensure that revn is filled before
> > adreno_is_aXYZ is called. This however doesn't work very well when revn is
> > 0 by design (such as for A635).
> >
> > Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set")
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 65379e4824d9..ef1bcb6b624e 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -149,7 +149,8 @@ bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
> >
> >  static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
> >  {
> > -       WARN_ON_ONCE(!gpu->revn);
> > +       /* revn can be zero, but if not is set at same time as info */
> > +       WARN_ON_ONCE(!gpu->info);
> >
> >         return gpu->revn == revn;
> >  }
> > @@ -161,14 +162,16 @@ static inline bool adreno_has_gmu_wrapper(const struct adreno_gpu *gpu)
> >
> >  static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
> >  {
> > -       WARN_ON_ONCE(!gpu->revn);
> > +       /* revn can be zero, but if not is set at same time as info */
> > +       WARN_ON_ONCE(!gpu->info);
> >
> >         return (gpu->revn < 300);
>
> This is then incorrect for a635 / a690 if they have revn at 0.

Fortunately not any more broken that it has ever been.  But as long as
sc7280/sc8280 have GPU OPP tables, you'd never hit this.  I'm working
on a better solution for next merge window.

BR,
-R

> >  }
> >
> >  static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
> >  {
> > -       WARN_ON_ONCE(!gpu->revn);
> > +       /* revn can be zero, but if not is set at same time as info */
> > +       WARN_ON_ONCE(!gpu->info);
> >
> >         return (gpu->revn < 210);
>
> And this too.
>
> >  }
> > --
> > 2.41.0
> >
>
>
> --
> With best wishes
> Dmitry
Dmitry Baryshkov July 13, 2023, 7:54 p.m. UTC | #3 
On 05/07/2023 17:42, Rob Clark wrote:
> On Tue, Jul 4, 2023 at 10:20 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Tue, 4 Jul 2023 at 19:36, Rob Clark <robdclark@gmail.com> wrote:
>>>
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Recently, a WARN_ON() was introduced to ensure that revn is filled before
>>> adreno_is_aXYZ is called. This however doesn't work very well when revn is
>>> 0 by design (such as for A635).
>>>
>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set")
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>   drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> index 65379e4824d9..ef1bcb6b624e 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> @@ -149,7 +149,8 @@ bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>>>
>>>   static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
>>>   {
>>> -       WARN_ON_ONCE(!gpu->revn);
>>> +       /* revn can be zero, but if not is set at same time as info */
>>> +       WARN_ON_ONCE(!gpu->info);
>>>
>>>          return gpu->revn == revn;
>>>   }
>>> @@ -161,14 +162,16 @@ static inline bool adreno_has_gmu_wrapper(const struct adreno_gpu *gpu)
>>>
>>>   static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
>>>   {
>>> -       WARN_ON_ONCE(!gpu->revn);
>>> +       /* revn can be zero, but if not is set at same time as info */
>>> +       WARN_ON_ONCE(!gpu->info);
>>>
>>>          return (gpu->revn < 300);
>>
>> This is then incorrect for a635 / a690 if they have revn at 0.
> 
> Fortunately not any more broken that it has ever been.  But as long as
> sc7280/sc8280 have GPU OPP tables, you'd never hit this.  I'm working
> on a better solution for next merge window.

Sounds fine with me then.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> BR,
> -R
> 
>>>   }
>>>
>>>   static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
>>>   {
>>> -       WARN_ON_ONCE(!gpu->revn);
>>> +       /* revn can be zero, but if not is set at same time as info */
>>> +       WARN_ON_ONCE(!gpu->info);
>>>
>>>          return (gpu->revn < 210);
>>
>> And this too.
>>
>>>   }
>>> --
>>> 2.41.0
>>>
>>
>>
>> --
>> With best wishes
>> Dmitry
Abhinav Kumar July 14, 2023, 10:06 p.m. UTC | #4 
On 7/4/2023 9:36 AM, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Recently, a WARN_ON() was introduced to ensure that revn is filled before
> adreno_is_aXYZ is called. This however doesn't work very well when revn is
> 0 by design (such as for A635).
> 
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set")
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 

Tested-by: Abhinav Kumar <quic_abhinavk@quicinc.com> # sc7280
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 65379e4824d9..ef1bcb6b624e 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -149,7 +149,8 @@  bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
 
 static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
 {
-	WARN_ON_ONCE(!gpu->revn);
+	/* revn can be zero, but if not is set at same time as info */
+	WARN_ON_ONCE(!gpu->info);
 
 	return gpu->revn == revn;
 }
@@ -161,14 +162,16 @@  static inline bool adreno_has_gmu_wrapper(const struct adreno_gpu *gpu)
 
 static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
 {
-	WARN_ON_ONCE(!gpu->revn);
+	/* revn can be zero, but if not is set at same time as info */
+	WARN_ON_ONCE(!gpu->info);
 
 	return (gpu->revn < 300);
 }
 
 static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
 {
-	WARN_ON_ONCE(!gpu->revn);
+	/* revn can be zero, but if not is set at same time as info */
+	WARN_ON_ONCE(!gpu->info);
 
 	return (gpu->revn < 210);
 }