[v3] drm/i915/cmtg: Disable the CMTG

Commit Message

Gustavo Sousa Dec. 24, 2024, 4:53 p.m. UTC

The CMTG is a timing generator that runs in parallel with transcoders
timing generators and can be used as a reference for synchronization.

On PTL (display Xe3_LPD), we have observed that we are inheriting from
GOP a display configuration with the CMTG enabled. Because our driver
doesn't currently implement any CMTG sequences, the CMTG ends up still
enabled after our driver takes over.

We need to make sure that the CMTG is not enabled if we are not going to
use it. For that, let's add a partial implementation in our driver that
only cares about disabling the CMTG if it was found enabled during
initial hardware readout. In the future, we can also implement sequences
for enabling CMTG if that becomes a needed feature.

For completeness, we do not only cover Xe3_LPD but also all previous
display IPs that provide the CMTG.

v2:
 - DG2 does not have the CMTG. Update HAS_CMTG() accordingly.
 - Update logic to force disabling of CMTG only for initial commit.
v3:
 - Add missing changes for v2 that were staged but not committed.

Signed-off-by: Gustavo Sousa <[e-mail]>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_cmtg.c     | 311 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_cmtg.h     |  38 +++
 .../gpu/drm/i915/display/intel_cmtg_regs.h    |  21 ++
 drivers/gpu/drm/i915/display/intel_display.c  |  11 +
 .../gpu/drm/i915/display/intel_display_core.h |   4 +
 .../drm/i915/display/intel_display_device.h   |   1 +
 .../drm/i915/display/intel_display_driver.c   |   5 +
 .../drm/i915/display/intel_modeset_setup.c    |   5 +
 drivers/gpu/drm/i915/i915_reg.h               |   1 +
 drivers/gpu/drm/xe/Makefile                   |   1 +
 11 files changed, 399 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.h
 create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg_regs.h

Comments

Gustavo Sousa Dec. 24, 2024, 6:53 p.m. UTC | #1

Quoting Patchwork (2024-12-24 14:46:20-03:00)
>== Series Details ==
>
>Series: drm/i915/cmtg: Disable the CMTG (rev3)
>URL   : https://patchwork.freedesktop.org/series/142947/
>State : failure
>
>== Summary ==
>
>CI Bug Log - changes from CI_DRM_15892 -> Patchwork_142947v3
>====================================================
>
>Summary
>-------
>
>  **FAILURE**
>
>  Serious unknown changes coming with Patchwork_142947v3 absolutely need to be
>  verified manually.
>  
>  If you think the reported changes have nothing to do with the changes
>  introduced in Patchwork_142947v3, please notify your bug team ([e-mail]) to allow them
>  to document this new failure mode, which will reduce false positives in CI.
>
>  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/index.html
>
>Participating hosts (41 -> 40)
>------------------------------
>
>  Missing    (1): fi-snb-2520m 
>
>Possible new issues
>-------------------
>
>  Here are the unknown changes that may have been introduced in Patchwork_142947v3:
>
>### IGT changes ###
>
>#### Possible regressions ####
>
>  * igt@i915_pm_rpm@module-reload:
>    - bat-dg1-7:          [PASS][1] -> [FAIL][2]
>   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-dg1-7/igt@[e-mail]
>   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-dg1-7/igt@[e-mail]
>    - bat-rpls-4:         [PASS][3] -> [FAIL][4]
>   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-rpls-4/igt@[e-mail]
>   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-rpls-4/igt@[e-mail]
>    - fi-tgl-1115g4:      [PASS][5] -> [FAIL][6]
>   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/fi-tgl-1115g4/igt@[e-mail]
>   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-tgl-1115g4/igt@[e-mail]

I don't believe this is related to the series. Please, re-report.

+Jani,

The signature is very similar to what was once captured by i915#12903,
which was recently closed.

[i915#12903]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12903

--
Gustavo Sousa

>
>  
>Known issues
>------------
>
>  Here are the changes found in Patchwork_142947v3 that come from known issues:
>
>### IGT changes ###
>
>#### Issues hit ####
>
>  * igt@dmabuf@all-tests:
>    - fi-pnv-d510:        NOTRUN -> [INCOMPLETE][7] ([i915#12904]) +1 other test incomplete
>   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-pnv-d510/igt@[e-mail]
>
>  * igt@fbdev@info:
>    - fi-bsw-nick:        NOTRUN -> [SKIP][8] ([i915#1849])
>   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-bsw-nick/igt@[e-mail]
>
>  * igt@gem_lmem_swapping@parallel-random-engines:
>    - fi-bsw-nick:        NOTRUN -> [SKIP][9] +42 other tests skip
>   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-bsw-nick/igt@[e-mail]
>
>  * igt@i915_selftest@live:
>    - bat-mtlp-8:         [PASS][10] -> [DMESG-FAIL][11] ([i915#13393]) +1 other test dmesg-fail
>   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-mtlp-8/igt@[e-mail]
>   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-mtlp-8/igt@[e-mail]
>    - bat-twl-2:          NOTRUN -> [ABORT][12] ([i915#12919] / [i915#13397])
>   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-twl-2/igt@[e-mail]
>
>  * igt@i915_selftest@live@gt_mocs:
>    - bat-twl-2:          NOTRUN -> [ABORT][13] ([i915#12919])
>   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-twl-2/igt@i915_selftest@live@gt_mocs.html
>
>  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
>    - bat-dg2-11:         [PASS][14] -> [SKIP][15] ([i915#9197]) +3 other tests skip
>   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-dg2-11/igt@[e-mail]
>   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-dg2-11/igt@[e-mail]
>
>  * igt@kms_psr@psr-primary-mmap-gtt:
>    - fi-pnv-d510:        NOTRUN -> [SKIP][16] +31 other tests skip
>   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-pnv-d510/igt@[e-mail]
>
>  
>#### Possible fixes ####
>
>  * igt@core_hotunplug@unbind-rebind:
>    - bat-rpls-4:         [DMESG-WARN][17] -> [PASS][18]
>   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-rpls-4/igt@[e-mail]
>   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-rpls-4/igt@[e-mail]
>
>  * igt@i915_selftest@live:
>    - bat-adlp-9:         [ABORT][19] -> [PASS][20] +1 other test pass
>   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-adlp-9/igt@[e-mail]
>   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-adlp-9/igt@[e-mail]
>
>  * igt@i915_selftest@live@workarounds:
>    - bat-mtlp-6:         [DMESG-FAIL][21] ([i915#13393]) -> [PASS][22] +1 other test pass
>   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-mtlp-6/igt@i915_selftest@[e-mail]
>   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-mtlp-6/igt@i915_selftest@[e-mail]
>
>  
>#### Warnings ####
>
>  * igt@gem_exec_gttfill@basic:
>    - fi-pnv-d510:        [ABORT][23] ([i915#13169]) -> [SKIP][24]
>   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/fi-pnv-d510/igt@[e-mail]
>   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-pnv-d510/igt@[e-mail]
>
>  
>  [i915#12904]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904
>  [i915#12919]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12919
>  [i915#13169]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13169
>  [i915#13393]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13393
>  [i915#13397]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13397
>  [i915#1849]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1849
>  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197
>
>
>Build changes
>-------------
>
>  * Linux: CI_DRM_15892 -> Patchwork_142947v3
>
>  CI-20190529: 20190529
>  CI_DRM_15892: 08bd590935a5258ffd79355c59adffd72fb2c642 @ git://anongit.freedesktop.org/gfx-ci/linux
>  IGT_8172: 9112581619aa198fa03041d5c7e18e02f42ac00f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
>  Patchwork_142947v3: 08bd590935a5258ffd79355c59adffd72fb2c642 @ git://anongit.freedesktop.org/gfx-ci/linux
>
>== Logs ==
>
>For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/index.html

Ravali, JupallyX Dec. 26, 2024, 5:27 a.m. UTC | #2

Hi,

https://patchwork.freedesktop.org/series/142947/- Re-reported.
 
i915.CI.BAT - Re-reported.
 
Thanks,
Ravali.

-----Original Message-----
From: I915-ci-infra <[e-mail]> On Behalf Of Gustavo Sousa
Sent: 25 December 2024 00:24
To: [e-mail]; [e-mail]
Cc: [e-mail]; Saarinen, Jani <[e-mail]>
Subject: Re: ✗ i915.CI.BAT: failure for drm/i915/cmtg: Disable the CMTG (rev3)

Quoting Patchwork (2024-12-24 14:46:20-03:00)
>== Series Details ==
>
>Series: drm/i915/cmtg: Disable the CMTG (rev3)
>URL   : https://patchwork.freedesktop.org/series/142947/
>State : failure
>
>== Summary ==
>
>CI Bug Log - changes from CI_DRM_15892 -> Patchwork_142947v3 
>====================================================
>
>Summary
>-------
>
>  **FAILURE**
>
>  Serious unknown changes coming with Patchwork_142947v3 absolutely 
> need to be  verified manually.
>  
>  If you think the reported changes have nothing to do with the changes  
> introduced in Patchwork_142947v3, please notify your bug team 
> ([e-mail]) to allow them  to document this new failure mode, which will reduce false positives in CI.
>
>  External URL: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/index.html
>
>Participating hosts (41 -> 40)
>------------------------------
>
>  Missing    (1): fi-snb-2520m 
>
>Possible new issues
>-------------------
>
>  Here are the unknown changes that may have been introduced in Patchwork_142947v3:
>
>### IGT changes ###
>
>#### Possible regressions ####
>
>  * igt@i915_pm_rpm@module-reload:
>    - bat-dg1-7:          [PASS][1] -> [FAIL][2]
>   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-dg1-7/igt@[e-mail]
>   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-dg1-7/igt@[e-mail]
>    - bat-rpls-4:         [PASS][3] -> [FAIL][4]
>   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-rpls-4/igt@[e-mail]
>   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-rpls-4/igt@[e-mail]
>    - fi-tgl-1115g4:      [PASS][5] -> [FAIL][6]
>   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/fi-tgl-1115g4/igt@[e-mail]
>   [6]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-tgl-111
> 5g4/igt@[e-mail]

I don't believe this is related to the series. Please, re-report.

+Jani,

The signature is very similar to what was once captured by i915#12903, which was recently closed.

[i915#12903]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12903

--
Gustavo Sousa

>
>  
>Known issues
>------------
>
>  Here are the changes found in Patchwork_142947v3 that come from known issues:
>
>### IGT changes ###
>
>#### Issues hit ####
>
>  * igt@dmabuf@all-tests:
>    - fi-pnv-d510:        NOTRUN -> [INCOMPLETE][7] ([i915#12904]) +1 other test incomplete
>   [7]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-pnv-d51
> 0/igt@[e-mail]
>
>  * igt@fbdev@info:
>    - fi-bsw-nick:        NOTRUN -> [SKIP][8] ([i915#1849])
>   [8]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-bsw-nic
> k/igt@[e-mail]
>
>  * igt@gem_lmem_swapping@parallel-random-engines:
>    - fi-bsw-nick:        NOTRUN -> [SKIP][9] +42 other tests skip
>   [9]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-bsw-nic
> k/igt@[e-mail]
>
>  * igt@i915_selftest@live:
>    - bat-mtlp-8:         [PASS][10] -> [DMESG-FAIL][11] ([i915#13393]) +1 other test dmesg-fail
>   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-mtlp-8/igt@[e-mail]
>   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-mtlp-8/igt@[e-mail]
>    - bat-twl-2:          NOTRUN -> [ABORT][12] ([i915#12919] / [i915#13397])
>   [12]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-twl-2/
> igt@[e-mail]
>
>  * igt@i915_selftest@live@gt_mocs:
>    - bat-twl-2:          NOTRUN -> [ABORT][13] ([i915#12919])
>   [13]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-twl-2/
> igt@i915_selftest@live@gt_mocs.html
>
>  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
>    - bat-dg2-11:         [PASS][14] -> [SKIP][15] ([i915#9197]) +3 other tests skip
>   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-dg2-11/igt@[e-mail]
>   [15]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-dg2-11
> /igt@[e-mail]
>
>  * igt@kms_psr@psr-primary-mmap-gtt:
>    - fi-pnv-d510:        NOTRUN -> [SKIP][16] +31 other tests skip
>   [16]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-pnv-d51
> 0/igt@[e-mail]
>
>  
>#### Possible fixes ####
>
>  * igt@core_hotunplug@unbind-rebind:
>    - bat-rpls-4:         [DMESG-WARN][17] -> [PASS][18]
>   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-rpls-4/igt@[e-mail]
>   [18]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-rpls-4
> /igt@[e-mail]
>
>  * igt@i915_selftest@live:
>    - bat-adlp-9:         [ABORT][19] -> [PASS][20] +1 other test pass
>   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-adlp-9/igt@[e-mail]
>   [20]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-adlp-9
> /igt@[e-mail]
>
>  * igt@i915_selftest@live@workarounds:
>    - bat-mtlp-6:         [DMESG-FAIL][21] ([i915#13393]) -> [PASS][22] +1 other test pass
>   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/bat-mtlp-6/igt@i915_selftest@[e-mail]
>   [22]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/bat-mtlp-6
> /igt@i915_selftest@[e-mail]
>
>  
>#### Warnings ####
>
>  * igt@gem_exec_gttfill@basic:
>    - fi-pnv-d510:        [ABORT][23] ([i915#13169]) -> [SKIP][24]
>   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15892/fi-pnv-d510/igt@[e-mail]
>   [24]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/fi-pnv-d51
> 0/igt@[e-mail]
>
>  
>  [i915#12904]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904
>  [i915#12919]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12919
>  [i915#13169]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13169
>  [i915#13393]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13393
>  [i915#13397]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13397
>  [i915#1849]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1849
>  [i915#9197]: 
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197
>
>
>Build changes
>-------------
>
>  * Linux: CI_DRM_15892 -> Patchwork_142947v3
>
>  CI-20190529: 20190529
>  CI_DRM_15892: 08bd590935a5258ffd79355c59adffd72fb2c642 @ 
> git://anongit.freedesktop.org/gfx-ci/linux
>  IGT_8172: 9112581619aa198fa03041d5c7e18e02f42ac00f @ 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
>  Patchwork_142947v3: 08bd590935a5258ffd79355c59adffd72fb2c642 @ 
> git://anongit.freedesktop.org/gfx-ci/linux
>
>== Logs ==
>
>For more details see: 
>https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_142947v3/index.html

Hogander, Jouni Dec. 31, 2024, 11:23 a.m. UTC | #3

On Tue, 2024-12-24 at 13:53 -0300, Gustavo Sousa wrote:
> The CMTG is a timing generator that runs in parallel with transcoders
> timing generators and can be used as a reference for synchronization.
> 
> On PTL (display Xe3_LPD), we have observed that we are inheriting
> from
> GOP a display configuration with the CMTG enabled. Because our driver
> doesn't currently implement any CMTG sequences, the CMTG ends up
> still
> enabled after our driver takes over.
> 
> We need to make sure that the CMTG is not enabled if we are not going
> to
> use it. For that, let's add a partial implementation in our driver
> that
> only cares about disabling the CMTG if it was found enabled during
> initial hardware readout. In the future, we can also implement
> sequences
> for enabling CMTG if that becomes a needed feature.
> 
> For completeness, we do not only cover Xe3_LPD but also all previous
> display IPs that provide the CMTG.
> 
> v2:
>  - DG2 does not have the CMTG. Update HAS_CMTG() accordingly.
>  - Update logic to force disabling of CMTG only for initial commit.
> v3:
>  - Add missing changes for v2 that were staged but not committed.
> 
> Signed-off-by: Gustavo Sousa <[e-mail]>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/display/intel_cmtg.c     | 311
> ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_cmtg.h     |  38 +++
>  .../gpu/drm/i915/display/intel_cmtg_regs.h    |  21 ++
>  drivers/gpu/drm/i915/display/intel_display.c  |  11 +
>  .../gpu/drm/i915/display/intel_display_core.h |   4 +
>  .../drm/i915/display/intel_display_device.h   |   1 +
>  .../drm/i915/display/intel_display_driver.c   |   5 +
>  .../drm/i915/display/intel_modeset_setup.c    |   5 +
>  drivers/gpu/drm/i915/i915_reg.h               |   1 +
>  drivers/gpu/drm/xe/Makefile                   |   1 +
>  11 files changed, 399 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.h
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile
> index 3dda9f0eda82..7e7414453765 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -231,6 +231,7 @@ i915-y += \
>  	display/intel_bo.o \
>  	display/intel_bw.o \
>  	display/intel_cdclk.o \
> +	display/intel_cmtg.o \
>  	display/intel_color.o \
>  	display/intel_combo_phy.o \
>  	display/intel_connector.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c
> b/drivers/gpu/drm/i915/display/intel_cmtg.c
> new file mode 100644
> index 000000000000..27491497f515
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#include <linux/string.h>
> +#include <linux/string_choices.h>
> +#include <linux/types.h>
> +
> +#include "i915_drv.h"
> +#include "i915_reg.h"
> +#include "intel_crtc.h"
> +#include "intel_de.h"
> +#include "intel_cmtg.h"
> +#include "intel_cmtg_regs.h"
> +#include "intel_display_device.h"
> +#include "intel_display_types.h"
> +
> +/**
> + * DOC: Common Primary Timing Generator (CMTG)
> + *
> + * The CMTG is a timing generator that runs in parallel to
> transcoders timing
> + * generators (TG) to provide a synchronization mechanism where CMTG
> acts as
> + * primary and transcoders TGs act as secondary to the CMTG. The
> CMTG outputs
> + * its TG start and frame sync signals to the transcoders that are
> configured
> + * as secondary, which use those signals to synchronize their own
> timing with
> + * the CMTG's.
> + *
> + * The CMTG can be used only with eDP or MIPI command mode and
> supports the
> + * following use cases:
> + *
> + * - Dual eDP: The CMTG can be used to keep two eDP TGs in sync when
> on a
> + *   dual eDP configuration (with or without PSR/PSR2 enabled).
> + *
> + * - Single eDP as secondary: It is also possible to use a single
> eDP
> + *   configuration with the transcoder TG as secondary to the CMTG.
> That would
> + *   allow a flow that would not require a modeset on the existing
> eDP when a
> + *   new eDP is added for a dual eDP configuration with CMTG.
> + *
> + * - DC6v: In DC6v, the transcoder might be off but the CMTG keeps
> running to
> + *   maintain frame timings. When exiting DC6v, the transcoder TG
> then is
> + *   synced back the CMTG.
> + *
> + * Currently, the driver does not use the CMTG, but we need to make
> sure that
> + * we disable it in case we inherit a display configuration with it
> enabled.
> + */
> +
> +static struct intel_global_state *
> +intel_cmtg_duplicate_state(struct intel_global_obj *obj)
> +{
> +	struct intel_cmtg_state *cmtg_state =
> to_intel_cmtg_state(obj->state);
> +
> +	cmtg_state = kmemdup(cmtg_state, sizeof(*cmtg_state),
> GFP_KERNEL);
> +	if (!cmtg_state)
> +		return NULL;
> +
> +	return &cmtg_state->base;
> +}
> +
> +static void intel_cmtg_destroy_state(struct intel_global_obj *obj,
> +				     struct intel_global_state
> *state)
> +{
> +	kfree(state);
> +}
> +
> +static const struct intel_global_state_funcs intel_cmtg_state_funcs
> = {
> +	.atomic_duplicate_state = intel_cmtg_duplicate_state,
> +	.atomic_destroy_state = intel_cmtg_destroy_state,
> +};
> +
> +static bool intel_cmtg_has_cmtg_b(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) >= 20;
> +}
> +
> +static bool intel_cmtg_has_clock_sel(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) >= 14;
> +}
> +
> +static bool intel_cmtg_requires_modeset(struct intel_display
> *display)
> +{
> +	return DISPLAY_VER(display) < 20;
> +}
> +
> +static void intel_cmtg_dump_state(struct intel_display *display,
> +				  struct intel_cmtg_state
> *cmtg_state)
> +{
> +	if (intel_cmtg_has_cmtg_b(display))
> +		drm_dbg_kms(display->drm,
> +			    "CMTG state readout: CMTG A: %s, CMTG B:
> %s, transcoder A secondary: %s, transcoder B secondary: %s\n",
> +			    str_enabled_disabled(cmtg_state-
> >cmtg_a_enable),
> +			    str_enabled_disabled(cmtg_state-
> >cmtg_b_enable),
> +			    str_yes_no(cmtg_state-
> >trans_a_secondary),
> +			    str_yes_no(cmtg_state-
> >trans_b_secondary));
> +	else
> +		drm_dbg_kms(display->drm,
> +			    "CMTG state readout: %s, Transcoder A
> secondary: %s, Transcoder B secondary: %s\n",
> +			    str_enabled_disabled(cmtg_state-
> >cmtg_a_enable),
> +			    str_yes_no(cmtg_state-
> >trans_a_secondary),
> +			    str_yes_no(cmtg_state-
> >trans_b_secondary));
> +}
> +
> +int intel_cmtg_init(struct intel_display *display)
> +{
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	struct intel_cmtg_state *cmtg_state;
> +
> +	cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL);
> +	if (!cmtg_state)
> +		return -ENOMEM;
> +
> +	intel_atomic_global_obj_init(i915, &display->cmtg.obj,
> +				     &cmtg_state->base,
> +				     &intel_cmtg_state_funcs);
> +
> +	return 0;
> +}
> +
> +void intel_cmtg_readout_state(struct intel_display *display,
> +			      struct intel_cmtg_state *cmtg_state)
> +{
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	u32 val;
> +
> +	if (!HAS_CMTG(display))
> +		return;
> +
> +	val = intel_de_read(display, TRANS_CMTG_CTL_A);
> +	cmtg_state->cmtg_a_enable = val & CMTG_ENABLE;
> +
> +	if (intel_cmtg_has_cmtg_b(display)) {
> +		val = intel_de_read(display, TRANS_CMTG_CTL_B);
> +		cmtg_state->cmtg_b_enable = val & CMTG_ENABLE;
> +	}
> +
> +	if (intel_crtc_for_pipe(display, PIPE_A)) {
> +		val = intel_de_read(display,
> TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A));
> +		cmtg_state->trans_a_secondary = val &
> CMTG_SECONDARY_MODE;
> +	}
> +
> +	if (intel_crtc_for_pipe(display, PIPE_B)) {
> +		val = intel_de_read(display,
> TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B));
> +		cmtg_state->trans_b_secondary = val &
> CMTG_SECONDARY_MODE;
> +	}
> +
> +	intel_cmtg_dump_state(display, cmtg_state);
> +}
> +
> +static struct intel_cmtg_state *
> +intel_atomic_get_cmtg_state(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_global_state *obj_state =
> +		intel_atomic_get_global_obj_state(state,
> +						  &display-
> >cmtg.obj);
> +
> +	if (IS_ERR(obj_state))
> +		return ERR_CAST(obj_state);
> +
> +	return to_intel_cmtg_state(obj_state);
> +}
> +
> +static struct intel_cmtg_state *
> +intel_atomic_get_old_cmtg_state(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_global_state *obj_state =
> +		intel_atomic_get_old_global_obj_state(state,
> +						      &display-
> >cmtg.obj);
> +
> +	if (!obj_state)
> +		return NULL;
> +
> +	return to_intel_cmtg_state(obj_state);
> +}
> +
> +static struct intel_cmtg_state *
> +intel_atomic_get_new_cmtg_state(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_global_state *obj_state =
> +		intel_atomic_get_new_global_obj_state(state,
> +						      &display-
> >cmtg.obj);
> +
> +	if (!obj_state)
> +		return NULL;
> +
> +	return to_intel_cmtg_state(obj_state);
> +}
> +
> +static bool intel_cmtg_state_changed(struct intel_cmtg_state
> *old_cmtg_state,
> +				     struct intel_cmtg_state
> *new_cmtg_state)
> +{
> +	if (!new_cmtg_state)
> +		return false;
> +
> +	return old_cmtg_state->cmtg_a_enable != new_cmtg_state-
> >cmtg_a_enable ||
> +		old_cmtg_state->cmtg_b_enable != new_cmtg_state-
> >cmtg_b_enable ||
> +		old_cmtg_state->trans_a_secondary != new_cmtg_state-
> >trans_a_secondary ||
> +		old_cmtg_state->trans_b_secondary != new_cmtg_state-
> >trans_b_secondary;
> +}
> +
> +static int intel_cmtg_check_modeset(struct intel_atomic_state
> *state,
> +				    struct intel_cmtg_state
> *old_cmtg_state,
> +				    struct intel_cmtg_state
> *new_cmtg_state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	u8 pipe_mask;
> +
> +	if (!intel_cmtg_requires_modeset(display))
> +		return 0;
> +
> +	pipe_mask = 0;
> +
> +	if (old_cmtg_state->trans_a_secondary != new_cmtg_state-
> >trans_a_secondary)
> +		pipe_mask |= BIT(PIPE_A);
> +
> +	if (old_cmtg_state->trans_b_secondary != new_cmtg_state-
> >trans_b_secondary)
> +		pipe_mask |= BIT(PIPE_B);

On PTL this would mean we are always doing full modeset on boot? Is
full modeset really required when disabling CTMG? Bspec says:

"Switching between the local timing generator within the eDP and the
CMTG will require a modeset."

Are we really using CTMG or is it just enabled?

BR,

Jouni Högander

> +
> +	if (!pipe_mask)
> +		return 0;
> +
> +	return intel_modeset_pipes_in_mask_early(state, "updating
> CMTG config", pipe_mask);
> +}
> +
> +int intel_cmtg_force_disabled(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_cmtg_state *new_cmtg_state;
> +
> +	if (!HAS_CMTG(display))
> +		return 0;
> +
> +	new_cmtg_state = intel_atomic_get_cmtg_state(state);
> +	if (IS_ERR(new_cmtg_state))
> +		return PTR_ERR(new_cmtg_state);
> +
> +	new_cmtg_state->cmtg_a_enable = false;
> +	new_cmtg_state->cmtg_b_enable = false;
> +	new_cmtg_state->trans_a_secondary = false;
> +	new_cmtg_state->trans_b_secondary = false;
> +	return 0;
> +}
> +
> +int intel_cmtg_atomic_check(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_cmtg_state *old_cmtg_state;
> +	struct intel_cmtg_state *new_cmtg_state;
> +	int ret;
> +
> +	if (!HAS_CMTG(display))
> +		return 0;
> +
> +	old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
> +	new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
> +	if (!intel_cmtg_state_changed(old_cmtg_state,
> new_cmtg_state))
> +		return 0;
> +
> +	ret = intel_cmtg_check_modeset(state, old_cmtg_state,
> new_cmtg_state);
> +	if (ret)
> +		return ret;
> +
> +	return intel_atomic_serialize_global_state(&new_cmtg_state-
> >base);
> +}
> +
> +/*
> + * Access to CMTG registers require the PHY PLL that provides its
> clock to be
> + * running (which is configured via CMTG_CLK_SEL). As such, this
> function needs
> + * to be called before intel_commit_modeset_disables() to ensure
> that the PHY
> + * PLL is still enabled when doing this.
> + */
> +void intel_cmtg_disable(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	struct intel_cmtg_state *old_cmtg_state;
> +	struct intel_cmtg_state *new_cmtg_state;
> +
> +	if (!HAS_CMTG(display))
> +		return;
> +
> +	old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
> +	new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
> +	if (!intel_cmtg_state_changed(old_cmtg_state,
> new_cmtg_state))
> +		return;
> +
> +	drm_info(display->drm, "Disabling CMTG\n");
> +
> +	intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915,
> TRANSCODER_A), CMTG_SECONDARY_MODE, 0);
> +	intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915,
> TRANSCODER_B), CMTG_SECONDARY_MODE, 0);
> +
> +	intel_de_rmw(display, TRANS_CMTG_CTL_A, CMTG_ENABLE, 0);
> +
> +	if (intel_cmtg_has_cmtg_b(display))
> +		intel_de_rmw(display, TRANS_CMTG_CTL_B, CMTG_ENABLE,
> 0);
> +
> +	if (intel_cmtg_has_clock_sel(display)) {
> +		u32 clk_sel_clr = CMTG_CLK_SEL_A_MASK;
> +		u32 clk_sel_set = CMTG_CLK_SEL_A_DISABLED;
> +
> +		if (intel_cmtg_has_cmtg_b(display)) {
> +			clk_sel_clr |= CMTG_CLK_SEL_B_MASK;
> +			clk_sel_set |= CMTG_CLK_SEL_B_DISABLED;
> +		}
> +
> +		intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr,
> clk_sel_set);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h
> b/drivers/gpu/drm/i915/display/intel_cmtg.h
> new file mode 100644
> index 000000000000..4dfd31906d81
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_CMTG_H__
> +#define __INTEL_CMTG_H__
> +
> +#include "intel_display_core.h"
> +#include "intel_global_state.h"
> +
> +/*
> + * We describe here only the minimum state required to allow us to
> properly
> + * disable the CMTG if necessary.
> + */
> +struct intel_cmtg_state {
> +	struct intel_global_state base;
> +
> +	bool cmtg_a_enable;
> +	/*
> +	 * Xe3_LPD adds a second CMTG that can be used for dual eDP
> async mode.
> +	 */
> +	bool cmtg_b_enable;
> +	bool trans_a_secondary;
> +	bool trans_b_secondary;
> +};
> +
> +#define to_intel_cmtg_state(global_state) \
> +	container_of_const((global_state), struct intel_cmtg_state,
> base)
> +
> +int intel_cmtg_init(struct intel_display *display);
> +void intel_cmtg_readout_state(struct intel_display *display,
> +			      struct intel_cmtg_state *cmtg_state);
> +int intel_cmtg_force_disabled(struct intel_atomic_state *state);
> +int intel_cmtg_atomic_check(struct intel_atomic_state *state);
> +void intel_cmtg_disable(struct intel_atomic_state *state);
> +
> +#endif /* __INTEL_CMTG_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> new file mode 100644
> index 000000000000..082f96cad284
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_CMTG_REGS_H__
> +#define __INTEL_CMTG_REGS_H__
> +
> +#include "i915_reg_defs.h"
> +
> +#define CMTG_CLK_SEL			_MMIO(0x46160)
> +#define CMTG_CLK_SEL_A_MASK		REG_GENMASK(31, 29)
> +#define
> CMTG_CLK_SEL_A_DISABLED		REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
> +#define CMTG_CLK_SEL_B_MASK		REG_GENMASK(15, 13)
> +#define
> CMTG_CLK_SEL_B_DISABLED		REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
> +
> +#define TRANS_CMTG_CTL_A		_MMIO(0x6fa88)
> +#define TRANS_CMTG_CTL_B		_MMIO(0x6fb88)
> +#define  CMTG_ENABLE			REG_BIT(31)
> +
> +#endif /* __INTEL_CMTG_REGS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 4271da219b41..098985ad7ad4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -62,6 +62,7 @@
>  #include "intel_bw.h"
>  #include "intel_cdclk.h"
>  #include "intel_clock_gating.h"
> +#include "intel_cmtg.h"
>  #include "intel_color.h"
>  #include "intel_crt.h"
>  #include "intel_crtc.h"
> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	ret = intel_cmtg_atomic_check(state);
> +	if (ret)
> +		goto fail;
> +
>  	for_each_new_intel_crtc_in_state(state, crtc,
> new_crtc_state, i) {
>  		if (!intel_crtc_needs_modeset(new_crtc_state))
>  			continue;
> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
>  			intel_modeset_get_crtc_power_domains(new_crt
> c_state, &put_domains[crtc->pipe]);
>  	}
>  
> +	intel_cmtg_disable(state);
> +
>  	intel_commit_modeset_disables(state);
>  
>  	intel_dp_tunnel_atomic_alloc_bw(state);
> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device
> *dev)
>  		}
>  	}
>  
> +	ret =
> intel_cmtg_force_disabled(to_intel_atomic_state(state));
> +	if (ret)
> +		goto out;
> +
>  	ret = drm_atomic_commit(state);
>  
>  out:
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 554870d2494b..d0b039114e2d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -354,6 +354,10 @@ struct intel_display {
>  		unsigned int skl_preferred_vco_freq;
>  	} cdclk;
>  
> +	struct {
> +		struct intel_global_obj obj;
> +	} cmtg;
> +
>  	struct {
>  		struct drm_property_blob *glk_linear_degamma_lut;
>  	} color;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 9a333d9e6601..a126247eb6b8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -145,6 +145,7 @@ struct intel_display_platforms {
>  #define HAS_BIGJOINER(__display)	(DISPLAY_VER(__display) >=
> 11 && HAS_DSC(__display))
>  #define HAS_CDCLK_CRAWL(__display)	(DISPLAY_INFO(__display)-
> >has_cdclk_crawl)
>  #define HAS_CDCLK_SQUASH(__display)	(DISPLAY_INFO(__display)-
> >has_cdclk_squash)
> +#define HAS_CMTG(__display)		(!(__display)->platform.dg2
> && DISPLAY_VER(__display) >= 13)
>  #define HAS_CUR_FBC(__display)		(!HAS_GMCH(__display) &&
> IS_DISPLAY_VER(__display, 7, 13))
>  #define HAS_D12_PLANE_MINIMIZATION(__display)	((__display)-
> >platform.rocketlake || (__display)->platform.alderlake_s)
>  #define
> HAS_DBUF_OVERLAP_DETECTION(__display)	(DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 497b4a1f045f..3e1483814e8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -25,6 +25,7 @@
>  #include "intel_bios.h"
>  #include "intel_bw.h"
>  #include "intel_cdclk.h"
> +#include "intel_cmtg.h"
>  #include "intel_color.h"
>  #include "intel_crtc.h"
>  #include "intel_display_debugfs.h"
> @@ -269,6 +270,10 @@ int intel_display_driver_probe_noirq(struct
> intel_display *display)
>  	if (ret)
>  		goto cleanup_vga_client_pw_domain_dmc;
>  
> +	ret = intel_cmtg_init(display);
> +	if (ret)
> +		goto cleanup_vga_client_pw_domain_dmc;
> +
>  	intel_init_quirks(display);
>  
>  	intel_fbc_init(display);
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index 9db30db428f7..737a43916ac5 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -15,6 +15,7 @@
>  #include "i9xx_wm.h"
>  #include "intel_atomic.h"
>  #include "intel_bw.h"
> +#include "intel_cmtg.h"
>  #include "intel_color.h"
>  #include "intel_crtc.h"
>  #include "intel_crtc_state_dump.h"
> @@ -702,6 +703,8 @@ static void intel_modeset_readout_hw_state(struct
> drm_i915_private *i915)
>  	struct intel_display *display = &i915->display;
>  	struct intel_cdclk_state *cdclk_state =
>  		to_intel_cdclk_state(i915->display.cdclk.obj.state);
> +	struct intel_cmtg_state *cmtg_state =
> +		to_intel_cmtg_state(display->cmtg.obj.state);
>  	struct intel_dbuf_state *dbuf_state =
>  		to_intel_dbuf_state(i915->display.dbuf.obj.state);
>  	struct intel_pmdemand_state *pmdemand_state =
> @@ -906,6 +909,8 @@ static void intel_modeset_readout_hw_state(struct
> drm_i915_private *i915)
>  	}
>  
>  	intel_pmdemand_init_pmdemand_params(i915, pmdemand_state);
> +
> +	intel_cmtg_readout_state(display, cmtg_state);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 765e6c0528fb..b34bccfb1ccc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3565,6 +3565,7 @@ enum skl_power_gate {
>  #define _TRANS_DDI_FUNC_CTL2_DSI1	0x6bc04
>  #define TRANS_DDI_FUNC_CTL2(dev_priv,
> tran)	_MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
>  #define  PORT_SYNC_MODE_ENABLE			REG_BIT(4)
> +#define  CMTG_SECONDARY_MODE			REG_BIT(3)
>  #define  PORT_SYNC_MODE_MASTER_SELECT_MASK	REG_GENMASK(2, 0)
>  #define 
> PORT_SYNC_MODE_MASTER_SELECT(x)	REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK,(x))
>  
> diff --git a/drivers/gpu/drm/xe/Makefile
> b/drivers/gpu/drm/xe/Makefile
> index 5c97ad6ed738..cd0e25fce14b 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -199,6 +199,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>  	i915-display/intel_bios.o \
>  	i915-display/intel_bw.o \
>  	i915-display/intel_cdclk.o \
> +	i915-display/intel_cmtg.o \
>  	i915-display/intel_color.o \
>  	i915-display/intel_combo_phy.o \
>  	i915-display/intel_connector.o \

Jani Nikula Dec. 31, 2024, 11:45 a.m. UTC | #4

On Tue, 24 Dec 2024, Gustavo Sousa <[e-mail]> wrote:
> The CMTG is a timing generator that runs in parallel with transcoders
> timing generators and can be used as a reference for synchronization.
>
> On PTL (display Xe3_LPD), we have observed that we are inheriting from
> GOP a display configuration with the CMTG enabled. Because our driver
> doesn't currently implement any CMTG sequences, the CMTG ends up still
> enabled after our driver takes over.
>
> We need to make sure that the CMTG is not enabled if we are not going to
> use it. For that, let's add a partial implementation in our driver that
> only cares about disabling the CMTG if it was found enabled during
> initial hardware readout. In the future, we can also implement sequences
> for enabling CMTG if that becomes a needed feature.
>
> For completeness, we do not only cover Xe3_LPD but also all previous
> display IPs that provide the CMTG.
>
> v2:
>  - DG2 does not have the CMTG. Update HAS_CMTG() accordingly.
>  - Update logic to force disabling of CMTG only for initial commit.
> v3:
>  - Add missing changes for v2 that were staged but not committed.
>
> Signed-off-by: Gustavo Sousa <[e-mail]>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/display/intel_cmtg.c     | 311 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_cmtg.h     |  38 +++
>  .../gpu/drm/i915/display/intel_cmtg_regs.h    |  21 ++
>  drivers/gpu/drm/i915/display/intel_display.c  |  11 +
>  .../gpu/drm/i915/display/intel_display_core.h |   4 +
>  .../drm/i915/display/intel_display_device.h   |   1 +
>  .../drm/i915/display/intel_display_driver.c   |   5 +
>  .../drm/i915/display/intel_modeset_setup.c    |   5 +
>  drivers/gpu/drm/i915/i915_reg.h               |   1 +
>  drivers/gpu/drm/xe/Makefile                   |   1 +
>  11 files changed, 399 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.h
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 3dda9f0eda82..7e7414453765 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -231,6 +231,7 @@ i915-y += \
>  	display/intel_bo.o \
>  	display/intel_bw.o \
>  	display/intel_cdclk.o \
> +	display/intel_cmtg.o \
>  	display/intel_color.o \
>  	display/intel_combo_phy.o \
>  	display/intel_connector.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c b/drivers/gpu/drm/i915/display/intel_cmtg.c
> new file mode 100644
> index 000000000000..27491497f515
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#include <linux/string.h>
> +#include <linux/string_choices.h>
> +#include <linux/types.h>
> +
> +#include "i915_drv.h"
> +#include "i915_reg.h"
> +#include "intel_crtc.h"
> +#include "intel_de.h"
> +#include "intel_cmtg.h"
> +#include "intel_cmtg_regs.h"
> +#include "intel_display_device.h"
> +#include "intel_display_types.h"
> +
> +/**
> + * DOC: Common Primary Timing Generator (CMTG)
> + *
> + * The CMTG is a timing generator that runs in parallel to transcoders timing
> + * generators (TG) to provide a synchronization mechanism where CMTG acts as
> + * primary and transcoders TGs act as secondary to the CMTG. The CMTG outputs
> + * its TG start and frame sync signals to the transcoders that are configured
> + * as secondary, which use those signals to synchronize their own timing with
> + * the CMTG's.
> + *
> + * The CMTG can be used only with eDP or MIPI command mode and supports the
> + * following use cases:
> + *
> + * - Dual eDP: The CMTG can be used to keep two eDP TGs in sync when on a
> + *   dual eDP configuration (with or without PSR/PSR2 enabled).
> + *
> + * - Single eDP as secondary: It is also possible to use a single eDP
> + *   configuration with the transcoder TG as secondary to the CMTG. That would
> + *   allow a flow that would not require a modeset on the existing eDP when a
> + *   new eDP is added for a dual eDP configuration with CMTG.
> + *
> + * - DC6v: In DC6v, the transcoder might be off but the CMTG keeps running to
> + *   maintain frame timings. When exiting DC6v, the transcoder TG then is
> + *   synced back the CMTG.
> + *
> + * Currently, the driver does not use the CMTG, but we need to make sure that
> + * we disable it in case we inherit a display configuration with it enabled.
> + */

Thanks for writing this, appreciated.

> +
> +static struct intel_global_state *
> +intel_cmtg_duplicate_state(struct intel_global_obj *obj)
> +{
> +	struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(obj->state);
> +
> +	cmtg_state = kmemdup(cmtg_state, sizeof(*cmtg_state), GFP_KERNEL);
> +	if (!cmtg_state)
> +		return NULL;
> +
> +	return &cmtg_state->base;
> +}
> +
> +static void intel_cmtg_destroy_state(struct intel_global_obj *obj,
> +				     struct intel_global_state *state)
> +{
> +	kfree(state);
> +}
> +
> +static const struct intel_global_state_funcs intel_cmtg_state_funcs = {
> +	.atomic_duplicate_state = intel_cmtg_duplicate_state,
> +	.atomic_destroy_state = intel_cmtg_destroy_state,
> +};
> +
> +static bool intel_cmtg_has_cmtg_b(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) >= 20;
> +}
> +
> +static bool intel_cmtg_has_clock_sel(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) >= 14;
> +}
> +
> +static bool intel_cmtg_requires_modeset(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) < 20;
> +}
> +
> +static void intel_cmtg_dump_state(struct intel_display *display,
> +				  struct intel_cmtg_state *cmtg_state)
> +{
> +	if (intel_cmtg_has_cmtg_b(display))
> +		drm_dbg_kms(display->drm,
> +			    "CMTG state readout: CMTG A: %s, CMTG B: %s, transcoder A secondary: %s, transcoder B secondary: %s\n",
> +			    str_enabled_disabled(cmtg_state->cmtg_a_enable),
> +			    str_enabled_disabled(cmtg_state->cmtg_b_enable),
> +			    str_yes_no(cmtg_state->trans_a_secondary),
> +			    str_yes_no(cmtg_state->trans_b_secondary));
> +	else
> +		drm_dbg_kms(display->drm,
> +			    "CMTG state readout: %s, Transcoder A secondary: %s, Transcoder B secondary: %s\n",
> +			    str_enabled_disabled(cmtg_state->cmtg_a_enable),
> +			    str_yes_no(cmtg_state->trans_a_secondary),
> +			    str_yes_no(cmtg_state->trans_b_secondary));

Quite a bit of duplication here, with capitalization differences.

Might just use the first one and:

intel_cmtg_has_cmtg_b(display) ? str_enabled_disabled(cmtg_state->cmtg_b_enable) : "N/A"


> +}
> +
> +int intel_cmtg_init(struct intel_display *display)
> +{
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	struct intel_cmtg_state *cmtg_state;
> +
> +	cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL);
> +	if (!cmtg_state)
> +		return -ENOMEM;
> +
> +	intel_atomic_global_obj_init(i915, &display->cmtg.obj,
> +				     &cmtg_state->base,
> +				     &intel_cmtg_state_funcs);
> +
> +	return 0;
> +}
> +
> +void intel_cmtg_readout_state(struct intel_display *display,
> +			      struct intel_cmtg_state *cmtg_state)

There are no functions with _readout_state() suffix but much more with
_readout_hw_state(). I think we should stick to that convention.

> +{
> +	struct drm_i915_private *i915 = to_i915(display->drm);

Unnecessary, see below.

> +	u32 val;
> +
> +	if (!HAS_CMTG(display))
> +		return;
> +
> +	val = intel_de_read(display, TRANS_CMTG_CTL_A);
> +	cmtg_state->cmtg_a_enable = val & CMTG_ENABLE;
> +
> +	if (intel_cmtg_has_cmtg_b(display)) {
> +		val = intel_de_read(display, TRANS_CMTG_CTL_B);
> +		cmtg_state->cmtg_b_enable = val & CMTG_ENABLE;
> +	}
> +
> +	if (intel_crtc_for_pipe(display, PIPE_A)) {
> +		val = intel_de_read(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A));

Please use s/i915/display/ here.

> +		cmtg_state->trans_a_secondary = val & CMTG_SECONDARY_MODE;
> +	}
> +
> +	if (intel_crtc_for_pipe(display, PIPE_B)) {
> +		val = intel_de_read(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B));

Ditto.

> +		cmtg_state->trans_b_secondary = val & CMTG_SECONDARY_MODE;
> +	}
> +
> +	intel_cmtg_dump_state(display, cmtg_state);
> +}
> +
> +static struct intel_cmtg_state *
> +intel_atomic_get_cmtg_state(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_global_state *obj_state =
> +		intel_atomic_get_global_obj_state(state,
> +						  &display->cmtg.obj);

Fewer newlines is just fine. 100 char width is okay if it improves
readability.

> +
> +	if (IS_ERR(obj_state))
> +		return ERR_CAST(obj_state);
> +
> +	return to_intel_cmtg_state(obj_state);
> +}
> +
> +static struct intel_cmtg_state *
> +intel_atomic_get_old_cmtg_state(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_global_state *obj_state =
> +		intel_atomic_get_old_global_obj_state(state,
> +						      &display->cmtg.obj);
> +
> +	if (!obj_state)
> +		return NULL;
> +
> +	return to_intel_cmtg_state(obj_state);
> +}
> +
> +static struct intel_cmtg_state *
> +intel_atomic_get_new_cmtg_state(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_global_state *obj_state =
> +		intel_atomic_get_new_global_obj_state(state,
> +						      &display->cmtg.obj);
> +
> +	if (!obj_state)
> +		return NULL;
> +
> +	return to_intel_cmtg_state(obj_state);
> +}
> +
> +static bool intel_cmtg_state_changed(struct intel_cmtg_state *old_cmtg_state,
> +				     struct intel_cmtg_state *new_cmtg_state)
> +{
> +	if (!new_cmtg_state)
> +		return false;
> +
> +	return old_cmtg_state->cmtg_a_enable != new_cmtg_state->cmtg_a_enable ||
> +		old_cmtg_state->cmtg_b_enable != new_cmtg_state->cmtg_b_enable ||
> +		old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary ||
> +		old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary;
> +}
> +
> +static int intel_cmtg_check_modeset(struct intel_atomic_state *state,
> +				    struct intel_cmtg_state *old_cmtg_state,
> +				    struct intel_cmtg_state *new_cmtg_state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	u8 pipe_mask;
> +
> +	if (!intel_cmtg_requires_modeset(display))
> +		return 0;
> +
> +	pipe_mask = 0;
> +
> +	if (old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary)
> +		pipe_mask |= BIT(PIPE_A);
> +
> +	if (old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary)
> +		pipe_mask |= BIT(PIPE_B);
> +
> +	if (!pipe_mask)
> +		return 0;
> +
> +	return intel_modeset_pipes_in_mask_early(state, "updating CMTG config", pipe_mask);
> +}
> +
> +int intel_cmtg_force_disabled(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_cmtg_state *new_cmtg_state;
> +
> +	if (!HAS_CMTG(display))
> +		return 0;
> +
> +	new_cmtg_state = intel_atomic_get_cmtg_state(state);
> +	if (IS_ERR(new_cmtg_state))
> +		return PTR_ERR(new_cmtg_state);
> +
> +	new_cmtg_state->cmtg_a_enable = false;
> +	new_cmtg_state->cmtg_b_enable = false;
> +	new_cmtg_state->trans_a_secondary = false;
> +	new_cmtg_state->trans_b_secondary = false;

Blank line before return.

> +	return 0;
> +}
> +
> +int intel_cmtg_atomic_check(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_cmtg_state *old_cmtg_state;
> +	struct intel_cmtg_state *new_cmtg_state;
> +	int ret;
> +
> +	if (!HAS_CMTG(display))
> +		return 0;
> +
> +	old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
> +	new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
> +	if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state))
> +		return 0;
> +
> +	ret = intel_cmtg_check_modeset(state, old_cmtg_state, new_cmtg_state);
> +	if (ret)
> +		return ret;
> +
> +	return intel_atomic_serialize_global_state(&new_cmtg_state->base);
> +}
> +
> +/*
> + * Access to CMTG registers require the PHY PLL that provides its clock to be
> + * running (which is configured via CMTG_CLK_SEL). As such, this function needs
> + * to be called before intel_commit_modeset_disables() to ensure that the PHY
> + * PLL is still enabled when doing this.
> + */
> +void intel_cmtg_disable(struct intel_atomic_state *state)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	struct intel_cmtg_state *old_cmtg_state;
> +	struct intel_cmtg_state *new_cmtg_state;
> +
> +	if (!HAS_CMTG(display))
> +		return;
> +
> +	old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
> +	new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
> +	if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state))
> +		return;
> +
> +	drm_info(display->drm, "Disabling CMTG\n");
> +
> +	intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A), CMTG_SECONDARY_MODE, 0);
> +	intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B), CMTG_SECONDARY_MODE, 0);
> +
> +	intel_de_rmw(display, TRANS_CMTG_CTL_A, CMTG_ENABLE, 0);
> +
> +	if (intel_cmtg_has_cmtg_b(display))
> +		intel_de_rmw(display, TRANS_CMTG_CTL_B, CMTG_ENABLE, 0);
> +
> +	if (intel_cmtg_has_clock_sel(display)) {
> +		u32 clk_sel_clr = CMTG_CLK_SEL_A_MASK;
> +		u32 clk_sel_set = CMTG_CLK_SEL_A_DISABLED;
> +
> +		if (intel_cmtg_has_cmtg_b(display)) {
> +			clk_sel_clr |= CMTG_CLK_SEL_B_MASK;
> +			clk_sel_set |= CMTG_CLK_SEL_B_DISABLED;
> +		}
> +
> +		intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr, clk_sel_set);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h b/drivers/gpu/drm/i915/display/intel_cmtg.h
> new file mode 100644
> index 000000000000..4dfd31906d81
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_CMTG_H__
> +#define __INTEL_CMTG_H__
> +
> +#include "intel_display_core.h"
> +#include "intel_global_state.h"
> +
> +/*
> + * We describe here only the minimum state required to allow us to properly
> + * disable the CMTG if necessary.
> + */
> +struct intel_cmtg_state {
> +	struct intel_global_state base;
> +
> +	bool cmtg_a_enable;
> +	/*
> +	 * Xe3_LPD adds a second CMTG that can be used for dual eDP async mode.
> +	 */
> +	bool cmtg_b_enable;
> +	bool trans_a_secondary;
> +	bool trans_b_secondary;
> +};
> +
> +#define to_intel_cmtg_state(global_state) \
> +	container_of_const((global_state), struct intel_cmtg_state, base)
> +
> +int intel_cmtg_init(struct intel_display *display);
> +void intel_cmtg_readout_state(struct intel_display *display,
> +			      struct intel_cmtg_state *cmtg_state);
> +int intel_cmtg_force_disabled(struct intel_atomic_state *state);
> +int intel_cmtg_atomic_check(struct intel_atomic_state *state);
> +void intel_cmtg_disable(struct intel_atomic_state *state);

I understand you're following patterns from elsewhere in the driver. But
I've always wondered why we use a mixture of atomic state, global state,
and the specific (e.g. struct intel_cmtg_state) here. Makes no sense.

I believe the specific global state structs should all be internal to
the implementation in the .c file, opaque outside, with accessor
functions. The to_intel_cmtg_state() should be a proper function
(although the constness handling may require a _Generic wrapper).

I actually have had patches to do this for all the global state stuff,
but they've conflicted and gone stale. It's hard when basically anyone
can just poke at the state when this shouldn't really be the case.


> +
> +#endif /* __INTEL_CMTG_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> new file mode 100644
> index 000000000000..082f96cad284
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_CMTG_REGS_H__
> +#define __INTEL_CMTG_REGS_H__
> +
> +#include "i915_reg_defs.h"
> +
> +#define CMTG_CLK_SEL			_MMIO(0x46160)
> +#define CMTG_CLK_SEL_A_MASK		REG_GENMASK(31, 29)
> +#define CMTG_CLK_SEL_A_DISABLED		REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
> +#define CMTG_CLK_SEL_B_MASK		REG_GENMASK(15, 13)
> +#define CMTG_CLK_SEL_B_DISABLED		REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
> +
> +#define TRANS_CMTG_CTL_A		_MMIO(0x6fa88)
> +#define TRANS_CMTG_CTL_B		_MMIO(0x6fb88)

Could make those underscore prefixed, with a parametrized
TRANS_CMTG_CTL(idx).

> +#define  CMTG_ENABLE			REG_BIT(31)
> +
> +#endif /* __INTEL_CMTG_REGS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 4271da219b41..098985ad7ad4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -62,6 +62,7 @@
>  #include "intel_bw.h"
>  #include "intel_cdclk.h"
>  #include "intel_clock_gating.h"
> +#include "intel_cmtg.h"
>  #include "intel_color.h"
>  #include "intel_crt.h"
>  #include "intel_crtc.h"
> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	ret = intel_cmtg_atomic_check(state);
> +	if (ret)
> +		goto fail;
> +
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		if (!intel_crtc_needs_modeset(new_crtc_state))
>  			continue;
> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  			intel_modeset_get_crtc_power_domains(new_crtc_state, &put_domains[crtc->pipe]);
>  	}
>  
> +	intel_cmtg_disable(state);
> +
>  	intel_commit_modeset_disables(state);
>  
>  	intel_dp_tunnel_atomic_alloc_bw(state);
> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device *dev)
>  		}
>  	}
>  
> +	ret = intel_cmtg_force_disabled(to_intel_atomic_state(state));
> +	if (ret)
> +		goto out;
> +

I think the usual way is to do foo_sanitize_state() at
intel_modeset_setup_hw_state().

The above is incredibly specific to what intel_initial_commit()
does. There's nothing like that, it's a nice pure high level function
currently.

>  	ret = drm_atomic_commit(state);
>  
>  out:
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 554870d2494b..d0b039114e2d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -354,6 +354,10 @@ struct intel_display {
>  		unsigned int skl_preferred_vco_freq;
>  	} cdclk;
>  
> +	struct {
> +		struct intel_global_obj obj;
> +	} cmtg;
> +
>  	struct {
>  		struct drm_property_blob *glk_linear_degamma_lut;
>  	} color;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 9a333d9e6601..a126247eb6b8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -145,6 +145,7 @@ struct intel_display_platforms {
>  #define HAS_BIGJOINER(__display)	(DISPLAY_VER(__display) >= 11 && HAS_DSC(__display))
>  #define HAS_CDCLK_CRAWL(__display)	(DISPLAY_INFO(__display)->has_cdclk_crawl)
>  #define HAS_CDCLK_SQUASH(__display)	(DISPLAY_INFO(__display)->has_cdclk_squash)
> +#define HAS_CMTG(__display)		(!(__display)->platform.dg2 && DISPLAY_VER(__display) >= 13)
>  #define HAS_CUR_FBC(__display)		(!HAS_GMCH(__display) && IS_DISPLAY_VER(__display, 7, 13))
>  #define HAS_D12_PLANE_MINIMIZATION(__display)	((__display)->platform.rocketlake || (__display)->platform.alderlake_s)
>  #define HAS_DBUF_OVERLAP_DETECTION(__display)	(DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 497b4a1f045f..3e1483814e8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -25,6 +25,7 @@
>  #include "intel_bios.h"
>  #include "intel_bw.h"
>  #include "intel_cdclk.h"
> +#include "intel_cmtg.h"
>  #include "intel_color.h"
>  #include "intel_crtc.h"
>  #include "intel_display_debugfs.h"
> @@ -269,6 +270,10 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>  	if (ret)
>  		goto cleanup_vga_client_pw_domain_dmc;
>  
> +	ret = intel_cmtg_init(display);
> +	if (ret)
> +		goto cleanup_vga_client_pw_domain_dmc;
> +
>  	intel_init_quirks(display);
>  
>  	intel_fbc_init(display);
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index 9db30db428f7..737a43916ac5 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -15,6 +15,7 @@
>  #include "i9xx_wm.h"
>  #include "intel_atomic.h"
>  #include "intel_bw.h"
> +#include "intel_cmtg.h"
>  #include "intel_color.h"
>  #include "intel_crtc.h"
>  #include "intel_crtc_state_dump.h"
> @@ -702,6 +703,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
>  	struct intel_display *display = &i915->display;
>  	struct intel_cdclk_state *cdclk_state =
>  		to_intel_cdclk_state(i915->display.cdclk.obj.state);
> +	struct intel_cmtg_state *cmtg_state =
> +		to_intel_cmtg_state(display->cmtg.obj.state);
>  	struct intel_dbuf_state *dbuf_state =
>  		to_intel_dbuf_state(i915->display.dbuf.obj.state);
>  	struct intel_pmdemand_state *pmdemand_state =
> @@ -906,6 +909,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
>  	}
>  
>  	intel_pmdemand_init_pmdemand_params(i915, pmdemand_state);
> +
> +	intel_cmtg_readout_state(display, cmtg_state);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 765e6c0528fb..b34bccfb1ccc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3565,6 +3565,7 @@ enum skl_power_gate {
>  #define _TRANS_DDI_FUNC_CTL2_DSI1	0x6bc04
>  #define TRANS_DDI_FUNC_CTL2(dev_priv, tran)	_MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
>  #define  PORT_SYNC_MODE_ENABLE			REG_BIT(4)
> +#define  CMTG_SECONDARY_MODE			REG_BIT(3)
>  #define  PORT_SYNC_MODE_MASTER_SELECT_MASK	REG_GENMASK(2, 0)
>  #define  PORT_SYNC_MODE_MASTER_SELECT(x)	REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK, (x))
>  
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 5c97ad6ed738..cd0e25fce14b 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -199,6 +199,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>  	i915-display/intel_bios.o \
>  	i915-display/intel_bw.o \
>  	i915-display/intel_cdclk.o \
> +	i915-display/intel_cmtg.o \
>  	i915-display/intel_color.o \
>  	i915-display/intel_combo_phy.o \
>  	i915-display/intel_connector.o \

Gustavo Sousa Dec. 31, 2024, 1:08 p.m. UTC | #5

Quoting Hogander, Jouni (2024-12-31 08:23:58-03:00)
>On Tue, 2024-12-24 at 13:53 -0300, Gustavo Sousa wrote:
>> The CMTG is a timing generator that runs in parallel with transcoders
>> timing generators and can be used as a reference for synchronization.
>> 
>> On PTL (display Xe3_LPD), we have observed that we are inheriting
>> from
>> GOP a display configuration with the CMTG enabled. Because our driver
>> doesn't currently implement any CMTG sequences, the CMTG ends up
>> still
>> enabled after our driver takes over.
>> 
>> We need to make sure that the CMTG is not enabled if we are not going
>> to
>> use it. For that, let's add a partial implementation in our driver
>> that
>> only cares about disabling the CMTG if it was found enabled during
>> initial hardware readout. In the future, we can also implement
>> sequences
>> for enabling CMTG if that becomes a needed feature.
>> 
>> For completeness, we do not only cover Xe3_LPD but also all previous
>> display IPs that provide the CMTG.
>> 
>> v2:
>>  - DG2 does not have the CMTG. Update HAS_CMTG() accordingly.
>>  - Update logic to force disabling of CMTG only for initial commit.
>> v3:
>>  - Add missing changes for v2 that were staged but not committed.
>> 
>> Signed-off-by: Gustavo Sousa <[e-mail]>
>> ---
>>  drivers/gpu/drm/i915/Makefile                 |   1 +
>>  drivers/gpu/drm/i915/display/intel_cmtg.c     | 311
>> ++++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_cmtg.h     |  38 +++
>>  .../gpu/drm/i915/display/intel_cmtg_regs.h    |  21 ++
>>  drivers/gpu/drm/i915/display/intel_display.c  |  11 +
>>  .../gpu/drm/i915/display/intel_display_core.h |   4 +
>>  .../drm/i915/display/intel_display_device.h   |   1 +
>>  .../drm/i915/display/intel_display_driver.c   |   5 +
>>  .../drm/i915/display/intel_modeset_setup.c    |   5 +
>>  drivers/gpu/drm/i915/i915_reg.h               |   1 +
>>  drivers/gpu/drm/xe/Makefile                   |   1 +
>>  11 files changed, 399 insertions(+)
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.c
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.h
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>> 
>> diff --git a/drivers/gpu/drm/i915/Makefile
>> b/drivers/gpu/drm/i915/Makefile
>> index 3dda9f0eda82..7e7414453765 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -231,6 +231,7 @@ i915-y += \
>>          display/intel_bo.o \
>>          display/intel_bw.o \
>>          display/intel_cdclk.o \
>> +        display/intel_cmtg.o \
>>          display/intel_color.o \
>>          display/intel_combo_phy.o \
>>          display/intel_connector.o \
>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c
>> b/drivers/gpu/drm/i915/display/intel_cmtg.c
>> new file mode 100644
>> index 000000000000..27491497f515
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
>> @@ -0,0 +1,311 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright (C) 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/string.h>
>> +#include <linux/string_choices.h>
>> +#include <linux/types.h>
>> +
>> +#include "i915_drv.h"
>> +#include "i915_reg.h"
>> +#include "intel_crtc.h"
>> +#include "intel_de.h"
>> +#include "intel_cmtg.h"
>> +#include "intel_cmtg_regs.h"
>> +#include "intel_display_device.h"
>> +#include "intel_display_types.h"
>> +
>> +/**
>> + * DOC: Common Primary Timing Generator (CMTG)
>> + *
>> + * The CMTG is a timing generator that runs in parallel to
>> transcoders timing
>> + * generators (TG) to provide a synchronization mechanism where CMTG
>> acts as
>> + * primary and transcoders TGs act as secondary to the CMTG. The
>> CMTG outputs
>> + * its TG start and frame sync signals to the transcoders that are
>> configured
>> + * as secondary, which use those signals to synchronize their own
>> timing with
>> + * the CMTG's.
>> + *
>> + * The CMTG can be used only with eDP or MIPI command mode and
>> supports the
>> + * following use cases:
>> + *
>> + * - Dual eDP: The CMTG can be used to keep two eDP TGs in sync when
>> on a
>> + *   dual eDP configuration (with or without PSR/PSR2 enabled).
>> + *
>> + * - Single eDP as secondary: It is also possible to use a single
>> eDP
>> + *   configuration with the transcoder TG as secondary to the CMTG.
>> That would
>> + *   allow a flow that would not require a modeset on the existing
>> eDP when a
>> + *   new eDP is added for a dual eDP configuration with CMTG.
>> + *
>> + * - DC6v: In DC6v, the transcoder might be off but the CMTG keeps
>> running to
>> + *   maintain frame timings. When exiting DC6v, the transcoder TG
>> then is
>> + *   synced back the CMTG.
>> + *
>> + * Currently, the driver does not use the CMTG, but we need to make
>> sure that
>> + * we disable it in case we inherit a display configuration with it
>> enabled.
>> + */
>> +
>> +static struct intel_global_state *
>> +intel_cmtg_duplicate_state(struct intel_global_obj *obj)
>> +{
>> +        struct intel_cmtg_state *cmtg_state =
>> to_intel_cmtg_state(obj->state);
>> +
>> +        cmtg_state = kmemdup(cmtg_state, sizeof(*cmtg_state),
>> GFP_KERNEL);
>> +        if (!cmtg_state)
>> +                return NULL;
>> +
>> +        return &cmtg_state->base;
>> +}
>> +
>> +static void intel_cmtg_destroy_state(struct intel_global_obj *obj,
>> +                                     struct intel_global_state
>> *state)
>> +{
>> +        kfree(state);
>> +}
>> +
>> +static const struct intel_global_state_funcs intel_cmtg_state_funcs
>> = {
>> +        .atomic_duplicate_state = intel_cmtg_duplicate_state,
>> +        .atomic_destroy_state = intel_cmtg_destroy_state,
>> +};
>> +
>> +static bool intel_cmtg_has_cmtg_b(struct intel_display *display)
>> +{
>> +        return DISPLAY_VER(display) >= 20;
>> +}
>> +
>> +static bool intel_cmtg_has_clock_sel(struct intel_display *display)
>> +{
>> +        return DISPLAY_VER(display) >= 14;
>> +}
>> +
>> +static bool intel_cmtg_requires_modeset(struct intel_display
>> *display)
>> +{
>> +        return DISPLAY_VER(display) < 20;
>> +}
>> +
>> +static void intel_cmtg_dump_state(struct intel_display *display,
>> +                                  struct intel_cmtg_state
>> *cmtg_state)
>> +{
>> +        if (intel_cmtg_has_cmtg_b(display))
>> +                drm_dbg_kms(display->drm,
>> +                            "CMTG state readout: CMTG A: %s, CMTG B:
>> %s, transcoder A secondary: %s, transcoder B secondary: %s\n",
>> +                            str_enabled_disabled(cmtg_state-
>> >cmtg_a_enable),
>> +                            str_enabled_disabled(cmtg_state-
>> >cmtg_b_enable),
>> +                            str_yes_no(cmtg_state-
>> >trans_a_secondary),
>> +                            str_yes_no(cmtg_state-
>> >trans_b_secondary));
>> +        else
>> +                drm_dbg_kms(display->drm,
>> +                            "CMTG state readout: %s, Transcoder A
>> secondary: %s, Transcoder B secondary: %s\n",
>> +                            str_enabled_disabled(cmtg_state-
>> >cmtg_a_enable),
>> +                            str_yes_no(cmtg_state-
>> >trans_a_secondary),
>> +                            str_yes_no(cmtg_state-
>> >trans_b_secondary));
>> +}
>> +
>> +int intel_cmtg_init(struct intel_display *display)
>> +{
>> +        struct drm_i915_private *i915 = to_i915(display->drm);
>> +        struct intel_cmtg_state *cmtg_state;
>> +
>> +        cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL);
>> +        if (!cmtg_state)
>> +                return -ENOMEM;
>> +
>> +        intel_atomic_global_obj_init(i915, &display->cmtg.obj,
>> +                                     &cmtg_state->base,
>> +                                     &intel_cmtg_state_funcs);
>> +
>> +        return 0;
>> +}
>> +
>> +void intel_cmtg_readout_state(struct intel_display *display,
>> +                              struct intel_cmtg_state *cmtg_state)
>> +{
>> +        struct drm_i915_private *i915 = to_i915(display->drm);
>> +        u32 val;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return;
>> +
>> +        val = intel_de_read(display, TRANS_CMTG_CTL_A);
>> +        cmtg_state->cmtg_a_enable = val & CMTG_ENABLE;
>> +
>> +        if (intel_cmtg_has_cmtg_b(display)) {
>> +                val = intel_de_read(display, TRANS_CMTG_CTL_B);
>> +                cmtg_state->cmtg_b_enable = val & CMTG_ENABLE;
>> +        }
>> +
>> +        if (intel_crtc_for_pipe(display, PIPE_A)) {
>> +                val = intel_de_read(display,
>> TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A));
>> +                cmtg_state->trans_a_secondary = val &
>> CMTG_SECONDARY_MODE;
>> +        }
>> +
>> +        if (intel_crtc_for_pipe(display, PIPE_B)) {
>> +                val = intel_de_read(display,
>> TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B));
>> +                cmtg_state->trans_b_secondary = val &
>> CMTG_SECONDARY_MODE;
>> +        }
>> +
>> +        intel_cmtg_dump_state(display, cmtg_state);
>> +}
>> +
>> +static struct intel_cmtg_state *
>> +intel_atomic_get_cmtg_state(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_global_state *obj_state =
>> +                intel_atomic_get_global_obj_state(state,
>> +                                                  &display-
>> >cmtg.obj);
>> +
>> +        if (IS_ERR(obj_state))
>> +                return ERR_CAST(obj_state);
>> +
>> +        return to_intel_cmtg_state(obj_state);
>> +}
>> +
>> +static struct intel_cmtg_state *
>> +intel_atomic_get_old_cmtg_state(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_global_state *obj_state =
>> +                intel_atomic_get_old_global_obj_state(state,
>> +                                                      &display-
>> >cmtg.obj);
>> +
>> +        if (!obj_state)
>> +                return NULL;
>> +
>> +        return to_intel_cmtg_state(obj_state);
>> +}
>> +
>> +static struct intel_cmtg_state *
>> +intel_atomic_get_new_cmtg_state(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_global_state *obj_state =
>> +                intel_atomic_get_new_global_obj_state(state,
>> +                                                      &display-
>> >cmtg.obj);
>> +
>> +        if (!obj_state)
>> +                return NULL;
>> +
>> +        return to_intel_cmtg_state(obj_state);
>> +}
>> +
>> +static bool intel_cmtg_state_changed(struct intel_cmtg_state
>> *old_cmtg_state,
>> +                                     struct intel_cmtg_state
>> *new_cmtg_state)
>> +{
>> +        if (!new_cmtg_state)
>> +                return false;
>> +
>> +        return old_cmtg_state->cmtg_a_enable != new_cmtg_state-
>> >cmtg_a_enable ||
>> +                old_cmtg_state->cmtg_b_enable != new_cmtg_state-
>> >cmtg_b_enable ||
>> +                old_cmtg_state->trans_a_secondary != new_cmtg_state-
>> >trans_a_secondary ||
>> +                old_cmtg_state->trans_b_secondary != new_cmtg_state-
>> >trans_b_secondary;
>> +}
>> +
>> +static int intel_cmtg_check_modeset(struct intel_atomic_state
>> *state,
>> +                                    struct intel_cmtg_state
>> *old_cmtg_state,
>> +                                    struct intel_cmtg_state
>> *new_cmtg_state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        u8 pipe_mask;
>> +
>> +        if (!intel_cmtg_requires_modeset(display))
>> +                return 0;
>> +
>> +        pipe_mask = 0;
>> +
>> +        if (old_cmtg_state->trans_a_secondary != new_cmtg_state-
>> >trans_a_secondary)
>> +                pipe_mask |= BIT(PIPE_A);
>> +
>> +        if (old_cmtg_state->trans_b_secondary != new_cmtg_state-
>> >trans_b_secondary)
>> +                pipe_mask |= BIT(PIPE_B);
>
>On PTL this would mean we are always doing full modeset on boot? Is

No. Because of intel_cmtg_requires_modeset(), we only go through this if
we are on a display prior to Xe2_LPD (LNL's display). So, for PTL, a
modeset will not be required.

>full modeset really required when disabling CTMG? Bspec says:
>
>"Switching between the local timing generator within the eDP and the
>CMTG will require a modeset."

This is from the BSpec for pre-LNL displays (i.e. pre-Xe2_LPD). The
BSpec for LNL and PTL does not impose this restriction.

>
>Are we really using CTMG or is it just enabled?

The setup from GOP is using the CMTG with Pipe A.

--
Gustavo Sousa

>
>BR,
>
>Jouni Högander
>
>> +
>> +        if (!pipe_mask)
>> +                return 0;
>> +
>> +        return intel_modeset_pipes_in_mask_early(state, "updating
>> CMTG config", pipe_mask);
>> +}
>> +
>> +int intel_cmtg_force_disabled(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_cmtg_state *new_cmtg_state;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return 0;
>> +
>> +        new_cmtg_state = intel_atomic_get_cmtg_state(state);
>> +        if (IS_ERR(new_cmtg_state))
>> +                return PTR_ERR(new_cmtg_state);
>> +
>> +        new_cmtg_state->cmtg_a_enable = false;
>> +        new_cmtg_state->cmtg_b_enable = false;
>> +        new_cmtg_state->trans_a_secondary = false;
>> +        new_cmtg_state->trans_b_secondary = false;
>> +        return 0;
>> +}
>> +
>> +int intel_cmtg_atomic_check(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_cmtg_state *old_cmtg_state;
>> +        struct intel_cmtg_state *new_cmtg_state;
>> +        int ret;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return 0;
>> +
>> +        old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
>> +        new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
>> +        if (!intel_cmtg_state_changed(old_cmtg_state,
>> new_cmtg_state))
>> +                return 0;
>> +
>> +        ret = intel_cmtg_check_modeset(state, old_cmtg_state,
>> new_cmtg_state);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return intel_atomic_serialize_global_state(&new_cmtg_state-
>> >base);
>> +}
>> +
>> +/*
>> + * Access to CMTG registers require the PHY PLL that provides its
>> clock to be
>> + * running (which is configured via CMTG_CLK_SEL). As such, this
>> function needs
>> + * to be called before intel_commit_modeset_disables() to ensure
>> that the PHY
>> + * PLL is still enabled when doing this.
>> + */
>> +void intel_cmtg_disable(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct drm_i915_private *i915 = to_i915(display->drm);
>> +        struct intel_cmtg_state *old_cmtg_state;
>> +        struct intel_cmtg_state *new_cmtg_state;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return;
>> +
>> +        old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
>> +        new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
>> +        if (!intel_cmtg_state_changed(old_cmtg_state,
>> new_cmtg_state))
>> +                return;
>> +
>> +        drm_info(display->drm, "Disabling CMTG\n");
>> +
>> +        intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915,
>> TRANSCODER_A), CMTG_SECONDARY_MODE, 0);
>> +        intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915,
>> TRANSCODER_B), CMTG_SECONDARY_MODE, 0);
>> +
>> +        intel_de_rmw(display, TRANS_CMTG_CTL_A, CMTG_ENABLE, 0);
>> +
>> +        if (intel_cmtg_has_cmtg_b(display))
>> +                intel_de_rmw(display, TRANS_CMTG_CTL_B, CMTG_ENABLE,
>> 0);
>> +
>> +        if (intel_cmtg_has_clock_sel(display)) {
>> +                u32 clk_sel_clr = CMTG_CLK_SEL_A_MASK;
>> +                u32 clk_sel_set = CMTG_CLK_SEL_A_DISABLED;
>> +
>> +                if (intel_cmtg_has_cmtg_b(display)) {
>> +                        clk_sel_clr |= CMTG_CLK_SEL_B_MASK;
>> +                        clk_sel_set |= CMTG_CLK_SEL_B_DISABLED;
>> +                }
>> +
>> +                intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr,
>> clk_sel_set);
>> +        }
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h
>> b/drivers/gpu/drm/i915/display/intel_cmtg.h
>> new file mode 100644
>> index 000000000000..4dfd31906d81
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright (C) 2024 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_CMTG_H__
>> +#define __INTEL_CMTG_H__
>> +
>> +#include "intel_display_core.h"
>> +#include "intel_global_state.h"
>> +
>> +/*
>> + * We describe here only the minimum state required to allow us to
>> properly
>> + * disable the CMTG if necessary.
>> + */
>> +struct intel_cmtg_state {
>> +        struct intel_global_state base;
>> +
>> +        bool cmtg_a_enable;
>> +        /*
>> +         * Xe3_LPD adds a second CMTG that can be used for dual eDP
>> async mode.
>> +         */
>> +        bool cmtg_b_enable;
>> +        bool trans_a_secondary;
>> +        bool trans_b_secondary;
>> +};
>> +
>> +#define to_intel_cmtg_state(global_state) \
>> +        container_of_const((global_state), struct intel_cmtg_state,
>> base)
>> +
>> +int intel_cmtg_init(struct intel_display *display);
>> +void intel_cmtg_readout_state(struct intel_display *display,
>> +                              struct intel_cmtg_state *cmtg_state);
>> +int intel_cmtg_force_disabled(struct intel_atomic_state *state);
>> +int intel_cmtg_atomic_check(struct intel_atomic_state *state);
>> +void intel_cmtg_disable(struct intel_atomic_state *state);
>> +
>> +#endif /* __INTEL_CMTG_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>> b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>> new file mode 100644
>> index 000000000000..082f96cad284
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright (C) 2024 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_CMTG_REGS_H__
>> +#define __INTEL_CMTG_REGS_H__
>> +
>> +#include "i915_reg_defs.h"
>> +
>> +#define CMTG_CLK_SEL                        _MMIO(0x46160)
>> +#define CMTG_CLK_SEL_A_MASK                REG_GENMASK(31, 29)
>> +#define
>> CMTG_CLK_SEL_A_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
>> +#define CMTG_CLK_SEL_B_MASK                REG_GENMASK(15, 13)
>> +#define
>> CMTG_CLK_SEL_B_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
>> +
>> +#define TRANS_CMTG_CTL_A                _MMIO(0x6fa88)
>> +#define TRANS_CMTG_CTL_B                _MMIO(0x6fb88)
>> +#define  CMTG_ENABLE                        REG_BIT(31)
>> +
>> +#endif /* __INTEL_CMTG_REGS_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> b/drivers/gpu/drm/i915/display/intel_display.c
>> index 4271da219b41..098985ad7ad4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -62,6 +62,7 @@
>>  #include "intel_bw.h"
>>  #include "intel_cdclk.h"
>>  #include "intel_clock_gating.h"
>> +#include "intel_cmtg.h"
>>  #include "intel_color.h"
>>  #include "intel_crt.h"
>>  #include "intel_crtc.h"
>> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>>          if (ret)
>>                  goto fail;
>>  
>> +        ret = intel_cmtg_atomic_check(state);
>> +        if (ret)
>> +                goto fail;
>> +
>>          for_each_new_intel_crtc_in_state(state, crtc,
>> new_crtc_state, i) {
>>                  if (!intel_crtc_needs_modeset(new_crtc_state))
>>                          continue;
>> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct
>> intel_atomic_state *state)
>>                          intel_modeset_get_crtc_power_domains(new_crt
>> c_state, &put_domains[crtc->pipe]);
>>          }
>>  
>> +        intel_cmtg_disable(state);
>> +
>>          intel_commit_modeset_disables(state);
>>  
>>          intel_dp_tunnel_atomic_alloc_bw(state);
>> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device
>> *dev)
>>                  }
>>          }
>>  
>> +        ret =
>> intel_cmtg_force_disabled(to_intel_atomic_state(state));
>> +        if (ret)
>> +                goto out;
>> +
>>          ret = drm_atomic_commit(state);
>>  
>>  out:
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
>> b/drivers/gpu/drm/i915/display/intel_display_core.h
>> index 554870d2494b..d0b039114e2d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> @@ -354,6 +354,10 @@ struct intel_display {
>>                  unsigned int skl_preferred_vco_freq;
>>          } cdclk;
>>  
>> +        struct {
>> +                struct intel_global_obj obj;
>> +        } cmtg;
>> +
>>          struct {
>>                  struct drm_property_blob *glk_linear_degamma_lut;
>>          } color;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
>> b/drivers/gpu/drm/i915/display/intel_display_device.h
>> index 9a333d9e6601..a126247eb6b8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>> @@ -145,6 +145,7 @@ struct intel_display_platforms {
>>  #define HAS_BIGJOINER(__display)        (DISPLAY_VER(__display) >=
>> 11 && HAS_DSC(__display))
>>  #define HAS_CDCLK_CRAWL(__display)        (DISPLAY_INFO(__display)-
>> >has_cdclk_crawl)
>>  #define HAS_CDCLK_SQUASH(__display)        (DISPLAY_INFO(__display)-
>> >has_cdclk_squash)
>> +#define HAS_CMTG(__display)                (!(__display)->platform.dg2
>> && DISPLAY_VER(__display) >= 13)
>>  #define HAS_CUR_FBC(__display)                (!HAS_GMCH(__display) &&
>> IS_DISPLAY_VER(__display, 7, 13))
>>  #define HAS_D12_PLANE_MINIMIZATION(__display)        ((__display)-
>> >platform.rocketlake || (__display)->platform.alderlake_s)
>>  #define
>> HAS_DBUF_OVERLAP_DETECTION(__display)        (DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> index 497b4a1f045f..3e1483814e8d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> @@ -25,6 +25,7 @@
>>  #include "intel_bios.h"
>>  #include "intel_bw.h"
>>  #include "intel_cdclk.h"
>> +#include "intel_cmtg.h"
>>  #include "intel_color.h"
>>  #include "intel_crtc.h"
>>  #include "intel_display_debugfs.h"
>> @@ -269,6 +270,10 @@ int intel_display_driver_probe_noirq(struct
>> intel_display *display)
>>          if (ret)
>>                  goto cleanup_vga_client_pw_domain_dmc;
>>  
>> +        ret = intel_cmtg_init(display);
>> +        if (ret)
>> +                goto cleanup_vga_client_pw_domain_dmc;
>> +
>>          intel_init_quirks(display);
>>  
>>          intel_fbc_init(display);
>> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> index 9db30db428f7..737a43916ac5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> @@ -15,6 +15,7 @@
>>  #include "i9xx_wm.h"
>>  #include "intel_atomic.h"
>>  #include "intel_bw.h"
>> +#include "intel_cmtg.h"
>>  #include "intel_color.h"
>>  #include "intel_crtc.h"
>>  #include "intel_crtc_state_dump.h"
>> @@ -702,6 +703,8 @@ static void intel_modeset_readout_hw_state(struct
>> drm_i915_private *i915)
>>          struct intel_display *display = &i915->display;
>>          struct intel_cdclk_state *cdclk_state =
>>                  to_intel_cdclk_state(i915->display.cdclk.obj.state);
>> +        struct intel_cmtg_state *cmtg_state =
>> +                to_intel_cmtg_state(display->cmtg.obj.state);
>>          struct intel_dbuf_state *dbuf_state =
>>                  to_intel_dbuf_state(i915->display.dbuf.obj.state);
>>          struct intel_pmdemand_state *pmdemand_state =
>> @@ -906,6 +909,8 @@ static void intel_modeset_readout_hw_state(struct
>> drm_i915_private *i915)
>>          }
>>  
>>          intel_pmdemand_init_pmdemand_params(i915, pmdemand_state);
>> +
>> +        intel_cmtg_readout_state(display, cmtg_state);
>>  }
>>  
>>  static void
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 765e6c0528fb..b34bccfb1ccc 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3565,6 +3565,7 @@ enum skl_power_gate {
>>  #define _TRANS_DDI_FUNC_CTL2_DSI1        0x6bc04
>>  #define TRANS_DDI_FUNC_CTL2(dev_priv,
>> tran)        _MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
>>  #define  PORT_SYNC_MODE_ENABLE                        REG_BIT(4)
>> +#define  CMTG_SECONDARY_MODE                        REG_BIT(3)
>>  #define  PORT_SYNC_MODE_MASTER_SELECT_MASK        REG_GENMASK(2, 0)
>>  #define 
>> PORT_SYNC_MODE_MASTER_SELECT(x)        REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK,(x))
>>  
>> diff --git a/drivers/gpu/drm/xe/Makefile
>> b/drivers/gpu/drm/xe/Makefile
>> index 5c97ad6ed738..cd0e25fce14b 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -199,6 +199,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>>          i915-display/intel_bios.o \
>>          i915-display/intel_bw.o \
>>          i915-display/intel_cdclk.o \
>> +        i915-display/intel_cmtg.o \
>>          i915-display/intel_color.o \
>>          i915-display/intel_combo_phy.o \
>>          i915-display/intel_connector.o \
>

Gustavo Sousa Dec. 31, 2024, 6:30 p.m. UTC | #6

Quoting Jani Nikula (2024-12-31 08:45:56-03:00)
>On Tue, 24 Dec 2024, Gustavo Sousa <[e-mail]> wrote:
>> The CMTG is a timing generator that runs in parallel with transcoders
>> timing generators and can be used as a reference for synchronization.
>>
>> On PTL (display Xe3_LPD), we have observed that we are inheriting from
>> GOP a display configuration with the CMTG enabled. Because our driver
>> doesn't currently implement any CMTG sequences, the CMTG ends up still
>> enabled after our driver takes over.
>>
>> We need to make sure that the CMTG is not enabled if we are not going to
>> use it. For that, let's add a partial implementation in our driver that
>> only cares about disabling the CMTG if it was found enabled during
>> initial hardware readout. In the future, we can also implement sequences
>> for enabling CMTG if that becomes a needed feature.
>>
>> For completeness, we do not only cover Xe3_LPD but also all previous
>> display IPs that provide the CMTG.
>>
>> v2:
>>  - DG2 does not have the CMTG. Update HAS_CMTG() accordingly.
>>  - Update logic to force disabling of CMTG only for initial commit.
>> v3:
>>  - Add missing changes for v2 that were staged but not committed.
>>
>> Signed-off-by: Gustavo Sousa <[e-mail]>
>> ---
>>  drivers/gpu/drm/i915/Makefile                 |   1 +
>>  drivers/gpu/drm/i915/display/intel_cmtg.c     | 311 ++++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_cmtg.h     |  38 +++
>>  .../gpu/drm/i915/display/intel_cmtg_regs.h    |  21 ++
>>  drivers/gpu/drm/i915/display/intel_display.c  |  11 +
>>  .../gpu/drm/i915/display/intel_display_core.h |   4 +
>>  .../drm/i915/display/intel_display_device.h   |   1 +
>>  .../drm/i915/display/intel_display_driver.c   |   5 +
>>  .../drm/i915/display/intel_modeset_setup.c    |   5 +
>>  drivers/gpu/drm/i915/i915_reg.h               |   1 +
>>  drivers/gpu/drm/xe/Makefile                   |   1 +
>>  11 files changed, 399 insertions(+)
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.c
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.h
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 3dda9f0eda82..7e7414453765 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -231,6 +231,7 @@ i915-y += \
>>          display/intel_bo.o \
>>          display/intel_bw.o \
>>          display/intel_cdclk.o \
>> +        display/intel_cmtg.o \
>>          display/intel_color.o \
>>          display/intel_combo_phy.o \
>>          display/intel_connector.o \
>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c b/drivers/gpu/drm/i915/display/intel_cmtg.c
>> new file mode 100644
>> index 000000000000..27491497f515
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
>> @@ -0,0 +1,311 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright (C) 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/string.h>
>> +#include <linux/string_choices.h>
>> +#include <linux/types.h>
>> +
>> +#include "i915_drv.h"
>> +#include "i915_reg.h"
>> +#include "intel_crtc.h"
>> +#include "intel_de.h"
>> +#include "intel_cmtg.h"
>> +#include "intel_cmtg_regs.h"
>> +#include "intel_display_device.h"
>> +#include "intel_display_types.h"
>> +
>> +/**
>> + * DOC: Common Primary Timing Generator (CMTG)
>> + *
>> + * The CMTG is a timing generator that runs in parallel to transcoders timing
>> + * generators (TG) to provide a synchronization mechanism where CMTG acts as
>> + * primary and transcoders TGs act as secondary to the CMTG. The CMTG outputs
>> + * its TG start and frame sync signals to the transcoders that are configured
>> + * as secondary, which use those signals to synchronize their own timing with
>> + * the CMTG's.
>> + *
>> + * The CMTG can be used only with eDP or MIPI command mode and supports the
>> + * following use cases:
>> + *
>> + * - Dual eDP: The CMTG can be used to keep two eDP TGs in sync when on a
>> + *   dual eDP configuration (with or without PSR/PSR2 enabled).
>> + *
>> + * - Single eDP as secondary: It is also possible to use a single eDP
>> + *   configuration with the transcoder TG as secondary to the CMTG. That would
>> + *   allow a flow that would not require a modeset on the existing eDP when a
>> + *   new eDP is added for a dual eDP configuration with CMTG.
>> + *
>> + * - DC6v: In DC6v, the transcoder might be off but the CMTG keeps running to
>> + *   maintain frame timings. When exiting DC6v, the transcoder TG then is
>> + *   synced back the CMTG.
>> + *
>> + * Currently, the driver does not use the CMTG, but we need to make sure that
>> + * we disable it in case we inherit a display configuration with it enabled.
>> + */
>
>Thanks for writing this, appreciated.
>
>> +
>> +static struct intel_global_state *
>> +intel_cmtg_duplicate_state(struct intel_global_obj *obj)
>> +{
>> +        struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(obj->state);
>> +
>> +        cmtg_state = kmemdup(cmtg_state, sizeof(*cmtg_state), GFP_KERNEL);
>> +        if (!cmtg_state)
>> +                return NULL;
>> +
>> +        return &cmtg_state->base;
>> +}
>> +
>> +static void intel_cmtg_destroy_state(struct intel_global_obj *obj,
>> +                                     struct intel_global_state *state)
>> +{
>> +        kfree(state);
>> +}
>> +
>> +static const struct intel_global_state_funcs intel_cmtg_state_funcs = {
>> +        .atomic_duplicate_state = intel_cmtg_duplicate_state,
>> +        .atomic_destroy_state = intel_cmtg_destroy_state,
>> +};
>> +
>> +static bool intel_cmtg_has_cmtg_b(struct intel_display *display)
>> +{
>> +        return DISPLAY_VER(display) >= 20;
>> +}
>> +
>> +static bool intel_cmtg_has_clock_sel(struct intel_display *display)
>> +{
>> +        return DISPLAY_VER(display) >= 14;
>> +}
>> +
>> +static bool intel_cmtg_requires_modeset(struct intel_display *display)
>> +{
>> +        return DISPLAY_VER(display) < 20;
>> +}
>> +
>> +static void intel_cmtg_dump_state(struct intel_display *display,
>> +                                  struct intel_cmtg_state *cmtg_state)
>> +{
>> +        if (intel_cmtg_has_cmtg_b(display))
>> +                drm_dbg_kms(display->drm,
>> +                            "CMTG state readout: CMTG A: %s, CMTG B: %s, transcoder A secondary: %s, transcoder B secondary: %s\n",
>> +                            str_enabled_disabled(cmtg_state->cmtg_a_enable),
>> +                            str_enabled_disabled(cmtg_state->cmtg_b_enable),
>> +                            str_yes_no(cmtg_state->trans_a_secondary),
>> +                            str_yes_no(cmtg_state->trans_b_secondary));
>> +        else
>> +                drm_dbg_kms(display->drm,
>> +                            "CMTG state readout: %s, Transcoder A secondary: %s, Transcoder B secondary: %s\n",
>> +                            str_enabled_disabled(cmtg_state->cmtg_a_enable),
>> +                            str_yes_no(cmtg_state->trans_a_secondary),
>> +                            str_yes_no(cmtg_state->trans_b_secondary));
>
>Quite a bit of duplication here, with capitalization differences.
>
>Might just use the first one and:
>
>intel_cmtg_has_cmtg_b(display) ? str_enabled_disabled(cmtg_state->cmtg_b_enable) : "N/A"

Okay. Going with this for next version.

>
>
>> +}
>> +
>> +int intel_cmtg_init(struct intel_display *display)
>> +{
>> +        struct drm_i915_private *i915 = to_i915(display->drm);
>> +        struct intel_cmtg_state *cmtg_state;
>> +
>> +        cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL);
>> +        if (!cmtg_state)
>> +                return -ENOMEM;
>> +
>> +        intel_atomic_global_obj_init(i915, &display->cmtg.obj,
>> +                                     &cmtg_state->base,
>> +                                     &intel_cmtg_state_funcs);
>> +
>> +        return 0;
>> +}
>> +
>> +void intel_cmtg_readout_state(struct intel_display *display,
>> +                              struct intel_cmtg_state *cmtg_state)
>
>There are no functions with _readout_state() suffix but much more with
>_readout_hw_state(). I think we should stick to that convention.

