[Intel-gfx] [PATCH] drm/i915: Update WaFlushCoherentL3CacheLinesAtContextSwitch

[Dave Gordon]

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 at chris-wilson.co.uk>
>>>> Cc: Dave Gordon <david.s.gordon at intel.com>
>>>> Signed-off-by: Arun Siluvery <arun.siluvery at 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 at 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?

.Dave.