[Mesa-dev] [PATCH] virgl: Allow RGB32* textures only as buffer objects

[Gert Wollny]

Am Samstag, den 21.07.2018, 14:14 -0400 schrieb Ilia Mirkin:
> On Sat, Jul 21, 2018 at 1:48 PM, Gert Wollny <gert.wollny at collabora.c
> om> wrote:
> > Am Samstag, den 21.07.2018, 11:33 -0400 schrieb Ilia Mirkin:
> > > 
> > > 
> > > On Sat, Jul 21, 2018, 05:45 Gert Wollny <gert.wollny at collabora.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. 

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