Re: [PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in format
On 19/01/2024 07:54, Klymenko, Anatoliy wrote: diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..926e07c255bb 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x00 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x10 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x20 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x30 What's this about? Were these wrong before? Sounds like a separate patch needed for these. It is an embedded bit shift that corresponds to DPSUB live video / gfx format register layout. Original values are technically correct but would require extra bit shifts to operate with. The current patch is the first instance of actual use of those defines. Do you think it's worth to factor those changes out into a separate patch? The value for the defines should then be something like (0x3 << 4), to make it clearer that it's shifted to the right position. Tomi
Re: Flaky tests for vkms
Hi Maintainers, vkms test patch (https://lore.kernel.org/dri-devel/20240201065346.801038-1-vignesh.ra...@collabora.com/) was retested with latest drm-misc-next and xfails have been updated, Pipeline url: https://gitlab.freedesktop.org/vigneshraman/linux/-/jobs/54431093 https://gitlab.freedesktop.org/vigneshraman/linux/-/jobs/54446123 # Board Name: vkms # Bug Report: https://lore.kernel.org/dri-devel/005da8f1-8050-bffd-653c-2a87ae637...@collabora.com/T/#u # IGT Version: 1.28-gb0cc8160e # Linux Version: 6.7.0-rc3 # Failure Rate: 50 # Reported by deqp-runner kms_cursor_legacy@cursorA-vs-flipA-legacy kms_cursor_legacy@cursorA-vs-flipA-varying-size kms_flip@flip-vs-expired-vblank-interruptible kms_flip@flip-vs-expired-vblank kms_flip@plain-flip-fb-recreate kms_flip@plain-flip-fb-recreate-interruptible kms_flip@plain-flip-ts-check-interruptible # The below test shows inconsistency across multiple runs, # giving results of Pass and Fail alternately. kms_cursor_legacy@cursorA-vs-flipA-toggle kms_pipe_crc_basic@nonblocking-crc Regards, Vignesh On 02/01/24 13:01, Vignesh Raman wrote: Hi Maintainers, The patch introducing vkms driver testing in drm-ci has been submitted upstream (https://patchwork.kernel.org/project/dri-devel/patch/20230922171237.550762-3-helen.ko...@collabora.com/) We will send an update to this patch with new test results from the latest drm-misc-next. There are some flaky tests reported for vkms with the latest tests. # Board Name: vkms # Failure Rate: 50 # IGT Version: 1.28-gd2af13d9f # Linux Version: 6.7.0-rc3 Pipeline url: https://gitlab.freedesktop.org/vigneshraman/linux/-/jobs/53081973 # Reported by deqp-runner kms_cursor_legacy@cursorA-vs-flipA-legacy kms_cursor_legacy@cursorA-vs-flipA-varying-size Will add these tests in drivers/gpu/drm/ci/xfails/vkms-none-flakes.txt (https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/gpu/automated_testing.rst#n70) Please could you have a look at these test results and let us know if you need more information. Thank you. Regards, Vignesh
Re: [PATCH v3 4/5] drm: xlnx: zynqmp_dpsub: Filter interrupts against mask
On 24/01/2024 04:54, Anatoliy Klymenko wrote: Filter out status register against the interrupts' mask. Some events are being reported via DP status register, even if corresponding interrupts have been disabled. One instance of such event leads to generation of VBLANK when the driver is in DRM bridge mode, which in turn results in NULL pointer dereferencing. We should avoid processing such events in an interrupt handler context. This problem is less noticeable when the driver operates in DMA mode, as in this case we have DRM CRTC object instantiated and DRM framework simply discards unwanted VBLANKs in drm_handle_vblank(). Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 5a3335e1fffa..9f48e5bbcdec 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1627,7 +1627,14 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) /* clear status register as soon as we read it */ zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); - if (!(status & ~mask)) + + /* +* Status register may report some events, which corresponding interrupts +* have been disabled. Filter out those events against interrupts' mask. +*/ + status &= ~mask; + + if (!status) return IRQ_NONE; /* dbg for diagnostic, but not much that the driver can do */ Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible
On 01/02/2024 05:10, dharm...@microchip.com wrote: > On 31/01/24 12:42 am, Rob Herring wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> >> On Tue, Jan 23, 2024 at 03:39:13AM +, dharm...@microchip.com wrote: >>> Hi Conor, >>> >>> On 22/01/24 10:07 pm, Conor Dooley wrote: On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote: > On 22/01/2024 09:29, Dharma Balasubiramani wrote: >> Add the 'sam9x7-lvds' compatible binding, which describes the >> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's >> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to >> define the properties and configuration for the LVDS Controller in DT. >> >> Signed-off-by: Dharma Balasubiramani >> --- >>.../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++ >>1 file changed, 59 insertions(+) >>create mode 100644 >> Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml >> >> diff --git >> a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml >> >> b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml >> new file mode 100644 >> index ..8c2c5b858c85 >> --- /dev/null >> +++ >> b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml >> @@ -0,0 +1,59 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml# >> +$schema:http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Microchip SAM9X7 LVDS Controller > What is the "X"? > >> + >> +maintainers: >> + - Dharma Balasubiramani >> + >> +description: | > Do not need '|' unless you need to preserve formatting. > >> + The Low Voltage Differential Signaling Controller (LVDSC) manages data >> + format conversion from the LCD Controller internal DPI bus to OpenLDI >> + LVDS output signals. LVDSC functions include bit mapping, balanced >> mode >> + management, and serializer. >> + >> +properties: >> + compatible: >> +const: microchip,sam9x7-lvds > What is "x"? Wildcard? Then no, don't use it and instead use proper SoC > version number. These SoCs actually do have an x in their name. However, and I do always get confused here, the sam9x7 is a series of SoCs (the cover letter does say this) rather than a specific device. I think the series current consists of a sam9x70 sam9x72 and a sam9x75. The devices are largely similar, but I am not sure if the sam9x70 supports LVDS at all. Having a compatible for the series does not seem correct to me. >>> Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while >>> sam9x70 does not. I will revise the compatibility to include both >>> sam9x72 and sam9x75, as outlined below: >>> >>> properties: >>> compatible: >>> enum: >>> - microchip,sam9x72-lvds >>> - microchip,sam9x75-lvds >> >> I would presume these 2 are the same, but the above implies they >> aren't. I think what you had is fine assuming these are all >> fundamentally the same part with just packaging or fused off h/w >> differences. > > Yes, so is it okay to have compatible for a series? Shall I go ahead with > " >compatible: > const: microchip,sam9x7-lvds You said 9x70, which would match such 9x7 "series", is different, so I still think series should not be used. I don't know much about Microchip naming scheme, so this x is always confusing. However if these are the same, maybe just use sam9x72? Best regards, Krzysztof
Re: [PATCH v3 3/5] drm: xlnx: zynqmp_dpsub: Clear status register ASAP
On 24/01/2024 04:54, Anatoliy Klymenko wrote: Clear status register as soon as we read it. Addressing comments from https://lore.kernel.org/dri-devel/beb551c7-bb7e-4cd0-b166-e9aad90c4...@ideasonboard.com/ Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index d60b7431603f..5a3335e1fffa 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1624,6 +1624,8 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) u32 status, mask; status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); + /* clear status register as soon as we read it */ + zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); if (!(status & ~mask)) return IRQ_NONE; @@ -1634,8 +1636,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) dev_dbg_ratelimited(dp->dev, "overflow interrupt\n"); - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); - if (status & ZYNQMP_DP_INT_VBLANK_START) zynqmp_dpsub_drm_handle_vblank(dp->dpsub); Reviewed-by: Tomi Valkeinen Tomi
[PATCH] drm/modes: Replace deprecated simple_strtol with kstrtol
This patch replaces the use of the deprecated simple_strtol [1] function in the drm_modes.c file with the recommended kstrtol function. This change improves error handling and boundary checks. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull Signed-off-by: Cong Liu --- drivers/gpu/drm/drm_modes.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 893f52ee4926..fce0fb1df9b2 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1942,7 +1942,7 @@ static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr, return -EINVAL; str++; - bpp = simple_strtol(str, end_ptr, 10); + bpp = kstrtol(str, end_ptr, 10); if (*end_ptr == str) return -EINVAL; @@ -1961,7 +1961,7 @@ static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr, return -EINVAL; str++; - refresh = simple_strtol(str, end_ptr, 10); + refresh = kstrtol(str, end_ptr, 10); if (*end_ptr == str) return -EINVAL; @@ -2033,7 +2033,7 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, int remaining, i; char *end_ptr; - xres = simple_strtol(str, _ptr, 10); + xres = kstrtol(str, _ptr, 10); if (end_ptr == str) return -EINVAL; @@ -2042,7 +2042,7 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, end_ptr++; str = end_ptr; - yres = simple_strtol(str, _ptr, 10); + yres = kstrtol(str, _ptr, 10); if (end_ptr == str) return -EINVAL; @@ -2100,7 +2100,7 @@ static int drm_mode_parse_cmdline_int(const char *delim, unsigned int *int_ret) return -EINVAL; value = delim + 1; - *int_ret = simple_strtol(value, , 10); + *int_ret = kstrtol(value, , 10); /* Make sure we have parsed something */ if (endp == value) -- 2.34.1
[PATCH] fbdev/core: Replace deprecated simple_strtol with kstrtol
This patch replaces the use of the deprecated simple_strtol [1] function in the modedb.c file with the recommended kstrtol function. This change improves error handling and boundary checks. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull Signed-off-by: Cong Liu --- drivers/video/fbdev/core/modedb.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c index 7196b055f2bd..eebbbc7e2aa3 100644 --- a/drivers/video/fbdev/core/modedb.c +++ b/drivers/video/fbdev/core/modedb.c @@ -661,7 +661,7 @@ int fb_find_mode(struct fb_var_screeninfo *var, namelen = i; if (!refresh_specified && !bpp_specified && !yres_specified) { - refresh = simple_strtol([i+1], NULL, + refresh = kstrtol([i+1], NULL, 10); refresh_specified = 1; if (cvt || rb) @@ -672,7 +672,7 @@ int fb_find_mode(struct fb_var_screeninfo *var, case '-': namelen = i; if (!bpp_specified && !yres_specified) { - bpp = simple_strtol([i+1], NULL, + bpp = kstrtol([i+1], NULL, 10); bpp_specified = 1; if (cvt || rb) @@ -682,7 +682,7 @@ int fb_find_mode(struct fb_var_screeninfo *var, break; case 'x': if (!yres_specified) { - yres = simple_strtol([i+1], NULL, + yres = kstrtol([i+1], NULL, 10); yres_specified = 1; } else @@ -719,7 +719,7 @@ int fb_find_mode(struct fb_var_screeninfo *var, } } if (i < 0 && yres_specified) { - xres = simple_strtol(name, NULL, 10); + xres = kstrtol(name, NULL, 10); res_specified = 1; } done: -- 2.34.1
[PATCH] fbdev/sh_mobile_lcdcfb: Replace deprecated simple_strtol with kstrtol
This patch replaces the use of the deprecated simple_strtol [1] function in the sh_mobile_lcdcfb.c file with the recommended kstrtol function. This change improves error handling and boundary checks. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull Signed-off-by: Cong Liu --- drivers/video/fbdev/sh_mobile_lcdcfb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c index eb2297b37504..5fc7d74b273e 100644 --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c @@ -1278,11 +1278,11 @@ overlay_position_store(struct device *dev, struct device_attribute *attr, int pos_x; int pos_y; - pos_x = simple_strtol(buf, , 10); + pos_x = kstrtol(buf, , 10); if (*endp != ',') return -EINVAL; - pos_y = simple_strtol(endp + 1, , 10); + pos_y = kstrtol(endp + 1, , 10); if (isspace(*endp)) endp++; -- 2.34.1
[PATCH v4] drm/ci: add tests on vkms
Add job that runs igt on top of vkms. Signed-off-by: Vignesh Raman Acked-by: Jessica Zhang Tested-by: Jessica Zhang Acked-by: Maxime Ripard Signed-off-by: Helen Koike --- v2: - do not mv modules to /lib/modules in the job definition, leave it to crosvm-runner.sh v3: - Enable CONFIG_DRM_VKMS in x86_64.config and update xfails v3: - Build vkms as module and test with latest IGT. This patch depends on https://lore.kernel.org/dri-devel/20240130150340.687871-1-vignesh.ra...@collabora.com/ --- MAINTAINERS | 1 + drivers/gpu/drm/ci/build.sh | 1 - drivers/gpu/drm/ci/gitlab-ci.yml | 2 +- drivers/gpu/drm/ci/igt_runner.sh | 6 ++-- drivers/gpu/drm/ci/image-tags.yml | 2 +- drivers/gpu/drm/ci/test.yml | 24 +- drivers/gpu/drm/ci/x86_64.config | 1 + .../drm/ci/xfails/virtio_gpu-none-fails.txt | 1 - drivers/gpu/drm/ci/xfails/vkms-none-fails.txt | 32 +++ .../gpu/drm/ci/xfails/vkms-none-flakes.txt| 19 +++ drivers/gpu/drm/ci/xfails/vkms-none-skips.txt | 16 ++ 11 files changed, 97 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/ci/xfails/vkms-none-fails.txt create mode 100644 drivers/gpu/drm/ci/xfails/vkms-none-flakes.txt create mode 100644 drivers/gpu/drm/ci/xfails/vkms-none-skips.txt diff --git a/MAINTAINERS b/MAINTAINERS index bcdc17d1aa26..09310a6f4b5f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6923,6 +6923,7 @@ L:dri-devel@lists.freedesktop.org S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc F: Documentation/gpu/vkms.rst +F: drivers/gpu/drm/ci/xfails/vkms* F: drivers/gpu/drm/vkms/ DRM DRIVER FOR VIRTUALBOX VIRTUAL GPU diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh index 331a61e0d25a..2e089e03f061 100644 --- a/drivers/gpu/drm/ci/build.sh +++ b/drivers/gpu/drm/ci/build.sh @@ -152,7 +152,6 @@ fi mkdir -p artifacts/install/lib mv install/* artifacts/install/. -rm -rf artifacts/install/modules ln -s common artifacts/install/ci-common cp .config artifacts/${CI_JOB_NAME}_config diff --git a/drivers/gpu/drm/ci/gitlab-ci.yml b/drivers/gpu/drm/ci/gitlab-ci.yml index e2b021616a8e..c69fb6af4cf8 100644 --- a/drivers/gpu/drm/ci/gitlab-ci.yml +++ b/drivers/gpu/drm/ci/gitlab-ci.yml @@ -107,7 +107,7 @@ stages: - meson - msm - rockchip - - virtio-gpu + - software-driver # YAML anchors for rule conditions # diff --git a/drivers/gpu/drm/ci/igt_runner.sh b/drivers/gpu/drm/ci/igt_runner.sh index 2fd09b9b7cf6..3c7f000805e5 100755 --- a/drivers/gpu/drm/ci/igt_runner.sh +++ b/drivers/gpu/drm/ci/igt_runner.sh @@ -20,10 +20,10 @@ cat /sys/kernel/debug/dri/*/state set -e case "$DRIVER_NAME" in -amdgpu) +amdgpu|vkms) # Cannot use HWCI_KERNEL_MODULES as at that point we don't have the module in /lib -mv /install/modules/lib/modules/* /lib/modules/. -modprobe amdgpu +mv /install/modules/lib/modules/* /lib/modules/. || true +modprobe --first-time $DRIVER_NAME ;; esac diff --git a/drivers/gpu/drm/ci/image-tags.yml b/drivers/gpu/drm/ci/image-tags.yml index cf07c3e09b8c..bf861ab8b9c2 100644 --- a/drivers/gpu/drm/ci/image-tags.yml +++ b/drivers/gpu/drm/ci/image-tags.yml @@ -4,7 +4,7 @@ variables: DEBIAN_BASE_TAG: "${CONTAINER_TAG}" DEBIAN_X86_64_BUILD_IMAGE_PATH: "debian/x86_64_build" - DEBIAN_BUILD_TAG: "2023-10-08-config" + DEBIAN_BUILD_TAG: "2024-01-29-vkms" KERNEL_ROOTFS_TAG: "2023-10-06-amd" PKG_REPO_REV: "67f2c46b" diff --git a/drivers/gpu/drm/ci/test.yml b/drivers/gpu/drm/ci/test.yml index 8ab8a8f56d6a..58c3cf4b18e0 100644 --- a/drivers/gpu/drm/ci/test.yml +++ b/drivers/gpu/drm/ci/test.yml @@ -399,7 +399,7 @@ meson:g12b-display: DRIVER_NAME: meson virtio_gpu:none: - stage: virtio-gpu + stage: software-driver variables: CROSVM_GALLIUM_DRIVER: llvmpipe DRIVER_NAME: virtio_gpu @@ -419,3 +419,25 @@ virtio_gpu:none: - debian/x86_64_test-gl - testing:x86_64 - igt:x86_64 + +vkms:none: + stage: software-driver + variables: +DRIVER_NAME: vkms +GPU_VERSION: vkms-none + extends: +- .test-gl +- .test-rules + tags: +- kvm + script: +- ln -sf $CI_PROJECT_DIR/install /install +- mv install/bzImage /lava-files/bzImage +- mkdir -p /lib/modules +- mkdir -p $CI_PROJECT_DIR/results +- ln -sf $CI_PROJECT_DIR/results /results +- ./install/crosvm-runner.sh ./install/igt_runner.sh + needs: +- debian/x86_64_test-gl +- testing:x86_64 +- igt:x86_64 diff --git a/drivers/gpu/drm/ci/x86_64.config b/drivers/gpu/drm/ci/x86_64.config index 1cbd49a5b23a..8eaba388b141 100644 --- a/drivers/gpu/drm/ci/x86_64.config +++ b/drivers/gpu/drm/ci/x86_64.config @@ -24,6 +24,7 @@ CONFIG_DRM=y CONFIG_DRM_PANEL_SIMPLE=y CONFIG_PWM_CROS_EC=y
[PATCH 1/1] fbdev/hyperv_fb: Fix logic error for Gen2 VMs in hvfb_getmem()
From: Michael Kelley A recent commit removing the use of screen_info introduced a logic error. The error causes hvfb_getmem() to always return -ENOMEM for Generation 2 VMs. As a result, the Hyper-V frame buffer device fails to initialize. The error was introduced by removing an "else if" clause, leaving Gen2 VMs to always take the -ENOMEM error path. Fix the problem by removing the error path "else" clause. Gen 2 VMs now always proceed through the MMIO memory allocation code, but with "base" and "size" defaulting to 0. Fixes: 0aa0838c84da ("fbdev/hyperv_fb: Remove firmware framebuffers with aperture helpers") Signed-off-by: Michael Kelley --- drivers/video/fbdev/hyperv_fb.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index c26ee6fd73c9..8fdccf033b2d 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -1010,8 +1010,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) goto getmem_done; } pr_info("Unable to allocate enough contiguous physical memory on Gen 1 VM. Using MMIO instead.\n"); - } else { - goto err1; } /* -- 2.25.1
Re: [PATCH 11/17] drm/msm/dp: add VSC SDP support for YUV420 over DP
On Thu, 1 Feb 2024 at 03:56, Abhinav Kumar wrote: > > > > On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote: > > On Sun, 28 Jan 2024 at 07:34, Paloma Arellano > > wrote: > >> > >> > >> On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: > >>> On 25/01/2024 21:38, Paloma Arellano wrote: > Add support to pack and send the VSC SDP packet for DP. This therefore > allows the transmision of format information to the sinks which is > needed for YUV420 support over DP. > > Signed-off-by: Paloma Arellano > --- > drivers/gpu/drm/msm/dp/dp_catalog.c | 147 > drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + > drivers/gpu/drm/msm/dp/dp_ctrl.c| 4 + > drivers/gpu/drm/msm/dp/dp_panel.c | 47 + > drivers/gpu/drm/msm/dp/dp_reg.h | 3 + > 5 files changed, 205 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > b/drivers/gpu/drm/msm/dp/dp_catalog.c > index c025786170ba5..7e4c68be23e56 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -29,6 +29,9 @@ > #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) > +#define DP_GENERIC0_6_YUV_8_BPCBIT(0) > +#define DP_GENERIC0_6_YUV_10_BPCBIT(1) > + > #define DP_INTERRUPT_STATUS1 \ > (DP_INTR_AUX_XFER_DONE| \ > DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ > @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct > dp_catalog *dp_catalog) > return 0; > } > +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog > *dp_catalog) > +{ > +struct dp_catalog_private *catalog; > +u32 header, parity, data; > +u8 bpc, off = 0; > +u8 buf[SZ_128]; > + > +if (!dp_catalog) { > +pr_err("invalid input\n"); > +return; > +} > + > +catalog = container_of(dp_catalog, struct dp_catalog_private, > dp_catalog); > + > +/* HEADER BYTE 1 */ > +header = dp_catalog->sdp.sdp_header.HB1; > +parity = dp_catalog_calculate_parity(header); > +data = ((header << HEADER_BYTE_1_BIT) | (parity << > PARITY_BYTE_1_BIT)); > +dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); > +memcpy(buf + off, , sizeof(data)); > +off += sizeof(data); > + > +/* HEADER BYTE 2 */ > +header = dp_catalog->sdp.sdp_header.HB2; > +parity = dp_catalog_calculate_parity(header); > +data = ((header << HEADER_BYTE_2_BIT) | (parity << > PARITY_BYTE_2_BIT)); > +dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > + > +/* HEADER BYTE 3 */ > +header = dp_catalog->sdp.sdp_header.HB3; > +parity = dp_catalog_calculate_parity(header); > +data = ((header << HEADER_BYTE_3_BIT) | (parity << > PARITY_BYTE_3_BIT)); > +data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); > +dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > +memcpy(buf + off, , sizeof(data)); > +off += sizeof(data); > >>> > >>> This seems to be common with the dp_audio code. Please extract this > >>> header writing too. > >> These are two different sdp's. audio and vsc, are different with > >> different registers being written to and different amount of registers > >> being set. Can you please clarify since in audio we only need 3 > >> registers to write to, and in vsc we need 10. > > > > Bitmagic with the header is the same. Then the rest of the data is > > written one dword per register, if I'm not mistaken. > > > > We can generalize the MMSS_DP_GENERIC0 register writing by breaking it > up to two things: > > 1) Add a function vsc_sdp_pack() similar to hdmi_avi_infoframe_pack_only() Note, there is already a hdmi_audio_infoframe_pack_for_dp() function. I think this patchset can add hdmi_colorimetry_infoframe_pack_for_dp() [you can choose any other similar name that suits from your POV]. Also please extract the function that inits the dp_sdp_header. It can be reused as is for both existing hdmi_audio_infoframe_pack_for_dp(), your new function and the dp_audio code. > > 2) dp_catalog_write_generic_pkt() which will just write the packed > buffer byte-by-byte to these MMSS_DP_GENERIC0_xxx register > > But audio seems a bit different. We use DP_AUDIO_STREAM_0/1. > More importantly, it uses this sdp_map and writes each header one by one > with dp_catalog_audio_set_header(). > > Not sure if that entirely fits with this pack and then write model. > > It can be simplified. But I dont think this effort is needed for this > series. > > So I would prefer to generalize audio SDP programming separately. I'd definitely ask to add a utility function that merges 4 header bytes with the parity data. We already have 5 instances of that code in dp_audio.c, which is already
Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible
On 31/01/24 12:42 am, Rob Herring wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Tue, Jan 23, 2024 at 03:39:13AM +, dharm...@microchip.com wrote: >> Hi Conor, >> >> On 22/01/24 10:07 pm, Conor Dooley wrote: >>> On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote: On 22/01/2024 09:29, Dharma Balasubiramani wrote: > Add the 'sam9x7-lvds' compatible binding, which describes the > Low Voltage Differential Signaling (LVDS) Controller found on Microchip's > sam9x7 series System-on-Chip (SoC) devices. This binding will be used to > define the properties and configuration for the LVDS Controller in DT. > > Signed-off-by: Dharma Balasubiramani > --- >.../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++ >1 file changed, 59 insertions(+) >create mode 100644 > Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml > > diff --git > a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml > > b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml > new file mode 100644 > index ..8c2c5b858c85 > --- /dev/null > +++ > b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml > @@ -0,0 +1,59 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml# > +$schema:http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip SAM9X7 LVDS Controller What is the "X"? > + > +maintainers: > + - Dharma Balasubiramani > + > +description: | Do not need '|' unless you need to preserve formatting. > + The Low Voltage Differential Signaling Controller (LVDSC) manages data > + format conversion from the LCD Controller internal DPI bus to OpenLDI > + LVDS output signals. LVDSC functions include bit mapping, balanced mode > + management, and serializer. > + > +properties: > + compatible: > +const: microchip,sam9x7-lvds What is "x"? Wildcard? Then no, don't use it and instead use proper SoC version number. >>> These SoCs actually do have an x in their name. However, and I do always >>> get confused here, the sam9x7 is a series of SoCs (the cover letter does >>> say this) rather than a specific device. >>> I think the series current consists of a sam9x70 sam9x72 and a sam9x75. >>> The devices are largely similar, but I am not sure if the sam9x70 >>> supports LVDS at all. Having a compatible for the series does not seem >>> correct to me. >> Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while >> sam9x70 does not. I will revise the compatibility to include both >> sam9x72 and sam9x75, as outlined below: >> >> properties: >> compatible: >> enum: >> - microchip,sam9x72-lvds >> - microchip,sam9x75-lvds > > I would presume these 2 are the same, but the above implies they > aren't. I think what you had is fine assuming these are all > fundamentally the same part with just packaging or fused off h/w > differences. Yes, so is it okay to have compatible for a series? Shall I go ahead with " compatible: const: microchip,sam9x7-lvds " itself? -- Thanks, Dharma B. > > Rob
Re: [linux][PATCH v5 0/3] Convert Microchip's HLCDC Text based DT bindings to JSON schema
Hi Rob, On 31/01/24 9:05 am, Dharma B - I70843 wrote: > Converted the text bindings to YAML and validated them individually using > following commands > > $ make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/ > $ make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/ > > changelogs are available in respective patches. > > As Sam suggested I'm sending the PWM binding as it is in this patch series, > clean up patch > will be sent as separate patch. > I would want to know if I can have the examples in display and pwm bindings separately or if I have to delete them from both and have a single, comprehensive example in mfd binding. I'm a little puzzled about this. > Dharma Balasubiramani (3): >dt-bindings: display: convert Atmel's HLCDC to DT schema >dt-bindings: atmel,hlcdc: convert pwm bindings to json-schema >dt-bindings: mfd: atmel,hlcdc: Convert to DT schema format > > .../atmel/atmel,hlcdc-display-controller.yaml | 85 > .../bindings/display/atmel/hlcdc-dc.txt | 75 -- > .../devicetree/bindings/mfd/atmel,hlcdc.yaml | 99 +++ > .../devicetree/bindings/mfd/atmel-hlcdc.txt | 56 --- > .../bindings/pwm/atmel,hlcdc-pwm.yaml | 44 + > .../bindings/pwm/atmel-hlcdc-pwm.txt | 29 -- > 6 files changed, 228 insertions(+), 160 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-display-controller.yaml > delete mode 100644 > Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt > create mode 100644 Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml > delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt > create mode 100644 > Documentation/devicetree/bindings/pwm/atmel,hlcdc-pwm.yaml > delete mode 100644 Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt > -- With Best Regards, Dharma B.
Re: [PATCH 14/17] drm/msm/dpu: modify encoder programming for CDM over DP
On Thu, 1 Feb 2024 at 03:30, Abhinav Kumar wrote: > > > > On 1/29/2024 3:44 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar > > wrote: > >> > >> On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: > >>> On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar > >>> wrote: > > > > On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar > > wrote: > >> > >> > >> > >> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: > >>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano > >>> wrote: > > > On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > > On 25/01/2024 21:38, Paloma Arellano wrote: > >> Adjust the encoder format programming in the case of video mode > >> for DP > >> to accommodate CDM related changes. > >> > >> Signed-off-by: Paloma Arellano > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 + > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 + > >> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 > >> --- > >> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++ > >> drivers/gpu/drm/msm/msm_drv.h | 9 - > >> 5 files changed, 75 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> index b0896814c1562..99ec53446ad21 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > >> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > >> }; > >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder > >> *drm_enc, > >> const struct drm_display_mode *mode) > >> +{ > >> +const struct dpu_encoder_virt *dpu_enc; > >> +const struct msm_display_info *disp_info; > >> +struct msm_drm_private *priv; > >> + > >> +dpu_enc = to_dpu_encoder_virt(drm_enc); > >> +disp_info = _enc->disp_info; > >> +priv = drm_enc->dev->dev_private; > >> + > >> +if (disp_info->intf_type == INTF_DP && > >> + > >> msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], > >> mode)) > > > > This should not require interacting with DP. If we got here, we must > > be sure that 4:2:0 is supported and can be configured. > Ack. Will drop this function and only check for if the mode is > YUV420. > > > >> +return DRM_FORMAT_YUV420; > >> + > >> +return DRM_FORMAT_RGB888; > >> +} > >> bool dpu_encoder_is_widebus_enabled(const struct > >> drm_encoder > >> *drm_enc) > >> { > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> index 7b4afa71f1f96..62255d0aa4487 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct > >> drm_encoder *drm_enc); > >>*/ > >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >> *drm_enc); > >> +/** > >> + * dpu_encoder_get_drm_fmt - return DRM fourcc format > >> + * @drm_enc:Pointer to previously created drm encoder > >> structure > >> + * @mode:Corresponding drm_display_mode for dpu encoder > >> + */ > >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >> +const struct drm_display_mode *mode); > >> + > >> /** > >>* dpu_encoder_get_crc_values_cnt - get number of physical > >> encoders > >> contained > >>*in virtual encoder that can collect CRC values > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> index e284bf448bdda..a1dde0ff35dc8 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> @@ -234,6 +234,7 @@ static void > >> dpu_encoder_phys_vid_setup_timing_engine( > >> { > >> struct drm_display_mode mode; > >> struct dpu_hw_intf_timing_params timing_params = { 0 }; > >> +struct dpu_hw_cdm *hw_cdm; >
Re: [PATCH 11/17] drm/msm/dp: add VSC SDP support for YUV420 over DP
On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote: On Sun, 28 Jan 2024 at 07:34, Paloma Arellano wrote: On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: On 25/01/2024 21:38, Paloma Arellano wrote: Add support to pack and send the VSC SDP packet for DP. This therefore allows the transmision of format information to the sinks which is needed for YUV420 support over DP. Signed-off-by: Paloma Arellano --- drivers/gpu/drm/msm/dp/dp_catalog.c | 147 drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_ctrl.c| 4 + drivers/gpu/drm/msm/dp/dp_panel.c | 47 + drivers/gpu/drm/msm/dp/dp_reg.h | 3 + 5 files changed, 205 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index c025786170ba5..7e4c68be23e56 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -29,6 +29,9 @@ #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) +#define DP_GENERIC0_6_YUV_8_BPCBIT(0) +#define DP_GENERIC0_6_YUV_10_BPCBIT(1) + #define DP_INTERRUPT_STATUS1 \ (DP_INTR_AUX_XFER_DONE| \ DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog) return 0; } +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog *dp_catalog) +{ +struct dp_catalog_private *catalog; +u32 header, parity, data; +u8 bpc, off = 0; +u8 buf[SZ_128]; + +if (!dp_catalog) { +pr_err("invalid input\n"); +return; +} + +catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); + +/* HEADER BYTE 1 */ +header = dp_catalog->sdp.sdp_header.HB1; +parity = dp_catalog_calculate_parity(header); +data = ((header << HEADER_BYTE_1_BIT) | (parity << PARITY_BYTE_1_BIT)); +dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); +memcpy(buf + off, , sizeof(data)); +off += sizeof(data); + +/* HEADER BYTE 2 */ +header = dp_catalog->sdp.sdp_header.HB2; +parity = dp_catalog_calculate_parity(header); +data = ((header << HEADER_BYTE_2_BIT) | (parity << PARITY_BYTE_2_BIT)); +dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); + +/* HEADER BYTE 3 */ +header = dp_catalog->sdp.sdp_header.HB3; +parity = dp_catalog_calculate_parity(header); +data = ((header << HEADER_BYTE_3_BIT) | (parity << PARITY_BYTE_3_BIT)); +data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); +dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); +memcpy(buf + off, , sizeof(data)); +off += sizeof(data); This seems to be common with the dp_audio code. Please extract this header writing too. These are two different sdp's. audio and vsc, are different with different registers being written to and different amount of registers being set. Can you please clarify since in audio we only need 3 registers to write to, and in vsc we need 10. Bitmagic with the header is the same. Then the rest of the data is written one dword per register, if I'm not mistaken. We can generalize the MMSS_DP_GENERIC0 register writing by breaking it up to two things: 1) Add a function vsc_sdp_pack() similar to hdmi_avi_infoframe_pack_only() 2) dp_catalog_write_generic_pkt() which will just write the packed buffer byte-by-byte to these MMSS_DP_GENERIC0_xxx register But audio seems a bit different. We use DP_AUDIO_STREAM_0/1. More importantly, it uses this sdp_map and writes each header one by one with dp_catalog_audio_set_header(). Not sure if that entirely fits with this pack and then write model. It can be simplified. But I dont think this effort is needed for this series. So I would prefer to generalize audio SDP programming separately. + +data = 0; +dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); +memcpy(buf + off, , sizeof(data)); +off += sizeof(data); Generally this is not how these functions are expected to be written. Please take a look at drivers/video/hdmi.c. It should be split into: - generic function that packs the C structure into a flat byte buffer, - driver-specific function that formats and writes the buffer to the hardware. +dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); +memcpy(buf + off, , sizeof(data)); +off += sizeof(data); + +dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); +memcpy(buf + off, , sizeof(data)); +off += sizeof(data); + +dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); +memcpy(buf + off, , sizeof(data)); +off += sizeof(data); + +switch (dp_catalog->vsc_sdp_data.bpc) { +case 10: +bpc = DP_GENERIC0_6_YUV_10_BPC; +break; +case 8: +default: +bpc = DP_GENERIC0_6_YUV_8_BPC; +break; +} + +/* VSC SDP payload as per table 2-117 of DP 1.4 specification */ +data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) <<
Re: [PATCH 14/17] drm/msm/dpu: modify encoder programming for CDM over DP
On 1/29/2024 3:44 PM, Dmitry Baryshkov wrote: On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar wrote: On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar wrote: On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar wrote: On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: On Sun, 28 Jan 2024 at 07:48, Paloma Arellano wrote: On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: On 25/01/2024 21:38, Paloma Arellano wrote: Adjust the encoder format programming in the case of video mode for DP to accommodate CDM related changes. Signed-off-by: Paloma Arellano --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 + .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 --- drivers/gpu/drm/msm/dp/dp_display.c | 12 +++ drivers/gpu/drm/msm/msm_drv.h | 9 - 5 files changed, 75 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index b0896814c1562..99ec53446ad21 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 }; +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, const struct drm_display_mode *mode) +{ +const struct dpu_encoder_virt *dpu_enc; +const struct msm_display_info *disp_info; +struct msm_drm_private *priv; + +dpu_enc = to_dpu_encoder_virt(drm_enc); +disp_info = _enc->disp_info; +priv = drm_enc->dev->dev_private; + +if (disp_info->intf_type == INTF_DP && + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], mode)) This should not require interacting with DP. If we got here, we must be sure that 4:2:0 is supported and can be configured. Ack. Will drop this function and only check for if the mode is YUV420. +return DRM_FORMAT_YUV420; + +return DRM_FORMAT_RGB888; +} bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 7b4afa71f1f96..62255d0aa4487 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); */ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); +/** + * dpu_encoder_get_drm_fmt - return DRM fourcc format + * @drm_enc:Pointer to previously created drm encoder structure + * @mode:Corresponding drm_display_mode for dpu encoder + */ +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, +const struct drm_display_mode *mode); + /** * dpu_encoder_get_crc_values_cnt - get number of physical encoders contained *in virtual encoder that can collect CRC values diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index e284bf448bdda..a1dde0ff35dc8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -234,6 +234,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine( { struct drm_display_mode mode; struct dpu_hw_intf_timing_params timing_params = { 0 }; +struct dpu_hw_cdm *hw_cdm; const struct dpu_format *fmt = NULL; u32 fmt_fourcc = DRM_FORMAT_RGB888; unsigned long lock_flags; @@ -254,17 +255,26 @@ static void dpu_encoder_phys_vid_setup_timing_engine( DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); drm_mode_debug_printmodeline(); -if (phys_enc->split_role != ENC_ROLE_SOLO) { +hw_cdm = phys_enc->hw_cdm; +if (hw_cdm) { +intf_cfg.cdm = hw_cdm->idx; +fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, ); +} + +if (phys_enc->split_role != ENC_ROLE_SOLO || +dpu_encoder_get_drm_fmt(phys_enc->parent, ) == DRM_FORMAT_YUV420) { mode.hdisplay >>= 1; mode.htotal >>= 1; mode.hsync_start >>= 1; mode.hsync_end >>= 1; +mode.hskew >>= 1; Separate patch. Ack. DPU_DEBUG_VIDENC(phys_enc, -"split_role %d, halve horizontal %d %d %d %d\n", +"split_role %d, halve horizontal %d %d %d %d %d\n", phys_enc->split_role, mode.hdisplay, mode.htotal, -mode.hsync_start, mode.hsync_end); +mode.hsync_start, mode.hsync_end, +mode.hskew); } drm_mode_to_intf_timing_params(phys_enc, , _params); @@ -412,8 +422,15 @@ static int
Re: [PATCH] drm/msm/dpu: fix the programming of INTF_CFG2_DATA_HCTL_EN
On 1/31/2024 5:05 PM, Dmitry Baryshkov wrote: On Thu, 1 Feb 2024 at 02:48, Abhinav Kumar wrote: Currently INTF_CFG2_DATA_HCTL_EN is coupled with the enablement of widebus but this is incorrect because we should be enabling this bit independent of widebus except for cases where compression is enabled in one pixel per clock mode. Fix this by making the condition checks more explicit and enabling INTF_CFG2_DATA_HCTL_EN for all other cases when supported by DPU. Fixes: 3309a7563971 ("drm/msm/dpu: revise timing engine programming to support widebus feature") Suggested-by: Dmitry Baryshkov Signed-off-by: Abhinav Kumar Thank you! Reviewed-by: Dmitry Baryshkov For the reference: although it is marked as a fix, I'd prefer if this patch undergoes a full cycle through msm-next rather than fast-tracking through msm-fixes. This would allow us to catch possible issues. WDYT? Yes, I dont plan to take this in -fixes. Even though this was tested with sc7280, sm8550 before posting, I would like to postpone it for a major release as no use-case other than YUV420 is broken without this. This will be taken as part of CDM over DP series as we will backout the other change: https://patchwork.freedesktop.org/patch/575963/ --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 +++ .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 15 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 + 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 83380bc92a00..467f874979d5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -230,6 +230,13 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) return dpu_enc->wide_bus_en; } +bool dpu_encoder_is_dsc_enabled(const struct drm_encoder *drm_enc) +{ + const struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); + + return dpu_enc->dsc ? true : false; +} + int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) { struct dpu_encoder_virt *dpu_enc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 4c05fd5e9ed1..fe6b1d312a74 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -158,6 +158,13 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); +/** + * dpu_encoder_is_dsc_enabled - indicate whether dsc is enabled + * for the encoder. + * @drm_enc:Pointer to previously created drm encoder structure + */ +bool dpu_encoder_is_dsc_enabled(const struct drm_encoder *drm_enc); + /** * dpu_encoder_get_crc_values_cnt - get number of physical encoders contained * in virtual encoder that can collect CRC values diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index d0f56c5c4cce..f562beb6f797 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -102,6 +102,7 @@ static void drm_mode_to_intf_timing_params( } timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent); + timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent); /* * for DP, divide the horizonal parameters by 2 when diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 6bba531d6dc4..965692ef7892 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -163,13 +163,8 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width; display_hctl = (hsync_end_x << 16) | hsync_start_x; - /* -* DATA_HCTL_EN controls data timing which can be different from -* video timing. It is recommended to enable it for all cases, except -* if compression is enabled in 1 pixel per clock mode -*/ if (p->wide_bus_en) - intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN; data_width = p->width; @@ -229,6 +224,14 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg); DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format); if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) { + /* +* DATA_HCTL_EN controls data timing which can be different from +
Re: [PATCH] drm/msm/dpu: fix the programming of INTF_CFG2_DATA_HCTL_EN
On Thu, 1 Feb 2024 at 02:48, Abhinav Kumar wrote: > > Currently INTF_CFG2_DATA_HCTL_EN is coupled with the enablement > of widebus but this is incorrect because we should be enabling > this bit independent of widebus except for cases where compression > is enabled in one pixel per clock mode. > > Fix this by making the condition checks more explicit and enabling > INTF_CFG2_DATA_HCTL_EN for all other cases when supported by DPU. > > Fixes: 3309a7563971 ("drm/msm/dpu: revise timing engine programming to > support widebus feature") > Suggested-by: Dmitry Baryshkov > Signed-off-by: Abhinav Kumar Thank you! Reviewed-by: Dmitry Baryshkov For the reference: although it is marked as a fix, I'd prefer if this patch undergoes a full cycle through msm-next rather than fast-tracking through msm-fixes. This would allow us to catch possible issues. WDYT? > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 +++ > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 15 +-- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 + > 5 files changed, 25 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 83380bc92a00..467f874979d5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -230,6 +230,13 @@ bool dpu_encoder_is_widebus_enabled(const struct > drm_encoder *drm_enc) > return dpu_enc->wide_bus_en; > } > > +bool dpu_encoder_is_dsc_enabled(const struct drm_encoder *drm_enc) > +{ > + const struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > + > + return dpu_enc->dsc ? true : false; > +} > + > int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) > { > struct dpu_encoder_virt *dpu_enc; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 4c05fd5e9ed1..fe6b1d312a74 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -158,6 +158,13 @@ int dpu_encoder_get_vsync_count(struct drm_encoder > *drm_enc); > > bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); > > +/** > + * dpu_encoder_is_dsc_enabled - indicate whether dsc is enabled > + * for the encoder. > + * @drm_enc:Pointer to previously created drm encoder structure > + */ > +bool dpu_encoder_is_dsc_enabled(const struct drm_encoder *drm_enc); > + > /** > * dpu_encoder_get_crc_values_cnt - get number of physical encoders contained > * in virtual encoder that can collect CRC values > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > index d0f56c5c4cce..f562beb6f797 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > @@ -102,6 +102,7 @@ static void drm_mode_to_intf_timing_params( > } > > timing->wide_bus_en = > dpu_encoder_is_widebus_enabled(phys_enc->parent); > + timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent); > > /* > * for DP, divide the horizonal parameters by 2 when > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > index 6bba531d6dc4..965692ef7892 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > @@ -163,13 +163,8 @@ static void dpu_hw_intf_setup_timing_engine(struct > dpu_hw_intf *ctx, > hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width; > display_hctl = (hsync_end_x << 16) | hsync_start_x; > > - /* > -* DATA_HCTL_EN controls data timing which can be different from > -* video timing. It is recommended to enable it for all cases, except > -* if compression is enabled in 1 pixel per clock mode > -*/ > if (p->wide_bus_en) > - intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; > + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN; > > data_width = p->width; > > @@ -229,6 +224,14 @@ static void dpu_hw_intf_setup_timing_engine(struct > dpu_hw_intf *ctx, > DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg); > DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format); > if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) { > + /* > +* DATA_HCTL_EN controls data timing which can be different > from > +* video timing. It is recommended to enable it for all > cases, except > +* if compression is enabled in 1 pixel per clock mode > +*/ > + if (!(p->compression_en && !p->wide_bus_en)) >
[PATCH] drm/msm/dpu: fix the programming of INTF_CFG2_DATA_HCTL_EN
Currently INTF_CFG2_DATA_HCTL_EN is coupled with the enablement of widebus but this is incorrect because we should be enabling this bit independent of widebus except for cases where compression is enabled in one pixel per clock mode. Fix this by making the condition checks more explicit and enabling INTF_CFG2_DATA_HCTL_EN for all other cases when supported by DPU. Fixes: 3309a7563971 ("drm/msm/dpu: revise timing engine programming to support widebus feature") Suggested-by: Dmitry Baryshkov Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 +++ .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 15 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 + 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 83380bc92a00..467f874979d5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -230,6 +230,13 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) return dpu_enc->wide_bus_en; } +bool dpu_encoder_is_dsc_enabled(const struct drm_encoder *drm_enc) +{ + const struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); + + return dpu_enc->dsc ? true : false; +} + int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) { struct dpu_encoder_virt *dpu_enc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 4c05fd5e9ed1..fe6b1d312a74 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -158,6 +158,13 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); +/** + * dpu_encoder_is_dsc_enabled - indicate whether dsc is enabled + * for the encoder. + * @drm_enc:Pointer to previously created drm encoder structure + */ +bool dpu_encoder_is_dsc_enabled(const struct drm_encoder *drm_enc); + /** * dpu_encoder_get_crc_values_cnt - get number of physical encoders contained * in virtual encoder that can collect CRC values diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index d0f56c5c4cce..f562beb6f797 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -102,6 +102,7 @@ static void drm_mode_to_intf_timing_params( } timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent); + timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent); /* * for DP, divide the horizonal parameters by 2 when diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 6bba531d6dc4..965692ef7892 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -163,13 +163,8 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width; display_hctl = (hsync_end_x << 16) | hsync_start_x; - /* -* DATA_HCTL_EN controls data timing which can be different from -* video timing. It is recommended to enable it for all cases, except -* if compression is enabled in 1 pixel per clock mode -*/ if (p->wide_bus_en) - intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN; data_width = p->width; @@ -229,6 +224,14 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg); DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format); if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) { + /* +* DATA_HCTL_EN controls data timing which can be different from +* video timing. It is recommended to enable it for all cases, except +* if compression is enabled in 1 pixel per clock mode +*/ + if (!(p->compression_en && !p->wide_bus_en)) + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; + DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2); DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl); DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h index 0bd57a32144a..6f4c87244f94 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h +++
Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops
On Wed, Jan 31, 2024 at 6:15 AM Joakim Bech wrote: > On Fri, Jan 12, 2024 at 05:23:07PM -0800, John Stultz wrote: > > So we need some clarity in what "restricted" really means. For > > instance, it being not cpu mappable vs other potential variations like > > being cpu mappable, but not cpu accessible. Or not cpu mappable, but > > only mappable between a set of 3 devices (Which 3 devices?! How can we > > tell?). > > > Can we flip things around? I.e., instead of saying which devices are > allowed to use the restricted buffer, can we instead say where it's not > allowed to be used? My view has been that by default the contents of the > types of buffers where talking about here is only accessible to things > running on the secure side, i.e, typically S-EL3, S-EL1 and a specific > Trusted Application running in S-EL0. I guess that serves as some kind > of baseline. ? This seems like you're suggesting enumerating badness? I'm not sure I understand the benefit of that. > From there, things turns to a more dynamic nature, where firewalls etc, > can be configured to give access to various IPs, blocks and runtimes. > > I understand that it's nice to be able to know all this from the Linux > kernel point of view, but does it have to be aware of this? What's the > major drawback if Linux doesn't know about it? Indeed, it doesn't necessarily. The idea with DMABUF heaps is it provides a name to abstract/wrap a type of constraint. So you can then allocate buffers that satisfy that constraint. Admittedly the downside with DMABUF heaps is that it has a bit of a gap in the abstraction in that we don't have a mapping of device constraints, so in Android gralloc provides a device specific usage/pipeline -> heap mapping. (Note: This I don't think is very problematic - I often use the example of fstab as device-specific config everyone is comfortable with - but I know folks would like to have something more generic) I believe Christian has previously proposed to have the devices provide something like symlinks from their sysfs nodes to the heaps the device supports, which is an interesting idea to mostly close that issue. Applications could then scan the devices in a pipeline and find the type they all support, and the specific names wouldn't matter. However, I'd expect the same hardware pipeline might support both restricted and unrestricted playback, so there would need to be some way to differentiate for the use case, so I'm not sure you can get away from some heap name to functionality mapping. My main concern with this patch series is that it seems to want to bundle all the different types of "restricted" buffers that might be possible under a single "restricted" heap name. Since we likely have devices with different security domains, thus different types of restrictions. So we may need to be able to differentiate between "secure video playback" uses and "protected device firmware" uses on the same machine. Thus, I'm not sure it's a good idea to bundle all of these under the same heap name. thanks -john
Re: [PATCH] drm/i915/gt: Restart the heartbeat timer when forcing a pulse
On 1/31/2024 10:48, Janusz Krzysztofik wrote: Hi John, On Wednesday, 10 January 2024 22:02:16 CET john.c.harri...@intel.com wrote: From: John Harrison The context persistence code does things like send super high priority heartbeat pulses to ensure any leaked context can still be pre-empted and thus isn't a total denial of service but only a minor denial of service. Unfortunately, it wasn't bothering to restart the heatbeat worker with a fresh timeout. Thus, if a persistent context happened to be closed just before the heartbeat was going to go ping anyway then the forced pulse would get a negligble execution time. And as the forced pulse is super high priority, the worker thread's next step is a reset. Which means a potentially innocent system randomly goes boom when attempting to close a context. So, force a re-schedule of the worker thread with the appropriate timeout. I haven't looked too much in heartbeat pulses code before, but I think I can understand your change. I've also got a positive opinion from Chris on it. I can provide my Ack, assuming the pre-merge failure reported by CI is not related, but could you please comment that failure first and/or ask BUG Filing to handle it so we have it cleaned up? Pretty confident the CI failure is unrelated. Not seeing how a change to the heartbeat timing of persistence context clean up could cause a HDMI test to fail to complete. However, I was really hoping for a full code review by someone who understands this code and would be able to say whether there could be unexpected side effects of the change. Or even if there is a better / safer way to fix the problem. @Andi Shyti, you were fingered as being someone who might have such knowledge. Can you comment? Thanks, John. Thanks, Janusz Signed-off-by: John Harrison --- drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c index 1a8e2b7db0138..4ae2fa0b61dd4 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c @@ -290,6 +290,9 @@ static int __intel_engine_pulse(struct intel_engine_cs *engine) heartbeat_commit(rq, ); GEM_BUG_ON(rq->sched.attr.priority < I915_PRIORITY_BARRIER); + /* Ensure the forced pulse gets a full period to execute */ + next_heartbeat(engine); + return 0; }
[PATCH v2 6/6] arm64: dts: rockchip: fix rk3399 hdmi ports node
Fix rk3399 hdmi ports node so that it matches the rockchip,dw-hdmi.yaml binding. Signed-off-by: Johan Jonker --- Changed V2: keep reg-io-width together with reg --- arch/arm64/boot/dts/rockchip/rk3399.dtsi | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index 0caa842bba0e..9d5f5b083e3c 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -2022,6 +2022,7 @@ simple-audio-card,codec { hdmi: hdmi@ff94 { compatible = "rockchip,rk3399-dw-hdmi"; reg = <0x0 0xff94 0x0 0x2>; + reg-io-width = <4>; interrupts = ; clocks = < PCLK_HDMI_CTRL>, < SCLK_HDMI_SFR>, @@ -2030,13 +2031,16 @@ hdmi: hdmi@ff94 { < PLL_VPLL>; clock-names = "iahb", "isfr", "cec", "grf", "ref"; power-domains = < RK3399_PD_HDCP>; - reg-io-width = <4>; rockchip,grf = <>; #sound-dai-cells = <0>; status = "disabled"; ports { - hdmi_in: port { + #address-cells = <1>; + #size-cells = <0>; + + hdmi_in: port@0 { + reg = <0>; #address-cells = <1>; #size-cells = <0>; @@ -2049,6 +2053,10 @@ hdmi_in_vopl: endpoint@1 { remote-endpoint = <_out_hdmi>; }; }; + + hdmi_out: port@1 { + reg = <1>; + }; }; }; -- 2.39.2
[PATCH v2 5/6] arm64: dts: rockchip: fix rk3328 hdmi ports node
Fix rk3328 hdmi ports node so that it matches the rockchip,dw-hdmi.yaml binding. Signed-off-by: Johan Jonker --- Changed V2: keep reg-io-width together with reg --- arch/arm64/boot/dts/rockchip/rk3328.dtsi | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi index fb5dcf6e9327..a73234b11ff1 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi @@ -745,11 +745,20 @@ hdmi: hdmi@ff3c { status = "disabled"; ports { - hdmi_in: port { + #address-cells = <1>; + #size-cells = <0>; + + hdmi_in: port@0 { + reg = <0>; + hdmi_in_vop: endpoint { remote-endpoint = <_out_hdmi>; }; }; + + hdmi_out: port@1 { + reg = <1>; + }; }; }; -- 2.39.2
[PATCH v2 4/6] ARM: dts: rockchip: fix rk322x hdmi ports node
Fix rk322x hdmi ports node so that it matches the rockchip,dw-hdmi.yaml binding. Signed-off-by: Johan Jonker --- Changed V2: keep reg-io-width together with reg --- arch/arm/boot/dts/rockchip/rk322x.dtsi | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/arch/arm/boot/dts/rockchip/rk322x.dtsi b/arch/arm/boot/dts/rockchip/rk322x.dtsi index 831561fc1814..96421355c274 100644 --- a/arch/arm/boot/dts/rockchip/rk322x.dtsi +++ b/arch/arm/boot/dts/rockchip/rk322x.dtsi @@ -736,14 +736,20 @@ hdmi: hdmi@200a { status = "disabled"; ports { - hdmi_in: port { - #address-cells = <1>; - #size-cells = <0>; - hdmi_in_vop: endpoint@0 { - reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + hdmi_in: port@0 { + reg = <0>; + + hdmi_in_vop: endpoint { remote-endpoint = <_out_hdmi>; }; }; + + hdmi_out: port@1 { + reg = <1>; + }; }; }; -- 2.39.2
[PATCH v2 3/6] ARM: dts: rockchip: fix rk3288 hdmi ports node
Fix rk3288 hdmi ports node so that it matches the rockchip,dw-hdmi.yaml binding with some reordering to align with the (new) documentation about property ordering. Signed-off-by: Johan Jonker --- Changed V2: keep reg-io-width together with reg reword --- arch/arm/boot/dts/rockchip/rk3288.dtsi | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/arch/arm/boot/dts/rockchip/rk3288.dtsi b/arch/arm/boot/dts/rockchip/rk3288.dtsi index ead343dc3df1..3f1d640afafa 100644 --- a/arch/arm/boot/dts/rockchip/rk3288.dtsi +++ b/arch/arm/boot/dts/rockchip/rk3288.dtsi @@ -1240,27 +1240,37 @@ hdmi: hdmi@ff98 { compatible = "rockchip,rk3288-dw-hdmi"; reg = <0x0 0xff98 0x0 0x2>; reg-io-width = <4>; - #sound-dai-cells = <0>; - rockchip,grf = <>; interrupts = ; clocks = < PCLK_HDMI_CTRL>, < SCLK_HDMI_HDCP>, < SCLK_HDMI_CEC>; clock-names = "iahb", "isfr", "cec"; power-domains = < RK3288_PD_VIO>; + rockchip,grf = <>; + #sound-dai-cells = <0>; status = "disabled"; ports { - hdmi_in: port { + #address-cells = <1>; + #size-cells = <0>; + + hdmi_in: port@0 { + reg = <0>; #address-cells = <1>; #size-cells = <0>; + hdmi_in_vopb: endpoint@0 { reg = <0>; remote-endpoint = <_out_hdmi>; }; + hdmi_in_vopl: endpoint@1 { reg = <1>; remote-endpoint = <_out_hdmi>; }; }; + + hdmi_out: port@1 { + reg = <1>; + }; }; }; -- 2.39.2
[PATCH v2 2/6] dt-bindings: display: rockchip,dw-hdmi: add power-domains property
Most Rockchip hdmi nodes are part of a power domain. Add a power-domains property and include it to the example with some reordering to align with the (new) documentation about property ordering. Signed-off-by: Johan Jonker Acked-by: Conor Dooley --- Changed V2: keep reg-io-width together with reg explain reordering --- .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml index 391c2a7e79de..af638b6c0d21 100644 --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml @@ -94,6 +94,9 @@ properties: - const: default - const: unwedge + power-domains: +maxItems: 1 + ports: $ref: /schemas/graph.yaml#/properties/ports @@ -138,16 +141,18 @@ examples: #include #include #include +#include hdmi: hdmi@ff98 { compatible = "rockchip,rk3288-dw-hdmi"; reg = <0xff98 0x2>; reg-io-width = <4>; -ddc-i2c-bus = <>; -rockchip,grf = <>; interrupts = ; clocks = < PCLK_HDMI_CTRL>, < SCLK_HDMI_HDCP>; clock-names = "iahb", "isfr"; +ddc-i2c-bus = <>; +power-domains = < RK3288_PD_VIO>; +rockchip,grf = <>; ports { #address-cells = <1>; -- 2.39.2
[PATCH v2 1/6] dt-bindings: display: rockchip: rockchip,dw-hdmi: remove port property
The hdmi-connector nodes are now functional and the new way to model hdmi ports nodes with both in and output port subnodes. Unfortunately with the conversion to YAML the old method with only an input port node was used. Later the new method was also added to the binding. A binding must be unambiguously, so remove the old port property entirely and make port@0 and port@1 a requirement as all upstream dts files are updated as well and because checking deprecated stuff is a bit pointless. Update the example to avoid use of the removed property. Signed-off-by: Johan Jonker --- Changed V2: rename title from deprecate to remove reword --- .../display/rockchip/rockchip,dw-hdmi.yaml| 24 +++ 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml index 7e59dee15a5f..391c2a7e79de 100644 --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml @@ -97,8 +97,8 @@ properties: ports: $ref: /schemas/graph.yaml#/properties/ports -patternProperties: - "^port(@0)?$": +properties: + port@0: $ref: /schemas/graph.yaml#/properties/port description: Input of the DWC HDMI TX properties: @@ -108,11 +108,14 @@ properties: description: Connection to the VOPB endpoint@1: description: Connection to the VOPL -properties: port@1: $ref: /schemas/graph.yaml#/properties/port description: Output of the DWC HDMI TX +required: + - port@0 + - port@1 + rockchip,grf: $ref: /schemas/types.yaml#/definitions/phandle description: @@ -147,7 +150,11 @@ examples: clock-names = "iahb", "isfr"; ports { -port { +#address-cells = <1>; +#size-cells = <0>; + +port@0 { +reg = <0>; #address-cells = <1>; #size-cells = <0>; @@ -155,11 +162,20 @@ examples: reg = <0>; remote-endpoint = <_out_hdmi>; }; + hdmi_in_vopl: endpoint@1 { reg = <1>; remote-endpoint = <_out_hdmi>; }; }; + +port@1 { +reg = <1>; + +hdmi_out_con: endpoint { +remote-endpoint = <_con_in>; +}; +}; }; }; -- 2.39.2
Re: Future handling of complex RGB devices on Linux
On Wed, Jan 31, 2024 at 3:42 AM Werner Sembach wrote: > > Hi, > > so I combined Hans last draft, with the discussion since then and the comments > from the OpenRGB maintainers from here > https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916 and my own experience > and came up witrh this rough updated draft for the new uapi: > > Future handling of complex RGB devices on Linux: > > Optional: Provide a basic leds-subsystem driver: > - The whole device is treated as a singular RGB led in the current leds > uapi > - Backwards compatibility > - Have something work out-of-the-box and during boot time > - The driver also registers a misc device with a singluar sysfs attribute > select_uapi > - reading this gives back "[leds] none" > - the current active uapi can be selected by writing it to that > attribute > - switching the uapi means deregistering the device from that > entirely > and registering and initializing it with the new one froms scratch > - selecting none only does the deregistering > > If the device is already reachable by userspace directly, e.g. via hidraw, the > kernel will only offer this basic implementation and a more complex driver has > to be implemented in userspace. > - This driver has to use the select_uapi attribute first and select > "none" > to avoid undefined behaviour caused by accessing the leds upai and hidraw to > control the lighting at the same time > - Question: How to best associate the select_uapi attribute to the > corresponding hidraw (or other) direct access channel? So that a userspace > driver can reliable check whether or not this has to be set. > > Devices not reachable by userspace directly, e.g. because they are controled > via > a wmi interface, can also be implemented in the new rgbledstring-subsystem > (working title) for more complex control > - a rgbledstring device provides an ioctl interface (ioctl only no r/w) > using /dev/rgbledstring0, /dev/rgbledstring1, etc. registered as a misc > chardev. > - get-device-info ioctl which returns in a single struct: > - char name[64] /* Device model name / > identifier, preferable human readable. For keyboards, if known to the driver, > physical layout (or even printed layout) should be separated here */ > - enum device_type /* e.g. keyboard, mouse, > lightbar, etc. */ > - char firmware_version_string[64] /* if known to the driver, > empty otherwise */ > - char serial_number[64]/* if known to the driver, > empty otherwise */ > - enum supported_modes[64] /* modes supported by the > firmware e.g. static/direct, breathing, scan, raindrops, etc. */ > - get-mode-info icotl, RFC here: Hans thinks it is better to have the > modes and their inputs staticly defined and have, if required, something like > breathing_clevo_1, breathing_clevo_2, breathing_tongfang_1 if the inputs vary > between vendors. I think a dynamic approach could be useful where userspace > just > queries the struct required for each individual mode. > - input: a mode from the supported_modes extracted from > get-device-info > - output: static information about the mode, e.g. > max_animation_speed, max_brightness, etc. > - output: the struct/a list of attributes and their types > required > to configure the mode > - set-mode ioctl takes a single struct: > - enum mode /* from supported_modes */ > - union data > - char raw[3072] > - > - The driver also registers a singluar sysfs attribute select_uapi > - reading this gives back "[leds] rgbledstring none" or > "[rgbledstring] none" respectifly > - Discussion question: should select_uapi instead be > use_leds_uapi > - if 1: use basic leds driver > - if 0: if device is userspace accessible no kernel driver is > active, if device ist not userspace accessible register rgbledstring (aka > implicit separation between rgbledstring and none instead of explicit one) > > Zone configuration would be seen as a subset of mode configuration, as I > suspect > not every mode needs the zone configuration even on devices that offer it? > > The most simple mode would be static/direct and the set-mode struct would look > like this: > { > enum mode, /* = static */ > { > uint8 brightness, /* global brightness, some keyboards offer this */ > uint8 color[*3] > } > } > > Question: Are there modes that have a separate setup command that is only > required once and then a continuous stream of update information? If yes, > should > we reflect that by splitting set-mode into set-mode-setup and set-mode-update > (with get-mode-info returning one struct for each)? Or should userspace
RE: Making drm_gpuvm work across gpu devices
Fixed one typo: GPU VA != GPU VA should be GPU VA can != CPU VA > -Original Message- > From: Zeng, Oak > Sent: Wednesday, January 31, 2024 3:17 PM > To: Daniel Vetter ; David Airlie > Cc: Christian König ; Thomas Hellström > ; Brost, Matthew > ; Felix Kuehling ; Welty, > Brian ; dri-devel@lists.freedesktop.org; Ghimiray, > Himal > Prasad ; Bommu, Krishnaiah > ; Gupta, saurabhg ; > Vishwanathapura, Niranjana ; intel- > x...@lists.freedesktop.org; Danilo Krummrich ; Shah, Ankur N > ; jgli...@redhat.com; rcampb...@nvidia.com; > apop...@nvidia.com > Subject: RE: Making drm_gpuvm work across gpu devices > > Hi Sima, Dave, > > I am well aware nouveau driver is not what Nvidia do with their customer. The > key argument is, can we move forward with the concept shared virtual address > space b/t CPU and GPU? This is the foundation of HMM. We already have split > address space support with other driver API. SVM, from its name, it means > shared address space. Are we allowed to implement another driver model to > allow SVM work, along with other APIs supporting split address space? Those > two > scheme can co-exist in harmony. We actually have real use cases to use both > models in one application. > > Hi Christian, Thomas, > > In your scheme, GPU VA can != CPU VA. This does introduce some flexibility. > But > this scheme alone doesn't solve the problem of the proxy process/para- > virtualization. You will still need a second mechanism to partition GPU VA > space > b/t guest process1 and guest process2 because proxy process (or the host > hypervisor whatever you call it) use one single gpu page table for all the > guest/client processes. GPU VA for different guest process can't overlap. If > this > second mechanism exist, we of course can use the same mechanism to partition > CPU VA space between guest processes as well, then we can still use shared VA > b/t CPU and GPU inside one process, but process1 and process2's address space > (for both cpu and gpu) doesn't overlap. This second mechanism is the key to > solve the proxy process problem, not the flexibility you introduced. > > In practice, your scheme also have a risk of running out of process space > because > you have to partition whole address space b/t processes. Apparently allowing > each guest process to own the whole process space and using separate GPU/CPU > page table for different processes is a better solution than using single > page table > and partition process space b/t processes. > > For Intel GPU, para-virtualization (xenGT, see https://github.com/intel/XenGT- > Preview-kernel. It is similar idea of the proxy process in Flex's email. They > are all > SW-based GPU virtualization technology) is an old project. It is now replaced > with > HW accelerated SRIOV/system virtualization. XenGT is abandoned long time ago. > So agreed your scheme add some flexibility. The question is, do we have a > valid > use case to use such flexibility? I don't see a single one ATM. > > I also pictured into how to implement your scheme. You basically rejected the > very foundation of hmm design which is shared address space b/t CPU and GPU. > In your scheme, GPU VA = CPU VA + offset. In every single place where driver > need to call hmm facilities such as hmm_range_fault, migrate_vma_setup and in > mmu notifier call back, you need to offset the GPU VA to get a CPU VA. From > application writer's perspective, whenever he want to use a CPU pointer in his > GPU program, he add to add that offset. Do you think this is awkward? > > Finally, to implement SVM, we need to implement some memory hint API which > applies to a virtual address range across all GPU devices. For example, user > would > say, for this virtual address range, I prefer the backing store memory to be > on > GPU deviceX (because user knows deviceX would use this address range much > more than other GPU devices or CPU). It doesn't make sense to me to make such > API per device based. For example, if you tell device A that the preferred > memory location is device B memory, this doesn't sounds correct to me because > in your scheme, device A is not even aware of the existence of device B. > right? > > Regards, > Oak > > -Original Message- > > From: Daniel Vetter > > Sent: Wednesday, January 31, 2024 4:15 AM > > To: David Airlie > > Cc: Zeng, Oak ; Christian König > > ; Thomas Hellström > > ; Daniel Vetter ; Brost, > > Matthew ; Felix Kuehling > > ; Welty, Brian ; dri- > > de...@lists.freedesktop.org; Ghimiray, Himal Prasad > > ; Bommu, Krishnaiah > > ; Gupta, saurabhg > ; > > Vishwanathapura, Niranjana ; intel- > > x...@lists.freedesktop.org; Danilo Krummrich ; Shah, Ankur > N > > ; jgli...@redhat.com; rcampb...@nvidia.com; > > apop...@nvidia.com > > Subject: Re: Making drm_gpuvm work across gpu devices > > > > On Wed, Jan 31, 2024 at 09:12:39AM +1000, David Airlie wrote: > > > On Wed, Jan 31, 2024 at 8:29 AM Zeng, Oak wrote: > > > > > > > > Hi Christian, > > > > > >
[PATCH] drm/i915/display: Include debugfs.h in intel_display_debugfs_params.c
Commit 8015bee0bfec ("drm/i915/display: Add framework to add parameters specific to display") added the file intel_display_debugfs_params.c, which calls the functions "debugfs_create_{bool, ulong, str}" -- all of which are defined in . The missing inclusion of this header file is breaking the ChromeOS build -- add an explicit include to fix that. Signed-off-by: Paz Zcharya --- drivers/gpu/drm/i915/display/intel_display_debugfs_params.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c index b7e68eb62452..f35718748555 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c @@ -3,6 +3,7 @@ * Copyright © 2023 Intel Corporation */ +#include #include #include -- 2.43.0.594.gd9cf4e227d-goog
RE: Making drm_gpuvm work across gpu devices
Hi Sima, Dave, I am well aware nouveau driver is not what Nvidia do with their customer. The key argument is, can we move forward with the concept shared virtual address space b/t CPU and GPU? This is the foundation of HMM. We already have split address space support with other driver API. SVM, from its name, it means shared address space. Are we allowed to implement another driver model to allow SVM work, along with other APIs supporting split address space? Those two scheme can co-exist in harmony. We actually have real use cases to use both models in one application. Hi Christian, Thomas, In your scheme, GPU VA can != GPU VA. This does introduce some flexibility. But this scheme alone doesn't solve the problem of the proxy process/para-virtualization. You will still need a second mechanism to partition GPU VA space b/t guest process1 and guest process2 because proxy process (or the host hypervisor whatever you call it) use one single gpu page table for all the guest/client processes. GPU VA for different guest process can't overlap. If this second mechanism exist, we of course can use the same mechanism to partition CPU VA space between guest processes as well, then we can still use shared VA b/t CPU and GPU inside one process, but process1 and process2's address space (for both cpu and gpu) doesn't overlap. This second mechanism is the key to solve the proxy process problem, not the flexibility you introduced. In practice, your scheme also have a risk of running out of process space because you have to partition whole address space b/t processes. Apparently allowing each guest process to own the whole process space and using separate GPU/CPU page table for different processes is a better solution than using single page table and partition process space b/t processes. For Intel GPU, para-virtualization (xenGT, see https://github.com/intel/XenGT-Preview-kernel. It is similar idea of the proxy process in Flex's email. They are all SW-based GPU virtualization technology) is an old project. It is now replaced with HW accelerated SRIOV/system virtualization. XenGT is abandoned long time ago. So agreed your scheme add some flexibility. The question is, do we have a valid use case to use such flexibility? I don't see a single one ATM. I also pictured into how to implement your scheme. You basically rejected the very foundation of hmm design which is shared address space b/t CPU and GPU. In your scheme, GPU VA = CPU VA + offset. In every single place where driver need to call hmm facilities such as hmm_range_fault, migrate_vma_setup and in mmu notifier call back, you need to offset the GPU VA to get a CPU VA. From application writer's perspective, whenever he want to use a CPU pointer in his GPU program, he add to add that offset. Do you think this is awkward? Finally, to implement SVM, we need to implement some memory hint API which applies to a virtual address range across all GPU devices. For example, user would say, for this virtual address range, I prefer the backing store memory to be on GPU deviceX (because user knows deviceX would use this address range much more than other GPU devices or CPU). It doesn't make sense to me to make such API per device based. For example, if you tell device A that the preferred memory location is device B memory, this doesn't sounds correct to me because in your scheme, device A is not even aware of the existence of device B. right? Regards, Oak > -Original Message- > From: Daniel Vetter > Sent: Wednesday, January 31, 2024 4:15 AM > To: David Airlie > Cc: Zeng, Oak ; Christian König > ; Thomas Hellström > ; Daniel Vetter ; Brost, > Matthew ; Felix Kuehling > ; Welty, Brian ; dri- > de...@lists.freedesktop.org; Ghimiray, Himal Prasad > ; Bommu, Krishnaiah > ; Gupta, saurabhg ; > Vishwanathapura, Niranjana ; intel- > x...@lists.freedesktop.org; Danilo Krummrich ; Shah, Ankur N > ; jgli...@redhat.com; rcampb...@nvidia.com; > apop...@nvidia.com > Subject: Re: Making drm_gpuvm work across gpu devices > > On Wed, Jan 31, 2024 at 09:12:39AM +1000, David Airlie wrote: > > On Wed, Jan 31, 2024 at 8:29 AM Zeng, Oak wrote: > > > > > > Hi Christian, > > > > > > > > > > > > Nvidia Nouveau driver uses exactly the same concept of SVM with HMM, > GPU address in the same process is exactly the same with CPU virtual address. > It > is already in upstream Linux kernel. We Intel just follow the same direction > for > our customers. Why we are not allowed? > > > > > > Oak, this isn't how upstream works, you don't get to appeal to > > customer or internal design. nouveau isn't "NVIDIA"'s and it certainly > > isn't something NVIDIA would ever suggest for their customers. We also > > likely wouldn't just accept NVIDIA's current solution upstream without > > some serious discussions. The implementation in nouveau was more of a > > sample HMM use case rather than a serious implementation. I suspect if > > we do get down the road of
Re: [PATCH] fbdev: Restrict FB_SH_MOBILE_LCDC to SuperH
On 1/31/24 17:08, Geert Uytterhoeven wrote: Since commit f402f7a02af6956d ("staging: board: Remove Armadillo-800-EVA board staging code"), there are no more users of the legacy SuperH Mobile LCDC framebuffer driver on Renesas ARM platforms. All former users on these platforms have been converted to the SH-Mobile DRM driver, using DT. Suggested-by: Arnd Bergmann Signed-off-by: Geert Uytterhoeven --- Commit f402f7a02af6956d is in staging-next (next-20240129 and later). applied to fbdev git tree. Thanks! Helge --- drivers/video/fbdev/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index 2d0bcc1d786e50bb..b688900bb67eed55 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -1554,7 +1554,7 @@ config FB_FSL_DIU config FB_SH_MOBILE_LCDC tristate "SuperH Mobile LCDC framebuffer support" depends on FB && HAVE_CLK && HAS_IOMEM - depends on SUPERH || ARCH_RENESAS || COMPILE_TEST + depends on SUPERH || COMPILE_TEST depends on FB_DEVICE select FB_BACKLIGHT select FB_DEFERRED_IO
Re: [PATCH 2/6] drm/nouveau/svm: remove unused but set variables
On 1/10/24 18:39, Jani Nikula wrote: Fix the W=1 warning -Wunused-but-set-variable. Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: nouv...@lists.freedesktop.org Signed-off-by: Jani Nikula Reviewed-by: Danilo Krummrich --- drivers/gpu/drm/nouveau/nouveau_svm.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index cc03e0c22ff3..4d1008915499 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -112,7 +112,7 @@ nouveau_svmm_bind(struct drm_device *dev, void *data, { struct nouveau_cli *cli = nouveau_cli(file_priv); struct drm_nouveau_svm_bind *args = data; - unsigned target, cmd, priority; + unsigned target, cmd; unsigned long addr, end; struct mm_struct *mm; @@ -136,9 +136,6 @@ nouveau_svmm_bind(struct drm_device *dev, void *data, return -EINVAL; } - priority = args->header >> NOUVEAU_SVM_BIND_PRIORITY_SHIFT; - priority &= NOUVEAU_SVM_BIND_PRIORITY_MASK; - /* FIXME support CPU target ie all target value < GPU_VRAM */ target = args->header >> NOUVEAU_SVM_BIND_TARGET_SHIFT; target &= NOUVEAU_SVM_BIND_TARGET_MASK; @@ -926,15 +923,14 @@ nouveau_pfns_map(struct nouveau_svmm *svmm, struct mm_struct *mm, unsigned long addr, u64 *pfns, unsigned long npages) { struct nouveau_pfnmap_args *args = nouveau_pfns_to_args(pfns); - int ret; args->p.addr = addr; args->p.size = npages << PAGE_SHIFT; mutex_lock(>mutex); - ret = nvif_object_ioctl(>vmm->vmm.object, args, - struct_size(args, p.phys, npages), NULL); + nvif_object_ioctl(>vmm->vmm.object, args, + struct_size(args, p.phys, npages), NULL); mutex_unlock(>mutex); }
Re: [PATCH 1/6] drm/nouveau/acr/ga102: remove unused but set variable
On 1/10/24 18:39, Jani Nikula wrote: Fix the W=1 warning -Wunused-but-set-variable. Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: nouv...@lists.freedesktop.org Signed-off-by: Jani Nikula Reviewed-by: Danilo Krummrich --- drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c index f36a359d4531..bd104a030243 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c @@ -218,7 +218,7 @@ nvkm_acr_lsfw_load_sig_image_desc_v2(struct nvkm_subdev *subdev, const struct firmware *hsbl; const struct nvfw_ls_hsbl_bin_hdr *hdr; const struct nvfw_ls_hsbl_hdr *hshdr; - u32 loc, sig, cnt, *meta; + u32 sig, cnt, *meta; ret = nvkm_firmware_load_name(subdev, path, "hs_bl_sig", ver, ); if (ret) @@ -227,7 +227,6 @@ nvkm_acr_lsfw_load_sig_image_desc_v2(struct nvkm_subdev *subdev, hdr = nvfw_ls_hsbl_bin_hdr(subdev, hsbl->data); hshdr = nvfw_ls_hsbl_hdr(subdev, hsbl->data + hdr->header_offset); meta = (u32 *)(hsbl->data + hshdr->meta_data_offset); - loc = *(u32 *)(hsbl->data + hshdr->patch_loc); sig = *(u32 *)(hsbl->data + hshdr->patch_sig); cnt = *(u32 *)(hsbl->data + hshdr->num_sig);
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Wed, Jan 31, 2024 at 06:09:04PM +0200, Ville Syrjälä wrote: > On Tue, Jan 23, 2024 at 12:28:33PM +0200, Imre Deak wrote: > > Add support for Display Port DP tunneling. For now this includes the > > support for Bandwidth Allocation Mode, leaving adding Panel Replay > > support for later. > > > > BWA allows using displays that share the same (Thunderbolt) link with > > their maximum resolution. Atm, this may not be possible due to the > > coarse granularity of partitioning the link BW among the displays on the > > link: the BW allocation policy is in a SW/FW/HW component on the link > > (on Thunderbolt it's the SW or FW Connection Manager), independent of > > the driver. This policy will set the DPRX maximum rate and lane count > > DPCD registers the GFX driver will see (0x0, 0x1, 0x02200, > > 0x02201) based on the available link BW. > > > > The granularity of the current BW allocation policy is course, based on > > the required link rate in the 1.62Gbs..8.1Gbps range and it may prevent > > using higher resolutions all together: the display connected first will > > get a share of the link BW which corresponds to its full DPRX capability > > (regardless of the actual mode it uses). A subsequent display connected > > will only get the remaining BW, which could be well below its full > > capability. > > > > BWA solves the above course granularity (reducing it to a 250Mbs..1Gps > > range) and first-come/first-served issues by letting the driver request > > the BW for each display on a link which reflects the actual modes the > > displays use. > > > > This patch adds the DRM core helper functions, while a follow-up change > > in the patchset takes them into use in the i915 driver. > > > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/display/Kconfig | 17 + > > drivers/gpu/drm/display/Makefile|2 + > > drivers/gpu/drm/display/drm_dp_tunnel.c | 1715 +++ > > include/drm/display/drm_dp.h| 60 + > > include/drm/display/drm_dp_tunnel.h | 270 > > 5 files changed, 2064 insertions(+) > > create mode 100644 drivers/gpu/drm/display/drm_dp_tunnel.c > > create mode 100644 include/drm/display/drm_dp_tunnel.h > > > > diff --git a/drivers/gpu/drm/display/Kconfig > > b/drivers/gpu/drm/display/Kconfig > > index 09712b88a5b83..b024a84b94c1c 100644 > > --- a/drivers/gpu/drm/display/Kconfig > > +++ b/drivers/gpu/drm/display/Kconfig > > @@ -17,6 +17,23 @@ config DRM_DISPLAY_DP_HELPER > > help > > DRM display helpers for DisplayPort. > > > > +config DRM_DISPLAY_DP_TUNNEL > > + bool > > + select DRM_DISPLAY_DP_HELPER > > + help > > + Enable support for DisplayPort tunnels. > > + > > +config DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > > + bool "Enable debugging the DP tunnel state" > > + depends on REF_TRACKER > > + depends on DRM_DISPLAY_DP_TUNNEL > > + depends on DEBUG_KERNEL > > + depends on EXPERT > > + help > > + Enables debugging the DP tunnel manager's status. > > + > > + If in doubt, say "N". > > It's not exactly clear what a "DP tunnel" is. > Shouldn't thunderbolt be mentioned here somewhere? The only way I'm aware of tunneling can work is through a TBT link yes, however I'm not sure if it couldn't work on any DP link, the interface - to request BW - is simply the AUX bus after all and AFAIR the standard doesn't mention TBT either (but have to reread that). The above descriptions should be extended anyway and the usual TBT scenario mentioned at least, so will do that. > > + > > config DRM_DISPLAY_HDCP_HELPER > > bool > > depends on DRM_DISPLAY_HELPER > > diff --git a/drivers/gpu/drm/display/Makefile > > b/drivers/gpu/drm/display/Makefile > > index 17ac4a1006a80..7ca61333c6696 100644 > > --- a/drivers/gpu/drm/display/Makefile > > +++ b/drivers/gpu/drm/display/Makefile > > @@ -8,6 +8,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER) += \ > > drm_dp_helper.o \ > > drm_dp_mst_topology.o \ > > drm_dsc_helper.o > > +drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_TUNNEL) += \ > > + drm_dp_tunnel.o > > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o > > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ > > drm_hdmi_helper.o \ > > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c > > b/drivers/gpu/drm/display/drm_dp_tunnel.c > > new file mode 100644 > > index 0..58f6330db7d9d > > --- /dev/null > > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > > @@ -0,0 +1,1715 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2023 Intel Corporation > > + */ > > + > > +#include > > +#include > > + > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define to_group(__private_obj) \ > > + container_of(__private_obj, struct drm_dp_tunnel_group, base) > > + > > +#define to_group_state(__private_state) \ > > + container_of(__private_state, struct
Re: [PATCH] drm/i915/gt: Restart the heartbeat timer when forcing a pulse
Hi John, On Wednesday, 10 January 2024 22:02:16 CET john.c.harri...@intel.com wrote: > From: John Harrison > > The context persistence code does things like send super high priority > heartbeat pulses to ensure any leaked context can still be pre-empted > and thus isn't a total denial of service but only a minor denial of > service. Unfortunately, it wasn't bothering to restart the heatbeat > worker with a fresh timeout. Thus, if a persistent context happened to > be closed just before the heartbeat was going to go ping anyway then > the forced pulse would get a negligble execution time. And as the > forced pulse is super high priority, the worker thread's next step is > a reset. Which means a potentially innocent system randomly goes boom > when attempting to close a context. So, force a re-schedule of the > worker thread with the appropriate timeout. I haven't looked too much in heartbeat pulses code before, but I think I can understand your change. I've also got a positive opinion from Chris on it. I can provide my Ack, assuming the pre-merge failure reported by CI is not related, but could you please comment that failure first and/or ask BUG Filing to handle it so we have it cleaned up? Thanks, Janusz > > Signed-off-by: John Harrison > --- > drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > index 1a8e2b7db0138..4ae2fa0b61dd4 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > @@ -290,6 +290,9 @@ static int __intel_engine_pulse(struct intel_engine_cs > *engine) > heartbeat_commit(rq, ); > GEM_BUG_ON(rq->sched.attr.priority < I915_PRIORITY_BARRIER); > > + /* Ensure the forced pulse gets a full period to execute */ > + next_heartbeat(engine); > + > return 0; > } > >
Re: [PATCH v3 1/2] drm/buddy: Implement tracking clear page feature
On 30/01/2024 20:30, Arunpravin Paneer Selvam wrote: Hi Matthew, On 12/21/2023 12:51 AM, Matthew Auld wrote: Hi, On 14/12/2023 13:42, Arunpravin Paneer Selvam wrote: - Add tracking clear page feature. - Driver should enable the DRM_BUDDY_CLEARED flag if it successfully clears the blocks in the free path. On the otherhand, DRM buddy marks each block as cleared. - Track the available cleared pages size - If driver requests cleared memory we prefer cleared memory but fallback to uncleared if we can't find the cleared blocks. when driver requests uncleared memory we try to use uncleared but fallback to cleared memory if necessary. - When a block gets freed we clear it and mark the freed block as cleared, when there are buddies which are cleared as well we can merge them. Otherwise, we prefer to keep the blocks as separated. I was not involved, but it looks like we have also tried enabling the clear-on-free idea for VRAM in i915 and then also tracking that in the allocator, however that work unfortunately is not upstream. The code is open source though: https://github.com/intel-gpu/intel-gpu-i915-backports/blob/backport/main/drivers/gpu/drm/i915/i915_buddy.c#L300 It looks like some of the design differences there are having two separate free lists, so mm->clean and mm->dirty (sounds reasonable to me). And also the inclusion of a de-fragmentation routine, since buddy blocks are now not always merged back, we might choose to run the defrag in some cases, which also sounds reasonable. IIRC in amdgpu userspace can control the page-size for an allocation, so perhaps you would want to run it first if the allocation fails, before trying to evict stuff? I checked the clear-on-free idea implemented in i915. In amdgpu version, we are clearing all the blocks in amdgpu free routine and DRM buddy expects only the DRM_BUDDY_CLEARED flag. Basically, we are keeping the cleared blocks ready to be allocated when the user request for the cleared memory. We observed that this improves the performance on games and resolves the stutter issues as well. I see i915 active fences part does the same job for i915. Could we move this part into i915 free routine and set the DRM_BUDDY_CLEARED flag. On de-fragmentation , I have included a function which can be called at places where we get -ENOSPC. This routine will merge back the clear and dirty blocks together to form a larger block of requested size. I am wondering where we could use this routine as for the non-contiguous memory we have the fallback method and for the contiguous memory we have the try harder method which searches through the tree. Don't you also want to call it from your vram manager when the requested page size is something large, before trying to evict stuff? That could now fail due to fragmention IIUC. Or am I misreading mdgpu_vram_mgr_new()? I agree we can have 2 lists (clear list and dirty list) and this would reduce the search iterations. But we need to handle the 2 lists design in all the functions which might require more time for testing on all platforms. Could we just go ahead with 1 list (free list) for now and I am going to take up this work as my next task. Sounds good. Thanks, Arun. v1: (Christian) - Depends on the flag check DRM_BUDDY_CLEARED, enable the block as cleared. Else, reset the clear flag for each block in the list. - For merging the 2 cleared blocks compare as below, drm_buddy_is_clear(block) != drm_buddy_is_clear(buddy) Signed-off-by: Arunpravin Paneer Selvam Suggested-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 6 +- drivers/gpu/drm/drm_buddy.c | 169 +++--- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 6 +- drivers/gpu/drm/tests/drm_buddy_test.c | 10 +- include/drm/drm_buddy.h | 18 +- 5 files changed, 168 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c index 08916538a615..d0e199cc8f17 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c @@ -556,7 +556,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man, return 0; error_free_blocks: - drm_buddy_free_list(mm, >blocks); + drm_buddy_free_list(mm, >blocks, 0); mutex_unlock(>lock); error_fini: ttm_resource_fini(man, >base); @@ -589,7 +589,7 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man, amdgpu_vram_mgr_do_reserve(man); - drm_buddy_free_list(mm, >blocks); + drm_buddy_free_list(mm, >blocks, 0); mutex_unlock(>lock); atomic64_sub(vis_usage, >vis_usage); @@ -897,7 +897,7 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev) kfree(rsv); list_for_each_entry_safe(rsv, temp, >reserved_pages, blocks) { -
Re: [PATCH v5 1/3] drm/buddy: Implement tracking clear page feature
On 30/01/2024 19:48, Arunpravin Paneer Selvam wrote: - Add tracking clear page feature. - Driver should enable the DRM_BUDDY_CLEARED flag if it successfully clears the blocks in the free path. On the otherhand, DRM buddy marks each block as cleared. - Track the available cleared pages size - If driver requests cleared memory we prefer cleared memory but fallback to uncleared if we can't find the cleared blocks. when driver requests uncleared memory we try to use uncleared but fallback to cleared memory if necessary. - When a block gets freed we clear it and mark the freed block as cleared, when there are buddies which are cleared as well we can merge them. Otherwise, we prefer to keep the blocks as separated. v1: (Christian) - Depends on the flag check DRM_BUDDY_CLEARED, enable the block as cleared. Else, reset the clear flag for each block in the list. - For merging the 2 cleared blocks compare as below, drm_buddy_is_clear(block) != drm_buddy_is_clear(buddy) Signed-off-by: Arunpravin Paneer Selvam Suggested-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 6 +- drivers/gpu/drm/drm_buddy.c | 169 +++--- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 6 +- drivers/gpu/drm/tests/drm_buddy_test.c| 10 +- include/drm/drm_buddy.h | 18 +- 5 files changed, 168 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c index 08916538a615..d0e199cc8f17 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c @@ -556,7 +556,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man, return 0; error_free_blocks: - drm_buddy_free_list(mm, >blocks); + drm_buddy_free_list(mm, >blocks, 0); mutex_unlock(>lock); error_fini: ttm_resource_fini(man, >base); @@ -589,7 +589,7 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man, amdgpu_vram_mgr_do_reserve(man); - drm_buddy_free_list(mm, >blocks); + drm_buddy_free_list(mm, >blocks, 0); mutex_unlock(>lock); atomic64_sub(vis_usage, >vis_usage); @@ -897,7 +897,7 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev) kfree(rsv); list_for_each_entry_safe(rsv, temp, >reserved_pages, blocks) { - drm_buddy_free_list(>mm, >allocated); + drm_buddy_free_list(>mm, >allocated, 0); kfree(rsv); } if (!adev->gmc.is_app_apu) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index f57e6d74fb0e..d44172f23f05 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -57,6 +57,16 @@ static void list_insert_sorted(struct drm_buddy *mm, __list_add(>link, node->link.prev, >link); } +static void clear_reset(struct drm_buddy_block *block) +{ + block->header &= ~DRM_BUDDY_HEADER_CLEAR; +} + +static void mark_cleared(struct drm_buddy_block *block) +{ + block->header |= DRM_BUDDY_HEADER_CLEAR; +} + static void mark_allocated(struct drm_buddy_block *block) { block->header &= ~DRM_BUDDY_HEADER_STATE; @@ -223,6 +233,12 @@ static int split_block(struct drm_buddy *mm, mark_free(mm, block->left); mark_free(mm, block->right); + if (drm_buddy_block_is_clear(block)) { + mark_cleared(block->left); + mark_cleared(block->right); + clear_reset(block); + } + mark_split(block); return 0; @@ -273,6 +289,13 @@ static void __drm_buddy_free(struct drm_buddy *mm, if (!drm_buddy_block_is_free(buddy)) break; + if (drm_buddy_block_is_clear(block) != + drm_buddy_block_is_clear(buddy)) + break; + + if (drm_buddy_block_is_clear(block)) + mark_cleared(parent); + list_del(>link); drm_block_free(mm, block); @@ -295,6 +318,9 @@ void drm_buddy_free_block(struct drm_buddy *mm, { BUG_ON(!drm_buddy_block_is_allocated(block)); mm->avail += drm_buddy_block_size(mm, block); + if (drm_buddy_block_is_clear(block)) + mm->clear_avail += drm_buddy_block_size(mm, block); + __drm_buddy_free(mm, block); } EXPORT_SYMBOL(drm_buddy_free_block); @@ -305,10 +331,20 @@ EXPORT_SYMBOL(drm_buddy_free_block); * @mm: DRM buddy manager * @objects: input list head to free blocks */ -void drm_buddy_free_list(struct drm_buddy *mm, struct list_head *objects) +void drm_buddy_free_list(struct drm_buddy *mm, +struct list_head *objects, +unsigned long flags) { struct drm_buddy_block *block, *on; + if (flags & DRM_BUDDY_CLEARED) { + list_for_each_entry(block, objects, link) +
Re: [PATCH] drm/imagination: On device loss, handle unplug after critical section
On 25/01/2024 18:44, Daniel Vetter wrote: > On Tue, Jan 23, 2024 at 01:04:24PM +, Matt Coster wrote: >> From: Donald Robson >> >> When the kernel driver 'loses' the device, for instance if the firmware >> stops communicating, the driver calls drm_dev_unplug(). This is >> currently done inside the drm_dev_enter() critical section, which isn't >> permitted. In addition, the bool that marks the device as lost is not >> atomic or protected by a lock. >> >> This fix replaces the bool with an atomic that also acts as a mechanism >> to ensure the device is unplugged after drm_dev_exit(), preventing a >> concurrent call to drm_dev_enter() from succeeding in a race between it >> and drm_dev_unplug(). > > Uh ... atomic_t does not make locking. > > From a quick look this entire thing smells a bit like bad design overall, > and my gut feeling is that you probably want to rip out pvr_dev->lost > outright. Or alternatively, explain what exactly this does beyond > drm_dev_enter/exit, and then probably add that functionality there instead > of hand-roll lockless trickery in drivers. > > From a quick look keeping track of where you realize the device is lost > and then calling drm_dev_unplug after the drm_dev_exit is probably the > clean solution. That also means the drm_dev_unplug() is not delayed due to > a drm_dev_enter/exit section on a different thread, which is probably a > good thing. > > Cheers, Sima Hi Sima, Thanks for taking the time to look over this patch. This was the last piece of work Donald did for us at Imagination but he never got a chance to send it out himself. I'll put it on my list to try a new, more minimal, approach before sending a v2. Your suggestion sounds very promising – that's probably the first thing I'll try. Cheers, Matt >> Reported-by: Steven Price >> Closes: >> https://lore.kernel.org/dri-devel/1b957ca4-71cf-42fd-ac81-1920592b9...@arm.com/ >> Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and >> META FW support") >> Signed-off-by: Donald Robson >> Signed-off-by: Matt Coster >> --- >> drivers/gpu/drm/imagination/pvr_ccb.c | 2 +- >> drivers/gpu/drm/imagination/pvr_device.c | 98 +- >> drivers/gpu/drm/imagination/pvr_device.h | 72 +--- >> drivers/gpu/drm/imagination/pvr_drv.c | 87 ++- >> drivers/gpu/drm/imagination/pvr_fw.c | 12 +-- >> drivers/gpu/drm/imagination/pvr_fw_trace.c | 4 +- >> drivers/gpu/drm/imagination/pvr_mmu.c | 20 ++--- >> drivers/gpu/drm/imagination/pvr_power.c| 42 +++--- >> drivers/gpu/drm/imagination/pvr_power.h| 2 - >> 9 files changed, 237 insertions(+), 102 deletions(-) >> >> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c >> b/drivers/gpu/drm/imagination/pvr_ccb.c >> index 4deeac7ed40a..1fe64adc0c2c 100644 >> --- a/drivers/gpu/drm/imagination/pvr_ccb.c >> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c >> @@ -247,7 +247,7 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device >> *pvr_dev, >> u32 old_write_offset; >> u32 new_write_offset; >> >> -WARN_ON(pvr_dev->lost); >> +WARN_ON(pvr_device_is_lost(pvr_dev)); >> >> mutex_lock(_ccb->lock); >> >> diff --git a/drivers/gpu/drm/imagination/pvr_device.c >> b/drivers/gpu/drm/imagination/pvr_device.c >> index 1704c0268589..397491375b7d 100644 >> --- a/drivers/gpu/drm/imagination/pvr_device.c >> +++ b/drivers/gpu/drm/imagination/pvr_device.c >> @@ -6,14 +6,15 @@ >> >> #include "pvr_fw.h" >> #include "pvr_params.h" >> -#include "pvr_power.h" >> #include "pvr_queue.h" >> #include "pvr_rogue_cr_defs.h" >> #include "pvr_stream.h" >> #include "pvr_vm.h" >> >> +#include >> #include >> >> +#include >> #include >> #include >> #include >> @@ -556,6 +557,101 @@ pvr_device_fini(struct pvr_device *pvr_dev) >> pvr_device_gpu_fini(pvr_dev); >> } >> >> +/** >> + * pvr_device_enter() - Try to enter device critical section. >> + * @pvr_dev: Target PowerVR device. >> + * @idx: Pointer to index that will be passed to the matching >> pvr_device_exit(). >> + * >> + * Use this in place of drm_dev_enter() within this driver. >> + * >> + * Returns: >> + * * %true if the critical section was entered, or >> + * * %false otherwise. >> + */ >> +bool pvr_device_enter(struct pvr_device *pvr_dev, int *idx) >> +{ >> +const enum pvr_device_state old_state = >> +atomic_cmpxchg(_dev->state, >> + PVR_DEVICE_STATE_PRESENT, >> + PVR_DEVICE_STATE_ENTERED); >> + >> +switch (old_state) { >> +case PVR_DEVICE_STATE_PRESENT: >> +case PVR_DEVICE_STATE_ENTERED: >> +return drm_dev_enter(from_pvr_device(pvr_dev), idx); >> + >> +case PVR_DEVICE_STATE_LOST: >> +case PVR_DEVICE_STATE_LOST_UNPLUGGED: >> +WARN_ONCE(1, "Attempt to use GPU after becoming lost."); >> +break; >> +} >> + >> +return false; >> +} >> + >> +/** >> + * pvr_device_exit() -
Re: [PATCH] drm/sched: Add Matthew Brost to maintainers
On Tue, Jan 30, 2024 at 07:03:02PM -0800, Matthew Brost wrote: > Add Matthew Brost to DRM scheduler maintainers. > > Cc: Luben Tuikov > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: Christian König > Signed-off-by: Matthew Brost Definitely need more people taking care of drm/sched, so thanks for volunteering! Acked-by: Daniel Vetter I think this also needs an ack from Luben and Christian. And you also need drm-misc commit rights first, or it's going to be a bit tricky to take care of maintainer duties for merging patches. But since your sched patches now have landed in upstream this should be just a formality. Cheers, Sima > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5c00fad59e91..e968d68a96c8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7308,6 +7308,7 @@ F: drivers/gpu/drm/xlnx/ > > DRM GPU SCHEDULER > M: Luben Tuikov > +M: Matthew Brost > L: dri-devel@lists.freedesktop.org > S: Maintained > T: git git://anongit.freedesktop.org/drm/drm-misc > -- > 2.34.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [linux][PATCH v5 2/3] dt-bindings: atmel,hlcdc: convert pwm bindings to json-schema
On Wed, Jan 31, 2024 at 09:05:22AM +0530, Dharma Balasubiramani wrote: > Convert device tree bindings for Atmel's HLCDC PWM controller to YAML > format. > > Signed-off-by: Dharma Balasubiramani > Reviewed-by: Conor Dooley Same comment about the examples here FWIW. > +examples: > + - | > +pwm { > + compatible = "atmel,hlcdc-pwm"; > + pinctrl-names = "default"; > + pinctrl-0 = <_lcd_pwm>; > + #pwm-cells = <3>; > +}; signature.asc Description: PGP signature
Re: [PATCH 4/5] drm/msm/dp: Try looking for link-frequencies into the port@0's endpoint first
On Wed, 31 Jan 2024 at 19:04, Abel Vesa wrote: > > On 24-01-29 17:08:29, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 15:19, Abel Vesa wrote: > > > > > > From: Abhinav Kumar > > > > > > On platforms where the endpoint used is on port@0, looking for port@1 > > > instead results in just ignoring the max link-frequencies altogether. > > > Look at port@0 first, then, if not found, look for port@1. > > > > NAK. Platforms do not "use port@0". It is for the connection between > > DPU and DP, while the link-frequencies property is for the link > > between DP controller and the actual display. > > I messed up. This patch is not needed, plus the author is wrong. > > Will drop in the next version. > > Sorry about that. No problem, don't worry. > > > > > > > > > Signed-off-by: Abhinav Kumar > > > Signed-off-by: Abel Vesa > > > --- > > > drivers/gpu/drm/msm/dp/dp_parser.c | 6 +- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c > > > b/drivers/gpu/drm/msm/dp/dp_parser.c > > > index 7032dcc8842b..eec5b8b83f4b 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_parser.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c > > > @@ -97,7 +97,11 @@ static u32 dp_parser_link_frequencies(struct > > > device_node *of_node) > > > u64 frequency = 0; > > > int cnt; > > > > > > - endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* > > > port@1 */ > > > + endpoint = of_graph_get_endpoint_by_regs(of_node, 0, 0); /* > > > port@0 */ > > > + > > > + if (!endpoint) > > > + endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); > > > /* port@1 */ > > > + > > > if (!endpoint) > > > return 0; > > > > > > > > > -- > > > 2.34.1 > > > > > > > > > -- > > With best wishes > > Dmitry -- With best wishes Dmitry
Re: [PATCH 4/5] drm/msm/dp: Try looking for link-frequencies into the port@0's endpoint first
On 24-01-29 17:08:29, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 15:19, Abel Vesa wrote: > > > > From: Abhinav Kumar > > > > On platforms where the endpoint used is on port@0, looking for port@1 > > instead results in just ignoring the max link-frequencies altogether. > > Look at port@0 first, then, if not found, look for port@1. > > NAK. Platforms do not "use port@0". It is for the connection between > DPU and DP, while the link-frequencies property is for the link > between DP controller and the actual display. I messed up. This patch is not needed, plus the author is wrong. Will drop in the next version. Sorry about that. > > > > > Signed-off-by: Abhinav Kumar > > Signed-off-by: Abel Vesa > > --- > > drivers/gpu/drm/msm/dp/dp_parser.c | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c > > b/drivers/gpu/drm/msm/dp/dp_parser.c > > index 7032dcc8842b..eec5b8b83f4b 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_parser.c > > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c > > @@ -97,7 +97,11 @@ static u32 dp_parser_link_frequencies(struct device_node > > *of_node) > > u64 frequency = 0; > > int cnt; > > > > - endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 > > */ > > + endpoint = of_graph_get_endpoint_by_regs(of_node, 0, 0); /* port@0 > > */ > > + > > + if (!endpoint) > > + endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* > > port@1 */ > > + > > if (!endpoint) > > return 0; > > > > > > -- > > 2.34.1 > > > > > -- > With best wishes > Dmitry
Re: [linux][PATCH v5 1/3] dt-bindings: display: convert Atmel's HLCDC to DT schema
On Wed, Jan 31, 2024 at 09:05:21AM +0530, Dharma Balasubiramani wrote: > Convert the existing DT binding to DT schema of the Atmel's HLCDC display > controller. I feel like I recall a request to only have a complete example in the mfd binding but nowhere else. Otherwise, Reviewed-by: Conor Dooley Cheers, Conor. > +examples: > + - | > +display-controller { > + compatible = "atmel,hlcdc-display-controller"; > + pinctrl-names = "default"; > + pinctrl-0 = <_lcd_base _lcd_rgb565>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > +#address-cells = <1>; > +#size-cells = <0>; > +reg = <0>; > + > +hlcdc_panel_output: endpoint@0 { > + reg = <0>; > + bus-width = <16>; > + remote-endpoint = <_input>; > +}; > + }; > +}; signature.asc Description: PGP signature
Re: [PATCH 18/19] drm/i915/dp: Suspend/resume DP tunnels
On Wed, Jan 31, 2024 at 06:18:22PM +0200, Ville Syrjälä wrote: > On Tue, Jan 23, 2024 at 12:28:49PM +0200, Imre Deak wrote: > > Suspend and resume DP tunnels during system suspend/resume, disabling > > the BW allocation mode during suspend, re-enabling it after resume. This > > reflects the link's BW management component (Thunderbolt CM) disabling > > BWA during suspend. Before any BW requests the driver must read the > > sink's DPRX capabilities (since the BW manager requires this > > information, so snoops for it on AUX), so ensure this read takes place. > > Isn't that going to screw up the age old problem of .compute_config() > potentially failing during the resume modeset if we no longer have > the same amount of bandwidth available as we had before suspend? > So far we've been getting away with this exactly by not updating > the dpcd stuff before the modeset during resume. Right, in the case where this would be a problem (so not counting where the caps haven't been read out yet and so we update here intel_dp->dpcd), the caps in intel_dp->dpcd will be preserved, not actually updated with the read-out values, see intel_dp_tunnel_resume() in patch 11. The same goes for the tunnel (group) BW: it will not be updated during resume (by way of the connector/tunnel detection being blocked during the restore modeset), so the restore modeset should see the same amount of BW as there was during suspend. > > > > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 16 +++- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index 8ebfb039000f6..bc138a54f8d7b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -36,6 +36,7 @@ > > #include > > > > #include > > +#include > > #include > > #include > > #include > > @@ -3320,18 +3321,21 @@ void intel_dp_sync_state(struct intel_encoder > > *encoder, > > const struct intel_crtc_state *crtc_state) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > - > > - if (!crtc_state) > > - return; > > + bool dpcd_updated = false; > > > > /* > > * Don't clobber DPCD if it's been already read out during output > > * setup (eDP) or detect. > > */ > > - if (intel_dp->dpcd[DP_DPCD_REV] == 0) > > + if (crtc_state && intel_dp->dpcd[DP_DPCD_REV] == 0) { > > intel_dp_get_dpcd(intel_dp); > > + dpcd_updated = true; > > + } > > > > - intel_dp_reset_max_link_params(intel_dp); > > + intel_dp_tunnel_resume(intel_dp, dpcd_updated); > > + > > + if (crtc_state) > > + intel_dp_reset_max_link_params(intel_dp); > > } > > > > bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > > @@ -5973,6 +5977,8 @@ void intel_dp_encoder_suspend(struct intel_encoder > > *intel_encoder) > > struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder); > > > > intel_pps_vdd_off_sync(intel_dp); > > + > > + intel_dp_tunnel_suspend(intel_dp); > > } > > > > void intel_dp_encoder_shutdown(struct intel_encoder *intel_encoder) > > -- > > 2.39.2 > > -- > Ville Syrjälä > Intel
Re: [linux][PATCH v5 3/3] dt-bindings: mfd: atmel,hlcdc: Convert to DT schema format
On Wed, Jan 31, 2024 at 09:05:23AM +0530, Dharma Balasubiramani wrote: > Convert the atmel,hlcdc binding to DT schema format. > > Align clocks and clock-names properties to clearly indicate that the LCD > controller expects lvds_pll_clk when interfaced with the lvds display. This > alignment with the specific hardware requirements ensures accurate device tree > configuration for systems utilizing the HLCDC IP. > > Signed-off-by: Dharma Balasubiramani Reviewed-by: Conor Dooley Thanks, Conor. signature.asc Description: PGP signature
Re: [PATCH] fbdev: Restrict FB_SH_MOBILE_LCDC to SuperH
On Wed, Jan 31, 2024 at 05:08:23PM +0100, Geert Uytterhoeven wrote: > Since commit f402f7a02af6956d ("staging: board: Remove Armadillo-800-EVA > board staging code"), there are no more users of the legacy SuperH > Mobile LCDC framebuffer driver on Renesas ARM platforms. All former > users on these platforms have been converted to the SH-Mobile DRM > driver, using DT. > > Suggested-by: Arnd Bergmann > Signed-off-by: Geert Uytterhoeven Reviewed-by: Wolfram Sang signature.asc Description: PGP signature
Re: 回复: [v3 4/6] drm/vs: Add KMS crtc
On Wed, Jan 31, 2024 at 08:57:21AM +, Keith Zhao wrote: > > > +static const struct vs_dc_info dc8200_info = { > > > + .name = "DC8200", > > > + .panel_num = 2, > > > + .plane_num = 8, > > > + .planes = dc_hw_planes_rev0, > > > + .layer_num = 6, > > > + .max_bpc= 10, > > > + .color_formats = DRM_COLOR_FORMAT_RGB444 | > > > + DRM_COLOR_FORMAT_YCBCR444 | > > > + DRM_COLOR_FORMAT_YCBCR422 | > > > + DRM_COLOR_FORMAT_YCBCR420, > > > + .gamma_size = GAMMA_EX_SIZE, > > > + .gamma_bits = 12, > > > + .pitch_alignment= 128, > > > + .pipe_sync = false, > > > + .background = true, > > > + .panel_sync = true, > > > + .cap_dec= true, > > > +}; > > > > I really think that entire thing is to workaround a suboptimal device tree > > binding. > > You should have two CRTCs in the device tree, you'll probe twice, and you > > won't > > get to do that whole dance. > > > Hi Maxime: > I tried to modify it according to this idea Found it too difficult In terms > of hardware, > the two crtc designs are too close to separate, and they are even designed > into the same reg with different bits representing crtc0 and crtc1. > It seems not easy to described the 2 ctrc hardware by 2 device nodes > > The idea is to avoid a whole dance > I don't know if I understand correctly about whole dance. > Is it means I create 2 ctrc and 8 plane in the dc_bind? It looks like you just sent the same mail? Maxime signature.asc Description: PGP signature
Re: [PATCH 18/19] drm/i915/dp: Suspend/resume DP tunnels
On Tue, Jan 23, 2024 at 12:28:49PM +0200, Imre Deak wrote: > Suspend and resume DP tunnels during system suspend/resume, disabling > the BW allocation mode during suspend, re-enabling it after resume. This > reflects the link's BW management component (Thunderbolt CM) disabling > BWA during suspend. Before any BW requests the driver must read the > sink's DPRX capabilities (since the BW manager requires this > information, so snoops for it on AUX), so ensure this read takes place. Isn't that going to screw up the age old problem of .compute_config() potentially failing during the resume modeset if we no longer have the same amount of bandwidth available as we had before suspend? So far we've been getting away with this exactly by not updating the dpcd stuff before the modeset during resume. > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/display/intel_dp.c | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 8ebfb039000f6..bc138a54f8d7b 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -36,6 +36,7 @@ > #include > > #include > +#include > #include > #include > #include > @@ -3320,18 +3321,21 @@ void intel_dp_sync_state(struct intel_encoder > *encoder, >const struct intel_crtc_state *crtc_state) > { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - > - if (!crtc_state) > - return; > + bool dpcd_updated = false; > > /* >* Don't clobber DPCD if it's been already read out during output >* setup (eDP) or detect. >*/ > - if (intel_dp->dpcd[DP_DPCD_REV] == 0) > + if (crtc_state && intel_dp->dpcd[DP_DPCD_REV] == 0) { > intel_dp_get_dpcd(intel_dp); > + dpcd_updated = true; > + } > > - intel_dp_reset_max_link_params(intel_dp); > + intel_dp_tunnel_resume(intel_dp, dpcd_updated); > + > + if (crtc_state) > + intel_dp_reset_max_link_params(intel_dp); > } > > bool intel_dp_initial_fastset_check(struct intel_encoder *encoder, > @@ -5973,6 +5977,8 @@ void intel_dp_encoder_suspend(struct intel_encoder > *intel_encoder) > struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder); > > intel_pps_vdd_off_sync(intel_dp); > + > + intel_dp_tunnel_suspend(intel_dp); > } > > void intel_dp_encoder_shutdown(struct intel_encoder *intel_encoder) > -- > 2.39.2 -- Ville Syrjälä Intel
Re: [PATCH] fbdev: Restrict FB_SH_MOBILE_LCDC to SuperH
Geert Uytterhoeven writes: Hello Geert, > Since commit f402f7a02af6956d ("staging: board: Remove Armadillo-800-EVA > board staging code"), there are no more users of the legacy SuperH > Mobile LCDC framebuffer driver on Renesas ARM platforms. All former > users on these platforms have been converted to the SH-Mobile DRM > driver, using DT. > > Suggested-by: Arnd Bergmann > Signed-off-by: Geert Uytterhoeven > --- > Commit f402f7a02af6956d is in staging-next (next-20240129 and later). > --- Makes senes to me. Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
RE: [PATCH] backlight: mp3309c: Use pwm_apply_might_sleep()
Hi Sean, I've just tested your change on my board that uses the mp3309c. All ok, thanks! ... > Subject: [PATCH] backlight: mp3309c: Use pwm_apply_might_sleep() > > pwm_apply_state() is deprecated since commit c748a6d77c06a ("pwm: > Rename > pwm_apply_state() to pwm_apply_might_sleep()"). This is the final user in the > tree. > > Signed-off-by: Sean Young > --- Tested-by: Flavio Suligoi
[PATCH] fbdev: Restrict FB_SH_MOBILE_LCDC to SuperH
Since commit f402f7a02af6956d ("staging: board: Remove Armadillo-800-EVA board staging code"), there are no more users of the legacy SuperH Mobile LCDC framebuffer driver on Renesas ARM platforms. All former users on these platforms have been converted to the SH-Mobile DRM driver, using DT. Suggested-by: Arnd Bergmann Signed-off-by: Geert Uytterhoeven --- Commit f402f7a02af6956d is in staging-next (next-20240129 and later). --- drivers/video/fbdev/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index 2d0bcc1d786e50bb..b688900bb67eed55 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -1554,7 +1554,7 @@ config FB_FSL_DIU config FB_SH_MOBILE_LCDC tristate "SuperH Mobile LCDC framebuffer support" depends on FB && HAVE_CLK && HAS_IOMEM - depends on SUPERH || ARCH_RENESAS || COMPILE_TEST + depends on SUPERH || COMPILE_TEST depends on FB_DEVICE select FB_BACKLIGHT select FB_DEFERRED_IO -- 2.34.1
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Tue, Jan 23, 2024 at 12:28:33PM +0200, Imre Deak wrote: > Add support for Display Port DP tunneling. For now this includes the > support for Bandwidth Allocation Mode, leaving adding Panel Replay > support for later. > > BWA allows using displays that share the same (Thunderbolt) link with > their maximum resolution. Atm, this may not be possible due to the > coarse granularity of partitioning the link BW among the displays on the > link: the BW allocation policy is in a SW/FW/HW component on the link > (on Thunderbolt it's the SW or FW Connection Manager), independent of > the driver. This policy will set the DPRX maximum rate and lane count > DPCD registers the GFX driver will see (0x0, 0x1, 0x02200, > 0x02201) based on the available link BW. > > The granularity of the current BW allocation policy is course, based on > the required link rate in the 1.62Gbs..8.1Gbps range and it may prevent > using higher resolutions all together: the display connected first will > get a share of the link BW which corresponds to its full DPRX capability > (regardless of the actual mode it uses). A subsequent display connected > will only get the remaining BW, which could be well below its full > capability. > > BWA solves the above course granularity (reducing it to a 250Mbs..1Gps > range) and first-come/first-served issues by letting the driver request > the BW for each display on a link which reflects the actual modes the > displays use. > > This patch adds the DRM core helper functions, while a follow-up change > in the patchset takes them into use in the i915 driver. > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/display/Kconfig | 17 + > drivers/gpu/drm/display/Makefile|2 + > drivers/gpu/drm/display/drm_dp_tunnel.c | 1715 +++ > include/drm/display/drm_dp.h| 60 + > include/drm/display/drm_dp_tunnel.h | 270 > 5 files changed, 2064 insertions(+) > create mode 100644 drivers/gpu/drm/display/drm_dp_tunnel.c > create mode 100644 include/drm/display/drm_dp_tunnel.h > > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig > index 09712b88a5b83..b024a84b94c1c 100644 > --- a/drivers/gpu/drm/display/Kconfig > +++ b/drivers/gpu/drm/display/Kconfig > @@ -17,6 +17,23 @@ config DRM_DISPLAY_DP_HELPER > help > DRM display helpers for DisplayPort. > > +config DRM_DISPLAY_DP_TUNNEL > + bool > + select DRM_DISPLAY_DP_HELPER > + help > + Enable support for DisplayPort tunnels. > + > +config DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > + bool "Enable debugging the DP tunnel state" > + depends on REF_TRACKER > + depends on DRM_DISPLAY_DP_TUNNEL > + depends on DEBUG_KERNEL > + depends on EXPERT > + help > + Enables debugging the DP tunnel manager's status. > + > + If in doubt, say "N". It's not exactly clear what a "DP tunnel" is. Shouldn't thunderbolt be mentioned here somewhere? > + > config DRM_DISPLAY_HDCP_HELPER > bool > depends on DRM_DISPLAY_HELPER > diff --git a/drivers/gpu/drm/display/Makefile > b/drivers/gpu/drm/display/Makefile > index 17ac4a1006a80..7ca61333c6696 100644 > --- a/drivers/gpu/drm/display/Makefile > +++ b/drivers/gpu/drm/display/Makefile > @@ -8,6 +8,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER) += \ > drm_dp_helper.o \ > drm_dp_mst_topology.o \ > drm_dsc_helper.o > +drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_TUNNEL) += \ > + drm_dp_tunnel.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ > drm_hdmi_helper.o \ > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c > b/drivers/gpu/drm/display/drm_dp_tunnel.c > new file mode 100644 > index 0..58f6330db7d9d > --- /dev/null > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -0,0 +1,1715 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2023 Intel Corporation > + */ > + > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#define to_group(__private_obj) \ > + container_of(__private_obj, struct drm_dp_tunnel_group, base) > + > +#define to_group_state(__private_state) \ > + container_of(__private_state, struct drm_dp_tunnel_group_state, base) > + > +#define is_dp_tunnel_private_obj(__obj) \ > + ((__obj)->funcs == _group_funcs) > + > +#define for_each_new_group_in_state(__state, __new_group_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_private_objs; \ > + (__i)++) \ > + for_each_if ((__state)->private_objs[__i].ptr && \ > + > is_dp_tunnel_private_obj((__state)->private_objs[__i].ptr) && \ > + ((__new_group_state) = \ > + > to_group_state((__state)->private_objs[__i].new_state), 1)) > + > +#define
Re: [drm-drm-misc:drm-misc-next v2] dt-bindings: nt35510: document 'port' property
On Wed, Jan 31, 2024 at 10:28:44AM +0100, Dario Binacchi wrote: > Allow 'port' property (coming from panel-common.yaml) to be used in DTS: > > st/stm32f769-disco-mb1166-reva09.dtb: panel@0: 'port' does not match any of > the regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Dario Binacchi > Cc: Alexandre Torgue Acked-by: Conor Dooley > > --- > > Changes in v2: > - Rework the patch to drop errors found by command > 'make DT_CHECKER_FLAGS=-m dt_binding_check'. > > .../devicetree/bindings/display/panel/novatek,nt35510.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml > b/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml > index a4afaff483b7..91921f4b0e5f 100644 > --- a/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml > +++ b/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml > @@ -31,6 +31,7 @@ properties: >vddi-supply: > description: regulator that supplies the vddi voltage >backlight: true > + port: true > > required: >- compatible > -- > 2.43.0 > signature.asc Description: PGP signature
Re: [PATCH] drm/tegra: Remove of_dma_configure() from host1x_device_add()
On Tue, Jan 30, 2024 at 09:55:18PM +, Jon Hunter wrote: > > On 30/01/2024 16:15, Jason Gunthorpe wrote: > > This was added in commit c95469aa5a18 ("gpu: host1x: Set DMA ops on device > > creation") with the note: > > > > Currently host1x-instanciated devices have their dma_ops left to NULL, > > which makes any DMA operation (like buffer import) on ARM64 fallback > > to the dummy_dma_ops and fail with an error. > > > > Since commit 14891af3799e ("iommu: Move the iommu driver sysfs setup into > > iommu_init/deinit_device()") this call now fails because the struct device > > is not fully configured enough to setup the sysfs and we now catch that > > error. > > > > This failure means the DMA ops are no longer set during this failing call. Looking at it more it seems the arch dma ops are setup still, we ignore the failure on multiple levels :( > > It seems this is no longer a problem because > > commit 07397df29e57 ("dma-mapping: move dma configuration to bus > > infrastructure") added another call to of_dma_configure() inside the > > bus_type->dma_configure() callback. > > > > So long as a driver is probed the to the device it will have DMA properly > > setup in the ordinary way. > > > > Remove the unnecessary call which also removes the new long print: > > > > [1.24] host1x drm: iommu configuration for device failed with > > -ENOENT > > > > Reported-by: Diogo Ivo > > Closes: > > https://lore.kernel.org/all/bbmhcoghrprmbdibnjum6lefix2eoquxrde7wyqeulm4xabmlm@b6jy32saugqh/ > > Reported-by: Jon Hunter > > Closes: > > https://lore.kernel.org/all/b0334c5e-3a6c-4b58-b525-e72bed889...@nvidia.com/ > > Signed-off-by: Jason Gunthorpe > > --- > > drivers/gpu/host1x/bus.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c > > index 84d042796d2e66..61214d35cadc34 100644 > > --- a/drivers/gpu/host1x/bus.c > > +++ b/drivers/gpu/host1x/bus.c > > @@ -458,8 +458,6 @@ static int host1x_device_add(struct host1x *host1x, > > device->dev.bus = _bus_type; > > device->dev.parent = host1x->dev; > > - of_dma_configure(>dev, host1x->dev->of_node, true); > > - > > device->dev.dma_parms = >dma_parms; > > dma_set_max_seg_size(>dev, UINT_MAX); > > > In my case the warning is coming from the of_dma_configure_id() in > drivers/gpu/host1x/context.c. So with the above change I am still seeing the > warning. You mean this sequence? err = device_add(>dev); if (err) { dev_err(host1x->dev, "could not add context device %d: %d\n", i, err); put_device(>dev); goto unreg_devices; } err = of_dma_configure_id(>dev, node, true, ); if (err) { dev_err(host1x->dev, "IOMMU configuration failed for context device %d: %d\n", i, err); device_unregister(>dev); goto unreg_devices; } I didn't seem an obvious place that this would get fixed up later? device_add() was done before so the iommu_device_link() shouldn't be failing? Are you hitting a duplicate link (ie remove the nowarn from iommu_device_link()) Jason
[PATCH v3] drm/msm/gem: Fix double resv lock aquire
From: Rob Clark Since commit 79e2cf2e7a19 ("drm/gem: Take reservation lock for vmap/vunmap operations"), the resv lock is already held in the prime vmap path, so don't try to grab it again. v2: This applies to vunmap path as well v3: Fix fixes commit Fixes: 79e2cf2e7a19 ("drm/gem: Take reservation lock for vmap/vunmap operations") Signed-off-by: Rob Clark Acked-by: Christian König --- drivers/gpu/drm/msm/msm_gem_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c index 5f68e31a3e4e..0915f3b68752 100644 --- a/drivers/gpu/drm/msm/msm_gem_prime.c +++ b/drivers/gpu/drm/msm/msm_gem_prime.c @@ -26,7 +26,7 @@ int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map) { void *vaddr; - vaddr = msm_gem_get_vaddr(obj); + vaddr = msm_gem_get_vaddr_locked(obj); if (IS_ERR(vaddr)) return PTR_ERR(vaddr); iosys_map_set_vaddr(map, vaddr); @@ -36,7 +36,7 @@ int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map) void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map) { - msm_gem_put_vaddr(obj); + msm_gem_put_vaddr_locked(obj); } struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev, -- 2.43.0
Re: [PATCH] drm/xe/display: Fix memleak in display initialization
On Wed, 31 Jan 2024, Lucas De Marchi wrote: > +Jani > > On Fri, Jan 26, 2024 at 11:34:53PM +0800, wangxiaoming321 wrote: >>intel_power_domains_init has been called twice in xe_device_probe: >>xe_device_probe -> xe_display_init_nommio -> intel_power_domains_init(xe) >>xe_device_probe -> xe_display_init_noirq -> intel_display_driver_probe_noirq >>-> intel_power_domains_init(i915) > > ok, once upon a time intel_power_domains_init() was called by the driver > initialization code and not initialized inside the display. I think. > Now it's part of the display probe and we never updated the xe side. > >> >>It needs remove one to avoid power_domains->power_wells double malloc. >> >>unreferenced object 0x88811150ee00 (size 512): >> comm "systemd-udevd", pid 506, jiffies 4294674198 (age 3605.560s) >> hex dump (first 32 bytes): >>10 b4 9d a0 ff ff ff ff ff ff ff ff ff ff ff ff >>ff ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 >> backtrace: >>[] __kmem_cache_alloc_node+0x1c1/0x2b0 >>[] __kmalloc+0x52/0x150 >>[] __set_power_wells+0xc3/0x360 [xe] >>[] xe_display_init_nommio+0x4c/0x70 [xe] >>[] xe_device_probe+0x3c/0x5a0 [xe] >>[] xe_pci_probe+0x33f/0x5a0 [xe] >>[] local_pci_probe+0x47/0xa0 >>[] pci_device_probe+0xc3/0x1f0 >>[] really_probe+0x1a2/0x410 >>[] __driver_probe_device+0x78/0x160 >>[] driver_probe_device+0x1e/0x90 >>[] __driver_attach+0xda/0x1d0 >>[] bus_for_each_dev+0x7c/0xd0 >>[] bus_add_driver+0x119/0x220 >>[] driver_register+0x60/0x120 >>[] 0xa05e50a0 >> > > This will need a Fixes trailer. This seems to be a suitable one: > > Fixes: 44e694958b95 ("drm/xe/display: Implement display support") > >>Signed-off-by: wangxiaoming321 >>--- >> drivers/gpu/drm/xe/xe_display.c | 6 -- >> 1 file changed, 6 deletions(-) >> >>diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c >>index 74391d9b11ae..e4db069f0db3 100644 >>--- a/drivers/gpu/drm/xe/xe_display.c >>+++ b/drivers/gpu/drm/xe/xe_display.c >>@@ -134,8 +134,6 @@ static void xe_display_fini_nommio(struct drm_device >>*dev, void *dummy) >> >> int xe_display_init_nommio(struct xe_device *xe) >> { >>- int err; >>- >> if (!xe->info.enable_display) >> return 0; >> >>@@ -145,10 +143,6 @@ int xe_display_init_nommio(struct xe_device *xe) >> /* This must be called before any calls to HAS_PCH_* */ >> intel_detect_pch(xe); >> >>- err = intel_power_domains_init(xe); >>- if (err) >>- return err; > > xe_display_init_nommio() has xe_display_fini_nommio() as its destructor > counter part. Unfortunately display side looks wrong as it does: > > init: > intel_display_driver_probe_noirq() -> intel_power_domains_init() > > destroy: > i915_driver_late_release() -> intel_power_domains_cleanup() > > I think leaving intel_power_domains_cleanup() as is for now so it's > called by xe works, but this needs to go through CI, which apparently > this series didn't go. I re-triggered it. > > +Jani if he thinks this can be changed in another way or already have > the complete solution. I don't. But it is and will be a recurring problem. i915 and xe core drivers should handle display init and cleanup the same way. But currently i915 goes on to call e.g. intel_power_domains_cleanup() directly from top level driver code. There are other examples. And we seem to have recently added *more*. See e.g. bd738d859e71 ("drm/i915: Prevent modesets during driver init/shutdown"). BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2 2/4] drm: Add drm_get_acpi_edid() helper
Hi Mario, kernel test robot noticed the following build warnings: [auto build test WARNING on rafael-pm/linux-next] [also build test WARNING on rafael-pm/acpi-bus linus/master v6.8-rc2 next-20240131] [cannot apply to drm-misc/drm-misc-next rafael-pm/devprop] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-video-Handle-fetching-EDID-that-is-longer-than-256-bytes/20240131-032909 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240130192608.11666-3-mario.limonciello%40amd.com patch subject: [PATCH v2 2/4] drm: Add drm_get_acpi_edid() helper config: x86_64-kismet-CONFIG_ACPI_WMI-CONFIG_DRM-0-0 (https://download.01.org/0day-ci/archive/20240131/202401312256.jbaomfd9-...@intel.com/config) reproduce: (https://download.01.org/0day-ci/archive/20240131/202401312256.jbaomfd9-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202401312256.jbaomfd9-...@intel.com/ kismet warnings: (new ones prefixed by >>) >> kismet: WARNING: unmet direct dependencies detected for ACPI_WMI when >> selected by DRM .config:248:warning: symbol value 'n' invalid for AIC79XX_CMDS_PER_DEVICE .config:250:warning: symbol value 'n' invalid for SATA_MOBILE_LPM_POLICY .config:308:warning: symbol value 'n' invalid for SQUASHFS_FRAGMENT_CACHE_SIZE .config:333:warning: symbol value 'n' invalid for PANEL_LCD_PIN_SDA .config:356:warning: symbol value 'n' invalid for PSTORE_BLK_MAX_REASON .config:414:warning: symbol value 'n' invalid for FB_OMAP2_NUM_FBS .config:459:warning: symbol value 'n' invalid for KFENCE_SAMPLE_INTERVAL .config:543:warning: symbol value 'n' invalid for CFAG12864B_RATE .config:651:warning: symbol value 'n' invalid for CRYPTO_DEV_QCE_SW_MAX_LEN .config:665:warning: symbol value 'n' invalid for BLK_DEV_LOOP_MIN_COUNT .config:756:warning: symbol value 'n' invalid for PANEL_LCD_CHARSET .config:840:warning: symbol value 'n' invalid for SND_AC97_POWER_SAVE_DEFAULT .config:855:warning: symbol value 'n' invalid for MAGIC_SYSRQ_DEFAULT_ENABLE .config:893:warning: symbol value 'n' invalid for DRM_I915_MAX_REQUEST_BUSYWAIT .config:894:warning: symbol value 'n' invalid for RAPIDIO_DISC_TIMEOUT .config:917:warning: symbol value 'n' invalid for FAT_DEFAULT_CODEPAGE .config:920:warning: symbol value 'n' invalid for SND_AT73C213_TARGET_BITRATE .config:966:warning: symbol value 'n' invalid for CMA_SIZE_MBYTES .config:967:warning: symbol value 'n' invalid for NET_EMATCH_STACK .config:969:warning: symbol value 'n' invalid for VMCP_CMA_SIZE .config:1152:warning: symbol value 'n' invalid for NODES_SHIFT .config:1247:warning: symbol value 'n' invalid for MTDRAM_ERASE_SIZE .config:1307:warning: symbol value 'n' invalid for SERIAL_UARTLITE_NR_UARTS .config:1318:warning: symbol value 'n' invalid for AIC7XXX_DEBUG_MASK .config:1479:warning: symbol value 'n' invalid for LEGACY_PTY_COUNT .config:1645:warning: symbol value 'n' invalid for AIC7XXX_RESET_DELAY_MS .config:1682:warning: symbol value 'n' invalid for INPUT_MOUSEDEV_SCREEN_Y .config:1861:warning: symbol value 'n' invalid for IBM_EMAC_POLL_WEIGHT .config:1936:warning: symbol value 'n' invalid for DRM_I915_STOP_TIMEOUT .config:2029:warning: symbol value 'n' invalid for USB_GADGET_STORAGE_NUM_BUFFERS .config:2151:warning: symbol value 'n' invalid for SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM .config:2259:warning: symbol value 'n' invalid for SND_HDA_PREALLOC_SIZE .config:2310:warning: symbol value 'n' invalid for RCU_FANOUT_LEAF .config:2467:warning: symbol value 'n' invalid for PANEL_LCD_BWIDTH .config:2523:warning: symbol value 'n' invalid for PANEL_LCD_PIN_E .config:2542:warning: symbol value 'n' invalid for PSTORE_BLK_CONSOLE_SIZE .config:2725:warning: symbol value 'n' invalid for PANEL_PARPORT .config:2745:warning: symbol value 'n' invalid for BOOKE_WDT_DEFAULT_TIMEOUT .config:2824:warning: symbol value 'n' invalid for NOUVEAU_DEBUG_DEFAULT .config:3026:warning: symbol value 'n' invalid for KCSAN_REPORT_ONCE_IN_MS .config:3136:warning: symbol value 'n' invalid for KCSAN_UDELAY_INTERRUPT .config:3162:warning: symbol value 'n' invalid for PANEL_LCD_PIN_BL .config:3190:warning: symbol value 'n' invalid for INITRAMFS_ROOT_GID .config:3306:warning: symbol value 'n' invalid for ATM_FORE200E_TX_RETRY .config:3347:warning: symbol value 'n' invalid for FB_OMAP2_DSS_MIN_FCK_PER_PCK .config:3351:warning: symbol value 'n' invalid for STACK_M
Re: [PATCH] drm/xe/display: Fix memleak in display initialization
+Jani On Fri, Jan 26, 2024 at 11:34:53PM +0800, wangxiaoming321 wrote: intel_power_domains_init has been called twice in xe_device_probe: xe_device_probe -> xe_display_init_nommio -> intel_power_domains_init(xe) xe_device_probe -> xe_display_init_noirq -> intel_display_driver_probe_noirq -> intel_power_domains_init(i915) ok, once upon a time intel_power_domains_init() was called by the driver initialization code and not initialized inside the display. I think. Now it's part of the display probe and we never updated the xe side. It needs remove one to avoid power_domains->power_wells double malloc. unreferenced object 0x88811150ee00 (size 512): comm "systemd-udevd", pid 506, jiffies 4294674198 (age 3605.560s) hex dump (first 32 bytes): 10 b4 9d a0 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 backtrace: [] __kmem_cache_alloc_node+0x1c1/0x2b0 [] __kmalloc+0x52/0x150 [] __set_power_wells+0xc3/0x360 [xe] [] xe_display_init_nommio+0x4c/0x70 [xe] [] xe_device_probe+0x3c/0x5a0 [xe] [] xe_pci_probe+0x33f/0x5a0 [xe] [] local_pci_probe+0x47/0xa0 [] pci_device_probe+0xc3/0x1f0 [] really_probe+0x1a2/0x410 [] __driver_probe_device+0x78/0x160 [] driver_probe_device+0x1e/0x90 [] __driver_attach+0xda/0x1d0 [] bus_for_each_dev+0x7c/0xd0 [] bus_add_driver+0x119/0x220 [] driver_register+0x60/0x120 [] 0xa05e50a0 This will need a Fixes trailer. This seems to be a suitable one: Fixes: 44e694958b95 ("drm/xe/display: Implement display support") Signed-off-by: wangxiaoming321 --- drivers/gpu/drm/xe/xe_display.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c index 74391d9b11ae..e4db069f0db3 100644 --- a/drivers/gpu/drm/xe/xe_display.c +++ b/drivers/gpu/drm/xe/xe_display.c @@ -134,8 +134,6 @@ static void xe_display_fini_nommio(struct drm_device *dev, void *dummy) int xe_display_init_nommio(struct xe_device *xe) { - int err; - if (!xe->info.enable_display) return 0; @@ -145,10 +143,6 @@ int xe_display_init_nommio(struct xe_device *xe) /* This must be called before any calls to HAS_PCH_* */ intel_detect_pch(xe); - err = intel_power_domains_init(xe); - if (err) - return err; xe_display_init_nommio() has xe_display_fini_nommio() as its destructor counter part. Unfortunately display side looks wrong as it does: init: intel_display_driver_probe_noirq() -> intel_power_domains_init() destroy: i915_driver_late_release() -> intel_power_domains_cleanup() I think leaving intel_power_domains_cleanup() as is for now so it's called by xe works, but this needs to go through CI, which apparently this series didn't go. I re-triggered it. +Jani if he thinks this can be changed in another way or already have the complete solution. Lucas De Marchi
Re: [PATCH] backlight: mp3309c: Use pwm_apply_might_sleep()
On Sun, Jan 28, 2024 at 03:49:04PM +, Sean Young wrote: > pwm_apply_state() is deprecated since commit c748a6d77c06a ("pwm: Rename > pwm_apply_state() to pwm_apply_might_sleep()"). This is the final user > in the tree. > > Signed-off-by: Sean Young Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] backlight: ktz8866: Correct the check for of_property_read_u32
On Mon, Jan 29, 2024 at 08:28:29PM +0800, Jianhua Lu wrote: > of_property_read_u32 returns 0 when success, so reverse the > return value to get the true value. > > Fixes: f8449c8f7355 ("backlight: ktz8866: Add support for Kinetic KTZ8866 > backlight") > Signed-off-by: Jianhua Lu Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v2 1/1] drm/virtio: Implement device_attach
Am 31.01.24 um 11:20 schrieb Zhang, Julia: On 2024/1/30 22:23, Christian König wrote: Am 30.01.24 um 12:16 schrieb Daniel Vetter: On Tue, Jan 30, 2024 at 12:10:31PM +0100, Daniel Vetter wrote: [SNIP] Hi Sima, Christian, Yeah, that is really just speculative. All importers need to set the peer2peer flag just in case. I see, I will modify this. What happens under the hood is that IOMMU redirects the "VRAM" memory access to whatever address the DMA-buf on the host is pointing to (system, VRAM, doorbell, IOMMU, whatever). I'm also not 100% sure if all the cache snooping is done correctly in all cases, but for now it seems to work. Frankly the more I look at the original patch that added vram export support the more this just looks like a "pls revert, this is just too broken". The commit I mean is this one: ea5ea3d8a117 ("drm/virtio: support mapping exported vram"). The commit message definitely needs to cite that one, and also needs a cc: stable because not rejecting invalid imports is a pretty big deal. Yeah, I've pointed out that commit in an internal discussion as well. I was just not aware that it's that severely broken. Yeah we have mentioned this patch before, but I don't totally understand why this is too broken. Without exporting vram objects, dGPU prime feature would not be realized. Would you mind to explain more about it. Thanks! One reason is that using sg tables without struct pages is actually a hack we came up with because we couldn't hope to clean up the sg table structure any time soon to not include struct page pointers. Another reason is that using this with devices which don't expect a DMA address pointing into a virtual PCI BAR. So doing this without checking the peer2peer flag can most likely cause quite a bit of trouble. Regards, Christian. Best regards, Julia Regards, Christian.
Re: [PATCH] dma-buf: heaps: Don't track CMA dma-buf pages under RssFile
Hi TJ, On Thu, 18 Jan 2024 at 15:32, Christian König wrote: > Am 17.01.24 um 19:11 schrieb T.J. Mercier: > > DMA buffers allocated from the CMA dma-buf heap get counted under > RssFile for processes that map them and trigger page faults. In > addition to the incorrect accounting reported to userspace, reclaim > behavior was influenced by the MM_FILEPAGES counter until linux 6.8, but > this memory is not reclaimable. [1] Change the CMA dma-buf heap to set > VM_PFNMAP on the VMA so MM does not poke at the memory managed by this > dma-buf heap, and use vmf_insert_pfn to correct the RSS accounting. > > The system dma-buf heap does not suffer from this issue since > remap_pfn_range is used during the mmap of the buffer, which also sets > VM_PFNMAP on the VMA. > > > Mhm, not an issue with this patch but Daniel wanted to add a check for > VM_PFNMAP to dma_buf_mmap() which would have noted this earlier. > > I don't fully remember the discussion but for some reason that was never > committed. We should probably try that again. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/mm/vmscan.c?id=fb46e22a9e3863e08aef8815df9f17d0f4b9aede > > Fixes: b61614ec318a ("dma-buf: heaps: Add CMA heap to dmabuf heaps") > Signed-off-by: T.J. Mercier > > > Acked-by: Christian König > > Thanks for the patch; pushed to drm-misc-fixes. Best, Sumit > > > --- > drivers/dma-buf/heaps/cma_heap.c | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c > b/drivers/dma-buf/heaps/cma_heap.c > index ee899f8e6721..4a63567e93ba 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -168,10 +168,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf) > if (vmf->pgoff > buffer->pagecount) > return VM_FAULT_SIGBUS; > > - vmf->page = buffer->pages[vmf->pgoff]; > - get_page(vmf->page); > - > - return 0; > + return vmf_insert_pfn(vma, vmf->address, > page_to_pfn(buffer->pages[vmf->pgoff])); > } > > static const struct vm_operations_struct dma_heap_vm_ops = { > @@ -185,6 +182,8 @@ static int cma_heap_mmap(struct dma_buf *dmabuf, struct > vm_area_struct *vma) > if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) > return -EINVAL; > > + vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP); > + > vma->vm_ops = _heap_vm_ops; > vma->vm_private_data = buffer; > > > > -- Thanks and regards, Sumit Semwal (he / him) Tech Lead - LCG, Vertical Technologies Linaro.org │ Open source software for ARM SoCs
Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops
On Fri, Jan 12, 2024 at 05:23:07PM -0800, John Stultz wrote: > On Fri, Jan 12, 2024 at 4:13 PM Jeffrey Kardatzke > wrote: > > On Fri, Jan 12, 2024 at 3:51 PM John Stultz wrote: > > > > > > On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke > > > wrote: > > > > On Fri, Jan 12, 2024 at 2:52 PM John Stultz wrote: > > > > > I know part of this effort here is to start to centralize all these > > > > > vendor specific restricted buffer implementations, which I think is > > > > > great, but I just worry that in doing it in the dmabuf heap interface, > > > > > it is a bit too user-facing. The likelihood of encoding a particular > > > > > semantic to the singular "restricted_heap" name seems high. > > > > > > > > In patch #5 it has the actual driver implementation for the MTK heap > > > > that includes the heap name of "restricted_mtk_cm", so there shouldn't > > > > be a heap named "restricted_heap" that's actually getting exposed. The > > > > > > Ah, I appreciate that clarification! Indeed, you're right the name is > > > passed through. Apologies for missing that detail. > > > > > > > restricted_heap code is a framework, and not a driver itself. Unless > > > > I'm missing something in this patchset...but that's the way it's > > > > *supposed* to be. > > > > > > So I guess I'm not sure I understand the benefit of the extra > > > indirection. What then does the restricted_heap.c logic itself > > > provide? > > > The dmabuf heaps framework already provides a way to add heap > > > implementations. > > > > So in the v1 patchset, it was done with just a Mediatek specific heap > > with no framework or abstractions for another vendor to build on top > > of. The feedback was to make this more generic since Mediatek won't be > > the only vendor who wants a restricted heap..which is how it ended up > > here. There was more code in the framework before relating to TEE > > calls, but then that was moved to the vendor specific code since not > > all restricted heaps are allocated through a TEE. > > Yeah. I apologize, as I know how frustrating the contradictory > feedback can be. I don't mean to demotivate. :( > > I think folks would very much like to see consolidation around the > various implementations, and I agree! > I just worry that creating the common framework for this concept in a > dmabuf heaps driver is maybe too peripheral/close to userland. > > > This was also desirable for the V4L2 pieces since there's going to be > > a V4L2 flag set when using restricted dma_bufs (and it wants to > > validate that)so in order to keep that more generic, there should > > be a higher level concept of restricted dma_bufs that isn't specific > > to a single vendor. One other thing that would ideally come out of > > this is a cleaner way to check that a dma_buf is restricted or not. > > Yeah. If there is a clear meaning to "restricted" here, I think having > a query method on the dmabuf is reasonable. > My only fret is if the meaning is too vague and userland starts > depending on it meaning what it meant for vendor1, but doesn't mean > for vendor2. > > So we need some clarity in what "restricted" really means. For > instance, it being not cpu mappable vs other potential variations like > being cpu mappable, but not cpu accessible. Or not cpu mappable, but > only mappable between a set of 3 devices (Which 3 devices?! How can we > tell?). > Can we flip things around? I.e., instead of saying which devices are allowed to use the restricted buffer, can we instead say where it's not allowed to be used? My view has been that by default the contents of the types of buffers where talking about here is only accessible to things running on the secure side, i.e, typically S-EL3, S-EL1 and a specific Trusted Application running in S-EL0. I guess that serves as some kind of baseline. >From there, things turns to a more dynamic nature, where firewalls etc, can be configured to give access to various IPs, blocks and runtimes. I understand that it's nice to be able to know all this from the Linux kernel point of view, but does it have to be aware of this? What's the major drawback if Linux doesn't know about it? > And if there is variation, maybe we need to enumerate the types of > "restricted" buffers so we can be specific when it's queried. > > That's where maybe having the framework for this be more central or > closer to the kernel mm code and not just a sub-type of a dmabuf heap > driver might be better? > > > The current V4L2 patchset just attaches the dma_buf and then checks if > > the page table is emptyand if so, it's restricted. But now I see > > there's other feedback indicating attaching a restricted dma_buf > > shouldn't even be allowed, so we'll need another strategy for > > detecting them. Ideally there is some function/macro like > > is_dma_buf_restricted(struct dma_buf*) that can indicate that...but we > > haven't come up with a good way to do that yet which doesn't involve > > adding another field to
Re: [PATCH v1 1/6] dt-bindings: display: rockchip: rockchip, dw-hdmi: deprecate port property
On Tue, Jan 30, 2024 at 06:18:49PM +, Conor Dooley wrote: > On Tue, Jan 30, 2024 at 03:55:43PM +0100, Johan Jonker wrote: > > The hdmi-connector nodes are now functional and the new way to model > > hdmi nodes with, so deprecate the port property and > > This doesn't really explain what makes having hdmi-connector nodes > replace the usecase for "port". > > > make port@0 and > > port@1 a requirement. > > Why? That means the deprecated way will always have warnings which makes documenting the deprecated stuff a bit pointless. Technically, new required properties are ABI break and something I'm working on making the tools check (by comparing 2 versions of schemas). That said, if all the upstream dts files are fixed already, then I don't care too much. > > Also update example. > > "Also do x" is a red flag when it comes to commit messages, as it > immediately makes me think that this should be more than one commit. > I'd probably write this as "Update the example to avoid use of the > deprecated property" or something to avoid bad gut reactions. > That's not worth resending for though obviously... > > > > > Signed-off-by: Johan Jonker > > --- > > .../display/rockchip/rockchip,dw-hdmi.yaml| 27 --- > > 1 file changed, 23 insertions(+), 4 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > > b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > > index 7e59dee15a5f..cd0a42f35f24 100644 > > --- > > a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > > +++ > > b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > > @@ -97,8 +97,11 @@ properties: > >ports: > > $ref: /schemas/graph.yaml#/properties/ports > > > > -patternProperties: > > - "^port(@0)?$": > > +properties: > > + port: > > +$ref: /schemas/graph.yaml#/properties/port > > +deprecated: true > > This change makes the deprecated property's description incomplete, > since it doesn't cover the endpoints any more. It also doesn't make > port@0 and port mutually exclusive. graph.yaml has a check that effectively does that. Rob
Re: [PATCH v1 2/6] dt-bindings: display: rockchip,dw-hdmi: add power-domains property
On Tue, Jan 30, 2024 at 03:57:23PM +0100, Johan Jonker wrote: > Most Rockchip hdmi nodes are part of a power domain. > Add a power-domains property. Fix example. > > Signed-off-by: Johan Jonker > --- > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > index cd0a42f35f24..6f421740b613 100644 > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > @@ -94,6 +94,9 @@ properties: >- const: default >- const: unwedge > > + power-domains: > +maxItems: 1 > + >ports: > $ref: /schemas/graph.yaml#/properties/ports > > @@ -141,16 +144,18 @@ examples: > #include > #include > #include > +#include > > hdmi: hdmi@ff98 { > compatible = "rockchip,rk3288-dw-hdmi"; > reg = <0xff98 0x2>; > -reg-io-width = <4>; It makes more sense to keep reg-io-width together with reg. > -ddc-i2c-bus = <>; > -rockchip,grf = <>; > interrupts = ; > clocks = < PCLK_HDMI_CTRL>, < SCLK_HDMI_HDCP>; > clock-names = "iahb", "isfr"; > +ddc-i2c-bus = <>; > +power-domains = < RK3288_PD_VIO>; > +reg-io-width = <4>; > +rockchip,grf = <>; > > ports { > #address-cells = <1>; > -- > 2.39.2 >
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Wed, Jan 31, 2024 at 02:50:16PM +0200, Hogander, Jouni wrote: > [...] > > + > > +struct drm_dp_tunnel_group; > > + > > +struct drm_dp_tunnel { > > + struct drm_dp_tunnel_group *group; > > + > > + struct list_head node; > > + > > + struct kref kref; > > +#ifdef CONFIG_DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > > + struct ref_tracker *tracker; > > +#endif > > + struct drm_dp_aux *aux; > > + char name[8]; > > + > > + int bw_granularity; > > + int estimated_bw; > > + int allocated_bw; > > + > > + int max_dprx_rate; > > + u8 max_dprx_lane_count; > > + > > + u8 adapter_id; > > + > > + bool bw_alloc_supported:1; > > + bool bw_alloc_enabled:1; > > + bool has_io_error:1; > > + bool destroyed:1; > > +}; > > + > > +struct drm_dp_tunnel_group_state; > > + > > +struct drm_dp_tunnel_state { > > + struct drm_dp_tunnel_group_state *group_state; > > + > > + struct drm_dp_tunnel_ref tunnel_ref; > > + > > + struct list_head node; > > + > > + u32 stream_mask; > > I'm wondering if drm_dp_tunnel_state can really contain several streams > and what kind of scenario this would be? From i915 point of view I > would understand that several pipes are routed to DP tunnel. Yes, multiple pipes through the same tunnel and the use case for that is MST with multiple streams. The "stream" term is only an abstraction where it could be a different physical thing in various drivers, but for i915 it just means pipes. Not 100% sure if that's the best mapping, since in case of bigjoiner there would be multiple pipes, but possibly (in the SST case) only one stream from the tunneling POV. > Is it bigjoiner case? IIUC in that (SST) case the streams would be joined already before going to the TBT DP_IN adapter, so that's only one stream in stream_mask above (unless MST + bigjoiner, where you could have 2 MST/DP tunnel streams each consisting of 2 pipes). > BR, > > Jouni Högander > > > + int *stream_bw; > > +}; > > + > > +struct drm_dp_tunnel_group_state { > > + struct drm_private_state base; > > + > > + struct list_head tunnel_states; > > +}; > > + > > +struct drm_dp_tunnel_group { > > + struct drm_private_obj base; > > + struct drm_dp_tunnel_mgr *mgr; > > + > > + struct list_head tunnels; > > + > > + int available_bw; /* available BW including the > > allocated_bw of all tunnels */ > > + int drv_group_id; > > + > > + char name[8]; > > + > > + bool active:1; > > +}; > > + > > +struct drm_dp_tunnel_mgr { > > + struct drm_device *dev; > > + > > + int group_count; > > + struct drm_dp_tunnel_group *groups; > > + wait_queue_head_t bw_req_queue; > > + > > +#ifdef CONFIG_DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > > + struct ref_tracker_dir ref_tracker; > > +#endif > > +}; > > + > > +static int next_reg_area(int *offset) > > +{ > > + *offset = find_next_bit(dptun_info_regs, 64, *offset); > > + > > + return find_next_zero_bit(dptun_info_regs, 64, *offset + 1) - > > *offset; > > +} > > + > > +#define tunnel_reg_ptr(__regs, __address) ({ \ > > + WARN_ON(!test_bit((__address) - DP_TUNNELING_BASE, > > dptun_info_regs)); \ > > + &(__regs)->buf[bitmap_weight(dptun_info_regs, (__address) - > > DP_TUNNELING_BASE)]; \ > > +}) > > + > > +static int read_tunnel_regs(struct drm_dp_aux *aux, struct > > drm_dp_tunnel_regs *regs) > > +{ > > + int offset = 0; > > + int len; > > + > > + while ((len = next_reg_area())) { > > + int address = DP_TUNNELING_BASE + offset; > > + > > + if (drm_dp_dpcd_read(aux, address, > > tunnel_reg_ptr(regs, address), len) < 0) > > + return -EIO; > > + > > + offset += len; > > + } > > + > > + return 0; > > +} > > + > > +static u8 tunnel_reg(const struct drm_dp_tunnel_regs *regs, int > > address) > > +{ > > + return *tunnel_reg_ptr(regs, address); > > +} > > + > > +static int tunnel_reg_drv_group_id(const struct drm_dp_tunnel_regs > > *regs) > > +{ > > + int drv_id = tunnel_reg(regs, DP_USB4_DRIVER_ID) & > > DP_USB4_DRIVER_ID_MASK; > > + int group_id = tunnel_reg(regs, > > DP_IN_ADAPTER_TUNNEL_INFORMATION) & DP_GROUP_ID_MASK; > > + > > + if (!group_id) > > + return 0; > > + > > + return (drv_id << DP_GROUP_ID_BITS) | group_id; > > +} > > + > > +/* Return granularity in kB/s units */ > > +static int tunnel_reg_bw_granularity(const struct drm_dp_tunnel_regs > > *regs) > > +{ > > + int gr = tunnel_reg(regs, DP_BW_GRANULARITY) & > > DP_BW_GRANULARITY_MASK; > > + > > + WARN_ON(gr > 2); > > + > > + return (25 << gr) / 8; > > +} > > + > > +static int tunnel_reg_max_dprx_rate(const struct drm_dp_tunnel_regs > > *regs) > > +{ > > + u8 bw_code = tunnel_reg(regs, DP_TUNNELING_MAX_LINK_RATE); > > + > > + return drm_dp_bw_code_to_link_rate(bw_code); > > +} >
Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops
On Fri, Jan 12, 2024 at 05:20:10PM +0800, Yong Wu wrote: > Add "struct restricted_heap_ops". For the restricted memory, totally there > are two steps: > a) memory_alloc: Allocate the buffer in kernel; > b) memory_restrict: Restrict/Protect/Secure that buffer. > The memory_alloc is mandatory while memory_restrict is optinal since it may > s/optinal/optional/ > be part of memory_alloc. > > Signed-off-by: Yong Wu > --- > drivers/dma-buf/heaps/restricted_heap.c | 41 - > drivers/dma-buf/heaps/restricted_heap.h | 12 > 2 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/restricted_heap.c > b/drivers/dma-buf/heaps/restricted_heap.c > index fd7c82abd42e..8c266a0f6192 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.c > +++ b/drivers/dma-buf/heaps/restricted_heap.c > @@ -12,10 +12,44 @@ > > #include "restricted_heap.h" > > +static int > +restricted_heap_memory_allocate(struct restricted_heap *heap, struct > restricted_buffer *buf) > +{ > + const struct restricted_heap_ops *ops = heap->ops; > + int ret; > + > + ret = ops->memory_alloc(heap, buf); > + if (ret) > + return ret; > + > + if (ops->memory_restrict) { > + ret = ops->memory_restrict(heap, buf); > + if (ret) > + goto memory_free; > + } > + return 0; > + > +memory_free: > + ops->memory_free(heap, buf); > + return ret; > +} > + > +static void > +restricted_heap_memory_free(struct restricted_heap *heap, struct > restricted_buffer *buf) > +{ > + const struct restricted_heap_ops *ops = heap->ops; > + > + if (ops->memory_unrestrict) > + ops->memory_unrestrict(heap, buf); > + > + ops->memory_free(heap, buf); > +} > + > static struct dma_buf * > restricted_heap_allocate(struct dma_heap *heap, unsigned long size, >unsigned long fd_flags, unsigned long heap_flags) > { > + struct restricted_heap *restricted_heap = dma_heap_get_drvdata(heap); > struct restricted_buffer *restricted_buf; > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > struct dma_buf *dmabuf; > @@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned > long size, > restricted_buf->size = ALIGN(size, PAGE_SIZE); > restricted_buf->heap = heap; > > + ret = restricted_heap_memory_allocate(restricted_heap, restricted_buf); > Can we guarantee that "restricted_heap" here isn't NULL (i.e., heap->priv). If not perhaps we should consider adding a check for NULL in the restricted_heap_memory_allocate() function? > + if (ret) > + goto err_free_buf; > exp_info.exp_name = dma_heap_get_name(heap); > exp_info.size = restricted_buf->size; > exp_info.flags = fd_flags; > @@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned > long size, > dmabuf = dma_buf_export(_info); > if (IS_ERR(dmabuf)) { > ret = PTR_ERR(dmabuf); > - goto err_free_buf; > + goto err_free_restricted_mem; > } > > return dmabuf; > > +err_free_restricted_mem: > + restricted_heap_memory_free(restricted_heap, restricted_buf); > err_free_buf: > kfree(restricted_buf); > return ERR_PTR(ret); > diff --git a/drivers/dma-buf/heaps/restricted_heap.h > b/drivers/dma-buf/heaps/restricted_heap.h > index 443028f6ba3b..ddeaf9805708 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.h > +++ b/drivers/dma-buf/heaps/restricted_heap.h > @@ -15,6 +15,18 @@ struct restricted_buffer { > > struct restricted_heap { > const char *name; > + > + const struct restricted_heap_ops *ops; > +}; > + > +struct restricted_heap_ops { > This have the same name as used for the dma_heap_ops in the file restricted_heap.c, this might be a little bit confusing, or? > + int (*heap_init)(struct restricted_heap *heap); > + > + int (*memory_alloc)(struct restricted_heap *heap, struct > restricted_buffer *buf); > + void(*memory_free)(struct restricted_heap *heap, struct > restricted_buffer *buf); > + > + int (*memory_restrict)(struct restricted_heap *heap, struct > restricted_buffer *buf); > + void(*memory_unrestrict)(struct restricted_heap *heap, struct > restricted_buffer *buf); > Is the prefix "memory_" superfluous here in these ops? Also related to a comment on the prior patch. The name here is "heap" for restricted_heap, but below you use rstrd_heap. It's the same struct, so I would advise to use the same name to avoid confusion when reading the code. As mentioned before, I think the name "rheap" would be a good choice. > }; > > int restricted_heap_add(struct restricted_heap *rstrd_heap); > -- > 2.25.1 > -- // Regards Joakim
Re: [PATCH v3 14/24] of: property: add of_graph_get_next_endpoint()
Hello Kuninori Morimoto, On Wed, 31 Jan 2024 05:06:36 + Kuninori Morimoto wrote: > To handle endpoint more intuitive, create of_graph_get_next_endpoint() > > of_graph_get_next_endpoint(port1, NULL); // A1 > of_graph_get_next_endpoint(port1, A1); // A2 > of_graph_get_next_endpoint(port1, A2); // NULL The idea looks good. My only concern is about reusing the of_graph_get_next_endpoint() name after having removed the old, different function having the same name. This can be confusing in the first place to who is used to the old function, and also to anybody rebasing their patches on top of a new kernel to find their code behaving differently. Also, as now we'd have two similar variants of this function, it would be good if each of them were having a name that clearly identifies in which way they differ from the other. So a better name for this function would probably be of_graph_get_next_port_endpoint() I guess, to clearly differentiate from of_graph_get_next_device_endpoint(). Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v4 2/7] dma-buf: heaps: Initialize a restricted heap
On Fri, Jan 12, 2024 at 05:20:09PM +0800, Yong Wu wrote: > Initialize a restricted heap. Currently just add a null heap, Prepare for > the later patches. > > Signed-off-by: Yong Wu > --- > drivers/dma-buf/heaps/Kconfig | 9 > drivers/dma-buf/heaps/Makefile | 3 +- > drivers/dma-buf/heaps/restricted_heap.c | 67 + > drivers/dma-buf/heaps/restricted_heap.h | 22 > 4 files changed, 100 insertions(+), 1 deletion(-) > create mode 100644 drivers/dma-buf/heaps/restricted_heap.c > create mode 100644 drivers/dma-buf/heaps/restricted_heap.h > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index a5eef06c4226..e54506f480ea 100644 > --- a/drivers/dma-buf/heaps/Kconfig > +++ b/drivers/dma-buf/heaps/Kconfig > @@ -12,3 +12,12 @@ config DMABUF_HEAPS_CMA > Choose this option to enable dma-buf CMA heap. This heap is backed > by the Contiguous Memory Allocator (CMA). If your system has these > regions, you should say Y here. > + > +config DMABUF_HEAPS_RESTRICTED > + bool "DMA-BUF Restricted Heap" > + depends on DMABUF_HEAPS > + help > + Choose this option to enable dma-buf restricted heap. The purpose of > this > + heap is to manage buffers that are inaccessible to the kernel and > user space. > + There may be several ways to restrict it, for example it may be > encrypted or > + protected by a TEE or hypervisor. If in doubt, say N. > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile > index 974467791032..a2437c1817e2 100644 > --- a/drivers/dma-buf/heaps/Makefile > +++ b/drivers/dma-buf/heaps/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > -obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o > obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)+= restricted_heap.o > +obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o > diff --git a/drivers/dma-buf/heaps/restricted_heap.c > b/drivers/dma-buf/heaps/restricted_heap.c > new file mode 100644 > index ..fd7c82abd42e > --- /dev/null > +++ b/drivers/dma-buf/heaps/restricted_heap.c > @@ -0,0 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * DMABUF restricted heap exporter > + * > + * Copyright (C) 2024 MediaTek Inc. > + */ > + > +#include > +#include > +#include > +#include > + > +#include "restricted_heap.h" > + > +static struct dma_buf * > +restricted_heap_allocate(struct dma_heap *heap, unsigned long size, > + unsigned long fd_flags, unsigned long heap_flags) > +{ > + struct restricted_buffer *restricted_buf; > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > + struct dma_buf *dmabuf; > + int ret; > + > + restricted_buf = kzalloc(sizeof(*restricted_buf), GFP_KERNEL); > + if (!restricted_buf) > + return ERR_PTR(-ENOMEM); > + > + restricted_buf->size = ALIGN(size, PAGE_SIZE); > + restricted_buf->heap = heap; > + > + exp_info.exp_name = dma_heap_get_name(heap); > + exp_info.size = restricted_buf->size; > + exp_info.flags = fd_flags; > + exp_info.priv = restricted_buf; > + > + dmabuf = dma_buf_export(_info); > + if (IS_ERR(dmabuf)) { > + ret = PTR_ERR(dmabuf); > + goto err_free_buf; > + } > + > + return dmabuf; > + > +err_free_buf: > + kfree(restricted_buf); > + return ERR_PTR(ret); > +} > + > +static const struct dma_heap_ops restricted_heap_ops = { > + .allocate = restricted_heap_allocate, > +}; > + > +int restricted_heap_add(struct restricted_heap *rstrd_heap) > Nothing wrong, but what about shortening rstrd_heap throughout the patch set to "rheap", I would find that easier to read. > +{ > + struct dma_heap_export_info exp_info; > + struct dma_heap *heap; > + > + exp_info.name = rstrd_heap->name; > + exp_info.ops = _heap_ops; > + exp_info.priv = (void *)rstrd_heap; > + > + heap = dma_heap_add(_info); > + if (IS_ERR(heap)) > + return PTR_ERR(heap); > + return 0; > +} > +EXPORT_SYMBOL_GPL(restricted_heap_add); > diff --git a/drivers/dma-buf/heaps/restricted_heap.h > b/drivers/dma-buf/heaps/restricted_heap.h > new file mode 100644 > index ..443028f6ba3b > --- /dev/null > +++ b/drivers/dma-buf/heaps/restricted_heap.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Restricted heap Header. > + * > + * Copyright (C) 2024 MediaTek, Inc. > + */ > + > +#ifndef _DMABUF_RESTRICTED_HEAP_H_ > +#define _DMABUF_RESTRICTED_HEAP_H_ > + > +struct restricted_buffer { > + struct dma_heap *heap; > + size_t size; > +}; > + > +struct restricted_heap { > + const char *name; > +}; > + > +int restricted_heap_add(struct restricted_heap *rstrd_heap); > + > +#endif > -- > 2.25.1 > -- // Regards Joakim
Re: [v3 4/6] drm/vs: Add KMS crtc
On Wed, Jan 31, 2024 at 09:33:06AM +, Keith Zhao wrote: > > > > -邮件原件- > > 发件人: Maxime Ripard > > 发送时间: 2023年12月6日 16:56 > > 收件人: Keith Zhao > > 抄送: devicet...@vger.kernel.org; dri-devel@lists.freedesktop.org; > > linux-ker...@vger.kernel.org; linux-ri...@lists.infradead.org; > > tzimmerm...@suse.de; airl...@gmail.com; krzysztof.kozlowski...@linaro.org; > > William Qiu ; Xingyu Wu > > ; paul.walms...@sifive.com; > > a...@eecs.berkeley.edu; pal...@dabbelt.com; p.za...@pengutronix.de; > > Shengyang Chen ; Jack Zhu > > ; Changhuang Liang > > ; maarten.lankho...@linux.intel.com; > > suijingf...@loongson.cn > > 主题: Re: [v3 4/6] drm/vs: Add KMS crtc > > > > On Mon, Dec 04, 2023 at 08:33:13PM +0800, Keith Zhao wrote: > > > +static const struct vs_plane_info dc_hw_planes_rev0[PLANE_NUM] = { > > > + { > > > + .name = "Primary", > > > + .id = PRIMARY_PLANE_0, > > > + .type = DRM_PLANE_TYPE_PRIMARY, > > > + .num_formats= ARRAY_SIZE(primary_overlay_format0), > > > + .formats= primary_overlay_format0, > > > + .num_modifiers = ARRAY_SIZE(format_modifier0), > > > + .modifiers = format_modifier0, > > > + .min_width = 0, > > > + .min_height = 0, > > > + .max_width = 4096, > > > + .max_height = 4096, > > > + .rotation = DRM_MODE_ROTATE_0 | > > > + DRM_MODE_ROTATE_90 | > > > + DRM_MODE_ROTATE_180 | > > > + DRM_MODE_ROTATE_270 | > > > + DRM_MODE_REFLECT_X | > > > + DRM_MODE_REFLECT_Y, > > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) | > > > + BIT(DRM_MODE_BLEND_PREMULTI) | > > > + BIT(DRM_MODE_BLEND_COVERAGE), > > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) | > > > + BIT(DRM_COLOR_YCBCR_BT2020), > > > + .degamma_size = DEGAMMA_SIZE, > > > + .min_scale = FRAC_16_16(1, 3), > > > + .max_scale = FRAC_16_16(10, 1), > > > + .zpos = 0, > > > + .watermark = true, > > > + .color_mgmt = true, > > > + .roi= true, > > > + }, > > > + { > > > + .name = "Overlay", > > > + .id = OVERLAY_PLANE_0, > > > + .type = DRM_PLANE_TYPE_OVERLAY, > > > + .num_formats= ARRAY_SIZE(primary_overlay_format0), > > > + .formats= primary_overlay_format0, > > > + .num_modifiers = ARRAY_SIZE(format_modifier0), > > > + .modifiers = format_modifier0, > > > + .min_width = 0, > > > + .min_height = 0, > > > + .max_width = 4096, > > > + .max_height = 4096, > > > + .rotation = DRM_MODE_ROTATE_0 | > > > + DRM_MODE_ROTATE_90 | > > > + DRM_MODE_ROTATE_180 | > > > + DRM_MODE_ROTATE_270 | > > > + DRM_MODE_REFLECT_X | > > > + DRM_MODE_REFLECT_Y, > > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) | > > > + BIT(DRM_MODE_BLEND_PREMULTI) | > > > + BIT(DRM_MODE_BLEND_COVERAGE), > > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) | > > > + BIT(DRM_COLOR_YCBCR_BT2020), > > > + .degamma_size = DEGAMMA_SIZE, > > > + .min_scale = FRAC_16_16(1, 3), > > > + .max_scale = FRAC_16_16(10, 1), > > > + .zpos = 1, > > > + .watermark = true, > > > + .color_mgmt = true, > > > + .roi= true, > > > + }, > > > + { > > > + .name = "Overlay_1", > > > + .id = OVERLAY_PLANE_1, > > > + .type = DRM_PLANE_TYPE_OVERLAY, > > > + .num_formats= ARRAY_SIZE(primary_overlay_format0), > > > + .formats= primary_overlay_format0, > > > + .num_modifiers = > > > ARRAY_SIZE(secondary_format_modifiers), > > > + .modifiers = secondary_format_modifiers, > > > + .min_width = 0, > > > + .min_height = 0, > > > + .max_width = 4096, > > > +
Re: [PATCH 2/6] drm/nouveau/svm: remove unused but set variables
On Wed, 10 Jan 2024, Jani Nikula wrote: > Fix the W=1 warning -Wunused-but-set-variable. > > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: nouv...@lists.freedesktop.org > Signed-off-by: Jani Nikula Ping? > --- > drivers/gpu/drm/nouveau/nouveau_svm.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c > b/drivers/gpu/drm/nouveau/nouveau_svm.c > index cc03e0c22ff3..4d1008915499 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c > @@ -112,7 +112,7 @@ nouveau_svmm_bind(struct drm_device *dev, void *data, > { > struct nouveau_cli *cli = nouveau_cli(file_priv); > struct drm_nouveau_svm_bind *args = data; > - unsigned target, cmd, priority; > + unsigned target, cmd; > unsigned long addr, end; > struct mm_struct *mm; > > @@ -136,9 +136,6 @@ nouveau_svmm_bind(struct drm_device *dev, void *data, > return -EINVAL; > } > > - priority = args->header >> NOUVEAU_SVM_BIND_PRIORITY_SHIFT; > - priority &= NOUVEAU_SVM_BIND_PRIORITY_MASK; > - > /* FIXME support CPU target ie all target value < GPU_VRAM */ > target = args->header >> NOUVEAU_SVM_BIND_TARGET_SHIFT; > target &= NOUVEAU_SVM_BIND_TARGET_MASK; > @@ -926,15 +923,14 @@ nouveau_pfns_map(struct nouveau_svmm *svmm, struct > mm_struct *mm, >unsigned long addr, u64 *pfns, unsigned long npages) > { > struct nouveau_pfnmap_args *args = nouveau_pfns_to_args(pfns); > - int ret; > > args->p.addr = addr; > args->p.size = npages << PAGE_SHIFT; > > mutex_lock(>mutex); > > - ret = nvif_object_ioctl(>vmm->vmm.object, args, > - struct_size(args, p.phys, npages), NULL); > + nvif_object_ioctl(>vmm->vmm.object, args, > + struct_size(args, p.phys, npages), NULL); > > mutex_unlock(>mutex); > } -- Jani Nikula, Intel
Re: [PATCH 1/6] drm/nouveau/acr/ga102: remove unused but set variable
On Wed, 10 Jan 2024, Jani Nikula wrote: > Fix the W=1 warning -Wunused-but-set-variable. > > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: nouv...@lists.freedesktop.org > Signed-off-by: Jani Nikula Ping? > --- > drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c > index f36a359d4531..bd104a030243 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c > @@ -218,7 +218,7 @@ nvkm_acr_lsfw_load_sig_image_desc_v2(struct nvkm_subdev > *subdev, > const struct firmware *hsbl; > const struct nvfw_ls_hsbl_bin_hdr *hdr; > const struct nvfw_ls_hsbl_hdr *hshdr; > - u32 loc, sig, cnt, *meta; > + u32 sig, cnt, *meta; > > ret = nvkm_firmware_load_name(subdev, path, "hs_bl_sig", ver, > ); > if (ret) > @@ -227,7 +227,6 @@ nvkm_acr_lsfw_load_sig_image_desc_v2(struct nvkm_subdev > *subdev, > hdr = nvfw_ls_hsbl_bin_hdr(subdev, hsbl->data); > hshdr = nvfw_ls_hsbl_hdr(subdev, hsbl->data + > hdr->header_offset); > meta = (u32 *)(hsbl->data + hshdr->meta_data_offset); > - loc = *(u32 *)(hsbl->data + hshdr->patch_loc); > sig = *(u32 *)(hsbl->data + hshdr->patch_sig); > cnt = *(u32 *)(hsbl->data + hshdr->num_sig); -- Jani Nikula, Intel
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Tue, 2024-01-23 at 12:28 +0200, Imre Deak wrote: > Add support for Display Port DP tunneling. For now this includes the > support for Bandwidth Allocation Mode, leaving adding Panel Replay > support for later. > > BWA allows using displays that share the same (Thunderbolt) link with > their maximum resolution. Atm, this may not be possible due to the > coarse granularity of partitioning the link BW among the displays on > the > link: the BW allocation policy is in a SW/FW/HW component on the link > (on Thunderbolt it's the SW or FW Connection Manager), independent of > the driver. This policy will set the DPRX maximum rate and lane count > DPCD registers the GFX driver will see (0x0, 0x1, 0x02200, > 0x02201) based on the available link BW. > > The granularity of the current BW allocation policy is course, based > on > the required link rate in the 1.62Gbs..8.1Gbps range and it may > prevent > using higher resolutions all together: the display connected first > will > get a share of the link BW which corresponds to its full DPRX > capability > (regardless of the actual mode it uses). A subsequent display > connected > will only get the remaining BW, which could be well below its full > capability. > > BWA solves the above course granularity (reducing it to a > 250Mbs..1Gps > range) and first-come/first-served issues by letting the driver > request > the BW for each display on a link which reflects the actual modes the > displays use. > > This patch adds the DRM core helper functions, while a follow-up > change > in the patchset takes them into use in the i915 driver. > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/display/Kconfig | 17 + > drivers/gpu/drm/display/Makefile | 2 + > drivers/gpu/drm/display/drm_dp_tunnel.c | 1715 > +++ > include/drm/display/drm_dp.h | 60 + > include/drm/display/drm_dp_tunnel.h | 270 > 5 files changed, 2064 insertions(+) > create mode 100644 drivers/gpu/drm/display/drm_dp_tunnel.c > create mode 100644 include/drm/display/drm_dp_tunnel.h > > diff --git a/drivers/gpu/drm/display/Kconfig > b/drivers/gpu/drm/display/Kconfig > index 09712b88a5b83..b024a84b94c1c 100644 > --- a/drivers/gpu/drm/display/Kconfig > +++ b/drivers/gpu/drm/display/Kconfig > @@ -17,6 +17,23 @@ config DRM_DISPLAY_DP_HELPER > help > DRM display helpers for DisplayPort. > > +config DRM_DISPLAY_DP_TUNNEL > + bool > + select DRM_DISPLAY_DP_HELPER > + help > + Enable support for DisplayPort tunnels. > + > +config DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > + bool "Enable debugging the DP tunnel state" > + depends on REF_TRACKER > + depends on DRM_DISPLAY_DP_TUNNEL > + depends on DEBUG_KERNEL > + depends on EXPERT > + help > + Enables debugging the DP tunnel manager's status. > + > + If in doubt, say "N". > + > config DRM_DISPLAY_HDCP_HELPER > bool > depends on DRM_DISPLAY_HELPER > diff --git a/drivers/gpu/drm/display/Makefile > b/drivers/gpu/drm/display/Makefile > index 17ac4a1006a80..7ca61333c6696 100644 > --- a/drivers/gpu/drm/display/Makefile > +++ b/drivers/gpu/drm/display/Makefile > @@ -8,6 +8,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER) += > \ > drm_dp_helper.o \ > drm_dp_mst_topology.o \ > drm_dsc_helper.o > +drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_TUNNEL) += \ > + drm_dp_tunnel.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += > drm_hdcp_helper.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ > drm_hdmi_helper.o \ > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c > b/drivers/gpu/drm/display/drm_dp_tunnel.c > new file mode 100644 > index 0..58f6330db7d9d > --- /dev/null > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -0,0 +1,1715 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2023 Intel Corporation > + */ > + > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#define to_group(__private_obj) \ > + container_of(__private_obj, struct drm_dp_tunnel_group, base) > + > +#define to_group_state(__private_state) \ > + container_of(__private_state, struct > drm_dp_tunnel_group_state, base) > + > +#define is_dp_tunnel_private_obj(__obj) \ > + ((__obj)->funcs == _group_funcs) > + > +#define for_each_new_group_in_state(__state, __new_group_state, __i) > \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_private_objs; \ > + (__i)++) \ > + for_each_if ((__state)->private_objs[__i].ptr && \ > + is_dp_tunnel_private_obj((__state)- > >private_objs[__i].ptr) && \ > + ((__new_group_state) = \ > + to_group_state((__state)- > >private_objs[__i].new_state), 1)) > + > +#define for_each_old_group_in_state(__state,
Re: [PATCH 4/6] drm/imx: prefer snprintf over sprintf
On Fri, 12 Jan 2024, Philipp Zabel wrote: > Hi Jani, > > On Mi, 2024-01-10 at 19:39 +0200, Jani Nikula wrote: >> This will trade the W=1 warning -Wformat-overflow to >> -Wformat-truncation. This lets us enable -Wformat-overflow subsystem >> wide. >> >> Cc: Philipp Zabel >> Signed-off-by: Jani Nikula > > Reviewed-by: Philipp Zabel Thanks, pushed this one patch to drm-misc-next as prep work. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 3/6] drm/amdgpu: prefer snprintf over sprintf
On Fri, 12 Jan 2024, Alex Deucher wrote: > On Wed, Jan 10, 2024 at 12:39 PM Jani Nikula wrote: >> >> This will trade the W=1 warning -Wformat-overflow to >> -Wformat-truncation. This lets us enable -Wformat-overflow subsystem >> wide. >> >> Cc: Alex Deucher >> Cc: Christian König >> Cc: Pan, Xinhui >> Cc: amd-...@lists.freedesktop.org >> Signed-off-by: Jani Nikula > > Acked-by: Alex Deucher > > Feel free to take this via whichever tree makes sense. Thanks, pushed this one patch to drm-misc-next as prep work. BR, Jani. > > Alex > >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> index b9674c57c436..82b4b2019fca 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> @@ -329,7 +329,8 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev, >> >> ring->eop_gpu_addr = kiq->eop_gpu_addr; >> ring->no_scheduler = true; >> - sprintf(ring->name, "kiq_%d.%d.%d.%d", xcc_id, ring->me, ring->pipe, >> ring->queue); >> + snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", >> +xcc_id, ring->me, ring->pipe, ring->queue); >> r = amdgpu_ring_init(adev, ring, 1024, irq, >> AMDGPU_CP_KIQ_IRQ_DRIVER0, >> AMDGPU_RING_PRIO_DEFAULT, NULL); >> if (r) >> -- >> 2.39.2 >> -- Jani Nikula, Intel
Re: [v3 2/3] ASoC: mediatek: mt8186: correct the HDMI widgets
Il 31/01/24 13:25, Jiaxin Yu (俞家鑫) ha scritto: On Wed, 2024-01-31 at 12:42 +0100, AngeloGioacchino Del Regno wrote: Il 03/08/23 21:33, Mark Brown ha scritto: On Thu, Aug 03, 2023 at 07:20:15AM +, Jiaxin Yu (俞家鑫) wrote: I agree with you, in fact the speaker is indeed doing this way. But about the hdmi that on the board, I did not find a defination link snd_soc_dapm_hdmi, so I use snd_soc_dapm_line to replace. The purpose is to control it link speaker. Or what do you suggest I should do? I think the sensible thing here is to define a DIGITAL_OUTPUT() which can be used for HDMI, S/PDIF and anything else that comes up and isn't clearly wrong like reusing one of the analog descriptions is. Hello Jiaxin, the MT8186 Corsola Chromebooks are broken upstream without this series. Are you still interested in upstreaming this one? Thanks, Angelo Hello Angelo, No, I'm still interesting in upstream this series. It's just that I have less time recently. I'm considering revisiting this issue next mouth. Do you have any suggestions for this? Nothing on top of Mark's suggestions. Angelo
Re: [v3 2/3] ASoC: mediatek: mt8186: correct the HDMI widgets
Re: [v3 2/3] ASoC: mediatek: mt8186: correct the HDMI widgets
Il 03/08/23 21:33, Mark Brown ha scritto: On Thu, Aug 03, 2023 at 07:20:15AM +, Jiaxin Yu (俞家鑫) wrote: I agree with you, in fact the speaker is indeed doing this way. But about the hdmi that on the board, I did not find a defination link snd_soc_dapm_hdmi, so I use snd_soc_dapm_line to replace. The purpose is to control it link speaker. Or what do you suggest I should do? I think the sensible thing here is to define a DIGITAL_OUTPUT() which can be used for HDMI, S/PDIF and anything else that comes up and isn't clearly wrong like reusing one of the analog descriptions is. Hello Jiaxin, the MT8186 Corsola Chromebooks are broken upstream without this series. Are you still interested in upstreaming this one? Thanks, Angelo
Future handling of complex RGB devices on Linux
Hi, so I combined Hans last draft, with the discussion since then and the comments from the OpenRGB maintainers from here https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916 and my own experience and came up witrh this rough updated draft for the new uapi: Future handling of complex RGB devices on Linux: Optional: Provide a basic leds-subsystem driver: - The whole device is treated as a singular RGB led in the current leds uapi - Backwards compatibility - Have something work out-of-the-box and during boot time - The driver also registers a misc device with a singluar sysfs attribute select_uapi - reading this gives back "[leds] none" - the current active uapi can be selected by writing it to that attribute - switching the uapi means deregistering the device from that entirely and registering and initializing it with the new one froms scratch - selecting none only does the deregistering If the device is already reachable by userspace directly, e.g. via hidraw, the kernel will only offer this basic implementation and a more complex driver has to be implemented in userspace. - This driver has to use the select_uapi attribute first and select "none" to avoid undefined behaviour caused by accessing the leds upai and hidraw to control the lighting at the same time - Question: How to best associate the select_uapi attribute to the corresponding hidraw (or other) direct access channel? So that a userspace driver can reliable check whether or not this has to be set. Devices not reachable by userspace directly, e.g. because they are controled via a wmi interface, can also be implemented in the new rgbledstring-subsystem (working title) for more complex control - a rgbledstring device provides an ioctl interface (ioctl only no r/w) using /dev/rgbledstring0, /dev/rgbledstring1, etc. registered as a misc chardev. - get-device-info ioctl which returns in a single struct: - char name[64] /* Device model name / identifier, preferable human readable. For keyboards, if known to the driver, physical layout (or even printed layout) should be separated here */ - enum device_type /* e.g. keyboard, mouse, lightbar, etc. */ - char firmware_version_string[64] /* if known to the driver, empty otherwise */ - char serial_number[64] /* if known to the driver, empty otherwise */ - enum supported_modes[64] /* modes supported by the firmware e.g. static/direct, breathing, scan, raindrops, etc. */ - get-mode-info icotl, RFC here: Hans thinks it is better to have the modes and their inputs staticly defined and have, if required, something like breathing_clevo_1, breathing_clevo_2, breathing_tongfang_1 if the inputs vary between vendors. I think a dynamic approach could be useful where userspace just queries the struct required for each individual mode. - input: a mode from the supported_modes extracted from get-device-info - output: static information about the mode, e.g. max_animation_speed, max_brightness, etc. - output: the struct/a list of attributes and their types required to configure the mode - set-mode ioctl takes a single struct: - enum mode /* from supported_modes */ - union data - char raw[3072] - - The driver also registers a singluar sysfs attribute select_uapi - reading this gives back "[leds] rgbledstring none" or "[rgbledstring] none" respectifly - Discussion question: should select_uapi instead be use_leds_uapi - if 1: use basic leds driver - if 0: if device is userspace accessible no kernel driver is active, if device ist not userspace accessible register rgbledstring (aka implicit separation between rgbledstring and none instead of explicit one) Zone configuration would be seen as a subset of mode configuration, as I suspect not every mode needs the zone configuration even on devices that offer it? The most simple mode would be static/direct and the set-mode struct would look like this: { enum mode, /* = static */ { uint8 brightness, /* global brightness, some keyboards offer this */ uint8 color[*3] } } Question: Are there modes that have a separate setup command that is only required once and then a continuous stream of update information? If yes, should we reflect that by splitting set-mode into set-mode-setup and set-mode-update (with get-mode-info returning one struct for each)? Or should userspace just always send setup and update information and it's up to the kernel driver to only resend the setup command when something has changed? In the former case set-mode-update might be a noop in most cases. Discussion on this might also happen
[PATCH v3 3/7] drm/mediatek: dsi: Use bitfield macros where useful
Instead of open coding bitshifting for various register fields, use the bitfield macro FIELD_PREP(): this allows to enhance the human readability, decrease likeliness of mistakes (and register field overflowing) and also to simplify the code. The latter is especially seen in mtk_dsi_rxtx_control(), where it was possible to change a switch to a short for loop and to also remove the need to check for maximum DSI lanes == 4 thanks to the FIELD_PREP macro masking the value. While at it, also add the missing DA_HS_SYNC bitmask, used in mtk_dsi_phy_timconfig(). Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 97 -- 1 file changed, 52 insertions(+), 45 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 8414ce73ce9f..cba63de5092d 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -3,6 +3,7 @@ * Copyright (c) 2015 MediaTek Inc. */ +#include #include #include #include @@ -70,16 +71,19 @@ #define DSI_PSCTRL 0x1c #define DSI_PS_WC GENMASK(14, 0) #define DSI_PS_SEL GENMASK(19, 16) -#define PACKED_PS_16BIT_RGB565 (0 << 16) -#define LOOSELY_PS_18BIT_RGB666(1 << 16) -#define PACKED_PS_18BIT_RGB666 (2 << 16) -#define PACKED_PS_24BIT_RGB888 (3 << 16) +#define PACKED_PS_16BIT_RGB565 0 +#define LOOSELY_PS_18BIT_RGB6661 +#define PACKED_PS_18BIT_RGB666 2 +#define PACKED_PS_24BIT_RGB888 3 #define DSI_VSA_NL 0x20 #define DSI_VBP_NL 0x24 #define DSI_VFP_NL 0x28 #define DSI_VACT_NL0x2C +#define VACT_NLGENMASK(14, 0) #define DSI_SIZE_CON 0x38 +#define DSI_HEIGHT GENMASK(30, 16) +#define DSI_WIDTH GENMASK(14, 0) #define DSI_HSA_WC 0x50 #define DSI_HBP_WC 0x54 #define DSI_HFP_WC 0x58 @@ -122,6 +126,7 @@ #define DSI_PHY_TIMECON2 0x118 #define CONT_DET GENMASK(7, 0) +#define DA_HS_SYNC GENMASK(15, 8) #define CLK_ZERO GENMASK(23, 16) #define CLK_TRAIL GENMASK(31, 24) @@ -253,14 +258,23 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi) timing->clk_hs_zero = timing->clk_hs_trail * 4; timing->clk_hs_exit = 2 * timing->clk_hs_trail; - timcon0 = timing->lpx | timing->da_hs_prepare << 8 | - timing->da_hs_zero << 16 | timing->da_hs_trail << 24; - timcon1 = timing->ta_go | timing->ta_sure << 8 | - timing->ta_get << 16 | timing->da_hs_exit << 24; - timcon2 = 1 << 8 | timing->clk_hs_zero << 16 | - timing->clk_hs_trail << 24; - timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 | - timing->clk_hs_exit << 16; + timcon0 = FIELD_PREP(LPX, timing->lpx) | + FIELD_PREP(HS_PREP, timing->da_hs_prepare) | + FIELD_PREP(HS_ZERO, timing->da_hs_zero) | + FIELD_PREP(HS_TRAIL, timing->da_hs_trail); + + timcon1 = FIELD_PREP(TA_GO, timing->ta_go) | + FIELD_PREP(TA_SURE, timing->ta_sure) | + FIELD_PREP(TA_GET, timing->ta_get) | + FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit); + + timcon2 = FIELD_PREP(DA_HS_SYNC, 1) | + FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) | + FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail); + + timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) | + FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) | + FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit); writel(timcon0, dsi->regs + DSI_PHY_TIMECON0); writel(timcon1, dsi->regs + DSI_PHY_TIMECON1); @@ -353,69 +367,61 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi) static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) { - u32 tmp_reg; + u32 regval, tmp_reg = 0; + u8 i; - switch (dsi->lanes) { - case 1: - tmp_reg = 1 << 2; - break; - case 2: - tmp_reg = 3 << 2; - break; - case 3: - tmp_reg = 7 << 2; - break; - case 4: - tmp_reg = 0xf << 2; - break; - default: - tmp_reg = 0xf << 2; - break; - } + /* Number of DSI lanes (max 4 lanes), each bit enables one DSI lane. */ + for (i = 0; i < dsi->lanes; i++) + tmp_reg |= BIT(i); + + regval = FIELD_PREP(LANE_NUM, tmp_reg); if (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) - tmp_reg |= HSTX_CKLP_EN; + regval |= HSTX_CKLP_EN; if (dsi->mode_flags &
[PATCH v3 6/7] drm/mediatek: dsi: Simplify with dev_err_probe and remove gotos
Most of the functions that are called in the probe callback are devm managed, or all but mipi_dsi_host_register(): simplify the probe function's error paths with dev_err_probe() and remove the lonely instance of `goto err_unregister_host` by just directly calling the mipi_dsi_host_unregister() function in the devm_request_irq() error path, allowing to also remove the same label. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 60 +- 1 file changed, 18 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 709a65656b79..5a0f078987d3 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -1117,64 +1117,44 @@ static int mtk_dsi_probe(struct platform_device *pdev) dsi->driver_data = of_device_get_match_data(dev); dsi->engine_clk = devm_clk_get(dev, "engine"); - if (IS_ERR(dsi->engine_clk)) { - ret = PTR_ERR(dsi->engine_clk); + if (IS_ERR(dsi->engine_clk)) + return dev_err_probe(dev, PTR_ERR(dsi->engine_clk), +"Failed to get engine clock\n"); - if (ret != -EPROBE_DEFER) - dev_err(dev, "Failed to get engine clock: %d\n", ret); - return ret; - } dsi->digital_clk = devm_clk_get(dev, "digital"); - if (IS_ERR(dsi->digital_clk)) { - ret = PTR_ERR(dsi->digital_clk); - - if (ret != -EPROBE_DEFER) - dev_err(dev, "Failed to get digital clock: %d\n", ret); - return ret; - } + if (IS_ERR(dsi->digital_clk)) + return dev_err_probe(dev, PTR_ERR(dsi->digital_clk), +"Failed to get digital clock\n"); dsi->hs_clk = devm_clk_get(dev, "hs"); - if (IS_ERR(dsi->hs_clk)) { - ret = PTR_ERR(dsi->hs_clk); - dev_err(dev, "Failed to get hs clock: %d\n", ret); - return ret; - } + if (IS_ERR(dsi->hs_clk)) + return dev_err_probe(dev, PTR_ERR(dsi->hs_clk), "Failed to get hs clock\n"); regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); dsi->regs = devm_ioremap_resource(dev, regs); - if (IS_ERR(dsi->regs)) { - ret = PTR_ERR(dsi->regs); - dev_err(dev, "Failed to ioremap memory: %d\n", ret); - return ret; - } + if (IS_ERR(dsi->regs)) + return dev_err_probe(dev, PTR_ERR(dsi->regs), "Failed to ioremap memory\n"); dsi->phy = devm_phy_get(dev, "dphy"); - if (IS_ERR(dsi->phy)) { - ret = PTR_ERR(dsi->phy); - dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret); - return ret; - } + if (IS_ERR(dsi->phy)) + return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get MIPI-DPHY\n"); irq_num = platform_get_irq(pdev, 0); - if (irq_num < 0) { - ret = irq_num; - return ret; - } + if (irq_num < 0) + return irq_num; dsi->host.ops = _dsi_ops; dsi->host.dev = dev; ret = mipi_dsi_host_register(>host); - if (ret < 0) { - dev_err(dev, "failed to register DSI host: %d\n", ret); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to register DSI host\n"); ret = devm_request_irq(>dev, irq_num, mtk_dsi_irq, IRQF_TRIGGER_NONE, dev_name(>dev), dsi); if (ret) { - dev_err(>dev, "failed to request mediatek dsi irq\n"); - goto err_unregister_host; + mipi_dsi_host_unregister(>host); + return dev_err_probe(>dev, ret, "Failed to request DSI irq\n"); } init_waitqueue_head(>irq_wait_queue); @@ -1186,10 +1166,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; return 0; - -err_unregister_host: - mipi_dsi_host_unregister(>host); - return ret; } static void mtk_dsi_remove(struct platform_device *pdev) -- 2.43.0
[PATCH v3 4/7] drm/mediatek: dsi: Replace open-coded instance of HZ_PER_MHZ
In mtk_dsi_phy_timconfig(), we're dividing the `data_rate` variable, expressed in Hz to retrieve a value in MHz: instead of open-coding, use the HZ_PER_MHZ definition, available in linux/units.h. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index cba63de5092d..cc625febe6c8 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -238,7 +239,7 @@ static void mtk_dsi_mask(struct mtk_dsi *dsi, u32 offset, u32 mask, u32 data) static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi) { u32 timcon0, timcon1, timcon2, timcon3; - u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, 100); + u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, HZ_PER_MHZ); struct mtk_phy_timing *timing = >phy_timing; timing->lpx = (60 * data_rate_mhz / (8 * 1000)) + 1; -- 2.43.0
[PATCH v3 2/7] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()
Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact(): merge the two in one mtk_dsi_ps_control() function by adding one function parameter `config_vact` which, when true, writes the VACT related registers. Reviewed-by: Fei Shao Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +- 1 file changed, 23 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 3b7392c03b4d..8414ce73ce9f 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -351,40 +351,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi) mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN); } -static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi) -{ - struct videomode *vm = >vm; - u32 dsi_buf_bpp, ps_wc; - u32 ps_bpp_mode; - - if (dsi->format == MIPI_DSI_FMT_RGB565) - dsi_buf_bpp = 2; - else - dsi_buf_bpp = 3; - - ps_wc = vm->hactive * dsi_buf_bpp; - ps_bpp_mode = ps_wc; - - switch (dsi->format) { - case MIPI_DSI_FMT_RGB888: - ps_bpp_mode |= PACKED_PS_24BIT_RGB888; - break; - case MIPI_DSI_FMT_RGB666: - ps_bpp_mode |= PACKED_PS_18BIT_RGB666; - break; - case MIPI_DSI_FMT_RGB666_PACKED: - ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666; - break; - case MIPI_DSI_FMT_RGB565: - ps_bpp_mode |= PACKED_PS_16BIT_RGB565; - break; - } - - writel(vm->vactive, dsi->regs + DSI_VACT_NL); - writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL); - writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC); -} - static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) { u32 tmp_reg; @@ -416,36 +382,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL); } -static void mtk_dsi_ps_control(struct mtk_dsi *dsi) +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact) { - u32 dsi_tmp_buf_bpp; - u32 tmp_reg; + struct videomode *vm = >vm; + u32 dsi_buf_bpp, ps_wc; + u32 ps_bpp_mode; + + if (dsi->format == MIPI_DSI_FMT_RGB565) + dsi_buf_bpp = 2; + else + dsi_buf_bpp = 3; + + ps_wc = vm->hactive * dsi_buf_bpp; + ps_bpp_mode = ps_wc; switch (dsi->format) { case MIPI_DSI_FMT_RGB888: - tmp_reg = PACKED_PS_24BIT_RGB888; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= PACKED_PS_24BIT_RGB888; break; case MIPI_DSI_FMT_RGB666: - tmp_reg = LOOSELY_PS_18BIT_RGB666; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= PACKED_PS_18BIT_RGB666; break; case MIPI_DSI_FMT_RGB666_PACKED: - tmp_reg = PACKED_PS_18BIT_RGB666; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666; break; case MIPI_DSI_FMT_RGB565: - tmp_reg = PACKED_PS_16BIT_RGB565; - dsi_tmp_buf_bpp = 2; - break; - default: - tmp_reg = PACKED_PS_24BIT_RGB888; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= PACKED_PS_16BIT_RGB565; break; } - tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC; - writel(tmp_reg, dsi->regs + DSI_PSCTRL); + if (config_vact) { + writel(vm->vactive, dsi->regs + DSI_VACT_NL); + writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC); + } + writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL); } static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) @@ -521,7 +491,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC); writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC); - mtk_dsi_ps_control(dsi); + mtk_dsi_ps_control(dsi, false); } static void mtk_dsi_start(struct mtk_dsi *dsi) @@ -666,7 +636,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) mtk_dsi_reset_engine(dsi); mtk_dsi_phy_timconfig(dsi); - mtk_dsi_ps_control_vact(dsi); + mtk_dsi_ps_control(dsi, true); mtk_dsi_set_vm_cmd(dsi); mtk_dsi_config_vdo_timing(dsi); mtk_dsi_set_interrupt_enable(dsi); -- 2.43.0
[PATCH v3 1/7] drm/mediatek: dsi: Use GENMASK() for register mask definitions
Change magic numerical masks with usage of the GENMASK() macro to improve readability. While at it, also fix the DSI_PS_SEL mask to include all bits instead of just a subset of them. This commit brings no functional changes. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++--- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index a2fdfc8ddb15..3b7392c03b4d 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -58,18 +58,18 @@ #define DSI_TXRX_CTRL 0x18 #define VC_NUM BIT(1) -#define LANE_NUM (0xf << 2) +#define LANE_NUM GENMASK(5, 2) #define DIS_EOTBIT(6) #define NULL_ENBIT(7) #define TE_FREERUN BIT(8) #define EXT_TE_EN BIT(9) #define EXT_TE_EDGEBIT(10) -#define MAX_RTN_SIZE (0xf << 12) +#define MAX_RTN_SIZE GENMASK(15, 12) #define HSTX_CKLP_EN BIT(16) #define DSI_PSCTRL 0x1c -#define DSI_PS_WC 0x3fff -#define DSI_PS_SEL (3 << 16) +#define DSI_PS_WC GENMASK(14, 0) +#define DSI_PS_SEL GENMASK(19, 16) #define PACKED_PS_16BIT_RGB565 (0 << 16) #define LOOSELY_PS_18BIT_RGB666(1 << 16) #define PACKED_PS_18BIT_RGB666 (2 << 16) @@ -109,26 +109,26 @@ #define LD0_WAKEUP_EN BIT(2) #define DSI_PHY_TIMECON0 0x110 -#define LPX(0xff << 0) -#define HS_PREP(0xff << 8) -#define HS_ZERO(0xff << 16) -#define HS_TRAIL (0xff << 24) +#define LPXGENMASK(7, 0) +#define HS_PREPGENMASK(15, 8) +#define HS_ZEROGENMASK(23, 16) +#define HS_TRAIL GENMASK(31, 24) #define DSI_PHY_TIMECON1 0x114 -#define TA_GO (0xff << 0) -#define TA_SURE(0xff << 8) -#define TA_GET (0xff << 16) -#define DA_HS_EXIT (0xff << 24) +#define TA_GO GENMASK(7, 0) +#define TA_SUREGENMASK(15, 8) +#define TA_GET GENMASK(23, 16) +#define DA_HS_EXIT GENMASK(31, 24) #define DSI_PHY_TIMECON2 0x118 -#define CONT_DET (0xff << 0) -#define CLK_ZERO (0xff << 16) -#define CLK_TRAIL (0xff << 24) +#define CONT_DET GENMASK(7, 0) +#define CLK_ZERO GENMASK(23, 16) +#define CLK_TRAIL GENMASK(31, 24) #define DSI_PHY_TIMECON3 0x11c -#define CLK_HS_PREP(0xff << 0) -#define CLK_HS_POST(0xff << 8) -#define CLK_HS_EXIT(0xff << 16) +#define CLK_HS_PREPGENMASK(7, 0) +#define CLK_HS_POSTGENMASK(15, 8) +#define CLK_HS_EXITGENMASK(23, 16) #define DSI_VM_CMD_CON 0x130 #define VM_CMD_EN BIT(0) @@ -138,13 +138,14 @@ #define FORCE_COMMIT BIT(0) #define BYPASS_SHADOW BIT(1) -#define CONFIG (0xff << 0) +/* CMDQ related bits */ +#define CONFIG GENMASK(7, 0) #define SHORT_PACKET 0 #define LONG_PACKET2 #define BTABIT(2) -#define DATA_ID(0xff << 8) -#define DATA_0 (0xff << 16) -#define DATA_1 (0xff << 24) +#define DATA_IDGENMASK(15, 8) +#define DATA_0 GENMASK(23, 16) +#define DATA_1 GENMASK(31, 24) #define NS_TO_CYCLE(n, c)((n) / (c) + (((n) % (c)) ? 1 : 0)) -- 2.43.0
[PATCH v3 5/7] drm/mediatek: dsi: Register DSI host after acquiring clocks and PHY
Registering the dsi host with its ops before getting dsi->regs is simply wrong: even though there's nothing (for now) asynchronously calling those ops before the end of the probe function, installing ops that are using iospace(s) and clocks before even initializing those is too fragile. Register the DSI host after getting clocks, iospace and PHY. This wil also allow to simplify the error paths in a later commit. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index cc625febe6c8..709a65656b79 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -1114,14 +1114,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) if (!dsi) return -ENOMEM; - dsi->host.ops = _dsi_ops; - dsi->host.dev = dev; - ret = mipi_dsi_host_register(>host); - if (ret < 0) { - dev_err(dev, "failed to register DSI host: %d\n", ret); - return ret; - } - dsi->driver_data = of_device_get_match_data(dev); dsi->engine_clk = devm_clk_get(dev, "engine"); @@ -1130,7 +1122,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) if (ret != -EPROBE_DEFER) dev_err(dev, "Failed to get engine clock: %d\n", ret); - goto err_unregister_host; + return ret; } dsi->digital_clk = devm_clk_get(dev, "digital"); @@ -1139,14 +1131,14 @@ static int mtk_dsi_probe(struct platform_device *pdev) if (ret != -EPROBE_DEFER) dev_err(dev, "Failed to get digital clock: %d\n", ret); - goto err_unregister_host; + return ret; } dsi->hs_clk = devm_clk_get(dev, "hs"); if (IS_ERR(dsi->hs_clk)) { ret = PTR_ERR(dsi->hs_clk); dev_err(dev, "Failed to get hs clock: %d\n", ret); - goto err_unregister_host; + return ret; } regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1154,20 +1146,28 @@ static int mtk_dsi_probe(struct platform_device *pdev) if (IS_ERR(dsi->regs)) { ret = PTR_ERR(dsi->regs); dev_err(dev, "Failed to ioremap memory: %d\n", ret); - goto err_unregister_host; + return ret; } dsi->phy = devm_phy_get(dev, "dphy"); if (IS_ERR(dsi->phy)) { ret = PTR_ERR(dsi->phy); dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret); - goto err_unregister_host; + return ret; } irq_num = platform_get_irq(pdev, 0); if (irq_num < 0) { ret = irq_num; - goto err_unregister_host; + return ret; + } + + dsi->host.ops = _dsi_ops; + dsi->host.dev = dev; + ret = mipi_dsi_host_register(>host); + if (ret < 0) { + dev_err(dev, "failed to register DSI host: %d\n", ret); + return ret; } ret = devm_request_irq(>dev, irq_num, mtk_dsi_irq, -- 2.43.0
[PATCH v3 7/7] drm/mediatek: dsi: Compress of_device_id entries and add sentinel
All entries fit in 82 columns, which is acceptable: compress all of the mtk_dsi_of_match[] entries to a single line for each. While at it, also add the usual sentinel comment to the last entry. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 17 ++--- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 5a0f078987d3..2c482742183e 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -1204,17 +1204,12 @@ static const struct mtk_dsi_driver_data mt8188_dsi_driver_data = { }; static const struct of_device_id mtk_dsi_of_match[] = { - { .compatible = "mediatek,mt2701-dsi", - .data = _dsi_driver_data }, - { .compatible = "mediatek,mt8173-dsi", - .data = _dsi_driver_data }, - { .compatible = "mediatek,mt8183-dsi", - .data = _dsi_driver_data }, - { .compatible = "mediatek,mt8186-dsi", - .data = _dsi_driver_data }, - { .compatible = "mediatek,mt8188-dsi", - .data = _dsi_driver_data }, - { }, + { .compatible = "mediatek,mt2701-dsi", .data = _dsi_driver_data }, + { .compatible = "mediatek,mt8173-dsi", .data = _dsi_driver_data }, + { .compatible = "mediatek,mt8183-dsi", .data = _dsi_driver_data }, + { .compatible = "mediatek,mt8186-dsi", .data = _dsi_driver_data }, + { .compatible = "mediatek,mt8188-dsi", .data = _dsi_driver_data }, + { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, mtk_dsi_of_match); -- 2.43.0
[PATCH v3 0/7] MediaTek DRM - DSI driver cleanups
Changes in v3: - Rebased over next-20240131 - Added bitfield.h inclusion in patch 3 - Added three more cleanup commits to the mix to simplify the probe function and remove gotos. Changes in v2: - Rebased over next-20231213 This series performs some cleanups for mtk_dsi, enhancing human readability, using kernel provided macros where possible and also reducing code size. Tested on MT8173 and MT8192 Chromebooks (using a DSI<->DP bridge) and on MT6795 Sony Xperia M5 (DSI video mode panel). AngeloGioacchino Del Regno (7): drm/mediatek: dsi: Use GENMASK() for register mask definitions drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}() drm/mediatek: dsi: Use bitfield macros where useful drm/mediatek: dsi: Replace open-coded instance of HZ_PER_MHZ drm/mediatek: dsi: Register DSI host after acquiring clocks and PHY drm/mediatek: dsi: Simplify with dev_err_probe and remove gotos drm/mediatek: dsi: Compress of_device_id entries and add sentinel drivers/gpu/drm/mediatek/mtk_dsi.c | 284 - 1 file changed, 117 insertions(+), 167 deletions(-) -- 2.43.0
Re: [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
On Wed, Jan 31, 2024 at 12:26:45PM +0200, Dmitry Baryshkov wrote: > On Wed, 31 Jan 2024 at 11:11, Daniel Vetter wrote: > > > > On Wed, Jan 31, 2024 at 05:17:08AM +, Jason-JH Lin (林睿祥) wrote: > > > On Thu, 2024-01-25 at 19:17 +0100, Daniel Vetter wrote: > > > > > > > > External email : Please do not click links or open attachments until > > > > you have verified the sender or the content. > > > > On Tue, Jan 23, 2024 at 06:09:05AM +, Jason-JH Lin (林睿祥) wrote: > > > > > Hi Maxime, Daniel, > > > > > > > > > > We encountered similar issue with mediatek SoCs. > > > > > > > > > > We have found that in drm_atomic_helper_commit_rpm(), when > > > > disabling > > > > > the cursor plane, the old_state->legacy_cursor_update in > > > > > drm_atomic_wait_for_vblank() is set to true. > > > > > As the result, we are not actually waiting for a vlbank to wait for > > > > our > > > > > hardware to close the cursor plane. Subsequently, the execution > > > > > proceeds to drm_atomic_helper_cleanup_planes() to free the cursor > > > > > buffer. This can lead to use-after-free issues with our hardware. > > > > > > > > > > Could you please apply this patch to fix our problem? > > > > > Or are there any considerations for not applying this patch? > > > > > > > > Mostly it needs someone to collect a pile of acks/tested-by and then > > > > land > > > > it. > > > > > > > > > > Got it. I would add tested-by tag for mediatek SoC. > > > > > > > I'd be _very_ happy if someone else can take care of that ... > > > > > > > > There's also the potential issue that it might slow down some of the > > > > legacy X11 use-cases that really needed a non-blocking cursor, but I > > > > think > > > > all the drivers where this matters have switched over to the async > > > > plane > > > > update stuff meanwhile. So hopefully that's good. > > > > > > > > > > I think all the drivers should have switched to async plane update. > > > > > > Can we add the checking condition to see if atomic_async_update/check > > > function are implemented? > > > > Pretty sure not all have done that, so really it boils down to whether we > > break a real user's use-case. Which pretty much can only be checked by > > merging the patch (hence the requirement to get as many acks as possible > > from display drivers) and then being willing to handle any fallout that's > > reported as regressions for a specific driver. > > > > It's a pile of work, at least when it goes south, that's why I'm looking > > for volunteers. > > I can check this on all sensible msm generations, including mdp4, but > it will be next week, after the FOSDEM. > > BTW, for technical reasons one of the msm platforms still has the > legacy cursor implementation might it be related? Yeah, msm is one of the drivers I had to change with some hacks to avoid really bad fallout. It should still work like before, but that's one that definitely needs testing. -Sima > > > > > Note that handling the fallout doesn't mean you have to fix that specific > > driver, the only realistic option might be to reinstate the legacy cursor > > behaviour, but as an explicit opt-in that only that specific driver > > enables. > > > > So maybe for next round of that patch it might be good to have a 2nd patch > > which implements this fallback plan in the shared atomic modeset code? > > > > Cheers, Sima > > > -- > With best wishes > Dmitry -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH AUTOSEL 6.1 04/53] drm: Fix color LUT rounding
On Tue, Jan 30, 2024 at 06:00:18PM -0500, Sasha Levin wrote: > On Mon, Jan 22, 2024 at 06:50:00PM +0200, Ville Syrjälä wrote: > >On Mon, Jan 22, 2024 at 10:08:05AM -0500, Sasha Levin wrote: > >> From: Ville Syrjälä > >> > >> [ Upstream commit c6fbb6bca10838485b820e8a26c23996f77ce580 ] > > > >Why is this being backported? > > Is this not a fix for an issue? It is, for a very minor one that's not really going to hurt anyone. But it doesn't exist in a vacuum so backprting it alone can introduce other issues that people will actually notice. If I wanted it backported I would have have tagged it as such. -- Ville Syrjälä Intel
Re: Re: Re: [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
Hi, On Wed, Jan 31, 2024 at 05:27:14AM +, Jason-JH Lin (林睿祥) wrote: > > On Sun, 2024-01-28 at 10:24 +0100, Maxime Ripard wrote: > > On Thu, Jan 25, 2024 at 07:17:21PM +0100, Daniel Vetter wrote: > > > On Tue, Jan 23, 2024 at 06:09:05AM +, Jason-JH Lin (林睿祥) wrote: > > > > Hi Maxime, Daniel, > > > > > > > > We encountered similar issue with mediatek SoCs. > > > > > > > > We have found that in drm_atomic_helper_commit_rpm(), when > > > > disabling > > > > the cursor plane, the old_state->legacy_cursor_update in > > > > drm_atomic_wait_for_vblank() is set to true. > > > > As the result, we are not actually waiting for a vlbank to wait > > > > for our > > > > hardware to close the cursor plane. Subsequently, the execution > > > > proceeds to drm_atomic_helper_cleanup_planes() to free the > > > > cursor > > > > buffer. This can lead to use-after-free issues with our hardware. > > > > > > > > Could you please apply this patch to fix our problem? > > > > Or are there any considerations for not applying this patch? > > > > > > Mostly it needs someone to collect a pile of acks/tested-by and > > > then land > > > it. > > > > > > I'd be _very_ happy if someone else can take care of that ... > > > > > > There's also the potential issue that it might slow down some of > > > the > > > legacy X11 use-cases that really needed a non-blocking cursor, but > > > I think > > > all the drivers where this matters have switched over to the async > > > plane > > > update stuff meanwhile. So hopefully that's good. > > > > I think there was also a regression with msm no one really figured > > out? > > OK... > But I am only available on MediaTek platform. I think most of us are in that situation, and which is part of the reason it kind of stalled :) > Does it also causes a regression with msn if I re-send a patch for > drm_atomic_helper.c only? Yes, that's my recollection at least. Fortunately, Dmitry might be able to clear that up. Maxime signature.asc Description: PGP signature
Re: [PATCH v2 2/4] drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()
Il 26/12/23 11:48, Fei Shao ha scritto: Hi Angelo, On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno wrote: Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact(): merge the two in one mtk_dsi_ps_control() function by adding one function parameter `config_vact` which, when true, writes the VACT related registers. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +- 1 file changed, 23 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 23d2c5be8dbb..b618e2e31022 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi) mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN); } -static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi) -{ - struct videomode *vm = >vm; - u32 dsi_buf_bpp, ps_wc; - u32 ps_bpp_mode; - - if (dsi->format == MIPI_DSI_FMT_RGB565) - dsi_buf_bpp = 2; - else - dsi_buf_bpp = 3; - - ps_wc = vm->hactive * dsi_buf_bpp; - ps_bpp_mode = ps_wc; - - switch (dsi->format) { - case MIPI_DSI_FMT_RGB888: - ps_bpp_mode |= PACKED_PS_24BIT_RGB888; - break; - case MIPI_DSI_FMT_RGB666: - ps_bpp_mode |= PACKED_PS_18BIT_RGB666; - break; - case MIPI_DSI_FMT_RGB666_PACKED: - ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666; - break; - case MIPI_DSI_FMT_RGB565: - ps_bpp_mode |= PACKED_PS_16BIT_RGB565; - break; - } - - writel(vm->vactive, dsi->regs + DSI_VACT_NL); - writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL); - writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC); -} - static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) { u32 tmp_reg; @@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL); } -static void mtk_dsi_ps_control(struct mtk_dsi *dsi) +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact) { - u32 dsi_tmp_buf_bpp; - u32 tmp_reg; + struct videomode *vm = >vm; + u32 dsi_buf_bpp, ps_wc; + u32 ps_bpp_mode; + + if (dsi->format == MIPI_DSI_FMT_RGB565) + dsi_buf_bpp = 2; + else + dsi_buf_bpp = 3; The same is also in mtk_dsi_config_vdo_timing(). Given this is a cleanup series, I think it'd be a good chance to add another patch and integrate those usages. Just a thought. :) Checking that right now. + + ps_wc = vm->hactive * dsi_buf_bpp; I noticed the "& DSI_PS_WC" part was dropped (but perhaps with awareness?). While the outcome seems to always fall within the range of DSI_PS_WC so we should be fine in practice, I think it doesn't hurt to keep the value masked to save readers some time to check if this would ever be possible to overflow and write undesired bits down the line. WDYT? Masking a pre-masked value doesn't look right. Besides, as for the concern of overflowing, if we masked that we'd still end up with broken functionality, as if the value is invalid... well, it's invalid. Masked or not. :-) Thanks for the R-b, sending a v3 soon with some fixes. Regards, Angelo Regardless of that, I didn't find obvious issue in this patch, so Reviewed-by: Fei Shao Regards, Fei + ps_bpp_mode = ps_wc; switch (dsi->format) { case MIPI_DSI_FMT_RGB888: - tmp_reg = PACKED_PS_24BIT_RGB888; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= PACKED_PS_24BIT_RGB888; break; case MIPI_DSI_FMT_RGB666: - tmp_reg = LOOSELY_PS_18BIT_RGB666; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= PACKED_PS_18BIT_RGB666; break; case MIPI_DSI_FMT_RGB666_PACKED: - tmp_reg = PACKED_PS_18BIT_RGB666; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666; break; case MIPI_DSI_FMT_RGB565: - tmp_reg = PACKED_PS_16BIT_RGB565; - dsi_tmp_buf_bpp = 2; - break; - default: - tmp_reg = PACKED_PS_24BIT_RGB888; - dsi_tmp_buf_bpp = 3; + ps_bpp_mode |= PACKED_PS_16BIT_RGB565; break; } - tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC; - writel(tmp_reg, dsi->regs + DSI_PSCTRL); + if (config_vact) { + writel(vm->vactive, dsi->regs + DSI_VACT_NL); + writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC); + } + writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL); } static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) @@ -522,7 +492,7 @@ static void
Re: [PATCH v2 1/4] drm/mediatek: dsi: Use GENMASK() for register mask definitions
Il 26/12/23 11:46, Fei Shao ha scritto: Hi Angelo, On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno wrote: Change magic numerical masks with usage of the GENMASK() macro to improve readability. This commit brings no functional changes. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_dsi.c | 46 -- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index a2fdfc8ddb15..23d2c5be8dbb 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -58,18 +58,18 @@ #define DSI_TXRX_CTRL 0x18 #define VC_NUM BIT(1) -#define LANE_NUM (0xf << 2) +#define LANE_NUM GENMASK(5, 2) #define DIS_EOTBIT(6) #define NULL_ENBIT(7) #define TE_FREERUN BIT(8) #define EXT_TE_EN BIT(9) #define EXT_TE_EDGEBIT(10) -#define MAX_RTN_SIZE (0xf << 12) +#define MAX_RTN_SIZE GENMASK(15, 12) #define HSTX_CKLP_EN BIT(16) #define DSI_PSCTRL 0x1c -#define DSI_PS_WC 0x3fff -#define DSI_PS_SEL (3 << 16) +#define DSI_PS_WC GENMASK(14, 0) +#define DSI_PS_SEL GENMASK(19, 16) GENMASK(17, 16) That was intended, and depends on the SoC - I should effectively advertise that in the commit description and I will. As per the datasheets: MT8192 - DSI_PS_SEL[18:16] MT8195 - DSI_PS_SEL[19:16] ...on SoCs that don't have those additional bits, the higher ones are reserved (unused), so it is safe to have this as 19, 16. #define PACKED_PS_16BIT_RGB565 (0 << 16) #define LOOSELY_PS_18BIT_RGB666(1 << 16) #define PACKED_PS_18BIT_RGB666 (2 << 16) @@ -109,26 +109,27 @@ #define LD0_WAKEUP_EN BIT(2) #define DSI_PHY_TIMECON0 0x110 -#define LPX(0xff << 0) -#define HS_PREP(0xff << 8) -#define HS_ZERO(0xff << 16) -#define HS_TRAIL (0xff << 24) +#define LPXGENMASK(7, 0) +#define HS_PREPGENMASK(15, 8) +#define HS_ZEROGENMASK(23, 16) +#define HS_TRAIL GENMASK(31, 24) #define DSI_PHY_TIMECON1 0x114 -#define TA_GO (0xff << 0) -#define TA_SURE(0xff << 8) -#define TA_GET (0xff << 16) -#define DA_HS_EXIT (0xff << 24) +#define TA_GO GENMASK(7, 0) +#define TA_SUREGENMASK(15, 8) +#define TA_GET GENMASK(23, 16) +#define DA_HS_EXIT GENMASK(31, 24) #define DSI_PHY_TIMECON2 0x118 -#define CONT_DET (0xff << 0) -#define CLK_ZERO (0xff << 16) -#define CLK_TRAIL (0xff << 24) +#define CONT_DET GENMASK(7, 0) +#define DA_HS_SYNC GENMASK(15, 8) This is new, so please introduce it in a separate patch if intended. I wouldn't really be comfortable doing so: this would mean that I'd have to first write a constant and then fix that in a later patch... P.S.: Sorry for working again on this one whole month after :-) Regards, Angelo The rest looks good to me. Regards, Fei +#define CLK_ZERO GENMASK(23, 16) +#define CLK_TRAIL GENMASK(31, 24) #define DSI_PHY_TIMECON3 0x11c -#define CLK_HS_PREP(0xff << 0) -#define CLK_HS_POST(0xff << 8) -#define CLK_HS_EXIT(0xff << 16) +#define CLK_HS_PREPGENMASK(7, 0) +#define CLK_HS_POSTGENMASK(15, 8) +#define CLK_HS_EXITGENMASK(23, 16) #define DSI_VM_CMD_CON 0x130 #define VM_CMD_EN BIT(0) @@ -138,13 +139,14 @@ #define FORCE_COMMIT BIT(0) #define BYPASS_SHADOW BIT(1) -#define CONFIG (0xff << 0) +/* CMDQ related bits */ +#define CONFIG GENMASK(7, 0) #define SHORT_PACKET 0 #define LONG_PACKET2 #define BTABIT(2) -#define DATA_ID(0xff << 8) -#define DATA_0 (0xff << 16) -#define DATA_1 (0xff << 24) +#define DATA_IDGENMASK(15, 8) +#define DATA_0 GENMASK(23, 16) +#define DATA_1
Re: [PATCH v2 2/4] drm: Add drm_get_acpi_edid() helper
Hi Mario, kernel test robot noticed the following build errors: [auto build test ERROR on rafael-pm/linux-next] [also build test ERROR on rafael-pm/acpi-bus linus/master v6.8-rc2 next-20240131] [cannot apply to drm-misc/drm-misc-next rafael-pm/devprop] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-video-Handle-fetching-EDID-that-is-longer-than-256-bytes/20240131-032909 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240130192608.11666-3-mario.limonciello%40amd.com patch subject: [PATCH v2 2/4] drm: Add drm_get_acpi_edid() helper config: i386-buildonly-randconfig-003-20240131 (https://download.01.org/0day-ci/archive/20240131/202401311847.xfzpeok4-...@intel.com/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240131/202401311847.xfzpeok4-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202401311847.xfzpeok4-...@intel.com/ All error/warnings (new ones prefixed by >>): >> drivers/platform/x86/wmi.c:68:2: error: unknown type name >> 'wmi_notify_handler'; did you mean 'acpi_notify_handler'? 68 | wmi_notify_handler handler; | ^~ | acpi_notify_handler include/acpi/actypes.h:1061:8: note: 'acpi_notify_handler' declared here 1061 | void (*acpi_notify_handler) (acpi_handle device, u32 value, void *context); |^ >> drivers/platform/x86/wmi.c:163:30: error: incomplete definition of type >> 'struct acpi_device' 163 | handle = wblock->acpi_device->handle; | ~~~^ include/linux/acpi.h:795:8: note: forward declaration of 'struct acpi_device' 795 | struct acpi_device; |^ >> drivers/platform/x86/wmi.c:166:11: error: call to undeclared function >> 'acpi_execute_simple_method'; ISO C99 and later do not support implicit >> function declarations [-Wimplicit-function-declaration] 166 | status = acpi_execute_simple_method(handle, method, enable); | ^ drivers/platform/x86/wmi.c:166:11: note: did you mean 'acpi_execute_reg_methods'? include/acpi/acpixf.h:662:8: note: 'acpi_execute_reg_methods' declared here 662 | acpi_execute_reg_methods(acpi_handle device, | ^ include/acpi/platform/aclinux.h:93:21: note: expanded from macro 'ACPI_EXTERNAL_RETURN_STATUS' 93 | static ACPI_INLINE prototype {return(AE_NOT_CONFIGURED);} |^ drivers/platform/x86/wmi.c:210:49: error: incomplete definition of type 'struct acpi_device' 210 | return acpi_evaluate_object(wblock->acpi_device->handle, "_WED", , out); | ~~~^ include/linux/acpi.h:795:8: note: forward declaration of 'struct acpi_device' 795 | struct acpi_device; |^ >> drivers/platform/x86/wmi.c:282:5: warning: no previous prototype for >> function 'wmi_instance_count' [-Wmissing-prototypes] 282 | int wmi_instance_count(const char *guid_string) | ^ drivers/platform/x86/wmi.c:282:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 282 | int wmi_instance_count(const char *guid_string) | ^ | static >> drivers/platform/x86/wmi.c:326:13: warning: no previous prototype for >> function 'wmi_evaluate_method' [-Wmissing-prototypes] 326 | acpi_status wmi_evaluate_method(const char *guid_string, u8 instance, u32 method_id, | ^ drivers/platform/x86/wmi.c:326:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 326 | acpi_status wmi_evaluate_method(const char *guid_string, u8 instance, u32 method_id, | ^ | static drivers/platform/x86/wmi.c:368:30: error: incomplete definition of type 'struct acpi_device' 368 | handle = wblock->acpi_device->handle; | ~~~^ include/linux/acpi.h:795:8: note: forward declaration of 'struct acpi_device' 795 | struct acpi_device; |^ drivers/platform/x86/wmi.c:412:30: error: incompl
[PATCHv2 1/2] drm/display/dp: Check for MSTM_CAP before MSTM_CTRL write
With DP2.1, multistream packetization and the underneth MST protocol will be required for SST. So check for MSTM_CAP to see if MST is really required and skip the MSTM_CTRL write so that we ensure that only the underneth protocol and the multistream packetization will be enabled and sink will not be confused by a corresponding dpcd write. Signed-off-by: Arun R Murthy --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 38 +++ 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 8ca01a6bf645..c5b3e51ea0c9 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -3666,12 +3666,14 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms mgr->mst_primary = mstb; drm_dp_mst_topology_get_mstb(mgr->mst_primary); - ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, -DP_MST_EN | -DP_UP_REQ_EN | -DP_UPSTREAM_IS_SRC); - if (ret < 0) - goto out_unlock; + if (drm_dp_read_mst_cap(mgr->aux, mgr->dpcd)) { + ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, +DP_MST_EN | +DP_UP_REQ_EN | +DP_UPSTREAM_IS_SRC); + if (ret < 0) + goto out_unlock; + } /* Write reset payload */ drm_dp_dpcd_write_payload(mgr, 0, 0, 0x3f); @@ -3684,7 +3686,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms mstb = mgr->mst_primary; mgr->mst_primary = NULL; /* this can fail if the device is gone */ - drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0); + if (drm_dp_read_mst_cap(mgr->aux, mgr->dpcd)) + drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0); ret = 0; mgr->payload_id_table_cleared = false; @@ -3724,8 +3727,9 @@ drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb) void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) { mutex_lock(>lock); - drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, - DP_MST_EN | DP_UPSTREAM_IS_SRC); + if (drm_dp_read_mst_cap(mgr->aux, mgr->dpcd)) + drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, + DP_MST_EN | DP_UPSTREAM_IS_SRC); mutex_unlock(>lock); flush_work(>up_req_work); flush_work(>work); @@ -3773,13 +3777,15 @@ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr, goto out_fail; } - ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, -DP_MST_EN | -DP_UP_REQ_EN | -DP_UPSTREAM_IS_SRC); - if (ret < 0) { - drm_dbg_kms(mgr->dev, "mst write failed - undocked during suspend?\n"); - goto out_fail; + if (drm_dp_read_mst_cap(mgr->aux, mgr->dpcd)) { + ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, +DP_MST_EN | +DP_UP_REQ_EN | +DP_UPSTREAM_IS_SRC); + if (ret < 0) { + drm_dbg_kms(mgr->dev, "mst write failed - undocked during suspend?\n"); + goto out_fail; + } } /* Some hubs forget their guids after they resume */ -- 2.25.1
[PATCHv2 2/2] drm/i915/display/dp: 128/132b DP-capable with SST
With a value of '0' read from MSTM_CAP register MST to be enabled. DP2.1 SCR updates the spec for 128/132b DP capable supporting only one stream and not supporting single stream sideband MSG. The underlying protocol will be MST to enable use of MTP. Signed-off-by: Arun R Murthy --- drivers/gpu/drm/i915/display/intel_dp.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 9ff0cbd9c0df..05722f10cdd7 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4037,9 +4037,15 @@ intel_dp_configure_mst(struct intel_dp *intel_dp) if (!intel_dp_mst_source_support(intel_dp)) return; - - intel_dp->is_mst = sink_can_mst && - i915->display.params.enable_dp_mst; + /* +* Even if dpcd reg MSTM_CAP is 0, if the sink supports UHBR rates then +* DP2.1 can be enabled with underlying protocol using MST for MTP +* TODO: Need to accommodate MSTM_CAP bit[0]=0, bit[1]=1 condition, i.e +* one stream with single stream sideband msg. +*/ + intel_dp->is_mst = (sink_can_mst || (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & +DP_CAP_ANSI_128B132B)) && + i915->display.params.enable_dp_mst; drm_dp_mst_topology_mgr_set_mst(_dp->mst_mgr, intel_dp->is_mst); -- 2.25.1
Re: [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
On Wed, 31 Jan 2024 at 11:11, Daniel Vetter wrote: > > On Wed, Jan 31, 2024 at 05:17:08AM +, Jason-JH Lin (林睿祥) wrote: > > On Thu, 2024-01-25 at 19:17 +0100, Daniel Vetter wrote: > > > > > > External email : Please do not click links or open attachments until > > > you have verified the sender or the content. > > > On Tue, Jan 23, 2024 at 06:09:05AM +, Jason-JH Lin (林睿祥) wrote: > > > > Hi Maxime, Daniel, > > > > > > > > We encountered similar issue with mediatek SoCs. > > > > > > > > We have found that in drm_atomic_helper_commit_rpm(), when > > > disabling > > > > the cursor plane, the old_state->legacy_cursor_update in > > > > drm_atomic_wait_for_vblank() is set to true. > > > > As the result, we are not actually waiting for a vlbank to wait for > > > our > > > > hardware to close the cursor plane. Subsequently, the execution > > > > proceeds to drm_atomic_helper_cleanup_planes() to free the cursor > > > > buffer. This can lead to use-after-free issues with our hardware. > > > > > > > > Could you please apply this patch to fix our problem? > > > > Or are there any considerations for not applying this patch? > > > > > > Mostly it needs someone to collect a pile of acks/tested-by and then > > > land > > > it. > > > > > > > Got it. I would add tested-by tag for mediatek SoC. > > > > > I'd be _very_ happy if someone else can take care of that ... > > > > > > There's also the potential issue that it might slow down some of the > > > legacy X11 use-cases that really needed a non-blocking cursor, but I > > > think > > > all the drivers where this matters have switched over to the async > > > plane > > > update stuff meanwhile. So hopefully that's good. > > > > > > > I think all the drivers should have switched to async plane update. > > > > Can we add the checking condition to see if atomic_async_update/check > > function are implemented? > > Pretty sure not all have done that, so really it boils down to whether we > break a real user's use-case. Which pretty much can only be checked by > merging the patch (hence the requirement to get as many acks as possible > from display drivers) and then being willing to handle any fallout that's > reported as regressions for a specific driver. > > It's a pile of work, at least when it goes south, that's why I'm looking > for volunteers. I can check this on all sensible msm generations, including mdp4, but it will be next week, after the FOSDEM. BTW, for technical reasons one of the msm platforms still has the legacy cursor implementation might it be related? > > Note that handling the fallout doesn't mean you have to fix that specific > driver, the only realistic option might be to reinstate the legacy cursor > behaviour, but as an explicit opt-in that only that specific driver > enables. > > So maybe for next round of that patch it might be good to have a 2nd patch > which implements this fallback plan in the shared atomic modeset code? > > Cheers, Sima -- With best wishes Dmitry
Re: [PATCH v2 1/1] drm/virtio: Implement device_attach
On 2024/1/30 22:23, Christian König wrote: > Am 30.01.24 um 12:16 schrieb Daniel Vetter: >> On Tue, Jan 30, 2024 at 12:10:31PM +0100, Daniel Vetter wrote: >>> On Mon, Jan 29, 2024 at 06:31:19PM +0800, Julia Zhang wrote: As vram objects don't have backing pages and thus can't implement drm_gem_object_funcs.get_sg_table callback. This removes drm dma-buf callbacks in virtgpu_gem_map_dma_buf()/virtgpu_gem_unmap_dma_buf() and implement virtgpu specific map/unmap/attach callbacks to support both of shmem objects and vram objects. Signed-off-by: Julia Zhang --- drivers/gpu/drm/virtio/virtgpu_prime.c | 40 +++--- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c index 44425f20d91a..b490a5343b06 100644 --- a/drivers/gpu/drm/virtio/virtgpu_prime.c +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c @@ -49,11 +49,26 @@ virtgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, { struct drm_gem_object *obj = attach->dmabuf->priv; struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); + struct sg_table *sgt; + int ret; if (virtio_gpu_is_vram(bo)) return virtio_gpu_vram_map_dma_buf(bo, attach->dev, dir); - return drm_gem_map_dma_buf(attach, dir); + sgt = drm_prime_pages_to_sg(obj->dev, + to_drm_gem_shmem_obj(obj)->pages, + obj->size >> PAGE_SHIFT); + if (IS_ERR(sgt)) + return sgt; + + ret = dma_map_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC); + if (ret) { + sg_free_table(sgt); + kfree(sgt); + return ERR_PTR(ret); + } + + return sgt; } static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, @@ -63,12 +78,29 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, struct drm_gem_object *obj = attach->dmabuf->priv; struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); + if (!sgt) + return; + if (virtio_gpu_is_vram(bo)) { virtio_gpu_vram_unmap_dma_buf(attach->dev, sgt, dir); - return; + } else { + dma_unmap_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC); + sg_free_table(sgt); + kfree(sgt); } +} + +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf, + struct dma_buf_attachment *attach) +{ + struct drm_gem_object *obj = attach->dmabuf->priv; + struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); + int ret = 0; + + if (!virtio_gpu_is_vram(bo) && obj->funcs->pin) + ret = obj->funcs->pin(obj); - drm_gem_unmap_dma_buf(attach, sgt, dir); + return ret; >>> This doesn't look like what I've expected. There should be no need to >>> change the map/unmap functions, especially not for the usual gem bo case. >>> We should definitely keep using the exact same code for that. Instead all >>> I expected is roughly >>> >>> virtgpu_gem_device_attach() >>> { >>> if (virtio_gpu_is_vram(bo)) { >>> if (can_access_virtio_vram_directly(attach->dev) >>> return 0; >>> else >>> return -EBUSY; >>> } else { >>> return drm_gem_map_attach(); >>> } >>> } >>> >>> Note that I think can_access_virtio_vram_directly() needs to be >>> implemented first. I'm not even sure it's possible, might be that all the >>> importers need to set the attachment->peer2peer flag. Which is why this >>> thing exists really. But that's a pile more work to do. > Hi Sima, Christian, > Yeah, that is really just speculative. All importers need to set the > peer2peer flag just in case. I see, I will modify this. > > What happens under the hood is that IOMMU redirects the "VRAM" memory access > to whatever address the DMA-buf on the host is pointing to (system, VRAM, > doorbell, IOMMU, whatever). > > I'm also not 100% sure if all the cache snooping is done correctly in all > cases, but for now it seems to work. >>> Frankly the more I look at the original patch that added vram export >>> support the more this just looks like a "pls revert, this is just too >>> broken". >> The commit I mean is this one: ea5ea3d8a117 ("drm/virtio: support mapping >> exported vram"). The commit message definitely needs to cite that one, and >> also needs a cc: stable because not rejecting invalid imports is a pretty >> big deal. > > Yeah, I've pointed out that commit in an internal discussion as well. I was > just not aware that it's that severely broken. > Yeah we have mentioned this patch before, but I don't totally understand why this
[Bug 218435] UBSAN: array-index-out-of-bounds in radeon_atombios.c:2620:43
https://bugzilla.kernel.org/show_bug.cgi?id=218435 Artem S. Tashkinov (a...@gmx.com) changed: What|Removed |Added Status|NEW |RESOLVED Kernel Version||6.5.0-15-generic ||#15~22.04.1-Ubuntu Resolution|--- |WILL_NOT_FIX --- Comment #2 from Artem S. Tashkinov (a...@gmx.com) --- This kernel release is no longer supported. Take it to your vendor: https://bugs.launchpad.net/ubuntu -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH] drm/msm/gem: Fix double resv lock aquire
Am 30.01.24 um 23:35 schrieb Rob Clark: From: Rob Clark Since commit 56e5abba8c3e ("dma-buf: Add unlocked variant of vmapping functions"), the resv lock is already held in the prime vmap path, so don't try to grab it again. Fixes: 56e5abba8c3e ("dma-buf: Add unlocked variant of vmapping functions") Signed-off-by: Rob Clark Acked-by: Christian König --- drivers/gpu/drm/msm/msm_gem_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c index 5f68e31a3e4e..8a27b57a5bea 100644 --- a/drivers/gpu/drm/msm/msm_gem_prime.c +++ b/drivers/gpu/drm/msm/msm_gem_prime.c @@ -26,7 +26,7 @@ int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map) { void *vaddr; - vaddr = msm_gem_get_vaddr(obj); + vaddr = msm_gem_get_vaddr_locked(obj); if (IS_ERR(vaddr)) return PTR_ERR(vaddr); iosys_map_set_vaddr(map, vaddr);