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:22:17 +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 2/6] drm: bridge: dw-hdmi: Implement connector
 atomic_begin/atomic_flush
Message-ID: <20200812092217.GC6057@pendragon.ideasonboard.com>
References: <20200812083120.743-1-algea.cao@rock-chips.com>
 <20200812083433.934-1-algea.cao@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200812083433.934-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:33PM +0800, Algea Cao wrote:
> Introduce dw_hdmi_connector_atomic_begin() and
> dw_hdmi_connector_atomic_flush() to implement connector
> atomic_begin/atomic_flush. When enc_out_bus_format or
> enc_in_bus_format changed, dw_hdmi_setup is called.
> 
> To avoid screen flash when updating bus format, it's need
> to send AVMUTE flag to make screen black, and clear flag
> after bus format updated.
> 
> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 65 +++++++++++++++++++++++
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |  4 ++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 6148a022569a..a1a81fc768c2 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -108,6 +108,8 @@ struct hdmi_vmode {
>  };
>  
>  struct hdmi_data_info {
> +	unsigned int prev_enc_in_bus_format;
> +	unsigned int prev_enc_out_bus_format;
>  	unsigned int enc_in_bus_format;
>  	unsigned int enc_out_bus_format;
>  	unsigned int enc_in_encoding;
> @@ -116,6 +118,7 @@ struct hdmi_data_info {
>  	unsigned int hdcp_enable;
>  	struct hdmi_vmode video_mode;
>  	bool rgb_limited_range;
> +	bool update;
>  };
>  
>  struct dw_hdmi_i2c {
> @@ -2401,6 +2404,60 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
>  
> +static void
> +dw_hdmi_connector_atomic_begin(struct drm_connector *connector,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> +					    connector);
> +	unsigned int enc_in_bus_fmt = hdmi->hdmi_data.enc_in_bus_format;
> +	unsigned int enc_out_bus_fmt = hdmi->hdmi_data.enc_out_bus_format;
> +	unsigned int prev_enc_in_bus_fmt =
> +		hdmi->hdmi_data.prev_enc_in_bus_format;
> +	unsigned int prev_enc_out_bus_fmt =
> +		hdmi->hdmi_data.prev_enc_out_bus_format;
> +
> +	if (!conn_state->crtc)
> +		return;
> +
> +	if (!hdmi->hdmi_data.video_mode.mpixelclock)
> +		return;
> +
> +	if (enc_in_bus_fmt != prev_enc_in_bus_fmt ||
> +	    enc_out_bus_fmt != prev_enc_out_bus_fmt) {
> +		hdmi->hdmi_data.update = true;
> +		hdmi_writeb(hdmi, HDMI_FC_GCP_SET_AVMUTE, HDMI_FC_GCP);
> +		/* Add delay to make av mute work on sink*/
> +		msleep(50);
> +	} else {
> +		hdmi->hdmi_data.update = false;
> +	}
> +}
> +
> +static void
> +dw_hdmi_connector_atomic_flush(struct drm_connector *connector,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> +					     connector);
> +
> +	if (!conn_state->crtc)
> +		return;
> +
> +	DRM_DEBUG("%s\n", __func__);
> +
> +	if (hdmi->hdmi_data.update) {
> +		dw_hdmi_setup(hdmi, hdmi->curr_conn, &hdmi->previous_mode);
> +		/*
> +		 * Before clear AVMUTE, delay is needed to
> +		 * prevent display flash.
> +		 */
> +		msleep(50);
> +		hdmi_writeb(hdmi, HDMI_FC_GCP_CLEAR_AVMUTE, HDMI_FC_GCP);
> +		hdmi->hdmi_data.update = false;
> +	}
> +}
> +
>  static bool hdr_metadata_equal(const struct drm_connector_state *old_state,
>  			       const struct drm_connector_state *new_state)
>  {
> @@ -2465,6 +2522,8 @@ static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
>  static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
>  	.get_modes = dw_hdmi_connector_get_modes,
>  	.atomic_check = dw_hdmi_connector_atomic_check,
> +	.atomic_begin = dw_hdmi_connector_atomic_begin,
> +	.atomic_flush = dw_hdmi_connector_atomic_flush,
>  };
>  
>  static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
> @@ -2778,6 +2837,12 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
>  {
>  	struct dw_hdmi *hdmi = bridge->driver_private;
>  
> +	hdmi->hdmi_data.prev_enc_out_bus_format =
> +			hdmi->hdmi_data.enc_out_bus_format;
> +
> +	hdmi->hdmi_data.prev_enc_in_bus_format =
> +			hdmi->hdmi_data.enc_in_bus_format;
> +
>  	hdmi->hdmi_data.enc_out_bus_format =
>  			bridge_state->output_bus_cfg.format;
>  

.atomic_check() isn't allowed to change the device state, neither the
hardware state, nor the software state stored in struct dw_hdmi. You
essentially need to treat the drm_bridge and dw_hdmi as const in the
.atomic_check() operation.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 1999db05bc3b..05182418efbb 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -842,6 +842,10 @@ enum {
>  	HDMI_FC_AVICONF3_QUANT_RANGE_LIMITED = 0x00,
>  	HDMI_FC_AVICONF3_QUANT_RANGE_FULL = 0x04,
>  
> +/* HDMI_FC_GCP */
> +	HDMI_FC_GCP_SET_AVMUTE = 0x2,
> +	HDMI_FC_GCP_CLEAR_AVMUTE = 0x1,
> +
>  /* FC_DBGFORCE field values */
>  	HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
>  	HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,

-- 
Regards,

Laurent Pinchart