| Message ID | 20240921204237.8006-1-john.schulz1@protonmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers/gpu: Switching Adreno x1-85 device check to family check. | expand |
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 > >
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 > >
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
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 > > > > >
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: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 > >
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)
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(-)