Re: [PATCH v3 5/8] media: staging/imx: Remove capture_device_set_format

[Steve Longerbeam <slongerbeam@xxxxxxxxx>]

On 5/2/19 3:01 AM, Rui Miguel Silva wrote:
> Hi Steve,
> Thanks for v3 with bisect fixed.
>
> On Tue 30 Apr 2019 at 23:50, Steve Longerbeam wrote:
> > Don't propagate the source pad format to the connected capture device.
> > It's now the responsibility of userspace to call VIDIOC_S_FMT on the
> > capture device to ensure the capture format and compose rectangle
> > are compatible with the connected source. To check this, validate
> > the capture format with the source before streaming starts.
> >
> > Signed-off-by: Steve Longerbeam <slongerbeam@xxxxxxxxx>
> > ---
> >   drivers/staging/media/imx/imx-ic-prpencvf.c   | 16 +----
> >   drivers/staging/media/imx/imx-media-capture.c | 64 +++++++++++++------
> >   drivers/staging/media/imx/imx-media-csi.c     | 16 +----
> >   drivers/staging/media/imx/imx-media.h         |  2 -
> >   drivers/staging/media/imx/imx7-media-csi.c    | 17 +----
> >   5 files changed, 50 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > index afaa3a8b15e9..63334fd61492 100644
> > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > @@ -906,9 +906,7 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
> >   		       struct v4l2_subdev_format *sdformat)
> >   {
> >   	struct prp_priv *priv = sd_to_priv(sd);
> > -	struct imx_media_video_dev *vdev = priv->vdev;
> >   	const struct imx_media_pixfmt *cc;
> > -	struct v4l2_pix_format vdev_fmt;
> >   	struct v4l2_mbus_framefmt *fmt;
> >   	int ret = 0;
> >   
> > @@ -945,19 +943,9 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
> >   			priv->cc[PRPENCVF_SRC_PAD] = outcc;
> >   	}
> >   
> > -	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY)
> > -		goto out;
> > -
> > -	priv->cc[sdformat->pad] = cc;
> > +	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		priv->cc[sdformat->pad] = cc;
> >   
> > -	/* propagate output pad format to capture device */
> > -	imx_media_mbus_fmt_to_pix_fmt(&vdev_fmt,
> > -				      &priv->format_mbus[PRPENCVF_SRC_PAD],
> > -				      priv->cc[PRPENCVF_SRC_PAD]);
> > -	mutex_unlock(&priv->lock);
> > -	imx_media_capture_device_set_format(vdev, &vdev_fmt);
> > -
> > -	return 0;
> >   out:
> >   	mutex_unlock(&priv->lock);
> >   	return ret;
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index 555f6204660b..b77a67bda47c 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -205,7 +205,8 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> >   
> >   static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> >   				     struct v4l2_subdev_format *fmt_src,
> > -				     struct v4l2_format *f)
> > +				     struct v4l2_format *f,
> > +				     struct v4l2_rect *compose)
> >   {
> >   	const struct imx_media_pixfmt *cc, *cc_src;
> >   
> > @@ -247,6 +248,13 @@ static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> >   
> >   	imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src->format, cc);
> >   
> > +	if (compose) {
> > +		compose->left = 0;
> > +		compose->top = 0;
> > +		compose->width = fmt_src->format.width;
> > +		compose->height = fmt_src->format.height;
> > +	}
> > +
> >   	return 0;
> >   }
> >   
> > @@ -263,7 +271,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> >   	if (ret)
> >   		return ret;
> >   
> > -	return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> > +	return __capture_try_fmt_vid_cap(priv, &fmt_src, f, NULL);
> >   }
> >   
> >   static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> > @@ -271,6 +279,7 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> >   {
> >   	struct capture_priv *priv = video_drvdata(file);
> >   	struct v4l2_subdev_format fmt_src;
> > +	struct v4l2_rect compose;
> >   	int ret;
> >   
> >   	if (vb2_is_busy(&priv->q)) {
> > @@ -284,17 +293,14 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> >   	if (ret)
> >   		return ret;
> >   
> > -	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> > +	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f, &compose);
> >   	if (ret)
> >   		return ret;
> >   
> >   	priv->vdev.fmt.fmt.pix = f->fmt.pix;
> >   	priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
> >   					      CS_SEL_ANY, true);
> > -	priv->vdev.compose.left = 0;
> > -	priv->vdev.compose.top = 0;
> > -	priv->vdev.compose.width = fmt_src.format.width;
> > -	priv->vdev.compose.height = fmt_src.format.height;
> > +	priv->vdev.compose = compose;
> >   
> >   	return 0;
> >   }
> > @@ -524,6 +530,33 @@ static void capture_buf_queue(struct vb2_buffer *vb)
> >   	spin_unlock_irqrestore(&priv->q_lock, flags);
> >   }
> >   
> > +static int capture_validate_fmt(struct capture_priv *priv)
> > +{
> > +	struct v4l2_subdev_format fmt_src;
> > +	struct v4l2_rect compose;
> > +	struct v4l2_format f;
> > +	int ret;
> > +
> > +	fmt_src.pad = priv->src_sd_pad;
> > +	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> > +	if (ret)
> > +		return ret;
> > +
> > +	v4l2_fill_pix_format(&f.fmt.pix, &fmt_src.format);
> > +
> > +	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, &f, &compose);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return (priv->vdev.fmt.fmt.pix.width != f.fmt.pix.width ||
> > +		priv->vdev.fmt.fmt.pix.height != f.fmt.pix.height ||
> > +		priv->vdev.cc->cs !=
> > +		ipu_pixelformat_to_colorspace(f.fmt.pix.pixelformat) ||
> This fails on imx7, no ipu, it returns unknown colorspace.

Oops, thanks for catching.

> Removing this check everything works ok on my setup with this
> series.
> Do not know the best way to fix this but you may have? :)

Maybe the best fix is simply to remove the check for same colorspace, 
verifying the rectangles is probably good enough.


> > +		priv->vdev.compose.width != compose.width ||
> > +		priv->vdev.compose.height != compose.height) ? -EINVAL : 0;
> > +}
> > +
> >   static int capture_start_streaming(struct vb2_queue *vq, unsigned int count)
> >   {
> >   	struct capture_priv *priv = vb2_get_drv_priv(vq);
> > @@ -531,6 +564,10 @@ static int capture_start_streaming(struct vb2_queue *vq, unsigned int count)
> >   	unsigned long flags;
> >   	int ret;
> >   
> > +	ret = capture_validate_fmt(priv);
> > +	if (ret)
> Maybe some verbose output here to let know userland what failed.

Ok.

Steve

> > +		goto return_bufs;
> > +
> >   	ret = imx_media_pipeline_set_stream(priv->md, &priv->src_sd->entity,
> >   					    true);
> >   	if (ret) {
> > @@ -654,19 +691,6 @@ static struct video_device capture_videodev = {
> >   	.device_caps	= V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING,
> >   };
> ---
> Cheers,
> 	Rui

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel