| Message ID | 20180422223430.16407-3-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, very minor comments below On Mon, Apr 23, 2018 at 01:34:24AM +0300, Laurent Pinchart wrote: > The implementation of the set_fmt pad operation is identical in the > three modules. Move it to a generic helper function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/platform/vsp1/vsp1_clu.c | 65 +++++---------------------- > drivers/media/platform/vsp1/vsp1_entity.c | 75 +++++++++++++++++++++++++++++++ > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++ > drivers/media/platform/vsp1/vsp1_lif.c | 65 +++++---------------------- > drivers/media/platform/vsp1/vsp1_lut.c | 65 +++++---------------------- > 5 files changed, 116 insertions(+), 160 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c > index 9626b6308585..96a448e1504c 100644 > --- a/drivers/media/platform/vsp1/vsp1_clu.c > +++ b/drivers/media/platform/vsp1/vsp1_clu.c > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config clu_mode_control = { > * V4L2 Subdevice Pad Operations > */ > > +static const unsigned int clu_codes[] = { > + MEDIA_BUS_FMT_ARGB8888_1X32, > + MEDIA_BUS_FMT_AHSV8888_1X32, > + MEDIA_BUS_FMT_AYUV8_1X32, > +}; > + > static int clu_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - static const unsigned int codes[] = { > - MEDIA_BUS_FMT_ARGB8888_1X32, > - MEDIA_BUS_FMT_AHSV8888_1X32, > - MEDIA_BUS_FMT_AYUV8_1X32, > - }; > - > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > - ARRAY_SIZE(codes)); > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes, > + ARRAY_SIZE(clu_codes)); > } > > static int clu_enum_frame_size(struct v4l2_subdev *subdev, > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt) > { > - struct vsp1_clu *clu = to_clu(subdev); > - struct v4l2_subdev_pad_config *config; > - struct v4l2_mbus_framefmt *format; > - int ret = 0; > - > - mutex_lock(&clu->entity.lock); > - > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which); > - if (!config) { > - ret = -EINVAL; > - goto done; > - } > - > - /* Default to YUV if the requested format is not supported. */ > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; The newly implemented vsp1_subdev_set_pad_format defaults to the first clu_codes[] member (ARGB888_1x32), while here the code chose the AYUV8_1x32 format. Is it ok? Should you revers the clu_codes[] order? > - > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad); > - > - if (fmt->pad == CLU_PAD_SOURCE) { > - /* The CLU output format can't be modified. */ > - fmt->format = *format; > - goto done; > - } > - > - format->code = fmt->format.code; > - format->width = clamp_t(unsigned int, fmt->format.width, > - CLU_MIN_SIZE, CLU_MAX_SIZE); > - format->height = clamp_t(unsigned int, fmt->format.height, > - CLU_MIN_SIZE, CLU_MAX_SIZE); > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > - > - fmt->format = *format; > - > - /* Propagate the format to the source pad. */ > - format = vsp1_entity_get_pad_format(&clu->entity, config, > - CLU_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(&clu->entity.lock); > - return ret; > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, > + ARRAY_SIZE(clu_codes), > + CLU_MIN_SIZE, CLU_MIN_SIZE, > + CLU_MAX_SIZE, CLU_MAX_SIZE); > } > > /* ----------------------------------------------------------------------------- > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > index 72354caf5746..239df047efd0 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.c > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev, > return ret; > } > > +/* > + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler > + * @subdev: V4L2 subdevice > + * @cfg: V4L2 subdev pad configuration > + * @fmt: V4L2 subdev format > + * @codes: Array of supported media bus codes > + * @ncodes: Number of supported media bus codes > + * @min_width: Minimum image width > + * @min_height: Minimum image height > + * @max_width: Maximum image width > + * @max_height: Maximum image height > + * > + * This function implements the subdev set_fmt pad operation for entities that > + * do not support scaling or cropping. It defaults to the first supplied media > + * bus code if the requested code isn't supported, clamps the size to the > + * supplied minimum and maximum, and propagates the sink pad format to the > + * source pad. > + */ > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt, > + const unsigned int *codes, unsigned int ncodes, > + unsigned int min_width, unsigned int min_height, > + unsigned int max_width, unsigned int max_height) > +{ > + struct vsp1_entity *entity = to_vsp1_entity(subdev); > + struct v4l2_subdev_pad_config *config; > + struct v4l2_mbus_framefmt *format; > + unsigned int i; > + int ret = 0; > + > + mutex_lock(&entity->lock); > + > + config = vsp1_entity_get_pad_config(entity, cfg, fmt->which); > + if (!config) { > + ret = -EINVAL; > + goto done; > + } > + > + format = vsp1_entity_get_pad_format(entity, config, fmt->pad); > + > + if (fmt->pad != 0) { This assumes the SINK pad is always 0, which indeed is the case for CLU, LIF and LUT entities. > + /* The output format can't be modified. */ > + fmt->format = *format; > + goto done; > + } > + > + /* > + * Default to the first media bus code if the requested format is not > + * supported. > + */ > + for (i = 0; i < ncodes; ++i) { > + if (fmt->format.code == codes[i]) > + break; > + } Braces not needed? > + > + format->code = i < ncodes ? codes[i] : codes[0]; > + format->width = clamp_t(unsigned int, fmt->format.width, > + min_width, max_width); > + format->height = clamp_t(unsigned int, fmt->format.height, > + min_height, max_height); > + format->field = V4L2_FIELD_NONE; > + format->colorspace = V4L2_COLORSPACE_SRGB; > + > + fmt->format = *format; > + > + /* Propagate the format to the source pad. */ > + format = vsp1_entity_get_pad_format(entity, config, 1); > + *format = fmt->format; > + > +done: > + mutex_unlock(&entity->lock); > + return ret; > +} > + > /* ----------------------------------------------------------------------------- > * Media Operations > */ > diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h > index fb20a1578f3b..0839a62cfa71 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.h > +++ b/drivers/media/platform/vsp1/vsp1_entity.h > @@ -160,6 +160,12 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad); > int vsp1_subdev_get_pad_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt); > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt, > + const unsigned int *codes, unsigned int ncodes, > + unsigned int min_width, unsigned int min_height, > + unsigned int max_width, unsigned int max_height); > int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code, > diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c > index b20b842f06ba..fbdd5715f829 100644 > --- a/drivers/media/platform/vsp1/vsp1_lif.c > +++ b/drivers/media/platform/vsp1/vsp1_lif.c > @@ -33,17 +33,17 @@ static inline void vsp1_lif_write(struct vsp1_lif *lif, struct vsp1_dl_list *dl, > * V4L2 Subdevice Operations > */ > > +static const unsigned int lif_codes[] = { > + MEDIA_BUS_FMT_ARGB8888_1X32, > + MEDIA_BUS_FMT_AYUV8_1X32, > +}; > + > static int lif_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - static const unsigned int codes[] = { > - MEDIA_BUS_FMT_ARGB8888_1X32, > - MEDIA_BUS_FMT_AYUV8_1X32, > - }; > - > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > - ARRAY_SIZE(codes)); > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lif_codes, > + ARRAY_SIZE(lif_codes)); > } > > static int lif_enum_frame_size(struct v4l2_subdev *subdev, > @@ -59,53 +59,10 @@ static int lif_set_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt) > { > - struct vsp1_lif *lif = to_lif(subdev); > - struct v4l2_subdev_pad_config *config; > - struct v4l2_mbus_framefmt *format; > - int ret = 0; > - > - mutex_lock(&lif->entity.lock); > - > - config = vsp1_entity_get_pad_config(&lif->entity, cfg, fmt->which); > - if (!config) { > - ret = -EINVAL; > - goto done; > - } > - > - /* Default to YUV if the requested format is not supported. */ > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; Same here and below Thanks j > - > - format = vsp1_entity_get_pad_format(&lif->entity, config, fmt->pad); > - > - if (fmt->pad == LIF_PAD_SOURCE) { > - /* > - * The LIF source format is always identical to its sink > - * format. > - */ > - fmt->format = *format; > - goto done; > - } > - > - format->code = fmt->format.code; > - format->width = clamp_t(unsigned int, fmt->format.width, > - LIF_MIN_SIZE, LIF_MAX_SIZE); > - format->height = clamp_t(unsigned int, fmt->format.height, > - LIF_MIN_SIZE, LIF_MAX_SIZE); > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > - > - fmt->format = *format; > - > - /* Propagate the format to the source pad. */ > - format = vsp1_entity_get_pad_format(&lif->entity, config, > - LIF_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(&lif->entity.lock); > - return ret; > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lif_codes, > + ARRAY_SIZE(lif_codes), > + LIF_MIN_SIZE, LIF_MIN_SIZE, > + LIF_MAX_SIZE, LIF_MAX_SIZE); > } > > static const struct v4l2_subdev_pad_ops lif_pad_ops = { > diff --git a/drivers/media/platform/vsp1/vsp1_lut.c b/drivers/media/platform/vsp1/vsp1_lut.c > index 7bdabb311c6c..f2e48a02ca7d 100644 > --- a/drivers/media/platform/vsp1/vsp1_lut.c > +++ b/drivers/media/platform/vsp1/vsp1_lut.c > @@ -90,18 +90,18 @@ static const struct v4l2_ctrl_config lut_table_control = { > * V4L2 Subdevice Pad Operations > */ > > +static const unsigned int lut_codes[] = { > + MEDIA_BUS_FMT_ARGB8888_1X32, > + MEDIA_BUS_FMT_AHSV8888_1X32, > + MEDIA_BUS_FMT_AYUV8_1X32, > +}; > + > static int lut_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - static const unsigned int codes[] = { > - MEDIA_BUS_FMT_ARGB8888_1X32, > - MEDIA_BUS_FMT_AHSV8888_1X32, > - MEDIA_BUS_FMT_AYUV8_1X32, > - }; > - > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > - ARRAY_SIZE(codes)); > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lut_codes, > + ARRAY_SIZE(lut_codes)); > } > > static int lut_enum_frame_size(struct v4l2_subdev *subdev, > @@ -117,51 +117,10 @@ static int lut_set_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt) > { > - struct vsp1_lut *lut = to_lut(subdev); > - struct v4l2_subdev_pad_config *config; > - struct v4l2_mbus_framefmt *format; > - int ret = 0; > - > - mutex_lock(&lut->entity.lock); > - > - config = vsp1_entity_get_pad_config(&lut->entity, cfg, fmt->which); > - if (!config) { > - ret = -EINVAL; > - goto done; > - } > - > - /* Default to YUV if the requested format is not supported. */ > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > - > - format = vsp1_entity_get_pad_format(&lut->entity, config, fmt->pad); > - > - if (fmt->pad == LUT_PAD_SOURCE) { > - /* The LUT output format can't be modified. */ > - fmt->format = *format; > - goto done; > - } > - > - format->code = fmt->format.code; > - format->width = clamp_t(unsigned int, fmt->format.width, > - LUT_MIN_SIZE, LUT_MAX_SIZE); > - format->height = clamp_t(unsigned int, fmt->format.height, > - LUT_MIN_SIZE, LUT_MAX_SIZE); > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > - > - fmt->format = *format; > - > - /* Propagate the format to the source pad. */ > - format = vsp1_entity_get_pad_format(&lut->entity, config, > - LUT_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(&lut->entity.lock); > - return ret; > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lut_codes, > + ARRAY_SIZE(lut_codes), > + LUT_MIN_SIZE, LUT_MIN_SIZE, > + LUT_MAX_SIZE, LUT_MAX_SIZE); > } > > /* ----------------------------------------------------------------------------- > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Saturday, 28 April 2018 12:50:48 EEST jacopo mondi wrote: > Hi Laurent, > very minor comments below > > On Mon, Apr 23, 2018 at 01:34:24AM +0300, Laurent Pinchart wrote: > > The implementation of the set_fmt pad operation is identical in the > > three modules. Move it to a generic helper function. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > drivers/media/platform/vsp1/vsp1_clu.c | 65 +++++-------------------- > > drivers/media/platform/vsp1/vsp1_entity.c | 75 ++++++++++++++++++++++++++ > > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++ > > drivers/media/platform/vsp1/vsp1_lif.c | 65 +++++-------------------- > > drivers/media/platform/vsp1/vsp1_lut.c | 65 +++++-------------------- > > 5 files changed, 116 insertions(+), 160 deletions(-) > > > > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c > > b/drivers/media/platform/vsp1/vsp1_clu.c index 9626b6308585..96a448e1504c > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_clu.c > > +++ b/drivers/media/platform/vsp1/vsp1_clu.c > > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config > > clu_mode_control = { > > * V4L2 Subdevice Pad Operations > > */ > > > > +static const unsigned int clu_codes[] = { > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > + MEDIA_BUS_FMT_AHSV8888_1X32, > > + MEDIA_BUS_FMT_AYUV8_1X32, > > +}; > > + > > static int clu_enum_mbus_code(struct v4l2_subdev *subdev, > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code) > > { > > - static const unsigned int codes[] = { > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > - MEDIA_BUS_FMT_AHSV8888_1X32, > > - MEDIA_BUS_FMT_AYUV8_1X32, > > - }; > > - > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > - ARRAY_SIZE(codes)); > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes, > > + ARRAY_SIZE(clu_codes)); > > } > > > > static int clu_enum_frame_size(struct v4l2_subdev *subdev, > > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev > > *subdev, > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_format *fmt) > > { > > - struct vsp1_clu *clu = to_clu(subdev); > > - struct v4l2_subdev_pad_config *config; > > - struct v4l2_mbus_framefmt *format; > > - int ret = 0; > > - > > - mutex_lock(&clu->entity.lock); > > - > > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which); > > - if (!config) { > > - ret = -EINVAL; > > - goto done; > > - } > > - > > - /* Default to YUV if the requested format is not supported. */ > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > The newly implemented vsp1_subdev_set_pad_format defaults to the first > clu_codes[] member (ARGB888_1x32), while here the code chose the AYUV8_1x32 > format. Is it ok? Should you revers the clu_codes[] order? But that would then change the order of the format enumeration. I don't think it's a big deal, the change here will only affect the format returned if userspace tries to pick a format that is not supported (and thus not returned by the enumeration). This shouldn't happen in the first place, and if it does, the driver has never guaranteed that a specific format would be returned. > > - > > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad); > > - > > - if (fmt->pad == CLU_PAD_SOURCE) { > > - /* The CLU output format can't be modified. */ > > - fmt->format = *format; > > - goto done; > > - } > > - > > - format->code = fmt->format.code; > > - format->width = clamp_t(unsigned int, fmt->format.width, > > - CLU_MIN_SIZE, CLU_MAX_SIZE); > > - format->height = clamp_t(unsigned int, fmt->format.height, > > - CLU_MIN_SIZE, CLU_MAX_SIZE); > > - format->field = V4L2_FIELD_NONE; > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > - > > - fmt->format = *format; > > - > > - /* Propagate the format to the source pad. */ > > - format = vsp1_entity_get_pad_format(&clu->entity, config, > > - CLU_PAD_SOURCE); > > - *format = fmt->format; > > - > > -done: > > - mutex_unlock(&clu->entity.lock); > > - return ret; > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, > > + ARRAY_SIZE(clu_codes), > > + CLU_MIN_SIZE, CLU_MIN_SIZE, > > + CLU_MAX_SIZE, CLU_MAX_SIZE); > > } > > > > /* ---------------------------------------------------------------------- > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c > > b/drivers/media/platform/vsp1/vsp1_entity.c index > > 72354caf5746..239df047efd0 100644 > > --- a/drivers/media/platform/vsp1/vsp1_entity.c > > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > > @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev > > *subdev, > > return ret; > > } > > > > +/* > > + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler > > + * @subdev: V4L2 subdevice > > + * @cfg: V4L2 subdev pad configuration > > + * @fmt: V4L2 subdev format > > + * @codes: Array of supported media bus codes > > + * @ncodes: Number of supported media bus codes > > + * @min_width: Minimum image width > > + * @min_height: Minimum image height > > + * @max_width: Maximum image width > > + * @max_height: Maximum image height > > + * > > + * This function implements the subdev set_fmt pad operation for entities > > that > > + * do not support scaling or cropping. It defaults to the first supplied > > media > > + * bus code if the requested code isn't supported, clamps the size to the > > + * supplied minimum and maximum, and propagates the sink pad format to > > the > > + * source pad. > > + */ > > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt, > > + const unsigned int *codes, unsigned int ncodes, > > + unsigned int min_width, unsigned int min_height, > > + unsigned int max_width, unsigned int max_height) > > +{ > > + struct vsp1_entity *entity = to_vsp1_entity(subdev); > > + struct v4l2_subdev_pad_config *config; > > + struct v4l2_mbus_framefmt *format; > > + unsigned int i; > > + int ret = 0; > > + > > + mutex_lock(&entity->lock); > > + > > + config = vsp1_entity_get_pad_config(entity, cfg, fmt->which); > > + if (!config) { > > + ret = -EINVAL; > > + goto done; > > + } > > + > > + format = vsp1_entity_get_pad_format(entity, config, fmt->pad); > > + > > + if (fmt->pad != 0) { > > This assumes the SINK pad is always 0, which indeed is the case for > CLU, LIF and LUT entities. > > > + /* The output format can't be modified. */ > > + fmt->format = *format; > > + goto done; > > + } > > + > > + /* > > + * Default to the first media bus code if the requested format is not > > + * supported. > > + */ > > + for (i = 0; i < ncodes; ++i) { > > + if (fmt->format.code == codes[i]) > > + break; > > + } > > Braces not needed? Not strictly required but I find it more readable. > > + > > + format->code = i < ncodes ? codes[i] : codes[0]; > > + format->width = clamp_t(unsigned int, fmt->format.width, > > + min_width, max_width); > > + format->height = clamp_t(unsigned int, fmt->format.height, > > + min_height, max_height); > > + format->field = V4L2_FIELD_NONE; > > + format->colorspace = V4L2_COLORSPACE_SRGB; > > + > > + fmt->format = *format; > > + > > + /* Propagate the format to the source pad. */ > > + format = vsp1_entity_get_pad_format(entity, config, 1); > > + *format = fmt->format; > > + > > +done: > > + mutex_unlock(&entity->lock); > > + return ret; > > +} [snip]
Hi Laurent, On 22/04/18 23:34, Laurent Pinchart wrote: > The implementation of the set_fmt pad operation is identical in the > three modules. Move it to a generic helper function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Only a minor pair of comments below regarding source/sink pad descriptions. If it's not convenient/accurate to define these with an enum then don't worry about it. Otherwise, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/platform/vsp1/vsp1_clu.c | 65 +++++---------------------- > drivers/media/platform/vsp1/vsp1_entity.c | 75 +++++++++++++++++++++++++++++++ > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++ > drivers/media/platform/vsp1/vsp1_lif.c | 65 +++++---------------------- > drivers/media/platform/vsp1/vsp1_lut.c | 65 +++++---------------------- > 5 files changed, 116 insertions(+), 160 deletions(-) That's a nice diffstat :-) > > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c > index 9626b6308585..96a448e1504c 100644 > --- a/drivers/media/platform/vsp1/vsp1_clu.c > +++ b/drivers/media/platform/vsp1/vsp1_clu.c > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config clu_mode_control = { > * V4L2 Subdevice Pad Operations > */ > > +static const unsigned int clu_codes[] = { > + MEDIA_BUS_FMT_ARGB8888_1X32, > + MEDIA_BUS_FMT_AHSV8888_1X32, > + MEDIA_BUS_FMT_AYUV8_1X32, > +}; > + > static int clu_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - static const unsigned int codes[] = { > - MEDIA_BUS_FMT_ARGB8888_1X32, > - MEDIA_BUS_FMT_AHSV8888_1X32, > - MEDIA_BUS_FMT_AYUV8_1X32, > - }; > - > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > - ARRAY_SIZE(codes)); > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes, > + ARRAY_SIZE(clu_codes)); > } > > static int clu_enum_frame_size(struct v4l2_subdev *subdev, > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt) > { > - struct vsp1_clu *clu = to_clu(subdev); > - struct v4l2_subdev_pad_config *config; > - struct v4l2_mbus_framefmt *format; > - int ret = 0; > - > - mutex_lock(&clu->entity.lock); > - > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which); > - if (!config) { > - ret = -EINVAL; > - goto done; > - } > - > - /* Default to YUV if the requested format is not supported. */ > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > - > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad); > - > - if (fmt->pad == CLU_PAD_SOURCE) { > - /* The CLU output format can't be modified. */ > - fmt->format = *format; > - goto done; > - } > - > - format->code = fmt->format.code; > - format->width = clamp_t(unsigned int, fmt->format.width, > - CLU_MIN_SIZE, CLU_MAX_SIZE); > - format->height = clamp_t(unsigned int, fmt->format.height, > - CLU_MIN_SIZE, CLU_MAX_SIZE); > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > - > - fmt->format = *format; > - > - /* Propagate the format to the source pad. */ > - format = vsp1_entity_get_pad_format(&clu->entity, config, > - CLU_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(&clu->entity.lock); > - return ret; > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, > + ARRAY_SIZE(clu_codes), > + CLU_MIN_SIZE, CLU_MIN_SIZE, > + CLU_MAX_SIZE, CLU_MAX_SIZE); > } > > /* ----------------------------------------------------------------------------- > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > index 72354caf5746..239df047efd0 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.c > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev, > return ret; > } > > +/* > + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler > + * @subdev: V4L2 subdevice > + * @cfg: V4L2 subdev pad configuration > + * @fmt: V4L2 subdev format > + * @codes: Array of supported media bus codes > + * @ncodes: Number of supported media bus codes > + * @min_width: Minimum image width > + * @min_height: Minimum image height > + * @max_width: Maximum image width > + * @max_height: Maximum image height > + * > + * This function implements the subdev set_fmt pad operation for entities that > + * do not support scaling or cropping. It defaults to the first supplied media > + * bus code if the requested code isn't supported, clamps the size to the > + * supplied minimum and maximum, and propagates the sink pad format to the > + * source pad. > + */ > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt, > + const unsigned int *codes, unsigned int ncodes, > + unsigned int min_width, unsigned int min_height, > + unsigned int max_width, unsigned int max_height) > +{ > + struct vsp1_entity *entity = to_vsp1_entity(subdev); > + struct v4l2_subdev_pad_config *config; > + struct v4l2_mbus_framefmt *format; > + unsigned int i; > + int ret = 0; > + > + mutex_lock(&entity->lock); > + > + config = vsp1_entity_get_pad_config(entity, cfg, fmt->which); > + if (!config) { > + ret = -EINVAL; > + goto done; > + } > + > + format = vsp1_entity_get_pad_format(entity, config, fmt->pad); > + > + if (fmt->pad != 0) { I guess we don't have any clear way to say != *_PAD_SINK here do we .. > + /* The output format can't be modified. */ > + fmt->format = *format; > + goto done; > + } > + > + /* > + * Default to the first media bus code if the requested format is not > + * supported. > + */ > + for (i = 0; i < ncodes; ++i) { > + if (fmt->format.code == codes[i]) > + break; > + } > + > + format->code = i < ncodes ? codes[i] : codes[0]; > + format->width = clamp_t(unsigned int, fmt->format.width, > + min_width, max_width); > + format->height = clamp_t(unsigned int, fmt->format.height, > + min_height, max_height); > + format->field = V4L2_FIELD_NONE; > + format->colorspace = V4L2_COLORSPACE_SRGB; > + > + fmt->format = *format; > + > + /* Propagate the format to the source pad. */ > + format = vsp1_entity_get_pad_format(entity, config, 1); If we can guarantee that ENTITY_PAD_SINK == 0 and ENTITY_PAD_SOURCE == 1, can we put those into an enum ? > + *format = fmt->format; > + > +done: > + mutex_unlock(&entity->lock); > + return ret; > +} > + > /* ----------------------------------------------------------------------------- > * Media Operations > */ > diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h > index fb20a1578f3b..0839a62cfa71 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.h > +++ b/drivers/media/platform/vsp1/vsp1_entity.h > @@ -160,6 +160,12 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad); > int vsp1_subdev_get_pad_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt); > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt, > + const unsigned int *codes, unsigned int ncodes, > + unsigned int min_width, unsigned int min_height, > + unsigned int max_width, unsigned int max_height); > int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code, > diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c > index b20b842f06ba..fbdd5715f829 100644 > --- a/drivers/media/platform/vsp1/vsp1_lif.c > +++ b/drivers/media/platform/vsp1/vsp1_lif.c > @@ -33,17 +33,17 @@ static inline void vsp1_lif_write(struct vsp1_lif *lif, struct vsp1_dl_list *dl, > * V4L2 Subdevice Operations > */ > > +static const unsigned int lif_codes[] = { > + MEDIA_BUS_FMT_ARGB8888_1X32, > + MEDIA_BUS_FMT_AYUV8_1X32, > +}; > + > static int lif_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - static const unsigned int codes[] = { > - MEDIA_BUS_FMT_ARGB8888_1X32, > - MEDIA_BUS_FMT_AYUV8_1X32, > - }; > - > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > - ARRAY_SIZE(codes)); > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lif_codes, > + ARRAY_SIZE(lif_codes)); > } > > static int lif_enum_frame_size(struct v4l2_subdev *subdev, > @@ -59,53 +59,10 @@ static int lif_set_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt) > { > - struct vsp1_lif *lif = to_lif(subdev); > - struct v4l2_subdev_pad_config *config; > - struct v4l2_mbus_framefmt *format; > - int ret = 0; > - > - mutex_lock(&lif->entity.lock); > - > - config = vsp1_entity_get_pad_config(&lif->entity, cfg, fmt->which); > - if (!config) { > - ret = -EINVAL; > - goto done; > - } > - > - /* Default to YUV if the requested format is not supported. */ > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > - > - format = vsp1_entity_get_pad_format(&lif->entity, config, fmt->pad); > - > - if (fmt->pad == LIF_PAD_SOURCE) { > - /* > - * The LIF source format is always identical to its sink > - * format. > - */ > - fmt->format = *format; > - goto done; > - } > - > - format->code = fmt->format.code; > - format->width = clamp_t(unsigned int, fmt->format.width, > - LIF_MIN_SIZE, LIF_MAX_SIZE); > - format->height = clamp_t(unsigned int, fmt->format.height, > - LIF_MIN_SIZE, LIF_MAX_SIZE); > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > - > - fmt->format = *format; > - > - /* Propagate the format to the source pad. */ > - format = vsp1_entity_get_pad_format(&lif->entity, config, > - LIF_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(&lif->entity.lock); > - return ret; > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lif_codes, > + ARRAY_SIZE(lif_codes), > + LIF_MIN_SIZE, LIF_MIN_SIZE, > + LIF_MAX_SIZE, LIF_MAX_SIZE); > } > > static const struct v4l2_subdev_pad_ops lif_pad_ops = { > diff --git a/drivers/media/platform/vsp1/vsp1_lut.c b/drivers/media/platform/vsp1/vsp1_lut.c > index 7bdabb311c6c..f2e48a02ca7d 100644 > --- a/drivers/media/platform/vsp1/vsp1_lut.c > +++ b/drivers/media/platform/vsp1/vsp1_lut.c > @@ -90,18 +90,18 @@ static const struct v4l2_ctrl_config lut_table_control = { > * V4L2 Subdevice Pad Operations > */ > > +static const unsigned int lut_codes[] = { > + MEDIA_BUS_FMT_ARGB8888_1X32, > + MEDIA_BUS_FMT_AHSV8888_1X32, > + MEDIA_BUS_FMT_AYUV8_1X32, > +}; > + > static int lut_enum_mbus_code(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - static const unsigned int codes[] = { > - MEDIA_BUS_FMT_ARGB8888_1X32, > - MEDIA_BUS_FMT_AHSV8888_1X32, > - MEDIA_BUS_FMT_AYUV8_1X32, > - }; > - > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > - ARRAY_SIZE(codes)); > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lut_codes, > + ARRAY_SIZE(lut_codes)); > } > > static int lut_enum_frame_size(struct v4l2_subdev *subdev, > @@ -117,51 +117,10 @@ static int lut_set_format(struct v4l2_subdev *subdev, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *fmt) > { > - struct vsp1_lut *lut = to_lut(subdev); > - struct v4l2_subdev_pad_config *config; > - struct v4l2_mbus_framefmt *format; > - int ret = 0; > - > - mutex_lock(&lut->entity.lock); > - > - config = vsp1_entity_get_pad_config(&lut->entity, cfg, fmt->which); > - if (!config) { > - ret = -EINVAL; > - goto done; > - } > - > - /* Default to YUV if the requested format is not supported. */ > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > - > - format = vsp1_entity_get_pad_format(&lut->entity, config, fmt->pad); > - > - if (fmt->pad == LUT_PAD_SOURCE) { > - /* The LUT output format can't be modified. */ > - fmt->format = *format; > - goto done; > - } > - > - format->code = fmt->format.code; > - format->width = clamp_t(unsigned int, fmt->format.width, > - LUT_MIN_SIZE, LUT_MAX_SIZE); > - format->height = clamp_t(unsigned int, fmt->format.height, > - LUT_MIN_SIZE, LUT_MAX_SIZE); > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > - > - fmt->format = *format; > - > - /* Propagate the format to the source pad. */ > - format = vsp1_entity_get_pad_format(&lut->entity, config, > - LUT_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(&lut->entity.lock); > - return ret; > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lut_codes, > + ARRAY_SIZE(lut_codes), > + LUT_MIN_SIZE, LUT_MIN_SIZE, > + LUT_MAX_SIZE, LUT_MAX_SIZE); > } > > /* ----------------------------------------------------------------------------- >
Hi Kieran, On Saturday, 28 April 2018 20:16:11 EEST Kieran Bingham wrote: > On 22/04/18 23:34, Laurent Pinchart wrote: > > The implementation of the set_fmt pad operation is identical in the > > three modules. Move it to a generic helper function. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > Only a minor pair of comments below regarding source/sink pad descriptions. > > If it's not convenient/accurate to define these with an enum then don't > worry about it. It's a good point. There are however other locations in vsp1_entity.c that hardcode pad numbers, so I'll submit a patch on top of this series to fix them all in one go. > Otherwise, > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > --- > > > > drivers/media/platform/vsp1/vsp1_clu.c | 65 +++++------------------- > > drivers/media/platform/vsp1/vsp1_entity.c | 75 ++++++++++++++++++++++++++ > > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++ > > drivers/media/platform/vsp1/vsp1_lif.c | 65 +++++-------------------- > > drivers/media/platform/vsp1/vsp1_lut.c | 65 +++++-------------------- > > 5 files changed, 116 insertions(+), 160 deletions(-) > > That's a nice diffstat :-) > > > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c > > b/drivers/media/platform/vsp1/vsp1_clu.c index 9626b6308585..96a448e1504c > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_clu.c > > +++ b/drivers/media/platform/vsp1/vsp1_clu.c > > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config > > clu_mode_control = {> > > * V4L2 Subdevice Pad Operations > > */ > > > > +static const unsigned int clu_codes[] = { > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > + MEDIA_BUS_FMT_AHSV8888_1X32, > > + MEDIA_BUS_FMT_AYUV8_1X32, > > +}; > > + > > > > static int clu_enum_mbus_code(struct v4l2_subdev *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code) > > > > { > > > > - static const unsigned int codes[] = { > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > - MEDIA_BUS_FMT_AHSV8888_1X32, > > - MEDIA_BUS_FMT_AYUV8_1X32, > > - }; > > - > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > - ARRAY_SIZE(codes)); > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes, > > + ARRAY_SIZE(clu_codes)); > > > > } > > > > static int clu_enum_frame_size(struct v4l2_subdev *subdev, > > > > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev > > *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_format *fmt) > > > > { > > > > - struct vsp1_clu *clu = to_clu(subdev); > > - struct v4l2_subdev_pad_config *config; > > - struct v4l2_mbus_framefmt *format; > > - int ret = 0; > > - > > - mutex_lock(&clu->entity.lock); > > - > > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which); > > - if (!config) { > > - ret = -EINVAL; > > - goto done; > > - } > > - > > - /* Default to YUV if the requested format is not supported. */ > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > - > > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad); > > - > > - if (fmt->pad == CLU_PAD_SOURCE) { > > - /* The CLU output format can't be modified. */ > > - fmt->format = *format; > > - goto done; > > - } > > - > > - format->code = fmt->format.code; > > - format->width = clamp_t(unsigned int, fmt->format.width, > > - CLU_MIN_SIZE, CLU_MAX_SIZE); > > - format->height = clamp_t(unsigned int, fmt->format.height, > > - CLU_MIN_SIZE, CLU_MAX_SIZE); > > - format->field = V4L2_FIELD_NONE; > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > - > > - fmt->format = *format; > > - > > - /* Propagate the format to the source pad. */ > > - format = vsp1_entity_get_pad_format(&clu->entity, config, > > - CLU_PAD_SOURCE); > > - *format = fmt->format; > > - > > -done: > > - mutex_unlock(&clu->entity.lock); > > - return ret; > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, > > + ARRAY_SIZE(clu_codes), > > + CLU_MIN_SIZE, CLU_MIN_SIZE, > > + CLU_MAX_SIZE, CLU_MAX_SIZE); > > > > } > > > > /* > > ------------------------------------------------------------------------ > > -----> > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c > > b/drivers/media/platform/vsp1/vsp1_entity.c index > > 72354caf5746..239df047efd0 100644 > > --- a/drivers/media/platform/vsp1/vsp1_entity.c > > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > > @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev > > *subdev,> > > return ret; > > > > } > > > > +/* > > + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler > > + * @subdev: V4L2 subdevice > > + * @cfg: V4L2 subdev pad configuration > > + * @fmt: V4L2 subdev format > > + * @codes: Array of supported media bus codes > > + * @ncodes: Number of supported media bus codes > > + * @min_width: Minimum image width > > + * @min_height: Minimum image height > > + * @max_width: Maximum image width > > + * @max_height: Maximum image height > > + * > > + * This function implements the subdev set_fmt pad operation for entities > > that > > + * do not support scaling or cropping. It defaults to the first supplied > > media > > + * bus code if the requested code isn't supported, clamps the size to the > > + * supplied minimum and maximum, and propagates the sink pad format to > > the > > + * source pad. > > + */ > > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt, > > + const unsigned int *codes, unsigned int ncodes, > > + unsigned int min_width, unsigned int min_height, > > + unsigned int max_width, unsigned int max_height) > > +{ > > + struct vsp1_entity *entity = to_vsp1_entity(subdev); > > + struct v4l2_subdev_pad_config *config; > > + struct v4l2_mbus_framefmt *format; > > + unsigned int i; > > + int ret = 0; > > + > > + mutex_lock(&entity->lock); > > + > > + config = vsp1_entity_get_pad_config(entity, cfg, fmt->which); > > + if (!config) { > > + ret = -EINVAL; > > + goto done; > > + } > > + > > + format = vsp1_entity_get_pad_format(entity, config, fmt->pad); > > + > > + if (fmt->pad != 0) { > > I guess we don't have any clear way to say != *_PAD_SINK here do we .. > > > + /* The output format can't be modified. */ > > + fmt->format = *format; > > + goto done; > > + } > > + > > + /* > > + * Default to the first media bus code if the requested format is not > > + * supported. > > + */ > > + for (i = 0; i < ncodes; ++i) { > > + if (fmt->format.code == codes[i]) > > + break; > > + } > > + > > + format->code = i < ncodes ? codes[i] : codes[0]; > > + format->width = clamp_t(unsigned int, fmt->format.width, > > + min_width, max_width); > > + format->height = clamp_t(unsigned int, fmt->format.height, > > + min_height, max_height); > > + format->field = V4L2_FIELD_NONE; > > + format->colorspace = V4L2_COLORSPACE_SRGB; > > + > > + fmt->format = *format; > > + > > + /* Propagate the format to the source pad. */ > > + format = vsp1_entity_get_pad_format(entity, config, 1); > > If we can guarantee that ENTITY_PAD_SINK == 0 and ENTITY_PAD_SOURCE == 1, > can we put those into an enum ? > > > + *format = fmt->format; > > + > > +done: > > + mutex_unlock(&entity->lock); > > + return ret; > > +} > > + > > > > /* > > ------------------------------------------------------------------------ > > -----> > > * Media Operations > > */ > > > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.h > > b/drivers/media/platform/vsp1/vsp1_entity.h index > > fb20a1578f3b..0839a62cfa71 100644 > > --- a/drivers/media/platform/vsp1/vsp1_entity.h > > +++ b/drivers/media/platform/vsp1/vsp1_entity.h > > @@ -160,6 +160,12 @@ struct media_pad *vsp1_entity_remote_pad(struct > > media_pad *pad);> > > int vsp1_subdev_get_pad_format(struct v4l2_subdev *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_format *fmt); > > > > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt, > > + const unsigned int *codes, unsigned int ncodes, > > + unsigned int min_width, unsigned int min_height, > > + unsigned int max_width, unsigned int max_height); > > > > int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code, > > > > diff --git a/drivers/media/platform/vsp1/vsp1_lif.c > > b/drivers/media/platform/vsp1/vsp1_lif.c index b20b842f06ba..fbdd5715f829 > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_lif.c > > +++ b/drivers/media/platform/vsp1/vsp1_lif.c > > @@ -33,17 +33,17 @@ static inline void vsp1_lif_write(struct vsp1_lif > > *lif, struct vsp1_dl_list *dl,> > > * V4L2 Subdevice Operations > > */ > > > > +static const unsigned int lif_codes[] = { > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > + MEDIA_BUS_FMT_AYUV8_1X32, > > +}; > > + > > > > static int lif_enum_mbus_code(struct v4l2_subdev *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code) > > > > { > > > > - static const unsigned int codes[] = { > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > - MEDIA_BUS_FMT_AYUV8_1X32, > > - }; > > - > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > - ARRAY_SIZE(codes)); > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lif_codes, > > + ARRAY_SIZE(lif_codes)); > > > > } > > > > static int lif_enum_frame_size(struct v4l2_subdev *subdev, > > > > @@ -59,53 +59,10 @@ static int lif_set_format(struct v4l2_subdev *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_format *fmt) > > > > { > > > > - struct vsp1_lif *lif = to_lif(subdev); > > - struct v4l2_subdev_pad_config *config; > > - struct v4l2_mbus_framefmt *format; > > - int ret = 0; > > - > > - mutex_lock(&lif->entity.lock); > > - > > - config = vsp1_entity_get_pad_config(&lif->entity, cfg, fmt->which); > > - if (!config) { > > - ret = -EINVAL; > > - goto done; > > - } > > - > > - /* Default to YUV if the requested format is not supported. */ > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > - > > - format = vsp1_entity_get_pad_format(&lif->entity, config, fmt->pad); > > - > > - if (fmt->pad == LIF_PAD_SOURCE) { > > - /* > > - * The LIF source format is always identical to its sink > > - * format. > > - */ > > - fmt->format = *format; > > - goto done; > > - } > > - > > - format->code = fmt->format.code; > > - format->width = clamp_t(unsigned int, fmt->format.width, > > - LIF_MIN_SIZE, LIF_MAX_SIZE); > > - format->height = clamp_t(unsigned int, fmt->format.height, > > - LIF_MIN_SIZE, LIF_MAX_SIZE); > > - format->field = V4L2_FIELD_NONE; > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > - > > - fmt->format = *format; > > - > > - /* Propagate the format to the source pad. */ > > - format = vsp1_entity_get_pad_format(&lif->entity, config, > > - LIF_PAD_SOURCE); > > - *format = fmt->format; > > - > > -done: > > - mutex_unlock(&lif->entity.lock); > > - return ret; > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lif_codes, > > + ARRAY_SIZE(lif_codes), > > + LIF_MIN_SIZE, LIF_MIN_SIZE, > > + LIF_MAX_SIZE, LIF_MAX_SIZE); > > > > } > > > > static const struct v4l2_subdev_pad_ops lif_pad_ops = { > > > > diff --git a/drivers/media/platform/vsp1/vsp1_lut.c > > b/drivers/media/platform/vsp1/vsp1_lut.c index 7bdabb311c6c..f2e48a02ca7d > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_lut.c > > +++ b/drivers/media/platform/vsp1/vsp1_lut.c > > @@ -90,18 +90,18 @@ static const struct v4l2_ctrl_config lut_table_control > > = {> > > * V4L2 Subdevice Pad Operations > > */ > > > > +static const unsigned int lut_codes[] = { > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > + MEDIA_BUS_FMT_AHSV8888_1X32, > > + MEDIA_BUS_FMT_AYUV8_1X32, > > +}; > > + > > > > static int lut_enum_mbus_code(struct v4l2_subdev *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code) > > > > { > > > > - static const unsigned int codes[] = { > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > - MEDIA_BUS_FMT_AHSV8888_1X32, > > - MEDIA_BUS_FMT_AYUV8_1X32, > > - }; > > - > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > - ARRAY_SIZE(codes)); > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lut_codes, > > + ARRAY_SIZE(lut_codes)); > > > > } > > > > static int lut_enum_frame_size(struct v4l2_subdev *subdev, > > > > @@ -117,51 +117,10 @@ static int lut_set_format(struct v4l2_subdev > > *subdev, > > > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_format *fmt) > > > > { > > > > - struct vsp1_lut *lut = to_lut(subdev); > > - struct v4l2_subdev_pad_config *config; > > - struct v4l2_mbus_framefmt *format; > > - int ret = 0; > > - > > - mutex_lock(&lut->entity.lock); > > - > > - config = vsp1_entity_get_pad_config(&lut->entity, cfg, fmt->which); > > - if (!config) { > > - ret = -EINVAL; > > - goto done; > > - } > > - > > - /* Default to YUV if the requested format is not supported. */ > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > - > > - format = vsp1_entity_get_pad_format(&lut->entity, config, fmt->pad); > > - > > - if (fmt->pad == LUT_PAD_SOURCE) { > > - /* The LUT output format can't be modified. */ > > - fmt->format = *format; > > - goto done; > > - } > > - > > - format->code = fmt->format.code; > > - format->width = clamp_t(unsigned int, fmt->format.width, > > - LUT_MIN_SIZE, LUT_MAX_SIZE); > > - format->height = clamp_t(unsigned int, fmt->format.height, > > - LUT_MIN_SIZE, LUT_MAX_SIZE); > > - format->field = V4L2_FIELD_NONE; > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > - > > - fmt->format = *format; > > - > > - /* Propagate the format to the source pad. */ > > - format = vsp1_entity_get_pad_format(&lut->entity, config, > > - LUT_PAD_SOURCE); > > - *format = fmt->format; > > - > > -done: > > - mutex_unlock(&lut->entity.lock); > > - return ret; > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lut_codes, > > + ARRAY_SIZE(lut_codes), > > + LUT_MIN_SIZE, LUT_MIN_SIZE, > > + LUT_MAX_SIZE, LUT_MAX_SIZE); > > > > } > > > > /* > > ------------------------------------------------------------------------ > > -----
Hi again, On Saturday, 28 April 2018 20:25:44 EEST Laurent Pinchart wrote: > On Saturday, 28 April 2018 20:16:11 EEST Kieran Bingham wrote: > > On 22/04/18 23:34, Laurent Pinchart wrote: > > > The implementation of the set_fmt pad operation is identical in the > > > three modules. Move it to a generic helper function. > > > > > > Signed-off-by: Laurent Pinchart > > > <laurent.pinchart+renesas@ideasonboard.com> > > > > Only a minor pair of comments below regarding source/sink pad > > descriptions. > > > > If it's not convenient/accurate to define these with an enum then don't > > worry about it. > > It's a good point. There are however other locations in vsp1_entity.c that > hardcode pad numbers, so I'll submit a patch on top of this series to fix > them all in one go. Actually I can compare the pad number to entity->source_pad, I'll update this patch accordingly in v3. > > Otherwise, > > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > > > --- > > > > > > drivers/media/platform/vsp1/vsp1_clu.c | 65 +++++------------------- > > > drivers/media/platform/vsp1/vsp1_entity.c | 75 > > > ++++++++++++++++++++++++++ > > > drivers/media/platform/vsp1/vsp1_entity.h | 6 +++ > > > drivers/media/platform/vsp1/vsp1_lif.c | 65 > > > +++++-------------------- > > > drivers/media/platform/vsp1/vsp1_lut.c | 65 > > > +++++-------------------- > > > 5 files changed, 116 insertions(+), 160 deletions(-) > > > > That's a nice diffstat :-) > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c > > > b/drivers/media/platform/vsp1/vsp1_clu.c index > > > 9626b6308585..96a448e1504c > > > 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_clu.c > > > +++ b/drivers/media/platform/vsp1/vsp1_clu.c > > > @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config > > > clu_mode_control = {> > > > > > > * V4L2 Subdevice Pad Operations > > > */ > > > > > > +static const unsigned int clu_codes[] = { > > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > > + MEDIA_BUS_FMT_AHSV8888_1X32, > > > + MEDIA_BUS_FMT_AYUV8_1X32, > > > +}; > > > + > > > > > > static int clu_enum_mbus_code(struct v4l2_subdev *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_mbus_code_enum *code) > > > > > > { > > > > > > - static const unsigned int codes[] = { > > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > > - MEDIA_BUS_FMT_AHSV8888_1X32, > > > - MEDIA_BUS_FMT_AYUV8_1X32, > > > - }; > > > - > > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > > - ARRAY_SIZE(codes)); > > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes, > > > + ARRAY_SIZE(clu_codes)); > > > > > > } > > > > > > static int clu_enum_frame_size(struct v4l2_subdev *subdev, > > > > > > @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev > > > *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_format *fmt) > > > > > > { > > > > > > - struct vsp1_clu *clu = to_clu(subdev); > > > - struct v4l2_subdev_pad_config *config; > > > - struct v4l2_mbus_framefmt *format; > > > - int ret = 0; > > > - > > > - mutex_lock(&clu->entity.lock); > > > - > > > - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which); > > > - if (!config) { > > > - ret = -EINVAL; > > > - goto done; > > > - } > > > - > > > - /* Default to YUV if the requested format is not supported. */ > > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > > - > > > - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad); > > > - > > > - if (fmt->pad == CLU_PAD_SOURCE) { > > > - /* The CLU output format can't be modified. */ > > > - fmt->format = *format; > > > - goto done; > > > - } > > > - > > > - format->code = fmt->format.code; > > > - format->width = clamp_t(unsigned int, fmt->format.width, > > > - CLU_MIN_SIZE, CLU_MAX_SIZE); > > > - format->height = clamp_t(unsigned int, fmt->format.height, > > > - CLU_MIN_SIZE, CLU_MAX_SIZE); > > > - format->field = V4L2_FIELD_NONE; > > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > > - > > > - fmt->format = *format; > > > - > > > - /* Propagate the format to the source pad. */ > > > - format = vsp1_entity_get_pad_format(&clu->entity, config, > > > - CLU_PAD_SOURCE); > > > - *format = fmt->format; > > > - > > > -done: > > > - mutex_unlock(&clu->entity.lock); > > > - return ret; > > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, > > > + ARRAY_SIZE(clu_codes), > > > + CLU_MIN_SIZE, CLU_MIN_SIZE, > > > + CLU_MAX_SIZE, CLU_MAX_SIZE); > > > > > > } > > > > > > /* > > > ----------------------------------------------------------------------- > > > - > > > -----> > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c > > > b/drivers/media/platform/vsp1/vsp1_entity.c index > > > 72354caf5746..239df047efd0 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_entity.c > > > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > > > @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev > > > *subdev,> > > > > > > return ret; > > > > > > } > > > > > > +/* > > > + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler > > > + * @subdev: V4L2 subdevice > > > + * @cfg: V4L2 subdev pad configuration > > > + * @fmt: V4L2 subdev format > > > + * @codes: Array of supported media bus codes > > > + * @ncodes: Number of supported media bus codes > > > + * @min_width: Minimum image width > > > + * @min_height: Minimum image height > > > + * @max_width: Maximum image width > > > + * @max_height: Maximum image height > > > + * > > > + * This function implements the subdev set_fmt pad operation for > > > entities > > > that > > > + * do not support scaling or cropping. It defaults to the first > > > supplied > > > media > > > + * bus code if the requested code isn't supported, clamps the size to > > > the > > > + * supplied minimum and maximum, and propagates the sink pad format to > > > the > > > + * source pad. > > > + */ > > > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > > > + struct v4l2_subdev_pad_config *cfg, > > > + struct v4l2_subdev_format *fmt, > > > + const unsigned int *codes, unsigned int ncodes, > > > + unsigned int min_width, unsigned int min_height, > > > + unsigned int max_width, unsigned int max_height) > > > +{ > > > + struct vsp1_entity *entity = to_vsp1_entity(subdev); > > > + struct v4l2_subdev_pad_config *config; > > > + struct v4l2_mbus_framefmt *format; > > > + unsigned int i; > > > + int ret = 0; > > > + > > > + mutex_lock(&entity->lock); > > > + > > > + config = vsp1_entity_get_pad_config(entity, cfg, fmt->which); > > > + if (!config) { > > > + ret = -EINVAL; > > > + goto done; > > > + } > > > + > > > + format = vsp1_entity_get_pad_format(entity, config, fmt->pad); > > > + > > > + if (fmt->pad != 0) { > > > > I guess we don't have any clear way to say != *_PAD_SINK here do we .. > > > > > + /* The output format can't be modified. */ > > > + fmt->format = *format; > > > + goto done; > > > + } > > > + > > > + /* > > > + * Default to the first media bus code if the requested format is not > > > + * supported. > > > + */ > > > + for (i = 0; i < ncodes; ++i) { > > > + if (fmt->format.code == codes[i]) > > > + break; > > > + } > > > + > > > + format->code = i < ncodes ? codes[i] : codes[0]; > > > + format->width = clamp_t(unsigned int, fmt->format.width, > > > + min_width, max_width); > > > + format->height = clamp_t(unsigned int, fmt->format.height, > > > + min_height, max_height); > > > + format->field = V4L2_FIELD_NONE; > > > + format->colorspace = V4L2_COLORSPACE_SRGB; > > > + > > > + fmt->format = *format; > > > + > > > + /* Propagate the format to the source pad. */ > > > + format = vsp1_entity_get_pad_format(entity, config, 1); > > > > If we can guarantee that ENTITY_PAD_SINK == 0 and ENTITY_PAD_SOURCE == 1, > > can we put those into an enum ? > > > > > + *format = fmt->format; > > > + > > > +done: > > > + mutex_unlock(&entity->lock); > > > + return ret; > > > +} > > > + > > > > > > /* > > > ----------------------------------------------------------------------- > > > - > > > -----> > > > > > > * Media Operations > > > */ > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.h > > > b/drivers/media/platform/vsp1/vsp1_entity.h index > > > fb20a1578f3b..0839a62cfa71 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_entity.h > > > +++ b/drivers/media/platform/vsp1/vsp1_entity.h > > > @@ -160,6 +160,12 @@ struct media_pad *vsp1_entity_remote_pad(struct > > > media_pad *pad);> > > > > > > int vsp1_subdev_get_pad_format(struct v4l2_subdev *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_format *fmt); > > > > > > +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > > > + struct v4l2_subdev_pad_config *cfg, > > > + struct v4l2_subdev_format *fmt, > > > + const unsigned int *codes, unsigned int ncodes, > > > + unsigned int min_width, unsigned int min_height, > > > + unsigned int max_width, unsigned int max_height); > > > > > > int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_mbus_code_enum *code, > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_lif.c > > > b/drivers/media/platform/vsp1/vsp1_lif.c index > > > b20b842f06ba..fbdd5715f829 > > > 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_lif.c > > > +++ b/drivers/media/platform/vsp1/vsp1_lif.c > > > @@ -33,17 +33,17 @@ static inline void vsp1_lif_write(struct vsp1_lif > > > *lif, struct vsp1_dl_list *dl,> > > > > > > * V4L2 Subdevice Operations > > > */ > > > > > > +static const unsigned int lif_codes[] = { > > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > > + MEDIA_BUS_FMT_AYUV8_1X32, > > > +}; > > > + > > > > > > static int lif_enum_mbus_code(struct v4l2_subdev *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_mbus_code_enum *code) > > > > > > { > > > > > > - static const unsigned int codes[] = { > > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > > - MEDIA_BUS_FMT_AYUV8_1X32, > > > - }; > > > - > > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > > - ARRAY_SIZE(codes)); > > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lif_codes, > > > + ARRAY_SIZE(lif_codes)); > > > > > > } > > > > > > static int lif_enum_frame_size(struct v4l2_subdev *subdev, > > > > > > @@ -59,53 +59,10 @@ static int lif_set_format(struct v4l2_subdev > > > *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_format *fmt) > > > > > > { > > > > > > - struct vsp1_lif *lif = to_lif(subdev); > > > - struct v4l2_subdev_pad_config *config; > > > - struct v4l2_mbus_framefmt *format; > > > - int ret = 0; > > > - > > > - mutex_lock(&lif->entity.lock); > > > - > > > - config = vsp1_entity_get_pad_config(&lif->entity, cfg, fmt->which); > > > - if (!config) { > > > - ret = -EINVAL; > > > - goto done; > > > - } > > > - > > > - /* Default to YUV if the requested format is not supported. */ > > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > > - > > > - format = vsp1_entity_get_pad_format(&lif->entity, config, fmt->pad); > > > - > > > - if (fmt->pad == LIF_PAD_SOURCE) { > > > - /* > > > - * The LIF source format is always identical to its sink > > > - * format. > > > - */ > > > - fmt->format = *format; > > > - goto done; > > > - } > > > - > > > - format->code = fmt->format.code; > > > - format->width = clamp_t(unsigned int, fmt->format.width, > > > - LIF_MIN_SIZE, LIF_MAX_SIZE); > > > - format->height = clamp_t(unsigned int, fmt->format.height, > > > - LIF_MIN_SIZE, LIF_MAX_SIZE); > > > - format->field = V4L2_FIELD_NONE; > > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > > - > > > - fmt->format = *format; > > > - > > > - /* Propagate the format to the source pad. */ > > > - format = vsp1_entity_get_pad_format(&lif->entity, config, > > > - LIF_PAD_SOURCE); > > > - *format = fmt->format; > > > - > > > -done: > > > - mutex_unlock(&lif->entity.lock); > > > - return ret; > > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lif_codes, > > > + ARRAY_SIZE(lif_codes), > > > + LIF_MIN_SIZE, LIF_MIN_SIZE, > > > + LIF_MAX_SIZE, LIF_MAX_SIZE); > > > > > > } > > > > > > static const struct v4l2_subdev_pad_ops lif_pad_ops = { > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_lut.c > > > b/drivers/media/platform/vsp1/vsp1_lut.c index > > > 7bdabb311c6c..f2e48a02ca7d > > > 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_lut.c > > > +++ b/drivers/media/platform/vsp1/vsp1_lut.c > > > @@ -90,18 +90,18 @@ static const struct v4l2_ctrl_config > > > lut_table_control > > > = {> > > > > > > * V4L2 Subdevice Pad Operations > > > */ > > > > > > +static const unsigned int lut_codes[] = { > > > + MEDIA_BUS_FMT_ARGB8888_1X32, > > > + MEDIA_BUS_FMT_AHSV8888_1X32, > > > + MEDIA_BUS_FMT_AYUV8_1X32, > > > +}; > > > + > > > > > > static int lut_enum_mbus_code(struct v4l2_subdev *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_mbus_code_enum *code) > > > > > > { > > > > > > - static const unsigned int codes[] = { > > > - MEDIA_BUS_FMT_ARGB8888_1X32, > > > - MEDIA_BUS_FMT_AHSV8888_1X32, > > > - MEDIA_BUS_FMT_AYUV8_1X32, > > > - }; > > > - > > > - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, > > > - ARRAY_SIZE(codes)); > > > + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lut_codes, > > > + ARRAY_SIZE(lut_codes)); > > > > > > } > > > > > > static int lut_enum_frame_size(struct v4l2_subdev *subdev, > > > > > > @@ -117,51 +117,10 @@ static int lut_set_format(struct v4l2_subdev > > > *subdev, > > > > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_format *fmt) > > > > > > { > > > > > > - struct vsp1_lut *lut = to_lut(subdev); > > > - struct v4l2_subdev_pad_config *config; > > > - struct v4l2_mbus_framefmt *format; > > > - int ret = 0; > > > - > > > - mutex_lock(&lut->entity.lock); > > > - > > > - config = vsp1_entity_get_pad_config(&lut->entity, cfg, fmt->which); > > > - if (!config) { > > > - ret = -EINVAL; > > > - goto done; > > > - } > > > - > > > - /* Default to YUV if the requested format is not supported. */ > > > - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > > - > > > - format = vsp1_entity_get_pad_format(&lut->entity, config, fmt->pad); > > > - > > > - if (fmt->pad == LUT_PAD_SOURCE) { > > > - /* The LUT output format can't be modified. */ > > > - fmt->format = *format; > > > - goto done; > > > - } > > > - > > > - format->code = fmt->format.code; > > > - format->width = clamp_t(unsigned int, fmt->format.width, > > > - LUT_MIN_SIZE, LUT_MAX_SIZE); > > > - format->height = clamp_t(unsigned int, fmt->format.height, > > > - LUT_MIN_SIZE, LUT_MAX_SIZE); > > > - format->field = V4L2_FIELD_NONE; > > > - format->colorspace = V4L2_COLORSPACE_SRGB; > > > - > > > - fmt->format = *format; > > > - > > > - /* Propagate the format to the source pad. */ > > > - format = vsp1_entity_get_pad_format(&lut->entity, config, > > > - LUT_PAD_SOURCE); > > > - *format = fmt->format; > > > - > > > -done: > > > - mutex_unlock(&lut->entity.lock); > > > - return ret; > > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lut_codes, > > > + ARRAY_SIZE(lut_codes), > > > + LUT_MIN_SIZE, LUT_MIN_SIZE, > > > + LUT_MAX_SIZE, LUT_MAX_SIZE); > > > > > > } > > > > > > /* > > > ----------------------------------------------------------------------- > > > - > > > -----
On 28/04/18 18:30, Laurent Pinchart wrote: > Hi again, > > On Saturday, 28 April 2018 20:25:44 EEST Laurent Pinchart wrote: >> On Saturday, 28 April 2018 20:16:11 EEST Kieran Bingham wrote: >>> On 22/04/18 23:34, Laurent Pinchart wrote: >>>> The implementation of the set_fmt pad operation is identical in the >>>> three modules. Move it to a generic helper function. >>>> >>>> Signed-off-by: Laurent Pinchart >>>> <laurent.pinchart+renesas@ideasonboard.com> >>> >>> Only a minor pair of comments below regarding source/sink pad >>> descriptions. >>> >>> If it's not convenient/accurate to define these with an enum then don't >>> worry about it. >> >> It's a good point. There are however other locations in vsp1_entity.c that >> hardcode pad numbers, so I'll submit a patch on top of this series to fix >> them all in one go. > > Actually I can compare the pad number to entity->source_pad, I'll update this > patch accordingly in v3. Perfect, that sounds more explicit, easier to read, and future proof against entities with multiple sinks, such as the BRx. -- Kieran
diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c index 9626b6308585..96a448e1504c 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.c +++ b/drivers/media/platform/vsp1/vsp1_clu.c @@ -114,18 +114,18 @@ static const struct v4l2_ctrl_config clu_mode_control = { * V4L2 Subdevice Pad Operations */ +static const unsigned int clu_codes[] = { + MEDIA_BUS_FMT_ARGB8888_1X32, + MEDIA_BUS_FMT_AHSV8888_1X32, + MEDIA_BUS_FMT_AYUV8_1X32, +}; + static int clu_enum_mbus_code(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - static const unsigned int codes[] = { - MEDIA_BUS_FMT_ARGB8888_1X32, - MEDIA_BUS_FMT_AHSV8888_1X32, - MEDIA_BUS_FMT_AYUV8_1X32, - }; - - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, - ARRAY_SIZE(codes)); + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, clu_codes, + ARRAY_SIZE(clu_codes)); } static int clu_enum_frame_size(struct v4l2_subdev *subdev, @@ -141,51 +141,10 @@ static int clu_set_format(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *fmt) { - struct vsp1_clu *clu = to_clu(subdev); - struct v4l2_subdev_pad_config *config; - struct v4l2_mbus_framefmt *format; - int ret = 0; - - mutex_lock(&clu->entity.lock); - - config = vsp1_entity_get_pad_config(&clu->entity, cfg, fmt->which); - if (!config) { - ret = -EINVAL; - goto done; - } - - /* Default to YUV if the requested format is not supported. */ - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; - - format = vsp1_entity_get_pad_format(&clu->entity, config, fmt->pad); - - if (fmt->pad == CLU_PAD_SOURCE) { - /* The CLU output format can't be modified. */ - fmt->format = *format; - goto done; - } - - format->code = fmt->format.code; - format->width = clamp_t(unsigned int, fmt->format.width, - CLU_MIN_SIZE, CLU_MAX_SIZE); - format->height = clamp_t(unsigned int, fmt->format.height, - CLU_MIN_SIZE, CLU_MAX_SIZE); - format->field = V4L2_FIELD_NONE; - format->colorspace = V4L2_COLORSPACE_SRGB; - - fmt->format = *format; - - /* Propagate the format to the source pad. */ - format = vsp1_entity_get_pad_format(&clu->entity, config, - CLU_PAD_SOURCE); - *format = fmt->format; - -done: - mutex_unlock(&clu->entity.lock); - return ret; + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, + ARRAY_SIZE(clu_codes), + CLU_MIN_SIZE, CLU_MIN_SIZE, + CLU_MAX_SIZE, CLU_MAX_SIZE); } /* ----------------------------------------------------------------------------- diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 72354caf5746..239df047efd0 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -307,6 +307,81 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev, return ret; } +/* + * vsp1_subdev_set_pad_format - Subdev pad set_fmt handler + * @subdev: V4L2 subdevice + * @cfg: V4L2 subdev pad configuration + * @fmt: V4L2 subdev format + * @codes: Array of supported media bus codes + * @ncodes: Number of supported media bus codes + * @min_width: Minimum image width + * @min_height: Minimum image height + * @max_width: Maximum image width + * @max_height: Maximum image height + * + * This function implements the subdev set_fmt pad operation for entities that + * do not support scaling or cropping. It defaults to the first supplied media + * bus code if the requested code isn't supported, clamps the size to the + * supplied minimum and maximum, and propagates the sink pad format to the + * source pad. + */ +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt, + const unsigned int *codes, unsigned int ncodes, + unsigned int min_width, unsigned int min_height, + unsigned int max_width, unsigned int max_height) +{ + struct vsp1_entity *entity = to_vsp1_entity(subdev); + struct v4l2_subdev_pad_config *config; + struct v4l2_mbus_framefmt *format; + unsigned int i; + int ret = 0; + + mutex_lock(&entity->lock); + + config = vsp1_entity_get_pad_config(entity, cfg, fmt->which); + if (!config) { + ret = -EINVAL; + goto done; + } + + format = vsp1_entity_get_pad_format(entity, config, fmt->pad); + + if (fmt->pad != 0) { + /* The output format can't be modified. */ + fmt->format = *format; + goto done; + } + + /* + * Default to the first media bus code if the requested format is not + * supported. + */ + for (i = 0; i < ncodes; ++i) { + if (fmt->format.code == codes[i]) + break; + } + + format->code = i < ncodes ? codes[i] : codes[0]; + format->width = clamp_t(unsigned int, fmt->format.width, + min_width, max_width); + format->height = clamp_t(unsigned int, fmt->format.height, + min_height, max_height); + format->field = V4L2_FIELD_NONE; + format->colorspace = V4L2_COLORSPACE_SRGB; + + fmt->format = *format; + + /* Propagate the format to the source pad. */ + format = vsp1_entity_get_pad_format(entity, config, 1); + *format = fmt->format; + +done: + mutex_unlock(&entity->lock); + return ret; +} + /* ----------------------------------------------------------------------------- * Media Operations */ diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h index fb20a1578f3b..0839a62cfa71 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.h +++ b/drivers/media/platform/vsp1/vsp1_entity.h @@ -160,6 +160,12 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad); int vsp1_subdev_get_pad_format(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *fmt); +int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt, + const unsigned int *codes, unsigned int ncodes, + unsigned int min_width, unsigned int min_height, + unsigned int max_width, unsigned int max_height); int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code, diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c index b20b842f06ba..fbdd5715f829 100644 --- a/drivers/media/platform/vsp1/vsp1_lif.c +++ b/drivers/media/platform/vsp1/vsp1_lif.c @@ -33,17 +33,17 @@ static inline void vsp1_lif_write(struct vsp1_lif *lif, struct vsp1_dl_list *dl, * V4L2 Subdevice Operations */ +static const unsigned int lif_codes[] = { + MEDIA_BUS_FMT_ARGB8888_1X32, + MEDIA_BUS_FMT_AYUV8_1X32, +}; + static int lif_enum_mbus_code(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - static const unsigned int codes[] = { - MEDIA_BUS_FMT_ARGB8888_1X32, - MEDIA_BUS_FMT_AYUV8_1X32, - }; - - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, - ARRAY_SIZE(codes)); + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lif_codes, + ARRAY_SIZE(lif_codes)); } static int lif_enum_frame_size(struct v4l2_subdev *subdev, @@ -59,53 +59,10 @@ static int lif_set_format(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *fmt) { - struct vsp1_lif *lif = to_lif(subdev); - struct v4l2_subdev_pad_config *config; - struct v4l2_mbus_framefmt *format; - int ret = 0; - - mutex_lock(&lif->entity.lock); - - config = vsp1_entity_get_pad_config(&lif->entity, cfg, fmt->which); - if (!config) { - ret = -EINVAL; - goto done; - } - - /* Default to YUV if the requested format is not supported. */ - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; - - format = vsp1_entity_get_pad_format(&lif->entity, config, fmt->pad); - - if (fmt->pad == LIF_PAD_SOURCE) { - /* - * The LIF source format is always identical to its sink - * format. - */ - fmt->format = *format; - goto done; - } - - format->code = fmt->format.code; - format->width = clamp_t(unsigned int, fmt->format.width, - LIF_MIN_SIZE, LIF_MAX_SIZE); - format->height = clamp_t(unsigned int, fmt->format.height, - LIF_MIN_SIZE, LIF_MAX_SIZE); - format->field = V4L2_FIELD_NONE; - format->colorspace = V4L2_COLORSPACE_SRGB; - - fmt->format = *format; - - /* Propagate the format to the source pad. */ - format = vsp1_entity_get_pad_format(&lif->entity, config, - LIF_PAD_SOURCE); - *format = fmt->format; - -done: - mutex_unlock(&lif->entity.lock); - return ret; + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lif_codes, + ARRAY_SIZE(lif_codes), + LIF_MIN_SIZE, LIF_MIN_SIZE, + LIF_MAX_SIZE, LIF_MAX_SIZE); } static const struct v4l2_subdev_pad_ops lif_pad_ops = { diff --git a/drivers/media/platform/vsp1/vsp1_lut.c b/drivers/media/platform/vsp1/vsp1_lut.c index 7bdabb311c6c..f2e48a02ca7d 100644 --- a/drivers/media/platform/vsp1/vsp1_lut.c +++ b/drivers/media/platform/vsp1/vsp1_lut.c @@ -90,18 +90,18 @@ static const struct v4l2_ctrl_config lut_table_control = { * V4L2 Subdevice Pad Operations */ +static const unsigned int lut_codes[] = { + MEDIA_BUS_FMT_ARGB8888_1X32, + MEDIA_BUS_FMT_AHSV8888_1X32, + MEDIA_BUS_FMT_AYUV8_1X32, +}; + static int lut_enum_mbus_code(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - static const unsigned int codes[] = { - MEDIA_BUS_FMT_ARGB8888_1X32, - MEDIA_BUS_FMT_AHSV8888_1X32, - MEDIA_BUS_FMT_AYUV8_1X32, - }; - - return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, - ARRAY_SIZE(codes)); + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, lut_codes, + ARRAY_SIZE(lut_codes)); } static int lut_enum_frame_size(struct v4l2_subdev *subdev, @@ -117,51 +117,10 @@ static int lut_set_format(struct v4l2_subdev *subdev, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *fmt) { - struct vsp1_lut *lut = to_lut(subdev); - struct v4l2_subdev_pad_config *config; - struct v4l2_mbus_framefmt *format; - int ret = 0; - - mutex_lock(&lut->entity.lock); - - config = vsp1_entity_get_pad_config(&lut->entity, cfg, fmt->which); - if (!config) { - ret = -EINVAL; - goto done; - } - - /* Default to YUV if the requested format is not supported. */ - if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; - - format = vsp1_entity_get_pad_format(&lut->entity, config, fmt->pad); - - if (fmt->pad == LUT_PAD_SOURCE) { - /* The LUT output format can't be modified. */ - fmt->format = *format; - goto done; - } - - format->code = fmt->format.code; - format->width = clamp_t(unsigned int, fmt->format.width, - LUT_MIN_SIZE, LUT_MAX_SIZE); - format->height = clamp_t(unsigned int, fmt->format.height, - LUT_MIN_SIZE, LUT_MAX_SIZE); - format->field = V4L2_FIELD_NONE; - format->colorspace = V4L2_COLORSPACE_SRGB; - - fmt->format = *format; - - /* Propagate the format to the source pad. */ - format = vsp1_entity_get_pad_format(&lut->entity, config, - LUT_PAD_SOURCE); - *format = fmt->format; - -done: - mutex_unlock(&lut->entity.lock); - return ret; + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, lut_codes, + ARRAY_SIZE(lut_codes), + LUT_MIN_SIZE, LUT_MIN_SIZE, + LUT_MAX_SIZE, LUT_MAX_SIZE); } /* -----------------------------------------------------------------------------
The implementation of the set_fmt pad operation is identical in the three modules. Move it to a generic helper function. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/media/platform/vsp1/vsp1_clu.c | 65 +++++---------------------- drivers/media/platform/vsp1/vsp1_entity.c | 75 +++++++++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_entity.h | 6 +++ drivers/media/platform/vsp1/vsp1_lif.c | 65 +++++---------------------- drivers/media/platform/vsp1/vsp1_lut.c | 65 +++++---------------------- 5 files changed, 116 insertions(+), 160 deletions(-)