[PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property
Igor Paunovic
royalnet026 at gmail.com
Wed Sep 9 12:40:35 PDT 2026
The bot is right, and it is worse than it reported. I went and read the
code rather than taking either its word or my own.
drm_bridge_connector_init() has two paths. With an HDMI bridge in the
chain it calls drmm_connector_hdmi_init() (drm_bridge_connector.c:1016);
otherwise drmm_connector_init() (:1027). And drmm_connector_hdmi_init()
already contains, at drm_connector.c:621-631, the exact block this patch
adds - same shape, same comment, because that is where I took it from -
and then calls drm_connector_attach_max_bpc_property() itself at :633.
So on a chain with an HDMI bridge my patch does two wrong things, not
one:
- it overwrites a connector->state that has just been allocated, which
is the leak the bot found;
- it calls drm_connector_attach_max_bpc_property() a second time. That
one is not a leak: the helper reuses connector->max_bpc_property when
it already exists, so my 6..10 range is silently discarded and the
8..max_bpc property created by the HDMI path stays. It does re-attach
the same property to the object and overwrite state->max_bpc.
On the boards I care about there is no HDMI bridge behind dw-dp, so
neither fires today - but "does not fire on my board" is not a reason to
leave it.
For v3 both blocks get guarded on what is already there:
if (!connector->state) {
... create the state ...
}
if (!connector->max_bpc_property) {
ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
...
}
That leaves a question I would rather ask than paper over. The state
creation exists only in drmm_connector_hdmi_init(), while
drm_bridge_connector_init() leaves the plain path without a state - so
any bridge driver that wants a connector property at bind time has to
copy that block, as I did. Would it be better to move it into
drm_bridge_connector_init() so both paths get a state, and have drivers
just attach their properties? I am happy to write that instead, but it
touches shared code and I would rather be told than guess.
On the pre-existing [High]: the bot is right that the error path I add
inherits it. dw_dp_bind() takes devm resources on the component device,
including a non-shared devm_request_threaded_irq(), so a failed bind
leaves them held and the retry gets -EBUSY. My dw_dp_unbind() call does
not release devres. With the guards above the new path becomes hard to
reach, but it does not stop being wrong, and I did not introduce it.
I can send a separate patch moving those to the master device if the
maintainers want it - it is a different change from this one.
Igor
More information about the Linux-rockchip
mailing list