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

[Boris Brezillon <boris.brezillon@collabora.com>]

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.