Re: [PATCH 6/7] [media] vimc: sca: Add scaler subdevice

Laurent Pinchart
2015-08-13T23:51:44Z

Hi Helen,

Thank you for the patch.

On Thursday 06 August 2015 17:26:13 Helen Fornazier wrote:
> Implement scaler and integrated with the core
>
> Signed-off-by: Helen Fornazier <helen.fornazier@gmail.com>
> ---
> drivers/media/platform/vimc/Makefile | 3 +-
> drivers/media/platform/vimc/vimc-core.c | 6 +-
> drivers/media/platform/vimc/vimc-scaler.c | 321 +++++++++++++++++++++++++++
> drivers/media/platform/vimc/vimc-scaler.h | 28 +++
> 4 files changed, 356 insertions(+), 2 deletions(-)
> create mode 100644 drivers/media/platform/vimc/vimc-scaler.c
> create mode 100644 drivers/media/platform/vimc/vimc-scaler.h

[snip]

> diff --git a/drivers/media/platform/vimc/vimc-scaler.c
> b/drivers/media/platform/vimc/vimc-scaler.c new file mode 100644
> index 0000000..ea26930
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -0,0 +1,321 @@
> +/*
> + * vimc-scaler.c Virtual Media Controller Driver
> + *
> + * Copyright (C) 2015 Helen Fornazier <helen.fornazier@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/freezer.h>

I don't think this header is needed. Same for patch 5/7.

> +#include <linux/vmalloc.h>
> +#include <linux/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include "vimc-scaler.h"
> +
> +/* TODO: add this as a parameter of this module */
> +#define VIMC_SCA_MULTIPLIER 3
> +
> +struct vimc_sca_device {
> + struct vimc_ent_subdevice vsd;
> + unsigned int mult;
> + /* The active format */
> + struct v4l2_mbus_framefmt sink_mbus_fmt;
> + unsigned int src_width;
> + unsigned int src_height;
> + /* Values calculated when the stream starts */
> + u8 *src_frame;
> + unsigned int src_frame_size;
> + unsigned int src_line_size;
> + unsigned int bpp;
> +};
> +
> +static int vimc_sca_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> +
> + /* Check if it is a valid pad */
> + if (code->pad >= vsca->vsd.sd.entity.num_pads)
> + return -EINVAL;

You should check code->index (here and in enum_frame_size) as explained in my
reply to patch 5/7. Please also consider the other comments I made for that
patch when they're applicable here.

