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@xxxxxxxxxxxxx>
>
> ---
>
> 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
