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
> >>@@ -131,7 +131,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
> >> .mask = MI_GLOBAL_GTT,
> >> .expected = 0,
> >> }}, ),
> >>- CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, W | B,
> >>+ CMD( MI_LOAD_REGISTER_MEM(1), SMI, !F, 0xFF, W | B,
> >> .reg = { .offset = 1, .mask = 0x007FFFFC },
> >> .bits = {{
> >> .offset = 0,
> >>@@ -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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
