[Pixman] [PATCH 3/3] MIPS: DSPr1: Basic infrastructure for DSPr1 optimizations

[Siarhei Siamashka]

On Thu, 25 Jul 2013 17:46:16 +0200 Nemanja Lukic wrote:

> > I welcome extending support to more MIPS variants, and it's surely
> > a good thing to have. Just the goal needs to be a bit more clear.
> > 
> > The point of runtime features detection is that the Linux distro
> > maintainers can build a single library binary, which can run on a lot
> > of devices and use CPU specific optimizations where they are available.
> > 
> > Do you know what kind of popular linux distributions are available
> > for MIPS right now and whether the pixman packages shipped in these
> > distributions are able to use DSPr2 optimizations properly?
> > 
> 
> Popular linux distributions available for MIPS can be found at:
> http://www.linux-mips.org/wiki/Distributions
> 
> I'm not sure if packages are built with MIPS32r1 or not in these
> distibutions, but our current state on Pixman repo has this check in
> configure for MIPS DSPr2:
> #if !(defined(__mips__) &&  __mips_isa_rev >= 2)
> #error MIPS DSPr2 is currently only available on MIPS32r2 platforms.
> #endif
> So if packages are not built with -mips32r2 in CFLAGS (or toolchain is not
> configured to automatically add this flag), they will not be able to use
> DSPr2 optimizations properly.
> 
> > We can use ARM as an example. The vast majority of ARM devices support
> > NEON SIMD, however there a few exceptions (Tegra2 and Marvell SoCs).
> > Because of these exceptions, Linux distributions don't enable NEON in
> > CFLAGS when building their packages and just target the lowest common
> > denominator. However pixman is still able to use NEON optimizations
> > even in such conservative Linux distributions as Debian thanks to
> > the runtime CPU features detection.
> > 
> > Now let's look at MIPS again. Hypothetically, some Linux distribution
> > may decide to build packages and select MIPS32r1 as the lowest common
> > denominator. And if some users install this distribution on their
> > MIPS32r2+DSPr2 devices, will they still benefit from the DSPr2
> > optimizations in pixman?
> > 
> 
> As said above, with current state of the code, no they will not. I would
> suggest, than, that we first remove configure check mentioned above, since
> it is useless

Agreed, the current requirement for at least MIPS32r2 CPU architecture
is too strict and can be surely relaxed.

Still as a reminder, the checks in configure script exist to test
whether the compiler can successfully compile the MIPS assembly code
and get something sane out of it. Just because we don't want the
compiler to try and fail the build on x86. Also not everyone uses gcc.
There may be other compilers, possibly with incompatible assembler
syntax. The configure script has to properly handle all of this.

> and utilize runtime CPU feature detection, similar to what is
> already done for NEON optimizations (forcing needed CFLAGS in configure.ac
> in order to include optimizations at build time, and then choose at runtime
> whether to use them).

Forcing CFLAGS is a bit fragile and is not a very good idea in general.
Just look at the iwmmxt mess (which is kinda justified and a bit hard
to avoid because of the use of intrinsics).

But MIPS assembly sources already include the following directives:

.set    arch=mips32r2;
.set    dspr2;

And can be successfully compiled with any CFLAGS. So the problem
does not exist in the first place. From "info as":

"   The directive `.set dsp' makes the assembler accept instructions
from the DSP Release 1 Application Specific Extension from that point
on in the assembly.  The `.set nodsp' directive prevents DSP Release 1
instructions from being accepted.

   The directive `.set dspr2' makes the assembler accept instructions
from the DSP Release 2 Application Specific Extension from that point
on in the assembly.  This dirctive implies `.set dsp'.  The `.set
nodspr2' directive prevents DSP Release 2 instructions from being
accepted."

> Also, since the initial work for MIPS DSPr2, a couple of years ago, things
> have changed, in terms of what can be detected at runtime on MIPS CPU. I'm
> not sure about kernel version, I think it is 3.7, but from the august last
> year, /proc/cpuinfo in section "ASEs implemented" can distinguish DSPr1 and
> DSPr2 ISA extensions.

That's a good news. The runtime check can be extended to also parse
this "ASEs implemented" section in addition to checking for the
MIPS cores which are known to support the DSP instructions. The
current check for the exact MIPS core types is still useful for the
compatibility with older kernel versions.

Does /proc/cpuinfo also report the cache line size now? Just as we
know, PrepareForStore prefetch is destructive and affects the whole
cache line. Running the code which assumes 32 byte cache line size on
the system with 64 byte cache lines may cause data corruption.

> > The examples of MIPS32r1 are the Broadcom SoCs that you mentioned and
> > also http://en.wikipedia.org/wiki/Ingenic_Semiconductor (which, I
> > believe, was the SoC used in real Android tablets).
> > 
> > Now I admit that I did not really search too hard for the relevant
> > MIPS related information about all this stuff. Actually, you are
> > probably the one who is the most interested in doing this mini
> > research to come up with a nice story about how your patches
> > are going to improve the situation in the real world :-)
> 
> So, to summarize, I would suggest than, that this patch set, also include
> one more commit that removes check mentioned above, which will than enable
> same mechanism as we have in ARM NEON. Regardless of the way packages on the
> linux distro will be compiled, we will include DSPr2 (and MIPS32r2 and
> DPSr1) optimizations and rely on the runtime detection to use them or not.
> I'll prepare new patch set, and upload it for review.

Thanks, this sounds like a good plan.

Also in order to have a clean patch set, it makes sense to first
mechanically split the pixman-mips-dspr2-asm.S file into separate
"mips32", "dsp" and "dspr2" files. You are currently using the
".set dspr2" directive per each function. But in order to let the
compiler help you do this split right and sort the function correctly,
pixman-mips-dspr2-asm.S should have ".set dspr2" directive and
pixman-mips-dsp-asm.S should have ".set dsp". This way even if you
accidentally add a dspr2 function to pixman-mips-dsp-asm.S file,
the compiler will reject it. And as another note, looks like the
memcpy function only needs MIPS32r1.

I checked the current pixman mipsel binary compiled for debian:
http://ftp.fi.debian.org/debian/pool/main/p/pixman/libpixman-1-0_0.30.0-1_mipsel.deb

It is "ELF 32-bit LSB shared object, MIPS, MIPS-II version 1 (SYSV)"
and "Tag_GNU_MIPS_ABI_FP: Hard float (double precision)"

http://en.wikipedia.org/wiki/MIPS_architecture#MIPS32 says "Introduced
in 1999 based on MIPS II with some additional features from MIPS III,
MIPS IV, and MIPS V". And "MIPS® Architecture For Programmers Volume
I-A: Introduction to the MIPS32® Architecture" document confirms this.

So we are dealing with 32-bit MIPS-II and hardware floating point ABI
as the lowest common denominator for debian. That's not the worst
possible choice and the runtime CPU features detection should work
quite nicely there.

-- 
Best regards,
Siarhei Siamashka