[Mesa-dev] [PATCH 03/10] mesa/st: add endian_format field to struct pipe_resource
Roland Scheidegger
sroland at vmware.com
Tue Apr 12 13:35:26 UTC 2016
Am 12.04.2016 um 09:50 schrieb Oded Gabbay:
> 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:
>
> Roland,
> Why is this gallium: ? These are all files in mesa/state_tracker folder.
> Could you please explain ?
As Marek pointed out, you modified gallium/include. This means this is
really a change to gallium interface - as such it affects eventually far
more than just mesa's statetracker (other state trackers, drivers, ...).
It is of course true that sometimes you have to modify multiple parts,
so the right subject might not be trivial. But interface changes are the
ones which require the most scrutiny since every driver, state tracker
etc is likely going to be affected - other changes are more isolated.
(In fact, I wouldn't have looked at this patch if it were just a mesa/st
change as I don't have time for that, but since at least it was obvious
this is going to be an interface change from the subject line I did.)
Roland
More information about the mesa-dev
mailing list