Re: [PATCH v1 3/7] drm/bridge: Add drm_bridge_new_crtc_state() helper
From: Maxime Ripard <hidden>
Date: 2021-07-22 07:32:38
Also in:
dri-devel, linux-arm-kernel
Hi, On Thu, Jul 22, 2021 at 08:22:42AM +0200, Sam Ravnborg wrote:
quoted hunk ↗ jump to hunk
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 <redacted> 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 <redacted> Cc: Neil Armstrong <redacted> Cc: Robert Foss <redacted> Cc: Daniel Vetter <redacted> --- 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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek