[Pixman] [PATCH 1/1] Disable implementations mentioned in the PIXMAN_DISABLE environment variable.

[Andrea Canciani]

On Sun, Feb 26, 2012 at 4:12 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Sat, Feb 25, 2012 at 9:39 PM, Søren Sandmann <sandmann at cs.au.dk> wrote:
>> From: Søren Sandmann Pedersen <ssp at redhat.com>
>>
>> With this, it becomes possible to do
>>
>>     PIXMAN_DISABLE="sse2 mmx" some_app
>>
>> which will run some_app without SSE2 and MMX enabled. This is useful
>> for benchmarking, testing and narrowing down bugs.
>> ---
>>  pixman/pixman-cpu.c |   44 ++++++++++++++++++++++++++++++++------------
>>  1 files changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/pixman/pixman-cpu.c b/pixman/pixman-cpu.c
>> index fcf591a..ba89df5 100644
>> --- a/pixman/pixman-cpu.c
>> +++ b/pixman/pixman-cpu.c
>> @@ -24,6 +24,7 @@
>>  #endif
>>
>>  #include <string.h>
>> +#include <stdlib.h>
>>
>>  #if defined(USE_ARM_SIMD) && defined(_MSC_VER)
>>  /* Needed for EXCEPTION_ILLEGAL_INSTRUCTION */
>> @@ -328,7 +329,6 @@ pixman_arm_read_auxv_or_cpu_features ()
>>
>>  #elif defined (__linux__) /* linux ELF */
>>
>> -#include <stdlib.h>
>>  #include <unistd.h>
>>  #include <sys/types.h>
>>  #include <sys/stat.h>
>> @@ -711,51 +711,71 @@ pixman_have_sse2 (void)
>>  #endif /* __amd64__ */
>>  #endif
>>
>> +static pixman_bool_t
>> +disabled (const char *name)
>> +{
>> +    const char *e;
>> +
>> +    if ((e = getenv ("PIXMAN_DISABLE")))
>> +    {
>> +       if (strstr (e, name))

I think that strstr might cause substrings to also disable unintended backends.
This would already happen for "mmx" vs "arm-iwmmxt".
Is this intentional?
Although in this specific case, it might be ok, I believe that it
might lead to unexpected behavior as other backends are added.

>> +       {
>> +           printf ("pixman: disabled %s implementation\n", name);
>> +           return TRUE;
>> +       }
>> +    }
>> +
>> +    return FALSE;
>> +}
>> +
>>  pixman_implementation_t *
>>  _pixman_choose_implementation (void)
>>  {
>>     pixman_implementation_t *imp;
>>
>>     imp = _pixman_implementation_create_general();
>> -    imp = _pixman_implementation_create_fast_path (imp);
>> -
>> +
>> +    if (!disabled ("fast"))
>> +       imp = _pixman_implementation_create_fast_path (imp);
>> +
>>  #ifdef USE_X86_MMX
>> -    if (pixman_have_mmx ())
>> +    if (pixman_have_mmx () && !disabled ("mmx"))
>>        imp = _pixman_implementation_create_mmx (imp);
>>  #endif
>>
>>  #ifdef USE_SSE2
>> -    if (pixman_have_sse2 ())
>> +    if (pixman_have_sse2 () && !disabled ("sse2"))
>>        imp = _pixman_implementation_create_sse2 (imp);
>>  #endif
>>
>>  #ifdef USE_ARM_SIMD
>> -    if (pixman_have_arm_simd ())
>> +    if (pixman_have_arm_simd () && !disabled ("arm-simd"))
>>        imp = _pixman_implementation_create_arm_simd (imp);
>>  #endif
>>
>>  #ifdef USE_ARM_IWMMXT
>> -    if (pixman_have_arm_iwmmxt ())
>> +    if (pixman_have_arm_iwmmxt () && !disabled ("arm-iwmmxt"))
>>        imp = _pixman_implementation_create_mmx (imp);
>>  #endif
>>
>>  #ifdef USE_ARM_NEON
>> -    if (pixman_have_arm_neon ())
>> +    if (pixman_have_arm_neon () && !disabled ("arm-neon"))
>>        imp = _pixman_implementation_create_arm_neon (imp);
>>  #endif
>>
>>  #ifdef USE_MIPS_DSPR2
>> -    if (pixman_have_mips_dspr2 ())
>> +    if (pixman_have_mips_dspr2 () && !disabled ("mips-dspr2"))
>>        imp = _pixman_implementation_create_mips_dspr2 (imp);
>>  #endif
>>
>>  #ifdef USE_VMX
>> -    if (pixman_have_vmx ())
>> +    if (pixman_have_vmx () && !disabled ("vmx"))
>>        imp = _pixman_implementation_create_vmx (imp);
>>  #endif
>>
>> -    imp = _pixman_implementation_create_noop (imp);
>> -
>> +    if (!disabled ("noop"))
>> +       imp = _pixman_implementation_create_noop (imp);
>> +
>>     return imp;
>>  }
>>
>> --
>> 1.6.0.6
>
> Indeed, this is very useful.
>
> I often seen if (!disabled(envvar) && supported()) where the disabling
> check is first in the if-statement. That's the only suggestion I have.
> Otherwise, this looks like a good change.

I agree that if the backend is disabled, we should avoid checking the support.
This way, we can use disable to ensure that the backend cannot break
the application status in any way (example: modify simd registers
without restoring them appropriately).

Andrea

>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman