[Intel-gfx] [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values

[Michał Winiarski]

On Fri, Mar 23, 2018 at 04:02:47PM +0200, Joonas Lahtinen wrote:
> Quoting Yaodong Li (2018-03-22 23:27:59)
> > On 03/22/2018 01:38 PM, Michał Winiarski wrote:
> > > On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
> > >> @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> > >>              return -E2BIG;
> > >>      }
> > >>   
> > >> -    guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
> > >> -                           GUC_WOPCM_OFFSET_ALIGNMENT);
> > >> -    if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> > >> -            DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> > >> -                      guc_wopcm_base / 1024);
> > >> -            return -E2BIG;
> > >> -    }
> > >> -
> > >> -    guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
> > >> -    guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
> > >> +    guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> > > We already discussed this elsewhere, but just to have this part available
> > > for wider audience:
> > >
> > > huc_fw_size is unknown (there may be no huc firmware present) - which means
> > > we're still not able to handle module reload from guc-without-huc into
> > > guc-with-huc
> > >
> > > Question remains whether we want to support this scenario, or whether we should
> > > tie GuC and HuC together and refuse to operate early on if either one is not
> > > present. We could remove a lot of code this way...
> > Thanks. As we discussed I think we should describe this problem in this way:
> > we shall not make any assumption on either guc_fw_size and huc_fw_size.
> > On the other handle, we do need calculate the WOPCM layout based on
> > both GuC and HuC FW sizes, especially when we want to support modparam
> > enable_guc to enable/disable HuC FW loading dynamically. That's why I 
> > suggest
> > to update the FW fetch code to report the FW size no matter we turn the HuC
> > FW loading on or not.
> > Other aspect of the discussion is whether we should support enable_guc 
> > switching
> > from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
> > In another word whether we should support FW image updates
> > (add/delete/update to a new version) in the filesystem without expecting 
> > a system
> > reboot. My point is it's unlikely we can support this since the FW sizes 
> > would likely
> > change and we need reconfigure the WO register with the latest FW 
> > available in
> > the FS, and I think it worth to do it to 100% make sure we won't run 
> > into any
> > module loading/init errors.
> 
> Did we ask the HW team about this? Surely the intention can't be to both
> support dynamic enable/disable but yet having a write-once variable holding
> the configuration?
> 
> Also, somebody mentioned that these were locked already before loading
> i915, so if the firmware sizes have to do with the configuration, how
> could that be? We'd have to match BIOS/whatever firmware, and that's not
> likely to happen, is it.

That was me, and that was an error in my environment. We expect to see unlocked
regs on reboot. And even if we don't (e.g. because it's a module reload), trying
to fit isn't really that hard to do (we're reusing most of the code), so I
believe this is the right approach.

-Michał
 
> Regards, Joonas