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
