linux-kernel - Re: [PATCH 3/4] drm/xe/gsc: add gsc device support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cf99d227-4fee-483a-ac51-fe8fc4acf579@intel.com>
Date:   Wed, 13 Sep 2023 08:59:00 -0700
From:   Daniele Ceraolo Spurio <daniele.ceraolospurio@...el.com>
To:     "Usyskin, Alexander" <alexander.usyskin@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "De Marchi, Lucas" <lucas.demarchi@...el.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>
CC:     "Winkler, Tomas" <tomas.winkler@...el.com>,
        "Lubart, Vitaly" <vitaly.lubart@...el.com>,
        "intel-xe@...ts.freedesktop.org" <intel-xe@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] drm/xe/gsc: add gsc device support



On 9/13/2023 5:48 AM, Usyskin, Alexander wrote:
>>> +struct xe_gsc {
>> Please use a different name for this instead of just xe_gsc. In Xe we're
>> likely never going to fully use the GSC via HECI, only the GSCFI/CSC
>> part. In MTL and newer we also have the actual GSC part being split off
>> and placed inside GT (behind the GSCCS), so if we call this just xe_gsc
>> as well it'll be confusing. I suggest calling this something like
>> xe_heci_gsc, xe_heci_interface or something like that. I had actually
>> suggested this for i915 as well
>> (https://patchwork.freedesktop.org/patch/509653/) but Tomas was
>> concerned it might make backporting fixes difficult, so I dropped it;
>> this is not a concern for Xe right now.
>>
>> Note that this means renaming all the exposed functions as well.
>>
>> Daniele
>>
> Sure, will rename it xe_heci_gsc and move to xe_device.
>
>>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>> index 1dee3e832eb5..1ae4cc9f1522 100644
>>> --- a/drivers/gpu/drm/xe/xe_irq.c
>>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>>> @@ -128,6 +128,7 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>>>    	struct xe_device *xe = gt_to_xe(gt);
>>>    	u32 ccs_mask, bcs_mask;
>>>    	u32 irqs, dmask, smask;
>>> +	u32 gsc_mask = GSC_IRQ_INTF(1);
>>>
>>>    	if (xe_device_guc_submission_enabled(xe)) {
>>>    		irqs = GT_RENDER_USER_INTERRUPT |
>>> @@ -180,6 +181,9 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>>>    		if (xe_hw_engine_mask_per_class(gt,
>> XE_ENGINE_CLASS_OTHER)) {
>>>    			xe_mmio_write32(gt, GUNIT_GSC_INTR_ENABLE,
>> irqs);
>>>    			xe_mmio_write32(gt, GUNIT_GSC_INTR_MASK,
>> ~irqs);
>>> +		} else if (HAS_HECI_GSCFI(xe)) {
>>> +			xe_mmio_write32(gt, GUNIT_GSC_INTR_ENABLE,
>> gsc_mask);
>>> +			xe_mmio_write32(gt, GUNIT_GSC_INTR_MASK,
>> ~gsc_mask);
> Danielle, how this enablement should look when gsc_heci is not gt related?
> I'm somewhat confused here.

This one you should be able to leave as-is, and your check in 
HAS_HECI_GSCFI is already at the xe device level and not at the gt 
level. Unfortunately the mmio writes functions only allow a gt parameter 
for now, so we need to stick with that (I believe there is a plan to 
change it at some point).

What will need to change is xe_gsc_irq_handler(), but you can change 
that to take the heci_gsc as a parameter and then do:

     xe_gsc_irq_handler(&gt_to_xe(gt)->heci_gsc, iir);

Daniele

>