Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

13 participants
3142 discussions

Re: [PATCH 2/3] dma-buf/heaps: replace kmap_atomic with kmap_local_page

by T.J. Mercier

On Tue, Oct 1, 2024 at 7:51 PM Pintu Kumar <quic_pintu(a)quicinc.com> wrote: > > Use of kmap_atomic/kunmap_atomic is deprecated, use > kmap_local_page/kunmap_local instead. > > This is reported by checkpatch. > Also fix repeated word issue. > > WARNING: Deprecated use of 'kmap_atomic', prefer 'kmap_local_page' instead > + void *vaddr = kmap_atomic(page); > > WARNING: Deprecated use of 'kunmap_atomic', prefer 'kunmap_local' instead > + kunmap_atomic(vaddr); > > WARNING: Possible repeated word: 'by' > + * has been killed by by SIGKILL > > total: 0 errors, 3 warnings, 405 lines checked > > Signed-off-by: Pintu Kumar <quic_pintu(a)quicinc.com> Reviewed-by: T.J. Mercier <tjmercier(a)google.com> The Android kernels have been doing this for over a year, so should be pretty well tested at this point: https://r.android.com/c/kernel/common/+/2500840 > --- > drivers/dma-buf/heaps/cma_heap.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index 93be88b805fe..8c55431cc16c 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -309,13 +309,13 @@ static struct dma_buf *cma_heap_allocate(struct dma_heap *heap, > struct page *page = cma_pages; > > while (nr_clear_pages > 0) { > - void *vaddr = kmap_atomic(page); > + void *vaddr = kmap_local_page(page); > > memset(vaddr, 0, PAGE_SIZE); > - kunmap_atomic(vaddr); > + kunmap_local(vaddr); > /* > * Avoid wasting time zeroing memory if the process > - * has been killed by by SIGKILL > + * has been killed by SIGKILL. > */ > if (fatal_signal_pending(current)) > goto free_cma; > -- > 2.17.1 >

1 year, 1 month

Re: [PATCH v1 01/10] dt-bindings: media: mediatek: add camsys device

by Rob Herring

On Wed, Oct 09, 2024 at 07:15:42PM +0800, Shu-hsiang Yang wrote: > 1. Add camera isp7x module device document > 2. Add camera interface device document > > Signed-off-by: Shu-hsiang Yang <Shu-hsiang.Yang(a)mediatek.com> > --- > .../media/mediatek/mediatek,cam-raw.yaml | 169 ++++++++++++++++++ > .../media/mediatek/mediatek,cam-yuv.yaml | 148 +++++++++++++++ > .../media/mediatek/mediatek,camisp.yaml | 71 ++++++++ > .../media/mediatek/mediatek,seninf-core.yaml | 106 +++++++++++ > .../media/mediatek/mediatek,seninf.yaml | 88 +++++++++ > 5 files changed, 582 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek/mediatek,cam-raw.yaml > create mode 100644 Documentation/devicetree/bindings/media/mediatek/mediatek,cam-yuv.yaml > create mode 100644 Documentation/devicetree/bindings/media/mediatek/mediatek,camisp.yaml > create mode 100644 Documentation/devicetree/bindings/media/mediatek/mediatek,seninf-core.yaml > create mode 100644 Documentation/devicetree/bindings/media/mediatek/mediatek,seninf.yaml > > diff --git a/Documentation/devicetree/bindings/media/mediatek/mediatek,cam-raw.yaml b/Documentation/devicetree/bindings/media/mediatek/mediatek,cam-raw.yaml > new file mode 100644 > index 000000000000..c709e4bf0a18 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek/mediatek,cam-raw.yaml > @@ -0,0 +1,169 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (c) 2024 MediaTek Inc. > + > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/mediatek/mediatek,cam-raw.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: The cam-raw unit of MediaTek ISP system > + > +maintainers: > + - Shu-hsiang Yang <shu-hsiang.yang(a)mediatek.com> > + - Shun-yi Wang <shun-yi.wang(a)mediatek.com> > + - Teddy Chen <teddy.chen(a)mediatek.com> > + > +description: > + MediaTek cam-raw is the camera RAW processing unit in MediaTek SoC. > + > +properties: > + compatible: > + const: mediatek,cam-raw Compatibles should be SoC specific. > + > + "#address-cells": > + const: 2 > + > + "#size-cells": > + const: 2 Don't need these. You don't have any child nodes with addresses. > + > + reg: > + items: > + minItems: 2 > + maxItems: 4 Drop items. > + minItems: 1 > + maxItems: 2 You have to specify what each region is. Why does it vary? A h/w block either has register region or it doesn't. > + > + reg-names: > + minItems: 1 > + maxItems: 2 You must define the names. > + > + mediatek,cam-id: > + description: > + Describes the index of MediaTek cam-raw unit for ISP > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2] No, we don't put module indices in DT. > + > + mediatek,larbs: > + description: > + Describes MediaTek bus infrastructure unit for ISP system. > + List of phandle to the local arbiters in the current SoCs. > + Refer to bindings/memory-controllers/mediatek,smi-larb.yaml. > + $ref: /schemas/types.yaml#/definitions/phandle-array > + minItems: 1 > + maxItems: 32 > + > + interrupts: > + minItems: 1 > + > + dma-ranges: > + description: > + Describes the address information of IOMMU mapping to memory. > + Defines six fields for the MediaTek IOMMU extended iova, pa, and size. > + minItems: 1 > + > + power-domains: > + minItems: 1 > + > + clocks: > + minItems: 4 > + maxItems: 16 You have to define what the clocks are. > + > + clock-names: > + minItems: 4 > + maxItems: 16 > + > + assigned-clocks: > + maxItems: 1 > + > + assigned-clock-parents: > + maxItems: 1 Drop. You don't need to document assigned-clocks. > + > + iommus: > + description: > + Points to the respective IOMMU block with master port as argument, see > + Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml for details. > + Ports are according to the HW. > + minItems: 1 > + maxItems: 32 > + > +required: > + - compatible > + - reg > + - reg-names > + - interrupts > + - power-domains > + - clocks > + - clock-names > + - iommus > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/power/mediatek,mt8188-power.h> > + #include <dt-bindings/clock/mediatek,mt8188-clk.h> > + #include <dt-bindings/memory/mediatek,mt8188-memory-port.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + cam_raw_a@16030000 { > + compatible = "mediatek,cam-raw"; > + reg = <0 0x16030000 0 0x8000>, > + <0 0x16038000 0 0x8000>; > + reg-names = "base", "inner_base"; > + mediatek,cam-id = <0>; > + mediatek,larbs = <&larb16a>; > + interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH 0>; > + #address-cells = <2>; > + #size-cells = <2>; > + dma-ranges = <0x2 0x0 0x0 0x40000000 0x1 0x0>; > + power-domains = <&spm MT8188_POWER_DOMAIN_CAM_SUBA>; > + clocks = <&camsys CLK_CAM_MAIN_CAM2MM0_GALS>, > + <&camsys CLK_CAM_MAIN_CAM2MM1_GALS>, > + <&camsys CLK_CAM_MAIN_CAM2SYS_GALS>, > + <&camsys CLK_CAM_MAIN_CAM>, > + <&camsys CLK_CAM_MAIN_CAMTG>, > + <&camsys_rawa CLK_CAM_RAWA_LARBX>, > + <&camsys_rawa CLK_CAM_RAWA_CAM>, > + <&camsys_rawa CLK_CAM_RAWA_CAMTG>, > + <&topckgen CLK_TOP_CAM>, > + <&topckgen CLK_TOP_CAMTG>, > + <&topckgen CLK_TOP_CAMTM>; > + clock-names = "camsys_cam2mm0_cgpdn", > + "camsys_cam2mm1_cgpdn", > + "camsys_cam2sys_cgpdn", > + "camsys_cam_cgpdn", > + "camsys_camtg_cgpdn", > + "camsys_rawa_larbx_cgpdn", > + "camsys_rawa_cam_cgpdn", > + "camsys_rawa_camtg_cgpdn", > + "topckgen_top_cam", > + "topckgen_top_camtg", > + "topckgen_top_camtm"; > + assigned-clocks = <&topckgen CLK_TOP_CAM>; > + assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL_D5>; > + iommus = <&vpp_iommu M4U_PORT_L16A_IMGO_R1>, > + <&vpp_iommu M4U_PORT_L16A_CQI_R1>, > + <&vpp_iommu M4U_PORT_L16A_CQI_R2>, > + <&vpp_iommu M4U_PORT_L16A_BPCI_R1>, > + <&vpp_iommu M4U_PORT_L16A_LSCI_R1>, > + <&vpp_iommu M4U_PORT_L16A_RAWI_R2>, > + <&vpp_iommu M4U_PORT_L16A_RAWI_R3>, > + <&vpp_iommu M4U_PORT_L16A_UFDI_R2>, > + <&vpp_iommu M4U_PORT_L16A_UFDI_R3>, > + <&vpp_iommu M4U_PORT_L16A_RAWI_R4>, > + <&vpp_iommu M4U_PORT_L16A_RAWI_R5>, > + <&vpp_iommu M4U_PORT_L16A_AAI_R1>, > + <&vpp_iommu M4U_PORT_L16A_UFDI_R5>, > + <&vpp_iommu M4U_PORT_L16A_FHO_R1>, > + <&vpp_iommu M4U_PORT_L16A_AAO_R1>, > + <&vpp_iommu M4U_PORT_L16A_TSFSO_R1>, > + <&vpp_iommu M4U_PORT_L16A_FLKO_R1>; > + }; > + }; > + > +... > diff --git a/Documentation/devicetree/bindings/media/mediatek/mediatek,cam-yuv.yaml b/Documentation/devicetree/bindings/media/mediatek/mediatek,cam-yuv.yaml > new file mode 100644 > index 000000000000..30dfd5e5ecb1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek/mediatek,cam-yuv.yaml Similar comments on the rest. Rob

1 year, 1 month

Re: [PATCH v1 02/10] media: platform: mediatek: add seninf controller

by AngeloGioacchino Del Regno

