RE: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-Car devices

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

Hi Laurent,

Thanks for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 02 March 2021 12:32
> Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> Car devices
> 
> Hi Fabrizio,
> 
> On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote:
> > On 02 March 2021 11:17, Ezequiel Garcia wrote:
> > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote:
> > > > On 26 February 2021 10:38, Arnd Bergmann wrote:
> > > > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro wrote:
> > > > > >
> > > > > > The DAB hardware accelerator found on R-Car E3 and R-Car M3-N
> devices is
> > > > > > a hardware accelerator for software DAB demodulators.
> > > > > > It consists of one FFT (Fast Fourier Transform) module and one
> decoder
> > > > > > module, compatible with DAB specification (ETSI EN 300 401 and
> > > > > > ETSI TS 102 563).
> > > > > > The decoder module can perform FIC decoding and MSC decoding
> processing
> > > > > > from de-puncture to final decoded result.
> > > > > >
> > > > > > This patch adds a device driver to support the FFT module only.
> > > > > >
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > > ---
> > > > > >  MAINTAINERS                      |   7 ++
> > > > > >  drivers/misc/Kconfig             |   1 +
> > > > > >  drivers/misc/Makefile            |   1 +
> > > > > >  drivers/misc/rcar_dab/Kconfig    |  11 ++
> > > > > >  drivers/misc/rcar_dab/Makefile   |   8 ++
> > > > > >  drivers/misc/rcar_dab/rcar_dev.c | 176
> +++++++++++++++++++++++++++++++
> > > > > >  drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++
> > > > > >  drivers/misc/rcar_dab/rcar_fft.c | 160
> ++++++++++++++++++++++++++++
> > > > > >  include/uapi/linux/rcar_dab.h    |  35 ++++++
> > > > >
> > > > > Can you explain why this is not in drivers/media/?
> > > >
> > > > I wasn't entirely sure were to put it to be honest as I couldn't
> find
> > > > anything similar, that's why "misc" for v1, to mainly get a feedback
> > > > and advice.
> > > >
> > > > > I don't think we want a custom ioctl interface for a device that
> > > > > implements
> > > > > a generic specification. My first feeling would be that this
> should not
> > > > > have a user-level API but instead get called by the DAB radio
> driver.
> > > >
> > > > I hear you, the trouble is that the modules of this IP should be
> seen
> > > > as part of a SW and HW processing pipeline.
> > > > Some of the SW components of the pipeline can be proprietary, and
> > > > unfortunately we don't have access (yet) to a framework that allows
> us to
> > > > test and demonstrate the whole pipeline, for the moment we can only
> test
> > > > things in isolation. It'll take us a while to come up with a full
> solution
> > > > (or to have access to one), and in the meantime our SoCs come with
> an IP
> > > > that can't be used because there is no driver for it (yet).
> > > >
> > > > After discussing things further with Geert and Laurent, I think they
> > > > are right in saying that the best path for this is probably to add
> this
> > > > driver under "drivers/staging" as an interim solution, so that the
> IP will
> > > > be accessible by developers, and when we have everything we need (a
> fully
> > > > fledged processing framework), we will able to integrate it better
> with
> > > > the other components (if possible).
> > > >
> > > > > What is the intended usage model here? I assume the idea is to
> > > > > use it in an application that receives audio or metadata from DAB.
> > > > > What driver do you use for that?
> > > >
> > > > This IP is similar to a DMA to some extent, in the sense that it
> takes
> > > > input data from a buffer in memory and it writes output data onto a
> buffer
> > > > in memory, and of course it does some processing in between.
> > >
> > > That sounds like something that can fit V4L2 MEM2MEM driver.
> > > You queue two buffers and an operation, and then run a job.
> >
> > V4L2 MEM2MEM seems good for this in general, however the DAB IP is going
> > to be shared by multiple processing pipelines (as usually we have
> several
> > DRIF interfaces, and only 1 DAB IP), and for what concerns FFT
> specifically
> > there is only 1 FFT module inside the DAB IP.
> > My understanding is that the capabilities of V4L2 MEM2MEM devices are
> > configured for the specific pipeline, but in the this context user space
> > would have to continuously switch the capabilities of the DAB IP (at the
> > right moment) to allow processing for data streams requiring different
> > capabilities.
> >
> > Am I wrong?
> 
> V4L2 M2M devices can be opened multiple times, but different processes,
> which can configure the device in different ways, and submit jobs that
> are then scheduled by the V4L2 M2M framework.

I'll give it a try in due time then.

I think the clock related patches are worth merging.

Thanks,
Fab

> 
> > > > It's not directly connected to any other Radio related IP.
> > > > The user space application can read DAB IQ samples from the R-Car
> DRIF
> > > > interface (via driver drivers/media/platform/rcar_drif.c, or from
> other
> > > > sources since this IP is decoupled from DRIF), and then when it's
> time
> > > > to accelerate FFT, FIC, or MSC, it can request services to the DAB
> IP, so
> > > > that the app can go on and use the processor to do some work, while
> the DAB
> > > > IP processes things.
> > > > A framework to connect and exchange processing blocks (either SW or
> HW) and
> > > > interfaces is therefore vital to properly place a kernel
> implementation
> > > > in the great scheme of things, in its absence a simple driver can
> help
> > >
> > > I'm not entirely sure we are missing a framework. What's missing in
> > > V4L2 MEM2MEM?
> >
> > I was referring to a user space framework (I should have been more
> specific
> > with my previous email).
> >
> > > Before considering drivers/staging, it would be interesting to figure
> > > out if V4L2 can do it as-is, and if not, discuss what's missing.
> >
> > I think an interim solution would allow us and users to evaluate things
> a
> > little bit better, so that we can integrate this IP with V4L2 later on.
> 
> --
> Regards,
> 
> Laurent Pinchart