| Message ID | 1436466554-24806-8-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[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 >
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). > > > > 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
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. > >> > >> 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
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
On 28/07/15 00:12, 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. I'm inclined to set the bias to GUC_WOPCM_TOP, and then define that as the sum of GUC_WOPCM_OFFSET_VALUE and GUC_WOPCM_SIZE_VALUE. That seems to be what the BSpec pages "WriteOnceProtectedContentMemory (WOPCM) Management" and "WOPCM Memory Map" suggest, although I think they're pretty unclear on the details :( Do you (both) agree this would be the right value? [snip] >>>> + /* 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 I changed the line above to "GuC submission", while the one a few lines further down now says: /* Unmask this bit to enable the GuC's internal scheduler */ to make it quite clear that we're not referring to the host-based GPU scheduler curently in review. .Dave.
On 28/07/15 16:16, Dave Gordon wrote: > On 28/07/15 00:12, 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. > > I'm inclined to set the bias to GUC_WOPCM_TOP, and then define that as > the sum of GUC_WOPCM_OFFSET_VALUE and GUC_WOPCM_SIZE_VALUE. That seems > to be what the BSpec pages "WriteOnceProtectedContentMemory (WOPCM) > Management" and "WOPCM Memory Map" suggest, although I think they're > pretty unclear on the details :( > > Do you (both) agree this would be the right value? Actually I've changed my mind (again). On rereading this stuff, I now think that GUC_WOPCM_TOP is the same as the value put into the SIZE register. The (physical) range between the (real) WOPCM BASE and that plus the GUC WOPCM OFFSET isn't part of the GuC address space at all, so GuC address 0 maps (would map) to (real WOPCM BASE+GUC WOPCM OFFSET) in physical addresses, except that the bottom 80k is shadowed by the bootrom and SRAM; and then the SIZE register defines the size of the range from (GuC address) 0 to GUC_WOPCM_TOP; and then higher addresses map through the GTT as expected. Or so I think. Does anyone know for sure? Please let me know ASAP as I want to submit an updated patchset tomorrow! Thanks, .Dave.
On Tue, Jul 28, 2015 at 04:16:03PM +0100, Dave Gordon wrote: > On 28/07/15 00:12, 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. > > I'm inclined to set the bias to GUC_WOPCM_TOP, and then define that > as the sum of GUC_WOPCM_OFFSET_VALUE and GUC_WOPCM_SIZE_VALUE. That > seems to be what the BSpec pages "WriteOnceProtectedContentMemory > (WOPCM) Management" and "WOPCM Memory Map" suggest, although I think > they're pretty unclear on the details :( > > Do you (both) agree this would be the right value? [TOR:] No, I do not think that is the right value. I think the excluded range should be [0 ... GUC_WOPCM_SIZE_VALUE) and that GUC_WOPCM_SIZE_VALUE should be used as the bias (as it is now) for objects used by GuC. The term "WOPCM_SIZE" is ambiguous since it could mean GUC_WOPCM_SIZE (as in 0xc050) or it could mean "size of WOPCM area" (as in 0x1082C0). It gets used both ways in the BSpec. > > [snip] [snip]
On Tue, Jul 28, 2015 at 08:40:58PM +0100, Dave Gordon wrote: > On 28/07/15 16:16, Dave Gordon wrote: > >On 28/07/15 00:12, 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. > > > >I'm inclined to set the bias to GUC_WOPCM_TOP, and then define that as > >the sum of GUC_WOPCM_OFFSET_VALUE and GUC_WOPCM_SIZE_VALUE. That seems > >to be what the BSpec pages "WriteOnceProtectedContentMemory (WOPCM) > >Management" and "WOPCM Memory Map" suggest, although I think they're > >pretty unclear on the details :( > > > >Do you (both) agree this would be the right value? > > Actually I've changed my mind (again). On rereading this stuff, I > now think that GUC_WOPCM_TOP is the same as the value put into the > SIZE register. The (physical) range between the (real) WOPCM BASE > and that plus the GUC WOPCM OFFSET isn't part of the GuC address > space at all, so GuC address 0 maps (would map) to (real WOPCM > BASE+GUC WOPCM OFFSET) in physical addresses, except that the bottom > 80k is shadowed by the bootrom and SRAM; and then the SIZE register > defines the size of the range from (GuC address) 0 to GUC_WOPCM_TOP; > and then higher addresses map through the GTT as expected. > > Or so I think. Does anyone know for sure? Please let me know ASAP as > I want to submit an updated patchset tomorrow! > > Thanks, > .Dave. [TOR:] Hi Dave, Sorry, I did not see your message earlier. Please see my other reply on this thread. I think you are right here, but to be clear, I think by "SIZE" you mean "GUC_WOPCM_SIZE_VALUE". Also, this should not matter here, but the SKL guc SRAM shadow is 128k, not 80k. Thanks, Tom
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); + #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. */ + 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;