diff mbox series

[v2] arm64: dts: qcom: disable GPU on x1e80100 by default

Message ID 20240715-x1e8-zap-name-v2-1-a82488e7f7c1@linaro.org (mailing list archive)
State Superseded
Headers show
Series [v2] arm64: dts: qcom: disable GPU on x1e80100 by default | expand

Commit Message

Dmitry Baryshkov July 15, 2024, 6:18 p.m. UTC 
The GPU on X1E80100 requires ZAP 'shader' file to be useful. Since the
file is signed by the OEM keys and might be not available by default,
disable the GPU node and drop the firmware name from the x1e80100.dtsi
file. Devices not being fused to use OEM keys can specify generic
location at `qcom/x1e80100/gen70500_zap.mbn` while enabling the GPU.

The CRD was lucky enough to work with the default settings, so reenable
the GPU on that platform and provide correct firmware-name (including
the SoC subdir).

Fixes: 721e38301b79 ("arm64: dts: qcom: x1e80100: Add gpu support")
Cc: Akhil P Oommen <quic_akhilpo@quicinc.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v2:
- Keep GPU enabled for X1E80100-CRD (Johan)
- Link to v1: https://lore.kernel.org/r/20240715-x1e8-zap-name-v1-1-b66df09d0b65@linaro.org
---
 arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 8 ++++++++
 arch/arm64/boot/dts/qcom/x1e80100.dtsi    | 3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)


---
base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
change-id: 20240715-x1e8-zap-name-7b3c79234401

Best regards,

Comments

