[PATCH 1/2] nv30: avoid doing extra work on clear and hitting unexpected states

[PATCH 1/2] nv30: avoid doing extra work on clear and hitting unexpected states

[Ilia Mirkin]

Clearing can happen at a time when various state objects are incoherent
and not ready for a draw. Some of the validation functions don't handle
this well, so only flush the framebuffer state. This has the advantage
of also not doing extra work.

This works around some crashes that can happen when clearing.

Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
---
 src/gallium/drivers/nouveau/nv30/nv30_clear.c          |  2 +-
 src/gallium/drivers/nouveau/nv30/nv30_context.h        |  2 +-
 src/gallium/drivers/nouveau/nv30/nv30_draw.c           |  4 ++--
 src/gallium/drivers/nouveau/nv30/nv30_state_validate.c | 10 ++++++----
 src/gallium/drivers/nouveau/nv30/nv30_vbo.c            |  2 +-
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_clear.c b/src/gallium/drivers/nouveau/nv30/nv30_clear.c
index 1ab8929..83fd1fa 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_clear.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_clear.c
@@ -58,7 +58,7 @@ nv30_clear(struct pipe_context *pipe, unsigned buffers,
    struct pipe_framebuffer_state *fb = &nv30->framebuffer;
    uint32_t colr = 0, zeta = 0, mode = 0;
 
-   if (!nv30_state_validate(nv30, TRUE))
+   if (!nv30_state_validate(nv30, NV30_NEW_FRAMEBUFFER | NV30_NEW_SCISSOR, TRUE))
       return;
 
    if (buffers & PIPE_CLEAR_COLOR && fb->nr_cbufs) {
diff --git a/src/gallium/drivers/nouveau/nv30/nv30_context.h b/src/gallium/drivers/nouveau/nv30/nv30_context.h
index 7b32aae..592cdbe 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_context.h
+++ b/src/gallium/drivers/nouveau/nv30/nv30_context.h
@@ -204,7 +204,7 @@ void
 nv30_render_vbo(struct pipe_context *pipe, const struct pipe_draw_info *info);
 
 boolean
-nv30_state_validate(struct nv30_context *nv30, boolean hwtnl);
+nv30_state_validate(struct nv30_context *nv30, uint32_t mask, boolean hwtnl);
 
 void
 nv30_state_release(struct nv30_context *nv30);
diff --git a/src/gallium/drivers/nouveau/nv30/nv30_draw.c b/src/gallium/drivers/nouveau/nv30/nv30_draw.c
index 3575c3d..38c31e9 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_draw.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_draw.c
@@ -129,7 +129,7 @@ nv30_render_draw_elements(struct vbuf_render *render,
                        NOUVEAU_BO_LOW | NOUVEAU_BO_RD, 0, 0);
    }
 
-   if (!nv30_state_validate(nv30, FALSE))
+   if (!nv30_state_validate(nv30, ~0, FALSE))
       return;
 
    BEGIN_NV04(push, NV30_3D(VERTEX_BEGIN_END), 1);
@@ -174,7 +174,7 @@ nv30_render_draw_arrays(struct vbuf_render *render, unsigned start, uint nr)
                        NOUVEAU_BO_LOW | NOUVEAU_BO_RD, 0, 0);
    }
 
-   if (!nv30_state_validate(nv30, FALSE))
+   if (!nv30_state_validate(nv30, ~0, FALSE))
       return;
 
    BEGIN_NV04(push, NV30_3D(VERTEX_BEGIN_END), 1);
diff --git a/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c b/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c
index 0f9d19d..86ac4f7 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_state_validate.c
@@ -456,7 +456,7 @@ nv30_state_context_switch(struct nv30_context *nv30)
 }
 
 boolean
-nv30_state_validate(struct nv30_context *nv30, boolean hwtnl)
+nv30_state_validate(struct nv30_context *nv30, uint32_t mask, boolean hwtnl)
 {
    struct nouveau_screen *screen = &nv30->screen->base;
    struct nouveau_pushbuf *push = nv30->base.pushbuf;
@@ -481,14 +481,16 @@ nv30_state_validate(struct nv30_context *nv30, boolean hwtnl)
    else
       validate = swtnl_validate_list;
 
-   if (nv30->dirty) {
+   mask &= nv30->dirty;
+
+   if (mask) {
       while (validate->func) {
-         if (nv30->dirty & validate->mask)
+         if (mask & validate->mask)
             validate->func(nv30);
          validate++;
       }
 
-      nv30->dirty = 0;
+      nv30->dirty &= ~mask;
    }
 
    nouveau_pushbuf_bufctx(push, bctx);
diff --git a/src/gallium/drivers/nouveau/nv30/nv30_vbo.c b/src/gallium/drivers/nouveau/nv30/nv30_vbo.c
index 67ab829..d4e384b 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_vbo.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_vbo.c
@@ -564,7 +564,7 @@ nv30_draw_vbo(struct pipe_context *pipe, const struct pipe_draw_info *info)
    if (nv30->vbo_user && !(nv30->dirty & (NV30_NEW_VERTEX | NV30_NEW_ARRAYS)))
       nv30_update_user_vbufs(nv30);
 
-   nv30_state_validate(nv30, TRUE);
+   nv30_state_validate(nv30, ~0, TRUE);
    if (nv30->draw_flags) {
       nv30_render_vbo(pipe, info);
       return;
-- 
2.3.6

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[PATCH 2/2] nv30: fix clip plane uploads and enable changes

[Ilia Mirkin]

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);
    }
 
    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 },
-- 
2.3.6

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Pitoiset]

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?

>      }
>   
>      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

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

[Tobias Klausmann]

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


>
>>      }
>>        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

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

[Pierre Moreau]

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

> 
> 
>> 
>>>     }
>>>       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

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

[Tobias Klausmann]

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

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

[Ilia Mirkin]

On Sun, May 24, 2015 at 10:56 AM, Tobias Klausmann
<tobias.johannes.klausmann@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@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 :-)

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@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

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

[Tobias Klausmann]

On 24.05.2015 17:42, Ilia Mirkin wrote:
> On Sun, May 24, 2015 at 10:56 AM, Tobias Klausmann
> <tobias.johannes.klausmann@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@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 :-)
> 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@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

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

[Ilia Mirkin]

On Sun, May 24, 2015 at 11:52 AM, Tobias Klausmann
<tobias.johannes.klausmann@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@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@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 :-)
>>
>> 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 mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau