Re: [PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

Daniele Ceraolo Spurio
2017-01-11T22:26:24Z

On 11/01/17 07:17, Michał Winiarski wrote:
> Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT offsets used
> by the GuC are mappable"), we're asserting that GuC firmware is in the
> GuC mappable range.
> Except we're not pinning the object with bias, which means it's possible
> to trigger this assert. Let's add a proper bias.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
> drivers/gpu/drm/i915/intel_guc_loader.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index aa2b866..5a6ab87 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -360,7 +360,8 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> return ret;
> }
>
> - vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> + vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0,
> + PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> if (IS_ERR(vma)) {
> DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> return PTR_ERR(vma);
>

This patch made me think about this again and actually I'm not sure
anymore that there is an offset requirement for the firmware object.
With the way we load the firmware the GuC should never access it in GGTT
because it is first copied in WOPCM via DMA, which should be able to
access the whole address range. I've asked a GuC dev but he has not been
able to confirm if there are any offset limitation with the DMA transfer
or not and unfortunately I don't have a platform to test this on at the
moment. I'll try to get my hands on a new SKL and double check.
Anyway, I'm happy to merge this while we clarify the requirement because
the firmware vma is immediately unpinned after the transfer so there
should be no risk of unneeded ggtt fragmentation; it also looks
generally cleaner to me to handle all guc-related objects the same way.

Thanks,
Daniele