Re: [Intel-gfx] [PATCH v4 2/2] drm/i915: Use to_root_gt() to refer to the root tile

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Michal,

> >> fist of all thanks for taking a look at this, I was eagerly
> >> waiting for reviewers.
> >>
> >> On Tue, Nov 30, 2021 at 01:07:30PM -0800, Lucas De Marchi wrote:
> >>> On Sun, Nov 28, 2021 at 01:09:26PM +0200, Andi Shyti wrote:
> >>> > Starting from a patch from Matt to_root_gt() returns the
> >>> > reference to the root tile in order to abstract the root tile
> >>> > from th callers.
> >>> >
> >>> > Being the root tile identified as tile '0', embed the id in the
> >>> > name so that i915->gt becomes i915->gt0.
> >>> >
> >>> > The renaming has been mostly done with the following command and
> >>> > some manual fixes.
> >>> >
> >>> > sed -i -e sed -i 's/\&i915\->gt\./\&to_root_gt(i915)\->/g' \
> >>> >     -e sed -i 's/\&dev_priv\->gt\./\&to_root_gt(dev_priv)\->/g' \
> >>> >     -e 's/\&dev_priv\->gt/to_root_gt(dev_priv)/g' \
> >>> >     -e 's/\&i915\->gt/to_root_gt(i915)/g' \
> >>> >     -e 's/dev_priv\->gt\./to_root_gt(dev_priv)\->/g' \
> >>> >     -e 's/i915\->gt\./to_root_gt(i915)\->/g' \
> >>> >     `find drivers/gpu/drm/i915/ -name *.[ch]`
> >>> >
> >>> > Two small changes have been added to this commit:
> >>> >
> >>> > 1. intel_reset_gpu() in intel_display.c retreives the gt from
> >>> >    to_scanout_gt()
> >>> > 2. in set_scheduler_caps() the gt is taken from the engine and
> >>> >    not from i915.
> >>>
> >>> Ideally the non-automatic changes should be in separate patches, before
> >>> the ones that can be done by automation. Because then it becomes easier
> >>> to apply the final result without conflicts.
> >>
> >> OK
> >>
> >>> This is quite a big diff to merge in one go. Looking at the pending
> >>> patches from Michal however I see he had similar changes, split in
> >>> sensible chunks..  Could you split your version like that? at least
> >>> gt/gem and display would be good to have separate. Or sync with Michal
> >>> on how to proceed with these versions Here are his patches:
> >>>
> >>>     drm/i915: Remove i915->ggtt
> >>>     drm/i915: Use to_gt() helper for GGTT accesses
> >>>     drm/i915: Use to_gt() helper
> >>>     drm/i915/gvt: Use to_gt() helper
> >>>     drm/i915/gem: Use to_gt() helper
> >>>     drm/i915/gt: Use to_gt() helper
> >>>     drm/i915/display: Use to_gt() helper
> >>>     drm/i915: Introduce to_gt() helper
> >>
> >> I understand... will follow this approach.
> >>
> >>> This first patch also removed the `struct intel_gt *gt = to_gt(pool)`,
> >>> that would otherwise be a leftover in
> >>> drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> >>
> >> One difference from Michal patch is that I am not using the
> >> wrapper
> >>
> >>  to_gt(...)
> >>
> >> but
> >>
> >>  to_root_gt(...)
> >>
> >> which was introduced by Matt. To me sounds more meaningful as it
> >> specifies that we are really looking for the root tile and not
> >> any tile.
> > 
> > yes, I think it makes sense, too.  Michal, any comment?  I think you
> > also had other plans to get the root gt by another helper... ?
> 
> The main rationale to use generic "to_gt()" helper name in all existing
> i915->gt cases in (other) Michal patches was that on some upcoming
> configs we want to distinguish between "primary" and "root" tile and use
> "to_root_gt()" helper only when referring to the root tile as described
> in Bspec:52416.
> 
> Note that since current code baseline is still "single" tile, you can't
> tell whether all of these functions really expects special "root" tile
> or just "any" tile.

this series is indeed preparatory for the multitile and making it
to_gt() now it will require to replace it with to_root_gt()
later.

The idea is that a GT is root even if it's alone. The next patch
after this will be the actual multitile.[*]

In this particular patch I am even renaming i915->gt to i915->gt0
to underline the difference.

> Thus to avoid confusion or mistakes I would suggest to keep simple name
> "to_gt()" as in most cases usages of this helper it will likely be
> replaced with iterator from for_each_gt loop and any remaining usages
> will just mean "primary" tile or replaced with explicit "to_root_gt()"
> if really needed.

Knowing what's about to come, I do not see this as a good reason
to have to_gt() as a mid step. Right?

Andi

[*] https://patchwork.freedesktop.org/patch/464475/?series=97352&rev=1