diff mbox

[v2] media: video-mux: fix compliance failures

Message ID 20180523092423.4386-1-p.zabel@pengutronix.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Philipp Zabel May 23, 2018, 9:24 a.m. UTC 
Limit frame sizes to the [1, 65536] 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>
---
Changes since v1:
 - Limit to [1, 65536] instead of [1, UINT_MAX - 1]
 - Add missing break in default case
 - Use .init_cfg pad op instead of .open internal op
---
 drivers/media/platform/video-mux.c | 108 +++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

Comments

Rui Miguel Silva May 24, 2018, 11:30 a.m. UTC | #1 
Hi,
On Wed 23 May 2018 at 09:24, Philipp Zabel wrote:
> Limit frame sizes to the [1, 65536] 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>
> ---
> Changes since v1:
>  - Limit to [1, 65536] instead of [1, UINT_MAX - 1]
>  - Add missing break in default case
>  - Use .init_cfg pad op instead of .open internal op
> ---

FWIW, in i.MX7:
Tested-by: Rui Miguel Silva <rui.silva@linaro.org>

v4l2-compliance SHA   : 47d43b130dc6e9e0edc900759fb37649208371e4

Compliance test for device /dev/v4l-subdev1:

Media Driver Info:
        Driver name      : imx7-csi
        Model            : imx-media
        Serial           : 
        Bus info         : 
        Media version    : 4.17.0
        Hardware revision: 0x00000000 (0)
        Driver version   : 4.17.0
Interface Info:
        ID               : 0x0300001b
        Type             : V4L Sub-Device
Entity Info:
        ID               : 0x0000000a (10)
        Name             : csi_mux
        Function         : Video Muxer
        Pad 0x0100000b   : Sink
        Pad 0x0100000c   : Sink
          Link 0x02000013: from remote pad 0x1000010 of entity 
          'imx7-mipi-csis.0': Data, Enabled
        Pad 0x0100000d   : Source
          Link 0x02000015: to remote pad 0x1000002 of entity 
          'csi': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK

Allow for multiple opens:
        test second /dev/v4l-subdev1 open: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Sub-Device ioctls (Sink Pad 0):
        test Try 
        VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK 
        (Not Supported)
        test Try VIDIOC_SUBDEV_G/S_FMT: OK
        test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not 
        Supported)
        test Active 
        VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK 
        (Not Supported)
        test Active VIDIOC_SUBDEV_G/S_FMT: OK
        test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not 
        Supported)
        test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Sub-Device ioctls (Sink Pad 1):
        test Try 
        VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK 
        (Not Supported)
        test Try VIDIOC_SUBDEV_G/S_FMT: OK
        test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not 
        Supported)
        test Active 
        VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK 
        (Not Supported)
        test Active VIDIOC_SUBDEV_G/S_FMT: OK
        test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not 
        Supported)
        test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Sub-Device ioctls (Source Pad 2):
        test Try 
        VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK 
        (Not Supported)
        test Try VIDIOC_SUBDEV_G/S_FMT: OK
        test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not 
        Supported)
        test Active 
        VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK 
        (Not Supported)
        test Active VIDIOC_SUBDEV_G/S_FMT: OK
        test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not 
        Supported)
        test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
        test VIDIOC_QUERYCTRL: OK (Not Supported)
        test VIDIOC_G/S_CTRL: OK (Not Supported)
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not 
        Supported)
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 0 Private Controls: 0

Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not 
        Supported)
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK (Not Supported)
        test VIDIOC_TRY_FMT: OK (Not Supported)
        test VIDIOC_S_FMT: OK (Not Supported)
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls:
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not 
        Supported)
        test VIDIOC_EXPBUF: OK (Not Supported)

Total: 61, Succeeded: 61, Failed: 0, Warnings: 0
Sakari Ailus May 24, 2018, 11:38 a.m. UTC | #2 
Hi Philipp,

Thanks for the patch.

