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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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.

>
> 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;
> > > >  }
> > > >
> > >