Okay.

>
>> +{
>> +        struct drm_i915_private *i915 = to_i915(display->drm);
>
>Unnecessary, see below.
>
>> +        u32 val;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return;
>> +
>> +        val = intel_de_read(display, TRANS_CMTG_CTL_A);
>> +        cmtg_state->cmtg_a_enable = val & CMTG_ENABLE;
>> +
>> +        if (intel_cmtg_has_cmtg_b(display)) {
>> +                val = intel_de_read(display, TRANS_CMTG_CTL_B);
>> +                cmtg_state->cmtg_b_enable = val & CMTG_ENABLE;
>> +        }
>> +
>> +        if (intel_crtc_for_pipe(display, PIPE_A)) {
>> +                val = intel_de_read(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A));
>
>Please use s/i915/display/ here.

Oh. I should have checked that _MMIO_TRANS2() uses the display struct.
Thanks.

>
>> +                cmtg_state->trans_a_secondary = val & CMTG_SECONDARY_MODE;
>> +        }
>> +
>> +        if (intel_crtc_for_pipe(display, PIPE_B)) {
>> +                val = intel_de_read(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B));
>
>Ditto.
>
>> +                cmtg_state->trans_b_secondary = val & CMTG_SECONDARY_MODE;
>> +        }
>> +
>> +        intel_cmtg_dump_state(display, cmtg_state);
>> +}
>> +
>> +static struct intel_cmtg_state *
>> +intel_atomic_get_cmtg_state(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_global_state *obj_state =
>> +                intel_atomic_get_global_obj_state(state,
>> +                                                  &display->cmtg.obj);
>
>Fewer newlines is just fine. 100 char width is okay if it improves
>readability.

Well, the whole declaration and initialization does not fit the 100
char limit. I guess, as a compromise, I could go with the call to
intel_atomic_get_global_obj_state() on its own line, but fitting a
single line.

>
>> +
>> +        if (IS_ERR(obj_state))
>> +                return ERR_CAST(obj_state);
>> +
>> +        return to_intel_cmtg_state(obj_state);
>> +}
>> +
>> +static struct intel_cmtg_state *
>> +intel_atomic_get_old_cmtg_state(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_global_state *obj_state =
>> +                intel_atomic_get_old_global_obj_state(state,
>> +                                                      &display->cmtg.obj);
>> +
>> +        if (!obj_state)
>> +                return NULL;
>> +
>> +        return to_intel_cmtg_state(obj_state);
>> +}
>> +
>> +static struct intel_cmtg_state *
>> +intel_atomic_get_new_cmtg_state(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_global_state *obj_state =
>> +                intel_atomic_get_new_global_obj_state(state,
>> +                                                      &display->cmtg.obj);
>> +
>> +        if (!obj_state)
>> +                return NULL;
>> +
>> +        return to_intel_cmtg_state(obj_state);
>> +}
>> +
>> +static bool intel_cmtg_state_changed(struct intel_cmtg_state *old_cmtg_state,
>> +                                     struct intel_cmtg_state *new_cmtg_state)
>> +{
>> +        if (!new_cmtg_state)
>> +                return false;
>> +
>> +        return old_cmtg_state->cmtg_a_enable != new_cmtg_state->cmtg_a_enable ||
>> +                old_cmtg_state->cmtg_b_enable != new_cmtg_state->cmtg_b_enable ||
>> +                old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary ||
>> +                old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary;
>> +}
>> +
>> +static int intel_cmtg_check_modeset(struct intel_atomic_state *state,
>> +                                    struct intel_cmtg_state *old_cmtg_state,
>> +                                    struct intel_cmtg_state *new_cmtg_state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        u8 pipe_mask;
>> +
>> +        if (!intel_cmtg_requires_modeset(display))
>> +                return 0;
>> +
>> +        pipe_mask = 0;
>> +
>> +        if (old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary)
>> +                pipe_mask |= BIT(PIPE_A);
>> +
>> +        if (old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary)
>> +                pipe_mask |= BIT(PIPE_B);
>> +
>> +        if (!pipe_mask)
>> +                return 0;
>> +
>> +        return intel_modeset_pipes_in_mask_early(state, "updating CMTG config", pipe_mask);
>> +}
>> +
>> +int intel_cmtg_force_disabled(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_cmtg_state *new_cmtg_state;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return 0;
>> +
>> +        new_cmtg_state = intel_atomic_get_cmtg_state(state);
>> +        if (IS_ERR(new_cmtg_state))
>> +                return PTR_ERR(new_cmtg_state);
>> +
>> +        new_cmtg_state->cmtg_a_enable = false;
>> +        new_cmtg_state->cmtg_b_enable = false;
>> +        new_cmtg_state->trans_a_secondary = false;
>> +        new_cmtg_state->trans_b_secondary = false;
>
>Blank line before return.

Okay.

>
>> +        return 0;
>> +}
>> +
>> +int intel_cmtg_atomic_check(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct intel_cmtg_state *old_cmtg_state;
>> +        struct intel_cmtg_state *new_cmtg_state;
>> +        int ret;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return 0;
>> +
>> +        old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
>> +        new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
>> +        if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state))
>> +                return 0;
>> +
>> +        ret = intel_cmtg_check_modeset(state, old_cmtg_state, new_cmtg_state);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return intel_atomic_serialize_global_state(&new_cmtg_state->base);
>> +}
>> +
>> +/*
>> + * Access to CMTG registers require the PHY PLL that provides its clock to be
>> + * running (which is configured via CMTG_CLK_SEL). As such, this function needs
>> + * to be called before intel_commit_modeset_disables() to ensure that the PHY
>> + * PLL is still enabled when doing this.
>> + */
>> +void intel_cmtg_disable(struct intel_atomic_state *state)
>> +{
>> +        struct intel_display *display = to_intel_display(state);
>> +        struct drm_i915_private *i915 = to_i915(display->drm);
>> +        struct intel_cmtg_state *old_cmtg_state;
>> +        struct intel_cmtg_state *new_cmtg_state;
>> +
>> +        if (!HAS_CMTG(display))
>> +                return;
>> +
>> +        old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
>> +        new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
>> +        if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state))
>> +                return;
>> +
>> +        drm_info(display->drm, "Disabling CMTG\n");
>> +
>> +        intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A), CMTG_SECONDARY_MODE, 0);
>> +        intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B), CMTG_SECONDARY_MODE, 0);
>> +
>> +        intel_de_rmw(display, TRANS_CMTG_CTL_A, CMTG_ENABLE, 0);
>> +
>> +        if (intel_cmtg_has_cmtg_b(display))
>> +                intel_de_rmw(display, TRANS_CMTG_CTL_B, CMTG_ENABLE, 0);
>> +
>> +        if (intel_cmtg_has_clock_sel(display)) {
>> +                u32 clk_sel_clr = CMTG_CLK_SEL_A_MASK;
>> +                u32 clk_sel_set = CMTG_CLK_SEL_A_DISABLED;
>> +
>> +                if (intel_cmtg_has_cmtg_b(display)) {
>> +                        clk_sel_clr |= CMTG_CLK_SEL_B_MASK;
>> +                        clk_sel_set |= CMTG_CLK_SEL_B_DISABLED;
>> +                }
>> +
>> +                intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr, clk_sel_set);
>> +        }
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h b/drivers/gpu/drm/i915/display/intel_cmtg.h
>> new file mode 100644
>> index 000000000000..4dfd31906d81
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright (C) 2024 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_CMTG_H__
>> +#define __INTEL_CMTG_H__
>> +
>> +#include "intel_display_core.h"
>> +#include "intel_global_state.h"
>> +
>> +/*
>> + * We describe here only the minimum state required to allow us to properly
>> + * disable the CMTG if necessary.
>> + */
>> +struct intel_cmtg_state {
>> +        struct intel_global_state base;
>> +
>> +        bool cmtg_a_enable;
>> +        /*
>> +         * Xe3_LPD adds a second CMTG that can be used for dual eDP async mode.
>> +         */
>> +        bool cmtg_b_enable;
>> +        bool trans_a_secondary;
>> +        bool trans_b_secondary;
>> +};
>> +
>> +#define to_intel_cmtg_state(global_state) \
>> +        container_of_const((global_state), struct intel_cmtg_state, base)
>> +
>> +int intel_cmtg_init(struct intel_display *display);
>> +void intel_cmtg_readout_state(struct intel_display *display,
>> +                              struct intel_cmtg_state *cmtg_state);
>> +int intel_cmtg_force_disabled(struct intel_atomic_state *state);
>> +int intel_cmtg_atomic_check(struct intel_atomic_state *state);
>> +void intel_cmtg_disable(struct intel_atomic_state *state);
>
>I understand you're following patterns from elsewhere in the driver. But

Correct.

>I've always wondered why we use a mixture of atomic state, global state,
>and the specific (e.g. struct intel_cmtg_state) here. Makes no sense.
>
>I believe the specific global state structs should all be internal to
>the implementation in the .c file, opaque outside, with accessor
>functions. The to_intel_cmtg_state() should be a proper function
>(although the constness handling may require a _Generic wrapper).

Yeah. I agree that the specific state bits should be private to the
implementation. Even the type "struct intel_cmtg_state" could be private
and then we would have the exposed interface dealing with only "struct
intel_global_state" or "struct intel_atomic_state" pointers.

The only function that is currently asking for a struct intel_cmtg_state
pointer is intel_cmtg_readout_state(), but that one can be easily
changed to receive a pointer to struct intel_global_state instead (or
even converted to be a function specific to display->cmtg.obj.state,
dropping the state argument).

With that, there would be no need to expose the struct intel_cmtg_state
type anymore and it can be made entirely private to intel_cmtg.c.

Let me know what you think.

>
>I actually have had patches to do this for all the global state stuff,
>but they've conflicted and gone stale. It's hard when basically anyone
>can just poke at the state when this shouldn't really be the case.

We could maybe start with CMTG state and progressively converting the
other guys?

(Although, after reading the entire review, if we decide to deal with
the CMTG only as part of the sanitization, I guess implementing the
whole global state "protocol" for the CMTG wouldn't make much sense
anymore, right?).

>
>
>> +
>> +#endif /* __INTEL_CMTG_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>> new file mode 100644
>> index 000000000000..082f96cad284
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright (C) 2024 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_CMTG_REGS_H__
>> +#define __INTEL_CMTG_REGS_H__
>> +
>> +#include "i915_reg_defs.h"
>> +
>> +#define CMTG_CLK_SEL                        _MMIO(0x46160)
>> +#define CMTG_CLK_SEL_A_MASK                REG_GENMASK(31, 29)
>> +#define CMTG_CLK_SEL_A_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
>> +#define CMTG_CLK_SEL_B_MASK                REG_GENMASK(15, 13)
>> +#define CMTG_CLK_SEL_B_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
>> +
>> +#define TRANS_CMTG_CTL_A                _MMIO(0x6fa88)
>> +#define TRANS_CMTG_CTL_B                _MMIO(0x6fb88)
>
>Could make those underscore prefixed, with a parametrized
>TRANS_CMTG_CTL(idx).

I had thought about that, but then decided to go with two separate
defines.

The main reason was because of the fact that the second instance was
added to support the async dual eDP case and not necessarily to be a
common "per pipe" resource like with other pipe/transcoder components.

>
>> +#define  CMTG_ENABLE                        REG_BIT(31)
>> +
>> +#endif /* __INTEL_CMTG_REGS_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 4271da219b41..098985ad7ad4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -62,6 +62,7 @@
>>  #include "intel_bw.h"
>>  #include "intel_cdclk.h"
>>  #include "intel_clock_gating.h"
>> +#include "intel_cmtg.h"
>>  #include "intel_color.h"
>>  #include "intel_crt.h"
>>  #include "intel_crtc.h"
>> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>>          if (ret)
>>                  goto fail;
>>  
>> +        ret = intel_cmtg_atomic_check(state);
>> +        if (ret)
>> +                goto fail;
>> +
>>          for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>                  if (!intel_crtc_needs_modeset(new_crtc_state))
>>                          continue;
>> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>                          intel_modeset_get_crtc_power_domains(new_crtc_state, &put_domains[crtc->pipe]);
>>          }
>>  
>> +        intel_cmtg_disable(state);
>> +
>>          intel_commit_modeset_disables(state);
>>  
>>          intel_dp_tunnel_atomic_alloc_bw(state);
>> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device *dev)
>>                  }
>>          }
>>  
>> +        ret = intel_cmtg_force_disabled(to_intel_atomic_state(state));
>> +        if (ret)
>> +                goto out;
>> +
>
>I think the usual way is to do foo_sanitize_state() at
>intel_modeset_setup_hw_state().

Hm... I see. In this case:

- For Xe2_LPD and newer, we can simply disable the CMTG as part of the
  sanitization;
- For pre-Xe2_LPD displays, which would require a modeset when disabling
  the CMTG, additionally force the CRTC to be disabled.

Right?

In this case, I guess implementing the whole "global state" protocol for
the CMTG wouldn't make much sense if we are not going to handle the
disabling as part of the initial commit. We could simply store the state
in a "vanilla" struct (and it would be good if such struct lived only
during the readout+sanitization time).

>
>The above is incredibly specific to what intel_initial_commit()
>does. There's nothing like that, it's a nice pure high level function
>currently.
>
>>          ret = drm_atomic_commit(state);
>>  
>>  out:
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>> index 554870d2494b..d0b039114e2d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> @@ -354,6 +354,10 @@ struct intel_display {
>>                  unsigned int skl_preferred_vco_freq;
>>          } cdclk;
>>  
>> +        struct {
>> +                struct intel_global_obj obj;
>> +        } cmtg;
>> +
>>          struct {
>>                  struct drm_property_blob *glk_linear_degamma_lut;
>>          } color;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>> index 9a333d9e6601..a126247eb6b8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>> @@ -145,6 +145,7 @@ struct intel_display_platforms {
>>  #define HAS_BIGJOINER(__display)        (DISPLAY_VER(__display) >= 11 && HAS_DSC(__display))
>>  #define HAS_CDCLK_CRAWL(__display)        (DISPLAY_INFO(__display)->has_cdclk_crawl)
>>  #define HAS_CDCLK_SQUASH(__display)        (DISPLAY_INFO(__display)->has_cdclk_squash)
>> +#define HAS_CMTG(__display)                (!(__display)->platform.dg2 && DISPLAY_VER(__display) >= 13)
>>  #define HAS_CUR_FBC(__display)                (!HAS_GMCH(__display) && IS_DISPLAY_VER(__display, 7, 13))
>>  #define HAS_D12_PLANE_MINIMIZATION(__display)        ((__display)->platform.rocketlake || (__display)->platform.alderlake_s)
>>  #define HAS_DBUF_OVERLAP_DETECTION(__display)        (DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> index 497b4a1f045f..3e1483814e8d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> @@ -25,6 +25,7 @@
>>  #include "intel_bios.h"
>>  #include "intel_bw.h"
>>  #include "intel_cdclk.h"
>> +#include "intel_cmtg.h"
>>  #include "intel_color.h"
>>  #include "intel_crtc.h"
>>  #include "intel_display_debugfs.h"
>> @@ -269,6 +270,10 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>>          if (ret)
>>                  goto cleanup_vga_client_pw_domain_dmc;
>>  
>> +        ret = intel_cmtg_init(display);
>> +        if (ret)
>> +                goto cleanup_vga_client_pw_domain_dmc;
>> +
>>          intel_init_quirks(display);
>>  
>>          intel_fbc_init(display);
>> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> index 9db30db428f7..737a43916ac5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> @@ -15,6 +15,7 @@
>>  #include "i9xx_wm.h"
>>  #include "intel_atomic.h"
>>  #include "intel_bw.h"
>> +#include "intel_cmtg.h"
>>  #include "intel_color.h"
>>  #include "intel_crtc.h"
>>  #include "intel_crtc_state_dump.h"
>> @@ -702,6 +703,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
>>          struct intel_display *display = &i915->display;
>>          struct intel_cdclk_state *cdclk_state =
>>                  to_intel_cdclk_state(i915->display.cdclk.obj.state);
>> +        struct intel_cmtg_state *cmtg_state =
>> +                to_intel_cmtg_state(display->cmtg.obj.state);
>>          struct intel_dbuf_state *dbuf_state =
>>                  to_intel_dbuf_state(i915->display.dbuf.obj.state);
>>          struct intel_pmdemand_state *pmdemand_state =
>> @@ -906,6 +909,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
>>          }
>>  
>>          intel_pmdemand_init_pmdemand_params(i915, pmdemand_state);
>> +
>> +        intel_cmtg_readout_state(display, cmtg_state);
>>  }
>>  
>>  static void
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 765e6c0528fb..b34bccfb1ccc 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3565,6 +3565,7 @@ enum skl_power_gate {
>>  #define _TRANS_DDI_FUNC_CTL2_DSI1        0x6bc04
>>  #define TRANS_DDI_FUNC_CTL2(dev_priv, tran)        _MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
>>  #define  PORT_SYNC_MODE_ENABLE                        REG_BIT(4)
>> +#define  CMTG_SECONDARY_MODE                        REG_BIT(3)
>>  #define  PORT_SYNC_MODE_MASTER_SELECT_MASK        REG_GENMASK(2, 0)
>>  #define  PORT_SYNC_MODE_MASTER_SELECT(x)        REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK, (x))
>>  
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 5c97ad6ed738..cd0e25fce14b 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -199,6 +199,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>>          i915-display/intel_bios.o \
>>          i915-display/intel_bw.o \
>>          i915-display/intel_cdclk.o \
>> +        i915-display/intel_cmtg.o \
>>          i915-display/intel_color.o \
>>          i915-display/intel_combo_phy.o \
>>          i915-display/intel_connector.o \
>
>-- 
>Jani Nikula, Intel

Hogander, Jouni Jan. 2, 2025, 11:16 a.m. UTC | #7

On Tue, 2024-12-31 at 10:08 -0300, Gustavo Sousa wrote:
> Quoting Hogander, Jouni (2024-12-31 08:23:58-03:00)
> > On Tue, 2024-12-24 at 13:53 -0300, Gustavo Sousa wrote:
> > > The CMTG is a timing generator that runs in parallel with
> > > transcoders
> > > timing generators and can be used as a reference for
> > > synchronization.
> > > 
> > > On PTL (display Xe3_LPD), we have observed that we are inheriting
> > > from
> > > GOP a display configuration with the CMTG enabled. Because our
> > > driver
> > > doesn't currently implement any CMTG sequences, the CMTG ends up
> > > still
> > > enabled after our driver takes over.
> > > 
> > > We need to make sure that the CMTG is not enabled if we are not
> > > going
> > > to
> > > use it. For that, let's add a partial implementation in our
> > > driver
> > > that
> > > only cares about disabling the CMTG if it was found enabled
> > > during
> > > initial hardware readout. In the future, we can also implement
> > > sequences
> > > for enabling CMTG if that becomes a needed feature.
> > > 
> > > For completeness, we do not only cover Xe3_LPD but also all
> > > previous
> > > display IPs that provide the CMTG.
> > > 
> > > v2:
> > >  - DG2 does not have the CMTG. Update HAS_CMTG() accordingly.
> > >  - Update logic to force disabling of CMTG only for initial
> > > commit.
> > > v3:
> > >  - Add missing changes for v2 that were staged but not committed.
> > > 
> > > Signed-off-by: Gustavo Sousa <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/Makefile                 |   1 +
> > >  drivers/gpu/drm/i915/display/intel_cmtg.c     | 311
> > > ++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_cmtg.h     |  38 +++
> > >  .../gpu/drm/i915/display/intel_cmtg_regs.h    |  21 ++
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  11 +
> > >  .../gpu/drm/i915/display/intel_display_core.h |   4 +
> > >  .../drm/i915/display/intel_display_device.h   |   1 +
> > >  .../drm/i915/display/intel_display_driver.c   |   5 +
> > >  .../drm/i915/display/intel_modeset_setup.c    |   5 +
> > >  drivers/gpu/drm/i915/i915_reg.h               |   1 +
> > >  drivers/gpu/drm/xe/Makefile                   |   1 +
> > >  11 files changed, 399 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.c
> > >  create mode 100644 drivers/gpu/drm/i915/display/intel_cmtg.h
> > >  create mode 100644
> > > drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > b/drivers/gpu/drm/i915/Makefile
> > > index 3dda9f0eda82..7e7414453765 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -231,6 +231,7 @@ i915-y += \
> > >          display/intel_bo.o \
> > >          display/intel_bw.o \
> > >          display/intel_cdclk.o \
> > > +        display/intel_cmtg.o \
> > >          display/intel_color.o \
> > >          display/intel_combo_phy.o \
> > >          display/intel_connector.o \
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > b/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > new file mode 100644
> > > index 000000000000..27491497f515
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > @@ -0,0 +1,311 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright (C) 2024 Intel Corporation
> > > + */
> > > +
> > > +#include <linux/string.h>
> > > +#include <linux/string_choices.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include "i915_drv.h"
> > > +#include "i915_reg.h"
> > > +#include "intel_crtc.h"
> > > +#include "intel_de.h"
> > > +#include "intel_cmtg.h"
> > > +#include "intel_cmtg_regs.h"
> > > +#include "intel_display_device.h"
> > > +#include "intel_display_types.h"
> > > +
> > > +/**
> > > + * DOC: Common Primary Timing Generator (CMTG)
> > > + *
> > > + * The CMTG is a timing generator that runs in parallel to
> > > transcoders timing
> > > + * generators (TG) to provide a synchronization mechanism where
> > > CMTG
> > > acts as
> > > + * primary and transcoders TGs act as secondary to the CMTG. The
> > > CMTG outputs
> > > + * its TG start and frame sync signals to the transcoders that
> > > are
> > > configured
> > > + * as secondary, which use those signals to synchronize their
> > > own
> > > timing with
> > > + * the CMTG's.
> > > + *
> > > + * The CMTG can be used only with eDP or MIPI command mode and
> > > supports the
> > > + * following use cases:
> > > + *
> > > + * - Dual eDP: The CMTG can be used to keep two eDP TGs in sync
> > > when
> > > on a
> > > + *   dual eDP configuration (with or without PSR/PSR2 enabled).
> > > + *
> > > + * - Single eDP as secondary: It is also possible to use a
> > > single
> > > eDP
> > > + *   configuration with the transcoder TG as secondary to the
> > > CMTG.
> > > That would
> > > + *   allow a flow that would not require a modeset on the
> > > existing
> > > eDP when a
> > > + *   new eDP is added for a dual eDP configuration with CMTG.
> > > + *
> > > + * - DC6v: In DC6v, the transcoder might be off but the CMTG
> > > keeps
> > > running to
> > > + *   maintain frame timings. When exiting DC6v, the transcoder
> > > TG
> > > then is
> > > + *   synced back the CMTG.
> > > + *
> > > + * Currently, the driver does not use the CMTG, but we need to
> > > make
> > > sure that
> > > + * we disable it in case we inherit a display configuration with
> > > it
> > > enabled.
> > > + */
> > > +
> > > +static struct intel_global_state *
> > > +intel_cmtg_duplicate_state(struct intel_global_obj *obj)
> > > +{
> > > +        struct intel_cmtg_state *cmtg_state =
> > > to_intel_cmtg_state(obj->state);
> > > +
> > > +        cmtg_state = kmemdup(cmtg_state, sizeof(*cmtg_state),
> > > GFP_KERNEL);
> > > +        if (!cmtg_state)
> > > +                return NULL;
> > > +
> > > +        return &cmtg_state->base;
> > > +}
> > > +
> > > +static void intel_cmtg_destroy_state(struct intel_global_obj
> > > *obj,
> > > +                                     struct intel_global_state
> > > *state)
> > > +{
> > > +        kfree(state);
> > > +}
> > > +
> > > +static const struct intel_global_state_funcs
> > > intel_cmtg_state_funcs
> > > = {
> > > +        .atomic_duplicate_state = intel_cmtg_duplicate_state,
> > > +        .atomic_destroy_state = intel_cmtg_destroy_state,
> > > +};
> > > +
> > > +static bool intel_cmtg_has_cmtg_b(struct intel_display *display)
> > > +{
> > > +        return DISPLAY_VER(display) >= 20;
> > > +}
> > > +
> > > +static bool intel_cmtg_has_clock_sel(struct intel_display
> > > *display)
> > > +{
> > > +        return DISPLAY_VER(display) >= 14;
> > > +}
> > > +
> > > +static bool intel_cmtg_requires_modeset(struct intel_display
> > > *display)
> > > +{
> > > +        return DISPLAY_VER(display) < 20;
> > > +}
> > > +
> > > +static void intel_cmtg_dump_state(struct intel_display *display,
> > > +                                  struct intel_cmtg_state
> > > *cmtg_state)
> > > +{
> > > +        if (intel_cmtg_has_cmtg_b(display))
> > > +                drm_dbg_kms(display->drm,
> > > +                            "CMTG state readout: CMTG A: %s,
> > > CMTG B:
> > > %s, transcoder A secondary: %s, transcoder B secondary: %s\n",
> > > +                            str_enabled_disabled(cmtg_state-
> > > > cmtg_a_enable),
> > > +                            str_enabled_disabled(cmtg_state-
> > > > cmtg_b_enable),
> > > +                            str_yes_no(cmtg_state-
> > > > trans_a_secondary),
> > > +                            str_yes_no(cmtg_state-
> > > > trans_b_secondary));
> > > +        else
> > > +                drm_dbg_kms(display->drm,
> > > +                            "CMTG state readout: %s, Transcoder
> > > A
> > > secondary: %s, Transcoder B secondary: %s\n",
> > > +                            str_enabled_disabled(cmtg_state-
> > > > cmtg_a_enable),
> > > +                            str_yes_no(cmtg_state-
> > > > trans_a_secondary),
> > > +                            str_yes_no(cmtg_state-
> > > > trans_b_secondary));
> > > +}
> > > +
> > > +int intel_cmtg_init(struct intel_display *display)
> > > +{
> > > +        struct drm_i915_private *i915 = to_i915(display->drm);
> > > +        struct intel_cmtg_state *cmtg_state;
> > > +
> > > +        cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL);
> > > +        if (!cmtg_state)
> > > +                return -ENOMEM;
> > > +
> > > +        intel_atomic_global_obj_init(i915, &display->cmtg.obj,
> > > +                                     &cmtg_state->base,
> > > +                                     &intel_cmtg_state_funcs);
> > > +
> > > +        return 0;
> > > +}
> > > +
> > > +void intel_cmtg_readout_state(struct intel_display *display,
> > > +                              struct intel_cmtg_state
> > > *cmtg_state)
> > > +{
> > > +        struct drm_i915_private *i915 = to_i915(display->drm);
> > > +        u32 val;
> > > +
> > > +        if (!HAS_CMTG(display))
> > > +                return;
> > > +
> > > +        val = intel_de_read(display, TRANS_CMTG_CTL_A);
> > > +        cmtg_state->cmtg_a_enable = val & CMTG_ENABLE;
> > > +
> > > +        if (intel_cmtg_has_cmtg_b(display)) {
> > > +                val = intel_de_read(display, TRANS_CMTG_CTL_B);
> > > +                cmtg_state->cmtg_b_enable = val & CMTG_ENABLE;
> > > +        }
> > > +
> > > +        if (intel_crtc_for_pipe(display, PIPE_A)) {
> > > +                val = intel_de_read(display,
> > > TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A));
> > > +                cmtg_state->trans_a_secondary = val &
> > > CMTG_SECONDARY_MODE;
> > > +        }
> > > +
> > > +        if (intel_crtc_for_pipe(display, PIPE_B)) {
> > > +                val = intel_de_read(display,
> > > TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B));
> > > +                cmtg_state->trans_b_secondary = val &
> > > CMTG_SECONDARY_MODE;
> > > +        }
> > > +
> > > +        intel_cmtg_dump_state(display, cmtg_state);
> > > +}
> > > +
> > > +static struct intel_cmtg_state *
> > > +intel_atomic_get_cmtg_state(struct intel_atomic_state *state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        struct intel_global_state *obj_state =
> > > +                intel_atomic_get_global_obj_state(state,
> > > +                                                  &display-
> > > > cmtg.obj);
> > > +
> > > +        if (IS_ERR(obj_state))
> > > +                return ERR_CAST(obj_state);
> > > +
> > > +        return to_intel_cmtg_state(obj_state);
> > > +}
> > > +
> > > +static struct intel_cmtg_state *
> > > +intel_atomic_get_old_cmtg_state(struct intel_atomic_state
> > > *state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        struct intel_global_state *obj_state =
> > > +                intel_atomic_get_old_global_obj_state(state,
> > > +                                                      &display-
> > > > cmtg.obj);
> > > +
> > > +        if (!obj_state)
> > > +                return NULL;
> > > +
> > > +        return to_intel_cmtg_state(obj_state);
> > > +}
> > > +
> > > +static struct intel_cmtg_state *
> > > +intel_atomic_get_new_cmtg_state(struct intel_atomic_state
> > > *state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        struct intel_global_state *obj_state =
> > > +                intel_atomic_get_new_global_obj_state(state,
> > > +                                                      &display-
> > > > cmtg.obj);
> > > +
> > > +        if (!obj_state)
> > > +                return NULL;
> > > +
> > > +        return to_intel_cmtg_state(obj_state);
> > > +}
> > > +
> > > +static bool intel_cmtg_state_changed(struct intel_cmtg_state
> > > *old_cmtg_state,
> > > +                                     struct intel_cmtg_state
> > > *new_cmtg_state)
> > > +{
> > > +        if (!new_cmtg_state)
> > > +                return false;
> > > +
> > > +        return old_cmtg_state->cmtg_a_enable != new_cmtg_state-
> > > > cmtg_a_enable ||
> > > +                old_cmtg_state->cmtg_b_enable != new_cmtg_state-
> > > > cmtg_b_enable ||
> > > +                old_cmtg_state->trans_a_secondary !=
> > > new_cmtg_state-
> > > > trans_a_secondary ||
> > > +                old_cmtg_state->trans_b_secondary !=
> > > new_cmtg_state-
> > > > trans_b_secondary;
> > > +}
> > > +
> > > +static int intel_cmtg_check_modeset(struct intel_atomic_state
> > > *state,
> > > +                                    struct intel_cmtg_state
> > > *old_cmtg_state,
> > > +                                    struct intel_cmtg_state
> > > *new_cmtg_state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        u8 pipe_mask;
> > > +
> > > +        if (!intel_cmtg_requires_modeset(display))
> > > +                return 0;
> > > +
> > > +        pipe_mask = 0;
> > > +
> > > +        if (old_cmtg_state->trans_a_secondary != new_cmtg_state-
> > > > trans_a_secondary)
> > > +                pipe_mask |= BIT(PIPE_A);
> > > +
> > > +        if (old_cmtg_state->trans_b_secondary != new_cmtg_state-
> > > > trans_b_secondary)
> > > +                pipe_mask |= BIT(PIPE_B);
> > 
> > On PTL this would mean we are always doing full modeset on boot? Is
> 
> No. Because of intel_cmtg_requires_modeset(), we only go through this
> if
> we are on a display prior to Xe2_LPD (LNL's display). So, for PTL, a
> modeset will not be required.
> 
> > full modeset really required when disabling CTMG? Bspec says:
> > 
> > "Switching between the local timing generator within the eDP and
> > the
> > CMTG will require a modeset."
> 
> This is from the BSpec for pre-LNL displays (i.e. pre-Xe2_LPD). The
> BSpec for LNL and PTL does not impose this restriction.
> 
> > 
> > Are we really using CTMG or is it just enabled?
> 
> The setup from GOP is using the CMTG with Pipe A.

Ok, thank you for the clarification.

BR,

Jouni Högander

> 
> --
> Gustavo Sousa
> 
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > > +
> > > +        if (!pipe_mask)
> > > +                return 0;
> > > +
> > > +        return intel_modeset_pipes_in_mask_early(state,
> > > "updating
> > > CMTG config", pipe_mask);
> > > +}
> > > +
> > > +int intel_cmtg_force_disabled(struct intel_atomic_state *state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        struct intel_cmtg_state *new_cmtg_state;
> > > +
> > > +        if (!HAS_CMTG(display))
> > > +                return 0;
> > > +
> > > +        new_cmtg_state = intel_atomic_get_cmtg_state(state);
> > > +        if (IS_ERR(new_cmtg_state))
> > > +                return PTR_ERR(new_cmtg_state);
> > > +
> > > +        new_cmtg_state->cmtg_a_enable = false;
> > > +        new_cmtg_state->cmtg_b_enable = false;
> > > +        new_cmtg_state->trans_a_secondary = false;
> > > +        new_cmtg_state->trans_b_secondary = false;
> > > +        return 0;
> > > +}
> > > +
> > > +int intel_cmtg_atomic_check(struct intel_atomic_state *state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        struct intel_cmtg_state *old_cmtg_state;
> > > +        struct intel_cmtg_state *new_cmtg_state;
> > > +        int ret;
> > > +
> > > +        if (!HAS_CMTG(display))
> > > +                return 0;
> > > +
> > > +        old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
> > > +        new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
> > > +        if (!intel_cmtg_state_changed(old_cmtg_state,
> > > new_cmtg_state))
> > > +                return 0;
> > > +
> > > +        ret = intel_cmtg_check_modeset(state, old_cmtg_state,
> > > new_cmtg_state);
> > > +        if (ret)
> > > +                return ret;
> > > +
> > > +        return
> > > intel_atomic_serialize_global_state(&new_cmtg_state-
> > > > base);
> > > +}
> > > +
> > > +/*
> > > + * Access to CMTG registers require the PHY PLL that provides
> > > its
> > > clock to be
> > > + * running (which is configured via CMTG_CLK_SEL). As such, this
> > > function needs
> > > + * to be called before intel_commit_modeset_disables() to ensure
> > > that the PHY
> > > + * PLL is still enabled when doing this.
> > > + */
> > > +void intel_cmtg_disable(struct intel_atomic_state *state)
> > > +{
> > > +        struct intel_display *display = to_intel_display(state);
> > > +        struct drm_i915_private *i915 = to_i915(display->drm);
> > > +        struct intel_cmtg_state *old_cmtg_state;
> > > +        struct intel_cmtg_state *new_cmtg_state;
> > > +
> > > +        if (!HAS_CMTG(display))
> > > +                return;
> > > +
> > > +        old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
> > > +        new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
> > > +        if (!intel_cmtg_state_changed(old_cmtg_state,
> > > new_cmtg_state))
> > > +                return;
> > > +
> > > +        drm_info(display->drm, "Disabling CMTG\n");
> > > +
> > > +        intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915,
> > > TRANSCODER_A), CMTG_SECONDARY_MODE, 0);
> > > +        intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915,
> > > TRANSCODER_B), CMTG_SECONDARY_MODE, 0);
> > > +
> > > +        intel_de_rmw(display, TRANS_CMTG_CTL_A, CMTG_ENABLE, 0);
> > > +
> > > +        if (intel_cmtg_has_cmtg_b(display))
> > > +                intel_de_rmw(display, TRANS_CMTG_CTL_B,
> > > CMTG_ENABLE,
> > > 0);
> > > +
> > > +        if (intel_cmtg_has_clock_sel(display)) {
> > > +                u32 clk_sel_clr = CMTG_CLK_SEL_A_MASK;
> > > +                u32 clk_sel_set = CMTG_CLK_SEL_A_DISABLED;
> > > +
> > > +                if (intel_cmtg_has_cmtg_b(display)) {
> > > +                        clk_sel_clr |= CMTG_CLK_SEL_B_MASK;
> > > +                        clk_sel_set |= CMTG_CLK_SEL_B_DISABLED;
> > > +                }
> > > +
> > > +                intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr,
> > > clk_sel_set);
> > > +        }
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > b/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > new file mode 100644
> > > index 000000000000..4dfd31906d81
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > @@ -0,0 +1,38 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright (C) 2024 Intel Corporation
> > > + */
> > > +
> > > +#ifndef __INTEL_CMTG_H__
> > > +#define __INTEL_CMTG_H__
> > > +
> > > +#include "intel_display_core.h"
> > > +#include "intel_global_state.h"
> > > +
> > > +/*
> > > + * We describe here only the minimum state required to allow us
> > > to
> > > properly
> > > + * disable the CMTG if necessary.
> > > + */
> > > +struct intel_cmtg_state {
> > > +        struct intel_global_state base;
> > > +
> > > +        bool cmtg_a_enable;
> > > +        /*
> > > +         * Xe3_LPD adds a second CMTG that can be used for dual
> > > eDP
> > > async mode.
> > > +         */
> > > +        bool cmtg_b_enable;
> > > +        bool trans_a_secondary;
> > > +        bool trans_b_secondary;
> > > +};
> > > +
> > > +#define to_intel_cmtg_state(global_state) \
> > > +        container_of_const((global_state), struct
> > > intel_cmtg_state,
> > > base)
> > > +
> > > +int intel_cmtg_init(struct intel_display *display);
> > > +void intel_cmtg_readout_state(struct intel_display *display,
> > > +                              struct intel_cmtg_state
> > > *cmtg_state);
> > > +int intel_cmtg_force_disabled(struct intel_atomic_state *state);
> > > +int intel_cmtg_atomic_check(struct intel_atomic_state *state);
> > > +void intel_cmtg_disable(struct intel_atomic_state *state);
> > > +
> > > +#endif /* __INTEL_CMTG_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > new file mode 100644
> > > index 000000000000..082f96cad284
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > @@ -0,0 +1,21 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright (C) 2024 Intel Corporation
> > > + */
> > > +
> > > +#ifndef __INTEL_CMTG_REGS_H__
> > > +#define __INTEL_CMTG_REGS_H__
> > > +
> > > +#include "i915_reg_defs.h"
> > > +
> > > +#define CMTG_CLK_SEL                        _MMIO(0x46160)
> > > +#define CMTG_CLK_SEL_A_MASK                REG_GENMASK(31, 29)
> > > +#define
> > > CMTG_CLK_SEL_A_DISABLED               
> > > REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
> > > +#define CMTG_CLK_SEL_B_MASK                REG_GENMASK(15, 13)
> > > +#define
> > > CMTG_CLK_SEL_B_DISABLED               
> > > REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
> > > +
> > > +#define TRANS_CMTG_CTL_A                _MMIO(0x6fa88)
> > > +#define TRANS_CMTG_CTL_B                _MMIO(0x6fb88)
> > > +#define  CMTG_ENABLE                        REG_BIT(31)
> > > +
> > > +#endif /* __INTEL_CMTG_REGS_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 4271da219b41..098985ad7ad4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -62,6 +62,7 @@
> > >  #include "intel_bw.h"
> > >  #include "intel_cdclk.h"
> > >  #include "intel_clock_gating.h"
> > > +#include "intel_cmtg.h"
> > >  #include "intel_color.h"
> > >  #include "intel_crt.h"
> > >  #include "intel_crtc.h"
> > > @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device
> > > *dev,
> > >          if (ret)
> > >                  goto fail;
> > >  
> > > +        ret = intel_cmtg_atomic_check(state);
> > > +        if (ret)
> > > +                goto fail;
> > > +
> > >          for_each_new_intel_crtc_in_state(state, crtc,
> > > new_crtc_state, i) {
> > >                  if (!intel_crtc_needs_modeset(new_crtc_state))
> > >                          continue;
> > > @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct
> > > intel_atomic_state *state)
> > >                         
> > > intel_modeset_get_crtc_power_domains(new_crt
> > > c_state, &put_domains[crtc->pipe]);
> > >          }
> > >  
> > > +        intel_cmtg_disable(state);
> > > +
> > >          intel_commit_modeset_disables(state);
> > >  
> > >          intel_dp_tunnel_atomic_alloc_bw(state);
> > > @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device
> > > *dev)
> > >                  }
> > >          }
> > >  
> > > +        ret =
> > > intel_cmtg_force_disabled(to_intel_atomic_state(state));
> > > +        if (ret)
> > > +                goto out;
> > > +
> > >          ret = drm_atomic_commit(state);
> > >  
> > >  out:
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > > index 554870d2494b..d0b039114e2d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > > @@ -354,6 +354,10 @@ struct intel_display {
> > >                  unsigned int skl_preferred_vco_freq;
> > >          } cdclk;
> > >  
> > > +        struct {
> > > +                struct intel_global_obj obj;
> > > +        } cmtg;
> > > +
> > >          struct {
> > >                  struct drm_property_blob
> > > *glk_linear_degamma_lut;
> > >          } color;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> > > b/drivers/gpu/drm/i915/display/intel_display_device.h
> > > index 9a333d9e6601..a126247eb6b8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > > @@ -145,6 +145,7 @@ struct intel_display_platforms {
> > >  #define HAS_BIGJOINER(__display)        (DISPLAY_VER(__display)
> > > >=
> > > 11 && HAS_DSC(__display))
> > >  #define HAS_CDCLK_CRAWL(__display)       
> > > (DISPLAY_INFO(__display)-
> > > > has_cdclk_crawl)
> > >  #define HAS_CDCLK_SQUASH(__display)       
> > > (DISPLAY_INFO(__display)-
> > > > has_cdclk_squash)
> > > +#define HAS_CMTG(__display)                (!(__display)-
> > > >platform.dg2
> > > && DISPLAY_VER(__display) >= 13)
> > >  #define HAS_CUR_FBC(__display)               
> > > (!HAS_GMCH(__display) &&
> > > IS_DISPLAY_VER(__display, 7, 13))
> > >  #define HAS_D12_PLANE_MINIMIZATION(__display)       
> > > ((__display)-
> > > > platform.rocketlake || (__display)->platform.alderlake_s)
> > >  #define
> > > HAS_DBUF_OVERLAP_DETECTION(__display)       
> > > (DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > b/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > index 497b4a1f045f..3e1483814e8d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > @@ -25,6 +25,7 @@
> > >  #include "intel_bios.h"
> > >  #include "intel_bw.h"
> > >  #include "intel_cdclk.h"
> > > +#include "intel_cmtg.h"
> > >  #include "intel_color.h"
> > >  #include "intel_crtc.h"
> > >  #include "intel_display_debugfs.h"
> > > @@ -269,6 +270,10 @@ int intel_display_driver_probe_noirq(struct
> > > intel_display *display)
> > >          if (ret)
> > >                  goto cleanup_vga_client_pw_domain_dmc;
> > >  
> > > +        ret = intel_cmtg_init(display);
> > > +        if (ret)
> > > +                goto cleanup_vga_client_pw_domain_dmc;
> > > +
> > >          intel_init_quirks(display);
> > >  
> > >          intel_fbc_init(display);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > index 9db30db428f7..737a43916ac5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > @@ -15,6 +15,7 @@
> > >  #include "i9xx_wm.h"
> > >  #include "intel_atomic.h"
> > >  #include "intel_bw.h"
> > > +#include "intel_cmtg.h"
> > >  #include "intel_color.h"
> > >  #include "intel_crtc.h"
> > >  #include "intel_crtc_state_dump.h"
> > > @@ -702,6 +703,8 @@ static void
> > > intel_modeset_readout_hw_state(struct
> > > drm_i915_private *i915)
> > >          struct intel_display *display = &i915->display;
> > >          struct intel_cdclk_state *cdclk_state =
> > >                  to_intel_cdclk_state(i915-
> > > >display.cdclk.obj.state);
> > > +        struct intel_cmtg_state *cmtg_state =
> > > +                to_intel_cmtg_state(display->cmtg.obj.state);
> > >          struct intel_dbuf_state *dbuf_state =
> > >                  to_intel_dbuf_state(i915-
> > > >display.dbuf.obj.state);
> > >          struct intel_pmdemand_state *pmdemand_state =
> > > @@ -906,6 +909,8 @@ static void
> > > intel_modeset_readout_hw_state(struct
> > > drm_i915_private *i915)
> > >          }
> > >  
> > >          intel_pmdemand_init_pmdemand_params(i915,
> > > pmdemand_state);
> > > +
> > > +        intel_cmtg_readout_state(display, cmtg_state);
> > >  }
> > >  
> > >  static void
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 765e6c0528fb..b34bccfb1ccc 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3565,6 +3565,7 @@ enum skl_power_gate {
> > >  #define _TRANS_DDI_FUNC_CTL2_DSI1        0x6bc04
> > >  #define TRANS_DDI_FUNC_CTL2(dev_priv,
> > > tran)        _MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
> > >  #define  PORT_SYNC_MODE_ENABLE                        REG_BIT(4)
> > > +#define  CMTG_SECONDARY_MODE                        REG_BIT(3)
> > >  #define  PORT_SYNC_MODE_MASTER_SELECT_MASK        REG_GENMASK(2,
> > > 0)
> > >  #define 
> > > PORT_SYNC_MODE_MASTER_SELECT(x)       
> > > REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK,(x))
> > >  
> > > diff --git a/drivers/gpu/drm/xe/Makefile
> > > b/drivers/gpu/drm/xe/Makefile
> > > index 5c97ad6ed738..cd0e25fce14b 100644
> > > --- a/drivers/gpu/drm/xe/Makefile
> > > +++ b/drivers/gpu/drm/xe/Makefile
> > > @@ -199,6 +199,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > >          i915-display/intel_bios.o \
> > >          i915-display/intel_bw.o \
> > >          i915-display/intel_cdclk.o \
> > > +        i915-display/intel_cmtg.o \
> > >          i915-display/intel_color.o \
> > >          i915-display/intel_combo_phy.o \
> > >          i915-display/intel_connector.o \
> >

Jani Nikula Jan. 3, 2025, 11:11 a.m. UTC | #8

On Tue, 31 Dec 2024, Gustavo Sousa <[e-mail]> wrote:
> Quoting Jani Nikula (2024-12-31 08:45:56-03:00)
>>On Tue, 24 Dec 2024, Gustavo Sousa <[e-mail]> wrote:
>>I understand you're following patterns from elsewhere in the driver. But
>
> Correct.
>
>>I've always wondered why we use a mixture of atomic state, global state,
>>and the specific (e.g. struct intel_cmtg_state) here. Makes no sense.
>>
>>I believe the specific global state structs should all be internal to
>>the implementation in the .c file, opaque outside, with accessor
>>functions. The to_intel_cmtg_state() should be a proper function
>>(although the constness handling may require a _Generic wrapper).
>
> Yeah. I agree that the specific state bits should be private to the
> implementation. Even the type "struct intel_cmtg_state" could be private
> and then we would have the exposed interface dealing with only "struct
> intel_global_state" or "struct intel_atomic_state" pointers.
>
> The only function that is currently asking for a struct intel_cmtg_state
> pointer is intel_cmtg_readout_state(), but that one can be easily
> changed to receive a pointer to struct intel_global_state instead (or
> even converted to be a function specific to display->cmtg.obj.state,
> dropping the state argument).
>
> With that, there would be no need to expose the struct intel_cmtg_state
> type anymore and it can be made entirely private to intel_cmtg.c.
>
> Let me know what you think.
>
>>
>>I actually have had patches to do this for all the global state stuff,
>>but they've conflicted and gone stale. It's hard when basically anyone
>>can just poke at the state when this shouldn't really be the case.
>
> We could maybe start with CMTG state and progressively converting the
> other guys?

I kind of jumped the gun with pmdemand that you already reviewed. That
could be the minimal direction here too. There's the to_intel_*_state()
but it could be an intermediate step towards the right direction. Could
do that here as well.

> (Although, after reading the entire review, if we decide to deal with
> the CMTG only as part of the sanitization, I guess implementing the
> whole global state "protocol" for the CMTG wouldn't make much sense
> anymore, right?).

I'll reply to this below.

>
>>
>>
>>> +
>>> +#endif /* __INTEL_CMTG_H__ */
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>> new file mode 100644
>>> index 000000000000..082f96cad284
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>> @@ -0,0 +1,21 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright (C) 2024 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __INTEL_CMTG_REGS_H__
>>> +#define __INTEL_CMTG_REGS_H__
>>> +
>>> +#include "i915_reg_defs.h"
>>> +
>>> +#define CMTG_CLK_SEL                        _MMIO(0x46160)
>>> +#define CMTG_CLK_SEL_A_MASK                REG_GENMASK(31, 29)
>>> +#define CMTG_CLK_SEL_A_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
>>> +#define CMTG_CLK_SEL_B_MASK                REG_GENMASK(15, 13)
>>> +#define CMTG_CLK_SEL_B_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
>>> +
>>> +#define TRANS_CMTG_CTL_A                _MMIO(0x6fa88)
>>> +#define TRANS_CMTG_CTL_B                _MMIO(0x6fb88)
>>
>>Could make those underscore prefixed, with a parametrized
>>TRANS_CMTG_CTL(idx).
>
> I had thought about that, but then decided to go with two separate
> defines.
>
> The main reason was because of the fact that the second instance was
> added to support the async dual eDP case and not necessarily to be a
> common "per pipe" resource like with other pipe/transcoder components.

Not insisting, not a huge deal.

>
>>
>>> +#define  CMTG_ENABLE                        REG_BIT(31)
>>> +
>>> +#endif /* __INTEL_CMTG_REGS_H__ */
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 4271da219b41..098985ad7ad4 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -62,6 +62,7 @@
>>>  #include "intel_bw.h"
>>>  #include "intel_cdclk.h"
>>>  #include "intel_clock_gating.h"
>>> +#include "intel_cmtg.h"
>>>  #include "intel_color.h"
>>>  #include "intel_crt.h"
>>>  #include "intel_crtc.h"
>>> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>>>          if (ret)
>>>                  goto fail;
>>>  
>>> +        ret = intel_cmtg_atomic_check(state);
>>> +        if (ret)
>>> +                goto fail;
>>> +
>>>          for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>>                  if (!intel_crtc_needs_modeset(new_crtc_state))
>>>                          continue;
>>> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>                          intel_modeset_get_crtc_power_domains(new_crtc_state, &put_domains[crtc->pipe]);
>>>          }
>>>  
>>> +        intel_cmtg_disable(state);
>>> +
>>>          intel_commit_modeset_disables(state);
>>>  
>>>          intel_dp_tunnel_atomic_alloc_bw(state);
>>> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device *dev)
>>>                  }
>>>          }
>>>  
>>> +        ret = intel_cmtg_force_disabled(to_intel_atomic_state(state));
>>> +        if (ret)
>>> +                goto out;
>>> +
>>
>>I think the usual way is to do foo_sanitize_state() at
>>intel_modeset_setup_hw_state().
>
> Hm... I see. In this case:
>
> - For Xe2_LPD and newer, we can simply disable the CMTG as part of the
>   sanitization;
> - For pre-Xe2_LPD displays, which would require a modeset when disabling
>   the CMTG, additionally force the CRTC to be disabled.
>
> Right?

I'm not sure what you mean by forcing the CRTC to be disabled, but I
think that's the general idea, yes.

> In this case, I guess implementing the whole "global state" protocol for
> the CMTG wouldn't make much sense if we are not going to handle the
> disabling as part of the initial commit. We could simply store the state
> in a "vanilla" struct (and it would be good if such struct lived only
> during the readout+sanitization time).

I think it all depends on what we'll need if/when we properly implement
CMTG support. If we're going to need global state for that, and you have
it written here, might just as well use it instead of throwing it in the
curb.

BR,
Jani.


>
>>
>>The above is incredibly specific to what intel_initial_commit()
>>does. There's nothing like that, it's a nice pure high level function
>>currently.
>>
>>>          ret = drm_atomic_commit(state);
>>>  
>>>  out:
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>>> index 554870d2494b..d0b039114e2d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>>> @@ -354,6 +354,10 @@ struct intel_display {
>>>                  unsigned int skl_preferred_vco_freq;
>>>          } cdclk;
>>>  
>>> +        struct {
>>> +                struct intel_global_obj obj;
>>> +        } cmtg;
>>> +
>>>          struct {
>>>                  struct drm_property_blob *glk_linear_degamma_lut;
>>>          } color;
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>>> index 9a333d9e6601..a126247eb6b8 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>>> @@ -145,6 +145,7 @@ struct intel_display_platforms {
>>>  #define HAS_BIGJOINER(__display)        (DISPLAY_VER(__display) >= 11 && HAS_DSC(__display))
>>>  #define HAS_CDCLK_CRAWL(__display)        (DISPLAY_INFO(__display)->has_cdclk_crawl)
>>>  #define HAS_CDCLK_SQUASH(__display)        (DISPLAY_INFO(__display)->has_cdclk_squash)
>>> +#define HAS_CMTG(__display)                (!(__display)->platform.dg2 && DISPLAY_VER(__display) >= 13)
>>>  #define HAS_CUR_FBC(__display)                (!HAS_GMCH(__display) && IS_DISPLAY_VER(__display, 7, 13))
>>>  #define HAS_D12_PLANE_MINIMIZATION(__display)        ((__display)->platform.rocketlake || (__display)->platform.alderlake_s)
>>>  #define HAS_DBUF_OVERLAP_DETECTION(__display)        (DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> index 497b4a1f045f..3e1483814e8d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> @@ -25,6 +25,7 @@
>>>  #include "intel_bios.h"
>>>  #include "intel_bw.h"
>>>  #include "intel_cdclk.h"
>>> +#include "intel_cmtg.h"
>>>  #include "intel_color.h"
>>>  #include "intel_crtc.h"
>>>  #include "intel_display_debugfs.h"
>>> @@ -269,6 +270,10 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>>>          if (ret)
>>>                  goto cleanup_vga_client_pw_domain_dmc;
>>>  
>>> +        ret = intel_cmtg_init(display);
>>> +        if (ret)
>>> +                goto cleanup_vga_client_pw_domain_dmc;
>>> +
>>>          intel_init_quirks(display);
>>>  
>>>          intel_fbc_init(display);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>>> index 9db30db428f7..737a43916ac5 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>>> @@ -15,6 +15,7 @@
>>>  #include "i9xx_wm.h"
>>>  #include "intel_atomic.h"
>>>  #include "intel_bw.h"
>>> +#include "intel_cmtg.h"
>>>  #include "intel_color.h"
>>>  #include "intel_crtc.h"
>>>  #include "intel_crtc_state_dump.h"
>>> @@ -702,6 +703,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
>>>          struct intel_display *display = &i915->display;
>>>          struct intel_cdclk_state *cdclk_state =
>>>                  to_intel_cdclk_state(i915->display.cdclk.obj.state);
>>> +        struct intel_cmtg_state *cmtg_state =
>>> +                to_intel_cmtg_state(display->cmtg.obj.state);
>>>          struct intel_dbuf_state *dbuf_state =
>>>                  to_intel_dbuf_state(i915->display.dbuf.obj.state);
>>>          struct intel_pmdemand_state *pmdemand_state =
>>> @@ -906,6 +909,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
>>>          }
>>>  
>>>          intel_pmdemand_init_pmdemand_params(i915, pmdemand_state);
>>> +
>>> +        intel_cmtg_readout_state(display, cmtg_state);
>>>  }
>>>  
>>>  static void
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 765e6c0528fb..b34bccfb1ccc 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -3565,6 +3565,7 @@ enum skl_power_gate {
>>>  #define _TRANS_DDI_FUNC_CTL2_DSI1        0x6bc04
>>>  #define TRANS_DDI_FUNC_CTL2(dev_priv, tran)        _MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
>>>  #define  PORT_SYNC_MODE_ENABLE                        REG_BIT(4)
>>> +#define  CMTG_SECONDARY_MODE                        REG_BIT(3)
>>>  #define  PORT_SYNC_MODE_MASTER_SELECT_MASK        REG_GENMASK(2, 0)
>>>  #define  PORT_SYNC_MODE_MASTER_SELECT(x)        REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK, (x))
>>>  
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 5c97ad6ed738..cd0e25fce14b 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -199,6 +199,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>>>          i915-display/intel_bios.o \
>>>          i915-display/intel_bw.o \
>>>          i915-display/intel_cdclk.o \
>>> +        i915-display/intel_cmtg.o \
>>>          i915-display/intel_color.o \
>>>          i915-display/intel_combo_phy.o \
>>>          i915-display/intel_connector.o \
>>
>>-- 
>>Jani Nikula, Intel

Gustavo Sousa Jan. 3, 2025, 11:57 a.m. UTC | #9

Quoting Jani Nikula (2025-01-03 08:11:59-03:00)
>On Tue, 31 Dec 2024, Gustavo Sousa <[e-mail]> wrote:
>> Quoting Jani Nikula (2024-12-31 08:45:56-03:00)
>>>On Tue, 24 Dec 2024, Gustavo Sousa <[e-mail]> wrote:
>>>I understand you're following patterns from elsewhere in the driver. But
>>
>> Correct.
>>
>>>I've always wondered why we use a mixture of atomic state, global state,
>>>and the specific (e.g. struct intel_cmtg_state) here. Makes no sense.
>>>
>>>I believe the specific global state structs should all be internal to
>>>the implementation in the .c file, opaque outside, with accessor
>>>functions. The to_intel_cmtg_state() should be a proper function
>>>(although the constness handling may require a _Generic wrapper).
>>
>> Yeah. I agree that the specific state bits should be private to the
>> implementation. Even the type "struct intel_cmtg_state" could be private
>> and then we would have the exposed interface dealing with only "struct
>> intel_global_state" or "struct intel_atomic_state" pointers.
>>
>> The only function that is currently asking for a struct intel_cmtg_state
>> pointer is intel_cmtg_readout_state(), but that one can be easily
>> changed to receive a pointer to struct intel_global_state instead (or
>> even converted to be a function specific to display->cmtg.obj.state,
>> dropping the state argument).
>>
>> With that, there would be no need to expose the struct intel_cmtg_state
>> type anymore and it can be made entirely private to intel_cmtg.c.
>>
>> Let me know what you think.
>>
>>>
>>>I actually have had patches to do this for all the global state stuff,
>>>but they've conflicted and gone stale. It's hard when basically anyone
>>>can just poke at the state when this shouldn't really be the case.
>>
>> We could maybe start with CMTG state and progressively converting the
>> other guys?
>
>I kind of jumped the gun with pmdemand that you already reviewed. That
>could be the minimal direction here too. There's the to_intel_*_state()
>but it could be an intermediate step towards the right direction. Could
>do that here as well.
>
>> (Although, after reading the entire review, if we decide to deal with
>> the CMTG only as part of the sanitization, I guess implementing the
>> whole global state "protocol" for the CMTG wouldn't make much sense
>> anymore, right?).
>
>I'll reply to this below.
>
>>
>>>
>>>
>>>> +
>>>> +#endif /* __INTEL_CMTG_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>>> new file mode 100644
>>>> index 000000000000..082f96cad284
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>>> @@ -0,0 +1,21 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright (C) 2024 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef __INTEL_CMTG_REGS_H__
>>>> +#define __INTEL_CMTG_REGS_H__
>>>> +
>>>> +#include "i915_reg_defs.h"
>>>> +
>>>> +#define CMTG_CLK_SEL                        _MMIO(0x46160)
>>>> +#define CMTG_CLK_SEL_A_MASK                REG_GENMASK(31, 29)
>>>> +#define CMTG_CLK_SEL_A_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
>>>> +#define CMTG_CLK_SEL_B_MASK                REG_GENMASK(15, 13)
>>>> +#define CMTG_CLK_SEL_B_DISABLED                REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
>>>> +
>>>> +#define TRANS_CMTG_CTL_A                _MMIO(0x6fa88)
>>>> +#define TRANS_CMTG_CTL_B                _MMIO(0x6fb88)
>>>
>>>Could make those underscore prefixed, with a parametrized
>>>TRANS_CMTG_CTL(idx).
>>
>> I had thought about that, but then decided to go with two separate
>> defines.
>>
>> The main reason was because of the fact that the second instance was
>> added to support the async dual eDP case and not necessarily to be a
>> common "per pipe" resource like with other pipe/transcoder components.
>
>Not insisting, not a huge deal.
>
>>
>>>
>>>> +#define  CMTG_ENABLE                        REG_BIT(31)
>>>> +
>>>> +#endif /* __INTEL_CMTG_REGS_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>> index 4271da219b41..098985ad7ad4 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -62,6 +62,7 @@
>>>>  #include "intel_bw.h"
>>>>  #include "intel_cdclk.h"
>>>>  #include "intel_clock_gating.h"
>>>> +#include "intel_cmtg.h"
>>>>  #include "intel_color.h"
>>>>  #include "intel_crt.h"
>>>>  #include "intel_crtc.h"
>>>> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>>>>          if (ret)
>>>>                  goto fail;
>>>>  
>>>> +        ret = intel_cmtg_atomic_check(state);
>>>> +        if (ret)
>>>> +                goto fail;
>>>> +
>>>>          for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>>>                  if (!intel_crtc_needs_modeset(new_crtc_state))
>>>>                          continue;
>>>> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>>                          intel_modeset_get_crtc_power_domains(new_crtc_state, &put_domains[crtc->pipe]);
>>>>          }
>>>>  
>>>> +        intel_cmtg_disable(state);
>>>> +
>>>>          intel_commit_modeset_disables(state);
>>>>  
>>>>          intel_dp_tunnel_atomic_alloc_bw(state);
>>>> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device *dev)
>>>>                  }
>>>>          }
>>>>  
>>>> +        ret = intel_cmtg_force_disabled(to_intel_atomic_state(state));
>>>> +        if (ret)
>>>> +                goto out;
>>>> +
>>>
>>>I think the usual way is to do foo_sanitize_state() at
>>>intel_modeset_setup_hw_state().
>>
>> Hm... I see. In this case:
>>
>> - For Xe2_LPD and newer, we can simply disable the CMTG as part of the
>>   sanitization;
>> - For pre-Xe2_LPD displays, which would require a modeset when disabling
>>   the CMTG, additionally force the CRTC to be disabled.
>>
>> Right?
>
>I'm not sure what you mean by forcing the CRTC to be disabled, but I
>think that's the general idea, yes.

What I thought was that I would have a intel_cmtg_sanitize_state() that
would disable the CMTG.

For platforms pre-LNL (i.e. display pre-Xe2_LPD), disabling the CMTG
requires a modeset. In this case, I was thinking about
intel_cmtg_sanitize_state() returning a mask of pipes that need to be
disabled as part of the sanitization.

I see that intel_sanitize_crtc() disables an active pipe if it does not
have an active connector. So such a mask could be passed to
intel_sanitize_all_crtcs().

Unless there is a better way... Is there a way of flagging that some
pipe needs a modeset during the sanitization step?

>
>> In this case, I guess implementing the whole "global state" protocol for
>> the CMTG wouldn't make much sense if we are not going to handle the
>> disabling as part of the initial commit. We could simply store the state
>> in a "vanilla" struct (and it would be good if such struct lived only
>> during the readout+sanitization time).
>
>I think it all depends on what we'll need if/when we properly implement
>CMTG support. If we're going to need global state for that, and you have
>it written here, might just as well use it instead of throwing it in the
>curb.

Okay. Sounds good.

I guess we could keep the state strucutures, but then, with the CMTG
being disabled during sanitization, we would not need to have those
get/get_new/get_old functions. Probably just the ones for duplicating
and freeing, for consistency.

--
Gustavo Sousa

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 3dda9f0eda82..7e7414453765 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -231,6 +231,7 @@  i915-y += \
 	display/intel_bo.o \
 	display/intel_bw.o \
 	display/intel_cdclk.o \
+	display/intel_cmtg.o \
 	display/intel_color.o \
 	display/intel_combo_phy.o \
 	display/intel_connector.o \
diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c b/drivers/gpu/drm/i915/display/intel_cmtg.c
new file mode 100644
index 000000000000..27491497f515
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
@@ -0,0 +1,311 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright (C) 2024 Intel Corporation
+ */
+
+#include <linux/string.h>
+#include <linux/string_choices.h>
+#include <linux/types.h>
+
+#include "i915_drv.h"
+#include "i915_reg.h"
+#include "intel_crtc.h"
+#include "intel_de.h"
+#include "intel_cmtg.h"
+#include "intel_cmtg_regs.h"
+#include "intel_display_device.h"
+#include "intel_display_types.h"
+
+/**
+ * DOC: Common Primary Timing Generator (CMTG)
+ *
+ * The CMTG is a timing generator that runs in parallel to transcoders timing
+ * generators (TG) to provide a synchronization mechanism where CMTG acts as
+ * primary and transcoders TGs act as secondary to the CMTG. The CMTG outputs
+ * its TG start and frame sync signals to the transcoders that are configured
+ * as secondary, which use those signals to synchronize their own timing with
+ * the CMTG's.
+ *
+ * The CMTG can be used only with eDP or MIPI command mode and supports the
+ * following use cases:
+ *
+ * - Dual eDP: The CMTG can be used to keep two eDP TGs in sync when on a
+ *   dual eDP configuration (with or without PSR/PSR2 enabled).
+ *
+ * - Single eDP as secondary: It is also possible to use a single eDP
+ *   configuration with the transcoder TG as secondary to the CMTG. That would
+ *   allow a flow that would not require a modeset on the existing eDP when a
+ *   new eDP is added for a dual eDP configuration with CMTG.
+ *
+ * - DC6v: In DC6v, the transcoder might be off but the CMTG keeps running to
+ *   maintain frame timings. When exiting DC6v, the transcoder TG then is
+ *   synced back the CMTG.
+ *
+ * Currently, the driver does not use the CMTG, but we need to make sure that
+ * we disable it in case we inherit a display configuration with it enabled.
+ */
+
+static struct intel_global_state *
+intel_cmtg_duplicate_state(struct intel_global_obj *obj)
+{
+	struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(obj->state);
+
+	cmtg_state = kmemdup(cmtg_state, sizeof(*cmtg_state), GFP_KERNEL);
+	if (!cmtg_state)
+		return NULL;
+
+	return &cmtg_state->base;
+}
+
+static void intel_cmtg_destroy_state(struct intel_global_obj *obj,
+				     struct intel_global_state *state)
+{
+	kfree(state);
+}
+
+static const struct intel_global_state_funcs intel_cmtg_state_funcs = {
+	.atomic_duplicate_state = intel_cmtg_duplicate_state,
+	.atomic_destroy_state = intel_cmtg_destroy_state,
+};
+
+static bool intel_cmtg_has_cmtg_b(struct intel_display *display)
+{
+	return DISPLAY_VER(display) >= 20;
+}
+
+static bool intel_cmtg_has_clock_sel(struct intel_display *display)
+{
+	return DISPLAY_VER(display) >= 14;
+}
+
+static bool intel_cmtg_requires_modeset(struct intel_display *display)
+{
+	return DISPLAY_VER(display) < 20;
+}
+
+static void intel_cmtg_dump_state(struct intel_display *display,
+				  struct intel_cmtg_state *cmtg_state)
+{
+	if (intel_cmtg_has_cmtg_b(display))
+		drm_dbg_kms(display->drm,
+			    "CMTG state readout: CMTG A: %s, CMTG B: %s, transcoder A secondary: %s, transcoder B secondary: %s\n",
+			    str_enabled_disabled(cmtg_state->cmtg_a_enable),
+			    str_enabled_disabled(cmtg_state->cmtg_b_enable),
+			    str_yes_no(cmtg_state->trans_a_secondary),
+			    str_yes_no(cmtg_state->trans_b_secondary));
+	else
+		drm_dbg_kms(display->drm,
+			    "CMTG state readout: %s, Transcoder A secondary: %s, Transcoder B secondary: %s\n",
+			    str_enabled_disabled(cmtg_state->cmtg_a_enable),
+			    str_yes_no(cmtg_state->trans_a_secondary),
+			    str_yes_no(cmtg_state->trans_b_secondary));
+}
+
+int intel_cmtg_init(struct intel_display *display)
+{
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	struct intel_cmtg_state *cmtg_state;
+
+	cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL);
+	if (!cmtg_state)
+		return -ENOMEM;
+
+	intel_atomic_global_obj_init(i915, &display->cmtg.obj,
+				     &cmtg_state->base,
+				     &intel_cmtg_state_funcs);
+
+	return 0;
+}
+
+void intel_cmtg_readout_state(struct intel_display *display,
+			      struct intel_cmtg_state *cmtg_state)
+{
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	u32 val;
+
+	if (!HAS_CMTG(display))
+		return;
+
+	val = intel_de_read(display, TRANS_CMTG_CTL_A);
+	cmtg_state->cmtg_a_enable = val & CMTG_ENABLE;
+
+	if (intel_cmtg_has_cmtg_b(display)) {
+		val = intel_de_read(display, TRANS_CMTG_CTL_B);
+		cmtg_state->cmtg_b_enable = val & CMTG_ENABLE;
+	}
+
+	if (intel_crtc_for_pipe(display, PIPE_A)) {
+		val = intel_de_read(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A));
+		cmtg_state->trans_a_secondary = val & CMTG_SECONDARY_MODE;
+	}
+
+	if (intel_crtc_for_pipe(display, PIPE_B)) {
+		val = intel_de_read(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B));
+		cmtg_state->trans_b_secondary = val & CMTG_SECONDARY_MODE;
+	}
+
+	intel_cmtg_dump_state(display, cmtg_state);
+}
+
+static struct intel_cmtg_state *
+intel_atomic_get_cmtg_state(struct intel_atomic_state *state)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct intel_global_state *obj_state =
+		intel_atomic_get_global_obj_state(state,
+						  &display->cmtg.obj);
+
+	if (IS_ERR(obj_state))
+		return ERR_CAST(obj_state);
+
+	return to_intel_cmtg_state(obj_state);
+}
+
+static struct intel_cmtg_state *
+intel_atomic_get_old_cmtg_state(struct intel_atomic_state *state)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct intel_global_state *obj_state =
+		intel_atomic_get_old_global_obj_state(state,
+						      &display->cmtg.obj);
+
+	if (!obj_state)
+		return NULL;
+
+	return to_intel_cmtg_state(obj_state);
+}
+
+static struct intel_cmtg_state *
+intel_atomic_get_new_cmtg_state(struct intel_atomic_state *state)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct intel_global_state *obj_state =
+		intel_atomic_get_new_global_obj_state(state,
+						      &display->cmtg.obj);
+
+	if (!obj_state)
+		return NULL;
+
+	return to_intel_cmtg_state(obj_state);
+}
+
+static bool intel_cmtg_state_changed(struct intel_cmtg_state *old_cmtg_state,
+				     struct intel_cmtg_state *new_cmtg_state)
+{
+	if (!new_cmtg_state)
+		return false;
+
+	return old_cmtg_state->cmtg_a_enable != new_cmtg_state->cmtg_a_enable ||
+		old_cmtg_state->cmtg_b_enable != new_cmtg_state->cmtg_b_enable ||
+		old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary ||
+		old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary;
+}
+
+static int intel_cmtg_check_modeset(struct intel_atomic_state *state,
+				    struct intel_cmtg_state *old_cmtg_state,
+				    struct intel_cmtg_state *new_cmtg_state)
+{
+	struct intel_display *display = to_intel_display(state);
+	u8 pipe_mask;
+
+	if (!intel_cmtg_requires_modeset(display))
+		return 0;
+
+	pipe_mask = 0;
+
+	if (old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary)
+		pipe_mask |= BIT(PIPE_A);
+
+	if (old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary)
+		pipe_mask |= BIT(PIPE_B);
+
+	if (!pipe_mask)
+		return 0;
+
+	return intel_modeset_pipes_in_mask_early(state, "updating CMTG config", pipe_mask);
+}
+
+int intel_cmtg_force_disabled(struct intel_atomic_state *state)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct intel_cmtg_state *new_cmtg_state;
+
+	if (!HAS_CMTG(display))
+		return 0;
+
+	new_cmtg_state = intel_atomic_get_cmtg_state(state);
+	if (IS_ERR(new_cmtg_state))
+		return PTR_ERR(new_cmtg_state);
+
+	new_cmtg_state->cmtg_a_enable = false;
+	new_cmtg_state->cmtg_b_enable = false;
+	new_cmtg_state->trans_a_secondary = false;
+	new_cmtg_state->trans_b_secondary = false;
+	return 0;
+}
+
+int intel_cmtg_atomic_check(struct intel_atomic_state *state)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct intel_cmtg_state *old_cmtg_state;
+	struct intel_cmtg_state *new_cmtg_state;
+	int ret;
+
+	if (!HAS_CMTG(display))
+		return 0;
+
+	old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
+	new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
+	if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state))
+		return 0;
+
+	ret = intel_cmtg_check_modeset(state, old_cmtg_state, new_cmtg_state);
+	if (ret)
+		return ret;
+
+	return intel_atomic_serialize_global_state(&new_cmtg_state->base);
+}
+
+/*
+ * Access to CMTG registers require the PHY PLL that provides its clock to be
+ * running (which is configured via CMTG_CLK_SEL). As such, this function needs
+ * to be called before intel_commit_modeset_disables() to ensure that the PHY
+ * PLL is still enabled when doing this.
+ */
+void intel_cmtg_disable(struct intel_atomic_state *state)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	struct intel_cmtg_state *old_cmtg_state;
+	struct intel_cmtg_state *new_cmtg_state;
+
+	if (!HAS_CMTG(display))
+		return;
+
+	old_cmtg_state = intel_atomic_get_old_cmtg_state(state);
+	new_cmtg_state = intel_atomic_get_new_cmtg_state(state);
+	if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state))
+		return;
+
+	drm_info(display->drm, "Disabling CMTG\n");
+
+	intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_A), CMTG_SECONDARY_MODE, 0);
+	intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(i915, TRANSCODER_B), CMTG_SECONDARY_MODE, 0);
+
+	intel_de_rmw(display, TRANS_CMTG_CTL_A, CMTG_ENABLE, 0);
+
+	if (intel_cmtg_has_cmtg_b(display))
+		intel_de_rmw(display, TRANS_CMTG_CTL_B, CMTG_ENABLE, 0);
+
+	if (intel_cmtg_has_clock_sel(display)) {
+		u32 clk_sel_clr = CMTG_CLK_SEL_A_MASK;
+		u32 clk_sel_set = CMTG_CLK_SEL_A_DISABLED;
+
+		if (intel_cmtg_has_cmtg_b(display)) {
+			clk_sel_clr |= CMTG_CLK_SEL_B_MASK;
+			clk_sel_set |= CMTG_CLK_SEL_B_DISABLED;
+		}
+
+		intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr, clk_sel_set);
+	}
+}
diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h b/drivers/gpu/drm/i915/display/intel_cmtg.h
new file mode 100644
index 000000000000..4dfd31906d81
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
@@ -0,0 +1,38 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_CMTG_H__
+#define __INTEL_CMTG_H__
+
+#include "intel_display_core.h"
+#include "intel_global_state.h"
+
+/*
+ * We describe here only the minimum state required to allow us to properly
+ * disable the CMTG if necessary.
+ */
+struct intel_cmtg_state {
+	struct intel_global_state base;
+
+	bool cmtg_a_enable;
+	/*
+	 * Xe3_LPD adds a second CMTG that can be used for dual eDP async mode.
+	 */
+	bool cmtg_b_enable;
+	bool trans_a_secondary;
+	bool trans_b_secondary;
+};
+
+#define to_intel_cmtg_state(global_state) \
+	container_of_const((global_state), struct intel_cmtg_state, base)
+
+int intel_cmtg_init(struct intel_display *display);
+void intel_cmtg_readout_state(struct intel_display *display,
+			      struct intel_cmtg_state *cmtg_state);
+int intel_cmtg_force_disabled(struct intel_atomic_state *state);
+int intel_cmtg_atomic_check(struct intel_atomic_state *state);
+void intel_cmtg_disable(struct intel_atomic_state *state);
+
+#endif /* __INTEL_CMTG_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
new file mode 100644
index 000000000000..082f96cad284
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
@@ -0,0 +1,21 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_CMTG_REGS_H__
+#define __INTEL_CMTG_REGS_H__
+
+#include "i915_reg_defs.h"
+
+#define CMTG_CLK_SEL			_MMIO(0x46160)
+#define CMTG_CLK_SEL_A_MASK		REG_GENMASK(31, 29)
+#define CMTG_CLK_SEL_A_DISABLED		REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
+#define CMTG_CLK_SEL_B_MASK		REG_GENMASK(15, 13)
+#define CMTG_CLK_SEL_B_DISABLED		REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
+
+#define TRANS_CMTG_CTL_A		_MMIO(0x6fa88)
+#define TRANS_CMTG_CTL_B		_MMIO(0x6fb88)
+#define  CMTG_ENABLE			REG_BIT(31)
+
+#endif /* __INTEL_CMTG_REGS_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4271da219b41..098985ad7ad4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -62,6 +62,7 @@ 
 #include "intel_bw.h"
 #include "intel_cdclk.h"
 #include "intel_clock_gating.h"
+#include "intel_cmtg.h"
 #include "intel_color.h"
 #include "intel_crt.h"
 #include "intel_crtc.h"
@@ -6828,6 +6829,10 @@  int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	ret = intel_cmtg_atomic_check(state);
+	if (ret)
+		goto fail;
+
 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
 		if (!intel_crtc_needs_modeset(new_crtc_state))
 			continue;
@@ -7757,6 +7762,8 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 			intel_modeset_get_crtc_power_domains(new_crtc_state, &put_domains[crtc->pipe]);
 	}
 
+	intel_cmtg_disable(state);
+
 	intel_commit_modeset_disables(state);
 
 	intel_dp_tunnel_atomic_alloc_bw(state);
@@ -8582,6 +8589,10 @@  int intel_initial_commit(struct drm_device *dev)
 		}
 	}
 
+	ret = intel_cmtg_force_disabled(to_intel_atomic_state(state));
+	if (ret)
+		goto out;
+
 	ret = drm_atomic_commit(state);
 
 out:
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index 554870d2494b..d0b039114e2d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -354,6 +354,10 @@  struct intel_display {
 		unsigned int skl_preferred_vco_freq;
 	} cdclk;
 
+	struct {
+		struct intel_global_obj obj;
+	} cmtg;
+
 	struct {
 		struct drm_property_blob *glk_linear_degamma_lut;
 	} color;
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 9a333d9e6601..a126247eb6b8 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -145,6 +145,7 @@  struct intel_display_platforms {
 #define HAS_BIGJOINER(__display)	(DISPLAY_VER(__display) >= 11 && HAS_DSC(__display))
 #define HAS_CDCLK_CRAWL(__display)	(DISPLAY_INFO(__display)->has_cdclk_crawl)
 #define HAS_CDCLK_SQUASH(__display)	(DISPLAY_INFO(__display)->has_cdclk_squash)
+#define HAS_CMTG(__display)		(!(__display)->platform.dg2 && DISPLAY_VER(__display) >= 13)
 #define HAS_CUR_FBC(__display)		(!HAS_GMCH(__display) && IS_DISPLAY_VER(__display, 7, 13))
 #define HAS_D12_PLANE_MINIMIZATION(__display)	((__display)->platform.rocketlake || (__display)->platform.alderlake_s)
 #define HAS_DBUF_OVERLAP_DETECTION(__display)	(DISPLAY_RUNTIME_INFO(__display)->has_dbuf_overlap_detection)
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 497b4a1f045f..3e1483814e8d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -25,6 +25,7 @@ 
 #include "intel_bios.h"
 #include "intel_bw.h"
 #include "intel_cdclk.h"
+#include "intel_cmtg.h"
 #include "intel_color.h"
 #include "intel_crtc.h"
 #include "intel_display_debugfs.h"
@@ -269,6 +270,10 @@  int intel_display_driver_probe_noirq(struct intel_display *display)
 	if (ret)
 		goto cleanup_vga_client_pw_domain_dmc;
 
+	ret = intel_cmtg_init(display);
+	if (ret)
+		goto cleanup_vga_client_pw_domain_dmc;
+
 	intel_init_quirks(display);
 
 	intel_fbc_init(display);
diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
index 9db30db428f7..737a43916ac5 100644
--- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
+++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
@@ -15,6 +15,7 @@ 
 #include "i9xx_wm.h"
 #include "intel_atomic.h"
 #include "intel_bw.h"
+#include "intel_cmtg.h"
 #include "intel_color.h"
 #include "intel_crtc.h"
 #include "intel_crtc_state_dump.h"
@@ -702,6 +703,8 @@  static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
 	struct intel_display *display = &i915->display;
 	struct intel_cdclk_state *cdclk_state =
 		to_intel_cdclk_state(i915->display.cdclk.obj.state);
+	struct intel_cmtg_state *cmtg_state =
+		to_intel_cmtg_state(display->cmtg.obj.state);
 	struct intel_dbuf_state *dbuf_state =
 		to_intel_dbuf_state(i915->display.dbuf.obj.state);
 	struct intel_pmdemand_state *pmdemand_state =
@@ -906,6 +909,8 @@  static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
 	}
 
 	intel_pmdemand_init_pmdemand_params(i915, pmdemand_state);
+
+	intel_cmtg_readout_state(display, cmtg_state);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 765e6c0528fb..b34bccfb1ccc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3565,6 +3565,7 @@  enum skl_power_gate {
 #define _TRANS_DDI_FUNC_CTL2_DSI1	0x6bc04
 #define TRANS_DDI_FUNC_CTL2(dev_priv, tran)	_MMIO_TRANS2(dev_priv, tran, _TRANS_DDI_FUNC_CTL2_A)
 #define  PORT_SYNC_MODE_ENABLE			REG_BIT(4)
+#define  CMTG_SECONDARY_MODE			REG_BIT(3)
 #define  PORT_SYNC_MODE_MASTER_SELECT_MASK	REG_GENMASK(2, 0)
 #define  PORT_SYNC_MODE_MASTER_SELECT(x)	REG_FIELD_PREP(PORT_SYNC_MODE_MASTER_SELECT_MASK, (x))
 
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 5c97ad6ed738..cd0e25fce14b 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -199,6 +199,7 @@  xe-$(CONFIG_DRM_XE_DISPLAY) += \
 	i915-display/intel_bios.o \
 	i915-display/intel_bw.o \
 	i915-display/intel_cdclk.o \
+	i915-display/intel_cmtg.o \
 	i915-display/intel_color.o \
 	i915-display/intel_combo_phy.o \
 	i915-display/intel_connector.o \