Re: [PATCH] drm/xe: Define all possible engines in media IP descriptors

["Chauhan, Shekhar" <shekhar.chauhan@intel.com>]

On 4/18/2024 09:46, Chauhan, Shekhar wrote:
>
> On 4/17/2024 23:48, Lucas De Marchi wrote:
>> On Wed, Apr 17, 2024 at 08:26:22AM GMT, Matt Roper wrote:
>>> Rather than trying to identify exactly which engines are available on
>>> each platform in the IP descriptor, just include the list of all media
>>> engines that the IP could theoretically support (i.e., 8 VCS + 4 VECS).
>>> We still rely on the media fuse registers to tell us which specific
>>> engine instances are actually present on a given platform, so there
>>> shouldn't be any functional change.  This will help prevent mistakes
>>> with engine numbering (for example ambiguity about whether the 2nd VCS
>>> engine on a platform with exactly two engines is numbered "VCS1" or
>>> "VCS2") and will also future-proof the code a bit more in case new SKUs
>>> or platform refreshes extend the engine list in the future.
>>>
>>> Note that the media fuse register technically has an 8-bit field for
>>> VECS engine presence starting on Xe2.  However there's still no MMIO
>>> register range reserved for VE engines above VECS3, so VE0-VE3 is still
>>> consider the "maximum" VE engine mask that the driver can support for
>>> now.
>>>
>>> Bspec: 52614, 52615, 62567
>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>
>> I remember we tried something similar for BCS and had to revert. However
>> for media it seems we have fuses available on all platforms, so should
>> be better handled by the current code.
>>
>> Assuming CI is happy,
>>
>> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>> Lucas De Marchi
>>
>>> ---
>>> drivers/gpu/drm/xe/xe_pci.c | 16 +++++++++-------
>>> 1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index 3b30353dbc09..0d21306f8995 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -185,8 +185,8 @@ static const struct xe_media_desc media_xem = {
>>>     .rel = 0,
>>>
>>>     .hw_engine_mask =
>>> -        BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VCS2) |
>>> -        BIT(XE_HW_ENGINE_VECS0),
>>> +        GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
>>> +        GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0),
>>> };
>>>
>>> static const struct xe_media_desc media_xehpm = {
>>> @@ -195,21 +195,23 @@ static const struct xe_media_desc media_xehpm = {
>>>     .rel = 55,
>>>
>>>     .hw_engine_mask =
>>> -        BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VCS2) |
>>> -        BIT(XE_HW_ENGINE_VECS0) | BIT(XE_HW_ENGINE_VECS1),
>>> +        GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
>>> +        GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0),
>>> };
>>>
>>> static const struct xe_media_desc media_xelpmp = {
>>>     .name = "Xe_LPM+",
>>>     .hw_engine_mask =
>>> -        BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VCS2) |
>>> -        BIT(XE_HW_ENGINE_VECS0) | BIT(XE_HW_ENGINE_GSCCS0)
>>> +        GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
>>> +        GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0) |
>>> +        BIT(XE_HW_ENGINE_GSCCS0)
>>> };
>>>
>>> static const struct xe_media_desc media_xe2 = {
>>>     .name = "Xe2_LPM / Xe2_HPM",
>>>     .hw_engine_mask =
>>> -        BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VECS0), /* TODO: 
>>> GSC0 */
>>> +        GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
>>> +        GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0), /* TODO: 
>>> GSC0 */
> Are we correct in excluding the 4 new additions that are part of a new 
> HSD, stated in BSpec 62567?
Correction: Not new, rather, different*
>>> };
>>>
>>> static const struct xe_device_desc tgl_desc = {
>>> -- 
>>> 2.44.0
>>>
-- 
-shekhar