[Intel-xe] [PATCH 2/4] drm/xe: Use dedicated function to read engine fuse registers

[Matt Roper]

On Wed, Nov 29, 2023 at 09:02:51AM -0600, Lucas De Marchi wrote:
> On Wed, Nov 15, 2023 at 03:38:36PM +0100, Michal Wajdeczko wrote:
> > These registers are not directly exposed to VFs, use function that
> > will provide required data also when running as a VF.
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_hw_engine.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index e831e63c5e48..1cf26c3c15c5 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -496,7 +496,7 @@ static void read_media_fuses(struct xe_gt *gt)
> > 
> > 	xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
> > 
> > -	media_fuse = xe_mmio_read32(gt, GT_VEBOX_VDBOX_DISABLE);
> > +	media_fuse = xe_mmio_fuse_read32(gt, GT_VEBOX_VDBOX_DISABLE);
> 
> I'm not sure about this approach as it's not really clear which register
> should or should not use this variant. For someone reading the code it
> will be very confusing. We already have the mcr variant... what if one
> of those registers is also MCR?
> 
> As an alternative, we could annotate the registers on their definition
> like we do for masked registers, and just tread them differently on the
> normal functions when in VF mode. See the first struct inside struct
> xe_reg. On register definition we would pass it as XE_REG_OPTION_XXXXXX
> 
> +Matt Roper
> 
> I'm unsure what's the best approach here without seeing the VF
> implementation. Just want to kickstart a discussion.

I don't think fuse registers are something that would ever be multicast
since they're "higher level" in the hardware design and not down inside
of specific hardware subunits.

I do like the idea of XE_REG_OPTION_XXXXXX since once you mark the
register that way once, any future code that accesses the same register
will automatically do the right thing, even if the person adding new
code isn't aware of the SRIOV-specific details.


Matt

> 
> Lucas De Marchi
> 
> 
> > 
> > 	/*
> > 	 * Pre-Xe_HP platforms had register bits representing absent engines,
> > @@ -541,7 +541,7 @@ static void read_copy_fuses(struct xe_gt *gt)
> > 
> > 	xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
> > 
> > -	bcs_mask = xe_mmio_read32(gt, MIRROR_FUSE3);
> > +	bcs_mask = xe_mmio_fuse_read32(gt, MIRROR_FUSE3);
> > 	bcs_mask = REG_FIELD_GET(MEML3_EN_MASK, bcs_mask);
> > 
> > 	/* BCS0 is always present; only BCS1-BCS8 may be fused off */
> > @@ -588,7 +588,7 @@ static void read_compute_fuses_from_reg(struct xe_gt *gt)
> > 	struct xe_device *xe = gt_to_xe(gt);
> > 	u32 ccs_mask;
> > 
> > -	ccs_mask = xe_mmio_read32(gt, XEHP_FUSE4);
> > +	ccs_mask = xe_mmio_fuse_read32(gt, XEHP_FUSE4);
> > 	ccs_mask = REG_FIELD_GET(CCS_EN_MASK, ccs_mask);
> > 
> > 	for (int i = XE_HW_ENGINE_CCS0, j = 0; i <= XE_HW_ENGINE_CCS3; ++i, ++j) {
> > -- 
> > 2.25.1
> > 

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