[Pixman] Basic infrastructure for MIPS32r2 and DSPr1 optimizations, and patch for mips* asm exports symbols.

Søren Sandmann soren.sandmann at gmail.com
Sat Aug 23 11:44:31 PDT 2014


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

> This patch set should address all previous code review remarks.

>From looking at this patch set fairly briefly,

- There is some confusion about the type of the new memcpy
  function. Does it return bool or void*? And what precisely does the
  return value mean? It seems to be used both as "this call succeeded"
  (boolean) and "copy of the destination pointer" (void *).

  In my opinion, the memcpy routines should take a
  pixman_implementation_t * as the first argument, and have no return
  value. This is similar to the other routines exported by
  implementations.

- I'm not sure the exported pixman_memcpy() and the
  s/memcpy/pixman_memcpy/g sed job make sense. The new memcpy()
  infrastructure should primarily be considered a way to support the
  implementations that wish to provide a faster memcpy() for the various
  graphics operations, and the various ancillary uses of memcpy() don't
  really need to super optimized.

Also, as mentioned in the last review:

- The blt() function in pixman-mips32r2.c is not MIPS specific. It can
  live in pixman-general and rely on the normal fallback mechanism. And
  instead of calling the new pixman_memcpy() it could just call
  implementation->toplevel->memcpy(), which is the usual way to call a
  lower-level function in pixman.

- The fill_buff pointers should be static and defined inside the C files
  that use them. It's not generally a good idea to define variables in a
  header file.


Søren


More information about the Pixman mailing list