Hi, Guennadi,
Thank you for the review!
Sergei Shtylyov wrote:
I also strongly suspent some #include <media/v4l2-*.h> headers are
missing
above.
Hm, I wonder which. I'm certainly not V4L2 expert...
added following:
#include <media/v4l2-common.h>
#include <media/v4l2-dev.h>
#include <media/v4l2-device.h>
#include <media/v4l2-mediabus.h>
#include <media/v4l2-subdev.h>
+ alloc_ctxs[0] = priv->alloc_ctx;
+
+ if (!vq->num_buffers)
+ priv->sequence = 0;
+
+ if (!*count)
+ *count = 2;
+ priv->vb_count = *count;
+
+ *num_planes = 1;
+
+ /* Number of hardware slots */
+ if (priv->vb_count > MAX_BUFFER_NUM)
+ priv->nr_hw_slots = MAX_BUFFER_NUM;
+ else
+ priv->nr_hw_slots = 1;
Is this really correct: with up to 3 buffers only one HW slot is used?
Probably not.
After replacing (priv->vb_count > MAX_BUFFER_NUM) with
is_continuous_transfer(priv) the logic becomes clear.
+ break;
+ default:
+ vnmc = VNMC_IM_ODD;
+ break;
+ }
+
+ /* input interface */
+ switch (icd->current_fmt->code) {
+ case V4L2_MBUS_FMT_YUYV8_1X16:
+ /* BT.601/BT.1358 16bit YCbCr422 */
+ vnmc |= VNMC_INF_YUV16;
+ input_is_yuv = 1;
+ break;
+ case V4L2_MBUS_FMT_YUYV8_2X8:
+ input_is_yuv = 1;
+ /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
+ vnmc |= priv->pdata->flags & RCAR_VIN_BT656 ?
+ VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
Let's clarify this. By BT.656 you mean embedded synchronisation
patterns,
right? In that case HSYNC and VSYNC signals aren't used.
Probably so, at least I know for sure HSYNC/VSYNC aren't used.
Yes, it is.
BT.656 uses embedded synchronization and HSYNC/VSYNC are not used for
this type of interface.
The BT.601 is a so-called by soc-camera layer MBUS_PARALLEL.
But in your
.set_bus_param() method you only support V4L2_MBUS_PARALLEL and not
V4L2_MBUS_BT656. And what do you call BT601? A bus with sync signals
used?
Yeah, judging from the manual, HSYNC, VSYNC, FIELD are used in BT.601.
The name ITUR BT601 YCbCr that comes from specification is a
MBUS_PARALLEL name per soc-camera layer naming.
The BT601 is expected to use h/w sync signals.
I've removed the pre initialization of v4l2_mbus_config .type filed in
favour of getting it from camera/decoder subdevice.
Thx for pointing to this.
+ }
+ }
+ spin_unlock_irq(&priv->lock);
+
+ pm_runtime_put_sync(ici->v4l2_dev.dev);
Do you really need the _sync version above?
I'm not runtime PM expert, to be honest...
replaced with pm_runtime_put().
thx for pointing to this.
+static u16 calc_scale(unsigned int src, unsigned int *dst)
+{
+ u16 scale;
+
+ if (src == *dst)
+ return 0;
+
+ scale = (src * 4096 / *dst) & ~7;
+
+ while (scale > 4096 && size_dst(src, scale) < *dst)
+ scale -= 8;
+
+ *dst = size_dst(src, scale);
+
+ return scale;
return value of this function is unused by the caller. Generally,
your use
of these two functions is different than on CEU, you might want to
get rid
of them completely.
I'd prefer to leave this function and provide fixes after scaling is
fully tested.
+ /* Set Start/End Pixel/Line Pre-Clip */
+ iowrite32(left_offset << dsize, priv->base + VNSPPRC_REG);
+ iowrite32((left_offset + cam->width - 1) << dsize,
+ priv->base + VNEPPRC_REG);
Do you have to shift for all 32-bit formats, not only for RGB32? I
understand this is related to the fact, that you don't support
pass-through...
At least the manual says to program an even number to VnSPPrC...
The driver explicitly says that V4L2_PIX_FMT_RGB32 is the only 32bit
format supported.
+static void capture_stop_preserve(struct rcar_vin_priv *priv, u32
*vnmc)
+{
+ *vnmc = ioread32(priv->base + VNMC_REG);
+ /* module disable */
+ iowrite32(*vnmc & ~VNMC_ME, priv->base + VNMC_REG);
+}
+
+static void capture_restore(struct rcar_vin_priv *priv, u32 vnmc)
+{
+ unsigned long timeout = jiffies + 10 * HZ;
+
+ if (!(vnmc & ~VNMC_ME))
+ /* Nothing to restore */
+ return;
And you don't have to wait for a frame end?
If the module wasn't active, there's probably no point... however,
let's
defer it to Vladimir.
Right. Thx for catching this.
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_YUYV,
+ .name = "YUYV",
+ .bits_per_sample = 16,
+ .packing = SOC_MBUS_PACKING_NONE,
+ .order = SOC_MBUS_ORDER_LE,
This conversion block is identical to the respective one in
soc_mediabus.c, which suggests to me, that no conversion is taking place
here. Then this mode should be usable for generic 8- or 16-bit
pass-through?
Let's defer this question to Vladimir.
Will add pass-through. Thank you.
+ switch (code) {
+ case V4L2_MBUS_FMT_YUYV8_1X16:
+ case V4L2_MBUS_FMT_YUYV8_2X8:
+ if (cam->extra_fmt)
+ break;
+
+ /* Add all our formats that can be generated by VIN */
+ cam->extra_fmt = rcar_vin_formats;
+
+ n = ARRAY_SIZE(rcar_vin_formats);
+ formats += n;
+ for (k = 0; xlate && k < n; k++, xlate++) {
+ xlate->host_fmt = &rcar_vin_formats[k];
+ xlate->code = code;
+ dev_dbg(dev, "Providing format %s using code %d\n",
+ rcar_vin_formats[k].name, code);
+ }
+ break;
+ default:
+ return 0;
The above tells me, that VIN (or at least this driver) can only capture
YUYV8 either over an 8- or a 16-bit bus. Isn't it possible to provide a
pass-through mode?
10/12-bit bus is also possible in UYUV format and 20/24-bit bus in
10/12 bits (Y) + 10/12 bits (CbCr) format on R-Car H1 VIN0/1. Not all
VIN interfaces are created equal in capabilities even within one
SoC... VIN2 indeed only supports 8 or 16 bits, and VIN3 only supports
8-bit bus.
Ditto
+/* Similar to set_crop multistage iterative algorithm */
+static int rcar_vin_set_fmt(struct soc_camera_device *icd,
+ struct v4l2_format *f)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+ struct rcar_vin_priv *priv = ici->priv;
+ struct rcar_vin_cam *cam = icd->host_priv;
+ struct v4l2_pix_format *pix = &f->fmt.pix;
+ struct v4l2_mbus_framefmt mf;
+ struct device *dev = icd->parent;
+ __u32 pixfmt = pix->pixelformat;
+ const struct soc_camera_format_xlate *xlate;
+ unsigned int vin_sub_width = 0, vin_sub_height = 0;
+ u16 scale_v, scale_h;
+ int ret;
+ bool can_scale;
+ bool data_through;
What exactly does data_through mean? I thought it meant a pass-through
mode, but it is set to true for a YUYV->RGB32 conversion, which isn't
pass-through obviously.
Maybe it's just bset incorrectly. As I said, data through should
only be supported on R-Car E1 IIRC.
the data_through is removed per prev request and added just the check
for RGB32 FMT in order to use a shift.
[...]
+ data_through = pixfmt == V4L2_PIX_FMT_RGB32;
What is "data_through" and why is RGB32 so special?
DIIK, to be honest. :-)
We will provide all detailed info once E1 will be up. Unfortunately, the
E1 is not up but it is on the schedule.
VIN can scale _everything_ except NV16 and RGB32?
Depends on SOC ..... RGB32 and NV16 are applicable to E1 only .... we'll
make the VIN module differentiation depending on SoC (H/M/E ...)
+static int rcar_vin_init_videobuf2(struct vb2_queue *vq,
+ struct soc_camera_device *icd)
+{
+ vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ vq->io_modes = VB2_MMAP | VB2_USERPTR;
+ vq->drv_priv = icd;
+ vq->ops = &rcar_vin_vb2_ops;
+ vq->mem_ops = &vb2_dma_contig_memops;
+ vq->buf_struct_size = sizeof(struct rcar_vin_buffer);
Please, add
vq->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
There is not such field in "struct vb2_queue".
All other parts of review are reworked per suggestions.
Regards,
Vladimir
--
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
