Closed Bug 1260351 Opened 3 years ago Closed 3 years ago

Image: Enable ConvolveVertically/Horizontally in LS3 MMI

Categories

(Core :: ImageLib, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(2 files, 4 obsolete files)

Use skia::ConvolveHorizontally and skia::ConvolveVertically with SIMD on Loongson.
Attachment #8735707 - Flags: review?(mh+mozilla)
Attachment #8735710 - Flags: review?(tnikkel) → review+
Comment on attachment 8735707 [details] [diff] [review]
0001-Bug-1260351-MozGlue-Build-Add-mips.h.-r-glandium.patch

Review of attachment 8735707 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/mips.h
@@ +6,5 @@
> +
> +#ifndef mozilla_mips_h_
> +#define mozilla_mips_h_
> +
> +// for definition of MFBT_DATA

You're not using MFBT_DATA :)

@@ +12,5 @@
> +
> +namespace mozilla {
> +
> +#ifdef _MIPS_ARCH_LOONGSON3A
> +  inline bool supports_mmi() { return true; }

Can this be detected at runtime?
Attachment #8735707 - Flags: review?(mh+mozilla)
Attachment #8735707 - Attachment is obsolete: true
Attachment #8736272 - Flags: review?(mh+mozilla)
Comment on attachment 8736272 [details] [diff] [review]
0001-Bug-1260351-MozGlue-Build-Add-mips-support.-r-glandi.patch

Review of attachment 8736272 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/mips.cpp
@@ +25,5 @@
> +    char buf[1024];
> +    memset(buf, 0, sizeof(buf));
> +    fread(buf, sizeof(char), sizeof(buf) - 1, fin);
> +    fclose(fin);
> +    if (strstr(buf, "Loongson"))

Do all loongson have the the extension you want to use? The previous patch seemed to imply not.
Attachment #8736272 - Flags: review?(mh+mozilla)
Flags: needinfo?(r)
(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 8736272 [details] [diff] [review]
> 0001-Bug-1260351-MozGlue-Build-Add-mips-support.-r-glandi.patch
> 
> Review of attachment 8736272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozglue/build/mips.cpp
> @@ +25,5 @@
> > +    char buf[1024];
> > +    memset(buf, 0, sizeof(buf));
> > +    fread(buf, sizeof(char), sizeof(buf) - 1, fin);
> > +    fclose(fin);
> > +    if (strstr(buf, "Loongson"))
> 
> Do all loongson have the the extension you want to use? The previous patch
> seemed to imply not.

Oh, No. Only works on Loongson-3 series CPUs. Thanks. ;)
Flags: needinfo?(r)
Attachment #8736272 - Attachment is obsolete: true
Attachment #8738052 - Flags: review?(mh+mozilla)
Comment on attachment 8738052 [details] [diff] [review]
0001-Bug-1260351-MozGlue-Build-Add-mips-support.-r-glandi.patch

Review of attachment 8738052 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/build/mips.cpp
@@ +19,5 @@
> +{
> +  unsigned  flags = 0;
> +  FILE     *fin;
> +
> +  fin = fopen ("/proc/cpuinfo","r");

no whitespace between function name and parenthesis.

@@ +25,5 @@
> +    char buf[1024];
> +    memset(buf, 0, sizeof(buf));
> +    fread(buf, sizeof(char), sizeof(buf) - 1, fin);
> +    fclose(fin);
> +    if (strstr(buf, "Loongson-3"))

isn't any of the items in the "isa" field indicative of what you're looking for? That would be better than a processor name check, which will not work with future processors with a different name, but with the right instruction set.

@@ +32,5 @@
> +  return flags;
> +}
> +
> +// Cache a local copy so we only have to read /proc/cpuinfo once.
> +static unsigned mips_cpu_flags = get_mips_cpu_flags();

You could move this in the check function below.
Attachment #8738052 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 8738052 [details] [diff] [review]
> 0001-Bug-1260351-MozGlue-Build-Add-mips-support.-r-glandi.patch
> 
> Review of attachment 8738052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozglue/build/mips.cpp
> @@ +19,5 @@
> > +{
> > +  unsigned  flags = 0;
> > +  FILE     *fin;
> > +
> > +  fin = fopen ("/proc/cpuinfo","r");
> 
> no whitespace between function name and parenthesis.
Nice!

> 
> @@ +25,5 @@
> > +    char buf[1024];
> > +    memset(buf, 0, sizeof(buf));
> > +    fread(buf, sizeof(char), sizeof(buf) - 1, fin);
> > +    fclose(fin);
> > +    if (strstr(buf, "Loongson-3"))
> 
> isn't any of the items in the "isa" field indicative of what you're looking
> for? That would be better than a processor name check, which will not work
> with future processors with a different name, but with the right instruction
> set.
No. The Linux kernel doesn't export the MMI features in 'isa' and 'ASEs implemented' fields. As far as know, no one to do it. Fortunately, All Loongson-3 series CPUs are supported 'MMI' ASE.

CPU info (Loongson-3):
processor		: 0
cpu model		: ICT Loongson-3 V0.5  FPU V0.1
model name		: ICT Loongson-3A R1 (Loongson-3A1000) @ 899MHz
BogoMIPS		: 717.28
wait instruction	: no
microsecond timers	: yes
tlb_entries		: 64
extra interrupt vector	: no
hardware watchpoint	: yes, count: 0, address/irw mask: []
isa			: mips1 mips2 mips3 mips4 mips5 mips32r1 mips64r1
ASEs implemented	:
shadow register sets	: 1
kscratch registers	: 0
package			: 0
core			: 0
VCED exceptions		: not available
VCEI exceptions		: not available

> 
> @@ +32,5 @@
> > +  return flags;
> > +}
> > +
> > +// Cache a local copy so we only have to read /proc/cpuinfo once.
> > +static unsigned mips_cpu_flags = get_mips_cpu_flags();
> 
> You could move this in the check function below.
Did you mean move mips_cpu_flags in the check_loongson3 function below? A compiler error caused by use mips_cpu_flags before define it...
Attachment #8738839 - Flags: review?(mh+mozilla)
Attachment #8738052 - Attachment is obsolete: true
> Did you mean move mips_cpu_flags in the check_loongson3 function below? A compiler error caused by use mips_cpu_flags before define it...

How can the following fail to build with the error you say?

static bool
check_loongson3(void)
{
  static unsigned mips_cpu_flags = get_mips_cpu_flags();
  return (mips_cpu_flags & MIPS_FLAG_LOONGSON3) != 0;
}
(In reply to Mike Hommey [:glandium] from comment #12)
> > Did you mean move mips_cpu_flags in the check_loongson3 function below? A compiler error caused by use mips_cpu_flags before define it...
> 
> How can the following fail to build with the error you say?
> 
> static bool
> check_loongson3(void)
> {
>   static unsigned mips_cpu_flags = get_mips_cpu_flags();
>   return (mips_cpu_flags & MIPS_FLAG_LOONGSON3) != 0;
> }

sorry, I understand error :(
Attachment #8738839 - Attachment is obsolete: true
Attachment #8738839 - Flags: review?(mh+mozilla)
Attachment #8740237 - Flags: review?(mh+mozilla)
Attachment #8740237 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/6e11636c6aa2
https://hg.mozilla.org/mozilla-central/rev/041577d97132
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.