Re: [Mesa-dev] [PATCH 3/4] tnl: Maintain the _WindowMap matrix in TNLcontext v2.

[Mathias Fröhlich]

Hi,

On Friday, August 07, 2015 12:17:15 Ilia Mirkin wrote:
> None of the other drivers appear to do it... should be safe. I'll def test
> it out before pushing, of course... I've been meaning to plug a nv1x in so
> I can play with a couple of minor items. Ideally it'd switch to the i965
> method, and only call tnl_InvalidateState when in swtnl mode (as well as
> calling tnl_wakeup on hwtnl -> !hwtnl transitions) but... meh. Actually it
> looks like SWTNL is largely unimplemented and it falls straight back to
> SWRAST? That's a bit unfortunate for nv04/nv05 :(
Sounds plausible so far.

Again, thanks for taking care of the problem report.

Greetings

Mathias___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/6] egl/x11: trust our loader over the xserver for the drivername

[Timothy Arceri]

This change is causing a bunch of issues during a piglit run on my system. I'm
getting error messages like this:

*** Error in `/home/timothy/data/piglit/bin/glslparsertest_gles2': free():
invalid pointer: 0x022d4be0 ***

Any ideas what might be wrong?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/6] egl/x11: trust our loader over the xserver for the drivername

[Boyan Ding]

2015-08-08 16:05 GMT+08:00 Timothy Arceri :
> This change is causing a bunch of issues during a piglit run on my system. I'm
> getting error messages like this:
>
> *** Error in `/home/timothy/data/piglit/bin/glslparsertest_gles2': free():
> invalid pointer: 0x022d4be0 ***
>
> Any ideas what might be wrong?

The problem is probably here:
> +   if (loader_driver_name) {
> +  free(driver_name);
> +  driver_name = loader_driver_name;
> +   }
> +

I think the 'free' here might be wrong. The glx code frees driverName because
the helper function DRI2Connect there malloc'ed a buffer to hold driver name
but the driver_name here is directly got from xcb reply. As you can see in the
following code, it should be dup'ed to use.

Regards,
Boyan Ding

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

[Mesa-dev] [PATCH] egl/x11: Fix driver_name acquisition

[Boyan Ding]

We don't need to free driverName string from dri2 reply, on the other
hand, the driver name acquired from loader doesn't need duplication.

This fixes commit 45e110ba

Cc: Emil Velikov 
Reported-by: Timothy Arceri 
Signed-off-by: Boyan Ding 
---
 src/egl/drivers/dri2/platform_x11.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_x11.c 
b/src/egl/drivers/dri2/platform_x11.c
index 61e5b77..ee5b53f 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -601,15 +601,14 @@ dri2_x11_connect(struct dri2_egl_display *dri2_dpy)
 * Otherwise, default to the server's value.
 */
