[Intel-gfx] [PATCH 3/7] drm/i915: Move engine-related mmio init to engines_init_mmio

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2020-06-26 15:46:53)
> 
> 
> On 6/26/20 7:38 AM, Chris Wilson wrote:
> > Quoting Daniele Ceraolo Spurio (2020-06-26 00:42:08)
> >> All the info we read in intel_device_info_init_mmio are engine-related
> >> and since we already have an engine_init_mmio function we can just
> >> perform the operations from there.
> >>
> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >> Cc: Andi Shyti <andi.shyti at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 72 ++++++++++++++++++++++-
> >>   drivers/gpu/drm/i915/i915_drv.c           |  4 --
> >>   drivers/gpu/drm/i915/intel_device_info.c  | 66 ---------------------
> >>   drivers/gpu/drm/i915/intel_device_info.h  |  2 -
> >>   4 files changed, 71 insertions(+), 73 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> >> index be92d1ef9aa9..8497106eb3a6 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> >> @@ -450,6 +450,74 @@ void intel_engines_free(struct intel_gt *gt)
> >>          }
> >>   }
> >>   
> >> +/*
> >> + * Determine which engines are fused off in our particular hardware. Since the
> >> + * fuse register is in the blitter powerwell, we need forcewake to be ready at
> >> + * this point (but later we need to prune the forcewake domains for engines that
> >> + * are indeed fused off).
> >> + */
> >> +static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
> >> +{
> >> +       struct drm_i915_private *i915 = gt->i915;
> >> +       struct intel_device_info *info = mkwrite_device_info(i915);
> >> +       struct intel_uncore *uncore = gt->uncore;
> >> +       unsigned int logical_vdbox = 0;
> >> +       unsigned int i;
> >> +       u32 media_fuse;
> >> +       u16 vdbox_mask;
> >> +       u16 vebox_mask;
> > 
> > assert_forcewakes_active(uncore, FORCEWAKE_BLITTER) ?
> > 
> > Since it's called out in the comment, might as well reinforce that with
> > an assert.
> 
> We don't expect it to be active, just to be initialized/usable.
> This comment is there to remind us of the catch-22 issue we have where 
> we need forcewake to be initialized to read the fuses, but we need the 
> fuses values to know which engines are available and therefore which fw 
> domains are there (and that's why we prune the domains later).

Gotcha, I assumed 'ready' == 'powered up'

How about adding that explanation to the comment :)
-Chris