Hi Laurent,
just a few small remarks...
On 07/31/2013 05:52 PM, Laurent Pinchart wrote:
The VSP1 is a video processing engine that includes a blender, scalers,
filters and statistics computation. Configurable data path routing logic
allows ordering the internal blocks in a flexible way.
Due to the configurable nature of the pipeline the driver implements the
media controller API and doesn't use the V4L2 mem-to-mem framework, even
though the device usually operates in memory to memory mode.
Only the read pixel formatters, up/down scalers, write pixel formatters
and LCDC interface are supported at this stage.
Signed-off-by: Laurent Pinchart<laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
Changes since v1:
- Updated to the v3.11 media controller API changes
- Only add the LIF entity to the entities list when the LIF is present
- Added a MODULE_ALIAS()
- Fixed file descriptions in comment blocks
- Removed function prototypes for the unimplemented destroy functions
- Fixed a typo in the HST register name
- Fixed format propagation for the UDS entities
- Added v4l2_capability::device_caps support
- Prefix the device name with "platform:" in bus_info
- Zero the v4l2_pix_format priv field in the internal try format handler
- Use vb2_is_busy() instead of vb2_is_streaming() when setting the
format
- Use the vb2_ioctl_* handlers where possible
- Fix register macros that were missing a n argument
- Mask unused bits when clearing the interrupt status register
- Explain why stride alignment to 128 bytes is needed
- Use the aligned stride value when computing the image size
- Assorted cosmetic changes
---
drivers/media/platform/Kconfig | 10 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/vsp1/Makefile | 5 +
drivers/media/platform/vsp1/vsp1.h | 73 ++
drivers/media/platform/vsp1/vsp1_drv.c | 497 +++++++++++++
drivers/media/platform/vsp1/vsp1_entity.c | 181 +++++
drivers/media/platform/vsp1/vsp1_entity.h | 68 ++
drivers/media/platform/vsp1/vsp1_lif.c | 238 ++++++
drivers/media/platform/vsp1/vsp1_lif.h | 37 +
drivers/media/platform/vsp1/vsp1_regs.h | 581 +++++++++++++++
drivers/media/platform/vsp1/vsp1_rpf.c | 209 ++++++
drivers/media/platform/vsp1/vsp1_rwpf.c | 124 ++++
drivers/media/platform/vsp1/vsp1_rwpf.h | 53 ++
drivers/media/platform/vsp1/vsp1_uds.c | 346 +++++++++
drivers/media/platform/vsp1/vsp1_uds.h | 40 +
drivers/media/platform/vsp1/vsp1_video.c | 1135 +++++++++++++++++++++++++++++
drivers/media/platform/vsp1/vsp1_video.h | 144 ++++
drivers/media/platform/vsp1/vsp1_wpf.c | 233 ++++++
include/linux/platform_data/vsp1.h | 25 +
19 files changed, 4001 insertions(+)
create mode 100644 drivers/media/platform/vsp1/Makefile
create mode 100644 drivers/media/platform/vsp1/vsp1.h
create mode 100644 drivers/media/platform/vsp1/vsp1_drv.c
create mode 100644 drivers/media/platform/vsp1/vsp1_entity.c
create mode 100644 drivers/media/platform/vsp1/vsp1_entity.h
create mode 100644 drivers/media/platform/vsp1/vsp1_lif.c
create mode 100644 drivers/media/platform/vsp1/vsp1_lif.h
create mode 100644 drivers/media/platform/vsp1/vsp1_regs.h
create mode 100644 drivers/media/platform/vsp1/vsp1_rpf.c
create mode 100644 drivers/media/platform/vsp1/vsp1_rwpf.c
create mode 100644 drivers/media/platform/vsp1/vsp1_rwpf.h
create mode 100644 drivers/media/platform/vsp1/vsp1_uds.c
create mode 100644 drivers/media/platform/vsp1/vsp1_uds.h
create mode 100644 drivers/media/platform/vsp1/vsp1_video.c
create mode 100644 drivers/media/platform/vsp1/vsp1_video.h
create mode 100644 drivers/media/platform/vsp1/vsp1_wpf.c
create mode 100644 include/linux/platform_data/vsp1.h
[...]
+/* -----------------------------------------------------------------------------
+ * Platform Driver
+ */
[...]
+static int vsp1_probe(struct platform_device *pdev)
+{
+ struct vsp1_device *vsp1;
+ struct resource *irq;
+ struct resource *io;
+ int ret;
+
+ vsp1 = devm_kzalloc(&pdev->dev, sizeof(*vsp1), GFP_KERNEL);
+ if (vsp1 == NULL) {
+ dev_err(&pdev->dev, "failed to allocate private data\n");
k*alloc already log any errors, hence this line could be dropped. I've
seen patches removing such logging.
+ return -ENOMEM;
+ }
+
+ vsp1->dev =&pdev->dev;
+ mutex_init(&vsp1->lock);
+ INIT_LIST_HEAD(&vsp1->entities);
+
+ vsp1->pdata = vsp1_get_platform_data(pdev);
+ if (vsp1->pdata == NULL)
+ return -ENODEV;
+
+ /* I/O, IRQ and clock resources */
+ io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+ if (!io || !irq) {
+ dev_err(&pdev->dev, "missing IRQ or IOMEM\n");
+ return -EINVAL;
+ }
+
+ vsp1->mmio = devm_ioremap_resource(&pdev->dev, io);
+ if (IS_ERR((void *)vsp1->mmio)) {
+ dev_err(&pdev->dev, "failed to remap memory resource\n");
Similarly devm_ioremap_resource() already logs any errors, including
checking
if the second argument is valid.
+ return PTR_ERR((void *)vsp1->mmio);
+ }
+
+ vsp1->clock = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(vsp1->clock)) {
+ dev_err(&pdev->dev, "failed to get clock\n");
+ return PTR_ERR(vsp1->clock);
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq->start, vsp1_irq_handler,
+ IRQF_SHARED, dev_name(&pdev->dev), vsp1);
+ if (ret< 0) {
+ dev_err(&pdev->dev, "failed to request IRQ\n");
+ return ret;
+ }
+
+ /* Instanciate entities */
+ ret = vsp1_create_entities(vsp1);
+ if (ret< 0) {
+ dev_err(&pdev->dev, "failed to create entities\n");
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, vsp1);
+
+ return 0;
+}
+
[...]
+/* -----------------------------------------------------------------------------
+ * Initialization and Cleanup
+ */
+
+struct vsp1_lif *vsp1_lif_create(struct vsp1_device *vsp1)
+{
+ struct v4l2_subdev *subdev;
+ struct vsp1_lif *lif;
+ int ret;
+
+ lif = devm_kzalloc(vsp1->dev, sizeof(*lif), GFP_KERNEL);
+ if (lif == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ lif->entity.type = VSP1_ENTITY_LIF;
+ lif->entity.id = VI6_DPR_NODE_LIF;
+
+ ret = vsp1_entity_init(vsp1,&lif->entity, 2);
+ if (ret< 0)
+ return ERR_PTR(ret);
+
+ /* Initialize the V4L2 subdev. */
+ subdev =&lif->entity.subdev;
+ v4l2_subdev_init(subdev,&lif_ops);
+
+ subdev->entity.ops =&vsp1_media_ops;
+ subdev->internal_ops =&vsp1_subdev_internal_ops;
+ snprintf(subdev->name, sizeof(subdev->name), "%s lif",
+ dev_name(vsp1->dev));
Using dev_name() looks reasonable since it guarantees the subdev names
are unique. But for dt and non-dt boot you will get different device
names. Not sure if it would have been really an issue though.
+ v4l2_set_subdevdata(subdev, lif);
+ subdev->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ vsp1_entity_init_formats(subdev, NULL);
+
+ return lif;
+}
[...]
+static int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
+{
+ struct vsp1_entity *entity;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&pipe->irqlock, flags);
+ pipe->state = VSP1_PIPELINE_STOPPING;
+ spin_unlock_irqrestore(&pipe->irqlock, flags);
+
+ ret = wait_event_timeout(pipe->wq, pipe->state == VSP1_PIPELINE_STOPPED,
+ msecs_to_jiffies(500));
+ ret = ret == 0 ? -ETIMEDOUT : 0;
Wouldn't be -ETIME more appropriate ?
#define ETIME 62 /* Timer expired */
...
#define ETIMEDOUT 110 /* Connection timed out */
+ list_for_each_entry(entity,&pipe->entities, list_pipe) {
+ if (entity->route)
+ vsp1_write(entity->vsp1, entity->route,
+ VI6_DPR_NODE_UNUSED);
+
+ v4l2_subdev_call(&entity->subdev, video, s_stream, 0);
+ }
+
+ return ret;
+}
[...]
+/* -----------------------------------------------------------------------------
+ * videobuf2 Queue Operations
+ */
+
+static int
+vsp1_video_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
+ unsigned int *nbuffers, unsigned int *nplanes,
+ unsigned int sizes[], void *alloc_ctxs[])
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vq);
+ struct v4l2_pix_format_mplane *format =&video->format;
+ unsigned int i;
If you don't support VIDIOC_CREATE_BUFS ioctl then there should probably
be at least something like:
if (fmt)
return -EINVAL;
But it's likely better to add proper handling of 'fmt' right away.
+ *nplanes = format->num_planes;
+
+ for (i = 0; i< format->num_planes; ++i) {
+ sizes[i] = format->plane_fmt[i].sizeimage;
+ alloc_ctxs[i] = video->alloc_ctx;
+ }
+
+ return 0;
+}
+
+static int vsp1_video_buffer_prepare(struct vb2_buffer *vb)
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vb->vb2_queue);
+ struct vsp1_video_buffer *buf = to_vsp1_video_buffer(vb);
+ unsigned int i;
+
+ buf->video = video;
+
+ for (i = 0; i< vb->num_planes; ++i) {
+ buf->addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);
+ buf->length[i] = vb2_plane_size(vb, i);
+ }
+
+ return 0;
+}
+
+static void vsp1_video_buffer_queue(struct vb2_buffer *vb)
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vb->vb2_queue);
+ struct vsp1_pipeline *pipe = to_vsp1_pipeline(&video->video.entity);
+ struct vsp1_video_buffer *buf = to_vsp1_video_buffer(vb);
+ unsigned long flags;
+ bool empty;
+
+ spin_lock_irqsave(&video->irqlock, flags);
+ empty = list_empty(&video->irqqueue);
+ list_add_tail(&buf->queue,&video->irqqueue);
+ spin_unlock_irqrestore(&video->irqlock, flags);
+
+ if (!empty)
+ return;
+
+ spin_lock_irqsave(&pipe->irqlock, flags);
+
+ video->ops->queue(video, buf);
+ pipe->buffers_ready |= 1<< video->pipe_index;
+
+ if (vb2_is_streaming(&video->queue)&&
+ vsp1_pipeline_ready(pipe))
+ vsp1_pipeline_run(pipe);
+
+ spin_unlock_irqrestore(&pipe->irqlock, flags);
+}
+
+static void vsp1_video_wait_prepare(struct vb2_queue *vq)
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vq);
+
+ mutex_unlock(&video->lock);
+}
+
+static void vsp1_video_wait_finish(struct vb2_queue *vq)
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vq);
+
+ mutex_lock(&video->lock);
+}
+
+static void vsp1_entity_route_setup(struct vsp1_entity *source)
+{
+ struct vsp1_entity *sink;
+
+ if (source->route == 0)
+ return;
+
+ sink = container_of(source->sink, struct vsp1_entity, subdev.entity);
+ vsp1_write(source->vsp1, source->route, sink->id);
+}
+
+static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vq);
+ struct vsp1_pipeline *pipe = to_vsp1_pipeline(&video->video.entity);
+ struct vsp1_entity *entity;
+ unsigned long flags;
+ int ret;
+
+ mutex_lock(&pipe->lock);
+ if (pipe->stream_count == pipe->num_video - 1) {
+ list_for_each_entry(entity,&pipe->entities, list_pipe) {
+ vsp1_entity_route_setup(entity);
+
+ ret = v4l2_subdev_call(&entity->subdev, video,
+ s_stream, 1);
+ if (ret< 0) {
+ mutex_unlock(&pipe->lock);
+ return ret;
+ }
+ }
+ }
+
+ pipe->stream_count++;
+ mutex_unlock(&pipe->lock);
+
+ spin_lock_irqsave(&pipe->irqlock, flags);
+ if (vsp1_pipeline_ready(pipe))
+ vsp1_pipeline_run(pipe);
+ spin_unlock_irqrestore(&pipe->irqlock, flags);
+
+ return 0;
+}
+
+static int vsp1_video_stop_streaming(struct vb2_queue *vq)
+{
+ struct vsp1_video *video = vb2_get_drv_priv(vq);
+ struct vsp1_pipeline *pipe = to_vsp1_pipeline(&video->video.entity);
+ unsigned long flags;
+ int ret;
+
+ mutex_lock(&pipe->lock);
+ if (--pipe->stream_count == 0) {
+ /* Stop the pipeline. */
+ ret = vsp1_pipeline_stop(pipe);
+ if (ret == -ETIMEDOUT)
ETIME ?
+ dev_err(video->vsp1->dev, "pipeline stop timeout\n");
+ }
+ mutex_unlock(&pipe->lock);
+
+ vsp1_pipeline_cleanup(pipe);
+ media_entity_pipeline_stop(&video->video.entity);
+
+ /* Remove all buffers from the IRQ queue. */
+ spin_lock_irqsave(&video->irqlock, flags);
+ INIT_LIST_HEAD(&video->irqqueue);
+ spin_unlock_irqrestore(&video->irqlock, flags);
+
+ return 0;
+}
+
+static struct vb2_ops vsp1_video_queue_qops = {
+ .queue_setup = vsp1_video_queue_setup,
+ .buf_prepare = vsp1_video_buffer_prepare,
+ .buf_queue = vsp1_video_buffer_queue,
+ .wait_prepare = vsp1_video_wait_prepare,
+ .wait_finish = vsp1_video_wait_finish,
+ .start_streaming = vsp1_video_start_streaming,
+ .stop_streaming = vsp1_video_stop_streaming,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 ioctls
+ */
[...]
+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.
+ */
Nitpicking: Isn't proper multi-line comment style
/*
* Retrieve format information and select the default format if the
* requested format isn't supported.
*/
? In fact the media subsystem code is pretty messy WRT that detail.
+ 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. While not documented in
+ * the datasheet, strides not aligned to a multiple of 128 bytes result
+ * in image corruption.
+ */
+ 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 align = 128;
+ unsigned int bpl;
+
+ bpl = clamp_t(unsigned int, pix->plane_fmt[i].bytesperline,
+ pix->width / hsub * info->bpp[i] / 8,
+ round_down(65535U, align));
+
+ pix->plane_fmt[i].bytesperline = round_up(bpl, align);
+ pix->plane_fmt[i].sizeimage = pix->plane_fmt[i].bytesperline
+ * pix->height / vsub;
+ }
+
+ if (info->planes == 3) {
+ /* The second and third planes must have the same stride. */
+ pix->plane_fmt[2].bytesperline = pix->plane_fmt[1].bytesperline;
+ pix->plane_fmt[2].sizeimage = pix->plane_fmt[1].sizeimage;
+ }
+
+ pix->num_planes = info->planes;
+
+ if (fmtinfo)
+ *fmtinfo = info;
+
+ return 0;
+}
+
Regards,
Sylwester
--
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
