On 10/05/2013 03:38 AM, Chris Forbes wrote: > - For HSW: Select the channel based on the component selected (swizzle > is done in HW) > - For IVB: Select the channel based on the swizzle state for the > component selected. Only apply the RG32F w/a if we actually want > green -- we're about to flag it regardless of swizzle state. > > Signed-off-by: Chris Forbes <chr...@ijw.co.nz> > --- > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > index 6141009..5508cdc 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > @@ -1475,7 +1475,8 @@ fs_visitor::visit(ir_texture *ir) > /* When tg4 is used with the degenerate ZERO/ONE swizzles, don't bother > * emitting anything other than setting up the constant result. > */ > - int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0); > + ir_constant *chan = ir->lod_info.component->as_constant(); > + int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0]);
Cool! On my first pass through the spec I hadn't realized that the channel parameter was required to be a constant integer expression, and you indeed enforced that by using ir_var_const_in. Makes life here so much easier. There's only one problem: right now, nothing enforces the constraint that the component parameter must be 0, 1, 2, or 3. Although it isn't explicitly stated in the spec, I believe specifying another value should be a compile error. I think this has to get handled at the ast_to_hir step, and can probably be an ugly special case for now. It looks like currently, the bitshifts in GET_SWZ will probably return 0, resulting in the red channel being selected. So there isn't an out of bounds access problem...just undefined results. A compile error is definitely more friendly, though. > if (swiz == SWIZZLE_ZERO || swiz == SWIZZLE_ONE) { > > fs_reg res = fs_reg(this, glsl_type::vec4_type); > @@ -1594,14 +1595,16 @@ fs_visitor::visit(ir_texture *ir) > uint32_t > fs_visitor::gather_channel(ir_texture *ir, int sampler) > { > - int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0 /* red */); > - if (c->key.tex.gather_channel_quirk_mask & (1<<sampler)) > - return 2; /* gather4 sampler is broken for green channel on RG32F -- > - * we must ask for blue instead. > - */ > + ir_constant *chan = ir->lod_info.component->as_constant(); > + int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0] /* red > */); The /* red */ comment here doesn't make sense anymore...I'd drop it. > switch (swiz) { > case SWIZZLE_X: return 0; > - case SWIZZLE_Y: return 1; > + case SWIZZLE_Y: > + if (c->key.tex.gather_channel_quirk_mask & (1<<sampler)) > + return 2; /* gather4 sampler is broken for green channel on > RG32F -- > + * we must ask for blue instead. > + */ > + return 1; I would like to see the comment above the block, rather than off to the side like this. Ditto for the next patch. > case SWIZZLE_Z: return 2; > case SWIZZLE_W: return 3; > default: > Patches 3-4 are: Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> (since my comment about bounds checking 'comp' would go in a different patch anyway) _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev