How about appending "by default" to the title?
On 2024-05-14 19:04: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=710000097, slow_rate=27000000
> [ 3.003869] [drm:msm_gpu_init] ebi1_clk: fffffffffffffffe
> [ 3.004002] adreno 5000000.gpu: supply vdd not found, using dummy regulator
> [ 3.008463] [drm:msm_gpu_init] gpu_reg: ffff0000819e4000
> [ 3.015105] adreno 5000000.gpu: supply vddcx not found, using dummy regulator
> [ 3.020702] [drm:msm_gpu_init] gpu_cx: ffff0000819e4180
> [ 3.028173] [drm:adreno_iommu_create_address_space]
> [ 3.054552] [drm:msm_gpu_init] gpu->aspace=ffffffffffffffed
> [ 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 5000000.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
This whole log is a tad spammy, do maintainers think that's worth having in the
commit or should it be moved below the cut (---), in favour of a more clear and
elaborate patch justification?
(Seems some logs are custom local changes, how about that?)
>
> adreno_smmu is required, so it must be enabled.
Required for what? This is not providing much if any context whatsoever, nor
justifying the change. Adreno is disabled by default, so it's fine to have the
corresponding Adreno SMMU disabled by default too.
Instead, why not copy-paste the justification that was helpfully provided to
you in IRC? At least three reasons popped up that could be used to fill up the
patch description:
- No other SoC disables adreno_smmu in DTSI;
- Nodes are typically only disabled if enabling them requires additional **board
specific** configuration (regulators, firmware paths, ...);
- Nodes are typically also disabled if they are optional and not used on every board
(wled and vibrator PMIC nodes were brought up).
>
> [ 3.220381] msm_dpu c901000.display-controller: bound 5000000.gpu (ops a3xx_ops)
> [ 3.235503] [drm:dpu_kms_hw_init:1053] dpu hardware revision:0x30000000
And this is the expected log when it works? You should annotate that.
The change itself is good to have.
> Fixes: 87cd46d68aeac8 ("Configure Adreno GPU and related IOMMU")
This isn't fixing any buggy behaviour, just lining up the DTSI to be more alike
the other SoCs. I'd drop this line.
- Marijn
> Signed-off-by: Marc Gonzalez <mgonzalez@xxxxxxxxxx>
> ---
> 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@5040000 {
> * SoC VDDMX RPM Power Domain in the Adreno driver.
> */
> power-domains = <&gpucc GPU_GX_GDSC>;
> - status = "disabled";
> };
>
> gpucc: clock-controller@5065000 {
> --
> 2.34.1
>
