From: Sakari Ailus <sakari.ailus@iki.fi>
To: Bob Veringa <bob.veringa@topic.nl>
Cc: leonl@leopardimaging.com, mchehab@kernel.org,
linux-media@vger.kernel.org, mike.looijmans@topic.nl
Subject: Re: [PATCH 1/2] media: imx274: improve v4l2-compliance
Date: Thu, 29 Apr 2021 19:16:25 +0300 [thread overview]
Message-ID: <20210429161625.GX3@valkosipuli.retiisi.eu> (raw)
In-Reply-To: <20210424111459.20730-1-bob.veringa@topic.nl>
Hi Bob,
Thanks for the patch.
On Sat, Apr 24, 2021 at 01:14:58PM +0200, Bob Veringa wrote:
> 1) Fix incorrect return of get_fmt when fmt->which is set
> to V4L2_SUBDEV_FORMAT_TRY. get_fmt always returns the active format,
> but should return the pad configuration when V4L2_SUBDEV_FORMAT_TRY
> is requested.
>
> 2) Fix set_fmt not functioning correctly when fmt->which is set to
> V4L2_SUBDEV_FORMAT_TRY. When a trying a format the driver assumes that
> try_crop is already set. If this is not the case it causes
> unexpected behavior. Add a default value if crop value is not set.
>
> 3) Add support for enumerating media bus formats
> via VIDIOC_SUBDEV_ENUM_MBUS_CODE.
>
> 4) Add support for enumerating frame sizes
> via VIDIOC_SUBDEV_ENUM_FRAME_SIZE.
>
> 5) Fix v4l2-compliance error 'VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL'
> by implementing v4l2_subdev_core_ops struct with default functions.
>
> Signed-off-by: Bob Veringa <bob.veringa@topic.nl>
> Acked-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> drivers/media/i2c/imx274.c | 72 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index d9849d34f39f..942407ed931b 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -28,6 +28,7 @@
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-subdev.h>
> +#include <media/v4l2-event.h>
>
> /*
> * See "SHR, SVR Setting" in datasheet
> @@ -72,6 +73,7 @@
> #define IMX274_MAX_FRAME_RATE (120)
> #define IMX274_MIN_FRAME_RATE (5)
> #define IMX274_DEF_FRAME_RATE (60)
> +#define IMX274_DEFAULT_MEDIA_FORMAT MEDIA_BUS_FMT_SRGGB10_1X10
>
> /*
> * register SHR is limited to (SVR value + 1) x VMAX value - 4
> @@ -1011,7 +1013,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
> u32 flags)
> {
> struct device *dev = &imx274->client->dev;
> - const struct v4l2_rect *cur_crop;
> + struct v4l2_rect *cur_crop;
> struct v4l2_mbus_framefmt *tgt_fmt;
> unsigned int i;
> const struct imx274_mode *best_mode = &imx274_modes[0];
> @@ -1025,6 +1027,11 @@ static int __imx274_change_compose(struct stimx274 *imx274,
> tgt_fmt = &imx274->format;
> }
>
> + if (cur_crop->height == 0 || cur_crop->width == 0) {
> + cur_crop->height = IMX274_MAX_HEIGHT;
> + cur_crop->width = IMX274_MAX_WIDTH;
Shouldn't this be done in probe / init_cfg (to be added to subdev ops),
unconditionally?
The try format should also be initialised there. Both should be device
defaults, i.e. the same as those set in probe.
> + }
> +
> for (i = 0; i < ARRAY_SIZE(imx274_modes); i++) {
> u8 wratio = imx274_modes[i].wbin_ratio;
> u8 hratio = imx274_modes[i].hbin_ratio;
> @@ -1072,11 +1079,24 @@ static int imx274_get_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct stimx274 *imx274 = to_imx274(sd);
> + int ret = 0;
>
> mutex_lock(&imx274->lock);
> - fmt->format = imx274->format;
> +
> + switch (fmt->which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> + break;
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + fmt->format = imx274->format;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> mutex_unlock(&imx274->lock);
> - return 0;
> + return ret;
> }
>
> /**
> @@ -1274,6 +1294,41 @@ static int imx274_set_selection(struct v4l2_subdev *sd,
> return -EINVAL;
> }
>
> +static int imx274_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + __u32 width, height;
u32, please. __u32 is for the user space facing headers.
> +
> + if (fse->index >= ARRAY_SIZE(imx274_modes))
> + return -EINVAL;
> +
> + width = IMX274_MAX_WIDTH / imx274_modes[fse->index].wbin_ratio;
> + height = IMX274_MAX_HEIGHT / imx274_modes[fse->index].hbin_ratio;
> +
> + fse->code = IMX274_DEFAULT_MEDIA_FORMAT;
> + fse->min_width = width;
> + fse->max_width = width;
> + fse->min_height = height;
> + fse->max_height = height;
> + return 0;
> +}
> +
> +static int imx274_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index > 0)
> + return -EINVAL;
> +
> + if (code->pad != 0)
> + return -EINVAL;
The caller has checked the pad field is valid, and with a single pad in the
entity no additional check is necessary.
> +
> + code->code = IMX274_DEFAULT_MEDIA_FORMAT;
> +
> + return 0;
> +}
> +
> static int imx274_apply_trimming(struct stimx274 *imx274)
> {
> u32 h_start;
> @@ -1903,11 +1958,19 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
> return err;
> }
>
> +static const struct v4l2_subdev_core_ops imx274_core_ops = {
> + .log_status = v4l2_ctrl_subdev_log_status,
> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
> .get_fmt = imx274_get_fmt,
> .set_fmt = imx274_set_fmt,
> .get_selection = imx274_get_selection,
> .set_selection = imx274_set_selection,
> + .enum_mbus_code = imx274_enum_mbus_code,
> + .enum_frame_size = imx274_enum_frame_size,
> };
>
> static const struct v4l2_subdev_video_ops imx274_video_ops = {
> @@ -1917,6 +1980,7 @@ static const struct v4l2_subdev_video_ops imx274_video_ops = {
> };
>
> static const struct v4l2_subdev_ops imx274_subdev_ops = {
> + .core = &imx274_core_ops,
> .pad = &imx274_pad_ops,
> .video = &imx274_video_ops,
> };
> @@ -1968,7 +2032,7 @@ static int imx274_probe(struct i2c_client *client)
> imx274->format.width = imx274->crop.width / imx274->mode->wbin_ratio;
> imx274->format.height = imx274->crop.height / imx274->mode->hbin_ratio;
> imx274->format.field = V4L2_FIELD_NONE;
> - imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> + imx274->format.code = IMX274_DEFAULT_MEDIA_FORMAT;
> imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
> imx274->frame_interval.numerator = 1;
> imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
--
Kind regards,
Sakari Ailus
prev parent reply other threads:[~2021-04-29 16:16 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <b236611b-3635-40c5-988a-5c9e9fae4458.949ef384-8293-46b8-903f-40a477c056ae.e28b24bb-3436-45c2-9a1c-f2edfb40a571@emailsignatures365.codetwo.com>
[not found] ` <b236611b-3635-40c5-988a-5c9e9fae4458.0d2bd5fa-15cc-4b27-b94e-83614f9e5b38.3cf110e4-7db2-437b-a373-f2dd879058a9@emailsignatures365.codetwo.com>
2021-04-24 11:14 ` [PATCH 1/2] media: imx274: improve v4l2-compliance Bob Veringa
[not found] ` <b236611b-3635-40c5-988a-5c9e9fae4458.949ef384-8293-46b8-903f-40a477c056ae.ad56e9af-b436-4404-b6ae-4ce93968c3b3@emailsignatures365.codetwo.com>
[not found] ` <b236611b-3635-40c5-988a-5c9e9fae4458.0d2bd5fa-15cc-4b27-b94e-83614f9e5b38.8af6a0ab-fe37-4108-bf7a-19098799e037@emailsignatures365.codetwo.com>
2021-04-24 11:14 ` [PATCH 2/2] media: imx274: remove imx274_load_default Bob Veringa
2021-04-29 16:16 ` Sakari Ailus [this message]
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=20210429161625.GX3@valkosipuli.retiisi.eu \
--to=sakari.ailus@iki.fi \
--cc=bob.veringa@topic.nl \
--cc=leonl@leopardimaging.com \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=mike.looijmans@topic.nl \
/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).