Re: [PATCH v7 2/7] drm/xe/guc: Add XE_LP steered register lists

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Wed, 2024-03-27 at 13:40 -0700, Zhanjun Dong wrote:
> Add the ability for runtime allocation and freeing of
> steered register list extentions that depend on the
> detected HW config fuses.
alan:snip...

> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -94,6 +94,8 @@
>  #define   FF_MODE2_TDS_TIMER_MASK              REG_GENMASK(23, 16)
>  #define   FF_MODE2_TDS_TIMER_128               REG_FIELD_PREP(FF_MODE2_TDS_TIMER_MASK, 4)
>  
> +#define XEHPG_INSTDONE_GEOM_SVG                        XE_REG_MCR(0x666c)
alan: i rather we follow the naming in the hw specs

> +
>  #define CACHE_MODE_1                           XE_REG(0x7004, XE_REG_OPTION_MASKED)
>  #define   MSAA_OPTIMIZATION_REDUC_DISABLE      REG_BIT(11)
>  
> @@ -323,6 +325,9 @@
>  #define   INVALIDATION_BROADCAST_MODE_DIS      REG_BIT(12)
>  #define   GLOBAL_INVALIDATION_MODE             REG_BIT(2)
>  
> +#define SAMPLER_INSTDONE                       XE_REG_MCR(0xe160)
> +#define ROW_INSTDONE                           XE_REG_MCR(0xe164)
alan: based on hw specs, we have an older version of ROW_INSTDONE and a newer
version at different offsets. Looks like it's one or the other based on the platform.
That said, you could use the newer one if we are not supporting the older platforms
on Xe (but that includes some of our internal testing platforms like MTL/ARL).
Actually i wonder if this means we have a gap on i915 MTL.

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index bc6b682998e2..bfa410f3a776 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
alan:snip ...

> +       { RING_BBADDR(0),           0,      0, "RING_BBADDR_LOW32" }, \
> +       { RING_BBADDR_UDW(0),       0,      0, "RING_BBADDR_UP32" }, \
> +       { RING_ACTHD(0),            0,      0, "ACTHD_LDW" }, \
> +       { RING_ACTHD_UDW(0),        0,      0, "ACTHD_UDW" }, \
> +       { RING_START(0),            0,      0, "START" }, \
alan:perhaps somethign we need to add? -> RING_BUFFER_START_UDW. Also lets check offline on those per engine PSMI
context debug regs, used to be there as different registers on legacy, but i dont see them in this list.
alan: nit: btw, in one of the earlier rev's reviews, i think someone asked about what the '0's were for.
I recall that we had cases where we wanted to use mask, but i dont see it now. If we dont have such
cases, we could just change the macro's definition to auto-insert the 0's making the list a bit
shorter. I personally prefer having this rows to be exactly as the structure is defined so there
is less "infered-magic" and readers can easily lookup exactly what the structure is defined as fully.
alan:snip...

> +static const struct __guc_mmio_reg_descr pre_xe_rc_inst_regs[] = {
alan: why are some of the names prefixed with "pre_xe_"? i would think that
everything in this patch is for xe platforms onwards. or was this agreed on
a prior review comment? (i dont wanna cause us going in circles). Or did i
misunderstand "xe" - where you were meaning Xe2 platforms while i was meaning
xe-kmd? if its the former, then you can change it to "pre_xe2" else if its
the latter, then just remove that prefix.
alan:snip...

> +static void __fill_ext_reg(struct __guc_mmio_reg_descr *ext,
> +                          const struct __ext_steer_reg *extlist,
> +                          int slice_id, int subslice_id)
> +{
> +       ext->reg = XE_REG(extlist->reg.__reg.addr);
> +       ext->flags = FIELD_PREP(GUC_REGSET_STEERING_GROUP, slice_id);
> +       ext->flags |= FIELD_PREP(GUC_REGSET_STEERING_INSTANCE, subslice_id);
> +       ext->regname = extlist->name;
alan: only now after forcing myself to relook at the fw specs, i notice that we may have missed a bit-flag for
"SteeringNeeded" in ext->flags (bit '1'). this too might be a bug in i915 (perhaps something we coule have missed
due to evolving specs that were new back then). lets connect offline and check with internal fw folks.
> +}
> +
> +static int
> +__alloc_ext_regs(struct __guc_mmio_reg_descr_group *newlist,
> +                const struct __guc_mmio_reg_descr_group *rootlist, int num_regs)
> +{
> +       struct __guc_mmio_reg_descr *list;
> +
> +       list = kcalloc(num_regs, sizeof(struct __guc_mmio_reg_descr), GFP_KERNEL);
alan: dont we have a drmm variation for kcalloc? else should we then use drmm_kzalloc instead?
> +       if (!list)
> +               return -ENOMEM;
> +
> +       newlist->extlist = list;
> +       newlist->num_regs = num_regs;
> +       newlist->owner = rootlist->owner;
> +       newlist->engine = rootlist->engine;
> +       newlist->type = rootlist->type;
> +
> +       return 0;
> +}
> +
> +static void
> +guc_capture_alloc_steered_lists(struct xe_guc *guc, const struct __guc_mmio_reg_descr_group *lists)
> +{
alan:snip...

> +
> +       /* allocate an extra for an end marker */
> +       extlists = kcalloc(2, sizeof(struct __guc_mmio_reg_descr_group), GFP_KERNEL);
alan: same here -use drmm variation. dont forget the kfree replacement too.
alan:snip...

> +
>  static const struct __guc_mmio_reg_descr_group *
>  guc_capture_get_device_reglist(struct xe_guc *guc)
>  {
> -       //FIXME: add register list
> -       return NULL;
> +       /*
> +        * For certain engine classes, there are slice and subslice
> +        * level registers requiring steering. We allocate and populate
> +        * these at init time based on hw config add it as an extension
> +        * list at the end of the pre-populated render list.
> +        */
> +       guc_capture_alloc_steered_lists(guc, xe_lp_lists);
> +
> +       return xe_lp_lists;
>  }
>