Hi André,
On 4/24/19 10:56 AM, André Almeida wrote:
> This commit adapts vimc-sensor to handle multiplanar pixel formats,
> adapting the memory allocation and TPG configuration.
>
> Signed-off-by: André Almeida <andrealm...@collabora.com>
> ---
> Changes in v3:
> - Adapt to new vimc_frame
>
> Changes in v2:
> - Fix alignment issues
> - Remove unnecessary variable assignment
> - Reuse and share the code to free the memory of planes with a goto
>
> drivers/media/platform/vimc/vimc-sensor.c | 52 +++++++++++++----------
> 1 file changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c
> b/drivers/media/platform/vimc/vimc-sensor.c
> index 3495a3f3dd60..d933a1db309c 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -97,16 +97,17 @@ static int vimc_sen_get_fmt(struct v4l2_subdev *sd,
> static void vimc_sen_tpg_s_format(struct vimc_sen_device *vsen)
> {
> u32 pixelformat = vsen->ved.stream->producer_pixfmt;
> - const struct v4l2_format_info *pix_info;
> -
> - pix_info = v4l2_format_info(pixelformat);
> + unsigned int i;
>
> tpg_reset_source(&vsen->tpg, vsen->mbus_format.width,
> vsen->mbus_format.height, vsen->mbus_format.field);
> - tpg_s_bytesperline(&vsen->tpg, 0,
> - vsen->mbus_format.width * pix_info->bpp[0]);
> tpg_s_buf_height(&vsen->tpg, vsen->mbus_format.height);
> tpg_s_fourcc(&vsen->tpg, pixelformat);
> +
> + for (i = 0; i < tpg_g_planes(&vsen->tpg); i++)
> + tpg_s_bytesperline(&vsen->tpg, i,
> + vsen->frame.fmt.plane_fmt[i].bytesperline);
> +
> /* TODO: add support for V4L2_FIELD_ALTERNATE */
> tpg_s_field(&vsen->tpg, vsen->mbus_format.field, false);
> tpg_s_colorspace(&vsen->tpg, vsen->mbus_format.colorspace);
> @@ -182,8 +183,12 @@ static struct vimc_frame *vimc_sen_process_frame(struct
> vimc_ent_device *ved,
> {
> struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
> ved);
> + unsigned int i;
> +
> + for (i = 0; i < tpg_g_planes(&vsen->tpg); i++)
> + tpg_fill_plane_buffer(&vsen->tpg, 0, i,
> + vsen->frame.plane_addr[i]);
>
> - tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame.plane_addr[0]);
> return &vsen->frame;
> }
>
> @@ -191,36 +196,39 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd,
> int enable)
> {
> struct vimc_sen_device *vsen =
> container_of(sd, struct vimc_sen_device, sd);
> + unsigned int i, ret = 0;
ret should be int, not unsigned int, this also applies to other patches.
>
> if (enable) {
> - u32 pixelformat = vsen->ved.stream->producer_pixfmt;
> - const struct v4l2_format_info *pix_info;
> - unsigned int frame_size;
> -
> /* Calculate the frame size */
> - pix_info = v4l2_format_info(pixelformat);
> - frame_size = vsen->mbus_format.width * pix_info->bpp[0] *
> - vsen->mbus_format.height;
> -
> + vimc_fill_frame(&vsen->frame, vsen->ved.stream->producer_pixfmt,
> + vsen->mbus_format.width,
> + vsen->mbus_format.height,
> + vsen->ved.stream->multiplanar);
> /*
> * Allocate the frame buffer. Use vmalloc to be able to
> * allocate a large amount of memory
> */
> - vsen->frame.plane_addr[0] = vmalloc(frame_size);
> - if (!vsen->frame.plane_addr[0])
> - return -ENOMEM;
> + for (i = 0; i < vsen->frame.fmt.num_planes; i++) {
> + vsen->frame.plane_addr[i] =
> + vmalloc(vsen->frame.fmt.plane_fmt[i].sizeimage);
> + if (!vsen->frame.plane_addr[i]) {
> + ret = -ENOMEM;
> + goto free_planes;
> + }
> + }
>
> /* configure the test pattern generator */
> vimc_sen_tpg_s_format(vsen);
>
> } else {
> -
> - vfree(vsen->frame.plane_addr[0]);
> - vsen->frame.plane_addr[0] = NULL;
> - return 0;
> +free_planes:
Hmm, its a bit weird to have a goto going inside an else statement.
It would be better if you remove the else statement, and add a return 0
in the end of the if statement.
This also applies to other patches.
Helen
> + for (i = 0; i < vsen->frame.fmt.num_planes; i++) {
> + vfree(vsen->frame.plane_addr[i]);
> + vsen->frame.plane_addr[i] = NULL;
> + }
> }
>
> - return 0;
> + return ret;
> }
>
> static const struct v4l2_subdev_core_ops vimc_sen_core_ops = {
>
