| Message ID | 20180628162054.25613-7-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Thu, 28 Jun 2018 18:20:38 +0200 Marco Felsch <m.felsch@pengutronix.de> escreveu: > Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") > the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is > no way to try different selections. The patch adds a helper function to > select the correct selection memory space (sub-device file handle or > driver state) which will be set/returned. > > The TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE > and the requested selection rectangle differs from the already set one. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/media/i2c/tvp5150.c | 107 ++++++++++++++++++++++++------------ > 1 file changed, 73 insertions(+), 34 deletions(-) > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > index d150487cc2d1..29eaf8166f25 100644 > --- a/drivers/media/i2c/tvp5150.c > +++ b/drivers/media/i2c/tvp5150.c > @@ -18,6 +18,7 @@ > #include <media/v4l2-ctrls.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-mc.h> > +#include <media/v4l2-rect.h> > > #include "tvp5150_reg.h" > > @@ -846,20 +847,38 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd) > } > } > > +static struct v4l2_rect * > +__tvp5150_get_pad_crop(struct tvp5150 *decoder, > + struct v4l2_subdev_pad_config *cfg, unsigned int pad, > + enum v4l2_subdev_format_whence which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_TRY: > + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); This is not ok. It causes compilation breakage if the subdev API is not selected: drivers/media/i2c/tvp5150.c: In function ‘__tvp5150_get_pad_crop’: drivers/media/i2c/tvp5150.c:857:10: error: implicit declaration of function ‘v4l2_subdev_get_try_crop’; did you mean ‘v4l2_subdev_has_op’? [-Werror=implicit-function-declaration] return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); ^~~~~~~~~~~~~~~~~~~~~~~~ v4l2_subdev_has_op drivers/media/i2c/tvp5150.c:857:10: warning: returning ‘int’ from a function with return type ‘struct v4l2_rect *’ makes pointer from integer without a cast [-Wint-conversion] return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The logic should keep working both with and without subdev API. > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &decoder->rect; > + default: > + return NULL; > + } > +} > + > static int tvp5150_fill_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *format) > { > struct v4l2_mbus_framefmt *f; > + struct v4l2_rect *__crop; > struct tvp5150 *decoder = to_tvp5150(sd); > > if (!format || (format->pad != DEMOD_PAD_VID_OUT)) > return -EINVAL; > > f = &format->format; > + __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad, > + format->which); > > - f->width = decoder->rect.width; > - f->height = decoder->rect.height / 2; > + f->width = __crop->width; > + f->height = __crop->height / 2; > > f->code = MEDIA_BUS_FMT_UYVY8_2X8; > f->field = V4L2_FIELD_ALTERNATE; > @@ -870,17 +889,51 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd, > return 0; > } > > +unsigned int tvp5150_get_hmax(struct v4l2_subdev *sd) > +{ > + struct tvp5150 *decoder = to_tvp5150(sd); > + v4l2_std_id std; > + > + /* Calculate height based on current standard */ > + if (decoder->norm == V4L2_STD_ALL) > + std = tvp5150_read_std(sd); > + else > + std = decoder->norm; > + > + return (std & V4L2_STD_525_60) ? > + TVP5150_V_MAX_525_60 : TVP5150_V_MAX_OTHERS; > +} > + > +static inline void > +__tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect) > +{ > + struct tvp5150 *decoder = to_tvp5150(sd); > + unsigned int hmax = tvp5150_get_hmax(sd); > + > + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); > + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, > + rect.top + rect.height - hmax); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, > + rect.left >> TVP5150_CROP_SHIFT); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, > + rect.left | (1 << TVP5150_CROP_SHIFT)); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, > + (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> > + TVP5150_CROP_SHIFT); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, > + rect.left + rect.width - TVP5150_MAX_CROP_LEFT); > +} > + > static int tvp5150_set_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_selection *sel) > { > struct tvp5150 *decoder = to_tvp5150(sd); > struct v4l2_rect rect = sel->r; > - v4l2_std_id std; > - int hmax; > + struct v4l2_rect *__crop; > + unsigned int hmax; > > - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE || > - sel->target != V4L2_SEL_TGT_CROP) > + if (sel->target != V4L2_SEL_TGT_CROP) > return -EINVAL; > > dev_dbg_lvl(sd->dev, 1, debug, "%s left=%d, top=%d, width=%d, height=%d\n", > @@ -889,17 +942,7 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd, > /* tvp5150 has some special limits */ > rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT); > rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP); > - > - /* Calculate height based on current standard */ > - if (decoder->norm == V4L2_STD_ALL) > - std = tvp5150_read_std(sd); > - else > - std = decoder->norm; > - > - if (std & V4L2_STD_525_60) > - hmax = TVP5150_V_MAX_525_60; > - else > - hmax = TVP5150_V_MAX_OTHERS; > + hmax = tvp5150_get_hmax(sd); > > /* > * alignments: > @@ -912,20 +955,18 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd, > hmax - TVP5150_MAX_CROP_TOP - rect.top, > hmax - rect.top, 0, 0); > > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, > - rect.top + rect.height - hmax); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, > - rect.left >> TVP5150_CROP_SHIFT); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, > - rect.left | (1 << TVP5150_CROP_SHIFT)); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, > - (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> > - TVP5150_CROP_SHIFT); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, > - rect.left + rect.width - TVP5150_MAX_CROP_LEFT); > + __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, > + sel->which); > + > + /* > + * Update output image size if the selection (crop) rectangle size or > + * position has been modified. > + */ > + if (!v4l2_rect_equal(&rect, __crop)) > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + __tvp5150_set_selection(sd, rect); > > - decoder->rect = rect; > + *__crop = rect; > > return 0; > } > @@ -937,9 +978,6 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, > struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd); > v4l2_std_id std; > > - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE) > - return -EINVAL; > - > switch (sel->target) { > case V4L2_SEL_TGT_CROP_BOUNDS: > case V4L2_SEL_TGT_CROP_DEFAULT: > @@ -958,7 +996,8 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, > sel->r.height = TVP5150_V_MAX_OTHERS; > return 0; > case V4L2_SEL_TGT_CROP: > - sel->r = decoder->rect; > + sel->r = *__tvp5150_get_pad_crop(decoder, cfg, sel->pad, > + sel->which); > return 0; > default: > return -EINVAL; Thanks, Mauro
Hi Mauro, during my work for a v2 series, I prepared two patches which fix this in a common way. The first will fix the compiler break and the second will fix a missing dependency, since those helper functions require the media_entity which is only available if MEDIA_CONTROLLER is enabled. I attached both inline as an RFC, can give me your feedback? I removed the devicetree guys to aviod noise. On 18-07-30 21:01, Mauro Carvalho Chehab wrote: > Em Thu, 28 Jun 2018 18:20:38 +0200 > Marco Felsch <m.felsch@pengutronix.de> escreveu: > > > Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") > > the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is > > no way to try different selections. The patch adds a helper function to > > select the correct selection memory space (sub-device file handle or > > driver state) which will be set/returned. > > > > The TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE > > and the requested selection rectangle differs from the already set one. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/media/i2c/tvp5150.c | 107 ++++++++++++++++++++++++------------ > > 1 file changed, 73 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > index d150487cc2d1..29eaf8166f25 100644 > > --- a/drivers/media/i2c/tvp5150.c > > +++ b/drivers/media/i2c/tvp5150.c > > @@ -18,6 +18,7 @@ > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-fwnode.h> > > #include <media/v4l2-mc.h> > > +#include <media/v4l2-rect.h> > > > > #include "tvp5150_reg.h" > > > > @@ -846,20 +847,38 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd) > > } > > } > > > > +static struct v4l2_rect * > > +__tvp5150_get_pad_crop(struct tvp5150 *decoder, > > + struct v4l2_subdev_pad_config *cfg, unsigned int pad, > > + enum v4l2_subdev_format_whence which) > > +{ > > + switch (which) { > > + case V4L2_SUBDEV_FORMAT_TRY: > > + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > > This is not ok. It causes compilation breakage if the subdev API is not > selected: > > drivers/media/i2c/tvp5150.c: In function ‘__tvp5150_get_pad_crop’: > drivers/media/i2c/tvp5150.c:857:10: error: implicit declaration of function ‘v4l2_subdev_get_try_crop’; did you mean ‘v4l2_subdev_has_op’? [-Werror=implicit-function-declaration] > return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > ^~~~~~~~~~~~~~~~~~~~~~~~ > v4l2_subdev_has_op > drivers/media/i2c/tvp5150.c:857:10: warning: returning ‘int’ from a function with return type ‘struct v4l2_rect *’ makes pointer from integer without a cast [-Wint-conversion] > return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > The logic should keep working both with and without subdev API. > [ snip ] From d87554df5d1c6d609e44852a1cb998a6f5d99601 Mon Sep 17 00:00:00 2001 From: Marco Felsch <m.felsch@pengutronix.de> Date: Wed, 8 Aug 2018 14:27:46 +0200 Subject: [RFC] [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_* In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't available. So each driver have to add ifdefs around those helpers or add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy. Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API isn't set to avoid ifdefs. This approach is less error prone too. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- include/media/v4l2-subdev.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 9102d6ca566e..ce48f1fcf295 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -912,8 +912,6 @@ struct v4l2_subdev_fh { #define to_v4l2_subdev_fh(fh) \ container_of(fh, struct v4l2_subdev_fh, vfh) -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) - /** * v4l2_subdev_get_try_format - ancillary routine to call * &struct v4l2_subdev_pad_config->try_fmt @@ -927,9 +925,13 @@ static inline struct v4l2_mbus_framefmt struct v4l2_subdev_pad_config *cfg, unsigned int pad) { +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) if (WARN_ON(pad >= sd->entity.num_pads)) pad = 0; return &cfg[pad].try_fmt; +#else + return NULL; +#endif } /** @@ -945,9 +947,13 @@ static inline struct v4l2_rect struct v4l2_subdev_pad_config *cfg, unsigned int pad) { +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) if (WARN_ON(pad >= sd->entity.num_pads)) pad = 0; return &cfg[pad].try_crop; +#else + return NULL; +#endif } /** @@ -963,11 +969,14 @@ static inline struct v4l2_rect struct v4l2_subdev_pad_config *cfg, unsigned int pad) { +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) if (WARN_ON(pad >= sd->entity.num_pads)) pad = 0; return &cfg[pad].try_compose; -} +#else + return NULL; #endif +} extern const struct v4l2_file_operations v4l2_subdev_fops;
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c index d150487cc2d1..29eaf8166f25 100644 --- a/drivers/media/i2c/tvp5150.c +++ b/drivers/media/i2c/tvp5150.c @@ -18,6 +18,7 @@ #include <media/v4l2-ctrls.h> #include <media/v4l2-fwnode.h> #include <media/v4l2-mc.h> +#include <media/v4l2-rect.h> #include "tvp5150_reg.h" @@ -846,20 +847,38 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd) } } +static struct v4l2_rect * +__tvp5150_get_pad_crop(struct tvp5150 *decoder, + struct v4l2_subdev_pad_config *cfg, unsigned int pad, + enum v4l2_subdev_format_whence which) +{ + switch (which) { + case V4L2_SUBDEV_FORMAT_TRY: + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); + case V4L2_SUBDEV_FORMAT_ACTIVE: + return &decoder->rect; + default: + return NULL; + } +} + static int tvp5150_fill_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *format) { struct v4l2_mbus_framefmt *f; + struct v4l2_rect *__crop; struct tvp5150 *decoder = to_tvp5150(sd); if (!format || (format->pad != DEMOD_PAD_VID_OUT)) return -EINVAL; f = &format->format; + __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad, + format->which); - f->width = decoder->rect.width; - f->height = decoder->rect.height / 2; + f->width = __crop->width; + f->height = __crop->height / 2; f->code = MEDIA_BUS_FMT_UYVY8_2X8; f->field = V4L2_FIELD_ALTERNATE; @@ -870,17 +889,51 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd, return 0; } +unsigned int tvp5150_get_hmax(struct v4l2_subdev *sd) +{ + struct tvp5150 *decoder = to_tvp5150(sd); + v4l2_std_id std; + + /* Calculate height based on current standard */ + if (decoder->norm == V4L2_STD_ALL) + std = tvp5150_read_std(sd); + else + std = decoder->norm; + + return (std & V4L2_STD_525_60) ? + TVP5150_V_MAX_525_60 : TVP5150_V_MAX_OTHERS; +} + +static inline void +__tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect) +{ + struct tvp5150 *decoder = to_tvp5150(sd); + unsigned int hmax = tvp5150_get_hmax(sd); + + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, + rect.top + rect.height - hmax); + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, + rect.left >> TVP5150_CROP_SHIFT); + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, + rect.left | (1 << TVP5150_CROP_SHIFT)); + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, + (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> + TVP5150_CROP_SHIFT); + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, + rect.left + rect.width - TVP5150_MAX_CROP_LEFT); +} + static int tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_selection *sel) { struct tvp5150 *decoder = to_tvp5150(sd); struct v4l2_rect rect = sel->r; - v4l2_std_id std; - int hmax; + struct v4l2_rect *__crop; + unsigned int hmax; - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE || - sel->target != V4L2_SEL_TGT_CROP) + if (sel->target != V4L2_SEL_TGT_CROP) return -EINVAL; dev_dbg_lvl(sd->dev, 1, debug, "%s left=%d, top=%d, width=%d, height=%d\n", @@ -889,17 +942,7 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd, /* tvp5150 has some special limits */ rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT); rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP); - - /* Calculate height based on current standard */ - if (decoder->norm == V4L2_STD_ALL) - std = tvp5150_read_std(sd); - else - std = decoder->norm; - - if (std & V4L2_STD_525_60) - hmax = TVP5150_V_MAX_525_60; - else - hmax = TVP5150_V_MAX_OTHERS; + hmax = tvp5150_get_hmax(sd); /* * alignments: @@ -912,20 +955,18 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd, hmax - TVP5150_MAX_CROP_TOP - rect.top, hmax - rect.top, 0, 0); - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, - rect.top + rect.height - hmax); - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, - rect.left >> TVP5150_CROP_SHIFT); - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, - rect.left | (1 << TVP5150_CROP_SHIFT)); - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, - (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> - TVP5150_CROP_SHIFT); - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, - rect.left + rect.width - TVP5150_MAX_CROP_LEFT); + __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, + sel->which); + + /* + * Update output image size if the selection (crop) rectangle size or + * position has been modified. + */ + if (!v4l2_rect_equal(&rect, __crop)) + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) + __tvp5150_set_selection(sd, rect); - decoder->rect = rect; + *__crop = rect; return 0; } @@ -937,9 +978,6 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd); v4l2_std_id std; - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE) - return -EINVAL; - switch (sel->target) { case V4L2_SEL_TGT_CROP_BOUNDS: case V4L2_SEL_TGT_CROP_DEFAULT: @@ -958,7 +996,8 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, sel->r.height = TVP5150_V_MAX_OTHERS; return 0; case V4L2_SEL_TGT_CROP: - sel->r = decoder->rect; + sel->r = *__tvp5150_get_pad_crop(decoder, cfg, sel->pad, + sel->which); return 0; default: return -EINVAL;
Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is no way to try different selections. The patch adds a helper function to select the correct selection memory space (sub-device file handle or driver state) which will be set/returned. The TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE and the requested selection rectangle differs from the already set one. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/media/i2c/tvp5150.c | 107 ++++++++++++++++++++++++------------ 1 file changed, 73 insertions(+), 34 deletions(-)