Re: [Intel-gfx] [PATCH 3/7] drm/i915/pcode: Extend pcode functions for multiple gt's

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

On Thu, 12 May 2022 03:36:31 -0700, Jani Nikula wrote:
>

Hi Jani,

> On Wed, 11 May 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> > Each gt contains an independent instance of pcode. Extend pcode functions
> > to interface with pcode on different gt's. To avoid creating dependency of
> > display functionality on intel_gt, pcode function interfaces are exposed in
> > terms of uncore rather than intel_gt. Callers have been converted to pass
> > in the appropritate (i915 or intel_gt) uncore to the pcode functions.
> >
> > v2: Expose pcode functions in terms of uncore rather than gt (Jani/Rodrigo)
> > v3: Retain previous function names to eliminate needless #defines (Rodrigo)
> > v4: Move out i915_pcode_init() to a separate patch (Tvrtko)
> >     Remove duplicated drm_err/drm_dbg from intel_pcode_init() (Tvrtko)
>
> Couple of nitpicks inline, and not insisting on changing. Basically ack
> on this from me.

Thanks!

> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > index 0c6b9eb724ae..90a440865037 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > @@ -138,7 +138,7 @@ static int gen6_drpc(struct seq_file *m)
> >	}
> >
> >	if (GRAPHICS_VER(i915) <= 7)
> > -		snb_pcode_read(i915, GEN6_PCODE_READ_RC6VIDS, &rc6vids, NULL);
> > +		snb_pcode_read(gt->uncore, GEN6_PCODE_READ_RC6VIDS, &rc6vids, NULL);
>
> Pedantically, I'm wondering if this (and similar places) should first be
> i915->uncore, to be replaced with gt->uncore in the next patch.

I did think about this and what you are suggesting is definitely possible
since we have an i915 variable. But on the other hand these structures are
already inside a gt and so 'i915->uncore' is really a 'gt->i915->uncore' so
someone might say why don't you just do a 'gt->uncore' which is the same as
'gt->i915->uncore'. But we could do it over two patches as you suggest.

> > diff --git a/drivers/gpu/drm/i915/intel_pcode.c b/drivers/gpu/drm/i915/intel_pcode.c
> > index ac727546868e..2be700932322 100644
> > --- a/drivers/gpu/drm/i915/intel_pcode.c
> > +++ b/drivers/gpu/drm/i915/intel_pcode.c
> > @@ -52,14 +52,12 @@ static int gen7_check_mailbox_status(u32 mbox)
> >	}
> >  }
> >
> > -static int __snb_pcode_rw(struct drm_i915_private *i915, u32 mbox,
> > +static int __snb_pcode_rw(struct intel_uncore *uncore, u32 mbox,
> >			  u32 *val, u32 *val1,
> >			  int fast_timeout_us, int slow_timeout_ms,
> >			  bool is_read)
> >  {
> > -	struct intel_uncore *uncore = &i915->uncore;
>
> Nitpick, personally, I would probably have just replaced the above with
>
>	struct drm_i915_private *i915 = uncore->i915;
>
> to minimize the diff. Ditto everywhere. But not a big deal.

Agreed.

Since you seem to be ok I am tending to not spin these patches again. But
if you feel otherwise please let me know and I can do it.

Thanks.
--
Ashutosh