Hi,
On Thu, Jul 22, 2021 at 08:22:42AM +0200, Sam Ravnborg wrote:
drm_bridge_new_crtc_state() will be used by bridge drivers to provide easy access to the mode from the drm_bridge_funcs operations.
The helper will be useful in the atomic operations of struct drm_bridge_funcs.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 34 ++++++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 3 +++ 2 files changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a8bbb021684b..93d513078e9a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1133,6 +1133,40 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
+/**
- drm_bridge_new_crtc_state - get crtc_state for the bridge
- @bridge: bridge object
- @old_bridge_state: state of the bridge
- This function returns the &struct drm_crtc_state for the given bridge/state,
- or NULL if no crtc_state could be looked up. In case no crtc_state then this is
- logged using WARN as the crtc_state is always expected to be present.
- This function is often used in the &struct drm_bridge_funcs operations.
- */
+const struct drm_crtc_state * +drm_bridge_new_crtc_state(struct drm_bridge *bridge,
Shouldn't we call this drm_atomic_bridge_get_new_crtc_state for consistency?
struct drm_bridge_state *old_bridge_state)
It seems odd to me to name it old_bridge_state, I guess it would make more sense to pass in the new bridge state?
+{
- struct drm_atomic_state *state = old_bridge_state->base.state;
- const struct drm_connector_state *conn_state;
- const struct drm_crtc_state *crtc_state;
- struct drm_connector *connector;
- connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
- if (WARN_ON(!connector))
return NULL;- conn_state = drm_atomic_get_new_connector_state(state, connector);
- if (WARN_ON(!conn_state || !conn_state->crtc))
return NULL;- crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
- if (WARN_ON(!crtc_state))
return NULL;- return crtc_state;
You don't even seem to use the bridge state itself, so maybe we just need to pass the drm_atomic_state? And thus we end up with something like drm_atomic_get_new_connector_for_encoder, so maybe we should just call it drm_atomic_get_new_crtc_for_bridge?
Also, can we end up with a commit that affects the bridge state but not the crtc state (like a connector property change)? In such a case drm_atomic_get_new_crtc_state would return NULL.
I'm not sure if it's a big deal or not, but we should make it clear in the documentation and remove the WARN_ON.
Maxime