[Mesa-dev] [PATCH 1/7] blorp: Handle the RGB workaround more like other workarounds
Pohjolainen, Topi
topi.pohjolainen at gmail.com
Thu Jan 26 07:04:06 UTC 2017
On Wed, Jan 25, 2017 at 10:51:53PM -0800, Jason Ekstrand wrote:
> On Wed, Jan 25, 2017 at 10:46 PM, Pohjolainen, Topi
> <[1]topi.pohjolainen at gmail.com> wrote:
>
> 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));
>
> 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?
>
> Yes, that's correct.
>
> > + }
> > +
> > /* 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, ¶ms->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;
>
> 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?
>
> Yes, that's correct.
>
> assert(!wm_prog_key->use_kill);
>
> i don't really see why it's needed but I'm happy to add it.
Blorp blits are sort of cascaded tool box where all the tools are not
compatible. I have tried to assert against the key as much as possible in
order to ease thinking which parts are applicable and which are not.
Anyway:
Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>
> > +
> > + surf_fake_rgb_with_red(batch->blorp->isl_dev,
> ¶ms->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(¶ms->src,
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > [2]mesa-dev at lists.freedesktop.org
> > [3]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> References
>
> 1. mailto:topi.pohjolainen at gmail.com
> 2. mailto:mesa-dev at lists.freedesktop.org
> 3. https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list