[Mesa-dev] [PATCH 04/12] i965/fs/skl+: Use lcd2dms_w instead of lcd2dms

[Ben Widawsky]

On Wed, Sep 23, 2015 at 06:28:42PM +0100, Neil Roberts wrote:
> Ben Widawsky <ben at bwidawsk.net> writes:
> 
> >> +         /* On Gen9+ we'll use lcd2ms_w instead which has two registers for
> >> +          * the MCS data.
> >> +          */
> >> +         if (op == SHADER_OPCODE_TXF_CMS_W) {
> >> +            bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_UD),
> >> +                    mcs.file == IMM ?
> >> +                    mcs :
> >> +                    offset(mcs, bld, 1));
> >> +            length++;
> >> +         }
> >
> > This hunk is very confusing to me without more context from later patches. I
> > assume the upper bits of mcs are 0 for the cases where you're doing < 16x, but
> > there is no way to determine that.
> 
> Well, the documentation says that the upper bits of MCS are ignored when
> the sample count is < 16, so it doesn't really matter if it's zeroed or
> not. The MCS data is retrieved from ld_mcs which already returns 4 or 8
> registers of data even before SKL. I guess I could add that to the
> commit message.
> 

Just to let everyone know what we discussed in PM. It /looks/ like the bits must
be 0, but that the upper bits should have been 0'd by the hardware - so its
safe. If you want to modify the commit, it would probably be good since the docs
are not as clear as I'd like.

> > I am not a big fan of the, lay out all the infrastructure, then use it
> > later, when it comes to patch review. Or am I missing something?
> 
> Hrm, I think for other patches I've had the opposite complaint so I
> don't know what is best.
> 

Yeah. I can't quite tell if I am in the minority on this. Certainly the vocal
majority is to cut things up into tiny little pieces. While this makes the
individual change easier to review, it makes it harder to review the entire
series. Sorry to give you a conflicting message - when it doubt, do the opposite
of what I say.

> > (Also, I think a separate case for SHADER_OPCODE_TXF_CMS_W would
> > probably look a bit nicer, but that's just a random thought).
> 
> Yes, you're probably right. I guess this patch is going to conflict with
> your fast clear branch anyway so one of us will have to split it out at
> that point :)

You can now add my:
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

(although I still wouldn't mind some squashing first :-) )
> 
> Regards,
> - Neil