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