[PATCH v12 0/7] Wave5 codec driver

[PATCH v12 0/7] Wave5 codec driver

[Sebastian Fricke]

The Wave5 codec driver is a stateful encoder/decoder.
It is found on the J721S2 SoC, JH7100 SoC, ssd202d SoC. Etc.
But current test report is based on J721S2 SoC and pre-silicon FPGA.

The driver currently supports V4L2_PIX_FMT_HEVC, V4L2_PIX_FMT_H264
for both encoding and decoding.

This driver has so far been tested on J721S2 EVM board and pre-silicon
FPGA.

Testing on J721S2 EVM board shows it working fine both decoder and
encoder.
The driver is successfully working with gstreamer v4l2 good-plugin
without any modification.

We have based the tree on top of the latest
https://git.linuxtv.org/media_tree.git repository.

M2M framework modification:
===========================

(Patch 1 & 2)
Added a set of changes to enable ignoring the streaming state of one or more
queues while checking whether a new job can be inserted into the ready queue.
The use-case we encountered for this is a stateful decoder/encoder chip with a
tight connection to a firmware. On that firmware the bitstream is first
analyzed and the firmware produces output indicating the requirements for the
output of the decode (e.g. the buffers of the CAPTURE queue). We want to be
able to perform this action within the M2M job workflow in order to rely on the
concurrency safety provided by the M2M jobs.

v4l2-compliance results:
========================

v4l2-compliance 1.25.0-5092, 64 bits, 64-bit time_t

Total for wave5-dec device /dev/video0: 45, Succeeded: 45, Failed: 0, Warnings: 0

Total for wave5-enc device /dev/video1: 45, Succeeded: 45, Failed: 0, Warnings: 0

Fluster test results:
=====================

Running test suite JCT-VC-HEVC_V1 with decoder GStreamer-H.265-V4L2-Gst1.0
Using 1 parallel job(s)
Ran 133/147 tests successfully               in 95.898 secs

(1 test fails because of a missing frame and slight corruption, 2 tests fail
because of sizes which are incompatible with the IP, 11 tests fail because of
unsupported 10 bit format)

Running test suite JVT-AVC_V1 with decoder GStreamer-H.264-V4L2-Gst1.0
Using 1 parallel job(s)
Ran 78/135 tests successfully               in 57.198 secs

(57 fail because the hardware is unable to decode
 MBAFF / FMO / Field / Extended profile streams.)

Encoder testing:
================

Among other tests the driver is able to produce valid output with the following test:
`gst-launch-1.0 videotestsrc num-buffers=300 ! v4l2h264enc ! h264parse ! qtmux ! filesink location=test.mp4`

Changes since v11:
==================

* Major rework of the decoder
  - Fix concurrency issues by moving the commands that invoke actions on the
    firmware into the device_run function from M2M, effectively causing M2M to
    take care of the concurrency via its job queue.
    - In order to do that device_run needs to be able to run before both queues
      are ready, as a sequence needs to be initialized on the firmware to get
      the required buffer count & communicate the result back to userspace,
      thus we added a routine to allow jobs to be queued in M2M with only one
      of two queues being started. (See: "Add ignore_streaming flag to M2M"
      series)
  - Add support for more output formats (YUV422P, YUV422M, NV16, NV16M, NV61,
    NV61M)
  - Add proper state switch function to verify state switches 
  - Simplify the queue_setup function and move the decoder opening to STREAMON
    as request in the review of V11
  - Enable handling dynamic resolution change and seeking
  - Remove thumbnail mode

* Similar reworks on the encoder
  - Move encoding into device_run and encoder opening + sequence intialization
    to STREAMON, this change simplifies the encoder as it previously was able
    to run multiple encode jobs simultaneously

* Remove unused configurations and support for untested hardware
  - Remove the ability to configure the endianess of memory writes as only a
    single hardware can be tested so far
  - Remove the ability to configure the bitstream_mode, as the driver is
    currently hardcoded for the INTERRUPT mode
  - Remove support for all CODECS, that were not tested (everything besides
    H264/HEVC encoder + decoder)
  - The encoder currently contains a lot of configurable parameter, which are
    hard-coded in the `wave5_set_enc_openparam` function, remove all parameters
    which aren't currently specified in that configuration
  - Remove unused rotation and mirroring options from the decoder

* Add FLUSH_FIRMWARE firmware command
* Refactoring
  * Add wrappers for frequently used routines or to make the code more
    descriptive
    - Wrapper for firmware command calling `send_firmware_command`
    - Wrappers to intialize the sequence and to set up the framebuffers in the
      decoder and encoder
    - Using more general kernel functions and macros in various places
  * Add Macros for constant values and bit shifts
  * Fix typos and improve comments

* Fix bug with M2M instance stored in the driver instance, multiple
  simultaneous instances would overwrite the M2M handler of each other. Use a
  M2M handler per device instead to avoid overwriting.
* Adjust TRACE_PATH in the coda directory as highlighted in the review of V11
* Applied requested changes from review to the devicetree bindings file
  (the bindings check didn't return any warnings or errors)

Changes since v10:
==================

* Remove structure member from the encoder and decoder output info
  structs, that have assigned values from the registers but aren't used
  in the driver, add comments to describe the register values in the
  register definitions
* Fix issue with decoding videos with a dimension where the height is
  not a multiple of 16 (270, 360, 540, 1024 etc.)
* Fix incorrect variable format identifiers in printks
* Use debug logs in loops to avoid flooding the message log
* Use the swap() function instead of manual swapping of two values
* Add extended controls for the encoder
* Fix control flow issue while handling bitstream buffers, where an
  error while writing the source buffer into the hardware ring buffer
  would result in skipping the problematic buffer, which in turn causes
  a reordering of source buffers
* Use the rectangle format as described by the hardware, the hardware
  uses for rectangles like the display rectangle 4 offsets (top, bottom,
  left, right), which depict the offset from the respective edge. Use
  this format instead of implicitly converting the bottom and right
  attributes to width and height attributes.
* Return an error upon reading the sequence header while STREAMON
* Squash the VDI and the VPUAPI layer commits as they had circular
  dependencies

changes since v9:
=================

* Move from staging to the media directory
  * Move coda driver to sub-directory

* Fixes:
  * Use platform_get_irq instead of platform_get_resource to fetch the IRQ

* General cleanups:
  * Add missing error messages to error conditions
  * Improve messages/variable names/comments, align parameter names across the driver
  * Use macros instead of magic numbers in multiple occassions
  * Reduce code duplication in multiple places
  * Fix whitespace, newline and tab alignment issues
  * Remove unused struct fields & commented out code
  * Convert signed integers to unsigned if signed is not necessary
  * Convert int/unsigned int to s32/u32, when the variable is assigned to the
    return of a register read or provided as a parameter for a register write
    (and vice versa)
  * Fix incorrect bitwise operators where logical operators are appropriate
  * Multiple smaller changes

* Generalization:
  * Add new helper file providing generalized routines for vpu-dec & vpu-enc
  * Generalize luma & chroma table size calculation and stride calculation

* Resource cleanup and error handling:
  * Add error handling to all calls with ignored return codes
  * Handle DMA resource cleanup properly
  * Fix insufficient instance cleanup while opening dec/enc

Changes since v8:
=================

* add 'wave5' to DEV_NAME
* update to support Multi-stream
* update to support loop test/dynamic resolution change
* remove unnecessary memset, g_volatile, old version option

Changes since v7:
=================

* update v4l2-compliance test report
* fix build error on linux-kernel 5.18.0-rc4

Changes since v6:
=================

* update TODO file
* get sram info from device tree

Changes since v5:
=================

* support NV12/NV21 pixelformat for encoder and decoder
* handle adnormal exit and EOS

Changes since v4:
=================

* refactor functions in wave5-hw and fix bug reported by Daniel Palmer
* rename functions and variables to better names
* change variable types such as replacing s32 with u32 and int with bool
* as appropriate

Changes since v3:
=================

* Fixing all issues commented by Dan Carpenter
* Change file names to have wave5- prefix
* In wave5_vpu_probe, enable the clocks before reading registers, as
* commented from Daniel Palmer
* Add more to the TODO list,

Changes since v2:
=================

Main fixes includes:
* change the yaml and dirver code to support up to 4 clks (instead of
* one)
* fix Kconfig format
* remove unneeded cast,
* change var types
* change var names, func names
* checkpatch fixes

Changes since v1:
=================

Fix changes due to comments from Ezequiel and Dan Carpenter. Main fixes
inclueds:
* move all files to one dir 'wave5'
* replace private error codes with standard error codes
* fix extra spaces
* various checkpatch fixes
* replace private 'DPRINTK' macro with standard 'dev_err/dbg ..'
* fix error handling
* add more possible fixes to the TODO file

To: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Hans Verkuil <hverkuil@xs4all.nl>
To: Philipp Zabel <p.zabel@pengutronix.de>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: NXP Linux Team <linux-imx@nxp.com>
To: Jackson Lee <jackson.lee@chipsnmedia.com>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc: Robert Beckett <bob.beckett@collabora.com>
Cc: Nas Chung <nas.chung@chipsnmedia.com>
CC: kernel@collabora.com
Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>

---
Nas Chung (2):
      media: chips-media: wave5: Add vpuapi layer
      media: chips-media: wave5: Add the v4l2 layer

Robert Beckett (2):
      dt-bindings: media: wave5: add yaml devicetree bindings
      media: chips-media: wave5: Add wave5 driver to maintainers file

Sebastian Fricke (3):
      media: v4l2: Add ignore_streaming flag
      media: v4l2: Allow M2M job queuing w/o streaming CAP queue
      media: platform: chips-media: Move Coda to separate folder

 .../devicetree/bindings/media/cnm,wave5.yaml       |   66 +
 MAINTAINERS                                        |   10 +-
 drivers/media/platform/chips-media/Kconfig         |   18 +-
 drivers/media/platform/chips-media/Makefile        |    6 +-
 drivers/media/platform/chips-media/coda/Kconfig    |   18 +
 drivers/media/platform/chips-media/coda/Makefile   |    6 +
 .../platform/chips-media/{ => coda}/coda-bit.c     |    0
 .../platform/chips-media/{ => coda}/coda-common.c  |    0
 .../platform/chips-media/{ => coda}/coda-gdi.c     |    0
 .../platform/chips-media/{ => coda}/coda-h264.c    |    0
 .../platform/chips-media/{ => coda}/coda-jpeg.c    |    0
 .../platform/chips-media/{ => coda}/coda-mpeg2.c   |    0
 .../platform/chips-media/{ => coda}/coda-mpeg4.c   |    0
 .../media/platform/chips-media/{ => coda}/coda.h   |    0
 .../platform/chips-media/{ => coda}/coda_regs.h    |    0
 .../platform/chips-media/{ => coda}/imx-vdoa.c     |    0
 .../platform/chips-media/{ => coda}/imx-vdoa.h     |    0
 .../media/platform/chips-media/{ => coda}/trace.h  |    2 +-
 drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
 drivers/media/platform/chips-media/wave5/Makefile  |   10 +
 .../platform/chips-media/wave5/wave5-helper.c      |  196 ++
 .../platform/chips-media/wave5/wave5-helper.h      |   30 +
 .../media/platform/chips-media/wave5/wave5-hw.c    | 2553 ++++++++++++++++++++
 .../platform/chips-media/wave5/wave5-regdefine.h   |  732 ++++++
 .../media/platform/chips-media/wave5/wave5-vdi.c   |  208 ++
 .../media/platform/chips-media/wave5/wave5-vdi.h   |   35 +
 .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1965 +++++++++++++++
 .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1825 ++++++++++++++
 .../media/platform/chips-media/wave5/wave5-vpu.c   |  331 +++
 .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
 .../platform/chips-media/wave5/wave5-vpuapi.c      |  958 ++++++++
 .../platform/chips-media/wave5/wave5-vpuapi.h      |  874 +++++++
 .../platform/chips-media/wave5/wave5-vpuconfig.h   |   77 +
 .../platform/chips-media/wave5/wave5-vpuerror.h    |  292 +++
 drivers/media/platform/chips-media/wave5/wave5.h   |  116 +
 drivers/media/v4l2-core/v4l2-mem2mem.c             |    9 +-
 include/media/v4l2-mem2mem.h                       |   17 +
 37 files changed, 10424 insertions(+), 25 deletions(-)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230915-wave5_v12_on_media_master-ac30ff2c4c00

Best regards,
-- 
Sebastian Fricke <sebastian.fricke@collabora.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Sebastian Fricke]

Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
must be streaming in order to allow queuing jobs to the ready queue.
Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
allow adding new jobs. This behavior limits the usability of M2M for
some drivers, as these have to be able, to perform analysis of the
sequence to ensure, that userspace prepares the CAPTURE queue correctly.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index d6c8eb2b5201..97a48e61e358 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
  * @rdy_spinlock: spin lock to protect the struct usage
  * @num_rdy:	number of buffers ready to be processed
  * @buffered:	is the queue buffered?
+ * @ignore_streaming: Dictates whether the queue must be streaming for a job to
+ *		      be queued.
+ *		      This is useful, for example, when the driver requires to
+ *		      initialize the sequence with a firmware, where only a
+ *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
+ *		      queue is required to perform the anlysis of the bitstream
+ *		      header.
+ *		      This means the driver is responsible for implementing the
+ *		      job_ready callback correctly to make sure that requirements
+ *		      for actual decoding are met.
  *
  * Queue for buffers ready to be processed as soon as this
  * instance receives access to the device.
@@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
 	spinlock_t		rdy_spinlock;
 	u8			num_rdy;
 	bool			buffered;
