Re: [Pixman] [PATCH v2] pixman-general: Tighten up calculation of temporary buffer sizes
On Thu, 24 Sep 2015 23:12:42 +0200 soren.sandm...@gmail.com (Søren Sandmann) wrote: > Pekka Paalanen writes: > > >> As a discussion point, wouldn't it be better for the ALIGN macro to > >> assume 32-byte cache lines? Both 16-byte and 32-byte cachelines are > >> currently in common use, but by aligning the buffers to 32-byte addresses > >> we would simultaneously achieve 16-byte alignment. > > It's not the size of cache lines that is interesting; it's the width of > SIMD instructions. > > > I think Ben's explanation as seen in > > https://patchwork.freedesktop.org/patch/49898/ > > covers all Søren's concerns (it quotes everything Søren said about the > > patch), and I see no reason to reject this. > > Well, the concern that SIMD destination fetchers (for example a 565 > destination iterator) can no longer do a full SIMD write to the buffer > is still there. It's not just combiners that write to the destination > buffer. > > But whatever, it's not really worth arguing about. Let's just push > this. We can fix it if SIMD destination fetchers ever actually happen. Cool. Pushed: 8b49d4b..23525b4 master -> master I wasn't sure if you wanted me to interpret that as an acked-by from you, so I didn't. Thanks, pq pgp2JYqzKPyww.pgp Description: OpenPGP digital signature ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v2] pixman-general: Tighten up calculation of temporary buffer sizes
Pekka Paalanen writes: >> As a discussion point, wouldn't it be better for the ALIGN macro to >> assume 32-byte cache lines? Both 16-byte and 32-byte cachelines are >> currently in common use, but by aligning the buffers to 32-byte addresses >> we would simultaneously achieve 16-byte alignment. It's not the size of cache lines that is interesting; it's the width of SIMD instructions. > I think Ben's explanation as seen in > https://patchwork.freedesktop.org/patch/49898/ > covers all Søren's concerns (it quotes everything Søren said about the > patch), and I see no reason to reject this. Well, the concern that SIMD destination fetchers (for example a 565 destination iterator) can no longer do a full SIMD write to the buffer is still there. It's not just combiners that write to the destination buffer. But whatever, it's not really worth arguing about. Let's just push this. We can fix it if SIMD destination fetchers ever actually happen. Søren ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v2] pixman-general: Tighten up calculation of temporary buffer sizes
On Tue, 22 Sep 2015 12:43:25 +0100 Ben Avison wrote: > Each of the aligns can only add a maximum of 15 bytes to the space > requirement. This permits some edge cases to use the stack buffer where > previously it would have deduced that a heap buffer was required. > --- > > This is an update of my previous patch (now posted over a year ago): > https://patchwork.freedesktop.org/patch/49898/ > > which conflicts with the patch just pushed: > http://patchwork.freedesktop.org/patch/60052/ > > Since this piece of code is fresh in people's minds, this might be a good > time to get this one pushed as well. > > Note that the scope of this change has been reduced: while the old code > added space to align the end address to a cacheline boundary (which as I > pointed out, was unnecessary), the new version works using buffer lengths > only. > > As a discussion point, wouldn't it be better for the ALIGN macro to > assume 32-byte cache lines? Both 16-byte and 32-byte cachelines are > currently in common use, but by aligning the buffers to 32-byte addresses > we would simultaneously achieve 16-byte alignment. > > pixman/pixman-general.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c > index fa88463..6141cb0 100644 > --- a/pixman/pixman-general.c > +++ b/pixman/pixman-general.c > @@ -158,9 +158,9 @@ general_composite_rect (pixman_implementation_t *imp, > if (width <= 0 || _pixman_multiply_overflows_int (width, Bpp * 3)) > return; > > -if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3) > +if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 15 * 3) > { > - scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32 * 3); > + scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 15 * 3); > > if (!scanline_buffer) > return; Reviewed-by: Pekka Paalanen I think Ben's explanation as seen in https://patchwork.freedesktop.org/patch/49898/ covers all Søren's concerns (it quotes everything Søren said about the patch), and I see no reason to reject this. I'll push this on Friday if no-one objects. Thanks, pq pgpL74xwNlDmG.pgp Description: OpenPGP digital signature ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH v2] pixman-general: Tighten up calculation of temporary buffer sizes
Each of the aligns can only add a maximum of 15 bytes to the space requirement. This permits some edge cases to use the stack buffer where previously it would have deduced that a heap buffer was required. --- This is an update of my previous patch (now posted over a year ago): https://patchwork.freedesktop.org/patch/49898/ which conflicts with the patch just pushed: http://patchwork.freedesktop.org/patch/60052/ Since this piece of code is fresh in people's minds, this might be a good time to get this one pushed as well. Note that the scope of this change has been reduced: while the old code added space to align the end address to a cacheline boundary (which as I pointed out, was unnecessary), the new version works using buffer lengths only. As a discussion point, wouldn't it be better for the ALIGN macro to assume 32-byte cache lines? Both 16-byte and 32-byte cachelines are currently in common use, but by aligning the buffers to 32-byte addresses we would simultaneously achieve 16-byte alignment. pixman/pixman-general.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c index fa88463..6141cb0 100644 --- a/pixman/pixman-general.c +++ b/pixman/pixman-general.c @@ -158,9 +158,9 @@ general_composite_rect (pixman_implementation_t *imp, if (width <= 0 || _pixman_multiply_overflows_int (width, Bpp * 3)) return; -if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3) +if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 15 * 3) { - scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32 * 3); + scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 15 * 3); if (!scanline_buffer) return; -- 1.7.5.4 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman