by Helen Koike

[permalink] [raw]

Subject: Re: [PATCH 00/16] media: vimc: Add support for multiplanar formats

Hi Andre,

Thanks for you patch. I didn't do a full review, but I did some minor
comments, please check.

Thanks
Helen

On 3/15/19 2:29 PM, André Almeida wrote:
>
>
> On 3/15/19 1:43 PM, André Almeida wrote:
>> Hello,
>>
>> This series implements support for multiplane pixel formats at vimc.
>> A lot of changes were required since vimc support for singleplane
>> was "hardcoded". The code has been adapted in order to support both
>> formats. When was possible, the functions were written generically,
>> avoiding functions for just one type of pixel format (single/multi)
>> and favoring code reuse.
>>
>> The debayer subdevice is the only one that currently doesn't supports
>> multiplanar formats. Documentation to each device will be made in a
>> future patch.
>>
>
> Forgot to mention that this patch series depends on this one:
>
> "[PATCH] media: vimc: propagate pixel format in the stream"
>
>> Thanks,
>>     André
>>
>> André Almeida (16):
>>    media: Move sp2mp functions to v4l2-common
>>    media: vimc: Remove unnecessary stream check
>>    media: vimc: Check if the stream is on using ved.stream
>>    media: vimc: cap: Change vimc_cap_device.format type
>>    media: vimc: Create multiplanar parameter
>>    media: vimc: cap: Dynamically define stream pixelformat
>>    media: vimc: cap: Add handler for singleplanar fmt ioctls
>>    media: vimc: cap: Add handler for multiplanar fmt ioctls
>>    media: vimc: cap: Add multiplanar formats
>>    media: vimc: cap: Add multiplanar default format
>>    media: vimc: cap: Allocate and verify mplanar buffers
>>    media: vimc: Add and use new struct vimc_frame
>>    media: vimc: sen: Add support for multiplanar formats
>>    media: vimc: sca: Add support for multiplanar formats
>>    media: vimc: cap: Add support for multiplanar formats
>>    media: vimc: cap: Dynamically define device caps
>>
>> drivers/media/platform/vimc/vimc-capture.c    | 310 +++++++++++++++---
>> drivers/media/platform/vimc/vimc-common.c     | 37 +++
>> drivers/media/platform/vimc/vimc-common.h     | 50 ++-
>> drivers/media/platform/vimc/vimc-core.c       |   8 +
>> drivers/media/platform/vimc/vimc-debayer.c    | 38 +--
>> drivers/media/platform/vimc/vimc-scaler.c     | 125 ++++---
>> drivers/media/platform/vimc/vimc-sensor.c     | 62 ++--
>> drivers/media/platform/vimc/vimc-streamer.c   |   2 +-
>> drivers/media/platform/vivid/vivid-vid-cap.c |   6 +-
>> .../media/platform/vivid/vivid-vid-common.c   | 59 ----
>> .../media/platform/vivid/vivid-vid-common.h   |   9 -
>> drivers/media/platform/vivid/vivid-vid-out.c |   6 +-
>> drivers/media/v4l2-core/v4l2-common.c         | 62 ++++
>> include/media/v4l2-common.h                   | 31 ++
>> 14 files changed, 580 insertions(+), 225 deletions(-)
>>

2019-03-15 19:34:40

by Helen Koike

[permalink] [raw]

Subject: Re: [PATCH 12/16] media: vimc: Add and use new struct vimc_frame

On 3/15/19 1:43 PM, André Almeida wrote:
> Struct vimc_frame is intended to hold metadata about a frame,
> such as memory address of a plane, number of planes and size
> of each plane, to better integrated with the multiplanar operations.
> The struct can be also used with singleplanar formats, making the
> implementation of frame manipulation generic for both type of
> formats.
>
> vimc_fill_frame function fills a vimc_frame structure given a
> pixelformat, height and width. This is done once to avoid recalculations
> and provide enough information to subdevices work with
> the frame.
>
> Change the return and argument type of process_frame from void* to
> vimc_frame*. Change the frame in subdevices structs from u8* to vimc_frame.
>
> Signed-off-by: André Almeida <[email protected]>
> ---
> drivers/media/platform/vimc/vimc-capture.c | 6 +--
> drivers/media/platform/vimc/vimc-common.c | 37 ++++++++++++++++
> drivers/media/platform/vimc/vimc-common.h | 48 +++++++++++++++++++--
> drivers/media/platform/vimc/vimc-debayer.c | 33 +++++++-------
> drivers/media/platform/vimc/vimc-scaler.c | 26 +++++------
> drivers/media/platform/vimc/vimc-sensor.c | 18 ++++----
> drivers/media/platform/vimc/vimc-streamer.c | 2 +-
> 7 files changed, 126 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 83196b8c31b5..bb982761562e 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -571,8 +571,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
> kfree(vcap);
> }
>
> -static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> - const void *frame)
> +static struct vimc_frame *vimc_cap_process_frame(struct vimc_ent_device *ved,
> + const struct vimc_frame *frame)
> {
> struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
> ved);
> @@ -601,7 +601,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>
> vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
>
> - memcpy(vbuf, frame, vcap->format.fmt.pix.sizeimage);
> + memcpy(vbuf, frame->plane_addr[0], vcap->format.fmt.pix.sizeimage);
>
> /* Set it as ready */
> vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index f664f23ee0ca..96247302f6c9 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -378,6 +378,43 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> }
> EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
>
> +void vimc_fill_frame(struct vimc_frame *frame, u32 pixelformat,
> + u32 width, u32 height)
> +{
> + unsigned int i;
> + const struct v4l2_format_info *pix_info;
> +
> + pix_info = v4l2_format_info(pixelformat);
> + frame->pixelformat = pixelformat;
> +
> + if (multiplanar) {
> + struct v4l2_pix_format_mplane pix_fmt_mp;
> +
> + v4l2_fill_pixfmt_mp(&pix_fmt_mp, pixelformat, width, height);
> +
> + frame->pixelformat = pixelformat;

This assigned was already done outside the if block

> + frame->num_planes = pix_fmt_mp.num_planes;
> + for (i = 0; i < pix_fmt_mp.num_planes; i++) {
> + frame->sizeimage[i] =
> + pix_fmt_mp.plane_fmt[i].sizeimage;

You can use a single line here, it will fix 80 chars exact :)

> + frame->bytesperline[i] =
> + pix_fmt_mp.plane_fmt[i].bytesperline;
> + frame->bpp[i] = pix_info->bpp[i];
> + frame->plane_addr[i] = NULL;
> + }
> + } else {
> + struct v4l2_pix_format pix_fmt;
> +
> + v4l2_fill_pixfmt(&pix_fmt, pixelformat, width, height);
> +
> + frame->num_planes = 1;
> + frame->sizeimage[0] = pix_fmt.sizeimage;
> + frame->bytesperline[0] = pix_fmt.bytesperline;
> + frame->bpp[0] = pix_info->bpp[0];
> + frame->plane_addr[0] = NULL;
> + }
> +}
> +
> void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
> {
> v4l2_device_unregister_subdev(sd);
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 25e47c8691dd..c891701e95a5 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -21,6 +21,7 @@
> #include <linux/slab.h>
> #include <media/media-device.h>
> #include <media/v4l2-device.h>
> +#include <media/tpg/v4l2-tpg.h>
>
> #include "vimc-streamer.h"
>
> @@ -81,6 +82,31 @@ struct vimc_platform_data {
> char entity_name[32];
> };
>
> +/**
> + * struct vimc_frame - metadata about frame components
> + *
> + * @pixelformat: fourcc pixelformat code
> + * @plane_addr: pointer to kernel address of the plane
> + * @num_planes: number of valid planes on a frame
> + * @sizeimage: size in bytes of a plane
> + * @bytesperline: number of bytes per line of a plane
> + * @bpp: number of bytes per pixel of a plane
> + *
> + * This struct helps subdevices to get information about the frame on
> + * multiplanar formats. If a singleplanar format is used, only the first
> + * index of each array is used and num_planes is set to 1, so the
> + * implementation is generic and the code will work for both formats.
> + */
> +
> +struct vimc_frame {
> + u32 pixelformat;
> + u8 *plane_addr[TPG_MAX_PLANES];
> + u8 num_planes;

please move u8 to the end to avoid weird padding in the struct.

> + u32 sizeimage[TPG_MAX_PLANES];
> + u32 bytesperline[TPG_MAX_PLANES];
> + u8 bpp[TPG_MAX_PLANES];
> +};
> +
> /**
> * struct vimc_ent_device - core struct that represents a node in the topology
> *
> @@ -103,10 +129,10 @@ struct vimc_ent_device {
> struct media_entity *ent;
> struct media_pad *pads;
> struct vimc_stream *stream;
> - void * (*process_frame)(struct vimc_ent_device *ved,
> - const void *frame);
> + struct vimc_frame * (*process_frame)(struct vimc_ent_device *ved,
> + const struct vimc_frame *frame);
> void (*vdev_get_format)(struct vimc_ent_device *ved,
> - struct v4l2_pix_format *fmt);
> + struct v4l2_pix_format *fmt);
> };
>
> /**
> @@ -206,4 +232,20 @@ void vimc_ent_sd_unregister(struct vimc_ent_device *ved,
> */
> int vimc_link_validate(struct media_link *link);
>
> +/**
> + * vimc_fill_frame - fills struct vimc_frame
> + *
> + * @frame: pointer to the frame to be filled
> + * @pixelformat: pixelformat fourcc code
> + * @width: width of the image
> + * @height: height of the image
> + *
> + * This function fills the fields of vimc_frame in order to subdevs have
> + * information about the frame being processed, works both for single
> + * and multiplanar pixel formats.
> + */
> +void vimc_fill_frame(struct vimc_frame *frame,
> + u32 pixelformat,
> + u32 width, u32 height);
> +
> #endif
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index f72f888ba5a6..19668de9a4d5 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -62,7 +62,7 @@ struct vimc_deb_device {
> void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
> unsigned int col, unsigned int rgb[3]);
> /* Values calculated when the stream starts */
> - u8 *src_frame;
> + struct vimc_frame src_frame;
> const struct vimc_deb_pix_map *sink_pix_map;
> unsigned int sink_bpp;
> };
> @@ -325,7 +325,7 @@ static void vimc_deb_set_rgb_pix_rgb24(struct vimc_deb_device *vdeb,
>
> index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3);
> for (i = 0; i < 3; i++)
> - vdeb->src_frame[index + i] = rgb[i];
> + vdeb->src_frame.plane_addr[0][index + i] = rgb[i];
> }
>
> static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> @@ -335,7 +335,6 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> if (enable) {
> u32 src_pixelformat = vdeb->ved.stream->producer_pixfmt;
> const struct v4l2_format_info *pix_info;
> - unsigned int frame_size;
>
> /* We only support translating bayer to RGB24 */
> if (src_pixelformat != V4L2_PIX_FMT_RGB24) {
> @@ -354,9 +353,8 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> vdeb->sink_pix_map->pixelformat;
>
> /* Calculate frame_size of the source */
> - pix_info = v4l2_format_info(src_pixelformat);
> - frame_size = vdeb->sink_fmt.width * vdeb->sink_fmt.height *
> - pix_info->bpp[0];
> + vimc_fill_frame(&vdeb->src_frame, src_pixelformat,
> + vdeb->sink_fmt.width, vdeb->sink_fmt.height);
>
> /* Get bpp from the sink */
> pix_info = v4l2_format_info(vdeb->sink_pix_map->pixelformat);
> @@ -366,16 +364,18 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> * Allocate the frame buffer. Use vmalloc to be able to
> * allocate a large amount of memory
> */
> - vdeb->src_frame = vmalloc(frame_size);
> - if (!vdeb->src_frame)
> + vdeb->src_frame.plane_addr[0] =
> + vmalloc(vdeb->src_frame.sizeimage[0]);
> + if (!vdeb->src_frame.plane_addr[0])
> return -ENOMEM;
>
> +
> } else {
> - if (!vdeb->src_frame)
> + if (!vdeb->src_frame.plane_addr[0])
> return 0;
>
> - vfree(vdeb->src_frame);
> - vdeb->src_frame = NULL;
> + vfree(vdeb->src_frame.plane_addr[0]);
> + vdeb->src_frame.plane_addr[0] = NULL;
> }
>
> return 0;
> @@ -487,8 +487,8 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
> }
> }
>
> -static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> - const void *sink_frame)
> +static struct vimc_frame *vimc_deb_process_frame(struct vimc_ent_device *ved,
> + const struct vimc_frame *sink_frame)
> {
> struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
> ved);
> @@ -496,16 +496,17 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> unsigned int i, j;
>
> /* If the stream in this node is not active, just return */
> - if (!vdeb->src_frame)
> + if (!vdeb->src_frame.plane_addr[0])
> return ERR_PTR(-EINVAL);
>
> for (i = 0; i < vdeb->sink_fmt.height; i++)
> for (j = 0; j < vdeb->sink_fmt.width; j++) {
> - vimc_deb_calc_rgb_sink(vdeb, sink_frame, i, j, rgb);
> + vimc_deb_calc_rgb_sink(vdeb, sink_frame->plane_addr[0],
> + i, j, rgb);

please align

Regards,
Helen

> vdeb->set_rgb_src(vdeb, i, j, rgb);
> }
>
> - return vdeb->src_frame;
> + return &vdeb->src_frame;
>
> }
>
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 6e88328dca5c..65519495ecca 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -50,7 +50,7 @@ struct vimc_sca_device {
> */
> struct v4l2_mbus_framefmt sink_fmt;
> /* Values calculated when the stream starts */
> - u8 *src_frame;
> + struct vimc_frame src_frame;
> unsigned int src_line_size;
> unsigned int bpp;
> };
> @@ -234,16 +234,17 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
> /* Allocate the frame buffer. Use vmalloc to be able to
> * allocate a large amount of memory
> */
> - vsca->src_frame = vmalloc(frame_size);
> - if (!vsca->src_frame)
> + vsca->src_frame.plane_addr[0] = vmalloc(frame_size);
> + vsca->src_frame.sizeimage[0] = frame_size;
> + if (!vsca->src_frame.plane_addr[0])
> return -ENOMEM;
>
> } else {
> - if (!vsca->src_frame)
> + if (!vsca->src_frame.plane_addr[0])
> return 0;
>
> - vfree(vsca->src_frame);
> - vsca->src_frame = NULL;
> + vfree(vsca->src_frame.plane_addr[0]);
> + vsca->src_frame.plane_addr[0] = NULL;
> }
>
> return 0;
> @@ -306,8 +307,9 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> vsca->sd.name, index + j);
>
> /* copy the pixel to the position index + j */
> - vimc_sca_fill_pix(&vsca->src_frame[index + j],
> - pixel, vsca->bpp);
> + vimc_sca_fill_pix(
> + &vsca->src_frame.plane_addr[0][index + j],
> + pixel, vsca->bpp);
> }
>
> /* move the index to the next line */
> @@ -327,8 +329,8 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
> vimc_sca_scale_pix(vsca, i, j, sink_frame);
> }
>
> -static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> - const void *sink_frame)
> +static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
> + const struct vimc_frame *sink_frame)
> {
> struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> ved);
> @@ -337,9 +339,9 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> if (!ved->stream)
> return ERR_PTR(-EINVAL);
>
> - vimc_sca_fill_src_frame(vsca, sink_frame);
> + vimc_sca_fill_src_frame(vsca, sink_frame->plane_addr[0]);
>
> - return vsca->src_frame;
> + return &vsca->src_frame;
> };
>
> static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index e60f1985edb0..020651320ac9 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -36,7 +36,7 @@ struct vimc_sen_device {
> struct device *dev;
> struct tpg_data tpg;
> struct task_struct *kthread_sen;
> - u8 *frame;
> + struct vimc_frame frame;
> /* The active format */
> struct v4l2_mbus_framefmt mbus_format;
> struct v4l2_ctrl_handler hdl;
> @@ -177,14 +177,14 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
> .set_fmt = vimc_sen_set_fmt,
> };
>
> -static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
> - const void *sink_frame)
> +static struct vimc_frame *vimc_sen_process_frame(struct vimc_ent_device *ved,
> + const struct vimc_frame *sink_frame)
> {
> struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
> ved);
>
> - tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
> - return vsen->frame;
> + tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame.plane_addr[0]);
> + return &vsen->frame;
> }
>
> static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
> @@ -206,8 +206,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
> * Allocate the frame buffer. Use vmalloc to be able to
> * allocate a large amount of memory
> */
> - vsen->frame = vmalloc(frame_size);
> - if (!vsen->frame)
> + vsen->frame.plane_addr[0] = vmalloc(frame_size);
> + if (!vsen->frame.plane_addr[0])
> return -ENOMEM;
>
> /* configure the test pattern generator */
> @@ -215,8 +215,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>
> } else {
>
> - vfree(vsen->frame);
> - vsen->frame = NULL;
> + vfree(vsen->frame.plane_addr[0]);
> + vsen->frame.plane_addr[0] = NULL;
> return 0;
> }
>
> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> index c19093b6c787..efbc6adc34be 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.c
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -124,7 +124,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> static int vimc_streamer_thread(void *data)
> {
> struct vimc_stream *stream = data;
> - u8 *frame = NULL;
> + struct vimc_frame *frame = NULL;
> int i;
>
> set_freezable();
>