On Fri, Oct 8, 2021 at 3:14 AM Andi Shyti andi.shyti@intel.com wrote:
Hi Lucas,
I am reproposing this patch exactly as it was proposed initially where the original interfaces are kept where they have been originally placed. It might generate some duplicated code but, well, it's debugfs and I don't see any issue. In the future we can transform the upper interfaces to act upon all the GTs and provide information from all the GTs. This is, for example, how the sysfs interfaces will act.
NACK. We've made this mistake in the past for other debugfs files. We don't want to do it again just to maintain 2 separate places for one year and then finally realize we want to merge them.
In my opinion it's all about what mistake you like the most because until we will have multi-gt support in upstream all the patches come with the "promise" of a follow-up and maintenance cost.
no. If you put the implementation in a single place, later you only have the decision on what to do with the per-device entrypoint:
- should we remove it once igt is converted? - should we make it iterate all gts? - should we make it mean root tile?
Then you take the action that is needed and decide it per interface. Here you are leaving behind a lot of code that we will need to maintain until there is support for such a thing.
It already happened once: we needed to maintain that duplicated code for over a year with multiple patches changing them (or failing to do so).
The reason I removed them in V1 is because igt as only user is not a strong reason to keep duplicated code, but as Chris suggested offline:
"It's debugfs, igt is the primary consumer. CI has to be bridged over changes to the interfaces it is using in any case, as you want comparable results before/after the patches land.
That doesn't mean you have to copy and paste it. It may mean you do the implementation in one of them and the other calls that implementation. See how I did the deduplication in commit d0c560316d6f ("drm/i915: deduplicate frequency dump on debugfs")
In this case, from a user perspective, which gt is the interface affecting? is it affecting all the system? or gt 0, 1...? Does the user know? The maintenance cost is that later you will need to use for_each_gt and make all those interfaces multitile, and this would be your "promise".
multi-gt is irrelevant here. This patch without any "promise" should do what the commit message says: *move*. The only reason to keep the old entrypoint around is because it's missing the igt conversion. If you are going to support a per-device entrypoint and do for_each_gt(), or do a symlink to the root tile, or whatever, it isn't very relevant to this patch. Right now we have just a single directory, gt.
Lucas De Marchi