intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: remove some debug-only registers from MCHBAR
Date: Tue, 6 Jul 2021 16:44:30 -0700	[thread overview]
Message-ID: <20210706234430.nm64jerbt3kkoeue@ldmartin-desk2> (raw)
In-Reply-To: <160456334665.5393.4671076622521791518@jlahtine-mobl.ger.corp.intel.com>

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.

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


Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2021-07-06 23:44 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-27  4:46 [Intel-gfx] [PATCH 1/3] drm/i915: Guard debugfs against invalid access without display Lucas De Marchi
2020-10-27  4:46 ` [Intel-gfx] [PATCH 2/3] drm/i915/display: remove debug message from error path Lucas De Marchi
2020-10-27  7:48   ` Jani Nikula
2020-10-27  4:46 ` [Intel-gfx] [PATCH 3/3] drm/i915: remove some debug-only registers from MCHBAR Lucas De Marchi
2020-10-29 22:06   ` Srivatsa, Anusha
2020-11-04  9:55   ` Joonas Lahtinen
2020-11-05  1:04     ` Lucas De Marchi
2020-11-05  8:02       ` Joonas Lahtinen
2021-07-06 23:44         ` Lucas De Marchi [this message]
2021-09-07 21:56           ` Lucas De Marchi
2021-09-21  5:40             ` Lucas De Marchi
2021-09-24 20:16           ` Rodrigo Vivi
2021-09-24 23:45             ` Lucas De Marchi
2020-10-27  7:48 ` [Intel-gfx] [PATCH 1/3] drm/i915: Guard debugfs against invalid access without display Jani Nikula
2020-10-27 16:59 ` Souza, Jose
2020-10-28  0:28 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
2020-10-28  3:17 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2020-10-28  7:30   ` Lucas De Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210706234430.nm64jerbt3kkoeue@ldmartin-desk2 \
    --to=lucas.demarchi@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=joonas.lahtinen@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).