+	bool			ignore_streaming;
 };
 
 /**
@@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
 	m2m_ctx->cap_q_ctx.buffered = buffered;
 }
 
+static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
+						     bool ignore_streaming)
+{
+	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
+}
+
 /**
  * v4l2_m2m_ctx_release() - release m2m context
  *

-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Hans Verkuil]

On 15/09/2023 23:11, Sebastian Fricke wrote:
> Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
> must be streaming in order to allow queuing jobs to the ready queue.
> Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
> allow adding new jobs. This behavior limits the usability of M2M for
> some drivers, as these have to be able, to perform analysis of the

able, to -> able to

> sequence to ensure, that userspace prepares the CAPTURE queue correctly.

ensure, that -> ensure that

> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index d6c8eb2b5201..97a48e61e358 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
>   * @rdy_spinlock: spin lock to protect the struct usage
>   * @num_rdy:	number of buffers ready to be processed
>   * @buffered:	is the queue buffered?
> + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
> + *		      be queued.
> + *		      This is useful, for example, when the driver requires to
> + *		      initialize the sequence with a firmware, where only a
> + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
> + *		      queue is required to perform the anlysis of the bitstream
> + *		      header.
> + *		      This means the driver is responsible for implementing the
> + *		      job_ready callback correctly to make sure that requirements
> + *		      for actual decoding are met.

This is a bad description and field name.

Basically what this field does is that, if true, the streaming state of the
capture queue is ignored. So just call it that: ignore_cap_streaming.

And explain that, if true, job_ready() will be called even if the capture
queue is not streaming, and that that can be used to allow hardware to
analyze the bitstream header that arrives on the OUTPUT queue.

Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
for the output queue, this is really a configuration for the m2m context as
a whole.

>   *
>   * Queue for buffers ready to be processed as soon as this
>   * instance receives access to the device.
> @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
>  	spinlock_t		rdy_spinlock;
>  	u8			num_rdy;
>  	bool			buffered;
> +	bool			ignore_streaming;
>  };
>  
>  /**
> @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
>  	m2m_ctx->cap_q_ctx.buffered = buffered;
>  }
>  
> +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
> +						     bool ignore_streaming)
> +{
> +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
> +}
> +

I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
document that drivers can set this.

Regards,

	Hans

>  /**
>   * v4l2_m2m_ctx_release() - release m2m context
>   *
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Nicolas Dufresne]

cc Tomasz Figa

Le mercredi 20 septembre 2023 à 14:59 +0200, Hans Verkuil a écrit :
> On 15/09/2023 23:11, Sebastian Fricke wrote:
> > Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
> > must be streaming in order to allow queuing jobs to the ready queue.
> > Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
> > allow adding new jobs. This behavior limits the usability of M2M for
> > some drivers, as these have to be able, to perform analysis of the
> 
> able, to -> able to
> 
> > sequence to ensure, that userspace prepares the CAPTURE queue correctly.
> 
> ensure, that -> ensure that
> 
> > 
> > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > index d6c8eb2b5201..97a48e61e358 100644
> > --- a/include/media/v4l2-mem2mem.h
> > +++ b/include/media/v4l2-mem2mem.h
> > @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
> >   * @rdy_spinlock: spin lock to protect the struct usage
> >   * @num_rdy:	number of buffers ready to be processed
> >   * @buffered:	is the queue buffered?
> > + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
> > + *		      be queued.
> > + *		      This is useful, for example, when the driver requires to
> > + *		      initialize the sequence with a firmware, where only a
> > + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
> > + *		      queue is required to perform the anlysis of the bitstream
> > + *		      header.
> > + *		      This means the driver is responsible for implementing the
> > + *		      job_ready callback correctly to make sure that requirements
> > + *		      for actual decoding are met.
> 
> This is a bad description and field name.

I wonder what's your opinion about the buffered one then :-D

> 
> Basically what this field does is that, if true, the streaming state of the
> capture queue is ignored. So just call it that: ignore_cap_streaming.
> 
> And explain that, if true, job_ready() will be called even if the capture
> queue is not streaming, and that that can be used to allow hardware to
> analyze the bitstream header that arrives on the OUTPUT queue.

Ack.

> 
> Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
> for the output queue, this is really a configuration for the m2m context as
> a whole.

Unless we come up with a completely new type of M2M that can behave like a gap
filler (like a video rate m2m), it indeed makes no sense for output. I'm just
illustrating that this is true "now" but someone can come up with valid
expectation. So I agree with you, we can move it up in the hierarchy.

Recently over IRC and other threads, Tomasz raised a concern that CODECs where
introducing too much complexity into M2M. And I believe buffered (which is
barely documented) and this mechanism was being pointed.

My take on that is that adding boolean configuration is what introduce
complexity, and we can fix it by doing less in the m2m. After this discussion, I
came with the idea that we should remove buffered and ignore_streaming. For
drivers that don't implement job_ready, this logic would be moved inside the
default implementation. We can then add a helper to check the common conditions.

The alternative suggested by Tomasz, was to layer two ops. We'd have a
device_ready() ops and its default implementation would include the check we
have and would call job_ready(). Personally, I'd rather remove then add, but I
understadt the reasoning and would be fine committing to that instead.

I'd like your feedback on this proposal. If this is something we want, I'll do
this prior to V13, otherwise we will address your comments and fix the added
mechanism. I think though that we agree that for decoders, this is nice addition
to not have to trigger work manually from vb2 ops.

regards,
Nicolas

> 
> >   *
> >   * Queue for buffers ready to be processed as soon as this
> >   * instance receives access to the device.
> > @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
> >  	spinlock_t		rdy_spinlock;
> >  	u8			num_rdy;
> >  	bool			buffered;
> > +	bool			ignore_streaming;
> >  };
> >  
> >  /**
> > @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
> >  	m2m_ctx->cap_q_ctx.buffered = buffered;
> >  }
> >  
> > +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
> > +						     bool ignore_streaming)
> > +{
> > +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
> > +}
> > +
> 
> I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
> document that drivers can set this.
> 
> Regards,
> 
> 	Hans
> 
> >  /**
> >   * v4l2_m2m_ctx_release() - release m2m context
> >   *
> > 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Hans Verkuil]

On 20/09/2023 16:08, Nicolas Dufresne wrote:
> cc Tomasz Figa
> 
> Le mercredi 20 septembre 2023 à 14:59 +0200, Hans Verkuil a écrit :
>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>> Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
>>> must be streaming in order to allow queuing jobs to the ready queue.
>>> Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
>>> allow adding new jobs. This behavior limits the usability of M2M for
>>> some drivers, as these have to be able, to perform analysis of the
>>
>> able, to -> able to
>>
>>> sequence to ensure, that userspace prepares the CAPTURE queue correctly.
>>
>> ensure, that -> ensure that
>>
>>>
>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> ---
>>>  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
>>> index d6c8eb2b5201..97a48e61e358 100644
>>> --- a/include/media/v4l2-mem2mem.h
>>> +++ b/include/media/v4l2-mem2mem.h
>>> @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
>>>   * @rdy_spinlock: spin lock to protect the struct usage
>>>   * @num_rdy:	number of buffers ready to be processed
>>>   * @buffered:	is the queue buffered?
>>> + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
>>> + *		      be queued.
>>> + *		      This is useful, for example, when the driver requires to
>>> + *		      initialize the sequence with a firmware, where only a
>>> + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
>>> + *		      queue is required to perform the anlysis of the bitstream
>>> + *		      header.
>>> + *		      This means the driver is responsible for implementing the
>>> + *		      job_ready callback correctly to make sure that requirements
>>> + *		      for actual decoding are met.
>>
>> This is a bad description and field name.
> 
> I wonder what's your opinion about the buffered one then :-D

Even worse :-)

I still don't really understand what that does. Patches welcome.

> 
>>
>> Basically what this field does is that, if true, the streaming state of the
>> capture queue is ignored. So just call it that: ignore_cap_streaming.
>>
>> And explain that, if true, job_ready() will be called even if the capture
>> queue is not streaming, and that that can be used to allow hardware to
>> analyze the bitstream header that arrives on the OUTPUT queue.
> 
> Ack.
> 
>>
>> Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
>> for the output queue, this is really a configuration for the m2m context as
>> a whole.
> 
> Unless we come up with a completely new type of M2M that can behave like a gap
> filler (like a video rate m2m), it indeed makes no sense for output. I'm just
> illustrating that this is true "now" but someone can come up with valid
> expectation. So I agree with you, we can move it up in the hierarchy.
> 
> Recently over IRC and other threads, Tomasz raised a concern that CODECs where
> introducing too much complexity into M2M. And I believe buffered (which is
> barely documented) and this mechanism was being pointed.
> 
> My take on that is that adding boolean configuration is what introduce
> complexity, and we can fix it by doing less in the m2m. After this discussion, I
> came with the idea that we should remove buffered and ignore_streaming. For
> drivers that don't implement job_ready, this logic would be moved inside the
> default implementation. We can then add a helper to check the common conditions.
> 
> The alternative suggested by Tomasz, was to layer two ops. We'd have a
> device_ready() ops and its default implementation would include the check we
> have and would call job_ready(). Personally, I'd rather remove then add, but I
> understadt the reasoning and would be fine committing to that instead.
> 
> I'd like your feedback on this proposal. If this is something we want, I'll do
> this prior to V13, otherwise we will address your comments and fix the added
> mechanism. I think though that we agree that for decoders, this is nice addition
> to not have to trigger work manually from vb2 ops.

It comes down to a matter of taste, I guess. I personally think that using bools
to tweak the behavior of a framework does not necessarily increase complexity,
provided it is clearly documented what it does and why it is needed.

I think an ignore_cap_streaming bool is pretty straightforward and has minimal
impact in the code. As long as there are good comments.

The 'buffered' flag is were this clearly failed completely, since I couldn't figure
out what it is supposed to do. But that is not because it makes the code more
complex, it is just because of shoddy documentation and naming.

Quite often implementing tweaks like that are quite easy in a framework, since
you have all the information readily available. In a driver it can quickly become
messy.

For codec support there are a number of issues that increase complexity:
implementing support for the LAST flag and events, and supporting buffers
that can be held. Especially since driver implementations tend to vary.

I've been experimenting with some cleanups and changes in v4l2-mem2mem.c
(https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=enc-dec-cmd), mainly
surrounding the handling of the LAST flag. Note: this is failing the compliance
tests, I haven't had the time to pursue this further.

I'm not sure whether the best approach is to move things out of the m2m framework,
or move things into the m2m framework, or add a more codec-specific layer on top
of the m2m framework, or a combination of all of these.

It is something that needs experimentation, just see what works.

But for this specific flag: I think it is fine to put that in the m2m framework,
just document and name it well.

Regards,

	Hans

> 
> regards,
> Nicolas
> 
>>
>>>   *
>>>   * Queue for buffers ready to be processed as soon as this
>>>   * instance receives access to the device.
>>> @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
>>>  	spinlock_t		rdy_spinlock;
>>>  	u8			num_rdy;
>>>  	bool			buffered;
>>> +	bool			ignore_streaming;
>>>  };
>>>  
>>>  /**
>>> @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
>>>  	m2m_ctx->cap_q_ctx.buffered = buffered;
>>>  }
>>>  
>>> +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
>>> +						     bool ignore_streaming)
>>> +{
>>> +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
>>> +}
>>> +
>>
>> I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
>> document that drivers can set this.
>>
>> Regards,
>>
>> 	Hans
>>
>>>  /**
>>>   * v4l2_m2m_ctx_release() - release m2m context
>>>   *
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Nicolas Dufresne]

Le mercredi 20 septembre 2023 à 16:49 +0200, Hans Verkuil a écrit :
> On 20/09/2023 16:08, Nicolas Dufresne wrote:
> > cc Tomasz Figa
> > 
> > Le mercredi 20 septembre 2023 à 14:59 +0200, Hans Verkuil a écrit :
> > > On 15/09/2023 23:11, Sebastian Fricke wrote:
> > > > Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
> > > > must be streaming in order to allow queuing jobs to the ready queue.
> > > > Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
> > > > allow adding new jobs. This behavior limits the usability of M2M for
> > > > some drivers, as these have to be able, to perform analysis of the
> > > 
> > > able, to -> able to
> > > 
> > > > sequence to ensure, that userspace prepares the CAPTURE queue correctly.
> > > 
> > > ensure, that -> ensure that
> > > 
> > > > 
> > > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > ---
> > > >  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
> > > >  1 file changed, 17 insertions(+)
> > > > 
> > > > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > > > index d6c8eb2b5201..97a48e61e358 100644
> > > > --- a/include/media/v4l2-mem2mem.h
> > > > +++ b/include/media/v4l2-mem2mem.h
> > > > @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
> > > >   * @rdy_spinlock: spin lock to protect the struct usage
> > > >   * @num_rdy:	number of buffers ready to be processed
> > > >   * @buffered:	is the queue buffered?
> > > > + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
> > > > + *		      be queued.
> > > > + *		      This is useful, for example, when the driver requires to
> > > > + *		      initialize the sequence with a firmware, where only a
> > > > + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
> > > > + *		      queue is required to perform the anlysis of the bitstream
> > > > + *		      header.
> > > > + *		      This means the driver is responsible for implementing the
> > > > + *		      job_ready callback correctly to make sure that requirements
> > > > + *		      for actual decoding are met.
> > > 
> > > This is a bad description and field name.
> > 
> > I wonder what's your opinion about the buffered one then :-D
> 
> Even worse :-)
> 
> I still don't really understand what that does. Patches welcome.
> 
> > 
> > > 
> > > Basically what this field does is that, if true, the streaming state of the
> > > capture queue is ignored. So just call it that: ignore_cap_streaming.
> > > 
> > > And explain that, if true, job_ready() will be called even if the capture
> > > queue is not streaming, and that that can be used to allow hardware to
> > > analyze the bitstream header that arrives on the OUTPUT queue.
> > 
> > Ack.
> > 
> > > 
> > > Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
> > > for the output queue, this is really a configuration for the m2m context as
> > > a whole.
> > 
> > Unless we come up with a completely new type of M2M that can behave like a gap
> > filler (like a video rate m2m), it indeed makes no sense for output. I'm just
> > illustrating that this is true "now" but someone can come up with valid
> > expectation. So I agree with you, we can move it up in the hierarchy.
> > 
> > Recently over IRC and other threads, Tomasz raised a concern that CODECs where
> > introducing too much complexity into M2M. And I believe buffered (which is
> > barely documented) and this mechanism was being pointed.
> > 
> > My take on that is that adding boolean configuration is what introduce
> > complexity, and we can fix it by doing less in the m2m. After this discussion, I
> > came with the idea that we should remove buffered and ignore_streaming. For
> > drivers that don't implement job_ready, this logic would be moved inside the
> > default implementation. We can then add a helper to check the common conditions.
> > 
> > The alternative suggested by Tomasz, was to layer two ops. We'd have a
> > device_ready() ops and its default implementation would include the check we
> > have and would call job_ready(). Personally, I'd rather remove then add, but I
> > understadt the reasoning and would be fine committing to that instead.
> > 
> > I'd like your feedback on this proposal. If this is something we want, I'll do
> > this prior to V13, otherwise we will address your comments and fix the added
> > mechanism. I think though that we agree that for decoders, this is nice addition
> > to not have to trigger work manually from vb2 ops.
> 
> It comes down to a matter of taste, I guess. I personally think that using bools
> to tweak the behavior of a framework does not necessarily increase complexity,
> provided it is clearly documented what it does and why it is needed.
> 
> I think an ignore_cap_streaming bool is pretty straightforward and has minimal
> impact in the code. As long as there are good comments.

So for wave5 we will opt for this and apply your suggested changes. And I may
come back later on the subject.

> 
> The 'buffered' flag is were this clearly failed completely, since I couldn't figure
> out what it is supposed to do. But that is not because it makes the code more
> complex, it is just because of shoddy documentation and naming.
> 
> Quite often implementing tweaks like that are quite easy in a framework, since
> you have all the information readily available. In a driver it can quickly become
> messy.

In this case, "buffered" is used to disable the checks for having at least one
buffer in the ready queues. In most cases, if you don't have at least 1 pending
capture and 1 pending output buffer, there is no point in calling device_run.

In reality, drivers will add use case specific checks in their job_ready()
implementation. For decoders, the cases I can think of are:

- On capture if you haven't parsed the stream header
- On capture if the driver removes them from ready queue as a way to track which
one are considered free and may be used at any time by the firmware
- On output queue, if you need device_run() to be called to complete the drain
the reorder queue

Yet, you want this check after stream headers are parsed, or whenever a new
bitstream decode operation is to be queued in the firmware. So this check gets
re-implemented, but dynamically, in all decoders.

Deinterlacers may needs this too with some algorithms (the one that introduce
delays at least). Its not clear to me why it was called buffered,
ignore_rdy_queue might have been an option, though I'm not fully confident. Note
that M2M can be confusing, since whenever you ask for last something, its always
relative to the ready queue, and may not make a lot of sense in the context it
is used.

> 
> For codec support there are a number of issues that increase complexity:
> implementing support for the LAST flag and events, and supporting buffers
> that can be held. Especially since driver implementations tend to vary.
> 
> I've been experimenting with some cleanups and changes in v4l2-mem2mem.c
> (https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=enc-dec-cmd), mainly
> surrounding the handling of the LAST flag. Note: this is failing the compliance
> tests, I haven't had the time to pursue this further.
> 
> I'm not sure whether the best approach is to move things out of the m2m framework,
> or move things into the m2m framework, or add a more codec-specific layer on top
> of the m2m framework, or a combination of all of these.
> 
> It is something that needs experimentation, just see what works.

I can see you have omitted mark_stopped() calles when refactoring, which makes
these patches change the behaviour. Could be related.

This is no longer strictly related to this patch, but I think cmd_stop()
implementation (even after your changes) are miss-fit for driver that speaks to
firmware. As the firmware is being made aware of the free buffers, you can't
just cherry-pick from the capture queue, you have to synchronise your state with
the firmware while draining. The helper should be split in two parts I suppose,
but cutting the line isn't easy.

Thread safe usage of the numerous boolean implicated in the draining state is
also difficult. There is no other option then introduce a mutex or spinlock (if
the state is needed in job_ready() implementation) to make this thread safe and
reliable.

> 
> But for this specific flag: I think it is fine to put that in the m2m framework,
> just document and name it well.

Ack.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > regards,
> > Nicolas
> > 
> > > 
> > > >   *
> > > >   * Queue for buffers ready to be processed as soon as this
> > > >   * instance receives access to the device.
> > > > @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
> > > >  	spinlock_t		rdy_spinlock;
> > > >  	u8			num_rdy;
> > > >  	bool			buffered;
> > > > +	bool			ignore_streaming;
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
> > > >  	m2m_ctx->cap_q_ctx.buffered = buffered;
> > > >  }
> > > >  
> > > > +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
> > > > +						     bool ignore_streaming)
> > > > +{
> > > > +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
> > > > +}
> > > > +
> > > 
> > > I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
> > > document that drivers can set this.
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > >  /**
> > > >   * v4l2_m2m_ctx_release() - release m2m context
> > > >   *
> > > > 
> > > 
> > 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Hans Verkuil]

On 21/09/2023 20:39, Nicolas Dufresne wrote:
> Le mercredi 20 septembre 2023 à 16:49 +0200, Hans Verkuil a écrit :
>> On 20/09/2023 16:08, Nicolas Dufresne wrote:
>>> cc Tomasz Figa
>>>
>>> Le mercredi 20 septembre 2023 à 14:59 +0200, Hans Verkuil a écrit :
>>>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>>>> Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
>>>>> must be streaming in order to allow queuing jobs to the ready queue.
>>>>> Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
>>>>> allow adding new jobs. This behavior limits the usability of M2M for
>>>>> some drivers, as these have to be able, to perform analysis of the
>>>>
>>>> able, to -> able to
>>>>
>>>>> sequence to ensure, that userspace prepares the CAPTURE queue correctly.
>>>>
>>>> ensure, that -> ensure that
>>>>
>>>>>
>>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> ---
>>>>>  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
>>>>>  1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
>>>>> index d6c8eb2b5201..97a48e61e358 100644
>>>>> --- a/include/media/v4l2-mem2mem.h
>>>>> +++ b/include/media/v4l2-mem2mem.h
>>>>> @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
>>>>>   * @rdy_spinlock: spin lock to protect the struct usage
>>>>>   * @num_rdy:	number of buffers ready to be processed
>>>>>   * @buffered:	is the queue buffered?
>>>>> + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
>>>>> + *		      be queued.
>>>>> + *		      This is useful, for example, when the driver requires to
>>>>> + *		      initialize the sequence with a firmware, where only a
>>>>> + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
>>>>> + *		      queue is required to perform the anlysis of the bitstream
>>>>> + *		      header.
>>>>> + *		      This means the driver is responsible for implementing the
>>>>> + *		      job_ready callback correctly to make sure that requirements
>>>>> + *		      for actual decoding are met.
>>>>
>>>> This is a bad description and field name.
>>>
>>> I wonder what's your opinion about the buffered one then :-D
>>
>> Even worse :-)
>>
>> I still don't really understand what that does. Patches welcome.
>>
>>>
>>>>
>>>> Basically what this field does is that, if true, the streaming state of the
>>>> capture queue is ignored. So just call it that: ignore_cap_streaming.
>>>>
>>>> And explain that, if true, job_ready() will be called even if the capture
>>>> queue is not streaming, and that that can be used to allow hardware to
>>>> analyze the bitstream header that arrives on the OUTPUT queue.
>>>
>>> Ack.
>>>
>>>>
>>>> Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
>>>> for the output queue, this is really a configuration for the m2m context as
>>>> a whole.
>>>
>>> Unless we come up with a completely new type of M2M that can behave like a gap
>>> filler (like a video rate m2m), it indeed makes no sense for output. I'm just
>>> illustrating that this is true "now" but someone can come up with valid
>>> expectation. So I agree with you, we can move it up in the hierarchy.
>>>
>>> Recently over IRC and other threads, Tomasz raised a concern that CODECs where
>>> introducing too much complexity into M2M. And I believe buffered (which is
>>> barely documented) and this mechanism was being pointed.
>>>
>>> My take on that is that adding boolean configuration is what introduce
>>> complexity, and we can fix it by doing less in the m2m. After this discussion, I
>>> came with the idea that we should remove buffered and ignore_streaming. For
>>> drivers that don't implement job_ready, this logic would be moved inside the
>>> default implementation. We can then add a helper to check the common conditions.
>>>
>>> The alternative suggested by Tomasz, was to layer two ops. We'd have a
>>> device_ready() ops and its default implementation would include the check we
>>> have and would call job_ready(). Personally, I'd rather remove then add, but I
>>> understadt the reasoning and would be fine committing to that instead.
>>>
>>> I'd like your feedback on this proposal. If this is something we want, I'll do
>>> this prior to V13, otherwise we will address your comments and fix the added
>>> mechanism. I think though that we agree that for decoders, this is nice addition
>>> to not have to trigger work manually from vb2 ops.
>>
>> It comes down to a matter of taste, I guess. I personally think that using bools
>> to tweak the behavior of a framework does not necessarily increase complexity,
>> provided it is clearly documented what it does and why it is needed.
>>
>> I think an ignore_cap_streaming bool is pretty straightforward and has minimal
>> impact in the code. As long as there are good comments.
> 
> So for wave5 we will opt for this and apply your suggested changes. And I may
> come back later on the subject.
> 
>>
>> The 'buffered' flag is were this clearly failed completely, since I couldn't figure
>> out what it is supposed to do. But that is not because it makes the code more
>> complex, it is just because of shoddy documentation and naming.
>>
>> Quite often implementing tweaks like that are quite easy in a framework, since
>> you have all the information readily available. In a driver it can quickly become
>> messy.
> 
> In this case, "buffered" is used to disable the checks for having at least one
> buffer in the ready queues. In most cases, if you don't have at least 1 pending
> capture and 1 pending output buffer, there is no point in calling device_run.

So it is really similar to ignore_cap_streaming: that relaxes the streaming test,
and 'buffered' relaxes the 'must have at least one capture and output buffer ready'
test.

So this should be renamed to: allow_empty_queues

Although I would prefer to split this into two bools: allow_empty_capture_queue and
allow_empty_output_queue. It is more flexible that way and I actually think it is
easier to understand.

I see also see in the v4l2-mem2mem.c source that the debug messages are very poorly
worded:
	src = v4l2_m2m_next_src_buf(m2m_ctx);

        if (!src && !m2m_ctx->out_q_ctx.buffered) {
                dprintk("No input buffers available\n");
                goto job_unlock;
        }

This should be either "source buffers" or "output buffers", but definitely not
"input buffers".

Ditto for the dst part.

> 
> In reality, drivers will add use case specific checks in their job_ready()
> implementation. For decoders, the cases I can think of are:
> 
> - On capture if you haven't parsed the stream header
> - On capture if the driver removes them from ready queue as a way to track which
> one are considered free and may be used at any time by the firmware
> - On output queue, if you need device_run() to be called to complete the drain
> the reorder queue
> 
> Yet, you want this check after stream headers are parsed, or whenever a new
> bitstream decode operation is to be queued in the firmware. So this check gets
> re-implemented, but dynamically, in all decoders.
> 
> Deinterlacers may needs this too with some algorithms (the one that introduce
> delays at least). Its not clear to me why it was called buffered,
> ignore_rdy_queue might have been an option, though I'm not fully confident. Note
> that M2M can be confusing, since whenever you ask for last something, its always
> relative to the ready queue, and may not make a lot of sense in the context it
> is used.
> 
>>
>> For codec support there are a number of issues that increase complexity:
>> implementing support for the LAST flag and events, and supporting buffers
>> that can be held. Especially since driver implementations tend to vary.
>>
>> I've been experimenting with some cleanups and changes in v4l2-mem2mem.c
>> (https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=enc-dec-cmd), mainly
>> surrounding the handling of the LAST flag. Note: this is failing the compliance
>> tests, I haven't had the time to pursue this further.
>>
>> I'm not sure whether the best approach is to move things out of the m2m framework,
>> or move things into the m2m framework, or add a more codec-specific layer on top
>> of the m2m framework, or a combination of all of these.
>>
>> It is something that needs experimentation, just see what works.
> 
> I can see you have omitted mark_stopped() calles when refactoring, which makes
> these patches change the behaviour. Could be related.

Could be. I hope to be able to spend a bit of time on this today.

> 
> This is no longer strictly related to this patch, but I think cmd_stop()
> implementation (even after your changes) are miss-fit for driver that speaks to
> firmware. As the firmware is being made aware of the free buffers, you can't
> just cherry-pick from the capture queue, you have to synchronise your state with
> the firmware while draining. The helper should be split in two parts I suppose,
> but cutting the line isn't easy.
> 
> Thread safe usage of the numerous boolean implicated in the draining state is
> also difficult. There is no other option then introduce a mutex or spinlock (if
> the state is needed in job_ready() implementation) to make this thread safe and
> reliable.

Regards,

	Hans

> 
>>
>> But for this specific flag: I think it is fine to put that in the m2m framework,
>> just document and name it well.
> 
> Ack.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> regards,
>>> Nicolas
>>>
>>>>
>>>>>   *
>>>>>   * Queue for buffers ready to be processed as soon as this
>>>>>   * instance receives access to the device.
>>>>> @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
>>>>>  	spinlock_t		rdy_spinlock;
>>>>>  	u8			num_rdy;
>>>>>  	bool			buffered;
>>>>> +	bool			ignore_streaming;
>>>>>  };
>>>>>  
>>>>>  /**
>>>>> @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
>>>>>  	m2m_ctx->cap_q_ctx.buffered = buffered;
>>>>>  }
>>>>>  
>>>>> +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
>>>>> +						     bool ignore_streaming)
>>>>> +{
>>>>> +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
>>>>> +}
>>>>> +
>>>>
>>>> I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
>>>> document that drivers can set this.
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>>  /**
>>>>>   * v4l2_m2m_ctx_release() - release m2m context
>>>>>   *
>>>>>
>>>>
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Nicolas Dufresne]

Le vendredi 22 septembre 2023 à 10:28 +0200, Hans Verkuil a écrit :
> On 21/09/2023 20:39, Nicolas Dufresne wrote:
> > Le mercredi 20 septembre 2023 à 16:49 +0200, Hans Verkuil a écrit :
> > > On 20/09/2023 16:08, Nicolas Dufresne wrote:
> > > > cc Tomasz Figa
> > > > 
> > > > Le mercredi 20 septembre 2023 à 14:59 +0200, Hans Verkuil a écrit :
> > > > > On 15/09/2023 23:11, Sebastian Fricke wrote:
> > > > > > Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
> > > > > > must be streaming in order to allow queuing jobs to the ready queue.
> > > > > > Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
> > > > > > allow adding new jobs. This behavior limits the usability of M2M for
> > > > > > some drivers, as these have to be able, to perform analysis of the
> > > > > 
> > > > > able, to -> able to
> > > > > 
> > > > > > sequence to ensure, that userspace prepares the CAPTURE queue correctly.
> > > > > 
> > > > > ensure, that -> ensure that
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > > > ---
> > > > > >  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
> > > > > >  1 file changed, 17 insertions(+)
> > > > > > 
> > > > > > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > > > > > index d6c8eb2b5201..97a48e61e358 100644
> > > > > > --- a/include/media/v4l2-mem2mem.h
> > > > > > +++ b/include/media/v4l2-mem2mem.h
> > > > > > @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
> > > > > >   * @rdy_spinlock: spin lock to protect the struct usage
> > > > > >   * @num_rdy:	number of buffers ready to be processed
> > > > > >   * @buffered:	is the queue buffered?
> > > > > > + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
> > > > > > + *		      be queued.
> > > > > > + *		      This is useful, for example, when the driver requires to
> > > > > > + *		      initialize the sequence with a firmware, where only a
> > > > > > + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
> > > > > > + *		      queue is required to perform the anlysis of the bitstream
> > > > > > + *		      header.
> > > > > > + *		      This means the driver is responsible for implementing the
> > > > > > + *		      job_ready callback correctly to make sure that requirements
> > > > > > + *		      for actual decoding are met.
> > > > > 
> > > > > This is a bad description and field name.
> > > > 
> > > > I wonder what's your opinion about the buffered one then :-D
> > > 
> > > Even worse :-)
> > > 
> > > I still don't really understand what that does. Patches welcome.
> > > 
> > > > 
> > > > > 
> > > > > Basically what this field does is that, if true, the streaming state of the
> > > > > capture queue is ignored. So just call it that: ignore_cap_streaming.
> > > > > 
> > > > > And explain that, if true, job_ready() will be called even if the capture
> > > > > queue is not streaming, and that that can be used to allow hardware to
> > > > > analyze the bitstream header that arrives on the OUTPUT queue.
> > > > 
> > > > Ack.
> > > > 
> > > > > 
> > > > > Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
> > > > > for the output queue, this is really a configuration for the m2m context as
> > > > > a whole.
> > > > 
> > > > Unless we come up with a completely new type of M2M that can behave like a gap
> > > > filler (like a video rate m2m), it indeed makes no sense for output. I'm just
> > > > illustrating that this is true "now" but someone can come up with valid
> > > > expectation. So I agree with you, we can move it up in the hierarchy.
> > > > 
> > > > Recently over IRC and other threads, Tomasz raised a concern that CODECs where
> > > > introducing too much complexity into M2M. And I believe buffered (which is
> > > > barely documented) and this mechanism was being pointed.
> > > > 
> > > > My take on that is that adding boolean configuration is what introduce
> > > > complexity, and we can fix it by doing less in the m2m. After this discussion, I
> > > > came with the idea that we should remove buffered and ignore_streaming. For
> > > > drivers that don't implement job_ready, this logic would be moved inside the
> > > > default implementation. We can then add a helper to check the common conditions.
> > > > 
> > > > The alternative suggested by Tomasz, was to layer two ops. We'd have a
> > > > device_ready() ops and its default implementation would include the check we
> > > > have and would call job_ready(). Personally, I'd rather remove then add, but I
> > > > understadt the reasoning and would be fine committing to that instead.
> > > > 
> > > > I'd like your feedback on this proposal. If this is something we want, I'll do
> > > > this prior to V13, otherwise we will address your comments and fix the added
> > > > mechanism. I think though that we agree that for decoders, this is nice addition
> > > > to not have to trigger work manually from vb2 ops.
> > > 
> > > It comes down to a matter of taste, I guess. I personally think that using bools
> > > to tweak the behavior of a framework does not necessarily increase complexity,
> > > provided it is clearly documented what it does and why it is needed.
> > > 
> > > I think an ignore_cap_streaming bool is pretty straightforward and has minimal
> > > impact in the code. As long as there are good comments.
> > 
> > So for wave5 we will opt for this and apply your suggested changes. And I may
> > come back later on the subject.
> > 
> > > 
> > > The 'buffered' flag is were this clearly failed completely, since I couldn't figure
> > > out what it is supposed to do. But that is not because it makes the code more
> > > complex, it is just because of shoddy documentation and naming.
> > > 
> > > Quite often implementing tweaks like that are quite easy in a framework, since
> > > you have all the information readily available. In a driver it can quickly become
> > > messy.
> > 
> > In this case, "buffered" is used to disable the checks for having at least one
> > buffer in the ready queues. In most cases, if you don't have at least 1 pending
> > capture and 1 pending output buffer, there is no point in calling device_run.
> 
> So it is really similar to ignore_cap_streaming: that relaxes the streaming test,
> and 'buffered' relaxes the 'must have at least one capture and output buffer ready'
> test.
> 
> So this should be renamed to: allow_empty_queues
> 
> Although I would prefer to split this into two bools: allow_empty_capture_queue and
> allow_empty_output_queue. It is more flexible that way and I actually think it is
> easier to understand.

Its on the queue ctx, so it does not have to be typed. It would have to be typed
if moved to m2m ctx.

> 
> I see also see in the v4l2-mem2mem.c source that the debug messages are very poorly
> worded:
> 	src = v4l2_m2m_next_src_buf(m2m_ctx);
> 
>         if (!src && !m2m_ctx->out_q_ctx.buffered) {
>                 dprintk("No input buffers available\n");
>                 goto job_unlock;
>         }
> 
> This should be either "source buffers" or "output buffers", but definitely not
> "input buffers".
> 
> Ditto for the dst part.

Indeed, I'll store this node somewhere for future work on the framework, this is
not strictly related to wave5 anymore.

> 
> > 
> > In reality, drivers will add use case specific checks in their job_ready()
> > implementation. For decoders, the cases I can think of are:
> > 
> > - On capture if you haven't parsed the stream header
> > - On capture if the driver removes them from ready queue as a way to track which
> > one are considered free and may be used at any time by the firmware
> > - On output queue, if you need device_run() to be called to complete the drain
> > the reorder queue
> > 
> > Yet, you want this check after stream headers are parsed, or whenever a new
> > bitstream decode operation is to be queued in the firmware. So this check gets
> > re-implemented, but dynamically, in all decoders.
> > 
> > Deinterlacers may needs this too with some algorithms (the one that introduce
> > delays at least). Its not clear to me why it was called buffered,
> > ignore_rdy_queue might have been an option, though I'm not fully confident. Note
> > that M2M can be confusing, since whenever you ask for last something, its always
> > relative to the ready queue, and may not make a lot of sense in the context it
> > is used.
> > 
> > > 
> > > For codec support there are a number of issues that increase complexity:
> > > implementing support for the LAST flag and events, and supporting buffers
> > > that can be held. Especially since driver implementations tend to vary.
> > > 
> > > I've been experimenting with some cleanups and changes in v4l2-mem2mem.c
> > > (https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=enc-dec-cmd), mainly
> > > surrounding the handling of the LAST flag. Note: this is failing the compliance
> > > tests, I haven't had the time to pursue this further.
> > > 
> > > I'm not sure whether the best approach is to move things out of the m2m framework,
> > > or move things into the m2m framework, or add a more codec-specific layer on top
> > > of the m2m framework, or a combination of all of these.
> > > 
> > > It is something that needs experimentation, just see what works.
> > 
> > I can see you have omitted mark_stopped() calles when refactoring, which makes
> > these patches change the behaviour. Could be related.
> 
> Could be. I hope to be able to spend a bit of time on this today.
> 
> > 
> > This is no longer strictly related to this patch, but I think cmd_stop()
> > implementation (even after your changes) are miss-fit for driver that speaks to
> > firmware. As the firmware is being made aware of the free buffers, you can't
> > just cherry-pick from the capture queue, you have to synchronise your state with
> > the firmware while draining. The helper should be split in two parts I suppose,
> > but cutting the line isn't easy.
> > 
> > Thread safe usage of the numerous boolean implicated in the draining state is
> > also difficult. There is no other option then introduce a mutex or spinlock (if
> > the state is needed in job_ready() implementation) to make this thread safe and
> > reliable.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > > 
> > > But for this specific flag: I think it is fine to put that in the m2m framework,
> > > just document and name it well.
> > 
> > Ack.
> > 
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > > 
> > > > regards,
> > > > Nicolas
> > > > 
> > > > > 
> > > > > >   *
> > > > > >   * Queue for buffers ready to be processed as soon as this
> > > > > >   * instance receives access to the device.
> > > > > > @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
> > > > > >  	spinlock_t		rdy_spinlock;
> > > > > >  	u8			num_rdy;
> > > > > >  	bool			buffered;
> > > > > > +	bool			ignore_streaming;
> > > > > >  };
> > > > > >  
> > > > > >  /**
> > > > > > @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
> > > > > >  	m2m_ctx->cap_q_ctx.buffered = buffered;
> > > > > >  }
> > > > > >  
> > > > > > +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
> > > > > > +						     bool ignore_streaming)
> > > > > > +{
> > > > > > +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
> > > > > document that drivers can set this.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > 	Hans
> > > > > 
> > > > > >  /**
> > > > > >   * v4l2_m2m_ctx_release() - release m2m context
> > > > > >   *
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 1/7] media: v4l2: Add ignore_streaming flag

[Hans Verkuil]

On 22/09/2023 22:20, Nicolas Dufresne wrote:
> Le vendredi 22 septembre 2023 à 10:28 +0200, Hans Verkuil a écrit :
>> On 21/09/2023 20:39, Nicolas Dufresne wrote:
>>> Le mercredi 20 septembre 2023 à 16:49 +0200, Hans Verkuil a écrit :
>>>> On 20/09/2023 16:08, Nicolas Dufresne wrote:
>>>>> cc Tomasz Figa
>>>>>
>>>>> Le mercredi 20 septembre 2023 à 14:59 +0200, Hans Verkuil a écrit :
>>>>>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>>>>>> Add a new flag to the `struct v4l2_m2m_dev` to toggle whether a queue
>>>>>>> must be streaming in order to allow queuing jobs to the ready queue.
>>>>>>> Currently, both queues (CAPTURE & OUTPUT) must be streaming in order to
>>>>>>> allow adding new jobs. This behavior limits the usability of M2M for
>>>>>>> some drivers, as these have to be able, to perform analysis of the
>>>>>>
>>>>>> able, to -> able to
>>>>>>
>>>>>>> sequence to ensure, that userspace prepares the CAPTURE queue correctly.
>>>>>>
>>>>>> ensure, that -> ensure that
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>> ---
>>>>>>>  include/media/v4l2-mem2mem.h | 17 +++++++++++++++++
>>>>>>>  1 file changed, 17 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
>>>>>>> index d6c8eb2b5201..97a48e61e358 100644
>>>>>>> --- a/include/media/v4l2-mem2mem.h
>>>>>>> +++ b/include/media/v4l2-mem2mem.h
>>>>>>> @@ -57,6 +57,16 @@ struct v4l2_m2m_dev;
>>>>>>>   * @rdy_spinlock: spin lock to protect the struct usage
>>>>>>>   * @num_rdy:	number of buffers ready to be processed
>>>>>>>   * @buffered:	is the queue buffered?
>>>>>>> + * @ignore_streaming: Dictates whether the queue must be streaming for a job to
>>>>>>> + *		      be queued.
>>>>>>> + *		      This is useful, for example, when the driver requires to
>>>>>>> + *		      initialize the sequence with a firmware, where only a
>>>>>>> + *		      queued OUTPUT queue buffer and STREAMON on the OUTPUT
>>>>>>> + *		      queue is required to perform the anlysis of the bitstream
>>>>>>> + *		      header.
>>>>>>> + *		      This means the driver is responsible for implementing the
>>>>>>> + *		      job_ready callback correctly to make sure that requirements
>>>>>>> + *		      for actual decoding are met.
>>>>>>
>>>>>> This is a bad description and field name.
>>>>>
>>>>> I wonder what's your opinion about the buffered one then :-D
>>>>
>>>> Even worse :-)
>>>>
>>>> I still don't really understand what that does. Patches welcome.
>>>>
>>>>>
>>>>>>
>>>>>> Basically what this field does is that, if true, the streaming state of the
>>>>>> capture queue is ignored. So just call it that: ignore_cap_streaming.
>>>>>>
>>>>>> And explain that, if true, job_ready() will be called even if the capture
>>>>>> queue is not streaming, and that that can be used to allow hardware to
>>>>>> analyze the bitstream header that arrives on the OUTPUT queue.
>>>>>
>>>>> Ack.
>>>>>
>>>>>>
>>>>>> Also, doesn't this field belong to struct v4l2_m2m_ctx? It makes no sense
>>>>>> for the output queue, this is really a configuration for the m2m context as
>>>>>> a whole.
>>>>>
>>>>> Unless we come up with a completely new type of M2M that can behave like a gap
>>>>> filler (like a video rate m2m), it indeed makes no sense for output. I'm just
>>>>> illustrating that this is true "now" but someone can come up with valid
>>>>> expectation. So I agree with you, we can move it up in the hierarchy.
>>>>>
>>>>> Recently over IRC and other threads, Tomasz raised a concern that CODECs where
>>>>> introducing too much complexity into M2M. And I believe buffered (which is
>>>>> barely documented) and this mechanism was being pointed.
>>>>>
>>>>> My take on that is that adding boolean configuration is what introduce
>>>>> complexity, and we can fix it by doing less in the m2m. After this discussion, I
>>>>> came with the idea that we should remove buffered and ignore_streaming. For
>>>>> drivers that don't implement job_ready, this logic would be moved inside the
>>>>> default implementation. We can then add a helper to check the common conditions.
>>>>>
>>>>> The alternative suggested by Tomasz, was to layer two ops. We'd have a
>>>>> device_ready() ops and its default implementation would include the check we
>>>>> have and would call job_ready(). Personally, I'd rather remove then add, but I
>>>>> understadt the reasoning and would be fine committing to that instead.
>>>>>
>>>>> I'd like your feedback on this proposal. If this is something we want, I'll do
>>>>> this prior to V13, otherwise we will address your comments and fix the added
>>>>> mechanism. I think though that we agree that for decoders, this is nice addition
>>>>> to not have to trigger work manually from vb2 ops.
>>>>
>>>> It comes down to a matter of taste, I guess. I personally think that using bools
>>>> to tweak the behavior of a framework does not necessarily increase complexity,
>>>> provided it is clearly documented what it does and why it is needed.
>>>>
>>>> I think an ignore_cap_streaming bool is pretty straightforward and has minimal
>>>> impact in the code. As long as there are good comments.
>>>
>>> So for wave5 we will opt for this and apply your suggested changes. And I may
>>> come back later on the subject.
>>>
>>>>
>>>> The 'buffered' flag is were this clearly failed completely, since I couldn't figure
>>>> out what it is supposed to do. But that is not because it makes the code more
>>>> complex, it is just because of shoddy documentation and naming.
>>>>
>>>> Quite often implementing tweaks like that are quite easy in a framework, since
>>>> you have all the information readily available. In a driver it can quickly become
>>>> messy.
>>>
>>> In this case, "buffered" is used to disable the checks for having at least one
>>> buffer in the ready queues. In most cases, if you don't have at least 1 pending
>>> capture and 1 pending output buffer, there is no point in calling device_run.
>>
>> So it is really similar to ignore_cap_streaming: that relaxes the streaming test,
>> and 'buffered' relaxes the 'must have at least one capture and output buffer ready'
>> test.
>>
>> So this should be renamed to: allow_empty_queues
>>
>> Although I would prefer to split this into two bools: allow_empty_capture_queue and
>> allow_empty_output_queue. It is more flexible that way and I actually think it is
>> easier to understand.
> 
> Its on the queue ctx, so it does not have to be typed. It would have to be typed
> if moved to m2m ctx.

Oops, I missed that. I'm not actually sure that's where it should be. If you
support flags that tweak the behavior, then put them together in a single place,
and not all over.

Regards,

	Hans

> 
>>
>> I see also see in the v4l2-mem2mem.c source that the debug messages are very poorly
>> worded:
>> 	src = v4l2_m2m_next_src_buf(m2m_ctx);
>>
>>         if (!src && !m2m_ctx->out_q_ctx.buffered) {
>>                 dprintk("No input buffers available\n");
>>                 goto job_unlock;
>>         }
>>
>> This should be either "source buffers" or "output buffers", but definitely not
>> "input buffers".
>>
>> Ditto for the dst part.
> 
> Indeed, I'll store this node somewhere for future work on the framework, this is
> not strictly related to wave5 anymore.
> 
>>
>>>
>>> In reality, drivers will add use case specific checks in their job_ready()
>>> implementation. For decoders, the cases I can think of are:
>>>
>>> - On capture if you haven't parsed the stream header
>>> - On capture if the driver removes them from ready queue as a way to track which
>>> one are considered free and may be used at any time by the firmware
>>> - On output queue, if you need device_run() to be called to complete the drain
>>> the reorder queue
>>>
>>> Yet, you want this check after stream headers are parsed, or whenever a new
>>> bitstream decode operation is to be queued in the firmware. So this check gets
>>> re-implemented, but dynamically, in all decoders.
>>>
>>> Deinterlacers may needs this too with some algorithms (the one that introduce
>>> delays at least). Its not clear to me why it was called buffered,
>>> ignore_rdy_queue might have been an option, though I'm not fully confident. Note
>>> that M2M can be confusing, since whenever you ask for last something, its always
>>> relative to the ready queue, and may not make a lot of sense in the context it
>>> is used.
>>>
>>>>
>>>> For codec support there are a number of issues that increase complexity:
>>>> implementing support for the LAST flag and events, and supporting buffers
>>>> that can be held. Especially since driver implementations tend to vary.
>>>>
>>>> I've been experimenting with some cleanups and changes in v4l2-mem2mem.c
>>>> (https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=enc-dec-cmd), mainly
>>>> surrounding the handling of the LAST flag. Note: this is failing the compliance
>>>> tests, I haven't had the time to pursue this further.
>>>>
>>>> I'm not sure whether the best approach is to move things out of the m2m framework,
>>>> or move things into the m2m framework, or add a more codec-specific layer on top
>>>> of the m2m framework, or a combination of all of these.
>>>>
>>>> It is something that needs experimentation, just see what works.
>>>
>>> I can see you have omitted mark_stopped() calles when refactoring, which makes
>>> these patches change the behaviour. Could be related.
>>
>> Could be. I hope to be able to spend a bit of time on this today.
>>
>>>
>>> This is no longer strictly related to this patch, but I think cmd_stop()
>>> implementation (even after your changes) are miss-fit for driver that speaks to
>>> firmware. As the firmware is being made aware of the free buffers, you can't
>>> just cherry-pick from the capture queue, you have to synchronise your state with
>>> the firmware while draining. The helper should be split in two parts I suppose,
>>> but cutting the line isn't easy.
>>>
>>> Thread safe usage of the numerous boolean implicated in the draining state is
>>> also difficult. There is no other option then introduce a mutex or spinlock (if
>>> the state is needed in job_ready() implementation) to make this thread safe and
>>> reliable.
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>>
>>>> But for this specific flag: I think it is fine to put that in the m2m framework,
>>>> just document and name it well.
>>>
>>> Ack.
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>>
>>>>> regards,
>>>>> Nicolas
>>>>>
>>>>>>
>>>>>>>   *
>>>>>>>   * Queue for buffers ready to be processed as soon as this
>>>>>>>   * instance receives access to the device.
>>>>>>> @@ -69,6 +79,7 @@ struct v4l2_m2m_queue_ctx {
>>>>>>>  	spinlock_t		rdy_spinlock;
>>>>>>>  	u8			num_rdy;
>>>>>>>  	bool			buffered;
>>>>>>> +	bool			ignore_streaming;
>>>>>>>  };
>>>>>>>  
>>>>>>>  /**
>>>>>>> @@ -564,6 +575,12 @@ static inline void v4l2_m2m_set_dst_buffered(struct v4l2_m2m_ctx *m2m_ctx,
>>>>>>>  	m2m_ctx->cap_q_ctx.buffered = buffered;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static inline void v4l2_m2m_set_dst_ignore_streaming(struct v4l2_m2m_ctx *m2m_ctx,
>>>>>>> +						     bool ignore_streaming)
>>>>>>> +{
>>>>>>> +	m2m_ctx->cap_q_ctx.ignore_streaming = ignore_streaming;
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> I think this is overkill, esp. when the field is moved to m2m_ctx. Just clearly
>>>>>> document that drivers can set this.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> 	Hans
>>>>>>
>>>>>>>  /**
>>>>>>>   * v4l2_m2m_ctx_release() - release m2m context
>>>>>>>   *
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12 2/7] media: v4l2: Allow M2M job queuing w/o streaming CAP queue

[Sebastian Fricke]

Allow decoder drivers to enable set the ignore_streaming flag on their
CAPTURE queue, to allow queuing jobs to the M2M ready queue and perform
firmware sequence analysis with just a streaming OUTPUT queue and
available bitstream data.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0cc30397fbad..2dbbe93be257 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -301,9 +301,12 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
 
 	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
 
-	if (!m2m_ctx->out_q_ctx.q.streaming
-	    || !m2m_ctx->cap_q_ctx.q.streaming) {
-		dprintk("Streaming needs to be on for both queues\n");
+	if (!m2m_ctx->out_q_ctx.q.streaming ||
+	    (!m2m_ctx->cap_q_ctx.q.streaming && !m2m_ctx->cap_q_ctx.ignore_streaming)) {
+		if (!m2m_ctx->cap_q_ctx.ignore_streaming)
+			dprintk("Streaming needs to be on for both queues\n");
+		else
+			dprintk("Streaming needs to be on for the OUTPUT queue\n");
 		return;
 	}
 

-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12 3/7] media: platform: chips-media: Move Coda to separate folder

[Sebastian Fricke]

Prepare the folder structure for a second Chips&Media driver.
Move the Coda driver to a sub-directory.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 MAINTAINERS                                            |  2 +-
 drivers/media/platform/chips-media/Kconfig             | 17 +----------------
 drivers/media/platform/chips-media/Makefile            |  5 +----
 drivers/media/platform/chips-media/coda/Kconfig        | 18 ++++++++++++++++++
 drivers/media/platform/chips-media/coda/Makefile       |  6 ++++++
 .../media/platform/chips-media/{ => coda}/coda-bit.c   |  0
 .../platform/chips-media/{ => coda}/coda-common.c      |  0
 .../media/platform/chips-media/{ => coda}/coda-gdi.c   |  0
 .../media/platform/chips-media/{ => coda}/coda-h264.c  |  0
 .../media/platform/chips-media/{ => coda}/coda-jpeg.c  |  0
 .../media/platform/chips-media/{ => coda}/coda-mpeg2.c |  0
 .../media/platform/chips-media/{ => coda}/coda-mpeg4.c |  0
 drivers/media/platform/chips-media/{ => coda}/coda.h   |  0
 .../media/platform/chips-media/{ => coda}/coda_regs.h  |  0
 .../media/platform/chips-media/{ => coda}/imx-vdoa.c   |  0
 .../media/platform/chips-media/{ => coda}/imx-vdoa.h   |  0
 drivers/media/platform/chips-media/{ => coda}/trace.h  |  2 +-
 17 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..063a11791bbf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5093,7 +5093,7 @@ M:	Philipp Zabel <p.zabel@pengutronix.de>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/coda.yaml
-F:	drivers/media/platform/chips-media/
+F:	drivers/media/platform/chips-media/coda
 
 CODE OF CONDUCT
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/drivers/media/platform/chips-media/Kconfig b/drivers/media/platform/chips-media/Kconfig
index 57f8f8a22df8..f87a0d693df7 100644
--- a/drivers/media/platform/chips-media/Kconfig
+++ b/drivers/media/platform/chips-media/Kconfig
@@ -2,19 +2,4 @@
 
 comment "Chips&Media media platform drivers"
 
-config VIDEO_CODA
-	tristate "Chips&Media Coda multi-standard codec IP"
-	depends on V4L_MEM2MEM_DRIVERS
-	depends on VIDEO_DEV && OF && (ARCH_MXC || COMPILE_TEST)
-	select SRAM
-	select VIDEOBUF2_DMA_CONTIG
-	select VIDEOBUF2_VMALLOC
-	select V4L2_JPEG_HELPER
-	select V4L2_MEM2MEM_DEV
-	select GENERIC_ALLOCATOR
-	help
-	   Coda is a range of video codec IPs that supports
-	   H.264, MPEG-4, and other video formats.
-
-config VIDEO_IMX_VDOA
-	def_tristate VIDEO_CODA if SOC_IMX6Q || COMPILE_TEST
+source "drivers/media/platform/chips-media/coda/Kconfig"
diff --git a/drivers/media/platform/chips-media/Makefile b/drivers/media/platform/chips-media/Makefile
index bbb16425a875..5ee693f651c1 100644
--- a/drivers/media/platform/chips-media/Makefile
+++ b/drivers/media/platform/chips-media/Makefile
@@ -1,6 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-coda-vpu-objs := coda-common.o coda-bit.o coda-gdi.o coda-h264.o coda-mpeg2.o coda-mpeg4.o coda-jpeg.o
-
-obj-$(CONFIG_VIDEO_CODA) += coda-vpu.o
-obj-$(CONFIG_VIDEO_IMX_VDOA) += imx-vdoa.o
+obj-y += coda/
diff --git a/drivers/media/platform/chips-media/coda/Kconfig b/drivers/media/platform/chips-media/coda/Kconfig
new file mode 100644
index 000000000000..cb7b66c71380
--- /dev/null
+++ b/drivers/media/platform/chips-media/coda/Kconfig
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config VIDEO_CODA
+	tristate "Chips&Media Coda multi-standard codec IP"
+	depends on V4L_MEM2MEM_DRIVERS
+	depends on VIDEO_DEV && OF && (ARCH_MXC || COMPILE_TEST)
+	select SRAM
+	select VIDEOBUF2_DMA_CONTIG
+	select VIDEOBUF2_VMALLOC
+	select V4L2_JPEG_HELPER
+	select V4L2_MEM2MEM_DEV
+	select GENERIC_ALLOCATOR
+	help
+	   Coda is a range of video codec IPs that supports
+	   H.264, MPEG-4, and other video formats.
+
+config VIDEO_IMX_VDOA
+	def_tristate VIDEO_CODA if SOC_IMX6Q || COMPILE_TEST
diff --git a/drivers/media/platform/chips-media/coda/Makefile b/drivers/media/platform/chips-media/coda/Makefile
new file mode 100644
index 000000000000..bbb16425a875
--- /dev/null
+++ b/drivers/media/platform/chips-media/coda/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+coda-vpu-objs := coda-common.o coda-bit.o coda-gdi.o coda-h264.o coda-mpeg2.o coda-mpeg4.o coda-jpeg.o
+
+obj-$(CONFIG_VIDEO_CODA) += coda-vpu.o
+obj-$(CONFIG_VIDEO_IMX_VDOA) += imx-vdoa.o
diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda/coda-bit.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-bit.c
rename to drivers/media/platform/chips-media/coda/coda-bit.c
diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda/coda-common.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-common.c
rename to drivers/media/platform/chips-media/coda/coda-common.c
diff --git a/drivers/media/platform/chips-media/coda-gdi.c b/drivers/media/platform/chips-media/coda/coda-gdi.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-gdi.c
rename to drivers/media/platform/chips-media/coda/coda-gdi.c
diff --git a/drivers/media/platform/chips-media/coda-h264.c b/drivers/media/platform/chips-media/coda/coda-h264.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-h264.c
rename to drivers/media/platform/chips-media/coda/coda-h264.c
diff --git a/drivers/media/platform/chips-media/coda-jpeg.c b/drivers/media/platform/chips-media/coda/coda-jpeg.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-jpeg.c
rename to drivers/media/platform/chips-media/coda/coda-jpeg.c
diff --git a/drivers/media/platform/chips-media/coda-mpeg2.c b/drivers/media/platform/chips-media/coda/coda-mpeg2.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-mpeg2.c
rename to drivers/media/platform/chips-media/coda/coda-mpeg2.c
diff --git a/drivers/media/platform/chips-media/coda-mpeg4.c b/drivers/media/platform/chips-media/coda/coda-mpeg4.c
similarity index 100%
rename from drivers/media/platform/chips-media/coda-mpeg4.c
rename to drivers/media/platform/chips-media/coda/coda-mpeg4.c
diff --git a/drivers/media/platform/chips-media/coda.h b/drivers/media/platform/chips-media/coda/coda.h
similarity index 100%
rename from drivers/media/platform/chips-media/coda.h
rename to drivers/media/platform/chips-media/coda/coda.h
diff --git a/drivers/media/platform/chips-media/coda_regs.h b/drivers/media/platform/chips-media/coda/coda_regs.h
similarity index 100%
rename from drivers/media/platform/chips-media/coda_regs.h
rename to drivers/media/platform/chips-media/coda/coda_regs.h
diff --git a/drivers/media/platform/chips-media/imx-vdoa.c b/drivers/media/platform/chips-media/coda/imx-vdoa.c
similarity index 100%
rename from drivers/media/platform/chips-media/imx-vdoa.c
rename to drivers/media/platform/chips-media/coda/imx-vdoa.c
diff --git a/drivers/media/platform/chips-media/imx-vdoa.h b/drivers/media/platform/chips-media/coda/imx-vdoa.h
similarity index 100%
rename from drivers/media/platform/chips-media/imx-vdoa.h
rename to drivers/media/platform/chips-media/coda/imx-vdoa.h
diff --git a/drivers/media/platform/chips-media/trace.h b/drivers/media/platform/chips-media/coda/trace.h
similarity index 99%
rename from drivers/media/platform/chips-media/trace.h
rename to drivers/media/platform/chips-media/coda/trace.h
index 19f98e6dafb9..abc6a01a74e9 100644
--- a/drivers/media/platform/chips-media/trace.h
+++ b/drivers/media/platform/chips-media/coda/trace.h
@@ -167,7 +167,7 @@ DEFINE_EVENT(coda_buf_class, coda_jpeg_done,
 #endif /* __CODA_TRACE_H__ */
 
 #undef TRACE_INCLUDE_PATH
