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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
