Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 12 Aug 2020 12:26:44 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Algea Cao <algea.cao@rock-chips.com>
Cc:     a.hajda@samsung.com, kuankuan.y@gmail.com, hjc@rock-chips.com,
        tzimmermann@suse.de, dri-devel@lists.freedesktop.org,
        sam@ravnborg.org, airlied@linux.ie, heiko@sntech.de,
        jernej.skrabec@siol.net, laurent.pinchart+renesas@ideasonboard.com,
        jonas@kwiboo.se, mripard@kernel.org, darekm@google.com,
        linux-rockchip@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, cychiang@chromium.org,
        linux-kernel@vger.kernel.org, narmstrong@baylibre.com,
        jbrunet@baylibre.com, maarten.lankhorst@linux.intel.com,
        daniel@ffwll.ch
Subject: Re: [PATCH 1/6] drm: Add connector atomic_begin/atomic_flush
Message-ID: <20200812092644.GD6057@pendragon.ideasonboard.com>
References: <20200812083120.743-1-algea.cao@rock-chips.com>
 <20200812083407.856-1-algea.cao@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200812083407.856-1-algea.cao@rock-chips.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Algea,

Thank you for the patch.

On Wed, Aug 12, 2020 at 04:34:07PM +0800, Algea Cao wrote:
> In some situations, connector should get some work done
> when plane is updating. Such as when change output color
> format, hdmi should send AVMUTE to make screen black before
> crtc updating color format, or screen may flash. After color
> updating, hdmi should clear AVMUTE bring screen back to normal.
> 
> The process is as follows:
> AVMUTE -> Update CRTC -> Update HDMI -> Clear AVMUTE
> 
> So we introduce connector atomic_begin/atomic_flush.

Implementing this through .atomic_begin() and .atomic_flush() seems like
a pretty big hack to me. Furthermore, I think this should be implemented
as bridge operations, not at the connector level, as the HDMI encoder
may not be the component that implements the drm_connector.

> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> 
> ---
> 
>  drivers/gpu/drm/drm_atomic_helper.c      | 46 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 19 ++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f68c69a45752..f4abd700d2c4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2471,6 +2471,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  				     struct drm_atomic_state *old_state,
>  				     uint32_t flags)
>  {
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_connector_state, *new_connector_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_plane *plane;
> @@ -2479,6 +2481,28 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  	bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
>  	bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
>  
> +	for_each_oldnew_connector_in_state(old_state, connector,
> +					   old_connector_state,
> +					   new_connector_state, i) {
> +		const struct drm_connector_helper_funcs *funcs;
> +
> +		if (!connector->state->crtc)
> +			continue;
> +
> +		if (!connector->state->crtc->state->active)
> +			continue;
> +
> +		funcs = connector->helper_private;
> +
> +		if (!funcs || !funcs->atomic_begin)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("flush beginning [CONNECTOR:%d:%s]\n",
> +				 connector->base.id, connector->name);
> +
> +		funcs->atomic_begin(connector, old_connector_state);
> +	}
> +
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
>  
> @@ -2550,6 +2574,28 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  
>  		funcs->atomic_flush(crtc, old_crtc_state);
>  	}
> +
> +	for_each_oldnew_connector_in_state(old_state, connector,
> +					   old_connector_state,
> +					   new_connector_state, i) {
> +		const struct drm_connector_helper_funcs *funcs;
> +
> +		if (!connector->state->crtc)
> +			continue;
> +
> +		if (!connector->state->crtc->state->active)
> +			continue;
> +
> +		funcs = connector->helper_private;
> +
> +		if (!funcs || !funcs->atomic_flush)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("flushing [CONNECTOR:%d:%s]\n",
> +				 connector->base.id, connector->name);
> +
> +		funcs->atomic_flush(connector, old_connector_state);
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
>  
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 421a30f08463..10f3f2e2fe28 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1075,6 +1075,25 @@ struct drm_connector_helper_funcs {
>  	void (*atomic_commit)(struct drm_connector *connector,
>  			      struct drm_connector_state *state);
>  
> +	/**
> +	 * @atomic_begin:
> +	 *
> +	 * flush atomic update
> +	 *
> +	 * This callback is used by the atomic modeset helpers but it is optional.
> +	 */
> +	void (*atomic_begin)(struct drm_connector *connector,
> +			     struct drm_connector_state *state);
> +
> +	/**
> +	 * @atomic_begin:
> +	 *
> +	 * begin atomic update
> +	 *
> +	 * This callback is used by the atomic modeset helpers but it is optional.
> +	 */
> +	void (*atomic_flush)(struct drm_connector *connector,
> +			     struct drm_connector_state *state);
>  	/**
>  	 * @prepare_writeback_job:
>  	 *

-- 
Regards,

Laurent Pinchart