-#define TRACE_INCLUDE_PATH ../../drivers/media/platform/chips-media
+#define TRACE_INCLUDE_PATH ../../drivers/media/platform/chips-media/coda
 #undef TRACE_INCLUDE_FILE
 #define TRACE_INCLUDE_FILE trace
 

-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Sebastian Fricke]

From: Robert Beckett <bob.beckett@collabora.com>

Add bindings for the wave5 chips&media codec driver

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 .../devicetree/bindings/media/cnm,wave5.yaml       | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/cnm,wave5.yaml b/Documentation/devicetree/bindings/media/cnm,wave5.yaml
new file mode 100644
index 000000000000..b8f383621805
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/cnm,wave5.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/cnm,wave5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Chips&Media Wave 5 Series multi-standard codec IP
+
+maintainers:
+  - Nas Chung <nas.chung@chipsnmedia.com>
+  - Jackson Lee <jackson.lee@chipsnmedia.com>
+
+description: |-
+  The Chips&Media WAVE codec IP is a multi format video encoder/decoder
+
+properties:
+  compatible:
+    enum:
+        - cnm,cm521c-vpu
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: VCODEC clock
+
+  clock-names:
+    items:
+      - const: vcodec
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+    description:
+      The VPU uses the SRAM to store some of the reference data instead of
+      storing it on DMA memory. It is mainly used for the purpose of reducing
+      bandwidth.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    vpu: video-codec@12345678 {
+        compatible = "cnm,cm521c-vpu";
+        reg = <0x12345678 0x1000>;
+        interrupts = <42>;
+        clocks = <&clks 42>;
+        clock-names = "vcodec";
+        sram = <&sram>;
+    };

-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Rob Herring]

On Fri, 15 Sep 2023 23:11:35 +0200, Sebastian Fricke wrote:
> From: Robert Beckett <bob.beckett@collabora.com>
> 
> Add bindings for the wave5 chips&media codec driver
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  .../devicetree/bindings/media/cnm,wave5.yaml       | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/media/cnm,wave5.yaml:19:9: [warning] wrong indentation: expected 6 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230915-wave5_v12_on_media_master-v12-6-92fc66cd685d@collabora.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Krzysztof Kozlowski]

On 15/09/2023 23:11, Sebastian Fricke wrote:
> From: Robert Beckett <bob.beckett@collabora.com>
> 
> Add bindings for the wave5 chips&media codec driver
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>

So this is v12 and still no tested?

A nit, subject: drop second/last, redundant "yaml devicetree indings".
The "dt-bindings" prefix is already stating that these are bindings.
Basically three words bringing zero information.

> ---
>  .../devicetree/bindings/media/cnm,wave5.yaml       | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/cnm,wave5.yaml b/Documentation/devicetree/bindings/media/cnm,wave5.yaml
> new file mode 100644
> index 000000000000..b8f383621805
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/cnm,wave5.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/cnm,wave5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Chips&Media Wave 5 Series multi-standard codec IP
> +
> +maintainers:
> +  - Nas Chung <nas.chung@chipsnmedia.com>
> +  - Jackson Lee <jackson.lee@chipsnmedia.com>
> +
> +description: |-

Do not need '|-' unless you need to preserve formatting.

> +  The Chips&Media WAVE codec IP is a multi format video encoder/decoder
> +
> +properties:
> +  compatible:
> +    enum:
> +        - cnm,cm521c-vpu

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Missing blank line

> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: VCODEC clock
> +
> +  clock-names:
> +    items:
> +      - const: vcodec

Drop clock-names, not really useful for one entry.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  sram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +

Drop blank line

> +    description:
> +      The VPU uses the SRAM to store some of the reference data instead of
> +      storing it on DMA memory. It is mainly used for the purpose of reducing
> +      bandwidth.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts

Keep the same order as listed in properties:

> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Sebastian Fricke]

Hey Krzysztof,

thanks for your review.

On 17.09.2023 09:56, Krzysztof Kozlowski wrote:
>On 15/09/2023 23:11, Sebastian Fricke wrote:
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> Add bindings for the wave5 chips&media codec driver
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>
>So this is v12 and still no tested?

I have tested it, multiple times actually since V11. (For some reason
that indentation issue slipped by me though ...)
If you mean the tested by tag, the patch was completely unnoticed until
v10 by the community, which was partially because me and the previous
commiters didn't use the right recipients for this patch. So from that
point of view this is more like v2.

>
>A nit, subject: drop second/last, redundant "yaml devicetree indings".
>The "dt-bindings" prefix is already stating that these are bindings.
>Basically three words bringing zero information.

Okay so:
`dt-bindings: media: wave5: add devicetree`
?

>
>> ---
>>  .../devicetree/bindings/media/cnm,wave5.yaml       | 66 ++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/cnm,wave5.yaml b/Documentation/devicetree/bindings/media/cnm,wave5.yaml
>> new file mode 100644
>> index 000000000000..b8f383621805
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/cnm,wave5.yaml
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/cnm,wave5.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Chips&Media Wave 5 Series multi-standard codec IP
>> +
>> +maintainers:
>> +  - Nas Chung <nas.chung@chipsnmedia.com>
>> +  - Jackson Lee <jackson.lee@chipsnmedia.com>
>> +
>> +description: |-
>
>Do not need '|-' unless you need to preserve formatting.

Ack.

>
>> +  The Chips&Media WAVE codec IP is a multi format video encoder/decoder
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +        - cnm,cm521c-vpu
>
>It does not look like you tested the bindings, at least after quick
>look. Please run `make dt_binding_check` (see
>Documentation/devicetree/bindings/writing-schema.rst for instructions).
>Maybe you need to update your dtschema and yamllint.

Here my testing output:
```
❯ make dt_binding_check DT_SCHEMA_FILES=cnm,wave5.yaml
   HOSTCC  scripts/basic/fixdep
   HOSTCC  scripts/dtc/dtc.o
   HOSTCC  scripts/dtc/flattree.o
   HOSTCC  scripts/dtc/fstree.o
   HOSTCC  scripts/dtc/data.o
   HOSTCC  scripts/dtc/livetree.o
   HOSTCC  scripts/dtc/treesource.o
   HOSTCC  scripts/dtc/srcpos.o
   HOSTCC  scripts/dtc/checks.o
   HOSTCC  scripts/dtc/util.o
   LEX     scripts/dtc/dtc-lexer.lex.c
   YACC    scripts/dtc/dtc-parser.tab.[ch]
   HOSTCC  scripts/dtc/dtc-lexer.lex.o
   HOSTCC  scripts/dtc/dtc-parser.tab.o
   HOSTLD  scripts/dtc/dtc
   LINT    Documentation/devicetree/bindings
./Documentation/devicetree/bindings/media/cnm,wave5.yaml:19:9: [warning] wrong indentation: expected 6 but found 8 (indentation)
   CHKDT   Documentation/devicetree/bindings/processed-schema.json
   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
   DTEX    Documentation/devicetree/bindings/media/cnm,wave5.example.dts
   DTC_CHK Documentation/devicetree/bindings/media/cnm,wave5.example.dtb
```

Again sorry about missing the indentation warning, but nothing else was
highlighted.

Both dtschema and yamllint seem to be up-to-date:
```
❯ python3 -m pip --version
pip 23.2.1 from /home/basti/.local/lib/python3.8/site-packages/pip (python 3.8)
❯ pip3 show dtschema
Name: dtschema
Version: 2023.7
Summary: DeviceTree validation schema and tools
Home-page: https://github.com/devicetree-org/dt-schema
Author: Rob Herring
Author-email: robh@kernel.org
License: BSD
Location: /home/basti/.local/lib/python3.8/site-packages
Requires: jsonschema, pylibfdt, rfc3987, ruamel.yaml
Required-by: 
❯ pip3 show yamllint
Name: yamllint
Version: 1.32.0
Summary: A linter for YAML files.
Home-page: 
Author: Adrien Vergé
Author-email: 
License: GPL-3.0-only
Location: /home/basti/.local/lib/python3.8/site-packages
Requires: pathspec, pyyaml
Required-by: 
```

>
>Missing blank line

Ack, will add that.
>
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: VCODEC clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: vcodec
>
>Drop clock-names, not really useful for one entry.

Ack

>
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  sram:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>
>Drop blank line

Ack

>
>> +    description:
>> +      The VPU uses the SRAM to store some of the reference data instead of
>> +      storing it on DMA memory. It is mainly used for the purpose of reducing
>> +      bandwidth.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>
>Keep the same order as listed in properties:

Ack

>
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>
>Best regards,
>Krzysztof

Sincerely,
Sebastian

>
>_______________________________________________
>Kernel mailing list -- kernel@mailman.collabora.com
>To unsubscribe send an email to kernel-leave@mailman.collabora.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Krzysztof Kozlowski]

On 18/09/2023 08:49, Sebastian Fricke wrote:
> Hey Krzysztof,
> 
> thanks for your review.
> 
> On 17.09.2023 09:56, Krzysztof Kozlowski wrote:
>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>> From: Robert Beckett <bob.beckett@collabora.com>
>>>
>>> Add bindings for the wave5 chips&media codec driver
>>>
>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>
>> So this is v12 and still no tested?
> 
> I have tested it, multiple times actually since V11. (For some reason
> that indentation issue slipped by me though ...)
> If you mean the tested by tag, the patch was completely unnoticed until
> v10 by the community, which was partially because me and the previous
> commiters didn't use the right recipients for this patch. So from that
> point of view this is more like v2.
> 
>>
>> A nit, subject: drop second/last, redundant "yaml devicetree indings".
>> The "dt-bindings" prefix is already stating that these are bindings.
>> Basically three words bringing zero information.
> 
> Okay so:
> `dt-bindings: media: wave5: add devicetree`

Still not, because devicetree is duplicating "dt". It's redundant.

Instead should be (with correct order of prefixes):

media: dt-bindings: wave5: add AzureWaveFooBar XYL ABC10 (whatever
company and full product name it is)


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Nicolas Dufresne]

Le lundi 18 septembre 2023 à 14:02 +0200, Krzysztof Kozlowski a écrit :
> On 18/09/2023 08:49, Sebastian Fricke wrote:
> > Hey Krzysztof,
> > 
> > thanks for your review.
> > 
> > On 17.09.2023 09:56, Krzysztof Kozlowski wrote:
> > > On 15/09/2023 23:11, Sebastian Fricke wrote:
> > > > From: Robert Beckett <bob.beckett@collabora.com>
> > > > 
> > > > Add bindings for the wave5 chips&media codec driver
> > > > 
> > > > Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > 
> > > So this is v12 and still no tested?
> > 
> > I have tested it, multiple times actually since V11. (For some reason
> > that indentation issue slipped by me though ...)
> > If you mean the tested by tag, the patch was completely unnoticed until
> > v10 by the community, which was partially because me and the previous
> > commiters didn't use the right recipients for this patch. So from that
> > point of view this is more like v2.
> > 
> > > 
> > > A nit, subject: drop second/last, redundant "yaml devicetree indings".
> > > The "dt-bindings" prefix is already stating that these are bindings.
> > > Basically three words bringing zero information.
> > 
> > Okay so:
> > `dt-bindings: media: wave5: add devicetree`
> 
> Still not, because devicetree is duplicating "dt". It's redundant.
> 
> Instead should be (with correct order of prefixes):
> 
> media: dt-bindings: wave5: add AzureWaveFooBar XYL ABC10 (whatever
> company and full product name it is)

So maybe this one ?

  media: dt-bindings: wave5: add Chips&Media 521c codec IP support

> 
> 
> Best regards,
> Krzysztof
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 6/7] dt-bindings: media: wave5: add yaml devicetree bindings

[Krzysztof Kozlowski]

On 18/09/2023 21:16, Nicolas Dufresne wrote:
>>>>
>>>> A nit, subject: drop second/last, redundant "yaml devicetree indings".
>>>> The "dt-bindings" prefix is already stating that these are bindings.
>>>> Basically three words bringing zero information.
>>>
>>> Okay so:
>>> `dt-bindings: media: wave5: add devicetree`
>>
>> Still not, because devicetree is duplicating "dt". It's redundant.
>>
>> Instead should be (with correct order of prefixes):
>>
>> media: dt-bindings: wave5: add AzureWaveFooBar XYL ABC10 (whatever
>> company and full product name it is)
> 
> So maybe this one ?
> 
>   media: dt-bindings: wave5: add Chips&Media 521c codec IP support

Sure, sounds good for me.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12 7/7] media: chips-media: wave5: Add wave5 driver to maintainers file

[Sebastian Fricke]

From: Robert Beckett <bob.beckett@collabora.com>

Add the Chips&Media wave5 encoder/decoder driver to the maintainers file

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 063a11791bbf..b6a592c14caa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23206,6 +23206,14 @@ F:	include/linux/watchdog.h
 F:	include/trace/events/watchdog.h
 F:	include/uapi/linux/watchdog.h
 
+WAVE5 VPU CODEC DRIVER
+M:	Nas Chung <nas.chung@chipsnmedia.com>
+M:	Jackson Lee <jackson.lee@chipsnmedia.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/cnm,wave5.yaml
+F:	drivers/media/platform/chips-media/wave5/
+
 WHISKEYCOVE PMIC GPIO DRIVER
 M:	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
 L:	linux-gpio@vger.kernel.org

-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 7/7] media: chips-media: wave5: Add wave5 driver to maintainers file

[Hans Verkuil]

On 15/09/2023 23:11, Sebastian Fricke wrote:
> From: Robert Beckett <bob.beckett@collabora.com>
> 
> Add the Chips&Media wave5 encoder/decoder driver to the maintainers file
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 063a11791bbf..b6a592c14caa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23206,6 +23206,14 @@ F:	include/linux/watchdog.h
>  F:	include/trace/events/watchdog.h
>  F:	include/uapi/linux/watchdog.h
>  
> +WAVE5 VPU CODEC DRIVER
> +M:	Nas Chung <nas.chung@chipsnmedia.com>
> +M:	Jackson Lee <jackson.lee@chipsnmedia.com>

Hmm, is this right? Shouldn't Sebastian be added here? Or is it really
intended that once this driver is merged, any maintenance will be done
by Chips&Media people?

Just checking if this is intended.

Regards,

	Hans

> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/cnm,wave5.yaml
> +F:	drivers/media/platform/chips-media/wave5/
> +
>  WHISKEYCOVE PMIC GPIO DRIVER
>  M:	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>  L:	linux-gpio@vger.kernel.org
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 7/7] media: chips-media: wave5: Add wave5 driver to maintainers file

[Sebastian Fricke]

Hey Hans,

thanks for your review on our driver.

On 20.09.2023 15:02, Hans Verkuil wrote:
>On 15/09/2023 23:11, Sebastian Fricke wrote:
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> Add the Chips&Media wave5 encoder/decoder driver to the maintainers file
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>> ---
>>  MAINTAINERS | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 063a11791bbf..b6a592c14caa 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23206,6 +23206,14 @@ F:	include/linux/watchdog.h
>>  F:	include/trace/events/watchdog.h
>>  F:	include/uapi/linux/watchdog.h
>>
>> +WAVE5 VPU CODEC DRIVER
>> +M:	Nas Chung <nas.chung@chipsnmedia.com>
>> +M:	Jackson Lee <jackson.lee@chipsnmedia.com>
>
>Hmm, is this right? Shouldn't Sebastian be added here? Or is it really
>intended that once this driver is merged, any maintenance will be done
>by Chips&Media people?
>
>Just checking if this is intended.

Yes, this is communicated with Chips & Media, they want to take full
responsibility for the maintenance.

>
>Regards,
>
>	Hans

Sincerely,
Sebastian

>
>> +L:	linux-media@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/media/cnm,wave5.yaml
>> +F:	drivers/media/platform/chips-media/wave5/
>> +
>>  WHISKEYCOVE PMIC GPIO DRIVER
>>  M:	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>  L:	linux-gpio@vger.kernel.org
>>
>
>_______________________________________________
>Kernel mailing list -- kernel@mailman.collabora.com
>To unsubscribe send an email to kernel-leave@mailman.collabora.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer

[Ivan Bornyakov]

Hi, Sebastian,

On Fri, Sep 15, 2023 at 23:11:34 +0200, Sebastian Fricke wrote:
> From: Nas Chung <nas.chung@chipsnmedia.com>
> 
> Add the decoder and encoder implementing the v4l2
> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>

[...]

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> new file mode 100644
> index 000000000000..a13d968f5d04
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c

[...]

> +static void wave5_vpu_get_interrupt_for_inst(struct vpu_instance *inst, u32 status)
> +{
> +	struct vpu_device *dev = inst->dev;
> +	u32 seq_done;
> +	u32 cmd_done;
> +	int val;
> +
> +	seq_done = wave5_vdi_read_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO);
> +	cmd_done = wave5_vdi_read_register(dev, W5_RET_QUEUE_CMD_DONE_INST);
> +
> +	if (status & BIT(INT_WAVE5_INIT_SEQ)) {
> +		if (seq_done & BIT(inst->id)) {
> +			seq_done &= ~BIT(inst->id);
> +			wave5_vdi_write_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO, seq_done);
> +			val = BIT(INT_WAVE5_INIT_SEQ);
> +			kfifo_in(&inst->irq_status, &val, sizeof(int));
> +		}
> +	}
> +	if (status & BIT(INT_WAVE5_ENC_SET_PARAM)) {
> +		if (seq_done & BIT(inst->id)) {
> +			seq_done &= ~BIT(inst->id);
> +			wave5_vdi_write_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO, seq_done);
> +			val = BIT(INT_WAVE5_ENC_SET_PARAM);
> +			kfifo_in(&inst->irq_status, &val, sizeof(int));
> +		}
> +	}
> +	if (status & BIT(INT_WAVE5_DEC_PIC) ||
> +	    status & BIT(INT_WAVE5_ENC_PIC)) {
> +		if (cmd_done & BIT(inst->id)) {
> +			cmd_done &= ~BIT(inst->id);
> +			wave5_vdi_write_register(dev, W5_RET_QUEUE_CMD_DONE_INST, cmd_done);
> +			val = BIT(INT_WAVE5_DEC_PIC);
> +			kfifo_in(&inst->irq_status, &val, sizeof(int));
> +		}
> +	}
> +}
> +
> +static irqreturn_t wave5_vpu_irq(int irq, void *dev_id)
> +{
> +	struct vpu_device *dev = dev_id;
> +
> +	if (wave5_vdi_read_register(dev, W5_VPU_VPU_INT_STS)) {
> +		struct vpu_instance *inst;
> +		u32 irq_status = wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
> +
> +		list_for_each_entry(inst, &dev->instances, list) {
> +			wave5_vpu_get_interrupt_for_inst(inst, irq_status);
> +		}
> +
> +		wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_status);
> +		wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
> +
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
> +{
> +	struct vpu_device *dev = dev_id;
> +	struct vpu_instance *inst;
> +	int irq_status, ret;
> +
> +	list_for_each_entry(inst, &dev->instances, list) {
> +		while (kfifo_len(&inst->irq_status)) {
> +			ret = kfifo_out(&inst->irq_status, &irq_status, sizeof(int));
> +			if (!ret)
> +				break;
> +
> +			if (irq_status == BIT(INT_WAVE5_INIT_SEQ) ||
> +			    irq_status == BIT(INT_WAVE5_ENC_SET_PARAM))
> +				complete(&inst->irq_done);
> +			else /* DEC/ENC_PIC */
> +				inst->ops->finish_process(inst);
> +
> +			wave5_vpu_clear_interrupt(inst, irq_status);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}

I believe, instead of
wave5_vpu_irq() + wave5_vpu_get_interrupt_for_inst() + wave5_vpu_irq_thread()
you can reduce interrupt handling to only threaded part with something like this:

static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
{
	u32 irq_status, seq_done, cmd_done;
	struct vpu_device *dev = dev_id;
	struct vpu_instance *inst;

	while (wave5_vdi_read_register(dev, W5_VPU_VPU_INT_STS)) {
		irq_status = wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
		seq_done = wave5_vdi_read_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO);
		cmd_done = wave5_vdi_read_register(dev, W5_RET_QUEUE_CMD_DONE_INST);

		list_for_each_entry(inst, &dev->instances, list) {
			if (irq_status & BIT(INT_WAVE5_INIT_SEQ) ||
			    irq_status & BIT(INT_WAVE5_ENC_SET_PARAM)) {
				if (seq_done & BIT(inst->id)) {
					seq_done &= ~BIT(inst->id);
					wave5_vdi_write_register(dev,
								 W5_RET_SEQ_DONE_INSTANCE_INFO,
								 seq_done);
					complete(&inst->irq_done);
				}
			}

			if (status & BIT(INT_WAVE5_DEC_PIC) ||
			    status & BIT(INT_WAVE5_ENC_PIC)) {
				if (cmd_done & BIT(inst->id)) {
					cmd_done &= ~BIT(inst->id);
					wave5_vdi_write_register(dev,
								 W5_RET_QUEUE_CMD_DONE_INST,
								 cmd_done);
					inst->ops->finish_process(inst);
				}
			}

			wave5_vpu_clear_interrupt(inst, irq_status);
		}
			
		wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_status);
		wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
	}

	return IRQ_HANDLED;
}

Is it better?

[...]

> +static int wave5_vpu_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct vpu_device *dev;
> +	const struct wave5_match_data *match_data;
> +	u32 fw_revision;
> +
> +	match_data = device_get_match_data(&pdev->dev);
> +	if (!match_data) {
> +		dev_err(&pdev->dev, "missing device match data\n");
> +		return -EINVAL;
> +	}
> +
> +	/* physical addresses limited to 32 bits */
> +	dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));

dma_set_mask_and_coherent()? Also error check?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer

[Hans Verkuil]

On 21/09/2023 21:11, Nicolas Dufresne wrote:
> Le mercredi 20 septembre 2023 à 17:13 +0200, Hans Verkuil a écrit :
>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>> From: Nas Chung <nas.chung@chipsnmedia.com>
>>>
>>> Add the decoder and encoder implementing the v4l2
>>> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
>>>
>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
>>> ---
>>>  drivers/media/platform/chips-media/Kconfig         |    1 +
>>>  drivers/media/platform/chips-media/Makefile        |    1 +
>>>  drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
>>>  drivers/media/platform/chips-media/wave5/Makefile  |   10 +
>>>  .../platform/chips-media/wave5/wave5-helper.c      |  196 ++
>>>  .../platform/chips-media/wave5/wave5-helper.h      |   30 +
>>>  .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1965 ++++++++++++++++++++
>>>  .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1825 ++++++++++++++++++
>>>  .../media/platform/chips-media/wave5/wave5-vpu.c   |  331 ++++
>>>  .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
>>>  10 files changed, 4454 insertions(+)
>>>

<snip>

>>> +static int wave5_vpu_dec_set_eos_on_firmware(struct vpu_instance *inst)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = wave5_vpu_dec_update_bitstream_buffer(inst, 0);
>>> +	if (ret) {
>>> +		dev_err(inst->dev->dev,
>>> +			"Setting EOS for the bitstream, fail: %d\n", ret);
>>
>> Is this an error due to a driver problem, or because a bad bitstream is
>> fed from userspace? In the first case, dev_err would be right, in the
>> second dev_dbg would be more appropriate. Bad userspace input should not
>> spam the kernel log in general.
> 
> Its the first. To set the EOS flag, a command is sent to the firmware. That
> command may never return (timeout) or may report an error. For this specific
> command, if that happens we are likely facing firmware of driver problem (or
> both).

OK, I'd add that as a comment here as this is unexpected behavior.

> 
>>
>>> +		return ret;
>>> +	}
>>> +	return 0;
>>> +}

<snip>

