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