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

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

On Tue, 2022-04-19 at 22:54 -0700, Dixit, Ashutosh wrote:
> On Fri, 15 Apr 2022 03:21:26 -0700, Rodrigo Vivi wrote:
> > On Thu, Apr 14, 2022 at 03:31:07PM -0700, Dixit, Ashutosh wrote:
> > > On Thu, 14 Apr 2022 06:28:57 -0700, Jani Nikula wrote:
> > > > 
> > > > On Wed, 13 Apr 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. Previous (GT0)
> > > > > pcode read/write
> > > > > interfaces are preserved.
> > > > 
> > > > The big problem here is that this hard couples display code to
> > > > gt code,
> > > > while we're trying hard to go the opposite direction. It
> > > > doesn't matter
> > > > that the existing interfaces are preserved as wrappers when it
> > > > relies on
> > > > an intel_gt being available (via i915->gt0).
> > 
> > I don't believe there is a big problem in here...
> > 
> > please note the intel_pcode.h is keeping the abstraction for
> > display
> > 
> > #define snb_pcode_write_timeout(i915, mbox, val, fast_timeout_us,
> > slow_timeout_ms) \
> >         intel_gt_pcode_write_timeout(&(i915)->gt0, mbox, val,
> > fast_timeout_us, slow_timeout_ms)
> > 
> > #define snb_pcode_write(i915, mbox, val) \
> >         snb_pcode_write_timeout(i915, mbox, val, 500, 0)
> > 
> > display only uses these macros that Ashutosh didn't touch.
> > 
> > > > 
> > > > Note how 'git grep intel_gt -- drivers/gpu/drm/i915/display/'
> > > > matches
> > > > only 1 line.
> > 
> > As well with the patches applied:
> > 
> > $ git log --oneline -1
> > 1f58f1195478 (HEAD -> drm-tip) drm/i915/gt: Expose per-gt RPS
> > defaults in sysfs
> > 
> > $ git grep intel_gt -- drivers/gpu/drm/i915/display/
> > drivers/gpu/drm/i915/display/intel_display.c:          
> > intel_gt_set_wedged(to_gt(dev_priv));
> > 
> > > 
> > > Hi Jani, would you have suggestions about how to do this (handle
> > > pcode on
> > > multiple gt's)? The thinking was this patch would be a
> > > straightforward way
> > > to avoid code duplication. Also:
> > 
> > Maybe it is just a matter of renaming the macros used by display
> > in intel_pcode.h to reflect that it should be used by display only?
> 
> In v2 I have added a patch ([PATCH 4/9] drm/i915/gt: Convert callers
> to
> user per-gt pcode functions) which correctly calls per-gt pcode
> functions
> where this is required. With this patch only display functions (and
> one
> other caller) are left calling the "global scope"
> snb_pcode_read/write*
> functions. So the legacy snb_pcode_read/write* are now basically
> being used
> only by display. Let's see if Jani is ok with this. Thanks.

Jani is not happy with this abstraction because it still creates some
dependency and also no with the name intel_gt_pcode_ in the
functions...

He has some valid points.

I believe the right way to do this is to keep intel_pcode totally clean
from intel_gt and only receive intel_uncore as the argument. Then, if
needed we create display/intel_display_pcode and/or gt/intel_gt_pcode
with the needed abstractions... but better with none I'd say.