On Thu, Jun 02, 2016 at 07:28:30PM +0200, Philipp Zabel wrote:
Hi Daniel,
Am Donnerstag, den 02.06.2016, 18:21 +0200 schrieb Daniel Vetter: [...]
Only the reference count of connectors that weren't previously bound to an encoder should be incremented after a call to drm_crtc_helper_set_config. And only the reference count of connectors that were previously bound to an encoder and are unbound afterwards should ever be decremented. The reference counts of the temporary copies in the save_connectors should not be touched at all.
This patch fixes the above error by only incrementing the reference count of those connectors in the set that are initially not bound to any encoder, and also by restoring the reference count of only those connectors in the set in the failure case.
Fixes: 0955c1250e96 ("drm/crtc: take references to connectors used in a modeset. (v2)") Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
I think this is worse. We can't save/restore the connector when there's a kref inside of it. But there's really only 2 things we need to save restore:
- connector->encoder pointer, for each connector.
- encoder->crtc pointer, for each encoder.
For a proper fix I think we need to rectify that first, then apply your bugfix on top. The refcount logic itself seems sound, but I've screwed that up way too often already.
Ok. Do we have to do it first, though? If we first get rid of the drm_connector_unreference(&save_connectors[count++]); part first, we can don't have to fix it up when the connector array goes away.
It's just a bugfix for the failure case. Doing a memcpy over a live kref is definitely not something we should ever do, and will break the refcounting. We want both patches in 4.7.
Also I'm somewhat confused about how you manage to blow things up. The refcount of each connector we're seeing should be 1 already: Once from the connector list, and a 2nd one from the lookup in the setcrtc ioctl code.
All I saw is that inside drm_crtc_helper_set_config all connectors in the set have their refcount incremented and then *all* connectors have their refcount decremented. Isn't the lookup refcount increment only done for connectors in the set? I see it look up connector 36, then reference it once more as part of the set. At the end it tries to unreference connector 34 from save_connectors, followed by the error:
[drm:drm_ioctl] pid=205, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETCRTC [drm:drm_mode_setcrtc] [CRTC:24:crtc-0] [drm:drm_mode_setcrtc] [CONNECTOR:36:LVDS-1] [drm:drm_crtc_helper_set_config] [drm:drm_crtc_helper_set_config] [CRTC:24:c 13.982686] [drm:drm_mode_debug_printmodeline] Modeline 0:"" 0 0 0 0 0 0 0 0 0 0 0x0 0x0 [drm:drm_mode_debug_printmodeline] Modeline 41:"1280x800" 60 71100 1280 1281 1439 1440 800 801 822 823 0x40 0x0 [drm:drm_mode_object_reference] OBJ ID: 36 (2) [drm:drm_crtc_helper_set_config] encoder changed, full mode switch [drm:drm_crtc_helper_set_config] crtc changed, full mode switch [drm:drm_crtc_helper_set_config] [CONNECTOR:36:LVDS-1] to [CRtc-0] [drm:drm_crtc_helper_set_mode] [ENCODER:35:LVDS-35] set [MODE:41:1280x800] [drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 1440, vtotal 823, vdisplay 800 [drm:drm_calc_timestamping_constants] crtc 24: clock 71100 kHz framedur 16668354 linedur 20253 [drm:drm_crtc_helper_set_config] Setting connector DPMS state to on [drm:drm_crtc_helper_set_config] [CONNECTOR:36:LVDS-1] set DPMS on [drm:drm_mode_object_unreference] OBJ ID: 34 (1)
The additional get/put I'm talking off happens in drm_mode_setcrtc (the get is hidden in drm_connector_lookup). Might be interesting to trace the refcount for each connector after each get and before each put.
Which means when we drop that 1 reference in the saved connector (which is entirely bogus code, I agree) it should only drop to 1, not 0. And youre cleanup code should not be called.
The other thing is that radeon/nouveau also uses this code, and no one complained yet. Together I think that's some good evidence that suggests there's also something strange going on in imx itself.
That's what I wonder, too. Right now I can't see it though.
Yep, something funny is definitely going on here. -Daniel