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

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

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?
>> };
>>
>> static const struct xe_device_desc tgl_desc = {
>> -- 
>> 2.44.0
>>
-- 
-shekhar