Re: [Mesa-dev] [PATCH v5] i965: Fix ETC2/EAC GetCompressed* functions on Gen7 GPUs
On Wed, Jul 18, 2018 at 05:34:13PM +0300, Eleni Maria Stea wrote: > On 07/10/2018 03:10 AM, Nanley Chery wrote: > > On Thu, Jun 14, 2018 at 10:50:57PM +0300, Eleni Maria Stea wrote: > >> On 06/14/2018 10:27 PM, Nanley Chery wrote: > >> > >>> +Jason, Ken > >>> > >>> Hello, > >>> > >>> I recently did some miptree work relating to the r8stencil_mt and I > >>> think I now have a more informed opinion about how things should be > >>> structured. I'd like to propose an alternative solution. > >>> > >>> I had initially thought we should have a separate miptree to hold the > >>> compressed data, like this patch does, but now I think we should > >>> actually have the compressed data be the main miptree and to store the > >>> decompressed miptree as part of the main one. The reasoning is that we > >>> could reuse this structure to handle the r8stencil workaround and to > >>> eventually handle the ASTC_LDR surfaces that are modified on gen9. > >>> > >>> I'm proposing something like the following: > >>> > >>> 1. Rename r8stencil_mt ->shadow_mt and > >>>r8stencil_needs_update -> shadow_needs_update. > >>> 2. Make shadow_mt hold the decompressed ETC miptree > >>> 3. Update shadow_needs_update whenever the main mt is modified > >>> 4. Add an function to update the shadow_mt using the main mt as a source > >>> 5. Sample from the shadow_mt as appropriate > >>> 6. Make the main miptree hold the compressed data > >>> > >>> This method should also be able to handle the CopyImage functions. What > >>> do you all think? > >>> > >>> -Nanley > >> > >> Hi Nanley, > >> > >> Thank you for your reply. I wasn't aware that there are other cases we > >> might need to store a 2nd image. I agree that it's more reasonable to > >> use one generic purpose miptree that can be accessible from different > >> parts of the i965 code for such cases instead of storing miptrees in > >> different places for different hacks when a feature is not supported. > >> > >> I will search your patch to get a look and I will also get a look at the > >> mesa code to see how easy this fix would be (which parts of the code it > >> might affect) and if everyone agrees that this is a good idea I will > >> modify this patch according to your suggestions. > >> > >> BR :) > >> Eleni > > > > Hi Eleni, > > > > I gave this more thought and am now thinking that what you have here is > > fine. Having two different ways of working with a shadow miptree > > suggests a refactor later on, but IMO this is ultimately a step in the > > right direction. Sorry for the noise. > > > > With code-sharing among shadow miptrees in mind, my two main > > suggestions are 1) to perform mapping operations only with the cmt (if > > it's present) and 2) to update the decompressed mt, on demand. Maybe > > with intel_miptree_copy_slice_sw? > > > > Regards, > > Nanley > > > > Hi Nanley, > > I talked to you on IRC but I reply here as well: > > Thank you for the suggestions, I had misunderstood something from our > IRC conversation that followed this e-mail, so the patch v6 has several > issues. I will send a new one soon and I will implement the solution you > suggested earlier (suggestions 1-6) instead. Sorry for the noise with > the patch v6. > Sounds good. By the way, I think it'd be helpful if you sent out the solution as a series of patches (see git format-patch - for example). That way it's easier to confirm each step of the solution is correct. -Nanley > Thanks, > Eleni > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5] i965: Fix ETC2/EAC GetCompressed* functions on Gen7 GPUs
On 07/10/2018 03:10 AM, Nanley Chery wrote: > On Thu, Jun 14, 2018 at 10:50:57PM +0300, Eleni Maria Stea wrote: >> On 06/14/2018 10:27 PM, Nanley Chery wrote: >> >>> +Jason, Ken >>> >>> Hello, >>> >>> I recently did some miptree work relating to the r8stencil_mt and I >>> think I now have a more informed opinion about how things should be >>> structured. I'd like to propose an alternative solution. >>> >>> I had initially thought we should have a separate miptree to hold the >>> compressed data, like this patch does, but now I think we should >>> actually have the compressed data be the main miptree and to store the >>> decompressed miptree as part of the main one. The reasoning is that we >>> could reuse this structure to handle the r8stencil workaround and to >>> eventually handle the ASTC_LDR surfaces that are modified on gen9. >>> >>> I'm proposing something like the following: >>> >>> 1. Rename r8stencil_mt ->shadow_mt and >>>r8stencil_needs_update -> shadow_needs_update. >>> 2. Make shadow_mt hold the decompressed ETC miptree >>> 3. Update shadow_needs_update whenever the main mt is modified >>> 4. Add an function to update the shadow_mt using the main mt as a source >>> 5. Sample from the shadow_mt as appropriate >>> 6. Make the main miptree hold the compressed data >>> >>> This method should also be able to handle the CopyImage functions. What >>> do you all think? >>> >>> -Nanley >> >> Hi Nanley, >> >> Thank you for your reply. I wasn't aware that there are other cases we >> might need to store a 2nd image. I agree that it's more reasonable to >> use one generic purpose miptree that can be accessible from different >> parts of the i965 code for such cases instead of storing miptrees in >> different places for different hacks when a feature is not supported. >> >> I will search your patch to get a look and I will also get a look at the >> mesa code to see how easy this fix would be (which parts of the code it >> might affect) and if everyone agrees that this is a good idea I will >> modify this patch according to your suggestions. >> >> BR :) >> Eleni > > Hi Eleni, > > I gave this more thought and am now thinking that what you have here is > fine. Having two different ways of working with a shadow miptree > suggests a refactor later on, but IMO this is ultimately a step in the > right direction. Sorry for the noise. > > With code-sharing among shadow miptrees in mind, my two main > suggestions are 1) to perform mapping operations only with the cmt (if > it's present) and 2) to update the decompressed mt, on demand. Maybe > with intel_miptree_copy_slice_sw? > > Regards, > Nanley > Hi Nanley, I talked to you on IRC but I reply here as well: Thank you for the suggestions, I had misunderstood something from our IRC conversation that followed this e-mail, so the patch v6 has several issues. I will send a new one soon and I will implement the solution you suggested earlier (suggestions 1-6) instead. Sorry for the noise with the patch v6. Thanks, Eleni ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5] i965: Fix ETC2/EAC GetCompressed* functions on Gen7 GPUs
On Thu, Jun 14, 2018 at 10:50:57PM +0300, Eleni Maria Stea wrote: > On 06/14/2018 10:27 PM, Nanley Chery wrote: > > > +Jason, Ken > > > > Hello, > > > > I recently did some miptree work relating to the r8stencil_mt and I > > think I now have a more informed opinion about how things should be > > structured. I'd like to propose an alternative solution. > > > > I had initially thought we should have a separate miptree to hold the > > compressed data, like this patch does, but now I think we should > > actually have the compressed data be the main miptree and to store the > > decompressed miptree as part of the main one. The reasoning is that we > > could reuse this structure to handle the r8stencil workaround and to > > eventually handle the ASTC_LDR surfaces that are modified on gen9. > > > > I'm proposing something like the following: > > > > 1. Rename r8stencil_mt ->shadow_mt and > >r8stencil_needs_update -> shadow_needs_update. > > 2. Make shadow_mt hold the decompressed ETC miptree > > 3. Update shadow_needs_update whenever the main mt is modified > > 4. Add an function to update the shadow_mt using the main mt as a source > > 5. Sample from the shadow_mt as appropriate > > 6. Make the main miptree hold the compressed data > > > > This method should also be able to handle the CopyImage functions. What > > do you all think? > > > > -Nanley > > Hi Nanley, > > Thank you for your reply. I wasn't aware that there are other cases we > might need to store a 2nd image. I agree that it's more reasonable to > use one generic purpose miptree that can be accessible from different > parts of the i965 code for such cases instead of storing miptrees in > different places for different hacks when a feature is not supported. > > I will search your patch to get a look and I will also get a look at the > mesa code to see how easy this fix would be (which parts of the code it > might affect) and if everyone agrees that this is a good idea I will > modify this patch according to your suggestions. > > BR :) > Eleni Hi Eleni, I gave this more thought and am now thinking that what you have here is fine. Having two different ways of working with a shadow miptree suggests a refactor later on, but IMO this is ultimately a step in the right direction. Sorry for the noise. With code-sharing among shadow miptrees in mind, my two main suggestions are 1) to perform mapping operations only with the cmt (if it's present) and 2) to update the decompressed mt, on demand. Maybe with intel_miptree_copy_slice_sw? Regards, Nanley ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5] i965: Fix ETC2/EAC GetCompressed* functions on Gen7 GPUs
On 06/14/2018 10:27 PM, Nanley Chery wrote: > +Jason, Ken > > Hello, > > I recently did some miptree work relating to the r8stencil_mt and I > think I now have a more informed opinion about how things should be > structured. I'd like to propose an alternative solution. > > I had initially thought we should have a separate miptree to hold the > compressed data, like this patch does, but now I think we should > actually have the compressed data be the main miptree and to store the > decompressed miptree as part of the main one. The reasoning is that we > could reuse this structure to handle the r8stencil workaround and to > eventually handle the ASTC_LDR surfaces that are modified on gen9. > > I'm proposing something like the following: > > 1. Rename r8stencil_mt ->shadow_mt and >r8stencil_needs_update -> shadow_needs_update. > 2. Make shadow_mt hold the decompressed ETC miptree > 3. Update shadow_needs_update whenever the main mt is modified > 4. Add an function to update the shadow_mt using the main mt as a source > 5. Sample from the shadow_mt as appropriate > 6. Make the main miptree hold the compressed data > > This method should also be able to handle the CopyImage functions. What > do you all think? > > -Nanley Hi Nanley, Thank you for your reply. I wasn't aware that there are other cases we might need to store a 2nd image. I agree that it's more reasonable to use one generic purpose miptree that can be accessible from different parts of the i965 code for such cases instead of storing miptrees in different places for different hacks when a feature is not supported. I will search your patch to get a look and I will also get a look at the mesa code to see how easy this fix would be (which parts of the code it might affect) and if everyone agrees that this is a good idea I will modify this patch according to your suggestions. BR :) Eleni ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5] i965: Fix ETC2/EAC GetCompressed* functions on Gen7 GPUs
On Thu, Jun 07, 2018 at 09:34:41AM +0300, Eleni Maria Stea wrote:
> Gen 7 GPUs store the compressed EAC/ETC2 images in other non-compressed
> formats that can render. When GetCompressed* functions are called, the
> pixels are returned in the non-compressed format that is used for the
> rendering.
>
> With this patch we store both the compressed and non-compressed versions
> of the image, so that both rendering commands and GetCompressed*
> commands work.
>
> Also, the assertions for GL_MAP_WRITE_BIT and GL_MAP_INVALIDATE_RANGE_BIT
> in intel_miptree_map_etc function have been removed because when the
> miptree is mapped for reading (for example from a GetCompress*
> function) the GL_MAP_WRITE_BIT won't be set (and shouldn't be set).
>
> Fixes: the following test in CTS for gen7:
> KHR-GL45.direct_state_access.textures_compressed_subimage test
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104272
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81843
>
> v2: fixes issues:
>a) initialized uninitialized variables (Juan A. Suarez, Andres Gomez)
>b) fixed race condition where mt and cmt were mapped at the same time
>c) fixed indentation issues (Andres Gomez)
> v3: adds bugzilla bug with id: 104272
> v4: adds bugzilla bug with id: 81843
> v5: replaced the flags with a bitfield, refactoring (Kenneth Graunke)
+Jason, Ken
Hello,
I recently did some miptree work relating to the r8stencil_mt and I
think I now have a more informed opinion about how things should be
structured. I'd like to propose an alternative solution.
I had initially thought we should have a separate miptree to hold the
compressed data, like this patch does, but now I think we should
actually have the compressed data be the main miptree and to store the
decompressed miptree as part of the main one. The reasoning is that we
could reuse this structure to handle the r8stencil workaround and to
eventually handle the ASTC_LDR surfaces that are modified on gen9.
I'm proposing something like the following:
1. Rename r8stencil_mt ->shadow_mt and
r8stencil_needs_update -> shadow_needs_update.
2. Make shadow_mt hold the decompressed ETC miptree
3. Update shadow_needs_update whenever the main mt is modified
4. Add an function to update the shadow_mt using the main mt as a source
5. Sample from the shadow_mt as appropriate
6. Make the main miptree hold the compressed data
This method should also be able to handle the CopyImage functions. What
do you all think?
-Nanley
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 10 +-
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 10 ++
> src/mesa/drivers/dri/i965/intel_tex.c | 106 +++---
> src/mesa/drivers/dri/i965/intel_tex.h | 1 -
> src/mesa/drivers/dri/i965/intel_tex_image.c | 46 +++-
> src/mesa/drivers/dri/i965/intel_tex_obj.h | 8 ++
> src/mesa/main/texstore.c | 51 ++---
> src/mesa/main/texstore.h | 8 +-
> 8 files changed, 197 insertions(+), 43 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 7d1fa96b91..cc807977de 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -733,9 +733,10 @@ miptree_create(struct brw_context *brw,
> mesa_format etc_format = MESA_FORMAT_NONE;
> uint32_t alloc_flags = 0;
>
> - format = intel_lower_compressed_format(brw, format);
> -
> - etc_format = (format != tex_format) ? tex_format : MESA_FORMAT_NONE;
> + if (!(flags & MIPTREE_CREATE_ETC)) {
> + format = intel_lower_compressed_format(brw, format);
> + etc_format = (format != tex_format) ? tex_format : MESA_FORMAT_NONE;
> + }
>
> if (flags & MIPTREE_CREATE_BUSY)
>alloc_flags |= BO_ALLOC_BUSY;
> @@ -3372,9 +3373,6 @@ intel_miptree_map_etc(struct brw_context *brw,
>assert(mt->format == MESA_FORMAT_R8G8B8X8_UNORM);
> }
>
> - assert(map->mode & GL_MAP_WRITE_BIT);
> - assert(map->mode & GL_MAP_INVALIDATE_RANGE_BIT);
> -
> intel_miptree_access_raw(brw, mt, level, slice, true);
>
> map->stride = _mesa_format_row_stride(mt->etc_format, map->w);
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index 42f73ba1f9..9e7a401229 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -74,6 +74,7 @@ struct intel_texture_image;
> * without transcoding back. This flag to intel_miptree_map() gets you that.
> */
> #define BRW_MAP_DIRECT_BIT 0x8000
> +#define BRW_MAP_ETC_BIT 0x4000
>
> struct intel_miptree_map {
> /** Bitfield of GL_MAP_*_BIT and BRW_MAP_*_BIT. */
> @@ -380,6 +381,15 @@ enum intel_miptree_create_flags {
> * that the miptree will be created with mt->aux_usage == NONE.
> */
> MIPTREE_CREATE_NO_AUX =
[Mesa-dev] [PATCH v5] i965: Fix ETC2/EAC GetCompressed* functions on Gen7 GPUs
Gen 7 GPUs store the compressed EAC/ETC2 images in other non-compressed
formats that can render. When GetCompressed* functions are called, the
pixels are returned in the non-compressed format that is used for the
rendering.
With this patch we store both the compressed and non-compressed versions
of the image, so that both rendering commands and GetCompressed*
commands work.
Also, the assertions for GL_MAP_WRITE_BIT and GL_MAP_INVALIDATE_RANGE_BIT
in intel_miptree_map_etc function have been removed because when the
miptree is mapped for reading (for example from a GetCompress*
function) the GL_MAP_WRITE_BIT won't be set (and shouldn't be set).
Fixes: the following test in CTS for gen7:
KHR-GL45.direct_state_access.textures_compressed_subimage test
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104272
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81843
v2: fixes issues:
a) initialized uninitialized variables (Juan A. Suarez, Andres Gomez)
b) fixed race condition where mt and cmt were mapped at the same time
c) fixed indentation issues (Andres Gomez)
v3: adds bugzilla bug with id: 104272
v4: adds bugzilla bug with id: 81843
v5: replaced the flags with a bitfield, refactoring (Kenneth Graunke)
---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 10 +-
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 10 ++
src/mesa/drivers/dri/i965/intel_tex.c | 106 +++---
src/mesa/drivers/dri/i965/intel_tex.h | 1 -
src/mesa/drivers/dri/i965/intel_tex_image.c | 46 +++-
src/mesa/drivers/dri/i965/intel_tex_obj.h | 8 ++
src/mesa/main/texstore.c | 51 ++---
src/mesa/main/texstore.h | 8 +-
8 files changed, 197 insertions(+), 43 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 7d1fa96b91..cc807977de 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -733,9 +733,10 @@ miptree_create(struct brw_context *brw,
mesa_format etc_format = MESA_FORMAT_NONE;
uint32_t alloc_flags = 0;
- format = intel_lower_compressed_format(brw, format);
-
- etc_format = (format != tex_format) ? tex_format : MESA_FORMAT_NONE;
+ if (!(flags & MIPTREE_CREATE_ETC)) {
+ format = intel_lower_compressed_format(brw, format);
+ etc_format = (format != tex_format) ? tex_format : MESA_FORMAT_NONE;
+ }
if (flags & MIPTREE_CREATE_BUSY)
alloc_flags |= BO_ALLOC_BUSY;
@@ -3372,9 +3373,6 @@ intel_miptree_map_etc(struct brw_context *brw,
assert(mt->format == MESA_FORMAT_R8G8B8X8_UNORM);
}
- assert(map->mode & GL_MAP_WRITE_BIT);
- assert(map->mode & GL_MAP_INVALIDATE_RANGE_BIT);
-
intel_miptree_access_raw(brw, mt, level, slice, true);
map->stride = _mesa_format_row_stride(mt->etc_format, map->w);
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
index 42f73ba1f9..9e7a401229 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -74,6 +74,7 @@ struct intel_texture_image;
* without transcoding back. This flag to intel_miptree_map() gets you that.
*/
#define BRW_MAP_DIRECT_BIT 0x8000
+#define BRW_MAP_ETC_BIT0x4000
struct intel_miptree_map {
/** Bitfield of GL_MAP_*_BIT and BRW_MAP_*_BIT. */
@@ -380,6 +381,15 @@ enum intel_miptree_create_flags {
* that the miptree will be created with mt->aux_usage == NONE.
*/
MIPTREE_CREATE_NO_AUX = 1 << 2,
+
+ /** Create a second miptree for the compressed pixels (Gen7 only)
+*
+* On Gen7, we need to store 2 miptrees for some compressed
+* formats so we can handle rendering as well as getting the
+* compressed image data. This flag indicates that the miptree
+* is expected to hold compressed data for the latter case.
+*/
+ MIPTREE_CREATE_ETC = 1 << 3,
};
struct intel_mipmap_tree *intel_miptree_create(struct brw_context *brw,
diff --git a/src/mesa/drivers/dri/i965/intel_tex.c
b/src/mesa/drivers/dri/i965/intel_tex.c
index 0650b6e629..3a94fa7477 100644
--- a/src/mesa/drivers/dri/i965/intel_tex.c
+++ b/src/mesa/drivers/dri/i965/intel_tex.c
@@ -66,6 +66,8 @@ intel_alloc_texture_image_buffer(struct gl_context *ctx,
struct intel_texture_image *intel_image = intel_texture_image(image);
struct gl_texture_object *texobj = image->TexObject;
struct intel_texture_object *intel_texobj = intel_texture_object(texobj);
+ struct gen_device_info *devinfo = >screen->devinfo;
+ mesa_format fmt = image->TexFormat;
assert(image->Border == 0);
@@ -110,6 +112,33 @@ intel_alloc_texture_image_buffer(struct gl_context *ctx,
image->Width, image->Height, image->Depth, intel_image->mt);
}
+ if (devinfo->gen < 8 && _mesa_is_format_etc2(fmt)) {
+ if (intel_texobj->cmt &&
+
