[Mesa-dev] [PATCH v3 10/44] i965: Define and use REG_MASK macro to make masked MMIO writes slightly more readable.

[Francisco Jerez]

Kristian Høgsberg <krh at bitplanet.net> writes:

> On Tue, Dec 01, 2015 at 12:19:28AM -0800, Jordan Justen wrote:
>> From: Francisco Jerez <currojerez at riseup.net>
>> 
>> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_defines.h      | 6 ++++++
>>  src/mesa/drivers/dri/i965/brw_state_upload.c | 2 +-
>>  src/mesa/drivers/dri/i965/gen7_l3_state.c    | 2 +-
>>  src/mesa/drivers/dri/i965/intel_reg.h        | 2 +-
>>  4 files changed, 9 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
>> index a511d5c..94f878b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_defines.h
>> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
>> @@ -41,6 +41,12 @@
>>  #define GET_BITS(data, high, low) ((data & INTEL_MASK((high), (low))) >> (low))
>>  #define GET_FIELD(word, field) (((word)  & field ## _MASK) >> field ## _SHIFT)
>>  
>> +/**
>> + * For use with masked MMIO registers where the upper 16 bits control which
>> + * of the lower bits are committed to the register.
>> + */
>> +#define REG_MASK(value) ((value) << 16)
>
> Might nice to also take the value of the bit and do
>
>   #define REG_MASK(bit, enable) ( ((bit) << 16) | (enable ? (bit) : 0) )
>
I'd be okay with that, but...

>> +
>>  #ifndef BRW_DEFINES_H
>>  #define BRW_DEFINES_H
>>  
>> diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c b/src/mesa/drivers/dri/i965/brw_state_upload.c
>> index aab5c91..3d14d65 100644
>> --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
>> +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
>> @@ -382,7 +382,7 @@ brw_upload_initial_gpu_state(struct brw_context *brw)
>>        BEGIN_BATCH(3);
>>        OUT_BATCH(MI_LOAD_REGISTER_IMM | (3 - 2));
>>        OUT_BATCH(GEN7_CACHE_MODE_1);
>> -      OUT_BATCH((GEN9_PARTIAL_RESOLVE_DISABLE_IN_VC << 16) |
>> +      OUT_BATCH(REG_MASK(GEN9_PARTIAL_RESOLVE_DISABLE_IN_VC) |
>>                  GEN9_PARTIAL_RESOLVE_DISABLE_IN_VC);
>
> and then this becomes
>
>       OUT_BATCH(REG_MASK(GEN9_PARTIAL_RESOLVE_DISABLE_IN_VC, true));
>
> which avoids having to type GEN9_PARTIAL_RESOLVE_DISABLE_IN_VC twice.
>
>>     }
>> diff --git a/src/mesa/drivers/dri/i965/gen7_l3_state.c b/src/mesa/drivers/dri/i965/gen7_l3_state.c
>> index 9aad563..2c692be 100644
>> --- a/src/mesa/drivers/dri/i965/gen7_l3_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen7_l3_state.c
>> @@ -264,7 +264,7 @@ setup_l3_config(struct brw_context *brw, const struct brw_l3_config *cfg)
>>           OUT_BATCH(HSW_SCRATCH1);
>>           OUT_BATCH(has_dc ? 0 : HSW_SCRATCH1_L3_ATOMIC_DISABLE);
>>           OUT_BATCH(HSW_ROW_CHICKEN3);
>> -         OUT_BATCH(HSW_ROW_CHICKEN3_L3_ATOMIC_DISABLE << 16 |
>> +         OUT_BATCH(REG_MASK(HSW_ROW_CHICKEN3_L3_ATOMIC_DISABLE) |
>>                     (has_dc ? 0 : HSW_ROW_CHICKEN3_L3_ATOMIC_DISABLE));
>
> and this is even nicer:
>
>   OUT_BATCH(REG_MASK(HSW_ROW_CHICKEN3_L3_ATOMIC_DISABLE, has_dc));
>
> Also, multipe REG_MASK()s can be bitwise or-ed together to set or
> clear multiple bits.
>
>>           ADVANCE_BATCH();
>>        }
>> diff --git a/src/mesa/drivers/dri/i965/intel_reg.h b/src/mesa/drivers/dri/i965/intel_reg.h
>> index 0b167d5..8888d6f 100644
>> --- a/src/mesa/drivers/dri/i965/intel_reg.h
>> +++ b/src/mesa/drivers/dri/i965/intel_reg.h
>> @@ -183,7 +183,7 @@
>>  # define GEN8_HIZ_NP_EARLY_Z_FAILS_DISABLE (1 << 13)
>>  # define GEN9_PARTIAL_RESOLVE_DISABLE_IN_VC (1 << 1)
>>  # define GEN8_HIZ_PMA_MASK_BITS \
>> -   ((GEN8_HIZ_NP_PMA_FIX_ENABLE | GEN8_HIZ_NP_EARLY_Z_FAILS_DISABLE) << 16)
>> +   REG_MASK(GEN8_HIZ_NP_PMA_FIX_ENABLE | GEN8_HIZ_NP_EARLY_Z_FAILS_DISABLE)

...it wouldn't work out that nicely here (or whenever the masked field
is an integer rather than a bitfield).  We could:
 - Keep REG_MASK() around and call the macro you proposed REG_MASKED_B()
   (B for bool in anticipation of an I variant.  I'm open to better
   name suggestions).
 - Use REG_MASKED_B(..., false) in this case.
 - Keep it open-coded.

I'd be okay with either possibility.

>>  
>>  /* Predicate registers */
>>  #define MI_PREDICATE_SRC0               0x2400
>> -- 
>> 2.6.2
>> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151207/2dd5860d/attachment.sig>