[Pixman] [PATCH] Don't discriminate PAD and REFLECT repeat in standard fast paths

[Soeren Sandmann]

Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:

> From: Siarhei Siamashka <siarhei.siamashka at nokia.com>
> 
> Without this fix, setting PAD repeat on a source image prevents
> the use of any nonscaled standard fast paths, affecting performance
> a lot. But as long as no pixels outside the source image boundaries
> are touched by the compositing operation, all the repeat types
> behave the same and can take the same fast paths.
> 
> This is important because setting PAD repeat instead of NONE is
> more hardware acceleration friendly (for the drivers implementing
> RENDER extension) and does not inhibit OVER->SRC operator
> optimization in pixman.
> ---
> 
> We discussed this problem and the related code with Soeren on IRC the
> other day, and it may be that the code dealing with FAST_PATH_COVERS_CLIP
> and FAST_PATH_SAMPLES_COVER_CLIP would better to be changed a bit. Along
> with the selection of fast path functions dealing with SOLID sources or
> masks (for example, any transformation set on a SOLID source image does
> not actually make any difference for end result, so it can be ignored).
> And there is a special optimization for NORMAL repeat in walk_region_internal()
> function which is also somehow related. Well, I guess this all stuff
> carried us away from the original problem :)
> 
> But now returning back to this issue. Ideally, PAD repeat would be a better
> default instead of NONE. Just because NONE repeat is a bit strange and
> hard to accelerate in hardware. This is one example from freedesktop.org
> bugzilla (and there are more similar reports):
> https://bugs.freedesktop.org/show_bug.cgi?id=27954
> 
> But right now setting PAD repeat for everying will cause serious performance
> problems for pixman, because it will stop using simple non-transformed fast
> paths.
> 
> This patch tries to address the problem. It passes current pixman tests and
> I think that it is most likely fine. But I'm still not totally sure if it is
> really safe in all possible cases.

I can't think of a case where it would be broken, but I don't think
it's really *right* either. The COVERS_CLIP flag is supposed to
indicate that the image doesn't have any undefined parts, which is
certainly the case when you have PAD or REFLECT set.

The fundamental problem is that the PIXMAN_STD_FAST_PATH macro is
mixing up several things. There are two kinds of images involved in
the standard fast paths: solids and bits. The problem is that the
required flags for those images are quite different.

A solid image has basically no requirements on the flags other than
the usual NO_ALPHAMAP, NO_CONVOLUTION etc. Generally, as long as it's
a solid color, the fast path will work.

The bits images have basically the requirement that we now call
SAMPLES_COVER_CLIP. That allows the biltters to simply walk the image
without worrying about reading out of bounds.

Then there is the 'simple repeat' stuff, which is an optimization that
notices that in some cases you can do a repeat operation by repeating
the compositing instead of sampling with wrap-around addressing. This
code has survived since the dawn of time, when pixman was called fb,
didn't have transformations, and NORMAL was the only repeat mode
available. Back then this code was the way to do repeating.

On IRC you pointed out that for 1xn repeated images, this simple
repeat stuff is not an optimization at all because it results in
terrible memory access patterns. It has historically had other
pathological cases, such as one it would end up calling a fast path a
zillion times with a 1x1 rectangle.

In fact, it was never written as an optimization, it was only left in
because it was assumed to be one. I don't think anyone has ever
measured the impact compared to the general path.

Considering that it is really at the wrong level in the pipeline too
(repeating conceptually happens during sampling, not after
compositing), maybe it's time to retire this code. There are cases
where it is probably still useful, but I'd rather avoid the headache
of thinking about special cases where it can't or shouldn't be used.

So, basically, what I think could be done is:

- Change the PIXMAN_STD_FAST_PATH to put in different flags when an
  image is solid vs. when it is bits. The flags in the solid case
  would be just the usual ones, in the bits case, it would be the
  usual ones plus SAMPLES_COVER_CLIP.

- Delete the COVERS_CLIP flag, since it would not be used in either
  case and isn't used elsewhere.

- Delete the simple_repeat stuff (along with the SIMPLE_REPEAT flags)

If there are specific benchmarks where the simple_repeat stuff made a
noticable difference, then it's probably better to just do a regular
fast path for them. A scanline approach would likely be faster than a
tiled approach anyway in those cases.



Soren