Re: [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface
On Fri, Jun 04, 2021 at 10:44:31AM +0200, Daniel Vetter wrote:
> On Wed, Jun 02, 2021 at 10:16:30PM -0700, Matthew Brost wrote:
> > From: Daniele Ceraolo Spurio
> >
> > GuC has its own defines for the engine classes. They're currently
> > mapping 1:1 to the defines used by the driver, but there is no guarantee
> > this will continue in the future. Given that we've been caught off-guard
> > in the past by similar divergences, we can prepare for the changes by
> > introducing helper functions to convert from engine class to GuC class and
> > back again.
> >
> > Signed-off-by: Daniele Ceraolo Spurio
> > Signed-off-by: Matthew Brost
> > Reviewed-by: Matthew Brost
> > Cc: John Harrison
> > Cc: Michal Wajdeczko
>
> Applied all up to this, except the Kconfig one.
>
Thanks, that one isn't really a blocker to next post which will update
the GuC firmware to the latest and greatest version.
Let me look at the issue with that patch now.
Matt
> Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 +++--
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 20 +---
> > drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 26 +
> > 3 files changed, 42 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index 3f9a811eb02b..69281b5aba51 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -265,6 +265,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum
> > intel_engine_id id)
> > const struct engine_info *info = _engines[id];
> > struct drm_i915_private *i915 = gt->i915;
> > struct intel_engine_cs *engine;
> > + u8 guc_class;
> >
> > BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
> > BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
> > @@ -293,9 +294,10 @@ static int intel_engine_setup(struct intel_gt *gt,
> > enum intel_engine_id id)
> > engine->i915 = i915;
> > engine->gt = gt;
> > engine->uncore = gt->uncore;
> > - engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
> > engine->hw_id = info->hw_id;
> > - engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
> > + guc_class = engine_class_to_guc_class(info->class);
> > + engine->guc_id = MAKE_GUC_ID(guc_class, info->instance);
> > + engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
> >
> > engine->irq_handler = nop_irq_handler;
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > index 17526717368c..efdce309b6f1 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -6,6 +6,7 @@
> > #include "gt/intel_gt.h"
> > #include "gt/intel_lrc.h"
> > #include "intel_guc_ads.h"
> > +#include "intel_guc_fwif.h"
> > #include "intel_uc.h"
> > #include "i915_drv.h"
> >
> > @@ -104,7 +105,7 @@ static void guc_mapping_table_init(struct intel_gt *gt,
> > GUC_MAX_INSTANCES_PER_CLASS;
> >
> > for_each_engine(engine, gt, id) {
> > - u8 guc_class = engine->class;
> > + u8 guc_class = engine_class_to_guc_class(engine->class);
> >
> > system_info->mapping_table[guc_class][engine->instance] =
> > engine->instance;
> > @@ -124,7 +125,7 @@ static void __guc_ads_init(struct intel_guc *guc)
> > struct __guc_ads_blob *blob = guc->ads_blob;
> > const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
> > u32 base;
> > - u8 engine_class;
> > + u8 engine_class, guc_class;
> >
> > /* GuC scheduling policies */
> > guc_policies_init(>policies);
> > @@ -140,22 +141,25 @@ static void __guc_ads_init(struct intel_guc *guc)
> > for (engine_class = 0; engine_class <= MAX_ENGINE_CLASS;
> > ++engine_class) {
> > if (engine_class == OTHER_CLASS)
> > continue;
> > +
> > + guc_class = engine_class_to_guc_class(engine_class);
> > +
> > /*
> > * TODO: Set context pointer to default state to allow
> > * GuC to re-init guilty contexts after internal reset.
> > */
> > - blob->ads.golden_context_lrca[engine_class] = 0;
> > - blob->ads.eng_state_size[engine_class] =
> > + blob->ads.golden_context_lrca[guc_class] = 0;
> > + blob->ads.eng_state_size[guc_class] =
> > intel_engine_context_size(guc_to_gt(guc),
> > engine_class) -
> > skipped_size;
> > }
> >
> > /* System info */
> > - blob->system_info.engine_enabled_masks[RENDER_CLASS] = 1;
> > - blob->system_info.engine_enabled_masks[COPY_ENGINE_CLASS] = 1;
> > - blob->system_info.engine_enabled_masks[VIDEO_DECODE_CLASS]
Re: [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface
On Wed, Jun 02, 2021 at 10:16:30PM -0700, Matthew Brost wrote:
> From: Daniele Ceraolo Spurio
>
> GuC has its own defines for the engine classes. They're currently
> mapping 1:1 to the defines used by the driver, but there is no guarantee
> this will continue in the future. Given that we've been caught off-guard
> in the past by similar divergences, we can prepare for the changes by
> introducing helper functions to convert from engine class to GuC class and
> back again.
>
> Signed-off-by: Daniele Ceraolo Spurio
> Signed-off-by: Matthew Brost
> Reviewed-by: Matthew Brost
> Cc: John Harrison
> Cc: Michal Wajdeczko
Applied all up to this, except the Kconfig one.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 +++--
> drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 20 +---
> drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 26 +
> 3 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 3f9a811eb02b..69281b5aba51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -265,6 +265,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum
> intel_engine_id id)
> const struct engine_info *info = _engines[id];
> struct drm_i915_private *i915 = gt->i915;
> struct intel_engine_cs *engine;
> + u8 guc_class;
>
> BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
> BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
> @@ -293,9 +294,10 @@ static int intel_engine_setup(struct intel_gt *gt, enum
> intel_engine_id id)
> engine->i915 = i915;
> engine->gt = gt;
> engine->uncore = gt->uncore;
> - engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
> engine->hw_id = info->hw_id;
> - engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
> + guc_class = engine_class_to_guc_class(info->class);
> + engine->guc_id = MAKE_GUC_ID(guc_class, info->instance);
> + engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
>
> engine->irq_handler = nop_irq_handler;
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index 17526717368c..efdce309b6f1 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -6,6 +6,7 @@
> #include "gt/intel_gt.h"
> #include "gt/intel_lrc.h"
> #include "intel_guc_ads.h"
> +#include "intel_guc_fwif.h"
> #include "intel_uc.h"
> #include "i915_drv.h"
>
> @@ -104,7 +105,7 @@ static void guc_mapping_table_init(struct intel_gt *gt,
> GUC_MAX_INSTANCES_PER_CLASS;
>
> for_each_engine(engine, gt, id) {
> - u8 guc_class = engine->class;
> + u8 guc_class = engine_class_to_guc_class(engine->class);
>
> system_info->mapping_table[guc_class][engine->instance] =
> engine->instance;
> @@ -124,7 +125,7 @@ static void __guc_ads_init(struct intel_guc *guc)
> struct __guc_ads_blob *blob = guc->ads_blob;
> const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
> u32 base;
> - u8 engine_class;
> + u8 engine_class, guc_class;
>
> /* GuC scheduling policies */
> guc_policies_init(>policies);
> @@ -140,22 +141,25 @@ static void __guc_ads_init(struct intel_guc *guc)
> for (engine_class = 0; engine_class <= MAX_ENGINE_CLASS;
> ++engine_class) {
> if (engine_class == OTHER_CLASS)
> continue;
> +
> + guc_class = engine_class_to_guc_class(engine_class);
> +
> /*
>* TODO: Set context pointer to default state to allow
>* GuC to re-init guilty contexts after internal reset.
>*/
> - blob->ads.golden_context_lrca[engine_class] = 0;
> - blob->ads.eng_state_size[engine_class] =
> + blob->ads.golden_context_lrca[guc_class] = 0;
> + blob->ads.eng_state_size[guc_class] =
> intel_engine_context_size(guc_to_gt(guc),
> engine_class) -
> skipped_size;
> }
>
> /* System info */
> - blob->system_info.engine_enabled_masks[RENDER_CLASS] = 1;
> - blob->system_info.engine_enabled_masks[COPY_ENGINE_CLASS] = 1;
> - blob->system_info.engine_enabled_masks[VIDEO_DECODE_CLASS] =
> VDBOX_MASK(gt);
> - blob->system_info.engine_enabled_masks[VIDEO_ENHANCEMENT_CLASS] =
> VEBOX_MASK(gt);
> + blob->system_info.engine_enabled_masks[GUC_RENDER_CLASS] = 1;
> + blob->system_info.engine_enabled_masks[GUC_BLITTER_CLASS] = 1;
> + blob->system_info.engine_enabled_masks[GUC_VIDEO_CLASS] =
> VDBOX_MASK(gt);
> +
