Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-04 Thread Kenneth Graunke 
On Tuesday, April 3, 2018 11:56:27 PM PDT Sergii Romantsov wrote:
> Hello, Mark.
> I've done: Cc, Tested-by and Reviewed-by also added.

I pushed your patch.  Thanks so much for tracking this down!


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-04 Thread Sergii Romantsov 
Hello, Mark.
I've done: Cc, Tested-by and Reviewed-by also added.

On Wed, Apr 4, 2018 at 8:16 AM, Mark Janes  wrote:

> This patch passes Intel's CI suites.
> It needs a CC for stable in the commit message.
>
> Tested-by: Mark Janes 
>
> Sergii Romantsov  writes:
>
> > Gen8+ use 48-bit address relocations so need to extend the sign
> > to 64-bit return value. Without it we have higher bits zeroed
> > and missing the negavive values.
> > Haswell and older use 32-bit deltas so are unaffected by this issue.
> >
> > v2:
> >   used int32_t fucntion parameter instead of explicit type conversion.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> > Signed-off-by: Sergii Romantsov 
> > Tested-by: Andriy Khulap 
> > ---
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index ebc02ff..7286140 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -1079,7 +1079,7 @@ brw_batch_references(struct intel_batchbuffer
> *batch, struct brw_bo *bo)
> >  static uint64_t
> >  emit_reloc(struct intel_batchbuffer *batch,
> > struct brw_reloc_list *rlist, uint32_t offset,
> > -   struct brw_bo *target, uint32_t target_offset,
> > +   struct brw_bo *target, int32_t target_offset,
> > unsigned int reloc_flags)
> >  {
> > assert(target != NULL);
> > --
> > 2.7.4
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-03 Thread Mark Janes 
This patch passes Intel's CI suites.
It needs a CC for stable in the commit message.

Tested-by: Mark Janes 

Sergii Romantsov  writes:

> Gen8+ use 48-bit address relocations so need to extend the sign
> to 64-bit return value. Without it we have higher bits zeroed
> and missing the negavive values.
> Haswell and older use 32-bit deltas so are unaffected by this issue.
>
> v2:
>   used int32_t fucntion parameter instead of explicit type conversion.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> Signed-off-by: Sergii Romantsov 
> Tested-by: Andriy Khulap 
> ---
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index ebc02ff..7286140 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -1079,7 +1079,7 @@ brw_batch_references(struct intel_batchbuffer *batch, 
> struct brw_bo *bo)
>  static uint64_t
>  emit_reloc(struct intel_batchbuffer *batch,
> struct brw_reloc_list *rlist, uint32_t offset,
> -   struct brw_bo *target, uint32_t target_offset,
> +   struct brw_bo *target, int32_t target_offset,
> unsigned int reloc_flags)
>  {
> assert(target != NULL);
> -- 
> 2.7.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-03 Thread Emil Velikov 
On 27 March 2018 at 09:40, Sergii Romantsov  wrote:
> Hello Chris and Emil.
> Could we make a final decision? Because I'm bit confused what you are
> expecting exactly.
>
> I would prefer to left initial patch as is (if its possible).
>
Same inclination here. Changing the storage and/or argument type would
be bigger patch with greater chances of causing regressions.

The patch looks reasonable IMHO.
Reviewed-by: Emil Velikov 

Can you please double-check which commit caused the regression and
resent with RB + Fixes tags.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-03 Thread Chris Wilson 
Quoting Sergii Romantsov (2018-04-02 07:59:06)
> Gen8+ use 48-bit address relocations so need to extend the sign
> to 64-bit return value. Without it we have higher bits zeroed
> and missing the negavive values.
> Haswell and older use 32-bit deltas so are unaffected by this issue.
> 
> v2:
>   used int32_t fucntion parameter instead of explicit type conversion.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> Signed-off-by: Sergii Romantsov 
> Tested-by: Andriy Khulap 

Lgtm,

Reviewed-by: Chris Wilson 
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-02 Thread Stuart Young 
Hi Sergii,

I built this into mesa 17.3.7 and it seems to solve all the issues I've
seen that were mentioned in 101408.

This includes not just the issues with the Mortar, but the other visual
artefacts that I've noticed with the player models and with other weapons
in the game.

Many thanks for digging into this.


On 26 March 2018 at 23:22, Sergii Romantsov <
sergii.romant...@globallogic.com> wrote:

> Hello, Stuart.
> Could You, please, test this patch?
>
> On Mon, Mar 26, 2018 at 3:16 PM, Sergii Romantsov <
> sergii.romant...@gmail.com> wrote:
>
>> Negative deltas are used to fake a range in a large buffer.
>> See 900a5c91eeb3
>> "i965: Use negative relocation deltas to minimise vertex uploads"
>>
>> Gen8+ use 48-bit address relocations so need to extend the sign
>> to 64-bit return value. Without it we have higher bits zeroed
>> and missing the negavive values.
>> Haswell and older use 32-bit deltas so are unaffected by this issue.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
>> Signed-off-by: Sergii Romantsov 
>> Tested-by: Andriy Khulap 
>> ---
>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> index d824ff2..128da77 100644
>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
>> /* Using the old buffer offset, write in what the right data would
>> be, in
>>  * case the buffer doesn't move and we can short-circuit the
>> relocation
>>  * processing in the kernel
>> +*
>> +* Some target_offsets may be negative, so extend the sign to 64 bits.
>>  */
>> -   return entry->offset + target_offset;
>> +   return entry->offset + (int64_t)((int32_t)target_offset);
>>  }
>>
>>  uint64_t
>> --
>> 2.7.4
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
>
> --
> Sergii Romantsov
> GlobalLogic Inc.
> www.globallogic.com
>



-- 
Stuart Young (aka Cefiar)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-04-02 Thread Sergii Romantsov 
Gen8+ use 48-bit address relocations so need to extend the sign
to 64-bit return value. Without it we have higher bits zeroed
and missing the negavive values.
Haswell and older use 32-bit deltas so are unaffected by this issue.

v2:
  used int32_t fucntion parameter instead of explicit type conversion.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
Signed-off-by: Sergii Romantsov 
Tested-by: Andriy Khulap 
---
 src/mesa/drivers/dri/i965/intel_batchbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index ebc02ff..7286140 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -1079,7 +1079,7 @@ brw_batch_references(struct intel_batchbuffer *batch, 
struct brw_bo *bo)
 static uint64_t
 emit_reloc(struct intel_batchbuffer *batch,
struct brw_reloc_list *rlist, uint32_t offset,
-   struct brw_bo *target, uint32_t target_offset,
+   struct brw_bo *target, int32_t target_offset,
unsigned int reloc_flags)
 {
assert(target != NULL);
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-27 Thread Sergii Romantsov 
Hello Chris and Emil.
Could we make a final decision? Because I'm bit confused what you are
expecting exactly.

I would prefer to left initial patch as is (if its possible).

On Tue, Mar 27, 2018 at 9:20 AM, Sergii Romantsov <
sergii.romant...@gmail.com> wrote:

> Hello Stuart,
> Nice to hear.
> And thanks for your time and contribution.
>
> On Tue, Mar 27, 2018 at 8:40 AM, Stuart Young  wrote:
>
>> Hi Sergii,
>>
>> I built this into mesa 17.3.7 and it seems to solve all the issues I've
>> seen that were mentioned in 101408.
>>
>> This includes not just the issues with the Mortar, but the other visual
>> artefacts that I've noticed with the player models and with other weapons
>> in the game.
>>
>> Many thanks for digging into this.
>>
>>
>> On 26 March 2018 at 23:22, Sergii Romantsov <
>> sergii.romant...@globallogic.com> wrote:
>>
>>> Hello, Stuart.
>>> Could You, please, test this patch?
>>>
>>> On Mon, Mar 26, 2018 at 3:16 PM, Sergii Romantsov <
>>> sergii.romant...@gmail.com> wrote:
>>>
 Negative deltas are used to fake a range in a large buffer.
 See 900a5c91eeb3
 "i965: Use negative relocation deltas to minimise vertex uploads"

 Gen8+ use 48-bit address relocations so need to extend the sign
 to 64-bit return value. Without it we have higher bits zeroed
 and missing the negavive values.
 Haswell and older use 32-bit deltas so are unaffected by this issue.

 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
 Signed-off-by: Sergii Romantsov 
 Tested-by: Andriy Khulap 
 ---
  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
 b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
 index d824ff2..128da77 100644
 --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
 +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
 @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
 /* Using the old buffer offset, write in what the right data would
 be, in
  * case the buffer doesn't move and we can short-circuit the
 relocation
  * processing in the kernel
 +*
 +* Some target_offsets may be negative, so extend the sign to 64
 bits.
  */
 -   return entry->offset + target_offset;
 +   return entry->offset + (int64_t)((int32_t)target_offset);
  }

  uint64_t
 --
 2.7.4

 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 https://lists.freedesktop.org/mailman/listinfo/mesa-dev

>>>
>>>
>>>
>>> --
>>> Sergii Romantsov
>>> GlobalLogic Inc.
>>> www.globallogic.com
>>>
>>
>>
>>
>> --
>> Stuart Young (aka Cefiar)
>>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-27 Thread Sergii Romantsov 
Hello Stuart,
Nice to hear.
And thanks for your time and contribution.

On Tue, Mar 27, 2018 at 8:40 AM, Stuart Young  wrote:

> Hi Sergii,
>
> I built this into mesa 17.3.7 and it seems to solve all the issues I've
> seen that were mentioned in 101408.
>
> This includes not just the issues with the Mortar, but the other visual
> artefacts that I've noticed with the player models and with other weapons
> in the game.
>
> Many thanks for digging into this.
>
>
> On 26 March 2018 at 23:22, Sergii Romantsov  com> wrote:
>
>> Hello, Stuart.
>> Could You, please, test this patch?
>>
>> On Mon, Mar 26, 2018 at 3:16 PM, Sergii Romantsov <
>> sergii.romant...@gmail.com> wrote:
>>
>>> Negative deltas are used to fake a range in a large buffer.
>>> See 900a5c91eeb3
>>> "i965: Use negative relocation deltas to minimise vertex uploads"
>>>
>>> Gen8+ use 48-bit address relocations so need to extend the sign
>>> to 64-bit return value. Without it we have higher bits zeroed
>>> and missing the negavive values.
>>> Haswell and older use 32-bit deltas so are unaffected by this issue.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
>>> Signed-off-by: Sergii Romantsov 
>>> Tested-by: Andriy Khulap 
>>> ---
>>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>>> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>>> index d824ff2..128da77 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>>> @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
>>> /* Using the old buffer offset, write in what the right data would
>>> be, in
>>>  * case the buffer doesn't move and we can short-circuit the
>>> relocation
>>>  * processing in the kernel
>>> +*
>>> +* Some target_offsets may be negative, so extend the sign to 64
>>> bits.
>>>  */
>>> -   return entry->offset + target_offset;
>>> +   return entry->offset + (int64_t)((int32_t)target_offset);
>>>  }
>>>
>>>  uint64_t
>>> --
>>> 2.7.4
>>>
>>> ___
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>
>>
>>
>>
>> --
>> Sergii Romantsov
>> GlobalLogic Inc.
>> www.globallogic.com
>>
>
>
>
> --
> Stuart Young (aka Cefiar)
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Sergii Romantsov 
Hello, Emil. Thanks for involving.
> Fixes: cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")

Actually that patch doesn't fix exactly cee9f3890351. Also I wrote earlier
(probably, my e-mails may go into spam :)):
"That issue is present even on mesa 13-0.0 and also solved with similar
type-conversion fix."

On Mon, Mar 26, 2018 at 4:57 PM, Emil Velikov 
wrote:

> On 26 March 2018 at 13:31, Chris Wilson  wrote:
> > Quoting Sergii Romantsov (2018-03-26 13:16:24)
> >> Negative deltas are used to fake a range in a large buffer.
> >> See 900a5c91eeb3
> >> "i965: Use negative relocation deltas to minimise vertex uploads"
> >>
> >> Gen8+ use 48-bit address relocations so need to extend the sign
> >
> > Note that 48-bit relocations were only switched on in
> > commit cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")
> > to save having to backport too far (although the patch is trivial).
> >
> Agreed, one could backport it for older instances, although it seems
> unneeded.
> Please use a simple fixes tag like:
>
> Fixes: cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")
>
> >> to 64-bit return value. Without it we have higher bits zeroed
> >> and missing the negavive values.
> >> Haswell and older use 32-bit deltas so are unaffected by this issue.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> >> Signed-off-by: Sergii Romantsov 
> >> Tested-by: Andriy Khulap 
> >> ---
> >>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> index d824ff2..128da77 100644
> >> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
> >> /* Using the old buffer offset, write in what the right data would
> be, in
> >>  * case the buffer doesn't move and we can short-circuit the
> relocation
> >>  * processing in the kernel
> >> +*
> >> +* Some target_offsets may be negative, so extend the sign to 64
> bits.
> >>  */
> >> -   return entry->offset + target_offset;
> >> +   return entry->offset + (int64_t)((int32_t)target_offset);
> >
> > Although just changing s/uint32_t target_offset/int32_t target_offset/
> > may be cleaner.
>
> Seems like there's plenty of users that opt for unsigned. Might be
> better to address the issue as-is and as a follow-up do a consistent
> to-signed sweep.
> It keeps the fix small and simple, plus avoids a ton of extra warnings
> [for those using -W*sign*] ;-)
>
> -Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Emil Velikov 
On 26 March 2018 at 13:31, Chris Wilson  wrote:
> Quoting Sergii Romantsov (2018-03-26 13:16:24)
>> Negative deltas are used to fake a range in a large buffer.
>> See 900a5c91eeb3
>> "i965: Use negative relocation deltas to minimise vertex uploads"
>>
>> Gen8+ use 48-bit address relocations so need to extend the sign
>
> Note that 48-bit relocations were only switched on in
> commit cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")
> to save having to backport too far (although the patch is trivial).
>
Agreed, one could backport it for older instances, although it seems unneeded.
Please use a simple fixes tag like:

