On Tue, 26 Oct 2021, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote:
> On Wed, Oct 13, 2021 at 03:12:55PM +0300, Jani Nikula wrote:
>>On Fri, 08 Oct 2021, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
>>> On a multi-tile platform, each tile has its own registers + GGTT space,
>>> and BAR 0 is extended to cover all of them. Upcoming patches will start
>>> exposing the tiles as multiple GTs within a single PCI device. In
>>> preparation for supporting such setups, restructure the driver's probe
>>> code a bit.
>>>
>>> Only the primary/root tile is initialized for now; the other tiles will
>>> be detected and plugged in by future patches once the necessary
>>> infrastructure is in place to handle them.
>>>
>>> Original-author: Abdiel Janulgue
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>>> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>>> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt.c | 45 ++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/gt/intel_gt.h | 3 ++
>>> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 9 ++++-
>>> drivers/gpu/drm/i915/gt/intel_gt_types.h | 5 +++
>>> drivers/gpu/drm/i915/i915_drv.c | 20 +++++------
>>> drivers/gpu/drm/i915/intel_uncore.c | 12 +++----
>>> drivers/gpu/drm/i915/intel_uncore.h | 3 +-
>>> 7 files changed, 76 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> index 1cb1948ac959..f4bea1f1de77 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -900,6 +900,51 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg)
>>> return intel_uncore_read_fw(gt->uncore, reg);
>>> }
>>>
>>> +static int
>>> +tile_setup(struct intel_gt *gt, unsigned int id, phys_addr_t phys_addr)
>>> +{
>>> + int ret;
>>> +
>>> + intel_uncore_init_early(gt->uncore, gt->i915);
>>> +
>>> + ret = intel_uncore_setup_mmio(gt->uncore, phys_addr);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + gt->phys_addr = phys_addr;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void tile_cleanup(struct intel_gt *gt)
>>> +{
>>> + intel_uncore_cleanup_mmio(gt->uncore);
>>> +}
>>> +
>>> +int intel_probe_gts(struct drm_i915_private *i915)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>>> + phys_addr_t phys_addr;
>>> + unsigned int mmio_bar;
>>> + int ret;
>>> +
>>> + mmio_bar = GRAPHICS_VER(i915) == 2 ? 1 : 0;
>>> + phys_addr = pci_resource_start(pdev, mmio_bar);
>>> +
>>> + /* We always have at least one primary GT on any device */
>>> + ret = tile_setup(&i915->gt, 0, phys_addr);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* TODO: add more tiles */
>>> + return 0;
>>> +}
>>> +
>>> +void intel_gts_release(struct drm_i915_private *i915)
>>> +{
>>> + tile_cleanup(&i915->gt);
>>> +}
>>
>>Please call the functions intel_gt_*.
>
> actually besides the name, the fact that these take i915 as argument
> seems to suggest they are in the wrong place.
As a rule of thumb, anything to do with just display should go under
display/, anything to do with just gt should go under gt/, etc. I think
we need more separation between the parts of the driver.
> Probably this part should
> remain in i915_drv.c with name i915_setup_gts()?
I dislike the use of plurals like this, because you don't know if it's
"a single GTS" or "multiple GT's". I first paused with "what's a
GTS". And as we know, new TLAs crop up constantly...
BR,
Jani.
>
> then we could export tile_setup as intel_gt_setup() or something else
> (name here is confusing IMO as in some places we have
> `init(); ...; setup()` and in other this is `setup(); ...; init();` like
> in patch 1. We already have
>
> - intel_gt_init_early()
> - intel_gt_init_hw_early()
> - intel_gt_init_mmio()
> - intel_gt_init()
>
>
> given this is just initiliazing mmio for that specific gt (tile), do we
> actually need a new init/setup entrypoint?
>
>>
>>BR,
>>Jani.
>>
>>
>>
>>> +
>>> void intel_gt_info_print(const struct intel_gt_info *info,
>>> struct drm_printer *p)
>>> {
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
>>> index 74e771871a9b..f4f35a70cbe4 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
>>> @@ -85,6 +85,9 @@ static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
>>>
>>> u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
>>>
>>> +int intel_probe_gts(struct drm_i915_private *i915);
>>> +void intel_gts_release(struct drm_i915_private *i915);
>>> +
>>> void intel_gt_info_print(const struct intel_gt_info *info,
>>> struct drm_printer *p);
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>>> index 524eaf678790..76f498edb0d5 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>>> @@ -126,7 +126,14 @@ static const struct intel_wakeref_ops wf_ops = {
>>>
>>> void intel_gt_pm_init_early(struct intel_gt *gt)
>>> {
>>> - intel_wakeref_init(>->wakeref, gt->uncore->rpm, &wf_ops);
>>> + /*
>>> + * We access the runtime_pm structure via gt->i915 here rather than
>>> + * gt->uncore as we do elsewhere in the file because gt->uncore is not
>>> + * yet initialized for all tiles at this point in the driver startup.
>>> + * runtime_pm is per-device rather than per-tile, so this is still the
>>> + * correct structure.
>>> + */
>>> + intel_wakeref_init(>->wakeref, >->i915->runtime_pm, &wf_ops);
>>> seqcount_mutex_init(>->stats.lock, >->wakeref.mutex);
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> index 14216cc471b1..66143316d92e 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> @@ -180,6 +180,11 @@ struct intel_gt {
>>>
>>> const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
>>>
>>> + /*
>>> + * Base of per-tile GTTMMADR where we can derive the MMIO and the GGTT.
>>> + */
>>> + phys_addr_t phys_addr;
>
> here and in the next patches it doesn't seem like we need to save
> phys_addr (which should probably be better named as gttmmadr)? Looking
> at what is coming it seems this will be needed only when initializing
> ggtt... We could move this to when it's needed or, just to be clear this
> is indeed desired for a future change, drop a comment in the commit
> message the address will be needed for the ggtt initialization?
>
> Lucas De Marchi
--
Jani Nikula, Intel Open Source Graphics Center
