Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible
On Thu, 04 May 2017, Chris Wilson wrote: > On Thu, May 04, 2017 at 06:26:04PM +0200, Michal Wajdeczko wrote: >> On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote: >> > On Thu, 04 May 2017, Michal Wajdeczko 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. > > The guc->send.base + offset approach is reasonable; it is certainly > the tried and trusted approach. I would stick with it, but we just can't > help with any suggestions without seeing the destination. That pretty much sums it up. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible
On Thu, May 04, 2017 at 06:26:04PM +0200, Michal Wajdeczko wrote: > On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote: > > On Thu, 04 May 2017, Michal Wajdeczko 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. The guc->send.base + offset approach is reasonable; it is certainly the tried and trusted approach. I would stick with it, but we just can't help with any suggestions without seeing the destination. Oh well, we can dream that instead of using mmio space for datagrams they move to ring (even WC will be better than a bunch of UC)! Don't overqualify the ints though, u32 base is ok, but it could be unsigned count (though an alternative would be u32 end, and even mark it as GEM_DEBUG_DECL!) and definitely unsigned fw_domains as that is not defined as being u32. (Just more than 32 domains is unlikely before tomorrow ;) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible
On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote:
> On Thu, 04 May 2017, Michal Wajdeczko 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
> > Suggested-by: Daniele Ceraolo Spurio
> > Cc: Daniele Ceraolo Spurio
> > Cc: Joonas Lahtinen
> > Reviewed-by: Daniele Ceraolo Spurio
> > ---
> > 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 @@ i
Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible
On Thu, 04 May 2017, Michal Wajdeczko 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.
BR,
Jani.
>
> v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
>
> Signed-off-by: Michal Wajdeczko
> Suggested-by: Daniele Ceraolo Spurio
> Cc: Daniele Ceraolo Spurio
> Cc: Joonas Lahtinen
> Reviewed-by: Daniele Ceraolo Spurio
> ---
> 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] [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible
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.
v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
Signed-off-by: Michal Wajdeczko
Suggested-by: Daniele Ceraolo Spurio
Cc: Daniele Ceraolo Spurio
Cc: Joonas Lahtinen
Reviewed-by: Daniele Ceraolo Spurio
---
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;
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
