Re: [PATCH] drm/i915: s/MI_STORE_DWORD_IMM_GEN8/MI_STORE_DWORD_IMM_GEN4/

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> 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@intel.com>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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@intel.com>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx