[PATCH v3] drm/atomic-helper: Add atomic_mode_set helper callback

[PATCH v3] drm/atomic-helper: Add atomic_mode_set helper callback

[Philipp Zabel]

Some encoders need more information from crtc and connector state or
connector display info than just the mode during mode setting. This
patch adds an atomic encoder mode setting variant that passes the crtc
state (which contains the modes) and the connector state.

atomic_enable/disable variants that additionally pass crtc and connector
state don't seem to be necessary for any current driver. mode_fixup
already has an atomic equivalent in atomic_check.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - Removed atomic_enable and atomic_disable hooks again, they are not useful
   to anyone for now
---
 drivers/gpu/drm/drm_atomic_helper.c      |  6 +++++-
 include/drm/drm_modeset_helper_vtables.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index de7fddc..a3c033f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -880,8 +880,12 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call mode_set hooks twice.
 		 */
-		if (funcs && funcs->mode_set)
+		if (funcs && funcs->atomic_mode_set) {
+			funcs->atomic_mode_set(encoder, new_crtc_state,
+					       connector->state);
+		} else if (funcs && funcs->mode_set) {
 			funcs->mode_set(encoder, mode, adjusted_mode);
+		}
 
 		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
 	}
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index b55f218..686feec 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -523,12 +523,41 @@ struct drm_encoder_helper_funcs {
 	 *
 	 * This callback is used both by the legacy CRTC helpers and the atomic
 	 * modeset helpers. It is optional in the atomic helpers.
+	 *
+	 * NOTE:
+	 *
+	 * If the driver uses the atomic modeset helpers and needs to inspect
+	 * the connector state or connector display info during mode setting,
+	 * @atomic_mode_set can be used instead.
 	 */
 	void (*mode_set)(struct drm_encoder *encoder,
 			 struct drm_display_mode *mode,
 			 struct drm_display_mode *adjusted_mode);
 
 	/**
+	 * @atomic_mode_set:
+	 *
+	 * This callback is used to update the display mode of an encoder.
+	 *
+	 * Note that the display pipe is completely off when this function is
+	 * called. Drivers which need hardware to be running before they program
+	 * the new display mode (because they implement runtime PM) should not
+	 * use this hook, because the helper library calls it only once and not
+	 * every time the display pipeline is suspended using either DPMS or the
+	 * new "ACTIVE" property. Such drivers should instead move all their
+	 * encoder setup into the ->enable() callback.
+	 *
+	 * This callback is used by the atomic modeset helpers in place of the
+	 * @mode_set callback, if set by the driver. It is optional and should
+	 * be used instead of @mode_set if the driver needs to inspect the
+	 * connector state or display info, since there is no direct way to
+	 * go from the encoder to the current connector.
+	 */
+	void (*atomic_mode_set)(struct drm_encoder *encoder,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *conn_state);
+
+	/**
 	 * @get_crtc:
 	 *
 	 * This callback is used by the legacy CRTC helpers to work around
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/atomic-helper: Add atomic_mode_set helper callback

[Daniel Vetter]

On Fri, Jul 29, 2016 at 03:20:04PM +0200, Philipp Zabel wrote:
> Some encoders need more information from crtc and connector state or
> connector display info than just the mode during mode setting. This
> patch adds an atomic encoder mode setting variant that passes the crtc
> state (which contains the modes) and the connector state.
> 
> atomic_enable/disable variants that additionally pass crtc and connector
> state don't seem to be necessary for any current driver. mode_fixup
> already has an atomic equivalent in atomic_check.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

lgtm. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Also it probably makes sense to pull this in through your driver tree, so
Ack for merging through that. But please send out a pull request for
drm-next latest by -rc2 to avoid conflicts. There's other people who might
need these atomic_ versions of the hooks (I'm chatting with Ben Skeggs
about some similar problems he has in the nouveau conversion).

Thanks, Daniel

> ---
> Changes since v2:
>  - Removed atomic_enable and atomic_disable hooks again, they are not useful
>    to anyone for now
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++++-
>  include/drm/drm_modeset_helper_vtables.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index de7fddc..a3c033f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -880,8 +880,12 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		 * Each encoder has at most one connector (since we always steal
>  		 * it away), so we won't call mode_set hooks twice.
>  		 */
> -		if (funcs && funcs->mode_set)
> +		if (funcs && funcs->atomic_mode_set) {
> +			funcs->atomic_mode_set(encoder, new_crtc_state,
> +					       connector->state);
> +		} else if (funcs && funcs->mode_set) {
>  			funcs->mode_set(encoder, mode, adjusted_mode);
> +		}
>  
>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>  	}
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index b55f218..686feec 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -523,12 +523,41 @@ struct drm_encoder_helper_funcs {
>  	 *
>  	 * This callback is used both by the legacy CRTC helpers and the atomic
>  	 * modeset helpers. It is optional in the atomic helpers.
> +	 *
> +	 * NOTE:
> +	 *
> +	 * If the driver uses the atomic modeset helpers and needs to inspect
> +	 * the connector state or connector display info during mode setting,
> +	 * @atomic_mode_set can be used instead.
>  	 */
>  	void (*mode_set)(struct drm_encoder *encoder,
>  			 struct drm_display_mode *mode,
>  			 struct drm_display_mode *adjusted_mode);
>  
>  	/**
> +	 * @atomic_mode_set:
> +	 *
> +	 * This callback is used to update the display mode of an encoder.
> +	 *
> +	 * Note that the display pipe is completely off when this function is
> +	 * called. Drivers which need hardware to be running before they program
> +	 * the new display mode (because they implement runtime PM) should not
> +	 * use this hook, because the helper library calls it only once and not
> +	 * every time the display pipeline is suspended using either DPMS or the
> +	 * new "ACTIVE" property. Such drivers should instead move all their
> +	 * encoder setup into the ->enable() callback.
> +	 *
> +	 * This callback is used by the atomic modeset helpers in place of the
> +	 * @mode_set callback, if set by the driver. It is optional and should
> +	 * be used instead of @mode_set if the driver needs to inspect the
> +	 * connector state or display info, since there is no direct way to
> +	 * go from the encoder to the current connector.
> +	 */
> +	void (*atomic_mode_set)(struct drm_encoder *encoder,
> +				struct drm_crtc_state *crtc_state,
> +				struct drm_connector_state *conn_state);
> +
> +	/**
>  	 * @get_crtc:
>  	 *
>  	 * This callback is used by the legacy CRTC helpers to work around
> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel