* [PATCH] media: video-mux: fix compliance failures
@ 2018-05-22 16:29 Philipp Zabel
2018-05-22 17:47 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Philipp Zabel @ 2018-05-22 16:29 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, kernel, Philipp Zabel
Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
the available list of formats, and initialize pad and try formats.
Reported-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/media/platform/video-mux.c | 110 +++++++++++++++++++++++++++++
1 file changed, 110 insertions(+)
diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index 1fb887293337..ade1dae706aa 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
if (!source_mbusformat)
return -EINVAL;
+ /* No size limitations except V4L2 compliance requirements */
+ v4l_bound_align_image(&sdformat->format.width, 1, UINT_MAX - 1, 0,
+ &sdformat->format.height, 1, UINT_MAX - 1, 0, 0);
+
+ /* All formats except LVDS and vendor specific formats are acceptable */
+ switch (sdformat->format.code) {
+ case MEDIA_BUS_FMT_RGB444_1X12:
+ case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
+ case MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE:
+ case MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE:
+ case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
+ case MEDIA_BUS_FMT_RGB565_1X16:
+ case MEDIA_BUS_FMT_BGR565_2X8_BE:
+ case MEDIA_BUS_FMT_BGR565_2X8_LE:
+ case MEDIA_BUS_FMT_RGB565_2X8_BE:
+ case MEDIA_BUS_FMT_RGB565_2X8_LE:
+ case MEDIA_BUS_FMT_RGB666_1X18:
+ case MEDIA_BUS_FMT_RBG888_1X24:
+ case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
+ case MEDIA_BUS_FMT_BGR888_1X24:
+ case MEDIA_BUS_FMT_GBR888_1X24:
+ case MEDIA_BUS_FMT_RGB888_1X24:
+ case MEDIA_BUS_FMT_RGB888_2X12_BE:
+ case MEDIA_BUS_FMT_RGB888_2X12_LE:
+ case MEDIA_BUS_FMT_ARGB8888_1X32:
+ case MEDIA_BUS_FMT_RGB888_1X32_PADHI:
+ case MEDIA_BUS_FMT_RGB101010_1X30:
+ case MEDIA_BUS_FMT_RGB121212_1X36:
+ case MEDIA_BUS_FMT_RGB161616_1X48:
+ case MEDIA_BUS_FMT_Y8_1X8:
+ case MEDIA_BUS_FMT_UV8_1X8:
+ case MEDIA_BUS_FMT_UYVY8_1_5X8:
+ case MEDIA_BUS_FMT_VYUY8_1_5X8:
+ case MEDIA_BUS_FMT_YUYV8_1_5X8:
+ case MEDIA_BUS_FMT_YVYU8_1_5X8:
+ case MEDIA_BUS_FMT_UYVY8_2X8:
+ case MEDIA_BUS_FMT_VYUY8_2X8:
+ case MEDIA_BUS_FMT_YUYV8_2X8:
+ case MEDIA_BUS_FMT_YVYU8_2X8:
+ case MEDIA_BUS_FMT_Y10_1X10:
+ case MEDIA_BUS_FMT_UYVY10_2X10:
+ case MEDIA_BUS_FMT_VYUY10_2X10:
+ case MEDIA_BUS_FMT_YUYV10_2X10:
+ case MEDIA_BUS_FMT_YVYU10_2X10:
+ case MEDIA_BUS_FMT_Y12_1X12:
+ case MEDIA_BUS_FMT_UYVY12_2X12:
+ case MEDIA_BUS_FMT_VYUY12_2X12:
+ case MEDIA_BUS_FMT_YUYV12_2X12:
+ case MEDIA_BUS_FMT_YVYU12_2X12:
+ case MEDIA_BUS_FMT_UYVY8_1X16:
+ case MEDIA_BUS_FMT_VYUY8_1X16:
+ case MEDIA_BUS_FMT_YUYV8_1X16:
+ case MEDIA_BUS_FMT_YVYU8_1X16:
+ case MEDIA_BUS_FMT_YDYUYDYV8_1X16:
+ case MEDIA_BUS_FMT_UYVY10_1X20:
+ case MEDIA_BUS_FMT_VYUY10_1X20:
+ case MEDIA_BUS_FMT_YUYV10_1X20:
+ case MEDIA_BUS_FMT_YVYU10_1X20:
+ case MEDIA_BUS_FMT_VUY8_1X24:
+ case MEDIA_BUS_FMT_YUV8_1X24:
+ case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
+ case MEDIA_BUS_FMT_UYVY12_1X24:
+ case MEDIA_BUS_FMT_VYUY12_1X24:
+ case MEDIA_BUS_FMT_YUYV12_1X24:
+ case MEDIA_BUS_FMT_YVYU12_1X24:
+ case MEDIA_BUS_FMT_YUV10_1X30:
+ case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
+ case MEDIA_BUS_FMT_AYUV8_1X32:
+ case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
+ case MEDIA_BUS_FMT_YUV12_1X36:
+ case MEDIA_BUS_FMT_YUV16_1X48:
+ case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
+ case MEDIA_BUS_FMT_JPEG_1X8:
+ case MEDIA_BUS_FMT_AHSV8888_1X32:
+ break;
+ default:
+ sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
+ }
+ if (sdformat->format.field == V4L2_FIELD_ANY)
+ sdformat->format.field = V4L2_FIELD_NONE;
+
mutex_lock(&vmux->lock);
/* Source pad mirrors active sink pad, no limitations on sink pads */
@@ -197,11 +278,33 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
return 0;
}
+static int video_mux_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+ struct v4l2_mbus_framefmt *mbusformat;
+ int i;
+
+ mutex_lock(&vmux->lock);
+
+ for (i = 0; i < sd->entity.num_pads; i++) {
+ mbusformat = v4l2_subdev_get_try_format(sd, fh->pad, i);
+ *mbusformat = vmux->format_mbus[i];
+ }
+
+ mutex_unlock(&vmux->lock);
+
+ return 0;
+}
+
static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
.get_fmt = video_mux_get_format,
.set_fmt = video_mux_set_format,
};
+static const struct v4l2_subdev_internal_ops video_mux_internal_ops = {
+ .open = video_mux_open,
+};
+
static const struct v4l2_subdev_ops video_mux_subdev_ops = {
.pad = &video_mux_pad_ops,
.video = &video_mux_subdev_video_ops,
@@ -226,6 +329,7 @@ static int video_mux_probe(struct platform_device *pdev)
v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ vmux->subdev.internal_ops = &video_mux_internal_ops;
vmux->subdev.dev = dev;
/*
@@ -263,6 +367,12 @@ static int video_mux_probe(struct platform_device *pdev)
for (i = 0; i < num_pads - 1; i++)
vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
+ for (i = 0; i < num_pads; i++) {
+ vmux->format_mbus[i].width = 1;
+ vmux->format_mbus[i].height = 1;
+ vmux->format_mbus[i].code = MEDIA_BUS_FMT_Y8_1X8;
+ vmux->format_mbus[i].field = V4L2_FIELD_NONE;
+ }
vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
--
2.17.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] media: video-mux: fix compliance failures
2018-05-22 16:29 [PATCH] media: video-mux: fix compliance failures Philipp Zabel
@ 2018-05-22 17:47 ` Hans Verkuil
2018-05-23 8:47 ` Philipp Zabel
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2018-05-22 17:47 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Rui Miguel Silva, kernel
On 22/05/18 18:29, Philipp Zabel wrote:
> Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
> the available list of formats, and initialize pad and try formats.
>
> Reported-by: Rui Miguel Silva <rui.silva@linaro.org>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/media/platform/video-mux.c | 110 +++++++++++++++++++++++++++++
> 1 file changed, 110 insertions(+)
>
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index 1fb887293337..ade1dae706aa 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> if (!source_mbusformat)
> return -EINVAL;
>
> + /* No size limitations except V4L2 compliance requirements */
> + v4l_bound_align_image(&sdformat->format.width, 1, UINT_MAX - 1, 0,
> + &sdformat->format.height, 1, UINT_MAX - 1, 0, 0);
This is a bit dubious. I would pick more realistic min/max values like 16 and
65536. UINT_MAX - 1 will overflow whenever code increments/multiplies it for some
reason, which can cause all sorts of weird issues.
> +
> + /* All formats except LVDS and vendor specific formats are acceptable */
> + switch (sdformat->format.code) {
> + case MEDIA_BUS_FMT_RGB444_1X12:
> + case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
> + case MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE:
> + case MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE:
> + case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
> + case MEDIA_BUS_FMT_RGB565_1X16:
> + case MEDIA_BUS_FMT_BGR565_2X8_BE:
> + case MEDIA_BUS_FMT_BGR565_2X8_LE:
> + case MEDIA_BUS_FMT_RGB565_2X8_BE:
> + case MEDIA_BUS_FMT_RGB565_2X8_LE:
> + case MEDIA_BUS_FMT_RGB666_1X18:
> + case MEDIA_BUS_FMT_RBG888_1X24:
> + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> + case MEDIA_BUS_FMT_BGR888_1X24:
> + case MEDIA_BUS_FMT_GBR888_1X24:
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + case MEDIA_BUS_FMT_RGB888_2X12_BE:
> + case MEDIA_BUS_FMT_RGB888_2X12_LE:
> + case MEDIA_BUS_FMT_ARGB8888_1X32:
> + case MEDIA_BUS_FMT_RGB888_1X32_PADHI:
> + case MEDIA_BUS_FMT_RGB101010_1X30:
> + case MEDIA_BUS_FMT_RGB121212_1X36:
> + case MEDIA_BUS_FMT_RGB161616_1X48:
> + case MEDIA_BUS_FMT_Y8_1X8:
> + case MEDIA_BUS_FMT_UV8_1X8:
> + case MEDIA_BUS_FMT_UYVY8_1_5X8:
> + case MEDIA_BUS_FMT_VYUY8_1_5X8:
> + case MEDIA_BUS_FMT_YUYV8_1_5X8:
> + case MEDIA_BUS_FMT_YVYU8_1_5X8:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_VYUY8_2X8:
> + case MEDIA_BUS_FMT_YUYV8_2X8:
> + case MEDIA_BUS_FMT_YVYU8_2X8:
> + case MEDIA_BUS_FMT_Y10_1X10:
> + case MEDIA_BUS_FMT_UYVY10_2X10:
> + case MEDIA_BUS_FMT_VYUY10_2X10:
> + case MEDIA_BUS_FMT_YUYV10_2X10:
> + case MEDIA_BUS_FMT_YVYU10_2X10:
> + case MEDIA_BUS_FMT_Y12_1X12:
> + case MEDIA_BUS_FMT_UYVY12_2X12:
> + case MEDIA_BUS_FMT_VYUY12_2X12:
> + case MEDIA_BUS_FMT_YUYV12_2X12:
> + case MEDIA_BUS_FMT_YVYU12_2X12:
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> + case MEDIA_BUS_FMT_VYUY8_1X16:
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> + case MEDIA_BUS_FMT_YVYU8_1X16:
> + case MEDIA_BUS_FMT_YDYUYDYV8_1X16:
> + case MEDIA_BUS_FMT_UYVY10_1X20:
> + case MEDIA_BUS_FMT_VYUY10_1X20:
> + case MEDIA_BUS_FMT_YUYV10_1X20:
> + case MEDIA_BUS_FMT_YVYU10_1X20:
> + case MEDIA_BUS_FMT_VUY8_1X24:
> + case MEDIA_BUS_FMT_YUV8_1X24:
> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> + case MEDIA_BUS_FMT_UYVY12_1X24:
> + case MEDIA_BUS_FMT_VYUY12_1X24:
> + case MEDIA_BUS_FMT_YUYV12_1X24:
> + case MEDIA_BUS_FMT_YVYU12_1X24:
> + case MEDIA_BUS_FMT_YUV10_1X30:
> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> + case MEDIA_BUS_FMT_AYUV8_1X32:
> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
> + case MEDIA_BUS_FMT_YUV12_1X36:
> + case MEDIA_BUS_FMT_YUV16_1X48:
> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
> + case MEDIA_BUS_FMT_JPEG_1X8:
> + case MEDIA_BUS_FMT_AHSV8888_1X32:
> + break;
> + default:
> + sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
Add a break here.
> + }
> + if (sdformat->format.field == V4L2_FIELD_ANY)
> + sdformat->format.field = V4L2_FIELD_NONE;
> +
> mutex_lock(&vmux->lock);
>
> /* Source pad mirrors active sink pad, no limitations on sink pads */
> @@ -197,11 +278,33 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> return 0;
> }
>
> +static int video_mux_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_mbus_framefmt *mbusformat;
> + int i;
> +
> + mutex_lock(&vmux->lock);
> +
> + for (i = 0; i < sd->entity.num_pads; i++) {
> + mbusformat = v4l2_subdev_get_try_format(sd, fh->pad, i);
> + *mbusformat = vmux->format_mbus[i];
> + }
> +
> + mutex_unlock(&vmux->lock);
> +
> + return 0;
> +}
This isn't the right approach. Instead implement the init_cfg pad op.
> +
> static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> .get_fmt = video_mux_get_format,
> .set_fmt = video_mux_set_format,
> };
>
> +static const struct v4l2_subdev_internal_ops video_mux_internal_ops = {
> + .open = video_mux_open,
> +};
So this can be dropped.
> +
> static const struct v4l2_subdev_ops video_mux_subdev_ops = {
> .pad = &video_mux_pad_ops,
> .video = &video_mux_subdev_video_ops,
> @@ -226,6 +329,7 @@ static int video_mux_probe(struct platform_device *pdev)
> v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
> snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
> vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + vmux->subdev.internal_ops = &video_mux_internal_ops;
> vmux->subdev.dev = dev;
>
> /*
> @@ -263,6 +367,12 @@ static int video_mux_probe(struct platform_device *pdev)
> for (i = 0; i < num_pads - 1; i++)
> vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
> vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
> + for (i = 0; i < num_pads; i++) {
> + vmux->format_mbus[i].width = 1;
> + vmux->format_mbus[i].height = 1;
> + vmux->format_mbus[i].code = MEDIA_BUS_FMT_Y8_1X8;
> + vmux->format_mbus[i].field = V4L2_FIELD_NONE;
> + }
>
> vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
> ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
>
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: video-mux: fix compliance failures
2018-05-22 17:47 ` Hans Verkuil
@ 2018-05-23 8:47 ` Philipp Zabel
2018-05-23 8:53 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Philipp Zabel @ 2018-05-23 8:47 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: Rui Miguel Silva, kernel
Hi Hans,
thank you for the review comments.
On Tue, 2018-05-22 at 19:47 +0200, Hans Verkuil wrote:
> On 22/05/18 18:29, Philipp Zabel wrote:
> > Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
> > the available list of formats, and initialize pad and try formats.
> >
> > Reported-by: Rui Miguel Silva <rui.silva@linaro.org>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > drivers/media/platform/video-mux.c | 110 +++++++++++++++++++++++++++++
> > 1 file changed, 110 insertions(+)
> >
> > diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> > index 1fb887293337..ade1dae706aa 100644
> > --- a/drivers/media/platform/video-mux.c
> > +++ b/drivers/media/platform/video-mux.c
> > @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> > if (!source_mbusformat)
> > return -EINVAL;
> >
> > + /* No size limitations except V4L2 compliance requirements */
> > + v4l_bound_align_image(&sdformat->format.width, 1, UINT_MAX - 1, 0,
> > + &sdformat->format.height, 1, UINT_MAX - 1, 0, 0);
>
> This is a bit dubious. I would pick more realistic min/max values like 16 and
Why 16? A grayscale or RGB sensor could crop down to 1x1, see mt9v032
for example.
> 65536. UINT_MAX - 1 will overflow whenever code increments/multiplies it for some
> reason, which can cause all sorts of weird issues.
Ok. Should v4l2-compliance check for > 65536 then, instead of (or
additionally to) UINT_MAX?
> > +
> > + /* All formats except LVDS and vendor specific formats are acceptable */
> > + switch (sdformat->format.code) {
> > + case MEDIA_BUS_FMT_RGB444_1X12:
> > + case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
[...]
> > + case MEDIA_BUS_FMT_JPEG_1X8:
> > + case MEDIA_BUS_FMT_AHSV8888_1X32:
> > + break;
> > + default:
> > + sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
>
> Add a break here.
Will do.
> > + }
> > + if (sdformat->format.field == V4L2_FIELD_ANY)
> > + sdformat->format.field = V4L2_FIELD_NONE;
> > +
> > mutex_lock(&vmux->lock);
> >
> > /* Source pad mirrors active sink pad, no limitations on sink pads */
> > @@ -197,11 +278,33 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static int video_mux_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> > + struct v4l2_mbus_framefmt *mbusformat;
> > + int i;
> > +
> > + mutex_lock(&vmux->lock);
> > +
> > + for (i = 0; i < sd->entity.num_pads; i++) {
> > + mbusformat = v4l2_subdev_get_try_format(sd, fh->pad, i);
> > + *mbusformat = vmux->format_mbus[i];
> > + }
> > +
> > + mutex_unlock(&vmux->lock);
> > +
> > + return 0;
> > +}
>
> This isn't the right approach. Instead implement the init_cfg pad op.
How embarrassing, yes.
> > +
> > static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> > .get_fmt = video_mux_get_format,
> > .set_fmt = video_mux_set_format,
> > };
> >
> > +static const struct v4l2_subdev_internal_ops video_mux_internal_ops = {
> > + .open = video_mux_open,
> > +};
>
> So this can be dropped.
Ok, thanks!
regards
Philipp
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: video-mux: fix compliance failures
2018-05-23 8:47 ` Philipp Zabel
@ 2018-05-23 8:53 ` Hans Verkuil
2018-05-23 9:30 ` Philipp Zabel
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2018-05-23 8:53 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Rui Miguel Silva, kernel
On 23/05/18 10:47, Philipp Zabel wrote:
> Hi Hans,
>
> thank you for the review comments.
>
> On Tue, 2018-05-22 at 19:47 +0200, Hans Verkuil wrote:
>> On 22/05/18 18:29, Philipp Zabel wrote:
>>> Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
>>> the available list of formats, and initialize pad and try formats.
>>>
>>> Reported-by: Rui Miguel Silva <rui.silva@linaro.org>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>> drivers/media/platform/video-mux.c | 110 +++++++++++++++++++++++++++++
>>> 1 file changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>>> index 1fb887293337..ade1dae706aa 100644
>>> --- a/drivers/media/platform/video-mux.c
>>> +++ b/drivers/media/platform/video-mux.c
>>> @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
>>> if (!source_mbusformat)
>>> return -EINVAL;
>>>
>>> + /* No size limitations except V4L2 compliance requirements */
>>> + v4l_bound_align_image(&sdformat->format.width, 1, UINT_MAX - 1, 0,
>>> + &sdformat->format.height, 1, UINT_MAX - 1, 0, 0);
>>
>> This is a bit dubious. I would pick more realistic min/max values like 16 and
>
> Why 16? A grayscale or RGB sensor could crop down to 1x1, see mt9v032
> for example.
Was that ever tested? Just because the software allows it, doesn't mean it actually
works.
>
>> 65536. UINT_MAX - 1 will overflow whenever code increments/multiplies it for some
>> reason, which can cause all sorts of weird issues.
>
> Ok. Should v4l2-compliance check for > 65536 then, instead of (or
> additionally to) UINT_MAX?
I think so, yes.
>
>>> +
>>> + /* All formats except LVDS and vendor specific formats are acceptable */
>>> + switch (sdformat->format.code) {
>>> + case MEDIA_BUS_FMT_RGB444_1X12:
>>> + case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
> [...]
>>> + case MEDIA_BUS_FMT_JPEG_1X8:
>>> + case MEDIA_BUS_FMT_AHSV8888_1X32:
>>> + break;
>>> + default:
>>> + sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
>>
>> Add a break here.
>
> Will do.
>
>>> + }
>>> + if (sdformat->format.field == V4L2_FIELD_ANY)
>>> + sdformat->format.field = V4L2_FIELD_NONE;
>>> +
>>> mutex_lock(&vmux->lock);
>>>
>>> /* Source pad mirrors active sink pad, no limitations on sink pads */
>>> @@ -197,11 +278,33 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
>>> return 0;
>>> }
>>>
>>> +static int video_mux_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>>> +{
>>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>>> + struct v4l2_mbus_framefmt *mbusformat;
>>> + int i;
>>> +
>>> + mutex_lock(&vmux->lock);
>>> +
>>> + for (i = 0; i < sd->entity.num_pads; i++) {
>>> + mbusformat = v4l2_subdev_get_try_format(sd, fh->pad, i);
>>> + *mbusformat = vmux->format_mbus[i];
>>> + }
>>> +
>>> + mutex_unlock(&vmux->lock);
>>> +
>>> + return 0;
>>> +}
>>
>> This isn't the right approach. Instead implement the init_cfg pad op.
>
> How embarrassing, yes.
>
>>> +
>>> static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
>>> .get_fmt = video_mux_get_format,
>>> .set_fmt = video_mux_set_format,
>>> };
>>>
>>> +static const struct v4l2_subdev_internal_ops video_mux_internal_ops = {
>>> + .open = video_mux_open,
>>> +};
>>
>> So this can be dropped.
>
> Ok, thanks!
>
> regards
> Philipp
>
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: video-mux: fix compliance failures
2018-05-23 8:53 ` Hans Verkuil
@ 2018-05-23 9:30 ` Philipp Zabel
0 siblings, 0 replies; 5+ messages in thread
From: Philipp Zabel @ 2018-05-23 9:30 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: Rui Miguel Silva, kernel
On Wed, 2018-05-23 at 10:53 +0200, Hans Verkuil wrote:
> On 23/05/18 10:47, Philipp Zabel wrote:
> > Hi Hans,
> >
> > thank you for the review comments.
> >
> > On Tue, 2018-05-22 at 19:47 +0200, Hans Verkuil wrote:
> > > On 22/05/18 18:29, Philipp Zabel wrote:
> > > > Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
> > > > the available list of formats, and initialize pad and try formats.
> > > >
> > > > Reported-by: Rui Miguel Silva <rui.silva@linaro.org>
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > > drivers/media/platform/video-mux.c | 110 +++++++++++++++++++++++++++++
> > > > 1 file changed, 110 insertions(+)
> > > >
> > > > diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> > > > index 1fb887293337..ade1dae706aa 100644
> > > > --- a/drivers/media/platform/video-mux.c
> > > > +++ b/drivers/media/platform/video-mux.c
> > > > @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> > > > if (!source_mbusformat)
> > > > return -EINVAL;
> > > >
> > > > + /* No size limitations except V4L2 compliance requirements */
> > > > + v4l_bound_align_image(&sdformat->format.width, 1, UINT_MAX - 1, 0,
> > > > + &sdformat->format.height, 1, UINT_MAX - 1, 0, 0);
> > >
> > > This is a bit dubious. I would pick more realistic min/max values like 16 and
> >
> > Why 16? A grayscale or RGB sensor could crop down to 1x1, see mt9v032
> > for example.
>
> Was that ever tested? Just because the software allows it, doesn't mean it actually
> works.
I don't know. I'll test this when I get access to a sensor that could
support such low cropping.
I'd just prefer this artificial limit not to be imposed by the generic
video mux driver, as a mux doesn't care about framing.
For example the i.MX media driver currently has an (also artificial)
limit to 16 pixel aligned frame sizes in the CSI subdev anyway.
regards
Philipp
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-23 9:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-22 16:29 [PATCH] media: video-mux: fix compliance failures Philipp Zabel
2018-05-22 17:47 ` Hans Verkuil
2018-05-23 8:47 ` Philipp Zabel
2018-05-23 8:53 ` Hans Verkuil
2018-05-23 9:30 ` Philipp Zabel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.