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@xxxxxxxxxx> > > > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxx> > > > > > + } > > > + > > > + 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 -- Sakari Ailus e-mail: sakari.ailus@xxxxxx
