| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <edcf9f4f-d798-ce03-a59b-6bdcd77557a3@gmail.com>
Date: Thu, 2 May 2019 10:02:20 -0700
From: Steve Longerbeam <slongerbeam@...il.com>
To: Rui Miguel Silva <rmfrfs@...il.com>
Cc: linux-media@...r.kernel.org,
Philipp Zabel <p.zabel@...gutronix.de>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
NXP Linux Team <linux-imx@....com>,
"open list:STAGING SUBSYSTEM" <devel@...verdev.osuosl.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 5/8] media: staging/imx: Remove
capture_device_set_format
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@...il.com>
>> ---
>> 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
>
Powered by blists - more mailing lists