[Mesa-dev] [PATCH v2 04/11] gallium: add endian_format field to struct pipe_resource

[Oded Gabbay]

On Tue, Apr 19, 2016 at 9:10 AM, Michel Dänzer <michel at daenzer.net> wrote:
> On 19.04.2016 00:03, Ilia Mirkin wrote:
>> On Mon, Apr 18, 2016 at 10:47 AM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>> On Thu, Apr 14, 2016 at 6:44 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>
>>> To make the GPU do a conversion during blitting, I need to configure
>>> registers. This is done in a couple of functions in the r600g driver
>>> (r600_translate_texformat, r600_colorformat_endian_swap,
>>> r600_translate_colorformat and r600_translate_colorswap).
>>>
>>> The problem is that transfer_map/unmap don't call directly to those
>>> functions. They call other functions which eventually call those 4
>>> functions. Among those "other" functions, there are several function
>>> calls which are *not* in the r600g driver. i.e. we go back to generic
>>> util functions. For example:
>>>
>>> #0  r600_translate_colorformat
>>> #1  evergreen_init_color_surface
>>> #2  evergreen_set_framebuffer_state
>>> #3  util_blitter_custom_depth_stencil
>>> #4  r600_blit_decompress_depth
>>> #5  r600_texture_transfer_map
>>>
>>> Am I allowed to now pass information from transfer_map/unmap all the
>>> way down to the 4 functions I mentioned through all these layers as
>>> additional parameters ? I preferred to put it in pipe_resource as that
>>> information goes all the way down to those functions, but if I can't
>>> use that, then what's an acceptable alternative ?
>>>
>>> This time, I would like to get an agreement *before* I implement it.
>>
>> Probably a good idea. And as issues are investigated, people's
>> opinions on the "correct" way might shift. Let's think about this...
>>
>> So clearly *a* correct way to handle this would be to stop all the
>> lying. What's the lie? The lie is the PIPE_FORMAT. It talks about e.g.
>> R5G6B5 but makes no mention of the byte layout in memory for those 16
>> bits. Really what we have right now is a format and an *implicit*
>> endian ordering, which is the CPU's. But what happens when the CPU and
>> GPU don't agree?
>>
>> There's a path we could take which would be to add an endianness
>> alongside each format (be it by doubling formats, or an explicit
>> second field). This would be a very far-reaching change though, and I
>> doubt you'll want to do it. What we're left with is having a format
>> and an *implicit* endianness. Which means that the consumers of the
>> format need to be able to work out the implicit endianness involved.
>> And the endianness will be GPU endian for regular resources, and CPU
>> endian for "staging" resources. So it's definitely tempting to stick
>> the endian thing into a private field of the resource, like Rob is
>> suggesting - when creating a staging texture in
>> transfer_map/unmap/flush, set the endianness the cpu endian. Otherwise
>> set it to gpu endian. And I think this is somewhat similar to your
>> former approach.
>
> A fundamental issue of the former approach was that it tried to attach
> endianness to 8888 array formats, which makes no sense.

I think this was mostly a semantic mistake. I didn't want to attach
endianess to 8888 array formats. What I did wanted to do, is to mark
them such that the r600g driver won't configure the registers to do
any byte swaps, or component swaps. Thus, I marked them as LE, and in
the configuration functions, if the mark is LE, I don't configure any
swap.

Perhaps a better way was to define natural endianess, or
alternatively, instead of marking them, just check in
r600_translate_colorswap() if the format is an array format, and if it
is, don't configure any component swaps. I actually have this covered
already in r600_colorformat_endian_swap() for the byte swaps part.

Oded

The order of
> colour components in memory either matches that described by such a
> format or it doesn't (in which case there's a bug that needs to be
> fixed, not hacked around), endianness doesn't come into play for that.
>
> Also, what you're describing above isn't really necessary (from a
> make-it-work POV, as opposed to a
> make-the-format-model-complete-and-consistent POV) for formats such as
> R5G6B5, because GPUs supported by the r600 driver can be programmed to
> just always access such formats using the CPU byte order.
>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer