[Mesa-dev] [PATCH v2 04/11] gallium: add endian_format field to struct pipe_resource
Oded Gabbay
oded.gabbay at gmail.com
Thu Apr 14 14:34:34 UTC 2016
On Thu, Apr 14, 2016 at 5:01 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> This feels like an incredibly confused interface to me...
I agree it's not the prettiest design I made, but considering
big-endian is a class F citizen, then that's the best I got right now.
I'm open to suggestions, but please guys don't tell me "that's not
good but we have no idea how to solve it otherwise". It's not
constructive.
>> you talk
> about CPU <-> GPU, but I think that's a misnomer. What happens if you
> use up too much VRAM and something gets evicted to GART? Does it get
> byteswapped to the CPU-endianness?
Good point, I need to think about that. If that happens, it may
require a complementary fix in kernel driver. However, it's not
mutually exclusive to this patch-set.
> Are you treating everything as
> 32-bit packed integers implicitly? What if you have, e.g., a R8 format
> (without the GBA bits)? Or even worse, a R16 format, where cpu bytes 0
> 1 2 3 would have to come out as 1 0 3 2.
I don't think I have the problem you describe. I'm not treating
anything in an implicit matter. Each format get's its own handling.
Please remember that r600 AMD GPUs *do support* big-endian. So, I
configure them to know the color format (R8, R16, L4A4, ...), and I
configure endian (8 in 16 swap, 8 in 32 swap, ...) and also I can
configure swizzling. Then, the GPU handles all the swapping.
Therefore, if it supports a certain format, it knows how to swap it
correctly.
>
> This seems like it might be enough for GL 1.3, but this sort of
> interface has to account for everything, IMO.
>
Well, for GL 3.3 with r600g, the piglit run on big-endian get's stuck
every time, after ~300 tests, even without my patches, so for sure its
broken. I just followed Marek's advise on how to solve it, and I
believe I reached a certain point where it is worth merging it.
> I was also struggling with such issues when getting the nv30 driver to
> work on big endian. This was compounded by the issue that the NVIDIA
> GPUs have a kinda-sorta-but-not-really big-endian mode which swaps
> some things but not others. Basically you need to intelligently be
> able to say what "endian" the data is in where, and in order to do
> that, you need to know what format it's in.
That's what I'm trying to do here.
And that's where we hit
> some fails, because all the transfer_map/unmap stuff is formatless.
> And so there's no way to know whether you should be swapping or not.
I tried to find a common ground for different formats, according to
how mesa/gallium treats them, and I believe I managed to do it to some
degree of success. I'm not saying its simple. There are different
paths in the code for the same action, and they behave differently.
e.g. in st_ReadPixels, you can read it using H/W blit or through
map+memcpy+unpack (fallback mode). So, for the same texture, it
depends which path you go through. In one path, I need to configure LE
and in the other path I need to configure BE.
> [I left these issues unresolved... nv30 on BE sorta works but not
> really.]
>
> -ilia
>
> On Thu, Apr 14, 2016 at 8:18 AM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>> This patch adds a new field, called "endian_format", to
>> "struct pipe_resource". The new field is of type "enum pipe_endian" and
>> can receive one of two values:
>> - PIPE_ENDIAN_LITTLE
>> - PIPE_ENDIAN_NATIVE
>>
>> PIPE_ENDIAN_NATIVE is initialized to either PIPE_ENDIAN_LITTLE or
>> PIPE_ENDIAN_BIG during build time.
>>
>> This field is needed to provide information to the H/W drivers about the
>> endianess current state or desired state of the resource. In other words,
>> for resources that are the source of the operation, this field indicates
>> the resource's current memory layout endianess (big or little endian).
>> For resources that are the destination of the operation, this field
>> indicates the resource's desired memory layout endianess.
>>
>> This field is mandatory because of how mesa works. When we get into the
>> H/W driver functions, the driver *ususally* doesn't know if it is doing a
>> CPU->GPU, a GPU->CPU, a CPU->CPU or a GPU->GPU operation, as this
>> information is "hidden" by the fact we go through common code
>> paths (state tracker layer). This isn't an issue in little-endian
>> architecture because both the CPU and GPU works in little-endian mode.
>> However, when the CPU is working in big-endian mode, the GPU needs to be
>> configured according to the requested operation's direction, because the
>> GPU *always* works in little-endian mode.
>>
>> There are two guidelines for setting this field:
>>
>> - This field must *never* be checked at the state tracker layer. It can
>> only be set in that layer. That way, drivers or state trackers that don't
>> use this field won't be effected at all.
>>
>> - The values that this field can be set to must be only
>> PIPE_ENDIAN_LITTLE or PIPE_ENDIAN_NATIVE. It should never be set
>> to PIPE_ENDIAN_BIG directly for the code to work correctly in both endian
>> modes.
>>
>> v2:
>>
>> This field is now initialized *only* at creation time, as pipe_resource
>> is an immutable object. For this change I did the following:
>>
>> - st_create_texture now receives a pipe_endian parameter, so whenever
>> a pipe_resource object is created using st_create_texture, the calling
>> function can set its endian_format during creation, same as pipe_format.
>>
>> - When a pipe_resource is created using a template, I initialized the
>> endian_format field of the template so the created object will contain
>> the correct endian value.
>>
>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>> ---
>> src/gallium/include/pipe/p_state.h | 1 +
>> src/mesa/state_tracker/st_cb_bitmap.c | 7 +++++--
>> src/mesa/state_tracker/st_cb_drawpixels.c | 15 +++++++++------
>> src/mesa/state_tracker/st_cb_readpixels.c | 4 ++++
>> src/mesa/state_tracker/st_cb_texture.c | 18 ++++++++++++++----
>> src/mesa/state_tracker/st_texture.c | 7 +++++--
>> src/mesa/state_tracker/st_texture.h | 3 ++-
>> 7 files changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
>> index 9e466ce..a669b3b 100644
>> --- a/src/gallium/include/pipe/p_state.h
>> +++ b/src/gallium/include/pipe/p_state.h
>> @@ -447,6 +447,7 @@ struct pipe_resource
>> struct pipe_screen *screen; /**< screen that this texture belongs to */
>> enum pipe_texture_target target; /**< PIPE_TEXTURE_x */
>> enum pipe_format format; /**< PIPE_FORMAT_x */
>> + enum pipe_endian endian_format; /**< PIPE_ENDIAN_x */
>>
>> unsigned width0;
>> unsigned height0;
>> diff --git a/src/mesa/state_tracker/st_cb_bitmap.c b/src/mesa/state_tracker/st_cb_bitmap.c
>> index 4fd2dfe..a4f241c 100644
>> --- a/src/mesa/state_tracker/st_cb_bitmap.c
>> +++ b/src/mesa/state_tracker/st_cb_bitmap.c
>> @@ -46,6 +46,7 @@
>> #include "st_program.h"
>> #include "st_cb_bitmap.h"
>> #include "st_texture.h"
>> +#include "st_format.h"
>>
>> #include "pipe/p_context.h"
>> #include "pipe/p_defines.h"
>> @@ -155,7 +156,8 @@ make_bitmap_texture(struct gl_context *ctx, GLsizei width, GLsizei height,
>> */
>> pt = st_texture_create(st, st->internal_target, st->bitmap.tex_format,
>> 0, width, height, 1, 1, 0,
>> - PIPE_BIND_SAMPLER_VIEW);
>> + PIPE_BIND_SAMPLER_VIEW,
>> + st_get_endian_by_format(st->bitmap.tex_format));
>> if (!pt) {
>> _mesa_unmap_pbo_source(ctx, unpack);
>> return NULL;
>> @@ -371,7 +373,8 @@ reset_cache(struct st_context *st)
>> st->bitmap.tex_format, 0,
>> BITMAP_CACHE_WIDTH, BITMAP_CACHE_HEIGHT,
>> 1, 1, 0,
>> - PIPE_BIND_SAMPLER_VIEW);
>> + PIPE_BIND_SAMPLER_VIEW,
>> + st_get_endian_by_format(st->bitmap.tex_format));
>> }
>>
>>
>> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c b/src/mesa/state_tracker/st_cb_drawpixels.c
>> index c3e05bb..96d97ca 100644
>> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
>> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
>> @@ -360,12 +360,13 @@ internal_format(struct gl_context *ctx, GLenum format, GLenum type)
>> */
>> static struct pipe_resource *
>> alloc_texture(struct st_context *st, GLsizei width, GLsizei height,
>> - enum pipe_format texFormat, unsigned bind)
>> + enum pipe_format texFormat, unsigned bind,
>> + enum pipe_endian endian_format)
>> {
>> struct pipe_resource *pt;
>>
>> pt = st_texture_create(st, st->internal_target, texFormat, 0,
>> - width, height, 1, 1, 0, bind);
>> + width, height, 1, 1, 0, bind, endian_format);
>>
>> return pt;
>> }
>> @@ -453,7 +454,8 @@ make_texture(struct st_context *st,
>> return NULL;
>>
>> /* alloc temporary texture */
>> - pt = alloc_texture(st, width, height, pipeFormat, PIPE_BIND_SAMPLER_VIEW);
>> + pt = alloc_texture(st, width, height, pipeFormat, PIPE_BIND_SAMPLER_VIEW,
>> + st_get_endian_by_format(pipeFormat));
>> if (!pt) {
>> _mesa_unmap_pbo_source(ctx, unpack);
>> return NULL;
>> @@ -473,7 +475,6 @@ make_texture(struct st_context *st,
>> PIPE_TRANSFER_WRITE, 0, 0,
>> width, height, &transfer);
>>
>> -
>> /* Put image into texture transfer.
>> * Note that the image is actually going to be upside down in
>> * the texture. We deal with that with texcoords.
>> @@ -1569,8 +1570,10 @@ st_CopyPixels(struct gl_context *ctx, GLint srcx, GLint srcy,
>> readW = MAX2(0, readW);
>> readH = MAX2(0, readH);
>>
>> - /* Allocate the temporary texture. */
>> - pt = alloc_texture(st, width, height, srcFormat, srcBind);
>> + /* Allocate the temporary texture, with the same endianess as that of the
>> + * source texture endianess. */
>> + pt = alloc_texture(st, width, height, srcFormat, srcBind,
>> + rbRead->texture->endian_format);
>> if (!pt)
>> return;
>>
>> diff --git a/src/mesa/state_tracker/st_cb_readpixels.c b/src/mesa/state_tracker/st_cb_readpixels.c
>> index 393b881..3d77bea 100644
>> --- a/src/mesa/state_tracker/st_cb_readpixels.c
>> +++ b/src/mesa/state_tracker/st_cb_readpixels.c
>> @@ -183,6 +183,10 @@ st_ReadPixels(struct gl_context *ctx, GLint x, GLint y,
>> dst_templ.format = dst_format;
>> dst_templ.bind = bind;
>> dst_templ.usage = PIPE_USAGE_STAGING;
>> + /* Because we are reading from the GPU (LE) we want to make sure the result
>> + * is in the host's endianess
>> + */
>> + dst_templ.endian_format = PIPE_ENDIAN_NATIVE;
>>
>> st_gl_texture_dims_to_pipe_dims(GL_TEXTURE_2D, width, height, 1,
>> &dst_templ.width0, &dst_templ.height0,
>> diff --git a/src/mesa/state_tracker/st_cb_texture.c b/src/mesa/state_tracker/st_cb_texture.c
>> index a18b08b..307d8cb 100644
>> --- a/src/mesa/state_tracker/st_cb_texture.c
>> +++ b/src/mesa/state_tracker/st_cb_texture.c
>> @@ -517,7 +517,8 @@ guess_and_alloc_texture(struct st_context *st,
>> ptHeight,
>> ptDepth,
>> ptLayers, 0,
>> - bindings);
>> + bindings,
>> + st_get_endian_by_format(fmt));
>>
>> stObj->lastLevel = lastLevel;
>>
>> @@ -604,7 +605,8 @@ st_AllocTextureImageBuffer(struct gl_context *ctx,
>> ptHeight,
>> ptDepth,
>> ptLayers, 0,
>> - bindings);
>> + bindings,
>> + st_get_endian_by_format(format));
>> return stImage->pt != NULL;
>> }
>> }
>> @@ -1791,6 +1793,8 @@ st_TexSubImage(struct gl_context *ctx, GLuint dims,
>> src_templ.format = src_format;
>> src_templ.bind = PIPE_BIND_SAMPLER_VIEW;
>> src_templ.usage = PIPE_USAGE_STAGING;
>> + /* source texture is created with host endianess */
>> + src_templ.endian_format = PIPE_ENDIAN_NATIVE;
>>
>> st_gl_texture_dims_to_pipe_dims(gl_target, width, height, depth,
>> &src_templ.width0, &src_templ.height0,
>> @@ -2253,6 +2257,10 @@ st_GetTexSubImage(struct gl_context * ctx,
>> dst_templ.format = dst_format;
>> dst_templ.bind = bind;
>> dst_templ.usage = PIPE_USAGE_STAGING;
>> + /* Because we are reading from the GPU (LE) we want to make sure the result
>> + * is in the host's endianess
>> + */
>> + dst_templ.endian_format = PIPE_ENDIAN_NATIVE;
>>
>> st_gl_texture_dims_to_pipe_dims(gl_target, width, height, depth,
>> &dst_templ.width0, &dst_templ.height0,
>> @@ -2868,7 +2876,8 @@ st_finalize_texture(struct gl_context *ctx,
>> ptHeight,
>> ptDepth,
>> ptLayers, ptNumSamples,
>> - bindings);
>> + bindings,
>> + st_get_endian_by_format(firstImageFormat));
>>
>> if (!stObj->pt) {
>> _mesa_error(ctx, GL_OUT_OF_MEMORY, "glTexImage");
>> @@ -2974,7 +2983,8 @@ st_AllocTextureStorage(struct gl_context *ctx,
>> ptHeight,
>> ptDepth,
>> ptLayers, num_samples,
>> - bindings);
>> + bindings,
>> + st_get_endian_by_format(fmt));
>> if (!stObj->pt)
>> return GL_FALSE;
>>
>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>> index 52b0943..9058b3d 100644
>> --- a/src/mesa/state_tracker/st_texture.c
>> +++ b/src/mesa/state_tracker/st_texture.c
>> @@ -62,7 +62,8 @@ st_texture_create(struct st_context *st,
>> GLuint depth0,
>> GLuint layers,
>> GLuint nr_samples,
>> - GLuint bind )
>> + GLuint bind,
>> + enum pipe_endian endian_format )
>> {
>> struct pipe_resource pt, *newtex;
>> struct pipe_screen *screen = st->pipe->screen;
>> @@ -93,6 +94,7 @@ st_texture_create(struct st_context *st,
>> pt.bind = bind;
>> pt.flags = 0;
>> pt.nr_samples = nr_samples;
>> + pt.endian_format = endian_format;
>>
>> newtex = screen->resource_create(screen, &pt);
>>
>> @@ -412,7 +414,8 @@ st_create_color_map_texture(struct gl_context *ctx)
>>
>> /* create texture for color map/table */
>> pt = st_texture_create(st, PIPE_TEXTURE_2D, format, 0,
>> - texSize, texSize, 1, 1, 0, PIPE_BIND_SAMPLER_VIEW);
>> + texSize, texSize, 1, 1, 0, PIPE_BIND_SAMPLER_VIEW,
>> + st_get_endian_by_format(format));
>> return pt;
>> }
>>
>> diff --git a/src/mesa/state_tracker/st_texture.h b/src/mesa/state_tracker/st_texture.h
>> index d8cd7c7..6122622 100644
>> --- a/src/mesa/state_tracker/st_texture.h
>> +++ b/src/mesa/state_tracker/st_texture.h
>> @@ -183,7 +183,8 @@ st_texture_create(struct st_context *st,
>> GLuint depth0,
>> GLuint layers,
>> GLuint nr_samples,
>> - GLuint tex_usage );
>> + GLuint tex_usage,
>> + enum pipe_endian endian_format );
>>
>>
>> extern void
>> --
>> 2.5.5
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list