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@xxxxxxxxx> 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@xxxxxxxxx/ >- 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
