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@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. 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@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? 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
