<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body style="background-color: rgb(255, 255, 255); color: rgb(0, 0, 0);" bgcolor="#FFFFFF" text="#000000"> <br> <br> <div class="moz-cite-prefix">On 07/27/2015 04:12 PM, O'Rourke, Tom wrote:<br> </div> <blockquote style="border-left: 2px solid #009900 ; border-right: 2px solid #009900 ; padding: 0px 15px 0px 15px; margin: 8px 2px;" cite="mid:20150727231206.GB76344@torourke-desk1" type="cite"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:yu.dai@intel.com"><yu.dai@intel.com></a> > >> > >> 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. </pre> </blockquote> <br> 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 (<b>Physical WOPCM allocated size - GuC WOPCM Base</b>). 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.<br> <br> Alex<br> <br> <blockquote style="border-left: 2px solid #009900 ; border-right: 2px solid #009900 ; padding: 0px 15px 0px 15px; margin: 8px 2px;" cite="mid:20150727231206.GB76344@torourke-desk1" type="cite"> <pre wrap="">> >> > >> 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 <a class="moz-txt-link-rfc2396E" href="mailto:yu.dai@intel.com"><yu.dai@intel.com></a> > >> Signed-off-by: Dave Gordon <a class="moz-txt-link-rfc2396E" href="mailto:david.s.gordon@intel.com"><david.s.gordon@intel.com></a> > >> --- > >> 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 </pre> </blockquote> <br> </body> </html>