Re: [PATCH v1] drm/rockchip: rk3066_hdmi: change to bridge driver mode
Hi, On Tue, Jul 09, 2024 at 08:01:26PM GMT, Johan Jonker wrote: > Change rk3066_hdmi.c to bridge driver mode. > > Signed-off-by: Johan Jonker Generally speaking, I think you should use the new HDMI bridge infrastructure that recently landed in drm-misc-next. Maxime signature.asc Description: PGP signature
Re: 6.10/bisected/regression - commits bc87d666c05 and 6d4279cb99ac cause appearing green flashing bar on top of screen on Radeon 6900XT and 120Hz
On Tue, Jul 9, 2024 at 7:48 PM Rodrigo Siqueira Jordao wrote: > Hi, > > I also tried it with 6900XT. I got the same results on my side. This is weird. > Anyway, I could not reproduce the issue with the below components. I may > be missing something that will trigger this bug; in this sense, could > you describe the following: > - The display resolution and refresh rate. 3840x2160 and 120Hz At 60Hz issue not reproduced. > - Are you able to reproduce this issue with DP and HDMI? My TV, an OLED LG C3, has only an HDMI 2.1 port. > - Could you provide the firmware information: sudo cat > /sys/kernel/debug/dri/0/amdgpu_firmware_info > sudo cat /sys/kernel/debug/dri/0/amdgpu_firmware_info [sudo] password for mikhail: VCE feature version: 0, firmware version: 0x UVD feature version: 0, firmware version: 0x MC feature version: 0, firmware version: 0x ME feature version: 38, firmware version: 0x000e PFP feature version: 38, firmware version: 0x000e CE feature version: 38, firmware version: 0x0003 RLC feature version: 1, firmware version: 0x001f RLC SRLC feature version: 1, firmware version: 0x0001 RLC SRLG feature version: 1, firmware version: 0x0001 RLC SRLS feature version: 1, firmware version: 0x0001 RLCP feature version: 0, firmware version: 0x RLCV feature version: 0, firmware version: 0x MEC feature version: 38, firmware version: 0x0015 MEC2 feature version: 38, firmware version: 0x0015 IMU feature version: 0, firmware version: 0x SOS feature version: 0, firmware version: 0x ASD feature version: 553648344, firmware version: 0x21d8 TA XGMI feature version: 0x, firmware version: 0x TA RAS feature version: 0x, firmware version: 0x TA HDCP feature version: 0x, firmware version: 0x173f TA DTM feature version: 0x, firmware version: 0x1216 TA RAP feature version: 0x, firmware version: 0x TA SECUREDISPLAY feature version: 0x, firmware version: 0x SMC feature version: 0, program: 0, firmware version: 0x00544fdf (84.79.223) SDMA0 feature version: 52, firmware version: 0x0009 VCN feature version: 0, firmware version: 0x0311f002 DMCU feature version: 0, firmware version: 0x DMCUB feature version: 0, firmware version: 0x05000f00 TOC feature version: 0, firmware version: 0x0007 MES_KIQ feature version: 0, firmware version: 0x MES feature version: 0, firmware version: 0x VPE feature version: 0, firmware version: 0x VBIOS version: 102-RAPHAEL-008 > Also, could you conduct the below tests and report the results: > > - Test 1: Just revert the fallback patch (drm/amd/display: Add fallback > configuration for set DRR in DCN10) and see if it solves the issue. It's not enough. I checked revert commit bc87d666c05 on top of 34afb82a3c67. > - Test 2: Try the latest amd-staging-drm-next > (https://gitlab.freedesktop.org/agd5f/linux) and see if the issue is gone. I checked commit 7cef45b1347a in the amd-staging-drm-next branch. Same here. > - Test 3: In the kernel that you see the issue, could you install the > latest firmware and see if it fix the issue? Check: > https://gitlab.freedesktop.org/drm/firmware P.S.: Don't forget to update > the initramfs or something similar in your system. Is this any sense? Fedora Rawhide always ships with the latest kernel and firmware. -- Best Regards, Mike Gavrilov.
Re: 6.10/bisected/regression - commits bc87d666c05 and 6d4279cb99ac cause appearing green flashing bar on top of screen on Radeon 6900XT and 120Hz
On Wed, Jul 10, 2024 at 12:01 PM Mikhail Gavrilov wrote: > > On Tue, Jul 9, 2024 at 7:48 PM Rodrigo Siqueira Jordao > wrote: > > Hi, > > > > I also tried it with 6900XT. I got the same results on my side. > > This is weird. > > > Anyway, I could not reproduce the issue with the below components. I may > > be missing something that will trigger this bug; in this sense, could > > you describe the following: > > - The display resolution and refresh rate. > > 3840x2160 and 120Hz > At 60Hz issue not reproduced. > > > - Are you able to reproduce this issue with DP and HDMI? > > My TV, an OLED LG C3, has only an HDMI 2.1 port. > > > - Could you provide the firmware information: sudo cat > > /sys/kernel/debug/dri/0/amdgpu_firmware_info > > > sudo cat /sys/kernel/debug/dri/0/amdgpu_firmware_info > [sudo] password for mikhail: > VCE feature version: 0, firmware version: 0x > UVD feature version: 0, firmware version: 0x > MC feature version: 0, firmware version: 0x > ME feature version: 38, firmware version: 0x000e > PFP feature version: 38, firmware version: 0x000e > CE feature version: 38, firmware version: 0x0003 > RLC feature version: 1, firmware version: 0x001f > RLC SRLC feature version: 1, firmware version: 0x0001 > RLC SRLG feature version: 1, firmware version: 0x0001 > RLC SRLS feature version: 1, firmware version: 0x0001 > RLCP feature version: 0, firmware version: 0x > RLCV feature version: 0, firmware version: 0x > MEC feature version: 38, firmware version: 0x0015 > MEC2 feature version: 38, firmware version: 0x0015 > IMU feature version: 0, firmware version: 0x > SOS feature version: 0, firmware version: 0x > ASD feature version: 553648344, firmware version: 0x21d8 > TA XGMI feature version: 0x, firmware version: 0x > TA RAS feature version: 0x, firmware version: 0x > TA HDCP feature version: 0x, firmware version: 0x173f > TA DTM feature version: 0x, firmware version: 0x1216 > TA RAP feature version: 0x, firmware version: 0x > TA SECUREDISPLAY feature version: 0x, firmware version: 0x > SMC feature version: 0, program: 0, firmware version: 0x00544fdf (84.79.223) > SDMA0 feature version: 52, firmware version: 0x0009 > VCN feature version: 0, firmware version: 0x0311f002 > DMCU feature version: 0, firmware version: 0x > DMCUB feature version: 0, firmware version: 0x05000f00 > TOC feature version: 0, firmware version: 0x0007 > MES_KIQ feature version: 0, firmware version: 0x > MES feature version: 0, firmware version: 0x > VPE feature version: 0, firmware version: 0x > VBIOS version: 102-RAPHAEL-008 > I forgot to add output for discrete GPU: > sudo cat /sys/kernel/debug/dri/1/amdgpu_firmware_info [sudo] password for mikhail: VCE feature version: 0, firmware version: 0x UVD feature version: 0, firmware version: 0x MC feature version: 0, firmware version: 0x ME feature version: 44, firmware version: 0x0040 PFP feature version: 44, firmware version: 0x0062 CE feature version: 44, firmware version: 0x0025 RLC feature version: 1, firmware version: 0x0060 RLC SRLC feature version: 0, firmware version: 0x RLC SRLG feature version: 0, firmware version: 0x RLC SRLS feature version: 0, firmware version: 0x RLCP feature version: 0, firmware version: 0x RLCV feature version: 0, firmware version: 0x MEC feature version: 44, firmware version: 0x0076 MEC2 feature version: 44, firmware version: 0x0076 IMU feature version: 0, firmware version: 0x SOS feature version: 0, firmware version: 0x00210e64 ASD feature version: 553648345, firmware version: 0x21d9 TA XGMI feature version: 0x, firmware version: 0x200f TA RAS feature version: 0x, firmware version: 0x1b00013e TA HDCP feature version: 0x, firmware version: 0x173f TA DTM feature version: 0x, firmware version: 0x1216 TA RAP feature version: 0x, firmware version: 0x0716 TA SECUREDISPLAY feature version: 0x, firmware version: 0x SMC feature version: 0, program: 0, firmware version: 0x003a5a00 (58.90.0) SDMA0 feature version: 52, firmware version: 0x0053 SDMA1 feature version: 52, firmware version: 0x0053 SDMA2 feature version: 52, firmware version: 0x0053 SDMA3 feature version: 52, firmware version: 0x0053 VCN feature version: 0, firmware version: 0x0311f002 DMCU feature version: 0, firmware version: 0x DMCUB feature version: 0, firmware version: 0x02020020 TOC feature version: 0, firmware version: 0x MES_KIQ feature version: 0, firmware version: 0x MES feature version: 0, firmware version: 0x VPE feature version: 0, firmware version: 0x VBIOS version: 113-D4120100-100 -- Best Regards, Mike Gavrilov.
Re: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds for firmware loading
On Tue, Jul 09, 2024 at 06:36:08PM GMT, Dragan Simic wrote: > > > > > As I already wrote earlier, and as the above-linked discussions > > > > > conclude, solving these issues doesn't belong to any specific driver. > > > > > It should be resolved within the kernel's firmware loading mechanism > > > > > instead, and no driver should be specific in that regard. > > > > > > > > IT would be good if it can be resolved within the kernel's firmware > > > > loading mechanism. > > > > > > ... we'll need this as a systemic solution. > > > > The general policy has been to put drivers that need a firmware as a > > module, and just never build them statically. > > I totally agree, but if Buildroot builds them statically and provides > no initial ramdisk, we need a better solution than having various drivers > attempt to implement their own workarounds. Buildroot typically allows custom kernel configurations, so it's not really "enforcing" anything like another distro does. It is definitely targetted towards very stripped down systems, so I guess building the drivers statically is a natural choice, but it works fine with modules too. Maxime signature.asc Description: PGP signature
Re: [PATCH v2] drm/qxl: Pin buffer objects for internal mappings
Merged into drm-misc-next-fixes Am 08.07.24 um 16:21 schrieb Thomas Zimmermann: Add qxl_bo_pin_and_vmap() that pins and vmaps a buffer object in one step. Update callers of the regular qxl_bo_vmap(). Fixes a bug where qxl accesses an unpinned buffer object while it is being moved; such as with the monitor-description BO. An typical error is shown below. [4.303586] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0 [4.586883] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0 [4.904036] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65335296x16777216+0+0 [5.374347] [drm:qxl_release_from_id_locked] *ERROR* failed to find id in release_idr Commit b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap") removed the implicit pin operation from qxl's vmap code. This is the correct behavior for GEM and PRIME interfaces, but the pin is still needed for qxl internal operation. Also add a corresponding function qxl_bo_vunmap_and_unpin() and remove the old qxl_bo_vmap() helpers. Future directions: BOs should not be pinned or vmapped unnecessarily. The pin-and-vmap operation should be removed from the driver and a temporary mapping should be established with a vmap_local-like helper. See the client helper drm_client_buffer_vmap_local() for semantics. v2: - unreserve BO on errors in qxl_bo_pin_and_vmap() (Dmitry) Signed-off-by: Thomas Zimmermann Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap") Reported-by: David Kaplan Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02...@suse.de/ Tested-by: David Kaplan Reviewed-by: Daniel Vetter Cc: Thomas Zimmermann Cc: Dmitry Osipenko Cc: Christian König Cc: Zack Rusin Cc: Dave Airlie Cc: Gerd Hoffmann Cc: virtualizat...@lists.linux.dev Cc: spice-de...@lists.freedesktop.org --- drivers/gpu/drm/qxl/qxl_display.c | 14 +++--- drivers/gpu/drm/qxl/qxl_object.c | 13 +++-- drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 86a5dea710c0..bc24af08dfcd 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,11 +584,11 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev, if (ret) goto err; - ret = qxl_bo_vmap(cursor_bo, &cursor_map); + ret = qxl_bo_pin_and_vmap(cursor_bo, &cursor_map); if (ret) goto err_unref; - ret = qxl_bo_vmap(user_bo, &user_map); + ret = qxl_bo_pin_and_vmap(user_bo, &user_map); if (ret) goto err_unmap; @@ -614,12 +614,12 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev, user_map.vaddr, size); } - qxl_bo_vunmap(user_bo); - qxl_bo_vunmap(cursor_bo); + qxl_bo_vunmap_and_unpin(user_bo); + qxl_bo_vunmap_and_unpin(cursor_bo); return cursor_bo; err_unmap: - qxl_bo_vunmap(cursor_bo); + qxl_bo_vunmap_and_unpin(cursor_bo); err_unref: qxl_bo_unpin(cursor_bo); qxl_bo_unref(&cursor_bo); @@ -1205,7 +1205,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj); - ret = qxl_bo_vmap(qdev->monitors_config_bo, &map); + ret = qxl_bo_pin_and_vmap(qdev->monitors_config_bo, &map); if (ret) return ret; @@ -1236,7 +1236,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - ret = qxl_bo_vunmap(qdev->monitors_config_bo); + ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 5893e27a7ae5..66635c55cf85 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -182,7 +182,7 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map) return 0; } -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map) +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map) { int r; @@ -190,7 +190,15 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map) if (r) return r; + r = qxl_bo_pin_locked(bo); + if (r) { + qxl_bo_unreserve(bo); + return r; + } + r = qxl_bo_vmap_locked(bo, map); + if (r) + qxl_bo_unpin_locked(bo); qxl_bo_unreserve(bo); return r; } @@ -241,7 +249,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo) ttm_bo_vunmap(&bo->tbo, &bo->map); } -int qxl_bo_vunmap(struct qxl_bo *bo) +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo) { int r; @@ -250,6 +258,7 @@ int qxl
Re: [PATCH 2/2] drm/msm/dpu: don't play tricks with debug macros
On Tue, 9 Jul 2024 at 22:39, Abhinav Kumar wrote: > > > > On 7/9/2024 6:48 AM, Dmitry Baryshkov wrote: > > DPU debugging macros need to be converted to a proper drm_debug_* > > macros, however this is a going an intrusive patch, not suitable for a > > fix. Wire DPU_DEBUG and DPU_DEBUG_DRIVER to always use DRM_DEBUG_DRIVER > > to make sure that DPU debugging messages always end up in the drm debug > > messages and are controlled via the usual drm.debug mask. > > > > These macros have been deprecated, is this waht you meant by the > conversion to proper drm_debug_*? Yes. Drop the driver-specific wrappers where they don't make sense. Use sensible format strings in the cases where it actually does (like VIDENC or _PLANE) > > /* NOTE: this is deprecated in favor of drm_dbg(NULL, ...). */ > #define DRM_DEBUG_DRIVER(fmt, ...) \ > __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__) > > I think all that this macro was doing was to have appropriate DRM_UT_* > macros enabled before calling the corresponding DRM_DEBUG_* macros. But > I think what was incorrect here is for DPU_DEBUG, we could have used > DRM_UT_CORE instead of DRM_UT_KMS. It pretty much tries to overplay the existing drm debugging mechanism by either sending the messages to the DRM channel or just using pr_debug. With DYNAMIC_DEBUG being disabled pr_debug is just an empty macro, so all the messages can end up in /dev/null. We should not be trying to be too smart, using standard DRM_DEBUG_DRIVER should be enough. This way all driver-related messages are controlled by drm.debug including or excluding the 0x02 bit. > > And DRM_DEBUG_DRIVER should have been used instead of DRM_ERROR. > > Was this causing the issue of the prints not getting enabled? I pretty much think so. > > > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") > > Signed-off-by: Dmitry Baryshkov > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 14 ++ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > index e2adc937ea63..935ff6fd172c 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > @@ -31,24 +31,14 @@ > >* @fmt: Pointer to format string > >*/ > > #define DPU_DEBUG(fmt, ...) > > \ > > - do { \ > > - if (drm_debug_enabled(DRM_UT_KMS)) \ > > - DRM_DEBUG(fmt, ##__VA_ARGS__); \ > > - else \ > > - pr_debug(fmt, ##__VA_ARGS__); \ > > - } while (0) > > + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) > > > > /** > >* DPU_DEBUG_DRIVER - macro for hardware driver logging > >* @fmt: Pointer to format string > >*/ > > #define DPU_DEBUG_DRIVER(fmt, ...) > > \ > > - do { \ > > - if (drm_debug_enabled(DRM_UT_DRIVER)) \ > > - DRM_ERROR(fmt, ##__VA_ARGS__); \ > > - else \ > > - pr_debug(fmt, ##__VA_ARGS__); \ > > - } while (0) > > + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) > > > > #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__) > > #define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" > > fmt, ##__VA_ARGS__) > > -- With best wishes Dmitry
[PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning
As the comment explains, the if check ensures that the divisor oa_period is a u32. Explicitly cast oa_period to u32 to remove the following Coccinelle/coccicheck warning reported by do_div.cocci: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead Signed-off-by: Thorsten Blum --- drivers/gpu/drm/i915/i915_perf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 0b1cd4c7a525..24722e758aaf 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf *perf, */ if (oa_period <= NSEC_PER_SEC) { u64 tmp = NSEC_PER_SEC; - do_div(tmp, oa_period); + do_div(tmp, (u32)oa_period); oa_freq_hz = tmp; } else oa_freq_hz = 0; -- 2.45.2
Re: [v4] drm/gma500: fix null pointer dereference in cdv_intel_lvds_get_modes
… > > Signed-off-by: Ma Ke > > Thanks for the patch! > Pushed to drm-misc-fixes Do you care for the applicability of the available information according to this tag? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n398 Regards, Markus
[PATCH v2 3/5] drm/mediatek: Support "Pre-multiplied" blending in OVL
From: Hsiao Chien Sung Support "Pre-multiplied" alpha blending mode on in OVL. Before this patch, only the "coverage" mode is supported. Signed-off-by: Hsiao Chien Sung Reviewed-by: CK Hu --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 32 +--- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index add671c38613..89b439dcf3a6 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -56,8 +56,12 @@ #define GMC_THRESHOLD_HIGH ((1 << GMC_THRESHOLD_BITS) / 4) #define GMC_THRESHOLD_LOW ((1 << GMC_THRESHOLD_BITS) / 8) +#define OVL_CON_CLRFMT_MAN BIT(23) #define OVL_CON_BYTE_SWAP BIT(24) -#define OVL_CON_MTX_YUV_TO_RGB (6 << 16) + +/* OVL_CON_RGB_SWAP works only if OVL_CON_CLRFMT_MAN is enabled */ +#define OVL_CON_RGB_SWAP BIT(25) + #define OVL_CON_CLRFMT_RGB (1 << 12) #define OVL_CON_CLRFMT_ARGB(2 << 12) #define OVL_CON_CLRFMT_RGBA(3 << 12) @@ -65,6 +69,11 @@ #define OVL_CON_CLRFMT_BGRA(OVL_CON_CLRFMT_ARGB | OVL_CON_BYTE_SWAP) #define OVL_CON_CLRFMT_UYVY(4 << 12) #define OVL_CON_CLRFMT_YUYV(5 << 12) +#define OVL_CON_MTX_YUV_TO_RGB (6 << 16) +#define OVL_CON_CLRFMT_PARGB ((3 << 12) | OVL_CON_CLRFMT_MAN) +#define OVL_CON_CLRFMT_PABGR (OVL_CON_CLRFMT_PARGB | OVL_CON_RGB_SWAP) +#define OVL_CON_CLRFMT_PBGRA (OVL_CON_CLRFMT_PARGB | OVL_CON_BYTE_SWAP) +#define OVL_CON_CLRFMT_PRGBA (OVL_CON_CLRFMT_PABGR | OVL_CON_BYTE_SWAP) #define OVL_CON_CLRFMT_RGB565(ovl) ((ovl)->data->fmt_rgb565_is_0 ? \ 0 : OVL_CON_CLRFMT_RGB) #define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ? \ @@ -377,7 +386,8 @@ void mtk_ovl_layer_off(struct device *dev, unsigned int idx, DISP_REG_OVL_RDMA_CTRL(idx)); } -static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl, unsigned int fmt) +static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl, unsigned int fmt, + unsigned int blend_mode) { /* The return value in switch "MEM_MODE_INPUT_FORMAT_XXX" * is defined in mediatek HW data sheet. @@ -398,22 +408,30 @@ static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl, unsigned int fmt) case DRM_FORMAT_RGBA: case DRM_FORMAT_RGBX1010102: case DRM_FORMAT_RGBA1010102: - return OVL_CON_CLRFMT_RGBA; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_RGBA : + OVL_CON_CLRFMT_PRGBA; case DRM_FORMAT_BGRX: case DRM_FORMAT_BGRA: case DRM_FORMAT_BGRX1010102: case DRM_FORMAT_BGRA1010102: - return OVL_CON_CLRFMT_BGRA; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_BGRA : + OVL_CON_CLRFMT_PBGRA; case DRM_FORMAT_XRGB: case DRM_FORMAT_ARGB: case DRM_FORMAT_XRGB2101010: case DRM_FORMAT_ARGB2101010: - return OVL_CON_CLRFMT_ARGB; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_ARGB : + OVL_CON_CLRFMT_PARGB; case DRM_FORMAT_XBGR: case DRM_FORMAT_ABGR: case DRM_FORMAT_XBGR2101010: case DRM_FORMAT_ABGR2101010: - return OVL_CON_CLRFMT_ABGR; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_ABGR : + OVL_CON_CLRFMT_PABGR; case DRM_FORMAT_UYVY: return OVL_CON_CLRFMT_UYVY | OVL_CON_MTX_YUV_TO_RGB; case DRM_FORMAT_YUYV: @@ -453,7 +471,7 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, return; } - con = ovl_fmt_convert(ovl, fmt); + con = ovl_fmt_convert(ovl, fmt, blend_mode); if (state->base.fb) { con |= OVL_CON_AEN; con |= state->base.alpha & OVL_CON_ALPHA; -- 2.43.0
[PATCH v2 4/5] drm/mediatek: Support "Pre-multiplied" blending in Mixer
From: Hsiao Chien Sung Support "Pre-multiplied" alpha blending mode in Mixer. Before this patch, only the coverage mode is supported. To replace the default setting that is set in mtk_ethdr_config(), we change mtk_ddp_write_mask() to mtk_ddp_write(), and this change will also reset the NON_PREMULTI_SOURCE bit that was assigned in mtk_ethdr_config(). Therefore, we must still set NON_PREMULTI_SOURCE bit if the blend mode is not DRM_MODE_BLEND_PREMULTI. Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_ethdr.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c b/drivers/gpu/drm/mediatek/mtk_ethdr.c index 80ccdad3741b..d1d9cf8b10e1 100644 --- a/drivers/gpu/drm/mediatek/mtk_ethdr.c +++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c @@ -36,6 +36,7 @@ #define MIX_SRC_L0_EN BIT(0) #define MIX_L_SRC_CON(n) (0x28 + 0x18 * (n)) #define NON_PREMULTI_SOURCE(2 << 12) +#define PREMULTI_SOURCE(3 << 12) #define MIX_L_SRC_SIZE(n) (0x30 + 0x18 * (n)) #define MIX_L_SRC_OFFSET(n)(0x34 + 0x18 * (n)) #define MIX_FUNC_DCM0 0x120 @@ -176,6 +177,11 @@ void mtk_ethdr_layer_config(struct device *dev, unsigned int idx, alpha_con |= state->base.alpha & MIXER_ALPHA; } + if (state->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) + alpha_con |= PREMULTI_SOURCE; + else + alpha_con |= NON_PREMULTI_SOURCE; + if ((state->base.fb && !state->base.fb->format->has_alpha) || state->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) { /* @@ -193,8 +199,7 @@ void mtk_ethdr_layer_config(struct device *dev, unsigned int idx, mtk_ddp_write(cmdq_pkt, pending->height << 16 | align_width, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_SIZE(idx)); mtk_ddp_write(cmdq_pkt, offset, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_OFFSET(idx)); - mtk_ddp_write_mask(cmdq_pkt, alpha_con, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_CON(idx), - 0x1ff); + mtk_ddp_write(cmdq_pkt, alpha_con, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_CON(idx)); mtk_ddp_write_mask(cmdq_pkt, BIT(idx), &mixer->cmdq_base, mixer->regs, MIX_SRC_CON, BIT(idx)); } -- 2.43.0
[PATCH v2 2/5] drm/mediatek: Support "None" blending in Mixer
From: Hsiao Chien Sung Support "None" alpha blending mode on MediaTek's chips. Change-Id: I9455c367bb74b75461935ecf4a3eb8e429f6e95e Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_ethdr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c b/drivers/gpu/drm/mediatek/mtk_ethdr.c index 9dfd13d32dfa..80ccdad3741b 100644 --- a/drivers/gpu/drm/mediatek/mtk_ethdr.c +++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c @@ -3,6 +3,7 @@ * Copyright (c) 2021 MediaTek Inc. */ +#include #include #include #include @@ -175,7 +176,8 @@ void mtk_ethdr_layer_config(struct device *dev, unsigned int idx, alpha_con |= state->base.alpha & MIXER_ALPHA; } - if (state->base.fb && !state->base.fb->format->has_alpha) { + if ((state->base.fb && !state->base.fb->format->has_alpha) || + state->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) { /* * Mixer doesn't support CONST_BLD mode, * use a trick to make the output equivalent -- 2.43.0
[PATCH v2 5/5] drm/mediatek: Support alpha blending in display driver
From: Hsiao Chien Sung Support "Pre-multiplied" and "None" blend mode on MediaTek's chips by adding correct blend mode property when the planes init. Before this patch, only the "Coverage" mode (default) is supported. For more information, there are three pixel blend modes in DRM driver: "None", "Pre-multiplied", and "Coverage". To understand the difference between these modes, let's take a look at the following two approaches to do alpha blending: 1. Straight: dst.RGB = src.RGB * src.A + dst.RGB * (1 - src.A) This is straightforward and easy to understand, when the source layer is compositing with the destination layer, it's alpha will affect the result. This is also known as "post-multiplied", or "Coverage" mode. 2. Pre-multiplied: dst.RGB = src.RGB + dst.RGB * (1 - src.A) Since the source RGB have already multiplied its alpha, only destination RGB need to multiply it. This is the "Pre-multiplied" mode in DRM. For the "None" blend mode in DRM, it means the pixel alpha is ignored when compositing the layers, only the constant alpha for the composited layer will take effects. Reviewed-by: CK Hu Reviewed-by: AngeloGioacchino Del Regno Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_plane.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c index 1723d4333f37..5bf757a3ef20 100644 --- a/drivers/gpu/drm/mediatek/mtk_plane.c +++ b/drivers/gpu/drm/mediatek/mtk_plane.c @@ -346,6 +346,17 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane, DRM_INFO("Create rotation property failed\n"); } + err = drm_plane_create_alpha_property(plane); + if (err) + DRM_ERROR("failed to create property: alpha\n"); + + err = drm_plane_create_blend_mode_property(plane, + BIT(DRM_MODE_BLEND_PREMULTI) | + BIT(DRM_MODE_BLEND_COVERAGE) | + BIT(DRM_MODE_BLEND_PIXEL_NONE)); + if (err) + DRM_ERROR("failed to create property: blend_mode\n"); + drm_plane_helper_add(plane, &mtk_plane_helper_funcs); return 0; -- 2.43.0
[PATCH v2 0/5] Support alpha blending in MTK display driver
Support "Pre-multiplied" and "None" blend mode on MediaTek's chips by adding correct blend mode property when the planes init. Before this patch, only the "Coverage" mode (default) is supported. Signed-off-by: Hsiao Chien Sung --- Changes in v2: - Remove unnecessary codes - Add more information to the commit message - Link to v1: https://lore.kernel.org/r/20240620-blend-v1-0-72670072c...@mediatek.com --- Hsiao Chien Sung (5): drm/mediatek: Support "None" blending in OVL drm/mediatek: Support "None" blending in Mixer drm/mediatek: Support "Pre-multiplied" blending in OVL drm/mediatek: Support "Pre-multiplied" blending in Mixer drm/mediatek: Support alpha blending in display driver drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 36 + drivers/gpu/drm/mediatek/mtk_ethdr.c| 13 +--- drivers/gpu/drm/mediatek/mtk_plane.c| 11 ++ 3 files changed, 49 insertions(+), 11 deletions(-) --- base-commit: 8ad49a92cff4bab13eb2f2725243f5f31eff3f3b change-id: 20240710-alpha-blending-067295570863 Best regards, -- Hsiao Chien Sung
[PATCH v2 1/5] drm/mediatek: Support "None" blending in OVL
From: Hsiao Chien Sung Support "None" alpha blending mode on MediaTek's chips. Reviewed-by: CK Hu Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index 9d6d9fd8342e..add671c38613 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -434,6 +434,7 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, unsigned int fmt = pending->format; unsigned int offset = (pending->y << 16) | pending->x; unsigned int src_size = (pending->height << 16) | pending->width; + unsigned int blend_mode = state->base.pixel_blend_mode; unsigned int ignore_pixel_alpha = 0; unsigned int con; bool is_afbc = pending->modifier != DRM_FORMAT_MOD_LINEAR; @@ -463,7 +464,8 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, * For RGB888 related formats, whether CONST_BLD is enabled or not won't * affect the result. Therefore we use !has_alpha as the condition. */ - if (state->base.fb && !state->base.fb->format->has_alpha) + if ((state->base.fb && !state->base.fb->format->has_alpha) || + blend_mode == DRM_MODE_BLEND_PIXEL_NONE) ignore_pixel_alpha = OVL_CONST_BLEND; if (pending->rotation & DRM_MODE_REFLECT_Y) { -- 2.43.0
Re: [PULL] drm-intel-next
On Tue, Jul 09, 2024 at 04:27:18PM -0400, Rodrigo Vivi wrote: > On Fri, Jun 28, 2024 at 05:46:01PM +0300, Jani Nikula wrote: > > > > Hi Dave & Sima - > > > > Another feature pull towards v6.11, hopefully last. This should also fix > > the 32-bit build issue [1] seen in drm-next. > > Sima, Dave, > > I just noticed that we don't have this one yet in drm-next. > > Anything missing or wrong with this PR? Nothing, I just made a mess last week processing -next and then lost this one. Pulled into drm-next now, and thanks for the ping. -Sima > > Thanks, > Rodrigo. > > > > > BR, > > Jani. > > > > > > [1] > > https://lore.kernel.org/r/CAPM=9tyNGA2wEgnsKdSyjHRGVikywZLdueZj=sytmfyeunz...@mail.gmail.com > > > > > > drm-intel-next-2024-06-28: > > drm/i915 feature pull #2 for v6.11: > > > > Features and functionality: > > - More eDP Panel Replay enabling (Jouni) > > - Add async flip and flip done tracepoints (Ville) > > > > Refactoring and cleanups: > > - Clean up BDW+ pipe interrupt register definitions (Ville) > > - Prep work for DSB based plane programming (Ville) > > - Relocate encoder suspend/shutdown helpers (Imre) > > - Polish plane surface alignment handling (Ville) > > > > Fixes: > > - Enable more fault interrupts on TGL+/MTL+ (Ville) > > - Fix CMRR 32-bit build (Mitul) > > - Fix PSR Selective Update Region Scan Line Capture Indication (Jouni) > > - Fix cursor fb unpinning (Maarten, Ville) > > - Fix Cx0 PHY PLL state verification in TBT mode (Imre) > > - Fix unnecessary MG DP programming on MTL+ Type-C (Imre) > > > > DRM changes: > > - Rename drm_plane_check_pixel_format() to drm_plane_has_format() and export > > (Ville) > > - Add drm_vblank_work_flush_all() (Maarten) > > > > Xe driver changes: > > - Call encoder .suspend_complete() hook also on Xe (Imre) > > > > BR, > > Jani. > > > > The following changes since commit d754ed2821fd9675d203cb73c4afcd593e28b7d0: > > > > Merge drm/drm-next into drm-intel-next (2024-06-19 11:38:31 +0300) > > > > are available in the Git repository at: > > > > https://gitlab.freedesktop.org/drm/i915/kernel.git > > tags/drm-intel-next-2024-06-28 > > > > for you to fetch changes up to 32a120f52a4c0121bca8f2328d4680d283693d60: > > > > drm/i915/mtl: Skip PLL state verification in TBT mode (2024-06-28 > > 12:50:52 +0300) > > > > > > drm/i915 feature pull #2 for v6.11: > > > > Features and functionality: > > - More eDP Panel Replay enabling (Jouni) > > - Add async flip and flip done tracepoints (Ville) > > > > Refactoring and cleanups: > > - Clean up BDW+ pipe interrupt register definitions (Ville) > > - Prep work for DSB based plane programming (Ville) > > - Relocate encoder suspend/shutdown helpers (Imre) > > - Polish plane surface alignment handling (Ville) > > > > Fixes: > > - Enable more fault interrupts on TGL+/MTL+ (Ville) > > - Fix CMRR 32-bit build (Mitul) > > - Fix PSR Selective Update Region Scan Line Capture Indication (Jouni) > > - Fix cursor fb unpinning (Maarten, Ville) > > - Fix Cx0 PHY PLL state verification in TBT mode (Imre) > > - Fix unnecessary MG DP programming on MTL+ Type-C (Imre) > > > > DRM changes: > > - Rename drm_plane_check_pixel_format() to drm_plane_has_format() and export > > (Ville) > > - Add drm_vblank_work_flush_all() (Maarten) > > > > Xe driver changes: > > - Call encoder .suspend_complete() hook also on Xe (Imre) > > > > > > Imre Deak (5): > > drm/i915: Move encoder suspend/shutdown helpers to intel_encoder.c > > drm/i915: Pass intel_display to the encoder suspend/shutdown helpers > > drm/xe: Use the encoder suspend helper also used by the i915 driver > > drm/i915/display: For MTL+ platforms skip mg dp programming > > drm/i915/mtl: Skip PLL state verification in TBT mode > > > > Jouni Högander (12): > > drm/i915/psr: Set DP_PSR_SU_REGION_SCANLINE_CAPTURE bit when needed > > drm/i915/psr: Check panel ALPM capability for eDP Panel Replay > > drm/i915/psr: Inform Panel Replay source support on eDP as well > > drm/i915/psr: enable sink for eDP1.5 Panel Replay > > drm/i915/psr: Check panel Early Transport capability for eDP PR > > drm/i915/psr: 128b/132b Panel Replay is not supported on eDP > > drm/i915/psr: HW will not allow PR on eDP when HDCP enabled > > drm/i915/alpm: Make crtc_state as const in intel_alpm_compute_params > > drm/i915/psr: Perform psr2 checks related to ALPM for Panel Replay > > drm/i915/psr: Perform scanline indication check for Panel Replay as > > well > > drm/i915/psr: Check Early Transport for Panel Replay as well > > drm/i915/psr: Modify dg2_activate_panel_replay to support eDP > > > > Maarten Lankhorst (2): > > drm: Add drm_vblank_work_flush_all(). > > drm/i915: Use the same vblank worker for atomic unpin > > > > Mitul Golani (1): > > d
Re: [PATCH 2/2] drm/amd/display: use drm_crtc_set_vblank_offdelay()
On Tue, Jul 09, 2024 at 10:02:08AM -0400, Hamza Mahfooz wrote: > On 7/9/24 06:09, Daniel Vetter wrote: > > On Tue, Jul 09, 2024 at 11:32:11AM +0200, Daniel Vetter wrote: > > > On Mon, Jul 08, 2024 at 04:29:07PM -0400, Hamza Mahfooz wrote: > > > > Hook up drm_crtc_set_vblank_offdelay() in amdgpu_dm, so that we can > > > > enable PSR more quickly for displays that support it. > > > > > > > > Signed-off-by: Hamza Mahfooz > > > > --- > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 30 ++- > > > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > index fdbc9b57a23d..ee6c31e9d3c4 100644 > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > @@ -8231,7 +8231,7 @@ static int amdgpu_dm_encoder_init(struct > > > > drm_device *dev, > > > > static void manage_dm_interrupts(struct amdgpu_device *adev, > > > > struct amdgpu_crtc *acrtc, > > > > -bool enable) > > > > +struct dm_crtc_state *acrtc_state) > > > > { > > > > /* > > > > * We have no guarantee that the frontend index maps to the same > > > > @@ -8239,12 +8239,25 @@ static void manage_dm_interrupts(struct > > > > amdgpu_device *adev, > > > > * > > > > * TODO: Use a different interrupt or check DC itself for the > > > > mapping. > > > > */ > > > > - int irq_type = > > > > - amdgpu_display_crtc_idx_to_irq_type( > > > > - adev, > > > > - acrtc->crtc_id); > > > > + int irq_type = amdgpu_display_crtc_idx_to_irq_type(adev, > > > > + > > > > acrtc->crtc_id); > > > > + struct dc_crtc_timing *timing; > > > > + int offdelay; > > > > + > > > > + if (acrtc_state) { > > > > + timing = &acrtc_state->stream->timing; > > > > + > > > > + /* at least 2 frames */ > > > > + offdelay = 2000 / > > > > div64_u64(div64_u64((timing->pix_clk_100hz * > > > > + (uint64_t)100), > > > > + timing->v_total), > > > > + timing->h_total) + 1; > > > > > > Yeah, _especially_ when you have a this short timeout your really have to > > > instead fix the vblank driver code properly so you can enable > > > vblank_disable_immediate. This is just cheating :-) > > > > Michel mentioned on irc that DC had immediate vblank disabling, but this > > was reverted with f64e6e0b6afe ("Revert "drm/amdgpu/display: set > > vblank_disable_immediate for DC""). > > > > I haven't looked at the details of the bug report, but stuttering is > > exactly what happens when the driver's vblank code has these races. Going > > for a very low timeout instead of zero just means it's a bit harder to hit > > the issue, and much, much harder to debug properly. > > > > So yeah even more reasons to look at the underlying root-cause here I > > think. > > -Sima > > The issue is that DMUB (display firmware) isn't able to keep up with all of > the requests that the driver is making. The issue is fairly difficult to > reproduce (I've only seen it once after letting the system run with a > program that would engage PSR every so often, after several hours). > It is also worth noting that we have the same 2 idle frame wait on the > windows > driver, for the same reasons. So, in all likelihood if it is your opinion > that > the series should be NAKed, we will probably have to move the wait into the > driver as a workaround. Well that's an entirely different reason, and needs to be recorded in the commit log that disabling/enabling vblank is too expensive and why. Also would be good to record that windows does the same. I'm also not entirely sure this is a good interface, so some thoughts/question: - is the issue only with psr, meaning that if we switch the panel to a different crtc, do we need to update the off delay. - there's still the question of why vblank_immediate_disable resulted in stuttering, is that the same bug? I think for consistency it'd be best if we enable immediate vblank disabling everywhere (for maximum testing), and then apply the 2 frame delay workaround only where needed explicitly. Otherwise if there's other issues than DMUB being slow, they might be mostly hidden and become really hard to track down when they show up. - I think an interface to set the right values in lockstep with the vblank on/off state would be best, so maybe a special drm_crtc_vblank_on_config that takes additional parameters? Cheers, Sima -- Daniel Vetter Software Engineer, Intel Corporation htt
[PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting
The VRSTEN and HRSTEN bits control whether a CRTC synchronizes its display signal with an external source on the VIDRST pin. The G200WB and G200EW3 models synchronize with a BMC chip, but different external video encoders, such as the Matrox Maven, can also be attached to the pin. Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the bits are independent from the BMC. Add the field set_vidrst to the CRTC state for this purpose. Off by default, control the CRTC VIDRST setting from the CRTC's atomic_check helper. v2: - keep logic entirely in CRTC (Jocelyn) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_bmc.c| 5 - drivers/gpu/drm/mgag200/mgag200_drv.h| 5 - drivers/gpu/drm/mgag200/mgag200_g200er.c | 2 +- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 2 +- drivers/gpu/drm/mgag200/mgag200_g200se.c | 2 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 14 ++ 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c index 23ef85aa7e37..1c7aa4f36787 100644 --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c @@ -77,11 +77,6 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev) { u8 tmp; - /* Ensure that the vrsten and hrsten are set */ - WREG8(MGAREG_CRTCEXT_INDEX, 1); - tmp = RREG8(MGAREG_CRTCEXT_DATA); - WREG8(MGAREG_CRTCEXT_DATA, tmp | 0x88); - /* Assert rstlvl2 */ WREG8(DAC_INDEX, MGA1064_REMHEADCTL2); tmp = RREG8(DAC_DATA); diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 7f7dfbd0f013..4b75613de882 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -179,6 +179,8 @@ struct mgag200_crtc_state { const struct drm_format_info *format; struct mgag200_pll_values pixpllc; + + bool set_vidrst; }; static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base) @@ -430,7 +432,8 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st .atomic_duplicate_state = mgag200_crtc_atomic_duplicate_state, \ .atomic_destroy_state = mgag200_crtc_atomic_destroy_state -void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode); +void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode, + bool set_vidrst); void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format); void mgag200_enable_display(struct mga_device *mdev); void mgag200_init_registers(struct mga_device *mdev); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index 4e8a1756138d..abfbed6ec390 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -195,7 +195,7 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index d884f3cb0ec7..acc9e2b5 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -196,7 +196,7 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c index a824bb8ad579..be4e124102c6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c @@ -327,7 +327,7 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc, funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index d4550e4b3b01..e746f365fecf 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -201,9 +2
[PATCH v2 0/3] drm/mgag200: Control VIDRST and BMC from CRTC
(was: drm/mgag200: Handle VIDRST from BMC helpers) The VIDRST pin controls CRTC synchronization with an external clock chip, such as a BMC or TV encoder. This patchset separates the CRTC state from the BMC state and streamlines the driver code. v2: - run BMC and VIDRST logic from CRTC code (Jocelyn) Thomas Zimmermann (3): drm/mgag200: Only set VIDRST bits in CRTC modesetting drm/mgag200: Remove vidrst callbacks from struct mgag200_device_funcs drm/mgag200: Rename BMC vidrst names drivers/gpu/drm/mgag200/mgag200_bmc.c | 9 ++- drivers/gpu/drm/mgag200/mgag200_drv.h | 31 --- drivers/gpu/drm/mgag200/mgag200_g200er.c | 9 +++ drivers/gpu/drm/mgag200/mgag200_g200ev.c | 9 +++ drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 2 -- drivers/gpu/drm/mgag200/mgag200_g200se.c | 9 +++ drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 -- drivers/gpu/drm/mgag200/mgag200_mode.c| 29 ++--- 8 files changed, 36 insertions(+), 64 deletions(-) -- 2.45.2
[PATCH v2 2/3] drm/mgag200: Remove vidrst callbacks from struct mgag200_device_funcs
The callbacks disable_vidrst and enable_vidrst are obsolete. Remove the fields from struct mgag200_device_funcs. Instead call their implementations directly of the field 'has_vidrst' has been set in struct mgag200_device_info. Also change the logic slightly. The BMC used to start and stop scanout during the CRTC's atomic_enable and atomic_disable. Plane updates were done while the BMC scanned out the display. Now only stop once in atomic_disable at the beginning of a modeset and then restart the scanout at the end of a modeset in atomic_enable. While the modeset takes place, the BMC does not scanout at all. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_drv.h | 12 drivers/gpu/drm/mgag200/mgag200_g200er.c | 7 ++- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 7 ++- drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 2 -- drivers/gpu/drm/mgag200/mgag200_g200se.c | 7 ++- drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 -- drivers/gpu/drm/mgag200/mgag200_mode.c| 15 --- 7 files changed, 10 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4b75613de882..4a46c8c006c8 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -247,18 +247,6 @@ struct mgag200_device_info { } struct mgag200_device_funcs { - /* -* Disables an external reset source (i.e., BMC) before programming -* a new display mode. -*/ - void (*disable_vidrst)(struct mga_device *mdev); - - /* -* Enables an external reset source (i.e., BMC) after programming -* a new display mode. -*/ - void (*enable_vidrst)(struct mga_device *mdev); - /* * Validate that the given state can be programmed into PIXPLLC. On * success, the calculated parameters should be stored in the CRTC's diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index abfbed6ec390..b3bb3e9fb0d1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -191,9 +191,6 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); const struct drm_format_info *format = mgag200_crtc_state->format; - if (funcs->disable_vidrst) - funcs->disable_vidrst(mdev); - mgag200_set_format_regs(mdev, format); mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); @@ -209,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (funcs->enable_vidrst) - funcs->enable_vidrst(mdev); + if (mdev->info->has_vidrst) + mgag200_bmc_enable_vidrst(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index acc9e2b5..3ac0a508e2c5 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -192,9 +192,6 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); const struct drm_format_info *format = mgag200_crtc_state->format; - if (funcs->disable_vidrst) - funcs->disable_vidrst(mdev); - mgag200_set_format_regs(mdev, format); mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); @@ -210,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (funcs->enable_vidrst) - funcs->enable_vidrst(mdev); + if (mdev->info->has_vidrst) + mgag200_bmc_enable_vidrst(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c index 839401e8b465..265f3e95830a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c @@ -146,8 +146,6 @@ static const struct mgag200_device_info mgag200_g200ew3_device_info = MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false); static const struct mgag200_device_funcs mgag200_g200ew3_device_funcs = { - .disable_vidrst = mgag200_bmc_disable_vidrst, - .enable_vidrst = mgag200_bmc_enable_vidrst, .pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check, .pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, // same as G200WB }; diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c in
[PATCH v2 3/3] drm/mgag200: Rename BMC vidrst names
The BMC's scanout synchronization is only indirectly related to the VIDRST functionality. Do some renaming. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_bmc.c| 4 ++-- drivers/gpu/drm/mgag200/mgag200_drv.h| 14 +++--- drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 ++-- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 ++-- drivers/gpu/drm/mgag200/mgag200_g200se.c | 4 ++-- drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c index 1c7aa4f36787..45e35dffb3ea 100644 --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c @@ -14,7 +14,7 @@ static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connect return container_of(connector, struct mgag200_bmc_connector, base); } -void mgag200_bmc_disable_vidrst(struct mga_device *mdev) +void mgag200_bmc_stop_scanout(struct mga_device *mdev) { u8 tmp; int iter_max; @@ -73,7 +73,7 @@ void mgag200_bmc_disable_vidrst(struct mga_device *mdev) } } -void mgag200_bmc_enable_vidrst(struct mga_device *mdev) +void mgag200_bmc_start_scanout(struct mga_device *mdev) { u8 tmp; diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4a46c8c006c8..f97eaa49b089 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -216,8 +216,8 @@ struct mgag200_device_info { */ unsigned long max_mem_bandwidth; - /* HW has external source (e.g., BMC) to synchronize with */ - bool has_vidrst:1; + /* Synchronize scanout with BMC */ + bool sync_bmc:1; struct { unsigned data_bit:3; @@ -232,13 +232,13 @@ struct mgag200_device_info { }; #define MGAG200_DEVICE_INFO_INIT(_max_hdisplay, _max_vdisplay, _max_mem_bandwidth, \ -_has_vidrst, _i2c_data_bit, _i2c_clock_bit, \ +_sync_bmc, _i2c_data_bit, _i2c_clock_bit, \ _bug_no_startadd) \ { \ .max_hdisplay = (_max_hdisplay), \ .max_vdisplay = (_max_vdisplay), \ .max_mem_bandwidth = (_max_mem_bandwidth), \ - .has_vidrst = (_has_vidrst), \ + .sync_bmc = (_sync_bmc), \ .i2c = { \ .data_bit = (_i2c_data_bit), \ .clock_bit = (_i2c_clock_bit), \ @@ -430,9 +430,9 @@ int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_avail /* mgag200_vga.c */ int mgag200_vga_output_init(struct mga_device *mdev); - /* mgag200_bmc.c */ -void mgag200_bmc_disable_vidrst(struct mga_device *mdev); -void mgag200_bmc_enable_vidrst(struct mga_device *mdev); +/* mgag200_bmc.c */ +void mgag200_bmc_stop_scanout(struct mga_device *mdev); +void mgag200_bmc_start_scanout(struct mga_device *mdev); int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector); #endif /* __MGAG200_DRV_H__ */ diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index b3bb3e9fb0d1..737a48aa9160 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -206,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (mdev->info->has_vidrst) - mgag200_bmc_enable_vidrst(mdev); + if (mdev->info->sync_bmc) + mgag200_bmc_start_scanout(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index 3ac0a508e2c5..8d1ccc2ad94a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -207,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (mdev->info->has_vidrst) - mgag200_bmc_enable_vidrst(mdev); + if (mdev->info->sync_bmc) + mgag200_bmc_start_scanout(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c index 7a8099eb100c..cf7f6897838f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c @@ -338,8 +338,8 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (mdev->info->has_vidrst) - mgag200_bmc_enable_vidrst(mdev); + if (mdev->info->sync_bmc) +
[PATCH v1 0/4] Break some CMDS into helper functions
This series main purpose is to break some common CMDS into helper functions (select page and reload CMDS), refer to the discussion between Linus and Doug [1]. It is expected that there will be no impact on the functionality, but I don???t have an actual board to verify it. [1] https://lore.kernel.org/dri-devel/CAD=FV=vssfzbxwh6i4e_mhht8vz_cnxcruhoetueo5xn-fm...@mail.gmail.com/ Cong Yang (4): drm/panel: boe-tv101wum-nl6: Break some CMDS into helper functions drm/panel: nt35521: Break some CMDS into helper functions drm/panel: nt36672e: Break some CMDS into helper functions drm/panel: ili9806e: Break some CMDS into helper functions .../gpu/drm/panel/panel-boe-tv101wum-nl6.c| 190 ++ drivers/gpu/drm/panel/panel-ilitek-ili9806e.c | 14 +- .../gpu/drm/panel/panel-novatek-nt36672e.c| 69 --- .../panel/panel-sony-tulip-truly-nt35521.c| 29 ++- 4 files changed, 138 insertions(+), 164 deletions(-) -- 2.25.1
[PATCH v1 1/4] drm/panel: boe-tv101wum-nl6: Break some CMDS into helper functions
hj110iz-01a and tv110c9m-ll3 both nt36523 controller, and they have some common cmds, so let's break them into helper functions. Signed-off-by: Cong Yang --- .../gpu/drm/panel/panel-boe-tv101wum-nl6.c| 190 ++ 1 file changed, 63 insertions(+), 127 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c index ce919a980875..3e5b0d8636d0 100644 --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c @@ -54,12 +54,22 @@ struct boe_panel { struct gpio_desc *enable_gpio; }; +#define NT36523_DCS_SWITCH_PAGE0xff + +#define nt36523_switch_page(ctx, page) \ + mipi_dsi_dcs_write_seq_multi(ctx, NT36523_DCS_SWITCH_PAGE, (page)) + +static void nt36523_enable_reload_cmds(struct mipi_dsi_multi_context *ctx) +{ + mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); +} + static int boe_tv110c9m_init(struct boe_panel *boe) { struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi }; - mipi_dsi_dcs_write_seq_multi(&ctx, 0xff, 0x20); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xfb, 0x01); + nt36523_switch_page(&ctx, 0x20); + nt36523_enable_reload_cmds(&ctx); mipi_dsi_dcs_write_seq_multi(&ctx, 0x05, 0xd9); mipi_dsi_dcs_write_seq_multi(&ctx, 0x07, 0x78); mipi_dsi_dcs_write_seq_multi(&ctx, 0x08, 0x5a); @@ -99,16 +109,14 @@ static int boe_tv110c9m_init(struct boe_panel *boe) mipi_dsi_dcs_write_seq_multi(&ctx, 0xbb, 0x03, 0x8e, 0x03, 0xa2, 0x03, 0xb7, 0x03, 0xe7, 0x03, 0xfd, 0x03, 0xff); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xff, 0x21); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xfb, 0x01); - + nt36523_switch_page(&ctx, 0x21); + nt36523_enable_reload_cmds(&ctx); mipi_dsi_dcs_write_seq_multi(&ctx, 0xb0, 0x00, 0x00, 0x00, 0x1b, 0x00, 0x45, 0x00, 0x65, 0x00, 0x81, 0x00, 0x99, 0x00, 0xae, 0x00, 0xc1); mipi_dsi_dcs_write_seq_multi(&ctx, 0xb1, 0x00, 0xd2, 0x01, 0x0b, 0x01, 0x34, 0x01, 0x76, 0x01, 0xa3, 0x01, 0xef, 0x02, 0x27, 0x02, 0x29); mipi_dsi_dcs_write_seq_multi(&ctx, 0xb2, 0x02, 0x5f, 0x02, 0x9e, 0x02, 0xc9, 0x03, 0x00, 0x03, 0x26, 0x03, 0x53, 0x03, 0x63, 0x03, 0x73); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xb3, 0x03, 0x86, 0x03, 0x9a, 0x03, 0xaf, 0x03, 0xdf, 0x03, 0xf5, 0x03, 0xe0); mipi_dsi_dcs_write_seq_multi(&ctx, 0xb4, 0x00, 0x00, 0x00, 0x1b, 0x00, 0x45, 0x00, 0x65, @@ -119,89 +127,66 @@ static int boe_tv110c9m_init(struct boe_panel *boe) 0x03, 0x26, 0x03, 0x53, 0x03, 0x63, 0x03, 0x73); mipi_dsi_dcs_write_seq_multi(&ctx, 0xb7, 0x03, 0x86, 0x03, 0x9a, 0x03, 0xaf, 0x03, 0xdf, 0x03, 0xf5, 0x03, 0xe0); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xb8, 0x00, 0x00, 0x00, 0x1b, 0x00, 0x45, 0x00, 0x65, 0x00, 0x81, 0x00, 0x99, 0x00, 0xae, 0x00, 0xc1); mipi_dsi_dcs_write_seq_multi(&ctx, 0xb9, 0x00, 0xd2, 0x01, 0x0b, 0x01, 0x34, 0x01, 0x76, 0x01, 0xa3, 0x01, 0xef, 0x02, 0x27, 0x02, 0x29); mipi_dsi_dcs_write_seq_multi(&ctx, 0xba, 0x02, 0x5f, 0x02, 0x9e, 0x02, 0xc9, 0x03, 0x00, 0x03, 0x26, 0x03, 0x53, 0x03, 0x63, 0x03, 0x73); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xbb, 0x03, 0x86, 0x03, 0x9a, 0x03, 0xaf, 0x03, 0xdf, 0x03, 0xf5, 0x03, 0xe0); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xff, 0x24); - mipi_dsi_dcs_write_seq_multi(&ctx, 0xfb, 0x01); + nt36523_switch_page(&ctx, 0x24); + nt36523_enable_reload_cmds(&ctx); mipi_dsi_dcs_write_seq_multi(&ctx, 0x00, 0x00); mipi_dsi_dcs_write_seq_multi(&ctx, 0x01, 0x00); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x02, 0x1c); mipi_dsi_dcs_write_seq_multi(&ctx, 0x03, 0x1c); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x04, 0x1d); mipi_dsi_dcs_write_seq_multi(&ctx, 0x05, 0x1d); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x06, 0x04); mipi_dsi_dcs_write_seq_multi(&ctx, 0x07, 0x04); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x08, 0x0f); mipi_dsi_dcs_write_seq_multi(&ctx, 0x09, 0x0f); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x0a, 0x0e); mipi_dsi_dcs_write_seq_multi(&ctx, 0x0b, 0x0e); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x0c, 0x0d); mipi_dsi_dcs_write_seq_multi(&ctx, 0x0d, 0x0d); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x0e, 0x0c); mipi_dsi_dcs_write_seq_multi(&ctx, 0x0f, 0x0c); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x10, 0x08); mipi_dsi_dcs_write_seq_multi(&ctx, 0x11, 0x08); - mipi_dsi_dcs_write_seq_multi(&ctx, 0x12, 0x00); mipi_dsi_
[PATCH v1 2/4] drm/panel: nt35521: Break some CMDS into helper functions
Break select page cmds into helper functions. Signed-off-by: Cong Yang --- .../panel/panel-sony-tulip-truly-nt35521.c| 29 ++- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c index f2198fa29735..104b2290560e 100644 --- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c +++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c @@ -25,6 +25,12 @@ struct truly_nt35521 { struct gpio_desc *blen_gpio; }; +#define NT35521_DCS_SWITCH_PAGE0xf0 + +#define nt35521_switch_page(dsi_ctx, page) \ + mipi_dsi_dcs_write_seq_multi(dsi_ctx, NT35521_DCS_SWITCH_PAGE, \ +0x55, 0xaa, 0x52, 0x08, (page)) + static inline struct truly_nt35521 *to_truly_nt35521(struct drm_panel *panel) { @@ -48,7 +54,7 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) dsi->mode_flags |= MIPI_DSI_MODE_LPM; - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x00); + nt35521_switch_page(&dsi_ctx, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xff, 0xaa, 0x55, 0xa5, 0x80); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x6f, 0x11, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf7, 0x20, 0x00); @@ -59,7 +65,8 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xbb, 0x11, 0x11); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xbc, 0x00, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb6, 0x02); - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x01); + + nt35521_switch_page(&dsi_ctx, 0x01); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x09, 0x09); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb1, 0x09, 0x09); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xbc, 0x8c, 0x00); @@ -71,7 +78,8 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb4, 0x25, 0x25); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb9, 0x43, 0x43); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xba, 0x24, 0x24); - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x02); + + nt35521_switch_page(&dsi_ctx, 0x02); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xee, 0x03); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x00, 0xb2, 0x00, 0xb3, 0x00, 0xb6, 0x00, 0xc3, @@ -103,7 +111,8 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) 0x02, 0x93, 0x02, 0xcd, 0x02, 0xf6, 0x03, 0x31, 0x03, 0x6c, 0x03, 0xe9, 0x03, 0xef, 0x03, 0xf4); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xbb, 0x03, 0xf6, 0x03, 0xf7); - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x03); + + nt35521_switch_page(&dsi_ctx, 0x03); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x22, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb1, 0x22, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb2, 0x05, 0x00, 0x60, 0x00, 0x00); @@ -122,7 +131,8 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xc5, 0xc0); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xc6, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xc7, 0x00); - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x05); + + nt35521_switch_page(&dsi_ctx, 0x05); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x17, 0x06); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb1, 0x17, 0x06); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb2, 0x17, 0x06); @@ -178,7 +188,8 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xeb, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xec, 0x00); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xed, 0x30); - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x06); + + nt35521_switch_page(&dsi_ctx, 0x06); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x31, 0x31); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb1, 0x31, 0x31); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb2, 0x2d, 0x2e); @@ -235,10 +246,12 @@ static int truly_nt35521_on(struct truly_nt35521 *ctx) mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x6f, 0x11); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf3, 0x01); mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x35, 0x00); - mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x00); + + nt35521_switch_page(&dsi_ctx, 0x00); mipi_dsi_generic_write_seq_mul
[PATCH v1 3/4] drm/panel: nt36672e: Break some CMDS into helper functions
Break select page cmds and reload cmds into helper functions. Signed-off-by: Cong Yang --- .../gpu/drm/panel/panel-novatek-nt36672e.c| 69 --- 1 file changed, 44 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672e.c b/drivers/gpu/drm/panel/panel-novatek-nt36672e.c index e81a70147259..8c9e04207ba9 100644 --- a/drivers/gpu/drm/panel/panel-novatek-nt36672e.c +++ b/drivers/gpu/drm/panel/panel-novatek-nt36672e.c @@ -44,6 +44,16 @@ struct nt36672e_panel { const struct panel_desc *desc; }; +#define NT36672E_DCS_SWITCH_PAGE 0xff + +#define nt36672e_switch_page(ctx, page) \ + mipi_dsi_dcs_write_seq_multi(ctx, NT36672E_DCS_SWITCH_PAGE, (page)) + +static void nt36672e_enable_reload_cmds(struct mipi_dsi_multi_context *ctx) +{ + mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); +} + static inline struct nt36672e_panel *to_nt36672e_panel(struct drm_panel *panel) { return container_of(panel, struct nt36672e_panel, panel); @@ -51,16 +61,16 @@ static inline struct nt36672e_panel *to_nt36672e_panel(struct drm_panel *panel) static void nt36672e_1080x2408_60hz_init(struct mipi_dsi_multi_context *ctx) { - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x10); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + nt36672e_switch_page(ctx, 0x10); + nt36672e_enable_reload_cmds(ctx); mipi_dsi_dcs_write_seq_multi(ctx, 0xb0, 0x00); mipi_dsi_dcs_write_seq_multi(ctx, 0xc0, 0x00); mipi_dsi_dcs_write_seq_multi(ctx, 0xc1, 0x89, 0x28, 0x00, 0x08, 0x00, 0xaa, 0x02, 0x0e, 0x00, 0x2b, 0x00, 0x07, 0x0d, 0xb7, 0x0c, 0xb7); - mipi_dsi_dcs_write_seq_multi(ctx, 0xc2, 0x1b, 0xa0); - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x20); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + + nt36672e_switch_page(ctx, 0x20); + nt36672e_enable_reload_cmds(ctx); mipi_dsi_dcs_write_seq_multi(ctx, 0x01, 0x66); mipi_dsi_dcs_write_seq_multi(ctx, 0x06, 0x40); mipi_dsi_dcs_write_seq_multi(ctx, 0x07, 0x38); @@ -76,8 +86,9 @@ static void nt36672e_1080x2408_60hz_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0xf7, 0x54); mipi_dsi_dcs_write_seq_multi(ctx, 0xf8, 0x64); mipi_dsi_dcs_write_seq_multi(ctx, 0xf9, 0x54); - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x24); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + + nt36672e_switch_page(ctx, 0x24); + nt36672e_enable_reload_cmds(ctx); mipi_dsi_dcs_write_seq_multi(ctx, 0x01, 0x0f); mipi_dsi_dcs_write_seq_multi(ctx, 0x03, 0x0c); mipi_dsi_dcs_write_seq_multi(ctx, 0x05, 0x1d); @@ -139,8 +150,9 @@ static void nt36672e_1080x2408_60hz_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0xc9, 0x00); mipi_dsi_dcs_write_seq_multi(ctx, 0xd9, 0x80); mipi_dsi_dcs_write_seq_multi(ctx, 0xe9, 0x02); - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x25); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + + nt36672e_switch_page(ctx, 0x25); + nt36672e_enable_reload_cmds(ctx); mipi_dsi_dcs_write_seq_multi(ctx, 0x18, 0x22); mipi_dsi_dcs_write_seq_multi(ctx, 0x19, 0xe4); mipi_dsi_dcs_write_seq_multi(ctx, 0x21, 0x40); @@ -164,8 +176,9 @@ static void nt36672e_1080x2408_60hz_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0xd7, 0x80); mipi_dsi_dcs_write_seq_multi(ctx, 0xef, 0x20); mipi_dsi_dcs_write_seq_multi(ctx, 0xf0, 0x84); - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x26); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + + nt36672e_switch_page(ctx, 0x26); + nt36672e_enable_reload_cmds(ctx); mipi_dsi_dcs_write_seq_multi(ctx, 0x81, 0x0f); mipi_dsi_dcs_write_seq_multi(ctx, 0x83, 0x01); mipi_dsi_dcs_write_seq_multi(ctx, 0x84, 0x03); @@ -185,8 +198,9 @@ static void nt36672e_1080x2408_60hz_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0x9c, 0x00); mipi_dsi_dcs_write_seq_multi(ctx, 0x9d, 0x00); mipi_dsi_dcs_write_seq_multi(ctx, 0x9e, 0x00); - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x27); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + + nt36672e_switch_page(ctx, 0x27); + nt36672e_enable_reload_cmds(ctx); mipi_dsi_dcs_write_seq_multi(ctx, 0x01, 0x68); mipi_dsi_dcs_write_seq_multi(ctx, 0x20, 0x81); mipi_dsi_dcs_write_seq_multi(ctx, 0x21, 0x6a); @@ -215,8 +229,9 @@ static void nt36672e_1080x2408_60hz_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0xe6, 0xd3); mipi_dsi_dcs_write_seq_multi(ctx, 0xeb, 0x03); mipi_dsi_dcs_write_seq_multi(ctx, 0xec, 0x28); - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0x2a); - mipi_dsi_dcs_write_seq_multi(ctx, 0xfb, 0x01); + + nt36672e_swit
[PATCH v1 4/4] drm/panel: ili9806e: Break some CMDS into helper functions
Break select page cmds into helper function. Signed-off-by: Cong Yang --- drivers/gpu/drm/panel/panel-ilitek-ili9806e.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c index e4a44cd26c4d..68fb9a1a4d80 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c @@ -35,6 +35,12 @@ struct ili9806e_panel { enum drm_panel_orientation orientation; }; +#define ILI9806E_DCS_SWITCH_PAGE 0xff + +#define ili9806e_switch_page(ctx, page) \ + mipi_dsi_dcs_write_seq_multi(ctx, ILI9806E_DCS_SWITCH_PAGE, \ +0xff, 0x98, 0x06, 0x04, (page)) + static const char * const regulator_names[] = { "vdd", "vccio", @@ -227,7 +233,7 @@ static void ili9806e_dsi_remove(struct mipi_dsi_device *dsi) static void com35h3p70ulc_init(struct mipi_dsi_multi_context *ctx) { /* Switch to page 1 */ - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x01); + ili9806e_switch_page(ctx, 0x01); /* Interface Settings */ mipi_dsi_dcs_write_seq_multi(ctx, 0x08, 0x18); mipi_dsi_dcs_write_seq_multi(ctx, 0x21, 0x01); @@ -285,14 +291,14 @@ static void com35h3p70ulc_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0xcf, 0x0a); /* Switch to page 7 */ - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x07); + ili9806e_switch_page(ctx, 0x07); /* Power Control */ mipi_dsi_dcs_write_seq_multi(ctx, 0x06, 0x00); mipi_dsi_dcs_write_seq_multi(ctx, 0x18, 0x1d); mipi_dsi_dcs_write_seq_multi(ctx, 0x17, 0x32); /* Switch to page 6 */ - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x06); + ili9806e_switch_page(ctx, 0x06); /* GIP settings */ mipi_dsi_dcs_write_seq_multi(ctx, 0x00, 0x20); mipi_dsi_dcs_write_seq_multi(ctx, 0x01, 0x02); @@ -352,7 +358,7 @@ static void com35h3p70ulc_init(struct mipi_dsi_multi_context *ctx) mipi_dsi_dcs_write_seq_multi(ctx, 0x53, 0x12); /* Switch to page 0 */ - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x00); + ili9806e_switch_page(ctx, 0x00); /* Interface Pixel format */ mipi_dsi_dcs_write_seq_multi(ctx, 0x3a, 0x60); }; -- 2.25.1
[PATCH v3 1/5] drm/mediatek: Support "None" blending in OVL
From: Hsiao Chien Sung Support "None" alpha blending mode on MediaTek's chips. Reviewed-by: CK Hu Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index 9d6d9fd8342e..add671c38613 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -434,6 +434,7 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, unsigned int fmt = pending->format; unsigned int offset = (pending->y << 16) | pending->x; unsigned int src_size = (pending->height << 16) | pending->width; + unsigned int blend_mode = state->base.pixel_blend_mode; unsigned int ignore_pixel_alpha = 0; unsigned int con; bool is_afbc = pending->modifier != DRM_FORMAT_MOD_LINEAR; @@ -463,7 +464,8 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, * For RGB888 related formats, whether CONST_BLD is enabled or not won't * affect the result. Therefore we use !has_alpha as the condition. */ - if (state->base.fb && !state->base.fb->format->has_alpha) + if ((state->base.fb && !state->base.fb->format->has_alpha) || + blend_mode == DRM_MODE_BLEND_PIXEL_NONE) ignore_pixel_alpha = OVL_CONST_BLEND; if (pending->rotation & DRM_MODE_REFLECT_Y) { -- 2.43.0
[PATCH v3 0/5] Support alpha blending in MTK display driver
Support "Pre-multiplied" and "None" blend mode on MediaTek's chips by adding correct blend mode property when the planes init. Before this patch, only the "Coverage" mode (default) is supported. Signed-off-by: Hsiao Chien Sung --- Changes in v3: - Remove the Change-Id - Link to v2: https://lore.kernel.org/r/20240710-alpha-blending-v2-0-d4b505e69...@mediatek.com Changes in v2: - Remove unnecessary codes - Add more information to the commit message - Link to v1: https://lore.kernel.org/r/20240620-blend-v1-0-72670072c...@mediatek.com --- Hsiao Chien Sung (5): drm/mediatek: Support "None" blending in OVL drm/mediatek: Support "None" blending in Mixer drm/mediatek: Support "Pre-multiplied" blending in OVL drm/mediatek: Support "Pre-multiplied" blending in Mixer drm/mediatek: Support alpha blending in display driver drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 36 + drivers/gpu/drm/mediatek/mtk_ethdr.c| 13 +--- drivers/gpu/drm/mediatek/mtk_plane.c| 11 ++ 3 files changed, 49 insertions(+), 11 deletions(-) --- base-commit: 8ad49a92cff4bab13eb2f2725243f5f31eff3f3b change-id: 20240710-alpha-blending-067295570863 Best regards, -- Hsiao Chien Sung
[PATCH v3 3/5] drm/mediatek: Support "Pre-multiplied" blending in OVL
From: Hsiao Chien Sung Support "Pre-multiplied" alpha blending mode on in OVL. Before this patch, only the "coverage" mode is supported. Signed-off-by: Hsiao Chien Sung Reviewed-by: CK Hu --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 32 +--- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index add671c38613..89b439dcf3a6 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -56,8 +56,12 @@ #define GMC_THRESHOLD_HIGH ((1 << GMC_THRESHOLD_BITS) / 4) #define GMC_THRESHOLD_LOW ((1 << GMC_THRESHOLD_BITS) / 8) +#define OVL_CON_CLRFMT_MAN BIT(23) #define OVL_CON_BYTE_SWAP BIT(24) -#define OVL_CON_MTX_YUV_TO_RGB (6 << 16) + +/* OVL_CON_RGB_SWAP works only if OVL_CON_CLRFMT_MAN is enabled */ +#define OVL_CON_RGB_SWAP BIT(25) + #define OVL_CON_CLRFMT_RGB (1 << 12) #define OVL_CON_CLRFMT_ARGB(2 << 12) #define OVL_CON_CLRFMT_RGBA(3 << 12) @@ -65,6 +69,11 @@ #define OVL_CON_CLRFMT_BGRA(OVL_CON_CLRFMT_ARGB | OVL_CON_BYTE_SWAP) #define OVL_CON_CLRFMT_UYVY(4 << 12) #define OVL_CON_CLRFMT_YUYV(5 << 12) +#define OVL_CON_MTX_YUV_TO_RGB (6 << 16) +#define OVL_CON_CLRFMT_PARGB ((3 << 12) | OVL_CON_CLRFMT_MAN) +#define OVL_CON_CLRFMT_PABGR (OVL_CON_CLRFMT_PARGB | OVL_CON_RGB_SWAP) +#define OVL_CON_CLRFMT_PBGRA (OVL_CON_CLRFMT_PARGB | OVL_CON_BYTE_SWAP) +#define OVL_CON_CLRFMT_PRGBA (OVL_CON_CLRFMT_PABGR | OVL_CON_BYTE_SWAP) #define OVL_CON_CLRFMT_RGB565(ovl) ((ovl)->data->fmt_rgb565_is_0 ? \ 0 : OVL_CON_CLRFMT_RGB) #define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ? \ @@ -377,7 +386,8 @@ void mtk_ovl_layer_off(struct device *dev, unsigned int idx, DISP_REG_OVL_RDMA_CTRL(idx)); } -static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl, unsigned int fmt) +static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl, unsigned int fmt, + unsigned int blend_mode) { /* The return value in switch "MEM_MODE_INPUT_FORMAT_XXX" * is defined in mediatek HW data sheet. @@ -398,22 +408,30 @@ static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl, unsigned int fmt) case DRM_FORMAT_RGBA: case DRM_FORMAT_RGBX1010102: case DRM_FORMAT_RGBA1010102: - return OVL_CON_CLRFMT_RGBA; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_RGBA : + OVL_CON_CLRFMT_PRGBA; case DRM_FORMAT_BGRX: case DRM_FORMAT_BGRA: case DRM_FORMAT_BGRX1010102: case DRM_FORMAT_BGRA1010102: - return OVL_CON_CLRFMT_BGRA; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_BGRA : + OVL_CON_CLRFMT_PBGRA; case DRM_FORMAT_XRGB: case DRM_FORMAT_ARGB: case DRM_FORMAT_XRGB2101010: case DRM_FORMAT_ARGB2101010: - return OVL_CON_CLRFMT_ARGB; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_ARGB : + OVL_CON_CLRFMT_PARGB; case DRM_FORMAT_XBGR: case DRM_FORMAT_ABGR: case DRM_FORMAT_XBGR2101010: case DRM_FORMAT_ABGR2101010: - return OVL_CON_CLRFMT_ABGR; + return blend_mode == DRM_MODE_BLEND_COVERAGE ? + OVL_CON_CLRFMT_ABGR : + OVL_CON_CLRFMT_PABGR; case DRM_FORMAT_UYVY: return OVL_CON_CLRFMT_UYVY | OVL_CON_MTX_YUV_TO_RGB; case DRM_FORMAT_YUYV: @@ -453,7 +471,7 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, return; } - con = ovl_fmt_convert(ovl, fmt); + con = ovl_fmt_convert(ovl, fmt, blend_mode); if (state->base.fb) { con |= OVL_CON_AEN; con |= state->base.alpha & OVL_CON_ALPHA; -- 2.43.0
[PATCH v3 4/5] drm/mediatek: Support "Pre-multiplied" blending in Mixer
From: Hsiao Chien Sung Support "Pre-multiplied" alpha blending mode in Mixer. Before this patch, only the coverage mode is supported. To replace the default setting that is set in mtk_ethdr_config(), we change mtk_ddp_write_mask() to mtk_ddp_write(), and this change will also reset the NON_PREMULTI_SOURCE bit that was assigned in mtk_ethdr_config(). Therefore, we must still set NON_PREMULTI_SOURCE bit if the blend mode is not DRM_MODE_BLEND_PREMULTI. Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_ethdr.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c b/drivers/gpu/drm/mediatek/mtk_ethdr.c index 80ccdad3741b..d1d9cf8b10e1 100644 --- a/drivers/gpu/drm/mediatek/mtk_ethdr.c +++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c @@ -36,6 +36,7 @@ #define MIX_SRC_L0_EN BIT(0) #define MIX_L_SRC_CON(n) (0x28 + 0x18 * (n)) #define NON_PREMULTI_SOURCE(2 << 12) +#define PREMULTI_SOURCE(3 << 12) #define MIX_L_SRC_SIZE(n) (0x30 + 0x18 * (n)) #define MIX_L_SRC_OFFSET(n)(0x34 + 0x18 * (n)) #define MIX_FUNC_DCM0 0x120 @@ -176,6 +177,11 @@ void mtk_ethdr_layer_config(struct device *dev, unsigned int idx, alpha_con |= state->base.alpha & MIXER_ALPHA; } + if (state->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) + alpha_con |= PREMULTI_SOURCE; + else + alpha_con |= NON_PREMULTI_SOURCE; + if ((state->base.fb && !state->base.fb->format->has_alpha) || state->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) { /* @@ -193,8 +199,7 @@ void mtk_ethdr_layer_config(struct device *dev, unsigned int idx, mtk_ddp_write(cmdq_pkt, pending->height << 16 | align_width, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_SIZE(idx)); mtk_ddp_write(cmdq_pkt, offset, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_OFFSET(idx)); - mtk_ddp_write_mask(cmdq_pkt, alpha_con, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_CON(idx), - 0x1ff); + mtk_ddp_write(cmdq_pkt, alpha_con, &mixer->cmdq_base, mixer->regs, MIX_L_SRC_CON(idx)); mtk_ddp_write_mask(cmdq_pkt, BIT(idx), &mixer->cmdq_base, mixer->regs, MIX_SRC_CON, BIT(idx)); } -- 2.43.0
[PATCH v3 5/5] drm/mediatek: Support alpha blending in display driver
From: Hsiao Chien Sung Support "Pre-multiplied" and "None" blend mode on MediaTek's chips by adding correct blend mode property when the planes init. Before this patch, only the "Coverage" mode (default) is supported. For more information, there are three pixel blend modes in DRM driver: "None", "Pre-multiplied", and "Coverage". To understand the difference between these modes, let's take a look at the following two approaches to do alpha blending: 1. Straight: dst.RGB = src.RGB * src.A + dst.RGB * (1 - src.A) This is straightforward and easy to understand, when the source layer is compositing with the destination layer, it's alpha will affect the result. This is also known as "post-multiplied", or "Coverage" mode. 2. Pre-multiplied: dst.RGB = src.RGB + dst.RGB * (1 - src.A) Since the source RGB have already multiplied its alpha, only destination RGB need to multiply it. This is the "Pre-multiplied" mode in DRM. For the "None" blend mode in DRM, it means the pixel alpha is ignored when compositing the layers, only the constant alpha for the composited layer will take effects. Reviewed-by: CK Hu Reviewed-by: AngeloGioacchino Del Regno Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_plane.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c index 1723d4333f37..5bf757a3ef20 100644 --- a/drivers/gpu/drm/mediatek/mtk_plane.c +++ b/drivers/gpu/drm/mediatek/mtk_plane.c @@ -346,6 +346,17 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane, DRM_INFO("Create rotation property failed\n"); } + err = drm_plane_create_alpha_property(plane); + if (err) + DRM_ERROR("failed to create property: alpha\n"); + + err = drm_plane_create_blend_mode_property(plane, + BIT(DRM_MODE_BLEND_PREMULTI) | + BIT(DRM_MODE_BLEND_COVERAGE) | + BIT(DRM_MODE_BLEND_PIXEL_NONE)); + if (err) + DRM_ERROR("failed to create property: blend_mode\n"); + drm_plane_helper_add(plane, &mtk_plane_helper_funcs); return 0; -- 2.43.0
[PATCH v3 2/5] drm/mediatek: Support "None" blending in Mixer
From: Hsiao Chien Sung Support "None" alpha blending mode on MediaTek's chips. Signed-off-by: Hsiao Chien Sung --- drivers/gpu/drm/mediatek/mtk_ethdr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c b/drivers/gpu/drm/mediatek/mtk_ethdr.c index 9dfd13d32dfa..80ccdad3741b 100644 --- a/drivers/gpu/drm/mediatek/mtk_ethdr.c +++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c @@ -3,6 +3,7 @@ * Copyright (c) 2021 MediaTek Inc. */ +#include #include #include #include @@ -175,7 +176,8 @@ void mtk_ethdr_layer_config(struct device *dev, unsigned int idx, alpha_con |= state->base.alpha & MIXER_ALPHA; } - if (state->base.fb && !state->base.fb->format->has_alpha) { + if ((state->base.fb && !state->base.fb->format->has_alpha) || + state->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) { /* * Mixer doesn't support CONST_BLD mode, * use a trick to make the output equivalent -- 2.43.0
Re: [syzbot] [dri?] possible deadlock in modeset_lock
On Tue, Jul 09, 2024 at 06:54:26AM -0700, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit:661e504db04c Merge tag 'for-6.10-rc6-tag' of git://git.ker.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=144e9f9998 > kernel config: https://syzkaller.appspot.com/x/.config?x=864caee5f78cab51 > dashboard link: https://syzkaller.appspot.com/bug?extid=6cebc1af246fe020a2f0 > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) > 2.40 > > Unfortunately, I don't have any reproducer for this issue yet. > > Downloadable assets: > disk image: > https://storage.googleapis.com/syzbot-assets/3e115f4e545a/disk-661e504d.raw.xz > vmlinux: > https://storage.googleapis.com/syzbot-assets/48cfbafd84c8/vmlinux-661e504d.xz > kernel image: > https://storage.googleapis.com/syzbot-assets/b19b9de9b5fd/bzImage-661e504d.xz > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+6cebc1af246fe020a...@syzkaller.appspotmail.com > > == > WARNING: possible circular locking dependency detected > 6.10.0-rc6-syzkaller-00163-g661e504db04c #0 Not tainted > -- > syz.3.2274/16483 is trying to acquire lock: > 88807aca9e18 (&mm->mmap_lock){}-{3:3}, at: __might_fault+0xaa/0x120 > mm/memory.c:6234 > > but task is already holding lock: > 88801fc08518 (crtc_ww_class_mutex){+.+.}-{3:3}, at: > modeset_lock+0x2bf/0x650 drivers/gpu/drm/drm_modeset_lock.c:314 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #8 (crtc_ww_class_mutex){+.+.}-{3:3}: >lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5754 >__mutex_lock_common kernel/locking/mutex.c:608 [inline] >__ww_mutex_lock+0x1ac/0x2790 kernel/locking/mutex.c:759 >ww_mutex_lock+0x40/0x1f0 kernel/locking/mutex.c:876 >modeset_lock+0x2bf/0x650 drivers/gpu/drm/drm_modeset_lock.c:314 >drmm_mode_config_init+0xe91/0x17d0 > drivers/gpu/drm/drm_mode_config.c:454 >vkms_modeset_init drivers/gpu/drm/vkms/vkms_drv.c:156 [inline] >vkms_create drivers/gpu/drm/vkms/vkms_drv.c:215 [inline] >vkms_init+0x380/0x730 drivers/gpu/drm/vkms/vkms_drv.c:252 >do_one_initcall+0x24a/0x880 init/main.c:1267 >do_initcall_level+0x157/0x210 init/main.c:1329 >do_initcalls+0x3f/0x80 init/main.c:1345 >kernel_init_freeable+0x435/0x5d0 init/main.c:1578 >kernel_init+0x1d/0x2b0 init/main.c:1467 >ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147 >ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > > -> #7 (crtc_ww_class_acquire){+.+.}-{0:0}: >lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5754 >ww_acquire_init include/linux/ww_mutex.h:149 [inline] >drm_modeset_acquire_init+0x1b7/0x360 > drivers/gpu/drm/drm_modeset_lock.c:250 >drm_client_modeset_commit_atomic+0xd5/0x7e0 > drivers/gpu/drm/drm_client_modeset.c:1002 >drm_client_modeset_commit_locked+0xe0/0x520 > drivers/gpu/drm/drm_client_modeset.c:1166 >drm_client_modeset_commit+0x4a/0x70 > drivers/gpu/drm/drm_client_modeset.c:1192 >__drm_fb_helper_restore_fbdev_mode_unlocked+0xc3/0x170 > drivers/gpu/drm/drm_fb_helper.c:251 >drm_fb_helper_set_par+0xaf/0x100 drivers/gpu/drm/drm_fb_helper.c:1347 >fbcon_init+0x112d/0x2100 drivers/video/fbdev/core/fbcon.c:1093 >visual_init+0x2e9/0x660 drivers/tty/vt/vt.c:1011 >do_bind_con_driver+0x863/0xf60 drivers/tty/vt/vt.c:3833 >do_take_over_console+0x5e7/0x750 drivers/tty/vt/vt.c:4399 >do_fbcon_takeover+0x11a/0x200 drivers/video/fbdev/core/fbcon.c:531 >do_fb_registered drivers/video/fbdev/core/fbcon.c:2968 [inline] >fbcon_fb_registered+0x364/0x620 drivers/video/fbdev/core/fbcon.c:2988 >do_register_framebuffer drivers/video/fbdev/core/fbmem.c:449 [inline] >register_framebuffer+0x66f/0x820 drivers/video/fbdev/core/fbmem.c:515 >__drm_fb_helper_initial_config_and_unlock+0x1716/0x1df0 > drivers/gpu/drm/drm_fb_helper.c:1871 >drm_fbdev_generic_client_hotplug+0x16e/0x230 > drivers/gpu/drm/drm_fbdev_generic.c:278 >drm_client_register+0x181/0x210 drivers/gpu/drm/drm_client.c:141 >vkms_create drivers/gpu/drm/vkms/vkms_drv.c:226 [inline] >vkms_init+0x5f5/0x730 drivers/gpu/drm/vkms/vkms_drv.c:252 >do_one_initcall+0x24a/0x880 init/main.c:1267 >do_initcall_level+0x157/0x210 init/main.c:1329 >do_initcalls+0x3f/0x80 init/main.c:1345 >kernel_init_freeable+0x435/0x5d0 init/main.c:1578 >kernel_init+0x1d/0x2b0 init/main.c:1467 >ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147 >ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > > -> #6 (&client->modeset_mutex){+.
Re: [PATCH v2 4/4] drm/panic: Add a qr_code panic screen
On Tue, Jul 9, 2024 at 5:10 PM Jocelyn Falempe wrote: > > I used to list all QR versions in an enum, but I find it a bit too much > boilerplate to ensure the version is between 1 and 40. > By transparent newtypes, you mean adding "#[repr(transparent)]" to a > struct ? > I don't plan to add more "version" usage, so probably not worth it. Yeah, that is what I meant. It may not be worth it in that case -- it is just something we should generally consider when we see "raw" types appear in parameters that need extra documentation/preconditions, but sometimes it simply does not make sense. Thanks! Cheers, Miguel
Re: [PATCH v1 4/4] drm/panel: ili9806e: Break some CMDS into helper functions
On Wed Jul 10, 2024 at 10:47 AM CEST, Cong Yang wrote: > Break select page cmds into helper function. Why though? I don't find that anything easier to read. In fact, I deliberately chose not to factor that out into a function. It's just a sequence of magic commands, taken straight from the datasheet. So, I'd like to keep it that way. -michael > Signed-off-by: Cong Yang > --- > drivers/gpu/drm/panel/panel-ilitek-ili9806e.c | 14 ++ > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c > b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c > index e4a44cd26c4d..68fb9a1a4d80 100644 > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c > @@ -35,6 +35,12 @@ struct ili9806e_panel { > enum drm_panel_orientation orientation; > }; > > +#define ILI9806E_DCS_SWITCH_PAGE 0xff > + > +#define ili9806e_switch_page(ctx, page) \ > + mipi_dsi_dcs_write_seq_multi(ctx, ILI9806E_DCS_SWITCH_PAGE, \ > + 0xff, 0x98, 0x06, 0x04, (page)) > + > static const char * const regulator_names[] = { > "vdd", > "vccio", > @@ -227,7 +233,7 @@ static void ili9806e_dsi_remove(struct mipi_dsi_device > *dsi) > static void com35h3p70ulc_init(struct mipi_dsi_multi_context *ctx) > { > /* Switch to page 1 */ > - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x01); > + ili9806e_switch_page(ctx, 0x01); > /* Interface Settings */ > mipi_dsi_dcs_write_seq_multi(ctx, 0x08, 0x18); > mipi_dsi_dcs_write_seq_multi(ctx, 0x21, 0x01); > @@ -285,14 +291,14 @@ static void com35h3p70ulc_init(struct > mipi_dsi_multi_context *ctx) > mipi_dsi_dcs_write_seq_multi(ctx, 0xcf, 0x0a); > > /* Switch to page 7 */ > - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x07); > + ili9806e_switch_page(ctx, 0x07); > /* Power Control */ > mipi_dsi_dcs_write_seq_multi(ctx, 0x06, 0x00); > mipi_dsi_dcs_write_seq_multi(ctx, 0x18, 0x1d); > mipi_dsi_dcs_write_seq_multi(ctx, 0x17, 0x32); > > /* Switch to page 6 */ > - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x06); > + ili9806e_switch_page(ctx, 0x06); > /* GIP settings */ > mipi_dsi_dcs_write_seq_multi(ctx, 0x00, 0x20); > mipi_dsi_dcs_write_seq_multi(ctx, 0x01, 0x02); > @@ -352,7 +358,7 @@ static void com35h3p70ulc_init(struct > mipi_dsi_multi_context *ctx) > mipi_dsi_dcs_write_seq_multi(ctx, 0x53, 0x12); > > /* Switch to page 0 */ > - mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x00); > + ili9806e_switch_page(ctx, 0x00); > /* Interface Pixel format */ > mipi_dsi_dcs_write_seq_multi(ctx, 0x3a, 0x60); > };
Re: [PATCH v5 3/4] drm/xe/migrate: Clear CCS when clearing bo on xe2
On 7/4/2024 10:18 AM, Nirmoy Das wrote: Clearing bo with uncompress PTE will trigger a CCS clearing as well for XE2, so skip emit_copy_ccs() when on xe2 when clearing bo. v2: When clearing BO, CCS clear happens with all command as long as PTEs are uncompress. Cc: Himal Prasad Ghimiray Cc: Matthew Auld Cc: "Thomas Hellström" Signed-off-by: Nirmoy Das --- drivers/gpu/drm/xe/xe_migrate.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c index e0a3f6921572..cc8beed2bf8e 100644 --- a/drivers/gpu/drm/xe/xe_migrate.c +++ b/drivers/gpu/drm/xe/xe_migrate.c @@ -1061,7 +1061,8 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m, if (clear_vram && xe_migrate_allow_identity(clear_L0, &src_it)) xe_res_next(&src_it, clear_L0); else - emit_pte(m, bb, clear_L0_pt, clear_vram, clear_ccs, + /* Use uncompressed pte so clear happens in the real memory. */ + emit_pte(m, bb, clear_L0_pt, clear_vram, false, &src_it, clear_L0, dst); bb->cs[bb->len++] = MI_BATCH_BUFFER_END; @@ -1070,7 +1071,9 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m, if (clear_bo_data) emit_clear(gt, bb, clear_L0_ofs, clear_L0, XE_PAGE_SIZE, clear_vram); - if (xe_device_has_flat_ccs(xe)) { + /* Clearing BO with uncompress PTE will clear CCS metadata as well on XE2 */ + if (xe_device_has_flat_ccs(xe) && clear_ccs && + !(clear_bo_data && GRAPHICS_VERx100(gt_to_xe(gt)) >= 2000)) { Looking into Akshata's recent patch made me realized that I missed out on reducing batch_size when this condition is not met. emit_copy_ccs(gt, bb, clear_L0_ofs, true, m->cleared_mem_ofs, false, clear_L0); flush_flags = MI_FLUSH_DW_CCS;
Re: [syzbot] [dri?] possible deadlock in drm_modeset_lock
On Tue, Jul 09, 2024 at 10:53:18AM -0700, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit:8e2f4becf4fa Merge remote-tracking branch 'tglx/devmsi-arm.. > git tree: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git > for-kernelci > console output: https://syzkaller.appspot.com/x/log.txt?x=10676a9e98 > kernel config: https://syzkaller.appspot.com/x/.config?x=15349546db652fd3 > dashboard link: https://syzkaller.appspot.com/bug?extid=2e171785a12db2e2bd5d > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) > 2.40 > userspace arch: arm64 > > Unfortunately, I don't have any reproducer for this issue yet. > > Downloadable assets: > disk image: > https://storage.googleapis.com/syzbot-assets/ee71a34a1c26/disk-8e2f4bec.raw.xz > vmlinux: > https://storage.googleapis.com/syzbot-assets/f8a6bf3c4b1c/vmlinux-8e2f4bec.xz > kernel image: > https://storage.googleapis.com/syzbot-assets/236760504de5/Image-8e2f4bec.gz.xz > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+2e171785a12db2e2b...@syzkaller.appspotmail.com > > == > WARNING: possible circular locking dependency detected > 6.10.0-rc6-syzkaller-g8e2f4becf4fa #0 Not tainted > -- > syz.4.1912/14164 is trying to acquire lock: > ccd2e988 (&mm->mmap_lock){}-{3:3}, at: __might_fault+0x9c/0x124 > mm/memory.c:6233 > > but task is already holding lock: > c8f64518 (crtc_ww_class_mutex){+.+.}-{3:3}, at: > drm_modeset_lock+0x78/0xa4 drivers/gpu/drm/drm_modeset_lock.c:398 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #8 (crtc_ww_class_mutex){+.+.}-{3:3}: >__mutex_lock_common+0x190/0x21a0 kernel/locking/mutex.c:608 >__ww_mutex_lock kernel/locking/mutex.c:759 [inline] >ww_mutex_lock+0x64/0x3a4 kernel/locking/mutex.c:876 >modeset_lock+0x278/0x59c drivers/gpu/drm/drm_modeset_lock.c:314 >drm_modeset_lock+0x64/0xa4 drivers/gpu/drm/drm_modeset_lock.c:396 >drmm_mode_config_init+0xba0/0x1280 > drivers/gpu/drm/drm_mode_config.c:454 >vkms_modeset_init drivers/gpu/drm/vkms/vkms_drv.c:156 [inline] >vkms_create drivers/gpu/drm/vkms/vkms_drv.c:215 [inline] >vkms_init+0x2fc/0x600 drivers/gpu/drm/vkms/vkms_drv.c:252 >do_one_initcall+0x24c/0x9c0 init/main.c:1267 >do_initcall_level+0x154/0x214 init/main.c:1329 >do_initcalls+0x58/0xac init/main.c:1345 >do_basic_setup+0x8c/0xa0 init/main.c:1364 >kernel_init_freeable+0x324/0x478 init/main.c:1578 >kernel_init+0x24/0x2a0 init/main.c:1467 >ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:860 > > -> #7 (crtc_ww_class_acquire){+.+.}-{0:0}: >ww_acquire_init include/linux/ww_mutex.h:149 [inline] >drm_modeset_acquire_init+0x194/0x330 > drivers/gpu/drm/drm_modeset_lock.c:250 >drm_client_modeset_commit_atomic+0xe0/0x730 > drivers/gpu/drm/drm_client_modeset.c:1002 >drm_client_modeset_commit_locked+0xd0/0x4a8 > drivers/gpu/drm/drm_client_modeset.c:1166 >drm_client_modeset_commit+0x50/0x7c > drivers/gpu/drm/drm_client_modeset.c:1192 >__drm_fb_helper_restore_fbdev_mode_unlocked+0xd4/0x178 > drivers/gpu/drm/drm_fb_helper.c:251 >drm_fb_helper_set_par+0xc4/0x110 drivers/gpu/drm/drm_fb_helper.c:1347 >fbcon_init+0xf34/0x1eb8 drivers/video/fbdev/core/fbcon.c:1093 >visual_init+0x27c/0x548 drivers/tty/vt/vt.c:1011 >do_bind_con_driver+0x7dc/0xe04 drivers/tty/vt/vt.c:3833 >do_take_over_console+0x4ac/0x5f0 drivers/tty/vt/vt.c:4399 >do_fbcon_takeover+0x158/0x260 drivers/video/fbdev/core/fbcon.c:531 >do_fb_registered drivers/video/fbdev/core/fbcon.c:2968 [inline] >fbcon_fb_registered+0x370/0x4ec drivers/video/fbdev/core/fbcon.c:2988 >do_register_framebuffer drivers/video/fbdev/core/fbmem.c:449 [inline] >register_framebuffer+0x470/0x610 drivers/video/fbdev/core/fbmem.c:515 >__drm_fb_helper_initial_config_and_unlock+0x13b0/0x19a4 > drivers/gpu/drm/drm_fb_helper.c:1871 >drm_fb_helper_initial_config+0x48/0x64 > drivers/gpu/drm/drm_fb_helper.c:1936 >drm_fbdev_generic_client_hotplug+0x158/0x22c > drivers/gpu/drm/drm_fbdev_generic.c:278 >drm_client_register+0x144/0x1e0 drivers/gpu/drm/drm_client.c:141 >drm_fbdev_generic_setup+0x11c/0x2cc > drivers/gpu/drm/drm_fbdev_generic.c:340 >vkms_create drivers/gpu/drm/vkms/vkms_drv.c:226 [inline] >vkms_init+0x4f0/0x600 drivers/gpu/drm/vkms/vkms_drv.c:252 >do_one_initcall+0x24c/0x9c0 init/main.c:1267 >do_initcall_level+0x154/0x214 init/main.c:1329 >do_initcalls+0x58/0xac init/main.c:1345 >do_basic_setup+0x8c/0xa0 init/main.c:1364 >kernel_init_freeable
[PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it. Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping. References: https://lore.kernel.org/dri-devel/73db8b061cd43...@google.com/ Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org --- drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx); + might_fault(); + ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK) -- 2.45.2
[PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines
console_lock is the outermost subsystem lock for a lot of subsystems, which means get/put_user must nest within. Which means it cannot be acquired somewhere deeply nested in other locks, and most definitely not while holding fs locks potentially needed to resolve faults. console_trylock is the best we can do here. Including printk folks since even trylock feels realyl iffy here to me. Reported-by: syzbot+6cebc1af246fe020a...@syzkaller.appspotmail.com References: https://lore.kernel.org/dri-devel/26c1ff061cd0d...@google.com/ Signed-off-by: Daniel Vetter Fixes: a8f354284304 ("bcachefs: bch2_print_string_as_lines()") Cc: # v6.7+ Cc: Kent Overstreet Cc: Brian Foster Cc: linux-bcach...@vger.kernel.org Cc: Petr Mladek Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky Signed-off-by: Daniel Vetter --- fs/bcachefs/util.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/bcachefs/util.c b/fs/bcachefs/util.c index de331dec2a99..02381c653603 100644 --- a/fs/bcachefs/util.c +++ b/fs/bcachefs/util.c @@ -255,13 +255,14 @@ void bch2_prt_u64_base2(struct printbuf *out, u64 v) void bch2_print_string_as_lines(const char *prefix, const char *lines) { const char *p; + int locked; if (!lines) { printk("%s (null)\n", prefix); return; } - console_lock(); + locked = console_trylock(); while (1) { p = strchrnul(lines, '\n'); printk("%s%.*s\n", prefix, (int) (p - lines), lines); @@ -269,7 +270,8 @@ void bch2_print_string_as_lines(const char *prefix, const char *lines) break; lines = p + 1; } - console_unlock(); + if (locked) + console_unlock(); } int bch2_save_backtrace(bch_stacktrace *stack, struct task_struct *task, unsigned skipnr, -- 2.45.2
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
Re: [PATCH v3] accel/ivpu: Add missing MODULE_FIRMWARE metadata
Reviewed-by: Jacek Lawrynowicz On 09.07.2024 13:54, Alexander F. Lent wrote: > Modules that load firmware from various paths at runtime must declare > those paths at compile time, via the MODULE_FIRMWARE macro, so that the > firmware paths are included in the module's metadata. > > The accel/ivpu driver loads firmware but lacks this metadata, > preventing dracut from correctly locating firmware files. Fix it. > > Fixes: 9ab43e95f922 ("accel/ivpu: Switch to generation based FW names") > Fixes: 02d5b0aacd05 ("accel/ivpu: Implement firmware parsing and booting") > Signed-off-by: Alexander F. Lent > --- > Hi Jacek, > > Thanks for catching the error, and for the more succinct comment. > Please find v3 attached. > --- > Changes in v3: > - Simplify comment, per review. > - Fix typo in 40xx firmware path, per review. > - Link to v2: > https://lore.kernel.org/r/20240708-fix-ivpu-firmware-metadata-v2-1-78b953172...@xanderlent.com > > Changes in v2: > - Only annotate the module with the production firmware paths, per review. > - Drop macros for de-duping firmware fileames, just use string literals, per > review. > - Link to v1: > https://lore.kernel.org/r/20240705-fix-ivpu-firmware-metadata-v1-1-704b73852...@xanderlent.com > --- > drivers/accel/ivpu/ivpu_fw.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/accel/ivpu/ivpu_fw.c b/drivers/accel/ivpu/ivpu_fw.c > index 1457300828bf..ef717802a3c8 100644 > --- a/drivers/accel/ivpu/ivpu_fw.c > +++ b/drivers/accel/ivpu/ivpu_fw.c > @@ -58,6 +58,10 @@ static struct { > { IVPU_HW_40XX, "intel/vpu/vpu_40xx_v0.0.bin" }, > }; > > +/* Production fw_names from the table above */ > +MODULE_FIRMWARE("intel/vpu/vpu_37xx_v0.0.bin"); > +MODULE_FIRMWARE("intel/vpu/vpu_40xx_v0.0.bin"); > + > static int ivpu_fw_request(struct ivpu_device *vdev) > { > int ret = -ENOENT; > > --- > base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826 > change-id: 20240704-fix-ivpu-firmware-metadata-3d02bd60768d > > Best regards,
Re: [PATCH net-next v16 11/13] net: add devmem TCP documentation
Mina Almasry writes: > Add documentation outlining the usage and details of devmem TCP. > > Signed-off-by: Mina Almasry > Reviewed-by: Bagas Sanjaya Reviewed-by: Donald Hunter
Re: [PATCH net-next v16 02/13] net: netdev netlink api to bind dma-buf to a net device
Mina Almasry writes: > API takes the dma-buf fd as input, and binds it to the netdevice. The > user can specify the rx queues to bind the dma-buf to. > > Suggested-by: Stanislav Fomichev > Signed-off-by: Mina Almasry Reviewed-by: Donald Hunter
Re: [PATCH] drm/i915/gt: Do not consider preemption during execlists_dequeue for gen8
On 09/07/2024 15:02, Tvrtko Ursulin wrote: On 09/07/2024 13:53, Nitin Gote wrote: We're seeing a GPU HANG issue on a CHV platform, which was caused by bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries for gen8"). Gen8 platform has only timeslice and doesn't support a preemption mechanism as engines do not have a preemption timer and doesn't send an irq if the preemption timeout expires. So, add a fix to not consider preemption during dequeuing for gen8 platforms. Also move can_preemt() above need_preempt() function to resolve implicit declaration of function ‘can_preempt' error and make can_preempt() function param as const to resolve error: passing argument 1 of ‘can_preempt’ discards ‘const’ qualifier from the pointer target type. Fixes: bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries for gen8") Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11396 Suggested-by: Andi Shyti Signed-off-by: Nitin Gote Cc: Chris Wilson CC: # v5.2+ --- .../drm/i915/gt/intel_execlists_submission.c | 24 --- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 21829439e686..30631cc690f2 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -294,11 +294,26 @@ static int virtual_prio(const struct intel_engine_execlists *el) return rb ? rb_entry(rb, struct ve_node, rb)->prio : INT_MIN; } +static bool can_preempt(const struct intel_engine_cs *engine) +{ + if (GRAPHICS_VER(engine->i915) > 8) + return true; + + if (IS_CHERRYVIEW(engine->i915) || IS_BROADWELL(engine->i915)) + return false; + + /* GPGPU on bdw requires extra w/a; not implemented */ + return engine->class != RENDER_CLASS; Aren't BDW and CHV the only Gen8 platforms, in which case this function can be simplifies as: ... { return GRAPHICS_VER(engine->i915) > 8; } ? +} + static bool need_preempt(const struct intel_engine_cs *engine, const struct i915_request *rq) { int last_prio; + if ((GRAPHICS_VER(engine->i915) <= 8) && can_preempt(engine)) The GRAPHICS_VER check here looks redundant with the one inside can_preempt(). One more thing - I think gen8_emit_bb_start() becomes dead code after this and can be removed. Regards, Tvrtko + return false; + if (!intel_engine_has_semaphores(engine)) return false; @@ -3313,15 +3328,6 @@ static void remove_from_engine(struct i915_request *rq) i915_request_notify_execute_cb_imm(rq); } -static bool can_preempt(struct intel_engine_cs *engine) -{ - if (GRAPHICS_VER(engine->i915) > 8) - return true; - - /* GPGPU on bdw requires extra w/a; not implemented */ - return engine->class != RENDER_CLASS; -} - static void kick_execlists(const struct i915_request *rq, int prio) { struct intel_engine_cs *engine = rq->engine;
Re: [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines
On 2024-07-10, Daniel Vetter wrote: > console_lock is the outermost subsystem lock for a lot of subsystems, > which means get/put_user must nest within. Which means it cannot be > acquired somewhere deeply nested in other locks, and most definitely > not while holding fs locks potentially needed to resolve faults. > > console_trylock is the best we can do here. > > Including printk folks since even trylock feels realyl iffy here to > me. Using the console lock here at all is wrong. The console lock does not prevent other CPUs from calling printk() and inserting lines in between. There is no way to guarantee a contiguous ringbuffer block using multiple printk() calls. The console_lock usage should be removed. John Ogness
Re: [PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning
On Wed, Jul 10, 2024 at 09:46:51AM +0200, Thorsten Blum wrote: > As the comment explains, the if check ensures that the divisor oa_period > is a u32. Explicitly cast oa_period to u32 to remove the following > Coccinelle/coccicheck warning reported by do_div.cocci: > > WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 > instead > > Signed-off-by: Thorsten Blum > --- > drivers/gpu/drm/i915/i915_perf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c > b/drivers/gpu/drm/i915/i915_perf.c > index 0b1cd4c7a525..24722e758aaf 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf > *perf, >*/ > if (oa_period <= NSEC_PER_SEC) { > u64 tmp = NSEC_PER_SEC; > - do_div(tmp, oa_period); > + do_div(tmp, (u32)oa_period); Why is this code even using do_div() when it doesn't need the remainder? > oa_freq_hz = tmp; > } else > oa_freq_hz = 0; > -- > 2.45.2 -- Ville Syrjälä Intel
Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
Am 10.07.24 um 11:31 schrieb Daniel Vetter: We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it. Mhm, lockdep should be able to deduce that when there might be faults under the dma_resv lock there might also be faults under the drm_modeset_lock. Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping. References: https://lore.kernel.org/dri-devel/73db8b061cd43...@google.com/ Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org On the other hand pointing it out explicitly doesn't hurts us at all, so Reviewed-by: Christian König . Regards, Christian. --- drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx); + might_fault(); + ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
Re: [PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning
On 10. Jul 2024, at 13:38, Ville Syrjälä wrote: > On Wed, Jul 10, 2024 at 09:46:51AM +0200, Thorsten Blum wrote: >> As the comment explains, the if check ensures that the divisor oa_period >> is a u32. Explicitly cast oa_period to u32 to remove the following >> Coccinelle/coccicheck warning reported by do_div.cocci: >> >> WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 >> instead >> >> Signed-off-by: Thorsten Blum >> --- >> drivers/gpu/drm/i915/i915_perf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c >> b/drivers/gpu/drm/i915/i915_perf.c >> index 0b1cd4c7a525..24722e758aaf 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf >> *perf, >> */ >> if (oa_period <= NSEC_PER_SEC) { >> u64 tmp = NSEC_PER_SEC; >> - do_div(tmp, oa_period); >> + do_div(tmp, (u32)oa_period); > > Why is this code even using do_div() when it doesn't need the > remainder? do_div() is an optimized 64-by-32 division and the compiler should automatically remove the remainder if it's not used.
Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
On Wed, 10 Jul 2024 at 13:39, Christian König wrote: > > Am 10.07.24 um 11:31 schrieb Daniel Vetter: > > We already teach lockdep that dma_resv nests within drm_modeset_lock, > > but there's a lot more: All drm kms ioctl rely on being able to > > put/get_user while holding modeset locks, so we really need a > > might_fault in there too to complete the picture. Add it. > > Mhm, lockdep should be able to deduce that when there might be faults > under the dma_resv lock there might also be faults under the > drm_modeset_lock. You're not allowed to take a fault under dma_resv, because drivers might need to take that lock to handle faults. So unfortunately in our combined lockdep priming, there really seems to be no chain yet that teaches about faults possibly happening while holding drm_modeset_lock. -Sima > > > > > Motivated by a syzbot report that blew up on bcachefs doing an > > unconditional console_lock way deep in the locking hierarchy, and > > lockdep only noticing the depency loop in a drm ioctl instead of much > > earlier. This annotation will make sure such issues have a much harder > > time escaping. > > > > References: > > https://lore.kernel.org/dri-devel/73db8b061cd43...@google.com/ > > Signed-off-by: Daniel Vetter > > Cc: Maarten Lankhorst > > Cc: Maxime Ripard > > Cc: Thomas Zimmermann > > Cc: Sumit Semwal > > Cc: "Christian König" > > Cc: linux-me...@vger.kernel.org > > Cc: linaro-mm-...@lists.linaro.org > > On the other hand pointing it out explicitly doesn't hurts us at all, so > Reviewed-by: Christian König . > > Regards, > Christian. > > > --- > > drivers/gpu/drm/drm_mode_config.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > b/drivers/gpu/drm/drm_mode_config.c > > index 568972258222..37d2e0a4ef4b 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) > > if (ret == -EDEADLK) > > ret = drm_modeset_backoff(&modeset_ctx); > > > > + might_fault(); > > + > > ww_acquire_init(&resv_ctx, &reservation_ww_class); > > ret = dma_resv_lock(&resv, &resv_ctx); > > if (ret == -EDEADLK) > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v3 3/5] drm/mediatek: Support "Pre-multiplied" blending in OVL
Il 10/07/24 10:52, Hsiao Chien Sung via B4 Relay ha scritto: From: Hsiao Chien Sung Support "Pre-multiplied" alpha blending mode on in OVL. Before this patch, only the "coverage" mode is supported. Signed-off-by: Hsiao Chien Sung Reviewed-by: CK Hu --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 32 +--- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index add671c38613..89b439dcf3a6 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -56,8 +56,12 @@ #define GMC_THRESHOLD_HIGH((1 << GMC_THRESHOLD_BITS) / 4) #define GMC_THRESHOLD_LOW ((1 << GMC_THRESHOLD_BITS) / 8) +#define OVL_CON_CLRFMT_MAN BIT(23) #define OVL_CON_BYTE_SWAP BIT(24) -#define OVL_CON_MTX_YUV_TO_RGB (6 << 16) + +/* OVL_CON_RGB_SWAP works only if OVL_CON_CLRFMT_MAN is enabled */ +#define OVL_CON_RGB_SWAP BIT(25) + #define OVL_CON_CLRFMT_RGB(1 << 12) #define OVL_CON_CLRFMT_ARGB (2 << 12) #define OVL_CON_CLRFMT_RGBA (3 << 12) @@ -65,6 +69,11 @@ #define OVL_CON_CLRFMT_BGRA (OVL_CON_CLRFMT_ARGB | OVL_CON_BYTE_SWAP) #define OVL_CON_CLRFMT_UYVY (4 << 12) #define OVL_CON_CLRFMT_YUYV (5 << 12) +#define OVL_CON_MTX_YUV_TO_RGB (6 << 16) +#define OVL_CON_CLRFMT_PARGB ((3 << 12) | OVL_CON_CLRFMT_MAN) Shouldn't this be (OVL_CON_CLRFMT_RGBA | OVL_CON_CLRFMT_MAN) ?? That's getting written to the same register, so I'd guess this is like that; but then, if it is like that, why is it PARGB888 and not PRGBA888?! Regards, Angelo
Re: [PATCH v3 2/5] drm/mediatek: Support "None" blending in Mixer
Il 10/07/24 10:52, Hsiao Chien Sung via B4 Relay ha scritto: From: Hsiao Chien Sung Support "None" alpha blending mode on MediaTek's chips. Signed-off-by: Hsiao Chien Sung Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v3 4/5] drm/mediatek: Support "Pre-multiplied" blending in Mixer
Il 10/07/24 10:52, Hsiao Chien Sung via B4 Relay ha scritto: From: Hsiao Chien Sung Support "Pre-multiplied" alpha blending mode in Mixer. Before this patch, only the coverage mode is supported. To replace the default setting that is set in mtk_ethdr_config(), we change mtk_ddp_write_mask() to mtk_ddp_write(), and this change will also reset the NON_PREMULTI_SOURCE bit that was assigned in mtk_ethdr_config(). Therefore, we must still set NON_PREMULTI_SOURCE bit if the blend mode is not DRM_MODE_BLEND_PREMULTI. Signed-off-by: Hsiao Chien Sung Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Fri, Jul 05, 2024 at 04:31:34PM GMT, Thierry Reding wrote: > On Thu, Jul 04, 2024 at 02:24:49PM GMT, Maxime Ripard wrote: > > On Fri, Jun 28, 2024 at 04:42:35PM GMT, Thierry Reding wrote: > > > On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote: > > > > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > > > > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > > > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter > > > > > > > wrote: > > > > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > > > > > > But it makes me a little nervous to add a new generic > > > > > > > > > allocation flag > > > > > > > > > for a feature most hardware doesn't support (yet, at least). > > > > > > > > > So it's > > > > > > > > > hard to weigh how common the actual usage will be across all > > > > > > > > > the > > > > > > > > > heaps. > > > > > > > > > > > > > > > > > > I apologize as my worry is mostly born out of seeing vendors > > > > > > > > > really > > > > > > > > > push opaque feature flags in their old ion heaps, so in > > > > > > > > > providing a > > > > > > > > > flags argument, it was mostly intended as an escape hatch for > > > > > > > > > obviously common attributes. So having the first be something > > > > > > > > > that > > > > > > > > > seems reasonable, but isn't actually that common makes me > > > > > > > > > fret some. > > > > > > > > > > > > > > > > > > So again, not an objection, just something for folks to stew > > > > > > > > > on to > > > > > > > > > make sure this is really the right approach. > > > > > > > > > > > > > > > > Another good reason to go with full heap names instead of > > > > > > > > opaque flags on > > > > > > > > existing heaps is that with the former we can use symlinks in > > > > > > > > sysfs to > > > > > > > > specify heaps, with the latter we need a new idea. We haven't > > > > > > > > yet gotten > > > > > > > > around to implement this anywhere, but it's been in the > > > > > > > > dma-buf/heap todo > > > > > > > > since forever, and I like it as a design approach. So would be > > > > > > > > a good idea > > > > > > > > to not toss it. With that display would have symlinks to > > > > > > > > cma-ecc and cma, > > > > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority > > > > > > > > order) for a > > > > > > > > SoC where the display needs contig memory for scanout. > > > > > > > > > > > > > > So indeed that is a good point to keep in mind, but I also think > > > > > > > it > > > > > > > might re-inforce the choice of having ECC as a flag here. > > > > > > > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is > > > > > > > about > > > > > > > being able to figure out a common heap from a collection of > > > > > > > devices, > > > > > > > it's really about the ability for the driver to access the type of > > > > > > > memory. If ECC is just an attribute of the type of memory (as in > > > > > > > this > > > > > > > patch series), it being on or off won't necessarily affect > > > > > > > compatibility of the buffer with the device. Similarly "uncached" > > > > > > > seems more of an attribute of memory type and not a type itself. > > > > > > > Hardware that can access non-contiguous "system" buffers can > > > > > > > access > > > > > > > uncached system buffers. > > > > > > > > > > > > Yeah, but in graphics there's a wide band where "shit performance" > > > > > > is > > > > > > defacto "not useable (as intended at least)". > > > > > > > > > > > > So if we limit the symlink idea to just making sure zero-copy > > > > > > access is > > > > > > possible, then we might not actually solve the real world problem > > > > > > we need > > > > > > to solve. And so the symlinks become somewhat useless, and we need > > > > > > to > > > > > > somewhere encode which flags you need to use with each symlink. > > > > > > > > > > > > But I also see the argument that there's a bit a combinatorial > > > > > > explosion > > > > > > possible. So I guess the question is where we want to handle it ... > > > > > > > > > > Sorry for jumping into this discussion so late. But are we really > > > > > concerned about this combinatorial explosion in practice? It may be > > > > > theoretically possible to create any combination of these, but do we > > > > > expect more than a couple of heaps to exist in any given system? > > > > > > > > I don't worry too much about the number of heaps available in a given > > > > system, it would indeed be fairly low. > > > > > > > > My concern is about the semantics combinatorial explosion. So far, the > > > > name has carried what semantics we were supposed to get from the buffer > > > > we allocate from that heap. > > > > > > > > The more variations and concepts we'll have, the more heap names we'll > > > > need, and with confusing names since we
Re: [PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning
On Wed, Jul 10, 2024 at 01:55:32PM +0200, Thorsten Blum wrote: > On 10. Jul 2024, at 13:38, Ville Syrjälä > wrote: > > On Wed, Jul 10, 2024 at 09:46:51AM +0200, Thorsten Blum wrote: > >> As the comment explains, the if check ensures that the divisor oa_period > >> is a u32. Explicitly cast oa_period to u32 to remove the following > >> Coccinelle/coccicheck warning reported by do_div.cocci: > >> > >> WARNING: do_div() does a 64-by-32 division, please consider using > >> div64_u64 instead > >> > >> Signed-off-by: Thorsten Blum > >> --- > >> drivers/gpu/drm/i915/i915_perf.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_perf.c > >> b/drivers/gpu/drm/i915/i915_perf.c > >> index 0b1cd4c7a525..24722e758aaf 100644 > >> --- a/drivers/gpu/drm/i915/i915_perf.c > >> +++ b/drivers/gpu/drm/i915/i915_perf.c > >> @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf > >> *perf, > >> */ > >> if (oa_period <= NSEC_PER_SEC) { > >> u64 tmp = NSEC_PER_SEC; > >> - do_div(tmp, oa_period); > >> + do_div(tmp, (u32)oa_period); > > > > Why is this code even using do_div() when it doesn't need the > > remainder? > > do_div() is an optimized 64-by-32 division and the compiler should > automatically remove the remainder if it's not used. The point is that do_div() is a bad API because it magically changes the divided in place. There are more sensible 64bit division helpers in math64.h that can be used instead. oa_exponent_to_ns() also hand rolls a DIV_ROUND_UP_ULL() for some reason... -- Ville Syrjälä Intel
Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
Am 10.07.24 um 13:58 schrieb Daniel Vetter: On Wed, 10 Jul 2024 at 13:39, Christian König wrote: Am 10.07.24 um 11:31 schrieb Daniel Vetter: We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it. Mhm, lockdep should be able to deduce that when there might be faults under the dma_resv lock there might also be faults under the drm_modeset_lock. You're not allowed to take a fault under dma_resv, because drivers might need to take that lock to handle faults. So unfortunately in our combined lockdep priming, there really seems to be no chain yet that teaches about faults possibly happening while holding drm_modeset_lock. Ah, of course! You are right, it was just the other way around. Thanks, Christian. -Sima Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping. References: https://lore.kernel.org/dri-devel/73db8b061cd43...@google.com/ Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org On the other hand pointing it out explicitly doesn't hurts us at all, so Reviewed-by: Christian König . Regards, Christian. --- drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx); + might_fault(); + ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
[PATCH 1/7] dma-buf/dma-resv: Introduce dma_resv_trylock_ctx()
From: Thomas Hellström For the drm_exec_trylock() functionality, there is a need to be able to trylock a dma-resv object as part of a drm_exec transaction. Therefore expose a variant of dma_resv_trylock that also takes a struct ww_acquire_ctx parameter. Cc: Christian König Cc: Somalapuram Amaranath Cc: Matthew Brost Cc: Cc: Signed-off-by: Thomas Hellström --- include/linux/dma-resv.h | 23 ++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 8d0e34dad446..68dae8f2a22c 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -405,6 +405,27 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj, return ww_mutex_lock_slow_interruptible(&obj->lock, ctx); } +/** + * dma_resv_trylock_ctx - trylock the reservation object + * @obj: the reservation object + * @ctx: The ww acquire context or NULL. + * + * Tries to lock the reservation object for exclusive access and modification. + * Note, that the lock is only against other writers, readers will run + * concurrently with a writer under RCU. The seqlock is used to notify readers + * if they overlap with a writer. The context parameter ensures that other + * ww transactions can perform deadlock backoff if necessary, and that + * subsequent attempts to dma_resv_lock() @obj for @ctx will return + * -EALREADY. + * + * Return: true if the lock was acquired, false otherwise. + */ +static inline bool __must_check +dma_resv_trylock_ctx(struct dma_resv *obj, struct ww_acquire_ctx *ctx) +{ + return ww_mutex_trylock(&obj->lock, ctx); +} + /** * dma_resv_trylock - trylock the reservation object * @obj: the reservation object @@ -421,7 +442,7 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj, */ static inline bool __must_check dma_resv_trylock(struct dma_resv *obj) { - return ww_mutex_trylock(&obj->lock, NULL); + return dma_resv_trylock_ctx(obj, NULL); } /** -- 2.34.1
Using drm_exec in TTM
Hi guys, while this works in an initial test I just realized that we need to merge the GEM and TTM reference count first for it to be 100% reliable. Otherwise it can be that we try to evict BOs which are in the process of being torn down. And this in turn will result in a whole bunch of problems. So going to work on that first, just sending out the patch set once more so that it won't get lost. Regards, Christian.
[PATCH 2/7] drm/exec: don't immediately add the prelocked obj
Some contended objects might never be locked again in the case of eviction handling for example. Make sure that those doesn't show up in the list of locked objects until they are explicitely mentioned. Signed-off-by: Christian König --- drivers/gpu/drm/drm_exec.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c index 2da094bdf8a4..220df336fbd9 100644 --- a/drivers/gpu/drm/drm_exec.c +++ b/drivers/gpu/drm/drm_exec.c @@ -61,8 +61,11 @@ static void drm_exec_unlock_all(struct drm_exec *exec) drm_gem_object_put(obj); } - drm_gem_object_put(exec->prelocked); - exec->prelocked = NULL; + if (exec->prelocked) { + dma_resv_unlock(exec->prelocked->resv); + drm_gem_object_put(exec->prelocked); + exec->prelocked = NULL; + } } /** @@ -179,16 +182,9 @@ static int drm_exec_lock_contended(struct drm_exec *exec) dma_resv_lock_slow(obj->resv, &exec->ticket); } - ret = drm_exec_obj_locked(exec, obj); - if (unlikely(ret)) - goto error_unlock; - exec->prelocked = obj; return 0; -error_unlock: - dma_resv_unlock(obj->resv); - error_dropref: drm_gem_object_put(obj); return ret; @@ -214,6 +210,10 @@ int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object *obj) return ret; if (exec->prelocked == obj) { + ret = drm_exec_obj_locked(exec, obj); + if (unlikely(ret)) + return ret; + drm_gem_object_put(exec->prelocked); exec->prelocked = NULL; return 0; -- 2.34.1
[PATCH 4/7] drm/ttm: move LRU walk defines into new internal header
That is something drivers really shouldn't mess with. Signed-off-by: Christian König --- drivers/gpu/drm/ttm/ttm_bo.c | 1 + drivers/gpu/drm/ttm/ttm_bo_util.c | 2 + drivers/gpu/drm/ttm/ttm_bo_util.h | 67 +++ include/drm/ttm/ttm_bo.h | 35 4 files changed, 70 insertions(+), 35 deletions(-) create mode 100644 drivers/gpu/drm/ttm/ttm_bo_util.h diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 0131ec802066..41bee8696e69 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -45,6 +45,7 @@ #include #include "ttm_module.h" +#include "ttm_bo_util.h" static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo, struct ttm_placement *placement) diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 3c07f4712d5c..03e28e3d0d03 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -37,6 +37,8 @@ #include +#include "ttm_bo_util.h" + struct ttm_transfer_obj { struct ttm_buffer_object base; struct ttm_buffer_object *bo; diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.h b/drivers/gpu/drm/ttm/ttm_bo_util.h new file mode 100644 index ..c19b50809208 --- /dev/null +++ b/drivers/gpu/drm/ttm/ttm_bo_util.h @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/** + * Copyright 2024 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + **/ +#ifndef _TTM_BO_UTIL_H_ +#define _TTM_BO_UTIL_H_ + +struct ww_acquire_ctx; + +struct ttm_buffer_object; +struct ttm_operation_ctx; +struct ttm_lru_walk; + +/** struct ttm_lru_walk_ops - Operations for a LRU walk. */ +struct ttm_lru_walk_ops { + /** +* process_bo - Process this bo. +* @walk: struct ttm_lru_walk describing the walk. +* @bo: A locked and referenced buffer object. +* +* Return: Negative error code on error, User-defined positive value +* (typically, but not always, size of the processed bo) on success. +* On success, the returned values are summed by the walk and the +* walk exits when its target is met. +* 0 also indicates success, -EBUSY means this bo was skipped. +*/ + s64 (*process_bo)(struct ttm_lru_walk *walk, + struct ttm_buffer_object *bo); +}; + +/** + * struct ttm_lru_walk - Structure describing a LRU walk. + */ +struct ttm_lru_walk { + /** @ops: Pointer to the ops structure. */ + const struct ttm_lru_walk_ops *ops; + /** @ctx: Pointer to the struct ttm_operation_ctx. */ + struct ttm_operation_ctx *ctx; + /** @ticket: The struct ww_acquire_ctx if any. */ + struct ww_acquire_ctx *ticket; + /** @tryock_only: Only use trylock for locking. */ + bool trylock_only; +}; + +s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, + struct ttm_resource_manager *man, s64 target); + +#endif diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h index d1a732d56259..5f7c967222a2 100644 --- a/include/drm/ttm/ttm_bo.h +++ b/include/drm/ttm/ttm_bo.h @@ -194,41 +194,6 @@ struct ttm_operation_ctx { uint64_t bytes_moved; }; -struct ttm_lru_walk; - -/** struct ttm_lru_walk_ops - Operations for a LRU walk. */ -struct ttm_lru_walk_ops { - /** -* process_bo - Process this bo. -* @walk: struct ttm_lru_walk describing the walk. -* @bo: A locked and referenced buffer object. -* -* Return: Negative error code on error, User-defined positive value -* (typically, but not always, size of the processed bo) on success. -* On success
[PATCH 5/7] drm/ttm: move needs_unlock into the walk
Not a walk parameter but important to have that status around. Signed-off-by: Christian König --- drivers/gpu/drm/ttm/ttm_bo_util.c | 26 -- drivers/gpu/drm/ttm/ttm_bo_util.h | 2 ++ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 03e28e3d0d03..7a4bc7e9950b 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -772,15 +772,14 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo) } static bool ttm_lru_walk_trylock(struct ttm_lru_walk *walk, -struct ttm_buffer_object *bo, -bool *needs_unlock) +struct ttm_buffer_object *bo) { struct ttm_operation_ctx *ctx = walk->ctx; - *needs_unlock = false; + walk->needs_unlock = false; if (dma_resv_trylock(bo->base.resv)) { - *needs_unlock = true; + walk->needs_unlock = true; return true; } @@ -793,8 +792,7 @@ static bool ttm_lru_walk_trylock(struct ttm_lru_walk *walk, } static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk, - struct ttm_buffer_object *bo, - bool *needs_unlock) + struct ttm_buffer_object *bo) { struct dma_resv *resv = bo->base.resv; int ret; @@ -805,7 +803,7 @@ static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk, ret = dma_resv_lock(resv, walk->ticket); if (!ret) { - *needs_unlock = true; + walk->needs_unlock = true; /* * Only a single ticketlock per loop. Ticketlocks are prone * to return -EDEADLK causing the eviction to fail, so @@ -821,9 +819,10 @@ static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk, return ret; } -static void ttm_lru_walk_unlock(struct ttm_buffer_object *bo, bool locked) +static void ttm_lru_walk_unlock(struct ttm_lru_walk *walk, + struct ttm_buffer_object *bo) { - if (locked) + if (walk->needs_unlock) dma_resv_unlock(bo->base.resv); } @@ -869,7 +868,6 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, spin_lock(&bdev->lru_lock); ttm_resource_manager_for_each_res(man, &cursor, res) { struct ttm_buffer_object *bo = res->bo; - bool bo_needs_unlock = false; bool bo_locked = false; int mem_type; @@ -878,14 +876,14 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, * since if we do it the other way around, and the trylock fails, * we need to drop the lru lock to put the bo. */ - if (ttm_lru_walk_trylock(walk, bo, &bo_needs_unlock)) + if (ttm_lru_walk_trylock(walk, bo)) bo_locked = true; else if (!walk->ticket || walk->ctx->no_wait_gpu || walk->trylock_only) continue; if (!ttm_bo_get_unless_zero(bo)) { - ttm_lru_walk_unlock(bo, bo_needs_unlock); + ttm_lru_walk_unlock(walk, bo); continue; } @@ -894,7 +892,7 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, lret = 0; if (!bo_locked) - lret = ttm_lru_walk_ticketlock(walk, bo, &bo_needs_unlock); + lret = ttm_lru_walk_ticketlock(walk, bo); /* * Note that in between the release of the lru lock and the @@ -906,7 +904,7 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, if (!lret && bo->resource && bo->resource->mem_type == mem_type) lret = walk->ops->process_bo(walk, bo); - ttm_lru_walk_unlock(bo, bo_needs_unlock); + ttm_lru_walk_unlock(walk, bo); ttm_bo_put(bo); if (lret == -EBUSY || lret == -EALREADY) lret = 0; diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.h b/drivers/gpu/drm/ttm/ttm_bo_util.h index c19b50809208..c653e16ccb76 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.h +++ b/drivers/gpu/drm/ttm/ttm_bo_util.h @@ -59,6 +59,8 @@ struct ttm_lru_walk { struct ww_acquire_ctx *ticket; /** @tryock_only: Only use trylock for locking. */ bool trylock_only; + /** @needs_unlock: If the current BO needs unlocking */ + bool needs_unlock; }; s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, -- 2.34.1
[PATCH 3/7] drm/exec: provide trylock interface for eviction
The TTM eviction path has some additional requirements which make it necessary to trylock an object and then eventually keep or drop the lock again. Signed-off-by: Christian König --- drivers/gpu/drm/drm_exec.c | 77 ++ include/drm/drm_exec.h | 5 +++ 2 files changed, 82 insertions(+) diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c index 220df336fbd9..b81bf5a92d97 100644 --- a/drivers/gpu/drm/drm_exec.c +++ b/drivers/gpu/drm/drm_exec.c @@ -336,5 +336,82 @@ int drm_exec_prepare_array(struct drm_exec *exec, } EXPORT_SYMBOL(drm_exec_prepare_array); +/** + * drm_exec_trylock_obj - try to lock a GEM object + * @exec: the drm_exec object with the state + * @obj: the GEM object to trylock + * + * Try to lock a GEM object but don't grab a reference yet. + * + * Since we can't handle contention here it's illegal to trylock the first + * object. + * + * This function is suposed to be used from atomic context and we don't + * know if the GEM object will actually be used or not. So we don't grab a + * reference yet. + * + * Returns: True if the object could be locked, false otherwise. + */ +bool drm_exec_trylock_obj(struct drm_exec *exec, struct drm_gem_object *obj) +{ + if (WARN_ON(!exec->num_objects)) + return false; + + if (exec->prelocked == obj) + return true; + + return dma_resv_trylock_ctx(obj->resv, &exec->ticket); +} +EXPORT_SYMBOL(drm_exec_trylock_obj); + +/** + * drm_exec_keep_trylocked_obj - keep the trylocked obj + * @exec: the drm_exec object with the state + * @obj: the GEM object to trylock + * + * Keep a trylocked object in the drm_exec state object. Grabs a reference to + * the object and adds it to the container of locked objects. + */ +int drm_exec_keep_trylocked_obj(struct drm_exec *exec, + struct drm_gem_object *obj) +{ + int ret; + + ret = drm_exec_obj_locked(exec, obj); + if (ret) { + dma_resv_unlock(obj->resv); + return ret; + } + + if (exec->prelocked == obj) { + drm_gem_object_put(exec->prelocked); + exec->prelocked = NULL; + } + + return ret; +} +EXPORT_SYMBOL(drm_exec_keep_trylocked_obj); + +/** + * drm_exec_drop_trylocked_obj - drop the trylocked obj + * @exec: the drm_exec object with the state + * @obj: the GEM object to trylock + * + * Used to drop a trylocked object in the drm_exec state object, drop the + * reservation lock again and cleanup all references. + */ +void drm_exec_drop_trylocked_obj(struct drm_exec *exec, +struct drm_gem_object *obj) +{ + /* +* We can't drop the reference of prelocked objects since we might still +* be in atomic context. Additionally it makes sense to keep the +* prelocked object around since we might need it again later on. +*/ + if (exec->prelocked != obj) + dma_resv_unlock(obj->resv); +} +EXPORT_SYMBOL(drm_exec_drop_trylocked_obj); + MODULE_DESCRIPTION("DRM execution context"); MODULE_LICENSE("Dual MIT/GPL"); diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h index aa786b828a0a..a3943057a3e8 100644 --- a/include/drm/drm_exec.h +++ b/include/drm/drm_exec.h @@ -146,5 +146,10 @@ int drm_exec_prepare_array(struct drm_exec *exec, struct drm_gem_object **objects, unsigned int num_objects, unsigned int num_fences); +bool drm_exec_trylock_obj(struct drm_exec *exec, struct drm_gem_object *obj); +int drm_exec_keep_trylocked_obj(struct drm_exec *exec, + struct drm_gem_object *obj); +void drm_exec_drop_trylocked_obj(struct drm_exec *exec, + struct drm_gem_object *obj); #endif -- 2.34.1
[PATCH 6/7] drm/ttm: support using drm_exec during eviction v2
Allow specifying a drm_exec object in TTMs operation context which is used to lock objects during eviction. This allows to handle deadlocks much more gracefully and with that avoid returning -ENOMEM on heavily contended domains. v2: rebased on top of Thomas work TODO: This still doesn't handle BOs which are about to be torn down correctly. Signed-off-by: Christian König --- drivers/gpu/drm/ttm/ttm_bo_util.c | 45 +-- drivers/gpu/drm/ttm/ttm_bo_util.h | 2 ++ include/drm/ttm/ttm_bo.h | 3 +++ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 7a4bc7e9950b..850e329ab5a5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -36,6 +36,7 @@ #include #include +#include #include "ttm_bo_util.h" @@ -776,15 +777,22 @@ static bool ttm_lru_walk_trylock(struct ttm_lru_walk *walk, { struct ttm_operation_ctx *ctx = walk->ctx; + walk->needs_drop = false; walk->needs_unlock = false; - if (dma_resv_trylock(bo->base.resv)) { - walk->needs_unlock = true; + if (bo->base.resv == ctx->resv && ctx->allow_res_evict) { + dma_resv_assert_held(bo->base.resv); return true; } - if (bo->base.resv == ctx->resv && ctx->allow_res_evict) { - dma_resv_assert_held(bo->base.resv); + if (walk->ctx->exec) { + if (drm_exec_trylock_obj(walk->ctx->exec, &bo->base)) { + walk->needs_drop = true; + return true; + } + + } else if (dma_resv_trylock(bo->base.resv)) { + walk->needs_unlock = true; return true; } @@ -797,7 +805,9 @@ static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk, struct dma_resv *resv = bo->base.resv; int ret; - if (walk->ctx->interruptible) + if (walk->ctx->exec) + ret = drm_exec_lock_obj(walk->ctx->exec, &bo->base); + else if (walk->ctx->interruptible) ret = dma_resv_lock_interruptible(resv, walk->ticket); else ret = dma_resv_lock(resv, walk->ticket); @@ -811,7 +821,8 @@ static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk, * trylocking for this walk. */ walk->ticket = NULL; - } else if (ret == -EDEADLK) { + + } else if (!walk->ctx->exec && ret == -EDEADLK) { /* Caller needs to exit the ww transaction. */ ret = -ENOSPC; } @@ -822,7 +833,15 @@ static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk, static void ttm_lru_walk_unlock(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo) { - if (walk->needs_unlock) + if (walk->needs_drop) + drm_exec_drop_trylocked_obj(walk->ctx->exec, &bo->base); + + if (!walk->needs_unlock) + return; + + if (walk->ctx->exec) + drm_exec_unlock_obj(walk->ctx->exec, &bo->base); + else dma_resv_unlock(bo->base.resv); } @@ -891,8 +910,18 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev, spin_unlock(&bdev->lru_lock); lret = 0; - if (!bo_locked) + if (!bo_locked) { lret = ttm_lru_walk_ticketlock(walk, bo); + } else if (walk->ctx->exec && !bo->deleted) { + lret = drm_exec_keep_trylocked_obj(walk->ctx->exec, + &bo->base); + if (!lret) { + walk->needs_drop = false; + walk->needs_unlock = true; + } + } else { + lret = 0; + } /* * Note that in between the release of the lru lock and the diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.h b/drivers/gpu/drm/ttm/ttm_bo_util.h index c653e16ccb76..5e1bb156837f 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.h +++ b/drivers/gpu/drm/ttm/ttm_bo_util.h @@ -59,6 +59,8 @@ struct ttm_lru_walk { struct ww_acquire_ctx *ticket; /** @tryock_only: Only use trylock for locking. */ bool trylock_only; + /** @needs_drop: If the current BO needs a drm_exec trylock drop */ + bool needs_drop; /** @needs_unlock: If the current BO needs unlocking */ bool needs_unlock; }; diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h index 5f7c967222a2..5bee917e01e2 100644 --- a/include/drm/ttm/ttm_bo.h +++ b/include/drm/ttm/ttm_bo.h @@ -180,6 +180,8 @@ struct ttm_bo_kmap_obj { * faults. Should only be used by TTM internally. * @resv: Reservation object to allow reserved evictions with. * @byte
[PATCH 7/7] drm/amdgpu: use drm_exec during BO validation
This allows to detect deadlocks happening because of resource constraints. Especially submissions which want to use all of GDS doesn't result in sporadic -ENOMEM any more. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 ++ 1 file changed, 46 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index ec888fc6ead8..ff532c8b7a62 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -782,7 +782,7 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo) struct ttm_operation_ctx ctx = { .interruptible = true, .no_wait_gpu = false, - .resv = bo->tbo.base.resv + .exec = &p->exec, }; uint32_t domain; int r; @@ -834,7 +834,10 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, union drm_amdgpu_cs *cs) { struct amdgpu_fpriv *fpriv = p->filp->driver_priv; - struct ttm_operation_ctx ctx = { true, false }; + struct ttm_operation_ctx ctx = { + .interruptible =true, + .exec = &p->exec + }; struct amdgpu_vm *vm = &fpriv->vm; struct amdgpu_bo_list_entry *e; struct drm_gem_object *obj; @@ -919,50 +922,56 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, if (unlikely(r)) goto out_free_user_pages; } - } - - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) { - struct mm_struct *usermm; - usermm = amdgpu_ttm_tt_get_usermm(e->bo->tbo.ttm); - if (usermm && usermm != current->mm) { - r = -EPERM; - goto out_free_user_pages; - } + amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) { + struct mm_struct *usermm; - if (amdgpu_ttm_tt_is_userptr(e->bo->tbo.ttm) && - e->user_invalidated && e->user_pages) { - amdgpu_bo_placement_from_domain(e->bo, - AMDGPU_GEM_DOMAIN_CPU); - r = ttm_bo_validate(&e->bo->tbo, &e->bo->placement, - &ctx); - if (r) + usermm = amdgpu_ttm_tt_get_usermm(e->bo->tbo.ttm); + if (usermm && usermm != current->mm) { + r = -EPERM; goto out_free_user_pages; + } + + if (amdgpu_ttm_tt_is_userptr(e->bo->tbo.ttm) && + e->user_invalidated && e->user_pages) { + amdgpu_bo_placement_from_domain(e->bo, + AMDGPU_GEM_DOMAIN_CPU); + r = ttm_bo_validate(&e->bo->tbo, &e->bo->placement, + &ctx); + drm_exec_retry_on_contention(&p->exec); + if (r) + goto out_free_user_pages; + + amdgpu_ttm_tt_set_user_pages(e->bo->tbo.ttm, +e->user_pages); + } - amdgpu_ttm_tt_set_user_pages(e->bo->tbo.ttm, -e->user_pages); + kvfree(e->user_pages); + e->user_pages = NULL; } - kvfree(e->user_pages); - e->user_pages = NULL; - } + amdgpu_cs_get_threshold_for_moves(p->adev, &p->bytes_moved_threshold, + &p->bytes_moved_vis_threshold); + p->bytes_moved = 0; + p->bytes_moved_vis = 0; - amdgpu_cs_get_threshold_for_moves(p->adev, &p->bytes_moved_threshold, - &p->bytes_moved_vis_threshold); - p->bytes_moved = 0; - p->bytes_moved_vis = 0; + r = amdgpu_vm_validate(p->adev, &fpriv->vm, NULL, + amdgpu_cs_bo_validate, p); + drm_exec_retry_on_contention(&p->exec); + if (r) { + DRM_ERROR("amdgpu_vm_validate() failed.\n"); + goto out_free_user_pages; + } - r = amdgpu_vm_validate(p->adev, &fpriv->vm, NULL, - amdgpu_cs_bo_validate, p); - if (r) { - DRM_ERROR("amdgpu_vm_validate() failed.\n"); - goto out_free_user_pages; - } + drm_exec_for_each_locked_object
Re: [PATCH] drm/buddy: Add start address support to trim function
On 10/07/2024 07:03, Paneer Selvam, Arunpravin wrote: Thanks Alex. Hi Matthew, Any comments? Do we not pass the required address alignment when allocating the pages in the first place? Thanks, Arun. On 7/9/2024 1:42 AM, Alex Deucher wrote: On Thu, Jul 4, 2024 at 4:40 AM Arunpravin Paneer Selvam wrote: - Add a new start parameter in trim function to specify exact address from where to start the trimming. This would help us in situations like if drivers would like to do address alignment for specific requirements. - Add a new flag DRM_BUDDY_TRIM_DISABLE. Drivers can use this flag to disable the allocator trimming part. This patch enables the drivers control trimming and they can do it themselves based on the application requirements. v1:(Matthew) - check new_start alignment with min chunk_size - use range_overflows() Signed-off-by: Arunpravin Paneer Selvam Series is: Acked-by: Alex Deucher I'd like to take this series through the amdgpu tree if there are no objections as it's required for display buffers on some chips and I'd like to make sure it lands in 6.11. Thanks, Alex --- drivers/gpu/drm/drm_buddy.c | 25 +++-- drivers/gpu/drm/xe/xe_ttm_vram_mgr.c | 2 +- include/drm/drm_buddy.h | 2 ++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 94f8c34fc293..8cebe1fa4e9d 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -851,6 +851,7 @@ static int __alloc_contig_try_harder(struct drm_buddy *mm, * drm_buddy_block_trim - free unused pages * * @mm: DRM buddy manager + * @start: start address to begin the trimming. * @new_size: original size requested * @blocks: Input and output list of allocated blocks. * MUST contain single block as input to be trimmed. @@ -866,11 +867,13 @@ static int __alloc_contig_try_harder(struct drm_buddy *mm, * 0 on success, error code on failure. */ int drm_buddy_block_trim(struct drm_buddy *mm, + u64 *start, u64 new_size, struct list_head *blocks) { struct drm_buddy_block *parent; struct drm_buddy_block *block; + u64 block_start, block_end; LIST_HEAD(dfs); u64 new_start; int err; @@ -882,6 +885,9 @@ int drm_buddy_block_trim(struct drm_buddy *mm, struct drm_buddy_block, link); + block_start = drm_buddy_block_offset(block); + block_end = block_start + drm_buddy_block_size(mm, block); + if (WARN_ON(!drm_buddy_block_is_allocated(block))) return -EINVAL; @@ -894,6 +900,20 @@ int drm_buddy_block_trim(struct drm_buddy *mm, if (new_size == drm_buddy_block_size(mm, block)) return 0; + new_start = block_start; + if (start) { + new_start = *start; + + if (new_start < block_start) + return -EINVAL; + + if (!IS_ALIGNED(new_start, mm->chunk_size)) + return -EINVAL; + + if (range_overflows(new_start, new_size, block_end)) + return -EINVAL; + } + list_del(&block->link); mark_free(mm, block); mm->avail += drm_buddy_block_size(mm, block); @@ -904,7 +924,6 @@ int drm_buddy_block_trim(struct drm_buddy *mm, parent = block->parent; block->parent = NULL; - new_start = drm_buddy_block_offset(block); list_add(&block->tmp_link, &dfs); err = __alloc_range(mm, &dfs, new_start, new_size, blocks, NULL); if (err) { @@ -1066,7 +1085,8 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, } while (1); /* Trim the allocated block to the required size */ - if (original_size != size) { + if (!(flags & DRM_BUDDY_TRIM_DISABLE) && + original_size != size) { struct list_head *trim_list; LIST_HEAD(temp); u64 trim_size; @@ -1083,6 +1103,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, } drm_buddy_block_trim(mm, + NULL, trim_size, trim_list); diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c index fe3779fdba2c..423b261ea743 100644 --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c @@ -150,7 +150,7 @@ static int xe_ttm_vram_mgr_new(struct ttm_resource_manager *man, } while (remaining_size); if (place->flags & TTM_PL_FLAG_CONTIGUOUS) { - if (!drm_buddy_block_trim(mm, vres->base.size, &vres->blocks)) + if (!drm_buddy_block_trim(mm, NULL, vres->base.size, &vres->blo
[PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
console_lock is the outermost subsystem lock for a lot of subsystems, which means get/put_user must nest within. Which means it cannot be acquired somewhere deeply nested in other locks, and most definitely not while holding fs locks potentially needed to resolve faults. console_trylock is the best we can do here. But John pointed out on a previous version that this is futile: "Using the console lock here at all is wrong. The console lock does not prevent other CPUs from calling printk() and inserting lines in between. "There is no way to guarantee a contiguous ringbuffer block using multiple printk() calls. "The console_lock usage should be removed." https://lore.kernel.org/lkml/87frsh33xp@jogness.linutronix.de/ Do that. Reported-by: syzbot+6cebc1af246fe020a...@syzkaller.appspotmail.com References: https://lore.kernel.org/dri-devel/26c1ff061cd0d...@google.com/ Signed-off-by: Daniel Vetter Fixes: a8f354284304 ("bcachefs: bch2_print_string_as_lines()") Cc: # v6.7+ Cc: Kent Overstreet Cc: Brian Foster Cc: linux-bcach...@vger.kernel.org Cc: Petr Mladek Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky Signed-off-by: Daniel Vetter -- v2: Dont trylock, drop console_lock entirely --- fs/bcachefs/util.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/fs/bcachefs/util.c b/fs/bcachefs/util.c index de331dec2a99..dc891563d502 100644 --- a/fs/bcachefs/util.c +++ b/fs/bcachefs/util.c @@ -8,7 +8,6 @@ #include #include -#include #include #include #include @@ -261,7 +260,6 @@ void bch2_print_string_as_lines(const char *prefix, const char *lines) return; } - console_lock(); while (1) { p = strchrnul(lines, '\n'); printk("%s%.*s\n", prefix, (int) (p - lines), lines); @@ -269,7 +267,6 @@ void bch2_print_string_as_lines(const char *prefix, const char *lines) break; lines = p + 1; } - console_unlock(); } int bch2_save_backtrace(bch_stacktrace *stack, struct task_struct *task, unsigned skipnr, -- 2.45.2
Re: [PATCH v6 0/5] Support Starry er88577 MIPI-DSI panel
Hi, On Tue, 09 Jul 2024 21:47:49 +0800, Zhaoxiong Lv wrote: > The Starry is a 10.1" WXGA TFT LCD panel. Because Starry-er88577 > and boe-th101mb31ig002 have very similar inti_code, after > discussing with Dmitry Baryshkov, We will modify it based on the > panel-boe-th101mb31ig002-28a.c driver instead of using a separate > driver. > > Changes between V6 and V5: > - PATCH 1/5: Corrected the use of "->init" in struct panel_desc, and modify > indentation > - PATCH 2/5: No changes. > - PATCH 3/5: No changes. > - PATCH 4/5: Modify the commit information and "reset gpio" binding. > - PATCH 5/5: Add two lines of init_code (D1 and D3) to modify the internal > resistance of the mipi channel. > - Link to v5: > https://lore.kernel.org/all/20240704072958.27876-1-lvzhaoxi...@huaqin.corp-partner.google.com/ > > [...] Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-next) [1/5] drm/panel: boe-th101mb31ig002 : Make it compatible with other panel. https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/24179ff9a2e4524ce83014b8827a73ad03a25c13 [2/5] drm/panel: boe-th101mb31ig002: switch to devm_gpiod_get_optional() for reset_gpio https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/7f58ebaccb67cb22b2936ba79c844f1e446dc73b [3/5] drm/panel: boe-th101mb31ig002: use wrapped MIPI DCS functions https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/a16b680a2140e6cbda41ac144564696c3ee2815f [4/5] dt-bindings: display: panel: Add compatible for starry-er88577 https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/3808a15e3248820c0859d9b8a0f2c7e5c8259044 [5/5] drm/panel: boe-th101mb31ig002: Support for starry-er88577 MIPI-DSI panel https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/e4bd1db1c1f771983393bf5574854dff26ca7532 -- Neil
Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs
Am 05.07.24 um 23:14 schrieb Lukas Wunner: On Fri, Jul 05, 2024 at 12:16:14PM -0500, Jeremy Linton wrote: Am 05.07.24 um 17:03 schrieb Lukas Wunner: Careful there, the patch vaguely says... With that added and identified as "BCM2848", an id in use by other OSs for this device, the dw2 controller on the BCM2711 will work. ...which sounds like they copy-pasted the BCM2848 id from somewhere else. I would assume that BCM2848 is really a different SoC and not just a different name for the BCM2835, but hopefully BroadCom folks will be able to confirm or deny this (and thus the necessity of the quirk on BCM2848 and not just on BCM2835). This id comes from the edk2-platforms ACPI tables and is currently used by both the rpi3 and rpi4, and AFAIK nothing else as the rpi5-dev work is currently only exposing XHCI. The ID is strictly the USB controller not the SoC. Its a bit confusingly named, but something we inherited from the much older windows/edk2 port, where it appears that the peripheral HID's were just picked in numerical order. [0] https://github.com/tianocore/edk2-platforms/blob/12f68d29abdc9d703f67bd743fdec23ebb1e966e/Platform/RaspberryPi/AcpiTables/GpuDevs.asl#L15 So BCM2848, BCM2849, BCM2850 and so on are just made-up IDs for a Windows/EDK2 port that got cargo-culted into the kernel? Yikes! Has anyone checked whether they collide with actual Broadcom products? Using public available information like this [1], I wasn't able to find any collision. [1] - https://github.com/anholt/linux/wiki/Devices-with-Videocore-graphics
[PATCH 01/12] drm/v3d: Prevent out of bounds access in performance query extensions
From: Tvrtko Ursulin Check that the number of perfmons userspace is passing in the copy and reset extensions is not greater than the internal kernel storage where the ids will be copied into. Signed-off-by: Tvrtko Ursulin Fixes: bae7cb5d6800 ("drm/v3d: Create a CPU job extension for the reset performance query job" Cc: Maíra Canal Cc: Iago Toral Quiroga Cc: # v6.8+ --- drivers/gpu/drm/v3d/v3d_submit.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 88f63d526b22..263fefc1d04f 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -637,6 +637,9 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, if (copy_from_user(&reset, ext, sizeof(reset))) return -EFAULT; + if (reset.nperfmons > V3D_MAX_PERFMONS) + return -EINVAL; + job->job_type = V3D_CPU_JOB_TYPE_RESET_PERFORMANCE_QUERY; job->performance_query.queries = kvmalloc_array(reset.count, @@ -708,6 +711,9 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, if (copy.pad) return -EINVAL; + if (copy.nperfmons > V3D_MAX_PERFMONS) + return -EINVAL; + job->job_type = V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY; job->performance_query.queries = kvmalloc_array(copy.count, -- 2.44.0
[PATCH 03/12] drm/v3d: Fix potential memory leak in the performance extension
From: Tvrtko Ursulin If fetching of userspace memory fails during the main loop, all drm sync objs looked up until that point will be leaked because of the missing drm_syncobj_put. Fix it by exporting and using a common cleanup helper. Signed-off-by: Tvrtko Ursulin Fixes: bae7cb5d6800 ("drm/v3d: Create a CPU job extension for the reset performance query job" Cc: Maíra Canal Cc: Iago Toral Quiroga Cc: # v6.8+ --- drivers/gpu/drm/v3d/v3d_drv.h| 2 ++ drivers/gpu/drm/v3d/v3d_sched.c | 22 +- drivers/gpu/drm/v3d/v3d_submit.c | 40 +--- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 95651c3c926f..38c80168da51 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -565,6 +565,8 @@ void v3d_mmu_remove_ptes(struct v3d_bo *bo); /* v3d_sched.c */ void __v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, unsigned int count); +void __v3d_performance_query_info_free(struct v3d_performance_query_info *qinfo, + unsigned int count); void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); int v3d_sched_init(struct v3d_dev *v3d); void v3d_sched_fini(struct v3d_dev *v3d); diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index e45d3ddc6f82..173801aa54ee 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -87,20 +87,30 @@ __v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, } } +void +__v3d_performance_query_info_free(struct v3d_performance_query_info *qinfo, + unsigned int count) +{ + if (qinfo->queries) { + unsigned int i; + + for (i = 0; i < count; i++) + drm_syncobj_put(qinfo->queries[i].syncobj); + + kvfree(qinfo->queries); + } +} + static void v3d_cpu_job_free(struct drm_sched_job *sched_job) { struct v3d_cpu_job *job = to_cpu_job(sched_job); - struct v3d_performance_query_info *performance_query = &job->performance_query; __v3d_timestamp_query_info_free(&job->timestamp_query, job->timestamp_query.count); - if (performance_query->queries) { - for (int i = 0; i < performance_query->count; i++) - drm_syncobj_put(performance_query->queries[i].syncobj); - kvfree(performance_query->queries); - } + __v3d_performance_query_info_free(&job->performance_query, + job->performance_query.count); v3d_job_cleanup(&job->base); } diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 2818afdd4807..ca1b1ad0a75c 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -637,6 +637,7 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, u32 __user *syncs; u64 __user *kperfmon_ids; struct drm_v3d_reset_performance_query reset; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -672,32 +673,36 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, u32 id; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } - job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); - if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } ids_pointer = u64_to_user_ptr(ids); for (int j = 0; j < reset.nperfmons; j++) { if (copy_from_user(&id, ids_pointer++, sizeof(id))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->performance_query.queries[i].kperfmon_ids[j] = id; } + + job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); } job->performance_query.count = reset.count; job->performance_query.nperfmons = reset.nperfmons; return 0; + +error: + __v3d_performance_query_info_free(qinfo, i); + return err; } static int @@ -708,6 +713,7 @@ v3d_get_cpu_copy_performance_
[PATCH 02/12] drm/v3d: Fix potential memory leak in the timestamp extension
From: Tvrtko Ursulin If fetching of userspace memory fails during the main loop, all drm sync objs looked up until that point will be leaked because of the missing drm_syncobj_put. Fix it by exporting and using a common cleanup helper. Signed-off-by: Tvrtko Ursulin Fixes: 9ba0ff3e083f ("drm/v3d: Create a CPU job extension for the timestamp query job") Cc: Maíra Canal Cc: Iago Toral Quiroga Cc: # v6.8+ --- drivers/gpu/drm/v3d/v3d_drv.h| 2 ++ drivers/gpu/drm/v3d/v3d_sched.c | 22 +-- drivers/gpu/drm/v3d/v3d_submit.c | 36 ++-- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 099b962bdfde..95651c3c926f 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -563,6 +563,8 @@ void v3d_mmu_insert_ptes(struct v3d_bo *bo); void v3d_mmu_remove_ptes(struct v3d_bo *bo); /* v3d_sched.c */ +void __v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, +unsigned int count); void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); int v3d_sched_init(struct v3d_dev *v3d); void v3d_sched_fini(struct v3d_dev *v3d); diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index 03df37a3acf5..e45d3ddc6f82 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -73,18 +73,28 @@ v3d_sched_job_free(struct drm_sched_job *sched_job) v3d_job_cleanup(job); } +void +__v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *qinfo, + unsigned int count) +{ + if (qinfo->queries) { + unsigned int i; + + for (i = 0; i < count; i++) + drm_syncobj_put(qinfo->queries[i].syncobj); + + kvfree(qinfo->queries); + } +} + static void v3d_cpu_job_free(struct drm_sched_job *sched_job) { struct v3d_cpu_job *job = to_cpu_job(sched_job); - struct v3d_timestamp_query_info *timestamp_query = &job->timestamp_query; struct v3d_performance_query_info *performance_query = &job->performance_query; - if (timestamp_query->queries) { - for (int i = 0; i < timestamp_query->count; i++) - drm_syncobj_put(timestamp_query->queries[i].syncobj); - kvfree(timestamp_query->queries); - } + __v3d_timestamp_query_info_free(&job->timestamp_query, + job->timestamp_query.count); if (performance_query->queries) { for (int i = 0; i < performance_query->count; i++) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 263fefc1d04f..2818afdd4807 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -452,6 +452,7 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_timestamp_query timestamp; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -484,15 +485,15 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, u32 offset, sync; if (copy_from_user(&offset, offsets++, sizeof(offset))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].offset = offset; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); @@ -500,6 +501,10 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, job->timestamp_query.count = timestamp.count; return 0; + +error: + __v3d_timestamp_query_info_free(qinfo, i); + return err; } static int @@ -509,6 +514,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, { u32 __user *syncs; struct drm_v3d_reset_timestamp_query reset; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -539,8 +545,8 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, job->timestamp_query.queries[i].offset = reset.offset + 8 * i; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->timestamp_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; }
[PATCH v2 00/12] v3d: Perfmon cleanup
From: Tvrtko Ursulin When we had to quickly deal with a tree build issue via merging 792d16b5375d ("drm/v3d: Move perfmon init completely into own unit"), we promised to follow up with a nicer solution. As in the process of eliminating the hardcoded defines we have discovered a few issues in handling of corner cases and userspace input validation, the fix has turned into a larger series, but hopefully the end result is a justifiable cleanup. v2: * Re-order the patches so fixes come first while last three are optional cleanups. Tvrtko Ursulin (12): drm/v3d: Prevent out of bounds access in performance query extensions drm/v3d: Fix potential memory leak in the timestamp extension drm/v3d: Fix potential memory leak in the performance extension drm/v3d: Validate passed in drm syncobj handles in the timestamp extension drm/v3d: Validate passed in drm syncobj handles in the performance extension drm/v3d: Move part of copying of reset/copy performance extension to a helper drm/v3d: Size the kperfmon_ids array at runtime drm/v3d: Do not use intermediate storage when copying performance query results drm/v3d: Move perfmon init completely into own unit drm/v3d: Align data types of internal and uapi counts drm/v3d: Add some local variables in queries/extensions drm/v3d: Prefer get_user for scalar types drivers/gpu/drm/v3d/v3d_drv.c | 9 +- drivers/gpu/drm/v3d/v3d_drv.h | 16 +- drivers/gpu/drm/v3d/v3d_perfmon.c | 44 +-- .../gpu/drm/v3d/v3d_performance_counters.h| 16 +- drivers/gpu/drm/v3d/v3d_sched.c | 106 --- drivers/gpu/drm/v3d/v3d_submit.c | 285 ++ 6 files changed, 281 insertions(+), 195 deletions(-) -- 2.44.0
[PATCH 04/12] drm/v3d: Validate passed in drm syncobj handles in the timestamp extension
From: Tvrtko Ursulin If userspace provides an unknown or invalid handle anywhere in the handle array the rest of the driver will not handle that well. Fix it by checking handle was looked up successfuly or otherwise fail the extension by jumping into the existing unwind. Signed-off-by: Tvrtko Ursulin Fixes: 9ba0ff3e083f ("drm/v3d: Create a CPU job extension for the timestamp query job") Cc: Maíra Canal Cc: Iago Toral Quiroga Cc: # v6.8+ --- drivers/gpu/drm/v3d/v3d_submit.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index ca1b1ad0a75c..3313423080e7 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -497,6 +497,10 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!job->timestamp_query.queries[i].syncobj) { + err = -ENOENT; + goto error; + } } job->timestamp_query.count = timestamp.count; @@ -550,6 +554,10 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!job->timestamp_query.queries[i].syncobj) { + err = -ENOENT; + goto error; + } } job->timestamp_query.count = reset.count; @@ -613,6 +621,10 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, } job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!job->timestamp_query.queries[i].syncobj) { + err = -ENOENT; + goto error; + } } job->timestamp_query.count = copy.count; -- 2.44.0
[PATCH 11/12] drm/v3d: Add some local variables in queries/extensions
From: Tvrtko Ursulin Add some local variables to make the code a bit less verbose, with the main benefit being pulling some lines to under 80 columns wide. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/v3d/v3d_submit.c | 79 +--- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 34ecd844f16a..b0c2a8e9cb06 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -452,6 +452,7 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_timestamp_query timestamp; + struct v3d_timestamp_query_info *qinfo = &job->timestamp_query; unsigned int i; int err; @@ -473,10 +474,10 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, job->job_type = V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY; - job->timestamp_query.queries = kvmalloc_array(timestamp.count, - sizeof(struct v3d_timestamp_query), - GFP_KERNEL); - if (!job->timestamp_query.queries) + qinfo->queries = kvmalloc_array(timestamp.count, + sizeof(struct v3d_timestamp_query), + GFP_KERNEL); + if (!qinfo->queries) return -ENOMEM; offsets = u64_to_user_ptr(timestamp.offsets); @@ -490,20 +491,20 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, goto error; } - job->timestamp_query.queries[i].offset = offset; + qinfo->queries[i].offset = offset; if (copy_from_user(&sync, syncs++, sizeof(sync))) { err = -EFAULT; goto error; } - job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); - if (!job->timestamp_query.queries[i].syncobj) { + qinfo->queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!qinfo->queries[i].syncobj) { err = -ENOENT; goto error; } } - job->timestamp_query.count = timestamp.count; + qinfo->count = timestamp.count; return 0; @@ -519,6 +520,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, { u32 __user *syncs; struct drm_v3d_reset_timestamp_query reset; + struct v3d_timestamp_query_info *qinfo = &job->timestamp_query; unsigned int i; int err; @@ -537,10 +539,10 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, job->job_type = V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY; - job->timestamp_query.queries = kvmalloc_array(reset.count, - sizeof(struct v3d_timestamp_query), - GFP_KERNEL); - if (!job->timestamp_query.queries) + qinfo->queries = kvmalloc_array(reset.count, + sizeof(struct v3d_timestamp_query), + GFP_KERNEL); + if (!qinfo->queries) return -ENOMEM; syncs = u64_to_user_ptr(reset.syncs); @@ -548,20 +550,20 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, for (i = 0; i < reset.count; i++) { u32 sync; - job->timestamp_query.queries[i].offset = reset.offset + 8 * i; + qinfo->queries[i].offset = reset.offset + 8 * i; if (copy_from_user(&sync, syncs++, sizeof(sync))) { err = -EFAULT; goto error; } - job->timestamp_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); - if (!job->timestamp_query.queries[i].syncobj) { + qinfo->queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!qinfo->queries[i].syncobj) { err = -ENOENT; goto error; } } - job->timestamp_query.count = reset.count; + qinfo->count = reset.count; return 0; @@ -578,6 +580,7 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_copy_timestamp_query copy; + struct v3d_timestamp_query_info *qinfo = &job->timestamp_query; unsigned int i; int err; @@ -599,10 +602,10 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, job->job_type = V3D_CPU_JOB_TYPE_COPY_TIMESTAMP_QUERY; - job->timestamp_query.queries = kvmalloc_array(copy.count, - sizeof(struct
[PATCH 06/12] drm/v3d: Move part of copying of reset/copy performance extension to a helper
From: Tvrtko Ursulin The loop which looks up the syncobj and copies the kperfmon ids is identical so lets move it to a helper. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/v3d/v3d_submit.c | 148 +-- 1 file changed, 64 insertions(+), 84 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index b51600e236c8..35682433f75b 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -641,13 +641,63 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, return err; } +static int +copy_query_info(struct v3d_performance_query_info *qinfo, + unsigned int count, + unsigned int nperfmons, + u32 __user *syncs, + u64 __user *kperfmon_ids, + struct drm_file *fpriv) +{ + unsigned int i, j; + int err; + + for (i = 0; i < count; i++) { + struct v3d_performance_query *query = &qinfo->queries[i]; + u32 __user *ids_pointer; + u32 sync, id; + u64 ids; + + if (get_user(sync, syncs++)) { + err = -EFAULT; + goto error; + } + + if (get_user(ids, kperfmon_ids++)) { + err = -EFAULT; + goto error; + } + + ids_pointer = u64_to_user_ptr(ids); + + for (j = 0; j < nperfmons; j++) { + if (get_user(id, ids_pointer++)) { + err = -EFAULT; + goto error; + } + + query->kperfmon_ids[j] = id; + } + + query->syncobj = drm_syncobj_find(fpriv, sync); + if (!query->syncobj) { + err = -ENOENT; + goto error; + } + } + + return 0; + +error: + __v3d_performance_query_info_free(qinfo, i); + return err; +} + static int v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, struct drm_v3d_extension __user *ext, struct v3d_cpu_job *job) { - u32 __user *syncs; - u64 __user *kperfmon_ids; struct drm_v3d_reset_performance_query reset; int err; @@ -675,50 +725,17 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, if (!job->performance_query.queries) return -ENOMEM; - syncs = u64_to_user_ptr(reset.syncs); - kperfmon_ids = u64_to_user_ptr(reset.kperfmon_ids); + err = copy_query_info(qinfo, reset.count, reset.nperfmons, + u64_to_user_ptr(reset.syncs), + u64_to_user_ptr(reset.kperfmon_ids), + file_priv); + if (err) + return err; - for (int i = 0; i < reset.count; i++) { - u32 sync; - u64 ids; - u32 __user *ids_pointer; - u32 id; - - if (copy_from_user(&sync, syncs++, sizeof(sync))) { - err = -EFAULT; - goto error; - } - - if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { - err = -EFAULT; - goto error; - } - - ids_pointer = u64_to_user_ptr(ids); - - for (int j = 0; j < reset.nperfmons; j++) { - if (copy_from_user(&id, ids_pointer++, sizeof(id))) { - err = -EFAULT; - goto error; - } - - job->performance_query.queries[i].kperfmon_ids[j] = id; - } - - job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); - if (!job->performance_query.queries[i].syncobj) { - err = -ENOENT; - goto error; - } - } job->performance_query.count = reset.count; job->performance_query.nperfmons = reset.nperfmons; return 0; - -error: - __v3d_performance_query_info_free(qinfo, i); - return err; } static int @@ -726,8 +743,6 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, struct drm_v3d_extension __user *ext, struct v3d_cpu_job *job) { - u32 __user *syncs; - u64 __user *kperfmon_ids; struct drm_v3d_copy_performance_query copy; int err; @@ -758,44 +773,13 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, if (!job->performance_query.queries) return -ENOMEM; - syncs = u64_to_user_ptr(copy.syncs); - kper
[PATCH 07/12] drm/v3d: Size the kperfmon_ids array at runtime
From: Tvrtko Ursulin Instead of statically reserving pessimistic space for the kperfmon_ids array, make the userspace extension code allocate the exactly required amount of space. Apart from saving some memory at runtime, this also removes the need for the V3D_MAX_PERFMONS macro whose removal will benefit further driver cleanup. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/v3d/v3d_drv.h| 6 +- drivers/gpu/drm/v3d/v3d_sched.c | 4 +++- drivers/gpu/drm/v3d/v3d_submit.c | 17 +++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 38c80168da51..00fe5d993175 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -351,13 +351,9 @@ struct v3d_timestamp_query { struct drm_syncobj *syncobj; }; -/* Number of perfmons required to handle all supported performance counters */ -#define V3D_MAX_PERFMONS DIV_ROUND_UP(V3D_MAX_COUNTERS, \ - DRM_V3D_MAX_PERF_COUNTERS) - struct v3d_performance_query { /* Performance monitor IDs for this query */ - u32 kperfmon_ids[V3D_MAX_PERFMONS]; + u32 *kperfmon_ids; /* Syncobj that indicates the query availability */ struct drm_syncobj *syncobj; diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index 173801aa54ee..fc8730264386 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -94,8 +94,10 @@ __v3d_performance_query_info_free(struct v3d_performance_query_info *qinfo, if (qinfo->queries) { unsigned int i; - for (i = 0; i < count; i++) + for (i = 0; i < count; i++) { drm_syncobj_put(qinfo->queries[i].syncobj); + kvfree(qinfo->queries[i].kperfmon_ids); + } kvfree(qinfo->queries); } diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 35682433f75b..8dae3ab5f936 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -668,10 +668,20 @@ copy_query_info(struct v3d_performance_query_info *qinfo, goto error; } + query->kperfmon_ids = + kvmalloc_array(nperfmons, + sizeof(struct v3d_performance_query *), + GFP_KERNEL); + if (!query->kperfmon_ids) { + err = -ENOMEM; + goto error; + } + ids_pointer = u64_to_user_ptr(ids); for (j = 0; j < nperfmons; j++) { if (get_user(id, ids_pointer++)) { + kvfree(query->kperfmon_ids); err = -EFAULT; goto error; } @@ -681,6 +691,7 @@ copy_query_info(struct v3d_performance_query_info *qinfo, query->syncobj = drm_syncobj_find(fpriv, sync); if (!query->syncobj) { + kvfree(query->kperfmon_ids); err = -ENOENT; goto error; } @@ -714,9 +725,6 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, if (copy_from_user(&reset, ext, sizeof(reset))) return -EFAULT; - if (reset.nperfmons > V3D_MAX_PERFMONS) - return -EINVAL; - job->job_type = V3D_CPU_JOB_TYPE_RESET_PERFORMANCE_QUERY; job->performance_query.queries = kvmalloc_array(reset.count, @@ -762,9 +770,6 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, if (copy.pad) return -EINVAL; - if (copy.nperfmons > V3D_MAX_PERFMONS) - return -EINVAL; - job->job_type = V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY; job->performance_query.queries = kvmalloc_array(copy.count, -- 2.44.0
[PATCH 08/12] drm/v3d: Do not use intermediate storage when copying performance query results
From: Tvrtko Ursulin Removing the intermediate buffer removes the last use of the V3D_MAX_COUNTERS define, which will enable further driver cleanup. While at it pull the 32 vs 64 bit copying decision outside the loop in order to reduce the number of conditional instructions. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/v3d/v3d_sched.c | 60 - 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index fc8730264386..77f795e38fad 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -421,18 +421,23 @@ v3d_reset_timestamp_queries(struct v3d_cpu_job *job) v3d_put_bo_vaddr(bo); } +static void write_to_buffer_32(u32 *dst, unsigned int idx, u32 value) +{ + dst[idx] = value; +} + +static void write_to_buffer_64(u64 *dst, unsigned int idx, u64 value) +{ + dst[idx] = value; +} + static void -write_to_buffer(void *dst, u32 idx, bool do_64bit, u64 value) +write_to_buffer(void *dst, unsigned int idx, bool do_64bit, u64 value) { - if (do_64bit) { - u64 *dst64 = (u64 *)dst; - - dst64[idx] = value; - } else { - u32 *dst32 = (u32 *)dst; - - dst32[idx] = (u32)value; - } + if (do_64bit) + write_to_buffer_64(dst, idx, value); + else + write_to_buffer_32(dst, idx, value); } static void @@ -505,18 +510,23 @@ v3d_reset_performance_queries(struct v3d_cpu_job *job) } static void -v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data, u32 query) +v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data, + unsigned int query) { - struct v3d_performance_query_info *performance_query = &job->performance_query; - struct v3d_copy_query_results_info *copy = &job->copy; + struct v3d_performance_query_info *performance_query = + &job->performance_query; struct v3d_file_priv *v3d_priv = job->base.file->driver_priv; struct v3d_dev *v3d = job->base.v3d; - struct v3d_perfmon *perfmon; - u64 counter_values[V3D_MAX_COUNTERS]; + unsigned int i, j, offset; - for (int i = 0; i < performance_query->nperfmons; i++) { - perfmon = v3d_perfmon_find(v3d_priv, - performance_query->queries[query].kperfmon_ids[i]); + for (i = 0, offset = 0; +i < performance_query->nperfmons; +i++, offset += DRM_V3D_MAX_PERF_COUNTERS) { + struct v3d_performance_query *q = + &performance_query->queries[query]; + struct v3d_perfmon *perfmon; + + perfmon = v3d_perfmon_find(v3d_priv, q->kperfmon_ids[i]); if (!perfmon) { DRM_DEBUG("Failed to find perfmon."); continue; @@ -524,14 +534,18 @@ v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data, u32 quer v3d_perfmon_stop(v3d, perfmon, true); - memcpy(&counter_values[i * DRM_V3D_MAX_PERF_COUNTERS], perfmon->values, - perfmon->ncounters * sizeof(u64)); + if (job->copy.do_64bit) { + for (j = 0; j < perfmon->ncounters; j++) + write_to_buffer_64(data, offset + j, + perfmon->values[j]); + } else { + for (j = 0; j < perfmon->ncounters; j++) + write_to_buffer_32(data, offset + j, + perfmon->values[j]); + } v3d_perfmon_put(perfmon); } - - for (int i = 0; i < performance_query->ncounters; i++) - write_to_buffer(data, i, copy->do_64bit, counter_values[i]); } static void -- 2.44.0
[PATCH 10/12] drm/v3d: Align data types of internal and uapi counts
From: Tvrtko Ursulin In the timestamp and performance extensions userspace type for counts is u32 so lets use unsigned in the kernel too. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/v3d/v3d_submit.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 8dae3ab5f936..34ecd844f16a 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -452,6 +452,7 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_timestamp_query timestamp; + unsigned int i; int err; if (!job) { @@ -481,7 +482,7 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, offsets = u64_to_user_ptr(timestamp.offsets); syncs = u64_to_user_ptr(timestamp.syncs); - for (int i = 0; i < timestamp.count; i++) { + for (i = 0; i < timestamp.count; i++) { u32 offset, sync; if (copy_from_user(&offset, offsets++, sizeof(offset))) { @@ -518,6 +519,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, { u32 __user *syncs; struct drm_v3d_reset_timestamp_query reset; + unsigned int i; int err; if (!job) { @@ -543,7 +545,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, syncs = u64_to_user_ptr(reset.syncs); - for (int i = 0; i < reset.count; i++) { + for (i = 0; i < reset.count; i++) { u32 sync; job->timestamp_query.queries[i].offset = reset.offset + 8 * i; @@ -576,7 +578,8 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, { u32 __user *offsets, *syncs; struct drm_v3d_copy_timestamp_query copy; - int i, err; + unsigned int i; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); -- 2.44.0
[PATCH 12/12] drm/v3d: Prefer get_user for scalar types
From: Tvrtko Ursulin It makes it just a tiny bit more obvious what is going on. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/v3d/v3d_submit.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index b0c2a8e9cb06..9273b0aadb79 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -486,14 +486,14 @@ v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv, for (i = 0; i < timestamp.count; i++) { u32 offset, sync; - if (copy_from_user(&offset, offsets++, sizeof(offset))) { + if (get_user(offset, offsets++)) { err = -EFAULT; goto error; } qinfo->queries[i].offset = offset; - if (copy_from_user(&sync, syncs++, sizeof(sync))) { + if (get_user(sync, syncs++)) { err = -EFAULT; goto error; } @@ -552,7 +552,7 @@ v3d_get_cpu_reset_timestamp_params(struct drm_file *file_priv, qinfo->queries[i].offset = reset.offset + 8 * i; - if (copy_from_user(&sync, syncs++, sizeof(sync))) { + if (get_user(sync, syncs++)) { err = -EFAULT; goto error; } @@ -614,14 +614,14 @@ v3d_get_cpu_copy_query_results_params(struct drm_file *file_priv, for (i = 0; i < copy.count; i++) { u32 offset, sync; - if (copy_from_user(&offset, offsets++, sizeof(offset))) { + if (get_user(offset, offsets++)) { err = -EFAULT; goto error; } qinfo->queries[i].offset = offset; - if (copy_from_user(&sync, syncs++, sizeof(sync))) { + if (get_user(sync, syncs++)) { err = -EFAULT; goto error; } -- 2.44.0
[PATCH 05/12] drm/v3d: Validate passed in drm syncobj handles in the performance extension
From: Tvrtko Ursulin If userspace provides an unknown or invalid handle anywhere in the handle array the rest of the driver will not handle that well. Fix it by checking handle was looked up successfuly or otherwise fail the extension by jumping into the existing unwind. Signed-off-by: Tvrtko Ursulin Fixes: bae7cb5d6800 ("drm/v3d: Create a CPU job extension for the reset performance query job" Cc: Maíra Canal Cc: Iago Toral Quiroga Cc: # v6.8+ --- drivers/gpu/drm/v3d/v3d_submit.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 3313423080e7..b51600e236c8 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -706,6 +706,10 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, } job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!job->performance_query.queries[i].syncobj) { + err = -ENOENT; + goto error; + } } job->performance_query.count = reset.count; job->performance_query.nperfmons = reset.nperfmons; @@ -787,6 +791,10 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, } job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); + if (!job->performance_query.queries[i].syncobj) { + err = -ENOENT; + goto error; + } } job->performance_query.count = copy.count; job->performance_query.nperfmons = copy.nperfmons; -- 2.44.0
[PATCH 09/12] drm/v3d: Move perfmon init completely into own unit
From: Tvrtko Ursulin Now that the build time dependencies on various array sizes have been removed, we can move the perfmon init completely into its own compilation unit and remove the hardcoded defines. This improves on the temporary fix quickly delivered in 792d16b5375d ("drm/v3d: Move perfmon init completely into own unit"). Signed-off-by: Tvrtko Ursulin References: 792d16b5375d ("drm/v3d: Move perfmon init completely into own unit") --- drivers/gpu/drm/v3d/v3d_drv.c | 9 +--- drivers/gpu/drm/v3d/v3d_drv.h | 6 +-- drivers/gpu/drm/v3d/v3d_perfmon.c | 44 +++ .../gpu/drm/v3d/v3d_performance_counters.h| 16 --- 4 files changed, 40 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index a47f00b443d3..491c638a4d74 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -95,7 +95,7 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data, args->value = 1; return 0; case DRM_V3D_PARAM_MAX_PERF_COUNTERS: - args->value = v3d->max_counters; + args->value = v3d->perfmon_info.max_counters; return 0; default: DRM_DEBUG("Unknown parameter %d\n", args->param); @@ -298,12 +298,7 @@ static int v3d_platform_drm_probe(struct platform_device *pdev) v3d->cores = V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_NCORES); WARN_ON(v3d->cores > 1); /* multicore not yet implemented */ - if (v3d->ver >= 71) - v3d->max_counters = V3D_V71_NUM_PERFCOUNTERS; - else if (v3d->ver >= 42) - v3d->max_counters = V3D_V42_NUM_PERFCOUNTERS; - else - v3d->max_counters = 0; + v3d_perfmon_init(v3d); v3d->reset = devm_reset_control_get_exclusive(dev, NULL); if (IS_ERR(v3d->reset)) { diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 00fe5d993175..6d2d34cd135c 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -104,10 +104,7 @@ struct v3d_dev { int ver; bool single_irq_line; - /* Different revisions of V3D have different total number of performance -* counters -*/ - unsigned int max_counters; + struct v3d_perfmon_info perfmon_info; void __iomem *hub_regs; void __iomem *core_regs[3]; @@ -568,6 +565,7 @@ int v3d_sched_init(struct v3d_dev *v3d); void v3d_sched_fini(struct v3d_dev *v3d); /* v3d_perfmon.c */ +void v3d_perfmon_init(struct v3d_dev *v3d); void v3d_perfmon_get(struct v3d_perfmon *perfmon); void v3d_perfmon_put(struct v3d_perfmon *perfmon); void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon); diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c b/drivers/gpu/drm/v3d/v3d_perfmon.c index b7d0b02e1a95..cd7f1eedf17f 100644 --- a/drivers/gpu/drm/v3d/v3d_perfmon.c +++ b/drivers/gpu/drm/v3d/v3d_perfmon.c @@ -195,6 +195,23 @@ static const struct v3d_perf_counter_desc v3d_v71_performance_counters[] = { {"QPU", "QPU-stalls-other", "[QPU] Stalled qcycles waiting for any other reason (vary/W/Z)"}, }; +void v3d_perfmon_init(struct v3d_dev *v3d) +{ + const struct v3d_perf_counter_desc *counters = NULL; + unsigned int max = 0; + + if (v3d->ver >= 71) { + counters = v3d_v71_performance_counters; + max = ARRAY_SIZE(v3d_v71_performance_counters); + } else if (v3d->ver >= 42) { + counters = v3d_v42_performance_counters; + max = ARRAY_SIZE(v3d_v42_performance_counters); + } + + v3d->perfmon_info.max_counters = max; + v3d->perfmon_info.counters = counters; +} + void v3d_perfmon_get(struct v3d_perfmon *perfmon) { if (perfmon) @@ -321,7 +338,7 @@ int v3d_perfmon_create_ioctl(struct drm_device *dev, void *data, /* Make sure all counters are valid. */ for (i = 0; i < req->ncounters; i++) { - if (req->counters[i] >= v3d->max_counters) + if (req->counters[i] >= v3d->perfmon_info.max_counters) return -EINVAL; } @@ -416,26 +433,15 @@ int v3d_perfmon_get_counter_ioctl(struct drm_device *dev, void *data, return -EINVAL; } - /* Make sure that the counter ID is valid */ - if (req->counter >= v3d->max_counters) - return -EINVAL; - - BUILD_BUG_ON(ARRAY_SIZE(v3d_v42_performance_counters) != -V3D_V42_NUM_PERFCOUNTERS); - BUILD_BUG_ON(ARRAY_SIZE(v3d_v71_performance_counters) != -V3D_V71_NUM_PERFCOUNTERS); - BUILD_BUG_ON(V3D_MAX_COUNTERS < V3D_V42_NUM_PERFCOUNTERS); - BUILD_BUG_ON(V3D_MAX_COUNTERS < V3D_V71_NUM_PERFCOUNTERS); - BUILD_BUG_ON((V3D_MAX_COUNTERS != V3D_V42_NUM_PERFCOUNTERS) && -(V3D_MAX_COUNTERS != V3D_V7
Re: [PATCH 00/12] v3d: Perfmon cleanup
Hi Iago, On 10/07/2024 07:06, Iago Toral wrote: El mar, 09-07-2024 a las 17:34 +0100, Tvrtko Ursulin escribió: From: Tvrtko Ursulin When we had to quickly deal with a tree build issue via merging 792d16b5375d ("drm/v3d: Move perfmon init completely into own unit"), we promised to follow up with a nicer solution. As in the process of eliminating the hardcoded defines we have discovered a few issues in handling of corner cases and userspace input validation, the fix has turned into a larger series, but hopefully the end result is a justifiable cleanup. Thanks for going the extra mile with this :) Patches 1 and 5-8 are: Reviewed-by: Iago Toral Quiroga Thank you! Unfortunately I had to re-order the patches in the series so fixes come first, and as that caused a lot of churn in each patch I did not apply your r-b's when re-sending. Hmmm actually I should have for the first patch, that one is unchanged. I will fix that one. Regards, Tvrtko Tvrtko Ursulin (12): drm/v3d: Prevent out of bounds access in performance query extensions drm/v3d: Prefer get_user for scalar types drm/v3d: Add some local variables in queries/extensions drm/v3d: Align data types of internal and uapi counts drm/v3d: Fix potential memory leak in the timestamp extension drm/v3d: Fix potential memory leak in the performance extension drm/v3d: Validate passed in drm syncobj handles in the timestamp extension drm/v3d: Validate passed in drm syncobj handles in the performance extension drm/v3d: Move part of copying of reset/copy performance extension to a helper drm/v3d: Size the kperfmon_ids array at runtime drm/v3d: Do not use intermediate storage when copying performance query results drm/v3d: Move perfmon init completely into own unit drivers/gpu/drm/v3d/v3d_drv.c | 9 +- drivers/gpu/drm/v3d/v3d_drv.h | 16 +- drivers/gpu/drm/v3d/v3d_perfmon.c | 44 +-- .../gpu/drm/v3d/v3d_performance_counters.h | 16 +- drivers/gpu/drm/v3d/v3d_sched.c | 106 --- drivers/gpu/drm/v3d/v3d_submit.c | 285 ++-- -- 6 files changed, 281 insertions(+), 195 deletions(-)
Re: [PATCH 01/12] drm/v3d: Prevent out of bounds access in performance query extensions
On 10/07/2024 14:41, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Check that the number of perfmons userspace is passing in the copy and reset extensions is not greater than the internal kernel storage where the ids will be copied into. Signed-off-by: Tvrtko Ursulin Fixes: bae7cb5d6800 ("drm/v3d: Create a CPU job extension for the reset performance query job" Cc: Maíra Canal Cc: Iago Toral Quiroga Cc: # v6.8+ On this one I forgot to carry over from v1: Reviewed-by: Iago Toral Quiroga Regards, Tvrtko --- drivers/gpu/drm/v3d/v3d_submit.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 88f63d526b22..263fefc1d04f 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -637,6 +637,9 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, if (copy_from_user(&reset, ext, sizeof(reset))) return -EFAULT; + if (reset.nperfmons > V3D_MAX_PERFMONS) + return -EINVAL; + job->job_type = V3D_CPU_JOB_TYPE_RESET_PERFORMANCE_QUERY; job->performance_query.queries = kvmalloc_array(reset.count, @@ -708,6 +711,9 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, if (copy.pad) return -EINVAL; + if (copy.nperfmons > V3D_MAX_PERFMONS) + return -EINVAL; + job->job_type = V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY; job->performance_query.queries = kvmalloc_array(copy.count,
Re: [PATCH v5 4/4] drm/doc: document some tracepoints as uAPI
Hi Maíra, Le 03/07/2024 à 17:41, Maíra Canal a écrit : Hi Pierre, On 6/14/24 05:16, Pierre-Eric Pelloux-Prayer wrote: This commit adds a document section in drm-uapi.rst about tracepoints, and mark the events gpu_scheduler_trace.h as stable uAPI. The goal is to explicitly state that tools can rely on the fields, formats and semantics of these events. Signed-off-by: Pierre-Eric Pelloux-Prayer --- Documentation/gpu/drm-uapi.rst | 19 .../gpu/drm/scheduler/gpu_scheduler_trace.h | 22 +++ 2 files changed, 41 insertions(+) diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst index 370d820be248..78496793a8f0 100644 --- a/Documentation/gpu/drm-uapi.rst +++ b/Documentation/gpu/drm-uapi.rst @@ -570,3 +570,22 @@ dma-buf interoperability Please see Documentation/userspace-api/dma-buf-alloc-exchange.rst for information on how dma-buf is integrated and exposed within DRM. + + +Trace events + + +See Documentation/trace/tracepoints.rst for the tracepoints documentation. I would write it: "See Documentation/trace/tracepoints.rst for information about using Linux Kernel Tracepoints." +In the drm subsystem, some events are considered stable uAPI to avoid Super small nit: s/drm/DRM +breaking tools (eg: gpuvis, umr) relying on them. Stable means that fields Super small nit: 1. s/eg:/e.g.: 2. s/gpuvis/GPUVis (maybe a URL to it?) 3. Maybe a URL to umr? +cannot be removed, nor their formatting updated. Adding new fields is +possible, under the normal uAPI requirements. + +Stable uAPI events +-- + +From ``drivers/gpu/drm/scheduler/gpu_scheduler_trace.h`` + Super small nit: from the rest of the file, I see that a title was never needed. Do we need it here? + +.. kernel-doc:: drivers/gpu/drm/scheduler/gpu_scheduler_trace.h + :doc: uAPI trace events \ No newline at end of file diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h index 0abcad26839c..63113803cdd5 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h +++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h @@ -33,6 +33,28 @@ #define TRACE_SYSTEM gpu_scheduler #define TRACE_INCLUDE_FILE gpu_scheduler_trace + +/** + * DOC: uAPI trace events + * + * ``drm_sched_job``, ``drm_run_job``, ``drm_sched_process_job``, + * and ``drm_sched_job_wait_dep`` are considered stable uAPI. Super small nit again, but I believe we should format function names with ``foo()``, if I understood kerneldoc documentation correctly. Apart from all those nits, I completely agree with Lucas, it is great to see this improvement. Acked-by: Maíra Canal Thanks a lot for the feedback, I'll integrate it in v6. Regards, Pierre-Eric Best Regards, - Maíra + * + * Common trace events attributes: + * + * * ``id`` - this is &drm_sched_job->id. It uniquely idenfies a job + * inside a &struct drm_gpu_scheduler. + * + * * ``dev`` - the dev_name() of the device running the job. + * + * * ``ring`` - the hardware ring running the job. Together with ``dev`` it + * uniquely identifies where the job is going to be executed. + * + * * ``fence`` - the &dma_fence.context and the &dma_fence.seqno of + * &drm_sched_fence.finished + * + */ + #ifndef __TRACE_EVENT_GPU_SCHEDULER_PRINT_FN #define __TRACE_EVENT_GPU_SCHEDULER_PRINT_FN /* Similar to trace_print_array_seq but for fences. */
Re: [PATCH 1/2] dt-bindings: display: panel: document BOE TV101WUM-LL2 DSI Display Panel
On Tue, Jul 09, 2024 at 03:05:44PM +0200, Neil Armstrong wrote: > Document the 1200x1920 BOE TV101WUM-LL2 DSI Display Panel found > in the Lenovo Smart Tab M10 tablet. The controller is unknown. > > Signed-off-by: Neil Armstrong Reviewed-by: Conor Dooley signature.asc Description: PGP signature
Re: [PATCH v3 3/5] dt-bindings: display: st7701: Add Anbernic RG28XX panel
On Sat, Jul 06, 2024 at 07:23:34PM +0900, Hironori KIKUCHI wrote: > The RG28XX panel is a display panel of the Anbernic RG28XX, a handheld > gaming device from Anbernic. It is 2.8 inches in size (diagonally) with > a resolution of 480x640. > > This panel is driven by a variant of the ST7701 driver IC internally, > confirmed by dumping and analyzing its BSP initialization sequence > by using a logic analyzer. It is very similar to the existing > densitron,dmt028vghmcmi-1a panel, but differs in some unknown > register values, so add a new entry for the panel to distinguish them. > > Additionally, the panel only has an SPI instead of MIPI DSI. > So add and modify for SPI as well. > > Signed-off-by: Hironori KIKUCHI With a mention in the commit message about why we are adding a property and then immediately forbidding its use: Reviewed-by: Conor Dooley Thanks, Conor. signature.asc Description: PGP signature
[PATCH v3 0/4] drm/panic: Add a QR code panic screen
This series adds a new panic screen, with the kmsg data embedded in a QR code. The main advantage of QR code, is that you can copy/paste the debug data to a bug report. The QR code encoder is written in rust, and is very specific to drm panic. The reason is that it is called in a panic handler, and thus can't allocate memory, or use locking. The rust code uses a few rust core API, and provides only two C entry points. There is no particular reason to do it in rust, I just wanted to learn rust, and see if it can work in the kernel. If you want to see what it looks like, I've put a few screenshots here: https://github.com/kdj0c/panic_report/issues/1 v2: * Rewrite the rust comments with Markdown (Alice Ryhl) * Mark drm_panic_qr_generate() as unsafe (Alice Ryhl) * Use CStr directly, and remove the call to as_str_unchecked() (Alice Ryhl) * Add a check for data_len <= data_size (Greg KH) v3: * Fix all rust comments (typo, punctuation) (Miguel Ojeda) * Change the wording of safety comments (Alice Ryhl) * Add a link to the javascript decoder in the Kconfig (Greg KH) * Fix data_size and tmp_size check in drm_panic_qr_generate() Jocelyn Falempe (4): drm/panic: Add integer scaling to blit() drm/rect: Add drm_rect_overlap() drm/panic: Simplify logo handling drm/panic: Add a QR code panic screen drivers/gpu/drm/Kconfig | 31 + drivers/gpu/drm/Makefile|1 + drivers/gpu/drm/drm_drv.c |3 + drivers/gpu/drm/drm_panic.c | 338 +-- drivers/gpu/drm/drm_panic_qr.rs | 1005 +++ include/drm/drm_panic.h |4 + include/drm/drm_rect.h | 15 + 7 files changed, 1358 insertions(+), 39 deletions(-) create mode 100644 drivers/gpu/drm/drm_panic_qr.rs base-commit: 5a716b06b329bd2108c95a4f04c71bbe491729f2 -- 2.45.2
[PATCH v3 1/4] drm/panic: Add integer scaling to blit()
Add a parameter to the blit function, to upscale the image. This is necessary to draw a QR code, otherwise, the pixels are usually too small to be readable by most QR code reader. It can also be used later for drawing fonts on high DPI display. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 33 +++-- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 948aed00595e..fb8c823b7c00 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -249,20 +249,20 @@ static bool drm_panic_is_pixel_fg(const u8 *sbuf8, unsigned int spitch, int x, i static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u16 fg16) +unsigned int scale, u16 fg16) { unsigned int y, x; for (y = 0; y < height; y++) for (x = 0; x < width; x++) - if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) + if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, fg16); } static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32) +unsigned int scale, u32 fg32) { unsigned int y, x; @@ -270,7 +270,7 @@ static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, for (x = 0; x < width; x++) { u32 off = y * dpitch + x * 3; - if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) { + if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) { /* write blue-green-red to output in little endianness */ iosys_map_wr(dmap, off, u8, (fg32 & 0x00FF) >> 0); iosys_map_wr(dmap, off + 1, u8, (fg32 & 0xFF00) >> 8); @@ -283,24 +283,25 @@ static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32) +unsigned int scale, u32 fg32) { unsigned int y, x; for (y = 0; y < height; y++) for (x = 0; x < width; x++) - if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) + if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, fg32); } static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect *clip, -const u8 *sbuf8, unsigned int spitch, u32 fg_color) +const u8 *sbuf8, unsigned int spitch, unsigned int scale, +u32 fg_color) { unsigned int y, x; for (y = 0; y < drm_rect_height(clip); y++) for (x = 0; x < drm_rect_width(clip); x++) - if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) + if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, fg_color); } @@ -310,18 +311,22 @@ static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect * @clip: destination rectangle * @sbuf8: source buffer, in monochrome format, 8 pixels per byte. * @spitch: source pitch in bytes + * @scale: integer scale, source buffer is scale time smaller than destination + * rectangle * @fg_color: foreground color, in destination format * * This can be used to draw a font character, which is a monochrome image, to a * framebuffer in other supported format. */ static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip, - const u8 *sbuf8, unsigned int spitch, u32 fg_color) + const u8 *sbuf8, unsigned int spitch, + unsigned int scale, u32 fg_color) + { struct iosys_map map; if (sb->set_pixel) - return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, fg_color); + return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, scale, fg_color); map = sb->map[0]; iosys_map_incr(&map, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0])
[PATCH v3 2/4] drm/rect: Add drm_rect_overlap()
Check if two rectangles overlap. It's a bit similar to drm_rect_intersect() but this won't modify the rectangle. Simplifies a bit drm_panic. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 3 +-- include/drm/drm_rect.h | 15 +++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index fb8c823b7c00..9e06609e1799 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -522,8 +522,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) /* Fill with the background color, and draw text on top */ drm_panic_fill(sb, &r_screen, bg_color); - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && - logo_width <= sb->width && logo_height <= sb->height) { + if (!drm_rect_overlap(&r_logo, &r_msg)) { if (logo_mono) drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), fg_color); diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h index 73fcb899a01d..7bafde747d56 100644 --- a/include/drm/drm_rect.h +++ b/include/drm/drm_rect.h @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, drm_rect_height(src) >> 16); } +/** + * drm_rect_overlap - Check if two rectangles overlap + * @r1: first rectangle + * @r2: second rectangle + * + * RETURNS: + * %true if the rectangles overlap, %false otherwise. + */ +static inline bool drm_rect_overlap(const struct drm_rect *r1, + const struct drm_rect *r2) +{ + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && + r1->y2 > r2->y1 && r2->y2 > r1->y1); +} + bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, const struct drm_rect *clip); -- 2.45.2
[PATCH v3 4/4] drm/panic: Add a QR code panic screen
This patch adds a new panic screen, with a QR code and the kmsg data embedded. If DRM_PANIC_SCREEN_QR_CODE_URL is set, then the kmsg data will be compressed with zlib and encoded as a numerical segment, and appended to the URL as a URL parameter. This allows to save space, and put about ~7500 bytes of kmsg data, in a V40 QR code. Linux distributions can customize the URL, and put a web frontend to directly open a bug report with the kmsg data. Otherwise the kmsg data will be encoded as a binary segment (ie raw ascii) and only a maximum of 2953 bytes of kmsg data will be available in the QR code. You can also limit the QR code size with DRM_PANIC_SCREEN_QR_VERSION. v2: * Rewrite the rust comments with Markdown (Alice Ryhl) * Mark drm_panic_qr_generate() as unsafe (Alice Ryhl) * Use CStr directly, and remove the call to as_str_unchecked() (Alice Ryhl) * Add a check for data_len <= data_size (Greg KH) v3: * Fix all rust comments (typo, punctuation) (Miguel Ojeda) * Change the wording of safety comments (Alice Ryhl) * Add a link to the javascript decoder in the Kconfig (Greg KH) * Fix data_size and tmp_size check in drm_panic_qr_generate() Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 31 + drivers/gpu/drm/Makefile|1 + drivers/gpu/drm/drm_drv.c |3 + drivers/gpu/drm/drm_panic.c | 247 drivers/gpu/drm/drm_panic_qr.rs | 1005 +++ include/drm/drm_panic.h |4 + 6 files changed, 1291 insertions(+) create mode 100644 drivers/gpu/drm/drm_panic_qr.rs diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 6dd0016fc9cd..50ac967b56be 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -149,6 +149,37 @@ config DRM_PANIC_SCREEN or by writing to /sys/module/drm/parameters/panic_screen sysfs entry Default is "user" +config DRM_PANIC_SCREEN_QR_CODE + bool "Add a panic screen with a QR code" + depends on DRM_PANIC && RUST + help + This option adds a QR code generator, and a panic screen with a QR + code. The QR code will contain the last lines of kmsg and other debug + information. This should be easier for the user to report a kernel + panic, with all debug information available. + To use this panic screen, also set DRM_PANIC_SCREEN to "qr_code" + +config DRM_PANIC_SCREEN_QR_CODE_URL + string "Base URL of the QR code in the panic screen" + depends on DRM_PANIC_SCREEN_QR_CODE + help + This option sets the base URL to report the kernel panic. If it's set + the QR code will contain the URL and the kmsg compressed with zlib as + a URL parameter. If it's empty, the QR code will contain the kmsg as + uncompressed text only. + There is a demo code in javascript, to decode and uncompress the kmsg + data from the URL parameter at https://github.com/kdj0c/panic_report + +config DRM_PANIC_SCREEN_QR_VERSION + int "Maximum version (size) of the QR code." + depends on DRM_PANIC_SCREEN_QR_CODE + default 40 + help + This option limits the version (or size) of the QR code. QR code + version ranges from Version 1 (21x21) to Version 40 (177x177). + Smaller QR code are easier to read, but will contain less debugging + data. Default is 40. + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 68cc9258ffc4..c62339b89d46 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -89,6 +89,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \ drm_privacy_screen_x86.o drm-$(CONFIG_DRM_ACCEL) += ../../accel/drm_accel.o drm-$(CONFIG_DRM_PANIC) += drm_panic.o +drm-$(CONFIG_DRM_PANIC_SCREEN_QR_CODE) += drm_panic_qr.o obj-$(CONFIG_DRM) += drm.o obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 93543071a500..27007b53a8c8 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -1067,6 +1067,7 @@ static const struct file_operations drm_stub_fops = { static void drm_core_exit(void) { drm_privacy_screen_lookup_exit(); + drm_panic_exit(); accel_core_exit(); unregister_chrdev(DRM_MAJOR, "drm"); debugfs_remove(drm_debugfs_root); @@ -1099,6 +1100,8 @@ static int __init drm_core_init(void) if (ret < 0) goto error; + drm_panic_init(); + drm_privacy_screen_lookup_init(); drm_core_init_complete = true; diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 450585374ca9..8c7895194a04 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -18,6 +18,8 @@ #include #include #include +
Re: [PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
On 2024-07-10, Daniel Vetter wrote: > console_lock is the outermost subsystem lock for a lot of subsystems, > which means get/put_user must nest within. Which means it cannot be > acquired somewhere deeply nested in other locks, and most definitely > not while holding fs locks potentially needed to resolve faults. > > console_trylock is the best we can do here. But John pointed out on a > previous version that this is futile: > > "Using the console lock here at all is wrong. The console lock does not > prevent other CPUs from calling printk() and inserting lines in between. > > "There is no way to guarantee a contiguous ringbuffer block using > multiple printk() calls. > > "The console_lock usage should be removed." > > https://lore.kernel.org/lkml/87frsh33xp@jogness.linutronix.de/ > > Do that. > > Reported-by: syzbot+6cebc1af246fe020a...@syzkaller.appspotmail.com > References: > https://lore.kernel.org/dri-devel/26c1ff061cd0d...@google.com/ > Signed-off-by: Daniel Vetter > Fixes: a8f354284304 ("bcachefs: bch2_print_string_as_lines()") Reviewed-by: John Ogness
[PATCH v3 3/4] drm/panic: Simplify logo handling
Move logo rectangle initialisation, and logo drawing in separate functions, so they can be re-used by different panic screens. It prepares the introduction of the QR code panic screen. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 57 + 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 9e06609e1799..450585374ca9 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -80,6 +80,7 @@ static struct drm_panic_line panic_msg[] = { PANIC_LINE(""), PANIC_LINE("Please reboot your computer."), }; +static const int panic_msg_lines = ARRAY_SIZE(panic_msg); static const struct drm_panic_line logo_ascii[] = { PANIC_LINE(" .--._"), @@ -90,6 +91,7 @@ static const struct drm_panic_line logo_ascii[] = { PANIC_LINE(" /'\\_ _/`\\(_)"), PANIC_LINE(" \\___)=(___/"), }; +static const int logo_ascii_lines = ARRAY_SIZE(logo_ascii); #if defined(CONFIG_LOGO) && !defined(MODULE) static const struct linux_logo *logo_mono; @@ -488,33 +490,45 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, } } +static void drm_panic_logo_rect(struct drm_rect *rect, const struct font_desc *font) +{ + if (logo_mono) + drm_rect_init(rect, 0, 0, logo_mono->width, logo_mono->height); + else { + int logo_width = get_max_line_len(logo_ascii, logo_ascii_lines) * font->width; + + drm_rect_init(rect, 0, 0, logo_width, logo_ascii_lines * font->height); + } +} + +static void drm_panic_logo_draw(struct drm_scanout_buffer *sb, struct drm_rect *rect, + const struct font_desc *font, u32 fg_color) +{ + if (logo_mono) + drm_panic_blit(sb, rect, logo_mono->data, + DIV_ROUND_UP(drm_rect_width(rect), 8), 1, fg_color); + else + draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, rect, + fg_color); +} + static void draw_panic_static_user(struct drm_scanout_buffer *sb) { - size_t msg_lines = ARRAY_SIZE(panic_msg); - size_t logo_ascii_lines = ARRAY_SIZE(logo_ascii); u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); struct drm_rect r_screen, r_logo, r_msg; - unsigned int logo_width, logo_height; + unsigned int panic_msg_width; if (!font) return; r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); - - if (logo_mono) { - logo_width = logo_mono->width; - logo_height = logo_mono->height; - } else { - logo_width = get_max_line_len(logo_ascii, logo_ascii_lines) * font->width; - logo_height = logo_ascii_lines * font->height; - } - - r_logo = DRM_RECT_INIT(0, 0, logo_width, logo_height); + drm_panic_logo_rect(&r_logo, font); + panic_msg_width = get_max_line_len(panic_msg, panic_msg_lines) * font->width; r_msg = DRM_RECT_INIT(0, 0, - min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width), - min(msg_lines * font->height, sb->height)); + min(panic_msg_width, sb->width), + min(panic_msg_lines * font->height, sb->height)); /* Center the panic message */ drm_rect_translate(&r_msg, (sb->width - r_msg.x2) / 2, (sb->height - r_msg.y2) / 2); @@ -522,15 +536,10 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) /* Fill with the background color, and draw text on top */ drm_panic_fill(sb, &r_screen, bg_color); - if (!drm_rect_overlap(&r_logo, &r_msg)) { - if (logo_mono) - drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), - fg_color); - else - draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo, - fg_color); - } - draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color); + if (!drm_rect_overlap(&r_logo, &r_msg)) + drm_panic_logo_draw(sb, &r_logo, font, fg_color); + + draw_txt_rectangle(sb, font, panic_msg, panic_msg_lines, true, &r_msg, fg_color); } /* -- 2.45.2
Re: [PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
On 2024-07-10, Daniel Vetter wrote: > console_lock is the outermost subsystem lock for a lot of subsystems, > which means get/put_user must nest within. Which means it cannot be > acquired somewhere deeply nested in other locks, and most definitely > not while holding fs locks potentially needed to resolve faults. > > console_trylock is the best we can do here. But John pointed out on a > previous version that this is futile: > > "Using the console lock here at all is wrong. The console lock does not > prevent other CPUs from calling printk() and inserting lines in between. > > "There is no way to guarantee a contiguous ringbuffer block using > multiple printk() calls. > > "The console_lock usage should be removed." > > https://lore.kernel.org/lkml/87frsh33xp@jogness.linutronix.de/ > > Do that. Note that there is more of this incorrect usage of console lock in: fs/bcachefs/debug.c:bch2_btree_verify_replica() fs/bcachefs/bset.c:bch2_dump_btree_node() from commit 1c6fdbd8f246("bcachefs: Initial commit") ... and its parent bcache: drivers/md/bcache/debug.c:bch_btree_verify() drivers/md/bcache/bset.c:bch_dump_bucket() from commit cafe56359144("bcache: A block layer cache") These should also be removed. Although Kent should verify that the console lock is not providing some sort of necessary side-effect synchronization. John Ogness
Re: [PATCH 0/2] Support direct I/O read and write for memory allocated by dmabuf
Am 10.07.24 um 15:57 schrieb Lei Liu: Use vm_insert_page to establish a mapping for the memory allocated by dmabuf, thus supporting direct I/O read and write; and fix the issue of incorrect memory statistics after mapping dmabuf memory. Well big NAK to that! Direct I/O is intentionally disabled on DMA-bufs. We already discussed enforcing that in the DMA-buf framework and this patch probably means that we should really do that. Regards, Christian. Lei Liu (2): mm: dmabuf_direct_io: Support direct_io for memory allocated by dmabuf mm: dmabuf_direct_io: Fix memory statistics error for dmabuf allocated memory with direct_io support drivers/dma-buf/heaps/system_heap.c | 5 +++-- fs/proc/task_mmu.c | 8 +++- include/linux/mm.h | 1 + mm/memory.c | 15 ++- mm/rmap.c | 9 + 5 files changed, 26 insertions(+), 12 deletions(-)
Re: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds for firmware loading
Hello Maxime, On 2024-07-10 09:13, Maxime Ripard wrote: On Tue, Jul 09, 2024 at 06:36:08PM GMT, Dragan Simic wrote: > > > > As I already wrote earlier, and as the above-linked discussions > > > > conclude, solving these issues doesn't belong to any specific driver. > > > > It should be resolved within the kernel's firmware loading mechanism > > > > instead, and no driver should be specific in that regard. > > > > > > IT would be good if it can be resolved within the kernel's firmware > > > loading mechanism. > > > > ... we'll need this as a systemic solution. > > The general policy has been to put drivers that need a firmware as a > module, and just never build them statically. I totally agree, but if Buildroot builds them statically and provides no initial ramdisk, we need a better solution than having various drivers attempt to implement their own workarounds. Buildroot typically allows custom kernel configurations, so it's not really "enforcing" anything like another distro does. It is definitely targetted towards very stripped down systems, so I guess building the drivers statically is a natural choice, but it works fine with modules too. It all leads to a conclusion that we need better in-kernel support for delayed firmware loading, instead of drivers implementing various workarounds, for the layouts in which drivers are built statically into the kernel image, but the required firmware blobs reside on the root filesystem. I'll start working on it, hopefully today. :)
Re: [PATCH v2 2/3] drm/mgag200: Remove vidrst callbacks from struct mgag200_device_funcs
On 10/07/2024 10:42, Thomas Zimmermann wrote: The callbacks disable_vidrst and enable_vidrst are obsolete. Remove the fields from struct mgag200_device_funcs. Instead call their implementations directly of the field 'has_vidrst' has been set in struct mgag200_device_info. Also change the logic slightly. The BMC used to start and stop scanout during the CRTC's atomic_enable and atomic_disable. Plane updates were done while the BMC scanned out the display. Now only stop once in atomic_disable at the beginning of a modeset and then restart the scanout at the end of a modeset in atomic_enable. While the modeset takes place, the BMC does not scanout at all. Thanks, it looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_drv.h | 12 drivers/gpu/drm/mgag200/mgag200_g200er.c | 7 ++- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 7 ++- drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 2 -- drivers/gpu/drm/mgag200/mgag200_g200se.c | 7 ++- drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 -- drivers/gpu/drm/mgag200/mgag200_mode.c| 15 --- 7 files changed, 10 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4b75613de882..4a46c8c006c8 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -247,18 +247,6 @@ struct mgag200_device_info { } struct mgag200_device_funcs { - /* -* Disables an external reset source (i.e., BMC) before programming -* a new display mode. -*/ - void (*disable_vidrst)(struct mga_device *mdev); - - /* -* Enables an external reset source (i.e., BMC) after programming -* a new display mode. -*/ - void (*enable_vidrst)(struct mga_device *mdev); - /* * Validate that the given state can be programmed into PIXPLLC. On * success, the calculated parameters should be stored in the CRTC's diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index abfbed6ec390..b3bb3e9fb0d1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -191,9 +191,6 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); const struct drm_format_info *format = mgag200_crtc_state->format; - if (funcs->disable_vidrst) - funcs->disable_vidrst(mdev); - mgag200_set_format_regs(mdev, format); mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); @@ -209,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (funcs->enable_vidrst) - funcs->enable_vidrst(mdev); + if (mdev->info->has_vidrst) + mgag200_bmc_enable_vidrst(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index acc9e2b5..3ac0a508e2c5 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -192,9 +192,6 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); const struct drm_format_info *format = mgag200_crtc_state->format; - if (funcs->disable_vidrst) - funcs->disable_vidrst(mdev); - mgag200_set_format_regs(mdev, format); mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); @@ -210,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (funcs->enable_vidrst) - funcs->enable_vidrst(mdev); + if (mdev->info->has_vidrst) + mgag200_bmc_enable_vidrst(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c index 839401e8b465..265f3e95830a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c @@ -146,8 +146,6 @@ static const struct mgag200_device_info mgag200_g200ew3_device_info = MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false); static const struct mgag200_device_funcs mgag200_g200ew3_device_funcs = { - .disable_vidrst = mgag200_bmc_disable_vidrst, - .enable_vidrst = mgag200_bmc_enable_vidrst, .pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check, .pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, // same
Re: [PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting
On 10/07/2024 10:42, Thomas Zimmermann wrote: The VRSTEN and HRSTEN bits control whether a CRTC synchronizes its display signal with an external source on the VIDRST pin. The G200WB and G200EW3 models synchronize with a BMC chip, but different external video encoders, such as the Matrox Maven, can also be attached to the pin. Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the bits are independent from the BMC. Add the field set_vidrst to the CRTC state for this purpose. Off by default, control the CRTC VIDRST setting from the CRTC's atomic_check helper. Thanks, I think this has less chance for regression. I've only a small nitpick below. Reviewed-by: Jocelyn Falempe v2: - keep logic entirely in CRTC (Jocelyn) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_bmc.c| 5 - drivers/gpu/drm/mgag200/mgag200_drv.h| 5 - drivers/gpu/drm/mgag200/mgag200_g200er.c | 2 +- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 2 +- drivers/gpu/drm/mgag200/mgag200_g200se.c | 2 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 14 ++ 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c index 23ef85aa7e37..1c7aa4f36787 100644 --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c @@ -77,11 +77,6 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev) { u8 tmp; - /* Ensure that the vrsten and hrsten are set */ - WREG8(MGAREG_CRTCEXT_INDEX, 1); - tmp = RREG8(MGAREG_CRTCEXT_DATA); - WREG8(MGAREG_CRTCEXT_DATA, tmp | 0x88); - /* Assert rstlvl2 */ WREG8(DAC_INDEX, MGA1064_REMHEADCTL2); tmp = RREG8(DAC_DATA); diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 7f7dfbd0f013..4b75613de882 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -179,6 +179,8 @@ struct mgag200_crtc_state { const struct drm_format_info *format; struct mgag200_pll_values pixpllc; + + bool set_vidrst; }; static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base) @@ -430,7 +432,8 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st .atomic_duplicate_state = mgag200_crtc_atomic_duplicate_state, \ .atomic_destroy_state = mgag200_crtc_atomic_destroy_state -void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode); +void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode, + bool set_vidrst); void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format); void mgag200_enable_display(struct mga_device *mdev); void mgag200_init_registers(struct mga_device *mdev); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index 4e8a1756138d..abfbed6ec390 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -195,7 +195,7 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index d884f3cb0ec7..acc9e2b5 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -196,7 +196,7 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c index a824bb8ad579..be4e124102c6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c @@ -327,7 +327,7 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc, funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mga
Re: [PATCH v2 3/3] drm/mgag200: Rename BMC vidrst names
On 10/07/2024 10:42, Thomas Zimmermann wrote: The BMC's scanout synchronization is only indirectly related to the VIDRST functionality. Do some renaming. Thanks, it looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_bmc.c| 4 ++-- drivers/gpu/drm/mgag200/mgag200_drv.h| 14 +++--- drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 ++-- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 ++-- drivers/gpu/drm/mgag200/mgag200_g200se.c | 4 ++-- drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c index 1c7aa4f36787..45e35dffb3ea 100644 --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c @@ -14,7 +14,7 @@ static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connect return container_of(connector, struct mgag200_bmc_connector, base); } -void mgag200_bmc_disable_vidrst(struct mga_device *mdev) +void mgag200_bmc_stop_scanout(struct mga_device *mdev) { u8 tmp; int iter_max; @@ -73,7 +73,7 @@ void mgag200_bmc_disable_vidrst(struct mga_device *mdev) } } -void mgag200_bmc_enable_vidrst(struct mga_device *mdev) +void mgag200_bmc_start_scanout(struct mga_device *mdev) { u8 tmp; diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4a46c8c006c8..f97eaa49b089 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -216,8 +216,8 @@ struct mgag200_device_info { */ unsigned long max_mem_bandwidth; - /* HW has external source (e.g., BMC) to synchronize with */ - bool has_vidrst:1; + /* Synchronize scanout with BMC */ + bool sync_bmc:1; struct { unsigned data_bit:3; @@ -232,13 +232,13 @@ struct mgag200_device_info { }; #define MGAG200_DEVICE_INFO_INIT(_max_hdisplay, _max_vdisplay, _max_mem_bandwidth, \ -_has_vidrst, _i2c_data_bit, _i2c_clock_bit, \ +_sync_bmc, _i2c_data_bit, _i2c_clock_bit, \ _bug_no_startadd) \ { \ .max_hdisplay = (_max_hdisplay), \ .max_vdisplay = (_max_vdisplay), \ .max_mem_bandwidth = (_max_mem_bandwidth), \ - .has_vidrst = (_has_vidrst), \ + .sync_bmc = (_sync_bmc), \ .i2c = { \ .data_bit = (_i2c_data_bit), \ .clock_bit = (_i2c_clock_bit), \ @@ -430,9 +430,9 @@ int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_avail /* mgag200_vga.c */ int mgag200_vga_output_init(struct mga_device *mdev); -/* mgag200_bmc.c */ -void mgag200_bmc_disable_vidrst(struct mga_device *mdev); -void mgag200_bmc_enable_vidrst(struct mga_device *mdev); +/* mgag200_bmc.c */ +void mgag200_bmc_stop_scanout(struct mga_device *mdev); +void mgag200_bmc_start_scanout(struct mga_device *mdev); int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector); #endif/* __MGAG200_DRV_H__ */ diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index b3bb3e9fb0d1..737a48aa9160 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -206,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (mdev->info->has_vidrst) - mgag200_bmc_enable_vidrst(mdev); + if (mdev->info->sync_bmc) + mgag200_bmc_start_scanout(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index 3ac0a508e2c5..8d1ccc2ad94a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -207,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (mdev->info->has_vidrst) - mgag200_bmc_enable_vidrst(mdev); + if (mdev->info->sync_bmc) + mgag200_bmc_start_scanout(mdev); } static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c index 7a8099eb100c..cf7f6897838f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c @@ -338,8 +338,8 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc, mgag200_enable_display(mdev); - if (mdev->info->has_vidrst) - mgag20
Re: [PATCH v4 1/3] dt-bindings: mailbox: Add mediatek,gce-props.yaml
On Sun, May 26, 2024 at 10:04 AM Jason-JH Lin (林睿祥) wrote: > > Hi Angelo, Jassi, > > Could you help me apply this series? > Thanks! > Please get it reviewed by DT maintainers Rob or Krzysztof. -Jassi
Re: [PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting
Hi Am 10.07.24 um 16:25 schrieb Jocelyn Falempe: + if (set_vidrst) crtcext1 |= MGAREG_CRTCEXT1_VRSTEN | MGAREG_CRTCEXT1_HRSTEN; + else + crtcext1 &= ~(MGAREG_CRTCEXT1_VRSTEN | MGAREG_CRTCEXT1_HRSTEN); The else case is useless, as crtcext1 has already this bits set to 0 unconditionnaly. Indeed. Will be fixed. Best regards Thomas crtcext2 = ((vtotal & 0xc00) >> 10) | ((vdisplay & 0x400) >> 8) | @@ -597,6 +599,7 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st struct mga_device *mdev = to_mga_device(dev); const struct mgag200_device_funcs *funcs = mdev->funcs; struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc); + struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state); struct drm_property_blob *new_gamma_lut = new_crtc_state->gamma_lut; int ret; @@ -607,6 +610,8 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st if (ret) return ret; + new_mgag200_crtc_state->set_vidrst = mdev->info->has_vidrst; + if (new_crtc_state->mode_changed) { if (funcs->pixpllc_atomic_check) { ret = funcs->pixpllc_atomic_check(crtc, new_state); @@ -656,7 +661,7 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_ funcs->disable_vidrst(mdev); mgag200_set_format_regs(mdev, format); - mgag200_set_mode_regs(mdev, adjusted_mode); + mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst); if (funcs->pixpllc_atomic_update) funcs->pixpllc_atomic_update(crtc, old_state); @@ -717,6 +722,7 @@ struct drm_crtc_state *mgag200_crtc_atomic_duplicate_state(struct drm_crtc *crtc new_mgag200_crtc_state->format = mgag200_crtc_state->format; memcpy(&new_mgag200_crtc_state->pixpllc, &mgag200_crtc_state->pixpllc, sizeof(new_mgag200_crtc_state->pixpllc)); + new_mgag200_crtc_state->set_vidrst = mgag200_crtc_state->set_vidrst; return &new_mgag200_crtc_state->base; } -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Re: [PATCH 0/2] Support direct I/O read and write for memory allocated by dmabuf
Am 10.07.24 um 16:35 schrieb Lei Liu: 在 2024/7/10 22:14, Christian König 写道: Am 10.07.24 um 15:57 schrieb Lei Liu: Use vm_insert_page to establish a mapping for the memory allocated by dmabuf, thus supporting direct I/O read and write; and fix the issue of incorrect memory statistics after mapping dmabuf memory. Well big NAK to that! Direct I/O is intentionally disabled on DMA-bufs. Hello! Could you explain why direct_io is disabled on DMABUF? Is there any historical reason for this? It's basically one of the most fundamental design decision of DMA-Buf. The attachment/map/fence model DMA-buf uses is not really compatible with direct I/O on the underlying pages. We already discussed enforcing that in the DMA-buf framework and this patch probably means that we should really do that. Regards, Christian. Thank you for your response. With the application of AI large model edgeification, we urgently need support for direct_io on DMABUF to read some very large files. Do you have any new solutions or plans for this? We have seen similar projects over the years and all of those turned out to be complete shipwrecks. There is currently a patch set under discussion to give the network subsystem DMA-buf support. If you are interest in network direct I/O that could help. Additional to that a lot of GPU drivers support userptr usages, e.g. to import malloced memory into the GPU driver. You can then also do direct I/O on that malloced memory and the kernel will enforce correct handling with the GPU driver through MMU notifiers. But as far as I know a general DMA-buf based solution isn't possible. Regards, Christian. Regards, Lei Liu. Lei Liu (2): mm: dmabuf_direct_io: Support direct_io for memory allocated by dmabuf mm: dmabuf_direct_io: Fix memory statistics error for dmabuf allocated memory with direct_io support drivers/dma-buf/heaps/system_heap.c | 5 +++-- fs/proc/task_mmu.c | 8 +++- include/linux/mm.h | 1 + mm/memory.c | 15 ++- mm/rmap.c | 9 + 5 files changed, 26 insertions(+), 12 deletions(-)
RE: [PATCH] drm/i915/gt: Do not consider preemption during execlists_dequeue for gen8
> -Original Message- > From: Tvrtko Ursulin > Sent: Wednesday, July 10, 2024 4:22 PM > To: Gote, Nitin R ; intel-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org; Shyti, Andi ; > chris.p.wil...@linux.intel.com; Das, Nirmoy ; > janusz.krzyszto...@linux.intel.com; sta...@vger.kernel.org > Subject: Re: [PATCH] drm/i915/gt: Do not consider preemption during > execlists_dequeue for gen8 > > > On 09/07/2024 15:02, Tvrtko Ursulin wrote: > > > > On 09/07/2024 13:53, Nitin Gote wrote: > >> We're seeing a GPU HANG issue on a CHV platform, which was caused by > >> bac24f59f454 ("drm/i915/execlists: Enable coarse preemption > >> boundaries for gen8"). > >> > >> Gen8 platform has only timeslice and doesn't support a preemption > >> mechanism as engines do not have a preemption timer and doesn't send > >> an irq if the preemption timeout expires. So, add a fix to not > >> consider preemption during dequeuing for gen8 platforms. > >> > >> Also move can_preemt() above need_preempt() function to resolve > >> implicit declaration of function ‘can_preempt' error and make > >> can_preempt() function param as const to resolve error: passing > >> argument 1 of ‘can_preempt’ discards ‘const’ qualifier from the pointer > target type. > >> > >> Fixes: bac24f59f454 ("drm/i915/execlists: Enable coarse preemption > >> boundaries for gen8") > >> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11396 > >> Suggested-by: Andi Shyti > >> Signed-off-by: Nitin Gote > >> Cc: Chris Wilson > >> CC: # v5.2+ > >> --- > >> .../drm/i915/gt/intel_execlists_submission.c | 24 > >> --- > >> 1 file changed, 15 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > >> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > >> index 21829439e686..30631cc690f2 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > >> @@ -294,11 +294,26 @@ static int virtual_prio(const struct > >> intel_engine_execlists *el) > >> return rb ? rb_entry(rb, struct ve_node, rb)->prio : INT_MIN; > >> } > >> +static bool can_preempt(const struct intel_engine_cs *engine) { > >> + if (GRAPHICS_VER(engine->i915) > 8) > >> + return true; > >> + > >> + if (IS_CHERRYVIEW(engine->i915) || IS_BROADWELL(engine->i915)) > >> + return false; > >> + > >> + /* GPGPU on bdw requires extra w/a; not implemented */ > >> + return engine->class != RENDER_CLASS; > > > > Aren't BDW and CHV the only Gen8 platforms, in which case this > > function can be simplifies as: > > > > ... > > { > > return GRAPHICS_VER(engine->i915) > 8; } > > > > ? > > > >> +} Yes. I will simply this function. > >> + > >> static bool need_preempt(const struct intel_engine_cs *engine, > >> const struct i915_request *rq) > >> { > >> int last_prio; > >> + if ((GRAPHICS_VER(engine->i915) <= 8) && can_preempt(engine)) > > > > The GRAPHICS_VER check here looks redundant with the one inside > > can_preempt(). True. I will update the condition. > > One more thing - I think gen8_emit_bb_start() becomes dead code after this > and can be removed. I think gen8_emit_bb_start() still require for graphics version < 12 as it is used in else part of if (GRAPHICS_VER_FULL(engine->i915) >= IP_VER(12, 55)) condition. So, it will not be dead code. Thanks and Regards, Nitin > > Regards, > > Tvrtko > > >> + return false; > >> + > >> if (!intel_engine_has_semaphores(engine)) > >> return false; > >> @@ -3313,15 +3328,6 @@ static void remove_from_engine(struct > >> i915_request *rq) > >> i915_request_notify_execute_cb_imm(rq); > >> } > >> -static bool can_preempt(struct intel_engine_cs *engine) -{ > >> - if (GRAPHICS_VER(engine->i915) > 8) > >> - return true; > >> - > >> - /* GPGPU on bdw requires extra w/a; not implemented */ > >> - return engine->class != RENDER_CLASS; -} > >> - > >> static void kick_execlists(const struct i915_request *rq, int prio) > >> { > >> struct intel_engine_cs *engine = rq->engine;