| Message ID | 20171005091349.9315-1-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michał Winiarski (2017-10-05 10:13:40) > We're using first page of kernel context state to share data with GuC, > let's precompute the ggtt offset at GuC initialization time rather than > everytime we're using GuC actions. So LRC_GUCSHR_PN is still 0. Plans for that to change? Atm, we should be changing one pointer deref for another... -Chris >
On Thu, 05 Oct 2017 11:13:40 +0200, Michał Winiarski <michal.winiarski@intel.com> wrote: > We're using first page of kernel context state to share data with GuC, > let's precompute the ggtt offset at GuC initialization time rather than > everytime we're using GuC actions. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 4 ++-- > drivers/gpu/drm/i915/intel_uc.c | 4 ++++ > drivers/gpu/drm/i915/intel_uc.h | 3 +++ > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > b/drivers/gpu/drm/i915/i915_guc_submission.c > index 04f1281d81a5..2c0aeee3143d 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1234,7 +1234,7 @@ int intel_guc_suspend(struct drm_i915_private > *dev_priv) > /* any value greater than GUC_POWER_D0 */ > data[1] = GUC_POWER_D1; > /* first page is shared data with GuC */ Please drop this now redundant comment > - data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * > PAGE_SIZE; > + data[2] = guc->shared_data_offset; > return intel_guc_send(guc, data, ARRAY_SIZE(data)); > } > @@ -1260,7 +1260,7 @@ int intel_guc_resume(struct drm_i915_private > *dev_priv) > data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; > data[1] = GUC_POWER_D0; > /* first page is shared data with GuC */ Please drop this now redundant comment > - data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * > PAGE_SIZE; > + data[2] = guc->shared_data_offset; > return intel_guc_send(guc, data, ARRAY_SIZE(data)); > } > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index e7875277ba97..f4893c85e54a 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -173,6 +173,10 @@ static void guc_free_load_err_log(struct intel_guc > *guc) > static int guc_enable_communication(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > + struct i915_gem_context *ctx = dev_priv->kernel_context; > + > + guc->shared_data_offset = guc_ggtt_offset(ctx->engine[RCS].state) + > + LRC_GUCSHR_PN * PAGE_SIZE; Hmm, I'm not sure that this is the best place for such initialization. What about doing it directly in intel_uc_init_hw() or in dedicated inline ? > if (HAS_GUC_CT(dev_priv)) > return intel_guc_enable_ct(guc); > diff --git a/drivers/gpu/drm/i915/intel_uc.h > b/drivers/gpu/drm/i915/intel_uc.h > index 4fa091e90b5f..10e8f0ed02e4 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -121,6 +121,9 @@ struct intel_guc { > /* To serialize the intel_guc_send actions */ > struct mutex send_mutex; > + /* Kernel context state ggtt offset, first page is shared with GuC */ > + u32 shared_data_offset; We want to keep all 'send' related members together. Maybe better place for this new member will be after 'execbuf_client' ? > + > /* GuC's FW specific send function */ > int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
On 05/10/17 02:33, Chris Wilson wrote: > Quoting Michał Winiarski (2017-10-05 10:13:40) >> We're using first page of kernel context state to share data with GuC, >> let's precompute the ggtt offset at GuC initialization time rather than >> everytime we're using GuC actions. > > So LRC_GUCSHR_PN is still 0. Plans for that to change? > This is a requirement from the GuC side. GuC expects each context to have that extra page before the PPHWSP and it uses it to dump some per-lrc info, part of which is for internal use and part is info for the host (although we don't need/use it). On certain events (reset/preempt/suspend etc) GuC will dump extra info and this is done in the page provided in the H2G. I think we use the one of the default ctx just for simplicity, but it should be possible to use a different one, possibly not attached to any lrc if needed, but I'm not sure if this has ever been tested. -Daniele > Atm, we should be changing one pointer deref for another... > -Chris >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Oct 05, 2017 at 05:02:39PM +0000, Daniele Ceraolo Spurio wrote: > > > On 05/10/17 02:33, Chris Wilson wrote: > > Quoting Michał Winiarski (2017-10-05 10:13:40) > > > We're using first page of kernel context state to share data with GuC, > > > let's precompute the ggtt offset at GuC initialization time rather than > > > everytime we're using GuC actions. > > > > So LRC_GUCSHR_PN is still 0. Plans for that to change? > > > > This is a requirement from the GuC side. GuC expects each context to have > that extra page before the PPHWSP and it uses it to dump some per-lrc info, > part of which is for internal use and part is info for the host (although we > don't need/use it). > On certain events (reset/preempt/suspend etc) GuC will dump extra info and > this is done in the page provided in the H2G. I think we use the one of the > default ctx just for simplicity, but it should be possible to use a > different one, possibly not attached to any lrc if needed, but I'm not sure > if this has ever been tested. Done that (allocating a separate object for GuC shared data), seems to work just fine on its own. Except if we try to remove the first page from contexts. It seems to make GuC upset even though we're not using actions. We could still do that, though without removing the extra page we're just being more wasteful. But perhaps it's cleaner that way? Having separate managed in GuC code rather than reusing random places in context state? Thoughts? -Michał > > -Daniele > > > Atm, we should be changing one pointer deref for another... > > -Chris > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On 06/10/17 05:35, Michał Winiarski wrote: > On Thu, Oct 05, 2017 at 05:02:39PM +0000, Daniele Ceraolo Spurio wrote: >> >> >> On 05/10/17 02:33, Chris Wilson wrote: >>> Quoting Michał Winiarski (2017-10-05 10:13:40) >>>> We're using first page of kernel context state to share data with GuC, >>>> let's precompute the ggtt offset at GuC initialization time rather than >>>> everytime we're using GuC actions. >>> >>> So LRC_GUCSHR_PN is still 0. Plans for that to change? >>> >> >> This is a requirement from the GuC side. GuC expects each context to have >> that extra page before the PPHWSP and it uses it to dump some per-lrc info, >> part of which is for internal use and part is info for the host (although we >> don't need/use it). >> On certain events (reset/preempt/suspend etc) GuC will dump extra info and >> this is done in the page provided in the H2G. I think we use the one of the >> default ctx just for simplicity, but it should be possible to use a >> different one, possibly not attached to any lrc if needed, but I'm not sure >> if this has ever been tested. > > Done that (allocating a separate object for GuC shared data), seems to > work just fine on its own. Except if we try to remove the first page from > contexts. It seems to make GuC upset even though we're not using actions. > Yep, as I mentioned above GuC dumps runtime info about each lrc it handles in that page (e.g. if an lrc has been submitted via proxy), so it is probably going to either page-fault or write in the wrong memory if that page is not allocated. > We could still do that, though without removing the extra page we're just being > more wasteful. But perhaps it's cleaner that way? Having separate managed in GuC > code rather than reusing random places in context state? Thoughts? > This is similar to what we used to do by using the PPHWSP of the default ctx as the global HWSP. Personally I'd prefer to keep it separate as it feels cleaner and a single extra page shouldn't hurt us that much, but there was some push-back when I suggested the same for the HWSP. Daniele > -Michał > >> >> -Daniele >> >>> Atm, we should be changing one pointer deref for another... >>> -Chris >>>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 04f1281d81a5..2c0aeee3143d 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1234,7 +1234,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) /* any value greater than GUC_POWER_D0 */ data[1] = GUC_POWER_D1; /* first page is shared data with GuC */ - data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE; + data[2] = guc->shared_data_offset; return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -1260,7 +1260,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; data[1] = GUC_POWER_D0; /* first page is shared data with GuC */ - data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE; + data[2] = guc->shared_data_offset; return intel_guc_send(guc, data, ARRAY_SIZE(data)); } diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index e7875277ba97..f4893c85e54a 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -173,6 +173,10 @@ static void guc_free_load_err_log(struct intel_guc *guc) static int guc_enable_communication(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); + struct i915_gem_context *ctx = dev_priv->kernel_context; + + guc->shared_data_offset = guc_ggtt_offset(ctx->engine[RCS].state) + + LRC_GUCSHR_PN * PAGE_SIZE; if (HAS_GUC_CT(dev_priv)) return intel_guc_enable_ct(guc); diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 4fa091e90b5f..10e8f0ed02e4 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -121,6 +121,9 @@ struct intel_guc { /* To serialize the intel_guc_send actions */ struct mutex send_mutex; + /* Kernel context state ggtt offset, first page is shared with GuC */ + u32 shared_data_offset; + /* GuC's FW specific send function */ int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
We're using first page of kernel context state to share data with GuC, let's precompute the ggtt offset at GuC initialization time rather than everytime we're using GuC actions. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jeff Mcgee <jeff.mcgee@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 4 ++-- drivers/gpu/drm/i915/intel_uc.c | 4 ++++ drivers/gpu/drm/i915/intel_uc.h | 3 +++ 3 files changed, 9 insertions(+), 2 deletions(-)