From: Ville Syrjälä ville.syrjala@linux.intel.com
Another respin of the possible_clones/crtcs fixing.
Changes based on v2 review: - introduce drm_mode_config_validate() - WARN for possible_clones!=0 when the encoder itself isn't in the mask - update the documentation to match the code
Other changes: - sligth refactoring of the code to make it more consistent - add a patch to fixup possible_crtcs too (might not be needed but included it just in case).
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch
Ville Syrjälä (7): drm: Include the encoder itself in possible_clones drm/gma500: Sanitize possible_clones drm/exynos: Use drm_encoder_mask() drm/imx: Remove the bogus possible_clones setup drm: Validate encoder->possible_clones drm: Validate encoder->possible_crtcs drm: Allow drivers to leave encoder->possible_crtcs==0
drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_drv.c | 3 + drivers/gpu/drm/drm_mode_config.c | 97 +++++++++++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 +- drivers/gpu/drm/gma500/framebuffer.c | 16 ++-- drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 4 +- drivers/gpu/drm/imx/imx-drm-core.c | 4 +- include/drm/drm_encoder.h | 12 ++- 8 files changed, 125 insertions(+), 17 deletions(-)
From: Ville Syrjälä ville.syrjala@linux.intel.com
The docs say possible_clones should always include the encoder itself. Since most drivers don't want to deal with the complexities of cloning let's allow them to set possible_clones=0 and instead we'll fix that up in the core.
We can't put this special case into drm_encoder_init() because drivers will have to fill up possible_clones after adding all the relevant encoders. Otherwise they wouldn't know the proper encoder indexes to use. So we'll just do it just before registering the device.
v2: Don't set the bit if possible_clones!=0 so that the validation (coming soon) will WARN (Thomas) Fix up the docs to allow possible_clones==0 (Daniel) .late_register() is too late, introduce drm_mode_config_validate() which gets called _before_ we register the char device (Daniel)
Acked-by: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_drv.c | 3 +++ drivers/gpu/drm/drm_mode_config.c | 19 +++++++++++++++++++ include/drm/drm_encoder.h | 4 +++- 4 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 16f2413403aa..ace363b4f82b 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -82,6 +82,7 @@ int drm_mode_setcrtc(struct drm_device *dev, /* drm_mode_config.c */ int drm_modeset_register_all(struct drm_device *dev); void drm_modeset_unregister_all(struct drm_device *dev); +void drm_mode_config_validate(struct drm_device *dev);
/* drm_modes.c */ const char *drm_get_mode_status_name(enum drm_mode_status status); diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 7b1a628d1f6e..65a0acb79323 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -946,6 +946,9 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) struct drm_driver *driver = dev->driver; int ret;
+ if (!driver->load) + drm_mode_config_validate(dev); + if (drm_dev_needs_global_mutex(dev)) mutex_lock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 08e6eff6a179..75e357c7e84d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -532,3 +532,22 @@ void drm_mode_config_cleanup(struct drm_device *dev) drm_modeset_lock_fini(&dev->mode_config.connection_mutex); } EXPORT_SYMBOL(drm_mode_config_cleanup); + +/* + * For some reason we want the encoder itself included in + * possible_clones. Make life easy for drivers by allowing them + * to leave possible_clones unset if no cloning is possible. + */ +static void fixup_encoder_possible_clones(struct drm_encoder *encoder) +{ + if (encoder->possible_clones == 0) + encoder->possible_clones = drm_encoder_mask(encoder); +} + +void drm_mode_config_validate(struct drm_device *dev) +{ + struct drm_encoder *encoder; + + drm_for_each_encoder(encoder, dev) + fixup_encoder_possible_clones(encoder); +} diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 5623994b6e9e..22d6cdf729f1 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -159,7 +159,9 @@ struct drm_encoder { * encoders can be used in a cloned configuration, they both should have * each another bits set. * - * In reality almost every driver gets this wrong. + * As an exception to the above rule if the driver doesn't implement + * any cloning it can leave @possible_clones set to 0. The core will + * automagically fix this up by setting the bit for the encoder itself. * * Note that since encoder objects can't be hotplugged the assigned indices * are stable and hence known before registering all objects.
On Tue, Feb 11, 2020 at 06:22:02PM +0200, Ville Syrjala wrote:
Looks solid.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
I doubt the DP+DP and SDVO+SDVO cloning works for this driver. i915 at least doesn't do those. Truthfully there could be some very specific circumstances where some of them would do doable, but genereally it's too much pain to deal with so we've chose not to bother. Let's use the same approach for gma500.
Also the LVDS+LVDS and DSI+DSI cases probably don't really exist as there is one of each at most.
This does mean we'll now leave possible_clones at 0 for these encoder types whereas previosuly we included the encoder itself in the bitmask. But that's fine as the core now treaks 0 as a special case and adds the encoder itself into the final bitmask reported to userspace.
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/gma500/framebuffer.c | 16 ++++++++-------- drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 1459076d1980..6ca4e6ded96c 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -581,31 +581,31 @@ static void psb_setup_outputs(struct drm_device *dev) break; case INTEL_OUTPUT_SDVO: crtc_mask = dev_priv->ops->sdvo_mask; - clone_mask = (1 << INTEL_OUTPUT_SDVO); + clone_mask = 0; break; case INTEL_OUTPUT_LVDS: - crtc_mask = dev_priv->ops->lvds_mask; - clone_mask = (1 << INTEL_OUTPUT_LVDS); + crtc_mask = dev_priv->ops->lvds_mask; + clone_mask = 0; break; case INTEL_OUTPUT_MIPI: crtc_mask = (1 << 0); - clone_mask = (1 << INTEL_OUTPUT_MIPI); + clone_mask = 0; break; case INTEL_OUTPUT_MIPI2: crtc_mask = (1 << 2); - clone_mask = (1 << INTEL_OUTPUT_MIPI2); + clone_mask = 0; break; case INTEL_OUTPUT_HDMI: - crtc_mask = dev_priv->ops->hdmi_mask; + crtc_mask = dev_priv->ops->hdmi_mask; clone_mask = (1 << INTEL_OUTPUT_HDMI); break; case INTEL_OUTPUT_DISPLAYPORT: crtc_mask = (1 << 0) | (1 << 1); - clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT); + clone_mask = 0; break; case INTEL_OUTPUT_EDP: crtc_mask = (1 << 1); - clone_mask = (1 << INTEL_OUTPUT_EDP); + clone_mask = 0; } encoder->possible_crtcs = crtc_mask; encoder->possible_clones = diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c index d4c65f268922..187817e0c004 100644 --- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c +++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c @@ -1006,10 +1006,10 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev, /*set possible crtcs and clones*/ if (dsi_connector->pipe) { encoder->possible_crtcs = (1 << 2); - encoder->possible_clones = (1 << 1); + encoder->possible_clones = 0; } else { encoder->possible_crtcs = (1 << 0); - encoder->possible_clones = (1 << 0); + encoder->possible_clones = 0; }
dsi_connector->base.encoder = &dpi_output->base.base;
From: Ville Syrjälä ville.syrjala@linux.intel.com
Replace the hand rolled encoder bitmask thing with drm_encoder_mask()
Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Acked-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index ba0f868b2477..57defeb44522 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev) struct drm_encoder *encoder; struct drm_device *drm; unsigned int clone_mask; - int cnt, ret; + int ret;
drm = drm_dev_alloc(&exynos_drm_driver, dev); if (IS_ERR(drm)) @@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev) exynos_drm_mode_config_init(drm);
/* setup possible_clones. */ - cnt = 0; clone_mask = 0; list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) - clone_mask |= (1 << (cnt++)); + clone_mask |= drm_encoder_mask(encoder);
list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) encoder->possible_clones = clone_mask;
20. 2. 12. 오전 1:22에 Ville Syrjala 이(가) 쓴 글:
Acked-by: Inki Dae inki.dae@samsung.com
THanks, Inki Dae
From: Ville Syrjälä ville.syrjala@linux.intel.com
It's not at all clear what cloning options this driver supports. So let's just clear possible_clones instead of setting it to some bogus value.
v2: Adjust the FIXME (Daniel)
Cc: Philipp Zabel p.zabel@pengutronix.de Acked-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index da87c70e413b..9e8e0b703498 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -139,8 +139,8 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
encoder->possible_crtcs = crtc_mask;
- /* FIXME: this is the mask of outputs which can clone this output. */ - encoder->possible_clones = ~0; + /* FIXME: cloning support not clear, disable it all for now */ + encoder->possible_clones = 0;
return 0; }
From: Ville Syrjälä ville.syrjala@linux.intel.com
Many drivers are populating encoder->possible_clones wrong. Let's persuade them to get it right by adding some loud WARNs.
We'll cross check the bits between any two encoders. So either both encoders can clone with the other, or neither can.
We'll also complain about effectively empty possible_clones, and possible_clones containing bits for encoders that don't exist.
v2: encoder->possible_clones now includes the encoder itelf v3: Move to drm_mode_config_validate() (Daniel) Document that you get a WARN when this is wrong (Daniel) Extract full_encoder_mask()
Acked-by: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++ include/drm/drm_encoder.h | 2 ++ 2 files changed, 42 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 75e357c7e84d..afc91447293a 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev) } EXPORT_SYMBOL(drm_mode_config_cleanup);
+static u32 full_encoder_mask(struct drm_device *dev) +{ + struct drm_encoder *encoder; + u32 encoder_mask = 0; + + drm_for_each_encoder(encoder, dev) + encoder_mask |= drm_encoder_mask(encoder); + + return encoder_mask; +} + /* * For some reason we want the encoder itself included in * possible_clones. Make life easy for drivers by allowing them @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder) encoder->possible_clones = drm_encoder_mask(encoder); }
+static void validate_encoder_possible_clones(struct drm_encoder *encoder) +{ + struct drm_device *dev = encoder->dev; + u32 encoder_mask = full_encoder_mask(dev); + struct drm_encoder *other; + + drm_for_each_encoder(other, dev) { + WARN(!(encoder->possible_clones & drm_encoder_mask(other)) != + !(other->possible_clones & drm_encoder_mask(encoder)), + "possible_clones mismatch: " + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. " + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n", + encoder->base.id, encoder->name, + drm_encoder_mask(encoder), encoder->possible_clones, + other->base.id, other->name, + drm_encoder_mask(other), other->possible_clones); + } + + WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 || + (encoder->possible_clones & ~encoder_mask) != 0, + "Bogus possible_clones: " + "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n", + encoder->base.id, encoder->name, + encoder->possible_clones, encoder_mask); +} + void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder;
drm_for_each_encoder(encoder, dev) fixup_encoder_possible_clones(encoder); + + drm_for_each_encoder(encoder, dev) + validate_encoder_possible_clones(encoder); } diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 22d6cdf729f1..3741963b9587 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -163,6 +163,8 @@ struct drm_encoder { * any cloning it can leave @possible_clones set to 0. The core will * automagically fix this up by setting the bit for the encoder itself. * + * You will get a WARN if you get this wrong in the driver. + * * Note that since encoder objects can't be hotplugged the assigned indices * are stable and hence known before registering all objects. */
On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
I wonder whether we should start to have some unit tests for stuff like this, like set up broken driver, make sure we have a WARN in dmesg. But ideally we'd do that with the mocking stuff Kunit hopefully has soon.
</idle musings>
Bikeshed: !! as canonical "make this a bool value" might be slightly clearer, but whatever.
Since it's next to each another double-checking that the fixup did add the self-clone is probably too much :-)
Nice.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
On Tue, Feb 11, 2020 at 06:02:33PM +0100, Daniel Vetter wrote:
Can repaint.
I changed the fixup to be just if (possible_clones == 0) possible_clones = drm_encoder_mask();
So if the driver tries to set it up but fails and forgets the encoder itself this WARN will still trip.
On Tue, Feb 11, 2020 at 07:13:31PM +0200, Ville Syrjälä wrote:
Duh, I was just blind, everything is looking good. -Daniel
From: Ville Syrjälä ville.syrjala@linux.intel.com
WARN if the encoder possible_crtcs is effectively empty or contains bits for non-existing crtcs.
v2: Move to drm_mode_config_validate() (Daniel) Make the docs say we WARN when this is wrong (Daniel) Extract full_crtc_mask()
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_mode_config.c | 27 ++++++++++++++++++++++++++- include/drm/drm_encoder.h | 2 +- 2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index afc91447293a..4c1b350ddb95 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -581,6 +581,29 @@ static void validate_encoder_possible_clones(struct drm_encoder *encoder) encoder->possible_clones, encoder_mask); }
+static u32 full_crtc_mask(struct drm_device *dev) +{ + struct drm_crtc *crtc; + u32 crtc_mask = 0; + + drm_for_each_crtc(crtc, dev) + crtc_mask |= drm_crtc_mask(crtc); + + return crtc_mask; +} + +static void validate_encoder_possible_crtcs(struct drm_encoder *encoder) +{ + u32 crtc_mask = full_crtc_mask(encoder->dev); + + WARN((encoder->possible_crtcs & crtc_mask) == 0 || + (encoder->possible_crtcs & ~crtc_mask) != 0, + "Bogus possible_crtcs: " + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n", + encoder->base.id, encoder->name, + encoder->possible_crtcs, crtc_mask); +} + void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct drm_device *dev) drm_for_each_encoder(encoder, dev) fixup_encoder_possible_clones(encoder);
- drm_for_each_encoder(encoder, dev) + drm_for_each_encoder(encoder, dev) { validate_encoder_possible_clones(encoder); + validate_encoder_possible_crtcs(encoder); + } } diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 3741963b9587..b236269f41ac 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -142,7 +142,7 @@ struct drm_encoder { * the bits for all &drm_crtc objects this encoder can be connected to * before calling drm_dev_register(). * - * In reality almost every driver gets this wrong. + * You will get a WARN if you get this wrong in the driver. * * Note that since CRTC objects can't be hotplugged the assigned indices * are stable and hence known before registering all objects.
On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
When pushing the fixup needs to be applied before the validation patch here, because we don't want to anger the bisect gods.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
I think with the fixup we should be good enough with the existing nonsense in drivers. Fingers crossed. -Daniel
On 11.02.20 18:04, Daniel Vetter wrote:
Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
[ 14.033246] ------------[ cut here ]------------ [ 14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf (full crtc mask=0x7) [ 14.033279] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm] [ 14.033279] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E) [ 14.033306] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E) [ 14.033310] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G E 5.9.0-rc3+ #2 [ 14.033311] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020 [ 14.033324] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm] [ 14.033327] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d [ 14.033328] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282 [ 14.033329] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027 [ 14.033330] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8 [ 14.033331] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724 [ 14.033331] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf8b800 [ 14.033332] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f [ 14.033333] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000 [ 14.033334] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 14.033335] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0 [ 14.033335] Call Trace: [ 14.033350] drm_dev_register+0x117/0x1e0 [drm] [ 14.033423] amdgpu_pci_probe+0x134/0x200 [amdgpu] [ 14.033428] local_pci_probe+0x42/0x90 [ 14.033430] pci_device_probe+0x108/0x1c0 [ 14.033433] really_probe+0xef/0x4a0 [ 14.033435] driver_probe_device+0xde/0x150 [ 14.033436] device_driver_attach+0x4f/0x60 [ 14.033438] __driver_attach+0x9a/0x140 [ 14.033439] ? device_driver_attach+0x60/0x60 [ 14.033441] bus_for_each_dev+0x76/0xc0 [ 14.033443] ? klist_add_tail+0x3b/0x70 [ 14.033445] bus_add_driver+0x144/0x220 [ 14.033446] ? 0xffffffffc0949000 [ 14.033447] driver_register+0x5b/0xf0 [ 14.033448] ? 0xffffffffc0949000 [ 14.033451] do_one_initcall+0x46/0x1f4 [ 14.033454] ? _cond_resched+0x15/0x40 [ 14.033456] ? kmem_cache_alloc_trace+0x40/0x440 [ 14.033459] ? do_init_module+0x22/0x213 [ 14.033460] do_init_module+0x5b/0x213 [ 14.033462] load_module+0x258c/0x2d30 [ 14.033465] ? __kernel_read+0xf5/0x160 [ 14.033467] ? __do_sys_finit_module+0xe9/0x110 [ 14.033468] __do_sys_finit_module+0xe9/0x110 [ 14.033471] do_syscall_64+0x33/0x80 [ 14.033473] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 14.033474] RIP: 0033:0x7feacf1bef59 [ 14.033477] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48 [ 14.033477] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 14.033478] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59 [ 14.033479] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015 [ 14.033479] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000 [ 14.033480] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000 [ 14.033480] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410 [ 14.033482] ---[ end trace 16aeaa08847a13d8 ]--- [ 14.033483] ------------[ cut here ]------------ [ 14.033484] Bogus possible_crtcs: [ENCODER:69:TMDS-69] possible_crtcs=0xf (full crtc mask=0x7) [ 14.033507] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm] [ 14.033507] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E) [ 14.033522] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E) [ 14.033524] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E 5.9.0-rc3+ #2 [ 14.033525] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020 [ 14.033538] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm] [ 14.033539] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d [ 14.033540] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282 [ 14.033540] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027 [ 14.033541] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8 [ 14.033542] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724 [ 14.033542] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf89800 [ 14.033543] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f [ 14.033544] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000 [ 14.033544] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 14.033545] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0 [ 14.033545] Call Trace: [ 14.033557] drm_dev_register+0x117/0x1e0 [drm] [ 14.033625] amdgpu_pci_probe+0x134/0x200 [amdgpu] [ 14.033627] local_pci_probe+0x42/0x90 [ 14.033629] pci_device_probe+0x108/0x1c0 [ 14.033630] really_probe+0xef/0x4a0 [ 14.033632] driver_probe_device+0xde/0x150 [ 14.033633] device_driver_attach+0x4f/0x60 [ 14.033634] __driver_attach+0x9a/0x140 [ 14.033635] ? device_driver_attach+0x60/0x60 [ 14.033636] bus_for_each_dev+0x76/0xc0 [ 14.033638] ? klist_add_tail+0x3b/0x70 [ 14.033639] bus_add_driver+0x144/0x220 [ 14.033640] ? 0xffffffffc0949000 [ 14.033641] driver_register+0x5b/0xf0 [ 14.033642] ? 0xffffffffc0949000 [ 14.033643] do_one_initcall+0x46/0x1f4 [ 14.033645] ? _cond_resched+0x15/0x40 [ 14.033646] ? kmem_cache_alloc_trace+0x40/0x440 [ 14.033648] ? do_init_module+0x22/0x213 [ 14.033649] do_init_module+0x5b/0x213 [ 14.033650] load_module+0x258c/0x2d30 [ 14.033652] ? __kernel_read+0xf5/0x160 [ 14.033654] ? __do_sys_finit_module+0xe9/0x110 [ 14.033655] __do_sys_finit_module+0xe9/0x110 [ 14.033657] do_syscall_64+0x33/0x80 [ 14.033659] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 14.033660] RIP: 0033:0x7feacf1bef59 [ 14.033661] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48 [ 14.033662] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 14.033663] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59 [ 14.033663] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015 [ 14.033664] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000 [ 14.033664] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000 [ 14.033665] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410 [ 14.033666] ---[ end trace 16aeaa08847a13d9 ]--- [ 14.033667] ------------[ cut here ]------------ [ 14.033668] Bogus possible_crtcs: [ENCODER:73:TMDS-73] possible_crtcs=0xf (full crtc mask=0x7) [ 14.033690] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm] [ 14.033690] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E) [ 14.033704] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E) [ 14.033706] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E 5.9.0-rc3+ #2 [ 14.033707] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020 [ 14.033719] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm] [ 14.033721] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d [ 14.033721] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282 [ 14.033722] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027 [ 14.033723] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8 [ 14.033723] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724 [ 14.033724] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf7c000 [ 14.033724] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f [ 14.033725] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000 [ 14.033726] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 14.033726] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0 [ 14.033727] Call Trace: [ 14.033739] drm_dev_register+0x117/0x1e0 [drm] [ 14.033806] amdgpu_pci_probe+0x134/0x200 [amdgpu] [ 14.033808] local_pci_probe+0x42/0x90 [ 14.033810] pci_device_probe+0x108/0x1c0 [ 14.033811] really_probe+0xef/0x4a0 [ 14.033813] driver_probe_device+0xde/0x150 [ 14.033814] device_driver_attach+0x4f/0x60 [ 14.033815] __driver_attach+0x9a/0x140 [ 14.033816] ? device_driver_attach+0x60/0x60 [ 14.033817] bus_for_each_dev+0x76/0xc0 [ 14.033818] ? klist_add_tail+0x3b/0x70 [ 14.033820] bus_add_driver+0x144/0x220 [ 14.033821] ? 0xffffffffc0949000 [ 14.033822] driver_register+0x5b/0xf0 [ 14.033823] ? 0xffffffffc0949000 [ 14.033824] do_one_initcall+0x46/0x1f4 [ 14.033825] ? _cond_resched+0x15/0x40 [ 14.033827] ? kmem_cache_alloc_trace+0x40/0x440 [ 14.033828] ? do_init_module+0x22/0x213 [ 14.033829] do_init_module+0x5b/0x213 [ 14.033831] load_module+0x258c/0x2d30 [ 14.033833] ? __kernel_read+0xf5/0x160 [ 14.033834] ? __do_sys_finit_module+0xe9/0x110 [ 14.033835] __do_sys_finit_module+0xe9/0x110 [ 14.033838] do_syscall_64+0x33/0x80 [ 14.033839] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 14.033840] RIP: 0033:0x7feacf1bef59 [ 14.033841] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48 [ 14.033842] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 14.033843] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59 [ 14.033843] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015 [ 14.033844] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000 [ 14.033844] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000 [ 14.033845] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410 [ 14.033846] ---[ end trace 16aeaa08847a13da ]---
Probably the same issue as in https://bugzilla.kernel.org/show_bug.cgi?id=209123. What does it practically mean? Can/should it be silenced in this setup?
Jan
On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka jan.kiszka@web.de wrote:
Adding amdgpu display folks. -Daniel
[AMD Public Use]
I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
Alex
On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka jan.kiszka@web.de wrote:
It's harmless, but I'll send out a patch soon.
Alex
On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher alexdeucher@gmail.com wrote:
I believe the attached patch should fix this.
Alex
On Thu, Dec 3, 2020 at 10:31 PM Alex Deucher alexdeucher@gmail.com wrote:
fwiw rb: me
Probably want to add the right Fixes: line too. -Daniel
From: Ville Syrjälä ville.syrjala@linux.intel.com
Let's simplify life of driver by allowing them to leave encoder->possible_crtcs unset if they have no restrictions in crtc<->encoder linkage. We'll just populate possible_crtcs with the full crtc mask when registering the encoder so that userspace doesn't have to deal with drivers not populating this correctly.
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- We might not actually need/want this, but included it here for future reference if that assumption turns out to be wrong. --- drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++- include/drm/drm_encoder.h | 4 ++++ 2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 4c1b350ddb95..ce18c3dd0bde 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev) return crtc_mask; }
+/* + * Make life easy for drivers by allowing them to leave + * possible_crtcs unset if there are not crtc<->encoder + * restrictions. + */ +static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder) +{ + if (encoder->possible_crtcs == 0) + encoder->possible_crtcs = full_crtc_mask(encoder->dev); +} + static void validate_encoder_possible_crtcs(struct drm_encoder *encoder) { u32 crtc_mask = full_crtc_mask(encoder->dev); @@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev) + drm_for_each_encoder(encoder, dev) { fixup_encoder_possible_clones(encoder); + fixup_encoder_possible_crtcs(encoder); + }
drm_for_each_encoder(encoder, dev) { validate_encoder_possible_clones(encoder); diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index b236269f41ac..bd033c5618bf 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -142,6 +142,10 @@ struct drm_encoder { * the bits for all &drm_crtc objects this encoder can be connected to * before calling drm_dev_register(). * + * As an exception to the above rule if any crtc can be connected to + * the encoder the driver can leave @possible_crtcs set to 0. The core + * will automagically fix this up by setting the bit for every crtc. + * * You will get a WARN if you get this wrong in the driver. * * Note that since CRTC objects can't be hotplugged the assigned indices
On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
I think this one is most definitely needed. _Lots_ of drivers get this toally wrong and just leave the value blank. It's encoded as official fallback in most userspace compositors.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
On Tue, Feb 11, 2020 at 06:05:45PM +0100, Daniel Vetter wrote:
OK. It's been a while since I dug around so can't really remmber how this was being handled. I'll reorder before pushing.
On Tue, Feb 11, 2020 at 07:14:51PM +0200, Ville Syrjälä wrote:
Hm otoh having "works with all crtcs" as default is a bit dangerous, whereas the "cannot be cloned" default for possible_clones is perfectly safe.
So now I'm kinda not sure whether this is a bright idea, and we shouldn't just eat the cost of fixing up all the various WARNING backtraces your previous patch triggers. I've done a full review and the following look suspect:
- tegara/sor.c Strangely it's the only one, the other output drivers do seem to set the possible_crtcs mask to something useful.
Everything else seems to be fine. I'd say let's drop this patch, and I'm adding Thierry to shed some light on what's going on in tegra/sor.c. -Daniel
On Wed, Feb 12, 2020 at 10:07:55AM +0100, Daniel Vetter wrote:
Strike that, it sets it using tegra_output_find_possible_crtcs().
I think everything is good and we really don't need this patch here to fix up possible_crtcs. -Daniel
On Wed, Feb 12, 2020 at 10:08:49AM +0100, Daniel Vetter wrote:
Finally pushed the other patches from the series to drm-misc-next. Thanks for the reviews.
Should the new possible_{crtcs,clones} WARNs start to trigger for anyone despite our best efforts, please holler and I'll look into what needs fixing.
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Deucher, Alexander -
Inki Dae -
Jan Kiszka -
Ville Syrjala -
Ville Syrjälä