On Thu, 12 May 2022 03:36:31 -0700, Jani Nikula wrote: > Hi Jani,
> On Wed, 11 May 2022, Ashutosh Dixit <[email protected]> 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
