Re: [Mesa-dev] [PATCH 1/6] i965/state: Ignore intel_obj->_Format for depth/stencil and ETC2

[Chad Versace]

On Tue 20 Feb 2018, Jason Ekstrand wrote:
> On Mon, Feb 19, 2018 at 10:01 AM, Chad Versace <[1]chadvers...@chromium.org>
> wrote:
> 
> On Wed 24 Jan 2018, Jason Ekstrand wrote:
> > We're about to start letting the intel_obj->_Format be the "real"
> > texture format.  For depth/stencil textures, this may be a combined
> > depth stencil format.  For ETC2 on gen7 and earlier, this will be the
> > actual ETC2 format.  This makes a bit more GL sense but means we have to
> > be careful in state upload.
> 
> What is the "real" format? It's not a rhetorical question. Throughout
> Mesa, I never know what's real and what's not. By "real", do you mean
> the untranslated user-specified glTextureView(internalformat) and
> glTexImage2D(internalformat)?  Or do you mean simply "more real than
> before" ;)
> 
> 
> By "real" format, I mean the one that the core mesa state tracking code thinks
> it is.  For texture views, that corresponds directly to an actual GL internal
> format.  For textures created through glTexImage2D (not TexStorage) with an
> internal format such as GL_RGB, it's something computed from the internal
> format and the format used for upload.

I understand now. Sounds good to me.

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

Re: [Mesa-dev] [PATCH 1/6] i965/state: Ignore intel_obj->_Format for depth/stencil and ETC2

[Jason Ekstrand]

On Mon, Feb 19, 2018 at 10:01 AM, Chad Versace 
wrote:

> On Wed 24 Jan 2018, Jason Ekstrand wrote:
> > We're about to start letting the intel_obj->_Format be the "real"
> > texture format.  For depth/stencil textures, this may be a combined
> > depth stencil format.  For ETC2 on gen7 and earlier, this will be the
> > actual ETC2 format.  This makes a bit more GL sense but means we have to
> > be careful in state upload.
>
> What is the "real" format? It's not a rhetorical question. Throughout
> Mesa, I never know what's real and what's not. By "real", do you mean
> the untranslated user-specified glTextureView(internalformat) and
> glTexImage2D(internalformat)?  Or do you mean simply "more real than
> before" ;)
>

By "real" format, I mean the one that the core mesa state tracking code
thinks it is.  For texture views, that corresponds directly to an actual GL
internal format.  For textures created through glTexImage2D (not
TexStorage) with an internal format such as GL_RGB, it's something computed
from the internal format and the format used for upload.


> > ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 16 +++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > index 38af6bc..844c23b 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -507,7 +507,21 @@ brw_update_texture_surface(struct gl_context *ctx,
> >const unsigned swizzle = (unlikely(alpha_depth) ? SWIZZLE_XYZW :
> >  brw_get_texture_swizzle(>ctx,
> obj));
> >
> > -  mesa_format mesa_fmt = plane == 0 ? intel_obj->_Format :
> mt->format;
> > +  mesa_format mesa_fmt;
> > +  if (firstImage->_BaseFormat == GL_DEPTH_STENCIL ||
> > +  firstImage->_BaseFormat == GL_DEPTH_COMPONENT) {
> > + /* The format from intel_obj may be a combined depth stencil
> format
> > +  * when we just want depth.  Pull it from the miptree
> instead.  This
> > +  * is safe because texture views aren't allowed on
> depth/stencil.
> > +  */
> > + mesa_fmt = mt->format;
> > +  } else if (mt->etc_format != MESA_FORMAT_NONE) {
> > + mesa_fmt = mt->format;
>
> This looks like it would break ETC2 texture views on hw where we decode
> the ETC2 on upload (Ivybridge?), if such views worked. I suspect such
> texture views never worked.
>

I'm pretty sure they've never worked.


> > +  } else if (plane > 0) {
> > + mesa_fmt = mt->format;
> > +  } else {
> > + mesa_fmt = intel_obj->_Format;
> > +  }
> >enum isl_format format = translate_tex_format(brw, mesa_fmt,
> >  for_txf ?
> GL_DECODE_EXT :
> >
> sampler->sRGBDecode);
>
> I want to give a r-b, but want to first hear your reply regarding the
> "real" format.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/6] i965/state: Ignore intel_obj->_Format for depth/stencil and ETC2

[Chad Versace]

On Wed 24 Jan 2018, Jason Ekstrand wrote:
> We're about to start letting the intel_obj->_Format be the "real"
> texture format.  For depth/stencil textures, this may be a combined
> depth stencil format.  For ETC2 on gen7 and earlier, this will be the
> actual ETC2 format.  This makes a bit more GL sense but means we have to
> be careful in state upload.

What is the "real" format? It's not a rhetorical question. Throughout
Mesa, I never know what's real and what's not. By "real", do you mean
the untranslated user-specified glTextureView(internalformat) and
glTexImage2D(internalformat)?  Or do you mean simply "more real than
before" ;)

> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c 
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index 38af6bc..844c23b 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -507,7 +507,21 @@ brw_update_texture_surface(struct gl_context *ctx,
>const unsigned swizzle = (unlikely(alpha_depth) ? SWIZZLE_XYZW :
>  brw_get_texture_swizzle(>ctx, obj));
>  
> -  mesa_format mesa_fmt = plane == 0 ? intel_obj->_Format : mt->format;
> +  mesa_format mesa_fmt;
> +  if (firstImage->_BaseFormat == GL_DEPTH_STENCIL ||
> +  firstImage->_BaseFormat == GL_DEPTH_COMPONENT) {
> + /* The format from intel_obj may be a combined depth stencil format
> +  * when we just want depth.  Pull it from the miptree instead.  This
> +  * is safe because texture views aren't allowed on depth/stencil.
> +  */
> + mesa_fmt = mt->format;
> +  } else if (mt->etc_format != MESA_FORMAT_NONE) {
> + mesa_fmt = mt->format;

This looks like it would break ETC2 texture views on hw where we decode
the ETC2 on upload (Ivybridge?), if such views worked. I suspect such
texture views never worked.

> +  } else if (plane > 0) {
> + mesa_fmt = mt->format;
> +  } else {
> + mesa_fmt = intel_obj->_Format;
> +  }
>enum isl_format format = translate_tex_format(brw, mesa_fmt,
>  for_txf ? GL_DECODE_EXT :
>  sampler->sRGBDecode);

I want to give a r-b, but want to first hear your reply regarding the
"real" format.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev