Message ID | 20171208010842.20047-13-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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@ragnatech.se> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > 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;
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) { 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; 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;