Re: [RFCv2 PATCH 03/10] v4l2-compat-ioctl32: add g/s_matrix support.

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/22/2013 12:02 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday 12 August 2013 12:58:26 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 54 ++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 8f7a6a4..1d238da
>> 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -777,6 +777,43 @@ static int put_v4l2_subdev_edid32(struct
>> v4l2_subdev_edid *kp, struct v4l2_subde return 0;
>>  }
>>
>> +struct v4l2_matrix32 {
>> +	__u32 type;
>> +	union {
>> +		__u32 raw[4];
>> +	} ref;
>> +	struct v4l2_rect rect;
>> +	compat_caddr_t matrix;
>> +	__u32 reserved[12];
>> +} __attribute__ ((packed));
>> +
>> +static int get_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32
>> __user *up) +{
>> +	u32 tmp;
>> +
>> +	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_matrix32)) ||
>> +			get_user(kp->type, &up->type) ||
>> +			copy_from_user(&kp->ref, &up->ref, sizeof(up->ref)) ||
>> +			copy_from_user(&kp->rect, &up->rect, sizeof(up->rect)) ||
>> +			get_user(tmp, &up->matrix) ||
>> +			copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> 
> Shouldn't you align all lines to the ! in the first line ?

Will change.

> 
>> +		return -EFAULT;
>> +	kp->matrix = compat_ptr(tmp);
>> +	return 0;
>> +}
>> +
>> +static int put_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32
>> __user *up) +{
>> +	u32 tmp = (u32)((unsigned long)kp->matrix);
>> +
>> +	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_matrix32)) ||
>> +			put_user(kp->type, &up->type) ||
>> +			copy_to_user(&kp->ref, &up->ref, sizeof(kp->ref)) ||
> 
> Are driver allowed to change the type and ref fields ? If not those two lines 
> could be removed.

Good point, I'll drop that. 'ref' goes away anyway after Sakari's comments.

> 
>> +			copy_to_user(&kp->rect, &up->rect, sizeof(kp->rect)) ||
>> +			copy_to_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> 
> Same question regarding the alignment here.

Will change.

> 
>> +		return -EFAULT;
>> +	return 0;
>> +}
>>
>>  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
>>  #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
>> @@ -796,6 +833,8 @@ static int put_v4l2_subdev_edid32(struct
>> v4l2_subdev_edid *kp, struct v4l2_subde #define	VIDIOC_DQEVENT32	_IOR ('V',
>> 89, struct v4l2_event32)
>>  #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
>>  #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
>> +#define VIDIOC_G_MATRIX32	_IOWR('V', 104, struct v4l2_matrix32)
>> +#define VIDIOC_S_MATRIX32	_IOWR('V', 105, struct v4l2_matrix32)
>>
>>  #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
>>  #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
>> @@ -817,6 +856,7 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar struct v4l2_event v2ev;
>>  		struct v4l2_create_buffers v2crt;
>>  		struct v4l2_subdev_edid v2edid;
>> +		struct v4l2_matrix v2matrix;
>>  		unsigned long vx;
>>  		int vi;
>>  	} karg;
>> @@ -851,6 +891,8 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar case VIDIOC_PREPARE_BUF32: cmd =
>> VIDIOC_PREPARE_BUF; break;
>>  	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
>>  	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
>> +	case VIDIOC_G_MATRIX32: cmd = VIDIOC_G_MATRIX; break;
>> +	case VIDIOC_S_MATRIX32: cmd = VIDIOC_S_MATRIX; break;
>>  	}
>>
>>  	switch (cmd) {
>> @@ -922,6 +964,12 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar case VIDIOC_DQEVENT:
>>  		compatible_arg = 0;
>>  		break;
>> +
>> +	case VIDIOC_G_MATRIX:
>> +	case VIDIOC_S_MATRIX:
>> +		err = get_v4l2_matrix32(&karg.v2matrix, up);
>> +		compatible_arg = 0;
>> +		break;
>>  	}
>>  	if (err)
>>  		return err;
>> @@ -994,6 +1042,11 @@ static long do_video_ioctl(struct file *file, unsigned
>> int cmd, unsigned long ar case VIDIOC_ENUMINPUT:
>>  		err = put_v4l2_input32(&karg.v2i, up);
>>  		break;
>> +
>> +	case VIDIOC_G_MATRIX:
>> +	case VIDIOC_S_MATRIX:
>> +		err = put_v4l2_matrix32(&karg.v2matrix, up);
>> +		break;
>>  	}
>>  	return err;
>>  }
>> @@ -1089,6 +1142,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
>> int cmd, unsigned long arg) case VIDIOC_ENUM_FREQ_BANDS:
>>  	case VIDIOC_SUBDEV_G_EDID32:
>>  	case VIDIOC_SUBDEV_S_EDID32:
>> +	case VIDIOC_QUERY_MATRIX:
>>  		ret = do_video_ioctl(file, cmd, arg);
>>  		break;

Thanks for the comments!

	Hans