mbox series

[v8,00/10] Add V4L2 M2M Driver for E5010 JPEG Encoder

Message ID 20240517171532.748684-1-devarsht@ti.com
Headers show
Series Add V4L2 M2M Driver for E5010 JPEG Encoder | expand

Message

Devarsh Thakkar May 17, 2024, 5:15 p.m. UTC
This adds support for V4L2 M2M based driver for E5010 JPEG Encoder
which is a stateful JPEG encoder from Imagination technologies
and is present in TI AM62A SoC.

While adding support for it, following additional framework changes were
made:
 - Moved reference quantization and huffman tables provided in
   ITU-T-REC-T.81 to v4l2-jpeg.c as suggested in mailing list [1].
 - Add macros to round to closest integer (either higher or lower) while
   rounding in order of 2.
 - Add KUnit tests for math functions.

v4l2-compliance test :
Link: https://gist.github.com/devarsht/1f039c631ca953a57f405cfce1b69e49

E5010 JPEG Encoder Manual tests :

Performance:
Link: https://gist.github.com/devarsht/c40672944fd71c9a53ab55adbfd9e28b

Functionality:
Link: https://gist.github.com/devarsht/8e88fcaabff016bb2bac83d89c9d23ce

Compression Quality:
Link: https://gist.github.com/devarsht/cbcc7cd97e8c48ba1486caa2b7884655

Multi Instance:
Link: https://gist.github.com/devarsht/22c2fca08cd3441fb40f2c7a4cebc95a

Crop support:
Link: https://gist.github.com/devarsht/de6f5142f678bb1a5338abfd9f814abd

Runtime PM:
Link: https://gist.github.com/devarsht/70cd95d4440ddc678489d93885ddd4dd

Math lib KUnit tests:
Link: https://gist.github.com/devarsht/3f9042825be3da4e133b8f4eda067876

[1]: 
https://lore.kernel.org/all/de46aefe-36da-4e1a-b4fa-b375b2749181@xs4all.nl/

Changelog:
V7->V8:
 - Add KUnit tests for math functions
 - Add roundclosest() for supporting rounding for non-multiple of 2
 - Update commit message as suggested
 - Add Reviewed-by and Acked-by tags to patches as received

V6->V7:
 - Fix cropping support
 - Move reference huffman and quantization tables to v4l2-jpeg.c
 - Fix suspend/resume use-case
 - Add Reviewed-by

V5->V6:
 - Fix sparse warnings

V4->V5:
 - Sort the #includes in driver file alphabetically
 - Rename huffman and quantization tables to not use '_'
 - Add Reviewed-by tag

V3->V4:
- Use ti-specific compatible ti,am62a-jpeg-enc as secondary one in
  dt-binding
- Remove clock-names as only single clock in dt-binding
- Fix issue with default params setting
- Correct v4l2 error prints
- Simplify register write functions with single statement return values
- Remove unrequired error checks from get_queue()
- Drop explicit device_caps setting as it is already taken care by v4l2
  core
- Remove unrequired multiplanar checks and memset from s_fmt, g_fmt
  callback functions
- Fix try_fmt callback to not update the queues
- Remove unrequired contiguous format attribute from queue_init
- Use dynamic allocation for video_device and remove unrequired
  assignments in probe()
- Remove unrequired checks from queue_setup function
- Return queued buffers back if start_streaming fails
- Use ARRAY_SIZE in place of hard-coding
- Use huffman and quantization tables from reference header file

V2->V3:
- Add DONOTMERGE patches for dts and defconfig
- Update driver with below changes :
  - Correct license headers
  - Use more generic name core instead of jasper for base registers
  - Add Comment for forward declarations
  - Simplify quantization table calculations
  - Use v4l2_apply_frmsize_constraints for updating framesize and remove
    unrequired functions
  - Place TODO at top of file and in commit message too
  - Use dev_err_probe helper in probe function
  - Fix return value checking for failure scenarios in probe function
  - Use v4l2_err/info/warn helpers instead of dev_err/info/warn helpers
  - Fix unexpected indentation
  - Correct commit message
- Update dt-bindings with below changes :
  - Add vendor specific compatible 
  - Fix commit title and message
  - Update reg names
  - Update clocks to 1
  - Fix dts example with proper naming

V1->V2:
 - Send dt-bindings and driver together

Patch-Diff between the series :
V7->V8 Range diff :
https://gist.github.com/devarsht/3fd6c4e8031ab114248f93d01c8dfc74

V6->V7 Range diff :
https://gist.github.com/devarsht/1db185b1e187eaf397e9e4c37066777e

V5->V6 Range diff :
https://gist.github.com/devarsht/c89180ac2b0d2814614f2b59d0705c19

V4->V5 Range diff :
https://gist.github.com/devarsht/298790af819f299a0a05fec89371097b

V3->V4 Range diff :
https://gist.github.com/devarsht/22a744d999080de6e813bcfb5a596272

Previous patch series:
V7: https://lore.kernel.org/all/20240510082603.1263256-1-devarsht@ti.com/
V6: https://lore.kernel.org/all/20240228141140.3530612-1-devarsht@ti.com/
V5: https://lore.kernel.org/all/20240215134641.3381478-1-devarsht@ti.com/
V4: https://lore.kernel.org/all/20240205114239.924697-1-devarsht@ti.com/
V3: https://lore.kernel.org/all/20230816152210.4080779-1-devarsht@ti.com/
V2: https://lore.kernel.org/all/20230727112546.2201995-1-devarsht@ti.com/

Daniel Latypov (1):
  lib: add basic KUnit test for lib/math

Devarsh Thakkar (9):
  media: dt-bindings: Add Imagination E5010 JPEG Encoder
  media: imagination: Add E5010 JPEG Encoder driver
  media: v4l2-jpeg: Export reference quantization and huffman tables
  media: imagination: Use exported tables from v4l2-jpeg core
  media: verisilcon : Use exported tables from v4l2-jpeg for hantro
    codec
  math.h: Add macros for rounding to closest value
  lib: math_kunit: Add tests for new macros related to rounding to
    nearest value
  media: imagination: Round to closest multiple for cropping region
  gpu: ipu-v3: Use generic macro for rounding closest to specified value

 .../bindings/media/img,e5010-jpeg-enc.yaml    |   75 +
 MAINTAINERS                                   |    7 +
 drivers/gpu/ipu-v3/ipu-image-convert.c        |    4 +-
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/imagination/Kconfig    |   12 +
 drivers/media/platform/imagination/Makefile   |    3 +
 .../platform/imagination/e5010-core-regs.h    |  585 ++++++
 .../platform/imagination/e5010-jpeg-enc-hw.c  |  267 +++
 .../platform/imagination/e5010-jpeg-enc-hw.h  |   42 +
 .../platform/imagination/e5010-jpeg-enc.c     | 1646 +++++++++++++++++
 .../platform/imagination/e5010-jpeg-enc.h     |  168 ++
 .../platform/imagination/e5010-mmu-regs.h     |  311 ++++
 .../media/platform/verisilicon/hantro_jpeg.c  |  128 +-
 drivers/media/v4l2-core/v4l2-jpeg.c           |  162 +-
 include/linux/math.h                          |   65 +
 include/media/v4l2-jpeg.h                     |   11 +
 lib/math/Kconfig                              |   11 +
 lib/math/Makefile                             |    1 +
 lib/math/math_kunit.c                         |  326 ++++
 20 files changed, 3708 insertions(+), 118 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml
 create mode 100644 drivers/media/platform/imagination/Kconfig
 create mode 100644 drivers/media/platform/imagination/Makefile
 create mode 100644 drivers/media/platform/imagination/e5010-core-regs.h
 create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc-hw.c
 create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc-hw.h
 create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc.c
 create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc.h
 create mode 100644 drivers/media/platform/imagination/e5010-mmu-regs.h
 create mode 100644 lib/math/math_kunit.c

Comments

Andy Shevchenko May 17, 2024, 8:14 p.m. UTC | #1
On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote:
> From: Daniel Latypov <dlatypov@google.com>
> 
> Add basic test coverage for files that don't require any config options:
> * part of math.h (what seem to be the most commonly used macros)
> * gcd.c
> * lcm.c
> * int_sqrt.c
> * reciprocal_div.c
> (Ignored int_pow.c since it's a simple textbook algorithm.)
> 
> These tests aren't particularly interesting, but they
> * provide short and simple examples of parameterized tests
> * provide a place to add tests for any new files in this dir
> * are written so adding new test cases to cover edge cases should be
>   easy
>   * looking at code coverage, we hit all the branches in the .c files

...

> [devarsht: Rebase to 6.9 and change license to GPL]

I'm not sure that you may change license. It needs the author's confirmation.

> ---
> Changes since v6:
> * Rebase to linux-next, change license to GPL as suggested by checkpatch.

Note, checkpatch.pl is not false positives free. Be careful
with what it suggests.

> +#include <kunit/test.h>
> +#include <linux/gcd.h>

> +#include <linux/kernel.h>

Do you know why this header is included?

> +#include <linux/lcm.h>

+ math.h // obviously
+ module.h

> +#include <linux/reciprocal_div.h>

+ types.h

...

Other than above, LGTM.
Daniel Latypov May 17, 2024, 8:53 p.m. UTC | #2
On Fri, May 17, 2024 at 1:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> > [devarsht: Rebase to 6.9 and change license to GPL]
>
> I'm not sure that you may change license. It needs the author's confirmation.

Checking, this is referring to the MODULE_LICENSE, which used to be
MODULE_LICENSE("GPL v2");

and is now
MODULE_LICENSE("GPL");

If checkpatch is suggesting that now, then changing it sounds good to me.

>
> > ---
> > Changes since v6:
> > * Rebase to linux-next, change license to GPL as suggested by checkpatch.
>
> Note, checkpatch.pl is not false positives free. Be careful
> with what it suggests.
>
> > +#include <kunit/test.h>
> > +#include <linux/gcd.h>
>
> > +#include <linux/kernel.h>
>
> Do you know why this header is included?

I think I had put it in the original before a lot of the work you did
to split things out of kernel.h.
I haven't had time to look apply this patch series locally yet, but
I'd be pretty sure we can remove it without anything breaking.

Thanks,
Daniel
Andy Shevchenko May 20, 2024, 9:26 a.m. UTC | #3
On Fri, May 17, 2024 at 01:53:47PM -0700, Daniel Latypov wrote:
> On Fri, May 17, 2024 at 1:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > > [devarsht: Rebase to 6.9 and change license to GPL]
> >
> > I'm not sure that you may change license. It needs the author's confirmation.
> 
> Checking, this is referring to the MODULE_LICENSE, which used to be
> MODULE_LICENSE("GPL v2");
> 
> and is now
> MODULE_LICENSE("GPL");
> 
> If checkpatch is suggesting that now, then changing it sounds good to me.

In this case I agree on the change as it's purely syntax and not semantic.

> > > ---
> > > Changes since v6:
> > > * Rebase to linux-next, change license to GPL as suggested by checkpatch.
> >
> > Note, checkpatch.pl is not false positives free. Be careful
> > with what it suggests.
> >
> > > +#include <kunit/test.h>
> > > +#include <linux/gcd.h>
> >
> > > +#include <linux/kernel.h>
> >
> > Do you know why this header is included?
> 
> I think I had put it in the original before a lot of the work you did
> to split things out of kernel.h.
> I haven't had time to look apply this patch series locally yet, but
> I'd be pretty sure we can remove it without anything breaking.

Briefly looking at the code I even not sure it was needed before, but maybe
I missed something, in any case, please remove / replace it.
Devarsh Thakkar May 20, 2024, 11:41 a.m. UTC | #4
Hi Andy, Daniel,

On 18/05/24 01:44, Andy Shevchenko wrote:
> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote:
[..]
> 
>> [devarsht: Rebase to 6.9 and change license to GPL]
> 
> I'm not sure that you may change license. It needs the author's confirmation.
> 

As per latest licensing doc [1], It quotes that GPL  is same as GPL v2 and GPL
v2 exists only for historical reasons as shared below :

“GPL v2 Same as “GPL”. It exists for historic reasons."

So I think it should be fine and more apt to use GPL. This is also highlighted
in below commit:
bf7fbeeae6db644ef5995085de2bc5c6121f8c8d module: Cure the MODULE_LICENSE "GPL"
vs. "GPL v2" bogosity

>> ---
>> Changes since v6:
>> * Rebase to linux-next, change license to GPL as suggested by checkpatch.
> 
> Note, checkpatch.pl is not false positives free. Be careful
> with what it suggests.
> 
>> +#include <kunit/test.h>
>> +#include <linux/gcd.h>
> 
>> +#include <linux/kernel.h>
> 
> Do you know why this header is included?
> 

It includes all the other required headers (including those you mentioned
below), and header list is probably copied from other files in same directory.
But it does suffice the requirements as I have verified the compilation.

>> +#include <linux/lcm.h>
> 
> + math.h // obviously
> + module.h
> 
>> +#include <linux/reciprocal_div.h>
> 
> + types.h
> 

All the above headers are already included as part of kernel.h

Kindly let me know if any queries.

[1]: https://docs.kernel.org/process/license-rules.html

Regards
Devarsh
Andy Shevchenko May 20, 2024, 12:22 p.m. UTC | #5
On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote:
> On 18/05/24 01:44, Andy Shevchenko wrote:
> > On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote:

[..]

> >> +#include <kunit/test.h>
> >> +#include <linux/gcd.h>
> > 
> >> +#include <linux/kernel.h>
> > 
> > Do you know why this header is included?
> 
> It includes all the other required headers (including those you mentioned
> below), and header list is probably copied from other files in same directory.
> But it does suffice the requirements as I have verified the compilation.

Yes, and one should follow IWYU principle and not cargo cult or whatever
arbitrary lists.

> >> +#include <linux/lcm.h>
> > 
> > + math.h // obviously
> > + module.h
> > 
> >> +#include <linux/reciprocal_div.h>
> > 
> > + types.h
> 
> All the above headers are already included as part of kernel.h

Yes, that's why you should not use "proxy" headers.
Have you read the top comment in the kernel.h?
Devarsh Thakkar May 20, 2024, 2:21 p.m. UTC | #6
On 20/05/24 17:52, Andy Shevchenko wrote:
> On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote:
>> On 18/05/24 01:44, Andy Shevchenko wrote:
>>> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote:
> 
> [..]
> 

[..]
> Yes, and one should follow IWYU principle and not cargo cult or whatever
> arbitrary lists.
> 

Agreed.

>>>> +#include <linux/lcm.h>
>>>
>>> + math.h // obviously
>>> + module.h
>>>
>>>> +#include <linux/reciprocal_div.h>
>>>
>>> + types.h
>>
>> All the above headers are already included as part of kernel.h
> 
> Yes, that's why you should not use "proxy" headers.
> Have you read the top comment in the kernel.h?
> 

Yes, it says it is not recommended to include this inside another header file.
Although here we are adding it inside c file, but I can still try avoid it and
include only the required headers instead of kernel.h as you recommended.

Regards
Devarsh
Andy Shevchenko May 20, 2024, 2:35 p.m. UTC | #7
On Mon, May 20, 2024 at 07:51:24PM +0530, Devarsh Thakkar wrote:
> On 20/05/24 17:52, Andy Shevchenko wrote:
> > On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote:
> >> On 18/05/24 01:44, Andy Shevchenko wrote:
> >>> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote:

[..]

> > Yes, and one should follow IWYU principle and not cargo cult or whatever
> > arbitrary lists.
> 
> Agreed.

> >>>> +#include <linux/lcm.h>
> >>>
> >>> + math.h // obviously
> >>> + module.h
> >>>
> >>>> +#include <linux/reciprocal_div.h>
> >>>
> >>> + types.h
> >>
> >> All the above headers are already included as part of kernel.h
> > 
> > Yes, that's why you should not use "proxy" headers.
> > Have you read the top comment in the kernel.h?
> 
> Yes, it says it is not recommended to include this inside another header file.
> Although here we are adding it inside c file, but I can still try avoid it and
> include only the required headers instead of kernel.h as you recommended.

Right, but the first sentence there is
"This header has combined a lot of unrelated to each other stuff."

Can you explain how you use in your code all that unrelated stuff?
For example, how do you use *trace_*() calls? Or maybe might_*() calls?
or anything else that is directly provided by kernel.h?

Besides IWYU principle above, it's good to have a justification for each
inclusion the C file has. I believe there is no a such in _this_ case.