Re: [PATCH v2 5/7] [media] vimc: cap: Support several image formats

[Helen Koike <helen.koike@xxxxxxxxxxxxx>]

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


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