On 6/3/19 10:02 PM, Boris Brezillon wrote:
> On Mon, 3 Jun 2019 14:45:37 +0200
> Hans Verkuil <hverk...@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.za...@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.
Regards,
Hans
