[Intel-xe] [PATCH v3 10/20] drm/xe: Don't "peek" into GMD_ID
Lucas De Marchi
lucas.demarchi at intel.com
Tue Nov 21 16:54:22 UTC 2023
On Tue, Nov 21, 2023 at 02:27:47PM +0100, Michał Winiarski wrote:
>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).
with that, few free to add
Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
thanks
Lucas De Marchi
More information about the Intel-xe
mailing list