On Mon, Dec 08, 2014 at 02:57:31PM +0000, Dave Gordon wrote: > On 05/12/14 12:51, Ville Syrjälä wrote: > > On Fri, Nov 14, 2014 at 06:16:56PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote: > >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> > >> MI_STORE_DWORD_IMM length has been the same ever since gen4. Rename > >> the define to avoid potential confusion if someone tries to use this > >> on pre-gen8. > >> > >> Also correct the comment on MI_MEM_VIRTUAL bit. It's present on 945,g33 > >> and 965 only. > >> > >> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > ping > > > >> --- > >> drivers/gpu/drm/i915/i915_reg.h | 4 ++-- > >> drivers/gpu/drm/i915/intel_lrc.c | 2 +- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 3de58ac..5228493 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -280,8 +280,8 @@ > >> #define MI_SEMAPHORE_POLL (1<<15) > >> #define MI_SEMAPHORE_SAD_GTE_SDD (1<<12) > >> #define MI_STORE_DWORD_IMM MI_INSTR(0x20, 1) > >> -#define MI_STORE_DWORD_IMM_GEN8 MI_INSTR(0x20, 2) > >> -#define MI_MEM_VIRTUAL (1 << 22) /* 965+ only */ > >> +#define MI_STORE_DWORD_IMM_GEN4 MI_INSTR(0x20, 2) > >> +#define MI_MEM_VIRTUAL (1 << 22) /* 945,g33,965 */ > >> #define MI_STORE_DWORD_INDEX MI_INSTR(0x21, 1) > >> #define MI_STORE_DWORD_INDEX_SHIFT 2 > >> /* Official intel docs are somewhat sloppy concerning MI_LOAD_REGISTER_IMM: > >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >> index 6025ac7..649d9ba 100644 > >> --- a/drivers/gpu/drm/i915/intel_lrc.c > >> +++ b/drivers/gpu/drm/i915/intel_lrc.c > >> @@ -1188,7 +1188,7 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf) > >> if (ret) > >> return ret; > >> > >> - cmd = MI_STORE_DWORD_IMM_GEN8; > >> + cmd = MI_STORE_DWORD_IMM_GEN4; > >> cmd |= MI_GLOBAL_GTT; > >> > >> intel_logical_ring_emit(ringbuf, cmd); > >> -- > >> 2.0.4 > > The GEN8->GEN4 change looks sensible to me. But I was puzzled over the > definition of MI_STORE_DWORD_IMM -- presumably this apparently more > generic definition is actually a pre-GEN4 legacy from the days when the > instruction had only 3 words? Perhaps a comment here from somebody who > knows -- or even something like > > #define MI_STORE_DWORD_IMM_LEGACY MI_INSTR(0x20, 1) /* pre-GEN4 */ > #define MI_STORE_DWORD_IMM_GEN4 MI_INSTR(0x20, 2) /* GEN4+ */ > #define MI_STORE_DWORD_IMM MI_STORE_DWORD_IMM_LEGACY /* for source > backward compatibility */ > > so it's clearer that (for now) the generic definition is actually the > legacy one. We tend to name things based on the oldest platform supporting it, so I think it's fairly clear the _IMM is for old stuff, and _IMM_GEN4 is for gen4+. To make it more clear we could add a _GEN2 suffix for the old platform define, but I don't see much point with that myself. Oh and source backwards compatibility is something we never really have to worry about. All the code is in the same tree, so we can just change all the users at the same time. Doing otherwise would just lead to a mess. > > The above bikeshedding notwithstanding, it gets my R-b anyway: > > Reviewed-by: Dave Gordon <david.s.gordon@xxxxxxxxx> -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
