LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <sean@poorly.run>
Cc: dri-devel@lists.freedesktop.org,
	"Sean Paul" <seanpaul@chromium.org>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <maxime.ripard@bootlin.com>,
	"David Airlie" <airlied@linux.ie>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs
Date: Wed, 8 May 2019 18:31:24 +0200	[thread overview]
Message-ID: <20190508163124.GY17751@phenom.ffwll.local> (raw)
In-Reply-To: <20190508160920.144739-2-sean@poorly.run>

On Wed, May 08, 2019 at 12:09:06PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch adds atomic_enable and atomic_disable callbacks to the
> encoder helpers. This will allow encoders to make informed decisions in
> their start-up/shutdown based on the committed state.
> 
> Aside from the new hooks, this patch also introduces the new signature
> for .atomic_* functions going forward. Instead of passing object state
> (well, encoders don't have atomic state, but let's ignore that), we pass
> the entire atomic state so the driver can inspect more than what's
> happening locally.
> 
> This is particularly important for the upcoming self refresh helpers.
> 
> Changes in v3:
> - Added patch to the set
> Changes in v4:
> - Move atomic_disable above prepare (Daniel)
> - Add breadcrumb to .enable() docbook (Daniel)

Why no r-b: me or did you not apply all my suggestions? Too lazy to read
it all again :-)
-Daniel

> 
> Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-2-sean@poorly.run
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  8 +++-
>  include/drm/drm_modeset_helper_vtables.h | 48 ++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 553415fe8ede..ccf01831f265 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -999,7 +999,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  
>  		/* Right function depends upon target state. */
>  		if (funcs) {
> -			if (new_conn_state->crtc && funcs->prepare)
> +			if (funcs->atomic_disable)
> +				funcs->atomic_disable(encoder, old_state);
> +			else if (new_conn_state->crtc && funcs->prepare)
>  				funcs->prepare(encoder);
>  			else if (funcs->disable)
>  				funcs->disable(encoder);
> @@ -1309,7 +1311,9 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  		drm_bridge_pre_enable(encoder->bridge);
>  
>  		if (funcs) {
> -			if (funcs->enable)
> +			if (funcs->atomic_enable)
> +				funcs->atomic_enable(encoder, old_state);
> +			else if (funcs->enable)
>  				funcs->enable(encoder);
>  			else if (funcs->commit)
>  				funcs->commit(encoder);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 8f3602811eb5..aa509c107083 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -675,6 +675,51 @@ struct drm_encoder_helper_funcs {
>  	enum drm_connector_status (*detect)(struct drm_encoder *encoder,
>  					    struct drm_connector *connector);
>  
> +	/**
> +	 * @atomic_disable:
> +	 *
> +	 * This callback should be used to disable the encoder. With the atomic
> +	 * drivers it is called before this encoder's CRTC has been shut off
> +	 * using their own &drm_crtc_helper_funcs.atomic_disable hook. If that
> +	 * sequence is too simple drivers can just add their own driver private
> +	 * encoder hooks and call them from CRTC's callback by looping over all
> +	 * encoders connected to it using for_each_encoder_on_crtc().
> +	 *
> +	 * This callback is a variant of @disable that provides the atomic state
> +	 * to the driver. It takes priority over @disable during atomic commits.
> +	 *
> +	 * This hook is used only by atomic helpers. Atomic drivers don't need
> +	 * to implement it if there's no need to disable anything at the encoder
> +	 * level. To ensure that runtime PM handling (using either DPMS or the
> +	 * new "ACTIVE" property) works @atomic_disable must be the inverse of
> +	 * @atomic_enable.
> +	 */
> +	void (*atomic_disable)(struct drm_encoder *encoder,
> +			       struct drm_atomic_state *state);
> +
> +	/**
> +	 * @atomic_enable:
> +	 *
> +	 * This callback should be used to enable the encoder. It is called
> +	 * after this encoder's CRTC has been enabled using their own
> +	 * &drm_crtc_helper_funcs.atomic_enable hook. If that sequence is
> +	 * too simple drivers can just add their own driver private encoder
> +	 * hooks and call them from CRTC's callback by looping over all encoders
> +	 * connected to it using for_each_encoder_on_crtc().
> +	 *
> +	 * This callback is a variant of @enable that provides the atomic state
> +	 * to the driver. It is called in place of @enable during atomic
> +	 * commits.
> +	 *
> +	 * This hook is used only by atomic helpers, for symmetry with @disable.
> +	 * Atomic drivers don't need to implement it if there's no need to
> +	 * enable anything at the encoder level. To ensure that runtime PM
> +	 * handling (using either DPMS or the new "ACTIVE" property) works
> +	 * @enable must be the inverse of @disable for atomic drivers.
> +	 */
> +	void (*atomic_enable)(struct drm_encoder *encoder,
> +			      struct drm_atomic_state *state);
> +
>  	/**
>  	 * @disable:
>  	 *
> @@ -691,6 +736,9 @@ struct drm_encoder_helper_funcs {
>  	 * handling (using either DPMS or the new "ACTIVE" property) works
>  	 * @disable must be the inverse of @enable for atomic drivers.
>  	 *
> +	 * For atomic drivers also consider @atomic_disable and save yourself
> +	 * from having to read the NOTE below!
> +	 *
>  	 * NOTE:
>  	 *
>  	 * With legacy CRTC helpers there's a big semantic difference between
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

  reply	other threads:[~2019-05-08 16:31 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190508160920.144739-1-sean@poorly.run>
2019-05-08 16:09 ` Sean Paul
2019-05-08 16:31   ` Daniel Vetter [this message]
2019-05-08 18:18     ` Sean Paul
2019-05-08 16:09 ` [PATCH v4 02/11] drm: Add drm_atomic_get_(old|new-_connector_for_encoder() helpers Sean Paul
2019-05-08 16:33   ` Daniel Vetter
2019-05-12 16:48     ` Laurent Pinchart
2019-05-08 16:09 ` [PATCH v4 03/11] drm: Add atomic variants for bridge enable/disable Sean Paul
2019-05-21  8:58   ` Andrzej Hajda
2019-05-21 14:31     ` Sean Paul
2019-05-08 16:09 ` [PATCH v4 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul
2019-05-08 16:09 ` [PATCH v4 05/11] drm: Add helpers to kick off self refresh mode in drivers Sean Paul
2019-05-08 16:35   ` Daniel Vetter
2019-05-08 16:09 ` [PATCH v4 06/11] drm/rockchip: Use dirtyfb helper Sean Paul
2019-05-08 16:09 ` [PATCH v4 07/11] drm/rockchip: Check for fast link training before enabling psr Sean Paul
2019-05-08 16:09 ` [PATCH v4 08/11] drm/rockchip: Use the helpers for PSR Sean Paul
2019-05-08 16:09 ` [PATCH v4 09/11] drm/rockchip: Use vop_win in vop_win_disable instead of vop_win_data Sean Paul
2019-05-08 16:09 ` [PATCH v4 10/11] drm/rockchip: Don't fully disable vop on self refresh Sean Paul
2019-05-08 16:09 ` [PATCH v4 11/11] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190508163124.GY17751@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=maxime.ripard@bootlin.com \
    --cc=sean@poorly.run \
    --cc=seanpaul@chromium.org \
    --cc=ville.syrjala@linux.intel.com \
    --subject='Re: [PATCH v4 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).