Bug 111454 - Gallium: removal of the manual defining of PIPE_FORMAT values breaks virgl
Summary: Gallium: removal of the manual defining of PIPE_FORMAT values breaks virgl
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Other (show other bugs)
Version: git
Hardware: All All
: medium critical
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: mesa-19.2
  Show dependency treegraph
 
Reported: 2019-08-21 11:20 UTC by Gert Wollny
Modified: 2019-09-05 07:39 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Gert Wollny 2019-08-21 11:20:52 UTC
Commit c45c33a5a2b75e791de14e845c994f414c77cc51 
  gallium: Remove manual defining of PIPE_FORMAT enum values.

breaks virgl, because virglrenderer holds a copy of that format list that must be in sync with the mesa version, so it would be nice to revert this. 

Adding a format translation to the virgl driver seems to me as a rather ugly option, and if we do it that it should be something that is done at the beginning of a release cycle.
Comment 1 Ilia Mirkin 2019-08-21 13:29:25 UTC
IMO that should just be backed out. PIPE_FORMAT enum values are used in TGSI code streams.
Comment 2 Marek Olšák 2019-08-21 15:07:30 UTC
Gallium is a private interface. Exposing Gallium externally is a mistake and misuse of the interface. There is nothing to do here.
Comment 3 Dave Airlie 2019-08-22 06:48:05 UTC
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1738

should fix it for now, as long as nobody renames format, if people do that we'd have to rewrite the shaders like we in some other cases.
Comment 4 Gert Wollny 2019-09-04 17:28:47 UTC
Fixed with  bba4d2f442f33bc68a4573a6f1f559f277d7ec51
Comment 5 Rob Clark 2019-09-04 22:34:04 UTC
(In reply to Ilia Mirkin from comment #1)
> IMO that should just be backed out. PIPE_FORMAT enum values are used in TGSI
> code streams.

I think the eventual goal is to unify PIPE_FORMAT_* and MESA_FORMAT_*, so I guess some further shuffling could happen..  do formats encoded in TGSI ever constitute some ABI?
Comment 6 Ilia Mirkin 2019-09-04 22:42:03 UTC
(In reply to Rob Clark from comment #5)
> (In reply to Ilia Mirkin from comment #1)
> > IMO that should just be backed out. PIPE_FORMAT enum values are used in TGSI
> > code streams.
> 
> I think the eventual goal is to unify PIPE_FORMAT_* and MESA_FORMAT_*, so I
> guess some further shuffling could happen..  do formats encoded in TGSI ever
> constitute some ABI?

You can argue it either way. Virgl uses (used?) this, presumably svga does too. The format is encoded in the TGSI ops for image declarations, and perhaps in the ops themselves too (due to bindless).

Whether TGSI forms a more permanent ABI or not ... up to us. I see it as a strong point of TGSI that it has this fairly stable well-defined wire format, but I also don't have the energy to fight it out.
Comment 7 Rob Clark 2019-09-04 23:04:47 UTC
(In reply to Ilia Mirkin from comment #6)
> (In reply to Rob Clark from comment #5)
> > (In reply to Ilia Mirkin from comment #1)
> > > IMO that should just be backed out. PIPE_FORMAT enum values are used in TGSI
> > > code streams.
> > 
> > I think the eventual goal is to unify PIPE_FORMAT_* and MESA_FORMAT_*, so I
> > guess some further shuffling could happen..  do formats encoded in TGSI ever
> > constitute some ABI?
> 
> You can argue it either way. Virgl uses (used?) this, presumably svga does
> too. The format is encoded in the TGSI ops for image declarations, and
> perhaps in the ops themselves too (due to bindless).
> 
> Whether TGSI forms a more permanent ABI or not ... up to us. I see it as a
> strong point of TGSI that it has this fairly stable well-defined wire
> format, but I also don't have the energy to fight it out.

looks like svga does not handle images (yet?).. but it does some amount of translating the tgsi, so I guess it could remap encoded formats to some stable over-the-wire format.

But virgl does, and seems to pass the tgsi straight thru.  Which makes me suspect it is still broken for shaders that use images :-/
Comment 8 Gert Wollny 2019-09-05 07:39:58 UTC
Virgl sends the TGSI as text so I think we are save. 

At least Dave and me we run the patches through piglit and the virglrenderer CI on a hardware host (the fdo one only used softpipe) and there all passes as expected.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.