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 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 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);
  
  

}

/*

---

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:

To make vsp1_subdev_set_pad_format() usable by entities that support selection rectangles, we need to reset the crop and compose rectangles when setting the format on the sink pad. Do so and replace the custom set_fmt implementation of the histogram code by a call to vsp1_subdev_set_pad_format().

Resetting the crop and compose rectangles for entities that don't support crop and compose has no adverse effect as the rectangles are ignored anyway.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

This one looks fairly straight forward.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/media/platform/vsp1/vsp1_entity.c | 16 +++++++++ drivers/media/platform/vsp1/vsp1_histo.c | 59 +++---------------------------- 2 files changed, 20 insertions(+), 55 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 239df047efd0..181a583aecad 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -335,6 +335,7 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, struct vsp1_entity *entity = to_vsp1_entity(subdev); struct v4l2_subdev_pad_config *config; struct v4l2_mbus_framefmt *format;

• struct v4l2_rect *selection; unsigned int i; int ret = 0;

@@ -377,6 +378,21 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, format = vsp1_entity_get_pad_format(entity, config, 1); *format = fmt->format;

• /* Reset the crop and compose rectangles */
• selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,

• 				  V4L2_SEL_TGT_CROP);
  

• selection->left = 0;
• selection->top = 0;
• selection->width = format->width;
• selection->height = format->height;
• selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,

• 				  V4L2_SEL_TGT_COMPOSE);
  

• selection->left = 0;
• selection->top = 0;
• selection->width = format->width;
• selection->height = format->height;

done: mutex_unlock(&entity->lock); return ret; diff --git a/drivers/media/platform/vsp1/vsp1_histo.c b/drivers/media/platform/vsp1/vsp1_histo.c index 029181c1fb61..5e15c8ff88d9 100644 --- a/drivers/media/platform/vsp1/vsp1_histo.c +++ b/drivers/media/platform/vsp1/vsp1_histo.c @@ -389,65 +389,14 @@ static int histo_set_format(struct v4l2_subdev *subdev, struct v4l2_subdev_format *fmt) { struct vsp1_histogram *histo = subdev_to_histo(subdev);

• struct v4l2_subdev_pad_config *config;

• struct v4l2_mbus_framefmt *format;

• struct v4l2_rect *selection;

• unsigned int i;

• int ret = 0;

  if (fmt->pad != HISTO_PAD_SINK) return histo_get_format(subdev, cfg, fmt);

• mutex_lock(&histo->entity.lock);

• config = vsp1_entity_get_pad_config(&histo->entity, cfg, fmt->which);

• if (!config) {

• ret = -EINVAL;
  

• goto done;
  

• }

• /*

• * Default to the first format if the requested format is not
  

• * supported.
  

• */
  

• for (i = 0; i < histo->num_formats; ++i) {

• if (fmt->format.code == histo->formats[i])
  

• 	break;
  

• }

• if (i == histo->num_formats)

• fmt->format.code = histo->formats[0];
  

• format = vsp1_entity_get_pad_format(&histo->entity, config, fmt->pad);

• format->code = fmt->format.code;

• format->width = clamp_t(unsigned int, fmt->format.width,

• 		HISTO_MIN_SIZE, HISTO_MAX_SIZE);
  

• format->height = clamp_t(unsigned int, fmt->format.height,

• 		 HISTO_MIN_SIZE, HISTO_MAX_SIZE);
  

• format->field = V4L2_FIELD_NONE;

• format->colorspace = V4L2_COLORSPACE_SRGB;

• fmt->format = *format;

• /* Reset the crop and compose rectangles */

• selection = vsp1_entity_get_pad_selection(&histo->entity, config,

• 				  fmt->pad, V4L2_SEL_TGT_CROP);
  

• selection->left = 0;

• selection->top = 0;

• selection->width = format->width;

• selection->height = format->height;

• selection = vsp1_entity_get_pad_selection(&histo->entity, config,

• 				  fmt->pad,
  

• 				  V4L2_SEL_TGT_COMPOSE);
  

• selection->left = 0;

• selection->top = 0;

• selection->width = format->width;

• selection->height = format->height;

-done:

• mutex_unlock(&histo->entity.lock);
• return ret;

• return vsp1_subdev_set_pad_format(subdev, cfg, fmt,

• 			  histo->formats, histo->num_formats,
  

• 			  HISTO_MIN_SIZE, HISTO_MIN_SIZE,
  

• 			  HISTO_MAX_SIZE, HISTO_MAX_SIZE);
  

}

static const struct v4l2_subdev_pad_ops histo_pad_ops = {

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:

The structure is used in the API that the VSP1 driver exposes to the DU driver. Documenting it is thus important.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

I look forward to being reminded to add a doc-line to this structure for my interlaced support branch which added 'bool interlaced' to this structure before it was documented, (thus doesn't add a doc string). Maybe me writing this comment will remind me in the future when I rebase the interlaced patches ;-)

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

Changes since v1:

• Fixed typo

---

include/media/vsp1.h | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/include/media/vsp1.h b/include/media/vsp1.h index 68a8abe4fac5..ff7ef894465d 100644 --- a/include/media/vsp1.h +++ b/include/media/vsp1.h @@ -41,6 +41,16 @@ struct vsp1_du_lif_config { int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index, const struct vsp1_du_lif_config *cfg);

+/**

• • struct vsp1_du_atomic_config - VSP atomic configuration parameters
• • @pixelformat: plane pixel format (V4L2 4CC)
• • @pitch: line pitch in bytes, for all planes
• • @mem: DMA memory address for each plane of the frame buffer
• • @src: source rectangle in the frame buffer (integer coordinates)
• • @dst: destination rectangle on the display (integer coordinates)
• • @alpha: alpha value (0: fully transparent, 255: fully opaque)
• • @zpos: Z position of the plane (from 0 to number of planes minus 1)
• */

struct vsp1_du_atomic_config { u32 pixelformat; unsigned int pitch;

Hi Laurent, just one minor comment below

On Mon, Apr 23, 2018 at 01:34:27AM +0300, Laurent Pinchart wrote:

Add a parameter (in the form of a structure to ease future API extensions) to the VSP atomic flush handler to pass CRC source configuration, and pass the CRC value to the completion callback.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 6 ++++-- drivers/media/platform/vsp1/vsp1_drm.c | 6 ++++-- drivers/media/platform/vsp1/vsp1_drm.h | 2 +- include/media/vsp1.h | 29 +++++++++++++++++++++++++++-- 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c33840b..bdcec201591f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -31,7 +31,7 @@ #include "rcar_du_kms.h" #include "rcar_du_vsp.h"

-static void rcar_du_vsp_complete(void *private, bool completed) +static void rcar_du_vsp_complete(void *private, bool completed, u32 crc) { struct rcar_du_crtc *crtc = private;

@@ -102,7 +102,9 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)

void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe);

• struct vsp1_du_atomic_pipe_config cfg = { { 0, } };
• vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);

}

/* Keep the two tables in sync. */ diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 2b29a83dceb9..5fc31578f9b0 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -36,7 +36,7 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe, bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;

if (drm_pipe->du_complete)

• drm_pipe->du_complete(drm_pipe->du_private, complete);
  

• drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
  
  

  if (completion & VSP1_DL_FRAME_END_INTERNAL) { drm_pipe->force_brx_release = false;

@@ -739,8 +739,10 @@ EXPORT_SYMBOL_GPL(vsp1_du_atomic_update);

• vsp1_du_atomic_flush - Commit an atomic update
• @dev: the VSP device
• @pipe_index: the DRM pipeline index

• • @cfg: atomic pipe configuration
  */

-void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index) +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,

• 	  const struct vsp1_du_atomic_pipe_config *cfg)
  

{ struct vsp1_device *vsp1 = dev_get_drvdata(dev); struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index]; diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h index f4af1b2b12d6..e5b88b28806c 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.h +++ b/drivers/media/platform/vsp1/vsp1_drm.h @@ -35,7 +35,7 @@ struct vsp1_drm_pipeline { wait_queue_head_t wait_queue;

/* Frame synchronisation */

• void (*du_complete)(void *, bool);

• void (*du_complete)(void *data, bool completed, u32 crc); void *du_private;

};

diff --git a/include/media/vsp1.h b/include/media/vsp1.h index ff7ef894465d..ac63a9928a79 100644 --- a/include/media/vsp1.h +++ b/include/media/vsp1.h @@ -34,7 +34,7 @@ struct vsp1_du_lif_config { unsigned int width; unsigned int height;

• void (*callback)(void *, bool);

• void (*callback)(void *data, bool completed, u32 crc); void *callback_data;

};

@@ -61,11 +61,36 @@ struct vsp1_du_atomic_config { unsigned int zpos; };

+/**

• • enum vsp1_du_crc_source - Source used for CRC calculation
• • @VSP1_DU_CRC_NONE: CRC calculation disabled
• • @VSP_DU_CRC_PLANE: Perform CRC calculation on an input plane
• • @VSP_DU_CRC_OUTPUT: Perform CRC calculation on the composed output

These two paramters are called VSP1_DU_CRC_* not VSP_DU_CRC_*

• */

+enum vsp1_du_crc_source {

• VSP1_DU_CRC_NONE,
• VSP1_DU_CRC_PLANE,
• VSP1_DU_CRC_OUTPUT,

+};

+/**

• • struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
• • @crc.source: source for CRC calculation
• • @crc.index: index of the CRC source plane (when crc.source is set to plane)
• */

+struct vsp1_du_atomic_pipe_config {

• struct {

• enum vsp1_du_crc_source source;
  

• unsigned int index;
  

• } crc;

+};

void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index); int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index, unsigned int rpf, const struct vsp1_du_atomic_config *cfg); -void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index); +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,

• 	  const struct vsp1_du_atomic_pipe_config *cfg);
  

int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt); void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);

-- Regards,

Laurent Pinchart

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:

Add a parameter (in the form of a structure to ease future API extensions) to the VSP atomic flush handler to pass CRC source configuration, and pass the CRC value to the completion callback.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Only a minor 'thought' below. (And Jacopo already caught the enum typos, so with those are fixed...)

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 6 ++++-- drivers/media/platform/vsp1/vsp1_drm.c | 6 ++++-- drivers/media/platform/vsp1/vsp1_drm.h | 2 +- include/media/vsp1.h | 29 +++++++++++++++++++++++++++-- 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c33840b..bdcec201591f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -31,7 +31,7 @@ #include "rcar_du_kms.h" #include "rcar_du_vsp.h"

-static void rcar_du_vsp_complete(void *private, bool completed) +static void rcar_du_vsp_complete(void *private, bool completed, u32 crc) { struct rcar_du_crtc *crtc = private;

@@ -102,7 +102,9 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)

void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe);

• struct vsp1_du_atomic_pipe_config cfg = { { 0, } }> +
• vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);

}

/* Keep the two tables in sync. */ diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 2b29a83dceb9..5fc31578f9b0 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -36,7 +36,7 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe, bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;

if (drm_pipe->du_complete)

• drm_pipe->du_complete(drm_pipe->du_private, complete);
  

• drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
  
  

  if (completion & VSP1_DL_FRAME_END_INTERNAL) { drm_pipe->force_brx_release = false;

@@ -739,8 +739,10 @@ EXPORT_SYMBOL_GPL(vsp1_du_atomic_update);

• vsp1_du_atomic_flush - Commit an atomic update
• @dev: the VSP device
• @pipe_index: the DRM pipeline index

• • @cfg: atomic pipe configuration
  */

-void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index) +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,

• 	  const struct vsp1_du_atomic_pipe_config *cfg)
  

{ struct vsp1_device *vsp1 = dev_get_drvdata(dev); struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index]; diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h index f4af1b2b12d6..e5b88b28806c 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.h +++ b/drivers/media/platform/vsp1/vsp1_drm.h @@ -35,7 +35,7 @@ struct vsp1_drm_pipeline { wait_queue_head_t wait_queue;

/* Frame synchronisation */

• void (*du_complete)(void *, bool);

• void (*du_complete)(void *data, bool completed, u32 crc); void *du_private;

};

diff --git a/include/media/vsp1.h b/include/media/vsp1.h index ff7ef894465d..ac63a9928a79 100644 --- a/include/media/vsp1.h +++ b/include/media/vsp1.h @@ -34,7 +34,7 @@ struct vsp1_du_lif_config { unsigned int width; unsigned int height;

• void (*callback)(void *, bool);

• void (*callback)(void *data, bool completed, u32 crc);

Did we have to add parameters to this callback for the dynamic BRx work ?

</me struggles to remember correctly>. If so we might have to consider turning it into a struct perhaps! - but for now or otherwise I think this will be OK.

Ok - so I stopped being lazy and went to take a look. We turned completed into a bitfield - so I think actually that's enough to store any more flags we might come up with, and the CRC doesn't fit in there :D

So lets leave this as adding an extra parameter for now.

void *callback_data; };

@@ -61,11 +61,36 @@ struct vsp1_du_atomic_config { unsigned int zpos; };

+/**

• • enum vsp1_du_crc_source - Source used for CRC calculation
• • @VSP1_DU_CRC_NONE: CRC calculation disabled
• • @VSP_DU_CRC_PLANE: Perform CRC calculation on an input plane
• • @VSP_DU_CRC_OUTPUT: Perform CRC calculation on the composed output

I see Jacopo has already caught these ...

• */

+enum vsp1_du_crc_source {

• VSP1_DU_CRC_NONE,
• VSP1_DU_CRC_PLANE,
• VSP1_DU_CRC_OUTPUT,

+};

+/**

• • struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
• • @crc.source: source for CRC calculation
• • @crc.index: index of the CRC source plane (when crc.source is set to plane)
• */

+struct vsp1_du_atomic_pipe_config {

• struct {

• enum vsp1_du_crc_source source;
  

• unsigned int index;
  

• } crc;

+};

void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index); int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index, unsigned int rpf, const struct vsp1_du_atomic_config *cfg); -void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index); +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,

• 	  const struct vsp1_du_atomic_pipe_config *cfg);
  

int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt); void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);

HI Laurent, a few comments, mostly minor ones...

On Mon, Apr 23, 2018 at 01:34:28AM +0300, Laurent Pinchart wrote:

The DISCOM calculates a CRC on a configurable window of the frame. It interfaces to the VSP through the UIF glue, hence the name used in the code.

The module supports configuration of the CRC window through the crop rectangle on the ink pad of the corresponding entity. However, unlike

sink pad?

the traditional V4L2 subdevice model, the crop rectangle does not influence the format on the source pad.

Modeling the DISCOM as a sink-only entity would allow adhering to the V4L2 subdevice model at the expense of more complex code in the driver, as at the hardware level the UIF is handled as a sink+source entity. As the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it is not exposed to userspace through V4L2 but controlled through the DU driver. We can thus change this model later if needed without fear of affecting userspace.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Changes since v1:

• Don't return uninitialized value from uif_set_selection()

---

drivers/media/platform/vsp1/Makefile | 2 +- drivers/media/platform/vsp1/vsp1.h | 4 + drivers/media/platform/vsp1/vsp1_drv.c | 20 +++ drivers/media/platform/vsp1/vsp1_entity.c | 6 + drivers/media/platform/vsp1/vsp1_entity.h | 1 + drivers/media/platform/vsp1/vsp1_regs.h | 41 +++++ drivers/media/platform/vsp1/vsp1_uif.c | 271 ++++++++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_uif.h | 32 ++++ 8 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h

diff --git a/drivers/media/platform/vsp1/Makefile b/drivers/media/platform/vsp1/Makefile index 596775f932c0..4bb4dcbef7b5 100644 --- a/drivers/media/platform/vsp1/Makefile +++ b/drivers/media/platform/vsp1/Makefile @@ -5,6 +5,6 @@ vsp1-y += vsp1_rpf.o vsp1_rwpf.o vsp1_wpf.o vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o vsp1-y += vsp1_brx.o vsp1_sru.o vsp1_uds.o vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o -vsp1-y += vsp1_lif.o +vsp1-y += vsp1_lif.o vsp1_uif.o

obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h index 9cf4e1c4b036..33f632331474 100644 --- a/drivers/media/platform/vsp1/vsp1.h +++ b/drivers/media/platform/vsp1/vsp1.h @@ -36,10 +36,12 @@ struct vsp1_lut; struct vsp1_rwpf; struct vsp1_sru; struct vsp1_uds; +struct vsp1_uif;

#define VSP1_MAX_LIF 2 #define VSP1_MAX_RPF 5 #define VSP1_MAX_UDS 3 +#define VSP1_MAX_UIF 2 #define VSP1_MAX_WPF 4

#define VSP1_HAS_LUT (1 << 1) @@ -60,6 +62,7 @@ struct vsp1_device_info { unsigned int lif_count; unsigned int rpf_count; unsigned int uds_count;

• unsigned int uif_count; unsigned int wpf_count; unsigned int num_bru_inputs; bool uapi;

@@ -86,6 +89,7 @@ struct vsp1_device { struct vsp1_rwpf *rpf[VSP1_MAX_RPF]; struct vsp1_sru *sru; struct vsp1_uds *uds[VSP1_MAX_UDS];

• struct vsp1_uif *uif[VSP1_MAX_UIF]; struct vsp1_rwpf *wpf[VSP1_MAX_WPF];

  struct list_head entities;

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 331a2e0af0d3..d29f9c4baebe 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -35,6 +35,7 @@ #include "vsp1_rwpf.h" #include "vsp1_sru.h" #include "vsp1_uds.h" +#include "vsp1_uif.h" #include "vsp1_video.h"

/* ----------------------------------------------------------------------------- @@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1) list_add_tail(&uds->entity.list_dev, &vsp1->entities); }

• for (i = 0; i < vsp1->info->uif_count; ++i) {

• struct vsp1_uif *uif;
  

• uif = vsp1_uif_create(vsp1, i);
  

• if (IS_ERR(uif)) {
  

• 	ret = PTR_ERR(uif);
  

• 	goto done;
  

• }
  

• vsp1->uif[i] = uif;
  

• list_add_tail(&uif->entity.list_dev, &vsp1->entities);
  

• }
• for (i = 0; i < vsp1->info->wpf_count; ++i) { struct vsp1_rwpf *wpf;

@@ -513,6 +527,9 @@ static int vsp1_device_init(struct vsp1_device *vsp1) for (i = 0; i < vsp1->info->uds_count; ++i) vsp1_write(vsp1, VI6_DPR_UDS_ROUTE(i), VI6_DPR_NODE_UNUSED);

• for (i = 0; i < vsp1->info->uif_count; ++i)

• vsp1_write(vsp1, VI6_DPR_UIF_ROUTE(i), VI6_DPR_NODE_UNUSED);
  

• vsp1_write(vsp1, VI6_DPR_SRU_ROUTE, VI6_DPR_NODE_UNUSED); vsp1_write(vsp1, VI6_DPR_LUT_ROUTE, VI6_DPR_NODE_UNUSED); vsp1_write(vsp1, VI6_DPR_CLU_ROUTE, VI6_DPR_NODE_UNUSED);

@@ -740,6 +757,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .features = VSP1_HAS_BRU | VSP1_HAS_WPF_VFLIP, .lif_count = 1, .rpf_count = 5,

• .uif_count = 1,
  

  .wpf_count = 2, .num_bru_inputs = 5, }, {

@@ -749,6 +767,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .features = VSP1_HAS_BRS | VSP1_HAS_BRU, .lif_count = 1, .rpf_count = 5,

• .uif_count = 1,
  

  .wpf_count = 1, .num_bru_inputs = 5, }, {

@@ -758,6 +777,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .features = VSP1_HAS_BRS | VSP1_HAS_BRU, .lif_count = 2, .rpf_count = 5,

• .uif_count = 2,
  

  .wpf_count = 2, .num_bru_inputs = 5, },

diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 181a583aecad..122e60eb1489 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -539,6 +539,10 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad) { VSP1_ENTITY_UDS, idx, VI6_DPR_UDS_ROUTE(idx), \ { VI6_DPR_NODE_UDS(idx) }, VI6_DPR_NODE_UDS(idx) }

+#define VSP1_ENTITY_ROUTE_UIF(idx) \

• { VSP1_ENTITY_UIF, idx, VI6_DPR_UIF_ROUTE(idx), \

•  { VI6_DPR_NODE_UIF(idx) }, VI6_DPR_NODE_UIF(idx) }
  

#define VSP1_ENTITY_ROUTE_WPF(idx) \ { VSP1_ENTITY_WPF, idx, 0, \ { VI6_DPR_NODE_WPF(idx) }, VI6_DPR_NODE_WPF(idx) } @@ -567,6 +571,8 @@ static const struct vsp1_route vsp1_routes[] = { VSP1_ENTITY_ROUTE_UDS(0), VSP1_ENTITY_ROUTE_UDS(1), VSP1_ENTITY_ROUTE_UDS(2),

• VSP1_ENTITY_ROUTE_UIF(0), /* Named UIF4 in the documentation */
• VSP1_ENTITY_ROUTE_UIF(1), /* Named UIF5 in the documentation */ VSP1_ENTITY_ROUTE_WPF(0), VSP1_ENTITY_ROUTE_WPF(1), VSP1_ENTITY_ROUTE_WPF(2),

diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h index 0839a62cfa71..94490d697dcf 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.h +++ b/drivers/media/platform/vsp1/vsp1_entity.h @@ -33,6 +33,7 @@ enum vsp1_entity_type { VSP1_ENTITY_RPF, VSP1_ENTITY_SRU, VSP1_ENTITY_UDS,

• VSP1_ENTITY_UIF, VSP1_ENTITY_WPF,

};

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index 3201ad4b77d4..0d249ff9f564 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -307,6 +307,44 @@ #define VI6_WPF_WRBCK_CTRL 0x1034 #define VI6_WPF_WRBCK_CTRL_WBMD (1 << 0)

+/* -----------------------------------------------------------------------------

• • UIF Control Registers
• */

+#define VI6_UIF_OFFSET 0x100

+#define VI6_UIF_DISCOM_DOCMCR 0x1c00 +#define VI6_UIF_DISCOM_DOCMCR_CMPRU (1 << 16) +#define VI6_UIF_DISCOM_DOCMCR_CMPR (1 << 0)

+#define VI6_UIF_DISCOM_DOCMSTR 0x1c04 +#define VI6_UIF_DISCOM_DOCMSTR_CMPPRE (1 << 1) +#define VI6_UIF_DISCOM_DOCMSTR_CMPST (1 << 0)

+#define VI6_UIF_DISCOM_DOCMCLSTR 0x1c08 +#define VI6_UIF_DISCOM_DOCMCLSTR_CMPCLPRE (1 << 1) +#define VI6_UIF_DISCOM_DOCMCLSTR_CMPCLST (1 << 0)

+#define VI6_UIF_DISCOM_DOCMIENR 0x1c0c +#define VI6_UIF_DISCOM_DOCMIENR_CMPPREIEN (1 << 1) +#define VI6_UIF_DISCOM_DOCMIENR_CMPIEN (1 << 0)

+#define VI6_UIF_DISCOM_DOCMMDR 0x1c10 +#define VI6_UIF_DISCOM_DOCMMDR_INTHRH(n) ((n) << 16)

+#define VI6_UIF_DISCOM_DOCMPMR 0x1c14 +#define VI6_UIF_DISCOM_DOCMPMR_CMPDFF(n) ((n) << 17) +#define VI6_UIF_DISCOM_DOCMPMR_CMPDFA(n) ((n) << 8) +#define VI6_UIF_DISCOM_DOCMPMR_CMPDAUF (1 << 7) +#define VI6_UIF_DISCOM_DOCMPMR_SEL(n) ((n) << 0)

+#define VI6_UIF_DISCOM_DOCMECRCR 0x1c18 +#define VI6_UIF_DISCOM_DOCMCCRCR 0x1c1c +#define VI6_UIF_DISCOM_DOCMSPXR 0x1c20 +#define VI6_UIF_DISCOM_DOCMSPYR 0x1c24 +#define VI6_UIF_DISCOM_DOCMSZXR 0x1c28 +#define VI6_UIF_DISCOM_DOCMSZYR 0x1c2c

/* -----------------------------------------------------------------------------

• DPR Control Registers

*/ @@ -339,7 +377,10 @@ #define VI6_DPR_SMPPT_PT_MASK (0x3f << 0) #define VI6_DPR_SMPPT_PT_SHIFT 0

+#define VI6_DPR_UIF_ROUTE(n) (0x2074 + (n) * 4)

#define VI6_DPR_NODE_RPF(n) (n) +#define VI6_DPR_NODE_UIF(n) (12 + (n)) #define VI6_DPR_NODE_SRU 16 #define VI6_DPR_NODE_UDS(n) (17 + (n)) #define VI6_DPR_NODE_LUT 22 diff --git a/drivers/media/platform/vsp1/vsp1_uif.c b/drivers/media/platform/vsp1/vsp1_uif.c new file mode 100644 index 000000000000..6de7e9c801ae --- /dev/null +++ b/drivers/media/platform/vsp1/vsp1_uif.c @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • vsp1_uif.c -- R-Car VSP1 User Logic Interface
• • Copyright (C) 2017-2018 Laurent Pinchart
• • Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
• */

+#include <linux/device.h> +#include <linux/gfp.h> +#include <linux/sys_soc.h>

+#include <media/media-entity.h> +#include <media/v4l2-subdev.h>

+#include "vsp1.h" +#include "vsp1_dl.h" +#include "vsp1_entity.h" +#include "vsp1_uif.h"

+#define UIF_MIN_SIZE 4U +#define UIF_MAX_SIZE 8190U

+/* -----------------------------------------------------------------------------

• • Device Access
• */

+static inline u32 vsp1_uif_read(struct vsp1_uif *uif, u32 reg) +{

• return vsp1_read(uif->entity.vsp1,

• 	 uif->entity.index * VI6_UIF_OFFSET + reg);
  

+} +static inline void vsp1_uif_write(struct vsp1_uif *uif, struct vsp1_dl_list *dl,

• 		  u32 reg, u32 data)
  

+{

• vsp1_dl_list_write(dl, reg + uif->entity.index * VI6_UIF_OFFSET, data);

+}

+u32 vsp1_uif_get_crc(struct vsp1_uif *uif) +{

• return vsp1_uif_read(uif, VI6_UIF_DISCOM_DOCMCCRCR);

+}

+/* -----------------------------------------------------------------------------

• • V4L2 Subdevice Pad Operations
• */

+static const unsigned int uif_codes[] = {

• MEDIA_BUS_FMT_ARGB8888_1X32,
• MEDIA_BUS_FMT_AHSV8888_1X32,
• MEDIA_BUS_FMT_AYUV8_1X32,

+};

+static int uif_enum_mbus_code(struct v4l2_subdev *subdev,

• 	      struct v4l2_subdev_pad_config *cfg,
  

• 	      struct v4l2_subdev_mbus_code_enum *code)
  

+{

• return vsp1_subdev_enum_mbus_code(subdev, cfg, code, uif_codes,

• 			  ARRAY_SIZE(uif_codes));
  

+}

+static int uif_enum_frame_size(struct v4l2_subdev *subdev,

• 	       struct v4l2_subdev_pad_config *cfg,
  

• 	       struct v4l2_subdev_frame_size_enum *fse)
  

+{

• return vsp1_subdev_enum_frame_size(subdev, cfg, fse, UIF_MIN_SIZE,

• 			   UIF_MIN_SIZE, UIF_MAX_SIZE,
  

• 			   UIF_MAX_SIZE);
  

+}

+static int uif_set_format(struct v4l2_subdev *subdev,

• 	    struct v4l2_subdev_pad_config *cfg,
  

• 	    struct v4l2_subdev_format *fmt)
  

+{

• return vsp1_subdev_set_pad_format(subdev, cfg, fmt, uif_codes,

• 			  ARRAY_SIZE(uif_codes),
  

• 			  UIF_MIN_SIZE, UIF_MIN_SIZE,
  

• 			  UIF_MAX_SIZE, UIF_MAX_SIZE);
  

+}

+static int uif_get_selection(struct v4l2_subdev *subdev,

• 	     struct v4l2_subdev_pad_config *cfg,
  

• 	     struct v4l2_subdev_selection *sel)
  

+{

• struct vsp1_uif *uif = to_uif(subdev);
• struct v4l2_subdev_pad_config *config;
• struct v4l2_mbus_framefmt *format;
• int ret = 0;
• if (sel->pad != UIF_PAD_SINK)

• return -EINVAL;
  

• mutex_lock(&uif->entity.lock);
• config = vsp1_entity_get_pad_config(&uif->entity, cfg, sel->which);
• if (!config) {

• ret = -EINVAL;
  

• goto done;
  

• }
• switch (sel->target) {
• case V4L2_SEL_TGT_CROP_BOUNDS:
• case V4L2_SEL_TGT_CROP_DEFAULT:

• format = vsp1_entity_get_pad_format(&uif->entity, config,
  

• 				    UIF_PAD_SINK);
  

• sel->r.left = 0;
  

• sel->r.top = 0;
  

• sel->r.width = format->width;
  

• sel->r.height = format->height;
  

• break;
  

• case V4L2_SEL_TGT_CROP:

• sel->r = *vsp1_entity_get_pad_selection(&uif->entity, config,
  

• 					sel->pad, sel->target);
  

• break;
  

• default:

• ret = -EINVAL;
  

• break;
  

• }

+done:

• mutex_unlock(&uif->entity.lock);
• return ret;

+}

+static int uif_set_selection(struct v4l2_subdev *subdev,

• 	     struct v4l2_subdev_pad_config *cfg,
  

• 	     struct v4l2_subdev_selection *sel)
  

+{

• struct vsp1_uif *uif = to_uif(subdev);
• struct v4l2_subdev_pad_config *config;
• struct v4l2_mbus_framefmt *format;
• struct v4l2_rect *selection;
• int ret = 0;
• if (sel->pad != UIF_PAD_SINK ||

•    sel->target != V4L2_SEL_TGT_CROP)
  

• return -EINVAL;
  

• mutex_lock(&uif->entity.lock);
• config = vsp1_entity_get_pad_config(&uif->entity, cfg, sel->which);
• if (!config) {

• ret = -EINVAL;
  

• goto done;
  

• }
• /* The crop rectangle must be inside the input frame. */
• format = vsp1_entity_get_pad_format(&uif->entity, config, UIF_PAD_SINK);
• sel->r.left = clamp_t(unsigned int, sel->r.left, 0, format->width - 1);
• sel->r.top = clamp_t(unsigned int, sel->r.top, 0, format->height - 1);
• sel->r.width = clamp_t(unsigned int, sel->r.width, UIF_MIN_SIZE,

• 	       format->width - sel->r.left);
  

• sel->r.height = clamp_t(unsigned int, sel->r.height, UIF_MIN_SIZE,

• 		format->height - sel->r.top);
  

• /* Store the crop rectangle. */
• selection = vsp1_entity_get_pad_selection(&uif->entity, config,

• 				  sel->pad, V4L2_SEL_TGT_CROP);
  

• *selection = sel->r;

+done:

• mutex_unlock(&uif->entity.lock);
• return ret;

+}

+/* -----------------------------------------------------------------------------

• • V4L2 Subdevice Operations
• */

+static const struct v4l2_subdev_pad_ops uif_pad_ops = {

• .init_cfg = vsp1_entity_init_cfg,
• .enum_mbus_code = uif_enum_mbus_code,
• .enum_frame_size = uif_enum_frame_size,
• .get_fmt = vsp1_subdev_get_pad_format,
• .set_fmt = uif_set_format,
• .get_selection = uif_get_selection,
• .set_selection = uif_set_selection,

+};

+static const struct v4l2_subdev_ops uif_ops = {

• .pad = &uif_pad_ops,

+};

+/* -----------------------------------------------------------------------------

• • VSP1 Entity Operations
• */

+static void uif_configure(struct vsp1_entity *entity,

• 	  struct vsp1_pipeline *pipe,
  

• 	  struct vsp1_dl_list *dl,
  

• 	  enum vsp1_entity_params params)
  

+{

• struct vsp1_uif *uif = to_uif(&entity->subdev);
• const struct v4l2_rect *crop;
• unsigned int left;
• unsigned int width;
• /*

• * Per-partition configuration isn't needed as the DISCOM is used in
  

• * display pipelines only.
  

• */
  

• if (params != VSP1_ENTITY_PARAMS_INIT)

• return;
  

• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMPMR,

•        VI6_UIF_DISCOM_DOCMPMR_SEL(9));
  

• crop = vsp1_entity_get_pad_selection(entity, entity->config,

• 			     UIF_PAD_SINK, V4L2_SEL_TGT_CROP);
  

• /* On M3-W the horizontal coordinates are twice the register value. */
• if (uif->m3w_quirk) {

• left = crop->left / 2;
  

• width = crop->width / 2;
  

• } else {

• left = crop->left;
  

• width = crop->width;
  

• }

I would write this as

left = crop->left; width = crop->width;

/* On M3-W the horizontal coordinates are twice the register value. */ if (uif->m3w_quirk) { left /= 2; width /= 2; }

But that's really up to you.

• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPXR, left);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPYR, crop->top);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZXR, width);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZYR, crop->height);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMCR,

•        VI6_UIF_DISCOM_DOCMCR_CMPR);
  

+}

+static const struct vsp1_entity_operations uif_entity_ops = {

• .configure = uif_configure,

+};

+/* -----------------------------------------------------------------------------

• • Initialization and Cleanup
• */

+static const struct soc_device_attribute vsp1_r8a7796[] = {

• { .soc_id = "r8a7796" },
• { /* sentinel */ }

+};

+struct vsp1_uif *vsp1_uif_create(struct vsp1_device *vsp1, unsigned int index) +{

• struct vsp1_uif *uif;
• char name[6];
• int ret;
• uif = devm_kzalloc(vsp1->dev, sizeof(*uif), GFP_KERNEL);
• if (uif == NULL)

if (!uif)

Otherwise checkpatch complains iirc.

Those are very minor comments, so feel free to add my reviewed by tag

Reviewed-by: Jacopo Mondi jacopo@jmondi.org

Thanks j

• return ERR_PTR(-ENOMEM);
  

• if (soc_device_match(vsp1_r8a7796))

• uif->m3w_quirk = true;
  

• uif->entity.ops = &uif_entity_ops;
• uif->entity.type = VSP1_ENTITY_UIF;
• uif->entity.index = index;
• /* The datasheet names the two UIF instances UIF4 and UIF5. */
• sprintf(name, "uif.%u", index + 4);
• ret = vsp1_entity_init(vsp1, &uif->entity, name, 2, &uif_ops,

• 	       MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
  

• if (ret < 0)

• return ERR_PTR(ret);
  

• return uif;

+} diff --git a/drivers/media/platform/vsp1/vsp1_uif.h b/drivers/media/platform/vsp1/vsp1_uif.h new file mode 100644 index 000000000000..c71ab5f6a6f8 --- /dev/null +++ b/drivers/media/platform/vsp1/vsp1_uif.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • vsp1_uif.h -- R-Car VSP1 User Logic Interface
• • Copyright (C) 2017-2018 Laurent Pinchart
• • Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
• */

+#ifndef __VSP1_UIF_H__ +#define __VSP1_UIF_H__

+#include "vsp1_entity.h"

+struct vsp1_device;

+#define UIF_PAD_SINK 0 +#define UIF_PAD_SOURCE 1

+struct vsp1_uif {

• struct vsp1_entity entity;
• bool m3w_quirk;

+};

+static inline struct vsp1_uif *to_uif(struct v4l2_subdev *subdev) +{

• return container_of(subdev, struct vsp1_uif, entity.subdev);

+}

+struct vsp1_uif *vsp1_uif_create(struct vsp1_device *vsp1, unsigned int index); +u32 vsp1_uif_get_crc(struct vsp1_uif *uif);

+#endif /* __VSP1_UIF_H__ */

Regards,

Laurent Pinchart

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:

The DISCOM calculates a CRC on a configurable window of the frame. It interfaces to the VSP through the UIF glue, hence the name used in the code.

The module supports configuration of the CRC window through the crop rectangle on the ink pad of the corresponding entity. However, unlike the traditional V4L2 subdevice model, the crop rectangle does not influence the format on the source pad.

Modeling the DISCOM as a sink-only entity would allow adhering to the V4L2 subdevice model at the expense of more complex code in the driver, as at the hardware level the UIF is handled as a sink+source entity. As the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it is not exposed to userspace through V4L2 but controlled through the DU driver. We can thus change this model later if needed without fear of affecting userspace.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Registers check out against the data sheet, and I haven't spotted anything else here on top of the comments Jacopo has highlighted.

I'll put a +1 on Jacopo's suggestion for handling the m3-w quirk though. It's an adjustment to the width/height in the event the quirk is active - so adjusting the values after they are set feels 'right' to me.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

Changes since v1:

• Don't return uninitialized value from uif_set_selection()

---

drivers/media/platform/vsp1/Makefile | 2 +- drivers/media/platform/vsp1/vsp1.h | 4 + drivers/media/platform/vsp1/vsp1_drv.c | 20 +++ drivers/media/platform/vsp1/vsp1_entity.c | 6 + drivers/media/platform/vsp1/vsp1_entity.h | 1 + drivers/media/platform/vsp1/vsp1_regs.h | 41 +++++ drivers/media/platform/vsp1/vsp1_uif.c | 271 ++++++++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_uif.h | 32 ++++ 8 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h

diff --git a/drivers/media/platform/vsp1/Makefile b/drivers/media/platform/vsp1/Makefile index 596775f932c0..4bb4dcbef7b5 100644 --- a/drivers/media/platform/vsp1/Makefile +++ b/drivers/media/platform/vsp1/Makefile @@ -5,6 +5,6 @@ vsp1-y += vsp1_rpf.o vsp1_rwpf.o vsp1_wpf.o vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o vsp1-y += vsp1_brx.o vsp1_sru.o vsp1_uds.o vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o -vsp1-y += vsp1_lif.o +vsp1-y += vsp1_lif.o vsp1_uif.o

obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h index 9cf4e1c4b036..33f632331474 100644 --- a/drivers/media/platform/vsp1/vsp1.h +++ b/drivers/media/platform/vsp1/vsp1.h @@ -36,10 +36,12 @@ struct vsp1_lut; struct vsp1_rwpf; struct vsp1_sru; struct vsp1_uds; +struct vsp1_uif;

#define VSP1_MAX_LIF 2 #define VSP1_MAX_RPF 5 #define VSP1_MAX_UDS 3 +#define VSP1_MAX_UIF 2 #define VSP1_MAX_WPF 4

#define VSP1_HAS_LUT (1 << 1) @@ -60,6 +62,7 @@ struct vsp1_device_info { unsigned int lif_count; unsigned int rpf_count; unsigned int uds_count;

• unsigned int uif_count; unsigned int wpf_count; unsigned int num_bru_inputs; bool uapi;

@@ -86,6 +89,7 @@ struct vsp1_device { struct vsp1_rwpf *rpf[VSP1_MAX_RPF]; struct vsp1_sru *sru; struct vsp1_uds *uds[VSP1_MAX_UDS];

• struct vsp1_uif *uif[VSP1_MAX_UIF]; struct vsp1_rwpf *wpf[VSP1_MAX_WPF];

  struct list_head entities;

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 331a2e0af0d3..d29f9c4baebe 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -35,6 +35,7 @@ #include "vsp1_rwpf.h" #include "vsp1_sru.h" #include "vsp1_uds.h" +#include "vsp1_uif.h" #include "vsp1_video.h"

/* ----------------------------------------------------------------------------- @@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1) list_add_tail(&uds->entity.list_dev, &vsp1->entities); }

• for (i = 0; i < vsp1->info->uif_count; ++i) {

• struct vsp1_uif *uif;
  

• uif = vsp1_uif_create(vsp1, i);
  

• if (IS_ERR(uif)) {
  

• 	ret = PTR_ERR(uif);
  

• 	goto done;
  

• }
  

• vsp1->uif[i] = uif;
  

• list_add_tail(&uif->entity.list_dev, &vsp1->entities);
  

• }
• for (i = 0; i < vsp1->info->wpf_count; ++i) { struct vsp1_rwpf *wpf;

@@ -513,6 +527,9 @@ static int vsp1_device_init(struct vsp1_device *vsp1) for (i = 0; i < vsp1->info->uds_count; ++i) vsp1_write(vsp1, VI6_DPR_UDS_ROUTE(i), VI6_DPR_NODE_UNUSED);

• for (i = 0; i < vsp1->info->uif_count; ++i)

• vsp1_write(vsp1, VI6_DPR_UIF_ROUTE(i), VI6_DPR_NODE_UNUSED);
  

• vsp1_write(vsp1, VI6_DPR_SRU_ROUTE, VI6_DPR_NODE_UNUSED); vsp1_write(vsp1, VI6_DPR_LUT_ROUTE, VI6_DPR_NODE_UNUSED); vsp1_write(vsp1, VI6_DPR_CLU_ROUTE, VI6_DPR_NODE_UNUSED);

@@ -740,6 +757,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .features = VSP1_HAS_BRU | VSP1_HAS_WPF_VFLIP, .lif_count = 1, .rpf_count = 5,

• .uif_count = 1,
  

  .wpf_count = 2, .num_bru_inputs = 5, }, {

@@ -749,6 +767,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .features = VSP1_HAS_BRS | VSP1_HAS_BRU, .lif_count = 1, .rpf_count = 5,

• .uif_count = 1,
  

  .wpf_count = 1, .num_bru_inputs = 5, }, {

@@ -758,6 +777,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .features = VSP1_HAS_BRS | VSP1_HAS_BRU, .lif_count = 2, .rpf_count = 5,

• .uif_count = 2,
  

  .wpf_count = 2, .num_bru_inputs = 5, },

diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 181a583aecad..122e60eb1489 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -539,6 +539,10 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad) { VSP1_ENTITY_UDS, idx, VI6_DPR_UDS_ROUTE(idx), \ { VI6_DPR_NODE_UDS(idx) }, VI6_DPR_NODE_UDS(idx) }

+#define VSP1_ENTITY_ROUTE_UIF(idx) \

• { VSP1_ENTITY_UIF, idx, VI6_DPR_UIF_ROUTE(idx), \

•  { VI6_DPR_NODE_UIF(idx) }, VI6_DPR_NODE_UIF(idx) }
  

#define VSP1_ENTITY_ROUTE_WPF(idx) \ { VSP1_ENTITY_WPF, idx, 0, \ { VI6_DPR_NODE_WPF(idx) }, VI6_DPR_NODE_WPF(idx) } @@ -567,6 +571,8 @@ static const struct vsp1_route vsp1_routes[] = { VSP1_ENTITY_ROUTE_UDS(0), VSP1_ENTITY_ROUTE_UDS(1), VSP1_ENTITY_ROUTE_UDS(2),

• VSP1_ENTITY_ROUTE_UIF(0), /* Named UIF4 in the documentation */
• VSP1_ENTITY_ROUTE_UIF(1), /* Named UIF5 in the documentation */

Intriguing - I wonder what happened to UIF{0-3} :-)

VSP1_ENTITY_ROUTE_WPF(0), VSP1_ENTITY_ROUTE_WPF(1), VSP1_ENTITY_ROUTE_WPF(2), diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h index 0839a62cfa71..94490d697dcf 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.h +++ b/drivers/media/platform/vsp1/vsp1_entity.h @@ -33,6 +33,7 @@ enum vsp1_entity_type { VSP1_ENTITY_RPF, VSP1_ENTITY_SRU, VSP1_ENTITY_UDS,

• VSP1_ENTITY_UIF, VSP1_ENTITY_WPF,

};

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index 3201ad4b77d4..0d249ff9f564 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -307,6 +307,44 @@ #define VI6_WPF_WRBCK_CTRL 0x1034 #define VI6_WPF_WRBCK_CTRL_WBMD (1 << 0)

+/* -----------------------------------------------------------------------------

• • UIF Control Registers
• */

+#define VI6_UIF_OFFSET 0x100

+#define VI6_UIF_DISCOM_DOCMCR 0x1c00 +#define VI6_UIF_DISCOM_DOCMCR_CMPRU (1 << 16) +#define VI6_UIF_DISCOM_DOCMCR_CMPR (1 << 0)

+#define VI6_UIF_DISCOM_DOCMSTR 0x1c04 +#define VI6_UIF_DISCOM_DOCMSTR_CMPPRE (1 << 1) +#define VI6_UIF_DISCOM_DOCMSTR_CMPST (1 << 0)

+#define VI6_UIF_DISCOM_DOCMCLSTR 0x1c08 +#define VI6_UIF_DISCOM_DOCMCLSTR_CMPCLPRE (1 << 1) +#define VI6_UIF_DISCOM_DOCMCLSTR_CMPCLST (1 << 0)

+#define VI6_UIF_DISCOM_DOCMIENR 0x1c0c +#define VI6_UIF_DISCOM_DOCMIENR_CMPPREIEN (1 << 1) +#define VI6_UIF_DISCOM_DOCMIENR_CMPIEN (1 << 0)

+#define VI6_UIF_DISCOM_DOCMMDR 0x1c10 +#define VI6_UIF_DISCOM_DOCMMDR_INTHRH(n) ((n) << 16)

+#define VI6_UIF_DISCOM_DOCMPMR 0x1c14 +#define VI6_UIF_DISCOM_DOCMPMR_CMPDFF(n) ((n) << 17) +#define VI6_UIF_DISCOM_DOCMPMR_CMPDFA(n) ((n) << 8) +#define VI6_UIF_DISCOM_DOCMPMR_CMPDAUF (1 << 7) +#define VI6_UIF_DISCOM_DOCMPMR_SEL(n) ((n) << 0)

+#define VI6_UIF_DISCOM_DOCMECRCR 0x1c18 +#define VI6_UIF_DISCOM_DOCMCCRCR 0x1c1c +#define VI6_UIF_DISCOM_DOCMSPXR 0x1c20 +#define VI6_UIF_DISCOM_DOCMSPYR 0x1c24 +#define VI6_UIF_DISCOM_DOCMSZXR 0x1c28 +#define VI6_UIF_DISCOM_DOCMSZYR 0x1c2c

/* -----------------------------------------------------------------------------

• DPR Control Registers

*/ @@ -339,7 +377,10 @@ #define VI6_DPR_SMPPT_PT_MASK (0x3f << 0) #define VI6_DPR_SMPPT_PT_SHIFT 0

+#define VI6_DPR_UIF_ROUTE(n) (0x2074 + (n) * 4)

#define VI6_DPR_NODE_RPF(n) (n) +#define VI6_DPR_NODE_UIF(n) (12 + (n)) #define VI6_DPR_NODE_SRU 16 #define VI6_DPR_NODE_UDS(n) (17 + (n)) #define VI6_DPR_NODE_LUT 22 diff --git a/drivers/media/platform/vsp1/vsp1_uif.c b/drivers/media/platform/vsp1/vsp1_uif.c new file mode 100644 index 000000000000..6de7e9c801ae --- /dev/null +++ b/drivers/media/platform/vsp1/vsp1_uif.c @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • vsp1_uif.c -- R-Car VSP1 User Logic Interface
• • Copyright (C) 2017-2018 Laurent Pinchart
• • Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
• */

+#include <linux/device.h> +#include <linux/gfp.h> +#include <linux/sys_soc.h>

+#include <media/media-entity.h> +#include <media/v4l2-subdev.h>

+#include "vsp1.h" +#include "vsp1_dl.h" +#include "vsp1_entity.h" +#include "vsp1_uif.h"

+#define UIF_MIN_SIZE 4U +#define UIF_MAX_SIZE 8190U

+/* -----------------------------------------------------------------------------

• • Device Access
• */

+static inline u32 vsp1_uif_read(struct vsp1_uif *uif, u32 reg) +{

• return vsp1_read(uif->entity.vsp1,

• 	 uif->entity.index * VI6_UIF_OFFSET + reg);
  

+} +static inline void vsp1_uif_write(struct vsp1_uif *uif, struct vsp1_dl_list *dl,

• 		  u32 reg, u32 data)
  

+{

• vsp1_dl_list_write(dl, reg + uif->entity.index * VI6_UIF_OFFSET, data);

+}

+u32 vsp1_uif_get_crc(struct vsp1_uif *uif) +{

• return vsp1_uif_read(uif, VI6_UIF_DISCOM_DOCMCCRCR);

+}

+/* -----------------------------------------------------------------------------

• • V4L2 Subdevice Pad Operations
• */

+static const unsigned int uif_codes[] = {

• MEDIA_BUS_FMT_ARGB8888_1X32,
• MEDIA_BUS_FMT_AHSV8888_1X32,
• MEDIA_BUS_FMT_AYUV8_1X32,

+};

+static int uif_enum_mbus_code(struct v4l2_subdev *subdev,

• 	      struct v4l2_subdev_pad_config *cfg,
  

• 	      struct v4l2_subdev_mbus_code_enum *code)
  

+{

• return vsp1_subdev_enum_mbus_code(subdev, cfg, code, uif_codes,

• 			  ARRAY_SIZE(uif_codes));
  

+}

+static int uif_enum_frame_size(struct v4l2_subdev *subdev,

• 	       struct v4l2_subdev_pad_config *cfg,
  

• 	       struct v4l2_subdev_frame_size_enum *fse)
  

+{

• return vsp1_subdev_enum_frame_size(subdev, cfg, fse, UIF_MIN_SIZE,

• 			   UIF_MIN_SIZE, UIF_MAX_SIZE,
  

• 			   UIF_MAX_SIZE);
  

+}

+static int uif_set_format(struct v4l2_subdev *subdev,

• 	    struct v4l2_subdev_pad_config *cfg,
  

• 	    struct v4l2_subdev_format *fmt)
  

+{

• return vsp1_subdev_set_pad_format(subdev, cfg, fmt, uif_codes,

• 			  ARRAY_SIZE(uif_codes),
  

• 			  UIF_MIN_SIZE, UIF_MIN_SIZE,
  

• 			  UIF_MAX_SIZE, UIF_MAX_SIZE);
  

+}

+static int uif_get_selection(struct v4l2_subdev *subdev,

• 	     struct v4l2_subdev_pad_config *cfg,
  

• 	     struct v4l2_subdev_selection *sel)
  

+{

• struct vsp1_uif *uif = to_uif(subdev);
• struct v4l2_subdev_pad_config *config;
• struct v4l2_mbus_framefmt *format;
• int ret = 0;
• if (sel->pad != UIF_PAD_SINK)

• return -EINVAL;
  

• mutex_lock(&uif->entity.lock);
• config = vsp1_entity_get_pad_config(&uif->entity, cfg, sel->which);
• if (!config) {

• ret = -EINVAL;
  

• goto done;
  

• }
• switch (sel->target) {
• case V4L2_SEL_TGT_CROP_BOUNDS:
• case V4L2_SEL_TGT_CROP_DEFAULT:

• format = vsp1_entity_get_pad_format(&uif->entity, config,
  

• 				    UIF_PAD_SINK);
  

• sel->r.left = 0;
  

• sel->r.top = 0;
  

• sel->r.width = format->width;
  

• sel->r.height = format->height;
  

• break;
  

• case V4L2_SEL_TGT_CROP:

• sel->r = *vsp1_entity_get_pad_selection(&uif->entity, config,
  

• 					sel->pad, sel->target);
  

• break;
  

• default:

• ret = -EINVAL;
  

• break;
  

• }

+done:

• mutex_unlock(&uif->entity.lock);
• return ret;

+}

+static int uif_set_selection(struct v4l2_subdev *subdev,

• 	     struct v4l2_subdev_pad_config *cfg,
  

• 	     struct v4l2_subdev_selection *sel)
  

+{

• struct vsp1_uif *uif = to_uif(subdev);
• struct v4l2_subdev_pad_config *config;
• struct v4l2_mbus_framefmt *format;
• struct v4l2_rect *selection;
• int ret = 0;
• if (sel->pad != UIF_PAD_SINK ||

•    sel->target != V4L2_SEL_TGT_CROP)
  

• return -EINVAL;
  

• mutex_lock(&uif->entity.lock);
• config = vsp1_entity_get_pad_config(&uif->entity, cfg, sel->which);
• if (!config) {

• ret = -EINVAL;
  

• goto done;
  

• }
• /* The crop rectangle must be inside the input frame. */
• format = vsp1_entity_get_pad_format(&uif->entity, config, UIF_PAD_SINK);
• sel->r.left = clamp_t(unsigned int, sel->r.left, 0, format->width - 1);
• sel->r.top = clamp_t(unsigned int, sel->r.top, 0, format->height - 1);
• sel->r.width = clamp_t(unsigned int, sel->r.width, UIF_MIN_SIZE,

• 	       format->width - sel->r.left);
  

• sel->r.height = clamp_t(unsigned int, sel->r.height, UIF_MIN_SIZE,

• 		format->height - sel->r.top);
  

• /* Store the crop rectangle. */
• selection = vsp1_entity_get_pad_selection(&uif->entity, config,

• 				  sel->pad, V4L2_SEL_TGT_CROP);
  

• *selection = sel->r;

+done:

• mutex_unlock(&uif->entity.lock);
• return ret;

+}

+/* -----------------------------------------------------------------------------

• • V4L2 Subdevice Operations
• */

+static const struct v4l2_subdev_pad_ops uif_pad_ops = {

• .init_cfg = vsp1_entity_init_cfg,
• .enum_mbus_code = uif_enum_mbus_code,
• .enum_frame_size = uif_enum_frame_size,
• .get_fmt = vsp1_subdev_get_pad_format,
• .set_fmt = uif_set_format,
• .get_selection = uif_get_selection,
• .set_selection = uif_set_selection,

+};

+static const struct v4l2_subdev_ops uif_ops = {

• .pad = &uif_pad_ops,

+};

+/* -----------------------------------------------------------------------------

• • VSP1 Entity Operations
• */

+static void uif_configure(struct vsp1_entity *entity,

• 	  struct vsp1_pipeline *pipe,
  

• 	  struct vsp1_dl_list *dl,
  

• 	  enum vsp1_entity_params params)
  

+{

• struct vsp1_uif *uif = to_uif(&entity->subdev);
• const struct v4l2_rect *crop;
• unsigned int left;
• unsigned int width;
• /*

• * Per-partition configuration isn't needed as the DISCOM is used in
  

• * display pipelines only.
  

• */
  

• if (params != VSP1_ENTITY_PARAMS_INIT)

• return;
  

• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMPMR,

•        VI6_UIF_DISCOM_DOCMPMR_SEL(9));
  

• crop = vsp1_entity_get_pad_selection(entity, entity->config,

• 			     UIF_PAD_SINK, V4L2_SEL_TGT_CROP);
  

• /* On M3-W the horizontal coordinates are twice the register value. */

Well that's just awkward :D

• if (uif->m3w_quirk) {

• left = crop->left / 2;
  

• width = crop->width / 2;
  

• } else {

• left = crop->left;
  

• width = crop->width;
  

• }

+1 here on Jacopo's suggestion :-)

• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPXR, left);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPYR, crop->top);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZXR, width);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZYR, crop->height);
• vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMCR,

•        VI6_UIF_DISCOM_DOCMCR_CMPR);
  

+}

+static const struct vsp1_entity_operations uif_entity_ops = {

• .configure = uif_configure,

+};

+/* -----------------------------------------------------------------------------

• • Initialization and Cleanup
• */

+static const struct soc_device_attribute vsp1_r8a7796[] = {

• { .soc_id = "r8a7796" },
• { /* sentinel */ }

+};

+struct vsp1_uif *vsp1_uif_create(struct vsp1_device *vsp1, unsigned int index) +{

• struct vsp1_uif *uif;
• char name[6];
• int ret;
• uif = devm_kzalloc(vsp1->dev, sizeof(*uif), GFP_KERNEL);
• if (uif == NULL)

• return ERR_PTR(-ENOMEM);
  

• if (soc_device_match(vsp1_r8a7796))

• uif->m3w_quirk = true;
  

• uif->entity.ops = &uif_entity_ops;
• uif->entity.type = VSP1_ENTITY_UIF;
• uif->entity.index = index;
• /* The datasheet names the two UIF instances UIF4 and UIF5. */
• sprintf(name, "uif.%u", index + 4);
• ret = vsp1_entity_init(vsp1, &uif->entity, name, 2, &uif_ops,

• 	       MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
  

• if (ret < 0)

• return ERR_PTR(ret);
  

• return uif;

+} diff --git a/drivers/media/platform/vsp1/vsp1_uif.h b/drivers/media/platform/vsp1/vsp1_uif.h new file mode 100644 index 000000000000..c71ab5f6a6f8 --- /dev/null +++ b/drivers/media/platform/vsp1/vsp1_uif.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • vsp1_uif.h -- R-Car VSP1 User Logic Interface
• • Copyright (C) 2017-2018 Laurent Pinchart
• • Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
• */

+#ifndef __VSP1_UIF_H__ +#define __VSP1_UIF_H__

+#include "vsp1_entity.h"

+struct vsp1_device;

+#define UIF_PAD_SINK 0 +#define UIF_PAD_SOURCE 1

+struct vsp1_uif {

• struct vsp1_entity entity;
• bool m3w_quirk;

+};

+static inline struct vsp1_uif *to_uif(struct v4l2_subdev *subdev) +{

• return container_of(subdev, struct vsp1_uif, entity.subdev);

+}

+struct vsp1_uif *vsp1_uif_create(struct vsp1_device *vsp1, unsigned int index); +u32 vsp1_uif_get_crc(struct vsp1_uif *uif);

+#endif /* __VSP1_UIF_H__ */

HI Laurent,

On Mon, Apr 23, 2018 at 01:34:29AM +0300, Laurent Pinchart wrote:

The DISCOM is used to compute CRCs on display frames. Integrate it in the display pipeline at the output of the blending unit to process output frames.

Computing CRCs on input frames is possible by positioning the DISCOM at a different point in the pipeline. This use case isn't supported at the moment and could be implemented by extending the API between the VSP1 and DU drivers if needed.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/media/platform/vsp1/vsp1_drm.c | 115 ++++++++++++++++++++++++++++++++- drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++ 2 files changed, 124 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -22,6 +22,7 @@ #include "vsp1_lif.h" #include "vsp1_pipe.h" #include "vsp1_rwpf.h" +#include "vsp1_uif.h"

#define BRX_NAME(e) (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"

@@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe, struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe); bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;

• if (drm_pipe->du_complete)

• drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
  

• if (drm_pipe->du_complete) {

• struct vsp1_entity *uif = drm_pipe->uif;
  

• u32 crc;
  

• crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
  

• drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
  

• }

  if (completion & VSP1_DL_FRAME_END_INTERNAL) { drm_pipe->force_brx_release = false;

@@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,

• Pipeline Configuration

*/

+/*

• • Insert the UIF in the pipeline between the prev and next entities. If no UIF
• • is available connect the two entities directly.
• */

+static int vsp1_du_insert_uif(struct vsp1_device *vsp1,

• 	      struct vsp1_pipeline *pipe,
  

• 	      struct vsp1_entity *uif,
  

• 	      struct vsp1_entity *prev, unsigned int prev_pad,
  

• 	      struct vsp1_entity *next, unsigned int next_pad)
  

+{

• int ret;
• if (uif) {

• struct v4l2_subdev_format format;
  

• prev->sink = uif;
  

• prev->sink_pad = UIF_PAD_SINK;
  

• memset(&format, 0, sizeof(format));
  

• format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
  

• format.pad = prev_pad;
  

• ret = v4l2_subdev_call(&prev->subdev, pad, get_fmt, NULL,
  

• 		       &format);
  

• if (ret < 0)
  

• 	return ret;
  

• format.pad = UIF_PAD_SINK;
  

• ret = v4l2_subdev_call(&uif->subdev, pad, set_fmt, NULL,
  

• 		       &format);
  

• if (ret < 0)
  

• 	return ret;
  

• dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
  

• 	__func__, format.format.width, format.format.height,
  

• 	format.format.code);
  

• /*
  

•  * The UIF doesn't mangle the format between its sink and
  

•  * source pads, so there is no need to retrieve the format on
  

•  * its source pad.
  

•  */
  

• uif->sink = next;
  

• uif->sink_pad = next_pad;
  

• } else {

• prev->sink = next;
  

• prev->sink_pad = next_pad;
  

Isn't the !uif case better handled in the caller? See below... Or otherwise, if you prefer handling it here, shouldn't the indentation be reduced with

if (!uif) { prev->sink = next; prev->sink_pad = next_pad;

return 0; }

• }
• return 0;

+}

/* Setup one RPF and the connected BRx sink pad. */ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe, struct vsp1_rwpf *rpf,

• 		      struct vsp1_entity *uif,
  	      unsigned int brx_input)
  

{ struct v4l2_subdev_selection sel; @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1, if (ret < 0) return ret;

• /* Insert and configure the UIF if available. */
• ret = vsp1_du_insert_uif(vsp1, pipe, uif, &rpf->entity, RWPF_PAD_SOURCE,

• 		 pipe->brx, brx_input);
  

• if (ret < 0)

• return ret;
  

• /* BRx sink, propagate the format from the RPF source. */ format.pad = brx_input;

@@ -297,7 +364,10 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf) static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe) {

• struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe); struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
• struct vsp1_entity *uif;
• bool use_uif = false; struct vsp1_brx *brx; unsigned int i; int ret;

@@ -358,7 +428,11 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, dev_dbg(vsp1->dev, "%s: connecting RPF.%u to %s:%u\n", __func__, rpf->entity.index, BRX_NAME(pipe->brx), i);

• ret = vsp1_du_pipeline_setup_rpf(vsp1, pipe, rpf, i);
  

• uif = drm_pipe->crc.source == VSP1_DU_CRC_PLANE &&
  

•       drm_pipe->crc.index == i ? drm_pipe->uif : NULL;
  

• if (uif)
  

• 	use_uif = true;
  

• ret = vsp1_du_pipeline_setup_rpf(vsp1, pipe, rpf, uif, i);
  

  if (ret < 0) { dev_err(vsp1->dev, "%s: failed to setup RPF.%u\n",

@@ -367,6 +441,31 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, } }

• /* Insert and configure the UIF at the BRx output if available. */
• uif = drm_pipe->crc.source == VSP1_DU_CRC_OUTPUT ? drm_pipe->uif : NULL;
• if (uif)

• use_uif = true;
  

• ret = vsp1_du_insert_uif(vsp1, pipe, uif,

• 		 pipe->brx, pipe->brx->source_pad,
  

• 		 &pipe->output->entity, 0);
  

• if (ret < 0)

• dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
  

• 	__func__, BRX_NAME(pipe->brx));
  

• /*

• * If the UIF is not in use schedule it for removal by setting its pipe
  

• * pointer to NULL, vsp1_du_pipeline_configure() will remove it from the
  

• * hardware pipeline and from the pipeline's list of entities. Otherwise
  

• * make sure it is present in the pipeline's list of entities if it
  

• * wasn't already.
  

• */
  

• if (!use_uif) {

... here. If you don't use uif, don't call vspi1_du_insert_uif(). True, you have to link entities explicitly here if there is not uif, but I may be missing where this was happening before this code was added.

Thanks j

• drm_pipe->uif->pipe = NULL;
  

• } else if (!drm_pipe->uif->pipe) {

• drm_pipe->uif->pipe = pipe;
  

• list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
  

• }
• return 0;

}

@@ -748,6 +847,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index, struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index]; struct vsp1_pipeline *pipe = &drm_pipe->pipe;

• drm_pipe->crc.source = cfg->crc.source;
• drm_pipe->crc.index = cfg->crc.index;
• vsp1_du_pipeline_setup_inputs(vsp1, pipe); vsp1_du_pipeline_configure(pipe); mutex_unlock(&vsp1->drm->lock);

@@ -816,6 +918,13 @@ int vsp1_drm_init(struct vsp1_device *vsp1)

pipe->lif->pipe = pipe;
list_add_tail(&pipe->lif->list_pipe, &pipe->entities);

• /*
  

•  * CRC computation is initially disabled, don't add the UIF to
  

•  * the pipeline.
  

•  */
  

• if (i < vsp1->info->uif_count)
  

Why 'initially disabled'? This seems to me to conditionally enable the UIF unit. Or is the lif count always the same as the uif count?

Thanks j

• 	drm_pipe->uif = &vsp1->uif[i]->entity;
  

  }

  /* Disable all RPFs initially. */

diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h index e5b88b28806c..1e7670955ef0 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.h +++ b/drivers/media/platform/vsp1/vsp1_drm.h @@ -13,6 +13,8 @@ #include <linux/videodev2.h> #include <linux/wait.h>

+#include <media/vsp1.h>

#include "vsp1_pipe.h"

/** @@ -22,6 +24,9 @@

• @height: output display height
• @force_brx_release: when set, release the BRx during the next reconfiguration
• @wait_queue: wait queue to wait for BRx release completion

• • @uif: UIF entity if available for the pipeline
• • @crc.source: source for CRC calculation
• • @crc.index: index of the CRC source plane (when crc.source is set to plane)
  • @du_complete: frame completion callback for the DU driver (optional)
  • @du_private: data to be passed to the du_complete callback
  */

@@ -34,6 +39,13 @@ struct vsp1_drm_pipeline { bool force_brx_release; wait_queue_head_t wait_queue;

• struct vsp1_entity *uif;
• struct {

• enum vsp1_du_crc_source source;
  

• unsigned int index;
  

• } crc;
• /* Frame synchronisation */ void (*du_complete)(void *data, bool completed, u32 crc); void *du_private;

-- Regards,

Laurent Pinchart

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:

The DISCOM is used to compute CRCs on display frames. Integrate it in the display pipeline at the output of the blending unit to process output frames.

Computing CRCs on input frames is possible by positioning the DISCOM at a different point in the pipeline. This use case isn't supported at the moment and could be implemented by extending the API between the VSP1 and DU drivers if needed.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Only a couple of small questions - but nothing to block an RB tag.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/media/platform/vsp1/vsp1_drm.c | 115 ++++++++++++++++++++++++++++++++- drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++ 2 files changed, 124 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -22,6 +22,7 @@ #include "vsp1_lif.h" #include "vsp1_pipe.h" #include "vsp1_rwpf.h" +#include "vsp1_uif.h"

#define BRX_NAME(e) (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"

@@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe, struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe); bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;

• if (drm_pipe->du_complete)

• drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
  

• if (drm_pipe->du_complete) {

• struct vsp1_entity *uif = drm_pipe->uif;
  

• u32 crc;
  

• crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
  

• drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
  

• }

  if (completion & VSP1_DL_FRAME_END_INTERNAL) { drm_pipe->force_brx_release = false;

@@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,

• Pipeline Configuration

*/

+/*

• • Insert the UIF in the pipeline between the prev and next entities. If no UIF
• • is available connect the two entities directly.
• */

+static int vsp1_du_insert_uif(struct vsp1_device *vsp1,

• 	      struct vsp1_pipeline *pipe,
  

• 	      struct vsp1_entity *uif,
  

• 	      struct vsp1_entity *prev, unsigned int prev_pad,
  

• 	      struct vsp1_entity *next, unsigned int next_pad)
  

+{

• int ret;
• if (uif) {

• struct v4l2_subdev_format format;
  

• prev->sink = uif;
  

• prev->sink_pad = UIF_PAD_SINK;
  

• memset(&format, 0, sizeof(format));
  

• format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
  

• format.pad = prev_pad;
  

• ret = v4l2_subdev_call(&prev->subdev, pad, get_fmt, NULL,
  

• 		       &format);
  

• if (ret < 0)
  

• 	return ret;
  

• format.pad = UIF_PAD_SINK;
  

• ret = v4l2_subdev_call(&uif->subdev, pad, set_fmt, NULL,
  

• 		       &format);
  

• if (ret < 0)
  

• 	return ret;
  

• dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
  

• 	__func__, format.format.width, format.format.height,
  

• 	format.format.code);
  

• /*
  

•  * The UIF doesn't mangle the format between its sink and
  

•  * source pads, so there is no need to retrieve the format on
  

•  * its source pad.
  

•  */
  

• uif->sink = next;
  

• uif->sink_pad = next_pad;
  

• } else {

• prev->sink = next;
  

• prev->sink_pad = next_pad;
  

• }
• return 0;

+}

/* Setup one RPF and the connected BRx sink pad. */ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe, struct vsp1_rwpf *rpf,

• 		      struct vsp1_entity *uif,
  	      unsigned int brx_input)
  

{ struct v4l2_subdev_selection sel; @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1, if (ret < 0) return ret;

• /* Insert and configure the UIF if available. */
• ret = vsp1_du_insert_uif(vsp1, pipe, uif, &rpf->entity, RWPF_PAD_SOURCE,

• 		 pipe->brx, brx_input);
  

• if (ret < 0)

• return ret;
  

• /* BRx sink, propagate the format from the RPF source. */ format.pad = brx_input;

@@ -297,7 +364,10 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf) static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe) {

• struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe); struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
• struct vsp1_entity *uif;
• bool use_uif = false; struct vsp1_brx *brx; unsigned int i; int ret;

@@ -358,7 +428,11 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, dev_dbg(vsp1->dev, "%s: connecting RPF.%u to %s:%u\n", __func__, rpf->entity.index, BRX_NAME(pipe->brx), i);

• ret = vsp1_du_pipeline_setup_rpf(vsp1, pipe, rpf, i);
  

• uif = drm_pipe->crc.source == VSP1_DU_CRC_PLANE &&
  

•       drm_pipe->crc.index == i ? drm_pipe->uif : NULL;
  

• if (uif)
  

• 	use_uif = true;
  

• ret = vsp1_du_pipeline_setup_rpf(vsp1, pipe, rpf, uif, i);
  

  if (ret < 0) { dev_err(vsp1->dev, "%s: failed to setup RPF.%u\n",

@@ -367,6 +441,31 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, } }

• /* Insert and configure the UIF at the BRx output if available. */
• uif = drm_pipe->crc.source == VSP1_DU_CRC_OUTPUT ? drm_pipe->uif : NULL;
• if (uif)

• use_uif = true;
  

• ret = vsp1_du_insert_uif(vsp1, pipe, uif,

• 		 pipe->brx, pipe->brx->source_pad,
  

• 		 &pipe->output->entity, 0);
  

• if (ret < 0)

• dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
  

• 	__func__, BRX_NAME(pipe->brx));
  

• /*

• * If the UIF is not in use schedule it for removal by setting its pipe
  

• * pointer to NULL, vsp1_du_pipeline_configure() will remove it from the
  

• * hardware pipeline and from the pipeline's list of entities. Otherwise
  

• * make sure it is present in the pipeline's list of entities if it
  

• * wasn't already.
  

• */
  

• if (!use_uif) {