Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/3] drm/bridge: Allow DSI encoders to decide when to
 call bridge funcs.
To:     Eric Anholt <eric@anholt.net>, dri-devel@lists.freedesktop.org,
        Archit Taneja <architt@codeaurora.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc:     linux-kernel@vger.kernel.org
From:   Andrzej Hajda <a.hajda@samsung.com>
Date:   Thu, 7 Jun 2018 11:37:16 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
        Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <20180606190431.1833-2-eric@anholt.net>
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Message-Id: <20180607093718eucas1p2b4661411d63639afab493e6098dc8f26~11y1kzGTK3005330053eucas1p2O@eucas1p2.samsung.com>
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20180606190431.1833-1-eric@anholt.net>
        <CGME20180606190437epcas1p3c33d51471a16b8a047c175a0cd3f8869@epcas1p3.samsung.com>
        <20180606190431.1833-2-eric@anholt.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 06.06.2018 21:04, Eric Anholt wrote:
> For DSI, we want to pre_enable once the DSI link is up but before
> video packets are being sent, and similarly we want to be able to
> post_disable while the link is still up but after video has stopped.
>
> Given that with drm_panels we've had issues with drivers missing calls
> to one of the hooks, include some checks in the atomic helpers that
> the driver got it right.

There is no explicit description of disable_midlayer_calls.

Anyway I have few general comments I put them here to simplify discussion:
1. disable_midlayer_calls is used only by drm core on only 1st bridge in
the chain, in such case the flag should be placed rather in drm_encoder.
On the other hand to support this case it would be enough to use private
drm_bridge pointer instead of drm_encoder->bridge. I guess it should be
enough to solve your case.
2. Similar issue can appear not only between encoder->bridge pair, but
also in bridge->bridge pair (for longer display chains), it would be
good to cover such case, so maybe instead of disable_midlayer_calls
there should be flag disable_recursive_calls/disable_chain_calls in
drm_bridge, but again it can be solved also by not using
drm_bridge->next member.
3. There are 4 bools to control state machine of the bridge (it makes 16
possibilities, most of them is invalid), but it has only 3 states: off,
prepared, enabled. I think it would be clearer if we define
enum with these three states and use it.
4. WARN in atomics checks only 1st bridge in the chain, in most cases it
is enough but in case of longer chains it will not check next
bridges/panels - this is definitely only partial solution, maybe
skipping it wouldn't be bad idea, but it is just my suggestion.

Regards
Andrzej

>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_bridge.c        | 12 +++++++++++
>  include/drm/drm_bridge.h            | 21 +++++++++++++++++++
>  3 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 130da5195f3b..2950ddcc9013 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -950,7 +950,13 @@ disable_outputs(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 disable hooks twice.
>  		 */
> -		drm_bridge_disable(encoder->bridge);
> +		if (encoder->bridge) {
> +			encoder->bridge->post_disable_called = false;
> +			encoder->bridge->disable_called = false;
> +
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_disable(encoder->bridge);
> +		}
>  
>  		/* Right function depends upon target state. */
>  		if (funcs) {
> @@ -962,7 +968,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>  		}
>  
> -		drm_bridge_post_disable(encoder->bridge);
> +		if (encoder->bridge) {
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_post_disable(encoder->bridge);
> +
> +			WARN_ON(!encoder->bridge->post_disable_called);
> +			WARN_ON(!encoder->bridge->disable_called);
> +		}
>  	}
>  
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1240,7 +1252,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  		 * Each encoder has at most one connector (since we always steal
>  		 * it away), so we won't call enable hooks twice.
>  		 */
> -		drm_bridge_pre_enable(encoder->bridge);
> +		if (encoder->bridge) {
> +			encoder->bridge->pre_enable_called = false;
> +			encoder->bridge->enable_called = false;
> +
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_pre_enable(encoder->bridge);
> +		}
>  
>  		if (funcs) {
>  			if (funcs->enable)
> @@ -1249,7 +1267,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  				funcs->commit(encoder);
>  		}
>  
> -		drm_bridge_enable(encoder->bridge);
> +		if (encoder->bridge) {
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_enable(encoder->bridge);
> +
> +			WARN_ON(!encoder->bridge->pre_enable_called);
> +			WARN_ON(!encoder->bridge->enable_called);
> +		}
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..847c4209da60 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -153,6 +153,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	bridge->disable_midlayer_calls = false;
> +
>  	bridge->dev = NULL;
>  }
>  
> @@ -248,6 +250,8 @@ void drm_bridge_disable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	bridge->disable_called = true;
> +
>  	drm_bridge_disable(bridge->next);
>  
>  	if (bridge->funcs->disable)
> @@ -270,6 +274,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	WARN_ON(!bridge->disable_called);
> +	bridge->post_disable_called = true;
> +
>  	if (bridge->funcs->post_disable)
>  		bridge->funcs->post_disable(bridge);
>  
> @@ -319,6 +326,8 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	bridge->pre_enable_called = true;
> +
>  	drm_bridge_pre_enable(bridge->next);
>  
>  	if (bridge->funcs->pre_enable)
> @@ -341,6 +350,9 @@ void drm_bridge_enable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	WARN_ON(!bridge->pre_enable_called);
> +	bridge->enable_called = true;
> +
>  	if (bridge->funcs->enable)
>  		bridge->funcs->enable(bridge);
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bd850747ce54..ba0a227c96b7 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -294,6 +294,27 @@ struct drm_bridge {
>  	const struct drm_bridge_funcs *funcs;
>  	/** @driver_private: pointer to the bridge driver's internal context */
>  	void *driver_private;
> +
> +	/**
> +	 * @disable_midlayer_calls:
> +	 *
> +	 * disables the calls from the DRM atomic helpers into the
> +	 * bridge, leaving them to be performed by the driver.  This
> +	 * may be useful for encoders such as DSI, where the
> +	 * pre_enable/post_disable hooks want to be called while the
> +	 * bus is still enabled but before/after video packets are
> +	 * being sent.
> +	 */
> +	bool disable_midlayer_calls : 1;
> +
> +	/* private: flags for atomic helpers to check that the driver
> +	 * called the bridge functions properly if
> +	 * @disable_midlayer_calls was set.
> +	 */
> +	bool pre_enable_called : 1;
> +	bool enable_called : 1;
> +	bool disable_called : 1;
> +	bool post_disable_called : 1;
>  };
>  
>  void drm_bridge_add(struct drm_bridge *bridge);