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

[Michel Dänzer]

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. 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