| Message ID | 20210728100624.16129-1-dafna.hirschfeld@collabora.com |
|---|---|
| Headers | show |
| Series | media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch | expand |
Hi Dafna, On 28/07/2021 12:06, Dafna Hirschfeld wrote: > Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64 > the function 'v4l2_compat_translate_cmd' doesn't have a 'translation' > for the cmd and so 'cmd' is returned as is. This cause > a failure '-ENOTTY' in __video_do_ioctl. I don't really see how this would happen. As far as I can tell (and admittedly, the code is hard to follow) on such a system there is no need for any 32-bit to 64-bit conversion in v4l2-compat-ioctl32.c since struct v4l2_event_time32 is the same for both architectures. And in v4l2-ioctl.c VIDIOC_DQEVENT_TIME32 is handled correctly as far as I can tell. Where exactly does the return -ENOTTY happen? It definitely does not make sense to modify v4l2-compat-ioctl32.c if there is nothing to convert between 32 and 64 bit. If there is a bug, then that should be in v4l2-ioctl.c. Note that it is perfectly possible that there is a bug for arm64 and this ioctl, since that's the one combination I've never tested since I don't have a test setup for that. Regards, Hans > This patch fixes it by defining VIDIOC_DQEVENT32_TIME32 to be > VIDIOC_DQEVENT_TIME32 for non x86-64 arch and translates it > to VIDIOC_DQEVENT. > In addition, add a call to put_v4l2_event_time32 to copy the data > to userspace for non x86-64 arch > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 21 ++++++++++++------- > include/media/v4l2-ioctl.h | 3 +++ > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 5623003a9705..20a80880d9b7 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -871,9 +871,13 @@ static int put_v4l2_edid32(struct v4l2_edid *p64, > #define VIDIOC_QUERYBUF32_TIME32 _IOWR('V', 9, struct v4l2_buffer32_time32) > #define VIDIOC_QBUF32_TIME32 _IOWR('V', 15, struct v4l2_buffer32_time32) > #define VIDIOC_DQBUF32_TIME32 _IOWR('V', 17, struct v4l2_buffer32_time32) > + > #ifdef CONFIG_X86_64 > #define VIDIOC_DQEVENT32_TIME32 _IOR ('V', 89, struct v4l2_event32_time32) > +#else > +#define VIDIOC_DQEVENT32_TIME32 VIDIOC_DQEVENT_TIME32 > #endif > + > #define VIDIOC_PREPARE_BUF32_TIME32 _IOWR('V', 93, struct v4l2_buffer32_time32) > #endif > > @@ -899,6 +903,8 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) > return VIDIOC_DQBUF; > case VIDIOC_PREPARE_BUF32_TIME32: > return VIDIOC_PREPARE_BUF; > + case VIDIOC_DQEVENT32_TIME32: > + return VIDIOC_DQEVENT; > #endif > case VIDIOC_QUERYBUF32: > return VIDIOC_QUERYBUF; > @@ -927,10 +933,6 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) > #ifdef CONFIG_X86_64 > case VIDIOC_DQEVENT32: > return VIDIOC_DQEVENT; > -#ifdef CONFIG_COMPAT_32BIT_TIME > - case VIDIOC_DQEVENT32_TIME32: > - return VIDIOC_DQEVENT; > -#endif > #endif > } > return cmd; > @@ -996,6 +998,13 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) > case VIDIOC_DQBUF32_TIME32: > case VIDIOC_PREPARE_BUF32_TIME32: > return put_v4l2_buffer32_time32(parg, arg); > + case VIDIOC_DQEVENT32_TIME32: > +#ifdef CONFIG_X86_64 > + return put_v4l2_event32_time32(parg, arg); > +#else > + return put_v4l2_event_time32(parg, arg); > +#endif > + > #endif > case VIDIOC_QUERYBUF32: > case VIDIOC_QBUF32: > @@ -1023,10 +1032,6 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) > #ifdef CONFIG_X86_64 > case VIDIOC_DQEVENT32: > return put_v4l2_event32(parg, arg); > -#ifdef CONFIG_COMPAT_32BIT_TIME > - case VIDIOC_DQEVENT32_TIME32: > - return put_v4l2_event32_time32(parg, arg); > -#endif > #endif > } > return 0; > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > index 0c209bbbc76f..3840a3ea384c 100644 > --- a/include/media/v4l2-ioctl.h > +++ b/include/media/v4l2-ioctl.h > @@ -786,7 +786,10 @@ struct v4l2_buffer_time32 { > }; > > /* > + * For architectures other than x86_64, the struct v4l2_event32_time32 > + * is the same as v4l2_event_time32. > * This function is used to copy the struct v4l2_event_time32 to userspace > + * if either the kernel is not 64-bit or if the architecture is other than x86_64. > */ > int put_v4l2_event_time32(void *parg, void __user *arg); > >
On 02/09/2021 10:24, Hans Verkuil wrote: > Hi Dafna, > > On 28/07/2021 12:06, Dafna Hirschfeld wrote: >> Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64 >> the function 'v4l2_compat_translate_cmd' doesn't have a 'translation' >> for the cmd and so 'cmd' is returned as is. This cause >> a failure '-ENOTTY' in __video_do_ioctl. > > I don't really see how this would happen. As far as I can tell (and admittedly, > the code is hard to follow) on such a system there is no need for any 32-bit to > 64-bit conversion in v4l2-compat-ioctl32.c since struct v4l2_event_time32 is the > same for both architectures. > > And in v4l2-ioctl.c VIDIOC_DQEVENT_TIME32 is handled correctly as far as I can > tell. Where exactly does the return -ENOTTY happen? > > It definitely does not make sense to modify v4l2-compat-ioctl32.c if there is > nothing to convert between 32 and 64 bit. If there is a bug, then that should be > in v4l2-ioctl.c. > > Note that it is perfectly possible that there is a bug for arm64 and this ioctl, > since that's the one combination I've never tested since I don't have a test setup > for that. How did you test this anyway? With which libc/compiler? It would be nice if I could test this myself on e.g. a Raspberry Pi. Regards, Hans > > Regards, > > Hans > >> This patch fixes it by defining VIDIOC_DQEVENT32_TIME32 to be >> VIDIOC_DQEVENT_TIME32 for non x86-64 arch and translates it >> to VIDIOC_DQEVENT. >> In addition, add a call to put_v4l2_event_time32 to copy the data >> to userspace for non x86-64 arch >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 21 ++++++++++++------- >> include/media/v4l2-ioctl.h | 3 +++ >> 2 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> index 5623003a9705..20a80880d9b7 100644 >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> @@ -871,9 +871,13 @@ static int put_v4l2_edid32(struct v4l2_edid *p64, >> #define VIDIOC_QUERYBUF32_TIME32 _IOWR('V', 9, struct v4l2_buffer32_time32) >> #define VIDIOC_QBUF32_TIME32 _IOWR('V', 15, struct v4l2_buffer32_time32) >> #define VIDIOC_DQBUF32_TIME32 _IOWR('V', 17, struct v4l2_buffer32_time32) >> + >> #ifdef CONFIG_X86_64 >> #define VIDIOC_DQEVENT32_TIME32 _IOR ('V', 89, struct v4l2_event32_time32) >> +#else >> +#define VIDIOC_DQEVENT32_TIME32 VIDIOC_DQEVENT_TIME32 >> #endif >> + >> #define VIDIOC_PREPARE_BUF32_TIME32 _IOWR('V', 93, struct v4l2_buffer32_time32) >> #endif >> >> @@ -899,6 +903,8 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) >> return VIDIOC_DQBUF; >> case VIDIOC_PREPARE_BUF32_TIME32: >> return VIDIOC_PREPARE_BUF; >> + case VIDIOC_DQEVENT32_TIME32: >> + return VIDIOC_DQEVENT; >> #endif >> case VIDIOC_QUERYBUF32: >> return VIDIOC_QUERYBUF; >> @@ -927,10 +933,6 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) >> #ifdef CONFIG_X86_64 >> case VIDIOC_DQEVENT32: >> return VIDIOC_DQEVENT; >> -#ifdef CONFIG_COMPAT_32BIT_TIME >> - case VIDIOC_DQEVENT32_TIME32: >> - return VIDIOC_DQEVENT; >> -#endif >> #endif >> } >> return cmd; >> @@ -996,6 +998,13 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) >> case VIDIOC_DQBUF32_TIME32: >> case VIDIOC_PREPARE_BUF32_TIME32: >> return put_v4l2_buffer32_time32(parg, arg); >> + case VIDIOC_DQEVENT32_TIME32: >> +#ifdef CONFIG_X86_64 >> + return put_v4l2_event32_time32(parg, arg); >> +#else >> + return put_v4l2_event_time32(parg, arg); >> +#endif >> + >> #endif >> case VIDIOC_QUERYBUF32: >> case VIDIOC_QBUF32: >> @@ -1023,10 +1032,6 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) >> #ifdef CONFIG_X86_64 >> case VIDIOC_DQEVENT32: >> return put_v4l2_event32(parg, arg); >> -#ifdef CONFIG_COMPAT_32BIT_TIME >> - case VIDIOC_DQEVENT32_TIME32: >> - return put_v4l2_event32_time32(parg, arg); >> -#endif >> #endif >> } >> return 0; >> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h >> index 0c209bbbc76f..3840a3ea384c 100644 >> --- a/include/media/v4l2-ioctl.h >> +++ b/include/media/v4l2-ioctl.h >> @@ -786,7 +786,10 @@ struct v4l2_buffer_time32 { >> }; >> >> /* >> + * For architectures other than x86_64, the struct v4l2_event32_time32 >> + * is the same as v4l2_event_time32. >> * This function is used to copy the struct v4l2_event_time32 to userspace >> + * if either the kernel is not 64-bit or if the architecture is other than x86_64. >> */ >> int put_v4l2_event_time32(void *parg, void __user *arg); >> >> >
(CC'ing Arnd) On Thu, Sep 02, 2021 at 10:39:46AM +0200, Hans Verkuil wrote: > On 02/09/2021 10:24, Hans Verkuil wrote: > > On 28/07/2021 12:06, Dafna Hirschfeld wrote: > >> Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64 > >> the function 'v4l2_compat_translate_cmd' doesn't have a 'translation' > >> for the cmd and so 'cmd' is returned as is. This cause > >> a failure '-ENOTTY' in __video_do_ioctl. > > > > I don't really see how this would happen. As far as I can tell (and admittedly, > > the code is hard to follow) on such a system there is no need for any 32-bit to > > 64-bit conversion in v4l2-compat-ioctl32.c since struct v4l2_event_time32 is the > > same for both architectures. > > > > And in v4l2-ioctl.c VIDIOC_DQEVENT_TIME32 is handled correctly as far as I can > > tell. Where exactly does the return -ENOTTY happen? > > > > It definitely does not make sense to modify v4l2-compat-ioctl32.c if there is > > nothing to convert between 32 and 64 bit. If there is a bug, then that should be > > in v4l2-ioctl.c. > > > > Note that it is perfectly possible that there is a bug for arm64 and this ioctl, > > since that's the one combination I've never tested since I don't have a test setup > > for that. > > How did you test this anyway? With which libc/compiler? It would be nice if I could > test this myself on e.g. a Raspberry Pi. > > Regards, > > Hans > > > > > Regards, > > > > Hans > > > >> This patch fixes it by defining VIDIOC_DQEVENT32_TIME32 to be > >> VIDIOC_DQEVENT_TIME32 for non x86-64 arch and translates it > >> to VIDIOC_DQEVENT. > >> In addition, add a call to put_v4l2_event_time32 to copy the data > >> to userspace for non x86-64 arch > >> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >> --- > >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 21 ++++++++++++------- > >> include/media/v4l2-ioctl.h | 3 +++ > >> 2 files changed, 16 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > >> index 5623003a9705..20a80880d9b7 100644 > >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > >> @@ -871,9 +871,13 @@ static int put_v4l2_edid32(struct v4l2_edid *p64, > >> #define VIDIOC_QUERYBUF32_TIME32 _IOWR('V', 9, struct v4l2_buffer32_time32) > >> #define VIDIOC_QBUF32_TIME32 _IOWR('V', 15, struct v4l2_buffer32_time32) > >> #define VIDIOC_DQBUF32_TIME32 _IOWR('V', 17, struct v4l2_buffer32_time32) > >> + > >> #ifdef CONFIG_X86_64 > >> #define VIDIOC_DQEVENT32_TIME32 _IOR ('V', 89, struct v4l2_event32_time32) > >> +#else > >> +#define VIDIOC_DQEVENT32_TIME32 VIDIOC_DQEVENT_TIME32 > >> #endif > >> + > >> #define VIDIOC_PREPARE_BUF32_TIME32 _IOWR('V', 93, struct v4l2_buffer32_time32) > >> #endif > >> > >> @@ -899,6 +903,8 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) > >> return VIDIOC_DQBUF; > >> case VIDIOC_PREPARE_BUF32_TIME32: > >> return VIDIOC_PREPARE_BUF; > >> + case VIDIOC_DQEVENT32_TIME32: > >> + return VIDIOC_DQEVENT; > >> #endif > >> case VIDIOC_QUERYBUF32: > >> return VIDIOC_QUERYBUF; > >> @@ -927,10 +933,6 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) > >> #ifdef CONFIG_X86_64 > >> case VIDIOC_DQEVENT32: > >> return VIDIOC_DQEVENT; > >> -#ifdef CONFIG_COMPAT_32BIT_TIME > >> - case VIDIOC_DQEVENT32_TIME32: > >> - return VIDIOC_DQEVENT; > >> -#endif > >> #endif > >> } > >> return cmd; > >> @@ -996,6 +998,13 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) > >> case VIDIOC_DQBUF32_TIME32: > >> case VIDIOC_PREPARE_BUF32_TIME32: > >> return put_v4l2_buffer32_time32(parg, arg); > >> + case VIDIOC_DQEVENT32_TIME32: > >> +#ifdef CONFIG_X86_64 > >> + return put_v4l2_event32_time32(parg, arg); > >> +#else > >> + return put_v4l2_event_time32(parg, arg); > >> +#endif > >> + > >> #endif > >> case VIDIOC_QUERYBUF32: > >> case VIDIOC_QBUF32: > >> @@ -1023,10 +1032,6 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) > >> #ifdef CONFIG_X86_64 > >> case VIDIOC_DQEVENT32: > >> return put_v4l2_event32(parg, arg); > >> -#ifdef CONFIG_COMPAT_32BIT_TIME > >> - case VIDIOC_DQEVENT32_TIME32: > >> - return put_v4l2_event32_time32(parg, arg); > >> -#endif > >> #endif > >> } > >> return 0; > >> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > >> index 0c209bbbc76f..3840a3ea384c 100644 > >> --- a/include/media/v4l2-ioctl.h > >> +++ b/include/media/v4l2-ioctl.h > >> @@ -786,7 +786,10 @@ struct v4l2_buffer_time32 { > >> }; > >> > >> /* > >> + * For architectures other than x86_64, the struct v4l2_event32_time32 > >> + * is the same as v4l2_event_time32. > >> * This function is used to copy the struct v4l2_event_time32 to userspace > >> + * if either the kernel is not 64-bit or if the architecture is other than x86_64. > >> */ > >> int put_v4l2_event_time32(void *parg, void __user *arg); > >> -- Regards, Laurent Pinchart
On 02/09/2021 10:49, Laurent Pinchart wrote: > (CC'ing Arnd) > > On Thu, Sep 02, 2021 at 10:39:46AM +0200, Hans Verkuil wrote: >> On 02/09/2021 10:24, Hans Verkuil wrote: >>> On 28/07/2021 12:06, Dafna Hirschfeld wrote: >>>> Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64 >>>> the function 'v4l2_compat_translate_cmd' doesn't have a 'translation' >>>> for the cmd and so 'cmd' is returned as is. This cause >>>> a failure '-ENOTTY' in __video_do_ioctl. >>> >>> I don't really see how this would happen. As far as I can tell (and admittedly, >>> the code is hard to follow) on such a system there is no need for any 32-bit to >>> 64-bit conversion in v4l2-compat-ioctl32.c since struct v4l2_event_time32 is the >>> same for both architectures. >>> >>> And in v4l2-ioctl.c VIDIOC_DQEVENT_TIME32 is handled correctly as far as I can >>> tell. Where exactly does the return -ENOTTY happen? >>> >>> It definitely does not make sense to modify v4l2-compat-ioctl32.c if there is >>> nothing to convert between 32 and 64 bit. If there is a bug, then that should be >>> in v4l2-ioctl.c. >>> >>> Note that it is perfectly possible that there is a bug for arm64 and this ioctl, >>> since that's the one combination I've never tested since I don't have a test setup >>> for that. >> >> How did you test this anyway? With which libc/compiler? It would be nice if I could >> test this myself on e.g. a Raspberry Pi. I added a test in v4l2-compliance and it appears that something is indeed wrong for arm64. I'll dig deeper into this. I don't get ENOTTY though, instead it returns 0 but the contents of the returned v4l2_event is garbled. It works fine for x86_64, but not for arm64. Regards, Hans >> >> Regards, >> >> Hans >> >>> >>> Regards, >>> >>> Hans >>> >>>> This patch fixes it by defining VIDIOC_DQEVENT32_TIME32 to be >>>> VIDIOC_DQEVENT_TIME32 for non x86-64 arch and translates it >>>> to VIDIOC_DQEVENT. >>>> In addition, add a call to put_v4l2_event_time32 to copy the data >>>> to userspace for non x86-64 arch >>>> >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 21 ++++++++++++------- >>>> include/media/v4l2-ioctl.h | 3 +++ >>>> 2 files changed, 16 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >>>> index 5623003a9705..20a80880d9b7 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >>>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >>>> @@ -871,9 +871,13 @@ static int put_v4l2_edid32(struct v4l2_edid *p64, >>>> #define VIDIOC_QUERYBUF32_TIME32 _IOWR('V', 9, struct v4l2_buffer32_time32) >>>> #define VIDIOC_QBUF32_TIME32 _IOWR('V', 15, struct v4l2_buffer32_time32) >>>> #define VIDIOC_DQBUF32_TIME32 _IOWR('V', 17, struct v4l2_buffer32_time32) >>>> + >>>> #ifdef CONFIG_X86_64 >>>> #define VIDIOC_DQEVENT32_TIME32 _IOR ('V', 89, struct v4l2_event32_time32) >>>> +#else >>>> +#define VIDIOC_DQEVENT32_TIME32 VIDIOC_DQEVENT_TIME32 >>>> #endif >>>> + >>>> #define VIDIOC_PREPARE_BUF32_TIME32 _IOWR('V', 93, struct v4l2_buffer32_time32) >>>> #endif >>>> >>>> @@ -899,6 +903,8 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) >>>> return VIDIOC_DQBUF; >>>> case VIDIOC_PREPARE_BUF32_TIME32: >>>> return VIDIOC_PREPARE_BUF; >>>> + case VIDIOC_DQEVENT32_TIME32: >>>> + return VIDIOC_DQEVENT; >>>> #endif >>>> case VIDIOC_QUERYBUF32: >>>> return VIDIOC_QUERYBUF; >>>> @@ -927,10 +933,6 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) >>>> #ifdef CONFIG_X86_64 >>>> case VIDIOC_DQEVENT32: >>>> return VIDIOC_DQEVENT; >>>> -#ifdef CONFIG_COMPAT_32BIT_TIME >>>> - case VIDIOC_DQEVENT32_TIME32: >>>> - return VIDIOC_DQEVENT; >>>> -#endif >>>> #endif >>>> } >>>> return cmd; >>>> @@ -996,6 +998,13 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) >>>> case VIDIOC_DQBUF32_TIME32: >>>> case VIDIOC_PREPARE_BUF32_TIME32: >>>> return put_v4l2_buffer32_time32(parg, arg); >>>> + case VIDIOC_DQEVENT32_TIME32: >>>> +#ifdef CONFIG_X86_64 >>>> + return put_v4l2_event32_time32(parg, arg); >>>> +#else >>>> + return put_v4l2_event_time32(parg, arg); >>>> +#endif >>>> + >>>> #endif >>>> case VIDIOC_QUERYBUF32: >>>> case VIDIOC_QBUF32: >>>> @@ -1023,10 +1032,6 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) >>>> #ifdef CONFIG_X86_64 >>>> case VIDIOC_DQEVENT32: >>>> return put_v4l2_event32(parg, arg); >>>> -#ifdef CONFIG_COMPAT_32BIT_TIME >>>> - case VIDIOC_DQEVENT32_TIME32: >>>> - return put_v4l2_event32_time32(parg, arg); >>>> -#endif >>>> #endif >>>> } >>>> return 0; >>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h >>>> index 0c209bbbc76f..3840a3ea384c 100644 >>>> --- a/include/media/v4l2-ioctl.h >>>> +++ b/include/media/v4l2-ioctl.h >>>> @@ -786,7 +786,10 @@ struct v4l2_buffer_time32 { >>>> }; >>>> >>>> /* >>>> + * For architectures other than x86_64, the struct v4l2_event32_time32 >>>> + * is the same as v4l2_event_time32. >>>> * This function is used to copy the struct v4l2_event_time32 to userspace >>>> + * if either the kernel is not 64-bit or if the architecture is other than x86_64. >>>> */ >>>> int put_v4l2_event_time32(void *parg, void __user *arg); >>>> >
On 02/09/2021 13:08, Hans Verkuil wrote: > On 02/09/2021 10:49, Laurent Pinchart wrote: >> (CC'ing Arnd) >> >> On Thu, Sep 02, 2021 at 10:39:46AM +0200, Hans Verkuil wrote: >>> On 02/09/2021 10:24, Hans Verkuil wrote: >>>> On 28/07/2021 12:06, Dafna Hirschfeld wrote: >>>>> Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64 >>>>> the function 'v4l2_compat_translate_cmd' doesn't have a 'translation' >>>>> for the cmd and so 'cmd' is returned as is. This cause >>>>> a failure '-ENOTTY' in __video_do_ioctl. >>>> >>>> I don't really see how this would happen. As far as I can tell (and admittedly, >>>> the code is hard to follow) on such a system there is no need for any 32-bit to >>>> 64-bit conversion in v4l2-compat-ioctl32.c since struct v4l2_event_time32 is the >>>> same for both architectures. >>>> >>>> And in v4l2-ioctl.c VIDIOC_DQEVENT_TIME32 is handled correctly as far as I can >>>> tell. Where exactly does the return -ENOTTY happen? >>>> >>>> It definitely does not make sense to modify v4l2-compat-ioctl32.c if there is >>>> nothing to convert between 32 and 64 bit. If there is a bug, then that should be >>>> in v4l2-ioctl.c. >>>> >>>> Note that it is perfectly possible that there is a bug for arm64 and this ioctl, >>>> since that's the one combination I've never tested since I don't have a test setup >>>> for that. >>> >>> How did you test this anyway? With which libc/compiler? It would be nice if I could >>> test this myself on e.g. a Raspberry Pi. > > I added a test in v4l2-compliance and it appears that something is indeed wrong for arm64. > I'll dig deeper into this. I don't get ENOTTY though, instead it returns 0 but the contents > of the returned v4l2_event is garbled. > > It works fine for x86_64, but not for arm64. Actually, commit c344f07aa1b4 ("media: v4l2-core: ignore native time32 ioctls on 64-bit") fixes this issue. I was testing with an older kernel version on my RPi that didn't have this commit, and then it failed. With the latest kernel it's fine. I'm marking this series as Obsolete. Regards, Hans
Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64 the function 'v4l2_compat_translate_cmd' doesn't have a 'translation' for the cmd and so 'cmd' is returned as is. This later cause a failure '-ENOTTY' in __video_do_ioctl. This patchset fix it by defining VIDIOC_DQEVENT32_TIME32 to be VIDIOC_DQEVENT_TIME32 for non x86-64 arch and translate it to VIDIOC_DQEVENT. I tested the patchset with vicodec on x86_64 with both v4l2-ctl v4l2-ctl-32 and I tested it on arm64 with v4l2-ctl compiled to 32 bits, - chromeos userspace on mt8173 device. The command I used: $ modprobe vicdec $ v4l2-ctl -d7 --stream-mmap --stream-out-mmap --stream-from jelly_700-1000-YU12.fwht Witout that patchset the command holds: $ v4l2-ctl -d7 --stream-mmap --stream-out-mmap --stream-from jelly_700-1000-YU12.fwht >>>> This patchset also fixes a failing test video.DecodeAccelVD.h264 from the 'tast' tests on mtk-vcodec: tast -verbose run -build=false 10.42.0.175 video.DecodeAccelVD.h264 Dafna Hirschfeld (3): media: v4l2-core: move the code that copies v4l2_event_time32 to a function media: v4l2-core: combine code chunks under macro condition together media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++--------- drivers/media/v4l2-core/v4l2-ioctl.c | 41 +++++++++------ include/media/v4l2-ioctl.h | 9 ++++ 3 files changed, 61 insertions(+), 40 deletions(-)