* [PATCH 1/2] nv30: avoid doing extra work on clear and hitting unexpected states @ 2015-05-24 4:58 Ilia Mirkin 2015-05-24 4:58 ` [PATCH 2/2] nv30: fix clip plane uploads and enable changes Ilia Mirkin 0 siblings, 1 reply; 9+ messages in thread From: Ilia Mirkin @ 2015-05-24 4:58 UTC (permalink / raw) To: nouveau, mesa-dev 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] nv30: fix clip plane uploads and enable changes 2015-05-24 4:58 [PATCH 1/2] nv30: avoid doing extra work on clear and hitting unexpected states Ilia Mirkin @ 2015-05-24 4:58 ` Ilia Mirkin [not found] ` <1432443489-1067-2-git-send-email-imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Ilia Mirkin @ 2015-05-24 4:58 UTC (permalink / raw) To: nouveau, mesa-dev 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <1432443489-1067-2-git-send-email-imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <1432443489-1067-2-git-send-email-imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> @ 2015-05-24 8:38 ` Samuel Pitoiset [not found] ` <55618E16.6090700-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Samuel Pitoiset @ 2015-05-24 8:38 UTC (permalink / raw) To: Ilia Mirkin, nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, mesa-dev-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <55618E16.6090700-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <55618E16.6090700-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-05-24 14:03 ` Tobias Klausmann [not found] ` <5561DA2D.50202-AqjdNwhu20eELgA04lAiVw@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Tobias Klausmann @ 2015-05-24 14:03 UTC (permalink / raw) To: Samuel Pitoiset, Ilia Mirkin, nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <5561DA2D.50202-AqjdNwhu20eELgA04lAiVw@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <5561DA2D.50202-AqjdNwhu20eELgA04lAiVw@public.gmane.org> @ 2015-05-24 14:15 ` Pierre Moreau [not found] ` <ED051C04-3BF9-4299-AF71-AAED9DE8DF9B-GANU6spQydw@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Pierre Moreau @ 2015-05-24 14:15 UTC (permalink / raw) To: Tobias Klausmann; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <ED051C04-3BF9-4299-AF71-AAED9DE8DF9B-GANU6spQydw@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <ED051C04-3BF9-4299-AF71-AAED9DE8DF9B-GANU6spQydw@public.gmane.org> @ 2015-05-24 14:56 ` Tobias Klausmann [not found] ` <5561E6AF.9050700-AqjdNwhu20eELgA04lAiVw@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Tobias Klausmann @ 2015-05-24 14:56 UTC (permalink / raw) To: Pierre Moreau; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <5561E6AF.9050700-AqjdNwhu20eELgA04lAiVw@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <5561E6AF.9050700-AqjdNwhu20eELgA04lAiVw@public.gmane.org> @ 2015-05-24 15:42 ` Ilia Mirkin [not found] ` <CAKb7Uvhkt7OGxbE3hkEkxUMgH7LkmJsa73xp5AxLYc8PN=9e3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Ilia Mirkin @ 2015-05-24 15:42 UTC (permalink / raw) To: Tobias Klausmann Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CAKb7Uvhkt7OGxbE3hkEkxUMgH7LkmJsa73xp5AxLYc8PN=9e3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <CAKb7Uvhkt7OGxbE3hkEkxUMgH7LkmJsa73xp5AxLYc8PN=9e3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-05-24 15:52 ` Tobias Klausmann [not found] ` <5561F3B6.2080903-AqjdNwhu20eELgA04lAiVw@public.gmane.org> 0 siblings, 1 reply; 9+ messages in thread From: Tobias Klausmann @ 2015-05-24 15:52 UTC (permalink / raw) To: Ilia Mirkin; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <5561F3B6.2080903-AqjdNwhu20eELgA04lAiVw@public.gmane.org>]
* Re: [Mesa-dev] [PATCH 2/2] nv30: fix clip plane uploads and enable changes [not found] ` <5561F3B6.2080903-AqjdNwhu20eELgA04lAiVw@public.gmane.org> @ 2015-05-24 15:55 ` Ilia Mirkin 0 siblings, 0 replies; 9+ messages in thread From: Ilia Mirkin @ 2015-05-24 15:55 UTC (permalink / raw) To: Tobias Klausmann Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-05-24 15:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-24 4:58 [PATCH 1/2] nv30: avoid doing extra work on clear and hitting unexpected states Ilia Mirkin
2015-05-24 4:58 ` [PATCH 2/2] nv30: fix clip plane uploads and enable changes Ilia Mirkin
[not found] ` <1432443489-1067-2-git-send-email-imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org>
2015-05-24 8:38 ` [Mesa-dev] " Samuel Pitoiset
[not found] ` <55618E16.6090700-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-05-24 14:03 ` Tobias Klausmann
[not found] ` <5561DA2D.50202-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
2015-05-24 14:15 ` Pierre Moreau
[not found] ` <ED051C04-3BF9-4299-AF71-AAED9DE8DF9B-GANU6spQydw@public.gmane.org>
2015-05-24 14:56 ` Tobias Klausmann
[not found] ` <5561E6AF.9050700-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
2015-05-24 15:42 ` Ilia Mirkin
[not found] ` <CAKb7Uvhkt7OGxbE3hkEkxUMgH7LkmJsa73xp5AxLYc8PN=9e3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-24 15:52 ` Tobias Klausmann
[not found] ` <5561F3B6.2080903-AqjdNwhu20eELgA04lAiVw@public.gmane.org>
2015-05-24 15:55 ` Ilia Mirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.