Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
If some formats are not supported as render targets, I recommend that they are not supported for texturing either. (radeonsi doesn't support unaligned 3-channel formats either. It only supports aligned formats, such as R8G8B8X8 and R32G32B32X32.) If you can't support RGBX formats as render targets, you can still expose them as long as you support RGBX for texturing. Everything will be correct except for blending where DST_ALPHA will read the value from memory instead of returning 1. (and you can fix the blend state to use ONE instead) Marek On Tue, Jul 10, 2018 at 9:42 AM, 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 > --- > > 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; > > /* GLES allows the driver to choose any format which matches > -- > 2.17.1 > > ___ > 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] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
Am Dienstag, den 10.07.2018, 11:03 -0400 schrieb Ilia Mirkin: > > Here's how every real driver handles it: > > If it's a texture, don't support rgb-only formats (except r11g11b10f > and so on). > > In the is_format_supported check, there are explicit tests for target > == PIPE_BUFFER in order to allow the tbo_rgb32 case. Great, thanks for the pointer, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
On Tue, Jul 10, 2018 at 10:59 AM, Gert Wollny wrote: > Am Dienstag, den 10.07.2018, 10:47 -0400 schrieb Ilia Mirkin: >> In practice, no one allocates GL_RGB textures. Only RGBX/RGBA. In >> part due to this issue, an in part due to the hardware not being able >> to support rendering to such textures (and usually not texturing >> either). > I see. Well, I was close to solve this problem by exactly doing that, > but on the way I lost ARB_tbo_rgb32. Anyway, if I understand you > correctly, then this should be solvable on the driver level. > >> ARB_tbo_rgb32 is part of GLES 3.1 as I recall, but this only has to >> do with buffer objects, so not renderable. > I actually dind't find it at all in the spec. only that all the GL_RGB > textures don't need to be renderable in general. > >> >> Separately, glCopyImageSubData has to work between compatible >> internal formats, irrespective of what happens under the hood. If >> i965 can't copy from a GL_RGB32F texture to a GL_RGB32I texture, >> that's a i965 bug. > Intel is doing just fine, it's gallium that has a problem here. I think > it assumes that the checks for compatibility done on the mesa side are > sufficient, and doesn't check whether the actual internal formats are > compatible (with the exception of an assertion though). I'm just not > sure whether the copy fallback path implemented there is also useful > for this case. Right ... that logic is a bit finicky. It assumes that the driver presents a logical group of formats. Here's how every real driver handles it: If it's a texture, don't support rgb-only formats (except r11g11b10f and so on). In the is_format_supported check, there are explicit tests for target == PIPE_BUFFER in order to allow the tbo_rgb32 case. Cheers, -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
Am Dienstag, den 10.07.2018, 10:47 -0400 schrieb Ilia Mirkin: > In practice, no one allocates GL_RGB textures. Only RGBX/RGBA. In > part due to this issue, an in part due to the hardware not being able > to support rendering to such textures (and usually not texturing > either). I see. Well, I was close to solve this problem by exactly doing that, but on the way I lost ARB_tbo_rgb32. Anyway, if I understand you correctly, then this should be solvable on the driver level. > ARB_tbo_rgb32 is part of GLES 3.1 as I recall, but this only has to > do with buffer objects, so not renderable. I actually dind't find it at all in the spec. only that all the GL_RGB textures don't need to be renderable in general. > > Separately, glCopyImageSubData has to work between compatible > internal formats, irrespective of what happens under the hood. If > i965 can't copy from a GL_RGB32F texture to a GL_RGB32I texture, > that's a i965 bug. Intel is doing just fine, it's gallium that has a problem here. I think it assumes that the checks for compatibility done on the mesa side are sufficient, and doesn't check whether the actual internal formats are compatible (with the exception of an assertion though). I'm just not sure whether the copy fallback path implemented there is also useful for this case. Best, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
Am Dienstag, den 10.07.2018, 16:30 +0200 schrieb Erik Faye-Lund: > > [snip] > On 10. juli 2018 16:23, Gert Wollny wrote: > > 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? FRAMEBUFFER_UNSUPPORTED is exactly the error message that Intel shows. > If so, we might have to be prepared to do something similar. In any case, IMHO allocating a 3-comp texture should still try to succeed without resorting to a four comp texture if possible (and if render-target was not requested, then it would succeed, hence the way I proposed the patch). This is essentially what the Intel driver does: It gives you a texture like requested without trying to be smart about it and when you try to use it as FBO attachment, it sais no. The other question is, of course, that gallium should not hit that assertion ... > 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 guess I'll do a bit code-reading ... best, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
In practice, no one allocates GL_RGB textures. Only RGBX/RGBA. In part due to this issue, an in part due to the hardware not being able to support rendering to such textures (and usually not texturing either). ARB_tbo_rgb32 is part of GLES 3.1 as I recall, but this only has to do with buffer objects, so not renderable. Separately, glCopyImageSubData has to work between compatible internal formats, irrespective of what happens under the hood. If i965 can't copy from a GL_RGB32F texture to a GL_RGB32I texture, that's a i965 bug. -ilia On Tue, Jul 10, 2018 at 10:30 AM, Erik Faye-Lund wrote: > 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 --- 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 >>>
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
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 --- 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
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
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 > > --- > > > > 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
Re: [Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
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 --- 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. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH] mesa/st: don't prematurely optimize for render targets when on virgl
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 --- 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; /* GLES allows the driver to choose any format which matches -- 2.17.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
