From: Hans Verkuil <hverkuil@xs4all.nl>
To: Benoit Parrot <bparrot@ti.com>
Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [Patch v2 08/13] media: am437x-vpfe: Use a per instance format array instead of a static one
Date: Fri, 20 Sep 2019 10:19:21 +0200 [thread overview]
Message-ID: <a73280f1-4fc0-2ea2-2611-9e410f3f0689@xs4all.nl> (raw)
In-Reply-To: <20190919204125.15254-9-bparrot@ti.com>
On 9/19/19 10:41 PM, Benoit Parrot wrote:
> Using a statically defined format array would cause issue when
> multiple vpfe instance would be connected to sub-device of
> different capabilities. We need to use an instance based array
> instead to properly maintain a per port/instance format list.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
> drivers/media/platform/am437x/am437x-vpfe.c | 110 ++++++++------------
> drivers/media/platform/am437x/am437x-vpfe.h | 36 +++++++
> 2 files changed, 77 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 8b218f48428f..569618a52082 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -69,31 +69,7 @@ static const struct vpfe_standard vpfe_standards[] = {
> {V4L2_STD_625_50, 720, 576, {54, 59}, 1},
> };
>
> -struct bus_format {
> - unsigned int width;
> - unsigned int bpp;
> -};
> -
> -/*
> - * struct vpfe_fmt - VPFE media bus format information
> - * @code: V4L2 media bus format code
> - * @shifted: V4L2 media bus format code for the same pixel layout but
> - * shifted to be 8 bits per pixel. =0 if format is not shiftable.
> - * @pixelformat: V4L2 pixel format FCC identifier
> - * @width: Bits per pixel (when transferred over a bus)
> - * @bpp: Bytes per pixel (when stored in memory)
> - * @supported: Indicates format supported by subdev
> - */
> -struct vpfe_fmt {
> - u32 fourcc;
> - u32 code;
> - struct bus_format l;
> - struct bus_format s;
> - bool supported;
> - u32 index;
> -};
> -
> -static struct vpfe_fmt formats[] = {
> +static struct vpfe_fmt formats[VPFE_MAX_ACTIVE_FMT] = {
> {
> .fourcc = V4L2_PIX_FMT_YUYV,
> .code = MEDIA_BUS_FMT_YUYV8_2X8,
> @@ -101,7 +77,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 4,
> .s.width = 8,
> .s.bpp = 2,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_UYVY,
> .code = MEDIA_BUS_FMT_UYVY8_2X8,
> @@ -109,7 +84,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 4,
> .s.width = 8,
> .s.bpp = 2,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_YVYU,
> .code = MEDIA_BUS_FMT_YVYU8_2X8,
> @@ -117,7 +91,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 4,
> .s.width = 8,
> .s.bpp = 2,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_VYUY,
> .code = MEDIA_BUS_FMT_VYUY8_2X8,
> @@ -125,7 +98,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 4,
> .s.width = 8,
> .s.bpp = 2,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_SBGGR8,
> .code = MEDIA_BUS_FMT_SBGGR8_1X8,
> @@ -133,7 +105,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 2,
> .s.width = 8,
> .s.bpp = 1,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_SGBRG8,
> .code = MEDIA_BUS_FMT_SGBRG8_1X8,
> @@ -141,7 +112,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 2,
> .s.width = 8,
> .s.bpp = 1,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_SGRBG8,
> .code = MEDIA_BUS_FMT_SGRBG8_1X8,
> @@ -149,7 +119,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 2,
> .s.width = 8,
> .s.bpp = 1,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_SRGGB8,
> .code = MEDIA_BUS_FMT_SRGGB8_1X8,
> @@ -157,7 +126,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 2,
> .s.width = 8,
> .s.bpp = 1,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_RGB565,
> .code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> @@ -165,7 +133,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 4,
> .s.width = 8,
> .s.bpp = 2,
> - .supported = false,
> }, {
> .fourcc = V4L2_PIX_FMT_RGB565X,
> .code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> @@ -173,7 +140,6 @@ static struct vpfe_fmt formats[] = {
> .l.bpp = 4,
> .s.width = 8,
> .s.bpp = 2,
> - .supported = false,
> },
> };
>
> @@ -181,13 +147,14 @@ static int
> __vpfe_get_format(struct vpfe_device *vpfe,
> struct v4l2_format *format, unsigned int *bpp);
>
> -static struct vpfe_fmt *find_format_by_code(unsigned int code)
> +static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> + unsigned int code)
> {
> struct vpfe_fmt *fmt;
> unsigned int k;
>
> - for (k = 0; k < ARRAY_SIZE(formats); k++) {
> - fmt = &formats[k];
> + for (k = 0; k < vpfe->num_active_fmt; k++) {
> + fmt = vpfe->active_fmt[k];
> if (fmt->code == code)
> return fmt;
> }
> @@ -195,13 +162,14 @@ static struct vpfe_fmt *find_format_by_code(unsigned int code)
> return NULL;
> }
>
> -static struct vpfe_fmt *find_format_by_pix(unsigned int pixelformat)
> +static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> + unsigned int pixelformat)
> {
> struct vpfe_fmt *fmt;
> unsigned int k;
>
> - for (k = 0; k < ARRAY_SIZE(formats); k++) {
> - fmt = &formats[k];
> + for (k = 0; k < vpfe->num_active_fmt; k++) {
> + fmt = vpfe->active_fmt[k];
> if (fmt->fourcc == pixelformat)
> return fmt;
> }
> @@ -218,7 +186,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
> unsigned int bus_width = sdinfo->vpfe_param.bus_width;
> struct vpfe_fmt *fmt;
>
> - fmt = find_format_by_code(mbus->code);
> + fmt = find_format_by_code(vpfe, mbus->code);
> if (WARN_ON(fmt == NULL)) {
> pr_err("Invalid mbus code set\n");
> *bpp = 1;
> @@ -241,12 +209,12 @@ static void pix_to_mbus(struct vpfe_device *vpfe,
> {
> struct vpfe_fmt *fmt;
>
> - fmt = find_format_by_pix(pix_fmt->pixelformat);
> + fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
> if (!fmt) {
> /* default to first entry */
> vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
> pix_fmt->pixelformat);
> - fmt = &formats[0];
> + fmt = vpfe->active_fmt[0];
> }
>
> memset(mbus_fmt, 0, sizeof(*mbus_fmt));
> @@ -1494,8 +1462,7 @@ static int vpfe_enum_fmt(struct file *file, void *priv,
> {
> struct vpfe_device *vpfe = video_drvdata(file);
> struct vpfe_subdev_info *sdinfo;
> - struct vpfe_fmt *fmt = NULL;
> - unsigned int k;
> + struct vpfe_fmt *fmt;
>
> vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n",
> f->index);
> @@ -1504,17 +1471,10 @@ static int vpfe_enum_fmt(struct file *file, void *priv,
> if (!sdinfo->sd)
> return -EINVAL;
>
> - if (f->index > ARRAY_SIZE(formats))
> + if (f->index >= vpfe->num_active_fmt)
> return -EINVAL;
>
> - for (k = 0; k < ARRAY_SIZE(formats); k++) {
> - if (formats[k].index == f->index) {
> - fmt = &formats[k];
> - break;
> - }
> - }
> - if (!fmt)
> - return -EINVAL;
> + fmt = vpfe->active_fmt[f->index];
>
> f->pixelformat = fmt->fourcc;
>
> @@ -1593,7 +1553,7 @@ static int vpfe_enum_size(struct file *file, void *priv,
> vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
>
> /* check for valid format */
> - fmt = find_format_by_pix(fsize->pixel_format);
> + fmt = find_format_by_pix(vpfe, fsize->pixel_format);
> if (!fmt) {
> vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
> fsize->pixel_format);
> @@ -2281,8 +2241,10 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
> struct vpfe_device, v4l2_dev);
> struct v4l2_subdev_mbus_code_enum mbus_code;
> struct vpfe_subdev_info *sdinfo;
> + struct vpfe_fmt *fmt;
> + int ret = 0;
> bool found = false;
> - int i, j;
> + int i, j, k;
>
> vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
>
> @@ -2304,27 +2266,37 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
>
> vpfe->video_dev.tvnorms |= sdinfo->inputs[0].std;
>
> - /* setup the supported formats & indexes */
> - for (j = 0, i = 0; ; ++j) {
> - struct vpfe_fmt *fmt;
> - int ret;
> -
> + vpfe->num_active_fmt = 0;
> + for (j = 0, i = 0; (ret != -EINVAL); ++j) {
> memset(&mbus_code, 0, sizeof(mbus_code));
> mbus_code.index = j;
> mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> ret = v4l2_subdev_call(subdev, pad, enum_mbus_code,
> - NULL, &mbus_code);
> + NULL, &mbus_code);
> if (ret)
> - break;
> -
> - fmt = find_format_by_code(mbus_code.code);
> - if (!fmt)
> continue;
>
> - fmt->supported = true;
> - fmt->index = i++;
> + vpfe_dbg(3, vpfe,
> + "subdev %s: code: %04x idx: %d\n",
> + subdev->name, mbus_code.code, j);
> +
> + for (k = 0; k < ARRAY_SIZE(formats); k++) {
> + fmt = &formats[k];
> + if (mbus_code.code != fmt->code)
> + continue;
> + vpfe->active_fmt[i] = fmt;
> + vpfe_dbg(3, vpfe,
> + "matched fourcc: %4.4s code: %04x idx: %d\n",
> + (char *)&fmt->fourcc, mbus_code.code, i);
> + vpfe->num_active_fmt = ++i;
> + }
> }
>
> + if (!i) {
> + vpfe_err(vpfe, "No suitable format reported by subdev %s\n",
> + subdev->name);
> + return -EINVAL;
> + }
> return 0;
> }
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
> index 2dde09780215..79a6505c083d 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.h
> +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> @@ -215,6 +215,39 @@ struct vpfe_ccdc {
> u32 ccdc_ctx[VPFE_REG_END / sizeof(u32)];
> };
>
> +/*
> + * struct bus_format - VPFE bus format information
> + * width: Bits per pixel (when transferred over a bus)
> + * bpp: Bytes per pixel (when stored in memory)
> + */
> +struct bus_format {
> + unsigned int width;
> + unsigned int bpp;
> +};
> +
> +/*
> + * struct vpfe_fmt - VPFE media bus format information
> + * fourcc: V4L2 pixel format code
> + * code: V4L2 media bus format code
> + * l: 10 bit bus format info
> + * s: 8 bit bus format info
> + */
> +struct vpfe_fmt {
> + u32 fourcc;
> + u32 code;
> + struct bus_format l;
> + struct bus_format s;
> +};
> +
> +/*
> + * This value needs to be at least as large as the number of entry in
> + * formats[].
> + * When formats[] is modified make sure to adjust this value also.
> + * Expect compile time warnings if VPFE_MAX_ACTIVE_FMT is smaller then
> + * the number of elements in formats[].
> + */
> +#define VPFE_MAX_ACTIVE_FMT 10
The comment is outdated since this define is used as the size of the formats
array. So you can't really get into problems.
For the same reason the name is now wrong as well, I'd call it VPFE_NUM_FORMATS.
That's what it actually is.
Regards,
Hans
> +
> struct vpfe_device {
> /* V4l2 specific parameters */
> /* Identifies video device for this channel */
> @@ -252,6 +285,9 @@ struct vpfe_device {
> struct v4l2_format fmt;
> /* Used to store current bytes per pixel based on current format */
> unsigned int bpp;
> + struct vpfe_fmt *active_fmt[VPFE_MAX_ACTIVE_FMT];
> + unsigned int num_active_fmt;
> +
> /*
> * used when IMP is chained to store the crop window which
> * is different from the image window
>
next prev parent reply other threads:[~2019-09-20 8:19 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-19 20:41 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
2019-09-19 20:41 ` [Patch v2 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config Benoit Parrot
2019-09-19 20:41 ` [Patch v2 02/13] media: am437x-vpfe: Fix missing first line Benoit Parrot
2019-09-19 20:41 ` [Patch v2 03/13] media: am437x-vpfe: Rework ISR routine for clarity Benoit Parrot
2019-09-19 20:41 ` [Patch v2 04/13] media: am437x-vpfe: Wait for end of frame before tear-down Benoit Parrot
2019-09-19 20:41 ` [Patch v2 05/13] media: am437x-vpfe: fix start streaming error path Benoit Parrot
2019-09-19 20:41 ` [Patch v2 06/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup Benoit Parrot
2019-09-19 20:41 ` [Patch v2 07/13] media: am437x-vpfe: Setting STD to current value is not an error Benoit Parrot
2019-09-19 20:41 ` [Patch v2 08/13] media: am437x-vpfe: Use a per instance format array instead of a static one Benoit Parrot
2019-09-20 8:19 ` Hans Verkuil [this message]
2019-09-20 12:13 ` Benoit Parrot
2019-09-19 20:41 ` [Patch v2 09/13] media: am437x-vpfe: fix function trace debug log Benoit Parrot
2019-09-19 20:41 ` [Patch v2 10/13] media: am437x-vpfe: Remove print_fourcc helper Benoit Parrot
2019-09-20 8:21 ` Hans Verkuil
2019-09-20 12:18 ` Benoit Parrot
2019-09-19 20:41 ` [Patch v2 11/13] media: am437x-vpfe: TRY_FMT ioctl is not really trying anything Benoit Parrot
2019-09-19 20:41 ` [Patch v2 12/13] media: am437x-vpfe: Remove per bus width static data Benoit Parrot
2019-09-19 20:41 ` [Patch v2 13/13] media: am437x-vpfe: Switch to SPDX Licensing Benoit Parrot
2019-09-20 8:25 ` [Patch 00/13] media: am437x-vpfe: overdue maintenance Hans Verkuil
2019-09-20 12:20 ` Benoit Parrot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a73280f1-4fc0-2ea2-2611-9e410f3f0689@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=bparrot@ti.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=prabhakar.csengg@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).