[Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl

[Gert Wollny]

Am Dienstag, den 10.07.2018, 16:01 +0200 schrieb Erik Faye-Lund:
> On 10. juli 2018 15:42, Gert Wollny wrote:
> > For three component textures virgl faces the problem that the host
> > driver
> > may report that these can not be used as a render target, and when
> > the
> > client requests such a texture a four-componet texture will be
> > choosen
> > even if only a sampler view was requested. One example where this
> > happens
> > is with the Intel i965 driver that doesn't support RGB32* as render
> > target.
> > The result is that when allocating a GL_RGB32F and a GL_RGB32I
> > texture, and
> > then glCopyImageSubData is called for these two texture, gallium
> > will fail
> > with an assertion, because it reports a different per pixel bit
> > count.
> > 
> > Therefore, when using the virgl driver, don't try to enable
> > BIND_RENDER_TARGET
> > for RGB textures that were requested with only BIND_SAMPLER_VIEW.
> > 
> > Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
> > ---
> > 
> > I'm aware that instead of using the device ID, I should probably
> > add a new caps
> > flag, but apart from that I was wondering whether there may be
> > better approaches
> > to achieve the same goal: The a texture is allocated with the
> > internal format
> > as closely as possible to the requested one. Especially it
> > shouldn't change the
> > percieved pixel bit count.
> > 
> > In fact, I was a bit surprised to see that the assertions regarding
> > the
> > different sizes was hit in st_copy_image:307 (swizzled_copy). It
> > seems that
> > there is some check missing that should redirect the copy in such a
> > case.
> > 
> > Many thanks for any comments,
> > Gert
> > 
> >   src/mesa/state_tracker/st_format.c | 22 +++++++++++++++-------
> >   1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/mesa/state_tracker/st_format.c
> > b/src/mesa/state_tracker/st_format.c
> > index 9ae796eca9..2d8ff756a9 100644
> > --- a/src/mesa/state_tracker/st_format.c
> > +++ b/src/mesa/state_tracker/st_format.c
> > @@ -2285,19 +2285,27 @@ st_ChooseTextureFormat(struct gl_context
> > *ctx, GLenum target,
> >   
> >      /* GL textures may wind up being render targets, but we don't
> > know
> >       * that in advance.  Specify potential render target flags now
> > for formats
> > -    * that we know should always be renderable.
> > +    * that we know should always be renderable, except when we are
> > on virgl,
> > +    * we don't try this for three component textures, because the
> > host might
> > +    * not support rendering to them, and then Gallium chooses a
> > four component
> > +    * internal format and calls to e.g. glCopyImageSubData will
> > fail for format
> > +    * that should be compatible.
> >       */
> >      bindings = PIPE_BIND_SAMPLER_VIEW;
> >      if (_mesa_is_depth_or_stencil_format(internalFormat))
> >         bindings |= PIPE_BIND_DEPTH_STENCIL;
> > -   else if (is_renderbuffer || internalFormat == 3 ||
> > internalFormat == 4 ||
> > -            internalFormat == GL_RGB || internalFormat == GL_RGBA
> > ||
> > -            internalFormat == GL_RGB8 || internalFormat ==
> > GL_RGBA8 ||
> > +   else if (is_renderbuffer  ||
> > +            internalFormat == GL_RGBA ||
> > +            internalFormat == GL_RGBA8 ||
> >               internalFormat == GL_BGRA ||
> > -            internalFormat == GL_RGB16F ||
> >               internalFormat == GL_RGBA16F ||
> > -            internalFormat == GL_RGB32F ||
> > -            internalFormat == GL_RGBA32F)
> > +            internalFormat == GL_RGBA32F ||
> > +            ((st->pipe->screen->get_param(st->pipe->screen,
> > PIPE_CAP_DEVICE_ID) != 0x1010) &&
> > +             (internalFormat == 3 || internalFormat == 4 ||
> > +              internalFormat == GL_RGB ||
> > +              internalFormat == GL_RGB8 ||
> > +              internalFormat == GL_RGB16F ||
> > +              internalFormat == GL_RGB32F )))
> >         bindings |= PIPE_BIND_RENDER_TARGET;
> 
> I don't think this is correct. 
I'm also quite sure that this would just be a work-around-something ...

> The problem is that the spec defines GL_RGB et al as color-
> renderable, and in OpenGL any color-renderable texture can become a
> render-target at any point. So the driver *has* to be prepared for
> rendering to GL_RGB.
> 
> The OpenGL 4.6 spec, section 9.4 "Framebuffer completeness" has this
> to say:
> 
> "An internal format is color-renderable if it is RED, RG, RGB, RGBA,
> or one of the sized internal formats from table 8.12 whose “CR” 
> (color-renderable) column is checked in that table"
Since this is also written in the 4.5 spec, and the Intel driver
advertises this version, but the test to attach such a texture to a
frame buffer device results in an incomplete framebuffer, the Intel
driver is not up to spec or so it would seem ... 

I wonder at which point this was introduced, 3.3 also has it, and on
r600 these tests were not resulting in assertion failures, only some
textures (sRGB I think) were allocated on the host side as four
component, and the copy fallback path was used, but this should be
managable, since it didn't refer to the 32 bit variants that are needed
for  ARB_texture_buffer_object_rgb32.

> So, all RGB formats must be color-renderable in OpenGL. For OpenGL
> ES, I think this is slightly different, where color-renderable
> guarantees for  RGB-textures are extensions for at least ES 2.0 IIRC.
> So *perhaps* we  could get away with something like this for that
> API.
A glipse over the GLES 3.1 spec gives me the impression that there we
don't need ARB_texture_buffer_object_rgb32, so for that goal we can can
just replace these formats by RGBX formats, but for Desktop OpenGL 4.0
is is needed.

Best, 
Gert