subject:"\[Mesa\-dev\] \[PATCH 15\/21\] intel\/blorp\: Use isl_surf_get_uncompressed

Re: [Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

2018-02-28 Thread Pohjolainen, Topi

On Wed, Feb 28, 2018 at 10:11:56AM -0800, Jason Ekstrand wrote:
> On February 28, 2018 09:23:59 "Pohjolainen, Topi"
>  wrote:
> 
> >On Thu, Feb 22, 2018 at 11:06:55PM -0800, Jason Ekstrand wrote:
> >>---
> >> src/intel/blorp/blorp_blit.c | 58 
> >> +---
> >> 1 file changed, 17 insertions(+), 41 deletions(-)
> >>
> >>diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
> >>index 876498d..5b63754 100644
> >>--- a/src/intel/blorp/blorp_blit.c
> >>+++ b/src/intel/blorp/blorp_blit.c
> >>@@ -2349,50 +2349,26 @@ blorp_surf_convert_to_uncompressed(const
> >>struct isl_device *isl_dev,
> >>
> >>assert(fmtl->bw > 1 || fmtl->bh > 1);
> >>
> >>-   /* This is a compressed surface.  We need to convert it to a single
> >>-* slice (because compressed layouts don't perfectly match uncompressed
> >>-* ones with the same bpb) and divide x, y, width, and height by the
> >>-* block size.
> >>-*/
> >>-   blorp_surf_convert_to_single_slice(isl_dev, info);
> >>-
> >>-   if (width && height) {
> >>-#ifndef NDEBUG
> >>-  uint32_t right_edge_px = info->tile_x_sa + *x + *width;
> >>-  uint32_t bottom_edge_px = info->tile_y_sa + *y + *height;
> >>-  assert(*width % fmtl->bw == 0 ||
> >>- right_edge_px == info->surf.logical_level0_px.width);
> >>-  assert(*height % fmtl->bh == 0 ||
> >>- bottom_edge_px == info->surf.logical_level0_px.height);
> >>-#endif
> >>-  *width = DIV_ROUND_UP(*width, fmtl->bw);
> >>-  *height = DIV_ROUND_UP(*height, fmtl->bh);
> >>-   }
> >>-
> >>-   if (x && y) {
> >>-  assert(*x % fmtl->bw == 0);
> >>-  assert(*y % fmtl->bh == 0);
> >>-  *x /= fmtl->bw;
> >>-  *y /= fmtl->bh;
> >>-   }
> >>-
> >>-   info->surf.logical_level0_px.width =
> >>-  DIV_ROUND_UP(info->surf.logical_level0_px.width, fmtl->bw);
> >>-   info->surf.logical_level0_px.height =
> >>-  DIV_ROUND_UP(info->surf.logical_level0_px.height, fmtl->bh);
> >>+   /* It's now an uncompressed surface so we need an uncompressed format */
> >>+   info->view.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
> >>
> >>-   assert(info->surf.phys_level0_sa.width % fmtl->bw == 0);
> >>-   assert(info->surf.phys_level0_sa.height % fmtl->bh == 0);
> >>-   info->surf.phys_level0_sa.width /= fmtl->bw;
> >>-   info->surf.phys_level0_sa.height /= fmtl->bh;
> >>+   /* We only one one level and slice */
> >>+   info->view.levels = 1;
> >>+   info->view.array_len = 1;
> >>
> >>-   assert(info->tile_x_sa % fmtl->bw == 0);
> >>-   assert(info->tile_y_sa % fmtl->bh == 0);
> >>-   info->tile_x_sa /= fmtl->bw;
> >>-   info->tile_y_sa /= fmtl->bh;
> >>+   uint32_t offset_B;
> >>+   isl_surf_get_uncompressed_surf(isl_dev, >surf, >view,
> >>+  >surf, >view, _B,
> >>+  >tile_x_sa, >tile_y_sa);
> >
> >Here we pass info->view in separate arguments for reading and writing. Having
> >just read the implementation I know that all the reading takes place before
> >any writing. But somebody might later on change
> >isl_surf_get_uncompressed_surf() in way that breaks here.
> 
> The documentation for isl_surf_get_uncompressed_surf explicitly says
> that this is a supported usage model.  My expectation was that we
> would continue to provide that guarantee.
> 
> >I was happy when you took away one or two such constructs earlier in the
> >series.
> 
> Are you saying that you would rather BLORP explicitly make a copy?
> We could do that if you wanted.

I missed the documentation, I guess that is fine. This patch is also:

Reviewed-by: Topi Pohjolainen 

I think I reviewed all others except number 17. I don't think there is
anything wrong with it, I just don't know the context well enough.

> 
> --Jason
> 
> >>+   info->addr.offset += offset_B;
> >>
> >>-   /* It's now an uncompressed surface so we need an uncompressed format */
> >>-   info->surf.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
> >>+   /* BLORP doesn't use the actual intratile offsets.  Instead, it needs 
> >>the
> >>+* surface to be a bit bigger and we offset the vertices instead.
> >>+*/
> >>+   info->surf.logical_level0_px.w += info->tile_x_sa;
> >>+   info->surf.logical_level0_px.h += info->tile_y_sa;
> >>+   info->surf.phys_level0_sa.w += info->tile_x_sa;
> >>+   info->surf.phys_level0_sa.h += info->tile_y_sa;
> >> }
> >>
> >> void
> >>--
> >>2.5.0.400.gff86faf
> >>
> >>___
> >>mesa-dev mailing list
> >>mesa-dev@lists.freedesktop.org
> >>https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

2018-02-28 Thread Jason Ekstrand

On February 28, 2018 09:23:59 "Pohjolainen, Topi" 
 wrote:



On Thu, Feb 22, 2018 at 11:06:55PM -0800, Jason Ekstrand wrote:

---
 src/intel/blorp/blorp_blit.c | 58 +---
 1 file changed, 17 insertions(+), 41 deletions(-)

diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
index 876498d..5b63754 100644
--- a/src/intel/blorp/blorp_blit.c
+++ b/src/intel/blorp/blorp_blit.c
@@ -2349,50 +2349,26 @@ blorp_surf_convert_to_uncompressed(const struct 
isl_device *isl_dev,


assert(fmtl->bw > 1 || fmtl->bh > 1);

-   /* This is a compressed surface.  We need to convert it to a single
-* slice (because compressed layouts don't perfectly match uncompressed
-* ones with the same bpb) and divide x, y, width, and height by the
-* block size.
-*/
-   blorp_surf_convert_to_single_slice(isl_dev, info);
-
-   if (width && height) {
-#ifndef NDEBUG
-  uint32_t right_edge_px = info->tile_x_sa + *x + *width;
-  uint32_t bottom_edge_px = info->tile_y_sa + *y + *height;
-  assert(*width % fmtl->bw == 0 ||
- right_edge_px == info->surf.logical_level0_px.width);
-  assert(*height % fmtl->bh == 0 ||
- bottom_edge_px == info->surf.logical_level0_px.height);
-#endif
-  *width = DIV_ROUND_UP(*width, fmtl->bw);
-  *height = DIV_ROUND_UP(*height, fmtl->bh);
-   }
-
-   if (x && y) {
-  assert(*x % fmtl->bw == 0);
-  assert(*y % fmtl->bh == 0);
-  *x /= fmtl->bw;
-  *y /= fmtl->bh;
-   }
-
-   info->surf.logical_level0_px.width =
-  DIV_ROUND_UP(info->surf.logical_level0_px.width, fmtl->bw);
-   info->surf.logical_level0_px.height =
-  DIV_ROUND_UP(info->surf.logical_level0_px.height, fmtl->bh);
+   /* It's now an uncompressed surface so we need an uncompressed format */
+   info->view.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);

-   assert(info->surf.phys_level0_sa.width % fmtl->bw == 0);
-   assert(info->surf.phys_level0_sa.height % fmtl->bh == 0);
-   info->surf.phys_level0_sa.width /= fmtl->bw;
-   info->surf.phys_level0_sa.height /= fmtl->bh;
+   /* We only one one level and slice */
+   info->view.levels = 1;
+   info->view.array_len = 1;

-   assert(info->tile_x_sa % fmtl->bw == 0);
-   assert(info->tile_y_sa % fmtl->bh == 0);
-   info->tile_x_sa /= fmtl->bw;
-   info->tile_y_sa /= fmtl->bh;
+   uint32_t offset_B;
+   isl_surf_get_uncompressed_surf(isl_dev, >surf, >view,
+  >surf, >view, _B,
+  >tile_x_sa, >tile_y_sa);


Here we pass info->view in separate arguments for reading and writing. Having
just read the implementation I know that all the reading takes place before
any writing. But somebody might later on change
isl_surf_get_uncompressed_surf() in way that breaks here.


The documentation for isl_surf_get_uncompressed_surf explicitly says that 
this is a supported usage model.  My expectation was that we would continue 
to provide that guarantee.



I was happy when you took away one or two such constructs earlier in the
series.


Are you saying that you would rather BLORP explicitly make a copy?  We 
could do that if you wanted.


--Jason


+   info->addr.offset += offset_B;

-   /* It's now an uncompressed surface so we need an uncompressed format */
-   info->surf.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
+   /* BLORP doesn't use the actual intratile offsets.  Instead, it needs the
+* surface to be a bit bigger and we offset the vertices instead.
+*/
+   info->surf.logical_level0_px.w += info->tile_x_sa;
+   info->surf.logical_level0_px.h += info->tile_y_sa;
+   info->surf.phys_level0_sa.w += info->tile_x_sa;
+   info->surf.phys_level0_sa.h += info->tile_y_sa;
 }

 void
--
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

2018-02-28 Thread Pohjolainen, Topi

On Thu, Feb 22, 2018 at 11:06:55PM -0800, Jason Ekstrand wrote:
> ---
>  src/intel/blorp/blorp_blit.c | 58 
> +---
>  1 file changed, 17 insertions(+), 41 deletions(-)
> 
> diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
> index 876498d..5b63754 100644
> --- a/src/intel/blorp/blorp_blit.c
> +++ b/src/intel/blorp/blorp_blit.c
> @@ -2349,50 +2349,26 @@ blorp_surf_convert_to_uncompressed(const struct 
> isl_device *isl_dev,
>  
> assert(fmtl->bw > 1 || fmtl->bh > 1);
>  
> -   /* This is a compressed surface.  We need to convert it to a single
> -* slice (because compressed layouts don't perfectly match uncompressed
> -* ones with the same bpb) and divide x, y, width, and height by the
> -* block size.
> -*/
> -   blorp_surf_convert_to_single_slice(isl_dev, info);
> -
> -   if (width && height) {
> -#ifndef NDEBUG
> -  uint32_t right_edge_px = info->tile_x_sa + *x + *width;
> -  uint32_t bottom_edge_px = info->tile_y_sa + *y + *height;
> -  assert(*width % fmtl->bw == 0 ||
> - right_edge_px == info->surf.logical_level0_px.width);
> -  assert(*height % fmtl->bh == 0 ||
> - bottom_edge_px == info->surf.logical_level0_px.height);
> -#endif
> -  *width = DIV_ROUND_UP(*width, fmtl->bw);
> -  *height = DIV_ROUND_UP(*height, fmtl->bh);
> -   }
> -
> -   if (x && y) {
> -  assert(*x % fmtl->bw == 0);
> -  assert(*y % fmtl->bh == 0);
> -  *x /= fmtl->bw;
> -  *y /= fmtl->bh;
> -   }
> -
> -   info->surf.logical_level0_px.width =
> -  DIV_ROUND_UP(info->surf.logical_level0_px.width, fmtl->bw);
> -   info->surf.logical_level0_px.height =
> -  DIV_ROUND_UP(info->surf.logical_level0_px.height, fmtl->bh);
> +   /* It's now an uncompressed surface so we need an uncompressed format */
> +   info->view.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
>  
> -   assert(info->surf.phys_level0_sa.width % fmtl->bw == 0);
> -   assert(info->surf.phys_level0_sa.height % fmtl->bh == 0);
> -   info->surf.phys_level0_sa.width /= fmtl->bw;
> -   info->surf.phys_level0_sa.height /= fmtl->bh;
> +   /* We only one one level and slice */
> +   info->view.levels = 1;
> +   info->view.array_len = 1;
>  
> -   assert(info->tile_x_sa % fmtl->bw == 0);
> -   assert(info->tile_y_sa % fmtl->bh == 0);
> -   info->tile_x_sa /= fmtl->bw;
> -   info->tile_y_sa /= fmtl->bh;
> +   uint32_t offset_B;
> +   isl_surf_get_uncompressed_surf(isl_dev, >surf, >view,
> +  >surf, >view, _B,
> +  >tile_x_sa, >tile_y_sa);

Here we pass info->view in separate arguments for reading and writing. Having
just read the implementation I know that all the reading takes place before
any writing. But somebody might later on change
isl_surf_get_uncompressed_surf() in way that breaks here.

I was happy when you took away one or two such constructs earlier in the
series.

> +   info->addr.offset += offset_B;
>  
> -   /* It's now an uncompressed surface so we need an uncompressed format */
> -   info->surf.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
> +   /* BLORP doesn't use the actual intratile offsets.  Instead, it needs the
> +* surface to be a bit bigger and we offset the vertices instead.
> +*/
> +   info->surf.logical_level0_px.w += info->tile_x_sa;
> +   info->surf.logical_level0_px.h += info->tile_y_sa;
> +   info->surf.phys_level0_sa.w += info->tile_x_sa;
> +   info->surf.phys_level0_sa.h += info->tile_y_sa;
>  }
>  
>  void
> -- 
> 2.5.0.400.gff86faf
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

