thr3ads.net - Nouveau - [Nouveau] [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [May 2015]

If this information is useful, please help other people find it:
Share via:

Ilia Mirkin

2015-May-24 15:42 UTC

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

On Sun, May 24, 2015 at 10:56 AM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:>
>
> On 24.05.2015 16:15, Pierre Moreau wrote:
>>>
>>> On 24 May 2015, at 16:03, Tobias Klausmann
>>> <tobias.johannes.klausmann at 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 at 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 :-)
What lines?
>
> With this sorted out, series is:
Not sure what you mean here... what do you want me to sort out? The 2
back into a +1? I was just looking at the defines like

#define NV30_3D_VP_CLIP_PLANES_ENABLE_PLANE1                  0x00000020

and the 2 << 4i seemed more obviously correct. Although they're
obviously identical.
>
> Reviewed-by: Tobias Klausmann <tobias.johannes.klausmann at
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 at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>>>
>>> _______________________________________________
>>> Nouveau mailing list
>>> Nouveau at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>
>

Tobias Klausmann

2015-May-24 15:52 UTC

head link

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

On 24.05.2015 17:42, Ilia Mirkin wrote:> On Sun, May 24, 2015 at 10:56 AM, Tobias Klausmann
> <tobias.johannes.klausmann at mni.thm.de> wrote:
>>
>> On 24.05.2015 16:15, Pierre Moreau wrote:
>>>> On 24 May 2015, at 16:03, Tobias Klausmann
>>>> <tobias.johannes.klausmann at 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 at
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 :-)
> What lines?
>
>> With this sorted out, series is:
> Not sure what you mean here... what do you want me to sort out? The 2
> back into a +1? I was just looking at the defines like
Nah, i meant that it _is_ allright the way you did it and that we should 
change similar lines for clip in nv50/nvc0 the way you did here
>
> #define NV30_3D_VP_CLIP_PLANES_ENABLE_PLANE1                  0x00000020
>
> and the 2 << 4i seemed more obviously correct. Although they're
> obviously identical.
>
>> Reviewed-by: Tobias Klausmann <tobias.johannes.klausmann at
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 at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>>>> _______________________________________________
>>>> Nouveau mailing list
>>>> Nouveau at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>>

Ilia Mirkin

2015-May-24 15:55 UTC

head link

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

On Sun, May 24, 2015 at 11:52 AM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:>
>
> On 24.05.2015 17:42, Ilia Mirkin wrote:
>>
>> On Sun, May 24, 2015 at 10:56 AM, Tobias Klausmann
>> <tobias.johannes.klausmann at mni.thm.de> wrote:
>>>
>>>
>>> On 24.05.2015 16:15, Pierre Moreau wrote:
>>>>>
>>>>> On 24 May 2015, at 16:03, Tobias Klausmann
>>>>> <tobias.johannes.klausmann at 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 at
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 :-)
>>
>> What lines?
>>
>>> With this sorted out, series is:
>>
>> Not sure what you mean here... what do you want me to sort out? The 2
>> back into a +1? I was just looking at the defines like
>
>
> Nah, i meant that it _is_ allright the way you did it and that we should
> change similar lines for clip in nv50/nvc0 the way you did here
>
Ah OK. I'll push this out shortly. I'm not sure what lines in
nv50/nvc0 you're referring to...

nvc0:

   if (nvc0->state.clip_enable != clip_enable) {
      nvc0->state.clip_enable = clip_enable;
      IMMED_NVC0(push, NVC0_3D(CLIP_DISTANCE_ENABLE), clip_enable);
   }

nv50:

   clip_enable = nv50->rast->pipe.clip_plane_enable;

   BEGIN_NV04(push, NV50_3D(CLIP_DISTANCE_ENABLE), 1);
   PUSH_DATA (push, clip_enable);

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

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

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

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