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

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

On 4/19/2024 19:53, Matt Roper wrote:
> On Thu, Apr 18, 2024 at 09:48:37AM +0530, Chauhan, Shekhar wrote:
>> 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
Reviewed-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
>>>>
>>>>> ---
>>>>> 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*
> That's what I meant by the second paragraph of the commit message.  The
> fuse register has 8 bits for VECS today, but it would be a problem if
> something like VECS6 were to show up without us having defined the
> engine itself in engine_infos[] with details about interrupt bits, MMIO
> base offsets, etc.  So whenever we finally get a platform in the future
> that defines all those various details about VECS5-8, we'll add them to
> the mask here at the same time we provide the rest of the engine
> definition details.

My bad on missing it out. Thanks. Other than that, LGTM.

-shekhar

>
>
> Matt
>
>>>>> };
>>>>>
>>>>> static const struct xe_device_desc tgl_desc = {
>>>>> -- 
>>>>> 2.44.0
>>>>>
>> -- 
>> -shekhar
>>
-- 
-shekhar