[v2,02/12] genxml: Preserve fields that share dword space with addresses.

Submitted by Rafael Antognolli on Jan. 19, 2018, 7:54 p.m.

Details

Message ID 20180119195447.17349-3-rafael.antognolli@intel.com
State New
Headers show
Series "Use clear color address in surface state." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli Jan. 19, 2018, 7:54 p.m.
Some instructions contain fields that are either an address or a value
of some type based on the content of other fields, such as clear color
values vs address. That works fine if these fields are in the less
significant dword, the lower 32 bits of the address, because they get
OR'ed with the address. But if they are in the higher 32 bits, they get
discarded.

On Gen10 we have fields that share space with the higher 16 bits of the
address too. This commit makes sure those fields don't get discarded.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 src/intel/genxml/gen_pack_header.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/genxml/gen_pack_header.py b/src/intel/genxml/gen_pack_header.py
index e6cea8646ff..e81695e2aea 100644
--- a/src/intel/genxml/gen_pack_header.py
+++ b/src/intel/genxml/gen_pack_header.py
@@ -486,11 +486,16 @@  class Group(object):
                 v_address = "v%d_address" % index
                 print("   const uint64_t %s =\n      __gen_combine_address(data, &dw[%d], values->%s, %s);" %
                       (v_address, index, dw.address.name + field.dim, v))
-                v = v_address
-
+                if len(dw.fields) > address_count:
+                    print("   dw[%d] = %s;" % (index, v_address))
+                    print("   dw[%d] = (%s >> 32) | (%s >> 32);" % (index + 1, v_address, v))
+                    continue
+                else:
+                    v = v_address
             print("   dw[%d] = %s;" % (index, v))
             print("   dw[%d] = %s >> 32;" % (index + 1, v))
 
+
 class Value(object):
     def __init__(self, attrs):
         self.name = safe_name(attrs["name"])

Comments

On Fri, Jan 19, 2018 at 11:54:37AM -0800, Rafael Antognolli wrote:
> Some instructions contain fields that are either an address or a value
> of some type based on the content of other fields, such as clear color
> values vs address. That works fine if these fields are in the less
> significant dword, the lower 32 bits of the address, because they get
> OR'ed with the address. But if they are in the higher 32 bits, they get
> discarded.
> 
> On Gen10 we have fields that share space with the higher 16 bits of the
> address too. This commit makes sure those fields don't get discarded.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  src/intel/genxml/gen_pack_header.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/intel/genxml/gen_pack_header.py b/src/intel/genxml/gen_pack_header.py
> index e6cea8646ff..e81695e2aea 100644
> --- a/src/intel/genxml/gen_pack_header.py
> +++ b/src/intel/genxml/gen_pack_header.py
> @@ -486,11 +486,16 @@ class Group(object):
>                  v_address = "v%d_address" % index
>                  print("   const uint64_t %s =\n      __gen_combine_address(data, &dw[%d], values->%s, %s);" %
>                        (v_address, index, dw.address.name + field.dim, v))
> -                v = v_address
> -
> +                if len(dw.fields) > address_count:
> +                    print("   dw[%d] = %s;" % (index, v_address))
> +                    print("   dw[%d] = (%s >> 32) | (%s >> 32);" % (index + 1, v_address, v))
> +                    continue
> +                else:
> +                    v = v_address
>              print("   dw[%d] = %s;" % (index, v))
>              print("   dw[%d] = %s >> 32;" % (index + 1, v))

I'm wondering if we could have left the "continue" out and write the
else-branch directly just like we did if:

               print("   dw[%d] = %s;" % (index, v_address))
               print("   dw[%d] = %s >> 32;" % (index + 1, v_address))

>  
> +
>  class Value(object):
>      def __init__(self, attrs):
>          self.name = safe_name(attrs["name"])
> -- 
> 2.14.3
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wed, Jan 24, 2018 at 11:20:07AM +0200, Pohjolainen, Topi wrote:
> On Fri, Jan 19, 2018 at 11:54:37AM -0800, Rafael Antognolli wrote:
> > Some instructions contain fields that are either an address or a value
> > of some type based on the content of other fields, such as clear color
> > values vs address. That works fine if these fields are in the less
> > significant dword, the lower 32 bits of the address, because they get
> > OR'ed with the address. But if they are in the higher 32 bits, they get
> > discarded.
> > 
> > On Gen10 we have fields that share space with the higher 16 bits of the
> > address too. This commit makes sure those fields don't get discarded.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  src/intel/genxml/gen_pack_header.py | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/intel/genxml/gen_pack_header.py b/src/intel/genxml/gen_pack_header.py
> > index e6cea8646ff..e81695e2aea 100644
> > --- a/src/intel/genxml/gen_pack_header.py
> > +++ b/src/intel/genxml/gen_pack_header.py
> > @@ -486,11 +486,16 @@ class Group(object):
> >                  v_address = "v%d_address" % index
> >                  print("   const uint64_t %s =\n      __gen_combine_address(data, &dw[%d], values->%s, %s);" %
> >                        (v_address, index, dw.address.name + field.dim, v))
> > -                v = v_address
> > -
> > +                if len(dw.fields) > address_count:
> > +                    print("   dw[%d] = %s;" % (index, v_address))
> > +                    print("   dw[%d] = (%s >> 32) | (%s >> 32);" % (index + 1, v_address, v))
> > +                    continue
> > +                else:
> > +                    v = v_address
> >              print("   dw[%d] = %s;" % (index, v))
> >              print("   dw[%d] = %s >> 32;" % (index + 1, v))
> 
> I'm wondering if we could have left the "continue" out and write the
> else-branch directly just like we did if:
> 
>                print("   dw[%d] = %s;" % (index, v_address))
>                print("   dw[%d] = %s >> 32;" % (index + 1, v_address))

Hi Topi,

I was rebasing the series on top of master and while applying your
suggestion, I just noticed it's not gonna work. Notice that the last 2
lines are executed both when the else branch is taken, or when the outer
"if dw.address:" is not taken. If I do as you suggest, that last case
won't be covered.

I know it looks really ugly, I'll try to think of a better way to do
this, but for now I'll just submit the updated series with this version
again to get reviews on other stuff.

Thanks for the review anyway.

> >  
> > +
> >  class Value(object):
> >      def __init__(self, attrs):
> >          self.name = safe_name(attrs["name"])
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev