On Mon, Dec 7, 2015 at 4:24 AM, Francisco Jerez <curroje...@riseup.net> wrote:
> Kristian Høgsberg <k...@bitplanet.net> writes:
>
>> On Tue, Dec 01, 2015 at 12:19:28AM -0800, Jordan Justen wrote:
>>> From: Francisco Jerez <curroje...@riseup.net>
>>>
>>> Reviewed-by: Samuel Iglesias Gonsálvez <sigles...@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.
Ah... I think I prefer either just keeping what you have in your
original patch or rewriting the pma stall workaround code to use the
REG_MASK macro in gen8_emit_pma_stall_workaround():
const bool enable = pma_fix_enable(brw);
const uint32_t bits = REG_MASK(GEN8_HIZ_NP_PMA_FIX_ENABLE, enable) |
REG_MASK(GEN8_HIZ_NP_EARLY_Z_FAILS_DISABLE, enable);
write_pma_stall_bits() will be storing the mask bits in
brw->pma_stall_bits as well, which is not entirely intuitive, but it
should work fine.
Kristian
>>>
>>> /* Predicate registers */
>>> #define MI_PREDICATE_SRC0 0x2400
>>> --
>>> 2.6.2
>>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
