[Intel-gfx] [PATCH 06/21] drm/i915/guc: Use guc_class instead of engine_class in fw interface
Lis, Tomasz
tomasz.lis at intel.com
Thu Aug 30 13:29:11 UTC 2018
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 at intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Michel Thierry <michel.thierry at intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>>> Cc: Tomasz Lis <tomasz.lis at intel.com>
Tested-by: Tomasz Lis <tomasz.lis at 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 at 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;
>>>
>>
>
More information about the Intel-gfx
mailing list