Re: [PATCH v6] media: platform: Renesas IMR driver

[Hans Verkuil]

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

[Konstantin Kozhevnikov]

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

[Sergei Shtylyov]

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

[Sergei Shtylyov]

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, 

Re: [PATCH v6] media: platform: Renesas IMR driver

[Hans Verkuil]

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

[Hans Verkuil]

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 

Re: [PATCH v6] media: platform: Renesas IMR driver

[Geert Uytterhoeven]

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

[Sergei Shtylyov]

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

[Rob Herring]

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

[Sergei Shtylyov]

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