Hi André,
Thanks for the patch, please see my comments below.
On 4/24/19 10:56 AM, André Almeida wrote:
> Change the scaling functions in order to scale planes. This change makes
> easier to support multiplanar pixel formats.
>
> Signed-off-by: André Almeida <andrealmeid@xxxxxxxxxxxxx>
> ---
> Change in v3:
> - Adapt to new vimc_frame
>
> Changes in v2:
> - Move the TODO comment to vimc-capture
> - Reuse and share the code to free the memory of planes with a goto
> - Fix comment block style
>
> drivers/media/platform/vimc/vimc-scaler.c | 113 ++++++++++++++--------
> 1 file changed, 72 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index f655b8312dc9..17c37076ff4e 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -39,6 +39,9 @@ static const u32 vimc_sca_supported_pixfmt[] = {
> V4L2_PIX_FMT_BGR24,
> V4L2_PIX_FMT_RGB24,
> V4L2_PIX_FMT_ARGB32,
> + V4L2_PIX_FMT_YUV420,
> + V4L2_PIX_FMT_YUV420M,
> + V4L2_PIX_FMT_NV12M,
> };
>
> struct vimc_sca_device {
> @@ -51,8 +54,8 @@ struct vimc_sca_device {
> struct v4l2_mbus_framefmt sink_fmt;
> /* Values calculated when the stream starts */
> struct vimc_frame src_frame;
> - unsigned int src_line_size;
> - unsigned int bpp;
> + unsigned int src_line_size[TPG_MAX_PLANES];
> + unsigned int bpp[TPG_MAX_PLANES];
> };
>
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> @@ -207,10 +210,10 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
> static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> + unsigned int i, ret = 0;
int ret
>
> if (enable) {
> u32 pixelformat = vsca->ved.stream->producer_pixfmt;
> - const struct v4l2_format_info *pix_info;
> unsigned int frame_size;
>
> if (!vimc_sca_is_pixfmt_supported(pixelformat)) {
> @@ -219,34 +222,47 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
> return -EINVAL;
> }
>
> - /* Save the bytes per pixel of the sink */
> - pix_info = v4l2_format_info(pixelformat);
> - vsca->bpp = pix_info->bpp[0];
> -
> - /* Calculate the width in bytes of the src frame */
> - vsca->src_line_size = vsca->sink_fmt.width *
> - sca_mult * vsca->bpp;
> -
> - /* Calculate the frame size of the source pad */
> - frame_size = vsca->src_line_size * vsca->sink_fmt.height *
> - sca_mult;
> -
> - /* Allocate the frame buffer. Use vmalloc to be able to
> - * allocate a large amount of memory
> - */
> - vsca->src_frame.plane_addr[0] = vmalloc(frame_size);
> - if (!vsca->src_frame.plane_addr[0])
> - return -ENOMEM;
> + vimc_fill_frame(&vsca->src_frame, pixelformat,
> + vsca->sink_fmt.width, vsca->sink_fmt.height,
> + vsca->ved.stream->multiplanar);
> +
> + for (i = 0; i < vsca->src_frame.fmt.num_planes; i++) {
> + /* Save the bytes per pixel of the sink */
> + vsca->bpp[i] = vsca->src_frame.fmt_info->bpp[i];
> +
> + /* Calculate the width in bytes of the src frame */
> + vsca->src_line_size[i] =
> + vsca->src_frame.fmt.plane_fmt[i].bytesperline *
> + sca_mult;
> +
> + /* Calculate the frame size of the source pad */
> + frame_size = vsca->src_frame.fmt.plane_fmt[i].sizeimage
> + * sca_mult * sca_mult;
> +
> + /**
> + * Allocate the frame buffer. Use vmalloc to be able to
> + * allocate a large amount of memory
> + */
> + vsca->src_frame.plane_addr[i] = vmalloc(frame_size);
> + if (!vsca->src_frame.plane_addr[i]) {
> + ret = -ENOMEM;
> + goto free_planes;
> + }
> + vsca->src_frame.fmt.plane_fmt[i].sizeimage = frame_size;
> + }
>
> } else {
> if (!vsca->src_frame.plane_addr[0])
> return 0;
>
> - vfree(vsca->src_frame.plane_addr[0]);
> - vsca->src_frame.plane_addr[0] = NULL;
> +free_planes:
please, prefer to have goto labels in the end and outside and if/else
statement.
> + for (i = 0; i < vsca->src_frame.fmt.num_planes; i++) {
> + vfree(vsca->src_frame.plane_addr[i]);
> + vsca->src_frame.plane_addr[i] = NULL;
> + }
> }
>
> - return 0;
> + return ret;
> }
>
> static const struct v4l2_subdev_video_ops vimc_sca_video_ops = {
> @@ -269,18 +285,19 @@ static void vimc_sca_fill_pix(u8 *const ptr,
> ptr[i] = pixel[i];
> }
>
> -static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> +/* TODO: parallelize this function */
> +static void vimc_sca_scale_plane(const struct vimc_sca_device *const vsca,
I think you could keep the old name, the function stills scale one
single pixel in a given plane.
> const unsigned int lin, const unsigned int col,
> - const u8 *const sink_frame)
> + const struct vimc_frame *sink_frame,
> + u8 num_plane, u32 width)
why not unsigned int for num_plane and for width ? :)
You can also align the indentation of the arguments here.
> +
> {
> unsigned int i, j, index;
> const u8 *pixel;
>
> /* Point to the pixel value in position (lin, col) in the sink frame */
> - index = VIMC_FRAME_INDEX(lin, col,
> - vsca->sink_fmt.width,
> - vsca->bpp);
> - pixel = &sink_frame[index];
> + index = VIMC_FRAME_INDEX(lin, col, width, vsca->bpp[num_plane]);
> + pixel = &sink_frame->plane_addr[num_plane][index];
>
> dev_dbg(vsca->dev,
> "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
> @@ -290,7 +307,7 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> * in the scaled src frame
> */
> index = VIMC_FRAME_INDEX(lin * sca_mult, col * sca_mult,
> - vsca->sink_fmt.width * sca_mult, vsca->bpp);
> + width * sca_mult, vsca->bpp[num_plane]);
>
> dev_dbg(vsca->dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
> vsca->sd.name, lin * sca_mult, col * sca_mult, index);
> @@ -300,32 +317,46 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> /* Iterate through each beginning of a
> * pixel repetition in a line
> */
> - for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
> + unsigned int bpp = vsca->bpp[num_plane];
> +
> + for (j = 0; j < sca_mult * bpp; j += bpp) {
> dev_dbg(vsca->dev,
> "sca: %s: sca: scale_pix src pos %d\n",
> vsca->sd.name, index + j);
>
> /* copy the pixel to the position index + j */
> vimc_sca_fill_pix(
> - &vsca->src_frame.plane_addr[0][index + j],
> - pixel, vsca->bpp);
> + &vsca->src_frame.plane_addr[num_plane][index+j],
> + pixel, bpp);
> }
>
> /* move the index to the next line */
> - index += vsca->src_line_size;
> + index += vsca->src_line_size[num_plane];
> }
> }
>
> static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
> - const u8 *const sink_frame)
> + const struct vimc_frame *sink_frame)
> {
> - unsigned int i, j;
> + unsigned int i, j, num_plane;
> + u32 width, height;
why not unsigned int?
> + const struct v4l2_format_info *info;
> +
> + info = v4l2_format_info(sink_frame->fmt_info->format);
info and sink_frame->fmt_info should be the same, you don't need to call
this function.
>
> /* Scale each pixel from the original sink frame */
> /* TODO: implement scale down, only scale up is supported for now */
> - for (i = 0; i < vsca->sink_fmt.height; i++)
> - for (j = 0; j < vsca->sink_fmt.width; j++)
> - vimc_sca_scale_pix(vsca, i, j, sink_frame);
> + for (num_plane = 0; num_plane < info->comp_planes; num_plane++) {
are you sure it is not info->mem_planes that you should use?
You can also use sink_frame->fmt.num_planes.
> + width = vsca->sink_fmt.width /
> + ((num_plane == 0) ? 1 : info->vdiv);
> + height = vsca->sink_fmt.height /
> + ((num_plane == 0) ? 1 : info->hdiv);
these things always confuses me, one is plane width/height, the other is
image width/height, maybe we could rename those to plane_width and
plane_height to make it clearer, what do you think?
You can also declare them inside this loop to make it clear it is just
used here (this applies to other patches as well).
Helen
> +
> + for (i = 0; i < height; i++)
> + for (j = 0; j < width; j++)
> + vimc_sca_scale_plane(vsca, i, j, sink_frame,
> + num_plane, width);
> + }
> }
>
> static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
> @@ -338,7 +369,7 @@ static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
> if (!ved->stream)
> return ERR_PTR(-EINVAL);
>
> - vimc_sca_fill_src_frame(vsca, sink_frame->plane_addr[0]);
> + vimc_sca_fill_src_frame(vsca, sink_frame);
>
> return &vsca->src_frame;
> };
>
