[Intel-gfx] [PATCH 3/3] drm/i915: remove some debug-only registers from MCHBAR

[Lucas De Marchi]

On Fri, Sep 24, 2021 at 04:16:24PM -0400, Rodrigo Vivi wrote:
>On Tue, Jul 06, 2021 at 04:44:30PM -0700, Lucas De Marchi wrote:
>> On Thu, Nov 05, 2020 at 10:02:27AM +0200, Joonas Lahtinen wrote:
>> > Quoting Lucas De Marchi (2020-11-05 03:04:22)
>> > > On Wed, Nov 04, 2020 at 11:55:15AM +0200, Joonas Lahtinen wrote:
>> > > >Quoting Lucas De Marchi (2020-10-27 06:46:18)
>> > > >> GT_PERF_STATUS and RP_STATE_LIMITS were added a long time ago in
>> > > >> commit 3b8d8d91d51c ("drm/i915: dynamic render p-state support for Sandy
>> > > >> Bridge").  Other than printing their values in debugfs we don't do
>> > > >> anything with them.  There's not much useful information in them. These
>> > > >> registers may change location in future platforms, but instead of adding
>> > > >> new locations, it's simpler to just remove them.
>> > > >
>> > > >This code seems to have been updated for Gen9LP, so that would indicate
>> > > >the debugging information is useful, right? The value is even decoded, not
>> > > >simply dumped as most registers. So I would be hesitant to drop it for
>> > > >not being useful.
>> > >
>> > > but just updating the register in itself for a new gen doesn't mean it's
>> > > actually useful... the commit message where this happened is pretty
>> > > vague: 350405623ff3 ("drm/i915: Update rps frequencies for BXT")
>> > >
>> > > My first reaction would be to do the same if the register had moved or
>> > > if it ceased to exist in a new platform. Talking with Matt Roper some
>> > > time ago we arrived to the conclusion that just printing these values is
>> > > not giving us much benefit and it could very well be accomplished by
>> > > intel_reg.
>> > >
>> > > So answering the question:  is it really useful as is? IMO, no.
>> >
>> > A quick discussion on #intel-gfx seems to indicate it was used for
>> > bug triaging in the past year. So that would indicate it is still
>> > useful to include.
>>
>> getting back to this as we are trying to upstream XeHP-SDV that doesn't
>> have access to the MCHBAR. So do you think we should just make it
>> conditional instead of removing?
>
>Yes, please let's make this conditional.
>
>>
>> I'm still on the side that this additional code doesn't bring much value
>> and could be replaced by intel-reg.
>
>In general I'd agree. However:
>
>1. Sometimes it is very hard to find out what registers and bits have
>some useful information.
>2. If it is hard to remove sometimes it is harder to add some information
>like this.
>3. I was not part of the IRC chat that Joonas mentioned, but apparently
>this data was useful in the past for some cases.

I will disagree and commit since 2 maintainers are against the
removal. I will send a new updated patch. Just to make my point clear:

1) This doesn't expose anything more than what is available with
intel_reg

2) we are already printing the wrong value for "recent" platforms:
one of the register wasn't updated for anything after gen9. The other
register doesn't actually seem to exist anymore after gen9.

3) It adds to the maintenance, particularly because this is an mchbar
register which is not available in bspec

4) It's still not clear how exactly this was useful

Lucas De Marchi