-----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
Some leftover, thanks for spotting this!
+}
+
+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?
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.
reviewed.There will be patches for other cards that have pxp as soon as this is+{
+ 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).
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.
drm_err_ratelimited fits here?+
+ if (!HAS_HECI_GSC(gt->i915))
+ return;
Likewise?
+set");
+ if (gt->gsc.intf[intf_id].irq <= 0) {
+ DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
Like this, but use logging functions which say which device please.
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
it?}0);
@@ -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,
intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0);GEN11_GUNIT_CSME_INTR_ENABLE, 0);
+ if (HAS_HECI_GSC(gt->i915))
+ intel_uncore_write(uncore,
~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,
if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))GEN12_VECS2_VECS3_INTR_MASK, ~0);
intel_uncore_write(uncore,
+ if (HAS_HECI_GSC(gt->i915))GEN11_GUNIT_CSME_INTR_MASK, ~0);
+ intel_uncore_write(uncore,
GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0);
intel_uncore_write(uncore,
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
overdue.No harm and the next patch will be soon, this patch unfortunately is long
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.