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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 16/02/2022 17:14, Usyskin, Alexander wrote:
> 
> 
>> -----Original Message-----
>> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Sent: Wednesday, February 16, 2022 14:04
>> To: Usyskin, Alexander <alexander.usyskin@intel.com>; Greg Kroah-
>> Hartman <gregkh@linuxfoundation.org>; Jani Nikula
>> <jani.nikula@linux.intel.com>; Joonas Lahtinen
>> <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
>> David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>
>> Cc: linux-kernel@vger.kernel.org; Winkler, Tomas
>> <tomas.winkler@intel.com>; Lubart, Vitaly <vitaly.lubart@intel.com>; intel-
>> gfx@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.

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