Il 09/10/24 13:15, Shu-hsiang Yang ha scritto: > Introduces support for the sensor interface in the MediaTek SoC, > with the focus on CSI and stream control. The key functionalities > include parameter control, metering and maintaining status information, > interrupt handling, and debugging. These features ensure effective > management and debugging of the camera sensor interface hardware. > > Signed-off-by: Shu-hsiang Yang <Shu-hsiang.Yang(a)mediatek.com> > --- > .../isp_7x/camsys/mtk_csi_phy_2_0/Makefile | 5 + > .../mtk_csi_phy_2_0/mtk_cam-seninf-cammux.h | 911 ++++++ > .../mtk_cam-seninf-csi0-cphy.h | 69 + > .../mtk_cam-seninf-csi0-dphy.h | 139 + > .../mtk_cam-seninf-hw_phy_2_0.c | 2879 +++++++++++++++++ > .../mtk_cam-seninf-mipi-rx-ana-cdphy-csi0a.h | 257 ++ > .../mtk_cam-seninf-seninf1-csi2.h | 415 +++ > .../mtk_cam-seninf-seninf1-mux.h | 147 + > .../mtk_csi_phy_2_0/mtk_cam-seninf-seninf1.h | 47 + > .../mtk_csi_phy_2_0/mtk_cam-seninf-tg1.h | 49 + > .../mtk_csi_phy_2_0/mtk_cam-seninf-top-ctrl.h | 99 + > 11 files changed, 5017 insertions(+) > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/Makefile The PHY driver should go to the PHY directory as a PHY driver - or at least part of this mtk_csi_phy_2_0 driver should go there. I see that this is tightly integrated with the rest of the code in seninf, but there seems to be many functions that are just handling a "real" PHY. > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-cammux.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-csi0-cphy.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-csi0-dphy.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-hw_phy_2_0.c > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-mipi-rx-ana-cdphy-csi0a.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-seninf1-csi2.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-seninf1-mux.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-seninf1.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-tg1.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-top-ctrl.h > > diff --git a/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/Makefile b/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/Makefile > new file mode 100644 > index 000000000000..e00b8d3904a9 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 MediaTek Inc. > + > +mtk-cam-isp-objs += \ > + mtk_csi_phy_2_0/mtk_cam-seninf-hw_phy_2_0.o > diff --git a/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-cammux.h b/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-cammux.h > new file mode 100644 > index 000000000000..ec3c621d742a > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-cammux.h > @@ -0,0 +1,911 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2022 MediaTek Inc. > + */ > + > +#ifndef __SENINF_CAM_MUX_H__ > +#define __SENINF_CAM_MUX_H__ > + > +#define SENINF_CAM_MUX_CTRL_0 0x0000 > +#define RG_SENINF_CAM_MUX0_SRC_SEL_SHIFT 0 > +#define RG_SENINF_CAM_MUX0_SRC_SEL_MASK (0xf << 0) For all definitions: drop _SHIFT and use the GENMASK(x,y) macro. #define RG_SENINF_CAM_MUX0_SRC_SEL GENMASK(7, 0) #define RG_SENINF_CAM_MUX1_SRC_SEL GENMASK(11, 8) etc. > +#define RG_SENINF_CAM_MUX1_SRC_SEL_SHIFT 8 > +#define RG_SENINF_CAM_MUX1_SRC_SEL_MASK (0xf << 8) > +#define RG_SENINF_CAM_MUX2_SRC_SEL_SHIFT 16 > +#define RG_SENINF_CAM_MUX2_SRC_SEL_MASK (0xf << 16) > +#define RG_SENINF_CAM_MUX3_SRC_SEL_SHIFT 24 > +#define RG_SENINF_CAM_MUX3_SRC_SEL_MASK (0xf << 24) ..snip.. > diff --git a/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-hw_phy_2_0.c b/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-hw_phy_2_0.c > new file mode 100644 > index 000000000000..f24d8a056d0e > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_csi_phy_2_0/mtk_cam-seninf-hw_phy_2_0.c > @@ -0,0 +1,2879 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2022 MediaTek Inc. > + > +#include <linux/module.h> > +#include <linux/delay.h> > + > +#include "../mtk_cam-seninf.h" > +#include "../mtk_cam-seninf-hw.h" > +#include "../mtk_cam-seninf-regs.h" > +#include "mtk_cam-seninf-top-ctrl.h" > +#include "mtk_cam-seninf-seninf1-mux.h" > +#include "mtk_cam-seninf-seninf1.h" > +#include "mtk_cam-seninf-seninf1-csi2.h" > +#include "mtk_cam-seninf-tg1.h" > +#include "mtk_cam-seninf-cammux.h" > +#include "mtk_cam-seninf-mipi-rx-ana-cdphy-csi0a.h" > +#include "mtk_cam-seninf-csi0-cphy.h" > +#include "mtk_cam-seninf-csi0-dphy.h" > +#include "../kd_imgsensor_define_v4l2.h" > + > +/* seninf cfg default, dts may override */ dts may override: for which reason? Why should DT override that? For which usecase? > +static struct mtk_cam_seninf_cfg _seninf_cfg = { > + .mux_num = 8, > + .seninf_num = 4, > + .cam_mux_num = 11, > + .pref_mux_num = 11, > +}; > + > +struct mtk_cam_seninf_cfg *g_seninf_cfg = &_seninf_cfg; That's unused. Drop. > + > +static inline void mtk_cam_seninf_set_di_ch_ctrl(void __iomem *pseninf, > + unsigned int stream_id, > + struct seninf_vc *vc) > +{ > + if (stream_id > 7) No magic numbers, please. #define SOMETHING 7 > + return; > + > + SENINF_BITS(pseninf, SENINF_CSI2_S0_DI_CTRL + (stream_id << 0x2), Is it me, or is SENINF_BITS() not defined?! > + RG_CSI2_DT_SEL, vc->dt); > + SENINF_BITS(pseninf, SENINF_CSI2_S0_DI_CTRL + (stream_id << 0x2), > + RG_CSI2_VC_SEL, vc->vc); > + SENINF_BITS(pseninf, SENINF_CSI2_S0_DI_CTRL + (stream_id << 0x2), > + RG_CSI2_DT_INTERLEAVE_MODE, 1); > + SENINF_BITS(pseninf, SENINF_CSI2_S0_DI_CTRL + (stream_id << 0x2), > + RG_CSI2_VC_INTERLEAVE_EN, 1); > + ..snip.. > +} > + > +int mtk_cam_seninf_init_iomem(struct seninf_ctx *ctx, void __iomem *if_base, > + void __iomem *ana_base) > +{ > + u32 i; > + > + ctx->reg_ana_csi_rx[CSI_PORT_0] = Please don't use "a = b = c" assignments. In this case, that impacts on human readability. > + ctx->reg_ana_csi_rx[CSI_PORT_0A] = ana_base + 0; > + ctx->reg_ana_csi_rx[CSI_PORT_0B] = ana_base + 0x1000; Again, no magic numbers please. #define SOMETHING 0x1000 > + > + ctx->reg_ana_csi_rx[CSI_PORT_1] = > + ctx->reg_ana_csi_rx[CSI_PORT_1A] = ana_base + 0x4000; > + ctx->reg_ana_csi_rx[CSI_PORT_1B] = ana_base + 0x5000; ..snip.. > + return 0; > +} > + > +int mtk_cam_seninf_init_port(struct seninf_ctx *ctx, int port) > +{ > + u32 port_num; > + > + if (port >= CSI_PORT_0A) > + port_num = (port - CSI_PORT_0) >> 1; I think that you really want to use bitfield.h macros to simplify this. > + else > + port_num = port; > + > + ctx->port = port; > + ctx->port_num = port_num; > + ctx->port_a = CSI_PORT_0A + (port_num << 1); > + ctx->port_b = ctx->port_a + 1; > + ctx->is_4d1c = (port == port_num); > + > + switch (port) { > + case CSI_PORT_0: > + ctx->seninf_idx = SENINF_1; Is the CSI port to SENINF mapping supposed to be static and unchangeable? If yes, then you want to add that into an array indexed by CSI_PORT_xx, so that you end up doing ctx->seninf_idx = seninf_csi[port]; > + break; > + case CSI_PORT_0A: > + ctx->seninf_idx = SENINF_1; > + break; > + case CSI_PORT_0B: > + ctx->seninf_idx = SENINF_2; > + break; > + case CSI_PORT_1: > + ctx->seninf_idx = SENINF_3; > + break; > + case CSI_PORT_1A: > + ctx->seninf_idx = SENINF_3; > + break; > + case CSI_PORT_1B: > + ctx->seninf_idx = SENINF_4; > + break; > + case CSI_PORT_2: > + ctx->seninf_idx = SENINF_5; > + break; > + case CSI_PORT_2A: > + ctx->seninf_idx = SENINF_5; > + break; > + case CSI_PORT_2B: > + ctx->seninf_idx = SENINF_6; > + break; > + case CSI_PORT_3: > + ctx->seninf_idx = SENINF_7; > + break; > + case CSI_PORT_3A: > + ctx->seninf_idx = SENINF_7; > + break; > + case CSI_PORT_3B: > + ctx->seninf_idx = SENINF_8; > + break; > + default: > + dev_dbg(ctx->dev, "invalid port %d\n", port); > + return -EINVAL; > + } > + > + return 0; > +} > + > +int mtk_cam_seninf_is_cammux_used(struct seninf_ctx *ctx, int cam_mux) > +{ > + void __iomem *seninf_cam_mux = ctx->reg_if_cam_mux; > + u32 temp = SENINF_READ_REG(seninf_cam_mux, SENINF_CAM_MUX_EN); Please, just use readl() or regmap_read() (preferrably the latter, and preferrably also define regmap reg fields). > + > + if (cam_mux >= _seninf_cfg.cam_mux_num) { > + dev_dbg(ctx->dev, > + "%s err cam_mux %d >= SENINF_CAM_MUX_NUM %d\n", > + __func__, cam_mux, _seninf_cfg.cam_mux_num); That shouldn't ever happen, should it. In that case, it's dev_err(), or even a WARN(), if necessary. > + > + return 0; > + } > + > + return !!(temp & (1 << cam_mux)); > +} > + > +int mtk_cam_seninf_cammux(struct seninf_ctx *ctx, int cam_mux) > +{ > + void __iomem *seninf_cam_mux = ctx->reg_if_cam_mux; > + u32 temp; > + > + if (cam_mux >= _seninf_cfg.cam_mux_num) { This check is duplicated across many functions... you want to do something such that it either never happens or it gets commonized. > + dev_dbg(ctx->dev, > + "%s err cam_mux %d >= SENINF_CAM_MUX_NUM %d\n", > + __func__, cam_mux, _seninf_cfg.cam_mux_num); > + > + return 0; You return 0, so there's no problem if this is the wrong cam_mux?!?!? > + } > + > + temp = SENINF_READ_REG(seninf_cam_mux, SENINF_CAM_MUX_EN); > + SENINF_WRITE_REG(seninf_cam_mux, SENINF_CAM_MUX_EN, > + temp | (1 << cam_mux)); BIT(cam_mux) > + > + SENINF_WRITE_REG(seninf_cam_mux, SENINF_CAM_MUX_IRQ_STATUS, > + 3 << (cam_mux * 2)); /* clr irq */ > + > + dev_dbg(ctx->dev, "cam_mux %d EN 0x%x IRQ_EN 0x%x IRQ_STATUS 0x%x\n", > + cam_mux, SENINF_READ_REG(seninf_cam_mux, SENINF_CAM_MUX_EN), > + SENINF_READ_REG(seninf_cam_mux, SENINF_CAM_MUX_IRQ_EN), > + SENINF_READ_REG(seninf_cam_mux, SENINF_CAM_MUX_IRQ_STATUS)); > + > + return 0; > +} > + > + > +int mtk_cam_seninf_set_vc(struct seninf_ctx *ctx, u32 seninf_idx, > + struct seninf_vcinfo *vcinfo) > +{ > + void __iomem *seninf_csi2 = ctx->reg_if_csi2[seninf_idx]; > + int i; > + struct seninf_vc *vc; > + > + if (!vcinfo || !vcinfo->cnt) > + return 0; Does !vcinfo mean that Virtual Channel should not be set? Please add a comment explaining that. > + > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S0_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S1_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S2_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S3_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S4_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S5_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S6_DI_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_S7_DI_CTRL, 0); > + > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_CH0_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_CH1_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_CH2_CTRL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_CH3_CTRL, 0); > + > + for (i = 0; i < vcinfo->cnt; i++) { > + vc = &vcinfo->vc[i]; > + > + /* General Long Packet Data Types: 0x10-0x17 */ > + if (vc->dt >= 0x10 && vc->dt <= 0x17) { > + SENINF_BITS(seninf_csi2, SENINF_CSI2_OPT, > + RG_CSI2_GENERIC_LONG_PACKET_EN, 1); > + } > + > + mtk_cam_seninf_set_di_ch_ctrl(seninf_csi2, i, vc); > + } > + > + dev_dbg(ctx->dev, "DI_CTRL 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n", > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S0_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S1_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S2_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S3_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S4_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S5_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S6_DI_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_S7_DI_CTRL)); > + > + dev_dbg(ctx->dev, "CH_CTRL 0x%x 0x%x 0x%x 0x%x\n", > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_CH0_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_CH1_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_CH2_CTRL), > + SENINF_READ_REG(seninf_csi2, SENINF_CSI2_CH3_CTRL)); > + > + return 0; > +} > + ..snip.. > + > +static int csirx_phyA_power_on(struct seninf_ctx *ctx, u32 port_idx, int en) Please, lower case on all functions. csirx_phy_a_power_on(). Also, you want to split this function in two: one should be csirx_phy_a_power_off() and one for power_on(). static void csirx_phy_a_power_off(struct seninf_ctx *ctx, u32 port_idx) { > +{ > + void __iomem *base = ctx->reg_ana_csi_rx[port_idx]; > + > + SENINF_BITS(base, CDPHY_RX_ANA_8, RG_CSI0_L0_T0AB_EQ_OS_CAL_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_8, RG_CSI0_L1_T1AB_EQ_OS_CAL_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_8, RG_CSI0_L2_T1BC_EQ_OS_CAL_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_8, RG_CSI0_XX_T0BC_EQ_OS_CAL_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_8, RG_CSI0_XX_T0CA_EQ_OS_CAL_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_8, RG_CSI0_XX_T1CA_EQ_OS_CAL_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, RG_CSI0_BG_LPF_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, RG_CSI0_BG_CORE_EN, 0); > + usleep_range(200, 300); > + } static int csirx_phy_a_power_on(struct seninf_ctx *ctx, u32 port_idx) <-- NO int en { void *iomem base = ..... /* Power it off first to ... reset, I believe? */ csirx_phy_a_power_off(); power on sequence here return 0; } > + if (en) { > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_BG_CORE_EN, 1); > + usleep_range(30, 40); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_BG_LPF_EN, 1); > + udelay(1); > + SENINF_BITS(base, CDPHY_RX_ANA_8, > + RG_CSI0_L0_T0AB_EQ_OS_CAL_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_8, > + RG_CSI0_L1_T1AB_EQ_OS_CAL_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_8, > + RG_CSI0_L2_T1BC_EQ_OS_CAL_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_8, > + RG_CSI0_XX_T0BC_EQ_OS_CAL_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_8, > + RG_CSI0_XX_T0CA_EQ_OS_CAL_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_8, > + RG_CSI0_XX_T1CA_EQ_OS_CAL_EN, 1); > + udelay(1); > + } > + > + return 0; > +} > + > +static int apply_efuse_data(struct seninf_ctx *ctx) > +{ void __iomem *base; u32 m_csi_efuse = ctx->m_csi_efuse; u32 port; int ret; > + int ret = 0; > + void __iomem *base; > + u32 port; > + u32 m_csi_efuse = ctx->m_csi_efuse; > + > + if (m_csi_efuse == 0) { > + dev_dbg(ctx->dev, "No efuse data. Returned.\n"); > + return -1; > + } ..snip.. > + > + return ret; return 0; > +} > + > +static int csirx_phyA_init(struct seninf_ctx *ctx) > +{ > + u32 i, port; > + void __iomem *base; > + > + port = ctx->port; > + for (i = 0; i <= ctx->is_4d1c; i++) { Move the setup in a different function, then call it with something(port); if (is_4d1c) something(port_b); > + port = i ? ctx->port_b : ctx->port; > + base = ctx->reg_ana_csi_rx[port]; > + SENINF_BITS(base, CDPHY_RX_ANA_1, > + RG_CSI0_BG_LPRX_VTL_SEL, 0x4); > + SENINF_BITS(base, CDPHY_RX_ANA_1, > + RG_CSI0_BG_LPRX_VTH_SEL, 0x4); > + SENINF_BITS(base, CDPHY_RX_ANA_2, > + RG_CSI0_BG_ALP_RX_VTL_SEL, 0x4); > + SENINF_BITS(base, CDPHY_RX_ANA_2, > + RG_CSI0_BG_ALP_RX_VTH_SEL, 0x4); > + SENINF_BITS(base, CDPHY_RX_ANA_1, > + RG_CSI0_BG_VREF_SEL, 0x8); > + SENINF_BITS(base, CDPHY_RX_ANA_1, > + RG_CSI0_CDPHY_EQ_DES_VREF_SEL, 0x2); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_9, > + RG_CSI0_RESERVE, 0x3003); > + SENINF_BITS(base, CDPHY_RX_ANA_SETTING_0, > + CSR_CSI_RST_MODE, 0x2); > + > + SENINF_BITS(base, CDPHY_RX_ANA_2, > + RG_CSI0_L0P_T0A_HSRT_CODE, 0x10); > + SENINF_BITS(base, CDPHY_RX_ANA_2, > + RG_CSI0_L0N_T0B_HSRT_CODE, 0x10); > + SENINF_BITS(base, CDPHY_RX_ANA_3, > + RG_CSI0_L1P_T0C_HSRT_CODE, 0x10); > + SENINF_BITS(base, CDPHY_RX_ANA_3, > + RG_CSI0_L1N_T1A_HSRT_CODE, 0x10); > + SENINF_BITS(base, CDPHY_RX_ANA_4, > + RG_CSI0_L2P_T1B_HSRT_CODE, 0x10); > + SENINF_BITS(base, CDPHY_RX_ANA_4, > + RG_CSI0_L2N_T1C_HSRT_CODE, 0x10); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_T0_CDR_FIRST_EDGE_EN, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_T1_CDR_FIRST_EDGE_EN, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_2, > + RG_CSI0_CPHY_T0_CDR_SELF_CAL_EN, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_2, > + RG_CSI0_CPHY_T1_CDR_SELF_CAL_EN, 0x0); > + > + SENINF_BITS(base, CDPHY_RX_ANA_6, > + RG_CSI0_CPHY_T0_CDR_CK_DELAY, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_7, > + RG_CSI0_CPHY_T1_CDR_CK_DELAY, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_6, > + RG_CSI0_CPHY_T0_CDR_AB_WIDTH, 0x9); > + SENINF_BITS(base, CDPHY_RX_ANA_6, > + RG_CSI0_CPHY_T0_CDR_BC_WIDTH, 0x9); > + SENINF_BITS(base, CDPHY_RX_ANA_6, > + RG_CSI0_CPHY_T0_CDR_CA_WIDTH, 0x9); > + SENINF_BITS(base, CDPHY_RX_ANA_7, > + RG_CSI0_CPHY_T1_CDR_AB_WIDTH, 0x9); > + SENINF_BITS(base, CDPHY_RX_ANA_7, > + RG_CSI0_CPHY_T1_CDR_BC_WIDTH, 0x9); > + SENINF_BITS(base, CDPHY_RX_ANA_7, > + RG_CSI0_CPHY_T1_CDR_CA_WIDTH, 0x9); > + > + dev_dbg(ctx->dev, "port:%d CDPHY_RX_ANA_0(0x%x)\n", > + port, SENINF_READ_REG(base, CDPHY_RX_ANA_0)); > + } > + > + apply_efuse_data(ctx); > + > + return 0; > +} > + > +static int csirx_dphy_init(struct seninf_ctx *ctx) > +{ > + void __iomem *base = ctx->reg_ana_dphy_top[ctx->port]; > + int settle_delay_dt, settle_delay_ck, hs_trail, hs_trail_en; > + int bit_per_pixel; u8 bits_per_pixel; > + u64 data_rate; > + > + settle_delay_dt = ctx->is_cphy ? ctx->core->cphy_settle_delay_dt : > + ctx->core->dphy_settle_delay_dt; Please set settle_delay_ck and hs_trail here. > + > + SENINF_BITS(base, DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_CDPHY_RX_LD0_TRIO0_HS_SETTLE_PARAMETER, > + settle_delay_dt); > + SENINF_BITS(base, DPHY_RX_DATA_LANE1_HS_PARAMETER, > + RG_CDPHY_RX_LD1_TRIO1_HS_SETTLE_PARAMETER, > + settle_delay_dt); > + SENINF_BITS(base, DPHY_RX_DATA_LANE2_HS_PARAMETER, > + RG_CDPHY_RX_LD2_TRIO2_HS_SETTLE_PARAMETER, > + settle_delay_dt); > + SENINF_BITS(base, DPHY_RX_DATA_LANE3_HS_PARAMETER, > + RG_CDPHY_RX_LD3_TRIO3_HS_SETTLE_PARAMETER, > + settle_delay_dt); > + > + settle_delay_ck = ctx->core->settle_delay_ck; > + > + SENINF_BITS(base, DPHY_RX_CLOCK_LANE0_HS_PARAMETER, > + RG_DPHY_RX_LC0_HS_SETTLE_PARAMETER, > + settle_delay_ck); > + SENINF_BITS(base, DPHY_RX_CLOCK_LANE1_HS_PARAMETER, > + RG_DPHY_RX_LC1_HS_SETTLE_PARAMETER, > + settle_delay_ck); > + > + SENINF_BITS(base, DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_CDPHY_RX_LD0_TRIO0_HS_PREPARE_PARAMETER, 2); > + SENINF_BITS(base, DPHY_RX_DATA_LANE1_HS_PARAMETER, > + RG_CDPHY_RX_LD1_TRIO1_HS_PREPARE_PARAMETER, 2); > + SENINF_BITS(base, DPHY_RX_DATA_LANE2_HS_PARAMETER, > + RG_CDPHY_RX_LD2_TRIO2_HS_PREPARE_PARAMETER, 2); > + SENINF_BITS(base, DPHY_RX_DATA_LANE3_HS_PARAMETER, > + RG_CDPHY_RX_LD3_TRIO3_HS_PREPARE_PARAMETER, 2); > + > + hs_trail = ctx->hs_trail_parameter; > + > + SENINF_BITS(base, DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_DPHY_RX_LD0_HS_TRAIL_PARAMETER, hs_trail); > + SENINF_BITS(base, DPHY_RX_DATA_LANE1_HS_PARAMETER, > + RG_DPHY_RX_LD1_HS_TRAIL_PARAMETER, hs_trail); > + SENINF_BITS(base, DPHY_RX_DATA_LANE2_HS_PARAMETER, > + RG_DPHY_RX_LD2_HS_TRAIL_PARAMETER, hs_trail); > + SENINF_BITS(base, DPHY_RX_DATA_LANE3_HS_PARAMETER, > + RG_DPHY_RX_LD3_HS_TRAIL_PARAMETER, hs_trail); > + > + if (!ctx->is_cphy) { > + bit_per_pixel = 10; data_rate = ctx->customized_pixel_rate ? ctx->customized_pixel_rate : ctx->mipi_pixel_rate; data_rate *= bits_per_pixel; > + if (ctx->customized_pixel_rate != 0) > + data_rate = ctx->customized_pixel_rate * bit_per_pixel; > + else > + data_rate = ctx->mipi_pixel_rate * bit_per_pixel; > + > + do_div(data_rate, ctx->num_data_lanes); > + hs_trail_en = data_rate < 1400000000; > + SENINF_BITS(base, DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_DPHY_RX_LD0_HS_TRAIL_EN, hs_trail_en); > + SENINF_BITS(base, DPHY_RX_DATA_LANE1_HS_PARAMETER, > + RG_DPHY_RX_LD1_HS_TRAIL_EN, hs_trail_en); > + SENINF_BITS(base, DPHY_RX_DATA_LANE2_HS_PARAMETER, > + RG_DPHY_RX_LD2_HS_TRAIL_EN, hs_trail_en); > + SENINF_BITS(base, DPHY_RX_DATA_LANE3_HS_PARAMETER, > + RG_DPHY_RX_LD3_HS_TRAIL_EN, hs_trail_en); > + } > + > + return 0; > +} > + > +static int csirx_cphy_init(struct seninf_ctx *ctx) > +{ > + void __iomem *base = ctx->reg_ana_cphy_top[ctx->port]; > + > + SENINF_BITS(base, CPHY_RX_DETECT_CTRL_POST, > + RG_CPHY_RX_DATA_VALID_POST_EN, 1); > + > + return 0; > +} > + > +static int csirx_phy_init(struct seninf_ctx *ctx) > +{ > + csirx_phyA_init(ctx); > + > + csirx_dphy_init(ctx); > + csirx_cphy_init(ctx); > + > + return 0; > +} > + > +static int csirx_seninf_csi2_setting(struct seninf_ctx *ctx) > +{ > + void __iomem *seninf_csi2 = ctx->reg_if_csi2[ctx->seninf_idx]; > + int csi_en; > + > + SENINF_BITS(seninf_csi2, SENINF_CSI2_DBG_CTRL, > + RG_CSI2_DBG_PACKET_CNT_EN, 1); > + > + /* lane/trio count */ > + SENINF_BITS(seninf_csi2, SENINF_CSI2_RESYNC_MERGE_CTRL, > + RG_CSI2_RESYNC_CYCLE_CNT_OPT, 1); > + > + csi_en = (1 << ctx->num_data_lanes) - 1; csi_en = BIT(ctx->num_data_lanes) - 1; or csi_en = GENMASK(ctx->num_data_lanes - 1, 0); > + > + if (!ctx->is_cphy) { > + SENINF_BITS(seninf_csi2, SENINF_CSI2_OPT, RG_CSI2_CPHY_SEL, 0); > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_EN, csi_en); > + SENINF_BITS(seninf_csi2, SENINF_CSI2_HDR_MODE_0, > + RG_CSI2_HEADER_MODE, 0); > + SENINF_BITS(seninf_csi2, SENINF_CSI2_HDR_MODE_0, > + RG_CSI2_HEADER_LEN, 0); > + } else { > + u8 map_hdr_len[] = {0, 1, 2, 4, 5}; u8 map_hdr_len[] = { 0, 1, 2, 4, 5 }; > + > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_EN, csi_en); > + SENINF_BITS(seninf_csi2, SENINF_CSI2_OPT, > + RG_CSI2_CPHY_SEL, 1); > + SENINF_BITS(seninf_csi2, SENINF_CSI2_HDR_MODE_0, > + RG_CSI2_HEADER_MODE, 2); > + SENINF_BITS(seninf_csi2, SENINF_CSI2_HDR_MODE_0, > + RG_CSI2_HEADER_LEN, > + map_hdr_len[ctx->num_data_lanes]); > + } > + > + return 0; > +} > + > +static int csirx_seninf_setting(struct seninf_ctx *ctx) > +{ > + void __iomem *seninf = ctx->reg_if_ctrl[ctx->seninf_idx]; > + > + /* enable/disable seninf csi2 */ > + SENINF_BITS(seninf, SENINF_CSI2_CTRL, RG_SENINF_CSI2_EN, 1); > + > + /* enable/disable seninf, enable after csi2, testmdl is done */ > + SENINF_BITS(seninf, SENINF_CTRL, SENINF_EN, 1); > + > + return 0; > +} > + > +static int csirx_seninf_top_setting(struct seninf_ctx *ctx) > +{ ..snip.. > + > + /* port operation mode */ > + switch (ctx->port) { > + case CSI_PORT_0: > + case CSI_PORT_0A: > + case CSI_PORT_0B: reg = TOP_PHY_CTRL_CSI0; field_cphy = PHY_SENINF_MUX0_CPHY_EN; field_dphy = PHY_SENINF_MUX0_DPHY_EN; break; case CSI_PORT_1: case CSI_PORT_1A: case CSI_PORT_1B: reg = TOP_PHY_CTRL_CSI1; field_cphy = PHY_SENINF_MUX1_CPHY_EN; field_dphy = PHY_SENINF_MUX1_DPHY_EN; break; .... etc } regmap_write_bits(regmap, reg, field_cphy, ctx->is_cphy); regmap_write_bits(regmap, reg, field_dphy, !ctx->is_cphy); > + if (!ctx->is_cphy) { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI0, > + PHY_SENINF_MUX0_CPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI0, > + PHY_SENINF_MUX0_DPHY_EN, 1); > + } else { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI0, > + PHY_SENINF_MUX0_DPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI0, > + PHY_SENINF_MUX0_CPHY_EN, 1); > + } > + break; > + case CSI_PORT_1: > + case CSI_PORT_1A: > + case CSI_PORT_1B: > + if (!ctx->is_cphy) { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI1, > + PHY_SENINF_MUX1_CPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI1, > + PHY_SENINF_MUX1_DPHY_EN, 1); > + } else { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI1, > + PHY_SENINF_MUX1_DPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI1, > + PHY_SENINF_MUX1_CPHY_EN, 1); > + } > + break; > + case CSI_PORT_2: > + case CSI_PORT_2A: > + case CSI_PORT_2B: > + if (!ctx->is_cphy) { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI2, > + PHY_SENINF_MUX2_CPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI2, > + PHY_SENINF_MUX2_DPHY_EN, 1); > + } else { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI2, > + PHY_SENINF_MUX2_DPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI2, > + PHY_SENINF_MUX2_CPHY_EN, 1); > + } > + break; > + case CSI_PORT_3: > + case CSI_PORT_3A: > + case CSI_PORT_3B: > + if (!ctx->is_cphy) { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI3, > + PHY_SENINF_MUX3_CPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI3, > + PHY_SENINF_MUX3_DPHY_EN, 1); > + } else { > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI3, > + PHY_SENINF_MUX3_DPHY_EN, 0); > + SENINF_BITS(seninf_top, SENINF_TOP_PHY_CTRL_CSI3, > + PHY_SENINF_MUX3_CPHY_EN, 1); > + } > + break; > + default: > + break; So if this is called with a CSI_PORT that is out of range, we're not erroring out?!?! > + } > + > + return 0; > +} > + > +static int csirx_phyA_setting(struct seninf_ctx *ctx) > +{ > + void __iomem *base, *baseA, *baseB; > + > + base = ctx->reg_ana_csi_rx[ctx->port]; > + baseA = ctx->reg_ana_csi_rx[ctx->port_a]; > + baseB = ctx->reg_ana_csi_rx[ctx->port_b]; > + > + if (!ctx->is_cphy) { /* dphy */ For this huge register write sequence, you want to split it in at least two functions to improve human readability and reduce indentation. > + if (ctx->is_4d1c) { > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_EN, 0); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_EN, 0); > + /* clear clk sel first */ > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKMODE_EN, 0); > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKMODE_EN, 0); > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKMODE_EN, 0); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKMODE_EN, 0); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKMODE_EN, 0); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKMODE_EN, 0); > + > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKSEL, 1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKSEL, 1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKSEL, 1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKSEL, 1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKSEL, 1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKSEL, 1); > + > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKMODE_EN, 0); > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKMODE_EN, 0); > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKMODE_EN, 1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKMODE_EN, 0); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKMODE_EN, 0); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKMODE_EN, 0); > + > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + } else { > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_EN, 0); > + /* clear clk sel first */ > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKMODE_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKMODE_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKMODE_EN, 0); > + > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKSEL, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKSEL, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKSEL, 0); > + > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L0_CKMODE_EN, 0); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L1_CKMODE_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_DPHY_L2_CKMODE_EN, 0); > + > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + } > + } else { /* cphy */ > + if (ctx->is_4d1c) { > + SENINF_BITS(baseA, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_EN, 1); > + SENINF_BITS(baseB, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_EN, 1); > + > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x0); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x3); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x0); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x3); > + SENINF_BITS(baseA, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + } else { > + SENINF_BITS(base, CDPHY_RX_ANA_0, > + RG_CSI0_CPHY_EN, 1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_BW, 0x3); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_IS, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_LATCH_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG0_EN, 0x1); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_DG1_EN, 0x0); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR0, 0x3); > + SENINF_BITS(base, CDPHY_RX_ANA_5, > + RG_CSI0_CDPHY_EQ_SR1, 0x0); > + } > + } > + > + /* phyA power on */ > + if (ctx->is_4d1c) { > + csirx_phyA_power_on(ctx, ctx->port_a, 1); > + csirx_phyA_power_on(ctx, ctx->port_b, 1); > + } else { > + csirx_phyA_power_on(ctx, ctx->port, 1); > + } > + > + return 0; > +} > + > +static int csirx_dphy_setting(struct seninf_ctx *ctx) > +{ > + void __iomem *base = ctx->reg_ana_dphy_top[ctx->port]; > + > + if (ctx->is_4d1c) { > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD3_SEL, 4); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD2_SEL, 0); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD1_SEL, 3); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD0_SEL, 1); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LC0_SEL, 2); > + > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD0_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD1_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD2_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD3_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LC0_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LC1_EN, 0); > + } else { > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD3_SEL, 5); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD2_SEL, 3); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD1_SEL, 2); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LD0_SEL, 0); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LC1_SEL, 4); > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, RG_DPHY_RX_LC0_SEL, 1); > + > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD0_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD1_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD2_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LD3_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LC0_EN, 1); > + SENINF_BITS(base, DPHY_RX_LANE_EN, DPHY_RX_LC1_EN, 1); > + } > + > + SENINF_BITS(base, DPHY_RX_LANE_SELECT, DPHY_RX_CK_DATA_MUX_EN, 1); > + > + return 0; > +} > + > +static int csirx_cphy_setting(struct seninf_ctx *ctx) > +{ > + void __iomem *base = ctx->reg_ana_cphy_top[ctx->port]; u32 reg_lprx[] = { CPHY_RX_TR0_LPRX_EN, CPHY_RX_TR1_LPRX_EN, ...TR2, ...TR3 }; > + > + switch (ctx->port) { > + case CSI_PORT_0: > + case CSI_PORT_1: > + case CSI_PORT_2: > + case CSI_PORT_3: > + case CSI_PORT_0A: > + case CSI_PORT_1A: > + case CSI_PORT_2A: > + case CSI_PORT_3A: for (i = 0; i < ctx->num_data_lanes; i++) regmap_write_bits(regmap, CPHY_RX_CTRL, reg_lprx[i], 1); for (; i < ARRAY_SIZE(reg_lprx); i++) regmap_write_bits(regmap, CPHY_RX_CTRL, reg_lprx[i], 0); > + if (ctx->num_data_lanes == 3) { > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR0_LPRX_EN, 1); > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR1_LPRX_EN, 1); > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR2_LPRX_EN, 1); > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR3_LPRX_EN, 0); > + } else if (ctx->num_data_lanes == 2) { > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR0_LPRX_EN, 1); > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR1_LPRX_EN, 1); > + } else { > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR0_LPRX_EN, 1); > + } > + break; > + case CSI_PORT_0B: > + case CSI_PORT_1B: > + case CSI_PORT_2B: > + case CSI_PORT_3B: for (i = 0; i < ctx->num_data_lanes; i++) { regmap_write_bits(regmap, CPHY_RX_CTRL, reg_lprx[i], 0); regmap_write_bits(regmap, CPHY_RX_CTRL, reg_lprx[i+2], 1); } > + if (ctx->num_data_lanes == 2) { > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR2_LPRX_EN, 1); > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR3_LPRX_EN, 1); > + } else { > + SENINF_BITS(base, CPHY_RX_CTRL, CPHY_RX_TR2_LPRX_EN, 1); > + } > + break; > + default: > + break; > + } > + > + return 0; > +} > + > +static int csirx_phy_setting(struct seninf_ctx *ctx) > +{ > + csirx_phyA_setting(ctx); > + Please, if (ctx->is_cphy) x() else y() > + if (!ctx->is_cphy) > + csirx_dphy_setting(ctx); > + else > + csirx_cphy_setting(ctx); > + > + return 0; > +} > + > +int mtk_cam_seninf_set_csi_mipi(struct seninf_ctx *ctx) > +{ > + csirx_phy_init(ctx); > + > + /* seninf csi2 */ > + csirx_seninf_csi2_setting(ctx); > + > + /* seninf */ > + csirx_seninf_setting(ctx); > + > + /* seninf top */ > + csirx_seninf_top_setting(ctx); > + > + /* phy */ > + csirx_phy_setting(ctx); > + > + return 0; > +} > + > +int mtk_cam_seninf_poweroff(struct seninf_ctx *ctx) > +{ > + void __iomem *seninf_csi2; > + > + seninf_csi2 = ctx->reg_if_csi2[ctx->seninf_idx]; > + > + SENINF_WRITE_REG(seninf_csi2, SENINF_CSI2_EN, 0x0); > + > + if (ctx->is_4d1c) { > + csirx_phyA_power_on(ctx, ctx->port_a, 0); > + csirx_phyA_power_on(ctx, ctx->port_b, 0); > + } else { > + csirx_phyA_power_on(ctx, ctx->port, 0); > + } > + > + return 0; > +} > + > +int mtk_cam_seninf_reset(struct seninf_ctx *ctx, u32 seninf_idx) > +{ > + int i; > + void __iomem *seninf_mux; > + void __iomem *seninf = ctx->reg_if_ctrl[seninf_idx]; > + > + SENINF_BITS(seninf, SENINF_CSI2_CTRL, SENINF_CSI2_SW_RST, 1); What about adding a reset controller that includes SENINF_CSI(x)_SW_RST? Also, why are you resetting only CSI2 and not the others? > + udelay(1); > + SENINF_BITS(seninf, SENINF_CSI2_CTRL, SENINF_CSI2_SW_RST, 0); > + > + dev_dbg(ctx->dev, "reset seninf %d\n", seninf_idx); > + > + for (i = SENINF_MUX1; i < _seninf_cfg.mux_num; i++) > + if (mtk_cam_seninf_get_top_mux_ctrl(ctx, i) == seninf_idx && > + mtk_cam_seninf_is_mux_used(ctx, i)) { > + seninf_mux = ctx->reg_if_mux[i]; > + SENINF_BITS(seninf_mux, SENINF_MUX_CTRL_0, > + SENINF_MUX_SW_RST, 1); > + udelay(1); > + SENINF_BITS(seninf_mux, SENINF_MUX_CTRL_0, > + SENINF_MUX_SW_RST, 0); > + dev_dbg(ctx->dev, "reset mux %d\n", i); > + } > + > + return 0; > +} > + > +int mtk_cam_seninf_set_idle(struct seninf_ctx *ctx) > +{ > + int i; > + struct seninf_vcinfo *vcinfo = &ctx->vcinfo; > + struct seninf_vc *vc; > + > + for (i = 0; i < vcinfo->cnt; i++) { > + vc = &vcinfo->vc[i]; > + if (vc->enable) { > + mtk_cam_seninf_disable_mux(ctx, vc->mux); > + mtk_cam_seninf_disable_cammux(ctx, vc->cam); > + ctx->pad2cam[vc->out_pad] = 0xff; > + } > + } > + > + return 0; > +} > + > +int mtk_cam_seninf_get_mux_meter(struct seninf_ctx *ctx, u32 mux, > + struct mtk_cam_seninf_mux_meter *meter) > +{ > + void __iomem *seninf_mux; > + s64 hv, hb, vv, vb, w, h; > + u64 mipi_pixel_rate, vb_in_us, hb_in_us, line_time_in_us; > + u32 res; > + > + seninf_mux = ctx->reg_if_mux[mux]; > + > + SENINF_BITS(seninf_mux, SENINF_MUX_FRAME_SIZE_MON_CTRL, > + RG_SENINF_MUX_FRAME_SIZE_MON_EN, 1); > + > + hv = SENINF_READ_REG(seninf_mux, SENINF_MUX_FRAME_SIZE_MON_H_VALID); > + hb = SENINF_READ_REG(seninf_mux, SENINF_MUX_FRAME_SIZE_MON_H_BLANK); > + vv = SENINF_READ_REG(seninf_mux, SENINF_MUX_FRAME_SIZE_MON_V_VALID); > + vb = SENINF_READ_REG(seninf_mux, SENINF_MUX_FRAME_SIZE_MON_V_BLANK); > + res = SENINF_READ_REG(seninf_mux, SENINF_MUX_SIZE); > + > + w = res & 0xffff; Another case of bitfield.h macros open-coded, or missed opportunity to just use regmap to simplify this. > + h = res >> 16; And same here, of course. > + > + if (ctx->fps_n && ctx->fps_d) { > + mipi_pixel_rate = w * ctx->fps_n * (vv + vb); > + do_div(mipi_pixel_rate, ctx->fps_d); Use div_s64() for safe 64-bits division. > + do_div(mipi_pixel_rate, hv); > + > + vb_in_us = vb * ctx->fps_d * 1000000; > + do_div(vb_in_us, vv + vb); > + do_div(vb_in_us, ctx->fps_n); > + > + hb_in_us = hb * ctx->fps_d * 1000000; > + do_div(hb_in_us, vv + vb); > + do_div(hb_in_us, ctx->fps_n); > + > + line_time_in_us = (hv + hb) * ctx->fps_d * 1000000; > + do_div(line_time_in_us, vv + vb); > + do_div(line_time_in_us, ctx->fps_n); > + > + meter->mipi_pixel_rate = mipi_pixel_rate; > + meter->vb_in_us = vb_in_us; > + meter->hb_in_us = hb_in_us; > + meter->line_time_in_us = line_time_in_us; > + } else { > + meter->mipi_pixel_rate = -1; > + meter->vb_in_us = -1; > + meter->hb_in_us = -1; > + meter->line_time_in_us = -1; > + } > + > + meter->width = w; > + meter->height = h; > + > + meter->h_valid = hv; > + meter->h_blank = hb; > + meter->v_valid = vv; > + meter->v_blank = vb; > + > + return 0; > +} > + > +ssize_t mtk_cam_seninf_show_status(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int i, len; > + struct seninf_core *core; struct seninf_core *core = dev_get_drvdata(dev); > + struct seninf_ctx *ctx; > + struct seninf_vc *vc; > + struct media_link *link; > + struct media_pad *pad; > + struct mtk_cam_seninf_mux_meter meter; > + void __iomem *csi2, *pmux; > + void __iomem *rx, *pcammux; int i, len = 0; > + > + core = dev_get_drvdata(dev); > + len = 0; > + > + mutex_lock(&core->mutex); > + > + list_for_each_entry(ctx, &core->list, list) { > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\n[%s] port %d intf %d test %d cphy %d lanes %d\n", > + ctx->subdev.name, ctx->port, ctx->seninf_idx, > + ctx->is_test_model, ctx->is_cphy, > + ctx->num_data_lanes); > + > + pad = &ctx->pads[PAD_SINK]; > + list_for_each_entry(link, &pad->entity->links, list) { > + if (link->sink == pad) { > + len += snprintf(buf + len, PAGE_SIZE - len, > + "source %s flags 0x%lx\n", > + link->source->entity->name, > + link->flags); > + } > + } > + > + if (!ctx->streaming) > + continue; > + > + csi2 = ctx->reg_if_csi2[ctx->seninf_idx]; > + rx = ctx->reg_ana_dphy_top[ctx->port]; > + > + len += snprintf(buf + len, PAGE_SIZE - len, > + "csi2 irq_stat 0x%08x\n", > + SENINF_READ_REG(csi2, SENINF_CSI2_IRQ_STATUS)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "csi2 line_frame_num 0x%08x\n", > + SENINF_READ_REG(csi2, SENINF_CSI2_LINE_FRAME_NUM)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "csi2 packet_status 0x%08x\n", > + SENINF_READ_REG(csi2, SENINF_CSI2_PACKET_STATUS)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "csi2 packet_cnt_status 0x%08x\n", > + SENINF_READ_REG(csi2, SENINF_CSI2_PACKET_CNT_STATUS)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "rx-ana settle ck 0x%02x dt 0x%02x\n", > + SENINF_READ_BITS(rx, DPHY_RX_CLOCK_LANE0_HS_PARAMETER, > + RG_DPHY_RX_LC0_HS_SETTLE_PARAMETER), > + SENINF_READ_BITS(rx, > + DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_CDPHY_RX_LD0_TRIO0_HS_SETTLE_PARAMETER)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "rx-ana trail en %u param 0x%02x\n", > + SENINF_READ_BITS(rx, DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_DPHY_RX_LD0_HS_TRAIL_EN), > + SENINF_READ_BITS(rx, DPHY_RX_DATA_LANE0_HS_PARAMETER, > + RG_DPHY_RX_LD0_HS_TRAIL_PARAMETER)); > + > + len += snprintf(buf + len, PAGE_SIZE - len, > + "data_not_enough_cnt : <%d>", > + ctx->data_not_enough_cnt); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "err_lane_resync_cnt : <%d>", > + ctx->err_lane_resync_cnt); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "crc_err_cnt : <%d>", ctx->crc_err_flag); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "ecc_err_double_cnt : <%d>", > + ctx->ecc_err_double_cnt); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "ecc_err_corrected_cnt : <%d>\n", > + ctx->ecc_err_corrected_cnt); > + > + for (i = 0; i < ctx->vcinfo.cnt; i++) { > + vc = &ctx->vcinfo.vc[i]; > + pmux = ctx->reg_if_mux[vc->mux]; > + pcammux = ctx->reg_if_cam_mux; > + > + len += snprintf(buf + len, PAGE_SIZE - len, > + "[%d] vc 0x%x dt 0x%x mux %d cam %d\n", > + i, vc->vc, vc->dt, vc->mux, vc->cam); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\tmux[%d] en %d src %d irq_stat 0x%x\n", > + vc->mux, > + mtk_cam_seninf_is_mux_used(ctx, vc->mux), > + mtk_cam_seninf_get_top_mux_ctrl(ctx, vc->mux), > + SENINF_READ_REG(pmux, SENINF_MUX_IRQ_STATUS)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\t\tfifo_overrun_cnt : <%d>\n", > + ctx->fifo_overrun_cnt); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\tcam[%d] en %d src %d exp 0x%x res 0x%x irq_stat 0x%x\n", > + vc->cam, > + mtk_cam_seninf_is_cammux_used(ctx, vc->cam), > + mtk_cam_seninf_get_cammux_ctrl(ctx, vc->cam), > + mtk_cam_seninf_get_cammux_exp(ctx, vc->cam), > + mtk_cam_seninf_get_cammux_res(ctx, vc->cam), > + SENINF_READ_REG(pcammux, > + SENINF_CAM_MUX_IRQ_STATUS)); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\t\tsize_err_cnt : <%d>\n", > + ctx->size_err_cnt); > + > + if (vc->feature == VC_RAW_DATA || > + vc->feature == VC_STAGGER_NE || > + vc->feature == VC_STAGGER_ME || > + vc->feature == VC_STAGGER_SE) { > + mtk_cam_seninf_get_mux_meter(ctx, vc->mux, > + &meter); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\t--- mux meter ---\n"); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\twidth %d height %d\n", > + meter.width, meter.height); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\th_valid %d, h_blank %d\n", > + meter.h_valid, meter.h_blank); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\tv_valid %d, v_blank %d\n", > + meter.v_valid, meter.v_blank); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\tmipi_pixel_rate %lld\n", > + meter.mipi_pixel_rate); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\tv_blank %lld us\n", > + meter.vb_in_us); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\th_blank %lld us\n", > + meter.hb_in_us); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "\tline_time %lld us\n", > + meter.line_time_in_us); > + } > + } > + } > + > + mutex_unlock(&core->mutex); > + > + return len; > +} > + > +#define SENINF_DRV_DEBUG_MAX_DELAY 400 > + > +static inline void > +mtk_cam_seninf_clear_matched_cam_mux_irq(struct seninf_ctx *ctx, > + u32 cam_mux_idx, Why is this a u32 param?.... > + u32 vc_idx, > + s32 enabled) > +{ > + u8 used_cammux; ...if this is a u8 local variable? (cam_mux_idx should be u8 instead - same for vc_idx) > + > + if (cam_mux_idx >= SENINF_CAM_MUX_NUM) { > + dev_info(ctx->dev, "unsupport cam_mux(%u)", cam_mux_idx); > + return; > + } > + if (vc_idx >= SENINF_VC_MAXCNT) { > + dev_info(ctx->dev, "unsupport vc_idx(%u)", vc_idx); > + return; > + } > + > + used_cammux = ctx->vcinfo.vc[vc_idx].cam; > + if (used_cammux == cam_mux_idx && > + enabled & (1 << cam_mux_idx)) { > + dev_dbg(ctx->dev, > + "before clear cam mux%u recSize = 0x%x, irq = 0x%x", > + cam_mux_idx, > + SENINF_READ_REG(ctx->reg_if_cam_mux, > + SENINF_CAM_MUX0_CHK_RES + (0x10 * cam_mux_idx)), > + SENINF_READ_REG(ctx->reg_if_cam_mux, > + SENINF_CAM_MUX_IRQ_STATUS)); > + > + SENINF_WRITE_REG(ctx->reg_if_cam_mux, > + SENINF_CAM_MUX_IRQ_STATUS, > + 3 << (cam_mux_idx * 2)); > + } > +} > + > +static inline void mtk_cam_seninf_check_matched_cam_mux(struct seninf_ctx *ctx, > + u32 cam_mux_idx, > + u32 vc_idx, > + s32 enabled, > + s32 irq_status) > +{ > + u8 used_cammux; > + > + if (cam_mux_idx >= SENINF_CAM_MUX_NUM) { > + dev_info(ctx->dev, "unsupport cam_mux(%u)", cam_mux_idx); > + return; > + } > + if (vc_idx >= SENINF_VC_MAXCNT) { > + dev_info(ctx->dev, "unsupport vc_idx(%u)", vc_idx); > + return; > + } > + > + used_cammux = ctx->vcinfo.vc[vc_idx].cam; > + > + if (used_cammux == cam_mux_idx && enabled & (1 << cam_mux_idx)) { > + int rec_size = SENINF_READ_REG(ctx->reg_if_cam_mux, > + SENINF_CAM_MUX0_CHK_RES + (0x10 * cam_mux_idx)); > + int exp_size = SENINF_READ_REG(ctx->reg_if_cam_mux, > + SENINF_CAM_MUX0_CHK_CTL_1 + (0x10 * cam_mux_idx)); > + if (rec_size != exp_size) { > + dev_dbg(ctx->dev, > + "cam mux%u size mismatch, (rec, exp) = (0x%x, 0x%x)", > + cam_mux_idx, rec_size, exp_size); > + } > + if ((irq_status & > + (3 << (cam_mux_idx * 2))) != 0) { > + dev_dbg(ctx->dev, > + "cam mux%u size mismatch!, irq = 0x%x", > + cam_mux_idx, irq_status); > + } > + } > +} > + ..snip.. > + > +#define SBUF 256 > +int mtk_cam_seninf_irq_handler(int irq, void *data) > +{ > + struct seninf_core *core = (struct seninf_core *)data; > + unsigned long flags; /* for mipi err detection */ > + struct seninf_ctx *ctx; > + struct seninf_vc *vc; > + void __iomem *csi2, *pmux, *seninf_cam_mux; > + int i; > + unsigned int csi_irq_ro; > + unsigned int mux_irq_ro; > + unsigned int cam_irq_exp_ro; > + unsigned int cam_irq_res_ro; > + char seninf_log[SBUF]; > + unsigned int wcnt = 0; > + > + spin_lock_irqsave(&core->spinlock_irq, flags); > + > + /* debug for set_reg case: REG_KEY_CSI_IRQ_EN */ > + if (core->csi_irq_en_flag) { > + list_for_each_entry(ctx, &core->list, list) { > + csi2 = ctx->reg_if_csi2[ctx->seninf_idx]; > + csi_irq_ro = > + SENINF_READ_REG(csi2, SENINF_CSI2_IRQ_STATUS); > + > + if (csi_irq_ro) { > + SENINF_WRITE_REG(csi2, SENINF_CSI2_IRQ_STATUS, > + 0xFFFFFFFF); > + } > + > + if (csi_irq_ro & (0x1 << RO_CSI2_ECC_ERR_CORRECTED_IRQ_SHIFT)) > + ctx->ecc_err_corrected_cnt++; > + if (csi_irq_ro & (0x1 << RO_CSI2_ECC_ERR_DOUBLE_IRQ_SHIFT)) > + ctx->ecc_err_double_cnt++; > + if (csi_irq_ro & (0x1 << RO_CSI2_CRC_ERR_IRQ_SHIFT)) > + ctx->crc_err_cnt++; > + if (csi_irq_ro & (0x1 << RO_CSI2_ERR_LANE_RESYNC_IRQ_SHIFT)) > + ctx->err_lane_resync_cnt++; > + if (csi_irq_ro & (0x1 << RO_CSI2_RECEIVE_DATA_NOT_ENOUGH_IRQ_SHIFT)) > + ctx->data_not_enough_cnt++; > + > + for (i = 0; i < ctx->vcinfo.cnt; i++) { > + vc = &ctx->vcinfo.vc[i]; > + pmux = ctx->reg_if_mux[vc->mux]; > + seninf_cam_mux = ctx->reg_if_cam_mux; > + > + mux_irq_ro = SENINF_READ_REG(pmux, > + SENINF_MUX_IRQ_STATUS); > + > + cam_irq_exp_ro = SENINF_READ_REG(seninf_cam_mux, > + SENINF_CAM_MUX0_CHK_CTL_1 + > + (0x10 * (vc->cam))); > + > + cam_irq_res_ro = SENINF_READ_REG(seninf_cam_mux, > + SENINF_CAM_MUX0_CHK_RES + > + (0x10 * (vc->cam))); > + > + if (mux_irq_ro) > + SENINF_WRITE_REG(pmux, > + SENINF_MUX_IRQ_STATUS, > + 0xFFFFFFFF); > + > + if (cam_irq_res_ro != cam_irq_exp_ro) > + SENINF_WRITE_REG(seninf_cam_mux, > + SENINF_CAM_MUX0_CHK_RES + > + (0x10 * (vc->cam)), > + 0xFFFFFFFF); > + > + if (mux_irq_ro & (0x1 << 0)) > + ctx->fifo_overrun_cnt++; > + > + if (cam_irq_res_ro != cam_irq_exp_ro) > + ctx->size_err_cnt++; > + } > + > + /* dump status counter: debug for electrical signal */ > + if (ctx->data_not_enough_cnt >= core->detection_cnt || > + ctx->err_lane_resync_cnt >= core->detection_cnt || > + ctx->crc_err_cnt >= core->detection_cnt || > + ctx->ecc_err_double_cnt >= core->detection_cnt || > + ctx->ecc_err_corrected_cnt >= core->detection_cnt || > + ctx->fifo_overrun_cnt >= core->detection_cnt || > + ctx->size_err_cnt >= core->detection_cnt) { > + /* disable all interrupts */ > + SENINF_WRITE_REG(csi2, SENINF_CSI2_IRQ_EN, 0x80000000); > + > + if (ctx->data_not_enough_cnt >= core->detection_cnt) > + ctx->data_not_enough_flag = 1; > + if (ctx->err_lane_resync_cnt >= core->detection_cnt) > + ctx->err_lane_resync_flag = 1; > + if (ctx->crc_err_cnt >= core->detection_cnt) > + ctx->crc_err_flag = 1; > + if (ctx->ecc_err_double_cnt >= core->detection_cnt) > + ctx->ecc_err_double_flag = 1; > + if (ctx->ecc_err_corrected_cnt >= core->detection_cnt) > + ctx->ecc_err_corrected_flag = 1; > + if (ctx->fifo_overrun_cnt >= core->detection_cnt) > + ctx->fifo_overrun_flag = 1; > + if (ctx->size_err_cnt >= core->detection_cnt) > + ctx->size_err_flag = 1; > + > + wcnt = snprintf(seninf_log, SBUF, "info: %s", __func__); You're not even printing seninf_log, so either print it (no, please don't) or just drop it. > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " data_not_enough_count: %d", > + ctx->data_not_enough_cnt); > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " err_lane_resync_count: %d", > + ctx->err_lane_resync_cnt); > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " crc_err_count: %d", > + ctx->crc_err_cnt); > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " ecc_err_double_count: %d", > + ctx->ecc_err_double_cnt); > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " ecc_err_corrected_count: %d", > + ctx->ecc_err_corrected_cnt); > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " fifo_overrun_count: %d", > + ctx->fifo_overrun_cnt); > + wcnt += snprintf(seninf_log + wcnt, SBUF - wcnt, > + " size_err_count: %d", > + ctx->size_err_cnt); > + } > + } > + } > + > + spin_unlock_irqrestore(&core->spinlock_irq, flags); > + > + return 0; > +} > + > +int mtk_cam_seninf_set_sw_cfg_busy(struct seninf_ctx *ctx, bool enable, > + int index) > +{ > + void __iomem *seninf_cam_mux = ctx->reg_if_cam_mux; mask = index ? RG_SENINF_CAM_MUX_DYN_SWITCH_BSY1 : RG_SENINF_CAM_MUX_DYN_SWITCH_BSY0 > + > + if (index == 0) > + SENINF_BITS(seninf_cam_mux, SENINF_CAM_MUX_DYN_CTRL, > + RG_SENINF_CAM_MUX_DYN_SWITCH_BSY0, enable); > + else > + SENINF_BITS(seninf_cam_mux, SENINF_CAM_MUX_DYN_CTRL, > + RG_SENINF_CAM_MUX_DYN_SWITCH_BSY1, enable); > + return 0; > +} > + > +int mtk_cam_seninf_set_cam_mux_dyn_en(struct seninf_ctx *ctx, bool enable, > + int cam_mux, int index) > +{ > + void __iomem *seninf_cam_mux = ctx->reg_if_cam_mux; > + u32 tmp = 0; > + > + if (index == 0) { > + tmp = SENINF_READ_BITS(seninf_cam_mux, SENINF_CAM_MUX_DYN_EN, > + RG_SENINF_CAM_MUX_DYN_SWITCH_EN0); regmap_set_bits() will definitely help here. > + if (enable) > + tmp = tmp | (1 << cam_mux); > + else > + tmp = tmp & ~(1 << cam_mux); > + > + SENINF_BITS(seninf_cam_mux, SENINF_CAM_MUX_DYN_EN, > + RG_SENINF_CAM_MUX_DYN_SWITCH_EN0, tmp); > + } else { > + tmp = SENINF_READ_BITS(seninf_cam_mux, SENINF_CAM_MUX_DYN_EN, > + RG_SENINF_CAM_MUX_DYN_SWITCH_EN1); > + if (enable) > + tmp = tmp | (1 << cam_mux); > + else > + tmp = tmp & ~(1 << cam_mux); > + > + SENINF_BITS(seninf_cam_mux, SENINF_CAM_MUX_DYN_EN, > + RG_SENINF_CAM_MUX_DYN_SWITCH_EN1, tmp); > + } > + > + return 0; > +} > + There is surely more to say on this driver, and it's far from being near to upstream quality. Please start with addressing these comments on the entire series, then we can go on with further reviews. Regards, Angelo

1 year, 1 month

[PATCH AUTOSEL 6.11 72/76] drm/amdgpu: nuke the VM PD/PT shadow handling

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit 7181faaa4703705939580abffaf9cb5d6b50dbb7 ] This was only used as workaround for recovering the page tables after VRAM was lost and is no longer necessary after the function amdgpu_vm_bo_reset_state_machine() started to do the same. Compute never used shadows either, so the only proplematic case left is SVM and that is most likely not recoverable in any way when VRAM is lost. Signed-off-by: Christian König <christian.koenig(a)amd.com> Acked-by: Lijo Lazar <lijo.lazar(a)amd.com> Signed-off-by: Alex Deucher <alexander.deucher(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 - drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 87 +-------------------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 67 +--------------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 21 ----- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ---- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 56 +------------ drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 19 +---- 7 files changed, 6 insertions(+), 265 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 137a88b8de453..a1b2bf3db55b8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1091,10 +1091,6 @@ struct amdgpu_device { struct amdgpu_virt virt; - /* link all shadow bo */ - struct list_head shadow_list; - struct mutex shadow_list_lock; - /* record hw reset is performed */ bool has_hw_reset; u8 reset_magic[AMDGPU_RESET_MAGIC_NUM]; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index bcacf2e35eba0..dfd468729d52b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4073,9 +4073,6 @@ int amdgpu_device_init(struct amdgpu_device *adev, spin_lock_init(&adev->mm_stats.lock); spin_lock_init(&adev->wb.lock); - INIT_LIST_HEAD(&adev->shadow_list); - mutex_init(&adev->shadow_list_lock); - INIT_LIST_HEAD(&adev->reset_list); INIT_LIST_HEAD(&adev->ras_list); @@ -4980,80 +4977,6 @@ static int amdgpu_device_ip_post_soft_reset(struct amdgpu_device *adev) return 0; } -/** - * amdgpu_device_recover_vram - Recover some VRAM contents - * - * @adev: amdgpu_device pointer - * - * Restores the contents of VRAM buffers from the shadows in GTT. Used to - * restore things like GPUVM page tables after a GPU reset where - * the contents of VRAM might be lost. - * - * Returns: - * 0 on success, negative error code on failure. - */ -static int amdgpu_device_recover_vram(struct amdgpu_device *adev) -{ - struct dma_fence *fence = NULL, *next = NULL; - struct amdgpu_bo *shadow; - struct amdgpu_bo_vm *vmbo; - long r = 1, tmo; - - if (amdgpu_sriov_runtime(adev)) - tmo = msecs_to_jiffies(8000); - else - tmo = msecs_to_jiffies(100); - - dev_info(adev->dev, "recover vram bo from shadow start\n"); - mutex_lock(&adev->shadow_list_lock); - list_for_each_entry(vmbo, &adev->shadow_list, shadow_list) { - /* If vm is compute context or adev is APU, shadow will be NULL */ - if (!vmbo->shadow) - continue; - shadow = vmbo->shadow; - - /* No need to recover an evicted BO */ - if (!shadow->tbo.resource || - shadow->tbo.resource->mem_type != TTM_PL_TT || - shadow->tbo.resource->start == AMDGPU_BO_INVALID_OFFSET || - shadow->parent->tbo.resource->mem_type != TTM_PL_VRAM) - continue; - - r = amdgpu_bo_restore_shadow(shadow, &next); - if (r) - break; - - if (fence) { - tmo = dma_fence_wait_timeout(fence, false, tmo); - dma_fence_put(fence); - fence = next; - if (tmo == 0) { - r = -ETIMEDOUT; - break; - } else if (tmo < 0) { - r = tmo; - break; - } - } else { - fence = next; - } - } - mutex_unlock(&adev->shadow_list_lock); - - if (fence) - tmo = dma_fence_wait_timeout(fence, false, tmo); - dma_fence_put(fence); - - if (r < 0 || tmo <= 0) { - dev_err(adev->dev, "recover vram bo from shadow failed, r is %ld, tmo is %ld\n", r, tmo); - return -EIO; - } - - dev_info(adev->dev, "recover vram bo from shadow done\n"); - return 0; -} - - /** * amdgpu_device_reset_sriov - reset ASIC for SR-IOV vf * @@ -5116,12 +5039,8 @@ static int amdgpu_device_reset_sriov(struct amdgpu_device *adev, if (r) return r; - if (adev->virt.gim_feature & AMDGIM_FEATURE_GIM_FLR_VRAMLOST) { + if (adev->virt.gim_feature & AMDGIM_FEATURE_GIM_FLR_VRAMLOST) amdgpu_inc_vram_lost(adev); - r = amdgpu_device_recover_vram(adev); - } - if (r) - return r; /* need to be called during full access so we can't do it later like * bare-metal does. @@ -5541,9 +5460,7 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, } } - if (!r) - r = amdgpu_device_recover_vram(tmp_adev); - else + if (r) tmp_adev->asic_reset_res = r; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index e32161f6b67a3..a987f671b1d53 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -77,24 +77,6 @@ static void amdgpu_bo_user_destroy(struct ttm_buffer_object *tbo) amdgpu_bo_destroy(tbo); } -static void amdgpu_bo_vm_destroy(struct ttm_buffer_object *tbo) -{ - struct amdgpu_device *adev = amdgpu_ttm_adev(tbo->bdev); - struct amdgpu_bo *shadow_bo = ttm_to_amdgpu_bo(tbo), *bo; - struct amdgpu_bo_vm *vmbo; - - bo = shadow_bo->parent; - vmbo = to_amdgpu_bo_vm(bo); - /* in case amdgpu_device_recover_vram got NULL of bo->parent */ - if (!list_empty(&vmbo->shadow_list)) { - mutex_lock(&adev->shadow_list_lock); - list_del_init(&vmbo->shadow_list); - mutex_unlock(&adev->shadow_list_lock); - } - - amdgpu_bo_destroy(tbo); -} - /** * amdgpu_bo_is_amdgpu_bo - check if the buffer object is an &amdgpu_bo * @bo: buffer object to be checked @@ -108,8 +90,7 @@ static void amdgpu_bo_vm_destroy(struct ttm_buffer_object *tbo) bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo) { if (bo->destroy == &amdgpu_bo_destroy || - bo->destroy == &amdgpu_bo_user_destroy || - bo->destroy == &amdgpu_bo_vm_destroy) + bo->destroy == &amdgpu_bo_user_destroy) return true; return false; @@ -722,52 +703,6 @@ int amdgpu_bo_create_vm(struct amdgpu_device *adev, return r; } -/** - * amdgpu_bo_add_to_shadow_list - add a BO to the shadow list - * - * @vmbo: BO that will be inserted into the shadow list - * - * Insert a BO to the shadow list. - */ -void amdgpu_bo_add_to_shadow_list(struct amdgpu_bo_vm *vmbo) -{ - struct amdgpu_device *adev = amdgpu_ttm_adev(vmbo->bo.tbo.bdev); - - mutex_lock(&adev->shadow_list_lock); - list_add_tail(&vmbo->shadow_list, &adev->shadow_list); - vmbo->shadow->parent = amdgpu_bo_ref(&vmbo->bo); - vmbo->shadow->tbo.destroy = &amdgpu_bo_vm_destroy; - mutex_unlock(&adev->shadow_list_lock); -} - -/** - * amdgpu_bo_restore_shadow - restore an &amdgpu_bo shadow - * - * @shadow: &amdgpu_bo shadow to be restored - * @fence: dma_fence associated with the operation - * - * Copies a buffer object's shadow content back to the object. - * This is used for recovering a buffer from its shadow in case of a gpu - * reset where vram context may be lost. - * - * Returns: - * 0 for success or a negative error code on failure. - */ -int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow, struct dma_fence **fence) - -{ - struct amdgpu_device *adev = amdgpu_ttm_adev(shadow->tbo.bdev); - struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring; - uint64_t shadow_addr, parent_addr; - - shadow_addr = amdgpu_bo_gpu_offset(shadow); - parent_addr = amdgpu_bo_gpu_offset(shadow->parent); - - return amdgpu_copy_buffer(ring, shadow_addr, parent_addr, - amdgpu_bo_size(shadow), NULL, fence, - true, false, 0); -} - /** * amdgpu_bo_kmap - map an &amdgpu_bo buffer object * @bo: &amdgpu_bo buffer object to be mapped diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index bc42ccbde659a..a4fa1f296daec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -130,8 +130,6 @@ struct amdgpu_bo_user { struct amdgpu_bo_vm { struct amdgpu_bo bo; - struct amdgpu_bo *shadow; - struct list_head shadow_list; struct amdgpu_vm_bo_base entries[]; }; @@ -269,22 +267,6 @@ static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) return bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED; } -/** - * amdgpu_bo_shadowed - check if the BO is shadowed - * - * @bo: BO to be tested. - * - * Returns: - * NULL if not shadowed or else return a BO pointer. - */ -static inline struct amdgpu_bo *amdgpu_bo_shadowed(struct amdgpu_bo *bo) -{ - if (bo->tbo.type == ttm_bo_type_kernel) - return to_amdgpu_bo_vm(bo)->shadow; - - return NULL; -} - bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo); void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain); @@ -343,9 +325,6 @@ u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo); u64 amdgpu_bo_gpu_offset_no_check(struct amdgpu_bo *bo); void amdgpu_bo_get_memory(struct amdgpu_bo *bo, struct amdgpu_mem_stats *stats); -void amdgpu_bo_add_to_shadow_list(struct amdgpu_bo_vm *vmbo); -int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow, - struct dma_fence **fence); uint32_t amdgpu_bo_get_preferred_domain(struct amdgpu_device *adev, uint32_t domain); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index a060c28f0877c..8cda1d02dade3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -465,7 +465,6 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm, { uint64_t new_vm_generation = amdgpu_vm_generation(adev, vm); struct amdgpu_vm_bo_base *bo_base; - struct amdgpu_bo *shadow; struct amdgpu_bo *bo; int r; @@ -486,16 +485,10 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm, spin_unlock(&vm->status_lock); bo = bo_base->bo; - shadow = amdgpu_bo_shadowed(bo); r = validate(param, bo); if (r) return r; - if (shadow) { - r = validate(param, shadow); - if (r) - return r; - } if (bo->tbo.type != ttm_bo_type_kernel) { amdgpu_vm_bo_moved(bo_base); @@ -2123,10 +2116,6 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev, { struct amdgpu_vm_bo_base *bo_base; - /* shadow bo doesn't have bo base, its validation needs its parent */ - if (bo->parent && (amdgpu_bo_shadowed(bo->parent) == bo)) - bo = bo->parent; - for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { struct amdgpu_vm *vm = bo_base->vm; @@ -2454,7 +2443,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, root_bo = amdgpu_bo_ref(&root->bo); r = amdgpu_bo_reserve(root_bo, true); if (r) { - amdgpu_bo_unref(&root->shadow); amdgpu_bo_unref(&root_bo); goto error_free_delayed; } @@ -2546,11 +2534,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm) vm->last_update = dma_fence_get_stub(); vm->is_compute_context = true; - /* Free the shadow bo for compute VM */ - amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow); - - goto unreserve_bo; - unreserve_bo: amdgpu_bo_unreserve(vm->root.bo); return r; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c index e39d6e7643bfb..c8e0b8cfd3363 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c @@ -383,14 +383,6 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (r) return r; - if (vmbo->shadow) { - struct amdgpu_bo *shadow = vmbo->shadow; - - r = ttm_bo_validate(&shadow->tbo, &shadow->placement, &ctx); - if (r) - return r; - } - if (!drm_dev_enter(adev_to_drm(adev), &idx)) return -ENODEV; @@ -448,10 +440,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id) { struct amdgpu_bo_param bp; - struct amdgpu_bo *bo; - struct dma_resv *resv; unsigned int num_entries; - int r; memset(&bp, 0, sizeof(bp)); @@ -484,42 +473,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (vm->root.bo) bp.resv = vm->root.bo->tbo.base.resv; - r = amdgpu_bo_create_vm(adev, &bp, vmbo); - if (r) - return r; - - bo = &(*vmbo)->bo; - if (vm->is_compute_context || (adev->flags & AMD_IS_APU)) { - (*vmbo)->shadow = NULL; - return 0; - } - - if (!bp.resv) - WARN_ON(dma_resv_lock(bo->tbo.base.resv, - NULL)); - resv = bp.resv; - memset(&bp, 0, sizeof(bp)); - bp.size = amdgpu_vm_pt_size(adev, level); - bp.domain = AMDGPU_GEM_DOMAIN_GTT; - bp.flags = AMDGPU_GEM_CREATE_CPU_GTT_USWC; - bp.type = ttm_bo_type_kernel; - bp.resv = bo->tbo.base.resv; - bp.bo_ptr_size = sizeof(struct amdgpu_bo); - bp.xcp_id_plus1 = xcp_id + 1; - - r = amdgpu_bo_create(adev, &bp, &(*vmbo)->shadow); - - if (!resv) - dma_resv_unlock(bo->tbo.base.resv); - - if (r) { - amdgpu_bo_unref(&bo); - return r; - } - - amdgpu_bo_add_to_shadow_list(*vmbo); - - return 0; + return amdgpu_bo_create_vm(adev, &bp, vmbo); } /** @@ -569,7 +523,6 @@ static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev, return 0; error_free_pt: - amdgpu_bo_unref(&pt->shadow); amdgpu_bo_unref(&pt_bo); return r; } @@ -581,17 +534,10 @@ static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev, */ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry) { - struct amdgpu_bo *shadow; - if (!entry->bo) return; entry->bo->vm_bo = NULL; - shadow = amdgpu_bo_shadowed(entry->bo); - if (shadow) { - ttm_bo_set_bulk_move(&shadow->tbo, NULL); - amdgpu_bo_unref(&shadow); - } ttm_bo_set_bulk_move(&entry->bo->tbo, NULL); spin_lock(&entry->vm->status_lock); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index 9b748d7058b5c..390432a22ddd5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c @@ -35,16 +35,7 @@ */ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo_vm *table) { - int r; - - r = amdgpu_ttm_alloc_gart(&table->bo.tbo); - if (r) - return r; - - if (table->shadow) - r = amdgpu_ttm_alloc_gart(&table->shadow->tbo); - - return r; + return amdgpu_ttm_alloc_gart(&table->bo.tbo); } /* Allocate a new job for @count PTE updates */ @@ -273,17 +264,13 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p, if (!p->pages_addr) { /* set page commands needed */ - if (vmbo->shadow) - amdgpu_vm_sdma_set_ptes(p, vmbo->shadow, pe, addr, - count, incr, flags); amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count, incr, flags); return 0; } /* copy commands needed */ - ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw * - (vmbo->shadow ? 2 : 1); + ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw; /* for padding */ ndw -= 7; @@ -298,8 +285,6 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p, pte[i] |= flags; } - if (vmbo->shadow) - amdgpu_vm_sdma_copy_ptes(p, vmbo->shadow, pe, nptes); amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes); pe += nptes * 8; -- 2.43.0