Fixes: cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")

>> to 64-bit return value. Without it we have higher bits zeroed
>> and missing the negavive values.
>> Haswell and older use 32-bit deltas so are unaffected by this issue.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
>> Signed-off-by: Sergii Romantsov 
>> Tested-by: Andriy Khulap 
>> ---
>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
>> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> index d824ff2..128da77 100644
>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
>> /* Using the old buffer offset, write in what the right data would be, in
>>  * case the buffer doesn't move and we can short-circuit the relocation
>>  * processing in the kernel
>> +*
>> +* Some target_offsets may be negative, so extend the sign to 64 bits.
>>  */
>> -   return entry->offset + target_offset;
>> +   return entry->offset + (int64_t)((int32_t)target_offset);
>
> Although just changing s/uint32_t target_offset/int32_t target_offset/
> may be cleaner.

Seems like there's plenty of users that opt for unsigned. Might be
better to address the issue as-is and as a follow-up do a consistent
to-signed sweep.
It keeps the fix small and simple, plus avoids a ton of extra warnings
[for those using -W*sign*] ;-)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Chris Wilson 
Quoting Sergii Romantsov (2018-03-26 14:11:26)
> Hello, Chris. Thank you for reviewing patch.
> Do you mean changing just only emit_reloc() parameter type or all the 
> preceding
> callers too?

