[PATCH 0/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[PATCH 0/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Dafna Hirschfeld]

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(-)

-- 
2.17.1

[PATCH 1/3] media: v4l2-core: move the code that copies v4l2_event_time32 to a function

[Dafna Hirschfeld]

Move the code that copies v4l2_event_time32 to function
"put_v4l2_event_time32". This function will also be used
for the case of 64-bit kernel other than x86_64 in later
commits.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 41 +++++++++++++++++-----------
 include/media/v4l2-ioctl.h           |  6 ++++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 05d5db3d85e5..24bdb2383595 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3167,6 +3167,28 @@ static int video_get_user(void __user *arg, void *parg,
 	return err;
 }
 
+int put_v4l2_event_time32(void *parg, void __user *arg)
+{
+	struct v4l2_event *ev = parg;
+	struct v4l2_event_time32 ev32;
+
+	memset(&ev32, 0, sizeof(ev32));
+
+	ev32.type	= ev->type;
+	ev32.pending	= ev->pending;
+	ev32.sequence	= ev->sequence;
+	ev32.timestamp.tv_sec	= ev->timestamp.tv_sec;
+	ev32.timestamp.tv_nsec	= ev->timestamp.tv_nsec;
+	ev32.id		= ev->id;
+
+	memcpy(&ev32.u, &ev->u, sizeof(ev->u));
+	memcpy(&ev32.reserved, &ev->reserved, sizeof(ev->reserved));
+
+	if (copy_to_user(arg, &ev32, sizeof(ev32)))
+		return -EFAULT;
+	return 0;
+}
+
 static int video_put_user(void __user *arg, void *parg,
 			  unsigned int real_cmd, unsigned int cmd)
 {
@@ -3186,23 +3208,10 @@ static int video_put_user(void __user *arg, void *parg,
 #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
 	switch (cmd) {
 	case VIDIOC_DQEVENT_TIME32: {
-		struct v4l2_event *ev = parg;
-		struct v4l2_event_time32 ev32;
-
-		memset(&ev32, 0, sizeof(ev32));
+		int ret = put_v4l2_event_time32(parg, arg);
 
-		ev32.type	= ev->type;
-		ev32.pending	= ev->pending;
-		ev32.sequence	= ev->sequence;
-		ev32.timestamp.tv_sec	= ev->timestamp.tv_sec;
-		ev32.timestamp.tv_nsec	= ev->timestamp.tv_nsec;
-		ev32.id		= ev->id;
-
-		memcpy(&ev32.u, &ev->u, sizeof(ev->u));
-		memcpy(&ev32.reserved, &ev->reserved, sizeof(ev->reserved));
-
-		if (copy_to_user(arg, &ev32, sizeof(ev32)))
-			return -EFAULT;
+		if (ret)
+			return ret;
 		break;
 	}
 	case VIDIOC_QUERYBUF_TIME32:
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index edb733f21604..0c209bbbc76f 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -784,6 +784,12 @@ struct v4l2_buffer_time32 {
 		__u32		reserved;
 	};
 };
+
+/*
+ * This function is used to copy the struct v4l2_event_time32 to userspace
+ */
+int put_v4l2_event_time32(void *parg, void __user *arg);
+
 #define VIDIOC_QUERYBUF_TIME32	_IOWR('V',  9, struct v4l2_buffer_time32)
 #define VIDIOC_QBUF_TIME32	_IOWR('V', 15, struct v4l2_buffer_time32)
 #define VIDIOC_DQBUF_TIME32	_IOWR('V', 17, struct v4l2_buffer_time32)
-- 
2.17.1

[PATCH 2/3] media: v4l2-core: combine code chunks under macro condition together

[Dafna Hirschfeld]

The struct v4l2_event32_time32 and the function put_v4l2_event32_time32
are both defined under the condition 'ifdef CONFIG_COMPAT_32BIT_TIME'.
In order to make the code more readable, move both definitions under
one 'ifdef'.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 30 +++++++++----------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 47aff3b19742..5623003a9705 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -765,21 +765,6 @@ struct v4l2_event32 {
 	__u32				reserved[8];
 };
 
-#ifdef CONFIG_COMPAT_32BIT_TIME
-struct v4l2_event32_time32 {
-	__u32				type;
-	union {
-		compat_s64		value64;
-		__u8			data[64];
-	} u;
-	__u32				pending;
-	__u32				sequence;
-	struct old_timespec32		timestamp;
-	__u32				id;
-	__u32				reserved[8];
-};
-#endif
-
 static int put_v4l2_event32(struct v4l2_event *p64,
 			    struct v4l2_event32 __user *p32)
 {
@@ -796,6 +781,19 @@ static int put_v4l2_event32(struct v4l2_event *p64,
 }
 
 #ifdef CONFIG_COMPAT_32BIT_TIME
+struct v4l2_event32_time32 {
+	__u32				type;
+	union {
+		compat_s64		value64;
+		__u8			data[64];
+	} u;
+	__u32				pending;
+	__u32				sequence;
+	struct old_timespec32		timestamp;
+	__u32				id;
+	__u32				reserved[8];
+};
+
 static int put_v4l2_event32_time32(struct v4l2_event *p64,
 				   struct v4l2_event32_time32 __user *p32)
 {
@@ -811,7 +809,7 @@ static int put_v4l2_event32_time32(struct v4l2_event *p64,
 	return 0;
 }
 #endif
-#endif
+#endif /* CONFIG_X86_64 */
 
 struct v4l2_edid32 {
 	__u32 pad;
-- 
2.17.1

[PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Dafna Hirschfeld]

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.
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);
 
-- 
2.17.1

Re: [PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Hans Verkuil]

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);
>  
> 

Re: [PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Hans Verkuil]

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);
>>  
>>
> 

Re: [PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Laurent Pinchart]

(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

Re: [PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Hans Verkuil]

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);
>>>>  
> 

Re: [PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Hans Verkuil]

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