[PATCH 1/5] drm/xe: Drop __engine_mask
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu May 2 14:12:43 UTC 2024
On 02.05.2024 14:55, Lucas De Marchi wrote:
> On Thu, May 02, 2024 at 01:54:55PM GMT, Michal Wajdeczko wrote:
>>
>>
>> On 30.04.2024 17:56, Matthew Brost wrote:
>>> On Mon, Apr 29, 2024 at 04:03:25PM -0500, Lucas De Marchi wrote:
>>>> On Mon, Apr 29, 2024 at 08:36:04PM GMT, Matthew Brost wrote:
>>>>> On Mon, Apr 29, 2024 at 04:29:34PM -0400, Rodrigo Vivi wrote:
>>>>>> On Thu, Apr 25, 2024 at 11:24:06AM -0700, Lucas De Marchi wrote:
>>>>>>> Not really used, it's just a copy of engine_mask, which already
>>>>>>> reads
>>>>>>> the fuses to mark engines as available/not-available.
>>>>>>
>>>>>> I got confused trying to understand why this ever existed.
>>>>>> It indeed doesn't make much sense on today's code.
>>>>>>
>>>>>
>>>>> That was my doing with the intent that we never use the engine mask
>>>>> until after hwconfig load as the engine mask would be read from
>>>>> that. Or
>>>>> alternatively in SRIOV via CTB (or MMIO) relays.
>>>>>
>>>>> i.e. it was fake way to enforce correct load ordering.
>>>>
>>>> but we never used it from hwconfig (instead we kept the fake copy here)
>>>> and we never will as it doesn't have the info we need.
>>>>
>>>> Instead we just went out of our way and started using __engine_mask
>>>> instead where needed. See e.g. drivers/gpu/drm/xe/xe_migrate.c. I
>>>> fail to
>>>> see how exactly this enforces the correct load ordering. At most it
>>>> would return the wrong value and we would silently ignore it (e.g.
>>>> if we
>>>> had a call like xe_hw_engine_mask_per_class() before that point
>>>> since it
>>>> uses gt->info.engine_mask).
>>>>
>>>> For SR-IOV, nothing overrides engine_mask anywhere. Not even if I look
>>>> at the wip branch for VF. So, I'd say that if VF needs it, then please
>>>> add the prep together with patches making use of that because the
>>>> way it
>>>> is is now it's too fragile.
>>
>> but the idea was that for any engine overrides there should be no VF
>> specific code, except the way how VF will know the fuse values
>>
>> in other words, the only SR-IOV requirement for the load ordering was to
>> make sure that we can use GuC CTB, to allow the VF communicate with GuC,
>> prior to making any manipulation around engines.
>>
>> separate requirement to use hwconfig by the native driver, was actually
>> from the arch team, and it was a prerequisite for SR-IOV requirement, as
>> fulfilling this requirement was guaranteeing that MMIO, GGTT and IRQ
>> would be operational, thus GuC CTB will work.
>>
>> note that actual driver implementation, before this series, still didn't
>> comply with initial requirements, as while we pretend that we can enable
>> CTB in xe_guc_min_load_for_hwconfig(), any CTB communication will fail
>> as IRQ are not enabled yet
>>
>>>>
>>>
>>> They should read the fuses via a GuC relay.
>>>
>>>>>
>>>>> Assuming fuses are read after this removed LoC, the load ordering is
>>>>> correct.
>>>>
>>>> fuses are read much much earlier than that, in xe_pci.c. Why would
>>>> it be
>>>> wrong?
>>
>> only GMDID is read there and VF must use GuC MMIO (lucky not CTB)
>> communication to get that.
>
> right, I looked in the wrong place. GMDID is read from xe_pci.c.
> Fuses follow this call chain:
>
> xe_device_probe() -> xe_gt_init() -> all_fw_domain_init() ->
> xe_hw_engines_init_early()
>
>>
>> on VFs we read fuses right after we get a hwconfig and enable
>> (malfunctioning) CTB
>
> where? except from the place I'm removing and the place we read the
> fuses, I didn't find anywhere the engine_mask being changed.
to be more specific: on VF we are getting fuse values at hwconfig phase,
just to have them cached locally, before driver will try to 'read' them
in the call chain listed above.
xe_device_probe
xe_gt_init_hwconfig
xe_uc_init_hwconfig
vf_guc_min_load_for_hwconfig
and that's before
xe_device_probe
xe_gt_init
and while it's after
xe_device_probe
xe_gt_init_early
it doesn't change too much right now as on VF we don't call any of:
xe_wa_process_gt
xe_wa_process_oob
xe_tuning_process_gt
as those access registers that are not available for the VFs
so we should be fine unless some WA/tuning should be handled by the VF
Michal
>
> thanks
> Lucas De Marchi
>
>>
>>>>
>>>
>>> I was thinking the fuse read required hwconfig step (also brings up
>>
>> that was due to the ordering suggested by arch:
>>
>> 1. read hwconfig
>> 2. read fuses
>>
>> and since 1) requires the GuC, so MMIO and GGTT must be functional
>>
>> thus for 2) the VFs can use CTB Relay (as MMIO/GGTT and IRQ shall be
>> already functional due to 1)
>>
>>> CTBs) but now that I think about this, I believe the fuses can be read
>>> via a MMIO relay so the current location probably works. Double check
>>
>> according to arch team, we can't use the GuC MMIO Relay anymore
>>
>>> with Michal Winarski on that.
>>>
>>> Anyways don't let anything I say hold this up.
>>
>> btw, I was able to test this series on my WIP SR-IOV branch and didn't
>> notice any regressions yet
>>
>> Michal
>>
>>>
>>> Matt
>>>
>>>> Lucas De Marchi
>>>>
>>>>>
>>>>> Matt
>>>>>
>>>>>>>
>>>>>>> While at it, use XE_HW_ENGINE_BCS_MASK to span all copy engines.
>>>>>>>
>>>>>>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>>>>>
>>>>>>
>>>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>>>>
>>>>>>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/xe/xe_gt.c | 3 ---
>>>>>>> drivers/gpu/drm/xe/xe_gt_types.h | 6 ------
>>>>>>> drivers/gpu/drm/xe/xe_migrate.c | 3 +--
>>>>>>> drivers/gpu/drm/xe/xe_pci.c | 6 +++---
>>>>>>> 4 files changed, 4 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>>>>>>> index e922e77f5010..00a22cf2f5b5 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_gt.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>>>>>>> @@ -515,9 +515,6 @@ int xe_gt_init_hwconfig(struct xe_gt *gt)
>>>>>>> if (err)
>>>>>>> goto out_fw;
>>>>>>>
>>>>>>> - /* XXX: Fake that we pull the engine mask from hwconfig blob */
>>>>>>> - gt->info.engine_mask = gt->info.__engine_mask;
>>>>>>> -
>>>>>>> out_fw:
>>>>>>> xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>>>>>>> out:
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h
>>>>>>> b/drivers/gpu/drm/xe/xe_gt_types.h
>>>>>>> index cfdc761ff7f4..72568414fb7d 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>>>>>>> @@ -116,12 +116,6 @@ struct xe_gt {
>>>>>>> u32 reference_clock;
>>>>>>> /** @info.engine_mask: mask of engines present on GT */
>>>>>>> u64 engine_mask;
>>>>>>> - /**
>>>>>>> - * @info.__engine_mask: mask of engines present on GT
>>>>>>> read from
>>>>>>> - * xe_pci.c, used to fake reading the engine_mask from the
>>>>>>> - * hwconfig blob.
>>>>>>> - */
>>>>>>> - u64 __engine_mask;
>>>>>>> /** @info.gmdid: raw GMD_ID value from hardware */
>>>>>>> u32 gmdid;
>>>>>>> } info;
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> b/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> index 9f6e9b7f11c8..59a3f24d31e6 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> @@ -936,8 +936,7 @@ static bool has_service_copy_support(struct
>>>>>>> xe_gt *gt)
>>>>>>> * all of the actual service copy engines (BCS1-BCS8) have
>>>>>>> been fused
>>>>>>> * off.
>>>>>>> */
>>>>>>> - return gt->info.__engine_mask & GENMASK(XE_HW_ENGINE_BCS8,
>>>>>>> - XE_HW_ENGINE_BCS1);
>>>>>>> + return gt->info.engine_mask & XE_HW_ENGINE_BCS_MASK;
>>>>>>> }
>>>>>>>
>>>>>>> static u32 emit_clear_cmd_len(struct xe_gt *gt)
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c
>>>>>>> b/drivers/gpu/drm/xe/xe_pci.c
>>>>>>> index a0cf5dd803c2..6b2086ea24ab 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>>>>>> @@ -656,9 +656,9 @@ static int xe_info_init(struct xe_device *xe,
>>>>>>> gt = tile->primary_gt;
>>>>>>> gt->info.id = xe->info.gt_count++;
>>>>>>> gt->info.type = XE_GT_TYPE_MAIN;
>>>>>>> - gt->info.__engine_mask = graphics_desc->hw_engine_mask;
>>>>>>> + gt->info.engine_mask = graphics_desc->hw_engine_mask;
>>>>>>> if (MEDIA_VER(xe) < 13 && media_desc)
>>>>>>> - gt->info.__engine_mask |= media_desc->hw_engine_mask;
>>>>>>> + gt->info.engine_mask |= media_desc->hw_engine_mask;
>>>>>>>
>>>>>>> if (MEDIA_VER(xe) < 13 || !media_desc)
>>>>>>> continue;
>>>>>>> @@ -673,7 +673,7 @@ static int xe_info_init(struct xe_device *xe,
>>>>>>>
>>>>>>> gt = tile->media_gt;
>>>>>>> gt->info.type = XE_GT_TYPE_MEDIA;
>>>>>>> - gt->info.__engine_mask = media_desc->hw_engine_mask;
>>>>>>> + gt->info.engine_mask = media_desc->hw_engine_mask;
>>>>>>> gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
>>>>>>> gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
>>>>>>>
>>>>>>> --
>>>>>>> 2.43.0
>>>>>>>
More information about the Intel-xe
mailing list