Hello Laurent, Antti, Hans, > Subject: Re: [RFC 5/5] doc_rst: media: New SDR formats SC16, SC18 & SC20 > > On 11/11/2016 02:57 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > On Friday 11 Nov 2016 14:53:58 Hans Verkuil wrote: > >> On 11/10/2016 09:08 AM, Laurent Pinchart wrote: > >>> Antti, Hans, ping ? Please see below. > >>> > >>> On Friday 04 Nov 2016 09:23:29 Ramesh Shanmugasundaram wrote: > >>>>> On 11/02/2016 10:58 PM, Laurent Pinchart wrote: > >>>>>> On Wednesday 02 Nov 2016 09:00:00 Ramesh Shanmugasundaram wrote: > >>>>>>>>> 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 > >>>>>>> > >>>>>>> [snip] > >>>>>>> > >>>>>>>>>> + > >>>>>>>>>> + - 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. > >>>>>> > >>>>>> If the formats are really device-specific then they should be > >>>>>> documented accordingly and not made generic. > >>>>>> > >>>>>>>> 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)? > >>>>>> > >>>>>> I have no answer to that question as I'm not familiar with SDR. > >>>>>> Antti, you've added v4l2_sdr_format to the API, what's your > >>>>>> opinion ? Hans, as you've acked the patch, your input would be > appreciated as well. > >>>>> > >>>>> If I understood correctly this hardware provides I and Q samples > >>>>> via different channels and driver now combines those channels as a > >>>>> sequential IQ sample pairs. > >>>> > >>>> The driver combines the two buffer ptrs and present as one single > buffer. > >>>> For a buffer of size 200 > >>>> > >>>> ptr + 0 : I I I I ... I > >>>> ptr + 100 : Q Q Q Q ... Q > >>>> > >>>>> I have never seen any other than hw which provides IQ IQ IQ IQ ... > IQ. > >>>> > >>>> There are some modes where this h/w combo can also do IQ IQ IQ > pattern. > >>>> Those modes are not added in the RFC patchset. > >>>> > >>>>> This is > >>>>> I I I I ... I > >>>>> Q Q Q Q ... Q > >>>>> I am not very familiar with planars, but it sounds like it is > >>>>> correct approach. So I think should be added rather than emulate > >>>>> packet sequential format. > >>>> > >>>> My understanding of V4L2 MPLANE constructs is limited to a quick > >>>> code read only. At this point MPLANE support seems specific to > >>>> video. SDR is defined as separate format like v4l2_pix_format. > >>>> Questions would be - should we define new SDR_MPLANE? or merge SDR > >>>> format with pix format & reuse existing MPLANE with some SDR > >>>> extensions (if possible)? These seem big design decisions. Any > >>>> suggestions please? > >>>> > >>>> For my use case, MPLANE support does not seem to add significant > >>>> benefit except it may be syntactically correct. I am doing cyclic > >>>> DMA with a small set of h/w buffers and copying each stage to one > >>>> mmapped vmalloc vb2_buffer at two offsets. If we add MPLANE > >>>> support, it can be two non-contiguous buffer pointers. > >>>> > >>>>>>>> - 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. > >> > >> Some terminology first: > >> > >> Planar formats separate the data into different memory areas: in this > >> case one part is all I and one part is all Q. This as opposed to > >> interleaved formats (IQIQIQIQ....). > >> > >> As long as both planes fit in the same buffer all is fine. Since that > >> is the case here there is no need to introduce a new MPLANE API. > >> > >> The MPLANE API was added for video to handle cases where the two > >> planes had to be in two different non-contiguous buffers. > > > > Not only that, it can also be used for cases where storing the two > > planes in separate buffers can be beneficial, even if a single > > contiguous buffer could work. > > > >> So instead of passing one buffer pointer, you need to pass two or > >> more buffer pointers. > >> > >> In hindsight we should have called it the MBUFFER API. > > > > The name was badly chosen, yes. > > > >> Oh well... > >> > >> Anyway, since there is no problem here apparently to keep both planes > >> in one buffer there is also no need to introduce a SDR_MPLANE. > > > > The question here is whether there could be a benefit in separating I > > and Q data in two buffers compared to storing them in the same buffer. > > > > The MPLANE API is very messy and introducing something like SDR_MPLANE is > not something I would promote. If we want that, then we should first make > a new v4l2_buffer struct that simplifies MPLANE handling (we discussed > that before). Thank you for the comments and closure on this topic. Thanks, Ramesh -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
