[Intel-gfx] [PATCH 07/13 v4] drm/i915: GuC submission setup, phase 1
O'Rourke, Tom
Tom.O'Rourke at intel.com
Mon Jul 27 16:12:06 PDT 2015
On Mon, Jul 27, 2015 at 03:41:31PM -0700, Yu Dai wrote:
>
>
> On 07/24/2015 03:31 PM, O'Rourke, Tom wrote:
> >[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 at intel.com>
> >>
> >> 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".
>
> Nope, GGTT range [0..WOPCM_SIZE) should be excluded from GuC usage.
> BSpec clearly says, from 0 to WOPCM_TOP-1 is for BootROM, SRAM and
> WOPCM. From WOPCM_TOP and above is GFX DRAM. Be note that, that GGTT
> space is still available to any gfx obj as long as it is not
> accessed by GuC (OK to pass through GuC).
>
[TOR:] Should we take a closer look at the pin offset bias
for guc objects? GUC_WOPCM_SIZE_VALUE is not the full 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 at intel.com>
> >> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> >> ---
> >> 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.
>
> For now, yes the GuC scheduling is only doing the 'submission' work
> because of the current kernel in-order queue. If we have client
> direct submission enabled, there WILL BE GuC scheduling inside
> firmware according to the priority of each queue etc.
>
> Thanks,
> Alex
More information about the Intel-gfx
mailing list