[Mesa-stable] [Mesa-dev] [PATCH] r600g: Adjust pipe format when decompressing depth in BE

[Marek Olšák]

On Fri, Mar 4, 2016 at 6:53 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
> On Fri, Mar 4, 2016 at 6:59 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> On Fri, Mar 4, 2016 at 4:56 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>> On Fri, Mar 4, 2016 at 2:19 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>>> Note that the DB only supports tiling and separate depth and stencil, so
>>>> it's unmappable. Before transfers and sometimes even texturing, the buffer
>>>> must be copied via the DB->CB path, because CB supports both interleaved and
>>>> linear layouts. The result is the flushed texture. The goal here is to
>>>> ensure the flushed texture uses the correct format.
>>>>
>>>> Marek
>>>>
>>> Marek,
>>> Thanks for the info, makes the code more clear :)
>>>
>>> I can do what you asked, but frankly, I don't think it looks better:
>>>
>>> @@ -2657,9 +2657,15 @@ uint32_t r600_translate_colorformat(enum
>>> chip_class chip, enum pipe_format forma
>>>                                         return V_0280A0_COLOR_32_32;
>>>                         }
>>>                 } else if (HAS_SIZE(8,24,0,0)) {
>>> -                       return V_0280A0_COLOR_24_8;
>>> +                       if (R600_BIG_ENDIAN)
>>> +                               return V_0280A0_COLOR_8_24;
>>> +                       else
>>> +                               return V_0280A0_COLOR_24_8;
>>>                 } else if (HAS_SIZE(24,8,0,0)) {
>>> -                       return V_0280A0_COLOR_8_24;
>>> +                       if (R600_BIG_ENDIAN)
>>> +                               return V_0280A0_COLOR_24_8;
>>> +                       else
>>> +                               return V_0280A0_COLOR_8_24;
>>>                 }
>>>                 break;
>>>         case 3:
>>>
>>>
>>> @@ -1296,7 +1296,11 @@ unsigned r600_translate_colorswap(enum
>>> pipe_format format)
>>>                          (HAS_SWIZZLE(0,Y) && HAS_SWIZZLE(1,NONE)) ||
>>>                          (HAS_SWIZZLE(0,NONE) && HAS_SWIZZLE(1,X))) {
>>>                         entry = 2;
>>> +#ifdef PIPE_ARCH_LITTLE_ENDIAN
>>>                         ret = V_0280A0_SWAP_STD_REV; /* YX__ */
>>> +#else
>>> +                       ret = V_0280A0_SWAP_STD; /* YX__ */
>>> +#endif
>>>                 } else if (HAS_SWIZZLE(0,X) && HAS_SWIZZLE(3,Y)) {
>>>                         entry = 3;
>>>                         ret = V_0280A0_SWAP_ALT; /* X__Y */
>>>
>>>
>>> Actually I think it looks worse as we need to intervene in two places
>>> to get the same result I got with just switching the pipe format of
>>> the CB. And I still didn't check what happens with textures.
>>>
>>> In any case, this is basically a workaround, because gallium still
>>> thinks it uses the PIPE_FORMAT_Z24_UNORM_S8_UINT format, while we
>>> program the GPU with the PIPE_FORMAT_S8_UINT_Z24_UNORM parameters.
>>>
>>> I think that if we use a workaround, better use a cleaner/simpler one.
>>
>> Have you tested entire piglit with your first workaround? I doubt it
>> passes depth texturing tests.
>
> No. I only focused so far on getting gl-1.0-readpixsanity to work...
>
> piglit run on BE r600g is in such worse shape, there is no way of
> telling. It crashes after about 300 tests (out of 8600) and in those
> 300 tests, about 50 are failing, and I get dmesg errors from the
> kernel driver.
> Really, BE r600g is so broken, that there can be no baseline until I
> manage to fix things so I get at least an entire piglit run without
> dmesg errors and computer crashes. And I think I'm not anywhere near
> that yet.

I suggest disabling as many features as possible, degrading the driver
to GL 1.x if you have to and running piglit on that. Then you can add
features and see if they work.

I can easily see where the GPU can hang without byteswapping in the
right places. For example, uniform buffer objects and indirect draws.

>
> I'm sure I will find out that some of my current fixes don't match all
> of the cases and they will need better tweaking. I don't see that as
> problematic.
>
>>
>> The problem with hacking the DB->CB decompression blit path is that
>> it's sometimes used for texturing and other times it's not, and it
>> depends on the format and the chip. The other option is that the
>> DB->CB copy is not used for texturing if the hardware supports
>> in-place DB decompression; if so, the DB->CB copy is only used for
>> downloads. The result is that we have to maintain 2 texturing
>> codepaths: one that uses the original texture used by DB and
>> decompressed in-place, and one that is the result of the DB->CB copy.
>
> I assume you mean r600_blit_decompress_depth() and
> r600_blit_decompress_depth_in_place() ?

Yes.

>
>>
>> If you want to change the format of the flushed texture, fine, but you
>> should change it for all users of that codepath, not just texture
>> downloads.
>
> I'm not sure what you mean by that. Could you please give me an
> example or point me to the relevant functions ?

What I mean is that there are 2 texturing codepaths:
1) A depth texture is decompressed in-place and used for texturing.
2) A depth texture is decompressed by doing a DB->CB copy to
"flushing_texture", which is used for texturing. Also used for texture
downloads (readpixels).

Anyway, my point is, if you want to swap the format like that, it
should be swapped in the whole driver and in the right places (such as
the translate functions).

Marek