[PATCH 4/4] drm/xe: Convert pre-GMDID IPs to struct xe_ip
Matt Roper
matthew.d.roper at intel.com
Thu Feb 20 21:31:42 UTC 2025
On Thu, Feb 20, 2025 at 06:25:14PM -0300, Gustavo Sousa wrote:
> Quoting Gustavo Sousa (2025-02-20 18:23:49-03:00)
> >Quoting Matt Roper (2025-02-20 18:10:50-03:00)
> >>On Thu, Feb 20, 2025 at 02:25:11PM -0300, Gustavo Sousa wrote:
> >>> We have now a struct xe_ip to fully describe an IP, but we are only
> >>> using that for GMDID-based IPs.
> >>>
> >>> For pre-GMDID IPs, we still describe release info (version and name) via
> >>> feature descriptors (struct xe_{graphics,media}_desc). Let's convert
> >>> those to use struct xe_ip.
> >>>
> >>> With this, we have a uniform way of describing IPs in the xe driver
> >>> instead of having different approaches based on whether the IPs use
> >>> GMDIDs or not.
> >>>
> >>> A nice side-effect of this change is that now we have an easy way to
> >>> lookup, in the source code, mappings between versions, names and
> >>> features for all supported IPs.
> >>>
> >>> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> >>> ---
> >>> drivers/gpu/drm/xe/xe_pci.c | 60 ++++++++++++++++---------------
> >>> drivers/gpu/drm/xe/xe_pci_types.h | 8 -----
> >>> 2 files changed, 32 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> >>> index 8b8111e954c1..c88c2da1e731 100644
> >>> --- a/drivers/gpu/drm/xe/xe_pci.c
> >>> +++ b/drivers/gpu/drm/xe/xe_pci.c
> >>> @@ -82,10 +82,6 @@ __diag_ignore_all("-Woverride-init", "Allow field overrides in table");
> >>> #define NOP(x) x
> >>>
> >>> static const struct xe_graphics_desc graphics_xelp = {
> >>> - .name = "Xe_LP",
> >>> - .ver = 12,
> >>> - .rel = 0,
> >>> -
> >>> .hw_engine_mask = BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0),
> >>>
> >>> .va_bits = 48,
> >>> @@ -93,10 +89,6 @@ static const struct xe_graphics_desc graphics_xelp = {
> >>> };
> >>>
> >>> static const struct xe_graphics_desc graphics_xelpp = {
> >>> - .name = "Xe_LP+",
> >>> - .ver = 12,
> >>> - .rel = 10,
> >>> -
> >>> .hw_engine_mask = BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0),
> >>>
> >>> .va_bits = 48,
> >>> @@ -109,10 +101,6 @@ static const struct xe_graphics_desc graphics_xelpp = {
> >>> .vm_max_level = 3
> >>>
> >>> static const struct xe_graphics_desc graphics_xehpg = {
> >>> - .name = "Xe_HPG",
> >>> - .ver = 12,
> >>> - .rel = 55,
> >>> -
> >>> .hw_engine_mask =
> >>> BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0) |
> >>> BIT(XE_HW_ENGINE_CCS0) | BIT(XE_HW_ENGINE_CCS1) |
> >>> @@ -125,10 +113,6 @@ static const struct xe_graphics_desc graphics_xehpg = {
> >>> };
> >>>
> >>> static const struct xe_graphics_desc graphics_xehpc = {
> >>> - .name = "Xe_HPC",
> >>> - .ver = 12,
> >>> - .rel = 60,
> >>> -
> >>> .hw_engine_mask =
> >>> BIT(XE_HW_ENGINE_BCS0) | BIT(XE_HW_ENGINE_BCS1) |
> >>> BIT(XE_HW_ENGINE_BCS2) | BIT(XE_HW_ENGINE_BCS3) |
> >>> @@ -175,20 +159,12 @@ static const struct xe_graphics_desc graphics_xe2 = {
> >>> };
> >>>
> >>> static const struct xe_media_desc media_xem = {
> >>> - .name = "Xe_M",
> >>> - .ver = 12,
> >>> - .rel = 0,
> >>> -
> >>> .hw_engine_mask =
> >>> 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 = {
> >>> - .name = "Xe_HPM",
> >>> - .ver = 12,
> >>> - .rel = 55,
> >>> -
> >>> .hw_engine_mask =
> >>> GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
> >>> GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0),
> >>> @@ -369,6 +345,13 @@ static const struct xe_device_desc ptl_desc = {
> >>> #undef PLATFORM
> >>> __diag_pop();
> >>>
> >>> +static const struct xe_ip pre_gmdid_ips[] = {
> >>> + { 1200, "Xe_LP", &graphics_xelp },
> >>> + { 1210, "Xe_LP+", &graphics_xelpp },
> >>> + { 1255, "Xe_HPG", &graphics_xehpg },
> >>> + { 1260, "Xe_HPC", &graphics_xehpc },
> >>> +};
> >>> +
> >>> /* GMDID-based Graphics IPs */
> >>> static const struct xe_ip graphics_ips[] = {
> >>> { 1270, "Xe_LPG", &graphics_xelpg },
> >>> @@ -380,6 +363,11 @@ static const struct xe_ip graphics_ips[] = {
> >>> { 3001, "Xe3_LPG", &graphics_xe2 },
> >>> };
> >>>
> >>> +static const struct xe_ip pre_gmdid_media_ips[] = {
> >>> + { 1200, "Xe_M", &media_xem },
> >>> + { 1255, "Xe_HPM", &media_xehpm },
> >>> +};
> >>> +
> >>> /* GMDID-based Media IPs */
> >>> static const struct xe_ip media_ips[] = {
> >>> { 1300, "Xe_LPM+", &media_xelpmp },
> >>> @@ -558,12 +546,28 @@ static void handle_pre_gmdid(struct xe_device *xe,
> >>> const struct xe_graphics_desc *graphics,
> >>> const struct xe_media_desc *media)
> >>> {
> >>> - xe->info.graphics_verx100 = graphics->ver * 100 + graphics->rel;
> >>> - xe->info.graphics_name = graphics->name;
> >>> + for (int i = 0; i < ARRAY_SIZE(pre_gmdid_ips); i++) {
> >>> + if (pre_gmdid_ips[i].desc == graphics) {
> >>
> >>Unfortunately it doesn't appear that we can consolidate now-identical
> >>structures (e.g., graphics_xelp and graphics_xelpp) since doing so would
> >>always match the first entry in the table an incorrect assign a version
> >>of 12.00 instead of 12.10. I guess that's not really any worse than the
> >>current situation of having near-identical structures that only vary in
> >>version though.
> >>
> >>We might want to add a ktest to ensure that we never try to point
> >>multiple versions at the same structure for pre-GMD_ID IPs.
> >
> >Hm... I wonder if we could make graphics and media members of struct
> >xe_device_desc be pointers to struct xe_ip instead of
> >xe_{graphics,media}_desc. That would allow descriptor reuse and rule out
> >issues related to matching the same descriptor for different pre-GMDID
> >platforms.
>
> Also, with that, the for-loop wouldn't even be necessary.
Yeah, that sounds fine to me. It's basically the situation we have
today ("one pointer to something that has all the name/version/feature
info").
Matt
>
> --
> Gustavo Sousa
>
> >
> >What do you think?
> >
> >--
> >Gustavo Sousa
> >
> >>
> >>Anyway,
> >>
> >>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> >>
> >>
> >>> + xe->info.graphics_verx100 = pre_gmdid_ips[i].verx100;
> >>> + xe->info.graphics_name = pre_gmdid_ips[i].name;
> >>> +
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + xe_assert(xe, xe->info.graphics_verx100);
> >>>
> >>> if (media) {
> >>> - xe->info.media_verx100 = media->ver * 100 + media->rel;
> >>> - xe->info.media_name = media->name;
> >>> + for (int i = 0; i < ARRAY_SIZE(pre_gmdid_media_ips); i++) {
> >>> + if (pre_gmdid_media_ips[i].desc == media) {
> >>> + xe->info.media_verx100 = pre_gmdid_media_ips[i].verx100;
> >>> + xe->info.media_name = pre_gmdid_media_ips[i].name;
> >>> +
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + xe_assert(xe, xe->info.media_verx100);
> >>> } else {
> >>> xe->info.media_name = "none";
> >>> }
> >>> diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
> >>> index f46426ef8ed8..e9b9bbc138d3 100644
> >>> --- a/drivers/gpu/drm/xe/xe_pci_types.h
> >>> +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> >>> @@ -9,10 +9,6 @@
> >>> #include <linux/types.h>
> >>>
> >>> struct xe_graphics_desc {
> >>> - const char *name;
> >>> - u8 ver;
> >>> - u8 rel;
> >>> -
> >>> u8 va_bits;
> >>> u8 vm_max_level;
> >>> u8 vram_flags;
> >>> @@ -28,10 +24,6 @@ struct xe_graphics_desc {
> >>> };
> >>>
> >>> struct xe_media_desc {
> >>> - const char *name;
> >>> - u8 ver;
> >>> - u8 rel;
> >>> -
> >>> u64 hw_engine_mask; /* hardware engines provided by media IP */
> >>>
> >>> u8 has_indirect_ring_state:1;
> >>> --
> >>> 2.48.1
> >>>
> >>
> >>--
> >>Matt Roper
> >>Graphics Software Engineer
> >>Linux GPU Platform Enablement
> >>Intel Corporation
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list