[Intel-gfx] [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Apr 19 15:31:11 UTC 2018
On Mon, 16 Apr 2018 19:28:04 +0200, Yaodong Li <yaodong.li at intel.com>
wrote:
> On 04/13/2018 07:15 PM, Michal Wajdeczko wrote:
>> On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li <yaodong.li at intel.com>
>> wrote:
>>
>>> After enabled the WOPCM write-once registers locking status checking,
>>> reloading of the i915 module will fail with modparam enable_guc set to
>>> 3
>>> (enable GuC and HuC firmware loading) if the module was originally
>>> loaded
>>> with enable_guc set to 1 (only enable GuC firmware loading).
>>
>> Is this frequent and required scenario ? or just for debug/development ?
>>
> My understanding is this should be a nice to have feature and mainly for
> debugging.
>>> This is
>>> because WOPCM registers were updated and locked without considering
>>> the HuC
>>> FW size. Since we need both GuC and HuC FW sizes to determine the final
>>> layout of WOPCM, we should always calculate the WOPCM layout based on
>>> the
>>> actual sizes of the GuC and HuC firmware available for a specific
>>> platform
>>> if we need continue to support enable/disable HuC FW loading
>>> dynamically
>>> with enable_guc modparam.
>>>
>>> This patch splits uC firmware fetching into two stages. First stage is
>>> to
>>> fetch the firmware image and verify the firmware header. uC firmware
>>> will
>>> be marked as verified and this will make FW info available for
>>> following
>>> WOPCM layout calculation. The second stage is to create a GEM object
>>> and
>>> copy the FW data into the created GEM object which will only be
>>> available
>>> when GuC/HuC loading is enabled by enable_guc modparam. This will
>>> guarantee
>>> that the WOPCM layout will be always be calculated correctly without
>>> making
>>> any assumptions to the GuC and HuC firmware sizes.
>>
>> You are also assuming that on reload exactly the same GuC/HuC firmwares
>> will bee used as in initial run. This will make this useless for debug/
>> development scenarios, where custom fw are likely to be specified.
>>
> This patch is mainly for providing a real fix to support
> enable_guc=1->3->1 use case.
> It based on the fact that it is inevitable that sometimes we need to
> reboot the system
> if the status of the fw was changed on the file system.
What do you mean by "status of the fw was changed on the file system" ?
* change of the fw binary/version/size, or
* change from not-present to present ?
> I am not sure how often we switch between different HuC FW with
> different sizes?
Just above you said that you need this "mainly for debugging" so
I would expect that then different fw sizes are expected.
>> If we want to support enable_guc=1->3->1 scenarios for debug/dev then
>> maybe more flexible will be other approach that makes allocations from
>> the other end as proposed in [1]
>>
>> [1] https://patchwork.freedesktop.org/patch/212471/
> Actually, I do think this might be one of the options, and I've also put
> some comments on this
> series. The main concern I have is it still make assumption on the GuC
> FW size and may
But in enable_guc=1-->3 scenario, I would assume that the only difference
will be HuC fw (as with enable=1 we already loaded GuC)
If you want just to test different GuC fws, then it is different scenario
as then enable_guc will always be = 1.
> run into the same issue if the GuC FW failed to meet the requirement.
> and for debugging purpose it would have the same possible for different
> GuC FW debugging.
>>
>>>
>>> v3:
>>> - Rebase
>>>
>>> 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: Michal Winiarski <michal.winiarski at intel.com>
>>> Cc: John Spotswood <john.a.spotswood at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_uc.c | 14 ++++----------
>>> drivers/gpu/drm/i915/intel_uc_fw.c | 31
>>> ++++++++++++++++++++-----------
>>> drivers/gpu/drm/i915/intel_uc_fw.h | 7 +++++--
>>> 3 files changed, 29 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>>> b/drivers/gpu/drm/i915/intel_uc.c
>>> index 1cffaf7..73b8f6c 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private
>>> *i915)
>>> sanitize_options_early(i915);
>>> - if (USES_GUC(i915))
>>> - intel_uc_fw_fetch(i915, &guc->fw);
>>> -
>>> - if (USES_HUC(i915))
>>> - intel_uc_fw_fetch(i915, &huc->fw);
>>> + intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
>>> + intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>>
>> Hmm, side effect of those unconditional fetches might be unwanted
>> warnings
>> about missing firmwares (on configs with disabled guc) as well as
>> extended
>> driver load time.
> Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will
> return directly.
I was referring to scenario when on platform with HAS_HUC and with
enable_guc=1 (just submission, no HuC) we will try to fetch HuC fw
(that may not be present at all) and then drop it as don't need it.
>>
>> Do we really need to support this corner case enable_guc=1->3 at all
>> costs?
> I think this is the real solution for this issue (with no assumption).
> However, we do
> need to decide whether we should support such a corner case which is
> mainly for
> debugging.
I'm repeating here Joonas' earlier statement:
"Then just require a reboot if after that partitioning,
changing the parameter causes the FW not to fit"
>>
>> /michal
>>
>>> }
>>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>>> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct
>>> drm_i915_private *i915)
>>> struct intel_guc *guc = &i915->guc;
>>> struct intel_huc *huc = &i915->huc;
>>> - if (USES_HUC(i915))
>>> - intel_uc_fw_fini(&huc->fw);
>>> -
>>> - if (USES_GUC(i915))
>>> - intel_uc_fw_fini(&guc->fw);
>>> + intel_uc_fw_fini(&huc->fw);
>>> + intel_uc_fw_fini(&guc->fw);
>>> guc_free_load_err_log(guc);
>>> }
>>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
>>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>>> index 6e8e0b5..a9cb900 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>>> @@ -33,11 +33,13 @@
>>> *
>>> * @dev_priv: device private
>>> * @uc_fw: uC firmware
>>> + * @copy_to_obj: whether fetch uC firmware into GEM object or not
>>
>> s/copy_to_obj/fetch
> sure.
>>
>>> *
>>> - * Fetch uC firmware into GEM obj.
>>> + * Fetch and verify uC firmware and copy firmware data into GEM
>>> object if
>>> + * @copy_to_obj is true.
>>> */
>>> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>>> - struct intel_uc_fw *uc_fw)
>>> + struct intel_uc_fw *uc_fw, bool copy_to_obj)
>>> {
>>> struct pci_dev *pdev = dev_priv->drm.pdev;
>>> struct drm_i915_gem_object *obj;
>>> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private
>>> *dev_priv,
>>> goto fail;
>>> }
>>> - obj = i915_gem_object_create_from_data(dev_priv, fw->data,
>>> fw->size);
>>> - if (IS_ERR(obj)) {
>>> - err = PTR_ERR(obj);
>>> - DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>>> - intel_uc_fw_type_repr(uc_fw->type), err);
>>> - goto fail;
>>> + uc_fw->size = fw->size;
>>> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
>>> +
>>> + if (copy_to_obj) {
>>> + obj = i915_gem_object_create_from_data(dev_priv, fw->data,
>>> + fw->size);
>>> + if (IS_ERR(obj)) {
>>> + err = PTR_ERR(obj);
>>> + DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>>> + intel_uc_fw_type_repr(uc_fw->type),
>>> + err);
>>> + goto fail;
>>> + }
>>> +
>>> + uc_fw->obj = obj;
>>> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>>> }
>>> - uc_fw->obj = obj;
>>> - uc_fw->size = fw->size;
>>> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>>> DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>>> intel_uc_fw_type_repr(uc_fw->type),
>>> intel_uc_fw_status_repr(uc_fw->fetch_status));
>>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h
>>> b/drivers/gpu/drm/i915/intel_uc_fw.h
>>> index dc33b12..4e7ecc8 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
>>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
>>> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
>>> INTEL_UC_FIRMWARE_FAIL = -1,
>>> INTEL_UC_FIRMWARE_NONE = 0,
>>> INTEL_UC_FIRMWARE_PENDING,
>>> + INTEL_UC_FIRMWARE_VERIFIED,
>>> INTEL_UC_FIRMWARE_SUCCESS
>>> };
>>> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum
>>> intel_uc_fw_status status)
>>> return "NONE";
>>> case INTEL_UC_FIRMWARE_PENDING:
>>> return "PENDING";
>>> + case INTEL_UC_FIRMWARE_VERIFIED:
>>> + return "VERIFIED";
>>> case INTEL_UC_FIRMWARE_SUCCESS:
>>> return "SUCCESS";
>>> }
>>> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct
>>> intel_uc_fw *uc_fw)
>>> */
>>> static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw
>>> *uc_fw)
>>> {
>>> - if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
>>> + if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
>>> return 0;
>>> return uc_fw->header_size + uc_fw->ucode_size;
>>> }
>>> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>>> - struct intel_uc_fw *uc_fw);
>>> + struct intel_uc_fw *uc_fw, bool copy_to_obj);
>>> int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>>> int (*xfer)(struct intel_uc_fw *uc_fw,
>>> struct i915_vma *vma));
More information about the Intel-gfx
mailing list