Bug 92687 - Add support for ARB_internalformat_query2
Summary: Add support for ARB_internalformat_query2
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Eduardo Lima Mitev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-27 08:23 UTC by Eduardo Lima Mitev
Modified: 2016-03-04 09:31 UTC (History)
8 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 Eduardo Lima Mitev 2015-10-27 08:23:37 UTC
This issue is to track progress for the implementation of EXT_internalformat_query2 extension in Mesa:

https://www.opengl.org/registry/specs/ARB/internalformat_query2.txt

This task involves adding the new GL API and symbols, driver hooks and default values, and piglit tests.
Comment 1 Eduardo Lima Mitev 2015-12-23 13:11:55 UTC
About time for an update! The "we" below refers to Antía, Alejandro and I, as the team working primarily on this extension for almost 2 months now.

From a high level point of view, we have completed more or less all the implementation bits. This means that all the code-flow and plumbery is already in place, but we still need to iterate on the implementation of specific queries. There are some queries for which we have doubts related to the wording in the spec, others about what the correct answer from Mesa should be, etc. We will post these issues/questions separately, in another update.

The current code can be checked at:

Mesa: https://github.com/Igalia/mesa/tree/internalformat-query2-rfc
Piglit: https://github.com/Igalia/piglit/tree/internalformat-query2-rfc

From a high level point of view, the structure and changes are:

* All the extension's frontend code is in main/formatquery.c, as it was before for query1. Only that it also handles query2 now.

* A new driver hook 'QueryInternalFormat' was added, replacing the previous one 'QuerySamplesForFormat'. The first commits in the branch make this change, preparing the stage for the query2 specific stuff that come after.

* A fallback, generic function _mesa_query_internal_format_default() provides generic implementation and sensible defaults for all queries, for drivers not implementing query2. Backends that only care about answering some queries, can call back this function for the other queries where a generic answer is ok.

* For all pnames, the frontend code will do generic validation as per the spec: check GL profile, version, extensions.
  - If the frontend fails basic validation, it will give the corresponding negative answer, depending on the pname, without going to the driver.
  - If the frontend is fully qualified to provide an answer, it will (i.e, MAX_WIDTH, COLOR_COMPONENTS, etc). Otherwise it will call the driver hook (i.e, INTERNALFORMAT_PREFERRED).
  - For the cases where the query must return full support, caveat support, or no support; Mesa/main will always call the driver to decide between full or caveat support (and only answer directly in the case of no-support).

* The last patches in the branch enable support for this extension in i965 backend (drivers/dri/i965/brw_formatquery.c). The backend code only handle queries where the answer is affected by driver-specific stuff. But by default, it calls back the frontend function with the default implementations.

The piglit branch adds basic tests that cover all queries. We could write more test cases and improve the coverage for more corner cases, but at this point we think an RFC round is needed to give us focus on this front too.

Not all piglit tests pass with our Mesa branch due to some issues that we will post soon. To run query2 tests only, the filter "-t internalformat_query2" can be used.
Comment 2 Eduardo Lima Mitev 2015-12-23 18:40:25 UTC
I forgot to mention that we implement the 64 bits version of the query introduced by this extension (GetInternalformati64v), as a wrapper around the 32 bits version. Since only one query really requires the 64 bits API (MAX_COMBINED_DIMENSIONS), we handle that pname as a special case. For the rest of queries, we just forward the call to the default, 32 bits version.


Following, there are some initial issues/questions we have been gathering:

* MAX_COMBINED_DIMENSIONS: the spec says "Note that the value returned can be >= 2^32 and should be queried with the 64-bit query.". In spite of this "should",  no error is mentioned if you don't use the 64-bit query (even reading Issues 5 and 8 about this pname). So the conclusion (for now) is that it is allowed. What it is not clarified is what the non-64-bit query should return is the value of MAX_COMBINED_DIMENSIONS is greater that 2^32 (NVIDIA returns 0). It is ok if we return a clamped value? It is better to return 0 as NVIDIA does (this somehow hints that something is wrong).

i965 specific:

* What known caveat supports do we have in i965? The spec defines this as:

CAVEAT_SUPPORT: the requested capability is supported by the implementation, but there may be some implementation-specific caveats that make support less than optimal. For example using the feature may result in reduced performance (relative to other formats or features), such as software rendering or other mechanisms of emulating the desired feature. 

* i965 doesn't override GenerateMipmap driver hook. It is using the default  implementation provided in mesa/main/mipmap.c. Does this mean it has CAVEAT support for mipmapping? It is software generated, so I have doubts.

* For INTERNALFORMAT_PREFERRED, the driver should return a "preferred" internal format that is compatible with the one given by the user as argument, and that has at least the same precision. Currently, what we do is to return the same internal format that was passed (same as NVidia proprietary), checking that it has a matching BRW format. For this, we first obtain a mesa_format from the internal format using a generic type, then call brw_format_for_mesa_format(). I have a lot of doubts about this.
So the questions are: does this make sense? What would be the criteria to choose the preferred internal format for i965? I could not trivially find a case where we have a better candidate than the passed internal format. 

Piglit related:

* There are some pnames that explicitly mention that if a resource is not supported, the returned value is zero. This is properly implemented on the query2 implementation, but we would like to test that on the piglit tests. But there isn't any equivalent pname to get this info (something like RESOURCE_SUPPORTED). Right now we are checking against INTERNALFORMAT_SUPPORTED, but this fails on cases where the internalformat is supported but not for a specific combination of target/internalformat. We are not sure how to deal with this, because the straightforward solution would be implement a equivalent to _is_resource_supported on piglit, but that sounds overkill.

Thanks for the feedback!
Comment 3 Eduardo Lima Mitev 2016-01-13 11:41:33 UTC
(In reply to Eduardo Lima Mitev from comment #2)
> 
> Following, there are some initial issues/questions we have been gathering:
> 

Independently of feedback to the branch we posted, it would be very useful to get insights on the issues/questions above, which are most of them generic.

In any case, we plan to send the branch as an RFC series to mesa-dev list soon, (e.g, end of this week).
Comment 4 Eduardo Lima Mitev 2016-01-21 17:01:59 UTC
The series was sent to mesa-dev early this week, as RFC:

http://lists.freedesktop.org/archives/mesa-dev/2016-January/105277.html
Comment 5 Alejandro Piñeiro (freenode IRC: apinheiro) 2016-01-22 17:14:06 UTC
(In reply to Eduardo Lima Mitev from comment #1)
> About time for an update! The "we" below refers to Antía, Alejandro and I,
> as the team working primarily on this extension for almost 2 months now.
> 
> From a high level point of view, we have completed more or less all the
> implementation bits. This means that all the code-flow and plumbery is
> already in place, but we still need to iterate on the implementation of
> specific queries. There are some queries for which we have doubts related to
> the wording in the spec, others about what the correct answer from Mesa
> should be, etc. We will post these issues/questions separately, in another
> update.
> 
> The current code can be checked at:
> 
> Mesa: https://github.com/Igalia/mesa/tree/internalformat-query2-rfc
> Piglit: https://github.com/Igalia/piglit/tree/internalformat-query2-rfc

A ready to review (no RFC) piglit series was sent to the mailing list:
http://lists.freedesktop.org/archives/piglit/2016-January/018777.html

In case you want to download all the patches from a repository, instead of the rfc branch, you can use this one:
https://github.com/Igalia/piglit/tree/internalformat-query2
Comment 6 Eduardo Lima Mitev 2016-03-03 14:34:33 UTC
I have just pushed the series to master, after review by Dave Airlie (thanks a bunch!).

I will delay closing this bug as fixed until we get the piglit test series in as well:
https://lists.freedesktop.org/archives/piglit/2016-January/018777.html
Comment 7 Alejandro Piñeiro (freenode IRC: apinheiro) 2016-03-04 09:18:29 UTC
(In reply to Eduardo Lima Mitev from comment #6)
> I have just pushed the series to master, after review by Dave Airlie (thanks
> a bunch!).
> 
> I will delay closing this bug as fixed until we get the piglit test series
> in as well:
> https://lists.freedesktop.org/archives/piglit/2016-January/018777.html

Dave Airlie just gave an ack to the piglit series (thanks a bunch x 2). Just pushed. 

Although I think that it is safe to close the bug, I will let Eduardo close the bug, as it is still assigned to him.
Comment 8 Eduardo Lima Mitev 2016-03-04 09:31:22 UTC
Closing as fixed.

Thanks everyone!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.