| Message ID | 20180213163738.9055-13-mika.kuoppala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 13 Feb 2018 17:37:30 +0100, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > From: Oscar Mateo <oscar.mateo@intel.com> > > In Gen11, the Video Decode engines (aka VDBOX, aka VCS, aka BSD) and the > Video Enhancement engines (aka VEBOX, aka VECS) could be fused off. Also, > each VDBOX and VEBOX has its own power well, which only exist if the > related > engine exists in the HW. > > Unfortunately, we have a Catch-22 situation going on: we need to read an > MMIO register with the fuse info, but we cannot fully enable MMIO until > we read it (since we need the real engines to initialize the forcewake > domains). We workaround this problem by reading the fuse after the MMIO > is partially ready, but before we initialize forcewake. > > Bspec: 20680 > > v2: We were shifting incorrectly for vebox disable (Vinay) > > v3: Assert mmio is ready and warn if we have attempted to initialize > forcewake for fused-off engines (Paulo) > > v4: > - Use INTEL_GEN in new code (Tvrtko) > - Shorter local variable (Tvrtko, Michal) > - Keep "if (!...) continue" style (Tvrtko) > - No unnecessary BUG_ON (Tvrtko) > - WARN_ON and cleanup if wrong mask (Tvrtko, Michal) > - Use I915_READ_FW (Michal) > - Use I915_MAX_VCS/VECS macros (Michal) > > v5: Rebased by Rodrigo fixing conflicts on top of: > commit 33def1ff7b0 ("drm/i915: Simplify intel_engines_init") > > v6: Fix v5. Remove info->num_rings. (by Oscar) > > v7: Rebase (Rodrigo). > > v8: > - s/intel_device_info_fused_off_engines/intel_device_info_init_mmio > (Chris) > - Make vdbox_disable & vebox_disable local variables (Chris) > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 5 +++ > drivers/gpu/drm/i915/intel_device_info.c | 54 > ++++++++++++++++++++++++++++++++ > 4 files changed, 62 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index 9380c9f69b0f..43b2f620bca7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1033,6 +1033,8 @@ static int i915_driver_init_mmio(struct > drm_i915_private *dev_priv) > if (ret < 0) > goto err_bridge; > + intel_device_info_init_mmio(dev_priv); > + > intel_uncore_init(dev_priv); > intel_uc_init_mmio(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 65e674668b2e..ba16c2025364 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3438,6 +3438,7 @@ void i915_unreserve_fence(struct > drm_i915_fence_reg *fence); > void i915_gem_revoke_fences(struct drm_i915_private *dev_priv); > void i915_gem_restore_fences(struct drm_i915_private *dev_priv); > +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv); This function should be declared in "intel_device_info.h" > void i915_gem_detect_bit_6_swizzle(struct drm_i915_private *dev_priv); > void i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj, > struct sg_table *pages); > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > index b6cd725ff0b7..2b8d3a13dd27 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2860,6 +2860,11 @@ enum i915_power_well_id { > #define GEN10_EU_DISABLE3 _MMIO(0x9140) > #define GEN10_EU_DIS_SS_MASK 0xff > +#define GEN11_GT_VEBOX_VDBOX_DISABLE _MMIO(0x9140) > +#define GEN11_GT_VDBOX_DISABLE_MASK 0xff > +#define GEN11_GT_VEBOX_DISABLE_SHIFT 16 > +#define GEN11_GT_VEBOX_DISABLE_MASK (0xff << > GEN11_GT_VEBOX_DISABLE_SHIFT) > + Missing indent (2 spaces) for above bit fields definitions. > #define GEN6_BSD_SLEEP_PSMI_CONTROL _MMIO(0x12050) > #define GEN6_BSD_SLEEP_MSG_DISABLE (1 << 0) > #define GEN6_BSD_SLEEP_FLUSH_DISABLE (1 << 2) > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > b/drivers/gpu/drm/i915/intel_device_info.c > index 9352f34e75c4..7c8779faf162 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -595,3 +595,57 @@ void intel_driver_caps_print(const struct > intel_driver_caps *caps, > { > drm_printf(p, "scheduler: %x\n", caps->scheduler); > } > + > +/* > + * Determine which engines are fused off in our particular hardware. > + * > + * This function needs to be called after the MMIO has been setup (as > we need > + * to read registers) but before uncore init (because the powerwell for > the > + * fused off engines doesn't exist, so we cannot initialize forcewake > for them) > + */ > +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv) > +{ > + struct intel_device_info *info = mkwrite_device_info(dev_priv); > + u8 vdbox_disable, vebox_disable; > + u32 media_fuse; > + int i; > + > + if (INTEL_GEN(dev_priv) < 11) > + return; > + > + GEM_BUG_ON(!dev_priv->regs); > + > + media_fuse = I915_READ_FW(GEN11_GT_VEBOX_VDBOX_DISABLE); > + > + vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK; > + vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >> > + GEN11_GT_VEBOX_DISABLE_SHIFT; > + > + DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable); > + for (i = 0; i < I915_MAX_VCS; i++) { > + if (!HAS_ENGINE(dev_priv, _VCS(i))) > + continue; > + > + if (!(BIT(i) & vdbox_disable)) > + continue; > + > + info->ring_mask &= ~ENGINE_MASK(_VCS(i)); > + WARN_ON(dev_priv->uncore.fw_domains & > + BIT(FW_DOMAIN_ID_MEDIA_VDBOX0 + i)); > + DRM_DEBUG_DRIVER("vcs%u fused off\n", i); > + } > + > + DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable); > + for (i = 0; i < I915_MAX_VECS; i++) { > + if (!HAS_ENGINE(dev_priv, _VECS(i))) > + continue; > + > + if (!(BIT(i) & vebox_disable)) > + continue; > + > + info->ring_mask &= ~ENGINE_MASK(_VECS(i)); > + WARN_ON(dev_priv->uncore.fw_domains & > + BIT(FW_DOMAIN_ID_MEDIA_VEBOX0 + i)); > + DRM_DEBUG_DRIVER("vecs%u fused off\n", i); > + } > +}
On 2/13/2018 10:07 PM, Mika Kuoppala wrote: > From: Oscar Mateo <oscar.mateo@intel.com> > > In Gen11, the Video Decode engines (aka VDBOX, aka VCS, aka BSD) and the > Video Enhancement engines (aka VEBOX, aka VECS) could be fused off. Also, > each VDBOX and VEBOX has its own power well, which only exist if the related > engine exists in the HW. > > Unfortunately, we have a Catch-22 situation going on: we need to read an > MMIO register with the fuse info, but we cannot fully enable MMIO until > we read it (since we need the real engines to initialize the forcewake > domains). We need to ensure BLITTER is initialized first and use low level functions fw_domains_get/put() around raw read to know these engines status. > We workaround this problem by reading the fuse after the MMIO > is partially ready, but before we initialize forcewake. > > Bspec: 20680 > > v2: We were shifting incorrectly for vebox disable (Vinay) > > v3: Assert mmio is ready and warn if we have attempted to initialize > forcewake for fused-off engines (Paulo) > > v4: > - Use INTEL_GEN in new code (Tvrtko) > - Shorter local variable (Tvrtko, Michal) > - Keep "if (!...) continue" style (Tvrtko) > - No unnecessary BUG_ON (Tvrtko) > - WARN_ON and cleanup if wrong mask (Tvrtko, Michal) > - Use I915_READ_FW (Michal) > - Use I915_MAX_VCS/VECS macros (Michal) > > v5: Rebased by Rodrigo fixing conflicts on top of: > commit 33def1ff7b0 ("drm/i915: Simplify intel_engines_init") > > v6: Fix v5. Remove info->num_rings. (by Oscar) > > v7: Rebase (Rodrigo). > > v8: > - s/intel_device_info_fused_off_engines/intel_device_info_init_mmio (Chris) > - Make vdbox_disable & vebox_disable local variables (Chris) > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 5 +++ > drivers/gpu/drm/i915/intel_device_info.c | 54 ++++++++++++++++++++++++++++++++ > 4 files changed, 62 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9380c9f69b0f..43b2f620bca7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1033,6 +1033,8 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > if (ret < 0) > goto err_bridge; > > + intel_device_info_init_mmio(dev_priv); This should be called during intel_uncore_fw_domains_init after fw_domain_init(.., FW_DOMAIN_ID_BLITTER, ..); > + > intel_uncore_init(dev_priv); > > intel_uc_init_mmio(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 65e674668b2e..ba16c2025364 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3438,6 +3438,7 @@ void i915_unreserve_fence(struct drm_i915_fence_reg *fence); > void i915_gem_revoke_fences(struct drm_i915_private *dev_priv); > void i915_gem_restore_fences(struct drm_i915_private *dev_priv); > > +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv); > void i915_gem_detect_bit_6_swizzle(struct drm_i915_private *dev_priv); > void i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj, > struct sg_table *pages); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b6cd725ff0b7..2b8d3a13dd27 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2860,6 +2860,11 @@ enum i915_power_well_id { > #define GEN10_EU_DISABLE3 _MMIO(0x9140) > #define GEN10_EU_DIS_SS_MASK 0xff > > +#define GEN11_GT_VEBOX_VDBOX_DISABLE _MMIO(0x9140) > +#define GEN11_GT_VDBOX_DISABLE_MASK 0xff > +#define GEN11_GT_VEBOX_DISABLE_SHIFT 16 > +#define GEN11_GT_VEBOX_DISABLE_MASK (0xff << GEN11_GT_VEBOX_DISABLE_SHIFT) > + > #define GEN6_BSD_SLEEP_PSMI_CONTROL _MMIO(0x12050) > #define GEN6_BSD_SLEEP_MSG_DISABLE (1 << 0) > #define GEN6_BSD_SLEEP_FLUSH_DISABLE (1 << 2) > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index 9352f34e75c4..7c8779faf162 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -595,3 +595,57 @@ void intel_driver_caps_print(const struct intel_driver_caps *caps, > { > drm_printf(p, "scheduler: %x\n", caps->scheduler); > } > + > +/* > + * Determine which engines are fused off in our particular hardware. > + * > + * This function needs to be called after the MMIO has been setup (as we need > + * to read registers) but before uncore init (because the powerwell for the > + * fused off engines doesn't exist, so we cannot initialize forcewake for them) > + */ > +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv) How about naming this function as intel_uncore_runtime_fw_domains_init such that fw_domain_init for each of the unfused engines in next patch get called as part of this. Additional effect is update of device info. Calling intel_uncore_runtime_fw_domains_init from intel_uncore_fw_domains_init and calls to intel_device_info_init* from i915_driver_init_* paths seems to make code consistent. > +{ > + struct intel_device_info *info = mkwrite_device_info(dev_priv); > + u8 vdbox_disable, vebox_disable; > + u32 media_fuse; > + int i; > + > + if (INTEL_GEN(dev_priv) < 11) > + return; > + > + GEM_BUG_ON(!dev_priv->regs); > + > + media_fuse = I915_READ_FW(GEN11_GT_VEBOX_VDBOX_DISABLE); this should be fw_domains_get(dev_priv, FORCEWAKE_BLITTER); media_fuse = I915_READ_FW(GEN11, GT_VEBOX_VDBOX_DISABLE); fw_domains_put(dev_priv, FORCEWAKE_BLITTER); Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE, ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init and use I915_READ here for reading the fuse. But that approach seems to expose the vfuncs and forcewake table before fw domains are initialized. Although we can get to know invalid access to read/write accessors before fw_domains get initialized, current ordering of fw_domains init followed by fw domain table/read/write vfuncs init seems right. So using fw_domains_get/put as suggested above should be the way. Chris, Tvrtko, Mika, do you agree? > + > + vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK; > + vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >> > + GEN11_GT_VEBOX_DISABLE_SHIFT; > + > + DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable); > + for (i = 0; i < I915_MAX_VCS; i++) { > + if (!HAS_ENGINE(dev_priv, _VCS(i))) > + continue; > + > + if (!(BIT(i) & vdbox_disable)) > + continue; > + > + info->ring_mask &= ~ENGINE_MASK(_VCS(i)); > + WARN_ON(dev_priv->uncore.fw_domains & > + BIT(FW_DOMAIN_ID_MEDIA_VDBOX0 + i)); > + DRM_DEBUG_DRIVER("vcs%u fused off\n", i); > + } > + > + DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable); > + for (i = 0; i < I915_MAX_VECS; i++) { > + if (!HAS_ENGINE(dev_priv, _VECS(i))) > + continue; > + > + if (!(BIT(i) & vebox_disable)) > + continue; > + > + info->ring_mask &= ~ENGINE_MASK(_VECS(i)); > + WARN_ON(dev_priv->uncore.fw_domains & > + BIT(FW_DOMAIN_ID_MEDIA_VEBOX0 + i)); > + DRM_DEBUG_DRIVER("vecs%u fused off\n", i); > + } > +}
Quoting Sagar Arun Kamble (2018-02-17 08:51:44) > Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE, > ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init > and use I915_READ here for reading the fuse. But that approach seems to > expose the vfuncs and forcewake table before fw domains > are initialized. Although we can get to know invalid access to > read/write accessors before fw_domains get initialized, current ordering > of fw_domains init followed by fw domain table/read/write vfuncs init > seems right. So using fw_domains_get/put as suggested above > should be the way. > > Chris, Tvrtko, Mika, do you agree? What's the complication with the fw_domains? Do the additional powerwell depend on the fused status of the extra engines? Does it matter if the fw_domain are prepped if they are never used? Imo, I would have placed the fused discovery in intel_engines_init_mmio() (where we do the setup and can take forcewake). Then adding something like intel_uncore_reinit_mmio() (which would just prune the uncore->fw_domains) after checking fused status with commentary doesn't seem that horrible. -Chris
On 2/17/2018 2:34 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2018-02-17 08:51:44) >> Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE, >> ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init >> and use I915_READ here for reading the fuse. But that approach seems to >> expose the vfuncs and forcewake table before fw domains >> are initialized. Although we can get to know invalid access to >> read/write accessors before fw_domains get initialized, current ordering >> of fw_domains init followed by fw domain table/read/write vfuncs init >> seems right. So using fw_domains_get/put as suggested above >> should be the way. >> >> Chris, Tvrtko, Mika, do you agree? > What's the complication with the fw_domains? Do the additional powerwell > depend on the fused status of the extra engines? Does it matter if the > fw_domain are prepped if they are never used? Yes. To discover the available VD/VE engines/power domains, fuse needs to be read under blitter forcewake as RC6 will be enabled by BIOS. We do have usage of forcewake in IVB to discover FORCEWAKE_MT availability in fw_domains_init. It should not be a problem if they are prepped but never used. > Imo, I would have placed the fused discovery in > intel_engines_init_mmio() (where we do the setup and can take forcewake). > Then adding something like intel_uncore_reinit_mmio() (which would just > prune the uncore->fw_domains) after checking fused status with commentary > doesn't seem that horrible. Yes. This approach looks good too. But, we might want to optimize the driver_load to avoid this setup at first place instead of pruning later. Another related setup that can be avoided/pruned is the fw_range for the engine fused off as it can improve the fw lookup. > -Chris
Quoting Sagar Arun Kamble (2018-02-17 12:10:32) > > > On 2/17/2018 2:34 PM, Chris Wilson wrote: > > Quoting Sagar Arun Kamble (2018-02-17 08:51:44) > >> Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE, > >> ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init > >> and use I915_READ here for reading the fuse. But that approach seems to > >> expose the vfuncs and forcewake table before fw domains > >> are initialized. Although we can get to know invalid access to > >> read/write accessors before fw_domains get initialized, current ordering > >> of fw_domains init followed by fw domain table/read/write vfuncs init > >> seems right. So using fw_domains_get/put as suggested above > >> should be the way. > >> > >> Chris, Tvrtko, Mika, do you agree? > > What's the complication with the fw_domains? Do the additional powerwell > > depend on the fused status of the extra engines? Does it matter if the > > fw_domain are prepped if they are never used? > Yes. To discover the available VD/VE engines/power domains, fuse needs > to be read under blitter forcewake as > RC6 will be enabled by BIOS. We do have usage of forcewake in IVB to > discover FORCEWAKE_MT availability in fw_domains_init. > It should not be a problem if they are prepped but never used. > > Imo, I would have placed the fused discovery in > > intel_engines_init_mmio() (where we do the setup and can take forcewake). > > Then adding something like intel_uncore_reinit_mmio() (which would just > > prune the uncore->fw_domains) after checking fused status with commentary > > doesn't seem that horrible. > Yes. This approach looks good too. But, we might want to optimize the > driver_load to avoid this setup at first place > instead of pruning later. How many cycles does it take to run through all domains and set up the register offsets? No mmio access required right, we are just moving memory around without even hitting locked instructions? > Another related setup that can be avoided/pruned is the fw_range for the > engine fused off as it can improve the fw lookup. Which is a bsearch on register range, I doubt that's going to be substantially impacted by removing a few ranges. Where it matters, we should be looking to precalculate the result anyway. -Chris
On 2/17/2018 5:48 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2018-02-17 12:10:32) >> >> On 2/17/2018 2:34 PM, Chris Wilson wrote: >>> Quoting Sagar Arun Kamble (2018-02-17 08:51:44) >>>> Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE, >>>> ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init >>>> and use I915_READ here for reading the fuse. But that approach seems to >>>> expose the vfuncs and forcewake table before fw domains >>>> are initialized. Although we can get to know invalid access to >>>> read/write accessors before fw_domains get initialized, current ordering >>>> of fw_domains init followed by fw domain table/read/write vfuncs init >>>> seems right. So using fw_domains_get/put as suggested above >>>> should be the way. >>>> >>>> Chris, Tvrtko, Mika, do you agree? >>> What's the complication with the fw_domains? Do the additional powerwell >>> depend on the fused status of the extra engines? Does it matter if the >>> fw_domain are prepped if they are never used? >> Yes. To discover the available VD/VE engines/power domains, fuse needs >> to be read under blitter forcewake as >> RC6 will be enabled by BIOS. We do have usage of forcewake in IVB to >> discover FORCEWAKE_MT availability in fw_domains_init. >> It should not be a problem if they are prepped but never used. >>> Imo, I would have placed the fused discovery in >>> intel_engines_init_mmio() (where we do the setup and can take forcewake). >>> Then adding something like intel_uncore_reinit_mmio() (which would just >>> prune the uncore->fw_domains) after checking fused status with commentary >>> doesn't seem that horrible. >> Yes. This approach looks good too. But, we might want to optimize the >> driver_load to avoid this setup at first place >> instead of pruning later. > How many cycles does it take to run through all domains and set up the > register offsets? No mmio access required right, we are just moving > memory around without even hitting locked instructions? Yes. Latency is minimal. We can go with your suggestion. Will need to maintain separation of engine_cs/device_info/uncore update through separate functions. >> Another related setup that can be avoided/pruned is the fw_range for the >> engine fused off as it can improve the fw lookup. > Which is a bsearch on register range, I doubt that's going to be > substantially impacted by removing a few ranges. Where it matters, we > should be looking to precalculate the result anyway. Yes. seems not so worth. > -Chris
On 17/02/18 06:17, Sagar Arun Kamble wrote: > > > On 2/17/2018 5:48 PM, Chris Wilson wrote: >> Quoting Sagar Arun Kamble (2018-02-17 12:10:32) >>> >>> On 2/17/2018 2:34 PM, Chris Wilson wrote: >>>> Quoting Sagar Arun Kamble (2018-02-17 08:51:44) >>>>> Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE, >>>>> ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init >>>>> and use I915_READ here for reading the fuse. But that approach >>>>> seems to >>>>> expose the vfuncs and forcewake table before fw domains >>>>> are initialized. Although we can get to know invalid access to >>>>> read/write accessors before fw_domains get initialized, current >>>>> ordering >>>>> of fw_domains init followed by fw domain table/read/write vfuncs init >>>>> seems right. So using fw_domains_get/put as suggested above >>>>> should be the way. >>>>> >>>>> Chris, Tvrtko, Mika, do you agree? >>>> What's the complication with the fw_domains? Do the additional >>>> powerwell >>>> depend on the fused status of the extra engines? Does it matter if the >>>> fw_domain are prepped if they are never used? >>> Yes. To discover the available VD/VE engines/power domains, fuse needs >>> to be read under blitter forcewake as >>> RC6 will be enabled by BIOS. We do have usage of forcewake in IVB to >>> discover FORCEWAKE_MT availability in fw_domains_init. >>> It should not be a problem if they are prepped but never used. >>>> Imo, I would have placed the fused discovery in >>>> intel_engines_init_mmio() (where we do the setup and can take >>>> forcewake). >>>> Then adding something like intel_uncore_reinit_mmio() (which would just >>>> prune the uncore->fw_domains) after checking fused status with >>>> commentary >>>> doesn't seem that horrible. >>> Yes. This approach looks good too. But, we might want to optimize the >>> driver_load to avoid this setup at first place >>> instead of pruning later. >> How many cycles does it take to run through all domains and set up the >> register offsets? No mmio access required right, we are just moving >> memory around without even hitting locked instructions? > Yes. Latency is minimal. We can go with your suggestion. Will need to > maintain separation of engine_cs/device_info/uncore update > through separate functions. We do currently call fw_domain_reset on all domains at initialization time, which could write to a register that doesn't exist. We don't wait for the ack and I assume the write would just be dropped if the power well is not there so it shouldn't be an issue, but it might be worth adding a comment in fw_domain_reset to remind us that we can't start waiting for the ack in there unless we move the fuse read to an earlier point. Daniele >>> Another related setup that can be avoided/pruned is the fw_range for the >>> engine fused off as it can improve the fw lookup. >> Which is a bsearch on register range, I doubt that's going to be >> substantially impacted by removing a few ranges. Where it matters, we >> should be looking to precalculate the result anyway. > Yes. seems not so worth. >> -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9380c9f69b0f..43b2f620bca7 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1033,6 +1033,8 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) if (ret < 0) goto err_bridge; + intel_device_info_init_mmio(dev_priv); + intel_uncore_init(dev_priv); intel_uc_init_mmio(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 65e674668b2e..ba16c2025364 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3438,6 +3438,7 @@ void i915_unreserve_fence(struct drm_i915_fence_reg *fence); void i915_gem_revoke_fences(struct drm_i915_private *dev_priv); void i915_gem_restore_fences(struct drm_i915_private *dev_priv); +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv); void i915_gem_detect_bit_6_swizzle(struct drm_i915_private *dev_priv); void i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj, struct sg_table *pages); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b6cd725ff0b7..2b8d3a13dd27 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2860,6 +2860,11 @@ enum i915_power_well_id { #define GEN10_EU_DISABLE3 _MMIO(0x9140) #define GEN10_EU_DIS_SS_MASK 0xff +#define GEN11_GT_VEBOX_VDBOX_DISABLE _MMIO(0x9140) +#define GEN11_GT_VDBOX_DISABLE_MASK 0xff +#define GEN11_GT_VEBOX_DISABLE_SHIFT 16 +#define GEN11_GT_VEBOX_DISABLE_MASK (0xff << GEN11_GT_VEBOX_DISABLE_SHIFT) + #define GEN6_BSD_SLEEP_PSMI_CONTROL _MMIO(0x12050) #define GEN6_BSD_SLEEP_MSG_DISABLE (1 << 0) #define GEN6_BSD_SLEEP_FLUSH_DISABLE (1 << 2) diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 9352f34e75c4..7c8779faf162 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -595,3 +595,57 @@ void intel_driver_caps_print(const struct intel_driver_caps *caps, { drm_printf(p, "scheduler: %x\n", caps->scheduler); } + +/* + * Determine which engines are fused off in our particular hardware. + * + * This function needs to be called after the MMIO has been setup (as we need + * to read registers) but before uncore init (because the powerwell for the + * fused off engines doesn't exist, so we cannot initialize forcewake for them) + */ +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv) +{ + struct intel_device_info *info = mkwrite_device_info(dev_priv); + u8 vdbox_disable, vebox_disable; + u32 media_fuse; + int i; + + if (INTEL_GEN(dev_priv) < 11) + return; + + GEM_BUG_ON(!dev_priv->regs); + + media_fuse = I915_READ_FW(GEN11_GT_VEBOX_VDBOX_DISABLE); + + vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK; + vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >> + GEN11_GT_VEBOX_DISABLE_SHIFT; + + DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable); + for (i = 0; i < I915_MAX_VCS; i++) { + if (!HAS_ENGINE(dev_priv, _VCS(i))) + continue; + + if (!(BIT(i) & vdbox_disable)) + continue; + + info->ring_mask &= ~ENGINE_MASK(_VCS(i)); + WARN_ON(dev_priv->uncore.fw_domains & + BIT(FW_DOMAIN_ID_MEDIA_VDBOX0 + i)); + DRM_DEBUG_DRIVER("vcs%u fused off\n", i); + } + + DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable); + for (i = 0; i < I915_MAX_VECS; i++) { + if (!HAS_ENGINE(dev_priv, _VECS(i))) + continue; + + if (!(BIT(i) & vebox_disable)) + continue; + + info->ring_mask &= ~ENGINE_MASK(_VECS(i)); + WARN_ON(dev_priv->uncore.fw_domains & + BIT(FW_DOMAIN_ID_MEDIA_VEBOX0 + i)); + DRM_DEBUG_DRIVER("vecs%u fused off\n", i); + } +}