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