Re: [Intel-gfx] [PATCH 05/11] drm/i915: Prepare for multiple gts

[Lucas De Marchi <lucas.demarchi@xxxxxxxxx>]

On Fri, Oct 08, 2021 at 02:56:29PM -0700, Matt Roper wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Add some basic plumbing to support more than one dynamically allocated
> struct intel_gt.  Up to four gts are supported in i915->gts[], with slot
> zero shadowing the existing i915->gt to enable source compatibility with
> legacy driver paths.  A for_each_gt macro is added to iterate over the
> GTs and will be used by upcoming patches that convert various parts of
> the driver to be multi-gt aware.
>
> 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         | 74 ++++++++++++++++++++--
> drivers/gpu/drm/i915/gt/intel_gt.h         |  8 ++-
> drivers/gpu/drm/i915/gt/intel_gt_types.h   |  2 +
> drivers/gpu/drm/i915/i915_drv.c            |  2 +-
> drivers/gpu/drm/i915/i915_drv.h            |  6 ++
> drivers/gpu/drm/i915/intel_memory_region.h |  3 +
> 6 files changed, 86 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 863039d56cba..736725411f51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -23,10 +23,13 @@
> #include "shmem_utils.h"
> #include "pxp/intel_pxp.h"
>
> -void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
> +static void
> +__intel_gt_init_early(struct intel_gt *gt,
> +		      struct intel_uncore *uncore,
> +		      struct drm_i915_private *i915)
> {
> 	gt->i915 = i915;
> -	gt->uncore = &i915->uncore;
> +	gt->uncore = uncore;
>
> 	spin_lock_init(&gt->irq_lock);
>
> @@ -46,13 +49,18 @@ void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
> 	intel_rps_init_early(&gt->rps);
> }
>
> -int intel_gt_probe_lmem(struct intel_gt *gt)
> +static int intel_gt_probe_lmem(struct intel_gt *gt)
> {
> 	struct drm_i915_private *i915 = gt->i915;
> +	unsigned int instance = gt->info.id;
> 	struct intel_memory_region *mem;
> 	int id;
> 	int err;
>
> +	id = INTEL_REGION_LMEM + instance;
> +	if (drm_WARN_ON(&i915->drm, id >= INTEL_REGION_STOLEN_SMEM))
> +		return -ENODEV;
> +
> 	mem = intel_gt_setup_lmem(gt);
> 	if (mem == ERR_PTR(-ENODEV))
> 		mem = intel_gt_setup_fake_lmem(gt);
> @@ -67,9 +75,8 @@ int intel_gt_probe_lmem(struct intel_gt *gt)
> 		return err;
> 	}
>
> -	id = INTEL_REGION_LMEM;
> -
> 	mem->id = id;
> +	mem->instance = instance;
>
> 	intel_memory_region_set_name(mem, "local%u", mem->instance);
>
> @@ -80,6 +87,11 @@ int intel_gt_probe_lmem(struct intel_gt *gt)
> 	return 0;
> }
>
> +void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
> +{
> +	__intel_gt_init_early(gt, &i915->uncore, i915);
> +}
> +
> void intel_gt_init_hw_early(struct intel_gt *gt, struct i915_ggtt *ggtt)
> {
> 	gt->ggtt = ggtt;
> @@ -903,9 +915,29 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg)
> static int
> tile_setup(struct intel_gt *gt, unsigned int id, phys_addr_t phys_addr)
> {
> +	struct drm_i915_private *i915 = gt->i915;
> +	struct intel_uncore *uncore;
> +	struct intel_uncore_mmio_debug *mmio_debug;
> 	int ret;
>
> -	intel_uncore_init_early(gt->uncore, gt);
> +	if (id) {
> +		uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);
> +		if (!uncore)
> +			return -ENOMEM;
> +
> +		mmio_debug = kzalloc(sizeof(*mmio_debug), GFP_KERNEL);
> +		if (!mmio_debug) {
> +			kfree(uncore);
> +			return -ENOMEM;
> +		}
> +
> +		__intel_gt_init_early(gt, uncore, i915);
> +	} else {
> +		uncore = &i915->uncore;
> +		mmio_debug = &i915->mmio_debug;
> +	}
> +
> +	intel_uncore_init_early(uncore, gt);
>
> 	ret = intel_uncore_setup_mmio(gt->uncore, phys_addr);
> 	if (ret)
> @@ -919,6 +951,11 @@ tile_setup(struct intel_gt *gt, unsigned int id, phys_addr_t phys_addr)
> static void tile_cleanup(struct intel_gt *gt)
> {
> 	intel_uncore_cleanup_mmio(gt->uncore);
> +
> +	if (gt->info.id) {
> +		kfree(gt->uncore);
> +		kfree(gt);
> +	}
> }
>
> int intel_probe_gts(struct drm_i915_private *i915)
> @@ -936,13 +973,36 @@ int intel_probe_gts(struct drm_i915_private *i915)
> 	if (ret)
> 		return ret;
>
> +	i915->gts[0] = &i915->gt;
> +
> 	/* TODO: add more tiles */
> 	return 0;
> }
>
> +int intel_gt_tiles_init(struct drm_i915_private *i915)
> +{
> +	struct intel_gt *gt;
> +	unsigned int id;
> +	int ret;
> +
> +	for_each_gt(i915, id, gt) {
> +		ret = intel_gt_probe_lmem(gt);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;

same thing as with previous patches that makes me confused. gt == tile,
why do we have such a function in intel_gt.c - intel_gt_tiles_init()
that receive the device struct? IMO this belongs to i915_drv.c

> +}
> +
> void intel_gts_release(struct drm_i915_private *i915)
> {
> -	tile_cleanup(&i915->gt);
> +	struct intel_gt *gt;
> +	unsigned int id;
> +
> +	for_each_gt(i915, id, gt) {
> +		tile_cleanup(gt);
> +		i915->gts[id] = NULL;

ditto

Lucas De Marchi