Re: [PATCH] drm/i915: Update WaFlushCoherentL3CacheLinesAtContextSwitch

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

On 06/07/15 16:41, Chris Wilson wrote:
> On Mon, Jul 06, 2015 at 04:33:05PM +0200, Daniel Vetter wrote:
>> On Mon, Jul 06, 2015 at 02:16:54PM +0100, Dave Gordon wrote:
>>> On 06/07/15 13:38, Daniel Vetter wrote:
>>>> On Mon, Jul 06, 2015 at 12:52:51PM +0100, Dave Gordon wrote:
>>>>> On 03/07/15 16:42, Chris Wilson wrote:
>>>>>> On Fri, Jul 03, 2015 at 02:27:31PM +0100, Arun Siluvery wrote:
>>>>>>> In this WA we need to set GEN8_L3SQCREG4[21:21] and reset it after PIPE_CONTROL
>>>>>>> instruction but there is a slight complication as this is applied in WA batch
>>>>>>> where the values are only initialized once.
>>>>>>> Dave identified an issue with the current implementation where the register value
>>>>>>> is read once at the beginning and it is reused; this patch corrects this by saving
>>>>>>> the register value to memory, update register with the bit of our interest and
>>>>>>> restore it back with original value.
>>>>>>>
>>>>>>> This implementation uses MI_LOAD_REGISTER_MEM which is currently only used
>>>>>>> by command parser and was using a default length of 0. This is now updated
>>>>>>> with correct length and moved to appropriate place.
>>>>>>>
>>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>>>>>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/i915_cmd_parser.c |  6 +--
>>>>>>>   drivers/gpu/drm/i915/i915_reg.h        |  3 +-
>>>>>>>   drivers/gpu/drm/i915/intel_lrc.c       | 72 +++++++++++++++++++++++++---------
>>>>>>>   3 files changed, 58 insertions(+), 23 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
>>>>>>> index 306d9e4..430571b 100644
>>>>>>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>>>>>>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>>>
>>>>>>> @@ -1021,7 +1021,7 @@ static bool check_cmd(const struct intel_engine_cs *ring,
>>>>>>>   			 * only MI_LOAD_REGISTER_IMM commands.
>>>>>>>   			 */
>>>>>>>   			if (reg_addr == OACONTROL) {
>>>>>>> -				if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
>>>>>>> +				if (desc->cmd.value == MI_LOAD_REGISTER_MEM(1)) {
>>>>>>
>>>>>> I had a double take here, but it all comes out in the wash. For one
>>>>>> moment, I thought the cmd matching had changed, but that has the length
>>>>>> masked out.
>>>>>>
>>>>>> Reviewed-by: Chris Wilson <chris@cris-wilson.co.uk>
>>>>
>>>> Queued for -next, thanks for the patch.
>>>>
>>>>>> Who will start to complain about all the extra frequent register writes,
>>>>>> probably into common power wells....
>>>>>> -Chris
>>>>>
>>>>> Hmm ... that is quite confusing, especially as the actual opcode in the
>>>>> instruction stream will be MI_LOAD_REGISTER_MEM(2) on GEN8+. It might almost
>>>>> be better to use MI_LOAD_REGISTER_MEM(0) to emphasise that the length field
>>>>> is a wildcard and not something that will be matched exactly.
>>>>
>>>> There's a separate _GEN8 #define to accomodate the differences, so I don't
>>>> fully understand your concern. We also don't do any decoding in the kernel
>>>> ...
>>>> -Daniel
>>>
>>> In the snippet:
>>>
>>>>> -	CMD(  MI_LOAD_REGISTER_MEM,         SMI,   !F,  0xFF,   W | B,
>>>>> +	CMD(  MI_LOAD_REGISTER_MEM(1),      SMI,   !F,  0xFF,   W | B,
>>>
>>> the (1) goes in the table but is ignored when matching instructions in the
>>> stream being parsed. It could just as well be (2) or (0) or (255).
>>>
>>> Then, in the test:
>>>
>>>>> -		if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
>>>>> +		if (desc->cmd.value == MI_LOAD_REGISTER_MEM(1)) {
>>>
>>> the thing on the left of the == is not the instruction being examined, but
>>> the entry in the table that matched that instruction. So here also we're not
>>> really using the length field, EXCEPT that it MUST be the same as the
>>> (arbitrary) value in the table.
>>>
>>> So my concern here was not about correctness but comprehensibility and hence
>>> maintainability -- after all, if Chris had to look twice it obviously isn't
>>> as clear as one would like!
>>>
>>> My suggestion was that maybe the "ignored" length field should be 0 to make
>>> it less likely that a reader would think this matches exactly (and only) an
>>> opcode of 0xa400001. Or maybe (255) would be even more obviously
>>> not-a-literal-match?
>>
>> Hm, given that the cmd parser is gen7 only I'm not too concerned about
>> this. It is indeed a bit surprising though, and I guess (0) would be less
>> surprising. Otoh other commands with a lenght field also use (1) in a
>> similar fashion, so at least this is consistent.
>>
>> tbh no opinion here at all from my side, but happy to merge a fixup on top
>> to clarify this, if you can agree on a clear improvement.
>
> iirc, the #define used CMD | (2*(x)-1), which blows up if passed in 0.
> -Chris

Yes; and furthermore the MI_LOAD_REGISTER_MEM instruction really doesn't 
take a list of (register,address) pairs; unlike 
MI_LOAD_REGISTER_IMMEDIATE it can only load ONE register from ONE memory 
address. So the (x) is completely bogus anyway, and it should revert to 
just:

#define MI_LOAD_REGISTER_MEM		MI_INSTR(0x29, 1)
#define MI_LOAD_REGISTER_MEM_GEN8	MI_INSTR(0x29, 2)

AFAICS LRI is the only CS register-manipulating opcode that actually 
supports a variable-length list?

.Dave.

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