Re: [Intel-gfx] [PATCH 03/11] drm/i915: Restructure probe to handle multi-tile platforms

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 26 Oct 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Wed, Oct 13, 2021 at 03:12:55PM +0300, Jani Nikula wrote:
>>On Fri, 08 Oct 2021, Matt Roper <matthew.d.roper@intel.com> 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@intel.com>
>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>> ---
>>>  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(&gt->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(&gt->wakeref, &gt->i915->runtime_pm, &wf_ops);
>>>  	seqcount_mutex_init(&gt->stats.lock, &gt->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