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

From: Tvrtko Ursulin
Date: Thu Feb 17 2022 - 04:26:34 EST



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.


+
+ 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