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 <joonas.lahti...@linux.intel.com>
> Cc: Michal Winiarski <michal.winiar...@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hi...@intel.com>

<SNIP>

> @@ -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(&dev_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(&dev_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(&dev_priv->guc->fw);
        if (ret)
                
goto err_huc;

        return 0;

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

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

>       intel_guc_fetch_fw(&dev_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

Reply via email to