[PATCH] drm/crtc: fix connector reference counting mismatch in drm_crtc_helper_set_config

[Philipp Zabel]

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 at 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.

> 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)

> 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.

regards
Philipp