[33/33] iris: enable building of iris video driver

  From: Dikshita Agarwal <quic_dikshita@quicinc.com>

This adds iris driver Makefile and Kconfig, also changes
v4l2 platform/qcom Makefile/Kconfig in order to
enable compilation of the driver.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/Kconfig       |  1 +
 drivers/media/platform/qcom/Makefile      |  1 +
 drivers/media/platform/qcom/iris/Kconfig  | 15 ++++++++++
 drivers/media/platform/qcom/iris/Makefile | 46 +++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+)
 create mode 100644 drivers/media/platform/qcom/iris/Kconfig
 create mode 100644 drivers/media/platform/qcom/iris/Makefile
  

On Fri, 28 Jul 2023 at 17:28, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>
> This adds iris driver Makefile and Kconfig, also changes
> v4l2 platform/qcom Makefile/Kconfig in order to
> enable compilation of the driver.
>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  drivers/media/platform/qcom/Kconfig       |  1 +
>  drivers/media/platform/qcom/Makefile      |  1 +
>  drivers/media/platform/qcom/iris/Kconfig  | 15 ++++++++++
>  drivers/media/platform/qcom/iris/Makefile | 46 +++++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/iris/Kconfig
>  create mode 100644 drivers/media/platform/qcom/iris/Makefile
>
> diff --git a/drivers/media/platform/qcom/Kconfig b/drivers/media/platform/qcom/Kconfig
> index cc5799b..b86bebd 100644
> --- a/drivers/media/platform/qcom/Kconfig
> +++ b/drivers/media/platform/qcom/Kconfig
> @@ -4,3 +4,4 @@ comment "Qualcomm media platform drivers"
>
>  source "drivers/media/platform/qcom/camss/Kconfig"
>  source "drivers/media/platform/qcom/venus/Kconfig"
> +source "drivers/media/platform/qcom/iris/Kconfig"
> diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile
> index 4f055c3..83eea29 100644
> --- a/drivers/media/platform/qcom/Makefile
> +++ b/drivers/media/platform/qcom/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-y += camss/
>  obj-y += venus/
> +obj-y += iris/
> diff --git a/drivers/media/platform/qcom/iris/Kconfig b/drivers/media/platform/qcom/iris/Kconfig
> new file mode 100644
> index 0000000..d434c31
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/Kconfig
> @@ -0,0 +1,15 @@
> +config VIDEO_QCOM_IRIS
> +       tristate "Qualcomm Iris V4L2 encoder/decoder driver"
> +       depends on V4L_MEM2MEM_DRIVERS
> +       depends on VIDEO_DEV && QCOM_SMEM
> +       depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST
> +       select QCOM_MDT_LOADER if ARCH_QCOM
> +       select QCOM_SCM
> +       select VIDEOBUF2_DMA_CONTIG
> +       select V4L2_MEM2MEM_DEV
> +       select DMABUF_HEAPS
> +       help
> +         This is a V4L2 driver for Qualcomm Iris video accelerator
> +         hardware. It accelerates encoding and decoding operations
> +         on various Qualcomm SoCs.
> +         To compile this driver as a module choose m here.
> diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile
> new file mode 100644
> index 0000000..e681c4f
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/Makefile
> @@ -0,0 +1,46 @@
> +KBUILD_OPTIONS+= VIDEO_ROOT=$(KERNEL_SRC)/$(M)
> +
> +VIDEO_COMPILE_TIME = $(shell date)
> +VIDEO_COMPILE_BY = $(shell whoami | sed 's/\\/\\\\/')
> +VIDEO_COMPILE_HOST = $(shell uname -n)
> +VIDEO_GEN_PATH = $(srctree)/$(src)/vidc/inc/video_generated_h
> +
> +$(shell echo '#define VIDEO_COMPILE_TIME "$(VIDEO_COMPILE_TIME)"' > $(VIDEO_GEN_PATH))
> +$(shell echo '#define VIDEO_COMPILE_BY "$(VIDEO_COMPILE_BY)"' >> $(VIDEO_GEN_PATH))
> +$(shell echo '#define VIDEO_COMPILE_HOST "$(VIDEO_COMPILE_HOST)"' >> $(VIDEO_GEN_PATH))

Why do you need this at all?

> +
> +iris-objs += vidc/src/msm_vidc_v4l2.o \
> +                  vidc/src/msm_vidc_vb2.o \
> +                  vidc/src/msm_vidc.o \
> +                  vidc/src/msm_vdec.o \
> +                  vidc/src/msm_venc.o \
> +                  vidc/src/msm_vidc_driver.o \
> +                  vidc/src/msm_vidc_control.o \
> +                  vidc/src/msm_vidc_buffer.o \
> +                  vidc/src/msm_vidc_power.o \
> +                  vidc/src/msm_vidc_probe.o \
> +                  vidc/src/resources.o \
> +                  vidc/src/firmware.o \
> +                  vidc/src/msm_vidc_debug.o \
> +                  vidc/src/msm_vidc_memory.o \
> +                  vidc/src/venus_hfi.o \
> +                  vidc/src/venus_hfi_queue.o \
> +                  vidc/src/hfi_packet.o \
> +                  vidc/src/venus_hfi_response.o \
> +                  vidc/src/msm_vidc_state.o \
> +                  platform/common/src/msm_vidc_platform.o \
> +                  platform/sm8550/src/msm_vidc_sm8550.o \
> +                  variant/common/src/msm_vidc_variant.o \
> +                  variant/iris3/src/msm_vidc_buffer_iris3.o \
> +                  variant/iris3/src/msm_vidc_iris3.o \
> +                  variant/iris3/src/msm_vidc_power_iris3.o \
> +                  variant/iris3/src/msm_vidc_bus_iris3.o \
> +                  variant/iris3/src/msm_vidc_clock_iris3.o
> +
> +obj-$(CONFIG_VIDEO_QCOM_IRIS) += iris.o
> +
> +ccflags-y += -I$(srctree)/$(src)/vidc/inc
> +ccflags-y += -I$(srctree)/$(src)/platform/common/inc
> +ccflags-y += -I$(srctree)/$(src)/platform/sm8550/inc
> +ccflags-y += -I$(srctree)/$(src)/variant/common/inc
> +ccflags-y += -I$(srctree)/$(src)/variant/iris3/inc

For me this is a sign of the bad structure of the include files.
Please define proper interfaces between submodules. The parts of the
driver usually should include files from the top-level dir only (and
from the local subdirectory of course).

> --
> 2.7.4
>
  

On 28/07/2023 14:23, Vikash Garodia wrote:
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
> 
> This adds iris driver Makefile and Kconfig, also changes
> v4l2 platform/qcom Makefile/Kconfig in order to
> enable compilation of the driver.

This is not a meaningfully bisectable patch.

It should go with the addition of the driver. Its good practice to break 
up incremental changes to a driver in a series but, I don't see why you 
really need to do that when adding a whole new driver.

Just

- Documentation
- Bindings
- Driver code

On the other hand if you were switching on IRIS in the default defconfig 
then that should be a separate patch.

If we were say adding inter-frame power-collapse to the existing venus 
as part of a series, then that makes sense as a standalone patch but IMO 
when adding a whole new driver, add it as one.

Its easier to read that way

---
bod
  

On 28/07/2023 18:25, Bryan O'Donoghue wrote:
> On 28/07/2023 14:23, Vikash Garodia wrote:
>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>
>> This adds iris driver Makefile and Kconfig, also changes
>> v4l2 platform/qcom Makefile/Kconfig in order to
>> enable compilation of the driver.
> 
> This is not a meaningfully bisectable patch.
> 
> It should go with the addition of the driver. Its good practice to break 
> up incremental changes to a driver in a series but, I don't see why you 
> really need to do that when adding a whole new driver.
> 
> Just
> 
> - Documentation
> - Bindings
> - Driver code
> 
> On the other hand if you were switching on IRIS in the default defconfig 
> then that should be a separate patch.
> 
> If we were say adding inter-frame power-collapse to the existing venus 
> as part of a series, then that makes sense as a standalone patch but IMO 
> when adding a whole new driver, add it as one.
> 
> Its easier to read that way

It wouldn't pass through mailing list filters.