Hi Niklas,
Thank you for the patch.
On Friday, 8 December 2017 03:08:26 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>
> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 88 +++++++++++++-------------
> drivers/media/platform/rcar-vin/rcar-vin.h | 12 ----
> 2 files changed, 42 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> d6298c684ab2d731..9cf9ff48ac1e2f4f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -90,24 +90,30 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format
> *pix) * V4L2
> */
>
> -static int rvin_reset_format(struct rvin_dev *vin)
> +static int rvin_get_sd_format(struct rvin_dev *vin, struct v4l2_pix_format
> *pix)
What does sd stand for here ? How about rvin_get_source_format() ?
As the function retrieves the format on a subdev I'd rather use a
v4l2_mbus_framefmt instead of a v4l2_pix_format, and convert in the callers if
needed.
> {
> struct v4l2_subdev_format fmt = {
> .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + .pad = vin->digital->source_pad,
> };
> - 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;
> + v4l2_fill_pix_format(pix, &fmt.format);
> +
> + return 0;
> +}
> +
> +static int rvin_reset_format(struct rvin_dev *vin)
> +{
> + int ret;
> +
> + ret = rvin_get_sd_format(vin, &vin->format);
> + if (ret)
> + return ret;
>
> /*
> * If the subdevice uses ALTERNATE field mode and G_STD is
> @@ -137,12 +143,12 @@ static int rvin_reset_format(struct rvin_dev *vin)
> }
>
> vin->crop.top = vin->crop.left = 0;
> - vin->crop.width = mf->width;
> - vin->crop.height = mf->height;
> + vin->crop.width = vin->format.width;
> + vin->crop.height = vin->format.height;
>
> vin->compose.top = vin->compose.left = 0;
> - vin->compose.width = mf->width;
> - vin->compose.height = mf->height;
> + vin->compose.width = vin->format.width;
> + vin->compose.height = vin->format.height;
>
> vin->format.bytesperline = rvin_format_bytesperline(&vin->format);
> vin->format.sizeimage = rvin_format_sizeimage(&vin->format);
> @@ -151,9 +157,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> }
>
> static int __rvin_try_format_source(struct rvin_dev *vin,
> - u32 which,
> - struct v4l2_pix_format *pix,
> - struct rvin_source_fmt *source)
> + u32 which, struct v4l2_pix_format *pix)
> {
> struct v4l2_subdev *sd;
> struct v4l2_subdev_pad_config *pad_cfg;
> @@ -186,25 +190,15 @@ static int __rvin_try_format_source(struct rvin_dev
> *vin, v4l2_fill_pix_format(pix, &format.format);
>
> pix->field = field;
> -
> - source->width = pix->width;
> - source->height = pix->height;
> -
> pix->width = width;
> pix->height = height;
> -
> - vin_dbg(vin, "Source resolution: %ux%u\n", source->width,
> - source->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)
> + u32 which, struct v4l2_pix_format *pix)
> {
> u32 walign;
> int ret;
> @@ -225,7 +219,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> pix->sizeimage = 0;
>
> /* Limit to source capabilities */
> - ret = __rvin_try_format_source(vin, which, pix, source);
> + ret = __rvin_try_format_source(vin, which, pix);
> if (ret)
> return ret;
>
> @@ -234,7 +228,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
> case V4L2_FIELD_BOTTOM:
> case V4L2_FIELD_ALTERNATE:
> pix->height /= 2;
> - source->height /= 2;
> break;
> case V4L2_FIELD_NONE:
> case V4L2_FIELD_INTERLACED_TB:
> @@ -286,30 +279,23 @@ 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;
>
> - 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);
> }
>
> 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;
> 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);
> if (ret)
> return ret;
>
> - vin->source.width = source.width;
> - vin->source.height = source.height;
> -
> vin->format = f->fmt.pix;
>
> return 0;
> @@ -340,6 +326,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;
> @@ -347,9 +335,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_sd_format(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;
> @@ -375,12 +366,14 @@ static int rvin_s_selection(struct file *file, void
> *fh, {
> struct rvin_dev *vin = video_drvdata(file);
> const struct rvin_video_format *fmt;
> + struct v4l2_pix_format pix;
> struct v4l2_rect r = s->r;
> struct v4l2_rect max_rect;
> struct v4l2_rect min_rect = {
> .width = 6,
> .height = 2,
> };
> + int ret;
>
> if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> return -EINVAL;
> @@ -390,22 +383,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_sd_format(vin, &pix);
> + if (ret)
> + return ret;
I think you can move this code before the switch.
> 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
> 67541b483ee43c52..f8e0e7cedeaa6c38 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
> @@ -125,7 +115,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
> @@ -152,7 +141,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
