Re: [Intel-gfx] [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

2017-02-22 Thread Arkadiusz Hiler 
On Wed, Feb 22, 2017 at 04:17:20PM +0200, Joonas Lahtinen wrote:
> On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> > intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> > 
> > It's also in the wrong place - 'guc_loader.h' - it's used for both guc
> > and huc, which was reflected in name, but not it's location, so let's
> > move it to 'intel_uc.c'.
> > 
> > We can make a intel_uc_fw callback out of it, to avoid leaking it
> > outside `intel_uc.c`
> > 
> > Cc: Joonas Lahtinen 
> > Cc: Michal Winiarski 
> > Signed-off-by: Arkadiusz Hiler 
> 
> 
> 
> > @@ -32,7 +36,10 @@ void intel_uc_init_early(struct drm_i915_private 
> > *dev_priv)
> >  
> >  void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
> >  {
> > +   dev_priv->huc.fw.fetch = uc_fetch_fw;
> >     intel_huc_fetch_fw(_priv->huc);
> > +
> > +   dev_priv->guc.fw.fetch = uc_fetch_fw;
> 
> I'm bit confused, I was under impression these functions were going to
> be different for each uC? If they're not, then it most definitely
> should not be a function pointer.

The issue I presented on the IRC (maybe not clearly enough) was just
placement and naming of that function. The proposition with callback
seemd odd, but since it was backed by couple of people I commited to it.

Glad you spoted that here.

>   int ret;
> 
>   ret = intel_huc_select_fw(dev_priv->huc);
>   if (ret)
>   goto err;
>   ret = intel_uc_fw_prepare(_priv->huc->fw);
>   if (ret)
>   goto err;
> 
>   ret = intel_guc_select_fw(dev_priv->guc);
>   if (ret)
>   goto err_huc;
>   ret = intel_uc_fw_prepare(_priv->guc->fw);
>   if (ret)
>   
> goto err_huc;
> 
>   return 0;
> 
> err_huc:
>   intel_uc_fw_unprepare(_priv->huc->fw);
> err:
>   return ret;
> 
> By always involving proper onion teardown, no dangling objects are left
> around.

That's exactly what I was missing in my approach. Thanks!

> >     intel_guc_fetch_fw(_priv->guc);
> >  }
> >  
> > @@ -120,3 +127,137 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
> > >   return intel_guc_send(guc, action, ARRAY_SIZE(action));
> >  }
> >  
> > +static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> > +     struct intel_uc_fw *uc_fw)
> > +{
> > +   struct pci_dev *pdev = dev_priv->drm.pdev;
> > +   struct drm_i915_gem_object *obj;
> > +   const struct firmware *fw = NULL;
> > +   struct uc_css_header *css;
> > +   size_t size;
> > +   int err;
> 
> This function should be named differently, because it doesn't simply
> fetch it, but also validates and makes an object of it (which may be
> left dangling).

intel_uc_fw_prepare you've used in the example above seems like a good
name.

Onto fixing this mess. Thanks for the input :-)

-- 
Cheers,
Arek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

2017-02-22 Thread Joonas Lahtinen 
On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> 
> It's also in the wrong place - 'guc_loader.h' - it's used for both guc
> and huc, which was reflected in name, but not it's location, so let's
> move it to 'intel_uc.c'.
> 
> We can make a intel_uc_fw callback out of it, to avoid leaking it
> outside `intel_uc.c`
> 
> Cc: Joonas Lahtinen 
> Cc: Michal Winiarski 
> Signed-off-by: Arkadiusz Hiler 



