| Message ID | 20180829191056.63760-7-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New GuC ABI | expand |
+Lionel (please see below as this touches the lrca format & relates to OA reporting too) On 8/29/2018 12:10 PM, Michal Wajdeczko wrote: > Until now the GuC and HW engine class has been the same, which allowed > us to use them interchangeable. But it is better to start doing the > right thing and use the GuC definitions for the firmware interface. > > We also keep the same class id in the ctx descriptor to be able to have > the same values in the driver and firmware logs. > > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Tomasz Lis <tomasz.lis@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 13 +++++++++++++ > drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++++ > drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++++- > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ > 4 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 1a34e8f..bc81354 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -85,6 +85,7 @@ struct engine_info { > unsigned int hw_id; > unsigned int uabi_id; > u8 class; > + u8 guc_class; > u8 instance; > /* mmio bases table *must* be sorted in reverse gen order */ > struct engine_mmio_base { > @@ -98,6 +99,7 @@ struct engine_info { > .hw_id = RCS_HW, > .uabi_id = I915_EXEC_RENDER, > .class = RENDER_CLASS, > + .guc_class = GUC_RENDER_CLASS, > .instance = 0, > .mmio_bases = { > { .gen = 1, .base = RENDER_RING_BASE } > @@ -107,6 +109,7 @@ struct engine_info { > .hw_id = BCS_HW, > .uabi_id = I915_EXEC_BLT, > .class = COPY_ENGINE_CLASS, > + .guc_class = GUC_BLITTER_CLASS, > .instance = 0, > .mmio_bases = { > { .gen = 6, .base = BLT_RING_BASE } > @@ -116,6 +119,7 @@ struct engine_info { > .hw_id = VCS_HW, > .uabi_id = I915_EXEC_BSD, > .class = VIDEO_DECODE_CLASS, > + .guc_class = GUC_VIDEO_CLASS, > .instance = 0, > .mmio_bases = { > { .gen = 11, .base = GEN11_BSD_RING_BASE }, > @@ -127,6 +131,7 @@ struct engine_info { > .hw_id = VCS2_HW, > .uabi_id = I915_EXEC_BSD, > .class = VIDEO_DECODE_CLASS, > + .guc_class = GUC_VIDEO_CLASS, > .instance = 1, > .mmio_bases = { > { .gen = 11, .base = GEN11_BSD2_RING_BASE }, > @@ -137,6 +142,7 @@ struct engine_info { > .hw_id = VCS3_HW, > .uabi_id = I915_EXEC_BSD, > .class = VIDEO_DECODE_CLASS, > + .guc_class = GUC_VIDEO_CLASS, > .instance = 2, > .mmio_bases = { > { .gen = 11, .base = GEN11_BSD3_RING_BASE } > @@ -146,6 +152,7 @@ struct engine_info { > .hw_id = VCS4_HW, > .uabi_id = I915_EXEC_BSD, > .class = VIDEO_DECODE_CLASS, > + .guc_class = GUC_VIDEO_CLASS, > .instance = 3, > .mmio_bases = { > { .gen = 11, .base = GEN11_BSD4_RING_BASE } > @@ -155,6 +162,7 @@ struct engine_info { > .hw_id = VECS_HW, > .uabi_id = I915_EXEC_VEBOX, > .class = VIDEO_ENHANCEMENT_CLASS, > + .guc_class = GUC_VIDEOENHANCE_CLASS, > .instance = 0, > .mmio_bases = { > { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, > @@ -165,6 +173,7 @@ struct engine_info { > .hw_id = VECS2_HW, > .uabi_id = I915_EXEC_VEBOX, > .class = VIDEO_ENHANCEMENT_CLASS, > + .guc_class = GUC_VIDEOENHANCE_CLASS, > .instance = 1, > .mmio_bases = { > { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } > @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) > if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) > return -EINVAL; > > + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) > + return -EINVAL; > + > if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) > return -EINVAL; > > @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) > engine->i915 = dev_priv; > __sprint_engine_name(engine->name, info); > engine->hw_id = engine->guc_id = info->hw_id; > + engine->guc_class = info->guc_class; > engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); > engine->class = info->class; > engine->instance = info->instance; > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 963da91..5b7a05b 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -39,6 +39,13 @@ > #define GUC_VIDEO_ENGINE2 4 > #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) > > +#define GUC_RENDER_CLASS 0 > +#define GUC_VIDEO_CLASS 1 > +#define GUC_VIDEOENHANCE_CLASS 2 > +#define GUC_BLITTER_CLASS 3 > +#define GUC_RESERVED_CLASS 4 > +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) > + > /* Work queue item header definitions */ > #define WQ_STATUS_ACTIVE 1 > #define WQ_STATUS_SUSPENDED 2 > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index f8ceb9c..f4b9972 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct intel_engine_cs *engine, > > /* TODO: decide what to do with SW counter (bits 55-60) */ > > - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; > + /* > + * Although GuC will never see this upper part as it fills > + * its own descriptor, using the guc_class here will help keep > + * the i915 and firmware logs in sync. > + */ > + if (HAS_GUC_SCHED(ctx->i915)) > + desc |= (u64)engine->guc_class << GEN11_ENGINE_CLASS_SHIFT; > + else > + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; > /* bits 61-63 */ OA also uses this upper part (see oa_get_render_ctx_id), so it's something to be aware of. My opinion is that it's useful to have the lrca matching the firmware logs, but OA should account of this change at it receives what the fw sent to the hw. Which one is more important is for others to decide (plus it only becomes a problem when engine-class and guc-class start to deviate). Acked-by: Michel Thierry <michel.thierry@intel.com> -Michel > } else { > GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 3f6920d..f47009f 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -350,7 +350,9 @@ struct intel_engine_cs { > > enum intel_engine_id id; > unsigned int hw_id; > + > unsigned int guc_id; > + u8 guc_class; > > u8 uabi_id; > u8 uabi_class; >
On 29/08/2018 20:58, Michel Thierry wrote: > +Lionel > (please see below as this touches the lrca format & relates to OA > reporting too) > > On 8/29/2018 12:10 PM, Michal Wajdeczko wrote: >> Until now the GuC and HW engine class has been the same, which allowed >> us to use them interchangeable. But it is better to start doing the >> right thing and use the GuC definitions for the firmware interface. >> >> We also keep the same class id in the ctx descriptor to be able to have >> the same values in the driver and firmware logs. >> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: Tomasz Lis <tomasz.lis@intel.com> >> --- >> drivers/gpu/drm/i915/intel_engine_cs.c | 13 +++++++++++++ >> drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++++ >> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++++- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >> 4 files changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >> b/drivers/gpu/drm/i915/intel_engine_cs.c >> index 1a34e8f..bc81354 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -85,6 +85,7 @@ struct engine_info { >> unsigned int hw_id; >> unsigned int uabi_id; >> u8 class; >> + u8 guc_class; >> u8 instance; >> /* mmio bases table *must* be sorted in reverse gen order */ >> struct engine_mmio_base { >> @@ -98,6 +99,7 @@ struct engine_info { >> .hw_id = RCS_HW, >> .uabi_id = I915_EXEC_RENDER, >> .class = RENDER_CLASS, >> + .guc_class = GUC_RENDER_CLASS, >> .instance = 0, >> .mmio_bases = { >> { .gen = 1, .base = RENDER_RING_BASE } >> @@ -107,6 +109,7 @@ struct engine_info { >> .hw_id = BCS_HW, >> .uabi_id = I915_EXEC_BLT, >> .class = COPY_ENGINE_CLASS, >> + .guc_class = GUC_BLITTER_CLASS, >> .instance = 0, >> .mmio_bases = { >> { .gen = 6, .base = BLT_RING_BASE } >> @@ -116,6 +119,7 @@ struct engine_info { >> .hw_id = VCS_HW, >> .uabi_id = I915_EXEC_BSD, >> .class = VIDEO_DECODE_CLASS, >> + .guc_class = GUC_VIDEO_CLASS, >> .instance = 0, >> .mmio_bases = { >> { .gen = 11, .base = GEN11_BSD_RING_BASE }, >> @@ -127,6 +131,7 @@ struct engine_info { >> .hw_id = VCS2_HW, >> .uabi_id = I915_EXEC_BSD, >> .class = VIDEO_DECODE_CLASS, >> + .guc_class = GUC_VIDEO_CLASS, >> .instance = 1, >> .mmio_bases = { >> { .gen = 11, .base = GEN11_BSD2_RING_BASE }, >> @@ -137,6 +142,7 @@ struct engine_info { >> .hw_id = VCS3_HW, >> .uabi_id = I915_EXEC_BSD, >> .class = VIDEO_DECODE_CLASS, >> + .guc_class = GUC_VIDEO_CLASS, >> .instance = 2, >> .mmio_bases = { >> { .gen = 11, .base = GEN11_BSD3_RING_BASE } >> @@ -146,6 +152,7 @@ struct engine_info { >> .hw_id = VCS4_HW, >> .uabi_id = I915_EXEC_BSD, >> .class = VIDEO_DECODE_CLASS, >> + .guc_class = GUC_VIDEO_CLASS, >> .instance = 3, >> .mmio_bases = { >> { .gen = 11, .base = GEN11_BSD4_RING_BASE } >> @@ -155,6 +162,7 @@ struct engine_info { >> .hw_id = VECS_HW, >> .uabi_id = I915_EXEC_VEBOX, >> .class = VIDEO_ENHANCEMENT_CLASS, >> + .guc_class = GUC_VIDEOENHANCE_CLASS, >> .instance = 0, >> .mmio_bases = { >> { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, >> @@ -165,6 +173,7 @@ struct engine_info { >> .hw_id = VECS2_HW, >> .uabi_id = I915_EXEC_VEBOX, >> .class = VIDEO_ENHANCEMENT_CLASS, >> + .guc_class = GUC_VIDEOENHANCE_CLASS, >> .instance = 1, >> .mmio_bases = { >> { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } >> @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, >> const struct engine_info *info) >> if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >> return -EINVAL; >> + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) >> + return -EINVAL; >> + >> if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >> return -EINVAL; >> @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, >> const struct engine_info *info) >> engine->i915 = dev_priv; >> __sprint_engine_name(engine->name, info); >> engine->hw_id = engine->guc_id = info->hw_id; >> + engine->guc_class = info->guc_class; >> engine->mmio_base = __engine_mmio_base(dev_priv, >> info->mmio_bases); >> engine->class = info->class; >> engine->instance = info->instance; >> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> index 963da91..5b7a05b 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> @@ -39,6 +39,13 @@ >> #define GUC_VIDEO_ENGINE2 4 >> #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) >> +#define GUC_RENDER_CLASS 0 >> +#define GUC_VIDEO_CLASS 1 >> +#define GUC_VIDEOENHANCE_CLASS 2 >> +#define GUC_BLITTER_CLASS 3 >> +#define GUC_RESERVED_CLASS 4 >> +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) >> + >> /* Work queue item header definitions */ >> #define WQ_STATUS_ACTIVE 1 >> #define WQ_STATUS_SUSPENDED 2 >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >> b/drivers/gpu/drm/i915/intel_lrc.c >> index f8ceb9c..f4b9972 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct >> intel_engine_cs *engine, >> /* TODO: decide what to do with SW counter (bits 55-60) */ >> - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >> + /* >> + * Although GuC will never see this upper part as it fills >> + * its own descriptor, using the guc_class here will help keep >> + * the i915 and firmware logs in sync. >> + */ >> + if (HAS_GUC_SCHED(ctx->i915)) >> + desc |= (u64)engine->guc_class << GEN11_ENGINE_CLASS_SHIFT; >> + else >> + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >> /* bits 61-63 */ > > OA also uses this upper part (see oa_get_render_ctx_id), so it's > something to be aware of. > > My opinion is that it's useful to have the lrca matching the firmware > logs, but OA should account of this change at it receives what the fw > sent to the hw. Which one is more important is for others to decide > (plus it only becomes a problem when engine-class and guc-class start > to deviate). > > Acked-by: Michel Thierry <michel.thierry@intel.com> > > -Michel If GuC still behaves the same as the Gen9 firmware I was testing with, parts of the upper 32bits of the descriptor will end up in HW. Just make sure i915_perf.c is in sync with intel_lrc.c and it should be fine :) > > >> } else { >> GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >> b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index 3f6920d..f47009f 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >> @@ -350,7 +350,9 @@ struct intel_engine_cs { >> enum intel_engine_id id; >> unsigned int hw_id; >> + >> unsigned int guc_id; >> + u8 guc_class; >> u8 uabi_id; >> u8 uabi_class; >> >
On 2018-08-30 02:16, Lionel Landwerlin wrote: > On 29/08/2018 20:58, Michel Thierry wrote: >> +Lionel >> (please see below as this touches the lrca format & relates to OA >> reporting too) >> >> On 8/29/2018 12:10 PM, Michal Wajdeczko wrote: >>> Until now the GuC and HW engine class has been the same, which allowed >>> us to use them interchangeable. But it is better to start doing the >>> right thing and use the GuC definitions for the firmware interface. >>> >>> We also keep the same class id in the ctx descriptor to be able to have >>> the same values in the driver and firmware logs. >>> >>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Michel Thierry <michel.thierry@intel.com> >>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>> Cc: Tomasz Lis <tomasz.lis@intel.com> Tested-by: Tomasz Lis <tomasz.lis@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 13 +++++++++++++ >>> drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++++ >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++++- >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >>> 4 files changed, 31 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >>> b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index 1a34e8f..bc81354 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -85,6 +85,7 @@ struct engine_info { >>> unsigned int hw_id; >>> unsigned int uabi_id; >>> u8 class; >>> + u8 guc_class; >>> u8 instance; >>> /* mmio bases table *must* be sorted in reverse gen order */ >>> struct engine_mmio_base { >>> @@ -98,6 +99,7 @@ struct engine_info { >>> .hw_id = RCS_HW, >>> .uabi_id = I915_EXEC_RENDER, >>> .class = RENDER_CLASS, >>> + .guc_class = GUC_RENDER_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 1, .base = RENDER_RING_BASE } >>> @@ -107,6 +109,7 @@ struct engine_info { >>> .hw_id = BCS_HW, >>> .uabi_id = I915_EXEC_BLT, >>> .class = COPY_ENGINE_CLASS, >>> + .guc_class = GUC_BLITTER_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 6, .base = BLT_RING_BASE } >>> @@ -116,6 +119,7 @@ struct engine_info { >>> .hw_id = VCS_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD_RING_BASE }, >>> @@ -127,6 +131,7 @@ struct engine_info { >>> .hw_id = VCS2_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 1, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD2_RING_BASE }, >>> @@ -137,6 +142,7 @@ struct engine_info { >>> .hw_id = VCS3_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 2, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD3_RING_BASE } >>> @@ -146,6 +152,7 @@ struct engine_info { >>> .hw_id = VCS4_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 3, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD4_RING_BASE } >>> @@ -155,6 +162,7 @@ struct engine_info { >>> .hw_id = VECS_HW, >>> .uabi_id = I915_EXEC_VEBOX, >>> .class = VIDEO_ENHANCEMENT_CLASS, >>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, >>> @@ -165,6 +173,7 @@ struct engine_info { >>> .hw_id = VECS2_HW, >>> .uabi_id = I915_EXEC_VEBOX, >>> .class = VIDEO_ENHANCEMENT_CLASS, >>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>> .instance = 1, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } >>> @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, >>> const struct engine_info *info) >>> if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >>> return -EINVAL; >>> + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) >>> + return -EINVAL; >>> + >>> if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >>> return -EINVAL; >>> @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, >>> const struct engine_info *info) >>> engine->i915 = dev_priv; >>> __sprint_engine_name(engine->name, info); >>> engine->hw_id = engine->guc_id = info->hw_id; >>> + engine->guc_class = info->guc_class; >>> engine->mmio_base = __engine_mmio_base(dev_priv, >>> info->mmio_bases); >>> engine->class = info->class; >>> engine->instance = info->instance; >>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >>> b/drivers/gpu/drm/i915/intel_guc_fwif.h >>> index 963da91..5b7a05b 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >>> @@ -39,6 +39,13 @@ >>> #define GUC_VIDEO_ENGINE2 4 >>> #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) >>> +#define GUC_RENDER_CLASS 0 >>> +#define GUC_VIDEO_CLASS 1 >>> +#define GUC_VIDEOENHANCE_CLASS 2 >>> +#define GUC_BLITTER_CLASS 3 >>> +#define GUC_RESERVED_CLASS 4 >>> +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) >>> + >>> /* Work queue item header definitions */ >>> #define WQ_STATUS_ACTIVE 1 >>> #define WQ_STATUS_SUSPENDED 2 >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >>> b/drivers/gpu/drm/i915/intel_lrc.c >>> index f8ceb9c..f4b9972 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct >>> intel_engine_cs *engine, >>> /* TODO: decide what to do with SW counter (bits 55-60) */ >>> - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>> + /* >>> + * Although GuC will never see this upper part as it fills >>> + * its own descriptor, using the guc_class here will help keep >>> + * the i915 and firmware logs in sync. >>> + */ >>> + if (HAS_GUC_SCHED(ctx->i915)) >>> + desc |= (u64)engine->guc_class << >>> GEN11_ENGINE_CLASS_SHIFT; >>> + else >>> + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>> /* bits 61-63 */ >> >> OA also uses this upper part (see oa_get_render_ctx_id), so it's >> something to be aware of. >> >> My opinion is that it's useful to have the lrca matching the firmware >> logs, but OA should account of this change at it receives what the fw >> sent to the hw. Which one is more important is for others to decide >> (plus it only becomes a problem when engine-class and guc-class start >> to deviate). >> >> Acked-by: Michel Thierry <michel.thierry@intel.com> >> >> -Michel > > > If GuC still behaves the same as the Gen9 firmware I was testing with, > parts of the upper 32bits of the descriptor will end up in HW. > > Just make sure i915_perf.c is in sync with intel_lrc.c and it should > be fine :) > Tested on KBL; works fine for both enable_guc=2 and enable_guc=3. ./tests/perf --run-subtest=gen8-unprivileged-single-ctx-counters IGT-Version: 1.22-g11db680 (x86_64) (Linux: 4.19.0-rc1tli+ x86_64) Subtest gen8-unprivileged-single-ctx-counters: SUCCESS (0,058s) -Tomasz > >> >> >>> } else { >>> GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> index 3f6920d..f47009f 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> @@ -350,7 +350,9 @@ struct intel_engine_cs { >>> enum intel_engine_id id; >>> unsigned int hw_id; >>> + >>> unsigned int guc_id; >>> + u8 guc_class; >>> u8 uabi_id; >>> u8 uabi_class; >>> >> >
Uhh, sorry - answered on wrong patch. Please ignore this one. -Tomasz On 2018-08-30 15:29, Lis, Tomasz wrote: > > > On 2018-08-30 02:16, Lionel Landwerlin wrote: >> On 29/08/2018 20:58, Michel Thierry wrote: >>> +Lionel >>> (please see below as this touches the lrca format & relates to OA >>> reporting too) >>> >>> On 8/29/2018 12:10 PM, Michal Wajdeczko wrote: >>>> Until now the GuC and HW engine class has been the same, which allowed >>>> us to use them interchangeable. But it is better to start doing the >>>> right thing and use the GuC definitions for the firmware interface. >>>> >>>> We also keep the same class id in the ctx descriptor to be able to >>>> have >>>> the same values in the driver and firmware logs. >>>> >>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Michel Thierry <michel.thierry@intel.com> >>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>>> Cc: Tomasz Lis <tomasz.lis@intel.com> > Tested-by: Tomasz Lis <tomasz.lis@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_engine_cs.c | 13 +++++++++++++ >>>> drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++++ >>>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++++- >>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >>>> 4 files changed, 31 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >>>> b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> index 1a34e8f..bc81354 100644 >>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> @@ -85,6 +85,7 @@ struct engine_info { >>>> unsigned int hw_id; >>>> unsigned int uabi_id; >>>> u8 class; >>>> + u8 guc_class; >>>> u8 instance; >>>> /* mmio bases table *must* be sorted in reverse gen order */ >>>> struct engine_mmio_base { >>>> @@ -98,6 +99,7 @@ struct engine_info { >>>> .hw_id = RCS_HW, >>>> .uabi_id = I915_EXEC_RENDER, >>>> .class = RENDER_CLASS, >>>> + .guc_class = GUC_RENDER_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 1, .base = RENDER_RING_BASE } >>>> @@ -107,6 +109,7 @@ struct engine_info { >>>> .hw_id = BCS_HW, >>>> .uabi_id = I915_EXEC_BLT, >>>> .class = COPY_ENGINE_CLASS, >>>> + .guc_class = GUC_BLITTER_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 6, .base = BLT_RING_BASE } >>>> @@ -116,6 +119,7 @@ struct engine_info { >>>> .hw_id = VCS_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD_RING_BASE }, >>>> @@ -127,6 +131,7 @@ struct engine_info { >>>> .hw_id = VCS2_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 1, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD2_RING_BASE }, >>>> @@ -137,6 +142,7 @@ struct engine_info { >>>> .hw_id = VCS3_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 2, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD3_RING_BASE } >>>> @@ -146,6 +152,7 @@ struct engine_info { >>>> .hw_id = VCS4_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 3, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD4_RING_BASE } >>>> @@ -155,6 +162,7 @@ struct engine_info { >>>> .hw_id = VECS_HW, >>>> .uabi_id = I915_EXEC_VEBOX, >>>> .class = VIDEO_ENHANCEMENT_CLASS, >>>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, >>>> @@ -165,6 +173,7 @@ struct engine_info { >>>> .hw_id = VECS2_HW, >>>> .uabi_id = I915_EXEC_VEBOX, >>>> .class = VIDEO_ENHANCEMENT_CLASS, >>>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>>> .instance = 1, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } >>>> @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, >>>> const struct engine_info *info) >>>> if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >>>> return -EINVAL; >>>> + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) >>>> + return -EINVAL; >>>> + >>>> if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >>>> return -EINVAL; >>>> @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, >>>> const struct engine_info *info) >>>> engine->i915 = dev_priv; >>>> __sprint_engine_name(engine->name, info); >>>> engine->hw_id = engine->guc_id = info->hw_id; >>>> + engine->guc_class = info->guc_class; >>>> engine->mmio_base = __engine_mmio_base(dev_priv, >>>> info->mmio_bases); >>>> engine->class = info->class; >>>> engine->instance = info->instance; >>>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> b/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> index 963da91..5b7a05b 100644 >>>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> @@ -39,6 +39,13 @@ >>>> #define GUC_VIDEO_ENGINE2 4 >>>> #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) >>>> +#define GUC_RENDER_CLASS 0 >>>> +#define GUC_VIDEO_CLASS 1 >>>> +#define GUC_VIDEOENHANCE_CLASS 2 >>>> +#define GUC_BLITTER_CLASS 3 >>>> +#define GUC_RESERVED_CLASS 4 >>>> +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) >>>> + >>>> /* Work queue item header definitions */ >>>> #define WQ_STATUS_ACTIVE 1 >>>> #define WQ_STATUS_SUSPENDED 2 >>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >>>> b/drivers/gpu/drm/i915/intel_lrc.c >>>> index f8ceb9c..f4b9972 100644 >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>>> @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct >>>> intel_engine_cs *engine, >>>> /* TODO: decide what to do with SW counter (bits 55-60) */ >>>> - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>>> + /* >>>> + * Although GuC will never see this upper part as it fills >>>> + * its own descriptor, using the guc_class here will help >>>> keep >>>> + * the i915 and firmware logs in sync. >>>> + */ >>>> + if (HAS_GUC_SCHED(ctx->i915)) >>>> + desc |= (u64)engine->guc_class << >>>> GEN11_ENGINE_CLASS_SHIFT; >>>> + else >>>> + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>>> /* bits 61-63 */ >>> >>> OA also uses this upper part (see oa_get_render_ctx_id), so it's >>> something to be aware of. >>> >>> My opinion is that it's useful to have the lrca matching the >>> firmware logs, but OA should account of this change at it receives >>> what the fw sent to the hw. Which one is more important is for >>> others to decide (plus it only becomes a problem when engine-class >>> and guc-class start to deviate). >>> >>> Acked-by: Michel Thierry <michel.thierry@intel.com> >>> >>> -Michel >> >> >> If GuC still behaves the same as the Gen9 firmware I was testing >> with, parts of the upper 32bits of the descriptor will end up in HW. >> >> Just make sure i915_perf.c is in sync with intel_lrc.c and it should >> be fine :) >> > Tested on KBL; works fine for both enable_guc=2 and enable_guc=3. > > ./tests/perf --run-subtest=gen8-unprivileged-single-ctx-counters > IGT-Version: 1.22-g11db680 (x86_64) (Linux: 4.19.0-rc1tli+ x86_64) > Subtest gen8-unprivileged-single-ctx-counters: SUCCESS (0,058s) > > -Tomasz >> >>> >>> >>>> } else { >>>> GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); >>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> index 3f6920d..f47009f 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> @@ -350,7 +350,9 @@ struct intel_engine_cs { >>>> enum intel_engine_id id; >>>> unsigned int hw_id; >>>> + >>>> unsigned int guc_id; >>>> + u8 guc_class; >>>> u8 uabi_id; >>>> u8 uabi_class; >>>> >>> >> >
On 30/08/2018 14:29, Lis, Tomasz wrote: > > > On 2018-08-30 02:16, Lionel Landwerlin wrote: >> On 29/08/2018 20:58, Michel Thierry wrote: >>> +Lionel >>> (please see below as this touches the lrca format & relates to OA >>> reporting too) >>> >>> On 8/29/2018 12:10 PM, Michal Wajdeczko wrote: >>>> Until now the GuC and HW engine class has been the same, which allowed >>>> us to use them interchangeable. But it is better to start doing the >>>> right thing and use the GuC definitions for the firmware interface. >>>> >>>> We also keep the same class id in the ctx descriptor to be able to >>>> have >>>> the same values in the driver and firmware logs. >>>> >>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Michel Thierry <michel.thierry@intel.com> >>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>>> Cc: Tomasz Lis <tomasz.lis@intel.com> > Tested-by: Tomasz Lis <tomasz.lis@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_engine_cs.c | 13 +++++++++++++ >>>> drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++++ >>>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++++- >>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >>>> 4 files changed, 31 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >>>> b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> index 1a34e8f..bc81354 100644 >>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> @@ -85,6 +85,7 @@ struct engine_info { >>>> unsigned int hw_id; >>>> unsigned int uabi_id; >>>> u8 class; >>>> + u8 guc_class; >>>> u8 instance; >>>> /* mmio bases table *must* be sorted in reverse gen order */ >>>> struct engine_mmio_base { >>>> @@ -98,6 +99,7 @@ struct engine_info { >>>> .hw_id = RCS_HW, >>>> .uabi_id = I915_EXEC_RENDER, >>>> .class = RENDER_CLASS, >>>> + .guc_class = GUC_RENDER_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 1, .base = RENDER_RING_BASE } >>>> @@ -107,6 +109,7 @@ struct engine_info { >>>> .hw_id = BCS_HW, >>>> .uabi_id = I915_EXEC_BLT, >>>> .class = COPY_ENGINE_CLASS, >>>> + .guc_class = GUC_BLITTER_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 6, .base = BLT_RING_BASE } >>>> @@ -116,6 +119,7 @@ struct engine_info { >>>> .hw_id = VCS_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD_RING_BASE }, >>>> @@ -127,6 +131,7 @@ struct engine_info { >>>> .hw_id = VCS2_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 1, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD2_RING_BASE }, >>>> @@ -137,6 +142,7 @@ struct engine_info { >>>> .hw_id = VCS3_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 2, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD3_RING_BASE } >>>> @@ -146,6 +152,7 @@ struct engine_info { >>>> .hw_id = VCS4_HW, >>>> .uabi_id = I915_EXEC_BSD, >>>> .class = VIDEO_DECODE_CLASS, >>>> + .guc_class = GUC_VIDEO_CLASS, >>>> .instance = 3, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_BSD4_RING_BASE } >>>> @@ -155,6 +162,7 @@ struct engine_info { >>>> .hw_id = VECS_HW, >>>> .uabi_id = I915_EXEC_VEBOX, >>>> .class = VIDEO_ENHANCEMENT_CLASS, >>>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>>> .instance = 0, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, >>>> @@ -165,6 +173,7 @@ struct engine_info { >>>> .hw_id = VECS2_HW, >>>> .uabi_id = I915_EXEC_VEBOX, >>>> .class = VIDEO_ENHANCEMENT_CLASS, >>>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>>> .instance = 1, >>>> .mmio_bases = { >>>> { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } >>>> @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, >>>> const struct engine_info *info) >>>> if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >>>> return -EINVAL; >>>> + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) >>>> + return -EINVAL; >>>> + >>>> if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >>>> return -EINVAL; >>>> @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, >>>> const struct engine_info *info) >>>> engine->i915 = dev_priv; >>>> __sprint_engine_name(engine->name, info); >>>> engine->hw_id = engine->guc_id = info->hw_id; >>>> + engine->guc_class = info->guc_class; >>>> engine->mmio_base = __engine_mmio_base(dev_priv, >>>> info->mmio_bases); >>>> engine->class = info->class; >>>> engine->instance = info->instance; >>>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> b/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> index 963da91..5b7a05b 100644 >>>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >>>> @@ -39,6 +39,13 @@ >>>> #define GUC_VIDEO_ENGINE2 4 >>>> #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) >>>> +#define GUC_RENDER_CLASS 0 >>>> +#define GUC_VIDEO_CLASS 1 >>>> +#define GUC_VIDEOENHANCE_CLASS 2 >>>> +#define GUC_BLITTER_CLASS 3 >>>> +#define GUC_RESERVED_CLASS 4 >>>> +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) >>>> + >>>> /* Work queue item header definitions */ >>>> #define WQ_STATUS_ACTIVE 1 >>>> #define WQ_STATUS_SUSPENDED 2 >>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >>>> b/drivers/gpu/drm/i915/intel_lrc.c >>>> index f8ceb9c..f4b9972 100644 >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>>> @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct >>>> intel_engine_cs *engine, >>>> /* TODO: decide what to do with SW counter (bits 55-60) */ >>>> - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>>> + /* >>>> + * Although GuC will never see this upper part as it fills >>>> + * its own descriptor, using the guc_class here will help >>>> keep >>>> + * the i915 and firmware logs in sync. >>>> + */ >>>> + if (HAS_GUC_SCHED(ctx->i915)) >>>> + desc |= (u64)engine->guc_class << >>>> GEN11_ENGINE_CLASS_SHIFT; >>>> + else >>>> + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>>> /* bits 61-63 */ >>> >>> OA also uses this upper part (see oa_get_render_ctx_id), so it's >>> something to be aware of. >>> >>> My opinion is that it's useful to have the lrca matching the >>> firmware logs, but OA should account of this change at it receives >>> what the fw sent to the hw. Which one is more important is for >>> others to decide (plus it only becomes a problem when engine-class >>> and guc-class start to deviate). >>> >>> Acked-by: Michel Thierry <michel.thierry@intel.com> >>> >>> -Michel >> >> >> If GuC still behaves the same as the Gen9 firmware I was testing >> with, parts of the upper 32bits of the descriptor will end up in HW. >> >> Just make sure i915_perf.c is in sync with intel_lrc.c and it should >> be fine :) >> > Tested on KBL; works fine for both enable_guc=2 and enable_guc=3. > > ./tests/perf --run-subtest=gen8-unprivileged-single-ctx-counters > IGT-Version: 1.22-g11db680 (x86_64) (Linux: 4.19.0-rc1tli+ x86_64) > Subtest gen8-unprivileged-single-ctx-counters: SUCCESS (0,058s) But this patch affect Gen11, I think this is where you need to test, not on KBL. > > -Tomasz >> >>> >>> >>>> } else { >>>> GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); >>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> index 3f6920d..f47009f 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> @@ -350,7 +350,9 @@ struct intel_engine_cs { >>>> enum intel_engine_id id; >>>> unsigned int hw_id; >>>> + >>>> unsigned int guc_id; >>>> + u8 guc_class; >>>> u8 uabi_id; >>>> u8 uabi_class; >>>> >>> >> > >
On 29/08/18 17:16, Lionel Landwerlin wrote: > On 29/08/2018 20:58, Michel Thierry wrote: >> +Lionel >> (please see below as this touches the lrca format & relates to OA >> reporting too) >> >> On 8/29/2018 12:10 PM, Michal Wajdeczko wrote: >>> Until now the GuC and HW engine class has been the same, which allowed >>> us to use them interchangeable. But it is better to start doing the >>> right thing and use the GuC definitions for the firmware interface. >>> >>> We also keep the same class id in the ctx descriptor to be able to have >>> the same values in the driver and firmware logs. >>> >>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Michel Thierry <michel.thierry@intel.com> >>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>> Cc: Tomasz Lis <tomasz.lis@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 13 +++++++++++++ >>> drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++++ >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++++- >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >>> 4 files changed, 31 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >>> b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index 1a34e8f..bc81354 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -85,6 +85,7 @@ struct engine_info { >>> unsigned int hw_id; >>> unsigned int uabi_id; >>> u8 class; >>> + u8 guc_class; >>> u8 instance; >>> /* mmio bases table *must* be sorted in reverse gen order */ >>> struct engine_mmio_base { >>> @@ -98,6 +99,7 @@ struct engine_info { >>> .hw_id = RCS_HW, >>> .uabi_id = I915_EXEC_RENDER, >>> .class = RENDER_CLASS, >>> + .guc_class = GUC_RENDER_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 1, .base = RENDER_RING_BASE } >>> @@ -107,6 +109,7 @@ struct engine_info { >>> .hw_id = BCS_HW, >>> .uabi_id = I915_EXEC_BLT, >>> .class = COPY_ENGINE_CLASS, >>> + .guc_class = GUC_BLITTER_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 6, .base = BLT_RING_BASE } >>> @@ -116,6 +119,7 @@ struct engine_info { >>> .hw_id = VCS_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD_RING_BASE }, >>> @@ -127,6 +131,7 @@ struct engine_info { >>> .hw_id = VCS2_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 1, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD2_RING_BASE }, >>> @@ -137,6 +142,7 @@ struct engine_info { >>> .hw_id = VCS3_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 2, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD3_RING_BASE } >>> @@ -146,6 +152,7 @@ struct engine_info { >>> .hw_id = VCS4_HW, >>> .uabi_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> + .guc_class = GUC_VIDEO_CLASS, >>> .instance = 3, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_BSD4_RING_BASE } >>> @@ -155,6 +162,7 @@ struct engine_info { >>> .hw_id = VECS_HW, >>> .uabi_id = I915_EXEC_VEBOX, >>> .class = VIDEO_ENHANCEMENT_CLASS, >>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>> .instance = 0, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, >>> @@ -165,6 +173,7 @@ struct engine_info { >>> .hw_id = VECS2_HW, >>> .uabi_id = I915_EXEC_VEBOX, >>> .class = VIDEO_ENHANCEMENT_CLASS, >>> + .guc_class = GUC_VIDEOENHANCE_CLASS, >>> .instance = 1, >>> .mmio_bases = { >>> { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } >>> @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, >>> const struct engine_info *info) >>> if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >>> return -EINVAL; >>> + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) >>> + return -EINVAL; >>> + >>> if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >>> return -EINVAL; >>> @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, >>> const struct engine_info *info) >>> engine->i915 = dev_priv; >>> __sprint_engine_name(engine->name, info); >>> engine->hw_id = engine->guc_id = info->hw_id; >>> + engine->guc_class = info->guc_class; >>> engine->mmio_base = __engine_mmio_base(dev_priv, >>> info->mmio_bases); >>> engine->class = info->class; >>> engine->instance = info->instance; >>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >>> b/drivers/gpu/drm/i915/intel_guc_fwif.h >>> index 963da91..5b7a05b 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >>> @@ -39,6 +39,13 @@ >>> #define GUC_VIDEO_ENGINE2 4 >>> #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) >>> +#define GUC_RENDER_CLASS 0 >>> +#define GUC_VIDEO_CLASS 1 >>> +#define GUC_VIDEOENHANCE_CLASS 2 >>> +#define GUC_BLITTER_CLASS 3 >>> +#define GUC_RESERVED_CLASS 4 >>> +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) >>> + >>> /* Work queue item header definitions */ >>> #define WQ_STATUS_ACTIVE 1 >>> #define WQ_STATUS_SUSPENDED 2 >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >>> b/drivers/gpu/drm/i915/intel_lrc.c >>> index f8ceb9c..f4b9972 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct >>> intel_engine_cs *engine, >>> /* TODO: decide what to do with SW counter (bits 55-60) */ >>> - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>> + /* >>> + * Although GuC will never see this upper part as it fills >>> + * its own descriptor, using the guc_class here will help keep >>> + * the i915 and firmware logs in sync. >>> + */ >>> + if (HAS_GUC_SCHED(ctx->i915)) >>> + desc |= (u64)engine->guc_class << GEN11_ENGINE_CLASS_SHIFT; >>> + else >>> + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; >>> /* bits 61-63 */ >> >> OA also uses this upper part (see oa_get_render_ctx_id), so it's >> something to be aware of. >> >> My opinion is that it's useful to have the lrca matching the firmware >> logs, but OA should account of this change at it receives what the fw >> sent to the hw. Which one is more important is for others to decide >> (plus it only becomes a problem when engine-class and guc-class start >> to deviate). >> >> Acked-by: Michel Thierry <michel.thierry@intel.com> >> >> -Michel > > > If GuC still behaves the same as the Gen9 firmware I was testing with, > parts of the upper 32bits of the descriptor will end up in HW. > > Just make sure i915_perf.c is in sync with intel_lrc.c and it should be > fine :) > Not sure about having i915_perf.c is in sync with intel_lrc.c. As Michel said, GuC will submit to the HW using the "real" IDs (i.e. the ones used in intel_lrc the non-guc path) so I would expect those to show up in perf counters but in guc logs we have the "fake" ones (i.e. the ones using the guc_ids). In this patch intel_lrc uses the guc_ids and I don't think we want that for perf. The ids are currently the same in both paths so testing is not going to show any issue with this until they diverge. Thanks, Daniele > >> >> >>> } else { >>> GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> index 3f6920d..f47009f 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> @@ -350,7 +350,9 @@ struct intel_engine_cs { >>> enum intel_engine_id id; >>> unsigned int hw_id; >>> + >>> unsigned int guc_id; >>> + u8 guc_class; >>> u8 uabi_id; >>> u8 uabi_class; >>> >> >
Quoting Michal Wajdeczko (2018-08-29 22:10:40) > Until now the GuC and HW engine class has been the same, which allowed > us to use them interchangeable. But it is better to start doing the > right thing and use the GuC definitions for the firmware interface. > > We also keep the same class id in the ctx descriptor to be able to have > the same values in the driver and firmware logs. > > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Tomasz Lis <tomasz.lis@intel.com> <SNIP> > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct intel_engine_cs *engine, > > /* TODO: decide what to do with SW counter (bits 55-60) */ > > - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; > + /* > + * Although GuC will never see this upper part as it fills > + * its own descriptor, using the guc_class here will help keep > + * the i915 and firmware logs in sync. > + */ > + if (HAS_GUC_SCHED(ctx->i915)) > + desc |= (u64)engine->guc_class << GEN11_ENGINE_CLASS_SHIFT; > + else > + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; > /* bits 61-63 */ I'm fairly confident I've given this review comment long ago, but here goes again. The new member name should just be hw_class or something else agnostic, and the branching of using ELSP vs. GuC identifier should happen at the engine init time (or at another sweet spot). And then the actual write should be unconditionally with the hw_class value. We should not be adding GuC specifics to intel_lrc.c, I think. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 1a34e8f..bc81354 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -85,6 +85,7 @@ struct engine_info { unsigned int hw_id; unsigned int uabi_id; u8 class; + u8 guc_class; u8 instance; /* mmio bases table *must* be sorted in reverse gen order */ struct engine_mmio_base { @@ -98,6 +99,7 @@ struct engine_info { .hw_id = RCS_HW, .uabi_id = I915_EXEC_RENDER, .class = RENDER_CLASS, + .guc_class = GUC_RENDER_CLASS, .instance = 0, .mmio_bases = { { .gen = 1, .base = RENDER_RING_BASE } @@ -107,6 +109,7 @@ struct engine_info { .hw_id = BCS_HW, .uabi_id = I915_EXEC_BLT, .class = COPY_ENGINE_CLASS, + .guc_class = GUC_BLITTER_CLASS, .instance = 0, .mmio_bases = { { .gen = 6, .base = BLT_RING_BASE } @@ -116,6 +119,7 @@ struct engine_info { .hw_id = VCS_HW, .uabi_id = I915_EXEC_BSD, .class = VIDEO_DECODE_CLASS, + .guc_class = GUC_VIDEO_CLASS, .instance = 0, .mmio_bases = { { .gen = 11, .base = GEN11_BSD_RING_BASE }, @@ -127,6 +131,7 @@ struct engine_info { .hw_id = VCS2_HW, .uabi_id = I915_EXEC_BSD, .class = VIDEO_DECODE_CLASS, + .guc_class = GUC_VIDEO_CLASS, .instance = 1, .mmio_bases = { { .gen = 11, .base = GEN11_BSD2_RING_BASE }, @@ -137,6 +142,7 @@ struct engine_info { .hw_id = VCS3_HW, .uabi_id = I915_EXEC_BSD, .class = VIDEO_DECODE_CLASS, + .guc_class = GUC_VIDEO_CLASS, .instance = 2, .mmio_bases = { { .gen = 11, .base = GEN11_BSD3_RING_BASE } @@ -146,6 +152,7 @@ struct engine_info { .hw_id = VCS4_HW, .uabi_id = I915_EXEC_BSD, .class = VIDEO_DECODE_CLASS, + .guc_class = GUC_VIDEO_CLASS, .instance = 3, .mmio_bases = { { .gen = 11, .base = GEN11_BSD4_RING_BASE } @@ -155,6 +162,7 @@ struct engine_info { .hw_id = VECS_HW, .uabi_id = I915_EXEC_VEBOX, .class = VIDEO_ENHANCEMENT_CLASS, + .guc_class = GUC_VIDEOENHANCE_CLASS, .instance = 0, .mmio_bases = { { .gen = 11, .base = GEN11_VEBOX_RING_BASE }, @@ -165,6 +173,7 @@ struct engine_info { .hw_id = VECS2_HW, .uabi_id = I915_EXEC_VEBOX, .class = VIDEO_ENHANCEMENT_CLASS, + .guc_class = GUC_VIDEOENHANCE_CLASS, .instance = 1, .mmio_bases = { { .gen = 11, .base = GEN11_VEBOX2_RING_BASE } @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) return -EINVAL; + if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES)) + return -EINVAL; + if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) return -EINVAL; @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) engine->i915 = dev_priv; __sprint_engine_name(engine->name, info); engine->hw_id = engine->guc_id = info->hw_id; + engine->guc_class = info->guc_class; engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); engine->class = info->class; engine->instance = info->instance; diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 963da91..5b7a05b 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -39,6 +39,13 @@ #define GUC_VIDEO_ENGINE2 4 #define GUC_MAX_ENGINES_NUM (GUC_VIDEO_ENGINE2 + 1) +#define GUC_RENDER_CLASS 0 +#define GUC_VIDEO_CLASS 1 +#define GUC_VIDEOENHANCE_CLASS 2 +#define GUC_BLITTER_CLASS 3 +#define GUC_RESERVED_CLASS 4 +#define GUC_MAX_ENGINE_CLASSES (GUC_RESERVED_CLASS + 1) + /* Work queue item header definitions */ #define WQ_STATUS_ACTIVE 1 #define WQ_STATUS_SUSPENDED 2 diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index f8ceb9c..f4b9972 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct intel_engine_cs *engine, /* TODO: decide what to do with SW counter (bits 55-60) */ - desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; + /* + * Although GuC will never see this upper part as it fills + * its own descriptor, using the guc_class here will help keep + * the i915 and firmware logs in sync. + */ + if (HAS_GUC_SCHED(ctx->i915)) + desc |= (u64)engine->guc_class << GEN11_ENGINE_CLASS_SHIFT; + else + desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT; /* bits 61-63 */ } else { GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH)); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 3f6920d..f47009f 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -350,7 +350,9 @@ struct intel_engine_cs { enum intel_engine_id id; unsigned int hw_id; + unsigned int guc_id; + u8 guc_class; u8 uabi_id; u8 uabi_class;