Re: [PATCH 3/8] drm/i915/huc: Add HuC fw loading support

From: Jani Nikula <jani.nikula@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	anushasr <anusha.srivatsa@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/8] drm/i915/huc: Add HuC fw loading support
Date: Wed, 14 Dec 2016 17:19:47 +0200	[thread overview]
Message-ID: <8760mmr10s.fsf@intel.com> (raw)
In-Reply-To: <b6fd72c3-963b-d42f-71d9-5b5e8e53cc31@linux.intel.com>

On Mon, 12 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@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 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx