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

[Philipp Zabel <p.zabel@pengutronix.de>]

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.

regards
Philipp