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

[Daniel Vetter <daniel@ffwll.ch>]

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