Re: [PATCH v7 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers
On 8/19/19 12:40 PM, Marco Felsch wrote: > On 19-08-16 13:27, Hans Verkuil wrote: >> On 8/15/19 1:57 PM, Marco Felsch wrote: >>> 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 >>> >>> --- >>> Changelog: >>> >>> v7: >>> - __tvp5150_get_pad_crop(): return error on default case >>> - simplify __tvp5150_get_pad_crop() error handling >>> - tvp5150_set_selection() squash __tvp5150_set_selection() execution >>> conditions >>> v6: >>> nothing >>> v5: >>> - handle stub for v4l2_subdev_get_try_crop() internal since commit >>>("media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*") >>>isn't anymore part of this series. >>> - add error handling of __tvp5150_get_pad_crop() >>> v4: >>> - fix merge conflict due to rebase on top of media-tree/master >>> - __tvp5150_get_pad_crop(): cosmetic alignment fixes >>> --- >>> drivers/media/i2c/tvp5150.c | 118 +--- >>> 1 file changed, 84 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c >>> index dfbf5bbc307c..ad59e65e6771 100644 >>> --- a/drivers/media/i2c/tvp5150.c >>> +++ b/drivers/media/i2c/tvp5150.c >>> @@ -19,6 +19,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include "tvp5150_reg.h" >>> >>> @@ -994,20 +995,44 @@ static void tvp5150_set_default(v4l2_std_id std, >>> struct v4l2_rect *crop) >>> crop->height = TVP5150_V_MAX_OTHERS; >>> } >>> >>> +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: >>> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) >>> + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); >>> +#else >>> + return ERR_PTR(-ENOTTY); >> >> That should be ERR_PTR(-EINVAL). Just because V4L2_SUBDEV_API is undefined, >> that >> doesn't mean that the whole functionality is not implemented! Just the TRY >> is not available. > > Okay.. The patch was made before you changed that for the other i2c > devices. I will change that. > >> >>> +#endif >>> + case V4L2_SUBDEV_FORMAT_ACTIVE: >>> + return &decoder->rect; >>> + default: >>> + return ERR_PTR(-EINVAL); >>> + } >>> +} >>> + >>> 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 != TVP5150_PAD_VID_OUT)) >>> return -EINVAL; >>> >>> f = &format->format; >>> + __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad, >>> + format->which); >>> + if (IS_ERR(__crop)) >>> + return PTR_ERR(__crop); >>> >>> - f->width = decoder->rect.width; >>> - f->height = decoder->rect.height / 2; >>> + f->width = __crop->width; >>> + f->height = __crop->height / 2; >>> >>> f->code = TVP5150_MBUS_FMT; >>> f->field = TVP5150_FIELD; >>> @@ -1018,17 +1043,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)); >>> + regm
Re: [PATCH v7 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers
On 19-08-16 13:27, Hans Verkuil wrote: > On 8/15/19 1:57 PM, Marco Felsch wrote: > > 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 > > > > --- > > Changelog: > > > > v7: > > - __tvp5150_get_pad_crop(): return error on default case > > - simplify __tvp5150_get_pad_crop() error handling > > - tvp5150_set_selection() squash __tvp5150_set_selection() execution > > conditions > > v6: > > nothing > > v5: > > - handle stub for v4l2_subdev_get_try_crop() internal since commit > >("media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*") > >isn't anymore part of this series. > > - add error handling of __tvp5150_get_pad_crop() > > v4: > > - fix merge conflict due to rebase on top of media-tree/master > > - __tvp5150_get_pad_crop(): cosmetic alignment fixes > > --- > > drivers/media/i2c/tvp5150.c | 118 +--- > > 1 file changed, 84 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > index dfbf5bbc307c..ad59e65e6771 100644 > > --- a/drivers/media/i2c/tvp5150.c > > +++ b/drivers/media/i2c/tvp5150.c > > @@ -19,6 +19,7 @@ > > #include > > #include > > #include > > +#include > > > > #include "tvp5150_reg.h" > > > > @@ -994,20 +995,44 @@ static void tvp5150_set_default(v4l2_std_id std, > > struct v4l2_rect *crop) > > crop->height = TVP5150_V_MAX_OTHERS; > > } > > > > +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: > > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > > + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > > +#else > > + return ERR_PTR(-ENOTTY); > > That should be ERR_PTR(-EINVAL). Just because V4L2_SUBDEV_API is undefined, > that > doesn't mean that the whole functionality is not implemented! Just the TRY > is not available. Okay.. The patch was made before you changed that for the other i2c devices. I will change that. > > > +#endif > > + case V4L2_SUBDEV_FORMAT_ACTIVE: > > + return &decoder->rect; > > + default: > > + return ERR_PTR(-EINVAL); > > + } > > +} > > + > > 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 != TVP5150_PAD_VID_OUT)) > > return -EINVAL; > > > > f = &format->format; > > + __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad, > > + format->which); > > + if (IS_ERR(__crop)) > > + return PTR_ERR(__crop); > > > > - f->width = decoder->rect.width; > > - f->height = decoder->rect.height / 2; > > + f->width = __crop->width; > > + f->height = __crop->height / 2; > > > > f->code = TVP5150_MBUS_FMT; > > f->field = TVP5150_FIELD; > > @@ -1018,17 +1043,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_M
Re: [PATCH v7 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers
On 8/15/19 1:57 PM, Marco Felsch wrote: > 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 > > --- > Changelog: > > v7: > - __tvp5150_get_pad_crop(): return error on default case > - simplify __tvp5150_get_pad_crop() error handling > - tvp5150_set_selection() squash __tvp5150_set_selection() execution > conditions > v6: > nothing > v5: > - handle stub for v4l2_subdev_get_try_crop() internal since commit >("media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*") >isn't anymore part of this series. > - add error handling of __tvp5150_get_pad_crop() > v4: > - fix merge conflict due to rebase on top of media-tree/master > - __tvp5150_get_pad_crop(): cosmetic alignment fixes > --- > drivers/media/i2c/tvp5150.c | 118 +--- > 1 file changed, 84 insertions(+), 34 deletions(-) > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > index dfbf5bbc307c..ad59e65e6771 100644 > --- a/drivers/media/i2c/tvp5150.c > +++ b/drivers/media/i2c/tvp5150.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include "tvp5150_reg.h" > > @@ -994,20 +995,44 @@ static void tvp5150_set_default(v4l2_std_id std, struct > v4l2_rect *crop) > crop->height = TVP5150_V_MAX_OTHERS; > } > > +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: > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > +#else > + return ERR_PTR(-ENOTTY); That should be ERR_PTR(-EINVAL). Just because V4L2_SUBDEV_API is undefined, that doesn't mean that the whole functionality is not implemented! Just the TRY is not available. > +#endif > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &decoder->rect; > + default: > + return ERR_PTR(-EINVAL); > + } > +} > + > 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 != TVP5150_PAD_VID_OUT)) > return -EINVAL; > > f = &format->format; > + __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad, > + format->which); > + if (IS_ERR(__crop)) > + return PTR_ERR(__crop); > > - f->width = decoder->rect.width; > - f->height = decoder->rect.height / 2; > + f->width = __crop->width; > + f->height = __crop->height / 2; > > f->code = TVP5150_MBUS_FMT; > f->field = TVP5150_FIELD; > @@ -1018,17 +1043,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(stru