RE: [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@xxxxxxxxxxxxxxx>
> Sent: Thursday, February 17, 2022 11:26
> To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>; Greg Kroah-
> Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>;
> David Airlie <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Winkler, Tomas
> <tomas.winkler@xxxxxxxxx>; Lubart, Vitaly <vitaly.lubart@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
> auxiliary device
> 
> 
> 
> On 16/02/2022 17:14, Usyskin, Alexander wrote:
> >
> >
> >> -----Original Message-----
> >> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> >> Sent: Wednesday, February 16, 2022 14:04
> >> To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>; Greg Kroah-
> >> Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jani Nikula
> >> <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> >> <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo
> <rodrigo.vivi@xxxxxxxxx>;
> >> David Airlie <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; Winkler, Tomas
> >> <tomas.winkler@xxxxxxxxx>; Lubart, Vitaly <vitaly.lubart@xxxxxxxxx>;
> intel-
> >> gfx@xxxxxxxxxxxxxxxxxxxxx
> >> 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.
> 
> Ok. :)
> 
> >>>>> +{
> >>>>> +	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.
> 
> By defaults you mean "-1" ?
> 
> My point is intel_gsc_def_dg1[] does not contain anything valid for
> interface zero. If you change HAS_HECI_PXP to return true, the code
> below does:
> 
>    def = &intel_gsc_def_dg1[intf_id];
> 
> And points to template data not populated.
> 
> So you have to change two in conjuction. Hence safest code for this
> patch would simply be:
> 
>    if (intf_id == 0) {
> 	drm_WARN_ON_ONCE(, "Code not implemented yet!\n");
> 	return;
>     }
> 
> When you add entries to intel_gsc_def_dg1[] in a later series/patch,
> then you simply remove the above lines altogether.
> 

Maybe better to add check after def = &intel_gsc_def_dg1[intf_id]; for name pointer
to catch mismatch between supported bits and filled structures in definition array, like:

    if (def->name == NULL) {
 	drm_WARN_ON_ONCE(, "HECI%d is not implemented!\n", intf_id + 1);
 	return;
     }

This way we can leave this line as we'll have more platforms without HECI1

> >>>>> +
> >>>>> +	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.
> 
> Well whatever.. As long as logging of spurious/unexpected interrupts is
> in place I can live with that.
> 
> Regards,
> 
> Tvrtko

-- 
Thanks,
Sasha