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

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Sep 19, 2024 at 02:10:41PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 19, 2024 at 01:00:02PM +0300, Jani Nikula wrote:
> > On Wed, 18 Sep 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > > On Tue, Sep 17, 2024 at 10:55:52AM GMT, Jani Nikula wrote:
> > >>On Fri, 13 Sep 2024, Matt Roper <matthew.d.roper@intel.com> 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.
> > >>>
> > >>> In the future we may want to move to an intel_display implementation
> > >>> that can be shared with i915, but we'll leave that for later.
> > >>>
> > >>> Suggested-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > >>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > >>
> > >>Mixed feelings about this. On the one hand moving to display seems
> > >>appropriate, but adding any new stuff to xe_display.c means more stuff
> > >>to clean up for later.
> > >>
> > >>As you know, i915 does this as well in i915 core. The next logical step
> > >>is then to have this in i915/display, and share the code between i915
> > >>and xe. Adding another interface for i915/display.
> > >
> > > humn... but what would be the alternative? Move the i915 one to
> > > i915/display and then make both xe-core and i915-core use that?
> > > If we move it to display/ here then we can land this and finish the
> > > cleanup later.
> > 
> > The alternative would be to keep it outside of display/ in both drivers,
> > because display doesn't appear to need it. The annoying part in that is,
> > obviously, that display should take care of display stuff.
> 
> This whole code seems rather dodgy. I see Windows has similar code
> so I presume that's where it came from. But does anyone know what
> this "Broadwell divider mode" actually does?

+Lionel, since you wrote the original code for this in commit
dab91783338b ("drm/i915: expose command stream timestamp frequency to
userspace") do you happen to remember if there was any other
documentation on the CTC_MODE setting?  I.e., what "Broadwell divider
mode" (the description in the bspec) actually means and how we should
know to use a display reference clock in that case?


Matt

> 
> If we assume that it means the display refclk is also used to
> generate the CS timestamps (I'm really suprised to learn that
> maybe there are systems with different refclks for display vs.
> GT) and that TIMESTAMP_CTR is always generated from the display
> refclk then display already reads that out from 
> DSSM, no need to read out the TIMESTAMP_OVERRIDE.
> 
> Also the current code that reads TIMESTAMP_OVERRIDE doesn't
> even seem to check whether the override is actually enabled.
> IIRC I saw bit 30==enable at least on some platforms...
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation