Hi Laurent,
Thank you for the review comments.
> On Wednesday 12 Oct 2016 15:10:29 Ramesh Shanmugasundaram wrote:
> > This patch adds documentation for the three new SDR formats
> >
> > V4L2_SDR_FMT_SCU16BE
> > V4L2_SDR_FMT_SCU18BE
> > V4L2_SDR_FMT_SCU20BE
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@xxxxxxxxxxxxxx> ---
> > .../media/uapi/v4l/pixfmt-sdr-scu16be.rst | 44
> ++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-sdr-scu18be.rst | 48
> +++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-sdr-scu20be.rst | 48
> +++++++++++++++++++
> > Documentation/media/uapi/v4l/sdr-formats.rst | 3 ++
> > 4 files changed, 143 insertions(+)
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-scu18be.rst
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-scu20be.rst
> >
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst new file mode
> > 100644 index 0000000..d6c2123
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-scu16be.rst
> > @@ -0,0 +1,44 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2-SDR-FMT-SCU16BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_SCU16BE ('SCU16')
>
> The value between parentheses is the ASCII representation of the 4CC, it
> should be SC16. Same comment for the other formats.
Agreed. I corrected it after I sent the patch :-(.
>
> > +******************************
> > +
> > +Sliced complex unsigned 16-bit big endian IQ sample
> > +
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 16 bit unsigned big endian
> > +number. I value starts first and Q value starts at an offset
> > +equalling half of the buffer size. 14 bit data is stored in 16 bit
> > +space with unused stuffed bits padded with 0.
>
> Please specify here how the 14-bit numbers are aligned (i.e. padding in
> bits
> 15:14 or bits 1:0 or any other strange option). Same comment for the other
> formats.
You are right. Actually the representation would be something like below. I will correct this for all the 3 formats. Thanks.
<------------------------32bits---------------------->
<--14 bit data + 2bit status---- 16bit padded zeros-->
<--14 bit data + 2bit status---- 16bit padded zeros-->
>
> > +
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +
> > +.. flat-table::
> > + :header-rows: 0
> > + :stub-columns: 0
> > +
> > + - .. row 1
>
> Please use the more compact table stable
Agreed.
>
> * - start + 0:
> - I'\ :sub:`0[D13:D6]`
> ...
>
> Same comment for the other formats.
Agreed.
>
> > +
> > + - start + 0:
> > +
> > + - I'\ :sub:`0[D13:D6]`
> > +
> > + - I'\ :sub:`0[D5:D0]`
> > +
> > + - .. row 2
> > +
> > + - start + buffer_size/2:
> > +
> > + - Q'\ :sub:`0[D13:D6]`
> > +
> > + - Q'\ :sub:`0[D5:D0]`
>
> The format looks planar, does it use one V4L2 plane (as does NV12) or two
> V4L2 planes (as does NV12M) ? Same question for the other formats.
Thank you for bringing up this topic. This is one of the key design dilemma.
The I & Q data for these three SDR formats comes from two different DMA channels and hence two separate pointers -> we could say it is v4l2 multi-planar. Right now, I am making it look like a single plane by presenting the data in one single buffer ptr.
For e.g. multi-planar SC16 format would look something like this
<------------------------32bits---------------------->
<--I(14 bit data) + 2bit status--16bit padded zeros--> : start0 + 0
<--I(14 bit data) + 2bit status--16bit padded zeros--> : start0 + 4
...
<--Q(14 bit data) + 2bit status--16bit padded zeros--> : start1 + 0
<--Q(14 bit data) + 2bit status--16bit padded zeros--> : start1 + 4
My concerns are
1) These formats are not a standard as the video "Image Formats". These formats are possible when we use DRIF + MAX2175 combination. If we interface with a different tuner vendor, the above format(s) MAY/MAY NOT be re-usable. We do not know at this point. This is the main open item for discussion in the cover letter.
2) MPLANE support within V4L2 seems specific to video. Please correct me if this is wrong interpretation.
- struct v4l2_format contains v4l2_sdr_format and v4l2_pix_format_mplane as members of union. Should I create a new v4l2_sdr_format_mplane? If I have to use v4l2_pix_format_mplane most of the video specific members would be unused (it would be similar to using v4l2_pix_format itself instead of v4l2_sdr_format)?
- The above decision (accomodate SDR & MPLANE) needs to be propagated across the framework. Is this the preferred approach?
It goes back to point (1). As of today, the change set for this combo (DRIF+MAX2175) introduces new SDR formats only. Should it add further SDR+MPLANE support to the framework as well?
I would appreciate your suggestions on this regard.
>
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-scu18be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-scu18be.rst new file mode
> > 100644 index 0000000..e6e0aff
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-scu18be.rst
> > @@ -0,0 +1,48 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2-SDR-FMT-SCU18BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_SCU18BE ('SCU18')
> > +******************************
> > +
> > +Sliced complex unsigned 18-bit big endian IQ sample
> > +
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 18 bit unsigned big endian
> > +number. I value starts first and Q value starts at an offset
> > +equalling half of the buffer size. 16 bit data is stored in 18 bit
> > +space with unused stuffed bits padded with 0.
>
> Your example below suggests that 18 bit data is stored in 24 bits. Similar
> comment for SCU20.
Agreed. The corrected representation is as I mentioned in the earlier comment.
Thanks,
Ramesh
