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

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

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

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

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.