[PATCH 2/3] drm/xe/guc: Use the steering flag when printing registers

[Matt Roper]

On Thu, Apr 17, 2025 at 01:45:32PM -0700, John Harrison wrote:
> On 4/17/2025 1:17 PM, Matt Roper wrote:
> > On Thu, Apr 17, 2025 at 12:52:13PM -0700, John.C.Harrison at Intel.com wrote:
> > > From: John Harrison <John.C.Harrison at Intel.com>
> > > 
> > > The printing code was doing a test on which list a register was in to
> > > decide whether it is steered or not. That might be valid at this
> > > moment but there may be other reasons for extended lists in the
> > > future. Plus, there is a flag specifically for identifying steered
> > > registers. So, just use that instead - it is simpler and safer.
> > > 
> > > Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_guc_capture.c | 6 ++----
> > >   1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> > > index 9095618648bc..f4b08136e472 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> > > @@ -1672,18 +1672,16 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
> > >   {
> > >   	struct xe_gt *gt = snapshot->hwe->gt;
> > >   	struct xe_device *xe = gt_to_xe(gt);
> > > -	struct xe_guc *guc = &gt->uc.guc;
> > >   	struct xe_devcoredump *devcoredump = &xe->devcoredump;
> > >   	struct xe_devcoredump_snapshot *devcore_snapshot = &devcoredump->snapshot;
> > >   	struct gcap_reg_list_info *reginfo = NULL;
> > >   	u32 i, last_value = 0;
> > > -	bool is_ext, low32_ready = false;
> > > +	bool low32_ready = false;
> > >   	if (!list || !list->list || list->num_regs == 0)
> > >   		return;
> > >   	XE_WARN_ON(!devcore_snapshot->matched_node);
> > > -	is_ext = list == guc->capture->extlists;
> > >   	reginfo = &devcore_snapshot->matched_node->reginfo[type];
> > >   	/*
> > > @@ -1749,7 +1747,7 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
> > >   			 */
> > >   			XE_WARN_ON(low32_ready);
> > > -			if (is_ext) {
> > > +			if (FIELD_GET(GUC_REGSET_STEERING_NEEDED, reg_desc->flags)) {
> > This is a single-bit flag, so can't we just do a simple '&' rather than
> > using FIELD_GET?
> I was just being consistent with the other usages. Could convert them all to
> bit ops instead, but having a mix seems wrong.

Okay, fair enough.  We can convert all the flag matches some other time.
The logic here looks fine, so

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>


Matt

> 
> John.
> 
> > 
> > 
> > Matt
> > 
> > >   				int dss, group, instance;
> > >   				group = FIELD_GET(GUC_REGSET_STEERING_GROUP, reg_desc->flags);
> > > -- 
> > > 2.49.0
> > > 
> > > 
> 

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