Re: [PATCH 07/13 v4] drm/i915: GuC submission setup, phase 1

[Yu Dai <yu.dai@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10561 bytes --]



On 07/27/2015 04:12 PM, O'Rourke, Tom wrote:
> 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@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.
>   

Yes, GUC_WOPCM_SIZE_VALUE is not the full size of WOPCM area. You can 
blame BSpec for it asks SW to program this 'WOPCM Size' register to be 
(*Physical WOPCM allocated size - GuC WOPCM Base*). So if you offset 
range [GuC WOPCM Base, WOPCM_TOP] by GuC WOPCM Base, it will be [0, 
GUC_WOPCM_SIZE]. This is the GuC 'accessible' range we would like to 
exclude here.

Alex

> > >>
> > >> 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@intel.com>
> > >> Signed-off-by: Dave Gordon <david.s.gordon@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


[-- Attachment #1.2: Type: text/html, Size: 13623 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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