Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base	and count flexible
Date: Thu, 4 May 2017 21:52:29 +0100	[thread overview]
Message-ID: <20170504205229.GQ24019@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20170504162604.GB41764@mwajdecz-MOBL1.ger.corp.intel.com>

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 <michal.wajdeczko@intel.com> 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