Re: [PATCH] media: vimc: Implement get/set selection
On 9/25/19 8:15 PM, Helen Koike wrote:
> +Hans +Shuah
>
> Hi Guilherme and Danilo,
>
> Thank you for the patch, please see my comments below.
>
> On 9/9/19 1:08 AM, Guilherme Alcarde Gallo wrote:
>> Add support for the scaler subdevice to respond VIDIOC_G_SELECTION and
>> VIDIOC_S_SELECTION ioctls with the following targets:
>> V4L2_SEL_TGT_COMPOSE_BOUNDS and V4L2_SEL_TGT_CROP.
>>
>> * Added new const struct crop_rect_default to initialize subdev scaler
>> properly.
>> * Make changes in sink pad format reflect the crop rectangle. E.g.
>> changing the frame format to a smaller size one can make the former
>> crop rectangle selects a non existing frame area. To solve this
>> situation the crop rectangle is clamped to the frame boundaries.
>> * Clamp crop rectangle respecting the sink bounds during set_selection
>> ioctl.
>>
>> Signed-off-by: Guilherme Alcarde Gallo
>> Co-developed-by: Danilo Figueiredo Rocha
>> Signed-off-by: Danilo Figueiredo Rocha
>>
>> ---
>>
>> This patch is based on the monolithic vimc driver from the patchset
>> named "Collapse vimc into single monolithic driver"
>> https://patchwork.kernel.org/patch/11136201/
>>
>> ---
>>
>> drivers/media/platform/vimc/vimc-scaler.c | 148 +++---
>> 1 file changed, 133 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c
>> b/drivers/media/platform/vimc/vimc-scaler.c
>> index a5a0855ad9cd..b50d11e76a2b 100644
>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>> @@ -18,6 +18,9 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>
>> #define MAX_ZOOM8
>>
>> +#define VIMC_SCA_FMT_WIDTH_DEFAULT 640
>> +#define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
>> +
>> struct vimc_sca_device {
>> struct vimc_ent_device ved;
>> struct v4l2_subdev sd;
>> @@ -26,6 +29,7 @@ struct vimc_sca_device {
>> * with the width and hight multiplied by mult
>> */
>> struct v4l2_mbus_framefmt sink_fmt;
>> +struct v4l2_rect crop_rect;
>> /* Values calculated when the stream starts */
>> u8 *src_frame;
>> unsigned int src_line_size;
>> @@ -33,22 +37,33 @@ struct vimc_sca_device {
>> };
>>
>> static const struct v4l2_mbus_framefmt sink_fmt_default = {
>> -.width = 640,
>> -.height = 480,
>> +.width = VIMC_SCA_FMT_WIDTH_DEFAULT,
>> +.height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
>> .code = MEDIA_BUS_FMT_RGB888_1X24,
>> .field = V4L2_FIELD_NONE,
>> .colorspace = V4L2_COLORSPACE_DEFAULT,
>> };
>>
>> +static const struct v4l2_rect crop_rect_default = {
>> +.width = VIMC_SCA_FMT_WIDTH_DEFAULT,
>> +.height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
>> +.top = 0,
>> +.left = 0,
>> +};
>> +
>> static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>> struct v4l2_subdev_pad_config *cfg)
>> {
>> struct v4l2_mbus_framefmt *mf;
>> +struct v4l2_rect *r;
>> unsigned int i;
>>
>> mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>> *mf = sink_fmt_default;
>>
>> +r = v4l2_subdev_get_try_crop(sd, cfg, 0);
>> +*r = crop_rect_default;
>> +
>> for (i = 1; i < sd->entity.num_pads; i++) {
>> mf = v4l2_subdev_get_try_format(sd, cfg, i);
>> *mf = sink_fmt_default;
>> @@ -107,16 +122,21 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
>> struct v4l2_subdev_format *format)
>> {
>> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
>> +struct v4l2_rect *crop_rect;
>>
>> /* Get the current sink format */
>> -format->format = (format->which == V4L2_SUBDEV_FORMAT_TRY) ?
>> - *v4l2_subdev_get_try_format(sd, cfg, 0) :
>> - vsca->sink_fmt;
>> +if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>> +format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
>> +crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
>> +} else {
>> +format->format = vsca->sink_fmt;
>> +crop_rect = &vsca->crop_rect;
>> +}
>>
>> /* Scale the frame size for the source pad */
>> if (VIMC_IS_SRC(format->pad)) {
>> -format->format.width = vsca->sink_fmt.width * sca_mult;
>> -format->format.height = vsca->sink_fmt.height * sca_mult;
>> +format->format.width = crop_rect->width * sca_mult;
>> +format->format.height = crop_rect->height * sca_mult;
>> }
>>
>> return 0;
>> @@ -148,6 +168,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>> {
>> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
>> struct v4l2_mbus_framefmt *sink_fmt;
>> +struct v4l2_rect *crop_rect;
>>
>> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>> /* Do not change the format while stream is on */
>> @@ -155,8 +176,10 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>> return -EBUSY;
>>
Re: [PATCH] media: vimc: Implement get/set selection
+Hans +Shuah
Hi Guilherme and Danilo,
Thank you for the patch, please see my comments below.
On 9/9/19 1:08 AM, Guilherme Alcarde Gallo wrote:
> Add support for the scaler subdevice to respond VIDIOC_G_SELECTION and
> VIDIOC_S_SELECTION ioctls with the following targets:
> V4L2_SEL_TGT_COMPOSE_BOUNDS and V4L2_SEL_TGT_CROP.
>
> * Added new const struct crop_rect_default to initialize subdev scaler
> properly.
> * Make changes in sink pad format reflect the crop rectangle. E.g.
> changing the frame format to a smaller size one can make the former
> crop rectangle selects a non existing frame area. To solve this
> situation the crop rectangle is clamped to the frame boundaries.
> * Clamp crop rectangle respecting the sink bounds during set_selection
> ioctl.
>
> Signed-off-by: Guilherme Alcarde Gallo
> Co-developed-by: Danilo Figueiredo Rocha
> Signed-off-by: Danilo Figueiredo Rocha
>
> ---
>
> This patch is based on the monolithic vimc driver from the patchset
> named "Collapse vimc into single monolithic driver"
> https://patchwork.kernel.org/patch/11136201/
>
> ---
>
> drivers/media/platform/vimc/vimc-scaler.c | 148 +++---
> 1 file changed, 133 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c
> b/drivers/media/platform/vimc/vimc-scaler.c
> index a5a0855ad9cd..b50d11e76a2b 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -18,6 +18,9 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>
> #define MAX_ZOOM 8
>
> +#define VIMC_SCA_FMT_WIDTH_DEFAULT 640
> +#define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
> +
> struct vimc_sca_device {
> struct vimc_ent_device ved;
> struct v4l2_subdev sd;
> @@ -26,6 +29,7 @@ struct vimc_sca_device {
>* with the width and hight multiplied by mult
>*/
> struct v4l2_mbus_framefmt sink_fmt;
> + struct v4l2_rect crop_rect;
> /* Values calculated when the stream starts */
> u8 *src_frame;
> unsigned int src_line_size;
> @@ -33,22 +37,33 @@ struct vimc_sca_device {
> };
>
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> - .width = 640,
> - .height = 480,
> + .width = VIMC_SCA_FMT_WIDTH_DEFAULT,
> + .height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
> .code = MEDIA_BUS_FMT_RGB888_1X24,
> .field = V4L2_FIELD_NONE,
> .colorspace = V4L2_COLORSPACE_DEFAULT,
> };
>
> +static const struct v4l2_rect crop_rect_default = {
> + .width = VIMC_SCA_FMT_WIDTH_DEFAULT,
> + .height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
> + .top = 0,
> + .left = 0,
> +};
> +
> static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>struct v4l2_subdev_pad_config *cfg)
> {
> struct v4l2_mbus_framefmt *mf;
> + struct v4l2_rect *r;
> unsigned int i;
>
> mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> *mf = sink_fmt_default;
>
> + r = v4l2_subdev_get_try_crop(sd, cfg, 0);
> + *r = crop_rect_default;
> +
> for (i = 1; i < sd->entity.num_pads; i++) {
> mf = v4l2_subdev_get_try_format(sd, cfg, i);
> *mf = sink_fmt_default;
> @@ -107,16 +122,21 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *format)
> {
> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> + struct v4l2_rect *crop_rect;
>
> /* Get the current sink format */
> - format->format = (format->which == V4L2_SUBDEV_FORMAT_TRY) ?
> - *v4l2_subdev_get_try_format(sd, cfg, 0) :
> - vsca->sink_fmt;
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> + format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
> + } else {
> + format->format = vsca->sink_fmt;
> + crop_rect = &vsca->crop_rect;
> + }
>
> /* Scale the frame size for the source pad */
> if (VIMC_IS_SRC(format->pad)) {
> - format->format.width = vsca->sink_fmt.width * sca_mult;
> - format->format.height = vsca->sink_fmt.height * sca_mult;
> + format->format.width = crop_rect->width * sca_mult;
> + format->format.height = crop_rect->height * sca_mult;
> }
>
> return 0;
> @@ -148,6 +168,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> {
> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt *sink_fmt;
> + struct v4l2_rect *crop_rect;
>
> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> /* Do not change the format while stream is on */
> @@ -155,8 +176,10 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> return -EBUSY;
>
> sink_fmt = &vsca->sink_fmt;
> + crop_rect = &vsca->crop_rect;
> } else {
>
