On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote:
> On Thu, 04 May 2017, Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> wrote:
> > We are using some scratch registers in MMIO based send function.
> > Make their base and count flexible in preparation of upcoming
> > GuC firmware/hardware changes. While around, change cmd len
> > parameter verification from WARN_ON to GEM_BUG_ON as we don't
> > need this all the time.
>
> I'm not generally fond of caching the registers like this or adding
> _MMIO() wrapping outside of i915_reg.h. Sure, we have some of that here
> and there, but here it's hard to see the rationale because you do this
> in preparation for something that we you're not sharing.
>
I can't share details atm, but as commit message says, there will be a
change in both offsets and number of scratch registers.
Imho any wrapping around these values can't go to the i915_[guc_]reg.h file
as that file shall include only raw MMIO definitions, without any extra
logic that is based on GEN or PLATFORM or FW version.
Alternate approach would be, thanks to the already defined virtual function
send(), to create new send_mmio function(s) that will be 100% the same as
the old send_mmio except offset and count of the scratch registers.
Then we can benefit from most optimal implementation per GEN|PLATFORM|FW that
can run without reading cached regs offsets/count, but at the cost of extra
code that need to be maintained to be in sync with the original function.
And then someone else can point out that we missed code sharing opportunity.
I'm afraid there is no clear winner.
-Michal
> BR,
> Jani.
>
> >
> > v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
> >
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
> > drivers/gpu/drm/i915/intel_uc.h | 7 +++++++
> > 2 files changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 72f49e6..9d11c42 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
> > __intel_uc_fw_fini(&dev_priv->huc.fw);
> > }
> >
> > +static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> > +{
> > + GEM_BUG_ON(!guc->send_regs.base);
> > + GEM_BUG_ON(!guc->send_regs.count);
> > + GEM_BUG_ON(i >= guc->send_regs.count);
> > +
> > + return _MMIO(guc->send_regs.base + 4 * i);
> > +}
> > +
> > +static void guc_init_send_regs(struct intel_guc *guc)
> > +{
> > + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > + enum forcewake_domains fw_domains = 0;
> > + u32 i;
> > +
> > + guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> > + guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> > +
> > + for (i = 0; i < guc->send_regs.count; i++) {
> > + fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> > + guc_send_reg(guc, i),
> > + FW_REG_READ | FW_REG_WRITE);
> > + }
> > + guc->send_regs.fw_domains = fw_domains;
> > +}
> > +
> > static int guc_enable_communication(struct intel_guc *guc)
> > {
> > /* XXX: placeholder for alternate setup */
> > + guc_init_send_regs(guc);
> > guc->send = intel_guc_send_mmio;
> > return 0;
> > }
> > @@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> > int i;
> > int ret;
> >
> > - if (WARN_ON(len < 1 || len > 15))
> > - return -EINVAL;
> > + GEM_BUG_ON(!len);
> > + GEM_BUG_ON(len > guc->send_regs.count);
> >
> > mutex_lock(&guc->send_mutex);
> > - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
> > + intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
> >
> > dev_priv->guc.action_count += 1;
> > dev_priv->guc.action_cmd = action[0];
> >
> > for (i = 0; i < len; i++)
> > - I915_WRITE(SOFT_SCRATCH(i), action[i]);
> > + I915_WRITE(guc_send_reg(guc, i), action[i]);
> >
> > - POSTING_READ(SOFT_SCRATCH(i - 1));
> > + POSTING_READ(guc_send_reg(guc, i - 1));
> >
> > intel_guc_notify(guc);
> >
> > @@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> > * Fast commands should still complete in 10us.
> > */
> > ret = __intel_wait_for_register_fw(dev_priv,
> > - SOFT_SCRATCH(0),
> > + guc_send_reg(guc, 0),
> > INTEL_GUC_RECV_MASK,
> > INTEL_GUC_RECV_MASK,
> > 10, 10, &status);
> > @@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> > }
> > dev_priv->guc.action_status = status;
> >
> > - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
> > + intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> > mutex_unlock(&guc->send_mutex);
> >
> > return ret;
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 097289b..a37a8cc 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -205,6 +205,13 @@ struct intel_guc {
> > uint64_t submissions[I915_NUM_ENGINES];
> > uint32_t last_seqno[I915_NUM_ENGINES];
> >
> > + /* GuC's FW specific registers used in MMIO send */
> > + struct {
> > + u32 base;
> > + u32 count;
> > + u32 fw_domains; /* enum forcewake_domains */
> > + } send_regs;
> > +
> > /* To serialize the intel_guc_send actions */
> > struct mutex send_mutex;
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
