Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
Am Sonntag, den 22.07.2018, 09:59 -0400 schrieb Ilia Mirkin: > On Sun, Jul 22, 2018 at 2:54 AM, Gert Wollny om> wrote: > > Am Samstag, den 21.07.2018, 14:14 -0400 schrieb Ilia Mirkin: > > > On Sat, Jul 21, 2018 at 1:48 PM, Gert Wollny > > ra.c > > > om> wrote: > > > > Am Samstag, den 21.07.2018, 11:33 -0400 schrieb Ilia Mirkin: > > > > > > > > > > > > > > > On Sat, Jul 21, 2018, 05:45 Gert Wollny > > > > a.co > > > > > m> > > > > > wrote: > > > > > > Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia > > > > > > Mirkin: > > > > > > > > > > > > > > > + /* Allow 3-comp 32 bit texturs only for TBOs > > > > > > > > (needed > > > > > > > > for > > > > > > > > ARB_tbo_rgb32) */ > > > > > > > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || > > > > > > > > + format == PIPE_FORMAT_R32G32B32_SINT || > > > > > > > > + format == PIPE_FORMAT_R32G32B32_UINT) && > > > > > > > > + target != PIPE_BUFFER) > > > > > > > > + return FALSE; > > > > > > > > > > > > > > My personal recommendation would be to disallow *all* > > > > > > > packed > > > > > > > RGB > > > > > > > formats unless target == PIPE_BUFFER. (And even then -- > > > > > > > > > > > > questionable, > > > > > > > except for RGB32, which is required.) > > > > > > > > > > > > > > > > > > > RGB8 and RGB16 are disabled/replaced by RGBX* on the host > > > > > > side, > > > > > > and > > > > > > R11G11B10 doesn't seem to make problems. > > > > > > > > > > No other driver supports these. What happens on the host side > > > > > is > > > > > hidden from virgl. > > > > > > > > Not exactly, of course the host tells virgl what formats are > > > > supported, > > > > and we test using dEQP and piglit quite heavily to get things > > > > right, so > > > > I'm quite confident that whatever problem comes up, we will > > > > catch > > > > it. > > > > > > The result is that you end up lying to mesa and the various > > > helpers > > > -- > > > the user creates a RGB8 texture, which your gallium driver claims > > > to > > > support. In reality, it's a RGBX texture on the host, but this > > > information is hidden from virgl > > > > No, it is not, virgl_is_format_supported will not claim that the > > RGB format is supported, it will pick PIPE_FORMAT_RnGnBnXn_* also > > on the guest side, because PIPE_FORMAT_RnGnBn_* (n = 8,16) is not > > in the list of driver supported formats (not even for buffers), > > this information is in the caps sampler.bitmask and render.bitmask > > that are send from the host and that are also checked in this > > function. Only because we can not remove PIPE_FORMAT_R32G32B32_* > > completely we need this additional check. > > So to confirm, is_format_supported(PIPE_FORMAT_R8G8B8_UNORM) will > return false, irrespective of what the host driver might be. Correct? Yes best. Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
On Sun, Jul 22, 2018 at 2:54 AM, Gert Wollny wrote: > Am Samstag, den 21.07.2018, 14:14 -0400 schrieb Ilia Mirkin: >> On Sat, Jul 21, 2018 at 1:48 PM, Gert Wollny > om> wrote: >> > Am Samstag, den 21.07.2018, 11:33 -0400 schrieb Ilia Mirkin: >> > > >> > > >> > > On Sat, Jul 21, 2018, 05:45 Gert Wollny > > > m> >> > > wrote: >> > > > Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia Mirkin: >> > > > > >> > > > > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed >> > > > > > for >> > > > > > ARB_tbo_rgb32) */ >> > > > > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || >> > > > > > + format == PIPE_FORMAT_R32G32B32_SINT || >> > > > > > + format == PIPE_FORMAT_R32G32B32_UINT) && >> > > > > > + target != PIPE_BUFFER) >> > > > > > + return FALSE; >> > > > > >> > > > > My personal recommendation would be to disallow *all* packed >> > > > > RGB >> > > > > formats unless target == PIPE_BUFFER. (And even then -- >> > > > >> > > > questionable, >> > > > > except for RGB32, which is required.) >> > > > > >> > > > >> > > > RGB8 and RGB16 are disabled/replaced by RGBX* on the host side, >> > > > and >> > > > R11G11B10 doesn't seem to make problems. >> > > >> > > No other driver supports these. What happens on the host side is >> > > hidden from virgl. >> > >> > Not exactly, of course the host tells virgl what formats are >> > supported, >> > and we test using dEQP and piglit quite heavily to get things >> > right, so >> > I'm quite confident that whatever problem comes up, we will catch >> > it. >> >> The result is that you end up lying to mesa and the various helpers >> -- >> the user creates a RGB8 texture, which your gallium driver claims to >> support. In reality, it's a RGBX texture on the host, but this >> information is hidden from virgl > No, it is not, virgl_is_format_supported will not claim that the RGB > format is supported, it will pick PIPE_FORMAT_RnGnBnXn_* also on the > guest side, because PIPE_FORMAT_RnGnBn_* (n = 8,16) is not in the list > of driver supported formats (not even for buffers), this information is > in the caps sampler.bitmask and render.bitmask that are send from the > host and that are also checked in this function. Only because we can > not remove PIPE_FORMAT_R32G32B32_* completely we need this additional > check. So to confirm, is_format_supported(PIPE_FORMAT_R8G8B8_UNORM) will return false, irrespective of what the host driver might be. Correct? -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
Am Samstag, den 21.07.2018, 14:14 -0400 schrieb Ilia Mirkin: > On Sat, Jul 21, 2018 at 1:48 PM, Gert Wollny om> wrote: > > Am Samstag, den 21.07.2018, 11:33 -0400 schrieb Ilia Mirkin: > > > > > > > > > On Sat, Jul 21, 2018, 05:45 Gert Wollny > > m> > > > wrote: > > > > Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia Mirkin: > > > > > > > > > > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed > > > > > > for > > > > > > ARB_tbo_rgb32) */ > > > > > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || > > > > > > + format == PIPE_FORMAT_R32G32B32_SINT || > > > > > > + format == PIPE_FORMAT_R32G32B32_UINT) && > > > > > > + target != PIPE_BUFFER) > > > > > > + return FALSE; > > > > > > > > > > My personal recommendation would be to disallow *all* packed > > > > > RGB > > > > > formats unless target == PIPE_BUFFER. (And even then -- > > > > > > > > questionable, > > > > > except for RGB32, which is required.) > > > > > > > > > > > > > RGB8 and RGB16 are disabled/replaced by RGBX* on the host side, > > > > and > > > > R11G11B10 doesn't seem to make problems. > > > > > > No other driver supports these. What happens on the host side is > > > hidden from virgl. > > > > Not exactly, of course the host tells virgl what formats are > > supported, > > and we test using dEQP and piglit quite heavily to get things > > right, so > > I'm quite confident that whatever problem comes up, we will catch > > it. > > The result is that you end up lying to mesa and the various helpers > -- > the user creates a RGB8 texture, which your gallium driver claims to > support. In reality, it's a RGBX texture on the host, but this > information is hidden from virgl No, it is not, virgl_is_format_supported will not claim that the RGB format is supported, it will pick PIPE_FORMAT_RnGnBnXn_* also on the guest side, because PIPE_FORMAT_RnGnBn_* (n = 8,16) is not in the list of driver supported formats (not even for buffers), this information is in the caps sampler.bitmask and render.bitmask that are send from the host and that are also checked in this function. Only because we can not remove PIPE_FORMAT_R32G32B32_* completely we need this additional check. 1. You're hitting paths no one ever thought to be possible since "no > way a backend driver implements such weird formats" (I guarantee you > I've followed this line of thinking in the past, but I don't remember > precisely where). I can tell you: it was the RFC that came before this patch (I suspected that is was not the correct way to handle the problem, that's why it was a RFC). In summary, the host driver (Intel i965) claims some support for these formats, this is tested via the GL interface and communicated to the guest, and Gallium couldn't deal with it correctly. My first stab was to replace all RGBn formats with RGBXn, which killed tbo_rgb32, so I needed a workaround for RGB32*, and thanks to your pointer I was able to fix this within the driver. > I don't see why you're fighting so hard to keep the RGB8/16 > formats... As pointed out above, I don't, the way to reject these formats is simply based on another mechanism that can not be used for RGB32*, because these are needed to some extend for tbo_rgb32. Thanks again for pointing me in the right direction for RGB32, and I hope I was able to clarify why the same is not needed for RGB8/16 best, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
On Sat, Jul 21, 2018 at 1:48 PM, Gert Wollny wrote: > Am Samstag, den 21.07.2018, 11:33 -0400 schrieb Ilia Mirkin: >> >> >> On Sat, Jul 21, 2018, 05:45 Gert Wollny >> wrote: >> > Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia Mirkin: >> > > >> > > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed for >> > > > ARB_tbo_rgb32) */ >> > > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || >> > > > + format == PIPE_FORMAT_R32G32B32_SINT || >> > > > + format == PIPE_FORMAT_R32G32B32_UINT) && >> > > > + target != PIPE_BUFFER) >> > > > + return FALSE; >> > > >> > > My personal recommendation would be to disallow *all* packed RGB >> > > formats unless target == PIPE_BUFFER. (And even then -- >> > questionable, >> > > except for RGB32, which is required.) >> > > >> > RGB8 and RGB16 are disabled/replaced by RGBX* on the host side, and >> > R11G11B10 doesn't seem to make problems. >> >> No other driver supports these. What happens on the host side is >> hidden from virgl. > Not exactly, of course the host tells virgl what formats are supported, > and we test using dEQP and piglit quite heavily to get things right, so > I'm quite confident that whatever problem comes up, we will catch it. The result is that you end up lying to mesa and the various helpers -- the user creates a RGB8 texture, which your gallium driver claims to support. In reality, it's a RGBX texture on the host, but this information is hidden from virgl and the state tracker and mesa core. So now you have two problems: 1. You're hitting paths no one ever thought to be possible since "no way a backend driver implements such weird formats" (I guarantee you I've followed this line of thinking in the past, but I don't remember precisely where). 2. You're lying to the state tracker about what the true format really is (since you claim it's RGB8 but deep down inside, hidden away from the guest, it's RGBX8). This will affect its various decisions. I don't see why you're fighting so hard to keep the RGB8/16 formats... -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
Am Samstag, den 21.07.2018, 11:33 -0400 schrieb Ilia Mirkin: > > > On Sat, Jul 21, 2018, 05:45 Gert Wollny > wrote: > > Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia Mirkin: > > > > > > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed for > > > > ARB_tbo_rgb32) */ > > > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || > > > > + format == PIPE_FORMAT_R32G32B32_SINT || > > > > + format == PIPE_FORMAT_R32G32B32_UINT) && > > > > + target != PIPE_BUFFER) > > > > + return FALSE; > > > > > > My personal recommendation would be to disallow *all* packed RGB > > > formats unless target == PIPE_BUFFER. (And even then -- > > questionable, > > > except for RGB32, which is required.) > > > > > RGB8 and RGB16 are disabled/replaced by RGBX* on the host side, and > > R11G11B10 doesn't seem to make problems. > > No other driver supports these. What happens on the host side is > hidden from virgl. Not exactly, of course the host tells virgl what formats are supported, and we test using dEQP and piglit quite heavily to get things right, so I'm quite confident that whatever problem comes up, we will catch it. Cheers, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
On Sat, Jul 21, 2018, 05:45 Gert Wollny wrote: > Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia Mirkin: > > > > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed for > > > ARB_tbo_rgb32) */ > > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || > > > + format == PIPE_FORMAT_R32G32B32_SINT || > > > + format == PIPE_FORMAT_R32G32B32_UINT) && > > > + target != PIPE_BUFFER) > > > + return FALSE; > > > > My personal recommendation would be to disallow *all* packed RGB > > formats unless target == PIPE_BUFFER. (And even then -- questionable, > > except for RGB32, which is required.) > > > RGB8 and RGB16 are disabled/replaced by RGBX* on the host side, and > R11G11B10 doesn't seem to make problems. > No other driver supports these. What happens on the host side is hidden from virgl. You're just signing yourself up for weird issues down the line. > but thanks for the feedback anyway, > Gert > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
Am Freitag, den 20.07.2018, 12:31 -0400 schrieb Ilia Mirkin: > > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed for > > ARB_tbo_rgb32) */ > > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || > > + format == PIPE_FORMAT_R32G32B32_SINT || > > + format == PIPE_FORMAT_R32G32B32_UINT) && > > + target != PIPE_BUFFER) > > + return FALSE; > > My personal recommendation would be to disallow *all* packed RGB > formats unless target == PIPE_BUFFER. (And even then -- questionable, > except for RGB32, which is required.) > RGB8 and RGB16 are disabled/replaced by RGBX* on the host side, and R11G11B10 doesn't seem to make problems. but thanks for the feedback anyway, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
On Thu, Jul 12, 2018 at 6:55 AM, Gert Wollny wrote: > When requesting a texture of the internal format GL_RGB32F Gallium will > try to allocate a renderable texture and returns RGBA32F or RGBX32F, but > when one requests GL_RGB32I or GL_RGB32UI the according 3-component > texture will be returned. This leads to problems later, when one wants > to use glCopyImageSubData to copy data between these textures that should > be compatible, but given the way virgl and Gallium handle this the latter > fails with an assertion, because the per-texel bit size is different. > > By allowing the GL_RGB32* only for texture buffers these problems are avoided > without losing the ARB_tbo_rgb32 extension (thanks Ilia Mirkin). > > Signed-off-by: Gert Wollny > --- > src/gallium/drivers/virgl/virgl_screen.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/src/gallium/drivers/virgl/virgl_screen.c > b/src/gallium/drivers/virgl/virgl_screen.c > index 2a340b004f..1dbd88dee8 100644 > --- a/src/gallium/drivers/virgl/virgl_screen.c > +++ b/src/gallium/drivers/virgl/virgl_screen.c > @@ -496,6 +496,13 @@ virgl_is_format_supported( struct pipe_screen *screen, >return virgl_is_vertex_format_supported(screen, format); > } > > + /* Allow 3-comp 32 bit texturs only for TBOs (needed for ARB_tbo_rgb32) */ > + if ((format == PIPE_FORMAT_R32G32B32_FLOAT || > + format == PIPE_FORMAT_R32G32B32_SINT || > + format == PIPE_FORMAT_R32G32B32_UINT) && > + target != PIPE_BUFFER) > + return FALSE; My personal recommendation would be to disallow *all* packed RGB formats unless target == PIPE_BUFFER. (And even then -- questionable, except for RGB32, which is required.) Your call - that's a larger change (in behavior). -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
On Thu, Jul 12, 2018 at 3:56 AM Gert Wollny wrote:
>
> When requesting a texture of the internal format GL_RGB32F Gallium will
> try to allocate a renderable texture and returns RGBA32F or RGBX32F, but
> when one requests GL_RGB32I or GL_RGB32UI the according 3-component
> texture will be returned. This leads to problems later, when one wants
> to use glCopyImageSubData to copy data between these textures that should
> be compatible, but given the way virgl and Gallium handle this the latter
> fails with an assertion, because the per-texel bit size is different.
>
> By allowing the GL_RGB32* only for texture buffers these problems are avoided
> without losing the ARB_tbo_rgb32 extension (thanks Ilia Mirkin).
>
> Signed-off-by: Gert Wollny
> ---
> src/gallium/drivers/virgl/virgl_screen.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/gallium/drivers/virgl/virgl_screen.c
> b/src/gallium/drivers/virgl/virgl_screen.c
> index 2a340b004f..1dbd88dee8 100644
> --- a/src/gallium/drivers/virgl/virgl_screen.c
> +++ b/src/gallium/drivers/virgl/virgl_screen.c
> @@ -496,6 +496,13 @@ virgl_is_format_supported( struct pipe_screen *screen,
>return virgl_is_vertex_format_supported(screen, format);
> }
>
> + /* Allow 3-comp 32 bit texturs only for TBOs (needed for ARB_tbo_rgb32) */
nit: textures
> + if ((format == PIPE_FORMAT_R32G32B32_FLOAT ||
> + format == PIPE_FORMAT_R32G32B32_SINT ||
> + format == PIPE_FORMAT_R32G32B32_UINT) &&
> + target != PIPE_BUFFER)
> + return FALSE;
> +
> if (bind & PIPE_BIND_RENDER_TARGET) {
>if (format_desc->colorspace == UTIL_FORMAT_COLORSPACE_ZS)
> return FALSE;
> --
> 2.17.1
Just one small spelling nit (which a committer can fix), otherwise:
Reviewed-by: Gurchetan Singh
> ___
> 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
[Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects
When requesting a texture of the internal format GL_RGB32F Gallium will
try to allocate a renderable texture and returns RGBA32F or RGBX32F, but
when one requests GL_RGB32I or GL_RGB32UI the according 3-component
texture will be returned. This leads to problems later, when one wants
to use glCopyImageSubData to copy data between these textures that should
be compatible, but given the way virgl and Gallium handle this the latter
fails with an assertion, because the per-texel bit size is different.
By allowing the GL_RGB32* only for texture buffers these problems are avoided
without losing the ARB_tbo_rgb32 extension (thanks Ilia Mirkin).
Signed-off-by: Gert Wollny
---
src/gallium/drivers/virgl/virgl_screen.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/gallium/drivers/virgl/virgl_screen.c
b/src/gallium/drivers/virgl/virgl_screen.c
index 2a340b004f..1dbd88dee8 100644
--- a/src/gallium/drivers/virgl/virgl_screen.c
+++ b/src/gallium/drivers/virgl/virgl_screen.c
@@ -496,6 +496,13 @@ virgl_is_format_supported( struct pipe_screen *screen,
return virgl_is_vertex_format_supported(screen, format);
}
+ /* Allow 3-comp 32 bit texturs only for TBOs (needed for ARB_tbo_rgb32) */
+ if ((format == PIPE_FORMAT_R32G32B32_FLOAT ||
+ format == PIPE_FORMAT_R32G32B32_SINT ||
+ format == PIPE_FORMAT_R32G32B32_UINT) &&
+ target != PIPE_BUFFER)
+ return FALSE;
+
if (bind & PIPE_BIND_RENDER_TARGET) {
if (format_desc->colorspace == UTIL_FORMAT_COLORSPACE_ZS)
return FALSE;
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
