Re: [Intel-gfx] [PATCH v5 2/7] drm/i915: Prepare for multiple GTs

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

Hi Andrzej,

[...]

> > -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_0 + instance;
> > +	if (drm_WARN_ON(&i915->drm, id >= INTEL_REGION_STOLEN_SMEM))
> 
> Do we need to check id correctness? wouldn't be enough to check it on
> initialization of gt->info.id.
> If yes, maybe (id > INTEL_REGION_LMEM_3) would be more readable, or (info.id
> < MAX_GT),  up to you.

yes, it's indeed redundant. Also because if that 'if' was true it
would be a bit more catastrophic than a simple warning. I will
remove it completely.

[...]

> > +	if (id) {
> > +		struct intel_uncore_mmio_debug *mmio_debug;
> > +		struct intel_uncore *uncore;
> > +
> > +		uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);
> > +		if (!gt->uncore)
> > +			return -ENOMEM;
> 
> s/gt->uncore/uncore/

thanks!

[...]

> > +static void
> > +intel_gt_tile_cleanup(struct intel_gt *gt)
> > +{
> > +	intel_uncore_cleanup_mmio(gt->uncore);
> > +
> > +	if (gt->info.id) {
> > +		kfree(gt->uncore);
> > +		kfree(gt);
> 
> What about gt->uncore->debug ?

you don't want to leak anything? :)

will add it, nice catch! Thanks!

[...]

> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 1c67ff735f18..144f989e4fef 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -320,9 +320,8 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
> >   	intel_device_info_subplatform_init(dev_priv);
> >   	intel_step_init(dev_priv);
> > -	intel_gt_init_early(to_gt(dev_priv), dev_priv);
> > +	/* All tiles share a single mmio_debug */
> 
> So why are we allocating mmio_debug in intel_gt_tile_setup ?

yes... this is a leftover from previous development cycles... I
will remove the comment. Indeed this goes only to tile 0.

[...]

> >   void intel_uncore_cleanup_mmio(struct intel_uncore *uncore)
> >   {
> > -	struct pci_dev *pdev = to_pci_dev(uncore->i915->drm.dev);
> > -
> > -	pci_iounmap(pdev, uncore->regs);
> > +	if (uncore->regs)
> > +		iounmap(uncore->regs);
> 
> 'if' is not necessary, up to you.

will remove, thanks!

[...]

Thank you for the review!
Andi