[PATCH] drm/i915/gsc: GSC firmware loading

[Teres Alexis, Alan Previn]

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 at intel.com>

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]