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

[Lucas De Marchi]

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?
>
>I'm still on the side that this additional code doesn't bring much value
>and could be replaced by intel-reg.

ping?

>
>>
>>So let's not remove it.
>>
>>>>The second question is why we have a huge block of 1-to-1 duplicated
>>>>code in there. Has there been an incorrect merge or some transition has
>>>>been left mid-way?
>>>
>>>not a bad merge, no. It seems to be to preserve the previous file
>>>location since now it moved to be inside a gt dir. Long term I think
>>>this is bad both because of the code duplication and because it's easy
>>>to update one and forget the other.
>>
>>I started a discussion in the thread of the original patch which called
>>to move code but left the old code in place too, effectively copying it.
>>
>>When this path was written and such code duplication noticed, would have
>>been good to highlight or address the code duplication.
>
>yes, but it doesn't mean there will be an action regarding that, as can
>be noticed since that duplication is still there today and this patch
>applies cleanly :-/... and they had slightly different changes according
>to
>
>	git log -L:frequency_show:drivers/gpu/drm/i915/gt/debugfs_gt_pm.c \
>		-L:i915_frequency_info:drivers/gpu/drm/i915/i915_debugfs.c

Just took a look for a potential deduplication fix there:
https://patchwork.freedesktop.org/series/94455/

Lucas De Marchi