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