Re: [PATCH 0/8] drm/mgag200: Support desktop chips
Am 16.07.20 um 07:44 schrieb Thomas Zimmermann: > Hi > > Am 15.07.20 um 21:56 schrieb Dave Airlie: >> On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann wrote: >>> >>> This patchset puts device initialization in the correct order and >>> adds support for G200 Desktop chips (PCI ids 0x520 and 0x521). >> >> why? :-) > > With G200 support in place, we can add also support for the newer cards > in the G-Series up to the G550. Believe it or not, the G550 for PCIe is > still being actively marketed and manufactured by Matrox. [1] Even the > predecessor chips G450 was only EOLed in Oct 2016. [2] So while the > chips might be 20yrs old, the devices are still current. > > Best regards > Thomas > > [1] > https://matrox.com/graphics/en/products/graphics_cards/g_series/g550pcie/?productTabs=1 > [2] https://www.matrox.com/graphics/en/products/legacy/g_series/g450pci/ > >> >> I'm pretty sure I NAKed the previous version because the userspace >> experience for these old cards was probably better with >> xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go >> ahead. I know SuSE use these for testing, but apart from that do we >> really think we have any users for this? Well, I got at least one email from someone thanking me for this patch. :) >> >> Dave. >> ___ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] drm/mgag200: Support desktop chips
Hi Am 15.07.20 um 21:56 schrieb Dave Airlie: > On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann wrote: >> >> This patchset puts device initialization in the correct order and >> adds support for G200 Desktop chips (PCI ids 0x520 and 0x521). > > why? :-) With G200 support in place, we can add also support for the newer cards in the G-Series up to the G550. Believe it or not, the G550 for PCIe is still being actively marketed and manufactured by Matrox. [1] Even the predecessor chips G450 was only EOLed in Oct 2016. [2] So while the chips might be 20yrs old, the devices are still current. Best regards Thomas [1] https://matrox.com/graphics/en/products/graphics_cards/g_series/g550pcie/?productTabs=1 [2] https://www.matrox.com/graphics/en/products/legacy/g_series/g450pci/ > > I'm pretty sure I NAKed the previous version because the userspace > experience for these old cards was probably better with > xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go > ahead. I know SuSE use these for testing, but apart from that do we > really think we have any users for this? > > Dave. > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/display: Ensure that ret is always initialized in icl_combo_phy_verify_state
On Wed, Jul 15, 2020 at 09:27:42PM -0700, Nathan Chancellor wrote:
> Clang warns:
>
> drivers/gpu/drm/i915/display/intel_combo_phy.c:268:3: warning: variable
> 'ret' is uninitialized when used here [-Wuninitialized]
> ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_LN0(phy),
> ^~~
> drivers/gpu/drm/i915/display/intel_combo_phy.c:261:10: note: initialize
> the variable 'ret' to silence this warning
> bool ret;
> ^
> = 0
> 1 warning generated.
>
> In practice, the bug this warning appears to be concerned with would not
> actually matter because ret gets initialized to the return value of
> cnl_verify_procmon_ref_values. However, that does appear to be a bug
> since it means the first hunk of the patch this fixes won't actually do
> anything (since the values of check_phy_reg won't factor into the final
> ret value). Initialize ret to true then make all of the assignments a
> bitwise AND with itself so that the function always does what it should
> do.
>
> Fixes: 239bef676d8e ("drm/i915/display: Implement new combo phy
> initialization step")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1094
> Signed-off-by: Nathan Chancellor
Reviewed-by: Matt Roper
> ---
> drivers/gpu/drm/i915/display/intel_combo_phy.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> index eccaa79cb4a9..a4b8aa6d0a9e 100644
> --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> @@ -258,7 +258,7 @@ static bool phy_is_master(struct drm_i915_private
> *dev_priv, enum phy phy)
> static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> enum phy phy)
> {
> - bool ret;
> + bool ret = true;
> u32 expected_val = 0;
>
> if (!icl_combo_phy_enabled(dev_priv, phy))
> @@ -276,7 +276,7 @@ static bool icl_combo_phy_verify_state(struct
> drm_i915_private *dev_priv,
>DCC_MODE_SELECT_CONTINUOSLY);
> }
>
> - ret = cnl_verify_procmon_ref_values(dev_priv, phy);
> + ret &= cnl_verify_procmon_ref_values(dev_priv, phy);
>
> if (phy_is_master(dev_priv, phy)) {
> ret &= check_phy_reg(dev_priv, phy, ICL_PORT_COMP_DW8(phy),
>
> base-commit: ca0e494af5edb59002665bf12871e94b4163a257
> --
> 2.28.0.rc0
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 207383] [Regression] 5.7 amdgpu/polaris11 gpf: amdgpu_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=207383 --- Comment #65 from Duncan (1i5t5.dun...@cox.net) --- (In reply to Duncan from comment #63) > NB: The 3202fa62f followups are cbfc35a48 and 89b83f282. That should let > anyone else with git and kernel building skills try reverting the three. > > Still too early (by days) to call it nailed down as I've had it take 2-3 > days to trigger, but no gfx freeze here yet on that v5.8-rc5+ with > 320-and-followups reverted so far, despite playing 4k video to try to > trigger it as it has previously on affected kernels. I'll be trying update > builds (gentoo) later today or tomorrow, another previous trigger, so we'll > see how it goes. I'm still not saying for sure, but that's actually looking like the culprit. Today's gentoo update included a dep of qtwebengine, which changed ABI so qtwebengine needed rebuilt on top of it, and qtwebengine is chromium-based. And as anyone that's built chromium (or firefox for that matter) can tell you, at least on older fx-based hardware, it's several hours of near constant 100% all-cores. While rebuilding qtwebengine (at a batch-nice of +19 so it doesn't interfere too badly with anything else I want to run), I was playing youtube videos at 1080p, not normally a problem by themselves (tho 4k can be, especially 4k60) but with qtwebengine building at the same time... No freezes. I'm going to run with the 320 commit and followups reverted a few more days before declaring it for sure the culprit, and I'm watching for Anthony's results as well, but the bug's sure doing a convincing job of hiding ATM if that commit isn't the culprit! I'd say it's time to start reviewing the amdgpu code to see what relocating the slub freelist pointer to the middle of the object (what the 320 commit did according to its git log explanation) could tickle, when the work goes on the work queue to run later, since that's consistently what the logs say is the scenario and what mnrzk confirmed by forcing it /not/ to go to the work queue in comment #30. Hopefully we can still get and confirm a proper codefix by 5.8.0 release. =:^) -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] drm/mgag200: Support desktop chips
Will try to look over this tomorrow, jfyi On Wed, 2020-07-15 at 16:58 +0200, Thomas Zimmermann wrote: > This patchset puts device initialization in the correct order and > adds support for G200 Desktop chips (PCI ids 0x520 and 0x521). > > The first 7 patches prepare the driver. Desktop chips would probably > work without them, but since we're at it we can also do it right. > > Patch 1 enables cached page mappings GEM buffers. SHMEM supports > this well now and the MGA device does not access the buffer memory > directly. So now corrupt display output is to be expected. > > Patches 2 to 6 fix the initialization of device registers. Several > fundamental registers were only done late during device initialization. > This was probably not a problem in practice, as the VGA BIOS does the > setup iduring POST anyway. These patches move the code to the beginning > of the device initialization. If we ever have to POST a MGA device from > the driver, the corect order of operations counts. > > G200SEs store a unique id in the device structure. Patch 7 moves the > value to model-specific data area. This will be helpful for patch 8. > > Patch 8 adds support for desktop chips' PCI ids. all the memory and > modesetting code continues to work as before. The PLL setup code gets > an additional helper for the new HW. PCI and DAC regsiters get a few > new default values. Most significantly, the driver parses the VGA BIOS > for clock settings. It's all separate from the server code, so no > regressions are to be expected. > > The new HW support is based on an earlier patch the was posted in July > 2017. [1] > > Tested on G200EW and G200 AGP hardware by running the fbdev console, > Weston and Gnome on Xorg. > > [1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147647.html > > Thomas Zimmermann (8): > drm/mgag200: Enable caching for SHMEM pages > drm/mgag200: Move register initialization into helper function > drm/mgag200: Initialize PCI registers early during device setup > drm/mgag200: Enable MGA mode during device register initialization > drm/mgag200: Set MISC memory flags in mm init code > drm/mgag200: Clear field during MM init > drm/mgag200: Move G200SE's unique id into model-specific data > drm/mgag200: Add support for G200 desktop cards > > MAINTAINERS| 2 +- > drivers/gpu/drm/mgag200/Kconfig| 12 +- > drivers/gpu/drm/mgag200/mgag200_drv.c | 213 +++-- > drivers/gpu/drm/mgag200/mgag200_drv.h | 19 ++- > drivers/gpu/drm/mgag200/mgag200_mm.c | 8 + > drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++--- > drivers/gpu/drm/mgag200/mgag200_reg.h | 4 + > 7 files changed, 328 insertions(+), 83 deletions(-) > > -- > 2.27.0 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/msm/dpu: fix/enable 6bpc dither with split-lm
From: Rob Clark
If split-lm is used (for ex, on sdm845), we can have multiple ping-
pongs, but only a single phys encoder. We need to configure dithering
on each of them.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 ++-
.../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 3 +--
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 46df0ff75b85..9b98b63c77fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -212,14 +212,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
-static void _dpu_encoder_setup_dither(struct dpu_encoder_phys *phys)
+static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned
bpc)
{
struct dpu_hw_dither_cfg dither_cfg = { 0 };
- if (!phys->hw_pp || !phys->hw_pp->ops.setup_dither)
+ if (!hw_pp->ops.setup_dither)
return;
- switch (phys->connector->display_info.bpc) {
+ switch (bpc) {
case 6:
dither_cfg.c0_bitdepth = 6;
dither_cfg.c1_bitdepth = 6;
@@ -228,14 +228,14 @@ static void _dpu_encoder_setup_dither(struct
dpu_encoder_phys *phys)
dither_cfg.temporal_en = 0;
break;
default:
- phys->hw_pp->ops.setup_dither(phys->hw_pp, NULL);
+ hw_pp->ops.setup_dither(hw_pp, NULL);
return;
}
memcpy(_cfg.matrix, dither_matrix,
sizeof(u32) * DITHER_MATRIX_SZ);
- phys->hw_pp->ops.setup_dither(phys->hw_pp, _cfg);
+ hw_pp->ops.setup_dither(hw_pp, _cfg);
}
void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
@@ -1132,11 +1132,13 @@ static void _dpu_encoder_virt_enable_helper(struct
drm_encoder *drm_enc)
_dpu_encoder_update_vsync_source(dpu_enc, _enc->disp_info);
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- _dpu_encoder_setup_dither(phys);
+ if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
+ !WARN_ON(dpu_enc->num_phys_encs == 0)) {
+ unsigned bpc =
dpu_enc->phys_encs[0]->connector->display_info.bpc;
+ for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+ if (!dpu_enc->hw_pp[i])
+ continue;
+ _dpu_encoder_setup_dither(dpu_enc->hw_pp[i], bpc);
}
}
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index 7411ab6bf6af..bea4ab5c58c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -231,8 +231,7 @@ static void _setup_pingpong_ops(struct dpu_hw_pingpong *c,
c->ops.poll_timeout_wr_ptr = dpu_hw_pp_poll_timeout_wr_ptr;
c->ops.get_line_count = dpu_hw_pp_get_line_count;
- if (test_bit(DPU_PINGPONG_DITHER, ) &&
- IS_SC7180_TARGET(c->hw.hwversion))
+ if (test_bit(DPU_PINGPONG_DITHER, ))
c->ops.setup_dither = dpu_hw_pp_setup_dither;
};
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[pull] amdgpu drm-fixes-5.8
Hi Dave, Daniel, Fixes for 5.8. The following changes since commit 38794a5465b752118098e36cf95c59083f9f1f88: Merge tag 'amd-drm-fixes-5.8-2020-07-09' of git://people.freedesktop.org/~agd5f/linux into drm-fixes (2020-07-10 07:02:02 +1000) are available in the Git repository at: git://people.freedesktop.org/~agd5f/linux tags/amd-drm-fixes-5.8-2020-07-15 for you to fetch changes up to 05051496b2622e4d12e2036b35165969aa502f89: drm/amdgpu/sdma5: fix wptr overwritten in ->get_wptr() (2020-07-14 15:42:17 -0400) amd-drm-fixes-5.8-2020-07-15: amdgpu: - Fix a race condition with KIQ - Preemption fix - Fix handling of fake MST encoders - OLED panel fix - Handle allocation failure in stream construction - Renoir SMC fix - SDMA 5.x fix Alex Deucher (1): drm/amdgpu/display: create fake mst encoders ahead of time (v4) Jack Xiao (2): drm/amdgpu/gfx10: fix race condition for kiq drm/amdgpu: fix preemption unit test Josip Pavic (1): drm/amd/display: handle failed allocation during stream construction Xiaojie Yuan (1): drm/amdgpu/sdma5: fix wptr overwritten in ->get_wptr() chen gong (1): drm/amdgpu/powerplay: Modify SMC message name for setting power profile mode hersen wu (1): drm/amd/display: OLED panel backlight adjust not work with external display connected drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c| 20 ++-- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 9 +++- drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 26 --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 ++ drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 11 - .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c| 53 +++--- .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.h| 3 ++ drivers/gpu/drm/amd/display/dc/core/dc_stream.c| 19 ++-- drivers/gpu/drm/amd/powerplay/renoir_ppt.c | 2 +- 9 files changed, 101 insertions(+), 56 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] dt-binding: display: Allow a single port node on rocktech, jh057n00900
On Fri, 03 Jul 2020 13:47:17 +0200, Ondrej Jirman wrote: > The display has one port. Allow it in the binding. > > Signed-off-by: Ondrej Jirman > --- > .../devicetree/bindings/display/panel/rocktech,jh057n00900.yaml | 1 + > 1 file changed, 1 insertion(+) > Reviewed-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] dt-bindings: display: Fix example in nwl-dsi.yaml
On Fri, 03 Jul 2020 13:47:16 +0200, Ondrej Jirman wrote: > The example is now validated against rocktech,jh057n00900 schema > that was ported to yaml, and didn't validate with: > > - '#address-cells', '#size-cells', 'port@0' do not match any of > the regexes: 'pinctrl-[0-9]+' > - 'vcc-supply' is a required property > - 'iovcc-supply' is a required property > - 'reset-gpios' is a required property > > Fix it. > > Signed-off-by: Ondrej Jirman > --- > .../devicetree/bindings/display/bridge/nwl-dsi.yaml | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > Reviewed-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] drm/mgag200: Support desktop chips
On Wed, Jul 15, 2020 at 9:56 PM Dave Airlie wrote: > > On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann wrote: > > > > This patchset puts device initialization in the correct order and > > adds support for G200 Desktop chips (PCI ids 0x520 and 0x521). > > why? :-) > > I'm pretty sure I NAKed the previous version because the userspace > experience for these old cards was probably better with > xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go > ahead. I know SuSE use these for testing, but apart from that do we > really think we have any users for this? I'm more of the "why not" kind ... if you don't want this driver, don't enable it. Maybe worst case the physical card driver ids should be a Kconfig option or so. But if the goal is to stomp fbdev into the ground I think we should be ok with having drivers for anything. Even if it's kinda horrible :-) Of course you're not going to get any kind of acceleration, but then modern desktops don't accelerate if you have anything less than maybe gles2 anyway, and that entire idea of a reasonable 2d api that's actually generally useful died a hundred times already. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv1 1/4] dt-bindings: display: panel-dsi-cm: convert to YAML
On Tue, 30 Jun 2020 00:33:12 +0200, Sebastian Reichel wrote: > Convert panel-dsi-cm bindings to YAML and add > missing properties while at it. > > Signed-off-by: Sebastian Reichel > --- > .../bindings/display/panel/panel-dsi-cm.txt | 29 - > .../bindings/display/panel/panel-dsi-cm.yaml | 100 ++ > 2 files changed, 100 insertions(+), 29 deletions(-) > delete mode 100644 > Documentation/devicetree/bindings/display/panel/panel-dsi-cm.txt > create mode 100644 > Documentation/devicetree/bindings/display/panel/panel-dsi-cm.yaml > Reviewed-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names
On Wed, Jul 15, 2020 at 12:07:30PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> If there is no interconnect-names, but there is an interconnects
> property, then of_icc_get(dev, "gfx-mem"); would return an error
> rather than NULL.
>
> Also, if there is no interconnect-names property, there will never
> be a ocmem path. But of_icc_get(dev, "ocmem") would return -EINVAL
> instead of -ENODATA. Just don't bother trying in this case.
>
> v2: explicity check for interconnect-names property
>
> Fixes: 8e29fb37b301 ("drm/msm: handle for EPROBE_DEFER for of_icc_get")
> Fixes: 00bb9243d346 ("drm/msm/gpu: add support for ocmem interconnect path")
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 0527e85184e1..e23641a5ec84 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -1003,22 +1003,23 @@ int adreno_gpu_init(struct drm_device *drm, struct
> platform_device *pdev,
> if (ret)
> return ret;
>
> - /* Check for an interconnect path for the bus */
> - gpu->icc_path = of_icc_get(dev, "gfx-mem");
> - if (!gpu->icc_path) {
> - /*
> - * Keep compatbility with device trees that don't have an
> - * interconnect-names property.
> - */
> + /*
> + * The legacy case, before "interconnect-names", only has a
> + * single interconnect path which is equivalent to "gfx-mem"
> + */
> + if (!of_find_property(dev->of_node, "interconnect-names", NULL)) {
> gpu->icc_path = of_icc_get(dev, NULL);
> + } else {
> + gpu->icc_path = of_icc_get(dev, "gfx-mem");
> + gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
> }
> +
> if (IS_ERR(gpu->icc_path)) {
> ret = PTR_ERR(gpu->icc_path);
> gpu->icc_path = NULL;
> return ret;
> }
>
> - gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
> if (IS_ERR(gpu->ocmem_icc_path)) {
> ret = PTR_ERR(gpu->ocmem_icc_path);
> gpu->ocmem_icc_path = NULL;
> @@ -1026,6 +1027,7 @@ int adreno_gpu_init(struct drm_device *drm, struct
> platform_device *pdev,
> if (ret != -ENODATA)
> return ret;
> }
> +
Nit for an extra blank line but otherwise looks fine. I like this workaround.
With that, Reviewed-by: Jordan Crouse
> return 0;
> }
>
> --
> 2.26.2
>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] drm/mgag200: Support desktop chips
On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann wrote: > > This patchset puts device initialization in the correct order and > adds support for G200 Desktop chips (PCI ids 0x520 and 0x521). why? :-) I'm pretty sure I NAKed the previous version because the userspace experience for these old cards was probably better with xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go ahead. I know SuSE use these for testing, but apart from that do we really think we have any users for this? Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[radeon-alex:drm-next 140/193] drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:1233:3: warning: variable 'direct_poll' set but not used
tree: git://people.freedesktop.org/~agd5f/linux.git drm-next
head: d7373aaf738393f38f40b0570bfa1403584eb982
commit: 1f61a43fcec1dceac2ec537a63aba6846dd0e80a [140/193] drm/amd/sriov
porting sriov cap to vcn3.0
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 1f61a43fcec1dceac2ec537a63aba6846dd0e80a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c: In function 'vcn_v3_0_start_sriov':
>> drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:1233:3: warning: variable
>> 'direct_poll' set but not used [-Wunused-but-set-variable]
1233 | direct_poll = { {0} };
| ^~~
In file included from drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:25:
At top level:
drivers/gpu/drm/amd/amdgpu/amdgpu.h:192:19: warning: 'debug_evictions'
defined but not used [-Wunused-const-variable=]
192 | static const bool debug_evictions; /* = false */
| ^~~
drivers/gpu/drm/amd/amdgpu/amdgpu.h:191:18: warning: 'sched_policy' defined
but not used [-Wunused-const-variable=]
191 | static const int sched_policy = KFD_SCHED_POLICY_HWS;
| ^~~~
In file included from drivers/gpu/drm/amd/amdgpu/../display/dc/dc_types.h:33,
from
drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services_types.h:30,
from
drivers/gpu/drm/amd/amdgpu/../include/dm_pp_interface.h:26,
from drivers/gpu/drm/amd/amdgpu/amdgpu.h:65,
from drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:25:
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:76:32: warning:
'dc_fixpt_ln2_div_2' defined but not used [-Wunused-const-variable=]
76 | static const struct fixed31_32 dc_fixpt_ln2_div_2 = { 1488522236LL };
|^~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:75:32: warning:
'dc_fixpt_ln2' defined but not used [-Wunused-const-variable=]
75 | static const struct fixed31_32 dc_fixpt_ln2 = { 2977044471LL };
|^~~~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:74:32: warning:
'dc_fixpt_e' defined but not used [-Wunused-const-variable=]
74 | static const struct fixed31_32 dc_fixpt_e = { 11674931555LL };
|^~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:73:32: warning:
'dc_fixpt_two_pi' defined but not used [-Wunused-const-variable=]
73 | static const struct fixed31_32 dc_fixpt_two_pi = { 26986075409LL };
|^~~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:72:32: warning:
'dc_fixpt_pi' defined but not used [-Wunused-const-variable=]
72 | static const struct fixed31_32 dc_fixpt_pi = { 13493037705LL };
|^~~
drivers/gpu/drm/amd/amdgpu/../display/include/fixed31_32.h:67:32: warning:
'dc_fixpt_zero' defined but not used [-Wunused-const-variable=]
67 | static const struct fixed31_32 dc_fixpt_zero = { 0 };
|^
vim +/direct_poll +1233 drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
1210
1211 static int vcn_v3_0_start_sriov(struct amdgpu_device *adev)
1212 {
1213 int i, j;
1214 struct amdgpu_ring *ring;
1215 uint64_t cache_addr;
1216 uint64_t rb_addr;
1217 uint64_t ctx_addr;
1218 uint32_t param, resp, expected;
1219 uint32_t offset, cache_size;
1220 uint32_t tmp, timeout;
1221 uint32_t id;
1222
1223 struct amdgpu_mm_table *table = >virt.mm_table;
1224 uint32_t *table_loc;
1225 uint32_t table_size;
1226 uint32_t size, size_dw;
1227
1228 struct mmsch_v3_0_cmd_direct_write
1229 direct_wt = { {0} };
1230 struct mmsch_v3_0_cmd_direct_read_modify_write
1231 direct_rd_mod_wt = { {0} };
1232 struct mmsch_v3_0_cmd_direct_polling
> 1233 direct_poll = { {0} };
1234 struct mmsch_v3_0_cmd_end end = { {0} };
1235 struct mmsch_v3_0_init_header header;
1236
1237 direct_wt.cmd_header.command_type =
1238 MMSCH_COMMAND__DIRECT_REG_WRITE;
1239 direct_rd_mod_wt.cmd_header.command_type =
1240
[PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names
From: Rob Clark
If there is no interconnect-names, but there is an interconnects
property, then of_icc_get(dev, "gfx-mem"); would return an error
rather than NULL.
Also, if there is no interconnect-names property, there will never
be a ocmem path. But of_icc_get(dev, "ocmem") would return -EINVAL
instead of -ENODATA. Just don't bother trying in this case.
v2: explicity check for interconnect-names property
Fixes: 8e29fb37b301 ("drm/msm: handle for EPROBE_DEFER for of_icc_get")
Fixes: 00bb9243d346 ("drm/msm/gpu: add support for ocmem interconnect path")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 0527e85184e1..e23641a5ec84 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -1003,22 +1003,23 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
if (ret)
return ret;
- /* Check for an interconnect path for the bus */
- gpu->icc_path = of_icc_get(dev, "gfx-mem");
- if (!gpu->icc_path) {
- /*
-* Keep compatbility with device trees that don't have an
-* interconnect-names property.
-*/
+ /*
+* The legacy case, before "interconnect-names", only has a
+* single interconnect path which is equivalent to "gfx-mem"
+*/
+ if (!of_find_property(dev->of_node, "interconnect-names", NULL)) {
gpu->icc_path = of_icc_get(dev, NULL);
+ } else {
+ gpu->icc_path = of_icc_get(dev, "gfx-mem");
+ gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
}
+
if (IS_ERR(gpu->icc_path)) {
ret = PTR_ERR(gpu->icc_path);
gpu->icc_path = NULL;
return ret;
}
- gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
if (IS_ERR(gpu->ocmem_icc_path)) {
ret = PTR_ERR(gpu->ocmem_icc_path);
gpu->ocmem_icc_path = NULL;
@@ -1026,6 +1027,7 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
if (ret != -ENODATA)
return ret;
}
+
return 0;
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 208573] Black screen on boot if two displays plugged in with NAVI 10
https://bugzilla.kernel.org/show_bug.cgi?id=208573 --- Comment #2 from Thomas Langkamp (thomas.langk...@medicalschool-hamburg.de) --- Created attachment 290305 --> https://bugzilla.kernel.org/attachment.cgi?id=290305=edit dmesg just after plugging in second display -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 208573] Black screen on boot if two displays plugged in with NAVI 10
https://bugzilla.kernel.org/show_bug.cgi?id=208573 --- Comment #3 from Thomas Langkamp (thomas.langk...@medicalschool-hamburg.de) --- Created attachment 290307 --> https://bugzilla.kernel.org/attachment.cgi?id=290307=edit xorg.log just after connecting second display -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 208413] amdgpu driver crash
https://bugzilla.kernel.org/show_bug.cgi?id=208413 ghutzr...@gmail.com changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |PATCH_ALREADY_AVAILABLE -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 208573] Black screen on boot if two displays plugged in with NAVI 10
https://bugzilla.kernel.org/show_bug.cgi?id=208573 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #1 from Alex Deucher (alexdeuc...@gmail.com) --- Please attach your full dmesg output and xorg log (if using X). -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names
On Wed, Jul 15, 2020 at 11:37 AM Jonathan Marek wrote:
>
> On 7/15/20 2:29 PM, Rob Clark wrote:
> > From: Rob Clark
> >
> > If there is no interconnect-names, but there is an interconnects
> > property, then of_icc_get(dev, "gfx-mem"); would return an error
> > rather than NULL.
> >
> > Also, if there is no interconnect-names property, there will never
> > be a ocmem path. But of_icc_get(dev, "ocmem") would return -EINVAL
> > instead of -ENODATA. Just don't bother trying in this case.
> >
> > Fixes: 8e29fb37b301 ("drm/msm: handle for EPROBE_DEFER for of_icc_get")
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 0527e85184e1..c4ac998b90c8 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -979,6 +979,7 @@ int adreno_gpu_init(struct drm_device *drm, struct
> > platform_device *pdev,
> > struct adreno_platform_config *config = dev->platform_data;
> > struct msm_gpu_config adreno_gpu_config = { 0 };
> > struct msm_gpu *gpu = _gpu->base;
> > + bool has_interconnect_names = true;
> > int ret;
> >
> > adreno_gpu->funcs = funcs;
> > @@ -1005,12 +1006,13 @@ int adreno_gpu_init(struct drm_device *drm, struct
> > platform_device *pdev,
> >
> > /* Check for an interconnect path for the bus */
> > gpu->icc_path = of_icc_get(dev, "gfx-mem");
> > - if (!gpu->icc_path) {
> > + if (IS_ERR_OR_NULL(gpu->icc_path)) {
> > /*
> >* Keep compatbility with device trees that don't have an
> >* interconnect-names property.
> >*/
> > gpu->icc_path = of_icc_get(dev, NULL);
>
> This is misleading because if it gets a EPROBE_DEFER error (or any other
> error), it will hit this path. Maybe there's a specific error you can
> check for instead to identify the "no-interconnect-names" case?
good point, we should probably instead just explicitly check with
of_find_property("interconnect-names")
fwiw, of_icc_get() returns:
- NULL if icc disabled, or no "interconnects" property
- -EINVAL if name!=NULL and no "interconnect-names"
- and looks like -ENODATA if name!=NULL but no match in
interconnect-names
The specific error returns aren't really called out in the API comment
block, so not really sure how much we should rely on them not being
implementation details.
BR,
-R
> Also don't think its a good idea to be calling of_icc_get(dev, NULL)
> again when there's a EPROBE_DEFER, the interconnect driver could come up
> between the two calls
>
> > + has_interconnect_names = false;
> > }
> > if (IS_ERR(gpu->icc_path)) {
> > ret = PTR_ERR(gpu->icc_path);
> > @@ -1018,7 +1020,9 @@ int adreno_gpu_init(struct drm_device *drm, struct
> > platform_device *pdev,
> > return ret;
> > }
> >
> > - gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
> > + if (has_interconnect_names)
> > + gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
> > +
> > if (IS_ERR(gpu->ocmem_icc_path)) {
> > ret = PTR_ERR(gpu->ocmem_icc_path);
> > gpu->ocmem_icc_path = NULL;
> >
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver
Hi Sam and Daniel, Thank you very much for reviewing the code. I will squish all the commits and send version 3, so it will be easier for you to review. Anitha > -Original Message- > From: Sam Ravnborg > Sent: Wednesday, July 15, 2020 10:07 AM > To: Daniel Vetter > Cc: Chrisanthus, Anitha ; Vetter, Daniel > ; intel-...@lists.freedesktop.org; Dea, Edmund J > ; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver > > On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > > Hi Anitha > > > > On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote: > > > This is a new DRM driver for Intel's KeemBay SOC. > > > The SoC couples an ARM Cortex A53 CPU with an Intel > > > Movidius VPU. > > > > > > This driver is tested with the KMB EVM board which is the refernce baord > > > for Keem Bay SOC. The SOC's display pipeline is as follows > > > > > > +--++-++---+ > > > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > > > +--++-++---+ > > > > > > LCD controller and Mipi DSI transmitter are part of the SOC and > > > mipi to HDMI converter is ADV7535 for KMB EVM board. > > > > > > The DRM driver is a basic KMS atomic modesetting display driver and > > > has no 2D or 3D graphics.It calls into the ADV bridge driver at > > > the connector level. > > > > > > Only 1080p resolution and single plane is supported at this time. > > > The usecase is for debugging video and camera outputs. > > > > > > Device tree patches are under review here > > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1- > daniele.alessandre...@linux.intel.com/T/ > > > > Cool, new driver, thanks a lot for submitting. > > > > > Changes since v1: > > > - Removed redundant license text, updated license > > > - Rearranged include blocks > > > - renamed global vars and removed extern in c > > > - Used upclassing for dev_private > > > - Used drm_dev_init in drm device create (will be updated to use > > > devm_drm_dev_alloc() in a separate patch later as kmb driver is > > > currently > > > developed on 5.4 kernel) > > > > drm moves fairly quickly, please develop the upstream submission on top of > > linux-next or similar. We constantly add new helpers to simplify drivers, > > and we expect new driver submissions to be up to date with all that. > Seconded! > > > > > Another thing: From your description it sounds like it's a very simple > > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > > Is the driver already using simple display pipeline helpers? I think that > > would be an ideal fit and probably greatly simplifies the code. > > > > > - minor cleanups > > > > The patch series looks like it contains the entire development history, or > > at least large chunks of it. That's useful for you, but for upstreaming > > the main focues (especially for smaller drivers) is whether your driver > > uses all the available helpers and integrations correctly. And for that > > it's much easier if the history is cleaned up, and all intermediate steps > > removed. > And also agree on this point. > The submission could be split up in a few patches where the split is > file based. So only with the latest patch, containing Makefile + > Kconfig,the driver i buildable. > This would ease review as one looses focus when trying to review 1000+ > lines in one mail. > > You will loose some of the internal history - but if important keep > relevant parts in sensible comments. > > Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 208573] New: Black screen on boot if two displays plugged in with NAVI 10
https://bugzilla.kernel.org/show_bug.cgi?id=208573 Bug ID: 208573 Summary: Black screen on boot if two displays plugged in with NAVI 10 Product: Drivers Version: 2.5 Kernel Version: 5.7.8-6-MANJARO Hardware: x86-64 OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Video(DRI - non Intel) Assignee: drivers_video-...@kernel-bugs.osdl.org Reporter: thomas.langk...@medicalschool-hamburg.de Regression: No With Radeon R9 Fury X and 2 4k displays @60hz, one LG TV@HDMI, one iiyama@DP => everything fine With new Radeon 5700 from Asus => black screen after grub menu and after some time the gpu-fans spin up a bit and the mainboard debug LED starts flashing. Then reset-button is the only option. With new Radeon 5700 XT from Gigabyte => same So I can already rule out a hardware fault. What helps: - Starting a recovery boot entry in grub - OR disconecting one display before reboot Both 5700 cards work always fine with only one display connected. If I connect the second display later, when I am already on desktop environment - also everything works. Other things I tried - changing to 30 hz then reboot - kernels from 5.4 onwards - different ports on the gpu - DP to HDMI-adapter sudo dmesg | grep amdgpu [sudo] Passwort für tom: [0.00] Command line: BOOT_IMAGE=/boot/vmlinuz-5.7-x86_64 root=UUID=74886ca0-4a71-4692-aa0c-b2a539ec6b48 rw quiet loglevel=3 rd.systemd.show_status=auto rd.udev.log_priority=3 amdgpu.ppfeaturemask=0xfffd7fff resume=LABEL=swap libahci.ignore_sss=1 [0.00] Kernel command line: BOOT_IMAGE=/boot/vmlinuz-5.7-x86_64 root=UUID=74886ca0-4a71-4692-aa0c-b2a539ec6b48 rw quiet loglevel=3 rd.systemd.show_status=auto rd.udev.log_priority=3 amdgpu.ppfeaturemask=0xfffd7fff resume=LABEL=swap libahci.ignore_sss=1 [2.597324] [drm] amdgpu kernel modesetting enabled. [2.597526] fb0: switching to amdgpudrmfb from EFI VGA [2.597631] amdgpu :0d:00.0: vgaarb: deactivate vga console [2.597664] amdgpu :0d:00.0: enabling device (0006 -> 0007) [2.615476] amdgpu :0d:00.0: VRAM: 8176M 0x0080 - 0x0081FEFF (8176M used) [2.615477] amdgpu :0d:00.0: GART: 512M 0x - 0x1FFF [2.615580] [drm] amdgpu: 8176M of VRAM memory ready [2.615583] [drm] amdgpu: 8176M of GTT memory ready. [3.433349] amdgpu :0d:00.0: RAS: optional ras ta ucode is not available [3.528083] snd_hda_intel :0d:00.1: bound :0d:00.0 (ops amdgpu_dm_audio_component_bind_ops [amdgpu]) [3.628396] fbcon: amdgpudrmfb (fb0) is primary device [3.739398] amdgpu :0d:00.0: fb0: amdgpudrmfb frame buffer device [3.783482] amdgpu :0d:00.0: ring gfx_0.0.0 uses VM inv eng 0 on hub 0 [3.783485] amdgpu :0d:00.0: ring comp_1.0.0 uses VM inv eng 1 on hub 0 [3.783487] amdgpu :0d:00.0: ring comp_1.1.0 uses VM inv eng 4 on hub 0 [3.783489] amdgpu :0d:00.0: ring comp_1.2.0 uses VM inv eng 5 on hub 0 [3.783491] amdgpu :0d:00.0: ring comp_1.3.0 uses VM inv eng 6 on hub 0 [3.783492] amdgpu :0d:00.0: ring comp_1.0.1 uses VM inv eng 7 on hub 0 [3.783494] amdgpu :0d:00.0: ring comp_1.1.1 uses VM inv eng 8 on hub 0 [3.783496] amdgpu :0d:00.0: ring comp_1.2.1 uses VM inv eng 9 on hub 0 [3.783498] amdgpu :0d:00.0: ring comp_1.3.1 uses VM inv eng 10 on hub 0 [3.783499] amdgpu :0d:00.0: ring kiq_2.1.0 uses VM inv eng 11 on hub 0 [3.783501] amdgpu :0d:00.0: ring sdma0 uses VM inv eng 12 on hub 0 [3.783503] amdgpu :0d:00.0: ring sdma1 uses VM inv eng 13 on hub 0 [3.783505] amdgpu :0d:00.0: ring vcn_dec uses VM inv eng 0 on hub 1 [3.783506] amdgpu :0d:00.0: ring vcn_enc0 uses VM inv eng 1 on hub 1 [3.783508] amdgpu :0d:00.0: ring vcn_enc1 uses VM inv eng 4 on hub 1 [3.783510] amdgpu :0d:00.0: ring jpeg_dec uses VM inv eng 5 on hub 1 [3.784426] [drm] Initialized amdgpu 3.37.0 20150101 for :0d:00.0 on minor 0 [ 2050.217010] amdgpu :0d:00.0: RAS: optional ras ta ucode is not available [ 2050.416499] [drm:dpcd_set_source_specific_data [amdgpu]] *ERROR* Error in DP aux read transaction, not writing source specific data [ 2050.925049] [drm:retrieve_link_cap [amdgpu]] *ERROR* retrieve_link_cap: Read dpcd data failed. [ 2050.934845] amdgpu :0d:00.0: ring gfx_0.0.0 uses VM inv eng 0 on hub 0 [ 2050.934846] amdgpu :0d:00.0: ring comp_1.0.0 uses VM inv eng 1 on hub 0 [ 2050.934846] amdgpu :0d:00.0: ring comp_1.1.0 uses VM inv eng 4 on hub 0 [ 2050.934847] amdgpu :0d:00.0: ring comp_1.2.0 uses VM inv eng 5 on hub 0 [ 2050.934847] amdgpu :0d:00.0: ring comp_1.3.0 uses VM inv eng 6 on hub 0 [ 2050.934848] amdgpu :0d:00.0: ring comp_1.0.1 uses VM inv eng 7 on hub 0 [ 2050.934849] amdgpu :0d:00.0: ring
[PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names
From: Rob Clark
If there is no interconnect-names, but there is an interconnects
property, then of_icc_get(dev, "gfx-mem"); would return an error
rather than NULL.
Also, if there is no interconnect-names property, there will never
be a ocmem path. But of_icc_get(dev, "ocmem") would return -EINVAL
instead of -ENODATA. Just don't bother trying in this case.
Fixes: 8e29fb37b301 ("drm/msm: handle for EPROBE_DEFER for of_icc_get")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 0527e85184e1..c4ac998b90c8 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -979,6 +979,7 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
struct adreno_platform_config *config = dev->platform_data;
struct msm_gpu_config adreno_gpu_config = { 0 };
struct msm_gpu *gpu = _gpu->base;
+ bool has_interconnect_names = true;
int ret;
adreno_gpu->funcs = funcs;
@@ -1005,12 +1006,13 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
/* Check for an interconnect path for the bus */
gpu->icc_path = of_icc_get(dev, "gfx-mem");
- if (!gpu->icc_path) {
+ if (IS_ERR_OR_NULL(gpu->icc_path)) {
/*
* Keep compatbility with device trees that don't have an
* interconnect-names property.
*/
gpu->icc_path = of_icc_get(dev, NULL);
+ has_interconnect_names = false;
}
if (IS_ERR(gpu->icc_path)) {
ret = PTR_ERR(gpu->icc_path);
@@ -1018,7 +1020,9 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
return ret;
}
- gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
+ if (has_interconnect_names)
+ gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
+
if (IS_ERR(gpu->ocmem_icc_path)) {
ret = PTR_ERR(gpu->ocmem_icc_path);
gpu->ocmem_icc_path = NULL;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 5/6] arm64: dts: qcom: sc7180: Add interconnects property for GPU
On Mon, Jul 13, 2020 at 5:42 AM Akhil P Oommen wrote:
>
> From: Sharat Masetty
>
> This patch adds the interconnects property to the GPU node. This enables
> the GPU->DDR path bandwidth voting.
>
> Signed-off-by: Sharat Masetty
> Signed-off-by: Akhil P Oommen
> ---
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 31b9217..a567297 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1470,6 +1470,8 @@
> operating-points-v2 = <_opp_table>;
> qcom,gmu = <>;
>
> + interconnects = <_noc MASTER_GFX3D _virt
> SLAVE_EBI1>;
I suppose this and the 845 dts patch should have:
interconnect-names = "gfx-mem";
(OTOH not having it was a good way to notice a bug in the driver
handling the legacy case without having 'interconnect-names')
BR,
-R
> +
> gpu_opp_table: opp-table {
> compatible = "operating-points-v2";
>
> --
> 2.7.4
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] dt-bindings: add binding for Ilitek ili9488 based display panels
On Sat, 13 Jun 2020 19:37:03 +0530, Kamlesh Gurudasani wrote: > This adds binding for ilitek,ili9488 based display panels > > Signed-off-by: Kamlesh Gurudasani > --- > .../bindings/display/ilitek,ili9488.yaml | 71 > ++ > 1 file changed, 71 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/ilitek,ili9488.yaml > Reviewed-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] dt-bindings: add vendor prefix for EastRising Technology Co., Ltd
On Sat, 13 Jun 2020 19:36:46 +0530, Kamlesh Gurudasani wrote: > Add vendor prefix for display manufacturer company EastRising > Technology Co.,Ltd > > [1]https://eastrising.en.ec21.com/ > > Signed-off-by: Kamlesh Gurudasani > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH 0/9] drm/msm: Avoid possible infinite probe deferral and speed booting
On Tue, Jul 14, 2020 at 4:54 PM Bjorn Andersson wrote: > > On Tue 14 Jul 15:13 PDT 2020, Rob Herring wrote: > > > On Tue, Jul 14, 2020 at 10:33 AM Jeffrey Hugo > > wrote: > > > > > > On Mon, Jul 13, 2020 at 5:50 PM Doug Anderson > > > wrote: > > > > > > > > Hi, > > > > > > > > On Mon, Jul 13, 2020 at 1:25 PM Rob Herring wrote: > > > > > > > > > > On Mon, Jul 13, 2020 at 9:08 AM Doug Anderson > > > > > wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Mon, Jul 13, 2020 at 7:11 AM Rob Herring > > > > > > wrote: > > > > > > > > > > > > > > On Fri, Jul 10, 2020 at 5:02 PM Douglas Anderson > > > > > > > wrote: > > > > > > > > > > > > > > > > I found that if I ever had a little mistake in my kernel config, > > > > > > > > or device tree, or graphics driver that my system would sit in > > > > > > > > a loop > > > > > > > > at bootup trying again and again and again. An example log was: > > > > > > > > > > > > > > Why do we care about optimizing the error case? > > > > > > > > > > > > It actually results in a _fully_ infinite loop. That is: if > > > > > > anything > > > > > > small causes a component of DRM to fail to probe then the whole > > > > > > system > > > > > > doesn't boot because it just loops trying to probe over and over > > > > > > again. The messages I put in the commit message are printed over > > > > > > and > > > > > > over and over again. > > > > > > > > > > Sounds like a bug as that's not what should happen. > > > > > > > > > > If you defer during boot (initcalls), then you'll be on the deferred > > > > > list until late_initcall and everything is retried. After > > > > > late_initcall, only devices getting added should trigger probing. But > > > > > maybe the adding and then removing a device is causing a re-trigger. > > > > > > > > Right, I'm nearly certain that the adding and then removing is causing > > > > a re-trigger. I believe the loop would happen for any case where we > > > > have a probe function that: > > > > > > > > 1. Adds devices. > > > > 2. After adding devices it decides that it needs to defer. > > > > 3. Removes the devices it added. > > > > 4. Return -EPROBE_DEFER from its probe function. > > > > > > > > Specifically from what I know about how -EPROBE_DEFER works I'm not > > > > sure how it wouldn't cause an infinite loop in that case. > > > > > > > > Perhaps the missing part of my explanation, though, is why it never > > > > gets out of this infinite loop. In my case I purposely made the > > > > bridge chip "ti-sn65dsi86.c" return an error (-EINVAL) in its probe > > > > every time. Obviously I wasn't going to get a display up like this, > > > > but I just wanted to not loop forever at bootup. I tracked down > > > > exactly why we get an - EPROBE_DEFER over and over in this case. > > > > > > > > You can see it in msm_dsi_host_register(). If some components haven't > > > > shown up when that function runs it will _always_ return > > > > -EPROBE_DEFER. > > > > > > > > In my case, since I caused the bridge to fail to probe, those > > > > components will _never_ show up. That means that > > > > msm_dsi_host_register() will _always_ return -EPROBE_DEFER. > > > > > > > > I haven't dug through all the DRM code enough, but it doesn't > > > > necessarily seem like the wrong behavior. If the bridge driver or a > > > > panel was a module then (presumably) they could show up later and so > > > > it should be OK for it to defer, right? > > > > > > > > So with all that, it doesn't really feel like this is a bug so much as > > > > it's an unsupported use case. The current deferral logic simply can't > > > > handle the case we're throwing at it. You cannot return -EPROBE_DEFER > > > > if your probe function adds devices each time through the probe > > > > function. > > > > > > > > Assuming all the above makes sense, that means we're stuck with: > > > > > > > > a) This patch series, which makes us not add devices. > > > > > > > > b) Some other patch series which rearchitects the MSM graphics stack > > > > to not return -EPROBE_DEFER in this case. > > > > > > This isn't a MSM specific issue. This is an issue with how the DSI > > > interface works, and how software is structured in Linux. I would > > > expect that pretty much any DSI host in the kernel would have some > > > version of this issue. > > > > > > The problem is that DSI is not "hot pluggable", so to give the DRM > > > stack the info it needs, we need both the DSI controller (aka the MSM > > > graphics stack in your case), and the thing it connects to (in your > > > case, the TI bridge, normally the actual panel) because the DRM stack > > > expects that if init completes, it has certain information > > > (resolution, etc), and some of that information is in the DSI > > > controller, and some of it is on the DSI device. > > > > Ah yes, DRM's lack of hot-plug and discrete component support... Is > > that not improved with some of the bridge rework? > > > > Anyways, given there is a child
[PULL] drm-misc-fixes
Hi Dave and Daniel, here is the PR for the current drm-misc-fixes. The aspeed fix is only about dmesg noise. The dmabuf locking appears to be a real bug. Best regards Thomas drm-misc-fixes-2020-07-15: * aspeed: setup fbdev console after registering device; avoids warning and stacktrace in dmesg log * dmabuf: protect dmabuf->name with a spinlock; avoids sleeping in atomic context The following changes since commit 00debf8109e5fad3db31375be2a3c515e1461b4a: drm/hisilicon/hibmc: Move drm_fbdev_generic_setup() down to avoid the splat (2020-07-08 09:08:22 +) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2020-07-15 for you to fetch changes up to 6348dd291e3653534a9e28e6917569bc9967b35b: dmabuf: use spinlock to access dmabuf->name (2020-07-10 15:39:29 +0530) * aspeed: setup fbdev console after registering device; avoids warning and stacktrace in dmesg log * dmabuf: protect dmabuf->name with a spinlock; avoids sleeping in atomic context Charan Teja Kalla (1): dmabuf: use spinlock to access dmabuf->name Guenter Roeck (1): drm/aspeed: Call drm_fbdev_generic_setup after drm_dev_register drivers/dma-buf/dma-buf.c | 11 +++ drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 3 +-- include/linux/dma-buf.h | 1 + 3 files changed, 9 insertions(+), 6 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 207383] [Regression] 5.7 amdgpu/polaris11 gpf: amdgpu_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=207383 --- Comment #64 from Anthony Ruhier (anthony.ruh...@gmail.com) --- (In reply to Duncan from comment #63) > (In reply to Duncan from comment #62) > > I've applied the three 320-and-followups revert-patches to > > v5.8-rc5-8-g0dc589da8 and just did the rebuild with them applied. > > Now to reboot to it and see if it still has our bug. > > NB: The 3202fa62f followups are cbfc35a48 and 89b83f282. That should let > anyone else with git and kernel building skills try reverting the three. > > Still too early (by days) to call it nailed down as I've had it take 2-3 > days to trigger, but no gfx freeze here yet on that v5.8-rc5+ with > 320-and-followups reverted so far, despite playing 4k video to try to > trigger it as it has previously on affected kernels. I'll be trying update > builds (gentoo) later today or tomorrow, another previous trigger, so we'll > see how it goes. > > But initial results are good enough to let others know that may want to try > it... Thanks a lot, I'm also trying on my side. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver
On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > Hi Anitha > > On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote: > > This is a new DRM driver for Intel's KeemBay SOC. > > The SoC couples an ARM Cortex A53 CPU with an Intel > > Movidius VPU. > > > > This driver is tested with the KMB EVM board which is the refernce baord > > for Keem Bay SOC. The SOC's display pipeline is as follows > > > > +--++-++---+ > > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > > +--++-++---+ > > > > LCD controller and Mipi DSI transmitter are part of the SOC and > > mipi to HDMI converter is ADV7535 for KMB EVM board. > > > > The DRM driver is a basic KMS atomic modesetting display driver and > > has no 2D or 3D graphics.It calls into the ADV bridge driver at > > the connector level. > > > > Only 1080p resolution and single plane is supported at this time. > > The usecase is for debugging video and camera outputs. > > > > Device tree patches are under review here > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandre...@linux.intel.com/T/ > > Cool, new driver, thanks a lot for submitting. > > > Changes since v1: > > - Removed redundant license text, updated license > > - Rearranged include blocks > > - renamed global vars and removed extern in c > > - Used upclassing for dev_private > > - Used drm_dev_init in drm device create (will be updated to use > > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > > developed on 5.4 kernel) > > drm moves fairly quickly, please develop the upstream submission on top of > linux-next or similar. We constantly add new helpers to simplify drivers, > and we expect new driver submissions to be up to date with all that. Seconded! > > Another thing: From your description it sounds like it's a very simple > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > Is the driver already using simple display pipeline helpers? I think that > would be an ideal fit and probably greatly simplifies the code. > > > - minor cleanups > > The patch series looks like it contains the entire development history, or > at least large chunks of it. That's useful for you, but for upstreaming > the main focues (especially for smaller drivers) is whether your driver > uses all the available helpers and integrations correctly. And for that > it's much easier if the history is cleaned up, and all intermediate steps > removed. And also agree on this point. The submission could be split up in a few patches where the split is file based. So only with the latest patch, containing Makefile + Kconfig,the driver i buildable. This would ease review as one looses focus when trying to review 1000+ lines in one mail. You will loose some of the internal history - but if important keep relevant parts in sensible comments. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 00/59] Add support for KeemBay DRM driver
Hi Anitha. On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote: > This is a new DRM driver for Intel's KeemBay SOC. > The SoC couples an ARM Cortex A53 CPU with an Intel > Movidius VPU. > > This driver is tested with the KMB EVM board which is the refernce baord > for Keem Bay SOC. The SOC's display pipeline is as follows > > +--++-++---+ > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > +--++-++---+ > > LCD controller and Mipi DSI transmitter are part of the SOC and > mipi to HDMI converter is ADV7535 for KMB EVM board. > > The DRM driver is a basic KMS atomic modesetting display driver and > has no 2D or 3D graphics.It calls into the ADV bridge driver at > the connector level. > > Only 1080p resolution and single plane is supported at this time. > The usecase is for debugging video and camera outputs. > > Device tree patches are under review here > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandre...@linux.intel.com/T/ > > Changes since v1: > - Removed redundant license text, updated license > - Rearranged include blocks > - renamed global vars and removed extern in c > - Used upclassing for dev_private > - Used drm_dev_init in drm device create (will be updated to use > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > developed on 5.4 kernel) > - minor cleanups Could you please include a TODO or something. It is not obvious if some points from last review are pending or they were skipped. From a quick look maybe half of the poitns were addressed which is good. But some points are yet to be done. Sam > > Anitha Chrisanthus (52): > drm/kmb: Add support for KeemBay Display > drm/kmb: Added id to kmb_plane > drm/kmb: Set correct values in the LAYERn_CFG register > drm/kmb: Use biwise operators for register definitions > drm/kmb: Updated kmb_plane_atomic_check > drm/kmb: Initial check-in for Mipi DSI > drm/kmb: Set OUT_FORMAT_CFG register > drm/kmb: Added mipi_dsi_host initialization > drm/kmb: Part 1 of Mipi Tx Initialization > drm/kmb: Part 2 of Mipi Tx Initialization > drm/kmb: Use correct mmio offset from data book > drm/kmb: Part3 of Mipi Tx initialization > drm/kmb: Part4 of Mipi Tx Initialization > drm/kmb: Correct address offsets for mipi registers > drm/kmb: Part5 of Mipi Tx Intitialization > drm/kmb: Part6 of Mipi Tx Initialization > drm/kmb: Part7 of Mipi Tx Initialization > drm/kmb: Part8 of Mipi Tx Initialization > drm/kmb: Added ioremap/iounmap for register access > drm/kmb: Register IRQ for LCD > drm/kmb: IRQ handlers for LCD and mipi dsi > drm/kmb: Set hardcoded values to LCD_VSYNC_START > drm/kmb: Additional register programming to update_plane > drm/kmb: Add ADV7535 bridge > drm/kmb: Display clock enable/disable > drm/kmb: rebase to newer kernel version > drm/kmb: minor name change to match device tree > drm/kmb: Changed MMIO size > drm/kmb: Defer Probe > drm/kmb: call bridge init in the very beginning > drm/kmb: Enable MSS_CAM_CLK_CTRL for LCD and MIPI > drm/kmb: Set MSS_CAM_RSTN_CTRL along with enable > drm/kmb: Mipi DPHY initialization changes > drm/kmb: Fixed driver unload > drm/kmb: Added LCD_TEST config > drm/kmb: Changes for LCD to Mipi > drm/kmb: Update LCD programming to match MIPI > drm/kmb: Changed name of driver to kmb-drm > drm/kmb: Mipi settings from input timings > drm/kmb: Enable LCD interrupts > drm/kmb: Enable LCD interrupts during modeset > drm/kmb: Don’t inadvertantly disable LCD controller > drm/kmb: SWAP R and B LCD Layer order > drm/kmb: Disable ping pong mode > drm/kmb: Do the layer initializations only once > drm/kmb: disable the LCD layer in EOF irq handler > drm/kmb: Initialize uninitialized variables > drm/kmb: Added useful messages in LCD ISR > kmb/drm: Prune unsupported modes > drm/kmb: workaround for dma undeflow issue > drm/kmb: Get System Clock from SCMI > drm/kmb: work around for planar formats > > Edmund Dea (7): > drm/kmb: Cleanup probe functions > drm/kmb: Revert dsi_host back to a static variable > drm/kmb: Initialize clocks for clk_msscam, clk_mipi_ecfg, & > clk_mipi_cfg. > drm/kmb: Remove declaration of irq_lcd/irq_mipi > drm/kmb: Enable MIPI TX HS Test Pattern Generation > drm/kmb: Write to LCD_LAYERn_CFG only once > drm/kmb: Cleaned up code > > drivers/gpu/drm/Kconfig |2 + > drivers/gpu/drm/Makefile|1 + > drivers/gpu/drm/kmb/Kconfig | 12 + > drivers/gpu/drm/kmb/Makefile|2 + > drivers/gpu/drm/kmb/kmb_crtc.c | 226 + > drivers/gpu/drm/kmb/kmb_crtc.h | 41 + > drivers/gpu/drm/kmb/kmb_drv.c | 809 > drivers/gpu/drm/kmb/kmb_drv.h | 176 > drivers/gpu/drm/kmb/kmb_dsi.c | 1927 > +++ >
[Bug 207383] [Regression] 5.7 amdgpu/polaris11 gpf: amdgpu_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=207383 --- Comment #63 from Duncan (1i5t5.dun...@cox.net) --- (In reply to Duncan from comment #62) > I've applied the three 320-and-followups revert-patches to > v5.8-rc5-8-g0dc589da8 and just did the rebuild with them applied. > Now to reboot to it and see if it still has our bug. NB: The 3202fa62f followups are cbfc35a48 and 89b83f282. That should let anyone else with git and kernel building skills try reverting the three. Still too early (by days) to call it nailed down as I've had it take 2-3 days to trigger, but no gfx freeze here yet on that v5.8-rc5+ with 320-and-followups reverted so far, despite playing 4k video to try to trigger it as it has previously on affected kernels. I'll be trying update builds (gentoo) later today or tomorrow, another previous trigger, so we'll see how it goes. But initial results are good enough to let others know that may want to try it... -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[git pull] vmwgfx-fixes-5.8
Dave, Daniel, vmwgfx-fixes pull for 5.8. Just one fix for now, but it's a really important one, causing black screens in VMs (sometimes on boot), hence marking it for stable. The following changes since commit 1f054fd26e29784d373c3d29c348ee48f1c41fb2: drm/vmwgfx: fix update of display surface when resolution changes (2020-07-14 04:05:52 +0200) are available in the Git repository at: git://people.freedesktop.org/~sroland/linux vmwgfx-fixes-5.8 for you to fetch changes up to 1f054fd26e29784d373c3d29c348ee48f1c41fb2: drm/vmwgfx: fix update of display surface when resolution changes (2020-07-14 04:05:52 +0200) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 --- Comment #30 from Cyrax (ev...@hotmail.com) --- The patch in https://bugzilla.kernel.org/show_bug.cgi?id=207979 works beatifully. 19 days heavy usage without system crash on patched 5.7.6 kernel. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail
On Wed, Jul 15, 2020 at 5:57 PM Melissa Wen wrote: > > On 07/15, Sidong Yang wrote: > > On Wed, Jul 15, 2020 at 10:17:56AM +0200, Daniel Vetter wrote: > > > On Tue, Jul 14, 2020 at 9:01 PM Melissa Wen wrote: > > > > > > > > On 07/14, Daniel Vetter wrote: > > > > > On Tue, Jul 14, 2020 at 07:39:42AM -0300, Melissa Wen wrote: > > > > > > On Tue, Jul 14, 2020 at 7:20 AM Melissa Wen > > > > > > wrote: > > > > > > > > > > > > > > On 07/13, Daniel Vetter wrote: > > > > > > > > On Fri, Jul 10, 2020 at 02:05:33PM -0300, Melissa Wen wrote: > > > > > > > > > On 07/02, Daniel Vetter wrote: > > > > > > > > > > On Wed, Jul 01, 2020 at 03:31:34PM +, Sidong Yang wrote: > > > > > > > > > > > there is an error when igt test is run continuously. > > > > > > > > > > > vkms_atomic_commit_tail() > > > > > > > > > > > need to call drm_atomic_helper_wait_for_vblanks() for > > > > > > > > > > > give up ownership of > > > > > > > > > > > vblank events. without this code, next atomic commit will > > > > > > > > > > > not enable vblank > > > > > > > > > > > and raise timeout error. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sidong Yang > > > > > > > > > > > --- > > > > > > > > > > > drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ > > > > > > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > > b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > > index 1e8b2169d834..10b9be67a068 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > > @@ -93,6 +93,8 @@ static void > > > > > > > > > > > vkms_atomic_commit_tail(struct drm_atomic_state > > > > > > > > > > > *old_state) > > > > > > > > > > > flush_work(_state->composer_work); > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > + drm_atomic_helper_wait_for_vblanks(dev, > > > > > > > > > > > old_state); > > > > > > > > > > > > > > > > > > > > Uh, we have a wait_for_flip_done right above, which should > > > > > > > > > > be doing > > > > > > > > > > exactly the same, but more precisely: Instead of just > > > > > > > > > > waiting for any > > > > > > > > > > vblank to happen, we wait for exactly the vblank > > > > > > > > > > corresponding to this > > > > > > > > > > atomic commit. So no races possible. If this is papering > > > > > > > > > > over some issue, > > > > > > > > > > then I think more debugging is needed. > > > > > > > > > > > > > > > > > > > > What exactly is going wrong here for you? > > > > > > > > > > > > > > > > > > Hi Daniel and Sidong, > > > > > > > > > > > > > > > > > > I noticed a similar issue when running the IGT test > > > > > > > > > kms_cursor_crc. For > > > > > > > > > example, a subtest that passes on the first run > > > > > > > > > (alpha-opaque) fails on > > > > > > > > > the second due to a kind of busy waiting in subtest > > > > > > > > > preparation (the > > > > > > > > > subtest fails before actually running). > > > > > > > > > > > > > > > > > > In addition, in the same test, the dpms subtest started to > > > > > > > > > fail since > > > > > > > > > the commit that change from wait_for_vblanks to > > > > > > > > > wait_for_flip_done. By > > > > > > > > > reverting this commit, the dpms subtest passes again and the > > > > > > > > > sequential > > > > > > > > > subtests return to normal. > > > > > > > > > > > > > > > > > > I am trying to figure out what's missing from using flip_done > > > > > > > > > op on > > > > > > > > > vkms, since I am also interested in solving this problem and I > > > > > > > > > understand that the change for flip_done has been discussed > > > > > > > > > in the past. > > > > > > > > > > > > > > > > > > Do you have any idea? > > > > > > > > > > > > > > > > Uh, not at all. This is indeed rather surprising ... > > > > > > > > > > > > > > > > What exactly is the failure mode when running a test the 2nd > > > > > > > > time? Full > > > > > > > > igt logs might give me an idea. But yeah this is kinda > > > > > > > > surprising. > > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > This is the IGT log of the 2nd run of kms_cursor_crc/alpha-opaque: > > > > > > > > > > > > > > IGT-Version: 1.25-NO-GIT (x86_64) (Linux: 5.8.0-rc2-DRM+ x86_64) > > > > > > > Force option used: Using driver vkms > > > > > > > Starting subtest: pipe-A-cursor-alpha-opaque > > > > > > > Timed out: Opening crc fd, and poll for first CRC. > > > > > > > Subtest pipe-A-cursor-alpha-opaque failed. > > > > > > > DEBUG > > > > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: > > > > > > > set_pipe(A) > > > > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: > > > > > > > Selecting pipe A > > > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > > > igt_create_fb_with_bo_size(width=1024, height=768, > > > > > > > format=XR24(0x34325258),
[Bug 206987] [drm] [amdgpu] Whole system crashes when the driver is in mode_support_and_system_configuration
https://bugzilla.kernel.org/show_bug.cgi?id=206987 Cyrax (ev...@hotmail.com) changed: What|Removed |Added Kernel Version|5.7.0 |5.7.6 -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail
On 07/15, Sidong Yang wrote: > On Wed, Jul 15, 2020 at 10:17:56AM +0200, Daniel Vetter wrote: > > On Tue, Jul 14, 2020 at 9:01 PM Melissa Wen wrote: > > > > > > On 07/14, Daniel Vetter wrote: > > > > On Tue, Jul 14, 2020 at 07:39:42AM -0300, Melissa Wen wrote: > > > > > On Tue, Jul 14, 2020 at 7:20 AM Melissa Wen > > > > > wrote: > > > > > > > > > > > > On 07/13, Daniel Vetter wrote: > > > > > > > On Fri, Jul 10, 2020 at 02:05:33PM -0300, Melissa Wen wrote: > > > > > > > > On 07/02, Daniel Vetter wrote: > > > > > > > > > On Wed, Jul 01, 2020 at 03:31:34PM +, Sidong Yang wrote: > > > > > > > > > > there is an error when igt test is run continuously. > > > > > > > > > > vkms_atomic_commit_tail() > > > > > > > > > > need to call drm_atomic_helper_wait_for_vblanks() for give > > > > > > > > > > up ownership of > > > > > > > > > > vblank events. without this code, next atomic commit will > > > > > > > > > > not enable vblank > > > > > > > > > > and raise timeout error. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sidong Yang > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ > > > > > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > index 1e8b2169d834..10b9be67a068 100644 > > > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > > @@ -93,6 +93,8 @@ static void > > > > > > > > > > vkms_atomic_commit_tail(struct drm_atomic_state *old_state) > > > > > > > > > > flush_work(_state->composer_work); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > + drm_atomic_helper_wait_for_vblanks(dev, old_state); > > > > > > > > > > > > > > > > > > Uh, we have a wait_for_flip_done right above, which should be > > > > > > > > > doing > > > > > > > > > exactly the same, but more precisely: Instead of just waiting > > > > > > > > > for any > > > > > > > > > vblank to happen, we wait for exactly the vblank > > > > > > > > > corresponding to this > > > > > > > > > atomic commit. So no races possible. If this is papering over > > > > > > > > > some issue, > > > > > > > > > then I think more debugging is needed. > > > > > > > > > > > > > > > > > > What exactly is going wrong here for you? > > > > > > > > > > > > > > > > Hi Daniel and Sidong, > > > > > > > > > > > > > > > > I noticed a similar issue when running the IGT test > > > > > > > > kms_cursor_crc. For > > > > > > > > example, a subtest that passes on the first run (alpha-opaque) > > > > > > > > fails on > > > > > > > > the second due to a kind of busy waiting in subtest preparation > > > > > > > > (the > > > > > > > > subtest fails before actually running). > > > > > > > > > > > > > > > > In addition, in the same test, the dpms subtest started to fail > > > > > > > > since > > > > > > > > the commit that change from wait_for_vblanks to > > > > > > > > wait_for_flip_done. By > > > > > > > > reverting this commit, the dpms subtest passes again and the > > > > > > > > sequential > > > > > > > > subtests return to normal. > > > > > > > > > > > > > > > > I am trying to figure out what's missing from using flip_done > > > > > > > > op on > > > > > > > > vkms, since I am also interested in solving this problem and I > > > > > > > > understand that the change for flip_done has been discussed in > > > > > > > > the past. > > > > > > > > > > > > > > > > Do you have any idea? > > > > > > > > > > > > > > Uh, not at all. This is indeed rather surprising ... > > > > > > > > > > > > > > What exactly is the failure mode when running a test the 2nd > > > > > > > time? Full > > > > > > > igt logs might give me an idea. But yeah this is kinda surprising. > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > This is the IGT log of the 2nd run of kms_cursor_crc/alpha-opaque: > > > > > > > > > > > > IGT-Version: 1.25-NO-GIT (x86_64) (Linux: 5.8.0-rc2-DRM+ x86_64) > > > > > > Force option used: Using driver vkms > > > > > > Starting subtest: pipe-A-cursor-alpha-opaque > > > > > > Timed out: Opening crc fd, and poll for first CRC. > > > > > > Subtest pipe-A-cursor-alpha-opaque failed. > > > > > > DEBUG > > > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: set_pipe(A) > > > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: Selecting > > > > > > pipe A > > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > > igt_create_fb_with_bo_size(width=1024, height=768, > > > > > > format=XR24(0x34325258), modifier=0x0, size=0) > > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > > igt_create_fb_with_bo_size(handle=1, pitch=4096) > > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: Test requirement passed: > > > > > > cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS > > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: >
Re: [PATCH v5 0/6] Add support for GPU DDR BW scaling
On Mon, Jul 13, 2020 at 5:41 AM Akhil P Oommen wrote: > > This series adds support for GPU DDR bandwidth scaling and is based on the > bindings from Georgi [1]. This is mostly a rebase of Sharat's patches [2] on > the > tip of msm-next branch. > > Changes from v4: > - Squashed a patch to another one to fix Jonathan's comment > - Add back the pm_runtime_get_if_in_use() check > > Changes from v3: > - Rebased on top of Jonathan's patch which adds support for changing gpu freq > through hfi on newer targets > - As suggested by Rob, left the icc_path intact for pre-a6xx GPUs > > [1] > https://kernel.googlesource.com/pub/scm/linux/kernel/git/vireshk/pm/+log/opp/linux-next/ > [2] https://patchwork.freedesktop.org/series/75291/ > > Sharat Masetty (6): > dt-bindings: drm/msm/gpu: Document gpu opp table > drm: msm: a6xx: send opp instead of a frequency > drm: msm: a6xx: use dev_pm_opp_set_bw to scale DDR > arm64: dts: qcom: SDM845: Enable GPU DDR bw scaling > arm64: dts: qcom: sc7180: Add interconnects property for GPU > arm64: dts: qcom: sc7180: Add opp-peak-kBps to GPU opp I can take the first two into msm-next, the 3rd will need to wait until dev_pm_opp_set_bw() lands Bjorn, I assume you take the last three? BR, -R > > .../devicetree/bindings/display/msm/gpu.txt| 28 ++ > arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++ > arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 ++ > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 108 > - > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +- > drivers/gpu/drm/msm/msm_gpu.c | 3 +- > drivers/gpu/drm/msm/msm_gpu.h | 3 +- > 7 files changed, 112 insertions(+), 50 deletions(-) > > -- > 2.7.4 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver
On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > Hi Anitha > > On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote: > > This is a new DRM driver for Intel's KeemBay SOC. > > The SoC couples an ARM Cortex A53 CPU with an Intel > > Movidius VPU. > > > > This driver is tested with the KMB EVM board which is the refernce baord > > for Keem Bay SOC. The SOC's display pipeline is as follows > > > > +--++-++---+ > > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > > +--++-++---+ > > > > LCD controller and Mipi DSI transmitter are part of the SOC and > > mipi to HDMI converter is ADV7535 for KMB EVM board. > > > > The DRM driver is a basic KMS atomic modesetting display driver and > > has no 2D or 3D graphics.It calls into the ADV bridge driver at > > the connector level. > > > > Only 1080p resolution and single plane is supported at this time. > > The usecase is for debugging video and camera outputs. > > > > Device tree patches are under review here > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandre...@linux.intel.com/T/ > > Cool, new driver, thanks a lot for submitting. > > > Changes since v1: > > - Removed redundant license text, updated license > > - Rearranged include blocks > > - renamed global vars and removed extern in c > > - Used upclassing for dev_private > > - Used drm_dev_init in drm device create (will be updated to use > > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > > developed on 5.4 kernel) > > drm moves fairly quickly, please develop the upstream submission on top of > linux-next or similar. We constantly add new helpers to simplify drivers, > and we expect new driver submissions to be up to date with all that. > > Another thing: From your description it sounds like it's a very simple > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > Is the driver already using simple display pipeline helpers? I think that > would be an ideal fit and probably greatly simplifies the code. > > > - minor cleanups > > The patch series looks like it contains the entire development history, or > at least large chunks of it. That's useful for you, but for upstreaming > the main focues (especially for smaller drivers) is whether your driver > uses all the available helpers and integrations correctly. And for that > it's much easier if the history is cleaned up, and all intermediate steps > removed. > > I think once that's done I can do a quick pass and drop suggestions for > cleanup and stuff like that, and then we should (usually at least) be able > to pull in the driver fairly quickly. > > Another thing to consider is where/how this driver will be maintained. > Preferred option is as part of drm-misc so that we have redudancy and all > that in a fairly big group. Works with commit rights, so maybe check out > some of our docs about that too. > > https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html > > The committer model comes with a full set of scripts and docs to avoid > oopsies in maintainership. Generally works really well. Oh if you have a git branch of this all somewhere I could do a quick high-level pass and see whether there's anything big. -Daniel > > Cheers, Daniel > > > > > > Anitha Chrisanthus (52): > > drm/kmb: Add support for KeemBay Display > > drm/kmb: Added id to kmb_plane > > drm/kmb: Set correct values in the LAYERn_CFG register > > drm/kmb: Use biwise operators for register definitions > > drm/kmb: Updated kmb_plane_atomic_check > > drm/kmb: Initial check-in for Mipi DSI > > drm/kmb: Set OUT_FORMAT_CFG register > > drm/kmb: Added mipi_dsi_host initialization > > drm/kmb: Part 1 of Mipi Tx Initialization > > drm/kmb: Part 2 of Mipi Tx Initialization > > drm/kmb: Use correct mmio offset from data book > > drm/kmb: Part3 of Mipi Tx initialization > > drm/kmb: Part4 of Mipi Tx Initialization > > drm/kmb: Correct address offsets for mipi registers > > drm/kmb: Part5 of Mipi Tx Intitialization > > drm/kmb: Part6 of Mipi Tx Initialization > > drm/kmb: Part7 of Mipi Tx Initialization > > drm/kmb: Part8 of Mipi Tx Initialization > > drm/kmb: Added ioremap/iounmap for register access > > drm/kmb: Register IRQ for LCD > > drm/kmb: IRQ handlers for LCD and mipi dsi > > drm/kmb: Set hardcoded values to LCD_VSYNC_START > > drm/kmb: Additional register programming to update_plane > > drm/kmb: Add ADV7535 bridge > > drm/kmb: Display clock enable/disable > > drm/kmb: rebase to newer kernel version > > drm/kmb: minor name change to match device tree > > drm/kmb: Changed MMIO size > > drm/kmb: Defer Probe > > drm/kmb: call bridge init in the very beginning > > drm/kmb: Enable MSS_CAM_CLK_CTRL for LCD and MIPI > > drm/kmb: Set MSS_CAM_RSTN_CTRL along with enable > > drm/kmb: Mipi
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote:
> On 2020/07/15 20:17, Tetsuo Handa wrote:
> > On 2020/07/15 18:48, Dan Carpenter wrote:
> >>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc,
> >>> struct fb_info *info,
> >>> region.color = color;
> >>> region.rop = ROP_COPY;
> >>>
> >>> - if (rw && !bottom_only) {
> >>> + if ((int) rw > 0 && !bottom_only) {
> >>> region.dx = info->var.xoffset + rs;
> >> ^^
> >>
> >> If you choose a very high positive "rw" then this addition can overflow.
> >> info->var.xoffset comes from the user and I don't think it's checked...
> >
> > Well, I think it would be checked by "struct fb_ops"->check_var hook.
> > For example, vmw_fb_check_var() has
> >
> > if ((var->xoffset + var->xres) > par->max_width ||
> > (var->yoffset + var->yres) > par->max_height) {
> > DRM_ERROR("Requested geom can not fit in framebuffer\n");
> > return -EINVAL;
> > }
> >
> > check. Of course, there might be integer overflow in that check...
> > Having sanity check at caller of "struct fb_ops"->check_var might be nice.
> >
>
> Well, while
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, );
> var.xres = var.yres = 4;
> var.xoffset = 4294967292U;
> ioctl(fd, FBIOPUT_VSCREENINFO, );
>
> bypassed
>
> (var->xoffset + var->xres) > par->max_width
>
> check in vmw_fb_check_var(),
>
> --
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
> fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> + printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u
> bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
> if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> --
>
> says that info->var.xoffset does not come from the user.
>
> --
> bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
> --
In fb_set_var() we do:
drivers/video/fbdev/core/fbmem.c
1055 ret = info->fbops->fb_check_var(var, info);
1056
1057 if (ret)
1058 return ret;
1059
1060 if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
1061 return 0;
1062
1063 if (!basic_checks(var))
1064 return -EINVAL;
1065
1066 if (info->fbops->fb_get_caps) {
1067 ret = fb_check_caps(info, var, var->activate);
1068
1069 if (ret)
1070 return ret;
1071 }
1072
1073 old_var = info->var;
1074 info->var = *var;
This should set "info->var.offset".
1075
1076 if (info->fbops->fb_set_par) {
1077 ret = info->fbops->fb_set_par(info);
1078
1079 if (ret) {
1080 info->var = old_var;
1081 printk(KERN_WARNING "detected "
I've complained about integer overflows in fbdev for a long time...
What I'd like to see is something like the following maybe. I don't
know how to get the vc_data in fbmem.c so it doesn't include your checks
for negative.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index caf817bcb05c..5c74181fea5d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -934,6 +934,54 @@ fb_pan_display(struct fb_info *info, struct
fb_var_screeninfo *var)
}
EXPORT_SYMBOL(fb_pan_display);
+static bool basic_checks(struct fb_var_screeninfo *var)
+{
+ unsigned int v_margins, h_margins;
+
+ /* I think var->height and var->width == UINT_MAX means something. */
+
+ if (var->xres > INT_MAX ||
+ var->yres > INT_MAX ||
+ var->xres_virtual > INT_MAX ||
+ var->yres_virtual > INT_MAX ||
+ var->xoffset > INT_MAX ||
+ var->yoffset > INT_MAX ||
+ var->left_margin > INT_MAX ||
+ var->right_margin > INT_MAX ||
+ var->upper_margin > INT_MAX ||
+ var->lower_margin > INT_MAX ||
+ var->hsync_len > INT_MAX ||
+ var->vsync_len > INT_MAX)
+ return false;
+
+ if (var->bits_per_pixel > 128)
+ return false;
+ if (var->rotate > FB_ROTATE_CCW)
+ return false;
+
+ if (var->xoffset > INT_MAX - var->xres)
+ return false;
+ if (var->yoffset > INT_MAX - var->yres)
+ return false;
+
+ if (var->left_margin > INT_MAX - var->right_margin ||
+ var->upper_margin >
Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver
Hi Anitha On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote: > This is a new DRM driver for Intel's KeemBay SOC. > The SoC couples an ARM Cortex A53 CPU with an Intel > Movidius VPU. > > This driver is tested with the KMB EVM board which is the refernce baord > for Keem Bay SOC. The SOC's display pipeline is as follows > > +--++-++---+ > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > +--++-++---+ > > LCD controller and Mipi DSI transmitter are part of the SOC and > mipi to HDMI converter is ADV7535 for KMB EVM board. > > The DRM driver is a basic KMS atomic modesetting display driver and > has no 2D or 3D graphics.It calls into the ADV bridge driver at > the connector level. > > Only 1080p resolution and single plane is supported at this time. > The usecase is for debugging video and camera outputs. > > Device tree patches are under review here > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandre...@linux.intel.com/T/ Cool, new driver, thanks a lot for submitting. > Changes since v1: > - Removed redundant license text, updated license > - Rearranged include blocks > - renamed global vars and removed extern in c > - Used upclassing for dev_private > - Used drm_dev_init in drm device create (will be updated to use > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > developed on 5.4 kernel) drm moves fairly quickly, please develop the upstream submission on top of linux-next or similar. We constantly add new helpers to simplify drivers, and we expect new driver submissions to be up to date with all that. Another thing: From your description it sounds like it's a very simple driver, just a single plane/crtc, nothing fancy, plus adv bridge output. Is the driver already using simple display pipeline helpers? I think that would be an ideal fit and probably greatly simplifies the code. > - minor cleanups The patch series looks like it contains the entire development history, or at least large chunks of it. That's useful for you, but for upstreaming the main focues (especially for smaller drivers) is whether your driver uses all the available helpers and integrations correctly. And for that it's much easier if the history is cleaned up, and all intermediate steps removed. I think once that's done I can do a quick pass and drop suggestions for cleanup and stuff like that, and then we should (usually at least) be able to pull in the driver fairly quickly. Another thing to consider is where/how this driver will be maintained. Preferred option is as part of drm-misc so that we have redudancy and all that in a fairly big group. Works with commit rights, so maybe check out some of our docs about that too. https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html The committer model comes with a full set of scripts and docs to avoid oopsies in maintainership. Generally works really well. Cheers, Daniel > > Anitha Chrisanthus (52): > drm/kmb: Add support for KeemBay Display > drm/kmb: Added id to kmb_plane > drm/kmb: Set correct values in the LAYERn_CFG register > drm/kmb: Use biwise operators for register definitions > drm/kmb: Updated kmb_plane_atomic_check > drm/kmb: Initial check-in for Mipi DSI > drm/kmb: Set OUT_FORMAT_CFG register > drm/kmb: Added mipi_dsi_host initialization > drm/kmb: Part 1 of Mipi Tx Initialization > drm/kmb: Part 2 of Mipi Tx Initialization > drm/kmb: Use correct mmio offset from data book > drm/kmb: Part3 of Mipi Tx initialization > drm/kmb: Part4 of Mipi Tx Initialization > drm/kmb: Correct address offsets for mipi registers > drm/kmb: Part5 of Mipi Tx Intitialization > drm/kmb: Part6 of Mipi Tx Initialization > drm/kmb: Part7 of Mipi Tx Initialization > drm/kmb: Part8 of Mipi Tx Initialization > drm/kmb: Added ioremap/iounmap for register access > drm/kmb: Register IRQ for LCD > drm/kmb: IRQ handlers for LCD and mipi dsi > drm/kmb: Set hardcoded values to LCD_VSYNC_START > drm/kmb: Additional register programming to update_plane > drm/kmb: Add ADV7535 bridge > drm/kmb: Display clock enable/disable > drm/kmb: rebase to newer kernel version > drm/kmb: minor name change to match device tree > drm/kmb: Changed MMIO size > drm/kmb: Defer Probe > drm/kmb: call bridge init in the very beginning > drm/kmb: Enable MSS_CAM_CLK_CTRL for LCD and MIPI > drm/kmb: Set MSS_CAM_RSTN_CTRL along with enable > drm/kmb: Mipi DPHY initialization changes > drm/kmb: Fixed driver unload > drm/kmb: Added LCD_TEST config > drm/kmb: Changes for LCD to Mipi > drm/kmb: Update LCD programming to match MIPI > drm/kmb: Changed name of driver to kmb-drm > drm/kmb: Mipi settings from input timings > drm/kmb: Enable LCD interrupts > drm/kmb: Enable LCD interrupts during modeset > drm/kmb: Don’t inadvertantly disable LCD
[PATCH 0/8] drm/mgag200: Support desktop chips
This patchset puts device initialization in the correct order and adds support for G200 Desktop chips (PCI ids 0x520 and 0x521). The first 7 patches prepare the driver. Desktop chips would probably work without them, but since we're at it we can also do it right. Patch 1 enables cached page mappings GEM buffers. SHMEM supports this well now and the MGA device does not access the buffer memory directly. So now corrupt display output is to be expected. Patches 2 to 6 fix the initialization of device registers. Several fundamental registers were only done late during device initialization. This was probably not a problem in practice, as the VGA BIOS does the setup iduring POST anyway. These patches move the code to the beginning of the device initialization. If we ever have to POST a MGA device from the driver, the corect order of operations counts. G200SEs store a unique id in the device structure. Patch 7 moves the value to model-specific data area. This will be helpful for patch 8. Patch 8 adds support for desktop chips' PCI ids. all the memory and modesetting code continues to work as before. The PLL setup code gets an additional helper for the new HW. PCI and DAC regsiters get a few new default values. Most significantly, the driver parses the VGA BIOS for clock settings. It's all separate from the server code, so no regressions are to be expected. The new HW support is based on an earlier patch the was posted in July 2017. [1] Tested on G200EW and G200 AGP hardware by running the fbdev console, Weston and Gnome on Xorg. [1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147647.html Thomas Zimmermann (8): drm/mgag200: Enable caching for SHMEM pages drm/mgag200: Move register initialization into helper function drm/mgag200: Initialize PCI registers early during device setup drm/mgag200: Enable MGA mode during device register initialization drm/mgag200: Set MISC memory flags in mm init code drm/mgag200: Clear field during MM init drm/mgag200: Move G200SE's unique id into model-specific data drm/mgag200: Add support for G200 desktop cards MAINTAINERS| 2 +- drivers/gpu/drm/mgag200/Kconfig| 12 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 213 +++-- drivers/gpu/drm/mgag200/mgag200_drv.h | 19 ++- drivers/gpu/drm/mgag200/mgag200_mm.c | 8 + drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++--- drivers/gpu/drm/mgag200/mgag200_reg.h | 4 + 7 files changed, 328 insertions(+), 83 deletions(-) -- 2.27.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 8/8] drm/mgag200: Add support for G200 desktop cards
This patch adds support for G200 desktop cards. We can reuse the whole
memory and modesetting code. A few PCI and DAC register values have to
be updated accordingly.
The most significant change is in the PLL setup. The get the clock limits
and reference clocks, parses the device's BIOS. With no BIOS found, safe
defaults are being used.
Signed-off-by: Thomas Zimmermann
Co-developed-by: Egbert Eich
Signed-off-by: Egbert Eich
Co-developed-by: Takashi Iwai
Signed-off-by: Takashi Iwai
---
MAINTAINERS| 2 +-
drivers/gpu/drm/mgag200/Kconfig| 12 +--
drivers/gpu/drm/mgag200/mgag200_drv.c | 125 -
drivers/gpu/drm/mgag200/mgag200_drv.h | 10 ++
drivers/gpu/drm/mgag200/mgag200_mode.c | 80
5 files changed, 220 insertions(+), 9 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 415954a98934..4c6f96e2b79b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5406,7 +5406,7 @@ S:Orphan / Obsolete
F: drivers/gpu/drm/mga/
F: include/uapi/drm/mga_drm.h
-DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
+DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
M: Dave Airlie
S: Odd Fixes
F: drivers/gpu/drm/mgag200/
diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
index 93be766715c9..eec59658a938 100644
--- a/drivers/gpu/drm/mgag200/Kconfig
+++ b/drivers/gpu/drm/mgag200/Kconfig
@@ -1,13 +1,11 @@
# SPDX-License-Identifier: GPL-2.0-only
config DRM_MGAG200
- tristate "Kernel modesetting driver for MGA G200 server engines"
+ tristate "Matrox G200"
depends on DRM && PCI && MMU
select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
help
-This is a KMS driver for the MGA G200 server chips, it
-does not support the original MGA G200 or any of the desktop
-chips. It requires 0.3.0 of the modesetting userspace driver,
-and a version of mga driver that will fail on KMS enabled
-devices.
-
+This is a KMS driver for Matrox G200 chips. It supports the original
+MGA G200 desktop chips and the server variants. It requires 0.3.0
+of the modesetting userspace driver, and a version of mga driver
+that will fail on KMS enabled devices.
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index f7652e16365c..419817d6e2cd 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
u8 crtcext3;
switch (mdev->type) {
+ case G200_PCI:
+ case G200_AGP:
+ if (mgag200_has_sgram(mdev))
+ option = 0x4049cd21;
+ else
+ option = 0x40499121;
+ option2 = 0x8000;
+ break;
case G200_SE_A:
case G200_SE_B:
if (mgag200_has_sgram(mdev))
@@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
return 0;
}
+static void mgag200_g200_interpret_bios(struct mga_device *mdev,
+ unsigned char __iomem *bios,
+ size_t size)
+{
+ static const unsigned int expected_length[6] = {
+ 0, 64, 64, 64, 128, 128
+ };
+
+ struct drm_device *dev = >base;
+ unsigned char __iomem *pins;
+ unsigned int pins_len, version;
+ int offset;
+ int tmp;
+
+ if (size < MGA_BIOS_OFFSET + 1)
+ return;
+
+ if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
+ bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
+ return;
+
+ offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
+
+ if (offset + 5 > size)
+ return;
+
+ pins = bios + offset;
+ if (pins[0] == 0x2e && pins[1] == 0x41) {
+ version = pins[5];
+ pins_len = pins[2];
+ } else {
+ version = 1;
+ pins_len = pins[0] + (pins[1] << 8);
+ }
+
+ if (version < 1 || version > 5) {
+ drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
+ return;
+ }
+ if (pins_len != expected_length[version]) {
+ drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
+pins_len, expected_length[version]);
+ return;
+ }
+
+ if (offset + pins_len > size)
+ return;
+
+ drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
+ version, pins_len);
+
+ switch (version) {
+ case 1:
+ tmp = pins[24] + (pins[25] << 8);
+ if (tmp)
+ mdev->model.g200.pclk_max = tmp * 10;
+ break;
+ case 2:
+ if (pins[41] != 0xff)
+
[PATCH 3/8] drm/mgag200: Initialize PCI registers early during device setup
So far, PCI option registers were initialized as part of modesetting,
which is late in the process. As these registers control fundamental
operation, they should be set early.
The patch moves the PCI option handling into device register setup,
before even the device MMIO memory is being mapped. No functional
changes made.
Moving the PCI code next to the device-register setup also allows to
remove the has_sdram field from struct mga_device. The state is now
local to the init helper.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_drv.c | 32 +++-
drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
drivers/gpu/drm/mgag200/mgag200_mode.c | 41 --
3 files changed, 31 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index e50c682c4702..3dbb00045c24 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -60,8 +60,38 @@ static bool mgag200_has_sgram(struct mga_device *mdev)
static int mgag200_regs_init(struct mga_device *mdev)
{
struct drm_device *dev = >base;
+ u32 option, option2;
+
+ switch (mdev->type) {
+ case G200_SE_A:
+ case G200_SE_B:
+ if (mgag200_has_sgram(mdev))
+ option |= PCI_MGA_OPTION_HARDPWMSK;
+ option2 = 0x8000;
+ break;
+ case G200_WB:
+ case G200_EW3:
+ option = 0x41049120;
+ option2 = 0xb000;
+ break;
+ case G200_EV:
+ option = 0x0120;
+ option2 = 0xb000;
+ break;
+ case G200_EH:
+ case G200_EH3:
+ option = 0x0120;
+ option2 = 0xb000;
+ break;
+ default:
+ option = 0;
+ option2 = 0;
+ }
- mdev->has_sdram = !mgag200_has_sgram(mdev);
+ if (option)
+ pci_write_config_dword(dev->pdev, PCI_MGA_OPTION, option);
+ if (option2)
+ pci_write_config_dword(dev->pdev, PCI_MGA_OPTION2, option2);
/* BAR 1 contains registers */
mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 3817520bfefc..819c03cc626b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -161,7 +161,6 @@ struct mga_device {
size_t vram_fb_available;
enum mga_type type;
- int has_sdram;
int bpp_shifts[4];
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e0d037a7413c..3aa078e69a5a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -9,7 +9,6 @@
*/
#include
-#include
#include
#include
@@ -877,45 +876,6 @@ static void mgag200_set_startadd(struct mga_device *mdev,
WREG_ECRT(0x00, crtcext0);
}
-static void mgag200_set_pci_regs(struct mga_device *mdev)
-{
- uint32_t option = 0, option2 = 0;
- struct drm_device *dev = >base;
-
- switch (mdev->type) {
- case G200_SE_A:
- case G200_SE_B:
- if (mdev->has_sdram)
- option = 0x40049120;
- else
- option = 0x4004d120;
- option2 = 0x8000;
- break;
- case G200_WB:
- case G200_EW3:
- option = 0x41049120;
- option2 = 0xb000;
- break;
- case G200_EV:
- option = 0x0120;
- option2 = 0xb000;
- break;
- case G200_EH:
- case G200_EH3:
- option = 0x0120;
- option2 = 0xb000;
- break;
- case G200_ER:
- break;
- }
-
- if (option)
- pci_write_config_dword(dev->pdev, PCI_MGA_OPTION, option);
-
- if (option2)
- pci_write_config_dword(dev->pdev, PCI_MGA_OPTION2, option2);
-}
-
static void mgag200_set_dac_regs(struct mga_device *mdev)
{
size_t i;
@@ -988,7 +948,6 @@ static void mgag200_init_regs(struct mga_device *mdev)
{
u8 crtc11, crtcext3, crtcext4, misc;
- mgag200_set_pci_regs(mdev);
mgag200_set_dac_regs(mdev);
WREG_SEQ(2, 0x0f);
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/8] drm/mgag200: Enable MGA mode during device register initialization
MGA cards can run in traditional VGA mode or an enhanced MGA mode; with
the latter being required for KMS. So far, MGA mode was enabled during
modesetting. As it's fundamental for device operation, the patch moves
it next to the device register setup.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_drv.c | 5 +
drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +-
drivers/gpu/drm/mgag200/mgag200_reg.h | 2 ++
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 3dbb00045c24..ac9ac5b6d587 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -61,6 +61,7 @@ static int mgag200_regs_init(struct mga_device *mdev)
{
struct drm_device *dev = >base;
u32 option, option2;
+ u8 crtcext3;
switch (mdev->type) {
case G200_SE_A:
@@ -107,6 +108,10 @@ static int mgag200_regs_init(struct mga_device *mdev)
if (mdev->rmmio == NULL)
return -ENOMEM;
+ RREG_ECRT(0x03, crtcext3);
+ crtcext3 |= MGAREG_CRTCEXT3_MGAMODE;
+ WREG_ECRT(0x03, crtcext3);
+
return 0;
}
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 3aa078e69a5a..7161b1651aa0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -946,7 +946,7 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
static void mgag200_init_regs(struct mga_device *mdev)
{
- u8 crtc11, crtcext3, crtcext4, misc;
+ u8 crtc11, crtcext4, misc;
mgag200_set_dac_regs(mdev);
@@ -961,12 +961,8 @@ static void mgag200_init_regs(struct mga_device *mdev)
WREG_CRT(14, 0);
WREG_CRT(15, 0);
- RREG_ECRT(0x03, crtcext3);
-
- crtcext3 |= BIT(7); /* enable MGA mode */
crtcext4 = 0x00;
- WREG_ECRT(0x03, crtcext3);
WREG_ECRT(0x04, crtcext4);
RREG_CRT(0x11, crtc11);
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h
b/drivers/gpu/drm/mgag200/mgag200_reg.h
index a44c08bf4074..977be0565c06 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -256,6 +256,8 @@
#define MGAREG_CRTCEXT1_VSYNCOFF BIT(5)
#define MGAREG_CRTCEXT1_HSYNCOFF BIT(4)
+#define MGAREG_CRTCEXT3_MGAMODEBIT(7)
+
/* Cursor X and Y position */
#define MGA_CURPOSXL 0x3c0c
#define MGA_CURPOSXH 0x3c0d
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 6/8] drm/mgag200: Clear field during MM init
The modesetting code initialized the memory-related register CRTCEXT4.
Move this code to MM initialization.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_mm.c | 2 ++
drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c
b/drivers/gpu/drm/mgag200/mgag200_mm.c
index 1b1918839e1e..641f1aa992be 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -94,6 +94,8 @@ int mgag200_mm_init(struct mga_device *mdev)
resource_size_t start, len;
int ret;
+ WREG_ECRT(0x04, 0x00);
+
misc = RREG8(MGA_MISC_IN);
misc |= MGAREG_MISC_RAMMAPEN |
MGAREG_MISC_HIGH_PG_SEL;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 66818ee10694..4fa64cf884cb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -946,7 +946,7 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
static void mgag200_init_regs(struct mga_device *mdev)
{
- u8 crtc11, crtcext4, misc;
+ u8 crtc11, misc;
mgag200_set_dac_regs(mdev);
@@ -961,10 +961,6 @@ static void mgag200_init_regs(struct mga_device *mdev)
WREG_CRT(14, 0);
WREG_CRT(15, 0);
- crtcext4 = 0x00;
-
- WREG_ECRT(0x04, crtcext4);
-
RREG_CRT(0x11, crtc11);
crtc11 &= ~(MGAREG_CRTC11_CRTCPROTECT |
MGAREG_CRTC11_VINTEN |
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 5/8] drm/mgag200: Set MISC memory flags in mm init code
The modesetting code initialized several memory-related flags in the
MISC register. Move this code to MM initialization.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_mm.c | 6 ++
drivers/gpu/drm/mgag200/mgag200_mode.c | 4 +---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c
b/drivers/gpu/drm/mgag200/mgag200_mm.c
index 7b69392bcb89..1b1918839e1e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -90,9 +90,15 @@ static void mgag200_mm_release(struct drm_device *dev, void
*ptr)
int mgag200_mm_init(struct mga_device *mdev)
{
struct drm_device *dev = >base;
+ u8 misc;
resource_size_t start, len;
int ret;
+ misc = RREG8(MGA_MISC_IN);
+ misc |= MGAREG_MISC_RAMMAPEN |
+ MGAREG_MISC_HIGH_PG_SEL;
+ WREG8(MGA_MISC_OUT, misc);
+
/* BAR 0 is VRAM */
start = pci_resource_start(dev->pdev, 0);
len = pci_resource_len(dev->pdev, 0);
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 7161b1651aa0..66818ee10694 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -978,9 +978,7 @@ static void mgag200_init_regs(struct mga_device *mdev)
WREG_ECRT(0x34, 0x5);
misc = RREG8(MGA_MISC_IN);
- misc |= MGAREG_MISC_IOADSEL |
- MGAREG_MISC_RAMMAPEN |
- MGAREG_MISC_HIGH_PG_SEL;
+ misc |= MGAREG_MISC_IOADSEL;
WREG8(MGA_MISC_OUT, misc);
}
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 7/8] drm/mgag200: Move G200SE's unique id into model-specific data
The unique revision id is only useful for G200SE devices. Store the
value in model-specific data within struct mga_device. While at it,
the patch also adds an init helper for the value.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_drv.c | 19 +--
drivers/gpu/drm/mgag200/mgag200_drv.h | 8 ++--
drivers/gpu/drm/mgag200/mgag200_mode.c | 18 +++---
3 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index ac9ac5b6d587..f7652e16365c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -115,6 +115,17 @@ static int mgag200_regs_init(struct mga_device *mdev)
return 0;
}
+static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
+{
+ struct drm_device *dev = >base;
+
+ /* stash G200 SE model number for later use */
+ mdev->model.g200se.unique_rev_id = RREG32(0x1e24);
+
+ drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
+ mdev->model.g200se.unique_rev_id);
+}
+
static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
{
struct drm_device *dev = >base;
@@ -127,12 +138,8 @@ static int mgag200_device_init(struct mga_device *mdev,
unsigned long flags)
if (ret)
return ret;
- /* stash G200 SE model number for later use */
- if (IS_G200_SE(mdev)) {
- mdev->unique_rev_id = RREG32(0x1e24);
- drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
- mdev->unique_rev_id);
- }
+ if (IS_G200_SE(mdev))
+ mgag200_g200se_init_unique_id(mdev);
ret = mgag200_mm_init(mdev);
if (ret)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 819c03cc626b..048efe635aff 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -166,8 +166,12 @@ struct mga_device {
int fb_mtrr;
- /* SE model number stored in reg 0x1e24 */
- u32 unique_rev_id;
+ union {
+ struct {
+ /* SE model number stored in reg 0x1e24 */
+ u32 unique_rev_id;
+ } g200se;
+ } model;
struct mga_connector connector;
struct drm_simple_display_pipe display_pipe;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 4fa64cf884cb..752409c7f326 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -112,6 +112,7 @@ static inline void mga_wait_busy(struct mga_device *mdev)
static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
{
+ u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
unsigned int vcomax, vcomin, pllreffreq;
unsigned int delta, tmpdelta, permitteddelta;
unsigned int testp, testm, testn;
@@ -121,7 +122,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev,
long clock)
unsigned int fvv;
unsigned int i;
- if (mdev->unique_rev_id <= 0x03) {
+ if (unique_rev_id <= 0x03) {
m = n = p = 0;
vcomax = 32;
@@ -219,7 +220,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev,
long clock)
WREG_DAC(MGA1064_PIX_PLLC_N, n);
WREG_DAC(MGA1064_PIX_PLLC_P, p);
- if (mdev->unique_rev_id >= 0x04) {
+ if (unique_rev_id >= 0x04) {
WREG_DAC(0x1a, 0x09);
msleep(20);
WREG_DAC(0x1a, 0x01);
@@ -1183,12 +1184,13 @@ static void mgag200_g200se_set_hiprilvl(struct
mga_device *mdev,
const struct drm_display_mode *mode,
const struct drm_framebuffer *fb)
{
+ u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
unsigned int hiprilvl;
u8 crtcext6;
- if (mdev->unique_rev_id >= 0x04) {
+ if (unique_rev_id >= 0x04) {
hiprilvl = 0;
- } else if (mdev->unique_rev_id >= 0x02) {
+ } else if (unique_rev_id >= 0x02) {
unsigned int bpp;
unsigned long mb;
@@ -1213,7 +1215,7 @@ static void mgag200_g200se_set_hiprilvl(struct mga_device
*mdev,
else
hiprilvl = 5;
- } else if (mdev->unique_rev_id >= 0x01) {
+ } else if (unique_rev_id >= 0x01) {
hiprilvl = 3;
} else {
hiprilvl = 4;
@@ -1337,7 +1339,9 @@ static enum drm_mode_status mga_vga_mode_valid(struct
drm_connector *connector,
int bpp = 32;
if (IS_G200_SE(mdev)) {
- if (mdev->unique_rev_id == 0x01) {
+ u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
+
+ if (unique_rev_id == 0x01) {
if
[PATCH 1/8] drm/mgag200: Enable caching for SHMEM pages
SHMEM pages use write-combine caching by default, but can also use the
platform's default page caching. Doing so may improve the performance
of I/O on the framebuffer.
Mgag200's hardware does not access framebuffer pages directly (i.e.,
via DMA), so enabling caching does not have an effect on consistency
of the framebuffer memory or the displayed data.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index e19660f4a637..7189c7745baf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -36,6 +36,7 @@ static struct drm_driver mgag200_driver = {
.major = DRIVER_MAJOR,
.minor = DRIVER_MINOR,
.patchlevel = DRIVER_PATCHLEVEL,
+ .gem_create_object = drm_gem_shmem_create_object_cached,
DRM_GEM_SHMEM_DRIVER_OPS,
};
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/8] drm/mgag200: Move register initialization into helper function
The mgag200 driver maps registers into the address space. Move the
code into a separate helper function. No functional changes.
One small difference is in the handling of SDRAM/SGRAM. MGA devices
can come with either SDRAM or SGRAM. So far, the driver checked for
SDRAM, which is the common case. The patch moves this code into a
separate helper and checks for SGRAM, which is the special case. The
test itself is the same as before.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_drv.c | 37 ++-
drivers/gpu/drm/mgag200/mgag200_reg.h | 2 ++
2 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 7189c7745baf..e50c682c4702 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -44,18 +44,26 @@ static struct drm_driver mgag200_driver = {
* DRM device
*/
-static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
+static bool mgag200_has_sgram(struct mga_device *mdev)
{
struct drm_device *dev = >base;
- int ret, option;
+ u32 option;
+ int ret;
- mdev->flags = mgag200_flags_from_driver_data(flags);
- mdev->type = mgag200_type_from_driver_data(flags);
+ ret = pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, );
+ if (drm_WARN(dev, ret, "failed to read PCI config dword: %d\n", ret))
+ return false;
+
+ return !!(option & PCI_MGA_OPTION_HARDPWMSK);
+}
- pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, );
- mdev->has_sdram = !(option & (1 << 14));
+static int mgag200_regs_init(struct mga_device *mdev)
+{
+ struct drm_device *dev = >base;
+
+ mdev->has_sdram = !mgag200_has_sgram(mdev);
- /* BAR 0 is the framebuffer, BAR 1 contains registers */
+ /* BAR 1 contains registers */
mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
@@ -69,6 +77,21 @@ static int mgag200_device_init(struct mga_device *mdev,
unsigned long flags)
if (mdev->rmmio == NULL)
return -ENOMEM;
+ return 0;
+}
+
+static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
+{
+ struct drm_device *dev = >base;
+ int ret;
+
+ mdev->flags = mgag200_flags_from_driver_data(flags);
+ mdev->type = mgag200_type_from_driver_data(flags);
+
+ ret = mgag200_regs_init(mdev);
+ if (ret)
+ return ret;
+
/* stash G200 SE model number for later use */
if (IS_G200_SE(mdev)) {
mdev->unique_rev_id = RREG32(0x1e24);
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h
b/drivers/gpu/drm/mgag200/mgag200_reg.h
index c3b7bcad52ed..a44c08bf4074 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -282,6 +282,8 @@
#define PCI_MGA_OPTION20x50
#define PCI_MGA_OPTION30x54
+#define PCI_MGA_OPTION_HARDPWMSK BIT(14)
+
#define RAMDAC_OFFSET 0x3c00
/* TVP3026 direct registers */
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
On Wed, Jul 15, 2020 at 4:37 PM Chris Wilson wrote:
>
> Quoting Daniel Vetter (2020-07-15 15:03:34)
> > On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson
> > wrote:
> > > There's a further problem in that we call INIT_LIST_HEAD on the
> > > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > > confirms the dma_fence_cb to be completely decoupled, the containing
> > > struct may still be inuse.
> >
> > The kerneldoc of dma_fence_remove_callback() already has a very stern
> > warning that this will blow up if you don't hold a full reference or
> > otherwise control the lifetime of this stuff. So I don't think we have
> > to worry about any of that. Or do you mean something else?
>
> It's the struct dma_fence_cb itself that may be freed/reused. Consider
> dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
> so in order to ensure that the callback is completed the list_empty
> check has to remain under the spinlock, or else
> dma_fence_default_wait_cb() can still be dereferencing wait->task as the
> function returns.
The current implementation of remove_callback doesn't work if you
don't own the callback structure. Or control its lifetime through some
other means.
So if we have callers removing other callback structures, that just
doesn't work, you can only remove your own.
>From a quick spot check across a few callers we don't seem to have a
problem here, all current callers for this function are in various
wait functions (driver specific, or multi fence waits, stuff like
that).
-Daniel
> So currently it is:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long
> timeout)
> {
> struct default_wait_cb cb;
> unsigned long flags;
> signed long ret = timeout ? timeout : 1;
>
> spin_lock_irqsave(fence->lock, flags);
>
> if (intr && signal_pending(current)) {
> ret = -ERESTARTSYS;
> goto out;
> }
>
> if (!__dma_fence_enable_signaling(fence))
> goto out;
>
> if (!timeout) {
> ret = 0;
> goto out;
> }
>
> cb.base.func = dma_fence_default_wait_cb;
> cb.task = current;
> list_add(, >cb_list);
>
> while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags) && ret >
> 0) {
> if (intr)
> __set_current_state(TASK_INTERRUPTIBLE);
> else
> __set_current_state(TASK_UNINTERRUPTIBLE);
> spin_unlock_irqrestore(fence->lock, flags);
>
> ret = schedule_timeout(ret);
>
> spin_lock_irqsave(fence->lock, flags);
> if (ret > 0 && intr && signal_pending(current))
> ret = -ERESTARTSYS;
> }
>
> if (!list_empty())
> list_del();
> __set_current_state(TASK_RUNNING);
>
> out:
> spin_unlock_irqrestore(fence->lock, flags);
> return ret;
> }
>
> but it could be written as:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long
> timeout)
> {
> struct default_wait_cb cb;
> int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
>
> cb.task = current;
> if (dma_fence_add_callback(fence, ,
> dma_fence_default_wait_cb))
> return timeout ? timeout : 1;
>
> for (;;) {
> set_current_state(state);
>
> if (dma_fence_is_signaled(fence)) {
> timeout = timeout ? timeout : 1;
> break;
> }
>
> if (signal_pending_state(state, current)) {
> timeout = -ERESTARTSYS;
> break;
> }
>
> if (!timeout)
> break;
>
> timeout = schedule_timeout(timeout);
> }
> __set_current_state(TASK_RUNNING);
>
> dma_fence_remove_callback(fence, );
>
> return timeout;
> }
> -Chris
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
Quoting Daniel Vetter (2020-07-15 15:03:34)
> On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson wrote:
> > There's a further problem in that we call INIT_LIST_HEAD on the
> > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > confirms the dma_fence_cb to be completely decoupled, the containing
> > struct may still be inuse.
>
> The kerneldoc of dma_fence_remove_callback() already has a very stern
> warning that this will blow up if you don't hold a full reference or
> otherwise control the lifetime of this stuff. So I don't think we have
> to worry about any of that. Or do you mean something else?
It's the struct dma_fence_cb itself that may be freed/reused. Consider
dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
so in order to ensure that the callback is completed the list_empty
check has to remain under the spinlock, or else
dma_fence_default_wait_cb() can still be dereferencing wait->task as the
function returns.
So currently it is:
signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
struct default_wait_cb cb;
unsigned long flags;
signed long ret = timeout ? timeout : 1;
spin_lock_irqsave(fence->lock, flags);
if (intr && signal_pending(current)) {
ret = -ERESTARTSYS;
goto out;
}
if (!__dma_fence_enable_signaling(fence))
goto out;
if (!timeout) {
ret = 0;
goto out;
}
cb.base.func = dma_fence_default_wait_cb;
cb.task = current;
list_add(, >cb_list);
while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags) && ret >
0) {
if (intr)
__set_current_state(TASK_INTERRUPTIBLE);
else
__set_current_state(TASK_UNINTERRUPTIBLE);
spin_unlock_irqrestore(fence->lock, flags);
ret = schedule_timeout(ret);
spin_lock_irqsave(fence->lock, flags);
if (ret > 0 && intr && signal_pending(current))
ret = -ERESTARTSYS;
}
if (!list_empty())
list_del();
__set_current_state(TASK_RUNNING);
out:
spin_unlock_irqrestore(fence->lock, flags);
return ret;
}
but it could be written as:
signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
struct default_wait_cb cb;
int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
cb.task = current;
if (dma_fence_add_callback(fence, , dma_fence_default_wait_cb))
return timeout ? timeout : 1;
for (;;) {
set_current_state(state);
if (dma_fence_is_signaled(fence)) {
timeout = timeout ? timeout : 1;
break;
}
if (signal_pending_state(state, current)) {
timeout = -ERESTARTSYS;
break;
}
if (!timeout)
break;
timeout = schedule_timeout(timeout);
}
__set_current_state(TASK_RUNNING);
dma_fence_remove_callback(fence, );
return timeout;
}
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PULL] drm-intel-next
On Wed, Jul 15, 2020 at 3:34 PM Jani Nikula wrote: > > > Argh, failed to mention: > > On Wed, 15 Jul 2020, Jani Nikula wrote: > > Lee Shawn C (1): > > drm/i915/mst: filter out the display mode exceed sink's capability > > The above depends on: > > > Lyude Paul (1): > > drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx > > Which has changes outside of i915: > > > drivers/gpu/drm/drm_crtc_helper_internal.h | 7 +- > > drivers/gpu/drm/drm_probe_helper.c | 97 +-- > > and does not have an explicit ack recorded, though drm-misc maintainers > have been Cc'd. :( > > I decided they were benign enough, but needed to be mentioned. Yeah looks all fine, adding Lyude just as fyi. -Daniel > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson wrote: > Quoting Chris Wilson (2020-07-15 13:21:43) > > Quoting Daniel Vetter (2020-07-15 13:10:22) > > > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote: > > > > When waiting with a callback on the stack, we must remove the callback > > > > upon wait completion. Since this will be notified by the fence signal > > > > callback, the removal often contends with the fence->lock being held by > > > > the signaler. We can look at the list entry to see if the callback was > > > > already signaled before we take the contended lock. > > > > > > > > Signed-off-by: Chris Wilson > > > > --- > > > > drivers/dma-buf/dma-fence.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > index 8d5bdfce638e..b910d7bc0854 100644 > > > > --- a/drivers/dma-buf/dma-fence.c > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, > > > > struct dma_fence_cb *cb) > > > > unsigned long flags; > > > > bool ret; > > > > > > > > + if (list_empty(>node)) > > > > > > I was about to say "but the races" but then noticed that Paul fixed > > > list_empty to use READ_ONCE like 5 years ago :-) > > > > I'm always going "when exactly do we need list_empty_careful()"? > > > > We can rule out a concurrent dma_fence_add_callback() for the same > > dma_fence_cb, as that is a lost cause. So we only have to worry about > > the concurrent list_del_init() from dma_fence_signal_locked(). So it's > > the timing of > > list_del_init(): WRITE_ONCE(list->next, list) > > vs > > READ_ONCE(list->next) == list > > and we don't need to care about the trailing instructions in > > list_del_init()... > > > > Wait that trailing instruction is actually important here if the > > dma_fence_cb is on the stack, or other imminent free. > > > > Ok, this does need to be list_empty_careful! Hm, tbh I'm not really clear what list_empty_careful does on top. Seems to lack READ_ONCE, so either some really big trickery with dependencies is going on, or I'm not even sure how this works without locks. I've now stared at list_empty_careful and a bunch of users quite a bit, and I have now idea when you'd want to use it. Lockless you want a READ_ONCE I think and a simple check, so list_empty. And just accept that any time you race you'll go into the locked slowpath for "list isn't empty". Also only works if the list_empty case is the "nothing to do, job already done" case, since the other one just isn't guaranteed to be accurate. list_empty_careful just wraps a bunch more magic around that will make this both worse, and more racy (if the compiler feels creative) because no READ_ONCE or anything like that. I don't get what that thing is for ... > There's a further problem in that we call INIT_LIST_HEAD on the > dma_fence_cb before the signal callback. So even if list_empty_careful() > confirms the dma_fence_cb to be completely decoupled, the containing > struct may still be inuse. The kerneldoc of dma_fence_remove_callback() already has a very stern warning that this will blow up if you don't hold a full reference or otherwise control the lifetime of this stuff. So I don't think we have to worry about any of that. Or do you mean something else? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/25] drm/malidp: Annotate dma-fence critical section in commit path
On Wed, Jul 15, 2020 at 2:53 PM Liviu Dudau wrote: > > On Tue, Jul 07, 2020 at 10:12:12PM +0200, Daniel Vetter wrote: > > Again needs to be put right after the call to > > drm_atomic_helper_commit_hw_done(), since that's the last thing which > > can hold up a subsequent atomic commit. > > > > No surprises here. > > I was (still am) hoping to do a testing for this patch but when I dug out the > series > from the ML it looked like it has extra dependencies, so I was waiting for > the dust > to settle. > > Otherwise, LGTM. I should all just apply I think, it's based on drm-tip. Branch with bunch of other random stuff is here: https://cgit.freedesktop.org/~danvet/drm/log/ If that doesn't cherry-pick cleanly I'm confused about something. -Daniel > > Best regards, > Liviu > > > > > Signed-off-by: Daniel Vetter > > Cc: "James (Qian) Wang" > > Cc: Liviu Dudau > > Cc: Mihail Atanassov > > --- > > drivers/gpu/drm/arm/malidp_drv.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c > > b/drivers/gpu/drm/arm/malidp_drv.c > > index 69fee05c256c..26e60401a8e1 100644 > > --- a/drivers/gpu/drm/arm/malidp_drv.c > > +++ b/drivers/gpu/drm/arm/malidp_drv.c > > @@ -234,6 +234,7 @@ static void malidp_atomic_commit_tail(struct > > drm_atomic_state *state) > > struct drm_crtc *crtc; > > struct drm_crtc_state *old_crtc_state; > > int i; > > + bool fence_cookie = dma_fence_begin_signalling(); > > > > pm_runtime_get_sync(drm->dev); > > > > @@ -260,6 +261,8 @@ static void malidp_atomic_commit_tail(struct > > drm_atomic_state *state) > > > > malidp_atomic_commit_hw_done(state); > > > > + dma_fence_end_signalling(fence_cookie); > > + > > pm_runtime_put(drm->dev); > > > > drm_atomic_helper_cleanup_planes(drm, state); > > -- > > 2.27.0 > > > > -- > > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --- > ¯\_(ツ)_/¯ -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PULL] drm-intel-next
Argh, failed to mention: On Wed, 15 Jul 2020, Jani Nikula wrote: > Lee Shawn C (1): > drm/i915/mst: filter out the display mode exceed sink's capability The above depends on: > Lyude Paul (1): > drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx Which has changes outside of i915: > drivers/gpu/drm/drm_crtc_helper_internal.h | 7 +- > drivers/gpu/drm/drm_probe_helper.c | 97 +-- and does not have an explicit ack recorded, though drm-misc maintainers have been Cc'd. :( I decided they were benign enough, but needed to be mentioned. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PULL] drm-intel-next
Hi Dave & Daniel - The 2nd and presumably the last i915 feature pull for v5.9. drm-intel-next-2020-07-15: drm/i915 features for v5.9, batch #2 Highlights: - Very early DG1 enabling (Abdiel, Lucas, Anusha) Gem/GT: - Fix spinlock recursion on signaling a signaled request (Chris) - Perf: Use GTT when saving/restoring engine GPR (Umesh Nerlige Ramappa) - SSEU refactoring, debugfs move under gt/ (Daniele, Venkata Sandeep Dhanalakota) - Various GT refactoring and cleanup, preparation for future changes (Daniele) - Adjust HuC state accordingly after GuC fetch error (Michał Winiarski) - UC debugfs updates (Michał Winiarski) - Only revoke the GGTT mmappings on aperture detiling changes (Chris) - Only revoke mmap handlers if active (Chris) - Split the context's obj:vma lut into its own mutex (Chris) - Various memory, mmap and performance optimisations (Chris) - Improve system stability in case of false CS events (Chris) - Various refactorings and cleanup (Chris) - Always reset the engine on execlist failures (Chris) - Trace placement of timeline HWSP (Chris) - Update dma-attributes for our sg DMA (Chris) Display: - TGL CDCLK workaround tweaks to unbreak 8K display support (Stanislav) - A number of FBC fixes, along with i865 FBC enabling (Ville) - Validate MST modes against PBN limits (Lyude, Shawn Lee) - Do not access non-existing swizzle registers (Lucas) - Revert GEN11+ HBR3 rate fix that caused issues on TGL (Matt Atwood) - Update TGL+ combo phy initialization to match spec update (José) - Fix HDCP Content Protection property state machine (Anshuman) - Fix HDCP revoked keys handling (Ram) - Improve DDI BUF status checks and waits (Manasi) - Various SDVO+HDMI+DVI fixes around colorimetry, clocking, pixel repeat etc. (Ville) - DP voltage swing function refactoring (José) - WARN if max vswing/pre-emphasis violates the DP spec (Ville) Other: - Add new EHL PCI IDs (José) - Unify struct intel_digital_port variable naming (Lucas) - Various taint updates to aid debugging and improve CI (Michał Winiarski) - Straggler conversions to new mmio register accessors (Daniele) BR, Jani. The following changes since commit d524b87f77364db096855d7eb714ffacec974ddf: drm/i915: Update DRIVER_DATE to 20200702 (2020-07-02 21:25:28 +0300) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-next-2020-07-15 for you to fetch changes up to e57bd05ec0d2d82d63725dedf9f5a063f879de25: drm/i915: Update DRIVER_DATE to 20200715 (2020-07-15 14:18:02 +0300) drm/i915 features for v5.9, batch #2 Highlights: - Very early DG1 enabling (Abdiel, Lucas, Anusha) Gem/GT: - Fix spinlock recursion on signaling a signaled request (Chris) - Perf: Use GTT when saving/restoring engine GPR (Umesh Nerlige Ramappa) - SSEU refactoring, debugfs move under gt/ (Daniele, Venkata Sandeep Dhanalakota) - Various GT refactoring and cleanup, preparation for future changes (Daniele) - Adjust HuC state accordingly after GuC fetch error (Michał Winiarski) - UC debugfs updates (Michał Winiarski) - Only revoke the GGTT mmappings on aperture detiling changes (Chris) - Only revoke mmap handlers if active (Chris) - Split the context's obj:vma lut into its own mutex (Chris) - Various memory, mmap and performance optimisations (Chris) - Improve system stability in case of false CS events (Chris) - Various refactorings and cleanup (Chris) - Always reset the engine on execlist failures (Chris) - Trace placement of timeline HWSP (Chris) - Update dma-attributes for our sg DMA (Chris) Display: - TGL CDCLK workaround tweaks to unbreak 8K display support (Stanislav) - A number of FBC fixes, along with i865 FBC enabling (Ville) - Validate MST modes against PBN limits (Lyude, Shawn Lee) - Do not access non-existing swizzle registers (Lucas) - Revert GEN11+ HBR3 rate fix that caused issues on TGL (Matt Atwood) - Update TGL+ combo phy initialization to match spec update (José) - Fix HDCP Content Protection property state machine (Anshuman) - Fix HDCP revoked keys handling (Ram) - Improve DDI BUF status checks and waits (Manasi) - Various SDVO+HDMI+DVI fixes around colorimetry, clocking, pixel repeat etc. (Ville) - DP voltage swing function refactoring (José) - WARN if max vswing/pre-emphasis violates the DP spec (Ville) Other: - Add new EHL PCI IDs (José) - Unify struct intel_digital_port variable naming (Lucas) - Various taint updates to aid debugging and improve CI (Michał Winiarski) - Straggler conversions to new mmio register accessors (Daniele) Abdiel Janulgue (2): drm/i915/dg1: add initial DG-1 definitions drm/i915/dg1: Add DG1 PCI IDs Anshuman Gupta (1): drm/i915/hdcp: Update CP as per the kernel internal state Anusha Srivatsa (1): drm/i915/dg1: Remove SHPD_FILTER_CNT register programming Chris Wilson (22): drm/i915/gem: Only revoke the GGTT mmapp
[PULL] drm-intel-fixes
Hi Dave & Daniel - drm-intel-fixes-2020-07-15: drm/i915 fixes for v5.8-rc6: - FBC w/a stride fix - Fix use-after-free fix on module reload - Ignore irq enabling on the virtual engines to fix device sleep - Use GTT when saving/restoring engine GPR - Fix selftest sort function BR, Jani. The following changes since commit 11ba468877bb23f28956a35e896356252d63c983: Linux 5.8-rc5 (2020-07-12 16:34:50 -0700) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-fixes-2020-07-15 for you to fetch changes up to 92e0575b99835b5b3aaab2132dd551e0e04eb96a: drm/i915: Recalculate FBC w/a stride when needed (2020-07-14 20:31:45 +0300) drm/i915 fixes for v5.8-rc6: - FBC w/a stride fix - Fix use-after-free fix on module reload - Ignore irq enabling on the virtual engines to fix device sleep - Use GTT when saving/restoring engine GPR - Fix selftest sort function Chris Wilson (2): drm/i915/gt: Ignore irq enabling on the virtual engines drm/i915/gt: Only swap to a random sibling once upon creation Maarten Lankhorst (1): drm/i915: Move cec_notifier to intel_hdmi_connector_unregister, v2. Sudeep Holla (1): drm/i915/selftests: Fix compare functions provided for sorting Umesh Nerlige Ramappa (1): drm/i915/perf: Use GTT when saving/restoring engine GPR Ville Syrjälä (1): drm/i915: Recalculate FBC w/a stride when needed drivers/gpu/drm/i915/display/intel_fbc.c | 33 --- drivers/gpu/drm/i915/display/intel_hdmi.c | 10 ++ drivers/gpu/drm/i915/gt/intel_lrc.c | 19 +- drivers/gpu/drm/i915/gt/selftest_rps.c| 8 drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_perf.c | 1 + 6 files changed, 39 insertions(+), 33 deletions(-) -- Jani Nikula, Intel Open Source Graphics Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/25] drm/malidp: Annotate dma-fence critical section in commit path
On Tue, Jul 07, 2020 at 10:12:12PM +0200, Daniel Vetter wrote: > Again needs to be put right after the call to > drm_atomic_helper_commit_hw_done(), since that's the last thing which > can hold up a subsequent atomic commit. > > No surprises here. I was (still am) hoping to do a testing for this patch but when I dug out the series from the ML it looked like it has extra dependencies, so I was waiting for the dust to settle. Otherwise, LGTM. Best regards, Liviu > > Signed-off-by: Daniel Vetter > Cc: "James (Qian) Wang" > Cc: Liviu Dudau > Cc: Mihail Atanassov > --- > drivers/gpu/drm/arm/malidp_drv.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c > b/drivers/gpu/drm/arm/malidp_drv.c > index 69fee05c256c..26e60401a8e1 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -234,6 +234,7 @@ static void malidp_atomic_commit_tail(struct > drm_atomic_state *state) > struct drm_crtc *crtc; > struct drm_crtc_state *old_crtc_state; > int i; > + bool fence_cookie = dma_fence_begin_signalling(); > > pm_runtime_get_sync(drm->dev); > > @@ -260,6 +261,8 @@ static void malidp_atomic_commit_tail(struct > drm_atomic_state *state) > > malidp_atomic_commit_hw_done(state); > > + dma_fence_end_signalling(fence_cookie); > + > pm_runtime_put(drm->dev); > > drm_atomic_helper_cleanup_planes(drm, state); > -- > 2.27.0 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail
On Wed, Jul 15, 2020 at 10:17:56AM +0200, Daniel Vetter wrote: > On Tue, Jul 14, 2020 at 9:01 PM Melissa Wen wrote: > > > > On 07/14, Daniel Vetter wrote: > > > On Tue, Jul 14, 2020 at 07:39:42AM -0300, Melissa Wen wrote: > > > > On Tue, Jul 14, 2020 at 7:20 AM Melissa Wen > > > > wrote: > > > > > > > > > > On 07/13, Daniel Vetter wrote: > > > > > > On Fri, Jul 10, 2020 at 02:05:33PM -0300, Melissa Wen wrote: > > > > > > > On 07/02, Daniel Vetter wrote: > > > > > > > > On Wed, Jul 01, 2020 at 03:31:34PM +, Sidong Yang wrote: > > > > > > > > > there is an error when igt test is run continuously. > > > > > > > > > vkms_atomic_commit_tail() > > > > > > > > > need to call drm_atomic_helper_wait_for_vblanks() for give up > > > > > > > > > ownership of > > > > > > > > > vblank events. without this code, next atomic commit will not > > > > > > > > > enable vblank > > > > > > > > > and raise timeout error. > > > > > > > > > > > > > > > > > > Signed-off-by: Sidong Yang > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ > > > > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > index 1e8b2169d834..10b9be67a068 100644 > > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > > @@ -93,6 +93,8 @@ static void vkms_atomic_commit_tail(struct > > > > > > > > > drm_atomic_state *old_state) > > > > > > > > > flush_work(_state->composer_work); > > > > > > > > > } > > > > > > > > > > > > > > > > > > + drm_atomic_helper_wait_for_vblanks(dev, old_state); > > > > > > > > > > > > > > > > Uh, we have a wait_for_flip_done right above, which should be > > > > > > > > doing > > > > > > > > exactly the same, but more precisely: Instead of just waiting > > > > > > > > for any > > > > > > > > vblank to happen, we wait for exactly the vblank corresponding > > > > > > > > to this > > > > > > > > atomic commit. So no races possible. If this is papering over > > > > > > > > some issue, > > > > > > > > then I think more debugging is needed. > > > > > > > > > > > > > > > > What exactly is going wrong here for you? > > > > > > > > > > > > > > Hi Daniel and Sidong, > > > > > > > > > > > > > > I noticed a similar issue when running the IGT test > > > > > > > kms_cursor_crc. For > > > > > > > example, a subtest that passes on the first run (alpha-opaque) > > > > > > > fails on > > > > > > > the second due to a kind of busy waiting in subtest preparation > > > > > > > (the > > > > > > > subtest fails before actually running). > > > > > > > > > > > > > > In addition, in the same test, the dpms subtest started to fail > > > > > > > since > > > > > > > the commit that change from wait_for_vblanks to > > > > > > > wait_for_flip_done. By > > > > > > > reverting this commit, the dpms subtest passes again and the > > > > > > > sequential > > > > > > > subtests return to normal. > > > > > > > > > > > > > > I am trying to figure out what's missing from using flip_done op > > > > > > > on > > > > > > > vkms, since I am also interested in solving this problem and I > > > > > > > understand that the change for flip_done has been discussed in > > > > > > > the past. > > > > > > > > > > > > > > Do you have any idea? > > > > > > > > > > > > Uh, not at all. This is indeed rather surprising ... > > > > > > > > > > > > What exactly is the failure mode when running a test the 2nd time? > > > > > > Full > > > > > > igt logs might give me an idea. But yeah this is kinda surprising. > > > > > > > > > > Hi Daniel, > > > > > > > > > > This is the IGT log of the 2nd run of kms_cursor_crc/alpha-opaque: > > > > > > > > > > IGT-Version: 1.25-NO-GIT (x86_64) (Linux: 5.8.0-rc2-DRM+ x86_64) > > > > > Force option used: Using driver vkms > > > > > Starting subtest: pipe-A-cursor-alpha-opaque > > > > > Timed out: Opening crc fd, and poll for first CRC. > > > > > Subtest pipe-A-cursor-alpha-opaque failed. > > > > > DEBUG > > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: set_pipe(A) > > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: Selecting > > > > > pipe A > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > igt_create_fb_with_bo_size(width=1024, height=768, > > > > > format=XR24(0x34325258), modifier=0x0, size=0) > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > igt_create_fb_with_bo_size(handle=1, pitch=4096) > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: Test requirement passed: > > > > > cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > igt_create_fb_with_bo_size(width=1024, height=768, > > > > > format=XR24(0x34325258), modifier=0x0, size=0) > > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > > igt_create_fb_with_bo_size(handle=2, pitch=4096) > > > > >
Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
Quoting Chris Wilson (2020-07-15 13:21:43) > Quoting Daniel Vetter (2020-07-15 13:10:22) > > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote: > > > When waiting with a callback on the stack, we must remove the callback > > > upon wait completion. Since this will be notified by the fence signal > > > callback, the removal often contends with the fence->lock being held by > > > the signaler. We can look at the list entry to see if the callback was > > > already signaled before we take the contended lock. > > > > > > Signed-off-by: Chris Wilson > > > --- > > > drivers/dma-buf/dma-fence.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > index 8d5bdfce638e..b910d7bc0854 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, > > > struct dma_fence_cb *cb) > > > unsigned long flags; > > > bool ret; > > > > > > + if (list_empty(>node)) > > > > I was about to say "but the races" but then noticed that Paul fixed > > list_empty to use READ_ONCE like 5 years ago :-) > > I'm always going "when exactly do we need list_empty_careful()"? > > We can rule out a concurrent dma_fence_add_callback() for the same > dma_fence_cb, as that is a lost cause. So we only have to worry about > the concurrent list_del_init() from dma_fence_signal_locked(). So it's > the timing of > list_del_init(): WRITE_ONCE(list->next, list) > vs > READ_ONCE(list->next) == list > and we don't need to care about the trailing instructions in > list_del_init()... > > Wait that trailing instruction is actually important here if the > dma_fence_cb is on the stack, or other imminent free. > > Ok, this does need to be list_empty_careful! There's a further problem in that we call INIT_LIST_HEAD on the dma_fence_cb before the signal callback. So even if list_empty_careful() confirms the dma_fence_cb to be completely decoupled, the containing struct may still be inuse. -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
Quoting Daniel Vetter (2020-07-15 13:10:22) > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote: > > When waiting with a callback on the stack, we must remove the callback > > upon wait completion. Since this will be notified by the fence signal > > callback, the removal often contends with the fence->lock being held by > > the signaler. We can look at the list entry to see if the callback was > > already signaled before we take the contended lock. > > > > Signed-off-by: Chris Wilson > > --- > > drivers/dma-buf/dma-fence.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index 8d5bdfce638e..b910d7bc0854 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, > > struct dma_fence_cb *cb) > > unsigned long flags; > > bool ret; > > > > + if (list_empty(>node)) > > I was about to say "but the races" but then noticed that Paul fixed > list_empty to use READ_ONCE like 5 years ago :-) I'm always going "when exactly do we need list_empty_careful()"? We can rule out a concurrent dma_fence_add_callback() for the same dma_fence_cb, as that is a lost cause. So we only have to worry about the concurrent list_del_init() from dma_fence_signal_locked(). So it's the timing of list_del_init(): WRITE_ONCE(list->next, list) vs READ_ONCE(list->next) == list and we don't need to care about the trailing instructions in list_del_init()... Wait that trailing instruction is actually important here if the dma_fence_cb is on the stack, or other imminent free. Ok, this does need to be list_empty_careful! -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote: > When waiting with a callback on the stack, we must remove the callback > upon wait completion. Since this will be notified by the fence signal > callback, the removal often contends with the fence->lock being held by > the signaler. We can look at the list entry to see if the callback was > already signaled before we take the contended lock. > > Signed-off-by: Chris Wilson > --- > drivers/dma-buf/dma-fence.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 8d5bdfce638e..b910d7bc0854 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct > dma_fence_cb *cb) > unsigned long flags; > bool ret; > > + if (list_empty(>node)) I was about to say "but the races" but then noticed that Paul fixed list_empty to use READ_ONCE like 5 years ago :-) Reviewed-by: Daniel Vetter > + return false; > + > spin_lock_irqsave(fence->lock, flags); > > ret = !list_empty(>node); > -- > 2.20.1 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: remove redundant assignment to variable 'ret'
On Wed, Jul 15, 2020 at 03:05:59PM +0800, Jing Xiangfeng wrote:
> The variable ret has been assigned the value '-EINVAL'. The assignment
> in the if() is redundant. We can remove it.
Nope, that's not correct. Before this assignement ret is guaranteed to be
0.
-Daniel
>
> Signed-off-by: Jing Xiangfeng
> ---
> drivers/gpu/drm/drm_auth.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 800ac39f3213..6e1b502f2797 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -299,7 +299,6 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void
> *data,
>
> if (file_priv->master->lessor != NULL) {
> DRM_DEBUG_LEASE("Attempt to drop lessee %d as master\n",
> file_priv->master->lessee_id);
> - ret = -EINVAL;
> goto out_unlock;
> }
>
> --
> 2.17.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/7] drm: drm_atomic.h: delete duplicated word in comment
On Tue, Jul 14, 2020 at 10:23:43PM -0700, Randy Dunlap wrote:
> Drop doubled word "than" in a comment.
>
> Signed-off-by: Randy Dunlap
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
Entire series pushed to drm-misc-next, thanks for your patches. Should
still make it to 5.9.
Cheers, Daniel
> ---
> include/drm/drm_atomic.h |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-next-20200714.orig/include/drm/drm_atomic.h
> +++ linux-next-20200714/include/drm/drm_atomic.h
> @@ -103,7 +103,7 @@ struct drm_crtc_commit {
>*
>* Will be signalled when all hw register changes for this commit have
>* been written out. Especially when disabling a pipe this can be much
> - * later than than @flip_done, since that can signal already when the
> + * later than @flip_done, since that can signal already when the
>* screen goes black, whereas to fully shut down a pipe more register
>* I/O is required.
>*
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
On Wed, Jul 15, 2020 at 1:47 PM Daniel Stone wrote: > > Hi, > > On Wed, 15 Jul 2020 at 12:05, Bas Nieuwenhuizen > wrote: > > On Wed, Jul 15, 2020 at 12:34 PM Chris Wilson > > wrote: > > > Maybe now is the time to ask: are you using sw_sync outside of > > > validation? > > > > Yes, this is used as part of the Android stack on Chrome OS (need to > > see if ChromeOS specific, but > > https://source.android.com/devices/graphics/sync#sync_timeline > > suggests not) > > Android used to mandate it for their earlier iteration of release > fences, which was an empty/future fence having no guarantee of > eventual forward progress until someone committed work later on. For > example, when you committed a buffer to SF, it would give you an empty > 'release fence' for that buffer which would only be tied to work to > signal it when you committed your _next_ buffer, which might never > happen. They removed that because a) future fences were a bad idea, > and b) it was only ever useful if you assumed strictly > FIFO/round-robin return order which wasn't always true. > > So now it's been watered down to 'use this if you don't have a > hardware timeline', but why don't we work with Android people to get > that removed entirely? I think there's some testcases still using these, but most real fence testcases use vgem nowadays. So from an upstream pov there's indeed not much if anything holding us back from just deleting this all. And would probably be a good idea. Adding Rob and John for more of the android pov. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 19/25] drm/amdgpu: s/GFP_KERNEL/GFP_ATOMIC in scheduler code
On Wed, Jul 15, 2020 at 11:17 AM Christian König wrote: > > Am 14.07.20 um 16:31 schrieb Daniel Vetter: > > On Tue, Jul 14, 2020 at 01:40:11PM +0200, Christian König wrote: > >> Am 14.07.20 um 12:49 schrieb Daniel Vetter: > >>> On Tue, Jul 07, 2020 at 10:12:23PM +0200, Daniel Vetter wrote: > My dma-fence lockdep annotations caught an inversion because we > allocate memory where we really shouldn't: > > kmem_cache_alloc+0x2b/0x6d0 > amdgpu_fence_emit+0x30/0x330 [amdgpu] > amdgpu_ib_schedule+0x306/0x550 [amdgpu] > amdgpu_job_run+0x10f/0x260 [amdgpu] > drm_sched_main+0x1b9/0x490 [gpu_sched] > kthread+0x12e/0x150 > > Trouble right now is that lockdep only validates against GFP_FS, which > would be good enough for shrinkers. But for mmu_notifiers we actually > need !GFP_ATOMIC, since they can be called from any page laundering, > even if GFP_NOFS or GFP_NOIO are set. > > I guess we should improve the lockdep annotations for > fs_reclaim_acquire/release. > > Ofc real fix is to properly preallocate this fence and stuff it into > the amdgpu job structure. But GFP_ATOMIC gets the lockdep splat out of > the way. > > v2: Two more allocations in scheduler paths. > > Frist one: > > __kmalloc+0x58/0x720 > amdgpu_vmid_grab+0x100/0xca0 [amdgpu] > amdgpu_job_dependency+0xf9/0x120 [amdgpu] > drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] > drm_sched_main+0xf9/0x490 [gpu_sched] > > Second one: > > kmem_cache_alloc+0x2b/0x6d0 > amdgpu_sync_fence+0x7e/0x110 [amdgpu] > amdgpu_vmid_grab+0x86b/0xca0 [amdgpu] > amdgpu_job_dependency+0xf9/0x120 [amdgpu] > drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] > drm_sched_main+0xf9/0x490 [gpu_sched] > > Cc: linux-me...@vger.kernel.org > Cc: linaro-mm-...@lists.linaro.org > Cc: linux-r...@vger.kernel.org > Cc: amd-...@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: Chris Wilson > Cc: Maarten Lankhorst > Cc: Christian König > Signed-off-by: Daniel Vetter > >>> Has anyone from amd side started looking into how to fix this properly? > >> Yeah I checked both and neither are any real problem. > > I'm confused ... do you mean "no real problem fixing them" or "not > > actually a real problem"? > > Both, at least the VMID stuff is trivial to avoid. > > And the fence allocation is extremely unlikely. E.g. when we allocate a > new one we previously most likely just freed one already. Yeah I think debugging we can avoid, just stop debugging if things get hung up like that. So mempool for the hw fences should be perfectly fine. The vmid stuff I don't really understand enough, but the hw fence stuff I think I grok, plus other scheduler users need that too from a quick look. I might be tackling that one (maybe put the mempool outright into drm_scheduler code as a helper), except if you have patches already in the works. vmid I'll leave to you guys :-) -Daniel > > > > >>> I looked a bit into fixing this with mempool, and the big guarantee we > >>> need is that > >>> - there's a hard upper limit on how many allocations we minimally need to > >>> guarantee forward progress. And the entire vmid allocation and > >>> amdgpu_sync_fence stuff kinda makes me question that's a valid > >>> assumption. > >> We do have hard upper limits for those. > >> > >> The VMID allocation could as well just return the fence instead of putting > >> it into the sync object IIRC. So that just needs some cleanup and can avoid > >> the allocation entirely. > > Yeah embedding should be simplest solution of all. > > > >> The hardware fence is limited by the number of submissions we can have > >> concurrently on the ring buffers, so also not a problem at all. > > Ok that sounds good. Wrt releasing the memory again, is that also done > > without any of the allocation-side locks held? I've seen some vmid manager > > somewhere ... > > Well that's the issue. We can't guarantee that for the hardware fence > memory since it could be that we hold another reference during debugging > IIRC. > > Still looking if and how we could fix this. But as I said this problem > is so extremely unlikely. > > Christian. > > > -Daniel > > > >> Regards, > >> Christian. > >> > >>> - mempool_free must be called without any locks in the way which are held > >>> while we call mempool_alloc. Otherwise we again have a nice deadlock > >>> with no forward progress. I tried auditing that, but got lost in > >>> amdgpu > >>> and scheduler code. Some lockdep annotations for mempool.c might help, > >>> but they're not going to catch everything. Plus it would be again > >>> manual > >>> annotations because this is yet another cross-release issue. So not > >>> sure > >>> that helps at all. > >>> > >>> iow, not
Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
Hi, On Wed, 15 Jul 2020 at 12:05, Bas Nieuwenhuizen wrote: > On Wed, Jul 15, 2020 at 12:34 PM Chris Wilson > wrote: > > Maybe now is the time to ask: are you using sw_sync outside of > > validation? > > Yes, this is used as part of the Android stack on Chrome OS (need to > see if ChromeOS specific, but > https://source.android.com/devices/graphics/sync#sync_timeline > suggests not) Android used to mandate it for their earlier iteration of release fences, which was an empty/future fence having no guarantee of eventual forward progress until someone committed work later on. For example, when you committed a buffer to SF, it would give you an empty 'release fence' for that buffer which would only be tied to work to signal it when you committed your _next_ buffer, which might never happen. They removed that because a) future fences were a bad idea, and b) it was only ever useful if you assumed strictly FIFO/round-robin return order which wasn't always true. So now it's been watered down to 'use this if you don't have a hardware timeline', but why don't we work with Android people to get that removed entirely? Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/4] drm/ttm: remove io_reserve_fastpath flag
Just use the use_io_reserve_lru flag. It doesn't make much
sense to have two flags.
Signed-off-by: Christian König
---
drivers/gpu/drm/nouveau/nouveau_bo.c | 1 -
drivers/gpu/drm/ttm/ttm_bo.c | 1 -
drivers/gpu/drm/ttm/ttm_bo_util.c| 8
include/drm/ttm/ttm_bo_driver.h | 2 --
4 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index a48652826f67..a1037478fa3f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -675,7 +675,6 @@ nouveau_bo_init_mem_type(struct ttm_bo_device *bdev,
uint32_t type,
}
man->func = _vram_manager;
- man->io_reserve_fastpath = false;
man->use_io_reserve_lru = true;
} else {
man->func = _bo_manager_func;
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 7be36b9996ed..8b9e7f62bea7 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1521,7 +1521,6 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned
type,
BUG_ON(type >= TTM_NUM_MEM_TYPES);
man = >man[type];
BUG_ON(man->has_type);
- man->io_reserve_fastpath = true;
man->use_io_reserve_lru = false;
mutex_init(>io_reserve_mutex);
spin_lock_init(>move_lock);
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 7d2c50fef456..6c05f4fd15ae 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -93,7 +93,7 @@ EXPORT_SYMBOL(ttm_bo_move_ttm);
int ttm_mem_io_lock(struct ttm_mem_type_manager *man, bool interruptible)
{
- if (likely(man->io_reserve_fastpath))
+ if (likely(!man->use_io_reserve_lru))
return 0;
if (interruptible)
@@ -105,7 +105,7 @@ int ttm_mem_io_lock(struct ttm_mem_type_manager *man, bool
interruptible)
void ttm_mem_io_unlock(struct ttm_mem_type_manager *man)
{
- if (likely(man->io_reserve_fastpath))
+ if (likely(!man->use_io_reserve_lru))
return;
mutex_unlock(>io_reserve_mutex);
@@ -136,7 +136,7 @@ int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
if (!bdev->driver->io_mem_reserve)
return 0;
- if (likely(man->io_reserve_fastpath))
+ if (likely(!man->use_io_reserve_lru))
return bdev->driver->io_mem_reserve(bdev, mem);
if (bdev->driver->io_mem_reserve &&
@@ -157,7 +157,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev,
{
struct ttm_mem_type_manager *man = >man[mem->mem_type];
- if (likely(man->io_reserve_fastpath))
+ if (likely(!man->use_io_reserve_lru))
return;
if (bdev->driver->io_mem_reserve &&
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 732167cad130..45522e4fbd6b 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -155,7 +155,6 @@ struct ttm_mem_type_manager_func {
* @use_io_reserve_lru: Use an lru list to try to unreserve io_mem_regions
* reserved by the TTM vm system.
* @io_reserve_lru: Optional lru list for unreserving io mem regions.
- * @io_reserve_fastpath: Only use bdev::driver::io_mem_reserve to obtain
* @move_lock: lock for move fence
* static information. bdev::driver::io_mem_free is never used.
* @lru: The lru list for this memory type.
@@ -184,7 +183,6 @@ struct ttm_mem_type_manager {
void *priv;
struct mutex io_reserve_mutex;
bool use_io_reserve_lru;
- bool io_reserve_fastpath;
spinlock_t move_lock;
/*
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/4] drm/ttm: cleanup io_mem interface with nouveau
Nouveau is the only user of this functionality and evicting io space
on -EAGAIN is really a misuse of the return code.
Instead switch to using -ENOSPC here which makes much more sense and
simplifies the code.
Signed-off-by: Christian König
---
drivers/gpu/drm/nouveau/nouveau_bo.c | 2 --
drivers/gpu/drm/ttm/ttm_bo_util.c| 4 ++--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 61355cfb7335..a48652826f67 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1505,8 +1505,6 @@ nouveau_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
struct ttm_mem_reg *reg)
if (ret != 1) {
if (WARN_ON(ret == 0))
return -EINVAL;
- if (ret == -ENOSPC)
- return -EAGAIN;
return ret;
}
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 5e0f3a9caedc..7d2c50fef456 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -116,7 +116,7 @@ static int ttm_mem_io_evict(struct ttm_mem_type_manager
*man)
struct ttm_buffer_object *bo;
if (!man->use_io_reserve_lru || list_empty(>io_reserve_lru))
- return -EAGAIN;
+ return -ENOSPC;
bo = list_first_entry(>io_reserve_lru,
struct ttm_buffer_object,
@@ -143,7 +143,7 @@ int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
mem->bus.io_reserved_count++ == 0) {
retry:
ret = bdev->driver->io_mem_reserve(bdev, mem);
- if (ret == -EAGAIN) {
+ if (ret == -ENOSPC) {
ret = ttm_mem_io_evict(man);
if (ret == 0)
goto retry;
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/4] drm: remove optional dummy function from drivers using TTM
Implementing those is completely unecessary.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 5 -
drivers/gpu/drm/drm_gem_vram_helper.c | 5 -
drivers/gpu/drm/qxl/qxl_ttm.c | 6 --
drivers/gpu/drm/radeon/radeon_ttm.c| 5 -
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 11 ---
5 files changed, 32 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 3df685287cc1..9c0f12f74af9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -836,10 +836,6 @@ static int amdgpu_ttm_io_mem_reserve(struct ttm_bo_device
*bdev, struct ttm_mem_
return 0;
}
-static void amdgpu_ttm_io_mem_free(struct ttm_bo_device *bdev, struct
ttm_mem_reg *mem)
-{
-}
-
static unsigned long amdgpu_ttm_io_mem_pfn(struct ttm_buffer_object *bo,
unsigned long page_offset)
{
@@ -1754,7 +1750,6 @@ static struct ttm_bo_driver amdgpu_bo_driver = {
.release_notify = _bo_release_notify,
.fault_reserve_notify = _bo_fault_reserve_notify,
.io_mem_reserve = _ttm_io_mem_reserve,
- .io_mem_free = _ttm_io_mem_free,
.io_mem_pfn = amdgpu_ttm_io_mem_pfn,
.access_memory = _ttm_access_memory,
.del_from_lru_notify = _vm_del_from_lru_notify
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
b/drivers/gpu/drm/drm_gem_vram_helper.c
index ad096600d89f..e62a2b68fe3a 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -1094,10 +1094,6 @@ static int bo_driver_io_mem_reserve(struct ttm_bo_device
*bdev,
return 0;
}
-static void bo_driver_io_mem_free(struct ttm_bo_device *bdev,
- struct ttm_mem_reg *mem)
-{ }
-
static struct ttm_bo_driver bo_driver = {
.ttm_tt_create = bo_driver_ttm_tt_create,
.ttm_tt_populate = ttm_pool_populate,
@@ -1107,7 +1103,6 @@ static struct ttm_bo_driver bo_driver = {
.evict_flags = bo_driver_evict_flags,
.move_notify = bo_driver_move_notify,
.io_mem_reserve = bo_driver_io_mem_reserve,
- .io_mem_free = bo_driver_io_mem_free,
};
/*
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 52eaa2d22745..a6e67149ef4a 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -129,11 +129,6 @@ int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
return 0;
}
-static void qxl_ttm_io_mem_free(struct ttm_bo_device *bdev,
- struct ttm_mem_reg *mem)
-{
-}
-
/*
* TTM backend functions.
*/
@@ -247,7 +242,6 @@ static struct ttm_bo_driver qxl_bo_driver = {
.evict_flags = _evict_flags,
.move = _bo_move,
.io_mem_reserve = _ttm_io_mem_reserve,
- .io_mem_free = _ttm_io_mem_free,
.move_notify = _bo_move_notify,
};
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index f4f1e63731a5..73085523fad7 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -457,10 +457,6 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device
*bdev, struct ttm_mem_
return 0;
}
-static void radeon_ttm_io_mem_free(struct ttm_bo_device *bdev, struct
ttm_mem_reg *mem)
-{
-}
-
/*
* TTM backend functions.
*/
@@ -774,7 +770,6 @@ static struct ttm_bo_driver radeon_bo_driver = {
.move_notify = _bo_move_notify,
.fault_reserve_notify = _bo_fault_reserve_notify,
.io_mem_reserve = _ttm_io_mem_reserve,
- .io_mem_free = _ttm_io_mem_free,
};
int radeon_ttm_init(struct radeon_device *rdev)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index fbcd11a7b215..bfd0c54ec30a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -815,15 +815,6 @@ static int vmw_ttm_io_mem_reserve(struct ttm_bo_device
*bdev, struct ttm_mem_reg
return 0;
}
-static void vmw_ttm_io_mem_free(struct ttm_bo_device *bdev, struct ttm_mem_reg
*mem)
-{
-}
-
-static int vmw_ttm_fault_reserve_notify(struct ttm_buffer_object *bo)
-{
- return 0;
-}
-
/**
* vmw_move_notify - TTM move_notify_callback
*
@@ -866,7 +857,5 @@ struct ttm_bo_driver vmw_bo_driver = {
.verify_access = vmw_verify_access,
.move_notify = vmw_move_notify,
.swap_notify = vmw_swap_notify,
- .fault_reserve_notify = _ttm_fault_reserve_notify,
.io_mem_reserve = _ttm_io_mem_reserve,
- .io_mem_free = _ttm_io_mem_free,
};
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/4] drm/ttm: cleanup coding style and implementation.
Only functional change is to always keep io_reserved_count up to date
for debugging even when it is not used otherwise.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo_util.c | 97 +++
1 file changed, 48 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 6c05f4fd15ae..7fb3e0bcbab4 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -115,39 +115,35 @@ static int ttm_mem_io_evict(struct ttm_mem_type_manager
*man)
{
struct ttm_buffer_object *bo;
- if (!man->use_io_reserve_lru || list_empty(>io_reserve_lru))
+ bo = list_first_entry_or_null(>io_reserve_lru,
+ struct ttm_buffer_object,
+ io_reserve_lru);
+ if (!bo)
return -ENOSPC;
- bo = list_first_entry(>io_reserve_lru,
- struct ttm_buffer_object,
- io_reserve_lru);
list_del_init(>io_reserve_lru);
ttm_bo_unmap_virtual_locked(bo);
-
return 0;
}
-
int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
struct ttm_mem_reg *mem)
{
struct ttm_mem_type_manager *man = >man[mem->mem_type];
- int ret = 0;
+ int ret;
+
+ if (mem->bus.io_reserved_count++)
+ return 0;
if (!bdev->driver->io_mem_reserve)
return 0;
- if (likely(!man->use_io_reserve_lru))
- return bdev->driver->io_mem_reserve(bdev, mem);
- if (bdev->driver->io_mem_reserve &&
- mem->bus.io_reserved_count++ == 0) {
retry:
- ret = bdev->driver->io_mem_reserve(bdev, mem);
- if (ret == -ENOSPC) {
- ret = ttm_mem_io_evict(man);
- if (ret == 0)
- goto retry;
- }
+ ret = bdev->driver->io_mem_reserve(bdev, mem);
+ if (ret == -ENOSPC) {
+ ret = ttm_mem_io_evict(man);
+ if (ret == 0)
+ goto retry;
}
return ret;
}
@@ -155,35 +151,31 @@ int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
void ttm_mem_io_free(struct ttm_bo_device *bdev,
struct ttm_mem_reg *mem)
{
- struct ttm_mem_type_manager *man = >man[mem->mem_type];
-
- if (likely(!man->use_io_reserve_lru))
+ if (--mem->bus.io_reserved_count)
return;
- if (bdev->driver->io_mem_reserve &&
- --mem->bus.io_reserved_count == 0 &&
- bdev->driver->io_mem_free)
- bdev->driver->io_mem_free(bdev, mem);
+ if (!bdev->driver->io_mem_free)
+ return;
+ bdev->driver->io_mem_free(bdev, mem);
}
int ttm_mem_io_reserve_vm(struct ttm_buffer_object *bo)
{
+ struct ttm_mem_type_manager *man = >bdev->man[bo->mem.mem_type];
struct ttm_mem_reg *mem = >mem;
int ret;
- if (!mem->bus.io_reserved_vm) {
- struct ttm_mem_type_manager *man =
- >bdev->man[mem->mem_type];
+ if (mem->bus.io_reserved_vm)
+ return 0;
- ret = ttm_mem_io_reserve(bo->bdev, mem);
- if (unlikely(ret != 0))
- return ret;
- mem->bus.io_reserved_vm = true;
- if (man->use_io_reserve_lru)
- list_add_tail(>io_reserve_lru,
- >io_reserve_lru);
- }
+ ret = ttm_mem_io_reserve(bo->bdev, mem);
+ if (unlikely(ret != 0))
+ return ret;
+ mem->bus.io_reserved_vm = true;
+ if (man->use_io_reserve_lru)
+ list_add_tail(>io_reserve_lru,
+ >io_reserve_lru);
return 0;
}
@@ -191,15 +183,17 @@ void ttm_mem_io_free_vm(struct ttm_buffer_object *bo)
{
struct ttm_mem_reg *mem = >mem;
- if (mem->bus.io_reserved_vm) {
- mem->bus.io_reserved_vm = false;
- list_del_init(>io_reserve_lru);
- ttm_mem_io_free(bo->bdev, mem);
- }
+ if (!mem->bus.io_reserved_vm)
+ return;
+
+ mem->bus.io_reserved_vm = false;
+ list_del_init(>io_reserve_lru);
+ ttm_mem_io_free(bo->bdev, mem);
}
-static int ttm_mem_reg_ioremap(struct ttm_bo_device *bdev, struct ttm_mem_reg
*mem,
- void **virtual)
+static int ttm_mem_reg_ioremap(struct ttm_bo_device *bdev,
+ struct ttm_mem_reg *mem,
+ void **virtual)
{
struct ttm_mem_type_manager *man = >man[mem->mem_type];
int ret;
@@ -216,9 +210,11 @@ static int ttm_mem_reg_ioremap(struct ttm_bo_device *bdev,
struct ttm_mem_reg *m
addr = mem->bus.addr;
} else {
if (mem->placement &
Re: [PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal
Reviewed-by: Bas Nieuwenhuizen
On Wed, Jul 15, 2020 at 12:04 PM Chris Wilson wrote:
>
> If a signal callback releases the sw_sync fence, that will trigger a
> deadlock as the timeline_fence_release recurses onto the fence->lock
> (used both for signaling and the the timeline tree).
>
> If we always hold a reference for an unsignaled fence held by the
> timeline, we no longer need to detach the fence from the timeline upon
> release. This is only possible since commit ea4d5a270b57
> ("dma-buf/sw_sync: force signal all unsignaled fences on dying timeline")
> where we introduced decoupling of the fences from the timeline upon release.
>
> Reported-by: Bas Nieuwenhuizen
> Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> Signed-off-by: Chris Wilson
> Cc: Sumit Semwal
> Cc: Chris Wilson
> Cc: Gustavo Padovan
> Cc: Christian König
> Cc:
> ---
> drivers/dma-buf/sw_sync.c | 32 +++-
> 1 file changed, 7 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 348b3a9170fa..4cc2ac03a84a 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -130,16 +130,7 @@ static const char
> *timeline_fence_get_timeline_name(struct dma_fence *fence)
>
> static void timeline_fence_release(struct dma_fence *fence)
> {
> - struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> struct sync_timeline *parent = dma_fence_parent(fence);
> - unsigned long flags;
> -
> - spin_lock_irqsave(fence->lock, flags);
> - if (!list_empty(>link)) {
> - list_del(>link);
> - rb_erase(>node, >pt_tree);
> - }
> - spin_unlock_irqrestore(fence->lock, flags);
>
> sync_timeline_put(parent);
> dma_fence_free(fence);
> @@ -203,18 +194,11 @@ static void sync_timeline_signal(struct sync_timeline
> *obj, unsigned int inc)
> if (!timeline_fence_signaled(>base))
> break;
>
> - list_del_init(>link);
> + list_del(>link);
> rb_erase(>node, >pt_tree);
>
> - /*
> -* A signal callback may release the last reference to this
> -* fence, causing it to be freed. That operation has to be
> -* last to avoid a use after free inside this loop, and must
> -* be after we remove the fence from the timeline in order to
> -* prevent deadlocking on timeline->lock inside
> -* timeline_fence_release().
> -*/
> dma_fence_signal_locked(>base);
> + dma_fence_put(>base);
> }
>
> spin_unlock_irq(>lock);
> @@ -261,13 +245,9 @@ static struct sync_pt *sync_pt_create(struct
> sync_timeline *obj,
> } else if (cmp < 0) {
> p = >rb_left;
> } else {
> - if (dma_fence_get_rcu(>base)) {
> - sync_timeline_put(obj);
> - kfree(pt);
> - pt = other;
> - goto unlock;
> - }
> - p = >rb_left;
> + dma_fence_put(>base);
> + pt = other;
> + goto unlock;
> }
> }
> rb_link_node(>node, parent, p);
> @@ -278,6 +258,7 @@ static struct sync_pt *sync_pt_create(struct
> sync_timeline *obj,
> parent ? _entry(parent, typeof(*pt),
> node)->link : >pt_list);
> }
> unlock:
> + dma_fence_get(>base); /* keep a ref for the timeline */
> spin_unlock_irq(>lock);
>
> return pt;
> @@ -316,6 +297,7 @@ static int sw_sync_debugfs_release(struct inode *inode,
> struct file *file)
> list_for_each_entry_safe(pt, next, >pt_list, link) {
> dma_fence_set_error(>base, -ENOENT);
> dma_fence_signal_locked(>base);
> + dma_fence_put(>base);
> }
>
> spin_unlock_irq(>lock);
> --
> 2.20.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: sw_sync deadlock avoidance, take 3
On Wed, Jul 15, 2020 at 12:34 PM Chris Wilson wrote: > > Quoting Bas Nieuwenhuizen (2020-07-15 11:23:35) > > Hi Chris, > > > > My concern with going in this direction was that we potentially allow > > an application to allocate a lot of kernel memory but not a lot of fds > > by creating lots of fences and then closing the fds but never > > signaling them. Is that not an issue? > > I did look to see if there was a quick way we could couple into the > sync_file release itself to remove the syncpt from the timeline, but > decided that for a debug feature, it wasn't a pressing concern. > > Maybe now is the time to ask: are you using sw_sync outside of > validation? Yes, this is used as part of the Android stack on Chrome OS (need to see if ChromeOS specific, but https://source.android.com/devices/graphics/sync#sync_timeline suggests not) > -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
When waiting with a callback on the stack, we must remove the callback upon wait completion. Since this will be notified by the fence signal callback, the removal often contends with the fence->lock being held by the signaler. We can look at the list entry to see if the callback was already signaled before we take the contended lock. Signed-off-by: Chris Wilson --- drivers/dma-buf/dma-fence.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 8d5bdfce638e..b910d7bc0854 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb) unsigned long flags; bool ret; + if (list_empty(>node)) + return false; + spin_lock_irqsave(fence->lock, flags); ret = !list_empty(>node); -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()
Rearrange the code to pull the operations beore the fence->lock critical
section, and remove a small amount of redundancy:
Function old new delta
dma_fence_add_callback 156 145 -11
Signed-off-by: Chris Wilson
---
drivers/dma-buf/dma-fence.c | 26 +++---
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 656e9ac2d028..8d5bdfce638e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -348,29 +348,25 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
dma_fence_func_t func)
{
- unsigned long flags;
- int ret = 0;
+ int ret = -ENOENT;
if (WARN_ON(!fence || !func))
return -EINVAL;
- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags)) {
- INIT_LIST_HEAD(>node);
- return -ENOENT;
- }
+ cb->func = func;
+ INIT_LIST_HEAD(>node);
- spin_lock_irqsave(fence->lock, flags);
+ if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags)) {
+ unsigned long flags;
- if (__dma_fence_enable_signaling(fence)) {
- cb->func = func;
- list_add_tail(>node, >cb_list);
- } else {
- INIT_LIST_HEAD(>node);
- ret = -ENOENT;
+ spin_lock_irqsave(fence->lock, flags);
+ if (__dma_fence_enable_signaling(fence)) {
+ list_add_tail(>node, >cb_list);
+ ret = 0;
+ }
+ spin_unlock_irqrestore(fence->lock, flags);
}
- spin_unlock_irqrestore(fence->lock, flags);
-
return ret;
}
EXPORT_SYMBOL(dma_fence_add_callback);
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: sw_sync deadlock avoidance, take 3
Quoting Bas Nieuwenhuizen (2020-07-15 11:23:35) > Hi Chris, > > My concern with going in this direction was that we potentially allow > an application to allocate a lot of kernel memory but not a lot of fds > by creating lots of fences and then closing the fds but never > signaling them. Is that not an issue? I did look to see if there was a quick way we could couple into the sync_file release itself to remove the syncpt from the timeline, but decided that for a debug feature, it wasn't a pressing concern. Maybe now is the time to ask: are you using sw_sync outside of validation? -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: sw_sync deadlock avoidance, take 3
Hi, On Wed, 15 Jul 2020 at 11:23, Bas Nieuwenhuizen wrote: > My concern with going in this direction was that we potentially allow > an application to allocate a lot of kernel memory but not a lot of fds > by creating lots of fences and then closing the fds but never > signaling them. Is that not an issue? sw_sync is a userspace DoS mechanism by design - if someone wants to enable and use it, they have bigger problems than unbounded memory allocations. Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: sw_sync deadlock avoidance, take 3
Hi Chris, My concern with going in this direction was that we potentially allow an application to allocate a lot of kernel memory but not a lot of fds by creating lots of fences and then closing the fds but never signaling them. Is that not an issue? - Bas On Wed, Jul 15, 2020 at 12:04 PM Chris Wilson wrote: > > dma_fence_release() objects to a fence being freed before it is > signaled, so instead of playing fancy tricks to avoid handling dying > requests, let's keep the syncpt alive until signaled. This neatly > removes the issue with having to decouple the syncpt from the timeline > upon fence release. > -Chris > > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] dma-buf/selftests: Add locking selftests for sw_sync
While sw_sync is purely a debug facility for userspace to create fences
and timelines it can control, nevertheless it has some tricky locking
semantics of its own. In particular, Bas Nieuwenhuizen reported that we
had reintroduced a deadlock if a signal callback attempted to destroy
the fence. So let's add a few trivial selftests to make sure that once
fixed again, it stays fixed.
Signed-off-by: Chris Wilson
Cc: Bas Nieuwenhuizen
Reviewed-by: Bas Nieuwenhuizen
---
drivers/dma-buf/Makefile | 3 +-
drivers/dma-buf/selftests.h | 1 +
drivers/dma-buf/st-sw_sync.c | 279 +++
drivers/dma-buf/sw_sync.c| 39 +
drivers/dma-buf/sync_debug.h | 8 +
5 files changed, 329 insertions(+), 1 deletion(-)
create mode 100644 drivers/dma-buf/st-sw_sync.c
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 995e05f609ff..9be4d4611609 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_UDMABUF) += udmabuf.o
dmabuf_selftests-y := \
selftest.o \
st-dma-fence.o \
- st-dma-fence-chain.o
+ st-dma-fence-chain.o \
+ st-sw_sync.o
obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o
diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h
index bc8cea67bf1e..232499a24872 100644
--- a/drivers/dma-buf/selftests.h
+++ b/drivers/dma-buf/selftests.h
@@ -12,3 +12,4 @@
selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */
selftest(dma_fence, dma_fence)
selftest(dma_fence_chain, dma_fence_chain)
+selftest(sw_sync, sw_sync)
diff --git a/drivers/dma-buf/st-sw_sync.c b/drivers/dma-buf/st-sw_sync.c
new file mode 100644
index ..145fd330f1c6
--- /dev/null
+++ b/drivers/dma-buf/st-sw_sync.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sync_debug.h"
+#include "selftest.h"
+
+static int sanitycheck(void *arg)
+{
+ struct sync_timeline *tl;
+ struct dma_fence *f;
+ int err = -ENOMEM;
+
+ /* Quick check we can create the timeline and syncpt */
+
+ tl = st_sync_timeline_create("mock");
+ if (!tl)
+ return -ENOMEM;
+
+ f = st_sync_pt_create(tl, 1);
+ if (!f)
+ goto out;
+
+ dma_fence_signal(f);
+ dma_fence_put(f);
+
+ err = 0;
+out:
+ st_sync_timeline_put(tl);
+ return err;
+}
+
+static int signal(void *arg)
+{
+ struct sync_timeline *tl;
+ struct dma_fence *f;
+ int err = -EINVAL;
+
+ /* Check that the syncpt fence is signaled when the timeline advances */
+
+ tl = st_sync_timeline_create("mock");
+ if (!tl)
+ return -ENOMEM;
+
+ f = st_sync_pt_create(tl, 1);
+ if (!f) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ if (dma_fence_is_signaled(f)) {
+ pr_err("syncpt:%lld signaled too early\n", f->seqno);
+ goto out_fence;
+ }
+
+ st_sync_timeline_signal(tl, 1);
+
+ if (!dma_fence_is_signaled(f)) {
+ pr_err("syncpt:%lld not signaled after increment\n", f->seqno);
+ goto out_fence;
+ }
+
+ err = 0;
+out_fence:
+ dma_fence_signal(f);
+ dma_fence_put(f);
+out:
+ st_sync_timeline_put(tl);
+ return err;
+}
+
+struct cb_destroy {
+ struct dma_fence_cb cb;
+ struct dma_fence *f;
+};
+
+static void cb_destroy(struct dma_fence *fence, struct dma_fence_cb *_cb)
+{
+ struct cb_destroy *cb = container_of(_cb, typeof(*cb), cb);
+
+ pr_info("syncpt:%llx destroying syncpt:%llx\n",
+ fence->seqno, cb->f->seqno);
+ dma_fence_put(cb->f);
+ cb->f = NULL;
+}
+
+static int cb_autodestroy(void *arg)
+{
+ struct sync_timeline *tl;
+ struct cb_destroy cb;
+ int err = -EINVAL;
+
+ /* Check that we can drop the final syncpt reference from a callback */
+
+ tl = st_sync_timeline_create("mock");
+ if (!tl)
+ return -ENOMEM;
+
+ cb.f = st_sync_pt_create(tl, 1);
+ if (!cb.f) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ if (dma_fence_add_callback(cb.f, , cb_destroy)) {
+ pr_err("syncpt:%lld signaled before increment\n", cb.f->seqno);
+ goto out;
+ }
+
+ st_sync_timeline_signal(tl, 1);
+ if (cb.f) {
+ pr_err("syncpt:%lld callback not run\n", cb.f->seqno);
+ dma_fence_put(cb.f);
+ goto out;
+ }
+
+ err = 0;
+out:
+ st_sync_timeline_put(tl);
+ return err;
+}
+
+static int cb_destroy_12(void *arg)
+{
+ struct sync_timeline *tl;
+ struct cb_destroy cb;
+ struct dma_fence *f;
+ int err = -EINVAL;
+
+ /* Check that we can drop some other syncpt reference
[PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal
If a signal callback releases the sw_sync fence, that will trigger a
deadlock as the timeline_fence_release recurses onto the fence->lock
(used both for signaling and the the timeline tree).
If we always hold a reference for an unsignaled fence held by the
timeline, we no longer need to detach the fence from the timeline upon
release. This is only possible since commit ea4d5a270b57
("dma-buf/sw_sync: force signal all unsignaled fences on dying timeline")
where we introduced decoupling of the fences from the timeline upon release.
Reported-by: Bas Nieuwenhuizen
Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
Signed-off-by: Chris Wilson
Cc: Sumit Semwal
Cc: Chris Wilson
Cc: Gustavo Padovan
Cc: Christian König
Cc:
---
drivers/dma-buf/sw_sync.c | 32 +++-
1 file changed, 7 insertions(+), 25 deletions(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 348b3a9170fa..4cc2ac03a84a 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -130,16 +130,7 @@ static const char *timeline_fence_get_timeline_name(struct
dma_fence *fence)
static void timeline_fence_release(struct dma_fence *fence)
{
- struct sync_pt *pt = dma_fence_to_sync_pt(fence);
struct sync_timeline *parent = dma_fence_parent(fence);
- unsigned long flags;
-
- spin_lock_irqsave(fence->lock, flags);
- if (!list_empty(>link)) {
- list_del(>link);
- rb_erase(>node, >pt_tree);
- }
- spin_unlock_irqrestore(fence->lock, flags);
sync_timeline_put(parent);
dma_fence_free(fence);
@@ -203,18 +194,11 @@ static void sync_timeline_signal(struct sync_timeline
*obj, unsigned int inc)
if (!timeline_fence_signaled(>base))
break;
- list_del_init(>link);
+ list_del(>link);
rb_erase(>node, >pt_tree);
- /*
-* A signal callback may release the last reference to this
-* fence, causing it to be freed. That operation has to be
-* last to avoid a use after free inside this loop, and must
-* be after we remove the fence from the timeline in order to
-* prevent deadlocking on timeline->lock inside
-* timeline_fence_release().
-*/
dma_fence_signal_locked(>base);
+ dma_fence_put(>base);
}
spin_unlock_irq(>lock);
@@ -261,13 +245,9 @@ static struct sync_pt *sync_pt_create(struct sync_timeline
*obj,
} else if (cmp < 0) {
p = >rb_left;
} else {
- if (dma_fence_get_rcu(>base)) {
- sync_timeline_put(obj);
- kfree(pt);
- pt = other;
- goto unlock;
- }
- p = >rb_left;
+ dma_fence_put(>base);
+ pt = other;
+ goto unlock;
}
}
rb_link_node(>node, parent, p);
@@ -278,6 +258,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline
*obj,
parent ? _entry(parent, typeof(*pt),
node)->link : >pt_list);
}
unlock:
+ dma_fence_get(>base); /* keep a ref for the timeline */
spin_unlock_irq(>lock);
return pt;
@@ -316,6 +297,7 @@ static int sw_sync_debugfs_release(struct inode *inode,
struct file *file)
list_for_each_entry_safe(pt, next, >pt_list, link) {
dma_fence_set_error(>base, -ENOENT);
dma_fence_signal_locked(>base);
+ dma_fence_put(>base);
}
spin_unlock_irq(>lock);
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
sw_sync deadlock avoidance, take 3
dma_fence_release() objects to a fence being freed before it is signaled, so instead of playing fancy tricks to avoid handling dying requests, let's keep the syncpt alive until signaled. This neatly removes the issue with having to decouple the syncpt from the timeline upon fence release. -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
> syzbot is reporting general protection fault in bitfill_aligned() [1]
> caused by integer underflow in bit_clear_margins(). The cause of this
> problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
>
> If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
> is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
> bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
> info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
> try to overrun the __iomem region and causes general protection fault.
>
> Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception. Since cols and lines are calculated as
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
>
> in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
> and var.yres < vc->vc_font.height makes rows = 0. This means that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, );
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, );
>
> easily reproduces integer underflow bug explained above.
>
> Of course, callers of vc_resize() are not handling vc_do_resize() failure
> is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
> as a band-aid workaround, this patch checks integer underflow in
> "struct fbcon_ops"->clear_margins call, assuming that
> vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
> cause integer overflow.
>
> [1]
> https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
>
> Reported-and-tested-by: syzbot
>
> Signed-off-by: Tetsuo Handa
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c
> b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
> fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
^^
If you choose a very high positive "rw" then this addition can overflow.
info->var.xoffset comes from the user and I don't think it's checked...
regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/3] dma-buf/sw_sync: Separate signal/timeline locks
Still Reviewed-by: Bas Nieuwenhuizen
On Tue, Jul 14, 2020 at 11:24 PM Chris Wilson wrote:
>
> Since we decouple the sync_pt from the timeline tree upon release, in
> order to allow releasing the sync_pt from a signal callback we need to
> separate the sync_pt signaling lock from the timeline tree lock.
>
> v2: Mark up the unlocked read of the current timeline value.
> v3: Store a timeline pointer in the sync_pt as we cannot use the common
> fence->lock trick to find our parent anymore.
>
> Suggested-by: Bas Nieuwenhuizen
> Signed-off-by: Chris Wilson
> Cc: Bas Nieuwenhuizen
> ---
> drivers/dma-buf/sw_sync.c| 40 +---
> drivers/dma-buf/sync_debug.c | 2 +-
> drivers/dma-buf/sync_debug.h | 13 +++-
> 3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 807c82148062..17a5c1a3b7ce 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -123,33 +123,39 @@ static const char
> *timeline_fence_get_driver_name(struct dma_fence *fence)
>
> static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
> {
> - struct sync_timeline *parent = dma_fence_parent(fence);
> -
> - return parent->name;
> + return sync_timeline(fence)->name;
> }
>
> static void timeline_fence_release(struct dma_fence *fence)
> {
> struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> - struct sync_timeline *parent = dma_fence_parent(fence);
> - unsigned long flags;
> + struct sync_timeline *parent = pt->timeline;
>
> - spin_lock_irqsave(fence->lock, flags);
> if (!list_empty(>link)) {
> - list_del(>link);
> - rb_erase(>node, >pt_tree);
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> + if (!list_empty(>link)) {
> + list_del(>link);
> + rb_erase(>node, >pt_tree);
> + }
> + spin_unlock_irqrestore(>lock, flags);
> }
> - spin_unlock_irqrestore(fence->lock, flags);
>
> sync_timeline_put(parent);
> dma_fence_free(fence);
> }
>
> -static bool timeline_fence_signaled(struct dma_fence *fence)
> +static int timeline_value(struct dma_fence *fence)
> {
> - struct sync_timeline *parent = dma_fence_parent(fence);
> + return READ_ONCE(sync_timeline(fence)->value);
> +}
>
> - return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
> +static bool timeline_fence_signaled(struct dma_fence *fence)
> +{
> + return !__dma_fence_is_later(fence->seqno,
> +timeline_value(fence),
> +fence->ops);
> }
>
> static bool timeline_fence_enable_signaling(struct dma_fence *fence)
> @@ -166,9 +172,7 @@ static void timeline_fence_value_str(struct dma_fence
> *fence,
> static void timeline_fence_timeline_value_str(struct dma_fence *fence,
> char *str, int size)
> {
> - struct sync_timeline *parent = dma_fence_parent(fence);
> -
> - snprintf(str, size, "%d", parent->value);
> + snprintf(str, size, "%d", timeline_value(fence));
> }
>
> static const struct dma_fence_ops timeline_fence_ops = {
> @@ -252,12 +256,14 @@ static struct sync_pt *sync_pt_create(struct
> sync_timeline *obj,
> return NULL;
>
> sync_timeline_get(obj);
> - dma_fence_init(>base, _fence_ops, >lock,
> + spin_lock_init(>lock);
> + dma_fence_init(>base, _fence_ops, >lock,
>obj->context, value);
> INIT_LIST_HEAD(>link);
> + pt->timeline = obj;
>
> spin_lock_irq(>lock);
> - if (!dma_fence_is_signaled_locked(>base)) {
> + if (!dma_fence_is_signaled(>base)) {
> struct rb_node **p = >pt_tree.rb_node;
> struct rb_node *parent = NULL;
>
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 101394f16930..2188ee17e889 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -65,7 +65,7 @@ static const char *sync_status_str(int status)
> static void sync_print_fence(struct seq_file *s,
> struct dma_fence *fence, bool show)
> {
> - struct sync_timeline *parent = dma_fence_parent(fence);
> + struct sync_timeline *parent = sync_timeline(fence);
> int status;
>
> status = dma_fence_get_status_locked(fence);
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 6176e52ba2d7..56589dae2159 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -45,23 +45,26 @@ struct sync_timeline {
> struct list_headsync_timeline_list;
> };
>
> -static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
> -{
> -
Re: [PATCH 19/25] drm/amdgpu: s/GFP_KERNEL/GFP_ATOMIC in scheduler code
Am 14.07.20 um 16:31 schrieb Daniel Vetter: On Tue, Jul 14, 2020 at 01:40:11PM +0200, Christian König wrote: Am 14.07.20 um 12:49 schrieb Daniel Vetter: On Tue, Jul 07, 2020 at 10:12:23PM +0200, Daniel Vetter wrote: My dma-fence lockdep annotations caught an inversion because we allocate memory where we really shouldn't: kmem_cache_alloc+0x2b/0x6d0 amdgpu_fence_emit+0x30/0x330 [amdgpu] amdgpu_ib_schedule+0x306/0x550 [amdgpu] amdgpu_job_run+0x10f/0x260 [amdgpu] drm_sched_main+0x1b9/0x490 [gpu_sched] kthread+0x12e/0x150 Trouble right now is that lockdep only validates against GFP_FS, which would be good enough for shrinkers. But for mmu_notifiers we actually need !GFP_ATOMIC, since they can be called from any page laundering, even if GFP_NOFS or GFP_NOIO are set. I guess we should improve the lockdep annotations for fs_reclaim_acquire/release. Ofc real fix is to properly preallocate this fence and stuff it into the amdgpu job structure. But GFP_ATOMIC gets the lockdep splat out of the way. v2: Two more allocations in scheduler paths. Frist one: __kmalloc+0x58/0x720 amdgpu_vmid_grab+0x100/0xca0 [amdgpu] amdgpu_job_dependency+0xf9/0x120 [amdgpu] drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] drm_sched_main+0xf9/0x490 [gpu_sched] Second one: kmem_cache_alloc+0x2b/0x6d0 amdgpu_sync_fence+0x7e/0x110 [amdgpu] amdgpu_vmid_grab+0x86b/0xca0 [amdgpu] amdgpu_job_dependency+0xf9/0x120 [amdgpu] drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] drm_sched_main+0xf9/0x490 [gpu_sched] Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org Cc: linux-r...@vger.kernel.org Cc: amd-...@lists.freedesktop.org Cc: intel-...@lists.freedesktop.org Cc: Chris Wilson Cc: Maarten Lankhorst Cc: Christian König Signed-off-by: Daniel Vetter Has anyone from amd side started looking into how to fix this properly? Yeah I checked both and neither are any real problem. I'm confused ... do you mean "no real problem fixing them" or "not actually a real problem"? Both, at least the VMID stuff is trivial to avoid. And the fence allocation is extremely unlikely. E.g. when we allocate a new one we previously most likely just freed one already. I looked a bit into fixing this with mempool, and the big guarantee we need is that - there's a hard upper limit on how many allocations we minimally need to guarantee forward progress. And the entire vmid allocation and amdgpu_sync_fence stuff kinda makes me question that's a valid assumption. We do have hard upper limits for those. The VMID allocation could as well just return the fence instead of putting it into the sync object IIRC. So that just needs some cleanup and can avoid the allocation entirely. Yeah embedding should be simplest solution of all. The hardware fence is limited by the number of submissions we can have concurrently on the ring buffers, so also not a problem at all. Ok that sounds good. Wrt releasing the memory again, is that also done without any of the allocation-side locks held? I've seen some vmid manager somewhere ... Well that's the issue. We can't guarantee that for the hardware fence memory since it could be that we hold another reference during debugging IIRC. Still looking if and how we could fix this. But as I said this problem is so extremely unlikely. Christian. -Daniel Regards, Christian. - mempool_free must be called without any locks in the way which are held while we call mempool_alloc. Otherwise we again have a nice deadlock with no forward progress. I tried auditing that, but got lost in amdgpu and scheduler code. Some lockdep annotations for mempool.c might help, but they're not going to catch everything. Plus it would be again manual annotations because this is yet another cross-release issue. So not sure that helps at all. iow, not sure what to do here. Ideas? Cheers, Daniel --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 8d84975885cd..a089a827fdfe 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -143,7 +143,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, uint32_t seq; int r; - fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL); + fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_ATOMIC); if (fence == NULL) return -ENOMEM; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index 267fa45ddb66..a333ca2d4ddd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++
Re: [Intel-gfx] [PATCH -next] drm/i915: Remove unused inline function drain_delayed_work()
Quoting YueHaibing (2020-07-15 04:21:04)
> It is not used since commit 058179e72e09 ("drm/i915/gt: Replace
> hangcheck by heartbeats")
>
> Signed-off-by: YueHaibing
Indeed, it is no more.
Reviewed-by: Chris Wilson
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.
Am 14.07.20 um 22:06 schrieb Chris Wilson:
From: Bas Nieuwenhuizen
Calltree:
timeline_fence_release
drm_sched_entity_wakeup
dma_fence_signal_locked
sync_timeline_signal
sw_sync_ioctl
Releasing the reference to the fence in the fence signal callback
seems reasonable to me, so this patch avoids the locking issue in
sw_sync.
d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists")
fixed the recursive locking issue but caused an use-after-free. Later
d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
fixed the use-after-free but reintroduced the recursive locking issue.
In this attempt we avoid the use-after-free still because the release
function still always locks, and outside of the locking region in the
signal function we have properly refcounted references.
We furthermore also avoid the recurive lock by making sure that either:
1) We have a properly refcounted reference, preventing the signal from
triggering the release function inside the locked region.
2) The refcount was already zero, and hence nobody will be able to trigger
the release function from the signal function.
v2: Move dma_fence_signal() into second loop in preparation to moving
the callback out of the timeline obj->lock.
Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
Cc: Sumit Semwal
Cc: Chris Wilson
Cc: Gustavo Padovan
Cc: Christian König
Cc:
Signed-off-by: Bas Nieuwenhuizen
Signed-off-by: Chris Wilson
Looks reasonable to me, but I'm not an expert on this container.
So patch is Acked-by: Christian König
Regards,
Christian.
---
drivers/dma-buf/sw_sync.c | 32 ++--
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 348b3a9170fa..807c82148062 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -192,6 +192,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
{
struct sync_pt *pt, *next;
+ LIST_HEAD(signal);
trace_sync_timeline(obj);
@@ -203,21 +204,32 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
if (!timeline_fence_signaled(>base))
break;
- list_del_init(>link);
- rb_erase(>node, >pt_tree);
-
/*
-* A signal callback may release the last reference to this
-* fence, causing it to be freed. That operation has to be
-* last to avoid a use after free inside this loop, and must
-* be after we remove the fence from the timeline in order to
-* prevent deadlocking on timeline->lock inside
-* timeline_fence_release().
+* We need to take a reference to avoid a release during
+* signalling (which can cause a recursive lock of obj->lock).
+* If refcount was already zero, another thread is already
+* taking care of destroying the fence.
*/
- dma_fence_signal_locked(>base);
+ if (!dma_fence_get_rcu(>base))
+ continue;
+
+ list_move_tail(>link, );
+ rb_erase(>node, >pt_tree);
}
spin_unlock_irq(>lock);
+
+ list_for_each_entry_safe(pt, next, , link) {
+ /*
+* This needs to be cleared before release, otherwise the
+* timeline_fence_release function gets confused about also
+* removing the fence from the pt_tree.
+*/
+ list_del_init(>link);
+
+ dma_fence_signal(>base);
+ dma_fence_put(>base);
+ }
}
/**
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail
On Tue, Jul 14, 2020 at 9:01 PM Melissa Wen wrote: > > On 07/14, Daniel Vetter wrote: > > On Tue, Jul 14, 2020 at 07:39:42AM -0300, Melissa Wen wrote: > > > On Tue, Jul 14, 2020 at 7:20 AM Melissa Wen wrote: > > > > > > > > On 07/13, Daniel Vetter wrote: > > > > > On Fri, Jul 10, 2020 at 02:05:33PM -0300, Melissa Wen wrote: > > > > > > On 07/02, Daniel Vetter wrote: > > > > > > > On Wed, Jul 01, 2020 at 03:31:34PM +, Sidong Yang wrote: > > > > > > > > there is an error when igt test is run continuously. > > > > > > > > vkms_atomic_commit_tail() > > > > > > > > need to call drm_atomic_helper_wait_for_vblanks() for give up > > > > > > > > ownership of > > > > > > > > vblank events. without this code, next atomic commit will not > > > > > > > > enable vblank > > > > > > > > and raise timeout error. > > > > > > > > > > > > > > > > Signed-off-by: Sidong Yang > > > > > > > > --- > > > > > > > > drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ > > > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > index 1e8b2169d834..10b9be67a068 100644 > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > > > > > > > > @@ -93,6 +93,8 @@ static void vkms_atomic_commit_tail(struct > > > > > > > > drm_atomic_state *old_state) > > > > > > > > flush_work(_state->composer_work); > > > > > > > > } > > > > > > > > > > > > > > > > + drm_atomic_helper_wait_for_vblanks(dev, old_state); > > > > > > > > > > > > > > Uh, we have a wait_for_flip_done right above, which should be > > > > > > > doing > > > > > > > exactly the same, but more precisely: Instead of just waiting for > > > > > > > any > > > > > > > vblank to happen, we wait for exactly the vblank corresponding to > > > > > > > this > > > > > > > atomic commit. So no races possible. If this is papering over > > > > > > > some issue, > > > > > > > then I think more debugging is needed. > > > > > > > > > > > > > > What exactly is going wrong here for you? > > > > > > > > > > > > Hi Daniel and Sidong, > > > > > > > > > > > > I noticed a similar issue when running the IGT test kms_cursor_crc. > > > > > > For > > > > > > example, a subtest that passes on the first run (alpha-opaque) > > > > > > fails on > > > > > > the second due to a kind of busy waiting in subtest preparation (the > > > > > > subtest fails before actually running). > > > > > > > > > > > > In addition, in the same test, the dpms subtest started to fail > > > > > > since > > > > > > the commit that change from wait_for_vblanks to wait_for_flip_done. > > > > > > By > > > > > > reverting this commit, the dpms subtest passes again and the > > > > > > sequential > > > > > > subtests return to normal. > > > > > > > > > > > > I am trying to figure out what's missing from using flip_done op on > > > > > > vkms, since I am also interested in solving this problem and I > > > > > > understand that the change for flip_done has been discussed in the > > > > > > past. > > > > > > > > > > > > Do you have any idea? > > > > > > > > > > Uh, not at all. This is indeed rather surprising ... > > > > > > > > > > What exactly is the failure mode when running a test the 2nd time? > > > > > Full > > > > > igt logs might give me an idea. But yeah this is kinda surprising. > > > > > > > > Hi Daniel, > > > > > > > > This is the IGT log of the 2nd run of kms_cursor_crc/alpha-opaque: > > > > > > > > IGT-Version: 1.25-NO-GIT (x86_64) (Linux: 5.8.0-rc2-DRM+ x86_64) > > > > Force option used: Using driver vkms > > > > Starting subtest: pipe-A-cursor-alpha-opaque > > > > Timed out: Opening crc fd, and poll for first CRC. > > > > Subtest pipe-A-cursor-alpha-opaque failed. > > > > DEBUG > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: set_pipe(A) > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: display: Virtual-1: Selecting pipe > > > > A > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > igt_create_fb_with_bo_size(width=1024, height=768, > > > > format=XR24(0x34325258), modifier=0x0, size=0) > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > igt_create_fb_with_bo_size(handle=1, pitch=4096) > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: Test requirement passed: > > > > cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > igt_create_fb_with_bo_size(width=1024, height=768, > > > > format=XR24(0x34325258), modifier=0x0, size=0) > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: > > > > igt_create_fb_with_bo_size(handle=2, pitch=4096) > > > > (kms_cursor_crc:2317) igt_fb-DEBUG: Test requirement passed: > > > > cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: Test requirement passed: plane_idx > > > > >= 0 && plane_idx < pipe->n_planes > > > > (kms_cursor_crc:2317) igt_kms-DEBUG: Test
Re: [Freedreno] [PATCH 0/9] drm/msm: Avoid possible infinite probe deferral and speed booting
On Tue 14 Jul 15:13 PDT 2020, Rob Herring wrote: > On Tue, Jul 14, 2020 at 10:33 AM Jeffrey Hugo > wrote: > > > > On Mon, Jul 13, 2020 at 5:50 PM Doug Anderson wrote: > > > > > > Hi, > > > > > > On Mon, Jul 13, 2020 at 1:25 PM Rob Herring wrote: > > > > > > > > On Mon, Jul 13, 2020 at 9:08 AM Doug Anderson > > > > wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Mon, Jul 13, 2020 at 7:11 AM Rob Herring > > > > > wrote: > > > > > > > > > > > > On Fri, Jul 10, 2020 at 5:02 PM Douglas Anderson > > > > > > wrote: > > > > > > > > > > > > > > I found that if I ever had a little mistake in my kernel config, > > > > > > > or device tree, or graphics driver that my system would sit in a > > > > > > > loop > > > > > > > at bootup trying again and again and again. An example log was: > > > > > > > > > > > > Why do we care about optimizing the error case? > > > > > > > > > > It actually results in a _fully_ infinite loop. That is: if anything > > > > > small causes a component of DRM to fail to probe then the whole system > > > > > doesn't boot because it just loops trying to probe over and over > > > > > again. The messages I put in the commit message are printed over and > > > > > over and over again. > > > > > > > > Sounds like a bug as that's not what should happen. > > > > > > > > If you defer during boot (initcalls), then you'll be on the deferred > > > > list until late_initcall and everything is retried. After > > > > late_initcall, only devices getting added should trigger probing. But > > > > maybe the adding and then removing a device is causing a re-trigger. > > > > > > Right, I'm nearly certain that the adding and then removing is causing > > > a re-trigger. I believe the loop would happen for any case where we > > > have a probe function that: > > > > > > 1. Adds devices. > > > 2. After adding devices it decides that it needs to defer. > > > 3. Removes the devices it added. > > > 4. Return -EPROBE_DEFER from its probe function. > > > > > > Specifically from what I know about how -EPROBE_DEFER works I'm not > > > sure how it wouldn't cause an infinite loop in that case. > > > > > > Perhaps the missing part of my explanation, though, is why it never > > > gets out of this infinite loop. In my case I purposely made the > > > bridge chip "ti-sn65dsi86.c" return an error (-EINVAL) in its probe > > > every time. Obviously I wasn't going to get a display up like this, > > > but I just wanted to not loop forever at bootup. I tracked down > > > exactly why we get an - EPROBE_DEFER over and over in this case. > > > > > > You can see it in msm_dsi_host_register(). If some components haven't > > > shown up when that function runs it will _always_ return > > > -EPROBE_DEFER. > > > > > > In my case, since I caused the bridge to fail to probe, those > > > components will _never_ show up. That means that > > > msm_dsi_host_register() will _always_ return -EPROBE_DEFER. > > > > > > I haven't dug through all the DRM code enough, but it doesn't > > > necessarily seem like the wrong behavior. If the bridge driver or a > > > panel was a module then (presumably) they could show up later and so > > > it should be OK for it to defer, right? > > > > > > So with all that, it doesn't really feel like this is a bug so much as > > > it's an unsupported use case. The current deferral logic simply can't > > > handle the case we're throwing at it. You cannot return -EPROBE_DEFER > > > if your probe function adds devices each time through the probe > > > function. > > > > > > Assuming all the above makes sense, that means we're stuck with: > > > > > > a) This patch series, which makes us not add devices. > > > > > > b) Some other patch series which rearchitects the MSM graphics stack > > > to not return -EPROBE_DEFER in this case. > > > > This isn't a MSM specific issue. This is an issue with how the DSI > > interface works, and how software is structured in Linux. I would > > expect that pretty much any DSI host in the kernel would have some > > version of this issue. > > > > The problem is that DSI is not "hot pluggable", so to give the DRM > > stack the info it needs, we need both the DSI controller (aka the MSM > > graphics stack in your case), and the thing it connects to (in your > > case, the TI bridge, normally the actual panel) because the DRM stack > > expects that if init completes, it has certain information > > (resolution, etc), and some of that information is in the DSI > > controller, and some of it is on the DSI device. > > Ah yes, DRM's lack of hot-plug and discrete component support... Is > that not improved with some of the bridge rework? > > Anyways, given there is a child dependency on the parent, I don't > think we should work-around DRM deficiencies in DT. > > BTW, There's also a deferred probe timeout you can use which stops > deferring probe some number of seconds after late_initcall. > I don't think we can rely on the deferred probe timeout, given that it was reverted back
Re: [PATCH] drm: sun4i: hdmi: Fix inverted HPD result
Chen-Yu Tsai writes:
> From: Chen-Yu Tsai
>
> When the extra HPD polling in sun4i_hdmi was removed, the result of
> HPD was accidentally inverted.
>
> Fix this by inverting the check.
>
> Fixes: bda8eaa6dee7 ("drm: sun4i: hdmi: Remove extra HPD polling")
> Signed-off-by: Chen-Yu Tsai
Tested-by: Mans Rullgard
> ---
>
> Sorry for the screw-up.
>
> ---
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index 557cbe5ab35f..2f2c9f0a1071 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -260,7 +260,7 @@ sun4i_hdmi_connector_detect(struct drm_connector
> *connector, bool force)
> unsigned long reg;
>
> reg = readl(hdmi->base + SUN4I_HDMI_HPD_REG);
> - if (reg & SUN4I_HDMI_HPD_HIGH) {
> + if (!(reg & SUN4I_HDMI_HPD_HIGH)) {
> cec_phys_addr_invalidate(hdmi->cec_adap);
> return connector_status_disconnected;
> }
> --
> 2.27.0
>
--
Måns Rullgård
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[drm/ttm] Memory corruption problem when ttm_tt_init() fails.
hi I've encountered [BUG: unable to handle kernel NULL pointer dereference at] which has call stack like your pattern2. And before this happended, I got a lot of memory allocation failure warnings. And my kernel is 3.10.0-327.62.1.el7.x86_64. Since, you mentioned it may be a bug of drm/tmm. So, I checked drm/ttm for possible patch to fix this problem, but found nothing. Could you please tell me is there any progress of this problem that you detected. Best wished! Jinxiang, Gu ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -next] drm/i915: Remove unused inline function drain_delayed_work()
It is not used since commit 058179e72e09 ("drm/i915/gt: Replace
hangcheck by heartbeats")
Signed-off-by: YueHaibing
---
drivers/gpu/drm/i915/i915_utils.h | 13 -
1 file changed, 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index b1c5955a52e1..54773371e6bd 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -266,19 +266,6 @@ static inline int list_is_last_rcu(const struct list_head
*list,
return READ_ONCE(list->next) == head;
}
-/*
- * Wait until the work is finally complete, even if it tries to postpone
- * by requeueing itself. Note, that if the worker never cancels itself,
- * we will spin forever.
- */
-static inline void drain_delayed_work(struct delayed_work *dw)
-{
- do {
- while (flush_delayed_work(dw))
- ;
- } while (delayed_work_pending(dw));
-}
-
static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
{
unsigned long j = msecs_to_jiffies(m);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [drm/ttm] Memory corruption problem when ttm_tt_init() fails.
On 2020/07/14 18:13, Gu Jinxiang wrote: > I've encountered [BUG: unable to handle kernel NULL pointer dereference at] > which has call stack like your pattern2. > And before this happended, I got a lot of memory allocation failure warnings. > And my kernel is 3.10.0-327.62.1.el7.x86_64. > > Since, you mentioned it may be a bug of drm/tmm. So, I checked drm/ttm for > possible patch to fix this problem, but found nothing. > Could you please tell me is there any progress of this problem that you > detected. I'm not aware of any progress on https://patchwork.kernel.org/patch/5681611/ . ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 5/7] drm: i915_drm.h: delete duplicated words in comments
Drop doubled words "the" and "be" in comments.
Signed-off-by: Randy Dunlap
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
---
include/uapi/drm/i915_drm.h |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- linux-next-20200714.orig/include/uapi/drm/i915_drm.h
+++ linux-next-20200714/include/uapi/drm/i915_drm.h
@@ -55,7 +55,7 @@ extern "C" {
* cause the related events to not be seen.
*
* I915_RESET_UEVENT - Event is generated just before an attempt to reset the
- * the GPU. The value supplied with the event is always 1. NOTE: Disable
+ * GPU. The value supplied with the event is always 1. NOTE: Disable
* reset via module parameter will cause this event to not be seen.
*/
#define I915_L3_PARITY_UEVENT "L3_PARITY_ERROR"
@@ -1934,7 +1934,7 @@ enum drm_i915_perf_property_id {
/**
* The value specifies which set of OA unit metrics should be
-* be configured, defining the contents of any OA unit reports.
+* configured, defining the contents of any OA unit reports.
*
* This property is available in perf revision 1.
*/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
Hello Tetsuo,
Can you try the a.out built from the original Syzkaller modified repro C
program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
// https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
// autogenerated by syzkaller (https://github.com/google/syzkaller)
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int verbose = 0;
void
dumpit(unsigned char *buf, int count, int addr)
{
int i, j;
char bp[256];
memset(bp, 0, 256);
for (i = j = 0; i < count; i++, j++) {
if (j == 16) {
j = 0;
printf("%s\n", bp);
memset(bp, 0, 256);
}
if (j == 0) {
sprintf([strlen(bp)], "%x: ", addr + i);
}
sprintf([strlen(bp)], "%02x ", buf[i]);
}
if (j != 0) {
printf("%s\n", bp);
}
}
uint64_t r[1] = {0x};
int main(int argc, char **argv)
{
syscall(__NR_mmap, 0x2000ul, 0x100ul, 3ul, 0x32ul, -1, 0);
intptr_t res = 0;
uint32_t activate = FB_ACTIVATE_NOW;
struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x21c0;
struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct
fb_var_screeninfo *));
char *vp = (char *)varp;
int i, sum, rtn, c;
extern char *optarg;
int limit = 0, passes = 0;
unsigned int start_address = 0;
unsigned int pattern = 0;
int breakit = 1;
while ((c = getopt (argc, argv, "a:v")) != -1)
switch (c)
{
case 'a':
activate = strtol(optarg, 0, 0);
break;
case 'v':
verbose++;
break;
default:
fprintf(stderr, "\nusage: %s [-a ] [-v]\n\n",
argv[0]);
return -1;
}
int fd = open("/dev/fb0", O_RDWR);
if (fd < 0) {
perror("open");
return 0;
}
printf("fd: %d\n", fd);
r[0] = fd;
rtn = syscall(__NR_ioctl, r[0], 0x4600ul, 0x21c0ul);
if (rtn < 0) {
perror("ioctl");
fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno);
}
if (verbose) {
printf("FBIOGET_VSCREENINFO:\n");
dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo),
0x21c0);
}
memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
fprintf(stderr, "activate = %d\n", activate);
varp->activate = activate;
if (verbose) {
printf("Pre FBIOPUT_VSCREENINFO:\n");
dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo),
0x21c0);
sleep(2);
}
rtn = syscall(__NR_ioctl, r[0], 0x4601ul, 0x21c0ul);
if (rtn < 0) {
perror("ioctl");
fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno);
}
limit = 2;
for (pattern = 0 ; pattern < 8 ; pattern++) {
unsigned long addr = 0x21c0;
passes = 0;
printf("\nWalk START addr = 0x%x, Break pattern=%x\n", addr,
pattern);
while (addr <= 0x225c) {
fprintf(stderr, " %d: addr=%x
\n", passes, addr);
memcpy(varp, starting_varp, sizeof(struct
fb_var_screeninfo));
*(uint32_t*)addr = pattern;
varp->activate = activate;
printf("Pre FBIOPUT_VSCREENINFO: pattern=%x\n",
pattern);
dumpit((unsigned char *)vp, sizeof(struct
fb_var_screeninfo), 0x21c0);
sleep(3);
rtn = syscall(__NR_ioctl, r[0], 0x4601ul, 0x21c0ul);
if (rtn < 0) {
perror("ioctl");
fprintf(stderr, "rtn=%d, errno=%d\n", rtn,
errno);
}
addr += 4;
passes++;
if (passes == limit)
break;
}
}
close(fd);
return 0;
}
With my patch it gets output like the following:
[root@localhost ~]# ./fb_break
fd: 3
activate = 0
Walk START addr = 0x21c0, Break pattern=0
0: addr=21c0
Pre FBIOPUT_VSCREENINFO: pattern=0
21c0: 00 00 00 00 00 03 00 00 00 04 00 00 00 03 00 00
21d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00
21e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00
21f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00
2200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00
2220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2230: 00 00 00 00 00 00 00 00 00
Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On 2020/07/15 2:15, George Kennedy wrote: > Can you try the a.out built from the original Syzkaller modified repro C > program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct. I'm not familiar with exploit code. What do you want to explain via this program? > struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x21c0; > struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct > fb_var_screeninfo *)); > memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo)); > memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo)); At least, I suspect there is a memory corruption bug in this program because of malloc()ing only sizeof(struct fb_var_screeninfo *) bytes. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/7] drm: drm_bridge.h: delete duplicated word in comment
Drop doubled word "should" in a comment.
Signed-off-by: Randy Dunlap
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
---
include/drm/drm_bridge.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-next-20200714.orig/include/drm/drm_bridge.h
+++ linux-next-20200714/include/drm/drm_bridge.h
@@ -475,7 +475,7 @@ struct drm_bridge_funcs {
* one of them should be provided.
*
* If drivers need to tweak _bridge_state.input_bus_cfg.flags or
-* _bridge_state.output_bus_cfg.flags it should should happen in
+* _bridge_state.output_bus_cfg.flags it should happen in
* this function. By default the _bridge_state.output_bus_cfg.flags
* field is set to the next bridge
* _bridge_state.input_bus_cfg.flags value or
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm: rockchip: add NV15, NV20 and NV30 support
Hi Jonas,
Am 07.07.20 um 00:30 schrieb Jonas Karlman:
Add support for displaying 10-bit 4:2:0 and 4:2:2 formats produced by the
Rockchip Video Decoder on RK322X, RK3288, RK3328, RK3368 and RK3399.
Also add support for 10-bit 4:4:4 format while at it.
V2: Added NV30 support
Signed-off-by: Jonas Karlman
Reviewed-by: Sandy Huang
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 29 +--
drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 1 +
drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 32 +
3 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c80f7d9fd13f..eb663e25ad9e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -261,6 +261,18 @@ static bool has_rb_swapped(uint32_t format)
}
}
+static bool is_fmt_10(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_NV15:
+ case DRM_FORMAT_NV20:
+ case DRM_FORMAT_NV30:
+ return true;
+ default:
+ return false;
+ }
+}
+
static enum vop_data_format vop_convert_format(uint32_t format)
{
switch (format) {
@@ -276,10 +288,13 @@ static enum vop_data_format vop_convert_format(uint32_t
format)
case DRM_FORMAT_BGR565:
return VOP_FMT_RGB565;
case DRM_FORMAT_NV12:
+ case DRM_FORMAT_NV15:
return VOP_FMT_YUV420SP;
case DRM_FORMAT_NV16:
+ case DRM_FORMAT_NV20:
return VOP_FMT_YUV422SP;
case DRM_FORMAT_NV24:
+ case DRM_FORMAT_NV30:
return VOP_FMT_YUV444SP;
default:
DRM_ERROR("unsupported format[%08x]\n", format);
@@ -922,7 +937,12 @@ static void vop_plane_atomic_update(struct drm_plane
*plane,
dsp_sty = dest->y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
dsp_st = dsp_sty << 16 | (dsp_stx & 0x);
- offset = (src->x1 >> 16) * fb->format->cpp[0];
+ if (fb->format->block_w[0])
+ offset = (src->x1 >> 16) * fb->format->char_per_block[0] /
+fb->format->block_w[0];
+ else
+ offset = (src->x1 >> 16) * fb->format->cpp[0];
+
offset += (src->y1 >> 16) * fb->pitches[0];
dma_addr = rk_obj->dma_addr + offset + fb->offsets[0];
@@ -948,6 +968,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
}
VOP_WIN_SET(vop, win, format, format);
+ VOP_WIN_SET(vop, win, fmt_10, is_fmt_10(fb->format->format));
VOP_WIN_SET(vop, win, yrgb_vir, DIV_ROUND_UP(fb->pitches[0], 4));
VOP_WIN_SET(vop, win, yrgb_mst, dma_addr);
VOP_WIN_YUV2YUV_SET(vop, win_yuv2yuv, y2r_en, is_yuv);
@@ -964,7 +985,11 @@ static void vop_plane_atomic_update(struct drm_plane
*plane,
uv_obj = fb->obj[1];
rk_uv_obj = to_rockchip_obj(uv_obj);
- offset = (src->x1 >> 16) * bpp / hsub;
+ if (fb->format->block_w[1])
+ offset = (src->x1 >> 16) * bpp /
+fb->format->block_w[1] / hsub;
+ else
+ offset = (src->x1 >> 16) * bpp / hsub;
offset += (src->y1 >> 16) * fb->pitches[1] / vsub;
dma_addr = rk_uv_obj->dma_addr + offset + fb->offsets[1];
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 4a2099cb582e..eab055d9b56d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -154,6 +154,7 @@ struct vop_win_phy {
struct vop_reg enable;
struct vop_reg gate;
struct vop_reg format;
+ struct vop_reg fmt_10;
struct vop_reg rb_swap;
struct vop_reg act_info;
struct vop_reg dsp_info;
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index 80053d91a301..2c55e1852c3d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -50,6 +50,23 @@ static const uint32_t formats_win_full[] = {
DRM_FORMAT_NV24,
};
+static const uint32_t formats_win_full_10[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV24,
+ DRM_FORMAT_NV15,
+ DRM_FORMAT_NV20,
+ DRM_FORMAT_NV30,
+};
+
static const uint64_t format_modifiers_win_full[] = {
DRM_FORMAT_MOD_LINEAR,
DRM_FORMAT_MOD_INVALID,
@@ -579,11 +596,12 @@ static const struct vop_scl_regs rk3288_win_full_scl = {
static const struct vop_win_phy rk3288_win01_data = {
.scl = _win_full_scl,
-
[PATCH 7/7] drm: drm_rect.h: delete duplicated word in comment
Drop doubled word "the" in a comment. Signed-off-by: Randy Dunlap Cc: David Airlie Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org --- include/drm/drm_rect.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-next-20200714.orig/include/drm/drm_rect.h +++ linux-next-20200714/include/drm/drm_rect.h @@ -180,7 +180,7 @@ static inline int drm_rect_height(const } /** - * drm_rect_visible - determine if the the rectangle is visible + * drm_rect_visible - determine if the rectangle is visible * @r: rectangle whose visibility is returned * * RETURNS: ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/msm: use kthread_create_worker instead of kthread_run
Use kthread_create_worker to simplify the code and optimise
the manager struct: msm_drm_thread. With this change, we
could remove struct element (struct task_struct *thread &
struct kthread_worker worker), instead, use one point (struct
kthread_worker *worker).
Signed-off-by: Bernard Zhao
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
drivers/gpu/drm/msm/msm_drv.c| 18 ++
drivers/gpu/drm/msm/msm_drv.h| 3 +--
3 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e15b42a780e0..c959c959021d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -396,7 +396,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
fevent->event = event;
fevent->crtc = crtc;
fevent->ts = ktime_get();
- kthread_queue_work(>event_thread[crtc_id].worker, >work);
+ kthread_queue_work(priv->event_thread[crtc_id].worker, >work);
}
void dpu_crtc_complete_commit(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f6ce40bf3699..82e79b82a594 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -238,10 +238,8 @@ static int msm_drm_uninit(struct device *dev)
/* clean up event worker threads */
for (i = 0; i < priv->num_crtcs; i++) {
- if (priv->event_thread[i].thread) {
- kthread_destroy_worker(>event_thread[i].worker);
- priv->event_thread[i].thread = NULL;
- }
+ if (priv->event_thread[i].worker)
+ kthread_destroy_worker(priv->event_thread[i].worker);
}
msm_gem_shrinker_cleanup(ddev);
@@ -504,19 +502,15 @@ static int msm_drm_init(struct device *dev, struct
drm_driver *drv)
for (i = 0; i < priv->num_crtcs; i++) {
/* initialize event thread */
priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
- kthread_init_worker(>event_thread[i].worker);
priv->event_thread[i].dev = ddev;
- priv->event_thread[i].thread =
- kthread_run(kthread_worker_fn,
- >event_thread[i].worker,
- "crtc_event:%d", priv->event_thread[i].crtc_id);
- if (IS_ERR(priv->event_thread[i].thread)) {
+ priv->event_thread[i].worker = kthread_create_worker(0,
+ "crtc_event:%d", priv->event_thread[i].crtc_id);
+ if (IS_ERR(priv->event_thread[i].worker)) {
DRM_DEV_ERROR(dev, "failed to create crtc_event
kthread\n");
- priv->event_thread[i].thread = NULL;
goto err_msm_uninit;
}
- ret = sched_setscheduler(priv->event_thread[i].thread,
+ ret = sched_setscheduler(priv->event_thread[i].worker->task,
SCHED_FIFO, );
if (ret)
dev_warn(dev, "event_thread set priority failed:%d\n",
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index e2d6a6056418..daf2f4e5548c 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -129,9 +129,8 @@ struct msm_display_info {
/* Commit/Event thread specific structure */
struct msm_drm_thread {
struct drm_device *dev;
- struct task_struct *thread;
unsigned int crtc_id;
- struct kthread_worker worker;
+ struct kthread_worker *worker;
};
struct msm_drm_private {
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
