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

["O'Rourke, Tom" <Tom.O'Rourke@intel.com>]

[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".

> 
> 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.

> +	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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx