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

[Oded Gabbay]

On Fri, Mar 4, 2016 at 10:08 PM, Marek Olšák <maraeo at gmail.com> wrote:
> 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'll follow you advice here and try to make batched patches for each
GL revision, also to reduce interruptions for you/Michel.

Oded

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

Got it, thanks.

>
> Marek