[Pixman] Basic infrastructure for MIPS32r2 and DSPr1 optimizations.

[Søren Sandmann]

Hi,

Some highlevel comments on this patch set:

* Most of the patches need more detail in the commit log. Generally,
  there should be around the level information that someone who is
  familiar with pixman could write the patch just based on the commit
  log.

  It's fine to say just "Add src_x888_8888 to lowlevel-blt-bench" or
  "Update author's email address" and leave it at that, but for example

       MIPS: dspr2: runtime detection extended

  or

       MIPS: dspr2: Removed build restrictions and repair compiler's check

  is not enough detail. The log should say why the existing code is not
  good enough, and what the new code is doing to fix it. And it's not
  enough to just say *what*, you also need to say *why*. So,

       MIPS: disabled non 32-bit platforms
    
       This patch add mechanism which allows optimizations to be run
       only on 32-bit platforms.

  needs an explanation of why optimizations should only be run on 32-bit
  platforms.


* In the final code after applying all the patches, the files
  pixman-mips32r32.c, pixman-mips-dspr2.c, pixman-mips-dspr1.c
  contain a word-for-word identical copy of the same blt function.

  This is really totally unnecessary. If you look at
  pixman-implementation.c, you'll see that there is already a mechanism
  in place to fall back from higher level implementations to lower
  levels.

  Moreover, the blt function that has been duplicated isn't even MIPS
  specific -- the functionality in question (implementing blt in terms
  of an optimized memcpy() function) makes just as much sense for other
  architectures.

  So I think this should be done by

    - adding a memcpy() field to pixman_implementation_t

    - adding something like the duplicated blt() function to
      pixman-general.c (except having it call
      implementation->toplevel->memcpy() instead of a global variable).

    - having the various MIPS implementations set the implementation of
      memcpy() to whatever they like.

  This has a number of advantages:

    - Other architectures can implement their own optimized memcpy() if
      they wish

    - There is no ad hoc global MIPS specific virtual memcpy function

    - There is no blt function duplicated across all MIPS
      implementations

* Something similar applies to the fill() routines: The fill in
  pixman-mips-dspr2.c doesn't use any DSPr2 specific functionality, so
  it should not exist. Instead just rely on the fallback mechanism that
  is already present in pixman-implementation.c.

  Similarly, the DSPr1 functionality should look like this:

     if (bpp == 16)
     {
          fill_buff16_dspr1

          return TRUE;
     }

     return FALSE;

  Then if the system in question supports MIPS2r2 and bpp is not 16, it
  will automatically fall back to the fill in pixman-mips32r2.c

  I can't say I love the global pixman_fill_buff* function pointers, but
  if they are made static and moved into the file that actually uses
  them (after making the above changed, each one will only be used from
  one file), I probably won't complain too much.

I will also write some specific comments to some of the patches.


Søren


Nemanja Lukic <nemanja.lukic at rt-rk.com> writes:

> Some of the optimizations introduced in previous DSPr2 commits were not DSPr2 specific. Some of the fast-paths didn't used DSPr2 instructions at all, and rather utilized more generic MIPS32r2 instruction set or previous version of DSP instruction set (DSPr1) for optimizations.
> Since Pixman's run-time CPU detection only added DSPr2 fast-paths on 74K MIPS cores, these optimizations couldn't be used on cores that don't support DSPr2, but do support MIPS32r2 or DSPr1 instructions (these are almost all newer MIPS CPU cores like 4K, 24K, 34K, 1004K, etc).
> These patches extract those MIPS32r2 and DSPr1 specific optimizations into new fast-path sets, with appropriate build and run time infrastructure. With these pathes no new optimizations are introduced, only refactoring of previous DSPr2 optimizations into MIPS32r2-specific and DSPr1-specific.
> Future commits will add more MIPS32r2 and DSPr1 specific fast paths.
> For testing, lowlevel-blt benchmark is used and two different MIPS cores:
>  - 24Kc - for MIPS32r2
>  - 1004Kc - for DSPr1
>
> This patch set should address all previous code review remarks.
>
> Any comments are available.
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman