diff mbox

[FFmpeg-devel,v2] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

Message ID 1563950774-29396-1-git-send-email-yinshiyou-hf@loongson.cn
State Superseded
Headers show

Commit Message

Shiyou Yin July 24, 2019, 6:46 a.m. UTC
Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
---
 libavcodec/mips/simple_idct_mmi.c | 2 +-
 libavutil/mips/mmiutils.h         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Reimar Döffinger July 25, 2019, 11:27 p.m. UTC | #1
Is there a mips maintainer? otherwise:

On 24.07.2019, at 08:46, Shiyou Yin <yinshiyou-hf@loongson.cn> wrote:

> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
> ---
> libavcodec/mips/simple_idct_mmi.c | 2 +-
> libavutil/mips/mmiutils.h         | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mips/simple_idct_mmi.c b/libavcodec/mips/simple_idct_mmi.c
> index 7f4bb74..73d797f 100644
> --- a/libavcodec/mips/simple_idct_mmi.c
> +++ b/libavcodec/mips/simple_idct_mmi.c
> @@ -39,7 +39,7 @@
> #define COL_SHIFT 20
> #define DC_SHIFT 3
> 
> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
>     W4,  W2,  W4,  W6,
>     W1,  W3,  W5,  W7,
>     W4,  W6, -W4, -W2,

This should be fine, simply as it should not be possible for it to cause issues.

> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
> index 05f6b31..bfa6d8b 100644
> --- a/libavutil/mips/mmiutils.h
> +++ b/libavutil/mips/mmiutils.h
> @@ -205,7 +205,7 @@
>  * backup register
>  */
> #define BACKUP_REG \
> -  double temp_backup_reg[8];                                    \
> +  double __attribute__ ((aligned (16))) temp_backup_reg[8];     \


I don't think we're supposed to use raw __attribute__ in FFmpeg, and for stack variables there can be even more issues.
Maybe check with what other code does...
Shiyou Yin July 26, 2019, 5:14 a.m. UTC | #2
>-----Original Message-----
>From: ffmpeg-devel-bounces@ffmpeg.org [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of
>Reimar D?ffinger
>Sent: Friday, July 26, 2019 7:28 AM
>To: FFmpeg development discussions and patches
>Subject: Re: [FFmpeg-devel] [PATCH v2] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
>
>Is there a mips maintainer? otherwise:

Since Manojkumar Bhosale stoped take care of mips section last year, 
I applied to be the mips maintainer, and trying to be qualified. Will keep learning and be rigorous.

>On 24.07.2019, at 08:46, Shiyou Yin <yinshiyou-hf@loongson.cn> wrote:
>
>> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
>> ---
>> libavcodec/mips/simple_idct_mmi.c | 2 +-
>> libavutil/mips/mmiutils.h         | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/mips/simple_idct_mmi.c b/libavcodec/mips/simple_idct_mmi.c
>> index 7f4bb74..73d797f 100644
>> --- a/libavcodec/mips/simple_idct_mmi.c
>> +++ b/libavcodec/mips/simple_idct_mmi.c
>> @@ -39,7 +39,7 @@
>> #define COL_SHIFT 20
>> #define DC_SHIFT 3
>>
>> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
>> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
>>     W4,  W2,  W4,  W6,
>>     W1,  W3,  W5,  W7,
>>     W4,  W6, -W4, -W2,
>
>This should be fine, simply as it should not be possible for it to cause issues.
>
>> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
>> index 05f6b31..bfa6d8b 100644
>> --- a/libavutil/mips/mmiutils.h
>> +++ b/libavutil/mips/mmiutils.h
>> @@ -205,7 +205,7 @@
>>  * backup register
>>  */
>> #define BACKUP_REG \
>> -  double temp_backup_reg[8];                                    \
>> +  double __attribute__ ((aligned (16))) temp_backup_reg[8];     \
>
>
>I don't think we're supposed to use raw __attribute__ in FFmpeg, and for stack variables there can be
>even more issues.
>Maybe check with what other code does...

Will replaced with DECLARE_ALIGNED.

>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

diff --git a/libavcodec/mips/simple_idct_mmi.c b/libavcodec/mips/simple_idct_mmi.c
index 7f4bb74..73d797f 100644
--- a/libavcodec/mips/simple_idct_mmi.c
+++ b/libavcodec/mips/simple_idct_mmi.c
@@ -39,7 +39,7 @@ 
 #define COL_SHIFT 20
 #define DC_SHIFT 3
 
-DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
+DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
     W4,  W2,  W4,  W6,
     W1,  W3,  W5,  W7,
     W4,  W6, -W4, -W2,
diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
index 05f6b31..bfa6d8b 100644
--- a/libavutil/mips/mmiutils.h
+++ b/libavutil/mips/mmiutils.h
@@ -205,7 +205,7 @@ 
  * backup register
  */
 #define BACKUP_REG \
-  double temp_backup_reg[8];                                    \
+  double __attribute__ ((aligned (16))) temp_backup_reg[8];     \
   if (_MIPS_SIM == _ABI64)                                      \
     __asm__ volatile (                                          \
       "gssqc1       $f25,      $f24,       0x00(%[temp])  \n\t" \