Message ID | 20250113204815.114019-1-gustavo.sousa@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v5] drm/i915/cmtg: Disable the CMTG | expand |
On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. Doesn't this patch disable the CRTC, not the CMTG? Can we switch off CMTG without a modeset? If not, I think we'd need to force a modeset for takeover. BR, Jani. > > 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. > v4: > - Avoid if/else duplication in intel_cmtg_dump_state() by using "n/a" > for CMTG B enabled/disabled string for platforms without it. (Jani) > - Prefer intel_cmtg_readout_hw_state() over intel_cmtg_readout_state(). > (Jani) > - Use display struct instead of i915 as first parameter for > TRANS_DDI_FUNC_CTL2(). (Jani) > - Fewer continuation lines in variable declaration/initialization for > better readability. (Jani) > - Coding style improvements. (Jani) > - Use drm_dbg_kms() instead of drm_info() for logging the disabling > of the CMTG. > - Make struct intel_cmtg_state entirely private to intel_cmtg.c. > v5: > - Do the disable sequence as part of the sanitization step after > hardware readout instead of initial modeset commit. (Jani) > - Adapt to commit 15133582465f ("drm/i915/display: convert global state > to struct intel_display") by using a display struct instead of i915 > as argument for intel_atomic_global_obj_init(). > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/display/intel_cmtg.c | 250 ++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_cmtg.h | 18 ++ > .../gpu/drm/i915/display/intel_cmtg_regs.h | 21 ++ > .../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 | 20 +- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/xe/Makefile | 1 + > 10 files changed, 317 insertions(+), 5 deletions(-) > 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..6ce8b979009a > --- /dev/null > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c > @@ -0,0 +1,250 @@ > +// 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_cmtg.h" > +#include "intel_cmtg_regs.h" > +#include "intel_de.h" > +#include "intel_display_device.h" > +#include "intel_display_types.h" > +#include "intel_global_state.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. > + */ > + > +/* > + * 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; > +}; > + > +static struct intel_cmtg_state *to_intel_cmtg_state(struct intel_global_state *obj_state) > +{ > + return container_of(obj_state, struct intel_cmtg_state, base); > +} > + > +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) > +{ > + 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), > + intel_cmtg_has_cmtg_b(display) ? str_enabled_disabled(cmtg_state->cmtg_b_enable) : "n/a", > + 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 intel_cmtg_state *cmtg_state; > + > + cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL); > + if (!cmtg_state) > + return -ENOMEM; > + > + intel_atomic_global_obj_init(display, &display->cmtg.obj, > + &cmtg_state->base, > + &intel_cmtg_state_funcs); > + > + return 0; > +} > + > +void intel_cmtg_readout_hw_state(struct intel_display *display) > +{ > + struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(display->cmtg.obj.state); > + 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(display, 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(display, TRANSCODER_B)); > + cmtg_state->trans_b_secondary = val & CMTG_SECONDARY_MODE; > + } > + > + intel_cmtg_dump_state(display, cmtg_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 void intel_cmtg_state_set_disabled(struct intel_cmtg_state *cmtg_state) > +{ > + cmtg_state->cmtg_a_enable = false; > + cmtg_state->cmtg_b_enable = false; > + cmtg_state->trans_a_secondary = false; > + cmtg_state->trans_b_secondary = false; > +} > + > +static void intel_cmtg_disable(struct intel_display *display, > + struct intel_cmtg_state *old_cmtg_state, > + struct intel_cmtg_state *new_cmtg_state) > +{ > + if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state)) > + return; > + > + drm_dbg_kms(display->drm, "Disabling CMTG\n"); > + > + intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(display, TRANSCODER_A), CMTG_SECONDARY_MODE, 0); > + intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(display, 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); > + } > +} > + > +static u32 intel_cmtg_modeset_crtc_mask(struct intel_display *display, > + struct intel_cmtg_state *old_cmtg_state, > + struct intel_cmtg_state *new_cmtg_state) > +{ > + u32 crtc_mask; > + > + if (intel_cmtg_requires_modeset(display)) > + return 0; > + > + crtc_mask = 0; > + > + if (old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary) > + crtc_mask |= drm_crtc_mask(&intel_crtc_for_pipe(display, PIPE_A)->base); > + > + if (old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary) > + crtc_mask |= drm_crtc_mask(&intel_crtc_for_pipe(display, PIPE_B)->base); > + > + return crtc_mask; > +} > + > +/* > + * Disable CMTG if enabled and return a mask of pipes that need to be disabled > + * (for platforms where disabling the CMTG requires a modeset). > + */ > +u32 intel_cmtg_sanitize_state(struct intel_display *display) > +{ > + struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(display->cmtg.obj.state); > + struct intel_cmtg_state old_cmtg_state; > + > + if (!HAS_CMTG(display)) > + return 0; > + > + old_cmtg_state = *cmtg_state; > + intel_cmtg_state_set_disabled(cmtg_state); > + intel_cmtg_disable(display, &old_cmtg_state, cmtg_state); > + > + return intel_cmtg_modeset_crtc_mask(display, &old_cmtg_state, cmtg_state); > +} > 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..3c51e144aa3f > --- /dev/null > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright (C) 2024 Intel Corporation > + */ > + > +#ifndef __INTEL_CMTG_H__ > +#define __INTEL_CMTG_H__ > + > +#include <linux/types.h> > + > +struct intel_display; > +struct intel_global_state; > + > +int intel_cmtg_init(struct intel_display *display); > +void intel_cmtg_readout_hw_state(struct intel_display *display); > +u32 intel_cmtg_sanitize_state(struct intel_display *display); > + > +#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_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 1aa0b298c278..758cf8b4fb32 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" > @@ -267,6 +268,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 9a2bea19f17b..091459244ab5 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" > @@ -475,10 +476,12 @@ static void intel_sanitize_fifo_underrun_reporting(const struct intel_crtc_state > } > > static bool intel_sanitize_crtc(struct intel_crtc *crtc, > - struct drm_modeset_acquire_ctx *ctx) > + struct drm_modeset_acquire_ctx *ctx, > + u32 force_off_crtc_mask) > { > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); > + u32 crtc_mask = drm_crtc_mask(&crtc->base); > bool needs_link_reset; > > if (crtc_state->hw.active) { > @@ -509,7 +512,8 @@ static bool intel_sanitize_crtc(struct intel_crtc *crtc, > * Adjust the state of the output pipe according to whether we have > * active connectors/encoders. > */ > - if (!needs_link_reset && intel_crtc_has_encoders(crtc)) > + if (!(crtc_mask & force_off_crtc_mask) && > + !needs_link_reset && intel_crtc_has_encoders(crtc)) > return false; > > intel_crtc_disable_noatomic(crtc, ctx); > @@ -527,7 +531,8 @@ static bool intel_sanitize_crtc(struct intel_crtc *crtc, > } > > static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, > - struct drm_modeset_acquire_ctx *ctx) > + struct drm_modeset_acquire_ctx *ctx, > + u32 force_off_crtc_mask) > { > struct intel_crtc *crtc; > u32 crtcs_forced_off = 0; > @@ -547,7 +552,7 @@ static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, > if (crtcs_forced_off & crtc_mask) > continue; > > - if (intel_sanitize_crtc(crtc, ctx)) > + if (intel_sanitize_crtc(crtc, ctx, force_off_crtc_mask)) > crtcs_forced_off |= crtc_mask; > } > if (crtcs_forced_off == old_mask) > @@ -909,6 +914,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) > } > > intel_pmdemand_init_pmdemand_params(display, pmdemand_state); > + > + intel_cmtg_readout_hw_state(display); > } > > static void > @@ -967,6 +974,7 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, > struct intel_encoder *encoder; > struct intel_crtc *crtc; > intel_wakeref_t wakeref; > + u32 force_off_crtc_mask; > > wakeref = intel_display_power_get(i915, POWER_DOMAIN_INIT); > > @@ -1009,7 +1017,9 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, > */ > intel_modeset_update_connector_atomic_state(i915); > > - intel_sanitize_all_crtcs(i915, ctx); > + force_off_crtc_mask = intel_cmtg_sanitize_state(display); > + > + intel_sanitize_all_crtcs(i915, ctx, force_off_crtc_mask); > > intel_dpll_sanitize_state(i915); > > 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 \
Quoting Jani Nikula (2025-01-14 12:21:50-03:00) >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. > >Doesn't this patch disable the CRTC, not the CMTG? It disables the CMTG and that's it for LNL and PTL. For platforms prior to LNL, disabling the CMTG requires a modeset; specifically for those, the CRTC is also disabled during the sanitization process (not sure if there is a clean way of forcing a modeset from the sanitization routine). > >Can we switch off CMTG without a modeset? If not, I think we'd need to >force a modeset for takeover. As mentioned above, we can switch it off without a modeset for recent platforms (starting with LNL). My previous approach (see patch #1 of v4) would disable the CMTG as part of the initial commit and, in that flow, a modeset would be performed for pre-LNL displays. Is there a way of flagging the need for a modeset during the sanitization? -- Gustavo Sousa > >BR, >Jani. > > > > >> >> 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. >> v4: >> - Avoid if/else duplication in intel_cmtg_dump_state() by using "n/a" >> for CMTG B enabled/disabled string for platforms without it. (Jani) >> - Prefer intel_cmtg_readout_hw_state() over intel_cmtg_readout_state(). >> (Jani) >> - Use display struct instead of i915 as first parameter for >> TRANS_DDI_FUNC_CTL2(). (Jani) >> - Fewer continuation lines in variable declaration/initialization for >> better readability. (Jani) >> - Coding style improvements. (Jani) >> - Use drm_dbg_kms() instead of drm_info() for logging the disabling >> of the CMTG. >> - Make struct intel_cmtg_state entirely private to intel_cmtg.c. >> v5: >> - Do the disable sequence as part of the sanitization step after >> hardware readout instead of initial modeset commit. (Jani) >> - Adapt to commit 15133582465f ("drm/i915/display: convert global state >> to struct intel_display") by using a display struct instead of i915 >> as argument for intel_atomic_global_obj_init(). >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> --- >> drivers/gpu/drm/i915/Makefile | 1 + >> drivers/gpu/drm/i915/display/intel_cmtg.c | 250 ++++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_cmtg.h | 18 ++ >> .../gpu/drm/i915/display/intel_cmtg_regs.h | 21 ++ >> .../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 | 20 +- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/xe/Makefile | 1 + >> 10 files changed, 317 insertions(+), 5 deletions(-) >> 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..6ce8b979009a >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c >> @@ -0,0 +1,250 @@ >> +// 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_cmtg.h" >> +#include "intel_cmtg_regs.h" >> +#include "intel_de.h" >> +#include "intel_display_device.h" >> +#include "intel_display_types.h" >> +#include "intel_global_state.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. >> + */ >> + >> +/* >> + * 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; >> +}; >> + >> +static struct intel_cmtg_state *to_intel_cmtg_state(struct intel_global_state *obj_state) >> +{ >> + return container_of(obj_state, struct intel_cmtg_state, base); >> +} >> + >> +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) >> +{ >> + 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), >> + intel_cmtg_has_cmtg_b(display) ? str_enabled_disabled(cmtg_state->cmtg_b_enable) : "n/a", >> + 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 intel_cmtg_state *cmtg_state; >> + >> + cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL); >> + if (!cmtg_state) >> + return -ENOMEM; >> + >> + intel_atomic_global_obj_init(display, &display->cmtg.obj, >> + &cmtg_state->base, >> + &intel_cmtg_state_funcs); >> + >> + return 0; >> +} >> + >> +void intel_cmtg_readout_hw_state(struct intel_display *display) >> +{ >> + struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(display->cmtg.obj.state); >> + 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(display, 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(display, TRANSCODER_B)); >> + cmtg_state->trans_b_secondary = val & CMTG_SECONDARY_MODE; >> + } >> + >> + intel_cmtg_dump_state(display, cmtg_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 void intel_cmtg_state_set_disabled(struct intel_cmtg_state *cmtg_state) >> +{ >> + cmtg_state->cmtg_a_enable = false; >> + cmtg_state->cmtg_b_enable = false; >> + cmtg_state->trans_a_secondary = false; >> + cmtg_state->trans_b_secondary = false; >> +} >> + >> +static void intel_cmtg_disable(struct intel_display *display, >> + struct intel_cmtg_state *old_cmtg_state, >> + struct intel_cmtg_state *new_cmtg_state) >> +{ >> + if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state)) >> + return; >> + >> + drm_dbg_kms(display->drm, "Disabling CMTG\n"); >> + >> + intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(display, TRANSCODER_A), CMTG_SECONDARY_MODE, 0); >> + intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(display, 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); >> + } >> +} >> + >> +static u32 intel_cmtg_modeset_crtc_mask(struct intel_display *display, >> + struct intel_cmtg_state *old_cmtg_state, >> + struct intel_cmtg_state *new_cmtg_state) >> +{ >> + u32 crtc_mask; >> + >> + if (intel_cmtg_requires_modeset(display)) >> + return 0; >> + >> + crtc_mask = 0; >> + >> + if (old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary) >> + crtc_mask |= drm_crtc_mask(&intel_crtc_for_pipe(display, PIPE_A)->base); >> + >> + if (old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary) >> + crtc_mask |= drm_crtc_mask(&intel_crtc_for_pipe(display, PIPE_B)->base); >> + >> + return crtc_mask; >> +} >> + >> +/* >> + * Disable CMTG if enabled and return a mask of pipes that need to be disabled >> + * (for platforms where disabling the CMTG requires a modeset). >> + */ >> +u32 intel_cmtg_sanitize_state(struct intel_display *display) >> +{ >> + struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(display->cmtg.obj.state); >> + struct intel_cmtg_state old_cmtg_state; >> + >> + if (!HAS_CMTG(display)) >> + return 0; >> + >> + old_cmtg_state = *cmtg_state; >> + intel_cmtg_state_set_disabled(cmtg_state); >> + intel_cmtg_disable(display, &old_cmtg_state, cmtg_state); >> + >> + return intel_cmtg_modeset_crtc_mask(display, &old_cmtg_state, cmtg_state); >> +} >> 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..3c51e144aa3f >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h >> @@ -0,0 +1,18 @@ >> +/* SPDX-License-Identifier: MIT */ >> +/* >> + * Copyright (C) 2024 Intel Corporation >> + */ >> + >> +#ifndef __INTEL_CMTG_H__ >> +#define __INTEL_CMTG_H__ >> + >> +#include <linux/types.h> >> + >> +struct intel_display; >> +struct intel_global_state; >> + >> +int intel_cmtg_init(struct intel_display *display); >> +void intel_cmtg_readout_hw_state(struct intel_display *display); >> +u32 intel_cmtg_sanitize_state(struct intel_display *display); >> + >> +#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_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 1aa0b298c278..758cf8b4fb32 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" >> @@ -267,6 +268,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 9a2bea19f17b..091459244ab5 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" >> @@ -475,10 +476,12 @@ static void intel_sanitize_fifo_underrun_reporting(const struct intel_crtc_state >> } >> >> static bool intel_sanitize_crtc(struct intel_crtc *crtc, >> - struct drm_modeset_acquire_ctx *ctx) >> + struct drm_modeset_acquire_ctx *ctx, >> + u32 force_off_crtc_mask) >> { >> struct drm_i915_private *i915 = to_i915(crtc->base.dev); >> struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); >> + u32 crtc_mask = drm_crtc_mask(&crtc->base); >> bool needs_link_reset; >> >> if (crtc_state->hw.active) { >> @@ -509,7 +512,8 @@ static bool intel_sanitize_crtc(struct intel_crtc *crtc, >> * Adjust the state of the output pipe according to whether we have >> * active connectors/encoders. >> */ >> - if (!needs_link_reset && intel_crtc_has_encoders(crtc)) >> + if (!(crtc_mask & force_off_crtc_mask) && >> + !needs_link_reset && intel_crtc_has_encoders(crtc)) >> return false; >> >> intel_crtc_disable_noatomic(crtc, ctx); >> @@ -527,7 +531,8 @@ static bool intel_sanitize_crtc(struct intel_crtc *crtc, >> } >> >> static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, >> - struct drm_modeset_acquire_ctx *ctx) >> + struct drm_modeset_acquire_ctx *ctx, >> + u32 force_off_crtc_mask) >> { >> struct intel_crtc *crtc; >> u32 crtcs_forced_off = 0; >> @@ -547,7 +552,7 @@ static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, >> if (crtcs_forced_off & crtc_mask) >> continue; >> >> - if (intel_sanitize_crtc(crtc, ctx)) >> + if (intel_sanitize_crtc(crtc, ctx, force_off_crtc_mask)) >> crtcs_forced_off |= crtc_mask; >> } >> if (crtcs_forced_off == old_mask) >> @@ -909,6 +914,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) >> } >> >> intel_pmdemand_init_pmdemand_params(display, pmdemand_state); >> + >> + intel_cmtg_readout_hw_state(display); >> } >> >> static void >> @@ -967,6 +974,7 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, >> struct intel_encoder *encoder; >> struct intel_crtc *crtc; >> intel_wakeref_t wakeref; >> + u32 force_off_crtc_mask; >> >> wakeref = intel_display_power_get(i915, POWER_DOMAIN_INIT); >> >> @@ -1009,7 +1017,9 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, >> */ >> intel_modeset_update_connector_atomic_state(i915); >> >> - intel_sanitize_all_crtcs(i915, ctx); >> + force_off_crtc_mask = intel_cmtg_sanitize_state(display); >> + >> + intel_sanitize_all_crtcs(i915, ctx, force_off_crtc_mask); >> >> intel_dpll_sanitize_state(i915); >> >> 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
On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: > Quoting Jani Nikula (2025-01-14 12:21:50-03:00) > >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. > > > >Doesn't this patch disable the CRTC, not the CMTG? > > It disables the CMTG and that's it for LNL and PTL. > > For platforms prior to LNL, disabling the CMTG requires a modeset; > specifically for those, the CRTC is also disabled during the > sanitization process (not sure if there is a clean way of forcing a > modeset from the sanitization routine). I'm not sure why this whole global state stuff is needed here. It seems to me that this should be handled more or less the same as port sync. Ie: - track the cmtg state in intel_crtc_state - read it out - add it to the state checker - add the necessary bits to the disable sequence (no need for enable right now I guess if we force a disable) - flag mode_changed=true for any crtc that has cmtg enabled in initial commit to force the modeset I guess the one open question is how to deal with cases where the same CMTG is used for two pipes (assuming that's a thing?). We may need to extend the port_sync master/slave handling in the enable/disable sequences to deal with cmtg as well to make sure things are done in the right order. Also it looks like CMTG is more or less a full blow trancoder (ie. has a full set of timing registers). The docs are rather confusing but it looks to me like they're saying that one should still program the normal transcoder registers as well, even when using CMTG. I guess if we ever implement proper support for this we should at least have some kind of sanity check to confirm that.
Quoting Ville Syrjälä (2025-01-14 14:32:45-03:00) >On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: >> Quoting Jani Nikula (2025-01-14 12:21:50-03:00) >> >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. >> > >> >Doesn't this patch disable the CRTC, not the CMTG? >> >> It disables the CMTG and that's it for LNL and PTL. >> >> For platforms prior to LNL, disabling the CMTG requires a modeset; >> specifically for those, the CRTC is also disabled during the >> sanitization process (not sure if there is a clean way of forcing a >> modeset from the sanitization routine). > >I'm not sure why this whole global state stuff is needed here. >It seems to me that this should be handled more or less the same >as port sync. Ie: > >- track the cmtg state in intel_crtc_state The main reasons I implemented CMTG state as a global state were that CMTG is not a exactly per-pipe thing and it could affect multiple pipes (A and B), at least not on pre-LNL platforms. On pre-LNL platforms, we have a single CMTG that can be used to synchronize the eDP TG of either or both pipes A and B. As of LNL (Xe2_LPD, in the current patch I mistankenly considered Xe3_LPD instead), a second CMTG instance is added. In this case, we have CMTG A wired to pipe A and CMTG B, to pipe B. For dual eDP with support from the CMTG, both CMTGs must be on. For single eDP, the respective CMTG should be used. Yeah, maybe tracking the CMTG state as part of intel_crtc_state could work. Just need to think then on how to handle the pre-LNL case. (Furthermore I would also need educate myself on how our driver handle port sync that you mentioned above :-)) >- read it out In this patch I only kept the state necessary for disabling. Should we keep it like that while we only care about disabling the CMTG? >- add it to the state checker By "state checker", do you refer to intel_pipe_config_compare()? One possible issue here is that for LNL and newer, disabling the CMTG does not require a modeset. So, could we be causing an unnecessary modeset in some cases? >- add the necessary bits to the disable sequence > (no need for enable right now I guess if we > force a disable) Yep, I believe I have the hardware programming sequence to actually disable. One thing I'm strugling is to find the proper place to cause the disabling. In my original approach (see [1]), I had that done as part of the initial commit. In this current patch, the disabling was done as part of the sanitization. [1] https://lore.kernel.org/all/20250104172937.64015-2-gustavo.sousa@intel.com/ >- flag mode_changed=true for any crtc that has cmtg enabled > in initial commit to force the modeset Well, for LNL I believe we can skip the modeset and trigger it only for pre-LNL. At which point exactly should we flag mode_changed=true? In [1], I forced a modeset in intel_cmtg_check_modeset() for pipes that would have their TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] bit changed (would would only be from 1 to 0 in this case). > >I guess the one open question is how to deal with cases >where the same CMTG is used for two pipes (assuming that's >a thing?). That's a thing for pre-LNL platforms. > We may need to extend the port_sync master/slave >handling in the enable/disable sequences to deal with cmtg >as well to make sure things are done in the right order. > >Also it looks like CMTG is more or less a full blow trancoder >(ie. has a full set of timing registers). The docs are rather >confusing but it looks to me like they're saying that one >should still program the normal transcoder registers as well, >even when using CMTG. I guess if we ever implement proper >support for this we should at least have some kind of >sanity check to confirm that. Yeah. I had to go through more documentation outside of the BSpec as well as go asking hardware folks to understand it better. As far as I understand, the CMTG is not exactly a full blow transcoder. I suspect it replicates only the functions related to timing generation. And it does not actually drive the port. It runs in parallel to the timing generator actually driving the port (i.e. the eDP TG). The only interaction between the two is for synchronization. When TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] is 1, the eDP TG will sync with the CMTG's TG start and frame sync signals. -- Gustavo Sousa > >-- >Ville Syrjälä >Intel
On Wed, Jan 15, 2025 at 09:44:14AM -0300, Gustavo Sousa wrote: > Quoting Ville Syrjälä (2025-01-14 14:32:45-03:00) > >On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: > >> Quoting Jani Nikula (2025-01-14 12:21:50-03:00) > >> >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. > >> > > >> >Doesn't this patch disable the CRTC, not the CMTG? > >> > >> It disables the CMTG and that's it for LNL and PTL. > >> > >> For platforms prior to LNL, disabling the CMTG requires a modeset; > >> specifically for those, the CRTC is also disabled during the > >> sanitization process (not sure if there is a clean way of forcing a > >> modeset from the sanitization routine). > > > >I'm not sure why this whole global state stuff is needed here. > >It seems to me that this should be handled more or less the same > >as port sync. Ie: > > > >- track the cmtg state in intel_crtc_state > > The main reasons I implemented CMTG state as a global state were that > CMTG is not a exactly per-pipe thing and it could affect multiple pipes > (A and B), at least not on pre-LNL platforms. I suppose. But it doesn't seem to be fully really independent thing either especially given the dependency to the port PLL and such, and that's all handled per-pipe. > On pre-LNL platforms, we have a single CMTG that can be used to > synchronize the eDP TG of either or both pipes A and B. > > As of LNL (Xe2_LPD, in the current patch I mistankenly considered > Xe3_LPD instead), a second CMTG instance is added. In this case, we have > CMTG A wired to pipe A and CMTG B, to pipe B. For dual eDP with support > from the CMTG, both CMTGs must be on. For single eDP, the respective > CMTG should be used. > > Yeah, maybe tracking the CMTG state as part of intel_crtc_state could > work. Just need to think then on how to handle the pre-LNL case. > > (Furthermore I would also need educate myself on how our driver handle > port sync that you mentioned above :-)) > > >- read it out > > In this patch I only kept the state necessary for disabling. Should we > keep it like that while we only care about disabling the CMTG? Yeah, I guess we don't need a full readout right now. > > >- add it to the state checker > > By "state checker", do you refer to intel_pipe_config_compare()? > > One possible issue here is that for LNL and newer, disabling the CMTG > does not require a modeset. So, could we be causing an unnecessary > modeset in some cases? We can skip the check for fastset, assuming we have a proper fastset codepath for disabling the CMTG. I don't know what kind of magic synchronization is needed around that. > > >- add the necessary bits to the disable sequence > > (no need for enable right now I guess if we > > force a disable) > > Yep, I believe I have the hardware programming sequence to actually > disable. > > One thing I'm strugling is to find the proper place to cause the > disabling. In my original approach (see [1]), I had that done as part of > the initial commit. In this current patch, the disabling was done as > part of the sanitization. > > [1] https://lore.kernel.org/all/20250104172937.64015-2-gustavo.sousa@intel.com/ > > >- flag mode_changed=true for any crtc that has cmtg enabled > > in initial commit to force the modeset > > Well, for LNL I believe we can skip the modeset and trigger it only for > pre-LNL. At which point exactly should we flag mode_changed=true? Around the same part where we have the color_mgmt_changed hack in intel_initial_commit() would seem fine to me. > > In [1], I forced a modeset in intel_cmtg_check_modeset() for pipes that > would have their TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] bit changed > (would would only be from 1 to 0 in this case). > > > > >I guess the one open question is how to deal with cases > >where the same CMTG is used for two pipes (assuming that's > >a thing?). > > That's a thing for pre-LNL platforms. > > > We may need to extend the port_sync master/slave > >handling in the enable/disable sequences to deal with cmtg > >as well to make sure things are done in the right order. > > > >Also it looks like CMTG is more or less a full blow trancoder > >(ie. has a full set of timing registers). The docs are rather > >confusing but it looks to me like they're saying that one > >should still program the normal transcoder registers as well, > >even when using CMTG. I guess if we ever implement proper > >support for this we should at least have some kind of > >sanity check to confirm that. > > Yeah. I had to go through more documentation outside of the BSpec as > well as go asking hardware folks to understand it better. > > As far as I understand, the CMTG is not exactly a full blow transcoder. > I suspect it replicates only the functions related to timing generation. > > And it does not actually drive the port. It runs in parallel to the > timing generator actually driving the port (i.e. the eDP TG). The only > interaction between the two is for synchronization. When > TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] is 1, the eDP TG will sync with > the CMTG's TG start and frame sync signals. > > -- > Gustavo Sousa > > > > >-- > >Ville Syrjälä > >Intel
Quoting Ville Syrjälä (2025-01-15 12:07:39-03:00) >On Wed, Jan 15, 2025 at 09:44:14AM -0300, Gustavo Sousa wrote: >> Quoting Ville Syrjälä (2025-01-14 14:32:45-03:00) >> >On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: >> >> Quoting Jani Nikula (2025-01-14 12:21:50-03:00) >> >> >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. >> >> > >> >> >Doesn't this patch disable the CRTC, not the CMTG? >> >> >> >> It disables the CMTG and that's it for LNL and PTL. >> >> >> >> For platforms prior to LNL, disabling the CMTG requires a modeset; >> >> specifically for those, the CRTC is also disabled during the >> >> sanitization process (not sure if there is a clean way of forcing a >> >> modeset from the sanitization routine). >> > >> >I'm not sure why this whole global state stuff is needed here. >> >It seems to me that this should be handled more or less the same >> >as port sync. Ie: >> > >> >- track the cmtg state in intel_crtc_state >> >> The main reasons I implemented CMTG state as a global state were that >> CMTG is not a exactly per-pipe thing and it could affect multiple pipes >> (A and B), at least not on pre-LNL platforms. > >I suppose. But it doesn't seem to be fully really independent >thing either especially given the dependency to the port PLL >and such, and that's all handled per-pipe. To make matters worse, it is possible for CMTG A being driven by PHY B and vice-versa. > >> On pre-LNL platforms, we have a single CMTG that can be used to >> synchronize the eDP TG of either or both pipes A and B. >> >> As of LNL (Xe2_LPD, in the current patch I mistankenly considered >> Xe3_LPD instead), a second CMTG instance is added. In this case, we have >> CMTG A wired to pipe A and CMTG B, to pipe B. For dual eDP with support >> from the CMTG, both CMTGs must be on. For single eDP, the respective >> CMTG should be used. >> >> Yeah, maybe tracking the CMTG state as part of intel_crtc_state could >> work. Just need to think then on how to handle the pre-LNL case. >> >> (Furthermore I would also need educate myself on how our driver handle >> port sync that you mentioned above :-)) >> >> >- read it out >> >> In this patch I only kept the state necessary for disabling. Should we >> keep it like that while we only care about disabling the CMTG? > >Yeah, I guess we don't need a full readout right now. > >> >> >- add it to the state checker >> >> By "state checker", do you refer to intel_pipe_config_compare()? >> >> One possible issue here is that for LNL and newer, disabling the CMTG >> does not require a modeset. So, could we be causing an unnecessary >> modeset in some cases? > >We can skip the check for fastset, assuming we have a proper fastset >codepath for disabling the CMTG. I don't know what kind of magic >synchronization is needed around that. Well, the way I understand it, for LNL and newer, clearing TRANS_CMTG_CTL[31] and TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] for the associated transcoder should be enough to untie the CMTG with the transcoder. That must be done before any "modeset disables" (if any) in the commit tail to ensure that the PHY driving the CMTG is active before clearing TRANS_CMTG_CTL[31]. Finally, we can program CMTG_CLK_SEL to select no PHY to effectively disable it. For previous platforms, the Bspec instructs to follow a modeset to disable the transcoder after clearing TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] and before clearing TRANS_CMTG_CTL[31]. And only then deal with CMTG_CLK_SEL. So, I would say we have three major steps here: 1. Tell the transcoder to stop synchronizing with the CMTG by clearing TRANS_DDI_FUNC_CTL2[CMTG Secondary mode]. 2. Modeset disables (already present in the commit tail). This will happen for pre-LNL and possibly not for LNL and newer if the initial commit results in a fastset. 3. Disable the CMTG by clearing TRANS_CMTG_CTL[31] and then clearing the CMTG's clock selection (CMTG_CLK_SEL). -- Gustavo Sousa > >> >> >- add the necessary bits to the disable sequence >> > (no need for enable right now I guess if we >> > force a disable) >> >> Yep, I believe I have the hardware programming sequence to actually >> disable. >> >> One thing I'm strugling is to find the proper place to cause the >> disabling. In my original approach (see [1]), I had that done as part of >> the initial commit. In this current patch, the disabling was done as >> part of the sanitization. >> >> [1] https://lore.kernel.org/all/20250104172937.64015-2-gustavo.sousa@intel.com/ >> >> >- flag mode_changed=true for any crtc that has cmtg enabled >> > in initial commit to force the modeset >> >> Well, for LNL I believe we can skip the modeset and trigger it only for >> pre-LNL. At which point exactly should we flag mode_changed=true? > >Around the same part where we have the color_mgmt_changed hack >in intel_initial_commit() would seem fine to me. > >> >> In [1], I forced a modeset in intel_cmtg_check_modeset() for pipes that >> would have their TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] bit changed >> (would would only be from 1 to 0 in this case). >> >> > >> >I guess the one open question is how to deal with cases >> >where the same CMTG is used for two pipes (assuming that's >> >a thing?). >> >> That's a thing for pre-LNL platforms. >> >> > We may need to extend the port_sync master/slave >> >handling in the enable/disable sequences to deal with cmtg >> >as well to make sure things are done in the right order. >> > >> >Also it looks like CMTG is more or less a full blow trancoder >> >(ie. has a full set of timing registers). The docs are rather >> >confusing but it looks to me like they're saying that one >> >should still program the normal transcoder registers as well, >> >even when using CMTG. I guess if we ever implement proper >> >support for this we should at least have some kind of >> >sanity check to confirm that. >> >> Yeah. I had to go through more documentation outside of the BSpec as >> well as go asking hardware folks to understand it better. >> >> As far as I understand, the CMTG is not exactly a full blow transcoder. >> I suspect it replicates only the functions related to timing generation. >> >> And it does not actually drive the port. It runs in parallel to the >> timing generator actually driving the port (i.e. the eDP TG). The only >> interaction between the two is for synchronization. When >> TRANS_DDI_FUNC_CTL2[CMTG Secondary mode] is 1, the eDP TG will sync with >> the CMTG's TG start and frame sync signals. >> >> -- >> Gustavo Sousa >> >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel
Quoting Gustavo Sousa (2025-01-15 13:18:48-03:00) >Quoting Ville Syrjälä (2025-01-15 12:07:39-03:00) >>On Wed, Jan 15, 2025 at 09:44:14AM -0300, Gustavo Sousa wrote: >>> Quoting Ville Syrjälä (2025-01-14 14:32:45-03:00) >>> >On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: >>> >> Quoting Jani Nikula (2025-01-14 12:21:50-03:00) >>> >> >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. >>> >> > >>> >> >Doesn't this patch disable the CRTC, not the CMTG? >>> >> >>> >> It disables the CMTG and that's it for LNL and PTL. >>> >> >>> >> For platforms prior to LNL, disabling the CMTG requires a modeset; >>> >> specifically for those, the CRTC is also disabled during the >>> >> sanitization process (not sure if there is a clean way of forcing a >>> >> modeset from the sanitization routine). >>> > >>> >I'm not sure why this whole global state stuff is needed here. >>> >It seems to me that this should be handled more or less the same >>> >as port sync. Ie: >>> > >>> >- track the cmtg state in intel_crtc_state >>> >>> The main reasons I implemented CMTG state as a global state were that >>> CMTG is not a exactly per-pipe thing and it could affect multiple pipes >>> (A and B), at least not on pre-LNL platforms. >> >>I suppose. But it doesn't seem to be fully really independent >>thing either especially given the dependency to the port PLL >>and such, and that's all handled per-pipe. > >To make matters worse, it is possible for CMTG A being driven by PHY B >and vice-versa. So... I'm trying to come up with a way to handle CMTG state as part of the intel_crtc_state. I have some questions that I was hoping you could help me with... 1) For those pre-LNL platforms that have a single CMTG, what would be your suggestion? I was thinking about keeping the state on pipe A's intel_crtc_state, but then how to handle when pipe B's eDP TG is sync'ing with the CMTG? Should we just pull in pipe A's into the atomic state and deal with it? If it is just transcoder B's eDP that is hooked up wit the CMTG, pulling pipe A into the atomic state only to handle the CMTG seems rather unnecessary to me. Just accept it and live on? 2) As of LNL, eDP A would sync only with CMTG A and eDP B, with CMTG B. So, I guess having each state in the respective intel_crtc_state seems okay here. If we were to encounter a CMTG dual sync mode (is it fair to consider that a possibility from the GOP?), since only care about disabling of CMTGs for now, I guess we do not need to worry about turning sure the secondary CMTG (which will also be disabled) into primary, right? 3) There is also the case that we could have a CMTG (the single one in pre-LNL; A or B for as of LNL) being clocked by a PHY that is not being used to drive any transcoder. Not sure we could expect that from GOP, but it is nevertheless a valid configuration. We probably wouldn't be able to disable the CMTG during the initial modeset commit in this case, because we need the PHY up before accessing CMTG registers, and such PHY would be already off because of our sanitization routine after hardware state readout. Since our driver doesn't even model the PHY being active and not driving a transcoder (to the best of my knowledge), should we keep this case to be dealt with in the future? -- Gustavo Sousa
On Wed, Jan 15, 2025 at 04:41:05PM -0300, Gustavo Sousa wrote: > Quoting Gustavo Sousa (2025-01-15 13:18:48-03:00) > >Quoting Ville Syrjälä (2025-01-15 12:07:39-03:00) > >>On Wed, Jan 15, 2025 at 09:44:14AM -0300, Gustavo Sousa wrote: > >>> Quoting Ville Syrjälä (2025-01-14 14:32:45-03:00) > >>> >On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: > >>> >> Quoting Jani Nikula (2025-01-14 12:21:50-03:00) > >>> >> >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. > >>> >> > > >>> >> >Doesn't this patch disable the CRTC, not the CMTG? > >>> >> > >>> >> It disables the CMTG and that's it for LNL and PTL. > >>> >> > >>> >> For platforms prior to LNL, disabling the CMTG requires a modeset; > >>> >> specifically for those, the CRTC is also disabled during the > >>> >> sanitization process (not sure if there is a clean way of forcing a > >>> >> modeset from the sanitization routine). > >>> > > >>> >I'm not sure why this whole global state stuff is needed here. > >>> >It seems to me that this should be handled more or less the same > >>> >as port sync. Ie: > >>> > > >>> >- track the cmtg state in intel_crtc_state > >>> > >>> The main reasons I implemented CMTG state as a global state were that > >>> CMTG is not a exactly per-pipe thing and it could affect multiple pipes > >>> (A and B), at least not on pre-LNL platforms. > >> > >>I suppose. But it doesn't seem to be fully really independent > >>thing either especially given the dependency to the port PLL > >>and such, and that's all handled per-pipe. > > > >To make matters worse, it is possible for CMTG A being driven by PHY B > >and vice-versa. > > So... I'm trying to come up with a way to handle CMTG state as part of > the intel_crtc_state. I have some questions that I was hoping you could > help me with... > > 1) For those pre-LNL platforms that have a single CMTG, what would be > your suggestion? > > I was thinking about keeping the state on pipe A's intel_crtc_state, but > then how to handle when pipe B's eDP TG is sync'ing with the CMTG? > Should we just pull in pipe A's into the atomic state and deal with it? I was thinking we could just have a bitmask of pipes just like with port sync. If one needs a modeset we could then suck all of them in. Althought for just the initial disable thing we'd not really need even that I guess since we'd any flag all of them. I suppose the one whose port PLL is providing the clock should be considered the primary for the purposes of the modeset sequence. > > If it is just transcoder B's eDP that is hooked up wit the CMTG, pulling > pipe A into the atomic state only to handle the CMTG seems rather > unnecessary to me. Just accept it and live on? > > 2) As of LNL, eDP A would sync only with CMTG A and eDP B, with CMTG B. > So, I guess having each state in the respective intel_crtc_state > seems okay here. > > If we were to encounter a CMTG dual sync mode (is it fair to > consider that a possibility from the GOP?), since only care about > disabling of CMTGs for now, I guess we do not need to worry about > turning sure the secondary CMTG (which will also be disabled) into > primary, right? Yeah, just making sure the thing gets disabled more or less properly should suffice for now. > > 3) There is also the case that we could have a CMTG (the single one in > pre-LNL; A or B for as of LNL) being clocked by a PHY that is not > being used to drive any transcoder. Not sure we could expect that > from GOP, but it is nevertheless a valid configuration. Is there even a way to turn on a port PLL without turning on the whole port in the current hw with its per-port PLLs? > > We probably wouldn't be able to disable the CMTG during the initial > modeset commit in this case, because we need the PHY up before > accessing CMTG registers, and such PHY would be already off because > of our sanitization routine after hardware state readout. > > Since our driver doesn't even model the PHY being active and not > driving a transcoder (to the best of my knowledge), should we keep > this case to be dealt with in the future? > > -- > Gustavo Sousa
Quoting Ville Syrjälä (2025-01-16 16:31:42-03:00) >On Wed, Jan 15, 2025 at 04:41:05PM -0300, Gustavo Sousa wrote: >> Quoting Gustavo Sousa (2025-01-15 13:18:48-03:00) >> >Quoting Ville Syrjälä (2025-01-15 12:07:39-03:00) >> >>On Wed, Jan 15, 2025 at 09:44:14AM -0300, Gustavo Sousa wrote: >> >>> Quoting Ville Syrjälä (2025-01-14 14:32:45-03:00) >> >>> >On Tue, Jan 14, 2025 at 01:31:20PM -0300, Gustavo Sousa wrote: >> >>> >> Quoting Jani Nikula (2025-01-14 12:21:50-03:00) >> >>> >> >On Mon, 13 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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. >> >>> >> > >> >>> >> >Doesn't this patch disable the CRTC, not the CMTG? >> >>> >> >> >>> >> It disables the CMTG and that's it for LNL and PTL. >> >>> >> >> >>> >> For platforms prior to LNL, disabling the CMTG requires a modeset; >> >>> >> specifically for those, the CRTC is also disabled during the >> >>> >> sanitization process (not sure if there is a clean way of forcing a >> >>> >> modeset from the sanitization routine). >> >>> > >> >>> >I'm not sure why this whole global state stuff is needed here. >> >>> >It seems to me that this should be handled more or less the same >> >>> >as port sync. Ie: >> >>> > >> >>> >- track the cmtg state in intel_crtc_state >> >>> >> >>> The main reasons I implemented CMTG state as a global state were that >> >>> CMTG is not a exactly per-pipe thing and it could affect multiple pipes >> >>> (A and B), at least not on pre-LNL platforms. >> >> >> >>I suppose. But it doesn't seem to be fully really independent >> >>thing either especially given the dependency to the port PLL >> >>and such, and that's all handled per-pipe. >> > >> >To make matters worse, it is possible for CMTG A being driven by PHY B >> >and vice-versa. >> >> So... I'm trying to come up with a way to handle CMTG state as part of >> the intel_crtc_state. I have some questions that I was hoping you could >> help me with... >> >> 1) For those pre-LNL platforms that have a single CMTG, what would be >> your suggestion? >> >> I was thinking about keeping the state on pipe A's intel_crtc_state, but >> then how to handle when pipe B's eDP TG is sync'ing with the CMTG? >> Should we just pull in pipe A's into the atomic state and deal with it? > >I was thinking we could just have a bitmask of pipes just like with >port sync. If one needs a modeset we could then suck all of them in. >Althought for just the initial disable thing we'd not really need even >that I guess since we'd any flag all of them. I suppose the one whose >port PLL is providing the clock should be considered the primary >for the purposes of the modeset sequence. Yeah, I tried to come up with something like this, but I have hit some issues. Please see below. I believe we would need to store at least two different states regarding CMTG for proper disabling and causing modeset on the correct CRTC: - We need to know whether a transcoder's TG is secondary to the CMTG, i.e., that it is synchronizing with the CMTG. Yes, we could simply, force a modeset on both transcoders A and B, but the ones that would really need a modeset would be those that are currently secondary to the CMTG; so keeping this state is nice. Like you mentioned, one way of doing it would be a bitmask of pipes (or transcoders, to be more accurate?). - We need to know whether a CMTG is enabled. It is a valid configuration, although wasteful, to have the CMTG enabled even without any TG synchronizing with it. In this case, we would want to simply disable the CMTG and no interaction with the transcoders would be necessary. I thought about following the strategy of keeping that state in the "primary" CRTC as you mentioned above: the one whose port PLL is providing the clock for the CMTG. Please allow me calling it the "owner" from now on, to make it clear in the discussion that CMTG is not secondary to the CRTC. While trying that, I reached some issues/observations: - For display version 13 (e.g. ADL), we always have DPLL0 providing the clock for the CMTG. Since DPLL0 could be shared by different ports, it becomes ambiguous which CRTC would own the CMTG. - For Xe_LPD+ (i.e. display version 14, MTL), we can determine the specific PHY that is driving the CMTG, so this would fine. However, here we are ignoring the fact that it is theoretically possible for the PHY PLL to be used only for the CMTG and not to be driving any transcoder at a certain point in time, I'll talk more about his later (as a response to your question further down). If we assume that GOP will never leave us on such configuration, we would be fine. - As of Xe2_LPD (i.e. display version 20, LNL), we have two instances of the CMTG, and both could be clocked by the same PHY. Although there is no ambiguity about which CRTC should own a CMTG, we now could have a single CRTC owning multiple CMTGs, and we would probably need to have to track multiple CMTG states in a single CRTC state. So, I'm not sure keeping CMTG state in intel_crtc_state would work very well here. Am I missing something? Also, keeping it as a global state is not that great either, because we are forcing serialization when we need to disable the CMTG. I guess today, it wouldn't be that bad, because that would only happen during initial commit (if we follow the strategy on v4). I think it would be more problematic when/if we implement full support to CMTG in the future. That said, maybe this is slightly less complicated than keeping the state in intel_crtc_state? Another option I thought about was to keep it in intel_atomic_state, similar to what is done for shared_dpll (or, more complicated though, have it as a drm_private_state). Finally, since we are only interested in disabling the CMTG, another thing I thought about was to only track in intel_crtc_state whether the transcoder is secondary to the CMTG, e.g. bool cmtg_secondary. After hardware readout, if there is no transcoder being secondary to the CMTG, if can simply disable the CMTG during sanitization if it was enabled. If there is at least one transcoder that is secondary to the CMTG, we then leave the disable sequence to the initial commit, where we would have cmtg_secondary forced to false for every enabled CRTC and then modeset could be triggered if applicable. Would any of those alternatives work for you? > >> >> If it is just transcoder B's eDP that is hooked up wit the CMTG, pulling >> pipe A into the atomic state only to handle the CMTG seems rather >> unnecessary to me. Just accept it and live on? >> >> 2) As of LNL, eDP A would sync only with CMTG A and eDP B, with CMTG B. >> So, I guess having each state in the respective intel_crtc_state >> seems okay here. >> >> If we were to encounter a CMTG dual sync mode (is it fair to >> consider that a possibility from the GOP?), since only care about >> disabling of CMTGs for now, I guess we do not need to worry about >> turning sure the secondary CMTG (which will also be disabled) into >> primary, right? > >Yeah, just making sure the thing gets disabled more or less >properly should suffice for now. > >> >> 3) There is also the case that we could have a CMTG (the single one in >> pre-LNL; A or B for as of LNL) being clocked by a PHY that is not >> being used to drive any transcoder. Not sure we could expect that >> from GOP, but it is nevertheless a valid configuration. > >Is there even a way to turn on a port PLL without turning on the whole >port in the current hw with its per-port PLLs? I think so. For C10/C20 PHYs, there is a step for enabling the PLL and another step to bring the PHY lanes out of reset. For a modeset sequence sequences, we do both (e.g. the step "Enable Port PLL and bring PHY Lanes Out of Reset" in Bspec 68849). I believe for usage with only the CMTG, the PHY lanes would be powered down. See "Note on Clock Selection" in Bspec 49262 for an explanation and example on a case where we could have the PHY clock driving the CMTG but the PHY itself not transmitting data to the port. -- Gustavo Sousa > >> >> We probably wouldn't be able to disable the CMTG during the initial >> modeset commit in this case, because we need the PHY up before >> accessing CMTG registers, and such PHY would be already off because >> of our sanitization routine after hardware state readout. >> >> Since our driver doesn't even model the PHY being active and not >> driving a transcoder (to the best of my knowledge), should we keep >> this case to be dealt with in the future? >> >> -- >> Gustavo Sousa > >-- >Ville Syrjälä >Intel
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..6ce8b979009a --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c @@ -0,0 +1,250 @@ +// 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_cmtg.h" +#include "intel_cmtg_regs.h" +#include "intel_de.h" +#include "intel_display_device.h" +#include "intel_display_types.h" +#include "intel_global_state.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. + */ + +/* + * 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; +}; + +static struct intel_cmtg_state *to_intel_cmtg_state(struct intel_global_state *obj_state) +{ + return container_of(obj_state, struct intel_cmtg_state, base); +} + +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) +{ + 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), + intel_cmtg_has_cmtg_b(display) ? str_enabled_disabled(cmtg_state->cmtg_b_enable) : "n/a", + 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 intel_cmtg_state *cmtg_state; + + cmtg_state = kzalloc(sizeof(*cmtg_state), GFP_KERNEL); + if (!cmtg_state) + return -ENOMEM; + + intel_atomic_global_obj_init(display, &display->cmtg.obj, + &cmtg_state->base, + &intel_cmtg_state_funcs); + + return 0; +} + +void intel_cmtg_readout_hw_state(struct intel_display *display) +{ + struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(display->cmtg.obj.state); + 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(display, 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(display, TRANSCODER_B)); + cmtg_state->trans_b_secondary = val & CMTG_SECONDARY_MODE; + } + + intel_cmtg_dump_state(display, cmtg_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 void intel_cmtg_state_set_disabled(struct intel_cmtg_state *cmtg_state) +{ + cmtg_state->cmtg_a_enable = false; + cmtg_state->cmtg_b_enable = false; + cmtg_state->trans_a_secondary = false; + cmtg_state->trans_b_secondary = false; +} + +static void intel_cmtg_disable(struct intel_display *display, + struct intel_cmtg_state *old_cmtg_state, + struct intel_cmtg_state *new_cmtg_state) +{ + if (!intel_cmtg_state_changed(old_cmtg_state, new_cmtg_state)) + return; + + drm_dbg_kms(display->drm, "Disabling CMTG\n"); + + intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(display, TRANSCODER_A), CMTG_SECONDARY_MODE, 0); + intel_de_rmw(display, TRANS_DDI_FUNC_CTL2(display, 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); + } +} + +static u32 intel_cmtg_modeset_crtc_mask(struct intel_display *display, + struct intel_cmtg_state *old_cmtg_state, + struct intel_cmtg_state *new_cmtg_state) +{ + u32 crtc_mask; + + if (intel_cmtg_requires_modeset(display)) + return 0; + + crtc_mask = 0; + + if (old_cmtg_state->trans_a_secondary != new_cmtg_state->trans_a_secondary) + crtc_mask |= drm_crtc_mask(&intel_crtc_for_pipe(display, PIPE_A)->base); + + if (old_cmtg_state->trans_b_secondary != new_cmtg_state->trans_b_secondary) + crtc_mask |= drm_crtc_mask(&intel_crtc_for_pipe(display, PIPE_B)->base); + + return crtc_mask; +} + +/* + * Disable CMTG if enabled and return a mask of pipes that need to be disabled + * (for platforms where disabling the CMTG requires a modeset). + */ +u32 intel_cmtg_sanitize_state(struct intel_display *display) +{ + struct intel_cmtg_state *cmtg_state = to_intel_cmtg_state(display->cmtg.obj.state); + struct intel_cmtg_state old_cmtg_state; + + if (!HAS_CMTG(display)) + return 0; + + old_cmtg_state = *cmtg_state; + intel_cmtg_state_set_disabled(cmtg_state); + intel_cmtg_disable(display, &old_cmtg_state, cmtg_state); + + return intel_cmtg_modeset_crtc_mask(display, &old_cmtg_state, cmtg_state); +} 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..3c51e144aa3f --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright (C) 2024 Intel Corporation + */ + +#ifndef __INTEL_CMTG_H__ +#define __INTEL_CMTG_H__ + +#include <linux/types.h> + +struct intel_display; +struct intel_global_state; + +int intel_cmtg_init(struct intel_display *display); +void intel_cmtg_readout_hw_state(struct intel_display *display); +u32 intel_cmtg_sanitize_state(struct intel_display *display); + +#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_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 1aa0b298c278..758cf8b4fb32 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" @@ -267,6 +268,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 9a2bea19f17b..091459244ab5 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" @@ -475,10 +476,12 @@ static void intel_sanitize_fifo_underrun_reporting(const struct intel_crtc_state } static bool intel_sanitize_crtc(struct intel_crtc *crtc, - struct drm_modeset_acquire_ctx *ctx) + struct drm_modeset_acquire_ctx *ctx, + u32 force_off_crtc_mask) { struct drm_i915_private *i915 = to_i915(crtc->base.dev); struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); + u32 crtc_mask = drm_crtc_mask(&crtc->base); bool needs_link_reset; if (crtc_state->hw.active) { @@ -509,7 +512,8 @@ static bool intel_sanitize_crtc(struct intel_crtc *crtc, * Adjust the state of the output pipe according to whether we have * active connectors/encoders. */ - if (!needs_link_reset && intel_crtc_has_encoders(crtc)) + if (!(crtc_mask & force_off_crtc_mask) && + !needs_link_reset && intel_crtc_has_encoders(crtc)) return false; intel_crtc_disable_noatomic(crtc, ctx); @@ -527,7 +531,8 @@ static bool intel_sanitize_crtc(struct intel_crtc *crtc, } static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, - struct drm_modeset_acquire_ctx *ctx) + struct drm_modeset_acquire_ctx *ctx, + u32 force_off_crtc_mask) { struct intel_crtc *crtc; u32 crtcs_forced_off = 0; @@ -547,7 +552,7 @@ static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, if (crtcs_forced_off & crtc_mask) continue; - if (intel_sanitize_crtc(crtc, ctx)) + if (intel_sanitize_crtc(crtc, ctx, force_off_crtc_mask)) crtcs_forced_off |= crtc_mask; } if (crtcs_forced_off == old_mask) @@ -909,6 +914,8 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) } intel_pmdemand_init_pmdemand_params(display, pmdemand_state); + + intel_cmtg_readout_hw_state(display); } static void @@ -967,6 +974,7 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, struct intel_encoder *encoder; struct intel_crtc *crtc; intel_wakeref_t wakeref; + u32 force_off_crtc_mask; wakeref = intel_display_power_get(i915, POWER_DOMAIN_INIT); @@ -1009,7 +1017,9 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, */ intel_modeset_update_connector_atomic_state(i915); - intel_sanitize_all_crtcs(i915, ctx); + force_off_crtc_mask = intel_cmtg_sanitize_state(display); + + intel_sanitize_all_crtcs(i915, ctx, force_off_crtc_mask); intel_dpll_sanitize_state(i915); 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 \
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. v4: - Avoid if/else duplication in intel_cmtg_dump_state() by using "n/a" for CMTG B enabled/disabled string for platforms without it. (Jani) - Prefer intel_cmtg_readout_hw_state() over intel_cmtg_readout_state(). (Jani) - Use display struct instead of i915 as first parameter for TRANS_DDI_FUNC_CTL2(). (Jani) - Fewer continuation lines in variable declaration/initialization for better readability. (Jani) - Coding style improvements. (Jani) - Use drm_dbg_kms() instead of drm_info() for logging the disabling of the CMTG. - Make struct intel_cmtg_state entirely private to intel_cmtg.c. v5: - Do the disable sequence as part of the sanitization step after hardware readout instead of initial modeset commit. (Jani) - Adapt to commit 15133582465f ("drm/i915/display: convert global state to struct intel_display") by using a display struct instead of i915 as argument for intel_atomic_global_obj_init(). Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/display/intel_cmtg.c | 250 ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cmtg.h | 18 ++ .../gpu/drm/i915/display/intel_cmtg_regs.h | 21 ++ .../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 | 20 +- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/xe/Makefile | 1 + 10 files changed, 317 insertions(+), 5 deletions(-) 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