diff mbox

[4/8] drm/i915/huc: Add BXT HuC Loading Support

Message ID 1480548694-23000-5-git-send-email-anusha.srivatsa@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Srivatsa, Anusha Nov. 30, 2016, 11:31 p.m. UTC 
This patch adds the HuC Loading for the BXT by using
the updated file construction.

Version 1.7 of the HuC firmware.

v2: rebased.
v3: rebased on top of drm-tip

Cc: Jeff Mcgee <jeff.mcgee@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/intel_huc_loader.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Tvrtko Ursulin Dec. 1, 2016, 1:10 p.m. UTC | #1 
On 30/11/2016 23:31, Anusha Srivatsa wrote:
> This patch adds the HuC Loading for the BXT by using
> the updated file construction.
>
> Version 1.7 of the HuC firmware.
>
> v2: rebased.
> v3: rebased on top of drm-tip
>
> Cc: Jeff Mcgee <jeff.mcgee@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_huc_loader.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
> index 663fcc4..6357c19 100644
> --- a/drivers/gpu/drm/i915/intel_huc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
> @@ -40,6 +40,10 @@
>   * Note that HuC firmware loading must be done before GuC loading.
>   */
>
> +#define BXT_FW_MAJOR 01
> +#define BXT_FW_MINOR 07
> +#define BXT_BLD_NUM 1398
> +
>  #define SKL_FW_MAJOR 01
>  #define SKL_FW_MINOR 07
>  #define SKL_BLD_NUM 1398
> @@ -52,6 +56,9 @@
>  	SKL_FW_MINOR, SKL_BLD_NUM)
>  MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
>
> +#define I915_BXT_HUC_UCODE HUC_FW_PATH(bxt, BXT_FW_MAJOR, \
> +	BXT_FW_MINOR, BXT_BLD_NUM)
> +MODULE_FIRMWARE(I915_BXT_HUC_UCODE);
>  /**
>   * huc_ucode_xfer() - DMA's the firmware
>   * @dev_priv: the drm device
> @@ -159,6 +166,10 @@ void intel_huc_init(struct drm_device *dev)
>  		fw_path = I915_SKL_HUC_UCODE;
>  		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
>  		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
> +	} else if (IS_BROXTON(dev_priv)) {
> +		fw_path = I915_BXT_HUC_UCODE;
> +		huc_fw->major_ver_wanted = BXT_FW_MAJOR;
> +		huc_fw->minor_ver_wanted = BXT_FW_MINOR;
>  	}
>
>  	huc_fw->uc_fw_path = fw_path;
>

Build number in the file name still worries me. Last time I've asked 
about it the thread kind of died off so I will re-state it.

My concern is that if we will be getting firmware releases with the same 
major-minor but different build numbers, then embedding the build number 
into the driver prevents loading of a newer firmware unless the kernel 
is also updated.

I am not sure if that is what we want. Perhaps it is not expected at all 
that will happen in production so it is not a concern?

Or if it could happen, perhaps we should either push back on the scheme 
- drop the build number and bump the minor in all cases, or 
alternatively for our purposes drop the build number from the driver and 
have a symlinked scheme on disk?

Regards,

Tvrtko
Srivatsa, Anusha Dec. 5, 2016, 8:42 p.m. UTC | #2 
>-----Original Message-----

>From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]

>Sent: Thursday, December 1, 2016 5:11 AM

>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-

>gfx@lists.freedesktop.org

>Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915/huc: Add BXT HuC Loading Support

>

>

>On 30/11/2016 23:31, Anusha Srivatsa wrote:

>> This patch adds the HuC Loading for the BXT by using the updated file

>> construction.

>>

>> Version 1.7 of the HuC firmware.

>>

>> v2: rebased.

>> v3: rebased on top of drm-tip

>>

>> Cc: Jeff Mcgee <jeff.mcgee@intel.com>

>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

>> Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>

>> ---

>>  drivers/gpu/drm/i915/intel_huc_loader.c | 11 +++++++++++

>>  1 file changed, 11 insertions(+)

>>

>> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c

>> b/drivers/gpu/drm/i915/intel_huc_loader.c

>> index 663fcc4..6357c19 100644

>> --- a/drivers/gpu/drm/i915/intel_huc_loader.c

>> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c

>> @@ -40,6 +40,10 @@

>>   * Note that HuC firmware loading must be done before GuC loading.

>>   */

