Re: [Pixman] [PATCH v2] pixman-general: Tighten up calculation of temporary buffer sizes

[Pekka Paalanen]

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

[Søren Sandmann]

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

[Pekka Paalanen]

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

[Ben Avison]

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