Hi Laurent,
Thanks for your feedback.
On 2018-03-02 13:06:27 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Friday, 2 March 2018 03:57:35 EET Niklas Söderlund wrote:
> > Instead of caching the subdevice format each time the video device
> > format is set read it directly when it's needed. As it turns out the
> > format is only needed when figuring out the max rectangle for cropping.
> >
> > This simplifies the code and makes it clearer what the source format is
> > used for.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 158 +++++++++++++------------
> > drivers/media/platform/rcar-vin/rcar-vin.h | 12 ---
> > 2 files changed, 80 insertions(+), 90 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > 3290e603b44cdf3a..55640c6b2a1200ca 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -144,67 +144,62 @@ static int rvin_format_align(struct rvin_dev *vin,
> > struct v4l2_pix_format *pix) * V4L2
> > */
> >
> > -static void rvin_reset_crop_compose(struct rvin_dev *vin)
> > +static int rvin_get_vin_format_from_source(struct rvin_dev *vin,
> > + struct v4l2_pix_format *pix)
> > {
> > + struct v4l2_subdev_format fmt = {
> > + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > + .pad = vin->digital->source_pad,
> > + };
> > + int ret;
> > +
> > + ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
> > + if (ret)
> > + return ret;
> > +
> > + v4l2_fill_pix_format(pix, &fmt.format);
> > +
> > + return rvin_format_align(vin, pix);
> > +}
> > +
> > +static int rvin_reset_format(struct rvin_dev *vin)
> > +{
> > + int ret;
> > +
> > + ret = rvin_get_vin_format_from_source(vin, &vin->format);
> > + if (ret)
> > + return ret;
> > +
> > vin->crop.top = vin->crop.left = 0;
> > - vin->crop.width = vin->source.width;
> > - vin->crop.height = vin->source.height;
> > + vin->crop.width = vin->format.width;
> > + vin->crop.height = vin->format.height;
> >
> > vin->compose.top = vin->compose.left = 0;
> > vin->compose.width = vin->format.width;
> > vin->compose.height = vin->format.height;
> > -}
> > -
> > -static int rvin_reset_format(struct rvin_dev *vin)
> > -{
> > - struct v4l2_subdev_format fmt = {
> > - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > - };
> > - struct v4l2_mbus_framefmt *mf = &fmt.format;
> > - int ret;
> > -
> > - fmt.pad = vin->digital->source_pad;
> > -
> > - ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
> > - if (ret)
> > - return ret;
> > -
> > - vin->format.width = mf->width;
> > - vin->format.height = mf->height;
> > - vin->format.colorspace = mf->colorspace;
> > - vin->format.field = mf->field;
> > -
> > - rvin_reset_crop_compose(vin);
> > -
> > - vin->format.bytesperline = rvin_format_bytesperline(&vin->format);
> > - vin->format.sizeimage = rvin_format_sizeimage(&vin->format);
> >
> > return 0;
> > }
> >
> > -static int __rvin_try_format_source(struct rvin_dev *vin,
> > - u32 which,
> > - struct v4l2_pix_format *pix,
> > - struct rvin_source_fmt *source)
> > +static int rvin_try_format(struct rvin_dev *vin, u32 which,
> > + struct v4l2_pix_format *pix,
> > + struct v4l2_rect *crop, struct v4l2_rect *compose)
> > {
> > - struct v4l2_subdev *sd;
> > + struct v4l2_subdev *sd = vin_to_source(vin);
> > struct v4l2_subdev_pad_config *pad_cfg;
> > struct v4l2_subdev_format format = {
> > .which = which,
> > + .pad = vin->digital->source_pad,
> > };
> > enum v4l2_field field;
> > u32 width, height;
> > int ret;
> >
> > - sd = vin_to_source(vin);
> > -
> > - v4l2_fill_mbus_format(&format.format, pix, vin->digital->code);
> > -
> > pad_cfg = v4l2_subdev_alloc_pad_config(sd);
> > if (pad_cfg == NULL)
> > return -ENOMEM;
> >
> > - format.pad = vin->digital->source_pad;
> > + v4l2_fill_mbus_format(&format.format, pix, vin->digital->code);
> >
> > /* Allow the video device to override field and to scale */
> > field = pix->field;
> > @@ -217,34 +212,34 @@ static int __rvin_try_format_source(struct rvin_dev
> > *vin,
> >
> > v4l2_fill_pix_format(pix, &format.format);
> >
> > - source->width = pix->width;
> > - source->height = pix->height;
> > + crop->top = crop->left = 0;
> > + crop->width = pix->width;
> > + crop->height = pix->height;
> > +
> > + /*
> > + * If source is ALTERNATE the driver will use the VIN hardware
> > + * to INTERLACE it. The crop height then needs to be doubled.
> > + */
> > + if (pix->field == V4L2_FIELD_ALTERNATE)
> > + crop->height *= 2;
> > +
> > + if (field != V4L2_FIELD_ANY)
> > + pix->field = field;
> >
> > - pix->field = field;
> > pix->width = width;
> > pix->height = height;
> >
> > - vin_dbg(vin, "Source resolution: %ux%u\n", source->width,
> > - source->height);
> > + ret = rvin_format_align(vin, pix);
> > + if (ret)
> > + return ret;
> >
> > + compose->top = compose->left = 0;
> > + compose->width = pix->width;
> > + compose->height = pix->height;
> > done:
> > v4l2_subdev_free_pad_config(pad_cfg);
> > - return ret;
> > -}
> >
> > -static int __rvin_try_format(struct rvin_dev *vin,
> > - u32 which,
> > - struct v4l2_pix_format *pix,
> > - struct rvin_source_fmt *source)
> > -{
> > - int ret;
> > -
> > - /* Limit to source capabilities */
> > - ret = __rvin_try_format_source(vin, which, pix, source);
> > - if (ret)
> > - return ret;
> > -
> > - return rvin_format_align(vin, pix);
> > + return 0;
> > }
> >
> > static int rvin_querycap(struct file *file, void *priv,
> > @@ -263,33 +258,30 @@ static int rvin_try_fmt_vid_cap(struct file *file,
> > void *priv, struct v4l2_format *f)
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> > - struct rvin_source_fmt source;
> > + struct v4l2_rect crop, compose;
> >
> > - return __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix,
> > - &source);
> > + return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix, &crop,
> > + &compose);
> > }
> >
> > static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> > struct v4l2_format *f)
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> > - struct rvin_source_fmt source;
> > + struct v4l2_rect crop, compose;
> > int ret;
> >
> > if (vb2_is_busy(&vin->queue))
> > return -EBUSY;
> >
> > - ret = __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_ACTIVE, &f->fmt.pix,
> > - &source);
> > + ret = rvin_try_format(vin, V4L2_SUBDEV_FORMAT_ACTIVE, &f->fmt.pix,
> > + &crop, &compose);
> > if (ret)
> > return ret;
> >
> > - vin->source.width = source.width;
> > - vin->source.height = source.height;
> > -
> > vin->format = f->fmt.pix;
> > -
> > - rvin_reset_crop_compose(vin);
> > + vin->crop = crop;
> > + vin->compose = compose;
> >
> > return 0;
> > }
> > @@ -319,6 +311,8 @@ static int rvin_g_selection(struct file *file, void *fh,
> > struct v4l2_selection *s)
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> > + struct v4l2_pix_format pix;
> > + int ret;
> >
> > if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > return -EINVAL;
> > @@ -326,9 +320,12 @@ static int rvin_g_selection(struct file *file, void
> > *fh, switch (s->target) {
> > case V4L2_SEL_TGT_CROP_BOUNDS:
> > case V4L2_SEL_TGT_CROP_DEFAULT:
> > + ret = rvin_get_vin_format_from_source(vin, &pix);
> > + if (ret)
> > + return ret;
> > s->r.left = s->r.top = 0;
> > - s->r.width = vin->source.width;
> > - s->r.height = vin->source.height;
> > + s->r.width = pix.width;
> > + s->r.height = pix.height;
> > break;
> > case V4L2_SEL_TGT_CROP:
> > s->r = vin->crop;
> > @@ -353,6 +350,7 @@ static int rvin_s_selection(struct file *file, void *fh,
> > struct v4l2_selection *s)
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> > + struct v4l2_pix_format pix;
> > const struct rvin_video_format *fmt;
> > struct v4l2_rect r = s->r;
> > struct v4l2_rect max_rect;
> > @@ -360,6 +358,7 @@ static int rvin_s_selection(struct file *file, void *fh,
> > .width = 6,
> > .height = 2,
> > };
> > + int ret;
> >
> > if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > return -EINVAL;
> > @@ -369,22 +368,25 @@ static int rvin_s_selection(struct file *file, void
> > *fh, switch (s->target) {
> > case V4L2_SEL_TGT_CROP:
> > /* Can't crop outside of source input */
> > + ret = rvin_get_vin_format_from_source(vin, &pix);
> > + if (ret)
> > + return ret;
>
> I don't think the crop rectangle should be validated against the current
> source format, but against the current video node format. As noted in my
> review of the previous version:
>
> If you stop caching the source format you'll end up validating the crop
> rectangle against the current source format. What will happen if the format
> changed since the last VIDIOC_S_FMT call ?
>
> And thinking about it, are sources allowed to change their format autonomously
> ? They surely can detect format changes, but can they update the format
> without notifying anyone ? Looking at the adv7180 driver for instance, the
> format depends on the TV standard, but the driver doesn't change it. It only
> reports newly detected standards and relies on someone then calling the s_std
> operation. The VIN driver should then in its S_STD handler retrieve the new
> source format, and reset the crop rectangle accordingly.
>
> I believe the same applies to V4L2_SEL_TGT_CROP_BOUNDS and
> V4L2_SEL_TGT_CROP_DEFAULT.
OK, after the latest version and some sleep I now see your point, and I
agree with it. I will keep caching the source format :-) I will
repurpose this patch to keep the format cleanups but keep caching of the
source dimensions. Thanks for being persistent and making me understand
the issue.
>
> > max_rect.top = max_rect.left = 0;
> > - max_rect.width = vin->source.width;
> > - max_rect.height = vin->source.height;
> > + max_rect.width = pix.width;
> > + max_rect.height = pix.height;
> > v4l2_rect_map_inside(&r, &max_rect);
> >
> > - v4l_bound_align_image(&r.width, 2, vin->source.width, 1,
> > - &r.height, 4, vin->source.height, 2, 0);
> > + v4l_bound_align_image(&r.width, 2, pix.width, 1,
> > + &r.height, 4, pix.height, 2, 0);
> >
> > - r.top = clamp_t(s32, r.top, 0, vin->source.height - r.height);
> > - r.left = clamp_t(s32, r.left, 0, vin->source.width - r.width);
> > + r.top = clamp_t(s32, r.top, 0, pix.height - r.height);
> > + r.left = clamp_t(s32, r.left, 0, pix.width - r.width);
> >
> > vin->crop = s->r = r;
> >
> > vin_dbg(vin, "Cropped %dx%d@%d:%d of %dx%d\n",
> > r.width, r.height, r.left, r.top,
> > - vin->source.width, vin->source.height);
> > + pix.width, pix.height);
> > break;
> > case V4L2_SEL_TGT_COMPOSE:
> > /* Make sure compose rect fits inside output format */
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > 8daba9db0e927a49..39051da31650bd79 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -48,16 +48,6 @@ enum rvin_dma_state {
> > STOPPING,
> > };
> >
> > -/**
> > - * struct rvin_source_fmt - Source information
> > - * @width: Width from source
> > - * @height: Height from source
> > - */
> > -struct rvin_source_fmt {
> > - u32 width;
> > - u32 height;
> > -};
> > -
> > /**
> > * struct rvin_video_format - Data format stored in memory
> > * @fourcc: Pixelformat
> > @@ -124,7 +114,6 @@ struct rvin_info {
> > * @sequence: V4L2 buffers sequence number
> > * @state: keeps track of operation state
> > *
> > - * @source: active format from the video source
> > * @format: active V4L2 pixel format
> > *
> > * @crop: active cropping
> > @@ -151,7 +140,6 @@ struct rvin_dev {
> > unsigned int sequence;
> > enum rvin_dma_state state;
> >
> > - struct rvin_source_fmt source;
> > struct v4l2_pix_format format;
> >
> > struct v4l2_rect crop;
>
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Niklas Söderlund
