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

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

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.