[TOR:] When I see "phase 1" I also look for "phase 2".
A subject that better describes the change in this patch
would help.
On Thu, Jul 09, 2015 at 07:29:08PM +0100, Dave Gordon wrote:
> From: Alex Dai <yu.dai@xxxxxxxxx>
>
> This adds the first of the data structures used to communicate with the
> GuC (the pool of guc_context structures).
>
> We create a GuC-specific wrapper round the GEM object allocator as all
> GEM objects shared with the GuC must be pinned into GGTT space at an
> address that is NOT in the range [0..WOPCM_SIZE), as that range of GGTT
> addresses is not accessible to the GuC (from the GuC's point of view,
> it's permanently reserved for other objects such as the BootROM & SRAM).
[TOR:] I would like a clarfication on the excluded range.
The excluded range should be 0 to "size for guc within
WOPCM area" and not 0 to "size of WOPCM area".
>
> Later, we will need to allocate additional GuC-sharable objects for the
> submission client(s) and the GuC's debug log.
>
> v2:
> Remove redundant initialisation [Chris Wilson]
> Defer adding struct members until needed [Chris Wilson]
> Local functions should pass dev_priv rather than dev [Chris Wilson]
>
> v4:
> Rebased
>
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx>
> Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 3 +-
> drivers/gpu/drm/i915/i915_guc_submission.c | 114 +++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_guc.h | 7 ++
> drivers/gpu/drm/i915/intel_guc_loader.c | 19 +++++
> 4 files changed, 142 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/i915/i915_guc_submission.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e604cfe..23f5612 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -40,7 +40,8 @@ i915-y += i915_cmd_parser.o \
> intel_uncore.o
>
> # general-purpose microcontroller (GuC) support
> -i915-y += intel_guc_loader.o
> +i915-y += intel_guc_loader.o \
> + i915_guc_submission.o
>
> # autogenerated null render state
> i915-y += intel_renderstate_gen6.o \
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> new file mode 100644
> index 0000000..70a0405
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +#include <linux/firmware.h>
> +#include <linux/circ_buf.h>
> +#include "i915_drv.h"
> +#include "intel_guc.h"
> +
> +/**
> + * gem_allocate_guc_obj() - Allocate gem object for GuC usage
> + * @dev: drm device
> + * @size: size of object
> + *
> + * This is a wrapper to create a gem obj. In order to use it inside GuC, the
> + * object needs to be pinned lifetime. Also we must pin it to gtt space other
> + * than [0, GUC_WOPCM_SIZE] because this range is reserved inside GuC.
> + *
> + * Return: A drm_i915_gem_object if successful, otherwise NULL.
> + */
> +static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
> + u32 size)
> +{
> + struct drm_i915_gem_object *obj;
> +
> + obj = i915_gem_alloc_object(dev, size);
> + if (!obj)
> + return NULL;
> +
> + if (i915_gem_object_get_pages(obj)) {
> + drm_gem_object_unreference(&obj->base);
> + return NULL;
> + }
> +
> + if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
> + PIN_OFFSET_BIAS | GUC_WOPCM_SIZE_VALUE)) {
> + drm_gem_object_unreference(&obj->base);
> + return NULL;
> + }
> +
> + return obj;
> +}
> +
> +/**
> + * gem_release_guc_obj() - Release gem object allocated for GuC usage
> + * @obj: gem obj to be released
> + */
> +static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
> +{
> + if (!obj)
> + return;
> +
> + if (i915_gem_obj_is_pinned(obj))
> + i915_gem_object_ggtt_unpin(obj);
> +
> + drm_gem_object_unreference(&obj->base);
> +}
> +
> +/*
> + * Set up the memory resources to be shared with the GuC. At this point,
> + * we require just one object that can be mapped through the GGTT.
> + */
> +int i915_guc_submission_init(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + const size_t ctxsize = sizeof(struct guc_context_desc);
> + const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
> + const size_t gemsize = round_up(poolsize, PAGE_SIZE);
> + struct intel_guc *guc = &dev_priv->guc;
> +
> + if (!i915.enable_guc_submission)
> + return 0; /* not enabled */
> +
> + if (guc->ctx_pool_obj)
> + return 0; /* already allocated */
> +
> + guc->ctx_pool_obj = gem_allocate_guc_obj(dev_priv->dev, gemsize);
> + if (!guc->ctx_pool_obj)
> + return -ENOMEM;
> +
> + ida_init(&guc->ctx_ids);
> +
> + return 0;
> +}
> +
> +void i915_guc_submission_fini(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_guc *guc = &dev_priv->guc;
> +
> + if (guc->ctx_pool_obj)
> + ida_destroy(&guc->ctx_ids);
> + gem_release_guc_obj(guc->ctx_pool_obj);
> + guc->ctx_pool_obj = NULL;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 2846b6d..be3cad8 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -56,6 +56,9 @@ struct intel_guc {
> struct intel_guc_fw guc_fw;
>
> uint32_t log_flags;
> +
> + struct drm_i915_gem_object *ctx_pool_obj;
> + struct ida ctx_ids;
> };
>
> /* intel_guc_loader.c */
> @@ -64,4 +67,8 @@ extern int intel_guc_ucode_load(struct drm_device *dev);
> extern void intel_guc_ucode_fini(struct drm_device *dev);
> extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>
> +/* i915_guc_submission.c */
> +int i915_guc_submission_init(struct drm_device *dev);
> +void i915_guc_submission_fini(struct drm_device *dev);
> +
[TOR:] i915_guc_submission_init gets used in this patch.
Unexpectedly, i915_guc_submission_fini does not get used
in this patch.
A later patch in this series adds the call to
i915_guc_submission_fini in intel_guc_ucode_fini.
Should that call be added in this patch?
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 2080bca..e5d7136 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -128,6 +128,21 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
> i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> }
>
> + /* If GuC scheduling is enabled, setup params here. */
[TOR:] I assume from this "GuC scheduling" == "GuC submission".
This is a little confusing. I recommend either reword
"GuC scheduling" or add comment text to explain.
> + if (i915.enable_guc_submission) {
> + u32 pgs = i915_gem_obj_ggtt_offset(dev_priv->guc.ctx_pool_obj);
> + u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
> +
> + pgs >>= PAGE_SHIFT;
> + params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
> + (ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
> +
> + params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
> +
> + /* Unmask this bit to enable GuC scheduler */
> + params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
> + }
> +
> I915_WRITE(SOFT_SCRATCH(0), 0);
>
> for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
> @@ -450,6 +465,10 @@ int intel_guc_ucode_load(struct drm_device *dev)
> break;
> }
>
> + err = i915_guc_submission_init(dev);
> + if (err)
> + goto fail;
> +
> err = guc_ucode_xfer(dev_priv);
> if (err)
> goto fail;
> --
> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