If you insist, but you don't have to. int32_t on the parameter is enough
to document and correct the type. Propagating it through to all callers
can be left for another day.
 
> Also, please, explain what you mean about patch cee9f3890351?
> That issue is present even on mesa 13-0.0 and also solved with similar
> type-conversion fix.

Hmm. I was thinking that if we only assigned 32b addresses the wrap
would be ok; but no, it's only ok if the kernel does the relocation.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Sergii Romantsov 
Hello, Chris. Thank you for reviewing patch.
Do you mean changing just only emit_reloc() parameter type or all the
preceding callers too?

Also, please, explain what you mean about patch cee9f3890351?
That issue is present even on mesa 13-0.0 and also solved with similar
type-conversion fix.


Regards, Sergii.

On Mon, Mar 26, 2018 at 3:31 PM, Chris Wilson 
wrote:

> Quoting Sergii Romantsov (2018-03-26 13:16:24)
> > Negative deltas are used to fake a range in a large buffer.
> > See 900a5c91eeb3
> > "i965: Use negative relocation deltas to minimise vertex uploads"
> >
> > Gen8+ use 48-bit address relocations so need to extend the sign
>
> Note that 48-bit relocations were only switched on in
> commit cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")
> to save having to backport too far (although the patch is trivial).
>
> > to 64-bit return value. Without it we have higher bits zeroed
> > and missing the negavive values.
> > Haswell and older use 32-bit deltas so are unaffected by this issue.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> > Signed-off-by: Sergii Romantsov 
> > Tested-by: Andriy Khulap 
> > ---
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index d824ff2..128da77 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
> > /* Using the old buffer offset, write in what the right data would
> be, in
> >  * case the buffer doesn't move and we can short-circuit the
> relocation
> >  * processing in the kernel
> > +*
> > +* Some target_offsets may be negative, so extend the sign to 64
> bits.
> >  */
> > -   return entry->offset + target_offset;
> > +   return entry->offset + (int64_t)((int32_t)target_offset);
>
> Although just changing s/uint32_t target_offset/int32_t target_offset/
> may be cleaner.
> -Chris
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>



-- 
Sergii Romantsov
GlobalLogic Inc.
www.globallogic.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Chris Wilson 
Quoting Sergii Romantsov (2018-03-26 13:16:24)
> Negative deltas are used to fake a range in a large buffer.
> See 900a5c91eeb3
> "i965: Use negative relocation deltas to minimise vertex uploads"
> 
> Gen8+ use 48-bit address relocations so need to extend the sign

Note that 48-bit relocations were only switched on in
commit cee9f3890351 ("i965: Allow 48-bit addressing on Gen8+.")
to save having to backport too far (although the patch is trivial).

> to 64-bit return value. Without it we have higher bits zeroed
> and missing the negavive values.
> Haswell and older use 32-bit deltas so are unaffected by this issue.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> Signed-off-by: Sergii Romantsov 
> Tested-by: Andriy Khulap 
> ---
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index d824ff2..128da77 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
> /* Using the old buffer offset, write in what the right data would be, in
>  * case the buffer doesn't move and we can short-circuit the relocation
>  * processing in the kernel
> +*
> +* Some target_offsets may be negative, so extend the sign to 64 bits.
>  */
> -   return entry->offset + target_offset;
> +   return entry->offset + (int64_t)((int32_t)target_offset);

Although just changing s/uint32_t target_offset/int32_t target_offset/
may be cleaner.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Sergii Romantsov 
Hello, Stuart.
Could You, please, test this patch?

On Mon, Mar 26, 2018 at 3:16 PM, Sergii Romantsov <
sergii.romant...@gmail.com> wrote:

> Negative deltas are used to fake a range in a large buffer.
> See 900a5c91eeb3
> "i965: Use negative relocation deltas to minimise vertex uploads"
>
> Gen8+ use 48-bit address relocations so need to extend the sign
> to 64-bit return value. Without it we have higher bits zeroed
> and missing the negavive values.
> Haswell and older use 32-bit deltas so are unaffected by this issue.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
> Signed-off-by: Sergii Romantsov 
> Tested-by: Andriy Khulap 
> ---
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index d824ff2..128da77 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
> /* Using the old buffer offset, write in what the right data would be,
> in
>  * case the buffer doesn't move and we can short-circuit the relocation
>  * processing in the kernel
> +*
> +* Some target_offsets may be negative, so extend the sign to 64 bits.
>  */
> -   return entry->offset + target_offset;
> +   return entry->offset + (int64_t)((int32_t)target_offset);
>  }
>
>  uint64_t
> --
> 2.7.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>



-- 
Sergii Romantsov
GlobalLogic Inc.
www.globallogic.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: Extend the negative 32-bit deltas to 64-bits

2018-03-26 Thread Sergii Romantsov 
Negative deltas are used to fake a range in a large buffer.
See 900a5c91eeb3
"i965: Use negative relocation deltas to minimise vertex uploads"

Gen8+ use 48-bit address relocations so need to extend the sign
to 64-bit return value. Without it we have higher bits zeroed
and missing the negavive values.
Haswell and older use 32-bit deltas so are unaffected by this issue.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101408
Signed-off-by: Sergii Romantsov 
Tested-by: Andriy Khulap 
---
 src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index d824ff2..128da77 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -1124,8 +1124,10 @@ emit_reloc(struct intel_batchbuffer *batch,
/* Using the old buffer offset, write in what the right data would be, in
 * case the buffer doesn't move and we can short-circuit the relocation
 * processing in the kernel
+*
+* Some target_offsets may be negative, so extend the sign to 64 bits.
 */
-   return entry->offset + target_offset;
+   return entry->offset + (int64_t)((int32_t)target_offset);
 }
 
 uint64_t
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev