[Intel-gfx] [PATCH 3/8] drm/i915/huc: Add HuC fw loading support

[Parenteau, Paul A]

>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
>Jani Nikula
>Sent: Wednesday, December 14, 2016 7:20 AM
>To: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>; Srivatsa, Anusha
><anusha.srivatsa at intel.com>; intel-gfx at lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 3/8] drm/i915/huc: Add HuC fw loading support
>
>On Mon, 12 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>> Hi all,
>>
>> Executive decision required below:
>>
>> On 08/12/2016 23:02, anushasr wrote:
>>
>> [snip]
>>
>>> +
>>> +/**
>>> + * DOC: HuC Firmware
>>> + *
>>> + * Motivation:
>>> + * GEN9 introduces a new dedicated firmware for usage in media HEVC
>>> +(High
>>> + * Efficiency Video Coding) operations. Userspace can use the
>>> +firmware
>>> + * capabilities by adding HuC specific commands to batch buffers.
>>> + *
>>> + * Implementation:
>>> + * The same firmware loader is used as the GuC. However, the actual
>>> + * loading to HW is deferred until GEM initialization is done.
>>> + *
>>> + * Note that HuC firmware loading must be done before GuC loading.
>>> + */
>>> +
>>> +#define SKL_FW_MAJOR 01
>>> +#define SKL_FW_MINOR 07
>>> +#define SKL_BLD_NUM 1398
>>> +
>>> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
>>> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
>>> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
>>> +
>>> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
>>> +	SKL_FW_MINOR, SKL_BLD_NUM)
>>> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
>>
>> Daniel & Jani, I understand in the GuC firmware discussions you were
>> very much in the favour of encoding the full name (major and minor
>> included) as the fw firmware?
>>
>> Argument was that we want to in effect claim support only for one
>> validated firmware binary with one version of i915.
>>
>> In the case of the HuC we have a very similar situation with two key
>> differences.
>>
>> First, there is a build number in the file name as provided by the
>> firmware team. We know that it will happen that only the build number
>> changes with some fixes and the minor stays the same. And we know that
>> the major indicates the interface compatibility.
>>
>> Secondly, from all I can see, there are no interactions between the
>> driver and the HuC firmware apart from driver loading it and thats it.
>>
>> In the light of that I was advocating only using the major in the
>> driver request fw name in order to improve usability both for
>> developers (easier to test with different firmwares), and for users
>> (be it distributions or end users - easier to upgrade the HuC firmware
>> in case of codec issues by not having to patch and recompile the kernel).
>>
>> Since I understand this topic has been beaten to death in the past and
>> there are strong opinions on it, could you just okay (or not) the
>> current proposal (as in posted patches) which encodes major, minor and
>> build number in the fw name?
>
>The question is the same as it ever was, can you absolutely guarantee a new
>firmware version (even if just a build number bump) will not cause regressions?
>Note that we'll probably only have resources to test the latest kernel against the
>latest firmware, but accepting future firmware versions means even stable
>kernels will start using the new firmware versions after linux-firmware updates.
>We also can't easily retroactively prevent this from happening even if we find
>out that there will be breakage.
>
>I still think we should only accept one firmware version. If we (or someone else)
>has the resources to test against older kernels, commits to enable newer
>firmware versions can be backported to stable.
>
I agree with Jani's position here.  We have to keep some controls to the combinations of FW and kernels so we know what works.  At least until FW matures in the future.
>
>I would love to be able to look at the firmware sources and say, oh, there are no
>interactions with the kernel whatsoever, but you know how that is. I don't know
>if there are interactions. I don't know if future blobs will have interactions, and
>break everything if the kernel doesn't do something the black box expects.
>
>If you want to help developers test various firmware versions, I suggest the same
>thing I suggested the last time: add an _unsafe module parameter to specify the
>firmware filename/version to load.
>
>BR,
>Jani.
>
>
>--
>Jani Nikula, Intel Open Source Technology Center
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx