Re: [PATCH v4 00/10] backlight: Replace struct fb_info in interfaces
Hello Thomas, On Tue, Mar 05, 2024 at 05:22:33PM +0100, Thomas Zimmermann wrote: > Backlight drivers implement struct backlight_ops.check_fb, which > uses struct fb_info in its interface. Replace the callback with one > that does not use fb_info. > > In DRM, we have several drivers that implement backlight support. By > including these drivers depend on . > At the same time, fbdev is deprecated for new drivers and likely to > be replaced on many systems. > > This patchset is part of a larger effort to implement the backlight > code without depending on fbdev. > > Patch 1 makes the backlight core match backlight and framebuffer > devices via struct fb_info.bl_dev. Patches 2 to 9 then go through > drivers and remove unnecessary implementations of check_fb. Finally, > patch 10 replaces the check_fb hook with controls_device, which > uses the framebuffer's Linux device instead of the framebuffer. I assume the merge plan for this series is via drm-misc in one go? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v2] drm/amdgpu: Clear the hotplug interrupt ack bit before hpd initialization
On Wed, 31 Jan 2024 15:57:03 +0800 Qiang Ma wrote: Hello everyone, please help review this patch. Qiang Ma > Problem: > The computer in the bios initialization process, unplug the HDMI > display, wait until the system up, plug in the HDMI display, did not > enter the hotplug interrupt function, the display is not bright. > > Fix: > After the above problem occurs, and the hpd ack interrupt bit is 1, > the interrupt should be cleared during hpd_init initialization so that > when the driver is ready, it can respond to the hpd interrupt > normally. > > Signed-off-by: Qiang Ma > --- > v2: > - Remove unused variable 'tmp' > - Fixed function spelling errors > > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 22 ++ > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 22 ++ > 4 files changed, 40 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c index > bb666cb7522e..12a8ba929a72 100644 --- > a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c +++ > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c @@ -51,6 +51,7 @@ > > static void dce_v10_0_set_display_funcs(struct amdgpu_device *adev); > static void dce_v10_0_set_irq_funcs(struct amdgpu_device *adev); > +static void dce_v10_0_hpd_int_ack(struct amdgpu_device *adev, int > hpd); > static const u32 crtc_offsets[] = { > CRTC0_REGISTER_OFFSET, > @@ -363,6 +364,7 @@ static void dce_v10_0_hpd_init(struct > amdgpu_device *adev) AMDGPU_HPD_DISCONNECT_INT_DELAY_IN_MS); > WREG32(mmDC_HPD_TOGGLE_FILT_CNTL + > hpd_offsets[amdgpu_connector->hpd.hpd], tmp); > + dce_v10_0_hpd_int_ack(adev, > amdgpu_connector->hpd.hpd); dce_v10_0_hpd_set_polarity(adev, > amdgpu_connector->hpd.hpd); amdgpu_irq_get(adev, &adev->hpd_irq, > amdgpu_connector->hpd.hpd); > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c index > 7af277f61cca..745e4fdffade 100644 --- > a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c +++ > b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c @@ -51,6 +51,7 @@ > > static void dce_v11_0_set_display_funcs(struct amdgpu_device *adev); > static void dce_v11_0_set_irq_funcs(struct amdgpu_device *adev); > +static void dce_v11_0_hpd_int_ack(struct amdgpu_device *adev, int > hpd); > static const u32 crtc_offsets[] = > { > @@ -387,6 +388,7 @@ static void dce_v11_0_hpd_init(struct > amdgpu_device *adev) AMDGPU_HPD_DISCONNECT_INT_DELAY_IN_MS); > WREG32(mmDC_HPD_TOGGLE_FILT_CNTL + > hpd_offsets[amdgpu_connector->hpd.hpd], tmp); > + dce_v11_0_hpd_int_ack(adev, > amdgpu_connector->hpd.hpd); dce_v11_0_hpd_set_polarity(adev, > amdgpu_connector->hpd.hpd); amdgpu_irq_get(adev, &adev->hpd_irq, > amdgpu_connector->hpd.hpd); } > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c index > 143efc37a17f..28c4a735716b 100644 --- > a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c +++ > b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c @@ -272,6 +272,21 @@ static > void dce_v6_0_hpd_set_polarity(struct amdgpu_device *adev, > WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd], tmp); } > > +static void dce_v6_0_hpd_int_ack(struct amdgpu_device *adev, > + int hpd) > +{ > + u32 tmp; > + > + if (hpd >= adev->mode_info.num_hpd) { > + DRM_DEBUG("invalid hdp %d\n", hpd); > + return; > + } > + > + tmp = RREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd]); > + tmp |= DC_HPD1_INT_CONTROL__DC_HPD1_INT_ACK_MASK; > + WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd], tmp); > +} > + > /** > * dce_v6_0_hpd_init - hpd setup callback. > * > @@ -311,6 +326,7 @@ static void dce_v6_0_hpd_init(struct > amdgpu_device *adev) continue; > } > > + dce_v6_0_hpd_int_ack(adev, > amdgpu_connector->hpd.hpd); dce_v6_0_hpd_set_polarity(adev, > amdgpu_connector->hpd.hpd); amdgpu_irq_get(adev, &adev->hpd_irq, > amdgpu_connector->hpd.hpd); } > @@ -3088,7 +3104,7 @@ static int dce_v6_0_hpd_irq(struct > amdgpu_device *adev, struct amdgpu_irq_src *source, > struct amdgpu_iv_entry *entry) > { > - uint32_t disp_int, mask, tmp; > + uint32_t disp_int, mask; > unsigned hpd; > > if (entry->src_data[0] >= adev->mode_info.num_hpd) { > @@ -3101,9 +3117,7 @@ static int dce_v6_0_hpd_irq(struct > amdgpu_device *adev, mask = interrupt_status_offsets[hpd].hpd; > > if (disp_int & mask) { > - tmp = RREG32(mmDC_HPD1_INT_CONTROL + > hpd_offsets[hpd]); > - tmp |= DC_HPD1_INT_CONTROL__DC_HPD1_INT_ACK_MASK; > - WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd], > tmp); > + dce_v6_0_hpd_int_ack(adev, hpd); > schedule_delayed_work(&adev->hotplug_work, 0); > DRM_DEBUG("IH: HPD%d\n", hpd + 1
[PATCH v18 8/9] drm/i915/display: Compute vrr_vsync params
Compute vrr_vsync_start/end, which sets the position for hardware to send the Vsync at a fixed position relative to the end of the Vblank. --v2: - Updated VSYNC_START/END macros to VRR_VSYNC_START/END. (Ankit) - Updated bit fields of VRR_VSYNC_START/END. (Ankit) --v3: - Add PIPE_CONF_CHECK_I(vrr.vsync_start/end). - Read/write vrr_vsync params only when we intend to send adaptive_sync sdp. --v4: - Use VRR_SYNC_START/END macros correctly. Signed-off-by: Mitul Golani --- drivers/gpu/drm/i915/display/intel_display.c | 2 ++ .../drm/i915/display/intel_display_types.h| 1 + drivers/gpu/drm/i915/display/intel_vrr.c | 30 +-- drivers/gpu/drm/i915/i915_reg.h | 7 + 4 files changed, 38 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 8f1d948408d3..fed4ed18d53b 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5377,6 +5377,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, PIPE_CONF_CHECK_I(vrr.flipline); PIPE_CONF_CHECK_I(vrr.pipeline_full); PIPE_CONF_CHECK_I(vrr.guardband); + PIPE_CONF_CHECK_I(vrr.vsync_start); + PIPE_CONF_CHECK_I(vrr.vsync_end); } #undef PIPE_CONF_CHECK_X diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 8a286751dc39..c2e08f641989 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1430,6 +1430,7 @@ struct intel_crtc_state { bool enable, in_range; u8 pipeline_full; u16 flipline, vmin, vmax, guardband; + u32 vsync_end, vsync_start; } vrr; /* Stream Splitter for eDP MSO */ diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c index eb5bd0743902..ed38fee196b8 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c @@ -9,6 +9,7 @@ #include "intel_de.h" #include "intel_display_types.h" #include "intel_vrr.h" +#include "intel_dp.h" bool intel_vrr_is_capable(struct intel_connector *connector) { @@ -113,6 +114,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, struct drm_i915_private *i915 = to_i915(crtc->base.dev); struct intel_connector *connector = to_intel_connector(conn_state->connector); + struct intel_dp *intel_dp = intel_attached_dp(connector); struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; const struct drm_display_info *info = &connector->base.display_info; int vmin, vmax; @@ -165,6 +167,15 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, if (crtc_state->uapi.vrr_enabled) { crtc_state->vrr.enable = true; crtc_state->mode_flags |= I915_MODE_FLAG_VRR; + + if (intel_dp_as_sdp_supported(intel_dp)) { + crtc_state->vrr.vsync_start = + (crtc_state->hw.adjusted_mode.crtc_vtotal - + crtc_state->hw.adjusted_mode.vsync_start); + crtc_state->vrr.vsync_end = + (crtc_state->hw.adjusted_mode.crtc_vtotal - + crtc_state->hw.adjusted_mode.vsync_end); + } } } @@ -204,6 +215,11 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state) intel_de_write(dev_priv, TRANS_VRR_VMAX(cpu_transcoder), crtc_state->vrr.vmax - 1); intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder), trans_vrr_ctl(crtc_state)); intel_de_write(dev_priv, TRANS_VRR_FLIPLINE(cpu_transcoder), crtc_state->vrr.flipline - 1); + + if (crtc_state->vrr.vsync_end && crtc_state->vrr.vsync_start) + intel_de_write(dev_priv, TRANS_VRR_VSYNC(cpu_transcoder), + VRR_VSYNC_END(crtc_state->hw.adjusted_mode.vsync_end) | + VRR_VSYNC_START(crtc_state->hw.adjusted_mode.vsync_start)); } void intel_vrr_send_push(const struct intel_crtc_state *crtc_state) @@ -264,7 +280,7 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; - u32 trans_vrr_ctl; + u32 trans_vrr_ctl, trans_vrr_vsync; trans_vrr_ctl = intel_de_read(dev_priv, TRANS_VRR_CTL(cpu_transcoder)); @@ -284,6 +300,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) crtc_state->vrr.vmin = intel_de_read(dev_priv, TRANS_VRR_VMIN(cpu_transcoder)) + 1;
Re: [PATCH v2 7/8] dt-bindings: xlnx: Add VTC and TPG bindings
On Tue, 12 Mar 2024 17:55:04 -0700, Anatoliy Klymenko wrote: > DO NOT MERGE. REFERENCE ONLY. > > Add binding for AMD/Xilinx Video Timing Controller and Test Pattern > Generator. > > Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node. > > Signed-off-by: Anatoliy Klymenko > --- > .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 ++ > .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 > include/dt-bindings/media/media-bus-format.h | 177 > + > 3 files changed, 329 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml:35:4: [warning] wrong indentation: expected 4 but found 3 (indentation) ./Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml:45:8: [warning] wrong indentation: expected 8 but found 7 (indentation) ./Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml:49:8: [warning] wrong indentation: expected 8 but found 7 (indentation) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml: xlnx,pixels-per-clock: missing type definition /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml: bus-format: missing type definition /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml: xlnx,bridge: missing type definition doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240312-dp-live-fmt-v2-7-a9c35dc5c...@amd.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
RE: [EXT] [PATCH v15 6/8] phy: freescale: Add DisplayPort/HDMI Combo-PHY driver for i.MX8MQ
Hi Alexander, Thanks for your combine HDMI and DP PHY driver into one driver. Basically, the HDMI and DP PHY initialize process and configuration same as V14. I have verify the DP and HDMI function with the patch set. I'm OK for the patch. B.R Sandor > > > From: Sandor Yu > > Add Cadence HDP-TX DisplayPort and HDMI PHY driver for i.MX8MQ. > > Cadence HDP-TX PHY could be put in either DP mode or > HDMI mode base on the configuration chosen. > DisplayPort or HDMI PHY mode is configured in the driver. > > Signed-off-by: Sandor Yu > Signed-off-by: Alexander Stein > --- > drivers/phy/freescale/Kconfig| 10 + > drivers/phy/freescale/Makefile |1 + > drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c | 1402 ++ > 3 files changed, 1413 insertions(+) > create mode 100644 drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c > > diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig > index 853958fb2c063..abacbe8ba8f46 100644 > --- a/drivers/phy/freescale/Kconfig > +++ b/drivers/phy/freescale/Kconfig > @@ -35,6 +35,16 @@ config PHY_FSL_IMX8M_PCIE > Enable this to add support for the PCIE PHY as found on > i.MX8M family of SOCs. > > +config PHY_FSL_IMX8MQ_HDPTX > + tristate "Freescale i.MX8MQ DP/HDMI PHY support" > + depends on OF && HAS_IOMEM > + depends on COMMON_CLK > + select GENERIC_PHY > + select CDNS_MHDP_HELPER > + help > + Enable this to support the Cadence HDPTX DP/HDMI PHY driver > + on i.MX8MQ SOC. > + > endif > > config PHY_FSL_LYNX_28G > diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile > index cedb328bc4d28..17546a4da840a 100644 > --- a/drivers/phy/freescale/Makefile > +++ b/drivers/phy/freescale/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_PHY_FSL_IMX8MQ_HDPTX) += phy-fsl-imx8mq-hdptx.o > obj-$(CONFIG_PHY_FSL_IMX8MQ_USB) += phy-fsl-imx8mq-usb.o > obj-$(CONFIG_PHY_MIXEL_LVDS_PHY) += phy-fsl-imx8qm-lvds-phy.o > obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.o > diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c > b/drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c > new file mode 100644 > index 0..3bf92718f826a > --- /dev/null > +++ b/drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c > @@ -0,0 +1,1402 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Cadence DP/HDMI PHY driver > + * > + * Copyright (C) 2022-2024 NXP Semiconductor, Inc. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define ADDR_PHY_AFE 0x8 > + > +/* PHY registers */ > +#define CMN_SSM_BIAS_TMR 0x0022 > +#define CMN_PLLSM0_PLLEN_TMR 0x0029 > +#define CMN_PLLSM0_PLLPRE_TMR 0x002a > +#define CMN_PLLSM0_PLLVREF_TMR 0x002b > +#define CMN_PLLSM0_PLLLOCK_TMR 0x002c > +#define CMN_PLLSM0_USER_DEF_CTRL 0x002f > +#define CMN_PSM_CLK_CTRL 0x0061 > +#define CMN_CDIAG_REFCLK_CTRL 0x0062 > +#define CMN_PLL0_VCOCAL_START 0x0081 > +#define CMN_PLL0_VCOCAL_INIT_TMR 0x0084 > +#define CMN_PLL0_VCOCAL_ITER_TMR 0x0085 > +#define CMN_PLL0_INTDIV0x0094 > +#define CMN_PLL0_FRACDIV 0x0095 > +#define CMN_PLL0_HIGH_THR 0x0096 > +#define CMN_PLL0_DSM_DIAG 0x0097 > +#define CMN_PLL0_SS_CTRL2 0x0099 > +#define CMN_ICAL_INIT_TMR 0x00c4 > +#define CMN_ICAL_ITER_TMR 0x00c5 > +#define CMN_RXCAL_INIT_TMR 0x00d4 > +#define CMN_RXCAL_ITER_TMR 0x00d5 > +#define CMN_TXPUCAL_CTRL 0x00e0 > +#define CMN_TXPUCAL_INIT_TMR 0x00e4 > +#define CMN_TXPUCAL_ITER_TMR 0x00e5 > +#define CMN_TXPDCAL_CTRL 0x00f0 > +#define CMN_TXPDCAL_INIT_TMR 0x00f4 > +#define CMN_TXPDCAL_ITER_TMR 0x00f5 > +#define CMN_ICAL_ADJ_INIT_TMR 0x0102 > +#define CMN_ICAL_ADJ_ITER_TMR 0x0103 > +#define CMN_RX_ADJ_INIT_TMR0x0106 > +#define CMN_RX_ADJ_ITER_TMR0x0107 > +#define CMN_TXPU_ADJ_CTRL 0x0108 > +#define CMN_TXPU_ADJ_INIT_TMR 0x010a > +#define CMN_TXPU_ADJ_ITER_TMR 0x010b > +#define CMN_TXPD_ADJ_CTRL 0x010c > +#define CMN_TXPD_ADJ_INIT_TMR 0x010e > +#define CMN_TXPD_ADJ_ITER_TMR 0x010f > +#define CMN_DIAG_PLL0_FBH_OVRD 0x01c0 > +#define CMN_DIAG_PLL0_FBL_OVRD 0x01c1 > +#define CMN_DIAG_PLL0_OVRD 0x01c2 > +#define CMN_DIAG_PLL0_TEST_MODE
Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
Hi, On Tue, Mar 12, 2024 at 5:47 PM Guenter Roeck wrote: > > On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson > wrote: > > > > As documented in the description of the transfer() function of > > "struct drm_dp_aux", the transfer() function can be called at any time > > regardless of the state of the DP port. Specifically if the kernel has > > the DP AUX character device enabled and userspace accesses > > "/dev/drm_dp_auxN" directly then the AUX transfer function will be > > called regardless of whether a DP device is connected. > > > > For eDP panels we have a special rule where we wait (with a 5 second > > timeout) for HPD to go high. This rule was important before all panels > > drivers were converted to call wait_hpd_asserted() and actually can be > > removed in a future commit. > > > > For external DP devices we never checked for HPD. That means that > > trying to access the DP AUX character device (AKA `hexdump -C > > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my > > system: > > $ time hexdump -C /dev/drm_dp_aux0 > > hexdump: /dev/drm_dp_aux0: Connection timed out > > > > real0m8.200s > > > > Let's add a check for HPD to avoid the slow timeout. This matches > > what, for instance, the intel_dp_aux_xfer() function does when it > > calls intel_tc_port_connected_locked(). That call has a document by it > > explaining that it's important to avoid the long timeouts. > > > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") > > Signed-off-by: Douglas Anderson > > --- > > > > drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++- > > drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++ > > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + > > 3 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > > b/drivers/gpu/drm/msm/dp/dp_aux.c > > index 03f4951c49f4..de0b0eabced9 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > > @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux > > *dp_aux, > > * turned on the panel and then tried to do an AUX transfer. The > > panel > > * driver has no way of knowing when the panel is ready, so it's up > > * to us to wait. For DP we never get into this situation so let's > > -* avoid ever doing the extra long wait for DP. > > +* avoid ever doing the extra long wait for DP and just query HPD > > +* directly. > > */ > > if (aux->is_edp) { > > ret = > > dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > > @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux > > *dp_aux, > > DRM_DEBUG_DP("Panel not ready for aux > > transactions\n"); > > goto exit; > > } > > + } else { > > + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) { > > + ret = -ENXIO; > > + goto exit; > > + } > > } > > > > dp_aux_update_offset_and_segment(aux, msg); > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > > b/drivers/gpu/drm/msm/dp/dp_catalog.c > > index 5142aeb705a4..93e2d413a1e7 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct > > dp_catalog *dp_catalog) > > 2000, 50); > > } > > > > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog) > > +{ > > + struct dp_catalog_private *catalog = container_of(dp_catalog, > > + struct dp_catalog_private, dp_catalog); > > + > > + /* poll for hpd connected status every 2ms and timeout after 500ms > > */ > > Maybe I am missing something, but the comment doesn't seem to match > the code below. > > Guenter > > > + return readl(catalog->io->dp_controller.aux.base + > > REG_DP_DP_HPD_INT_STATUS) & > > + DP_DP_HPD_STATE_STATUS_CONNECTED; LOL. I guess I overlooked that. Thanks for catching! The comment got copied from the dp_catalog_aux_wait_for_hpd_connect_state(). I'll remove the comment and send a v2, but I'll wait a little bit to see if there is additional feedback. -Doug
[PATCH v2 2/8] drm: xlnx: zynqmp_dpsub: Update live format defines
Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register layout. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..fa3935384834 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASKGENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASKGENMASK(5, 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRSTBIT(8) #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400 -- 2.25.1
[PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver
DO NOT MERGE. REFERENCE ONLY. Add CRTC driver based on AMD/Xilinx Video Test Pattern Generator IP. TPG based FPGA design represents minimalistic harness useful for testing links between FPGA based CRTC and external DRM encoders, both FPGA and hardened IP based. Add driver for AMD/Xilinx Video Timing Controller. The VTC, working in generator mode, suplements TPG with video timing signals. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/Kconfig | 21 + drivers/gpu/drm/xlnx/Makefile| 4 + drivers/gpu/drm/xlnx/xlnx_tpg.c | 854 +++ drivers/gpu/drm/xlnx/xlnx_vtc.c | 452 ++ drivers/gpu/drm/xlnx/xlnx_vtc.h | 101 + drivers/gpu/drm/xlnx/xlnx_vtc_list.c | 160 +++ 6 files changed, 1592 insertions(+) diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig index 68ee897de9d7..c40e98c1a5e6 100644 --- a/drivers/gpu/drm/xlnx/Kconfig +++ b/drivers/gpu/drm/xlnx/Kconfig @@ -15,3 +15,24 @@ config DRM_ZYNQMP_DPSUB This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose this option if you have a Xilinx ZynqMP SoC with DisplayPort subsystem. + +config DRM_XLNX_BRIDGE_VTC + bool "Xilinx DRM VTC Driver" + depends on OF + help + DRM brige driver for Xilinx Video Timing Controller. Choose + this option to make VTC a part of the CRTC in display pipeline. + Currently the support is added to the Xilinx Video Mixer and + Xilinx PL display CRTC drivers. This driver provides ability + to generate timings through the bridge layer. + +config DRM_XLNX_TPG + bool "Xilinx DRM TPG Driver" + depends on DRM && OF + select DRM_XLNX_BRIDGE_VTC + select VIDEOMODE_HELPERS + help + CRTC driver based on AMD/Xilinx Test Pattern Generator IP. Choose + this driver to enable Test Pattern Generator CRTC. This driver + implements simplistic CRTC with the single plane and is perfect for + testing PL to PS and PL to PL display output pipelines. diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile index ea1422a39502..26fb3ad21fa9 100644 --- a/drivers/gpu/drm/xlnx/Makefile +++ b/drivers/gpu/drm/xlnx/Makefile @@ -1,2 +1,6 @@ zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o zynqmp_kms.o obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o +xlnx-tpg-objs := xlnx_tpg.o +xlnx-tpg-$(CONFIG_DRM_XLNX_BRIDGE_VTC) += xlnx_vtc_list.o +obj-$(CONFIG_DRM_XLNX_TPG) += xlnx-tpg.o +obj-$(CONFIG_DRM_XLNX_BRIDGE_VTC) += xlnx_vtc.o diff --git a/drivers/gpu/drm/xlnx/xlnx_tpg.c b/drivers/gpu/drm/xlnx/xlnx_tpg.c new file mode 100644 index ..297a65fdb289 --- /dev/null +++ b/drivers/gpu/drm/xlnx/xlnx_tpg.c @@ -0,0 +1,854 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Xilinx logicore test pattern generator driver + * + * Copyright (C) 2024 Advanced Micro Devices, Inc. + * + * Author: Anatoliy Klymenko + * + * This driver introduces support for the test CRTC based on AMD/Xilinx + * Test Pattern Generator IP. The main goal of the driver is to enable + * simplistic FPGA design that could be used to test FPGA CRTC to external + * encoder IP connectivity. + * Reference: https://docs.xilinx.com/r/en-US/pg103-v-tpg + */ + +#include "xlnx_vtc.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define DRIVER_NAME"xlnx-tpg" +#define DRIVER_DESC"Xilinx TPG DRM KMS Driver" +#define DRIVER_DATE"20240307" +#define DRIVER_MAJOR 1 +#define DRIVER_MINOR 0 + +#define XLNX_TPG_CONTROL 0x +#define XLNX_TPG_GLOBAL_IRQ_EN 0x0004 +#define XLNX_TPG_IP_IRQ_EN 0x0008 +#define XLNX_TPG_IP_IRQ_STATUS 0x000C +#define XLNX_TPG_ACTIVE_HEIGHT 0x0010 +#define XLNX_TPG_ACTIVE_WIDTH 0x0018 +#define XLNX_TPG_PATTERN_ID0x0020 +#define XLNX_TPG_COLOR_FORMAT 0x0040 + +#define XLNX_TPG_IP_IRQ_AP_DONEBIT(0) + +#define XLNX_TPG_START BIT(0) +#define XLNX_TPG_AUTO_RESTART BIT(7) + +enum xlnx_tpg_pattern { + XTPG_PAT_HORIZONTAL_RAMP = 0x1, + XTPG_PAT_VERTICAL_RAMP, + XTPG_PAT_TEMPORAL_RAMP, + XTPG_PAT_SOLID_RED, + XTPG_PAT_SOLID_GREEN, + XTPG_PAT_SOLID_BLUE, + XTPG_PAT_SOLID_BLACK, + XTPG_PAT_SOLID_WHITE, + XTPG_PAT_COLOR_BARS, + XTPG_PAT_ZONE_PLATE, + XTPG_PAT_TARTAN_COLOR_BARS, + XTPG_PAT_CROSS_HATCH, + XTPG_PAT_COLOR_SWEEP, + XTPG_PAT_COMBO_RAMP, + XTPG_PAT_CHECKER_BOARD, + XTPG_PAT_DP_COLOR_RAMP, + XTPG_PAT_DP_VERTICAL_LINES, + XTPG_PAT_DP_COLOR_SQUARE, +}; + +static const struct drm_prop_enum_list xtpg_pattern_list[] = { + { XTPG_PAT_HORIZONTAL_RAMP, "horizontal-ramp
[PATCH v2 7/8] dt-bindings: xlnx: Add VTC and TPG bindings
DO NOT MERGE. REFERENCE ONLY. Add binding for AMD/Xilinx Video Timing Controller and Test Pattern Generator. Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node. Signed-off-by: Anatoliy Klymenko --- .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 ++ .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 include/dt-bindings/media/media-bus-format.h | 177 + 3 files changed, 329 insertions(+) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml new file mode 100644 index ..df5ee5b547cf --- /dev/null +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml @@ -0,0 +1,87 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/xlnx/xlnx,v-tpg.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Xilinx Video Test Pattern Generator (TPG) + +description: + AMD/Xilinx Video Test Pattern Generator IP is a soft IP designed to + generate test video signal in AXI4-Stream Video format. + https://docs.xilinx.com/r/en-US/pg103-v-tpg + +maintainers: + - Anatoliy Klymenko + +properties: + compatible: +description: + TPG versions backward-compatible with previous versions should list all + compatible versions in the newer to older order. +enum: ["xlnx,v-tpg-8.0", "xlnx,v-tpg-8.2"] + + reg: +maxItems: 1 + + interrupts: +maxItems: 1 + + bus-format: +description: Output media bus format, one of MEDIA_BUS_FMT_* +maxItems: 1 + + xlnx,bridge: + description: Reference to Video Timing Controller + maxItems: 1 + + ports: +$ref: /schemas/graph.yaml#/properties/ports +description: + Connections from and to external components in the video pipeline. + +properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: Sink port connected to downstream video IP. + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: Source port to connect to optional video signal source. + +required: + - port@0 + +required: + - compatible + - reg + - interrupts + - bus-format + - xlnx,bridge + - ports + +additionalProperties: false + +examples: + - | +#include + +tpg_0: tpg@4005 { + compatible = "xlnx,v-tpg-8.0"; + reg = <0x4005 0x1>; + interrupts = <0 89 4>; + xlnx,bridge = <&vtc_3>; + bus-format = ; + ports { +#address-cells = <1>; +#size-cells = <0>; +port@0 { + reg = <0>; + tpg_out: endpoint { +remote-endpoint = <&dp_encoder>; + }; +}; + }; +}; + +... diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml new file mode 100644 index ..51eb12cdcfdc --- /dev/null +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml @@ -0,0 +1,65 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/xlnx/xlnx,vtc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Device-Tree for Xilinx Video Timing Controller(VTC) + +description: + Xilinx VTC is a general purpose video timing generator and detector. + The input side of this core automatically detects horizontal and + vertical synchronization, pulses, polarity, blanking timing and active pixels. + While on the output, it generates the horizontal and vertical blanking and + synchronization pulses used with a standard video system including support + for programmable pulse polarity. + + The core is commonly used with Video in to AXI4-Stream core to detect the + format and timing of incoming video data or with AXI4-Stream to Video out core + to generate outgoing video timing for downstream sinks like a video monitor. + + For details please refer to + https://docs.xilinx.com/r/en-US/pg016_v_tc + +maintainers: + - Sagar Vishal + +properties: + compatible: +const: "xlnx,bridge-v-tc-6.1" + + reg: +maxItems: 1 + + xlnx,pixels-per-clock: +description: Pixels per clock of the stream. +enum: [1, 2, 4] + + clocks: +minItems: 2 + + clock-names: +items: + - const: clk + - const: s_axi_aclk + +required: + - compatible + - reg + - xlnx,pixels-per-clock + - clocks + - clock-names + +additionalProperties: false + +examples: + - | +v_tc_0: v_tc@8001 { + clock-names = "clk", "s_axi_aclk"; + clocks = <&clk_wiz_0>, <&misc_clk_0>; + compatible = "xlnx,bridge-v-tc-6.1"; + xlnx,pixels-per-clock = <1>; + reg = <0x8001 0x1>; +}; + +... diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h new file mode 100644 index ..60fc6e11dabc --- /dev/
[PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag
Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer context. This flag signals whether the driver runs in DRM CRTC or DRM bridge mode, assuming that all display layers share the same live or non-live mode of operation. Using per-layer mode instead of global flag will siplify future support of the hybrid scenario. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index af851190f447..dd48fa60fa9a 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer) { unsigned int i; - if (layer->disp->dpsub->dma_enabled) { + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) { for (i = 0; i < layer->drm_fmt->num_planes; i++) dmaengine_terminate_sync(layer->dmas[i].chan); } @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt); - if (!layer->disp->dpsub->dma_enabled) + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) return; /* @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer, const struct drm_format_info *info = layer->drm_fmt; unsigned int i; - if (!layer->disp->dpsub->dma_enabled) + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) return 0; for (i = 0; i < info->num_planes; i++) { @@ -1089,7 +1089,7 @@ static void zynqmp_disp_layer_release_dma(struct zynqmp_disp *disp, { unsigned int i; - if (!layer->info || !disp->dpsub->dma_enabled) + if (!layer->info) return; for (i = 0; i < layer->info->num_channels; i++) { @@ -1132,9 +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp, unsigned int i; int ret; - if (!disp->dpsub->dma_enabled) - return 0; - for (i = 0; i < layer->info->num_channels; i++) { struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; char dma_channel_name[16]; -- 2.25.1
[PATCH v2 6/8] drm/atomic-helper: Add select_output_bus_format callback
Add optional drm_crtc_helper_funcs.select_output_bus_format callback. This callback allows to negotiate compatible media bus format on the link between CRTC and connected DRM encoder or DRM bridge chain. A good usage example is the CRTC implemented as FPGA soft IP. This kind of CRTC will most certainly support a single output media bus format, as supporting multiple runtime options consumes extra FPGA resources. A variety of options for the FPGA designs are usually achieved by synthesizing IP with different parameters. Add drm_helper_crtc_select_output_bus_format that wraps drm_crtc_helper_funcs.select_output_bus_format. Incorporate select_output_bus_format callback into the format negotiation stage to fix the input bus format of the first DRM bridge in the chain. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/drm_bridge.c | 14 +++-- drivers/gpu/drm/drm_crtc_helper.c| 36 include/drm/drm_crtc_helper.h| 5 + include/drm/drm_modeset_helper_vtables.h | 30 ++ 4 files changed, 83 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..955ca108cd4b 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -28,6 +28,7 @@ #include #include +#include #include #include #include @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, unsigned int i, num_in_bus_fmts = 0; struct drm_bridge_state *cur_state; struct drm_bridge *prev_bridge; - u32 *in_bus_fmts; + struct drm_crtc *crtc = crtc_state->crtc; + u32 *in_bus_fmts, in_fmt; int ret; prev_bridge = drm_bridge_get_prev_bridge(cur_bridge); @@ -933,7 +935,15 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, return -ENOMEM; if (first_bridge == cur_bridge) { - cur_state->input_bus_cfg.format = in_bus_fmts[0]; + in_fmt = drm_helper_crtc_select_output_bus_format(crtc, + crtc_state, + in_bus_fmts, + num_in_bus_fmts); + if (!in_fmt) { + kfree(in_bus_fmts); + return -ENOTSUPP; + } + cur_state->input_bus_cfg.format = in_fmt; cur_state->output_bus_cfg.format = out_bus_fmt; kfree(in_bus_fmts); return 0; diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 2dafc39a27cb..f2e12a3c4e5f 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -1055,3 +1055,39 @@ int drm_helper_force_disable_all(struct drm_device *dev) return ret; } EXPORT_SYMBOL(drm_helper_force_disable_all); + +/** + * drm_helper_crtc_select_output_bus_format - Select output media bus format + * @crtc: The CRTC to query + * @crtc_state: The new CRTC state + * @supported_fmts: List of media bus format options to pick from + * @num_supported_fmts: Number of media bus formats in @supported_fmts list + * + * Encoder drivers may call this helper to give the connected CRTC a chance to + * select compatible or preffered media bus format to use over the CRTC encoder + * link. Encoders should provide list of supported input MEDIA_BUS_FMT_* for + * CRTC to pick from. CRTC driver is expected to select preferred media bus + * format from the list and, once enabled, generate the signal accordingly. + * + * Returns: + * Selected preferred media bus format or 0 if CRTC does not support any from + * @supported_fmts list. + */ +u32 drm_helper_crtc_select_output_bus_format(struct drm_crtc *crtc, +struct drm_crtc_state *crtc_state, +const u32 *supported_fmts, +unsigned int num_supported_fmts) +{ + if (!crtc || !supported_fmts || !num_supported_fmts) + return 0; + + if (!crtc->helper_private || + !crtc->helper_private->select_output_bus_format) + return supported_fmts[0]; + + return crtc->helper_private->select_output_bus_format(crtc, + crtc_state, + supported_fmts, + num_supported_fmts); +} +EXPORT_SYMBOL(drm_helper_crtc_select_output_bus_format); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 8c886fc46ef2..b7eb52f3ce41 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -38,6 +38,7 @@ struct drm_atomic_state; struct drm_connector; struct drm_crtc; +struct drm_crtc_state; st
[PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
DPSUB in bridge mode supports multiple input media bus formats. Announce the list of supported input media bus formats via drm_bridge.atomic_get_input_bus_fmts callback. Introduce a set of live input formats, supported by DPSUB. Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats() to reflect semantics for both live and non-live layer format lists. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp.c | 67 +- drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +-- drivers/gpu/drm/xlnx/zynqmp_dp.c | 26 +++ drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- 4 files changed, 88 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index e6d26ef60e89..af851190f447 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -77,12 +78,16 @@ enum zynqmp_dpsub_layer_mode { /** * struct zynqmp_disp_format - Display subsystem format information * @drm_fmt: DRM format (4CC) + * @bus_fmt: Media bus format * @buf_fmt: AV buffer format * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats * @sf: Scaling factors for color components */ struct zynqmp_disp_format { - u32 drm_fmt; + union { + u32 drm_fmt; + u32 bus_fmt; + }; u32 buf_fmt; bool swap; const u32 *sf; @@ -182,6 +187,12 @@ static const u32 scaling_factors_565[] = { ZYNQMP_DISP_AV_BUF_5BIT_SF, }; +static const u32 scaling_factors_666[] = { + ZYNQMP_DISP_AV_BUF_6BIT_SF, + ZYNQMP_DISP_AV_BUF_6BIT_SF, + ZYNQMP_DISP_AV_BUF_6BIT_SF, +}; + static const u32 scaling_factors_888[] = { ZYNQMP_DISP_AV_BUF_8BIT_SF, ZYNQMP_DISP_AV_BUF_8BIT_SF, @@ -364,6 +375,36 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = { }, }; +/* List of live video layer formats */ +static const struct zynqmp_disp_format avbuf_live_fmts[] = { + { + .bus_fmt= MEDIA_BUS_FMT_RGB666_1X18, + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 | + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, + .sf = scaling_factors_666, + }, { + .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X24, + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, + .sf = scaling_factors_888, + }, { + .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16, + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, + .sf = scaling_factors_888, + }, { + .bus_fmt= MEDIA_BUS_FMT_VUY8_1X24, + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444, + .sf = scaling_factors_888, + }, { + .bus_fmt= MEDIA_BUS_FMT_UYVY10_1X20, + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 | + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, + .sf = scaling_factors_101010, + }, +}; + static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg) { return readl(disp->avbuf.base + reg); @@ -883,16 +924,17 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer, } /** - * zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the layer + * zynqmp_disp_layer_formats - Return DRM or media bus formats supported by + * the layer * @layer: The layer * @num_formats: Pointer to the returned number of formats * - * Return: A newly allocated u32 array that stores all the DRM formats + * Return: A newly allocated u32 array that stores all DRM or media bus formats * supported by the layer. The number of formats in the array is returned * through the num_formats argument. */ -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, - unsigned int *num_formats) +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer, + unsigned int *num_formats) { unsigned int i; u32 *formats; @@ -1131,6 +1173,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp) .num_channels = 1, }, }; + static const struct zynqmp_disp_layer_info live_layer_info = { + .formats = avbuf_live_fmts, + .num_formats = ARRAY_SIZE(avbuf_live_fmts), + .num_channels = 0, + }; unsigned int i; int ret; @@ -1140,12 +1
[PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
Program live video input format according to selected media bus format. In the bridge mode of operation, DPSUB is connected to FPGA CRTC which almost certainly supports a single media bus format as its output. Expect this to be delivered via the new bridge atomic state. Program DPSUB registers accordingly. Update zynqmp_disp_layer_set_format() API to fit both live and non-live layer types. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp.c | 93 +++--- drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 -- drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- 4 files changed, 87 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index dd48fa60fa9a..0cacd597f4b8 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -383,7 +383,7 @@ static const struct zynqmp_disp_format avbuf_live_fmts[] = { ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, .sf = scaling_factors_666, }, { - .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X24, + .bus_fmt= MEDIA_BUS_FMT_RBG888_1X24, .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, .sf = scaling_factors_888, @@ -433,19 +433,28 @@ static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp *disp, const struct zynqmp_disp_format *fmt) { unsigned int i; - u32 val; + u32 val, reg; - val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT); - val &= zynqmp_disp_layer_is_video(layer) - ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK - : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK; - val |= fmt->buf_fmt; - zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val); + layer->disp_fmt = fmt; + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) { + reg = ZYNQMP_DISP_AV_BUF_FMT; + val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT); + val &= zynqmp_disp_layer_is_video(layer) + ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK + : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK; + val |= fmt->buf_fmt; + } else { + reg = zynqmp_disp_layer_is_video(layer) + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG; + val = fmt->buf_fmt; + } + zynqmp_disp_avbuf_write(disp, reg, val); for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) { - unsigned int reg = zynqmp_disp_layer_is_video(layer) -? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i) -: ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i); + reg = zynqmp_disp_layer_is_video(layer) + ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i) + : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i); zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]); } @@ -984,23 +993,73 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer) zynqmp_disp_blend_layer_disable(layer->disp, layer); } +struct zynqmp_disp_bus_to_drm { + u32 bus_fmt; + u32 drm_fmt; +}; + +/** + * zynqmp_disp_reference_drm_format - Get reference DRM format corresponding + * to the given media bus format. + * @bus_format: Media bus format + * + * Map media bus format to some DRM format that represents the same color space + * and chroma subsampling as the @bus_format video signal. These DRM format + * properties are required to program the blender. + * + * Return: DRM format code corresponding to @bus_format + */ +static u32 zynqmp_disp_reference_drm_format(u32 bus_format) +{ + static const struct zynqmp_disp_bus_to_drm format_map[] = { + { + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18, + .drm_fmt = DRM_FORMAT_RGB565, + }, { + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24, + .drm_fmt = DRM_FORMAT_RGB888, + }, { + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16, + .drm_fmt = DRM_FORMAT_YUV422, + }, { + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24, + .drm_fmt = DRM_FORMAT_YUV444, + }, { + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20, + .drm_fmt = DRM_FORMAT_P210, + }, + }; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(format_map); ++i) + if (format_map[i].bus_fmt == bus_format) + return format_map[i].drm_fmt; + + return DRM_FORMAT_INVALID; +} + /** * zynqmp_disp_layer_set_format
[PATCH v2 1/8] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
Set layer mode of operation (live or dma-based) during layer creation. Each DPSUB layer mode of operation is defined by corresponding DT node port connection, so it is possible to assign it during layer object creation. Previously it was set in layer enable functions, although it is too late as setting layer format depends on layer mode, and should be done before given layer enabled. Signed-off-by: Anatoliy Klymenko Reviewed-by: Laurent Pinchart --- drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 + drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +- drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index 8a39b3accce5..e6d26ef60e89 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -64,6 +64,16 @@ #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3 +/** + * enum zynqmp_dpsub_layer_mode - Layer mode + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode + */ +enum zynqmp_dpsub_layer_mode { + ZYNQMP_DPSUB_LAYER_NONLIVE, + ZYNQMP_DPSUB_LAYER_LIVE, +}; + /** * struct zynqmp_disp_format - Display subsystem format information * @drm_fmt: DRM format (4CC) @@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, /** * zynqmp_disp_layer_enable - Enable a layer * @layer: The layer - * @mode: Operating mode of layer * * Enable the @layer in the audio/video buffer manager and the blender. DMA * channels are started separately by zynqmp_disp_layer_update(). */ -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer, - enum zynqmp_dpsub_layer_mode mode) +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer) { - layer->mode = mode; zynqmp_disp_avbuf_enable_video(layer->disp, layer); zynqmp_disp_blend_layer_enable(layer->disp, layer); } @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp) layer->id = i; layer->disp = disp; layer->info = &layer_info[i]; + /* For now assume dpsub works in either live or non-live mode for both layers. +* Hybrid mode is not supported yet. +*/ + layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE + : ZYNQMP_DPSUB_LAYER_LIVE; ret = zynqmp_disp_layer_request_dma(disp, layer); if (ret) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h index 123cffac08be..9b8b202224d9 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id { ZYNQMP_DPSUB_LAYER_GFX, }; -/** - * enum zynqmp_dpsub_layer_mode - Layer mode - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode - */ -enum zynqmp_dpsub_layer_mode { - ZYNQMP_DPSUB_LAYER_NONLIVE, - ZYNQMP_DPSUB_LAYER_LIVE, -}; - void zynqmp_disp_enable(struct zynqmp_disp *disp); void zynqmp_disp_disable(struct zynqmp_disp *disp); int zynqmp_disp_setup_clock(struct zynqmp_disp *disp, @@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp, u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, unsigned int *num_formats); -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer, - enum zynqmp_dpsub_layer_mode mode); +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer); void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer); void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, const struct drm_format_info *info); diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 1846c4971fd8..04b6bcac3b07 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp, /* TODO: Make the format configurable. */ info = drm_format_info(DRM_FORMAT_YUV422); zynqmp_disp_layer_set_format(layer, info); - zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE); + zynqmp_disp_layer_enable(layer); if (layer_id == ZYNQMP_DPSUB_LAYER_GFX) zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255); diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c index db3bb4afbfc4..43bf416b33d5 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c @@ -122,7 +122,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plan
[PATCH v2 0/8] Setting live video input format for ZynqMP DPSUB
Implement live video input format setting for ZynqMP DPSUB. ZynqMP DPSUB can operate in 2 modes: DMA-based and live. In the live mode, DPSUB receives a live video signal from FPGA-based CRTC. DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune into the incoming video signal, DPSUB should be programmed with the proper media bus format. This patch series addresses this task. Patch 1/8: Set the DPSUB layer mode of operation prior to enabling the layer. Allows to use layer operational mode before its enablement. Patch 2/8: Update some IP register defines. Patch 3/8: Announce supported input media bus formats via drm_bridge_funcs.atomic_get_input_bus_fmts callback. Patch 4/8: Minimize usage of a global flag. Minor improvement. Patch 5/8: Program DPSUB live video input format based on selected bus config in the new atomic bridge state. Patch 6/8: New optional CRTC atomic helper proposal that will allow CRTC to participate in DRM bridge chain format negotiation and impose format restrictions. Incorporate this callback into the DRM bridge format negotiation process. Patch 7/8: DT bindings documentation for Video Timing Controller and Test Pattern Generator IPs. Patch 8/8: Reference FPGA CRTC driver based on AMD/Xilinx Test Pattern Generator (TPG) IP. Add driver for the AMD/Xilinx Video Timing Controller (VTC), which supplements TPG. To: Laurent Pinchart To: Maarten Lankhorst To: Maxime Ripard To: Thomas Zimmermann To: David Airlie To: Daniel Vetter To: Michal Simek To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Jonas Karlman To: Jernej Skrabec To: Rob Herring To: Krzysztof Kozlowski To: Conor Dooley To: Mauro Carvalho Chehab Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Cc: devicet...@vger.kernel.org Cc: linux-me...@vger.kernel.org Signed-off-by: Anatoliy Klymenko Changes in v2: - Factor out register defines update into separate patch. - Add some improvements minimizing ithe usage of a global flag. - Reuse existing format setting API instead of introducing new versions. - Add warning around NULL check on new bridge state within atomic enable callback. - Add drm_helper_crtc_select_output_bus_format() that wraps drm_crtc_helper_funcs.select_output_bus_format(). - Update API comments per review recommendations. - Address some minor review comments. - Add reference CRTC driver that demonstrates the usage of the proposed drm_crtc_helper_funcs.select_output_bus_format() API. - Link to v1: https://lore.kernel.org/r/20240226-dp-live-fmt-v1-0-b78c3f69c...@amd.com --- Anatoliy Klymenko (8): drm: xlnx: zynqmp_dpsub: Set layer mode during creation drm: xlnx: zynqmp_dpsub: Update live format defines drm: xlnx: zynqmp_dpsub: Anounce supported input formats drm: xlnx: zynqmp_dpsub: Minimize usage of global flag drm: xlnx: zynqmp_dpsub: Set input live format drm/atomic-helper: Add select_output_bus_format callback dt-bindings: xlnx: Add VTC and TPG bindings drm: xlnx: Intoduce TPG CRTC driver .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 +++ .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 ++ drivers/gpu/drm/drm_bridge.c | 14 +- drivers/gpu/drm/drm_crtc_helper.c | 36 + drivers/gpu/drm/xlnx/Kconfig | 21 + drivers/gpu/drm/xlnx/Makefile | 4 + drivers/gpu/drm/xlnx/xlnx_tpg.c| 854 + drivers/gpu/drm/xlnx/xlnx_vtc.c| 452 +++ drivers/gpu/drm/xlnx/xlnx_vtc.h| 101 +++ drivers/gpu/drm/xlnx/xlnx_vtc_list.c | 160 drivers/gpu/drm/xlnx/zynqmp_disp.c | 187 - drivers/gpu/drm/xlnx/zynqmp_disp.h | 19 +- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h| 8 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 41 +- drivers/gpu/drm/xlnx/zynqmp_kms.c | 6 +- include/drm/drm_crtc_helper.h | 5 + include/drm/drm_modeset_helper_vtables.h | 30 + include/dt-bindings/media/media-bus-format.h | 177 + 18 files changed, 2204 insertions(+), 63 deletions(-) --- base-commit: bfa4437fd3938ae2e186e7664b2db65bb8775670 change-id: 20240226-dp-live-fmt-6415773b5a68 Best regards, -- Anatoliy Klymenko
Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson wrote: > > As documented in the description of the transfer() function of > "struct drm_dp_aux", the transfer() function can be called at any time > regardless of the state of the DP port. Specifically if the kernel has > the DP AUX character device enabled and userspace accesses > "/dev/drm_dp_auxN" directly then the AUX transfer function will be > called regardless of whether a DP device is connected. > > For eDP panels we have a special rule where we wait (with a 5 second > timeout) for HPD to go high. This rule was important before all panels > drivers were converted to call wait_hpd_asserted() and actually can be > removed in a future commit. > > For external DP devices we never checked for HPD. That means that > trying to access the DP AUX character device (AKA `hexdump -C > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my > system: > $ time hexdump -C /dev/drm_dp_aux0 > hexdump: /dev/drm_dp_aux0: Connection timed out > > real0m8.200s > > Let's add a check for HPD to avoid the slow timeout. This matches > what, for instance, the intel_dp_aux_xfer() function does when it > calls intel_tc_port_connected_locked(). That call has a document by it > explaining that it's important to avoid the long timeouts. > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") > Signed-off-by: Douglas Anderson > --- > > drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++- > drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index 03f4951c49f4..de0b0eabced9 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > * turned on the panel and then tried to do an AUX transfer. The panel > * driver has no way of knowing when the panel is ready, so it's up > * to us to wait. For DP we never get into this situation so let's > -* avoid ever doing the extra long wait for DP. > +* avoid ever doing the extra long wait for DP and just query HPD > +* directly. > */ > if (aux->is_edp) { > ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > DRM_DEBUG_DP("Panel not ready for aux > transactions\n"); > goto exit; > } > + } else { > + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) { > + ret = -ENXIO; > + goto exit; > + } > } > > dp_aux_update_offset_and_segment(aux, msg); > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > b/drivers/gpu/drm/msm/dp/dp_catalog.c > index 5142aeb705a4..93e2d413a1e7 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct > dp_catalog *dp_catalog) > 2000, 50); > } > > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog) > +{ > + struct dp_catalog_private *catalog = container_of(dp_catalog, > + struct dp_catalog_private, dp_catalog); > + > + /* poll for hpd connected status every 2ms and timeout after 500ms */ Maybe I am missing something, but the comment doesn't seem to match the code below. Guenter > + return readl(catalog->io->dp_controller.aux.base + > REG_DP_DP_HPD_INT_STATUS) & > + DP_DP_HPD_STATE_STATUS_CONNECTED; > +} > + > static void dump_regs(void __iomem *base, int len) > { > int i; > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h > b/drivers/gpu/drm/msm/dp/dp_catalog.h > index 38786e855b51..1694040c530f 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.h > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h > @@ -86,6 +86,7 @@ void dp_catalog_aux_reset(struct dp_catalog *dp_catalog); > void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable); > void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog); > int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog); > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog); > u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog); > > /* DP Controller APIs */ > -- > 2.44.0.278.ge034bb2e1d-goog >
[PATCH 3/3] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
Before the introduction of the wait_hpd_asserted() callback in commit 841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct drm_dp_aux") the API between panel drivers and DP AUX bus drivers was that it was up to the AUX bus driver to wait for HPD in the transfer() function. Now wait_hpd_asserted() has been added. The two panel drivers that are DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit 3b5765df375c ("drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux"). We've implemented wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252 ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is no longer any reason for long wait in the AUX transfer() function. Remove it. NOTE: the wait_hpd_asserted() is listed as "optional". That means it's optional for the DP AUX bus to implement. In the case of the MSM DP driver we implement it so we can assume it will be called. ALSO NOTE: the wait wasn't actually _hurting_ anything and wasn't even causing long timeouts, but it's still nice to get rid of unneeded code. Specificaly it's not truly needed because to handle other DP drivers that can't power on as quickly (specifically parade-ps8640) we already avoid DP AUX transfers for eDP panels that aren't powered on. See commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX transfers when eDP panels are not powered"). Signed-off-by: Douglas Anderson --- drivers/gpu/drm/msm/dp/dp_aux.c | 26 +++--- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index fc398e8a69a7..dd62ad6007a6 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -302,26 +302,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, } /* -* For eDP it's important to give a reasonably long wait here for HPD -* to be asserted. This is because the panel driver may have _just_ -* turned on the panel and then tried to do an AUX transfer. The panel -* driver has no way of knowing when the panel is ready, so it's up -* to us to wait. For DP we never get into this situation so let's -* avoid ever doing the extra long wait for DP and just query HPD -* directly. +* If HPD isn't asserted then the transfer won't succeed. Return +* right away. If we don't do this we can end up with long timeouts +* if someone tries to access the DP AUX character device when no +* DP device is connected. */ - if (aux->is_edp) { - ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog, - 50); - if (ret) { - DRM_DEBUG_DP("Panel not ready for aux transactions\n"); - goto exit; - } - } else { - if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) { - ret = -ENXIO; - goto exit; - } + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) { + ret = -ENXIO; + goto exit; } dp_aux_update_offset_and_segment(aux, msg); -- 2.44.0.278.ge034bb2e1d-goog
[PATCH 2/3] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback
The DP wait_hpd_asserted() callback is passed a timeout which indicates how long we should wait for HPD. This timeout was being ignored in the MSM DP implementation and instead a hardcoded 500 ms timeout was used. Fix it to use the proper timeout. As part of this we move the hardcoded 500 ms number into the AUX transfer function, which isn't given a timeout. The wait in the AUX transfer function will be removed in a future commit. Fixes: e2969ee30252 ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()") Signed-off-by: Douglas Anderson --- drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++-- drivers/gpu/drm/msm/dp/dp_catalog.c | 7 --- drivers/gpu/drm/msm/dp/dp_catalog.h | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index de0b0eabced9..fc398e8a69a7 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -311,7 +311,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, * directly. */ if (aux->is_edp) { - ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog, + 50); if (ret) { DRM_DEBUG_DP("Panel not ready for aux transactions\n"); goto exit; @@ -516,7 +517,7 @@ static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux, aux = container_of(dp_aux, struct dp_aux_private, dp_aux); pm_runtime_get_sync(aux->dev); - ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog, wait_us); pm_runtime_put_sync(aux->dev); return ret; diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 93e2d413a1e7..b45cf3174aa0 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -253,17 +253,18 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) phy_calibrate(phy); } -int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog, + unsigned long wait_us) { u32 state; struct dp_catalog_private *catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); - /* poll for hpd connected status every 2ms and timeout after 500ms */ + /* poll for hpd connected status every 2ms and timeout after wait_us */ return readl_poll_timeout(catalog->io->dp_controller.aux.base + REG_DP_DP_HPD_INT_STATUS, state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, - 2000, 50); + min(wait_us, 2000), wait_us); } bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog) diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 1694040c530f..4248c8de5cf7 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -85,7 +85,8 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog); void dp_catalog_aux_reset(struct dp_catalog *dp_catalog); void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable); void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog); -int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog); +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog, + unsigned long wait_us); bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog); u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog); -- 2.44.0.278.ge034bb2e1d-goog
[PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
As documented in the description of the transfer() function of "struct drm_dp_aux", the transfer() function can be called at any time regardless of the state of the DP port. Specifically if the kernel has the DP AUX character device enabled and userspace accesses "/dev/drm_dp_auxN" directly then the AUX transfer function will be called regardless of whether a DP device is connected. For eDP panels we have a special rule where we wait (with a 5 second timeout) for HPD to go high. This rule was important before all panels drivers were converted to call wait_hpd_asserted() and actually can be removed in a future commit. For external DP devices we never checked for HPD. That means that trying to access the DP AUX character device (AKA `hexdump -C /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my system: $ time hexdump -C /dev/drm_dp_aux0 hexdump: /dev/drm_dp_aux0: Connection timed out real0m8.200s Let's add a check for HPD to avoid the slow timeout. This matches what, for instance, the intel_dp_aux_xfer() function does when it calls intel_tc_port_connected_locked(). That call has a document by it explaining that it's important to avoid the long timeouts. Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") Signed-off-by: Douglas Anderson --- drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++- drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++ drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 03f4951c49f4..de0b0eabced9 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, * turned on the panel and then tried to do an AUX transfer. The panel * driver has no way of knowing when the panel is ready, so it's up * to us to wait. For DP we never get into this situation so let's -* avoid ever doing the extra long wait for DP. +* avoid ever doing the extra long wait for DP and just query HPD +* directly. */ if (aux->is_edp) { ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, DRM_DEBUG_DP("Panel not ready for aux transactions\n"); goto exit; } + } else { + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) { + ret = -ENXIO; + goto exit; + } } dp_aux_update_offset_and_segment(aux, msg); diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 5142aeb705a4..93e2d413a1e7 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) 2000, 50); } +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + + /* poll for hpd connected status every 2ms and timeout after 500ms */ + return readl(catalog->io->dp_controller.aux.base + REG_DP_DP_HPD_INT_STATUS) & + DP_DP_HPD_STATE_STATUS_CONNECTED; +} + static void dump_regs(void __iomem *base, int len) { int i; diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 38786e855b51..1694040c530f 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -86,6 +86,7 @@ void dp_catalog_aux_reset(struct dp_catalog *dp_catalog); void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable); void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog); int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog); +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog); u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog); /* DP Controller APIs */ -- 2.44.0.278.ge034bb2e1d-goog
[PATCH 0/3] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions
The main goal of this patch series is to avoid problems running "fwupd" on Qualcomm devices. Right now several of the plugins used with fwupd try talking over all DP AUX busses and this results in a very long timeout on Qualcomm devices. As part of fixing this, I noticed a case where the MSM DP code wasn't respecing the timeout properly when asked to wait for HPD. I also noticed that, now that we've implemented wait_hpd_asserted(), we no longer need the long hardcoded timeout / special cse for eDP in the AUX transfer function. NOTE: I no longer have any hardware setup that uses this driver for eDP so I've only tested the DP case. The eDP changes are straightforward so hopefully there are no problems there. Douglas Anderson (3): drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer drivers/gpu/drm/msm/dp/dp_aux.c | 21 - drivers/gpu/drm/msm/dp/dp_catalog.c | 17 ++--- drivers/gpu/drm/msm/dp/dp_catalog.h | 4 +++- 3 files changed, 25 insertions(+), 17 deletions(-) -- 2.44.0.278.ge034bb2e1d-goog
Re: [PATCH] drm/i915/guc: Update w/a 14019159160
On Tue, Mar 12, 2024 at 04:43:06PM -0700, John Harrison wrote: On 3/12/2024 09:24, Matt Roper wrote: diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 0c67d674c94de..4c3dae98656af 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -296,7 +296,8 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc) /* Wa_16019325821 */ /* Wa_14019159160 */ - if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) + if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)) || From what I can see, this workaround is also needed on Xe_LPG+ (12.74) Isn't that an Xe platform? Or is 12.74 just ARL? official xe platforms start with Xe2, with graphics version being 20 Lucas De Marchi
Re: [PATCH 1/5] drm/i915: Drop WA 16015675438
On Tue, Mar 12, 2024 at 03:54:09PM -0700, Matt Roper wrote: On Wed, Mar 06, 2024 at 11:36:39AM -0800, Lucas De Marchi wrote: With dynamic load-balancing disabled on the compute side, there's no reason left to enable WA 16015675438. Drop it from both PVC and DG2. Note that this can be done because now the driver always set a fixed partition of EUs during initialization via the ccs_mode configuration. The flag to GuC is still needed because of 18020744125, so update the comment accordingly. Cc: Mateusz Jablonski Cc: Michal Mrozek Cc: Rodrigo Vivi Signed-off-by: Lucas De Marchi Dynamic load-balancing disable hasn't landed in i915 yet (although it probably will soon). Assuming we wait for that to happen first before applying this, Reviewed-by: Matt Roper Humn... I probably grepped the wrong tree for this one since I was seeing ccs_mode being set. Indeed it isn't :-/, so I will have to land a fix or revert since this patch already landed a few days ago. Lucas De Marchi
Re: [PATCH] drm/i915/guc: Update w/a 14019159160
On 3/12/2024 09:24, Matt Roper wrote: On Thu, Mar 07, 2024 at 06:01:29PM -0800, john.c.harri...@intel.com wrote: From: John Harrison An existing workaround has been extended in both platforms affected and implementation complexity. Signed-off-by: John Harrison --- drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h | 3 ++- drivers/gpu/drm/i915/gt/uc/intel_guc.c| 3 ++- drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c| 21 ++- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h index bebf28e3c4794..3e7060e859794 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h @@ -105,7 +105,8 @@ enum { * Workaround keys: */ enum { - GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE = 0x9001, + GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE = 0x9001, /* Wa_14019159160 */ + GUC_WORKAROUND_KLV_AVOID_GFX_CLEAR_WHILE_ACTIVE = 0x9006, /* Wa_14019159160 */ }; #endif /* _ABI_GUC_KLVS_ABI_H */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 0c67d674c94de..4c3dae98656af 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -296,7 +296,8 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc) /* Wa_16019325821 */ /* Wa_14019159160 */ - if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) + if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)) || From what I can see, this workaround is also needed on Xe_LPG+ (12.74) Isn't that an Xe platform? Or is 12.74 just ARL? John. now. Matt + IS_DG2(gt->i915)) flags |= GUC_WA_RCS_CCS_SWITCHOUT; /* diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c index 5c9908b56616e..00fe3c21a9b1c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c @@ -815,23 +815,23 @@ guc_capture_prep_lists(struct intel_guc *guc) return PAGE_ALIGN(total_size); } -/* Wa_14019159160 */ -static u32 guc_waklv_ra_mode(struct intel_guc *guc, u32 offset, u32 remain) +static void guc_waklv_enable_simple(struct intel_guc *guc, u32 *offset, u32 *remain, u32 klv_id) { u32 size; u32 klv_entry[] = { /* 16:16 key/length */ - FIELD_PREP(GUC_KLV_0_KEY, GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE) | + FIELD_PREP(GUC_KLV_0_KEY, klv_id) | FIELD_PREP(GUC_KLV_0_LEN, 0), /* 0 dwords data */ }; size = sizeof(klv_entry); - GEM_BUG_ON(remain < size); + GEM_BUG_ON(*remain < size); - iosys_map_memcpy_to(&guc->ads_map, offset, klv_entry, size); + iosys_map_memcpy_to(&guc->ads_map, *offset, klv_entry, size); - return size; + *offset += size; + *remain -= size; } static void guc_waklv_init(struct intel_guc *guc) @@ -850,10 +850,11 @@ static void guc_waklv_init(struct intel_guc *guc) remain = guc_ads_waklv_size(guc); /* Wa_14019159160 */ - if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) { - size = guc_waklv_ra_mode(guc, offset, remain); - offset += size; - remain -= size; + if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)) || IS_DG2(gt->i915)) { + guc_waklv_enable_simple(guc, &offset, &remain, + GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE); + guc_waklv_enable_simple(guc, &offset, &remain, + GUC_WORKAROUND_KLV_AVOID_GFX_CLEAR_WHILE_ACTIVE); } size = guc_ads_waklv_size(guc) - remain; -- 2.43.0
Re: [PATCH 2/5] drm/i915: Drop dead code for xehpsdv
On Tue, Mar 12, 2024 at 03:58:19PM -0700, Matt Roper wrote: On Wed, Mar 06, 2024 at 11:36:40AM -0800, Lucas De Marchi wrote: PCI IDs for XEHPSDV were never added and platform always marked with force_probe. Drop what's not used and rename some places to either be xehp or dg2, depending on the platform/IP checks. The registers not used anymore are also removed. Signed-off-by: Lucas De Marchi --- Potential problem here that needs a deeper look, the changes in __gen12_fw_ranges. Some ranges had comments saying they were XEHPSDV so I removed them, but it needs to be double checked with spec and CI results. ... diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 76400e9c40f0..4f1e56187442 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1536,17 +1536,12 @@ static const struct intel_forcewake_range __gen12_fw_ranges[] = { GEN_FW_RANGE(0x13200, 0x13fff, FORCEWAKE_MEDIA_VDBOX2), /* \ 0x13200 - 0x133ff: VD2 (DG2 only) \ 0x13400 - 0x13fff: reserved */ \ - GEN_FW_RANGE(0x14000, 0x141ff, FORCEWAKE_MEDIA_VDBOX0), /* XEHPSDV only */ \ - GEN_FW_RANGE(0x14200, 0x143ff, FORCEWAKE_MEDIA_VDBOX2), /* XEHPSDV only */ \ - GEN_FW_RANGE(0x14400, 0x145ff, FORCEWAKE_MEDIA_VDBOX4), /* XEHPSDV only */ \ - GEN_FW_RANGE(0x14600, 0x147ff, FORCEWAKE_MEDIA_VDBOX6), /* XEHPSDV only */ \ We can't just remove ranges in the middle of the table since that breaks the "watertight" table requirement that our selftests check for. We need to either roll the now-unused ranges into an adjacent range, or add a new "reserved" range. see 23n224gu57lfd4wbroqflav4pih6usrkf53q2ve4ntekhueylb@eqigxyktri6b GEN_FW_RANGE(0x14800, 0x14fff, FORCEWAKE_RENDER), \ GEN_FW_RANGE(0x15000, 0x16dff, FORCEWAKE_GT), /* \ 0x15000 - 0x15fff: gt (DG2 only) \ 0x16000 - 0x16dff: reserved */ \ GEN_FW_RANGE(0x16e00, 0x1, FORCEWAKE_RENDER), \ - GEN_FW_RANGE(0x2, 0x21fff, FORCEWAKE_MEDIA_VDBOX0), /* \ - 0x2 - 0x20fff: VD0 (XEHPSDV only) \ + GEN_FW_RANGE(0x21000, 0x21fff, FORCEWAKE_MEDIA_VDBOX0), /* \ 0x21000 - 0x21fff: reserved */ \ GEN_FW_RANGE(0x22000, 0x23fff, FORCEWAKE_GT), \ GEN_FW_RANGE(0x24000, 0x2417f, 0), /* \ @@ -1627,10 +1622,6 @@ static const struct intel_forcewake_range __gen12_fw_ranges[] = { 0x1f6e00 - 0x1f7fff: reserved */ \ GEN_FW_RANGE(0x1f8000, 0x1fa0ff, FORCEWAKE_MEDIA_VEBOX3), -static const struct intel_forcewake_range __xehp_fw_ranges[] = { - XEHP_FWRANGES(FORCEWAKE_GT) -}; - static const struct intel_forcewake_range __dg2_fw_ranges[] = { XEHP_FWRANGES(FORCEWAKE_RENDER) We can drop the macro here now and just make this a normal table like everything else. will add that in v2 too, thanks Lucas De Marchi
Re: [PATCH 2/5] drm/i915: Drop dead code for xehpsdv
On Wed, Mar 06, 2024 at 11:36:40AM -0800, Lucas De Marchi wrote: > PCI IDs for XEHPSDV were never added and platform always marked with > force_probe. Drop what's not used and rename some places to either be > xehp or dg2, depending on the platform/IP checks. > > The registers not used anymore are also removed. > > Signed-off-by: Lucas De Marchi > --- > > Potential problem here that needs a deeper look, the changes in > __gen12_fw_ranges. Some ranges had comments saying they were XEHPSDV so > I removed them, but it needs to be double checked with spec and CI > results. > ... > diff --git a/drivers/gpu/drm/i915/intel_uncore.c > b/drivers/gpu/drm/i915/intel_uncore.c > index 76400e9c40f0..4f1e56187442 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1536,17 +1536,12 @@ static const struct intel_forcewake_range > __gen12_fw_ranges[] = { > GEN_FW_RANGE(0x13200, 0x13fff, FORCEWAKE_MEDIA_VDBOX2), /* > \ > 0x13200 - 0x133ff: VD2 (DG2 only) > \ > 0x13400 - 0x13fff: reserved */ > \ > - GEN_FW_RANGE(0x14000, 0x141ff, FORCEWAKE_MEDIA_VDBOX0), /* XEHPSDV only > */ \ > - GEN_FW_RANGE(0x14200, 0x143ff, FORCEWAKE_MEDIA_VDBOX2), /* XEHPSDV only > */ \ > - GEN_FW_RANGE(0x14400, 0x145ff, FORCEWAKE_MEDIA_VDBOX4), /* XEHPSDV only > */ \ > - GEN_FW_RANGE(0x14600, 0x147ff, FORCEWAKE_MEDIA_VDBOX6), /* XEHPSDV only > */ \ We can't just remove ranges in the middle of the table since that breaks the "watertight" table requirement that our selftests check for. We need to either roll the now-unused ranges into an adjacent range, or add a new "reserved" range. > GEN_FW_RANGE(0x14800, 0x14fff, FORCEWAKE_RENDER), > \ > GEN_FW_RANGE(0x15000, 0x16dff, FORCEWAKE_GT), /* > \ > 0x15000 - 0x15fff: gt (DG2 only) > \ > 0x16000 - 0x16dff: reserved */ > \ > GEN_FW_RANGE(0x16e00, 0x1, FORCEWAKE_RENDER), > \ > - GEN_FW_RANGE(0x2, 0x21fff, FORCEWAKE_MEDIA_VDBOX0), /* > \ > - 0x2 - 0x20fff: VD0 (XEHPSDV only) > \ > + GEN_FW_RANGE(0x21000, 0x21fff, FORCEWAKE_MEDIA_VDBOX0), /* > \ > 0x21000 - 0x21fff: reserved */ > \ > GEN_FW_RANGE(0x22000, 0x23fff, FORCEWAKE_GT), > \ > GEN_FW_RANGE(0x24000, 0x2417f, 0), /* > \ > @@ -1627,10 +1622,6 @@ static const struct intel_forcewake_range > __gen12_fw_ranges[] = { > 0x1f6e00 - 0x1f7fff: reserved */ > \ > GEN_FW_RANGE(0x1f8000, 0x1fa0ff, FORCEWAKE_MEDIA_VEBOX3), > > -static const struct intel_forcewake_range __xehp_fw_ranges[] = { > - XEHP_FWRANGES(FORCEWAKE_GT) > -}; > - > static const struct intel_forcewake_range __dg2_fw_ranges[] = { > XEHP_FWRANGES(FORCEWAKE_RENDER) We can drop the macro here now and just make this a normal table like everything else. Matt -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
Re: [PATCH 1/5] drm/i915: Drop WA 16015675438
On Wed, Mar 06, 2024 at 11:36:39AM -0800, Lucas De Marchi wrote: > With dynamic load-balancing disabled on the compute side, there's no > reason left to enable WA 16015675438. Drop it from both PVC and DG2. > Note that this can be done because now the driver always set a fixed > partition of EUs during initialization via the ccs_mode configuration. > > The flag to GuC is still needed because of 18020744125, so update > the comment accordingly. > > Cc: Mateusz Jablonski > Cc: Michal Mrozek > Cc: Rodrigo Vivi > Signed-off-by: Lucas De Marchi Dynamic load-balancing disable hasn't landed in i915 yet (although it probably will soon). Assuming we wait for that to happen first before applying this, Reviewed-by: Matt Roper Matt > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 +- > drivers/gpu/drm/i915/gt/uc/intel_guc.c | 2 +- > 2 files changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index d67d44611c28..7f812409c30a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -2928,14 +2928,10 @@ general_render_compute_wa_init(struct intel_engine_cs > *engine, struct i915_wa_li > wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, > DISABLE_D8_D16_COASLESCE); > } > > - if (IS_PONTEVECCHIO(i915) || IS_DG2(i915)) { > + if (IS_PONTEVECCHIO(i915) || IS_DG2(i915)) > /* Wa_14015227452:dg2,pvc */ > wa_mcr_masked_en(wal, GEN9_ROW_CHICKEN4, XEHP_DIS_BBL_SYSPIPE); > > - /* Wa_16015675438:dg2,pvc */ > - wa_masked_en(wal, FF_SLICE_CS_CHICKEN2, > GEN12_PERF_FIX_BALANCING_CFE_DISABLE); > - } > - > if (IS_DG2(i915)) { > /* >* Wa_16011620976:dg2_g11 > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc.c > index d2b7425bbdcc..c6603793af89 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c > @@ -315,7 +315,7 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc) > if (IS_DG2_G11(gt->i915)) > flags |= GUC_WA_CONTEXT_ISOLATION; > > - /* Wa_16015675438 */ > + /* Wa_18020744125 */ > if (!RCS_MASK(gt)) > flags |= GUC_WA_RCS_REGS_IN_CCS_REGS_LIST; > > -- > 2.43.0 > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
Re: [PATCH 41/43] drm/tiny/st7735r: Use fbdev-dma
On 3/12/24 10:45 AM, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by st7735r. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: David Lechner > --- Acked-by: David Lechner
Re: [PATCH 34/43] drm/tiny/ili9225: Use fbdev-dma
On 3/12/24 10:45 AM, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by ili9225. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: David Lechner > --- Acked-by: David Lechner
Re: [PATCH 40/43] drm/tiny/st7586: Use fbdev-dma
On 3/12/24 10:45 AM, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by st7586. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: David Lechner > --- Acked-by: David Lechner
Re: [PATCH 04/14] kunit: Add documentation for warning backtrace suppression API
On Tue, Mar 12, 2024 at 10:02:59AM -0700, Guenter Roeck wrote: > Document API functions for suppressing warning backtraces. > > Signed-off-by: Guenter Roeck Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 03/14] kunit: Add test cases for backtrace warning suppression
On Tue, Mar 12, 2024 at 10:02:58AM -0700, Guenter Roeck wrote: > Add unit tests to verify that warning backtrace suppression works. > > If backtrace suppression does _not_ work, the unit tests will likely > trigger unsuppressed backtraces, which should actually help to get > the affected architectures / platforms fixed. > > Signed-off-by: Guenter Roeck Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 02/14] kunit: bug: Count suppressed warning backtraces
On Tue, Mar 12, 2024 at 10:02:57AM -0700, Guenter Roeck wrote: > Count suppressed warning backtraces to enable code which suppresses > warning backtraces to check if the expected backtrace(s) have been > observed. > > Using atomics for the backtrace count resulted in build errors on some > architectures due to include file recursion, so use a plain integer > for now. > > Signed-off-by: Guenter Roeck > --- > include/kunit/bug.h | 7 ++- > lib/kunit/bug.c | 4 +++- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/include/kunit/bug.h b/include/kunit/bug.h > index 1e34da961599..2097a854ac8c 100644 > --- a/include/kunit/bug.h > +++ b/include/kunit/bug.h > @@ -20,6 +20,7 @@ > struct __suppressed_warning { > struct list_head node; > const char *function; > + int counter; Thanks for adding a counter! > }; > > void __start_suppress_warning(struct __suppressed_warning *warning); > @@ -28,7 +29,7 @@ bool __is_suppressed_warning(const char *function); > > #define DEFINE_SUPPRESSED_WARNING(func) \ > struct __suppressed_warning __kunit_suppress_##func = \ > - { .function = __stringify(func) } > + { .function = __stringify(func), .counter = 0 } > > #define START_SUPPRESSED_WARNING(func) \ > __start_suppress_warning(&__kunit_suppress_##func) > @@ -39,12 +40,16 @@ bool __is_suppressed_warning(const char *function); > #define IS_SUPPRESSED_WARNING(func) \ > __is_suppressed_warning(func) > > +#define SUPPRESSED_WARNING_COUNT(func) \ > + (__kunit_suppress_##func.counter) > + > #else /* CONFIG_KUNIT */ > > #define DEFINE_SUPPRESSED_WARNING(func) > #define START_SUPPRESSED_WARNING(func) > #define END_SUPPRESSED_WARNING(func) > #define IS_SUPPRESSED_WARNING(func) (false) > +#define SUPPRESSED_WARNING_COUNT(func) (0) > > #endif /* CONFIG_KUNIT */ > #endif /* __ASSEMBLY__ */ > diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c > index f93544d7a9d1..13b3d896c114 100644 > --- a/lib/kunit/bug.c > +++ b/lib/kunit/bug.c > @@ -32,8 +32,10 @@ bool __is_suppressed_warning(const char *function) > return false; > > list_for_each_entry(warning, &suppressed_warnings, node) { > - if (!strcmp(function, warning->function)) > + if (!strcmp(function, warning->function)) { > + warning->counter++; > return true; > + } > } > return false; > } > -- > 2.39.2 > Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 01/14] bug/kunit: Core support for suppressing warning backtraces
On Tue, Mar 12, 2024 at 10:02:56AM -0700, Guenter Roeck wrote: > Some unit tests intentionally trigger warning backtraces by passing > bad parameters to API functions. Such unit tests typically check the > return value from those calls, not the existence of the warning backtrace. > > Such intentionally generated warning backtraces are neither desirable > nor useful for a number of reasons. > - They can result in overlooked real problems. > - A warning that suddenly starts to show up in unit tests needs to be > investigated and has to be marked to be ignored, for example by > adjusting filter scripts. Such filters are ad-hoc because there is > no real standard format for warnings. On top of that, such filter > scripts would require constant maintenance. > > One option to address problem would be to add messages such as "expected > warning backtraces start / end here" to the kernel log. However, that > would again require filter scripts, it might result in missing real > problematic warning backtraces triggered while the test is running, and > the irrelevant backtrace(s) would still clog the kernel log. > > Solve the problem by providing a means to identify and suppress specific > warning backtraces while executing test code. > > Cc: Dan Carpenter > Cc: Daniel Diaz > Cc: Naresh Kamboju > Cc: Kees Cook > Signed-off-by: Guenter Roeck Yup, this looks fine to me. Reviewed-by: Kees Cook -- Kees Cook
[PATCH] fbcon: Increase maximum font width x height to 64 x 64
This remains relatively simple by just enlarging integers. It wouldn't be that simple to get to the console's 64x128 maximum, as it would require 128b integers. Signed-off-by: Samuel Thibault Index: linux-6.4/drivers/video/fbdev/core/fbcon.c === --- linux-6.4.orig/drivers/video/fbdev/core/fbcon.c +++ linux-6.4/drivers/video/fbdev/core/fbcon.c @@ -101,6 +101,9 @@ enum { FBCON_LOGO_DONTSHOW = -3/* do not show the logo */ }; +#define FBCON_MAX_FONT_WIDTH (sizeof(((struct fb_pixmap *) 0)->blit_x) * 8) +#define FBCON_MAX_FONT_HEIGHT (sizeof(((struct fb_pixmap *) 0)->blit_y) * 8) + static struct fbcon_display fb_display[MAX_NR_CONSOLES]; struct fb_info *fbcon_registered_fb[FB_MAX]; @@ -2485,12 +2488,12 @@ static int fbcon_set_font(struct vc_data h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres)) return -EINVAL; - if (font->width > 32 || font->height > 32) + if (font->width > FBCON_MAX_FONT_WIDTH || font->height > FBCON_MAX_FONT_HEIGHT) return -EINVAL; /* Make sure drawing engine can handle the font */ - if (!(info->pixmap.blit_x & BIT(font->width - 1)) || - !(info->pixmap.blit_y & BIT(font->height - 1))) + if (!(info->pixmap.blit_x & BIT_ULL(font->width - 1)) || + !(info->pixmap.blit_y & BIT_ULL(font->height - 1))) return -EINVAL; /* Make sure driver can handle the font length */ @@ -3084,8 +3087,8 @@ void fbcon_get_requirement(struct fb_inf vc = vc_cons[i].d; if (vc && vc->vc_mode == KD_TEXT && info->node == con2fb_map[i]) { - caps->x |= 1 << (vc->vc_font.width - 1); - caps->y |= 1 << (vc->vc_font.height - 1); + caps->x |= 1ULL << (vc->vc_font.width - 1); + caps->y |= 1ULL << (vc->vc_font.height - 1); charcnt = vc->vc_font.charcount; if (caps->len < charcnt) caps->len = charcnt; @@ -3096,8 +3099,8 @@ void fbcon_get_requirement(struct fb_inf if (vc && vc->vc_mode == KD_TEXT && info->node == con2fb_map[fg_console]) { - caps->x = 1 << (vc->vc_font.width - 1); - caps->y = 1 << (vc->vc_font.height - 1); + caps->x = 1ULL << (vc->vc_font.width - 1); + caps->y = 1ULL << (vc->vc_font.height - 1); caps->len = vc->vc_font.charcount; } } Index: linux-6.4/include/linux/fb.h === --- linux-6.4.orig/include/linux/fb.h +++ linux-6.4/include/linux/fb.h @@ -143,8 +143,8 @@ struct fb_event { }; struct fb_blit_caps { - u32 x; - u32 y; + u64 x; + u64 y; u32 len; u32 flags; }; @@ -191,10 +191,10 @@ struct fb_pixmap { u32 scan_align; /* alignment per scanline */ u32 access_align; /* alignment per read/write (bits) */ u32 flags; /* see FB_PIXMAP_* */ - u32 blit_x; /* supported bit block dimensions (1-32)*/ - u32 blit_y; /* Format: blit_x = 1 << (width - 1)*/ + u64 blit_x; /* supported bit block dimensions (1-64)*/ + u64 blit_y; /* Format: blit_x = 1 << (width - 1)*/ /* blit_y = 1 << (height - 1) */ - /* if 0, will be set to 0x (all)*/ + /* if 0, will be set to ~0ull (all) */ /* access methods */ void (*writeio)(struct fb_info *info, void __iomem *dst, void *src, unsigned int size); void (*readio) (struct fb_info *info, void *dst, void __iomem *src, unsigned int size);
Re: [PATCH v2] drm/i915/hwmon: Fix locking inversion in sysfs getter
On Tue, 12 Mar 2024 13:34:25 -0700, Janusz Krzysztofik wrote: > Hi Janusz, > On Tuesday, 12 March 2024 17:25:14 CET Dixit, Ashutosh wrote: > > On Mon, 11 Mar 2024 13:34:58 -0700, Janusz Krzysztofik wrote: > > > > > > In i915 hwmon sysfs getter path we now take a hwmon_lock, then acquire an > > > rpm wakeref. That results in lock inversion: > > > > > > <4> [197.079335] == > > > <4> [197.085473] WARNING: possible circular locking dependency detected > > > <4> [197.091611] 6.8.0-rc7-Patchwork_129026v7-gc4dc92fb1152+ #1 Not > > > tainted > > > <4> [197.098096] -- > > > <4> [197.104231] prometheus-node/839 is trying to acquire lock: > > > <4> [197.109680] 82764d80 (fs_reclaim){+.+.}-{0:0}, at: > > > __kmalloc+0x9a/0x350 > > > <4> [197.116939] > > > but task is already holding lock: > > > <4> [197.122730] 88811b772a40 (&hwmon->hwmon_lock){+.+.}-{3:3}, at: > > > hwm_energy+0x4b/0x100 [i915] > > > <4> [197.131543] > > > which lock already depends on the new lock. > > > ... > > > <4> [197.507922] Chain exists of: > > > fs_reclaim --> >->reset.mutex --> &hwmon->hwmon_lock > > > <4> [197.518528] Possible unsafe locking scenario: > > > <4> [197.524411]CPU0CPU1 > > > <4> [197.528916] > > > <4> [197.533418] lock(&hwmon->hwmon_lock); > > > <4> [197.537237]lock(>->reset.mutex); > > > <4> [197.543376]lock(&hwmon->hwmon_lock); > > > <4> [197.549682] lock(fs_reclaim); > > > ... > > > <4> [197.632548] Call Trace: > > > <4> [197.634990] > > > <4> [197.637088] dump_stack_lvl+0x64/0xb0 > > > <4> [197.640738] check_noncircular+0x15e/0x180 > > > <4> [197.652968] check_prev_add+0xe9/0xce0 > > > <4> [197.656705] __lock_acquire+0x179f/0x2300 > > > <4> [197.660694] lock_acquire+0xd8/0x2d0 > > > <4> [197.673009] fs_reclaim_acquire+0xa1/0xd0 > > > <4> [197.680478] __kmalloc+0x9a/0x350 > > > <4> [197.689063] acpi_ns_internalize_name.part.0+0x4a/0xb0 > > > <4> [197.694170] acpi_ns_get_node_unlocked+0x60/0xf0 > > > <4> [197.720608] acpi_ns_get_node+0x3b/0x60 > > > <4> [197.724428] acpi_get_handle+0x57/0xb0 > > > <4> [197.728164] acpi_has_method+0x20/0x50 > > > <4> [197.731896] acpi_pci_set_power_state+0x43/0x120 > > > <4> [197.736485] pci_power_up+0x24/0x1c0 > > > <4> [197.740047] pci_pm_default_resume_early+0x9/0x30 > > > <4> [197.744725] pci_pm_runtime_resume+0x2d/0x90 > > > <4> [197.753911] __rpm_callback+0x3c/0x110 > > > <4> [197.762586] rpm_callback+0x58/0x70 > > > <4> [197.766064] rpm_resume+0x51e/0x730 > > > <4> [197.769542] rpm_resume+0x267/0x730 > > > <4> [197.773020] rpm_resume+0x267/0x730 > > > <4> [197.776498] rpm_resume+0x267/0x730 > > > <4> [197.779974] __pm_runtime_resume+0x49/0x90 > > > <4> [197.784055] __intel_runtime_pm_get+0x19/0xa0 [i915] > > > <4> [197.789070] hwm_energy+0x55/0x100 [i915] > > > <4> [197.793183] hwm_read+0x9a/0x310 [i915] > > > <4> [197.797124] hwmon_attr_show+0x36/0x120 > > > <4> [197.800946] dev_attr_show+0x15/0x60 > > > <4> [197.804509] sysfs_kf_seq_show+0xb5/0x100 > > > > > > Acquire the wakeref before the lock and hold it as long as the lock is > > > also held. Follow that pattern across the whole source file where similar > > > lock inversion can happen. > > > > > > v2: Keep hardware read under the lock so the whole operation of updating > > > energy from hardware is still atomic (Guenter), > > > - instead, acquire the rpm wakeref before the lock and hold it as long > > > as the lock is held, > > > - use the same aproach for other similar places across the i915_hwmon.c > > > source file (Rodrigo). > > > > > > Fixes: c41b8bdcc297 ("drm/i915/hwmon: Show device level energy usage") > > > > I would think that the lock inversion issue was introduced here: > > > > 1b44019a93e2 ("drm/i915/guc: Disable PL1 power limit when loading GuC > > firmware") > > > > This is the commit which introduced this sequence: > > lock(>->reset.mutex); > > lock(&hwmon->hwmon_lock); > > > > Before this, everything was fine. So perhaps the Fixes tag should reference > > this commit? > > OK, thanks for pointing that out. > > > Otherwise the patch LGTM: > > > > Reviewed-by: Ashutosh Dixit Thanks for fixing this. Somehow I didn't see it when I did 1b44019a93e2. Maybe just didn't have lockdep enabled in the kernel. Thanks. -- Ashutosh
Re: [PATCH v5 2/4] drm/i915/gt: Refactor uabi engine class/instance list creation
On Tue, Mar 12, 2024 at 10:08:33AM -0700, Matt Roper wrote: > On Fri, Mar 08, 2024 at 09:22:17PM +0100, Andi Shyti wrote: > > For the upcoming changes we need a cleaner way to build the list > > of uabi engines. > > > > Suggested-by: Tvrtko Ursulin > > Signed-off-by: Andi Shyti > > Cc: # v6.2+ > > I don't really see why we need patches 2 & 3 in this series. For patch number '2' We had a round of review with Tvrtko and we wanted to avoid the change I pasted at the bottom[*], which would decrease something that was increased earlier. > If we want > to restrict the platform to a single CCS engine for now (and give that > single engine access to all of the cslices), it would be much simpler to > only create a single intel_engine_cs which which would then cause both > i915 and userspace to only consider a single engine, even if more than > one is physically present. That could be done with a simple adjustment > to engine_mask_apply_compute_fuses() to mask off extra bits from the > engine mask such that only a single CCS can get returned rather than the > mask of all CCSs that are present. > > Managing all of the engines in the KMD but only exposing one (some) of > them to userspace might be something we need if you want to add extra > functionality down to road to "hotplug" extra engines, or to allow > userspace to explicitly request multi-CCS mode. But none of that seems > necessary for this series, especially for something you're backporting > to stable kernels. It's true, it would even be easier to mask out all the CCS engines after the first. I thought of this. On one hand hand, adding a for_each_available_engine() throught the stable path its a bit of abusing, but it's functional to the single CCS mode. I was aiming for a longer term solution. If I add a patch to mask off CCS engines, then I will need to revert it quite soon for the stable release. I'm not sure which one is better, though. Thanks, Andi [*] diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c index 833987015b8b..7041acc77810 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c @@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915) if (engine->uabi_class == I915_NO_UABI_CLASS) continue; + /* +* Do not list and do not count CCS engines other than the first +*/ + if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE && + engine->uabi_instance > 0) { + i915->engine_uabi_class_count[engine->uabi_class]--; + continue; + } + rb_link_node(&engine->uabi_node, prev, p); rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
Re: [PATCH v5 1/4] drm/i915/gt: Disable HW load balancing for CCS
Hi Matt, ... > > #define GEN12_RCU_MODE _MMIO(0x14800) > > #define GEN12_RCU_MODE_CCS_ENABLEREG_BIT(0) > > +#define XEHP_RCU_MODE_FIXED_SLICE_CCS_MODE REG_BIT(1) > > Nitpick: we usually order register bits in descending order. Aside from > that, I can take care of it. > Reviewed-by: Matt Roper Thanks! Andi
Re: [PATCH v2] drm/i915/hwmon: Fix locking inversion in sysfs getter
On Tuesday, 12 March 2024 18:09:37 CET Andi Shyti wrote: > Hi Janusz, > > On Mon, Mar 11, 2024 at 09:34:58PM +0100, Janusz Krzysztofik wrote: > > In i915 hwmon sysfs getter path we now take a hwmon_lock, then acquire an > > rpm wakeref. That results in lock inversion: > > > > <4> [197.079335] == > > <4> [197.085473] WARNING: possible circular locking dependency detected > > <4> [197.091611] 6.8.0-rc7-Patchwork_129026v7-gc4dc92fb1152+ #1 Not tainted > > <4> [197.098096] -- > > <4> [197.104231] prometheus-node/839 is trying to acquire lock: > > <4> [197.109680] 82764d80 (fs_reclaim){+.+.}-{0:0}, at: > > __kmalloc+0x9a/0x350 > > <4> [197.116939] > > but task is already holding lock: > > <4> [197.122730] 88811b772a40 (&hwmon->hwmon_lock){+.+.}-{3:3}, at: > > hwm_energy+0x4b/0x100 [i915] > > <4> [197.131543] > > which lock already depends on the new lock. > > ... > > <4> [197.507922] Chain exists of: > > fs_reclaim --> >->reset.mutex --> &hwmon->hwmon_lock > > <4> [197.518528] Possible unsafe locking scenario: > > <4> [197.524411]CPU0CPU1 > > <4> [197.528916] > > <4> [197.533418] lock(&hwmon->hwmon_lock); > > <4> [197.537237]lock(>->reset.mutex); > > <4> [197.543376]lock(&hwmon->hwmon_lock); > > <4> [197.549682] lock(fs_reclaim); > > ... > > <4> [197.632548] Call Trace: > > <4> [197.634990] > > <4> [197.637088] dump_stack_lvl+0x64/0xb0 > > <4> [197.640738] check_noncircular+0x15e/0x180 > > <4> [197.652968] check_prev_add+0xe9/0xce0 > > <4> [197.656705] __lock_acquire+0x179f/0x2300 > > <4> [197.660694] lock_acquire+0xd8/0x2d0 > > <4> [197.673009] fs_reclaim_acquire+0xa1/0xd0 > > <4> [197.680478] __kmalloc+0x9a/0x350 > > <4> [197.689063] acpi_ns_internalize_name.part.0+0x4a/0xb0 > > <4> [197.694170] acpi_ns_get_node_unlocked+0x60/0xf0 > > <4> [197.720608] acpi_ns_get_node+0x3b/0x60 > > <4> [197.724428] acpi_get_handle+0x57/0xb0 > > <4> [197.728164] acpi_has_method+0x20/0x50 > > <4> [197.731896] acpi_pci_set_power_state+0x43/0x120 > > <4> [197.736485] pci_power_up+0x24/0x1c0 > > <4> [197.740047] pci_pm_default_resume_early+0x9/0x30 > > <4> [197.744725] pci_pm_runtime_resume+0x2d/0x90 > > <4> [197.753911] __rpm_callback+0x3c/0x110 > > <4> [197.762586] rpm_callback+0x58/0x70 > > <4> [197.766064] rpm_resume+0x51e/0x730 > > <4> [197.769542] rpm_resume+0x267/0x730 > > <4> [197.773020] rpm_resume+0x267/0x730 > > <4> [197.776498] rpm_resume+0x267/0x730 > > <4> [197.779974] __pm_runtime_resume+0x49/0x90 > > <4> [197.784055] __intel_runtime_pm_get+0x19/0xa0 [i915] > > <4> [197.789070] hwm_energy+0x55/0x100 [i915] > > <4> [197.793183] hwm_read+0x9a/0x310 [i915] > > <4> [197.797124] hwmon_attr_show+0x36/0x120 > > <4> [197.800946] dev_attr_show+0x15/0x60 > > <4> [197.804509] sysfs_kf_seq_show+0xb5/0x100 > > > > Acquire the wakeref before the lock and hold it as long as the lock is > > also held. Follow that pattern across the whole source file where similar > > lock inversion can happen. > > > > v2: Keep hardware read under the lock so the whole operation of updating > > energy from hardware is still atomic (Guenter), > > - instead, acquire the rpm wakeref before the lock and hold it as long > > as the lock is held, > > - use the same aproach for other similar places across the i915_hwmon.c > > source file (Rodrigo). > > > > Fixes: c41b8bdcc297 ("drm/i915/hwmon: Show device level energy usage") > > Signed-off-by: Janusz Krzysztofik > > Cc: Rodrigo Vivi > > Cc: Guenter Roeck > > Cc: # v6.2+ > > Reviewed-by: Andi Shyti > > If you want I can change the Fixes tag as suggested by Ashutosh > before applying the patch before pushing the change. Yes, please do, and then also s/v6.2+/v6.5+/. Thanks, Janusz > > Andi >
Re: [PATCH v2] drm/i915/hwmon: Fix locking inversion in sysfs getter
Hi Ashutosh, On Tuesday, 12 March 2024 17:25:14 CET Dixit, Ashutosh wrote: > On Mon, 11 Mar 2024 13:34:58 -0700, Janusz Krzysztofik wrote: > > > > In i915 hwmon sysfs getter path we now take a hwmon_lock, then acquire an > > rpm wakeref. That results in lock inversion: > > > > <4> [197.079335] == > > <4> [197.085473] WARNING: possible circular locking dependency detected > > <4> [197.091611] 6.8.0-rc7-Patchwork_129026v7-gc4dc92fb1152+ #1 Not tainted > > <4> [197.098096] -- > > <4> [197.104231] prometheus-node/839 is trying to acquire lock: > > <4> [197.109680] 82764d80 (fs_reclaim){+.+.}-{0:0}, at: > > __kmalloc+0x9a/0x350 > > <4> [197.116939] > > but task is already holding lock: > > <4> [197.122730] 88811b772a40 (&hwmon->hwmon_lock){+.+.}-{3:3}, at: > > hwm_energy+0x4b/0x100 [i915] > > <4> [197.131543] > > which lock already depends on the new lock. > > ... > > <4> [197.507922] Chain exists of: > > fs_reclaim --> >->reset.mutex --> &hwmon->hwmon_lock > > <4> [197.518528] Possible unsafe locking scenario: > > <4> [197.524411]CPU0CPU1 > > <4> [197.528916] > > <4> [197.533418] lock(&hwmon->hwmon_lock); > > <4> [197.537237]lock(>->reset.mutex); > > <4> [197.543376]lock(&hwmon->hwmon_lock); > > <4> [197.549682] lock(fs_reclaim); > > ... > > <4> [197.632548] Call Trace: > > <4> [197.634990] > > <4> [197.637088] dump_stack_lvl+0x64/0xb0 > > <4> [197.640738] check_noncircular+0x15e/0x180 > > <4> [197.652968] check_prev_add+0xe9/0xce0 > > <4> [197.656705] __lock_acquire+0x179f/0x2300 > > <4> [197.660694] lock_acquire+0xd8/0x2d0 > > <4> [197.673009] fs_reclaim_acquire+0xa1/0xd0 > > <4> [197.680478] __kmalloc+0x9a/0x350 > > <4> [197.689063] acpi_ns_internalize_name.part.0+0x4a/0xb0 > > <4> [197.694170] acpi_ns_get_node_unlocked+0x60/0xf0 > > <4> [197.720608] acpi_ns_get_node+0x3b/0x60 > > <4> [197.724428] acpi_get_handle+0x57/0xb0 > > <4> [197.728164] acpi_has_method+0x20/0x50 > > <4> [197.731896] acpi_pci_set_power_state+0x43/0x120 > > <4> [197.736485] pci_power_up+0x24/0x1c0 > > <4> [197.740047] pci_pm_default_resume_early+0x9/0x30 > > <4> [197.744725] pci_pm_runtime_resume+0x2d/0x90 > > <4> [197.753911] __rpm_callback+0x3c/0x110 > > <4> [197.762586] rpm_callback+0x58/0x70 > > <4> [197.766064] rpm_resume+0x51e/0x730 > > <4> [197.769542] rpm_resume+0x267/0x730 > > <4> [197.773020] rpm_resume+0x267/0x730 > > <4> [197.776498] rpm_resume+0x267/0x730 > > <4> [197.779974] __pm_runtime_resume+0x49/0x90 > > <4> [197.784055] __intel_runtime_pm_get+0x19/0xa0 [i915] > > <4> [197.789070] hwm_energy+0x55/0x100 [i915] > > <4> [197.793183] hwm_read+0x9a/0x310 [i915] > > <4> [197.797124] hwmon_attr_show+0x36/0x120 > > <4> [197.800946] dev_attr_show+0x15/0x60 > > <4> [197.804509] sysfs_kf_seq_show+0xb5/0x100 > > > > Acquire the wakeref before the lock and hold it as long as the lock is > > also held. Follow that pattern across the whole source file where similar > > lock inversion can happen. > > > > v2: Keep hardware read under the lock so the whole operation of updating > > energy from hardware is still atomic (Guenter), > > - instead, acquire the rpm wakeref before the lock and hold it as long > > as the lock is held, > > - use the same aproach for other similar places across the i915_hwmon.c > > source file (Rodrigo). > > > > Fixes: c41b8bdcc297 ("drm/i915/hwmon: Show device level energy usage") > > I would think that the lock inversion issue was introduced here: > > 1b44019a93e2 ("drm/i915/guc: Disable PL1 power limit when loading GuC > firmware") > > This is the commit which introduced this sequence: > lock(>->reset.mutex); > lock(&hwmon->hwmon_lock); > > Before this, everything was fine. So perhaps the Fixes tag should reference > this commit? OK, thanks for pointing that out. > Otherwise the patch LGTM: > > Reviewed-by: Ashutosh Dixit Thank you, Janusz
Re: [PATCH 0/7] Additions to "Reimplement line-per-line pixel conversion for plane reading" series
On 03/08, Maíra Canal wrote: > Hi Arthur, > > Would it be possible for you to coordinate with Louis and create a > single series with all the modification? > > I don't see a reason to submit fixes to a series that it is still > on review. I agree. Moreover, the fix for drm_fixp2int_round() should go in a single patch detached from these multiple work branches, since it is addressing an issue already upstream. Thanks, Melissa > > Best Regards, > - Maíra > > On 3/6/24 17:03, Arthur Grillo wrote: > > These are some patches that add some fixes/features to the series by > > Louis Chauvet[1], it was based on top of the patches from v4. > > > > Patches #2 and #3 should be amended to "[PATCH v4 11/14] drm/vkms: Add > > YUV support". To make patch #3 work, we need patch #1. So, please, add > > it before the patch that #2 and #3 amend to. > > > > Patches #4 to #6 should be amended to "[PATCH v4 14/14] drm/vkms: Create > > KUnit tests for YUV conversions". While doing the additions, I found > > some compilation issues, so I fixed them (patch #4). That's when I > > thought that it would be good to add some documentation on how to run > > them (patch #7), this patch should be added to the series as new patch. > > > > [1]: https://lore.kernel.org/r/20240304-yuv-v4-0-76beac8e9...@bootlin.com > > > > To: Rodrigo Siqueira > > To: Melissa Wen > > To: Maíra Canal > > To: Haneen Mohammed > > To: Daniel Vetter > > To: Maarten Lankhorst > > To: Maxime Ripard > > To: Thomas Zimmermann > > To: David Airlie > > To: arthurgri...@riseup.net > > To: Jonathan Corbet > > To: pekka.paala...@haloniitty.fi > > To: Louis Chauvet > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-ker...@vger.kernel.org > > Cc: jeremie.dautheri...@bootlin.com > > Cc: miquel.ray...@bootlin.com > > Cc: thomas.petazz...@bootlin.com > > Cc: seanp...@google.com > > Cc: marc...@google.com > > Cc: nicolejade...@google.com > > > > Signed-off-by: Arthur Grillo > > --- > > Arthur Grillo (7): > >drm: Fix drm_fixp2int_round() making it add 0.5 > >drm/vkms: Add comments > >drm/vkmm: Use drm_fixed api > >drm/vkms: Fix compilation issues > >drm/vkms: Add comments to format tests > >drm/vkms: Change the gray RGB representation > >drm/vkms: Add how to run the Kunit tests > > > > Documentation/gpu/vkms.rst| 11 +++ > > drivers/gpu/drm/vkms/tests/vkms_format_test.c | 81 +++-- > > drivers/gpu/drm/vkms/vkms_drv.h | 4 + > > drivers/gpu/drm/vkms/vkms_formats.c | 101 > > +++--- > > include/drm/drm_fixed.h | 2 +- > > 5 files changed, 165 insertions(+), 34 deletions(-) > > --- > > base-commit: 9658aba38ae9f3f3068506c9c8e93e85b500fcb4 > > change-id: 20240306-louis-vkms-conv-61362ff12ab8 > > > > Best regards,
Re: [PATCH 2/7] drm/vkms: Add comments
On 03/06, Arthur Grillo wrote: Hi, can you describe better the scope of your changes? Limiting the scope in the commit message and also describing in the body a summary of what you are documenting and reasons. Thanks, Melissa > Signed-off-by: Arthur Grillo > --- > drivers/gpu/drm/vkms/vkms_formats.c | 47 > + > 1 file changed, 47 insertions(+) > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c > b/drivers/gpu/drm/vkms/vkms_formats.c > index 44d9b9b3bdc3..55ed3f598bd7 100644 > --- a/drivers/gpu/drm/vkms/vkms_formats.c > +++ b/drivers/gpu/drm/vkms/vkms_formats.c > @@ -577,6 +577,18 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 0, > }; > + > + /* > + * Those matrixies were generated using the colour python framework > + * > + * Below are the function calls used to generate eac matrix, go to > + * > https://colour.readthedocs.io/en/develop/generated/colour.matrix_YCbCr.html > + * for more info: > + * > + * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R > BT.601"], > + * is_legal = False, > + * bits = 8) * 2**32).astype(int) > + */ > static struct conversion_matrix yuv_bt601_full = { > .matrix = { > { 4294967296, 0, 6021544149 }, > @@ -585,6 +597,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 0, > }; > + > + /* > + * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R > BT.601"], > + * is_legal = True, > + * bits = 8) * 2**32).astype(int) > + */ > static struct conversion_matrix yuv_bt601_limited = { > .matrix = { > { 5020601039, 0, 6881764740 }, > @@ -593,6 +611,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 16, > }; > + > + /* > + * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R > BT.709"], > + * is_legal = False, > + * bits = 8) * 2**32).astype(int) > + */ > static struct conversion_matrix yuv_bt709_full = { > .matrix = { > { 4294967296, 0, 6763714498 }, > @@ -601,6 +625,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 0, > }; > + > + /* > + * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R > BT.709"], > + * is_legal = True, > + * bits = 8) * 2**32).astype(int) > + */ > static struct conversion_matrix yuv_bt709_limited = { > .matrix = { > { 5020601039, 0, 7729959424 }, > @@ -609,6 +639,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 16, > }; > + > + /* > + * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R > BT.2020"], > + * is_legal = False, > + * bits = 8) * 2**32).astype(int) > + */ > static struct conversion_matrix yuv_bt2020_full = { > .matrix = { > { 4294967296, 0, 658775 }, > @@ -617,6 +653,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 0, > }; > + > + /* > + * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R > BT.2020"], > + * is_legal = True, > + * bits = 8) * 2**32).astype(int) > + */ > static struct conversion_matrix yuv_bt2020_limited = { > .matrix = { > { 5020601039, 0, 7238124312 }, > @@ -625,6 +667,11 @@ get_conversion_matrix_to_argb_u16(u32 format, enum > drm_color_encoding encoding, > }, > .y_offset = 16, > }; > + > + /* > + * The next matrices are just the previous ones, but with the first and > + * second columns swapped > + */ > static struct conversion_matrix yvu_bt601_full = { > .matrix = { > { 4294967296, 6021544149, 0 }, > > -- > 2.43.0 >
Re: [PATCH 6/7] drm/vkms: Change the gray RGB representation
On 03/06, Arthur Grillo wrote: > Using the drm_fixed calls, this needs to be changed. Which in fact is > more correct, colour.YCbCr_to_RGB() gives 0x8080 for same the yuv > parameters. Hi Arthur, For consistency, shouldn't this change be together with the previous patch that uses the drm_fixed api? I mean, a single patch that changes to drm_fixed calls and adjust the color values accordingly, avoiding room for mismatches? Melissa > > Signed-off-by: Arthur Grillo > --- > drivers/gpu/drm/vkms/tests/vkms_format_test.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c > b/drivers/gpu/drm/vkms/tests/vkms_format_test.c > index 66cdd83c3d25..49125cf76eb5 100644 > --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c > +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c > @@ -48,7 +48,7 @@ static struct yuv_u8_to_argb_u16_case > yuv_u8_to_argb_u16_cases[] = { > .n_colors = 6, > .colors = { > {"white", {0xff, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > - {"gray", {0x80, 0x80, 0x80}, {0x, 0x8000, 0x8000, > 0x8000}}, > + {"gray", {0x80, 0x80, 0x80}, {0x, 0x8080, 0x8080, > 0x8080}}, > {"black", {0x00, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > {"red", {0x4c, 0x55, 0xff}, {0x, 0x, 0x, > 0x}}, > {"green", {0x96, 0x2c, 0x15}, {0x, 0x, 0x, > 0x}}, > @@ -71,7 +71,7 @@ static struct yuv_u8_to_argb_u16_case > yuv_u8_to_argb_u16_cases[] = { > .n_colors = 6, > .colors = { > {"white", {0xeb, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > - {"gray", {0x7e, 0x80, 0x80}, {0x, 0x8000, 0x8000, > 0x8000}}, > + {"gray", {0x7e, 0x80, 0x80}, {0x, 0x8080, 0x8080, > 0x8080}}, > {"black", {0x10, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > {"red", {0x51, 0x5a, 0xf0}, {0x, 0x, 0x, > 0x}}, > {"green", {0x91, 0x36, 0x22}, {0x, 0x, 0x, > 0x}}, > @@ -94,7 +94,7 @@ static struct yuv_u8_to_argb_u16_case > yuv_u8_to_argb_u16_cases[] = { > .n_colors = 4, > .colors = { > {"white", {0xff, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > - {"gray", {0x80, 0x80, 0x80}, {0x, 0x8000, 0x8000, > 0x8000}}, > + {"gray", {0x80, 0x80, 0x80}, {0x, 0x8080, 0x8080, > 0x8080}}, > {"black", {0x00, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > {"red", {0x36, 0x63, 0xff}, {0x, 0x, 0x, > 0x}}, > {"green", {0xb6, 0x1e, 0x0c}, {0x, 0x, 0x, > 0x}}, > @@ -117,7 +117,7 @@ static struct yuv_u8_to_argb_u16_case > yuv_u8_to_argb_u16_cases[] = { > .n_colors = 4, > .colors = { > {"white", {0xeb, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > - {"gray", {0x7e, 0x80, 0x80}, {0x, 0x8000, 0x8000, > 0x8000}}, > + {"gray", {0x7e, 0x80, 0x80}, {0x, 0x8080, 0x8080, > 0x8080}}, > {"black", {0x10, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > {"red", {0x3f, 0x66, 0xf0}, {0x, 0x, 0x, > 0x}}, > {"green", {0xad, 0x2a, 0x1a}, {0x, 0x, 0x, > 0x}}, > @@ -140,7 +140,7 @@ static struct yuv_u8_to_argb_u16_case > yuv_u8_to_argb_u16_cases[] = { > .n_colors = 4, > .colors = { > {"white", {0xff, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > - {"gray", {0x80, 0x80, 0x80}, {0x, 0x8000, 0x8000, > 0x8000}}, > + {"gray", {0x80, 0x80, 0x80}, {0x, 0x8080, 0x8080, > 0x8080}}, > {"black", {0x00, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > {"red", {0x43, 0x5c, 0xff}, {0x, 0x, 0x, > 0x}}, > {"green", {0xad, 0x24, 0x0b}, {0x, 0x, 0x, > 0x}}, > @@ -163,7 +163,7 @@ static struct yuv_u8_to_argb_u16_case > yuv_u8_to_argb_u16_cases[] = { > .n_colors = 4, > .colors = { > {"white", {0xeb, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > - {"gray", {0x7e, 0x80, 0x80}, {0x, 0x8000, 0x8000, > 0x8000}}, > + {"gray", {0x7e, 0x80, 0x80}, {0x, 0x8080, 0x8080, > 0x8080}}, > {"black", {0x10, 0x80, 0x80}, {0x, 0x, 0x, > 0x}}, > {"red", {0x4a, 0x61, 0xf0}, {0x, 0x, 0x, > 0x}}, >
Re: [PATCH] drm: Don't return unsupported formats in drm_mode_legacy_fb_format
Hi Thomas, Thomas Zimmermann writes: > Am 11.03.24 um 20:34 schrieb Frej Drejhammar: > >> What I can't really see is what "switch all fbdev code over to >> drm_driver_legacy_fb_format" would entail [...] > > Your patch modifies drm_mode_legacy_fb_format() in a number of > *_fbdev_*.c files. In those instances, the code could certainly use > drm_driver_legacy_fb_format() instead. > > The fbdev files provide the base for the kernel framebuffer console > and should behave like DRM clients in userspace; That's the understanding I was missing, thank you for the explanation! I'll work up a two-patch series during the week-end. > About tilcdc: it uses fbdev-dma, which sets up an XRGB format > [1]. Shouldn't this already fail? Do you see a framebuffer console? I get the penguin, but I'm running this particular Beagleboard with a serial console, so I don't know if the rest of the console works. From some extra kprints I can see that (the unmodified) drm_mode_legacy_fb_format() returns RG16 which means that the framebuffer asked for a depth=16, bpp=16 format, and RG16 is one of the supported formats, so the check that triggers the regression won't trigger. Best regards, Frej
RE: [PATCH 29/43] drm/renesas/rz-du: Use fbdev-dma
Hi Thomas, > -Original Message- > From: Thomas Zimmermann > Sent: Tuesday, March 12, 2024 3:45 PM > To: dan...@ffwll.ch; airl...@gmail.com; del...@gmx.de; javi...@redhat.com > Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org; Thomas > Zimmermann > ; Biju Das > Subject: [PATCH 29/43] drm/renesas/rz-du: Use fbdev-dma > > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage > handling, which is required by > rz-du. Avoids the overhead of fbdev-generic's additional shadow buffering. No > functional changes. > > Signed-off-by: Thomas Zimmermann Tested-by: Biju Das Cheers, Biju > Cc: Biju Das > --- > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > b/drivers/gpu/drm/renesas/rz- > du/rzg2l_du_drv.c > index 470d34da1d6c4..e5eca8691a331 100644 > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c > @@ -14,7 +14,7 @@ > > #include > #include > -#include > +#include > #include > #include > > @@ -149,7 +149,7 @@ static int rzg2l_du_probe(struct platform_device *pdev) > > drm_info(&rcdu->ddev, "Device %s probed\n", dev_name(&pdev->dev)); > > - drm_fbdev_generic_setup(&rcdu->ddev, 32); > + drm_fbdev_dma_setup(&rcdu->ddev, 32); > > return 0; > > -- > 2.44.0
Re: [RFC PATCH v4 07/42] drm/vkms: Avoid reading beyond LUT array
On 02/26, Harry Wentland wrote: > When the floor LUT index (drm_fixp2int(lut_index) is the last > index of the array the ceil LUT index will point to an entry > beyond the array. Make sure we guard against it and use the > value of the floor LUT index. > > v3: > - Drop bits from commit description that didn't contribute >anything of value > > Fixes: db1f254f2cfaf ("drm/vkms: Add support to 1D gamma LUT") > Signed-off-by: Harry Wentland > Cc: Arthur Grillo > Reviewed-by: Melissa Wen Same. Already applied upstream: https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm?id=2fee84030d12d9fddfa874e4562d71761a129277 I guess you are working on top of asdn branch and it's okay to me. I'm just mentioning to avoid confusion. Melissa > --- > drivers/gpu/drm/vkms/vkms_composer.c | 14 ++ > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c > b/drivers/gpu/drm/vkms/vkms_composer.c > index d178f2a400f6..b90e446d5954 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct > vkms_color_lut *lut, u16 chan > enum lut_channel channel) > { > s64 lut_index = get_lut_index(lut, channel_value); > + u16 *floor_lut_value, *ceil_lut_value; > + u16 floor_channel_value, ceil_channel_value; > > /* >* This checks if `struct drm_color_lut` has any gap added by the > compiler > @@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct > vkms_color_lut *lut, u16 chan >*/ > static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4); > > - u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)]; > - u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)]; > + floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)]; > + if (drm_fixp2int(lut_index) == (lut->lut_length - 1)) > + /* We're at the end of the LUT array, use same value for ceil > and floor */ > + ceil_lut_value = floor_lut_value; > + else > + ceil_lut_value = (__u16 > *)&lut->base[drm_fixp2int_ceil(lut_index)]; > > - u16 floor_channel_value = floor_lut_value[channel]; > - u16 ceil_channel_value = ceil_lut_value[channel]; > + floor_channel_value = floor_lut_value[channel]; > + ceil_channel_value = ceil_lut_value[channel]; > > return lerp_u16(floor_channel_value, ceil_channel_value, > lut_index & DRM_FIXED_DECIMAL_MASK); > -- > 2.44.0 >
Re: [RFC PATCH v4 05/42] drm/vkms: Create separate Kconfig file for VKMS
On 02/26, Harry Wentland wrote: > This aligns with most other DRM drivers and will allow > us to add new VKMS config options without polluting > the DRM Kconfig. > > v3: > - Change SPDX to GPL-2.0-only to match DRM KConfig >SPDX (Simon) > > Signed-off-by: Harry Wentland > Reviewed-by: Simon Ser Just to let you know this patch is already upstream. I cherry-picked it from the last patchset version and applied to drm-misc-next: https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm?id=ffcc67cd79ff2e93fd0bdb837c99cbab6c59d38c Thanks again, Melissa > --- > drivers/gpu/drm/Kconfig | 14 +- > drivers/gpu/drm/vkms/Kconfig | 15 +++ > 2 files changed, 16 insertions(+), 13 deletions(-) > create mode 100644 drivers/gpu/drm/vkms/Kconfig > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 2520db0b776e..e3ea8793cb8a 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -289,19 +289,7 @@ config DRM_VGEM > as used by Mesa's software renderer for enhanced performance. > If M is selected the module will be called vgem. > > -config DRM_VKMS > - tristate "Virtual KMS (EXPERIMENTAL)" > - depends on DRM && MMU > - select DRM_KMS_HELPER > - select DRM_GEM_SHMEM_HELPER > - select CRC32 > - default n > - help > - Virtual Kernel Mode-Setting (VKMS) is used for testing or for > - running GPU in a headless machines. Choose this option to get > - a VKMS. > - > - If M is selected the module will be called vkms. > +source "drivers/gpu/drm/vkms/Kconfig" > > source "drivers/gpu/drm/exynos/Kconfig" > > diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig > new file mode 100644 > index ..b9ecdebecb0b > --- /dev/null > +++ b/drivers/gpu/drm/vkms/Kconfig > @@ -0,0 +1,15 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +config DRM_VKMS > + tristate "Virtual KMS (EXPERIMENTAL)" > + depends on DRM && MMU > + select DRM_KMS_HELPER > + select DRM_GEM_SHMEM_HELPER > + select CRC32 > + default n > + help > + Virtual Kernel Mode-Setting (VKMS) is used for testing or for > + running GPU in a headless machines. Choose this option to get > + a VKMS. > + > + If M is selected the module will be called vkms. > -- > 2.44.0 >
Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5
On 03/06, Arthur Grillo wrote: > As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong. > To round a number, you need to add 0.5 to the number and floor that, > drm_fixp2int_round() is adding 0.076. Make it add 0.5. > > [1]: > https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paala...@collabora.com/ > Hi Arthur, thanks for addressing this issue. Please, add a fix tag to the commit that you are fixing, so we can easily backport. Might be this commit: https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665 > Suggested-by: Pekka Paalanen > Signed-off-by: Arthur Grillo > --- > include/drm/drm_fixed.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h > index 0c9f917a4d4b..de3a79909ac9 100644 > --- a/include/drm/drm_fixed.h > +++ b/include/drm/drm_fixed.h > @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a) > > static inline int drm_fixp2int_round(s64 a) > { > - return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1))); Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also remove it as it won't be used anymore? > + return drm_fixp2int(a + DRM_FIXED_ONE / 2); Would this division be equivalent to just shifting 1ULL by 31 instead of 32 as done in DRM_FIXED_ONE? Melissa > } > > static inline int drm_fixp2int_ceil(s64 a) > > -- > 2.43.0 >
Re: [PATCH 11/43] drm/hyperv: Use fbdev-shmem
Reviewed-by: Deepak Rawat On Tue, Mar 12, 2024 at 8:48 AM Thomas Zimmermann wrote: > > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Deepak Rawat > --- > drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > index cff85086f2d66..ff93e08d5036d 100644 > --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > @@ -11,7 +11,7 @@ > #include > #include > #include > -#include > +#include > #include > #include > > @@ -149,7 +149,7 @@ static int hyperv_vmbus_probe(struct hv_device *hdev, > goto err_free_mmio; > } > > - drm_fbdev_generic_setup(dev, 0); > + drm_fbdev_shmem_setup(dev, 0); > > return 0; > > -- > 2.44.0 >
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 26/02/2024 17:09, Mark Brown wrote: On Mon, Feb 26, 2024 at 03:01:50PM +0100, amerg...@baylibre.com wrote: index ..13e95c227114 --- /dev/null +++ b/sound/soc/codecs/mt6357.c @@ -0,0 +1,1805 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * MT6357 ALSA SoC audio codec driver + * Please use a C++ comment for the whole comment to make it clearer that this is intentional. ok +static void set_playback_gpio(struct mt6357_priv *priv, bool enable) +{ + if (enable) { + /* set gpio mosi mode */ + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI | + GPIO9_MODE_SET_AUD_DAT_MOSI0 | + GPIO10_MODE_SET_AUD_DAT_MOSI1 | + GPIO11_MODE_SET_AUD_SYNC_MOSI); This would be a lot more legible if you worked out the values to set and then had a single set of writes, currently the indentation makes it very hard to read. Similarly for other similar functions. ok +static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); + struct mt6357_priv *priv = snd_soc_component_get_drvdata(component); + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; + unsigned int reg; + int ret; + + ret = snd_soc_put_volsw(kcontrol, ucontrol); + if (ret < 0) + return ret; + + switch (mc->reg) { + case MT6357_ZCD_CON2: + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®); + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; + break; It would probably be less code and would definitely be clearer and simpler to just read the values when we need them rather than constatly keeping a cache separate to the register cache. Actually you must save the values because the gain selected by the user will be override to do a ramp => volume_ramp(.): - When you switch on the HP, you start from gain=-40db to final_gain (selected by user). - When you switch off the HP, you start from final_gain (selected by user) to gain=-40db. Also, the microphone's gain change when it's enabled/disabled. So, it can implemented differently but currently it's aligned with the other MTK codecs and I don't see any resource wasted here. + /* ul channel swap */ + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), On/off controls should end in Switch. Sorry, I don't understand your comment. Can you reword it please ? +static const char * const hslo_mux_map[] = { + "Open", "DACR", "Playback", "Test mode" +}; + +static int hslo_mux_map_value[] = { + 0x0, 0x1, 0x2, 0x3, +}; Why not just use a normal mux here, there's no missing values or reordering? Similarly for other muxes. I've dug into some other codecs and it's done like that, but I've probably misunderstood something. The only bad thing I see is enum is missing currently: enum { PGA_MUX_OPEN = 0, PGA_MUX_DACR, PGA_MUX_PB, PGA_MUX_TM, PGA_MUX_MASK = 0x3, }; static const char * const hslo_mux_map[] = { "Open", "DACR", "Playback", "Test mode" }; static int hslo_mux_map_value[] = { PGA_MUX_OPEN, PGA_MUX_DACR, PGA_MUX_PB, PGA_MUX_TM, }; +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) +{ + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); + unsigned int val; + + pr_debug("%s() reg = 0x%x", __func__, reg); + regmap_read(priv->regmap, reg, &val); + return val; +} Remove these, there are vastly more logging facilities as standard in the regmap core. ok +/* Reg bit defines */ +/* MT6357_GPIO_DIR0 */ +#define GPIO8_DIR_MASK BIT(8) +#define GPIO8_DIR_INPUT0 Please namespace your defines, these look very generic. ok -- Regards, Alexandre
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On 3/12/2024 9:59 AM, Johan Hovold wrote: On Tue, Mar 12, 2024 at 05:41:23PM +0100, Johan Hovold wrote: On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote: On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote: @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device *dev) { struct dp_display_private *dp = dev_get_dp_display_private(dev); + dp->dp_display.link_ready = false; As I also pointed out in the other thread, setting link_ready to false here means that any spurious connect event (during physical disconnect) will always be processed, something which can currently lead to a leaked runtime pm reference. Wasting some power is of course preferred over crashing the machine, but please take it into consideration anyway. Especially if your intention with this patch was to address the resets we saw with sc8280xp which are gone since the HPD notify revert (which fixed the hotplug detect issue that left the bridge in a half-initialised state). Heh. This is getting ridiculous. I just tried running with this patch and it again breaks hotplug detect in a VT console and in X (where I could enable a reconnected external display by running xrandr twice before). So, please, do not apply this one. To make things worse, I indeed also hit the reset when disconnecting after such a failed hotplug. Johan Ack, I will hold off till I analyze your issues more which you have listed in separate replies. Especially about the spurious connect, I believe you are trying to mention that, by adding logs, you are able to delay the processing of a connect event to *make* it like a spurious one? In case, I got this part wrong, can you pls explain the spurious connect scenario again? A short response on why this change was made is that commit can be issued by userspace or the fbdev client. So userspace involvement only makes commit happen from a different path. It would be incorrect to assume the issues from the earlier bug and the current one are different only because there was userspace involvement in that one and not this. Because in the end, it manifests itself in the same way that atomic_enable() did not go through after an atomic_disable() and the next atomic_disable() crashes. Reverting the hpd_notify patch only eliminated some paths but I think both you and me agree the issue can still happen and in the very same way. So till someone else reports this issue, till HPD is reworked, I wanted to do whats possible to avoid this situation. If users are fine with what we have, I have no inclination to push this.
Re: [PATCH v2] drm/i915/hwmon: Fix locking inversion in sysfs getter
Hi Janusz, On Mon, Mar 11, 2024 at 09:34:58PM +0100, Janusz Krzysztofik wrote: > In i915 hwmon sysfs getter path we now take a hwmon_lock, then acquire an > rpm wakeref. That results in lock inversion: > > <4> [197.079335] == > <4> [197.085473] WARNING: possible circular locking dependency detected > <4> [197.091611] 6.8.0-rc7-Patchwork_129026v7-gc4dc92fb1152+ #1 Not tainted > <4> [197.098096] -- > <4> [197.104231] prometheus-node/839 is trying to acquire lock: > <4> [197.109680] 82764d80 (fs_reclaim){+.+.}-{0:0}, at: > __kmalloc+0x9a/0x350 > <4> [197.116939] > but task is already holding lock: > <4> [197.122730] 88811b772a40 (&hwmon->hwmon_lock){+.+.}-{3:3}, at: > hwm_energy+0x4b/0x100 [i915] > <4> [197.131543] > which lock already depends on the new lock. > ... > <4> [197.507922] Chain exists of: > fs_reclaim --> >->reset.mutex --> &hwmon->hwmon_lock > <4> [197.518528] Possible unsafe locking scenario: > <4> [197.524411]CPU0CPU1 > <4> [197.528916] > <4> [197.533418] lock(&hwmon->hwmon_lock); > <4> [197.537237]lock(>->reset.mutex); > <4> [197.543376]lock(&hwmon->hwmon_lock); > <4> [197.549682] lock(fs_reclaim); > ... > <4> [197.632548] Call Trace: > <4> [197.634990] > <4> [197.637088] dump_stack_lvl+0x64/0xb0 > <4> [197.640738] check_noncircular+0x15e/0x180 > <4> [197.652968] check_prev_add+0xe9/0xce0 > <4> [197.656705] __lock_acquire+0x179f/0x2300 > <4> [197.660694] lock_acquire+0xd8/0x2d0 > <4> [197.673009] fs_reclaim_acquire+0xa1/0xd0 > <4> [197.680478] __kmalloc+0x9a/0x350 > <4> [197.689063] acpi_ns_internalize_name.part.0+0x4a/0xb0 > <4> [197.694170] acpi_ns_get_node_unlocked+0x60/0xf0 > <4> [197.720608] acpi_ns_get_node+0x3b/0x60 > <4> [197.724428] acpi_get_handle+0x57/0xb0 > <4> [197.728164] acpi_has_method+0x20/0x50 > <4> [197.731896] acpi_pci_set_power_state+0x43/0x120 > <4> [197.736485] pci_power_up+0x24/0x1c0 > <4> [197.740047] pci_pm_default_resume_early+0x9/0x30 > <4> [197.744725] pci_pm_runtime_resume+0x2d/0x90 > <4> [197.753911] __rpm_callback+0x3c/0x110 > <4> [197.762586] rpm_callback+0x58/0x70 > <4> [197.766064] rpm_resume+0x51e/0x730 > <4> [197.769542] rpm_resume+0x267/0x730 > <4> [197.773020] rpm_resume+0x267/0x730 > <4> [197.776498] rpm_resume+0x267/0x730 > <4> [197.779974] __pm_runtime_resume+0x49/0x90 > <4> [197.784055] __intel_runtime_pm_get+0x19/0xa0 [i915] > <4> [197.789070] hwm_energy+0x55/0x100 [i915] > <4> [197.793183] hwm_read+0x9a/0x310 [i915] > <4> [197.797124] hwmon_attr_show+0x36/0x120 > <4> [197.800946] dev_attr_show+0x15/0x60 > <4> [197.804509] sysfs_kf_seq_show+0xb5/0x100 > > Acquire the wakeref before the lock and hold it as long as the lock is > also held. Follow that pattern across the whole source file where similar > lock inversion can happen. > > v2: Keep hardware read under the lock so the whole operation of updating > energy from hardware is still atomic (Guenter), > - instead, acquire the rpm wakeref before the lock and hold it as long > as the lock is held, > - use the same aproach for other similar places across the i915_hwmon.c > source file (Rodrigo). > > Fixes: c41b8bdcc297 ("drm/i915/hwmon: Show device level energy usage") > Signed-off-by: Janusz Krzysztofik > Cc: Rodrigo Vivi > Cc: Guenter Roeck > Cc: # v6.2+ Reviewed-by: Andi Shyti If you want I can change the Fixes tag as suggested by Ashutosh before applying the patch before pushing the change. Andi
Re: [PATCH v5 2/4] drm/i915/gt: Refactor uabi engine class/instance list creation
On Fri, Mar 08, 2024 at 09:22:17PM +0100, Andi Shyti wrote: > For the upcoming changes we need a cleaner way to build the list > of uabi engines. > > Suggested-by: Tvrtko Ursulin > Signed-off-by: Andi Shyti > Cc: # v6.2+ I don't really see why we need patches 2 & 3 in this series. If we want to restrict the platform to a single CCS engine for now (and give that single engine access to all of the cslices), it would be much simpler to only create a single intel_engine_cs which which would then cause both i915 and userspace to only consider a single engine, even if more than one is physically present. That could be done with a simple adjustment to engine_mask_apply_compute_fuses() to mask off extra bits from the engine mask such that only a single CCS can get returned rather than the mask of all CCSs that are present. Managing all of the engines in the KMD but only exposing one (some) of them to userspace might be something we need if you want to add extra functionality down to road to "hotplug" extra engines, or to allow userspace to explicitly request multi-CCS mode. But none of that seems necessary for this series, especially for something you're backporting to stable kernels. Matt > --- > drivers/gpu/drm/i915/gt/intel_engine_user.c | 29 - > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c > b/drivers/gpu/drm/i915/gt/intel_engine_user.c > index 833987015b8b..11cc06c0c785 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c > @@ -203,7 +203,7 @@ static void engine_rename(struct intel_engine_cs *engine, > const char *name, u16 > > void intel_engines_driver_register(struct drm_i915_private *i915) > { > - u16 name_instance, other_instance = 0; > + u16 class_instance[I915_LAST_UABI_ENGINE_CLASS + 2] = { }; > struct legacy_ring ring = {}; > struct list_head *it, *next; > struct rb_node **p, *prev; > @@ -214,6 +214,8 @@ void intel_engines_driver_register(struct > drm_i915_private *i915) > prev = NULL; > p = &i915->uabi_engines.rb_node; > list_for_each_safe(it, next, &engines) { > + u16 uabi_class; > + > struct intel_engine_cs *engine = > container_of(it, typeof(*engine), uabi_list); > > @@ -222,15 +224,14 @@ void intel_engines_driver_register(struct > drm_i915_private *i915) > > GEM_BUG_ON(engine->class >= ARRAY_SIZE(uabi_classes)); > engine->uabi_class = uabi_classes[engine->class]; > - if (engine->uabi_class == I915_NO_UABI_CLASS) { > - name_instance = other_instance++; > - } else { > - GEM_BUG_ON(engine->uabi_class >= > -ARRAY_SIZE(i915->engine_uabi_class_count)); > - name_instance = > - > i915->engine_uabi_class_count[engine->uabi_class]++; > - } > - engine->uabi_instance = name_instance; > + > + if (engine->uabi_class == I915_NO_UABI_CLASS) > + uabi_class = I915_LAST_UABI_ENGINE_CLASS + 1; > + else > + uabi_class = engine->uabi_class; > + > + GEM_BUG_ON(uabi_class >= ARRAY_SIZE(class_instance)); > + engine->uabi_instance = class_instance[uabi_class]++; > > /* >* Replace the internal name with the final user and log facing > @@ -238,11 +239,15 @@ void intel_engines_driver_register(struct > drm_i915_private *i915) >*/ > engine_rename(engine, > intel_engine_class_repr(engine->class), > - name_instance); > + engine->uabi_instance); > > - if (engine->uabi_class == I915_NO_UABI_CLASS) > + if (uabi_class > I915_LAST_UABI_ENGINE_CLASS) > continue; > > + GEM_BUG_ON(uabi_class >= > +ARRAY_SIZE(i915->engine_uabi_class_count)); > + i915->engine_uabi_class_count[uabi_class]++; > + > rb_link_node(&engine->uabi_node, prev, p); > rb_insert_color(&engine->uabi_node, &i915->uabi_engines); > > -- > 2.43.0 > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
[PATCH 12/14] sh: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). Signed-off-by: Guenter Roeck --- arch/sh/include/asm/bug.h | 26 ++ 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/arch/sh/include/asm/bug.h b/arch/sh/include/asm/bug.h index 05a485c4fabc..cadc335eb759 100644 --- a/arch/sh/include/asm/bug.h +++ b/arch/sh/include/asm/bug.h @@ -24,21 +24,36 @@ * The offending file and line are encoded in the __bug_table section. */ #ifdef CONFIG_DEBUG_BUGVERBOSE + +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR"\t.long %O2\n" +#else +# define __BUG_FUNC_PTR +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #define _EMIT_BUG_ENTRY\ "\t.pushsection __bug_table,\"aw\"\n" \ "2:\t.long 1b, %O1\n" \ - "\t.short %O2, %O3\n" \ - "\t.org 2b+%O4\n" \ + __BUG_FUNC_PTR \ + "\t.short %O3, %O4\n" \ + "\t.org 2b+%O5\n" \ "\t.popsection\n" #else #define _EMIT_BUG_ENTRY\ "\t.pushsection __bug_table,\"aw\"\n" \ "2:\t.long 1b\n"\ - "\t.short %O3\n"\ - "\t.org 2b+%O4\n" \ + "\t.short %O4\n"\ + "\t.org 2b+%O5\n" \ "\t.popsection\n" #endif +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNCNULL +#endif + #define BUG() \ do { \ __asm__ __volatile__ ( \ @@ -47,6 +62,7 @@ do { \ : \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ + "i" (__BUG_FUNC),\ "i" (__LINE__), "i" (0), \ "i" (sizeof(struct bug_entry))); \ unreachable(); \ @@ -60,6 +76,7 @@ do { \ : \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ + "i" (__BUG_FUNC),\ "i" (__LINE__), \ "i" (BUGFLAG_WARNING|(flags)), \ "i" (sizeof(struct bug_entry))); \ @@ -85,6 +102,7 @@ do { \ : \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ + "i" (__BUG_FUNC),\ "i" (__LINE__), \ "i" (BUGFLAG_UNWINDER), \ "i" (sizeof(struct bug_entry))); \ -- 2.39.2
[PATCH 14/14] powerpc: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). Signed-off-by: Guenter Roeck --- arch/powerpc/include/asm/bug.h | 37 +- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index 1db485aacbd9..330d4983f90e 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -14,6 +14,9 @@ .section __bug_table,"aw" 5001: .4byte \addr - . .4byte 5002f - . +#if IS_ENABLED(CONFIG_KUNIT) +.4byte 0 +#endif .short \line, \flags .org 5001b+BUG_ENTRY_SIZE .previous @@ -32,30 +35,46 @@ #endif /* verbose */ #else /* !__ASSEMBLY__ */ -/* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3 to be FILE, LINE, flags and - sizeof(struct bug_entry), respectively */ +/* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3,%4 to be FILE, __func__, LINE, flags + and sizeof(struct bug_entry), respectively */ #ifdef CONFIG_DEBUG_BUGVERBOSE + +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR" .4byte %1 - .\n" +#else +# define __BUG_FUNC_PTR +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #define _EMIT_BUG_ENTRY\ ".section __bug_table,\"aw\"\n" \ "2: .4byte 1b - .\n"\ " .4byte %0 - .\n"\ - " .short %1, %2\n"\ - ".org 2b+%3\n" \ + __BUG_FUNC_PTR \ + " .short %2, %3\n"\ + ".org 2b+%4\n" \ ".previous\n" #else #define _EMIT_BUG_ENTRY\ ".section __bug_table,\"aw\"\n" \ "2: .4byte 1b - .\n"\ - " .short %2\n"\ - ".org 2b+%3\n" \ + " .short %3\n"\ + ".org 2b+%4\n" \ ".previous\n" #endif +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNCNULL +#endif + #define BUG_ENTRY(insn, flags, ...)\ __asm__ __volatile__( \ "1: " insn "\n" \ _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry)), \ ##__VA_ARGS__) @@ -80,7 +99,7 @@ if (x) \ BUG(); \ } else {\ - BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \ + BUG_ENTRY(PPC_TLNEI " %5, 0", 0, "r" ((__force long)(x))); \ } \ } while (0) @@ -90,7 +109,7 @@ if (__ret_warn_on) \ __WARN(); \ } else {\ - BUG_ENTRY(PPC_TLNEI " %4, 0", \ + BUG_ENTRY(PPC_TLNEI " %5, 0", \ BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ "r" (__ret_warn_on)); \ } \ -- 2.39.2
[PATCH 13/14] riscv: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). To simplify the implementation, unify the __BUG_ENTRY_ADDR and __BUG_ENTRY_FILE macros into a single macro named __BUG_REL() which takes the address, file, or function reference as parameter. Signed-off-by: Guenter Roeck --- arch/riscv/include/asm/bug.h | 38 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h index 1aaea81fb141..8955ee5c1c27 100644 --- a/arch/riscv/include/asm/bug.h +++ b/arch/riscv/include/asm/bug.h @@ -30,26 +30,39 @@ typedef u32 bug_insn_t; #ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS -#define __BUG_ENTRY_ADDR RISCV_INT " 1b - ." -#define __BUG_ENTRY_FILE RISCV_INT " %0 - ." +#define __BUG_REL(val) RISCV_INT " " __stringify(val) " - ." #else -#define __BUG_ENTRY_ADDR RISCV_PTR " 1b" -#define __BUG_ENTRY_FILE RISCV_PTR " %0" +#define __BUG_REL(val) RISCV_PTR " " __stringify(val) #endif #ifdef CONFIG_DEBUG_BUGVERBOSE + +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR__BUG_REL(%1) +#else +# define __BUG_FUNC_PTR +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #define __BUG_ENTRY\ - __BUG_ENTRY_ADDR "\n\t" \ - __BUG_ENTRY_FILE "\n\t" \ - RISCV_SHORT " %1\n\t" \ - RISCV_SHORT " %2" + __BUG_REL(1b) "\n\t"\ + __BUG_REL(%0) "\n\t"\ + __BUG_FUNC_PTR "\n\t" \ + RISCV_SHORT " %2\n\t" \ + RISCV_SHORT " %3" #else #define __BUG_ENTRY\ - __BUG_ENTRY_ADDR "\n\t" \ - RISCV_SHORT " %2" + __BUG_REL(1b) "\n\t"\ + RISCV_SHORT " %3" #endif #ifdef CONFIG_GENERIC_BUG +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNCNULL +#endif + #define __BUG_FLAGS(flags) \ do { \ __asm__ __volatile__ ( \ @@ -58,10 +71,11 @@ do { \ ".pushsection __bug_table,\"aw\"\n\t" \ "2:\n\t"\ __BUG_ENTRY "\n\t" \ - ".org 2b + %3\n\t" \ + ".org 2b + %4\n\t" \ ".popsection" \ : \ - : "i" (__FILE__), "i" (__LINE__), \ + : "i" (__FILE__), "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry))); \ } while (0) -- 2.39.2
[PATCH 11/14] s390: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). Signed-off-by: Guenter Roeck --- arch/s390/include/asm/bug.h | 19 +++ 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h index aebe1e22c7be..01e2aa4069d7 100644 --- a/arch/s390/include/asm/bug.h +++ b/arch/s390/include/asm/bug.h @@ -8,19 +8,30 @@ #ifdef CONFIG_DEBUG_BUGVERBOSE +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR" .long %0-.\n" +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNCNULL +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #define __EMIT_BUG(x) do { \ asm_inline volatile(\ "0: mc 0,0\n" \ ".section .rodata.str,\"aMS\",@progbits,1\n"\ "1: .asciz \""__FILE__"\"\n" \ ".previous\n" \ - ".section __bug_table,\"awM\",@progbits,%2\n" \ + ".section __bug_table,\"awM\",@progbits,%3\n" \ "2: .long 0b-.\n" \ " .long 1b-.\n" \ - " .short %0,%1\n"\ - " .org2b+%2\n"\ + __BUG_FUNC_PTR \ + " .short %1,%2\n"\ + " .org2b+%3\n"\ ".previous\n" \ - : : "i" (__LINE__), \ + : : "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (x),\ "i" (sizeof(struct bug_entry)));\ } while (0) -- 2.39.2
[PATCH 04/14] kunit: Add documentation for warning backtrace suppression API
Document API functions for suppressing warning backtraces. Signed-off-by: Guenter Roeck --- Documentation/dev-tools/kunit/usage.rst | 30 - 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 22955d56b379..8d3d36d4103d 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -157,6 +157,34 @@ Alternatively, one can take full control over the error message by using if (some_setup_function()) KUNIT_FAIL(test, "Failed to setup thing for testing"); +Suppressing warning backtraces +-- + +Some unit tests trigger warning backtraces either intentionally or as side +effect. Such backtraces are normally undesirable since they distract from +the actual test and may result in the impression that there is a problem. + +Such backtraces can be suppressed. To suppress a backtrace in some_function(), +use the following code. + +.. code-block:: c + + static void some_test(struct kunit *test) + { + DEFINE_SUPPRESSED_WARNING(some_function); + + START_SUPPRESSED_WARNING(some_function); + trigger_backtrace(); + END_SUPPRESSED_WARNING(some_function); + } + +SUPPRESSED_WARNING_COUNT() returns the number of suppressed backtraces. If the +suppressed backtrace was triggered on purpose, this can be used to check if +the backtrace was actually triggered. + +.. code-block:: c + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(some_function), 1); Test Suites ~~~ @@ -857,4 +885,4 @@ For example: dev_managed_string = devm_kstrdup(fake_device, "Hello, World!"); // Everything is cleaned up automatically when the test ends. - } \ No newline at end of file + } -- 2.39.2
[PATCH 10/14] parisc: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). While at it, declare assembler parameters as constants where possible. Refine .blockz instructions to calculate the necessary padding instead of using fixed values. Signed-off-by: Guenter Roeck --- arch/parisc/include/asm/bug.h | 29 + 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h index 833555f74ffa..792dacc2a653 100644 --- a/arch/parisc/include/asm/bug.h +++ b/arch/parisc/include/asm/bug.h @@ -23,8 +23,17 @@ # define __BUG_REL(val) ".word " __stringify(val) #endif - #ifdef CONFIG_DEBUG_BUGVERBOSE + +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR__BUG_REL(%c1) +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNCNULL +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #define BUG() \ do {\ asm volatile("\n" \ @@ -33,10 +42,12 @@ "\t.align 4\n" \ "2:\t" __BUG_REL(1b) "\n" \ "\t" __BUG_REL(%c0) "\n" \ -"\t.short %1, %2\n"\ -"\t.blockz %3-2*4-2*2\n" \ +"\t" __BUG_FUNC_PTR "\n" \ +"\t.short %c2, %c3\n" \ +"\t.blockz %c4-(.-2b)\n" \ "\t.popsection"\ -: : "i" (__FILE__), "i" (__LINE__),\ +: : "i" (__FILE__), "i" (__BUG_FUNC), \ +"i" (__LINE__),\ "i" (0), "i" (sizeof(struct bug_entry)) ); \ unreachable(); \ } while(0) @@ -58,10 +69,12 @@ "\t.align 4\n" \ "2:\t" __BUG_REL(1b) "\n" \ "\t" __BUG_REL(%c0) "\n" \ -"\t.short %1, %2\n"\ -"\t.blockz %3-2*4-2*2\n" \ +"\t" __BUG_FUNC_PTR "\n" \ +"\t.short %c2, %3\n" \ +"\t.blockz %c4-(.-2b)\n" \ "\t.popsection"\ -: : "i" (__FILE__), "i" (__LINE__),\ +: : "i" (__FILE__), "i" (__BUG_FUNC), \ +"i" (__LINE__),\ "i" (BUGFLAG_WARNING|(flags)), \ "i" (sizeof(struct bug_entry)) ); \ } while(0) @@ -74,7 +87,7 @@ "\t.align 4\n" \ "2:\t" __BUG_REL(1b) "\n" \ "\t.short %0\n"\ -"\t.blockz %1-4-2\n" \ +"\t.blockz %c1-(.-2b)\n" \ "\t.popsection"\ : : "i" (BUGFLAG_WARNING|(flags)), \ "i" (sizeof(struct bug_entry)) ); \ -- 2.39.2
[PATCH 09/14] loongarch: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). Signed-off-by: Guenter Roeck --- arch/loongarch/include/asm/bug.h | 38 +++- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/arch/loongarch/include/asm/bug.h b/arch/loongarch/include/asm/bug.h index d4ca3ba25418..25f2b5ae7702 100644 --- a/arch/loongarch/include/asm/bug.h +++ b/arch/loongarch/include/asm/bug.h @@ -3,47 +3,63 @@ #define __ASM_BUG_H #include +#include #include #ifndef CONFIG_DEBUG_BUGVERBOSE -#define _BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) #else -#define __BUGVERBOSE_LOCATION(file, line) \ +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR(func) .long func - .; +#else +# define __BUG_FUNC_PTR(func) +#endif + +#define __BUGVERBOSE_LOCATION(file, func, line)\ .pushsection .rodata.str, "aMS", @progbits, 1; \ 10002: .string file; \ .popsection;\ \ .long 10002b - .; \ + __BUG_FUNC_PTR(func)\ .short line; -#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) __BUGVERBOSE_LOCATION(file, func, line) #endif #ifndef CONFIG_GENERIC_BUG -#define __BUG_ENTRY(flags) +#define __BUG_ENTRY(flags, func) #else -#define __BUG_ENTRY(flags) \ +#define __BUG_ENTRY(flags, func) \ .pushsection __bug_table, "aw"; \ .align 2; \ 1: .long 10001f - .; \ - _BUGVERBOSE_LOCATION(__FILE__, __LINE__)\ + _BUGVERBOSE_LOCATION(__FILE__, func, __LINE__) \ .short flags; \ .popsection;\ 10001: #endif -#define ASM_BUG_FLAGS(flags) \ - __BUG_ENTRY(flags) \ +#define ASM_BUG_FLAGS(flags, func) \ + __BUG_ENTRY(flags, func)\ break BRK_BUG -#define ASM_BUG() ASM_BUG_FLAGS(0) +#define ASM_BUG() ASM_BUG_FLAGS(0, .) + +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNCNULL +#endif #define __BUG_FLAGS(flags) \ - asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags))); + asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags, %0)) : : "i" (__BUG_FUNC)); #define __WARN_FLAGS(flags)\ do { \ instrumentation_begin();\ - __BUG_FLAGS(BUGFLAG_WARNING|(flags)); \ + if (!IS_SUPPRESSED_WARNING(__func__)) \ + __BUG_FLAGS(BUGFLAG_WARNING|(flags)); \ instrumentation_end(); \ } while (0) -- 2.39.2
[PATCH 07/14] x86: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). Signed-off-by: Guenter Roeck --- arch/x86/include/asm/bug.h | 21 - 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index a3ec87d198ac..d87bf8bab238 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -23,18 +23,28 @@ #ifdef CONFIG_DEBUG_BUGVERBOSE +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR__BUG_REL(%c1) +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNCNULL +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ ".pushsection __bug_table,\"aw\"\n"\ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \ -"\t.word %c1""\t# bug_entry::line\n" \ -"\t.word %c2""\t# bug_entry::flags\n" \ -"\t.org 2b+%c3\n" \ +"\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \ +"\t.word %c2""\t# bug_entry::line\n" \ +"\t.word %c3""\t# bug_entry::flags\n" \ +"\t.org 2b+%c4\n" \ ".popsection\n"\ extra \ -: : "i" (__FILE__), "i" (__LINE__),\ +: : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \ } while (0) @@ -80,7 +90,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin();\ - _BUG_FLAGS(ASM_UD2, __flags, ASM_REACHABLE);\ + if (!IS_SUPPRESSED_WARNING(__func__)) \ + _BUG_FLAGS(ASM_UD2, __flags, ASM_REACHABLE);\ instrumentation_end(); \ } while (0) -- 2.39.2
[PATCH 08/14] arm64: Add support for suppressing warning backtraces
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces. To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable). Signed-off-by: Guenter Roeck --- arch/arm64/include/asm/asm-bug.h | 29 +++-- arch/arm64/include/asm/bug.h | 8 +++- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h index c762038ba400..6884089a7191 100644 --- a/arch/arm64/include/asm/asm-bug.h +++ b/arch/arm64/include/asm/asm-bug.h @@ -8,36 +8,45 @@ #include #ifdef CONFIG_DEBUG_BUGVERBOSE -#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) -#define __BUGVERBOSE_LOCATION(file, line) \ + +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR(func) .long func - .; +#else +# define __BUG_FUNC_PTR(func) +#endif + +#define _BUGVERBOSE_LOCATION(file, func, line) __BUGVERBOSE_LOCATION(file, func, line) +#define __BUGVERBOSE_LOCATION(file, func, line)\ .pushsection .rodata.str,"aMS",@progbits,1; \ 14472: .string file; \ .popsection;\ \ .long 14472b - .; \ + __BUG_FUNC_PTR(func)\ .short line; #else -#define _BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) #endif #ifdef CONFIG_GENERIC_BUG -#define __BUG_ENTRY(flags) \ +#define __BUG_ENTRY(flags, func) \ .pushsection __bug_table,"aw"; \ .align 2; \ 14470: .long 14471f - .; \ -_BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ - .short flags; \ +_BUGVERBOSE_LOCATION(__FILE__, func, __LINE__) \ + .short flags; \ .popsection;\ 14471: #else -#define __BUG_ENTRY(flags) +#define __BUG_ENTRY(flags, func) #endif -#define ASM_BUG_FLAGS(flags) \ - __BUG_ENTRY(flags) \ +#define ASM_BUG_FLAGS(flags, func) \ + __BUG_ENTRY(flags, func)\ brk BUG_BRK_IMM -#define ASM_BUG() ASM_BUG_FLAGS(0) +#define ASM_BUG() ASM_BUG_FLAGS(0, .) #endif /* __ASM_ASM_BUG_H */ diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@ #include +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC__func__ +#else +# define __BUG_FUNCNULL +#endif + #define __BUG_FLAGS(flags) \ - asm volatile (__stringify(ASM_BUG_FLAGS(flags))); + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC)); #define BUG() do { \ __BUG_FLAGS(0); \ -- 2.39.2
[PATCH 06/14] net: kunit: Suppress lock warning noise at end of dev_addr_lists tests
dev_addr_lists_test generates lock warning noise at the end of tests if lock debugging is enabled. There are two sets of warnings. WARNING: CPU: 0 PID: 689 at kernel/locking/mutex.c:923 __mutex_unlock_slowpath.constprop.0+0x13c/0x368 DEBUG_LOCKS_WARN_ON(__owner_task(owner) != __get_current()) WARNING: kunit_try_catch/1336 still has locks held! KUnit test cleanup is not guaranteed to run in the same thread as the test itself. For this test, this means that rtnl_lock() and rtnl_unlock() may be called from different threads. This triggers the warnings. Suppress the warnings because they are irrelevant for the test and just confusing. The first warning can be suppressed by using START_SUPPRESSED_WARNING() and END_SUPPRESSED_WARNING() around the call to rtnl_unlock(). To suppress the second warning, it is necessary to set debug_locks_silent while the rtnl lock is held. Cc: David Gow Cc: Jakub Kicinski Signed-off-by: Guenter Roeck --- net/core/dev_addr_lists_test.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/net/core/dev_addr_lists_test.c b/net/core/dev_addr_lists_test.c index 4dbd0dc6aea2..b427dd1a3c93 100644 --- a/net/core/dev_addr_lists_test.c +++ b/net/core/dev_addr_lists_test.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include +#include #include #include #include @@ -49,6 +50,7 @@ static int dev_addr_test_init(struct kunit *test) KUNIT_FAIL(test, "Can't register netdev %d", err); } + debug_locks_silent = 1; rtnl_lock(); return 0; } @@ -56,8 +58,12 @@ static int dev_addr_test_init(struct kunit *test) static void dev_addr_test_exit(struct kunit *test) { struct net_device *netdev = test->priv; + DEFINE_SUPPRESSED_WARNING(__mutex_unlock_slowpath); + START_SUPPRESSED_WARNING(__mutex_unlock_slowpath); rtnl_unlock(); + END_SUPPRESSED_WARNING(__mutex_unlock_slowpath); + debug_locks_silent = 0; unregister_netdev(netdev); free_netdev(netdev); } -- 2.39.2
[PATCH 03/14] kunit: Add test cases for backtrace warning suppression
Add unit tests to verify that warning backtrace suppression works. If backtrace suppression does _not_ work, the unit tests will likely trigger unsuppressed backtraces, which should actually help to get the affected architectures / platforms fixed. Signed-off-by: Guenter Roeck --- lib/kunit/Makefile | 3 +- lib/kunit/backtrace-suppression-test.c | 104 + 2 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 lib/kunit/backtrace-suppression-test.c diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 545b57c3be48..6a44d2b54ea9 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -19,7 +19,8 @@ endif obj-y += hooks.o \ bug.o -obj-$(CONFIG_KUNIT_TEST) +=kunit-test.o +obj-$(CONFIG_KUNIT_TEST) +=kunit-test.o \ + backtrace-suppression-test.o # string-stream-test compiles built-in only. ifeq ($(CONFIG_KUNIT_TEST),y) diff --git a/lib/kunit/backtrace-suppression-test.c b/lib/kunit/backtrace-suppression-test.c new file mode 100644 index ..47c619283802 --- /dev/null +++ b/lib/kunit/backtrace-suppression-test.c @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for suppressing warning tracebacks + * + * Copyright (C) 2024, Guenter Roeck + * Author: Guenter Roeck + */ + +#include +#include + +static void backtrace_suppression_test_warn_direct(struct kunit *test) +{ + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + WARN(1, "This backtrace should be suppressed"); + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_direct), 1); +} + +static void trigger_backtrace_warn(void) +{ + WARN(1, "This backtrace should be suppressed"); +} + +static void backtrace_suppression_test_warn_indirect(struct kunit *test) +{ + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn); + + START_SUPPRESSED_WARNING(trigger_backtrace_warn); + trigger_backtrace_warn(); + END_SUPPRESSED_WARNING(trigger_backtrace_warn); + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1); +} + +static void backtrace_suppression_test_warn_multi(struct kunit *test) +{ + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn); + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + START_SUPPRESSED_WARNING(trigger_backtrace_warn); + WARN(1, "This backtrace should be suppressed"); + trigger_backtrace_warn(); + END_SUPPRESSED_WARNING(trigger_backtrace_warn); + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_multi), 1); + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1); +} + +static void backtrace_suppression_test_warn_on_direct(struct kunit *test) +{ + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) && !IS_ENABLED(CONFIG_KALLSYMS)) + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS"); + + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + WARN_ON(1); + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + + KUNIT_EXPECT_EQ(test, + SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_on_direct), 1); +} + +static void trigger_backtrace_warn_on(void) +{ + WARN_ON(1); +} + +static void backtrace_suppression_test_warn_on_indirect(struct kunit *test) +{ + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE)) + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE"); + + START_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + trigger_backtrace_warn_on(); + END_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn_on), 1); +} + +static struct kunit_case backtrace_suppression_test_cases[] = { + KUNIT_CASE(backtrace_suppression_test_warn_direct), + KUNIT_CASE(backtrace_suppression_test_warn_indirect), + KUNIT_CASE(backtrace_suppression_test_warn_multi), + KUNIT_CASE(backtrace_suppression_test_warn_on_direct), + KUNIT_CASE(backtrace_suppression_test_warn_on_indirect), + {} +}; + +static struct kunit_suite backtrace_suppression_test_suite = { + .name = "backtrace-suppression-test", + .test_cases = backtrace_suppression_test_cases, +}; +kunit_test_suites(&backtrace_suppression_test_suite); + +
[PATCH 05/14] drm: Suppress intentional warning backtraces in scaling unit tests
The drm_test_rect_calc_hscale and drm_test_rect_calc_vscale unit tests intentionally trigger warning backtraces by providing bad parameters to the tested functions. What is tested is the return value, not the existence of a warning backtrace. Suppress the backtraces to avoid clogging the kernel log. Signed-off-by: Guenter Roeck --- drivers/gpu/drm/tests/drm_rect_test.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/tests/drm_rect_test.c b/drivers/gpu/drm/tests/drm_rect_test.c index 76332cd2ead8..75614cb4deb5 100644 --- a/drivers/gpu/drm/tests/drm_rect_test.c +++ b/drivers/gpu/drm/tests/drm_rect_test.c @@ -406,22 +406,28 @@ KUNIT_ARRAY_PARAM(drm_rect_scale, drm_rect_scale_cases, drm_rect_scale_case_desc static void drm_test_rect_calc_hscale(struct kunit *test) { + DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor; + START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_hscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + END_SUPPRESSED_WARNING(drm_calc_scale); KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); } static void drm_test_rect_calc_vscale(struct kunit *test) { + DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor; + START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_vscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + END_SUPPRESSED_WARNING(drm_calc_scale); KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); } -- 2.39.2
[PATCH 02/14] kunit: bug: Count suppressed warning backtraces
Count suppressed warning backtraces to enable code which suppresses warning backtraces to check if the expected backtrace(s) have been observed. Using atomics for the backtrace count resulted in build errors on some architectures due to include file recursion, so use a plain integer for now. Signed-off-by: Guenter Roeck --- include/kunit/bug.h | 7 ++- lib/kunit/bug.c | 4 +++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/include/kunit/bug.h b/include/kunit/bug.h index 1e34da961599..2097a854ac8c 100644 --- a/include/kunit/bug.h +++ b/include/kunit/bug.h @@ -20,6 +20,7 @@ struct __suppressed_warning { struct list_head node; const char *function; + int counter; }; void __start_suppress_warning(struct __suppressed_warning *warning); @@ -28,7 +29,7 @@ bool __is_suppressed_warning(const char *function); #define DEFINE_SUPPRESSED_WARNING(func)\ struct __suppressed_warning __kunit_suppress_##func = \ - { .function = __stringify(func) } + { .function = __stringify(func), .counter = 0 } #define START_SUPPRESSED_WARNING(func) \ __start_suppress_warning(&__kunit_suppress_##func) @@ -39,12 +40,16 @@ bool __is_suppressed_warning(const char *function); #define IS_SUPPRESSED_WARNING(func) \ __is_suppressed_warning(func) +#define SUPPRESSED_WARNING_COUNT(func) \ + (__kunit_suppress_##func.counter) + #else /* CONFIG_KUNIT */ #define DEFINE_SUPPRESSED_WARNING(func) #define START_SUPPRESSED_WARNING(func) #define END_SUPPRESSED_WARNING(func) #define IS_SUPPRESSED_WARNING(func) (false) +#define SUPPRESSED_WARNING_COUNT(func) (0) #endif /* CONFIG_KUNIT */ #endif /* __ASSEMBLY__ */ diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c index f93544d7a9d1..13b3d896c114 100644 --- a/lib/kunit/bug.c +++ b/lib/kunit/bug.c @@ -32,8 +32,10 @@ bool __is_suppressed_warning(const char *function) return false; list_for_each_entry(warning, &suppressed_warnings, node) { - if (!strcmp(function, warning->function)) + if (!strcmp(function, warning->function)) { + warning->counter++; return true; + } } return false; } -- 2.39.2
[PATCH 01/14] bug/kunit: Core support for suppressing warning backtraces
Some unit tests intentionally trigger warning backtraces by passing bad parameters to API functions. Such unit tests typically check the return value from those calls, not the existence of the warning backtrace. Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons. - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance. One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log. Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Cc: Dan Carpenter Cc: Daniel Diaz Cc: Naresh Kamboju Cc: Kees Cook Signed-off-by: Guenter Roeck --- include/asm-generic/bug.h | 16 +--- include/kunit/bug.h | 51 +++ include/kunit/test.h | 1 + include/linux/bug.h | 13 ++ lib/bug.c | 51 --- lib/kunit/Makefile| 6 +++-- lib/kunit/bug.c | 40 ++ 7 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/bug.c diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 6e794420bd39..c170b6477689 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -18,6 +18,7 @@ #endif #ifndef __ASSEMBLY__ +#include #include #include @@ -39,8 +40,14 @@ struct bug_entry { #ifdef CONFIG_DEBUG_BUGVERBOSE #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS const char *file; +#ifdef HAVE_BUG_FUNCTION + const char *function; +#endif #else signed int file_disp; +#ifdef HAVE_BUG_FUNCTION + signed int function_disp; +#endif #endif unsigned short line; #endif @@ -96,15 +103,18 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #define __WARN() __WARN_printf(TAINT_WARN, NULL) #define __WARN_printf(taint, arg...) do { \ instrumentation_begin();\ - warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \ + if (!IS_SUPPRESSED_WARNING(__func__)) \ + warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);\ instrumentation_end(); \ } while (0) #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \ instrumentation_begin();\ - __warn_printk(arg); \ - __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + if (!IS_SUPPRESSED_WARNING(__func__)) { \ + __warn_printk(arg); \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + } \ instrumentation_end(); \ } while (0) #define WARN_ON_ONCE(condition) ({ \ diff --git a/include/kunit/bug.h b/include/kunit/bug.h new file mode 100644 index ..1e34da961599 --- /dev/null +++ b/include/kunit/bug.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * KUnit helpers for backtrace suppression + * + * Copyright (c) 2024 Guenter Roeck + */ + +#ifndef _KUNIT_BUG_H +#define _KUNIT_BUG_H + +#ifndef __ASSEMBLY__ + +#include + +#if IS_ENABLED(CONFIG_KUNIT) + +#include +#include + +struct __suppressed_warning { + struct list_head node; + const char *function; +}; + +void __start_suppress_warning(struct __suppressed_warning *warning); +void __end_suppress_warning(struct __suppressed_warning *warning); +bool __is_suppressed_warning(const char *function); + +#define DEFINE_SUPPRESSED_WARNING(func)\ + struct __suppressed_warning __kunit_suppress_##func = \ + { .function = __stringify(func) } + +#define START_SUPPRESSED_WARNING(func) \ + __start_suppress_warning(&__kunit_suppress_##func) + +#define END_SUPPRESSED_WARNING(func) \ + __end_suppress_warning(&__kunit_suppress_##func) + +#define IS_SUPPRESSED_WARNING(func) \ + __is_su
[PATCH 00/14] Add support for suppressing warning backtraces
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace. Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons. - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance. One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log. Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled. The first patch of the series introduces the necessary infrastructure. The second patch introduces support for counting suppressed backtraces. This capability is used in patch three to implement unit tests. Patch four documents the new API. The next two patches add support for suppressing backtraces in drm_rect and dev_addr_lists unit tests. These patches are intended to serve as examples for the use of the functionality introduced with this series. The remaining patches implement the necessary changes for all architectures with GENERIC_BUG support. This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b08-ba81-d94f3b691c9a@moroto.mountain/ and offers a more comprehensive solution of the problem discussed there. Design note: Function pointers are only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image size increases if CONFIG_KUNIT=n. There would be some benefits to adding those pointers all the time (reduced complexity, ability to display function names in BUG/WARNING messages). That change, if desired, can be made later. Checkpatch note: Remaining checkpatch errors and warnings were deliberately ignored. Some are triggered by matching coding style or by comments interpreted as code, others by assembler macros which are disliked by checkpatch. Suggestions for improvements are welcome. Changes since RFC: - Minor cleanups and bug fixes - Added support for all affected architectures - Added support for counting suppressed warnings - Added unit tests using those counters - Added patch to suppress warning backtraces in dev_addr_lists tests
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On Tue, Mar 12, 2024 at 05:41:23PM +0100, Johan Hovold wrote: > On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote: > > On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote: > > > > @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device > > > *dev) > > > { > > > struct dp_display_private *dp = dev_get_dp_display_private(dev); > > > > > > + dp->dp_display.link_ready = false; > > > > As I also pointed out in the other thread, setting link_ready to false > > here means that any spurious connect event (during physical disconnect) > > will always be processed, something which can currently lead to a leaked > > runtime pm reference. > > > > Wasting some power is of course preferred over crashing the machine, but > > please take it into consideration anyway. > > > > Especially if your intention with this patch was to address the resets > > we saw with sc8280xp which are gone since the HPD notify revert (which > > fixed the hotplug detect issue that left the bridge in a > > half-initialised state). > > Heh. This is getting ridiculous. I just tried running with this patch > and it again breaks hotplug detect in a VT console and in X (where I > could enable a reconnected external display by running xrandr twice > before). > > So, please, do not apply this one. To make things worse, I indeed also hit the reset when disconnecting after such a failed hotplug. Johan
Re: [PATCH v5 1/4] drm/i915/gt: Disable HW load balancing for CCS
On Fri, Mar 08, 2024 at 09:22:16PM +0100, Andi Shyti wrote: > The hardware should not dynamically balance the load between CCS > engines. Wa_14019159160 recommends disabling it across all > platforms. > > Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement") > Signed-off-by: Andi Shyti > Cc: Chris Wilson > Cc: Joonas Lahtinen > Cc: Matt Roper > Cc: # v6.2+ > --- > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + > drivers/gpu/drm/i915/gt/intel_workarounds.c | 23 +++-- > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index 50962cfd1353..cf709f6c05ae 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1478,6 +1478,7 @@ > > #define GEN12_RCU_MODE _MMIO(0x14800) > #define GEN12_RCU_MODE_CCS_ENABLE REG_BIT(0) > +#define XEHP_RCU_MODE_FIXED_SLICE_CCS_MODE REG_BIT(1) Nitpick: we usually order register bits in descending order. Aside from that, Reviewed-by: Matt Roper although I still hope our architects will push through a formal documentation update for this. Matt > > #define CHV_FUSE_GT _MMIO(VLV_GUNIT_BASE + 0x2168) > #define CHV_FGT_DISABLE_SS0(1 << 10) > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 25413809b9dc..4865eb5ca9c9 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -51,7 +51,8 @@ > * registers belonging to BCS, VCS or VECS should be implemented in > * xcs_engine_wa_init(). Workarounds for registers not belonging to a > specific > * engine's MMIO range but that are part of of the common RCS/CCS reset > domain > - * should be implemented in general_render_compute_wa_init(). > + * should be implemented in general_render_compute_wa_init(). The settings > + * about the CCS load balancing should be added in ccs_engine_wa_mode(). > * > * - GT workarounds: the list of these WAs is applied whenever these > registers > * revert to their default values: on GPU reset, suspend/resume [1]_, etc. > @@ -2854,6 +2855,22 @@ add_render_compute_tuning_settings(struct intel_gt *gt, > wa_write_clr(wal, GEN8_GARBCNTL, GEN12_BUS_HASH_CTL_BIT_EXC); > } > > +static void ccs_engine_wa_mode(struct intel_engine_cs *engine, struct > i915_wa_list *wal) > +{ > + struct intel_gt *gt = engine->gt; > + > + if (!IS_DG2(gt->i915)) > + return; > + > + /* > + * Wa_14019159160: This workaround, along with others, leads to > + * significant challenges in utilizing load balancing among the > + * CCS slices. Consequently, an architectural decision has been > + * made to completely disable automatic CCS load balancing. > + */ > + wa_masked_en(wal, GEN12_RCU_MODE, XEHP_RCU_MODE_FIXED_SLICE_CCS_MODE); > +} > + > /* > * The workarounds in this function apply to shared registers in > * the general render reset domain that aren't tied to a > @@ -3004,8 +3021,10 @@ engine_init_workarounds(struct intel_engine_cs > *engine, struct i915_wa_list *wal >* to a single RCS/CCS engine's workaround list since >* they're reset as part of the general render domain reset. >*/ > - if (engine->flags & I915_ENGINE_FIRST_RENDER_COMPUTE) > + if (engine->flags & I915_ENGINE_FIRST_RENDER_COMPUTE) { > general_render_compute_wa_init(engine, wal); > + ccs_engine_wa_mode(engine, wal); > + } > > if (engine->class == COMPUTE_CLASS) > ccs_engine_wa_init(engine, wal); > -- > 2.43.0 > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
Re: [v9,9/9] drm/ast: Add drm_panic support
On 2024/3/7 17:14, Jocelyn Falempe wrote: Add support for the drm_panic module, which displays a message to the screen when a kernel panic occurs. v7 * Use drm_for_each_primary_visible_plane() v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions Signed-off-by: Jocelyn Falempe I have done very limited test with ast2600, basically works by trigger via debugfs interface as the cover-letter said. so, Tested-by: Sui Jingfeng
Re: [v9,5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
Hi, I'm get attracted by your patch, so I want to comment. Please correct or ignore me if I say something wrong. On 2024/3/7 17:14, Jocelyn Falempe wrote: This was initialy done for imx6, but should work on most drivers using drm_fb_dma_helper. v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * go back to get_scanout_buffer() * move get_scanout_buffer() to plane helper functions Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 47 + include/drm/drm_fb_dma_helper.h | 4 +++ 2 files changed, 51 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index 3b535ad1b07c..010327069ad4 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, } } EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent); + +#if defined(CONFIG_DRM_PANIC) +/** + * @plane: DRM primary plane + * @drm_scanout_buffer: scanout buffer for the panic handler + * Returns: 0 or negative error code + * + * Generic get_scanout_buffer() implementation, for drivers that uses the + * drm_fb_dma_helper. + */ +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb) +{ + struct drm_gem_dma_object *dma_obj; + struct drm_framebuffer *fb; + + fb = plane->state->fb; + /* Only support linear modifier */ + if (fb->modifier != DRM_FORMAT_MOD_LINEAR) + return -ENODEV; I think, the framebuffer format check clause here should be moved to the core layer. For example, move this check into thedrm_panic_is_format_supported() function. And update the drm_panic_is_format_supported() function to make it take 'struct drm_framebuffer *fb' as argument. Because this check is needed for all implement, not driver specific or implement specific. I know that some display controller support TILE format, but then you can try to trigger modeset to let the display controller using linear format to display. Or, you can also convert local linear buffer(with content pained) to the device specific TILED framebuffer, then feed TILED framebuffer to the display controller to scanout. This is something like reverse the process of resolve, but the convert program is running on the CPU. As panic is not performance critical, so it's possible. This maybe a bit more difficult, but the idea behind this is that the goal of this drm_panic_gem_get_scanout_buffer() function is just to get a buffer scanout-able. Other things beyond that (like format checking) can be moved to upper level(the caller of it). So you don't need to modify(update) the specific implement if one day you have some fancy idea implemented. + dma_obj = drm_fb_dma_get_gem_obj(fb, 0); + + /* Buffer should be accessible from the CPU */ + if (dma_obj->base.import_attach) + return -ENODEV; + + /* Buffer should be already mapped to CPU */ + if (!dma_obj->vaddr) + return -ENODEV; How about to call drm_gem_vmap_unlocked(dma_obj, &sb->map); ? + iosys_map_set_vaddr(&sb->map, dma_obj->vaddr); + sb->format = fb->format; + sb->height = fb->height; + sb->width = fb->width; + sb->pitch = fb->pitches[0]; + return 0; +} +#else +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb) +{ + return -ENODEV; +} +#endif +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer); diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h index d5e036c57801..61f24c2aba2f 100644 --- a/include/drm/drm_fb_dma_helper.h +++ b/include/drm/drm_fb_dma_helper.h @@ -7,6 +7,7 @@ struct drm_device; struct drm_framebuffer; struct drm_plane_state; +struct drm_scanout_buffer; struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb, unsigned int plane); @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, struct drm_plane_state *old_state, struct drm_plane_state *state); +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb); + #endif
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote: > On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote: > > @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device > > *dev) > > { > > struct dp_display_private *dp = dev_get_dp_display_private(dev); > > > > + dp->dp_display.link_ready = false; > > As I also pointed out in the other thread, setting link_ready to false > here means that any spurious connect event (during physical disconnect) > will always be processed, something which can currently lead to a leaked > runtime pm reference. > > Wasting some power is of course preferred over crashing the machine, but > please take it into consideration anyway. > > Especially if your intention with this patch was to address the resets > we saw with sc8280xp which are gone since the HPD notify revert (which > fixed the hotplug detect issue that left the bridge in a > half-initialised state). Heh. This is getting ridiculous. I just tried running with this patch and it again breaks hotplug detect in a VT console and in X (where I could enable a reconnected external display by running xrandr twice before). So, please, do not apply this one. Johan
Re: [PATCH 2/5] drm/i915: Drop dead code for xehpsdv
On Mon, Mar 11, 2024 at 11:16:06AM -0400, Rodrigo Vivi wrote: On Wed, Mar 06, 2024 at 11:36:40AM -0800, Lucas De Marchi wrote: PCI IDs for XEHPSDV were never added and platform always marked with force_probe. Drop what's not used and rename some places to either be xehp or dg2, depending on the platform/IP checks. The registers not used anymore are also removed. Signed-off-by: Lucas De Marchi --- Potential problem here that needs a deeper look, the changes in __gen12_fw_ranges. Some ranges had comments saying they were XEHPSDV so I removed them, but it needs to be double checked with spec and CI results. I have checked the specs and your patch looks right because those bits should be reserved for DG2. But the main issue I see is that we were using that (wrongly?) for DG2 so far. So it probably deserves a separate patch anyway. With this patch only removing the comments and a separate patch to remove that for DG2 (and standalone CI run on that patch by itself): After double checking I think the main issue is that the changed table became wrong since it poke holes. From the docs: * All platforms' forcewake tables below must be sorted by offset ranges. * Furthermore, new forcewake tables added should be "watertight" and hav * no gaps between ranges. I *think* this would be the more correct change: @@ -1533,21 +1533,16 @@ static const struct intel_forcewake_range __gen12_fw_ranges[] = { 0x12000 - 0x127ff: always on \ 0x12800 - 0x12fff: reserved */ \ GEN_FW_RANGE(0x13000, 0x131ff, FORCEWAKE_MEDIA_VDBOX0), /* DG2 only */ \ - GEN_FW_RANGE(0x13200, 0x13fff, FORCEWAKE_MEDIA_VDBOX2), /* \ + GEN_FW_RANGE(0x13200, 0x147ff, FORCEWAKE_MEDIA_VDBOX2), /* \ 0x13200 - 0x133ff: VD2 (DG2 only) \ - 0x13400 - 0x13fff: reserved */ \ - GEN_FW_RANGE(0x14000, 0x141ff, FORCEWAKE_MEDIA_VDBOX0), /* XEHPSDV only */ \ - GEN_FW_RANGE(0x14200, 0x143ff, FORCEWAKE_MEDIA_VDBOX2), /* XEHPSDV only */ \ - GEN_FW_RANGE(0x14400, 0x145ff, FORCEWAKE_MEDIA_VDBOX4), /* XEHPSDV only */ \ - GEN_FW_RANGE(0x14600, 0x147ff, FORCEWAKE_MEDIA_VDBOX6), /* XEHPSDV only */ \ + 0x13400 - 0x147ff: reserved */ \ GEN_FW_RANGE(0x14800, 0x14fff, FORCEWAKE_RENDER), \ GEN_FW_RANGE(0x15000, 0x16dff, FORCEWAKE_GT), /* \ 0x15000 - 0x15fff: gt (DG2 only) \ 0x16000 - 0x16dff: reserved */ \ - GEN_FW_RANGE(0x16e00, 0x1, FORCEWAKE_RENDER), \ - GEN_FW_RANGE(0x2, 0x21fff, FORCEWAKE_MEDIA_VDBOX0), /* \ - 0x2 - 0x20fff: VD0 (XEHPSDV only) \ - 0x21000 - 0x21fff: reserved */ \ + GEN_FW_RANGE(0x16e00, 0x21fff, FORCEWAKE_RENDER), /* \ + 0x16e00 - 0x1: render \ + 0x2 - 0x21fff: reserved */ \ GEN_FW_RANGE(0x22000, 0x23fff, FORCEWAKE_GT), \ GEN_FW_RANGE(0x24000, 0x2417f, 0), /* \ 0x24000 - 0x2407f: always on \ did you find any access on DG2 within the reserved ranges? Lucas De Marchi Reviewed-by: Rodrigo Vivi Documentation/gpu/rfc/i915_vm_bind.h | 11 +-- drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 40 drivers/gpu/drm/i915/gt/intel_gsc.c | 15 --- drivers/gpu/drm/i915/gt/intel_gt_mcr.c| 20 +--- drivers/gpu/drm/i915/gt/intel_gt_regs.h | 50 -- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 21 ++-- drivers/gpu/drm/i915/gt/intel_lrc.c | 43 - drivers/gpu/drm/i915/gt/intel_migrate.c | 18 ++-- drivers/gpu/drm/i915/gt/intel_mocs.c | 31 -- drivers/gpu/drm/i915/gt/intel_rps.c | 2 - drivers/gpu/drm/i915/gt/intel_workarounds.c | 95 --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 4 +- drivers/gpu/drm/i915/i915_drv.h | 4 - drivers/gpu/drm/i915/i915_hwmon.c | 6 -- drivers/gpu/drm/i915/i915_pci.c | 17 drivers/gpu/drm/i915/i915_perf.c | 11 +-- drivers/gpu/drm/i915/i915_reg.h | 3 +- drivers/gpu/drm/i915/intel_clock_gating.c | 10 -- drivers/gpu/drm/i915/intel_device_info.c | 1 - drivers/gpu/drm/i915/intel_device_info.h | 1 - drivers/gpu/drm/i915/intel_step.c | 10 -- drivers/gpu/drm/i915/intel_uncore.c | 15 +-- drivers/gpu
Re: [PATCH v2] drm/i915/hwmon: Fix locking inversion in sysfs getter
On Mon, 11 Mar 2024 13:34:58 -0700, Janusz Krzysztofik wrote: > > In i915 hwmon sysfs getter path we now take a hwmon_lock, then acquire an > rpm wakeref. That results in lock inversion: > > <4> [197.079335] == > <4> [197.085473] WARNING: possible circular locking dependency detected > <4> [197.091611] 6.8.0-rc7-Patchwork_129026v7-gc4dc92fb1152+ #1 Not tainted > <4> [197.098096] -- > <4> [197.104231] prometheus-node/839 is trying to acquire lock: > <4> [197.109680] 82764d80 (fs_reclaim){+.+.}-{0:0}, at: > __kmalloc+0x9a/0x350 > <4> [197.116939] > but task is already holding lock: > <4> [197.122730] 88811b772a40 (&hwmon->hwmon_lock){+.+.}-{3:3}, at: > hwm_energy+0x4b/0x100 [i915] > <4> [197.131543] > which lock already depends on the new lock. > ... > <4> [197.507922] Chain exists of: > fs_reclaim --> >->reset.mutex --> &hwmon->hwmon_lock > <4> [197.518528] Possible unsafe locking scenario: > <4> [197.524411]CPU0CPU1 > <4> [197.528916] > <4> [197.533418] lock(&hwmon->hwmon_lock); > <4> [197.537237]lock(>->reset.mutex); > <4> [197.543376]lock(&hwmon->hwmon_lock); > <4> [197.549682] lock(fs_reclaim); > ... > <4> [197.632548] Call Trace: > <4> [197.634990] > <4> [197.637088] dump_stack_lvl+0x64/0xb0 > <4> [197.640738] check_noncircular+0x15e/0x180 > <4> [197.652968] check_prev_add+0xe9/0xce0 > <4> [197.656705] __lock_acquire+0x179f/0x2300 > <4> [197.660694] lock_acquire+0xd8/0x2d0 > <4> [197.673009] fs_reclaim_acquire+0xa1/0xd0 > <4> [197.680478] __kmalloc+0x9a/0x350 > <4> [197.689063] acpi_ns_internalize_name.part.0+0x4a/0xb0 > <4> [197.694170] acpi_ns_get_node_unlocked+0x60/0xf0 > <4> [197.720608] acpi_ns_get_node+0x3b/0x60 > <4> [197.724428] acpi_get_handle+0x57/0xb0 > <4> [197.728164] acpi_has_method+0x20/0x50 > <4> [197.731896] acpi_pci_set_power_state+0x43/0x120 > <4> [197.736485] pci_power_up+0x24/0x1c0 > <4> [197.740047] pci_pm_default_resume_early+0x9/0x30 > <4> [197.744725] pci_pm_runtime_resume+0x2d/0x90 > <4> [197.753911] __rpm_callback+0x3c/0x110 > <4> [197.762586] rpm_callback+0x58/0x70 > <4> [197.766064] rpm_resume+0x51e/0x730 > <4> [197.769542] rpm_resume+0x267/0x730 > <4> [197.773020] rpm_resume+0x267/0x730 > <4> [197.776498] rpm_resume+0x267/0x730 > <4> [197.779974] __pm_runtime_resume+0x49/0x90 > <4> [197.784055] __intel_runtime_pm_get+0x19/0xa0 [i915] > <4> [197.789070] hwm_energy+0x55/0x100 [i915] > <4> [197.793183] hwm_read+0x9a/0x310 [i915] > <4> [197.797124] hwmon_attr_show+0x36/0x120 > <4> [197.800946] dev_attr_show+0x15/0x60 > <4> [197.804509] sysfs_kf_seq_show+0xb5/0x100 > > Acquire the wakeref before the lock and hold it as long as the lock is > also held. Follow that pattern across the whole source file where similar > lock inversion can happen. > > v2: Keep hardware read under the lock so the whole operation of updating > energy from hardware is still atomic (Guenter), > - instead, acquire the rpm wakeref before the lock and hold it as long > as the lock is held, > - use the same aproach for other similar places across the i915_hwmon.c > source file (Rodrigo). > > Fixes: c41b8bdcc297 ("drm/i915/hwmon: Show device level energy usage") I would think that the lock inversion issue was introduced here: 1b44019a93e2 ("drm/i915/guc: Disable PL1 power limit when loading GuC firmware") This is the commit which introduced this sequence: lock(>->reset.mutex); lock(&hwmon->hwmon_lock); Before this, everything was fine. So perhaps the Fixes tag should reference this commit? Otherwise the patch LGTM: Reviewed-by: Ashutosh Dixit
Re: [PATCH] drm/i915/guc: Update w/a 14019159160
On Thu, Mar 07, 2024 at 06:01:29PM -0800, john.c.harri...@intel.com wrote: > From: John Harrison > > An existing workaround has been extended in both platforms affected > and implementation complexity. > > Signed-off-by: John Harrison > --- > drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h | 3 ++- > drivers/gpu/drm/i915/gt/uc/intel_guc.c| 3 ++- > drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c| 21 ++- > 3 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h > b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h > index bebf28e3c4794..3e7060e859794 100644 > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h > @@ -105,7 +105,8 @@ enum { > * Workaround keys: > */ > enum { > - GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE = > 0x9001, > + GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE = > 0x9001, /* Wa_14019159160 */ > + GUC_WORKAROUND_KLV_AVOID_GFX_CLEAR_WHILE_ACTIVE = > 0x9006, /* Wa_14019159160 */ > }; > > #endif /* _ABI_GUC_KLVS_ABI_H */ > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc.c > index 0c67d674c94de..4c3dae98656af 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c > @@ -296,7 +296,8 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc) > > /* Wa_16019325821 */ > /* Wa_14019159160 */ > - if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) > + if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)) || >From what I can see, this workaround is also needed on Xe_LPG+ (12.74) now. Matt > + IS_DG2(gt->i915)) > flags |= GUC_WA_RCS_CCS_SWITCHOUT; > > /* > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c > index 5c9908b56616e..00fe3c21a9b1c 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c > @@ -815,23 +815,23 @@ guc_capture_prep_lists(struct intel_guc *guc) > return PAGE_ALIGN(total_size); > } > > -/* Wa_14019159160 */ > -static u32 guc_waklv_ra_mode(struct intel_guc *guc, u32 offset, u32 remain) > +static void guc_waklv_enable_simple(struct intel_guc *guc, u32 *offset, u32 > *remain, u32 klv_id) > { > u32 size; > u32 klv_entry[] = { > /* 16:16 key/length */ > - FIELD_PREP(GUC_KLV_0_KEY, > GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE) | > + FIELD_PREP(GUC_KLV_0_KEY, klv_id) | > FIELD_PREP(GUC_KLV_0_LEN, 0), > /* 0 dwords data */ > }; > > size = sizeof(klv_entry); > - GEM_BUG_ON(remain < size); > + GEM_BUG_ON(*remain < size); > > - iosys_map_memcpy_to(&guc->ads_map, offset, klv_entry, size); > + iosys_map_memcpy_to(&guc->ads_map, *offset, klv_entry, size); > > - return size; > + *offset += size; > + *remain -= size; > } > > static void guc_waklv_init(struct intel_guc *guc) > @@ -850,10 +850,11 @@ static void guc_waklv_init(struct intel_guc *guc) > remain = guc_ads_waklv_size(guc); > > /* Wa_14019159160 */ > - if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) { > - size = guc_waklv_ra_mode(guc, offset, remain); > - offset += size; > - remain -= size; > + if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)) || > IS_DG2(gt->i915)) { > + guc_waklv_enable_simple(guc, &offset, &remain, > + GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE); > + guc_waklv_enable_simple(guc, &offset, &remain, > + > GUC_WORKAROUND_KLV_AVOID_GFX_CLEAR_WHILE_ACTIVE); > } > > size = guc_ads_waklv_size(guc) - remain; > -- > 2.43.0 > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
Re: [PATCH 08/43] drm/fbdev: Add fbdev-shmem
Hi Thomas, On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann wrote: > Add an fbdev emulation for SHMEM-based memory managers. The code is > similar to fbdev-generic, but does not require an addition shadow > buffer for mmap(). Fbdev-shmem operates directly on the buffer object's > SHMEM pages. Fbdev's deferred-I/O mechanism updates the hardware state > on write operations. > > Signed-off-by: Thomas Zimmermann Thanks for your patch! > --- /dev/null > +++ b/drivers/gpu/drm/drm_fbdev_shmem.c > +static int drm_fbdev_shmem_helper_fb_probe(struct drm_fb_helper *fb_helper, > + struct drm_fb_helper_surface_size > *sizes) > +{ > + struct drm_client_dev *client = &fb_helper->client; > + struct drm_device *dev = fb_helper->dev; > + struct drm_client_buffer *buffer; > + struct drm_gem_shmem_object *shmem; > + struct drm_framebuffer *fb; > + struct fb_info *info; > + u32 format; > + struct iosys_map map; > + int ret; > + > + drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", > + sizes->surface_width, sizes->surface_height, > + sizes->surface_bpp); > + > + format = drm_mode_legacy_fb_format(sizes->surface_bpp, > sizes->surface_depth); Oops, one more caller of the imprecise let's-guess-the-format-from-bpp-and-depth machinery to get rid of... > + buffer = drm_client_framebuffer_create(client, sizes->surface_width, > + sizes->surface_height, format); [...] > +} > +/** > + * drm_fbdev_shmem_setup() - Setup fbdev emulation for GEM SHMEM helpers > + * @dev: DRM device > + * @preferred_bpp: Preferred bits per pixel for the device. > + * 32 is used if this is zero. > + * > + * This function sets up fbdev emulation for GEM DMA drivers that support > + * dumb buffers with a virtual address and that can be mmap'ed. > + * drm_fbdev_shmem_setup() shall be called after the DRM driver registered > + * the new DRM device with drm_dev_register(). > + * > + * Restore, hotplug events and teardown are all taken care of. Drivers that > do > + * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() > themselves. > + * Simple drivers might use drm_mode_config_helper_suspend(). > + * > + * This function is safe to call even when there are no connectors present. > + * Setup will be retried on the next hotplug event. > + * > + * The fbdev is destroyed by drm_dev_unregister(). > + */ > +void drm_fbdev_shmem_setup(struct drm_device *dev, unsigned int > preferred_bpp) As this is a completely new function, can we please get a preferred_format parameter instead? > +{ > + struct drm_fb_helper *fb_helper; > + int ret; > + > + drm_WARN(dev, !dev->registered, "Device has not been registered.\n"); > + drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n"); > + > + fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL); > + if (!fb_helper) > + return; > + drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, > &drm_fbdev_shmem_helper_funcs); > + > + ret = drm_client_init(dev, &fb_helper->client, "fbdev", > &drm_fbdev_shmem_client_funcs); > + if (ret) { > + drm_err(dev, "Failed to register client: %d\n", ret); > + goto err_drm_client_init; > + } > + > + drm_client_register(&fb_helper->client); > + > + return; > + > +err_drm_client_init: > + drm_fb_helper_unprepare(fb_helper); > + kfree(fb_helper); > +} > +EXPORT_SYMBOL(drm_fbdev_shmem_setup); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v2 2/7] clk: qcom: clk-alpha-pll: Add HUAYRA_2290 support
On 2/23/24 23:48, Trilok Soni wrote: On 2/23/2024 1:21 PM, Konrad Dybcio wrote: + /* Wait 50us for PLL_LOCK_DET bit to go high */ + usleep_range(50, 55); + + /* Enable PLL output */ + regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, PLL_OUTCTRL); +} +EXPORT_SYMBOL(clk_huayra_2290_pll_configure); Please use EXPORT_SYMBOL_GPL. Sure, I glanced over this! I've also noticed that it's a very common oversight.. would you be interested in extending scripts/checkpatch.pl to suggest the _GPL variant? Konrad
Re: [RFC PATCH v4 10/42] drm/colorop: Add TYPE property
On Tue, 12 Mar 2024 15:15:13 + Simon Ser wrote: > On Tuesday, March 12th, 2024 at 16:02, Pekka Paalanen > wrote: > > > This list here is the list of all values the enum could take, right? > > Should it not be just the one value it's going to have? It'll never > > change, and it can never be changed. > > Listing all possible values is how other properties behave. Example: > > "type" (immutable): enum {Overlay, Primary, Cursor} = Primary Yeah, that's weird, now that you pointed it out. Userspace does nothing with the knowledge of what this specific object's property "could" be since it's immutable. Only the kernel internals and UAPI docs need the full list, and userspace needs to hardcode the full list in general so it can recognize each value. But on the property instance on a specific object, I see no use for the full list. Thanks, pq pgpHkVkHOFcJP.pgp Description: OpenPGP digital signature
Re: [RFC PATCH v4 22/42] drm/vkms: Use s32 for internal color pipeline precision
On Mon, 26 Feb 2024 16:10:36 -0500 Harry Wentland wrote: > Certain operations require us to preserve values below 0.0 and > above 1.0 (0x0 and 0x respectively in 16 bpc unorm). One > such operation is a BT709 encoding operation followed by its > decoding operation, or the reverse. > > We'll use s32 values as intermediate in and outputs of our > color operations, for the operations where it matters. > > For now this won't apply to LUT operations. We might want to > update those to work on s32 as well, but it's unclear how > that should work for unorm LUT definitions. We'll revisit > that once we add LUT + CTM tests. > > In order to allow for this we'll also invert the nesting of our > colorop processing loops. We now use the pixel iteration loop > on the outside and the colorop iteration on the inside. I always go through the same thought process: - putting the pixel iteration loop outermost is going to be horrible for performance - maybe should turn the temporary line buffer to argb_s32 for all of VKMS - that's going to be a lot of memory traffic... maybe your way is not horrible for performance - maybe processing pixel by pixel is good, if you can prepare the operation in advance, so you don't need to analyze colorops again and again on each pixel - eh, maybe it's not a gain after all, needs benchmarking My comment on patch 17 was like the step 0 of this train of thought. I just always get the same comments in my mind when seeing the same code again. Thanks, pq > > v4: > - Clarify that we're packing 16-bit UNORM into s32, not >converting values to a different representation (Pekka) > > v3: > - Use new colorop->next pointer > > Signed-off-by: Harry Wentland > --- > drivers/gpu/drm/vkms/vkms_composer.c | 57 +--- > drivers/gpu/drm/vkms/vkms_drv.h | 4 ++ > 2 files changed, 48 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c > b/drivers/gpu/drm/vkms/vkms_composer.c > index 25b786b49db0..d2101fa55aa3 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -164,7 +164,7 @@ static void apply_lut(const struct vkms_crtc_state > *crtc_state, struct line_buff > } > } > > -static void apply_colorop(struct pixel_argb_u16 *pixel, struct drm_colorop > *colorop) > +static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop > *colorop) > { > /* TODO is this right? */ > struct drm_colorop_state *colorop_state = colorop->state; > @@ -191,25 +191,56 @@ static void apply_colorop(struct pixel_argb_u16 *pixel, > struct drm_colorop *colo > > static void pre_blend_color_transform(const struct vkms_plane_state > *plane_state, struct line_buffer *output_buffer) > { > - struct drm_colorop *colorop = plane_state->base.base.color_pipeline; > + struct drm_colorop *colorop; > + struct pixel_argb_s32 pixel; > > - while (colorop) { > - struct drm_colorop_state *colorop_state; > + for (size_t x = 0; x < output_buffer->n_pixels; x++) { > > - if (!colorop) > - return; > + /* > + * Some operations, such as applying a BT709 encoding matrix, > + * followed by a decoding matrix, require that we preserve > + * values above 1.0 and below 0.0 until the end of the pipeline. > + * > + * Pack the 16-bit UNORM values into s32 to give us head-room to > + * avoid clipping until we're at the end of the pipeline. Clip > + * intentionally at the end of the pipeline before packing > + * UNORM values back into u16. > + */ > + pixel.a = output_buffer->pixels[x].a; > + pixel.r = output_buffer->pixels[x].r; > + pixel.g = output_buffer->pixels[x].g; > + pixel.b = output_buffer->pixels[x].b; > > - /* TODO this is probably wrong */ > - colorop_state = colorop->state; > + colorop = plane_state->base.base.color_pipeline; > + while (colorop) { > + struct drm_colorop_state *colorop_state; > > - if (!colorop_state) > - return; > + if (!colorop) > + return; > + > + /* TODO this is probably wrong */ > + colorop_state = colorop->state; > + > + if (!colorop_state) > + return; > > - for (size_t x = 0; x < output_buffer->n_pixels; x++) > if (!colorop_state->bypass) > - apply_colorop(&output_buffer->pixels[x], > colorop); > + apply_colorop(&pixel, colorop); > > - colorop = colorop->next; > + colorop = colorop->next; > + } > + > + /* clamp pixel */ > +
[PATCH 41/43] drm/tiny/st7735r: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by st7735r. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: David Lechner --- drivers/gpu/drm/tiny/st7735r.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c index 477eb36fbb70d..1676da00883d3 100644 --- a/drivers/gpu/drm/tiny/st7735r.c +++ b/drivers/gpu/drm/tiny/st7735r.c @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -241,7 +241,7 @@ static int st7735r_probe(struct spi_device *spi) spi_set_drvdata(spi, drm); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 39/43] drm/tiny/repaper: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by repaper. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: "Noralf Trønnes" --- drivers/gpu/drm/tiny/repaper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 8fd6758f5725f..1f78aa3d26bbd 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -26,7 +26,7 @@ #include #include #include -#include +#include #include #include #include @@ -1118,7 +1118,7 @@ static int repaper_probe(struct spi_device *spi) DRM_DEBUG_DRIVER("SPI speed: %uMHz\n", spi->max_speed_hz / 100); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 33/43] drm/tiny/ili9163: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by ili9163. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/tiny/ili9163.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c index bc4384d410fcc..86f9d88349015 100644 --- a/drivers/gpu/drm/tiny/ili9163.c +++ b/drivers/gpu/drm/tiny/ili9163.c @@ -9,7 +9,7 @@ #include #include -#include +#include #include #include #include @@ -185,7 +185,7 @@ static int ili9163_probe(struct spi_device *spi) if (ret) return ret; - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 42/43] drm/fbdev-generic: Convert to fbdev-ttm
Only TTM-based drivers use fbdev-generic. Rename it to fbdev-ttm and change the symbol infix from _generic_ to _ttm_. Link the source file into TTM helpers, so that it is only build if TTM-based drivers have been selected. Select DRM_TTM_HELPER for loongson. Signed-off-by: Thomas Zimmermann --- Documentation/gpu/drm-kms-helpers.rst | 2 +- drivers/gpu/drm/Makefile | 5 +- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 +- .../{drm_fbdev_generic.c => drm_fbdev_ttm.c} | 80 +-- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 +- drivers/gpu/drm/loongson/Kconfig | 1 + drivers/gpu/drm/loongson/lsdc_drv.c | 4 +- drivers/gpu/drm/nouveau/nouveau_drm.c | 6 +- drivers/gpu/drm/qxl/qxl_drv.c | 4 +- drivers/gpu/drm/tiny/bochs.c | 4 +- drivers/gpu/drm/vboxvideo/vbox_drv.c | 4 +- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +- include/drm/drm_fbdev_generic.h | 15 include/drm/drm_fbdev_ttm.h | 15 14 files changed, 77 insertions(+), 77 deletions(-) rename drivers/gpu/drm/{drm_fbdev_generic.c => drm_fbdev_ttm.c} (76%) delete mode 100644 include/drm/drm_fbdev_generic.h create mode 100644 include/drm/drm_fbdev_ttm.h diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 59cfe8a7a8bac..e46ab9b670acd 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -116,7 +116,7 @@ fbdev Helper Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c :export: -.. kernel-doc:: drivers/gpu/drm/drm_fbdev_generic.c +.. kernel-doc:: drivers/gpu/drm/drm_fbdev_ttm.c :export: format Helper Functions Reference diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 50a2f0cffdac2..7bd4c525fd825 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -117,6 +117,7 @@ drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o drm_ttm_helper-y := drm_gem_ttm_helper.o +drm_ttm_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_ttm.o obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o # @@ -142,9 +143,7 @@ drm_kms_helper-y := \ drm_self_refresh_helper.o \ drm_simple_kms_helper.o drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o -drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += \ - drm_fbdev_generic.o \ - drm_fb_helper.o +drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o # diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 6acffedf648c8..e13c03d29c162 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -24,7 +24,7 @@ #include #include -#include +#include #include #include #include @@ -2308,9 +2308,9 @@ static int amdgpu_pci_probe(struct pci_dev *pdev, !list_empty(&adev_to_drm(adev)->mode_config.connector_list)) { /* select 8 bpp console on low vram cards */ if (adev->gmc.real_vram_size <= (32*1024*1024)) - drm_fbdev_generic_setup(adev_to_drm(adev), 8); + drm_fbdev_ttm_setup(adev_to_drm(adev), 8); else - drm_fbdev_generic_setup(adev_to_drm(adev), 32); + drm_fbdev_ttm_setup(adev_to_drm(adev), 32); } ret = amdgpu_debugfs_init(adev); diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_ttm.c similarity index 76% rename from drivers/gpu/drm/drm_fbdev_generic.c rename to drivers/gpu/drm/drm_fbdev_ttm.c index b4659cd6285ab..2114a589bd7d3 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_ttm.c @@ -10,10 +10,10 @@ #include #include -#include +#include /* @user: 1=userspace, 0=fbcon */ -static int drm_fbdev_generic_fb_open(struct fb_info *info, int user) +static int drm_fbdev_ttm_fb_open(struct fb_info *info, int user) { struct drm_fb_helper *fb_helper = info->par; @@ -24,7 +24,7 @@ static int drm_fbdev_generic_fb_open(struct fb_info *info, int user) return 0; } -static int drm_fbdev_generic_fb_release(struct fb_info *info, int user) +static int drm_fbdev_ttm_fb_release(struct fb_info *info, int user) { struct drm_fb_helper *fb_helper = info->par; @@ -34,11 +34,11 @@ static int drm_fbdev_generic_fb_release(struct fb_info *info, int user) return 0; } -FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(drm_fbdev_generic, +FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(drm_fbdev_ttm, drm_fb_helper_damage_range, drm_fb_helper_damage_area); -static void drm_fbdev_generic_fb_destroy(struct fb_info *info) +static void drm_fbdev_ttm_fb_destroy(struct fb_info *info)
[PATCH 35/43] drm/tiny/ili9341: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by ili9341. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Kamlesh Gurudasani --- drivers/gpu/drm/tiny/ili9341.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c index 47b61c3bf1457..8bcada30af717 100644 --- a/drivers/gpu/drm/tiny/ili9341.c +++ b/drivers/gpu/drm/tiny/ili9341.c @@ -17,7 +17,7 @@ #include #include -#include +#include #include #include #include @@ -218,7 +218,7 @@ static int ili9341_probe(struct spi_device *spi) spi_set_drvdata(spi, drm); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 37/43] drm/tiny/mi0283qt: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by mi0283qt. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: "Noralf Trønnes" --- drivers/gpu/drm/tiny/mi0283qt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c index 01ff43c8ac3ff..e46581d74c257 100644 --- a/drivers/gpu/drm/tiny/mi0283qt.c +++ b/drivers/gpu/drm/tiny/mi0283qt.c @@ -15,7 +15,7 @@ #include #include -#include +#include #include #include #include @@ -226,7 +226,7 @@ static int mi0283qt_probe(struct spi_device *spi) spi_set_drvdata(spi, drm); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 32/43] drm/tiny/hx8357d: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by hx8357d. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/tiny/hx8357d.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c index cdc4486e059b5..2e631282edeb4 100644 --- a/drivers/gpu/drm/tiny/hx8357d.c +++ b/drivers/gpu/drm/tiny/hx8357d.c @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -256,7 +256,7 @@ static int hx8357d_probe(struct spi_device *spi) spi_set_drvdata(spi, drm); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 17/43] drm/tiny/simpledrm: Use fbdev-shmem
Implement fbdev emulation with fbdev-shmem. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Thomas Zimmermann Cc: Javier Martinez Canillas --- drivers/gpu/drm/tiny/simpledrm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 7ce1c46176750..3cb09ad5f7537 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -17,7 +17,7 @@ #include #include #include -#include +#include #include #include #include @@ -1026,7 +1026,7 @@ static int simpledrm_probe(struct platform_device *pdev) if (color_mode == 16) color_mode = sdev->format->depth; // can be 15 or 16 - drm_fbdev_generic_setup(dev, color_mode); + drm_fbdev_shmem_setup(dev, color_mode); return 0; } -- 2.44.0
[PATCH 25/43] drm/ingenic: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by ingenic. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Paul Cercueil --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 0751235007a7e..39fa291f43dd1 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -31,7 +31,7 @@ #include #include #include -#include +#include #include #include #include @@ -1399,7 +1399,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) goto err_clk_notifier_unregister; } - drm_fbdev_generic_setup(drm, 32); + drm_fbdev_dma_setup(drm, 32); return 0; -- 2.44.0
[PATCH 15/43] drm/tiny/gm12u320: Use fbdev-shmem
Implement fbdev emulation with fbdev-shmem. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Hans de Goede --- drivers/gpu/drm/tiny/gm12u320.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c index 0187539ff5eaa..8b4efd39d7c41 100644 --- a/drivers/gpu/drm/tiny/gm12u320.c +++ b/drivers/gpu/drm/tiny/gm12u320.c @@ -13,7 +13,7 @@ #include #include #include -#include +#include #include #include #include @@ -699,7 +699,7 @@ static int gm12u320_usb_probe(struct usb_interface *interface, if (ret) goto err_put_device; - drm_fbdev_generic_setup(dev, 0); + drm_fbdev_shmem_setup(dev, 0); return 0; -- 2.44.0
[PATCH 30/43] drm/renesas/shmobile: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by shmobile. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Laurent Pinchart Cc: Geert Uytterhoeven --- drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c index e83c3e52251de..890cc2f6408d6 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c @@ -19,7 +19,7 @@ #include #include -#include +#include #include #include #include @@ -250,7 +250,7 @@ static int shmob_drm_probe(struct platform_device *pdev) if (ret < 0) goto err_modeset_cleanup; - drm_fbdev_generic_setup(ddev, 16); + drm_fbdev_dma_setup(ddev, 16); return 0; -- 2.44.0
[PATCH 40/43] drm/tiny/st7586: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by st7586. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: David Lechner --- drivers/gpu/drm/tiny/st7586.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c index 7336fa1ddaed1..9e080d3b084c5 100644 --- a/drivers/gpu/drm/tiny/st7586.c +++ b/drivers/gpu/drm/tiny/st7586.c @@ -16,7 +16,7 @@ #include #include #include -#include +#include #include #include #include @@ -371,7 +371,7 @@ static int st7586_probe(struct spi_device *spi) spi_set_drvdata(spi, drm); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 21/43] drm/fbdev-dma: Implement damage handling and deferred I/O
Add support for damage handling and deferred I/O to fbdev-dma. This enables fbdev-dma to support all DMA-memory-based DRM drivers, even such with a dirty callback in their framebuffers. The patch adds the code for deferred I/O and also sets a dedicated helper for struct fb_ops.fb_mmap that support coherent mappings. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fbdev_dma.c | 65 ++--- 1 file changed, 51 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c index 6c9427bb4053b..8ffd072368bca 100644 --- a/drivers/gpu/drm/drm_fbdev_dma.c +++ b/drivers/gpu/drm/drm_fbdev_dma.c @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -35,6 +36,22 @@ static int drm_fbdev_dma_fb_release(struct fb_info *info, int user) return 0; } +FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(drm_fbdev_dma, + drm_fb_helper_damage_range, + drm_fb_helper_damage_area); + +static int drm_fbdev_dma_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) +{ + struct drm_fb_helper *fb_helper = info->par; + struct drm_framebuffer *fb = fb_helper->fb; + struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0); + + if (!dma->map_noncoherent) + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); + + return fb_deferred_io_mmap(info, vma); +} + static void drm_fbdev_dma_fb_destroy(struct fb_info *info) { struct drm_fb_helper *fb_helper = info->par; @@ -51,20 +68,13 @@ static void drm_fbdev_dma_fb_destroy(struct fb_info *info) kfree(fb_helper); } -static int drm_fbdev_dma_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) -{ - struct drm_fb_helper *fb_helper = info->par; - - return drm_gem_prime_mmap(fb_helper->buffer->gem, vma); -} - static const struct fb_ops drm_fbdev_dma_fb_ops = { .owner = THIS_MODULE, .fb_open = drm_fbdev_dma_fb_open, .fb_release = drm_fbdev_dma_fb_release, - __FB_DEFAULT_DMAMEM_OPS_RDWR, + __FB_DEFAULT_DEFERRED_OPS_RDWR(drm_fbdev_dma), DRM_FB_HELPER_DEFAULT_OPS, - __FB_DEFAULT_DMAMEM_OPS_DRAW, + __FB_DEFAULT_DEFERRED_OPS_DRAW(drm_fbdev_dma), .fb_mmap = drm_fbdev_dma_fb_mmap, .fb_destroy = drm_fbdev_dma_fb_destroy, }; @@ -98,10 +108,6 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, dma_obj = to_drm_gem_dma_obj(buffer->gem); fb = buffer->fb; - if (drm_WARN_ON(dev, fb->funcs->dirty)) { - ret = -ENODEV; /* damage handling not supported; use generic emulation */ - goto err_drm_client_buffer_delete; - } ret = drm_client_buffer_vmap(buffer, &map); if (ret) { @@ -112,7 +118,7 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, } fb_helper->buffer = buffer; - fb_helper->fb = buffer->fb; + fb_helper->fb = fb; info = drm_fb_helper_alloc_info(fb_helper); if (IS_ERR(info)) { @@ -133,8 +139,19 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); info->fix.smem_len = info->screen_size; + /* deferred I/O */ + fb_helper->fbdefio.delay = HZ / 20; + fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io; + + info->fbdefio = &fb_helper->fbdefio; + ret = fb_deferred_io_init(info); + if (ret) + goto err_drm_fb_helper_release_info; + return 0; +err_drm_fb_helper_release_info: + drm_fb_helper_release_info(fb_helper); err_drm_client_buffer_vunmap: fb_helper->fb = NULL; fb_helper->buffer = NULL; @@ -144,8 +161,28 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, return ret; } +static int drm_fbdev_dma_helper_fb_dirty(struct drm_fb_helper *helper, +struct drm_clip_rect *clip) +{ + struct drm_device *dev = helper->dev; + int ret; + + /* Call damage handlers only if necessary */ + if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2)) + return 0; + + if (helper->fb->funcs->dirty) { + ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1); + if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret)) + return ret; + } + + return 0; +} + static const struct drm_fb_helper_funcs drm_fbdev_dma_helper_funcs = { .fb_probe = drm_fbdev_dma_helper_fb_probe, + .fb_dirty = drm_fbdev_dma_helper_fb_dirty, }; /* -- 2.44.0
[PATCH 24/43] drm/imx/lcdc: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by lcdc. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Shawn Guo Cc: Sascha Hauer Cc: Pengutronix Kernel Team Cc: Fabio Estevam Cc: NXP Linux Team --- drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c index 43ddf3a9810b6..36668455aee8c 100644 --- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c @@ -5,7 +5,7 @@ #include #include #include -#include +#include #include #include #include @@ -501,7 +501,7 @@ static int imx_lcdc_probe(struct platform_device *pdev) if (ret) return dev_err_probe(dev, ret, "Cannot register device\n"); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 31/43] drm/rockchip: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by rockchip. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Sandy Huang Cc: "Heiko Stübner" Cc: Andy Yan --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index ab55d71325500..44d769d9234d6 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -191,7 +191,7 @@ static int rockchip_drm_bind(struct device *dev) if (ret) goto err_kms_helper_poll_fini; - drm_fbdev_generic_setup(drm_dev, 0); + drm_fbdev_dma_setup(drm_dev, 0); return 0; err_kms_helper_poll_fini: -- 2.44.0
[PATCH 28/43] drm/renesas/rcar-du: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by rcar-du. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Laurent Pinchart Cc: Kieran Bingham --- drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c index dee530e4c8b27..fb719d9aff10d 100644 --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c @@ -20,7 +20,7 @@ #include #include -#include +#include #include #include #include @@ -716,7 +716,7 @@ static int rcar_du_probe(struct platform_device *pdev) drm_info(&rcdu->ddev, "Device %s probed\n", dev_name(&pdev->dev)); - drm_fbdev_generic_setup(&rcdu->ddev, 32); + drm_fbdev_dma_setup(&rcdu->ddev, 32); return 0; -- 2.44.0
[PATCH 38/43] drm/tiny/panel-mipi-dbi: Use fbdev-dma
Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports damage handling, which is required by panel-mipi-dbi. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: "Noralf Trønnes" --- drivers/gpu/drm/tiny/panel-mipi-dbi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c index f80a141fcf365..4cc4ca787c7d1 100644 --- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c @@ -16,7 +16,7 @@ #include #include -#include +#include #include #include #include @@ -335,7 +335,7 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi) spi_set_drvdata(spi, drm); - drm_fbdev_generic_setup(drm, 0); + drm_fbdev_dma_setup(drm, 0); return 0; } -- 2.44.0
[PATCH 20/43] drm/vkms: Use fbdev-shmem
Implement fbdev emulation with fbdev-shmem. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Rodrigo Siqueira Cc: Melissa Wen Cc: "Maíra Canal" Cc: Haneen Mohammed Cc: Daniel Vetter --- drivers/gpu/drm/vkms/vkms_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index dd0af086e7fa9..8dc9dc13896e9 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -17,7 +17,7 @@ #include #include #include -#include +#include #include #include #include @@ -223,7 +223,7 @@ static int vkms_create(struct vkms_config *config) if (ret) goto out_devres; - drm_fbdev_generic_setup(&vkms_device->drm, 0); + drm_fbdev_shmem_setup(&vkms_device->drm, 0); return 0; -- 2.44.0
[PATCH 43/43] drm/fbdev: Clean up fbdev documentation
Rewrite some docs that are not up-to-date any longer. Remove the TODO item for fbdev-generic conversion, as the helper has been replaced. Make documentation for DMA, SHMEM and TTM emulation available. Signed-off-by: Thomas Zimmermann Cc: Jonathan Corbet --- Documentation/gpu/drm-kms-helpers.rst | 12 +--- Documentation/gpu/todo.rst| 13 - drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 11 ++- include/drm/drm_mode_config.h | 4 ++-- 5 files changed, 14 insertions(+), 28 deletions(-) diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index e46ab9b670acd..8435e8621cc08 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -110,15 +110,21 @@ fbdev Helper Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c :doc: fbdev helpers -.. kernel-doc:: include/drm/drm_fb_helper.h - :internal: +.. kernel-doc:: drivers/gpu/drm/drm_fbdev_dma.c + :export: -.. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c +.. kernel-doc:: drivers/gpu/drm/drm_fbdev_shmem.c :export: .. kernel-doc:: drivers/gpu/drm/drm_fbdev_ttm.c :export: +.. kernel-doc:: include/drm/drm_fb_helper.h + :internal: + +.. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c + :export: + format Helper Functions Reference = diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index fb9ad120b1414..e2a0585915b32 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -243,19 +243,6 @@ Contact: Maintainer of the driver you plan to convert Level: Intermediate -Convert drivers to use drm_fbdev_generic_setup() - - -Most drivers can use drm_fbdev_generic_setup(). Driver have to implement -atomic modesetting and GEM vmap support. Historically, generic fbdev emulation -expected the framebuffer in system memory or system-like memory. By employing -struct iosys_map, drivers with frambuffers in I/O memory can be supported -as well. - -Contact: Maintainer of the driver you plan to convert - -Level: Intermediate - Reimplement functions in drm_fbdev_fb_ops without fbdev --- diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 243cacb3575c0..cfcd45480d326 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -345,7 +345,7 @@ void drm_minor_release(struct drm_minor *minor) * if (ret) * return ret; * - * drm_fbdev_generic_setup(drm, 32); + * drm_fbdev_{...}_setup(drm, 32); * * return 0; * } diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index d612133e2cf7e..e2e19f49342e1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -85,12 +85,8 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); * The fb helper functions are useful to provide an fbdev on top of a drm kernel * mode setting driver. They can be used mostly independently from the crtc * helper functions used by many drivers to implement the kernel mode setting - * interfaces. - * - * Drivers that support a dumb buffer with a virtual address and mmap support, - * should try out the generic fbdev emulation using drm_fbdev_generic_setup(). - * It will automatically set up deferred I/O if the driver requires a shadow - * buffer. + * interfaces. Drivers that use one of the shared memory managers, TTM, SHMEM, + * DMA, should instead use the corresponding fbdev emulation. * * Existing fbdev implementations should restore the fbdev console by using * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback. @@ -126,9 +122,6 @@ static DEFINE_MUTEX(kernel_fb_helper_lock); * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io * callback it will also schedule dirty_work with the damage collected from the * mmap page writes. - * - * Deferred I/O is not compatible with SHMEM. Such drivers should request an - * fbdev shadow buffer and call drm_fbdev_generic_setup() instead. */ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 973119a9176b2..1e4b8d01212e6 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -106,8 +106,8 @@ struct drm_mode_config_funcs { * Drivers implementing fbdev emulation use drm_kms_helper_hotplug_event() * to call this hook to inform the fbdev helper of output changes. * -* This hook is deprecated, drivers should instead use -* drm_fbdev_generic_setup() which takes care of any necessary +* This hook is deprecated, drivers should instead implement fbdev +* support with struct drm_client, which takes care of any neces
[PATCH 11/43] drm/hyperv: Use fbdev-shmem
Implement fbdev emulation with fbdev-shmem. Avoids the overhead of fbdev-generic's additional shadow buffering. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Deepak Rawat --- drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c index cff85086f2d66..ff93e08d5036d 100644 --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include @@ -149,7 +149,7 @@ static int hyperv_vmbus_probe(struct hv_device *hdev, goto err_free_mmio; } - drm_fbdev_generic_setup(dev, 0); + drm_fbdev_shmem_setup(dev, 0); return 0; -- 2.44.0
[PATCH 07/43] fbdev/deferred-io: Provide get_page hook in struct fb_deferred_io
Add a callback for drivers to provide framebuffer pages to fbdev's deferred-I/O helpers. Implementations need to acquire a reference on the page before returning it. Returning NULL generates a SIGBUS signal. This will be useful for DRM's fbdev emulation with GEM-shemem buffer objects. Signed-off-by: Thomas Zimmermann --- drivers/video/fbdev/core/fb_defio.c | 4 include/linux/fb.h | 1 + 2 files changed, 5 insertions(+) diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c index 6e0bbcfdb01b5..51928ff7961a5 100644 --- a/drivers/video/fbdev/core/fb_defio.c +++ b/drivers/video/fbdev/core/fb_defio.c @@ -25,9 +25,13 @@ static struct page *fb_deferred_io_get_page(struct fb_info *info, unsigned long offs) { + struct fb_deferred_io *fbdefio = info->fbdefio; const void *screen_buffer = info->screen_buffer; struct page *page = NULL; + if (fbdefio->get_page) + return fbdefio->get_page(info, offs); + if (is_vmalloc_addr(screen_buffer + offs)) page = vmalloc_to_page(screen_buffer + offs); else if (info->fix.smem_start) diff --git a/include/linux/fb.h b/include/linux/fb.h index 708e6a177b1be..bbb0805c0ab1e 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -219,6 +219,7 @@ struct fb_deferred_io { struct mutex lock; /* mutex that protects the pageref list */ struct list_head pagereflist; /* list of pagerefs for touched pages */ /* callback */ + struct page *(*get_page)(struct fb_info *info, unsigned long offset); void (*deferred_io)(struct fb_info *info, struct list_head *pagelist); }; #endif -- 2.44.0