[PATCH v2 1/3] drm/xe: Move display reference timestamp readout to display/

[Lucas De Marchi]

On Mon, Sep 16, 2024 at 01:39:12PM GMT, Matt Roper wrote:
>On Mon, Sep 16, 2024 at 02:09:45PM -0400, Rodrigo Vivi wrote:
>> On Fri, Sep 13, 2024 at 09:29:11AM -0700, Matt Roper wrote:
>> > It's quite unusual to read display registers as part of GT
>> > initialization, but use of the display reference timestamp is one
>> > approach to calculating the GT clock frequency on older platforms.
>> > Rename the function that does this readout and move it to display/ to
>> > make it more clear what's actually happening when this route is taken.
>> > Also add an assert that we've probed display before calling this
>> > function since we never expect this to be the route taken on platforms
>> > that lack display.
>>
>> oh! that's ugly.
>> How much of this calculations and clocks are actually a coincidence then?
>>
>> Shouldn't we be using a GT based timestamp then?
>> Or perhaps a MCHBAR one? But why display timestamp would give anything
>> GT related?
>
>We choose whether or not to use this register based on CTC_MODE[0].  The
>two options are either "Crystal Clock Pin Source (default)" or
>"Broadwell Divide Logic."  The description of the latter doesn't really
>mean anything to me and doesn't seem to be explained further anywhere in
>the bspec that I can find, but presumably it means the clock is tied to
>some soc-level clock that also happens to be the same one used as the
>display's reference clock.  In fact I suspect that DSSM[31:29] would
>have worked just as well for this purpose as TIMESTAMP_OVERRIDE[9:0]
>does, but that's still just another display register.  That also likely
>means that if we didn't still care about gen12lp platforms in Xe, we
>could probably avoid reading the register at all.  I.e., everything
>Xe_HP and later has only had a single display reference clock (38.4 MHz)
>and we could probably just hardcode that value if we see the "Broadwell
>Divide Logic" setting.
>
>The use of this register was introduced (in i915) in commit dab91783338b
>("drm/i915: expose command stream timestamp frequency to userspace").
>Adding Lionel to the Cc list just in case he remembers any more details
>about where the "use display register when CTC_MODE[0] says so"
>information was documented at the time.

I'll leave my r-b since this seems better than the current state. But
we should wait this to settle.


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi