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

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

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?

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