On Wed, May 23, 2018 at 11:24:23AM +0200, Philipp Zabel wrote:
> Limit frame sizes to the [1, 65536] 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>
> ---
> Changes since v1:
>  - Limit to [1, 65536] instead of [1, UINT_MAX - 1]
>  - Add missing break in default case
>  - Use .init_cfg pad op instead of .open internal op
> ---
>  drivers/media/platform/video-mux.c | 108 +++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index 1fb887293337..d27cb42ce6b1 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -180,6 +180,88 @@ 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, 65536, 0,
> +			      &sdformat->format.height, 1, 65536, 0, 0);

Why 65536? And not e.g. U32_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_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;
> +		break;
> +	}
> +	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,7 +279,27 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int video_mux_init_cfg(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_mbus_framefmt *mbusformat;
> +	int i;

unsigned int i

> +
> +	mutex_lock(&vmux->lock);
> +
> +	for (i = 0; i < sd->entity.num_pads; i++) {
> +		mbusformat = v4l2_subdev_get_try_format(sd, cfg, i);
> +		*mbusformat = vmux->format_mbus[i];

The initial format is the default one, not the current configured format.

With these addressed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +	}
> +
> +	mutex_unlock(&vmux->lock);
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> +	.init_cfg = video_mux_init_cfg,
>  	.get_fmt = video_mux_get_format,
>  	.set_fmt = video_mux_set_format,
>  };
> @@ -263,6 +365,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
>
Philipp Zabel May 24, 2018, 12:42 p.m. UTC | #3 
Hi Sakari,

thank you for the review comments.

On Thu, 2018-05-24 at 14:38 +0300, Sakari Ailus wrote:
> Hi Philipp,
> 
> Thanks for the patch.
> 
> On Wed, May 23, 2018 at 11:24:23AM +0200, Philipp Zabel wrote:
> > Limit frame sizes to the [1, 65536] 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>
> > ---
> > Changes since v1:
> >  - Limit to [1, 65536] instead of [1, UINT_MAX - 1]
> >  - Add missing break in default case
> >  - Use .init_cfg pad op instead of .open internal op
> > ---
> >  drivers/media/platform/video-mux.c | 108 +++++++++++++++++++++++++++++
> >  1 file changed, 108 insertions(+)
> > 
> > diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> > index 1fb887293337..d27cb42ce6b1 100644
> > --- a/drivers/media/platform/video-mux.c
> > +++ b/drivers/media/platform/video-mux.c
> > @@ -180,6 +180,88 @@ 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, 65536, 0,
> > +			      &sdformat->format.height, 1, 65536, 0, 0);
> 
> Why 65536? And not e.g. U32_MAX?

v4l2-compliance submits a format struct memset to all 0xff and complains
if width or height return U32_MAX.
I first evaded this by limiting to UINT_MAX - 1, but Hans suggested I
reduce this to 65536 as a more realistic maximum [1].

[1] https://patchwork.linuxtv.org/patch/49827/

[...]
> > @@ -197,7 +279,27 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int video_mux_init_cfg(struct v4l2_subdev *sd,
> > +			      struct v4l2_subdev_pad_config *cfg)
> > +{
> > +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> > +	struct v4l2_mbus_framefmt *mbusformat;
> > +	int i;
> 
> unsigned int i

Ok.

> > +
> > +	mutex_lock(&vmux->lock);
> > +
> > +	for (i = 0; i < sd->entity.num_pads; i++) {
> > +		mbusformat = v4l2_subdev_get_try_format(sd, cfg, i);
> > +		*mbusformat = vmux->format_mbus[i];
> 
> The initial format is the default one, not the current configured format.

I'm fine with changing that if that is the expected behavior.
Is this what was meant with:
  "Try formats do not depend on active formats, but can depend on the
   current links configuration or sub-device controls value." [1]
? I read that as currently active formats should not limit the try
formats accepted on the other pad.

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/vidioc-subdev-g-fmt.html#description

> With these addressed,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> > +	}
> > +
> > +	mutex_unlock(&vmux->lock);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> > +	.init_cfg = video_mux_init_cfg,
> >  	.get_fmt = video_mux_get_format,
> >  	.set_fmt = video_mux_set_format,
> >  };
> > @@ -263,6 +365,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++) {

I'll also turn this i into unsigned int.

> > +		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

regards
Philipp
Sakari Ailus May 24, 2018, 2:21 p.m. UTC | #4 
On Thu, May 24, 2018 at 02:42:41PM +0200, Philipp Zabel wrote:
> Hi Sakari,
> 
> thank you for the review comments.
> 
> On Thu, 2018-05-24 at 14:38 +0300, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > Thanks for the patch.
> > 
> > On Wed, May 23, 2018 at 11:24:23AM +0200, Philipp Zabel wrote:
> > > Limit frame sizes to the [1, 65536] 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>
> > > ---
> > > Changes since v1:
> > >  - Limit to [1, 65536] instead of [1, UINT_MAX - 1]
> > >  - Add missing break in default case
> > >  - Use .init_cfg pad op instead of .open internal op
> > > ---
> > >  drivers/media/platform/video-mux.c | 108 +++++++++++++++++++++++++++++
> > >  1 file changed, 108 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> > > index 1fb887293337..d27cb42ce6b1 100644
> > > --- a/drivers/media/platform/video-mux.c
> > > +++ b/drivers/media/platform/video-mux.c
> > > @@ -180,6 +180,88 @@ 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, 65536, 0,
> > > +			      &sdformat->format.height, 1, 65536, 0, 0);
> > 
> > Why 65536? And not e.g. U32_MAX?
> 
> v4l2-compliance submits a format struct memset to all 0xff and complains
> if width or height return U32_MAX.
> I first evaded this by limiting to UINT_MAX - 1, but Hans suggested I
> reduce this to 65536 as a more realistic maximum [1].
> 
> [1] https://patchwork.linuxtv.org/patch/49827/

Ack.

> 
> [...]
> > > @@ -197,7 +279,27 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> > >  	return 0;
> > >  }
> > >  
> > > +static int video_mux_init_cfg(struct v4l2_subdev *sd,
> > > +			      struct v4l2_subdev_pad_config *cfg)
> > > +{
> > > +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> > > +	struct v4l2_mbus_framefmt *mbusformat;
> > > +	int i;
> > 
> > unsigned int i
> 
> Ok.
> 
> > > +
> > > +	mutex_lock(&vmux->lock);
> > > +
> > > +	for (i = 0; i < sd->entity.num_pads; i++) {
> > > +		mbusformat = v4l2_subdev_get_try_format(sd, cfg, i);
> > > +		*mbusformat = vmux->format_mbus[i];
> > 
> > The initial format is the default one, not the current configured format.
> 
> I'm fine with changing that if that is the expected behavior.
> Is this what was meant with:
>   "Try formats do not depend on active formats, but can depend on the
>    current links configuration or sub-device controls value." [1]
> ? I read that as currently active formats should not limit the try
> formats accepted on the other pad.

Correct. Note that this is just the initial try format for the file handle.

> 
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/vidioc-subdev-g-fmt.html#description
> 
> > With these addressed,
> > 
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > > +	}
> > > +
> > > +	mutex_unlock(&vmux->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> > > +	.init_cfg = video_mux_init_cfg,
> > >  	.get_fmt = video_mux_get_format,
> > >  	.set_fmt = video_mux_set_format,
> > >  };
> > > @@ -263,6 +365,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++) {
> 
> I'll also turn this i into unsigned int.

Sounds good.

> 
> > > +		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
> 
> regards
> Philipp
diff mbox

Patch

diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index 1fb887293337..d27cb42ce6b1 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -180,6 +180,88 @@  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, 65536, 0,
+			      &sdformat->format.height, 1, 65536, 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;
+		break;
+	}
+	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,7 +279,27 @@  static int video_mux_set_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int video_mux_init_cfg(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_pad_config *cfg)
+{
+	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, cfg, i);
+		*mbusformat = vmux->format_mbus[i];
+	}
+
+	mutex_unlock(&vmux->lock);
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
+	.init_cfg = video_mux_init_cfg,
 	.get_fmt = video_mux_get_format,
 	.set_fmt = video_mux_set_format,
 };
@@ -263,6 +365,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,