Hi Laurent, Thank you for your great work for VSP1. Some comments below. 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. (snip) > +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. > +#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. (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. > + 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'. 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. Thanks, --- Katsuya Matsubara / IGEL Co., Ltd matsu@xxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html