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

[Matt Roper]

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 at 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 at 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*

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.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation