[Mesa-dev] [PATCH] nv50: make sure to clear _all_ layers of all attachments

[Emil Velikov]

On 13/02/14 16:14, Ilia Mirkin wrote:
> On Thu, Feb 13, 2014 at 9:57 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 13/02/14 10:13, Ilia Mirkin wrote:
>>> Unfortunately there's only one RT_ARRAY_MODE setting for all
>>> attachments, so clears were previously truncated to the minimum number
>>> of layers any attachment had. Instead set the RT_ARRAY_MODE to 512 (the
>>> max number of layers) before doing the clear. This fixes
>>> gl-3.2-layered-rendering-clear-color-mismatched-layer-count.
>>>
>> Pardon my ignorance but how did you gather the maximum number of layers
>> ? The headers list values up-to 0xffff.
>>
>> Guessing that setting the maximum number of layers is a slight overkill
>> but I'm definitely not opposed to it.
> 
> The headers sure do...
> 
> [369050.901062] nouveau E[  PGRAPH][0000:02:00.0] DATA_ERROR INVALID_VALUE
> [369050.901069] nouveau E[  PGRAPH][0000:02:00.0] ch 4 [0x003f92e000
> gl-3.2-layered-[13468]] subc 3 class 0x8297 mthd 0x1224 data
> 0x0000ffff
> [369178.121557] nouveau E[  PGRAPH][0000:02:00.0] DATA_ERROR INVALID_VALUE
> [369178.121567] nouveau E[  PGRAPH][0000:02:00.0] ch 4 [0x003f92e000
> gl-3.2-layered-[14400]] subc 3 class 0x8297 mthd 0x1224 data
> 0x00000201
> 
> And also
> 
>    case PIPE_CAP_MAX_TEXTURE_ARRAY_LAYERS:
>       return 512;
> 
> So I'm pretty sure that 512 is max. I'll have to check whether the
> rnndb can support a max value like that.
> 
> CLEAR_BUFFERS explicitly gets the layer id from the code, so we'll
> never try to clear a layer that we don't think is there. The whole
> problem originates from the issues around multiple attachments with
> different numbers of layers. Setting the max just avoids the card from
> checking whether the layer id is valid or not based on the
> RT_ARRAY_MODE.
> 
>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_surface.c b/src/gallium/drivers/nouveau/nv50/nv50_surface.c
>>> index a22436b..68924c9 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_surface.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_surface.c
>>> @@ -366,7 +366,7 @@ nv50_clear_depth_stencil(struct pipe_context *pipe,
>>>     PUSH_DATA (push, bo->offset + sf->offset);
>>>     PUSH_DATA (push, nv50_format_table[dst->format].rt);
>>>     PUSH_DATA (push, mt->level[sf->base.u.tex.level].tile_mode);
>>> -   PUSH_DATA (push, 0);
>>> +   PUSH_DATA (push, mt->layer_stride >> 2);
>> I'm guessing that this hunk could be separated, as it does make sense to
>> set the stride when doing the clears.
> 
> I guess it could be. But it's related to clearing and layers and I
> noticed it when I was doing this exercise, so... seems reasonable to
> throw it in here.
> 
Fair enough. Can you please add a couple of words in the commit msg.

>>
>> Does this make make any difference wrt the test in question ?
> 
> Not in the least. The test goes down the nv50_clear path. No idea what
> hits this path, but it's part of the pipe interface.
> 
Three users - st/vega, st/clover and the aux/postprocess(effectively
mlaa). mlaa gets initialized as it's detected in the dri-config,
regardless if it's settings :\

Not sure how that would cause any issues though.

FWIW the patch is Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>

-Emil
>   -ilia
>