mbox series

[v3,00/15] GMU-less A6xx support (A610, A619_holi)

Message ID 20230223-topic-gmuwrapper-v3-0-5be55a336819@linaro.org
Headers show
Series GMU-less A6xx support (A610, A619_holi) | expand

Message

Konrad Dybcio Feb. 23, 2023, 12:06 p.m. UTC
v2 -> v3:
New dependencies:
- https://lore.kernel.org/linux-arm-msm/20230223-topic-opp-v3-0-5f22163cd1df@linaro.org/T/#t
- https://lore.kernel.org/linux-arm-msm/20230120172233.1905761-1-konrad.dybcio@linaro.org/

Sidenote: A speedbin rework is in progress, the of_machine_is_compatible
calls in A619_holi are ugly (but well, necessary..) but they'll be
replaced with socid matching in this or the next kernel cycle.

Due to the new way of identifying GMU wrapper GPUs, configuring 6350
to use wrapper would cause the wrong fuse values to be checked, but that
will be solved by the conversion + the ultimate goal is to use the GMU
whenever possible with the wrapper left for GMU-less Adrenos and early
bringup debugging of GMU-equipped ones.

- Ship dt-bindings in this series as we're referencing the compatible now

- "De-staticize" -> "remove static keyword" [3/15]

- Track down all the values in [4/15]

- Add many comments and explanations in [4/15]

- Fix possible return-before-mutex-unlock [5/15]

- Explain the GMU wrapper a bit more in the commit msg [5/15]

- Separate out pm_resume/suspend for GMU-wrapper GPUs to make things
  cleaner [5/15]

- Don't check if `info` exists, it has to at this point [5/15]

- Assign gpu->info early and clean up following if statements in
  a6xx_gpu_init [5/15]

- Determine whether we use GMU wrapper based on the GMU compatible
  instead of a quirk [5/15]

- Use a struct field to annotate whether we're using gmu wrapper so
  that it can be assigned at runtime (turns out a619 holi-ness cannot
  be determined by patchid + that will make it easier to test out GMU
  GPUs without actually turning on the GMU if anybody wants to do so)
  [5/15]

- Unconditionally hook up gx to the gmu wrapper (otherwise our gpu
  will not get power) [5/15]

- Don't check for gx domain presence in gmu_wrapper paths, it's
  guaranteed [5/15]

- Use opp set rate in the gmuwrapper suspend path [5/15]

- Call opp functions on the GPU device and not on the DRM device of
  mdp4/5/DPU1 half the time (WHOOOOPS!) [5/15]

- Disable the memory clock in a6xx_pm_suspend instead of enabling it
  (moderate oops) [5/15]

- Call the forgotten clk_bulk_disable_unprepare in a6xx_pm_suspend [5/15]

- Set rate to FMIN (a6xx really doesn't like rate=0 + that's what
  msm-5.x does anyway) before disabling core clock [5/15]

- pm_runtime_get_sync -> pm_runtime_resume_and_get [5/15]

- Don't annotate no cached BO support with a quirk, as A619_holi is
  merged into the A619 entry in the big const struct - this means
  that all GPUs operating in gmu wrapper configuration will be
  implicitly treated as if they didn't have this feature [7/15]

- Drop OPP rate & icc related patches, they're a part of a separate
  series now; rebase on it

- Clean up extra parentheses [8/15]

- Identify A619_holi by checking the compatible of its GMU instead
  of patchlevel [8/15]

- Drop "Fix up A6XX protected registers" - unnecessary, Rob will add
  a comment explaining why

- Fix existing UBWC values for A680, new patch [10/15]

- Use adreno_is_aXYZ macros in speedbin matching [13/15] - new patch

v2: https://lore.kernel.org/linux-arm-msm/20230214173145.2482651-1-konrad.dybcio@linaro.org/

v1 -> v2:
- Fix A630 values in [2/14]
- Fix [6/14] for GMU-equipped GPUs

Link to v1: https://lore.kernel.org/linux-arm-msm/20230126151618.225127-1-konrad.dybcio@linaro.org/

This series concludes my couple-weeks-long suffering of figuring out
the ins and outs of the "non-standard" A6xx GPUs which feature no GMU.

The GMU functionality is essentially emulated by parting out a
"GMU wrapper" region, which is essentially just a register space
within the GPU. It's modeled to be as similar to the actual GMU
as possible while staying as unnecessary as we can make it - there's
no IRQs, communicating with a microcontroller, no RPMh communication
etc. etc. I tried to reuse as much code as possible without making
a mess where every even line is used for GMU and every odd line is
used for GMU wrapper..

This series contains:
- plumbing for non-GMU operation, if-ing out GMU calls based on
  GMU presence
- GMU wrapper support
- A610 support (w/ speedbin)
- A619 support (w/ speedbin)
- couple of minor fixes and improvements
- VDDCX/VDDGX scaling fix for non-GMU GPUs (concerns more than just
  A6xx)
- Enablement of opp interconnect properties

A619_holi works perfectly fine using the already-present A619 support
in mesa. A610 needs more work on that front, but can already replay
command traces captures on downstream.

NOTE: the "drm/msm/a6xx: Add support for A619_holi" patch contains
two occurences of 0x18 used in place of a register #define, as it's
supposed to be RBBM_GPR0_CNTL, but that will only be present after
mesa-side changes are merged and headers are synced from there.

Speedbin patches depend on:
https://lore.kernel.org/linux-arm-msm/20230120172233.1905761-1-konrad.dybcio@linaro.org/

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (15):
      dt-bindings: display/msm: gpu: Document GMU wrapper-equipped A6xx
      dt-bindings: display/msm/gmu: Add GMU wrapper
      drm/msm/a6xx: Remove static keyword from sptprac en/disable functions
      drm/msm/a6xx: Extend and explain UBWC config
      drm/msm/a6xx: Introduce GMU wrapper support
      drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init
      drm/msm/adreno: Disable has_cached_coherent in GMU wrapper configurations
      drm/msm/a6xx: Add support for A619_holi
      drm/msm/a6xx: Add A610 support
      drm/msm/a6xx: Fix A680 highest bank bit value
      drm/msm/a6xx: Fix some A619 tunables
      drm/msm/a6xx: Use "else if" in GPU speedbin rev matching
      drm/msm/a6xx: Use adreno_is_aXYZ macros in speedbin matching
      drm/msm/a6xx: Add A619_holi speedbin support
      drm/msm/a6xx: Add A610 speedbin support

 .../devicetree/bindings/display/msm/gmu.yaml       |  49 +-
 .../devicetree/bindings/display/msm/gpu.yaml       |  63 ++-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c              |  57 ++-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h              |   2 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c              | 513 ++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h              |   1 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c        |  14 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c         |  17 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.h            |  33 +-
 9 files changed, 655 insertions(+), 94 deletions(-)
---
base-commit: f122501715b5bb8ea340e077401257795b6638a1
change-id: 20230223-topic-gmuwrapper-b4fff5fd7789

Best regards,

Comments

Krzysztof Kozlowski Feb. 24, 2023, 11:17 a.m. UTC | #1
On 23/02/2023 13:06, Konrad Dybcio wrote:
> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
> specified under the GPU node, just like their older cousins.
> Account for that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/display/msm/gpu.yaml       | 63 ++++++++++++++++++----
>  1 file changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.yaml b/Documentation/devicetree/bindings/display/msm/gpu.yaml
> index d4191cca71fb..e6d3160601bc 100644
> --- a/Documentation/devicetree/bindings/display/msm/gpu.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/gpu.yaml
> @@ -36,10 +36,7 @@ properties:
>  
>    reg-names:
>      minItems: 1
> -    items:
> -      - const: kgsl_3d0_reg_memory
> -      - const: cx_mem
> -      - const: cx_dbgc
> +    maxItems: 3
>  
>    interrupts:
>      maxItems: 1
> @@ -147,26 +144,72 @@ allOf:
>                  description: GPU Alternative Memory Interface clock
>                - const: gfx3d
>                  description: GPU 3D engine clock
> +              - const: gmu
> +                description: CX GMU clock
>                - const: rbbmtimer
>                  description: GPU RBBM Timer for Adreno 5xx series
>                - const: rbcpr
>                  description: GPU RB Core Power Reduction clock
> +              - const: xo
> +                description: GPUCC clocksource clock
>            minItems: 2
> -          maxItems: 7
> +          maxItems: 9

Your commit says A6xx but this is a3-5xx. I don't understand this change.

>  
>        required:
>          - clocks
>          - clock-names
> +
>    - if:
>        properties:
>          compatible:
>            contains:
> -            pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
> -
> -    then: # Since Adreno 6xx series clocks should be defined in GMU
> +            enum:
> +              - qcom,adreno-610.0
> +              - qcom,adreno-619.1
> +    then:
>        properties:
> -        clocks: false
> -        clock-names: false
> +        clock-names:
> +          items:
> +            - const: core
> +              description: GPU Core clock
> +            - const: iface
> +              description: GPU Interface clock
> +            - const: mem_iface
> +              description: GPU Memory Interface clock
> +            - const: alt_mem_iface
> +              description: GPU Alternative Memory Interface clock
> +            - const: gmu
> +              description: CX GMU clock
> +            - const: xo
> +              description: GPUCC clocksource clock
> +
> +        reg-names:
> +          minItems: 1
> +          items:
> +            - const: kgsl_3d0_reg_memory
> +            - const: cx_dbgc
> +
> +      required:
> +        - clocks
> +        - clock-names
> +    else:
> +      if:
> +        properties:
> +          compatible:
> +            contains:
> +              pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
> +
> +      then: # Starting with A6xx, the clocks are usually defined in the GMU node

The comment is not accurate anymore.


Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 24, 2023, 11:19 a.m. UTC | #2
On 23/02/2023 13:06, Konrad Dybcio wrote:
> GMU wrapper is essentially a register space within the GPU, which
> Linux sees as a dumbed-down regular GMU: there's no clocks,
> interrupts, multiple regs, iommus and OPP. Document it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/display/msm/gmu.yaml       | 49 ++++++++++++++++------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/gmu.yaml b/Documentation/devicetree/bindings/display/msm/gmu.yaml
> index ab14e81cb050..021373e686e1 100644
> --- a/Documentation/devicetree/bindings/display/msm/gmu.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/gmu.yaml
> @@ -19,16 +19,18 @@ description: |
>  
>  properties:
>    compatible:
> -    items:
> -      - pattern: '^qcom,adreno-gmu-6[0-9][0-9]\.[0-9]$'
> -      - const: qcom,adreno-gmu
> +    oneOf:
> +      - items:
> +          - pattern: '^qcom,adreno-gmu-6[0-9][0-9]\.[0-9]$'
> +          - const: qcom,adreno-gmu
> +      - const: qcom,adreno-gmu-wrapper

Why wrapper is part of this binding then? Usually wrapper means there is
wrapper node with a GMU child (at least this is what we call for all
wrappers of custom IP blocks like USB DWC). Where is the child?


Best regards,
Krzysztof
Konrad Dybcio Feb. 24, 2023, 11:50 a.m. UTC | #3
On 24.02.2023 12:19, Krzysztof Kozlowski wrote:
> On 23/02/2023 13:06, Konrad Dybcio wrote:
>> GMU wrapper is essentially a register space within the GPU, which
>> Linux sees as a dumbed-down regular GMU: there's no clocks,
>> interrupts, multiple regs, iommus and OPP. Document it.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  .../devicetree/bindings/display/msm/gmu.yaml       | 49 ++++++++++++++++------
>>  1 file changed, 37 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/gmu.yaml b/Documentation/devicetree/bindings/display/msm/gmu.yaml
>> index ab14e81cb050..021373e686e1 100644
>> --- a/Documentation/devicetree/bindings/display/msm/gmu.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/gmu.yaml
>> @@ -19,16 +19,18 @@ description: |
>>  
>>  properties:
>>    compatible:
>> -    items:
>> -      - pattern: '^qcom,adreno-gmu-6[0-9][0-9]\.[0-9]$'
>> -      - const: qcom,adreno-gmu
>> +    oneOf:
>> +      - items:
>> +          - pattern: '^qcom,adreno-gmu-6[0-9][0-9]\.[0-9]$'
>> +          - const: qcom,adreno-gmu
>> +      - const: qcom,adreno-gmu-wrapper
> 
> Why wrapper is part of this binding then? Usually wrapper means there is
> wrapper node with a GMU child (at least this is what we call for all
> wrappers of custom IP blocks like USB DWC). Where is the child?
"GMU wrapper" is a sorta confusing name that Qualcomm chose for
the "fake GMU" which has the GMU_CX and GMU_GX registers responsible
for things like powering up some GPU things internally and some
perf/pwr counters. It is _not_ a wrapper in the sense of a parent-child
relationship. The GMU wrapper has no HFI (Hardware Firmware Interface)
to communicate through crafted messages, but relies on plain register
accesses.

Konrad
> 
> 
> Best regards,
> Krzysztof
>
Konrad Dybcio Feb. 24, 2023, 11:51 a.m. UTC | #4
On 24.02.2023 12:17, Krzysztof Kozlowski wrote:
> On 23/02/2023 13:06, Konrad Dybcio wrote:
>> GMU wrapper-equipped A6xx GPUs require clocks and clock-names to be
>> specified under the GPU node, just like their older cousins.
>> Account for that.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  .../devicetree/bindings/display/msm/gpu.yaml       | 63 ++++++++++++++++++----
>>  1 file changed, 53 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.yaml b/Documentation/devicetree/bindings/display/msm/gpu.yaml
>> index d4191cca71fb..e6d3160601bc 100644
>> --- a/Documentation/devicetree/bindings/display/msm/gpu.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/gpu.yaml
>> @@ -36,10 +36,7 @@ properties:
>>  
>>    reg-names:
>>      minItems: 1
>> -    items:
>> -      - const: kgsl_3d0_reg_memory
>> -      - const: cx_mem
>> -      - const: cx_dbgc
>> +    maxItems: 3
>>  
>>    interrupts:
>>      maxItems: 1
>> @@ -147,26 +144,72 @@ allOf:
>>                  description: GPU Alternative Memory Interface clock
>>                - const: gfx3d
>>                  description: GPU 3D engine clock
>> +              - const: gmu
>> +                description: CX GMU clock
>>                - const: rbbmtimer
>>                  description: GPU RBBM Timer for Adreno 5xx series
>>                - const: rbcpr
>>                  description: GPU RB Core Power Reduction clock
>> +              - const: xo
>> +                description: GPUCC clocksource clock
>>            minItems: 2
>> -          maxItems: 7
>> +          maxItems: 9
> 
> Your commit says A6xx but this is a3-5xx. I don't understand this change.
Right, it's a leftover unrelated hunk. I'll remove it.

> 
>>  
>>        required:
>>          - clocks
>>          - clock-names
>> +
>>    - if:
>>        properties:
>>          compatible:
>>            contains:
>> -            pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
>> -
>> -    then: # Since Adreno 6xx series clocks should be defined in GMU
>> +            enum:
>> +              - qcom,adreno-610.0
>> +              - qcom,adreno-619.1
>> +    then:
>>        properties:
>> -        clocks: false
>> -        clock-names: false
>> +        clock-names:
>> +          items:
>> +            - const: core
>> +              description: GPU Core clock
>> +            - const: iface
>> +              description: GPU Interface clock
>> +            - const: mem_iface
>> +              description: GPU Memory Interface clock
>> +            - const: alt_mem_iface
>> +              description: GPU Alternative Memory Interface clock
>> +            - const: gmu
>> +              description: CX GMU clock
>> +            - const: xo
>> +              description: GPUCC clocksource clock
>> +
>> +        reg-names:
>> +          minItems: 1
>> +          items:
>> +            - const: kgsl_3d0_reg_memory
>> +            - const: cx_dbgc
>> +
>> +      required:
>> +        - clocks
>> +        - clock-names
>> +    else:
>> +      if:
>> +        properties:
>> +          compatible:
>> +            contains:
>> +              pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
>> +
>> +      then: # Starting with A6xx, the clocks are usually defined in the GMU node
> 
> The comment is not accurate anymore.
I'll argue the semantics, they are still "usually" defined
in the GMU node..

Konrad
> 
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 24, 2023, 12:54 p.m. UTC | #5
On 24/02/2023 12:51, Konrad Dybcio wrote:
>>> +    else:
>>> +      if:
>>> +        properties:
>>> +          compatible:
>>> +            contains:
>>> +              pattern: '^qcom,adreno-6[0-9][0-9]\.[0-9]$'
>>> +
>>> +      then: # Starting with A6xx, the clocks are usually defined in the GMU node
>>
>> The comment is not accurate anymore.
> I'll argue the semantics, they are still "usually" defined
> in the GMU node..

Ah, usually. It's fine then.

Best regards,
Krzysztof