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

[Yaodong Li]

On 03/23/2018 07:01 AM, Michał Winiarski wrote:
> On Thu, Mar 22, 2018 at 02:27:59PM -0700, Yaodong Li wrote:
>> On 03/22/2018 01:38 PM, Michał Winiarski wrote:
>>> On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
>>>> In current code, we only compare the locked WOPCM register values with the
>>>> calculated values. However, we can continue loading GuC/HuC firmware if the
>>>> locked (or partially locked) values were valid for current GuC/HuC firmware
>>>> sizes.
>>>>
>>>> This patch added a new code path to verify whether the locked register
>>>> values can be used for GuC/HuC firmware loading, it will recalculate the
>>>> verify the new values if these registers were partially locked, so that we
>>>> won't fail the GuC/HuC firmware loading even if the locked register values
>>>> are different from the calculated ones.
>>>>
>>>> Signed-off-by: Jackie Li <yaodong.li at intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>>>> Cc: John Spotswood <john.a.spotswood at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
>>>>    1 file changed, 157 insertions(+), 26 deletions(-)
>>> [snip]
>>>
>>>>    /**
>>>>     * intel_wopcm_init() - Initialize the WOPCM structure.
>>>>     * @wopcm: pointer to intel_wopcm.
>>>> @@ -155,10 +202,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>>>    	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>>>>    	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
>>>>    	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
>>>> -	u32 ctx_rsvd = context_reserved_size(i915);
>>>>    	u32 guc_wopcm_base;
>>>>    	u32 guc_wopcm_size;
>>>> -	u32 guc_wopcm_rsvd;
>>>>    	int err;
>>>>    	GEM_BUG_ON(!wopcm->size);
>>>> @@ -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.
> We can make assumptions on guc_fw_size - it's known in all the relevant cases
> (enable_guc=1,2,3).
> (unless we want to support varying guc_fw_size - more on that later).
What I meant is we cannot make any assumption one the actual size
of guc and huc fw:)
>> 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.
> We could do that, but we don't really *need* to do that.
> If user doesn't want HuC, why are we reading HuC from the filesystem? (well,
> because we decided we're going to base our paritioning around it).
Because of the enable_guc=1 and enable_guc=3 case and the fact
that we only can write to these registers for once + we need to know
the size to 100% make sure we do the partitioning correctly.
please refer to my patch set: 
https://patchwork.freedesktop.org/series/40541/
>> 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.
> It's never going to be 100%, but we can at least try to work with 99% ;)
With correct FW images, it would be 100%, isn't it?:)
> The only reason why you need HuC size is because you're building the
> partitioning from the bottom (starting from HuC).
> We could just as well start from the top (starting from GuC).
That's right. but either starting from the bottom (based on HuC FW size) or
starting from the top (based on GuC FW size), we are ignoring one fact 
is that
the final layout is depended on both GuC and HuC FW sizes.
> The only thing you gain by starting from the bottom, is that you're much more
> tolerant for variations in GuC fw size (because you're paritioning HuC region
> precisely, leaving the rest for GuC. Well, that, and the HW workaround).
We would guarantee we would have maximum guc wopcm size in this
way which will be the most likely way to meet current hw limitations.
and more important it's simper and easier:)
> The only thing that changes when you're starting from the top, is that you're
> going the other way around (more tolerant for HuC fw size variations).
As I said, I agree that we would likely solve the enable_guc=1 then
enable_guc=3 case with these changes which I think this the the only benefit
that we can get from the starting from the top way.
But my point is just like the from the bottom way, you are ignoring the
HuC FW size. e.g. you will need to grow the guc wopcm size for the hw 
restrictions.
The problem is currently we do have this restriction on huc fw size v.s. 
guc wopcm
size. In you solution, you are actually counting on the assumption that
guc fw size should be large enough so that we can ignore the huc fw size
and expect it still works when we set enable_huc=3. and my answer to
this is yes it will work for the cases that guc fw size is large enough, but
still risky to fail in case of guc fw size < huc_fw_size + 16K. and that 
comes
to my point:
why not make life easier and accurate - If we decide to support dynamically
switching on/off huc fw loading and the fact we can get actual FW sizes,
why not drop all these assumptions and fix it in a way that we won't be
bothered by any FW side changes? :)

Regards,
-Jackie