Re: [PATCH v6] media: platform: Renesas IMR driver
On 08/07/17 15:31, Konstantin Kozhevnikov wrote: > Hello all, > > the sample is made publicly available, and can be taken from > https://github.com/CogentEmbedded/imr-sv-utest/blob/master/utest/utest-imr.c. > > It doesn't show how luminance/chrominance correction actually works, however. > That feature has been tested once a while ago, and probably we are going to > release that soon. > > Regarding usage of "chromacity" word in the comments, I actually meant > "chrominance" or "chroma". The difference for non-native speaker is probably > a bit vague, just like one between "luminance" and > "luminosity". In the R-Car manual it is referred to as "hue", but what is > meant is the "luma" and "chroma". These short forms seem a bit weird to me, > hence I was using the words "luminance" and > "chromacity". If that's confusing, I don't mind them be replaced with just > "luma"/"chroma". luma/chroma works well for me. 'chromacity' was confusing for me because it is close to 'chromaticity' which is a valid word but it has a different meaning. > > For documentation part, I am not 100% sure Renesas is okay with disclosing > each and every detail, it might be the case we should obtain a permit from > their legals. Still, I believe the person who is > about to use the driver is having an access to at least Technical Reference > Manual of R-Car Gen2/3, so adding a reference to a chapter in TRM will most > likely be sufficient. A reference to the TRM is fine. I assume people who write code for this will have the TRM available. Regarding the code: having that example code is certainly useful, but what I am really looking for is a code snippet that allocates and fills in the data to pass on with the ioctl. Especially the handling of the 'type' field remains very fuzzy to me. Especially the fact that it is a bitmask is strange. Usually a type is an enum that defines how to interpret the struct (e.g. similar to type in struct v4l2_format). I get the feeling your mixing type with flags. Perhaps if you split it up into a type and flags field things will make more sense. > > The question about usage of "user-pointer" buffers (V4L2_MEMORY_USERPTR) is a > bit confusing. Indeed, right now we are using that scheme, though I wouldn't > say we are absolutely required to do that. > Specifically, we are allocating the contiguous buffers using Renesas' > proprietary "mmngr" driver (it's not a rocket science thing, but it's made > proprietary for some reason). Then we are using the > buffers for various persons, both in EGL and in IMR. I guess we are okay with > moving completely to DMA-fd (given the fact we have an accompanying driver > "mmngrbuf" which serves for translation of > memory pointers to DMA-fds for further cross-process sharing and stuff). I > mean, if USERPTR is tagged as an obsolete / deprecated function, we are fine > with dropping it. However, there is one thing > I'd like to understand from V4L2 experts. I do see sometimes (during > application closing or shortly after it) the bunches of warnings from kernel > regarding "corrupted" MMU state (don't recall exactly, > but it sounds like page which is supposed to be free gets somehow corrupted). > Is that something that is related to (mis)use of USERPTR? I was trying to > find out if there is any memory corruption > caused by application logic, came to conclusion it's not. To me it looks like > a race condition between unmapping of VMAs and V4L2 buffers deallocation > which yields sometimes unpredictable results. Can > you please provide some details about possible issues with usage of USERPTR > with DMA-contiguous buffer driver, it would be good to find a match. I am not aware of any warnings like you described. Originally USERPTR was designed to be used with scatter-gather DMA engines. It requires a really good DMA engine that is able to handle partial pages and address alignments of 8 bytes or less. That way it allowed userspace to use malloc to allocate the buffers. Later it was abused for embedded systems that used contiguous DMA but had special code that carved out memory. Userspace would pass a pointer to the driver using the USERPTR API and the driver would verify that it indeed pointed to physically contiguous memory. When dmabuf came along (together with the CMA subsystem) the need for abusing USERPTR in that way disappeared and our recommendation for new drivers is to always support DMABUF and only use USERPTR if the hardware can *really* handle malloc()ed buffers. I.e. without special alignment requirements such as the buffer must start at a page boundary. Most hardware fails that test. In practice we discourage the use of USERPTR for embedded systems. Apologies for the delay in replying, I was on vacation last week. Regards, Hans
Re: [PATCH v6] media: platform: Renesas IMR driver
Hello! On 07/06/2017 09:16 PM, Sergei Shtylyov wrote: [...] += + +This file documents some driver-specific aspects of the IMR driver, such as +driver-specific ioctls. + +The ioctl reference +~~~ + +VIDIOC_IMR_MESH - Set mapping data +^^ + +Argument: struct imr_map_desc + +**Description**: + +This ioctl sets up the mesh using which the input frames will be s/using/through/ +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). + +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 bits were 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. What if any of the other types are used like IMR_MAP_LUCE? IMR_MAP_LUCE only affects the vertex object. Is this documented in a Renesas datasheet? Yes. Well, not exactly. The different mesh types are a software concept, the hardware only understands series of triangles. [...] Regards, Hans MBR, Sergei
Re: [PATCH v6] media: platform: Renesas IMR driver
Hello all, the sample is made publicly available, and can be taken from https://github.com/CogentEmbedded/imr-sv-utest/blob/master/utest/utest-imr.c. It doesn't show how luminance/chrominance correction actually works, however. That feature has been tested once a while ago, and probably we are going to release that soon. Regarding usage of "chromacity" word in the comments, I actually meant "chrominance" or "chroma". The difference for non-native speaker is probably a bit vague, just like one between "luminance" and "luminosity". In the R-Car manual it is referred to as "hue", but what is meant is the "luma" and "chroma". These short forms seem a bit weird to me, hence I was using the words "luminance" and "chromacity". If that's confusing, I don't mind them be replaced with just "luma"/"chroma". For documentation part, I am not 100% sure Renesas is okay with disclosing each and every detail, it might be the case we should obtain a permit from their legals. Still, I believe the person who is about to use the driver is having an access to at least Technical Reference Manual of R-Car Gen2/3, so adding a reference to a chapter in TRM will most likely be sufficient. The question about usage of "user-pointer" buffers (V4L2_MEMORY_USERPTR) is a bit confusing. Indeed, right now we are using that scheme, though I wouldn't say we are absolutely required to do that. Specifically, we are allocating the contiguous buffers using Renesas' proprietary "mmngr" driver (it's not a rocket science thing, but it's made proprietary for some reason). Then we are using the buffers for various persons, both in EGL and in IMR. I guess we are okay with moving completely to DMA-fd (given the fact we have an accompanying driver "mmngrbuf" which serves for translation of memory pointers to DMA-fds for further cross-process sharing and stuff). I mean, if USERPTR is tagged as an obsolete / deprecated function, we are fine with dropping it. However, there is one thing I'd like to understand from V4L2 experts. I do see sometimes (during application closing or shortly after it) the bunches of warnings from kernel regarding "corrupted" MMU state (don't recall exactly, but it sounds like page which is supposed to be free gets somehow corrupted). Is that something that is related to (mis)use of USERPTR? I was trying to find out if there is any memory corruption caused by application logic, came to conclusion it's not. To me it looks like a race condition between unmapping of VMAs and V4L2 buffers deallocation which yields sometimes unpredictable results. Can you please provide some details about possible issues with usage of USERPTR with DMA-contiguous buffer driver, it would be good to find a match. (Sorry, it got pretty long) Sincerely, Kostya On 06/07/17 21:16, Sergei Shtylyov wrote: Hello! On 07/03/2017 03:43 PM, Hans Verkuil wrote: Index: media_tree/Documentation/media/v4l-drivers/rcar_imr.rst === --- /dev/null +++ media_tree/Documentation/media/v4l-drivers/rcar_imr.rst @@ -0,0 +1,86 @@ +Renesas R-Car Image Rendeder (IMR) Driver Rendeder -> Renderer Oops, sorry. :-) += + +This file documents some driver-specific aspects of the IMR driver, such as +driver-specific ioctls. + +The ioctl reference +~~~ + +VIDIOC_IMR_MESH - Set mapping data +^^ + +Argument: struct imr_map_desc + +**Description**: + +This ioctl sets up the mesh using which the input frames will be s/using/through/ +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). + +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 bits were 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. What if any of the other types are used like IMR_MAP_LUCE? IMR_MAP_LUCE only affects the vertex object. Is this documented in a Renesas datasheet? Yes. If so, add a reference to that in this documentation. Unfortunately it's not publicly available. + +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. + +A vertex object has a complex structure: + +.. code-block:: none + +__u16vvertical \ source coordinates (only present +__u16uhorizontal / if IMR_MAP_AUTOSG isn't set) +__u16Yvertical \ destination coordinates (only here +__u16Xhorizontal / if IMR_MAP_AUTODG isn't set)
Re: [PATCH v6] media: platform: Renesas IMR driver
Hello! On 07/03/2017 03:43 PM, Hans Verkuil wrote: Index: media_tree/Documentation/media/v4l-drivers/rcar_imr.rst === --- /dev/null +++ media_tree/Documentation/media/v4l-drivers/rcar_imr.rst @@ -0,0 +1,86 @@ +Renesas R-Car Image Rendeder (IMR) Driver Rendeder -> Renderer Oops, sorry. :-) += + +This file documents some driver-specific aspects of the IMR driver, such as +driver-specific ioctls. + +The ioctl reference +~~~ + +VIDIOC_IMR_MESH - Set mapping data +^^ + +Argument: struct imr_map_desc + +**Description**: + +This ioctl sets up the mesh using which the input frames will be s/using/through/ +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). + +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 bits were 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. What if any of the other types are used like IMR_MAP_LUCE? IMR_MAP_LUCE only affects the vertex object. Is this documented in a Renesas datasheet? Yes. If so, add a reference to that in this documentation. Unfortunately it's not publicly available. + +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. + +A vertex object has a complex structure: + +.. code-block:: none + +__u16vvertical \ source coordinates (only present +__u16uhorizontal / if IMR_MAP_AUTOSG isn't set) +__u16Yvertical \ destination coordinates (only here +__u16Xhorizontal / if IMR_MAP_AUTODG isn't set) +__s8lofstoffset \ luminance correction parameters +__u8lscalscale > (only present if IMR_MAP_LUCE +__u16reserved / is set) +__s8vrofsV value offset \ hue correction parameters +__u8vrsclV value scale \ (only present if IMR_MAP_CLCE +__s8ubofsU value offset / is set) +__u8ubsclU value scale / Is this the internal structure? Or something that userspace has to fill in? Yes, the user space have to pass that to the driver which constructs the display lists using these data. It's not clear at all. I recommend giving a few code examples of how this should be used. Konstantin, can we give some examples? + +**Return value**: + +On success 0 is returned. On error -1 is returned and errno is set +appropriately. + +**Data types**: + +.. code-block:: none + +* struct imr_map_desc + +__u32typemapping types This is a bitmask? If so, what combination of bits are allowed? Yes, bitmask. +__u32sizetotal size of the mesh structure +__u64datamap-specific user-pointer + +IMR_MAP_MESHregular mesh specification +IMR_MAP_AUTOSGauto-generated source coordinates +IMR_MAP_AUTODGauto-generated destination coordinates +IMR_MAP_LUCEluminance correction flag +IMR_MAP_CLCEchromacity correction flag You probably mean 'chroma'. 'chromacity' isn't a word. But it's recognized by all online translators I've tried. :-) +IMR_MAP_TCMvertex clockwise-mode order +IMR_MAP_UVDPOR(n)source coordinate decimal point position +IMR_MAP_DDPdestination coordinate sub-pixel mode +IMR_MAP_YLDPO(n)luminance correction offset decimal point +position +IMR_MAP_UBDPO(n)chromacity (U) correction offset decimal point +position +IMR_MAP_VRDPO(n)chromacity (V) correction offset decimal point +position There is no documentation what how these types relate to IMR_MAP_MESH and IMR_MAP_AUTOS/DG. They are basically orthogonal, IIRC. + +* struct imr_meshregular mesh specification + +__u16rows, columnsrectangular mesh sizes +__u16x0, y0, dx, dyauto-generated mesh parameters + +* struct imr_vbovertex-buffer-object (VBO) descriptor + +__u16numnumber of triangles Sorry, this needs more work. Sigh, everybody hates writing docs, I guess... :-) Regards, Hans MBR, Sergei
Re: [PATCH v6] media: platform: Renesas IMR driver
Hello! On 07/03/2017 03:25 PM, Hans Verkuil wrote: From: Konstantin Kozhevnikov 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 and the UAPI documentation, 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, replaced 'imr_ctx::crop' array with the 'imr_ctx::rect' structure, replaced imr_{g|s}_crop() with imr_{g|s}_selection(), completely rewrote imr_queue_setup(), removed 'imr_format_info::name', moved the applicable code from imr_buf_queue() to imr_buf_prepare() and moved the rest of imr_buf_queue() after imr_buf_finish(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), removed imr_start_streaming(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), clarified the math in imt_tri_type_{a|b|c}_length(), clarified the pointer math and avoided casts to 'void *' in imr_tri_set_type_{a|b|c}(), replaced imr_{reqbufs|querybuf| dqbuf|expbuf|streamon|streamoff}() with the generic helpers, implemented vidioc_{create_bufs|prepare_buf}() methods, used ALIGN() macro and merged the matrix size checks and replaced kmalloc()/copy_from_user() calls with memdup_user() call in imr_ioctl_map(), moved setting device capabilities from imr_querycap() to imr_probe(), set the valid default queue format in imr_probe(), removed leading dots and fixed grammar in the comments, fixed up the 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 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, 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, added useful local variable, uppercased and spelled out 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, declared 'imr_map_desc::data' as '__u64' instead of 'void *', switched to '__u{16|32}' in the UAPI header, enclosed the macro parameters in parens, exchanged the values of IMR_MAP_AUTO{S|D}G macros.] As Geert suggested, just replace this with a 'Based-on' line. OK, the list grew too long indeed. :-) Index: media_tree/drivers/media/platform/rcar_imr.c === --- /dev/null +++ media_tree/drivers/media/platform/rcar_imr.c @@ -0,0 +1,1877 @@ +/* add reference to the current configuration */ +static struct imr_cfg *imr_cfg_ref(struct imr_ctx *ctx) imr_cfg_ref -> imr_cfg_ref_get OK, but imr_cfg_get() seems a better name. +{ +struct imr_cfg *cfg = ctx->cfg; + +BUG_ON(!cfg); Perhaps this can be replaced by: if (WARN_ON(!cfg)) return NULL; I'm afraid imr_device_run() will cause oops in this case... +cfg->refcount++; +return cfg; +} + +/* mesh configuration destructor */ +static void imr_cfg_unref(struct imr_ctx *ctx, struct imr_cfg *cfg) imr_cfg_unref -> imr_cfg_ref_put OK, but I'll call it imr_cfg_put(). That follows the standard naming conventions for refcounting. +{ +struct imr_device *imr = ctx->imr; + +/* no atomicity is required as operation is locked with device mutex */ +if (!cfg || --cfg->refcount) +return; + +/* release memory allocated for a display list */ +if (cfg->dl_vaddr) +dma_free_writecombine(imr->dev, cfg->dl_size, cfg->dl_vaddr, +
Re: [PATCH v6] media: platform: Renesas IMR driver
On 06/23/2017 10:34 PM, Sergei Shtylyov wrote: Index: media_tree/Documentation/media/v4l-drivers/rcar_imr.rst === --- /dev/null +++ media_tree/Documentation/media/v4l-drivers/rcar_imr.rst @@ -0,0 +1,86 @@ +Renesas R-Car Image Rendeder (IMR) Driver Rendeder -> Renderer += + +This file documents some driver-specific aspects of the IMR driver, such as +driver-specific ioctls. + +The ioctl reference +~~~ + +VIDIOC_IMR_MESH - Set mapping data +^^ + +Argument: struct imr_map_desc + +**Description**: + + This ioctl sets up the mesh using which the input frames will be s/using/through/ + 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). + + 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 bits were 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. What if any of the other types are used like IMR_MAP_LUCE? Is this documented in a Renesas datasheet? If so, add a reference to that in this documentation. + + 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. + + A vertex object has a complex structure: + +.. code-block:: none + + __u16 v vertical \ source coordinates (only present + __u16 u horizontal / if IMR_MAP_AUTOSG isn't set) + __u16 Y vertical \ destination coordinates (only here + __u16 X horizontal / if IMR_MAP_AUTODG isn't set) + __s8lofst offset \ luminance correction parameters + __u8lscal scale > (only present if IMR_MAP_LUCE + __u16 reserved / is set) + __s8vrofs V value offset \ hue correction parameters + __u8vrscl V value scale \ (only present if IMR_MAP_CLCE + __s8ubofs U value offset / is set) + __u8ubscl U value scale / Is this the internal structure? Or something that userspace has to fill in? It's not clear at all. I recommend giving a few code examples of how this should be used. + +**Return value**: + + On success 0 is returned. On error -1 is returned and errno is set + appropriately. + +**Data types**: + +.. code-block:: none + + * struct imr_map_desc + + __u32 typemapping types This is a bitmask? If so, what combination of bits are allowed? + __u32 sizetotal size of the mesh structure + __u64 datamap-specific user-pointer + + IMR_MAP_MESHregular mesh specification + IMR_MAP_AUTOSG auto-generated source coordinates + IMR_MAP_AUTODG auto-generated destination coordinates + IMR_MAP_LUCEluminance correction flag + IMR_MAP_CLCEchromacity correction flag You probably mean 'chroma'. 'chromacity' isn't a word. + IMR_MAP_TCM vertex clockwise-mode order + IMR_MAP_UVDPOR(n) source coordinate decimal point position + IMR_MAP_DDP destination coordinate sub-pixel mode + IMR_MAP_YLDPO(n)luminance correction offset decimal point + position + IMR_MAP_UBDPO(n)chromacity (U) correction offset decimal point + position + IMR_MAP_VRDPO(n)chromacity (V) correction offset decimal point + position There is no documentation what how these types relate to IMR_MAP_MESH and IMR_MAP_AUTOS/DG. + + * struct imr_mesh regular mesh specification + + __u16 rows, columns rectangular mesh sizes + __u16 x0, y0, dx, dy auto-generated mesh parameters + + * struct imr_vbovertex-buffer-object (VBO) descriptor + + __u16 num number of triangles Sorry, this needs more work. Regards, Hans
Re: [PATCH v6] media: platform: Renesas IMR driver
Hi Sergei, Some comments below: On 06/23/2017 10:34 PM, Sergei Shtylyov wrote: From: Konstantin Kozhevnikov 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 and the UAPI documentation, 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, replaced 'imr_ctx::crop' array with the 'imr_ctx::rect' structure, replaced imr_{g|s}_crop() with imr_{g|s}_selection(), completely rewrote imr_queue_setup(), removed 'imr_format_info::name', moved the applicable code from imr_buf_queue() to imr_buf_prepare() and moved the rest of imr_buf_queue() after imr_buf_finish(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), removed imr_start_streaming(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), clarified the math in imt_tri_type_{a|b|c}_length(), clarified the pointer math and avoided casts to 'void *' in imr_tri_set_type_{a|b|c}(), replaced imr_{reqbufs|querybuf| dqbuf|expbuf|streamon|streamoff}() with the generic helpers, implemented vidioc_{create_bufs|prepare_buf}() methods, used ALIGN() macro and merged the matrix size checks and replaced kmalloc()/copy_from_user() calls with memdup_user() call in imr_ioctl_map(), moved setting device capabilities from imr_querycap() to imr_probe(), set the valid default queue format in imr_probe(), removed leading dots and fixed grammar in the comments, fixed up the 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 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, 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, added useful local variable, uppercased and spelled out 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, declared 'imr_map_desc::data' as '__u64' instead of 'void *', switched to '__u{16|32}' in the UAPI header, enclosed the macro parameters in parens, exchanged the values of IMR_MAP_AUTO{S|D}G macros.] As Geert suggested, just replace this with a 'Based-on' line. Index: media_tree/drivers/media/platform/rcar_imr.c === --- /dev/null +++ media_tree/drivers/media/platform/rcar_imr.c @@ -0,0 +1,1877 @@ +/* add reference to the current configuration */ +static struct imr_cfg *imr_cfg_ref(struct imr_ctx *ctx) imr_cfg_ref -> imr_cfg_ref_get +{ + struct imr_cfg *cfg = ctx->cfg; + + BUG_ON(!cfg); Perhaps this can be replaced by: if (WARN_ON(!cfg)) return NULL; + cfg->refcount++; + return cfg; +} + +/* mesh configuration destructor */ +static void imr_cfg_unref(struct imr_ctx *ctx, struct imr_cfg *cfg) imr_cfg_unref -> imr_cfg_ref_put That follows the standard naming conventions for refcounting. +{ + struct imr_device *imr = ctx->imr; + + /* no atomicity is required as operation is locked with device mutex */ + if (!cfg || --cfg->refcount) + return; + + /* release memory allocated for a display list */ + if (cfg->dl_vaddr) + dma_free_writecombine(imr->dev, cfg->dl_size, cfg->dl_vaddr, + cfg->dl_dma_addr); + + /* destroy the configuration structure */ + kfree(cfg); Ad
Re: [PATCH v6] media: platform: Renesas IMR driver
Hi Sergei, On Mon, Jun 26, 2017 at 9:56 PM, Sergei Shtylyov wrote: > On 06/26/2017 10:49 PM, Rob Herring wrote: >>> From: Konstantin Kozhevnikov >>> >>> 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 [...] >>> macros.] >> >> >> TL;DR needed here IMO. > >Not sure I understand... stands for "too long; didn't read", right? > >> Not sure anyone really cares every detail you >> changed in re-writing this. If they did, it should all be separate >> commits. > >AFAIK this is a way that's things are dealt with when you submit somebody > else's work with your changes. Sorry if the list is too long... Based on a patch by Konstantin Kozhevnikov ? Of course, that's bad for your coworker's patch statistics... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v6] media: platform: Renesas IMR driver
Hello! On 06/26/2017 10:49 PM, Rob Herring wrote: From: Konstantin Kozhevnikov 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 and the UAPI documentation, 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, replaced 'imr_ctx::crop' array with the 'imr_ctx::rect' structure, replaced imr_{g|s}_crop() with imr_{g|s}_selection(), completely rewrote imr_queue_setup(), removed 'imr_format_info::name', moved the applicable code from imr_buf_queue() to imr_buf_prepare() and moved the rest of imr_buf_queue() after imr_buf_finish(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), removed imr_start_streaming(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), clarified the math in imt_tri_type_{a|b|c}_length(), clarified the pointer math and avoided casts to 'void *' in imr_tri_set_type_{a|b|c}(), replaced imr_{reqbufs|querybuf| dqbuf|expbuf|streamon|streamoff}() with the generic helpers, implemented vidioc_{create_bufs|prepare_buf}() methods, used ALIGN() macro and merged the matrix size checks and replaced kmalloc()/copy_from_user() calls with memdup_user() call in imr_ioctl_map(), moved setting device capabilities from imr_querycap() to imr_probe(), set the valid default queue format in imr_probe(), removed leading dots and fixed grammar in the comments, fixed up the 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 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, 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, added useful local variable, uppercased and spelled out 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, declared 'imr_map_desc::data' as '__u64' instead of 'void *', switched to '__u{16|32}' in the UAPI header, enclosed the macro parameters in parens, exchanged the values of IMR_MAP_AUTO{S|D}G macros.] TL;DR needed here IMO. Not sure I understand... stands for "too long; didn't read", right? Not sure anyone really cares every detail you changed in re-writing this. If they did, it should all be separate commits. AFAIK this is a way that's things are dealt with when you submit somebody else's work with your changes. Sorry if the list is too long... Signed-off-by: Konstantin Kozhevnikov Signed-off-by: Sergei Shtylyov I acked v5 and it doesn't seem the binding changed. Sorry, I realized that I'd missed to collect you ACK just after sending v6... I believe there'll be v7 yet, so I'll finally collect it. Rob MBR, Sergei
Re: [PATCH v6] media: platform: Renesas IMR driver
On Fri, Jun 23, 2017 at 11:34:44PM +0300, Sergei Shtylyov wrote: > From: Konstantin Kozhevnikov > > 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 and the UAPI documentation, > 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, replaced 'imr_ctx::crop' array with the 'imr_ctx::rect' > structure, replaced imr_{g|s}_crop() with imr_{g|s}_selection(), completely > rewrote imr_queue_setup(), removed 'imr_format_info::name', moved the > applicable code from imr_buf_queue() to imr_buf_prepare() and moved the > rest of imr_buf_queue() after imr_buf_finish(), assigned 'src_vq->dev' and > 'dst_vq->dev' in imr_queue_init(), removed imr_start_streaming(), assigned > 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), clarified the math in > imt_tri_type_{a|b|c}_length(), clarified the pointer math and avoided casts > to 'void *' in imr_tri_set_type_{a|b|c}(), replaced imr_{reqbufs|querybuf| > dqbuf|expbuf|streamon|streamoff}() with the generic helpers, implemented > vidioc_{create_bufs|prepare_buf}() methods, used ALIGN() macro and merged > the matrix size checks and replaced kmalloc()/copy_from_user() calls with > memdup_user() call in imr_ioctl_map(), moved setting device capabilities > from imr_querycap() to imr_probe(), set the valid default queue format in > imr_probe(), removed leading dots and fixed grammar in the comments, fixed > up the 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 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, 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, added > useful local variable, uppercased and spelled out 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, declared 'imr_map_desc::data' as '__u64' instead > of 'void *', switched to '__u{16|32}' in the UAPI header, enclosed the > macro parameters in parens, exchanged the values of IMR_MAP_AUTO{S|D}G > macros.] TL;DR needed here IMO. Not sure anyone really cares every detail you changed in re-writing this. If they did, it should all be separate commits. > Signed-off-by: Konstantin Kozhevnikov > > Signed-off-by: Sergei Shtylyov I acked v5 and it doesn't seem the binding changed. Rob
[PATCH v6] media: platform: Renesas IMR driver
From: Konstantin Kozhevnikov 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 and the UAPI documentation, 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, replaced 'imr_ctx::crop' array with the 'imr_ctx::rect' structure, replaced imr_{g|s}_crop() with imr_{g|s}_selection(), completely rewrote imr_queue_setup(), removed 'imr_format_info::name', moved the applicable code from imr_buf_queue() to imr_buf_prepare() and moved the rest of imr_buf_queue() after imr_buf_finish(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), removed imr_start_streaming(), assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(), clarified the math in imt_tri_type_{a|b|c}_length(), clarified the pointer math and avoided casts to 'void *' in imr_tri_set_type_{a|b|c}(), replaced imr_{reqbufs|querybuf| dqbuf|expbuf|streamon|streamoff}() with the generic helpers, implemented vidioc_{create_bufs|prepare_buf}() methods, used ALIGN() macro and merged the matrix size checks and replaced kmalloc()/copy_from_user() calls with memdup_user() call in imr_ioctl_map(), moved setting device capabilities from imr_querycap() to imr_probe(), set the valid default queue format in imr_probe(), removed leading dots and fixed grammar in the comments, fixed up the 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 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, 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, added useful local variable, uppercased and spelled out 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, declared 'imr_map_desc::data' as '__u64' instead of 'void *', switched to '__u{16|32}' in the UAPI header, enclosed the macro parameters in parens, exchanged the values of IMR_MAP_AUTO{S|D}G macros.] Signed-off-by: Konstantin Kozhevnikov Signed-off-by: Sergei Shtylyov --- Changes in version 6: - fixed the bug where if imr_cfg_create() fails, 'ctx->cfg' wasn't set to NULL and so wouldn't fail the validity checks; - fixed the height minimum/alignment passed to v4l_bound_align_image(); - removed the buggy !V4L2_TYPE_IS_OUTPUT() check from imr_qbuf(); - added the driver UAPI documentation; - replaced 'imr_ctx::crop' array with the 'imr_ctx::rect' structure; - replaced imr_{g|s}_crop() with imr_{g|s}_selection(); - removed 'imr_format_info::name' and the related code; - completely rewrote imr_queue_setup(); - moved the applicable code from imr_buf_queue() to imr_buf_prepare(), moved the rest of imr_buf_queue() after imr_buf_finish(); - removed imr_start_streaming(); - assigned 'src_vq->dev' and 'dst_vq->dev' in imr_queue_init(); - clarified the math in imt_tri_type_{a|b|c}_length(); - removed useless local variables, added useful variables, avoided casts to 'void *', and clarified the pointer math in imr_tri_set_type_{a|b|c}(); - replaced kmalloc()/copy_from_user() calls with memdup_user() call; - moved the 'type' variable assignment in imr_ioctl_map() after the following comment; - merged the matrix size checks for the cases of the automatic generation pattern and the absolute coordinates in imr_ioctl_map(); - swapped the operands in the VBO size check in imr_ioctl