> @@ -32,7 +36,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
>  {
> + dev_priv->huc.fw.fetch = uc_fetch_fw;
>   intel_huc_fetch_fw(_priv->huc);
> +
> + dev_priv->guc.fw.fetch = uc_fetch_fw;

I'm bit confused, I was under impression these functions were going to
be different for each uC? If they're not, then it most definitely
should not be a function pointer.

int ret;

ret = intel_huc_select_fw(dev_priv->huc);
if (ret)
goto err;
ret = intel_uc_fw_prepare(_priv->huc->fw);
if (ret)
goto err;

ret = intel_guc_select_fw(dev_priv->guc);
if (ret)
goto err_huc;
ret = intel_uc_fw_prepare(_priv->guc->fw);
if (ret)

goto err_huc;

return 0;

err_huc:
intel_uc_fw_unprepare(_priv->huc->fw);
err:
return ret;

By always involving proper onion teardown, no dangling objects are left
around.

>   intel_guc_fetch_fw(_priv->guc);
>  }
>  
> @@ -120,3 +127,137 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
> >     return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
>  
> +static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> +   struct intel_uc_fw *uc_fw)
> +{
> + struct pci_dev *pdev = dev_priv->drm.pdev;
> + struct drm_i915_gem_object *obj;
> + const struct firmware *fw = NULL;
> + struct uc_css_header *css;
> + size_t size;
> + int err;

This function should be named differently, because it doesn't simply
fetch it, but also validates and makes an object of it (which may be
left dangling).

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

2017-02-22 Thread Arkadiusz Hiler 
intel_uc_fw_fetch() is confusingly named in the light of recent changes.

It's also in the wrong place - 'guc_loader.h' - it's used for both guc
and huc, which was reflected in name, but not it's location, so let's
move it to 'intel_uc.c'.

We can make a intel_uc_fw callback out of it, to avoid leaking it
outside `intel_uc.c`

Cc: Joonas Lahtinen 
Cc: Michal Winiarski 
Signed-off-by: Arkadiusz Hiler 
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 137 +--
 drivers/gpu/drm/i915/intel_huc.c|   2 +-
 drivers/gpu/drm/i915/intel_uc.c | 141 
 drivers/gpu/drm/i915/intel_uc.h |   4 +-
 4 files changed, 145 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 753aeef..110dfd1 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -26,7 +26,6 @@
  *Dave Gordon 
  *Alex Dai 
  */
-#include 
 #include "i915_drv.h"
 #include "intel_uc.h"
 
@@ -587,140 +586,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
return ret;
 }
 
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-struct intel_uc_fw *uc_fw)
-{
-   struct pci_dev *pdev = dev_priv->drm.pdev;
-   struct drm_i915_gem_object *obj;
-   const struct firmware *fw = NULL;
-   struct uc_css_header *css;
-   size_t size;
-   int err;
-
-   DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
-   intel_uc_fw_status_repr(uc_fw->fetch_status));
-
-   err = request_firmware(, uc_fw->path, >dev);
-   if (err)
-   goto fail;
-   if (!fw)
-   goto fail;
-
-   DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
-   uc_fw->path, fw);
-
-   /* Check the size of the blob before examining buffer contents */
-   if (fw->size < sizeof(struct uc_css_header)) {
-   DRM_NOTE("Firmware header is missing\n");
-   goto fail;
-   }
-
-   css = (struct uc_css_header *)fw->data;
-
-   /* Firmware bits always start from header */
-   uc_fw->header_offset = 0;
-   uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
-   css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
-
-   if (uc_fw->header_size != sizeof(struct uc_css_header)) {
-   DRM_NOTE("CSS header definition mismatch\n");
-   goto fail;
-   }
-
-   /* then, uCode */
-   uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
-   uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
-
-   /* now RSA */
-   if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-   DRM_NOTE("RSA key size is bad\n");
-   goto fail;
-   }
-   uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
-   uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
-
-   /* At least, it should have header, uCode and RSA. Size of all three. */
-   size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
-   if (fw->size < size) {
-   DRM_NOTE("Missing firmware components\n");
-   goto fail;
-   }
-
-   /*
-* The GuC firmware image has the version number embedded at a 
well-known
-* offset within the firmware blob; note that major / minor version are
-* TWO bytes each (i.e. u16), although all pointers and offsets are 
defined
-* in terms of bytes (u8).
-*/
-   switch (uc_fw->fw) {
-   case INTEL_UC_FW_TYPE_GUC:
-   /* Header and uCode will be loaded to WOPCM. Size of the two. */
-   size = uc_fw->header_size + uc_fw->ucode_size;
-
-   /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
-   if (size > intel_guc_wopcm_size(dev_priv)) {
-   DRM_ERROR("Firmware is too large to fit in WOPCM\n");
-   goto fail;
-   }
-   uc_fw->major_ver_found = css->guc.sw_version >> 16;
-   uc_fw->minor_ver_found = css->guc.sw_version & 0x;
-   break;
-
-   case INTEL_UC_FW_TYPE_HUC:
-   uc_fw->major_ver_found = css->huc.sw_version >> 16;
-   uc_fw->minor_ver_found = css->huc.sw_version & 0x;
-   break;
-
-   default:
-   DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw);
-   err = -ENOEXEC;
-   goto fail;
-   }
-
-   if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
-   uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
-   DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
-

Re: [Intel-gfx] [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

2017-02-21 Thread Arkadiusz Hiler 
On Fri, Feb 17, 2017 at 02:39:35PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:54PM +0100, Arkadiusz Hiler wrote:
> > intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> > 
> > It's also in the worng place - 'guc_loader.h' - it's used for both guc
> 
> Typo s/worng/wrong

Done.

> > and huc, which was reflected in name, but not it's location, so let's
> > move it to 'intel_uc.c'.
> > 
> > We can make a intel_uc_fw callback out of it, to avoid leaking it
> > outside `intel_uc.c`
> 
> Hmm, why do you think it is a problem to expose this function outside of 
> intel_uc.c?
> I can't see any real gain, rather unnecessary code complexity

I was trying to figure out a good name for it (to not confuse it with
intel_uc_fw_fetch) and maybe prefix it with __ to denote that nobody
should really bother with it, but that's reserved for statics.

I brought the topic on #intel-gfx and Joonas suggested this approach.

That seems to be general trend with i915.

-- 
Cheers,
Arek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

2017-02-17 Thread Michal Wajdeczko 
On Fri, Feb 17, 2017 at 02:05:54PM +0100, Arkadiusz Hiler wrote:
> intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> 
> It's also in the worng place - 'guc_loader.h' - it's used for both guc

Typo s/worng/wrong

> and huc, which was reflected in name, but not it's location, so let's
> move it to 'intel_uc.c'.
> 
> We can make a intel_uc_fw callback out of it, to avoid leaking it
> outside `intel_uc.c`

Hmm, why do you think it is a problem to expose this function outside of 
intel_uc.c?
I can't see any real gain, rather unnecessary code complexity

-Michal

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

2017-02-17 Thread Arkadiusz Hiler 
intel_uc_fw_fetch() is confusingly named in the light of recent changes.

It's also in the worng place - 'guc_loader.h' - it's used for both guc
and huc, which was reflected in name, but not it's location, so let's
move it to 'intel_uc.c'.

We can make a intel_uc_fw callback out of it, to avoid leaking it
outside `intel_uc.c`

Cc: Joonas Lahtinen 
Cc: Michal Wajdeczko 
Cc: Michal Winiarski 
Signed-off-by: Arkadiusz Hiler 
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 137 +--
 drivers/gpu/drm/i915/intel_huc.c|   2 +-
 drivers/gpu/drm/i915/intel_uc.c | 141 
 drivers/gpu/drm/i915/intel_uc.h |   4 +-
 4 files changed, 145 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 753aeef..110dfd1 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -26,7 +26,6 @@
  *Dave Gordon 
  *Alex Dai 
  */
-#include 
 #include "i915_drv.h"
 #include "intel_uc.h"
 
@@ -587,140 +586,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
return ret;
 }
 
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-struct intel_uc_fw *uc_fw)
-{
-   struct pci_dev *pdev = dev_priv->drm.pdev;
-   struct drm_i915_gem_object *obj;
-   const struct firmware *fw = NULL;
-   struct uc_css_header *css;
-   size_t size;
-   int err;
-
-   DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
-   intel_uc_fw_status_repr(uc_fw->fetch_status));
-
-   err = request_firmware(, uc_fw->path, >dev);
-   if (err)
-   goto fail;
-   if (!fw)
-   goto fail;
-
-   DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
-   uc_fw->path, fw);
-
-   /* Check the size of the blob before examining buffer contents */
-   if (fw->size < sizeof(struct uc_css_header)) {
-   DRM_NOTE("Firmware header is missing\n");
-   goto fail;
-   }
-
-   css = (struct uc_css_header *)fw->data;
-
-   /* Firmware bits always start from header */
-   uc_fw->header_offset = 0;
-   uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
-   css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
-
-   if (uc_fw->header_size != sizeof(struct uc_css_header)) {
-   DRM_NOTE("CSS header definition mismatch\n");
-   goto fail;
-   }
-
-   /* then, uCode */
-   uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
-   uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
-
-   /* now RSA */
-   if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-   DRM_NOTE("RSA key size is bad\n");
-   goto fail;
-   }
-   uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
-   uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
-
-   /* At least, it should have header, uCode and RSA. Size of all three. */
-   size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
-   if (fw->size < size) {
-   DRM_NOTE("Missing firmware components\n");
-   goto fail;
-   }
-
-   /*
-* The GuC firmware image has the version number embedded at a 
well-known
-* offset within the firmware blob; note that major / minor version are
-* TWO bytes each (i.e. u16), although all pointers and offsets are 
defined
-* in terms of bytes (u8).
-*/
-   switch (uc_fw->fw) {
-   case INTEL_UC_FW_TYPE_GUC:
-   /* Header and uCode will be loaded to WOPCM. Size of the two. */
-   size = uc_fw->header_size + uc_fw->ucode_size;
-
-   /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
-   if (size > intel_guc_wopcm_size(dev_priv)) {
-   DRM_ERROR("Firmware is too large to fit in WOPCM\n");
-   goto fail;
-   }
-   uc_fw->major_ver_found = css->guc.sw_version >> 16;
-   uc_fw->minor_ver_found = css->guc.sw_version & 0x;
-   break;
-
-   case INTEL_UC_FW_TYPE_HUC:
-   uc_fw->major_ver_found = css->huc.sw_version >> 16;
-   uc_fw->minor_ver_found = css->huc.sw_version & 0x;
-   break;
-
-   default:
-   DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw);
-   err = -ENOEXEC;
-   goto fail;
-   }
-
-   if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
-   uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
-   DRM_NOTE("uC firmware version %d.%d,