Re: [PATCH] drm/i915: Update WaFlushCoherentL3CacheLinesAtContextSwitch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxx>
Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
Signed-off-by: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxx>

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux