* [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC @ 2019-02-01 11:21 Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory Frederic Chen ` (7 more replies) 0 siblings, 8 replies; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media Hello, This is the first version of the RFC patch series adding Digital Image Processing (DIP) driver on Mediatek mt8183 SoC, which will be used in camera features on CrOS application. It belongs to the first Mediatek’s ISP driver series based on V4L2 and media controller framework. I posted the main part of the DIP driver as RFC to discuss first and would like some review comments on the overall structure of the driver. Digital Image Processing (DIP) unit can accept the tuning parameters and adjust the image content in Mediatek ISP system. Furthermore, it performs demosaicing and noise reduction on the image to support the advanced camera features of the application. The DIP driver also support image format conversion, resizing and rotation with its hardware path. The driver is implemented with V4L2 and media controller framework. We have the following entities describing the DIP path. 1. Meta (output video device): connects to DIP sub device. It accepts the input tuning buffer from userspace. The metadata interface used currently is only a temporary solution to kick off driver development and is not ready for reviewed yet. 2. RAW (output video device): connects to DIP sub device. It accepts input image buffer from userspace. 3. DIP (sub device): connects to MDP-0 and MDP-1. When processing an image, DIP hardware support multiple output image with different size and format so it needs two capture video devices to return the streaming data to the user. 4. MDP-0 (capture video device): return the processed image data. 5. MDP-1 (capture video device): return the processed image data, the image size and format can be different from the ones of MDP-0. The overall file structure of the DIP driver is as following: * mtk_dip-dev-ctx-core.c: Implements common software flow of DIP driver. DIP driver supports two or more software contexts. For example, context 0 is created for preview path and context 1 is for capture path. Both the two contexts share the same DIP hardware to process the images. * mtk_dip-v4l2.c: Static DIP contexts configuration. * mtk_dip.c: Controls the hardware flow. * mtk_dip-dev.c: Implements context-independent flow. * mtk_dip-ctrl.c: Handles the HW ctrl request from userspace. * mtk_dip-smem-drv.c: Provides the shared memory management required operation. We reserved a memory region for the co-processor and DIP to exchange the tuning and hardware configuration data. * mtk_dip-v4l2-util.c: Implements V4L2 and vb2 ops. Frederic Chen (7): [media] dt-bindings: mt8183: Add binding for DIP shared memory dts: arm64: mt8183: Add DIP shared memory node [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings [media] dt-bindings: mt8183: Added DIP dt-bindings dts: arm64: mt8183: Add DIP nodes media: platform: Add Mediatek DIP driver KConfig [media] platform: mtk-isp: Add Mediatek DIP driver .../bindings/media/mediatek,dip_smem.txt | 29 + .../bindings/media/mediatek,mt8183-dip.txt | 35 + .../mediatek,reserve-memory-dip_smem.txt | 45 + arch/arm64/boot/dts/mediatek/mt8183.dtsi | 36 + drivers/media/platform/Kconfig | 2 + drivers/media/platform/mtk-isp/Kconfig | 21 + drivers/media/platform/mtk-isp/Makefile | 18 + drivers/media/platform/mtk-isp/isp_50/Makefile | 17 + drivers/media/platform/mtk-isp/isp_50/dip/Makefile | 35 + .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h | 188 +++ .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c | 173 +++ .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h | 43 + .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h | 319 ++++ .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c | 1643 ++++++++++++++++++++ .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c | 374 +++++ .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h | 191 +++ .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c | 452 ++++++ .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h | 25 + .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c | 1000 ++++++++++++ .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h | 38 + .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c | 292 ++++ .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h | 60 + .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c | 1385 +++++++++++++++++ .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h | 93 ++ 24 files changed, 6514 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt create mode 100644 drivers/media/platform/mtk-isp/Kconfig create mode 100644 drivers/media/platform/mtk-isp/Makefile create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-core.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen @ 2019-02-01 11:21 ` Frederic Chen 2019-02-09 15:59 ` Sakari Ailus 2019-02-01 11:21 ` [RFC PATCH V0 2/7] dts: arm64: mt8183: Add DIP shared memory node Frederic Chen ` (6 subsequent siblings) 7 siblings, 1 reply; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media This patch adds the binding for describing the shared memory used to exchange configuration and tuning data between the co-processor and Digital Image Processing (DIP) unit of the camera ISP system on Mediatek SoCs. Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> --- .../mediatek,reserve-memory-dip_smem.txt | 45 ++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt new file mode 100644 index 0000000..0ded478 --- /dev/null +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt @@ -0,0 +1,45 @@ +Mediatek DIP Shared Memory binding + +This binding describes the shared memory, which serves the purpose of +describing the shared memory region used to exchange data between Digital +Image Processing (DIP) and co-processor in Mediatek SoCs. + +The co-processor doesn't have the iommu so we need to use the physical +address to access the shared buffer in the firmware. + +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so +it can use dma address to access the memory region. +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU) + + +Required properties: + +- compatible: must be "mediatek,reserve-memory-dip_smem" + +- reg: required for static allocation (see reserved-memory.txt for + the detailed usage) + +- alloc-range: required for dynamic allocation. The range must + between 0x00000400 and 0x100000000 due to the co-processer's + addressing limitation + +- size: required for dynamic allocation. The unit is bytes. + If you want to enable the full feature of Digital Processing Unit, + you need 20 MB at least. + + +Example: + +The following example shows the DIP shared memory setup for MT8183. + + reserved-memory { + #address-cells = <2>; + #size-cells = <2>; + ranges; + reserve-memory-isp_smem { + compatible = "mediatek,reserve-memory-dip_smem"; + size = <0 0x1400000>; + alignment = <0 0x1000>; + alloc-ranges = <0 0x40000000 0 0x50000000>; + }; + }; -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory 2019-02-01 11:21 ` [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory Frederic Chen @ 2019-02-09 15:59 ` Sakari Ailus 2019-02-09 18:17 ` Laurent Pinchart 0 siblings, 1 reply; 19+ messages in thread From: Sakari Ailus @ 2019-02-09 15:59 UTC (permalink / raw) To: Frederic Chen Cc: Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, linux-mediatek, matthias.bgg, hans.verkuil, mchehab, linux-arm-kernel, linux-media Hi Frederic, Could you cc the devicetree list, please? On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote: > This patch adds the binding for describing the shared memory > used to exchange configuration and tuning data between the > co-processor and Digital Image Processing (DIP) unit of the > camera ISP system on Mediatek SoCs. > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > --- > .../mediatek,reserve-memory-dip_smem.txt | 45 ++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > new file mode 100644 > index 0000000..0ded478 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > @@ -0,0 +1,45 @@ > +Mediatek DIP Shared Memory binding > + > +This binding describes the shared memory, which serves the purpose of > +describing the shared memory region used to exchange data between Digital > +Image Processing (DIP) and co-processor in Mediatek SoCs. > + > +The co-processor doesn't have the iommu so we need to use the physical > +address to access the shared buffer in the firmware. > + > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so > +it can use dma address to access the memory region. > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU) What kind of purpose is the memory used for? Buffers containing video data, or something else? Could the buffer objects be mapped on the devices based on the need instead? > + > + > +Required properties: > + > +- compatible: must be "mediatek,reserve-memory-dip_smem" > + > +- reg: required for static allocation (see reserved-memory.txt for > + the detailed usage) > + > +- alloc-range: required for dynamic allocation. The range must > + between 0x00000400 and 0x100000000 due to the co-processer's > + addressing limitation > + > +- size: required for dynamic allocation. The unit is bytes. > + If you want to enable the full feature of Digital Processing Unit, > + you need 20 MB at least. > + > + > +Example: > + > +The following example shows the DIP shared memory setup for MT8183. > + > + reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + reserve-memory-isp_smem { > + compatible = "mediatek,reserve-memory-dip_smem"; > + size = <0 0x1400000>; > + alignment = <0 0x1000>; > + alloc-ranges = <0 0x40000000 0 0x50000000>; > + }; > + }; -- Kind regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory 2019-02-09 15:59 ` Sakari Ailus @ 2019-02-09 18:17 ` Laurent Pinchart 2019-02-12 9:37 ` Frederic Chen 0 siblings, 1 reply; 19+ messages in thread From: Laurent Pinchart @ 2019-02-09 18:17 UTC (permalink / raw) To: Sakari Ailus Cc: Sean.Cheng, Frederic Chen, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, laurent.pinchart+renesas, hans.verkuil, matthias.bgg, linux-mediatek, mchehab, linux-arm-kernel, linux-media On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote: > Hi Frederic, > > Could you cc the devicetree list, please? > > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote: > > This patch adds the binding for describing the shared memory > > used to exchange configuration and tuning data between the > > co-processor and Digital Image Processing (DIP) unit of the > > camera ISP system on Mediatek SoCs. > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > --- > > .../mediatek,reserve-memory-dip_smem.txt | 45 ++++++++++++++++++++++ > > 1 file changed, 45 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > new file mode 100644 > > index 0000000..0ded478 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > @@ -0,0 +1,45 @@ > > +Mediatek DIP Shared Memory binding > > + > > +This binding describes the shared memory, which serves the purpose of > > +describing the shared memory region used to exchange data between Digital > > +Image Processing (DIP) and co-processor in Mediatek SoCs. > > + > > +The co-processor doesn't have the iommu so we need to use the physical > > +address to access the shared buffer in the firmware. > > + > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so > > +it can use dma address to access the memory region. > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU) > > What kind of purpose is the memory used for? Buffers containing video data, > or something else? Could the buffer objects be mapped on the devices > based on the need instead? And could CMA be used when physically contiguous memory is needed ? > > + > > + > > +Required properties: > > + > > +- compatible: must be "mediatek,reserve-memory-dip_smem" > > + > > +- reg: required for static allocation (see reserved-memory.txt for > > + the detailed usage) > > + > > +- alloc-range: required for dynamic allocation. The range must > > + between 0x00000400 and 0x100000000 due to the co-processer's > > + addressing limitation > > + > > +- size: required for dynamic allocation. The unit is bytes. > > + If you want to enable the full feature of Digital Processing Unit, > > + you need 20 MB at least. > > + > > + > > +Example: > > + > > +The following example shows the DIP shared memory setup for MT8183. > > + > > + reserved-memory { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > > + reserve-memory-isp_smem { > > + compatible = "mediatek,reserve-memory-dip_smem"; > > + size = <0 0x1400000>; > > + alignment = <0 0x1000>; > > + alloc-ranges = <0 0x40000000 0 0x50000000>; > > + }; > > + }; -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory 2019-02-09 18:17 ` Laurent Pinchart @ 2019-02-12 9:37 ` Frederic Chen 2019-02-13 3:41 ` Tomasz Figa 0 siblings, 1 reply; 19+ messages in thread From: Frederic Chen @ 2019-02-12 9:37 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus Cc: devicetree, Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, linux-mediatek, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, hans.verkuil, matthias.bgg, Sakari Ailus, mchehab, linux-arm-kernel, linux-media Dear Laurent and Sakari, I appreciate your messages. On Sat, 2019-02-09 at 20:17 +0200, Laurent Pinchart wrote: > On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote: > > Hi Frederic, > > > > Could you cc the devicetree list, please? > > > > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote: > > > This patch adds the binding for describing the shared memory > > > used to exchange configuration and tuning data between the > > > co-processor and Digital Image Processing (DIP) unit of the > > > camera ISP system on Mediatek SoCs. > > > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > > --- > > > .../mediatek,reserve-memory-dip_smem.txt | 45 ++++++++++++++++++++++ > > > 1 file changed, 45 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > new file mode 100644 > > > index 0000000..0ded478 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > @@ -0,0 +1,45 @@ > > > +Mediatek DIP Shared Memory binding > > > + > > > +This binding describes the shared memory, which serves the purpose of > > > +describing the shared memory region used to exchange data between Digital > > > +Image Processing (DIP) and co-processor in Mediatek SoCs. > > > + > > > +The co-processor doesn't have the iommu so we need to use the physical > > > +address to access the shared buffer in the firmware. > > > + > > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so > > > +it can use dma address to access the memory region. > > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU) > > > > What kind of purpose is the memory used for? Buffers containing video data, > > or something else? Could the buffer objects be mapped on the devices > > based on the need instead? The memory buffers contain camera 3A tuning data, which are used by the co-processor and DIP IP. About mapping the buffer based on the need instead, I’m not sure I understand this point. Do you mean that allocating and mapping the memory dynamically? > > And could CMA be used when physically contiguous memory is needed ? DIP driver does not use CMA now, because the first version will be used by CrOS but CrOS won’t enable CMA. > > > > + > > > + > > > +Required properties: > > > + > > > +- compatible: must be "mediatek,reserve-memory-dip_smem" > > > + > > > +- reg: required for static allocation (see reserved-memory.txt for > > > + the detailed usage) > > > + > > > +- alloc-range: required for dynamic allocation. The range must > > > + between 0x00000400 and 0x100000000 due to the co-processer's > > > + addressing limitation > > > + > > > +- size: required for dynamic allocation. The unit is bytes. > > > + If you want to enable the full feature of Digital Processing Unit, > > > + you need 20 MB at least. > > > + > > > + > > > +Example: > > > + > > > +The following example shows the DIP shared memory setup for MT8183. > > > + > > > + reserved-memory { > > > + #address-cells = <2>; > > > + #size-cells = <2>; > > > + ranges; > > > + reserve-memory-isp_smem { > > > + compatible = "mediatek,reserve-memory-dip_smem"; > > > + size = <0 0x1400000>; > > > + alignment = <0 0x1000>; > > > + alloc-ranges = <0 0x40000000 0 0x50000000>; > > > + }; > > > + }; > Sincerely, Frederic Chen _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory 2019-02-12 9:37 ` Frederic Chen @ 2019-02-13 3:41 ` Tomasz Figa 0 siblings, 0 replies; 19+ messages in thread From: Tomasz Figa @ 2019-02-13 3:41 UTC (permalink / raw) To: Frederic Chen, Laurent Pinchart, Sakari Ailus Cc: devicetree, Sean Cheng (鄭昇弘), Laurent Pinchart, Rynn Wu (吳育恩), Christie Yu (游雅惠), srv_heupstream, Holmes Chiou (邱挺), Jerry-ch.Chen, Jungo Lin (林明俊), Sj Huang, Hans Verkuil, Matthias Brugger, linux-mediatek, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Tue, Feb 12, 2019 at 6:37 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > Dear Laurent and Sakari, > > I appreciate your messages. > > On Sat, 2019-02-09 at 20:17 +0200, Laurent Pinchart wrote: > > On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote: > > > Hi Frederic, > > > > > > Could you cc the devicetree list, please? > > > > > > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote: > > > > This patch adds the binding for describing the shared memory > > > > used to exchange configuration and tuning data between the > > > > co-processor and Digital Image Processing (DIP) unit of the > > > > camera ISP system on Mediatek SoCs. > > > > > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > > > --- > > > > .../mediatek,reserve-memory-dip_smem.txt | 45 ++++++++++++++++++++++ > > > > 1 file changed, 45 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > > > > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > > new file mode 100644 > > > > index 0000000..0ded478 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > > > > @@ -0,0 +1,45 @@ > > > > +Mediatek DIP Shared Memory binding > > > > + > > > > +This binding describes the shared memory, which serves the purpose of > > > > +describing the shared memory region used to exchange data between Digital > > > > +Image Processing (DIP) and co-processor in Mediatek SoCs. > > > > + > > > > +The co-processor doesn't have the iommu so we need to use the physical > > > > +address to access the shared buffer in the firmware. > > > > + > > > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so > > > > +it can use dma address to access the memory region. > > > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU) > > > > > > What kind of purpose is the memory used for? Buffers containing video data, > > > or something else? Could the buffer objects be mapped on the devices > > > based on the need instead? > > The memory buffers contain camera 3A tuning data, which are used by the > co-processor and DIP IP. About mapping the buffer based on the need > instead, I’m not sure I understand this point. Do you mean that > allocating and mapping the memory dynamically? > > > > > And could CMA be used when physically contiguous memory is needed ? > > DIP driver does not use CMA now, because the first version will be used > by CrOS but CrOS won’t enable CMA. > Thanks Frederic for replying. Let me further clarify what's the problem here. The co-processor is behind a simple MPU (Memory Protection Unit), which does not have any mapping capabilities, but can only allow or deny access to particular parts of the physical address space. That means that we have to either build some scatter gather capability inside of the firmware or just do with contiguous allocation. There is also a security aspect here. The MPU can be accessed from both the co-processor and CPU, but it has a lockdown mode, which makes it read only until the SoC is reset. If we allocate the memory dynamically, we need to keep the MPU unlocked, which automatically grants the firmware access to all the address space. For security reasons we decided to go with preallocated memory pool, which lets us pre-program the MPU and lock it down. Best regards, Tomasz > > > > > > + > > > > + > > > > +Required properties: > > > > + > > > > +- compatible: must be "mediatek,reserve-memory-dip_smem" > > > > + > > > > +- reg: required for static allocation (see reserved-memory.txt for > > > > + the detailed usage) > > > > + > > > > +- alloc-range: required for dynamic allocation. The range must > > > > + between 0x00000400 and 0x100000000 due to the co-processer's > > > > + addressing limitation > > > > + > > > > +- size: required for dynamic allocation. The unit is bytes. > > > > + If you want to enable the full feature of Digital Processing Unit, > > > > + you need 20 MB at least. > > > > + > > > > + > > > > +Example: > > > > + > > > > +The following example shows the DIP shared memory setup for MT8183. > > > > + > > > > + reserved-memory { > > > > + #address-cells = <2>; > > > > + #size-cells = <2>; > > > > + ranges; > > > > + reserve-memory-isp_smem { > > > > + compatible = "mediatek,reserve-memory-dip_smem"; > > > > + size = <0 0x1400000>; > > > > + alignment = <0 0x1000>; > > > > + alloc-ranges = <0 0x40000000 0 0x50000000>; > > > > + }; > > > > + }; > > > > Sincerely, > > Frederic Chen > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH V0 2/7] dts: arm64: mt8183: Add DIP shared memory node 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory Frederic Chen @ 2019-02-01 11:21 ` Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings Frederic Chen ` (5 subsequent siblings) 7 siblings, 0 replies; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media This patch adds a shared memory region used on mt8183 for exchanging tuning data between co-processor and Digital Image Processing (DIP) unit. Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> --- arch/arm64/boot/dts/mediatek/mt8183.dtsi | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi index 63db9cc..fff67c4 100644 --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi @@ -133,6 +133,19 @@ clock-output-names = "clk26m"; }; + reserved-memory { + #address-cells = <2>; + #size-cells = <2>; + ranges; + reserve-memory-dip_smem { + compatible = "mediatek,reserve-memory-dip_smem"; + no-map; + size = <0 0x01400000>; /*20 MB share mem size */ + alignment = <0 0x1000>; + alloc-ranges = <0 0x40000000 0 0x10000000>; + }; + }; + timer { compatible = "arm,armv8-timer"; interrupt-parent = <&gic>; -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 2/7] dts: arm64: mt8183: Add DIP shared memory node Frederic Chen @ 2019-02-01 11:21 ` Frederic Chen 2019-02-09 15:59 ` Sakari Ailus 2019-02-01 11:21 ` [RFC PATCH V0 4/7] [media] dt-bindings: mt8183: Added DIP dt-bindings Frederic Chen ` (4 subsequent siblings) 7 siblings, 1 reply; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media This patch adds the DT binding documentation for the shared memory between DIP (Digital Image Processing) unit of the camera ISP system and the co-processor in Mediatek SoCs. Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> --- .../bindings/media/mediatek,dip_smem.txt | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt new file mode 100644 index 0000000..5533721 --- /dev/null +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt @@ -0,0 +1,29 @@ +Mediatek ISP Shared Memory Device + +Mediatek ISP Shared Memory Device is used to manage shared memory +among CPU, ISP IPs and coprocessor. It is associated with a reserved +memory region (Please see Documentation\devicetree\bindings\ +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and +and provide the context to allocate memory with dma addresses. + +Required properties: +- compatible: Should be "mediatek,isp_smem" + +- iommus: should point to the respective IOMMU block with master port + as argument. Please set the ports which may be accessed + through the common path. You can see + Documentation/devicetree/bindings/iommu/mediatek,iommu.txt + for the detail. + +- mediatek,larb: must contain the local arbiters in the current Socs. + Please set the larb of camsys for Pass 1 and imgsys for DIP, or both + if you are using all the camera function. You can see + Documentation/devicetree/bindings/memory-controllers/ + mediatek,smi-larb.txt for the detail. + +Example: + isp_smem: isp_smem { + compatible = "mediatek,isp_smem"; + mediatek,larb = <&larb5>; + iommus = <&iommu M4U_PORT_CAM_IMGI>; + }; -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings 2019-02-01 11:21 ` [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings Frederic Chen @ 2019-02-09 15:59 ` Sakari Ailus 2019-02-09 18:20 ` Laurent Pinchart 0 siblings, 1 reply; 19+ messages in thread From: Sakari Ailus @ 2019-02-09 15:59 UTC (permalink / raw) To: Frederic Chen Cc: Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, linux-mediatek, matthias.bgg, hans.verkuil, mchehab, linux-arm-kernel, linux-media Hi Frederic, Thanks for the patchset. Could you also cc the devicetree list, please? On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote: > This patch adds the DT binding documentation for the shared memory > between DIP (Digital Image Processing) unit of the camera ISP system > and the co-processor in Mediatek SoCs. > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > --- > .../bindings/media/mediatek,dip_smem.txt | 29 ++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > new file mode 100644 > index 0000000..5533721 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > @@ -0,0 +1,29 @@ > +Mediatek ISP Shared Memory Device > + > +Mediatek ISP Shared Memory Device is used to manage shared memory > +among CPU, ISP IPs and coprocessor. It is associated with a reserved > +memory region (Please see Documentation\devicetree\bindings\ > +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and s/\\/\//g; > +and provide the context to allocate memory with dma addresses. > + > +Required properties: > +- compatible: Should be "mediatek,isp_smem" s/Should/Shall/ > + > +- iommus: should point to the respective IOMMU block with master port s/should/shall/ > + as argument. Please set the ports which may be accessed > + through the common path. You can see > + Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > + for the detail. > + > +- mediatek,larb: must contain the local arbiters in the current Socs. Perhaps "SoCs"? > + Please set the larb of camsys for Pass 1 and imgsys for DIP, or both > + if you are using all the camera function. You can see > + Documentation/devicetree/bindings/memory-controllers/ > + mediatek,smi-larb.txt for the detail. > + > +Example: > + isp_smem: isp_smem { > + compatible = "mediatek,isp_smem"; > + mediatek,larb = <&larb5>; > + iommus = <&iommu M4U_PORT_CAM_IMGI>; > + }; -- Kind regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings 2019-02-09 15:59 ` Sakari Ailus @ 2019-02-09 18:20 ` Laurent Pinchart 2019-02-12 9:50 ` Frederic Chen 0 siblings, 1 reply; 19+ messages in thread From: Laurent Pinchart @ 2019-02-09 18:20 UTC (permalink / raw) To: Sakari Ailus Cc: Sean.Cheng, Frederic Chen, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, laurent.pinchart+renesas, hans.verkuil, matthias.bgg, linux-mediatek, mchehab, linux-arm-kernel, linux-media Hello Frederic, On Sat, Feb 09, 2019 at 05:59:35PM +0200, Sakari Ailus wrote: > Hi Frederic, > > Thanks for the patchset. > > Could you also cc the devicetree list, please? > > On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote: > > This patch adds the DT binding documentation for the shared memory > > between DIP (Digital Image Processing) unit of the camera ISP system > > and the co-processor in Mediatek SoCs. > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > --- > > .../bindings/media/mediatek,dip_smem.txt | 29 ++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > new file mode 100644 > > index 0000000..5533721 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > @@ -0,0 +1,29 @@ > > +Mediatek ISP Shared Memory Device > > + > > +Mediatek ISP Shared Memory Device is used to manage shared memory > > +among CPU, ISP IPs and coprocessor. It is associated with a reserved > > +memory region (Please see Documentation\devicetree\bindings\ > > +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and > > s/\\/\//g; > > > +and provide the context to allocate memory with dma addresses. Does this represent a real device (as in IP core) in the SoC ? There seems to be no driver associated with the compatible string defined herein in this patch series, what is this node used for ? > > +Required properties: > > +- compatible: Should be "mediatek,isp_smem" > > s/Should/Shall/ > > > + > > +- iommus: should point to the respective IOMMU block with master port > > s/should/shall/ > > > + as argument. Please set the ports which may be accessed > > + through the common path. You can see > > + Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > > + for the detail. > > + > > +- mediatek,larb: must contain the local arbiters in the current Socs. > > Perhaps "SoCs"? > > > + Please set the larb of camsys for Pass 1 and imgsys for DIP, or both > > + if you are using all the camera function. You can see > > + Documentation/devicetree/bindings/memory-controllers/ > > + mediatek,smi-larb.txt for the detail. > > + > > +Example: > > + isp_smem: isp_smem { > > + compatible = "mediatek,isp_smem"; > > + mediatek,larb = <&larb5>; > > + iommus = <&iommu M4U_PORT_CAM_IMGI>; > > + }; -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings 2019-02-09 18:20 ` Laurent Pinchart @ 2019-02-12 9:50 ` Frederic Chen 0 siblings, 0 replies; 19+ messages in thread From: Frederic Chen @ 2019-02-12 9:50 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus Cc: devicetree, Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, hans.verkuil, matthias.bgg, linux-mediatek, mchehab, linux-arm-kernel, linux-media Dear Laurent and Sakari, On Sat, 2019-02-09 at 20:20 +0200, Laurent Pinchart wrote: > Hello Frederic, > > On Sat, Feb 09, 2019 at 05:59:35PM +0200, Sakari Ailus wrote: > > Hi Frederic, > > > > Thanks for the patchset. > > > > Could you also cc the devicetree list, please? > > > > On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote: > > > This patch adds the DT binding documentation for the shared memory > > > between DIP (Digital Image Processing) unit of the camera ISP system > > > and the co-processor in Mediatek SoCs. > > > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > > --- > > > .../bindings/media/mediatek,dip_smem.txt | 29 ++++++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > > new file mode 100644 > > > index 0000000..5533721 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > > > @@ -0,0 +1,29 @@ > > > +Mediatek ISP Shared Memory Device > > > + > > > +Mediatek ISP Shared Memory Device is used to manage shared memory > > > +among CPU, ISP IPs and coprocessor. It is associated with a reserved > > > +memory region (Please see Documentation\devicetree\bindings\ > > > +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and > > > > s/\\/\//g; > > > > > +and provide the context to allocate memory with dma addresses. > > Does this represent a real device (as in IP core) in the SoC ? There > seems to be no driver associated with the compatible string defined > herein in this patch series, what is this node used for ? It does not represent a real device. It is used for creating the DIP-specific vb2 buffer allocation context (implemented in /drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c). The compatible string has been renamed as “mediatek,dip_smem” in this patch series and I will correct it in this binding document. > > > > +Required properties: > > > +- compatible: Should be "mediatek,isp_smem" > > > > s/Should/Shall/ > > > > > + > > > +- iommus: should point to the respective IOMMU block with master port > > > > s/should/shall/ > > > > > + as argument. Please set the ports which may be accessed > > > + through the common path. You can see > > > + Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > > > + for the detail. > > > + > > > +- mediatek,larb: must contain the local arbiters in the current Socs. > > > > Perhaps "SoCs"? > > > > > + Please set the larb of camsys for Pass 1 and imgsys for DIP, or both > > > + if you are using all the camera function. You can see > > > + Documentation/devicetree/bindings/memory-controllers/ > > > + mediatek,smi-larb.txt for the detail. > > > + > > > +Example: > > > + isp_smem: isp_smem { > > > + compatible = "mediatek,isp_smem"; > > > + mediatek,larb = <&larb5>; > > > + iommus = <&iommu M4U_PORT_CAM_IMGI>; > > > + }; > Sincerely, Frederic Chen _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH V0 4/7] [media] dt-bindings: mt8183: Added DIP dt-bindings 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen ` (2 preceding siblings ...) 2019-02-01 11:21 ` [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings Frederic Chen @ 2019-02-01 11:21 ` Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 5/7] dts: arm64: mt8183: Add DIP nodes Frederic Chen ` (3 subsequent siblings) 7 siblings, 0 replies; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media This patch adds DT binding documentation for the Digital Image Processing (DIP) unit of camera ISP system on Mediatek’s SoCs. Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> --- .../bindings/media/mediatek,mt8183-dip.txt | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt b/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt new file mode 100644 index 0000000..0e1994b --- /dev/null +++ b/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt @@ -0,0 +1,35 @@ +* Mediatek Digital Image Processor (DIP) + +Digital Image Processor (DIP) unit in Mediatek ISP system is responsible for +image content adjustment according to the tuning parameters. DIP can process +the image form memory buffer and output the processed image to multiple output +buffers. Furthermore, it can support demosaicing and noise reduction on the +images. + +Required properties: +- compatible: "mediatek,mt8183-dip" +- reg: Physical base address and length of the function block register space +- interrupts: interrupt number to the cpu +- iommus: should point to the respective IOMMU block with master port as + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt + for details. +- mediatek,larb: must contain the local arbiters in the current Socs, see + Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt + for details. +- clocks: must contain the local arbiters 5 (LARB5) and DIP clock +- clock-names: must contain DIP_CG_IMG_LARB5 and DIP_CG_IMG_DIP + +Example: + dip: dip@15022000 { + compatible = "mediatek,mt8183-dip"; + mediatek,larb = <&larb5>; + mediatek,mdp3 = <&mdp_rdma0>; + mediatek,vpu = <&vpu>; + iommus = <&iommu M4U_PORT_CAM_IMGI>; + reg = <0 0x15022000 0 0x6000>; + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_LOW>; + clocks = <&imgsys CLK_IMG_LARB5>, + <&imgsys CLK_IMG_DIP>; + clock-names = "DIP_CG_IMG_LARB5", + "DIP_CG_IMG_DIP"; + }; -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC PATCH V0 5/7] dts: arm64: mt8183: Add DIP nodes 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen ` (3 preceding siblings ...) 2019-02-01 11:21 ` [RFC PATCH V0 4/7] [media] dt-bindings: mt8183: Added DIP dt-bindings Frederic Chen @ 2019-02-01 11:21 ` Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 6/7] media: platform: Add Mediatek DIP driver KConfig Frederic Chen ` (2 subsequent siblings) 7 siblings, 0 replies; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media This patch adds nodes for Digital Image Processing (DIP). DIP is embedded in Mediatek SoCs and works with the co-processor to adjust image content according to tuning input data. It also provides image format conversion, resizing, and rotation features. Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> --- arch/arm64/boot/dts/mediatek/mt8183.dtsi | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi index fff67c4..19b2c13 100644 --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi @@ -377,6 +377,29 @@ #clock-cells = <1>; }; + dip_smem: dip_smem { + compatible = "mediatek,dip_smem"; + mediatek,larb = <&larb5>; + iommus = <&iommu M4U_PORT_CAM_IMGI>; + }; + + dip: dip@15022000 { + compatible = "mediatek,mt8183-dip"; + mediatek,larb = <&larb5>; + mediatek,mdp3 = <&mdp_rdma0>; + mediatek,vpu = <&vpu>; + iommus = <&iommu M4U_PORT_CAM_IMGI>; + reg = <0 0x15022000 0 0x6000>; + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_LOW>; + clocks = + <&imgsys CLK_IMG_LARB5>, + <&imgsys CLK_IMG_DIP>; + clock-names = + "DIP_CG_IMG_LARB5", + "DIP_CG_IMG_DIP"; + smem_device = <&dip_smem>; + }; + vdecsys: syscon@16000000 { compatible = "mediatek,mt8183-vdecsys", "syscon"; reg = <0 0x16000000 0 0x1000>; -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC PATCH V0 6/7] media: platform: Add Mediatek DIP driver KConfig 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen ` (4 preceding siblings ...) 2019-02-01 11:21 ` [RFC PATCH V0 5/7] dts: arm64: mt8183: Add DIP nodes Frederic Chen @ 2019-02-01 11:21 ` Frederic Chen [not found] ` <1549020091-42064-8-git-send-email-frederic.chen@mediatek.com> 2019-03-14 8:46 ` [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Hans Verkuil 7 siblings, 0 replies; 19+ messages in thread From: Frederic Chen @ 2019-02-01 11:21 UTC (permalink / raw) To: hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, frederic.chen, linux-arm-kernel, linux-media This patch adds KConfig for Mediatek Digital Image Processing driver (DIP). DIP is embedded in Mediatek SoCs. It can provide image format conversion, resizing, and rotation function. Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> --- drivers/media/platform/Kconfig | 2 ++ drivers/media/platform/mtk-isp/Kconfig | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 drivers/media/platform/mtk-isp/Kconfig diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index a505e9f..ef08d48 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -32,6 +32,8 @@ source "drivers/media/platform/davinci/Kconfig" source "drivers/media/platform/omap/Kconfig" +source "drivers/media/platform/mtk-isp/Kconfig" + config VIDEO_ASPEED tristate "Aspeed AST2400 and AST2500 Video Engine driver" depends on VIDEO_V4L2 diff --git a/drivers/media/platform/mtk-isp/Kconfig b/drivers/media/platform/mtk-isp/Kconfig new file mode 100644 index 0000000..e5f36d1 --- /dev/null +++ b/drivers/media/platform/mtk-isp/Kconfig @@ -0,0 +1,21 @@ +config VIDEO_MEDIATEK_ISP_DIP_SUPPORT + bool "Mediatek Digital Image Processing function" + select DMA_SHARED_BUFFER + select VIDEO_V4L2_SUBDEV_API + select VIDEOBUF2_DMA_CONTIG + select VIDEOBUF2_CORE + select VIDEOBUF2_V4L2 + select VIDEOBUF2_MEMOPS + select VIDEOBUF2_VMALLOC + select MEDIA_CONTROLLER + + default n + help + Support the basic Digital Image Processing (DIP) driver. + + DIP driver provides image format conversion, resizing, + and rotation function through the low power hardware. + DIP also supports multiple output feature. It can + generate two or more output image with different effect + based on a single input image at the same time. + -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <1549020091-42064-8-git-send-email-frederic.chen@mediatek.com>]
* Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver [not found] ` <1549020091-42064-8-git-send-email-frederic.chen@mediatek.com> @ 2019-02-07 19:08 ` Brian Norris [not found] ` <1550756198.11724.86.camel@mtksdccf07> 0 siblings, 1 reply; 19+ messages in thread From: Brian Norris @ 2019-02-07 19:08 UTC (permalink / raw) To: Frederic Chen Cc: Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu, christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, tfiga, jungo.lin, sj.huang, linux-mediatek, matthias.bgg, hans.verkuil, mchehab, linux-arm-kernel, linux-media Hi, On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote: > This patch adds the driver of Digital Image Processing (DIP) > unit in Mediatek ISP system, providing image format conversion, > resizing, and rotation features. > > The mtk-isp directory will contain drivers for multiple IP > blocks found in Mediatek ISP system. It will include ISP Pass 1 > driver (CAM), sensor interface driver, DIP driver and face > detection driver. > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > --- > drivers/media/platform/mtk-isp/Makefile | 18 + > drivers/media/platform/mtk-isp/isp_50/Makefile | 17 + > drivers/media/platform/mtk-isp/isp_50/dip/Makefile | 35 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h | 188 +++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c | 173 +++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h | 43 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h | 319 ++++ > .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c | 1643 ++++++++++++++++++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c | 374 +++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h | 191 +++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c | 452 ++++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h | 25 + > .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c | 1000 ++++++++++++ > .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h | 38 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c | 292 ++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h | 60 + > .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c | 1385 +++++++++++++++++ > .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h | 93 ++ > 18 files changed, 6346 insertions(+) > create mode 100644 drivers/media/platform/mtk-isp/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-core.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h > ... > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > new file mode 100644 > index 0000000..9d29507 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > @@ -0,0 +1,173 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include "mtk_dip-dev.h" > +#include "mtk_dip-ctrl.h" > + > +#define CONFIG_MTK_DIP_COMMON_UT Please don't do this. You're pretending to have configurability that you don't actually have. > + > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl); > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl); > +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl); > + > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl) > +{ > + struct mtk_dip_ctx_queue *queue = > + container_of(ctrl->handler, > + struct mtk_dip_ctx_queue, ctrl_handler); > + > + if (ctrl->val < MTK_DIP_V4l2_BUF_USAGE_DEFAULT || > + ctrl->val >= MTK_DIP_V4l2_BUF_USAGE_NONE) { > + pr_err("Invalid buffer usage id %d", ctrl->val); > + return; > + } > + queue->buffer_usage = ctrl->val; > +} > + > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl) > +{ > + struct mtk_dip_ctx_queue *queue = > + container_of(ctrl->handler, > + struct mtk_dip_ctx_queue, ctrl_handler); > + > + if (ctrl->val != 0 || ctrl->val != 90 || > + ctrl->val != 180 || ctrl->val != 270) { > + pr_err("Invalid buffer rotation %d", ctrl->val); > + return; > + } > + queue->rotation = ctrl->val; > +} > + > +static const struct v4l2_ctrl_ops mtk_dip_ctx_ctrl_ops = { > + .s_ctrl = mtk_dip_ctx_s_ctrl, > +}; > + > +#ifdef CONFIG_MTK_DIP_COMMON_UT Kill the #ifdef if you're not actually going to make this a Kconfig. Same elsewhere. > + > +static void handle_ctrl_common_util_ut_set_debug_mode > + (struct v4l2_ctrl *ctrl) > +{ > + struct mtk_dip_ctx *dev_ctx = > + container_of(ctrl->handler, struct mtk_dip_ctx, ctrl_handler); > + dev_ctx->mode = ctrl->val; > + dev_dbg(&dev_ctx->pdev->dev, "Set ctx(id = %d) mode to %d\n", > + dev_ctx->ctx_id, dev_ctx->mode); > +} > + > +static const struct v4l2_ctrl_config mtk_dip_mode_config = { > + .ops = &mtk_dip_ctx_ctrl_ops, > + .id = V4L2_CID_PRIVATE_SET_CTX_MODE_NUM, > + .name = "MTK ISP UNIT TEST CASE", > + .type = V4L2_CTRL_TYPE_INTEGER, > + .min = 0, > + .max = 65535, > + .step = 1, > + .def = 0, > + .flags = V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE, > +}; > +#endif /* CONFIG_MTK_DIP_COMMON_UT */ > + > +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + switch (ctrl->id) { > + #ifdef CONFIG_MTK_DIP_COMMON_UT > + case V4L2_CID_PRIVATE_SET_CTX_MODE_NUM: > + handle_ctrl_common_util_ut_set_debug_mode(ctrl); > + break; > + #endif /* CONFIG_MTK_DIP_COMMON_UT */ > + default: > + break; > + } > + return 0; > +} > + ... > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > new file mode 100644 > index 0000000..c735919 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > @@ -0,0 +1,1643 @@ ... > +#ifdef MTK_DIP_CTX_DIP_V4L2_UT What is this #ifdef'ery? I don't see MTK_DIP_CTX_DIP_V4L2_UT anywhere. > +static int check_and_refill_dip_ut_start_ipi_param > + (struct img_ipi_frameparam *ipi_param, > + struct mtk_dip_ctx_buffer *ctx_buf_in, > + struct mtk_dip_ctx_buffer *ctx_buf_out) > +{ > + /* Check the buffer size information from user space */ > + int ret = 0; > + unsigned char *buffer_ptr = NULL; > + const unsigned int src_width = 3264; ... > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > new file mode 100644 > index 0000000..b425031 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > @@ -0,0 +1,1000 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 Mediatek Corporation. > + * Copyright (c) 2017 Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * MTK_DIP-v4l2 is highly based on Intel IPU 3 chrome driver > + * > + */ > + > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/videodev2.h> > +#include <media/v4l2-ioctl.h> > +#include <media/videobuf2-dma-contig.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-event.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > + > +#include "mtk_dip-dev.h" > +#include "mtk_dip-v4l2-util.h" > + > +static u32 mtk_dip_node_get_v4l2_cap > + (struct mtk_dip_ctx_queue *node_ctx); > + > +static int mtk_dip_videoc_s_meta_fmt(struct file *file, > + void *fh, struct v4l2_format *f); > + > +static int mtk_dip_subdev_open(struct v4l2_subdev *sd, > + struct v4l2_subdev_fh *fh) > +{ > + struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd); > + > + isp_dev->ctx.fh = fh; > + > + return mtk_dip_ctx_open(&isp_dev->ctx); > +} > + > +static int mtk_dip_subdev_close(struct v4l2_subdev *sd, > + struct v4l2_subdev_fh *fh) > +{ > + struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd); > + > + return mtk_dip_ctx_release(&isp_dev->ctx); > +} > + > +static int mtk_dip_subdev_s_stream(struct v4l2_subdev *sd, > + int enable) > +{ > + int ret = 0; > + > + struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd); > + > + if (enable) { > + ret = mtk_dip_ctx_streamon(&isp_dev->ctx); > + > + if (!ret) > + ret = mtk_dip_dev_queue_buffers > + (mtk_dip_ctx_to_dev(&isp_dev->ctx), 1); > + if (ret) > + pr_err("failed to queue initial buffers (%d)", ret); > + } else { > + ret = mtk_dip_ctx_streamoff(&isp_dev->ctx); > + } > + > + if (!ret) > + isp_dev->mem2mem2.streaming = enable; > + > + return ret; > +} > + > +int mtk_dip_subdev_subscribe_event(struct v4l2_subdev *subdev, > + struct v4l2_fh *fh, > + struct v4l2_event_subscription *sub) Should be static. > +{ > + pr_info("sub type(%x)", sub->type); I feel like you have this problem in other places too: this definitely shouldn't be at KERN_INFO level. It seems a bit excessive anyway. > + if (sub->type != V4L2_EVENT_PRIVATE_START && > + sub->type != V4L2_EVENT_MTK_DIP_FRAME_DONE) > + return -EINVAL; > + > + return v4l2_event_subscribe(fh, sub, 0, NULL); > +} > + > +int mtk_dip_subdev_unsubscribe_event(struct v4l2_subdev *subdev, > + struct v4l2_fh *fh, Static. > + struct v4l2_event_subscription *sub) > +{ > + return v4l2_event_unsubscribe(fh, sub); > +} > + > +static int mtk_dip_link_setup(struct media_entity *entity, > + const struct media_pad *local, > + const struct media_pad *remote, u32 flags) > +{ > + struct mtk_dip_mem2mem2_device *m2m2 = > + container_of(entity, > + struct mtk_dip_mem2mem2_device, > + subdev.entity); > + struct mtk_dip_dev *isp_dev = > + container_of(m2m2, struct mtk_dip_dev, mem2mem2); > + > + u32 pad = local->index; > + > + dev_dbg(&isp_dev->pdev->dev, > + "link setup: %d --> %d\n", pad, remote->index); > + > + WARN_ON(entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV); > + > + WARN_ON(pad >= m2m2->num_nodes); > + > + m2m2->nodes[pad].enabled = !!(flags & MEDIA_LNK_FL_ENABLED); > + > + /* queue_enable can be phase out in the future since */ > + /* we don't have internal queue of each node in */ > + /* v4l2 common module */ > + isp_dev->queue_enabled[pad] = m2m2->nodes[pad].enabled; > + > + return 0; > +} > + > +static void mtk_dip_vb2_buf_queue(struct vb2_buffer *vb) > +{ > + struct mtk_dip_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > + > + struct mtk_dip_dev *mtk_dip_dev = mtk_dip_m2m_to_dev(m2m2); > + > + struct device *dev = &mtk_dip_dev->pdev->dev; > + > + struct mtk_dip_dev_buffer *buf = NULL; > + > + struct vb2_v4l2_buffer *v4l2_buf = NULL; > + > + struct mtk_dip_dev_video_device *node = > + mtk_dip_vbq_to_isp_node(vb->vb2_queue); > + > + int queue = mtk_dip_dev_get_queue_id_of_dev_node(mtk_dip_dev, node); You've got a lot of extra blank lines in here. > + > + dev_dbg(dev, > + "queue vb2_buf: Node(%s) queue id(%d)\n", > + node->name, > + queue); > + > + if (queue < 0) { > + dev_err(m2m2->dev, "Invalid mtk_dip_dev node.\n"); > + return; > + } > + > + if (mtk_dip_dev->ctx.mode == MTK_DIP_CTX_MODE_DEBUG_BYPASS_ALL) { > + dev_dbg(m2m2->dev, "By pass mode, just loop back the buffer\n"); > + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); > + return; > + } > + > + if (!vb) > + pr_err("VB can't be null\n"); > + > + buf = mtk_dip_vb2_buf_to_dev_buf(vb); > + > + if (!buf) > + pr_err("buf can't be null\n"); > + > + v4l2_buf = to_vb2_v4l2_buffer(vb); > + > + if (!v4l2_buf) > + pr_err("v4l2_buf can't be null\n"); > + > + mutex_lock(&mtk_dip_dev->lock); > + > + /* the dma address will be filled in later frame buffer handling*/ > + mtk_dip_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0); > + > + /* Added the buffer into the tracking list */ > + list_add_tail(&buf->m2m2_buf.list, > + &m2m2->nodes[node - m2m2->nodes].buffers); > + mutex_unlock(&mtk_dip_dev->lock); > + > + /* Enqueue the buffer */ > + if (mtk_dip_dev->mem2mem2.streaming) { > + dev_dbg(dev, "%s: mtk_dip_dev_queue_buffers\n", > + node->name); > + mtk_dip_dev_queue_buffers(mtk_dip_dev, 0); > + } > +} ... > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > new file mode 100644 > index 0000000..ffdc45e > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > @@ -0,0 +1,292 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include "mtk_dip-ctx.h" > +#include "mtk_dip.h" > +#include "mtk_dip-v4l2.h" > +#include "mtk-mdp3-regs.h" > + > +#define MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME "MTK-ISP-DIP-V4L2" > +#define MTK_DIP_DEV_DIP_PREVIEW_NAME \ > + MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME > +#define MTK_DIP_DEV_DIP_CAPTURE_NAME "MTK-ISP-DIP-CAP-V4L2" > + > +#ifdef MTK_DIP_CTX_DIP_V4L2_UT > +#include "mtk_dip-dev-ctx-dip-test.h" The above macros was never defined, and this header doesn't exist. Please remove. > +#endif > + ... > +int mtk_dip_ctx_dip_v4l2_init(struct platform_device *pdev, > + struct mtk_dip_dev *isp_preview_dev, > + struct mtk_dip_dev *isp_capture_dev) > +{ > + struct media_device *media_dev; > + struct v4l2_device *v4l2_dev; > + struct v4l2_ctrl_handler *ctrl_handler; > + int ret = 0; > + > + /* initialize the v4l2 common part */ > + dev_info(&pdev->dev, "init v4l2 common part: dev=%llx\n", > + (unsigned long long)&pdev->dev); > + > + media_dev = &isp_preview_dev->media_dev; > + v4l2_dev = &isp_preview_dev->v4l2_dev; > + ctrl_handler = &isp_preview_dev->ctx.ctrl_handler; > + > + ret = mtk_dip_media_register(&pdev->dev, > + media_dev, > + MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME); > + > + ret = mtk_dip_v4l2_register(&pdev->dev, > + media_dev, > + v4l2_dev, > + ctrl_handler); > + > + dev_info(&pdev->dev, "init v4l2 preview part\n"); > + ret = mtk_dip_dev_core_init_ext(pdev, > + isp_preview_dev, > + &mtk_dip_ctx_desc_dip_preview, > + media_dev, v4l2_dev); > + > + if (ret) > + dev_err(&pdev->dev, "Preview v4l2 part init failed: %d\n", ret); > + > + dev_info(&pdev->dev, "init v4l2 capture part\n"); > + > + ret = mtk_dip_dev_core_init_ext(pdev, > + isp_capture_dev, > + &mtk_dip_ctx_desc_dip_capture, > + media_dev, v4l2_dev); > + > + if (ret) > + dev_err(&pdev->dev, "Capture v4l2 part init failed: %d\n", ret); > + > + return ret; > +} > + > +/* MTK ISP context initialization */ > +int mtk_dip_ctx_dip_preview_init(struct mtk_dip_ctx *preview_ctx) > +{ > + preview_ctx->ctx_id = MTK_DIP_CTX_P2_ID_PREVIEW; > + > + /* Initialize main data structure */ > + mtk_dip_ctx_core_queue_setup(preview_ctx, &preview_queues_setting); > + > + return mtk_dip_ctx_core_steup(preview_ctx, > + &mtk_dip_ctx_dip_preview_setting); > +} > +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_preview_init); > + > +/* MTK ISP context initialization */ > +int mtk_dip_ctx_dip_capture_init(struct mtk_dip_ctx *capture_ctx) > +{ > + capture_ctx->ctx_id = MTK_DIP_CTX_P2_ID_CAPTURE; > + /* Initialize main data structure */ > + mtk_dip_ctx_core_queue_setup(capture_ctx, &capture_queues_setting); > + > + return mtk_dip_ctx_core_steup(capture_ctx, > + &mtk_dip_ctx_dip_capture_setting); > +} > +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_capture_init); ... > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > new file mode 100644 > index 0000000..3569c7c > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > @@ -0,0 +1,1385 @@ > +// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Holmes Chiou <holmes.chiou@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/of_device.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/kthread.h> /* thread functions */ > +#include <linux/pm_runtime.h> > +#include <linux/dma-iommu.h> > +#include <linux/spinlock.h> > +#include <linux/slab.h> /* kzalloc and kfree */ > + > +#include "mtk_vpu.h" > +#include "mtk-mdp3-cmdq.h" > + > +#include "mtk_dip-dev.h" > +#include "mtk_dip.h" > +#include "mtk_dip-core.h" > +#include "mtk_dip-v4l2.h" > + > +#define DIP_DEV_NAME "camera-dip" > + > +#define DIP_COMPOSER_THREAD_TIMEOUT (16U) > +#define DIP_COMPOSING_WQ_TIMEOUT (16U) > +#define DIP_COMPOSING_MAX_NUM (3) > +#define DIP_FLUSHING_WQ_TIMEOUT (16U) > + > +#define DIP_MAX_ERR_COUNT (188U) > + > +#define DIP_FRM_SZ (76 * 1024) > +#define DIP_SUB_FRM_SZ (16 * 1024) > +#define DIP_TUNING_SZ (32 * 1024) > +#define DIP_COMP_SZ (24 * 1024) > +#define DIP_FRAMEPARAM_SZ (4 * 1024) > + > +#define DIP_TUNING_OFFSET (DIP_SUB_FRM_SZ) > +#define DIP_COMP_OFFSET (DIP_TUNING_OFFSET + DIP_TUNING_SZ) > +#define DIP_FRAMEPARAM_OFFSET (DIP_COMP_OFFSET + DIP_COMP_SZ) > + > +#define DIP_SUB_FRM_DATA_NUM (32) > + > +#define DIP_SCP_WORKINGBUF_OFFSET (5 * 1024 * 1024) > + > +#define DIP_GET_ID(x) (((x) & 0xffff0000) >> 16) > + > +static const struct of_device_id dip_of_ids[] = { > + /* Remider: Add this device node manually in .dtsi */ > + { .compatible = "mediatek,mt8183-dip", }, > + {} > +}; Please add: MODULE_DEVICE_TABLE(of, dip_of_ids); > + > +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev, > + struct img_ipi_frameparam *iparam); > + > +static struct img_frameparam *dip_create_framejob(int sequence) > +{ > + struct dip_frame_job *fjob = NULL; > + > + fjob = kzalloc(sizeof(*fjob), GFP_ATOMIC); > + > + if (!fjob) > + return NULL; > + > + fjob->sequence = sequence; > + > + return &fjob->fparam; > +} > + > +static void dip_free_framejob(struct img_frameparam *fparam) > +{ > + struct dip_frame_job *fjob = NULL; > + > + fjob = mtk_dip_fparam_to_job(fparam); > + > + /* to avoid use after free issue */ > + fjob->sequence = -1; > + > + kfree(fjob); > +} > + > +static void dip_enable_ccf_clock(struct dip_device *dip_dev) > +{ > + int ret; > + > + ret = pm_runtime_get_sync(dip_dev->larb_dev); > + if (ret < 0) > + dev_err(&dip_dev->pdev->dev, "cannot get smi larb clock\n"); > + > + ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_LARB5); > + if (ret) > + dev_err(&dip_dev->pdev->dev, > + "cannot prepare and enable DIP_IMG_LARB5 clock\n"); > + > + ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_DIP); > + if (ret) > + dev_err(&dip_dev->pdev->dev, > + "cannot prepare and enable DIP_IMG_DIP clock\n"); > +} > + > +static void dip_disable_ccf_clock(struct dip_device *dip_dev) > +{ > + clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_DIP); > + clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_LARB5); > + pm_runtime_put_sync(dip_dev->larb_dev); > +} > + > +static int dip_send(struct platform_device *pdev, enum ipi_id id, > + void *buf, unsigned int len, unsigned int wait) > +{ > + vpu_ipi_send_sync_async(pdev, id, buf, len, wait); > + return 0; > +} > + > +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev, > + struct img_ipi_frameparam *iparam) > +{ > + struct mtk_dip_ctx_finish_param fparam; > + struct mtk_isp_dip_drv_data *drv_data; > + struct mtk_dip_ctx *dev_ctx; > + int ctx_id = 0; > + int r = 0; > + > + if (!dip_dev) { > + pr_err("Can't update buffer status, dip_dev can't be NULL\n"); > + return; > + } > + > + if (!iparam) { > + dev_err(&dip_dev->pdev->dev, > + "%s: iparam can't be NULL\n", __func__); > + return; > + } > + > + drv_data = dip_dev_to_drv(dip_dev); > + > + frame_param_ipi_to_ctx(iparam, &fparam); > + ctx_id = MTK_DIP_GET_CTX_ID_FROM_SEQUENCE(fparam.frame_id); > + > + if (ctx_id == MTK_DIP_CTX_P2_ID_PREVIEW) { > + dev_ctx = &drv_data->isp_preview_dev.ctx; > + } else if (ctx_id == MTK_DIP_CTX_P2_ID_CAPTURE) { > + dev_ctx = &drv_data->isp_capture_dev.ctx; > + } else { > + dev_err(&dip_dev->pdev->dev, > + "unknown ctx id: %d\n", ctx_id); > + return; > + } > + > + r = mtk_dip_ctx_core_job_finish(dev_ctx, &fparam); > + > + if (r) > + dev_err(&dip_dev->pdev->dev, "finish op failed: %d\n", > + r); > + dev_dbg(&dip_dev->pdev->dev, "Ready to return buffers: CTX(%d), Frame(%d)\n", > + ctx_id, fparam.frame_id); > +} > + > +static void mtk_dip_notify(void *data) > +{ > + struct dip_device *dip_dev; > + struct mtk_dip_hw_ctx *dip_ctx; > + struct img_frameparam *framejob; > + struct dip_user_id *user_id; > + struct dip_subframe *buf, *tmpbuf; > + struct img_ipi_frameparam *frameparam; > + u32 num; > + bool found = false; > + > + frameparam = (struct img_ipi_frameparam *)data; > + dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data; > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > + framejob = container_of(frameparam, > + struct img_frameparam, > + frameparam); > + > + if (frameparam->state == FRAME_STATE_HW_TIMEOUT) { > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > + (void *)frameparam, sizeof(*frameparam), 0); > + dev_err(&dip_dev->pdev->dev, "frame no(%d) HW timeout\n", > + frameparam->frame_no); > + } > + > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > + list_for_each_entry_safe(buf, tmpbuf, > + &dip_ctx->dip_usedbufferlist.queue, > + list_entry) { > + if (buf->buffer.pa == frameparam->subfrm_data.pa) { > + list_del(&buf->list_entry); > + dip_ctx->dip_usedbufferlist.queue_cnt--; > + found = true; > + dev_dbg(&dip_dev->pdev->dev, > + "Find used buffer (%x)\n", buf->buffer.pa); > + break; > + } > + } > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > + > + if (!found) { > + dev_err(&dip_dev->pdev->dev, > + "frame_no(%d) buffer(%x) used buffer count(%d)\n", > + frameparam->frame_no, frameparam->subfrm_data.pa, > + dip_ctx->dip_usedbufferlist.queue_cnt); > + > + frameparam->state = FRAME_STATE_ERROR; > + > + } else { > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > + list_add_tail(&buf->list_entry, > + &dip_ctx->dip_freebufferlist.queue); > + dip_ctx->dip_freebufferlist.queue_cnt++; > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > + > + frameparam->state = FRAME_STATE_DONE; > + } > + > + call_mtk_dip_ctx_finish(dip_dev, frameparam); > + > + found = false; > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > + list_for_each_entry(user_id, > + &dip_ctx->dip_useridlist.queue, > + list_entry) { > + if (DIP_GET_ID(frameparam->index) == user_id->id) { > + user_id->num--; > + dev_dbg(&dip_dev->pdev->dev, > + "user_id(%x) is found, and cnt: %d\n", > + user_id->id, user_id->num); > + found = true; > + break; > + } > + } > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > + wake_up(&dip_ctx->flushing_wq); > + dev_dbg(&dip_dev->pdev->dev, > + "frame_no(%d) is finished\n", framejob->frameparam.frame_no); > + dip_free_framejob(framejob); > + > + num = atomic_dec_return(&dip_ctx->num_running); > + dev_dbg(&dip_dev->pdev->dev, "Running count: %d\n", num); > +} > + > +static void mdp_cb_worker(struct work_struct *work) > +{ > + struct mtk_mdpcb_work *mdpcb_work; > + > + mdpcb_work = container_of(work, struct mtk_mdpcb_work, frame_work); > + mtk_dip_notify(mdpcb_work->frameparams); > + kfree(mdpcb_work); > +} > + > +static struct img_ipi_frameparam *convert_to_fparam(struct cmdq_cb_data *data) > +{ > + struct device *dev = NULL; Every use of dev_err() in this function is wrong, since you're guaranteed to be NULL. Either come up with some better way to report device errors using the pointers you have, or else just switch to pr_err(). > + struct dip_device *dip_dev = NULL; > + struct dip_frame_job *fjob = NULL; > + struct img_ipi_frameparam *ipi_fparam = NULL; > + > + if (!data) { > + dev_err(dev, "DIP got NULL in cmdq_cb_data,%s\n", > + __func__); > + return NULL; > + } > + > + if (data->sta != CMDQ_CB_NORMAL) { > + dev_warn(dev, "DIP got CMDQ CB (%d) without CMDQ_CB_NORMAL\n", > + data->sta); > + } > + > + if (!data->data) { > + dev_err(dev, "DIP got NULL data in cmdq_cb_data,%s\n", > + __func__); > + return NULL; > + } > + > + fjob = mtk_dip_ipi_fparam_to_job(data->data); > + > + if (fjob->sequence == -1) { > + dev_err(dev, "Invalid cmdq_cb_data(%llx)\n", > + (unsigned long long)data); > + ipi_fparam = NULL; > + } else { > + ipi_fparam = &fjob->fparam.frameparam; > + dip_dev = dip_hw_ctx_to_dev((void *)ipi_fparam->drv_data); > + dev = &dip_dev->pdev->dev; > + } > + > + dev_dbg(dev, "framejob(0x%llx,seq:%d):\n", > + (unsigned long long)fjob, fjob->sequence); > + dev_dbg(dev, "idx(%d),no(%d),s(%d),n_in(%d),n_out(%d),drv(%llx)\n", > + fjob->fparam.frameparam.index, > + fjob->fparam.frameparam.frame_no, > + fjob->fparam.frameparam.state, > + fjob->fparam.frameparam.num_inputs, > + fjob->fparam.frameparam.num_outputs, > + (unsigned long long)fjob->fparam.frameparam.drv_data > + ); > + > + return ipi_fparam; > +} > + > +/* Maybe in IRQ context of cmdq */ > +void dip_mdp_cb_func(struct cmdq_cb_data data) Make this static. > +{ > + struct img_ipi_frameparam *frameparam; > + struct mtk_dip_hw_ctx *dip_ctx; > + struct mtk_mdpcb_work *mdpcb_work; > + > + frameparam = convert_to_fparam(&data); > + > + if (!frameparam) { > + dev_err(NULL, "%s return due to invalid cmdq_cb_data(%llx)", Don't directly pass NULL to dev_err(). Just use pr_err() or similar. > + __func__, &data); > + return; > + } > + > + dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data; > + > + mdpcb_work = kzalloc(sizeof(*mdpcb_work), GFP_ATOMIC); > + WARN_ONCE(!mdpcb_work, "frame_no(%d) is lost", frameparam->frame_no); > + if (!mdpcb_work) > + return; > + > + INIT_WORK(&mdpcb_work->frame_work, mdp_cb_worker); > + mdpcb_work->frameparams = frameparam; > + if (data.sta != CMDQ_CB_NORMAL) > + mdpcb_work->frameparams->state = FRAME_STATE_HW_TIMEOUT; > + > + queue_work(dip_ctx->mdpcb_workqueue, &mdpcb_work->frame_work); > +} > + > +static void dip_vpu_handler(void *data, unsigned int len, void *priv) > +{ > + struct img_frameparam *framejob; > + struct img_ipi_frameparam *frameparam; > + struct mtk_dip_hw_ctx *dip_ctx; > + struct dip_device *dip_dev; > + unsigned long flags; > + u32 num; > + > + WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__); > + if (!data) You can combine these lines: if (WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__)) return; > + return; > + > + frameparam = (struct img_ipi_frameparam *)data; > + > + framejob = dip_create_framejob(frameparam->index); > + WARN_ONCE(!framejob, "frame_no(%d) is lost", frameparam->frame_no); > + if (!framejob) Same here. > + return; > + > + dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data; > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > + > + wake_up(&dip_ctx->composing_wq); > + memcpy(&framejob->frameparam, data, sizeof(framejob->frameparam)); > + num = atomic_dec_return(&dip_ctx->num_composing); > + > + spin_lock_irqsave(&dip_ctx->dip_gcejoblist.queuelock, flags); > + list_add_tail(&framejob->list_entry, &dip_ctx->dip_gcejoblist.queue); > + dip_ctx->dip_gcejoblist.queue_cnt++; > + spin_unlock_irqrestore(&dip_ctx->dip_gcejoblist.queuelock, flags); > + > + dev_dbg(&dip_dev->pdev->dev, > + "frame_no(%d) is back, composing num: %d\n", > + frameparam->frame_no, num); > + > + wake_up(&dip_ctx->dip_runner_thread.wq); > +} > + ... > +static int dip_runner_func(void *data) > +{ ... > + > + mdp_cmdq_sendtask I don't see this defined anywhere? > + (dip_ctx->mdp_pdev, > + (struct img_config *) > + framejob->frameparam.config_data.va, > + &framejob->frameparam, NULL, false, > + dip_mdp_cb_func, > + (void *)&framejob->frameparam); > + ... > + return 0; > +} > + > +static void dip_submit_worker(struct work_struct *work) > +{ > + struct mtk_dip_submit_work *dip_submit_work = > + container_of(work, struct mtk_dip_submit_work, frame_work); > + > + struct mtk_dip_hw_ctx *dip_ctx = dip_submit_work->dip_ctx; > + struct mtk_dip_work *dip_work; > + struct dip_device *dip_dev; > + struct dip_subframe *buf; > + u32 len, num; > + int ret; > + > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > + num = atomic_read(&dip_ctx->num_composing); > + > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > + dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, > + struct mtk_dip_work, list_entry); > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); I see you grab the head of the list here, but then you release the lock. Then you later assume that reference is still valid, throughout this function. That's usually true, because you only remove/delete entries from this list within this same workqueue (at the end of this function). But it's not true in dip_release_context() (which doesn't even grab the lock, BTW). I think there could be several ways to solve this, but judging by how this list entry is used...couldn't you just remove it from the list here, while holding the lock? Then you only have to kfree() it when you're done under the free_work_list label. > + > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > + if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > + > + dip_work->frameparams.state = FRAME_STATE_STREAMOFF; > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > + > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > + dip_work->user_id->num--; > + dev_dbg(&dip_dev->pdev->dev, > + "user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n", > + dip_work->user_id->id, dip_work->user_id->num, > + dip_work->frameparams.frame_no, > + dip_work->frameparams.index); > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > + > + goto free_work_list; > + } > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > + > + while (num >= DIP_COMPOSING_MAX_NUM) { > + ret = wait_event_interruptible_timeout > + (dip_ctx->composing_wq, > + (num < DIP_COMPOSING_MAX_NUM), > + msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT)); > + > + if (ret == -ERESTARTSYS) > + dev_err(&dip_dev->pdev->dev, > + "interrupted by a signal!\n"); > + else if (ret == 0) > + dev_dbg(&dip_dev->pdev->dev, > + "timeout frame_no(%d), num: %d\n", > + dip_work->frameparams.frame_no, num); > + else > + dev_dbg(&dip_dev->pdev->dev, > + "wakeup frame_no(%d), num: %d\n", > + dip_work->frameparams.frame_no, num); > + > + num = atomic_read(&dip_ctx->num_composing); > + }; > + > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > + if (list_empty(&dip_ctx->dip_freebufferlist.queue)) { > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > + > + dev_err(&dip_dev->pdev->dev, > + "frame_no(%d) index: %x no free buffer: %d\n", > + dip_work->frameparams.frame_no, > + dip_work->frameparams.index, > + dip_ctx->dip_freebufferlist.queue_cnt); > + > + /* Call callback to notify V4L2 common framework > + * for failure of enqueue > + */ > + dip_work->frameparams.state = FRAME_STATE_ERROR; > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > + > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > + dip_work->user_id->num--; > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > + > + goto free_work_list; > + } > + > + buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue, > + struct dip_subframe, > + list_entry); > + list_del(&buf->list_entry); > + dip_ctx->dip_freebufferlist.queue_cnt--; > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > + > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > + list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue); > + dip_ctx->dip_usedbufferlist.queue_cnt++; > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > + > + memcpy(&dip_work->frameparams.subfrm_data, > + &buf->buffer, sizeof(buf->buffer)); > + > + memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ); > + > + memcpy(&dip_work->frameparams.config_data, > + &buf->config_data, sizeof(buf->config_data)); > + > + memset((char *)buf->config_data.va, 0, DIP_COMP_SZ); > + > + if (dip_work->frameparams.tuning_data.pa == 0) { > + dev_dbg(&dip_dev->pdev->dev, > + "frame_no(%d) has no tuning_data\n", > + dip_work->frameparams.frame_no); > + > + memcpy(&dip_work->frameparams.tuning_data, > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > + > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > + /* When user enqueued without tuning buffer, > + * it would use driver internal buffer. > + * So, tuning_data.va should be 0 > + */ > + dip_work->frameparams.tuning_data.va = 0; > + } > + > + dip_work->frameparams.drv_data = (u64)dip_ctx; > + dip_work->frameparams.state = FRAME_STATE_COMPOSING; > + > + memcpy((void *)buf->frameparam.va, &dip_work->frameparams, > + sizeof(dip_work->frameparams)); > + > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > + (void *)&dip_work->frameparams, > + sizeof(dip_work->frameparams), 0); > + num = atomic_inc_return(&dip_ctx->num_composing); > + > +free_work_list: > + > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > + list_del(&dip_work->list_entry); > + dip_ctx->dip_worklist.queue_cnt--; > + len = dip_ctx->dip_worklist.queue_cnt; > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > + > + dev_dbg(&dip_dev->pdev->dev, > + "frame_no(%d) index: %x, worklist count: %d, composing num: %d\n", > + dip_work->frameparams.frame_no, dip_work->frameparams.index, > + len, num); > + > + kfree(dip_work); > +} ... > +int dip_open_context(struct dip_device *dip_dev) Should be static. > +{ ... > +} > + > +int dip_release_context(struct dip_device *dip_dev) Should be static. > +{ > + u32 i = 0; > + struct dip_subframe *buf, *tmpbuf; > + struct mtk_dip_work *dip_work, *tmp_work; > + struct dip_user_id *dip_userid, *tmp_id; > + struct mtk_dip_hw_ctx *dip_ctx; > + > + dip_ctx = &dip_dev->dip_ctx; > + dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n", > + dip_ctx->dip_worklist.queue_cnt); > + > + list_for_each_entry_safe(dip_work, tmp_work, > + &dip_ctx->dip_worklist.queue, > + list_entry) { Shouldn't you be holding the mutex for this? Or alternatively, cancel any outstanding work and move the flush_workqueue()/destroy_workqueue() up. Similar questions for the other lists we're going through here. > + list_del(&dip_work->list_entry); > + dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n", > + dip_work->frameparams.frame_no); > + kfree(dip_work); > + dip_ctx->dip_worklist.queue_cnt--; > + } > + > + if (dip_ctx->dip_worklist.queue_cnt != 0) > + dev_dbg(&dip_dev->pdev->dev, > + "dip_worklist is not empty (%d)\n", > + dip_ctx->dip_worklist.queue_cnt); > + > + list_for_each_entry_safe(dip_userid, tmp_id, > + &dip_ctx->dip_useridlist.queue, > + list_entry) { > + list_del(&dip_userid->list_entry); > + dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n", > + dip_userid->id); > + kfree(dip_userid); > + dip_ctx->dip_useridlist.queue_cnt--; > + } > + > + if (dip_ctx->dip_useridlist.queue_cnt != 0) > + dev_dbg(&dip_dev->pdev->dev, > + "dip_useridlist is not empty (%d)\n", > + dip_ctx->dip_useridlist.queue_cnt); > + > + flush_workqueue(dip_ctx->mdpcb_workqueue); > + destroy_workqueue(dip_ctx->mdpcb_workqueue); > + dip_ctx->mdpcb_workqueue = NULL; > + > + flush_workqueue(dip_ctx->composer_wq); > + destroy_workqueue(dip_ctx->composer_wq); > + dip_ctx->composer_wq = NULL; > + > + atomic_set(&dip_ctx->num_composing, 0); > + atomic_set(&dip_ctx->num_running, 0); > + > + kthread_stop(dip_ctx->dip_runner_thread.thread); > + dip_ctx->dip_runner_thread.thread = NULL; > + > + atomic_set(&dip_ctx->dip_user_cnt, 0); > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > + > + /* All the buffer should be in the freebufferlist when release */ > + list_for_each_entry_safe(buf, tmpbuf, > + &dip_ctx->dip_freebufferlist.queue, > + list_entry) { > + struct sg_table *sgt = &buf->table; > + > + dev_dbg(&dip_dev->pdev->dev, > + "buf pa (%d): %x\n", i, buf->buffer.pa); > + dip_ctx->dip_freebufferlist.queue_cnt--; > + dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl, > + sgt->orig_nents, > + DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC); > + sg_free_table(sgt); > + list_del(&buf->list_entry); > + kfree(buf); > + buf = NULL; > + i++; > + } > + > + if (dip_ctx->dip_freebufferlist.queue_cnt != 0 && > + i != DIP_SUB_FRM_DATA_NUM) > + dev_err(&dip_dev->pdev->dev, > + "dip_freebufferlist is not empty (%d/%d)\n", > + dip_ctx->dip_freebufferlist.queue_cnt, i); > + > + mutex_destroy(&dip_ctx->dip_useridlist.queuelock); > + mutex_destroy(&dip_ctx->dip_worklist.queuelock); > + mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock); > + mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock); > + > + return 0; > +} > + ... > +static int mtk_dip_probe(struct platform_device *pdev) > +{ > + struct mtk_isp_dip_drv_data *dip_drv; > + struct dip_device *dip_dev; > + struct mtk_dip_hw_ctx *dip_ctx; > + struct device_node *node; > + struct platform_device *larb_pdev; > + > + int ret = 0; > + > + dev_info(&pdev->dev, "E. DIP driver probe.\n"); > + > + dip_drv = devm_kzalloc(&pdev->dev, sizeof(*dip_drv), GFP_KERNEL); Need to check for NULL. > + dev_set_drvdata(&pdev->dev, dip_drv); > + dip_dev = &dip_drv->dip_dev; > + > + if (!dip_dev) > + return -ENOMEM; > + > + dev_info(&pdev->dev, "Created dip_dev = 0x%p\n", dip_dev); > + > + dip_dev->pdev = pdev; > + dip_ctx = &dip_dev->dip_ctx; > + > + node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0); > + if (!node) { > + dev_err(&pdev->dev, "no mediatek,larb found"); > + return -EINVAL; > + } > + larb_pdev = of_find_device_by_node(node); > + if (!larb_pdev) { > + dev_err(&pdev->dev, "no mediatek,larb device found"); > + return -EINVAL; > + } > + dip_dev->larb_dev = &larb_pdev->dev; > + > + /*CCF: Grab clock pointer (struct clk*) */ Add a space before 'CCF'. > + dip_dev->dip_clk.DIP_IMG_LARB5 = devm_clk_get(&pdev->dev, > + "DIP_CG_IMG_LARB5"); > + dip_dev->dip_clk.DIP_IMG_DIP = devm_clk_get(&pdev->dev, > + "DIP_CG_IMG_DIP"); > + if (IS_ERR(dip_dev->dip_clk.DIP_IMG_LARB5)) { > + dev_err(&pdev->dev, "cannot get DIP_IMG_LARB5 clock\n"); > + return PTR_ERR(dip_dev->dip_clk.DIP_IMG_LARB5); > + } > + if (IS_ERR(dip_dev->dip_clk.DIP_IMG_DIP)) { > + dev_err(&pdev->dev, "cannot get DIP_IMG_DIP clock\n"); > + return PTR_ERR(dip_dev->dip_clk.DIP_IMG_DIP); > + } > + > + pm_runtime_enable(&pdev->dev); > + > + atomic_set(&dip_ctx->dip_user_cnt, 0); > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > + > + atomic_set(&dip_ctx->num_composing, 0); > + atomic_set(&dip_ctx->num_running, 0); > + > + dip_ctx->dip_worklist.queue_cnt = 0; > + > + ret = mtk_dip_ctx_dip_v4l2_init(pdev, > + &dip_drv->isp_preview_dev, > + &dip_drv->isp_capture_dev); > + > + if (ret) > + dev_err(&pdev->dev, "v4l2 init failed: %d\n", ret); > + > + dev_info(&pdev->dev, "X. DIP driver probe.\n"); > + > + return ret; > +} > + > +static int mtk_dip_remove(struct platform_device *pdev) > +{ > + struct mtk_isp_dip_drv_data *drv_data = > + dev_get_drvdata(&pdev->dev); > + > + /* */ What's with the empty comments? Here and below. > + if (drv_data) { > + mtk_dip_dev_core_release(pdev, &drv_data->isp_preview_dev); > + mtk_dip_dev_core_release(pdev, &drv_data->isp_capture_dev); > + dev_info(&pdev->dev, "E. %s\n", __func__); Remove this line. > + } > + > + pm_runtime_disable(&pdev->dev); > + > + /* */ > + return 0; > +} > + ... > +static struct platform_driver mtk_dip_driver = { > + .probe = mtk_dip_probe, > + .remove = mtk_dip_remove, > + .driver = { > + .name = DIP_DEV_NAME, > + .owner = THIS_MODULE, You don't need the .owner line. module_platform_driver() / platform_driver_register() takes care of this for you. Brian > + .of_match_table = dip_of_ids, > + .pm = &mtk_dip_pm_ops, > + } > +}; > + > +module_platform_driver(mtk_dip_driver); > + > +MODULE_DESCRIPTION("Camera DIP driver"); > +MODULE_LICENSE("GPL"); ... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <1550756198.11724.86.camel@mtksdccf07>]
* Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver [not found] ` <1550756198.11724.86.camel@mtksdccf07> @ 2019-02-23 6:18 ` Frederic Chen 2019-02-28 3:24 ` Brian Norris 0 siblings, 1 reply; 19+ messages in thread From: Frederic Chen @ 2019-02-23 6:18 UTC (permalink / raw) To: Brian Norris Cc: Sean Cheng (鄭昇弘), laurent.pinchart+renesas, Rynn Wu (吳育恩), srv_heupstream, Holmes Chiou (邱挺), Jerry-ch Chen (陳敬憲), tfiga, Jungo Lin (林明俊), Sj Huang (黃信璋), hans.verkuil, linux-mediatek, matthias.bgg, Christie Yu (游雅惠), mchehab, linux-arm-kernel, linux-media Dear Brian, I appreciate your comments. I'm really sorry for the delay in responding to the comments due to some mail subscribing failed issue inside my company. On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote: > Re-sent to Frederic due to company Mail system issue. > > On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote: > > Hi, > > > > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote: > > > This patch adds the driver of Digital Image Processing (DIP) > > > unit in Mediatek ISP system, providing image format conversion, > > > resizing, and rotation features. > > > > > > The mtk-isp directory will contain drivers for multiple IP > > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > > driver (CAM), sensor interface driver, DIP driver and face > > > detection driver. > > > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > > --- > > > drivers/media/platform/mtk-isp/Makefile | 18 + > > > drivers/media/platform/mtk-isp/isp_50/Makefile | 17 + > > > drivers/media/platform/mtk-isp/isp_50/dip/Makefile | 35 + > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h | 188 +++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c | 173 +++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h | 43 + > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h | 319 ++++ > > > .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c | 1643 ++++++++++++++++++++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c | 374 +++++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h | 191 +++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c | 452 ++++++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h | 25 + > > > .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c | 1000 ++++++++++++ > > > .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h | 38 + > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c | 292 ++++ > > > .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h | 60 + > > > .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c | 1385 +++++++++++++++++ > > > .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h | 93 ++ > > > 18 files changed, 6346 insertions(+) > > > create mode 100644 drivers/media/platform/mtk-isp/Makefile > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-core.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h > > > > > > > ... > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > > > new file mode 100644 > > > index 0000000..9d29507 > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > > > @@ -0,0 +1,173 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (c) 2018 MediaTek Inc. > > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License version 2 as > > > + * published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > + > > > +#include <linux/device.h> > > > +#include <linux/platform_device.h> > > > +#include "mtk_dip-dev.h" > > > +#include "mtk_dip-ctrl.h" > > > + > > > +#define CONFIG_MTK_DIP_COMMON_UT > > > > Please don't do this. You're pretending to have configurability that you > > don't actually have. > > I got it. I will remove CONFIG_MTK_DIP_COMMON_UT and other similar macro in the next patch. > > > + > > > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl); > > > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl); > > > +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl); > > > + > > > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl) > > > +{ > > > + struct mtk_dip_ctx_queue *queue = > > > + container_of(ctrl->handler, > > > + struct mtk_dip_ctx_queue, ctrl_handler); > > > + > > > + if (ctrl->val < MTK_DIP_V4l2_BUF_USAGE_DEFAULT || > > > + ctrl->val >= MTK_DIP_V4l2_BUF_USAGE_NONE) { > > > + pr_err("Invalid buffer usage id %d", ctrl->val); > > > + return; > > > + } > > > + queue->buffer_usage = ctrl->val; > > > +} > > > + > > > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl) > > > +{ > > > + struct mtk_dip_ctx_queue *queue = > > > + container_of(ctrl->handler, > > > + struct mtk_dip_ctx_queue, ctrl_handler); > > > + > > > + if (ctrl->val != 0 || ctrl->val != 90 || > > > + ctrl->val != 180 || ctrl->val != 270) { > > > + pr_err("Invalid buffer rotation %d", ctrl->val); > > > + return; > > > + } > > > + queue->rotation = ctrl->val; > > > +} > > > + > > > +static const struct v4l2_ctrl_ops mtk_dip_ctx_ctrl_ops = { > > > + .s_ctrl = mtk_dip_ctx_s_ctrl, > > > +}; > > > + > > > +#ifdef CONFIG_MTK_DIP_COMMON_UT > > > > Kill the #ifdef if you're not actually going to make this a Kconfig. > > Same elsewhere. > > I will remove CONFIG_MTK_DIP_COMMON_UT and the related codes in the next patch. > > > + > > > +static void handle_ctrl_common_util_ut_set_debug_mode > > > + (struct v4l2_ctrl *ctrl) > > > +{ > > > + struct mtk_dip_ctx *dev_ctx = > > > + container_of(ctrl->handler, struct mtk_dip_ctx, ctrl_handler); > > > + dev_ctx->mode = ctrl->val; > > > + dev_dbg(&dev_ctx->pdev->dev, "Set ctx(id = %d) mode to %d\n", > > > + dev_ctx->ctx_id, dev_ctx->mode); > > > +} > > > + > > > +static const struct v4l2_ctrl_config mtk_dip_mode_config = { > > > + .ops = &mtk_dip_ctx_ctrl_ops, > > > + .id = V4L2_CID_PRIVATE_SET_CTX_MODE_NUM, > > > + .name = "MTK ISP UNIT TEST CASE", > > > + .type = V4L2_CTRL_TYPE_INTEGER, > > > + .min = 0, > > > + .max = 65535, > > > + .step = 1, > > > + .def = 0, > > > + .flags = V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE, > > > +}; > > > +#endif /* CONFIG_MTK_DIP_COMMON_UT */ > > > + > > > +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl) > > > +{ > > > + switch (ctrl->id) { > > > + #ifdef CONFIG_MTK_DIP_COMMON_UT > > > + case V4L2_CID_PRIVATE_SET_CTX_MODE_NUM: > > > + handle_ctrl_common_util_ut_set_debug_mode(ctrl); > > > + break; > > > + #endif /* CONFIG_MTK_DIP_COMMON_UT */ > > > + default: > > > + break; > > > + } > > > + return 0; > > > +} > > > + > > > > ... > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > > > new file mode 100644 > > > index 0000000..c735919 > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > > > @@ -0,0 +1,1643 @@ > > ... > > > > > +#ifdef MTK_DIP_CTX_DIP_V4L2_UT > > > > What is this #ifdef'ery? I don't see MTK_DIP_CTX_DIP_V4L2_UT anywhere. > > MTK_DIP_CTX_DIP_V4L2_UT is only used for our internal test. I will also remove it in the next patch. > > > +static int check_and_refill_dip_ut_start_ipi_param > > > + (struct img_ipi_frameparam *ipi_param, > > > + struct mtk_dip_ctx_buffer *ctx_buf_in, > > > + struct mtk_dip_ctx_buffer *ctx_buf_out) > > > +{ > > > + /* Check the buffer size information from user space */ > > > + int ret = 0; > > > + unsigned char *buffer_ptr = NULL; > > > + const unsigned int src_width = 3264; > > ... > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > > > new file mode 100644 > > > index 0000000..b425031 > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > > > @@ -0,0 +1,1000 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (c) 2018 Mediatek Corporation. > > > + * Copyright (c) 2017 Intel Corporation. > > > + * > > > + * This program is free software; you can redistribute it and/or > > > + * modify it under the terms of the GNU General Public License version > > > + * 2 as published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + * > > > + * MTK_DIP-v4l2 is highly based on Intel IPU 3 chrome driver > > > + * > > > + */ > > > + > > > +#include <linux/module.h> > > > +#include <linux/pm_runtime.h> > > > +#include <linux/videodev2.h> > > > +#include <media/v4l2-ioctl.h> > > > +#include <media/videobuf2-dma-contig.h> > > > +#include <media/v4l2-subdev.h> > > > +#include <media/v4l2-event.h> > > > +#include <linux/device.h> > > > +#include <linux/platform_device.h> > > > + > > > +#include "mtk_dip-dev.h" > > > +#include "mtk_dip-v4l2-util.h" > > > + > > > +static u32 mtk_dip_node_get_v4l2_cap > > > + (struct mtk_dip_ctx_queue *node_ctx); > > > + > > > +static int mtk_dip_videoc_s_meta_fmt(struct file *file, > > > + void *fh, struct v4l2_format *f); > > > + > > > +static int mtk_dip_subdev_open(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_fh *fh) > > > +{ > > > + struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd); > > > + > > > + isp_dev->ctx.fh = fh; > > > + > > > + return mtk_dip_ctx_open(&isp_dev->ctx); > > > +} > > > + > > > +static int mtk_dip_subdev_close(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_fh *fh) > > > +{ > > > + struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd); > > > + > > > + return mtk_dip_ctx_release(&isp_dev->ctx); > > > +} > > > + > > > +static int mtk_dip_subdev_s_stream(struct v4l2_subdev *sd, > > > + int enable) > > > +{ > > > + int ret = 0; > > > + > > > + struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd); > > > + > > > + if (enable) { > > > + ret = mtk_dip_ctx_streamon(&isp_dev->ctx); > > > + > > > + if (!ret) > > > + ret = mtk_dip_dev_queue_buffers > > > + (mtk_dip_ctx_to_dev(&isp_dev->ctx), 1); > > > + if (ret) > > > + pr_err("failed to queue initial buffers (%d)", ret); > > > + } else { > > > + ret = mtk_dip_ctx_streamoff(&isp_dev->ctx); > > > + } > > > + > > > + if (!ret) > > > + isp_dev->mem2mem2.streaming = enable; > > > + > > > + return ret; > > > +} > > > + > > > +int mtk_dip_subdev_subscribe_event(struct v4l2_subdev *subdev, > > > + struct v4l2_fh *fh, > > > + struct v4l2_event_subscription *sub) > > > > Should be static. I got it. > > > > > +{ > > > + pr_info("sub type(%x)", sub->type); > > > > I feel like you have this problem in other places too: this definitely > > shouldn't be at KERN_INFO level. It seems a bit excessive anyway. I will use KERN_DEBUG instead and check the same problem in other places. > > > > > + if (sub->type != V4L2_EVENT_PRIVATE_START && > > > + sub->type != V4L2_EVENT_MTK_DIP_FRAME_DONE) > > > + return -EINVAL; > > > + > > > + return v4l2_event_subscribe(fh, sub, 0, NULL); > > > +} > > > + > > > +int mtk_dip_subdev_unsubscribe_event(struct v4l2_subdev *subdev, > > > + struct v4l2_fh *fh, > > > > Static. > > I got it. > > > + struct v4l2_event_subscription *sub) > > > +{ > > > + return v4l2_event_unsubscribe(fh, sub); > > > +} > > > + > > > +static int mtk_dip_link_setup(struct media_entity *entity, > > > + const struct media_pad *local, > > > + const struct media_pad *remote, u32 flags) > > > +{ > > > + struct mtk_dip_mem2mem2_device *m2m2 = > > > + container_of(entity, > > > + struct mtk_dip_mem2mem2_device, > > > + subdev.entity); > > > + struct mtk_dip_dev *isp_dev = > > > + container_of(m2m2, struct mtk_dip_dev, mem2mem2); > > > + > > > + u32 pad = local->index; > > > + > > > + dev_dbg(&isp_dev->pdev->dev, > > > + "link setup: %d --> %d\n", pad, remote->index); > > > + > > > + WARN_ON(entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV); > > > + > > > + WARN_ON(pad >= m2m2->num_nodes); > > > + > > > + m2m2->nodes[pad].enabled = !!(flags & MEDIA_LNK_FL_ENABLED); > > > + > > > + /* queue_enable can be phase out in the future since */ > > > + /* we don't have internal queue of each node in */ > > > + /* v4l2 common module */ > > > + isp_dev->queue_enabled[pad] = m2m2->nodes[pad].enabled; > > > + > > > + return 0; > > > +} > > > + > > > +static void mtk_dip_vb2_buf_queue(struct vb2_buffer *vb) > > > +{ > > > + struct mtk_dip_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > > > + > > > + struct mtk_dip_dev *mtk_dip_dev = mtk_dip_m2m_to_dev(m2m2); > > > + > > > + struct device *dev = &mtk_dip_dev->pdev->dev; > > > + > > > + struct mtk_dip_dev_buffer *buf = NULL; > > > + > > > + struct vb2_v4l2_buffer *v4l2_buf = NULL; > > > + > > > + struct mtk_dip_dev_video_device *node = > > > + mtk_dip_vbq_to_isp_node(vb->vb2_queue); > > > + > > > + int queue = mtk_dip_dev_get_queue_id_of_dev_node(mtk_dip_dev, node); > > > > You've got a lot of extra blank lines in here. > > I will remove them in the next patch. > > > + > > > + dev_dbg(dev, > > > + "queue vb2_buf: Node(%s) queue id(%d)\n", > > > + node->name, > > > + queue); > > > + > > > + if (queue < 0) { > > > + dev_err(m2m2->dev, "Invalid mtk_dip_dev node.\n"); > > > + return; > > > + } > > > + > > > + if (mtk_dip_dev->ctx.mode == MTK_DIP_CTX_MODE_DEBUG_BYPASS_ALL) { > > > + dev_dbg(m2m2->dev, "By pass mode, just loop back the buffer\n"); > > > + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); > > > + return; > > > + } > > > + > > > + if (!vb) > > > + pr_err("VB can't be null\n"); > > > + > > > + buf = mtk_dip_vb2_buf_to_dev_buf(vb); > > > + > > > + if (!buf) > > > + pr_err("buf can't be null\n"); > > > + > > > + v4l2_buf = to_vb2_v4l2_buffer(vb); > > > + > > > + if (!v4l2_buf) > > > + pr_err("v4l2_buf can't be null\n"); > > > + > > > + mutex_lock(&mtk_dip_dev->lock); > > > + > > > + /* the dma address will be filled in later frame buffer handling*/ > > > + mtk_dip_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0); > > > + > > > + /* Added the buffer into the tracking list */ > > > + list_add_tail(&buf->m2m2_buf.list, > > > + &m2m2->nodes[node - m2m2->nodes].buffers); > > > + mutex_unlock(&mtk_dip_dev->lock); > > > + > > > + /* Enqueue the buffer */ > > > + if (mtk_dip_dev->mem2mem2.streaming) { > > > + dev_dbg(dev, "%s: mtk_dip_dev_queue_buffers\n", > > > + node->name); > > > + mtk_dip_dev_queue_buffers(mtk_dip_dev, 0); > > > + } > > > +} > > ... > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > > > new file mode 100644 > > > index 0000000..ffdc45e > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > > > @@ -0,0 +1,292 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (c) 2018 MediaTek Inc. > > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License version 2 as > > > + * published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > + > > > +#include <linux/device.h> > > > +#include <linux/platform_device.h> > > > +#include "mtk_dip-ctx.h" > > > +#include "mtk_dip.h" > > > +#include "mtk_dip-v4l2.h" > > > +#include "mtk-mdp3-regs.h" > > > + > > > +#define MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME "MTK-ISP-DIP-V4L2" > > > +#define MTK_DIP_DEV_DIP_PREVIEW_NAME \ > > > + MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME > > > +#define MTK_DIP_DEV_DIP_CAPTURE_NAME "MTK-ISP-DIP-CAP-V4L2" > > > + > > > +#ifdef MTK_DIP_CTX_DIP_V4L2_UT > > > +#include "mtk_dip-dev-ctx-dip-test.h" > > > > The above macros was never defined, and this header doesn't exist. > > Please remove. > > I will remove them in the next patch. > > > +#endif > > > + > > > > ... > > > > > +int mtk_dip_ctx_dip_v4l2_init(struct platform_device *pdev, > > > + struct mtk_dip_dev *isp_preview_dev, > > > + struct mtk_dip_dev *isp_capture_dev) > > > +{ > > > + struct media_device *media_dev; > > > + struct v4l2_device *v4l2_dev; > > > + struct v4l2_ctrl_handler *ctrl_handler; > > > + int ret = 0; > > > + > > > + /* initialize the v4l2 common part */ > > > + dev_info(&pdev->dev, "init v4l2 common part: dev=%llx\n", > > > + (unsigned long long)&pdev->dev); > > > + > > > + media_dev = &isp_preview_dev->media_dev; > > > + v4l2_dev = &isp_preview_dev->v4l2_dev; > > > + ctrl_handler = &isp_preview_dev->ctx.ctrl_handler; > > > + > > > + ret = mtk_dip_media_register(&pdev->dev, > > > + media_dev, > > > + MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME); > > > + > > > + ret = mtk_dip_v4l2_register(&pdev->dev, > > > + media_dev, > > > + v4l2_dev, > > > + ctrl_handler); > > > + > > > + dev_info(&pdev->dev, "init v4l2 preview part\n"); > > > + ret = mtk_dip_dev_core_init_ext(pdev, > > > + isp_preview_dev, > > > + &mtk_dip_ctx_desc_dip_preview, > > > + media_dev, v4l2_dev); > > > + > > > + if (ret) > > > + dev_err(&pdev->dev, "Preview v4l2 part init failed: %d\n", ret); > > > + > > > + dev_info(&pdev->dev, "init v4l2 capture part\n"); > > > + > > > + ret = mtk_dip_dev_core_init_ext(pdev, > > > + isp_capture_dev, > > > + &mtk_dip_ctx_desc_dip_capture, > > > + media_dev, v4l2_dev); > > > + > > > + if (ret) > > > + dev_err(&pdev->dev, "Capture v4l2 part init failed: %d\n", ret); > > > + > > > + return ret; > > > +} > > > + > > > +/* MTK ISP context initialization */ > > > +int mtk_dip_ctx_dip_preview_init(struct mtk_dip_ctx *preview_ctx) > > > +{ > > > + preview_ctx->ctx_id = MTK_DIP_CTX_P2_ID_PREVIEW; > > > + > > > + /* Initialize main data structure */ > > > + mtk_dip_ctx_core_queue_setup(preview_ctx, &preview_queues_setting); > > > + > > > + return mtk_dip_ctx_core_steup(preview_ctx, > > > + &mtk_dip_ctx_dip_preview_setting); > > > +} > > > +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_preview_init); > > > + > > > +/* MTK ISP context initialization */ > > > +int mtk_dip_ctx_dip_capture_init(struct mtk_dip_ctx *capture_ctx) > > > +{ > > > + capture_ctx->ctx_id = MTK_DIP_CTX_P2_ID_CAPTURE; > > > + /* Initialize main data structure */ > > > + mtk_dip_ctx_core_queue_setup(capture_ctx, &capture_queues_setting); > > > + > > > + return mtk_dip_ctx_core_steup(capture_ctx, > > > + &mtk_dip_ctx_dip_capture_setting); > > > +} > > > +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_capture_init); > > > > ... > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > > > new file mode 100644 > > > index 0000000..3569c7c > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > > > @@ -0,0 +1,1385 @@ > > > +// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note > > > +/* > > > + * Copyright (c) 2018 MediaTek Inc. > > > + * Author: Holmes Chiou <holmes.chiou@mediatek.com> > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License version 2 as > > > + * published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > + > > > +#include <linux/of_device.h> > > > +#include <linux/module.h> > > > +#include <linux/device.h> > > > +#include <linux/kthread.h> /* thread functions */ > > > +#include <linux/pm_runtime.h> > > > +#include <linux/dma-iommu.h> > > > +#include <linux/spinlock.h> > > > +#include <linux/slab.h> /* kzalloc and kfree */ > > > + > > > +#include "mtk_vpu.h" > > > +#include "mtk-mdp3-cmdq.h" > > > + > > > +#include "mtk_dip-dev.h" > > > +#include "mtk_dip.h" > > > +#include "mtk_dip-core.h" > > > +#include "mtk_dip-v4l2.h" > > > + > > > +#define DIP_DEV_NAME "camera-dip" > > > + > > > +#define DIP_COMPOSER_THREAD_TIMEOUT (16U) > > > +#define DIP_COMPOSING_WQ_TIMEOUT (16U) > > > +#define DIP_COMPOSING_MAX_NUM (3) > > > +#define DIP_FLUSHING_WQ_TIMEOUT (16U) > > > + > > > +#define DIP_MAX_ERR_COUNT (188U) > > > + > > > +#define DIP_FRM_SZ (76 * 1024) > > > +#define DIP_SUB_FRM_SZ (16 * 1024) > > > +#define DIP_TUNING_SZ (32 * 1024) > > > +#define DIP_COMP_SZ (24 * 1024) > > > +#define DIP_FRAMEPARAM_SZ (4 * 1024) > > > + > > > +#define DIP_TUNING_OFFSET (DIP_SUB_FRM_SZ) > > > +#define DIP_COMP_OFFSET (DIP_TUNING_OFFSET + DIP_TUNING_SZ) > > > +#define DIP_FRAMEPARAM_OFFSET (DIP_COMP_OFFSET + DIP_COMP_SZ) > > > + > > > +#define DIP_SUB_FRM_DATA_NUM (32) > > > + > > > +#define DIP_SCP_WORKINGBUF_OFFSET (5 * 1024 * 1024) > > > + > > > +#define DIP_GET_ID(x) (((x) & 0xffff0000) >> 16) > > > + > > > +static const struct of_device_id dip_of_ids[] = { > > > + /* Remider: Add this device node manually in .dtsi */ > > > + { .compatible = "mediatek,mt8183-dip", }, > > > + {} > > > +}; > > > > Please add: > > > > MODULE_DEVICE_TABLE(of, dip_of_ids); > > I see. I will add this line in the next patch. > > > + > > > +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev, > > > + struct img_ipi_frameparam *iparam); > > > + > > > +static struct img_frameparam *dip_create_framejob(int sequence) > > > +{ > > > + struct dip_frame_job *fjob = NULL; > > > + > > > + fjob = kzalloc(sizeof(*fjob), GFP_ATOMIC); > > > + > > > + if (!fjob) > > > + return NULL; > > > + > > > + fjob->sequence = sequence; > > > + > > > + return &fjob->fparam; > > > +} > > > + > > > +static void dip_free_framejob(struct img_frameparam *fparam) > > > +{ > > > + struct dip_frame_job *fjob = NULL; > > > + > > > + fjob = mtk_dip_fparam_to_job(fparam); > > > + > > > + /* to avoid use after free issue */ > > > + fjob->sequence = -1; > > > + > > > + kfree(fjob); > > > +} > > > + > > > +static void dip_enable_ccf_clock(struct dip_device *dip_dev) > > > +{ > > > + int ret; > > > + > > > + ret = pm_runtime_get_sync(dip_dev->larb_dev); > > > + if (ret < 0) > > > + dev_err(&dip_dev->pdev->dev, "cannot get smi larb clock\n"); > > > + > > > + ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_LARB5); > > > + if (ret) > > > + dev_err(&dip_dev->pdev->dev, > > > + "cannot prepare and enable DIP_IMG_LARB5 clock\n"); > > > + > > > + ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_DIP); > > > + if (ret) > > > + dev_err(&dip_dev->pdev->dev, > > > + "cannot prepare and enable DIP_IMG_DIP clock\n"); > > > +} > > > + > > > +static void dip_disable_ccf_clock(struct dip_device *dip_dev) > > > +{ > > > + clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_DIP); > > > + clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_LARB5); > > > + pm_runtime_put_sync(dip_dev->larb_dev); > > > +} > > > + > > > +static int dip_send(struct platform_device *pdev, enum ipi_id id, > > > + void *buf, unsigned int len, unsigned int wait) > > > +{ > > > + vpu_ipi_send_sync_async(pdev, id, buf, len, wait); > > > + return 0; > > > +} > > > + > > > +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev, > > > + struct img_ipi_frameparam *iparam) > > > +{ > > > + struct mtk_dip_ctx_finish_param fparam; > > > + struct mtk_isp_dip_drv_data *drv_data; > > > + struct mtk_dip_ctx *dev_ctx; > > > + int ctx_id = 0; > > > + int r = 0; > > > + > > > + if (!dip_dev) { > > > + pr_err("Can't update buffer status, dip_dev can't be NULL\n"); > > > + return; > > > + } > > > + > > > + if (!iparam) { > > > + dev_err(&dip_dev->pdev->dev, > > > + "%s: iparam can't be NULL\n", __func__); > > > + return; > > > + } > > > + > > > + drv_data = dip_dev_to_drv(dip_dev); > > > + > > > + frame_param_ipi_to_ctx(iparam, &fparam); > > > + ctx_id = MTK_DIP_GET_CTX_ID_FROM_SEQUENCE(fparam.frame_id); > > > + > > > + if (ctx_id == MTK_DIP_CTX_P2_ID_PREVIEW) { > > > + dev_ctx = &drv_data->isp_preview_dev.ctx; > > > + } else if (ctx_id == MTK_DIP_CTX_P2_ID_CAPTURE) { > > > + dev_ctx = &drv_data->isp_capture_dev.ctx; > > > + } else { > > > + dev_err(&dip_dev->pdev->dev, > > > + "unknown ctx id: %d\n", ctx_id); > > > + return; > > > + } > > > + > > > + r = mtk_dip_ctx_core_job_finish(dev_ctx, &fparam); > > > + > > > + if (r) > > > + dev_err(&dip_dev->pdev->dev, "finish op failed: %d\n", > > > + r); > > > + dev_dbg(&dip_dev->pdev->dev, "Ready to return buffers: CTX(%d), Frame(%d)\n", > > > + ctx_id, fparam.frame_id); > > > +} > > > + > > > +static void mtk_dip_notify(void *data) > > > +{ > > > + struct dip_device *dip_dev; > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > + struct img_frameparam *framejob; > > > + struct dip_user_id *user_id; > > > + struct dip_subframe *buf, *tmpbuf; > > > + struct img_ipi_frameparam *frameparam; > > > + u32 num; > > > + bool found = false; > > > + > > > + frameparam = (struct img_ipi_frameparam *)data; > > > + dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data; > > > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > > > + framejob = container_of(frameparam, > > > + struct img_frameparam, > > > + frameparam); > > > + > > > + if (frameparam->state == FRAME_STATE_HW_TIMEOUT) { > > > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > > > + (void *)frameparam, sizeof(*frameparam), 0); > > > + dev_err(&dip_dev->pdev->dev, "frame no(%d) HW timeout\n", > > > + frameparam->frame_no); > > > + } > > > + > > > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > > > + list_for_each_entry_safe(buf, tmpbuf, > > > + &dip_ctx->dip_usedbufferlist.queue, > > > + list_entry) { > > > + if (buf->buffer.pa == frameparam->subfrm_data.pa) { > > > + list_del(&buf->list_entry); > > > + dip_ctx->dip_usedbufferlist.queue_cnt--; > > > + found = true; > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "Find used buffer (%x)\n", buf->buffer.pa); > > > + break; > > > + } > > > + } > > > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > > > + > > > + if (!found) { > > > + dev_err(&dip_dev->pdev->dev, > > > + "frame_no(%d) buffer(%x) used buffer count(%d)\n", > > > + frameparam->frame_no, frameparam->subfrm_data.pa, > > > + dip_ctx->dip_usedbufferlist.queue_cnt); > > > + > > > + frameparam->state = FRAME_STATE_ERROR; > > > + > > > + } else { > > > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > > > + list_add_tail(&buf->list_entry, > > > + &dip_ctx->dip_freebufferlist.queue); > > > + dip_ctx->dip_freebufferlist.queue_cnt++; > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > + > > > + frameparam->state = FRAME_STATE_DONE; > > > + } > > > + > > > + call_mtk_dip_ctx_finish(dip_dev, frameparam); > > > + > > > + found = false; > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > + list_for_each_entry(user_id, > > > + &dip_ctx->dip_useridlist.queue, > > > + list_entry) { > > > + if (DIP_GET_ID(frameparam->index) == user_id->id) { > > > + user_id->num--; > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "user_id(%x) is found, and cnt: %d\n", > > > + user_id->id, user_id->num); > > > + found = true; > > > + break; > > > + } > > > + } > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > + wake_up(&dip_ctx->flushing_wq); > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "frame_no(%d) is finished\n", framejob->frameparam.frame_no); > > > + dip_free_framejob(framejob); > > > + > > > + num = atomic_dec_return(&dip_ctx->num_running); > > > + dev_dbg(&dip_dev->pdev->dev, "Running count: %d\n", num); > > > +} > > > + > > > +static void mdp_cb_worker(struct work_struct *work) > > > +{ > > > + struct mtk_mdpcb_work *mdpcb_work; > > > + > > > + mdpcb_work = container_of(work, struct mtk_mdpcb_work, frame_work); > > > + mtk_dip_notify(mdpcb_work->frameparams); > > > + kfree(mdpcb_work); > > > +} > > > + > > > +static struct img_ipi_frameparam *convert_to_fparam(struct cmdq_cb_data *data) > > > +{ > > > + struct device *dev = NULL; > > > > Every use of dev_err() in this function is wrong, since you're > > guaranteed to be NULL. Either come up with some better way to report > > device errors using the pointers you have, or else just switch to > > pr_err(). > > I see. I would like to switch to pr_err(). > > > + struct dip_device *dip_dev = NULL; > > > + struct dip_frame_job *fjob = NULL; > > > + struct img_ipi_frameparam *ipi_fparam = NULL; > > > + > > > + if (!data) { > > > + dev_err(dev, "DIP got NULL in cmdq_cb_data,%s\n", > > > + __func__); > > > + return NULL; > > > + } > > > + > > > + if (data->sta != CMDQ_CB_NORMAL) { > > > + dev_warn(dev, "DIP got CMDQ CB (%d) without CMDQ_CB_NORMAL\n", > > > + data->sta); > > > + } > > > + > > > + if (!data->data) { > > > + dev_err(dev, "DIP got NULL data in cmdq_cb_data,%s\n", > > > + __func__); > > > + return NULL; > > > + } > > > + > > > + fjob = mtk_dip_ipi_fparam_to_job(data->data); > > > + > > > + if (fjob->sequence == -1) { > > > + dev_err(dev, "Invalid cmdq_cb_data(%llx)\n", > > > + (unsigned long long)data); > > > + ipi_fparam = NULL; > > > + } else { > > > + ipi_fparam = &fjob->fparam.frameparam; > > > + dip_dev = dip_hw_ctx_to_dev((void *)ipi_fparam->drv_data); > > > + dev = &dip_dev->pdev->dev; > > > + } > > > + > > > + dev_dbg(dev, "framejob(0x%llx,seq:%d):\n", > > > + (unsigned long long)fjob, fjob->sequence); > > > + dev_dbg(dev, "idx(%d),no(%d),s(%d),n_in(%d),n_out(%d),drv(%llx)\n", > > > + fjob->fparam.frameparam.index, > > > + fjob->fparam.frameparam.frame_no, > > > + fjob->fparam.frameparam.state, > > > + fjob->fparam.frameparam.num_inputs, > > > + fjob->fparam.frameparam.num_outputs, > > > + (unsigned long long)fjob->fparam.frameparam.drv_data > > > + ); > > > + > > > + return ipi_fparam; > > > +} > > > + > > > +/* Maybe in IRQ context of cmdq */ > > > +void dip_mdp_cb_func(struct cmdq_cb_data data) > > > > Make this static. > > I got it. > > > +{ > > > + struct img_ipi_frameparam *frameparam; > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > + struct mtk_mdpcb_work *mdpcb_work; > > > + > > > + frameparam = convert_to_fparam(&data); > > > + > > > + if (!frameparam) { > > > + dev_err(NULL, "%s return due to invalid cmdq_cb_data(%llx)", > > > > Don't directly pass NULL to dev_err(). Just use pr_err() or similar. > > I will use pr_err() in the next patch. > > > + __func__, &data); > > > + return; > > > + } > > > + > > > + dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data; > > > + > > > + mdpcb_work = kzalloc(sizeof(*mdpcb_work), GFP_ATOMIC); > > > + WARN_ONCE(!mdpcb_work, "frame_no(%d) is lost", frameparam->frame_no); > > > + if (!mdpcb_work) > > > + return; > > > + > > > + INIT_WORK(&mdpcb_work->frame_work, mdp_cb_worker); > > > + mdpcb_work->frameparams = frameparam; > > > + if (data.sta != CMDQ_CB_NORMAL) > > > + mdpcb_work->frameparams->state = FRAME_STATE_HW_TIMEOUT; > > > + > > > + queue_work(dip_ctx->mdpcb_workqueue, &mdpcb_work->frame_work); > > > +} > > > + > > > +static void dip_vpu_handler(void *data, unsigned int len, void *priv) > > > +{ > > > + struct img_frameparam *framejob; > > > + struct img_ipi_frameparam *frameparam; > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > + struct dip_device *dip_dev; > > > + unsigned long flags; > > > + u32 num; > > > + > > > + WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__); > > > + if (!data) > > > > You can combine these lines: > > > > > > if (WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__)) > > return; > > I got it. I will combine them. > > > + return; > > > + > > > + frameparam = (struct img_ipi_frameparam *)data; > > > + > > > + framejob = dip_create_framejob(frameparam->index); > > > + WARN_ONCE(!framejob, "frame_no(%d) is lost", frameparam->frame_no); > > > + if (!framejob) > > > > Same here. > > I will also combine them here. > > > + return; > > > + > > > + dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data; > > > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > > > + > > > + wake_up(&dip_ctx->composing_wq); > > > + memcpy(&framejob->frameparam, data, sizeof(framejob->frameparam)); > > > + num = atomic_dec_return(&dip_ctx->num_composing); > > > + > > > + spin_lock_irqsave(&dip_ctx->dip_gcejoblist.queuelock, flags); > > > + list_add_tail(&framejob->list_entry, &dip_ctx->dip_gcejoblist.queue); > > > + dip_ctx->dip_gcejoblist.queue_cnt++; > > > + spin_unlock_irqrestore(&dip_ctx->dip_gcejoblist.queuelock, flags); > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "frame_no(%d) is back, composing num: %d\n", > > > + frameparam->frame_no, num); > > > + > > > + wake_up(&dip_ctx->dip_runner_thread.wq); > > > +} > > > + > > > > ... > > > +static int dip_runner_func(void *data) > > > +{ > > > > ... > > > > > + > > > + mdp_cmdq_sendtask > > > > I don't see this defined anywhere? > > mdp_cmdq_sendtask() is defined in MDP 3 driver. We will send the RFC patch for Mediatek MDP 3 driver by 2/28. > > > + (dip_ctx->mdp_pdev, > > > + (struct img_config *) > > > + framejob->frameparam.config_data.va, > > > + &framejob->frameparam, NULL, false, > > > + dip_mdp_cb_func, > > > + (void *)&framejob->frameparam); > > > + > > ... > > > + return 0; > > > +} > > > + > > > +static void dip_submit_worker(struct work_struct *work) > > > +{ > > > + struct mtk_dip_submit_work *dip_submit_work = > > > + container_of(work, struct mtk_dip_submit_work, frame_work); > > > + > > > + struct mtk_dip_hw_ctx *dip_ctx = dip_submit_work->dip_ctx; > > > + struct mtk_dip_work *dip_work; > > > + struct dip_device *dip_dev; > > > + struct dip_subframe *buf; > > > + u32 len, num; > > > + int ret; > > > + > > > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > > > + num = atomic_read(&dip_ctx->num_composing); > > > + > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > + dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, > > > + struct mtk_dip_work, list_entry); > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > I see you grab the head of the list here, but then you release the lock. > > Then you later assume that reference is still valid, throughout this > > function. > > > > That's usually true, because you only remove/delete entries from this > > list within this same workqueue (at the end of this function). But it's > > not true in dip_release_context() (which doesn't even grab the lock, > > BTW). > > > > I think there could be several ways to solve this, but judging by how > > this list entry is used...couldn't you just remove it from the list > > here, while holding the lock? Then you only have to kfree() it when > > you're done under the free_work_list label. > > I see. I would like to modify the codes as following: mutex_lock(&dip_ctx->dip_useridlist.queuelock); dip_work->user_id->num--; list_del(&dip_work->list_entry); dip_ctx->dip_worklist.queue_cnt--; len = dip_ctx->dip_worklist.queue_cnt; mutex_unlock(&dip_ctx->dip_useridlist.queuelock); goto free_work_list; /* ...... */ free_work_list: kfree(dip_work); > > > + > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > + if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > + > > > + dip_work->frameparams.state = FRAME_STATE_STREAMOFF; > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > + > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > + dip_work->user_id->num--; > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n", > > > + dip_work->user_id->id, dip_work->user_id->num, > > > + dip_work->frameparams.frame_no, > > > + dip_work->frameparams.index); > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > + > > > + goto free_work_list; > > > + } > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > + > > > + while (num >= DIP_COMPOSING_MAX_NUM) { > > > + ret = wait_event_interruptible_timeout > > > + (dip_ctx->composing_wq, > > > + (num < DIP_COMPOSING_MAX_NUM), > > > + msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT)); > > > + > > > + if (ret == -ERESTARTSYS) > > > + dev_err(&dip_dev->pdev->dev, > > > + "interrupted by a signal!\n"); > > > + else if (ret == 0) > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "timeout frame_no(%d), num: %d\n", > > > + dip_work->frameparams.frame_no, num); > > > + else > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "wakeup frame_no(%d), num: %d\n", > > > + dip_work->frameparams.frame_no, num); > > > + > > > + num = atomic_read(&dip_ctx->num_composing); > > > + }; > > > + > > > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > > > + if (list_empty(&dip_ctx->dip_freebufferlist.queue)) { > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > + > > > + dev_err(&dip_dev->pdev->dev, > > > + "frame_no(%d) index: %x no free buffer: %d\n", > > > + dip_work->frameparams.frame_no, > > > + dip_work->frameparams.index, > > > + dip_ctx->dip_freebufferlist.queue_cnt); > > > + > > > + /* Call callback to notify V4L2 common framework > > > + * for failure of enqueue > > > + */ > > > + dip_work->frameparams.state = FRAME_STATE_ERROR; > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > + > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > + dip_work->user_id->num--; > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > + > > > + goto free_work_list; > > > + } > > > + > > > + buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue, > > > + struct dip_subframe, > > > + list_entry); > > > + list_del(&buf->list_entry); > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > + > > > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > > > + list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue); > > > + dip_ctx->dip_usedbufferlist.queue_cnt++; > > > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > > > + > > > + memcpy(&dip_work->frameparams.subfrm_data, > > > + &buf->buffer, sizeof(buf->buffer)); > > > + > > > + memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ); > > > + > > > + memcpy(&dip_work->frameparams.config_data, > > > + &buf->config_data, sizeof(buf->config_data)); > > > + > > > + memset((char *)buf->config_data.va, 0, DIP_COMP_SZ); > > > + > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "frame_no(%d) has no tuning_data\n", > > > + dip_work->frameparams.frame_no); > > > + > > > + memcpy(&dip_work->frameparams.tuning_data, > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > + > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > + /* When user enqueued without tuning buffer, > > > + * it would use driver internal buffer. > > > + * So, tuning_data.va should be 0 > > > + */ > > > + dip_work->frameparams.tuning_data.va = 0; > > > + } > > > + > > > + dip_work->frameparams.drv_data = (u64)dip_ctx; > > > + dip_work->frameparams.state = FRAME_STATE_COMPOSING; > > > + > > > + memcpy((void *)buf->frameparam.va, &dip_work->frameparams, > > > + sizeof(dip_work->frameparams)); > > > + > > > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > > > + (void *)&dip_work->frameparams, > > > + sizeof(dip_work->frameparams), 0); > > > + num = atomic_inc_return(&dip_ctx->num_composing); > > > + > > > +free_work_list: > > > + > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > + list_del(&dip_work->list_entry); > > > + dip_ctx->dip_worklist.queue_cnt--; > > > + len = dip_ctx->dip_worklist.queue_cnt; > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "frame_no(%d) index: %x, worklist count: %d, composing num: %d\n", > > > + dip_work->frameparams.frame_no, dip_work->frameparams.index, > > > + len, num); > > > + > > > + kfree(dip_work); > > > +} > > > > ... > > > > > +int dip_open_context(struct dip_device *dip_dev) > > > > Should be static. > > I will change it to static. > > > +{ > > > > ... > > > > > +} > > > + > > > +int dip_release_context(struct dip_device *dip_dev) > > > > Should be static. > > I will change it to static. > > > +{ > > > + u32 i = 0; > > > + struct dip_subframe *buf, *tmpbuf; > > > + struct mtk_dip_work *dip_work, *tmp_work; > > > + struct dip_user_id *dip_userid, *tmp_id; > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > + > > > + dip_ctx = &dip_dev->dip_ctx; > > > + dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n", > > > + dip_ctx->dip_worklist.queue_cnt); > > > + > > > + list_for_each_entry_safe(dip_work, tmp_work, > > > + &dip_ctx->dip_worklist.queue, > > > + list_entry) { > > > > Shouldn't you be holding the mutex for this? Or alternatively, cancel > > any outstanding work and move the flush_workqueue()/destroy_workqueue() > > up. > > > > Similar questions for the other lists we're going through here. > > We missed the mutex holding here. I would like to change the codes as following: mutex_lock(&dip_ctx->dip_worklist.queuelock); list_for_each_entry_safe(dip_work, tmp_work, &dip_ctx->dip_worklist.queue, list_entry) { list_del(&dip_work->list_entry); dip_ctx->dip_worklist.queue_cnt--; kfree(dip_work); } mutex_unlock(&dip_ctx->dip_worklist.queuelock); I will also modify dip_useridlist and dip_ctx->dip_freebufferlist parts like dip_ctx->dip_worklist. > > > + list_del(&dip_work->list_entry); > > > + dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n", > > > + dip_work->frameparams.frame_no); > > > + kfree(dip_work); > > > + dip_ctx->dip_worklist.queue_cnt--; > > > + } > > > + > > > + if (dip_ctx->dip_worklist.queue_cnt != 0) > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "dip_worklist is not empty (%d)\n", > > > + dip_ctx->dip_worklist.queue_cnt); > > > + > > > + list_for_each_entry_safe(dip_userid, tmp_id, > > > + &dip_ctx->dip_useridlist.queue, > > > + list_entry) { > > > + list_del(&dip_userid->list_entry); > > > + dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n", > > > + dip_userid->id); > > > + kfree(dip_userid); > > > + dip_ctx->dip_useridlist.queue_cnt--; > > > + } > > > + > > > + if (dip_ctx->dip_useridlist.queue_cnt != 0) > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "dip_useridlist is not empty (%d)\n", > > > + dip_ctx->dip_useridlist.queue_cnt); > > > + > > > + flush_workqueue(dip_ctx->mdpcb_workqueue); > > > + destroy_workqueue(dip_ctx->mdpcb_workqueue); > > > + dip_ctx->mdpcb_workqueue = NULL; > > > + > > > + flush_workqueue(dip_ctx->composer_wq); > > > + destroy_workqueue(dip_ctx->composer_wq); > > > + dip_ctx->composer_wq = NULL; > > > + > > > + atomic_set(&dip_ctx->num_composing, 0); > > > + atomic_set(&dip_ctx->num_running, 0); > > > + > > > + kthread_stop(dip_ctx->dip_runner_thread.thread); > > > + dip_ctx->dip_runner_thread.thread = NULL; > > > + > > > + atomic_set(&dip_ctx->dip_user_cnt, 0); > > > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > > > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > > > + > > > + /* All the buffer should be in the freebufferlist when release */ > > > + list_for_each_entry_safe(buf, tmpbuf, > > > + &dip_ctx->dip_freebufferlist.queue, > > > + list_entry) { > > > + struct sg_table *sgt = &buf->table; > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "buf pa (%d): %x\n", i, buf->buffer.pa); > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > + dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl, > > > + sgt->orig_nents, > > > + DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC); > > > + sg_free_table(sgt); > > > + list_del(&buf->list_entry); > > > + kfree(buf); > > > + buf = NULL; > > > + i++; > > > + } > > > + > > > + if (dip_ctx->dip_freebufferlist.queue_cnt != 0 && > > > + i != DIP_SUB_FRM_DATA_NUM) > > > + dev_err(&dip_dev->pdev->dev, > > > + "dip_freebufferlist is not empty (%d/%d)\n", > > > + dip_ctx->dip_freebufferlist.queue_cnt, i); > > > + > > > + mutex_destroy(&dip_ctx->dip_useridlist.queuelock); > > > + mutex_destroy(&dip_ctx->dip_worklist.queuelock); > > > + mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock); > > > + mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock); > > > + > > > + return 0; > > > +} > > > + > > > > ... > > > > > +static int mtk_dip_probe(struct platform_device *pdev) > > > +{ > > > + struct mtk_isp_dip_drv_data *dip_drv; > > > + struct dip_device *dip_dev; > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > + struct device_node *node; > > > + struct platform_device *larb_pdev; > > > + > > > + int ret = 0; > > > + > > > + dev_info(&pdev->dev, "E. DIP driver probe.\n"); > > > + > > > + dip_drv = devm_kzalloc(&pdev->dev, sizeof(*dip_drv), GFP_KERNEL); > > > > Need to check for NULL. > > I got it. > > > + dev_set_drvdata(&pdev->dev, dip_drv); > > > + dip_dev = &dip_drv->dip_dev; > > > + > > > + if (!dip_dev) > > > + return -ENOMEM; > > > + > > > + dev_info(&pdev->dev, "Created dip_dev = 0x%p\n", dip_dev); > > > + > > > + dip_dev->pdev = pdev; > > > + dip_ctx = &dip_dev->dip_ctx; > > > + > > > + node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0); > > > + if (!node) { > > > + dev_err(&pdev->dev, "no mediatek,larb found"); > > > + return -EINVAL; > > > + } > > > + larb_pdev = of_find_device_by_node(node); > > > + if (!larb_pdev) { > > > + dev_err(&pdev->dev, "no mediatek,larb device found"); > > > + return -EINVAL; > > > + } > > > + dip_dev->larb_dev = &larb_pdev->dev; > > > + > > > + /*CCF: Grab clock pointer (struct clk*) */ > > > > Add a space before 'CCF'. > > I got it. > > > + dip_dev->dip_clk.DIP_IMG_LARB5 = devm_clk_get(&pdev->dev, > > > + "DIP_CG_IMG_LARB5"); > > > + dip_dev->dip_clk.DIP_IMG_DIP = devm_clk_get(&pdev->dev, > > > + "DIP_CG_IMG_DIP"); > > > + if (IS_ERR(dip_dev->dip_clk.DIP_IMG_LARB5)) { > > > + dev_err(&pdev->dev, "cannot get DIP_IMG_LARB5 clock\n"); > > > + return PTR_ERR(dip_dev->dip_clk.DIP_IMG_LARB5); > > > + } > > > + if (IS_ERR(dip_dev->dip_clk.DIP_IMG_DIP)) { > > > + dev_err(&pdev->dev, "cannot get DIP_IMG_DIP clock\n"); > > > + return PTR_ERR(dip_dev->dip_clk.DIP_IMG_DIP); > > > + } > > > + > > > + pm_runtime_enable(&pdev->dev); > > > + > > > + atomic_set(&dip_ctx->dip_user_cnt, 0); > > > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > > > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > > > + > > > + atomic_set(&dip_ctx->num_composing, 0); > > > + atomic_set(&dip_ctx->num_running, 0); > > > + > > > + dip_ctx->dip_worklist.queue_cnt = 0; > > > + > > > + ret = mtk_dip_ctx_dip_v4l2_init(pdev, > > > + &dip_drv->isp_preview_dev, > > > + &dip_drv->isp_capture_dev); > > > + > > > + if (ret) > > > + dev_err(&pdev->dev, "v4l2 init failed: %d\n", ret); > > > + > > > + dev_info(&pdev->dev, "X. DIP driver probe.\n"); > > > + > > > + return ret; > > > +} > > > + > > > +static int mtk_dip_remove(struct platform_device *pdev) > > > +{ > > > + struct mtk_isp_dip_drv_data *drv_data = > > > + dev_get_drvdata(&pdev->dev); > > > + > > > + /* */ > > > > What's with the empty comments? Here and below. > > I will remove them. > > > + if (drv_data) { > > > + mtk_dip_dev_core_release(pdev, &drv_data->isp_preview_dev); > > > + mtk_dip_dev_core_release(pdev, &drv_data->isp_capture_dev); > > > + dev_info(&pdev->dev, "E. %s\n", __func__); > > > > Remove this line. > > I will remove this line. > > > + } > > > + > > > + pm_runtime_disable(&pdev->dev); > > > + > > > + /* */ > > > + return 0; > > > +} > > > + > > > > ... > > > > > +static struct platform_driver mtk_dip_driver = { > > > + .probe = mtk_dip_probe, > > > + .remove = mtk_dip_remove, > > > + .driver = { > > > + .name = DIP_DEV_NAME, > > > + .owner = THIS_MODULE, > > > > You don't need the .owner line. module_platform_driver() / > > platform_driver_register() takes care of this for you. > > I got it. I will remove the .owner line > > Brian > > > > > + .of_match_table = dip_of_ids, > > > + .pm = &mtk_dip_pm_ops, > > > + } > > > +}; > > > + > > > +module_platform_driver(mtk_dip_driver); > > > + > > > +MODULE_DESCRIPTION("Camera DIP driver"); > > > +MODULE_LICENSE("GPL"); > > ... > > Sincerely, Frederic Chen _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver 2019-02-23 6:18 ` Frederic Chen @ 2019-02-28 3:24 ` Brian Norris 2019-03-06 17:07 ` Frederic Chen 0 siblings, 1 reply; 19+ messages in thread From: Brian Norris @ 2019-02-28 3:24 UTC (permalink / raw) To: Frederic Chen Cc: Sean Cheng (鄭昇弘), laurent.pinchart+renesas, Rynn Wu (吳育恩), srv_heupstream, Holmes Chiou (邱挺), Jerry-ch Chen (陳敬憲), tfiga, Jungo Lin (林明俊), Sj Huang (黃信璋), hans.verkuil, linux-mediatek, matthias.bgg, Christie Yu (游雅惠), mchehab, linux-arm-kernel, linux-media Hi Frederic, On Sat, Feb 23, 2019 at 02:18:54PM +0800, Frederic Chen wrote: > Dear Brian, > > I appreciate your comments. I'm really sorry for the delay in responding > to the comments due to some mail subscribing failed issue inside my company. No problem. > On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote: > > On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote: > > > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote: > > > > +static void dip_submit_worker(struct work_struct *work) > > > > +{ > > > > + struct mtk_dip_submit_work *dip_submit_work = > > > > + container_of(work, struct mtk_dip_submit_work, frame_work); > > > > + > > > > + struct mtk_dip_hw_ctx *dip_ctx = dip_submit_work->dip_ctx; > > > > + struct mtk_dip_work *dip_work; > > > > + struct dip_device *dip_dev; > > > > + struct dip_subframe *buf; > > > > + u32 len, num; > > > > + int ret; > > > > + > > > > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > > > > + num = atomic_read(&dip_ctx->num_composing); > > > > + > > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > > + dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, > > > > + struct mtk_dip_work, list_entry); > > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > > > I see you grab the head of the list here, but then you release the lock. > > > Then you later assume that reference is still valid, throughout this > > > function. > > > > > > That's usually true, because you only remove/delete entries from this > > > list within this same workqueue (at the end of this function). But it's > > > not true in dip_release_context() (which doesn't even grab the lock, > > > BTW). > > > > > > I think there could be several ways to solve this, but judging by how > > > this list entry is used...couldn't you just remove it from the list > > > here, while holding the lock? Then you only have to kfree() it when > > > you're done under the free_work_list label. > > > > > I see. I would like to modify the codes as following: > > mutex_lock(&dip_ctx->dip_useridlist.queuelock); You missed the part where you get the head of the list: dip_work = list_first_entry(...); But otherwise mostly looks OK. > dip_work->user_id->num--; Why do you need to do that with the queuelock held? Once you remove this work item from the list (safely under the lock), shouldn't you be the only one accessing it? (Note, I don't actually know what that 'num' really means. I'm just looking at basic driver mechanics.) > list_del(&dip_work->list_entry); > dip_ctx->dip_worklist.queue_cnt--; > len = dip_ctx->dip_worklist.queue_cnt; > mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > goto free_work_list; > > /* ...... */ > > free_work_list: > kfree(dip_work); > > > > > + > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > + if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > + > > > > + dip_work->frameparams.state = FRAME_STATE_STREAMOFF; > > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > > + > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > + dip_work->user_id->num--; > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n", > > > > + dip_work->user_id->id, dip_work->user_id->num, > > > > + dip_work->frameparams.frame_no, > > > > + dip_work->frameparams.index); > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > + > > > > + goto free_work_list; > > > > + } > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > + > > > > + while (num >= DIP_COMPOSING_MAX_NUM) { > > > > + ret = wait_event_interruptible_timeout > > > > + (dip_ctx->composing_wq, > > > > + (num < DIP_COMPOSING_MAX_NUM), > > > > + msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT)); > > > > + > > > > + if (ret == -ERESTARTSYS) > > > > + dev_err(&dip_dev->pdev->dev, > > > > + "interrupted by a signal!\n"); > > > > + else if (ret == 0) > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "timeout frame_no(%d), num: %d\n", > > > > + dip_work->frameparams.frame_no, num); > > > > + else > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "wakeup frame_no(%d), num: %d\n", > > > > + dip_work->frameparams.frame_no, num); > > > > + > > > > + num = atomic_read(&dip_ctx->num_composing); > > > > + }; > > > > + > > > > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > > > > + if (list_empty(&dip_ctx->dip_freebufferlist.queue)) { > > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > > + > > > > + dev_err(&dip_dev->pdev->dev, > > > > + "frame_no(%d) index: %x no free buffer: %d\n", > > > > + dip_work->frameparams.frame_no, > > > > + dip_work->frameparams.index, > > > > + dip_ctx->dip_freebufferlist.queue_cnt); > > > > + > > > > + /* Call callback to notify V4L2 common framework > > > > + * for failure of enqueue > > > > + */ > > > > + dip_work->frameparams.state = FRAME_STATE_ERROR; > > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > > + > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > + dip_work->user_id->num--; > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > + > > > > + goto free_work_list; > > > > + } > > > > + > > > > + buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue, > > > > + struct dip_subframe, > > > > + list_entry); > > > > + list_del(&buf->list_entry); > > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > > + > > > > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > > > > + list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue); > > > > + dip_ctx->dip_usedbufferlist.queue_cnt++; > > > > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > > > > + > > > > + memcpy(&dip_work->frameparams.subfrm_data, > > > > + &buf->buffer, sizeof(buf->buffer)); > > > > + > > > > + memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ); > > > > + > > > > + memcpy(&dip_work->frameparams.config_data, > > > > + &buf->config_data, sizeof(buf->config_data)); > > > > + > > > > + memset((char *)buf->config_data.va, 0, DIP_COMP_SZ); > > > > + > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "frame_no(%d) has no tuning_data\n", > > > > + dip_work->frameparams.frame_no); > > > > + > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > + > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > + /* When user enqueued without tuning buffer, > > > > + * it would use driver internal buffer. > > > > + * So, tuning_data.va should be 0 > > > > + */ > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > + } > > > > + > > > > + dip_work->frameparams.drv_data = (u64)dip_ctx; > > > > + dip_work->frameparams.state = FRAME_STATE_COMPOSING; > > > > + > > > > + memcpy((void *)buf->frameparam.va, &dip_work->frameparams, > > > > + sizeof(dip_work->frameparams)); > > > > + > > > > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > > > > + (void *)&dip_work->frameparams, > > > > + sizeof(dip_work->frameparams), 0); > > > > + num = atomic_inc_return(&dip_ctx->num_composing); > > > > + > > > > +free_work_list: > > > > + > > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > > + list_del(&dip_work->list_entry); > > > > + dip_ctx->dip_worklist.queue_cnt--; > > > > + len = dip_ctx->dip_worklist.queue_cnt; > > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "frame_no(%d) index: %x, worklist count: %d, composing num: %d\n", > > > > + dip_work->frameparams.frame_no, dip_work->frameparams.index, > > > > + len, num); > > > > + > > > > + kfree(dip_work); > > > > +} > > > > +int dip_release_context(struct dip_device *dip_dev) > > > > > > Should be static. > > > > > I will change it to static. > > > > > +{ > > > > + u32 i = 0; > > > > + struct dip_subframe *buf, *tmpbuf; > > > > + struct mtk_dip_work *dip_work, *tmp_work; > > > > + struct dip_user_id *dip_userid, *tmp_id; > > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > > + > > > > + dip_ctx = &dip_dev->dip_ctx; > > > > + dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n", > > > > + dip_ctx->dip_worklist.queue_cnt); > > > > + > > > > + list_for_each_entry_safe(dip_work, tmp_work, > > > > + &dip_ctx->dip_worklist.queue, > > > > + list_entry) { > > > > > > Shouldn't you be holding the mutex for this? Or alternatively, cancel > > > any outstanding work and move the flush_workqueue()/destroy_workqueue() > > > up. > > > > > > Similar questions for the other lists we're going through here. > > > > > We missed the mutex holding here. I would like to change the codes as following: > > mutex_lock(&dip_ctx->dip_worklist.queuelock); > list_for_each_entry_safe(dip_work, tmp_work, > &dip_ctx->dip_worklist.queue, > list_entry) { > list_del(&dip_work->list_entry); > dip_ctx->dip_worklist.queue_cnt--; > kfree(dip_work); > } > mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > I will also modify dip_useridlist and dip_ctx->dip_freebufferlist > parts like dip_ctx->dip_worklist. Seems about right. Brian > > > > + list_del(&dip_work->list_entry); > > > > + dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n", > > > > + dip_work->frameparams.frame_no); > > > > + kfree(dip_work); > > > > + dip_ctx->dip_worklist.queue_cnt--; > > > > + } > > > > + > > > > + if (dip_ctx->dip_worklist.queue_cnt != 0) > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "dip_worklist is not empty (%d)\n", > > > > + dip_ctx->dip_worklist.queue_cnt); > > > > + > > > > + list_for_each_entry_safe(dip_userid, tmp_id, > > > > + &dip_ctx->dip_useridlist.queue, > > > > + list_entry) { > > > > + list_del(&dip_userid->list_entry); > > > > + dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n", > > > > + dip_userid->id); > > > > + kfree(dip_userid); > > > > + dip_ctx->dip_useridlist.queue_cnt--; > > > > + } > > > > + > > > > + if (dip_ctx->dip_useridlist.queue_cnt != 0) > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "dip_useridlist is not empty (%d)\n", > > > > + dip_ctx->dip_useridlist.queue_cnt); > > > > + > > > > + flush_workqueue(dip_ctx->mdpcb_workqueue); > > > > + destroy_workqueue(dip_ctx->mdpcb_workqueue); > > > > + dip_ctx->mdpcb_workqueue = NULL; > > > > + > > > > + flush_workqueue(dip_ctx->composer_wq); > > > > + destroy_workqueue(dip_ctx->composer_wq); > > > > + dip_ctx->composer_wq = NULL; > > > > + > > > > + atomic_set(&dip_ctx->num_composing, 0); > > > > + atomic_set(&dip_ctx->num_running, 0); > > > > + > > > > + kthread_stop(dip_ctx->dip_runner_thread.thread); > > > > + dip_ctx->dip_runner_thread.thread = NULL; > > > > + > > > > + atomic_set(&dip_ctx->dip_user_cnt, 0); > > > > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > > > > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > > > > + > > > > + /* All the buffer should be in the freebufferlist when release */ > > > > + list_for_each_entry_safe(buf, tmpbuf, > > > > + &dip_ctx->dip_freebufferlist.queue, > > > > + list_entry) { > > > > + struct sg_table *sgt = &buf->table; > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "buf pa (%d): %x\n", i, buf->buffer.pa); > > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > > + dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl, > > > > + sgt->orig_nents, > > > > + DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC); > > > > + sg_free_table(sgt); > > > > + list_del(&buf->list_entry); > > > > + kfree(buf); > > > > + buf = NULL; > > > > + i++; > > > > + } > > > > + > > > > + if (dip_ctx->dip_freebufferlist.queue_cnt != 0 && > > > > + i != DIP_SUB_FRM_DATA_NUM) > > > > + dev_err(&dip_dev->pdev->dev, > > > > + "dip_freebufferlist is not empty (%d/%d)\n", > > > > + dip_ctx->dip_freebufferlist.queue_cnt, i); > > > > + > > > > + mutex_destroy(&dip_ctx->dip_useridlist.queuelock); > > > > + mutex_destroy(&dip_ctx->dip_worklist.queuelock); > > > > + mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock); > > > > + mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock); > > > > + > > > > + return 0; > > > > +} > > > > + _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver 2019-02-28 3:24 ` Brian Norris @ 2019-03-06 17:07 ` Frederic Chen 0 siblings, 0 replies; 19+ messages in thread From: Frederic Chen @ 2019-03-06 17:07 UTC (permalink / raw) To: Brian Norris Cc: Sean Cheng (鄭昇弘), laurent.pinchart+renesas, Rynn Wu (吳育恩), srv_heupstream, Holmes Chiou (邱挺), Jerry-ch Chen (陳敬憲), tfiga, Jungo Lin (林明俊), Sj Huang (黃信璋), hans.verkuil, linux-mediatek, matthias.bgg, Christie Yu (游雅惠), mchehab, linux-arm-kernel, linux-media Dear Brian, I appreciate your comments. On Wed, 2019-02-27 at 19:24 -0800, Brian Norris wrote: > Hi Frederic, > > On Sat, Feb 23, 2019 at 02:18:54PM +0800, Frederic Chen wrote: > > Dear Brian, > > > > I appreciate your comments. I'm really sorry for the delay in responding > > to the comments due to some mail subscribing failed issue inside my company. > > No problem. > > > On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote: > > > On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote: > > > > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote: > > > > > > +static void dip_submit_worker(struct work_struct *work) > > > > > +{ > > > > > + struct mtk_dip_submit_work *dip_submit_work = > > > > > + container_of(work, struct mtk_dip_submit_work, frame_work); > > > > > + > > > > > + struct mtk_dip_hw_ctx *dip_ctx = dip_submit_work->dip_ctx; > > > > > + struct mtk_dip_work *dip_work; > > > > > + struct dip_device *dip_dev; > > > > > + struct dip_subframe *buf; > > > > > + u32 len, num; > > > > > + int ret; > > > > > + > > > > > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > > > > > + num = atomic_read(&dip_ctx->num_composing); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > > > + dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, > > > > > + struct mtk_dip_work, list_entry); > > > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > > > > > I see you grab the head of the list here, but then you release the lock. > > > > Then you later assume that reference is still valid, throughout this > > > > function. > > > > > > > > That's usually true, because you only remove/delete entries from this > > > > list within this same workqueue (at the end of this function). But it's > > > > not true in dip_release_context() (which doesn't even grab the lock, > > > > BTW). > > > > > > > > I think there could be several ways to solve this, but judging by how > > > > this list entry is used...couldn't you just remove it from the list > > > > here, while holding the lock? Then you only have to kfree() it when > > > > you're done under the free_work_list label. > > > > > > > > I see. I would like to modify the codes as following: > > > > mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > You missed the part where you get the head of the list: > > dip_work = list_first_entry(...); > > But otherwise mostly looks OK. > > > dip_work->user_id->num--; > > Why do you need to do that with the queuelock held? Once you remove this > work item from the list (safely under the lock), shouldn't you be the > only one accessing it? > > (Note, I don't actually know what that 'num' really means. I'm just > looking at basic driver mechanics.) > Yes, there is only one user of the dip work at that time. I made a mistake on the usage of dip_useridlist.queuelock and dip_worklist.queuelock here. What I would like to do is to decrease the total number of the frames of the user, which is protected by dip_useridlist.queuelock. (user_id->num saves the total number of the dip frames belongs to a user; the user may be the preview or capture context.) On the other hand, the list of dip work is protected by another lock, dip_worklist.queuelock. In regarding to that point, I would like change the codes as following: mutex_lock(&dip_ctx->dip_worklist.queuelock); dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, struct mtk_dip_work, list_entry); list_del(&dip_work->list_entry); dip_ctx->dip_worklist.queue_cnt--; len = dip_ctx->dip_worklist.queue_cnt; mutex_unlock(&dip_ctx->dip_worklist.queuelock); /* If the frame's user (preview or capture device) */ /* is in stream off state, */ /* return and release the buffers of the frame */ mutex_lock(&dip_ctx->dip_useridlist.queuelock); if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { dip_work->user_id->num--; mutex_unlock(&dip_ctx->dip_useridlist.queuelock); dip_work->frameparams.state = FRAME_STATE_STREAMOFF; call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); goto free_work_list; mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > list_del(&dip_work->list_entry); > > dip_ctx->dip_worklist.queue_cnt--; > > len = dip_ctx->dip_worklist.queue_cnt; > > mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > goto free_work_list; > > > > /* ...... */ > > > > free_work_list: > > kfree(dip_work); > > > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > > + if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + dip_work->frameparams.state = FRAME_STATE_STREAMOFF; > > > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > > + dip_work->user_id->num--; > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n", > > > > > + dip_work->user_id->id, dip_work->user_id->num, > > > > > + dip_work->frameparams.frame_no, > > > > > + dip_work->frameparams.index); > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + goto free_work_list; > > > > > + } > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + while (num >= DIP_COMPOSING_MAX_NUM) { > > > > > + ret = wait_event_interruptible_timeout > > > > > + (dip_ctx->composing_wq, > > > > > + (num < DIP_COMPOSING_MAX_NUM), > > > > > + msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT)); > > > > > + > > > > > + if (ret == -ERESTARTSYS) > > > > > + dev_err(&dip_dev->pdev->dev, > > > > > + "interrupted by a signal!\n"); > > > > > + else if (ret == 0) > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "timeout frame_no(%d), num: %d\n", > > > > > + dip_work->frameparams.frame_no, num); > > > > > + else > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "wakeup frame_no(%d), num: %d\n", > > > > > + dip_work->frameparams.frame_no, num); > > > > > + > > > > > + num = atomic_read(&dip_ctx->num_composing); > > > > > + }; > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + if (list_empty(&dip_ctx->dip_freebufferlist.queue)) { > > > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + > > > > > + dev_err(&dip_dev->pdev->dev, > > > > > + "frame_no(%d) index: %x no free buffer: %d\n", > > > > > + dip_work->frameparams.frame_no, > > > > > + dip_work->frameparams.index, > > > > > + dip_ctx->dip_freebufferlist.queue_cnt); > > > > > + > > > > > + /* Call callback to notify V4L2 common framework > > > > > + * for failure of enqueue > > > > > + */ > > > > > + dip_work->frameparams.state = FRAME_STATE_ERROR; > > > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > > + dip_work->user_id->num--; > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + goto free_work_list; > > > > > + } > > > > > + > > > > > + buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue, > > > > > + struct dip_subframe, > > > > > + list_entry); > > > > > + list_del(&buf->list_entry); > > > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > > > > > + list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue); > > > > > + dip_ctx->dip_usedbufferlist.queue_cnt++; > > > > > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > > > > > + > > > > > + memcpy(&dip_work->frameparams.subfrm_data, > > > > > + &buf->buffer, sizeof(buf->buffer)); > > > > > + > > > > > + memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ); > > > > > + > > > > > + memcpy(&dip_work->frameparams.config_data, > > > > > + &buf->config_data, sizeof(buf->config_data)); > > > > > + > > > > > + memset((char *)buf->config_data.va, 0, DIP_COMP_SZ); > > > > > + > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "frame_no(%d) has no tuning_data\n", > > > > > + dip_work->frameparams.frame_no); > > > > > + > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > > + > > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > > + /* When user enqueued without tuning buffer, > > > > > + * it would use driver internal buffer. > > > > > + * So, tuning_data.va should be 0 > > > > > + */ > > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > > + } > > > > > + > > > > > + dip_work->frameparams.drv_data = (u64)dip_ctx; > > > > > + dip_work->frameparams.state = FRAME_STATE_COMPOSING; > > > > > + > > > > > + memcpy((void *)buf->frameparam.va, &dip_work->frameparams, > > > > > + sizeof(dip_work->frameparams)); > > > > > + > > > > > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > > > > > + (void *)&dip_work->frameparams, > > > > > + sizeof(dip_work->frameparams), 0); > > > > > + num = atomic_inc_return(&dip_ctx->num_composing); > > > > > + > > > > > +free_work_list: > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > > > + list_del(&dip_work->list_entry); > > > > > + dip_ctx->dip_worklist.queue_cnt--; > > > > > + len = dip_ctx->dip_worklist.queue_cnt; > > > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > > + > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "frame_no(%d) index: %x, worklist count: %d, composing num: %d\n", > > > > > + dip_work->frameparams.frame_no, dip_work->frameparams.index, > > > > > + len, num); > > > > > + > > > > > + kfree(dip_work); > > > > > +} > > > > > > > +int dip_release_context(struct dip_device *dip_dev) > > > > > > > > Should be static. > > > > > > > > I will change it to static. > > > > > > > +{ > > > > > + u32 i = 0; > > > > > + struct dip_subframe *buf, *tmpbuf; > > > > > + struct mtk_dip_work *dip_work, *tmp_work; > > > > > + struct dip_user_id *dip_userid, *tmp_id; > > > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > > > + > > > > > + dip_ctx = &dip_dev->dip_ctx; > > > > > + dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n", > > > > > + dip_ctx->dip_worklist.queue_cnt); > > > > > + > > > > > + list_for_each_entry_safe(dip_work, tmp_work, > > > > > + &dip_ctx->dip_worklist.queue, > > > > > + list_entry) { > > > > > > > > Shouldn't you be holding the mutex for this? Or alternatively, cancel > > > > any outstanding work and move the flush_workqueue()/destroy_workqueue() > > > > up. > > > > > > > > Similar questions for the other lists we're going through here. > > > > > > > > We missed the mutex holding here. I would like to change the codes as following: > > > > mutex_lock(&dip_ctx->dip_worklist.queuelock); > > list_for_each_entry_safe(dip_work, tmp_work, > > &dip_ctx->dip_worklist.queue, > > list_entry) { > > list_del(&dip_work->list_entry); > > dip_ctx->dip_worklist.queue_cnt--; > > kfree(dip_work); > > } > > mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > I will also modify dip_useridlist and dip_ctx->dip_freebufferlist > > parts like dip_ctx->dip_worklist. > > Seems about right. > > Brian > > > > > > + list_del(&dip_work->list_entry); > > > > > + dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n", > > > > > + dip_work->frameparams.frame_no); > > > > > + kfree(dip_work); > > > > > + dip_ctx->dip_worklist.queue_cnt--; > > > > > + } > > > > > + > > > > > + if (dip_ctx->dip_worklist.queue_cnt != 0) > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "dip_worklist is not empty (%d)\n", > > > > > + dip_ctx->dip_worklist.queue_cnt); > > > > > + > > > > > + list_for_each_entry_safe(dip_userid, tmp_id, > > > > > + &dip_ctx->dip_useridlist.queue, > > > > > + list_entry) { > > > > > + list_del(&dip_userid->list_entry); > > > > > + dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n", > > > > > + dip_userid->id); > > > > > + kfree(dip_userid); > > > > > + dip_ctx->dip_useridlist.queue_cnt--; > > > > > + } > > > > > + > > > > > + if (dip_ctx->dip_useridlist.queue_cnt != 0) > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "dip_useridlist is not empty (%d)\n", > > > > > + dip_ctx->dip_useridlist.queue_cnt); > > > > > + > > > > > + flush_workqueue(dip_ctx->mdpcb_workqueue); > > > > > + destroy_workqueue(dip_ctx->mdpcb_workqueue); > > > > > + dip_ctx->mdpcb_workqueue = NULL; > > > > > + > > > > > + flush_workqueue(dip_ctx->composer_wq); > > > > > + destroy_workqueue(dip_ctx->composer_wq); > > > > > + dip_ctx->composer_wq = NULL; > > > > > + > > > > > + atomic_set(&dip_ctx->num_composing, 0); > > > > > + atomic_set(&dip_ctx->num_running, 0); > > > > > + > > > > > + kthread_stop(dip_ctx->dip_runner_thread.thread); > > > > > + dip_ctx->dip_runner_thread.thread = NULL; > > > > > + > > > > > + atomic_set(&dip_ctx->dip_user_cnt, 0); > > > > > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > > > > > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > > > > > + > > > > > + /* All the buffer should be in the freebufferlist when release */ > > > > > + list_for_each_entry_safe(buf, tmpbuf, > > > > > + &dip_ctx->dip_freebufferlist.queue, > > > > > + list_entry) { > > > > > + struct sg_table *sgt = &buf->table; > > > > > + > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "buf pa (%d): %x\n", i, buf->buffer.pa); > > > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > > > + dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl, > > > > > + sgt->orig_nents, > > > > > + DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC); > > > > > + sg_free_table(sgt); > > > > > + list_del(&buf->list_entry); > > > > > + kfree(buf); > > > > > + buf = NULL; > > > > > + i++; > > > > > + } > > > > > + > > > > > + if (dip_ctx->dip_freebufferlist.queue_cnt != 0 && > > > > > + i != DIP_SUB_FRM_DATA_NUM) > > > > > + dev_err(&dip_dev->pdev->dev, > > > > > + "dip_freebufferlist is not empty (%d/%d)\n", > > > > > + dip_ctx->dip_freebufferlist.queue_cnt, i); > > > > > + > > > > > + mutex_destroy(&dip_ctx->dip_useridlist.queuelock); > > > > > + mutex_destroy(&dip_ctx->dip_worklist.queuelock); > > > > > + mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock); > > > > > + mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + Sincerely, Frederic Chen _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen ` (6 preceding siblings ...) [not found] ` <1549020091-42064-8-git-send-email-frederic.chen@mediatek.com> @ 2019-03-14 8:46 ` Hans Verkuil 7 siblings, 0 replies; 19+ messages in thread From: Hans Verkuil @ 2019-03-14 8:46 UTC (permalink / raw) To: Frederic Chen, hans.verkuil, laurent.pinchart+renesas, tfiga, matthias.bgg, mchehab Cc: Sean.Cheng, Rynn.Wu, srv_heupstream, holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, christie.yu, linux-mediatek, linux-arm-kernel, linux-media Hi Frederic Chen, On 2/1/19 12:21 PM, Frederic Chen wrote: > Hello, > > This is the first version of the RFC patch series adding Digital Image > Processing (DIP) driver on Mediatek mt8183 SoC, which will be used in camera > features on CrOS application. It belongs to the first Mediatek’s ISP driver > series based on V4L2 and media controller framework. I posted the main part of > the DIP driver as RFC to discuss first and would like some review comments on > the overall structure of the driver. > > Digital Image Processing (DIP) unit can accept the tuning parameters and adjust > the image content in Mediatek ISP system. Furthermore, it performs demosaicing > and noise reduction on the image to support the advanced camera features of the > application. The DIP driver also support image format conversion, resizing and > rotation with its hardware path. > > The driver is implemented with V4L2 and media controller framework. We have the > following entities describing the DIP path. > > 1. Meta (output video device): connects to DIP sub device. It accepts the input > tuning buffer from userspace. The metadata interface used currently is only > a temporary solution to kick off driver development and is not ready for > reviewed yet. > > 2. RAW (output video device): connects to DIP sub device. It accepts input image > buffer from userspace. > > 3. DIP (sub device): connects to MDP-0 and MDP-1. When processing an image, DIP > hardware support multiple output image with different size and format so it > needs two capture video devices to return the streaming data to the user. > > 4. MDP-0 (capture video device): return the processed image data. > > 5. MDP-1 (capture video device): return the processed image data, the image > size and format can be different from the ones of MDP-0. Just a high-level comment before you post the next version of this series and for your "platform: Add support for ISP Pass 1 on mt8183 SoC" series Please compile the latest version of v4l2-compliance (part of git://linuxtv.org/v4l-utils.git) and run it against your driver: v4l2-compliance -m /dev/mediaX Whenever you post a new version of this series, please do a 'git pull' of the v4l-utils repo, recompile and retest with v4l2-compliance and post the test results in the cover letter. Obviously, there should be no FAILs and probably no warnings. I suspect that streaming (e.g. adding the -s10 option to v4l2-compliance) might not work since v4l2-compliance doesn't know about the meta data formats. But give it a try and see what happens :-) Regards, Hans > > The overall file structure of the DIP driver is as following: > > * mtk_dip-dev-ctx-core.c: Implements common software flow of DIP driver. > DIP driver supports two or more software contexts. For example, context 0 is > created for preview path and context 1 is for capture path. Both the two > contexts share the same DIP hardware to process the images. > * mtk_dip-v4l2.c: Static DIP contexts configuration. > * mtk_dip.c: Controls the hardware flow. > * mtk_dip-dev.c: Implements context-independent flow. > * mtk_dip-ctrl.c: Handles the HW ctrl request from userspace. > * mtk_dip-smem-drv.c: Provides the shared memory management required operation. > We reserved a memory region for the co-processor and DIP to exchange the > tuning and hardware configuration data. > * mtk_dip-v4l2-util.c: Implements V4L2 and vb2 ops. > > Frederic Chen (7): > [media] dt-bindings: mt8183: Add binding for DIP shared memory > dts: arm64: mt8183: Add DIP shared memory node > [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings > [media] dt-bindings: mt8183: Added DIP dt-bindings > dts: arm64: mt8183: Add DIP nodes > media: platform: Add Mediatek DIP driver KConfig > [media] platform: mtk-isp: Add Mediatek DIP driver > > .../bindings/media/mediatek,dip_smem.txt | 29 + > .../bindings/media/mediatek,mt8183-dip.txt | 35 + > .../mediatek,reserve-memory-dip_smem.txt | 45 + > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 36 + > drivers/media/platform/Kconfig | 2 + > drivers/media/platform/mtk-isp/Kconfig | 21 + > drivers/media/platform/mtk-isp/Makefile | 18 + > drivers/media/platform/mtk-isp/isp_50/Makefile | 17 + > drivers/media/platform/mtk-isp/isp_50/dip/Makefile | 35 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h | 188 +++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c | 173 +++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h | 43 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h | 319 ++++ > .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c | 1643 ++++++++++++++++++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c | 374 +++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h | 191 +++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c | 452 ++++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h | 25 + > .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c | 1000 ++++++++++++ > .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h | 38 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c | 292 ++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h | 60 + > .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c | 1385 +++++++++++++++++ > .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h | 93 ++ > 24 files changed, 6514 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt > create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt > create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt > create mode 100644 drivers/media/platform/mtk-isp/Kconfig > create mode 100644 drivers/media/platform/mtk-isp/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-core.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-03-14 8:46 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-01 11:21 [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory Frederic Chen 2019-02-09 15:59 ` Sakari Ailus 2019-02-09 18:17 ` Laurent Pinchart 2019-02-12 9:37 ` Frederic Chen 2019-02-13 3:41 ` Tomasz Figa 2019-02-01 11:21 ` [RFC PATCH V0 2/7] dts: arm64: mt8183: Add DIP shared memory node Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings Frederic Chen 2019-02-09 15:59 ` Sakari Ailus 2019-02-09 18:20 ` Laurent Pinchart 2019-02-12 9:50 ` Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 4/7] [media] dt-bindings: mt8183: Added DIP dt-bindings Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 5/7] dts: arm64: mt8183: Add DIP nodes Frederic Chen 2019-02-01 11:21 ` [RFC PATCH V0 6/7] media: platform: Add Mediatek DIP driver KConfig Frederic Chen [not found] ` <1549020091-42064-8-git-send-email-frederic.chen@mediatek.com> 2019-02-07 19:08 ` [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver Brian Norris [not found] ` <1550756198.11724.86.camel@mtksdccf07> 2019-02-23 6:18 ` Frederic Chen 2019-02-28 3:24 ` Brian Norris 2019-03-06 17:07 ` Frederic Chen 2019-03-14 8:46 ` [RFC PATCH V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).