Re: [PATCH v2] arm64: dts: qcom: msm8998: enable adreno_smmu by default

2024-05-15 Thread Marijn Suijten
On 2024-05-15 16:27:44, Marc Gonzalez wrote:
> 15 qcom platform DTSI files define an adreno_smmu node.
> msm8998 is the only one with adreno_smmu disabled by default.
> 
> There's no reason why this SMMU should be disabled by default,
> it doesn't need any further configuration.
> 
> Bring msm8998 in line with the 14 other platforms.
> 
> This fixes GPU init failing with ENODEV:

Nit: I'd specialize this to:

"This saves every MSM8998 board DTS from having to enable adreno_smmu when
enabling the Adreno GPU node, which leads to the following unclear probe failure
when forgotten about:"

But no need to send a v3 for that.

> msm_dpu c901000.display-controller: failed to load adreno gpu
> msm_dpu c901000.display-controller: failed to bind 500.gpu (ops 
> a3xx_ops): -19

And as a more general discussion, which is not really relevant to have your
commit message, we could have a separate patch adding an error message detailing
**where** this -19 came from.  In this case create_address_space().

> Fixes: 87cd46d68aeac8 ("Configure Adreno GPU and related IOMMU")
> Signed-off-by: Marc Gonzalez 
> Reviewed-by: Bryan O'Donoghue 

Reviewed-by: Marijn Suijten 

> ---
> New in v2: rewrote commit message with input from Martin, Bryan, Luca

Thanks!

> Supersedes: <1ba7031f-c97c-41f1-8cbc-d99f1e848...@freebox.fr>
> 
> Maintainers, feel free to drop the Fixes tag.

For context, I don't think the original patch was necessarily wrong in disabling
this node, as any node using it (adreno_gpu) is also disabled by default.  It is
however less consistent with other DTSI, which don't require every board DTS to
re-enable adreno_smmu.

- Marijn

> Failure log:
> [2.756363] [drm:adreno_bind] Found GPU: 5.4.0.1
> [2.767183] [drm:a5xx_gpu_init]
> [2.767422] [drm:adreno_gpu_init] fast_rate=71097, slow_rate=2700
> [3.003869] [drm:msm_gpu_init] ebi1_clk: fffe
> [3.004002] adreno 500.gpu: supply vdd not found, using dummy regulator
> [3.008463] [drm:msm_gpu_init] gpu_reg: 819e4000
> [3.015105] adreno 500.gpu: supply vddcx not found, using dummy 
> regulator
> [3.020702] [drm:msm_gpu_init] gpu_cx: 819e4180
> [3.028173] [drm:adreno_iommu_create_address_space]
> [3.054552] [drm:msm_gpu_init] gpu->aspace=ffed
> [3.058112] [drm:a5xx_destroy] 5.4.0.1
> [3.065922] [drm:msm_gpu_cleanup] 5.4.0.1
> [3.074237] msm_dpu c901000.display-controller: failed to load adreno gpu
> [3.082412] msm_dpu c901000.display-controller: failed to bind 500.gpu 
> (ops a3xx_ops): -19
> [3.088342] msm_dpu c901000.display-controller: [drm:drm_managed_release] 
> drmres release begin
> ...
> [3.197694] [drm:drm_managed_release] drmres release end
> [3.204009] msm_dpu c901000.display-controller: adev bind failed: -19
> ---
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi 
> b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 3d3b1f61c0690..edf379c28e1e1 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -1580,7 +1580,6 @@ adreno_smmu: iommu@504 {
>* SoC VDDMX RPM Power Domain in the Adreno driver.
>*/
>   power-domains = <&gpucc GPU_GX_GDSC>;
> - status = "disabled";
>   };
>  
>   gpucc: clock-controller@5065000 {
> -- 
> 2.34.1
> 


Re: [PATCH] arm64: dts: qcom: msm8998: enable adreno_smmu

2024-05-15 Thread Bryan O'Donoghue

On 14/05/2024 18:04, Marc Gonzalez wrote:

Right now, GPU init fails:

[2.756363] [drm:adreno_bind] Found GPU: 5.4.0.1
[2.767183] [drm:a5xx_gpu_init]
[2.767422] [drm:adreno_gpu_init] fast_rate=71097, slow_rate=2700
[3.003869] [drm:msm_gpu_init] ebi1_clk: fffe
[3.004002] adreno 500.gpu: supply vdd not found, using dummy regulator
[3.008463] [drm:msm_gpu_init] gpu_reg: 819e4000
[3.015105] adreno 500.gpu: supply vddcx not found, using dummy regulator
[3.020702] [drm:msm_gpu_init] gpu_cx: 819e4180
[3.028173] [drm:adreno_iommu_create_address_space]
[3.054552] [drm:msm_gpu_init] gpu->aspace=ffed
[3.058112] [drm:a5xx_destroy] 5.4.0.1
[3.065922] [drm:msm_gpu_cleanup] 5.4.0.1
[3.074237] msm_dpu c901000.display-controller: failed to load adreno gpu
[3.082412] msm_dpu c901000.display-controller: failed to bind 500.gpu 
(ops a3xx_ops): -19
[3.088342] msm_dpu c901000.display-controller: [drm:drm_managed_release] 
drmres release begin
...
[3.197694] [drm:drm_managed_release] drmres release end
[3.204009] msm_dpu c901000.display-controller: adev bind failed: -19



I agree with Marjin the log of the failure isn't required.

How about this as a commit log ?

"The GPU currently fails to initialise because the adreno_smmu node is 
missing. We require the adreno_smmu node to translate device virtual 
addresses properly.


Enable it now."



[3.220381] msm_dpu c901000.display-controller: bound 500.gpu (ops 
a3xx_ops)
[3.235503] [drm:dpu_kms_hw_init:1053] dpu hardware revision:0x3000


I'd drop that, we know the fix works.



Fixes: 87cd46d68aeac8 ("Configure Adreno GPU and related IOMMU")


Retain this though, its a legitimate fix the GPU won't work without this 
change.


Then add

Reviewed-by: Bryan O'Donoghue 

---
bod