> + code->code = vsca->sink_mbus_fmt.code;
> +
> + return 0;
> +}
> +
> +static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> + struct media_pad *pad;
> +
> + /* Check if it is a valid pad */
> + if (fse->pad >= vsca->vsd.sd.entity.num_pads)
> + return -EINVAL;
> +
> + /* TODO: Add support to other formats sizes */
> +
> + pad = &vsca->vsd.sd.entity.pads[fse->pad];
> + if ((pad->flags & MEDIA_PAD_FL_SOURCE)) {
> + fse->min_width = vsca->src_width;
> + fse->max_width = vsca->src_width;
> + fse->min_height = vsca->src_height;
> + fse->max_height = vsca->src_height;
> + } else if ((pad->flags & MEDIA_PAD_FL_SINK)) {
> + fse->min_width = vsca->sink_mbus_fmt.width;
> + fse->max_width = vsca->sink_mbus_fmt.width;
> + fse->min_height = vsca->sink_mbus_fmt.height;
> + fse->max_height = vsca->sink_mbus_fmt.height;
> + } else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> + struct media_pad *pad;
> +
> + /* Check if it is a valid pad */
> + if (format->pad >= vsca->vsd.sd.entity.num_pads)
> + return -EINVAL;
> +
> + pad = &vsca->vsd.sd.entity.pads[format->pad];
> + if ((pad->flags & MEDIA_PAD_FL_SOURCE)) {
> + format->format = vsca->sink_mbus_fmt;
> + format->format.width = vsca->src_width;
> + format->format.height = vsca->src_height;
> + } else if ((pad->flags & MEDIA_PAD_FL_SINK))
> + format->format = vsca->sink_mbus_fmt;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
> + .enum_mbus_code = vimc_sca_enum_mbus_code,
> + .enum_frame_size = vimc_sca_enum_frame_size,
> + .get_fmt = vimc_sca_get_fmt,
> + /* TODO: Add support to other formats */
> + .set_fmt = vimc_sca_get_fmt,
> +};
> +
> +static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> +
> + if (enable) {
> + const struct vimc_pix_map *vpix;
> +
> + if (vsca->src_frame)
> + return -EINVAL;
> +
> + /* Save the bytes per pixel of the sink */
> + vpix = vimc_pix_map_by_code(vsca->sink_mbus_fmt.code);
> + /* This should never be NULL, as we won't allow any format
> + * other then the ones in the vimc_pix_map_list table */
> + BUG_ON(!vpix);
> + vsca->bpp = vpix->bpp;
> +
> + /* Calculate the width in bytes of the src frame */
> + vsca->src_line_size = vsca->src_width * vsca->bpp;
> +
> + /* Calculate the frame size of the source pad */
> + vsca->src_frame_size = vsca->src_line_size * vsca->src_height;
> +
> + /* Allocate the frame buffer. Use vmalloc to be able to
> + * allocate a large amount of memory*/
> + vsca->src_frame = vmalloc(vsca->src_frame_size);
> + if (!vsca->src_frame)
> + return -ENOMEM;
> +
> + /* Turn the stream on in the subdevices directly connected */
> + if (vimc_pipeline_s_stream(&vsca->vsd.sd.entity, 1)) {
> + vfree(vsca->src_frame);
> + vsca->src_frame = NULL;
> + return -EINVAL;
> + }
> + } else {
> + if (!vsca->src_frame)
> + return -EINVAL;
> + vfree(vsca->src_frame);
> + vsca->src_frame = NULL;
> + vimc_pipeline_s_stream(&vsca->vsd.sd.entity, 0);
> + }
> +
> + return 0;
> +}
> +
> +struct v4l2_subdev_video_ops vimc_sca_video_ops = {
> + .s_stream = vimc_sca_s_stream,
> +};
> +
> +static const struct v4l2_subdev_ops vimc_sca_ops = {
> + .pad = &vimc_sca_pad_ops,
> + .video = &vimc_sca_video_ops,
> +};
> +
> +static void vimc_sca_fill_pix(u8 *const ptr,
> + const u8 *const pixel,
> + const unsigned int bpp)
> +{
> + unsigned int i;
> +
> + /* copy the pixel to the pointer */
> + for (i = 0; i < bpp; i++)
> + ptr[i] = pixel[i];
> +}
> +
> +static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> + const unsigned int lin, const unsigned int col,
> + const u8 *const sink_frame)
> +{
> + 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_mbus_fmt.width,
> + vsca->bpp);
> + pixel = &sink_frame[index];
> +
> + dev_dbg(vsca->vsd.dev,
> + "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
> + vsca->vsd.sd.name, lin, col, index);
> +
> + /* point to the place we are going to put the first pixel
> + * in the scaled src frame */
> + index = VIMC_FRAME_INDEX(lin * vsca->mult, col * vsca->mult,
> + vsca->src_width, vsca->bpp);
> +
> + dev_dbg(vsca->vsd.dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
> + vsca->vsd.sd.name, lin * vsca->mult, col * vsca->mult, index);
> +
> + /* Repeat this pixel mult times */
> + for (i = 0; i < vsca->mult; i++) {
> + /* Iterate though each beginning of a
> + * pixel repetition in a line */
> + for (j = 0; j < vsca->mult * vsca->bpp; j += vsca->bpp) {
> + dev_dbg(vsca->vsd.dev,
> + "sca: %s: sca: scale_pix src pos %d\n",
> + vsca->vsd.sd.name, index + j);
> +
> + /* copy the pixel to the position index + j */
> + vimc_sca_fill_pix(&vsca->src_frame[index + j],
> + pixel, vsca->bpp);
> + }
> +
> + /* move the index to the next line */
> + index += vsca->src_line_size;
> + }
> +}
> +
> +static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const
> vsca,
> + const u8 *const sink_frame)
> +{
> + unsigned int i, j;
> +
> + /* 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_mbus_fmt.height; i++)
> + for (j = 0; j < vsca->sink_mbus_fmt.width; j++)
> + vimc_sca_scale_pix(vsca, i, j, sink_frame);
> +}
> +
> +static void vimc_sca_process_frame(struct vimc_ent_device *ved,
> + struct media_pad *sink,
> + const void *sink_frame)
> +{
> + unsigned int i;
> + struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> + vsd.ved);
> +
> + /* If the stream in this node is not active, just return */
> + if (!vsca->src_frame)
> + return;
> +
> + vimc_sca_fill_src_frame(vsca, sink_frame);
> +
> + /* Propagate the frame thought all source pads */
> + for (i = 0; i < vsca->vsd.sd.entity.num_pads; i++) {
> + struct media_pad *pad = &vsca->vsd.sd.entity.pads[i];
> +
> + if (pad->flags & MEDIA_PAD_FL_SOURCE)
> + vimc_propagate_frame(vsca->vsd.dev,
> + pad, vsca->src_frame);
> + }
> +};
> +
> +static void vimc_sca_destroy(struct vimc_ent_device *ved)
> +{
> + struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> + vsd.ved);
> +
> + vimc_ent_sd_cleanup(&vsca->vsd);
> +}
> +
> +struct vimc_ent_device *vimc_sca_create(struct v4l2_device *v4l2_dev,
> + const char *const name,
> + u16 num_pads,
> + const unsigned long *pads_flag)
> +{
> + int ret;
> + struct vimc_sca_device *vsca;
> + struct vimc_ent_subdevice *vsd;
> +
> + vsd = vimc_ent_sd_init(sizeof(struct vimc_sca_device),
> + v4l2_dev, name, num_pads, pads_flag,
> + &vimc_sca_ops, vimc_sca_destroy);
> + if (IS_ERR(vsd))
> + return (struct vimc_ent_device *)vsd;
> +
> + vsca = container_of(vsd, struct vimc_sca_device, vsd);
> +
> + /* Set the default active frame format (this is hardcoded for now) */
> + vsca->sink_mbus_fmt.width = 640;
> + vsca->sink_mbus_fmt.height = 480;
> + vsca->sink_mbus_fmt.code = MEDIA_BUS_FMT_RGB888_1X24;
> + vsca->sink_mbus_fmt.field = V4L2_FIELD_NONE;
> + vsca->sink_mbus_fmt.colorspace = V4L2_COLORSPACE_SRGB;
> + vsca->sink_mbus_fmt.quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + vsca->sink_mbus_fmt.xfer_func = V4L2_XFER_FUNC_SRGB;
> +
> + /* Set the scaler multiplier factor */
> + vsca->mult = VIMC_SCA_MULTIPLIER;
> +
> + /* Calculate the width and the height of the src frame */
> + vsca->src_width = vsca->sink_mbus_fmt.width * vsca->mult;
> + vsca->src_height = vsca->sink_mbus_fmt.height * vsca->mult;
> +
> + /* Set the process frame callback */
> + vsca->vsd.ved.process_frame = vimc_sca_process_frame;
> +
> + /* Register the subdev with the v4l2 and the media framework */
> + ret = v4l2_device_register_subdev(vsca->vsd.v4l2_dev, &vsca->vsd.sd);
> + if (ret) {
> + dev_err(vsca->vsd.dev,
> + "subdev register failed (err=%d)\n", ret);
> +
> + vimc_ent_sd_cleanup(vsd);
> +
> + return ERR_PTR(ret);
> + }
> +
> + return &vsca->vsd.ved;
> +}
> diff --git a/drivers/media/platform/vimc/vimc-scaler.h
> b/drivers/media/platform/vimc/vimc-scaler.h new file mode 100644
> index 0000000..863278f
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-scaler.h
> @@ -0,0 +1,28 @@
> +/*
> + * vimc-scaler.h Virtual Media Controller Driver
> + *
> + * Copyright (C) 2015 Helen Fornazier <helen.fornazier@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _VIMC_SCALER_H_
> +#define _VIMC_SCALER_H_
> +
> +#include "vimc-core.h"
> +
> +struct vimc_ent_device *vimc_sca_create(struct v4l2_device *v4l2_dev,
> + const char *const name,
> + u16 num_pads,
> + const unsigned long *pads_flag);
> +
> +#endif

--
Regards,

Laurent Pinchart