[Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer

[Mathias Fröhlich]

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 <erik.faye-lund at collabora.com>

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 <Mathias.Froehlich at web.de>

best

Mathias