[Mesa-dev] [PATCH 5/5] i965: Maximum allowed size of SEND messages is 15 (4 bits)

[Iago Toral]

On Fri, 2015-09-18 at 09:09 -0700, Kenneth Graunke wrote:
> On Friday, September 18, 2015 10:08:52 AM Iago Toral Quiroga wrote:
> > Until now we only used MRFs 1..15 for regular SEND messages, so the
> > message length could not possibly exceed the maximum size. Now that
> > we allow to use MRF registers 1..23 in gen6, we need to be careful
> > not to build messages that can go beyond the limit. That could occur,
> > specifically, when building URB write messages, which we may need to
> > split in chunks due to their size. Previously we would simply go and
> > create a new message when we reached MRF 13 (since 13..15 were
> > reserved for spilling), now we also want to check the size of the
> > message explicitly.
> > 
> > Besides adding that condition to split URB write messages properly,
> > this patch also adds asserts in the generator. Notice that
> > brw_inst_set_mlen already asserts for this, but asserting in the
> > generators is easy and can make debugging easier in some cases.
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   | 1 +
> >  src/mesa/drivers/dri/i965/brw_inst.h             | 3 +++
> >  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 1 +
> >  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp   | 5 +++--
> >  4 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > index 6cbd42f..c65084d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > @@ -1561,6 +1561,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
> >        brw_set_default_exec_size(p, cvt(inst->exec_size) - 1);
> >  
> >        assert(inst->base_mrf + inst->mlen <= BRW_MAX_MRF(devinfo->gen));
> > +      assert(inst->mlen <= BRW_MAX_MSG_LENGTH);
> >  
> >        switch (inst->exec_size) {
> >        case 1:
> > diff --git a/src/mesa/drivers/dri/i965/brw_inst.h b/src/mesa/drivers/dri/i965/brw_inst.h
> > index 46eff1d..c5132ba 100644
> > --- a/src/mesa/drivers/dri/i965/brw_inst.h
> > +++ b/src/mesa/drivers/dri/i965/brw_inst.h
> > @@ -39,6 +39,9 @@
> >  extern "C" {
> >  #endif
> >  
> > +/** Maximum SEND message length */
> > +#define BRW_MAX_MSG_LENGTH 15
> > +
> >  /* brw_context.h has a forward declaration of brw_inst, so name the struct. */
> >  typedef struct brw_inst {
> >     uint64_t data[2];
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > index d5943d2..05f2044 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > @@ -1136,6 +1136,7 @@ vec4_generator::generate_code(const cfg_t *cfg)
> >        brw_set_default_acc_write_control(p, inst->writes_accumulator);
> >  
> >        assert(inst->base_mrf + inst->mlen <= BRW_MAX_MRF(devinfo->gen));
> > +      assert(inst->mlen <= BRW_MAX_MSG_LENGTH);
> >  
> >        unsigned pre_emit_nr_insn = p->nr_insn;
> >  
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > index 7f06050..514ccd6 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > @@ -3320,9 +3320,10 @@ vec4_visitor::emit_vertex()
> >                         prog_data->vue_map.slot_to_varying[slot]);
> >  
> >           /* If this was max_usable_mrf, we can't fit anything more into this
> > -          * URB WRITE.
> > +          * URB WRITE. Same thing if we reached the maximum length available.
> >            */
> > -         if (mrf > max_usable_mrf) {
> > +         if (mrf > max_usable_mrf ||
> > +             align_interleaved_urb_mlen(devinfo, mrf - base_mrf + 1) > BRW_MAX_MSG_LENGTH) {
> >              slot++;
> >              break;
> >           }
> > 
> 
> Doesn't this hunk need to go before patch 2?  It seems like otherwise
> we'll be emitting URB write messages that are too long until patch 5.

Yeah, you're right. I'll fix that before pushing.

> With that fixed, the series is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> Thanks for doing this, Iago!

Thanks for reviewing it! ;)