Re: [PATCH v6 6/7] phy: phy-mtk-dp: Add driver for DP phy
On Wed, 2021-11-10 at 14:06 +0100, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann > > This is a new driver that supports the integrated DisplayPort phy for > mediatek SoCs, especially the mt8195. The phy is integrated into the > DisplayPort controller and will be created by the mtk-dp driver. This > driver expects a struct regmap to be able to work on the same > registers > as the DisplayPort controller. It sets the device data to be the > struct > phy so that the DisplayPort controller can easily work with it. > > The driver does not have any devicetree bindings because the > datasheet > does not list the controller and the phy as distinct units. > > The interaction with the controller can be covered by the configure > callback of the phy framework and its displayport parameters. > > Signed-off-by: Markus Schneider-Pargmann > Signed-off-by: Guillaume Ranquet > --- > MAINTAINERS | 1 + > drivers/phy/mediatek/Kconfig | 8 ++ > drivers/phy/mediatek/Makefile | 1 + > drivers/phy/mediatek/phy-mtk-dp.c | 219 > ++ > 4 files changed, 229 insertions(+) > create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 170bbbeefc3f4..f9a71b6d2a4a9 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6367,6 +6367,7 @@ L: linux-media...@lists.infradead.org > (moderated for non-subscribers) > S: Supported > F: Documentation/devicetree/bindings/display/mediatek/ > F: drivers/gpu/drm/mediatek/ > +F: drivers/phy/mediatek/phy-mtk-dp.c > F: drivers/phy/mediatek/phy-mtk-hdmi* > F: drivers/phy/mediatek/phy-mtk-mipi* > > diff --git a/drivers/phy/mediatek/Kconfig > b/drivers/phy/mediatek/Kconfig > index 55f8e6c048ab3..f7ec860590492 100644 > --- a/drivers/phy/mediatek/Kconfig > +++ b/drivers/phy/mediatek/Kconfig > @@ -55,3 +55,11 @@ config PHY_MTK_MIPI_DSI > select GENERIC_PHY > help > Support MIPI DSI for Mediatek SoCs. > + > +config PHY_MTK_DP > + tristate "MediaTek DP-PHY Driver" > + depends on ARCH_MEDIATEK || COMPILE_TEST > + depends on OF > + select GENERIC_PHY > + help > + Support DisplayPort PHY for Mediatek SoCs. > diff --git a/drivers/phy/mediatek/Makefile > b/drivers/phy/mediatek/Makefile > index ace660fbed3a1..4ba1e06504346 100644 > --- a/drivers/phy/mediatek/Makefile > +++ b/drivers/phy/mediatek/Makefile > @@ -3,6 +3,7 @@ > # Makefile for the phy drivers. > # > > +obj-$(CONFIG_PHY_MTK_DP) += phy-mtk-dp.o > obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o > obj-$(CONFIG_PHY_MTK_UFS)+= phy-mtk-ufs.o > obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > new file mode 100644 > index 0..4f8d26ec0346b > --- /dev/null > +++ b/drivers/phy/mediatek/phy-mtk-dp.c > @@ -0,0 +1,219 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2021 BayLibre > + * Author: Markus Schneider-Pargmann > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PHY_OFFSET 0x1000 Why not provide register base address in DTS? I find that phy's platform device is registered by dp driver but the one automatically created when parse dts? > + > +#define MTK_DP_PHY_DIG_PLL_CTL_1 (PHY_OFFSET + 0x014) > +# define TPLL_SSC_EN BIT(3) > + > +#define MTK_DP_PHY_DIG_BIT_RATE (PHY_OFFSET + > 0x03C) > +# define BIT_RATE_RBR0 > +# define BIT_RATE_HBR1 > +# define BIT_RATE_HBR2 2 > +# define BIT_RATE_HBR3 3 > + > +#define MTK_DP_PHY_DIG_SW_RST(PHY_OFFSET + > 0x038) > +# define DP_GLB_SW_RST_PHYD BIT(0) > + > +#define MTK_DP_LANE0_DRIVING_PARAM_3 (PHY_OFFSET + 0x138) > +#define MTK_DP_LANE1_DRIVING_PARAM_3 (PHY_OFFSET + 0x238) > +#define MTK_DP_LANE2_DRIVING_PARAM_3 (PHY_OFFSET + 0x338) > +#define MTK_DP_LANE3_DRIVING_PARAM_3 (PHY_OFFSET + 0x438) > +# define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT 0x10 > +# define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT (0x14 << 8) > +# define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT (0x18 << 16) > +# define XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT (0x20 << 24) > +# define DRIVING_PARAM_3_DEFAULT (XTP_LN_TX_LCTXC0_SW0_P > RE0_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_P > RE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_P > RE2_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_P > RE3_DEFAULT) > + > +#define MTK_DP_LANE0_DRIVING_PARAM_4 (PHY_OFFSET + 0x13C) > +#define MTK_DP_LANE1_DRIVING_PARAM_4 (PHY_OFFSET + 0x23C) > +#define MTK_DP_LANE2_DRIVING_PARAM_4 (PHY_OF
[PATCH] dma-buf: add DMA_BUF_IOCTL_SYNC_PARTIAL support
Add DMA_BUF_IOCTL_SYNC_PARTIAL support for user to sync dma-buf with
offset and len.
Change-Id: I03d2d2e10e48d32aa83c31abade57e0931e1be49
Signed-off-by: Jianqun Xu
---
drivers/dma-buf/dma-buf.c| 42
include/uapi/linux/dma-buf.h | 8 +++
2 files changed, 50 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d9948d58b3f4..78f37f7c3462 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -392,6 +392,7 @@ static long dma_buf_ioctl(struct file *file,
{
struct dma_buf *dmabuf;
struct dma_buf_sync sync;
+ struct dma_buf_sync_partial sync_p;
enum dma_data_direction direction;
int ret;
@@ -430,6 +431,47 @@ static long dma_buf_ioctl(struct file *file,
case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
+ case DMA_BUF_IOCTL_SYNC_PARTIAL:
+ if (copy_from_user(&sync_p, (void __user *) arg,
sizeof(sync_p)))
+ return -EFAULT;
+
+ if (sync_p.len == 0)
+ return 0;
+
+ if ((sync_p.offset % cache_line_size()) || (sync_p.len %
cache_line_size()))
+ return -EINVAL;
+
+ if (sync_p.len > dmabuf->size || sync_p.offset > dmabuf->size -
sync_p.len)
+ return -EINVAL;
+
+ if (sync_p.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
+ return -EINVAL;
+
+ switch (sync_p.flags & DMA_BUF_SYNC_RW) {
+ case DMA_BUF_SYNC_READ:
+ direction = DMA_FROM_DEVICE;
+ break;
+ case DMA_BUF_SYNC_WRITE:
+ direction = DMA_TO_DEVICE;
+ break;
+ case DMA_BUF_SYNC_RW:
+ direction = DMA_BIDIRECTIONAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (sync_p.flags & DMA_BUF_SYNC_END)
+ ret = dma_buf_end_cpu_access_partial(dmabuf, direction,
+sync_p.offset,
+sync_p.len);
+ else
+ ret = dma_buf_begin_cpu_access_partial(dmabuf,
direction,
+ sync_p.offset,
+ sync_p.len);
+
+ return ret;
+
default:
return -ENOTTY;
}
diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
index 7f30393b92c3..6236c644624d 100644
--- a/include/uapi/linux/dma-buf.h
+++ b/include/uapi/linux/dma-buf.h
@@ -47,4 +47,12 @@ struct dma_buf_sync {
#define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32)
#define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64)
+struct dma_buf_sync_partial {
+ __u64 flags;
+ __u32 offset;
+ __u32 len;
+};
+
+#define DMA_BUF_IOCTL_SYNC_PARTIAL _IOW(DMA_BUF_BASE, 2, struct
dma_buf_sync_partial)
+
#endif
--
2.25.1
Re: [PATCH] drm/amd/display: fix cond_no_effect.cocci warnings
Applied. Thanks!
Alex
On Fri, Nov 12, 2021 at 1:17 AM wrote:
>
> From: Ye Guojin
>
> This was found by coccicheck:
> ./drivers/gpu/drm/amd/display/dc/core/dc_resource.c, 2516, 7-9, WARNING
> possible condition with no effect (if == else)
>
> hdmi_info.bits.YQ0_YQ1 is always YYC_QUANTIZATION_LIMITED_RANGE.
>
> Reported-by: Zeal Robot
> Signed-off-by: Ye Guojin
> ---
> drivers/gpu/drm/amd/display/dc/core/dc_resource.c | 12 +---
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> index fabe1b83bd4f..564163a85d2c 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> @@ -2509,17 +2509,7 @@ static void set_avi_info_frame(
>
> /* TODO : We should handle YCC quantization */
> /* but we do not have matrix calculation */
> - if (stream->qy_bit == 1) {
> - if (color_space == COLOR_SPACE_SRGB ||
> - color_space == COLOR_SPACE_2020_RGB_FULLRANGE)
> - hdmi_info.bits.YQ0_YQ1 =
> YYC_QUANTIZATION_LIMITED_RANGE;
> - else if (color_space == COLOR_SPACE_SRGB_LIMITED ||
> - color_space ==
> COLOR_SPACE_2020_RGB_LIMITEDRANGE)
> - hdmi_info.bits.YQ0_YQ1 =
> YYC_QUANTIZATION_LIMITED_RANGE;
> - else
> - hdmi_info.bits.YQ0_YQ1 =
> YYC_QUANTIZATION_LIMITED_RANGE;
> - } else
> - hdmi_info.bits.YQ0_YQ1 = YYC_QUANTIZATION_LIMITED_RANGE;
> + hdmi_info.bits.YQ0_YQ1 = YYC_QUANTIZATION_LIMITED_RANGE;
>
> ///VIC
> format = stream->timing.timing_3d_format;
> --
> 2.25.1
>
Re: [PATCH v2] drm/msm/dp: employ bridge mechanism for display enable and disable
On 11/6/2021 9:25 AM, Bjorn Andersson wrote:
On Fri 05 Nov 16:22 CDT 2021, Kuogee Hsieh wrote:
Currently the msm_dp_*** functions implement the same sequence which would
happen when drm_bridge is used. hence get rid of this intermediate layer
and align with the drm_bridge usage to avoid customized implementation.
Signed-off-by: Kuogee Hsieh
Changes in v2:
-- revise commit text
-- rename dp_bridge to msm_dp_bridge
-- delete empty functions
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 7 +++
drivers/gpu/drm/msm/dp/dp_display.c | 18 +++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 91 +
drivers/gpu/drm/msm/msm_drv.h | 16 +++--
6 files changed, 120 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 31050aa..c4e08c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1003,9 +1003,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
- msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
-
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
conn = conn_iter;
@@ -1181,14 +1178,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
- if (ret) {
- DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
- ret);
- goto out;
- }
- }
dpu_enc->enabled = true;
out:
@@ -1214,11 +1203,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
- DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
- }
-
dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_PRE_STOP);
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
@@ -1243,11 +1227,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
- DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
- }
-
mutex_unlock(&dpu_enc->enc_lock);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 27d98b5..d16337f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -557,6 +557,13 @@ static int _dpu_kms_initialize_displayport(struct
drm_device *dev,
encoder->base.id, rc);
return rc;
}
+
+ rc = msm_dp_bridge_init(priv->dp[i], dev, encoder);
+ if (rc) {
+ DPU_ERROR("failed to setup DPU bridge %d: rc:%d\n",
+ encoder->base.id, rc);
+ return rc;
+ }
}
return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index e41dd40..e9ea6ed 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -569,8 +569,8 @@ static int dp_hpd_plug_handle(struct dp_display_private
*dp, u32 data)
return 0;
};
-static int dp_display_enable(struct dp_display_private *dp, u32 data);
-static int dp_display_disable(struct dp_display_private *dp, u32 data);
+static int __dp_display_enable(struct dp_display_private *dp, u32 data);
+static int __dp_display_disable(struct dp_display_private *dp, u32 data);
Can you please help me understand why you're changing the name of these
functions?
my mistake, will remove this change
static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
{
@@ -855,7 +855,7 @@ static int dp_display_prepare(struct msm_dp *dp_display)
return 0;
}
-static int dp_display_enable(struct dp_display_private *dp, u32 data)
+static int __dp_display_enable(struct dp_display_private *dp, u32 data)
{
int rc = 0;
@@ -898,7 +898,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
return 0;
}
-static int dp_display_disable(struct dp_display_private *dp, u32 data)
Re: [PATCH] drm/amd/display: Clean up some inconsistent indenting
Applied. Thanks! Alex On Thu, Nov 11, 2021 at 5:09 AM Christian König wrote: > > > > Am 11.11.21 um 11:03 schrieb Jiapeng Chong: > > Eliminate the follow smatch warning: > > > > drivers/gpu/drm/amd/amdgpu/../display/dmub/src/dmub_srv.c:622 > > dmub_srv_cmd_execute() warn: inconsistent indenting. > > > > Reported-by: Abaci Robot > > Signed-off-by: Jiapeng Chong > > Reviewed-by: Christian König > > > --- > > drivers/gpu/drm/amd/display/dmub/src/dmub_srv.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/display/dmub/src/dmub_srv.c > > b/drivers/gpu/drm/amd/display/dmub/src/dmub_srv.c > > index 56a0332..e9fadf1 100644 > > --- a/drivers/gpu/drm/amd/display/dmub/src/dmub_srv.c > > +++ b/drivers/gpu/drm/amd/display/dmub/src/dmub_srv.c > > @@ -618,8 +618,8 @@ enum dmub_status dmub_srv_cmd_execute(struct dmub_srv > > *dmub) > >* read back stale, fully invalid or partially invalid data. > >*/ > > dmub_rb_flush_pending(&dmub->inbox1_rb); > > + dmub->hw_funcs.set_inbox1_wptr(dmub, dmub->inbox1_rb.wrpt); > > > > - dmub->hw_funcs.set_inbox1_wptr(dmub, dmub->inbox1_rb.wrpt); > > return DMUB_STATUS_OK; > > } > > >
Re: [PATCH] drm/amdgpu: fix set scaling mode Full/Full aspect/Center not works on vga and dvi connectors
Applied. Thanks! On Wed, Nov 10, 2021 at 10:54 PM hongao wrote: > > amdgpu_connector_vga_get_modes missed function amdgpu_get_native_mode > which assign amdgpu_encoder->native_mode with *preferred_mode result in > amdgpu_encoder->native_mode.clock always be 0. That will cause > amdgpu_connector_set_property returned early on: > if ((rmx_type != DRM_MODE_SCALE_NONE) && > (amdgpu_encoder->native_mode.clock == 0)) > when we try to set scaling mode Full/Full aspect/Center. > Add the missing function to amdgpu_connector_vga_get_mode can fix this. > It also works on dvi connectors because > amdgpu_connector_dvi_helper_funcs.get_mode use the same method. > > Signed-off-by: hongao > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > index b9c11c2b2885..0de66f59adb8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > @@ -827,6 +827,7 @@ static int amdgpu_connector_vga_get_modes(struct > drm_connector *connector) > > amdgpu_connector_get_edid(connector); > ret = amdgpu_connector_ddc_get_modes(connector); > + amdgpu_get_native_mode(connector); > > return ret; > } > -- > 2.20.1 > > >
Re: [PATCH] MAINTAINERS: update information for nouveau
Acked-by: Lyude Paul On Wed, 2021-11-10 at 14:31 +0100, Karol Herbst wrote: > Some side notes on this. Atm we do want to use gitlab for bug tracking and > merge requests. But due to the nature of the current linux kernel > development, we can only do so for nouveau internal changes. > > Everything else still needs to be sent as emails and this is also includes > changes to UAPI etc. > > Anyway, if somebody wants to submit patches via gitlab, they are free to > do so and this should just make this more official and documented. > > People listed as maintainers are such that have push access to drm-misc > (where changes are pushed to after landing in gitlab) and are known > nouveau developers. > We did this already for some trivial changes and critical bug fixes > already, we just weren't thinking about updating the MAINTAINERS file. > > Cc: Ben Skeggs > Cc: Lyude Paul > Cc: David Airlie > Cc: Daniel Vetter > Cc: dri-devel@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Signed-off-by: Karol Herbst > --- > MAINTAINERS | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 8805df335326..270dc9c0a427 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5961,10 +5961,17 @@ F: drivers/gpu/drm/panel/panel-novatek- > nt36672a.c > > DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS > M: Ben Skeggs > +M: Karol Herbst > +M: Lyude Paul > L: dri-devel@lists.freedesktop.org > L: nouv...@lists.freedesktop.org > S: Supported > -T: git git://github.com/skeggsb/linux > +W: https://nouveau.freedesktop.org/ > +Q: https://patchwork.freedesktop.org/project/nouveau/ > +Q: https://gitlab.freedesktop.org/drm/nouveau/-/merge_requests > +B: https://gitlab.freedesktop.org/drm/nouveau/-/issues > +C: irc://irc.oftc.net/nouveau > +T: git https://gitlab.freedesktop.org/drm/nouveau.git > F: drivers/gpu/drm/nouveau/ > F: include/uapi/drm/nouveau_drm.h > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [PATCH v4 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Thu, Nov 04, 2021 at 11:04:29PM -0400, Sean Paul wrote: > From: Sean Paul > > This patch adds the bindings for the MSM DisplayPort HDCP registers > which are required to write the HDCP key into the display controller as > well as the registers to enable HDCP authentication/key > exchange/encryption. > > We'll use a new compatible string for this since the fields are optional. > > Cc: Rob Herring > Cc: Stephen Boyd > Signed-off-by: Sean Paul > Link: > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run > #v1 > Link: > https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run > #v2 > Link: > https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-13-s...@poorly.run > #v3 > > Changes in v2: > -Drop register range names (Stephen) > -Fix yaml errors (Rob) > Changes in v3: > -Add new compatible string for dp-hdcp > -Add descriptions to reg > -Add minItems/maxItems to reg > -Make reg depend on the new hdcp compatible string > Changes in v4: > -Rebase on Bjorn's multi-dp patchset > --- > .../devicetree/bindings/display/msm/dp-controller.yaml| 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > index b36d74c1da7c..f6e4b102373a 100644 > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > @@ -21,12 +21,16 @@ properties: >- qcom,sc8180x-edp > >reg: > +minItems: 5 > +maxItems: 7 This should be a warning. Not sure why the bot didn't run. You just need 'minItems: 5' > items: >- description: ahb register block >- description: aux register block >- description: link register block >- description: p0 register block >- description: p1 register block > + - description: (Optional) Registers for HDCP device key injection > + - description: (Optional) Registers for HDCP TrustZone interaction > >interrupts: > maxItems: 1 > @@ -111,7 +115,9 @@ examples: ><0xae90200 0x200>, ><0xae90400 0xc00>, ><0xae91000 0x400>, > - <0xae91400 0x400>; > + <0xae91400 0x400>, > + <0x0aed1000 0x174>, > + <0x0aee1000 0x2c>; Be consistent and drop the leading 0. > interrupt-parent = <&mdss>; > interrupts = <12>; > clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, > -- > Sean Paul, Software Engineer, Google / Chromium OS > >
[PATCH] drm/i915/dp: Perform 30ms delay after source OUI write
While working on supporting the Intel HDR backlight interface, I noticed
that there's a couple of laptops that will very rarely manage to boot up
without detecting Intel HDR backlight support - even though it's supported
on the system. One example of such a laptop is the Lenovo P17 1st
generation.
Following some investigation Ville Syrjälä did through the docs they have
available to them, they discovered that there's actually supposed to be a
30ms wait after writing the source OUI before we begin setting up the rest
of the backlight interface.
This seems to be correct, as adding this 30ms delay seems to have
completely fixed the probing issues I was previously seeing. So - let's
start performing a 30ms wait after writing the OUI, which we do in a manner
similar to how we keep track of PPS delays (e.g. record the timestamp of
the OUI write, and then wait for however many ms are left since that
timestamp right before we interact with the backlight) in order to avoid
waiting any longer then we need to. As well, this also avoids us performing
this delay on systems where we don't end up using the HDR backlight
interface.
Signed-off-by: Lyude Paul
Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight interface (only
SDR for now)")
Cc: Ville Syrjälä
Cc: # v5.12+
---
drivers/gpu/drm/i915/display/intel_display_types.h| 3 +++
drivers/gpu/drm/i915/display/intel_dp.c | 3 +++
drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 11 +++
3 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
b/drivers/gpu/drm/i915/display/intel_display_types.h
index ea1e8a6e10b0..b9c967837872 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1653,6 +1653,9 @@ struct intel_dp {
struct intel_dp_pcon_frl frl;
struct intel_psr psr;
+
+ /* When we last wrote the OUI for eDP */
+ unsigned long last_oui_write;
};
enum lspcon_vendor {
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 0a424bf69396..77d9a9390c1e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2010,6 +2011,8 @@ intel_edp_init_source_oui(struct intel_dp *intel_dp, bool
careful)
if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui))
< 0)
drm_err(&i915->drm, "Failed to write source OUI\n");
+
+ intel_dp->last_oui_write = jiffies;
}
/* If the device supports it, try to set the power state appropriately */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 569d17b4d00f..2c35b999ec2c 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -96,6 +96,13 @@
#define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_10x359
/* Intel EDP backlight callbacks */
+static void
+wait_for_oui(struct drm_i915_private *i915, struct intel_dp *intel_dp)
+{
+ drm_dbg_kms(&i915->drm, "Performing OUI wait\n");
+ wait_remaining_ms_from_jiffies(intel_dp->last_oui_write, 30);
+}
+
static bool
intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
{
@@ -106,6 +113,8 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector
*connector)
int ret;
u8 tcon_cap[4];
+ wait_for_oui(i915, intel_dp);
+
ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap,
sizeof(tcon_cap));
if (ret != sizeof(tcon_cap))
return false;
@@ -204,6 +213,8 @@ intel_dp_aux_hdr_enable_backlight(const struct
intel_crtc_state *crtc_state,
int ret;
u8 old_ctrl, ctrl;
+ wait_for_oui(i915, intel_dp);
+
ret = drm_dp_dpcd_readb(&intel_dp->aux,
INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
if (ret != 1) {
drm_err(&i915->drm, "Failed to read current backlight control
mode: %d\n", ret);
--
2.31.1
Re: [RESEND PATCH v2 04/13] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On 2021-11-12 13:35:03, Marijn Suijten wrote:
> On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:
> > > When not specifying num-strings in the DT the default is used, but +1 is
> > > added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
> > > of 3 and 4 respectively, causing out-of-bounds reads and register
> > > read/writes. This +1 exists for a deficiency in the DT parsing code,
> > > and is simply omitted entirely - solving this oob issue - by parsing the
> > > property separately much like qcom,enabled-strings.
> > >
> > > This also allows more stringent checks on the maximum value when
> > > qcom,enabled-strings is provided in the DT. Note that num-strings is
> > > parsed after enabled-strings to give it final sign-off over the length,
> > > which DT currently utilizes to get around an incorrect fixed read of
> > > four elements from that array (has been addressed in a prior patch).
> > >
> > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > Signed-off-by: Marijn Suijten
> > > Reviewed-By: AngeloGioacchino Del Regno
> > >
> > > ---
> > > drivers/video/backlight/qcom-wled.c | 51 +++--
> > > 1 file changed, 19 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > b/drivers/video/backlight/qcom-wled.c
> > > index 977cd75827d7..c5232478a343 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> > > }
> > > }
> > >
> > > + rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> > > + if (!rc) {
> > > + if (val < 1 || val > wled->max_string_count) {
> > > + dev_err(dev, "qcom,num-strings must be between 1 and
> > > %d\n",
> > > + wled->max_string_count);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (string_len > 0) {
> > > + dev_warn(dev, "qcom,num-strings and
> > > qcom,enabled-strings are ambiguous\n");
> >
> > The warning should also be below the error message on the next if statement.
>
> Agreed.
Thinking about this again while reworking the patches, I initially put
this above the error to make DT writers aware. There's no point telling
them that their values are out of sync (num-strings >
len(enabled-strings)), when they "shouldn't even" (don't need to) set
both in the first place. They might needlessly fix the discrepancy, see
the driver finally probe (working backlight) and carry on without
noticing this warning that now appears.
Sorry for bringing this back up, but I'm curious about your opinion.
- Marijn
Re: [PATCH 0/2] Nuke PAGE_KERNEL_IO
On 10/21/21 11:15, Lucas De Marchi wrote: Last user of PAGE_KERNEL_IO is the i915 driver. While removing it from there as we seek to bring the driver to other architectures, Daniel suggested that we could finish the cleanup and remove it altogether, through the tip tree. So here I'm sending both commits needed for that. Lucas De Marchi (2): drm/i915/gem: stop using PAGE_KERNEL_IO x86/mm: nuke PAGE_KERNEL_IO arch/x86/include/asm/fixmap.h | 2 +- arch/x86/include/asm/pgtable_types.h | 7 --- arch/x86/mm/ioremap.c | 2 +- arch/x86/xen/setup.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 4 ++-- include/asm-generic/fixmap.h | 2 +- 6 files changed, 6 insertions(+), 13 deletions(-) Acked-by: Andy Lutomirski
Re: [PATCH] drm/i915/pmu: Increase the live_engine_busy_stats sample period
On Thu, Nov 11, 2021 at 06:52:22PM -0800, Umesh Nerlige Ramappa wrote: > Irrespective of the backend for request submissions, busyness for an > engine with an active context is calculated using: > > busyness = total + (current_time - context_switch_in_time) > > In execlists mode of operation, the context switch events are handled > by the CPU. Context switch in/out time and current_time are captured > in CPU time domain using ktime_get(). > > In GuC mode of submission, context switch events are handled by GuC and > the times in the above formula are captured in GT clock domain. This > information is shared with the CPU through shared memory. This results > in 2 caveats: > > 1) The time taken between start of a batch and the time that CPU is able > to see the context_switch_in_time in shared memory is dependent on GuC > and memory bandwidth constraints. > > 2) Determining current_time requires an MMIO read that can take anywhere > between a few us to a couple ms. A reference CPU time is captured soon > after reading the MMIO so that the caller can compare the cpu delta > between 2 busyness samples. The issue here is that the CPU delta and the > busyness delta can be skewed because of the time taken to read the > register. > > These 2 factors affect the accuracy of the selftest - > live_engine_busy_stats. For (1) the selftest waits until busyness stats > are visible to the CPU. The effects of (2) are more prominent for the > current busyness sample period of 100 us. Increase the busyness sample > period from 100 us to 10 ms to overccome (2). > > Signed-off-by: Umesh Nerlige Ramappa Explaination of increased wait period makes sense to me. With that: Reviewed-by: Matthew Brost > --- > drivers/gpu/drm/i915/gt/selftest_engine_pm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c > b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c > index 0bfd738dbf3a..96cc565afa78 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c > +++ b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c > @@ -316,7 +316,7 @@ static int live_engine_busy_stats(void *arg) > ENGINE_TRACE(engine, "measuring busy time\n"); > preempt_disable(); > de = intel_engine_get_busy_time(engine, &t[0]); > - udelay(100); > + udelay(1); > de = ktime_sub(intel_engine_get_busy_time(engine, &t[1]), de); > preempt_enable(); > dt = ktime_sub(t[1], t[0]); > -- > 2.20.1 >
Re: [PATCH v1 2/2] drm/tegra: Use drm_dp_aux_register_ddc/chardev() helpers
12.11.2021 23:26, Lyude Paul пишет:
>> BTW, I see now that DPAUX I2C transfer helper may access
>> aux->drm_device. Hence v1 patch isn't correct anyways.
>
> JFYI - unless I'm misunderstanding you, the aux->drm_dev accesses in the DPAUX
> i2c transfer functions are just from the various drm_{dbg,err,etc.} calls,
> which means that they all should be able to handle aux->drm_dev being NULL. If
> you can set aux->drm_dev before i2c transfers start that's more ideal, since
> otherwise you'll see the AUX device name as "(null)" in the kernel log, but
> any point before device registration should work.
Thanks, I realized that have seen DRM log with a such debug messages
just a day ago.
drm drm: [drm:drm_dp_i2c_do_msg] (null): transaction timed out
So yes, it's indeed not critical.
Re: [git pull] drm fixes + one missed next for 5.16-rc1
The pull request you sent on Fri, 12 Nov 2021 13:25:30 +1000: > git://anongit.freedesktop.org/drm/drm tags/drm-next-2021-11-12 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/304ac8032d3fa2d37750969cd4b8d5736a1829d9 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Re: [PATCH v1 2/2] drm/tegra: Use drm_dp_aux_register_ddc/chardev() helpers
On Fri, 2021-11-12 at 17:32 +0300, Dmitry Osipenko wrote:
> 12.11.2021 13:52, Thierry Reding пишет:
> > On Tue, Nov 09, 2021 at 05:39:02PM +0300, Dmitry Osipenko wrote:
> > > 09.11.2021 17:17, Dmitry Osipenko пишет:
> > > > 09.11.2021 17:08, Dmitry Osipenko пишет:
> > > > > > +static void host1x_drm_dev_deinit(struct host1x_device *dev)
> > > > > > +{
> > > > > > + struct drm_device *drm = dev_get_drvdata(&dev->dev);
> > > > > And platform_unregister_drivers() should be moved here.
> > > > >
> > > >
> > > > Nah, that should cause deadlock. This ad-hoc is too lame.
> > >
> > > Actually, there is no problem here as I see now. The host1x driver
> > > populates DT nodes after host1x_register() [1], meaning that Host1x DRM
> > > will be always inited first.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v5.15/source/drivers/gpu/host1x/dev.c#L475
> > >
> > > Still I'm not a fan of the ad-hoc solution.
> >
> > Could we not fix this by making the panel "hot-pluggable"? I don't think
> > there's anything inherent to the driver that would prevent doing so. The
> > original reason for why things are as intertwined as they are now is
> > because back at the time deferred framebuffer creation didn't exist. In
> > fact I added deferred framebuffer support with Daniel's help precisely
> > to fix a similar issue for things like HDMI and DP.
>
> I don't understand what do you mean by "hot-pluggable", panel is static.
> Please either clarify more or type the patch.
>
> Keep in mind that fix should be simple and portable because stable
> kernels are wrecked.
>
> > With HDMI and DP it's slightly less critical, because a lack of mode
> > support would've created a 1024x768 framebuffer, which most HDMI/DP
> > monitors support. However, with panels we need to ensure that the exact
> > mode from the panel is used to create the framebuffer, so it can only be
> > created when the panel is available.
> >
> > But, given that deferred framebuffer creation works now (which allows
> > the framebuffer console to show up at the preferred resolution for HDMI
> > and DP), even if a monitor is attached only after the driver has probed
> > already, we should be able to make something like that work with panels
> > as well.
>
> BTW, I see now that DPAUX I2C transfer helper may access
> aux->drm_device. Hence v1 patch isn't correct anyways.
JFYI - unless I'm misunderstanding you, the aux->drm_dev accesses in the DPAUX
i2c transfer functions are just from the various drm_{dbg,err,etc.} calls,
which means that they all should be able to handle aux->drm_dev being NULL. If
you can set aux->drm_dev before i2c transfers start that's more ideal, since
otherwise you'll see the AUX device name as "(null)" in the kernel log, but
any point before device registration should work.
>
> For now I'll try to test more the ad-hoc variant with Thomas and send it
> as v2 if we won't have a better solution.
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
Re: [PATCH 0/2] Nuke PAGE_KERNEL_IO
On 11/12/21 12:09 PM, Lucas De Marchi wrote: > The intention was to merge this through the tip tree. Although now I'm > not sure. Options: > > 1) take the first patch through the drm-intel tree and apply the > second patch later > 2) take everything through the drm tree > 3) take everything through the tip tree > > What's your preference here? It's fine with me to take it through tip unless that causes a problem for anyone. I was planning on doing queuing it after -rc1.
Re: [git pull] drm fixes + one missed next for 5.16-rc1
On Thu, Nov 11, 2021 at 7:25 PM Dave Airlie wrote: > > I missed a drm-misc-next pull for the main pull last week. It wasn't > that major and isn't the bulk of this at all. This has a bunch of > fixes all over, a lot for amdgpu and i915. Ugh. The i915 conflict was trivial, but made me aware of that absolutely disgusting "wbinvd_on_all_cpus()" hack. And that thing is much too ugly to survive. I made my merge resolution remove that disgusting thing. That driver is x86-only anyway, so it all seemed completely bogus in the first place. And if there is some actual non-x86 work in progress for i915, then that wbinvd_on_all_cpus() needs to be replaced with something proper and architecture-neutral anyway, most definitely involving a name change, and almost certainly also involving a range for the cache writeback. Because that "create broken macro on other architectures" thing is *NOT* acceptable. And I sincerely hope to the gods that no cache-incoherent i915 mess ever makes it out of the x86 world. Incoherent IO was always a historical mistake and should never ever happen again, so we should not spread that horrific pattern around. Linus
Re: [PATCH 0/2] Nuke PAGE_KERNEL_IO
On Fri, Nov 12, 2021 at 08:04:03PM +0100, Peter Zijlstra wrote: On Thu, Oct 21, 2021 at 11:15:09AM -0700, Lucas De Marchi wrote: Last user of PAGE_KERNEL_IO is the i915 driver. While removing it from there as we seek to bring the driver to other architectures, Daniel suggested that we could finish the cleanup and remove it altogether, through the tip tree. So here I'm sending both commits needed for that. Lucas De Marchi (2): drm/i915/gem: stop using PAGE_KERNEL_IO x86/mm: nuke PAGE_KERNEL_IO arch/x86/include/asm/fixmap.h | 2 +- arch/x86/include/asm/pgtable_types.h | 7 --- arch/x86/mm/ioremap.c | 2 +- arch/x86/xen/setup.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 4 ++-- include/asm-generic/fixmap.h | 2 +- 6 files changed, 6 insertions(+), 13 deletions(-) Acked-by: Peter Zijlstra (Intel) thanks, Peter. The intention was to merge this through the tip tree. Although now I'm not sure. Options: 1) take the first patch through the drm-intel tree and apply the second patch later 2) take everything through the drm tree 3) take everything through the tip tree What's your preference here? Lucas De Marchi
[Bug 205089] amdgpu : drm:amdgpu_cs_ioctl : Failed to initialize parser -125
https://bugzilla.kernel.org/show_bug.cgi?id=205089 Antoni (56turtl...@gmail.com) changed: What|Removed |Added CC||56turtl...@gmail.com --- Comment #24 from Antoni (56turtl...@gmail.com) --- this bug triggers almost every day for me. I use awesomeWM on arch linux (also tried KDE but it also happens there). software linux 5.14.16.arch1-1 mesa 21.2.4-1 awesome 4.3-3 hardware amd ryzen 5 5600g (integrated gpu) -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH 0/2] Nuke PAGE_KERNEL_IO
On Thu, Oct 21, 2021 at 11:15:09AM -0700, Lucas De Marchi wrote: > Last user of PAGE_KERNEL_IO is the i915 driver. While removing it from > there as we seek to bring the driver to other architectures, Daniel > suggested that we could finish the cleanup and remove it altogether, > through the tip tree. So here I'm sending both commits needed for that. > > Lucas De Marchi (2): > drm/i915/gem: stop using PAGE_KERNEL_IO > x86/mm: nuke PAGE_KERNEL_IO > > arch/x86/include/asm/fixmap.h | 2 +- > arch/x86/include/asm/pgtable_types.h | 7 --- > arch/x86/mm/ioremap.c | 2 +- > arch/x86/xen/setup.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 4 ++-- > include/asm-generic/fixmap.h | 2 +- > 6 files changed, 6 insertions(+), 13 deletions(-) Acked-by: Peter Zijlstra (Intel)
Re: [Intel-gfx] [PATCH] drm/i915/execlists: Weak parallel submission support for execlists
On Fri, Nov 12, 2021 at 02:13:50PM +, Tvrtko Ursulin wrote:
>
> On 11/11/2021 16:49, Matthew Brost wrote:
> > On Mon, Nov 01, 2021 at 10:35:09AM +, Tvrtko Ursulin wrote:
> > >
> > > On 27/10/2021 21:10, Matthew Brost wrote:
> > > > On Wed, Oct 27, 2021 at 01:04:49PM -0700, John Harrison wrote:
> > > > > On 10/27/2021 12:17, Matthew Brost wrote:
> > > > > > On Tue, Oct 26, 2021 at 02:58:00PM -0700, John Harrison wrote:
> > > > > > > On 10/20/2021 14:47, Matthew Brost wrote:
> > > > > > > > A weak implementation of parallel submission (multi-bb execbuf
> > > > > > > > IOCTL) for
> > > > > > > > execlists. Doing as little as possible to support this
> > > > > > > > interface for
> > > > > > > > execlists - basically just passing submit fences between each
> > > > > > > > request
> > > > > > > > generated and virtual engines are not allowed. This is on par
> > > > > > > > with what
> > > > > > > > is there for the existing (hopefully soon deprecated) bonding
> > > > > > > > interface.
> > > > > > > >
> > > > > > > > We perma-pin these execlists contexts to align with GuC
> > > > > > > > implementation.
> > > > > > > >
> > > > > > > > v2:
> > > > > > > > (John Harrison)
> > > > > > > > - Drop siblings array as num_siblings must be 1
> > > > > > > >
> > > > > > > > Signed-off-by: Matthew Brost
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 10 +++--
> > > > > > > > drivers/gpu/drm/i915/gt/intel_context.c | 4 +-
> > > > > > > > .../drm/i915/gt/intel_execlists_submission.c | 44
> > > > > > > > ++-
> > > > > > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +
> > > > > > > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 -
> > > > > > > > 5 files changed, 52 insertions(+), 10 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > > index fb33d0322960..35e87a7d0ea9 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > > @@ -570,10 +570,6 @@
> > > > > > > > set_proto_ctx_engines_parallel_submit(struct
> > > > > > > > i915_user_extension __user *base,
> > > > > > > > struct intel_engine_cs **siblings = NULL;
> > > > > > > > intel_engine_mask_t prev_mask;
> > > > > > > > - /* FIXME: This is NIY for execlists */
> > > > > > > > - if (!(intel_uc_uses_guc_submission(&i915->gt.uc)))
> > > > > > > > - return -ENODEV;
> > > > > > > > -
> > > > > > > > if (get_user(slot, &ext->engine_index))
> > > > > > > > return -EFAULT;
> > > > > > > > @@ -583,6 +579,12 @@
> > > > > > > > set_proto_ctx_engines_parallel_submit(struct
> > > > > > > > i915_user_extension __user *base,
> > > > > > > > if (get_user(num_siblings, &ext->num_siblings))
> > > > > > > > return -EFAULT;
> > > > > > > > + if (!intel_uc_uses_guc_submission(&i915->gt.uc) &&
> > > > > > > > num_siblings != 1) {
> > > > > > > > + drm_dbg(&i915->drm, "Only 1 sibling (%d)
> > > > > > > > supported in non-GuC mode\n",
> > > > > > > > + num_siblings);
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > if (slot >= set->num_engines) {
> > > > > > > > drm_dbg(&i915->drm, "Invalid placement value,
> > > > > > > > %d >= %d\n",
> > > > > > > > slot, set->num_engines);
> > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > > > b/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > > > index 5634d14052bc..1bec92e1d8e6 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > > > @@ -79,7 +79,8 @@ static int
> > > > > > > > intel_context_active_acquire(struct intel_context *ce)
> > > > > > > > __i915_active_acquire(&ce->active);
> > > > > > > > - if (intel_context_is_barrier(ce) ||
> > > > > > > > intel_engine_uses_guc(ce->engine))
> > > > > > > > + if (intel_context_is_barrier(ce) ||
> > > > > > > > intel_engine_uses_guc(ce->engine) ||
> > > > > > > > + intel_context_is_parallel(ce))
> > > > > > > > return 0;
> > > > > > > > /* Preallocate tracking nodes */
> > > > > > > > @@ -563,7 +564,6 @@ void intel_context_bind_parent_child(struct
> > > > > > > > intel_context *parent,
> > > > > > > > * Callers responsibility to validate that this
> > > > > > > > function is used
> > > > > > > > * correctly but we use GEM_BUG_ON here ensure that
> > > > > > > > they do.
> > > > > > > > */
> > > > > > > > - GEM_BUG_ON(!intel_engine_uses_guc(parent->engine));
> > > > > > > > GEM_BUG_ON(intel_context_
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On Fri, 12 Nov 2021 12:32:23 -0500 Jason Baron wrote: > Ok, it looks like Vincent's patch defines a dyndbg event and then uses > 'trace_dyndbg()' to output to the 'main' log. So all dynamic output to > the 'main' ftrace buffer goes through that event if I understand it > correctly. Here's a pointer to it for reference: > > https://lore.kernel.org/lkml/20200825153338.17061-3-vincent.whitchu...@axis.com/ > > Would you be ok with that approach? Yes that approach is fine, because it doesn't actually go to the main log unless you enable the dyndbg trace event in the main buffer. You could also enable that event in an instance and have it go there. -- Steve
Re: [PATCH v3 3/3] MAINTAINERS: Mark VMware mailing list entries as email aliases
[ Resending since my previous reply didn't reach the mailing lists. ] On 11/11/21 5:55 AM, Jakub Kicinski wrote: > On Wed, 10 Nov 2021 21:19:53 -0800 Joe Perches wrote: >> On Wed, 2021-11-10 at 17:39 -0800, Jakub Kicinski wrote: >>> On Wed, 10 Nov 2021 12:09:06 -0800 Srivatsa S. Bhat wrote: DRM DRIVER FOR VMWARE VIRTUAL GPU -M:"VMware Graphics" M:Zack Rusin +R:VMware Graphics Reviewers L:dri-devel@lists.freedesktop.org S:Supported T:git git://anongit.freedesktop.org/drm/drm-misc >>> >>> It'd be preferable for these corporate entries to be marked or >>> otherwise distinguishable so that we can ignore them when we try >>> to purge MAINTAINERS from developers who stopped participating. >>> >>> These addresses will never show up in a commit tag which is normally >>> sign of inactivity. >> >> Funny. >> >> The link below is from over 5 years ago. >> >> https://lore.kernel.org/lkml/1472081625.3746.217.ca...@perches.com/ >> >> Almost all of those entries are still in MAINTAINERS. >> >> I think the concept of purging is a non-issue. > > I cleaned networking in January and intend to do it again in 2 months. > See: > > 054c4610bd05 MAINTAINERS: dccp: move Gerrit Renker to CREDITS > 4f3786e01194 MAINTAINERS: ipvs: move Wensong Zhang to CREDITS > 0e4ed0b62b5a MAINTAINERS: tls: move Aviad to CREDITS > c41efbf2ad56 MAINTAINERS: ena: remove Zorik Machulsky from reviewers > 5e62d124f75a MAINTAINERS: vrf: move Shrijeet to CREDITS > 09cd3f4683a9 MAINTAINERS: net: move Alexey Kuznetsov to CREDITS > 93089de91e85 MAINTAINERS: altx: move Jay Cliburn to CREDITS > 8b0f64b113d6 MAINTAINERS: remove names from mailing list maintainers > I'm assuming the purging is not totally automated, is it? As long as the entries are informative to a human reader, it should be possible to skip the relevant ones when purging inactive entries. I believe this patch makes the situation better than it is currently (at least for the human reader), by marking lists without public read-access in a format that is more appropriate. In the future, we could perhaps improve on it to ease automation too, but for now I think it is worthwhile to merge this change (unless there are strong objections or better alternatives that everyone agrees on). Regards, Srivatsa
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On 11/12/21 12:07 PM, Steven Rostedt wrote: > On Fri, 12 Nov 2021 10:08:41 -0500 > Jason Baron wrote: > >>> A key difference between that patchset and this patch (besides that >>> small fact that I used +x instead of +T) was that my patchset allowed >>> the dyndbg trace to be emitted to the main buffer and did not force them >>> to be in an instance-specific buffer. >> >> Yes, I agree I'd prefer that we print here to the 'main' buffer - it seems >> to keep things simpler and easier to combine the output from different >> sources as you mentioned. > > I do not want anything to print to the "main buffer" that can not be > filtered or turned off by the tracing infrastructure itself (aka tracefs > file system). > > Once we allow that, then the trace file will become useless because > everything will write to the main buffer and fill it with noise. > > Events that can be enabled and disabled from tracefs are fine, as they can > be limited. This is why I added that nasty warning if people leave around > trace_printk(), because it does exactly this (write to the main buffer). > It's fine for debugging a custom kernel, but should never be enabled in > something that is shipped, or part of mainline. > > -- Steve > Ok, it looks like Vincent's patch defines a dyndbg event and then uses 'trace_dyndbg()' to output to the 'main' log. So all dynamic output to the 'main' ftrace buffer goes through that event if I understand it correctly. Here's a pointer to it for reference: https://lore.kernel.org/lkml/20200825153338.17061-3-vincent.whitchu...@axis.com/ Would you be ok with that approach? Thanks, -Jason
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On Fri, 12 Nov 2021 10:08:41 -0500 Jason Baron wrote: > > A key difference between that patchset and this patch (besides that > > small fact that I used +x instead of +T) was that my patchset allowed > > the dyndbg trace to be emitted to the main buffer and did not force them > > to be in an instance-specific buffer. > > Yes, I agree I'd prefer that we print here to the 'main' buffer - it seems to > keep things simpler and easier to combine the output from different > sources as you mentioned. I do not want anything to print to the "main buffer" that can not be filtered or turned off by the tracing infrastructure itself (aka tracefs file system). Once we allow that, then the trace file will become useless because everything will write to the main buffer and fill it with noise. Events that can be enabled and disabled from tracefs are fine, as they can be limited. This is why I added that nasty warning if people leave around trace_printk(), because it does exactly this (write to the main buffer). It's fine for debugging a custom kernel, but should never be enabled in something that is shipped, or part of mainline. -- Steve
Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
On Thu, Nov 11, 2021 at 12:39:28PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 11, 2021 at 12:11:20PM +0100, Javier Martinez Canillas wrote:
> > The efifb and simplefb drivers just render to a pre-allocated frame buffer
> > and rely on the display hardware being initialized before the kernel boots.
> >
> > But if another driver already probed correctly and registered a fbdev, the
> > generic drivers shouldn't be probed since an actual driver for the display
> > hardware is already present.
> >
> > This is more likely to occur after commit d391c5827107 ("drivers/firmware:
> > move x86 Generic System Framebuffers support") since the "efi-framebuffer"
> > and "simple-framebuffer" platform devices are registered at a later time.
> >
> > Link: https://lore.kernel.org/r/2020200253.rfudkt3edbd3nsyj@lahvuun/
> > Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System
> > Framebuffers support")
> > Reported-by: Ilya Trukhanov
> > Signed-off-by: Javier Martinez Canillas
> > Reviewed-by: Daniel Vetter
> > ---
> >
> > Changes in v2:
> > - Add a Link: tag with a reference to the bug report (Thorsten Leemhuis).
> > - Add a comment explaining why the probe fails earlier (Daniel Vetter).
> > - Add a Fixes: tag for stable to pick the fix (Daniel Vetter).
>
> That does not mean that it will make it into the stable tree. Please
> read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
Defacto your auto-picker is aggressive enough that just Fixes: is actually
good enough to get it into stable :-)
But yeah explicit cc: stable can't hurt.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: Questions about KMS flip
On 2021-11-12 16:03, Christian König wrote: > Am 12.11.21 um 15:30 schrieb Michel Dänzer: >> On 2021-11-12 15:29, Michel Dänzer wrote: >>> On 2021-11-12 13:47, Christian König wrote: Anyway this unfortunately turned out to be work for Harray and Nicholas. In detail it's about this bug report here: https://bugzilla.kernel.org/show_bug.cgi?id=214621 Lang was able to reproduce the issue and narrow it down to the pin in amdgpu_display_crtc_page_flip_target(). In other words we somehow have an unbalanced pinning of the scanout buffer in DC. >>> DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The >>> corresponding pin with DC would be in dm_plane_helper_prepare_fb, paired >>> with the unpin in >>> dm_plane_helper_cleanup_fb. >>> >>> >>> With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired with >>> the unpin in dm_plane_helper_cleanup_fb >> This should say amdgpu_display_unpin_work_func. > > Ah! So that is the classic (e.g. non atomic) path? Presumably. >>> & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by if >>> (!adev->enable_virtual_display), but the unpins seem unconditional. So >>> could this be about virtual display, and the problem is actually trying to >>> unpin a BO that was never pinned? > > Nope, my educated guess is rather that we free up the BO before > amdgpu_display_unpin_work_func is called. > > E.g. not pin unbalance, but rather use after free. amdgpu_display_crtc_page_flip_target calls amdgpu_bo_ref(work->old_abo), and amdgpu_display_unpin_work_func calls amdgpu_bo_unref(&work->old_abo) only after amdgpu_bo_unpin. So what you describe could only happen if there's an imbalance elsewhere such that amdgpu_bo_unref is called more often than amdgpu_bo_ref, or maybe if amdgpu_bo_reserve fails in amdgpu_display_unpin_work_func (in which case the "failed to reserve buffer after flip" error message should appear in dmesg). -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [PATCH] drm/i915/guc/slpc: Check GuC status before freq boost
On Thu, 11 Nov 2021 23:10:16 -0800, Vinay Belgaumkar wrote:
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> index 4e1d3cd29164..22c1c12369f2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> @@ -183,11 +183,15 @@ static int slpc_unset_param(struct intel_guc_slpc *slpc,
> static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq)
> {
> struct drm_i915_private *i915 = slpc_to_i915(slpc);
> + struct intel_guc *guc = slpc_to_guc(slpc);
> intel_wakeref_t wakeref;
> int ret = 0;
>
> lockdep_assert_held(&slpc->lock);
>
> + if (!intel_guc_is_ready(guc))
> + return -ENODEV;
> +
Reviewed-by: Ashutosh Dixit
The test wedges/resets the GPU after a request is queued but before it is
retired.
Re: [Intel-gfx] [PATCH 02/28] drm/i915: use new iterator in i915_gem_object_wait_reservation
On Thu, Nov 11, 2021 at 12:36:47PM +0100, Christian König wrote: > Am 01.11.21 um 10:41 schrieb Tvrtko Ursulin: > > > > On 28/10/2021 16:30, Daniel Vetter wrote: > > > On Thu, Oct 28, 2021 at 10:41:38AM +0200, Christian König wrote: > > > > Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin: > > > > > > > > > > On 21/10/2021 12:06, Maarten Lankhorst wrote: > > > > > > Op 21-10-2021 om 12:38 schreef Christian König: > > > > > > > Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: > > > > > > > > From: Christian König > > > > > > > > > > > > > > > > Simplifying the code a bit. > > > > > > > > > > > > > > > > Signed-off-by: Christian König > > > > > > > > [mlankhorst: Handle timeout = 0 correctly, use new > > > > > > > > i915_request_wait_timeout.] > > > > > > > > Signed-off-by: Maarten Lankhorst > > > > > > > > > > > > > > > > > > > > > > LGTM, do you want to push it or should I pick it up > > > > > > > into drm-misc-next? > > > > > > > > > > > > I think it can be applied to drm-intel-gt-next, after a backmerge. > > > > > > It needs patch 1 too, which fixes > > > > > > > > > > > > i915_request_wait semantics when used in dma-fence. It exports a > > > > > > dma-fence compatible i915_request_wait_timeout function, used in > > > > > > this patch. > > > > > > > > What about the other i915 patches? I guess you then want to merge them > > > > through drm-intel-gt-next as well. > > > > > > > > > I don't think my open has been resolved, at least I haven't > > > > > seen a reply > > > > > from Daniel on the topic of potential for infinite waits > > > > > with untrusted > > > > > clients after this change. +Daniel > > > > > > > > Please resolve that internally and let me know the result. I'm > > > > fine to use > > > > any of the possible approaches, I just need to know which one. > > > > > > I thought I explained this in the patch set from Maarten. This isn't an > > > issue, since the exact same thing can happen if you get interrupts and > > > stuff. > > > > Ah were you trying to point out all this time the infinite wait just got > > moved from inside the "old" dma_resv_get_fences to the new iterator > > caller? > > At least I think so, yes. But Daniel needs to answer that. Well maybe there's also an infinite wait inside the old dma_resv_get_fences, what I mean is that when you have some signals interrupting you, then the infinite wait is already there due to a retry loop outside of the syscall even. Anyway _any_ userspace which wants to use this wait on a shared bo and waits to be safe against the other end adding more rendering has to use something else (like the sync_file export ioctl on the dma-buf that Jason typed). Trying to make this ioctl here against fence addition is just bs. > > Regards, > > > > Tvrtko > > > > > > > > The only proper fix for bounding the waits is a) compositor grabs a > > > stable > > > set of dma_fence from the dma-buf through the proposed fence export > > > ioctl > > > b) compositor waits on that fence (or drm_syncobj). > > > > > > Everything else is cargo-culted nonsense, and very much includes > > > that igt > > > patch that's floating around internally. > > > > > > I can also whack this into drm-next if this is stuck in this silly > > > bikeshed. > > Can we move forward with those patches? I still don't see them in > drm-misc-next. > > I you want I can also pick Maartens modified version here up and send it out > with the reset of the i915 patches once more. > > Everything else is already pushed. Please push to drm-misc-next or wherever (assuming CI is happy) and feel free to add my ack. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/i915: Don't read query SSEU for non-existent slice 0 on old platforms
Pre-HSW platforms don't use the gt SSEU structures; this means that
calling intel_sseu_get_subslices() on slice 0 for these platforms will
trip a GEM_BUG_ON(slice >= sseu->max_slices) warning.
Let's move the DSS lookup for a DG2 workaround into a helper function
that will only get called after we've already decided that we're on a
DG2 platform.
Fixes: 645cc0b9d972 ("drm/i915/dg2: Add initial gt/ctx/engine workarounds")
Signed-off-by: Matt Roper
---
drivers/gpu/drm/i915/gt/intel_workarounds.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 51591119da15..a9727447c037 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -2019,11 +2019,18 @@ engine_fake_wa_init(struct intel_engine_cs *engine,
struct i915_wa_list *wal)
CMD_CCTL_MOCS_OVERRIDE(mocs, mocs));
}
}
+
+static bool needs_wa_1308578152(struct intel_engine_cs *engine)
+{
+ u64 dss_mask = intel_sseu_get_subslices(&engine->gt->info.sseu, 0);
+
+ return (dss_mask & GENMASK(GEN_DSS_PER_GSLICE - 1, 0)) == 0;
+}
+
static void
rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
{
struct drm_i915_private *i915 = engine->i915;
- u64 dss_mask = intel_sseu_get_subslices(&engine->gt->info.sseu, 0);
if (IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0)) {
/* Wa_14013392000:dg2_g11 */
@@ -2057,7 +2064,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct
i915_wa_list *wal)
/* Wa_1308578152:dg2_g10 when first gslice is fused off */
if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_B0, STEP_C0) &&
- (dss_mask & GENMASK(GEN_DSS_PER_GSLICE - 1, 0)) == 0) {
+ needs_wa_1308578152(engine)) {
wa_masked_dis(wal, GEN12_CS_DEBUG_MODE1_CCCSUNIT_BE_COMMON,
GEN12_REPLAY_MODE_GRANULARITY);
}
--
2.33.0
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On 11/12/21 6:49 AM, Vincent Whitchurch wrote: > On Thu, Nov 11, 2021 at 03:02:04PM -0700, Jim Cromie wrote: >> Sean Paul proposed, in: >> https://urldefense.com/v3/__https://patchwork.freedesktop.org/series/78133/__;!!GjvTz_vk!HcKnMRByYkIdyF1apqQjlN5aBIomzJR1an3YWXM6KXs0EftVMQdrewRA8Dki4A$ >> >> drm/trace: Mirror DRM debug logs to tracefs >> >> His patchset's objective is to be able to independently steer some of >> the drm.debug stream to an alternate tracing destination, by splitting >> drm_debug_enabled() into syslog & trace flavors, and enabling them >> separately. 2 advantages were identified: >> >> 1- syslog is heavyweight, tracefs is much lighter >> 2- separate selection of enabled categories means less traffic >> >> Dynamic-Debug can do 2nd exceedingly well: >> >> A- all work is behind jump-label's NOOP, zero off cost. >> B- exact site selectivity, precisely the useful traffic. >>can tailor enabled set interactively, at shell. >> >> Since the tracefs interface is effective for drm (the threads suggest >> so), adding that interface to dynamic-debug has real potential for >> everyone including drm. >> >> if CONFIG_TRACING: >> >> Grab Sean's trace_init/cleanup code, use it to provide tracefs >> available by default to all pr_debugs. This will likely need some >> further per-module treatment; perhaps something reflecting hierarchy >> of module,file,function,line, maybe with a tuned flattening. >> >> endif CONFIG_TRACING >> >> Add a new +T flag to enable tracing, independent of +p, and add and >> use 3 macros: dyndbg_site_is_enabled/logging/tracing(), to encapsulate >> the flag checks. Existing code treats T like other flags. > > I posted a patchset a while ago to do something very similar, but that > got stalled for some reason and I unfortunately didn't follow it up: > > > https://urldefense.com/v3/__https://lore.kernel.org/lkml/20200825153338.17061-1-vincent.whitchu...@axis.com/__;!!GjvTz_vk!HcKnMRByYkIdyF1apqQjlN5aBIomzJR1an3YWXM6KXs0EftVMQdrewRGytKHPg$ > > > A key difference between that patchset and this patch (besides that > small fact that I used +x instead of +T) was that my patchset allowed > the dyndbg trace to be emitted to the main buffer and did not force them > to be in an instance-specific buffer. Yes, I agree I'd prefer that we print here to the 'main' buffer - it seems to keep things simpler and easier to combine the output from different sources as you mentioned. Thanks, -Jason > > That feature is quite important at least for my use case since I often > use dyndbg combined with function tracing, and the latter doesn't work > on non-main instances according to Documentation/trace/ftrace.rst. > > For example, here's a random example of a bootargs from one of my recent > debugging sessions: > > trace_event=printk:* ftrace_filter=_mmc*,mmc*,sd*,dw_mci*,mci* > ftrace=function trace_buf_size=20M dyndbg="file drivers/mmc/* +x" >
Re: [PATCH v3] drm/i915: Skip error capture when wedged on init
On 11/11/2021 13:06, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Trying to capture uninitialised engines when we wedged on init ends in
tears. Skip that together with uC capture, since failure to initialise the
latter can actually be one of the reasons for wedging on init.
v2:
* Use i915_disable_error_state when wedging on init/fini.
v3:
* Handle mock tests.
Signed-off-by: Tvrtko Ursulin
Reviewed-by: Matthew Auld # v1
Assuming this works locally, r-b still stands.
---
drivers/gpu/drm/i915/gt/intel_reset.c| 2 ++
drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
b/drivers/gpu/drm/i915/gt/intel_reset.c
index 51b56b8e5003..0fbd6dbadce7 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -1448,6 +1448,7 @@ void intel_gt_set_wedged_on_init(struct intel_gt *gt)
BUILD_BUG_ON(I915_RESET_ENGINE + I915_NUM_ENGINES >
I915_WEDGED_ON_INIT);
intel_gt_set_wedged(gt);
+ i915_disable_error_state(gt->i915, -ENODEV);
set_bit(I915_WEDGED_ON_INIT, >->reset.flags);
/* Wedged on init is non-recoverable */
@@ -1457,6 +1458,7 @@ void intel_gt_set_wedged_on_init(struct intel_gt *gt)
void intel_gt_set_wedged_on_fini(struct intel_gt *gt)
{
intel_gt_set_wedged(gt);
+ i915_disable_error_state(gt->i915, -ENODEV);
set_bit(I915_WEDGED_ON_FINI, >->reset.flags);
intel_gt_retire_requests(gt); /* cleanup any wedged requests */
}
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index 9ab3f284d1dd..d0e2e61de8d4 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -177,6 +177,8 @@ struct drm_i915_private *mock_gem_device(void)
mock_uncore_init(&i915->uncore, i915);
+ spin_lock_init(&i915->gpu_error.lock);
+
i915_gem_init__mm(i915);
intel_gt_init_early(&i915->gt, i915);
atomic_inc(&i915->gt.wakeref.count); /* disable; no hw support */
Re: [PATCH] drm/cma-helper: Release non-coherent memory with dma_free_noncoherent()
Hi Paul
Am 12.11.21 um 16:26 schrieb Paul Cercueil:
Hi Thomas,
I never received the original patch and I can't find it online either?
It was posted a while ago [1] and got lost. I remember that you had a
problem with your email setup. Maybe that's why you didn't see it.
Anyway:
Acked-by: Paul Cercueil
Thanks a lot.
Best regards
Thomas
[1]
https://lore.kernel.org/dri-devel/20210708175146.10618-1-tzimmerm...@suse.de/
Cheers,
-Paul
Le ven., nov. 12 2021 at 16:05:47 +0100, Thomas Zimmermann
a écrit :
Ping for review.
Am 08.07.21 um 19:51 schrieb Thomas Zimmermann:
The GEM CMA helpers allocate non-coherent (i.e., cached) backing storage
with dma_alloc_noncoherent(), but release it with dma_free_wc(). Fix
this
with a call to dma_free_noncoherent(). Writecombining storage is still
released with dma_free_wc().
Signed-off-by: Thomas Zimmermann
Fixes: cf8ccbc72d61 ("drm: Add support for GEM buffers backed by
non-coherent memory")
Cc: Paul Cercueil
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/drm_gem_cma_helper.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
index d53388199f34..9d05674550a4 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -210,8 +210,13 @@ void drm_gem_cma_free_object(struct
drm_gem_object *gem_obj)
dma_buf_vunmap(gem_obj->import_attach->dmabuf, &map);
drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
} else if (cma_obj->vaddr) {
- dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
- cma_obj->vaddr, cma_obj->paddr);
+ if (cma_obj->map_noncoherent)
+ dma_free_noncoherent(gem_obj->dev->dev, cma_obj->base.size,
+ cma_obj->vaddr, cma_obj->paddr,
+ DMA_TO_DEVICE);
+ else
+ dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
+ cma_obj->vaddr, cma_obj->paddr);
}
drm_gem_object_release(gem_obj);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
[PATCH 6/6] drm/i915: Drain the ttm delayed workqueue too
From: Maarten Lankhorst
Lets be thorough here. Users of the TTM backend would likely expect this
behaviour.
Signed-off-by: Maarten Lankhorst
Reviewed-by: Matthew Auld
Signed-off-by: Matthew Auld
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index da39bf5508ca..9641aab3e2cc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1819,6 +1819,7 @@ static inline void i915_gem_drain_freed_objects(struct
drm_i915_private *i915)
*/
while (atomic_read(&i915->mm.free_count)) {
flush_work(&i915->mm.free_work);
+ flush_delayed_work(&i915->bdev.wq);
rcu_barrier();
}
}
--
2.31.1
[PATCH 5/6] drm/i915: Remove resv from i915_vma
From: Maarten Lankhorst
It's just an alias to vma->obj->base.resv, no need to duplicate it.
Signed-off-by: Maarten Lankhorst
Reviewed-by: Niranjana Vishwanathapura
Signed-off-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++--
drivers/gpu/drm/i915/i915_vma.c| 9 -
drivers/gpu/drm/i915/i915_vma.h| 6 +++---
drivers/gpu/drm/i915/i915_vma_types.h | 1 -
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index ea5b7b2a4d70..9f7c6ecadb90 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1001,7 +1001,7 @@ static int eb_validate_vmas(struct i915_execbuffer *eb)
}
if (!(ev->flags & EXEC_OBJECT_WRITE)) {
- err = dma_resv_reserve_shared(vma->resv, 1);
+ err = dma_resv_reserve_shared(vma->obj->base.resv, 1);
if (err)
return err;
}
@@ -2175,7 +2175,7 @@ static int eb_parse(struct i915_execbuffer *eb)
goto err_trampoline;
}
- err = dma_resv_reserve_shared(shadow->resv, 1);
+ err = dma_resv_reserve_shared(shadow->obj->base.resv, 1);
if (err)
goto err_trampoline;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index fd7594e3e7e7..72c373a170a1 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -116,7 +116,6 @@ vma_create(struct drm_i915_gem_object *obj,
vma->vm = i915_vm_get(vm);
vma->ops = &vm->vma_ops;
vma->obj = obj;
- vma->resv = obj->base.resv;
vma->size = obj->base.size;
vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
@@ -1032,7 +1031,7 @@ int i915_ggtt_pin(struct i915_vma *vma, struct
i915_gem_ww_ctx *ww,
GEM_BUG_ON(!i915_vma_is_ggtt(vma));
#ifdef CONFIG_LOCKDEP
- WARN_ON(!ww && dma_resv_held(vma->resv));
+ WARN_ON(!ww && dma_resv_held(vma->obj->base.resv));
#endif
do {
@@ -1251,19 +1250,19 @@ int _i915_vma_move_to_active(struct i915_vma *vma,
}
if (fence) {
- dma_resv_add_excl_fence(vma->resv, fence);
+ dma_resv_add_excl_fence(vma->obj->base.resv, fence);
obj->write_domain = I915_GEM_DOMAIN_RENDER;
obj->read_domains = 0;
}
} else {
if (!(flags & __EXEC_OBJECT_NO_RESERVE)) {
- err = dma_resv_reserve_shared(vma->resv, 1);
+ err = dma_resv_reserve_shared(vma->obj->base.resv, 1);
if (unlikely(err))
return err;
}
if (fence) {
- dma_resv_add_shared_fence(vma->resv, fence);
+ dma_resv_add_shared_fence(vma->obj->base.resv, fence);
obj->write_domain = 0;
}
}
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 312933c06017..4033aa08d5e4 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -234,16 +234,16 @@ static inline void __i915_vma_put(struct i915_vma *vma)
kref_put(&vma->ref, i915_vma_release);
}
-#define assert_vma_held(vma) dma_resv_assert_held((vma)->resv)
+#define assert_vma_held(vma) dma_resv_assert_held((vma)->obj->base.resv)
static inline void i915_vma_lock(struct i915_vma *vma)
{
- dma_resv_lock(vma->resv, NULL);
+ dma_resv_lock(vma->obj->base.resv, NULL);
}
static inline void i915_vma_unlock(struct i915_vma *vma)
{
- dma_resv_unlock(vma->resv);
+ dma_resv_unlock(vma->obj->base.resv);
}
int __must_check
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h
b/drivers/gpu/drm/i915/i915_vma_types.h
index 4ee6e54799f4..f03fa96a1701 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i915_vma_types.h
@@ -187,7 +187,6 @@ struct i915_vma {
const struct i915_vma_ops *ops;
struct drm_i915_gem_object *obj;
- struct dma_resv *resv; /** Alias of obj->resv */
struct sg_table *pages;
void __iomem *iomap;
--
2.31.1
[PATCH 1/6] drm/i915: move the pre_pin earlier
In intel_context_do_pin_ww, when calling into the pre_pin hook(which is passed the ww context) it could in theory return -EDEADLK(which is very likely with debug kernels), once we start adding more ww locking in there, like in the next patch. If so then we need to be mindful of having to restart the do_pin at this point. If this is the kernel_context, or some other early in-kernel context where we have yet to setup the default_state, then we always inhibit the context restore, and instead rely on the delayed active_release to set the CONTEXT_VALID_BIT for us(if we even care), which should indicate that we have context switched away, and that our newly saved context state should now be valid. However, since we currently grab the active reference before the potential ww dance, we can end up setting the CONTEXT_VALID_BIT much too early, if we need to backoff, and then upon re-trying the do_pin, we could potentially cause the hardware to incorrectly load some garbage context state when later context switching to that context, but at the very least this will trigger the GEM_BUG_ON() in __engine_unpark. For now let's just move any ww dance stuff prior to arming the active reference. For normal user contexts this shouldn't be a concern, since we should already have the default_state ready when initialising the lrc state, and so there should be no concern with active_release somehow prematurely setting the CONTEXT_VALID_BIT. Signed-off-by: Matthew Auld Cc: Thomas Hellström Cc: Maarten Lankhorst --- drivers/gpu/drm/i915/gt/intel_context.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 5634d14052bc..ad44860faaf3 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -228,17 +228,17 @@ int __intel_context_do_pin_ww(struct intel_context *ce, if (err) return err; - err = i915_active_acquire(&ce->active); + err = ce->ops->pre_pin(ce, ww, &vaddr); if (err) goto err_ctx_unpin; - err = ce->ops->pre_pin(ce, ww, &vaddr); + err = i915_active_acquire(&ce->active); if (err) - goto err_release; + goto err_post_unpin; err = mutex_lock_interruptible(&ce->pin_mutex); if (err) - goto err_post_unpin; + goto err_release; intel_engine_pm_might_get(ce->engine); -- 2.31.1
[PATCH 2/6] drm/i915: Create a dummy object for gen6 ppgtt
From: Maarten Lankhorst
We currently have to special case vma->obj being NULL because
of gen6 ppgtt and mock_engine. Fix gen6 ppgtt, so we may soon
be able to remove a few checks. As the object only exists as
a fake object pointing to ggtt, we have no backing storage,
so no real object is created. It just has to look real enough.
Also kill pin_mutex, it's not compatible with ww locking,
and we can use the vm lock instead.
v2:
- Drop IS_SHRINKABLE and shorten overly long line
v3:
- Checkpatch fix for alignment
Signed-off-by: Maarten Lankhorst
Reviewed-by: Matthew Auld
Signed-off-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 44 ---
drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 123 +++
drivers/gpu/drm/i915/gt/gen6_ppgtt.h | 1 -
drivers/gpu/drm/i915/i915_drv.h | 4 +
4 files changed, 100 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index a57a6b7013c2..c5150a1ee3d2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -145,24 +145,10 @@ static const struct drm_i915_gem_object_ops
i915_gem_object_internal_ops = {
.put_pages = i915_gem_object_put_pages_internal,
};
-/**
- * i915_gem_object_create_internal: create an object with volatile pages
- * @i915: the i915 device
- * @size: the size in bytes of backing storage to allocate for the object
- *
- * Creates a new object that wraps some internal memory for private use.
- * This object is not backed by swappable storage, and as such its contents
- * are volatile and only valid whilst pinned. If the object is reaped by the
- * shrinker, its pages and data will be discarded. Equally, it is not a full
- * GEM object and so not valid for access from userspace. This makes it useful
- * for hardware interfaces like ringbuffers (which are pinned from the time
- * the request is written to the time the hardware stops accessing it), but
- * not for contexts (which need to be preserved when not active for later
- * reuse). Note that it is not cleared upon allocation.
- */
struct drm_i915_gem_object *
-i915_gem_object_create_internal(struct drm_i915_private *i915,
- phys_addr_t size)
+__i915_gem_object_create_internal(struct drm_i915_private *i915,
+ const struct drm_i915_gem_object_ops *ops,
+ phys_addr_t size)
{
static struct lock_class_key lock_class;
struct drm_i915_gem_object *obj;
@@ -179,7 +165,7 @@ i915_gem_object_create_internal(struct drm_i915_private
*i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, size);
- i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
0);
+ i915_gem_object_init(obj, ops, &lock_class, 0);
obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
/*
@@ -199,3 +185,25 @@ i915_gem_object_create_internal(struct drm_i915_private
*i915,
return obj;
}
+
+/**
+ * i915_gem_object_create_internal: create an object with volatile pages
+ * @i915: the i915 device
+ * @size: the size in bytes of backing storage to allocate for the object
+ *
+ * Creates a new object that wraps some internal memory for private use.
+ * This object is not backed by swappable storage, and as such its contents
+ * are volatile and only valid whilst pinned. If the object is reaped by the
+ * shrinker, its pages and data will be discarded. Equally, it is not a full
+ * GEM object and so not valid for access from userspace. This makes it useful
+ * for hardware interfaces like ringbuffers (which are pinned from the time
+ * the request is written to the time the hardware stops accessing it), but
+ * not for contexts (which need to be preserved when not active for later
+ * reuse). Note that it is not cleared upon allocation.
+ */
+struct drm_i915_gem_object *
+i915_gem_object_create_internal(struct drm_i915_private *i915,
+ phys_addr_t size)
+{
+ return __i915_gem_object_create_internal(i915,
&i915_gem_object_internal_ops, size);
+}
diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
index ae693bf88ef0..4a166d25fe60 100644
--- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
@@ -261,13 +261,10 @@ static void gen6_ppgtt_cleanup(struct i915_address_space
*vm)
{
struct gen6_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
- __i915_vma_put(ppgtt->vma);
-
gen6_ppgtt_free_pd(ppgtt);
free_scratch(vm);
mutex_destroy(&ppgtt->flush);
- mutex_destroy(&ppgtt->pin_mutex);
free_pd(&ppgtt->base.vm, ppgtt->base.pd);
}
@@ -330,37 +327,6 @@ static const struct i915_vma_ops pd_vma_ops = {
.unbind_vma = pd_vma_unbind,
};
-static struct i915_vma *pd_vma_create
[PATCH 4/6] drm/i915: vma is always backed by an object.
From: Maarten Lankhorst
vma->obj and vma->resv are now never NULL, and some checks can be removed.
Signed-off-by: Maarten Lankhorst
Reviewed-by: Matthew Auld
Signed-off-by: Matthew Auld
---
drivers/gpu/drm/i915/gt/intel_context.c | 2 +-
.../gpu/drm/i915/gt/intel_ring_submission.c | 2 +-
drivers/gpu/drm/i915/i915_vma.c | 48 ---
drivers/gpu/drm/i915/i915_vma.h | 3 --
4 files changed, 22 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index ad44860faaf3..e31669657dae 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -219,7 +219,7 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
*/
err = i915_gem_object_lock(ce->timeline->hwsp_ggtt->obj, ww);
- if (!err && ce->ring->vma->obj)
+ if (!err)
err = i915_gem_object_lock(ce->ring->vma->obj, ww);
if (!err && ce->state)
err = i915_gem_object_lock(ce->state->obj, ww);
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 586dca1731ce..3e6fac0340ef 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -1357,7 +1357,7 @@ int intel_ring_submission_setup(struct intel_engine_cs
*engine)
err = i915_gem_object_lock(timeline->hwsp_ggtt->obj, &ww);
if (!err && gen7_wa_vma)
err = i915_gem_object_lock(gen7_wa_vma->obj, &ww);
- if (!err && engine->legacy.ring->vma->obj)
+ if (!err)
err = i915_gem_object_lock(engine->legacy.ring->vma->obj, &ww);
if (!err)
err = intel_timeline_pin(timeline, &ww);
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 8781c4f61952..fd7594e3e7e7 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -40,12 +40,12 @@
static struct kmem_cache *slab_vmas;
-struct i915_vma *i915_vma_alloc(void)
+static struct i915_vma *i915_vma_alloc(void)
{
return kmem_cache_zalloc(slab_vmas, GFP_KERNEL);
}
-void i915_vma_free(struct i915_vma *vma)
+static void i915_vma_free(struct i915_vma *vma)
{
return kmem_cache_free(slab_vmas, vma);
}
@@ -426,10 +426,8 @@ int i915_vma_bind(struct i915_vma *vma,
work->base.dma.error = 0; /* enable the queue_work() */
- if (vma->obj) {
- __i915_gem_object_pin_pages(vma->obj);
- work->pinned = i915_gem_object_get(vma->obj);
- }
+ __i915_gem_object_pin_pages(vma->obj);
+ work->pinned = i915_gem_object_get(vma->obj);
} else {
vma->ops->bind_vma(vma->vm, NULL, vma, cache_level, bind_flags);
}
@@ -670,7 +668,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64
alignment, u64 flags)
}
color = 0;
- if (vma->obj && i915_vm_has_cache_coloring(vma->vm))
+ if (i915_vm_has_cache_coloring(vma->vm))
color = vma->obj->cache_level;
if (flags & PIN_OFFSET_FIXED) {
@@ -795,17 +793,14 @@ static bool try_qad_pin(struct i915_vma *vma, unsigned
int flags)
static int vma_get_pages(struct i915_vma *vma)
{
int err = 0;
- bool pinned_pages = false;
+ bool pinned_pages = true;
if (atomic_add_unless(&vma->pages_count, 1, 0))
return 0;
- if (vma->obj) {
- err = i915_gem_object_pin_pages(vma->obj);
- if (err)
- return err;
- pinned_pages = true;
- }
+ err = i915_gem_object_pin_pages(vma->obj);
+ if (err)
+ return err;
/* Allocations ahoy! */
if (mutex_lock_interruptible(&vma->pages_mutex)) {
@@ -838,8 +833,8 @@ static void __vma_put_pages(struct i915_vma *vma, unsigned
int count)
if (atomic_sub_return(count, &vma->pages_count) == 0) {
vma->ops->clear_pages(vma);
GEM_BUG_ON(vma->pages);
- if (vma->obj)
- i915_gem_object_unpin_pages(vma->obj);
+
+ i915_gem_object_unpin_pages(vma->obj);
}
mutex_unlock(&vma->pages_mutex);
}
@@ -875,7 +870,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct
i915_gem_ww_ctx *ww,
int err;
#ifdef CONFIG_PROVE_LOCKING
- if (debug_locks && !WARN_ON(!ww) && vma->resv)
+ if (debug_locks && !WARN_ON(!ww))
assert_vma_held(vma);
#endif
@@ -983,7 +978,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct
i915_gem_ww_ctx *ww,
GEM_BUG_ON(!vma->pages);
err = i915_vma_bind(vma,
- vma->obj ? vma->obj->cache_level : 0,
+ vma->obj->cache_level,
flags, work);
[PATCH 3/6] drm/i915: Create a full object for mock_ring, v2.
From: Maarten Lankhorst
This allows us to finally get rid of all the assumptions that vma->obj
is NULL.
Changes since v1:
- Ensure the mock_ring vma is pinned to prevent a fault.
- Pin it high to avoid failure in evict_for_vma selftest.
Signed-off-by: Maarten Lankhorst
Reviewed-by: Matthew Auld
Signed-off-by: Matthew Auld
---
drivers/gpu/drm/i915/gt/mock_engine.c | 38 ---
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c
b/drivers/gpu/drm/i915/gt/mock_engine.c
index 8b89215afe46..bb99fc03f503 100644
--- a/drivers/gpu/drm/i915/gt/mock_engine.c
+++ b/drivers/gpu/drm/i915/gt/mock_engine.c
@@ -35,9 +35,31 @@ static void mock_timeline_unpin(struct intel_timeline *tl)
atomic_dec(&tl->pin_count);
}
+static struct i915_vma *create_ring_vma(struct i915_ggtt *ggtt, int size)
+{
+ struct i915_address_space *vm = &ggtt->vm;
+ struct drm_i915_private *i915 = vm->i915;
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
+
+ obj = i915_gem_object_create_internal(i915, size);
+ if (IS_ERR(obj))
+ return ERR_CAST(obj);
+
+ vma = i915_vma_instance(obj, vm, NULL);
+ if (IS_ERR(vma))
+ goto err;
+
+ return vma;
+
+err:
+ i915_gem_object_put(obj);
+ return vma;
+}
+
static struct intel_ring *mock_ring(struct intel_engine_cs *engine)
{
- const unsigned long sz = PAGE_SIZE / 2;
+ const unsigned long sz = PAGE_SIZE;
struct intel_ring *ring;
ring = kzalloc(sizeof(*ring) + sz, GFP_KERNEL);
@@ -50,15 +72,11 @@ static struct intel_ring *mock_ring(struct intel_engine_cs
*engine)
ring->vaddr = (void *)(ring + 1);
atomic_set(&ring->pin_count, 1);
- ring->vma = i915_vma_alloc();
- if (!ring->vma) {
+ ring->vma = create_ring_vma(engine->gt->ggtt, PAGE_SIZE);
+ if (IS_ERR(ring->vma)) {
kfree(ring);
return NULL;
}
- i915_active_init(&ring->vma->active, NULL, NULL, 0);
- __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(ring->vma));
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &ring->vma->node.flags);
- ring->vma->node.size = sz;
intel_ring_update_space(ring);
@@ -67,8 +85,7 @@ static struct intel_ring *mock_ring(struct intel_engine_cs
*engine)
static void mock_ring_free(struct intel_ring *ring)
{
- i915_active_fini(&ring->vma->active);
- i915_vma_free(ring->vma);
+ i915_vma_put(ring->vma);
kfree(ring);
}
@@ -125,6 +142,7 @@ static void mock_context_unpin(struct intel_context *ce)
static void mock_context_post_unpin(struct intel_context *ce)
{
+ i915_vma_unpin(ce->ring->vma);
}
static void mock_context_destroy(struct kref *ref)
@@ -169,7 +187,7 @@ static int mock_context_alloc(struct intel_context *ce)
static int mock_context_pre_pin(struct intel_context *ce,
struct i915_gem_ww_ctx *ww, void **unused)
{
- return 0;
+ return i915_vma_pin_ww(ce->ring->vma, ww, 0, 0, PIN_GLOBAL | PIN_HIGH);
}
static int mock_context_pin(struct intel_context *ce, void *unused)
--
2.31.1
Re: [PATCH] drm/cma-helper: Release non-coherent memory with dma_free_noncoherent()
Hi Thomas,
I never received the original patch and I can't find it online either?
Anyway:
Acked-by: Paul Cercueil
Cheers,
-Paul
Le ven., nov. 12 2021 at 16:05:47 +0100, Thomas Zimmermann
a écrit :
Ping for review.
Am 08.07.21 um 19:51 schrieb Thomas Zimmermann:
The GEM CMA helpers allocate non-coherent (i.e., cached) backing
storage
with dma_alloc_noncoherent(), but release it with dma_free_wc(). Fix
this
with a call to dma_free_noncoherent(). Writecombining storage is
still
released with dma_free_wc().
Signed-off-by: Thomas Zimmermann
Fixes: cf8ccbc72d61 ("drm: Add support for GEM buffers backed by
non-coherent memory")
Cc: Paul Cercueil
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/drm_gem_cma_helper.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
index d53388199f34..9d05674550a4 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -210,8 +210,13 @@ void drm_gem_cma_free_object(struct
drm_gem_object *gem_obj)
dma_buf_vunmap(gem_obj->import_attach->dmabuf, &map);
drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
} else if (cma_obj->vaddr) {
- dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
- cma_obj->vaddr, cma_obj->paddr);
+ if (cma_obj->map_noncoherent)
+ dma_free_noncoherent(gem_obj->dev->dev,
cma_obj->base.size,
+cma_obj->vaddr, cma_obj->paddr,
+DMA_TO_DEVICE);
+ else
+ dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
+ cma_obj->vaddr, cma_obj->paddr);
}
drm_gem_object_release(gem_obj);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On Fri, Nov 12, 2021 at 03:19:17PM +0100, Marijn Suijten wrote:
> On 2021-11-12 13:23:36, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:
> > > On 2021-11-12 12:12:38, Daniel Thompson wrote:
> > > > On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > > > > The length of qcom,enabled-strings as property array is enough to
> > > > > determine the number of strings to be enabled, without needing to set
> > > > > qcom,num-strings to override the default number of strings when less
> > > > > than the default (which is also the maxium) is provided in DT.
> > > > >
> > > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver
> > > > > for WLED3")
> > > > > Signed-off-by: Marijn Suijten
> > > > > Reviewed-by: AngeloGioacchino Del Regno
> > > > >
> > > > > ---
> > > > > drivers/video/backlight/qcom-wled.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > > > b/drivers/video/backlight/qcom-wled.c
> > > > > index c5232478a343..9bfbf601762a 100644
> > > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > > > > return -EINVAL;
> > > > > }
> > > > > }
> > > > > +
> > > > > + cfg->num_strings = string_len;
> > > >
> > > > I still don't really understand why this wants to be a separate patch.
> > >
> > > I'm viewing this as a separate issue, and this makes it easier to
> > > document the change in a loose commit.
> > >
> > > > The warning text emitted by the previous patch (whatever text we agree
> > > > on) will be nonsense until this patch is applied.
> > > >
> > > > If this patch cannot appear before the warning is introduces then there
> > > > is no correct order for patches 4 and 5 (which implies they should be
> > > > the
> > > > same patch).
> > >
> > > Agreed, this is a weird way of doing things in v2 - the error message is
> > > printed yet the length of qcom,enabled-strings is always ignored before
> > > this patch.
> > >
> > > If we were to reorder patch 5 before patch 4 that should also
> > > temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
> > > this `if` so that `qcom,num-strings` remains the definitive way to
> > > set/override length. That's doable, and makes it easier to read patch 4
> > > as that bit of code will be replaced by of_property_read_u32 on that
> > > exact line. Let me know which method you prefer.
> >
> > Personally I would just squash them together. There are no redundant
> > values in the DT that could be fixed until we can use the string_len
> > to set num_strings.
>
> Reordering this patch before patch 4 in the way described above should
> allow just that, except that no warnings will be given for ambiguity
> until patch 4 is applied after that - which is weird given that that
> patch only intends the off-by-one error. Perhaps we should keep the
> order as it is, but add the ambiguity warning in this patch instead.
That works for me. Sounds good.
Daniel.
Re: [PATCH] drm/cma-helper: Release non-coherent memory with dma_free_noncoherent()
Ping for review.
Am 08.07.21 um 19:51 schrieb Thomas Zimmermann:
The GEM CMA helpers allocate non-coherent (i.e., cached) backing storage
with dma_alloc_noncoherent(), but release it with dma_free_wc(). Fix this
with a call to dma_free_noncoherent(). Writecombining storage is still
released with dma_free_wc().
Signed-off-by: Thomas Zimmermann
Fixes: cf8ccbc72d61 ("drm: Add support for GEM buffers backed by non-coherent
memory")
Cc: Paul Cercueil
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/drm_gem_cma_helper.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
index d53388199f34..9d05674550a4 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -210,8 +210,13 @@ void drm_gem_cma_free_object(struct drm_gem_object
*gem_obj)
dma_buf_vunmap(gem_obj->import_attach->dmabuf, &map);
drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
} else if (cma_obj->vaddr) {
- dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
- cma_obj->vaddr, cma_obj->paddr);
+ if (cma_obj->map_noncoherent)
+ dma_free_noncoherent(gem_obj->dev->dev,
cma_obj->base.size,
+cma_obj->vaddr, cma_obj->paddr,
+DMA_TO_DEVICE);
+ else
+ dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
+ cma_obj->vaddr, cma_obj->paddr);
}
drm_gem_object_release(gem_obj);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: Questions about KMS flip
Am 12.11.21 um 15:30 schrieb Michel Dänzer: On 2021-11-12 15:29, Michel Dänzer wrote: On 2021-11-12 13:47, Christian König wrote: Anyway this unfortunately turned out to be work for Harray and Nicholas. In detail it's about this bug report here: https://bugzilla.kernel.org/show_bug.cgi?id=214621 Lang was able to reproduce the issue and narrow it down to the pin in amdgpu_display_crtc_page_flip_target(). In other words we somehow have an unbalanced pinning of the scanout buffer in DC. DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The corresponding pin with DC would be in dm_plane_helper_prepare_fb, paired with the unpin in dm_plane_helper_cleanup_fb. With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired with the unpin in dm_plane_helper_cleanup_fb This should say amdgpu_display_unpin_work_func. Ah! So that is the classic (e.g. non atomic) path? & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by if (!adev->enable_virtual_display), but the unpins seem unconditional. So could this be about virtual display, and the problem is actually trying to unpin a BO that was never pinned? Nope, my educated guess is rather that we free up the BO before amdgpu_display_unpin_work_func is called. E.g. not pin unbalance, but rather use after free. Regards, Christian.
Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes
On Thu, Nov 11, 2021 at 04:10:41PM -0500, Harry Wentland wrote:
>
>
> On 2021-11-11 15:42, Shankar, Uma wrote:
> >
> >
> >> -Original Message-
> >> From: Ville Syrjälä
> >> Sent: Thursday, November 11, 2021 10:13 PM
> >> To: Harry Wentland
> >> Cc: Shankar, Uma ; intel-...@lists.freedesktop.org;
> >> dri-
> >> de...@lists.freedesktop.org; ppaala...@gmail.com; brian.star...@arm.com;
> >> sebast...@sebastianwick.net; shashank.sha...@amd.com
> >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range
> >> struct for
> >> HDR planes
> >>
> >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, Harry Wentland wrote:
> >>>
> >>>
> >>> On 2021-09-06 17:38, Uma Shankar wrote:
> Define the structure with XE_LPD degamma lut ranges. HDR and SDR
> planes have different capabilities, implemented respective structure
> for the HDR planes.
>
> Signed-off-by: Uma Shankar
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 52
> ++
> 1 file changed, 52 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index afcb4bf3826c..6403bd74324b 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -2092,6 +2092,58 @@ static void icl_read_luts(struct intel_crtc_state
> >> *crtc_state)
> }
> }
>
> + /* FIXME input bpc? */
> +__maybe_unused
> +static const struct drm_color_lut_range d13_degamma_hdr[] = {
> +/* segment 1 */
> +{
> +.flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_NON_DECREASING),
> +.count = 128,
> +.input_bpc = 24, .output_bpc = 16,
> +.start = 0, .end = (1 << 24) - 1,
> +.min = 0, .max = (1 << 24) - 1,
> +},
> +/* segment 2 */
> +{
> +.flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_REUSE_LAST |
> + DRM_MODE_LUT_NON_DECREASING),
> +.count = 1,
> +.input_bpc = 24, .output_bpc = 16,
> +.start = (1 << 24) - 1, .end = 1 << 24,
> +.min = 0, .max = (1 << 27) - 1,
> +},
> +/* Segment 3 */
> +{
> +.flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_REUSE_LAST |
> + DRM_MODE_LUT_NON_DECREASING),
> +.count = 1,
> +.input_bpc = 24, .output_bpc = 16,
> +.start = 1 << 24, .end = 3 << 24,
> +.min = 0, .max = (1 << 27) - 1,
> +},
> +/* Segment 4 */
> +{
> +.flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_REUSE_LAST |
> + DRM_MODE_LUT_NON_DECREASING),
> +.count = 1,
> +.input_bpc = 24, .output_bpc = 16,
> +.start = 3 << 24, .end = 7 << 24,
> +.min = 0, .max = (1 << 27) - 1,
> +},
> +};
> >>>
> >>> If I understand this right, userspace would need this definition in
> >>> order to populate the degamma blob. Should this sit in a UAPI header?
> >
> > Hi Harry, Pekka and Ville,
> > Sorry for being a bit late on the replies, got side tracked with various
> > issues.
> > I am back on this. Apologies for delay.
> >
> >> My original idea (not sure it's fully realized in this series) is to have
> >> a new
> >> GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum
> >> value points to a kernel provided blob that contains one of these LUT
> >> descriptors.
> >> Userspace can then query them dynamically and pick the best one for its
> >> current use
> >> case.
> >
> > We have this as part of the series Ville. Patch 3 of this series creates a
> > DEGAMMA_MODE
> > property just for this. With that userspace can just query the blob_id's
> > and will get the
> > various degamma mode possible and the respective segment and lut
> > distributions.
> >
> > This will be generic, so for userspace it should just be able to query this
> > and parse and get
> > the lut di
Re: [PATCH v6 3/8] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema
On Thu, Nov 11, 2021 at 08:57:26AM -0600, Rob Herring wrote: > On Wed, 10 Nov 2021 20:43:28 +0100, H. Nikolaus Schaller wrote: > > From: Sam Ravnborg > > > > Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. > > Based on .txt binding from Zubair Lutfullah Kakakhel > > > > We also add add generic ddc-i2c-bus to synopsys,dw-hdmi.yaml > > > > Signed-off-by: Sam Ravnborg > > Signed-off-by: H. Nikolaus Schaller > > Cc: Rob Herring > > Cc: devicet...@vger.kernel.org > > --- > > .../display/bridge/synopsys,dw-hdmi.yaml | 3 + > > .../bindings/display/ingenic-jz4780-hdmi.yaml | 76 +++ This goes in display/bridge/. And use compatible string for the file name: ingenic,jz4780-hdmi.yaml > > 2 files changed, 79 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml > > > > 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/ingenic-jz4780-hdmi.yaml:36:5: > [warning] wrong indentation: expected 2 but found 4 (indentation) > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dt.yaml: > hdmi@1018: 'clock-names', 'ddc-i2c-bus', 'interrupt-parent', > 'interrupts', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+' > From schema: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml I think you need to use 'unevaluatedPropertes' instead of 'additionalProperties' if you are getting these properties from synopsys,dw-hdmi.yaml. Rob
Re: [PATCH v1 2/2] drm/tegra: Use drm_dp_aux_register_ddc/chardev() helpers
12.11.2021 13:52, Thierry Reding пишет:
> On Tue, Nov 09, 2021 at 05:39:02PM +0300, Dmitry Osipenko wrote:
>> 09.11.2021 17:17, Dmitry Osipenko пишет:
>>> 09.11.2021 17:08, Dmitry Osipenko пишет:
> +static void host1x_drm_dev_deinit(struct host1x_device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
>>>
>>> Nah, that should cause deadlock. This ad-hoc is too lame.
>>
>> Actually, there is no problem here as I see now. The host1x driver
>> populates DT nodes after host1x_register() [1], meaning that Host1x DRM
>> will be always inited first.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v5.15/source/drivers/gpu/host1x/dev.c#L475
>>
>> Still I'm not a fan of the ad-hoc solution.
>
> Could we not fix this by making the panel "hot-pluggable"? I don't think
> there's anything inherent to the driver that would prevent doing so. The
> original reason for why things are as intertwined as they are now is
> because back at the time deferred framebuffer creation didn't exist. In
> fact I added deferred framebuffer support with Daniel's help precisely
> to fix a similar issue for things like HDMI and DP.
I don't understand what do you mean by "hot-pluggable", panel is static.
Please either clarify more or type the patch.
Keep in mind that fix should be simple and portable because stable
kernels are wrecked.
> With HDMI and DP it's slightly less critical, because a lack of mode
> support would've created a 1024x768 framebuffer, which most HDMI/DP
> monitors support. However, with panels we need to ensure that the exact
> mode from the panel is used to create the framebuffer, so it can only be
> created when the panel is available.
>
> But, given that deferred framebuffer creation works now (which allows
> the framebuffer console to show up at the preferred resolution for HDMI
> and DP), even if a monitor is attached only after the driver has probed
> already, we should be able to make something like that work with panels
> as well.
BTW, I see now that DPAUX I2C transfer helper may access
aux->drm_device. Hence v1 patch isn't correct anyways.
For now I'll try to test more the ad-hoc variant with Thomas and send it
as v2 if we won't have a better solution.
Re: Questions about KMS flip
On 2021-11-12 15:29, Michel Dänzer wrote: > On 2021-11-12 13:47, Christian König wrote: >> >> Anyway this unfortunately turned out to be work for Harray and Nicholas. In >> detail it's about this bug report here: >> https://bugzilla.kernel.org/show_bug.cgi?id=214621 >> >> Lang was able to reproduce the issue and narrow it down to the pin in >> amdgpu_display_crtc_page_flip_target(). >> >> In other words we somehow have an unbalanced pinning of the scanout buffer >> in DC. > > DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The corresponding > pin with DC would be in dm_plane_helper_prepare_fb, paired with the unpin in > dm_plane_helper_cleanup_fb. > > > With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired with > the unpin in dm_plane_helper_cleanup_fb This should say amdgpu_display_unpin_work_func. > & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by if > (!adev->enable_virtual_display), but the unpins seem unconditional. So could > this be about virtual display, and the problem is actually trying to unpin a > BO that was never pinned? > > -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: Questions about KMS flip
On 2021-11-12 13:47, Christian König wrote: > > Anyway this unfortunately turned out to be work for Harray and Nicholas. In > detail it's about this bug report here: > https://bugzilla.kernel.org/show_bug.cgi?id=214621 > > Lang was able to reproduce the issue and narrow it down to the pin in > amdgpu_display_crtc_page_flip_target(). > > In other words we somehow have an unbalanced pinning of the scanout buffer in > DC. DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The corresponding pin with DC would be in dm_plane_helper_prepare_fb, paired with the unpin in dm_plane_helper_cleanup_fb. With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired with the unpin in dm_plane_helper_cleanup_fb & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by if (!adev->enable_virtual_display), but the unpins seem unconditional. So could this be about virtual display, and the problem is actually trying to unpin a BO that was never pinned? -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On 2021-11-12 13:23:36, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:
> > On 2021-11-12 12:12:38, Daniel Thompson wrote:
> > > On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > > > The length of qcom,enabled-strings as property array is enough to
> > > > determine the number of strings to be enabled, without needing to set
> > > > qcom,num-strings to override the default number of strings when less
> > > > than the default (which is also the maxium) is provided in DT.
> > > >
> > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for
> > > > WLED3")
> > > > Signed-off-by: Marijn Suijten
> > > > Reviewed-by: AngeloGioacchino Del Regno
> > > >
> > > > ---
> > > > drivers/video/backlight/qcom-wled.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > > b/drivers/video/backlight/qcom-wled.c
> > > > index c5232478a343..9bfbf601762a 100644
> > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > > > return -EINVAL;
> > > > }
> > > > }
> > > > +
> > > > + cfg->num_strings = string_len;
> > >
> > > I still don't really understand why this wants to be a separate patch.
> >
> > I'm viewing this as a separate issue, and this makes it easier to
> > document the change in a loose commit.
> >
> > > The warning text emitted by the previous patch (whatever text we agree
> > > on) will be nonsense until this patch is applied.
> > >
> > > If this patch cannot appear before the warning is introduces then there
> > > is no correct order for patches 4 and 5 (which implies they should be the
> > > same patch).
> >
> > Agreed, this is a weird way of doing things in v2 - the error message is
> > printed yet the length of qcom,enabled-strings is always ignored before
> > this patch.
> >
> > If we were to reorder patch 5 before patch 4 that should also
> > temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
> > this `if` so that `qcom,num-strings` remains the definitive way to
> > set/override length. That's doable, and makes it easier to read patch 4
> > as that bit of code will be replaced by of_property_read_u32 on that
> > exact line. Let me know which method you prefer.
>
> Personally I would just squash them together. There are no redundant
> values in the DT that could be fixed until we can use the string_len
> to set num_strings.
Reordering this patch before patch 4 in the way described above should
allow just that, except that no warnings will be given for ambiguity
until patch 4 is applied after that - which is weird given that that
patch only intends the off-by-one error. Perhaps we should keep the
order as it is, but add the ambiguity warning in this patch instead.
That means we have one patch to fix the off-by-one first, and another
that allows qcom,num-strings to provide a default for num_strings. I
guess that's better to keep separated?
- Marijn
[PATCH 4/7] agp/ati: Return error from ati_create_page_map()
Fix the compiler warning
drivers/char/agp/ati-agp.c: In function 'ati_create_page_map':
drivers/char/agp/ati-agp.c:58:16: warning: variable 'err' set but not used
[-Wunused-but-set-variable]
58 | int i, err = 0;
by returing the error to the caller.
Signed-off-by: Thomas Zimmermann
---
drivers/char/agp/ati-agp.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/char/agp/ati-agp.c b/drivers/char/agp/ati-agp.c
index 857b37141a07..785cc1ecf4e0 100644
--- a/drivers/char/agp/ati-agp.c
+++ b/drivers/char/agp/ati-agp.c
@@ -55,7 +55,7 @@ static struct _ati_generic_private {
static int ati_create_page_map(struct ati_page_map *page_map)
{
- int i, err = 0;
+ int i, err;
page_map->real = (unsigned long *) __get_free_page(GFP_KERNEL);
if (page_map->real == NULL)
@@ -63,6 +63,8 @@ static int ati_create_page_map(struct ati_page_map *page_map)
set_memory_uc((unsigned long)page_map->real, 1);
err = map_page_into_agp(virt_to_page(page_map->real));
+ if (err)
+ goto err_free_page;
page_map->remapped = page_map->real;
for (i = 0; i < PAGE_SIZE / sizeof(unsigned long); i++) {
@@ -71,6 +73,10 @@ static int ati_create_page_map(struct ati_page_map *page_map)
}
return 0;
+
+err_free_page:
+ free_page((unsigned long)page_map->real);
+ return err;
}
--
2.33.1
[PATCH 3/7] agp: Documentation fixes
Fix compiler warnings drivers/char/agp/backend.c:68: warning: Function parameter or member 'pdev' not described in 'agp_backend_acquire' drivers/char/agp/backend.c:93: warning: Function parameter or member 'bridge' not described in 'agp_backend_release' by adding the necessary documentation. Signed-off-by: Thomas Zimmermann --- drivers/char/agp/backend.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c index 004a3ce8ba72..0e19c600db53 100644 --- a/drivers/char/agp/backend.c +++ b/drivers/char/agp/backend.c @@ -62,6 +62,7 @@ EXPORT_SYMBOL(agp_find_bridge); /** * agp_backend_acquire - attempt to acquire an agp backend. + * @pdev: the PCI device * */ struct agp_bridge_data *agp_backend_acquire(struct pci_dev *pdev) @@ -83,6 +84,7 @@ EXPORT_SYMBOL(agp_backend_acquire); /** * agp_backend_release - release the lock on the agp backend. + * @bridge: the AGP backend to release * * The caller must insure that the graphics aperture translation table * is read for use by another entity. -- 2.33.1
[PATCH 6/7] agp/sworks: Remove unused variable 'current_size'
Fix the compiler warning
drivers/char/agp/sworks-agp.c: In function 'serverworks_configure':
drivers/char/agp/sworks-agp.c:265:37: warning: variable 'current_size' set
but not used [-Wunused-but-set-variable]
265 | struct aper_size_info_lvl2 *current_size;
by removing the variable.
Signed-off-by: Thomas Zimmermann
---
drivers/char/agp/sworks-agp.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/char/agp/sworks-agp.c b/drivers/char/agp/sworks-agp.c
index b15d3d4f71d5..b91da5998dd7 100644
--- a/drivers/char/agp/sworks-agp.c
+++ b/drivers/char/agp/sworks-agp.c
@@ -262,13 +262,10 @@ static void serverworks_tlbflush(struct agp_memory *temp)
static int serverworks_configure(void)
{
- struct aper_size_info_lvl2 *current_size;
u32 temp;
u8 enable_reg;
u16 cap_reg;
- current_size = A_SIZE_LVL2(agp_bridge->current_size);
-
/* Get the memory mapped registers */
pci_read_config_dword(agp_bridge->dev, serverworks_private.mm_addr_ofs,
&temp);
temp = (temp & PCI_BASE_ADDRESS_MEM_MASK);
--
2.33.1
[PATCH 5/7] agp/nvidia: Ignore value returned by readl()
Fix the compiler warning
drivers/char/agp/nvidia-agp.c: In function 'nvidia_tlbflush':
drivers/char/agp/nvidia-agp.c:264:22: warning: variable 'temp' set but not
used [-Wunused-but-set-variable]
264 | u32 wbc_reg, temp;
by removing the unused variable. The affected readl() is only
required for flushing caches, but the returned value is not of
interest.
Signed-off-by: Thomas Zimmermann
---
drivers/char/agp/nvidia-agp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/agp/nvidia-agp.c b/drivers/char/agp/nvidia-agp.c
index f78e756157db..437b3581cbe5 100644
--- a/drivers/char/agp/nvidia-agp.c
+++ b/drivers/char/agp/nvidia-agp.c
@@ -261,7 +261,7 @@ static int nvidia_remove_memory(struct agp_memory *mem,
off_t pg_start, int type
static void nvidia_tlbflush(struct agp_memory *mem)
{
unsigned long end;
- u32 wbc_reg, temp;
+ u32 wbc_reg;
int i;
/* flush chipset */
@@ -283,9 +283,9 @@ static void nvidia_tlbflush(struct agp_memory *mem)
/* flush TLB entries */
for (i = 0; i < 32 + 1; i++)
- temp = readl(nvidia_private.aperture+(i * PAGE_SIZE /
sizeof(u32)));
+ (void)readl(nvidia_private.aperture+(i * PAGE_SIZE /
sizeof(u32)));
for (i = 0; i < 32 + 1; i++)
- temp = readl(nvidia_private.aperture+(i * PAGE_SIZE /
sizeof(u32)));
+ (void)readl(nvidia_private.aperture+(i * PAGE_SIZE /
sizeof(u32)));
}
--
2.33.1
[PATCH 2/7] agp: Include "compat_ioctl.h" where necessary
Fix compiler warnings like drivers/char/agp/frontend.c:46:20: warning: no previous prototype for 'agp_find_mem_by_key' [-Wmissing-prototypes] 46 | struct agp_memory *agp_find_mem_by_key(int key) by including the compat_ioctl.h in the source file. Signed-off-by: Thomas Zimmermann --- drivers/char/agp/frontend.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c index 6802a6bbf0f2..321118a9cfa5 100644 --- a/drivers/char/agp/frontend.c +++ b/drivers/char/agp/frontend.c @@ -39,7 +39,9 @@ #include #include #include + #include "agp.h" +#include "compat_ioctl.h" struct agp_front_data agp_fe; -- 2.33.1
[PATCH 7/7] agp/via: Remove unused variable 'current_size'
Fix the compiler warning
drivers/char/agp/via-agp.c: In function 'via_configure_agp3':
drivers/char/agp/via-agp.c:131:35: warning: variable 'current_size' set but
not used [-Wunused-but-set-variable]
131 | struct aper_size_info_16 *current_size;
by removing the variable.
Signed-off-by: Thomas Zimmermann
---
drivers/char/agp/via-agp.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/char/agp/via-agp.c b/drivers/char/agp/via-agp.c
index 87a92a044570..dc594f4eca38 100644
--- a/drivers/char/agp/via-agp.c
+++ b/drivers/char/agp/via-agp.c
@@ -128,9 +128,6 @@ static int via_fetch_size_agp3(void)
static int via_configure_agp3(void)
{
u32 temp;
- struct aper_size_info_16 *current_size;
-
- current_size = A_SIZE_16(agp_bridge->current_size);
/* address to map to */
agp_bridge->gart_bus_addr = pci_bus_address(agp_bridge->dev,
--
2.33.1
[PATCH 1/7] agp: Remove trailing whitespaces
Trivial coding-style fix.
Signed-off-by: Thomas Zimmermann
---
drivers/char/agp/ati-agp.c| 2 +-
drivers/char/agp/frontend.c | 2 +-
drivers/char/agp/sworks-agp.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/agp/ati-agp.c b/drivers/char/agp/ati-agp.c
index 20bf5f78a362..857b37141a07 100644
--- a/drivers/char/agp/ati-agp.c
+++ b/drivers/char/agp/ati-agp.c
@@ -303,7 +303,7 @@ static int ati_insert_memory(struct agp_memory * mem,
for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr;
cur_gatt = GET_GATT(addr);
- writel(agp_bridge->driver->mask_memory(agp_bridge,
+ writel(agp_bridge->driver->mask_memory(agp_bridge,
page_to_phys(mem->pages[i]),
mem->type),
cur_gatt+GET_GATT_OFF(addr));
diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c
index 00ff5fcb808a..6802a6bbf0f2 100644
--- a/drivers/char/agp/frontend.c
+++ b/drivers/char/agp/frontend.c
@@ -1017,7 +1017,7 @@ static long agp_ioctl(struct file *file,
case AGPIOC_UNBIND:
ret_val = agpioc_unbind_wrap(curr_priv, (void __user *) arg);
break;
-
+
case AGPIOC_CHIPSET_FLUSH:
break;
}
diff --git a/drivers/char/agp/sworks-agp.c b/drivers/char/agp/sworks-agp.c
index f875970bda65..b15d3d4f71d5 100644
--- a/drivers/char/agp/sworks-agp.c
+++ b/drivers/char/agp/sworks-agp.c
@@ -350,7 +350,7 @@ static int serverworks_insert_memory(struct agp_memory *mem,
for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr;
cur_gatt = SVRWRKS_GET_GATT(addr);
- writel(agp_bridge->driver->mask_memory(agp_bridge,
+ writel(agp_bridge->driver->mask_memory(agp_bridge,
page_to_phys(mem->pages[i]), mem->type),
cur_gatt+GET_GATT_OFF(addr));
}
--
2.33.1
[PATCH 0/7] agp: Various minor fixes
Fix a number of compiler warnings in the AGP drivers. No functional changes. Thomas Zimmermann (7): agp: Remove trailing whitespaces agp: Include "compat_ioctl.h" where necessary agp: Documentation fixes agp/ati: Return error from ati_create_page_map() agp/nvidia: Ignore value returned by readl() agp/sworks: Remove unused variable 'current_size' agp/via: Remove unused variable 'current_size' drivers/char/agp/ati-agp.c| 10 -- drivers/char/agp/backend.c| 2 ++ drivers/char/agp/frontend.c | 4 +++- drivers/char/agp/nvidia-agp.c | 6 +++--- drivers/char/agp/sworks-agp.c | 5 + drivers/char/agp/via-agp.c| 3 --- 6 files changed, 17 insertions(+), 13 deletions(-) -- 2.33.1
Re: [Intel-gfx] [PATCH] drm/i915/execlists: Weak parallel submission support for execlists
On 11/11/2021 16:49, Matthew Brost wrote:
On Mon, Nov 01, 2021 at 10:35:09AM +, Tvrtko Ursulin wrote:
On 27/10/2021 21:10, Matthew Brost wrote:
On Wed, Oct 27, 2021 at 01:04:49PM -0700, John Harrison wrote:
On 10/27/2021 12:17, Matthew Brost wrote:
On Tue, Oct 26, 2021 at 02:58:00PM -0700, John Harrison wrote:
On 10/20/2021 14:47, Matthew Brost wrote:
A weak implementation of parallel submission (multi-bb execbuf IOCTL) for
execlists. Doing as little as possible to support this interface for
execlists - basically just passing submit fences between each request
generated and virtual engines are not allowed. This is on par with what
is there for the existing (hopefully soon deprecated) bonding interface.
We perma-pin these execlists contexts to align with GuC implementation.
v2:
(John Harrison)
- Drop siblings array as num_siblings must be 1
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 10 +++--
drivers/gpu/drm/i915/gt/intel_context.c | 4 +-
.../drm/i915/gt/intel_execlists_submission.c | 44 ++-
drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 -
5 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fb33d0322960..35e87a7d0ea9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -570,10 +570,6 @@ set_proto_ctx_engines_parallel_submit(struct
i915_user_extension __user *base,
struct intel_engine_cs **siblings = NULL;
intel_engine_mask_t prev_mask;
- /* FIXME: This is NIY for execlists */
- if (!(intel_uc_uses_guc_submission(&i915->gt.uc)))
- return -ENODEV;
-
if (get_user(slot, &ext->engine_index))
return -EFAULT;
@@ -583,6 +579,12 @@ set_proto_ctx_engines_parallel_submit(struct
i915_user_extension __user *base,
if (get_user(num_siblings, &ext->num_siblings))
return -EFAULT;
+ if (!intel_uc_uses_guc_submission(&i915->gt.uc) && num_siblings != 1) {
+ drm_dbg(&i915->drm, "Only 1 sibling (%d) supported in non-GuC
mode\n",
+ num_siblings);
+ return -EINVAL;
+ }
+
if (slot >= set->num_engines) {
drm_dbg(&i915->drm, "Invalid placement value, %d >= %d\n",
slot, set->num_engines);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 5634d14052bc..1bec92e1d8e6 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -79,7 +79,8 @@ static int intel_context_active_acquire(struct intel_context
*ce)
__i915_active_acquire(&ce->active);
- if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine))
+ if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine) ||
+ intel_context_is_parallel(ce))
return 0;
/* Preallocate tracking nodes */
@@ -563,7 +564,6 @@ void intel_context_bind_parent_child(struct intel_context
*parent,
* Callers responsibility to validate that this function is used
* correctly but we use GEM_BUG_ON here ensure that they do.
*/
- GEM_BUG_ON(!intel_engine_uses_guc(parent->engine));
GEM_BUG_ON(intel_context_is_pinned(parent));
GEM_BUG_ON(intel_context_is_child(parent));
GEM_BUG_ON(intel_context_is_pinned(child));
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index bedb80057046..2865b422300d 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -927,8 +927,7 @@ static void execlists_submit_ports(struct intel_engine_cs
*engine)
static bool ctx_single_port_submission(const struct intel_context *ce)
{
- return (IS_ENABLED(CONFIG_DRM_I915_GVT) &&
- intel_context_force_single_submission(ce));
+ return intel_context_force_single_submission(ce);
I think this is actually going to break GVT.
Not so much this change here but the whole use of single submission outside
of GVT. It looks like the GVT driver overloads the single submission flag to
tag requests that it owns. If we start using that flag elsewhere when GVT is
active, I think that will cause much confusion within the GVT code.
The correct fix would be to create a new flag just for GVT usage alongside
the single submission one. GVT would then set both but only check for its
own private flag. The parallel code would obviously only set the existing
single submission flag.
Ok, see below.
}
static bool can_merge_ctx(const struct intel_context *prev,
@@ -2598,6 +2597,46 @@ static void execlists_context_cancel_reques
Re: [PATCH] drm/i915: Use per device iommu check
On 12/11/2021 00:58, Lu Baolu wrote:
On 11/11/21 11:18 PM, Tvrtko Ursulin wrote:
On 10/11/2021 14:37, Robin Murphy wrote:
On 2021-11-10 14:11, Tvrtko Ursulin wrote:
On 10/11/2021 12:35, Lu Baolu wrote:
On 2021/11/10 20:08, Tvrtko Ursulin wrote:
On 10/11/2021 12:04, Lu Baolu wrote:
On 2021/11/10 17:30, Tvrtko Ursulin wrote:
On 10/11/2021 07:12, Lu Baolu wrote:
Hi Tvrtko,
On 2021/11/9 20:17, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
On igfx + dgfx setups, it appears that intel_iommu=igfx_off
option only
disables the igfx iommu. Stop relying on global
intel_iommu_gfx_mapped
and probe presence of iommu domain per device to accurately
reflect its
status.
Signed-off-by: Tvrtko Ursulin
Cc: Lu Baolu
---
Baolu, is my understanding here correct? Maybe I am confused
by both
intel_iommu_gfx_mapped and dmar_map_gfx being globals in the
intel_iommu
driver. But it certainly appears the setup can assign some
iommu ops (and
assign the discrete i915 to iommu group) when those two are
set to off.
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index e967cd08f23e..9fb38a54f1fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1763,26 +1763,27 @@ static inline bool run_as_guest(void)
#define HAS_D12_PLANE_MINIMIZATION(dev_priv)
(IS_ROCKETLAKE(dev_priv) || \
IS_ALDERLAKE_S(dev_priv))
-static inline bool intel_vtd_active(void)
+static inline bool intel_vtd_active(struct drm_i915_private
*i915)
{
-#ifdef CONFIG_INTEL_IOMMU
- if (intel_iommu_gfx_mapped)
+ if (iommu_get_domain_for_dev(i915->drm.dev))
return true;
-#endif
/* Running as a guest, we assume the host is enforcing
VT'd */
return run_as_guest();
}
Have you verified this change? I am afraid that
iommu_get_domain_for_dev() always gets a valid iommu domain even
intel_iommu_gfx_mapped == 0.
Yes it seems to work as is:
default:
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: enabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
intel_iommu=igfx_off:
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: disabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
On my system dri device 0 is integrated graphics and 1 is discrete.
The drm device 0 has a dedicated iommu. When the user request
igfx not
mapped, the VT-d implementation will turn it off to save power.
But for
shared iommu, you definitely will get it enabled.
Sorry I am not following, what exactly do you mean? Is there a
platform with integrated graphics without a dedicated iommu, in
which case intel_iommu=igfx_off results in intel_iommu_gfx_mapped
== 0 and iommu_get_domain_for_dev returning non-NULL?
Your code always work for an igfx with a dedicated iommu. This
might be
always true on today's platforms. But from driver's point of view, we
should not make such assumption.
For example, if the iommu implementation decides not to turn off the
graphic iommu (perhaps due to some hw quirk or for graphic
virtualization), your code will be broken.
If I got it right, this would go back to your earlier recommendation
to have the check look like this:
static bool intel_vtd_active(struct drm_i915_private *i915)
{
struct iommu_domain *domain;
domain = iommu_get_domain_for_dev(i915->drm.dev);
if (domain && (domain->type & __IOMMU_DOMAIN_PAGING))
return true;
...
This would be okay as a first step?
Elsewhere in the thread Robin suggested looking at the dec->dma_ops
and comparing against iommu_dma_ops. These two solution would be
effectively the same?
Effectively, yes. See iommu_setup_dma_ops() - the only way to end up
with iommu_dma_ops is if a managed translation domain is present; if
the IOMMU is present but the default domain type has been set to
passthrough (either globally or forced for the given device) it will
do nothing and leave you with dma-direct, while if the IOMMU has been
ignored entirely then it should never even be called. Thus it neatly
encapsulates what you're after here.
One concern I have is whether the pass-through mode truly does nothing
or addresses perhaps still go through the dmar hardware just with no
translation?
Pass-through mode means the latter.
If latter then most like for like change is actually exactly what the
first version of my patch did. That is replace intel_iommu_gfx_mapped
with a plain non-NULL check on iommu_get_domain_for_dev.
Depends on what you want here,
#1) the graphic device works in iommu pass-through mode
- device have an iommu
- but iommu does no translation
- the dma transactions go through iommu with the same destination
memory address specified by the device;
Do you have any indications of the slowdown this adds?
#2) the graphic device works without a system iomm
Re: [PATCH][next] drm/i915: make array states static const
On Wed, 15 Sep 2021, Colin King wrote:
> From: Colin Ian King
>
> Don't populate the read-only array states on the stack but instead it
> static. Also makes the object code smaller.
Finally pushed, sorry for the delay.
BR,
Jani.
>
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index cce1a926fcc1..a60710348613 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -893,7 +893,7 @@ static u32
> sanitize_target_dc_state(struct drm_i915_private *dev_priv,
>u32 target_dc_state)
> {
> - u32 states[] = {
> + static const u32 states[] = {
> DC_STATE_EN_UPTO_DC6,
> DC_STATE_EN_UPTO_DC5,
> DC_STATE_EN_DC3CO,
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v5 4/6] drm: Decouple nomodeset from CONFIG_VGA_CONSOLE
On Fri, 12 Nov 2021, Javier Martinez Canillas wrote:
> This relationship was only for historical reasons and the nomodeset option
> should be available even on platforms that don't enable CONFIG_VGA_CONSOLE.
>
> Suggested-by: Thomas Zimmermann
> Signed-off-by: Javier Martinez Canillas
> Acked-by: Thomas Zimmermann
> Acked-by: Jani Nikula
> Acked-by: Pekka Paalanen
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/Kconfig | 6 ++
> drivers/gpu/drm/Makefile | 2 +-
> include/drm/drm_drv.h| 4
> 3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git drivers/gpu/drm/Kconfig drivers/gpu/drm/Kconfig
> index fb144617055b..483d534eb074 100644
> --- drivers/gpu/drm/Kconfig
> +++ drivers/gpu/drm/Kconfig
> @@ -8,6 +8,7 @@
> menuconfig DRM
> tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI
> support)"
> depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
> + select DRM_NOMODESET
> select DRM_PANEL_ORIENTATION_QUIRKS
> select HDMI
> select FB_CMDLINE
> @@ -493,6 +494,11 @@ config DRM_EXPORT_FOR_TESTS
> config DRM_PANEL_ORIENTATION_QUIRKS
> tristate
>
> +# Separate option because nomodeset parameter is global and expected built-in
> +config DRM_NOMODESET
> + bool
> + default n
> +
> config DRM_LIB_RANDOM
> bool
> default n
> diff --git drivers/gpu/drm/Makefile drivers/gpu/drm/Makefile
> index c74810c285af..fa16d3e0bbdc 100644
> --- drivers/gpu/drm/Makefile
> +++ drivers/gpu/drm/Makefile
> @@ -33,7 +33,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
> drm_privacy_screen_x86.
>
> obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>
> -obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
> +obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
>
> drm_cma_helper-y := drm_gem_cma_helper.o
> obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
> diff --git include/drm/drm_drv.h include/drm/drm_drv.h
> index 89e26a732175..da0c836fe8e1 100644
> --- include/drm/drm_drv.h
> +++ include/drm/drm_drv.h
> @@ -601,10 +601,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct
> drm_device *dev)
>
> int drm_dev_set_unique(struct drm_device *dev, const char *name);
>
> -#ifdef CONFIG_VGA_CONSOLE
> extern bool drm_firmware_drivers_only(void);
I'm not bikeshedding this series anymore, but please follow up with a
patch removing the extern keyword. It's useless.
BR,
Jani.
> -#else
> -static inline bool drm_firmware_drivers_only(void) { return false; }
> -#endif
>
> #endif
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/i915: Use per device iommu check
On 12/11/2021 00:53, Lu Baolu wrote:
On 11/11/21 11:06 PM, Tvrtko Ursulin wrote:
On 10/11/2021 12:35, Lu Baolu wrote:
On 2021/11/10 20:08, Tvrtko Ursulin wrote:
On 10/11/2021 12:04, Lu Baolu wrote:
On 2021/11/10 17:30, Tvrtko Ursulin wrote:
On 10/11/2021 07:12, Lu Baolu wrote:
Hi Tvrtko,
On 2021/11/9 20:17, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
On igfx + dgfx setups, it appears that intel_iommu=igfx_off
option only
disables the igfx iommu. Stop relying on global
intel_iommu_gfx_mapped
and probe presence of iommu domain per device to accurately
reflect its
status.
Signed-off-by: Tvrtko Ursulin
Cc: Lu Baolu
---
Baolu, is my understanding here correct? Maybe I am confused by
both
intel_iommu_gfx_mapped and dmar_map_gfx being globals in the
intel_iommu
driver. But it certainly appears the setup can assign some iommu
ops (and
assign the discrete i915 to iommu group) when those two are set
to off.
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index e967cd08f23e..9fb38a54f1fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1763,26 +1763,27 @@ static inline bool run_as_guest(void)
#define HAS_D12_PLANE_MINIMIZATION(dev_priv)
(IS_ROCKETLAKE(dev_priv) || \
IS_ALDERLAKE_S(dev_priv))
-static inline bool intel_vtd_active(void)
+static inline bool intel_vtd_active(struct drm_i915_private *i915)
{
-#ifdef CONFIG_INTEL_IOMMU
- if (intel_iommu_gfx_mapped)
+ if (iommu_get_domain_for_dev(i915->drm.dev))
return true;
-#endif
/* Running as a guest, we assume the host is enforcing VT'd */
return run_as_guest();
}
Have you verified this change? I am afraid that
iommu_get_domain_for_dev() always gets a valid iommu domain even
intel_iommu_gfx_mapped == 0.
Yes it seems to work as is:
default:
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: enabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
intel_iommu=igfx_off:
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: disabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
On my system dri device 0 is integrated graphics and 1 is discrete.
The drm device 0 has a dedicated iommu. When the user request igfx not
mapped, the VT-d implementation will turn it off to save power. But
for
shared iommu, you definitely will get it enabled.
Sorry I am not following, what exactly do you mean? Is there a
platform with integrated graphics without a dedicated iommu, in
which case intel_iommu=igfx_off results in intel_iommu_gfx_mapped ==
0 and iommu_get_domain_for_dev returning non-NULL?
Your code always work for an igfx with a dedicated iommu. This might be
always true on today's platforms. But from driver's point of view, we
should not make such assumption.
For example, if the iommu implementation decides not to turn off the
graphic iommu (perhaps due to some hw quirk or for graphic
virtualization), your code will be broken.
I tried your suggestion (checking for __IOMMU_DOMAIN_PAGING) and it
works better, however I have observed one odd behaviour (for me at
least).
In short - why does the DMAR mode for the discrete device change
depending on igfx_off parameter?
Consider the laptop has these two graphics cards:
# cat /sys/kernel/debug/dri/0/name
i915 dev=:00:02.0 unique=:00:02.0 # integrated
# cat /sys/kernel/debug/dri/1/name
i915 dev=:03:00.0 unique=:03:00.0 # discrete
Booting with different options:
===
default / intel_iommu=on
# cat /sys/class/iommu/dmar0/devices/:00:02.0/iommu_group/type
DMA-FQ
# cat /sys/class/iommu/dmar2/devices/:03:00.0/iommu_group/type
DMA-FQ
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: enabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
All good.
intel_iommu=igfx_off
## no dmar0 in sysfs
# cat /sys/class/iommu/dmar2/devices/:03:00.0/iommu_group/type
identity
Unexpected!?
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: disabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: disabled # At least
the i915 patch detects it correctly.
intel_iommu=off
---
## no dmar0 in sysfs
## no dmar2 in sysfs
# grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
/sys/kernel/debug/dri/0/i915_capabilities:iommu: disabled
/sys/kernel/debug/dri/1/i915_capabilities:iommu: disabled
All good.
The fact discrete graphics changes from translated to pass-through
when igfx_off is set is surprising to me. Is this a bug?
The existing VT-d implementation doesn't distinguish igfx from dgfx. It
only checks whether the device is of a display class:
#define IS_GFX_DEVICE(pdev) ((pdev->class >> 16) == PCI_BASE_CLA
[PATCH v5 6/6] drm: Make the nomodeset message less sensational
The message printed when nomodeset is present in the kernel command line
makes it look as if the parameter must never be used and it's a bad idea.
But there are valid reasons to use this parameter and the message should
not imply otherwise. Change the text to be more accurate and restrained.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
Acked-by: Jani Nikula
Acked-by: Pekka Paalanen
---
(no changes since v4)
Changes in v4:
- Don't mention DRM drivers in the kernel message and instead explain
that only the system framebuffer will be available.
drivers/gpu/drm/drm_nomodeset.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git drivers/gpu/drm/drm_nomodeset.c drivers/gpu/drm/drm_nomodeset.c
index 287edfb18b5d..f3978d5bd3a1 100644
--- drivers/gpu/drm/drm_nomodeset.c
+++ drivers/gpu/drm/drm_nomodeset.c
@@ -15,9 +15,7 @@ static int __init disable_modeset(char *str)
{
drm_nomodeset = true;
- pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
- pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
- pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+ pr_warn("Booted with the nomodeset parameter. Only the system
framebuffer will be available\n");
return 1;
}
--
2.33.1
[PATCH v5 5/6] Documentation/admin-guide: Document nomodeset kernel parameter
The nomodeset kernel command line parameter is not documented. Its name is quite vague and is not intuitive what's the behaviour when it is set. Document in kernel-parameters.txt what actually happens when nomodeset is used. That way, users could know if they want to enable this option. Signed-off-by: Javier Martinez Canillas Acked-by: Thomas Zimmermann Acked-by: Jani Nikula Acked-by: Pekka Paalanen --- (no changes since v4) Changes in v4: - Don't mention the simpledrm driver and instead explain in high level terms what the nomodeset option is about. Documentation/admin-guide/kernel-parameters.txt | 7 +++ 1 file changed, 7 insertions(+) diff --git Documentation/admin-guide/kernel-parameters.txt Documentation/admin-guide/kernel-parameters.txt index 91ba391f9b32..f6434aff943d 100644 --- Documentation/admin-guide/kernel-parameters.txt +++ Documentation/admin-guide/kernel-parameters.txt @@ -3521,6 +3521,13 @@ shutdown the other cpus. Instead use the REBOOT_VECTOR irq. + nomodeset Disable kernel modesetting. DRM drivers will not perform + display-mode changes or accelerated rendering. Only the + system framebuffer will be available for use if this was + set-up by the firmware or boot loader. + + Useful as fallback, or for testing and debugging. + nomoduleDisable module load nopat [X86] Disable PAT (page attribute table extension of -- 2.33.1
[PATCH v5 3/6] drm: Move nomodeset kernel parameter to the DRM subsystem
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it.
Let's move the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename it to drm_firmware_drivers_only() and
make it return true if "nomodeset" was used and false otherwise. This is
a better description of the condition that the drivers are testing for.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
Acked-by: Jani Nikula
Acked-by: Pekka Paalanen
---
Changes in v5:
- Rename drm_get_modeset() to drm_firmware_drivers_only().
Changes in v3:
- Drop the drm_drv_enabled() function and just call to drm_get_modeset().
- Make drm_get_modeset() just a getter function and don't return an error.
- Move independent cleanups in drivers as separate preparatory patches.
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
- Squash patch to add drm_drv_enabled() and make drivers use it.
- Make the drivers changes before moving nomodeset logic to DRM.
- Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
- Remove debug and error messages in drivers.
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
drivers/gpu/drm/ast/ast_drv.c | 3 +--
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 4 ++--
drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +--
drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++--
drivers/gpu/drm/qxl/qxl_drv.c | 3 +--
drivers/gpu/drm/radeon/radeon_drv.c | 3 +--
drivers/gpu/drm/tiny/bochs.c| 3 +--
drivers/gpu/drm/tiny/cirrus.c | 4 ++--
drivers/gpu/drm/vboxvideo/vbox_drv.c| 3 +--
drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +--
drivers/video/console/vgacon.c | 21
include/drm/drm_drv.h | 5 +
include/linux/console.h | 6 --
17 files changed, 48 insertions(+), 51 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git drivers/gpu/drm/Makefile drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 100644
--- drivers/gpu/drm/Makefile
+++ drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 289d04999ced..fc761087c358 100644
--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
@@ -2514,7 +2513,7 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force())
+ if (drm_firmware_drivers_only())
return -EINVAL;
r = amdgpu_sync_init();
diff --git drivers/gpu/drm/ast/ast_drv.c drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..6d8613f6fe1c 100644
--- drivers/gpu/drm/ast/ast_drv.c
+++ drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
@@ -233,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
+ if (drm_firmware_drivers_only() && ast_modeset == -1)
return -EINVAL;
if (ast_modeset == 0)
diff --git drivers/gpu/drm/drm_nomodeset.c drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..287edfb18b5d
--- /dev/null
+++ drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool drm_nomodeset;
+
+bool drm_firmware_drivers_only(void)
+{
+ return drm_nomodeset;
+}
+EXPORT_SYMBOL(drm_firmware_drivers_only);
+
+static int __init disable_modeset(char *str)
+{
+ drm_nomodeset = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+
+ return 1;
+}
+
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git drivers/gpu/drm/i9
[PATCH v5 1/6] drm: Don't print messages if drivers are disabled due nomodeset
The nomodeset kernel parameter handler already prints a message that the
DRM drivers will be disabled, so there's no need for drivers to do that.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
Acked-by: Jani Nikula
Acked-by: Pekka Paalanen
---
(no changes since v1)
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +---
drivers/gpu/drm/radeon/radeon_drv.c | 8 ++--
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..289d04999ced 100644
--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2514,10 +2514,8 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+ if (vgacon_text_force())
return -EINVAL;
- }
r = amdgpu_sync_init();
if (r)
diff --git drivers/gpu/drm/radeon/radeon_drv.c
drivers/gpu/drm/radeon/radeon_drv.c
index b74cebca1f89..380adc61e71c 100644
--- drivers/gpu/drm/radeon/radeon_drv.c
+++ drivers/gpu/drm/radeon/radeon_drv.c
@@ -637,15 +637,11 @@ static struct pci_driver radeon_kms_pci_driver = {
static int __init radeon_module_init(void)
{
- if (vgacon_text_force() && radeon_modeset == -1) {
- DRM_INFO("VGACON disable radeon kernel modesetting.\n");
+ if (vgacon_text_force() && radeon_modeset == -1)
radeon_modeset = 0;
- }
- if (radeon_modeset == 0) {
- DRM_ERROR("No UMS support in radeon module!\n");
+ if (radeon_modeset == 0)
return -EINVAL;
- }
DRM_INFO("radeon kernel modesetting enabled.\n");
radeon_register_atpx_handler();
--
2.33.1
[PATCH v5 2/6] drm/vboxvideo: Drop CONFIG_VGA_CONSOLE guard to call vgacon_text_force()
It is already handled by the console.h macro since a stub inline function
is defined for vgacon_text_force() if CONFIG_VGA_CONSOLE is not set.
There's no need to have ifdefery in the driver when calling the function.
Signed-off-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
Acked-by: Jani Nikula
Acked-by: Pekka Paalanen
---
(no changes since v1)
drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 --
1 file changed, 2 deletions(-)
diff --git drivers/gpu/drm/vboxvideo/vbox_drv.c
drivers/gpu/drm/vboxvideo/vbox_drv.c
index a6c81af37345..fd7abb029c65 100644
--- drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -193,10 +193,8 @@ static const struct drm_driver driver = {
static int __init vbox_init(void)
{
-#ifdef CONFIG_VGA_CONSOLE
if (vgacon_text_force() && vbox_modeset == -1)
return -EINVAL;
-#endif
if (vbox_modeset == 0)
return -EINVAL;
--
2.33.1
[PATCH v5 4/6] drm: Decouple nomodeset from CONFIG_VGA_CONSOLE
This relationship was only for historical reasons and the nomodeset option
should be available even on platforms that don't enable CONFIG_VGA_CONSOLE.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
Acked-by: Jani Nikula
Acked-by: Pekka Paalanen
---
(no changes since v1)
drivers/gpu/drm/Kconfig | 6 ++
drivers/gpu/drm/Makefile | 2 +-
include/drm/drm_drv.h| 4
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git drivers/gpu/drm/Kconfig drivers/gpu/drm/Kconfig
index fb144617055b..483d534eb074 100644
--- drivers/gpu/drm/Kconfig
+++ drivers/gpu/drm/Kconfig
@@ -8,6 +8,7 @@
menuconfig DRM
tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI
support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
+ select DRM_NOMODESET
select DRM_PANEL_ORIENTATION_QUIRKS
select HDMI
select FB_CMDLINE
@@ -493,6 +494,11 @@ config DRM_EXPORT_FOR_TESTS
config DRM_PANEL_ORIENTATION_QUIRKS
tristate
+# Separate option because nomodeset parameter is global and expected built-in
+config DRM_NOMODESET
+ bool
+ default n
+
config DRM_LIB_RANDOM
bool
default n
diff --git drivers/gpu/drm/Makefile drivers/gpu/drm/Makefile
index c74810c285af..fa16d3e0bbdc 100644
--- drivers/gpu/drm/Makefile
+++ drivers/gpu/drm/Makefile
@@ -33,7 +33,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
-obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git include/drm/drm_drv.h include/drm/drm_drv.h
index 89e26a732175..da0c836fe8e1 100644
--- include/drm/drm_drv.h
+++ include/drm/drm_drv.h
@@ -601,10 +601,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct
drm_device *dev)
int drm_dev_set_unique(struct drm_device *dev, const char *name);
-#ifdef CONFIG_VGA_CONSOLE
extern bool drm_firmware_drivers_only(void);
-#else
-static inline bool drm_firmware_drivers_only(void) { return false; }
-#endif
#endif
--
2.33.1
[PATCH v5 0/6] Cleanups for the nomodeset kernel command line parameter logic
There is a lot of historical baggage on this parameter. It is defined in the vgacon driver as nomodeset, but its set function is called text_mode() and the value queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver should be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. This is a v5 of the patches, that just renames the function used by drivers to check to drm_firmware_drivers_only(). Other than that is the same as v4. Patch #1 and #2 are just trivial cleanups. Patch #3 moves the nomodeset boot option to the DRM subsystem and renames the variables and functions names. Patch #4 removes the relationship between the nomodeset parameter and the CONFIG_VGA_CONSOLE Kconfig symbol. Patch #5 adds nomodeset to the kernel parameters documentation. Patch #6 improves the message when nomodeset is enabled to make it more accurate and less sensational. Changes in v5: - Rename drm_get_modeset() to drm_firmware_drivers_only(). Changes in v4: - Don't mention the simpledrm driver and instead explain in high level terms what the nomodeset option is about. - Don't mention DRM drivers in the kernel message and instead explain that only the system framebuffer will be available. Changes in v3: - Drop the drm_drv_enabled() function and just call to drm_get_modeset(). - Make drm_get_modeset() just a getter function and don't return an error. - Move independent cleanups in drivers as separate preparatory patches. Changes in v2: - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set. - Squash patches to move nomodeset logic to DRM and do the renaming. - Name the function drm_check_modeset() and make it return -ENODEV. - Squash patch to add drm_drv_enabled() and make drivers use it. - Make the drivers changes before moving nomodeset logic to DRM. - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset. - Remove debug and error messages in drivers. Javier Martinez Canillas (6): drm: Don't print messages if drivers are disabled due nomodeset drm/vboxvideo: Drop CONFIG_VGA_CONSOLE guard to call vgacon_text_force() drm: Move nomodeset kernel parameter to the DRM subsystem drm: Decouple nomodeset from CONFIG_VGA_CONSOLE Documentation/admin-guide: Document nomodeset kernel parameter drm: Make the nomodeset message less sensational .../admin-guide/kernel-parameters.txt | 7 ++ drivers/gpu/drm/Kconfig | 6 + drivers/gpu/drm/Makefile | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +--- drivers/gpu/drm/ast/ast_drv.c | 3 +-- drivers/gpu/drm/drm_nomodeset.c | 24 +++ drivers/gpu/drm/i915/i915_module.c| 4 ++-- drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++-- drivers/gpu/drm/qxl/qxl_drv.c | 3 +-- drivers/gpu/drm/radeon/radeon_drv.c | 9 ++- drivers/gpu/drm/tiny/bochs.c | 3 +-- drivers/gpu/drm/tiny/cirrus.c | 4 ++-- drivers/gpu/drm/vboxvideo/vbox_drv.c | 5 +--- drivers/gpu/drm/virtio/virtgpu_drv.c | 3 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-- drivers/video/console/vgacon.c| 21 include/drm/drm_drv.h | 1 + include/linux/console.h | 6 - 19 files changed, 56 insertions(+), 60 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
Re: [PATCH v3 0/6] drm/meson: rework encoders to pass ATTACH_NO_CONNECTOR
On 20/10/2021 14:39, Neil Armstrong wrote:
> This serie finnally reworks the drm/meson driver by extracting the encoders
> in their own file and moves to bridge-only callbacks.
>
> This permits passing the ATTACH_NO_CONNECTOR bridge attach flag and finally
> use the CVBS & HDMI display-connector driver.
>
> This will ease Martin Blumenstingl writing the HDMI transceiver driver for
> the older Meson8/8b SoCs, and sets the proper architecture for the work in
> progress MIPI-DSI support.
>
> Finally, this serie will path the way to a removal of the device component
> and use the drmm memory management.
>
> Changes since v2 at [2]:
> - patch 1: no changes
> - patch 2: added martin's ack
> - patch 3: moved ->enable & ->disable to atomic, added sam's & martin's acks
> - patch 4: added martin's ack
> - patch 5: added martin's ack
> - patch 6: moved ->enable & ->disable to atomic, added martin's ack
>
> Changes since v1 at [1];
> - patch 1: added sam's review tag, fixed include order, fixed doc wording
> - patch 2: added sam's ack tag, switched to dev_dbg()
> - patch 3: moved mode_set to atomic_enable, removed DRM_DEBUG, fixed include
> order
> - patch 4: added sam's ack tag & applied to drm-misc-next
> - patch 5 & 6: added sam's ack tag
> - patch 7: added sam's review tag, stopped saving connector, moved mode_set
> to atomic_enable,
> added missing atomic state callbacks, fixed include order, switched to
> dev_dbg/dev_err
>
> [1] https://lore.kernel.org/r/20211014152606.2289528-1-narmstr...@baylibre.com
> [2] https://lore.kernel.org/r/20211015141107.2430800-1-narmstr...@baylibre.com
>
> Neil Armstrong (6):
> drm/bridge: display-connector: implement bus fmts callbacks
> drm/meson: remove useless recursive components matching
> drm/meson: split out encoder from meson_dw_hdmi
> drm/meson: encoder_hdmi: switch to bridge
> DRM_BRIDGE_ATTACH_NO_CONNECTOR
> drm/meson: rename venc_cvbs to encoder_cvbs
> drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR
>
> drivers/gpu/drm/bridge/display-connector.c| 86
> drivers/gpu/drm/meson/Kconfig | 2 +
> drivers/gpu/drm/meson/Makefile| 3 +-
> drivers/gpu/drm/meson/meson_drv.c | 71 ++-
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 342 +-
> drivers/gpu/drm/meson/meson_encoder_cvbs.c| 284 +++
> ...meson_venc_cvbs.h => meson_encoder_cvbs.h} | 2 +-
> drivers/gpu/drm/meson/meson_encoder_hdmi.c| 446 ++
> drivers/gpu/drm/meson/meson_encoder_hdmi.h| 12 +
> drivers/gpu/drm/meson/meson_venc_cvbs.c | 293
> 10 files changed, 881 insertions(+), 660 deletions(-)
> create mode 100644 drivers/gpu/drm/meson/meson_encoder_cvbs.c
> rename drivers/gpu/drm/meson/{meson_venc_cvbs.h => meson_encoder_cvbs.h}
> (92%)
> create mode 100644 drivers/gpu/drm/meson/meson_encoder_hdmi.c
> create mode 100644 drivers/gpu/drm/meson/meson_encoder_hdmi.h
> delete mode 100644 drivers/gpu/drm/meson/meson_venc_cvbs.c
>
>
> base-commit: f6632721cd6231e1bf28b5317dcc7543e43359f7
>
Applied to drm-misc-next
Now
https://lore.kernel.org/all/20211029135947.3022875-1-narmstr...@baylibre.com/
has been applied, we should have no regression.
Neil
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:
> On 2021-11-12 12:12:38, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > > The length of qcom,enabled-strings as property array is enough to
> > > determine the number of strings to be enabled, without needing to set
> > > qcom,num-strings to override the default number of strings when less
> > > than the default (which is also the maxium) is provided in DT.
> > >
> > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for
> > > WLED3")
> > > Signed-off-by: Marijn Suijten
> > > Reviewed-by: AngeloGioacchino Del Regno
> > >
> > > ---
> > > drivers/video/backlight/qcom-wled.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > b/drivers/video/backlight/qcom-wled.c
> > > index c5232478a343..9bfbf601762a 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > > return -EINVAL;
> > > }
> > > }
> > > +
> > > + cfg->num_strings = string_len;
> >
> > I still don't really understand why this wants to be a separate patch.
>
> I'm viewing this as a separate issue, and this makes it easier to
> document the change in a loose commit.
>
> > The warning text emitted by the previous patch (whatever text we agree
> > on) will be nonsense until this patch is applied.
> >
> > If this patch cannot appear before the warning is introduces then there
> > is no correct order for patches 4 and 5 (which implies they should be the
> > same patch).
>
> Agreed, this is a weird way of doing things in v2 - the error message is
> printed yet the length of qcom,enabled-strings is always ignored before
> this patch.
>
> If we were to reorder patch 5 before patch 4 that should also
> temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
> this `if` so that `qcom,num-strings` remains the definitive way to
> set/override length. That's doable, and makes it easier to read patch 4
> as that bit of code will be replaced by of_property_read_u32 on that
> exact line. Let me know which method you prefer.
Personally I would just squash them together. There are no redundant
values in the DT that could be fixed until we can use the string_len
to set num_strings.
However I won't object to the other approach providing the result is
bisectable.
Daniel.
Re: [RESEND PATCH v2 04/13] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On Fri, Nov 12, 2021 at 01:35:01PM +0100, Marijn Suijten wrote:
> On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:
> > > + if (string_len > 0) {
> > > + dev_warn(dev, "qcom,num-strings and
> > > qcom,enabled-strings are ambiguous\n");
> >
> > The warning should also be below the error message on the next if statement.
>
> Agreed.
>
> > This warning occurs even when there is no ambiguity.
> >
> > This could be:
> >
> > if (string_len > 0 && val != string_len)
> >
> > Combined these changes allows us to give a much more helpful and assertive
> > warning message:
> >
> > qcom,num-strings mis-matches and will partially override
> > qcom,enabled-strings (remove qcom,num-strings?)
>
> I want to let the user know it's set regardless of whether they're
> equivalent; no need to set both.
>
> How about:
>
> Only one of qcom,num-strings or qcom,enabled-strings should be set
>
> That should be more descriptive? Otherwise, let me know if you really
> want to allow users to (unnecessarily) set both - or if it can / should
> be caught in DT validation instead.
Yes. I can live with that text. Let's use that.
Maybe I wouldn't if there gazilions of existing DTs with both
properties but IIRC the number is likely to be small or zero
(although we couldn't be 100% sure which).
Daniel.
Re: [PATCH v2 6/8] drm: vkms: Refactor the plane composer to accept new formats
Hi Pekka,
On Thu, Nov 11, 2021 at 11:37 AM Pekka Paalanen wrote:
>
> On Thu, 11 Nov 2021 11:07:21 -0300
> Igor Torrente wrote:
>
> > Hi Pekka,
> >
> > On Thu, Nov 11, 2021 at 6:33 AM Pekka Paalanen wrote:
> > >
> > > On Wed, 10 Nov 2021 13:56:54 -0300
> > > Igor Torrente wrote:
> > >
> > > > On Tue, Nov 9, 2021 at 8:40 AM Pekka Paalanen
> > > > wrote:
> > > > >
> > > > > Hi Igor,
> > > > >
> > > > > again, that is a really nice speed-up. Unfortunately, I find the code
> > > > > rather messy and hard to follow. I hope my comments below help with
> > > > > re-designing it to be easier to understand.
> > > > >
> > > > >
> > > > > On Tue, 26 Oct 2021 08:34:06 -0300
> > > > > Igor Torrente wrote:
> > > > >
> > > > > > Currently the blend function only accepts XRGB_ and ARGB_
> > > > > > as a color input.
> > > > > >
> > > > > > This patch refactors all the functions related to the plane
> > > > > > composition
> > > > > > to overcome this limitation.
> > > > > >
> > > > > > Now the blend function receives a struct
> > > > > > `vkms_pixel_composition_functions`
> > > > > > containing two handlers.
> > > > > >
> > > > > > One will generate a buffer of each line of the frame with the pixels
> > > > > > converted to ARGB16161616. And the other will take this line buffer,
> > > > > > do some computation on it, and store the pixels in the destination.
> > > > > >
> > > > > > Both the handlers have the same signature. They receive a pointer to
> > > > > > the pixels that will be processed(`pixels_addr`), the number of
> > > > > > pixels
> > > > > > that will be treated(`length`), and the intermediate buffer of the
> > > > > > size
> > > > > > of a frame line (`line_buffer`).
> > > > > >
> > > > > > The first function has been totally described previously.
> > > > >
> > > > > What does this sentence mean?
> > > >
> > > > In the sentence "One will generate...", I give an overview of the two
> > > > types of
> > > > handlers. And the overview of the first handler describes the full
> > > > behavior of
> > > > it.
> > > >
> > > > But it doesn't look clear enough, I will improve it in the future.
> > > >
> > > > >
> > > > > >
> > > > > > The second is more interesting, as it has to perform two roles
> > > > > > depending
> > > > > > on where it is called in the code.
> > > > > >
> > > > > > The first is to convert(if necessary) the data received in the
> > > > > > `line_buffer` and write in the memory pointed by `pixels_addr`.
> > > > > >
> > > > > > The second role is to perform the `alpha_blend`. So, it takes the
> > > > > > pixels
> > > > > > in the `line_buffer` and `pixels_addr`, executes the blend, and
> > > > > > stores
> > > > > > the result back to the `pixels_addr`.
> > > > > >
> > > > > > The per-line implementation was chosen for performance reasons.
> > > > > > The per-pixel functions were having performance issues due to
> > > > > > indirect
> > > > > > function call overhead.
> > > > > >
> > > > > > The per-line code trades off memory for execution time. The
> > > > > > `line_buffer`
> > > > > > allows us to diminish the number of function calls.
> > > > > >
> > > > > > Results in the IGT test `kms_cursor_crc`:
> > > > > >
> > > > > > | Frametime |
> > > > > > |:---:|:-:|:--:|::|
> > > > > > | implmentation | Current | Per-pixel | Per-line |
> > > > > > | frametime range | 8~22 ms | 32~56 ms | 6~19 ms |
> > > > > > | Average | 10.0 ms | 35.8 ms | 8.6 ms |
> > > > > >
> > > > > > Reported-by: kernel test robot
> > > > > > Signed-off-by: Igor Torrente
> > > > > > ---
> > > > > > V2: Improves the performance drastically, by perfoming the
> > > > > > operations
> > > > > > per-line and not per-pixel(Pekka Paalanen).
> > > > > > Minor improvements(Pekka Paalanen).
> > > > > > ---
> > > > > > drivers/gpu/drm/vkms/vkms_composer.c | 321
> > > > > > ---
> > > > > > drivers/gpu/drm/vkms/vkms_formats.h | 155 +
> > > > > > 2 files changed, 342 insertions(+), 134 deletions(-)
> > > > > > create mode 100644 drivers/gpu/drm/vkms/vkms_formats.h
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > > b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > > index 383ca657ddf7..69fe3a89bdc9 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>
> ...
>
> > > > > > +struct vkms_pixel_composition_functions {
> > > > > > + void (*get_src_line)(void *pixels_addr, int length, u64
> > > > > > *line_buffer);
> > > > > > + void (*set_output_line)(void *pixels_addr, int length, u64
> > > > > > *line_buffer);
> > > > >
> > > > > I would be a little more comfortable if instead of u64 *line_buffer
> > > > > you
> > > > > would have something like
> > > > >
> > > > > struct line_buffer {
> > > > > u16 *row;
> > > > > size_t nelem;
> > > > > }
> >
Re: Questions about KMS flip
Hi guys, Am 10.11.21 um 14:26 schrieb Daniel Vetter: [SNIP] stack depot, not stuff it into a string. stack depot is a lot more efficient and capturing backtraces, since it de-dupes and hashes and all you need is a pointer iirc. linux/stackdepot.h for interface, I think we recently gained a few more users of it in drm. It's _really_ nifty. -Daniel thanks for the pointer Daniel, that framework indeed looks like it can become handy. Anyway this unfortunately turned out to be work for Harray and Nicholas. In detail it's about this bug report here: https://bugzilla.kernel.org/show_bug.cgi?id=214621 Lang was able to reproduce the issue and narrow it down to the pin in amdgpu_display_crtc_page_flip_target(). In other words we somehow have an unbalanced pinning of the scanout buffer in DC. What should we do about that? Regards, Christian.
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On 2021-11-12 12:12:38, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > The length of qcom,enabled-strings as property array is enough to
> > determine the number of strings to be enabled, without needing to set
> > qcom,num-strings to override the default number of strings when less
> > than the default (which is also the maxium) is provided in DT.
> >
> > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for
> > WLED3")
> > Signed-off-by: Marijn Suijten
> > Reviewed-by: AngeloGioacchino Del Regno
> >
> > ---
> > drivers/video/backlight/qcom-wled.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> > index c5232478a343..9bfbf601762a 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > return -EINVAL;
> > }
> > }
> > +
> > + cfg->num_strings = string_len;
>
> I still don't really understand why this wants to be a separate patch.
I'm viewing this as a separate issue, and this makes it easier to
document the change in a loose commit.
> The warning text emitted by the previous patch (whatever text we agree
> on) will be nonsense until this patch is applied.
>
> If this patch cannot appear before the warning is introduces then there
> is no correct order for patches 4 and 5 (which implies they should be the
> same patch).
Agreed, this is a weird way of doing things in v2 - the error message is
printed yet the length of qcom,enabled-strings is always ignored before
this patch.
If we were to reorder patch 5 before patch 4 that should also
temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
this `if` so that `qcom,num-strings` remains the definitive way to
set/override length. That's doable, and makes it easier to read patch 4
as that bit of code will be replaced by of_property_read_u32 on that
exact line. Let me know which method you prefer.
- Marijn
[Bug 214621] WARNING: CPU: 3 PID: 521 at drivers/gpu/drm/ttm/ttm_bo.c:409 ttm_bo_release+0xb64/0xe40 [ttm]
https://bugzilla.kernel.org/show_bug.cgi?id=214621 --- Comment #20 from Christian König (christian.koe...@amd.com) --- Nice work Lang, question is now only how to fix it? We probably need to assign this bug to Harry and Nicholas. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [RESEND PATCH v2 04/13] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On 2021-11-12 12:08:39, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:
> > When not specifying num-strings in the DT the default is used, but +1 is
> > added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
> > of 3 and 4 respectively, causing out-of-bounds reads and register
> > read/writes. This +1 exists for a deficiency in the DT parsing code,
> > and is simply omitted entirely - solving this oob issue - by parsing the
> > property separately much like qcom,enabled-strings.
> >
> > This also allows more stringent checks on the maximum value when
> > qcom,enabled-strings is provided in the DT. Note that num-strings is
> > parsed after enabled-strings to give it final sign-off over the length,
> > which DT currently utilizes to get around an incorrect fixed read of
> > four elements from that array (has been addressed in a prior patch).
> >
> > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > Signed-off-by: Marijn Suijten
> > Reviewed-By: AngeloGioacchino Del Regno
> >
> > ---
> > drivers/video/backlight/qcom-wled.c | 51 +++--
> > 1 file changed, 19 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> > index 977cd75827d7..c5232478a343 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> > }
> > }
> >
> > + rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> > + if (!rc) {
> > + if (val < 1 || val > wled->max_string_count) {
> > + dev_err(dev, "qcom,num-strings must be between 1 and
> > %d\n",
> > + wled->max_string_count);
> > + return -EINVAL;
> > + }
> > +
> > + if (string_len > 0) {
> > + dev_warn(dev, "qcom,num-strings and
> > qcom,enabled-strings are ambiguous\n");
>
> The warning should also be below the error message on the next if statement.
Agreed.
> This warning occurs even when there is no ambiguity.
>
> This could be:
>
> if (string_len > 0 && val != string_len)
>
> Combined these changes allows us to give a much more helpful and assertive
> warning message:
>
> qcom,num-strings mis-matches and will partially override
> qcom,enabled-strings (remove qcom,num-strings?)
I want to let the user know it's set regardless of whether they're
equivalent; no need to set both.
How about:
Only one of qcom,num-strings or qcom,enabled-strings should be set
That should be more descriptive? Otherwise, let me know if you really
want to allow users to (unnecessarily) set both - or if it can / should
be caught in DT validation instead.
- Marijn
> > + if (val > string_len) {
> > + dev_err(dev, "qcom,num-strings exceeds
> > qcom,enabled-strings\n");
> > + return -EINVAL;
> > + }
> > + }
>
>
> Daniel.
Re: [RESEND PATCH v2 09/13] backlight: qcom-wled: Respect enabled-strings in set_brightness
On Fri, Nov 12, 2021 at 01:27:02AM +0100, Marijn Suijten wrote:
> The hardware is capable of controlling any non-contiguous sequence of
> LEDs specified in the DT using qcom,enabled-strings as u32
> array, and this also follows from the DT-bindings documentation. The
> numbers specified in this array represent indices of the LED strings
> that are to be enabled and disabled.
>
> Its value is appropriately used to setup and enable string modules, but
> completely disregarded in the set_brightness paths which only iterate
> over the number of strings linearly.
> Take an example where only string 2 is enabled with
> qcom,enabled_strings=<2>: this string is appropriately enabled but
> subsequent brightness changes would have only touched the zero'th
> brightness register because num_strings is 1 here. This is simply
> addressed by looking up the string for this index in the enabled_strings
> array just like the other codepaths that iterate over num_strings.
>
> Likewise enabled_strings is now also used in the autodetection path for
> consistent behaviour: when a list of strings is specified in DT only
> those strings will be probed for autodetection, analogous to how the
> number of strings that need to be probed is already bound by
> qcom,num-strings. After all autodetection uses the set_brightness
> helpers to set an initial value, which could otherwise end up changing
> brightness on a different set of strings.
>
> Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
> Fixes: 03b2b5e86986 ("backlight: qcom-wled: Add support for WLED4 peripheral")
> Signed-off-by: Marijn Suijten
> Reviewed-by: AngeloGioacchino Del Regno
>
Reviewed-by: Daniel Thompson
Daniel.
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> The length of qcom,enabled-strings as property array is enough to
> determine the number of strings to be enabled, without needing to set
> qcom,num-strings to override the default number of strings when less
> than the default (which is also the maxium) is provided in DT.
>
> Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
> Signed-off-by: Marijn Suijten
> Reviewed-by: AngeloGioacchino Del Regno
>
> ---
> drivers/video/backlight/qcom-wled.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index c5232478a343..9bfbf601762a 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> return -EINVAL;
> }
> }
> +
> + cfg->num_strings = string_len;
I still don't really understand why this wants to be a separate patch.
The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.
If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).
Daniel.
Re: [PATCH v4 0/6] Cleanups for the nomodeset kernel command line parameter logic
On Fri, 12 Nov 2021 12:20:14 +0100 Javier Martinez Canillas wrote: > On 11/12/21 11:57, Thomas Zimmermann wrote: > > [snip] > > >>> > >>> This is what HW-specific drivers want to query in their init/probing > >>> code. The actual semantics of this decision is hidden from the driver. > >>> It's also easier to read than the other name IMHO > >> > >> Ok, but what is a "native driver"? Or a "non-native driver"? > >> Is that established kernel terminology? > >> > >> I'd think a non-native driver is something that e.g. ndiswrapper is > >> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is > >> the driver that would not consult this function, right? > > > > We use that term for hw-specific drivers. A 'non-native' driver would be > > called generic or firmware driver. > > > > My concern with the 'modeset' term is that it exposes an implementation > > detail, which can mislead a driver to to the wrong thing: a HW-specifc > > driver that disables it's modesetting functionality would pass the test > > for (!modeset). But that's not what we want, we want to disable all of > > the driver and not even load it. > > > > How about we invert the test function and use something like > > > > bool drm_firmware_drivers_only() > > > > That name I think is more self explanatory, so it works for me. I'm not going to argue against that. :-) Thanks, pq pgpqCFg2pvHLS.pgp Description: OpenPGP digital signature
Re: [RESEND PATCH v2 04/13] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:
> When not specifying num-strings in the DT the default is used, but +1 is
> added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
> of 3 and 4 respectively, causing out-of-bounds reads and register
> read/writes. This +1 exists for a deficiency in the DT parsing code,
> and is simply omitted entirely - solving this oob issue - by parsing the
> property separately much like qcom,enabled-strings.
>
> This also allows more stringent checks on the maximum value when
> qcom,enabled-strings is provided in the DT. Note that num-strings is
> parsed after enabled-strings to give it final sign-off over the length,
> which DT currently utilizes to get around an incorrect fixed read of
> four elements from that array (has been addressed in a prior patch).
>
> Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> Signed-off-by: Marijn Suijten
> Reviewed-By: AngeloGioacchino Del Regno
>
> ---
> drivers/video/backlight/qcom-wled.c | 51 +++--
> 1 file changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index 977cd75827d7..c5232478a343 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> }
> }
>
> + rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> + if (!rc) {
> + if (val < 1 || val > wled->max_string_count) {
> + dev_err(dev, "qcom,num-strings must be between 1 and
> %d\n",
> + wled->max_string_count);
> + return -EINVAL;
> + }
> +
> + if (string_len > 0) {
> + dev_warn(dev, "qcom,num-strings and
> qcom,enabled-strings are ambiguous\n");
This warning occurs even when there is no ambiguity.
This could be:
if (string_len > 0 && val != string_len)
The warning should also be below the error message on the next if statement.
Combined these changes allows us to give a much more helpful and assertive
warning message:
qcom,num-strings mis-matches and will partially override
qcom,enabled-strings (remove qcom,num-strings?)
> + if (val > string_len) {
> + dev_err(dev, "qcom,num-strings exceeds
> qcom,enabled-strings\n");
> + return -EINVAL;
> + }
> + }
Daniel.
RE: [Intel-gfx] [PATCH 2/3] drm/i915/dg2: Add initial gt/ctx/engine workarounds
This issue was not catched by CI, because of series of unfortunate events.
Before, CI has rebooted without module blocklist, and CI catched boot-time
dmesg correctly and marked it as 'ci@boot' test with failure if there was a
taint.
I've been doing changes to make blocklisting i915 possible and load it as
the first test of IGT: that'd make possible to remove some workarounds
and integrate the result better on our framework.
The test to decide if i915 should be modprobed was slightly off, and
on these runs where i915 failed to load in boot, it was modprobed again,
and modprobe hanged because of existing i915. Results were not collected.
I've added the condition to the conditional modprobe, and the results
from failed boot-time modprobe should be soon available as before,
eg. CI_DRM_10873 later shards with SNB.
Regards,
Tomi
> From: Latvala, Petri
> On Tue, Nov 02, 2021 at 03:25:10PM -0700, Matt Roper wrote:
> > Bspec: 54077,68173,54833
> > Cc: Anusha Srivatsa
> > Signed-off-by: Matt Roper
> > ---
> > drivers/gpu/drm/i915/gt/intel_workarounds.c | 278
> +++-
> > drivers/gpu/drm/i915/i915_reg.h | 94 +--
> > drivers/gpu/drm/i915/intel_pm.c | 21 +-
> > 3 files changed, 372 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 4aaa210fc003..37fd541a9719 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -644,6 +644,42 @@ static void dg1_ctx_workarounds_init(struct
> intel_engine_cs *engine,
> >
> DG1_HZ_READ_SUPPRESSION_OPTIMIZATION_DISABLE);
> > }
> >
> > +static void dg2_ctx_workarounds_init(struct intel_engine_cs *engine,
> > +struct
> i915_wa_list *wal)
> > +{
> > + gen12_ctx_gt_tuning_init(engine, wal);
> > +
> > + /* Wa_16011186671:dg2_g11 */
> > + if (IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0,
> STEP_B0)) {
> > + wa_masked_dis(wal, VFLSKPD,
> DIS_MULT_MISS_RD_SQUASH);
> > + wa_masked_en(wal, VFLSKPD,
> DIS_OVER_FETCH_CACHE);
> > + }
> > +
> > + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0,
> STEP_B0)) {
> > + /* Wa_14010469329:dg2_g10 */
> > + wa_masked_en(wal,
> GEN11_COMMON_SLICE_CHICKEN3,
> > +
> XEHP_DUAL_SIMD8_SEQ_MERGE_DISABLE);
> > +
> > + /*
> > +* Wa_22010465075:dg2_g10
> > +* Wa_22010613112:dg2_g10
> > +* Wa_14010698770:dg2_g10
> > +*/
> > + wa_masked_en(wal,
> GEN11_COMMON_SLICE_CHICKEN3,
> > +
> GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > + }
> > +
> > + /* Wa_16013271637:dg2 */
> > + wa_masked_en(wal, SLICE_COMMON_ECO_CHICKEN1,
> > +
> MSC_MSAA_REODER_BUF_BYPASS_DISABLE);
> > +
> > + /* Wa_22012532006:dg2 */
> > + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0,
> STEP_C0) ||
> > + IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0,
> STEP_B0))
> > + wa_masked_en(wal,
> GEN9_HALF_SLICE_CHICKEN7,
> > +
> DG2_DISABLE_ROUND_ENABLE_ALLOW_FOR_SSLA);
> > +}
> > +
> > static void fakewa_disable_nestedbb_mode(struct intel_engine_cs
> *engine,
> >
> struct i915_wa_list *wal)
> > {
> > @@ -730,7 +766,9 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs
> *engine,
> > if (engine->class != RENDER_CLASS)
> > goto done;
> >
> > - if (IS_XEHPSDV(i915))
> > + if (IS_DG2(i915))
> > + dg2_ctx_workarounds_init(engine, wal);
> > + else if (IS_XEHPSDV(i915))
> > ; /* noop; none at this time */
> > else if (IS_DG1(i915))
> > dg1_ctx_workarounds_init(engine, wal);
> > @@ -1343,12 +1381,117 @@ xehpsdv_gt_workarounds_init(struct intel_gt
> *gt, struct i915_wa_list *wal)
> > GLOBAL_INVALIDATION_MODE);
> > }
> >
> > +static void
> > +dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > +{
> > + struct intel_engine_cs *engine;
> > + int id;
> > +
> > + xehp_init_mcr(gt, wal);
> > +
> > + /* Wa_14011060649:dg2 */
> > + wa_14011060649(gt, wal);
> > +
> > + /*
> > +* Although there are per-engine instances of these registers,
> > +* they technically exist outside the engine itself and are not
> > +* impacted by engine resets. Furthermore, they're part of the
> > +* GuC blacklist so trying to treat them as engine workarounds
> > +* will result in GuC initialization failure and a wedged GPU.
> > +*/
> > + for_each_engine(engine, gt, id) {
> > + if (engine->class != VIDEO_DECODE_CLASS)
> > + continue;
> > +
> > + /* Wa_16010515920:dg2_g10 */
> > + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10,
> STEP_A0, STEP_B0))
> > + wa_write_or(wal,
> VDBOX_CGCTL3F18(engine->mmio_base),
> > +
> ALNUNIT_CLKGATE_DIS);
> > + }
> > +
> > + if (IS_DG2_G10(gt->i915)) {
> > + /* Wa_22010523718:dg2 */
> > + wa_writ
Re: [RESEND PATCH v2 01/13] backlight: qcom-wled: Validate enabled string indices in DT
On Fri, Nov 12, 2021 at 01:26:54AM +0100, Marijn Suijten wrote:
> The strings passed in DT may possibly cause out-of-bounds register
> accesses and should be validated before use.
>
> Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
> Signed-off-by: Marijn Suijten
> Reviewed-by: AngeloGioacchino Del Regno
>
Reviewed-by: Daniel Thompson
Daniel.
[PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read()
edid_read() was assumed to return 0 on success. After
7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()"),
the function can return >= 0 for successful case. Fix the g_edid_break
condition in sp_tx_edid_read().
Fixes: 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from
sp_tx_rst_aux()")
Signed-off-by: Hsin-Yi Wang
---
drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 1a871f6b6822ee..392203b576042b 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -826,7 +826,7 @@ static int sp_tx_edid_read(struct anx7625_data *ctx,
g_edid_break = edid_read(ctx, offset,
pblock_buf);
- if (g_edid_break)
+ if (g_edid_break < 0)
break;
memcpy(&pedid_blocks_buf[offset],
--
2.34.0.rc1.387.gb447b232ab-goog
Re: [PATCH v4 0/6] Cleanups for the nomodeset kernel command line parameter logic
Hi Am 12.11.21 um 12:20 schrieb Javier Martinez Canillas: On 11/12/21 11:57, Thomas Zimmermann wrote: [snip] This is what HW-specific drivers want to query in their init/probing code. The actual semantics of this decision is hidden from the driver. It's also easier to read than the other name IMHO Ok, but what is a "native driver"? Or a "non-native driver"? Is that established kernel terminology? I'd think a non-native driver is something that e.g. ndiswrapper is loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is the driver that would not consult this function, right? We use that term for hw-specific drivers. A 'non-native' driver would be called generic or firmware driver. My concern with the 'modeset' term is that it exposes an implementation detail, which can mislead a driver to to the wrong thing: a HW-specifc driver that disables it's modesetting functionality would pass the test for (!modeset). But that's not what we want, we want to disable all of the driver and not even load it. How about we invert the test function and use something like bool drm_firmware_drivers_only() That name I think is more self explanatory, so it works for me. There was also another bikeshed about where to put the function declaration, I added to but with that name I believe that should be in instead. I agree with drm_drv.h. It's a DRM-wide function and it fit's there best, I'd say. Best regards Thomas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v4 0/6] Cleanups for the nomodeset kernel command line parameter logic
On 11/12/21 11:57, Thomas Zimmermann wrote: [snip] >>> >>> This is what HW-specific drivers want to query in their init/probing >>> code. The actual semantics of this decision is hidden from the driver. >>> It's also easier to read than the other name IMHO >> >> Ok, but what is a "native driver"? Or a "non-native driver"? >> Is that established kernel terminology? >> >> I'd think a non-native driver is something that e.g. ndiswrapper is >> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is >> the driver that would not consult this function, right? > > We use that term for hw-specific drivers. A 'non-native' driver would be > called generic or firmware driver. > > My concern with the 'modeset' term is that it exposes an implementation > detail, which can mislead a driver to to the wrong thing: a HW-specifc > driver that disables it's modesetting functionality would pass the test > for (!modeset). But that's not what we want, we want to disable all of > the driver and not even load it. > > How about we invert the test function and use something like > > bool drm_firmware_drivers_only() > That name I think is more self explanatory, so it works for me. There was also another bikeshed about where to put the function declaration, I added to but with that name I believe that should be in instead. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH 2/3] drm/i915/dg2: Add initial gt/ctx/engine workarounds
On Tue, Nov 02, 2021 at 03:25:10PM -0700, Matt Roper wrote:
> Bspec: 54077,68173,54833
> Cc: Anusha Srivatsa
> Signed-off-by: Matt Roper
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 278 +++-
> drivers/gpu/drm/i915/i915_reg.h | 94 +--
> drivers/gpu/drm/i915/intel_pm.c | 21 +-
> 3 files changed, 372 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 4aaa210fc003..37fd541a9719 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -644,6 +644,42 @@ static void dg1_ctx_workarounds_init(struct
> intel_engine_cs *engine,
>DG1_HZ_READ_SUPPRESSION_OPTIMIZATION_DISABLE);
> }
>
> +static void dg2_ctx_workarounds_init(struct intel_engine_cs *engine,
> + struct i915_wa_list *wal)
> +{
> + gen12_ctx_gt_tuning_init(engine, wal);
> +
> + /* Wa_16011186671:dg2_g11 */
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0)) {
> + wa_masked_dis(wal, VFLSKPD, DIS_MULT_MISS_RD_SQUASH);
> + wa_masked_en(wal, VFLSKPD, DIS_OVER_FETCH_CACHE);
> + }
> +
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_B0)) {
> + /* Wa_14010469329:dg2_g10 */
> + wa_masked_en(wal, GEN11_COMMON_SLICE_CHICKEN3,
> + XEHP_DUAL_SIMD8_SEQ_MERGE_DISABLE);
> +
> + /*
> + * Wa_22010465075:dg2_g10
> + * Wa_22010613112:dg2_g10
> + * Wa_14010698770:dg2_g10
> + */
> + wa_masked_en(wal, GEN11_COMMON_SLICE_CHICKEN3,
> + GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> + }
> +
> + /* Wa_16013271637:dg2 */
> + wa_masked_en(wal, SLICE_COMMON_ECO_CHICKEN1,
> + MSC_MSAA_REODER_BUF_BYPASS_DISABLE);
> +
> + /* Wa_22012532006:dg2 */
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_C0) ||
> + IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0))
> + wa_masked_en(wal, GEN9_HALF_SLICE_CHICKEN7,
> + DG2_DISABLE_ROUND_ENABLE_ALLOW_FOR_SSLA);
> +}
> +
> static void fakewa_disable_nestedbb_mode(struct intel_engine_cs *engine,
>struct i915_wa_list *wal)
> {
> @@ -730,7 +766,9 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs *engine,
> if (engine->class != RENDER_CLASS)
> goto done;
>
> - if (IS_XEHPSDV(i915))
> + if (IS_DG2(i915))
> + dg2_ctx_workarounds_init(engine, wal);
> + else if (IS_XEHPSDV(i915))
> ; /* noop; none at this time */
> else if (IS_DG1(i915))
> dg1_ctx_workarounds_init(engine, wal);
> @@ -1343,12 +1381,117 @@ xehpsdv_gt_workarounds_init(struct intel_gt *gt,
> struct i915_wa_list *wal)
> GLOBAL_INVALIDATION_MODE);
> }
>
> +static void
> +dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> +{
> + struct intel_engine_cs *engine;
> + int id;
> +
> + xehp_init_mcr(gt, wal);
> +
> + /* Wa_14011060649:dg2 */
> + wa_14011060649(gt, wal);
> +
> + /*
> + * Although there are per-engine instances of these registers,
> + * they technically exist outside the engine itself and are not
> + * impacted by engine resets. Furthermore, they're part of the
> + * GuC blacklist so trying to treat them as engine workarounds
> + * will result in GuC initialization failure and a wedged GPU.
> + */
> + for_each_engine(engine, gt, id) {
> + if (engine->class != VIDEO_DECODE_CLASS)
> + continue;
> +
> + /* Wa_16010515920:dg2_g10 */
> + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_B0))
> + wa_write_or(wal, VDBOX_CGCTL3F18(engine->mmio_base),
> + ALNUNIT_CLKGATE_DIS);
> + }
> +
> + if (IS_DG2_G10(gt->i915)) {
> + /* Wa_22010523718:dg2 */
> + wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE,
> + CG3DDISCFEG_CLKGATE_DIS);
> +
> + /* Wa_14011006942:dg2 */
> + wa_write_or(wal, SUBSLICE_UNIT_LEVEL_CLKGATE,
> + DSS_ROUTER_CLKGATE_DIS);
> + }
> +
> + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_B0)) {
> + /* Wa_14010680813:dg2_g10 */
> + wa_write_or(wal, GEN12_GAMSTLB_CTRL, CONTROL_BLOCK_CLKGATE_DIS |
> + EGRESS_BLOCK_CLKGATE_DIS | TAG_BLOCK_CLKGATE_DIS);
> +
> + /* Wa_14010948348:dg2_g10 */
> + wa_write_or(wal, UNSLCGCTL9430, MSQDUNIT_CLKGATE_DIS);
> +
> + /* Wa_14011037102:dg2_g10 */
> + wa_write_or(wal, UNSLCGCTL9444, LTCDD_CLKGATE_DIS);
> +
> + /* Wa_14011371254:
[Bug 214621] WARNING: CPU: 3 PID: 521 at drivers/gpu/drm/ttm/ttm_bo.c:409 ttm_bo_release+0xb64/0xe40 [ttm]
https://bugzilla.kernel.org/show_bug.cgi?id=214621 --- Comment #19 from Erhard F. (erhar...@mailbox.org) --- (In reply to Lang Yu from comment #18) > Hi all, > > I reproduced the issue. Thanks for Erhard F.'s work! > > The problem is the pinned BO of last call to > amdgpu_display_crtc_page_flip_target() was not unpinned properly. Thanks for your work on it Lang! I was rather busy and would have been able to test it out this weekend. But glad you found the cause of the issue anyhow! -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[PATCH v9 15/15] arm64: dts: mediatek: Get rid of mediatek, larb for MM nodes
After adding device_link between the IOMMU consumer and smi,
the mediatek,larb is unnecessary now.
CC: Matthias Brugger
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
---
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 16
arch/arm64/boot/dts/mediatek/mt8183.dtsi | 6 --
2 files changed, 22 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index d9e005ae5bb0..205c221696a6 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -1009,7 +1009,6 @@
<&mmsys CLK_MM_MUTEX_32K>;
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
iommus = <&iommu M4U_PORT_MDP_RDMA0>;
- mediatek,larb = <&larb0>;
mediatek,vpu = <&vpu>;
};
@@ -1020,7 +1019,6 @@
<&mmsys CLK_MM_MUTEX_32K>;
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
iommus = <&iommu M4U_PORT_MDP_RDMA1>;
- mediatek,larb = <&larb4>;
};
mdp_rsz0: rsz@14003000 {
@@ -1050,7 +1048,6 @@
clocks = <&mmsys CLK_MM_MDP_WDMA>;
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
iommus = <&iommu M4U_PORT_MDP_WDMA>;
- mediatek,larb = <&larb0>;
};
mdp_wrot0: wrot@14007000 {
@@ -1059,7 +1056,6 @@
clocks = <&mmsys CLK_MM_MDP_WROT0>;
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
iommus = <&iommu M4U_PORT_MDP_WROT0>;
- mediatek,larb = <&larb0>;
};
mdp_wrot1: wrot@14008000 {
@@ -1068,7 +1064,6 @@
clocks = <&mmsys CLK_MM_MDP_WROT1>;
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
iommus = <&iommu M4U_PORT_MDP_WROT1>;
- mediatek,larb = <&larb4>;
};
ovl0: ovl@1400c000 {
@@ -1078,7 +1073,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_OVL0>;
iommus = <&iommu M4U_PORT_DISP_OVL0>;
- mediatek,larb = <&larb0>;
mediatek,gce-client-reg = <&gce SUBSYS_1400 0xc000
0x1000>;
};
@@ -1089,7 +1083,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_OVL1>;
iommus = <&iommu M4U_PORT_DISP_OVL1>;
- mediatek,larb = <&larb4>;
mediatek,gce-client-reg = <&gce SUBSYS_1400 0xd000
0x1000>;
};
@@ -1100,7 +1093,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_RDMA0>;
iommus = <&iommu M4U_PORT_DISP_RDMA0>;
- mediatek,larb = <&larb0>;
mediatek,gce-client-reg = <&gce SUBSYS_1400 0xe000
0x1000>;
};
@@ -,7 +1103,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_RDMA1>;
iommus = <&iommu M4U_PORT_DISP_RDMA1>;
- mediatek,larb = <&larb4>;
mediatek,gce-client-reg = <&gce SUBSYS_1400 0xf000
0x1000>;
};
@@ -1122,7 +1113,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_RDMA2>;
iommus = <&iommu M4U_PORT_DISP_RDMA2>;
- mediatek,larb = <&larb4>;
mediatek,gce-client-reg = <&gce SUBSYS_1401 0
0x1000>;
};
@@ -1133,7 +1123,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_WDMA0>;
iommus = <&iommu M4U_PORT_DISP_WDMA0>;
- mediatek,larb = <&larb0>;
mediatek,gce-client-reg = <&gce SUBSYS_1401 0x1000
0x1000>;
};
@@ -1144,7 +1133,6 @@
power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
clocks = <&mmsys CLK_MM_DISP_WDMA1>;
iommus = <&iommu M4U_PORT_DISP_WDMA1>;
- mediatek,larb = <&larb4>;
mediatek,gce-client-reg = <&gce SUBSYS_1401 0x2000
0x1000>;
};
@@ -1395,7 +1383,6 @@
<0 0x16027800 0 0x800>, /* VDEC_HWB */
<0 0x16028400 0 0x400>; /* VDEC_HWG */
[PATCH v9 14/15] arm: dts: mediatek: Get rid of mediatek, larb for MM nodes
After adding device_link between the IOMMU consumer and smi, the
mediatek,larb is unnecessary now.
CC: Matthias Brugger
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
arch/arm/boot/dts/mt2701.dtsi | 2 --
arch/arm/boot/dts/mt7623n.dtsi | 5 -
2 files changed, 7 deletions(-)
diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
index 4776f85d6d5b..ef583cfd3baf 100644
--- a/arch/arm/boot/dts/mt2701.dtsi
+++ b/arch/arm/boot/dts/mt2701.dtsi
@@ -564,7 +564,6 @@
clock-names = "jpgdec-smi",
"jpgdec";
power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
- mediatek,larb = <&larb2>;
iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
<&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
};
@@ -577,7 +576,6 @@
clocks = <&imgsys CLK_IMG_VENC>;
clock-names = "jpgenc";
power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
- mediatek,larb = <&larb2>;
iommus = <&iommu MT2701_M4U_PORT_JPGENC_RDMA>,
<&iommu MT2701_M4U_PORT_JPGENC_BSDMA>;
};
diff --git a/arch/arm/boot/dts/mt7623n.dtsi b/arch/arm/boot/dts/mt7623n.dtsi
index bcb0846e29fd..3adab5cd1fef 100644
--- a/arch/arm/boot/dts/mt7623n.dtsi
+++ b/arch/arm/boot/dts/mt7623n.dtsi
@@ -121,7 +121,6 @@
clock-names = "jpgdec-smi",
"jpgdec";
power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
- mediatek,larb = <&larb2>;
iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
<&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
};
@@ -144,7 +143,6 @@
interrupts = ;
clocks = <&mmsys CLK_MM_DISP_OVL>;
iommus = <&iommu MT2701_M4U_PORT_DISP_OVL_0>;
- mediatek,larb = <&larb0>;
};
rdma0: rdma@14008000 {
@@ -154,7 +152,6 @@
interrupts = ;
clocks = <&mmsys CLK_MM_DISP_RDMA>;
iommus = <&iommu MT2701_M4U_PORT_DISP_RDMA>;
- mediatek,larb = <&larb0>;
};
wdma@14009000 {
@@ -164,7 +161,6 @@
interrupts = ;
clocks = <&mmsys CLK_MM_DISP_WDMA>;
iommus = <&iommu MT2701_M4U_PORT_DISP_WDMA>;
- mediatek,larb = <&larb0>;
};
bls: pwm@1400a000 {
@@ -215,7 +211,6 @@
interrupts = ;
clocks = <&mmsys CLK_MM_DISP_RDMA1>;
iommus = <&iommu MT2701_M4U_PORT_DISP_RDMA1>;
- mediatek,larb = <&larb0>;
};
dpi0: dpi@14014000 {
--
2.18.0
[PATCH v9 13/15] memory: mtk-smi: Get rid of mtk_smi_larb_get/put
After adding device_link between the iommu consumer and smi-larb,
the pm_runtime_get(_sync) of smi-larb and smi-common will be called
automatically. we can get rid of mtk_smi_larb_get/put.
CC: Matthias Brugger
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
Acked-by: Krzysztof Kozlowski
Acked-by: Matthias Brugger
Reviewed-by: Dafna Hirschfeld
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
drivers/memory/mtk-smi.c | 14 --
include/soc/mediatek/smi.h | 20
2 files changed, 34 deletions(-)
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index c5fb51f73b34..7c61c924e220 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -134,20 +134,6 @@ static void mtk_smi_clk_disable(const struct mtk_smi *smi)
clk_disable_unprepare(smi->clk_apb);
}
-int mtk_smi_larb_get(struct device *larbdev)
-{
- int ret = pm_runtime_resume_and_get(larbdev);
-
- return (ret < 0) ? ret : 0;
-}
-EXPORT_SYMBOL_GPL(mtk_smi_larb_get);
-
-void mtk_smi_larb_put(struct device *larbdev)
-{
- pm_runtime_put_sync(larbdev);
-}
-EXPORT_SYMBOL_GPL(mtk_smi_larb_put);
-
static int
mtk_smi_larb_bind(struct device *dev, struct device *master, void *data)
{
diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
index 15e3397cec58..11f7d6b59642 100644
--- a/include/soc/mediatek/smi.h
+++ b/include/soc/mediatek/smi.h
@@ -19,26 +19,6 @@ struct mtk_smi_larb_iommu {
unsigned char bank[32];
};
-/*
- * mtk_smi_larb_get: Enable the power domain and clocks for this local arbiter.
- * It also initialize some basic setting(like iommu).
- * mtk_smi_larb_put: Disable the power domain and clocks for this local
arbiter.
- * Both should be called in non-atomic context.
- *
- * Returns 0 if successful, negative on failure.
- */
-int mtk_smi_larb_get(struct device *larbdev);
-void mtk_smi_larb_put(struct device *larbdev);
-
-#else
-
-static inline int mtk_smi_larb_get(struct device *larbdev)
-{
- return 0;
-}
-
-static inline void mtk_smi_larb_put(struct device *larbdev) { }
-
#endif
#endif
--
2.18.0
[PATCH v9 12/15] media: mtk-vcodec: enc: Remove mtk_vcodec_release_enc_pm
After this patchset, mtk_vcodec_release_enc_pm has only one line.
then remove that function, use pm_runtime_disable instead.
meanwhile, mtk_vcodec_init_enc_pm only operate for the clocks,
rename it from the _pm to _clk.
No functional change.
CC: Tiffany Lin
CC: Irui Wang
Signed-off-by: Yong Wu
---
drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 6 +++---
drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 8 +---
drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.h | 3 +--
3 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
index 45d1870c83dd..136798051f21 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
@@ -272,7 +272,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
return PTR_ERR(dev->fw_handler);
dev->venc_pdata = of_device_get_match_data(&pdev->dev);
- ret = mtk_vcodec_init_enc_pm(dev);
+ ret = mtk_vcodec_init_enc_clk(dev);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to get mtk vcodec clock source!");
goto err_enc_pm;
@@ -384,7 +384,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
err_enc_alloc:
v4l2_device_unregister(&dev->v4l2_dev);
err_res:
- mtk_vcodec_release_enc_pm(dev);
+ pm_runtime_disable(&pdev->dev);
err_enc_pm:
mtk_vcodec_fw_release(dev->fw_handler);
return ret;
@@ -463,7 +463,7 @@ static int mtk_vcodec_enc_remove(struct platform_device
*pdev)
video_unregister_device(dev->vfd_enc);
v4l2_device_unregister(&dev->v4l2_dev);
- mtk_vcodec_release_enc_pm(dev);
+ pm_runtime_disable(&pdev->dev);
mtk_vcodec_fw_release(dev->fw_handler);
return 0;
}
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
index dffb190267ed..12637908e5f6 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
@@ -12,7 +12,7 @@
#include "mtk_vcodec_enc_pm.h"
#include "mtk_vcodec_util.h"
-int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
+int mtk_vcodec_init_enc_clk(struct mtk_vcodec_dev *mtkdev)
{
struct platform_device *pdev;
struct mtk_vcodec_pm *pm;
@@ -60,12 +60,6 @@ int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
return 0;
}
-void mtk_vcodec_release_enc_pm(struct mtk_vcodec_dev *mtkdev)
-{
- pm_runtime_disable(mtkdev->pm.dev);
-}
-
-
void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm)
{
struct mtk_vcodec_clk *enc_clk = &pm->venc_clk;
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.h
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.h
index b7ecdfd74823..bc455cefc0cd 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.h
@@ -9,8 +9,7 @@
#include "mtk_vcodec_drv.h"
-int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *dev);
-void mtk_vcodec_release_enc_pm(struct mtk_vcodec_dev *dev);
+int mtk_vcodec_init_enc_clk(struct mtk_vcodec_dev *dev);
void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm);
void mtk_vcodec_enc_clock_off(struct mtk_vcodec_pm *pm);
--
2.18.0
[PATCH v9 11/15] media: mtk-vcodec: dec: Remove mtk_vcodec_release_dec_pm
After this patchset, mtk_vcodec_release_dec_pm has only one line.
then remove that function. Use pm_runtime_disable directly instead.
For symmetry, move the pm_runtime_enable out from
mtk_vcodec_init_dec_pm, then mtk_vcodec_init_dec_pm only operate for
the clocks, rename it from the _pm to _clk.
No functional change.
CC: Tiffany Lin
CC: Yunfei Dong
Signed-off-by: Yong Wu
---
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 8 +---
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 9 +
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 3 +--
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index f87dc47d9e63..830c400b9830 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -240,12 +241,13 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
if (IS_ERR(dev->fw_handler))
return PTR_ERR(dev->fw_handler);
- ret = mtk_vcodec_init_dec_pm(dev);
+ ret = mtk_vcodec_init_dec_clk(dev);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to get mt vcodec clock source");
goto err_dec_pm;
}
+ pm_runtime_enable(&pdev->dev);
for (i = 0; i < NUM_MAX_VDEC_REG_BASE; i++) {
dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i);
if (IS_ERR((__force void *)dev->reg_base[i])) {
@@ -345,7 +347,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
err_dec_alloc:
v4l2_device_unregister(&dev->v4l2_dev);
err_res:
- mtk_vcodec_release_dec_pm(dev);
+ pm_runtime_disable(&pdev->dev);
err_dec_pm:
mtk_vcodec_fw_release(dev->fw_handler);
return ret;
@@ -371,7 +373,7 @@ static int mtk_vcodec_dec_remove(struct platform_device
*pdev)
video_unregister_device(dev->vfd_dec);
v4l2_device_unregister(&dev->v4l2_dev);
- mtk_vcodec_release_dec_pm(dev);
+ pm_runtime_disable(&pdev->dev);
mtk_vcodec_fw_release(dev->fw_handler);
return 0;
}
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
index d0bf9aa3b29d..3df87944e9a2 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -12,7 +12,7 @@
#include "mtk_vcodec_dec_pm.h"
#include "mtk_vcodec_util.h"
-int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
+int mtk_vcodec_init_dec_clk(struct mtk_vcodec_dev *mtkdev)
{
struct platform_device *pdev;
struct mtk_vcodec_pm *pm;
@@ -57,16 +57,9 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
return PTR_ERR(clk_info->vcodec_clk);
}
}
-
- pm_runtime_enable(&pdev->dev);
return 0;
}
-void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
-{
- pm_runtime_disable(dev->pm.dev);
-}
-
int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
{
int ret;
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
index 280aeaefdb65..dace91401e23 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
@@ -9,8 +9,7 @@
#include "mtk_vcodec_drv.h"
-int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev);
-void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev);
+int mtk_vcodec_init_dec_clk(struct mtk_vcodec_dev *dev);
int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm);
--
2.18.0
Re: [PATCH v4 0/6] Cleanups for the nomodeset kernel command line parameter logic
Hi
Am 12.11.21 um 11:22 schrieb Pekka Paalanen:
On Fri, 12 Nov 2021 11:09:13 +0100
Thomas Zimmermann wrote:
Hi
Am 12.11.21 um 10:39 schrieb Javier Martinez Canillas:
Hello Pekka,
On 11/12/21 09:56, Pekka Paalanen wrote:
[snip]
Hi,
these ideas make sense to me, so FWIW,
Acked-by: Pekka Paalanen
Thanks.
There is one nitpick I'd like to ask about:
+bool drm_get_modeset(void)
+{
+ return !drm_nomodeset;
+}
+EXPORT_SYMBOL(drm_get_modeset);
Doesn't "get" have a special meaning in the kernel land, like "take a
strong reference on an object and return it"?
That's a very good point.
As this is just returning bool without changing anything, the usual
word to use is "is". Since this function is also used at most once per
driver, which is rarely, it could have a long and descriptive name.
For example:
bool drm_is_modeset_driver_allowed(void);
I'd nominate
bool drm_native_drivers_enabled()
This is what HW-specific drivers want to query in their init/probing
code. The actual semantics of this decision is hidden from the driver.
It's also easier to read than the other name IMHO
Ok, but what is a "native driver"? Or a "non-native driver"?
Is that established kernel terminology?
I'd think a non-native driver is something that e.g. ndiswrapper is
loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
the driver that would not consult this function, right?
We use that term for hw-specific drivers. A 'non-native' driver would be
called generic or firmware driver.
My concern with the 'modeset' term is that it exposes an implementation
detail, which can mislead a driver to to the wrong thing: a HW-specifc
driver that disables it's modesetting functionality would pass the test
for (!modeset). But that's not what we want, we want to disable all of
the driver and not even load it.
How about we invert the test function and use something like
bool drm_firmware_drivers_only()
? HW-native drivers can do
if (drm_firmware_drivers_only())
return;
as early as possible. fbdev uses the flag FBINFO_MISC_FIRMWARE to mark
rsp drivers. So the terminology is there already.
Best regards
Thomas
Thanks,
pq
Best regards
Thomas
Yeah, naming is hard. Jani also mentioned that he didn't like this
function name, so I don't mind to re-spin the series only for that.
- "drm" is the namespace
- "is" implies it is a read-only boolean inspection
- "modeset" is the feature being checked
- "driver" implies it is supposed gate driver loading or
initialization, rather than modesets after drivers have loaded
- "allowed" says it is asking about general policy rather than what a
driver does
I believe that name is more verbose than needed. But don't have a
strong opinion and could use it if others agree.
Just a bikeshed, I'll leave it to actual kernel devs to say if this
would be more appropriate or worth it.
I think is worth it and better to do it now before the patches land, but
we could wait for others to chime in.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
[PATCH v9 10/15] media: mtk-vcodec: Get rid of mtk_smi_larb_get/put
MediaTek IOMMU has already added the device_link between the consumer
and smi-larb device. If the vcodec device call the pm_runtime_get_sync,
the smi-larb's pm_runtime_get_sync also be called automatically.
CC: Tiffany Lin
CC: Irui Wang
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
Acked-by: Tiffany Lin
Reviewed-by: Dafna Hirschfeld
---
.../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 37 +++-
.../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 --
.../platform/mtk-vcodec/mtk_vcodec_enc.c | 1 -
.../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 44 +++
4 files changed, 10 insertions(+), 75 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
index 6038db96f71c..d0bf9aa3b29d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -8,14 +8,12 @@
#include
#include
#include
-#include
#include "mtk_vcodec_dec_pm.h"
#include "mtk_vcodec_util.h"
int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
{
- struct device_node *node;
struct platform_device *pdev;
struct mtk_vcodec_pm *pm;
struct mtk_vcodec_clk *dec_clk;
@@ -26,18 +24,7 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
pm = &mtkdev->pm;
pm->mtkdev = mtkdev;
dec_clk = &pm->vdec_clk;
- node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
- if (!node) {
- mtk_v4l2_err("of_parse_phandle mediatek,larb fail!");
- return -1;
- }
- pdev = of_find_device_by_node(node);
- of_node_put(node);
- if (WARN_ON(!pdev)) {
- return -1;
- }
- pm->larbvdec = &pdev->dev;
pdev = mtkdev->plat_dev;
pm->dev = &pdev->dev;
@@ -47,14 +34,11 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
dec_clk->clk_info = devm_kcalloc(&pdev->dev,
dec_clk->clk_num, sizeof(*clk_info),
GFP_KERNEL);
- if (!dec_clk->clk_info) {
- ret = -ENOMEM;
- goto put_device;
- }
+ if (!dec_clk->clk_info)
+ return -ENOMEM;
} else {
mtk_v4l2_err("Failed to get vdec clock count");
- ret = -EINVAL;
- goto put_device;
+ return -EINVAL;
}
for (i = 0; i < dec_clk->clk_num; i++) {
@@ -63,29 +47,24 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
"clock-names", i, &clk_info->clk_name);
if (ret) {
mtk_v4l2_err("Failed to get clock name id = %d", i);
- goto put_device;
+ return ret;
}
clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
clk_info->clk_name);
if (IS_ERR(clk_info->vcodec_clk)) {
mtk_v4l2_err("devm_clk_get (%d)%s fail", i,
clk_info->clk_name);
- ret = PTR_ERR(clk_info->vcodec_clk);
- goto put_device;
+ return PTR_ERR(clk_info->vcodec_clk);
}
}
pm_runtime_enable(&pdev->dev);
return 0;
-put_device:
- put_device(pm->larbvdec);
- return ret;
}
void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
{
pm_runtime_disable(dev->pm.dev);
- put_device(dev->pm.larbvdec);
}
int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
@@ -122,11 +101,6 @@ void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
}
}
- ret = mtk_smi_larb_get(pm->larbvdec);
- if (ret) {
- mtk_v4l2_err("mtk_smi_larb_get larbvdec fail %d", ret);
- goto error;
- }
return;
error:
@@ -139,7 +113,6 @@ void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
struct mtk_vcodec_clk *dec_clk = &pm->vdec_clk;
int i = 0;
- mtk_smi_larb_put(pm->larbvdec);
for (i = dec_clk->clk_num - 1; i >= 0; i--)
clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
}
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
index c6c7672fecfb..64b73dd880ce 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
@@ -189,10 +189,7 @@ struct mtk_vcodec_clk {
*/
struct mtk_vcodec_pm {
struct mtk_vcodec_clk vdec_clk;
- struct device *larbvdec;
-
struct mtk_vcodec_clk venc_clk;
- struct device *larbvenc;
struct device *dev;
struct mtk_vcodec_dev *mtkdev;
};
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
b/drivers/medi
[PATCH v9 09/15] drm/mediatek: Get rid of mtk_smi_larb_get/put
MediaTek IOMMU has already added the device_link between the consumer
and smi-larb device. If the drm device call the pm_runtime_get_sync,
the smi-larb's pm_runtime_get_sync also be called automatically.
CC: CK Hu
CC: Philipp Zabel
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
Acked-by: Chun-Kuang Hu
Reviewed-by: Dafna Hirschfeld
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 10 --
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 36 ++---
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 -
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +--
4 files changed, 3 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 455ea23c6130..445c30cc823f 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -12,7 +12,6 @@
#include
#include
-#include
#include
#include
@@ -643,22 +642,14 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc
*crtc,
DRM_DEBUG_DRIVER("%s %d\n", __func__, crtc->base.id);
- ret = mtk_smi_larb_get(comp->larb_dev);
- if (ret) {
- DRM_ERROR("Failed to get larb: %d\n", ret);
- return;
- }
-
ret = pm_runtime_resume_and_get(comp->dev);
if (ret < 0) {
- mtk_smi_larb_put(comp->larb_dev);
DRM_DEV_ERROR(comp->dev, "Failed to enable power domain: %d\n",
ret);
return;
}
ret = mtk_crtc_ddp_hw_init(mtk_crtc);
if (ret) {
- mtk_smi_larb_put(comp->larb_dev);
pm_runtime_put(comp->dev);
return;
}
@@ -695,7 +686,6 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc
*crtc,
drm_crtc_vblank_off(crtc);
mtk_crtc_ddp_hw_fini(mtk_crtc);
- mtk_smi_larb_put(comp->larb_dev);
ret = pm_runtime_put(comp->dev);
if (ret < 0)
DRM_DEV_ERROR(comp->dev, "Failed to disable power domain:
%d\n", ret);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index 99cbf44463e4..48642e814370 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -414,37 +414,15 @@ unsigned int mtk_drm_find_possible_crtc_by_comp(struct
drm_device *drm,
return ret;
}
-static int mtk_ddp_get_larb_dev(struct device_node *node, struct mtk_ddp_comp
*comp,
- struct device *dev)
-{
- struct device_node *larb_node;
- struct platform_device *larb_pdev;
-
- larb_node = of_parse_phandle(node, "mediatek,larb", 0);
- if (!larb_node) {
- dev_err(dev, "Missing mediadek,larb phandle in %pOF node\n",
node);
- return -EINVAL;
- }
-
- larb_pdev = of_find_device_by_node(larb_node);
- if (!larb_pdev) {
- dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
- of_node_put(larb_node);
- return -EPROBE_DEFER;
- }
- of_node_put(larb_node);
- comp->larb_dev = &larb_pdev->dev;
-
- return 0;
-}
-
int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
enum mtk_ddp_comp_id comp_id)
{
struct platform_device *comp_pdev;
enum mtk_ddp_comp_type type;
struct mtk_ddp_comp_dev *priv;
+#if IS_REACHABLE(CONFIG_MTK_CMDQ)
int ret;
+#endif
if (comp_id < 0 || comp_id >= DDP_COMPONENT_ID_MAX)
return -EINVAL;
@@ -460,16 +438,6 @@ int mtk_ddp_comp_init(struct device_node *node, struct
mtk_ddp_comp *comp,
}
comp->dev = &comp_pdev->dev;
- /* Only DMA capable components need the LARB property */
- if (type == MTK_DISP_OVL ||
- type == MTK_DISP_OVL_2L ||
- type == MTK_DISP_RDMA ||
- type == MTK_DISP_WDMA) {
- ret = mtk_ddp_get_larb_dev(node, comp, comp->dev);
- if (ret)
- return ret;
- }
-
if (type == MTK_DISP_AAL ||
type == MTK_DISP_BLS ||
type == MTK_DISP_CCORR ||
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index bb914d976cf5..1b582262b682 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -70,7 +70,6 @@ struct mtk_ddp_comp_funcs {
struct mtk_ddp_comp {
struct device *dev;
int irq;
- struct device *larb_dev;
enum mtk_ddp_comp_id id;
const struct mtk_ddp_comp_funcs *funcs;
};
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index aec39724ebeb..c234293fc2c3 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -603,11 +603,8 @@ static int mtk_drm_prob
[PATCH v9 08/15] drm/mediatek: Add pm runtime support for ovl and rdma
From: Yongqiang Niu
Prepare for smi cleaning up "mediatek,larb".
Display use the dispsys device to call pm_rumtime_get_sync before.
This patch add pm_runtime_xx with ovl and rdma device whose nodes has
"iommus" property, then display could help pm_runtime_get for smi via
ovl or rdma device.
CC: CK Hu
Signed-off-by: Yongqiang Niu
Signed-off-by: Yong Wu
(Yong: Use pm_runtime_resume_and_get instead of pm_runtime_get_sync)
Acked-by: Chun-Kuang Hu
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 8 +++-
drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 9 -
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 13 -
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 5326989d5206..716eac6831f2 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include "mtk_disp_drv.h"
@@ -414,9 +415,13 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
return ret;
}
+ pm_runtime_enable(dev);
+
ret = component_add(dev, &mtk_disp_ovl_component_ops);
- if (ret)
+ if (ret) {
+ pm_runtime_disable(dev);
dev_err(dev, "Failed to add component: %d\n", ret);
+ }
return ret;
}
@@ -424,6 +429,7 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
static int mtk_disp_ovl_remove(struct platform_device *pdev)
{
component_del(&pdev->dev, &mtk_disp_ovl_component_ops);
+ pm_runtime_disable(&pdev->dev);
return 0;
}
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
index 75d7f45579e2..251f034acb09 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include "mtk_disp_drv.h"
@@ -327,9 +328,13 @@ static int mtk_disp_rdma_probe(struct platform_device
*pdev)
platform_set_drvdata(pdev, priv);
+ pm_runtime_enable(dev);
+
ret = component_add(dev, &mtk_disp_rdma_component_ops);
- if (ret)
+ if (ret) {
+ pm_runtime_disable(dev);
dev_err(dev, "Failed to add component: %d\n", ret);
+ }
return ret;
}
@@ -338,6 +343,8 @@ static int mtk_disp_rdma_remove(struct platform_device
*pdev)
{
component_del(&pdev->dev, &mtk_disp_rdma_component_ops);
+ pm_runtime_disable(&pdev->dev);
+
return 0;
}
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 5f81489fc60c..455ea23c6130 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -649,9 +649,17 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc
*crtc,
return;
}
+ ret = pm_runtime_resume_and_get(comp->dev);
+ if (ret < 0) {
+ mtk_smi_larb_put(comp->larb_dev);
+ DRM_DEV_ERROR(comp->dev, "Failed to enable power domain: %d\n",
ret);
+ return;
+ }
+
ret = mtk_crtc_ddp_hw_init(mtk_crtc);
if (ret) {
mtk_smi_larb_put(comp->larb_dev);
+ pm_runtime_put(comp->dev);
return;
}
@@ -664,7 +672,7 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc
*crtc,
{
struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
- int i;
+ int i, ret;
DRM_DEBUG_DRIVER("%s %d\n", __func__, crtc->base.id);
if (!mtk_crtc->enabled)
@@ -688,6 +696,9 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc
*crtc,
drm_crtc_vblank_off(crtc);
mtk_crtc_ddp_hw_fini(mtk_crtc);
mtk_smi_larb_put(comp->larb_dev);
+ ret = pm_runtime_put(comp->dev);
+ if (ret < 0)
+ DRM_DEV_ERROR(comp->dev, "Failed to disable power domain:
%d\n", ret);
mtk_crtc->enabled = false;
}
--
2.18.0
[PATCH v9 07/15] media: mtk-mdp: Get rid of mtk_smi_larb_get/put
MediaTek IOMMU has already added the device_link between the consumer
and smi-larb device. If the mdp device call the pm_runtime_get_sync,
the smi-larb's pm_runtime_get_sync also be called automatically.
CC: Minghsiu Tsai
CC: Houlong Wei
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
Reviewed-by: Houlong Wei
Reviewed-by: Dafna Hirschfeld
---
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 40 ---
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 -
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 1 -
3 files changed, 43 deletions(-)
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index b3426a551bea..1e3833f1c9ae 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -9,7 +9,6 @@
#include
#include
#include
-#include
#include "mtk_mdp_comp.h"
@@ -18,14 +17,6 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct
mtk_mdp_comp *comp)
{
int i, err;
- if (comp->larb_dev) {
- err = mtk_smi_larb_get(comp->larb_dev);
- if (err)
- dev_err(dev,
- "failed to get larb, err %d. type:%d\n",
- err, comp->type);
- }
-
for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
if (IS_ERR(comp->clk[i]))
continue;
@@ -46,17 +37,12 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct
mtk_mdp_comp *comp)
continue;
clk_disable_unprepare(comp->clk[i]);
}
-
- if (comp->larb_dev)
- mtk_smi_larb_put(comp->larb_dev);
}
int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
struct mtk_mdp_comp *comp,
enum mtk_mdp_comp_type comp_type)
{
- struct device_node *larb_node;
- struct platform_device *larb_pdev;
int ret;
int i;
@@ -77,32 +63,6 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node
*node,
break;
}
- /* Only DMA capable components need the LARB property */
- comp->larb_dev = NULL;
- if (comp->type != MTK_MDP_RDMA &&
- comp->type != MTK_MDP_WDMA &&
- comp->type != MTK_MDP_WROT)
- return 0;
-
- larb_node = of_parse_phandle(node, "mediatek,larb", 0);
- if (!larb_node) {
- dev_err(dev,
- "Missing mediadek,larb phandle in %pOF node\n", node);
- ret = -EINVAL;
- goto put_dev;
- }
-
- larb_pdev = of_find_device_by_node(larb_node);
- if (!larb_pdev) {
- dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
- of_node_put(larb_node);
- ret = -EPROBE_DEFER;
- goto put_dev;
- }
- of_node_put(larb_node);
-
- comp->larb_dev = &larb_pdev->dev;
-
return 0;
put_dev:
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index 7897766c96bb..ae41dd3cd72a 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -26,14 +26,12 @@ enum mtk_mdp_comp_type {
* @node: list node to track sibing MDP components
* @dev_node: component device node
* @clk: clocks required for component
- * @larb_dev: SMI device required for component
* @type: component type
*/
struct mtk_mdp_comp {
struct list_headnode;
struct device_node *dev_node;
struct clk *clk[2];
- struct device *larb_dev;
enum mtk_mdp_comp_type type;
};
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 976aa1f4829b..70a8eab16863 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -17,7 +17,6 @@
#include
#include
#include
-#include
#include "mtk_mdp_core.h"
#include "mtk_mdp_m2m.h"
--
2.18.0
[PATCH v9 06/15] media: mtk-jpeg: Get rid of mtk_smi_larb_get/put
MediaTek IOMMU has already added device_link between the consumer
and smi-larb device. If the jpg device call the pm_runtime_get_sync,
the smi-larb's pm_runtime_get_sync also be called automatically.
After removing the larb_get operations, then mtk_jpeg_clk_init is
also unnecessary. Remove it too.
CC: Rick Chang
CC: Xia Jiang
Signed-off-by: Yong Wu
Reviewed-by: Evan Green
Acked-by: Rick Chang
Reviewed-by: Dafna Hirschfeld
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
.../media/platform/mtk-jpeg/mtk_jpeg_core.c | 45 +--
.../media/platform/mtk-jpeg/mtk_jpeg_core.h | 2 -
2 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index a89c7b206eef..4fea2c512434 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -22,7 +22,6 @@
#include
#include
#include
-#include
#include "mtk_jpeg_enc_hw.h"
#include "mtk_jpeg_dec_hw.h"
@@ -1055,10 +1054,6 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
{
int ret;
- ret = mtk_smi_larb_get(jpeg->larb);
- if (ret)
- dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret);
-
ret = clk_bulk_prepare_enable(jpeg->variant->num_clks,
jpeg->variant->clks);
if (ret)
@@ -1069,7 +1064,6 @@ static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg)
{
clk_bulk_disable_unprepare(jpeg->variant->num_clks,
jpeg->variant->clks);
- mtk_smi_larb_put(jpeg->larb);
}
static irqreturn_t mtk_jpeg_enc_done(struct mtk_jpeg_dev *jpeg)
@@ -1284,35 +1278,6 @@ static struct clk_bulk_data mtk_jpeg_clocks[] = {
{ .id = "jpgenc" },
};
-static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg)
-{
- struct device_node *node;
- struct platform_device *pdev;
- int ret;
-
- node = of_parse_phandle(jpeg->dev->of_node, "mediatek,larb", 0);
- if (!node)
- return -EINVAL;
- pdev = of_find_device_by_node(node);
- if (WARN_ON(!pdev)) {
- of_node_put(node);
- return -EINVAL;
- }
- of_node_put(node);
-
- jpeg->larb = &pdev->dev;
-
- ret = devm_clk_bulk_get(jpeg->dev, jpeg->variant->num_clks,
- jpeg->variant->clks);
- if (ret) {
- dev_err(&pdev->dev, "failed to get jpeg clock:%d\n", ret);
- put_device(&pdev->dev);
- return ret;
- }
-
- return 0;
-}
-
static void mtk_jpeg_job_timeout_work(struct work_struct *work)
{
struct mtk_jpeg_dev *jpeg = container_of(work, struct mtk_jpeg_dev,
@@ -1333,11 +1298,6 @@ static void mtk_jpeg_job_timeout_work(struct work_struct
*work)
v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
}
-static inline void mtk_jpeg_clk_release(struct mtk_jpeg_dev *jpeg)
-{
- put_device(jpeg->larb);
-}
-
static int mtk_jpeg_probe(struct platform_device *pdev)
{
struct mtk_jpeg_dev *jpeg;
@@ -1376,7 +1336,8 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
goto err_req_irq;
}
- ret = mtk_jpeg_clk_init(jpeg);
+ ret = devm_clk_bulk_get(jpeg->dev, jpeg->variant->num_clks,
+ jpeg->variant->clks);
if (ret) {
dev_err(&pdev->dev, "Failed to init clk, err %d\n", ret);
goto err_clk_init;
@@ -1442,7 +1403,6 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
v4l2_device_unregister(&jpeg->v4l2_dev);
err_dev_register:
- mtk_jpeg_clk_release(jpeg);
err_clk_init:
@@ -1460,7 +1420,6 @@ static int mtk_jpeg_remove(struct platform_device *pdev)
video_device_release(jpeg->vdev);
v4l2_m2m_release(jpeg->m2m_dev);
v4l2_device_unregister(&jpeg->v4l2_dev);
- mtk_jpeg_clk_release(jpeg);
return 0;
}
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index 595f7f10c9fd..3e4811a41ba2 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -85,7 +85,6 @@ struct mtk_jpeg_variant {
* @alloc_ctx: videobuf2 memory allocator's context
* @vdev: video device node for jpeg mem2mem mode
* @reg_base: JPEG registers mapping
- * @larb: SMI device
* @job_timeout_work: IRQ timeout structure
* @variant: driver variant to be used
*/
@@ -99,7 +98,6 @@ struct mtk_jpeg_dev {
void*alloc_ctx;
struct video_device *vdev;
void __iomem*reg_base;
- struct device *larb;
struct delayed_work job_timeout_work;
const struct mtk_jpeg_variant *variant;
};
--
2.18.0
[PATCH v9 05/15] iommu/mediatek: Add device_link between the consumer and the larb devices
MediaTek IOMMU-SMI diagram is like below. all the consumer connect with
smi-larb, then connect with smi-common.
M4U
|
smi-common
|
-
| |...
| |
larb1 larb2
| |
vdec venc
When the consumer works, it should enable the smi-larb's power which
also need enable the smi-common's power firstly.
Thus, First of all, use the device link connect the consumer and the
smi-larbs. then add device link between the smi-larb and smi-common.
This patch adds device_link between the consumer and the larbs.
When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
pm_runtime_xx to keep the original status of clocks. It can avoid two
issues:
1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
all the clocks are enabled before entering kernel, but the clocks for
display HW(always in larb0) will be gated after clk_enable and clk_disable
called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
operation happened before display driver probe. At that time, the display
HW will be abnormal.
2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
pm_runtime_xx to avoid the deadlock.
Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
device_link_removed should be added explicitly.
Meanwhile, Currently we don't have a device connect with 2 larbs at the
same time. Disallow this case, print the error log.
[1] https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
[2] https://lore.kernel.org/patchwork/patch/1086569/
Suggested-by: Tomasz Figa
Signed-off-by: Yong Wu
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
drivers/iommu/mtk_iommu.c| 30 ++
drivers/iommu/mtk_iommu_v1.c | 29 -
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 0033c0634e5e..5fed0b64ddd0 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -560,22 +560,52 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct mtk_iommu_data *data;
+ struct device_link *link;
+ struct device *larbdev;
+ unsigned int larbid, larbidx, i;
if (!fwspec || fwspec->ops != &mtk_iommu_ops)
return ERR_PTR(-ENODEV); /* Not a iommu client device */
data = dev_iommu_priv_get(dev);
+ /*
+* Link the consumer device with the smi-larb device(supplier).
+* The device that connects with each a larb is a independent HW.
+* All the ports in each a device should be in the same larbs.
+*/
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ for (i = 1; i < fwspec->num_ids; i++) {
+ larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
+ if (larbid != larbidx) {
+ dev_err(dev, "Can only use one larb. Fail@larb%d-%d.\n",
+ larbid, larbidx);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+ larbdev = data->larb_imu[larbid].dev;
+ link = device_link_add(dev, larbdev,
+ DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+ if (!link)
+ dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
return &data->iommu;
}
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct mtk_iommu_data *data;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != &mtk_iommu_ops)
return;
+ data = dev_iommu_priv_get(dev);
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ larbdev = data->larb_imu[larbid].dev;
+ device_link_remove(dev, larbdev);
+
iommu_fwspec_free(dev);
}
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 4089077256f4..4052aad75a81 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -423,7 +423,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct of_phandle_args iommu_spec;
struct mtk_iommu_data *data;
- int err, idx = 0;
+ int err, idx = 0, larbid, larbidx;
+ struct device_link *link;
+ struct device *larbdev;
/*
* In the deferred case, free the existed fwspec.
@@ -453,6 +455,23 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
data = dev_iommu_priv_get(dev);
+ /* Link the consumer device with the smi-larb device(supplier) */
+ larbid = mt2701_m4u_to_larb(fwspec->ids[0]);
+ for (idx = 1; idx < fwspec->num_ids; idx++) {
+ larbidx = mt2701_m4u_to_larb(fwspec->ids[idx]);
+
