Re: [PATCH v5] drm/i915/cmtg: Disable the CMTG

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel