Re: [PATCH v3 08/10] media: hantro: add initial i.MX8MQ support

[Adam Ford <aford173@gmail.com>]

On Tue, Jun 11, 2019 at 7:26 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Tue, 2019-06-04 at 12:42 +0200, Hans Verkuil wrote:
> > On 6/3/19 10:02 PM, Boris Brezillon wrote:
> > > On Mon, 3 Jun 2019 14:45:37 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > >
> > > > On 5/31/19 10:55 AM, Philipp Zabel wrote:
> > > > > For now this just enables MPEG-2 decoding on the Hantro G1 on i.MX8MQ.
> > > > >
> > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > ---
> > > > > Changes since v2 [1]:
> > > > >  - Adapted to changes in patches 4 and 5
> > > > >
> > > > > [1] https://patchwork.linuxtv.org/patch/56420/
> > > > > ---
> > > > >  drivers/staging/media/hantro/Kconfig        |   8 +-
> > > > >  drivers/staging/media/hantro/Makefile       |   1 +
> > > > >  drivers/staging/media/hantro/hantro_drv.c   |   1 +
> > > > >  drivers/staging/media/hantro/hantro_hw.h    |   1 +
> > > > >  drivers/staging/media/hantro/imx8m_vpu_hw.c | 171 ++++++++++++++++++++
> > > > >  5 files changed, 178 insertions(+), 4 deletions(-)
> > > > >  create mode 100644 drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > >
> > > > > diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig
> > > > > index 660cca358f04..6fdb72df7bd3 100644
> > > > > --- a/drivers/staging/media/hantro/Kconfig
> > > > > +++ b/drivers/staging/media/hantro/Kconfig
> > > > > @@ -1,15 +1,15 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > >  config VIDEO_HANTRO
> > > > >         tristate "Hantro VPU driver"
> > > > > -       depends on ARCH_ROCKCHIP || COMPILE_TEST
> > > > > +       depends on ARCH_MXC || ARCH_ROCKCHIP || COMPILE_TEST
> > > > >         depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER
> > > > >         depends on MEDIA_CONTROLLER_REQUEST_API
> > > > >         select VIDEOBUF2_DMA_CONTIG
> > > > >         select VIDEOBUF2_VMALLOC
> > > > >         select V4L2_MEM2MEM_DEV
> > > > >         help
> > > > > -         Support for the Hantro IP based Video Processing Unit present on
> > > > > -         Rockchip SoC, which accelerates video and image encoding and
> > > > > -         decoding.
> > > > > +         Support for the Hantro IP based Video Processing Units present on
> > > > > +         Rockchip and NXP i.MX8M SoCs, which accelerate video and image
> > > > > +         encoding and decoding.
> > > > >           To compile this driver as a module, choose M here: the module
> > > > >           will be called hantro-vpu.
> > > > > diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile
> > > > > index 14f17a4e48cb..1dac16af451e 100644
> > > > > --- a/drivers/staging/media/hantro/Makefile
> > > > > +++ b/drivers/staging/media/hantro/Makefile
> > > > > @@ -9,5 +9,6 @@ hantro-vpu-y += \
> > > > >                 rk3399_vpu_hw.o \
> > > > >                 rk3399_vpu_hw_jpeg_enc.o \
> > > > >                 rk3399_vpu_hw_mpeg2_dec.o \
> > > > > +               imx8m_vpu_hw.o \
> > > > >                 hantro_jpeg.o \
> > > > >                 hantro_mpeg2.o
> > > >
> > > > I'm a bit concerned about how this is organized. As far as I can tell,
> > > > enabling this driver would compile both rockchip and imx8 code into the
> > > > same driver. You would expect that only the code for the selected
> > > > architectures would be compiled in (or all if COMPILE_TEST is set, of course).
> > > >
> > > > Can you take a look at this?
> > >
> > > Shouldn't be hard to do:
> > >
> > > config VIDEO_HANTRO
> > >     tristate "Hantro VPU driver"
> > >     ...
> > >
> > > config VIDEO_HANTRO_ROCKCHIP
> > >     bool "Rockchip Hantro VPU driver"
> > >     depends on ARCH_ROCKCHIP || COMPILE_TEST
> > >     depends on VIDEO_HANTRO
> > >     ...
> > >
> > > config VIDEO_HANTRO_IMX8
> > >     bool "IMX8 Hantro VPU driver"
> > >     depends on ARCH_IMX || COMPILE_TEST
> > >     depends on VIDEO_HANTRO
> > >     ...
> > >
> > > hantro-vpu-$(VIDEO_HANTRO_RK3288)   += rkxxxx...
> > > hantro-vpu-$(VIDEO_HANTRO_IMX8)             += imx8...
> > >
> > > and a couple of #ifdef in rockchip_vpu_drv.c.
> > >
> > > This being said, I think most of the code in the SoC specific files
> > > could be shared if we find a way to abstract the reg layout (using
> > > regmap/reg_field?), leaving a small amount of SoC-specific code, so I'm
> > > not sure it's a big deal if have support for all SoCs compiled in. What
> > > could be a problem though is if each SoC starts pulling its own set of
> > > dependencies.
> > >
> >
> > I'd rather we do this right from the start. It's easy enough to implement,
> > and it is cleaner this way.
>
> I'm not sure the size difference is worth the additional Kconfig
> options, but I'll add them and see.

What is the status of this?  It would be nice to support the hantro
driver on the i.MX8M... family if possible.

let me know if I can help in any way.

adam
>
> regards
> Philipp