Hi Guennadi,
On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> On a second thought:
>
> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> > Hi Wills,
> >
> > Thanks for the patch. First and foremost, the title of the patch is wrong.
> > This patch does more than just adding some "adv7604 compatibility." It's
> > adding pad-level API to soc-camera.
> >
> > This is just a rough review. I'm not an expert in media-controller /
> > pad-level API, I hope someone with a better knowledge of those areas will
> > help me reviewing this.
> >
> > Another general comment: it has been discussed since a long time, whether
> > a wrapper wouldn't be desired to enable a seamless use of both subdev
> > drivers using and not using the pad-level API. Maybe it's the right time
> > now?..
>
> This would be a considerable change and would most probably take a rather
> long time, given how busy everyone is.
If I understood correctly Hans Verkuil told me over the weekend that he wanted
to address this problem in the near future. Hans, could you detail your plans
?
> I personally would be fine with a (yet another) intermittent solution,
> whereby we create a soc_camera_subdev.c file, in which we collect all those
> function to call either a video or a pad subdev operation, depending on
> whether the latter is available. E.g.
>
> int soc_camera_sd_enum_mbus_fmt(sd, code)
> {
> int ret;
>
> if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, code);
> } else {
> u32 pixcode;
>
> ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, code->index,
&pixcode);
> if (!ret)
> code->code= pixcode;
> }
>
> return ret;
> }
>
> Similarly for other ops.
>
> Thanks
> Guennadi
>
> > On Thu, 29 Jan 2015, William Towle wrote:
> > > Add 'struct media_pad pad' member and suitable glue code, so that
> > > soc_camera/rcar_vin can become agnostic to whether an old or new-
> > > style driver (wrt pad API use) can sit underneath
> > >
> > > This version has been reworked to include appropriate constant and
> > > datatype names for kernel v3.18
> > > ---
> > >
> > > drivers/media/platform/soc_camera/soc_camera.c | 148
> > > +++++++++++++++++++-
> > > drivers/media/platform/soc_camera/soc_scale_crop.c | 43 +++++-
> > > include/media/soc_camera.h | 1 +
> > > 3 files changed, 182 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > > b/drivers/media/platform/soc_camera/soc_camera.c index f4be2a1..efc20bf
> > > 100644
> > > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > > @@ -37,8 +37,11 @@
> > >
> > > #include <media/v4l2-ioctl.h>
> > > #include <media/v4l2-dev.h>
> > > #include <media/v4l2-of.h>
> > >
> > > +#if 0
> > >
> > > #include <media/videobuf-core.h>
> > > #include <media/videobuf2-core.h>
> > >
> > > +#endif
> >
> > No. These headers are needed even if the code can be compiled without
> > them.
> >
> > > +#include <media/v4l2-mediabus.h>
> >
> > Well, maybe. This header is included indirectly via soc_mediabus.h, but
> > yes, as I just said above, headers, whose defines, structs etc. are used,
> > should be encluded directly. Further, you'll need more headers, e.g.
> > media-entity.h, maybe some more.
> >
> > > /* Default to VGA resolution */
> > > #define DEFAULT_WIDTH 640
> > >
> > > @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file,
> > > void *priv,> >
> > > return vb2_expbuf(&icd->vb2_vidq, p);
> > >
> > > }
> > >
> > > +static int soc_camera_init_user_formats_pad(struct soc_camera_device
> > > *icd, int src_pad_idx) +{
> > > + struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> > > + struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > > + struct v4l2_subdev_mbus_code_enum code;
> > > + int fmts= 0, raw_fmts, i, ret;
> >
> > Please, run this patch through checkpatch.pl. It will tell you to add a
> > Signed-off-by line, (hopefully) to add spaces before "=" in multiple
> > places, to place braces correctly, to not use C++-style comments etc. Only
> > feel free to ignore 80-character warnings.
> >
> > > +
> > > + code.pad= src_pad_idx;
> > > + code.index= 0;
> > > +
> > > + // subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > + if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> >
> > This function is called only once below and only after the above test has
> > already returned success. Looks like you don't need it here again and the
> > below "else" branch can be dropped completely?
> >
> > > + while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> > > + code.index++;
> > > + } else {
> > > + u32 pixcode;
> > > +
> > > + while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index,
> > > &pixcode)) + {
> > > + code.code= pixcode;
> > > + code.index++;
> > > + }
> > > + }
> > > + raw_fmts= code.index;
> > > +
> > > + if (!ici->ops->get_formats) {
> > > + /*
> > > + * Fallback mode - the host will have to serve all
> > > + * sensor-provided formats one-to-one to the user
> > > + */
> > > + fmts = raw_fmts;
> > > + }
> > > + else {
> > > + /*
> > > + * First pass - only count formats this host-sensor
> > > + * configuration can provide
> > > + */
> > > + for (i = 0; i < raw_fmts; i++) {
> > > + int ret = ici->ops->get_formats(icd, i, NULL);
> > > + if (ret < 0)
> > > + return ret;
> > > + fmts += ret;
> > > + }
> > > + }
> > > +
> > > + if (!fmts)
> > > + return -ENXIO;
> > > +
> > > + icd->user_formats =
> > > + vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> > > + if (!icd->user_formats)
> > > + return -ENOMEM;
> > > +
> > > + dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> > > +
> > > + /* Second pass - actually fill data formats */
> > > + fmts = 0;
> > > + for (i = 0; i < raw_fmts; i++) {
> > > + if (!ici->ops->get_formats) {
> > > + code.index= i;
> > > + // subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > + if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> >
> > Same test again?? Or am I missing something? If indeed these tests are
> > redundant, after you remove them this function will become very similar to
> > the original soc_camera_init_user_formats(), so, maybe some code reuse
> > will become possible.
> >
> > > + v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> > > + } else {
> > > + u32 pixcode;
> > > +
> > > + v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index,
&pixcode);
> > > + code.code= pixcode;
> > > + }
> > > + icd->user_formats[fmts].host_fmt =
> > > + soc_mbus_get_fmtdesc(code.code);
> > > + if (icd->user_formats[fmts].host_fmt)
> > > + icd->user_formats[fmts++].code = code.code;
> > > + } else {
> > > + ret = ici->ops->get_formats(icd, i,
> > > + &icd->user_formats[fmts]);
> > > + if (ret < 0)
> > > + goto egfmt;
> > > + fmts += ret;
> > > + }
> > > + }
> > > +
> > > + icd->num_user_formats = fmts;
> > > + icd->current_fmt = &icd->user_formats[0];
> > > +
> > > + return 0;
> > > +
> > > +egfmt:
> > > + vfree(icd->user_formats);
> > > + return ret;
> > > +}
> > > +
> > >
> > > /* Always entered with .host_lock held */
> > > static int soc_camera_init_user_formats(struct soc_camera_device *icd)
> > > {
> > >
> > > @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> >
> > > {
> > >
> > > struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > > struct v4l2_mbus_framefmt mf;
> > >
> > > + int src_pad_idx= -1;
> > >
> > > int ret;
> > >
> > > sd->grp_id = soc_camera_grp_id(icd);
> > >
> > > @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> >
> > > }
> > >
> > > /* At this point client .probe() should have run already */
> > >
> > > - ret = soc_camera_init_user_formats(icd);
> > > + // subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > + if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))
> >
> > This is the test, that I meant above.
> >
> > > + ret = soc_camera_init_user_formats(icd);
> > > + else {
> > > + ret = media_entity_init(&icd->vdev->entity, 1,
> > > + &icd->pad, 0);
> >
> > Ok, maybe this hard-coded 1 pad with no extras is justified here, but
> > let's here what others say.
> >
> > > + if (!ret) {
> > > + for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads;
> > > src_pad_idx++)
> > > + if (sd->entity.pads[src_pad_idx].flags ==
MEDIA_PAD_FL_SOURCE)
> > > + break;
> > > +
> > > + if (src_pad_idx < sd->entity.num_pads) {
> > > + ret = media_entity_create_link(
> > > + &icd->vdev->entity, 0,
> > > + &sd->entity, src_pad_idx,
> > > + MEDIA_LNK_FL_IMMUTABLE |
> > > + MEDIA_LNK_FL_ENABLED);
> >
> > Let's try to preserve the style. I normally try to avoid splitting the
> > line after "f(" and adding at least the first function parameter above
> > will not make that line longer, than the ones above. So, let's do that.
> >
> > > + }
> > > + }
> > > +
> > > + if (!ret)
> > > + ret = soc_camera_init_user_formats_pad(icd,
> > > + src_pad_idx);
> >
> > Probably no need to break the line here either.
> >
> > > + }
> > >
> > > if (ret < 0)
> > >
> > > goto eusrfmt;
> > >
> > > @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> >
> > > goto evidstart;
> > >
> > > /* Try to improve our guess of a reasonable window format */
> > >
> > > - if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > > - icd->user_width = mf.width;
> > > - icd->user_height = mf.height;
> > > - icd->colorspace = mf.colorspace;
> > > - icd->field = mf.field;
> > > + // subdev_has_op -> get_fmt vs g_mbus_fmt
> > > + if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> > > + && v4l2_subdev_has_op(sd, pad, get_fmt)
> > > + && src_pad_idx != -1) {
> >
> > The rest of the file puts operations after the first argument, not before
> > the second one when breaking the line. Let's do that here too.
> >
> > Thanks
> > Guennadi
> >
> > > + struct v4l2_subdev_format sd_format;
> > > +
> > > + sd_format.pad= src_pad_idx;
> > > + sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +
> > > + if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> > > + icd->user_width = sd_format.format.width;
> > > + icd->user_height = sd_format.format.height;
> > > + icd->colorspace = sd_format.format.colorspace;
> > > + icd->field = sd_format.format.field;
> > > + }
> > > + } else {
> > > + if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > > + icd->user_width = mf.width;
> > > + icd->user_height = mf.height;
> > > + icd->colorspace = mf.colorspace;
> > > + icd->field = mf.field;
> > > + }
> > >
> > > }
> > > soc_camera_remove_device(icd);
> > >
> > > diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > b/drivers/media/platform/soc_camera/soc_scale_crop.c index
> > > 8e74fb7..8a1ca05 100644
> > > --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device
> > > *icd,
> > >
> > > bool host_1to1;
> > > int ret;
> > >
> > > - ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > - soc_camera_grp_id(icd), video,
> > > - s_mbus_fmt, mf);
> > > + // subdev_has_op -> set_fmt vs s_mbus_fmt
> > > + if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > > + struct v4l2_subdev_format sd_format;
> > > + struct media_pad *remote_pad;
> > > +
> > > + remote_pad= media_entity_remote_pad(
> > > + &icd->vdev->entity.pads[0]);
> > > + sd_format.pad = remote_pad->index;
> > > + sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > + sd_format.format= *mf;
> > > +
> > > + ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > + soc_camera_grp_id(icd), pad, set_fmt, NULL,
> > > + &sd_format);
> > > +
> > > + mf->width = sd_format.format.width;
> > > + mf->height = sd_format.format.height;
> > > + } else {
> > > + ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > + soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> > > + }
> > >
> > > if (ret < 0)
> > >
> > > return ret;
> > >
> > > @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device
> > > *icd,
> > >
> > > tmp_h = min(2 * tmp_h, max_height);
> > > mf->width = tmp_w;
> > > mf->height = tmp_h;
> > >
> > > - ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > + // subdev_has_op -> set_fmt vs s_mbus_fmt
> > > + if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > > + struct v4l2_subdev_format sd_format;
> > > + struct media_pad *remote_pad;
> > > +
> > > + remote_pad= media_entity_remote_pad(
> > > + &icd->vdev->entity.pads[0]);
> > > + sd_format.pad = remote_pad->index;
> > > + sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > + sd_format.format= *mf;
> > > +
> > > + ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > + soc_camera_grp_id(icd),
> > > + pad, set_fmt, NULL,
> > > + &sd_format);
> > > + } else {
> > > + ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > >
> > > soc_camera_grp_id(icd), video,
> > > s_mbus_fmt, mf);
> > >
> > > + }
> > >
> > > dev_geo(dev, "Camera scaled to %ux%u\n",
> > >
> > > mf->width, mf->height);
> > >
> > > if (ret < 0) {
> > >
> > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > > index 2f6261f..f0c5238 100644
> > > --- a/include/media/soc_camera.h
> > > +++ b/include/media/soc_camera.h
> > > @@ -42,6 +42,7 @@ struct soc_camera_device {
> > >
> > > unsigned char devnum; /* Device number per host */
> > > struct soc_camera_sense *sense; /* See comment in struct definition
*/
> > > struct video_device *vdev;
> > >
> > > + struct media_pad pad;
> > >
> > > struct v4l2_ctrl_handler ctrl_handler;
> > > const struct soc_camera_format_xlate *current_fmt;
> > > struct soc_camera_format_xlate *user_formats;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
