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@xxxxxxxxxxxxxx>
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