>>

>> +#define BXT_FW_MAJOR 01

>> +#define BXT_FW_MINOR 07

>> +#define BXT_BLD_NUM 1398

>> +

>>  #define SKL_FW_MAJOR 01

>>  #define SKL_FW_MINOR 07

>>  #define SKL_BLD_NUM 1398

>> @@ -52,6 +56,9 @@

>>  	SKL_FW_MINOR, SKL_BLD_NUM)

>>  MODULE_FIRMWARE(I915_SKL_HUC_UCODE);

>>

>> +#define I915_BXT_HUC_UCODE HUC_FW_PATH(bxt, BXT_FW_MAJOR, \

>> +	BXT_FW_MINOR, BXT_BLD_NUM)

>> +MODULE_FIRMWARE(I915_BXT_HUC_UCODE);

>>  /**

>>   * huc_ucode_xfer() - DMA's the firmware

>>   * @dev_priv: the drm device

>> @@ -159,6 +166,10 @@ void intel_huc_init(struct drm_device *dev)

>>  		fw_path = I915_SKL_HUC_UCODE;

>>  		huc_fw->major_ver_wanted = SKL_FW_MAJOR;

>>  		huc_fw->minor_ver_wanted = SKL_FW_MINOR;

>> +	} else if (IS_BROXTON(dev_priv)) {

>> +		fw_path = I915_BXT_HUC_UCODE;

>> +		huc_fw->major_ver_wanted = BXT_FW_MAJOR;

>> +		huc_fw->minor_ver_wanted = BXT_FW_MINOR;

>>  	}

>>

>>  	huc_fw->uc_fw_path = fw_path;

>>

>

>Build number in the file name still worries me. Last time I've asked about it the

>thread kind of died off so I will re-state it.

>

>My concern is that if we will be getting firmware releases with the same major-

>minor but different build numbers, then embedding the build number into the

>driver prevents loading of a newer firmware unless the kernel is also updated.

>

>I am not sure if that is what we want. Perhaps it is not expected at all that will

>happen in production so it is not a concern?

>

>Or if it could happen, perhaps we should either push back on the scheme

>- drop the build number and bump the minor in all cases, or alternatively for our

>purposes drop the build number from the driver and have a symlinked scheme on

>disk?

>

>Regards,

>

>Tvrtko


Hi Tvrtko,
Sincere apologies for responding so late. According to my understanding, Jeff correct me if I am wrong, we are finalizing the firmware version number for every kernel version. So a certain kernel will have only one possible firware major-minor and build number for a certain platform.

 I have cc-ed Jeff in this thread so he can add his comment on build number. Jeff, any comments?

Regards,
Anusha
jeff.mcgee@intel.com Dec. 8, 2016, 3:43 p.m. UTC | #3 
On Mon, Dec 05, 2016 at 12:42:11PM -0800, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
> >Sent: Thursday, December 1, 2016 5:11 AM
> >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> >gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915/huc: Add BXT HuC Loading Support
> >
> >
> >On 30/11/2016 23:31, Anusha Srivatsa wrote:
> >> This patch adds the HuC Loading for the BXT by using the updated file
> >> construction.
> >>
> >> Version 1.7 of the HuC firmware.
> >>
> >> v2: rebased.
> >> v3: rebased on top of drm-tip
> >>
> >> Cc: Jeff Mcgee <jeff.mcgee@intel.com>
> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_huc_loader.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c
> >> b/drivers/gpu/drm/i915/intel_huc_loader.c
> >> index 663fcc4..6357c19 100644
> >> --- a/drivers/gpu/drm/i915/intel_huc_loader.c
> >> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
> >> @@ -40,6 +40,10 @@
> >>   * Note that HuC firmware loading must be done before GuC loading.
> >>   */
> >>
> >> +#define BXT_FW_MAJOR 01
> >> +#define BXT_FW_MINOR 07
> >> +#define BXT_BLD_NUM 1398
> >> +
> >>  #define SKL_FW_MAJOR 01
> >>  #define SKL_FW_MINOR 07
> >>  #define SKL_BLD_NUM 1398
> >> @@ -52,6 +56,9 @@
> >>  	SKL_FW_MINOR, SKL_BLD_NUM)
> >>  MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
> >>
> >> +#define I915_BXT_HUC_UCODE HUC_FW_PATH(bxt, BXT_FW_MAJOR, \
> >> +	BXT_FW_MINOR, BXT_BLD_NUM)
> >> +MODULE_FIRMWARE(I915_BXT_HUC_UCODE);
> >>  /**
> >>   * huc_ucode_xfer() - DMA's the firmware
> >>   * @dev_priv: the drm device
> >> @@ -159,6 +166,10 @@ void intel_huc_init(struct drm_device *dev)
> >>  		fw_path = I915_SKL_HUC_UCODE;
> >>  		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
> >>  		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
> >> +	} else if (IS_BROXTON(dev_priv)) {
> >> +		fw_path = I915_BXT_HUC_UCODE;
> >> +		huc_fw->major_ver_wanted = BXT_FW_MAJOR;
> >> +		huc_fw->minor_ver_wanted = BXT_FW_MINOR;
> >>  	}
> >>
> >>  	huc_fw->uc_fw_path = fw_path;
> >>
> >
> >Build number in the file name still worries me. Last time I've asked about it the
> >thread kind of died off so I will re-state it.
> >
> >My concern is that if we will be getting firmware releases with the same major-
> >minor but different build numbers, then embedding the build number into the
> >driver prevents loading of a newer firmware unless the kernel is also updated.
> >
> >I am not sure if that is what we want. Perhaps it is not expected at all that will
> >happen in production so it is not a concern?
> >
> >Or if it could happen, perhaps we should either push back on the scheme
> >- drop the build number and bump the minor in all cases, or alternatively for our
> >purposes drop the build number from the driver and have a symlinked scheme on
> >disk?
> >
> >Regards,
> >
> >Tvrtko
> 
> Hi Tvrtko,
> Sincere apologies for responding so late. According to my understanding, Jeff correct me if I am wrong, we are finalizing the firmware version number for every kernel version. So a certain kernel will have only one possible firware major-minor and build number for a certain platform.
> 
>  I have cc-ed Jeff in this thread so he can add his comment on build number. Jeff, any comments?
> 
> Regards,
> Anusha

Sorry for delayed response. I'm checking with HuC firmware team on their
intended release model.
-Jeff
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
index 663fcc4..6357c19 100644
--- a/drivers/gpu/drm/i915/intel_huc_loader.c
+++ b/drivers/gpu/drm/i915/intel_huc_loader.c
@@ -40,6 +40,10 @@ 
  * Note that HuC firmware loading must be done before GuC loading.
  */
 
+#define BXT_FW_MAJOR 01
+#define BXT_FW_MINOR 07
+#define BXT_BLD_NUM 1398
+
 #define SKL_FW_MAJOR 01
 #define SKL_FW_MINOR 07
 #define SKL_BLD_NUM 1398
@@ -52,6 +56,9 @@ 
 	SKL_FW_MINOR, SKL_BLD_NUM)
 MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
 
+#define I915_BXT_HUC_UCODE HUC_FW_PATH(bxt, BXT_FW_MAJOR, \
+	BXT_FW_MINOR, BXT_BLD_NUM)
+MODULE_FIRMWARE(I915_BXT_HUC_UCODE);
 /**
  * huc_ucode_xfer() - DMA's the firmware
  * @dev_priv: the drm device
@@ -159,6 +166,10 @@  void intel_huc_init(struct drm_device *dev)
 		fw_path = I915_SKL_HUC_UCODE;
 		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
 		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
+	} else if (IS_BROXTON(dev_priv)) {
+		fw_path = I915_BXT_HUC_UCODE;
+		huc_fw->major_ver_wanted = BXT_FW_MAJOR;
+		huc_fw->minor_ver_wanted = BXT_FW_MINOR;
 	}
 
 	huc_fw->uc_fw_path = fw_path;