Re: [PATCH v3 03/15] drm/i915: Unified firmware loading mechanism

[Dave Gordon <david.s.gordon@intel.com>]

On 17/04/15 22:21, yu.dai@intel.com wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
> 
> Factor out the common code of loading firmware into a new file,
> leaving only the uC-specific parts in the GuC loaders.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile          |   3 +
>  drivers/gpu/drm/i915/intel_uc_loader.c | 244 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc_loader.h |  78 +++++++++++
>  3 files changed, 325 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/intel_uc_loader.c
>  create mode 100644 drivers/gpu/drm/i915/intel_uc_loader.h

In general, yes; but I noticed that the firmware image blob isn't
released in the 'success' path, which means we're keeping both the raw
image and the GEM copy around forever. It would be better to release the
raw image and only keep the GEM object (which is swappable).

Unfortunately, that wouldn't work with the current version of the GuC
loader because of the way it needs the data reorganised for DMA, and
therefore accesses the blob on each reload :(

So I think the way to go here is to upgrade the uc_fw_check() callback
to allow it to override the way the data is saved, if the default (GEM
object is just a copy of the firmware image) doesn't match the specific
uC requirements.

The GuC callback can then shuffle the data during this one-off copy, and
the subsequent DMA transfer will be much more straightforward.

I'll post a new suggestion for this file soon, probably tomorrow ...

.Dave.

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

> +static void uc_fw_finish(struct drm_device *dev, struct intel_uc_fw *uc_fw)
> +{
> +	struct drm_i915_gem_object *obj = NULL;
> +	const struct firmware *fw;
> +
> +	DRM_DEBUG_DRIVER("before waiting: %s fw fetch status %d, fw %p\n",
> +		uc_fw->uc_name, uc_fw->uc_fw_fetch_status, uc_fw->uc_fw_blob);
> +
> +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +	WARN_ON(uc_fw->uc_fw_fetch_status != INTEL_UC_FIRMWARE_PENDING);
> +
> +	wait_for_completion(&uc_fw->uc_fw_fetched);
> +
> +	DRM_DEBUG_DRIVER("after waiting: %s fw fetch status %d, fw %p\n",
> +		uc_fw->uc_name, uc_fw->uc_fw_fetch_status, uc_fw->uc_fw_blob);
> +
> +	fw = uc_fw->uc_fw_blob;
> +	if (!fw) {
> +		/* no firmware found; try again in case FS was not mounted */
> +		DRM_DEBUG_DRIVER("retry fetching %s fw from <%s>\n",
> +			uc_fw->uc_name, uc_fw->uc_fw_path);
> +		if (request_firmware(&fw, uc_fw->uc_fw_path, &dev->pdev->dev))
> +			goto fail;
> +		DRM_DEBUG_DRIVER("fetch %s fw from <%s> succeeded, fw %p\n",
> +			uc_fw->uc_name, uc_fw->uc_fw_path, fw);
> +		uc_fw->uc_fw_blob = fw;
> +	}
> +
> +	obj = i915_gem_alloc_object(dev, round_up(fw->size, PAGE_SIZE));
> +	if (!obj)
> +		goto fail;
> +
> +	if (i915_gem_object_write(obj, fw->data, fw->size))
> +		goto fail;
> +
> +	DRM_DEBUG_DRIVER("%s fw fetch status SUCCESS\n", uc_fw->uc_name);
> +	uc_fw->uc_fw_obj = obj;
> +	uc_fw->uc_fw_size = fw->size;
> +	uc_fw->uc_fw_fetch_status = INTEL_UC_FIRMWARE_SUCCESS;

We ought to be able to release_firmware() here.

> +	return;
> +
> +fail:
> +	DRM_DEBUG_DRIVER("%s fw fetch status FAIL; fw %p, obj %p\n",
> +		uc_fw->uc_name, fw, obj);
> +	DRM_ERROR("Failed to fetch %s firmware from <%s>\n",
> +		  uc_fw->uc_name, uc_fw->uc_fw_path);
> +
> +	if (obj)
> +		drm_gem_object_unreference(&obj->base);
> +
> +	uc_fw->uc_fw_fetch_status = INTEL_UC_FIRMWARE_FAIL;
> +	uc_fw->uc_fw_blob = NULL;
> +	release_firmware(fw);		/* OK even if fw is NULL */
> +}

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx