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

[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 <emil.l.velikov at gmail.com>
wrote:

> On 26 March 2018 at 13:31, Chris Wilson <chris at chris-wilson.co.uk> 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 <sergii.romantsov at globallogic.com>
> >> Tested-by: Andriy Khulap <andriy.khulap at globallogic.com>
> >> ---
> >>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180326/8b9a6da0/attachment-0001.html>