[Mesa-dev] [PATCH] intel/genxml: Canonicalize addresses before writing them

[Jason Ekstrand]

On Thu, May 11, 2017 at 9:54 AM, Chris Wilson <chris at chris-wilson.co.uk>
wrote:

> On Thu, May 11, 2017 at 09:13:55AM -0700, Jason Ekstrand wrote:
> > This doesn't actually fix any 48-bit bugs because the addresses provided
> > to us from the kernel are already sign-extended.  However, it still
> > seems like a good idea.
>
> addr=1<<47-4096, delta=4096
>

Ugh... You're right.  I'll update the commit message with that and add a CC
to stable.


> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  src/intel/genxml/gen_pack_header.py | 34 ++++++++++++++++++++++++++++++
> +---
> >  1 file changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/intel/genxml/gen_pack_header.py
> b/src/intel/genxml/gen_pack_header.py
> > index 020dbe4..df71689 100644
> > --- a/src/intel/genxml/gen_pack_header.py
> > +++ b/src/intel/genxml/gen_pack_header.py
> > @@ -168,6 +168,31 @@ __gen_ufixed(float v, uint32_t start, uint32_t end,
> uint32_t fract_bits)
> >  #error #define __gen_combine_address before including this file
> >  #endif
> >
> > +static inline uint64_t
> > +__gen_address(__gen_user_data *data, void *location, __gen_address_type
> addr,
> > +              uint32_t delta, uint32_t start, uint32_t end)
> > +{
> > +   __gen_validate_value(addr);
> > +
> > +   uint64_t offset = __gen_combine_address(data, location, addr, delta);
> > +
> > +   if (end >= 32) {
> > +      /* From the Broadwell PRM Vol. 2a, MI_LOAD_REGISTER_MEM::
> MemoryAddress:
> > +       *
> > +       *    "This field specifies the address of the memory location
> where the
> > +       *    register value specified in the DWord above will read from.
> The
> > +       *    address specifies the DWord location of the data. Range =
> > +       *    GraphicsVirtualAddress[63:2] for a DWord register
> GraphicsAddress
> > +       *    [63:48] are ignored by the HW and assumed to be in correct
> > +       *    canonical form [63:48] == [47]."
> > +       */
> > +      const int shift = 63 - 47;
> > +      return (((int64_t)offset) << shift) >> shift;
>
> I presume that start/end are known at compile time, so the branch here
> is just a figment of my imagination.
>

Yes.  The function is declared inline and all the arguments are constants.
There is a chance GCC will decide to not inline it but I've never seen that
happen.  (And, yes, I do look at objdump of these things on a reasonably
regular basis.)


> There's no chance of having the shift + comments pulled in from a header?
>

Not sure what you mean by that.  I mean, we could add a header to
src/intel/common to store some of these things.  Not sure what else would
go in there, but I wouldn't be opposed to starting one.


> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>

Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170511/b917bdc4/attachment.html>