| Message ID | 1491604632-23544-6-git-send-email-helen.koike@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/08/2017 12:37 AM, Helen Koike wrote: > Allow user space to change the image format as the frame size, the > pixel format, colorspace, quantization, field YCbCr encoding > and the transfer function > > Signed-off-by: Helen Koike <helen.koike@collabora.com> > > --- > > Changes in v2: > [media] vimc: cap: Support several image formats > - this is a new commit in the serie (the old one was splitted in two) > - allow user space to change all fields from struct v4l2_pix_format > (e.g. colospace, quantization, field, xfer_func, ycbcr_enc) > - link_validate and try_fmt: also checks colospace, quantization, field, xfer_func, ycbcr_enc > - add struct v4l2_pix_format fmt_default > - add enum_framesizes > - enum_fmt_vid_cap: enumerate all formats from vimc_pix_map_table > - add mode dev_dbg > > > --- > drivers/media/platform/vimc/vimc-capture.c | 167 ++++++++++++++++++++++++----- > 1 file changed, 139 insertions(+), 28 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c > index 93f6a09..a6441f7 100644 > --- a/drivers/media/platform/vimc/vimc-capture.c > +++ b/drivers/media/platform/vimc/vimc-capture.c > @@ -40,6 +40,16 @@ struct vimc_cap_device { > struct media_pipeline pipe; > }; > > +static const struct v4l2_pix_format fmt_default = { > + .width = 640, > + .height = 480, > + .pixelformat = V4L2_PIX_FMT_RGB24, > + .field = V4L2_FIELD_NONE, > + .colorspace = V4L2_COLORSPACE_SRGB, > + .quantization = V4L2_QUANTIZATION_FULL_RANGE, > + .xfer_func = V4L2_XFER_FUNC_SRGB, I actually think we should keep .quantization and .xfer_func to 0 (DEFAULT). It's what most drivers will do. Same for the previous patch (I didn't mention it there). > +}; > + > struct vimc_cap_buffer { > /* > * struct vb2_v4l2_buffer must be the first element > @@ -64,7 +74,7 @@ static int vimc_cap_querycap(struct file *file, void *priv, > return 0; > } > > -static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, > +static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_format *f) > { > struct vimc_cap_device *vcap = video_drvdata(file); > @@ -74,16 +84,112 @@ static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, > return 0; > } > > +static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, > + struct v4l2_format *f) > +{ > + struct v4l2_pix_format *format = &f->fmt.pix; > + const struct vimc_pix_map *vpix; > + > + format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH, > + VIMC_FRAME_MAX_WIDTH); > + format->height = clamp_t(u32, format->height, VIMC_FRAME_MIN_HEIGHT, > + VIMC_FRAME_MAX_HEIGHT); > + > + /* Don't accept a pixelformat that is not on the table */ > + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); > + if (!vpix) { > + format->pixelformat = fmt_default.pixelformat; > + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); > + } > + /* TODO: Add support for custom bytesperline values */ > + format->bytesperline = format->width * vpix->bpp; > + format->sizeimage = format->bytesperline * format->height; > + > + if (format->field == V4L2_FIELD_ANY) > + format->field = fmt_default.field; > + > + /* Check if values are out of range */ > + if (format->colorspace == V4L2_COLORSPACE_DEFAULT > + || format->colorspace > V4L2_COLORSPACE_DCI_P3) > + format->colorspace = fmt_default.colorspace; > + if (format->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT > + || format->ycbcr_enc > V4L2_YCBCR_ENC_SMPTE240M) > + format->ycbcr_enc = fmt_default.ycbcr_enc; > + if (format->quantization == V4L2_QUANTIZATION_DEFAULT > + || format->quantization > V4L2_QUANTIZATION_LIM_RANGE) > + format->quantization = fmt_default.quantization; > + if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT > + || format->xfer_func > V4L2_XFER_FUNC_SMPTE2084) > + format->xfer_func = fmt_default.xfer_func; Same comments in the previous patch regarding width/height and the colorspace information apply here as well. > + > + return 0; > +} > + > +static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv, > + struct v4l2_format *f) > +{ > + struct vimc_cap_device *vcap = video_drvdata(file); > + > + /* Do not change the format while stream is on */ > + if (vb2_is_busy(&vcap->queue)) > + return -EBUSY; > + > + vimc_cap_try_fmt_vid_cap(file, priv, f); > + > + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: format update: " > + "old:%dx%d (0x%x, %d, %d, %d, %d) " > + "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, > + /* old */ > + vcap->format.width, vcap->format.height, > + vcap->format.pixelformat, vcap->format.colorspace, > + vcap->format.quantization, vcap->format.xfer_func, > + vcap->format.ycbcr_enc, > + /* new */ > + f->fmt.pix.width, f->fmt.pix.height, > + f->fmt.pix.pixelformat, f->fmt.pix.colorspace, > + f->fmt.pix.quantization, f->fmt.pix.xfer_func, > + f->fmt.pix.ycbcr_enc); > + > + vcap->format = f->fmt.pix; > + > + return 0; > +} > + > static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_fmtdesc *f) > { > - struct vimc_cap_device *vcap = video_drvdata(file); > + const struct vimc_pix_map *vpix = vimc_pix_map_by_index(f->index); > > - if (f->index > 0) > + if (!vpix) > return -EINVAL; > > - /* We only support one format for now */ > - f->pixelformat = vcap->format.pixelformat; > + f->pixelformat = vpix->pixelformat; > + f->flags = V4L2_FMT_FLAG_COMPRESSED; Why set this to COMPRESSED? The flag is set by the v4l2 core anyway, but the format we support here isn't a compressed format at all. Perhaps a left-over from an earlier version? > + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; No need for this, this op can only be called for such types anyway. > + > + return 0; > +} > + > +static int vimc_cap_enum_framesizes(struct file *file, void *fh, > + struct v4l2_frmsizeenum *fsize) > +{ > + const struct vimc_pix_map *vpix; > + > + if (fsize->index) > + return -EINVAL; > + > + /* Only accept code in the pix map table */ > + vpix = vimc_pix_map_by_code(fsize->pixel_format); > + if (!vpix) > + return -EINVAL; > + > + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > + fsize->stepwise.min_width = VIMC_FRAME_MIN_WIDTH; > + fsize->stepwise.max_width = VIMC_FRAME_MAX_WIDTH; > + fsize->stepwise.min_height = VIMC_FRAME_MIN_HEIGHT; > + fsize->stepwise.max_height = VIMC_FRAME_MAX_HEIGHT; > + fsize->stepwise.step_width = 1; > + fsize->stepwise.step_height = 1; I think the step should be at least 2. > > return 0; > } > @@ -101,10 +207,11 @@ static const struct v4l2_file_operations vimc_cap_fops = { > static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = { > .vidioc_querycap = vimc_cap_querycap, > > - .vidioc_g_fmt_vid_cap = vimc_cap_fmt_vid_cap, > - .vidioc_s_fmt_vid_cap = vimc_cap_fmt_vid_cap, > - .vidioc_try_fmt_vid_cap = vimc_cap_fmt_vid_cap, > + .vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap, > + .vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap, > + .vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap, > .vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap, > + .vidioc_enum_framesizes = vimc_cap_enum_framesizes, > > .vidioc_reqbufs = vb2_ioctl_reqbufs, > .vidioc_create_bufs = vb2_ioctl_create_bufs, > @@ -270,20 +377,21 @@ static int vimc_cap_link_validate(struct media_link *link) > if (ret) > return ret; > > - dev_dbg(vcap->vdev.v4l2_dev->dev, > - "%s: link validate formats src:%dx%d %d sink:%dx%d %d\n", > - vcap->vdev.name, > + vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); > + > + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: link validate formats: " > + "src:%dx%d (0x%x, %d, %d, %d, %d) " > + "snk:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, > + /* src */ > source_fmt.format.width, source_fmt.format.height, > - source_fmt.format.code, > + source_fmt.format.code, source_fmt.format.colorspace, > + source_fmt.format.quantization, source_fmt.format.xfer_func, > + source_fmt.format.ycbcr_enc, > + /* sink */ > sink_fmt->width, sink_fmt->height, > - sink_fmt->pixelformat); > - > - /* The width, height and code must match. */ > - vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); > - if (source_fmt.format.width != sink_fmt->width > - || source_fmt.format.height != sink_fmt->height > - || vpix->code != source_fmt.format.code) > - return -EPIPE; > + vpix->code, sink_fmt->colorspace, > + sink_fmt->quantization, sink_fmt->xfer_func, > + sink_fmt->ycbcr_enc); > > /* > * The field order must match, or the sink field order must be NONE > @@ -294,6 +402,15 @@ static int vimc_cap_link_validate(struct media_link *link) > sink_fmt->field != V4L2_FIELD_NONE) > return -EPIPE; > > + if (source_fmt.format.width != sink_fmt->width > + || source_fmt.format.height != sink_fmt->height > + || source_fmt.format.colorspace != sink_fmt->colorspace > + || source_fmt.format.quantization != sink_fmt->quantization > + || source_fmt.format.xfer_func != sink_fmt->xfer_func > + || source_fmt.format.ycbcr_enc != sink_fmt->ycbcr_enc You can't just compare this. I just discussed this with Philipp Zabel as well, and I don't think this should be included in the link validation, unless the hardware block can do actual colorspace conversions. In most cases this is irrelevant to the link validation. And if this has to be validated, then we need a helper function for this. Since one side can set quantization to the DEFAULT value while the other side sets it explicitly. Then you need to use the V4L2_MAP_QUANTIZATION_DEFAULT macro to figure out what the DEFAULT value really maps to before you compare the two sides. That requires a helper function, otherwise the driver code would become too complex. But the reality is that there really is no need to compare the two sides since none of this matters for how the data is processed. > + || vpix->code != source_fmt.format.code) > + return -EPIPE; > + > return 0; > } > > @@ -417,15 +534,9 @@ struct vimc_ent_device *vimc_cap_create(struct v4l2_device *v4l2_dev, > INIT_LIST_HEAD(&vcap->buf_list); > spin_lock_init(&vcap->qlock); > > - /* Set the frame format (this is hardcoded for now) */ > - vcap->format.width = 640; > - vcap->format.height = 480; > - vcap->format.pixelformat = V4L2_PIX_FMT_RGB24; > - vcap->format.field = V4L2_FIELD_NONE; > - vcap->format.colorspace = V4L2_COLORSPACE_SRGB; > - > + /* Set default frame format */ > + vcap->format = fmt_default; > vpix = vimc_pix_map_by_pixelformat(vcap->format.pixelformat); > - > vcap->format.bytesperline = vcap->format.width * vpix->bpp; > vcap->format.sizeimage = vcap->format.bytesperline * > vcap->format.height; > Regards, Hans
Hi Hans, Thank you for your review. I just have a question for this one. On 2017-05-08 08:53 AM, Hans Verkuil wrote: > On 04/08/2017 12:37 AM, Helen Koike wrote: >> Allow user space to change the image format as the frame size, the >> pixel format, colorspace, quantization, field YCbCr encoding >> and the transfer function >> >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> >> --- >> >> Changes in v2: >> [media] vimc: cap: Support several image formats >> - this is a new commit in the serie (the old one was splitted in two) >> - allow user space to change all fields from struct v4l2_pix_format >> (e.g. colospace, quantization, field, xfer_func, ycbcr_enc) >> - link_validate and try_fmt: also checks colospace, quantization, field, xfer_func, ycbcr_enc >> - add struct v4l2_pix_format fmt_default >> - add enum_framesizes >> - enum_fmt_vid_cap: enumerate all formats from vimc_pix_map_table >> - add mode dev_dbg >> >> >> --- >> drivers/media/platform/vimc/vimc-capture.c | 167 ++++++++++++++++++++++++----- >> 1 file changed, 139 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >> index 93f6a09..a6441f7 100644 >> --- a/drivers/media/platform/vimc/vimc-capture.c >> +++ b/drivers/media/platform/vimc/vimc-capture.c >> @@ -40,6 +40,16 @@ struct vimc_cap_device { >> struct media_pipeline pipe; >> }; >> >> +static const struct v4l2_pix_format fmt_default = { >> + .width = 640, >> + .height = 480, >> + .pixelformat = V4L2_PIX_FMT_RGB24, >> + .field = V4L2_FIELD_NONE, >> + .colorspace = V4L2_COLORSPACE_SRGB, >> + .quantization = V4L2_QUANTIZATION_FULL_RANGE, >> + .xfer_func = V4L2_XFER_FUNC_SRGB, > > I actually think we should keep .quantization and .xfer_func to 0 (DEFAULT). > It's what most drivers will do. Same for the previous patch (I didn't mention > it there). > >> +}; >> + >> struct vimc_cap_buffer { >> /* >> * struct vb2_v4l2_buffer must be the first element >> @@ -64,7 +74,7 @@ static int vimc_cap_querycap(struct file *file, void *priv, >> return 0; >> } >> >> -static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, >> +static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, >> struct v4l2_format *f) >> { >> struct vimc_cap_device *vcap = video_drvdata(file); >> @@ -74,16 +84,112 @@ static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, >> return 0; >> } >> >> +static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, >> + struct v4l2_format *f) >> +{ >> + struct v4l2_pix_format *format = &f->fmt.pix; >> + const struct vimc_pix_map *vpix; >> + >> + format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH, >> + VIMC_FRAME_MAX_WIDTH); >> + format->height = clamp_t(u32, format->height, VIMC_FRAME_MIN_HEIGHT, >> + VIMC_FRAME_MAX_HEIGHT); >> + >> + /* Don't accept a pixelformat that is not on the table */ >> + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); >> + if (!vpix) { >> + format->pixelformat = fmt_default.pixelformat; >> + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); >> + } >> + /* TODO: Add support for custom bytesperline values */ >> + format->bytesperline = format->width * vpix->bpp; >> + format->sizeimage = format->bytesperline * format->height; >> + >> + if (format->field == V4L2_FIELD_ANY) >> + format->field = fmt_default.field; >> + >> + /* Check if values are out of range */ >> + if (format->colorspace == V4L2_COLORSPACE_DEFAULT >> + || format->colorspace > V4L2_COLORSPACE_DCI_P3) >> + format->colorspace = fmt_default.colorspace; >> + if (format->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT >> + || format->ycbcr_enc > V4L2_YCBCR_ENC_SMPTE240M) >> + format->ycbcr_enc = fmt_default.ycbcr_enc; >> + if (format->quantization == V4L2_QUANTIZATION_DEFAULT >> + || format->quantization > V4L2_QUANTIZATION_LIM_RANGE) >> + format->quantization = fmt_default.quantization; >> + if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT >> + || format->xfer_func > V4L2_XFER_FUNC_SMPTE2084) >> + format->xfer_func = fmt_default.xfer_func; > > Same comments in the previous patch regarding width/height and the colorspace > information apply here as well. > >> + >> + return 0; >> +} >> + >> +static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv, >> + struct v4l2_format *f) >> +{ >> + struct vimc_cap_device *vcap = video_drvdata(file); >> + >> + /* Do not change the format while stream is on */ >> + if (vb2_is_busy(&vcap->queue)) >> + return -EBUSY; >> + >> + vimc_cap_try_fmt_vid_cap(file, priv, f); >> + >> + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: format update: " >> + "old:%dx%d (0x%x, %d, %d, %d, %d) " >> + "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, >> + /* old */ >> + vcap->format.width, vcap->format.height, >> + vcap->format.pixelformat, vcap->format.colorspace, >> + vcap->format.quantization, vcap->format.xfer_func, >> + vcap->format.ycbcr_enc, >> + /* new */ >> + f->fmt.pix.width, f->fmt.pix.height, >> + f->fmt.pix.pixelformat, f->fmt.pix.colorspace, >> + f->fmt.pix.quantization, f->fmt.pix.xfer_func, >> + f->fmt.pix.ycbcr_enc); >> + >> + vcap->format = f->fmt.pix; >> + >> + return 0; >> +} >> + >> static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv, >> struct v4l2_fmtdesc *f) >> { >> - struct vimc_cap_device *vcap = video_drvdata(file); >> + const struct vimc_pix_map *vpix = vimc_pix_map_by_index(f->index); >> >> - if (f->index > 0) >> + if (!vpix) >> return -EINVAL; >> >> - /* We only support one format for now */ >> - f->pixelformat = vcap->format.pixelformat; >> + f->pixelformat = vpix->pixelformat; >> + f->flags = V4L2_FMT_FLAG_COMPRESSED; > > Why set this to COMPRESSED? The flag is set by the v4l2 core anyway, but the format > we support here isn't a compressed format at all. Perhaps a left-over from an earlier > version? > >> + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > No need for this, this op can only be called for such types anyway. > >> + >> + return 0; >> +} >> + >> +static int vimc_cap_enum_framesizes(struct file *file, void *fh, >> + struct v4l2_frmsizeenum *fsize) >> +{ >> + const struct vimc_pix_map *vpix; >> + >> + if (fsize->index) >> + return -EINVAL; >> + >> + /* Only accept code in the pix map table */ >> + vpix = vimc_pix_map_by_code(fsize->pixel_format); >> + if (!vpix) >> + return -EINVAL; >> + >> + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; >> + fsize->stepwise.min_width = VIMC_FRAME_MIN_WIDTH; >> + fsize->stepwise.max_width = VIMC_FRAME_MAX_WIDTH; >> + fsize->stepwise.min_height = VIMC_FRAME_MIN_HEIGHT; >> + fsize->stepwise.max_height = VIMC_FRAME_MAX_HEIGHT; >> + fsize->stepwise.step_width = 1; >> + fsize->stepwise.step_height = 1; > > I think the step should be at least 2. > >> >> return 0; >> } >> @@ -101,10 +207,11 @@ static const struct v4l2_file_operations vimc_cap_fops = { >> static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = { >> .vidioc_querycap = vimc_cap_querycap, >> >> - .vidioc_g_fmt_vid_cap = vimc_cap_fmt_vid_cap, >> - .vidioc_s_fmt_vid_cap = vimc_cap_fmt_vid_cap, >> - .vidioc_try_fmt_vid_cap = vimc_cap_fmt_vid_cap, >> + .vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap, >> + .vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap, >> + .vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap, >> .vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap, >> + .vidioc_enum_framesizes = vimc_cap_enum_framesizes, >> >> .vidioc_reqbufs = vb2_ioctl_reqbufs, >> .vidioc_create_bufs = vb2_ioctl_create_bufs, >> @@ -270,20 +377,21 @@ static int vimc_cap_link_validate(struct media_link *link) >> if (ret) >> return ret; >> >> - dev_dbg(vcap->vdev.v4l2_dev->dev, >> - "%s: link validate formats src:%dx%d %d sink:%dx%d %d\n", >> - vcap->vdev.name, >> + vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); >> + >> + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: link validate formats: " >> + "src:%dx%d (0x%x, %d, %d, %d, %d) " >> + "snk:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, >> + /* src */ >> source_fmt.format.width, source_fmt.format.height, >> - source_fmt.format.code, >> + source_fmt.format.code, source_fmt.format.colorspace, >> + source_fmt.format.quantization, source_fmt.format.xfer_func, >> + source_fmt.format.ycbcr_enc, >> + /* sink */ >> sink_fmt->width, sink_fmt->height, >> - sink_fmt->pixelformat); >> - >> - /* The width, height and code must match. */ >> - vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); >> - if (source_fmt.format.width != sink_fmt->width >> - || source_fmt.format.height != sink_fmt->height >> - || vpix->code != source_fmt.format.code) >> - return -EPIPE; >> + vpix->code, sink_fmt->colorspace, >> + sink_fmt->quantization, sink_fmt->xfer_func, >> + sink_fmt->ycbcr_enc); >> >> /* >> * The field order must match, or the sink field order must be NONE >> @@ -294,6 +402,15 @@ static int vimc_cap_link_validate(struct media_link *link) >> sink_fmt->field != V4L2_FIELD_NONE) >> return -EPIPE; >> >> + if (source_fmt.format.width != sink_fmt->width >> + || source_fmt.format.height != sink_fmt->height >> + || source_fmt.format.colorspace != sink_fmt->colorspace >> + || source_fmt.format.quantization != sink_fmt->quantization >> + || source_fmt.format.xfer_func != sink_fmt->xfer_func >> + || source_fmt.format.ycbcr_enc != sink_fmt->ycbcr_enc > > You can't just compare this. I just discussed this with Philipp Zabel as well, > and I don't think this should be included in the link validation, unless the > hardware block can do actual colorspace conversions. > > In most cases this is irrelevant to the link validation. If it is irrelevant, then what it means to have different entities with different colorspaces? If I have a topology like [sensor]0->0[scaler]1->0[video cap] for example, the user can configure different colorspaces in each pad no? Should these configuration just be ignored ? > > And if this has to be validated, then we need a helper function for this. Since > one side can set quantization to the DEFAULT value while the other side sets it > explicitly. Then you need to use the V4L2_MAP_QUANTIZATION_DEFAULT macro to > figure out what the DEFAULT value really maps to before you compare the two sides. > > That requires a helper function, otherwise the driver code would become too complex. > > But the reality is that there really is no need to compare the two sides since > none of this matters for how the data is processed. > >> + || vpix->code != source_fmt.format.code) >> + return -EPIPE; >> + >> return 0; >> } >> >> @@ -417,15 +534,9 @@ struct vimc_ent_device *vimc_cap_create(struct v4l2_device *v4l2_dev, >> INIT_LIST_HEAD(&vcap->buf_list); >> spin_lock_init(&vcap->qlock); >> >> - /* Set the frame format (this is hardcoded for now) */ >> - vcap->format.width = 640; >> - vcap->format.height = 480; >> - vcap->format.pixelformat = V4L2_PIX_FMT_RGB24; >> - vcap->format.field = V4L2_FIELD_NONE; >> - vcap->format.colorspace = V4L2_COLORSPACE_SRGB; >> - >> + /* Set default frame format */ >> + vcap->format = fmt_default; >> vpix = vimc_pix_map_by_pixelformat(vcap->format.pixelformat); >> - >> vcap->format.bytesperline = vcap->format.width * vpix->bpp; >> vcap->format.sizeimage = vcap->format.bytesperline * >> vcap->format.height; >> > > Regards, > > Hans > Thanks LN
On 05/29/2017 07:48 PM, Helen Koike wrote: > Hi Hans, > > Thank you for your review. I just have a question for this one. > > On 2017-05-08 08:53 AM, Hans Verkuil wrote: >> On 04/08/2017 12:37 AM, Helen Koike wrote: >>> Allow user space to change the image format as the frame size, the >>> pixel format, colorspace, quantization, field YCbCr encoding >>> and the transfer function >>> >>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>> >>> --- >>> >>> Changes in v2: >>> [media] vimc: cap: Support several image formats >>> - this is a new commit in the serie (the old one was splitted in two) >>> - allow user space to change all fields from struct v4l2_pix_format >>> (e.g. colospace, quantization, field, xfer_func, ycbcr_enc) >>> - link_validate and try_fmt: also checks colospace, quantization, field, xfer_func, ycbcr_enc >>> - add struct v4l2_pix_format fmt_default >>> - add enum_framesizes >>> - enum_fmt_vid_cap: enumerate all formats from vimc_pix_map_table >>> - add mode dev_dbg >>> >>> >>> --- >>> drivers/media/platform/vimc/vimc-capture.c | 167 ++++++++++++++++++++++++----- >>> 1 file changed, 139 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>> index 93f6a09..a6441f7 100644 >>> --- a/drivers/media/platform/vimc/vimc-capture.c >>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>> @@ -40,6 +40,16 @@ struct vimc_cap_device { >>> struct media_pipeline pipe; >>> }; >>> >>> +static const struct v4l2_pix_format fmt_default = { >>> + .width = 640, >>> + .height = 480, >>> + .pixelformat = V4L2_PIX_FMT_RGB24, >>> + .field = V4L2_FIELD_NONE, >>> + .colorspace = V4L2_COLORSPACE_SRGB, >>> + .quantization = V4L2_QUANTIZATION_FULL_RANGE, >>> + .xfer_func = V4L2_XFER_FUNC_SRGB, >> >> I actually think we should keep .quantization and .xfer_func to 0 (DEFAULT). >> It's what most drivers will do. Same for the previous patch (I didn't mention >> it there). >> >>> +}; >>> + >>> struct vimc_cap_buffer { >>> /* >>> * struct vb2_v4l2_buffer must be the first element >>> @@ -64,7 +74,7 @@ static int vimc_cap_querycap(struct file *file, void *priv, >>> return 0; >>> } >>> >>> -static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, >>> +static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, >>> struct v4l2_format *f) >>> { >>> struct vimc_cap_device *vcap = video_drvdata(file); >>> @@ -74,16 +84,112 @@ static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, >>> return 0; >>> } >>> >>> +static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, >>> + struct v4l2_format *f) >>> +{ >>> + struct v4l2_pix_format *format = &f->fmt.pix; >>> + const struct vimc_pix_map *vpix; >>> + >>> + format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH, >>> + VIMC_FRAME_MAX_WIDTH); >>> + format->height = clamp_t(u32, format->height, VIMC_FRAME_MIN_HEIGHT, >>> + VIMC_FRAME_MAX_HEIGHT); >>> + >>> + /* Don't accept a pixelformat that is not on the table */ >>> + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); >>> + if (!vpix) { >>> + format->pixelformat = fmt_default.pixelformat; >>> + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); >>> + } >>> + /* TODO: Add support for custom bytesperline values */ >>> + format->bytesperline = format->width * vpix->bpp; >>> + format->sizeimage = format->bytesperline * format->height; >>> + >>> + if (format->field == V4L2_FIELD_ANY) >>> + format->field = fmt_default.field; >>> + >>> + /* Check if values are out of range */ >>> + if (format->colorspace == V4L2_COLORSPACE_DEFAULT >>> + || format->colorspace > V4L2_COLORSPACE_DCI_P3) >>> + format->colorspace = fmt_default.colorspace; >>> + if (format->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT >>> + || format->ycbcr_enc > V4L2_YCBCR_ENC_SMPTE240M) >>> + format->ycbcr_enc = fmt_default.ycbcr_enc; >>> + if (format->quantization == V4L2_QUANTIZATION_DEFAULT >>> + || format->quantization > V4L2_QUANTIZATION_LIM_RANGE) >>> + format->quantization = fmt_default.quantization; >>> + if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT >>> + || format->xfer_func > V4L2_XFER_FUNC_SMPTE2084) >>> + format->xfer_func = fmt_default.xfer_func; >> >> Same comments in the previous patch regarding width/height and the colorspace >> information apply here as well. >> >>> + >>> + return 0; >>> +} >>> + >>> +static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv, >>> + struct v4l2_format *f) >>> +{ >>> + struct vimc_cap_device *vcap = video_drvdata(file); >>> + >>> + /* Do not change the format while stream is on */ >>> + if (vb2_is_busy(&vcap->queue)) >>> + return -EBUSY; >>> + >>> + vimc_cap_try_fmt_vid_cap(file, priv, f); >>> + >>> + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: format update: " >>> + "old:%dx%d (0x%x, %d, %d, %d, %d) " >>> + "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, >>> + /* old */ >>> + vcap->format.width, vcap->format.height, >>> + vcap->format.pixelformat, vcap->format.colorspace, >>> + vcap->format.quantization, vcap->format.xfer_func, >>> + vcap->format.ycbcr_enc, >>> + /* new */ >>> + f->fmt.pix.width, f->fmt.pix.height, >>> + f->fmt.pix.pixelformat, f->fmt.pix.colorspace, >>> + f->fmt.pix.quantization, f->fmt.pix.xfer_func, >>> + f->fmt.pix.ycbcr_enc); >>> + >>> + vcap->format = f->fmt.pix; >>> + >>> + return 0; >>> +} >>> + >>> static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv, >>> struct v4l2_fmtdesc *f) >>> { >>> - struct vimc_cap_device *vcap = video_drvdata(file); >>> + const struct vimc_pix_map *vpix = vimc_pix_map_by_index(f->index); >>> >>> - if (f->index > 0) >>> + if (!vpix) >>> return -EINVAL; >>> >>> - /* We only support one format for now */ >>> - f->pixelformat = vcap->format.pixelformat; >>> + f->pixelformat = vpix->pixelformat; >>> + f->flags = V4L2_FMT_FLAG_COMPRESSED; >> >> Why set this to COMPRESSED? The flag is set by the v4l2 core anyway, but the format >> we support here isn't a compressed format at all. Perhaps a left-over from an earlier >> version? >> >>> + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> >> No need for this, this op can only be called for such types anyway. >> >>> + >>> + return 0; >>> +} >>> + >>> +static int vimc_cap_enum_framesizes(struct file *file, void *fh, >>> + struct v4l2_frmsizeenum *fsize) >>> +{ >>> + const struct vimc_pix_map *vpix; >>> + >>> + if (fsize->index) >>> + return -EINVAL; >>> + >>> + /* Only accept code in the pix map table */ >>> + vpix = vimc_pix_map_by_code(fsize->pixel_format); >>> + if (!vpix) >>> + return -EINVAL; >>> + >>> + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; >>> + fsize->stepwise.min_width = VIMC_FRAME_MIN_WIDTH; >>> + fsize->stepwise.max_width = VIMC_FRAME_MAX_WIDTH; >>> + fsize->stepwise.min_height = VIMC_FRAME_MIN_HEIGHT; >>> + fsize->stepwise.max_height = VIMC_FRAME_MAX_HEIGHT; >>> + fsize->stepwise.step_width = 1; >>> + fsize->stepwise.step_height = 1; >> >> I think the step should be at least 2. >> >>> >>> return 0; >>> } >>> @@ -101,10 +207,11 @@ static const struct v4l2_file_operations vimc_cap_fops = { >>> static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = { >>> .vidioc_querycap = vimc_cap_querycap, >>> >>> - .vidioc_g_fmt_vid_cap = vimc_cap_fmt_vid_cap, >>> - .vidioc_s_fmt_vid_cap = vimc_cap_fmt_vid_cap, >>> - .vidioc_try_fmt_vid_cap = vimc_cap_fmt_vid_cap, >>> + .vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap, >>> + .vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap, >>> + .vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap, >>> .vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap, >>> + .vidioc_enum_framesizes = vimc_cap_enum_framesizes, >>> >>> .vidioc_reqbufs = vb2_ioctl_reqbufs, >>> .vidioc_create_bufs = vb2_ioctl_create_bufs, >>> @@ -270,20 +377,21 @@ static int vimc_cap_link_validate(struct media_link *link) >>> if (ret) >>> return ret; >>> >>> - dev_dbg(vcap->vdev.v4l2_dev->dev, >>> - "%s: link validate formats src:%dx%d %d sink:%dx%d %d\n", >>> - vcap->vdev.name, >>> + vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); >>> + >>> + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: link validate formats: " >>> + "src:%dx%d (0x%x, %d, %d, %d, %d) " >>> + "snk:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, >>> + /* src */ >>> source_fmt.format.width, source_fmt.format.height, >>> - source_fmt.format.code, >>> + source_fmt.format.code, source_fmt.format.colorspace, >>> + source_fmt.format.quantization, source_fmt.format.xfer_func, >>> + source_fmt.format.ycbcr_enc, >>> + /* sink */ >>> sink_fmt->width, sink_fmt->height, >>> - sink_fmt->pixelformat); >>> - >>> - /* The width, height and code must match. */ >>> - vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); >>> - if (source_fmt.format.width != sink_fmt->width >>> - || source_fmt.format.height != sink_fmt->height >>> - || vpix->code != source_fmt.format.code) >>> - return -EPIPE; >>> + vpix->code, sink_fmt->colorspace, >>> + sink_fmt->quantization, sink_fmt->xfer_func, >>> + sink_fmt->ycbcr_enc); >>> >>> /* >>> * The field order must match, or the sink field order must be NONE >>> @@ -294,6 +402,15 @@ static int vimc_cap_link_validate(struct media_link *link) >>> sink_fmt->field != V4L2_FIELD_NONE) >>> return -EPIPE; >>> >>> + if (source_fmt.format.width != sink_fmt->width >>> + || source_fmt.format.height != sink_fmt->height >>> + || source_fmt.format.colorspace != sink_fmt->colorspace >>> + || source_fmt.format.quantization != sink_fmt->quantization >>> + || source_fmt.format.xfer_func != sink_fmt->xfer_func >>> + || source_fmt.format.ycbcr_enc != sink_fmt->ycbcr_enc >> >> You can't just compare this. I just discussed this with Philipp Zabel as well, >> and I don't think this should be included in the link validation, unless the >> hardware block can do actual colorspace conversions. >> >> In most cases this is irrelevant to the link validation. > > > If it is irrelevant, then what it means to have different entities with > different colorspaces? > > If I have a topology like [sensor]0->0[scaler]1->0[video cap] for > example, the user can configure different colorspaces in each pad no? > Should these configuration just be ignored ? Yes. To be honest, this is something we need to clarify. It certainly shouldn't be used in the validation check, but it is not really defined what should be done when userspace sets these fields to non-0 values. Right now I think most drivers just set it and do not otherwise touch it. I looked at this with Philipp Zabel recently for the imx6 driver: https://patchwork.linuxtv.org/patch/41451/ What is done there is probably the best solution. Regards, Hans
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c index 93f6a09..a6441f7 100644 --- a/drivers/media/platform/vimc/vimc-capture.c +++ b/drivers/media/platform/vimc/vimc-capture.c @@ -40,6 +40,16 @@ struct vimc_cap_device { struct media_pipeline pipe; }; +static const struct v4l2_pix_format fmt_default = { + .width = 640, + .height = 480, + .pixelformat = V4L2_PIX_FMT_RGB24, + .field = V4L2_FIELD_NONE, + .colorspace = V4L2_COLORSPACE_SRGB, + .quantization = V4L2_QUANTIZATION_FULL_RANGE, + .xfer_func = V4L2_XFER_FUNC_SRGB, +}; + struct vimc_cap_buffer { /* * struct vb2_v4l2_buffer must be the first element @@ -64,7 +74,7 @@ static int vimc_cap_querycap(struct file *file, void *priv, return 0; } -static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, +static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f) { struct vimc_cap_device *vcap = video_drvdata(file); @@ -74,16 +84,112 @@ static int vimc_cap_fmt_vid_cap(struct file *file, void *priv, return 0; } +static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, + struct v4l2_format *f) +{ + struct v4l2_pix_format *format = &f->fmt.pix; + const struct vimc_pix_map *vpix; + + format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH, + VIMC_FRAME_MAX_WIDTH); + format->height = clamp_t(u32, format->height, VIMC_FRAME_MIN_HEIGHT, + VIMC_FRAME_MAX_HEIGHT); + + /* Don't accept a pixelformat that is not on the table */ + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); + if (!vpix) { + format->pixelformat = fmt_default.pixelformat; + vpix = vimc_pix_map_by_pixelformat(format->pixelformat); + } + /* TODO: Add support for custom bytesperline values */ + format->bytesperline = format->width * vpix->bpp; + format->sizeimage = format->bytesperline * format->height; + + if (format->field == V4L2_FIELD_ANY) + format->field = fmt_default.field; + + /* Check if values are out of range */ + if (format->colorspace == V4L2_COLORSPACE_DEFAULT + || format->colorspace > V4L2_COLORSPACE_DCI_P3) + format->colorspace = fmt_default.colorspace; + if (format->ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT + || format->ycbcr_enc > V4L2_YCBCR_ENC_SMPTE240M) + format->ycbcr_enc = fmt_default.ycbcr_enc; + if (format->quantization == V4L2_QUANTIZATION_DEFAULT + || format->quantization > V4L2_QUANTIZATION_LIM_RANGE) + format->quantization = fmt_default.quantization; + if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT + || format->xfer_func > V4L2_XFER_FUNC_SMPTE2084) + format->xfer_func = fmt_default.xfer_func; + + return 0; +} + +static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv, + struct v4l2_format *f) +{ + struct vimc_cap_device *vcap = video_drvdata(file); + + /* Do not change the format while stream is on */ + if (vb2_is_busy(&vcap->queue)) + return -EBUSY; + + vimc_cap_try_fmt_vid_cap(file, priv, f); + + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: format update: " + "old:%dx%d (0x%x, %d, %d, %d, %d) " + "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, + /* old */ + vcap->format.width, vcap->format.height, + vcap->format.pixelformat, vcap->format.colorspace, + vcap->format.quantization, vcap->format.xfer_func, + vcap->format.ycbcr_enc, + /* new */ + f->fmt.pix.width, f->fmt.pix.height, + f->fmt.pix.pixelformat, f->fmt.pix.colorspace, + f->fmt.pix.quantization, f->fmt.pix.xfer_func, + f->fmt.pix.ycbcr_enc); + + vcap->format = f->fmt.pix; + + return 0; +} + static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv, struct v4l2_fmtdesc *f) { - struct vimc_cap_device *vcap = video_drvdata(file); + const struct vimc_pix_map *vpix = vimc_pix_map_by_index(f->index); - if (f->index > 0) + if (!vpix) return -EINVAL; - /* We only support one format for now */ - f->pixelformat = vcap->format.pixelformat; + f->pixelformat = vpix->pixelformat; + f->flags = V4L2_FMT_FLAG_COMPRESSED; + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + + return 0; +} + +static int vimc_cap_enum_framesizes(struct file *file, void *fh, + struct v4l2_frmsizeenum *fsize) +{ + const struct vimc_pix_map *vpix; + + if (fsize->index) + return -EINVAL; + + /* Only accept code in the pix map table */ + vpix = vimc_pix_map_by_code(fsize->pixel_format); + if (!vpix) + return -EINVAL; + + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; + fsize->stepwise.min_width = VIMC_FRAME_MIN_WIDTH; + fsize->stepwise.max_width = VIMC_FRAME_MAX_WIDTH; + fsize->stepwise.min_height = VIMC_FRAME_MIN_HEIGHT; + fsize->stepwise.max_height = VIMC_FRAME_MAX_HEIGHT; + fsize->stepwise.step_width = 1; + fsize->stepwise.step_height = 1; return 0; } @@ -101,10 +207,11 @@ static const struct v4l2_file_operations vimc_cap_fops = { static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = { .vidioc_querycap = vimc_cap_querycap, - .vidioc_g_fmt_vid_cap = vimc_cap_fmt_vid_cap, - .vidioc_s_fmt_vid_cap = vimc_cap_fmt_vid_cap, - .vidioc_try_fmt_vid_cap = vimc_cap_fmt_vid_cap, + .vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap, + .vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap, + .vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap, .vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap, + .vidioc_enum_framesizes = vimc_cap_enum_framesizes, .vidioc_reqbufs = vb2_ioctl_reqbufs, .vidioc_create_bufs = vb2_ioctl_create_bufs, @@ -270,20 +377,21 @@ static int vimc_cap_link_validate(struct media_link *link) if (ret) return ret; - dev_dbg(vcap->vdev.v4l2_dev->dev, - "%s: link validate formats src:%dx%d %d sink:%dx%d %d\n", - vcap->vdev.name, + vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); + + dev_dbg(vcap->vdev.v4l2_dev->dev, "%s: link validate formats: " + "src:%dx%d (0x%x, %d, %d, %d, %d) " + "snk:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name, + /* src */ source_fmt.format.width, source_fmt.format.height, - source_fmt.format.code, + source_fmt.format.code, source_fmt.format.colorspace, + source_fmt.format.quantization, source_fmt.format.xfer_func, + source_fmt.format.ycbcr_enc, + /* sink */ sink_fmt->width, sink_fmt->height, - sink_fmt->pixelformat); - - /* The width, height and code must match. */ - vpix = vimc_pix_map_by_pixelformat(sink_fmt->pixelformat); - if (source_fmt.format.width != sink_fmt->width - || source_fmt.format.height != sink_fmt->height - || vpix->code != source_fmt.format.code) - return -EPIPE; + vpix->code, sink_fmt->colorspace, + sink_fmt->quantization, sink_fmt->xfer_func, + sink_fmt->ycbcr_enc); /* * The field order must match, or the sink field order must be NONE @@ -294,6 +402,15 @@ static int vimc_cap_link_validate(struct media_link *link) sink_fmt->field != V4L2_FIELD_NONE) return -EPIPE; + if (source_fmt.format.width != sink_fmt->width + || source_fmt.format.height != sink_fmt->height + || source_fmt.format.colorspace != sink_fmt->colorspace + || source_fmt.format.quantization != sink_fmt->quantization + || source_fmt.format.xfer_func != sink_fmt->xfer_func + || source_fmt.format.ycbcr_enc != sink_fmt->ycbcr_enc + || vpix->code != source_fmt.format.code) + return -EPIPE; + return 0; } @@ -417,15 +534,9 @@ struct vimc_ent_device *vimc_cap_create(struct v4l2_device *v4l2_dev, INIT_LIST_HEAD(&vcap->buf_list); spin_lock_init(&vcap->qlock); - /* Set the frame format (this is hardcoded for now) */ - vcap->format.width = 640; - vcap->format.height = 480; - vcap->format.pixelformat = V4L2_PIX_FMT_RGB24; - vcap->format.field = V4L2_FIELD_NONE; - vcap->format.colorspace = V4L2_COLORSPACE_SRGB; - + /* Set default frame format */ + vcap->format = fmt_default; vpix = vimc_pix_map_by_pixelformat(vcap->format.pixelformat); - vcap->format.bytesperline = vcap->format.width * vpix->bpp; vcap->format.sizeimage = vcap->format.bytesperline * vcap->format.height;
Allow user space to change the image format as the frame size, the pixel format, colorspace, quantization, field YCbCr encoding and the transfer function Signed-off-by: Helen Koike <helen.koike@collabora.com> --- Changes in v2: [media] vimc: cap: Support several image formats - this is a new commit in the serie (the old one was splitted in two) - allow user space to change all fields from struct v4l2_pix_format (e.g. colospace, quantization, field, xfer_func, ycbcr_enc) - link_validate and try_fmt: also checks colospace, quantization, field, xfer_func, ycbcr_enc - add struct v4l2_pix_format fmt_default - add enum_framesizes - enum_fmt_vid_cap: enumerate all formats from vimc_pix_map_table - add mode dev_dbg --- drivers/media/platform/vimc/vimc-capture.c | 167 ++++++++++++++++++++++++----- 1 file changed, 139 insertions(+), 28 deletions(-)