loader_driver_name = loader_get_driver_for_fd(dri2_dpy->fd, 0);
-   if (loader_driver_name) {
-  free(driver_name);
-  driver_name = loader_driver_name;
+   if (loader_driver_name)
+  dri2_dpy->driver_name = loader_driver_name;
+   else {
+  dri2_dpy->driver_name =
+ strndup(driver_name,
+ xcb_dri2_connect_driver_name_length(connect));
}
 
-   dri2_dpy->driver_name =
-  strndup(driver_name,
-  xcb_dri2_connect_driver_name_length(connect));
-
if (dri2_dpy->device_name == NULL || dri2_dpy->driver_name == NULL) {
   close(dri2_dpy->fd);
   free(dri2_dpy->device_name);
-- 
2.5.0

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

Re: [Mesa-dev] [Mesa-stable] [PATCH 62/70] i965: Prevent coordinate overflow in intel_emit_linear_blit

[Chris Wilson]

On Fri, Aug 07, 2015 at 05:22:27PM -0700, Anuj Phogat wrote:
>I don't see any new changes as compared to the original patch you sent
>earlier.
>Reviewed-by: Anuj Phogat <[11]anuj.pho...@gmail.com>

Sorry, there were none, I just forgot about pushing to the head of the
queue and commiting it.

Thanks for the review and gentle poke.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] i965: Add SKL support to brw_miptree_get_horizontal_slice_pitch().

[Francisco Jerez]

Jason Ekstrand  writes:

> I'm not a huge fan of this patch.  Really, given how complicated 3-D
> textures are on SKL, there really is no sensible horizontal slice
> pitch.  We could return 0 as an "invalid value" but I think I'd rather
> keep it an assert.  Code that is dealing with 3-D textures should no
> better than to just blindly call this function on SKL.
> --Jason

How so?  The sub-tile structure may indeed be more complicated on SKL,
but the way how Z-slices of whole tiles are laid out in 2D memory for 3D
textures is even simpler than on earlier generations -- Say a given
tile-slice of LOD l starts at 2D coordinates (x, y), the next tile-slice
will start at (x, y + qpitch), IOW the horizontal offset between
tile-slices is zero which is what this patch does.  We could probably
keep the assertion I had but that would complicate
update_texture_image_param() a bit more so I'd rather do what AFAIUI is
the right thing here.

>
> On Wed, May 13, 2015 at 9:37 AM, Francisco Jerez  
> wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c 
>> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index 72b02a2..6c6dc5c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -281,9 +281,7 @@ brw_miptree_get_horizontal_slice_pitch(const struct 
>> brw_context *brw,
>> const struct intel_mipmap_tree *mt,
>> unsigned level)
>>  {
>> -   assert(brw->gen < 9);
>> -
>> -   if (mt->target == GL_TEXTURE_3D ||
>> +   if ((brw->gen < 9 && mt->target == GL_TEXTURE_3D) ||
>> (brw->gen == 4 && mt->target == GL_TEXTURE_CUBE_MAP)) {
>>return ALIGN(minify(mt->physical_width0, level), mt->align_w);
>> } else {
>> --
>> 2.3.5
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCHv2 07/14] i965: Implement surface state set-up for shader images.

[Francisco Jerez]

Jason Ekstrand  writes:

> On Wed, May 13, 2015 at 9:43 AM, Francisco Jerez  
> wrote:
>> v2: Add SKL support.
>> ---
>>  src/mesa/drivers/dri/i965/brw_context.h  |   2 +
>>  src/mesa/drivers/dri/i965/brw_surface_formats.c  | 109 
>> +++
>>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  77 
>>  3 files changed, 188 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
>> b/src/mesa/drivers/dri/i965/brw_context.h
>> index 2dcc23c..c55dcec 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -1741,6 +1741,8 @@ void brw_upload_abo_surfaces(struct brw_context *brw,
>>  bool brw_render_target_supported(struct brw_context *brw,
>>   struct gl_renderbuffer *rb);
>>  uint32_t brw_depth_format(struct brw_context *brw, mesa_format format);
>> +mesa_format brw_lower_mesa_image_format(const struct brw_device_info 
>> *devinfo,
>> +mesa_format format);
>>
>>  /* brw_performance_monitor.c */
>>  void brw_init_performance_monitors(struct brw_context *brw);
>> diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
>> b/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> index 016f87a..e0e7fb6 100644
>> --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> @@ -805,3 +805,112 @@ brw_depth_format(struct brw_context *brw, mesa_format 
>> format)
>>unreachable("Unexpected depth format.");
>> }
>>  }
>> +
>> +mesa_format
>> +brw_lower_mesa_image_format(const struct brw_device_info *devinfo,
>> +mesa_format format)
>> +{
>> +   switch (format) {
>> +   /* These are never lowered.  Up to BDW we'll have to fall back to untyped
>> +* surface access for 128bpp formats.
>> +*/
>> +   case MESA_FORMAT_RGBA_UINT32:
>> +   case MESA_FORMAT_RGBA_SINT32:
>> +   case MESA_FORMAT_RGBA_FLOAT32:
>> +   case MESA_FORMAT_R_UINT32:
>> +   case MESA_FORMAT_R_SINT32:
>> +   case MESA_FORMAT_R_FLOAT32:
>> +  return format;
>
> If it's an unsupported format, why not use MESA_FORMAT_NONE?  That
> would make it easier for functions that call this function to know
> that it's just not supported and they shouldn't bother trying.
>
Because they are all supported.  As the comment says these formats are
always accessed unlowered, the catch is that the shader may have to use
a different (much more painful) mechanism to access them depending on
the hardware generation -- But this function simply implements the
mapping between API image formats and what the 3D pipeline will see,
which is R(GBA)_*32 in this case regardless of whether untyped or typed
messages can be used.

>> +
>> +   /* From HSW to BDW the only 64bpp format supported for typed access is
>> +* RGBA_UINT16.  IVB falls back to untyped.
>> +*/
>> +   case MESA_FORMAT_RGBA_UINT16:
>> +   case MESA_FORMAT_RGBA_SINT16:
>> +   case MESA_FORMAT_RGBA_FLOAT16:
>> +   case MESA_FORMAT_RG_UINT32:
>> +   case MESA_FORMAT_RG_SINT32:
>> +   case MESA_FORMAT_RG_FLOAT32:
>> +  return (devinfo->gen >= 9 ? format :
>> +  devinfo->gen >= 8 || devinfo->is_haswell ?
>> +  MESA_FORMAT_RGBA_UINT16 : MESA_FORMAT_RG_UINT32);
>> +
>> +   /* Up to BDW no SINT or FLOAT formats of less than 32 bits per component
>> +* are supported.  IVB doesn't support formats with more than one 
>> component
>> +* for typed access.  For 8 and 16 bpp formats IVB relies on the
>> +* undocumented behavior that typed reads from R_UINT8 and R_UINT16
>> +* surfaces actually do a 32-bit misaligned read.  The alternative would 
>> be
>> +* to use two surface state entries with different formats for each 
>> image,
>> +* one for reading (using R_UINT32) and another one for writing (using
>> +* R_UINT8 or R_UINT16), but that would complicate the shaders we 
>> generate
>> +* even more.
>> +*/
>
> Let's make sure I'm understanding this correctly.  On BDW and HSW, we
> can just use the UINT format for SINT and FLOAT.  On IVB, we set the
> surface state to UINT32 and unpack the components in the shader?  What
> happens with writes on IVB to 16-bpp images?
>
Heh, not exactly.  On IVB we disobey the hardware spec and bind 16-bpp
formats as R16_UINT and 8-bpp formats as R8_UINT, which aren't
documented to work for typed reads.  Writes work just fine and update
the right 8- or 16-bit word on the image.  Reads *almost* do what one
would expect: The LSBs of the values returned from the data port will
contain the texel we asked for, but the MSBs will be filled with garbage
which we simply mask out
(c.f. image_format_info::has_undefined_high_bits).

>> +   case MESA_FORMAT_RGBA_UINT8:
>> +   case MESA_FORMAT_RGBA_SINT8:
>> +  return (devinfo->gen >= 9 ? format :
>> +  devinfo->gen >= 8 || devinfo->is_haswell ?
>> +  MESA_FORMAT_RGBA_UINT8 : MESA_FO

Re: [Mesa-dev] [PATCH] clover: Properly initialize LLVM targets when linking with component libs

[Francisco Jerez]

Tom Stellard  writes:

> Calls to LLVMIntialize* fail when we are linking against individual
> component libraries rather than one large shared object, because
> we only include component libraries that are required by the drivers.
>
> We need to make sure to only initialize the targets that we need.
>
> CC: 10.6 
> ---
>  configure.ac  |  4 
>  src/gallium/state_trackers/clover/Makefile.am |  3 ++-
>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 17 +
>  3 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 36197d3..e1a7d7a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2040,8 +2040,10 @@ require_egl_drm() {
>  radeon_llvm_check() {
>  if test ${LLVM_VERSION_INT} -lt 307; then
>  amdgpu_llvm_target_name='r600'
> + CLOVER_CPP_FLAGS="${CLOVER_CPP_FLAGS} -DCLOVER_INIT_R600_TARGET"
>  else
>  amdgpu_llvm_target_name='amdgpu'
> + CLOVER_CPP_FLAGS="${CLOVER_CPP_FLAGS} -DCLOVER_INIT_AMDGPU_TARGET"
>  fi
>  if test "x$enable_gallium_llvm" != "xyes"; then
>  AC_MSG_ERROR([--enable-gallium-llvm is required when building $1])
> @@ -2285,6 +2287,8 @@ AC_SUBST([XA_MINOR], $XA_MINOR)
>  AC_SUBST([XA_TINY], $XA_TINY)
>  AC_SUBST([XA_VERSION], "$XA_MAJOR.$XA_MINOR.$XA_TINY")
>  
> +AC_SUBST([CLOVER_CPP_FLAGS], $CLOVER_CPP_FLAGS)
> +
>  dnl Restore LDFLAGS and CPPFLAGS
>  LDFLAGS="$_SAVE_LDFLAGS"
>  CPPFLAGS="$_SAVE_CPPFLAGS"
> diff --git a/src/gallium/state_trackers/clover/Makefile.am 
> b/src/gallium/state_trackers/clover/Makefile.am
> index fd0ccf8..975b36f 100644
> --- a/src/gallium/state_trackers/clover/Makefile.am
> +++ b/src/gallium/state_trackers/clover/Makefile.am
> @@ -45,7 +45,8 @@ libclllvm_la_CXXFLAGS = \
>   $(DEFINES) \
>   -DLIBCLC_INCLUDEDIR=\"$(LIBCLC_INCLUDEDIR)/\" \
>   -DLIBCLC_LIBEXECDIR=\"$(LIBCLC_LIBEXECDIR)/\" \
> - -DCLANG_RESOURCE_DIR=\"$(CLANG_RESOURCE_DIR)\"
> + -DCLANG_RESOURCE_DIR=\"$(CLANG_RESOURCE_DIR)\" \
> + $(CLOVER_CPP_FLAGS)
>  
>  libclllvm_la_SOURCES = $(LLVM_SOURCES)
>  
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
> b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index 86859af..361a149 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -786,10 +786,19 @@ namespace {
> init_targets() {
>static bool targets_initialized = false;
>if (!targets_initialized) {
> - LLVMInitializeAllTargets();
> - LLVMInitializeAllTargetInfos();
> - LLVMInitializeAllTargetMCs();
> - LLVMInitializeAllAsmPrinters();
> +#ifdef CLOVER_INIT_AMDGPU_TARGET
> + LLVMInitializeAMDGPUTarget();
> + LLVMInitializeAMDGPUTargetInfo();
> + LLVMInitializeAMDGPUTargetMC();
> + LLVMInitializeAMDGPUAsmPrinter();
> +#endif
> +
> +#ifdef CLOVER_INIT_R600_TARGET
> + LLVMInitializeR600Target();
> + LLVMInitializeR600TargetInfo();
> + LLVMInitializeR600TargetMC();
> + LLVMInitializeR600AsmPrinter();
> +#endif

Doesn't this feel like a layering violation?  Why should clover
initialize specific LLVM back-ends?  And isn't it a build-system bug if,
say, LLVMInitializeAllTargets() is being pulled in but some symbol it
depends on isn't?

>   targets_initialized = true;
>}
> }
> -- 
> 2.0.4


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 (part2) 49/59] glsl: Do not allow assignments to read-only variables

[Francisco Jerez]

Iago Toral  writes:

> On Wed, 2015-08-05 at 22:22 +1000, Timothy Arceri wrote:
>> On Wed, 2015-08-05 at 13:45 +0200, Iago Toral wrote:
>> > On Wed, 2015-08-05 at 20:04 +1000, Timothy Arceri wrote:
>> > > On Wed, 2015-08-05 at 10:30 +0200, Iago Toral Quiroga wrote:
>> > > > ---
>> > > >  src/glsl/ast_to_hir.cpp | 9 -
>> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
>> > > > 
>> > > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> > > > index e834a46..518612d 100644
>> > > > --- a/src/glsl/ast_to_hir.cpp
>> > > > +++ b/src/glsl/ast_to_hir.cpp
>> > > > @@ -811,8 +811,15 @@ do_assignment(exec_list *instructions, struct 
>> > > > _mesa_glsl_parse_state *state,
>> > > > }
>> > > >  
>> > > > ir_variable *lhs_var = lhs->variable_referenced();
>> > > > -   if (lhs_var)
>> > > > +   if (lhs_var) {
>> > > > +  if (lhs_var->data.image_read_only) {
>> > > 
>> > > It looks like data.read_only is always set to true for images so 
>> > > wouldn't 
>> > > this
>> > > already be caught already by the existing read-only check?
>> > > 
>> > >   else if (lhs_var != NULL && lhs_var->data.read_only) {
>> > >  _mesa_glsl_error(&lhs_loc, state,
>> > >   "assignment to read-only variable '%s'",
>> > >   lhs_var->name);
>> > 
>> > Not as it is now, because with SSBOs we only set image_read_only and not
>> > read_only when the readonly qualifier is used. I suppose this is what we
>> > are expected to do since the SSBO spec says that behavior for these
>> > qualifiers on SSBOs is the same as for images:
>> > https://www.opengl.org/registry/specs/ARB/shader_storage_buffer_object.txt
>> > 
>> > "Modify Section 4.10, Memory Qualifiers (p. 71)"
>> > (...)
>> > "(insert after third paragraph, p. 73) The memory qualifiers "coherent",
>> > "volatile", "restrict", "readonly", and "writeonly" may be used in the
>> > declaration of buffer variables (i.e., members of shader storage blocks).
>> > When a buffer variable is declared with a memory qualifier, the behavior
>> > specified for memory accesses involving image variables described above
>> > applies identically to memory accesses involving that buffer variable.  It
>> > is an error to assign to a buffer variable qualified with "readonly" or to
>> > read from a buffer variable qualified with "writeonly".
>> > 
>> > What is a bit confusing for me is that images seem to set
>> > image_read_only depending on whether we used the readonly qualifier or
>> > not (like ssbos) but then they also set read_only to true
>> > unconditionally, so I guess there is a difference between both fields,
>> 
>> Asking what the difference is was originally going to be my first question to
>> you :)
>> 
>> > but I don't know what it is exactly, specially since you can also use
>> > writeonly on images, for example.
>> 
>> So I really dont know much about images but after some reading the conclusion
>> I've come to is the qualifiers (image_read_only) are meant to limit how you
>> can use imageStore(), imageLoad() and imageAtomic*() etc.
>
> Looking at ARB_shader_image_load_store that seems consistent...  In that
> case I imagine that we could just set read_only for buffer variables
> with the readonly qualifier instead of image_read_only and drop this
> patch. We will need to add, at least, write_only to ir_variable as well
> I guess... I imagine that the 3 other fields (image_coherent,
> image_restrict, image_volatile) do not have image-specific semantics
> like image_read_only and image_write_oly and can be shared with ssbos
> we do not have to replicate them in ir_variable as well (in that case we
> might want to rename them so it is clear that image_read_only and
> image_write_only really are special and specific to images)
>
> Curro, what do you think?
>
Yes, the image_* fields (which we should definitely rename now that they
also affect SSBO variables) are just the memory qualifiers defined by
the GLSL spec, in the case of images the difference between these and
the original ir_variable::read_only is clear: The former apply to the
memory pointed to by the variable, while the latter refers to the
variable itself -- So in principle one could have a fixed image uniform
(fixed in the sense of always pointing to the same image unit) pointing
to some write-only image (i.e. only ir_variable::read_only set), or a
image uniform mutated during the execution of the program pointing to a
read-only image (i.e. only ir_variable::image_read_only set -- Although
in practice this latter possibility is disallowed by the spec, which is
why images currently always have ir_variable::read_only set).

For buffer variables the distinction is blurred because syntactically
the variable is both pointer-to-storage and storage at the same time, so
AFAICT the approach you've taken of checking whether image_read_only is
set for the LHS of an assignment seems reasonable to me, because this
way all image_* memory qualifiers apply to t

[Mesa-dev] [PATCH] gallium/radeon: fix r600g build if LLVM is disabled

[Marek Olšák]

From: Marek Olšák 

MESA_LLVM_VERSION_PATCH is undefined.
---
 src/gallium/drivers/radeon/r600_pipe_common.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c 
b/src/gallium/drivers/radeon/r600_pipe_common.c
index d6b9a01..e50e768 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -891,10 +891,11 @@ bool r600_common_screen_init(struct r600_common_screen 
*rscreen,
 
ws->query_info(ws, &rscreen->info);
 
-   if (HAVE_LLVM)
-   snprintf(llvm_string, sizeof(llvm_string),
-", LLVM %i.%i.%i", (HAVE_LLVM >> 8) & 0xff,
-HAVE_LLVM & 0xff, MESA_LLVM_VERSION_PATCH);
+#if HAVE_LLVM
+   snprintf(llvm_string, sizeof(llvm_string),
+", LLVM %i.%i.%i", (HAVE_LLVM >> 8) & 0xff,
+HAVE_LLVM & 0xff, MESA_LLVM_VERSION_PATCH);
+#endif
 
snprintf(rscreen->renderer_string, sizeof(rscreen->renderer_string),
 "%s (DRM %i.%i.%i%s)",
-- 
2.1.4

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

Re: [Mesa-dev] [PATCH] egl/x11: Fix driver_name acquisition

[Emil Velikov]

On 8 August 2015 at 10:23, Boyan Ding  wrote:
> We don't need to free driverName string from dri2 reply, on the other
> hand, the driver name acquired from loader doesn't need duplication.
>
> This fixes commit 45e110ba
>
> Cc: Emil Velikov 
> Reported-by: Timothy Arceri 
> Signed-off-by: Boyan Ding 
> ---
>  src/egl/drivers/dri2/platform_x11.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_x11.c 
> b/src/egl/drivers/dri2/platform_x11.c
> index 61e5b77..ee5b53f 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -601,15 +601,14 @@ dri2_x11_connect(struct dri2_egl_display *dri2_dpy)
>  * Otherwise, default to the server's value.
>  */
> loader_driver_name = loader_get_driver_for_fd(dri2_dpy->fd, 0);
> -   if (loader_driver_name) {
> -  free(driver_name);
> -  driver_name = loader_driver_name;
> +   if (loader_driver_name)
> +  dri2_dpy->driver_name = loader_driver_name;
> +   else {
> +  dri2_dpy->driver_name =
> + strndup(driver_name,
> + xcb_dri2_connect_driver_name_length(connect));
> }
I've added a pair of brackets, to keep the if symmetrical to the else,
and pushed this to master.

Thanks for fixing my thinko Boyan.
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/x11: Fix driver_name acquisition

[Timothy Arceri]

On Sat, 2015-08-08 at 17:23 +0800, Boyan Ding wrote:
> We don't need to free driverName string from dri2 reply, on the other
> hand, the driver name acquired from loader doesn't need duplication.
> 
> This fixes commit 45e110ba
> 
> Cc: Emil Velikov 
> Reported-by: Timothy Arceri 
> Signed-off-by: Boyan Ding 

Nice thanks, fixes the issue and looks right to me.

Reviewed-by: Timothy Arceri 

> ---
>  src/egl/drivers/dri2/platform_x11.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_x11.c 
> b/src/egl/drivers/dri2/platform_x11.c
> index 61e5b77..ee5b53f 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -601,15 +601,14 @@ dri2_x11_connect(struct dri2_egl_display *dri2_dpy)
>  * Otherwise, default to the server's value.
>  */
> loader_driver_name = loader_get_driver_for_fd(dri2_dpy->fd, 0);
> -   if (loader_driver_name) {
> -  free(driver_name);
> -  driver_name = loader_driver_name;
> +   if (loader_driver_name)
> +  dri2_dpy->driver_name = loader_driver_name;
> +   else {
> +  dri2_dpy->driver_name =
> + strndup(driver_name,
> + xcb_dri2_connect_driver_name_length(connect));
> }
>  
> -   dri2_dpy->driver_name =
> -  strndup(driver_name,
> -  xcb_dri2_connect_driver_name_length(connect));
> -
> if (dri2_dpy->device_name == NULL || dri2_dpy->driver_name == NULL) {
>close(dri2_dpy->fd);
>free(dri2_dpy->device_name);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] mesa/formats: Only do byteswapping for packed formats

[Jason Ekstrand]

Cc: Iago Toral 
Cc: Oded Gabbay 
---
 src/mesa/main/formats.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
index baeb1bf..d927073 100644
--- a/src/mesa/main/formats.c
+++ b/src/mesa/main/formats.c
@@ -372,10 +372,10 @@ uint32_t
 _mesa_format_to_array_format(mesa_format format)
 {
const struct gl_format_info *info = _mesa_get_format_info(format);
-   if (_mesa_little_endian())
-  return info->ArrayFormat;
-   else
+   if (!_mesa_little_endian() && info->Layout == MESA_FORMAT_LAYOUT_PACKED)
   return _mesa_array_format_flip_channels(info->ArrayFormat);
+   else
+  return info->ArrayFormat;
 }
 
 static struct hash_table *format_array_format_table;
-- 
2.4.3

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

Re: [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

[Jason Ekstrand]

On Fri, Aug 7, 2015 at 10:38 PM, Oded Gabbay  wrote:
> On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand  wrote:
>> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay  wrote:
>>> This patch fixes a bug that is manifested in the read path of mesa when
>>> running on big-endian machines. The effects can be seen when running
>>> piglit sanity test and/or taking a screen capture.
>>
>> piglit sanity isn't all that convincing.  It's quite possible that
>> there are two byte-swapping bugs that just happen to cancel out.  If
>> it fixes huge numbers of piglit tests, that's more convincing.
>>
>
> Well, without it about 65% of piglit tests fails. With this fix, about
> 30% fails. I would say that accounts for something...

Yes, that counts for far more than just sanity.  It would be
instructive to see the piglit diff between 2ec8718d, 8ec6534b2, and
5038d839b.  Those three patches are sequential and the one in the
middle, uses _mesa_format_convert for texture upload.

>>> The bug is caused when _mesa_format_convert receives src_format as
>>> mesa_format, which it thens changes to mesa_array_format. During this
>>> change, it checks for endianness and swaps the bytes accordingly.
>>> However, because the bytes are _already_ swapped in the memory itself
>>> (being written there by llvmpipe/softpipe/sw rast), and src_format
>>> value matches the _actual_ contents of the memory, the result of the
>>> read is wrong.
>>>
>>> Therefore, because other layers/functions, such as llvm or
>>> read_rgba_pixels() takes care whether we are dealing with big-endian or
>>> little-endian, _mesa_format_convert should be endian agnostic to avoid
>>> duplicate swapping of bytes.
>>
>> As far as I know, the core mesa format conversion code has handled
>> byte-swapping for a very long time.  it is *not* simply punted to the
>> driver.  I'm 100% ready to believe that there is a problem but it's
>> probably far more subtle than just removing support for byte-swapping
>> in _mesa_format_to_array_format.
>
> First of all, this bug has been in a very long time as well (I tracked
> it to between 10.4 to 10.5). It has shown up since mesa started to use
> _mesa_format_convert
> From my debugging, I see that llvmpipe (and other) write the data to
> memory in BE format. e.g if the format is RGBA, then the data is
> actually written in AGBR format.
> Now, in _mesa_format_convert you get the src_format as AGBR (already
> swapped). However, because it arrives as format and not array_format,
> _mesa_format_convert needs to build the array_format in
> _mesa_format_to_array_format()
>
> In _mesa_format_to_array_format, after the array_format is created,
> you swap it if it is BE, so you now have RGBA in array_format
> representing the source. And that is the bug!

Not quite... MESA_FORMAT formats are specified one of two ways: array
and packed.  Packed formats are specified in terms of low bits and
high bits in the word while array formats are specified in terms of an
array of elements of a certain type.  For the most part, packed is
used for things like 565 which obviously aren't array formats.
However, packed is also used for the normal 8-bit RGBA formats.  Since
they are specified in terms of low vs. high bits in a byte, they *do*
need to be swapped when going to an array format.  There was, indeed a
bug in there: we were swapping even if the MESA_FORMAT was already an
array format.  I just sent out a patch to fix that bug.

It would also be instructive to know what the actual enums looked like
and what the data that you're refering to looked like.  The enum
values are described in mesa/main/formats.h.

It's quite possible that, in all the BE cases, we aren't actually
treating the MESA_FORMATs as described above.  If that is the case,
then we should figure out what they do with the formats and either
change the BE cases or redefine the MESA_FORMATs in such a way that it
makes more sense on BE hardware.  However, as it currently stands
_mesa_format_to_array_format (as patched by the one I just sent)
matches the definitions.

> of course I checked that _mesa_format_to_array_format() is only called
> from _mesa_format_convert so I can safely modify it according to
> _mesa_format_convert usage.
>
> So I don't think it is "far more subtle" than removing this
> byte-swapping. If you still think so, please elaborate.

There are many places in the driver where we look at endianness and do
something with formats.  Obviously, something changed with 5038d839b;
we were taking something into account and aren't now or we weren't
then and are now.  What that something was, I'm not sure.

Where all could the bug be?  Lots of places.  It could be in generated
pack/unpack code but I find that unlikely.  It could be where your
patch indicates but that seems to violate the definition of the
MESA_FORMATs.  It's possible that piglit is setting LSB_FIRST and
that's not getting respected.  If you're looking at gallium swrast or
llvmpipe, it could be in st/mesa where we convert from 

Re: [Mesa-dev] [PATCH] mesa/formats: Only do byteswapping for packed formats

[Oded Gabbay]

On Sat, Aug 8, 2015 at 7:04 PM, Jason Ekstrand  wrote:
> Cc: Iago Toral 
> Cc: Oded Gabbay 
> ---
>  src/mesa/main/formats.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
> index baeb1bf..d927073 100644
> --- a/src/mesa/main/formats.c
> +++ b/src/mesa/main/formats.c
> @@ -372,10 +372,10 @@ uint32_t
>  _mesa_format_to_array_format(mesa_format format)
>  {
> const struct gl_format_info *info = _mesa_get_format_info(format);
> -   if (_mesa_little_endian())
> -  return info->ArrayFormat;
> -   else
> +   if (!_mesa_little_endian() && info->Layout == MESA_FORMAT_LAYOUT_PACKED)
>return _mesa_array_format_flip_channels(info->ArrayFormat);
> +   else
> +  return info->ArrayFormat;
>  }
>
>  static struct hash_table *format_array_format_table;
> --
> 2.4.3
>
Your patch doesn't solve the original bug. Piglit sanity (and others)
still fail and screen capture is still wrong.

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

Re: [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

[Oded Gabbay]

On Sat, Aug 8, 2015 at 7:34 PM, Jason Ekstrand  wrote:
> On Fri, Aug 7, 2015 at 10:38 PM, Oded Gabbay  wrote:
>> On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand  wrote:
>>> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay  wrote:
 This patch fixes a bug that is manifested in the read path of mesa when
 running on big-endian machines. The effects can be seen when running
 piglit sanity test and/or taking a screen capture.
>>>
>>> piglit sanity isn't all that convincing.  It's quite possible that
>>> there are two byte-swapping bugs that just happen to cancel out.  If
>>> it fixes huge numbers of piglit tests, that's more convincing.
>>>
>>
>> Well, without it about 65% of piglit tests fails. With this fix, about
>> 30% fails. I would say that accounts for something...
>
> Yes, that counts for far more than just sanity.  It would be
> instructive to see the piglit diff between 2ec8718d, 8ec6534b2, and
> 5038d839b.  Those three patches are sequential and the one in the
> middle, uses _mesa_format_convert for texture upload.
>
 The bug is caused when _mesa_format_convert receives src_format as
 mesa_format, which it thens changes to mesa_array_format. During this
 change, it checks for endianness and swaps the bytes accordingly.
 However, because the bytes are _already_ swapped in the memory itself
 (being written there by llvmpipe/softpipe/sw rast), and src_format
 value matches the _actual_ contents of the memory, the result of the
 read is wrong.

 Therefore, because other layers/functions, such as llvm or
 read_rgba_pixels() takes care whether we are dealing with big-endian or
 little-endian, _mesa_format_convert should be endian agnostic to avoid
 duplicate swapping of bytes.
>>>
>>> As far as I know, the core mesa format conversion code has handled
>>> byte-swapping for a very long time.  it is *not* simply punted to the
>>> driver.  I'm 100% ready to believe that there is a problem but it's
>>> probably far more subtle than just removing support for byte-swapping
>>> in _mesa_format_to_array_format.
>>
>> First of all, this bug has been in a very long time as well (I tracked
>> it to between 10.4 to 10.5). It has shown up since mesa started to use
>> _mesa_format_convert
>> From my debugging, I see that llvmpipe (and other) write the data to
>> memory in BE format. e.g if the format is RGBA, then the data is
>> actually written in AGBR format.
>> Now, in _mesa_format_convert you get the src_format as AGBR (already
>> swapped). However, because it arrives as format and not array_format,
>> _mesa_format_convert needs to build the array_format in
>> _mesa_format_to_array_format()
>>
>> In _mesa_format_to_array_format, after the array_format is created,
>> you swap it if it is BE, so you now have RGBA in array_format
>> representing the source. And that is the bug!
>
> Not quite... MESA_FORMAT formats are specified one of two ways: array
> and packed.  Packed formats are specified in terms of low bits and
> high bits in the word while array formats are specified in terms of an
> array of elements of a certain type.  For the most part, packed is
> used for things like 565 which obviously aren't array formats.
> However, packed is also used for the normal 8-bit RGBA formats.  Since
> they are specified in terms of low vs. high bits in a byte, they *do*
> need to be swapped when going to an array format.  There was, indeed a
> bug in there: we were swapping even if the MESA_FORMAT was already an
> array format.  I just sent out a patch to fix that bug.
>
> It would also be instructive to know what the actual enums looked like
> and what the data that you're refering to looked like.  The enum
> values are described in mesa/main/formats.h.
>
> It's quite possible that, in all the BE cases, we aren't actually
> treating the MESA_FORMATs as described above.  If that is the case,
> then we should figure out what they do with the formats and either
> change the BE cases or redefine the MESA_FORMATs in such a way that it
> makes more sense on BE hardware.  However, as it currently stands
> _mesa_format_to_array_format (as patched by the one I just sent)
> matches the definitions.
>
>> of course I checked that _mesa_format_to_array_format() is only called
>> from _mesa_format_convert so I can safely modify it according to
>> _mesa_format_convert usage.
>>
>> So I don't think it is "far more subtle" than removing this
>> byte-swapping. If you still think so, please elaborate.
>
> There are many places in the driver where we look at endianness and do
> something with formats.  Obviously, something changed with 5038d839b;
> we were taking something into account and aren't now or we weren't
> then and are now.  What that something was, I'm not sure.
>
> Where all could the bug be?  Lots of places.  It could be in generated
> pack/unpack code but I find that unlikely.  It could be where your
> patch indicates but that seems to violate the definition of the
> MESA_FORMA

Re: [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

[Jason Ekstrand]

On Sat, Aug 8, 2015 at 1:01 PM, Oded Gabbay  wrote:
> On Sat, Aug 8, 2015 at 7:34 PM, Jason Ekstrand  wrote:
>> On Fri, Aug 7, 2015 at 10:38 PM, Oded Gabbay  wrote:
>>> On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand  wrote:
 On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay  wrote:
> This patch fixes a bug that is manifested in the read path of mesa when
> running on big-endian machines. The effects can be seen when running
> piglit sanity test and/or taking a screen capture.

 piglit sanity isn't all that convincing.  It's quite possible that
 there are two byte-swapping bugs that just happen to cancel out.  If
 it fixes huge numbers of piglit tests, that's more convincing.

>>>
>>> Well, without it about 65% of piglit tests fails. With this fix, about
>>> 30% fails. I would say that accounts for something...
>>
>> Yes, that counts for far more than just sanity.  It would be
>> instructive to see the piglit diff between 2ec8718d, 8ec6534b2, and
>> 5038d839b.  Those three patches are sequential and the one in the
>> middle, uses _mesa_format_convert for texture upload.
>>
> The bug is caused when _mesa_format_convert receives src_format as
> mesa_format, which it thens changes to mesa_array_format. During this
> change, it checks for endianness and swaps the bytes accordingly.
> However, because the bytes are _already_ swapped in the memory itself
> (being written there by llvmpipe/softpipe/sw rast), and src_format
> value matches the _actual_ contents of the memory, the result of the
> read is wrong.
>
> Therefore, because other layers/functions, such as llvm or
> read_rgba_pixels() takes care whether we are dealing with big-endian or
> little-endian, _mesa_format_convert should be endian agnostic to avoid
> duplicate swapping of bytes.

 As far as I know, the core mesa format conversion code has handled
 byte-swapping for a very long time.  it is *not* simply punted to the
 driver.  I'm 100% ready to believe that there is a problem but it's
 probably far more subtle than just removing support for byte-swapping
 in _mesa_format_to_array_format.
>>>
>>> First of all, this bug has been in a very long time as well (I tracked
>>> it to between 10.4 to 10.5). It has shown up since mesa started to use
>>> _mesa_format_convert
>>> From my debugging, I see that llvmpipe (and other) write the data to
>>> memory in BE format. e.g if the format is RGBA, then the data is
>>> actually written in AGBR format.
>>> Now, in _mesa_format_convert you get the src_format as AGBR (already
>>> swapped). However, because it arrives as format and not array_format,
>>> _mesa_format_convert needs to build the array_format in
>>> _mesa_format_to_array_format()
>>>
>>> In _mesa_format_to_array_format, after the array_format is created,
>>> you swap it if it is BE, so you now have RGBA in array_format
>>> representing the source. And that is the bug!
>>
>> Not quite... MESA_FORMAT formats are specified one of two ways: array
>> and packed.  Packed formats are specified in terms of low bits and
>> high bits in the word while array formats are specified in terms of an
>> array of elements of a certain type.  For the most part, packed is
>> used for things like 565 which obviously aren't array formats.
>> However, packed is also used for the normal 8-bit RGBA formats.  Since
>> they are specified in terms of low vs. high bits in a byte, they *do*
>> need to be swapped when going to an array format.  There was, indeed a
>> bug in there: we were swapping even if the MESA_FORMAT was already an
>> array format.  I just sent out a patch to fix that bug.
>>
>> It would also be instructive to know what the actual enums looked like
>> and what the data that you're refering to looked like.  The enum
>> values are described in mesa/main/formats.h.
>>
>> It's quite possible that, in all the BE cases, we aren't actually
>> treating the MESA_FORMATs as described above.  If that is the case,
>> then we should figure out what they do with the formats and either
>> change the BE cases or redefine the MESA_FORMATs in such a way that it
>> makes more sense on BE hardware.  However, as it currently stands
>> _mesa_format_to_array_format (as patched by the one I just sent)
>> matches the definitions.
>>
>>> of course I checked that _mesa_format_to_array_format() is only called
>>> from _mesa_format_convert so I can safely modify it according to
>>> _mesa_format_convert usage.
>>>
>>> So I don't think it is "far more subtle" than removing this
>>> byte-swapping. If you still think so, please elaborate.
>>
>> There are many places in the driver where we look at endianness and do
>> something with formats.  Obviously, something changed with 5038d839b;
>> we were taking something into account and aren't now or we weren't
>> then and are now.  What that something was, I'm not sure.
>>
>> Where all could the bug be?  Lots of places.  It could be in generated
>> 

[Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Jason Ekstrand]

Mesa formats and gallium formats are defined a bit differently.  In mesa
there are "packed" formats which are based on byte-order within a 8, 16, or
32-bit word and there are "array" formats which are simply an array of 8,
16, or 32-bit values.  In gallium, they do something different called
"plain" which I've never been able to fully understand.  However, for those
formats which mesa defines as "packed", they should match up 1-1 with the
corresponding "plane" gallium format.

Unfortunately, st_format.c was using the endian-dependent wrappers such as
PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
big-endian systems, it breaks because it introduces one extra byte-swap.

Cc: Brian Paul 
Cc: Oded Gabbay 
---
 src/mesa/state_tracker/st_format.c | 94 +++---
 1 file changed, 47 insertions(+), 47 deletions(-)

diff --git a/src/mesa/state_tracker/st_format.c 
b/src/mesa/state_tracker/st_format.c
index db7b5b7..f946f24 100644
--- a/src/mesa/state_tracker/st_format.c
+++ b/src/mesa/state_tracker/st_format.c
@@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
mesa_format mesaFormat)
 {
switch (mesaFormat) {
case MESA_FORMAT_A8B8G8R8_UNORM:
-  return PIPE_FORMAT_ABGR_UNORM;
+  return PIPE_FORMAT_A8B8G8R8_UNORM;
case MESA_FORMAT_R8G8B8A8_UNORM:
-  return PIPE_FORMAT_RGBA_UNORM;
+  return PIPE_FORMAT_R8G8B8A8_UNORM;
case MESA_FORMAT_B8G8R8A8_UNORM:
-  return PIPE_FORMAT_BGRA_UNORM;
+  return PIPE_FORMAT_B8G8R8A8_UNORM;
case MESA_FORMAT_A8R8G8B8_UNORM:
-  return PIPE_FORMAT_ARGB_UNORM;
+  return PIPE_FORMAT_A8R8G8B8_UNORM;
case MESA_FORMAT_X8B8G8R8_UNORM:
-  return PIPE_FORMAT_XBGR_UNORM;
+  return PIPE_FORMAT_X8B8G8R8_UNORM;
case MESA_FORMAT_R8G8B8X8_UNORM:
-  return PIPE_FORMAT_RGBX_UNORM;
+  return PIPE_FORMAT_R8G8B8X8_UNORM;
case MESA_FORMAT_B8G8R8X8_UNORM:
-  return PIPE_FORMAT_BGRX_UNORM;
+  return PIPE_FORMAT_B8G8R8X8_UNORM;
case MESA_FORMAT_X8R8G8B8_UNORM:
-  return PIPE_FORMAT_XRGB_UNORM;
+  return PIPE_FORMAT_X8R8G8B8_UNORM;
case MESA_FORMAT_B5G5R5A1_UNORM:
   return PIPE_FORMAT_B5G5R5A1_UNORM;
case MESA_FORMAT_B4G4R4A4_UNORM:
@@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
mesa_format mesaFormat)
case MESA_FORMAT_BGR_SRGB8:
   return PIPE_FORMAT_R8G8B8_SRGB;
case MESA_FORMAT_A8B8G8R8_SRGB:
-  return PIPE_FORMAT_ABGR_SRGB;
+  return PIPE_FORMAT_A8B8G8R8_SRGB;
case MESA_FORMAT_R8G8B8A8_SRGB:
-  return PIPE_FORMAT_RGBA_SRGB;
+  return PIPE_FORMAT_R8G8B8A8_SRGB;
case MESA_FORMAT_B8G8R8A8_SRGB:
-  return PIPE_FORMAT_BGRA_SRGB;
+  return PIPE_FORMAT_B8G8R8A8_SRGB;
case MESA_FORMAT_A8R8G8B8_SRGB:
-  return PIPE_FORMAT_ARGB_SRGB;
+  return PIPE_FORMAT_A8R8G8B8_SRGB;
case MESA_FORMAT_RGBA_FLOAT32:
   return PIPE_FORMAT_R32G32B32A32_FLOAT;
case MESA_FORMAT_RGBA_FLOAT16:
@@ -199,13 +199,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
mesa_format mesaFormat)
case MESA_FORMAT_R_UNORM16:
   return PIPE_FORMAT_R16_UNORM;
case MESA_FORMAT_R8G8_UNORM:
-  return PIPE_FORMAT_RG88_UNORM;
+  return PIPE_FORMAT_R8G8_UNORM;
case MESA_FORMAT_G8R8_UNORM:
-  return PIPE_FORMAT_GR88_UNORM;
+  return PIPE_FORMAT_G8R8_UNORM;
case MESA_FORMAT_R16G16_UNORM:
-  return PIPE_FORMAT_RG1616_UNORM;
+  return PIPE_FORMAT_R16G16_UNORM;
case MESA_FORMAT_G16R16_UNORM:
-  return PIPE_FORMAT_GR1616_UNORM;
+  return PIPE_FORMAT_G16R16_UNORM;
case MESA_FORMAT_RGBA_UNORM16:
   return PIPE_FORMAT_R16G16B16A16_UNORM;
 
@@ -357,7 +357,7 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
mesa_format mesaFormat)
case MESA_FORMAT_G8R8_SNORM:
   return PIPE_FORMAT_GR88_SNORM;
case MESA_FORMAT_R8G8B8A8_SNORM:
-  return PIPE_FORMAT_RGBA_SNORM;
+  return PIPE_FORMAT_R8G8B8A8_SNORM;
case MESA_FORMAT_A8B8G8R8_SNORM:
   return PIPE_FORMAT_ABGR_SNORM;
 
@@ -476,21 +476,21 @@ mesa_format
 st_pipe_format_to_mesa_format(enum pipe_format format)
 {
switch (format) {
-   case PIPE_FORMAT_ABGR_UNORM:
+   case PIPE_FORMAT_A8B8G8R8_UNORM:
   return MESA_FORMAT_A8B8G8R8_UNORM;
-   case PIPE_FORMAT_RGBA_UNORM:
+   case PIPE_FORMAT_R8G8B8A8_UNORM:
   return MESA_FORMAT_R8G8B8A8_UNORM;
-   case PIPE_FORMAT_BGRA_UNORM:
+   case PIPE_FORMAT_B8G8R8A8_UNORM:
   return MESA_FORMAT_B8G8R8A8_UNORM;
-   case PIPE_FORMAT_ARGB_UNORM:
+   case PIPE_FORMAT_A8R8G8B8_UNORM:
   return MESA_FORMAT_A8R8G8B8_UNORM;
-   case PIPE_FORMAT_XBGR_UNORM:
+   case PIPE_FORMAT_X8B8G8R8_UNORM:
   return MESA_FORMAT_X8B8G8R8_UNORM;
-   case PIPE_FORMAT_RGBX_UNORM:
+   case PIPE_FORMAT_R8G8B8X8_UNORM:
   return MESA_FORMAT_R8G8B8X8_UNORM;
-   cas

Re: [Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Jason Ekstrand]

It's worth noting that this only fixes some of the more obvious
problems.  For the formats that mesa defines as array formats, I think
things are still pretty broken.

This also brings into question how "worth it" it is having different
format enums.  I personally prefer the way that mesa core has them
defined as I find it much simpler to understand.  However, changing
either is probably going to be a buggy mess. :-(

--Jason

On Sat, Aug 8, 2015 at 1:45 PM, Jason Ekstrand  wrote:
> Mesa formats and gallium formats are defined a bit differently.  In mesa
> there are "packed" formats which are based on byte-order within a 8, 16, or
> 32-bit word and there are "array" formats which are simply an array of 8,
> 16, or 32-bit values.  In gallium, they do something different called
> "plain" which I've never been able to fully understand.  However, for those
> formats which mesa defines as "packed", they should match up 1-1 with the
> corresponding "plane" gallium format.
>
> Unfortunately, st_format.c was using the endian-dependent wrappers such as
> PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
> MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
> big-endian systems, it breaks because it introduces one extra byte-swap.
>
> Cc: Brian Paul 
> Cc: Oded Gabbay 
> ---
>  src/mesa/state_tracker/st_format.c | 94 
> +++---
>  1 file changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_format.c 
> b/src/mesa/state_tracker/st_format.c
> index db7b5b7..f946f24 100644
> --- a/src/mesa/state_tracker/st_format.c
> +++ b/src/mesa/state_tracker/st_format.c
> @@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
>  {
> switch (mesaFormat) {
> case MESA_FORMAT_A8B8G8R8_UNORM:
> -  return PIPE_FORMAT_ABGR_UNORM;
> +  return PIPE_FORMAT_A8B8G8R8_UNORM;
> case MESA_FORMAT_R8G8B8A8_UNORM:
> -  return PIPE_FORMAT_RGBA_UNORM;
> +  return PIPE_FORMAT_R8G8B8A8_UNORM;
> case MESA_FORMAT_B8G8R8A8_UNORM:
> -  return PIPE_FORMAT_BGRA_UNORM;
> +  return PIPE_FORMAT_B8G8R8A8_UNORM;
> case MESA_FORMAT_A8R8G8B8_UNORM:
> -  return PIPE_FORMAT_ARGB_UNORM;
> +  return PIPE_FORMAT_A8R8G8B8_UNORM;
> case MESA_FORMAT_X8B8G8R8_UNORM:
> -  return PIPE_FORMAT_XBGR_UNORM;
> +  return PIPE_FORMAT_X8B8G8R8_UNORM;
> case MESA_FORMAT_R8G8B8X8_UNORM:
> -  return PIPE_FORMAT_RGBX_UNORM;
> +  return PIPE_FORMAT_R8G8B8X8_UNORM;
> case MESA_FORMAT_B8G8R8X8_UNORM:
> -  return PIPE_FORMAT_BGRX_UNORM;
> +  return PIPE_FORMAT_B8G8R8X8_UNORM;
> case MESA_FORMAT_X8R8G8B8_UNORM:
> -  return PIPE_FORMAT_XRGB_UNORM;
> +  return PIPE_FORMAT_X8R8G8B8_UNORM;
> case MESA_FORMAT_B5G5R5A1_UNORM:
>return PIPE_FORMAT_B5G5R5A1_UNORM;
> case MESA_FORMAT_B4G4R4A4_UNORM:
> @@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
> case MESA_FORMAT_BGR_SRGB8:
>return PIPE_FORMAT_R8G8B8_SRGB;
> case MESA_FORMAT_A8B8G8R8_SRGB:
> -  return PIPE_FORMAT_ABGR_SRGB;
> +  return PIPE_FORMAT_A8B8G8R8_SRGB;
> case MESA_FORMAT_R8G8B8A8_SRGB:
> -  return PIPE_FORMAT_RGBA_SRGB;
> +  return PIPE_FORMAT_R8G8B8A8_SRGB;
> case MESA_FORMAT_B8G8R8A8_SRGB:
> -  return PIPE_FORMAT_BGRA_SRGB;
> +  return PIPE_FORMAT_B8G8R8A8_SRGB;
> case MESA_FORMAT_A8R8G8B8_SRGB:
> -  return PIPE_FORMAT_ARGB_SRGB;
> +  return PIPE_FORMAT_A8R8G8B8_SRGB;
> case MESA_FORMAT_RGBA_FLOAT32:
>return PIPE_FORMAT_R32G32B32A32_FLOAT;
> case MESA_FORMAT_RGBA_FLOAT16:
> @@ -199,13 +199,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
> case MESA_FORMAT_R_UNORM16:
>return PIPE_FORMAT_R16_UNORM;
> case MESA_FORMAT_R8G8_UNORM:
> -  return PIPE_FORMAT_RG88_UNORM;
> +  return PIPE_FORMAT_R8G8_UNORM;
> case MESA_FORMAT_G8R8_UNORM:
> -  return PIPE_FORMAT_GR88_UNORM;
> +  return PIPE_FORMAT_G8R8_UNORM;
> case MESA_FORMAT_R16G16_UNORM:
> -  return PIPE_FORMAT_RG1616_UNORM;
> +  return PIPE_FORMAT_R16G16_UNORM;
> case MESA_FORMAT_G16R16_UNORM:
> -  return PIPE_FORMAT_GR1616_UNORM;
> +  return PIPE_FORMAT_G16R16_UNORM;
> case MESA_FORMAT_RGBA_UNORM16:
>return PIPE_FORMAT_R16G16B16A16_UNORM;
>
> @@ -357,7 +357,7 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
> case MESA_FORMAT_G8R8_SNORM:
>return PIPE_FORMAT_GR88_SNORM;
> case MESA_FORMAT_R8G8B8A8_SNORM:
> -  return PIPE_FORMAT_RGBA_SNORM;
> +  return PIPE_FORMAT_R8G8B8A8_SNORM;
> case MESA_FORMAT_A8B8G8R8_SNORM:
>return PIPE_FORMAT_ABGR_SNORM;
>
> @@ -476,21 +476,21 @@ mesa_format
>  st_pipe_format_to_mesa_format(enum pipe_format format)
>  {
> switch (format) {
> -   c

Re: [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

[Rob Clark]

On Sat, Aug 8, 2015 at 4:26 PM, Jason Ekstrand  wrote:
> On Sat, Aug 8, 2015 at 1:01 PM, Oded Gabbay  wrote:
>> On Sat, Aug 8, 2015 at 7:34 PM, Jason Ekstrand  wrote:
>>> On Fri, Aug 7, 2015 at 10:38 PM, Oded Gabbay  wrote:
 On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand  
 wrote:
> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay  
> wrote:
>> This patch fixes a bug that is manifested in the read path of mesa when
>> running on big-endian machines. The effects can be seen when running
>> piglit sanity test and/or taking a screen capture.
>
> piglit sanity isn't all that convincing.  It's quite possible that
> there are two byte-swapping bugs that just happen to cancel out.  If
> it fixes huge numbers of piglit tests, that's more convincing.
>

 Well, without it about 65% of piglit tests fails. With this fix, about
 30% fails. I would say that accounts for something...
>>>
>>> Yes, that counts for far more than just sanity.  It would be
>>> instructive to see the piglit diff between 2ec8718d, 8ec6534b2, and
>>> 5038d839b.  Those three patches are sequential and the one in the
>>> middle, uses _mesa_format_convert for texture upload.
>>>
>> The bug is caused when _mesa_format_convert receives src_format as
>> mesa_format, which it thens changes to mesa_array_format. During this
>> change, it checks for endianness and swaps the bytes accordingly.
>> However, because the bytes are _already_ swapped in the memory itself
>> (being written there by llvmpipe/softpipe/sw rast), and src_format
>> value matches the _actual_ contents of the memory, the result of the
>> read is wrong.
>>
>> Therefore, because other layers/functions, such as llvm or
>> read_rgba_pixels() takes care whether we are dealing with big-endian or
>> little-endian, _mesa_format_convert should be endian agnostic to avoid
>> duplicate swapping of bytes.
>
> As far as I know, the core mesa format conversion code has handled
> byte-swapping for a very long time.  it is *not* simply punted to the
> driver.  I'm 100% ready to believe that there is a problem but it's
> probably far more subtle than just removing support for byte-swapping
> in _mesa_format_to_array_format.

 First of all, this bug has been in a very long time as well (I tracked
 it to between 10.4 to 10.5). It has shown up since mesa started to use
 _mesa_format_convert
 From my debugging, I see that llvmpipe (and other) write the data to
 memory in BE format. e.g if the format is RGBA, then the data is
 actually written in AGBR format.
 Now, in _mesa_format_convert you get the src_format as AGBR (already
 swapped). However, because it arrives as format and not array_format,
 _mesa_format_convert needs to build the array_format in
 _mesa_format_to_array_format()

 In _mesa_format_to_array_format, after the array_format is created,
 you swap it if it is BE, so you now have RGBA in array_format
 representing the source. And that is the bug!
>>>
>>> Not quite... MESA_FORMAT formats are specified one of two ways: array
>>> and packed.  Packed formats are specified in terms of low bits and
>>> high bits in the word while array formats are specified in terms of an
>>> array of elements of a certain type.  For the most part, packed is
>>> used for things like 565 which obviously aren't array formats.
>>> However, packed is also used for the normal 8-bit RGBA formats.  Since
>>> they are specified in terms of low vs. high bits in a byte, they *do*
>>> need to be swapped when going to an array format.  There was, indeed a
>>> bug in there: we were swapping even if the MESA_FORMAT was already an
>>> array format.  I just sent out a patch to fix that bug.
>>>
>>> It would also be instructive to know what the actual enums looked like
>>> and what the data that you're refering to looked like.  The enum
>>> values are described in mesa/main/formats.h.
>>>
>>> It's quite possible that, in all the BE cases, we aren't actually
>>> treating the MESA_FORMATs as described above.  If that is the case,
>>> then we should figure out what they do with the formats and either
>>> change the BE cases or redefine the MESA_FORMATs in such a way that it
>>> makes more sense on BE hardware.  However, as it currently stands
>>> _mesa_format_to_array_format (as patched by the one I just sent)
>>> matches the definitions.
>>>
 of course I checked that _mesa_format_to_array_format() is only called
 from _mesa_format_convert so I can safely modify it according to
 _mesa_format_convert usage.

 So I don't think it is "far more subtle" than removing this
 byte-swapping. If you still think so, please elaborate.
>>>
>>> There are many places in the driver where we look at endianness and do
>>> something with formats.  Obviously, something changed with 5038d839b;
>>> we were taking something into account and aren't n

Re: [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

[Jason Ekstrand]

On Sat, Aug 8, 2015 at 2:10 PM, Rob Clark  wrote:
> On Sat, Aug 8, 2015 at 4:26 PM, Jason Ekstrand  wrote:
>> On Sat, Aug 8, 2015 at 1:01 PM, Oded Gabbay  wrote:
>>> On Sat, Aug 8, 2015 at 7:34 PM, Jason Ekstrand  wrote:
 On Fri, Aug 7, 2015 at 10:38 PM, Oded Gabbay  wrote:
> On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand  
> wrote:
>> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay  
>> wrote:
>>> This patch fixes a bug that is manifested in the read path of mesa when
>>> running on big-endian machines. The effects can be seen when running
>>> piglit sanity test and/or taking a screen capture.
>>
>> piglit sanity isn't all that convincing.  It's quite possible that
>> there are two byte-swapping bugs that just happen to cancel out.  If
>> it fixes huge numbers of piglit tests, that's more convincing.
>>
>
> Well, without it about 65% of piglit tests fails. With this fix, about
> 30% fails. I would say that accounts for something...

 Yes, that counts for far more than just sanity.  It would be
 instructive to see the piglit diff between 2ec8718d, 8ec6534b2, and
 5038d839b.  Those three patches are sequential and the one in the
 middle, uses _mesa_format_convert for texture upload.

>>> The bug is caused when _mesa_format_convert receives src_format as
>>> mesa_format, which it thens changes to mesa_array_format. During this
>>> change, it checks for endianness and swaps the bytes accordingly.
>>> However, because the bytes are _already_ swapped in the memory itself
>>> (being written there by llvmpipe/softpipe/sw rast), and src_format
>>> value matches the _actual_ contents of the memory, the result of the
>>> read is wrong.
>>>
>>> Therefore, because other layers/functions, such as llvm or
>>> read_rgba_pixels() takes care whether we are dealing with big-endian or
>>> little-endian, _mesa_format_convert should be endian agnostic to avoid
>>> duplicate swapping of bytes.
>>
>> As far as I know, the core mesa format conversion code has handled
>> byte-swapping for a very long time.  it is *not* simply punted to the
>> driver.  I'm 100% ready to believe that there is a problem but it's
>> probably far more subtle than just removing support for byte-swapping
>> in _mesa_format_to_array_format.
>
> First of all, this bug has been in a very long time as well (I tracked
> it to between 10.4 to 10.5). It has shown up since mesa started to use
> _mesa_format_convert
> From my debugging, I see that llvmpipe (and other) write the data to
> memory in BE format. e.g if the format is RGBA, then the data is
> actually written in AGBR format.
> Now, in _mesa_format_convert you get the src_format as AGBR (already
> swapped). However, because it arrives as format and not array_format,
> _mesa_format_convert needs to build the array_format in
> _mesa_format_to_array_format()
>
> In _mesa_format_to_array_format, after the array_format is created,
> you swap it if it is BE, so you now have RGBA in array_format
> representing the source. And that is the bug!

 Not quite... MESA_FORMAT formats are specified one of two ways: array
 and packed.  Packed formats are specified in terms of low bits and
 high bits in the word while array formats are specified in terms of an
 array of elements of a certain type.  For the most part, packed is
 used for things like 565 which obviously aren't array formats.
 However, packed is also used for the normal 8-bit RGBA formats.  Since
 they are specified in terms of low vs. high bits in a byte, they *do*
 need to be swapped when going to an array format.  There was, indeed a
 bug in there: we were swapping even if the MESA_FORMAT was already an
 array format.  I just sent out a patch to fix that bug.

 It would also be instructive to know what the actual enums looked like
 and what the data that you're refering to looked like.  The enum
 values are described in mesa/main/formats.h.

 It's quite possible that, in all the BE cases, we aren't actually
 treating the MESA_FORMATs as described above.  If that is the case,
 then we should figure out what they do with the formats and either
 change the BE cases or redefine the MESA_FORMATs in such a way that it
 makes more sense on BE hardware.  However, as it currently stands
 _mesa_format_to_array_format (as patched by the one I just sent)
 matches the definitions.

> of course I checked that _mesa_format_to_array_format() is only called
> from _mesa_format_convert so I can safely modify it according to
> _mesa_format_convert usage.
>
> So I don't think it is "far more subtle" than removing this
> byte-swapping. If you still think so, please elaborate.

 There are many places in the driver where we look at endia

Re: [Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Roland Scheidegger]

Am 08.08.2015 um 22:45 schrieb Jason Ekstrand:
> Mesa formats and gallium formats are defined a bit differently.  In mesa
> there are "packed" formats which are based on byte-order within a 8, 16, or
> 32-bit word and there are "array" formats which are simply an array of 8,
> 16, or 32-bit values.  In gallium, they do something different called
> "plain" which I've never been able to fully understand.  However, for those
> formats which mesa defines as "packed", they should match up 1-1 with the
> corresponding "plane" gallium format.
"plain" I guess.
Though plain in gallium really just means the layout and components can
be defined in generic terms.

FWIW I think gallium's definition of array formats makes more sense. If
all channels are the same width and the channel width is divisible by 8,
then it is an array format. That is kinda obvious, you could easily
address that in c (with byte, short or int members) as an array. The
auto-generated format handling code obviously uses this - if you define
such formats as packed instead the code would fetch all elements at once
and shuffle things into place.
I have no idea what array format means in core mesa, why exactly isn't
the standard rgba8 stuff not an array format? Maybe that comes from some
OpenGL oddities (the UNSIGNED_INT_8_8_8_8 vs. UNSIGNED_INT_8_8_8_8_REV
stuff is always fun stuff, and one of them will match UNSIGNED_BYTE on
big endian the other on little endian...) which gallium ultimately
doesn't care much about.

(As for the patch I can't review that, thinking about endianness issues
just gives me a headache.)

Roland



> 
> Unfortunately, st_format.c was using the endian-dependent wrappers such as
> PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
> MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
> big-endian systems, it breaks because it introduces one extra byte-swap.
> 
> Cc: Brian Paul 
> Cc: Oded Gabbay 
> ---
>  src/mesa/state_tracker/st_format.c | 94 
> +++---
>  1 file changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/src/mesa/state_tracker/st_format.c 
> b/src/mesa/state_tracker/st_format.c
> index db7b5b7..f946f24 100644
> --- a/src/mesa/state_tracker/st_format.c
> +++ b/src/mesa/state_tracker/st_format.c
> @@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
>  {
> switch (mesaFormat) {
> case MESA_FORMAT_A8B8G8R8_UNORM:
> -  return PIPE_FORMAT_ABGR_UNORM;
> +  return PIPE_FORMAT_A8B8G8R8_UNORM;
> case MESA_FORMAT_R8G8B8A8_UNORM:
> -  return PIPE_FORMAT_RGBA_UNORM;
> +  return PIPE_FORMAT_R8G8B8A8_UNORM;
> case MESA_FORMAT_B8G8R8A8_UNORM:
> -  return PIPE_FORMAT_BGRA_UNORM;
> +  return PIPE_FORMAT_B8G8R8A8_UNORM;
> case MESA_FORMAT_A8R8G8B8_UNORM:
> -  return PIPE_FORMAT_ARGB_UNORM;
> +  return PIPE_FORMAT_A8R8G8B8_UNORM;
> case MESA_FORMAT_X8B8G8R8_UNORM:
> -  return PIPE_FORMAT_XBGR_UNORM;
> +  return PIPE_FORMAT_X8B8G8R8_UNORM;
> case MESA_FORMAT_R8G8B8X8_UNORM:
> -  return PIPE_FORMAT_RGBX_UNORM;
> +  return PIPE_FORMAT_R8G8B8X8_UNORM;
> case MESA_FORMAT_B8G8R8X8_UNORM:
> -  return PIPE_FORMAT_BGRX_UNORM;
> +  return PIPE_FORMAT_B8G8R8X8_UNORM;
> case MESA_FORMAT_X8R8G8B8_UNORM:
> -  return PIPE_FORMAT_XRGB_UNORM;
> +  return PIPE_FORMAT_X8R8G8B8_UNORM;
> case MESA_FORMAT_B5G5R5A1_UNORM:
>return PIPE_FORMAT_B5G5R5A1_UNORM;
> case MESA_FORMAT_B4G4R4A4_UNORM:
> @@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
> case MESA_FORMAT_BGR_SRGB8:
>return PIPE_FORMAT_R8G8B8_SRGB;
> case MESA_FORMAT_A8B8G8R8_SRGB:
> -  return PIPE_FORMAT_ABGR_SRGB;
> +  return PIPE_FORMAT_A8B8G8R8_SRGB;
> case MESA_FORMAT_R8G8B8A8_SRGB:
> -  return PIPE_FORMAT_RGBA_SRGB;
> +  return PIPE_FORMAT_R8G8B8A8_SRGB;
> case MESA_FORMAT_B8G8R8A8_SRGB:
> -  return PIPE_FORMAT_BGRA_SRGB;
> +  return PIPE_FORMAT_B8G8R8A8_SRGB;
> case MESA_FORMAT_A8R8G8B8_SRGB:
> -  return PIPE_FORMAT_ARGB_SRGB;
> +  return PIPE_FORMAT_A8R8G8B8_SRGB;
> case MESA_FORMAT_RGBA_FLOAT32:
>return PIPE_FORMAT_R32G32B32A32_FLOAT;
> case MESA_FORMAT_RGBA_FLOAT16:
> @@ -199,13 +199,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
> mesa_format mesaFormat)
> case MESA_FORMAT_R_UNORM16:
>return PIPE_FORMAT_R16_UNORM;
> case MESA_FORMAT_R8G8_UNORM:
> -  return PIPE_FORMAT_RG88_UNORM;
> +  return PIPE_FORMAT_R8G8_UNORM;
> case MESA_FORMAT_G8R8_UNORM:
> -  return PIPE_FORMAT_GR88_UNORM;
> +  return PIPE_FORMAT_G8R8_UNORM;
> case MESA_FORMAT_R16G16_UNORM:
> -  return PIPE_FORMAT_RG1616_UNORM;
> +  return PIPE_FORMAT_R16G16_UNORM;
> case MESA_FORMAT_G16R16_UNORM:
> -  return PIPE_FORMAT_GR1616_UNORM;
> +  return PI

Re: [Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Jason Ekstrand]

On Sat, Aug 8, 2015 at 2:14 PM, Roland Scheidegger  wrote:
> Am 08.08.2015 um 22:45 schrieb Jason Ekstrand:
>> Mesa formats and gallium formats are defined a bit differently.  In mesa
>> there are "packed" formats which are based on byte-order within a 8, 16, or
>> 32-bit word and there are "array" formats which are simply an array of 8,
>> 16, or 32-bit values.  In gallium, they do something different called
>> "plain" which I've never been able to fully understand.  However, for those
>> formats which mesa defines as "packed", they should match up 1-1 with the
>> corresponding "plane" gallium format.
> "plain" I guess.
> Though plain in gallium really just means the layout and components can
> be defined in generic terms.
>
> FWIW I think gallium's definition of array formats makes more sense. If
> all channels are the same width and the channel width is divisible by 8,
> then it is an array format. That is kinda obvious, you could easily
> address that in c (with byte, short or int members) as an array. The

Right.  mesa is more explicit;  It's either an array format or a packed format.

Are you saying that the gallium A8R8G8B8 format is an array format
with the same array-based channel ordering on both LE and BE?  If so
then I have, once again, failed to understand gallium formats and this
patch is bogus.

Just to be 100% clear MESA_FORMAT_R8G8B8A8 is defined as ((A << 24) |
(B << 16) | (G << 8) | (R << 0)).

> auto-generated format handling code obviously uses this - if you define
> such formats as packed instead the code would fetch all elements at once
> and shuffle things into place.
> I have no idea what array format means in core mesa, why exactly isn't
> the standard rgba8 stuff not an array format? Maybe that comes from some

Good question.  It certainly could be done either way for -style
things.  To be honest, I don't really know.  Probably some history in
there.

> OpenGL oddities (the UNSIGNED_INT_8_8_8_8 vs. UNSIGNED_INT_8_8_8_8_REV
> stuff is always fun stuff, and one of them will match UNSIGNED_BYTE on
> big endian the other on little endian...) which gallium ultimately
> doesn't care much about.
>
> (As for the patch I can't review that, thinking about endianness issues
> just gives me a headache.)

Me too.

> Roland
>
>
>
>>
>> Unfortunately, st_format.c was using the endian-dependent wrappers such as
>> PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
>> MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
>> big-endian systems, it breaks because it introduces one extra byte-swap.
>>
>> Cc: Brian Paul 
>> Cc: Oded Gabbay 
>> ---
>>  src/mesa/state_tracker/st_format.c | 94 
>> +++---
>>  1 file changed, 47 insertions(+), 47 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_format.c 
>> b/src/mesa/state_tracker/st_format.c
>> index db7b5b7..f946f24 100644
>> --- a/src/mesa/state_tracker/st_format.c
>> +++ b/src/mesa/state_tracker/st_format.c
>> @@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
>> mesa_format mesaFormat)
>>  {
>> switch (mesaFormat) {
>> case MESA_FORMAT_A8B8G8R8_UNORM:
>> -  return PIPE_FORMAT_ABGR_UNORM;
>> +  return PIPE_FORMAT_A8B8G8R8_UNORM;
>> case MESA_FORMAT_R8G8B8A8_UNORM:
>> -  return PIPE_FORMAT_RGBA_UNORM;
>> +  return PIPE_FORMAT_R8G8B8A8_UNORM;
>> case MESA_FORMAT_B8G8R8A8_UNORM:
>> -  return PIPE_FORMAT_BGRA_UNORM;
>> +  return PIPE_FORMAT_B8G8R8A8_UNORM;
>> case MESA_FORMAT_A8R8G8B8_UNORM:
>> -  return PIPE_FORMAT_ARGB_UNORM;
>> +  return PIPE_FORMAT_A8R8G8B8_UNORM;
>> case MESA_FORMAT_X8B8G8R8_UNORM:
>> -  return PIPE_FORMAT_XBGR_UNORM;
>> +  return PIPE_FORMAT_X8B8G8R8_UNORM;
>> case MESA_FORMAT_R8G8B8X8_UNORM:
>> -  return PIPE_FORMAT_RGBX_UNORM;
>> +  return PIPE_FORMAT_R8G8B8X8_UNORM;
>> case MESA_FORMAT_B8G8R8X8_UNORM:
>> -  return PIPE_FORMAT_BGRX_UNORM;
>> +  return PIPE_FORMAT_B8G8R8X8_UNORM;
>> case MESA_FORMAT_X8R8G8B8_UNORM:
>> -  return PIPE_FORMAT_XRGB_UNORM;
>> +  return PIPE_FORMAT_X8R8G8B8_UNORM;
>> case MESA_FORMAT_B5G5R5A1_UNORM:
>>return PIPE_FORMAT_B5G5R5A1_UNORM;
>> case MESA_FORMAT_B4G4R4A4_UNORM:
>> @@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
>> mesa_format mesaFormat)
>> case MESA_FORMAT_BGR_SRGB8:
>>return PIPE_FORMAT_R8G8B8_SRGB;
>> case MESA_FORMAT_A8B8G8R8_SRGB:
>> -  return PIPE_FORMAT_ABGR_SRGB;
>> +  return PIPE_FORMAT_A8B8G8R8_SRGB;
>> case MESA_FORMAT_R8G8B8A8_SRGB:
>> -  return PIPE_FORMAT_RGBA_SRGB;
>> +  return PIPE_FORMAT_R8G8B8A8_SRGB;
>> case MESA_FORMAT_B8G8R8A8_SRGB:
>> -  return PIPE_FORMAT_BGRA_SRGB;
>> +  return PIPE_FORMAT_B8G8R8A8_SRGB;
>> case MESA_FORMAT_A8R8G8B8_SRGB:
>> -  return PIPE_FORMAT_ARGB_SRGB;
>> +  return PIPE_FORMAT_A8R8G8B8_SRGB;
>>

Re: [Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Roland Scheidegger]

Am 08.08.2015 um 23:26 schrieb Jason Ekstrand:
> On Sat, Aug 8, 2015 at 2:14 PM, Roland Scheidegger  wrote:
>> Am 08.08.2015 um 22:45 schrieb Jason Ekstrand:
>>> Mesa formats and gallium formats are defined a bit differently.  In mesa
>>> there are "packed" formats which are based on byte-order within a 8, 16, or
>>> 32-bit word and there are "array" formats which are simply an array of 8,
>>> 16, or 32-bit values.  In gallium, they do something different called
>>> "plain" which I've never been able to fully understand.  However, for those
>>> formats which mesa defines as "packed", they should match up 1-1 with the
>>> corresponding "plane" gallium format.
>> "plain" I guess.
>> Though plain in gallium really just means the layout and components can
>> be defined in generic terms.
>>
>> FWIW I think gallium's definition of array formats makes more sense. If
>> all channels are the same width and the channel width is divisible by 8,
>> then it is an array format. That is kinda obvious, you could easily
>> address that in c (with byte, short or int members) as an array. The
> 
> Right.  mesa is more explicit;  It's either an array format or a packed 
> format.
> 
> Are you saying that the gallium A8R8G8B8 format is an array format
> with the same array-based channel ordering on both LE and BE?  If so
> then I have, once again, failed to understand gallium formats and this
> patch is bogus.

I'm not saying anything on that matter :-).

> Just to be 100% clear MESA_FORMAT_R8G8B8A8 is defined as ((A << 24) |
> (B << 16) | (G << 8) | (R << 0)).

The gallium docs though disagree with that (see format.rst).

Roland

> 
>> auto-generated format handling code obviously uses this - if you define
>> such formats as packed instead the code would fetch all elements at once
>> and shuffle things into place.
>> I have no idea what array format means in core mesa, why exactly isn't
>> the standard rgba8 stuff not an array format? Maybe that comes from some
> 
> Good question.  It certainly could be done either way for -style
> things.  To be honest, I don't really know.  Probably some history in
> there.
> 
>> OpenGL oddities (the UNSIGNED_INT_8_8_8_8 vs. UNSIGNED_INT_8_8_8_8_REV
>> stuff is always fun stuff, and one of them will match UNSIGNED_BYTE on
>> big endian the other on little endian...) which gallium ultimately
>> doesn't care much about.
>>
>> (As for the patch I can't review that, thinking about endianness issues
>> just gives me a headache.)
> 
> Me too.
> 
>> Roland
>>
>>
>>
>>>
>>> Unfortunately, st_format.c was using the endian-dependent wrappers such as
>>> PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
>>> MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
>>> big-endian systems, it breaks because it introduces one extra byte-swap.
>>>
>>> Cc: Brian Paul 
>>> Cc: Oded Gabbay 
>>> ---
>>>  src/mesa/state_tracker/st_format.c | 94 
>>> +++---
>>>  1 file changed, 47 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_format.c 
>>> b/src/mesa/state_tracker/st_format.c
>>> index db7b5b7..f946f24 100644
>>> --- a/src/mesa/state_tracker/st_format.c
>>> +++ b/src/mesa/state_tracker/st_format.c
>>> @@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
>>> mesa_format mesaFormat)
>>>  {
>>> switch (mesaFormat) {
>>> case MESA_FORMAT_A8B8G8R8_UNORM:
>>> -  return PIPE_FORMAT_ABGR_UNORM;
>>> +  return PIPE_FORMAT_A8B8G8R8_UNORM;
>>> case MESA_FORMAT_R8G8B8A8_UNORM:
>>> -  return PIPE_FORMAT_RGBA_UNORM;
>>> +  return PIPE_FORMAT_R8G8B8A8_UNORM;
>>> case MESA_FORMAT_B8G8R8A8_UNORM:
>>> -  return PIPE_FORMAT_BGRA_UNORM;
>>> +  return PIPE_FORMAT_B8G8R8A8_UNORM;
>>> case MESA_FORMAT_A8R8G8B8_UNORM:
>>> -  return PIPE_FORMAT_ARGB_UNORM;
>>> +  return PIPE_FORMAT_A8R8G8B8_UNORM;
>>> case MESA_FORMAT_X8B8G8R8_UNORM:
>>> -  return PIPE_FORMAT_XBGR_UNORM;
>>> +  return PIPE_FORMAT_X8B8G8R8_UNORM;
>>> case MESA_FORMAT_R8G8B8X8_UNORM:
>>> -  return PIPE_FORMAT_RGBX_UNORM;
>>> +  return PIPE_FORMAT_R8G8B8X8_UNORM;
>>> case MESA_FORMAT_B8G8R8X8_UNORM:
>>> -  return PIPE_FORMAT_BGRX_UNORM;
>>> +  return PIPE_FORMAT_B8G8R8X8_UNORM;
>>> case MESA_FORMAT_X8R8G8B8_UNORM:
>>> -  return PIPE_FORMAT_XRGB_UNORM;
>>> +  return PIPE_FORMAT_X8R8G8B8_UNORM;
>>> case MESA_FORMAT_B5G5R5A1_UNORM:
>>>return PIPE_FORMAT_B5G5R5A1_UNORM;
>>> case MESA_FORMAT_B4G4R4A4_UNORM:
>>> @@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
>>> mesa_format mesaFormat)
>>> case MESA_FORMAT_BGR_SRGB8:
>>>return PIPE_FORMAT_R8G8B8_SRGB;
>>> case MESA_FORMAT_A8B8G8R8_SRGB:
>>> -  return PIPE_FORMAT_ABGR_SRGB;
>>> +  return PIPE_FORMAT_A8B8G8R8_SRGB;
>>> case MESA_FORMAT_R8G8B8A8_SRGB:
>>> -  return PIPE_FORMAT_RGBA_SRGB;
>>> + 

Re: [Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Oded Gabbay]

On Sun, Aug 9, 2015 at 12:26 AM, Jason Ekstrand  wrote:
> On Sat, Aug 8, 2015 at 2:14 PM, Roland Scheidegger  wrote:
>> Am 08.08.2015 um 22:45 schrieb Jason Ekstrand:
>>> Mesa formats and gallium formats are defined a bit differently.  In mesa
>>> there are "packed" formats which are based on byte-order within a 8, 16, or
>>> 32-bit word and there are "array" formats which are simply an array of 8,
>>> 16, or 32-bit values.  In gallium, they do something different called
>>> "plain" which I've never been able to fully understand.  However, for those
>>> formats which mesa defines as "packed", they should match up 1-1 with the
>>> corresponding "plane" gallium format.
>> "plain" I guess.
>> Though plain in gallium really just means the layout and components can
>> be defined in generic terms.
>>
>> FWIW I think gallium's definition of array formats makes more sense. If
>> all channels are the same width and the channel width is divisible by 8,
>> then it is an array format. That is kinda obvious, you could easily
>> address that in c (with byte, short or int members) as an array. The
>
> Right.  mesa is more explicit;  It's either an array format or a packed 
> format.
>
> Are you saying that the gallium A8R8G8B8 format is an array format
> with the same array-based channel ordering on both LE and BE?  If so
> then I have, once again, failed to understand gallium formats and this
> patch is bogus.
>
> Just to be 100% clear MESA_FORMAT_R8G8B8A8 is defined as ((A << 24) |
> (B << 16) | (G << 8) | (R << 0)).
>
>> auto-generated format handling code obviously uses this - if you define
>> such formats as packed instead the code would fetch all elements at once
>> and shuffle things into place.
>> I have no idea what array format means in core mesa, why exactly isn't
>> the standard rgba8 stuff not an array format? Maybe that comes from some
>
> Good question.  It certainly could be done either way for -style
> things.  To be honest, I don't really know.  Probably some history in
> there.
>
>> OpenGL oddities (the UNSIGNED_INT_8_8_8_8 vs. UNSIGNED_INT_8_8_8_8_REV
>> stuff is always fun stuff, and one of them will match UNSIGNED_BYTE on
>> big endian the other on little endian...) which gallium ultimately
>> doesn't care much about.
>>
>> (As for the patch I can't review that, thinking about endianness issues
>> just gives me a headache.)
>
> Me too.
>
Same here but I don't have a choice as its my job ;)

>> Roland
>>
>>
>>
>>>
>>> Unfortunately, st_format.c was using the endian-dependent wrappers such as
>>> PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
>>> MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
>>> big-endian systems, it breaks because it introduces one extra byte-swap.
>>>
>>> Cc: Brian Paul 
>>> Cc: Oded Gabbay 
>>> ---
>>>  src/mesa/state_tracker/st_format.c | 94 
>>> +++---
>>>  1 file changed, 47 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_format.c 
>>> b/src/mesa/state_tracker/st_format.c
>>> index db7b5b7..f946f24 100644
>>> --- a/src/mesa/state_tracker/st_format.c
>>> +++ b/src/mesa/state_tracker/st_format.c
>>> @@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
>>> mesa_format mesaFormat)
>>>  {
>>> switch (mesaFormat) {
>>> case MESA_FORMAT_A8B8G8R8_UNORM:
>>> -  return PIPE_FORMAT_ABGR_UNORM;
>>> +  return PIPE_FORMAT_A8B8G8R8_UNORM;
>>> case MESA_FORMAT_R8G8B8A8_UNORM:
>>> -  return PIPE_FORMAT_RGBA_UNORM;
>>> +  return PIPE_FORMAT_R8G8B8A8_UNORM;
>>> case MESA_FORMAT_B8G8R8A8_UNORM:
>>> -  return PIPE_FORMAT_BGRA_UNORM;
>>> +  return PIPE_FORMAT_B8G8R8A8_UNORM;
>>> case MESA_FORMAT_A8R8G8B8_UNORM:
>>> -  return PIPE_FORMAT_ARGB_UNORM;
>>> +  return PIPE_FORMAT_A8R8G8B8_UNORM;
>>> case MESA_FORMAT_X8B8G8R8_UNORM:
>>> -  return PIPE_FORMAT_XBGR_UNORM;
>>> +  return PIPE_FORMAT_X8B8G8R8_UNORM;
>>> case MESA_FORMAT_R8G8B8X8_UNORM:
>>> -  return PIPE_FORMAT_RGBX_UNORM;
>>> +  return PIPE_FORMAT_R8G8B8X8_UNORM;
>>> case MESA_FORMAT_B8G8R8X8_UNORM:
>>> -  return PIPE_FORMAT_BGRX_UNORM;
>>> +  return PIPE_FORMAT_B8G8R8X8_UNORM;
>>> case MESA_FORMAT_X8R8G8B8_UNORM:
>>> -  return PIPE_FORMAT_XRGB_UNORM;
>>> +  return PIPE_FORMAT_X8R8G8B8_UNORM;
>>> case MESA_FORMAT_B5G5R5A1_UNORM:
>>>return PIPE_FORMAT_B5G5R5A1_UNORM;
>>> case MESA_FORMAT_B4G4R4A4_UNORM:
>>> @@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
>>> mesa_format mesaFormat)
>>> case MESA_FORMAT_BGR_SRGB8:
>>>return PIPE_FORMAT_R8G8B8_SRGB;
>>> case MESA_FORMAT_A8B8G8R8_SRGB:
>>> -  return PIPE_FORMAT_ABGR_SRGB;
>>> +  return PIPE_FORMAT_A8B8G8R8_SRGB;
>>> case MESA_FORMAT_R8G8B8A8_SRGB:
>>> -  return PIPE_FORMAT_RGBA_SRGB;
>>> +  return PIPE_FORMAT_R8G8B8A8_SRGB;
>>> case MESA_FORMAT

Re: [Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Jason Ekstrand]

On Sat, Aug 8, 2015 at 3:14 PM, Oded Gabbay  wrote:
> On Sun, Aug 9, 2015 at 12:26 AM, Jason Ekstrand  wrote:
>> On Sat, Aug 8, 2015 at 2:14 PM, Roland Scheidegger  
>> wrote:
>>> Am 08.08.2015 um 22:45 schrieb Jason Ekstrand:
 Mesa formats and gallium formats are defined a bit differently.  In mesa
 there are "packed" formats which are based on byte-order within a 8, 16, or
 32-bit word and there are "array" formats which are simply an array of 8,
 16, or 32-bit values.  In gallium, they do something different called
 "plain" which I've never been able to fully understand.  However, for those
 formats which mesa defines as "packed", they should match up 1-1 with the
 corresponding "plane" gallium format.
>>> "plain" I guess.
>>> Though plain in gallium really just means the layout and components can
>>> be defined in generic terms.
>>>
>>> FWIW I think gallium's definition of array formats makes more sense. If
>>> all channels are the same width and the channel width is divisible by 8,
>>> then it is an array format. That is kinda obvious, you could easily
>>> address that in c (with byte, short or int members) as an array. The
>>
>> Right.  mesa is more explicit;  It's either an array format or a packed 
>> format.
>>
>> Are you saying that the gallium A8R8G8B8 format is an array format
>> with the same array-based channel ordering on both LE and BE?  If so
>> then I have, once again, failed to understand gallium formats and this
>> patch is bogus.
>>
>> Just to be 100% clear MESA_FORMAT_R8G8B8A8 is defined as ((A << 24) |
>> (B << 16) | (G << 8) | (R << 0)).
>>
>>> auto-generated format handling code obviously uses this - if you define
>>> such formats as packed instead the code would fetch all elements at once
>>> and shuffle things into place.
>>> I have no idea what array format means in core mesa, why exactly isn't
>>> the standard rgba8 stuff not an array format? Maybe that comes from some
>>
>> Good question.  It certainly could be done either way for -style
>> things.  To be honest, I don't really know.  Probably some history in
>> there.
>>
>>> OpenGL oddities (the UNSIGNED_INT_8_8_8_8 vs. UNSIGNED_INT_8_8_8_8_REV
>>> stuff is always fun stuff, and one of them will match UNSIGNED_BYTE on
>>> big endian the other on little endian...) which gallium ultimately
>>> doesn't care much about.
>>>
>>> (As for the patch I can't review that, thinking about endianness issues
>>> just gives me a headache.)
>>
>> Me too.
>>
> Same here but I don't have a choice as its my job ;)
>
>>> Roland
>>>
>>>
>>>

 Unfortunately, st_format.c was using the endian-dependent wrappers such as
 PIPE_FORMAT_ARGB_ that are defined in p_format.h and mapping them to
 MESA_FORMAT_A8R8G8B8.  On little-endian systems, this is fine but on
 big-endian systems, it breaks because it introduces one extra byte-swap.

 Cc: Brian Paul 
 Cc: Oded Gabbay 
 ---
  src/mesa/state_tracker/st_format.c | 94 
 +++---
  1 file changed, 47 insertions(+), 47 deletions(-)

 diff --git a/src/mesa/state_tracker/st_format.c 
 b/src/mesa/state_tracker/st_format.c
 index db7b5b7..f946f24 100644
 --- a/src/mesa/state_tracker/st_format.c
 +++ b/src/mesa/state_tracker/st_format.c
 @@ -58,21 +58,21 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
 mesa_format mesaFormat)
  {
 switch (mesaFormat) {
 case MESA_FORMAT_A8B8G8R8_UNORM:
 -  return PIPE_FORMAT_ABGR_UNORM;
 +  return PIPE_FORMAT_A8B8G8R8_UNORM;
 case MESA_FORMAT_R8G8B8A8_UNORM:
 -  return PIPE_FORMAT_RGBA_UNORM;
 +  return PIPE_FORMAT_R8G8B8A8_UNORM;
 case MESA_FORMAT_B8G8R8A8_UNORM:
 -  return PIPE_FORMAT_BGRA_UNORM;
 +  return PIPE_FORMAT_B8G8R8A8_UNORM;
 case MESA_FORMAT_A8R8G8B8_UNORM:
 -  return PIPE_FORMAT_ARGB_UNORM;
 +  return PIPE_FORMAT_A8R8G8B8_UNORM;
 case MESA_FORMAT_X8B8G8R8_UNORM:
 -  return PIPE_FORMAT_XBGR_UNORM;
 +  return PIPE_FORMAT_X8B8G8R8_UNORM;
 case MESA_FORMAT_R8G8B8X8_UNORM:
 -  return PIPE_FORMAT_RGBX_UNORM;
 +  return PIPE_FORMAT_R8G8B8X8_UNORM;
 case MESA_FORMAT_B8G8R8X8_UNORM:
 -  return PIPE_FORMAT_BGRX_UNORM;
 +  return PIPE_FORMAT_B8G8R8X8_UNORM;
 case MESA_FORMAT_X8R8G8B8_UNORM:
 -  return PIPE_FORMAT_XRGB_UNORM;
 +  return PIPE_FORMAT_X8R8G8B8_UNORM;
 case MESA_FORMAT_B5G5R5A1_UNORM:
return PIPE_FORMAT_B5G5R5A1_UNORM;
 case MESA_FORMAT_B4G4R4A4_UNORM:
 @@ -154,13 +154,13 @@ st_mesa_format_to_pipe_format(struct st_context *st, 
 mesa_format mesaFormat)
 case MESA_FORMAT_BGR_SRGB8:
return PIPE_FORMAT_R8G8B8_SRGB;
 case MESA_FORMAT_A8B8G8R8_SRGB:
 -  return PIPE_FORMAT_ABGR_SRGB;
 +  return PIPE_FORMAT

Re: [Mesa-dev] [PATCH v4 (part2) 49/59] glsl: Do not allow assignments to read-only variables

[Timothy Arceri]

On Sat, 2015-08-08 at 14:25 +0300, Francisco Jerez wrote:
> Iago Toral  writes:
> 
> > On Wed, 2015-08-05 at 22:22 +1000, Timothy Arceri wrote:
> > > On Wed, 2015-08-05 at 13:45 +0200, Iago Toral wrote:
> > > > On Wed, 2015-08-05 at 20:04 +1000, Timothy Arceri wrote:
> > > > > On Wed, 2015-08-05 at 10:30 +0200, Iago Toral Quiroga wrote:
> > > > > > ---
> > > > > >  src/glsl/ast_to_hir.cpp | 9 -
> > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > > > > > index e834a46..518612d 100644
> > > > > > --- a/src/glsl/ast_to_hir.cpp
> > > > > > +++ b/src/glsl/ast_to_hir.cpp
> > > > > > @@ -811,8 +811,15 @@ do_assignment(exec_list *instructions, struct 
> > > > > > _mesa_glsl_parse_state *state,
> > > > > > }
> > > > > >  
> > > > > > ir_variable *lhs_var = lhs->variable_referenced();
> > > > > > -   if (lhs_var)
> > > > > > +   if (lhs_var) {
> > > > > > +  if (lhs_var->data.image_read_only) {
> > > > > 
> > > > > It looks like data.read_only is always set to true for images so 
> > > > > wouldn't 
> > > > > this
> > > > > already be caught already by the existing read-only check?
> > > > > 
> > > > >   else if (lhs_var != NULL && lhs_var->data.read_only) {
> > > > >  _mesa_glsl_error(&lhs_loc, state,
> > > > >   "assignment to read-only variable '%s'",
> > > > >   lhs_var->name);
> > > > 
> > > > Not as it is now, because with SSBOs we only set image_read_only and 
> > > > not
> > > > read_only when the readonly qualifier is used. I suppose this is what 
> > > > we
> > > > are expected to do since the SSBO spec says that behavior for these
> > > > qualifiers on SSBOs is the same as for images:
> > > > https://www.opengl.org/registry/specs/ARB/shader_storage_buffer_object
> > > > .txt
> > > > 
> > > > "Modify Section 4.10, Memory Qualifiers (p. 71)"
> > > > (...)
> > > > "(insert after third paragraph, p. 73) The memory qualifiers 
> > > > "coherent",
> > > > "volatile", "restrict", "readonly", and "writeonly" may be used in the
> > > > declaration of buffer variables (i.e., members of shader storage 
> > > > blocks).
> > > > When a buffer variable is declared with a memory qualifier, the 
> > > > behavior
> > > > specified for memory accesses involving image variables described 
> > > > above
> > > > applies identically to memory accesses involving that buffer variable.
> > > >   It
> > > > is an error to assign to a buffer variable qualified with "readonly" 
> > > > or to
> > > > read from a buffer variable qualified with "writeonly".
> > > > 
> > > > What is a bit confusing for me is that images seem to set
> > > > image_read_only depending on whether we used the readonly qualifier or
> > > > not (like ssbos) but then they also set read_only to true
> > > > unconditionally, so I guess there is a difference between both fields,
> > > 
> > > Asking what the difference is was originally going to be my first 
> > > question to
> > > you :)
> > > 
> > > > but I don't know what it is exactly, specially since you can also use
> > > > writeonly on images, for example.
> > > 
> > > So I really dont know much about images but after some reading the 
> > > conclusion
> > > I've come to is the qualifiers (image_read_only) are meant to limit how 
> > > you
> > > can use imageStore(), imageLoad() and imageAtomic*() etc.
> > 
> > Looking at ARB_shader_image_load_store that seems consistent...  In that
> > case I imagine that we could just set read_only for buffer variables
> > with the readonly qualifier instead of image_read_only and drop this
> > patch. We will need to add, at least, write_only to ir_variable as well
> > I guess... I imagine that the 3 other fields (image_coherent,
> > image_restrict, image_volatile) do not have image-specific semantics
> > like image_read_only and image_write_oly and can be shared with ssbos
> > we do not have to replicate them in ir_variable as well (in that case we
> > might want to rename them so it is clear that image_read_only and
> > image_write_only really are special and specific to images)
> > 
> > Curro, what do you think?
> > 
> Yes, the image_* fields (which we should definitely rename now that they
> also affect SSBO variables) are just the memory qualifiers defined by
> the GLSL spec, in the case of images the difference between these and
> the original ir_variable::read_only is clear: The former apply to the
> memory pointed to by the variable, while the latter refers to the
> variable itself -- So in principle one could have a fixed image uniform
> (fixed in the sense of always pointing to the same image unit) pointing
> to some write-only image (i.e. only ir_variable::read_only set), or a
> image uniform mutated during the execution of the program pointing to a
> read-only image (i.e. only ir_variable::image_read_only set -- Although
> in practice this latter possibility is disallowed by the spec,

[Mesa-dev] [PATCH] glsl: Add missing spec quote about atomic counter in structs

[Timothy Arceri]

---
 src/glsl/ast_to_hir.cpp | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 9385922..f67c951 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -5679,10 +5679,10 @@ ast_process_structure_or_interface_block(exec_list 
*instructions,
  }
 
  if (field_type->contains_atomic()) {
-/* FINISHME: Add a spec quotation here once updated spec
- * FINISHME: language is available.  See Khronos bug #10903
- * FINISHME: on whether atomic counters are allowed in
- * FINISHME: structures.
+/* From section 4.1.7.3 of the GLSL 4.40 spec:
+ *
+ *"Members of structures cannot be declared as atomic counter"
+ *" types."
  */
 YYLTYPE loc = decl_list->get_location();
 _mesa_glsl_error(&loc, state, "atomic counter in structure, "
-- 
2.4.3

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

[Mesa-dev] [PATCH 3/6] glsl: add AoA support to subroutines

[Timothy Arceri]

---
 src/glsl/ast_function.cpp | 43 ++-
 src/glsl/lower_subroutine.cpp |  2 +-
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
index d003655..42affad 100644
--- a/src/glsl/ast_function.cpp
+++ b/src/glsl/ast_function.cpp
@@ -527,6 +527,37 @@ match_subroutine_by_name(const char *name,
return sig;
 }
 
+static ir_rvalue *
+generate_array_index(void *mem_ctx, exec_list *instructions,
+ struct _mesa_glsl_parse_state *state, YYLTYPE loc,
+ const ast_expression *array, ast_expression *idx,
+ const char **function_name, exec_list *actual_parameters)
+{
+   if (array->oper == ast_array_index) {
+  /* This handles arrays of arrays */
+  ir_rvalue *outer_array = generate_array_index(mem_ctx, instructions,
+state, loc,
+array->subexpressions[0],
+array->subexpressions[1],
+function_name, 
actual_parameters);
+  ir_rvalue *outer_array_idx = idx->hir(instructions, state);
+
+  YYLTYPE index_loc = idx->get_location();
+  return _mesa_ast_array_index_to_hir(mem_ctx, state, outer_array,
+  outer_array_idx, loc,
+  index_loc);
+   } else {
+  ir_variable *sub_var = NULL;
+  *function_name = array->primary_expression.identifier;
+
+  match_subroutine_by_name(*function_name, actual_parameters,
+   state, &sub_var);
+
+  ir_rvalue *outer_array_idx = idx->hir(instructions, state);
+  return new(mem_ctx) ir_dereference_array(sub_var, outer_array_idx);
+   }
+}
+
 static void
 print_function_prototypes(_mesa_glsl_parse_state *state, YYLTYPE *loc,
   ir_function *f)
@@ -1901,16 +1932,18 @@ ast_function_expression::hir(exec_list *instructions,
   ir_variable *sub_var = NULL;
   ir_rvalue *array_idx = NULL;
 
+  process_parameters(instructions, &actual_parameters, &this->expressions,
+state);
+
   if (id->oper == ast_array_index) {
- func_name = id->subexpressions[0]->primary_expression.identifier;
-array_idx = id->subexpressions[1]->hir(instructions, state);
+ array_idx = generate_array_index(ctx, instructions, state, loc,
+  id->subexpressions[0],
+  id->subexpressions[1], &func_name,
+  &actual_parameters);
   } else {
  func_name = id->primary_expression.identifier;
   }
 
-  process_parameters(instructions, &actual_parameters, &this->expressions,
-state);
-
   ir_function_signature *sig =
 match_function_by_name(func_name, &actual_parameters, state);
 
diff --git a/src/glsl/lower_subroutine.cpp b/src/glsl/lower_subroutine.cpp
index b29912a..5bb8ee5 100644
--- a/src/glsl/lower_subroutine.cpp
+++ b/src/glsl/lower_subroutine.cpp
@@ -84,7 +84,7 @@ lower_subroutine_visitor::visit_leave(ir_call *ir)
  continue;
 
   if (ir->array_idx != NULL)
- var = new(mem_ctx) ir_dereference_array(ir->sub_var, 
ir->array_idx->clone(mem_ctx, NULL));
+ var = ir->array_idx->clone(mem_ctx, NULL);
   else
  var = new(mem_ctx) ir_dereference_variable(ir->sub_var);
 
-- 
2.4.3

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

[Mesa-dev] ARB_arrays_of_arrays Desktop series

[Timothy Arceri]

This applies on top of my V3 ARB_arrays_of_arrays GLSL ES series

If useful the series is in the 'desktop_AoA_no_interface' branch of the repo 
here:
https://github.com/tarceri/Mesa_arrays_of_arrays.git

This series:
 - Adds support for input/output AoA on i965
 - AoA subroutine support
 - Enables AoA on i965
 - Disallows interface block AoA on Desktop GL

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

[Mesa-dev] [PATCH 2/6] glsl: disable interface block AoA

[Timothy Arceri]

Desktop GL supports interface block AoA however AMD and Nvidia
dont support it in their drivers curently so we can get away
with disabling it for now.
---
 src/glsl/ast_to_hir.cpp | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 403aa02..9385922 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -6228,15 +6228,17 @@ ast_interface_block::hir(exec_list *instructions,
 }
  }
 
-  /* From section 4.3.9 (Interface Blocks) of the GLSL ES 3.10 spec:
+ /* From section 4.3.9 (Interface Blocks) of the GLSL ES 3.10 spec:
   *
   * * Arrays of arrays of blocks are not allowed
   */
- if (state->es_shader && block_array_type->is_array() &&
+ /* FIXME: Desktop GL allows interface AoA */
+ if (block_array_type->is_array() &&
  block_array_type->fields.array->is_array()) {
 _mesa_glsl_error(&loc, state,
- "arrays of arrays interface blocks are "
- "not allowed");
+ "arrays of arrays interface blocks are %s",
+ state->es_shader ? "not allowed" :
+ "not currently supported");
  }
 
  if (this->layout.flags.q.explicit_binding)
-- 
2.4.3

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

[Mesa-dev] [PATCH 6/6] docs: Mark AoA as done for i965

[Timothy Arceri]

---
 docs/GL3.txt  | 4 ++--
 docs/relnotes/11.0.0.html | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/GL3.txt b/docs/GL3.txt
index 8124383..79c80e0 100644
--- a/docs/GL3.txt
+++ b/docs/GL3.txt
@@ -149,7 +149,7 @@ GL 4.2, GLSL 4.20:
 
 GL 4.3, GLSL 4.30:
 
-  GL_ARB_arrays_of_arrays  started (Timothy)
+  GL_ARB_arrays_of_arrays  DONE (i965)
   GL_ARB_ES3_compatibility DONE (all drivers that 
support GLSL 3.30)
   GL_ARB_clear_buffer_object   DONE (all drivers)
   GL_ARB_compute_shaderin progress (jljusten)
@@ -203,7 +203,7 @@ GL 4.5, GLSL 4.50:
 
 These are the extensions cherry-picked to make GLES 3.1
 GLES3.1, GLSL ES 3.1
-  GL_ARB_arrays_of_arrays  started (Timothy)
+  GL_ARB_arrays_of_arrays  DONE (i965)
   GL_ARB_compute_shaderin progress (jljusten)
   GL_ARB_draw_indirect DONE (i965, nvc0, r600, 
radeonsi, llvmpipe, softpipe)
   GL_ARB_explicit_uniform_location DONE (all drivers that 
support GLSL)
diff --git a/docs/relnotes/11.0.0.html b/docs/relnotes/11.0.0.html
index 1b63331..00a0308 100644
--- a/docs/relnotes/11.0.0.html
+++ b/docs/relnotes/11.0.0.html
@@ -45,6 +45,7 @@ Note: some of the new features are only available with 
certain drivers.
 
 
 GL_AMD_vertex_shader_viewport_index on radeonsi
+GL_ARB_arrays_of_arrays on i965
 GL_ARB_conditional_render_inverted on r600, radeonsi
 GL_ARB_derivative_control on radeonsi
 GL_ARB_fragment_layer_viewport on radeonsi
-- 
2.4.3

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

[Mesa-dev] [PATCH 5/6] i965: enable ARB_arrays_of_arrays

[Timothy Arceri]

---
 src/mesa/drivers/dri/i965/intel_extensions.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c 
b/src/mesa/drivers/dri/i965/intel_extensions.c
index 4a0..5febb95 100644
--- a/src/mesa/drivers/dri/i965/intel_extensions.c
+++ b/src/mesa/drivers/dri/i965/intel_extensions.c
@@ -176,6 +176,7 @@ intelInitExtensions(struct gl_context *ctx)
 
assert(brw->gen >= 4);
 
+   ctx->Extensions.ARB_arrays_of_arrays = true;
ctx->Extensions.ARB_buffer_storage = true;
ctx->Extensions.ARB_clear_texture = true;
ctx->Extensions.ARB_clip_control = true;
-- 
2.4.3

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

[Mesa-dev] [PATCH 4/6] i965: add arrays of arrays support for varyings

[Timothy Arceri]

---
 src/mesa/drivers/dri/i965/brw_fs.cpp | 4 ++--
 src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index ce1edc3..620ce13 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -1053,11 +1053,11 @@ fs_visitor::emit_general_interpolation(fs_reg attr, 
const char *name,
unsigned int array_elements;
 
if (type->is_array()) {
-  array_elements = type->length;
+  array_elements = type->arrays_of_arrays_size();
   if (array_elements == 0) {
  fail("dereferenced array '%s' has length 0\n", name);
   }
-  type = type->fields.array;
+  type = type->without_array();
} else {
   array_elements = 1;
}
diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
index ee964a0..c5e475c 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
@@ -78,7 +78,8 @@ fs_visitor::nir_setup_inputs(nir_shader *shader)
  const unsigned components = t->components();
  const unsigned cols = t->matrix_columns;
  const unsigned elts = t->vector_elements;
- unsigned array_length = var->type->is_array() ? var->type->length : 1;
+ unsigned array_length =
+var->type->is_array() ? var->type->arrays_of_arrays_size() : 1;
  for (unsigned i = 0; i < array_length; i++) {
 for (unsigned j = 0; j < cols; j++) {
for (unsigned k = 0; k < elts; k++) {
-- 
2.4.3

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

[Mesa-dev] [PATCH 1/6] glsl: avoid hitting assert for arrays of arrays

[Timothy Arceri]

Also add TODO comment about adding proper support

Signed-off-by: Timothy Arceri 
---
 src/glsl/ir_set_program_inouts.cpp | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/glsl/ir_set_program_inouts.cpp 
b/src/glsl/ir_set_program_inouts.cpp
index b7a0f6e..d7c29b0 100644
--- a/src/glsl/ir_set_program_inouts.cpp
+++ b/src/glsl/ir_set_program_inouts.cpp
@@ -242,6 +242,12 @@ 
ir_set_program_inouts_visitor::try_mark_partial_variable(ir_variable *var,
   type = type->fields.array;
}
 
+   /* TODO: implement proper arrays of arrays support
+* for now let the caller mark whole variable as used.
+*/
+   if (type->is_array() && type->fields.array->is_array())
+  return false;
+
/* The code below only handles:
 *
 * - Indexing into matrices
-- 
2.4.3

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

[Mesa-dev] [PATCH] glsl: remove specical case subroutine type counting

[Timothy Arceri]

Unlike samplers we can get the correct value for subroutines from
component_slots()
---
 src/glsl/link_uniforms.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
index 4757fd5..3c6930a 100644
--- a/src/glsl/link_uniforms.cpp
+++ b/src/glsl/link_uniforms.cpp
@@ -47,10 +47,9 @@
 static unsigned
 values_for_type(const glsl_type *type)
 {
-   if (type->is_sampler() || type->is_subroutine()) {
+   if (type->is_sampler()) {
   return 1;
-   } else if (type->is_array() && (type->fields.array->is_sampler() ||
-   type->fields.array->is_subroutine())) {
+   } else if (type->is_array() && type->fields.array->is_sampler()) {
   return type->array_size();
} else {
   return type->component_slots();
-- 
2.4.3

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