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

[Sergei Shtylyov]

On 04/16/2018 04:27 PM, Geert Uytterhoeven wrote:

>> 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...
>>
>> Based on the original patch by Konstantin Kozhevnikov.
>>
>> Signed-off-by: Konstantin Kozhevnikov 
>> 
>> Signed-off-by: Sergei Shtylyov 
>> Acked-by: Rob Herring 
> 
>>  Documentation/devicetree/bindings/media/rcar_imr.txt |   27
>>  Documentation/media/v4l-drivers/index.rst|1
>>  Documentation/media/v4l-drivers/rcar_imr.rst |  372 +++
>>  drivers/media/platform/Kconfig   |   13
>>  drivers/media/platform/Makefile  |1
>>  drivers/media/platform/rcar_imr.c| 1832 
>> +++
>>  include/uapi/linux/rcar_imr.h|  182 +
>>  7 files changed, 2428 insertions(+)
> 
> What's the status of this patch?

   Changes requested, and I'm still having no bandwidth to make them... 

> The compatible value "renesas,r8a7796-imr-lx4" has been in use since v4.14.

   That's because the SoC bindings are unlikely to change...

> Thanks!
> 
> Gr{oetje,eeting}s,
> 
> Geert

MBR, Sergei

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

[Geert Uytterhoeven]

Hi Sergei,

On Fri, Aug 4, 2017 at 8:03 PM, Sergei Shtylyov
 wrote:
> 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...
>
> Based on the original patch by Konstantin Kozhevnikov.
>
> Signed-off-by: Konstantin Kozhevnikov 
> 
> Signed-off-by: Sergei Shtylyov 
> Acked-by: Rob Herring 

>  Documentation/devicetree/bindings/media/rcar_imr.txt |   27
>  Documentation/media/v4l-drivers/index.rst|1
>  Documentation/media/v4l-drivers/rcar_imr.rst |  372 +++
>  drivers/media/platform/Kconfig   |   13
>  drivers/media/platform/Makefile  |1
>  drivers/media/platform/rcar_imr.c| 1832 
> +++
>  include/uapi/linux/rcar_imr.h|  182 +
>  7 files changed, 2428 insertions(+)

What's the status of this patch?
The compatible value "renesas,r8a7796-imr-lx4" has been in use since v4.14.

Thanks!

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 v7] media: platform: Renesas IMR driver

[Sergei Shtylyov]

Hello!

On 08/17/2017 10:59 AM, Hans Verkuil wrote:


A quick review. I'm concentrating on the mesh ioctl, since that's what sets this
driver apart.


   OK, waiting for the detailed review...


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.


   Sure, I can.


+
+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.


   Well, in my Russian mind, mesh consists of the 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?


   No, there are no source or destination (not both) coordinate structures 
present in the vertex object; at least one of them must always be there.



Since it's auto generated by the hardware?


   Yes.


I believe we discussed in the past whether 'type' should be split in a 'type'
and 'flags' field.


   In this version, I still tried Cogent's original userland working, so the 
structures were left intact (aside of renaming).



+
+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?


   I think I have explained the IRM_MAPO_AUTO* bits above.


+    ==  ==  
===
+\   ``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 

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

[Hans Verkuil]

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:
> 
> 
> 
>> 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 

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

[Hans Verkuil]

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:



> 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 

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

[Hans Verkuil]

Hi Sergei,

A few high level comments (I'll look at the patch itself later):

- There is no MAINTAINERS entry, please add one.
- Don't attach the patch, post it inline (ideally with 'git send-email')
- Split up the patch into 4 separate patches: bindings, doc changes,
  driver and MAINTAINERS patch. This will make it easier to review.
- Please give the v4l2-compliance output in the cover letter of the v8
  patch series. I can't merge this driver without being certain there
  are no compliance issues.
- You also have IMR-LSX3 and IMR-LX3 patches, why not add them to the
  patch series? I can review the set as a single unit. Up to you, though.

Regards,

Hans

[PATCH v7] media: platform: Renesas IMR driver

[Sergei Shtylyov]

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...

Based on the original patch by Konstantin Kozhevnikov.

Signed-off-by: Konstantin Kozhevnikov 

Signed-off-by: Sergei Shtylyov 
Acked-by: Rob Herring 

---
The patch is against the 'media_tree.git' repo's 'master' branch.

Changes in version 7:
- switched to using 'v4l2_fh::m2m_ctx' which permitted using v4l2_m2m_fop_{poll|
  mmap} instead of imr_{poll|mmap}();
- moved the configuration check from imr_qbuf() to imr_buf_prepare(), replacing
  the former with v4l2_m2m_ioctl_qbuf();
- renamed imr_cfg_[un]ref() to imr_cfg_{get|put}();
- removed VB2_USERPTR from imr_queue_init();
- removed V4L2_CAP_VIDEO_{CAPTURE|OUTPUT} device capabilities;
- replaced "luminance" with "luma" and "chrominance" with "chroma" in the
  driver and UAPI header comments;
- fixed typo in the email address in MODULE_AUTHOR();
- added the vertex-related structures to the UAPI header;
- replaced  the '__packed' annotations with explicit __attribute__((packed)) in
  the UAPI  header;
- fixed/clarified the comments in the UAPI  header;
- added the dependence table for  the vertex object variants in the driver
  document;
- added the description of the luma/chroma correction in the driver document;
- added the mesh construction code example to the driver document;
- removed the fragments effectively duplicating the UAPI hearder comments in
  the driver document, referring a reader to that header instead;
- clarified the use of the auto-generated source/destination coordinates in
  the driver document;
- marked up the references to the code in the driver document;
- fixed typo/wording in the driver document;
- removed the main text indentation in the driver document;
- changed the patch authorship to mine;
- added Rob Herring's ACK.

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_map();
- moved setting device capabilities from imr_querycap() to imr_probe();
- replaced imr_{reqbufs|querybuf|dqbuf|expbuf|streamon|streamoff}() with the
  generic helpers, implemented vidioc_{create_bufs|prepare_buf}() methods;
- set the valid default queue pixel format in imr_probe();
- used tabs for indentation where possible in imr_probe();
- removed the rest of the *inline* keywords;
- declared 'imr_map_desc::data' as '__u64' instead of 'void *';
- switched to '__u{16|32}' instead of 'u{16|32}' in the UAPI header;
- spelled out the VBO abbreviation and removed redundancy in the UAPI header
  comments.

Changes in version 5:
- used ALIGN() macro in imr_ioctl_map();
- moved the display list #define's after the register #define's, removing the
  redundant comment;
- uppercased the "tbd" abbreviation in the comments;
- made the TRI instruction types A/B/C named consistently;
- avoided quotes around the coordinate's names;
- avoided some  hyphens in the comments;
- removed spaces around / and before ? in the comments;
- reworded some comments;
- reformatted some multiline comments;
- fixed typos in the comments.

Changes in version 4:
- added/used the SUSR fields/shifts.

Changes in version 3:
- added/used the {UV|CP}DPOR fields/shifts;
- removed unsupported LINE instruction;
- replaced '*' with 'x' in the string passed to v4l2_dbg()