Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes

From: Tobias Klausmann <tobias.johannes.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
To: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes
Date: Sun, 24 May 2015 16:56:47 +0200	[thread overview]
Message-ID: <5561E6AF.9050700@mni.thm.de> (raw)
In-Reply-To: <ED051C04-3BF9-4299-AF71-AAED9DE8DF9B-GANU6spQydw@public.gmane.org>

On 24.05.2015 16:15, Pierre Moreau wrote:
>> On 24 May 2015, at 16:03, Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de> wrote:
>>
>>
>>
>> On 24.05.2015 10:38, Samuel Pitoiset wrote:
>>>
>>> On 05/24/2015 06:58 AM, Ilia Mirkin wrote:
>>>> nv30_validate_clip depends on the rasterizer state. Also we should
>>>> upload all the new clip planes on change since next time the plane data
>>>> won't have changed, but the enables might.
>>>>
>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>> ---
>>>>   src/gallium/drivers/nouveau/nv30/nv30_state_validate.c | 16 +++++++---------
>>>>   1 file changed, 7 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c b/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c
>>>> index 86ac4f7..a954dcc 100644
>>>> --- a/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c
>>>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c
>>>> @@ -272,15 +272,13 @@ nv30_validate_clip(struct nv30_context *nv30)
>>>>      uint32_t clpd_enable = 0;
>>>>        for (i = 0; i < 6; i++) {
>>>> -      if (nv30->rast->pipe.clip_plane_enable & (1 << i)) {
>>>> -         if (nv30->dirty & NV30_NEW_CLIP) {
>>>> -            BEGIN_NV04(push, NV30_3D(VP_UPLOAD_CONST_ID), 5);
>>>> -            PUSH_DATA (push, i);
>>>> -            PUSH_DATAp(push, nv30->clip.ucp[i], 4);
>>>> -         }
>>>> -
>>>> -         clpd_enable |= 1 << (1 + 4*i);
>>>> +      if (nv30->dirty & NV30_NEW_CLIP) {
>>>> +         BEGIN_NV04(push, NV30_3D(VP_UPLOAD_CONST_ID), 5);
>>>> +         PUSH_DATA (push, i);
>>>> +         PUSH_DATAp(push, nv30->clip.ucp[i], 4);
>>>>         }
>>>> +      if (nv30->rast->pipe.clip_plane_enable & (1 << i))
>>>> +         clpd_enable |= 2 << (4*i);
>>> Can you explain why did you change this line?
>> This does bother me as well :)
> It should be the same as before but using one less addition: shifting 1 by 5 or 2 by 4 is similar.

*dang* you are right. maybe we should change those lines along in nv50 
and nvc0, save the additional addition :-)

With this sorted out, series is:

Reviewed-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>

>
>>
>>>>      }
>>>>        BEGIN_NV04(push, NV30_3D(VP_CLIP_PLANES_ENABLE), 1);
>>>> @@ -389,7 +387,7 @@ static struct state_validate hwtnl_validate_list[] = {
>>>>       { nv30_validate_stipple,       NV30_NEW_STIPPLE },
>>>>       { nv30_validate_scissor,       NV30_NEW_SCISSOR | NV30_NEW_RASTERIZER },
>>>>       { nv30_validate_viewport,      NV30_NEW_VIEWPORT },
>>>> -    { nv30_validate_clip,          NV30_NEW_CLIP },
>>>> +    { nv30_validate_clip,          NV30_NEW_CLIP | NV30_NEW_RASTERIZER },
>>>>       { nv30_fragprog_validate,      NV30_NEW_FRAGPROG | NV30_NEW_FRAGCONST },
>>>>       { nv30_vertprog_validate,      NV30_NEW_VERTPROG | NV30_NEW_VERTCONST |
>>>>                                      NV30_NEW_FRAGPROG | NV30_NEW_RASTERIZER },
>>> _______________________________________________
>>> Nouveau mailing list
>>> Nouveau@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau