[Intel-xe] [PATCH v3 10/20] drm/xe: Don't "peek" into GMD_ID
Michał Winiarski
michal.winiarski at intel.com
Tue Nov 21 13:27:47 UTC 2023
On Mon, Nov 20, 2023 at 01:57:42PM -0600, Lucas De Marchi wrote:
> On Wed, Nov 15, 2023 at 01:51:00PM -0800, Matt Roper wrote:
> > On Tue, Nov 14, 2023 at 02:02:21PM +0100, Michał Winiarski wrote:
> > > Now that MMIO init got moved to device early, we can use regular
> > > xe_mmio_read helpers to get to GMD_ID register.
> > >
> > > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/regs/xe_gt_regs.h | 1 +
> > > drivers/gpu/drm/xe/xe_pci.c | 35 ++++++++--------------------
> > > 2 files changed, 11 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > index cc27fe8fc3633..6b195115564ce 100644
> > > --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > @@ -38,6 +38,7 @@
> > > #define GMD_ID_ARCH_MASK REG_GENMASK(31, 22)
> > > #define GMD_ID_RELEASE_MASK REG_GENMASK(21, 14)
> > > #define GMD_ID_REVID REG_GENMASK(5, 0)
> > > +#define MEDIA_GMD_ID XE_REG(GMD_ID.addr + MEDIA_GT_GSI_OFFSET)
> > >
> > > #define FORCEWAKE_ACK_GSC XE_REG(0xdf8)
> > > #define FORCEWAKE_ACK_GT_MTL XE_REG(0xdfc)
> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > index a1026b7ee4727..a6c08577b01d2 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > @@ -21,6 +21,7 @@
> > > #include "xe_drv.h"
> > > #include "xe_gt.h"
> > > #include "xe_macros.h"
> > > +#include "xe_mmio.h"
> > > #include "xe_module.h"
> > > #include "xe_pci_types.h"
> > > #include "xe_pm.h"
> > > @@ -441,29 +442,6 @@ find_subplatform(const struct xe_device *xe, const struct xe_device_desc *desc)
> > > return NULL;
> > > }
> > >
> > > -static void peek_gmdid(struct xe_device *xe, u32 gmdid_offset, u32 *ver, u32 *revid)
> > > -{
> > > - struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> > > - void __iomem *map = pci_iomap_range(pdev, 0, gmdid_offset, sizeof(u32));
> > > - u32 val;
> > > -
> > > - if (!map) {
> > > - drm_err(&xe->drm, "Failed to read GMD_ID (%#x) from PCI BAR.\n",
> > > - gmdid_offset);
> > > - *ver = 0;
> > > - *revid = 0;
> > > -
> > > - return;
> > > - }
> > > -
> > > - val = ioread32(map);
> > > - pci_iounmap(pdev, map);
> > > -
> > > - *ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 +
> > > - REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
> > > - *revid = REG_FIELD_GET(GMD_ID_REVID, val);
> > > -}
> > > -
> > > /*
> > > * Pre-GMD_ID platform: device descriptor already points to the appropriate
> > > * graphics descriptor. Simply forward the description and calculate the version
> > > @@ -495,9 +473,14 @@ static void handle_gmdid(struct xe_device *xe,
> > > u32 *graphics_revid,
> > > u32 *media_revid)
> > > {
> > > + struct xe_gt *gt = xe_root_mmio_gt(xe);
> > > + u32 val;
> > > u32 ver;
> > >
> > > - peek_gmdid(xe, GMD_ID.addr, &ver, graphics_revid);
> > > + val = xe_mmio_read32(gt, GMD_ID);
> > > + ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 + REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
> > > + *graphics_revid = REG_FIELD_GET(GMD_ID_REVID, val);
> > > +
> > > for (int i = 0; i < ARRAY_SIZE(graphics_ip_map); i++) {
> > > if (ver == graphics_ip_map[i].ver) {
> > > xe->info.graphics_verx100 = ver;
> > > @@ -512,7 +495,9 @@ static void handle_gmdid(struct xe_device *xe,
> > > ver / 100, ver % 100);
> > > }
> > >
> > > - peek_gmdid(xe, GMD_ID.addr + 0x380000, &ver, media_revid);
> > > + val = xe_mmio_read32(gt, MEDIA_GMD_ID);
> >
> > It's unfortunate that we have to read the GMD_ID register through the
> > wrong GT pointer, but I don't see a good way to avoid it. Maybe rather
> > than defining MEDIA_GMD_ID with the built-in offset (which people may
> > try to copy/paste for other register definitions without realizing this
> > is a one-time special case), we should just do something like
> >
> > struct xe_reg media_gmdid_reg = GMD_ID;
> >
> > /*
> > * Since we haven't initialized the media GT yet, we need to
> > * read media's GMD_ID register through the primary GT's
> > * mmio; that means we also need to explicitly add in the
> > * media GSI offset.
> > */
> > media_gmdid_reg.addr += MEDIA_GT_GSI_OFFSET;
> > val = xe_mmio_read32(gt, media_gmdid_reg);
>
>
> since we already have a new use case for having a peek_gmdid
> (https://patchwork.freedesktop.org/series/126609/), maybe rename the
> function and leave this logic adding MEDIA_GT_GSI_OFFSET inside that
> single function?
>
> with this patch getting a r-b, we already have all the pre-requisites
> reviewed so I'd be happy to rebase my series on top.
I'll bring back the "peek" as:
static void read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver, u32 *revid)
and use GMD_ID reg directly (with += MEDIA_GT_GSI_OFFSET for media).
Thanks,
-Michał
>
> Lucas De Marchi
>
> >
> >
> > Matt
> >
> > > + ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 + REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
> > > + *media_revid = REG_FIELD_GET(GMD_ID_REVID, val);
> > >
> > > /* Media may legitimately be fused off / not present */
> > > if (ver == 0)
> > > --
> > > 2.42.1
> > >
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation
More information about the Intel-xe
mailing list