Re: [PATCH v5 11/11] drm/mediatek: gamma: Program gamma LUT type for descending or rising
Hi AngeloGioacchino, kernel test robot noticed the following build warnings: https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/AngeloGioacchino-Del-Regno/drm-mediatek-gamma-Adjust-mtk_drm_gamma_set_common-parameters/20230601-222357 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20230601121022.2401844-12-angelogioacchino.delregno%40collabora.com patch subject: [PATCH v5 11/11] drm/mediatek: gamma: Program gamma LUT type for descending or rising config: arm64-randconfig-m031-20230608 (https://download.01.org/0day-ci/archive/20230610/202306101458.lrxhee0z-...@intel.com/config) compiler: aarch64-linux-gcc (GCC) 12.3.0 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 | Reported-by: Dan Carpenter | Closes: https://lore.kernel.org/r/202306101458.lrxhee0z-...@intel.com/ smatch warnings: drivers/gpu/drm/mediatek/mtk_disp_gamma.c:192 mtk_gamma_set_common() error: we previously assumed 'gamma->data' could be null (see line 120) vim +192 drivers/gpu/drm/mediatek/mtk_disp_gamma.c 4873468a82b553 Jason-JH.Lin 2023-06-01 103 void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state) 69a4237ab1d13a Yongqiang Niu 2021-01-29 104 { 4873468a82b553 Jason-JH.Lin 2023-06-01 105 struct mtk_disp_gamma *gamma = dev_get_drvdata(dev); b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 106 void __iomem *lut0_base = regs + DISP_GAMMA_LUT; b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 107 void __iomem *lut1_base = regs + DISP_GAMMA_LUT1; b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 108 u32 cfg_val, data_mode, lbank_val, word[2]; b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 109 int cur_bank, num_lut_banks; b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 110 u16 lut_bank_size, lut_size; 69a4237ab1d13a Yongqiang Niu 2021-01-29 111 struct drm_color_lut *lut; b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 112 unsigned int i; 4873468a82b553 Jason-JH.Lin 2023-06-01 113 bool lut_diff; 1829ae02cf6bb6 AngeloGioacchino Del Regno 2023-06-01 114 u8 lut_bits; 69a4237ab1d13a Yongqiang Niu 2021-01-29 115 e824bd353592b5 AngeloGioacchino Del Regno 2023-06-01 116 /* If there's no gamma lut there's nothing to do here. */ e824bd353592b5 AngeloGioacchino Del Regno 2023-06-01 117 if (!state->gamma_lut) e824bd353592b5 AngeloGioacchino Del Regno 2023-06-01 118 return; e824bd353592b5 AngeloGioacchino Del Regno 2023-06-01 119 ca340e013e3733 AngeloGioacchino Del Regno 2023-06-01 @120 if (gamma && gamma->data) { ^^^ This code assumes "gamma->data" can be NULL 4873468a82b553 Jason-JH.Lin 2023-06-01 121 lut_diff = gamma->data->lut_diff; a26000d47a579c AngeloGioacchino Del Regno 2023-06-01 122 lut_bank_size = gamma->data->lut_bank_size; 1829ae02cf6bb6 AngeloGioacchino Del Regno 2023-06-01 123 lut_bits = gamma->data->lut_bits; ca340e013e3733 AngeloGioacchino Del Regno 2023-06-01 124 lut_size = gamma->data->lut_size; ca340e013e3733 AngeloGioacchino Del Regno 2023-06-01 125 } else { 4873468a82b553 Jason-JH.Lin 2023-06-01 126 lut_diff = false; a26000d47a579c AngeloGioacchino Del Regno 2023-06-01 127 lut_bank_size = LUT_SIZE_DEFAULT; 1829ae02cf6bb6 AngeloGioacchino Del Regno 2023-06-01 128 lut_bits = LUT_BITS_DEFAULT; ca340e013e3733 AngeloGioacchino Del Regno 2023-06-01 129 lut_size = LUT_SIZE_DEFAULT; ca340e013e3733 AngeloGioacchino Del Regno 2023-06-01 130 } a26000d47a579c AngeloGioacchino Del Regno 2023-06-01 131 num_lut_banks = lut_size / lut_bank_size; 4873468a82b553 Jason-JH.Lin 2023-06-01 132 ee2cb37b9ac9e2 AngeloGioacchino Del Regno 2023-06-01 133 cfg_val = readl(regs + DISP_GAMMA_CFG); 69a4237ab1d13a Yongqiang Niu 2021-01-29 134 lut = (struct drm_color_lut *)state->gamma_lut->data; a26000d47a579c AngeloGioacchino Del Regno 2023-06-01 135 b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 136 /* Switch to 12 bits data mode if supported */ b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 137 data_mode = FIELD_PREP(DISP_GAMMA_BANK_DATA_MODE, !!(lut_bits == 12)); b13cb453a8db5d AngeloGioacchino Del Regno 2023-06-01 138 a26000d47a579c AngeloGioacchino Del Regno 2023-06-01 139 for (cur_bank = 0; cur_bank < num_lut_banks; cur_bank++) { a26000d47a579c AngeloGioacchino Del Regno 2023-06-01 140 a26000d4
Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu
On Sat, 10 Jun 2023, Masahiro Yamada wrote: > On Sat, Jun 10, 2023 at 5:17 AM Nathan Chancellor wrote: >> >> + Masahiro and linux-kbuild >> >> On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: >> > We have a clean build with W=1 as of >> > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move >> > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable >> > these checks unconditionally for the entire module to catch these errors >> > during development. >> > >> > Cc: Alex Deucher >> > Cc: Nathan Chancellor >> > Signed-off-by: Hamza Mahfooz >> >> I think this is fine, especially since it will help catch issues in >> amdgpu quickly and hopefully encourage developers to fix their problems >> before they make it to a tree with wider impact lika -next. >> >> However, this is now the third place that W=1 has been effectively >> enabled (i915 and btrfs are the other two I know of) and it would be >> nice if this was a little more unified, especially since it is not >> uncommon for the warnings under W=1 to shift around and keeping them >> unified will make maintainence over the longer term a little easier. I >> am not sure if this has been brought up in the past and I don't want to >> hold up this change but I suspect this sentiment of wanting to enable >> W=1 on a per-subsystem basis is going to continue to grow. > > > > I believe this patch is the right way because > we will be able to add a new warning option to > scripts/Makefile.extrawarn without fixing any code. > > I remember somebody argued that drivers should be > able to do > subdir-ccflags-y += $(W1_FLAGS) Personally, I think this would be the viable way to make the kernel free of W=1 warnings. Make it clean driver and subsystem at a time, with constant progress. Currently, there's haphazard fixing of issues, with new ones creeping back in, because kernel-wide W=1 is too verbose for most developers. It's whac-a-mole. > However, if a new flag, -Wfoo, emits warnings > for drivers/gpu/drm/{i915,amd}, > you cannot add it to W=1 until fixing the code. Or adding -Wno-foo where it breaks, until fixed. > If many drivers start to do likewise, > W=1 warning will not be W=1 any more. I don't know, is the goal to fix the warnings, or keep adding stuff to W=1 so that it'll always emit warnings? :p BR, Jani. > Another good thing for hard-coding warning options > is you can lift up a warning flag one by one. > > > Let's say you fixed the entire DRM subsystem so > it is -Wunused free now. > > Then, you can move -Wunused to drivers/gpu/drm/Makefile, > while other warning options stay in drivers Makefiles. > > > > > > > >> >> Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building >> drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or >> allmodconfig. >> >> Reviewed-by: Nathan Chancellor >> Tested-by: Nathan Chancellor >> >> > --- >> > drivers/gpu/drm/amd/amdgpu/Makefile | 13 - >> > 1 file changed, 12 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile >> > b/drivers/gpu/drm/amd/amdgpu/Makefile >> > index 86b833085f19..8d16f280b695 100644 >> > --- a/drivers/gpu/drm/amd/amdgpu/Makefile >> > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >> > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ >> > -I$(FULL_AMD_PATH)/amdkfd >> > >> > subdir-ccflags-y := -Wextra >> > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) >> > +subdir-ccflags-y += -Wunused >> > +subdir-ccflags-y += -Wmissing-prototypes >> > +subdir-ccflags-y += -Wmissing-declarations >> > +subdir-ccflags-y += -Wmissing-include-dirs >> > +subdir-ccflags-y += -Wold-style-definition >> > +subdir-ccflags-y += -Wmissing-format-attribute >> > +# Need this to avoid recursive variable evaluation issues >> > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ >> > + $(call cc-option, -Wunused-const-variable) \ >> > + $(call cc-option, -Wstringop-truncation) \ >> > + $(call cc-option, -Wpacked-not-aligned) >> > +subdir-ccflags-y += $(cond-flags) >> > subdir-ccflags-y += -Wno-unused-parameter >> > subdir-ccflags-y += -Wno-type-limits >> > subdir-ccflags-y += -Wno-sign-compare >> > -- >> > 2.40.1 >> > -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 07/36] drm/amd/display: add plane driver-specific properties for degamma LUT
Hi, Adding a plane color_mgmt_changed doesn't really work as it could be different plane object each time but not need a reset at the hardware level. What needs to be done is to add the properties to should_reset_plane directly in the old_other_state stuff like so: /* HDR/Transfer Function changes. */ if (old_other_state->degamma_tf != new_other_state->degamma_tf || old_other_state->degamma_lut != new_other_state->degamma_lut || old_other_state->hdr_mult != new_other_state->hdr_mult) old_other_state->hdr_mult != new_other_state->hdr_mult || old_other_state->shaper_lut != new_other_state->shaper_lut || old_other_state->shaper_tf != new_other_state->shaper_tf || old_other_state->lut3d != new_other_state->lut3d) return true; This is the same for all of the plane properties fwiw. Thanks! - Joshie 🐸✨ On 6/6/23 18:15, Melissa Wen wrote: On 06/01, Harry Wentland wrote: On 5/23/23 18:14, Melissa Wen wrote: Create and attach driver-private properties for plane color management. First add plane degamma LUT properties that means user-blob and its size. We will add more plane color properties in the next commits. In addition, we keep these driver-private plane properties limited by defining AMD_PRIVATE_COLOR. Co-developed-by: Joshua Ashton Signed-off-by: Joshua Ashton Signed-off-by: Melissa Wen --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 14 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 8 ++ .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 9 +++ .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 77 +++ 4 files changed, 108 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index 88af075e6c18..fa67c84f5994 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -1275,6 +1275,20 @@ amdgpu_display_create_color_properties(struct amdgpu_device *adev) return -ENOMEM; adev->mode_info.regamma_tf_property = prop; + prop = drm_property_create(adev_to_drm(adev), + DRM_MODE_PROP_BLOB, + "AMD_PLANE_DEGAMMA_LUT", 0); + if (!prop) + return -ENOMEM; + adev->mode_info.plane_degamma_lut_property = prop; + + prop = drm_property_create_range(adev_to_drm(adev), +DRM_MODE_PROP_IMMUTABLE, +"AMD_PLANE_DEGAMMA_LUT_SIZE", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + adev->mode_info.plane_degamma_lut_size_property = prop; + Same as with previous patch and the following ones... Would be great to have this sit in amdgpu_dm_color.c. Ack Harry return 0; } #endif diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index 881446c51b36..6c165ad9bdf0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -352,6 +352,14 @@ struct amdgpu_mode_info { * drm_transfer_function`. */ struct drm_property *regamma_tf_property; + /* @plane_degamma_lut_property: Plane property to set a degamma LUT to +* convert color space before blending. +*/ + struct drm_property *plane_degamma_lut_property; + /* @plane_degamma_lut_size_property: Plane property to define the max +* size of degamma LUT as supported by the driver (read-only). +*/ + struct drm_property *plane_degamma_lut_size_property; }; #define AMDGPU_MAX_BL_LEVEL 0xFF diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index ad5ee28b83dc..22e126654767 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -716,6 +716,15 @@ enum drm_transfer_function { struct dm_plane_state { struct drm_plane_state base; struct dc_plane_state *dc_state; + + /* Plane color mgmt */ + /** +* @degamma_lut: +* +* LUT for converting plane pixel data before going into plane merger. +* The blob (if not NULL) is an array of &struct drm_color_lut. +*/ + struct drm_property_blob *degamma_lut; }; struct dm_crtc_state { diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c index 322668973747..e9cedc4068f1 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c @@ -1338,6 +1338,9 @@ dm_drm_plane_duplicate_state(struct drm_plane *plane) dc_plane_state_retain(d
Re: [PATCH v4 1/8] dt-bindings: display: mediatek: add MT8195 hdmi bindings
Hi, Guillaume: Guillaume Ranquet 於 2023年6月9日 週五 下午11:50寫道: > > On Thu, 08 Jun 2023 23:05, Rob Herring wrote: > >On Mon, May 29, 2023 at 04:30:58PM +0200, Guillaume Ranquet wrote: > >> Add mt8195 SoC bindings for hdmi and hdmi-ddc > >> > >> On mt8195 the ddc i2c controller is part of the hdmi IP block and thus has > >> no > >> specific register range, power domain or interrupt, making it simpler > >> than the legacy "mediatek,hdmi-ddc" binding. > >> > >> Signed-off-by: Guillaume Ranquet > >> --- > >> .../bindings/display/mediatek/mediatek,hdmi.yaml | 59 > >> ++ > >> .../display/mediatek/mediatek,mt8195-hdmi-ddc.yaml | 45 + > >> 2 files changed, 93 insertions(+), 11 deletions(-) > >> > >> diff --git > >> a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > >> b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > >> index b90b6d18a828..4f62e6b94048 100644 > >> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > >> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml > >> @@ -21,6 +21,7 @@ properties: > >>- mediatek,mt7623-hdmi > >>- mediatek,mt8167-hdmi > >>- mediatek,mt8173-hdmi > >> + - mediatek,mt8195-hdmi > >> > >>reg: > >> maxItems: 1 > >> @@ -29,18 +30,10 @@ properties: > >> maxItems: 1 > >> > >>clocks: > >> -items: > >> - - description: Pixel Clock > >> - - description: HDMI PLL > >> - - description: Bit Clock > >> - - description: S/PDIF Clock > >> +maxItems: 4 > >> > >>clock-names: > >> -items: > >> - - const: pixel > >> - - const: pll > >> - - const: bclk > >> - - const: spdif > >> +maxItems: 4 > >> > >>phys: > >> maxItems: 1 > >> @@ -58,6 +51,9 @@ properties: > >> description: | > >>phandle link and register offset to the system configuration > >> registers. > >> > >> + power-domains: > >> +maxItems: 1 > >> + > >>ports: > >> $ref: /schemas/graph.yaml#/properties/ports > >> > >> @@ -86,9 +82,50 @@ required: > >>- clock-names > >>- phys > >>- phy-names > >> - - mediatek,syscon-hdmi > >>- ports > >> > >> +allOf: > >> + - if: > >> + properties: > >> +compatible: > >> + contains: > >> +const: mediatek,mt8195-hdmi > >> +then: > >> + properties: > >> +clocks: > >> + items: > >> +- description: APB > >> +- description: HDCP > >> +- description: HDCP 24M > >> +- description: Split HDMI > >> +clock-names: > >> + items: > >> +- const: hdmi_apb_sel > >> +- const: hdcp_sel > >> +- const: hdcp24_sel > >> +- const: split_hdmi > >> + > >> + required: > >> +- power-domains > >> +else: > >> + properties: > >> +clocks: > >> + items: > >> +- description: Pixel Clock > >> +- description: HDMI PLL > >> +- description: Bit Clock > >> +- description: S/PDIF Clock > >> + > >> +clock-names: > >> + items: > >> +- const: pixel > >> +- const: pll > >> +- const: bclk > >> +- const: spdif > > > >I don't understand how the same h/w block can have completely different > >clocks. If not the same h/w or evolution of the same h/w, then do a > >separate schema. > > > > Hi Rob, > > I'm not entirely sure what's the best approach here. > The IPs are different enough to warrant a separate schema IMHO. > Though CK asked me to merge both IPs together (for both schema and code). > > CK might want to chime in and advocate his point of view? For all the mediatek hdmi device, input mediatek internal video signal, and output hdmi signal, so I'm curious about how mt8195-hdmi device is different with other mediatek hdmi device. I think pixel clock is an important clock which is related to resolution and fps. So I think every hdmi device should have a clock which related to pixel clock. I does not find it in mt8195 device. Is one of the clock the parent clock of pixel clock or one of the clock is pixel clock but you give it another naming? Please conduct with mediatek stuff and give us these information, so we have information to judge mt8195-hdmi is different with other mediatek hdmi device or similar to. For other three clocks, I still need these information. Regards, Chun-Kuang. > > >> + > >> + required: > >> +- mediatek,syscon-hdmi > >> + > >> additionalProperties: false > >> > >> examples: > >> diff --git > >> a/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml > >> > >> b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml > >> new file mode 100644 > >> index ..84c096835b47 > >> --- /dev/null > >> +++ > >> b/Documentation/devicetree/bin
[Bug 217537] [AMDGPU] RDNA Freesync problem with CVT-Reduced display profile
https://bugzilla.kernel.org/show_bug.cgi?id=217537 Paulo Marcos de Souza Arruda do Nascimento (contato-mygh...@protonmail.com) changed: What|Removed |Added Kernel Version||6.3.6 -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217537] New: [AMDGPU] RDNA Freesync problem with CVT-Reduced display profile
https://bugzilla.kernel.org/show_bug.cgi?id=217537 Bug ID: 217537 Summary: [AMDGPU] RDNA Freesync problem with CVT-Reduced display profile Product: Drivers Version: 2.5 Hardware: AMD OS: Linux Status: NEW Severity: normal Priority: P3 Component: Video(DRI - non Intel) Assignee: drivers_video-...@kernel-bugs.osdl.org Reporter: contato-mygh...@protonmail.com Regression: No Created attachment 304393 --> https://bugzilla.kernel.org/attachment.cgi?id=304393&action=edit edid of my AOC 24G2 Monitor The problem: When a CVT-RB or CVT-RB2 monitor profile is enabled, using Freesync on a game that demands 100% of GPU utilization makes the mouse pointer to perform very slow (2~3fps) on Wayland, even if the game is running above 90fps smoothly. Recording the screen results in a video without this problem happening (maybe pipewire simulates the mouse?) With x11, the game itself starts jumping frames while recording the screen (CVT-RB only), or always have a terrible struttering (CVT-RB2). Why do I want to use CVT-RB or CVT-RB2 monitor profiles? I want to decrease the idle power consumption of my GPU. Using one of those monitor profiles do the trick (15W -> 3W). Does these profiles work on Windows? Yes, without any issue. I'm sure the problem is only happening on Linux distros. Which things I've done to try fixing the issues? Tried linux-lts, switched between both amdvlk and radv (vulkan-radeon), tested amdgpu.dc=0 (it ended up freezing the GPU before launching display server), made a clean install of Arch Linux, tried different display profiles while modifying the edid and none of these options helped at all. Hardware: GPU: AMD RX 5500XT 8GB (GV-R55XTOC-8GD) Monitor: AOC 24G2 1920x1080 144hz Freesync Premium Both are connected with DisplayPort 1.2 Software: OS: Arch Linux DE: Plasma 5.27.5 Kernel: linux 6.3.6 / linux-lts 6.1.33 Mesa: 23.1.1-1 I'm attaching the original edid of my monitor, just in case if someone wants to analyze it (dumped with read-edid). -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[PATCH] accel/qaic: Fix dereferencing freed memory
smatch warning: drivers/accel/qaic/qaic_data.c:620 qaic_free_object() error: dereferencing freed memory 'obj->import_attach' obj->import_attach is detached and freed using dma_buf_detach(). But used after free to decrease the dmabuf ref count using dma_buf_put(). Fixes: ff13be830333 ("accel/qaic: Add datapath") Signed-off-by: Sukrut Bellary --- drivers/accel/qaic/qaic_data.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c index e42c1f98..7cba4d680ea8 100644 --- a/drivers/accel/qaic/qaic_data.c +++ b/drivers/accel/qaic/qaic_data.c @@ -613,11 +613,13 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc static void qaic_free_object(struct drm_gem_object *obj) { struct qaic_bo *bo = to_qaic_bo(obj); + struct dma_buf *dmabuf; if (obj->import_attach) { /* DMABUF/PRIME Path */ + dmabuf = obj->import_attach->dmabuf; dma_buf_detach(obj->import_attach->dmabuf, obj->import_attach); - dma_buf_put(obj->import_attach->dmabuf); + dma_buf_put(dmabuf); } else { /* Private buffer allocation path */ qaic_free_sgt(bo->sgt); -- 2.34.1
Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu
On Sat, Jun 10, 2023 at 5:17 AM Nathan Chancellor wrote: > > + Masahiro and linux-kbuild > > On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: > > We have a clean build with W=1 as of > > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move > > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable > > these checks unconditionally for the entire module to catch these errors > > during development. > > > > Cc: Alex Deucher > > Cc: Nathan Chancellor > > Signed-off-by: Hamza Mahfooz > > I think this is fine, especially since it will help catch issues in > amdgpu quickly and hopefully encourage developers to fix their problems > before they make it to a tree with wider impact lika -next. > > However, this is now the third place that W=1 has been effectively > enabled (i915 and btrfs are the other two I know of) and it would be > nice if this was a little more unified, especially since it is not > uncommon for the warnings under W=1 to shift around and keeping them > unified will make maintainence over the longer term a little easier. I > am not sure if this has been brought up in the past and I don't want to > hold up this change but I suspect this sentiment of wanting to enable > W=1 on a per-subsystem basis is going to continue to grow. I believe this patch is the right way because we will be able to add a new warning option to scripts/Makefile.extrawarn without fixing any code. I remember somebody argued that drivers should be able to do subdir-ccflags-y += $(W1_FLAGS) However, if a new flag, -Wfoo, emits warnings for drivers/gpu/drm/{i915,amd}, you cannot add it to W=1 until fixing the code. If many drivers start to do likewise, W=1 warning will not be W=1 any more. Another good thing for hard-coding warning options is you can lift up a warning flag one by one. Let's say you fixed the entire DRM subsystem so it is -Wunused free now. Then, you can move -Wunused to drivers/gpu/drm/Makefile, while other warning options stay in drivers Makefiles. > > Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building > drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or > allmodconfig. > > Reviewed-by: Nathan Chancellor > Tested-by: Nathan Chancellor > > > --- > > drivers/gpu/drm/amd/amdgpu/Makefile | 13 - > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile > > b/drivers/gpu/drm/amd/amdgpu/Makefile > > index 86b833085f19..8d16f280b695 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > > -I$(FULL_AMD_PATH)/amdkfd > > > > subdir-ccflags-y := -Wextra > > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > > +subdir-ccflags-y += -Wunused > > +subdir-ccflags-y += -Wmissing-prototypes > > +subdir-ccflags-y += -Wmissing-declarations > > +subdir-ccflags-y += -Wmissing-include-dirs > > +subdir-ccflags-y += -Wold-style-definition > > +subdir-ccflags-y += -Wmissing-format-attribute > > +# Need this to avoid recursive variable evaluation issues > > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ > > + $(call cc-option, -Wunused-const-variable) \ > > + $(call cc-option, -Wstringop-truncation) \ > > + $(call cc-option, -Wpacked-not-aligned) > > +subdir-ccflags-y += $(cond-flags) > > subdir-ccflags-y += -Wno-unused-parameter > > subdir-ccflags-y += -Wno-type-limits > > subdir-ccflags-y += -Wno-sign-compare > > -- > > 2.40.1 > > -- Best Regards Masahiro Yamada
Re: [RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
On 6/8/2023 12:51 PM, Abhinav Kumar wrote: On 6/7/2023 2:56 PM, Dmitry Baryshkov wrote: 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. I am fine with that too but as you noted in (3), if planes_changed will be set by those functions and you can drop explicit assignment of it in this patch, that will be the best option for me. 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 p
Re: [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 10/06/2023 01:57, Jessica Zhang wrote: Add documentation comments explaining the pclk_rate and hdisplay math related to DSC. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index fb1d3a25765f..aeaadc18bc7b 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host) static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode, const struct drm_dsc_config *dsc) { + /* +* Adjust the pclk rate by calculating a new hdisplay proportional to +* the compression ratio such that: +* new_hdisplay = old_hdisplay * target_bpp / source_bpp I'd say `* compressed_bpp / uncompressed_bpp'. This is easier to follow than source and target. +* +* Porches need not be adjusted during compression. +*/ int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), dsc->bits_per_component * 3); @@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) /* Divide the display by 3 but keep back/font porch and * pulse width same +* +* hdisplay will be divided by 3 here to account for the fact +* that DPU sends 3 bytes per pclk cycle to DSI. */ h_total -= hdisplay; hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); -- With best wishes Dmitry
Re: [RFC] Plane color pipeline KMS uAPI
On 6/9/2023 12:30 PM, Simon Ser wrote: Hi Christopher, On Friday, June 9th, 2023 at 17:52, Christopher Braga wrote: The new COLOROP objects also expose a number of KMS properties. Each has a type, a reference to the next COLOROP object in the linked list, and other type-specific properties. Here is an example for a 1D LUT operation: Color operation 42 ├─ "type": enum {Bypass, 1D curve} = 1D curve ├─ "1d_curve_type": enum {LUT, sRGB, PQ, BT.709, HLG, …} = LUT The options sRGB / PQ / BT.709 / HLG would select hard-coded 1D curves? Will different hardware be allowed to expose a subset of these enum values? Yes. Only hardcoded LUTs supported by the HW are exposed as enum entries. ├─ "lut_size": immutable range = 4096 ├─ "lut_data": blob └─ "next": immutable color operation ID = 43 Some hardware has per channel 1D LUT values, while others use the same LUT for all channels. We will definitely need to expose this in the UAPI in some form. Hm, I was assuming per-channel 1D LUTs here, just like the existing GAMMA_LUT/ DEGAMMA_LUT properties work. If some hardware can't support that, it'll need to get exposed as another color operation block. To configure this hardware block, user-space can fill a KMS blob with 4096 u32 entries, then set "lut_data" to the blob ID. Other color operation types might have different properties. The bit-depth of the LUT is an important piece of information we should include by default. Are we assuming that the DRM driver will always reduce the input values to the resolution supported by the pipeline? This could result in differences between the hardware behavior and the shader behavior. Additionally, some pipelines are floating point while others are fixed. How would user space know if it needs to pack 32 bit integer values vs 32 bit float values? Again, I'm deferring to the existing GAMMA_LUT/DEGAMMA_LUT. These use a common definition of LUT blob (u16 elements) and it's up to the driver to convert. Using a very precise format for the uAPI has the nice property of making the uAPI much simpler to use. User-space sends high precision data and it's up to drivers to map that to whatever the hardware accepts. Conversion from a larger uint type to a smaller type sounds low effort, however if a block works in a floating point space things are going to get messy really quickly. If the block operates in FP16 space and the interface is 16 bits we are good, but going from 32 bits to FP16 (such as in the matrix case or 3DLUT) is less than ideal. Exposing the actual hardware precision is something we've talked about during the hackfest. It'll probably be useful to some extent, but will require some discussion to figure out how to design the uAPI. Maybe a simple property is enough, maybe not (e.g. fully describing the precision of segmented LUTs would probably be trickier). I'd rather keep things simple for the first pass, we can always add more properties for bit depth etc later on. Indicating if a block operates on / with fixed vs float values is significant enough that I think we should account for this in initial design. It will have a affect on both the user space value packing + expected value ranges in the hardware. Here is another example with a 3D LUT: Color operation 42 ├─ "type": enum {Bypass, 3D LUT} = 3D LUT ├─ "lut_size": immutable range = 33 ├─ "lut_data": blob └─ "next": immutable color operation ID = 43 We are going to need to expose the packing order here to avoid any programming uncertainty. I don't think we can safely assume all hardware is equivalent. The driver can easily change the layout of the matrix and do any conversion necessary when programming the hardware. We do need to document what layout is used in the uAPI for sure. And one last example with a matrix: Color operation 42 ├─ "type": enum {Bypass, Matrix} = Matrix ├─ "matrix_data": blob └─ "next": immutable color operation ID = 43 It is unclear to me what the default sizing of this matrix is. Any objections to exposing these details with an additional property? The existing CTM property uses 9 uint64 (S31.32) values. Is there a case where that wouldn't be enough? Larger cases do exist, but as you mention this can be resolved with a different type then. I don't have any issues with the default 'Matrix' type being 9 entries. Dithering logic exists in some pipelines. I think we need a plan to expose that here as well. Hm, I'm not too familiar with dithering. Do you think it would make sense to expose as an additional colorop block? Do you think it would have more consequences on the design? I want to re-iterate that we don't need to ship all features from day 1. We just need to come up with a uAPI design on which new features can be built on. Agreed. I don't think this will affect the proposed design so this can be figured out once we have a DRM driver impl that
Re: [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression
On 6/9/2023 3:57 PM, Jessica Zhang wrote: Adjust the pclk rate to divide hdisplay by the compression ratio when DSC is enabled. Signed-off-by: Jessica Zhang --- Reviewed-by: Abhinav Kumar
Re: [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
On 6/9/2023 3:57 PM, Jessica Zhang wrote: Add a DPU INTF op to set the DCE_DATA_COMPRESS bit to enable the DCE/DSC 1.2 datapath Note: For now, this op is called for command mode encoders only. Changes to set DATA_COMPRESS for video mode encoders will be posted along with DSC v1.2 support for DP. Signed-off-by: Jessica Zhang --- Reviewed-by: Abhinav Kumar
[PATCH v6 0/6] Add DSC v1.2 Support for DSI
This is a series of changes for DSI to enable command mode support for DSC v1.2. This includes: 1) Rounding up `hdisplay / 3` in dsc_timing_setup() 2) Adjusting pclk_rate to account for compression 3) Fixing incorrect uses of slice_count in DSI DSC calculations 4) Setting the DATA_COMPRESS bit when DSC is enabled With these changes (and the dependency below), DSC v1.2 should work over DSI in command mode. Note: Changes that add DSC v1.2 support for video mode will be posted with the DP support changes. Depends-on: - "add DSC 1.2 dpu supports" [1] - "Introduce MSM-specific DSC helpers" [2] - "drm/msm/dsi: use mult_frac for pclk_bpp calculation" [3] [1] https://patchwork.freedesktop.org/series/116789/ [2] https://patchwork.freedesktop.org/series/115833/ [3] https://patchwork.freedesktop.org/patch/538273/?series=118072&rev=1 Signed-off-by: Jessica Zhang --- Changes in v6: - "Adjust" --> "Reduce" in pclk patch title (Marijn) - dsi_adjust_compressed_pclk() --> dsi_adjust_pclk_for_compression() (Marijn) - Moved dsi_adjust_pclk_for_compression() to before is_bonded_dsi pclk adjustment (Dmitry) - Added documentation comments explaining the pclk_rate and hdisplay adjustments related to DSC (Dmitry) - Only set DATA_COMPRESS bit if DSC is enabled (Abhinav) - Rebased on top of latest msm-next-lumag branch - Link to v5: https://lore.kernel.org/r/20230405-add-dsc-support-v5-0-028c10850...@quicinc.com Changes in v5: - Added newline before enable_compression() function pointer definition - Rebased on top of "drm/msm/dsi: use mult_frac for pclk_bpp calculation" - Reworded commit messages for clarity - Dropped mentions of "soft slice" in commit messages - "slice_per_packet" -> "slice_per_pkt" - Picked up reviewed-by tags - Link to v4: https://lore.kernel.org/r/20230405-add-dsc-support-v4-0-15daf84f8...@quicinc.com Changes in v4: - Clarified slice_per_pkt comment regarding pkt_per_line calculations - Reworded commit message for "drm/msm/dsi: Remove incorrect references to slice_count" - Wrapped INTF_SC7280_MASK macro definition in parentheses - Fixed incorrect commit hash in "msm/drm/dsi: Round up DSC hdisplay calculation" - Picked up Reviewed-by tag - Link to v3: https://lore.kernel.org/r/20230405-add-dsc-support-v3-0-6e1d35a20...@quicinc.com Changes in v3: - Added fix to round up hdisplay DSC adjustment - Fixed inconsistent whitespace in dpu_hw_intf_ops comment doc - Moved placement of dpu_hw_intf_enable_compression - Picked up "drm/msm/dsi: Fix calculation for pkt_per_line" patch and squashed all slice_count fixes into a single patch - Use drm_mode_vrefresh() to calculate adjusted pclk rate - Moved compressed pclk adjustment to dsi_adjust_compressed_pclk() helper - Rebased changes on top of updated dependencies - Reworded commit message for "drm/msm/dpu: Set DATA_COMPRESS for command mode" for clarity - Removed revision changelog in commit messages - Link to v2: https://lore.kernel.org/r/20230405-add-dsc-support-v2-0-1072c70e9...@quicinc.com Changes in v2: - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag - Changed pclk math to only divide hdisplay by compression ratio - Reworded word count TODO comment - Make DATA_COMPRESS an INTF flag - Read INTF_CONFIG2 before writing to DATA_COMPRESS bit - Fixed whitespace issue in macro definition - Removed `inline` from dpu_hw_intf_enable_compression declaration - Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set - Reworded commit messages and cover letter for clarity - Link to v1: https://lore.kernel.org/r/20230405-add-dsc-support-v1-0-6bc6f03ae...@quicinc.com --- Jessica Zhang (6): msm/drm/dsi: Round up DSC hdisplay calculation drm/msm/dsi: Reduce pclk rate for compression drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 drm/msm/dsi: Remove incorrect references to slice_count drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 13 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 3 ++ drivers/gpu/drm/msm/dsi/dsi_host.c | 59 +- 6 files changed, 67 insertions(+), 15 deletions(-) --- base-commit: 8dc20f06b90af85c083866df73dae69236183b62 change-id: 20230405-add-dsc-support-fe130ba49841 Best regards, -- Jessica Zhang
[PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
Add documentation comments explaining the pclk_rate and hdisplay math related to DSC. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index fb1d3a25765f..aeaadc18bc7b 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host) static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode, const struct drm_dsc_config *dsc) { + /* +* Adjust the pclk rate by calculating a new hdisplay proportional to +* the compression ratio such that: +* new_hdisplay = old_hdisplay * target_bpp / source_bpp +* +* Porches need not be adjusted during compression. +*/ int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), dsc->bits_per_component * 3); @@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) /* Divide the display by 3 but keep back/font porch and * pulse width same +* +* hdisplay will be divided by 3 here to account for the fact +* that DPU sends 3 bytes per pclk cycle to DSI. */ h_total -= hdisplay; hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); -- 2.40.1
[PATCH v6 3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
In DPU 7.x and later, DSC/DCE enablement registers have been moved from PINGPONG to INTF. Thus, add a DPU_INTF_DATA_COMPRESS feature flag that will be set if the DATA_COMPRESS register is in the INTF block. Reviewed-by: Marijn Suijten Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index 008df60b00f0..36ba3f58dcdf 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -105,7 +105,7 @@ BIT(DPU_INTF_STATUS_SUPPORTED) | \ BIT(DPU_DATA_HCTL_EN)) -#define INTF_SC7280_MASK (INTF_SC7180_MASK) +#define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS)) #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \ BIT(DPU_WB_UBWC) | \ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index d3598dd9d448..b860784ade72 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -181,6 +181,7 @@ enum { * @DPU_DATA_HCTL_ENAllows data to be transferred at different rate * than video timing * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register + * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register * @DPU_INTF_MAX */ enum { @@ -188,6 +189,7 @@ enum { DPU_INTF_TE, DPU_DATA_HCTL_EN, DPU_INTF_STATUS_SUPPORTED, + DPU_INTF_DATA_COMPRESS, DPU_INTF_MAX }; -- 2.40.1
[PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression
Adjust the pclk rate to divide hdisplay by the compression ratio when DSC is enabled. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index a448931af804..98ea1da492c7 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -561,12 +561,27 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host) clk_disable_unprepare(msm_host->byte_clk); } -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi) +static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode, + const struct drm_dsc_config *dsc) +{ + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), + dsc->bits_per_component * 3); + + int new_htotal = mode->htotal - mode->hdisplay + new_hdisplay; + + return new_htotal * mode->vtotal * drm_mode_vrefresh(mode); +} + +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, + const struct drm_dsc_config *dsc, bool is_bonded_dsi) { unsigned long pclk_rate; pclk_rate = mode->clock * 1000; + if (dsc) + pclk_rate = dsi_adjust_pclk_for_compression(mode, dsc); + /* * For bonded DSI mode, the current DRM mode has the complete width of the * panel. Since, the complete panel is driven by two DSI controllers, @@ -585,7 +600,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d struct msm_dsi_host *msm_host = to_msm_dsi_host(host); u8 lanes = msm_host->lanes; u32 bpp = dsi_get_bpp(msm_host->format); - unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi); + unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi); unsigned long pclk_bpp; if (lanes == 0) { @@ -604,7 +619,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi) { - msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi); + msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi); msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi, msm_host->mode); -- 2.40.1
[PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
Add a DPU INTF op to set the DCE_DATA_COMPRESS bit to enable the DCE/DSC 1.2 datapath Note: For now, this op is called for command mode encoders only. Changes to set DATA_COMPRESS for video mode encoders will be posted along with DSC v1.2 support for DP. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 13 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++ 3 files changed, 19 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 63ba0082b6ee..b856c6286c85 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -67,6 +67,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( phys_enc->hw_intf->ops.bind_pingpong_blk( phys_enc->hw_intf, phys_enc->hw_pp->idx); + + if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression) + phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf); } static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 530f82e34c1e..5b0f6627e29b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -91,6 +91,7 @@ #define INTF_CFG2_DATABUS_WIDENBIT(0) #define INTF_CFG2_DATA_HCTL_EN BIT(4) +#define INTF_CFG2_DCE_DATA_COMPRESS BIT(12) static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, @@ -512,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf, } +static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx) +{ + u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2); + + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS; + + DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2); +} + static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, unsigned long cap) { @@ -532,6 +542,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, ops->vsync_sel = dpu_hw_intf_vsync_sel; ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh; } + + if (cap & BIT(DPU_INTF_DATA_COMPRESS)) + ops->enable_compression = dpu_hw_intf_enable_compression; } struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h index 33895eca1211..99e21c4137f9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h @@ -70,6 +70,7 @@ struct intf_status { * @get_autorefresh:Retrieve autorefresh config from hardware * Return: 0 on success, -ETIMEDOUT on timeout * @vsync_sel: Select vsync signal for tear-effect configuration + * @enable_compression: Enable data compression */ struct dpu_hw_intf_ops { void (*setup_timing_gen)(struct dpu_hw_intf *intf, @@ -106,6 +107,8 @@ struct dpu_hw_intf_ops { * Disable autorefresh if enabled */ void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay); + + void (*enable_compression)(struct dpu_hw_intf *intf); }; struct dpu_hw_intf { -- 2.40.1
[PATCH v6 5/6] drm/msm/dsi: Remove incorrect references to slice_count
Currently, slice_count is being used to calculate word count and pkt_per_line. Instead, these values should be calculated using slice per packet, which is not the same as slice_count. Slice count represents the number of slices per interface, and its value will not always match that of slice per packet. For example, it is possible to have cases where there are multiple slices per interface but the panel specifies only one slice per packet. Thus, use the default value of one slice per packet and remove slice_count from the aforementioned calculations. Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration") Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count") Reviewed-by: Marijn Suijten Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 26 -- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 98ea1da492c7..fb1d3a25765f 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -863,18 +863,17 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod */ slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay); - /* -* If slice_count is greater than slice_per_intf -* then default to 1. This can happen during partial -* update. -*/ - if (dsc->slice_count > slice_per_intf) - dsc->slice_count = 1; - total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf; eol_byte_num = total_bytes_per_intf % 3; - pkt_per_line = slice_per_intf / dsc->slice_count; + + /* +* Typically, pkt_per_line = slice_per_intf * slice_per_pkt. +* +* Since the current driver only supports slice_per_pkt = 1, +* pkt_per_line will be equal to slice per intf for now. +*/ + pkt_per_line = slice_per_intf; if (is_cmd_mode) /* packet data type */ reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE); @@ -998,7 +997,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) if (!msm_host->dsc) wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1; else - wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1; + /* +* When DSC is enabled, WC = slice_chunk_size * slice_per_pkt + 1. +* Currently, the driver only supports default value of slice_per_pkt = 1 +* +* TODO: Expand mipi_dsi_device struct to hold slice_per_pkt info +* and adjust DSC math to account for slice_per_pkt. +*/ + wc = msm_host->dsc->slice_chunk_size + 1; dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL, DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) | -- 2.40.1
[PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation
Currently, when compression is enabled, hdisplay is reduced via integer division. This causes issues for modes where the original hdisplay is not a multiple of 3. To fix this, use DIV_ROUND_UP to divide hdisplay. Suggested-by: Marijn Suijten Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration") Reviewed-by: Marijn Suijten Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 1a99d75025dc..a448931af804 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -949,7 +949,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) * pulse width same */ h_total -= hdisplay; - hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3; + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); h_total += hdisplay; ha_end = ha_start + hdisplay; } -- 2.40.1
Re: [PATCH v8 10/18] drm/msm/a6xx: Introduce GMU wrapper support
On Mon, May 29, 2023 at 03:52:29PM +0200, Konrad Dybcio wrote: > > Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs > but don't implement the associated GMUs. This is due to the fact that > the GMU directly pokes at RPMh. Sadly, this means we have to take care > of enabling & scaling power rails, clocks and bandwidth ourselves. > > Reuse existing Adreno-common code and modify the deeply-GMU-infused > A6XX code to facilitate these GPUs. This involves if-ing out lots > of GMU callbacks and introducing a new type of GMU - GMU wrapper (it's > the actual name that Qualcomm uses in their downstream kernels). > > This is essentially a register region which is convenient to model > as a device. We'll use it for managing the GDSCs. The register > layout matches the actual GMU_CX/GX regions on the "real GMU" devices > and lets us reuse quite a bit of gmu_read/write/rmw calls. > > Signed-off-by: Konrad Dybcio > --- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 72 +- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 211 > > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 1 + > drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 14 +- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8 +- > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 6 + > 6 files changed, 277 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 5ba8cba69383..385ca3a12462 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -1437,6 +1437,7 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, > struct platform_device *pdev, > > void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu) > { > + struct adreno_gpu *adreno_gpu = &a6xx_gpu->base; > struct a6xx_gmu *gmu = &a6xx_gpu->gmu; > struct platform_device *pdev = to_platform_device(gmu->dev); > > @@ -1462,10 +1463,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu) > gmu->mmio = NULL; > gmu->rscc = NULL; > > - a6xx_gmu_memory_free(gmu); > + if (!adreno_has_gmu_wrapper(adreno_gpu)) { > + a6xx_gmu_memory_free(gmu); > > - free_irq(gmu->gmu_irq, gmu); > - free_irq(gmu->hfi_irq, gmu); > + free_irq(gmu->gmu_irq, gmu); > + free_irq(gmu->hfi_irq, gmu); > + } > > /* Drop reference taken in of_find_device_by_node */ > put_device(gmu->dev); > @@ -1484,6 +1487,69 @@ static int cxpd_notifier_cb(struct notifier_block *nb, > return 0; > } > > +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node > *node) > +{ > + struct platform_device *pdev = of_find_device_by_node(node); > + struct a6xx_gmu *gmu = &a6xx_gpu->gmu; > + int ret; > + > + if (!pdev) > + return -ENODEV; > + > + gmu->dev = &pdev->dev; > + > + of_dma_configure(gmu->dev, node, true); > + > + pm_runtime_enable(gmu->dev); > + > + /* Mark legacy for manual SPTPRAC control */ > + gmu->legacy = true; > + > + /* Map the GMU registers */ > + gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu"); > + if (IS_ERR(gmu->mmio)) { > + ret = PTR_ERR(gmu->mmio); > + goto err_mmio; > + } > + > + gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx"); > + if (IS_ERR(gmu->cxpd)) { > + ret = PTR_ERR(gmu->cxpd); > + goto err_mmio; > + } > + > + if (!device_link_add(gmu->dev, gmu->cxpd, DL_FLAG_PM_RUNTIME)) { > + ret = -ENODEV; > + goto detach_cxpd; > + } > + > + init_completion(&gmu->pd_gate); > + complete_all(&gmu->pd_gate); > + gmu->pd_nb.notifier_call = cxpd_notifier_cb; > + > + /* Get a link to the GX power domain to reset the GPU */ > + gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx"); > + if (IS_ERR(gmu->gxpd)) { > + ret = PTR_ERR(gmu->gxpd); > + goto err_mmio; > + } > + > + gmu->initialized = true; > + > + return 0; > + > +detach_cxpd: > + dev_pm_domain_detach(gmu->cxpd, false); > + > +err_mmio: > + iounmap(gmu->mmio); > + > + /* Drop reference taken in of_find_device_by_node */ > + put_device(gmu->dev); > + > + return ret; > +} > + > int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node) > { > struct adreno_gpu *adreno_gpu = &a6xx_gpu->base; > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 58bf405b85d8..0a44762dbb6d 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -21,7 +21,7 @@ static inline bool _a6xx_check_idle(struct msm_gpu *gpu) > struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > > /* Check that the GMU is idle */ > - if (!a6xx_gmu_isidle(&a6xx_gpu->gmu)) > + if (!adreno_has_gmu_wrapper(adreno_gpu) && > !a6xx_gmu_isidle(&a6xx_gpu->gmu)) >
[PATCH] drm/i915/guc/slpc: Apply min softlimit correctly
We were skipping when min_softlimit was equal to RPn. We need to apply it rergardless as efficient frequency will push the SLPC min to RPe. This will break scenarios where user sets a min softlimit < RPe before reset and then performs a GT reset. Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency") Signed-off-by: Vinay Belgaumkar --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 01b75529311c..ee9f83af7cf6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -606,7 +606,7 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) if (unlikely(ret)) return ret; slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit; - } else if (slpc->min_freq_softlimit != slpc->min_freq) { + } else { return intel_guc_slpc_set_min_freq(slpc, slpc->min_freq_softlimit); } -- 2.38.1
Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu
+ Masahiro and linux-kbuild On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: > We have a clean build with W=1 as of > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable > these checks unconditionally for the entire module to catch these errors > during development. > > Cc: Alex Deucher > Cc: Nathan Chancellor > Signed-off-by: Hamza Mahfooz I think this is fine, especially since it will help catch issues in amdgpu quickly and hopefully encourage developers to fix their problems before they make it to a tree with wider impact lika -next. However, this is now the third place that W=1 has been effectively enabled (i915 and btrfs are the other two I know of) and it would be nice if this was a little more unified, especially since it is not uncommon for the warnings under W=1 to shift around and keeping them unified will make maintainence over the longer term a little easier. I am not sure if this has been brought up in the past and I don't want to hold up this change but I suspect this sentiment of wanting to enable W=1 on a per-subsystem basis is going to continue to grow. Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or allmodconfig. Reviewed-by: Nathan Chancellor Tested-by: Nathan Chancellor > --- > drivers/gpu/drm/amd/amdgpu/Makefile | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile > b/drivers/gpu/drm/amd/amdgpu/Makefile > index 86b833085f19..8d16f280b695 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > -I$(FULL_AMD_PATH)/amdkfd > > subdir-ccflags-y := -Wextra > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > +subdir-ccflags-y += -Wunused > +subdir-ccflags-y += -Wmissing-prototypes > +subdir-ccflags-y += -Wmissing-declarations > +subdir-ccflags-y += -Wmissing-include-dirs > +subdir-ccflags-y += -Wold-style-definition > +subdir-ccflags-y += -Wmissing-format-attribute > +# Need this to avoid recursive variable evaluation issues > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ > + $(call cc-option, -Wunused-const-variable) \ > + $(call cc-option, -Wstringop-truncation) \ > + $(call cc-option, -Wpacked-not-aligned) > +subdir-ccflags-y += $(cond-flags) > subdir-ccflags-y += -Wno-unused-parameter > subdir-ccflags-y += -Wno-type-limits > subdir-ccflags-y += -Wno-sign-compare > -- > 2.40.1 >
Re: [PATCH] drm: etnaviv: Replace of_platform.h with explicit includes
On Mon, Apr 10, 2023 at 5:26 PM Rob Herring wrote: > > Etnaviv doesn't use anything from of_platform.h, but depends on > of.h, of_device.h, and platform_device.h which are all implicitly > included, but that is going to be removed soon. > > Signed-off-by: Rob Herring > --- > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Ping! > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index 44ca803237a5..c68e83ed5a23 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > @@ -6,7 +6,9 @@ > #include > #include > #include > -#include > +#include > +#include > +#include > #include > > #include > -- > 2.39.2 >
Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices
On Sat, Jun 10, 2023 at 02:07:58AM +0800, Sui Jingfeng wrote: > On 2023/6/10 01:52, Bjorn Helgaas wrote: > > On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote: > > > On 2023/6/9 01:32, Bjorn Helgaas wrote: > > > > On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote: > > > > > From: Sui Jingfeng > > > > > > > > > > This patch adds PCI driver support on top of what we already have. > > > > > Take > > > > > the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI > > > > > device > > > > > driver. There is only one GPU core for the GC1000 in the LS7A1000 and > > > > > LS2K1000. Therefore, component frameworks can be avoided. > > > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, > > > > > 0}, > > > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, > > > > > 0}, > > > > PCI_VDEVICE() > > > This make it impossible to hook device-specific data in the future. > > > > > > But currently there no device specific data associated with the > > > 0x7a05 and 0x7a15, > > > > > > so it's acceptable for now. Thanks. > > Haha, ISTR having this conversation before, sorry for repeating it. > > > > Indeed, it's fine as-is. But PCI_VDEVICE() actually *does* allow for > > vendor-specific data because it doesn't include the data element, > > which defaults to zero if you don't specify it. > > > > So for example, drivers/net/ethernet/realtek/r8169_main.c has this: > > > >{ PCI_VDEVICE(REALTEK, 0x8129) }, > >{ PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT }, > > > > where 0x8129 has no driver_data (it defaults to zero), but 0x8136 > > does. > > PCI_VDEVICE macro end with two zero. (I thought it was three) No worries, I thought the same thing the first five times I read it :)
Re: [PATCH] drm/radeon: Disable outputs when releasing fbdev client
Applied. Thanks! Alex On Fri, Jun 9, 2023 at 1:40 PM Borislav Petkov wrote: > > On Fri, Jun 09, 2023 at 04:03:56PM +0200, Thomas Zimmermann wrote: > > Disable the modesetting pipeline before release the radeon's fbdev > > client. Fixes the following error: > > > > [ 17.217408] WARNING: CPU: 5 PID: 1464 at > > drivers/gpu/drm/ttm/ttm_bo.c:326 ttm_bo_release+0x27e/0x2d0 [ttm] > > [ 17.217418] Modules linked in: edac_mce_amd radeon(+) drm_ttm_helper ttm > > video drm_suballoc_helper drm_display_helper kvm irqbypass drm_kms_helper > > syscopyarea crc32_pclmul sysfillrect sha512_ssse3 sysimgblt sha512_generic > > cfbfillrect cfbimgblt wmi_bmof aesni_intel cfbcopyarea crypto_simd cryptd > > k10temp acpi_cpufreq wmi dm_mod > > [ 17.217432] CPU: 5 PID: 1464 Comm: systemd-udevd Not tainted 6.4.0-rc4+ > > #1 > > [ 17.217436] Hardware name: Micro-Star International Co., Ltd. > > MS-7A38/B450M PRO-VDH MAX (MS-7A38), BIOS B.G0 07/26/2022 > > [ 17.217438] RIP: 0010:ttm_bo_release+0x27e/0x2d0 [ttm] > > [ 17.217444] Code: 48 89 43 38 48 89 43 40 48 8b 5c 24 30 48 8b b5 40 08 > > 00 00 48 8b 6c 24 38 48 83 c4 58 e9 7a 49 f7 e0 48 89 ef e9 6c fe ff ff > > <0f> 0b 48 83 7b 20 00 0f 84 b7 fd ff ff 0f 0b 0f 1f 00 e9 ad fd ff > > [ 17.217448] RSP: 0018:c995fbb0 EFLAGS: 00010202 > > [ 17.217451] RAX: 0001 RBX: 8881052c8de0 RCX: > > > > [ 17.217453] RDX: 0001 RSI: RDI: > > 8881052c8de0 > > [ 17.217455] RBP: 888104a66e00 R08: 8881052c8de0 R09: > > 888104a7cf08 > > [ 17.217457] R10: c995fbe0 R11: c995fbe8 R12: > > 8881052c8c78 > > [ 17.217458] R13: 8881052c8c78 R14: dead0100 R15: > > 88810528b108 > > [ 17.217460] FS: 7f319fcbb8c0() GS:1a54() > > knlGS: > > [ 17.217463] CS: 0010 DS: ES: CR0: 80050033 > > [ 17.217464] CR2: 55dc8b0224a0 CR3: 00010373d000 CR4: > > 00750ee0 > > [ 17.217466] PKRU: 5554 > > [ 17.217468] Call Trace: > > [ 17.217470] > > [ 17.217472] ? __warn+0x97/0x160 > > [ 17.217476] ? ttm_bo_release+0x27e/0x2d0 [ttm] > > [ 17.217481] ? report_bug+0x1ec/0x200 > > [ 17.217487] ? handle_bug+0x3c/0x70 > > [ 17.217490] ? exc_invalid_op+0x1f/0x90 > > [ 17.217493] ? preempt_count_sub+0xb5/0x100 > > [ 17.217496] ? asm_exc_invalid_op+0x16/0x20 > > [ 17.217500] ? ttm_bo_release+0x27e/0x2d0 [ttm] > > [ 17.217505] ? ttm_resource_move_to_lru_tail+0x1ab/0x1d0 [ttm] > > [ 17.217511] radeon_bo_unref+0x1a/0x30 [radeon] > > [ 17.217547] radeon_gem_object_free+0x20/0x30 [radeon] > > [ 17.217579] radeon_fbdev_fb_destroy+0x57/0x90 [radeon] > > [ 17.217616] unregister_framebuffer+0x72/0x110 > > [ 17.217620] drm_client_dev_unregister+0x6d/0xe0 > > [ 17.217623] drm_dev_unregister+0x2e/0x90 > > [ 17.217626] drm_put_dev+0x26/0x90 > > [ 17.217628] pci_device_remove+0x44/0xc0 > > [ 17.217631] really_probe+0x257/0x340 > > [ 17.217635] __driver_probe_device+0x73/0x120 > > [ 17.217638] driver_probe_device+0x2c/0xb0 > > [ 17.217641] __driver_attach+0xa0/0x150 > > [ 17.217643] ? __pfx___driver_attach+0x10/0x10 > > [ 17.217646] bus_for_each_dev+0x67/0xa0 > > [ 17.217649] bus_add_driver+0x10e/0x210 > > [ 17.217651] driver_register+0x5c/0x120 > > [ 17.217653] ? __pfx_radeon_module_init+0x10/0x10 [radeon] > > [ 17.217681] do_one_initcall+0x44/0x220 > > [ 17.217684] ? kmalloc_trace+0x37/0xc0 > > [ 17.217688] do_init_module+0x64/0x240 > > [ 17.217691] __do_sys_finit_module+0xb2/0x100 > > [ 17.217694] do_syscall_64+0x3b/0x90 > > [ 17.217697] entry_SYSCALL_64_after_hwframe+0x72/0xdc > > [ 17.217700] RIP: 0033:0x7f319feaa5a9 > > [ 17.217702] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 > > 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 > > <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 27 08 0d 00 f7 d8 64 89 01 48 > > [ 17.217706] RSP: 002b:7ffc6bf3e7f8 EFLAGS: 0246 ORIG_RAX: > > 0139 > > [ 17.217709] RAX: ffda RBX: 5607204f3170 RCX: > > 7f319feaa5a9 > > [ 17.217710] RDX: RSI: 7f31a002eefd RDI: > > 0018 > > [ 17.217712] RBP: 7f31a002eefd R08: R09: > > 5607204f1860 > > [ 17.217714] R10: 0018 R11: 0246 R12: > > 0002 > > [ 17.217716] R13: R14: 560720522450 R15: > > 560720255899 > > [ 17.217718] > > [ 17.217719] ---[ end trace ]--- > > > > The buffer object backing the fbdev emulation got pinned twice: by the > > fb_probe helper radeon_fbdev_create_pinned_object() and the modesetting > > code when the framebuffer got displayed. It only got unpinned once by > > the fbdev helper radeon_fbdev_destroy_pinned_object(). Hence TTM's BO- > > release function complains about the pin counter.
Re: [RESEND 15/15] drm/amd/amdgpu/sdma_v6_0: Demote a bunch of half-completed function headers
These have already been fixed up. Thanks! Alex On Fri, Jun 9, 2023 at 4:18 AM Lee Jones wrote: > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:248: warning: Function parameter or > member 'job' not described in 'sdma_v6_0_ring_emit_ib' > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:248: warning: Function parameter or > member 'flags' not described in 'sdma_v6_0_ring_emit_ib' > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:945: warning: Function parameter or > member 'timeout' not described in 'sdma_v6_0_ring_test_ib' > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:1124: warning: Function parameter or > member 'ring' not described in 'sdma_v6_0_ring_pad_ib' > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:1175: warning: Function parameter or > member 'vmid' not described in 'sdma_v6_0_ring_emit_vm_flush' > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:1175: warning: Function parameter or > member 'pd_addr' not described in 'sdma_v6_0_ring_emit_vm_flush' > > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Sumit Semwal > Cc: Stanley Yang > Cc: amd-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-me...@vger.kernel.org > Cc: linaro-mm-...@lists.linaro.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c > b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c > index 3b03dda854fdc..8cd7abe74e6c4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c > @@ -233,7 +233,7 @@ static void sdma_v6_0_ring_insert_nop(struct amdgpu_ring > *ring, uint32_t count) > amdgpu_ring_write(ring, ring->funcs->nop); > } > > -/** > +/* > * sdma_v6_0_ring_emit_ib - Schedule an IB on the DMA engine > * > * @ring: amdgpu ring pointer > @@ -936,7 +936,7 @@ static int sdma_v6_0_ring_test_ring(struct amdgpu_ring > *ring) > return r; > } > > -/** > +/* > * sdma_v6_0_ring_test_ib - test an IB on the DMA engine > * > * @ring: amdgpu_ring structure holding ring information > @@ -1118,7 +1118,7 @@ static void sdma_v6_0_vm_set_pte_pde(struct amdgpu_ib > *ib, > ib->ptr[ib->length_dw++] = count - 1; /* number of entries */ > } > > -/** > +/* > * sdma_v6_0_ring_pad_ib - pad the IB > * @ib: indirect buffer to fill with padding > * @ring: amdgpu ring pointer > @@ -1167,7 +1167,7 @@ static void sdma_v6_0_ring_emit_pipeline_sync(struct > amdgpu_ring *ring) > SDMA_PKT_POLL_REGMEM_DW5_INTERVAL(4)); /* retry > count, poll interval */ > } > > -/** > +/* > * sdma_v6_0_ring_emit_vm_flush - vm flush using sDMA > * > * @ring: amdgpu_ring pointer > -- > 2.41.0.162.gfafddb0af9-goog >
Re: [RESEND 14/15] drm/radeon/radeon_ttm: Remove unused variable 'rbo' from radeon_bo_move()
This patch is no longer applicable. Alex On Fri, Jun 9, 2023 at 4:18 AM Lee Jones wrote: > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/radeon/radeon_ttm.c: In function ‘radeon_bo_move’: > drivers/gpu/drm/radeon/radeon_ttm.c:201:27: warning: variable ‘rbo’ set but > not used [-Wunused-but-set-variable] > > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Jerome Glisse > Cc: amd-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/radeon/radeon_ttm.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c > b/drivers/gpu/drm/radeon/radeon_ttm.c > index 4eb83ccc4906a..de4e6d78f1e12 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -197,7 +197,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, > bool evict, > { > struct ttm_resource *old_mem = bo->resource; > struct radeon_device *rdev; > - struct radeon_bo *rbo; > int r; > > if (new_mem->mem_type == TTM_PL_TT) { > @@ -210,7 +209,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, > bool evict, > if (r) > return r; > > - rbo = container_of(bo, struct radeon_bo, tbo); > rdev = radeon_get_rdev(bo->bdev); > if (!old_mem || (old_mem->mem_type == TTM_PL_SYSTEM && > bo->ttm == NULL)) { > -- > 2.41.0.162.gfafddb0af9-goog >
Re: [PATCH v8 09/18] drm/msm/a6xx: Extend and explain UBWC config
On Mon, May 29, 2023 at 03:52:28PM +0200, Konrad Dybcio wrote: > > Rename lower_bit to hbb_lo and explain what it signifies. > Add explanations (wherever possible to other tunables). > > Port setting min_access_length, ubwc_mode and hbb_hi from downstream. > > Reviewed-by: Rob Clark > Signed-off-by: Konrad Dybcio > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 39 > +++ > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index dfde5fb65eed..58bf405b85d8 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -786,10 +786,25 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > { > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > - u32 lower_bit = 2; > - u32 amsbc = 0; > + /* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */ > u32 rgb565_predicator = 0; > + /* Unknown, introduced with A650 family */ > u32 uavflagprd_inv = 0; > + /* Whether the minimum access length is 64 bits */ > + u32 min_acc_len = 0; > + /* Entirely magic, per-GPU-gen value */ > + u32 ubwc_mode = 0; > + /* > + * The Highest Bank Bit value represents the bit of the highest DDR > bank. > + * We then subtract 13 from it (13 is the minimum value allowed by hw) > and > + * write the lowest two bits of the remaining value as hbb_lo and the > + * one above it as hbb_hi to the hardware. This should ideally use DRAM > + * type detection. > + */ > + u32 hbb_hi = 0; > + u32 hbb_lo = 2; > + /* Unknown, introduced with A640/680 */ > + u32 amsbc = 0; > > /* a618 is using the hw default values */ > if (adreno_is_a618(adreno_gpu)) > @@ -800,25 +815,31 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > > if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) { > /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */ > - lower_bit = 3; > + hbb_lo = 3; > amsbc = 1; > rgb565_predicator = 1; > uavflagprd_inv = 2; > } > > if (adreno_is_7c3(adreno_gpu)) { > - lower_bit = 1; > + hbb_lo = 1; > amsbc = 1; > rgb565_predicator = 1; > uavflagprd_inv = 2; > } > > gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL, > - rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1); > - gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1); > - gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, > - uavflagprd_inv << 4 | lower_bit << 1); > - gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21); > + rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 | > + min_acc_len << 3 | hbb_lo << 1 | ubwc_mode); > + > + gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 | > + min_acc_len << 3 | hbb_lo << 1 | ubwc_mode); > + > + gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 | > + uavflagprd_inv << 4 | min_acc_len << 3 | > + hbb_lo << 1 | ubwc_mode); > + > + gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << > 21); > } > > static int a6xx_cp_init(struct msm_gpu *gpu) > Reviewed-by: Akhil P Oommen -Akhil > -- > 2.40.1 >
Re: [PATCH v8 08/18] drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init
On 9.06.2023 20:25, Akhil P Oommen wrote: > On Mon, May 29, 2023 at 03:52:27PM +0200, Konrad Dybcio wrote: >> >> Currently we're only deasserting REG_A6XX_RBBM_GBIF_HALT, but we also >> need REG_A6XX_GBIF_HALT to be set to 0. >> >> This is typically done automatically on successful GX collapse, but in >> case that fails, we should take care of it. >> >> Also, add a memory barrier to ensure it's gone through before jumping >> to further initialization. >> >> Reviewed-by: Dmitry Baryshkov >> Signed-off-by: Konrad Dybcio >> --- >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> index 083ccb5bcb4e..dfde5fb65eed 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> @@ -1003,8 +1003,12 @@ static int hw_init(struct msm_gpu *gpu) >> a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET); >> >> /* Clear GBIF halt in case GX domain was not collapsed */ >> -if (a6xx_has_gbif(adreno_gpu)) >> +if (a6xx_has_gbif(adreno_gpu)) { >> +gpu_write(gpu, REG_A6XX_GBIF_HALT, 0); >> gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0); >> +/* Let's make extra sure that the GPU can access the memory.. */ >> +mb(); > This barrier is unnecessary because writel transactions are ordered and > we don't expect a traffic from GPU immediately after this. > > -Akhil Right, let's remove it! Konrad >> +} >> >> gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0); >> >> >> -- >> 2.40.1 >>
Re: [PATCH v8 08/18] drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init
On Mon, May 29, 2023 at 03:52:27PM +0200, Konrad Dybcio wrote: > > Currently we're only deasserting REG_A6XX_RBBM_GBIF_HALT, but we also > need REG_A6XX_GBIF_HALT to be set to 0. > > This is typically done automatically on successful GX collapse, but in > case that fails, we should take care of it. > > Also, add a memory barrier to ensure it's gone through before jumping > to further initialization. > > Reviewed-by: Dmitry Baryshkov > Signed-off-by: Konrad Dybcio > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 083ccb5bcb4e..dfde5fb65eed 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -1003,8 +1003,12 @@ static int hw_init(struct msm_gpu *gpu) > a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET); > > /* Clear GBIF halt in case GX domain was not collapsed */ > - if (a6xx_has_gbif(adreno_gpu)) > + if (a6xx_has_gbif(adreno_gpu)) { > + gpu_write(gpu, REG_A6XX_GBIF_HALT, 0); > gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0); > + /* Let's make extra sure that the GPU can access the memory.. */ > + mb(); This barrier is unnecessary because writel transactions are ordered and we don't expect a traffic from GPU immediately after this. -Akhil > + } > > gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0); > > > -- > 2.40.1 >
Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices
Hi, On 2023/6/10 01:52, Bjorn Helgaas wrote: On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote: On 2023/6/9 01:32, Bjorn Helgaas wrote: On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote: From: Sui Jingfeng This patch adds PCI driver support on top of what we already have. Take the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver. There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000. Therefore, component frameworks can be avoided. + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0}, + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0}, PCI_VDEVICE() This make it impossible to hook device-specific data in the future. But currently there no device specific data associated with the 0x7a05 and 0x7a15, so it's acceptable for now. Thanks. Haha, ISTR having this conversation before, sorry for repeating it. Indeed, it's fine as-is. But PCI_VDEVICE() actually *does* allow for vendor-specific data because it doesn't include the data element, which defaults to zero if you don't specify it. So for example, drivers/net/ethernet/realtek/r8169_main.c has this: { PCI_VDEVICE(REALTEK, 0x8129) }, { PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT }, where 0x8129 has no driver_data (it defaults to zero), but 0x8136 does. Yeah, I'm wrong. PCI_VDEVICE macro end with two zero. (I thought it was three) Thanks for the education. With those lessons learned, I somewhat know how to create patch. It should meet community's requirement before sending. I'm too naive in the before. Thanks a lot, really. Bjorn -- Jingfeng
[PATCH v2] drm/sched: Make sure we wait for all dependencies in kill_jobs_cb()
drm_sched_entity_kill_jobs_cb() logic is omitting the last fence popped from the dependency array that was waited upon before drm_sched_entity_kill() was called (drm_sched_entity::dependency field), so we're basically waiting for all dependencies except one. In theory, this wait shouldn't be needed because resources should have their users registered to the dma_resv object, thus guaranteeing that future jobs wanting to access these resources wait on all the previous users (depending on the access type, of course). But we want to keep these explicit waits in the kill entity path just in case. Let's make sure we keep all dependencies in the array in drm_sched_job_dependency(), so we can iterate over the array and wait in drm_sched_entity_kill_jobs_cb(). Signed-off-by: Boris Brezillon Suggested-by: "Christian König" Cc: Frank Binns Cc: Sarah Walker Cc: Donald Robson Cc: Luben Tuikov Cc: David Airlie Cc: Daniel Vetter Cc: Sumit Semwal Cc: "Christian König" --- drivers/gpu/drm/scheduler/sched_entity.c | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 68e807ae136a..e1b437e66f3c 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -176,13 +176,14 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f, { struct drm_sched_job *job = container_of(cb, struct drm_sched_job, finish_cb); + unsigned long index; int r; dma_fence_put(f); /* Wait for all dependencies to avoid data corruptions */ - while (!xa_empty(&job->dependencies)) { - f = xa_erase(&job->dependencies, job->last_dependency++); + xa_for_each(&job->dependencies, index, f) { + xa_erase(&job->dependencies, index); r = dma_fence_add_callback(f, &job->finish_cb, drm_sched_entity_kill_jobs_cb); if (!r) @@ -415,8 +416,17 @@ static struct dma_fence * drm_sched_job_dependency(struct drm_sched_job *job, struct drm_sched_entity *entity) { - if (!xa_empty(&job->dependencies)) - return xa_erase(&job->dependencies, job->last_dependency++); + struct dma_fence *f; + + /* We keep the fence around, so we can iterate over all dependencies +* in drm_sched_entity_kill_jobs_cb() to make all deps are signaled +* before killing the job. +*/ + f = xa_load(&job->dependencies, job->last_dependency); + if (f) { + job->last_dependency++; + return dma_fence_get(f); + } if (job->sched->ops->prepare_job) return job->sched->ops->prepare_job(job, entity); -- 2.40.1
Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices
On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote: > On 2023/6/9 01:32, Bjorn Helgaas wrote: > > On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote: > > > From: Sui Jingfeng > > > > > > This patch adds PCI driver support on top of what we already have. Take > > > the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device > > > driver. There is only one GPU core for the GC1000 in the LS7A1000 and > > > LS2K1000. Therefore, component frameworks can be avoided. > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0}, > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0}, > > > > PCI_VDEVICE() > > This make it impossible to hook device-specific data in the future. > > But currently there no device specific data associated with the > 0x7a05 and 0x7a15, > > so it's acceptable for now. Thanks. Haha, ISTR having this conversation before, sorry for repeating it. Indeed, it's fine as-is. But PCI_VDEVICE() actually *does* allow for vendor-specific data because it doesn't include the data element, which defaults to zero if you don't specify it. So for example, drivers/net/ethernet/realtek/r8169_main.c has this: { PCI_VDEVICE(REALTEK, 0x8129) }, { PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT }, where 0x8129 has no driver_data (it defaults to zero), but 0x8136 does. Bjorn
[pull] amdgpu, amdkfd, radeon, ttm, drm, UAPI drm-next-6.5
Hi Dave, Daniel, New stuff for 6.5. Includes last week's PR with fixed sob tag, plus a few new things. The following changes since commit e82c98f2ca439356d5595ba8c9cd782f993f6f8c: Merge tag 'amd-drm-next-6.4-2023-04-14' of https://gitlab.freedesktop.org/agd5f/linux into drm-next (2023-04-17 10:54:59 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-next-6.5-2023-06-09 for you to fetch changes up to 3b718dcaf163d17fe907ea098c8449e0cd6bc271: drm/amd/display: Filter out AC mode frequencies on DC mode systems (2023-06-09 12:50:55 -0400) amd-drm-next-6.5-2023-06-02: amdgpu: - SR-IOV fixes - Warning fixes - Misc code cleanups and spelling fixes - DCN 3.2 updates - Improved DC FAMS support for better power management - Improved DC SubVP support for better power management - DCN 3.1.x fixes - Max IB size query - DC GPU reset fixes - RAS updates - DCN 3.0.x fixes - S/G display fixes - CP shadow buffer support - Implement connector force callback - Z8 power improvements - PSP 13.0.10 vbflash support - Mode2 reset fixes - Store MQDs in VRAM to improve queue switch latency - VCN 3.x fixes - JPEG 3.x fixes - Enable DC_FP on LoongArch - GFXOFF fixes - GC 9.4.3 partition support - SDMA 4.4.2 partition support - VCN/JPEG 4.0.3 partition support - VCN 4.0.3 updates - NBIO 7.9 updates - GC 9.4.3 updates - Take NUMA into account when allocating memory - Handle NUMA for partitions - SMU 13.0.6 updates - GC 9.4.3 RAS updates - Stop including unused swiotlb.h - SMU 13.0.7 fixes - Fix clock output ordering on some APUs - Clean up DC FPGA code - GFX9 preemption fixes - Misc irq fixes - S0ix fixes - Add new DRM_AMDGPU_WERROR config parameter to help with CI - PCIe fix for RDNA2 - kdoc fixes - Documentation updates amdkfd: - Query TTM mem limit rather than hardcoding it - GC 9.4.3 partition support - Handle NUMA for partitions radeon: - Fix possible double free - Stop including unused swiotlb.h - Fix possible division by zero ttm: - Add query for TTM mem limit - Add NUMA awareness to pools - Export ttm_pool_fini() UAPI: - Add new ctx query flag to better handle GPU resets Mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22290 - Add new interface to query and set shadow buffer for RDNA3 Mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21986 - Add new INFO query for max IB size Proposed userspace: https://gitlab.freedesktop.org/bnieuwenhuizen/mesa/-/commits/ib-rejection-v3 amd-drm-next-6.5-2023-06-09: amdgpu: - S0ix fixes - Initial SMU13 Overdrive support - kdoc fixes - Misc clode cleanups - Flexible array fixes - Display OTG fixes - SMU 13.0.6 updates - Revert some broken clock counter updates - Misc display fixes - GFX9 preemption fixes - Add support for newer EEPROM bad page table format - Add missing radeon secondary id - Add support for new colorspace KMS API - CSA fix - Stable pstate fixes for APUs - make vbl interface admin only - Handle PCI accelerator class amdkfd: - Add debugger support for gdb radeon: - Fix possible UAF drm: - Add Colorspace functionality UAPI: - Add debugger interface for enabling gdb Proposed userspace: https://github.com/ROCm-Developer-Tools/ROCdbgapi/tree/wip-dbgapi - Add KMS colorspace API Discussion: https://lists.freedesktop.org/archives/dri-devel/2023-June/408128.html Alan Liu (4): drm/amd/display: Fix in disabling secure display drm/amdgpu: Fix desktop freezed after gpu-reset drm/amd/display: Fix in secure display context creation drm/amd/display: Fix warning in disabling vblank irq Alex Deucher (37): drm/amdgpu/gfx11: add FW version check for new CP GFX shadow feature drm/amdgpu/gfx11: check the CP FW version CP GFX shadow support drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers drm/amdgpu: don't require a job for cond_exec and shadow drm/amdgpu: add UAPI to query GFX shadow sizes drm/amdgpu: add gfx shadow callback drm/amdgpu: add get_gfx_shadow_info callback for gfx11 drm/amdgpu: add support for new GFX shadow size query drm/amdgpu: bump driver version number for CP GFX shadow drm/amdgpu: track MQD size for gfx and compute drm/amdgpu: add debugfs interface for reading MQDs drm/amdgpu/gfx11: update gpu_clock_counter logic drm/amdgpu/gfx11: drop old bring up code drm/amdgpu/gfx10: drop old bring up code drm/amdgpu: add [en/dis]able_kgq() functions drm/amdgpu/gfx10: use generic [en/dis]able_kgq() helpers drm/amdgpu/gfx11: use generic [en/dis]able_kgq() helpers drm/amdgpu/gfx10: drop unused variable drm/amdgpu/gfx11: drop unused variable drm/amdgpu/gfx8: always restore kcq MQDs drm/amdgpu/gfx9: always restore kcq MQDs drm/amdgpu/gfx10: always restore kcq/kgq MQDs drm/a
Re: [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register
On 2023/6/10 00:48, Bjorn Helgaas wrote: On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote: On 2023/6/9 03:19, Bjorn Helgaas wrote: On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote: From: Sui Jingfeng The vga_is_firmware_default() function is arch-dependent, which doesn't sound right. At least, it also works on the Mips and LoongArch platforms. Tested with the drm/amdgpu and drm/radeon drivers. However, it's difficult to enumerate all arch-driver combinations. I'm wrong if there is only one exception. With the observation that device drivers typically have better knowledge about which PCI bar contains the firmware framebuffer, which could avoid the need to iterate all of the PCI BARs. But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is probably not suitable to make such an optimization for a specific device. There are PCI display controllers that don't have a dedicated VRAM bar, this function will lose its effectiveness in such a case. Luckily, the device driver can provide an accurate workaround. Therefore, this patch introduces a callback that allows the device driver to tell the VGAARB if the device is the default boot device. This patch only intends to introduce the mechanism, while the implementation is left to the device driver authors. Also honor the comment: "Clients have two callback mechanisms they can use" s/bar/BAR/ (several) Nothing here uses the callback. I don't want to merge this until we have a user. This is chicken and egg question. If you could help get this merge first, I will show you the first user. I'm not sure why the device driver should know whether its device is the default boot device. It's not that the device driver should know, but it's about that the device driver has the right to override. Device driver may have better approach to identify the default boot device. The way we usually handle this is to include the new callback in the same series as the first user of it. That has two benefits: (1) everybody can review the whole picture and possibly suggest different approaches, and (2) when we merge the infrastructure, we also merge a user of it at the same time, so the whole thing can be tested and we don't end up with unused code. OK, acceptable I will try to prepare the user code of this callback and respin the patch. I may resend it with another patch set in the future, this series already drop it, see v5[1] [1] https://patchwork.freedesktop.org/series/119134/ Bjorn
Re: [PATCH] drm/radeon: Disable outputs when releasing fbdev client
On Fri, Jun 09, 2023 at 04:03:56PM +0200, Thomas Zimmermann wrote: > Disable the modesetting pipeline before release the radeon's fbdev > client. Fixes the following error: > > [ 17.217408] WARNING: CPU: 5 PID: 1464 at drivers/gpu/drm/ttm/ttm_bo.c:326 > ttm_bo_release+0x27e/0x2d0 [ttm] > [ 17.217418] Modules linked in: edac_mce_amd radeon(+) drm_ttm_helper ttm > video drm_suballoc_helper drm_display_helper kvm irqbypass drm_kms_helper > syscopyarea crc32_pclmul sysfillrect sha512_ssse3 sysimgblt sha512_generic > cfbfillrect cfbimgblt wmi_bmof aesni_intel cfbcopyarea crypto_simd cryptd > k10temp acpi_cpufreq wmi dm_mod > [ 17.217432] CPU: 5 PID: 1464 Comm: systemd-udevd Not tainted 6.4.0-rc4+ #1 > [ 17.217436] Hardware name: Micro-Star International Co., Ltd. > MS-7A38/B450M PRO-VDH MAX (MS-7A38), BIOS B.G0 07/26/2022 > [ 17.217438] RIP: 0010:ttm_bo_release+0x27e/0x2d0 [ttm] > [ 17.217444] Code: 48 89 43 38 48 89 43 40 48 8b 5c 24 30 48 8b b5 40 08 00 > 00 48 8b 6c 24 38 48 83 c4 58 e9 7a 49 f7 e0 48 89 ef e9 6c fe ff ff <0f> 0b > 48 83 7b 20 00 0f 84 b7 fd ff ff 0f 0b 0f 1f 00 e9 ad fd ff > [ 17.217448] RSP: 0018:c995fbb0 EFLAGS: 00010202 > [ 17.217451] RAX: 0001 RBX: 8881052c8de0 RCX: > > [ 17.217453] RDX: 0001 RSI: RDI: > 8881052c8de0 > [ 17.217455] RBP: 888104a66e00 R08: 8881052c8de0 R09: > 888104a7cf08 > [ 17.217457] R10: c995fbe0 R11: c995fbe8 R12: > 8881052c8c78 > [ 17.217458] R13: 8881052c8c78 R14: dead0100 R15: > 88810528b108 > [ 17.217460] FS: 7f319fcbb8c0() GS:1a54() > knlGS: > [ 17.217463] CS: 0010 DS: ES: CR0: 80050033 > [ 17.217464] CR2: 55dc8b0224a0 CR3: 00010373d000 CR4: > 00750ee0 > [ 17.217466] PKRU: 5554 > [ 17.217468] Call Trace: > [ 17.217470] > [ 17.217472] ? __warn+0x97/0x160 > [ 17.217476] ? ttm_bo_release+0x27e/0x2d0 [ttm] > [ 17.217481] ? report_bug+0x1ec/0x200 > [ 17.217487] ? handle_bug+0x3c/0x70 > [ 17.217490] ? exc_invalid_op+0x1f/0x90 > [ 17.217493] ? preempt_count_sub+0xb5/0x100 > [ 17.217496] ? asm_exc_invalid_op+0x16/0x20 > [ 17.217500] ? ttm_bo_release+0x27e/0x2d0 [ttm] > [ 17.217505] ? ttm_resource_move_to_lru_tail+0x1ab/0x1d0 [ttm] > [ 17.217511] radeon_bo_unref+0x1a/0x30 [radeon] > [ 17.217547] radeon_gem_object_free+0x20/0x30 [radeon] > [ 17.217579] radeon_fbdev_fb_destroy+0x57/0x90 [radeon] > [ 17.217616] unregister_framebuffer+0x72/0x110 > [ 17.217620] drm_client_dev_unregister+0x6d/0xe0 > [ 17.217623] drm_dev_unregister+0x2e/0x90 > [ 17.217626] drm_put_dev+0x26/0x90 > [ 17.217628] pci_device_remove+0x44/0xc0 > [ 17.217631] really_probe+0x257/0x340 > [ 17.217635] __driver_probe_device+0x73/0x120 > [ 17.217638] driver_probe_device+0x2c/0xb0 > [ 17.217641] __driver_attach+0xa0/0x150 > [ 17.217643] ? __pfx___driver_attach+0x10/0x10 > [ 17.217646] bus_for_each_dev+0x67/0xa0 > [ 17.217649] bus_add_driver+0x10e/0x210 > [ 17.217651] driver_register+0x5c/0x120 > [ 17.217653] ? __pfx_radeon_module_init+0x10/0x10 [radeon] > [ 17.217681] do_one_initcall+0x44/0x220 > [ 17.217684] ? kmalloc_trace+0x37/0xc0 > [ 17.217688] do_init_module+0x64/0x240 > [ 17.217691] __do_sys_finit_module+0xb2/0x100 > [ 17.217694] do_syscall_64+0x3b/0x90 > [ 17.217697] entry_SYSCALL_64_after_hwframe+0x72/0xdc > [ 17.217700] RIP: 0033:0x7f319feaa5a9 > [ 17.217702] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 89 > f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d > 01 f0 ff ff 73 01 c3 48 8b 0d 27 08 0d 00 f7 d8 64 89 01 48 > [ 17.217706] RSP: 002b:7ffc6bf3e7f8 EFLAGS: 0246 ORIG_RAX: > 0139 > [ 17.217709] RAX: ffda RBX: 5607204f3170 RCX: > 7f319feaa5a9 > [ 17.217710] RDX: RSI: 7f31a002eefd RDI: > 0018 > [ 17.217712] RBP: 7f31a002eefd R08: R09: > 5607204f1860 > [ 17.217714] R10: 0018 R11: 0246 R12: > 0002 > [ 17.217716] R13: R14: 560720522450 R15: > 560720255899 > [ 17.217718] > [ 17.217719] ---[ end trace ]--- > > The buffer object backing the fbdev emulation got pinned twice: by the > fb_probe helper radeon_fbdev_create_pinned_object() and the modesetting > code when the framebuffer got displayed. It only got unpinned once by > the fbdev helper radeon_fbdev_destroy_pinned_object(). Hence TTM's BO- > release function complains about the pin counter. Forcing the outputs > off also undoes the modesettings pin increment. > > Reported-by: Borislav Petkov > Closes: > https://lore.kernel.org/dri-devel/20230603174814.GCZHt83pN+wNjf63sC@fat_crate.local/ > Signed-off-by: Thomas Zimmermann > Fixes: e317a69fe891 ("drm/rad
Re: [PATCH] drm/edid: Add quirk for OSVR HDK 2.0
On 6/9/23 02:03, Jani Nikula wrote: On Thu, 08 Jun 2023, Ralph Campbell wrote: The OSVR virtual reality headset HDK 2.0 uses a different EDID vendor and device identifier than the HDK 1.1 - 1.4 headsets. Add the HDK 2.0 vendor and device identifier to the quirks table so that window managers do not try to display the desktop screen on the headset display. At some point in time we requested bugs to be filed about quirks, with EDIDs attached, so we could look at them later, and maybe remove the quirks. The headset non-desktop thing started off as a quirk, but since then we've added both Microsoft VSDB and DisplayID primary use as ways to indicate this without quirks. BR, Jani. If you want me to file a bug, I can do that and I have the EDID too. Where would I file it? I did see the DisplayID 2.0 code. This headset is no longer being manufactured so updating the EDID is not practical. Signed-off-by: Ralph Campbell Tested-by: Ralph Campbell --- drivers/gpu/drm/drm_edid.c | 1 + 1 file changed, 1 insertion(+) I don't know how many of these VR headsets are still around but I have a working one and I saw and entry for HDK 1.x so I thought it would be good to add HDK 2.0. diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 0454da505687..3b8cc1fe05e8 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -230,6 +230,7 @@ static const struct edid_quirk { /* OSVR HDK and HDK2 VR Headsets */ EDID_QUIRK('S', 'V', 'R', 0x1019, EDID_QUIRK_NON_DESKTOP), + EDID_QUIRK('A', 'O', 'U', 0x, EDID_QUIRK_NON_DESKTOP), }; /*
Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
On 6/9/2023 9:58 AM, Dmitry Baryshkov wrote: On 08/06/2023 23:36, Marijn Suijten wrote: Same title suggestion as earlier: s/adjust/reduce On 2023-05-22 18:08:56, Jessica Zhang wrote: Adjust the pclk rate to divide hdisplay by the compression ratio when DSC is enabled. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index a448931af804..88f370dd2ea1 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -561,7 +561,18 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host) clk_disable_unprepare(msm_host->byte_clk); } -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi) +static unsigned long dsi_adjust_compressed_pclk(const struct drm_display_mode *mode, Nit: adjust_pclk_for_compression As discussed before we realized that this change is more-or-less a hack, since downstream calculates pclk quite differently - at least for command-mode panels. Do you still intend to land this patch this way, or go the proper route by introducing the right math from the get-go? Or is the math at least correct for video-mode panels? Can we please postpone the cmd-vs-video discussion? Otherwise I will reserve myself a right to push a patch dropping CMD mode support until somebody comes with a proper way to handle CMD clock calculation. It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel timings are a kind of a hack and should be improved. No, we can not do this as a part of this series. I think everybody agrees that with the current way of calculating CMD panel timings, this function does some kind of a trick. This function requires a documentation comment to explain that all. + const struct drm_dsc_config *dsc) +{ + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if bits_per_component==8 is assumed. In fact, it then becomes identical to the following line in dsi_host.c which you added previously: hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); This would imply a simple /3, but as far as I understand it is not correct here. If not, what is the difference between these two calculations? Maybe they both need to be in a properly-named helper. + dsc->bits_per_component * 3); I hope to see a documentation patch to be posted, telling that this scales hdisplay and thus pclk by the factor of compressed_bpp / uncompressed_bpp. This is not how it is usually done, but I would accept a separate documentation patch going over the calculation here and in dsi_timing_setup (and maybe other unobvious cases, if there is anything left). As we established in the drm/msm issue [2] there is currently a confusion whether this /3 (and the /3 in dsi_timing_setup) come from the ratio between dsi_get_bpp() and dsc->bpp or something else. Can you clarify that with constants and comments? [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24 + + return (new_hdisplay + (mode->htotal - mode->hdisplay)) + * mode->vtotal * drm_mode_vrefresh(mode); As clarified in [1] I was not necessarily suggesting to move this math to a separate helper, but to also use a few more properly-named intermediate variables to not have multi-line math and self-documenting code. These lines could be split to be much more clear. I think it's fine more or less. One pair of parenthesis is unnecessary, but that's mostly it. Maybe `new_htotal' variable would make some sense. Also, please excuse me if this was discussed somewhere. This calculation means that only the displayed data is compressed, but porches are not touched. Correct? Hi Dmitry, Correct, we will apply the compression ratio to only the hdisplay but keep the porches as is. [1]: https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/ +} + +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, + const struct drm_dsc_config *dsc, bool is_bonded_dsi) { unsigned long pclk_rate; @@ -576,6 +587,10 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool if (is_bonded_dsi) pclk_rate /= 2; + /* If DSC is enabled, divide hdisplay by compression ratio */ + if (dsc) + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc); Looking for the perfection, I'd also move the pclk adjustment to come before the is_bonded_dsi check. Acked. Thanks, Jessica Zhang The confusion with this comment (and the reason the aforementioned discussion [2] carried on so long) stems from the fact a division makes sense for a bit/byte clock, but not for a pixel clock: we st
Re: [PATCH 0/2] accel/qaic fixes for 6.4 part 2
On 6/2/2023 3:04 PM, Jeffrey Hugo wrote: Two additional fixes for corner cases found during development when buggy userspace or firmware ends up subjecting the KMD to error scenarios. Carl Vanderlip (1): accel/qaic: Free user handle on interrupted mutex Jeffrey Hugo (1): accel/qaic: Fix NULL pointer deref in qaic_destroy_drm_device() drivers/accel/qaic/qaic_drv.c | 4 1 file changed, 4 insertions(+) Pushed to drm-misc-fixes
Re: [PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj
On Fri, Jun 9, 2023 at 7:12 AM Tvrtko Ursulin wrote: > > > On 09/06/2023 13:44, Iddamsetty, Aravind wrote: > > On 09-06-2023 17:41, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin > >> > >> I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which > >> will return a reference to a newly created GEM objects (if created), in > >> order to enable tracking of imported i915 GEM objects in the following > >> patch. > > > > instead of this what if we implement the open call back in i915 > > > > struct drm_gem_object_funcs { > > > > /** > > * @open: > > * > > * Called upon GEM handle creation. > > * > > * This callback is optional. > > */ > > int (*open)(struct drm_gem_object *obj, struct drm_file *file); > > > > which gets called whenever a handle(drm_gem_handle_create_tail) is > > created and in the open we can check if to_intel_bo(obj)->base.dma_buf > > then it is imported if not it is owned or created by it. > > I wanted to track as much memory usage as we have which is buffer object > backed, including page tables and contexts. And since those are not user > visible (they don't have handles), they wouldn't be covered by the > obj->funcs->open() callback. > > If we wanted to limit to objects with handles we could simply do what > Rob proposed and that is to walk the handles idr. But that does not feel > like the right direction to me. Open for discussion I guess. I guess you just have a few special case objects per context? Wouldn't it be easier to just track _those_ specially and append them to the results after doing the normal idr table walk? (Also, doing something special for dma-buf smells a bit odd.. considering that we also have legacy flink name based sharing as well.) BR, -R
[PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()
The driver only supports OLED controllers that have a native DRM_FORMAT_C1 pixel format and that is why it has harcoded a division of the width by 8. But the driver might be extended to support devices that have a different pixel format. So it's better to use the struct drm_format_info helpers to compute the size of the buffer, used to store the pixels in native format. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann --- (no changes since v1) drivers/gpu/drm/solomon/ssd130x.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 0be3b476dc60..b3dc1ca9dc10 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -150,9 +150,16 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) { unsigned int page_height = ssd130x->device_info->page_height; unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); + const struct drm_format_info *fi; + unsigned int pitch; - ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), - ssd130x->height, GFP_KERNEL); + fi = drm_format_info(DRM_FORMAT_C1); + if (!fi) + return -EINVAL; + + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); + + ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); if (!ssd130x->buffer) return -ENOMEM; -- 2.40.1
[PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
The resolutions for these panels are fixed and defined in the Device Tree, so there's no point to allocate the buffers on each plane update and that can just be done once. Let's do the allocation and free on the encoder enable and disable helpers since that's where others initialization and teardown operations are done. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann --- (no changes since v1) drivers/gpu/drm/solomon/ssd130x.c | 88 +++ drivers/gpu/drm/solomon/ssd130x.h | 3 ++ 2 files changed, 56 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 5cac1149e34e..0be3b476dc60 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -146,6 +146,31 @@ static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm) return container_of(drm, struct ssd130x_device, drm); } +static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x) +{ + unsigned int page_height = ssd130x->device_info->page_height; + unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height); + + ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), + ssd130x->height, GFP_KERNEL); + if (!ssd130x->buffer) + return -ENOMEM; + + ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); + if (!ssd130x->data_array) { + kfree(ssd130x->buffer); + return -ENOMEM; + } + + return 0; +} + +static void ssd130x_buf_free(struct ssd130x_device *ssd130x) +{ + kfree(ssd130x->data_array); + kfree(ssd130x->buffer); +} + /* * Helper to write data (SSD130X_DATA) to the device. */ @@ -434,11 +459,12 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) SSD130X_SET_ADDRESS_MODE_HORIZONTAL); } -static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, - struct drm_rect *rect) +static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect) { unsigned int x = rect->x1; unsigned int y = rect->y1; + u8 *buf = ssd130x->buffer; + u8 *data_array = ssd130x->data_array; unsigned int width = drm_rect_width(rect); unsigned int height = drm_rect_height(rect); unsigned int line_length = DIV_ROUND_UP(width, 8); @@ -447,14 +473,9 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, struct drm_device *drm = &ssd130x->drm; u32 array_idx = 0; int ret, i, j, k; - u8 *data_array = NULL; drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n"); - data_array = kcalloc(width, pages, GFP_KERNEL); - if (!data_array) - return -ENOMEM; - /* * The screen is divided in pages, each having a height of 8 * pixels, and the width of the screen. When sending a byte of @@ -488,11 +509,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, /* Set address range for horizontal addressing mode */ ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width); if (ret < 0) - goto out_free; + return ret; ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y / 8, pages); if (ret < 0) - goto out_free; + return ret; } for (i = 0; i < pages; i++) { @@ -522,11 +543,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, ssd130x->page_offset + i, ssd130x->col_offset + x); if (ret < 0) - goto out_free; + return ret; ret = ssd130x_write_data(ssd130x, data_array, width); if (ret < 0) - goto out_free; + return ret; array_idx = 0; } @@ -536,14 +557,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, if (!ssd130x->page_address_mode) ret = ssd130x_write_data(ssd130x, data_array, width * pages); -out_free: - kfree(data_array); return ret; } static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) { - u8 *buf = NULL; struct drm_rect fullscreen = { .x1 = 0, .x2 = ssd130x->width, @@ -551,14 +569,7 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) .y2 = ssd130x->height, }; - buf = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), ssd130x->height, -
[PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
A default resolution in the ssd130x driver isn't set to an arbitrary 96x16 anymore. Instead is set to a width and height that's controller dependent. The datasheets for the chips describes the following display resolutions: - SH1106: 132 x 64 Dot Matrix OLED/PLED - SSD1305: 132 x 64 Dot Matrix OLED/PLED - SSD1306: 128 x 64 Dot Matrix OLED/PLED - SSD1307: 128 x 39 Dot Matrix OLED/PLED - SSD1309: 128 x 64 Dot Matrix OLED/PLED Update DT schema to reflect what the driver does and make its users aware. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann --- Changes in v2: - List per controller default width/height values in DT schema (Maxime Ripard). .../bindings/display/solomon,ssd1307fb.yaml | 28 --- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml index 94bb5ef567c6..20e2bd15d4d2 100644 --- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml @@ -49,15 +49,15 @@ properties: solomon,height: $ref: /schemas/types.yaml#/definitions/uint32 -default: 16 description: - Height in pixel of the screen driven by the controller + Height in pixel of the screen driven by the controller. + The default value is controller-dependent. solomon,width: $ref: /schemas/types.yaml#/definitions/uint32 -default: 96 description: - Width in pixel of the screen driven by the controller + Width in pixel of the screen driven by the controller. + The default value is controller-dependent. solomon,page-offset: $ref: /schemas/types.yaml#/definitions/uint32 @@ -157,6 +157,10 @@ allOf: const: sinowealth,sh1106 then: properties: +width: + default: 132 +height: + default: 64 solomon,dclk-div: default: 1 solomon,dclk-frq: @@ -171,6 +175,10 @@ allOf: - solomon,ssd1305 then: properties: +width: + default: 132 +height: + default: 64 solomon,dclk-div: default: 1 solomon,dclk-frq: @@ -185,6 +193,10 @@ allOf: - solomon,ssd1306 then: properties: +width: + default: 128 +height: + default: 64 solomon,dclk-div: default: 1 solomon,dclk-frq: @@ -199,6 +211,10 @@ allOf: - solomon,ssd1307 then: properties: +width: + default: 128 +height: + default: 39 solomon,dclk-div: default: 2 solomon,dclk-frq: @@ -215,6 +231,10 @@ allOf: - solomon,ssd1309 then: properties: +width: + default: 128 +height: + default: 64 solomon,dclk-div: default: 1 solomon,dclk-frq: -- 2.40.1
[PATCH v2 3/5] drm/ssd130x: Set the page height value in the device info data
The driver only supports OLED controllers that have a page height of 8 but there are devices that have different page heights. So it is better to not hardcode this value and instead have it as a per controller data value. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann --- (no changes since v1) drivers/gpu/drm/solomon/ssd130x.c | 15 +++ drivers/gpu/drm/solomon/ssd130x.h | 1 + 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index a0e5e26c0bc9..5cac1149e34e 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -102,6 +102,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { .default_width = 132, .default_height = 64, .page_mode_only = 1, + .page_height = 8, }, [SSD1305_ID] = { .default_vcomh = 0x34, @@ -109,6 +110,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { .default_dclk_frq = 7, .default_width = 132, .default_height = 64, + .page_height = 8, }, [SSD1306_ID] = { .default_vcomh = 0x20, @@ -117,6 +119,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { .need_chargepump = 1, .default_width = 128, .default_height = 64, + .page_height = 8, }, [SSD1307_ID] = { .default_vcomh = 0x20, @@ -125,6 +128,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { .need_pwm = 1, .default_width = 128, .default_height = 39, + .page_height = 8, }, [SSD1309_ID] = { .default_vcomh = 0x34, @@ -132,6 +136,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { .default_dclk_frq = 10, .default_width = 128, .default_height = 64, + .page_height = 8, } }; EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); @@ -437,7 +442,8 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, unsigned int width = drm_rect_width(rect); unsigned int height = drm_rect_height(rect); unsigned int line_length = DIV_ROUND_UP(width, 8); - unsigned int pages = DIV_ROUND_UP(height, 8); + unsigned int page_height = ssd130x->device_info->page_height; + unsigned int pages = DIV_ROUND_UP(height, page_height); struct drm_device *drm = &ssd130x->drm; u32 array_idx = 0; int ret, i, j, k; @@ -559,16 +565,17 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m struct drm_rect *rect) { struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); + unsigned int page_height = ssd130x->device_info->page_height; struct iosys_map dst; unsigned int dst_pitch; int ret = 0; u8 *buf = NULL; /* Align y to display page boundaries */ - rect->y1 = round_down(rect->y1, 8); - rect->y2 = min_t(unsigned int, round_up(rect->y2, 8), ssd130x->height); + rect->y1 = round_down(rect->y1, page_height); + rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height); buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL); if (!buf) return -ENOMEM; diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h index a2bc8d75078b..87968b3e7fb8 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -39,6 +39,7 @@ struct ssd130x_deviceinfo { u32 default_dclk_frq; u32 default_width; u32 default_height; + u32 page_height; int need_pwm; int need_chargepump; bool page_mode_only; -- 2.40.1
[PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent
Currently the driver hardcodes the default values to 96x16 pixels but this default resolution depends on the controller. The datasheets for the chips describes the following display controller resolutions: - SH1106: 132 x 64 Dot Matrix OLED/PLED - SSD1305: 132 x 64 Dot Matrix OLED/PLED - SSD1306: 128 x 64 Dot Matrix OLED/PLED - SSD1307: 128 x 39 Dot Matrix OLED/PLED - SSD1309: 128 x 64 Dot Matrix OLED/PLED Add this information to the devices' info structures, and use it set as a default if not defined in DT rather than hardcoding to an arbitrary value. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann --- (no changes since v1) drivers/gpu/drm/solomon/ssd130x.c | 14 -- drivers/gpu/drm/solomon/ssd130x.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 8cbf5aa66e19..a0e5e26c0bc9 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -99,29 +99,39 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = { .default_vcomh = 0x40, .default_dclk_div = 1, .default_dclk_frq = 5, + .default_width = 132, + .default_height = 64, .page_mode_only = 1, }, [SSD1305_ID] = { .default_vcomh = 0x34, .default_dclk_div = 1, .default_dclk_frq = 7, + .default_width = 132, + .default_height = 64, }, [SSD1306_ID] = { .default_vcomh = 0x20, .default_dclk_div = 1, .default_dclk_frq = 8, .need_chargepump = 1, + .default_width = 128, + .default_height = 64, }, [SSD1307_ID] = { .default_vcomh = 0x20, .default_dclk_div = 2, .default_dclk_frq = 12, .need_pwm = 1, + .default_width = 128, + .default_height = 39, }, [SSD1309_ID] = { .default_vcomh = 0x34, .default_dclk_div = 1, .default_dclk_frq = 10, + .default_width = 128, + .default_height = 64, } }; EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X); @@ -798,10 +808,10 @@ static void ssd130x_parse_properties(struct ssd130x_device *ssd130x) struct device *dev = ssd130x->dev; if (device_property_read_u32(dev, "solomon,width", &ssd130x->width)) - ssd130x->width = 96; + ssd130x->width = ssd130x->device_info->default_width; if (device_property_read_u32(dev, "solomon,height", &ssd130x->height)) - ssd130x->height = 16; + ssd130x->height = ssd130x->device_info->default_height; if (device_property_read_u32(dev, "solomon,page-offset", &ssd130x->page_offset)) ssd130x->page_offset = 1; diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h index db03ee5db392..a2bc8d75078b 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -37,6 +37,8 @@ struct ssd130x_deviceinfo { u32 default_vcomh; u32 default_dclk_div; u32 default_dclk_frq; + u32 default_width; + u32 default_height; int need_pwm; int need_chargepump; bool page_mode_only; -- 2.40.1
[PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups
Hello, While working on adding support for the SSD132X family of 4-bit grayscale Solomon OLED panel controllers, I noticed a few things in the driver that can be improved and make extending to support other chip families easier. I've split the preparatory patches in this series and will post the actual SSD132X support as a separate patch-set once this one is merged. Best regards, Javier Changes in v2: - List per controller default width/height values in DT schema (Maxime Ripard). Javier Martinez Canillas (5): drm/ssd130x: Make default width and height to be controller dependent dt-bindings: display: ssd1307fb: Remove default width and height values drm/ssd130x: Set the page height value in the device info data drm/ssd130x: Don't allocate buffers on each plane update drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc() .../bindings/display/solomon,ssd1307fb.yaml | 28 +++- drivers/gpu/drm/solomon/ssd130x.c | 124 -- drivers/gpu/drm/solomon/ssd130x.h | 6 + 3 files changed, 113 insertions(+), 45 deletions(-) -- 2.40.1
Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
On 08/06/2023 23:36, Marijn Suijten wrote: Same title suggestion as earlier: s/adjust/reduce On 2023-05-22 18:08:56, Jessica Zhang wrote: Adjust the pclk rate to divide hdisplay by the compression ratio when DSC is enabled. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index a448931af804..88f370dd2ea1 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -561,7 +561,18 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host) clk_disable_unprepare(msm_host->byte_clk); } -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi) +static unsigned long dsi_adjust_compressed_pclk(const struct drm_display_mode *mode, Nit: adjust_pclk_for_compression As discussed before we realized that this change is more-or-less a hack, since downstream calculates pclk quite differently - at least for command-mode panels. Do you still intend to land this patch this way, or go the proper route by introducing the right math from the get-go? Or is the math at least correct for video-mode panels? Can we please postpone the cmd-vs-video discussion? Otherwise I will reserve myself a right to push a patch dropping CMD mode support until somebody comes with a proper way to handle CMD clock calculation. It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel timings are a kind of a hack and should be improved. No, we can not do this as a part of this series. I think everybody agrees that with the current way of calculating CMD panel timings, this function does some kind of a trick. This function requires a documentation comment to explain that all. + const struct drm_dsc_config *dsc) +{ + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if bits_per_component==8 is assumed. In fact, it then becomes identical to the following line in dsi_host.c which you added previously: hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); This would imply a simple /3, but as far as I understand it is not correct here. If not, what is the difference between these two calculations? Maybe they both need to be in a properly-named helper. + dsc->bits_per_component * 3); I hope to see a documentation patch to be posted, telling that this scales hdisplay and thus pclk by the factor of compressed_bpp / uncompressed_bpp. This is not how it is usually done, but I would accept a separate documentation patch going over the calculation here and in dsi_timing_setup (and maybe other unobvious cases, if there is anything left). As we established in the drm/msm issue [2] there is currently a confusion whether this /3 (and the /3 in dsi_timing_setup) come from the ratio between dsi_get_bpp() and dsc->bpp or something else. Can you clarify that with constants and comments? [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24 + + return (new_hdisplay + (mode->htotal - mode->hdisplay)) + * mode->vtotal * drm_mode_vrefresh(mode); As clarified in [1] I was not necessarily suggesting to move this math to a separate helper, but to also use a few more properly-named intermediate variables to not have multi-line math and self-documenting code. These lines could be split to be much more clear. I think it's fine more or less. One pair of parenthesis is unnecessary, but that's mostly it. Maybe `new_htotal' variable would make some sense. Also, please excuse me if this was discussed somewhere. This calculation means that only the displayed data is compressed, but porches are not touched. Correct? [1]: https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/ +} + +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, + const struct drm_dsc_config *dsc, bool is_bonded_dsi) { unsigned long pclk_rate; @@ -576,6 +587,10 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool if (is_bonded_dsi) pclk_rate /= 2; + /* If DSC is enabled, divide hdisplay by compression ratio */ + if (dsc) + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc); Looking for the perfection, I'd also move the pclk adjustment to come before the is_bonded_dsi check. The confusion with this comment (and the reason the aforementioned discussion [2] carried on so long) stems from the fact a division makes sense for a bit/byte clock, but not for a pixel clock: we still intend to send the same number of pixels, just spending less bytes on them. So as you clarify the /3 above, can you also cl
[linux-next:master] BUILD REGRESSION 53ab6975c12d1ad86c599a8927e8c698b144d669
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 53ab6975c12d1ad86c599a8927e8c698b144d669 Add linux-next specific files for 20230609 Error/Warning reports: https://lore.kernel.org/oe-kbuild-all/202306081708.gtvacxsh-...@intel.com https://lore.kernel.org/oe-kbuild-all/202306082341.uqtcm8po-...@intel.com https://lore.kernel.org/oe-kbuild-all/202306100035.vtusnhm4-...@intel.com https://lore.kernel.org/oe-kbuild-all/202306100056.z3g9guft-...@intel.com Error/Warning: (recently discovered and may have been fixed) drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:113:17: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat] drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:147:17: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat] drivers/leds/leds-cht-wcove.c:144:21: warning: no previous prototype for 'cht_wc_leds_brightness_get' [-Wmissing-prototypes] drivers/scsi/FlashPoint.c:1712:12: warning: stack frame size (1056) exceeds limit (1024) in 'FlashPoint_HandleInterrupt' [-Wframe-larger-than] 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=] include/drm/drm_print.h:456:39: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=] uvdevice.c:(.init.text+0x2e): undefined reference to `uv_info' Unverified Error/Warning (likely false positive, please contact us if interested): drivers/net/ethernet/emulex/benet/be_main.c:2460 be_rx_compl_process_gro() error: buffer overflow '((skb_end_pointer(skb)))->frags' 17 <= u16max drivers/usb/cdns3/cdns3-starfive.c:23: warning: expecting prototype for cdns3(). Prototype was for USB_STRAP_HOST() instead fs/btrfs/volumes.c:6407 btrfs_map_block() error: we previously assumed 'mirror_num_ret' could be null (see line 6245) kernel/events/uprobes.c:478 uprobe_write_opcode() warn: passing zero to 'PTR_ERR' Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- i386-allyesconfig | |-- drivers-leds-leds-cht-wcove.c:warning:no-previous-prototype-for-cht_wc_leds_brightness_get | `-- include-drm-drm_print.h:error:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int |-- i386-randconfig-i004-20230608 | `-- include-drm-drm_print.h:error:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int |-- i386-randconfig-i062-20230608 | `-- include-drm-drm_print.h:warning:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int |-- riscv-allmodconfig | `-- drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead |-- riscv-allyesconfig | `-- drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead |-- s390-randconfig-r044-20230609 | `-- uvdevice.c:(.init.text):undefined-reference-to-uv_info |-- x86_64-allyesconfig | `-- drivers-leds-leds-cht-wcove.c:warning:no-previous-prototype-for-cht_wc_leds_brightness_get `-- x86_64-randconfig-m001-20230608 |-- drivers-net-ethernet-emulex-benet-be_main.c-be_rx_compl_process_gro()-error:buffer-overflow-((skb_end_pointer(skb)))-frags-u16max |-- fs-btrfs-volumes.c-btrfs_map_block()-error:we-previously-assumed-mirror_num_ret-could-be-null-(see-line-) `-- kernel-events-uprobes.c-uprobe_write_opcode()-warn:passing-zero-to-PTR_ERR clang_recent_errors |-- i386-randconfig-i014-20230608 | `-- drivers-gpu-drm-i915-pxp-intel_pxp_gsccs.c:warning:format-specifies-type-long-but-the-argument-has-type-size_t-(aka-unsigned-int-) |-- i386-randconfig-r026-20230608 | `-- drivers-gpu-drm-i915-pxp-intel_pxp_gsccs.c:warning:format-specifies-type-long-but-the-argument-has-type-size_t-(aka-unsigned-int-) `-- powerpc-allmodconfig `-- drivers-scsi-FlashPoint.c:warning:stack-frame-size-()-exceeds-limit-()-in-FlashPoint_HandleInterrupt elapsed time: 729m configs tested: 149 configs skipped: 6 tested configs: alphaallyesconfig gcc alpha defconfig gcc arc allyesconfig gcc arc defconfig gcc arc haps_hs_defconfig gcc arc randconfig-r011-20230608 gcc arc randconfig-r033-20230608 gcc arc randconfig-r043-20230608 gcc arm allmodconfig gcc arm allyesconfig gcc arm
Re: [PATCH v2] drm/i915/gsc: take a wakeref for the proxy-init-completion check
On 6/8/2023 4:07 PM, Alan Previn wrote: Ensure intel_gsc_uc_fw_init_done and intel_gsc_uc_fw_proxy_init takes a wakeref before reading GSC Shim registers. NOTE: another patch in review also adds a call from selftest to this same function. (https://patchwork.freedesktop.org/series/117713/) which is why i am adding the wakeref inside the callee, not the caller. v2: - add a helper, 'gsc_uc_get_fw_status' for both callers (Daniele Ceraolo) Fixes: 99afb7cc8c44 ("drm/i915/pxp: Add ARB session creation and cleanup") Signed-off-by: Alan Previn Reviewed-by: Daniele Ceraolo Spurio Daniele --- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 22 ++ 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index f46eb17a7a98..60e9c6c9e775 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -24,21 +24,27 @@ static bool gsc_is_in_reset(struct intel_uncore *uncore) GSC_FW_CURRENT_STATE_RESET; } -bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) +static u32 gsc_uc_get_fw_status(struct intel_uncore *uncore) { - struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore; - u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); + intel_wakeref_t wakeref; + u32 fw_status = 0; - return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) == + with_intel_runtime_pm(uncore->rpm, wakeref) + fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); + + return fw_status; +} + +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) +{ + return REG_FIELD_GET(GSC_FW_CURRENT_STATE, +gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore)) == GSC_FW_PROXY_STATE_NORMAL; } bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) { - struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore; - u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); - - return fw_status & GSC_FW_INIT_COMPLETE_BIT; + return gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore) & GSC_FW_INIT_COMPLETE_BIT; } static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc) base-commit: 27187d09511e1d47dbaaf91c7332319551a8edab
Re: [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register
On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote: > On 2023/6/9 03:19, Bjorn Helgaas wrote: > > On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote: > > > From: Sui Jingfeng > > > > > > The vga_is_firmware_default() function is arch-dependent, which doesn't > > > sound right. At least, it also works on the Mips and LoongArch platforms. > > > Tested with the drm/amdgpu and drm/radeon drivers. However, it's difficult > > > to enumerate all arch-driver combinations. I'm wrong if there is only one > > > exception. > > > > > > With the observation that device drivers typically have better knowledge > > > about which PCI bar contains the firmware framebuffer, which could avoid > > > the need to iterate all of the PCI BARs. > > > > > > But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is > > > probably not suitable to make such an optimization for a specific device. > > > > > > There are PCI display controllers that don't have a dedicated VRAM bar, > > > this function will lose its effectiveness in such a case. Luckily, the > > > device driver can provide an accurate workaround. > > > > > > Therefore, this patch introduces a callback that allows the device driver > > > to tell the VGAARB if the device is the default boot device. This patch > > > only intends to introduce the mechanism, while the implementation is left > > > to the device driver authors. Also honor the comment: "Clients have two > > > callback mechanisms they can use" > > s/bar/BAR/ (several) > > > > Nothing here uses the callback. I don't want to merge this until we > > have a user. > > This is chicken and egg question. > > If you could help get this merge first, I will show you the first user. > > > I'm not sure why the device driver should know whether its device is > > the default boot device. > > It's not that the device driver should know, > > but it's about that the device driver has the right to override. > > Device driver may have better approach to identify the default boot > device. The way we usually handle this is to include the new callback in the same series as the first user of it. That has two benefits: (1) everybody can review the whole picture and possibly suggest different approaches, and (2) when we merge the infrastructure, we also merge a user of it at the same time, so the whole thing can be tested and we don't end up with unused code. Bjorn
[PATCH] drm/amd/amdgpu: enable W=1 for amdgpu
We have a clean build with W=1 as of commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable these checks unconditionally for the entire module to catch these errors during development. Cc: Alex Deucher Cc: Nathan Chancellor Signed-off-by: Hamza Mahfooz --- drivers/gpu/drm/amd/amdgpu/Makefile | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile index 86b833085f19..8d16f280b695 100644 --- a/drivers/gpu/drm/amd/amdgpu/Makefile +++ b/drivers/gpu/drm/amd/amdgpu/Makefile @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ -I$(FULL_AMD_PATH)/amdkfd subdir-ccflags-y := -Wextra -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) +subdir-ccflags-y += -Wunused +subdir-ccflags-y += -Wmissing-prototypes +subdir-ccflags-y += -Wmissing-declarations +subdir-ccflags-y += -Wmissing-include-dirs +subdir-ccflags-y += -Wold-style-definition +subdir-ccflags-y += -Wmissing-format-attribute +# Need this to avoid recursive variable evaluation issues +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ + $(call cc-option, -Wunused-const-variable) \ + $(call cc-option, -Wstringop-truncation) \ + $(call cc-option, -Wpacked-not-aligned) +subdir-ccflags-y += $(cond-flags) subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-sign-compare -- 2.40.1
Re: [RFC] Plane color pipeline KMS uAPI
Hi Christopher, On Friday, June 9th, 2023 at 17:52, Christopher Braga wrote: > > The new COLOROP objects also expose a number of KMS properties. Each has a > > type, a reference to the next COLOROP object in the linked list, and other > > type-specific properties. Here is an example for a 1D LUT operation: > > > > Color operation 42 > > ├─ "type": enum {Bypass, 1D curve} = 1D curve > > ├─ "1d_curve_type": enum {LUT, sRGB, PQ, BT.709, HLG, …} = LUT > The options sRGB / PQ / BT.709 / HLG would select hard-coded 1D > curves? Will different hardware be allowed to expose a subset of these > enum values? Yes. Only hardcoded LUTs supported by the HW are exposed as enum entries. > > ├─ "lut_size": immutable range = 4096 > > ├─ "lut_data": blob > > └─ "next": immutable color operation ID = 43 > > > Some hardware has per channel 1D LUT values, while others use the same > LUT for all channels. We will definitely need to expose this in the > UAPI in some form. Hm, I was assuming per-channel 1D LUTs here, just like the existing GAMMA_LUT/ DEGAMMA_LUT properties work. If some hardware can't support that, it'll need to get exposed as another color operation block. > > To configure this hardware block, user-space can fill a KMS blob with > > 4096 u32 > > entries, then set "lut_data" to the blob ID. Other color operation types > > might > > have different properties. > > > The bit-depth of the LUT is an important piece of information we should > include by default. Are we assuming that the DRM driver will always > reduce the input values to the resolution supported by the pipeline? > This could result in differences between the hardware behavior > and the shader behavior. > > Additionally, some pipelines are floating point while others are fixed. > How would user space know if it needs to pack 32 bit integer values vs > 32 bit float values? Again, I'm deferring to the existing GAMMA_LUT/DEGAMMA_LUT. These use a common definition of LUT blob (u16 elements) and it's up to the driver to convert. Using a very precise format for the uAPI has the nice property of making the uAPI much simpler to use. User-space sends high precision data and it's up to drivers to map that to whatever the hardware accepts. Exposing the actual hardware precision is something we've talked about during the hackfest. It'll probably be useful to some extent, but will require some discussion to figure out how to design the uAPI. Maybe a simple property is enough, maybe not (e.g. fully describing the precision of segmented LUTs would probably be trickier). I'd rather keep things simple for the first pass, we can always add more properties for bit depth etc later on. > > Here is another example with a 3D LUT: > > > > Color operation 42 > > ├─ "type": enum {Bypass, 3D LUT} = 3D LUT > > ├─ "lut_size": immutable range = 33 > > ├─ "lut_data": blob > > └─ "next": immutable color operation ID = 43 > > > We are going to need to expose the packing order here to avoid any > programming uncertainty. I don't think we can safely assume all hardware > is equivalent. The driver can easily change the layout of the matrix and do any conversion necessary when programming the hardware. We do need to document what layout is used in the uAPI for sure. > > And one last example with a matrix: > > > > Color operation 42 > > ├─ "type": enum {Bypass, Matrix} = Matrix > > ├─ "matrix_data": blob > > └─ "next": immutable color operation ID = 43 > > > It is unclear to me what the default sizing of this matrix is. Any > objections to exposing these details with an additional property? The existing CTM property uses 9 uint64 (S31.32) values. Is there a case where that wouldn't be enough? > Dithering logic exists in some pipelines. I think we need a plan to > expose that here as well. Hm, I'm not too familiar with dithering. Do you think it would make sense to expose as an additional colorop block? Do you think it would have more consequences on the design? I want to re-iterate that we don't need to ship all features from day 1. We just need to come up with a uAPI design on which new features can be built on. > > [Simon note: an alternative would be to split the color pipeline into > > two, by > > having two plane properties ("color_pipeline_pre_scale" and > > "color_pipeline_post_scale") instead of a single one. This would be > > similar to > > the way we want to split pre-blending and post-blending. This could be less > > expressive for drivers, there may be hardware where there are dependencies > > between the pre- and post-scaling pipeline?] > > > As others have noted, breaking up the pipeline with immutable blocks > makes the most sense to me here. This way we don't have to predict ahead > of time every type of block that maybe affected by pipeline ordering. > Splitting the pipeline into two properties now means future > logical splits would require introduction of further plane properti
Re: [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers
On Wed, Jun 07, 2023 at 02:49:26PM -0700, Douglas Anderson wrote: > 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(+) Acked-by: Rob Herring
[PATCH v2 1/2] of: module: Export of_device_uevent()
The content of of_device_uevent() is currently hardcoded in a driver that can be compiled as a module. Nothing prevents of_device_uevent() to be exported to modules, most of the other helpers in of/device.c actually are. The reason why this helper was not exported is because it has been so far only useful in drivers/base, which is built-in anyway. With the idea of getting rid of the hardcoded implementation of of_device_uevent() in other places in the kernel, let's export it to GPL modules (very much like its cousins in the same file). Signed-off-by: Miquel Raynal --- drivers/of/device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/of/device.c b/drivers/of/device.c index 0f00f1b80708..90131de6d75b 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -312,6 +312,7 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env) } mutex_unlock(&of_mutex); } +EXPORT_SYMBOL_GPL(of_device_uevent); int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env) { -- 2.34.1
[PATCH v2 2/2] gpu: host1x: Stop open-coding of_device_uevent()
There is apparently no reasons to open-code of_device_uevent() besides: - The helper receives a struct device while we want to use the of_node member of the struct device *parent*. - of_device_uevent() could not be called by modules because of a missing EXPORT_SYMBOL*(). In practice, the former point is not very constraining, just calling of_device_uevent(dev->parent, ...) would have made the trick. The latter point is more an observation rather than a real blocking point because nothing prevented of_uevent() (called by the inline function of_device_uevent()) to be exported to modules. In practice, this helper is now exported, so nothing prevent us from using of_device_uevent() anymore. Let's use the core helper directly instead of open-coding it. Cc: Thierry Reding Cc: David Airlie Cc: Daniel Vetter Cc: Mikko Perttunen Cc: Rob Herring Cc: Frank Rowand Signed-off-by: Miquel Raynal --- This patch depends on the changes performed earlier in the series under the drivers/of/ folder. --- drivers/gpu/host1x/bus.c | 29 ++--- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c index 4d16a3396c4a..dae589b83be1 100644 --- a/drivers/gpu/host1x/bus.c +++ b/drivers/gpu/host1x/bus.c @@ -338,32 +338,15 @@ static int host1x_device_match(struct device *dev, struct device_driver *drv) return strcmp(dev_name(dev), drv->name) == 0; } +/* + * Note that this is really only needed for backwards compatibility + * with libdrm, which parses this information from sysfs and will + * fail if it can't find the OF_FULLNAME, specifically. + */ static int host1x_device_uevent(const struct device *dev, struct kobj_uevent_env *env) { - struct device_node *np = dev->parent->of_node; - unsigned int count = 0; - struct property *p; - const char *compat; - - /* -* This duplicates most of of_device_uevent(), but the latter cannot -* be called from modules and operates on dev->of_node, which is not -* available in this case. -* -* Note that this is really only needed for backwards compatibility -* with libdrm, which parses this information from sysfs and will -* fail if it can't find the OF_FULLNAME, specifically. -*/ - add_uevent_var(env, "OF_NAME=%pOFn", np); - add_uevent_var(env, "OF_FULLNAME=%pOF", np); - - of_property_for_each_string(np, "compatible", p, compat) { - add_uevent_var(env, "OF_COMPATIBLE_%u=%s", count, compat); - count++; - } - - add_uevent_var(env, "OF_COMPATIBLE_N=%u", count); + of_device_uevent((const struct device *)&dev->parent, env); return 0; } -- 2.34.1
[PATCH v2 0/2] Small of/device cleanup
My previous attempt to slightly clean the OF core wrt device structures was rather unsuccessful as the idea behind the discussed cleanup was more impacting than what I thought, leading to most of the previous series to be dropped. However, aside, two patches seemed actually relevant, so here they are, alone. Link: https://lore.kernel.org/all/20230608184903.ga3200973-r...@kernel.org/ Thanks, Miquèl Changes in v2: * Dropped all the of_device.h/of_module.h changes * Directly used of_device_uevent() from the host1x driver Miquel Raynal (2): of: module: Export of_device_uevent() gpu: host1x: Stop open-coding of_device_uevent() drivers/gpu/host1x/bus.c | 29 ++--- drivers/of/device.c | 1 + 2 files changed, 7 insertions(+), 23 deletions(-) -- 2.34.1
Re: [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens
On 07/06/2023 23:49, Douglas Anderson wrote: > 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. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [RFC] Plane color pipeline KMS uAPI
Hi all, The goal of this RFC is to expose a generic KMS uAPI to configure the color pipeline before blending, ie. after a pixel is tapped from a plane's framebuffer and before it's blended with other planes. With this new uAPI we aim to reduce the battery life impact of color management and HDR on mobile devices, to improve performance and to decrease latency by skipping composition on the 3D engine. This proposal is the result of discussions at the Red Hat HDR hackfest [1] which took place a few days ago. Engineers familiar with the AMD, Intel and NVIDIA hardware have participated in the discussion. This proposal takes a prescriptive approach instead of a descriptive approach. Drivers describe the available hardware blocks in terms of low-level mathematical operations, then user-space configures each block. We decided against a descriptive approach where user-space would provide a high-level description of the colorspace and other parameters: we want to give more control and flexibility to user-space, e.g. to be able to replicate exactly the color pipeline with shaders and switch between shaders and KMS pipelines seamlessly, and to avoid forcing user-space into a particular color management policy. Thanks for posting this Simon! This overview does a great job of breaking down the proposal. A few questions inline below. We've decided against mirroring the existing CRTC properties DEGAMMA_LUT/CTM/GAMMA_LUT onto KMS planes. Indeed, the color management pipeline can significantly differ between vendors and this approach cannot accurately abstract all hardware. In particular, the availability, ordering and capabilities of hardware blocks is different on each display engine. So, we've decided to go for a highly detailed hardware capability discovery. This new uAPI should not be in conflict with existing standard KMS properties, since there are none which control the pre-blending color pipeline at the moment. It does conflict with any vendor-specific properties like NV_INPUT_COLORSPACE or the patches on the mailing list adding AMD-specific properties. Drivers will need to either reject atomic commits configuring both uAPIs, or alternatively we could add a DRM client cap which hides the vendor properties and shows the new generic properties when enabled. To use this uAPI, first user-space needs to discover hardware capabilities via KMS objects and properties, then user-space can configure the hardware via an atomic commit. This works similarly to the existing KMS uAPI, e.g. planes. Our proposal introduces a new "color_pipeline" plane property, and a new KMS object type, "COLOROP" (short for color operation). The "color_pipeline" plane property is an enum, each enum entry represents a color pipeline supported by the hardware. The special zero entry indicates that the pipeline is in "bypass"/"no-op" mode. For instance, the following plane properties describe a primary plane with 2 supported pipelines but currently configured in bypass mode: Plane 10 ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary ├─ … └─ "color_pipeline": enum {0, 42, 52} = 0 The non-zero entries describe color pipelines as a linked list of COLOROP KMS objects. The entry value is an object ID pointing to the head of the linked list (the first operation in the color pipeline). The new COLOROP objects also expose a number of KMS properties. Each has a type, a reference to the next COLOROP object in the linked list, and other type-specific properties. Here is an example for a 1D LUT operation: Color operation 42 ├─ "type": enum {Bypass, 1D curve} = 1D curve ├─ "1d_curve_type": enum {LUT, sRGB, PQ, BT.709, HLG, …} = LUT The options sRGB / PQ / BT.709 / HLG would select hard-coded 1D curves? Will different hardware be allowed to expose a subset of these enum values? ├─ "lut_size": immutable range = 4096 ├─ "lut_data": blob └─ "next": immutable color operation ID = 43 Some hardware has per channel 1D LUT values, while others use the same LUT for all channels. We will definitely need to expose this in the UAPI in some form. To configure this hardware block, user-space can fill a KMS blob with 4096 u32 entries, then set "lut_data" to the blob ID. Other color operation types might have different properties. The bit-depth of the LUT is an important piece of information we should include by default. Are we assuming that the DRM driver will always reduce the input values to the resolution supported by the pipeline? This could result in differences between the hardware behavior and the shader behavior. Additionally, some pipelines are floating point while others are fixed. How would user space know if it needs to pack 32 bit integer values vs 32 bit float values? Here is another example with a 3D LUT: Color operation 42 ├─ "type": enum {Bypass, 3D LUT} = 3D LUT ├─ "lut_size": immutable range = 33 ├─ "lut_data": bl
Re: [PATCH v4 1/8] dt-bindings: display: mediatek: add MT8195 hdmi bindings
On Thu, 08 Jun 2023 23:05, Rob Herring wrote: >On Mon, May 29, 2023 at 04:30:58PM +0200, Guillaume Ranquet wrote: >> Add mt8195 SoC bindings for hdmi and hdmi-ddc >> >> On mt8195 the ddc i2c controller is part of the hdmi IP block and thus has no >> specific register range, power domain or interrupt, making it simpler >> than the legacy "mediatek,hdmi-ddc" binding. >> >> Signed-off-by: Guillaume Ranquet >> --- >> .../bindings/display/mediatek/mediatek,hdmi.yaml | 59 >> ++ >> .../display/mediatek/mediatek,mt8195-hdmi-ddc.yaml | 45 + >> 2 files changed, 93 insertions(+), 11 deletions(-) >> >> diff --git >> a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml >> b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml >> index b90b6d18a828..4f62e6b94048 100644 >> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml >> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml >> @@ -21,6 +21,7 @@ properties: >>- mediatek,mt7623-hdmi >>- mediatek,mt8167-hdmi >>- mediatek,mt8173-hdmi >> + - mediatek,mt8195-hdmi >> >>reg: >> maxItems: 1 >> @@ -29,18 +30,10 @@ properties: >> maxItems: 1 >> >>clocks: >> -items: >> - - description: Pixel Clock >> - - description: HDMI PLL >> - - description: Bit Clock >> - - description: S/PDIF Clock >> +maxItems: 4 >> >>clock-names: >> -items: >> - - const: pixel >> - - const: pll >> - - const: bclk >> - - const: spdif >> +maxItems: 4 >> >>phys: >> maxItems: 1 >> @@ -58,6 +51,9 @@ properties: >> description: | >>phandle link and register offset to the system configuration >> registers. >> >> + power-domains: >> +maxItems: 1 >> + >>ports: >> $ref: /schemas/graph.yaml#/properties/ports >> >> @@ -86,9 +82,50 @@ required: >>- clock-names >>- phys >>- phy-names >> - - mediatek,syscon-hdmi >>- ports >> >> +allOf: >> + - if: >> + properties: >> +compatible: >> + contains: >> +const: mediatek,mt8195-hdmi >> +then: >> + properties: >> +clocks: >> + items: >> +- description: APB >> +- description: HDCP >> +- description: HDCP 24M >> +- description: Split HDMI >> +clock-names: >> + items: >> +- const: hdmi_apb_sel >> +- const: hdcp_sel >> +- const: hdcp24_sel >> +- const: split_hdmi >> + >> + required: >> +- power-domains >> +else: >> + properties: >> +clocks: >> + items: >> +- description: Pixel Clock >> +- description: HDMI PLL >> +- description: Bit Clock >> +- description: S/PDIF Clock >> + >> +clock-names: >> + items: >> +- const: pixel >> +- const: pll >> +- const: bclk >> +- const: spdif > >I don't understand how the same h/w block can have completely different >clocks. If not the same h/w or evolution of the same h/w, then do a >separate schema. > Hi Rob, I'm not entirely sure what's the best approach here. The IPs are different enough to warrant a separate schema IMHO. Though CK asked me to merge both IPs together (for both schema and code). CK might want to chime in and advocate his point of view? >> + >> + required: >> +- mediatek,syscon-hdmi >> + >> additionalProperties: false >> >> examples: >> diff --git >> a/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml >> >> b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml >> new file mode 100644 >> index ..84c096835b47 >> --- /dev/null >> +++ >> b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml >> @@ -0,0 +1,45 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: >> http://devicetree.org/schemas/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Mediatek HDMI DDC for mt8195 >> + >> +maintainers: >> + - CK Hu >> + - Jitao shi >> + >> +description: | >> + The HDMI DDC i2c controller is used to interface with the HDMI DDC pins. >> + >> +properties: >> + compatible: >> +enum: >> + - mediatek,mt8195-hdmi-ddc >> + >> + clocks: >> +maxItems: 1 >> + >> + mediatek,hdmi: >> +$ref: /schemas/types.yaml#/definitions/phandle >> +description: >> + A phandle to the mt8195 hdmi controller >> + >> +required: >> + - compatible >> + - clocks >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> +#include >> +#include >> +hdmiddc0: i2c { >> + compatible = "mediatek,mt8195-hdmi-ddc"; >> + mediatek,hdmi = <&hdmi0>; >> + clocks = <&clk26m>; >
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On 6/10/22 10:59, Daniel Vetter wrote: On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: Hi all, Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here: - lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-) Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot. - the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers. I think the simplest solution for this is: 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong? - third issue: These special virtual display properties arent documented. Aside from hotspot there's also suggested X/Y and maybe other stuff. I have no idea what suggested X/Y does and what userspace should do with it. I think we need a new section for virtualized drivers which: - documents all the properties involved - documents the new cap for enabling virtual cursor planes - documents some of the key flows that compositors should implement for best experience - documents how exactly the user experience will degrade if compositors pretend it's just a normal kms driver (maybe put that into each of the special flows that a compositor ideally supports) - whatever other comments and gaps I've missed, I'm sure Simon/Pekka/others will chime in once the patch exists. Great bonus would be an igt which demonstrates these flows. With the interactive debug breakpoints to wait for resizing or whatever this should be all neatly possible. -Daniel Hi all, We have been testing the v2 of this patch and it works correctly for us. First we tested with a patched Mutter, the mouse clicks were correct, and behavior was as expected. But I've also added an IGT test as suggested above: https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274 I could post the IGT patch upstream once Zack's patches land. Hope that helps! -Albert There's a bit of fixing oopsies (virtualized drivers really shouldn't have enabled universal planes for their cursors) and debt (all these properties predate the push to document stuff so we need to fix that), but I don't think it's too much. And I think, from reading the threads, that this should cover everything? Anything I've missed? Or got completely wrong? Cheers, Daniel On Fri, Jun 03, 2022 at 02:14:59PM +, Simon Ser wrote: Hi, Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615 It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software. In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). cc wayland-devel and Pekka for user-space feedback. On Thursday, June 2nd, 2022 at 17:42, Zack Rusin wrote: - all userspace code needs to hardcore a list of drivers which require hotspots because there's no way to query from drm "does this driver require hotspot" Can you elaborate? I'm not sure I understand what you mean here. Thanks, Simon -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
> Hi Carl, > besides this, ask a dumb question. How we retrieve the pat_index from a shared resource though dma_buf fd? maybe we need to know whether it could be CPU cached if we want map it. Of course, looks there are no real usage to access it though CPU. Just use it directly without any pat related options ? >>> >>> I am not understanding. Do you want to ask the PAT table to the driver? Are >>> you referring to the CPU PAT index? >>> >>> In any case, if I understood correctly, you don't necessarily always need to >>> set the PAT options and the cache options will fall into the default values. >>> >>> Please let me know if I haven't answered the question. >>> >> >> If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it >> to a dma fd. >> Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to >> a gem bo. >> But media does not know the PAT index , because mesa create it and set it. >> So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not >> know whether it could be WB. > > That's a good point. To be honest I am not really sure how this > is handled. > > Fei, Jordan? Do you have suggestion here? Is it possible to pass the PAT setting when sharing the fd? Or perhaps we should have kept the get_caching ioctl functional? Joonas, could you chime in here? > Andi
[PATCH] drm: atmel-hlcdc: Support inverting the pixel clock polarity
On the SoC host controller, the pixel clock can be: * standard: data is launched on the rising edge * inverted: data is launched on the falling edge Some panels may need the inverted option to be used so let's support this DRM flag. Signed-off-by: Miquel Raynal --- Hello, this change was tested on a Sama5d36 based custom board with a panel not behaving correctly if the pixel clock was not inverted. Cheers, Miquèl .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 25 +++ 1 file changed, 25 insertions(+) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 58184cd6ab0b..cc5cf4c2faf7 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -68,7 +68,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c); struct regmap *regmap = crtc->dc->hlcdc->regmap; struct drm_display_mode *adj = &c->state->adjusted_mode; + struct drm_encoder *encoder = NULL, *en_iter; + struct drm_connector *connector = NULL; struct atmel_hlcdc_crtc_state *state; + struct drm_device *ddev = c->dev; + struct drm_connector_list_iter iter; unsigned long mode_rate; struct videomode vm; unsigned long prate; @@ -76,6 +80,23 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) unsigned int cfg = 0; int div, ret; + /* get encoder from crtc */ + drm_for_each_encoder(en_iter, ddev) { + if (en_iter->crtc == c) { + encoder = en_iter; + break; + } + } + + if (encoder) { + /* Get the connector from encoder */ + drm_connector_list_iter_begin(ddev, &iter); + drm_for_each_connector_iter(connector, &iter) + if (connector->encoder == encoder) + break; + drm_connector_list_iter_end(&iter); + } + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); if (ret) return; @@ -134,6 +155,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) cfg |= ATMEL_HLCDC_CLKDIV(div); + if (connector && + connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) + cfg |= ATMEL_HLCDC_CLKPOL; + regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0), mask, cfg); state = drm_crtc_state_to_atmel_hlcdc_crtc_state(c->state); -- 2.34.1
Re: [RESEND 11/15] drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef
On 6/9/23 04:17, Lee Jones wrote: Fixes the following W=1 kernel build warning(s): drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:48:22: warning: ‘SYNAPTICS_DEVICE_ID’ defined but not used [-Wunused-const-variable=] Cc: Harry Wentland Cc: Leo Li Cc: Rodrigo Siqueira Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: David Airlie Cc: Daniel Vetter Cc: amd-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Lee Jones Applied, thanks! --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index 09e056a647087..cd20cfc049969 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -44,9 +44,6 @@ #include "dm_helpers.h" #include "ddc_service_types.h" -/* MST Dock */ -static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA"; - /* dm_helpers_parse_edid_caps * * Parse edid caps @@ -702,6 +699,9 @@ static void apply_synaptics_fifo_reset_wa(struct drm_dp_aux *aux) DC_LOG_DC("Done apply_synaptics_fifo_reset_wa\n"); } +/* MST Dock */ +static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA"; + static uint8_t write_dsc_enable_synaptics_non_virtual_dpcd_mst( struct drm_dp_aux *aux, const struct dc_stream_state *stream, -- Hamza
Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
Hi, On Fri, Jun 9, 2023 at 2:27 AM Benjamin Tissoires wrote: > > > I suspect that it's not worth it, but I'll do this if you feel > > strongly about it. > > > > I guess the simplest way I can think of to move this to its own file > > would be to put the whole private data structure (struct i2c_hid) in a > > private header file and then add prototypes for i2c_hid_core_resume() > > and i2c_hid_core_suspend() there. Then I could add something like > > i2c_hid_core_handle_panel_follower() that would have all the > > registration logic. I'd still need special cases in the core > > suspend/resume/remove code unless I add a level of abstraction. While > > the level of abstraction is more "pure", it also would make the code > > harder to follow. > > > > Unless I hear a strong opinion (or if this series changes > > significantly), I'll plan to keep things in the same file and just use > > a Kconfig. > > > > Right, a separate file might not be the best then :( > > Do you envision this to be used on the ACPI side of i2c-hid? Because > if this is OF only, then maybe it would be interesting to put it there > (in i2c-hid-of.c), instead of having it in the core. IIRC i2c-hid-of > also has ways to set up/down regulators, so maybe it'll be better > there? There is no reason why this problem would be limited to devices using devicetree. Even if ACPI could somehow magically power sequence the touchscreen and panel together, doing it behind the back of the kernel driver would be a bad idea anyway so folks using ACPI would need the same code. I don't have tons of experience with ACPI nor how to hook this up there, but I purposely made the API for registering the panel follower such that the client doesn't pass anything devicetree specific. If someone could figure out how to detect a link between a panel and a touchscreen for ACPI and add this code to drm_panel_add_follower() then it would automatically work for the ACPI case as well. -Doug
[PATCH 7/7] drm/panel: sitronix-st7789v: Check display ID
A very basic debugging rule when a device is connected for the first time is to access a read-only register which contains known data in order to ensure the communication protocol is properly working. This driver lacked any read helper which is often a critical peace for fastening bring-ups. Add a read helper and use it to verify the communication with the panel is working as soon as possible in order to fail early if this is not the case. Signed-off-by: Miquel Raynal --- .../gpu/drm/panel/panel-sitronix-st7789v.c| 78 +++ 1 file changed, 78 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index 7de192a3a8aa..fb21d52a7a63 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -113,6 +113,9 @@ return val; \ } while (0) +#define ST7789V_IDS { 0x85, 0x85, 0x52 } +#define ST7789V_IDS_SIZE 3 + struct st7789v_panel_info { const struct drm_display_mode *display_mode; u16 width_mm; @@ -165,6 +168,77 @@ static int st7789v_write_data(struct st7789v *ctx, u8 cmd) return st7789v_spi_write(ctx, ST7789V_DATA, cmd); } +static int st7789v_read_data(struct st7789v *ctx, u8 cmd, u8 *buf, +unsigned int len) +{ + struct spi_transfer xfer[2] = { }; + struct spi_message msg; + u16 txbuf = ((ST7789V_COMMAND & 1) << 8) | cmd; + u16 rxbuf[4] = {}; + u8 bit9 = 0; + int ret, i; + + switch (len) { + case 1: + case 3: + case 4: + break; + default: + return -EOPNOTSUPP; + } + + spi_message_init(&msg); + + xfer[0].tx_buf = &txbuf; + xfer[0].len = sizeof(txbuf); + spi_message_add_tail(&xfer[0], &msg); + + xfer[1].rx_buf = rxbuf; + xfer[1].len = len * 2; + spi_message_add_tail(&xfer[1], &msg); + + ret = spi_sync(ctx->spi, &msg); + if (ret) + return ret; + + for (i = 0; i < len; i++) { + buf[i] = rxbuf[i] >> i | (bit9 << (9 - i)); + if (i) + bit9 = rxbuf[i] & GENMASK(i - 1, 0); + } + + return 0; +} + +static int st7789v_check_id(struct drm_panel *panel) +{ + const u8 st7789v_ids[ST7789V_IDS_SIZE] = ST7789V_IDS; + struct st7789v *ctx = panel_to_st7789v(panel); + bool invalid_ids = false; + int ret, i; + u8 ids[3]; + + ret = st7789v_read_data(ctx, MIPI_DCS_GET_DISPLAY_ID, ids, ST7789V_IDS_SIZE); + if (ret) { + dev_err(panel->dev, "Failed to read IDs\n"); + return ret; + } + + for (i = 0; i < ST7789V_IDS_SIZE; i++) { + if (ids[i] != st7789v_ids[i]) { + invalid_ids = true; + break; + } + } + + if (invalid_ids) { + dev_err(panel->dev, "Unrecognized panel IDs"); + return -EIO; + } + + return 0; +} + static const struct drm_display_mode default_mode = { .clock = 7000, .hdisplay = 240, @@ -237,6 +311,10 @@ static int st7789v_prepare(struct drm_panel *panel) gpiod_set_value(ctx->reset, 0); msleep(120); + ret = st7789v_check_id(panel); + if (ret) + return ret; + ST7789V_TEST(ret, st7789v_write_command(ctx, MIPI_DCS_EXIT_SLEEP_MODE)); /* We need to wait 120ms after a sleep out command */ -- 2.34.1
[PATCH 2/7] drm/panel: sitronix-st7789v: Use 9 bits per spi word by default
The Sitronix controller expects 9-bit words, provide this as default at probe time rather than specifying this in each and every access. Signed-off-by: Miquel Raynal --- drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index c67b9adb157f..e9ca7ebb458a 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -138,7 +138,6 @@ static int st7789v_spi_write(struct st7789v *ctx, enum st7789v_prefix prefix, spi_message_init(&msg); xfer.tx_buf = &txbuf; - xfer.bits_per_word = 9; xfer.len = sizeof(txbuf); spi_message_add_tail(&xfer, &msg); @@ -365,6 +364,13 @@ static int st7789v_probe(struct spi_device *spi) spi_set_drvdata(spi, ctx); ctx->spi = spi; + spi->bits_per_word = 9; + ret = spi_setup(spi); + if (ret < 0) { + dev_err(&spi->dev, "spi_setup failed: %d\n", ret); + return ret; + } + drm_panel_init(&ctx->panel, &spi->dev, &st7789v_drm_funcs, DRM_MODE_CONNECTOR_DPI); -- 2.34.1
[PATCH 3/7] drm/panel: sitronix-st7789v: Specify the expected bus format
The LCD controller supports RGB444, RGB565 and RGB888. The value that is written in the COLMOD register indicates using RGB888, so let's clearly specify the in-use bus format. Signed-off-by: Miquel Raynal --- drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index e9ca7ebb458a..0abb45bea18d 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -170,6 +171,7 @@ static int st7789v_get_modes(struct drm_panel *panel, struct drm_connector *connector) { struct drm_display_mode *mode; + u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18; mode = drm_mode_duplicate(connector->dev, &default_mode); if (!mode) { @@ -186,6 +188,8 @@ static int st7789v_get_modes(struct drm_panel *panel, connector->display_info.width_mm = 61; connector->display_info.height_mm = 103; + drm_display_info_set_bus_formats(&connector->display_info, +&bus_format, 1); return 1; } -- 2.34.1
[PATCH 5/7] dt-bindings: display: st7789v: Add the edt, et028013dma panel compatible
The ST7789V LCD controller is also embedded in the ET028013DMA panel. In fact, "sitronix,st7789v" might not be totally relevant alone as most of the time -if not all- the LCD controller will always be packaged into a display with its own physical properties. Let's keep "sitronix,st7789v" valid alone for backward compatibility, but we should definitely provide two compatibles to fully describe such panel, so let's expect to have both when describing a panel such as the EDT ET028013DMA. Signed-off-by: Miquel Raynal --- .../bindings/display/panel/sitronix,st7789v.yaml | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml index d984b59daa4a..d4c8af9a973d 100644 --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml @@ -15,7 +15,12 @@ allOf: properties: compatible: -const: sitronix,st7789v +oneOf: + - items: + - enum: + - edt,et028013dma + - const: sitronix,st7789v + - const: sitronix,st7789v reg: true reset-gpios: true -- 2.34.1
[PATCH 6/7] drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support
This panel from Emerging Display Technologies Corporation features an ST7789V2 panel inside which is almost identical to what the Sitronix panel driver supports. In practice, the module physical size is specific, and experiments show that the display will malfunction if any of the following situation occurs: * Pixel clock is above 3MHz * Pixel clock is not inverted I could not properly identify the reasons behind these failures, scope captures show valid input signals. Signed-off-by: Miquel Raynal --- .../gpu/drm/panel/panel-sitronix-st7789v.c| 34 +-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index 212bccc31804..7de192a3a8aa 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -30,7 +30,8 @@ #define ST7789V_RGBCTRL_RCM(n) (((n) & 3) << 5) #define ST7789V_RGBCTRL_VSYNC_HIGH BIT(3) #define ST7789V_RGBCTRL_HSYNC_HIGH BIT(2) -#define ST7789V_RGBCTRL_PCLK_HIGH BIT(1) +#define ST7789V_RGBCTRL_PCLK_FALLING BIT(1) +#define ST7789V_RGBCTRL_PCLK_RISING0 #define ST7789V_RGBCTRL_VBP(n) ((n) & 0x7f) #define ST7789V_RGBCTRL_HBP(n) ((n) & 0x1f) @@ -117,6 +118,7 @@ struct st7789v_panel_info { u16 width_mm; u16 height_mm; u32 bus_format; + u32 bus_flags; }; struct st7789v { @@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = { .vtotal = 320 + 8 + 4 + 4, }; +static const struct drm_display_mode slow_mode = { + .clock = 3000, + .hdisplay = 240, + .hsync_start = 240 + 38, + .hsync_end = 240 + 38 + 10, + .htotal = 240 + 38 + 10 + 10, + .vdisplay = 320, + .vsync_start = 320 + 8, + .vsync_end = 320 + 8 + 4, + .vtotal = 320 + 8 + 4 + 4, +}; + static int st7789v_get_modes(struct drm_panel *panel, struct drm_connector *connector) { @@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel, connector->display_info.width_mm = panel_info->width_mm; connector->display_info.height_mm = panel_info->height_mm; + connector->display_info.bus_flags = panel_info->bus_flags; drm_display_info_set_bus_formats(&connector->display_info, &panel_info->bus_format, 1); @@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel, static int st7789v_prepare(struct drm_panel *panel) { struct st7789v *ctx = panel_to_st7789v(panel); + const struct st7789v_panel_info *panel_info = ctx->panel_info; + u8 pck = ST7789V_RGBCTRL_PCLK_FALLING; int ret; + if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE) + pck = ST7789V_RGBCTRL_PCLK_RISING; + ret = regulator_enable(ctx->power); if (ret) return ret; @@ -321,7 +341,7 @@ static int st7789v_prepare(struct drm_panel *panel) ST7789V_RGBCTRL_RCM(2) | ST7789V_RGBCTRL_VSYNC_HIGH | ST7789V_RGBCTRL_HSYNC_HIGH | -ST7789V_RGBCTRL_PCLK_HIGH)); +pck)); ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8))); ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(20))); @@ -422,14 +442,24 @@ static const struct st7789v_panel_info st7789v_info = { .bus_format = MEDIA_BUS_FMT_RGB666_1X18, }; +static const struct st7789v_panel_info et028013dma_info = { + .display_mode = &slow_mode, + .width_mm = 43, + .height_mm = 58, + .bus_format = MEDIA_BUS_FMT_RGB666_1X18, + .bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, +}; + static const struct of_device_id st7789v_of_match[] = { { .compatible = "sitronix,st7789v", .data = &st7789v_info }, + { .compatible = "edt,et028013dma", .data = &et028013dma_info }, { } }; MODULE_DEVICE_TABLE(of, st7789v_of_match); static const struct spi_device_id st7789v_ids[] = { { "st7789v", }, + { "et028013dma", }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(spi, st7789v_ids); -- 2.34.1
[PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings
The spi core warns us about using an of_device_id table without a spi_device_id table aside for module utilities in orter to not rely on OF modaliases. Just add this table using the device name without the vendor prefix (as it is usually done). Signed-off-by: Miquel Raynal --- drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index bbc4569cbcdc..c67b9adb157f 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -400,9 +400,16 @@ static const struct of_device_id st7789v_of_match[] = { }; MODULE_DEVICE_TABLE(of, st7789v_of_match); +static const struct spi_device_id st7789v_ids[] = { + { "st7789v", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(spi, st7789v_ids); + static struct spi_driver st7789v_driver = { .probe = st7789v_probe, .remove = st7789v_remove, + .id_table = st7789v_ids, .driver = { .name = "st7789v", .of_match_table = st7789v_of_match, -- 2.34.1
[PATCH 4/7] drm/panel: sitronix-st7789v: Use platform data
The Sitronix ST7789V LCD controller is actually packaged in a number of different panels with slightly different properties. Before introducing the support for another pannel using this same LCD controller, let's move all the panel-specific information into a dedicated structure that is available as platform data. There is no functional change. Signed-off-by: Miquel Raynal --- .../gpu/drm/panel/panel-sitronix-st7789v.c| 30 +++ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index 0abb45bea18d..212bccc31804 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -112,11 +112,19 @@ return val; \ } while (0) +struct st7789v_panel_info { + const struct drm_display_mode *display_mode; + u16 width_mm; + u16 height_mm; + u32 bus_format; +}; + struct st7789v { struct drm_panel panel; struct spi_device *spi; struct gpio_desc *reset; struct regulator *power; + const struct st7789v_panel_info *panel_info; }; enum st7789v_prefix { @@ -170,10 +178,11 @@ static const struct drm_display_mode default_mode = { static int st7789v_get_modes(struct drm_panel *panel, struct drm_connector *connector) { + struct st7789v *ctx = panel_to_st7789v(panel); + const struct st7789v_panel_info *panel_info = ctx->panel_info; struct drm_display_mode *mode; - u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18; - mode = drm_mode_duplicate(connector->dev, &default_mode); + mode = drm_mode_duplicate(connector->dev, panel_info->display_mode); if (!mode) { dev_err(panel->dev, "failed to add mode %ux%ux@%u\n", default_mode.hdisplay, default_mode.vdisplay, @@ -186,10 +195,10 @@ static int st7789v_get_modes(struct drm_panel *panel, mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); - connector->display_info.width_mm = 61; - connector->display_info.height_mm = 103; + connector->display_info.width_mm = panel_info->width_mm; + connector->display_info.height_mm = panel_info->height_mm; drm_display_info_set_bus_formats(&connector->display_info, -&bus_format, 1); +&panel_info->bus_format, 1); return 1; } @@ -365,6 +374,8 @@ static int st7789v_probe(struct spi_device *spi) if (!ctx) return -ENOMEM; + ctx->panel_info = device_get_match_data(&spi->dev); + spi_set_drvdata(spi, ctx); ctx->spi = spi; @@ -404,8 +415,15 @@ static void st7789v_remove(struct spi_device *spi) drm_panel_remove(&ctx->panel); } +static const struct st7789v_panel_info st7789v_info = { + .display_mode = &default_mode, + .width_mm = 64, + .height_mm = 103, + .bus_format = MEDIA_BUS_FMT_RGB666_1X18, +}; + static const struct of_device_id st7789v_of_match[] = { - { .compatible = "sitronix,st7789v" }, + { .compatible = "sitronix,st7789v", .data = &st7789v_info }, { } }; MODULE_DEVICE_TABLE(of, st7789v_of_match); -- 2.34.1
[PATCH 0/7] drm/panel: sitronix-st7789v: Support ET028013DMA panel
Hello, The aim of this series is to add support for the EDT ET028013DMA panel. This panel features a Sitronix ST7789V2 LCD controller, which is already supported mainline (or very close to the ST7789V for which Maxime added support years ago). The EDT panel is slightly different on the geometry and appears not to support refresh rates higher than 30fps (above, glitches are visible, despite the incoming signals being rather clean). While I was working on this panel, I found quite inconvenient to not be able to read anything back as it is a great tool for debugging purposes. So the last patch actually adds a read helper and uses it to perform a sanity check at probe time by verifying the Sitronix controller IDs. If deemed irrelevant, this patch may be discarded. Thanks, Miquèl Miquel Raynal (7): drm/panel: sitronix-st7789v: Prevent core spi warnings drm/panel: sitronix-st7789v: Use 9 bits per spi word by default drm/panel: sitronix-st7789v: Specify the expected bus format drm/panel: sitronix-st7789v: Use platform data dt-bindings: display: st7789v: Add the edt,et028013dma panel compatible drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support drm/panel: sitronix-st7789v: Check display ID .../display/panel/sitronix,st7789v.yaml | 7 +- .../gpu/drm/panel/panel-sitronix-st7789v.c| 157 +- 2 files changed, 156 insertions(+), 8 deletions(-) -- 2.34.1
Re: [RESEND 11/15] drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef
On 6/9/23 04:17, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:48:22: > warning: ‘SYNAPTICS_DEVICE_ID’ defined but not used [-Wunused-const-variable=] > > Cc: Harry Wentland > Cc: Leo Li > Cc: Rodrigo Siqueira > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: David Airlie > Cc: Daniel Vetter > Cc: amd-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Lee Jones Reviewed-by: Harry Wentland Harry > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index 09e056a647087..cd20cfc049969 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > @@ -44,9 +44,6 @@ > #include "dm_helpers.h" > #include "ddc_service_types.h" > > -/* MST Dock */ > -static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA"; > - > /* dm_helpers_parse_edid_caps > * > * Parse edid caps > @@ -702,6 +699,9 @@ static void apply_synaptics_fifo_reset_wa(struct > drm_dp_aux *aux) > DC_LOG_DC("Done apply_synaptics_fifo_reset_wa\n"); > } > > +/* MST Dock */ > +static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA"; > + > static uint8_t write_dsc_enable_synaptics_non_virtual_dpcd_mst( > struct drm_dp_aux *aux, > const struct dc_stream_state *stream,
Re: Adreno devfreq lockdep splat with 6.3-rc2
On Thu, Jun 8, 2023 at 11:17 PM Johan Hovold wrote: > > On Thu, Jun 08, 2023 at 02:17:45PM -0700, Rob Clark wrote: > > On Thu, Jun 8, 2023 at 7:12 AM Johan Hovold wrote: > > > > Have you had a chance to look at this regression yet? It prevents us > > > from using lockdep on the X13s as it is disabled as soon as we start > > > the GPU. > > > > Hmm, curious what is different between x13s and sc7180/sc7280 things? > > It seems like lockdep needs to hit the tear down path in order to > detect the circular lock dependency. Perhaps you don't hit that on your > sc7180/sc7280? > > It is due to the fact that the panel is looked up way too late so that > bind fails unless the panel driver is already loaded when the msm drm > driver probes. Oh, this seems likely > Manually loading the panel driver before msm makes the splat go away. > > > Or did lockdep recently get more clever (or more annotation)? > > I think this is indeed a new problem related to some of the devfreq work > you did in 6.3-rc1 (e.g. fadcc3ab1302 ("drm/msm/gpu: Bypass PM QoS > constraint for idle clamp")). > > > I did spend some time a while back trying to bring some sense to > > devfreq/pm-qos/icc locking: > > https://patchwork.freedesktop.org/series/115028/ > > > > but haven't had time to revisit that for a while > > That's the series I link to below, but IIRC it did not look directly > applicable to the splat I see on X13s (e.g. does not involve > fs_reclaim). Ahh, right, sorry I've not had time to do more than glance at the thread.. and yeah, that one is mostly just trying to solve the reclaim problem by moving allocations out from under the big-pm-qos-lock. As far as fadcc3ab1302 ("drm/msm/gpu: Bypass PM QoS constraint for idle clamp"), it should be just taking the lock that dev_pm_qos_update_request() would have indirectly, although I guess without some intervening lock? We can't really avoid taking the devfreq lock, I don't think. But I'd have to spend time I don't have right now digging into it.. BR, -R > > > On Wed, Mar 15, 2023 at 10:19:21AM +0100, Johan Hovold wrote: > > > > > > > > Since 6.3-rc2 (or possibly -rc1), I'm now seeing the below > > > > devfreq-related lockdep splat. > > > > > > > > I noticed that you posted a fix for something similar here: > > > > > > > > > > > > https://lore.kernel.org/r/20230312204150.1353517-9-robdcl...@gmail.com > > > > > > > > but that particular patch makes no difference. > > > > > > > > From skimming the calltraces below and qos/devfreq related changes in > > > > 6.3-rc1 it seems like this could be related to: > > > > > > > > fadcc3ab1302 ("drm/msm/gpu: Bypass PM QoS constraint for idle > > > > clamp") > > Johan
Re: [PATCH v8 2/2] soc: mediatek: remove DDP_DOMPONENT_DITHER from enum
On 31/05/2023 09:43, Chen-Yu Tsai wrote: Hi Matthias, On Mon, Mar 6, 2023 at 4:07 PM Jason-JH.Lin wrote: After mmsys and drm change DITHER enum to DDP_COMPONENT_DITHER0, mmsys header can remove the useless DDP_COMPONENT_DITHER enum. Signed-off-by: Jason-JH.Lin Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Rex-BC Chen Acked-by: Matthias Brugger CK didn't pick up this patch. Since the other patch already got picked up in v6.4-rc1, could you merge this for v6.5? Yes, I gave an acked-by as I thought that CK will take both of them. Anyway applied now. Matthias Thanks ChenYu --- include/linux/soc/mediatek/mtk-mmsys.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/linux/soc/mediatek/mtk-mmsys.h b/include/linux/soc/mediatek/mtk-mmsys.h index dc2963a0a0f7..8eb5846985b4 100644 --- a/include/linux/soc/mediatek/mtk-mmsys.h +++ b/include/linux/soc/mediatek/mtk-mmsys.h @@ -27,8 +27,7 @@ enum mtk_ddp_comp_id { DDP_COMPONENT_CCORR, DDP_COMPONENT_COLOR0, DDP_COMPONENT_COLOR1, - DDP_COMPONENT_DITHER, - DDP_COMPONENT_DITHER0 = DDP_COMPONENT_DITHER, + DDP_COMPONENT_DITHER0, DDP_COMPONENT_DITHER1, DDP_COMPONENT_DP_INTF0, DDP_COMPONENT_DP_INTF1, -- 2.18.0
Re: [RESEND 05/15] drm/mediatek/mtk_disp_ccorr: Remove half completed incorrect struct header
On 09/06/2023 10:17, Lee Jones wrote: Fixes the following W=1 kernel build warning(s): drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or member 'clk' not described in 'mtk_disp_ccorr' drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or member 'regs' not described in 'mtk_disp_ccorr' drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or member 'cmdq_reg' not described in 'mtk_disp_ccorr' drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or member 'data' not described in 'mtk_disp_ccorr' Cc: Chun-Kuang Hu Cc: Philipp Zabel Cc: David Airlie Cc: Daniel Vetter Cc: Matthias Brugger Cc: AngeloGioacchino Del Regno Cc: dri-devel@lists.freedesktop.org Cc: linux-media...@lists.infradead.org Cc: linux-arm-ker...@lists.infradead.org Signed-off-by: Lee Jones --- drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c index 1773379b24398..720f3c7ef7b4f 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c @@ -34,11 +34,6 @@ struct mtk_disp_ccorr_data { u32 matrix_bits; }; -/** - * struct mtk_disp_ccorr - DISP_CCORR driver structure - * @ddp_comp - structure containing type enum and hardware resources - * @crtc - associated crtc to report irq events to - */ That surely suppress the warning but I think the correct to do here is to fix the documentation instead of deleting it. Regards, Matthias struct mtk_disp_ccorr { struct clk *clk; void __iomem *regs;
Re: [RFC PATCH] drm/sched: Wait for the currently popped dependency in kill_jobs_cb()
Am 09.06.23 um 16:10 schrieb Boris Brezillon: Hello Christian, On Fri, 9 Jun 2023 13:53:59 +0200 Christian König wrote: Am 08.06.23 um 08:55 schrieb Boris Brezillon: If I understand correctly, drm_sched_entity_kill_jobs_cb() is supposed to wait on all the external dependencies (those added to drm_sched_job::dependencies) before signaling the job finished fence. This is done this way to prevent jobs depending on these canceled jobs from considering the resources they want to access as ready, when they're actually still used by other components, thus leading to potential data corruptions. The problem is, the kill_jobs logic is omitting the last fence popped from the dependencies array that was waited upon before drm_sched_entity_kill() was called (drm_sched_entity::dependency field), so we're basically waiting for all dependencies except one. This is an attempt at fixing that, but also an opportunity to make sure I understood the drm_sched_entity_kill(), because I'm not 100% sure if skipping this currently popped dependency was intentional or not. I can't see a good reason why we'd want to skip that one, but I might be missing something. Signed-off-by: Boris Brezillon Cc: Frank Binns Cc: Sarah Walker Cc: Donald Robson Cc: Luben Tuikov Cc: David Airlie Cc: Daniel Vetter Cc: Sumit Semwal Cc: "Christian König" --- Stumbled across this issue while working on native dependency support with Donald (which will be posted soon). Flagged as RFC, because I'm not sure this is legit, and also not sure we want to fix it this way. I tried re-using drm_sched_entity::dependency, but it's a bit of a mess because of the asynchronousity of the wait, and the fact we use drm_sched_entity::dependency to know if we have a clear_dep() callback registered, so we can easily reset it without removing the callback. Well yes, that's a known problem. But this is really not the right approach to fixing this. Trying to wait for all the dependencies before killing jobs was added because of the way we kept track of dma_fences in dma_resv objects. Basically adding exclusive fences removed all other fences leading to a bit fragile memory management. Okay. This handling was removed by now and so the workaround for waiting for dependencies is not really necessary any more, but I think it is still better to do so. The right approach of getting this waiting for dependencies completely straight is also not to touch entity->dependency in any way, but to stop removing them from the XA in drm_sched_job_dependency(). Otherwise you don't catch the pipeline optimized ones either. Do you want me to post a v2 doing that, or should I forget about it? If we decide to keep things like that, it might be good to at least add a comment explaining why we don't care. Well, if you have time fixing this fully and keeping the dependency handling is certainly be preferred. Regards, Christian. Regards, Boris
Re: [PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj
On 09/06/2023 13:44, Iddamsetty, Aravind wrote: On 09-06-2023 17:41, Tvrtko Ursulin wrote: From: Tvrtko Ursulin I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which will return a reference to a newly created GEM objects (if created), in order to enable tracking of imported i915 GEM objects in the following patch. instead of this what if we implement the open call back in i915 struct drm_gem_object_funcs { /** * @open: * * Called upon GEM handle creation. * * This callback is optional. */ int (*open)(struct drm_gem_object *obj, struct drm_file *file); which gets called whenever a handle(drm_gem_handle_create_tail) is created and in the open we can check if to_intel_bo(obj)->base.dma_buf then it is imported if not it is owned or created by it. I wanted to track as much memory usage as we have which is buffer object backed, including page tables and contexts. And since those are not user visible (they don't have handles), they wouldn't be covered by the obj->funcs->open() callback. If we wanted to limit to objects with handles we could simply do what Rob proposed and that is to walk the handles idr. But that does not feel like the right direction to me. Open for discussion I guess. Regards, Tvrtko
Re: [RFC PATCH] drm/sched: Wait for the currently popped dependency in kill_jobs_cb()
Hello Christian, On Fri, 9 Jun 2023 13:53:59 +0200 Christian König wrote: > Am 08.06.23 um 08:55 schrieb Boris Brezillon: > > If I understand correctly, drm_sched_entity_kill_jobs_cb() is supposed > > to wait on all the external dependencies (those added to > > drm_sched_job::dependencies) before signaling the job finished fence. > > This is done this way to prevent jobs depending on these canceled jobs > > from considering the resources they want to access as ready, when > > they're actually still used by other components, thus leading to > > potential data corruptions. > > > > The problem is, the kill_jobs logic is omitting the last fence popped > > from the dependencies array that was waited upon before > > drm_sched_entity_kill() was called (drm_sched_entity::dependency field), > > so we're basically waiting for all dependencies except one. > > > > This is an attempt at fixing that, but also an opportunity to make sure > > I understood the drm_sched_entity_kill(), because I'm not 100% sure if > > skipping this currently popped dependency was intentional or not. I can't > > see a good reason why we'd want to skip that one, but I might be missing > > something. > > > > Signed-off-by: Boris Brezillon > > Cc: Frank Binns > > Cc: Sarah Walker > > Cc: Donald Robson > > Cc: Luben Tuikov > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Sumit Semwal > > Cc: "Christian König" > > --- > > Stumbled across this issue while working on native dependency support > > with Donald (which will be posted soon). Flagged as RFC, because I'm > > not sure this is legit, and also not sure we want to fix it this way. > > I tried re-using drm_sched_entity::dependency, but it's a bit of a mess > > because of the asynchronousity of the wait, and the fact we use > > drm_sched_entity::dependency to know if we have a clear_dep() > > callback registered, so we can easily reset it without removing the > > callback. > > Well yes, that's a known problem. But this is really not the right > approach to fixing this. > > Trying to wait for all the dependencies before killing jobs was added > because of the way we kept track of dma_fences in dma_resv objects. > Basically adding exclusive fences removed all other fences leading to a > bit fragile memory management. Okay. > > This handling was removed by now and so the workaround for waiting for > dependencies is not really necessary any more, but I think it is still > better to do so. > > The right approach of getting this waiting for dependencies completely > straight is also not to touch entity->dependency in any way, but to stop > removing them from the XA in drm_sched_job_dependency(). Otherwise you > don't catch the pipeline optimized ones either. Do you want me to post a v2 doing that, or should I forget about it? If we decide to keep things like that, it might be good to at least add a comment explaining why we don't care. Regards, Boris
[PATCH] dt-bindings: gpu: drop unneeded quotes
Cleanup bindings dropping unneeded quotes. Once all these are fixed, checking for this can be enabled in yamllint. Signed-off-by: Krzysztof Kozlowski --- Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 2 +- Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 0400a361875d..e796a1ff8c82 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -86,7 +86,7 @@ properties: const: 2 dynamic-power-coefficient: -$ref: '/schemas/types.yaml#/definitions/uint32' +$ref: /schemas/types.yaml#/definitions/uint32 description: A u32 value that represents the running time dynamic power coefficient in units of uW/MHz/V^2. The diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml index 2a25384ca3ef..ca02baba5526 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml @@ -92,7 +92,7 @@ properties: dma-coherent: true dynamic-power-coefficient: -$ref: '/schemas/types.yaml#/definitions/uint32' +$ref: /schemas/types.yaml#/definitions/uint32 description: A u32 value that represents the running time dynamic power coefficient in units of uW/MHz/V^2. The -- 2.34.1
Re: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled
On 09/06/2023 13:52, Christian König wrote: Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin: On 09/06/2023 07:32, Christian König wrote: Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin: From: Tvrtko Ursulin For dma_fence_is_signaled signaling critical path annotations are an annoying cause of false positives when using dma_fence_is_signaled and indirectly higher level helpers such as dma_resv_test_signaled etc. Drop the critical path annotation since the "is signaled" API does not guarantee to ever change the signaled status anyway. We do that by adding a low level _dma_fence_signal helper and use it from dma_fence_is_signaled. I have been considering dropping the signaling from the dma_fence_is_signaled() function altogether. Doing this together with the spin locking we have in the dma_fence is just utterly nonsense since the purpose of the external spinlock is to keep the signaling in order while this here defeats that. The quick check is ok I think, but signaling the dma_fence and issuing the callbacks should always come from the interrupt handler. What do you think is broken with regards to ordering with the current code? The unlocked fast check? Well it's not broken, the complexity just doesn't make sense. The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t itself. That was done to make sure that all dma_fence objects from a single context (or in other words hardware device) signal in the order of their sequence number, e.g. 1 first, then 2, then 3 etc... But when somebody uses the dma_fence_is_signaled() function it's perfectly possible that this races with an interrupt handler which wants to signal fences on another CPU. In other words we get: CPU A: dma_fence_is_signaled(fence with seqno=3) fence->ops->signaled() returns true dma_fence_signal() spin_lock_irqsave(fence->lock) signal seqno=3 ... CPU B: device_driver_interrupt_handler() spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete signal seqno=1 signal seqno=2 seqno=3 is already signaled. signal seqno=4 ... Right I see. However hm.. would the order of be guaranteed anyway, if someone would be observing what CPU B is doing via the dma_fence_is_signaled->test_bit? And in which scenarios would it matter if out of order signaled status could be observed? Either fence->lock should not be a pointer or we should not signal the fence from dma_fence_is_signaled(). I strongly think that later should be the way to go. Despite having wrote the above, I don't have any objections to removing this either. I don't see anything in the contract that requires it, but it was probably a bit before my time to know why it was added so I don't know if it could cause any subtle issues. It should be okay to try and see. Regards, Tvrtko
[PATCH] drm/radeon: Disable outputs when releasing fbdev client
Disable the modesetting pipeline before release the radeon's fbdev client. Fixes the following error: [ 17.217408] WARNING: CPU: 5 PID: 1464 at drivers/gpu/drm/ttm/ttm_bo.c:326 ttm_bo_release+0x27e/0x2d0 [ttm] [ 17.217418] Modules linked in: edac_mce_amd radeon(+) drm_ttm_helper ttm video drm_suballoc_helper drm_display_helper kvm irqbypass drm_kms_helper syscopyarea crc32_pclmul sysfillrect sha512_ssse3 sysimgblt sha512_generic cfbfillrect cfbimgblt wmi_bmof aesni_intel cfbcopyarea crypto_simd cryptd k10temp acpi_cpufreq wmi dm_mod [ 17.217432] CPU: 5 PID: 1464 Comm: systemd-udevd Not tainted 6.4.0-rc4+ #1 [ 17.217436] Hardware name: Micro-Star International Co., Ltd. MS-7A38/B450M PRO-VDH MAX (MS-7A38), BIOS B.G0 07/26/2022 [ 17.217438] RIP: 0010:ttm_bo_release+0x27e/0x2d0 [ttm] [ 17.217444] Code: 48 89 43 38 48 89 43 40 48 8b 5c 24 30 48 8b b5 40 08 00 00 48 8b 6c 24 38 48 83 c4 58 e9 7a 49 f7 e0 48 89 ef e9 6c fe ff ff <0f> 0b 48 83 7b 20 00 0f 84 b7 fd ff ff 0f 0b 0f 1f 00 e9 ad fd ff [ 17.217448] RSP: 0018:c995fbb0 EFLAGS: 00010202 [ 17.217451] RAX: 0001 RBX: 8881052c8de0 RCX: [ 17.217453] RDX: 0001 RSI: RDI: 8881052c8de0 [ 17.217455] RBP: 888104a66e00 R08: 8881052c8de0 R09: 888104a7cf08 [ 17.217457] R10: c995fbe0 R11: c995fbe8 R12: 8881052c8c78 [ 17.217458] R13: 8881052c8c78 R14: dead0100 R15: 88810528b108 [ 17.217460] FS: 7f319fcbb8c0() GS:1a54() knlGS: [ 17.217463] CS: 0010 DS: ES: CR0: 80050033 [ 17.217464] CR2: 55dc8b0224a0 CR3: 00010373d000 CR4: 00750ee0 [ 17.217466] PKRU: 5554 [ 17.217468] Call Trace: [ 17.217470] [ 17.217472] ? __warn+0x97/0x160 [ 17.217476] ? ttm_bo_release+0x27e/0x2d0 [ttm] [ 17.217481] ? report_bug+0x1ec/0x200 [ 17.217487] ? handle_bug+0x3c/0x70 [ 17.217490] ? exc_invalid_op+0x1f/0x90 [ 17.217493] ? preempt_count_sub+0xb5/0x100 [ 17.217496] ? asm_exc_invalid_op+0x16/0x20 [ 17.217500] ? ttm_bo_release+0x27e/0x2d0 [ttm] [ 17.217505] ? ttm_resource_move_to_lru_tail+0x1ab/0x1d0 [ttm] [ 17.217511] radeon_bo_unref+0x1a/0x30 [radeon] [ 17.217547] radeon_gem_object_free+0x20/0x30 [radeon] [ 17.217579] radeon_fbdev_fb_destroy+0x57/0x90 [radeon] [ 17.217616] unregister_framebuffer+0x72/0x110 [ 17.217620] drm_client_dev_unregister+0x6d/0xe0 [ 17.217623] drm_dev_unregister+0x2e/0x90 [ 17.217626] drm_put_dev+0x26/0x90 [ 17.217628] pci_device_remove+0x44/0xc0 [ 17.217631] really_probe+0x257/0x340 [ 17.217635] __driver_probe_device+0x73/0x120 [ 17.217638] driver_probe_device+0x2c/0xb0 [ 17.217641] __driver_attach+0xa0/0x150 [ 17.217643] ? __pfx___driver_attach+0x10/0x10 [ 17.217646] bus_for_each_dev+0x67/0xa0 [ 17.217649] bus_add_driver+0x10e/0x210 [ 17.217651] driver_register+0x5c/0x120 [ 17.217653] ? __pfx_radeon_module_init+0x10/0x10 [radeon] [ 17.217681] do_one_initcall+0x44/0x220 [ 17.217684] ? kmalloc_trace+0x37/0xc0 [ 17.217688] do_init_module+0x64/0x240 [ 17.217691] __do_sys_finit_module+0xb2/0x100 [ 17.217694] do_syscall_64+0x3b/0x90 [ 17.217697] entry_SYSCALL_64_after_hwframe+0x72/0xdc [ 17.217700] RIP: 0033:0x7f319feaa5a9 [ 17.217702] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 27 08 0d 00 f7 d8 64 89 01 48 [ 17.217706] RSP: 002b:7ffc6bf3e7f8 EFLAGS: 0246 ORIG_RAX: 0139 [ 17.217709] RAX: ffda RBX: 5607204f3170 RCX: 7f319feaa5a9 [ 17.217710] RDX: RSI: 7f31a002eefd RDI: 0018 [ 17.217712] RBP: 7f31a002eefd R08: R09: 5607204f1860 [ 17.217714] R10: 0018 R11: 0246 R12: 0002 [ 17.217716] R13: R14: 560720522450 R15: 560720255899 [ 17.217718] [ 17.217719] ---[ end trace ]--- The buffer object backing the fbdev emulation got pinned twice: by the fb_probe helper radeon_fbdev_create_pinned_object() and the modesetting code when the framebuffer got displayed. It only got unpinned once by the fbdev helper radeon_fbdev_destroy_pinned_object(). Hence TTM's BO- release function complains about the pin counter. Forcing the outputs off also undoes the modesettings pin increment. Reported-by: Borislav Petkov Closes: https://lore.kernel.org/dri-devel/20230603174814.GCZHt83pN+wNjf63sC@fat_crate.local/ Signed-off-by: Thomas Zimmermann Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation") Cc: Alex Deucher Cc: Thomas Zimmermann Cc: "Christian König" Cc: "Pan, Xinhui" Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/radeon/radeon_fbdev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/ra
[PATCH v2 1/4] drm/todo: Add atomic modesetting references
The section about converting existing KMS drivers to atomic modesetting mentions the existence of a conversion guide, but does not reference it. While the guide is old and rusty, it still contains useful information, so add a link to it. Also link to the LWN.net articles that give an overview about the atomic mode setting design. While at it, remove the reference to unconverted virtual HW drivers, as they've been converted. Signed-off-by: Geert Uytterhoeven Reviewed-by: Javier Martinez Canillas Reviewed-by: Laurent Pinchart --- v2: - Add Reviewed-by, - Drop double space after full stop, - Use footnotes for references, - Remore reference to unconverted virtual HW drivers. --- Documentation/gpu/todo.rst | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 68bdafa0284f55f6..6c328613c049fc1d 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -49,14 +49,18 @@ converted over. Modern compositors like Wayland or Surfaceflinger on Android really want an atomic modeset interface, so this is all about the bright future. -There is a conversion guide for atomic and all you need is a GPU for a -non-converted driver (again virtual HW drivers for KVM are still all -suitable). +There is a conversion guide for atomic [1]_ and all you need is a GPU for a +non-converted driver. The "Atomic mode setting design overview" series [2]_ +[3]_ at LWN.net can also be helpful. As part of this drivers also need to convert to universal plane (which means exposing primary & cursor as proper plane objects). But that's much easier to do by directly using the new atomic helper driver callbacks. + .. [1] https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html + .. [2] https://lwn.net/Articles/653071/ + .. [3] https://lwn.net/Articles/653466/ + Contact: Daniel Vetter, respective driver maintainers Level: Advanced -- 2.34.1
[PATCH v2 0/4] drm: Atomic modesetting doc and comment improvements
Hi all, This patch series contains various improvements to the documentation and comments related to atomic modesetting. Hopefully, it will ease the job of DRM novice who want to tackle the daunting task of converting a legacy DRM driver to atomic modesetting. Changes compared to 1: - Add Reviewed-by, - Drop double space after full stop, - Use footnotes for references, - Remore reference to unconverted virtual HW drivers, - New patch [2/4], - Drop "first part" in drivers/gpu/drm/drm_plane_helper.c. Thanks for your comments! Geert Uytterhoeven (4): drm/todo: Add atomic modesetting references drm/todo: Convert list of fbconv links to footnotes drm: Remove references to removed transitional helpers drm: Fix references to drm_plane_helper_check_state() Documentation/gpu/todo.rst | 20 ++ drivers/gpu/drm/drm_plane_helper.c | 12 +- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 +- drivers/gpu/drm/tidss/tidss_plane.c | 3 +- include/drm/drm_crtc.h | 5 --- include/drm/drm_modeset_helper_vtables.h | 48 +++- 6 files changed, 39 insertions(+), 52 deletions(-) -- 2.34.1 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
[PATCH v2 3/4] drm: Remove references to removed transitional helpers
The transitional helpers were removed a long time ago, but some references stuck. Remove them. Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers") Signed-off-by: Geert Uytterhoeven --- v2: - Drop "first part" in drivers/gpu/drm/drm_plane_helper.c. --- drivers/gpu/drm/drm_plane_helper.c | 12 +- include/drm/drm_crtc.h | 5 --- include/drm/drm_modeset_helper_vtables.h | 48 +++- 3 files changed, 23 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index c91e454eba097942..5e95089676ff81ed 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -40,8 +40,8 @@ /** * DOC: overview * - * This helper library has two parts. The first part has support to implement - * primary plane support on top of the normal CRTC configuration interface. + * This helper library contains helpers to implement primary plane support on + * top of the normal CRTC configuration interface. * Since the legacy &drm_mode_config_funcs.set_config interface ties the primary * plane together with the CRTC state this does not allow userspace to disable * the primary plane itself. The default primary plane only expose XRBG and @@ -51,14 +51,6 @@ * planes, and newly merged drivers must not rely upon these transitional * helpers. * - * The second part also implements transitional helpers which allow drivers to - * gradually switch to the atomic helper infrastructure for plane updates. Once - * that switch is complete drivers shouldn't use these any longer, instead using - * the proper legacy implementations for update and disable plane hooks provided - * by the atomic helpers. - * - * Again drivers are strongly urged to switch to the new interfaces. - * * The plane helpers share the function table structures with other helpers, * specifically also the atomic helpers. See &struct drm_plane_helper_funcs for * the details. diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8e1cbc75143ef216..8b48a1974da3143c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -77,11 +77,6 @@ struct drm_plane_helper_funcs; * intended to indicate whether a full modeset is needed, rather than strictly * describing what has changed in a commit. See also: * drm_atomic_crtc_needs_modeset() - * - * WARNING: Transitional helpers (like drm_helper_crtc_mode_set() or - * drm_helper_crtc_mode_set_base()) do not maintain many of the derived control - * state like @plane_mask so drivers not converted over to atomic helpers should - * not rely on these being accurate! */ struct drm_crtc_state { /** @crtc: backpointer to the CRTC */ diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 965faf082a6d1acb..e3c3ac615909474b 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -59,8 +59,8 @@ enum mode_set_atomic { /** * struct drm_crtc_helper_funcs - helper operations for CRTCs * - * These hooks are used by the legacy CRTC helpers, the transitional plane - * helpers and the new atomic modesetting helpers. + * These hooks are used by the legacy CRTC helpers and the new atomic + * modesetting helpers. */ struct drm_crtc_helper_funcs { /** @@ -216,9 +216,7 @@ struct drm_crtc_helper_funcs { * * This callback is used to update the display mode of a CRTC without * changing anything of the primary plane configuration. This fits the -* requirement of atomic and hence is used by the atomic helpers. It is -* also used by the transitional plane helpers to implement a -* @mode_set hook in drm_helper_crtc_mode_set(). +* requirement of atomic and hence is used by the atomic helpers. * * Note that the display pipe is completely off when this function is * called. Atomic drivers which need hardware to be running before they @@ -333,8 +331,8 @@ struct drm_crtc_helper_funcs { * all updated. Again the recommendation is to just call check helpers * until a maximal configuration is reached. * -* This callback is used by the atomic modeset helpers and by the -* transitional plane helpers, but it is optional. +* This callback is used by the atomic modeset helpers, but it is +* optional. * * NOTE: * @@ -373,8 +371,8 @@ struct drm_crtc_helper_funcs { * has picked. See drm_atomic_helper_commit_planes() for a discussion of * the tradeoffs and variants of plane commit helpers. * -* This callback is used by the atomic modeset helpers and by the -* transitional plane helpers, but it is optional. +* This callback is used by the atomic modeset helpers, but it is +* optional. */ void (*atomic_begin)(struct drm_crtc *crtc
[PATCH v2 4/4] drm: Fix references to drm_plane_helper_check_state()
As of commit a01cb8ba3f628293 ("drm: Move drm_plane_helper_check_state() into drm_atomic_helper.c"), drm_plane_helper_check_state() no longer exists, but is part of drm_atomic_helper_check_plane_state(). Signed-off-by: Geert Uytterhoeven Reviewed-by: Laurent Pinchart --- v2: - Add Reviewed-by. --- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++- drivers/gpu/drm/tidss/tidss_plane.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index d759e019218181ce..e445fac8e0b46c21 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -600,7 +600,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane, if (!state->crtc) { /* * The visible field is not reset by the DRM core but only -* updated by drm_plane_helper_check_state(), set it manually. +* updated by drm_atomic_helper_check_plane_state(), set it +* manually. */ state->visible = false; *format = NULL; diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c index 6bdd6e4a955ab3cc..e1c0ef0c3894c855 100644 --- a/drivers/gpu/drm/tidss/tidss_plane.c +++ b/drivers/gpu/drm/tidss/tidss_plane.c @@ -38,7 +38,8 @@ static int tidss_plane_atomic_check(struct drm_plane *plane, if (!new_plane_state->crtc) { /* * The visible field is not reset by the DRM core but only -* updated by drm_plane_helper_check_state(), set it manually. +* updated by drm_atomic_helper_check_plane_state(), set it +* manually. */ new_plane_state->visible = false; return 0; -- 2.34.1
[PATCH v2 2/4] drm/todo: Convert list of fbconv links to footnotes
Convert the references to fbconv links to footnotes, so they can be navigated. Signed-off-by: Geert Uytterhoeven --- v2: - New. --- Documentation/gpu/todo.rst | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 6c328613c049fc1d..ce1d4e22c327063b 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -753,16 +753,16 @@ existing hardware. The new driver's call-back functions are filled from existing fbdev code. More complex fbdev drivers can be refactored step-by-step into a DRM -driver with the help of the DRM fbconv helpers. [1] These helpers provide +driver with the help of the DRM fbconv helpers [4]_. These helpers provide the transition layer between the DRM core infrastructure and the fbdev driver interface. Create a new DRM driver on top of the fbconv helpers, copy over the fbdev driver, and hook it up to the DRM code. Examples for -several fbdev drivers are available at [1] and a tutorial of this process -available at [2]. The result is a primitive DRM driver that can run X11 +several fbdev drivers are available at [4]_ and a tutorial of this process +available at [5]_. The result is a primitive DRM driver that can run X11 and Weston. - - [1] https://gitlab.freedesktop.org/tzimmermann/linux/tree/fbconv - - [2] https://gitlab.freedesktop.org/tzimmermann/linux/blob/fbconv/drivers/gpu/drm/drm_fbconv_helper.c + .. [4] https://gitlab.freedesktop.org/tzimmermann/linux/tree/fbconv + .. [5] https://gitlab.freedesktop.org/tzimmermann/linux/blob/fbconv/drivers/gpu/drm/drm_fbconv_helper.c Contact: Thomas Zimmermann -- 2.34.1
Re: [PATCH] accel/habanalabs: add more debugfs stub helpers
On 09/06/2023 15:06, Arnd Bergmann wrote: > From: Arnd Bergmann > > Two functions got added with normal prototypes for debugfs, but not > alternative when building without it: > > drivers/accel/habanalabs/common/device.c: In function 'hl_device_init': > drivers/accel/habanalabs/common/device.c:2177:14: error: implicit declaration > of function 'hl_debugfs_device_init'; did you mean 'hl_debugfs_init'? > [-Werror=implicit-function-declaration] > drivers/accel/habanalabs/common/device.c:2305:9: error: implicit declaration > of function 'hl_debugfs_device_fini'; did you mean 'hl_debugfs_remove_file'? > [-Werror=implicit-function-declaration] > > Add stubs for these as well. > > Fixes: 553311fc7b76e ("accel/habanalabs: expose debugfs files later") > Signed-off-by: Arnd Bergmann Thanks, Reviewed-by: Tomer Tayar > --- > drivers/accel/habanalabs/common/habanalabs.h | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/accel/habanalabs/common/habanalabs.h > b/drivers/accel/habanalabs/common/habanalabs.h > index d92ba2e30e310..2f027d5a82064 100644 > --- a/drivers/accel/habanalabs/common/habanalabs.h > +++ b/drivers/accel/habanalabs/common/habanalabs.h > @@ -3980,6 +3980,15 @@ static inline void hl_debugfs_fini(void) > { > } > > +static inline int hl_debugfs_device_init(struct hl_device *hdev) > +{ > + return 0; > +} > + > +static inline void hl_debugfs_device_fini(struct hl_device *hdev) > +{ > +} > + > static inline void hl_debugfs_add_device(struct hl_device *hdev) > { > }
Re: [PATCH v10 9/9] drm: Remove superfluous print statements in DRM core
On Wed, 07 Jun 2023 00:00:10 +0530, Siddh Raman Pant wrote: > There are a couple of superfluous print statements using the drm_* > macros, which do stuff like printing newlines, print OOM messages > (OOM while allocating memory is already supposed to be noisy), and > printing strings like "Initialised" with no extra info whatsoever. > > Thus, remove a couple of these superfluous strings. > > Suggested-by: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com> > Signed-off-by: Siddh Raman Pant c...@siddh.me> This patch is the only one introducing these changes additionally, no other patch content has been changed from v9 and sent for merge. I should have clarified this in the cover. This patch may or may not be dropped, courtesy follow-up discussion on v9 regarding it: https://lore.kernel.org/lkml/87jzwfu1wf@intel.com/ Thanks, Siddh
Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing
Hi, On Friday, June 9th, 2023 at 13:31, Thomas Zimmermann wrote: > Is there a v3 of this patchset? It was Acked with the one errno code > changed. Since this was a minor change, I did it locally and pushed the patch to drm-misc-next already. Simon
Re: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled
Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin: On 09/06/2023 07:32, Christian König wrote: Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin: From: Tvrtko Ursulin For dma_fence_is_signaled signaling critical path annotations are an annoying cause of false positives when using dma_fence_is_signaled and indirectly higher level helpers such as dma_resv_test_signaled etc. Drop the critical path annotation since the "is signaled" API does not guarantee to ever change the signaled status anyway. We do that by adding a low level _dma_fence_signal helper and use it from dma_fence_is_signaled. I have been considering dropping the signaling from the dma_fence_is_signaled() function altogether. Doing this together with the spin locking we have in the dma_fence is just utterly nonsense since the purpose of the external spinlock is to keep the signaling in order while this here defeats that. The quick check is ok I think, but signaling the dma_fence and issuing the callbacks should always come from the interrupt handler. What do you think is broken with regards to ordering with the current code? The unlocked fast check? Well it's not broken, the complexity just doesn't make sense. The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t itself. That was done to make sure that all dma_fence objects from a single context (or in other words hardware device) signal in the order of their sequence number, e.g. 1 first, then 2, then 3 etc... But when somebody uses the dma_fence_is_signaled() function it's perfectly possible that this races with an interrupt handler which wants to signal fences on another CPU. In other words we get: CPU A: dma_fence_is_signaled(fence with seqno=3) fence->ops->signaled() returns true dma_fence_signal() spin_lock_irqsave(fence->lock) signal seqno=3 ... CPU B: device_driver_interrupt_handler() spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete signal seqno=1 signal seqno=2 seqno=3 is already signaled. signal seqno=4 ... Either fence->lock should not be a pointer or we should not signal the fence from dma_fence_is_signaled(). I strongly think that later should be the way to go. Regards, Christian. Regards, Tvrtko Christian. Signed-off-by: Tvrtko Ursulin Cc: Christian König Cc: Daniel Vetter --- drivers/dma-buf/dma-fence.c | 26 -- include/linux/dma-fence.h | 3 ++- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index f177c56269bb..ace1415f0566 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -444,6 +444,25 @@ int dma_fence_signal_locked(struct dma_fence *fence) } EXPORT_SYMBOL(dma_fence_signal_locked); +/** + * __dma_fence_signal - signal completion of a fence bypassing critical section annotation + * @fence: the fence to signal + * + * This low-level helper should not be used by code external to dma-fence.h|c! + */ +int _dma_fence_signal(struct dma_fence *fence) +{ + unsigned long flags; + int ret; + + spin_lock_irqsave(fence->lock, flags); + ret = dma_fence_signal_timestamp_locked(fence, ktime_get()); + spin_unlock_irqrestore(fence->lock, flags); + + return ret; +} +EXPORT_SYMBOL(_dma_fence_signal); + /** * dma_fence_signal - signal completion of a fence * @fence: the fence to signal @@ -459,7 +478,6 @@ EXPORT_SYMBOL(dma_fence_signal_locked); */ int dma_fence_signal(struct dma_fence *fence) { - unsigned long flags; int ret; bool tmp; @@ -467,11 +485,7 @@ int dma_fence_signal(struct dma_fence *fence) return -EINVAL; tmp = dma_fence_begin_signalling(); - - spin_lock_irqsave(fence->lock, flags); - ret = dma_fence_signal_timestamp_locked(fence, ktime_get()); - spin_unlock_irqrestore(fence->lock, flags); - + ret = _dma_fence_signal(fence); dma_fence_end_signalling(tmp); return ret; diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index d54b595a0fe0..d94768ad70e4 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -387,6 +387,7 @@ static inline void dma_fence_end_signalling(bool cookie) {} static inline void __dma_fence_might_wait(void) {} #endif +int _dma_fence_signal(struct dma_fence *fence); int dma_fence_signal(struct dma_fence *fence); int dma_fence_signal_locked(struct dma_fence *fence); int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp); @@ -452,7 +453,7 @@ dma_fence_is_signaled(struct dma_fence *fence) return true; if (fence->ops->signaled && fence->ops->signaled(fence)) { - dma_fence_signal(fence); + _dma_fence_signal(fence); return true; }
Re: [PATCH 2/3] drm: Remove references to removed transitional helpers
On Fri, Jun 2, 2023 at 1:17 PM Geert Uytterhoeven wrote: > On Fri, Jun 2, 2023 at 1:05 PM Laurent Pinchart > wrote: > > On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote: > > > The transitional helpers were removed a long time ago, but some > > > references stuck. Remove them. > > > > > > Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers") > > > Signed-off-by: Geert Uytterhoeven > > > > --- a/drivers/gpu/drm/drm_plane_helper.c > > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > > @@ -51,14 +51,6 @@ > > > * planes, and newly merged drivers must not rely upon these transitional > > > * helpers. > > > * > > > > The first paragraph starts with "This helper library has two parts.". As > > you're dropping the mention of the second part, I think you should > > rework the first paragraph too. > > That was my initial thought, too. > However, the code still has a second part, not related to the topic of > the first part (primary plane support). Upon deeper investigation, all of this is related to primary plane support. Will update... 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 6/8] drm: Add drm_gem_prime_fd_to_handle_obj
On 09-06-2023 17:41, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which > will return a reference to a newly created GEM objects (if created), in > order to enable tracking of imported i915 GEM objects in the following > patch. instead of this what if we implement the open call back in i915 struct drm_gem_object_funcs { /** * @open: * * Called upon GEM handle creation. * * This callback is optional. */ int (*open)(struct drm_gem_object *obj, struct drm_file *file); which gets called whenever a handle(drm_gem_handle_create_tail) is created and in the open we can check if to_intel_bo(obj)->base.dma_buf then it is imported if not it is owned or created by it. Thanks, Aravind. > > Minor code reshuffule and only trivial additions on top of > drm_gem_prime_fd_to_handle. > > Signed-off-by: Tvrtko Ursulin > --- > drivers/gpu/drm/drm_prime.c | 41 - > include/drm/drm_prime.h | 4 > 2 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index d29dafce9bb0..ef75f67e3057 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -284,11 +284,12 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf) > EXPORT_SYMBOL(drm_gem_dmabuf_release); > > /** > - * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers > + * drm_gem_prime_fd_to_handle_obj - PRIME import function for GEM drivers > * @dev: drm_device to import into > * @file_priv: drm file-private structure > * @prime_fd: fd id of the dma-buf which should be imported > * @handle: pointer to storage for the handle of the imported buffer object > + * @objp: optional pointer in which reference to created GEM object can be > returned > * > * This is the PRIME import function which must be used mandatorily by GEM > * drivers to ensure correct lifetime management of the underlying GEM > object. > @@ -297,9 +298,10 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); > * > * Returns 0 on success or a negative error code on failure. > */ > -int drm_gem_prime_fd_to_handle(struct drm_device *dev, > -struct drm_file *file_priv, int prime_fd, > -uint32_t *handle) > +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, > +struct drm_file *file_priv, int prime_fd, > +uint32_t *handle, > +struct drm_gem_object **objp) > { > struct dma_buf *dma_buf; > struct drm_gem_object *obj; > @@ -336,7 +338,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > > /* _handle_create_tail unconditionally unlocks dev->object_name_lock. */ > ret = drm_gem_handle_create_tail(file_priv, obj, handle); > - drm_gem_object_put(obj); > + if (!objp) > + drm_gem_object_put(obj); > if (ret) > goto out_put; > > @@ -348,6 +351,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > > dma_buf_put(dma_buf); > > + if (objp) > + *objp = obj; > + > return 0; > > fail: > @@ -356,6 +362,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, >*/ > drm_gem_handle_delete(file_priv, *handle); > dma_buf_put(dma_buf); > + if (objp) > + drm_gem_object_put(obj); > return ret; > > out_unlock: > @@ -365,6 +373,29 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > dma_buf_put(dma_buf); > return ret; > } > +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle_obj); > + > +/** > + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers > + * @dev: drm_device to import into > + * @file_priv: drm file-private structure > + * @prime_fd: fd id of the dma-buf which should be imported > + * @handle: pointer to storage for the handle of the imported buffer object > + * > + * This is the PRIME import function which must be used mandatorily by GEM > + * drivers to ensure correct lifetime management of the underlying GEM > object. > + * The actual importing of GEM object from the dma-buf is done through the > + * &drm_driver.gem_prime_import driver callback. > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int drm_gem_prime_fd_to_handle(struct drm_device *dev, > +struct drm_file *file_priv, int prime_fd, > +uint32_t *handle) > +{ > + return drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, handle, > + NULL); > +} > EXPORT_SYMBOL(drm_gem_prime_fd_to_handle); > > int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index 2a1d01e5b56b..10d145ce6586 100644
Re: [PATCH v5 3/4] PCI/VGA: Tidy up the code and comment format
Hi, On 2023/6/9 20:00, Andi Shyti wrote: Hi Sui, On Fri, Jun 09, 2023 at 07:24:16PM +0800, Sui Jingfeng wrote: This patch replaces the leading space with a tab and removes the double blank line, no functional change. You mainly fixed comment style, though and it's not written here. No need to resend for me as you also wrote it in the title. But next time please list all the changes in the commit log. OK, I remember this. This patch is trivial, that's fine. There are probably more complex patch in future, probably will send to you if the checkpatch.pl script mandate it. :-) Thanks a lot really. Signed-off-by: Sui Jingfeng Reviewed-by: Andi Shyti Andi --- drivers/pci/vgaarb.c | 108 - include/linux/vgaarb.h | 4 +- 2 files changed, 65 insertions(+), 47 deletions(-) diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 22a505e877dc..ceb914245383 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -61,7 +61,6 @@ static bool vga_arbiter_used; static DEFINE_SPINLOCK(vga_lock); static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue); - static const char *vga_iostate_to_str(unsigned int iostate) { /* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */ @@ -79,8 +78,10 @@ static const char *vga_iostate_to_str(unsigned int iostate) static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state) { - /* we could in theory hand out locks on IO and mem -* separately to userspace but it can cause deadlocks */ + /* +* We could in theory hand out locks on IO and mem +* separately to userspace but it can cause deadlocks +*/ if (strncmp(buf, "none", 4) == 0) { *io_state = VGA_RSRC_NONE; return 1; @@ -99,7 +100,7 @@ static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state) return 1; } -/* this is only used a cookie - it should not be dereferenced */ +/* This is only used as cookie, it should not be dereferenced */ static struct pci_dev *vga_default; /* Find somebody in our list */ @@ -193,14 +194,17 @@ int vga_remove_vgacon(struct pci_dev *pdev) #endif EXPORT_SYMBOL(vga_remove_vgacon); -/* If we don't ever use VGA arb we should avoid - turning off anything anywhere due to old X servers getting - confused about the boot device not being VGA */ +/* + * If we don't ever use VGA arb we should avoid + * turning off anything anywhere due to old X servers getting + * confused about the boot device not being VGA + */ static void vga_check_first_use(void) { - /* we should inform all GPUs in the system that -* VGA arb has occurred and to try and disable resources -* if they can */ + /* +* We should inform all GPUs in the system that +* vgaarb has occurred and to try and disable resources if they can +*/ if (!vga_arbiter_used) { vga_arbiter_used = true; vga_arbiter_notify_clients(); @@ -216,7 +220,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, unsigned int pci_bits; u32 flags = 0; - /* Account for "normal" resources to lock. If we decode the legacy, + /* +* Account for "normal" resources to lock. If we decode the legacy, * counterpart, we need to request it as well */ if ((rsrc & VGA_RSRC_NORMAL_IO) && @@ -236,7 +241,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, if (wants == 0) goto lock_them; - /* We don't need to request a legacy resource, we just enable + /* +* We don't need to request a legacy resource, we just enable * appropriate decoding and go */ legacy_wants = wants & VGA_RSRC_LEGACY_MASK; @@ -252,7 +258,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, if (vgadev == conflict) continue; - /* We have a possible conflict. before we go further, we must + /* +* We have a possible conflict. before we go further, we must * check if we sit on the same bus as the conflicting device. * if we don't, then we must tie both IO and MEM resources * together since there is only a single bit controlling @@ -263,13 +270,15 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM; } - /* Check if the guy has a lock on the resource. If he does, + /* +* Check if the guy has a lock on the resource. If he does, * return the conflicting entry */ if (conflict->locks & lwants) return conflict; - /* Ok, now check if it owns the resource we want. We can + /* +
Re: [PATCH v5 1/4] PCI/VGA: Use unsigned type for the io_state variable
Hi, On 2023/6/9 19:48, Andi Shyti wrote: Hi Sui, On Fri, Jun 09, 2023 at 07:24:14PM +0800, Sui Jingfeng wrote: The io_state variable in the vga_arb_write() function is declared with unsigned int type, while the vga_str_to_iostate() function takes int * type. To keep them consistent, replace the third argument of vga_str_to_iostate() function with the unsigned int * type. Signed-off-by: Sui Jingfeng Reviewed-by: Andi Shyti Thanks a lot. Andi --- drivers/pci/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 5a696078b382..c1bc6c983932 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -77,7 +77,7 @@ static const char *vga_iostate_to_str(unsigned int iostate) return "none"; } -static int vga_str_to_iostate(char *buf, int str_size, int *io_state) +static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state) { /* we could in theory hand out locks on IO and mem * separately to userspace but it can cause deadlocks */ -- 2.25.1 -- Jingfeng
Re: [Intel-gfx] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices
Hi, On 2023/6/9 03:12, Bjorn Helgaas wrote: Start with verb and capitalize to match ("Deal only with ...") On Thu, Jun 08, 2023 at 07:43:21PM +0800, Sui Jingfeng wrote: From: Sui Jingfeng vgaarb only deal with the VGA devcie(pdev->class == 0x0300), so replace the pci_get_subsys() function with pci_get_class(). Filter the non pci display device out. There no need to process the non display PCI device. s/non pci/non-PCI/ s/non display/non-display/ This is fine, and I'll merge this, but someday I would like to get rid of the bus_register_notifier() and call vga_arbiter_add_pci_device() and vga_arbiter_del_pci_device() directly from the PCI core. Nice idea! But I'm wondering there are traps in this. The pci_notifier in vgaarb.c is still need on Mips platform. Because of loading order problems. On MIPS system, PCI devices are enumerated by pcibios_init(), which runs after vga_arb_device_init(). This is true until now. When vga_arb_device_init() function get called, it will capture nothing. On that time, the system have no PCI device enumerated. Because of this, there are various problems in the past. This is the reason we still need the notifier, we need a way to capture the PCI display device after vgaarb already loaded(finished). On complex case, there are ASpeed BMC, loongson integrated display controller, and radeon discrete video card co-exist on Loongson 3B4000 server. I have fixed various bug at our downstream(linux-4.19) environment. I have fixed a bug on downstream involved with aspeed bmc, by workaround[1]. Chen Huacai grabbing my patch[1] and rewrite it, submit together with him-self's. Its fine, but those patch still look paleness in the front of Loongson integrated display controller. Loongson integrated display controller don't has a dedicated VRAM bar. vga_is_firmware_default() will lose its effectiveness then. This is the reason we reveal our patch(0004 in this series) to face upstream. Its not only for loongson integrated display controller through. Its not uncommon that ARM servers have A aspeed BMC and discrete amdgpu or old radeon card. Let the device drivers gave us a hint probably can help to resolve multi-video card co-exist problem. Consider that sometime user want to use integrate gpu, sometime user want to use discrete gpu. Also, during driver developing(or debugging), driver writer may want override the default boot device. vgaarb probable shouldn't make the decision without giving the device driver a chance to override. Beside, vga_is_firmware_default() only apply for PCI display device. On ARM64 system, there are a lot of platform device. If we move this function back to the device driver, it probably applicable for a platform display controller + one/two PCI display devices case. We find a method at downstream during we get efifb works LoongArch platform. We can utilize the screen_info, screen_info say where's the firmware framebuffer is located at. Drivers for specific hardware platform perhaps know more clearly than vgaarb. if it is the default boot device. [1] https://lore.kernel.org/all/20210514080025.1828197-6-chenhua...@loongson.cn/ Or if you wanted to do that now, that would be even better :) Bjorn Signed-off-by: Sui Jingfeng --- drivers/pci/vgaarb.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 7f0347f4f6fd..b0bf4952a95d 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -756,10 +756,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) struct pci_dev *bridge; u16 cmd; - /* Only deal with VGA class devices */ - if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) - return false; - /* Allocate structure */ vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL); if (vgadev == NULL) { @@ -1499,7 +1495,9 @@ static int pci_notify(struct notifier_block *nb, unsigned long action, struct pci_dev *pdev = to_pci_dev(dev); bool notify = false; - vgaarb_dbg(dev, "%s\n", __func__); + /* Only deal with VGA class devices */ + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) + return 0; /* For now we're only intereted in devices added and removed. I didn't * test this thing here, so someone needs to double check for the @@ -1509,6 +1507,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action, else if (action == BUS_NOTIFY_DEL_DEVICE) notify = vga_arbiter_del_pci_device(pdev); + vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action); + if (notify) vga_arbiter_notify_clients(); return 0; @@ -1533,8 +1533,8 @@ static struct miscdevice vga_arb_device = { static int __init vga_arb_device_init(void) { + struct pci_dev *pdev = NULL; int rc; - struct pci_dev *
[PATCH 8/8] drm/i915: Implement fdinfo memory stats printing
From: Tvrtko Ursulin Use the newly added drm_print_memory_stats helper to show memory utilisation of our objects in drm/driver specific fdinfo output. To collect the stats we walk the per memory regions object lists and accumulate object size into the respective drm_memory_stats categories. Objects with multiple possible placements are reported in multiple regions for total and shared sizes, while other categories are counted only for the currently active region. Signed-off-by: Tvrtko Ursulin Cc: Aravind Iddamsetty Cc: Rob Clark --- drivers/gpu/drm/i915/i915_drm_client.c | 64 ++ 1 file changed, 64 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c index 31316edbf30b..596de36ee09c 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.c +++ b/drivers/gpu/drm/i915/i915_drm_client.c @@ -48,6 +48,68 @@ void __i915_drm_client_free(struct kref *kref) } #ifdef CONFIG_PROC_FS +static void +obj_meminfo(struct drm_i915_gem_object *obj, + struct drm_memory_stats stats[INTEL_REGION_UNKNOWN]) +{ + struct intel_memory_region *mr; + u64 sz = obj->base.size; + enum intel_region_id id; + unsigned int i; + + /* Attribute size and shared to all possible memory regions. */ + for (i = 0; i < obj->mm.n_placements; i++) { + mr = obj->mm.placements[i]; + id = mr->id; + + if (obj->base.handle_count > 1) + stats[id].shared += sz; + else + stats[id].private += sz; + } + + /* Attribute other categories to only the current region. */ + mr = obj->mm.region; + if (mr) + id = mr->id; + else + id = INTEL_REGION_SMEM; + + if (i915_gem_object_has_pages(obj)) { + stats[id].resident += sz; + + if (!dma_resv_test_signaled(obj->base.resv, + dma_resv_usage_rw(true))) + stats[id].active += sz; + else if (i915_gem_object_is_shrinkable(obj) && +obj->mm.madv == I915_MADV_DONTNEED) + stats[id].purgeable += sz; + } +} + +static void show_meminfo(struct drm_printer *p, struct drm_file *file) +{ + struct drm_memory_stats stats[INTEL_REGION_UNKNOWN] = {}; + struct drm_i915_file_private *fpriv = file->driver_priv; + struct i915_drm_client *client = fpriv->client; + struct drm_i915_private *i915 = fpriv->i915; + struct drm_i915_gem_object *obj; + struct intel_memory_region *mr; + unsigned int id; + + spin_lock_irq(&client->objects_lock); + list_for_each_entry(obj, &client->objects_list, client_link) + obj_meminfo(obj, stats); + spin_unlock_irq(&client->objects_lock); + + for_each_memory_region(mr, i915, id) + drm_print_memory_stats(p, + &stats[id], + DRM_GEM_OBJECT_RESIDENT | + DRM_GEM_OBJECT_PURGEABLE, + mr->name); +} + static const char * const uabi_class_names[] = { [I915_ENGINE_CLASS_RENDER] = "render", [I915_ENGINE_CLASS_COPY] = "copy", @@ -109,6 +171,8 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file) * ** */ + show_meminfo(p, file); + if (GRAPHICS_VER(i915) < 8) return; -- 2.39.2
[PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj
From: Tvrtko Ursulin I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which will return a reference to a newly created GEM objects (if created), in order to enable tracking of imported i915 GEM objects in the following patch. Minor code reshuffule and only trivial additions on top of drm_gem_prime_fd_to_handle. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/drm_prime.c | 41 - include/drm/drm_prime.h | 4 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index d29dafce9bb0..ef75f67e3057 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -284,11 +284,12 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf) EXPORT_SYMBOL(drm_gem_dmabuf_release); /** - * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers + * drm_gem_prime_fd_to_handle_obj - PRIME import function for GEM drivers * @dev: drm_device to import into * @file_priv: drm file-private structure * @prime_fd: fd id of the dma-buf which should be imported * @handle: pointer to storage for the handle of the imported buffer object + * @objp: optional pointer in which reference to created GEM object can be returned * * This is the PRIME import function which must be used mandatorily by GEM * drivers to ensure correct lifetime management of the underlying GEM object. @@ -297,9 +298,10 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); * * Returns 0 on success or a negative error code on failure. */ -int drm_gem_prime_fd_to_handle(struct drm_device *dev, - struct drm_file *file_priv, int prime_fd, - uint32_t *handle) +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle, + struct drm_gem_object **objp) { struct dma_buf *dma_buf; struct drm_gem_object *obj; @@ -336,7 +338,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, /* _handle_create_tail unconditionally unlocks dev->object_name_lock. */ ret = drm_gem_handle_create_tail(file_priv, obj, handle); - drm_gem_object_put(obj); + if (!objp) + drm_gem_object_put(obj); if (ret) goto out_put; @@ -348,6 +351,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, dma_buf_put(dma_buf); + if (objp) + *objp = obj; + return 0; fail: @@ -356,6 +362,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, */ drm_gem_handle_delete(file_priv, *handle); dma_buf_put(dma_buf); + if (objp) + drm_gem_object_put(obj); return ret; out_unlock: @@ -365,6 +373,29 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, dma_buf_put(dma_buf); return ret; } +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle_obj); + +/** + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers + * @dev: drm_device to import into + * @file_priv: drm file-private structure + * @prime_fd: fd id of the dma-buf which should be imported + * @handle: pointer to storage for the handle of the imported buffer object + * + * This is the PRIME import function which must be used mandatorily by GEM + * drivers to ensure correct lifetime management of the underlying GEM object. + * The actual importing of GEM object from the dma-buf is done through the + * &drm_driver.gem_prime_import driver callback. + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_gem_prime_fd_to_handle(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle) +{ + return drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, handle, + NULL); +} EXPORT_SYMBOL(drm_gem_prime_fd_to_handle); int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index 2a1d01e5b56b..10d145ce6586 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -69,6 +69,10 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf); int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle, + struct drm_gem_object **obj); int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd); -- 2.3
[PATCH 7/8] drm/i915: Track imported dma-buf objects in memory stats
From: Tvrtko Ursulin We want to be able to show memory usage of imported dma-buf opjects in the fdinfo stats. To achieve this we wrap drm_gem_prime_fd_to_handle(_obj) in i915_gem_prime_fd_to_handle and append some client management at the end. Signed-off-by: Tvrtko Ursulin Cc: Aravind Iddamsetty --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 32 ++ drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h | 7 + drivers/gpu/drm/i915/i915_driver.c | 2 +- 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index fd556a076d05..2e2d9d7c1992 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -11,7 +11,10 @@ #include +#include + #include "gem/i915_gem_dmabuf.h" +#include "i915_drm_client.h" #include "i915_drv.h" #include "i915_gem_object.h" #include "i915_scatterlist.h" @@ -344,6 +347,35 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, return ERR_PTR(ret); } +int i915_gem_prime_fd_to_handle(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle) +{ + struct drm_gem_object *gem_obj = NULL; + int ret; + + if (IS_ENABLED(CONFIG_PROC_FS)) + ret = drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, +handle, &gem_obj); + else + ret = drm_gem_prime_fd_to_handle(dev, file_priv, prime_fd, +handle); + if (ret) + return ret; + + if (gem_obj) { + struct drm_i915_file_private *fpriv = file_priv->driver_priv; + struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); + + /* Really imported and not just alias? */ + if (obj->ops == &i915_gem_object_dmabuf_ops) + i915_drm_client_add_object(fpriv->client, obj); + i915_gem_object_put(obj); + } + + return 0; +} + #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/mock_dmabuf.c" #include "selftests/i915_gem_dmabuf.c" diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h index 6e0405d47ce1..63635c221c7c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h @@ -6,8 +6,11 @@ #ifndef __I915_GEM_DMABUF_H__ #define __I915_GEM_DMABUF_H__ +#include + struct drm_gem_object; struct drm_device; +struct drm_file; struct dma_buf; struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, @@ -15,4 +18,8 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, struct dma_buf *i915_gem_prime_export(struct drm_gem_object *gem_obj, int flags); +int i915_gem_prime_fd_to_handle(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle); + #endif /* __I915_GEM_DMABUF_H__ */ diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index ace8534b6cc5..03f3157371bf 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -1806,7 +1806,7 @@ static const struct drm_driver i915_drm_driver = { .show_fdinfo = i915_drm_client_fdinfo, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, - .prime_fd_to_handle = drm_gem_prime_fd_to_handle, + .prime_fd_to_handle = i915_gem_prime_fd_to_handle, .gem_prime_import = i915_gem_prime_import, .dumb_create = i915_gem_dumb_create, -- 2.39.2