| Message ID | 20250213-bridge-connector-v3-32-e71598f49c8f@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: Various quality of life improvements | expand |
On Thu, Feb 13, 2025 at 03:43:51PM +0100, Maxime Ripard wrote: > Other entities (drm_connector.crtc, drm_encoder.crtc, etc.) have > pointer to other currently bound entities. They are all considered > relevant only for non-atomic drivers, and generally perceived as > deprecated in favour of the equivalent pointers in the atomic states. I think there is a significant difference between mentioned fields and drm_bridge.encoder: the former fields are variable and can change. The latter one is static and set at the bridge attachment time. Nevertheless I think it is a good idea to deprecate it. > > It used to be useful because we didn't have alternatives, but now that > we do, let's make sure it's done for drm_bridge.encoder too. > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > include/drm/drm_bridge.h | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-)
On Thu, Feb 13, 2025 at 06:35:15PM +0200, Dmitry Baryshkov wrote: > On Thu, Feb 13, 2025 at 03:43:51PM +0100, Maxime Ripard wrote: > > Other entities (drm_connector.crtc, drm_encoder.crtc, etc.) have > > pointer to other currently bound entities. They are all considered > > relevant only for non-atomic drivers, and generally perceived as > > deprecated in favour of the equivalent pointers in the atomic states. > > I think there is a significant difference between mentioned fields and > drm_bridge.encoder: the former fields are variable and can change. The > latter one is static and set at the bridge attachment time. Nevertheless > I think it is a good idea to deprecate it. I'm sorry, it's not clear to me here what you want here either. Do you want me to change anything to that patch? Maxime
On Fri, Feb 14, 2025 at 02:07:18PM +0100, Maxime Ripard wrote: > On Thu, Feb 13, 2025 at 06:35:15PM +0200, Dmitry Baryshkov wrote: > > On Thu, Feb 13, 2025 at 03:43:51PM +0100, Maxime Ripard wrote: > > > Other entities (drm_connector.crtc, drm_encoder.crtc, etc.) have > > > pointer to other currently bound entities. They are all considered > > > relevant only for non-atomic drivers, and generally perceived as > > > deprecated in favour of the equivalent pointers in the atomic states. > > > > I think there is a significant difference between mentioned fields and > > drm_bridge.encoder: the former fields are variable and can change. The > > latter one is static and set at the bridge attachment time. Nevertheless > > I think it is a good idea to deprecate it. > > I'm sorry, it's not clear to me here what you want here either. Do you > want me to change anything to that patch? Well... I was thinking if we should expand the commit message. Most likely it's fine though. In the end, I've r-b'ed the patch.
On Fri, Feb 14, 2025 at 03:29:12PM +0200, Dmitry Baryshkov wrote: > On Fri, Feb 14, 2025 at 02:07:18PM +0100, Maxime Ripard wrote: > > On Thu, Feb 13, 2025 at 06:35:15PM +0200, Dmitry Baryshkov wrote: > > > On Thu, Feb 13, 2025 at 03:43:51PM +0100, Maxime Ripard wrote: > > > > Other entities (drm_connector.crtc, drm_encoder.crtc, etc.) have > > > > pointer to other currently bound entities. They are all considered > > > > relevant only for non-atomic drivers, and generally perceived as > > > > deprecated in favour of the equivalent pointers in the atomic states. > > > > > > I think there is a significant difference between mentioned fields and > > > drm_bridge.encoder: the former fields are variable and can change. The > > > latter one is static and set at the bridge attachment time. Nevertheless > > > I think it is a good idea to deprecate it. > > > > I'm sorry, it's not clear to me here what you want here either. Do you > > want me to change anything to that patch? > > Well... I was thinking if we should expand the commit message. Most > likely it's fine though. In the end, I've r-b'ed the patch. I think this one is wrong, since the bridge->encoder link is static. This is unlike the connector->encoder->crtc chain, which isn't, and where you really want to go through the atomic states or you get funny stuff. I don't think we have bridge chains with multiple connectors though, so this is fairly academic and so maybe still a good idea to make this all more flexible? Unless I've missed the memo and atomic bridges have flexible routing, and in that case yes this shouldn't be used. Mildly confused ... -Sima
On Mon, Feb 17, 2025 at 05:38:08PM +0100, Simona Vetter wrote: > On Fri, Feb 14, 2025 at 03:29:12PM +0200, Dmitry Baryshkov wrote: > > On Fri, Feb 14, 2025 at 02:07:18PM +0100, Maxime Ripard wrote: > > > On Thu, Feb 13, 2025 at 06:35:15PM +0200, Dmitry Baryshkov wrote: > > > > On Thu, Feb 13, 2025 at 03:43:51PM +0100, Maxime Ripard wrote: > > > > > Other entities (drm_connector.crtc, drm_encoder.crtc, etc.) have > > > > > pointer to other currently bound entities. They are all considered > > > > > relevant only for non-atomic drivers, and generally perceived as > > > > > deprecated in favour of the equivalent pointers in the atomic states. > > > > > > > > I think there is a significant difference between mentioned fields and > > > > drm_bridge.encoder: the former fields are variable and can change. The > > > > latter one is static and set at the bridge attachment time. Nevertheless > > > > I think it is a good idea to deprecate it. > > > > > > I'm sorry, it's not clear to me here what you want here either. Do you > > > want me to change anything to that patch? > > > > Well... I was thinking if we should expand the commit message. Most > > likely it's fine though. In the end, I've r-b'ed the patch. > > I think this one is wrong, since the bridge->encoder link is static. This > is unlike the connector->encoder->crtc chain, which isn't, and where you > really want to go through the atomic states or you get funny stuff. > > I don't think we have bridge chains with multiple connectors though, so > this is fairly academic and so maybe still a good idea to make this all > more flexible? Unless I've missed the memo and atomic bridges have > flexible routing, and in that case yes this shouldn't be used. No, bridges are still attached to a single encoder, however I see Maxime's idea in uniformity. It's easier to remember that all direct pointers are deprecated rather than remembering that bridge->encoder is static, but everything else is dynamic and should be accessed through the state. > > Mildly confused ... > -Sima > -- > Simona Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 4cc12b8bbdfe2b496546607d1ae0b66a903c8f89..a4dd9be13c5205512c2b3441a3d87d557f411bfb 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -822,12 +822,18 @@ enum drm_bridge_ops { struct drm_bridge { /** @base: inherit from &drm_private_object */ struct drm_private_obj base; /** @dev: DRM device this bridge belongs to */ struct drm_device *dev; - /** @encoder: encoder to which this bridge is connected */ + + /** @encoder: + * + * encoder to which this bridge is connected. Only meaningful + * for non-atomic drivers. Deprecated. + */ struct drm_encoder *encoder; + /** @chain_node: used to form a bridge chain */ struct list_head chain_node; /** @of_node: device node pointer to the bridge */ struct device_node *of_node; /** @list: to keep track of all added bridges */