<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 25, 2017 at 10:46 PM, Pohjolainen, Topi <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Jan 24, 2017 at 03:45:48PM -0800, Jason Ekstrand wrote: > The previous version was sort-of strapped on in that it just adjusted > the blit rectangle and trusted in the fact that we would use texelFetch > and round to the nearest integer to ensure that the component positions > matched. This new version, while slightly more complicated, is more > accurate because all three components end up with exactly the same > dst_pos and so they will get interpolated and sampled at the same > texture coordinate. This makes the workaround suitable for using with > scaled blits. > --- > src/intel/blorp/blorp_blit.c | 60 ++++++++++++++++++++++---------------------- > 1 file changed, 30 insertions(+), 30 deletions(-) > > diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c > index 111f1c1..fc76fd4 100644 > --- a/src/intel/blorp/blorp_blit.c > +++ b/src/intel/blorp/blorp_blit.c > @@ -1138,6 +1138,20 @@ brw_blorp_build_nir_shader(struct blorp_context *blorp, void *mem_ctx, > key->dst_layout); > } > > + nir_ssa_def *comp = NULL; > + if (key->dst_rgb) { > + /* The destination image is bound as a red texture three times as wide > + * as the actual image. Our shader is effectively running one color > + * component at a time. We need to save off the component and adjust > + * the destination position. > + */ > + assert(dst_pos->num_components == 2); > + nir_ssa_def *dst_x = nir_channel(&b, dst_pos, 0); > + comp = nir_umod(&b, dst_x, nir_imm_int(&b, 3)); > + dst_pos = nir_vec2(&b, nir_idiv(&b, dst_x, nir_imm_int(&b, 3)), > + nir_channel(&b, dst_pos, 1)); </div></div>Just to check that I understood correctly, before we had this scale-factor of three in v_coord_transform. Now we have it here explicitly? </blockquote><div> </div><div>Yes, that's correct. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + } > + > /* Now (X, Y, S) = decode_msaa(dst_samples, detile(dst_tiling, offset)). > * > * That is: X, Y and S now contain the true coordinates and sample index of > @@ -1267,8 +1281,6 @@ brw_blorp_build_nir_shader(struct blorp_context *blorp, void *mem_ctx, > * from the source color and write that to destination red. > */ > assert(dst_pos->num_components == 2); > - nir_ssa_def *comp = > - nir_umod(&b, nir_channel(&b, dst_pos, 0), nir_imm_int(&b, 3)); > > nir_ssa_def *color_component = > nir_bcsel(&b, nir_ieq(&b, comp, nir_imm_int(&b, 0)), > @@ -1543,15 +1555,12 @@ struct blt_coords { > > static void > surf_fake_rgb_with_red(const struct isl_device *isl_dev, > - struct brw_blorp_surface_info *info, > - uint32_t *x, uint32_t *width) > + struct brw_blorp_surface_info *info) > { > surf_convert_to_single_slice(isl_dev, info); > > info->surf.logical_level0_px.width *= 3; > info->surf.phys_level0_sa.width *= 3; > - *x *= 3; > - *width *= 3; > > enum isl_format red_format; > switch (info->view.format) { > @@ -1581,28 +1590,6 @@ surf_fake_rgb_with_red(const struct isl_device *isl_dev, > info->surf.format = info->view.format = red_format; > } > > -static void > -fake_dest_rgb_with_red(const struct isl_device *dev, > - struct blorp_params *params, > - struct brw_blorp_blit_prog_key *wm_prog_key, > - struct blt_coords *coords) > -{ > - /* Handle RGB destinations for blorp_copy */ > - const struct isl_format_layout *dst_fmtl = > - isl_format_get_layout(params->dst.surf.format); > - > - if (dst_fmtl->bpb % 3 == 0) { > - uint32_t dst_x = coords->x.dst0; > - uint32_t dst_width = coords->x.dst1 - dst_x; > - surf_fake_rgb_with_red(dev, &params->dst, > - &dst_x, &dst_width); > - coords->x.dst0 = dst_x; > - coords->x.dst1 = dst_x + dst_width; > - wm_prog_key->dst_rgb = true; > - wm_prog_key->need_dst_offset = true; > - } > -} > - > enum blit_shrink_status { > BLIT_NO_SHRINK = 0, > BLIT_WIDTH_SHRINK = 1, > @@ -1621,8 +1608,6 @@ try_blorp_blit(struct blorp_batch *batch, > { > const struct gen_device_info *devinfo = batch->blorp->isl_dev->info; > > - fake_dest_rgb_with_red(batch->blorp->isl_dev, params, wm_prog_key, coords); > - > if (isl_format_has_sint_channel(params->src.view.format)) { > wm_prog_key->texture_data_type = nir_type_int; > } else if (isl_format_has_uint_channel(params->src.view.format)) { > @@ -1799,6 +1784,21 @@ try_blorp_blit(struct blorp_batch *batch, > > params->num_samples = params->dst.surf.samples; > > + if (isl_format_get_layout(params->dst.view.format)->bpb % 3 == 0) { > + /* We can't render to RGB formats natively because they aren't a > + * power-of-two size. Instead, we fake them by using a red format > + * with the same channel type and size and emitting shader code to > + * only write one channel at a time. > + */ > + params->x0 *= 3; > + params->x1 *= 3; </div></div>It looks to me that we stop adjusting v_discard_rect as well. Before it was calculated against adjusted coords. Now we only adjust the vertices. This means blorp_nir_discard_if_outside_rect() isn't used for the rgb-trick - otherwise it would clip too much in the right hand side. Or am I missing something? If not, can we add? </blockquote><div> </div><div>Yes, that's correct. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> assert(!wm_prog_key->use_kill); </blockquote><div> </div><div>i don't really see why it's needed but I'm happy to add it. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + surf_fake_rgb_with_red(batch->blorp->isl_dev, &params->dst); > + > + wm_prog_key->dst_rgb = true; > + wm_prog_key->need_dst_offset = true; > + } > + > if (params->src.tile_x_sa || params->src.tile_y_sa) { > assert(wm_prog_key->need_src_offset); > surf_get_intratile_offset_px(&params->src, > -- > 2.5.0.400.gff86faf > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>