>>> +static int wave5_vpu_dec_create_bufs(struct file *file, void *priv,
>>> +				     struct v4l2_create_buffers *create)
>>> +{
>>> +	struct v4l2_format *f = &create->format;
>>> +
>>> +	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>> +		return -ENOTTY;
>>
>> Huh? Why is this needed?
> 
> Minimally a comment should be added. The why is that we support CREATE_BUF for
> OUTPUT queue (bitstream) but not for CAPTURE queues. This is simply not
> supported by Wave5 firmware. Do you have any suggestion how this asymmetry can
> be implemented better ?

Certainly not with ENOTTY: the ioctl exists, it is just not supported for
CAPTURE queues.

How about -EPERM? And document this error as well in the VIDIOC_CREATE_BUFS
documentation. And you want a dev_dbg here too.

So I would propose that EPERM is returned if CREATE_BUFS is only supported
for for one of the two queues of an M2M device.

> 
>>
>>> +
>>> +	return v4l2_m2m_ioctl_create_bufs(file, priv, create);
>>> +}

<snip>

>>> +static int wave5_vpu_dec_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
>>> +				     unsigned int *num_planes, unsigned int sizes[],
>>> +				     struct device *alloc_devs[])
>>> +{
>>> +	struct vpu_instance *inst = vb2_get_drv_priv(q);
>>> +	struct v4l2_pix_format_mplane inst_format =
>>> +		(q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ? inst->src_fmt : inst->dst_fmt;
>>> +	unsigned int i;
>>> +
>>> +	dev_dbg(inst->dev->dev, "%s: num_buffers: %u | num_planes: %u | type: %u\n", __func__,
>>> +		*num_buffers, *num_planes, q->type);
>>> +
>>> +	/* the CREATE_BUFS case */
>>> +	if (*num_planes) {
>>> +		if (inst_format.num_planes != *num_planes)
>>> +			return -EINVAL;
>>> +
>>> +		for (i = 0; i < *num_planes; i++) {
>>> +			if (sizes[i] < inst_format.plane_fmt[i].sizeimage)
>>> +				return -EINVAL;
>>> +		}
>>> +	/* the REQBUFS case */
>>> +	} else {
>>> +		*num_planes = inst_format.num_planes;
>>> +
>>> +		if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>> +			sizes[0] = inst_format.plane_fmt[0].sizeimage;
>>> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
>>> +		} else if (*num_planes == 1) {
>>> +			if (inst->output_format == FORMAT_422)
>>> +				sizes[0] = inst_format.width * inst_format.height * 2;
>>> +			else
>>> +				sizes[0] = inst_format.width * inst_format.height * 3 / 2;
>>> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
>>> +		} else if (*num_planes == 2) {
>>> +			sizes[0] = inst_format.width * inst_format.height;
>>> +			if (inst->output_format == FORMAT_422)
>>> +				sizes[1] = inst_format.width * inst_format.height;
>>> +			else
>>> +				sizes[1] = inst_format.width * inst_format.height / 2;
>>> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u\n",
>>> +				__func__, sizes[0], sizes[1]);
>>> +		} else if (*num_planes == 3) {
>>> +			sizes[0] = inst_format.width * inst_format.height;
>>> +			if (inst->output_format == FORMAT_422) {
>>> +				sizes[1] = inst_format.width * inst_format.height / 2;
>>> +				sizes[2] = inst_format.width * inst_format.height / 2;
>>> +			} else {
>>> +				sizes[1] = inst_format.width * inst_format.height / 4;
>>> +				sizes[2] = inst_format.width * inst_format.height / 4;
>>> +			}
>>> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u | size[2]: %u\n",
>>> +				__func__, sizes[0], sizes[1], sizes[2]);
>>> +		}
>>> +	}
>>> +
>>> +	if (inst->state == VPU_INST_STATE_INIT_SEQ &&
>>> +	    q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>>> +		if (*num_buffers > inst->dst_buf_count &&
>>> +		    *num_buffers < WAVE5_MAX_FBS)
>>> +			inst->dst_buf_count = *num_buffers;
>>
>> In the create_bufs case, *num_buffers is the number of buffers you are
>> adding to the already existing buffers. Frankly, the logic here is
>> dubious. I'm not sure what the intent is. Why do you need to keep track
>> of the buf count at all when the vb2_queue already does that?
> 
> This needs to be cleaned up. CREATE_BUFS case is not supported for capture, and
> so there is no such weirdo case second calls for that queue at least. Meanwhile,
> inst->dst_buf_count is used a MIN_BUFFERS_FOR_CAPTURE initially, and the number
> of allocated buffer later. I think it would be better to simply store the min,
> and use the queue to track the number of allocated buffers.
> 
> In this diver, the reference frame are stored separately, and compressed. The
> capture queue contains the display frame. There is a gap when comes time to
> transfer timestamp, and for this reason we had to keep the two fbc_count equal.
> We classified this as hardware issue and moved on.
> 
> I think the dst_buf_count can be dropped now and the "*num_buffers > inst-
>> dst_buf_count" not longer make any sense.
> 
>>
>> WAVE5_MAX_FBS == 32, which is VIDEO_MAX_FRAMES. You can just drop the check
>> against WAVE5_MAX_FBS since the core ensures already it will never allocate
>> more than VIDEO_MAX_FRAMES.
>>
>> I'm not sure why WAVE5_MAX_FBS is defined at all, when it is just equal to
>> VIDEO_MAX_FRAMES. Perhaps it can be replaced everywhere with VIDEO_MAX_FRAMES?
> 
> That is more challenging changes, since VIDEO_MAX_FRAMES is a software
> limitation, but WAVE5_MAX_FBS is a hardware limitation. Buffer index only have 4
> bits on this hardware. And the marking of frame being used for display is using
> a 32bit flag. Considering there is effort to lift that software limitation, it
> seems ill advised to completely stop ensuring this HW limit is respected.

If there are only 4 bits for the buffer index, shouldn't WAVE5_MAX_FBS be 16? Or
did you mean '5 bits'? Assuming that you meant '5 bits', then that makes
WAVE5_MAX_FBS identical to VIDEO_MAX_FRAMES, but that is just luck, really.

In any case, you should document at the place where WAVE5_MAX_FBS is defined that
this is a hardware limitation, and not a random software limit.

I also think that the DELETE_BUFS series should allow drivers to set max_num_buffers
to a value less than 32 (currently the requirement is that it is at least 32).

I saw a few more drivers that limit the number of buffers, usually based on the
format (and so the buffer size) and some HW memory limitation. It might be interesting
to allow drivers to change max_num_buffers on the fly (provided no buffers are
allocated, of course). Limit checking is really something that vb2 should do, and
not the driver.

> 
> I'm open for suggestions.
> 
>>
>>> +
>>> +		*num_buffers = inst->dst_buf_count;
>>> +	}
>>> +
>>> +	return 0;
>>> +}

Regards,

	Hans

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer

[Nicolas Dufresne]

Le vendredi 22 septembre 2023 à 09:33 +0200, Hans Verkuil a écrit :
> On 21/09/2023 21:11, Nicolas Dufresne wrote:
> > Le mercredi 20 septembre 2023 à 17:13 +0200, Hans Verkuil a écrit :
> > > On 15/09/2023 23:11, Sebastian Fricke wrote:
> > > > From: Nas Chung <nas.chung@chipsnmedia.com>
> > > > 
> > > > Add the decoder and encoder implementing the v4l2
> > > > API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
> > > > 
> > > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > > > ---
> > > >  drivers/media/platform/chips-media/Kconfig         |    1 +
> > > >  drivers/media/platform/chips-media/Makefile        |    1 +
> > > >  drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
> > > >  drivers/media/platform/chips-media/wave5/Makefile  |   10 +
> > > >  .../platform/chips-media/wave5/wave5-helper.c      |  196 ++
> > > >  .../platform/chips-media/wave5/wave5-helper.h      |   30 +
> > > >  .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1965 ++++++++++++++++++++
> > > >  .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1825 ++++++++++++++++++
> > > >  .../media/platform/chips-media/wave5/wave5-vpu.c   |  331 ++++
> > > >  .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
> > > >  10 files changed, 4454 insertions(+)
> > > > 
> 
> <snip>
> 
> > > > +static int wave5_vpu_dec_set_eos_on_firmware(struct vpu_instance *inst)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = wave5_vpu_dec_update_bitstream_buffer(inst, 0);
> > > > +	if (ret) {
> > > > +		dev_err(inst->dev->dev,
> > > > +			"Setting EOS for the bitstream, fail: %d\n", ret);
> > > 
> > > Is this an error due to a driver problem, or because a bad bitstream is
> > > fed from userspace? In the first case, dev_err would be right, in the
> > > second dev_dbg would be more appropriate. Bad userspace input should not
> > > spam the kernel log in general.
> > 
> > Its the first. To set the EOS flag, a command is sent to the firmware. That
> > command may never return (timeout) or may report an error. For this specific
> > command, if that happens we are likely facing firmware of driver problem (or
> > both).
> 
> OK, I'd add that as a comment here as this is unexpected behavior.
> 
> > 
> > > 
> > > > +		return ret;
> > > > +	}
> > > > +	return 0;
> > > > +}
> 
> <snip>
> 
> > > > +static int wave5_vpu_dec_create_bufs(struct file *file, void *priv,
> > > > +				     struct v4l2_create_buffers *create)
> > > > +{
> > > > +	struct v4l2_format *f = &create->format;
> > > > +
> > > > +	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > > +		return -ENOTTY;
> > > 
> > > Huh? Why is this needed?
> > 
> > Minimally a comment should be added. The why is that we support CREATE_BUF for
> > OUTPUT queue (bitstream) but not for CAPTURE queues. This is simply not
> > supported by Wave5 firmware. Do you have any suggestion how this asymmetry can
> > be implemented better ?
> 
> Certainly not with ENOTTY: the ioctl exists, it is just not supported for
> CAPTURE queues.
> 
> How about -EPERM? And document this error as well in the VIDIOC_CREATE_BUFS
> documentation. And you want a dev_dbg here too.

The suggestion cannot be used since there is documentation for that one already,
and it does not match "unsupported".

"Permission denied. Can be returned if the device needs write permission, or
some special capabilities is needed (e. g. root)"

What about using the most logical error code, which name is actually obvious,
like ENOTSUP ?

   #define ENOTSUPP	524	/* Operation is not supported */

> 
> So I would propose that EPERM is returned if CREATE_BUFS is only supported
> for for one of the two queues of an M2M device.

Note that userspace does not care of the difference between an ioctl not being
implemented at all or not being implement for one queue. GStreamer have been
testing with both queue type for couple of years now. Adding this distinction is
just leaking an implementation details to userspace. I'm fine to just do what
you'd like, just stating the obvious that while it may look logical inside the
kernel, its a bit of a non-sense for our users.

regards,
Nicolas

> 
> > 
> > > 
> > > > +
> > > > +	return v4l2_m2m_ioctl_create_bufs(file, priv, create);
> > > > +}
> 
> <snip>
> 
> > > > +static int wave5_vpu_dec_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
> > > > +				     unsigned int *num_planes, unsigned int sizes[],
> > > > +				     struct device *alloc_devs[])
> > > > +{
> > > > +	struct vpu_instance *inst = vb2_get_drv_priv(q);
> > > > +	struct v4l2_pix_format_mplane inst_format =
> > > > +		(q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ? inst->src_fmt : inst->dst_fmt;
> > > > +	unsigned int i;
> > > > +
> > > > +	dev_dbg(inst->dev->dev, "%s: num_buffers: %u | num_planes: %u | type: %u\n", __func__,
> > > > +		*num_buffers, *num_planes, q->type);
> > > > +
> > > > +	/* the CREATE_BUFS case */
> > > > +	if (*num_planes) {
> > > > +		if (inst_format.num_planes != *num_planes)
> > > > +			return -EINVAL;
> > > > +
> > > > +		for (i = 0; i < *num_planes; i++) {
> > > > +			if (sizes[i] < inst_format.plane_fmt[i].sizeimage)
> > > > +				return -EINVAL;
> > > > +		}
> > > > +	/* the REQBUFS case */
> > > > +	} else {
> > > > +		*num_planes = inst_format.num_planes;
> > > > +
> > > > +		if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > > > +			sizes[0] = inst_format.plane_fmt[0].sizeimage;
> > > > +			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
> > > > +		} else if (*num_planes == 1) {
> > > > +			if (inst->output_format == FORMAT_422)
> > > > +				sizes[0] = inst_format.width * inst_format.height * 2;
> > > > +			else
> > > > +				sizes[0] = inst_format.width * inst_format.height * 3 / 2;
> > > > +			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
> > > > +		} else if (*num_planes == 2) {
> > > > +			sizes[0] = inst_format.width * inst_format.height;
> > > > +			if (inst->output_format == FORMAT_422)
> > > > +				sizes[1] = inst_format.width * inst_format.height;
> > > > +			else
> > > > +				sizes[1] = inst_format.width * inst_format.height / 2;
> > > > +			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u\n",
> > > > +				__func__, sizes[0], sizes[1]);
> > > > +		} else if (*num_planes == 3) {
> > > > +			sizes[0] = inst_format.width * inst_format.height;
> > > > +			if (inst->output_format == FORMAT_422) {
> > > > +				sizes[1] = inst_format.width * inst_format.height / 2;
> > > > +				sizes[2] = inst_format.width * inst_format.height / 2;
> > > > +			} else {
> > > > +				sizes[1] = inst_format.width * inst_format.height / 4;
> > > > +				sizes[2] = inst_format.width * inst_format.height / 4;
> > > > +			}
> > > > +			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u | size[2]: %u\n",
> > > > +				__func__, sizes[0], sizes[1], sizes[2]);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (inst->state == VPU_INST_STATE_INIT_SEQ &&
> > > > +	    q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> > > > +		if (*num_buffers > inst->dst_buf_count &&
> > > > +		    *num_buffers < WAVE5_MAX_FBS)
> > > > +			inst->dst_buf_count = *num_buffers;
> > > 
> > > In the create_bufs case, *num_buffers is the number of buffers you are
> > > adding to the already existing buffers. Frankly, the logic here is
> > > dubious. I'm not sure what the intent is. Why do you need to keep track
> > > of the buf count at all when the vb2_queue already does that?
> > 
> > This needs to be cleaned up. CREATE_BUFS case is not supported for capture, and
> > so there is no such weirdo case second calls for that queue at least. Meanwhile,
> > inst->dst_buf_count is used a MIN_BUFFERS_FOR_CAPTURE initially, and the number
> > of allocated buffer later. I think it would be better to simply store the min,
> > and use the queue to track the number of allocated buffers.
> > 
> > In this diver, the reference frame are stored separately, and compressed. The
> > capture queue contains the display frame. There is a gap when comes time to
> > transfer timestamp, and for this reason we had to keep the two fbc_count equal.
> > We classified this as hardware issue and moved on.
> > 
> > I think the dst_buf_count can be dropped now and the "*num_buffers > inst-
> > > dst_buf_count" not longer make any sense.
> > 
> > > 
> > > WAVE5_MAX_FBS == 32, which is VIDEO_MAX_FRAMES. You can just drop the check
> > > against WAVE5_MAX_FBS since the core ensures already it will never allocate
> > > more than VIDEO_MAX_FRAMES.
> > > 
> > > I'm not sure why WAVE5_MAX_FBS is defined at all, when it is just equal to
> > > VIDEO_MAX_FRAMES. Perhaps it can be replaced everywhere with VIDEO_MAX_FRAMES?
> > 
> > That is more challenging changes, since VIDEO_MAX_FRAMES is a software
> > limitation, but WAVE5_MAX_FBS is a hardware limitation. Buffer index only have 4
> > bits on this hardware. And the marking of frame being used for display is using
> > a 32bit flag. Considering there is effort to lift that software limitation, it
> > seems ill advised to completely stop ensuring this HW limit is respected.
> 
> If there are only 4 bits for the buffer index, shouldn't WAVE5_MAX_FBS be 16? Or
> did you mean '5 bits'? Assuming that you meant '5 bits', then that makes
> WAVE5_MAX_FBS identical to VIDEO_MAX_FRAMES, but that is just luck, really.
> 
> In any case, you should document at the place where WAVE5_MAX_FBS is defined that
> this is a hardware limitation, and not a random software limit.
> 
> I also think that the DELETE_BUFS series should allow drivers to set max_num_buffers
> to a value less than 32 (currently the requirement is that it is at least 32).
> 
> I saw a few more drivers that limit the number of buffers, usually based on the
> format (and so the buffer size) and some HW memory limitation. It might be interesting
> to allow drivers to change max_num_buffers on the fly (provided no buffers are
> allocated, of course). Limit checking is really something that vb2 should do, and
> not the driver.
> 
> > 
> > I'm open for suggestions.
> > 
> > > 
> > > > +
> > > > +		*num_buffers = inst->dst_buf_count;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> 
> Regards,
> 
> 	Hans


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer

[Hans Verkuil]

On 27/09/2023 01:29, Nicolas Dufresne wrote:
> Le vendredi 22 septembre 2023 à 09:33 +0200, Hans Verkuil a écrit :
>> On 21/09/2023 21:11, Nicolas Dufresne wrote:
>>> Le mercredi 20 septembre 2023 à 17:13 +0200, Hans Verkuil a écrit :
>>>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>>>> From: Nas Chung <nas.chung@chipsnmedia.com>
>>>>>
>>>>> Add the decoder and encoder implementing the v4l2
>>>>> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
>>>>>
>>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
>>>>> ---
>>>>>  drivers/media/platform/chips-media/Kconfig         |    1 +
>>>>>  drivers/media/platform/chips-media/Makefile        |    1 +
>>>>>  drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
>>>>>  drivers/media/platform/chips-media/wave5/Makefile  |   10 +
>>>>>  .../platform/chips-media/wave5/wave5-helper.c      |  196 ++
>>>>>  .../platform/chips-media/wave5/wave5-helper.h      |   30 +
>>>>>  .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1965 ++++++++++++++++++++
>>>>>  .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1825 ++++++++++++++++++
>>>>>  .../media/platform/chips-media/wave5/wave5-vpu.c   |  331 ++++
>>>>>  .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
>>>>>  10 files changed, 4454 insertions(+)
>>>>>
>>
>> <snip>
>>
>>>>> +static int wave5_vpu_dec_set_eos_on_firmware(struct vpu_instance *inst)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = wave5_vpu_dec_update_bitstream_buffer(inst, 0);
>>>>> +	if (ret) {
>>>>> +		dev_err(inst->dev->dev,
>>>>> +			"Setting EOS for the bitstream, fail: %d\n", ret);
>>>>
>>>> Is this an error due to a driver problem, or because a bad bitstream is
>>>> fed from userspace? In the first case, dev_err would be right, in the
>>>> second dev_dbg would be more appropriate. Bad userspace input should not
>>>> spam the kernel log in general.
>>>
>>> Its the first. To set the EOS flag, a command is sent to the firmware. That
>>> command may never return (timeout) or may report an error. For this specific
>>> command, if that happens we are likely facing firmware of driver problem (or
>>> both).
>>
>> OK, I'd add that as a comment here as this is unexpected behavior.
>>
>>>
>>>>
>>>>> +		return ret;
>>>>> +	}
>>>>> +	return 0;
>>>>> +}
>>
>> <snip>
>>
>>>>> +static int wave5_vpu_dec_create_bufs(struct file *file, void *priv,
>>>>> +				     struct v4l2_create_buffers *create)
>>>>> +{
>>>>> +	struct v4l2_format *f = &create->format;
>>>>> +
>>>>> +	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>>> +		return -ENOTTY;
>>>>
>>>> Huh? Why is this needed?
>>>
>>> Minimally a comment should be added. The why is that we support CREATE_BUF for
>>> OUTPUT queue (bitstream) but not for CAPTURE queues. This is simply not
>>> supported by Wave5 firmware. Do you have any suggestion how this asymmetry can
>>> be implemented better ?
>>
>> Certainly not with ENOTTY: the ioctl exists, it is just not supported for
>> CAPTURE queues.
>>
>> How about -EPERM? And document this error as well in the VIDIOC_CREATE_BUFS
>> documentation. And you want a dev_dbg here too.
> 
> The suggestion cannot be used since there is documentation for that one already,
> and it does not match "unsupported".
> 
> "Permission denied. Can be returned if the device needs write permission, or
> some special capabilities is needed (e. g. root)"
> 
> What about using the most logical error code, which name is actually obvious,
> like ENOTSUP ?
> 
>    #define ENOTSUPP	524	/* Operation is not supported */
> 

Let's go with EOPNOTSUPP. That seems to be the more commonly used error
code in drivers.

>>
>> So I would propose that EPERM is returned if CREATE_BUFS is only supported
>> for for one of the two queues of an M2M device.
> 
> Note that userspace does not care of the difference between an ioctl not being
> implemented at all or not being implement for one queue. GStreamer have been
> testing with both queue type for couple of years now. Adding this distinction is
> just leaking an implementation details to userspace. I'm fine to just do what
> you'd like, just stating the obvious that while it may look logical inside the
> kernel, its a bit of a non-sense for our users.

I don't agree with that. If an ioctl returns ENOTTY, then userspace can be certain
that that ioctl is not implemented for the given file descriptor. That's not the case
here: it is implemented, the operation is just not supported for one of the queues.

Regards,

	Hans

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer

[Deborah Brouwer]

On Wed, Sep 27, 2023 at 09:19:46AM +0200, Hans Verkuil wrote:
> On 27/09/2023 01:29, Nicolas Dufresne wrote:
> > Le vendredi 22 septembre 2023 à 09:33 +0200, Hans Verkuil a écrit :
> >> On 21/09/2023 21:11, Nicolas Dufresne wrote:
> >>> Le mercredi 20 septembre 2023 à 17:13 +0200, Hans Verkuil a écrit :
> >>>> On 15/09/2023 23:11, Sebastian Fricke wrote:
> >>>>> From: Nas Chung <nas.chung@chipsnmedia.com>
> >>>>>
> >>>>> Add the decoder and encoder implementing the v4l2
> >>>>> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
> >>>>>
> >>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> >>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> >>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>>>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> >>>>> ---
> >>>>>  drivers/media/platform/chips-media/Kconfig         |    1 +
> >>>>>  drivers/media/platform/chips-media/Makefile        |    1 +
> >>>>>  drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
> >>>>>  drivers/media/platform/chips-media/wave5/Makefile  |   10 +
> >>>>>  .../platform/chips-media/wave5/wave5-helper.c      |  196 ++
> >>>>>  .../platform/chips-media/wave5/wave5-helper.h      |   30 +
> >>>>>  .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1965 ++++++++++++++++++++
> >>>>>  .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1825 ++++++++++++++++++
> >>>>>  .../media/platform/chips-media/wave5/wave5-vpu.c   |  331 ++++
> >>>>>  .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
> >>>>>  10 files changed, 4454 insertions(+)
> >>>>>
> >>
> >> <snip>
> >>
> >>>>> +static int wave5_vpu_dec_set_eos_on_firmware(struct vpu_instance *inst)
> >>>>> +{
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	ret = wave5_vpu_dec_update_bitstream_buffer(inst, 0);
> >>>>> +	if (ret) {
> >>>>> +		dev_err(inst->dev->dev,
> >>>>> +			"Setting EOS for the bitstream, fail: %d\n", ret);
> >>>>
> >>>> Is this an error due to a driver problem, or because a bad bitstream is
> >>>> fed from userspace? In the first case, dev_err would be right, in the
> >>>> second dev_dbg would be more appropriate. Bad userspace input should not
> >>>> spam the kernel log in general.
> >>>
> >>> Its the first. To set the EOS flag, a command is sent to the firmware. That
> >>> command may never return (timeout) or may report an error. For this specific
> >>> command, if that happens we are likely facing firmware of driver problem (or
> >>> both).
> >>
> >> OK, I'd add that as a comment here as this is unexpected behavior.
> >>
> >>>
> >>>>
> >>>>> +		return ret;
> >>>>> +	}
> >>>>> +	return 0;
> >>>>> +}
> >>
> >> <snip>
> >>
> >>>>> +static int wave5_vpu_dec_create_bufs(struct file *file, void *priv,
> >>>>> +				     struct v4l2_create_buffers *create)
> >>>>> +{
> >>>>> +	struct v4l2_format *f = &create->format;
> >>>>> +
> >>>>> +	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>>> +		return -ENOTTY;
> >>>>
> >>>> Huh? Why is this needed?
> >>>
> >>> Minimally a comment should be added. The why is that we support CREATE_BUF for
> >>> OUTPUT queue (bitstream) but not for CAPTURE queues. This is simply not
> >>> supported by Wave5 firmware. Do you have any suggestion how this asymmetry can
> >>> be implemented better ?
> >>
> >> Certainly not with ENOTTY: the ioctl exists, it is just not supported for
> >> CAPTURE queues.
> >>
> >> How about -EPERM? And document this error as well in the VIDIOC_CREATE_BUFS
> >> documentation. And you want a dev_dbg here too.
> > 
> > The suggestion cannot be used since there is documentation for that one already,
> > and it does not match "unsupported".
> > 
> > "Permission denied. Can be returned if the device needs write permission, or
> > some special capabilities is needed (e. g. root)"
> > 
> > What about using the most logical error code, which name is actually obvious,
> > like ENOTSUP ?
> > 
> >    #define ENOTSUPP	524	/* Operation is not supported */
> > 
> 
> Let's go with EOPNOTSUPP. That seems to be the more commonly used error
> code in drivers.

Hi Hans,

Sorry to belabour this issue but when I change the return value
to EOPNOTSUPP, it now causes v4l2-compliance to fail because
v4l2-test-buffers.cpp expects ENOTTY if CREATE_BUFS is not supported.

We didn't get this warning before because there was a typo in the
buffer check and it was only checking for single-planar buffers.

How would you prefer to handle this? The options seem like
keep ENOTTY in this driver or
patch v4l2-compliance to warn if it also receives EOPNOTSUPP?

> 
> >>
> >> So I would propose that EPERM is returned if CREATE_BUFS is only supported
> >> for for one of the two queues of an M2M device.
> > 
> > Note that userspace does not care of the difference between an ioctl not being
> > implemented at all or not being implement for one queue. GStreamer have been
> > testing with both queue type for couple of years now. Adding this distinction is
> > just leaking an implementation details to userspace. I'm fine to just do what
> > you'd like, just stating the obvious that while it may look logical inside the
> > kernel, its a bit of a non-sense for our users.
> 
> I don't agree with that. If an ioctl returns ENOTTY, then userspace can be certain
> that that ioctl is not implemented for the given file descriptor. That's not the case
> here: it is implemented, the operation is just not supported for one of the queues.
> 
> Regards,
> 
> 	Hans

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer

[Hans Verkuil]

Hi Deb,

On 03/10/2023 01:51, Deborah Brouwer wrote:
> On Wed, Sep 27, 2023 at 09:19:46AM +0200, Hans Verkuil wrote:
>> On 27/09/2023 01:29, Nicolas Dufresne wrote:
>>> Le vendredi 22 septembre 2023 à 09:33 +0200, Hans Verkuil a écrit :
>>>> On 21/09/2023 21:11, Nicolas Dufresne wrote:
>>>>> Le mercredi 20 septembre 2023 à 17:13 +0200, Hans Verkuil a écrit :
>>>>>> On 15/09/2023 23:11, Sebastian Fricke wrote:
>>>>>>> From: Nas Chung <nas.chung@chipsnmedia.com>
>>>>>>>
>>>>>>> Add the decoder and encoder implementing the v4l2
>>>>>>> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
>>>>>>>
>>>>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>>>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
>>>>>>> ---
>>>>>>>  drivers/media/platform/chips-media/Kconfig         |    1 +
>>>>>>>  drivers/media/platform/chips-media/Makefile        |    1 +
>>>>>>>  drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
>>>>>>>  drivers/media/platform/chips-media/wave5/Makefile  |   10 +
>>>>>>>  .../platform/chips-media/wave5/wave5-helper.c      |  196 ++
>>>>>>>  .../platform/chips-media/wave5/wave5-helper.h      |   30 +
>>>>>>>  .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1965 ++++++++++++++++++++
>>>>>>>  .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1825 ++++++++++++++++++
>>>>>>>  .../media/platform/chips-media/wave5/wave5-vpu.c   |  331 ++++
>>>>>>>  .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
>>>>>>>  10 files changed, 4454 insertions(+)
>>>>>>>
>>>>
>>>> <snip>
>>>>
>>>>>>> +static int wave5_vpu_dec_set_eos_on_firmware(struct vpu_instance *inst)
>>>>>>> +{
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	ret = wave5_vpu_dec_update_bitstream_buffer(inst, 0);
>>>>>>> +	if (ret) {
>>>>>>> +		dev_err(inst->dev->dev,
>>>>>>> +			"Setting EOS for the bitstream, fail: %d\n", ret);
>>>>>>
>>>>>> Is this an error due to a driver problem, or because a bad bitstream is
>>>>>> fed from userspace? In the first case, dev_err would be right, in the
>>>>>> second dev_dbg would be more appropriate. Bad userspace input should not
>>>>>> spam the kernel log in general.
>>>>>
>>>>> Its the first. To set the EOS flag, a command is sent to the firmware. That
>>>>> command may never return (timeout) or may report an error. For this specific
>>>>> command, if that happens we are likely facing firmware of driver problem (or
>>>>> both).
>>>>
>>>> OK, I'd add that as a comment here as this is unexpected behavior.
>>>>
>>>>>
>>>>>>
>>>>>>> +		return ret;
>>>>>>> +	}
>>>>>>> +	return 0;
>>>>>>> +}
>>>>
>>>> <snip>
>>>>
>>>>>>> +static int wave5_vpu_dec_create_bufs(struct file *file, void *priv,
>>>>>>> +				     struct v4l2_create_buffers *create)
>>>>>>> +{
>>>>>>> +	struct v4l2_format *f = &create->format;
>>>>>>> +
>>>>>>> +	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>>>>> +		return -ENOTTY;
>>>>>>
>>>>>> Huh? Why is this needed?
>>>>>
>>>>> Minimally a comment should be added. The why is that we support CREATE_BUF for
>>>>> OUTPUT queue (bitstream) but not for CAPTURE queues. This is simply not
>>>>> supported by Wave5 firmware. Do you have any suggestion how this asymmetry can
>>>>> be implemented better ?
>>>>
>>>> Certainly not with ENOTTY: the ioctl exists, it is just not supported for
>>>> CAPTURE queues.
>>>>
>>>> How about -EPERM? And document this error as well in the VIDIOC_CREATE_BUFS
>>>> documentation. And you want a dev_dbg here too.
>>>
>>> The suggestion cannot be used since there is documentation for that one already,
>>> and it does not match "unsupported".
>>>
>>> "Permission denied. Can be returned if the device needs write permission, or
>>> some special capabilities is needed (e. g. root)"
>>>
>>> What about using the most logical error code, which name is actually obvious,
>>> like ENOTSUP ?
>>>
>>>    #define ENOTSUPP	524	/* Operation is not supported */
>>>
>>
>> Let's go with EOPNOTSUPP. That seems to be the more commonly used error
>> code in drivers.
> 
> Hi Hans,
> 
> Sorry to belabour this issue but when I change the return value
> to EOPNOTSUPP, it now causes v4l2-compliance to fail because
> v4l2-test-buffers.cpp expects ENOTTY if CREATE_BUFS is not supported.
> 
> We didn't get this warning before because there was a typo in the
> buffer check and it was only checking for single-planar buffers.
> 
> How would you prefer to handle this? The options seem like
> keep ENOTTY in this driver or
> patch v4l2-compliance to warn if it also receives EOPNOTSUPP?

You patch v4l2-compliance. It makes sense: we're making a uAPI modification,
so that implies changes to v4l2-compliance.

So v4l2-compliance needs to understand EOPNOTSUPP for CREATE_BUFS: if it is
returned it has to check that it is used correctly: so there has to be at
least one buffer type for which CREATE_BUFS actually works. In other words,
v4l2-compliance must check that EOPNOTSUPP isn't used as a replacement
for ENOTTY.

This can be done in testReqBufs().

Regards,

	Hans

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel