[Mesa-dev] [PATCH 03/10] mesa/st: add endian_format field to struct pipe_resource
Oded Gabbay
oded.gabbay at gmail.com
Tue Apr 12 07:29:29 UTC 2016
On Mon, Apr 11, 2016 at 6:34 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 11.04.2016 um 16:34 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 which 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.
>>
>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>> ---
>> src/gallium/include/pipe/p_state.h | 1 +
>> src/mesa/state_tracker/st_cb_drawpixels.c | 11 ++++++-
>> src/mesa/state_tracker/st_cb_fbo.c | 16 ++++++++++-
>> src/mesa/state_tracker/st_cb_readpixels.c | 5 ++++
>> src/mesa/state_tracker/st_cb_texture.c | 13 +++++++++
>> src/mesa/state_tracker/st_texture.c | 48 +++++++++++++++++++++++++++++++
>> 6 files changed, 92 insertions(+), 2 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_drawpixels.c b/src/mesa/state_tracker/st_cb_drawpixels.c
>> index 01ed544..5eafdc0 100644
>> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
>> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
>> @@ -465,7 +465,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.
>> @@ -1222,6 +1221,11 @@ copy_stencil_pixels(struct gl_context *ctx, GLint srcx, GLint srcy,
>> assert(util_format_get_blockwidth(rbDraw->texture->format) == 1);
>> assert(util_format_get_blockheight(rbDraw->texture->format) == 1);
>>
>> + /* We are going to upload texture to GPU, so mark texture according
>> + * to the host's endianess
>> + */
>> + rbDraw->texture->endian_format = PIPE_ENDIAN_NATIVE;
>> +
>> /* map the stencil buffer */
>> drawMap = pipe_transfer_map(pipe,
>> rbDraw->texture,
>> @@ -1561,6 +1565,11 @@ st_CopyPixels(struct gl_context *ctx, GLint srcx, GLint srcy,
>> if (!pt)
>> return;
>>
>> + /* Temporary texture needs to be in GPU format as the swapping happens
>> + * when blitting to it
>> + */
>> + pt->endian_format = PIPE_ENDIAN_LITTLE;
>> +
>> sv[0] = st_create_texture_sampler_view(st->pipe, pt);
>> if (!sv[0]) {
>> pipe_resource_reference(&pt, NULL);
>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>> index 456ad83..0fcadb2 100644
>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>> @@ -754,6 +754,7 @@ st_MapRenderbuffer(struct gl_context *ctx,
>> unsigned usage;
>> GLuint y2;
>> GLubyte *map;
>> + enum pipe_endian saved_endian_format;
>>
>> if (strb->software) {
>> /* software-allocated renderbuffer (probably an accum buffer) */
>> @@ -771,9 +772,17 @@ st_MapRenderbuffer(struct gl_context *ctx,
>> return;
>> }
>>
>> + /* save current endian format of renderbuffer in case we change it */
>> + saved_endian_format = strb->texture->endian_format;
>> +
>> usage = 0x0;
>> - if (mode & GL_MAP_READ_BIT)
>> + if (mode & GL_MAP_READ_BIT) {
>> usage |= PIPE_TRANSFER_READ;
>> + /* reading from the GPU into the texture of the renderbuffer so we
>> + * need to do swapping according to host's endianess
>> + */
>> + strb->texture->endian_format = PIPE_ENDIAN_NATIVE;
>> + }
>> if (mode & GL_MAP_WRITE_BIT)
>> usage |= PIPE_TRANSFER_WRITE;
>> if (mode & GL_MAP_INVALIDATE_RANGE_BIT)
>> @@ -807,6 +816,11 @@ st_MapRenderbuffer(struct gl_context *ctx,
>> *mapOut = NULL;
>> *rowStrideOut = 0;
>> }
>> +
>> + /* Need to restore original endian format for all cases renderbuffer is
>> + * used as source
>> + */
>> + strb->texture->endian_format = saved_endian_format;
>> }
>>
>>
>> diff --git a/src/mesa/state_tracker/st_cb_readpixels.c b/src/mesa/state_tracker/st_cb_readpixels.c
>> index 5153c4b..7686c42 100644
>> --- a/src/mesa/state_tracker/st_cb_readpixels.c
>> +++ b/src/mesa/state_tracker/st_cb_readpixels.c
>> @@ -193,6 +193,11 @@ st_ReadPixels(struct gl_context *ctx, GLint x, GLint y,
>> goto fallback;
>> }
>>
>> + /* Because we are reading from the GPU (LE) we want to make sure the result
>> + * is in the host's endianess
>> + */
>> + dst->endian_format = PIPE_ENDIAN_NATIVE;
>> +
>> memset(&blit, 0, sizeof(blit));
>> blit.src.resource = src;
>> blit.src.level = strb->surface->u.tex.level;
>> diff --git a/src/mesa/state_tracker/st_cb_texture.c b/src/mesa/state_tracker/st_cb_texture.c
>> index 3980f5d..8bd3707 100644
>> --- a/src/mesa/state_tracker/st_cb_texture.c
>> +++ b/src/mesa/state_tracker/st_cb_texture.c
>> @@ -1809,6 +1809,14 @@ st_TexSubImage(struct gl_context *ctx, GLuint dims,
>> goto fallback;
>> }
>>
>> + /* source texture is created with host endianess */
>> + src->endian_format = PIPE_ENDIAN_NATIVE;
>> +
>> + /* Destination image will be in GPU and all subsequent actions will
>> + * be done inside/from GPU, so we need to mark it as LE as we don't want
>> + * unnecessary swaps. The only swap we want is from the src to dst */
>> + dst->endian_format = PIPE_ENDIAN_LITTLE;
>> +
>> /* Map source pixels. */
>> pixels = _mesa_validate_pbo_teximage(ctx, dims, width, height, depth,
>> format, type, pixels, unpack,
>> @@ -2262,6 +2270,11 @@ st_GetTexSubImage(struct gl_context * ctx,
>> goto fallback;
>> }
>>
>> + /* Because we are reading from the GPU (LE) we want to make sure the result
>> + * is in the host's endianess
>> + */
>> + dst->endian_format = PIPE_ENDIAN_NATIVE;
>> +
>> /* From now on, we need the gallium representation of dimensions. */
>> if (gl_target == GL_TEXTURE_1D_ARRAY) {
>> zoffset = yoffset;
>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>> index 52b0943..a780c80 100644
>> --- a/src/mesa/state_tracker/st_texture.c
>> +++ b/src/mesa/state_tracker/st_texture.c
>> @@ -94,6 +94,54 @@ st_texture_create(struct st_context *st,
>> pt.flags = 0;
>> pt.nr_samples = nr_samples;
>>
>> + /* set it to Host endianess by default, except for a couple of gallium
>> + * formats, where gallium provides different formats based on endianess */
>> + switch (format)
>> + {
>> + case PIPE_FORMAT_R8G8B8A8_UNORM:
>> + case PIPE_FORMAT_R8G8B8X8_UNORM:
>> + case PIPE_FORMAT_B8G8R8A8_UNORM:
>> + case PIPE_FORMAT_B8G8R8X8_UNORM:
>> + case PIPE_FORMAT_A8R8G8B8_UNORM:
>> + case PIPE_FORMAT_X8R8G8B8_UNORM:
>> + case PIPE_FORMAT_A8B8G8R8_UNORM:
>> + case PIPE_FORMAT_X8B8G8R8_UNORM:
>> + case PIPE_FORMAT_R8G8B8A8_SNORM:
>> + case PIPE_FORMAT_R8G8B8X8_SNORM:
>> + case PIPE_FORMAT_A8B8G8R8_SNORM:
>> + case PIPE_FORMAT_X8B8G8R8_SNORM:
>> + case PIPE_FORMAT_R8G8B8A8_SRGB:
>> + case PIPE_FORMAT_R8G8B8X8_SRGB:
>> + case PIPE_FORMAT_B8G8R8A8_SRGB:
>> + case PIPE_FORMAT_B8G8R8X8_SRGB:
>> + case PIPE_FORMAT_A8R8G8B8_SRGB:
>> + case PIPE_FORMAT_X8R8G8B8_SRGB:
>> + case PIPE_FORMAT_A8B8G8R8_SRGB:
>> + case PIPE_FORMAT_X8B8G8R8_SRGB:
>> + case PIPE_FORMAT_A8L8_UNORM:
>> + case PIPE_FORMAT_L8A8_UNORM:
>> + case PIPE_FORMAT_A8L8_SNORM:
>> + case PIPE_FORMAT_L8A8_SNORM:
>> + case PIPE_FORMAT_A8L8_SRGB:
>> + case PIPE_FORMAT_L8A8_SRGB:
>> + case PIPE_FORMAT_A16L16_UNORM:
>> + case PIPE_FORMAT_L16A16_UNORM:
>> + case PIPE_FORMAT_G8R8_UNORM:
>> + case PIPE_FORMAT_R8G8_UNORM:
>> + case PIPE_FORMAT_G8R8_SNORM:
>> + case PIPE_FORMAT_R8G8_SNORM:
>> + case PIPE_FORMAT_G16R16_UNORM:
>> + case PIPE_FORMAT_R16G16_UNORM:
>> + case PIPE_FORMAT_G16R16_SNORM:
>> + case PIPE_FORMAT_R16G16_SNORM:
>> + pt.endian_format = PIPE_ENDIAN_LITTLE;
>> + break;
>> +
>> + default:
>> + pt.endian_format = PIPE_ENDIAN_NATIVE;
>> + break;
>> + }
>> +
>> newtex = screen->resource_create(screen, &pt);
>>
>> assert(!newtex || pipe_is_referenced(&newtex->reference));
>>
>
> That would be a gallium: change not mesa/st:
>
> NAK though. First, you're modifying the pipe_resource fields after
> creation for instance in st_TexSubImage - these are immutable, so no,
> you absolutely can't do that, no way.
Got it. I'll try to follow Marek's advice.
>
> I don't claim I know how to solve it properly but it sort of sounds to
> me like some more places need to know about endianness than they do
> know? Be it state tracker or driver.
Well, yeah, that's why I added this new field.
But this field will only be used by H/W drivers though, because
llvmpipe/softpipe use the pack/unpack/convert functions that are
created automatically by gallium (see u_format_table.c), and those
functions handle all the endianess conversion issues.
Oded
>
> Roland
>
More information about the mesa-dev
mailing list