Hi Matsubara-san,
On Wednesday 24 July 2013 19:38:39 Katsuya MATSUBARA wrote:
> Hi Laurent,
>
> Thank you for your great work for VSP1.
> Some comments below.
Thank you for the review.
> From: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> Date: Wed, 17 Jul 2013 16:54:42 +0200
>
> (snip)
>
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > +/*
> > + * vsp1_drv.c -- R-Car VSP1 Driver
> > + *
> > + * Copyright (C) 2013 Renesas Corporation
> > + *
> > + * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/videodev2.h>
> > +
> > +#include "vsp1.h"
> > +#include "vsp1_lif.h"
> > +#include "vsp1_rwpf.h"
> > +#include "vsp1_uds.h"
> > +
> > +/*
> > -------------------------------------------------------------------------
> > ---- + * Interrupt Handling
> > + */
> > +
> > +static irqreturn_t vsp1_irq_handler(int irq, void *data)
> > +{
> > + struct vsp1_device *vsp1 = data;
> > + irqreturn_t ret = IRQ_NONE;
> > + unsigned int i;
> > +
> > + for (i = 0; i < VPS1_MAX_WPF; ++i) {
> > + struct vsp1_rwpf *wpf = vsp1->wpf[i];
> > + struct vsp1_pipeline *pipe;
> > + u32 status;
> > +
> > + if (wpf == NULL)
> > + continue;
> > +
> > + pipe = to_vsp1_pipeline(&wpf->entity.subdev.entity);
> > + status = vsp1_read(vsp1, VI6_WPF_IRQ_STA(i));
> > + vsp1_write(vsp1, VI6_WPF_IRQ_STA(i), ~status);
>
> The value set into the VI6_WPF_IRQ_STA register should be masked with
> VI6_WFP_IRQ_STA_FRE since unused (upper) bits in the register must be
> written with 0.
Good point. I'll fix that.
> > +static int vsp1_create_links(struct vsp1_device *vsp1, struct vsp1_entity
> > *sink) +{
> > + struct media_entity *entity = &sink->subdev.entity;
> > + struct vsp1_entity *source;
> > + unsigned int pad;
> > + int ret;
> > +
> > + list_for_each_entry(source, &vsp1->entities, list_dev) {
> > + u32 flags;
> > +
> > + if (source->type == sink->type)
> > + continue;
> > +
> > + if (source->type == VSP1_ENTITY_LIF ||
> > + source->type == VSP1_ENTITY_WPF)
> > + continue;
> > +
> > + flags = source->type == VSP1_ENTITY_RPF &&
> > + sink->type == VSP1_ENTITY_WPF &&
> > + source->index == sink->index
> > + ? MEDIA_LNK_FL_ENABLED : 0;
> > +
> > + for (pad = 0; pad < entity->num_pads; ++pad) {
> > + if (!(entity->pads[pad].flags & MEDIA_PAD_FL_SINK))
> > + continue;
> > +
> > + ret = media_entity_create_link(&source->subdev.entity,
> > + source->source_pad,
> > + entity, pad, flags);
> > + if (ret < 0)
> > + return ret;
>
> This initialization enables some of links as the initial status.
> I think link_setup() for each linked entity should be invoked here
> to set up the sink value in the vsp_entity structure.
>
> (snip)
>
> > +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > @@ -0,0 +1,581 @@
> > +/*
> > + * vsp1_regs.h -- R-Car VSP1 Registers Definitions
> > + *
> > + * Copyright (C) 2013 Renesas Electronics Corporation
> > + *
> > + * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2
> > + * as published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __VSP1_REGS_H__
> > +#define __VSP1_REGS_H__
> > +
>
> (snip)
>
> > +/*
> > -------------------------------------------------------------------------
> > ---- + * HGO Control Registers
> > + */
> > +
> > +#define VI6_HGO_OFFSET 0x3000
> > +#define VI6_HGO_SIZE 0x3004
> > +#define VI6_HGO_MODE 0x3008
> > +#define VI6_HGO_LB_TH 0x300c
> > +#define VI6_HGO_LBn_H (0x3010 + (n) * 8)
> > +#define VI6_HGO_LBn_V (0x3014 + (n) * 8)
>
> It looks like the 'n' argument is missing for VI6_HGO_LBn_H
> and VI6_HGO_LBn_V.
Will fix as well.
> > +#define VI6_HGO_R_HISTO 0x3030
> > +#define VI6_HGO_R_MAXMIN 0x3130
> > +#define VI6_HGO_R_SUM 0x3134
> > +#define VI6_HGO_R_LB_DET 0x3138
> > +#define VI6_HGO_G_HISTO 0x3140
> > +#define VI6_HGO_G_MAXMIN 0x3240
> > +#define VI6_HGO_G_SUM 0x3244
> > +#define VI6_HGO_G_LB_DET 0x3248
> > +#define VI6_HGO_B_HISTO 0x3250
> > +#define VI6_HGO_B_MAXMIN 0x3350
> > +#define VI6_HGO_B_SUM 0x3354
> > +#define VI6_HGO_B_LB_DET 0x3358
> > +#define VI6_HGO_REGRST 0x33fc
> > +
> > +/*
> > -------------------------------------------------------------------------
> > ---- + * HGT Control Registers
> > + */
> > +
> > +#define VI6_HGT_OFFSET 0x3400
> > +#define VI6_HGT_SIZE 0x3404
> > +#define VI6_HGT_MODE 0x3408
> > +#define VI6_HGT_HUE_AREA(n) (0x340c + (n) * 4)
> > +#define VI6_HGT_LB_TH 0x3424
> > +#define VI6_HGT_LBn_H (0x3438 + (n) * 8)
> > +#define VI6_HGT_LBn_V (0x342c + (n) * 8)
>
> The 'n' arguments for VI6_HGT_LBn_H and VI6_HGT_LBn_H are missing too.
Will fix as well.
> (snip)
>
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -0,0 +1,1129 @@
> > +/*
> > + * vsp1_video.c -- R-Car VSP1 Video Node
> > + *
> > + * Copyright (C) 2013 Renesas Corporation
> > + *
> > + * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> > + *
> > + * 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.
> > + */
> > +
>
> (snip)
>
> > +static int __vsp1_video_try_format(struct vsp1_video *video,
> > + struct v4l2_pix_format_mplane *pix,
> > + const struct vsp1_format_info **fmtinfo)
> > +{
> > + const struct vsp1_format_info *info;
> > + unsigned int width = pix->width;
> > + unsigned int height = pix->height;
> > + unsigned int i;
> > +
> > + /* Retrieve format information and select the default format if the
> > + * requested format isn't supported.
> > + */
> > + info = vsp1_get_format_info(pix->pixelformat);
> > + if (info == NULL)
> > + info = vsp1_get_format_info(VSP1_VIDEO_DEF_FORMAT);
> > +
> > + pix->pixelformat = info->fourcc;
> > + pix->colorspace = V4L2_COLORSPACE_SRGB;
> > + pix->field = V4L2_FIELD_NONE;
> > + memset(pix->reserved, 0, sizeof(pix->reserved));
> > +
> > + /* Align the width and height for YUV 4:2:2 and 4:2:0 formats. */
> > + width = round_down(width, info->hsub);
> > + height = round_down(height, info->vsub);
> > +
> > + /* Clamp the width and height. */
> > + pix->width = clamp(width, VSP1_VIDEO_MIN_WIDTH,
VSP1_VIDEO_MAX_WIDTH);
> > + pix->height = clamp(height, VSP1_VIDEO_MIN_HEIGHT,
> > + VSP1_VIDEO_MAX_HEIGHT);
> > +
> > + /* Compute and clamp the stride and image size. */
> > + for (i = 0; i < max(info->planes, 2U); ++i) {
> > + unsigned int hsub = i > 0 ? info->hsub : 1;
> > + unsigned int vsub = i > 0 ? info->vsub : 1;
> > + unsigned int bpl;
> > +
> > + bpl = clamp_t(unsigned int, pix->plane_fmt[i].bytesperline,
> > + pix->width / hsub * info->bpp[i] / 8,
> > + round_down(65535U, 128));
> > +
> > + pix->plane_fmt[i].bytesperline = round_up(bpl, 128);
>
> I am not sure why 'bytesperlines' should be aligned to 128 bytes. VSP1
> doesn't seem to have this limitation.
If I recall correctly, strides lower than 128 pixels resulted in corruption in
the output image. Please see the attached file for an example. That was in the
beginning of VSP1 development so the problem might have been unrelated and
fixed by something completely different, it would be worth it retrying with
different alignment values.
> > + pix->plane_fmt[i].sizeimage = bpl * pix->height / vsub;
>
> If the round up for bytesperlines is required, sizeimage should be
> calculated with 'bytesperline' rather than 'bpl'.
Indeed, I will fix that.
> Actually, I had been implementing a V4L2 MEM2MEM driver for VIO6, which is
> an older version of the VSP1 used by many of the current Renesas SoCs.
>
> VSP1 should be compatible with VIO6, so I rebased my work onto your
> implementation. I will post a patch to add support of VIO6 for your code
> once it is finished.
That would be great, thank you. By the way, I plan to post patches to enable
HST, HSI, SRU and LUT support next week.
--
Regards,
Laurent Pinchart
Attachment:
frame-000007.png
Description: PNG image
