Re: [PATCH v1] drm/xe/throttle: Log throttle reasons

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Nov 15, 2024 at 06:56:33AM -0800, Dixit, Ashutosh wrote:
> On Fri, 15 Nov 2024 06:40:49 -0800, Raag Jadav wrote:
> >
> > On Fri, Nov 15, 2024 at 05:48:49AM -0800, Dixit, Ashutosh wrote:
> > > On Fri, 15 Nov 2024 05:12:08 -0800, Michal Wajdeczko wrote:
> > > > On 15.11.2024 13:56, Raag Jadav wrote:
> > > > > Log throttle register MMIO reads which will be useful for debugging.
> > > > >
> > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/xe/xe_gt_throttle.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/xe/xe_gt_throttle.c b/drivers/gpu/drm/xe/xe_gt_throttle.c
> > > > > index 03b225364101..0ce21ffc08dc 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_gt_throttle.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_gt_throttle.c
> > > > > @@ -37,15 +37,18 @@ dev_to_gt(struct device *dev)
> > > > >
> > > > >  u32 xe_gt_throttle_get_limit_reasons(struct xe_gt *gt)
> > > > >  {
> > > > > +	struct xe_device *xe = gt_to_xe(gt);
> > > > > +	bool media = xe_gt_is_media_type(gt);
> > >
> > > Also not sure what the point of these new variables is, previous code is
> > > fine. I actually prefer it to the new code.
> > >
> > > So this patch should be just a single line of xe_gt_dbg, IMO.
> >
> > We normally have temp variables for repeated uses. Makes the rest a bit
> > easier on the eyes.
> 
> Disagree. Adding additional lines to do unnecessary things makes the code
> harder to read, easier.

I agree with Ashutosh.

I liked the Matt Roper threashold: new gt/tile variables only for 3 or more
calls inside the same function.

But also, in this case here we do not have to print 'media' this information
we already know by knowing the gt. So, just use the gt dbg variant as Michal
pointed out.


> 
> >
> > Raag
> >
> > > > >	u32 reg;
> > > > >
> > > > > -	xe_pm_runtime_get(gt_to_xe(gt));
> > > > > -	if (xe_gt_is_media_type(gt))
> > > > > +	xe_pm_runtime_get(xe);
> > > > > +	if (media)
> > > > >		reg = xe_mmio_read32(&gt->mmio, MTL_MEDIA_PERF_LIMIT_REASONS);
> > > > >	else
> > > > >		reg = xe_mmio_read32(&gt->mmio, GT0_PERF_LIMIT_REASONS);
> > > > > -	xe_pm_runtime_put(gt_to_xe(gt));
> > > > > +	xe_pm_runtime_put(xe);
> > > > >
> > > > > +	drm_dbg(&xe->drm, "%s reg: 0x%x\n", media ? "media" : "gt", reg);
> > > >
> > > > we prefer GT oriented logs, so this should be:
> > > >
> > > >	xe_gt_dbg(gt, "....
> > > >
> > > > >	return reg;
> > > > >  }
> > > > >
> > > >