[PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[John Schulz]

Switches the is_x185 check to is_x1xx_family to accommodate more devices.
Note that I got the X1-45 GPU ID from Windows which may not be correct.

Signed-off-by: John Schulz <john.schulz1@protonmail.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  3 ++-
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 06cab2c6fd66..f04aeacae3c2 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2017-2019 The Linux Foundation. All rights reserved. */
 
 
+#include "adreno_gpu.h"
 #include "msm_gem.h"
 #include "msm_mmu.h"
 #include "msm_gpu_trace.h"
@@ -1026,7 +1027,7 @@ static int hw_init(struct msm_gpu *gpu)
 	gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, BIT(7) | 0x1);
 
 	/* Set weights for bicubic filtering */
-	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x185(adreno_gpu)) {
+	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x1xx_family(adreno_gpu)) {
 		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_0, 0);
 		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_1,
 			0x3fe05ff4);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 58d7e7915c57..ec36fc915433 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -526,9 +526,15 @@ static inline int adreno_is_a750(struct adreno_gpu *gpu)
 	return gpu->info->chip_ids[0] == 0x43051401;
 }
 
-static inline int adreno_is_x185(struct adreno_gpu *gpu)
-{
-	return gpu->info->chip_ids[0] == 0x43050c01;
+static inline int adreno_is_x1xx_family(struct adreno_gpu *gpu)
+{
+	switch (gpu->info->chip_ids[0]) {
+	case 0x1fc31043; // X1-45
+	case 0x43050c01; // X1-85
+		return 1;
+	default:
+		return 0;
+	}
 }
 
 static inline int adreno_is_a740_family(struct adreno_gpu *gpu)
-- 
2.46.1

Re: [PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[Dmitry Baryshkov]

On Sat, Sep 21, 2024 at 08:42:54PM GMT, John Schulz wrote:

Subject prefix is not correct, it should be drm/msm

> Switches the is_x185 check to is_x1xx_family to accommodate more devices.
> Note that I got the X1-45 GPU ID from Windows which may not be correct.

How did you test it? It's not that the driver is going to work on that
GPU without a catalog entry.

> 
> Signed-off-by: John Schulz <john.schulz1@protonmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  3 ++-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 +++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 06cab2c6fd66..f04aeacae3c2 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2017-2019 The Linux Foundation. All rights reserved. */
>  
>  
> +#include "adreno_gpu.h"
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  #include "msm_gpu_trace.h"
> @@ -1026,7 +1027,7 @@ static int hw_init(struct msm_gpu *gpu)
>  	gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, BIT(7) | 0x1);
>  
>  	/* Set weights for bicubic filtering */
> -	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x185(adreno_gpu)) {
> +	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x1xx_family(adreno_gpu)) {
>  		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_0, 0);
>  		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_1,
>  			0x3fe05ff4);
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 58d7e7915c57..ec36fc915433 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -526,9 +526,15 @@ static inline int adreno_is_a750(struct adreno_gpu *gpu)
>  	return gpu->info->chip_ids[0] == 0x43051401;
>  }
>  
> -static inline int adreno_is_x185(struct adreno_gpu *gpu)
> -{
> -	return gpu->info->chip_ids[0] == 0x43050c01;
> +static inline int adreno_is_x1xx_family(struct adreno_gpu *gpu)
> +{
> +	switch (gpu->info->chip_ids[0]) {
> +	case 0x1fc31043; // X1-45

Just by comparing it with other IDs it looks like you got it backwards.

> +	case 0x43050c01; // X1-85
> +		return 1;
> +	default:
> +		return 0;
> +	}
>  }
>  
>  static inline int adreno_is_a740_family(struct adreno_gpu *gpu)
> -- 
> 2.46.1
> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[Bjorn Andersson]

On Sat, Sep 21, 2024 at 08:42:54PM +0000, John Schulz wrote:
> Switches the is_x185 check to is_x1xx_family to accommodate more devices.
> Note that I got the X1-45 GPU ID from Windows which may not be correct.
> 

I assume from your patch that you have a X1-45 that you want to support
and you think there will be more of these and therefor you think it's
better to move to some form of family check.

It would be preferred if you clearly state the problem you're trying to
solve, to avoid current and future reviewers of the code from having to
assume/deduce the reasoning behind a patch.

E.g. why do you prefer adding is_family() instead of just adding
is_x145()?

Please also read:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Regards,
Bjorn

> Signed-off-by: John Schulz <john.schulz1@protonmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  3 ++-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 +++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 06cab2c6fd66..f04aeacae3c2 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2017-2019 The Linux Foundation. All rights reserved. */
>  
>  
> +#include "adreno_gpu.h"
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  #include "msm_gpu_trace.h"
> @@ -1026,7 +1027,7 @@ static int hw_init(struct msm_gpu *gpu)
>  	gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, BIT(7) | 0x1);
>  
>  	/* Set weights for bicubic filtering */
> -	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x185(adreno_gpu)) {
> +	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x1xx_family(adreno_gpu)) {
>  		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_0, 0);
>  		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_1,
>  			0x3fe05ff4);
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 58d7e7915c57..ec36fc915433 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -526,9 +526,15 @@ static inline int adreno_is_a750(struct adreno_gpu *gpu)
>  	return gpu->info->chip_ids[0] == 0x43051401;
>  }
>  
> -static inline int adreno_is_x185(struct adreno_gpu *gpu)
> -{
> -	return gpu->info->chip_ids[0] == 0x43050c01;
> +static inline int adreno_is_x1xx_family(struct adreno_gpu *gpu)
> +{
> +	switch (gpu->info->chip_ids[0]) {
> +	case 0x1fc31043; // X1-45
> +	case 0x43050c01; // X1-85
> +		return 1;
> +	default:
> +		return 0;
> +	}
>  }
>  
>  static inline int adreno_is_a740_family(struct adreno_gpu *gpu)
> -- 
> 2.46.1
> 
> 

(No Subject)

[John Schulz]

Hi Dmitry, 

I do not have a way of testing this patch. I do see your point in that it may be redundant/unnecessary
since basic/generic drivers should at the very least output a shell interface. 

Upon doing more research, I came across the fact that some X1 GPUs have OEM signing that prevent the 
driver from working. I suspect that is what I am running into when I try testing various distros. All
of them exhibit the same behavior of the display halting during the bootloader handoff and the HDMI
port does not output anything. Even the Debian 12 image from Linaro exhibits the same issue. 

I find this a bit odd given that there is a dts entry for the Asus Vivobook S 15 X1E varient. I 
don't see any comments on whether the dts for that laptop was tested. The Vivobook I have is the
X1P varient which, to my knowledge, is identical to the X1E one but a different SoC. 

Perhaps it would be of better use of my (and others) time figuring out if the GPU drivers not
working is due to OEM locking and if so, trying to unlock it. What do y'all think? 

P.S. Apologies for the incorrect prefix, should have done more research instead of trying to 
make an educated guess. 

Thanks,
John

[PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[John Schulz]

Switches the is_x185 check to is_x1xx_family to accommodate more devices.
Note that I got the X1-45 GPU ID from Windows which may not be correct.

Signed-off-by: John Schulz <john.schulz1@protonmail.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  3 ++-
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 06cab2c6fd66..f04aeacae3c2 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2017-2019 The Linux Foundation. All rights reserved. */
 
 
+#include "adreno_gpu.h"
 #include "msm_gem.h"
 #include "msm_mmu.h"
 #include "msm_gpu_trace.h"
@@ -1026,7 +1027,7 @@ static int hw_init(struct msm_gpu *gpu)
 	gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, BIT(7) | 0x1);
 
 	/* Set weights for bicubic filtering */
-	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x185(adreno_gpu)) {
+	if (adreno_is_a650_family(adreno_gpu) || adreno_is_x1xx_family(adreno_gpu)) {
 		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_0, 0);
 		gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_1,
 			0x3fe05ff4);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 58d7e7915c57..ec36fc915433 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -526,9 +526,15 @@ static inline int adreno_is_a750(struct adreno_gpu *gpu)
 	return gpu->info->chip_ids[0] == 0x43051401;
 }
 
-static inline int adreno_is_x185(struct adreno_gpu *gpu)
-{
-	return gpu->info->chip_ids[0] == 0x43050c01;
+static inline int adreno_is_x1xx_family(struct adreno_gpu *gpu)
+{
+	switch (gpu->info->chip_ids[0]) {
+	case 0x1fc31043; // X1-45
+	case 0x43050c01; // X1-85
+		return 1;
+	default:
+		return 0;
+	}
 }
 
 static inline int adreno_is_a740_family(struct adreno_gpu *gpu)
-- 
2.46.1

Re: [PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[Rob Clark]

On Tue, Sep 24, 2024 at 6:54 AM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> On Sat, Sep 21, 2024 at 08:42:54PM +0000, John Schulz wrote:
> > Switches the is_x185 check to is_x1xx_family to accommodate more devices.
> > Note that I got the X1-45 GPU ID from Windows which may not be correct.
> >
>
> I assume from your patch that you have a X1-45 that you want to support
> and you think there will be more of these and therefor you think it's
> better to move to some form of family check.
>
> It would be preferred if you clearly state the problem you're trying to
> solve, to avoid current and future reviewers of the code from having to
> assume/deduce the reasoning behind a patch.
>
> E.g. why do you prefer adding is_family() instead of just adding
> is_x145()?

Note, I'll hold off on merging something like this until we actually
have x1-45 support.  I'd _guess_ x1-45 is probably pretty similar to
x1-85, but it is premature to speculate until someone shows up with
x1-45 patches.

BR,
-R

> Please also read:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
> Regards,
> Bjorn
>
> > Signed-off-by: John Schulz <john.schulz1@protonmail.com>
> > ---
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  3 ++-
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 +++++++++---
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index 06cab2c6fd66..f04aeacae3c2 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -2,6 +2,7 @@
> >  /* Copyright (c) 2017-2019 The Linux Foundation. All rights reserved. */
> >
> >
> > +#include "adreno_gpu.h"
> >  #include "msm_gem.h"
> >  #include "msm_mmu.h"
> >  #include "msm_gpu_trace.h"
> > @@ -1026,7 +1027,7 @@ static int hw_init(struct msm_gpu *gpu)
> >       gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, BIT(7) | 0x1);
> >
> >       /* Set weights for bicubic filtering */
> > -     if (adreno_is_a650_family(adreno_gpu) || adreno_is_x185(adreno_gpu)) {
> > +     if (adreno_is_a650_family(adreno_gpu) || adreno_is_x1xx_family(adreno_gpu)) {
> >               gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_0, 0);
> >               gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_1,
> >                       0x3fe05ff4);
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 58d7e7915c57..ec36fc915433 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -526,9 +526,15 @@ static inline int adreno_is_a750(struct adreno_gpu *gpu)
> >       return gpu->info->chip_ids[0] == 0x43051401;
> >  }
> >
> > -static inline int adreno_is_x185(struct adreno_gpu *gpu)
> > -{
> > -     return gpu->info->chip_ids[0] == 0x43050c01;
> > +static inline int adreno_is_x1xx_family(struct adreno_gpu *gpu)
> > +{
> > +     switch (gpu->info->chip_ids[0]) {
> > +     case 0x1fc31043; // X1-45
> > +     case 0x43050c01; // X1-85
> > +             return 1;
> > +     default:
> > +             return 0;
> > +     }
> >  }
> >
> >  static inline int adreno_is_a740_family(struct adreno_gpu *gpu)
> > --
> > 2.46.1
> >
> >
>

Re: (No Subject)

[Rob Clark]

On Tue, Sep 24, 2024 at 8:55 AM John Schulz <john.schulz1@protonmail.com> wrote:
>
> Hi Dmitry,
>
> I do not have a way of testing this patch. I do see your point in that it may be redundant/unnecessary
> since basic/generic drivers should at the very least output a shell interface.
>
> Upon doing more research, I came across the fact that some X1 GPUs have OEM signing that prevent the
> driver from working. I suspect that is what I am running into when I try testing various distros. All
> of them exhibit the same behavior of the display halting during the bootloader handoff and the HDMI
> port does not output anything. Even the Debian 12 image from Linaro exhibits the same issue.
>
> I find this a bit odd given that there is a dts entry for the Asus Vivobook S 15 X1E varient. I
> don't see any comments on whether the dts for that laptop was tested. The Vivobook I have is the
> X1P varient which, to my knowledge, is identical to the X1E one but a different SoC.

I think we'd need an x1p variant of x1e80100.dtsi which has the
description of the SoC itself.. which I guess is similar, but not the
same as, x1e.  Maybe someone from qcom or linaro is already working on
this?

> Perhaps it would be of better use of my (and others) time figuring out if the GPU drivers not
> working is due to OEM locking and if so, trying to unlock it. What do y'all think?

It is probably not OEM locking that is the issue, but OEM signing of
zap shader.  If you look at the x1e laptops, as an example, the GPU
node has the device specific firmware-path for the zap shader, ie. for
the yoga 7x, it is:

&gpu {
        status = "okay";

        zap-shader {
                firmware-name = "qcom/x1e80100/LENOVO/83ED/qcdxkmsuc8380.mbn";
        };
};

That qcdxkmsuc8380.mbn file would need to be copied from the windows
partition currently.

BR,
-R

> P.S. Apologies for the incorrect prefix, should have done more research instead of trying to
> make an educated guess.
>
> Thanks,
> John
>
>
>

Re: [PATCH] drivers/gpu: Switching Adreno x1-85 device check to family check.

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 03:55:38PM GMT, John Schulz wrote:
> Switches the is_x185 check to is_x1xx_family to accommodate more devices.
> Note that I got the X1-45 GPU ID from Windows which may not be correct.
> 
> Signed-off-by: John Schulz <john.schulz1@protonmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  3 ++-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 +++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)

I don't see a point in reposting the patch with the same set of issues.

-- 
With best wishes
Dmitry

Re: (No Subject)

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 03:54:59PM GMT, John Schulz wrote:
> Hi Dmitry, 
> 
> I do not have a way of testing this patch. I do see your point in that it may be redundant/unnecessary
> since basic/generic drivers should at the very least output a shell interface. 
> 
> Upon doing more research, I came across the fact that some X1 GPUs have OEM signing that prevent the 
> driver from working. I suspect that is what I am running into when I try testing various distros. All
> of them exhibit the same behavior of the display halting during the bootloader handoff and the HDMI
> port does not output anything. Even the Debian 12 image from Linaro exhibits the same issue. 

Ok. So you have a device with X1P, which you don't seem to be able to
get to work? Could you please try doing the following:

- you might need to modify the GPU DT node to use a different ID there.
  If your guess is right, you might need to specify qcom,adreno-4310c31f
  (this is from your patch)

- you need to define a corresponding entry in a7xx_gpus[]. Try using
  X1-85 as an inspiration.

- only with that in place the x1xx family makes sense.


Also it would be nice if you describe your issue. What exactly do you
observe and what doesn't seem to work?

> 
> I find this a bit odd given that there is a dts entry for the Asus Vivobook S 15 X1E varient. I 
> don't see any comments on whether the dts for that laptop was tested. The Vivobook I have is the
> X1P varient which, to my knowledge, is identical to the X1E one but a different SoC. 
> 
> Perhaps it would be of better use of my (and others) time figuring out if the GPU drivers not
> working is due to OEM locking and if so, trying to unlock it. What do y'all think? 
> 
> P.S. Apologies for the incorrect prefix, should have done more research instead of trying to 
> make an educated guess. 
> 
> Thanks,
> John
> 
> 

-- 
With best wishes
Dmitry