Re: [PATCH 5/5] accel/ivpu: Mark 64 kB contiguous areas as contiguous in PTEs
On Thu, May 18, 2023 at 08:17:02AM -0600, Jeffrey Hugo wrote: > On 5/18/2023 7:16 AM, Stanislaw Gruszka wrote: > > From: Karol Wachowski > > > > Whenever KMD maps region larger than 64kB that is both aligned and > > contiguous, set contiguous bit (52) in MMU PTE descriptor for each page > > in that region. > > > > This allows to treat 16 contiguous pages as one and reduce > > number of MMU page walks required what results in lower latency. > > what -> which Fixed this and applied the set to drm-misc-next Thanks Stanislaw
Re: [PATCH] accel/ivpu: Use struct_size()
On Thu, Jun 01, 2023 at 05:58:48PM +0200, Marco Pagani wrote: > > On 2023-05-29 15:28, Christophe JAILLET wrote: > > Use struct_size() instead of hand-writing it. It is less verbose, more > > robust and more informative. > > > > Signed-off-by: Christophe JAILLET > > Reviewed-by: Marco Pagani Applied to drm-misc-next Thanks Stanislaw > > > --- > > drivers/accel/ivpu/ivpu_job.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c > > index 3c6f1e16cf2f..0a09bba8da24 100644 > > --- a/drivers/accel/ivpu/ivpu_job.c > > +++ b/drivers/accel/ivpu/ivpu_job.c > > @@ -289,15 +289,13 @@ ivpu_create_job(struct ivpu_file_priv *file_priv, u32 > > engine_idx, u32 bo_count) > > { > > struct ivpu_device *vdev = file_priv->vdev; > > struct ivpu_job *job; > > - size_t buf_size; > > int ret; > > > > ret = ivpu_rpm_get(vdev); > > if (ret < 0) > > return NULL; > > > > - buf_size = sizeof(*job) + bo_count * sizeof(struct ivpu_bo *); > > - job = kzalloc(buf_size, GFP_KERNEL); > > + job = kzalloc(struct_size(job, bos, bo_count), GFP_KERNEL); > > if (!job) > > goto err_rpm_put; > > >
Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
Hi Douglas, kernel test robot noticed the following build errors: [auto build test ERROR on robh/for-next] [also build test ERROR on hid/for-next dtor-input/next dtor-input/for-linus] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-touchscreens/20230608-055515 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15%40changeid patch subject: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower config: arc-randconfig-r021-20230607 (https://download.01.org/0day-ci/archive/20230608/202306081344.m0jnn0ce-...@intel.com/config) compiler: arceb-elf-gcc (GCC) 12.3.0 reproduce (this is a W=1 build): mkdir -p ~/bin wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git remote add robh https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git git fetch robh for-next git checkout robh/for-next b4 shazam https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arc olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202306081344.m0jnn0ce-...@intel.com/ All errors (new ones prefixed by >>): `.exit.text' referenced in section `__jump_table' of lib/test_dynamic_debug.o: defined in discarded section `.exit.text' of lib/test_dynamic_debug.o `.exit.text' referenced in section `__jump_table' of lib/test_dynamic_debug.o: defined in discarded section `.exit.text' of lib/test_dynamic_debug.o `.exit.text' referenced in section `__jump_table' of drivers/misc/phantom.o: defined in discarded section `.exit.text' of drivers/misc/phantom.o `.exit.text' referenced in section `__jump_table' of drivers/misc/phantom.o: defined in discarded section `.exit.text' of drivers/misc/phantom.o `.exit.text' referenced in section `__jump_table' of drivers/target/target_core_configfs.o: defined in discarded section `.exit.text' of drivers/target/target_core_configfs.o `.exit.text' referenced in section `__jump_table' of drivers/target/target_core_configfs.o: defined in discarded section `.exit.text' of drivers/target/target_core_configfs.o arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.o: in function `i2c_hid_core_remove': drivers/hid/i2c-hid/i2c-hid-core.c:1218: undefined reference to `drm_panel_remove_follower' >> arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.c:1218: undefined reference >> to `drm_panel_remove_follower' arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.o: in function `i2c_hid_core_probe': drivers/hid/i2c-hid/i2c-hid-core.c:1159: undefined reference to `drm_panel_add_follower' >> arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.c:1159: undefined reference >> to `drm_panel_add_follower' -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/7 20:19, Maxime Ripard wrote: On Wed, Jun 07, 2023 at 06:30:01PM +0800, Sui Jingfeng wrote: On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Not really, no. All DT support is doing is setting some generic device parameter based on that property, but the infrastructure is very much generic and can be used on systems without a DT. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. > Please do not mitigate the problems with confusing method. It's not a confusing method, it's one of the two main API to deal with DMA buffers. And you might disagree with Paul but there's no need to be rude about it. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific thing. Dependent on how does the arch_dma_ops is implemented. The definition of the coherent on different ARCH has different meanings. The definition of the wirte-combine on different ARCH has different meanings. The wirte-combine(uncache acceleration) on mips is non dma-coherent. Then MIPS breaks the DMA allocation semantics. A buffer allocated with dma_alloc_wc is supposed to be coherent. But on arm, It seem that wirte-combine is coherent. (guaranteed by arch implement). I also heard using dma_alloc_coherent to allocation the buffer for the non-coherent doesn't hurt, but the reverse is not true. But please do not create confusion. software composite is faster because better cacheusing rate and cache is faster to read. It is faster because it is cached, not because it is non-coherent. non-coherent is arch thing and/or driver-side thing, it is a side effect of using the cached mapping. Honestly, it's not clear to me what your point or issue is. Going back to the description in your commit log, you mention that you want to support multiple hardware that might or might not be DMA coherent, and thus you want to allocate a buffer with different attributes depending on that? Like, you say that the LS3A4000 has a coherency unit and thus doing the allocation with dma_alloc_coherent is preferred. Preferred to what? A WC buffer? Why? A WC buffer is a coherent buffer that is allowed to cache writes. It doesn't have to, and worst case scenario you're inexactly the same case than a dma_alloc_coherent buffer. It should left to driver to handle such a side effect. The device driver know their device, so its the device driver's responsibility to maintain the coherency. Not really, no. Some driver are used across multiple SoCs and multiple arch. It doesn't make any sense to encode this in the driver... which is why it's in the DT in the first place, and abstracted away by the DMA API. Like, do you really expect the amdgpu driver to know the DMA attributes it needs to allocate a buffer from when running from a RaspberryPi? On loongson platform, we don't need to call drm_fb_dma_sync_non_coherent() function, Its already guaranteed by hardware. And mostly guaranteed by dma_alloc_coherent. And if you wanted to call it anyway, it would be a nop if the device is declared as coherent already. I think you're thinking about this backward. A buffer has mapping attributes, and a device has hardware properties. The driver (ie,
[PATCH v2] drm/logicvc: Kconfig: select REGMAP and REGMAP_MMIO
drm/logicvc driver is depend on REGMAP and REGMAP_MMIO, should select this two kconfig option, otherwise the driver failed to compile on platform without REGMAP_MMIO selected: ERROR: modpost: "__devm_regmap_init_mmio_clk" [drivers/gpu/drm/logicvc/logicvc-drm.ko] undefined! make[1]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1 make: *** [Makefile:1978: modpost] Error 2 Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/logicvc/Kconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/logicvc/Kconfig b/drivers/gpu/drm/logicvc/Kconfig index fa7a88368809..1df22a852a23 100644 --- a/drivers/gpu/drm/logicvc/Kconfig +++ b/drivers/gpu/drm/logicvc/Kconfig @@ -5,5 +5,7 @@ config DRM_LOGICVC select DRM_KMS_HELPER select DRM_KMS_DMA_HELPER select DRM_GEM_DMA_HELPER + select REGMAP + select REGMAP_MMIO help DRM display driver for the logiCVC programmable logic block from Xylon -- 2.25.1
[PATCH v3] drm/bridge: ti-sn65dsi86: Avoid possible buffer overflow
Smatch error:buffer overflow 'ti_sn_bridge_refclk_lut' 5 <= 5. Fixes: cea86c5bb442 ("drm/bridge: ti-sn65dsi86: Implement the pwm_chip") Signed-off-by: Su Hui --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 7a748785c545..4676cf2900df 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -298,6 +298,10 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) if (refclk_lut[i] == refclk_rate) break; + /* avoid buffer overflow and "1" is the default rate in the datasheet. */ + if (i >= refclk_lut_size) + i = 1; + regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, REFCLK_FREQ(i)); -- 2.30.2
Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Avoid possible buffer overflow
On 2023/6/7 22:03, Doug Anderson wrote: Hi, On Tue, Jun 6, 2023 at 6:25 PM Su Hui wrote: Smatch error:buffer overflow 'ti_sn_bridge_refclk_lut' 5 <= 5. Fixes: cea86c5bb442 ("drm/bridge: ti-sn65dsi86: Implement the pwm_chip") Signed-off-by: Su Hui --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 7a748785c545..bb88406495e9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -305,7 +305,7 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG, * regardless of its actual sourcing. */ - pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i]; + pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i < refclk_lut_size ? i : 1]; This looks more correct, but it really needs a comment since it's totally not obviously what you're doing here. IMO the best solution here is to update "i" right after the for loop and have a comment about the datasheet saying that "1" is the default rate so we'll fall back to that if we couldn't find a match. Moving it to right after the for loop will change the value written into the registers, but that's fine and makes it clearer what's happening. Got it. Add some comment and move the code up. I will send patch v3 soon. Thanks for your suggestion again :) . Su Hui -Doug
Re: [Intel-gfx] [PATCH] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset
Hi Dong, On Wed, Jun 07, 2023 at 12:03:50PM -0700, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed work > and intel_gt_reset. > Switched from cancel_delayed_work_sync to cancel_delayed_work, the non-sync > version for reset path, it is safe as the worker has the trylock code to > handle the lock; Meanwhile keep the sync version for park/fini to ensure the > worker is not still running during suspend or shutdown. > > WARNING: possible circular locking dependency detected > 6.4.0-rc1-drmtip_1340-g31e3463b0edb+ #1 Not tainted > -- > kms_pipe_crc_ba/6415 is trying to acquire lock: > 88813e6cc640 > ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}, at: > __flush_work+0x42/0x530 > > but task is already holding lock: > 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: > intel_gt_reset+0x19e/0x470 [i915] > > which lock already depends on the new lock. > > the existing dependency chain (in reverse order) is: > > -> #3 (>reset.mutex){+.+.}-{3:3}: > lock_acquire+0xd8/0x2d0 > i915_gem_shrinker_taints_mutex+0x31/0x50 [i915] > intel_gt_init_reset+0x65/0x80 [i915] > intel_gt_common_init_early+0xe1/0x170 [i915] > intel_root_gt_init_early+0x48/0x60 [i915] > i915_driver_probe+0x671/0xcb0 [i915] > i915_pci_probe+0xdc/0x210 [i915] > pci_device_probe+0x95/0x120 > really_probe+0x164/0x3c0 > __driver_probe_device+0x73/0x160 > driver_probe_device+0x19/0xa0 > __driver_attach+0xb6/0x180 > bus_for_each_dev+0x77/0xd0 > bus_add_driver+0x114/0x210 > driver_register+0x5b/0x110 > __pfx_vgem_open+0x3/0x10 [vgem] > do_one_initcall+0x57/0x270 > do_init_module+0x5f/0x220 > load_module+0x1ca4/0x1f00 > __do_sys_finit_module+0xb4/0x130 > do_syscall_64+0x3c/0x90 > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > -> #2 (fs_reclaim){+.+.}-{0:0}: > lock_acquire+0xd8/0x2d0 > fs_reclaim_acquire+0xac/0xe0 > kmem_cache_alloc+0x32/0x260 > i915_vma_instance+0xb2/0xc60 [i915] > i915_gem_object_ggtt_pin_ww+0x175/0x370 [i915] > vm_fault_gtt+0x22d/0xf60 [i915] > __do_fault+0x2f/0x1d0 > do_pte_missing+0x4a/0xd20 > __handle_mm_fault+0x5b0/0x790 > handle_mm_fault+0xa2/0x230 > do_user_addr_fault+0x3ea/0xa10 > exc_page_fault+0x68/0x1a0 > asm_exc_page_fault+0x26/0x30 > > -> #1 (>reset.backoff_srcu){}-{0:0}: > lock_acquire+0xd8/0x2d0 > _intel_gt_reset_lock+0x57/0x330 [i915] > guc_timestamp_ping+0x35/0x130 [i915] > process_one_work+0x250/0x510 > worker_thread+0x4f/0x3a0 > kthread+0xff/0x130 > ret_from_fork+0x29/0x50 > > -> #0 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}: > check_prev_add+0x90/0xc60 > __lock_acquire+0x1998/0x2590 > lock_acquire+0xd8/0x2d0 > __flush_work+0x74/0x530 > __cancel_work_timer+0x14f/0x1f0 > intel_guc_submission_reset_prepare+0x81/0x4b0 [i915] > intel_uc_reset_prepare+0x9c/0x120 [i915] > reset_prepare+0x21/0x60 [i915] > intel_gt_reset+0x1dd/0x470 [i915] > intel_gt_reset_global+0xfb/0x170 [i915] > intel_gt_handle_error+0x368/0x420 [i915] > intel_gt_debugfs_reset_store+0x5c/0xc0 [i915] > i915_wedged_set+0x29/0x40 [i915] > simple_attr_write_xsigned.constprop.0+0xb4/0x110 > full_proxy_write+0x52/0x80 > vfs_write+0xc5/0x4f0 > ksys_write+0x64/0xe0 > do_syscall_64+0x3c/0x90 > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > other info that might help us debug this: > Chain exists of: > (work_completion)(&(>timestamp.work)->work) --> fs_reclaim --> > >reset.mutex > Possible unsafe locking scenario: > CPU0CPU1 > >lock(>reset.mutex); > lock(fs_reclaim); > lock(>reset.mutex); >lock((work_completion)(&(>timestamp.work)->work)); > > *** DEADLOCK *** > 3 locks held by kms_pipe_crc_ba/6415: > #0: 888101541430 (sb_writers#15){.+.+}-{0:0}, at: ksys_write+0x64/0xe0 > #1: 888136c7eab8 (>mutex){+.+.}-{3:3}, at: > simple_attr_write_xsigned.constprop.0+0x47/0x110 > #2: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: > intel_gt_reset+0x19e/0x470 [i915] > > Signed-off-by: Zhanjun Dong Andrzej's r-b is missing here. > --- Please add a version to your patch and a changelog. Thanks, Andi
Re: [v1] drm/xe/guc: Fix h2g_write usage of GUC_CTB_MSG_MAX_LEN
On Fri, 2023-06-02 at 11:52 -0700, Alan Previn wrote: > In the ABI header, GUC_CTB_MSG_MIN_LEN is '1' because > GUC_CTB_HDR_LEN is 1. This aligns with H2G/G2H CTB specification > where all command formats are defined in units of dwords so that '1' > is a dword. Accordingly, GUC_CTB_MSG_MAX_LEN is 256-1 (i.e. 255 > dwords). However, h2g_write was incorrectly assuming that > GUC_CTB_MSG_MAX_LEN was in bytes. Fix this. > > alan:snip > The patch itself make sense but concerned about the value of > GUC_CTB_MSG_MAX_LEN. Is the max size of CTB really 256 DWs? That seems > way to large. Also I think some tools are going to complain with the > stack being this large too. > > Matt alan: good point - i will go back and find if we have this value internally spec'd before we continue.
Re: [PATCH v6 00/12] SM63(50|75) DPU support
On 06/06/2023 15:43, Konrad Dybcio wrote: [skipped the changelog] --- Konrad Dybcio (12): dt-bindings: display/msm: dsi-controller-main: Add SM6350 dt-bindings: display/msm: dsi-controller-main: Add SM6375 dt-bindings: display/msm: sc7180-dpu: Describe SM6350 and SM6375 dt-bindings: display/msm: Add SM6350 MDSS dt-bindings: display/msm: Add SM6375 MDSS drm/msm/dpu: Add SM6350 support drm/msm: mdss: Add SM6350 support drm/msm/dpu: Add SM6375 support drm/msm: mdss: Add SM6375 support Will, we have finally picked up the display related patches. Could you please pick up the IOMMU patches if they look fine to you. iommu/arm-smmu-qcom: Sort the compatible list alphabetically iommu/arm-smmu-qcom: Add SM6375 DPU compatible iommu/arm-smmu-qcom: Add SM6350 DPU compatible .../bindings/display/msm/dsi-controller-main.yaml | 4 + .../bindings/display/msm/qcom,sc7180-dpu.yaml | 23 ++- .../bindings/display/msm/qcom,sm6350-mdss.yaml | 213 .../bindings/display/msm/qcom,sm6375-mdss.yaml | 215 + .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 173 + .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 139 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 2 + drivers/gpu/drm/msm/msm_mdss.c | 10 + drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 +- 11 files changed, 790 insertions(+), 3 deletions(-) --- base-commit: 6db29e14f4fb7bce9eb5290288e71b05c2b0d118 change-id: 20230411-topic-straitlagoon_mdss-8f34cacd5e26 Best regards, -- With best wishes Dmitry
Re: [PATCH v17] drm/msm/dpu: add DSC blocks to the catalog of MSM8998
On Tue, 06 Jun 2023 13:11:12 -0700, Kuogee Hsieh wrote: > Some platforms have DSC blocks which have not been declared in the catalog. > Complete DSC 1.1 support for all platforms by adding the missing blocks to > MSM8998. > > Changes in v9: > -- add MSM8998 and SC8180x to commit title > > [...] Applied, thanks! [1/1] drm/msm/dpu: add DSC blocks to the catalog of MSM8998 https://gitlab.freedesktop.org/lumag/msm/-/commit/203b2019b3ac Best regards, -- Dmitry Baryshkov
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On Tue, 06 Jun 2023 11:33:03 +0300, Dan Carpenter wrote: > The "vsync_hz" variable is unsigned int so it can't be less > than zero. The dpu_kms_get_clk_rate() function used to return a u64 > but I previously changed it to return an unsigned long and zero on > error so it matches clk_get_rate(). > > Change the "vsync_hz" type to unsigned long as well and change the > error checking to check for zero instead of negatives. This change > does not affect runtime at all. It's just a clean up. > > [...] Applied, thanks! [1/1] drm/msm/dpu: tidy up some error checking https://gitlab.freedesktop.org/lumag/msm/-/commit/e7a2cf8e058e Best regards, -- Dmitry Baryshkov
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On 07/06/2023 17:42, Dan Carpenter wrote: On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote: @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config( * frequency divided by the no. of rows (lines) in the LCDpanel. */ vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync"); - if (vsync_hz <= 0) { - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n", + if (!vsync_hz) { + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n", vsync_hz); Nit: no need to print the value here, you know it's zero. Could be clarified to just "no vsync clock". Yeah. That's obviously not useful. Sorry, I will resend. I'll fix while applying. Seems easier. -- With best wishes Dmitry
Re: [PATCH v17] drm/msm/dpu: add DSC blocks to the catalog of MSM8998
On 06/06/2023 23:11, Kuogee Hsieh wrote: From: Abhinav Kumar Some platforms have DSC blocks which have not been declared in the catalog. Complete DSC 1.1 support for all platforms by adding the missing blocks to MSM8998. Changes in v9: -- add MSM8998 and SC8180x to commit title Changes in v10: -- fix grammar at commit text Changes in v12: -- fix "titil" with "title" at changes in v9 Changes in v14: -- "dsc" tp "DSC" at commit title Changes in v15: -- fix merge conflicts at dpu_5_1_sc8180x.h Changes in v16 -- fix cherry-pick error by deleting both redundant .dsc and .dsc_count assignment from dpu_5_1_sc8180x.h Changes in v17 -- remove sc8180x from both commit title and text -- remove Reviewed-by Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 7 +++ 1 file changed, 7 insertions(+) Reviewed-by: Dmitry Baryshkov I'll fix the commit message. -- With best wishes Dmitry
Re: [PATCH v6 00/12] SM63(50|75) DPU support
On Tue, 06 Jun 2023 14:43:51 +0200, Konrad Dybcio wrote: > v5 -> v6: > - Drop unnecessary items: level in bindings > - Use INTF_SC7180_MASK for 6375 to avoid enabling DPU_INTF_DATA_COMPRESS on > DPU6 > - Pick up tags > > v5: > https://lore.kernel.org/r/20230411-topic-straitlagoon_mdss-v5-0-998b4d2f7...@linaro.org > > [...] Applied, thanks! [01/12] dt-bindings: display/msm: dsi-controller-main: Add SM6350 https://gitlab.freedesktop.org/lumag/msm/-/commit/e99b2d0670a7 [02/12] dt-bindings: display/msm: dsi-controller-main: Add SM6375 https://gitlab.freedesktop.org/lumag/msm/-/commit/27a869221bb7 [03/12] dt-bindings: display/msm: sc7180-dpu: Describe SM6350 and SM6375 https://gitlab.freedesktop.org/lumag/msm/-/commit/ed41005f5b7c [04/12] dt-bindings: display/msm: Add SM6350 MDSS https://gitlab.freedesktop.org/lumag/msm/-/commit/3b7502b0c205 [05/12] dt-bindings: display/msm: Add SM6375 MDSS https://gitlab.freedesktop.org/lumag/msm/-/commit/2a5c1021bc77 [06/12] drm/msm/dpu: Add SM6350 support https://gitlab.freedesktop.org/lumag/msm/-/commit/3186acba5cdc [07/12] drm/msm: mdss: Add SM6350 support https://gitlab.freedesktop.org/lumag/msm/-/commit/c2c1217e61bd [08/12] drm/msm/dpu: Add SM6375 support https://gitlab.freedesktop.org/lumag/msm/-/commit/27f0df03f3ff [09/12] drm/msm: mdss: Add SM6375 support https://gitlab.freedesktop.org/lumag/msm/-/commit/5ff3d3a0a09e Best regards, -- Dmitry Baryshkov
Re: [PATCH v2 1/2] drm/msm/dpu: use PINGPONG_NONE to unbind INTF from PP
On Sun, 04 Jun 2023 06:13:07 +0300, Dmitry Baryshkov wrote: > Currently the driver passes the PINGPONG index to > dpu_hw_intf_ops::bind_pingpong_blk() callback and uses separate boolean > flag to tell whether INTF should be bound or unbound. Simplify this by > passing PINGPONG_NONE in case of unbinding and drop the flag completely. > > Applied, thanks! [1/2] drm/msm/dpu: use PINGPONG_NONE to unbind INTF from PP https://gitlab.freedesktop.org/lumag/msm/-/commit/a03b7c4698d7 [2/2] drm/msm/dpu: use PINGPONG_NONE to unbind WB from PP https://gitlab.freedesktop.org/lumag/msm/-/commit/0f86d9c980a3 Best regards, -- Dmitry Baryshkov
Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable
Geert Uytterhoeven writes: Hello Geert and Thomas, > Hi Thomas, > > On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann wrote: >> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven: >> > On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann >> > wrote: >> >> --- a/drivers/video/fbdev/Kconfig >> >> +++ b/drivers/video/fbdev/Kconfig >> >> @@ -57,6 +57,15 @@ config FIRMWARE_EDID >> >>combination with certain motherboards and monitors are known to >> >>suffer from this problem. >> >> >> >> +config FB_DEVICE >> >> +bool "Provide legacy /dev/fb* device" >> > >> > Perhaps "default y if !DRM", although that does not help for a >> > mixed drm/fbdev kernel build? >> >> We could simply set it to "default y". But OTOH is it worth making it a >> default? Distributions will set it to the value they need/want. The very >> few people that build their own kernels to get certain fbdev drivers >> will certainly be able to enable the option by hand as well. > > Defaulting to "n" (the default) means causing regressions when these > few people use an existing defconfig. > Having "dfault y if !DRM" makes sense to me. I guess is a corner case but at least it won't silently be disabled for users that only want fbdev as Geert mentioned. I wouldn't call it a regression though, because AFAIK the Kconfig options are not a stable API ? >> > Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE, >> > to be selected by both FB and DRM_FBDEV_EMULATION? >> > Then FB_DEVICE can depend on FB_CORE, and default to y if FB. Funny that you mention because it's exactly what I attempted in the past: https://lore.kernel.org/all/20210827100531.1578604-1-javi...@redhat.com/T/#u >> >> That wouldn't work. In Tumbleweed, we still have efifb and vesafb >> enabled under certain conditions; merely for the kernel console. We'd >> have to enable CONFIG_FB, which would bring back the device. > > "Default y" does not mean that you cannot disable FB_DEVICE, so > you are not forced to bring back the device? > I think we can have both to make the kernel more configurable: 1) Allow to only disable fbdev user-space APIs (/dev/fb?, /proc/fb, etc), which is what the series is doing with the new FB_DEVICE config symbol. 2) Allow to disable all "native" fbdev drivers and only keep the DRM fbdev emulation layer. That's what my series attempted to do with the FB_CORE Kconfig symbol. I believe that there are use cases for both, for example as Thomas' said many distros are disabling all the fbdev drivers and their user-space only requires DRM/KMS, so makes sense to not expose any fbdev uAPI at all. But may be that other users want the opposite, they have an old user-space that requires fbdev, but is running on newer hardware that only have a DRM driver. So they will want DRM fbdev emulation but none fbdev driver at all. That's why I think that FB_DEVICE and FB_CORE are complementary and we can support any combination of the two, if you agree there are uses for either. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v1 2/8] dt-bindings: display: panel: mipi-dbi-spi: add spi-3wire property
On Wed, 07 Jun 2023 13:55:01 +0200, Leonard Göhrs wrote: > Some MIPI DBI panels support a three wire mode (clock, chip select, > bidirectional data) that can be used to ask the panel if it is already set > up by e.g. the bootloader and can thus skip the initialization. > This enables a flicker-free boot. > > Signed-off-by: Leonard Göhrs > --- > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Reviewed-by: Rob Herring
Re: [PATCH v1 1/8] dt-bindings: display: panel: mipi-dbi-spi: add shineworld lh133k compatible
On Wed, 07 Jun 2023 13:55:00 +0200, Leonard Göhrs wrote: > The Shineworld LH133K is a 1.3" 240x240px RGB LCD with a MIPI DBI > compatible SPI interface. > The initialization procedure is quite basic with the exception of > requiring inverted colors. > A basic mipi-dbi-cmd[1] script to get the display running thus looks > like this: > > $ cat shineworld,lh133k.txt > command 0x11 # exit sleep mode > delay 120 > > # The display seems to require display color inversion, so enable it. > command 0x21 # INVON > > # Enable normal display mode (in contrast to partial display mode). > command 0x13 # NORON > command 0x29 # MIPI_DCS_SET_DISPLAY_ON > > $ mipi-dbi-cmd shineworld,lh133k.bin shineworld,lh133k.txt > > [1]: https://github.com/notro/panel-mipi-dbi > > Signed-off-by: Leonard Göhrs > --- > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 1 + > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 2 files changed, 3 insertions(+) > Reviewed-by: Rob Herring
Re: [PATCH v1 2/8] dt-bindings: display: panel: mipi-dbi-spi: add spi-3wire property
On Wed, Jun 07, 2023 at 09:59:47PM +0200, Noralf Trønnes wrote: > > > On 6/7/23 13:55, Leonard Göhrs wrote: > > Some MIPI DBI panels support a three wire mode (clock, chip select, > > bidirectional data) that can be used to ask the panel if it is already set > > up by e.g. the bootloader and can thus skip the initialization. > > This enables a flicker-free boot. > > > > Signed-off-by: Leonard Göhrs > > --- > > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > > index c07da1a9e6288..2f0238b770eba 100644 > > --- > > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > > +++ > > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > > @@ -87,6 +87,8 @@ properties: > >Logic level supply for interface signals (Vddi). > >No need to set if this is the same as power-supply. > > > > + spi-3wire: true > > + > > I don't think this should be added here. spi-cpha and spi-cpol are also > supported but they are not mentioned. Instead those are documented in > bindings/spi/spi-controller.yaml. Why they're not documented in > bindings/spi/spi-peripheral-props.yaml instead which this binding has a > ref to, I have no idea. spi-peripheral-props.yaml are properties of the controller in the peripheral nodes. spi-cpha and spi-cpol are properties of the device which are completely invalid on some devices. We can only check that by documenting where they are valid. I think spi-3wire is similar. There should be more explanation in the spi-peripheral-props.yaml commit history. Rob
Re: [PATCH 28/30] fbdev/core: Move file-I/O code into separate file
Thomas Zimmermann writes: > Move fbdev's file-I/O code into a separate file and contain it in > init and cleanup helpers. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- [...] > + > +#include #include here, the robot complained about: drivers/video/fbdev/core/fb_devfs.c:183:9: error: unknown type name 'compat_caddr_t' -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH net-next v6 2/6] dt-bindings: net: Brcm ASP 2.0 Ethernet controller
On Thu, Jun 01, 2023 at 03:12:27PM -0700, Justin Chen wrote: > From: Florian Fainelli > > Add a binding document for the Broadcom ASP 2.0 Ethernet > controller. > > Reviewed-by: Conor Dooley > Signed-off-by: Florian Fainelli > Signed-off-by: Justin Chen > --- > v6 > - Moved compatible to the top > - Changed quotes to be consistent > - Elaborated on brcm,channel description > > v5 > - Fix compatible string yaml format to properly capture what we want > > v4 > - Adjust compatible string example to reference SoC and HW ver > > v3 > - Minor formatting issues > - Change channel prop to brcm,channel for vendor specific format > - Removed redundant v2.0 from compat string > - Fix ranges field > > v2 > - Minor formatting issues > > .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 153 > + > 1 file changed, 153 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > > diff --git a/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > new file mode 100644 > index ..3f2bf64b65c0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > @@ -0,0 +1,153 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/brcm,asp-v2.0.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom ASP 2.0 Ethernet controller > + > +maintainers: > + - Justin Chen > + - Florian Fainelli > + > +description: Broadcom Ethernet controller first introduced with 72165 > + > +properties: > + compatible: > +oneOf: > + - items: > + - enum: > + - brcm,bcm74165-asp > + - const: brcm,asp-v2.1 > + - items: > + - enum: > + - brcm,bcm72165-asp > + - const: brcm,asp-v2.0 > + > + "#address-cells": > +const: 1 > + "#size-cells": > +const: 1 > + > + reg: > +maxItems: 1 > + > + ranges: true > + > + interrupts: > +minItems: 1 > +items: > + - description: RX/TX interrupt > + - description: Port 0 Wake-on-LAN > + - description: Port 1 Wake-on-LAN > + > + clocks: > +maxItems: 1 > + > + ethernet-ports: > +type: object > +properties: > + "#address-cells": > +const: 1 > + "#size-cells": > +const: 0 > + > +patternProperties: > + "^port@[0-9]+$": > +type: object > + > +$ref: ethernet-controller.yaml# unevaluatedProperties: false > + > +properties: > + reg: > +maxItems: 1 > +description: Port number > + > + brcm,channel: > +$ref: /schemas/types.yaml#/definitions/uint32 > +description: | > + ASP Channel Number > + > + The depacketizer channel that consumes packets from > + the unimac/port. > + > +required: > + - reg > + - brcm,channel > + > +additionalProperties: false
Re: [PATCH 23/30] fbdev/tdfxfb: Set i2c adapter parent to hardware device
Thomas Zimmermann writes: > Use the 3dfx hardware device from the Linux device hierarchy as > parent device of the i2c adapter. Aligns the driver with the rest > of the codebase and prepares fbdev for making struct fb_info.dev > optional. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 22/30] fbdev/smscufx: Detect registered fb_info from refcount
Thomas Zimmermann writes: > Detect registered instances of fb_info by reading the reference > counter from struct fb_info.read. Avoids looking at the dev field > and prepares fbdev for making struct fb_info.dev optional. > > Signed-off-by: Thomas Zimmermann > Cc: Steve Glendinning > --- > drivers/video/fbdev/smscufx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c > index 17cec62cc65d..adb2b1fe8383 100644 > --- a/drivers/video/fbdev/smscufx.c > +++ b/drivers/video/fbdev/smscufx.c > @@ -1496,7 +1496,7 @@ static int ufx_setup_modes(struct ufx_data *dev, struct > fb_info *info, > u8 *edid; > int i, result = 0, tries = 3; > > - if (info->dev) /* only use mutex if info has been registered */ > + if (refcount_read(>count)) /* only use mutex if info has been > registered */ The struct fb_info .count refcount is protected by the registration_lock mutex in register_framebuffer(). I think this operation isn't thread safe ? But that was also the case for info->dev check, so I guess is OK and this change is for an old fbdev driver anyways. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
On 08/06/2023 00:05, Abhinav Kumar wrote: On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote: Only several SSPP blocks support such features as YUV output or scaling, thus different DRM planes have different features. Properly utilizing all planes requires the attention of the compositor, who should prefer simpler planes to YUV-supporting ones. Otherwise it is very easy to end up in a situation when all featureful planes are already allocated for simple windows, leaving no spare plane for YUV playback. To solve this problem make all planes virtual. Each plane is registered as if it supports all possible features, but then at the runtime during the atomic_check phase the driver selects backing SSPP block for each plane. Note, this does not provide support for using two different SSPP blocks for a single plane or using two rectangles of an SSPP to drive two planes. Each plane still gets its own SSPP and can utilize either a solo rectangle or both multirect rectangles depending on the resolution. Note #2: By default support for virtual planes is turned off and the driver still uses old code path with preallocated SSPP block for each plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' kernel parameter. Signed-off-by: Dmitry Baryshkov --- There will be some rebase needed to switch back to encoder based allocation so I am not going to comment on those parts and will let you handle that when you post v3. But my questions/comments below are for other things. drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 65 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 +++ 7 files changed, 413 insertions(+), 70 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 8ef191fd002d..cdece21b81c9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc, struct drm_crtc_stat return 0; } +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) +{ + struct dpu_global_state *global_state; + struct drm_plane *plane; + int rc; + + global_state = dpu_kms_get_global_state(crtc_state->state); + if (IS_ERR(global_state)) + return PTR_ERR(global_state); + + dpu_rm_release_all_sspp(global_state, crtc); + + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { + rc = dpu_plane_virtual_assign_resources(plane, crtc, + global_state, + crtc_state->state); + if (rc) + return rc; + } + + return 0; +} + static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) { @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); - const struct drm_plane_state *pstate; struct drm_plane *plane; int rc = 0; @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, return rc; } + if (dpu_use_virtual_planes && + crtc_state->planes_changed) { + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); + if (rc < 0) + return rc; + } Can you please explain a bit more about the planes_changed condition? 1) Are we doing this because the plane's atomic check happens before the CRTC atomic check? Yes. Another alternative would be to stop using drm_atomic_helper_check() and implement our own code instead, inverting the plane / CRTC check order. 2) So the DRM core sets this to true already when plane is switching CRTCs or being connected to a CRTC for the first time, we need to handle the conditions additional to that right? Nit: it is not possible to switch CRTCs. Plane first has to be disconnected and then to be connected to another CRTC. 3) If (2) is correct, arent there other conditions then to be handled for setting planes_changed to true? Some examples include, switching from a scaling to non-scaling scenario, needing rotation vs not needing etc. Setting the plane_changed is not required. Both drm_atomic_helper_disable_plane() and drm_atomic_helper_update_plane() will add the plane to the state. Then drm_atomic_helper_check_planes() will call drm_atomic_helper_plane_changed(), setting {old_,new_}crtc_state->planes_changed. I should check if the format check below is required or not. It looks like I can drop that clause too. Basically it seems like all of this
[PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels
Let's provide the proper link from the touchscreen to the panel on trogdor devices where the touchscreen support it. This allows the OS to power sequence the touchscreen more properly. For the most part, this is just expected to marginally improve power consumption while the screen is off. However, in at least one trogdor model (wormdingler) it's suspected that this will fix some behavorial corner cases when the panel power cycles (like for a modeset) without the touchscreen power cycling. NOTE: some trogdor variants use touchscreens that don't (yet) support linking the touchscreen and the panel. Those variants are left alone. Signed-off-by: Douglas Anderson --- (no changes since v1) arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi| 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi| 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 + 6 files changed, 6 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi index 8b8ea8af165d..b4f328d3e1f6 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi @@ -104,6 +104,7 @@ ap_ts: touchscreen@5d { interrupt-parent = <>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; + panel = <>; reset-gpios = < 8 GPIO_ACTIVE_LOW>; vdd-supply = <_ts>; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi index b3ba23a88a0b..88aeb415bd5b 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi @@ -116,6 +116,7 @@ ap_ts: touchscreen@14 { interrupt-parent = <>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; + panel = <>; reset-gpios = < 8 GPIO_ACTIVE_LOW>; vdd-supply = <_touch>; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi index 269007d73162..c65f18ea3e5c 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi @@ -43,6 +43,7 @@ ap_ts: touchscreen@10 { interrupt-parent = <>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; + panel = <>; post-power-on-delay-ms = <20>; hid-descr-addr = <0x0001>; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi index 6c5287bd27d6..d2aafd1ea672 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi @@ -102,6 +102,7 @@ ap_ts: touchscreen@10 { interrupt-parent = <>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; + panel = <>; post-power-on-delay-ms = <20>; hid-descr-addr = <0x0001>; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi index 8e7b42f843d4..0785873d1345 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi @@ -99,6 +99,7 @@ ap_ts: touchscreen@10 { interrupt-parent = <>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; + panel = <>; post-power-on-delay-ms = <20>; hid-descr-addr = <0x0001>; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi index 262d6691abd9..f70f5b42c845 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi @@ -154,6 +154,7 @@ ap_ts: touchscreen@1 { interrupt-parent = <>; interrupts = <9 IRQ_TYPE_EDGE_FALLING>; + panel = <>; post-power-on-delay-ms = <70>; hid-descr-addr = <0x0001>; -- 2.41.0.162.gfafddb0af9-goog
[PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
As talked about in the patch ("drm/panel: Add a way for other devices to follow panel state"), we really want to keep the power states of a touchscreen and the panel it's attached to in sync with each other. In that spirit, add support to i2c-hid to be a panel follower. This will let the i2c-hid driver get informed when the panel is powered on and off. From there we can match the i2c-hid device's power state to that of the panel. NOTE: this patch specifically _doesn't_ use pm_runtime to keep track of / manage the power state of the i2c-hid device, even though my first instinct said that would be the way to go. Specific problems with using pm_runtime(): * The initial power up couldn't happen in a runtime resume function since it create sub-devices and, apparently, that's not good to do in your resume function. * Managing our power state with pm_runtime meant fighting to make the right thing happen at system suspend to prevent the system from trying to resume us only to suspend us again. While this might be able to be solved, it added complexity. Overall the code without pm_runtime() ended up being smaller and easier to understand. Signed-off-by: Douglas Anderson --- Changes in v2: - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static. drivers/hid/i2c-hid/i2c-hid-core.c | 82 +- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index fa8a1ca43d7f..368db3ae612f 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -38,6 +38,8 @@ #include #include +#include + #include "../hid-ids.h" #include "i2c-hid.h" @@ -107,6 +109,8 @@ struct i2c_hid { struct mutexreset_lock; struct i2chid_ops *ops; + struct drm_panel_follower panel_follower; + boolis_panel_follower; }; static const struct i2c_hid_quirks { @@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid) return ret; } +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower) +{ + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); + struct hid_device *hid = ihid->hid; + + /* +* hid->version is set on the first power up. If it's still zero then +* this is the first power on so we should perform initial power up +* steps. +*/ + if (!hid->version) + return i2c_hid_core_initial_power_up(ihid); + + return i2c_hid_core_resume(ihid); +} + +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower) +{ + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); + + return i2c_hid_core_suspend(ihid); +} + +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = { + .panel_prepared = i2c_hid_core_panel_prepared, + .panel_unpreparing = i2c_hid_core_panel_unpreparing, +}; + int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, u16 hid_descriptor_address, u32 quirks) { @@ -1119,6 +1151,41 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, hid->bus = BUS_I2C; hid->initial_quirks = quirks; + /* +* See if we're following a panel. If drm_panel_add_follower() +* returns no error then we are. +*/ + ihid->panel_follower.funcs = _hid_core_panel_follower_funcs; + ret = drm_panel_add_follower(>dev, >panel_follower); + if (!ret) { + /* We're a follower. That means we'll power things up later. */ + ihid->is_panel_follower = true; + + /* +* If we're not in control of our own power up/power down then +* we can't do the logic to manage wakeups. Give a warning if +* a user thought that was possible then force the capability +* off. +*/ + if (device_can_wakeup(>dev)) { + dev_warn(>dev, "Can't wakeup if following panel\n"); + device_set_wakeup_capable(>dev, false); + } + + return 0; + } + + /* +* -ENODEV means that we're not following a panel, so any other error +* is a real problem (like -EPROBE_DEFER, -ENOMEM, ...). +*/ + if (ret != -ENODEV) + goto err_mem_free; + + /* +* We're not following a panel. That's fine and means that we +* can power up right away. +*/ ret = i2c_hid_core_initial_power_up(ihid); if (ret) goto err_mem_free; @@ -1143,7 +1210,14 @@ void i2c_hid_core_remove(struct i2c_client *client) struct i2c_hid *ihid = i2c_get_clientdata(client); struct hid_device *hid; -
[PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq
Turning on an i2c-hid device can be a slow process. This is why i2c-hid devices use PROBE_PREFER_ASYNCHRONOUS. Unfortunately, when we're a panel follower the i2c-hid power up sequence now blocks the power on of the panel. Let's fix that by scheduling the work on the system_wq. Signed-off-by: Douglas Anderson --- Changes in v2: - ihid_core_panel_prepare_work() is now static. - Improve documentation for smp_wmb(). drivers/hid/i2c-hid/i2c-hid-core.c | 50 +++--- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 368db3ae612f..de1a0624be08 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -110,7 +110,9 @@ struct i2c_hid { struct i2chid_ops *ops; struct drm_panel_follower panel_follower; + struct work_struct panel_follower_prepare_work; boolis_panel_follower; + boolprepare_work_finished; }; static const struct i2c_hid_quirks { @@ -1062,10 +1064,12 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid) return ret; } -static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower) +static void ihid_core_panel_prepare_work(struct work_struct *work) { - struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); + struct i2c_hid *ihid = container_of(work, struct i2c_hid, + panel_follower_prepare_work); struct hid_device *hid = ihid->hid; + int ret; /* * hid->version is set on the first power up. If it's still zero then @@ -1073,15 +1077,52 @@ static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower) * steps. */ if (!hid->version) - return i2c_hid_core_initial_power_up(ihid); + ret = i2c_hid_core_initial_power_up(ihid); + else + ret = i2c_hid_core_resume(ihid); - return i2c_hid_core_resume(ihid); + if (ret) + dev_warn(>client->dev, "Power on failed: %d\n", ret); + else + WRITE_ONCE(ihid->prepare_work_finished, true); + + /* +* The work APIs provide a number of memory ordering guarantees +* including one that says that memory writes before schedule_work() +* are always visible to the work function, but they don't appear to +* guarantee that a write that happened in the work is visible after +* cancel_work_sync(). We'll add a write memory barrier here to match +* with i2c_hid_core_panel_unpreparing() to ensure that our write to +* prepare_work_finished is visible there. +*/ + smp_wmb(); +} + +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower) +{ + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); + + /* +* Powering on a touchscreen can be a slow process. Queue the work to +* the system workqueue so we don't block the panel's power up. +*/ + WRITE_ONCE(ihid->prepare_work_finished, false); + schedule_work(>panel_follower_prepare_work); + + return 0; } static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower) { struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); + cancel_work_sync(>panel_follower_prepare_work); + + /* Match with ihid_core_panel_prepare_work() */ + smp_rmb(); + if (!READ_ONCE(ihid->prepare_work_finished)) + return 0; + return i2c_hid_core_suspend(ihid); } @@ -1124,6 +1165,7 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, init_waitqueue_head(>wait); mutex_init(>reset_lock); + INIT_WORK(>panel_follower_prepare_work, ihid_core_panel_prepare_work); /* we need to allocate the command buffer without knowing the maximum * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the -- 2.41.0.162.gfafddb0af9-goog
[PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later
In a future patch, we want to change i2c-hid not to necessarily power up the touchscreen during probe. In preparation for that, rearrange the probe function so that we put as much stuff _before_ powering up the device as possible. This change is expected to have no functional effect. Signed-off-by: Douglas Anderson --- Changes in v2: - i2c_hid_core_initial_power_up() is now static. drivers/hid/i2c-hid/i2c-hid-core.c | 124 ++--- 1 file changed, 77 insertions(+), 47 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 19d985c20a5c..d29e6421ecba 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -855,7 +855,8 @@ static int i2c_hid_init_irq(struct i2c_client *client) irqflags = IRQF_TRIGGER_LOW; ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq, - irqflags | IRQF_ONESHOT, client->name, ihid); + irqflags | IRQF_ONESHOT | IRQF_NO_AUTOEN, + client->name, ihid); if (ret < 0) { dev_warn(>dev, "Could not register for %s interrupt, irq = %d," @@ -940,6 +941,72 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid) ihid->ops->shutdown_tail(ihid->ops); } +/** + * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device. + * @ihid: The ihid object created during probe. + * + * This function is called at probe time. + * + * The initial power on is where we do some basic validation that the device + * exists, where we fetch the HID descriptor, and where we create the actual + * HID devices. + * + * Return: 0 or error code. + */ +static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid) +{ + struct i2c_client *client = ihid->client; + struct hid_device *hid = ihid->hid; + int ret; + + ret = i2c_hid_core_power_up(ihid); + if (ret) + return ret; + + /* Make sure there is something at this address */ + ret = i2c_smbus_read_byte(client); + if (ret < 0) { + i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret); + ret = -ENXIO; + goto err; + } + + ret = i2c_hid_fetch_hid_descriptor(ihid); + if (ret < 0) { + dev_err(>dev, + "Failed to fetch the HID Descriptor\n"); + goto err; + } + + enable_irq(client->irq); + + hid->version = le16_to_cpu(ihid->hdesc.bcdVersion); + hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID); + hid->product = le16_to_cpu(ihid->hdesc.wProductID); + + hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor, + hid->product); + + snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", +client->name, (u16)hid->vendor, (u16)hid->product); + strscpy(hid->phys, dev_name(>dev), sizeof(hid->phys)); + + ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product); + + ret = hid_add_device(hid); + if (ret) { + if (ret != -ENODEV) + hid_err(client, "can't add hid device: %d\n", ret); + goto err; + } + + return 0; + +err: + i2c_hid_core_power_down(ihid); + return ret; +} + int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, u16 hid_descriptor_address, u32 quirks) { @@ -966,16 +1033,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, if (!ihid) return -ENOMEM; - ihid->ops = ops; - - ret = i2c_hid_core_power_up(ihid); - if (ret) - return ret; - i2c_set_clientdata(client, ihid); + ihid->ops = ops; ihid->client = client; - ihid->wHIDDescRegister = cpu_to_le16(hid_descriptor_address); init_waitqueue_head(>wait); @@ -986,28 +1047,12 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, * real computation later. */ ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE); if (ret < 0) - goto err_powered; - + return ret; device_enable_async_suspend(>dev); - /* Make sure there is something at this address */ - ret = i2c_smbus_read_byte(client); - if (ret < 0) { - i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret); - ret = -ENXIO; - goto err_powered; - } - - ret = i2c_hid_fetch_hid_descriptor(ihid); - if (ret < 0) { - dev_err(>dev, - "Failed to fetch the HID Descriptor\n"); - goto err_powered; - } - ret = i2c_hid_init_irq(client); if (ret < 0) - goto err_powered; + goto
[PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions
In a future patch we'd like to be able to call the current i2c-hid suspend and resume functions from times other than system suspend. Move the functions higher up in the file and have them take a "struct i2c_hid" to make this simpler. We'll then add tiny wrappers of the functions for use with system suspend. This change is expected to have no functional effect. Signed-off-by: Douglas Anderson --- (no changes since v1) drivers/hid/i2c-hid/i2c-hid-core.c | 98 +- 1 file changed, 56 insertions(+), 42 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index d29e6421ecba..fa8a1ca43d7f 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -941,6 +941,57 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid) ihid->ops->shutdown_tail(ihid->ops); } +static int i2c_hid_core_suspend(struct i2c_hid *ihid) +{ + struct i2c_client *client = ihid->client; + struct hid_device *hid = ihid->hid; + int ret; + + ret = hid_driver_suspend(hid, PMSG_SUSPEND); + if (ret < 0) + return ret; + + /* Save some power */ + i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); + + disable_irq(client->irq); + + if (!device_may_wakeup(>dev)) + i2c_hid_core_power_down(ihid); + + return 0; +} + +static int i2c_hid_core_resume(struct i2c_hid *ihid) +{ + struct i2c_client *client = ihid->client; + struct hid_device *hid = ihid->hid; + int ret; + + if (!device_may_wakeup(>dev)) + i2c_hid_core_power_up(ihid); + + enable_irq(client->irq); + + /* Instead of resetting device, simply powers the device on. This +* solves "incomplete reports" on Raydium devices 2386:3118 and +* 2386:4B33 and fixes various SIS touchscreens no longer sending +* data after a suspend/resume. +* +* However some ALPS touchpads generate IRQ storm without reset, so +* let's still reset them here. +*/ + if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) + ret = i2c_hid_hwreset(ihid); + else + ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); + + if (ret) + return ret; + + return hid_driver_reset_resume(hid); +} + /** * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device. * @ihid: The ihid object created during probe. @@ -1115,61 +1166,24 @@ void i2c_hid_core_shutdown(struct i2c_client *client) } EXPORT_SYMBOL_GPL(i2c_hid_core_shutdown); -static int i2c_hid_core_suspend(struct device *dev) +static int i2c_hid_core_pm_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct i2c_hid *ihid = i2c_get_clientdata(client); - struct hid_device *hid = ihid->hid; - int ret; - - ret = hid_driver_suspend(hid, PMSG_SUSPEND); - if (ret < 0) - return ret; - /* Save some power */ - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); - - disable_irq(client->irq); - - if (!device_may_wakeup(>dev)) - i2c_hid_core_power_down(ihid); - - return 0; + return i2c_hid_core_suspend(ihid); } -static int i2c_hid_core_resume(struct device *dev) +static int i2c_hid_core_pm_resume(struct device *dev) { - int ret; struct i2c_client *client = to_i2c_client(dev); struct i2c_hid *ihid = i2c_get_clientdata(client); - struct hid_device *hid = ihid->hid; - if (!device_may_wakeup(>dev)) - i2c_hid_core_power_up(ihid); - - enable_irq(client->irq); - - /* Instead of resetting device, simply powers the device on. This -* solves "incomplete reports" on Raydium devices 2386:3118 and -* 2386:4B33 and fixes various SIS touchscreens no longer sending -* data after a suspend/resume. -* -* However some ALPS touchpads generate IRQ storm without reset, so -* let's still reset them here. -*/ - if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) - ret = i2c_hid_hwreset(ihid); - else - ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); - - if (ret) - return ret; - - return hid_driver_reset_resume(hid); + return i2c_hid_core_resume(ihid); } const struct dev_pm_ops i2c_hid_core_pm = { - SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume) + SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume) }; EXPORT_SYMBOL_GPL(i2c_hid_core_pm); -- 2.41.0.162.gfafddb0af9-goog
[PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
The SYSTEM_SLEEP_PM_OPS() allows us to get rid of '#ifdef CONFIG_PM_SLEEP', as talked about in commit 1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old ones"). This change is expected to have no functional effect. Signed-off-by: Douglas Anderson --- (no changes since v1) drivers/hid/i2c-hid/i2c-hid-core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index efbba0465eef..19d985c20a5c 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -1085,7 +1085,6 @@ void i2c_hid_core_shutdown(struct i2c_client *client) } EXPORT_SYMBOL_GPL(i2c_hid_core_shutdown); -#ifdef CONFIG_PM_SLEEP static int i2c_hid_core_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -1138,10 +1137,9 @@ static int i2c_hid_core_resume(struct device *dev) return hid_driver_reset_resume(hid); } -#endif const struct dev_pm_ops i2c_hid_core_pm = { - SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume) + SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume) }; EXPORT_SYMBOL_GPL(i2c_hid_core_pm); -- 2.41.0.162.gfafddb0af9-goog
[PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers
Inform fw_devlink of the fact that a panel follower (like a touchscreen) is effectively a consumer of the panel from the purposes of fw_devlink. NOTE: this patch isn't required for correctness but instead optimizes probe order / helps avoid deferrals. Signed-off-by: Douglas Anderson --- Since this is so small, I'd presume it's OK for it to go through a DRM tree with the proper Ack. That being said, this patch is just an optimization and thus it could land completely separately from the rest and they could all meet up in mainline. Changes in v2: - ("Add a devlink for panel followers") new for v2. drivers/of/property.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/of/property.c b/drivers/of/property.c index ddc75cd50825..cf8dacf3e3b8 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1266,6 +1266,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells") DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells") DEFINE_SIMPLE_PROP(leds, "leds", NULL) DEFINE_SIMPLE_PROP(backlight, "backlight", NULL) +DEFINE_SIMPLE_PROP(panel, "panel", NULL) DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") @@ -1354,6 +1355,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_resets, }, { .parse_prop = parse_leds, }, { .parse_prop = parse_backlight, }, + { .parse_prop = parse_panel, }, { .parse_prop = parse_gpio_compat, }, { .parse_prop = parse_interrupts, }, { .parse_prop = parse_regulators, }, -- 2.41.0.162.gfafddb0af9-goog
[PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state
These days, it's fairly common to see panels that have touchscreens attached to them. The panel and the touchscreen can somewhat be thought of as totally separate devices and, historically, this is how Linux has treated them. However, treating them as separate isn't necessarily the best way to model the two devices, it was just that there was no better way. Specifically, there is little practical reason to have the touchscreen powered on when the panel is turned off, but if we model the devices separately we have no way to keep the two devices' power states in sync with each other. The issue described above makes it sound as if the problem here is just about efficiency. We're wasting power keeping the touchscreen powered up when the screen is off. While that's true, the problem can go deeper. Specifically, hardware designers see that there's no reason to have the touchscreen on while the screen is off and then build hardware assuming that software would never turn the touchscreen on while the screen is off. In the very simplest case of hardware designs like this, the touchscreen and the panel share some power rails. In most cases, this turns out not to be terrible and is, again, just a little less efficient. Specifically if we tell Linux that the touchscreen and the panel are using the same rails then Linux will keep the rails on when _either_ device is turned on. That ends to work OK-ish, but now if you turn the panel off not only will the touchscreen remain powered, but the power rails for the panel itself won't be switched off, burning extra power. The above two inefficiencies are _extra_ minor when you consider the fact that laptops rarely spend much time with the screen off. The main use case would be when an external screen (and presumably a power supply) is attached. Unfortunately, it gets worse from here. On sc7180-trogdor-homestar, for instance, the display's TCON (timing controller) sometimes crashes if you don't power cycle it whenever you stop and restart the video stream (like during a modeset). The touchscreen keeping the power rails on causes real problems. One proposal in the homestar timeframe was to move the touchscreen to an always-on rail, dedicating the main power rail to the panel. That caused _different_ problems as talked about in commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to the regulator"). The end result of all of this was to add an extra regulator to the board, increasing cost. Recently, Cong Yang posted a patch [1] where things are even worse. The panel and touch controller on that system seem even more intimately tied together and really can't be thought of separately. To address this issue, let's start allowing devices to register themselves as "panel followers". These devices will get called after a panel has been powered on and before a panel is powered off. This makes the panel the primary device in charge of the power state, which matches how userspace uses it. The panel follower API should be fairly straightforward to use. The current code assumes that panel followers are using device tree and have a "panel" property pointing to the panel to follow. More flexibility and non-DT implementations could be added as needed. Right now, panel followers can follow the prepare/unprepare functions. There could be arguments made that, instead, they should follow enable/disable. I've chosen prepare/unprepare for now since those functions are guaranteed to power up/power down the panel and it seems better to start the process earlier. A bit of explaining about why this is a roll-your-own API instead of using something more standard: 1. In standard APIs in Linux, parent devices are automatically powered on when a child needs power. Applying that here, it would mean that we'd force the panel on any time someone was listening to the touchscreen. That, unfortunately, would have broken homestar's need (if we hadn't changed the hardware, as per above) where the panel absolutely needs to be able to power cycle itself. While one could argue that homestar is broken hardware and we shouldn't have the API do backflips for it, _officially_ the eDP timing guidelines agree with homestar's needs and the panel power sequencing diagrams show power going off. It's nice to be able to support this. 2. We could, conceibably, try to add a new flag to device_link causing the parent to be in charge of power. Then we could at least use normal pm_runtime APIs. This sounds great, except that we run into problems with initial probe. As talked about in the later patch ("HID: i2c-hid: Support being a panel follower") the initial power on of a panel follower might need to do things (like add sub-devices) that aren't allowed in a runtime_resume function. The above complexities explain why this API isn't using common functions. That being said, this patch is very small and self-contained, so if someone was later able to adapt it to
[PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel
In a whole pile of panel drivers, we have code to make the prepare/unprepare/enable/disable callbacks behave as no-ops if they've already been called. It's silly to have this code duplicated everywhere. Add it to the core instead so that we can eventually delete it from all the drivers. Note: to get some idea of the duplicated code, try: git grep 'if.*>prepared' -- drivers/gpu/drm/panel git grep 'if.*>enabled' -- drivers/gpu/drm/panel NOTE: arguably, the right thing to do here is actually to skip this patch and simply remove all the extra checks from the individual drivers. Perhaps the checks were needed at some point in time in the past but maybe they no longer are? Certainly as we continue transitioning over to "panel_bridge" then we expect there to be much less variety in how these calls are made. When we're called as part of the bridge chain, things should be pretty simple. In fact, there was some discussion in the past about these checks [1], including a discussion about whether the checks were needed and whether the calls ought to be refcounted. At the time, I decided not to mess with it because it felt too risky. Looking closer at it now, I'm fairly certain that nothing in the existing codebase is expecting these calls to be refcounted. The only real question is whether someone is already doing something to ensure prepare()/unprepare() match and enabled()/disable() match. I would say that, even if there is something else ensuring that things match, there's enough complexity that adding an extra bool and an extra double-check here is a good idea. Let's add a drm_warn() to let people know that it's considered a minor error to take advantage of drm_panel's double-checking but we'll still make things work fine. [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid Acked-by: Neil Armstrong Signed-off-by: Douglas Anderson --- This has Neil's Ack and I could commit it to drm-misc-next, but for now I'm holding off to see where this series ends up. If the series ends up looking good we'll have to coordinate landing the various bits between the drm and the hid trees and the second drm patch in my series depends on this one. If my series implodes I'll land this one on its own. In any case, once this lands somewhere I'll take an AI to cleanup the panels. (no changes since v1) drivers/gpu/drm/drm_panel.c | 49 - include/drm/drm_panel.h | 14 +++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index f634371c717a..4e1c4e42575b 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove); */ int drm_panel_prepare(struct drm_panel *panel) { + int ret; + if (!panel) return -EINVAL; - if (panel->funcs && panel->funcs->prepare) - return panel->funcs->prepare(panel); + if (panel->prepared) { + dev_warn(panel->dev, "Skipping prepare of already prepared panel\n"); + return 0; + } + + if (panel->funcs && panel->funcs->prepare) { + ret = panel->funcs->prepare(panel); + if (ret < 0) + return ret; + } + panel->prepared = true; return 0; } @@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_unprepare(struct drm_panel *panel) { + int ret; + if (!panel) return -EINVAL; - if (panel->funcs && panel->funcs->unprepare) - return panel->funcs->unprepare(panel); + if (!panel->prepared) { + dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); + return 0; + } + + if (panel->funcs && panel->funcs->unprepare) { + ret = panel->funcs->unprepare(panel); + if (ret < 0) + return ret; + } + panel->prepared = false; return 0; } @@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel) if (!panel) return -EINVAL; + if (panel->enabled) { + dev_warn(panel->dev, "Skipping enable of already enabled panel\n"); + return 0; + } + if (panel->funcs && panel->funcs->enable) { ret = panel->funcs->enable(panel); if (ret < 0) return ret; } + panel->enabled = true; ret = backlight_enable(panel->backlight); if (ret < 0) @@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel) if (!panel) return -EINVAL; + if (!panel->enabled) { + dev_warn(panel->dev, "Skipping disable of already disabled panel\n"); + return 0; + } + ret = backlight_disable(panel->backlight); if
[PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens
As talked about in the patch ("drm/panel: Add a way for other devices to follow panel state"), touchscreens that are connected to panels are generally expected to be power sequenced together with the panel they're attached to. Today, nothing provides information allowing you to find out that a touchscreen is connected to a panel. Let's add a phandle for this. The proerty is added to the generic touchscreen bindings and then enabled in the bindings for the i2c-hid backed devices. This can and should be added for other touchscreens in the future, but for now let's start small. Signed-off-by: Douglas Anderson --- Changes in v2: - Move the description to the generic touchscreen.yaml. - Update the desc to make it clearer it's only for integrated devices. Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 5 + .../devicetree/bindings/input/goodix,gt7375p.yaml | 5 + Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 2 ++ .../devicetree/bindings/input/touchscreen/touchscreen.yaml | 7 +++ 4 files changed, 19 insertions(+) diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml index 05e6f2df604c..3e2d216c6432 100644 --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml @@ -13,6 +13,9 @@ description: Supports the Elan eKTH6915 touchscreen controller. This touchscreen controller uses the i2c-hid protocol with a reset GPIO. +allOf: + - $ref: /schemas/input/touchscreen/touchscreen.yaml# + properties: compatible: items: @@ -24,6 +27,8 @@ properties: interrupts: maxItems: 1 + panel: true + reset-gpios: description: Reset GPIO; not all touchscreens using eKTH6915 hook this up. diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml index ce18d7dadae2..72212382d702 100644 --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml @@ -14,6 +14,9 @@ description: This touchscreen uses the i2c-hid protocol but has some non-standard power sequencing required. +allOf: + - $ref: /schemas/input/touchscreen/touchscreen.yaml# + properties: compatible: oneOf: @@ -30,6 +33,8 @@ properties: interrupts: maxItems: 1 + panel: true + reset-gpios: true diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml index 7156b08f7645..138caad96a29 100644 --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml @@ -44,6 +44,8 @@ properties: description: HID descriptor address $ref: /schemas/types.yaml#/definitions/uint32 + panel: true + post-power-on-delay-ms: description: Time required by the device after enabling its regulators or powering it on, before it is ready for communication. diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml index 895592da9626..431c13335c40 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml @@ -10,6 +10,13 @@ maintainers: - Dmitry Torokhov properties: + panel: +description: If this touchscreen is integrally connected to a panel, this + is a reference to that panel. The presence of this reference indicates + that the touchscreen should be power sequenced together with the panel + and that they may share power and/or reset signals. +$ref: /schemas/types.yaml#/definitions/phandle + touchscreen-min-x: description: minimum x coordinate reported $ref: /schemas/types.yaml#/definitions/uint32 -- 2.41.0.162.gfafddb0af9-goog
[PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
The big motivation for this patch series is mostly described in the patch ("drm/panel: Add a way for other devices to follow panel state"), but to quickly summarize here: for touchscreens that are connected to a panel we need the ability to power sequence the two device together. This is not a new need, but so far we've managed to get by through a combination of inefficiency, added costs, or perhaps just a little bit of brokenness. It's time to do better. This patch series allows us to do better. Assuming that people think this patch series looks OK, we'll have to figure out the right way to land it. The panel patches and i2c-hid patches will go through very different trees and so either we'll need an Ack from one side or the other or someone to create a tag for the other tree to pull in. This will _probably_ require the true drm-misc maintainers to get involved, not a lowly committer. ;-) Version 2 of this patch series doesn't change too much. At a high level: * I added all the forgotten "static" to functions. * I've hopefully made the bindings better. * I've integrated into fw_devlink. * I cleaned up a few descriptions / comments. This still needs someone to say that the idea looks OK or to suggest an alternative that solves the problems. ;-) Changes in v2: - Move the description to the generic touchscreen.yaml. - Update the desc to make it clearer it's only for integrated devices. - Add even more text to the commit message. - A few comment cleanups. - ("Add a devlink for panel followers") new for v2. - i2c_hid_core_initial_power_up() is now static. - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static. - ihid_core_panel_prepare_work() is now static. - Improve documentation for smp_wmb(). Douglas Anderson (10): dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens drm/panel: Check for already prepared/enabled in drm_panel drm/panel: Add a way for other devices to follow panel state of: property: fw_devlink: Add a devlink for panel followers HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() HID: i2c-hid: Rearrange probe() to power things up later HID: i2c-hid: Make suspend and resume into helper functions HID: i2c-hid: Support being a panel follower HID: i2c-hid: Do panel follower work on the system_wq arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels .../bindings/input/elan,ekth6915.yaml | 5 + .../bindings/input/goodix,gt7375p.yaml| 5 + .../bindings/input/hid-over-i2c.yaml | 2 + .../input/touchscreen/touchscreen.yaml| 7 + .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 + .../dts/qcom/sc7180-trogdor-homestar.dtsi | 1 + .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 + .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 + .../qcom/sc7180-trogdor-quackingstick.dtsi| 1 + .../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 + drivers/gpu/drm/drm_panel.c | 196 +- drivers/hid/i2c-hid/i2c-hid-core.c| 338 +- drivers/of/property.c | 2 + include/drm/drm_panel.h | 89 + 14 files changed, 555 insertions(+), 95 deletions(-) -- 2.41.0.162.gfafddb0af9-goog
[pull] amdgpu, radeon drm-fixes-6.4
Hi Dave, Daniel, Fixes for 6.4. The following changes since commit 9561de3a55bed6bdd44a12820ba81ec416e705a7: Linux 6.4-rc5 (2023-06-04 14:04:27 -0400) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-fixes-6.4-2023-06-07 for you to fetch changes up to e1a600208286c197c2696e51fc313e49889315bd: drm/amd/display: Reduce sdp bw after urgent to 90% (2023-06-07 17:02:25 -0400) amd-drm-fixes-6.4-2023-06-07: amdgpu: - S0ix fixes - GPU reset fixes - SMU13 fixes - SMU11 fixes - Misc Display fixes - Revert RV/RV2/PCO clock counter changes - Fix Stoney xclk value - Fix reserved vram debug info radeon: - Fix a potential use after free Alex Deucher (3): Revert "drm/amdgpu: change the reference clock for raven/raven2" Revert "drm/amdgpu: Differentiate between Raven2 and Raven/Picasso according to revision id" Revert "drm/amdgpu: switch to golden tsc registers for raven/raven2" Alvin Lee (1): drm/amd/display: Reduce sdp bw after urgent to 90% Chia-I Wu (1): drm/amdgpu: fix xclk freq on CHIP_STONEY Evan Quan (1): drm/amd/pm: conditionally disable pcie lane switching for some sienna_cichlid SKUs Horatio Zhang (1): drm/amdgpu: fix Null pointer dereference error in amdgpu_device_recover_vram Lijo Lazar (1): drm/amd/pm: Fix power context allocation in SMU13 Mario Limonciello (2): drm/amd: Disallow s0ix without BIOS support again drm/amd: Make lack of `ACPI_FADT_LOW_POWER_S0` or `CONFIG_AMD_PMC` louder during suspend path Min Li (1): drm/radeon: fix race condition UAF in radeon_gem_set_domain_ioctl Samson Tam (1): drm/amd/display: add ODM case when looking for first split pipe YiPeng Chai (1): drm/amdgpu: change reserved vram info print drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 12 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 10 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 7 +- drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 35 drivers/gpu/drm/amd/amdgpu/soc15.c | 7 +- drivers/gpu/drm/amd/amdgpu/vi.c| 11 ++- drivers/gpu/drm/amd/display/dc/core/dc.c | 36 - drivers/gpu/drm/amd/display/dc/core/dc_resource.c | 20 + .../gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c | 2 +- .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c| 92 +- drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 4 +- drivers/gpu/drm/radeon/radeon_gem.c| 4 +- 13 files changed, 162 insertions(+), 79 deletions(-)
Re: [PATCH v3] drm/i915/mtl/gsc: Add a gsc_info debugfs
On Mon, 2023-06-05 at 21:32 -0700, Ceraolo Spurio, Daniele wrote: > Add a new debugfs to dump information about the GSC. This includes: > > - the FW path and SW tracking status; > - the release, security and compatibility versions; > - the HECI1 status registers. > > Note that those are the same registers that the mei driver dumps in > their own status sysfs on DG2 (where mei owns the GSC). > alan:snip all looks good. (ofc we do have to follow up on those 2 actions from last rev's conversation (that we agreed should be separate patch). For now, this patch is Reviewed-by: Alan Previn
RE: [PATCH v3] drm/i915: Fix a VMA UAF for multi-gt platform
Thank you, Nirmoy. We have verified it on Chrome. Tested-by: Sushma Venkatesh Reddy -Original Message- From: Das, Nirmoy Sent: Wednesday, June 7, 2023 1:11 AM To: intel-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org; Das, Nirmoy ; Joonas Lahtinen ; Vivi, Rodrigo ; Tvrtko Ursulin ; Thomas Hellström ; Wilson, Chris P ; Andi Shyti ; Hajda, Andrzej ; Venkatesh Reddy, Sushma Subject: [PATCH v3] drm/i915: Fix a VMA UAF for multi-gt platform Ensure correct handling of closed VMAs on multi-gt platforms to prevent Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are exclusively added to GT0's closed_vma link (gt->closed_vma) and subsequently freed by i915_vma_parked(), which assumes the entire GPU is idle. However, on platforms with multiple GTs, such as MTL, GT1 may remain active while GT0 is idle. This causes GT0 to mistakenly consider the closed VMAs in its closed_vma list as unnecessary, potentially leading to Use-After-Free issues if a job for GT1 attempts to access a freed VMA. Although we do take a wakeref for GT0 but it happens later, after evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref early. v2: Use gt id to detect multi-tile(Andi) Fix the incorrect error path. v3: Add more comment(Andi) Use the new gt var when possible(Andrzej) Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Tvrtko Ursulin Cc: Thomas Hellström Cc: Chris Wilson Cc: Andi Shyti Cc: Andrzej Hajda Cc: Sushma Venkatesh Reddy Signed-off-by: Nirmoy Das Tested-by: Andi Shyti Reviewed-by: Andi Shyti Reviewed-by: Andrzej Hajda --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 21 +-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 5fb459ea4294..1de9de1e4782 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2692,6 +2692,7 @@ static int eb_select_engine(struct i915_execbuffer *eb) { struct intel_context *ce, *child; + struct intel_gt *gt; unsigned int idx; int err; @@ -2715,10 +2716,17 @@ eb_select_engine(struct i915_execbuffer *eb) } } eb->num_batches = ce->parallel.number_children + 1; + gt = ce->engine->gt; for_each_child(ce, child) intel_context_get(child); - intel_gt_pm_get(ce->engine->gt); + intel_gt_pm_get(gt); + /* +* Keep GT0 active on MTL so that i915_vma_parked() doesn't +* free VMAs while execbuf ioctl is validating VMAs. +*/ + if (gt->info.id) + intel_gt_pm_get(to_gt(gt->i915)); if (!test_bit(CONTEXT_ALLOC_BIT, >flags)) { err = intel_context_alloc_state(ce); @@ -2757,7 +2765,10 @@ eb_select_engine(struct i915_execbuffer *eb) return err; err: - intel_gt_pm_put(ce->engine->gt); + if (gt->info.id) + intel_gt_pm_put(to_gt(gt->i915)); + + intel_gt_pm_put(gt); for_each_child(ce, child) intel_context_put(child); intel_context_put(ce); @@ -2770,6 +2781,12 @@ eb_put_engine(struct i915_execbuffer *eb) struct intel_context *child; i915_vm_put(eb->context->vm); + /* +* This works in conjunction with eb_select_engine() to prevent +* i915_vma_parked() from interfering while execbuf validates vmas. +*/ + if (eb->gt->info.id) + intel_gt_pm_put(to_gt(eb->gt->i915)); intel_gt_pm_put(eb->gt); for_each_child(eb->context, child) intel_context_put(child); -- 2.39.0
Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote: Only several SSPP blocks support such features as YUV output or scaling, thus different DRM planes have different features. Properly utilizing all planes requires the attention of the compositor, who should prefer simpler planes to YUV-supporting ones. Otherwise it is very easy to end up in a situation when all featureful planes are already allocated for simple windows, leaving no spare plane for YUV playback. To solve this problem make all planes virtual. Each plane is registered as if it supports all possible features, but then at the runtime during the atomic_check phase the driver selects backing SSPP block for each plane. Note, this does not provide support for using two different SSPP blocks for a single plane or using two rectangles of an SSPP to drive two planes. Each plane still gets its own SSPP and can utilize either a solo rectangle or both multirect rectangles depending on the resolution. Note #2: By default support for virtual planes is turned off and the driver still uses old code path with preallocated SSPP block for each plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' kernel parameter. Signed-off-by: Dmitry Baryshkov --- There will be some rebase needed to switch back to encoder based allocation so I am not going to comment on those parts and will let you handle that when you post v3. But my questions/comments below are for other things. drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 65 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h| 24 +++ 7 files changed, 413 insertions(+), 70 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 8ef191fd002d..cdece21b81c9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc, struct drm_crtc_stat return 0; } +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) +{ + struct dpu_global_state *global_state; + struct drm_plane *plane; + int rc; + + global_state = dpu_kms_get_global_state(crtc_state->state); + if (IS_ERR(global_state)) + return PTR_ERR(global_state); + + dpu_rm_release_all_sspp(global_state, crtc); + + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { + rc = dpu_plane_virtual_assign_resources(plane, crtc, + global_state, + crtc_state->state); + if (rc) + return rc; + } + + return 0; +} + static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) { @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); - const struct drm_plane_state *pstate; struct drm_plane *plane; int rc = 0; @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, return rc; } + if (dpu_use_virtual_planes && + crtc_state->planes_changed) { + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); + if (rc < 0) + return rc; + } Can you please explain a bit more about the planes_changed condition? 1) Are we doing this because the plane's atomic check happens before the CRTC atomic check? 2) So the DRM core sets this to true already when plane is switching CRTCs or being connected to a CRTC for the first time, we need to handle the conditions additional to that right? 3) If (2) is correct, arent there other conditions then to be handled for setting planes_changed to true? Some examples include, switching from a scaling to non-scaling scenario, needing rotation vs not needing etc. Basically it seems like all of this is handled within the reserve_sspp() function but if planes_changes is not set then that wont get invoked at all. + if (!crtc_state->enable || !crtc_state->active) { DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n", crtc->base.id, crtc_state->enable, @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, if (cstate->num_mixers) _dpu_crtc_setup_lm_bounds(crtc, crtc_state); - /* FIXME: move this to
Re: [Freedreno] [PATCH v3 6/7] drm/msm/dsi: Add phy configuration for MSM8226
On Mittwoch, 7. Juni 2023 21:46:31 CEST Jeykumar Sankaran wrote: > On 6/1/2023 10:00 AM, Luca Weiss wrote: > > MSM8226 uses a modified PLL lock sequence compared to MSM8974, which is > > based on the function dsi_pll_enable_seq_m in the msm-3.10 kernel. > > > > Worth noting that the msm-3.10 downstream kernel also will try other > > sequences in case this one doesn't work, but during testing it has shown > > > > that the _m sequence succeeds first time also: > >.pll_enable_seqs[0] = dsi_pll_enable_seq_m, > >.pll_enable_seqs[1] = dsi_pll_enable_seq_m, > >.pll_enable_seqs[2] = dsi_pll_enable_seq_d, > >.pll_enable_seqs[3] = dsi_pll_enable_seq_d, > >.pll_enable_seqs[4] = dsi_pll_enable_seq_f1, > >.pll_enable_seqs[5] = dsi_pll_enable_seq_c, > >.pll_enable_seqs[6] = dsi_pll_enable_seq_e, > > > > We may need to expand this in the future. > > > > Signed-off-by: Luca Weiss > > --- > > > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 + > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 3 +- > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 97 > > ++ 3 files changed, 101 insertions(+), 1 > > deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c index bb09cbe8ff86..9d5795c58a98 > > 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c > > @@ -541,6 +541,8 @@ static const struct of_device_id dsi_phy_dt_match[] = > > { > > > > .data = _phy_28nm_hpm_famb_cfgs }, > > > > { .compatible = "qcom,dsi-phy-28nm-lp", > > > > .data = _phy_28nm_lp_cfgs }, > > > > + { .compatible = "qcom,dsi-phy-28nm-8226", > > + .data = _phy_28nm_8226_cfgs }, > > > > #endif > > #ifdef CONFIG_DRM_MSM_DSI_20NM_PHY > > > > { .compatible = "qcom,dsi-phy-20nm", > > > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h index 7137a17ae523..8b640d174785 > > 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > @@ -46,8 +46,9 @@ struct msm_dsi_phy_cfg { > > > > extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs; > > > > -extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs; > > +extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8226_cfgs; > > > > extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs; > > > > +extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs; > > > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_2290_cfgs; > > > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c index > > 4c1bf55c5f38..ceec7bb87bf1 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c > > @@ -37,6 +37,7 @@ > > > > /* v2.0.0 28nm LP implementation */ > > #define DSI_PHY_28NM_QUIRK_PHY_LP BIT(0) > > > > +#define DSI_PHY_28NM_QUIRK_PHY_8226BIT(1) > > > > #define LPFR_LUT_SIZE 10 > > struct lpfr_cfg { > > > > @@ -377,6 +378,74 @@ static int dsi_pll_28nm_vco_prepare_hpm(struct clk_hw > > *hw)> > > return ret; > > > > } > > > > +static int dsi_pll_28nm_vco_prepare_8226(struct clk_hw *hw) > > +{ > > + struct dsi_pll_28nm *pll_28nm = to_pll_28nm(hw); > > + struct device *dev = _28nm->phy->pdev->dev; > > + void __iomem *base = pll_28nm->phy->pll_base; > > + u32 max_reads = 5, timeout_us = 100; > > + bool locked; > > + u32 val; > > + int i; > > + > > + DBG("id=%d", pll_28nm->phy->id); > > + > > + pll_28nm_software_reset(pll_28nm); > > + > > + /* > > +* PLL power up sequence. > > +* Add necessary delays recommended by hardware. > > +*/ > > + dsi_phy_write(base + REG_DSI_28nm_PHY_PLL_CAL_CFG1, 0x34); > > + > > + val = DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRDN_B; > > + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 200); > > + > > + val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRGEN_PWRDN_B; > > + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 200); > > + > > + val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_LDO_PWRDN_B; > > + val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_ENABLE; > > + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 600); > > + > > + for (i = 0; i < 7; i++) { > > + /* DSI Uniphy lock detect setting */ > > + dsi_phy_write(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2, 0x0d); > > + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2, > > + 0x0c, 100); > > + dsi_phy_write(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2, 0x0d); > > + > > + /* poll for PLL ready status */ > >
Re: [PATCH] mm: fix hugetlb page unmap count balance issue
On Wed, 7 Jun 2023 13:53:10 -0700 Mike Kravetz wrote: > > > > BUGs aren't good. Can we please find a way to push this along? > > > > Have we heard anything from any udmabuf people? > > > > I have not heard anything. When this issue popped up, it took me by surprise. > > udmabuf maintainer (Gerd Hoffmann), the people who added hugetlb support and > the list where udmabuf was developed (dri-devel@lists.freedesktop.org) have > been on cc. Maybe Greg can suggest a way forward. > My 'gut reaction' would be to remove hugetlb support from udmabuf. From a > quick look, if we really want this support then there will need to be some > API changes. For example UDMABUF_CREATE should be hugetlb page aligned > and a multiple of hugetlb page size if using a hugetlb mapping. > > It would be good to know about users of the driver. So disabling "hugetlb=on" (and adding an explanatory printk) would suffice for now?
Re: [PATCH 29/30] fbdev/core: Rework fb init code
Hi Thomas. On Mon, Jun 05, 2023 at 04:48:11PM +0200, Thomas Zimmermann wrote: > Init the class "graphics" before the rest of fbdev. Later steps, such > as the sysfs code, depend on the class. Also arrange the module's exit > code in reverse order. > > Unexport the global variable fb_class, which is only shared internally > within the fbdev core module. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Sam Ravnborg > --- > drivers/video/fbdev/core/fb_internal.h | 1 + > drivers/video/fbdev/core/fbcon.c | 1 + > drivers/video/fbdev/core/fbmem.c | 52 ++ > include/linux/fb.h | 1 - > 4 files changed, 22 insertions(+), 33 deletions(-) > > diff --git a/drivers/video/fbdev/core/fb_internal.h > b/drivers/video/fbdev/core/fb_internal.h > index abe06c9da36e..0b43c0cd5096 100644 > --- a/drivers/video/fbdev/core/fb_internal.h > +++ b/drivers/video/fbdev/core/fb_internal.h > @@ -11,6 +11,7 @@ int fb_register_chrdev(void); > void fb_unregister_chrdev(void); > > /* fbmem.c */ > +extern struct class *fb_class; > extern struct mutex registration_lock; > extern struct fb_info *registered_fb[FB_MAX]; > extern int num_registered_fb; > diff --git a/drivers/video/fbdev/core/fbcon.c > b/drivers/video/fbdev/core/fbcon.c > index c6c9d040bdec..8e76bc246b38 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -78,6 +78,7 @@ > #include > > #include "fbcon.h" > +#include "fb_internal.h" > > /* > * FIXME: Locking > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index 2d26ac46337b..b21b66017c01 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -45,6 +45,8 @@ > > #define FBPIXMAPSIZE (1024 * 8) > > +struct class *fb_class; > + > DEFINE_MUTEX(registration_lock); > struct fb_info *registered_fb[FB_MAX] __read_mostly; > int num_registered_fb __read_mostly; > @@ -897,9 +899,6 @@ fb_blank(struct fb_info *info, int blank) > } > EXPORT_SYMBOL(fb_blank); > > -struct class *fb_class; > -EXPORT_SYMBOL(fb_class); > - > static int fb_check_foreignness(struct fb_info *fi) > { > const bool foreign_endian = fi->flags & FBINFO_FOREIGN_ENDIAN; > @@ -1106,59 +1105,48 @@ void fb_set_suspend(struct fb_info *info, int state) > } > EXPORT_SYMBOL(fb_set_suspend); > > -/** > - * fbmem_init - init frame buffer subsystem > - * > - * Initialize the frame buffer subsystem. > - * > - * NOTE: This function is _only_ to be called by drivers/char/mem.c. > - * > - */ > - > -static int __init > -fbmem_init(void) > +static int __init fbmem_init(void) > { > int ret; > > + fb_class = class_create("graphics"); > + if (IS_ERR(fb_class)) { > + ret = PTR_ERR(fb_class); > + pr_err("Unable to create fb class; errno = %d\n", ret); > + goto err_fb_class; > + } > + > ret = fb_init_procfs(); > if (ret) > - return ret; > + goto err_class_destroy; > > ret = fb_register_chrdev(); > if (ret) > - goto err_chrdev; > - > - fb_class = class_create("graphics"); > - if (IS_ERR(fb_class)) { > - ret = PTR_ERR(fb_class); > - pr_warn("Unable to create fb class; errno = %d\n", ret); > - fb_class = NULL; > - goto err_class; > - } > + goto err_fb_cleanup_procfs; > > fb_console_init(); > > return 0; > > -err_class: > - fb_unregister_chrdev(); > -err_chrdev: > +err_fb_cleanup_procfs: > fb_cleanup_procfs(); > +err_class_destroy: > + class_destroy(fb_class); > +err_fb_class: > + fb_class = NULL; > return ret; > } > > #ifdef MODULE > -module_init(fbmem_init); > -static void __exit > -fbmem_exit(void) > +static void __exit fbmem_exit(void) > { > fb_console_exit(); > - > + fb_unregister_chrdev(); > fb_cleanup_procfs(); > class_destroy(fb_class); > - fb_unregister_chrdev(); > } > > +module_init(fbmem_init); > module_exit(fbmem_exit); > MODULE_LICENSE("GPL"); > MODULE_DESCRIPTION("Framebuffer base"); > diff --git a/include/linux/fb.h b/include/linux/fb.h > index 1988d11f78bc..541a0e3ce21f 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -609,7 +609,6 @@ extern int fb_new_modelist(struct fb_info *info); > > extern bool fb_center_logo; > extern int fb_logo_count; > -extern struct class *fb_class; > > static inline void lock_fb_info(struct fb_info *info) > { > -- > 2.40.1
Re: [PATCH 28/30] fbdev/core: Move file-I/O code into separate file
Hi Thomas. On Mon, Jun 05, 2023 at 04:48:10PM +0200, Thomas Zimmermann wrote: > Move fbdev's file-I/O code into a separate file and contain it in > init and cleanup helpers. No functional changes. > > Signed-off-by: Thomas Zimmermann Consider moving the two helps as noted below. With or without this move: Reviewed-by: Sam Ravnborg > --- > drivers/video/fbdev/core/Makefile | 1 + > drivers/video/fbdev/core/fb_devfs.c| 484 + > drivers/video/fbdev/core/fb_internal.h | 6 + > drivers/video/fbdev/core/fbmem.c | 478 +--- > 4 files changed, 497 insertions(+), 472 deletions(-) > create mode 100644 drivers/video/fbdev/core/fb_devfs.c > > diff --git a/drivers/video/fbdev/core/Makefile > b/drivers/video/fbdev/core/Makefile > index 665320160f70..125d24f50c36 100644 > --- a/drivers/video/fbdev/core/Makefile > +++ b/drivers/video/fbdev/core/Makefile > @@ -2,6 +2,7 @@ > obj-$(CONFIG_FB_NOTIFY) += fb_notify.o > obj-$(CONFIG_FB) += fb.o > fb-y := fb_backlight.o \ > + fb_devfs.o \ > fb_info.o \ > fb_procfs.o \ > fbmem.o fbmon.o fbcmap.o fbsysfs.o \ > diff --git a/drivers/video/fbdev/core/fb_devfs.c > b/drivers/video/fbdev/core/fb_devfs.c > new file mode 100644 > index ..5ab16cb24524 > --- /dev/null > +++ b/drivers/video/fbdev/core/fb_devfs.c devfs gives me another understanding of what this file is used for. fb_ioctl.c? > @@ -0,0 +1,484 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include > +#include > +#include > +#include > + > +#include "fb_internal.h" > + > +/* > + * We hold a reference to the fb_info in file->private_data, > + * but if the current registered fb has changed, we don't > + * actually want to use it. > + * > + * So look up the fb_info using the inode minor number, > + * and just verify it against the reference we have. > + */ > +static struct fb_info *file_fb_info(struct file *file) > +{ > + struct inode *inode = file_inode(file); > + int fbidx = iminor(inode); > + struct fb_info *info = registered_fb[fbidx]; > + > + if (info != file->private_data) > + info = NULL; > + return info; > +} > + > +static ssize_t fb_read(struct file *file, char __user *buf, size_t count, > loff_t *ppos) > +{ > + struct fb_info *info = file_fb_info(file); > + > + if (!info) > + return -ENODEV; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + if (info->fbops->fb_read) > + return info->fbops->fb_read(info, buf, count, ppos); > + > + return fb_io_read(info, buf, count, ppos); > +} > + > +static ssize_t fb_write(struct file *file, const char __user *buf, size_t > count, loff_t *ppos) > +{ > + struct fb_info *info = file_fb_info(file); > + > + if (!info) > + return -ENODEV; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + if (info->fbops->fb_write) > + return info->fbops->fb_write(info, buf, count, ppos); > + > + return fb_io_write(info, buf, count, ppos); > +} > + > +static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > + unsigned long arg) > +{ > + const struct fb_ops *fb; > + struct fb_var_screeninfo var; > + struct fb_fix_screeninfo fix; > + struct fb_cmap cmap_from; > + struct fb_cmap_user cmap; > + void __user *argp = (void __user *)arg; > + long ret = 0; > + > + switch (cmd) { > + case FBIOGET_VSCREENINFO: > + lock_fb_info(info); > + var = info->var; > + unlock_fb_info(info); > + > + ret = copy_to_user(argp, , sizeof(var)) ? -EFAULT : 0; > + break; > + case FBIOPUT_VSCREENINFO: > + if (copy_from_user(, argp, sizeof(var))) > + return -EFAULT; > + /* only for kernel-internal use */ > + var.activate &= ~FB_ACTIVATE_KD_TEXT; > + console_lock(); > + lock_fb_info(info); > + ret = fbcon_modechange_possible(info, ); > + if (!ret) > + ret = fb_set_var(info, ); > + if (!ret) > + fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL); > + unlock_fb_info(info); > + console_unlock(); > + if (!ret && copy_to_user(argp, , sizeof(var))) > + ret = -EFAULT; > + break; > + case FBIOGET_FSCREENINFO: > + lock_fb_info(info); > + memcpy(, >fix, sizeof(fix)); > + if (info->flags & FBINFO_HIDE_SMEM_START) > + fix.smem_start = 0; > + unlock_fb_info(info); > + > + ret = copy_to_user(argp, , sizeof(fix)) ? -EFAULT : 0; > +
Re: [PATCH v1 2/8] dt-bindings: display: panel: mipi-dbi-spi: add spi-3wire property
On 6/7/23 13:55, Leonard Göhrs wrote: > Some MIPI DBI panels support a three wire mode (clock, chip select, > bidirectional data) that can be used to ask the panel if it is already set > up by e.g. the bootloader and can thus skip the initialization. > This enables a flicker-free boot. > > Signed-off-by: Leonard Göhrs > --- > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > index c07da1a9e6288..2f0238b770eba 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > @@ -87,6 +87,8 @@ properties: >Logic level supply for interface signals (Vddi). >No need to set if this is the same as power-supply. > > + spi-3wire: true > + I don't think this should be added here. spi-cpha and spi-cpol are also supported but they are not mentioned. Instead those are documented in bindings/spi/spi-controller.yaml. Why they're not documented in bindings/spi/spi-peripheral-props.yaml instead which this binding has a ref to, I have no idea. Noralf. > required: >- compatible >- reg
Re: [PATCH 27/30] fbdev/core: Move procfs code to separate file
Hi Thomas, On Mon, Jun 05, 2023 at 04:48:09PM +0200, Thomas Zimmermann wrote: > Move fbdev's procfs code into a separate file and contain it in > init and cleanup helpers. No functional changes. Maybe add: Delete the unused for_each_registered_fb while touching the code. The change to use proc_remove is not really documented. But code looks ok. I am not fan that we have introduced a few globals again. But it looks like the way to go for now. > > Signed-off-by: Thomas Zimmermann With an improved changelog: Reviewed-by: Sam Ravnborg > --- > drivers/video/fbdev/core/Makefile | 1 + > drivers/video/fbdev/core/fb_internal.h | 12 - > drivers/video/fbdev/core/fb_procfs.c | 62 ++ > drivers/video/fbdev/core/fbmem.c | 51 +++-- > 4 files changed, 80 insertions(+), 46 deletions(-) > create mode 100644 drivers/video/fbdev/core/fb_procfs.c > > diff --git a/drivers/video/fbdev/core/Makefile > b/drivers/video/fbdev/core/Makefile > index eee3295bc225..665320160f70 100644 > --- a/drivers/video/fbdev/core/Makefile > +++ b/drivers/video/fbdev/core/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_FB_NOTIFY) += fb_notify.o > obj-$(CONFIG_FB) += fb.o > fb-y := fb_backlight.o \ > fb_info.o \ > + fb_procfs.o \ > fbmem.o fbmon.o fbcmap.o fbsysfs.o \ > modedb.o fbcvt.o fb_cmdline.o > fb_io_fops.o > fb-$(CONFIG_FB_DEFERRED_IO) += fb_defio.o > diff --git a/drivers/video/fbdev/core/fb_internal.h > b/drivers/video/fbdev/core/fb_internal.h > index 0b9640ae7a3d..51f7c9f04e45 100644 > --- a/drivers/video/fbdev/core/fb_internal.h > +++ b/drivers/video/fbdev/core/fb_internal.h > @@ -3,7 +3,17 @@ > #ifndef _FB_INTERNAL_H > #define _FB_INTERNAL_H > > -struct fb_info; > +#include > +#include > + > +/* fbmem.c */ > +extern struct mutex registration_lock; > +extern struct fb_info *registered_fb[FB_MAX]; > +extern int num_registered_fb; > + > +/* fb_procfs.c */ > +int fb_init_procfs(void); > +void fb_cleanup_procfs(void); > > /* fbsysfs.c */ > int fb_device_create(struct fb_info *fb_info); > diff --git a/drivers/video/fbdev/core/fb_procfs.c > b/drivers/video/fbdev/core/fb_procfs.c > new file mode 100644 > index ..59641142f8aa > --- /dev/null > +++ b/drivers/video/fbdev/core/fb_procfs.c > @@ -0,0 +1,62 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include > + > +#include "fb_internal.h" > + > +static struct proc_dir_entry *fb_proc_dir_entry; > + > +static void *fb_seq_start(struct seq_file *m, loff_t *pos) > +{ > + mutex_lock(_lock); > + > + return (*pos < FB_MAX) ? pos : NULL; > +} > + > +static void fb_seq_stop(struct seq_file *m, void *v) > +{ > + mutex_unlock(_lock); > +} > + > +static void *fb_seq_next(struct seq_file *m, void *v, loff_t *pos) > +{ > + (*pos)++; > + > + return (*pos < FB_MAX) ? pos : NULL; > +} > + > +static int fb_seq_show(struct seq_file *m, void *v) > +{ > + int i = *(loff_t *)v; > + struct fb_info *fi = registered_fb[i]; > + > + if (fi) > + seq_printf(m, "%d %s\n", fi->node, fi->fix.id); > + > + return 0; > +} > + > +static const struct seq_operations __maybe_unused fb_proc_seq_ops = { > + .start = fb_seq_start, > + .stop = fb_seq_stop, > + .next = fb_seq_next, > + .show = fb_seq_show, > +}; > + > +int fb_init_procfs(void) > +{ > + struct proc_dir_entry *proc; > + > + proc = proc_create_seq("fb", 0, NULL, _proc_seq_ops); > + if (!proc) > + return -ENOMEM; > + > + fb_proc_dir_entry = proc; > + > + return 0; > +} > + > +void fb_cleanup_procfs(void) > +{ > + proc_remove(fb_proc_dir_entry); > +} > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index 66532774d351..de1e45240161 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -24,9 +24,7 @@ > #include > #include > #include > -#include > #include > -#include > #include > #include > #include > @@ -48,13 +46,9 @@ > > #define FBPIXMAPSIZE (1024 * 8) > > -static DEFINE_MUTEX(registration_lock); > - > +DEFINE_MUTEX(registration_lock); > struct fb_info *registered_fb[FB_MAX] __read_mostly; > int num_registered_fb __read_mostly; > -#define for_each_registered_fb(i)\ > - for (i = 0; i < FB_MAX; i++)\ > - if (!registered_fb[i]) {} else > > bool fb_center_logo __read_mostly; > > @@ -705,40 +699,6 @@ int fb_show_logo(struct fb_info *info, int rotate) { > return 0; } > EXPORT_SYMBOL(fb_prepare_logo); > EXPORT_SYMBOL(fb_show_logo); > > -static void *fb_seq_start(struct seq_file *m, loff_t *pos) > -{ > - mutex_lock(_lock); > - return (*pos < FB_MAX) ? pos : NULL; > -} > - > -static void *fb_seq_next(struct seq_file *m, void
Re: [PATCH drm-next v4 04/14] drm: debugfs: provide infrastructure to dump a DRM GPU VA space
Hi Danilo, kernel test robot noticed the following build warnings: [auto build test WARNING on 33a86170888b7e4aa0cea94ebb9c67180139cea9] url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230607-063442 base: 33a86170888b7e4aa0cea94ebb9c67180139cea9 patch link:https://lore.kernel.org/r/20230606223130.6132-5-dakr%40redhat.com patch subject: [PATCH drm-next v4 04/14] drm: debugfs: provide infrastructure to dump a DRM GPU VA space config: i386-randconfig-s001-20230607 (https://download.01.org/0day-ci/archive/20230608/202306080453.w2dcrevr-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/513e00e3cf4889b115cd8ab122b8e51adf2805ab git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230607-063442 git checkout 513e00e3cf4889b115cd8ab122b8e51adf2805ab # save the config file mkdir build_dir && cp config build_dir/.config make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/ If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202306080453.w2dcrevr-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/gpu/drm/drm_debugfs.c:213:33: sparse: sparse: non size-preserving >> pointer to integer cast vim +213 drivers/gpu/drm/drm_debugfs.c 178 179 /** 180 * drm_debugfs_gpuva_info - dump the given DRM GPU VA space 181 * @m: pointer to the _file to write 182 * @mgr: the _gpuva_manager representing the GPU VA space 183 * 184 * Dumps the GPU VA mappings of a given DRM GPU VA manager. 185 * 186 * For each DRM GPU VA space drivers should call this function from their 187 * _info_list's show callback. 188 * 189 * Returns: 0 on success, -ENODEV if the is not initialized 190 */ 191 int drm_debugfs_gpuva_info(struct seq_file *m, 192 struct drm_gpuva_manager *mgr) 193 { 194 DRM_GPUVA_ITER(it, mgr, 0); 195 struct drm_gpuva *va, *kva = >kernel_alloc_node; 196 197 if (!mgr->name) 198 return -ENODEV; 199 200 seq_printf(m, "DRM GPU VA space (%s) [0x%016llx;0x%016llx]\n", 201 mgr->name, mgr->mm_start, mgr->mm_start + mgr->mm_range); 202 seq_printf(m, "Kernel reserved node [0x%016llx;0x%016llx]\n", 203 kva->va.addr, kva->va.addr + kva->va.range); 204 seq_puts(m, "\n"); 205 seq_puts(m, " VAs | start | range | end| object | object offset\n"); 206 seq_puts(m, "-\n"); 207 drm_gpuva_iter_for_each(va, it) { 208 if (unlikely(va == >kernel_alloc_node)) 209 continue; 210 211 seq_printf(m, " | 0x%016llx | 0x%016llx | 0x%016llx | 0x%016llx | 0x%016llx\n", 212 va->va.addr, va->va.range, va->va.addr + va->va.range, > 213 (u64)va->gem.obj, va->gem.offset); 214 } 215 216 return 0; 217 } 218 EXPORT_SYMBOL(drm_debugfs_gpuva_info); 219 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [Intel-gfx] [PATCH] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset
On 6/7/2023 12:03, Zhanjun Dong wrote: This attempts to avoid circular locking dependency between flush delayed work and intel_gt_reset. Switched from cancel_delayed_work_sync to cancel_delayed_work, the non-sync version for reset path, it is safe as the worker has the trylock code to handle the lock; Meanwhile keep the sync version for park/fini to ensure the worker is not still running during suspend or shutdown. WARNING: possible circular locking dependency detected 6.4.0-rc1-drmtip_1340-g31e3463b0edb+ #1 Not tainted -- kms_pipe_crc_ba/6415 is trying to acquire lock: 88813e6cc640 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}, at: __flush_work+0x42/0x530 but task is already holding lock: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: intel_gt_reset+0x19e/0x470 [i915] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (>reset.mutex){+.+.}-{3:3}: lock_acquire+0xd8/0x2d0 i915_gem_shrinker_taints_mutex+0x31/0x50 [i915] intel_gt_init_reset+0x65/0x80 [i915] intel_gt_common_init_early+0xe1/0x170 [i915] intel_root_gt_init_early+0x48/0x60 [i915] i915_driver_probe+0x671/0xcb0 [i915] i915_pci_probe+0xdc/0x210 [i915] pci_device_probe+0x95/0x120 really_probe+0x164/0x3c0 __driver_probe_device+0x73/0x160 driver_probe_device+0x19/0xa0 __driver_attach+0xb6/0x180 bus_for_each_dev+0x77/0xd0 bus_add_driver+0x114/0x210 driver_register+0x5b/0x110 __pfx_vgem_open+0x3/0x10 [vgem] do_one_initcall+0x57/0x270 do_init_module+0x5f/0x220 load_module+0x1ca4/0x1f00 __do_sys_finit_module+0xb4/0x130 do_syscall_64+0x3c/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc -> #2 (fs_reclaim){+.+.}-{0:0}: lock_acquire+0xd8/0x2d0 fs_reclaim_acquire+0xac/0xe0 kmem_cache_alloc+0x32/0x260 i915_vma_instance+0xb2/0xc60 [i915] i915_gem_object_ggtt_pin_ww+0x175/0x370 [i915] vm_fault_gtt+0x22d/0xf60 [i915] __do_fault+0x2f/0x1d0 do_pte_missing+0x4a/0xd20 __handle_mm_fault+0x5b0/0x790 handle_mm_fault+0xa2/0x230 do_user_addr_fault+0x3ea/0xa10 exc_page_fault+0x68/0x1a0 asm_exc_page_fault+0x26/0x30 -> #1 (>reset.backoff_srcu){}-{0:0}: lock_acquire+0xd8/0x2d0 _intel_gt_reset_lock+0x57/0x330 [i915] guc_timestamp_ping+0x35/0x130 [i915] process_one_work+0x250/0x510 worker_thread+0x4f/0x3a0 kthread+0xff/0x130 ret_from_fork+0x29/0x50 -> #0 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}: check_prev_add+0x90/0xc60 __lock_acquire+0x1998/0x2590 lock_acquire+0xd8/0x2d0 __flush_work+0x74/0x530 __cancel_work_timer+0x14f/0x1f0 intel_guc_submission_reset_prepare+0x81/0x4b0 [i915] intel_uc_reset_prepare+0x9c/0x120 [i915] reset_prepare+0x21/0x60 [i915] intel_gt_reset+0x1dd/0x470 [i915] intel_gt_reset_global+0xfb/0x170 [i915] intel_gt_handle_error+0x368/0x420 [i915] intel_gt_debugfs_reset_store+0x5c/0xc0 [i915] i915_wedged_set+0x29/0x40 [i915] simple_attr_write_xsigned.constprop.0+0xb4/0x110 full_proxy_write+0x52/0x80 vfs_write+0xc5/0x4f0 ksys_write+0x64/0xe0 do_syscall_64+0x3c/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc other info that might help us debug this: Chain exists of: (work_completion)(&(>timestamp.work)->work) --> fs_reclaim --> >reset.mutex Possible unsafe locking scenario: CPU0CPU1 lock(>reset.mutex); lock(fs_reclaim); lock(>reset.mutex); lock((work_completion)(&(>timestamp.work)->work)); *** DEADLOCK *** 3 locks held by kms_pipe_crc_ba/6415: #0: 888101541430 (sb_writers#15){.+.+}-{0:0}, at: ksys_write+0x64/0xe0 #1: 888136c7eab8 (>mutex){+.+.}-{3:3}, at: simple_attr_write_xsigned.constprop.0+0x47/0x110 #2: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: intel_gt_reset+0x19e/0x470 [i915] Signed-off-by: Zhanjun Dong --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a0e3ef1c65d2..cca6960d3490 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1357,9 +1357,12 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) mod_delayed_work(system_highpri_wq, >timestamp.work, guc->timestamp.ping_delay); } -static void
Re: [Freedreno] [PATCH v3 6/7] drm/msm/dsi: Add phy configuration for MSM8226
On 6/1/2023 10:00 AM, Luca Weiss wrote: MSM8226 uses a modified PLL lock sequence compared to MSM8974, which is based on the function dsi_pll_enable_seq_m in the msm-3.10 kernel. Worth noting that the msm-3.10 downstream kernel also will try other sequences in case this one doesn't work, but during testing it has shown that the _m sequence succeeds first time also: .pll_enable_seqs[0] = dsi_pll_enable_seq_m, .pll_enable_seqs[1] = dsi_pll_enable_seq_m, .pll_enable_seqs[2] = dsi_pll_enable_seq_d, .pll_enable_seqs[3] = dsi_pll_enable_seq_d, .pll_enable_seqs[4] = dsi_pll_enable_seq_f1, .pll_enable_seqs[5] = dsi_pll_enable_seq_c, .pll_enable_seqs[6] = dsi_pll_enable_seq_e, We may need to expand this in the future. Signed-off-by: Luca Weiss --- drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 + drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 3 +- drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 97 ++ 3 files changed, 101 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c index bb09cbe8ff86..9d5795c58a98 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c @@ -541,6 +541,8 @@ static const struct of_device_id dsi_phy_dt_match[] = { .data = _phy_28nm_hpm_famb_cfgs }, { .compatible = "qcom,dsi-phy-28nm-lp", .data = _phy_28nm_lp_cfgs }, + { .compatible = "qcom,dsi-phy-28nm-8226", + .data = _phy_28nm_8226_cfgs }, #endif #ifdef CONFIG_DRM_MSM_DSI_20NM_PHY { .compatible = "qcom,dsi-phy-20nm", diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h index 7137a17ae523..8b640d174785 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h @@ -46,8 +46,9 @@ struct msm_dsi_phy_cfg { extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_famb_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_28nm_lp_cfgs; -extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs; +extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8226_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs; +extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_14nm_2290_cfgs; diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c index 4c1bf55c5f38..ceec7bb87bf1 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c @@ -37,6 +37,7 @@ /* v2.0.0 28nm LP implementation */ #define DSI_PHY_28NM_QUIRK_PHY_LP BIT(0) +#define DSI_PHY_28NM_QUIRK_PHY_8226BIT(1) #define LPFR_LUT_SIZE 10 struct lpfr_cfg { @@ -377,6 +378,74 @@ static int dsi_pll_28nm_vco_prepare_hpm(struct clk_hw *hw) return ret; } +static int dsi_pll_28nm_vco_prepare_8226(struct clk_hw *hw) +{ + struct dsi_pll_28nm *pll_28nm = to_pll_28nm(hw); + struct device *dev = _28nm->phy->pdev->dev; + void __iomem *base = pll_28nm->phy->pll_base; + u32 max_reads = 5, timeout_us = 100; + bool locked; + u32 val; + int i; + + DBG("id=%d", pll_28nm->phy->id); + + pll_28nm_software_reset(pll_28nm); + + /* +* PLL power up sequence. +* Add necessary delays recommended by hardware. +*/ + dsi_phy_write(base + REG_DSI_28nm_PHY_PLL_CAL_CFG1, 0x34); + + val = DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRDN_B; + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 200); + + val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRGEN_PWRDN_B; + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 200); + + val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_LDO_PWRDN_B; + val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_ENABLE; + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 600); + + for (i = 0; i < 7; i++) { + /* DSI Uniphy lock detect setting */ + dsi_phy_write(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2, 0x0d); + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2, + 0x0c, 100); + dsi_phy_write(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2, 0x0d); + + /* poll for PLL ready status */ + locked = pll_28nm_poll_for_ready(pll_28nm, + max_reads, timeout_us); + if (locked) + break; + + pll_28nm_software_reset(pll_28nm); + + /* +* PLL power up sequence. +* Add necessary delays recommended by hardware. +*/ + dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_PWRGEN_CFG, 0x00, 50); +
Re: [PATCH 26/30] fbdev/core: Add fb_device_{create,destroy}()
Hi Thomas, On Mon, Jun 05, 2023 at 04:48:08PM +0200, Thomas Zimmermann wrote: > Move the logic to create and destroy fbdev devices into the new > helpers fb_device_create() and fb_device_destroy(). > > There was a call to fb_cleanup_device() in do_unregister_framebuffer() > that was too late. The device had already been removed at this point. > Move the call into fb_device_destroy(). > > Declare the helpers in the new internal header file fb_internal.h, as s/ / / > they are only used within the fbdev core module. > > Signed-off-by: Thomas Zimmermann > --- > drivers/video/fbdev/core/fb_internal.h | 12 > drivers/video/fbdev/core/fbmem.c | 21 +++--- > drivers/video/fbdev/core/fbsysfs.c | 38 -- > include/linux/fb.h | 3 -- > 4 files changed, 52 insertions(+), 22 deletions(-) > create mode 100644 drivers/video/fbdev/core/fb_internal.h > > diff --git a/drivers/video/fbdev/core/fb_internal.h > b/drivers/video/fbdev/core/fb_internal.h > new file mode 100644 > index ..0b9640ae7a3d > --- /dev/null > +++ b/drivers/video/fbdev/core/fb_internal.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _FB_INTERNAL_H > +#define _FB_INTERNAL_H > + > +struct fb_info; > + > +/* fbsysfs.c */ > +int fb_device_create(struct fb_info *fb_info); > +void fb_device_destroy(struct fb_info *fb_info); > + > +#endif > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index f91ae7d4c94d..66532774d351 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -40,6 +40,8 @@ > #include > #include > > +#include "fb_internal.h" > + > /* > * Frame buffer device initialization and setup routines > */ > @@ -1447,14 +1449,7 @@ static int do_register_framebuffer(struct fb_info > *fb_info) > mutex_init(_info->lock); > mutex_init(_info->mm_lock); > > - fb_info->dev = device_create(fb_class, fb_info->device, > - MKDEV(FB_MAJOR, i), NULL, "fb%d", i); > - if (IS_ERR(fb_info->dev)) { > - /* Not fatal */ > - printk(KERN_WARNING "Unable to create device for framebuffer > %d; errno = %ld\n", i, PTR_ERR(fb_info->dev)); > - fb_info->dev = NULL; > - } else > - fb_init_device(fb_info); > + fb_device_create(fb_info); The return result is ignored here. If we do not need it in later patches then drop the return result. > > if (fb_info->pixmap.addr == NULL) { > fb_info->pixmap.addr = kmalloc(FBPIXMAPSIZE, GFP_KERNEL); > @@ -1515,16 +1510,9 @@ static void unlink_framebuffer(struct fb_info *fb_info) > if (WARN_ON(i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)) > return; > > - if (!fb_info->dev) > - return; > - > - device_destroy(fb_class, MKDEV(FB_MAJOR, i)); > - > + fb_device_destroy(fb_info); > pm_vt_switch_unregister(fb_info->device); > - > unbind_console(fb_info); > - > - fb_info->dev = NULL; > } > > static void do_unregister_framebuffer(struct fb_info *fb_info) > @@ -1539,7 +1527,6 @@ static void do_unregister_framebuffer(struct fb_info > *fb_info) > fb_destroy_modelist(_info->modelist); > registered_fb[fb_info->node] = NULL; > num_registered_fb--; > - fb_cleanup_device(fb_info); > #ifdef CONFIG_GUMSTIX_AM200EPD > { > struct fb_event event; > diff --git a/drivers/video/fbdev/core/fbsysfs.c > b/drivers/video/fbdev/core/fbsysfs.c > index 849073f1ca06..fafe574398b0 100644 > --- a/drivers/video/fbdev/core/fbsysfs.c > +++ b/drivers/video/fbdev/core/fbsysfs.c > @@ -8,6 +8,9 @@ > #include > #include > #include > +#include > + > +#include "fb_internal.h" > > #define FB_SYSFS_FLAG_ATTR 1 > > @@ -435,7 +438,7 @@ static struct device_attribute device_attrs[] = { > #endif > }; > > -int fb_init_device(struct fb_info *fb_info) > +static int fb_init_device(struct fb_info *fb_info) > { > int i, error = 0; > > @@ -459,7 +462,7 @@ int fb_init_device(struct fb_info *fb_info) > return 0; > } > > -void fb_cleanup_device(struct fb_info *fb_info) > +static void fb_cleanup_device(struct fb_info *fb_info) > { > unsigned int i; > > @@ -470,3 +473,34 @@ void fb_cleanup_device(struct fb_info *fb_info) > fb_info->class_flag &= ~FB_SYSFS_FLAG_ATTR; > } > } > + > +int fb_device_create(struct fb_info *fb_info) > +{ > + int node = fb_info->node; > + dev_t devt = MKDEV(FB_MAJOR, node); > + int ret; > + > + fb_info->dev = device_create(fb_class, fb_info->device, devt, NULL, > "fb%d", node); > + if (IS_ERR(fb_info->dev)) { > + /* Not fatal */ > + ret = PTR_ERR(fb_info->dev); This error code is lost as we always return 0. > + pr_warn("Unable to create device for framebuffer %d; error > %d\n", node, ret); > +
Re: [PATCH 25/30] fbdev/core: Move framebuffer and backlight helpers into separate files
Hi Thomas, On Mon, Jun 05, 2023 at 04:48:07PM +0200, Thomas Zimmermann wrote: > Move framebuffer and backlight helpers into separate files. Leave > fbsysfs.c to sysfs-related code. No functional changes. > > The framebuffer helpers are not in fbmem.c because they are under > GPL-2.0-or-later copyright, while fbmem.c is GPL-2.0. > > Signed-off-by: Thomas Zimmermann Some nits that you decide what to do with. Reviewed-by: Sam Ravnborg I do not get why they are moved out in separate files. I guess the picture will materialize later. Sam > --- > drivers/video/fbdev/core/Makefile | 4 +- > drivers/video/fbdev/core/fb_backlight.c | 32 +++ > drivers/video/fbdev/core/fb_info.c | 76 > drivers/video/fbdev/core/fbsysfs.c | 110 +--- > 4 files changed, 112 insertions(+), 110 deletions(-) > create mode 100644 drivers/video/fbdev/core/fb_backlight.c > create mode 100644 drivers/video/fbdev/core/fb_info.c > > diff --git a/drivers/video/fbdev/core/Makefile > b/drivers/video/fbdev/core/Makefile > index 8f0060160ffb..eee3295bc225 100644 > --- a/drivers/video/fbdev/core/Makefile > +++ b/drivers/video/fbdev/core/Makefile > @@ -1,7 +1,9 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_FB_NOTIFY) += fb_notify.o > obj-$(CONFIG_FB) += fb.o > -fb-y := fbmem.o fbmon.o fbcmap.o fbsysfs.o \ > +fb-y := fb_backlight.o \ > + fb_info.o \ > + fbmem.o fbmon.o fbcmap.o fbsysfs.o \ > modedb.o fbcvt.o fb_cmdline.o > fb_io_fops.o There is "+=" that can be used in Makefile, but people prefer '\' - sigh! > fb-$(CONFIG_FB_DEFERRED_IO) += fb_defio.o > > diff --git a/drivers/video/fbdev/core/fb_backlight.c > b/drivers/video/fbdev/core/fb_backlight.c > new file mode 100644 > index ..feffe6c68039 > --- /dev/null > +++ b/drivers/video/fbdev/core/fb_backlight.c > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later Hmm, can we change the license from 2.0 to 2.0-or-later without any concern? I hope so. > + > +#include > +#include #include - to avoid relying on indirect includes? > + > +#if IS_ENABLED(CONFIG_FB_BACKLIGHT) > +/* > + * This function generates a linear backlight curve > + * > + * 0: off > + * 1-7: min > + * 8-127: linear from min to max > + */ > +void fb_bl_default_curve(struct fb_info *fb_info, u8 off, u8 min, u8 max) > +{ > + unsigned int i, flat, count, range = (max - min); > + > + mutex_lock(_info->bl_curve_mutex); > + > + fb_info->bl_curve[0] = off; > + > + for (flat = 1; flat < (FB_BACKLIGHT_LEVELS / 16); ++flat) > + fb_info->bl_curve[flat] = min; > + > + count = FB_BACKLIGHT_LEVELS * 15 / 16; > + for (i = 0; i < count; ++i) > + fb_info->bl_curve[flat + i] = min + (range * (i + 1) / count); > + > + mutex_unlock(_info->bl_curve_mutex); > +} > +EXPORT_SYMBOL_GPL(fb_bl_default_curve); > +#endif > diff --git a/drivers/video/fbdev/core/fb_info.c > b/drivers/video/fbdev/core/fb_info.c > new file mode 100644 > index ..fb5b75009ee7 > --- /dev/null > +++ b/drivers/video/fbdev/core/fb_info.c > @@ -0,0 +1,76 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#include > +#include Same as above, consider including mutex.h Also slab.h > + > +/** > + * framebuffer_alloc - creates a new frame buffer info structure > + * > + * @size: size of driver private data, can be zero > + * @dev: pointer to the device for this fb, this can be NULL > + * > + * Creates a new frame buffer info structure. Also reserves @size bytes > + * for driver private data (info->par). info->par (if any) will be > + * aligned to sizeof(long). > + * > + * Returns the new structure, or NULL if an error occurred. > + * > + */ > +struct fb_info *framebuffer_alloc(size_t size, struct device *dev) > +{ > +#define BYTES_PER_LONG (BITS_PER_LONG/8) > +#define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG)) > + int fb_info_size = sizeof(struct fb_info); > + struct fb_info *info; > + char *p; > + > + if (size) > + fb_info_size += PADDING; > + > + p = kzalloc(fb_info_size + size, GFP_KERNEL); > + > + if (!p) > + return NULL; > + > + info = (struct fb_info *) p; > + > + if (size) > + info->par = p + fb_info_size; > + > + info->device = dev; > + info->fbcon_rotate_hint = -1; > + > +#if IS_ENABLED(CONFIG_FB_BACKLIGHT) > + mutex_init(>bl_curve_mutex); > +#endif > + > + return info; > +#undef PADDING > +#undef BYTES_PER_LONG > +} > +EXPORT_SYMBOL(framebuffer_alloc); > + > +/** > + * framebuffer_release - marks the structure available for freeing > + * > + * @info: frame buffer info structure > + * > + * Drop the reference count of the device embedded in the > + * framebuffer info
Re: [PATCH] mm: fix hugetlb page unmap count balance issue
On 17.05.23 00:34, Mike Kravetz wrote: On 05/15/23 10:04, Mike Kravetz wrote: On 05/12/23 16:29, Mike Kravetz wrote: On 05/12/23 14:26, James Houghton wrote: On Fri, May 12, 2023 at 12:20 AM Junxiao Chang wrote: This alone doesn't fix mapcounting for PTE-mapped HugeTLB pages. You need something like [1]. I can resend it if that's what we should be doing, but this mapcounting scheme doesn't work when the page structs have been freed. It seems like it was a mistake to include support for hugetlb memfds in udmabuf. IIUC, it was added with commit 16c243e99d33 udmabuf: Add support for mapping hugepages (v4). Looks like it was never sent to linux-mm? That is unfortunate as hugetlb vmemmap freeing went in at about the same time. And, as you have noted udmabuf will not work if hugetlb vmemmap freeing is enabled. Sigh! Trying to think of a way forward. -- Mike Kravetz [1]: https://lore.kernel.org/linux-mm/20230306230004.1387007-2-jthough...@google.com/ - James Adding people and list on Cc: involved with commit 16c243e99d33. There are several issues with trying to map tail pages of hugetllb pages not taken into account with udmabuf. James spent quite a bit of time trying to understand and address all the issues with the HGM code. While using the scheme proposed by James, may be an approach to the mapcount issue there are also other issues that need attention. For example, I do not see how the fault code checks the state of the hugetlb page (such as poison) as none of that state is carried in tail pages. The more I think about it, the more I think udmabuf should treat hugetlb pages as hugetlb pages. They should be mapped at the appropriate level in the page table. Of course, this would impose new restrictions on the API (mmap and ioctl) that may break existing users. I have no idea how extensively udmabuf is being used with hugetlb mappings. Verified that using udmabug on a hugetlb mapping with vmemmap optimization will BUG as: [14106.812312] BUG: unable to handle page fault for address: ea000a7c4030 [14106.813704] #PF: supervisor write access in kernel mode [14106.814791] #PF: error_code(0x0003) - permissions violation [14106.815921] PGD 27fff9067 P4D 27fff9067 PUD 27fff8067 PMD 17ec34067 PTE 800285dab021 [14106.818489] Oops: 0003 [#1] PREEMPT SMP PTI [14106.819345] CPU: 2 PID: 2313 Comm: udmabuf Not tainted 6.4.0-rc1-next-20230508+ #44 [14106.820906] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc37 04/01/2014 [14106.822679] RIP: 0010:page_add_file_rmap+0x2e/0x270 I started looking more closely at the driver and I do not fully understand the usage model. I took clues from the selftest and driver. It seems the first step is to create a buffer via the UDMABUF_CREATE ioctl. This will copy 4K pages from the page cache to an array associated with a file. I did note that hugetlb and shm behavior is different here as the driver can not add missing hugetlb pages to the cache as it does with shm. However, what seems more concerning is that there is nothing to prevent the pages from being replaced in the cache before being added to a udmabuf mapping. This means udmabuf mapping and original memfd could be operating on a different set of pages. Is this acceptable, or somehow prevented? In my role, I am more interested in udmabuf handling of hugetlb pages. Trying to use individual 4K pages of hugetlb pages is something that should be avoided here. Would it be acceptable to change code so that only whole hugetlb pages are used by udmabuf? If not, then perhaps the existing hugetlb support can be removed? I'm wondering if that VMA shouldn't be some kind of special mapping (VM_PFNMAP), such that the struct page is entirely ignored? I'm quite confused and concerned when I read that code (what the hell is it doing with shmem/hugetlb pages? why does the mapcount even get adjusted?) This all has a bad smell to it, I hope I'm missing something important ... -- Cheers, David / dhildenb
Re: [PATCH 24/30] fbdev/core: Pass Linux device to pm_vt_switch_*() functions
On Mon, Jun 05, 2023 at 04:48:06PM +0200, Thomas Zimmermann wrote: > Pass the Linux device to pm_vt_switch_*() instead of the virtual > fbdev device. Prepares fbdev for making struct fb_info.dev optional. > > The type of device that is passed to the PM functions does not matter > much. It is only a token within the internal list of known devices. > The PM functions do not refer to any of the device's properties or its > type. > > Signed-off-by: Thomas Zimmermann > Cc: "Rafael J. Wysocki" > Cc: Pavel Machek > Cc: linux...@vger.kernel.org Reviewed-by: Sam Ravnborg > --- > drivers/video/fbdev/core/fbmem.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index 329d16e49a90..f91ae7d4c94d 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1478,9 +1478,9 @@ static int do_register_framebuffer(struct fb_info > *fb_info) > INIT_LIST_HEAD(_info->modelist); > > if (fb_info->skip_vt_switch) > - pm_vt_switch_required(fb_info->dev, false); > + pm_vt_switch_required(fb_info->device, false); > else > - pm_vt_switch_required(fb_info->dev, true); > + pm_vt_switch_required(fb_info->device, true); > > fb_var_to_videomode(, _info->var); > fb_add_videomode(, _info->modelist); > @@ -1520,7 +1520,7 @@ static void unlink_framebuffer(struct fb_info *fb_info) > > device_destroy(fb_class, MKDEV(FB_MAJOR, i)); > > - pm_vt_switch_unregister(fb_info->dev); > + pm_vt_switch_unregister(fb_info->device); > > unbind_console(fb_info); > > -- > 2.40.1
Re: [PATCH v2 1/5] drm/i915/gsc: fixes and updates for GSC memory allocation
On Mon, 2023-06-05 at 19:23 -0700, Ceraolo Spurio, Daniele wrote: > A few fixes/updates are required around the GSC memory allocation and it > is easier to do them all at the same time. The changes are as follows: > > 1 - Switch the memory allocation to stolen memory. We need to avoid > accesses from GSC FW to normal memory after the suspend function has > completed and to do so we can either switch to using stolen or make sure > the GSC is gone to sleep before the end of the suspend function. Given > that the GSC waits for a bit before going idle even if there are no > pending operations, it is easier and quicker to just use stolen memory. > > 2 - Reduce the GSC allocation size to 4MBs, which is the POR requirement. > The 8MBs were needed only for early FW and I had misunderstood that as > being the expected POR size when I sent the original patch. > > 3 - Perma-map the GSC allocation. This isn't required immediately, but it > will be needed later to be able to quickly extract the GSC logs, which are > inside the allocation. Since the mapping code needs to be rewritten due to > switching to stolen, it makes sense to do the switch immediately to avoid > having to change it again later. > > Note that the explicit setting of CACHE_NONE for Wa_22016122933 has been > dropped because that's the default setting for stolen memory on !LLC > platforms. > > v2: only memset the memory we're not overwriting (Alan) > LGTM so .. Reviewed-by: Alan Previn
Re: [PATCH] drm: rcar-du: Replace DRM_INFO() with drm_info()
Quoting Laurent Pinchart (2023-05-30 10:26:39) > drm_info() adds proper context to the kernel log message, as it receives > the drm_device pointer. It is thus preferred over DRM_INFO(). Replace > the latter with the former. > > Signed-off-by: Laurent Pinchart Reviewed-by: Kieran Bingham > --- > drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > index 91095f9deb8b..fe4d3b3c9b0c 100644 > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > @@ -713,7 +713,7 @@ static int rcar_du_probe(struct platform_device *pdev) > if (ret) > goto error; > > - DRM_INFO("Device %s probed\n", dev_name(>dev)); > + drm_info(>ddev, "Device %s probed\n", dev_name(>dev)); > > drm_fbdev_generic_setup(>ddev, 32); > > -- > Regards, > > Laurent Pinchart >
[PATCH] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset
This attempts to avoid circular locking dependency between flush delayed work and intel_gt_reset. Switched from cancel_delayed_work_sync to cancel_delayed_work, the non-sync version for reset path, it is safe as the worker has the trylock code to handle the lock; Meanwhile keep the sync version for park/fini to ensure the worker is not still running during suspend or shutdown. WARNING: possible circular locking dependency detected 6.4.0-rc1-drmtip_1340-g31e3463b0edb+ #1 Not tainted -- kms_pipe_crc_ba/6415 is trying to acquire lock: 88813e6cc640 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}, at: __flush_work+0x42/0x530 but task is already holding lock: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: intel_gt_reset+0x19e/0x470 [i915] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (>reset.mutex){+.+.}-{3:3}: lock_acquire+0xd8/0x2d0 i915_gem_shrinker_taints_mutex+0x31/0x50 [i915] intel_gt_init_reset+0x65/0x80 [i915] intel_gt_common_init_early+0xe1/0x170 [i915] intel_root_gt_init_early+0x48/0x60 [i915] i915_driver_probe+0x671/0xcb0 [i915] i915_pci_probe+0xdc/0x210 [i915] pci_device_probe+0x95/0x120 really_probe+0x164/0x3c0 __driver_probe_device+0x73/0x160 driver_probe_device+0x19/0xa0 __driver_attach+0xb6/0x180 bus_for_each_dev+0x77/0xd0 bus_add_driver+0x114/0x210 driver_register+0x5b/0x110 __pfx_vgem_open+0x3/0x10 [vgem] do_one_initcall+0x57/0x270 do_init_module+0x5f/0x220 load_module+0x1ca4/0x1f00 __do_sys_finit_module+0xb4/0x130 do_syscall_64+0x3c/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc -> #2 (fs_reclaim){+.+.}-{0:0}: lock_acquire+0xd8/0x2d0 fs_reclaim_acquire+0xac/0xe0 kmem_cache_alloc+0x32/0x260 i915_vma_instance+0xb2/0xc60 [i915] i915_gem_object_ggtt_pin_ww+0x175/0x370 [i915] vm_fault_gtt+0x22d/0xf60 [i915] __do_fault+0x2f/0x1d0 do_pte_missing+0x4a/0xd20 __handle_mm_fault+0x5b0/0x790 handle_mm_fault+0xa2/0x230 do_user_addr_fault+0x3ea/0xa10 exc_page_fault+0x68/0x1a0 asm_exc_page_fault+0x26/0x30 -> #1 (>reset.backoff_srcu){}-{0:0}: lock_acquire+0xd8/0x2d0 _intel_gt_reset_lock+0x57/0x330 [i915] guc_timestamp_ping+0x35/0x130 [i915] process_one_work+0x250/0x510 worker_thread+0x4f/0x3a0 kthread+0xff/0x130 ret_from_fork+0x29/0x50 -> #0 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}: check_prev_add+0x90/0xc60 __lock_acquire+0x1998/0x2590 lock_acquire+0xd8/0x2d0 __flush_work+0x74/0x530 __cancel_work_timer+0x14f/0x1f0 intel_guc_submission_reset_prepare+0x81/0x4b0 [i915] intel_uc_reset_prepare+0x9c/0x120 [i915] reset_prepare+0x21/0x60 [i915] intel_gt_reset+0x1dd/0x470 [i915] intel_gt_reset_global+0xfb/0x170 [i915] intel_gt_handle_error+0x368/0x420 [i915] intel_gt_debugfs_reset_store+0x5c/0xc0 [i915] i915_wedged_set+0x29/0x40 [i915] simple_attr_write_xsigned.constprop.0+0xb4/0x110 full_proxy_write+0x52/0x80 vfs_write+0xc5/0x4f0 ksys_write+0x64/0xe0 do_syscall_64+0x3c/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc other info that might help us debug this: Chain exists of: (work_completion)(&(>timestamp.work)->work) --> fs_reclaim --> >reset.mutex Possible unsafe locking scenario: CPU0CPU1 lock(>reset.mutex); lock(fs_reclaim); lock(>reset.mutex); lock((work_completion)(&(>timestamp.work)->work)); *** DEADLOCK *** 3 locks held by kms_pipe_crc_ba/6415: #0: 888101541430 (sb_writers#15){.+.+}-{0:0}, at: ksys_write+0x64/0xe0 #1: 888136c7eab8 (>mutex){+.+.}-{3:3}, at: simple_attr_write_xsigned.constprop.0+0x47/0x110 #2: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: intel_gt_reset+0x19e/0x470 [i915] Signed-off-by: Zhanjun Dong --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a0e3ef1c65d2..cca6960d3490 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1357,9 +1357,12 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) mod_delayed_work(system_highpri_wq, >timestamp.work, guc->timestamp.ping_delay); } -static void guc_cancel_busyness_worker(struct intel_guc *guc) +static void guc_cancel_busyness_worker(struct intel_guc *guc, bool sync)
Re: [PATCH] mm: fix hugetlb page unmap count balance issue
On Tue, 16 May 2023 15:34:40 -0700 Mike Kravetz wrote: > On 05/15/23 10:04, Mike Kravetz wrote: > > On 05/12/23 16:29, Mike Kravetz wrote: > > > On 05/12/23 14:26, James Houghton wrote: > > > > On Fri, May 12, 2023 at 12:20 AM Junxiao Chang > > > > wrote: > > > > > > > > This alone doesn't fix mapcounting for PTE-mapped HugeTLB pages. You > > > > need something like [1]. I can resend it if that's what we should be > > > > doing, but this mapcounting scheme doesn't work when the page structs > > > > have been freed. > > > > > > > > It seems like it was a mistake to include support for hugetlb memfds in > > > > udmabuf. > > > > > > IIUC, it was added with commit 16c243e99d33 udmabuf: Add support for > > > mapping > > > hugepages (v4). Looks like it was never sent to linux-mm? That is > > > unfortunate > > > as hugetlb vmemmap freeing went in at about the same time. And, as you > > > have > > > noted udmabuf will not work if hugetlb vmemmap freeing is enabled. > > > > > > Sigh! > > > > > > Trying to think of a way forward. > > > -- > > > Mike Kravetz > > > > > > > > > > > [1]: > > > > https://lore.kernel.org/linux-mm/20230306230004.1387007-2-jthough...@google.com/ > > > > > > > > - James > > > > Adding people and list on Cc: involved with commit 16c243e99d33. > > > > There are several issues with trying to map tail pages of hugetllb pages > > not taken into account with udmabuf. James spent quite a bit of time trying > > to understand and address all the issues with the HGM code. While using > > the scheme proposed by James, may be an approach to the mapcount issue there > > are also other issues that need attention. For example, I do not see how > > the fault code checks the state of the hugetlb page (such as poison) as none > > of that state is carried in tail pages. > > > > The more I think about it, the more I think udmabuf should treat hugetlb > > pages as hugetlb pages. They should be mapped at the appropriate level > > in the page table. Of course, this would impose new restrictions on the > > API (mmap and ioctl) that may break existing users. I have no idea how > > extensively udmabuf is being used with hugetlb mappings. > > Verified that using udmabug on a hugetlb mapping with vmemmap optimization > will > BUG as: BUGs aren't good. Can we please find a way to push this along? Have we heard anything from any udmabuf people?
Re: [PATCH 1/1] drm/bridge: Silence error messages upon probe deferral
Hi Alexander, On Wed, Jun 07, 2023 at 01:26:03PM +0200, Alexander Stein wrote: > Am Dienstag, 6. Juni 2023, 17:12:29 CEST schrieb Laurent Pinchart: > > On Tue, Jun 06, 2023 at 04:48:33PM +0200, Alexander Stein wrote: > > > When -EPROBE_DEFER is returned do not raise an error, but silently return > > > this error instead. Fixes error like this: > > > [drm:drm_bridge_attach] *ERROR* failed to attach bridge > > > /soc@0/bus@3080/mipi-dsi@30a0 to encoder None-34: -517 > > > [drm:drm_bridge_attach] *ERROR* failed to attach bridge > > > /soc@0/bus@3080/mipi-dsi@30a0 to encoder None-34: -517 > > > > > > Signed-off-by: Alexander Stein > > > --- > > > dev_err_probe() would be the best, but I am not sure if this function is > > > always used within a driver's probe() call. > > > > > > drivers/gpu/drm/drm_bridge.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > > index c3d69af02e79d..07773d6441a1f 100644 > > > --- a/drivers/gpu/drm/drm_bridge.c > > > +++ b/drivers/gpu/drm/drm_bridge.c > > > @@ -350,6 +350,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, > > > struct drm_bridge *bridge,> > > > bridge->encoder = NULL; > > > list_del(>chain_node); > > > > > > + if (ret != -EPROBE_DEFER) { > > > > > > #ifdef CONFIG_OF > > > > > > DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n", > > > > > > bridge->of_node, encoder->name, ret); > > > > Wrong indentation. > > Ah, right. Thanks for pointing out. > > > dev_err_probe() could be useful, but this function is likely not called > > from probe paths only :-S > > I was afraid this might be the cause. But I'm wondering in which situation > this can be the case, hence -EPROBE_DEFER could be returned then. I've had a quick look, and one example of a non-probe path is in mcde_modeset_init(), with the call to drm_simple_display_pipe_attach_bridge() which calls drm_bridge_attach(). mcde_modeset_init() is called from mcde_drm_bind(), the handler for the .bind() operation in component_master_ops. I'd argue that we should really drop the component framework and replace it with something better, but that's beyond the scope of this patch :-) > > When not called from a probe path, dropping the message will result in a > > silent error, which would be hard to debug :-( > > On the other hand -EPROBE_DEFER is invalid on non-probe path also. > Assuming dev_err_probe is used here, an error will still be raised, - > EPROBE_DEFER should not occur then. I agree that -EPROBE_DEFER shouldn't occur, and in many cases, it won't for drivers that use the component framework, as the whole purpose of the framework is to make sure the bridges are available before we try to attach to them. The framework (or at least the way it's used in drivers) however doesn't handle chains of components: the main DRM driver will have its next bridge available by the time it calls drm_bridge_attach(), but the bridge may then try to acquire the next bridge in the chain, and get an error that results in a probe deferral. Maybe that's not supposed to happen though, bridges probably should acquire the next bridge at probe time, but I can't guarantee this will always be done. And this is my point: I'm scared that this patch will cause silent and hard to debug failures in some cases. Those cases may be incorrect usage of APIs by drivers, but making them silent will make it more difficult to fix them. If everybody thinks I'm over-concerned, please feel free to move forward with this patch, and I'll do my best not to lose sleep :-) > > > @@ -357,6 +358,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, > > > struct drm_bridge *bridge,> > > > DRM_ERROR("failed to attach bridge to encoder %s: %d\n", > > > > > > encoder->name, ret); > > > > > > #endif > > > > > > + } > > > > > > return ret; > > > > > > } -- Regards, Laurent Pinchart
Re: [PATCH v6 05/12] dt-bindings: display/msm: Add SM6375 MDSS
On 06/06/2023 14:43, Konrad Dybcio wrote: > Document the SM6375 MDSS. > > Signed-off-by: Konrad Dybcio > --- > .../bindings/display/msm/qcom,sm6375-mdss.yaml | 215 > + > 1 file changed, 215 insertions(+) > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH v6 04/12] dt-bindings: display/msm: Add SM6350 MDSS
On 06/06/2023 14:43, Konrad Dybcio wrote: > Document the SM6350 MDSS. > > Signed-off-by: Konrad Dybcio > --- > .../bindings/display/msm/qcom,sm6350-mdss.yaml | 213 > + > 1 file changed, 213 insertions(+) > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: PROBLEM: AMD Ryzen 9 7950X iGPU - Blinking Issue
On Wed, Jun 7, 2023 at 4:42 AM Felix Richter wrote: > > Hi Guys, > > so I checked, the kernel I am running has this commit > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > /commit/?id=08da182175db4c7f80850354849d95f2670e8cd9) applied already! > > https://github.com/ju6ge/linux/commit/917680e6056aa288cac288d3afd2745d372beb61u > > And the bug of display flickering persists with or without the > amdgpu.sg_display=0 variable applied! That is unexpected. Setting sg_display=0 should be equivalent to reverting 81d0bcf9900932633d270d5bc4a54ff599c6ebdb. Does the attached patch (with sg_display=0 set) make any difference? Alex > > Kind regards, > Felix Richter > > > On 6/5/23 16:11, Alex Deucher wrote: > > + Hamza > > This is a known issue. You can workaround it by setting > > amdgpu.sg_display=0. It should be issue should be fixed in: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=08da182175db4c7f80850354849d95f2670e8cd9 > > > > Alex > > > > > > > >> Now if this is the desired long term fix I do not know … > >> > >> Kind regards, > >> Felix Richter > >> > >> On 02.05.23 16:12, Linux regression tracking (Thorsten Leemhuis) wrote: > >>> On 02.05.23 15:48, Felix Richter wrote: > On 5/2/23 15:34, Linux regression tracking (Thorsten Leemhuis) wrote: > > On 02.05.23 15:13, Alex Deucher wrote: > >> On Tue, May 2, 2023 at 7:45 AM Linux regression tracking (Thorsten > >> Leemhuis) wrote: > >> > >>> On 30.04.23 13:44, Felix Richter wrote: > Hi, > > I am running into an issue with the integrated GPU of the Ryzen 9 > 7950X. It seems to be a regression from kernel version 6.1 to 6.2. > The bug materializes in from of my monitor blinking, meaning it > turns full white shortly. This happens very often so that the > system becomes unpleasant to use. > > I am running the Archlinux Kernel: > The Issue happens on the bleeding edge kernel: 6.2.13 > Switching back to the LTS kernel resolves the issue: 6.1.26 > > I have two monitors attached to the system. One 42 inch 4k Display > and a 24 inch 1080p Display and am running sway as my desktop. > > Let me know if there is more information I could provide to help > narrow down the issue. > >>> Thanks for the report. To be sure the issue doesn't fall through the > >>> cracks unnoticed, I'm adding it to regzbot, the Linux kernel > >>> regression > >>> tracking bot: > >>> > >>> #regzbot ^introduced v6.1..v6.2 > >>> #regzbot title drm: amdgpu: system becomes unpleasant to use after > >>> monitor starts blinking and turns full white > >>> #regzbot ignore-activity > >>> > >>> This isn't a regression? This issue or a fix for it are already > >>> discussed somewhere else? It was fixed already? You want to clarify > >>> when > >>> the regression started to happen? Or point out I got the title or > >>> something else totally wrong? Then just reply and tell me -- ideally > >>> while also telling regzbot about it, as explained by the page listed > >>> in > >>> the footer of this mail. > >>> > >>> Developers: When fixing the issue, remember to add 'Link:' tags > >>> pointing > >>> to the report (the parent of this mail). See page linked in footer for > >>> details. > >> This sounds exactly like the issue that was fixed in this patch which > >> is already on it's way to Linus: > >> https://gitlab.freedesktop.org/agd5f/linux/-/commit/08da182175db4c7f80850354849d95f2670e8cd9 > > FWIW, you in the flood of emails likely missed that this is the same > > thread where you yesterday replied "If the module parameter didn't help > > then perhaps you are seeing some other issue. Can you bisect?". That's > > why I decided to add this to the tracking. Or am I missing something > > obvious here? > > > > /me looks around again and can't see anything, but that doesn't have to > > mean anything... > > > > Felix, btw, this guide might help you with the bisection, even if it's > > just for kernel compilation: > > > > https://docs.kernel.org/next/admin-guide/quickly-build-trimmed-linux.html > > > > And to indirectly reply to your mail from yesterday[1]. You might want > > to ignore the arch linux kernel git repo and just do a bisection between > > 6.1 and the latest 6.2.y kernel using upstream repos; and if I were you > > I'd also try 6.3 or even mainline before that, in case the issue was > > fixed already. > > > > [1] > > https://lore.kernel.org/all/04749ee4-0728-92fe-bcb0-a7320279e...@felixrichter.tech/ > > > Thanks for the pointers, I'll do a bisection on my desktop from 6.1 to > the newest commit. > >>> FWIW, I wonder what you actually mean with "newest commit" here: a >
Re: [PATCH v1 1/8] dt-bindings: display: panel: mipi-dbi-spi: add shineworld lh133k compatible
On Wed, Jun 07, 2023 at 01:55:00PM +0200, Leonard Göhrs wrote: > The Shineworld LH133K is a 1.3" 240x240px RGB LCD with a MIPI DBI > compatible SPI interface. > The initialization procedure is quite basic with the exception of > requiring inverted colors. > A basic mipi-dbi-cmd[1] script to get the display running thus looks > like this: > > $ cat shineworld,lh133k.txt > command 0x11 # exit sleep mode > delay 120 > > # The display seems to require display color inversion, so enable it. > command 0x21 # INVON > > # Enable normal display mode (in contrast to partial display mode). > command 0x13 # NORON > command 0x29 # MIPI_DCS_SET_DISPLAY_ON > > $ mipi-dbi-cmd shineworld,lh133k.bin shineworld,lh133k.txt > > [1]: https://github.com/notro/panel-mipi-dbi > > Signed-off-by: Leonard Göhrs > --- > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 1 + > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > index 9b701df5e9d28..c07da1a9e6288 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > @@ -67,6 +67,7 @@ properties: > items: >- enum: >- sainsmart18 > + - shineworld,lh133k >- const: panel-mipi-dbi-spi > >write-only: > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml > b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index 82d39ab0231b0..b0afa421bc4a5 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -1189,6 +1189,8 @@ patternProperties: > description: SHIFT GmbH >"^shimafuji,.*": > description: Shimafuji Electric, Inc. > + "^shineworld,.*": > +description: ShineWorld Innovations >"^shiratech,.*": > description: Shiratech Solutions >"^si-en,.*": AFAIU, these are supposed to be split into separate patches. Otherwise, Acked-by: Conor Dooley Cheers, Conor. signature.asc Description: PGP signature
Re: [PATCH v1 2/8] dt-bindings: display: panel: mipi-dbi-spi: add spi-3wire property
On Wed, Jun 07, 2023 at 01:55:01PM +0200, Leonard Göhrs wrote: > Some MIPI DBI panels support a three wire mode (clock, chip select, > bidirectional data) that can be used to ask the panel if it is already set > up by e.g. the bootloader and can thus skip the initialization. > This enables a flicker-free boot. > > Signed-off-by: Leonard Göhrs Acked-by: Conor Dooley Cheers, Conor. signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset
On 6/6/2023 10:53, John Harrison wrote: On 6/5/2023 20:00, Zhanjun Dong wrote: This attemps to avoid circular locing dependency between flush delayed work and intel_gt_reset. locing -> locking WARNING: possible circular locking dependency detected 6.4.0-rc1-drmtip_1340-g31e3463b0edb+ #1 Not tainted -- kms_pipe_crc_ba/6415 is trying to acquire lock: 88813e6cc640 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}, at: __flush_work+0x42/0x530 but task is already holding lock: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: intel_gt_reset+0x19e/0x470 [i915] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (>reset.mutex){+.+.}-{3:3}: lock_acquire+0xd8/0x2d0 i915_gem_shrinker_taints_mutex+0x31/0x50 [i915] intel_gt_init_reset+0x65/0x80 [i915] intel_gt_common_init_early+0xe1/0x170 [i915] intel_root_gt_init_early+0x48/0x60 [i915] i915_driver_probe+0x671/0xcb0 [i915] i915_pci_probe+0xdc/0x210 [i915] pci_device_probe+0x95/0x120 really_probe+0x164/0x3c0 __driver_probe_device+0x73/0x160 driver_probe_device+0x19/0xa0 __driver_attach+0xb6/0x180 bus_for_each_dev+0x77/0xd0 bus_add_driver+0x114/0x210 driver_register+0x5b/0x110 __pfx_vgem_open+0x3/0x10 [vgem] do_one_initcall+0x57/0x270 do_init_module+0x5f/0x220 load_module+0x1ca4/0x1f00 __do_sys_finit_module+0xb4/0x130 do_syscall_64+0x3c/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc -> #2 (fs_reclaim){+.+.}-{0:0}: lock_acquire+0xd8/0x2d0 fs_reclaim_acquire+0xac/0xe0 kmem_cache_alloc+0x32/0x260 i915_vma_instance+0xb2/0xc60 [i915] i915_gem_object_ggtt_pin_ww+0x175/0x370 [i915] vm_fault_gtt+0x22d/0xf60 [i915] __do_fault+0x2f/0x1d0 do_pte_missing+0x4a/0xd20 __handle_mm_fault+0x5b0/0x790 handle_mm_fault+0xa2/0x230 do_user_addr_fault+0x3ea/0xa10 exc_page_fault+0x68/0x1a0 asm_exc_page_fault+0x26/0x30 -> #1 (>reset.backoff_srcu){}-{0:0}: lock_acquire+0xd8/0x2d0 _intel_gt_reset_lock+0x57/0x330 [i915] guc_timestamp_ping+0x35/0x130 [i915] process_one_work+0x250/0x510 worker_thread+0x4f/0x3a0 kthread+0xff/0x130 ret_from_fork+0x29/0x50 -> #0 ((work_completion)(&(>timestamp.work)->work)){+.+.}-{0:0}: check_prev_add+0x90/0xc60 __lock_acquire+0x1998/0x2590 lock_acquire+0xd8/0x2d0 __flush_work+0x74/0x530 __cancel_work_timer+0x14f/0x1f0 intel_guc_submission_reset_prepare+0x81/0x4b0 [i915] intel_uc_reset_prepare+0x9c/0x120 [i915] reset_prepare+0x21/0x60 [i915] intel_gt_reset+0x1dd/0x470 [i915] intel_gt_reset_global+0xfb/0x170 [i915] intel_gt_handle_error+0x368/0x420 [i915] intel_gt_debugfs_reset_store+0x5c/0xc0 [i915] i915_wedged_set+0x29/0x40 [i915] simple_attr_write_xsigned.constprop.0+0xb4/0x110 full_proxy_write+0x52/0x80 vfs_write+0xc5/0x4f0 ksys_write+0x64/0xe0 do_syscall_64+0x3c/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc other info that might help us debug this: Chain exists of: (work_completion)(&(>timestamp.work)->work) --> fs_reclaim --> >reset.mutex Possible unsafe locking scenario: CPU0 CPU1 lock(>reset.mutex); lock(fs_reclaim); lock(>reset.mutex); lock((work_completion)(&(>timestamp.work)->work)); *** DEADLOCK *** 3 locks held by kms_pipe_crc_ba/6415: #0: 888101541430 (sb_writers#15){.+.+}-{0:0}, at: ksys_write+0x64/0xe0 #1: 888136c7eab8 (>mutex){+.+.}-{3:3}, at: simple_attr_write_xsigned.constprop.0+0x47/0x110 #2: 88813e6cce90 (>reset.mutex){+.+.}-{3:3}, at: intel_gt_reset+0x19e/0x470 [i915] Signed-off-by: Zhanjun Dong --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a0e3ef1c65d2..22390704542e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1359,7 +1359,7 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) static void guc_cancel_busyness_worker(struct intel_guc *guc) { - cancel_delayed_work_sync(>timestamp.work); + cancel_delayed_work(>timestamp.work); I think it is worth adding a comment here to explain that it is safe to call the non _sync variant (because of the trylock code in the worker itself) and that the _sync variant hits circular
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/8 00:12, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. No problem. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. The preblem is that device driver can have various demand. It probably want to create different kind of buffers for different thing simultaneously. Say, one allocated with dma_alloc_coherent for command buffer or dma descriptor another one allocated with dma_alloc_wc for uploading shader etc. also has the third one allocated with dma_alloc_noncoherent() for doing some else. Simple setting by DT or firmware which override all allocation is not what we want. My patch is toward the drm core, leave the choice to the device drivers. How does the device driver fetch hardware descriptions is the device driver's thing. either via DT, or ACPI, kernel cmd or hard-code. Its device drivers policy. My patch do not require the platform make the decision for the device driver. Nor does it depend on DT. Your approaches are neither sufficient nor necessary. It gives the freedom to the the device driver. Device driver has full control over the buffer allocation. For our hardware, It don't use DT on some application scene. Out hardware is dma-coherent and cached coherent. We don't want a dma-coherent buffer attached with the name of "map_noncoherent". While my patch is trying to create a *mechanism* which could probably works on all platform. It is based the patch you have already commuted. Thanks for your excellent contribution. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. Please do not mitigate the problems with confusing method. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the
[linux-next:master] BUILD REGRESSION abbd8bb42915d9ed06df11b430bf4ecb3d8ac5ad
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: abbd8bb42915d9ed06df11b430bf4ecb3d8ac5ad Add linux-next specific files for 20230607 Error/Warning reports: https://lore.kernel.org/oe-kbuild-all/202305132244.dwzbucud-...@intel.com https://lore.kernel.org/oe-kbuild-all/202306021936.okttcmat-...@intel.com https://lore.kernel.org/oe-kbuild-all/202306051812.1ydwyzca-...@intel.com https://lore.kernel.org/oe-kbuild-all/202306071513.vcmugxai-...@intel.com Error/Warning: (recently discovered and may have been fixed) ERROR: modpost: "lynx_pcs_destroy" [drivers/net/ethernet/stmicro/stmmac/stmmac.ko] undefined! drivers/bus/fsl-mc/fsl-mc-allocator.c:108:12: warning: variable 'mc_bus_dev' is uninitialized when used here [-Wuninitialized] drivers/cpufreq/cpufreq-dt-platdev.c:105:34: warning: 'blocklist' defined but not used [-Wunused-const-variable=] drivers/cpufreq/cpufreq-dt-platdev.c:18:34: warning: 'allowlist' defined but not used [-Wunused-const-variable=] drivers/net/ethernet/altera/altera_tse_main.c:1419: undefined reference to `lynx_pcs_create_mdiodev' drivers/net/ethernet/altera/altera_tse_main.c:1473: undefined reference to `lynx_pcs_destroy' include/drm/drm_print.h:456:39: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Werror=format=] lib/kunit/executor_test.c:138:4: error: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Werror,-Wcast-function-type-strict] microblaze-linux-ld: (.text+0x14a4): undefined reference to `lynx_pcs_destroy' nios2-linux-ld: drivers/net/ethernet/altera/altera_tse_main.c:1451: undefined reference to `lynx_pcs_destroy' tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:330:18: warning: no previous prototype for 'bpf_kfunc_call_test_offset' [-Wmissing-prototypes] tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:336:1: warning: no previous prototype for 'bpf_kfunc_call_memb_acquire' [-Wmissing-prototypes] tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:342:18: warning: no previous prototype for 'bpf_kfunc_call_memb1_release' [-Wmissing-prototypes] tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:394:18: warning: no previous prototype for 'bpf_kfunc_call_test_fail1' [-Wmissing-prototypes] tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:398:18: warning: no previous prototype for 'bpf_kfunc_call_test_fail2' [-Wmissing-prototypes] tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:402:18: warning: no previous prototype for 'bpf_kfunc_call_test_fail3' [-Wmissing-prototypes] tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:410:18: warning: no previous prototype for 'bpf_kfunc_call_test_mem_len_fail1' [-Wmissing-prototypes] Unverified Error/Warning (likely false positive, please contact us if interested): arch/arm64/kvm/mmu.c:147:3-9: preceding lock on line 140 drivers/clk/qcom/gpucc-sm8550.c:37:22: sparse: sparse: decimal constant 23 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here drivers/clk/qcom/videocc-sm8550.c:34:22: sparse: sparse: decimal constant 23 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here drivers/nvme/host/pr.c:268:23-26: ERROR: reference preceded by free on line 278 drivers/pci/endpoint/functions/pci-epf-mhi.c:362:2-9: line 362 is redundant because platform_get_irq() already prints an error drivers/usb/cdns3/cdns3-starfive.c:23: warning: expecting prototype for cdns3(). Prototype was for USB_STRAP_HOST() instead drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c:217:30: sparse: sparse: incorrect type in argument 1 (different base types) kernel/events/uprobes.c:478 uprobe_write_opcode() warn: passing zero to 'PTR_ERR' lib/kunit/test.c:336 __kunit_abort() warn: ignoring unreachable code. Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- arc-allyesconfig | `-- drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead |-- arm-allmodconfig | `-- drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead |-- arm-allyesconfig | `-- drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead |-- arm64-allyesconfig | `-- drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead |-- arm64-randconfig-c004-20230607 | `-- arch-arm64-kvm-mmu.c:preceding-lock-on-line |-- csky-randconfig-s053-20230607 | |-- drivers-clk-qcom-gpucc-sm8550.c:sparse:sparse:decimal-constant-is-between-LONG_MAX-and-ULONG_MAX.-For-C99-that-means-long-long-C90-compilers-are-very-likely-
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : > Hi, welcome to discussion. > > > I have limited skills in manipulating English. > > It may not express what I'm really means in the short time. > > Part of word in the sentence may not as accurate as your. > > Well, please don't misunderstand, I'm not doing the rude to you. No problem. > > I will explain it with more details. > > See below: > > > On 2023/6/7 20:09, Paul Cercueil wrote: > > Hi Sui, > > > > Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : > > > Hi, > > > > > > > > > On 2023/6/7 17:36, Paul Cercueil wrote: > > > > Hi Sui, > > > > > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > > > The single map_noncoherent member of struct > > > > > drm_gem_dma_object > > > > > may > > > > > not > > > > > sufficient for describing the backing memory of the GEM > > > > > buffer > > > > > object. > > > > > > > > > > Especially on dma-coherent systems, the backing memory is > > > > > both > > > > > cached > > > > > coherent for multi-core CPUs and dma-coherent for peripheral > > > > > device. > > > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, > > > > > etc. > > > > > > > > > > Whether a peripheral device is dma-coherent or not can be > > > > > implementation-dependent. The single map_noncoherent option > > > > > is > > > > > not > > > > > enough > > > > > to reflect real hardware anymore. For example, the Loongson > > > > > LS3A4000 > > > > > CPU > > > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > > > platform > > > > > allways snoop CPU's cache. Doing the allocation with > > > > > dma_alloc_coherent > > > > > function is preferred. The return buffer is cached, it should > > > > > not > > > > > using > > > > > the default write-combine mapping. While with the current > > > > > implement, > > > > > there > > > > > no way to tell the drm core to reflect this. > > > > > > > > > > This patch adds cached and coherent members to struct > > > > > drm_gem_dma_object. > > > > > which allow driver implements to inform the core. Introducing > > > > > new > > > > > mappings > > > > > while keeping the original default behavior unchanged. > > > > Did you try to simply set the "dma-coherent" property to the > > > > device's > > > > node? > > > But this approach can only be applied for the device driver with > > > DT > > > support. > > > > > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically > > > do > > > not > > > have DT support. > > > > > > They using ACPI to pass parameter from the firmware to Linux > > > kernel. > > > > > > You approach will lost the effectiveness on such a case. > > Well, I don't really know how ACPI handles it - but it should just > > be a > > matter of setting dev->dma_coherent. That's basically what the DT > > code > > does. > > > > Some MIPS boards set it in their setup code for instance. > > > This is a *strategy*, not a *mechanism*. > > In this case, DT is just used to describing the hardware. > > (It is actually a hardware feature describing language, the > granularity > is large) > > It does not changing the state of the hardware. > > It's your platform firmware or kernel setting up code who actually do > such a things. > > > It's just that it works on *one* platform, it does not guarantee it > will > works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. > While my patch is trying to create a *mechanism* which could probably > > works on all platform. > > > It is based the patch you have already commuted. > > Thanks for your excellent contribution. > > > > > > From what I understand if you add that property then Linux > > > > will > > > > use DMA > > > > coherent memory even though you use dma_alloc_noncoherent() and > > > > the > > > > sync_single_for_cpu() / sync_single_for_device() are then NOPs. > > > Please do not mitigate the problems with confusing method. > > > > > > > > > This approach not only tend to generate confusion but also > > > implement-dependent > > > > > > and arch-dependent. It's definitely problematic. > > > > > > > > > How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH > > > specific > > > thing. > > > > > > Dependent on how does the arch_dma_ops is implemented. > > > > > > > > > The definition of the coherent on different ARCH has different > > > meanings. > > > > > > The definition of the wirte-combine on different ARCH has > > > different > > > meanings. > > > > > > > > > The wirte-combine(uncache acceleration) on mips is non dma- > > >
Re: [PATCH v6 00/13] Enable Colorspace connector property in amdgpu
Thanks! All of the core DRM patches (1-6) are Reviewed-by: Simon Ser
Re: [PATCH drm-next v4 12/14] drm/nouveau: nvkm/vmm: implement raw ops to manage uvmm
Hi Danilo, kernel test robot noticed the following build warnings: [auto build test WARNING on 33a86170888b7e4aa0cea94ebb9c67180139cea9] url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230607-063442 base: 33a86170888b7e4aa0cea94ebb9c67180139cea9 patch link: https://lore.kernel.org/r/20230606223130.6132-13-dakr%40redhat.com patch subject: [PATCH drm-next v4 12/14] drm/nouveau: nvkm/vmm: implement raw ops to manage uvmm config: riscv-rv32_defconfig (https://download.01.org/0day-ci/archive/20230607/202306072317.vpcwyh1w-...@intel.com/config) compiler: riscv32-linux-gcc (GCC) 12.3.0 reproduce (this is a W=1 build): mkdir -p ~/bin wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 33a86170888b7e4aa0cea94ebb9c67180139cea9 b4 shazam https://lore.kernel.org/r/20230606223130.6132-13-d...@redhat.com # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=riscv olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/gpu/drm/ If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202306072317.vpcwyh1w-...@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h:4, from drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.h:5, from drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c:22: drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c: In function 'nvkm_uvmm_mthd_raw_map': >> drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c:422:31: warning: cast to >> pointer from integer of different size [-Wint-to-pointer-cast] 422 | (void *)args->argv, args->argc); | ^ drivers/gpu/drm/nouveau/include/nvkm/core/memory.h:66:43: note: in definition of macro 'nvkm_memory_map' 66 | (p)->func->map((p),(o),(vm),(va),(av),(ac)) | ^~ vim +422 drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c 388 389 static int 390 nvkm_uvmm_mthd_raw_map(struct nvkm_uvmm *uvmm, struct nvif_vmm_raw_v0 *args) 391 { 392 struct nvkm_client *client = uvmm->object.client; 393 struct nvkm_vmm *vmm = uvmm->vmm; 394 struct nvkm_vma vma = { 395 .addr = args->addr, 396 .size = args->size, 397 .used = true, 398 .mapref = false, 399 .no_comp = true, 400 }; 401 struct nvkm_memory *memory; 402 u64 handle = args->memory; 403 u8 refd; 404 int ret; 405 406 if (!nvkm_vmm_in_managed_range(vmm, args->addr, args->size)) 407 return -EINVAL; 408 409 ret = nvkm_uvmm_page_index(uvmm, args->size, args->shift, ); 410 if (ret) 411 return ret; 412 413 vma.page = vma.refd = refd; 414 415 memory = nvkm_umem_search(client, args->memory); 416 if (IS_ERR(memory)) { 417 VMM_DEBUG(vmm, "memory %016llx %ld\n", handle, PTR_ERR(memory)); 418 return PTR_ERR(memory); 419 } 420 421 ret = nvkm_memory_map(memory, args->offset, vmm, , > 422(void *)args->argv, args->argc); 423 424 nvkm_memory_unref(); 425 nvkm_memory_unref(); 426 return ret; 427 } 428 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
Hi Fei, On Tue, Jun 06, 2023 at 12:00:42PM +0200, Andi Shyti wrote: > From: Fei Yang > > To comply with the design that buffer objects shall have immutable > cache setting through out their life cycle, {set, get}_caching ioctl's > are no longer supported from MTL onward. With that change caching > policy can only be set at object creation time. The current code > applies a default (platform dependent) cache setting for all objects. > However this is not optimal for performance tuning. The patch extends > the existing gem_create uAPI to let user set PAT index for the object > at creation time. > The new extension is platform independent, so UMD's can switch to using > this extension for older platforms as well, while {set, get}_caching are > still supported on these legacy paltforms for compatibility reason. > However, since PAT index was not clearly defined for platforms prior to > GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms > only. See ext_set_pat() in for the implementation details. > > Note: The documentation related to the PAT/MOCS tables is currently > available for Tiger Lake here: > https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/tiger-lake.html > > BSpec: 45101 > > Mesa support has been submitted in this merge request: > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878 > > The media driver is supported by the following commits: > https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341 > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd > https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000 > > The IGT test related to this change is > igt@gem_create@create-ext-set-pat > > Signed-off-by: Fei Yang > Cc: Chris Wilson > Cc: Matt Roper > Cc: Andi Shyti > Reviewed-by: Andi Shyti > Acked-by: Jordan Justen > Tested-by: Jordan Justen > Acked-by: Carl Zhang > Tested-by: Lihao Gu > Signed-off-by: Andi Shyti pushed to drm-intel-gt-next with: - Tvrtko's ack - Slawek's ack - the pull request link from media guys Thank you! Andi
Re: [PATCH drm-next v4 09/14] drm/nouveau: fence: separate fence alloc and emit
Hi Danilo, kernel test robot noticed the following build errors: [auto build test ERROR on 33a86170888b7e4aa0cea94ebb9c67180139cea9] url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230607-063442 base: 33a86170888b7e4aa0cea94ebb9c67180139cea9 patch link: https://lore.kernel.org/r/20230606223130.6132-10-dakr%40redhat.com patch subject: [PATCH drm-next v4 09/14] drm/nouveau: fence: separate fence alloc and emit config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230607/202306072327.bhc88w12-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): git checkout 33a86170888b7e4aa0cea94ebb9c67180139cea9 b4 shazam https://lore.kernel.org/r/20230606223130.6132-10-d...@redhat.com # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=x86_64 olddefconfig make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/ If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202306072327.bhc88w12-...@intel.com/ All errors (new ones prefixed by >>): drivers/gpu/drm/nouveau/nouveau_dmem.c: In function 'nouveau_dmem_migrate_chunk': >> drivers/gpu/drm/nouveau/nouveau_dmem.c:681:43: error: 'chunk' undeclared >> (first use in this function) 681 | nouveau_fence_emit(fence, chunk->drm->dmem->migrate.chan); | ^ drivers/gpu/drm/nouveau/nouveau_dmem.c:681:43: note: each undeclared identifier is reported only once for each function it appears in vim +/chunk +681 drivers/gpu/drm/nouveau/nouveau_dmem.c 664 665 static void nouveau_dmem_migrate_chunk(struct nouveau_drm *drm, 666 struct nouveau_svmm *svmm, struct migrate_vma *args, 667 dma_addr_t *dma_addrs, u64 *pfns) 668 { 669 struct nouveau_fence *fence; 670 unsigned long addr = args->start, nr_dma = 0, i; 671 672 for (i = 0; addr < args->end; i++) { 673 args->dst[i] = nouveau_dmem_migrate_copy_one(drm, svmm, 674 args->src[i], dma_addrs + nr_dma, pfns + i); 675 if (!dma_mapping_error(drm->dev->dev, dma_addrs[nr_dma])) 676 nr_dma++; 677 addr += PAGE_SIZE; 678 } 679 680 if (!nouveau_fence_new()) > 681 nouveau_fence_emit(fence, > chunk->drm->dmem->migrate.chan); 682 migrate_vma_pages(args); 683 nouveau_dmem_fence_done(); 684 nouveau_pfns_map(svmm, args->vma->vm_mm, args->start, pfns, i); 685 686 while (nr_dma--) { 687 dma_unmap_page(drm->dev->dev, dma_addrs[nr_dma], PAGE_SIZE, 688 DMA_BIDIRECTIONAL); 689 } 690 migrate_vma_finalize(args); 691 } 692 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2] drm/panel-edp: Add AUO B116XAB01.4 edp panel entry
Hi, On Wed, Jun 7, 2023 at 8:06 AM Laura Nao wrote: > > Add a panel entry for the AUO B116XAB01.4 edp panel, found in the Acer > Chromebook Spin 311 (CP311-3H) laptop. > > Signed-off-by: Laura Nao > --- > > Changes in v2: > - Sorted by product ID > > drivers/gpu/drm/panel/panel-edp.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Douglas Anderson For simple updates to this table, there's not much reason to leave them idling on the list, so pushed to drm-misc-next: 1ebc9f0365ef drm/panel-edp: Add AUO B116XAB01.4 edp panel entry
[PATCH v6 10/13] drm/amd/display: Send correct DP colorspace infopacket
Look at connector->colorimetry to determine output colorspace. We don't want to impact current SDR behavior, so DRM_MODE_COLORIMETRY_DEFAULT preserves current behavior. Also add support to explicitly set BT601 and BT709. v4: - Roll support for BT709 and BT601 into this patch - Add default case to avoid warnings for unhandled enum values Signed-off-by: Harry Wentland Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Joshua Ashton Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 --- 1 file changed, 31 insertions(+), 17 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 793ea29b4cfe..060a975f9885 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5338,21 +5338,44 @@ get_aspect_ratio(const struct drm_display_mode *mode_in) } static enum dc_color_space -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing, + const struct drm_connector_state *connector_state) { enum dc_color_space color_space = COLOR_SPACE_SRGB; - switch (dc_crtc_timing->pixel_encoding) { - case PIXEL_ENCODING_YCBCR422: - case PIXEL_ENCODING_YCBCR444: - case PIXEL_ENCODING_YCBCR420: - { + switch (connector_state->colorspace) { + case DRM_MODE_COLORIMETRY_BT601_YCC: + if (dc_crtc_timing->flags.Y_ONLY) + color_space = COLOR_SPACE_YCBCR601_LIMITED; + else + color_space = COLOR_SPACE_YCBCR601; + break; + case DRM_MODE_COLORIMETRY_BT709_YCC: + if (dc_crtc_timing->flags.Y_ONLY) + color_space = COLOR_SPACE_YCBCR709_LIMITED; + else + color_space = COLOR_SPACE_YCBCR709; + break; + case DRM_MODE_COLORIMETRY_OPRGB: + color_space = COLOR_SPACE_ADOBERGB; + break; + case DRM_MODE_COLORIMETRY_BT2020_RGB: + case DRM_MODE_COLORIMETRY_BT2020_YCC: + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) + color_space = COLOR_SPACE_2020_RGB_FULLRANGE; + else + color_space = COLOR_SPACE_2020_YCBCR; + break; + case DRM_MODE_COLORIMETRY_DEFAULT: // ITU601 + default: + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) { + color_space = COLOR_SPACE_SRGB; /* * 27030khz is the separation point between HDTV and SDTV * according to HDMI spec, we use YCbCr709 and YCbCr601 * respectively */ - if (dc_crtc_timing->pix_clk_100hz > 270300) { + } else if (dc_crtc_timing->pix_clk_100hz > 270300) { if (dc_crtc_timing->flags.Y_ONLY) color_space = COLOR_SPACE_YCBCR709_LIMITED; @@ -5365,15 +5388,6 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) else color_space = COLOR_SPACE_YCBCR601; } - - } - break; - case PIXEL_ENCODING_RGB: - color_space = COLOR_SPACE_SRGB; - break; - - default: - WARN_ON(1); break; } @@ -5512,7 +5526,7 @@ static void fill_stream_properties_from_drm_display_mode( } } - stream->output_color_space = get_output_color_space(timing_out); + stream->output_color_space = get_output_color_space(timing_out, connector_state); } static void fill_audio_info(struct audio_info *audio_info, -- 2.41.0
[PATCH v6 12/13] drm/amd/display: Add debugfs for testing output colorspace
In order to IGT test colorspace we'll want to print the currently enabled colorspace on a stream. We add a new debugfs to do so, using the same scheme as current bpc reporting. This might also come in handy when debugging display issues. v4: - Fix function doc comment - Fix sRGB debug print Signed-off-by: Harry Wentland Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Joshua Ashton Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 57 +++ 1 file changed, 57 insertions(+) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c index 82234397dd44..caf13b2e8cb6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c @@ -906,6 +906,61 @@ static int amdgpu_current_bpc_show(struct seq_file *m, void *data) } DEFINE_SHOW_ATTRIBUTE(amdgpu_current_bpc); +/* + * Returns the current colorspace for the crtc. + * Example usage: cat /sys/kernel/debug/dri/0/crtc-0/amdgpu_current_colorspace + */ +static int amdgpu_current_colorspace_show(struct seq_file *m, void *data) +{ + struct drm_crtc *crtc = m->private; + struct drm_device *dev = crtc->dev; + struct dm_crtc_state *dm_crtc_state = NULL; + int res = -ENODEV; + + mutex_lock(>mode_config.mutex); + drm_modeset_lock(>mutex, NULL); + if (crtc->state == NULL) + goto unlock; + + dm_crtc_state = to_dm_crtc_state(crtc->state); + if (dm_crtc_state->stream == NULL) + goto unlock; + + switch (dm_crtc_state->stream->output_color_space) { + case COLOR_SPACE_SRGB: + seq_printf(m, "sRGB"); + break; + case COLOR_SPACE_YCBCR601: + case COLOR_SPACE_YCBCR601_LIMITED: + seq_printf(m, "BT601_YCC"); + break; + case COLOR_SPACE_YCBCR709: + case COLOR_SPACE_YCBCR709_LIMITED: + seq_printf(m, "BT709_YCC"); + break; + case COLOR_SPACE_ADOBERGB: + seq_printf(m, "opRGB"); + break; + case COLOR_SPACE_2020_RGB_FULLRANGE: + seq_printf(m, "BT2020_RGB"); + break; + case COLOR_SPACE_2020_YCBCR: + seq_printf(m, "BT2020_YCC"); + break; + default: + goto unlock; + } + res = 0; + +unlock: + drm_modeset_unlock(>mutex); + mutex_unlock(>mode_config.mutex); + + return res; +} +DEFINE_SHOW_ATTRIBUTE(amdgpu_current_colorspace); + + /* * Example usage: * Disable dsc passthrough, i.e.,: have dsc decoding at converver, not external RX @@ -3139,6 +3194,8 @@ void crtc_debugfs_init(struct drm_crtc *crtc) #endif debugfs_create_file("amdgpu_current_bpc", 0644, crtc->debugfs_entry, crtc, _current_bpc_fops); + debugfs_create_file("amdgpu_current_colorspace", 0644, crtc->debugfs_entry, + crtc, _current_colorspace_fops); } /* -- 2.41.0
[PATCH v6 13/13] drm/amd/display: Refactor avi_info_frame colorimetry determination
From: Joshua Ashton Replace the messy two if-else chains here that were on the same value with a switch on the enum. Signed-off-by: Joshua Ashton Signed-off-by: Harry Wentland Reviewed-by: Harry Wentland Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Joshua Ashton Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- .../gpu/drm/amd/display/dc/core/dc_resource.c | 28 +++ 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c index c72540d37aef..2f3d9a698486 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c @@ -3035,23 +3035,29 @@ static void set_avi_info_frame( hdmi_info.bits.S0_S1 = scan_type; /* C0, C1 : Colorimetry */ - if (color_space == COLOR_SPACE_YCBCR709 || - color_space == COLOR_SPACE_YCBCR709_LIMITED) + switch (color_space) { + case COLOR_SPACE_YCBCR709: + case COLOR_SPACE_YCBCR709_LIMITED: hdmi_info.bits.C0_C1 = COLORIMETRY_ITU709; - else if (color_space == COLOR_SPACE_YCBCR601 || - color_space == COLOR_SPACE_YCBCR601_LIMITED) + break; + case COLOR_SPACE_YCBCR601: + case COLOR_SPACE_YCBCR601_LIMITED: hdmi_info.bits.C0_C1 = COLORIMETRY_ITU601; - else { - hdmi_info.bits.C0_C1 = COLORIMETRY_NO_DATA; - } - if (color_space == COLOR_SPACE_2020_RGB_FULLRANGE || - color_space == COLOR_SPACE_2020_RGB_LIMITEDRANGE || - color_space == COLOR_SPACE_2020_YCBCR) { + break; + case COLOR_SPACE_2020_RGB_FULLRANGE: + case COLOR_SPACE_2020_RGB_LIMITEDRANGE: + case COLOR_SPACE_2020_YCBCR: hdmi_info.bits.EC0_EC2 = COLORIMETRYEX_BT2020RGBYCBCR; hdmi_info.bits.C0_C1 = COLORIMETRY_EXTENDED; - } else if (color_space == COLOR_SPACE_ADOBERGB) { + break; + case COLOR_SPACE_ADOBERGB: hdmi_info.bits.EC0_EC2 = COLORIMETRYEX_ADOBERGB; hdmi_info.bits.C0_C1 = COLORIMETRY_EXTENDED; + break; + case COLOR_SPACE_SRGB: + default: + hdmi_info.bits.C0_C1 = COLORIMETRY_NO_DATA; + break; } if (pixel_encoding && color_space == COLOR_SPACE_2020_YCBCR && -- 2.41.0
[PATCH v6 04/13] drm/connector: Use common colorspace_names array
We an use bitfields to track the support ones for HDMI and DP. This allows us to print colorspaces in a consistent manner without needing to know whether we're dealing with DP or HDMI. v4: - Rename _MAX to _COUNT and leave comment to indicate it's not a valid value - Fix misplaced function doc v6: - Drop magic in drm_mode_create_colorspace_property for dealing with "0" supported_colorspaces. Expect the caller to always provide a non-zero supported_colorspaces. - Improve error checking and logging Signed-off-by: Harry Wentland Reviewed-by: Sebastian Wick Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Jani Nikula Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/drm_connector.c | 130 +++- include/drm/drm_connector.h | 2 + 2 files changed, 79 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 9c087d6f5691..484effc731df 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1016,64 +1016,70 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = { DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name, drm_dp_subconnector_enum_list) -static const struct drm_prop_enum_list hdmi_colorspaces[] = { + +static const char * const colorspace_names[] = { /* For Default case, driver will set the colorspace */ - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, + [DRM_MODE_COLORIMETRY_DEFAULT] = "Default", /* Standard Definition Colorimetry based on CEA 861 */ - { DRM_MODE_COLORIMETRY_SMPTE_170M_YCC, "SMPTE_170M_YCC" }, - { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" }, + [DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = "SMPTE_170M_YCC", + [DRM_MODE_COLORIMETRY_BT709_YCC] = "BT709_YCC", /* Standard Definition Colorimetry based on IEC 61966-2-4 */ - { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" }, + [DRM_MODE_COLORIMETRY_XVYCC_601] = "XVYCC_601", /* High Definition Colorimetry based on IEC 61966-2-4 */ - { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" }, + [DRM_MODE_COLORIMETRY_XVYCC_709] = "XVYCC_709", /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ - { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" }, + [DRM_MODE_COLORIMETRY_SYCC_601] = "SYCC_601", /* Colorimetry based on IEC 61966-2-5 [33] */ - { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" }, + [DRM_MODE_COLORIMETRY_OPYCC_601] = "opYCC_601", /* Colorimetry based on IEC 61966-2-5 */ - { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, + [DRM_MODE_COLORIMETRY_OPRGB] = "opRGB", /* Colorimetry based on ITU-R BT.2020 */ - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" }, + [DRM_MODE_COLORIMETRY_BT2020_CYCC] = "BT2020_CYCC", /* Colorimetry based on ITU-R BT.2020 */ - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" }, + [DRM_MODE_COLORIMETRY_BT2020_RGB] = "BT2020_RGB", /* Colorimetry based on ITU-R BT.2020 */ - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" }, + [DRM_MODE_COLORIMETRY_BT2020_YCC] = "BT2020_YCC", /* Added as part of Additional Colorimetry Extension in 861.G */ - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" }, - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" }, + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65] = "DCI-P3_RGB_D65", + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER] = "DCI-P3_RGB_Theater", + [DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED] = "RGB_WIDE_FIXED", + /* Colorimetry based on scRGB (IEC 61966-2-2) */ + [DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT] = "RGB_WIDE_FLOAT", + [DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC", }; +static const u32 hdmi_colorspaces = + BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) | + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) | + BIT(DRM_MODE_COLORIMETRY_XVYCC_601) | + BIT(DRM_MODE_COLORIMETRY_XVYCC_709) | + BIT(DRM_MODE_COLORIMETRY_SYCC_601) | + BIT(DRM_MODE_COLORIMETRY_OPYCC_601) | + BIT(DRM_MODE_COLORIMETRY_OPRGB) | + BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) | + BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) | + BIT(DRM_MODE_COLORIMETRY_BT2020_YCC) | + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) | + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER); + /* * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel Encoding/Colorimetry * Format Table 2-120 */ -static const struct drm_prop_enum_list dp_colorspaces[] = { - /* For Default case, driver will set the colorspace */ - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, - { DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED, "RGB_Wide_Gamut_Fixed_Point" }, - /* Colorimetry based on scRGB (IEC 61966-2-2)
[PATCH v6 03/13] drm/connector: Pull out common create_colorspace_property code
Signed-off-by: Harry Wentland Reviewed-by: Sebastian Wick Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Jani Nikula Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/drm_connector.c | 54 - 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 547356e00341..9c087d6f5691 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1975,33 +1975,44 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property); * drm_mode_create_dp_colorspace_property() is used for DP connector. */ -/** - * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property - * @connector: connector to create the Colorspace property on. - * - * Called by a driver the first time it's needed, must be attached to desired - * HDMI connectors. - * - * Returns: - * Zero on success, negative errno on failure. - */ -int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector) +static int drm_mode_create_colorspace_property(struct drm_connector *connector, + const struct drm_prop_enum_list *colorspaces, + int size) { struct drm_device *dev = connector->dev; if (connector->colorspace_property) return 0; + if (!colorspaces) + return 0; + connector->colorspace_property = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", -hdmi_colorspaces, -ARRAY_SIZE(hdmi_colorspaces)); + colorspaces, + size); if (!connector->colorspace_property) return -ENOMEM; return 0; } +/** + * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property + * @connector: connector to create the Colorspace property on. + * + * Called by a driver the first time it's needed, must be attached to desired + * HDMI connectors. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector) +{ + return drm_mode_create_colorspace_property(connector, + hdmi_colorspaces, + ARRAY_SIZE(hdmi_colorspaces)); +} EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property); /** @@ -2016,20 +2027,9 @@ EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property); */ int drm_mode_create_dp_colorspace_property(struct drm_connector *connector) { - struct drm_device *dev = connector->dev; - - if (connector->colorspace_property) - return 0; - - connector->colorspace_property = - drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", -dp_colorspaces, -ARRAY_SIZE(dp_colorspaces)); - - if (!connector->colorspace_property) - return -ENOMEM; - - return 0; + return drm_mode_create_colorspace_property(connector, + dp_colorspaces, + ARRAY_SIZE(dp_colorspaces)); } EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property); -- 2.41.0
[PATCH v6 11/13] drm/amd/display: Always set crtcinfo from create_stream_for_sink
From: Joshua Ashton Given that we always pass dm_state into here now, this won't ever trigger anymore. This is needed for we will always fail mode validation with invalid clocks or link bandwidth errors. Signed-off-by: Joshua Ashton Signed-off-by: Harry Wentland Reviewed-by: Harry Wentland Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Joshua Ashton Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 060a975f9885..e17c8afce2f4 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6054,7 +6054,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, if (recalculate_timing) drm_mode_set_crtcinfo(_mode, 0); - else if (!dm_state) + else drm_mode_set_crtcinfo(, 0); /* -- 2.41.0
[PATCH v6 09/13] drm/amd/display: Signal mode_changed if colorspace changed
We need to signal mode_changed to make sure we update the output colorspace. v2: No need to call drm_hdmi_avi_infoframe_colorimetry as DC does its own infoframe packing. Signed-off-by: Harry Wentland Reviewed-by: Leo Li Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Joshua Ashton Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) 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 bdda136235af..793ea29b4cfe 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6699,6 +6699,14 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn, if (!crtc) return 0; + if (new_con_state->colorspace != old_con_state->colorspace) { + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(new_crtc_state)) + return PTR_ERR(new_crtc_state); + + new_crtc_state->mode_changed = true; + } + if (!drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state)) { struct dc_info_packet hdr_infopacket; @@ -6721,7 +6729,7 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn, * set is permissible, however. So only force a * modeset if we're entering or exiting HDR. */ - new_crtc_state->mode_changed = + new_crtc_state->mode_changed = new_crtc_state->mode_changed || !old_con_state->hdr_output_metadata || !new_con_state->hdr_output_metadata; } -- 2.41.0
[PATCH v6 08/13] drm/amd/display: Register Colorspace property for DP and HDMI
We want compositors to be able to set the output colorspace on DP and HDMI outputs, based on the caps reported from the receiver via EDID. Signed-off-by: Harry Wentland Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Joshua Ashton Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++ 1 file changed, 15 insertions(+) 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 27868dbb09f6..bdda136235af 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7246,6 +7246,12 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector) return amdgpu_dm_connector->num_modes; } +static const u32 supported_colorspaces = + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) | + BIT(DRM_MODE_COLORIMETRY_OPRGB) | + BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) | + BIT(DRM_MODE_COLORIMETRY_BT2020_YCC); + void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, struct amdgpu_dm_connector *aconnector, int connector_type, @@ -7326,6 +7332,15 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, adev->mode_info.abm_level_property, 0); } + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { + if (!drm_mode_create_hdmi_colorspace_property(>base, supported_colorspaces)) + drm_connector_attach_colorspace_property(>base); + } else if (connector_type == DRM_MODE_CONNECTOR_DisplayPort || + connector_type == DRM_MODE_CONNECTOR_eDP) { + if (!drm_mode_create_dp_colorspace_property(>base, supported_colorspaces)) + drm_connector_attach_colorspace_property(>base); + } + if (connector_type == DRM_MODE_CONNECTOR_HDMIA || connector_type == DRM_MODE_CONNECTOR_DisplayPort || connector_type == DRM_MODE_CONNECTOR_eDP) { -- 2.41.0
[PATCH v6 06/13] drm/connector: Allow drivers to pass list of supported colorspaces
Drivers might not support all colorspaces defined in dp_colorspaces and hdmi_colorspaces. This results in undefined behavior when userspace is setting an unsupported colorspace. Allow drivers to pass the list of supported colorspaces when creating the colorspace property. v2: - Use 0 to indicate support for all colorspaces (Jani) - Print drm_dbg_kms message when drivers pass 0 to signal that drivers should specify supported colorspaecs explicity (Jani) v3: - Move changes to create a common colorspace_names array to separate patch v6: - Avoid magic when passing 0 for supported_colorspaces; be explicit in treating it as "all DP/HDMI" Signed-off-by: Harry Wentland Reviewed-by: Sebastian Wick Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Jani Nikula Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/drm_connector.c | 24 +++ .../gpu/drm/i915/display/intel_connector.c| 4 ++-- drivers/gpu/drm/vc4/vc4_hdmi.c| 2 +- include/drm/drm_connector.h | 7 -- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 46bf1f2ad535..8fe695047ced 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2050,9 +2050,17 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector, * Returns: * Zero on success, negative errno on failure. */ -int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector) +int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector, +u32 supported_colorspaces) { - return drm_mode_create_colorspace_property(connector, hdmi_colorspaces); + u32 colorspaces; + + if (supported_colorspaces) + colorspaces = supported_colorspaces & hdmi_colorspaces; + else + colorspaces = hdmi_colorspaces; + + return drm_mode_create_colorspace_property(connector, colorspaces); } EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property); @@ -2066,9 +2074,17 @@ EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property); * Returns: * Zero on success, negative errno on failure. */ -int drm_mode_create_dp_colorspace_property(struct drm_connector *connector) +int drm_mode_create_dp_colorspace_property(struct drm_connector *connector, + u32 supported_colorspaces) { - return drm_mode_create_colorspace_property(connector, dp_colorspaces); + u32 colorspaces; + + if (supported_colorspaces) + colorspaces = supported_colorspaces & dp_colorspaces; + else + colorspaces = dp_colorspaces; + + return drm_mode_create_colorspace_property(connector, colorspaces); } EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property); diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 6205ddd3ded0..e8b4a352a7a6 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -283,14 +283,14 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector) void intel_attach_hdmi_colorspace_property(struct drm_connector *connector) { - if (!drm_mode_create_hdmi_colorspace_property(connector)) + if (!drm_mode_create_hdmi_colorspace_property(connector, 0)) drm_connector_attach_colorspace_property(connector); } void intel_attach_dp_colorspace_property(struct drm_connector *connector) { - if (!drm_mode_create_dp_colorspace_property(connector)) + if (!drm_mode_create_dp_colorspace_property(connector, 0)) drm_connector_attach_colorspace_property(connector); } diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 55744216392b..eee53e841701 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -618,7 +618,7 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, if (ret) return ret; - ret = drm_mode_create_hdmi_colorspace_property(connector); + ret = drm_mode_create_hdmi_colorspace_property(connector, 0); if (ret) return ret; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 9d9e4d6f0449..f799cbd755a3 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -1904,8 +1905,10 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
[PATCH v6 07/13] drm/amd/display: Always pass connector_state to stream validation
We need the connector_state for colorspace and scaling information and can get it from connector->state. Signed-off-by: Harry Wentland Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Joshua Ashton Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 + 1 file changed, 5 insertions(+), 4 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 90de0d37f1d2..27868dbb09f6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5954,15 +5954,14 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, { struct drm_display_mode *preferred_mode = NULL; struct drm_connector *drm_connector; - const struct drm_connector_state *con_state = - dm_state ? _state->base : NULL; + const struct drm_connector_state *con_state = _state->base; struct dc_stream_state *stream = NULL; struct drm_display_mode mode; struct drm_display_mode saved_mode; struct drm_display_mode *freesync_mode = NULL; bool native_mode_found = false; bool recalculate_timing = false; - bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false; + bool scale = dm_state->scaling != RMX_OFF; int mode_refresh; int preferred_refresh = 0; enum color_transfer_func tf = TRANSFER_FUNC_UNKNOWN; @@ -6604,7 +6603,9 @@ enum drm_mode_status amdgpu_dm_connector_mode_valid(struct drm_connector *connec goto fail; } - stream = create_validate_stream_for_sink(aconnector, mode, NULL, NULL); + stream = create_validate_stream_for_sink(aconnector, mode, + to_dm_connector_state(connector->state), +NULL); if (stream) { dc_stream_release(stream); result = MODE_OK; -- 2.41.0
[PATCH v6 05/13] drm/connector: Print connector colorspace in state debugfs
v3: Fix kerneldocs (kernel test robot) v4: Avoid returning NULL from drm_get_colorspace_name Signed-off-by: Harry Wentland Reviewed-by: Sebastian Wick Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Jani Nikula Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/drm_atomic.c| 1 + drivers/gpu/drm/drm_connector.c | 15 +++ include/drm/drm_connector.h | 1 + 3 files changed, 17 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index c0dc5858a723..d6d04c4ccfc0 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1071,6 +1071,7 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)"); drm_printf(p, "\tself_refresh_aware=%d\n", state->self_refresh_aware); drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); + drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) if (state->writeback_job && state->writeback_job->fb) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 484effc731df..46bf1f2ad535 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1048,6 +1048,21 @@ static const char * const colorspace_names[] = { [DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC", }; +/** + * drm_get_colorspace_name - return a string for color encoding + * @colorspace: color space to compute name of + * + * In contrast to the other drm_get_*_name functions this one here returns a + * const pointer and hence is threadsafe. + */ +const char *drm_get_colorspace_name(enum drm_colorspace colorspace) +{ + if (colorspace < ARRAY_SIZE(colorspace_names) && colorspace_names[colorspace]) + return colorspace_names[colorspace]; + else + return "(null)"; +} + static const u32 hdmi_colorspaces = BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) | BIT(DRM_MODE_COLORIMETRY_BT709_YCC) | diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index fad38cdf4f79..9d9e4d6f0449 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1988,6 +1988,7 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter); bool drm_connector_has_possible_encoder(struct drm_connector *connector, struct drm_encoder *encoder); +const char *drm_get_colorspace_name(enum drm_colorspace colorspace); /** * drm_for_each_connector_iter - connector_list iterator macro -- 2.41.0
[PATCH v6 02/13] drm/connector: Add enum documentation to drm_colorspace
From: Joshua Ashton To match the other enums, and add more information about these values. v2: - Specify where an enum entry comes from - Clarify DEFAULT and NO_DATA behavior - BT.2020 CYCC is "constant luminance" - correct type for BT.601 v4: - drop DP/HDMI clarifications that might create more questions than answers v5: - Add note on YCC and RGB variants Signed-off-by: Joshua Ashton Signed-off-by: Harry Wentland Reviewed-by: Harry Wentland Reviewed-by: Sebastian Wick Acked-by: Pekka Paalanen Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- include/drm/drm_connector.h | 70 +++-- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 77401e425341..907f40851e80 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -363,13 +363,79 @@ enum drm_privacy_screen_status { PRIVACY_SCREEN_ENABLED_LOCKED, }; -/* - * This is a consolidated colorimetry list supported by HDMI and +/** + * enum drm_colorspace - color space + * + * This enum is a consolidated colorimetry list supported by HDMI and * DP protocol standard. The respective connectors will register * a property with the subset of this list (supported by that * respective protocol). Userspace will set the colorspace through * a colorspace property which will be created and exposed to * userspace. + * + * DP definitions come from the DP v2.0 spec + * HDMI definitions come from the CTA-861-H spec + * + * A note on YCC and RGB variants: + * + * Since userspace is not aware of the encoding on the wire + * (RGB or YCbCr), drivers are free to pick the appropriate + * variant, regardless of what userspace selects. E.g., if + * BT2020_RGB is selected by userspace a driver will pick + * BT2020_YCC if the encoding on the wire is YUV444 or YUV420. + * + * @DRM_MODE_COLORIMETRY_DEFAULT: + * Driver specific behavior. + * @DRM_MODE_COLORIMETRY_NO_DATA: + * Driver specific behavior. + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC: + * (HDMI) + * SMPTE ST 170M colorimetry format + * @DRM_MODE_COLORIMETRY_BT709_YCC: + * (HDMI, DP) + * ITU-R BT.709 colorimetry format + * @DRM_MODE_COLORIMETRY_XVYCC_601: + * (HDMI, DP) + * xvYCC601 colorimetry format + * @DRM_MODE_COLORIMETRY_XVYCC_709: + * (HDMI, DP) + * xvYCC709 colorimetry format + * @DRM_MODE_COLORIMETRY_SYCC_601: + * (HDMI, DP) + * sYCC601 colorimetry format + * @DRM_MODE_COLORIMETRY_OPYCC_601: + * (HDMI, DP) + * opYCC601 colorimetry format + * @DRM_MODE_COLORIMETRY_OPRGB: + * (HDMI, DP) + * opRGB colorimetry format + * @DRM_MODE_COLORIMETRY_BT2020_CYCC: + * (HDMI, DP) + * ITU-R BT.2020 Y'c C'bc C'rc (constant luminance) colorimetry format + * @DRM_MODE_COLORIMETRY_BT2020_RGB: + * (HDMI, DP) + * ITU-R BT.2020 R' G' B' colorimetry format + * @DRM_MODE_COLORIMETRY_BT2020_YCC: + * (HDMI, DP) + * ITU-R BT.2020 Y' C'b C'r colorimetry format + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65: + * (HDMI) + * SMPTE ST 2113 P3D65 colorimetry format + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER: + * (HDMI) + * SMPTE ST 2113 P3DCI colorimetry format + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED: + * (DP) + * RGB wide gamut fixed point colorimetry format + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT: + * (DP) + * RGB wide gamut floating point + * (scRGB (IEC 61966-2-2)) colorimetry format + * @DRM_MODE_COLORIMETRY_BT601_YCC: + * (DP) + * ITU-R BT.601 colorimetry format + * The DP spec does not say whether this is the 525 or the 625 + * line version. */ enum drm_colorspace { /* For Default case, driver will set the colorspace */ -- 2.41.0
[PATCH v6 01/13] drm/connector: Convert DRM_MODE_COLORIMETRY to enum
This allows us to use strongly typed arguments. v2: - Bring NO_DATA back - Provide explicit enum values v3: - Drop unnecessary '&' from kerneldoc (emersion) v4: - Fix Normal Colorimetry comment Signed-off-by: Harry Wentland Reviewed-by: Simon Ser Reviewed-by: Sebastian Wick Reviewed-by: Pekka Paalanen Reviewed-by: Joshua Ashton Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Simon Ser Cc: Ville Syrjälä Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- include/drm/display/drm_dp.h | 2 +- include/drm/drm_connector.h | 49 ++-- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h index f1be179c5f1f..7f858352cb43 100644 --- a/include/drm/display/drm_dp.h +++ b/include/drm/display/drm_dp.h @@ -1626,7 +1626,7 @@ enum dp_pixelformat { * * This enum is used to indicate DP VSC SDP Colorimetry formats. * It is based on DP 1.4 spec [Table 2-117: VSC SDP Payload for DB16 through - * DB18] and a name of enum member follows DRM_MODE_COLORIMETRY definition. + * DB18] and a name of enum member follows enum drm_colorimetry definition. * * @DP_COLORIMETRY_DEFAULT: sRGB (IEC 61966-2-1) or * ITU-R BT.601 colorimetry format diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 565cf9d3c550..77401e425341 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -371,29 +371,30 @@ enum drm_privacy_screen_status { * a colorspace property which will be created and exposed to * userspace. */ - -/* For Default case, driver will set the colorspace */ -#define DRM_MODE_COLORIMETRY_DEFAULT 0 -/* CEA 861 Normal Colorimetry options */ -#define DRM_MODE_COLORIMETRY_NO_DATA 0 -#define DRM_MODE_COLORIMETRY_SMPTE_170M_YCC1 -#define DRM_MODE_COLORIMETRY_BT709_YCC 2 -/* CEA 861 Extended Colorimetry Options */ -#define DRM_MODE_COLORIMETRY_XVYCC_601 3 -#define DRM_MODE_COLORIMETRY_XVYCC_709 4 -#define DRM_MODE_COLORIMETRY_SYCC_601 5 -#define DRM_MODE_COLORIMETRY_OPYCC_601 6 -#define DRM_MODE_COLORIMETRY_OPRGB 7 -#define DRM_MODE_COLORIMETRY_BT2020_CYCC 8 -#define DRM_MODE_COLORIMETRY_BT2020_RGB9 -#define DRM_MODE_COLORIMETRY_BT2020_YCC10 -/* Additional Colorimetry extension added as part of CTA 861.G */ -#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D6511 -#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER12 -/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */ -#define DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED13 -#define DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT14 -#define DRM_MODE_COLORIMETRY_BT601_YCC 15 +enum drm_colorspace { + /* For Default case, driver will set the colorspace */ + DRM_MODE_COLORIMETRY_DEFAULT= 0, + /* CEA 861 Normal Colorimetry options */ + DRM_MODE_COLORIMETRY_NO_DATA= 0, + DRM_MODE_COLORIMETRY_SMPTE_170M_YCC = 1, + DRM_MODE_COLORIMETRY_BT709_YCC = 2, + /* CEA 861 Extended Colorimetry Options */ + DRM_MODE_COLORIMETRY_XVYCC_601 = 3, + DRM_MODE_COLORIMETRY_XVYCC_709 = 4, + DRM_MODE_COLORIMETRY_SYCC_601 = 5, + DRM_MODE_COLORIMETRY_OPYCC_601 = 6, + DRM_MODE_COLORIMETRY_OPRGB = 7, + DRM_MODE_COLORIMETRY_BT2020_CYCC= 8, + DRM_MODE_COLORIMETRY_BT2020_RGB = 9, + DRM_MODE_COLORIMETRY_BT2020_YCC = 10, + /* Additional Colorimetry extension added as part of CTA 861.G */ + DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65 = 11, + DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER = 12, + /* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */ + DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED = 13, + DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT = 14, + DRM_MODE_COLORIMETRY_BT601_YCC = 15, +}; /** * enum drm_bus_flags - bus_flags info for _display_info @@ -828,7 +829,7 @@ struct drm_connector_state { * colorspace change on Sink. This is most commonly used to switch * to wider color gamuts like BT2020. */ - u32 colorspace; + enum drm_colorspace colorspace; /** * @writeback_job: Writeback job for writeback connectors -- 2.41.0
[PATCH v6 00/13] Enable Colorspace connector property in amdgpu
This patchset is based on Joshua's previous patchset [1], as well as my previous patchset [2]. It is - enabling support for the colorspace property in amdgpu, as well as - allowing drivers to specify the supported set of colorspaces, and Colorspace, Infoframes, and YCbCr matrix --- Even though the initial intent of the colorspace property was to set the colorspace field in the respective HDMI AVI and DP SDP infoframes that is not sufficient in all scenarios. For DP the colorspace information also affects the MSA (main stream attribute) packet. For YUV output the colorspace affects the RGB-to-YCbCr conversion matrix. The colorspace field of the infopackets also depends on the encoding used, which is something that is decided by the driver and not known to userspace. For these reasons a driver will need to be able to select the supported colorspaces at property creation. Note: There seems to be an understanding that the colorspace property should ONLY modify the infoframe. While this is current behavior and sufficient in some cases it is nowhere specified that this should be the only use of this property. As outlined above this limitation is not going to work in all cases. This patchset does not affect current behavior for the drivers that implement this property: i915 and vc4. In the future we might want to give userspace control over the encoding format on the wire, in particular to avoid use of YUV420 when image fidelity is important. This work would likely go hand in hand with a min_bpc property and wouldn't conflict with the work done in this patchset. I would expect this future work to tag along with a drm_crtc or drm_connector's Color Pipeline, similar to the one propsed for drm_plane [3]. Colorspace on crtc or connector? There have been suggestions of programming 'colorspace' on the drm_crtc but I don't think the crtc is the right place for this property. The drm_plane and drm_crtc will be used to offload color processing that would normally be done via the GFX or other pipelines. The drm_connector controls the signalling with the display and ensures the wire format is appropriate for the encoding by programming the RGB-to-YCbCr matrix. [1] https://patchwork.freedesktop.org/series/113632/ [2] https://patchwork.freedesktop.org/series/111865/ [3] https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html v2: - Tested with DP and HDMI analyzers - Confirmed driver will fallback to lower bpc when needed - Dropped hunk to set HDMI AVI infoframe as it was a no-op - Fixed BT.2020 YCbCr colorimetry (JoshuaAshton) - Simplify initialization of supported colorspaces (Jani) - Fix kerneldoc (kernel test robot) v3: - Added documentation for colorspaces (Pekka, Joshua) - Split 'Allow drivers to pass list of supported colorspaces' patch to pull out code to create common colorspace array and keep it separate from change to create only supported colorspaces v4: - Don't "deprecate" existing enum values - Fixes based on review comments throughout - Dropped Josh's RBs v5: - Add documentation that drivers are free to pick appropriate RGB or YCC variant v6: - Remove magic when drivers pass '0' as supported_colorspaces to indicate default support for all DP/HDMI colorspaces Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: Jani Nikula Cc: Michel Dänzer Cc: Simon Ser Cc: Melissa Wen Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org Harry Wentland (10): drm/connector: Convert DRM_MODE_COLORIMETRY to enum drm/connector: Pull out common create_colorspace_property code drm/connector: Use common colorspace_names array drm/connector: Print connector colorspace in state debugfs drm/connector: Allow drivers to pass list of supported colorspaces drm/amd/display: Always pass connector_state to stream validation drm/amd/display: Register Colorspace property for DP and HDMI drm/amd/display: Signal mode_changed if colorspace changed drm/amd/display: Send correct DP colorspace infopacket drm/amd/display: Add debugfs for testing output colorspace Joshua Ashton (3): drm/connector: Add enum documentation to drm_colorspace drm/amd/display: Always set crtcinfo from create_stream_for_sink drm/amd/display: Refactor avi_info_frame colorimetry determination .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 84 +--- .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 57 ++ .../gpu/drm/amd/display/dc/core/dc_resource.c | 28 ++- drivers/gpu/drm/drm_atomic.c | 1 + drivers/gpu/drm/drm_connector.c | 189 +++--- .../gpu/drm/i915/display/intel_connector.c| 4 +- drivers/gpu/drm/vc4/vc4_hdmi.c| 2 +- include/drm/display/drm_dp.h | 2 +- include/drm/drm_connector.h | 129 +--- 9 files changed, 363
Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable
Hi Thomas, On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann wrote: > Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven: > > On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann > > wrote: > >> --- a/drivers/video/fbdev/Kconfig > >> +++ b/drivers/video/fbdev/Kconfig > >> @@ -57,6 +57,15 @@ config FIRMWARE_EDID > >>combination with certain motherboards and monitors are known to > >>suffer from this problem. > >> > >> +config FB_DEVICE > >> +bool "Provide legacy /dev/fb* device" > > > > Perhaps "default y if !DRM", although that does not help for a > > mixed drm/fbdev kernel build? > > We could simply set it to "default y". But OTOH is it worth making it a > default? Distributions will set it to the value they need/want. The very > few people that build their own kernels to get certain fbdev drivers > will certainly be able to enable the option by hand as well. Defaulting to "n" (the default) means causing regressions when these few people use an existing defconfig. > > Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE, > > to be selected by both FB and DRM_FBDEV_EMULATION? > > Then FB_DEVICE can depend on FB_CORE, and default to y if FB. > > That wouldn't work. In Tumbleweed, we still have efifb and vesafb > enabled under certain conditions; merely for the kernel console. We'd > have to enable CONFIG_FB, which would bring back the device. "Default y" does not mean that you cannot disable FB_DEVICE, so you are not forced to bring back the device? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable
Hi Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven: Hi Thomas, Thanks for your patch! On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann wrote: Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev device optional. If the new option has not been selected, fbdev does not create a files in devfs or sysfs. Most modern Linux systems run a DRM-based graphics stack that uses the kernel's framebuffer console, but has otherwise deprecated fbdev support. Yet fbdev userspace interfaces are still present. The option makes it possible to use the fbdev subsystem as console implementation without support for userspace. This closes potential entry points to manipulate kernel or I/O memory via framebuffers. It I'd leave out the part about manipulating kernel memory, as that's a driver bug, if possible. The driver/core distinction is somewhat fuzzy: the recent bug with OOB access was introduced accidentally in shared helper code within DRM. And whenever I want to modify the fbdev code, I have to start bugfixing first. Just look at this patchset. A good number of the patches are bugfixes. Driver or not, I no longer trust any of the fbdev code to get anything right. also prevents the execution of driver code via ioctl or sysfs, both of which might allow malicious software to exploit bugs in the fbdev code. Of course disabling ioctls reduces the attack surface, and this is not limited to fbdev... ;-) Other subsystems should do the same where possible. The specific problem with DRM-plus-fbdev is that the fbdev device doesn't provide any additional value. It's too limited in functionality (even by fbdev standards), a possible source for bugs, and today's userspace wants DRM functionality. I'm wondering if it would be worthwhile to optionally provide a more limited userspace API for real fbdev drivers: 1. No access to MMIO, only to the mapped frame buffer, 2. No driver-specific ioctls, only the standard ones. That issue is only relevant to fbdev drivers and would be a separate patchset. My concern lies with the current distributions, which don't need the fbdev device and shouldn't provide it for the given reasons. A small number of fbdev drivers require struct fbinfo.dev to be initialized, usually for the support of sysfs interface. Make these drivers depend on FB_DEVICE. They can later be fixed if necessary. Signed-off-by: Thomas Zimmermann --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -57,6 +57,15 @@ config FIRMWARE_EDID combination with certain motherboards and monitors are known to suffer from this problem. +config FB_DEVICE +bool "Provide legacy /dev/fb* device" Perhaps "default y if !DRM", although that does not help for a mixed drm/fbdev kernel build? We could simply set it to "default y". But OTOH is it worth making it a default? Distributions will set it to the value they need/want. The very few people that build their own kernels to get certain fbdev drivers will certainly be able to enable the option by hand as well. Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE, to be selected by both FB and DRM_FBDEV_EMULATION? Then FB_DEVICE can depend on FB_CORE, and default to y if FB. That wouldn't work. In Tumbleweed, we still have efifb and vesafb enabled under certain conditions; merely for the kernel console. We'd have to enable CONFIG_FB, which would bring back the device. Best regards Thomas +depends on FB +help + Say Y here if you want the legacy /dev/fb* device file. It's + only required if you have userspace programs that depend on + fbdev for graphics output. This does not effect the framebuffer affect + console. + config FB_DDC tristate depends on FB Gr{oetje,eeting}s, Geert -- 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) OpenPGP_signature Description: OpenPGP digital signature
[PATCH v4 1/3] dt-bindings: add fannal vendor prefix
Fannal is a one-stop-solution provider for touch displays in industrial application, committed to delivering a variety of professional high-quality products and technical services globally. Website: www.fannal.com Signed-off-by: Paulo Pavacic Acked-by: Krzysztof Kozlowski --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 82d39ab0231b..f962750f630a 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -462,6 +462,8 @@ patternProperties: description: Facebook "^fairphone,.*": description: Fairphone B.V. + "^fannal,.*": +description: Fannal Electronics Co., Ltd "^faraday,.*": description: Faraday Technology Corporation "^fastrax,.*": -- 2.40.1
[PATCH v4 3/3] drm/panel-fannal-c3004: Add fannal c3004 DSI panel
Fannal C3004 is a 480x800 display made by fannal that requires DCS initialization sequences. Signed-off-by: Paulo Pavacic --- v4 changelog: - removal of blank lines, code wrapping changes - reset pin initialization handling changes v3 changelog: - formatting and style changes - remove BIT_NONEXCLUSIVE flag from reset pin - use dev_err_probe instead of dev_err v2 changelog: - using generic mipi_dsi_dcs_write_seq - removed success prints - removed some comments - simplified code/removed support for different panels - changed namespace from fann to fannal v1 changelog: - renamed from panel-mipi-dsi-bringup - only one MAINTAINER e-mail --- MAINTAINERS| 1 + drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-fannal-c3004.c | 314 + 4 files changed, 327 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-fannal-c3004.c diff --git a/MAINTAINERS b/MAINTAINERS index 978133f87c5e..7ad37ecd7e09 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6431,6 +6431,7 @@ DRM DRIVER FOR FANNAL C3004373132019A M: Paulo Pavacic S: Maintained F: Documentation/devicetree/bindings/display/panel/panel-fannal,c3004.yaml +F: drivers/gpu/drm/panel/panel-fannal-c3004.c DRM DRIVER FOR FARADAY TVE200 TV ENCODER M: Linus Walleij diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 2b9d6db7860b..a1041c1e6bf6 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -144,6 +144,17 @@ config DRM_PANEL_ELIDA_KD35T133 KD35T133 controller for 320x480 LCD panels with MIPI-DSI system interfaces. +config DRM_PANEL_FANNAL_C3004 + tristate "Fannal C3004 panel" + depends on OF + depends on DRM_MIPI_DSI + help + Say Y here if you want to enable support for the Fannal C3004 + 2-lane 480x800 MIPI DSI panel which requires initialization + sequence. + + If M is selected the module will be called panel-fannal-c3004. + config DRM_PANEL_FEIXIN_K101_IM2BA02 tristate "Feixin K101 IM2BA02 panel" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index ff169781e82d..13c0f00038b5 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o obj-$(CONFIG_DRM_PANEL_EDP) += panel-edp.o obj-$(CONFIG_DRM_PANEL_EBBG_FT8719) += panel-ebbg-ft8719.o obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o +obj-$(CONFIG_DRM_PANEL_FANNAL_C3004) += panel-fannal-c3004.o obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.o obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o obj-$(CONFIG_DRM_PANEL_HIMAX_HX8394) += panel-himax-hx8394.o diff --git a/drivers/gpu/drm/panel/panel-fannal-c3004.c b/drivers/gpu/drm/panel/panel-fannal-c3004.c new file mode 100644 index ..002c424f2486 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-fannal-c3004.c @@ -0,0 +1,314 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * MIPI DSI driver for fannal C3004. + * Copyright (C) 2023, Zenitel + * Author: Paulo Pavacic + */ + +// ↓ include headers, static values, static functions ↓ +#include +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include + +struct fannal_panel_data { + struct drm_panel panel; + struct gpio_desc *reset; +}; + +static struct fannal_panel_data * +get_fannal_panel_data_from_panel(struct drm_panel *panel) +{ + return container_of(panel, struct fannal_panel_data, panel); +} + +static const u32 fannal_bus_formats[] = { + MEDIA_BUS_FMT_RGB888_1X24, +}; + +// resolution 480p x 800p, 56mmx93mm +static const struct drm_display_mode fannal_c3004_display_mode = { + .clock = 27000, + .hdisplay = 480, // display height pixels + .hsync_start = 480 + 30, // hdisplay + HBP + .hsync_end = 480 + 30 + 8, // hdisplay + HBP + HSync + .htotal = 480 + 30 + 8 + 30, // hdisplay + HBP + HSync + HFP + .vdisplay = 800, // display width pixels + .vsync_start = 800 + 20, // vdisplay + VBP + .vsync_end = 800 + 20 + 8, // vdisplay + VBP + VSync + .vtotal = 800 + 20 + 8 + 20, // vdisplay + VBP + VSync + VFP + .width_mm = 93, + .height_mm = 56, + .flags = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, +}; + +static void fannal_panel_remove(struct mipi_dsi_device *dsi) +{ + struct fannal_panel_data *panel_data = mipi_dsi_get_drvdata(dsi); + struct device *dev = >dev; + int ret; + + ret = mipi_dsi_detach(dsi); + if (ret) + dev_err(dev, "error: disable: mipi detach (%d)\n", ret); + + drm_panel_remove(_data->panel); +} + +static int fannal_panel_disable(struct drm_panel *panel) +{ +
[PATCH v4 2/3] dt-bindings: display: panel: add fannal,c3004
Added fannal to vendor-prefixes and dt bindings for Fannal C3004. Fannal C3004 is a 480x800 MIPI DSI Panel which requires DCS initialization sequences with certain delays between certain commands. Signed-off-by: Paulo Pavacic Reviewed-by: Krzysztof Kozlowski --- v4 changelog: - removed driver's community from MAINTAINERS file v2 changelog: - added empty lines between properties in yml - renamed yml file - refactored yml file to describe fannal,c3004 - added matrix URI to MAINTAINERS v1 changelog: - revised driver title, now describes purpose - revised description, now describes hw - revised maintainers, now has only 1 mail - removed diacritics from commit/commit author - properties/compatible is now enum - compatible using only lowercase - revised dts example - modified MAINTAINERS in this commit (instead of driver commit) - dt_bindings_check checked yml - checkpatch warning fixed --- .../bindings/display/panel/fannal,c3004.yaml | 78 +++ MAINTAINERS | 5 ++ 2 files changed, 83 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml diff --git a/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml b/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml new file mode 100644 index ..bbddb036094b --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml @@ -0,0 +1,78 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/fannal,c3004.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Fannal C3004 MIPI-DSI + +maintainers: + - Paulo Pavacic + +description: | + Fannal C3004 is a 480x800 panel which requires DSI DCS + initialization sequences. + +allOf: + - $ref: panel-common.yaml# + +properties: + compatible: +items: + - const: fannal,c3004 + + reg: true + + reset-gpios: true + + vdd-supply: +description: power supply voltage + + vddio-supply: +description: power supply voltage for IO + + width-mm: +description: physical panel width [mm] + + height-mm: +description: physical panel height [mm] + + panel-timing: true + +required: + - compatible + - reg + - reset-gpios + +additionalProperties: false + +examples: + - | +#include +dsi { +#address-cells = <1>; +#size-cells = <0>; +panel@0 { +compatible = "fannal,c3004"; +reg = <0>; +pinctrl-0 = <_mipi_dsi_rst>; +pinctrl-names = "default"; +reset-gpios = < 9 GPIO_ACTIVE_LOW>; +vdd-supply = <>; +vddio-supply = <>; +width-mm = <93>; +height-mm = <56>; +panel-timing { +clock-frequency = <2700>; +hactive = <480>; +vactive = <800>; +hfront-porch = <30>; +hback-porch = <30>; +hsync-len = <8>; +vback-porch = <30>; +vfront-porch = <30>; +vsync-len = <8>; +}; +}; +}; +... diff --git a/MAINTAINERS b/MAINTAINERS index 5c22c828ab46..978133f87c5e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6427,6 +6427,11 @@ T: git git://anongit.freedesktop.org/drm/drm-misc F: Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml F: drivers/gpu/drm/panel/panel-ebbg-ft8719.c +DRM DRIVER FOR FANNAL C3004373132019A +M: Paulo Pavacic +S: Maintained +F: Documentation/devicetree/bindings/display/panel/panel-fannal,c3004.yaml + DRM DRIVER FOR FARADAY TVE200 TV ENCODER M: Linus Walleij S: Maintained -- 2.40.1
[PATCH v4 0/3] drm/panel: add fannal c3004 panel
Fannal C3004 is a 2 lane MIPI DSI 480x800 panel which requires initialization with DSI DCS commands. After few initialization commands delay is required. Paulo Pavacic (3): dt-bindings: add fannal vendor prefix dt-bindings: display: panel: add fannal,c3004 drm/panel-fannal-c3004: Add fannal c3004 DSI panel .../bindings/display/panel/fannal,c3004.yaml | 78 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 6 + drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile| 1 + drivers/gpu/drm/panel/panel-fannal-c3004.c| 314 ++ 6 files changed, 412 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml create mode 100644 drivers/gpu/drm/panel/panel-fannal-c3004.c -- 2.40.1
Re: [PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value
On 6/7/23 14:53, Lucas Stach wrote: Am Freitag, dem 02.06.2023 um 23:34 +0200 schrieb Marek Vasut: On 6/2/23 21:15, Lucas Stach wrote: From: David Jander The documentation is not clear about how this delay works. Empirical tests have shown that with a VSDELAY of 0, the first scanline is not properly formatted in the output stream when DSI->DP mode is used. The calculation spreadsheets from Toshiba seem to always make this value equal to the HFP + 10 for DSI->DP use-case. For DSI->DPI this value should be > 2 and for DPI->DP it seems to always be 0x64. Signed-off-by: David Jander Signed-off-by: Lucas Stach --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 46916ae30f8f..9f2c67b4a488 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc, * sync signals */ ret = regmap_write(tc->regmap, VPCTRL0, - FIELD_PREP(VSDELAY, 0) | + FIELD_PREP(VSDELAY, right_margin + 10) | OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED); if (ret) return ret; Aren't you running into a problem due to VS timing misconfiguration on the scanout engine or DSI serializer side ? The VSDELAY seems to increase the length of VSYNC active . No, as far as I understand the VSDELAY adds an offset between input an output side of the video FIFO. It shouldn't increase the length of any sync signal, but delays the read side of the FIFO, so the write (DSI) side has some margin to put data into the FIFO before DP side starts to assemble packets. Does this apply to DSI-to-DPI mode too ? Which DSI bus mode do you use, sync events/pulses/burst ? At the time when this patch was written it still was the SYNC_PULSE mode. Can you please double-check this with current burst mode ?
[PATCH v2] drm/panel-edp: Add AUO B116XAB01.4 edp panel entry
Add a panel entry for the AUO B116XAB01.4 edp panel, found in the Acer Chromebook Spin 311 (CP311-3H) laptop. Signed-off-by: Laura Nao --- Changes in v2: - Sorted by product ID drivers/gpu/drm/panel/panel-edp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index fbd114b4f0be..df7e3cff004c 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -1866,6 +1866,7 @@ static const struct panel_delay delay_200_500_e200 = { */ static const struct edp_panel_entry edp_panels[] = { EDP_PANEL_ENTRY('A', 'U', 'O', 0x1062, _200_500_e50, "B120XAN01.0"), + EDP_PANEL_ENTRY('A', 'U', 'O', 0x145c, _200_500_e50, "B116XAB01.4"), EDP_PANEL_ENTRY('A', 'U', 'O', 0x1e9b, _200_500_e50, "B133UAN02.1"), EDP_PANEL_ENTRY('A', 'U', 'O', 0x1ea5, _200_500_e50, "B116XAK01.6"), EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, "B116XAK01"), -- 2.30.2
Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
On Tue, Jun 06, 2023 at 12:00:42PM +0200, Andi Shyti wrote: > From: Fei Yang > > To comply with the design that buffer objects shall have immutable > cache setting through out their life cycle, {set, get}_caching ioctl's > are no longer supported from MTL onward. With that change caching > policy can only be set at object creation time. The current code > applies a default (platform dependent) cache setting for all objects. > However this is not optimal for performance tuning. The patch extends > the existing gem_create uAPI to let user set PAT index for the object > at creation time. > The new extension is platform independent, so UMD's can switch to using > this extension for older platforms as well, while {set, get}_caching are > still supported on these legacy paltforms for compatibility reason. > However, since PAT index was not clearly defined for platforms prior to > GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms > only. See ext_set_pat() in for the implementation details. > > Note: The documentation related to the PAT/MOCS tables is currently > available for Tiger Lake here: > https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/tiger-lake.html > > BSpec: 45101 > > Mesa support has been submitted in this merge request: > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878 > > The media driver is supported by the following commits: > https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341 > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd > https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000 > > The IGT test related to this change is > igt@gem_create@create-ext-set-pat > > Signed-off-by: Fei Yang > Cc: Chris Wilson > Cc: Matt Roper > Cc: Andi Shyti > Reviewed-by: Andi Shyti > Acked-by: Jordan Justen > Tested-by: Jordan Justen > Acked-by: Carl Zhang > Tested-by: Lihao Gu > Signed-off-by: Andi Shyti I'm also adding: Acked-by: Slawomir Milczarek from the Compute UMD team. Thanks, Slawek! Andi > --- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 40 + > drivers/gpu/drm/i915/gem/i915_gem_object.c | 6 > include/uapi/drm/i915_drm.h| 41 ++ > 3 files changed, 87 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c > b/drivers/gpu/drm/i915/gem/i915_gem_create.c > index bfe1dbda4cb75..d24c0ce8805c7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > @@ -245,6 +245,7 @@ struct create_ext { > unsigned int n_placements; > unsigned int placement_mask; > unsigned long flags; > + unsigned int pat_index; > }; > > static void repr_placements(char *buf, size_t size, > @@ -394,11 +395,43 @@ static int ext_set_protected(struct i915_user_extension > __user *base, void *data > return 0; > } > > +static int ext_set_pat(struct i915_user_extension __user *base, void *data) > +{ > + struct create_ext *ext_data = data; > + struct drm_i915_private *i915 = ext_data->i915; > + struct drm_i915_gem_create_ext_set_pat ext; > + unsigned int max_pat_index; > + > + BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) != > + offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd)); > + > + /* Limiting the extension only to Meteor Lake */ > + if (!IS_METEORLAKE(i915)) > + return -ENODEV; > + > + if (copy_from_user(, base, sizeof(ext))) > + return -EFAULT; > + > + max_pat_index = INTEL_INFO(i915)->max_pat_index; > + > + if (ext.pat_index > max_pat_index) { > + drm_dbg(>drm, "PAT index is invalid: %u\n", > + ext.pat_index); > + return -EINVAL; > + } > + > + ext_data->pat_index = ext.pat_index; > + > + return 0; > +} > + > static const i915_user_extension_fn create_extensions[] = { > [I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements, > [I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected, > + [I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat, > }; > > +#define PAT_INDEX_NOT_SET0x > /** > * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle > to it. > * @dev: drm device pointer > @@ -418,6 +451,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void > *data, > if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS) > return -EINVAL; > > + ext_data.pat_index = PAT_INDEX_NOT_SET; > ret = i915_user_extensions(u64_to_user_ptr(args->extensions), > create_extensions, > ARRAY_SIZE(create_extensions), > @@ -454,5 +488,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void > *data, > if (IS_ERR(obj)) > return PTR_ERR(obj); > > + if
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote: > > @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config( > > * frequency divided by the no. of rows (lines) in the LCDpanel. > > */ > > vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync"); > > - if (vsync_hz <= 0) { > > - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n", > > + if (!vsync_hz) { > > + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n", > > vsync_hz); > > Nit: no need to print the value here, you know it's zero. Could be > clarified to just "no vsync clock". > Yeah. That's obviously not useful. Sorry, I will resend. regards, dan carpenter
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. While my patch is trying to create a *mechanism* which could probably works on all platform. It is based the patch you have already commuted. Thanks for your excellent contribution. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. Please do not mitigate the problems with confusing method. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific thing. Dependent on how does the arch_dma_ops is implemented. The definition of the coherent on different ARCH has different meanings. The definition of the wirte-combine on different ARCH has different meanings. The wirte-combine(uncache acceleration) on mips is non dma-coherent. It is dma-coherent on Ingenic SoCs. It is dma-coherent ? How does it achieve it? As far as I know, there is a write buffer within the mips cpu. typically 64 byte, but it is not cache. It will gather the CPU write access, When a peripheral device do the DMA, how does you platform guarantee the data in the CPU write buffer has been already arrived at (or flushed out to) the system RAM? Does the peripheral device snoop the CPU's write buffer, or it need manually flush the write buffer with SYNC instruction? But on arm, It seem that wirte-combine is coherent. (guaranteed by arch implement). I also heard using dma_alloc_coherent to allocation the buffer for the non-coherent doesn't hurt, but the reverse is not true. But please do not create confusion. software composite is faster because better cacheusing rate and cache is faster to read. It is faster because it is cached, not because it is non-coherent. non-coherent is arch thing and/or driver-side thing, it is a side effect of using the cached mapping. Yes, I know that. It should left to driver to handle such a side effect. The device driver know their device, so its the device driver's responsibility to maintain the coherency. On loongson platform, we don't need to call drm_fb_dma_sync_non_coherent() function, Its already guaranteed by hardware. I understand. What I'm saying, is that you
Re: [PATCH V8 0/7] drm: bridge: samsung-dsim: Support variable clocking
On Wed, Jun 7, 2023 at 8:27 AM Adam Ford wrote: > > On Wed, Jun 7, 2023 at 8:15 AM Rasmus Villemoes > wrote: > > > > On 26/05/2023 05.05, Adam Ford wrote: > > > This series fixes the blanking pack size and the PMS calculation. It then > > > adds support to allows the DSIM to dynamically DPHY clocks, and support > > > non-burst mode while allowing the removal of the hard-coded clock values > > > for the PLL for imx8m mini/nano/plus, and it allows the removal of the > > > burst-clock device tree entry when burst-mode isn't supported by connected > > > devices like an HDMI brige. In that event, the HS clock is set to the > > > value requested by the bridge chip. > > > > > > This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should > > > work on i.MX8M Mini as well. Marek Szyprowski has tested it on various > > > Exynos boards. > > > > Hi all > > > > We're testing this on top of v6.4-rc4 on our imx8mp board, which has a > > ti-sn65dsi86 DSI -> DisplayPort bridge. We do get an image at > > 1920x1200, but the monitor says it's only at 58Hz, and measuring on the > > DSI signals does seem to confirm that the update frequency is about 57.7 > > or 57.8Hz (it's pretty hard to get a good measurement). It looks like > > it's the lines that are too long, by a time that corresponds to about 80 > > pixels. But all the frontporch/backporch/hsync values look sane and > > completely standard for that resolution. > > > > Setting samsung,burst-clock-frequency explicitly to something large > > enough or letting it be derived from the 154MHz pixel clock makes no > > difference. > > > > Any ideas? > > What refresh rate are you trying to achieve? It seems like 57.7 or > 57.8 is really close to the 58 the Monitor states. I would expect the > refresh to be driven by whatever the monitor states it can handle. > > Have you tried using modetest to see what refresh rates are available? > When I was doing this driver work, I would use modetest to determine > the connector ID, then use modetest -s > :- to display various resolutions > and refresh rates. > > The 8MP shares the video-pll clock with both disp1 and disp2 clocks, > and the imx-lcdif driver, which sends the display signals to the DSI, > uses the disp clock, so the video-pll needs to be an exact multiple of > the pixel clock or the output won't sink. Modetest should also show > you the desired pixel clock for a given resolution and refresh. > My displays didn't show 19200x1200 as an option, so I wasn't able to > test that configuration. Another thing you could try would be this rounding patch that I'm experimenting with [1]. >From what I can see, some resolutions end up with math that end up rounding down, and this patch corrects the timings a bit to attempt to compensate. I haven't tested this extensively yet, but you can try it to see if it helps. adam [1] - https://github.com/aford173/linux/commit/183cf6d154afeb9b0300500b09d7b8ec53047a12 > > adam > > > > Thanks, > > Rasmus > >
Re: [PATCH 00/30] fbdev: Make userspace interfaces optional
> Ha! It says 'distrobutions'. Do you tend to prefer any distributions? Regards, Markus
Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Avoid possible buffer overflow
Hi, On Tue, Jun 6, 2023 at 6:25 PM Su Hui wrote: > > Smatch error:buffer overflow 'ti_sn_bridge_refclk_lut' 5 <= 5. > > Fixes: cea86c5bb442 ("drm/bridge: ti-sn65dsi86: Implement the pwm_chip") > Signed-off-by: Su Hui > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 7a748785c545..bb88406495e9 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -305,7 +305,7 @@ static void ti_sn_bridge_set_refclk_freq(struct > ti_sn65dsi86 *pdata) > * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG, > * regardless of its actual sourcing. > */ > - pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i]; > + pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i < refclk_lut_size > ? i : 1]; This looks more correct, but it really needs a comment since it's totally not obviously what you're doing here. IMO the best solution here is to update "i" right after the for loop and have a comment about the datasheet saying that "1" is the default rate so we'll fall back to that if we couldn't find a match. Moving it to right after the for loop will change the value written into the registers, but that's fine and makes it clearer what's happening. -Doug On Tue, Jun 6, 2023 at 6:25 PM Su Hui wrote: > > Smatch error:buffer overflow 'ti_sn_bridge_refclk_lut' 5 <= 5. > > Fixes: cea86c5bb442 ("drm/bridge: ti-sn65dsi86: Implement the pwm_chip") > Signed-off-by: Su Hui > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 7a748785c545..bb88406495e9 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -305,7 +305,7 @@ static void ti_sn_bridge_set_refclk_freq(struct > ti_sn65dsi86 *pdata) > * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG, > * regardless of its actual sourcing. > */ > - pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i]; > + pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i < refclk_lut_size > ? i : 1]; > } > > static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) > -- > 2.30.2 >