[Mesa-dev] [PATCH 1/7] blorp: Handle the RGB workaround more like other workarounds

[Pohjolainen, Topi]

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?

> +   }
> +
>     /* 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;

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?

         assert(!wm_prog_key->use_kill);

> +
> +      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
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev