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

[Erik Faye-Lund]

On 10. juli 2018 16:23, Gert Wollny wrote:
> 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 ...

OpenGL has this other excape hatch, where it alllows drivers to say 
FRAMEBUFFER_UNSUPPORTED for basically any combination of attachments it 
doesn't like. Perhaps the Intel is using this as a way out? If so, we 
might have to be prepared to do something similar.

I don't think Gallium has a way of rejecting framebuffers in this way, 
though. So if that's what's going on, we might have to introduce one.

> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev