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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 01/10/2024 02:26, Matt Roper wrote:
> 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?


I don't remember this stuff having anything to do with display HW (is 
TIMESTAMP_OVERRIDE part of the display HW?).

And yes, I had to read the windows code to get what register to read 
based on the value of CTC_MODE because there is no documentation about 
that afaict.


-Lionel


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


That bit seems deprecated since 396385 398004 (Gfx9 era)


>>
>> -- 
>> Ville Syrjälä
>> Intel