On 08/17/17 09:59, Hans Verkuil wrote:
> Hi Sergei,
>
> A quick review. I'm concentrating on the mesh ioctl, since that's what sets this
> driver apart.
>
> On 08/04/2017 08:03 PM, Sergei Shtylyov wrote:
>
> <snip>
>
>> Index: media_tree/Documentation/media/v4l-drivers/rcar_imr.rst
>> ===================================================================
>> --- /dev/null
>> +++ media_tree/Documentation/media/v4l-drivers/rcar_imr.rst
>> @@ -0,0 +1,372 @@
>> +Renesas R-Car Image Renderer (IMR) Driver
>> +=========================================
>> +
>> +This file documents some driver-specific aspects of the IMR driver, such as
>> +the driver-specific ioctl.
>
> Just drop the part ', such as...'.
>
> Can you add a high-level description of the functionality here? Similar to what
> you did in the bindings document.
>
>> +
>> +The ioctl reference
>> +~~~~~~~~~~~~~~~~~~~
>> +
>> +See ``include/uapi/linux/rcar_imr.h`` for the data structures used.
>> +
>> +VIDIOC_IMR_MESH - Set mapping data
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Argument: ``struct imr_map_desc``
>> +
>> +**Description**:
>> +
>> +This ioctl sets up the mesh through which the input frames will be transformed
>> +into the output frames. The mesh can be strictly rectangular (when
>> +``IMR_MAP_MESH`` bit is set in ``imr_map_desc::type``) or arbitrary (when that
>> +bit is not set).
>
> Wouldn't something like 'IMR_MAP_RECTANGULAR' be much more descriptive than _MESH?
> There is nothing in the name _MESH to indicate that this switches the data to
> rectangles.
>
>> +
>> +A rectangular mesh consists of the imr_mesh structure followed by M*N vertex
>> +objects (where M is ``imr_mesh::rows`` and N is ``imr_mesh::columns``).
>> +In case either ``IMR_MAP_AUTOSG`` or ``IMR_MAP_AUTODG`` (not both) bits are
>> +set in ``imr_map_desc::type``, ``imr_mesh::{x|y}0`` specify the coordinates
>> +of the top left corner of the auto-generated mesh and ``imr_mesh::d{x|y}``
>> +specify the mesh's X/Y steps.
>
> So if the auto bits are set, then there are no vertex objects? Since it's auto
> generated by the hardware?
>
> I believe we discussed in the past whether 'type' should be split in a 'type'
> and 'flags' field.
>
>> +
>> +An arbitrary mesh consists of the imr_vbo structure followed by N triangle
>> +objects (where N is ``imr_vbo::num``), consisting of 3 vertex objects each.
>> +Setting ``IMR_MAP_AUTODG`` and ``IMR_MAP_AUTOSG`` bits in
>> +``imr_map_desc::type``) isn't allowed for this type of mesh.
>> +
>> +The vertex object has a complex structure depending on some of the bits in
>> +``imr_map_desc::type``:
>> +
>> +============ ============ ============== ============== ===========================
>> +IMR_MAP_CLCE IMR_MAP_LUCE IMR_MAP_AUTODG IMR_MAP_AUTOSG Vertex structure variant
>
> You should explain the meaning of these bits in this section. I.e., what does
> CLCE or AUTODG stand for?
>
>> +============ ============ ============== ============== ===========================
>> +\ ``imr_full_coord``
>> +\ X ``imr_dst_coord``
>> +\ X ``imr_src_coord``
>> +\ X ``imr_full_coord_any_correct``
>> +\ X X ``imr_auto_coord_any_correct``
>> +\ X X ``imr_auto_coord_any_crrect``
>
> crrect -> correct
>
>> +X ``imr_full_coord_any_correct``
>> +X X ``imr_auto_coord_any_correct``
>> +X X ``imr_auto_coord_any_correct``
>> +X X ``imr_full_coord_both_correct``
>> +X X X ``imr_auto_coord_both_correct``
>> +X X X ``imr_auto_coord_both_correct``
>> +============ ============ ============== ============== ===========================
>> +
>> +The luma correction is calculated according to the following formula (where
>> +``Y`` is the luma value after texture mapping, ``Y'`` is the luma value after
>> +luma correction, ``lscal`` and ``lofst`` are the luma correction scale and
>> +offset taken from ``struct imr_luma_correct``, ``YLDPO`` is a luma correction
>> +scale decimal point position specified by ``IMR_MAP_YLDPO(n)``): ::
>> +
>> + Y' = ((Y * lscal) >> YLDPO) + lofst
>> +
>> +The chroma correction is calculated according to the following formula (where
>> +``U/V`` are the chroma values after texture mapping, ``U'/V'`` are the chroma
>> +values after chroma correction, ``ubscl/vrscl`` and ``ubofs/vrofs`` are the
>> +U/V value chroma correction scales and offsets taken from
>> +``struct imr_chroma_correct``, ``UBDPO/VRDPO`` are the chroma correction scale
>> +decimal point positions specified by ``IMR_MAP_{UBDPO|VRDPO}(n)``): ::
>> +
>> + U' = ((U + ubofs) * ubscl) >> UBDPO
>> + V' = ((V + vrofs) * vrscl) >> VRDPO
>> +
>> +**Return value**:
>> +
>> +On success 0 is returned. On error -1 is returned and ``errno`` is set
>> +appropriately.
>> +
>> +**Example code**:
>> +
>> +Below is an example code for constructing the meshes: ``imr_map_create()``
>> +constructs an arbitraty mesh, ``imr_map_mesh_src()`` function constructs
>
> arbitrary
>
>> +a rectangular mesh with the auto-generated destination coordinates.
>> +
>> +.. code-block:: C
>> +
>> + #include <malloc.h>
>> + #include <math.h>
>> + #include <linux/rcar_imr.h>
>> +
>> + /* IMR device data */
>> + struct imr_device {
>> + /* V4L2 file decriptor */
>
> descriptor
>
>> + int vfd;
>> +
>> + /* input/output buffers dimensions */
>> + int w, h, W, H;
>> + };
>> +
>> + #define IMR_SRC_SUBSAMPLE 5
>> + #define IMR_DST_SUBSAMPLE 2
>> + #define IMR_COORD_THRESHOLD (128 * 128 * (1 << 2 * IMR_DST_SUBSAMPLE))
>> +
>> + /* find the longest side (index) */
>> + static void find_longest_side(int n, __s16 xy0, __s16 xy1, int *max, int *side)
>> + {
>> + int t = xy1 - xy0;
>> +
>> + t *= t;
>> + if (*max < t) {
>> + *max = t;
>> + *side = n;
>> + }
>> + }
>> +
>> + /* recursively split a triangle until it can be passed to IMR */
>> + static int split_triangle(struct imr_full_coord *coord,
>> + __s16 *xy0, __s16 *xy1, __s16 *xy2,
>> + __u16 *uv0, __u16 *uv1, __u16 *uv2, int avail)
>> + {
>> + int max = 0, k = 0;
>> +
>> + /* we need to have at least one available triangle */
>> + if (avail < 1)
>> + return 0;
>> +
>> + find_longest_side(0, xy0[0], xy1[0], &max, &k);
>> + find_longest_side(0, xy0[1], xy1[1], &max, &k);
>> + find_longest_side(1, xy1[0], xy2[0], &max, &k);
>> + find_longest_side(1, xy1[1], xy2[1], &max, &k);
>> + find_longest_side(2, xy2[0], xy0[0], &max, &k);
>> + find_longest_side(2, xy2[1], xy0[1], &max, &k);
>> +
>> + /* if value exceeds the threshold, do splitting */
>> + if (max >= IMR_COORD_THRESHOLD) {
>> + __s16 XY[2];
>> + __u16 UV[2];
>> + int n;
>> +
>> + switch (k) {
>> + case 0:
>> + /* split triangle along edge 0 - 1 */
>> + XY[0] = (xy0[0] + xy1[0]) / 2;
>> + XY[1] = (xy0[1] + xy1[1]) / 2;
>> + UV[0] = (uv0[0] + uv1[0]) / 2;
>> + UV[1] = (uv0[1] + uv1[1]) / 2;
>> + n = split_triangle(coord, xy0, XY, xy2, uv0, UV, uv2,
>> + avail);
>> + n += split_triangle(coord + 3 * n, XY, xy1, xy2,
>> + UV, uv1, uv2, avail - n);
>> + break;
>> + case 1:
>> + /* split triangle along edge 1 - 2 */
>> + XY[0] = (xy1[0] + xy2[0]) / 2;
>> + XY[1] = (xy1[1] + xy2[1]) / 2;
>> + UV[0] = (uv1[0] + uv2[0]) / 2;
>> + UV[1] = (uv1[1] + uv2[1]) / 2;
>> + n = split_triangle(coord, xy1, XY, xy0, uv1, UV, uv0,
>> + avail);
>> + n += split_triangle(coord + 3 * n, XY, xy2, xy0,
>> + UV, uv2, uv0, avail - n);
>> + break;
>> + default:
>> + /* split triangle along edge 2 - 0 */
>> + XY[0] = (xy2[0] + xy0[0]) / 2;
>> + XY[1] = (xy2[1] + xy0[1]) / 2;
>> + UV[0] = (uv2[0] + uv0[0]) / 2;
>> + UV[1] = (uv2[1] + uv0[1]) / 2;
>> + n = split_triangle(coord, xy2, XY, xy1, uv2, UV, uv1,
>> + avail);
>> + n += split_triangle(coord + 3 * n, XY, xy0, xy1,
>> + UV, uv0, uv1, avail - n);
>> + }
>> +
>> + /* return number of triangles added */
>> + return n;
>> + } else {
>> + /* no need to split a rectangle; save coordinates */
>> + coord[0].src.u = uv0[0];
>> + coord[0].src.v = uv0[1];
>> + coord[0].dst.x = xy0[0];
>> + coord[0].dst.y = xy0[1];
>> + coord[1].src.u = uv1[0];
>> + coord[1].src.v = uv1[1];
>> + coord[1].dst.x = xy1[0];
>> + coord[1].dst.y = xy1[1];
>> + coord[2].src.u = uv2[0];
>> + coord[2].src.v = uv2[1];
>> + coord[2].dst.x = xy2[0];
>> + coord[2].dst.y = xy2[1];
>> +
>> + /* single triangle is created */
>> + return 1;
>> + }
>> + }
>> +
>> + /* process single triangle */
>> + static int process_triangle(struct imr_full_coord *coord, __u16 *uv, __s16 *xy,
>> + int avail)
>> + {
>> + /* cull invisible triangle first */
>> + if ((xy[2] - xy[0]) * (xy[5] - xy[3]) >=
>> + (xy[3] - xy[1]) * (xy[4] - xy[2])) {
>> + return 0;
>> + } else {
>> + /* recursively split triangle into smaller ones */
>> + return split_triangle(coord, xy + 0, xy + 2, xy + 4,
>> + uv + 0, uv + 2, uv + 4, avail);
>> + }
>> + }
>> +
>> + /* clamp texture coordinates to not exceed input dimensions */
>> + static void clamp_texture(__u16 *uv, float *UV, int w, int h)
>> + {
>> + float t;
>> +
>> + t = UV[0];
>> + if (t < 0)
>> + uv[0] = 0;
>> + t *= w;
>> + if (t > w - 1)
>> + uv[0] = w - 1;
>> + else
>> + uv[0] = round(t);
>> +
>> + t = UV[1];
>> + if (t < 0)
>> + uv[1] = 0;
>> + t *= h;
>> + if (t > h - 1)
>> + uv[1] = h - 1;
>> + else
>> + uv[1] = round(t);
>> + }
>> +
>> + /* clamp vertex coordinates */
>> + static int clamp_vertex(__s16 *xy, float *XY, int W, int H)
>> + {
>> + float x = round(XY[0] * W), y = round(XY[1] * H), z = XY[2];
>> +
>> + if (z < 0.1)
>> + return 0;
>> + if (x < -(256 << IMR_DST_SUBSAMPLE) || x >= W + (256 << IMR_DST_SUBSAMPLE))
>> + return 0;
>> + if (y < -(256 << IMR_DST_SUBSAMPLE) || y >= H + (256 << IMR_DST_SUBSAMPLE))
>> + return 0;
>> +
>> + xy[0] = (__s16)x;
>> + xy[1] = (__s16)y;
>> +
>> + return 1;
>> + }
>> +
>> + /* create arbitrary mesh */
>> + struct imr_map_desc *imr_map_create(struct imr_device *dev,
>> + float *uv, float *xy, int n)
>> + {
>> + struct imr_map_desc *desc;
>> + struct imr_vbo *vbo;
>> + struct imr_full_coord *coord;
>> + int j, m, w, h, W, H;
>> +
>> + /* create a configuration structure */
>> + desc = malloc(sizeof(*desc) + sizeof(*vbo) + 3 * n * sizeof(*coord));
>> + if (!desc)
>> + return NULL;
>> +
>> + /* fill-in VBO coordinates */
>> + vbo = (void *)(desc + 1);
>> + coord = (void *)(vbo + 1);
>> +
>> + /* calculate source/destination dimensions in subpixel coordinates */
>> + w = dev->w << IMR_SRC_SUBSAMPLE;
>> + h = dev->h << IMR_SRC_SUBSAMPLE;
>> + W = dev->W << IMR_DST_SUBSAMPLE;
>> + H = dev->H << IMR_DST_SUBSAMPLE;
>> +
>> + /* put at most N triangles into mesh descriptor */
>> + for (j = 0, m = 0; j < n && m < n; j++, xy += 9, uv += 6) {
>> + __u16 UV[6];
>> + __s16 XY[6];
>> + int k;
>> +
>> + /* translate model coordinates to fixed-point */
>> + if (!clamp_vertex(XY + 0, xy + 0, W, H))
>> + continue;
>> + if (!clamp_vertex(XY + 2, xy + 3, W, H))
>> + continue;
>> + if (!clamp_vertex(XY + 4, xy + 6, W, H))
>> + continue;
>> +
>> + /* translate source coordinates */
>> + clamp_texture(UV + 0, uv + 0, w, h);
>> + clamp_texture(UV + 2, uv + 2, w, h);
>> + clamp_texture(UV + 4, uv + 4, w, h);
>> +
>> + /* process single triangle */
>> + k = process_triangle(coord, UV, XY, n - m);
>> + if (k != 0) {
>> + /* advance vertex coordinates pointer */
>> + coord += 3 * k;
>> + m += k;
>> + }
>> + }
>> +
>> + /* put number of triangles in VBO */
>> + vbo->num = m;
>> +
>> + /* fill-in descriptor */
>> + desc->type = IMR_MAP_UVDPOR(IMR_SRC_SUBSAMPLE) |
>> + (IMR_DST_SUBSAMPLE ? IMR_MAP_DDP : 0);
>> + desc->size = (void *)coord - (void *)vbo;
>> + desc->data = (__u64)vbo;
>> +
>> + return desc;
>> + }
>> +
>> + /* create rectangular mesh */
>> + struct imr_map_desc *imr_map_mesh_src(struct imr_device *dev, float *uv,
>> + int rows, int columns,
>> + float x0, float y0, float dx, float dy)
>> + {
>> + struct imr_map_desc *desc;
>> + struct imr_mesh *mesh;
>> + struct imr_src_coord *coord;
>> + int k, w, h, W, H;
>> +
>> + /* create a configuration structure */
>> + desc = malloc(sizeof(*desc) + sizeof(*mesh) + rows * columns *
>> + sizeof(*coord));
>> + if (!desc)
>> + return NULL;
>> +
>> + /* fill-in rectangular mesh coordinates */
>> + mesh = (void *)(desc + 1);
>> + coord = (void *)(mesh + 1);
>> +
>> + /* calculate source/destination dimensions in subpixel coordinates */
>> + w = dev->w << IMR_SRC_SUBSAMPLE;
>> + h = dev->h << IMR_SRC_SUBSAMPLE;
>> + W = dev->W << IMR_DST_SUBSAMPLE;
>> + H = dev->H << IMR_DST_SUBSAMPLE;
>> +
>> + /* set mesh parameters */
>> + mesh->rows = rows;
>> + mesh->columns = columns;
>> + mesh->x0 = (__u16)round(x0 * W);
>> + mesh->y0 = (__u16)round(y0 * H);
>> + mesh->dx = (__u16)round(dx * W);
>> + mesh->dy = (__u16)round(dy * H);
>> +
>> + /* put mesh coordinates */
>> + for (k = 0; k < rows * columns; k++, uv += 2, coord++) {
>> + __u16 UV[2];
>> +
>> + /* transform point into texture coordinates */
>> + clamp_texture(UV, uv, w, h);
>> +
>> + /* fill the mesh */
>> + coord->u = UV[0];
>> + coord->v = UV[1];
>> + }
>> +
>> + /* fill-in descriptor */
>> + desc->type = IMR_MAP_MESH | IMR_MAP_AUTODG |
>> + IMR_MAP_UVDPOR(IMR_SRC_SUBSAMPLE) |
>> + (IMR_DST_SUBSAMPLE ? IMR_MAP_DDP : 0);
>> + desc->size = (void *)coord - (void *)mesh;
>> + desc->data = (__u64)mesh;
>> +
>> + return desc;
>> + }
>
> <snip>
>
>> Index: media_tree/include/uapi/linux/rcar_imr.h
>> ===================================================================
>> --- /dev/null
>> +++ media_tree/include/uapi/linux/rcar_imr.h
>> @@ -0,0 +1,182 @@
>> +/*
>> + * 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 {
>> + /* bitmask of the mapping type (see below) */
>> + __u32 type;
>> +
>> + /* total data size */
>> + __u32 size;
>> +
>> + /* data user-pointer */
>> + __u64 data;
>> +} __attribute__((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)
>> +
>> +/* luma correction flag */
>> +#define IMR_MAP_LUCE (1 << 3)
>> +
>> +/* chroma correction flag */
>> +#define IMR_MAP_CLCE (1 << 4)
>> +
>> +/* vertex clockwise-mode order */
>> +#define IMR_MAP_TCM (1 << 5)
>> +
>> +/* source coordinate decimal point position */
>> +#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 scale 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)
>> +
>> +/* chroma (U) correction scale 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)
>> +
>> +/* chroma (V) correction scale 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)
>
> You need to document all these bits in the documentation. I.e. TCM and DDP
> are not currently documented.
>
> It helps a lot to understand these defines if the documentation and the
> comments explain what the abbreviations mean. E.g. what does DDP stand
> for, or YLDPO? It's an alphabet soup right now.
>
>> +
>> +/* regular mesh specification */
>> +struct imr_mesh {
>
> How about imr_rectangles? There is no indication in the struct name that this
> describes a rectangle.
>
>> + /* rectangular mesh size */
>> + __u16 rows, columns;
>> +
>> + /* auto-generated mesh parameters */
>> + __u16 x0, y0, dx, dy;
>> +} __attribute__((packed));
>> +
>> +/* vertex-buffer-object (VBO) descriptor */
>> +struct imr_vbo {
>> + /* number of triangles */
>> + __u16 num;
>> +} __attribute__((packed));
>> +
>> +/*******************************************************************************
>> + * Vertex-related structures
>> + ******************************************************************************/
>> +
>> +/* source coordinates */
>> +struct imr_src_coord {
>> + /* vertical, horizontal */
>> + __u16 v, u;
>
> Confusing: why isn't this 'v, h;' given the comment? Or does this refer to
> chroma (U and V)? And what does 'vertical, horizontal' mean anyway? Vertical
> what?
>
>> +} __attribute__((packed));
>> +
>> +/* destination coordinates */
>> +struct imr_dst_coord {
>> + /* vertical, horizontal */
>
> Confusing comment as well. I assume y and x are simply coordinates of a vertex?
> Just say so. This comment doesn't mean anything.
>
>> + __u16 y, x;
>> +} __attribute__((packed));
>> +
>> +/* luma correction parameters */
>> +struct imr_luma_correct {
>> + /* offset */
>> + __s8 lofst;
>> +
>> + /* scale */
>> + __u8 lscal;
>> +
>> + __u16 reserved;
>
> Why is this reserved? Is that for padding? If so, then add a comment that mentions
> that.
>
>> +} __attribute__((packed));
>> +
>> +/* chroma correction parameters */
>> +struct imr_chroma_correct {
>> + /* V value offset */
>> + __s8 vrofs;
>> +
>> + /* V value scale */
>> + __u8 vrscl;
>> +
>> + /* U value offset */
>> + __s8 ubofs;
>> +
>> + /* V value scale */
>> + __u8 ubscl;
>> +} __attribute__((packed));
>> +
>> +/* fully specified source/destination coordinates */
>> +struct imr_full_coord {
>> + struct imr_src_coord src;
>> + struct imr_dst_coord dst;
>> +} __attribute__((packed));
>> +
>> +/* auto-generated coordinates with luma or chroma correction */
>> +struct imr_auto_coord_any_correct {
>> + union {
>> + struct imr_src_coord src;
>> + struct imr_dst_coord dst;
>
> Why have separate imr_src_coord and imr_dst_coord structs? Why not just
> call it imr_coord? I think that is part of the reason of my confusion
> regarding understanding those structs.
>
> The field name here indicates whether it is a source or destination,
> the coordinate information is the same for both.
>
>> + };
>> + union {
>> + struct imr_luma_correct luma;
>> + struct imr_chroma_correct chroma;
>> + };
>> +} __attribute__((packed));
>> +
>> +/* auto-generated coordinates with both luma and chroma correction */
>> +struct imr_auto_coord_both_correct {
>> + union {
>> + struct imr_src_coord src;
>> + struct imr_dst_coord dst;
>> + };
>> + struct imr_luma_correct luma;
>> + struct imr_chroma_correct chroma;
>> +} __attribute__((packed));
>> +
>> +/* fully specified coordinates with luma or chroma correction */
>> +struct imr_full_coord_any_correct {
>> + struct imr_src_coord src;
>> + struct imr_dst_coord dst;
>> + union {
>> + struct imr_luma_correct luma;
>> + struct imr_chroma_correct chroma;
>> + };
>> +} __attribute__((packed));
>> +
>> +/* fully specified coordinates with both luma and chroma correction */
>> +struct imr_full_coord_both_correct {
>> + struct imr_src_coord src;
>> + struct imr_dst_coord dst;
>> + struct imr_luma_correct luma;
>> + struct imr_chroma_correct chroma;
>> +} __attribute__((packed));
>> +
>> +/*******************************************************************************
>> + * Private IOCTL codes
>> + ******************************************************************************/
>> +
>> +#define VIDIOC_IMR_MESH _IOW('V', BASE_VIDIOC_PRIVATE + 0, struct imr_map_desc)
>> +
>> +#endif /* __RCAR_IMR_H */
>>
>
> Do you know the typical number of rectangles or triangles that are passed to this
> function? Is there an upper hardware limit?
>
> I ask, because I wonder whether using a fixed vertex struct like imr_full_coord_both_correct
> for all variations isn't much simpler. The driver just ignores the fields it doesn't
> need in that case.
>
> Yes, you get some memory overhead, but the code for both userspace and kernelspace
> will be much simpler.
I just had a brainwave: rather than making these complicated structure variants,
why not just do this (just thrown together, I didn't look at alignment etc.):
struct imr_map_desc {
/* bitmask of the mapping type (see below) */
__u32 type;
/* total data size */
__u32 size;
/* number of vertices */
__u32 vertices;
/* rectangular mesh size (ignored for triangles) */
__u16 rows, columns;
/* auto-generated mesh parameters (ignored if not auto-generated) */
__u16 x0, y0, dx, dy;
/* data user-pointer */
__u64 src_coords;
__u64 dst_coords;
__u64 luma_corr;
__u64 chroma_corr;
} __attribute__((packed));
Just leave the corresponding data pointer 0 when not needed, and otherwise
these data pointers point to a struct imr_coord, imr_luma_corr or imr_chroma_corr
arrays. Much simpler and still memory efficient.
BTW, I prefer either _corr or _correction over _correct. 'correct' is confusing
since it is also a normal english word and it is not obvious that it is an
abbreviation for 'correction'.
Regards,
Hans
