Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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>>> >>> 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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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 > > >> > > > 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 > > > > > --- > > > > > > 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_ARGB_1X32, > > > + MEDIA_BUS_FMT_AHSV_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_ARGB_1X32, > > > - MEDIA_BUS_FMT_AHSV_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(>entity.lock); > > > - > > > - config = vsp1_entity_get_pad_config(>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_ARGB_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 && > > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > > - > > > - format = vsp1_entity_get_pad_format(>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(>entity, config, > > > - CLU_PAD_SOURCE); > > > - *format = fmt->format; > > > - > > > -done: > > > - mutex_unlock(>entity.lock); > > > - return ret; > > > + return vsp1_subdev_set_pad_format(subdev, cfg, fmt, clu_codes, > > > + ARRAY_SIZE(clu_codes), > > > +
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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 > >> > 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 > > > --- > > > > 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_ARGB_1X32, > > + MEDIA_BUS_FMT_AHSV_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_ARGB_1X32, > > - MEDIA_BUS_FMT_AHSV_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(>entity.lock); > > - > > - config = vsp1_entity_get_pad_config(>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_ARGB_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > > - > > - format = vsp1_entity_get_pad_format(>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(>entity, config, > > - CLU_PAD_SOURCE); > > - *format = fmt->format; > > - > > -done: > > - mutex_unlock(>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 > >
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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 PinchartOnly 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 > --- > 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_ARGB_1X32, > + MEDIA_BUS_FMT_AHSV_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_ARGB_1X32, > - MEDIA_BUS_FMT_AHSV_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(>entity.lock); > - > - config = vsp1_entity_get_pad_config(>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_ARGB_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) > - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; > - > - format = vsp1_entity_get_pad_format(>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(>entity, config, > - CLU_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(>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
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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 > >> > --- > > > > 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_ARGB_1X32, > > + MEDIA_BUS_FMT_AHSV_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_ARGB_1X32, > > - MEDIA_BUS_FMT_AHSV_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(>entity.lock); > > - > > - config = vsp1_entity_get_pad_config(>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_ARGB_1X32 && > > - fmt->format.code != MEDIA_BUS_FMT_AHSV_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(>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(>entity, config, > > - CLU_PAD_SOURCE); > > - *format = fmt->format; > > - > > -done: > > - mutex_unlock(>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); > > } > > > > /*
Re: [PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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> --- > 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_ARGB_1X32, > + MEDIA_BUS_FMT_AHSV_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_ARGB_1X32, > - MEDIA_BUS_FMT_AHSV_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(>entity.lock); > - > - config = vsp1_entity_get_pad_config(>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_ARGB_1X32 && > - fmt->format.code != MEDIA_BUS_FMT_AHSV_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(>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(>entity, config, > - CLU_PAD_SOURCE); > - *format = fmt->format; > - > -done: > - mutex_unlock(>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
[PATCH v2 2/8] v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
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--- 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_ARGB_1X32, + MEDIA_BUS_FMT_AHSV_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_ARGB_1X32, - MEDIA_BUS_FMT_AHSV_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(>entity.lock); - - config = vsp1_entity_get_pad_config(>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_ARGB_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AHSV_1X32 && - fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32) - fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32; - - format = vsp1_entity_get_pad_format(>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(>entity, config, - CLU_PAD_SOURCE); - *format = fmt->format; - -done: - mutex_unlock(>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,