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
