| Message ID | 8f667da9aa39452524abef1333226b645438d2cc.1649871650.git.ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Media freq factor and per-gt enhancements/fixes | expand |
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). Note how 'git grep intel_gt -- drivers/gpu/drm/i915/display/' matches only 1 line. BR, Jani. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Mike Ruhl <michael.j.ruhl@intel.com> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/intel_pcode.c | 108 ++++++++++++++++------------- > drivers/gpu/drm/i915/intel_pcode.h | 27 ++++++-- > 2 files changed, 82 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pcode.c b/drivers/gpu/drm/i915/intel_pcode.c > index ac727546868e..0cff212cc81b 100644 > --- a/drivers/gpu/drm/i915/intel_pcode.c > +++ b/drivers/gpu/drm/i915/intel_pcode.c > @@ -6,6 +6,7 @@ > #include "i915_drv.h" > #include "i915_reg.h" > #include "intel_pcode.h" > +#include "gt/intel_gt.h" > > static int gen6_check_mailbox_status(u32 mbox) > { > @@ -52,14 +53,14 @@ static int gen7_check_mailbox_status(u32 mbox) > } > } > > -static int __snb_pcode_rw(struct drm_i915_private *i915, u32 mbox, > - u32 *val, u32 *val1, > - int fast_timeout_us, int slow_timeout_ms, > - bool is_read) > +static int __gt_pcode_rw(struct intel_gt *gt, u32 mbox, > + u32 *val, u32 *val1, > + int fast_timeout_us, int slow_timeout_ms, > + bool is_read) > { > - struct intel_uncore *uncore = &i915->uncore; > + struct intel_uncore *uncore = gt->uncore; > > - lockdep_assert_held(&i915->sb_lock); > + lockdep_assert_held(>->i915->sb_lock); > > /* > * GEN6_PCODE_* are outside of the forcewake domain, we can use > @@ -88,60 +89,60 @@ static int __snb_pcode_rw(struct drm_i915_private *i915, u32 mbox, > if (is_read && val1) > *val1 = intel_uncore_read_fw(uncore, GEN6_PCODE_DATA1); > > - if (GRAPHICS_VER(i915) > 6) > + if (GRAPHICS_VER(gt->i915) > 6) > return gen7_check_mailbox_status(mbox); > else > return gen6_check_mailbox_status(mbox); > } > > -int snb_pcode_read(struct drm_i915_private *i915, u32 mbox, u32 *val, u32 *val1) > +int intel_gt_pcode_read(struct intel_gt *gt, u32 mbox, u32 *val, u32 *val1) > { > int err; > > - mutex_lock(&i915->sb_lock); > - err = __snb_pcode_rw(i915, mbox, val, val1, 500, 20, true); > - mutex_unlock(&i915->sb_lock); > + mutex_lock(>->i915->sb_lock); > + err = __gt_pcode_rw(gt, mbox, val, val1, 500, 20, true); > + mutex_unlock(>->i915->sb_lock); > > if (err) { > - drm_dbg(&i915->drm, > - "warning: pcode (read from mbox %x) mailbox access failed for %ps: %d\n", > - mbox, __builtin_return_address(0), err); > + drm_dbg(>->i915->drm, > + "gt %d: warning: pcode (read from mbox %x) mailbox access failed for %ps: %d\n", > + gt->info.id, mbox, __builtin_return_address(0), err); > } > > return err; > } > > -int snb_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox, u32 val, > - int fast_timeout_us, int slow_timeout_ms) > +int intel_gt_pcode_write_timeout(struct intel_gt *gt, u32 mbox, u32 val, > + int fast_timeout_us, int slow_timeout_ms) > { > int err; > > - mutex_lock(&i915->sb_lock); > - err = __snb_pcode_rw(i915, mbox, &val, NULL, > - fast_timeout_us, slow_timeout_ms, false); > - mutex_unlock(&i915->sb_lock); > + mutex_lock(>->i915->sb_lock); > + err = __gt_pcode_rw(gt, mbox, &val, NULL, > + fast_timeout_us, slow_timeout_ms, false); > + mutex_unlock(>->i915->sb_lock); > > if (err) { > - drm_dbg(&i915->drm, > - "warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n", > - val, mbox, __builtin_return_address(0), err); > + drm_dbg(>->i915->drm, > + "gt %d: warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n", > + gt->info.id, val, mbox, __builtin_return_address(0), err); > } > > return err; > } > > -static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox, > - u32 request, u32 reply_mask, u32 reply, > - u32 *status) > +static bool __gt_pcode_try_request(struct intel_gt *gt, u32 mbox, > + u32 request, u32 reply_mask, u32 reply, > + u32 *status) > { > - *status = __snb_pcode_rw(i915, mbox, &request, NULL, 500, 0, true); > + *status = __gt_pcode_rw(gt, mbox, &request, NULL, 500, 0, true); > > return (*status == 0) && ((request & reply_mask) == reply); > } > > /** > - * skl_pcode_request - send PCODE request until acknowledgment > - * @i915: device private > + * intel_gt_pcode_request - send PCODE request until acknowledgment > + * @gt: gt > * @mbox: PCODE mailbox ID the request is targeted for > * @request: request ID > * @reply_mask: mask used to check for request acknowledgment > @@ -158,16 +159,16 @@ static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox, > * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some > * other error as reported by PCODE. > */ > -int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request, > - u32 reply_mask, u32 reply, int timeout_base_ms) > +int intel_gt_pcode_request(struct intel_gt *gt, u32 mbox, u32 request, > + u32 reply_mask, u32 reply, int timeout_base_ms) > { > u32 status; > int ret; > > - mutex_lock(&i915->sb_lock); > + mutex_lock(>->i915->sb_lock); > > #define COND \ > - skl_pcode_try_request(i915, mbox, request, reply_mask, reply, &status) > + __gt_pcode_try_request(gt, mbox, request, reply_mask, reply, &status) > > /* > * Prime the PCODE by doing a request first. Normally it guarantees > @@ -193,35 +194,48 @@ int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request, > * requests, and for any quirks of the PCODE firmware that delays > * the request completion. > */ > - drm_dbg_kms(&i915->drm, > + drm_dbg_kms(>->i915->drm, > "PCODE timeout, retrying with preemption disabled\n"); > - drm_WARN_ON_ONCE(&i915->drm, timeout_base_ms > 3); > + drm_WARN_ON_ONCE(>->i915->drm, timeout_base_ms > 3); > preempt_disable(); > ret = wait_for_atomic(COND, 50); > preempt_enable(); > > out: > - mutex_unlock(&i915->sb_lock); > + mutex_unlock(>->i915->sb_lock); > return status ? status : ret; > #undef COND > } > > +static int __gt_pcode_init(struct intel_gt *gt) > +{ > + int ret = intel_gt_pcode_request(gt, DG1_PCODE_STATUS, > + DG1_UNCORE_GET_INIT_STATUS, > + DG1_UNCORE_INIT_STATUS_COMPLETE, > + DG1_UNCORE_INIT_STATUS_COMPLETE, 180000); > + > + drm_dbg(>->i915->drm, "gt %d: PCODE init status %d\n", gt->info.id, ret); > + > + if (ret) > + drm_err(>->i915->drm, "gt %d: Pcode did not report uncore initialization completion!\n", > + gt->info.id); > + > + return ret; > +} > + > int intel_pcode_init(struct drm_i915_private *i915) > { > - int ret = 0; > + struct intel_gt *gt; > + int i, ret = 0; > > if (!IS_DGFX(i915)) > return ret; > > - ret = skl_pcode_request(i915, DG1_PCODE_STATUS, > - DG1_UNCORE_GET_INIT_STATUS, > - DG1_UNCORE_INIT_STATUS_COMPLETE, > - DG1_UNCORE_INIT_STATUS_COMPLETE, 180000); > - > - drm_dbg(&i915->drm, "PCODE init status %d\n", ret); > - > - if (ret) > - drm_err(&i915->drm, "Pcode did not report uncore initialization completion!\n"); > + for_each_gt(gt, i915, i) { > + ret = __gt_pcode_init(gt); > + if (ret) > + return ret; > + } > > - return ret; > + return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_pcode.h b/drivers/gpu/drm/i915/intel_pcode.h > index 0962a17fac48..96c954ec91f9 100644 > --- a/drivers/gpu/drm/i915/intel_pcode.h > +++ b/drivers/gpu/drm/i915/intel_pcode.h > @@ -8,16 +8,31 @@ > > #include <linux/types.h> > > +struct intel_gt; > struct drm_i915_private; > > -int snb_pcode_read(struct drm_i915_private *i915, u32 mbox, u32 *val, u32 *val1); > -int snb_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox, u32 val, > - int fast_timeout_us, int slow_timeout_ms); > -#define snb_pcode_write(i915, mbox, val) \ > +int intel_gt_pcode_read(struct intel_gt *gt, u32 mbox, u32 *val, u32 *val1); > + > +int intel_gt_pcode_write_timeout(struct intel_gt *gt, u32 mbox, u32 val, > + int fast_timeout_us, int slow_timeout_ms); > + > +#define intel_gt_pcode_write(gt, mbox, val) \ > + intel_gt_pcode_write_timeout(gt, mbox, val, 500, 0) > + > +int intel_gt_pcode_request(struct intel_gt *gt, u32 mbox, u32 request, > + u32 reply_mask, u32 reply, int timeout_base_ms); > + > +#define snb_pcode_read(i915, mbox, val, val1) \ > + intel_gt_pcode_read(&(i915)->gt0, mbox, val, val1) > + > +#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) > > -int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request, > - u32 reply_mask, u32 reply, int timeout_base_ms); > +#define skl_pcode_request(i915, mbox, request, reply_mask, reply, timeout_base_ms) \ > + intel_gt_pcode_request(&(i915)->gt0, mbox, request, reply_mask, reply, timeout_base_ms) > > int intel_pcode_init(struct drm_i915_private *i915);
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). > > Note how 'git grep intel_gt -- drivers/gpu/drm/i915/display/' matches > only 1 line. 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: int intel_gt_probe_all() { ... /* * We always have at least one primary GT on any device * and it has been already initialized early during probe * in i915_driver_probe() */ Thanks.
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? Thanks, Rodrigo. > > int intel_gt_probe_all() { > ... > /* > * We always have at least one primary GT on any device > * and it has been already initialized early during probe > * in i915_driver_probe() > */ > > Thanks.
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.
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.
On Wed, 20 Apr 2022, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: > 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. I'd prefer it if you only passed uncore, not gt, to the pcode functions. BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_pcode.c b/drivers/gpu/drm/i915/intel_pcode.c index ac727546868e..0cff212cc81b 100644 --- a/drivers/gpu/drm/i915/intel_pcode.c +++ b/drivers/gpu/drm/i915/intel_pcode.c @@ -6,6 +6,7 @@ #include "i915_drv.h" #include "i915_reg.h" #include "intel_pcode.h" +#include "gt/intel_gt.h" static int gen6_check_mailbox_status(u32 mbox) { @@ -52,14 +53,14 @@ static int gen7_check_mailbox_status(u32 mbox) } } -static int __snb_pcode_rw(struct drm_i915_private *i915, u32 mbox, - u32 *val, u32 *val1, - int fast_timeout_us, int slow_timeout_ms, - bool is_read) +static int __gt_pcode_rw(struct intel_gt *gt, u32 mbox, + u32 *val, u32 *val1, + int fast_timeout_us, int slow_timeout_ms, + bool is_read) { - struct intel_uncore *uncore = &i915->uncore; + struct intel_uncore *uncore = gt->uncore; - lockdep_assert_held(&i915->sb_lock); + lockdep_assert_held(>->i915->sb_lock); /* * GEN6_PCODE_* are outside of the forcewake domain, we can use @@ -88,60 +89,60 @@ static int __snb_pcode_rw(struct drm_i915_private *i915, u32 mbox, if (is_read && val1) *val1 = intel_uncore_read_fw(uncore, GEN6_PCODE_DATA1); - if (GRAPHICS_VER(i915) > 6) + if (GRAPHICS_VER(gt->i915) > 6) return gen7_check_mailbox_status(mbox); else return gen6_check_mailbox_status(mbox); } -int snb_pcode_read(struct drm_i915_private *i915, u32 mbox, u32 *val, u32 *val1) +int intel_gt_pcode_read(struct intel_gt *gt, u32 mbox, u32 *val, u32 *val1) { int err; - mutex_lock(&i915->sb_lock); - err = __snb_pcode_rw(i915, mbox, val, val1, 500, 20, true); - mutex_unlock(&i915->sb_lock); + mutex_lock(>->i915->sb_lock); + err = __gt_pcode_rw(gt, mbox, val, val1, 500, 20, true); + mutex_unlock(>->i915->sb_lock); if (err) { - drm_dbg(&i915->drm, - "warning: pcode (read from mbox %x) mailbox access failed for %ps: %d\n", - mbox, __builtin_return_address(0), err); + drm_dbg(>->i915->drm, + "gt %d: warning: pcode (read from mbox %x) mailbox access failed for %ps: %d\n", + gt->info.id, mbox, __builtin_return_address(0), err); } return err; } -int snb_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox, u32 val, - int fast_timeout_us, int slow_timeout_ms) +int intel_gt_pcode_write_timeout(struct intel_gt *gt, u32 mbox, u32 val, + int fast_timeout_us, int slow_timeout_ms) { int err; - mutex_lock(&i915->sb_lock); - err = __snb_pcode_rw(i915, mbox, &val, NULL, - fast_timeout_us, slow_timeout_ms, false); - mutex_unlock(&i915->sb_lock); + mutex_lock(>->i915->sb_lock); + err = __gt_pcode_rw(gt, mbox, &val, NULL, + fast_timeout_us, slow_timeout_ms, false); + mutex_unlock(>->i915->sb_lock); if (err) { - drm_dbg(&i915->drm, - "warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n", - val, mbox, __builtin_return_address(0), err); + drm_dbg(>->i915->drm, + "gt %d: warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n", + gt->info.id, val, mbox, __builtin_return_address(0), err); } return err; } -static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox, - u32 request, u32 reply_mask, u32 reply, - u32 *status) +static bool __gt_pcode_try_request(struct intel_gt *gt, u32 mbox, + u32 request, u32 reply_mask, u32 reply, + u32 *status) { - *status = __snb_pcode_rw(i915, mbox, &request, NULL, 500, 0, true); + *status = __gt_pcode_rw(gt, mbox, &request, NULL, 500, 0, true); return (*status == 0) && ((request & reply_mask) == reply); } /** - * skl_pcode_request - send PCODE request until acknowledgment - * @i915: device private + * intel_gt_pcode_request - send PCODE request until acknowledgment + * @gt: gt * @mbox: PCODE mailbox ID the request is targeted for * @request: request ID * @reply_mask: mask used to check for request acknowledgment @@ -158,16 +159,16 @@ static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox, * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some * other error as reported by PCODE. */ -int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request, - u32 reply_mask, u32 reply, int timeout_base_ms) +int intel_gt_pcode_request(struct intel_gt *gt, u32 mbox, u32 request, + u32 reply_mask, u32 reply, int timeout_base_ms) { u32 status; int ret; - mutex_lock(&i915->sb_lock); + mutex_lock(>->i915->sb_lock); #define COND \ - skl_pcode_try_request(i915, mbox, request, reply_mask, reply, &status) + __gt_pcode_try_request(gt, mbox, request, reply_mask, reply, &status) /* * Prime the PCODE by doing a request first. Normally it guarantees @@ -193,35 +194,48 @@ int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request, * requests, and for any quirks of the PCODE firmware that delays * the request completion. */ - drm_dbg_kms(&i915->drm, + drm_dbg_kms(>->i915->drm, "PCODE timeout, retrying with preemption disabled\n"); - drm_WARN_ON_ONCE(&i915->drm, timeout_base_ms > 3); + drm_WARN_ON_ONCE(>->i915->drm, timeout_base_ms > 3); preempt_disable(); ret = wait_for_atomic(COND, 50); preempt_enable(); out: - mutex_unlock(&i915->sb_lock); + mutex_unlock(>->i915->sb_lock); return status ? status : ret; #undef COND } +static int __gt_pcode_init(struct intel_gt *gt) +{ + int ret = intel_gt_pcode_request(gt, DG1_PCODE_STATUS, + DG1_UNCORE_GET_INIT_STATUS, + DG1_UNCORE_INIT_STATUS_COMPLETE, + DG1_UNCORE_INIT_STATUS_COMPLETE, 180000); + + drm_dbg(>->i915->drm, "gt %d: PCODE init status %d\n", gt->info.id, ret); + + if (ret) + drm_err(>->i915->drm, "gt %d: Pcode did not report uncore initialization completion!\n", + gt->info.id); + + return ret; +} + int intel_pcode_init(struct drm_i915_private *i915) { - int ret = 0; + struct intel_gt *gt; + int i, ret = 0; if (!IS_DGFX(i915)) return ret; - ret = skl_pcode_request(i915, DG1_PCODE_STATUS, - DG1_UNCORE_GET_INIT_STATUS, - DG1_UNCORE_INIT_STATUS_COMPLETE, - DG1_UNCORE_INIT_STATUS_COMPLETE, 180000); - - drm_dbg(&i915->drm, "PCODE init status %d\n", ret); - - if (ret) - drm_err(&i915->drm, "Pcode did not report uncore initialization completion!\n"); + for_each_gt(gt, i915, i) { + ret = __gt_pcode_init(gt); + if (ret) + return ret; + } - return ret; + return 0; } diff --git a/drivers/gpu/drm/i915/intel_pcode.h b/drivers/gpu/drm/i915/intel_pcode.h index 0962a17fac48..96c954ec91f9 100644 --- a/drivers/gpu/drm/i915/intel_pcode.h +++ b/drivers/gpu/drm/i915/intel_pcode.h @@ -8,16 +8,31 @@ #include <linux/types.h> +struct intel_gt; struct drm_i915_private; -int snb_pcode_read(struct drm_i915_private *i915, u32 mbox, u32 *val, u32 *val1); -int snb_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox, u32 val, - int fast_timeout_us, int slow_timeout_ms); -#define snb_pcode_write(i915, mbox, val) \ +int intel_gt_pcode_read(struct intel_gt *gt, u32 mbox, u32 *val, u32 *val1); + +int intel_gt_pcode_write_timeout(struct intel_gt *gt, u32 mbox, u32 val, + int fast_timeout_us, int slow_timeout_ms); + +#define intel_gt_pcode_write(gt, mbox, val) \ + intel_gt_pcode_write_timeout(gt, mbox, val, 500, 0) + +int intel_gt_pcode_request(struct intel_gt *gt, u32 mbox, u32 request, + u32 reply_mask, u32 reply, int timeout_base_ms); + +#define snb_pcode_read(i915, mbox, val, val1) \ + intel_gt_pcode_read(&(i915)->gt0, mbox, val, val1) + +#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) -int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request, - u32 reply_mask, u32 reply, int timeout_base_ms); +#define skl_pcode_request(i915, mbox, request, reply_mask, reply, timeout_base_ms) \ + intel_gt_pcode_request(&(i915)->gt0, mbox, request, reply_mask, reply, timeout_base_ms) int intel_pcode_init(struct drm_i915_private *i915);
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. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Mike Ruhl <michael.j.ruhl@intel.com> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/intel_pcode.c | 108 ++++++++++++++++------------- drivers/gpu/drm/i915/intel_pcode.h | 27 ++++++-- 2 files changed, 82 insertions(+), 53 deletions(-)