1 year, 1 month

[PATCH AUTOSEL 6.10 66/70] drm/amdgpu: nuke the VM PD/PT shadow handling

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit 7181faaa4703705939580abffaf9cb5d6b50dbb7 ] This was only used as workaround for recovering the page tables after VRAM was lost and is no longer necessary after the function amdgpu_vm_bo_reset_state_machine() started to do the same. Compute never used shadows either, so the only proplematic case left is SVM and that is most likely not recoverable in any way when VRAM is lost. Signed-off-by: Christian König <christian.koenig(a)amd.com> Acked-by: Lijo Lazar <lijo.lazar(a)amd.com> Signed-off-by: Alex Deucher <alexander.deucher(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 - drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 87 +-------------------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 67 +--------------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 21 ----- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ---- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 56 +------------ drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 19 +---- 7 files changed, 6 insertions(+), 265 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index f87d53e183c3d..b48e8066c01cc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1078,10 +1078,6 @@ struct amdgpu_device { struct amdgpu_virt virt; - /* link all shadow bo */ - struct list_head shadow_list; - struct mutex shadow_list_lock; - /* record hw reset is performed */ bool has_hw_reset; u8 reset_magic[AMDGPU_RESET_MAGIC_NUM]; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index e66546df0bc19..1df57a02a7598 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4072,9 +4072,6 @@ int amdgpu_device_init(struct amdgpu_device *adev, spin_lock_init(&adev->mm_stats.lock); spin_lock_init(&adev->wb.lock); - INIT_LIST_HEAD(&adev->shadow_list); - mutex_init(&adev->shadow_list_lock); - INIT_LIST_HEAD(&adev->reset_list); INIT_LIST_HEAD(&adev->ras_list); @@ -4979,80 +4976,6 @@ static int amdgpu_device_ip_post_soft_reset(struct amdgpu_device *adev) return 0; } -/** - * amdgpu_device_recover_vram - Recover some VRAM contents - * - * @adev: amdgpu_device pointer - * - * Restores the contents of VRAM buffers from the shadows in GTT. Used to - * restore things like GPUVM page tables after a GPU reset where - * the contents of VRAM might be lost. - * - * Returns: - * 0 on success, negative error code on failure. - */ -static int amdgpu_device_recover_vram(struct amdgpu_device *adev) -{ - struct dma_fence *fence = NULL, *next = NULL; - struct amdgpu_bo *shadow; - struct amdgpu_bo_vm *vmbo; - long r = 1, tmo; - - if (amdgpu_sriov_runtime(adev)) - tmo = msecs_to_jiffies(8000); - else - tmo = msecs_to_jiffies(100); - - dev_info(adev->dev, "recover vram bo from shadow start\n"); - mutex_lock(&adev->shadow_list_lock); - list_for_each_entry(vmbo, &adev->shadow_list, shadow_list) { - /* If vm is compute context or adev is APU, shadow will be NULL */ - if (!vmbo->shadow) - continue; - shadow = vmbo->shadow; - - /* No need to recover an evicted BO */ - if (!shadow->tbo.resource || - shadow->tbo.resource->mem_type != TTM_PL_TT || - shadow->tbo.resource->start == AMDGPU_BO_INVALID_OFFSET || - shadow->parent->tbo.resource->mem_type != TTM_PL_VRAM) - continue; - - r = amdgpu_bo_restore_shadow(shadow, &next); - if (r) - break; - - if (fence) { - tmo = dma_fence_wait_timeout(fence, false, tmo); - dma_fence_put(fence); - fence = next; - if (tmo == 0) { - r = -ETIMEDOUT; - break; - } else if (tmo < 0) { - r = tmo; - break; - } - } else { - fence = next; - } - } - mutex_unlock(&adev->shadow_list_lock); - - if (fence) - tmo = dma_fence_wait_timeout(fence, false, tmo); - dma_fence_put(fence); - - if (r < 0 || tmo <= 0) { - dev_err(adev->dev, "recover vram bo from shadow failed, r is %ld, tmo is %ld\n", r, tmo); - return -EIO; - } - - dev_info(adev->dev, "recover vram bo from shadow done\n"); - return 0; -} - - /** * amdgpu_device_reset_sriov - reset ASIC for SR-IOV vf * @@ -5112,12 +5035,8 @@ static int amdgpu_device_reset_sriov(struct amdgpu_device *adev, if (r) return r; - if (adev->virt.gim_feature & AMDGIM_FEATURE_GIM_FLR_VRAMLOST) { + if (adev->virt.gim_feature & AMDGIM_FEATURE_GIM_FLR_VRAMLOST) amdgpu_inc_vram_lost(adev); - r = amdgpu_device_recover_vram(adev); - } - if (r) - return r; /* need to be called during full access so we can't do it later like * bare-metal does. @@ -5529,9 +5448,7 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, } } - if (!r) - r = amdgpu_device_recover_vram(tmp_adev); - else + if (r) tmp_adev->asic_reset_res = r; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index c556c8b653fa4..5f3b2c157824e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -77,24 +77,6 @@ static void amdgpu_bo_user_destroy(struct ttm_buffer_object *tbo) amdgpu_bo_destroy(tbo); } -static void amdgpu_bo_vm_destroy(struct ttm_buffer_object *tbo) -{ - struct amdgpu_device *adev = amdgpu_ttm_adev(tbo->bdev); - struct amdgpu_bo *shadow_bo = ttm_to_amdgpu_bo(tbo), *bo; - struct amdgpu_bo_vm *vmbo; - - bo = shadow_bo->parent; - vmbo = to_amdgpu_bo_vm(bo); - /* in case amdgpu_device_recover_vram got NULL of bo->parent */ - if (!list_empty(&vmbo->shadow_list)) { - mutex_lock(&adev->shadow_list_lock); - list_del_init(&vmbo->shadow_list); - mutex_unlock(&adev->shadow_list_lock); - } - - amdgpu_bo_destroy(tbo); -} - /** * amdgpu_bo_is_amdgpu_bo - check if the buffer object is an &amdgpu_bo * @bo: buffer object to be checked @@ -108,8 +90,7 @@ static void amdgpu_bo_vm_destroy(struct ttm_buffer_object *tbo) bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo) { if (bo->destroy == &amdgpu_bo_destroy || - bo->destroy == &amdgpu_bo_user_destroy || - bo->destroy == &amdgpu_bo_vm_destroy) + bo->destroy == &amdgpu_bo_user_destroy) return true; return false; @@ -722,52 +703,6 @@ int amdgpu_bo_create_vm(struct amdgpu_device *adev, return r; } -/** - * amdgpu_bo_add_to_shadow_list - add a BO to the shadow list - * - * @vmbo: BO that will be inserted into the shadow list - * - * Insert a BO to the shadow list. - */ -void amdgpu_bo_add_to_shadow_list(struct amdgpu_bo_vm *vmbo) -{ - struct amdgpu_device *adev = amdgpu_ttm_adev(vmbo->bo.tbo.bdev); - - mutex_lock(&adev->shadow_list_lock); - list_add_tail(&vmbo->shadow_list, &adev->shadow_list); - vmbo->shadow->parent = amdgpu_bo_ref(&vmbo->bo); - vmbo->shadow->tbo.destroy = &amdgpu_bo_vm_destroy; - mutex_unlock(&adev->shadow_list_lock); -} - -/** - * amdgpu_bo_restore_shadow - restore an &amdgpu_bo shadow - * - * @shadow: &amdgpu_bo shadow to be restored - * @fence: dma_fence associated with the operation - * - * Copies a buffer object's shadow content back to the object. - * This is used for recovering a buffer from its shadow in case of a gpu - * reset where vram context may be lost. - * - * Returns: - * 0 for success or a negative error code on failure. - */ -int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow, struct dma_fence **fence) - -{ - struct amdgpu_device *adev = amdgpu_ttm_adev(shadow->tbo.bdev); - struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring; - uint64_t shadow_addr, parent_addr; - - shadow_addr = amdgpu_bo_gpu_offset(shadow); - parent_addr = amdgpu_bo_gpu_offset(shadow->parent); - - return amdgpu_copy_buffer(ring, shadow_addr, parent_addr, - amdgpu_bo_size(shadow), NULL, fence, - true, false, 0); -} - /** * amdgpu_bo_kmap - map an &amdgpu_bo buffer object * @bo: &amdgpu_bo buffer object to be mapped diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index bc42ccbde659a..a4fa1f296daec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -130,8 +130,6 @@ struct amdgpu_bo_user { struct amdgpu_bo_vm { struct amdgpu_bo bo; - struct amdgpu_bo *shadow; - struct list_head shadow_list; struct amdgpu_vm_bo_base entries[]; }; @@ -269,22 +267,6 @@ static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) return bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED; } -/** - * amdgpu_bo_shadowed - check if the BO is shadowed - * - * @bo: BO to be tested. - * - * Returns: - * NULL if not shadowed or else return a BO pointer. - */ -static inline struct amdgpu_bo *amdgpu_bo_shadowed(struct amdgpu_bo *bo) -{ - if (bo->tbo.type == ttm_bo_type_kernel) - return to_amdgpu_bo_vm(bo)->shadow; - - return NULL; -} - bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo); void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain); @@ -343,9 +325,6 @@ u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo); u64 amdgpu_bo_gpu_offset_no_check(struct amdgpu_bo *bo); void amdgpu_bo_get_memory(struct amdgpu_bo *bo, struct amdgpu_mem_stats *stats); -void amdgpu_bo_add_to_shadow_list(struct amdgpu_bo_vm *vmbo); -int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow, - struct dma_fence **fence); uint32_t amdgpu_bo_get_preferred_domain(struct amdgpu_device *adev, uint32_t domain); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 0f7106066480e..4820277e3c550 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -465,7 +465,6 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm, { uint64_t new_vm_generation = amdgpu_vm_generation(adev, vm); struct amdgpu_vm_bo_base *bo_base; - struct amdgpu_bo *shadow; struct amdgpu_bo *bo; int r; @@ -486,16 +485,10 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm, spin_unlock(&vm->status_lock); bo = bo_base->bo; - shadow = amdgpu_bo_shadowed(bo); r = validate(param, bo); if (r) return r; - if (shadow) { - r = validate(param, shadow); - if (r) - return r; - } if (bo->tbo.type != ttm_bo_type_kernel) { amdgpu_vm_bo_moved(bo_base); @@ -2125,10 +2118,6 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev, { struct amdgpu_vm_bo_base *bo_base; - /* shadow bo doesn't have bo base, its validation needs its parent */ - if (bo->parent && (amdgpu_bo_shadowed(bo->parent) == bo)) - bo = bo->parent; - for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { struct amdgpu_vm *vm = bo_base->vm; @@ -2456,7 +2445,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, root_bo = amdgpu_bo_ref(&root->bo); r = amdgpu_bo_reserve(root_bo, true); if (r) { - amdgpu_bo_unref(&root->shadow); amdgpu_bo_unref(&root_bo); goto error_free_delayed; } @@ -2548,11 +2536,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm) vm->last_update = dma_fence_get_stub(); vm->is_compute_context = true; - /* Free the shadow bo for compute VM */ - amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow); - - goto unreserve_bo; - unreserve_bo: amdgpu_bo_unreserve(vm->root.bo); return r; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c index f07647a9a9d97..ad3a9e594a406 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c @@ -383,14 +383,6 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (r) return r; - if (vmbo->shadow) { - struct amdgpu_bo *shadow = vmbo->shadow; - - r = ttm_bo_validate(&shadow->tbo, &shadow->placement, &ctx); - if (r) - return r; - } - if (!drm_dev_enter(adev_to_drm(adev), &idx)) return -ENODEV; @@ -448,10 +440,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id) { struct amdgpu_bo_param bp; - struct amdgpu_bo *bo; - struct dma_resv *resv; unsigned int num_entries; - int r; memset(&bp, 0, sizeof(bp)); @@ -484,42 +473,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (vm->root.bo) bp.resv = vm->root.bo->tbo.base.resv; - r = amdgpu_bo_create_vm(adev, &bp, vmbo); - if (r) - return r; - - bo = &(*vmbo)->bo; - if (vm->is_compute_context || (adev->flags & AMD_IS_APU)) { - (*vmbo)->shadow = NULL; - return 0; - } - - if (!bp.resv) - WARN_ON(dma_resv_lock(bo->tbo.base.resv, - NULL)); - resv = bp.resv; - memset(&bp, 0, sizeof(bp)); - bp.size = amdgpu_vm_pt_size(adev, level); - bp.domain = AMDGPU_GEM_DOMAIN_GTT; - bp.flags = AMDGPU_GEM_CREATE_CPU_GTT_USWC; - bp.type = ttm_bo_type_kernel; - bp.resv = bo->tbo.base.resv; - bp.bo_ptr_size = sizeof(struct amdgpu_bo); - bp.xcp_id_plus1 = xcp_id + 1; - - r = amdgpu_bo_create(adev, &bp, &(*vmbo)->shadow); - - if (!resv) - dma_resv_unlock(bo->tbo.base.resv); - - if (r) { - amdgpu_bo_unref(&bo); - return r; - } - - amdgpu_bo_add_to_shadow_list(*vmbo); - - return 0; + return amdgpu_bo_create_vm(adev, &bp, vmbo); } /** @@ -569,7 +523,6 @@ static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev, return 0; error_free_pt: - amdgpu_bo_unref(&pt->shadow); amdgpu_bo_unref(&pt_bo); return r; } @@ -581,17 +534,10 @@ static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev, */ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry) { - struct amdgpu_bo *shadow; - if (!entry->bo) return; entry->bo->vm_bo = NULL; - shadow = amdgpu_bo_shadowed(entry->bo); - if (shadow) { - ttm_bo_set_bulk_move(&shadow->tbo, NULL); - amdgpu_bo_unref(&shadow); - } ttm_bo_set_bulk_move(&entry->bo->tbo, NULL); spin_lock(&entry->vm->status_lock); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index 9b748d7058b5c..390432a22ddd5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c @@ -35,16 +35,7 @@ */ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo_vm *table) { - int r; - - r = amdgpu_ttm_alloc_gart(&table->bo.tbo); - if (r) - return r; - - if (table->shadow) - r = amdgpu_ttm_alloc_gart(&table->shadow->tbo); - - return r; + return amdgpu_ttm_alloc_gart(&table->bo.tbo); } /* Allocate a new job for @count PTE updates */ @@ -273,17 +264,13 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p, if (!p->pages_addr) { /* set page commands needed */ - if (vmbo->shadow) - amdgpu_vm_sdma_set_ptes(p, vmbo->shadow, pe, addr, - count, incr, flags); amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count, incr, flags); return 0; } /* copy commands needed */ - ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw * - (vmbo->shadow ? 2 : 1); + ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw; /* for padding */ ndw -= 7; @@ -298,8 +285,6 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p, pte[i] |= flags; } - if (vmbo->shadow) - amdgpu_vm_sdma_copy_ptes(p, vmbo->shadow, pe, nptes); amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes); pe += nptes * 8; -- 2.43.0

1 year, 1 month

Re: [PATCH 1/3] dma-buf: replace symbolic permission S_IRUGO with octal 0444

by Sumit Semwal

Hello Pintu, On Tue, 1 Oct 2024 at 23:16, Pintu Kumar <quic_pintu(a)quicinc.com> wrote: > > Symbolic permissions are not preferred, instead use the octal. > Also, fix other warnings/errors as well for cleanup. > > WARNING: Block comments use * on subsequent lines > + /* only support discovering the end of the buffer, > + but also allow SEEK_SET to maintain the idiomatic > > WARNING: Block comments use a trailing */ on a separate line > + SEEK_END(0), SEEK_CUR(0) pattern */ > > WARNING: Block comments use a trailing */ on a separate line > + * before passing the sgt back to the exporter. */ > > ERROR: "foo * bar" should be "foo *bar" > +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > + d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, > > total: 1 errors, 4 warnings, 1746 lines checked > > Signed-off-by: Pintu Kumar <quic_pintu(a)quicinc.com> Thanks for this patch - could you please also mention in the commit log how did you find this? It looks like you ran checkpatch, but it's not clear from the commit log. Since this patch does multiple things related to checkpatch warnings (change S_IRUGO to 0444, comments correction, function declaration correction), can I please ask you to change the commit title to also reflect that? > --- > drivers/dma-buf/dma-buf.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 8892bc701a66..2e63d50e46d3 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -176,8 +176,9 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) > dmabuf = file->private_data; > > /* only support discovering the end of the buffer, > - but also allow SEEK_SET to maintain the idiomatic > - SEEK_END(0), SEEK_CUR(0) pattern */ > + * but also allow SEEK_SET to maintain the idiomatic > + * SEEK_END(0), SEEK_CUR(0) pattern. > + */ > if (whence == SEEK_END) > base = dmabuf->size; > else if (whence == SEEK_SET) > @@ -782,13 +783,14 @@ static void mangle_sg_table(struct sg_table *sg_table) > /* To catch abuse of the underlying struct page by importers mix > * up the bits, but take care to preserve the low SG_ bits to > * not corrupt the sgt. The mixing is undone in __unmap_dma_buf > - * before passing the sgt back to the exporter. */ > + * before passing the sgt back to the exporter. > + */ > for_each_sgtable_sg(sg_table, sg, i) > sg->page_link ^= ~0xffUL; > #endif > > } > -static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, > +static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach, > enum dma_data_direction direction) > { > struct sg_table *sg_table; > @@ -1694,7 +1696,7 @@ static int dma_buf_init_debugfs(void) > > dma_buf_debugfs_dir = d; > > - d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, > + d = debugfs_create_file("bufinfo", 0444, dma_buf_debugfs_dir, > NULL, &dma_buf_debug_fops); > if (IS_ERR(d)) { > pr_debug("dma_buf: debugfs: failed to create node bufinfo\n"); > -- > 2.17.1 > Best, Sumit.

1 year, 1 month

Re: [PATCH v8 0/5] Support fdinfo runtime and memory stats on Panthor

by Jani Nikula

On Wed, 02 Oct 2024, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > Queued to drm-misc-next after applying the few modifications I > mentioned. Also added Steve's ack (given on IRC) on the first patch. Can we have the drm-tip rebuild conflict resolution too, please? diff --cc drivers/gpu/drm/panthor/panthor_drv.c index c520f156e2d7,f9b93f84d611..000000000000 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@@ -1383,7 -1476,7 +1476,11 @@@ static const struct file_operations pan .read = drm_read, .llseek = noop_llseek, .mmap = panthor_mmap, ++<<<<<<< HEAD + .fop_flags = FOP_UNSIGNED_OFFSET, ++======= + .show_fdinfo = drm_show_fdinfo, ++>>>>>>> drm-misc/drm-misc-next }; #ifdef CONFIG_DEBUG_FS > >> >> .../testing/sysfs-driver-panthor-profiling | 10 + >> Documentation/gpu/panthor.rst | 46 +++ >> drivers/gpu/drm/panthor/panthor_devfreq.c | 18 +- >> drivers/gpu/drm/panthor/panthor_device.h | 36 ++ >> drivers/gpu/drm/panthor/panthor_drv.c | 73 ++++ >> drivers/gpu/drm/panthor/panthor_gem.c | 12 + >> drivers/gpu/drm/panthor/panthor_sched.c | 384 +++++++++++++++--- >> drivers/gpu/drm/panthor/panthor_sched.h | 2 + >> 8 files changed, 531 insertions(+), 50 deletions(-) >> create mode 100644 Documentation/ABI/testing/sysfs-driver-panthor-profiling >> create mode 100644 Documentation/gpu/panthor.rst >> > -- Jani Nikula, Intel

1 year, 1 month

Re: [PATCH v8 1/5] drm/panthor: introduce job cycle and timestamp accounting

by Steven Price

On 02/10/2024 09:38, Boris Brezillon wrote: > On Tue, 24 Sep 2024 00:06:21 +0100 > Adrián Larumbe <adrian.larumbe(a)collabora.com> wrote: > >> +static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev, >> + u32 cs_ringbuf_size) >> +{ >> + u32 min_profiled_job_instrs = U32_MAX; >> + u32 last_flag = fls(PANTHOR_DEVICE_PROFILING_ALL); >> + >> + /* >> + * We want to calculate the minimum size of a profiled job's CS, >> + * because since they need additional instructions for the sampling >> + * of performance metrics, they might take up further slots in >> + * the queue's ringbuffer. This means we might not need as many job >> + * slots for keeping track of their profiling information. What we >> + * need is the maximum number of slots we should allocate to this end, >> + * which matches the maximum number of profiled jobs we can place >> + * simultaneously in the queue's ring buffer. >> + * That has to be calculated separately for every single job profiling >> + * flag, but not in the case job profiling is disabled, since unprofiled >> + * jobs don't need to keep track of this at all. >> + */ >> + for (u32 i = 0; i < last_flag; i++) { >> + if (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) > > I'll get rid of this check when applying, as suggested by Steve. Steve, > with this modification do you want me to add your R-b? Yes, please do. Thanks, Steve > BTW, I've also fixed a bunch of checkpatch errors/warnings, so you > might want to run checkpatch --strict next time. > >> + min_profiled_job_instrs = >> + min(min_profiled_job_instrs, calc_job_credits(BIT(i))); >> + } >> + >> + return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64)); >> +}

1 year, 1 month

Re: Question about 'dma_resv_get_fences'

by Christian König

Hi, Am 30.09.24 um 21:38 schrieb Zichen Xie: > Dear Linux Developers for DMA BUFFER SHARING FRAMEWORK, > > We are curious about the function 'dma_resv_get_fences' here: > https://elixir.bootlin.com/linux/v6.11/source/drivers/dma-buf/dma-resv.c#L5…, > and the logic below: > ``` > dma_resv_for_each_fence_unlocked(&cursor, fence) { > > if (dma_resv_iter_is_restarted(&cursor)) { > struct dma_fence **new_fences; > unsigned int count; > > while (*num_fences) > dma_fence_put((*fences)[--(*num_fences)]); > > count = cursor.num_fences + 1; > > /* Eventually re-allocate the array */ > new_fences = krealloc_array(*fences, count, > sizeof(void *), > GFP_KERNEL); > if (count && !new_fences) { > kfree(*fences); > *fences = NULL; > *num_fences = 0; > dma_resv_iter_end(&cursor); > return -ENOMEM; > } > *fences = new_fences; > } > > (*fences)[(*num_fences)++] = dma_fence_get(fence); > } > ``` > The existing check 'if (count && !new_fences)' may fail if count==0, > and 'krealloc_array' with count==0 is an undefined behavior. The > realloc may fail and return a NULL pointer, leading to a NULL Pointer > Dereference in '(*fences)[(*num_fences)++] = dma_fence_get(fence);' You already answered the question yourself "count = cursor.num_fences + 1;". So count can never be 0. What could theoretically be possible is that num_fences overflows, but this value isn't userspace controllable and we would run into memory allocation failures long before that happened. But we could potentially remove this whole handling since if there are no fences in the dma_resv object we don't enter the loop in the first place. Regards, Christian. > > Please correct us if we miss some key prerequisites for this function! > Thank you very much!

1 year, 1 month

Re: [PATCH v6 1/5] drm/panthor: introduce job cycle and timestamp accounting

by Steven Price

On 27/09/2024 15:53, Adrián Larumbe wrote: > On 25.09.2024 10:56, Steven Price wrote: >> On 23/09/2024 21:43, Adrián Larumbe wrote: >>> Hi Steve, >>> >>> On 23.09.2024 09:55, Steven Price wrote: >>>> On 20/09/2024 23:36, Adrián Larumbe wrote: >>>>> Hi Steve, thanks for the review. >>>> >>>> Hi Adrián, >>>> >>>>> I've applied all of your suggestions for the next patch series revision, so I'll >>>>> only be answering to your question about the calc_profiling_ringbuf_num_slots >>>>> function further down below. >>>>> >>>> >>>> [...] >>>> >>>>>>> @@ -3003,6 +3190,34 @@ static const struct drm_sched_backend_ops panthor_queue_sched_ops = { >>>>>>> .free_job = queue_free_job, >>>>>>> }; >>>>>>> >>>>>>> +static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev, >>>>>>> + u32 cs_ringbuf_size) >>>>>>> +{ >>>>>>> + u32 min_profiled_job_instrs = U32_MAX; >>>>>>> + u32 last_flag = fls(PANTHOR_DEVICE_PROFILING_ALL); >>>>>>> + >>>>>>> + /* >>>>>>> + * We want to calculate the minimum size of a profiled job's CS, >>>>>>> + * because since they need additional instructions for the sampling >>>>>>> + * of performance metrics, they might take up further slots in >>>>>>> + * the queue's ringbuffer. This means we might not need as many job >>>>>>> + * slots for keeping track of their profiling information. What we >>>>>>> + * need is the maximum number of slots we should allocate to this end, >>>>>>> + * which matches the maximum number of profiled jobs we can place >>>>>>> + * simultaneously in the queue's ring buffer. >>>>>>> + * That has to be calculated separately for every single job profiling >>>>>>> + * flag, but not in the case job profiling is disabled, since unprofiled >>>>>>> + * jobs don't need to keep track of this at all. >>>>>>> + */ >>>>>>> + for (u32 i = 0; i < last_flag; i++) { >>>>>>> + if (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) >>>>>>> + min_profiled_job_instrs = >>>>>>> + min(min_profiled_job_instrs, calc_job_credits(BIT(i))); >>>>>>> + } >>>>>>> + >>>>>>> + return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64)); >>>>>>> +} >>>>>> >>>>>> I may be missing something, but is there a situation where this is >>>>>> different to calc_job_credits(0)? AFAICT the infrastructure you've added >>>>>> can only add extra instructions to the no-flags case - whereas this >>>>>> implies you're thinking that instructions may also be removed (or replaced). >>>>>> >>>>>> Steve >>>>> >>>>> Since we create a separate kernel BO to hold the profiling information slot, we >>>>> need one that would be able to accomodate as many slots as the maximum number of >>>>> profiled jobs we can insert simultaneously into the queue's ring buffer. Because >>>>> profiled jobs always take more instructions than unprofiled ones, then we would >>>>> usually need fewer slots than the number of unprofiled jobs we could insert at >>>>> once in the ring buffer. >>>>> >>>>> Because we represent profiling metrics with a bit mask, then we need to test the >>>>> size of the CS for every single metric enabled in isolation, since enabling more >>>>> than one will always mean a bigger CS, and therefore fewer jobs tracked at once >>>>> in the queue's ring buffer. >>>>> >>>>> In our case, calling calc_job_credits(0) would simply tell us the number of >>>>> instructions we need for a normal job with no profiled features enabled, which >>>>> would always requiere less instructions than profiled ones, and therefore more >>>>> slots in the profiling info kernel BO. But we don't need to keep track of >>>>> profiling numbers for unprofiled jobs, so there's no point in calculating this >>>>> number. >>>>> >>>>> At first I was simply allocating a profiling info kernel BO as big as the number >>>>> of simultaneous unprofiled job slots in the ring queue, but Boris pointed out >>>>> that since queue ringbuffers can be as big as 2GiB, a lot of this memory would >>>>> be wasted, since profiled jobs always require more slots because they hold more >>>>> instructions, so fewer profiling slots in said kernel BO. >>>>> >>>>> The value of this approach will eventually manifest if we decided to keep track of >>>>> more profiling metrics, since this code won't have to change at all, other than >>>>> adding new profiling flags in the panthor_device_profiling_flags enum. >>>> >>>> Thanks for the detailed explanation. I think what I was missing is that >>>> the loop is checking each bit flag independently and *not* checking >>>> calc_job_credits(0). >>>> >>>> The check for (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) is probably what >>>> confused me - that should be completely redundant. Or at least we need >>>> something more intelligent if we have profiling bits which are not >>>> mutually compatible. >>> >>> I thought of an alternative that would only test bits that are actually part of >>> the mask: >>> >>> static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev, >>> u32 cs_ringbuf_size) >>> { >>> u32 min_profiled_job_instrs = U32_MAX; >>> u32 profiling_mask = PANTHOR_DEVICE_PROFILING_ALL; >>> >>> while (profiling_mask) { >>> u32 i = ffs(profiling_mask) - 1; >>> profiling_mask &= ~BIT(i); >>> min_profiled_job_instrs = >>> min(min_profiled_job_instrs, calc_job_credits(BIT(i))); >>> } >>> >>> return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64)); >>> } >>> >>> However, I don't think this would be more efficient, because ffs() is probably >>> fetching the first set bit by performing register shifts, and I guess this would >>> take somewhat longer than iterating over every single bit from the last one, >>> even if also matching them against the whole mask, just in case in future >>> additions of performance metrics we decide to leave some of the lower >>> significance bits untouched. >> >> Efficiency isn't very important here - we're not on a fast path, so it's >> more about ensuring the code is readable. I don't think the above is >> more readable then the original for loop. >> >>> Regarding your question about mutual compatibility, I don't think that is an >>> issue here, because we're testing bits in isolation. If in the future we find >>> out that some of the values we're profiling cannot be sampled at once, we can >>> add that logic to the sysfs knob handler, to make sure UM cannot set forbidden >>> profiling masks. >> >> My comment about compatibility is because in the original above you were >> calculating the top bit of PANTHOR_DEVICE_PROFILING_ALL: >> >>> u32 last_flag = fls(PANTHOR_DEVICE_PROFILING_ALL); >> >> then looping between 0 and that bit: >> >>> for (u32 i = 0; i < last_flag; i++) { >> >> So the test: >> >>> if (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) >> >> would only fail if PANTHOR_DEVICE_PROFILING_ALL had gaps in the bits >> that it set. The only reason I can think for that to be true in the >> future is if there is some sort of incompatibility - e.g. maybe there's >> an old and new way of doing some form of profiling with the old way >> being kept for backwards compatibility. But I suspect if/when that is >> required we'll need to revisit this function anyway. So that 'if' >> statement seems completely redundant (it's trivially always true). > > I think you're right about this. Would you be fine with the rest of the patch > as it is in revision 8 if I also deleted this bitmask check? Yes the rest of it looks fine. Thanks, Steve >> Steve >> >>>> I'm also not entirely sure that the amount of RAM saved is significant, >>>> but you've already written the code so we might as well have the saving ;) >>> >>> I think this was more evident before Boris suggested we reduce the basic slot >>> size to that of a single cache line, because then the minimum profiled job >>> might've taken twice as many ringbuffer slots as a nonprofiled one. In that >>> case, we would need a half as big BO for holding the sampled data (in case the >>> least size profiled job CS would extend over the 16 instruction boundary). >>> I still think this is a good idea so that in the future we don't need to worry >>> about adjusting the code that deals with preparing the right boilerplate CS, >>> since it'll only be a matter of adding new instructions inside prepare_job_instrs(). >>> >>>> Thanks, >>>> Steve >>>> >>>>> Regards, >>>>> Adrian >>>>> >>>>>>> + >>>>>>> static struct panthor_queue * >>>>>>> group_create_queue(struct panthor_group *group, >>>>>>> const struct drm_panthor_queue_create *args) >>>>>>> @@ -3056,9 +3271,35 @@ group_create_queue(struct panthor_group *group, >>>>>>> goto err_free_queue; >>>>>>> } >>>>>>> >>>>>>> + queue->profiling.slot_count = >>>>>>> + calc_profiling_ringbuf_num_slots(group->ptdev, args->ringbuf_size); >>>>>>> + >>>>>>> + queue->profiling.slots = >>>>>>> + panthor_kernel_bo_create(group->ptdev, group->vm, >>>>>>> + queue->profiling.slot_count * >>>>>>> + sizeof(struct panthor_job_profiling_data), >>>>>>> + DRM_PANTHOR_BO_NO_MMAP, >>>>>>> + DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | >>>>>>> + DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, >>>>>>> + PANTHOR_VM_KERNEL_AUTO_VA); >>>>>>> + >>>>>>> + if (IS_ERR(queue->profiling.slots)) { >>>>>>> + ret = PTR_ERR(queue->profiling.slots); >>>>>>> + goto err_free_queue; >>>>>>> + } >>>>>>> + >>>>>>> + ret = panthor_kernel_bo_vmap(queue->profiling.slots); >>>>>>> + if (ret) >>>>>>> + goto err_free_queue; >>>>>>> + >>>>>>> + /* >>>>>>> + * Credit limit argument tells us the total number of instructions >>>>>>> + * across all CS slots in the ringbuffer, with some jobs requiring >>>>>>> + * twice as many as others, depending on their profiling status. >>>>>>> + */ >>>>>>> ret = drm_sched_init(&queue->scheduler, &panthor_queue_sched_ops, >>>>>>> group->ptdev->scheduler->wq, 1, >>>>>>> - args->ringbuf_size / (NUM_INSTRS_PER_SLOT * sizeof(u64)), >>>>>>> + args->ringbuf_size / sizeof(u64), >>>>>>> 0, msecs_to_jiffies(JOB_TIMEOUT_MS), >>>>>>> group->ptdev->reset.wq, >>>>>>> NULL, "panthor-queue", group->ptdev->base.dev); >>>>>>> @@ -3354,6 +3595,7 @@ panthor_job_create(struct panthor_file *pfile, >>>>>>> { >>>>>>> struct panthor_group_pool *gpool = pfile->groups; >>>>>>> struct panthor_job *job; >>>>>>> + u32 credits; >>>>>>> int ret; >>>>>>> >>>>>>> if (qsubmit->pad) >>>>>>> @@ -3407,9 +3649,16 @@ panthor_job_create(struct panthor_file *pfile, >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> + job->profiling.mask = pfile->ptdev->profile_mask; >>>>>>> + credits = calc_job_credits(job->profiling.mask); >>>>>>> + if (credits == 0) { >>>>>>> + ret = -EINVAL; >>>>>>> + goto err_put_job; >>>>>>> + } >>>>>>> + >>>>>>> ret = drm_sched_job_init(&job->base, >>>>>>> &job->group->queues[job->queue_idx]->entity, >>>>>>> - 1, job->group); >>>>>>> + credits, job->group); >>>>>>> if (ret) >>>>>>> goto err_put_job; >>>>>>> >>>>>

1 year, 1 month

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig