Re: [PATCH v3 6/6] drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 12, 2015 at 06:03:55PM +0100, Dave Gordon wrote:
> On 12/06/15 12:58, Siluvery, Arun wrote:
> > On 09/06/2015 19:43, Dave Gordon wrote:
> >> On 05/06/15 14:57, Arun Siluvery wrote:
> >>> In Per context w/a batch buffer,
> >>> WaRsRestoreWithPerCtxtBb
> >>>
> >>> v2: This patches modifies definitions of MI_LOAD_REGISTER_MEM and
> >>> MI_LOAD_REGISTER_REG; Add GEN8 specific defines for these instructions
> >>> so as to not break any future users of existing definitions (Michel)
> >>>
> >>> Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
> >>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_reg.h  | 26 ++++++++++++++++++
> >>>   drivers/gpu/drm/i915/intel_lrc.c | 59
> >>> ++++++++++++++++++++++++++++++++++++++++
> >>>   2 files changed, 85 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >>> b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 33b0ff1..6928162 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> [snip]
> >>>   #define MI_LOAD_REGISTER_MEM    MI_INSTR(0x29, 0)
> >>>   #define MI_LOAD_REGISTER_REG    MI_INSTR(0x2A, 0)
> >>> +#define MI_LOAD_REGISTER_MEM_GEN8 MI_INSTR(0x29, 2)
> >>> +#define   MI_LRM_USE_GLOBAL_GTT (1<<22)
> >>> +#define   MI_LRM_ASYNC_MODE_ENABLE (1<<21)
> >>> +#define MI_LOAD_REGISTER_REG_GEN8 MI_INSTR(0x2A, 1)
> >>
> >> Isn't the original definition of MI_LOAD_REGISTER_REG wrong anyway? It's
> >> a two-operand instruction, each of which is a one-word MMIO register
> >> address, hence always 3 words total. The length bias is 2, so the
> >> so-called 'flags' field must be 1. The original definition (where the
> >> second argument of the MI_INSTR macro is 0) shouldn't work.
> >>
> >> So just correct the original definition of MI_LOAD_REGISTER_REG; this
> >> isn't something that's actually changed on GEN8.
> >>
> > I did notice that the original instructions are odd but thought I might
> > be wrong hence I created new ones to not disturb the original ones.
> > ok I will just correct original one and reuse it.
> > 
> >> While we're mentioning it, I think the above MI_LOAD_REGISTER_MEM is
> >> wrong too. The length should be 1 pre-GEN8, and 2 in GEN8+.
> >>
> > ok.
> >>>   #define MI_RS_STORE_DATA_IMM    MI_INSTR(0x2B, 0)
> >>>   #define MI_LOAD_URB_MEM         MI_INSTR(0x2C, 0)
> >>>   #define MI_STORE_URB_MEM        MI_INSTR(0x2D, 0)
> >>
> >> And these are wrong too! In fact all of these instructions have been
> >> added under a comment which says "Commands used only by the command
> >> parser". Looks like they were added as placeholders without the proper
> >> length fields, and then people have started using them as though they
> >> were complete definitions :(
> >>
> >> Time update them all, perhaps ...
> > these are not related to this patch, so it can be taken up as a
> > different patch.
> 
> As a minimum, you should move these updated #defines out of the section
> under the comment "Commands used only by the command parser" and put
> them in the appropriate place in the regular list of MI_ commnds,
> preferably in numerical order. Then the ones that are genuinely only
> used by the command parser could be left for another patch ...

Please just correct the #defines while at it, this really is way to
tempting a trap to keep it hot. Can be done in a separate patch ofc, but
imo not fixing an obvious issue when we spot it because its not perfectly
directly related to the feature work at hand is bad practice leading to
piles of technical debt.

And that's the kind of stuff that robs me of my sleep at night ;-)

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx