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

[Dave Gordon <david.s.gordon@intel.com>]

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.

The above bikeshedding notwithstanding, it gets my R-b anyway:

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx