[Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei auxiliary device

[Usyskin, Alexander]

> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Sent: Wednesday, February 16, 2022 14:04
> To: Usyskin, Alexander <alexander.usyskin at intel.com>; Greg Kroah-
> Hartman <gregkh at linuxfoundation.org>; Jani Nikula
> <jani.nikula at linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen at linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> David Airlie <airlied at linux.ie>; Daniel Vetter <daniel at ffwll.ch>
> Cc: linux-kernel at vger.kernel.org; Winkler, Tomas
> <tomas.winkler at intel.com>; Lubart, Vitaly <vitaly.lubart at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
> auxiliary device
> 
> 
> 
> On 15/02/2022 15:22, Usyskin, Alexander wrote:
> 
> >>> +{
> >>> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> >>> +				      handle_simple_irq, "gsc_irq_handler");
> >>> +
> >>> +	return irq_set_chip_data(irq, dev_priv);
> >>
> >> I am not familiar with this interrupt scheme - does dev_priv get used at
> >> all by handle_simple_irq, or anyone, after being set here?
> 
> What about this? Is dev_priv required or you could pass in NULL just as
> well?
> 

It is not used, will remove

> >>
> >>> +}
> >>> +
> >>> +struct intel_gsc_def {
> >>> +	const char *name;
> >>> +	const unsigned long bar;
> >>
> >> Unusual, why const out of curiosity? And is it "bar" or "base" would be
> >> more accurate?
> >>
> > Some leftover, thanks for spotting this!
> > It is a base of bar. I prefer bar name here. But not really matter.
> 
> Is it?
> 
> +	adev->bar.start = def->bar + pdev->resource[0].start;
> 
> Looks like offset on top of BAR, no?
> 

Offset on top of DG bar; but start of HECI1/2 bar too.

> >>> +{
> >>> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> >>> +	struct mei_aux_device *adev;
> >>> +	struct auxiliary_device *aux_dev;
> >>> +	const struct intel_gsc_def *def;
> >>> +	int ret;
> >>> +
> >>> +	intf->irq = -1;
> >>> +	intf->id = intf_id;
> >>> +
> >>> +	if (intf_id == 0 && !HAS_HECI_PXP(dev_priv))
> >>> +		return;
> >>
> >> Isn't inf_id == 0 always a bug with this patch, regardless of
> >> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
> >> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
> >>
> > There will be patches for other cards that have pxp as soon as this is
> reviewed.
> > It is better to have infra prepared for two heads.
> 
> My point is things are half-prepared since you don't have the id 0 in
> the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but
> if you add a patch which enables it to be true, you have to modify the
> array at the same time or risk a broken patch in the middle.
> 
> I don't see the point of the condition making it sound like there are
> two criteria to enter below, while in fact there is only one in current
> code, and that it that it must not be entered because array is incomplete!
> 

We initialize both cells in gsc->intf array, the first one with defaults (two lines before this line)
for systems without working PXP, like DG1.
The code on GSC level does not know that we don't have PXP and don't want to know.

> >>> +
> >>> +	if (!HAS_HECI_GSC(gt->i915))
> >>> +		return;
> >>
> >> Likewise?
> >>
> >>> +
> >>> +	if (gt->gsc.intf[intf_id].irq <= 0) {
> >>> +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
> >> set");
> >>
> >> Like this, but use logging functions which say which device please.
> >>
> > drm_err_ratelimited fits here?
> 
> AFAICT it would be a programming bug and not something that can happen
> at runtime hence drm_warn_on_once sounds correct for both.
> 

Sure, will do

> >>>    }
> >>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >>>    	/* Disable RCS, BCS, VCS and VECS class engines. */
> >>>    	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
> >> 0);
> >>>    	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE,	  0);
> >>> +	if (HAS_HECI_GSC(gt->i915))
> >>> +		intel_uncore_write(uncore,
> >> GEN11_GUNIT_CSME_INTR_ENABLE, 0);
> >>>
> >>>    	/* Restore masks irqs on RCS, BCS, VCS and VECS engines. */
> >>>    	intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK,	~0);
> >>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >>>    	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
> >> 	~0);
> >>>    	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
> >>>    		intel_uncore_write(uncore,
> >> GEN12_VECS2_VECS3_INTR_MASK, ~0);
> >>> +	if (HAS_HECI_GSC(gt->i915))
> >>> +		intel_uncore_write(uncore,
> >> GEN11_GUNIT_CSME_INTR_MASK, ~0);
> >>>
> >>>    	intel_uncore_write(uncore,
> >> GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0);
> >>>    	intel_uncore_write(uncore,
> >> GEN11_GPM_WGBOXPERF_INTR_MASK,  ~0);
> >>> @@ -209,6 +216,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >>>    {
> >>>    	struct intel_uncore *uncore = gt->uncore;
> >>>    	u32 irqs = GT_RENDER_USER_INTERRUPT;
> >>> +	const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1);
> >>
> >> Why enable the one which is not supported by the patch? No harm doing
> it?
> >>
> > No harm and the next patch will be soon, this patch unfortunately is long
> overdue.
> 
> Just feels a bit lazy. You are adding two feature test macros to
> prepare, so why not use them.
> 

I've been told that better to enable them both from the HW perspective,
the real interrupt enable magic happens in GSC FW, not here.
 
> Regards,
> 
> Tvrtko

-- 
Thanks,
Sasha