Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
Sounds perfect, thanks! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
On Tue, 2018-12-11 at 15:29 +0100, Erik Faye-Lund wrote:
> On Tue, 2018-12-11 at 15:26 +0100, Erik Faye-Lund wrote:
> > Virglrenderer does the wrong thing when given an instance divisor;
> > it tries to use the element-index rather than the binding-index as
> > the argument to glVertexBindingDivisor(). This worked fine as long
> > as there was a 1:1 relationship between elements and bindings,
> > which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO in
> > st_atom_array.c.".
> >
> > So let's detect instance divisors, and restore a 1:1 relationship in
> > that case. This will make old versions of virglrenderer behave
> > correctly. For newer versions, we can consider making a better
> > interface, where the instance divisor isn't specified per element,
> > but rather per binding. But let's save that for another day.
> >
> > Signed-off-by: Erik Faye-Lund
>
> I suppose this should have had:
>
> Fixes: 19a91841c34 "st/mesa: Use Array._DrawVAO in st_atom_array.c."
>
As 19a91841c34 "st/mesa: Use Array._DrawVAO in st_atom_array.c." is included in
18.2 stable branch, this patch 4/4 is also a candidate to be included.
Nevertheless, in order to apply it cleanly, I also need to apply patch 3/4,
8447b642387 ("virgl: wrap vertex element state in a struct").
I've applied both of them. If you prefer not apply any of them, just let me
know.
Thanks.
J.A.
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
This fixes rendering in Supertuxkart, Serious Sam 3, and Shadow Warrior
via virgl for me. So for the series:
Tested-By: Gert Wollny
Am Dienstag, den 11.12.2018, 15:26 +0100 schrieb Erik Faye-Lund:
> Virglrenderer does the wrong thing when given an instance divisor;
> it tries to use the element-index rather than the binding-index as
> the argument to glVertexBindingDivisor(). This worked fine as long
> as there was a 1:1 relationship between elements and bindings,
> which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO in
> st_atom_array.c.".
>
> So let's detect instance divisors, and restore a 1:1 relationship in
> that case. This will make old versions of virglrenderer behave
> correctly. For newer versions, we can consider making a better
> interface, where the instance divisor isn't specified per element,
> but rather per binding. But let's save that for another day.
>
> Signed-off-by: Erik Faye-Lund
> ---
> src/gallium/drivers/virgl/virgl_context.c | 29
> ++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/virgl/virgl_context.c
> b/src/gallium/drivers/virgl/virgl_context.c
> index 121d9139f28..ce72f73a0f6 100644
> --- a/src/gallium/drivers/virgl/virgl_context.c
> +++ b/src/gallium/drivers/virgl/virgl_context.c
> @@ -50,6 +50,8 @@
>
> struct virgl_vertex_elements_state {
> uint32_t handle;
> + uint8_t binding_map[PIPE_MAX_ATTRIBS];
> + uint8_t num_bindings;
> };
>
> static uint32_t next_handle;
> @@ -390,10 +392,24 @@ static void
> *virgl_create_vertex_elements_state(struct pipe_context *ctx,
> unsigned
> num_elements,
> const struct
> pipe_vertex_element *elements)
> {
> + struct pipe_vertex_element new_elements[PIPE_MAX_ATTRIBS];
> struct virgl_context *vctx = virgl_context(ctx);
> struct virgl_vertex_elements_state *state =
>CALLOC_STRUCT(virgl_vertex_elements_state);
>
> + for (int i = 0; i < num_elements; ++i) {
> + if (elements[i].instance_divisor) {
> + for (int j = 0; j < num_elements; ++j) {
> +new_elements[j] = elements[j];
> +new_elements[j].vertex_buffer_index = j;
> +state->binding_map[j] = elements[j].vertex_buffer_index;
> + }
> + elements = new_elements;
> + state->num_bindings = num_elements;
> + break;
> + }
> + }
> +
> state->handle = virgl_object_assign_handle();
> virgl_encoder_create_vertex_elements(vctx, state->handle,
> num_elements, elements);
> @@ -419,6 +435,7 @@ static void
> virgl_bind_vertex_elements_state(struct pipe_context *ctx,
> vctx->vertex_elements = state;
> virgl_encode_bind_object(vctx, state ? state->handle : 0,
> VIRGL_OBJECT_VERTEX_ELEMENTS);
> + vctx->vertex_array_dirty = TRUE;
> }
>
> static void virgl_set_vertex_buffers(struct pipe_context *ctx,
> @@ -438,7 +455,17 @@ static void virgl_set_vertex_buffers(struct
> pipe_context *ctx,
> static void virgl_hw_set_vertex_buffers(struct virgl_context *vctx)
> {
> if (vctx->vertex_array_dirty) {
> - virgl_encoder_set_vertex_buffers(vctx, vctx-
> >num_vertex_buffers, vctx->vertex_buffer);
> + struct virgl_vertex_elements_state *ve = vctx-
> >vertex_elements;
> +
> + if (ve->num_bindings) {
> + struct pipe_vertex_buffer vertex_buffers[PIPE_MAX_ATTRIBS];
> + for (int i = 0; i < ve->num_bindings; ++i)
> +vertex_buffers[i] = vctx->vertex_buffer[ve-
> >binding_map[i]];
> +
> + virgl_encoder_set_vertex_buffers(vctx, ve->num_bindings,
> vertex_buffers);
> + } else
> + virgl_encoder_set_vertex_buffers(vctx, vctx-
> >num_vertex_buffers, vctx->vertex_buffer);
> +
>virgl_attach_res_vertex_buffers(vctx);
> }
> }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
Good Morning,
> > One thing, may be. Do you want to add some documentation beside the
> > git log message why we do something surprising like replicating out
> > the
> > buffers and assigning new buffer indices? Just something that allows
> > a reader to get an idea why non straight forward things happen here.
> > The git annotate references to the commit messages tend to vanish
> > over time behind further changes in the code.
> >
>
> Yeah, that's a good point. This deserves a comment to clear things up a
> bit.
>
> How about something like this squashed in?
>
> ---8<---
> diff --git a/src/gallium/drivers/virgl/virgl_context.c
> b/src/gallium/drivers/virgl/virgl_context.c
> index ce72f73a0f6..2e3202b04e9 100644
> --- a/src/gallium/drivers/virgl/virgl_context.c
> +++ b/src/gallium/drivers/virgl/virgl_context.c
> @@ -399,6 +399,10 @@ static void
> *virgl_create_vertex_elements_state(struct pipe_context *ctx,
>
> for (int i = 0; i < num_elements; ++i) {
>if (elements[i].instance_divisor) {
> + /* Virglrenderer doesn't deal with instance_divisor correctly
> if
> + * there isn't a 1:1 relationship between elements and
> bindings.
> + * So let's make sure there is, by duplicating bindings.
> + */
>for (int j = 0; j < num_elements; ++j) {
> new_elements[j] = elements[j];
> new_elements[j].vertex_buffer_index = j;
> ---8<---
That's good for me.
Also the squashed get my RB.
best and Thanks
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
On Wed, 2018-12-12 at 06:18 +0100, Mathias Fröhlich wrote:
> Erik,
>
> On Tuesday, 11 December 2018 15:29:49 CET Erik Faye-Lund wrote:
> > On Tue, 2018-12-11 at 15:26 +0100, Erik Faye-Lund wrote:
> > > Virglrenderer does the wrong thing when given an instance
> > > divisor;
> > > it tries to use the element-index rather than the binding-index
> > > as
> > > the argument to glVertexBindingDivisor(). This worked fine as
> > > long
> > > as there was a 1:1 relationship between elements and bindings,
> > > which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO
> > > in
> > > st_atom_array.c.".
> > >
> > > So let's detect instance divisors, and restore a 1:1 relationship
> > > in
> > > that case. This will make old versions of virglrenderer behave
> > > correctly. For newer versions, we can consider making a better
> > > interface, where the instance divisor isn't specified per
> > > element,
> > > but rather per binding. But let's save that for another day.
> > >
> > > Signed-off-by: Erik Faye-Lund
>
> I don't exactly know what kind of coding standards and that you use
> in virgl.
> But the patch series looks good and should help for the issues we
> observed.
>
> One thing, may be. Do you want to add some documentation beside the
> git log message why we do something surprising like replicating out
> the
> buffers and assigning new buffer indices? Just something that allows
> a reader to get an idea why non straight forward things happen here.
> The git annotate references to the commit messages tend to vanish
> over time behind further changes in the code.
>
Yeah, that's a good point. This deserves a comment to clear things up a
bit.
How about something like this squashed in?
---8<---
diff --git a/src/gallium/drivers/virgl/virgl_context.c
b/src/gallium/drivers/virgl/virgl_context.c
index ce72f73a0f6..2e3202b04e9 100644
--- a/src/gallium/drivers/virgl/virgl_context.c
+++ b/src/gallium/drivers/virgl/virgl_context.c
@@ -399,6 +399,10 @@ static void
*virgl_create_vertex_elements_state(struct pipe_context *ctx,
for (int i = 0; i < num_elements; ++i) {
if (elements[i].instance_divisor) {
+/* Virglrenderer doesn't deal with instance_divisor correctly
if
+ * there isn't a 1:1 relationship between elements and
bindings.
+ * So let's make sure there is, by duplicating bindings.
+ */
for (int j = 0; j < num_elements; ++j) {
new_elements[j] = elements[j];
new_elements[j].vertex_buffer_index = j;
---8<---
> > I suppose this should have had:
> >
> > Fixes: 19a91841c34 "st/mesa: Use Array._DrawVAO in
> > st_atom_array.c."
> Probably at least for patch #3 and #4.
>
> With or without such comment and for the series:
> Reviewed-by: Mathias Fröhlich
>
> best
>
> Mathias
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
Erik, On Tuesday, 11 December 2018 15:29:49 CET Erik Faye-Lund wrote: > On Tue, 2018-12-11 at 15:26 +0100, Erik Faye-Lund wrote: > > Virglrenderer does the wrong thing when given an instance divisor; > > it tries to use the element-index rather than the binding-index as > > the argument to glVertexBindingDivisor(). This worked fine as long > > as there was a 1:1 relationship between elements and bindings, > > which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO in > > st_atom_array.c.". > > > > So let's detect instance divisors, and restore a 1:1 relationship in > > that case. This will make old versions of virglrenderer behave > > correctly. For newer versions, we can consider making a better > > interface, where the instance divisor isn't specified per element, > > but rather per binding. But let's save that for another day. > > > > Signed-off-by: Erik Faye-Lund I don't exactly know what kind of coding standards and that you use in virgl. But the patch series looks good and should help for the issues we observed. One thing, may be. Do you want to add some documentation beside the git log message why we do something surprising like replicating out the buffers and assigning new buffer indices? Just something that allows a reader to get an idea why non straight forward things happen here. The git annotate references to the commit messages tend to vanish over time behind further changes in the code. > I suppose this should have had: > > Fixes: 19a91841c34 "st/mesa: Use Array._DrawVAO in st_atom_array.c." Probably at least for patch #3 and #4. With or without such comment and for the series: Reviewed-by: Mathias Fröhlich best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
On Tue, 2018-12-11 at 15:26 +0100, Erik Faye-Lund wrote: > Virglrenderer does the wrong thing when given an instance divisor; > it tries to use the element-index rather than the binding-index as > the argument to glVertexBindingDivisor(). This worked fine as long > as there was a 1:1 relationship between elements and bindings, > which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO in > st_atom_array.c.". > > So let's detect instance divisors, and restore a 1:1 relationship in > that case. This will make old versions of virglrenderer behave > correctly. For newer versions, we can consider making a better > interface, where the instance divisor isn't specified per element, > but rather per binding. But let's save that for another day. > > Signed-off-by: Erik Faye-Lund I suppose this should have had: Fixes: 19a91841c34 "st/mesa: Use Array._DrawVAO in st_atom_array.c." ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