2018-02-22 Thread Jason Ekstrand

---
 src/intel/blorp/blorp_blit.c | 58 +---
 1 file changed, 17 insertions(+), 41 deletions(-)

diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
index 876498d..5b63754 100644
--- a/src/intel/blorp/blorp_blit.c
+++ b/src/intel/blorp/blorp_blit.c
@@ -2349,50 +2349,26 @@ blorp_surf_convert_to_uncompressed(const struct 
isl_device *isl_dev,
 
assert(fmtl->bw > 1 || fmtl->bh > 1);
 
-   /* This is a compressed surface.  We need to convert it to a single
-* slice (because compressed layouts don't perfectly match uncompressed
-* ones with the same bpb) and divide x, y, width, and height by the
-* block size.
-*/
-   blorp_surf_convert_to_single_slice(isl_dev, info);
-
-   if (width && height) {
-#ifndef NDEBUG
-  uint32_t right_edge_px = info->tile_x_sa + *x + *width;
-  uint32_t bottom_edge_px = info->tile_y_sa + *y + *height;
-  assert(*width % fmtl->bw == 0 ||
- right_edge_px == info->surf.logical_level0_px.width);
-  assert(*height % fmtl->bh == 0 ||
- bottom_edge_px == info->surf.logical_level0_px.height);
-#endif
-  *width = DIV_ROUND_UP(*width, fmtl->bw);
-  *height = DIV_ROUND_UP(*height, fmtl->bh);
-   }
-
-   if (x && y) {
-  assert(*x % fmtl->bw == 0);
-  assert(*y % fmtl->bh == 0);
-  *x /= fmtl->bw;
-  *y /= fmtl->bh;
-   }
-
-   info->surf.logical_level0_px.width =
-  DIV_ROUND_UP(info->surf.logical_level0_px.width, fmtl->bw);
-   info->surf.logical_level0_px.height =
-  DIV_ROUND_UP(info->surf.logical_level0_px.height, fmtl->bh);
+   /* It's now an uncompressed surface so we need an uncompressed format */
+   info->view.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
 
-   assert(info->surf.phys_level0_sa.width % fmtl->bw == 0);
-   assert(info->surf.phys_level0_sa.height % fmtl->bh == 0);
-   info->surf.phys_level0_sa.width /= fmtl->bw;
-   info->surf.phys_level0_sa.height /= fmtl->bh;
+   /* We only one one level and slice */
+   info->view.levels = 1;
+   info->view.array_len = 1;
 
-   assert(info->tile_x_sa % fmtl->bw == 0);
-   assert(info->tile_y_sa % fmtl->bh == 0);
-   info->tile_x_sa /= fmtl->bw;
-   info->tile_y_sa /= fmtl->bh;
+   uint32_t offset_B;
+   isl_surf_get_uncompressed_surf(isl_dev, >surf, >view,
+  >surf, >view, _B,
+  >tile_x_sa, >tile_y_sa);
+   info->addr.offset += offset_B;
 
-   /* It's now an uncompressed surface so we need an uncompressed format */
-   info->surf.format = get_copy_format_for_bpb(isl_dev, fmtl->bpb);
+   /* BLORP doesn't use the actual intratile offsets.  Instead, it needs the
+* surface to be a bit bigger and we offset the vertices instead.
+*/
+   info->surf.logical_level0_px.w += info->tile_x_sa;
+   info->surf.logical_level0_px.h += info->tile_y_sa;
+   info->surf.phys_level0_sa.w += info->tile_x_sa;
+   info->surf.phys_level0_sa.h += info->tile_y_sa;
 }
 
 void
-- 
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

Re: [Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

Re: [Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

[Mesa-dev] [PATCH 15/21] intel/blorp: Use isl_surf_get_uncompressed_surf

4 matches

Site Navigation

Mail list logo

Footer information