[4/5] add a cap for TGSI precise modifiers

Submitted by Erik Faye-Lund on July 12, 2018, 7:55 a.m.

Details

Message ID 20180712075539.11993-5-erik.faye-lund@collabora.com
State New
Headers show
Series "tesselation fixes" ( rev: 1 ) in Virgil 3D

Not browsing as part of any series.

Commit Message

Erik Faye-Lund July 12, 2018, 7:55 a.m.
This way, mesa can know that the TGSI-parser groks the _PRECISE
modifier, and it doesn't need to cull them any more.

Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
---
 src/virgl_hw.h       | 1 +
 src/vrend_renderer.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/virgl_hw.h b/src/virgl_hw.h
index 44c7108..bbd4d38 100644
--- a/src/virgl_hw.h
+++ b/src/virgl_hw.h
@@ -215,6 +215,7 @@  enum virgl_formats {
 #define VIRGL_CAP_NONE 0
 #define VIRGL_CAP_TGSI_INVARIANT       (1 << 0)
 #define VIRGL_CAP_TEXTURE_VIEW         (1 << 1)
+#define VIRGL_CAP_TGSI_PRECISE         (1 << 2)
 
 struct virgl_caps_bool_set1 {
         unsigned indep_blend_enable:1;
diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
index cc99087..388f08d 100644
--- a/src/vrend_renderer.c
+++ b/src/vrend_renderer.c
@@ -7550,7 +7550,7 @@  void vrend_renderer_fill_caps(uint32_t set, uint32_t version,
 
    caps->v1.max_samples = vrend_renderer_query_multisample_caps(max, &caps->v2);
 
-   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT;
+   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT | VIRGL_CAP_TGSI_PRECISE;
 
    if (gl_ver >= 43 || epoxy_has_gl_extension("GL_ARB_texture_view"))
       caps->v2.capability_bits |= VIRGL_CAP_TEXTURE_VIEW;

Comments

Am Donnerstag, den 12.07.2018, 09:55 +0200 schrieb Erik Faye-Lund:
> This way, mesa can know that the TGSI-parser groks the _PRECISE
> modifier, and it doesn't need to cull them any more.
> 
> Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
> ---
>  src/virgl_hw.h       | 1 +
>  src/vrend_renderer.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/virgl_hw.h b/src/virgl_hw.h
> index 44c7108..bbd4d38 100644
> --- a/src/virgl_hw.h
> +++ b/src/virgl_hw.h
> @@ -215,6 +215,7 @@ enum virgl_formats {
>  #define VIRGL_CAP_NONE 0
>  #define VIRGL_CAP_TGSI_INVARIANT       (1 << 0)
>  #define VIRGL_CAP_TEXTURE_VIEW         (1 << 1)
> +#define VIRGL_CAP_TGSI_PRECISE         (1 << 2)
We should agree on the numbers here, Dave's experimental tree uses
(1<<2) already for compute shaders, and I added (1<<3) for copy_image,
so (1 << 4) would be the next one. 


>  
>  struct virgl_caps_bool_set1 {
>          unsigned indep_blend_enable:1;
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index cc99087..388f08d 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -7550,7 +7550,7 @@ void vrend_renderer_fill_caps(uint32_t set,
> uint32_t version,
>  
>     caps->v1.max_samples = vrend_renderer_query_multisample_caps(max,
> &caps->v2);
>  
> -   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT;
> +   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT |
> VIRGL_CAP_TGSI_PRECISE;
>  
>     if (gl_ver >= 43 ||
> epoxy_has_gl_extension("GL_ARB_texture_view"))
>        caps->v2.capability_bits |= VIRGL_CAP_TEXTURE_VIEW;
On 12. juli 2018 14:35, Gert Wollny wrote:
> Am Donnerstag, den 12.07.2018, 09:55 +0200 schrieb Erik Faye-Lund:
>> This way, mesa can know that the TGSI-parser groks the _PRECISE
>> modifier, and it doesn't need to cull them any more.
>>
>> Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
>> ---
>>   src/virgl_hw.h       | 1 +
>>   src/vrend_renderer.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/virgl_hw.h b/src/virgl_hw.h
>> index 44c7108..bbd4d38 100644
>> --- a/src/virgl_hw.h
>> +++ b/src/virgl_hw.h
>> @@ -215,6 +215,7 @@ enum virgl_formats {
>>   #define VIRGL_CAP_NONE 0
>>   #define VIRGL_CAP_TGSI_INVARIANT       (1 << 0)
>>   #define VIRGL_CAP_TEXTURE_VIEW         (1 << 1)
>> +#define VIRGL_CAP_TGSI_PRECISE         (1 << 2)
> We should agree on the numbers here, Dave's experimental tree uses
> (1<<2) already for compute shaders, and I added (1<<3) for copy_image,
> so (1 << 4) would be the next one.
>
I don't think we need to. We could just go first-come-first-serve, and 
let the merge-to-master point decide the number.

Synchronizing this seems like more work than just rebasing. Besides, it 
only solves half of the problem; we have a similar issue with the 
ordering of other caps, and I don't think there's anything else that 
really works than merge-point.

Maybe we could make it a bit easier to deal with by not having the 
bitflags in the caps, but an enum instead.
Something like this:

---8<---
enum virgl_cap {
     VIRGL_CAP_TGSI_INVARIANT = 0,
     VIRGL_CAP_TEXTURE_VIEW,
     VIRGL_CAP_TEXTURE_PRECISE
}

inline bool virgl_has_cap(union virgl_caps caps, enum virgl_cap cap)
{
     return caps.v2.capability_bits & (1 << (int)cap);
}
---8<---

We still need to keep the order up-to-date, but that might be a bit 
easier than having to change the values as well... But on the other 
hand, it's a lot of churn for a kinda hypothetical gain...
On 12. juli 2018 14:50, Erik Faye-Lund wrote:
> On 12. juli 2018 14:35, Gert Wollny wrote:
>> Am Donnerstag, den 12.07.2018, 09:55 +0200 schrieb Erik Faye-Lund:
>>> This way, mesa can know that the TGSI-parser groks the _PRECISE
>>> modifier, and it doesn't need to cull them any more.
>>>
>>> Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
>>> ---
>>>   src/virgl_hw.h       | 1 +
>>>   src/vrend_renderer.c | 2 +-
>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/virgl_hw.h b/src/virgl_hw.h
>>> index 44c7108..bbd4d38 100644
>>> --- a/src/virgl_hw.h
>>> +++ b/src/virgl_hw.h
>>> @@ -215,6 +215,7 @@ enum virgl_formats {
>>>   #define VIRGL_CAP_NONE 0
>>>   #define VIRGL_CAP_TGSI_INVARIANT       (1 << 0)
>>>   #define VIRGL_CAP_TEXTURE_VIEW         (1 << 1)
>>> +#define VIRGL_CAP_TGSI_PRECISE         (1 << 2)
>> We should agree on the numbers here, Dave's experimental tree uses
>> (1<<2) already for compute shaders, and I added (1<<3) for copy_image,
>> so (1 << 4) would be the next one.
>>
> I don't think we need to. We could just go first-come-first-serve, and 
> let the merge-to-master point decide the number.
>
> Synchronizing this seems like more work than just rebasing. Besides, 
> it only solves half of the problem; we have a similar issue with the 
> ordering of other caps, and I don't think there's anything else that 
> really works than merge-point.
>
> Maybe we could make it a bit easier to deal with by not having the 
> bitflags in the caps, but an enum instead.
> Something like this:
>
> ---8<---
> enum virgl_cap {
>     VIRGL_CAP_TGSI_INVARIANT = 0,
>     VIRGL_CAP_TEXTURE_VIEW,
>     VIRGL_CAP_TEXTURE_PRECISE
> }
>
> inline bool virgl_has_cap(union virgl_caps caps, enum virgl_cap cap)
> {
>     return caps.v2.capability_bits & (1 << (int)cap);
> }
> ---8<---
>
> We still need to keep the order up-to-date, but that might be a bit 
> easier than having to change the values as well... But on the other 
> hand, it's a lot of churn for a kinda hypothetical gain...

Thinking about this a bit more (it is rather painful to not be able to 
easily add caps in any order), it'd have been a lot more neat if the 
caps were not bitflags, but string-lists instead. That's a lot easier to 
gracefully extend. Similarly, I think it'd been nice to have simple 
key-value properties for all constants we want.

Oh well, I guess this ship has kinda sailed...
Am Donnerstag, den 12.07.2018, 15:43 +0200 schrieb Erik Faye-Lund:
> 
> Thinking about this a bit more (it is rather painful to not be able
> to easily add caps in any order), it'd have been a lot more neat if
> the  caps were not bitflags, but string-lists instead. That's a lot
> easier to gracefully extend. Similarly, I think it'd been nice to
> have simple key-value properties for all constants we want.
Indeed, that would have been better. 

Another alternative would be a header file that is shared between
mesa/virgl and virglrenderer, so that one has to add things in only one
place ...
On 12. juli 2018 15:54, Gert Wollny wrote:
> Am Donnerstag, den 12.07.2018, 15:43 +0200 schrieb Erik Faye-Lund:
>> Thinking about this a bit more (it is rather painful to not be able
>> to easily add caps in any order), it'd have been a lot more neat if
>> the  caps were not bitflags, but string-lists instead. That's a lot
>> easier to gracefully extend. Similarly, I think it'd been nice to
>> have simple key-value properties for all constants we want.
> Indeed, that would have been better.
>
> Another alternative would be a header file that is shared between
> mesa/virgl and virglrenderer, so that one has to add things in only one
> place ...
>
Well, that's kinda what we have. It's just diverged a bit. I'm working 
on some patches to get them back in sync.
Am Donnerstag, den 12.07.2018, 15:56 +0200 schrieb Erik Faye-Lund:
> 
> > Another alternative would be a header file that is shared between
> > mesa/virgl and virglrenderer, so that one has to add things in only
> > one
> > place ...
> > 
> 
> Well, that's kinda what we have. It's just diverged a bit. I'm
> working on some patches to get them back in sync. 

What I meant was literally one file, not two files that life in
different projects and have to be kept in sync manually :)
On 12. juli 2018 17:12, Gert Wollny wrote:
> Am Donnerstag, den 12.07.2018, 15:56 +0200 schrieb Erik Faye-Lund:
>>> Another alternative would be a header file that is shared between
>>> mesa/virgl and virglrenderer, so that one has to add things in only
>>> one
>>> place ...
>>>
>> Well, that's kinda what we have. It's just diverged a bit. I'm
>> working on some patches to get them back in sync.
> What I meant was literally one file, not two files that life in
> different projects and have to be kept in sync manually :)
Sure, that'd be nice. I just don't know how we'd make that happen, 
since, well, two repos need this... Perhaps virglrenderer could be 
installed an be a build-time dependency for virgl in mesa or something?
On Fri., 13 Jul. 2018, 01:19 Erik Faye-Lund, <erik.faye-lund@collabora.com>
wrote:

> On 12. juli 2018 17:12, Gert Wollny wrote:
> > Am Donnerstag, den 12.07.2018, 15:56 +0200 schrieb Erik Faye-Lund:
> >>> Another alternative would be a header file that is shared between
> >>> mesa/virgl and virglrenderer, so that one has to add things in only
> >>> one
> >>> place ...
> >>>
> >> Well, that's kinda what we have. It's just diverged a bit. I'm
> >> working on some patches to get them back in sync.
> > What I meant was literally one file, not two files that life in
> > different projects and have to be kept in sync manually :)
> Sure, that'd be nice. I just don't know how we'd make that happen,
> since, well, two repos need this... Perhaps virglrenderer could be
> installed an be a build-time dependency for virgl in mesa or something?
>

Let's not overthink it, we aren't that far off finished feature wise,
review and rebase often should see us through.

Dave.
On Thu, Jul 12, 2018 at 12:56 AM Erik Faye-Lund
<erik.faye-lund@collabora.com> wrote:
>
> This way, mesa can know that the TGSI-parser groks the _PRECISE
> modifier, and it doesn't need to cull them any more.
>
> Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
> ---
>  src/virgl_hw.h       | 1 +
>  src/vrend_renderer.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/virgl_hw.h b/src/virgl_hw.h
> index 44c7108..bbd4d38 100644
> --- a/src/virgl_hw.h
> +++ b/src/virgl_hw.h
> @@ -215,6 +215,7 @@ enum virgl_formats {
>  #define VIRGL_CAP_NONE 0
>  #define VIRGL_CAP_TGSI_INVARIANT       (1 << 0)
>  #define VIRGL_CAP_TEXTURE_VIEW         (1 << 1)
> +#define VIRGL_CAP_TGSI_PRECISE         (1 << 2)

Is there a Mesa side patch that makes use of VIRGL_CAP_TGSI_PRECISE?
We don't really do anything with VIRGL_CAP_TGSI_INVARIANT, even though
send over the wire.  Can you send out a patch that makes use of both
of them?

>
>  struct virgl_caps_bool_set1 {
>          unsigned indep_blend_enable:1;
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index cc99087..388f08d 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -7550,7 +7550,7 @@ void vrend_renderer_fill_caps(uint32_t set, uint32_t version,
>
>     caps->v1.max_samples = vrend_renderer_query_multisample_caps(max, &caps->v2);
>
> -   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT;
> +   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT | VIRGL_CAP_TGSI_PRECISE;
>
>     if (gl_ver >= 43 || epoxy_has_gl_extension("GL_ARB_texture_view"))
>        caps->v2.capability_bits |= VIRGL_CAP_TEXTURE_VIEW;
> --
> 2.18.0.rc2
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
On 13. juli 2018 17:50, Gurchetan Singh wrote:
> On Thu, Jul 12, 2018 at 12:56 AM Erik Faye-Lund
> <erik.faye-lund@collabora.com> wrote:
>> This way, mesa can know that the TGSI-parser groks the _PRECISE
>> modifier, and it doesn't need to cull them any more.
>>
>> Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
>> ---
>>   src/virgl_hw.h       | 1 +
>>   src/vrend_renderer.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/virgl_hw.h b/src/virgl_hw.h
>> index 44c7108..bbd4d38 100644
>> --- a/src/virgl_hw.h
>> +++ b/src/virgl_hw.h
>> @@ -215,6 +215,7 @@ enum virgl_formats {
>>   #define VIRGL_CAP_NONE 0
>>   #define VIRGL_CAP_TGSI_INVARIANT       (1 << 0)
>>   #define VIRGL_CAP_TEXTURE_VIEW         (1 << 1)
>> +#define VIRGL_CAP_TGSI_PRECISE         (1 << 2)
> Is there a Mesa side patch that makes use of VIRGL_CAP_TGSI_PRECISE?
> We don't really do anything with VIRGL_CAP_TGSI_INVARIANT, even though
> send over the wire.  Can you send out a patch that makes use of both
> of them?

Yes, I have a branch for that, I should of course have mentioned this in 
the cover-letter:

https://gitlab.freedesktop.org/kusma/mesa/tree/virgl-precise

I noticed that we don't do anything for invariant, but I don't know what 
we'd want to do... I wasn't involved in the patches that introduced that 
feature, so I don't really know what went on.

>>   struct virgl_caps_bool_set1 {
>>           unsigned indep_blend_enable:1;
>> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
>> index cc99087..388f08d 100644
>> --- a/src/vrend_renderer.c
>> +++ b/src/vrend_renderer.c
>> @@ -7550,7 +7550,7 @@ void vrend_renderer_fill_caps(uint32_t set, uint32_t version,
>>
>>      caps->v1.max_samples = vrend_renderer_query_multisample_caps(max, &caps->v2);
>>
>> -   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT;
>> +   caps->v2.capability_bits |= VIRGL_CAP_TGSI_INVARIANT | VIRGL_CAP_TGSI_PRECISE;
>>
>>      if (gl_ver >= 43 || epoxy_has_gl_extension("GL_ARB_texture_view"))
>>         caps->v2.capability_bits |= VIRGL_CAP_TEXTURE_VIEW;
>> --
>> 2.18.0.rc2
>>
>> _______________________________________________
>> virglrenderer-devel mailing list
>> virglrenderer-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel