Hi Kieran,
Thanks for the review!
I wonder what's the status of this. Will this patch get merged?
Best,
Yunke
On Tue, Apr 19, 2022 at 9:16 PM Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Yunke,
>
> This is a very interesting development!
>
> Quoting Yunke Cao (2022-04-15 03:38:54)
> > Add a basic version of vimc lens.
> > Link lens with sensors using ancillary links.
> >
> > Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx>
> > ---
> > drivers/media/test-drivers/vimc/Makefile | 2 +-
> > drivers/media/test-drivers/vimc/vimc-common.h | 1 +
> > drivers/media/test-drivers/vimc/vimc-core.c | 86 +++++++++++----
> > drivers/media/test-drivers/vimc/vimc-lens.c | 102 ++++++++++++++++++
> > 4 files changed, 170 insertions(+), 21 deletions(-)
> > create mode 100644 drivers/media/test-drivers/vimc/vimc-lens.c
> >
> > diff --git a/drivers/media/test-drivers/vimc/Makefile b/drivers/media/test-drivers/vimc/Makefile
> > index a53b2b532e9f..9b9631562473 100644
> > --- a/drivers/media/test-drivers/vimc/Makefile
> > +++ b/drivers/media/test-drivers/vimc/Makefile
> > @@ -1,6 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0
> > vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
> > - vimc-debayer.o vimc-scaler.o vimc-sensor.o
> > + vimc-debayer.o vimc-scaler.o vimc-sensor.o vimc-lens.o
> >
> > obj-$(CONFIG_VIDEO_VIMC) += vimc.o
> >
> > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> > index ba1930772589..37f6b687ce10 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-common.h
> > +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> > @@ -171,6 +171,7 @@ extern struct vimc_ent_type vimc_sen_type;
> > extern struct vimc_ent_type vimc_deb_type;
> > extern struct vimc_ent_type vimc_sca_type;
> > extern struct vimc_ent_type vimc_cap_type;
> > +extern struct vimc_ent_type vimc_len_type;
>
> Aha, I see 'len' is short for 'lens'... I think that confused me below.
>
> I wonder if these should be longer. 'len' makes me think of 'length' too
> much, and 'lens' is only one char longer. But I guess this is
> established in this driver already so it would need a patch to change
> vimc_{deb,sca,cap}_type before calling it lens.
>
>
> > /**
> > * vimc_pix_map_by_index - get vimc_pix_map struct by its index
> > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> > index 06edf9d4d92c..166323406c6b 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-core.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> > @@ -24,7 +24,7 @@ MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> >
> > #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> >
> > -#define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> > +#define VIMC_DATA_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> > .src_ent = src, \
> > .src_pad = srcpad, \
> > .sink_ent = sink, \
> > @@ -32,8 +32,13 @@ MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> > .flags = link_flags, \
> > }
> >
> > -/* Structure which describes links between entities */
> > -struct vimc_ent_link {
> > +#define VIMC_ANCILLARY_LINK(primary, ancillary) { \
> > + .primary_ent = primary, \
> > + .ancillary_ent = ancillary \
> > +}
> > +
> > +/* Structure which describes data links between entities */
> > +struct vimc_data_link {
> > unsigned int src_ent;
> > u16 src_pad;
> > unsigned int sink_ent;
> > @@ -41,12 +46,20 @@ struct vimc_ent_link {
> > u32 flags;
> > };
> >
> > +/* Structure which describes ancillary links between entities */
> > +struct vimc_ancillary_link {
> > + unsigned int primary_ent;
> > + unsigned int ancillary_ent;
> > +};
> > +
> > /* Structure which describes the whole topology */
> > struct vimc_pipeline_config {
> > const struct vimc_ent_config *ents;
> > size_t num_ents;
> > - const struct vimc_ent_link *links;
> > - size_t num_links;
> > + const struct vimc_data_link *data_links;
> > + size_t num_data_links;
> > + const struct vimc_ancillary_link *ancillary_links;
> > + size_t num_ancillary_links;
> > };
> >
> > /* --------------------------------------------------------------------------
> > @@ -91,32 +104,49 @@ static struct vimc_ent_config ent_config[] = {
> > .name = "RGB/YUV Capture",
> > .type = &vimc_cap_type
> > },
> > + {
> > + .name = "Lens A",
> > + .type = &vimc_len_type
> > + },
> > + {
> > + .name = "Lens B",
> > + .type = &vimc_len_type
> > + },
> > };
> >
> > -static const struct vimc_ent_link ent_links[] = {
> > +static const struct vimc_data_link data_links[] = {
> > /* Link: Sensor A (Pad 0)->(Pad 0) Debayer A */
> > - VIMC_ENT_LINK(0, 0, 2, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > + VIMC_DATA_LINK(0, 0, 2, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > /* Link: Sensor A (Pad 0)->(Pad 0) Raw Capture 0 */
> > - VIMC_ENT_LINK(0, 0, 4, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > + VIMC_DATA_LINK(0, 0, 4, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > /* Link: Sensor B (Pad 0)->(Pad 0) Debayer B */
> > - VIMC_ENT_LINK(1, 0, 3, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > + VIMC_DATA_LINK(1, 0, 3, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > /* Link: Sensor B (Pad 0)->(Pad 0) Raw Capture 1 */
> > - VIMC_ENT_LINK(1, 0, 5, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > + VIMC_DATA_LINK(1, 0, 5, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > /* Link: Debayer A (Pad 1)->(Pad 0) Scaler */
> > - VIMC_ENT_LINK(2, 1, 7, 0, MEDIA_LNK_FL_ENABLED),
> > + VIMC_DATA_LINK(2, 1, 7, 0, MEDIA_LNK_FL_ENABLED),
> > /* Link: Debayer B (Pad 1)->(Pad 0) Scaler */
> > - VIMC_ENT_LINK(3, 1, 7, 0, 0),
> > + VIMC_DATA_LINK(3, 1, 7, 0, 0),
> > /* Link: RGB/YUV Input (Pad 0)->(Pad 0) Scaler */
> > - VIMC_ENT_LINK(6, 0, 7, 0, 0),
> > + VIMC_DATA_LINK(6, 0, 7, 0, 0),
> > /* Link: Scaler (Pad 1)->(Pad 0) RGB/YUV Capture */
> > - VIMC_ENT_LINK(7, 1, 8, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > + VIMC_DATA_LINK(7, 1, 8, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
> > +};
> > +
> > +static const struct vimc_ancillary_link ancillary_links[] = {
> > + /* Link: Sensor A -> Lens A */
> > + VIMC_ANCILLARY_LINK(0, 9),
> > + /* Link: Sensor B -> Lens B */
> > + VIMC_ANCILLARY_LINK(1, 10),
> > };
>
> There's a lot of magic indexes here (not a fault of your patch) - I
> would probably restructure this to have the indexes named in an enum.
>
> But - I don't think that is required for your patch - just something I
> think should be done for VIMC to make this clearer and less likely to
> get an incorrect index one day.
>
>
> > static struct vimc_pipeline_config pipe_cfg = {
> > - .ents = ent_config,
> > - .num_ents = ARRAY_SIZE(ent_config),
> > - .links = ent_links,
> > - .num_links = ARRAY_SIZE(ent_links)
> > + .ents = ent_config,
> > + .num_ents = ARRAY_SIZE(ent_config),
> > + .data_links = data_links,
> > + .num_data_links = ARRAY_SIZE(data_links),
> > + .ancillary_links = ancillary_links,
> > + .num_ancillary_links = ARRAY_SIZE(ancillary_links),
> > };
> >
> > /* -------------------------------------------------------------------------- */
> > @@ -135,8 +165,8 @@ static int vimc_create_links(struct vimc_device *vimc)
> > int ret;
> >
> > /* Initialize the links between entities */
> > - for (i = 0; i < vimc->pipe_cfg->num_links; i++) {
> > - const struct vimc_ent_link *link = &vimc->pipe_cfg->links[i];
> > + for (i = 0; i < vimc->pipe_cfg->num_data_links; i++) {
> > + const struct vimc_data_link *link = &vimc->pipe_cfg->data_links[i];
> >
> > struct vimc_ent_device *ved_src =
> > vimc->ent_devs[link->src_ent];
> > @@ -150,6 +180,22 @@ static int vimc_create_links(struct vimc_device *vimc)
> > goto err_rm_links;
> > }
> >
> > + for (i = 0; i < vimc->pipe_cfg->num_ancillary_links; i++) {
> > + const struct vimc_ancillary_link *link = &vimc->pipe_cfg->ancillary_links[i];
> > +
> > + struct vimc_ent_device *ved_primary =
> > + vimc->ent_devs[link->primary_ent];
> > + struct vimc_ent_device *ved_ancillary =
> > + vimc->ent_devs[link->ancillary_ent];
> > + struct media_link *ret_link =
> > + media_create_ancillary_link(ved_primary->ent, ved_ancillary->ent);
> > +
> > + if (IS_ERR(ret_link)) {
> > + ret = PTR_ERR(link);
> > + goto err_rm_links;
> > + }
> > + }
> > +
> > return 0;
> >
> > err_rm_links:
> > diff --git a/drivers/media/test-drivers/vimc/vimc-lens.c b/drivers/media/test-drivers/vimc/vimc-lens.c
> > new file mode 100644
> > index 000000000000..dfe824d3addb
> > --- /dev/null
> > +++ b/drivers/media/test-drivers/vimc/vimc-lens.c
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * vimc-lens.c Virtual Media Controller Driver
> > + * Copyright (C) 2022 Google, Inc
> > + * Author: yunkec@xxxxxxxxxx (Yunke Cao)
> > + */
> > +
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include "vimc-common.h"
> > +
> > +#define VIMC_LEN_MAX_FOCUS_POS 1023
> > +#define VIMC_LEN_MAX_FOCUS_STEP 1
> > +
> > +struct vimc_len_device {
> > + struct vimc_ent_device ved;
> > + struct v4l2_subdev sd;
> > + struct v4l2_ctrl_handler hdl;
> > + u32 focus_absolute;
>
> I'm not 100% certain we need to actually store this, as I think having
> the control itself is enough - but I think it's good to keep this here.
>
> I wonder if we might have some filter on the value sometime for
> retrieval so we can emulate the physical movement delays ... but I'm
> getting ahead of myself there, and this is fine as is.
>
> I can't actually see any particular issues with this as it stands, and
> my comments so far are really only about separate developments or
> patches on top of this - so I am already tempted to offer:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
>
> > +};
> > +
> > +static const struct v4l2_subdev_core_ops vimc_len_core_ops = {
> > + .log_status = v4l2_ctrl_subdev_log_status,
>
> Oh nice so that already logs the current control settings.
>
> > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > +};
> > +
> > +static const struct v4l2_subdev_ops vimc_len_ops = {
> > + .core = &vimc_len_core_ops
> > +};
> > +
> > +static int vimc_len_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct vimc_len_device *vlen =
> > + container_of(ctrl->handler, struct vimc_len_device, hdl);
> > + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) {
> > + vlen->focus_absolute = ctrl->val;
> > + return 0;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops vimc_len_ctrl_ops = {
> > + .s_ctrl = vimc_len_s_ctrl,
> > +};
> > +
> > +static struct vimc_ent_device *vimc_len_add(struct vimc_device *vimc,
> > + const char *vcfg_name)
> > +{
> > + struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
> > + struct vimc_len_device *vlen;
> > + int ret;
> > +
> > + /* Allocate the vlen struct */
> > + vlen = kzalloc(sizeof(*vlen), GFP_KERNEL);
> > + if (!vlen)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + v4l2_ctrl_handler_init(&vlen->hdl, 1);
> > +
> > + v4l2_ctrl_new_std(&vlen->hdl, &vimc_len_ctrl_ops,
> > + V4L2_CID_FOCUS_ABSOLUTE, 0,
> > + VIMC_LEN_MAX_FOCUS_POS, VIMC_LEN_MAX_FOCUS_STEP, 0);
> > + vlen->sd.ctrl_handler = &vlen->hdl;
> > + if (vlen->hdl.error) {
> > + ret = vlen->hdl.error;
> > + goto err_free_vlen;
> > + }
> > + vlen->ved.dev = vimc->mdev.dev;
> > +
> > + ret = vimc_ent_sd_register(&vlen->ved, &vlen->sd, v4l2_dev,
> > + vcfg_name, MEDIA_ENT_F_LENS, 0,
> > + NULL, &vimc_len_ops);
> > + if (ret)
> > + goto err_free_hdl;
> > +
> > + return &vlen->ved;
> > +
> > +err_free_hdl:
> > + v4l2_ctrl_handler_free(&vlen->hdl);
> > +err_free_vlen:
> > + kfree(vlen);
> > +
> > + return ERR_PTR(ret);
> > +}
> > +
> > +static void vimc_len_release(struct vimc_ent_device *ved)
> > +{
> > + struct vimc_len_device *vlen =
> > + container_of(ved, struct vimc_len_device, ved);
> > +
> > + v4l2_ctrl_handler_free(&vlen->hdl);
> > + media_entity_cleanup(vlen->ved.ent);
> > + kfree(vlen);
> > +}
> > +
> > +struct vimc_ent_type vimc_len_type = {
> > + .add = vimc_len_add,
> > + .release = vimc_len_release
> > +};
> > --
> > 2.36.0.rc0.470.gd361397f0d-goog
> >
