Diffed the patches and reviewed the changes -- i.e. the use of the worker for the gsc fw loading cmd submission.
All looks good - just a few nits below.
Reviewed-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
On Mon, 2022-12-05 at 21:15 -0800, Ceraolo Spurio, Daniele wrote:
> GSC FW is loaded by submitting a dedicated command via the GSC engine.
> The memory area used for loading the FW is then re-purposed as local
> memory for the GSC itself, so we use a separate allocation instead of
> using the one where we keep the firmware stored for reload.
>
>
>
Alan:[snip]
> +int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
> +{
> + struct intel_gt *gt = gsc_uc_to_gt(gsc);
> + struct intel_uc_fw *gsc_fw = &gsc->fw;
> + int err;
> +
> + /* check current fw status */
> + if (intel_gsc_uc_fw_init_done(gsc)) {
> + if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
> + intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
> + return -EEXIST;
> + }
> +
Alan: Nit: I see you've put the -EEXIST back again - not sure if we need it. I'm marking this as Nit simply because we
dont consumer the return value from where its being called - but its still weird that we are returning an error in the
case where FW is already there so we skip loading and allow normal operational flow (not error-ing out).
Alan: Nit: not sure if we should explain in the comments how we can already find the gsc-fw pre-loaded (IIRC, it could
be a prior driver unload that didn't do the FLR?).
> + if (!intel_uc_fw_is_loadable(gsc_fw))
> + return -ENOEXEC;
> +
> + /* FW blob is ok, so clean the status */
> + intel_uc_fw_sanitize(&gsc->fw);
> +
> + if (!gsc_is_in_reset(gt->uncore))
> + return -EIO;
> +
> + err = gsc_fw_load_prepare(gsc);
> + if (err)
> + goto fail;
> +
> +
Alan: [snip]
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index 65cbf1ce9fa1..3692ba387834 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -7,8 +7,19 @@
>
> #include "gt/intel_gt.h"
> #include "intel_gsc_uc.h"
> +#include "intel_gsc_fw.h"
Alan: alphabetical ordering? (not sure if this is a hard rule, for me its a nit)
> #include "i915_drv.h"
>
> +static void gsc_work(struct work_struct *work)
Alan: Nit: would ne nicer to call it gsc_load_worker unless there maybe future plans to expand this worker's scope.
> +{
> + struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
> + struct intel_gt *gt = gsc_uc_to_gt(gsc);
> + intel_wakeref_t wakeref;
> +
> + with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> + intel_gsc_uc_fw_upload(gsc);
> +}
> +
Alan:[snip]
> struct intel_gsc_uc {
> /* Generic uC firmware management */
> struct intel_uc_fw fw;
> +
> + /* GSC-specific additions */
> + struct i915_vma *local; /* private memory for GSC usage */
> + struct intel_context *ce; /* for submission to GSC FW via GSC engine */
> +
> + struct work_struct work; /* for delayed load */
Alan: nit: would be nicer to call it "load_worker" unless the future plan is to expand the scope of what else that
worker could be used for.
Alan:[snip]