Akhil P Oommen July 15, 2024, 7:01 p.m. UTC | #1 
On Mon, Jul 15, 2024 at 09:18:49PM +0300, Dmitry Baryshkov wrote:
> The GPU on X1E80100 requires ZAP 'shader' file to be useful. Since the
> file is signed by the OEM keys and might be not available by default,
> disable the GPU node and drop the firmware name from the x1e80100.dtsi
> file. Devices not being fused to use OEM keys can specify generic
> location at `qcom/x1e80100/gen70500_zap.mbn` while enabling the GPU.
> 
> The CRD was lucky enough to work with the default settings, so reenable
> the GPU on that platform and provide correct firmware-name (including
> the SoC subdir).
> 
> Fixes: 721e38301b79 ("arm64: dts: qcom: x1e80100: Add gpu support")
> Cc: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> Changes in v2:
> - Keep GPU enabled for X1E80100-CRD (Johan)
> - Link to v1: https://lore.kernel.org/r/20240715-x1e8-zap-name-v1-1-b66df09d0b65@linaro.org
> ---
>  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 8 ++++++++
>  arch/arm64/boot/dts/qcom/x1e80100.dtsi    | 3 ++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> index 6152bcd0bc1f..8f4e238f014f 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> @@ -637,6 +637,14 @@ vreg_l3j_0p8: ldo3 {
>  	};
>  };
>  
> +&gpu {
> +	satus = "okay";

s/satus/status

Also, we should add this chunk to x1e80100-qcp.dts too.

> +
> +	zap-shader {
> +		firmware-name = "qcom/x1e80100/gen70500_zap.mbn";
> +	};
> +};
> +
>  &i2c0 {
>  	clock-frequency = <400000>;
>  
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 7bca5fcd7d52..8df90d01eba8 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -3155,9 +3155,10 @@ gpu: gpu@3d00000 {
>  			interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>;
>  			interconnect-names = "gfx-mem";
>  
> +			status = "disabled";
> +
>  			zap-shader {
>  				memory-region = <&gpu_microcode_mem>;
> -				firmware-name = "qcom/gen70500_zap.mbn";

In general, why not keep a default zap firmware listed here? Anyway we
are disabling gpu node here in case of platforms which doesn't upstream
secure firmwares.

-Akhil

>  			};
>  
>  			gpu_opp_table: opp-table {
> 
> ---
> base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
> change-id: 20240715-x1e8-zap-name-7b3c79234401
> 
> Best regards,
> -- 
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
Dmitry Baryshkov July 16, 2024, 11:03 a.m. UTC | #2 
On Mon, 15 Jul 2024 at 22:01, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On Mon, Jul 15, 2024 at 09:18:49PM +0300, Dmitry Baryshkov wrote:
> > The GPU on X1E80100 requires ZAP 'shader' file to be useful. Since the
> > file is signed by the OEM keys and might be not available by default,
> > disable the GPU node and drop the firmware name from the x1e80100.dtsi
> > file. Devices not being fused to use OEM keys can specify generic
> > location at `qcom/x1e80100/gen70500_zap.mbn` while enabling the GPU.
> >
> > The CRD was lucky enough to work with the default settings, so reenable
> > the GPU on that platform and provide correct firmware-name (including
> > the SoC subdir).
> >
> > Fixes: 721e38301b79 ("arm64: dts: qcom: x1e80100: Add gpu support")
> > Cc: Akhil P Oommen <quic_akhilpo@quicinc.com>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > Changes in v2:
> > - Keep GPU enabled for X1E80100-CRD (Johan)
> > - Link to v1: https://lore.kernel.org/r/20240715-x1e8-zap-name-v1-1-b66df09d0b65@linaro.org
> > ---
> >  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 8 ++++++++
> >  arch/arm64/boot/dts/qcom/x1e80100.dtsi    | 3 ++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >

[..]

> > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > index 7bca5fcd7d52..8df90d01eba8 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > @@ -3155,9 +3155,10 @@ gpu: gpu@3d00000 {
> >                       interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>;
> >                       interconnect-names = "gfx-mem";
> >
> > +                     status = "disabled";
> > +
> >                       zap-shader {
> >                               memory-region = <&gpu_microcode_mem>;
> > -                             firmware-name = "qcom/gen70500_zap.mbn";
>
> In general, why not keep a default zap firmware listed here? Anyway we
> are disabling gpu node here in case of platforms which doesn't upstream
> secure firmwares.

Excuse me, I missed the question before sending v3, however the answer
is still going to be the same:

First of all, we don't do it for other platforms
Second, we don't do it for other firmware. Each DT declares its own
set of files.
Last, but not least, it's better to get an error message regarding
firmware-name not being present rather than a possibly cryptic message
regarding firmware failing authentication.

>
> -Akhil
>
> >                       };
> >
> >                       gpu_opp_table: opp-table {
> >
> > ---
> > base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
> > change-id: 20240715-x1e8-zap-name-7b3c79234401
> >
> > Best regards,
> > --
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >



--
With best wishes
Dmitry
Rob Clark July 16, 2024, 2:49 p.m. UTC | #3 
On Tue, Jul 16, 2024 at 4:03 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 15 Jul 2024 at 22:01, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >
> > On Mon, Jul 15, 2024 at 09:18:49PM +0300, Dmitry Baryshkov wrote:
> > > The GPU on X1E80100 requires ZAP 'shader' file to be useful. Since the
> > > file is signed by the OEM keys and might be not available by default,
> > > disable the GPU node and drop the firmware name from the x1e80100.dtsi
> > > file. Devices not being fused to use OEM keys can specify generic
> > > location at `qcom/x1e80100/gen70500_zap.mbn` while enabling the GPU.
> > >
> > > The CRD was lucky enough to work with the default settings, so reenable
> > > the GPU on that platform and provide correct firmware-name (including
> > > the SoC subdir).
> > >
> > > Fixes: 721e38301b79 ("arm64: dts: qcom: x1e80100: Add gpu support")
> > > Cc: Akhil P Oommen <quic_akhilpo@quicinc.com>
> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > Changes in v2:
> > > - Keep GPU enabled for X1E80100-CRD (Johan)
> > > - Link to v1: https://lore.kernel.org/r/20240715-x1e8-zap-name-v1-1-b66df09d0b65@linaro.org
> > > ---
> > >  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 8 ++++++++
> > >  arch/arm64/boot/dts/qcom/x1e80100.dtsi    | 3 ++-
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >
>
> [..]
>
> > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > > index 7bca5fcd7d52..8df90d01eba8 100644
> > > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > > @@ -3155,9 +3155,10 @@ gpu: gpu@3d00000 {
> > >                       interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>;
> > >                       interconnect-names = "gfx-mem";
> > >
> > > +                     status = "disabled";
> > > +
> > >                       zap-shader {
> > >                               memory-region = <&gpu_microcode_mem>;
> > > -                             firmware-name = "qcom/gen70500_zap.mbn";
> >
> > In general, why not keep a default zap firmware listed here? Anyway we
> > are disabling gpu node here in case of platforms which doesn't upstream
> > secure firmwares.
>
> Excuse me, I missed the question before sending v3, however the answer
> is still going to be the same:
>
> First of all, we don't do it for other platforms
> Second, we don't do it for other firmware. Each DT declares its own
> set of files.
> Last, but not least, it's better to get an error message regarding
> firmware-name not being present rather than a possibly cryptic message
> regarding firmware failing authentication.

tbh, I think it might be better to just remove the default fw name
from a6xx_catalog.c device table.  That would better reflect the
situation, ie. some fw is needed and not available (if the
firmware-name prop isn't provided).  Trying to fall back to loading
the wrong fw is a mis-design.

BR,
-R

> >
> > -Akhil
> >
> > >                       };
> > >
> > >                       gpu_opp_table: opp-table {
> > >
> > > ---
> > > base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
> > > change-id: 20240715-x1e8-zap-name-7b3c79234401
> > >
> > > Best regards,
> > > --
> > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
>
>
>
> --
> With best wishes
> Dmitry
>
Akhil P Oommen July 16, 2024, 9:40 p.m. UTC | #4 
On Tue, Jul 16, 2024 at 07:49:07AM -0700, Rob Clark wrote:
> On Tue, Jul 16, 2024 at 4:03 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Mon, 15 Jul 2024 at 22:01, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> > >
> > > On Mon, Jul 15, 2024 at 09:18:49PM +0300, Dmitry Baryshkov wrote:
> > > > The GPU on X1E80100 requires ZAP 'shader' file to be useful. Since the
> > > > file is signed by the OEM keys and might be not available by default,
> > > > disable the GPU node and drop the firmware name from the x1e80100.dtsi
> > > > file. Devices not being fused to use OEM keys can specify generic
> > > > location at `qcom/x1e80100/gen70500_zap.mbn` while enabling the GPU.
> > > >
> > > > The CRD was lucky enough to work with the default settings, so reenable
> > > > the GPU on that platform and provide correct firmware-name (including
> > > > the SoC subdir).
> > > >
> > > > Fixes: 721e38301b79 ("arm64: dts: qcom: x1e80100: Add gpu support")
> > > > Cc: Akhil P Oommen <quic_akhilpo@quicinc.com>
> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > Changes in v2:
> > > > - Keep GPU enabled for X1E80100-CRD (Johan)
> > > > - Link to v1: https://lore.kernel.org/r/20240715-x1e8-zap-name-v1-1-b66df09d0b65@linaro.org
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 8 ++++++++
> > > >  arch/arm64/boot/dts/qcom/x1e80100.dtsi    | 3 ++-
> > > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > > >
> >
> > [..]
> >
> > > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > > > index 7bca5fcd7d52..8df90d01eba8 100644
> > > > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > > > @@ -3155,9 +3155,10 @@ gpu: gpu@3d00000 {
> > > >                       interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>;
> > > >                       interconnect-names = "gfx-mem";
> > > >
> > > > +                     status = "disabled";
> > > > +
> > > >                       zap-shader {
> > > >                               memory-region = <&gpu_microcode_mem>;
> > > > -                             firmware-name = "qcom/gen70500_zap.mbn";
> > >
> > > In general, why not keep a default zap firmware listed here? Anyway we
> > > are disabling gpu node here in case of platforms which doesn't upstream
> > > secure firmwares.
> >
> > Excuse me, I missed the question before sending v3, however the answer
> > is still going to be the same:
> >
> > First of all, we don't do it for other platforms
> > Second, we don't do it for other firmware. Each DT declares its own
> > set of files.
> > Last, but not least, it's better to get an error message regarding
> > firmware-name not being present rather than a possibly cryptic message
> > regarding firmware failing authentication.
> 
> tbh, I think it might be better to just remove the default fw name
> from a6xx_catalog.c device table.  That would better reflect the
> situation, ie. some fw is needed and not available (if the
> firmware-name prop isn't provided).  Trying to fall back to loading
> the wrong fw is a mis-design.

Agree. Thanks for the clarification.

-Akhil

> 
> BR,
> -R
> 
> > >
> > > -Akhil
> > >
> > > >                       };
> > > >
> > > >                       gpu_opp_table: opp-table {
> > > >
> > > > ---
> > > > base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
> > > > change-id: 20240715-x1e8-zap-name-7b3c79234401
> > > >
> > > > Best regards,
> > > > --
> > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > >
> >
> >
> >
> > --
> > With best wishes
> > Dmitry
> >
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
index 6152bcd0bc1f..8f4e238f014f 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
@@ -637,6 +637,14 @@  vreg_l3j_0p8: ldo3 {
 	};
 };
 
+&gpu {
+	satus = "okay";
+
+	zap-shader {
+		firmware-name = "qcom/x1e80100/gen70500_zap.mbn";
+	};
+};
+
 &i2c0 {
 	clock-frequency = <400000>;
 
diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 7bca5fcd7d52..8df90d01eba8 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -3155,9 +3155,10 @@  gpu: gpu@3d00000 {
 			interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>;
 			interconnect-names = "gfx-mem";
 
+			status = "disabled";
+
 			zap-shader {
 				memory-region = <&gpu_microcode_mem>;
-				firmware-name = "qcom/gen70500_zap.mbn";
 			};
 
 			gpu_opp_table: opp-table {