On Fri, Oct 8, 2021 at 3:14 AM Andi Shyti <andi.shyti@xxxxxxxxx> 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
