[Mesa-dev] [PATCH v2 04/11] gallium: add endian_format field to struct pipe_resource
Oded Gabbay
oded.gabbay at gmail.com
Mon Apr 18 08:43:00 UTC 2016
On Fri, Apr 15, 2016 at 2:35 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, Apr 14, 2016 at 7:26 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 14.04.2016 um 14:18 schrieb Oded Gabbay:
>>> 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
>>>
>>
>> This is now better as it doesn't violate the contract of the immutable
>> objects.
>> However, I have to agree with Ilia, this doesn't really seem like a
>> clean solution. Having to create differently endianness flagged
>> resources depending on what they are intended to be used for just seems
>> wrong. That's just not how those resource objects are supposed to work.
>> Doing workarounds at transfer_map time would seem better. However, I
>> have no idea how either of these solutions is going to work with
>> coherent/persistent or whatnot mappings, I think there's still something
>> wrong (but I'm no expert on endianness issues, thinking about it too
>> much gives me a headache).
>
> Any solution for persistent/coherent mappings will have to require
> that cpu and gpu agree on the format. However ARB_buffer_storage is
> the only way to access that, and losing that on a LE/BE hybrid system
> doesn't seem like the worst thing.
>
> -ilia
Hi Ilia,
I read your proposal and I'll try to see if I can make it work.
Thanks,
Oded
More information about the mesa-dev
mailing list