Hello! On 03/17/2017 05:24 PM, Hans Verkuil wrote:
Sorry for the long wait before I got around to reviewing this, but here are (finally!) my review comments.
Addressing your comments took significant time too, and I wasn't able to address all of them yet...
On 03/09/17 21:08, Sergei Shtylyov wrote:From: Konstantin Kozhevnikov <Konstantin.Kozhevnikov@xxxxxxxxxxxxxxxxxx> The image renderer, or the distortion correction engine, is a drawing processor with a simple instruction system capable of referencing video capture data or data in an external memory as the 2D texture data and performing texture mapping and drawing with respect to any shape that is split into triangular objects. This V4L2 memory-to-memory device driver only supports image renderer light extended 4 (IMR-LX4) found in the R-Car gen3 SoCs; the R-Car gen2 support can be added later... [Sergei: merged 2 original patches, added the patch description, removed unrelated parts, added the binding document, ported the driver to the modern kernel, renamed the UAPI header file and the guard macros to match the driver name, extended the copyrights, fixed up Kconfig prompt/depends/ help, made use of the BIT/GENMASK() macros, sorted #include's, removed leading dots and fixed grammar in the comments, fixed up indentation to use tabs where possible, renamed DLSR, CMRCR.DY1{0|2}, and ICR bits to match the manual, changed the prefixes of the CMRCR[2]/TRI{M|C}R bits/ fields to match the manual, removed non-existent TRIMR.D{Y|U}D{X|V}M bits, added/used the IMR/{UV|CP}DPOR/SUSR bits/fields/shifts, separated the register offset/bit #define's, sorted the instruction macros by opcode, removed unsupported LINE instruction, masked the register address in WTL[2]/WTS instruction macros, moved the display list #define's after the register #define's, removing the redundant comment, avoided setting reserved bits when writing CMRCCR[2]/TRIMCR, used the SR bits instead of a bare number, used ALIGN() macro in imr_ioctl_map(), removed *inline* from .c file, fixed lines over 80 columns, removed useless spaces, comments, parens, operators, casts, braces, variables, #include's, statements, and even 1 function, uppercased the abbreviations, made comment wording more consistent/correct, fixed the comment typos, reformatted some multiline comments, inserted empty line after declaration, removed extra empty lines, reordered some local variable desclarations, removed calls to 4l2_err() on kmalloc() failure, replaced '*' with 'x' in some format strings for v4l2_dbg(), fixed the error returned by imr_default(), avoided code duplication in the IRQ handler, used '__packed' for the UAPI structures, enclosed the macro parameters in parens, exchanged the values of IMR_MAP_AUTO{S|D}G macros.] Signed-off-by: Konstantin Kozhevnikov <Konstantin.Kozhevnikov@xxxxxxxxxxxxxxxxxx> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
[...]
Index: media_tree/drivers/media/platform/rcar_imr.c =================================================================== --- /dev/null +++ media_tree/drivers/media/platform/rcar_imr.c @@ -0,0 +1,1943 @@
[...]
+/* M2M device processing queue initialization */ +static int imr_queue_init(void *priv, struct vb2_queue *src_vq, + struct vb2_queue *dst_vq) +{ + struct imr_ctx *ctx = priv; + int ret; + + memset(src_vq, 0, sizeof(*src_vq)); + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; + src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF; + src_vq->drv_priv = ctx; + src_vq->buf_struct_size = sizeof(struct imr_buffer); + src_vq->ops = &imr_qops; + src_vq->mem_ops = &vb2_dma_contig_memops; + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; + src_vq->lock = &ctx->imr->mutex;
[...]
+ ret = vb2_queue_init(src_vq); + if (ret) + return ret; + + memset(dst_vq, 0, sizeof(*dst_vq)); + dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;Drop VB2_USERPTR here and above. Not recommended to use this with dma-contig.
Hm... but we need it -- that's what Konstantin has been actively using. [...]
+/* retrieve current queue format; operation is locked? */ +static int imr_g_fmt(struct file *file, void *priv, struct v4l2_format *f) +{ + struct imr_ctx *ctx = fh_to_ctx(priv); + struct imr_q_data *q_data; + struct vb2_queue *vq; + + vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); + if (!vq) + return -EINVAL; + + q_data = &ctx->queue[V4L2_TYPE_IS_OUTPUT(f->type) ? 0 : 1]; + + /* processing is locked? TBD */ + f->fmt.pix = q_data->fmt; + + return 0; +} + +/* test particular format; operation is not locked */ +static int imr_try_fmt(struct file *file, void *priv, struct v4l2_format *f) +{ + struct imr_ctx *ctx = fh_to_ctx(priv); + struct vb2_queue *vq; + + /* make sure we have a queue of particular type */ + vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); + if (!vq) + return -EINVAL; + + /* test if format is supported (adjust as appropriate) */ + return __imr_try_fmt(ctx, f) >= 0 ? 0 : -EINVAL; +} + +/* apply queue format; operation is locked? */ +static int imr_s_fmt(struct file *file, void *priv, struct v4l2_format *f) +{ + struct imr_ctx *ctx = fh_to_ctx(priv); + struct imr_q_data *q_data; + struct vb2_queue *vq; + int i; + + vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); + if (!vq) + return -EINVAL; + + /* check if queue is busy */ + if (vb2_is_busy(vq)) + return -EBUSY; + + /* test if format is supported (adjust as appropriate) */ + i = __imr_try_fmt(ctx, f); + if (i < 0) + return -EINVAL; + + /* format is supported; save current format in a queue-specific data */ + q_data = &ctx->queue[V4L2_TYPE_IS_OUTPUT(f->type) ? 0 : 1]; + + /* processing is locked? TBD */ + q_data->fmt = f->fmt.pix; + q_data->flags = imr_lx4_formats[i].flags; + + /* set default crop factors */ + if (!V4L2_TYPE_IS_OUTPUT(f->type)) { + ctx->crop[0] = 0; + ctx->crop[1] = f->fmt.pix.width - 1; + ctx->crop[2] = 0; + ctx->crop[3] = f->fmt.pix.height - 1; + }I'm pretty sure v4l2-compliance will complain about missing colorspace fields.
Don't we have the colorspace field in q_data->fmt?
drivers/media/platform/vim2m.c gives a good idea how to handle this for m2m devices.
I didn't see much handling there... nothing seems to depend on the colorspace...
[...]
Index: media_tree/include/uapi/linux/rcar_imr.h =================================================================== --- /dev/null +++ media_tree/include/uapi/linux/rcar_imr.h @@ -0,0 +1,94 @@ +/* + * rcar_imr.h -- R-Car IMR-LX4 Driver UAPI + * + * Copyright (C) 2016-2017 Cogent Embedded, Inc. <source@xxxxxxxxxxxxxxxxxx> + * + * 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. + */ + +#ifndef __RCAR_IMR_H +#define __RCAR_IMR_H + +#include <linux/videodev2.h> + +/******************************************************************************* + * Mapping specification descriptor + ******************************************************************************/ + +struct imr_map_desc { + /* mapping types */ + u32 type; + + /* total size of the mesh structure */ + u32 size; + + /* map-specific user-pointer */ + void *data;Note that this void pointer would require compat32 support if this IP block can be used on a 64-bit arm.
It was developed on ARM64. :-) I've changed the type to __u64 now.
+} __packed; + +/* regular mesh specification */ +#define IMR_MAP_MESH (1 << 0) + +/* auto-generated source coordinates */ +#define IMR_MAP_AUTOSG (1 << 1) + +/* auto-generated destination coordinates */ +#define IMR_MAP_AUTODG (1 << 2) + +/* luminance correction flag */ +#define IMR_MAP_LUCE (1 << 3) + +/* chromacity correction flag */ +#define IMR_MAP_CLCE (1 << 4) + +/* vertex clockwise-mode order */ +#define IMR_MAP_TCM (1 << 5) + +/* source coordinate decimal point position bit index */ +#define __IMR_MAP_UVDPOR_SHIFT 8 +#define __IMR_MAP_UVDPOR(v) (((v) >> __IMR_MAP_UVDPOR_SHIFT) & 0x7) +#define IMR_MAP_UVDPOR(n) (((n) & 0x7) << __IMR_MAP_UVDPOR_SHIFT) + +/* destination coordinate sub-pixel mode */ +#define IMR_MAP_DDP (1 << 11) + +/* luminance correction offset decimal point position */ +#define __IMR_MAP_YLDPO_SHIFT 12 +#define __IMR_MAP_YLDPO(v) (((v) >> __IMR_MAP_YLDPO_SHIFT) & 0x7) +#define IMR_MAP_YLDPO(n) (((n) & 0x7) << __IMR_MAP_YLDPO_SHIFT) + +/* chromacity (U) correction offset decimal point position */ +#define __IMR_MAP_UBDPO_SHIFT 15 +#define __IMR_MAP_UBDPO(v) (((v) >> __IMR_MAP_UBDPO_SHIFT) & 0x7) +#define IMR_MAP_UBDPO(n) (((n) & 0x7) << __IMR_MAP_UBDPO_SHIFT) + +/* chromacity (V) correction offset decimal point position */ +#define __IMR_MAP_VRDPO_SHIFT 18 +#define __IMR_MAP_VRDPO(v) (((v) >> __IMR_MAP_VRDPO_SHIFT) & 0x7) +#define IMR_MAP_VRDPO(n) (((n) & 0x7) << __IMR_MAP_VRDPO_SHIFT) + +/* regular mesh specification */ +struct imr_mesh { + /* rectangular mesh size */ + u16 rows, columns; + + /* mesh parameters */ + u16 x0, y0, dx, dy; +} __packed; + +/* VBO descriptor */ +struct imr_vbo { + /* number of triangles */ + u16 num; +} __packed;Is this part of the public API? Not clear at all.
Yes, I've tried to document it... perhaps still not well enough but I still didn't grasp the ReST markup enough...
+ +/******************************************************************************* + * Private IOCTL codes + ******************************************************************************/ + +#define VIDIOC_IMR_MESH _IOW('V', BASE_VIDIOC_PRIVATE + 0, struct imr_map_desc)This needs to be documented. It's not clear at all how this should be used. A new file in Documentation/media/v4l-drivers/ is a good place to do that. I'm not sure how much of the HW documentation is public. If it is, then you can
It's not public at all, unfortunately.
refer to that and you just have to explain how to use this ioctl. Regards, Hans
MBR, Sergei
