diff mbox series

[v12,27/28] drm/rockchip: inno_hdmi: Switch to HDMI connector

Message ID 20240423-kms-hdmi-connector-state-v12-27-3338e4c0b189@kernel.org (mailing list archive)
State New
Headers show
Series drm/connector: Create HDMI Connector infrastructure | expand

Commit Message

Maxime Ripard April 23, 2024, 10:45 a.m. UTC 
The new HDMI connector infrastructure allows to remove some boilerplate,
especially to generate infoframes. Let's switch to it.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/rockchip/Kconfig     |   1 +
 drivers/gpu/drm/rockchip/inno_hdmi.c | 143 +++++++++++++----------------------
 2 files changed, 53 insertions(+), 91 deletions(-)

Comments

Andy Yan April 27, 2024, 10:12 a.m. UTC | #1 
Hi Maxime,

On 4/23/24 18:45, Maxime Ripard wrote:
> The new HDMI connector infrastructure allows to remove some boilerplate,
> especially to generate infoframes. Let's switch to it.
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/rockchip/Kconfig     |   1 +
>   drivers/gpu/drm/rockchip/inno_hdmi.c | 143 +++++++++++++----------------------
>   2 files changed, 53 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
> index 4c7072e6e34e..e2ec20c2e2eb 100644
> --- a/drivers/gpu/drm/rockchip/Kconfig
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -72,10 +72,11 @@ config ROCKCHIP_DW_MIPI_DSI
>   	  enable MIPI DSI on RK3288 or RK3399 based SoC, you should
>   	  select this option.
>   
>   config ROCKCHIP_INNO_HDMI
>   	bool "Rockchip specific extensions for Innosilicon HDMI"
> +	depends on 

Is this supposed to be  DRM_DISPLAY_HDMI_STATE_HELPER whith you introduced in PATCH 04/28?
I couldn't find any place where  DRM_HDMI_STATE_HELPER is defined.

>   	help
>   	  This selects support for Rockchip SoC specific extensions
>   	  for the Innosilicon HDMI driver. If you want to enable
>   	  HDMI on RK3036 based SoC, you should select this option.
>   
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 1d2261643743..3756259bfb10 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -65,13 +65,11 @@ struct inno_hdmi {
>   	const struct inno_hdmi_variant *variant;
>   };
>   
>   struct inno_hdmi_connector_state {
>   	struct drm_connector_state	base;
> -	unsigned int			enc_out_format;
>   	unsigned int			colorimetry;
> -	bool				rgb_limited_range;
>   };
>   
>   static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
>   {
>   	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> @@ -255,90 +253,53 @@ static void inno_hdmi_reset(struct inno_hdmi *hdmi)
>   	hdmi_modb(hdmi, HDMI_SYS_CTRL, msk, val);
>   
>   	inno_hdmi_standby(hdmi);
>   }
>   
> -static void inno_hdmi_disable_frame(struct inno_hdmi *hdmi,
> -				    enum hdmi_infoframe_type type)
> +static int inno_hdmi_disable_frame(struct drm_connector *connector,
> +				   enum hdmi_infoframe_type type)
>   {
> -	struct drm_connector *connector = &hdmi->connector;
> -
> -	if (type != HDMI_INFOFRAME_TYPE_AVI) {
> -		drm_err(connector->dev,
> -			"Unsupported infoframe type: %u\n", type);
> -		return;
> -	}
> -
> -	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
> -}
> -
> -static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi,
> -				  union hdmi_infoframe *frame, enum hdmi_infoframe_type type)
> -{
> -	struct drm_connector *connector = &hdmi->connector;
> -	u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
> -	ssize_t rc, i;
> +	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
>   
>   	if (type != HDMI_INFOFRAME_TYPE_AVI) {
>   		drm_err(connector->dev,
>   			"Unsupported infoframe type: %u\n", type);
>   		return 0;
>   	}
>   
> -	inno_hdmi_disable_frame(hdmi, type);
> +	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
>   
> -	rc = hdmi_infoframe_pack(frame, packed_frame,
> -				 sizeof(packed_frame));
> -	if (rc < 0)
> -		return rc;
> +	return 0;
> +}
>   
> -	for (i = 0; i < rc; i++)
> +static int inno_hdmi_upload_frame(struct drm_connector *connector,
> +				  enum hdmi_infoframe_type type,
> +				  const u8 *buffer, size_t len)
> +{
> +	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
> +	u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
> +	ssize_t i;
> +
> +	if (type != HDMI_INFOFRAME_TYPE_AVI) {
> +		drm_err(connector->dev,
> +			"Unsupported infoframe type: %u\n", type);
> +		return 0;
> +	}
> +
> +	inno_hdmi_disable_frame(connector, type);
> +
> +	for (i = 0; i < len; i++)
>   		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i,
>   			    packed_frame[i]);
>   
>   	return 0;
>   }
>   
> -static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> -				      struct drm_display_mode *mode)
> -{
> -	struct drm_connector *connector = &hdmi->connector;
> -	struct drm_connector_state *conn_state = connector->state;
> -	struct inno_hdmi_connector_state *inno_conn_state =
> -					to_inno_hdmi_conn_state(conn_state);
> -	union hdmi_infoframe frame;
> -	int rc;
> -
> -	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> -						      &hdmi->connector,
> -						      mode);
> -	if (rc) {
> -		inno_hdmi_disable_frame(hdmi, HDMI_INFOFRAME_TYPE_AVI);
> -		return rc;
> -	}
> -
> -	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444)
> -		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> -	else if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV422)
> -		frame.avi.colorspace = HDMI_COLORSPACE_YUV422;
> -	else
> -		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> -
> -	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
> -		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> -						   connector, mode,
> -						   inno_conn_state->rgb_limited_range ?
> -						   HDMI_QUANTIZATION_RANGE_LIMITED :
> -						   HDMI_QUANTIZATION_RANGE_FULL);
> -	} else {
> -		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> -		frame.avi.ycc_quantization_range =
> -			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> -	}
> -
> -	return inno_hdmi_upload_frame(hdmi, &frame, HDMI_INFOFRAME_TYPE_AVI);
> -}
> +static const struct drm_connector_hdmi_funcs inno_hdmi_hdmi_connector_funcs = {
> +	.clear_infoframe	= inno_hdmi_disable_frame,
> +	.write_infoframe	= inno_hdmi_upload_frame,
> +};
>   
>   static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>   {
>   	struct drm_connector *connector = &hdmi->connector;
>   	struct drm_connector_state *conn_state = connector->state;
> @@ -359,12 +320,12 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>   	value = v_VIDEO_INPUT_BITS(VIDEO_INPUT_8BITS) |
>   		v_VIDEO_OUTPUT_COLOR(0) |
>   		v_VIDEO_INPUT_CSP(0);
>   	hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL2, value);
>   
> -	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
> -		if (inno_conn_state->rgb_limited_range) {
> +	if (conn_state->hdmi.output_format == HDMI_COLORSPACE_RGB) {
> +		if (conn_state->hdmi.is_limited_range) {
>   			csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>   			auto_csc = AUTO_CSC_DISABLE;
>   			c0_c2_change = C0_C2_CHANGE_DISABLE;
>   			csc_enable = v_CSC_ENABLE;
>   
> @@ -378,18 +339,18 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>   				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>   			return 0;
>   		}
>   	} else {
>   		if (inno_conn_state->colorimetry == HDMI_COLORIMETRY_ITU_601) {
> -			if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444) {
> +			if (conn_state->hdmi.output_format == HDMI_COLORSPACE_YUV444) {
>   				csc_mode = CSC_RGB_0_255_TO_ITU601_16_235_8BIT;
>   				auto_csc = AUTO_CSC_DISABLE;
>   				c0_c2_change = C0_C2_CHANGE_DISABLE;
>   				csc_enable = v_CSC_ENABLE;
>   			}
>   		} else {
> -			if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444) {
> +			if (conn_state->hdmi.output_format == HDMI_COLORSPACE_YUV444) {
>   				csc_mode = CSC_RGB_0_255_TO_ITU709_16_235_8BIT;
>   				auto_csc = AUTO_CSC_DISABLE;
>   				c0_c2_change = C0_C2_CHANGE_DISABLE;
>   				csc_enable = v_CSC_ENABLE;
>   			}
> @@ -460,14 +421,16 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
>   
>   	return 0;
>   }
>   
>   static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> -			   struct drm_display_mode *mode)
> +			   struct drm_crtc_state *new_crtc_state,
> +			   struct drm_connector_state *new_conn_state)
>   {
> -	struct drm_display_info *display = &hdmi->connector.display_info;
> -	unsigned long mpixelclock = mode->clock * 1000;
> +	struct drm_connector *connector = &hdmi->connector;
> +	struct drm_display_info *display = &connector->display_info;
> +	struct drm_display_mode *mode = &new_crtc_state->adjusted_mode;
>   
>   	/* Mute video and audio output */
>   	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
>   		  v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
>   
> @@ -477,26 +440,26 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>   
>   	inno_hdmi_config_video_timing(hdmi, mode);
>   
>   	inno_hdmi_config_video_csc(hdmi);
>   
> -	if (display->is_hdmi)
> -		inno_hdmi_config_video_avi(hdmi, mode);
> +	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
> +							   new_conn_state->state);
>   
>   	/*
>   	 * When IP controller have configured to an accurate video
>   	 * timing, then the TMDS clock source would be switched to
>   	 * DCLK_LCDC, so we need to init the TMDS rate to mode pixel
>   	 * clock rate, and reconfigure the DDC clock.
>   	 */
> -	inno_hdmi_i2c_init(hdmi, mpixelclock);
> +	inno_hdmi_i2c_init(hdmi, new_conn_state->hdmi.tmds_char_rate);
>   
>   	/* Unmute video and audio output */
>   	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
>   		  v_AUDIO_MUTE(0) | v_VIDEO_MUTE(0));
>   
> -	inno_hdmi_power_up(hdmi, mpixelclock);
> +	inno_hdmi_power_up(hdmi, new_conn_state->hdmi.tmds_char_rate);
>   
>   	return 0;
>   }
>   
>   static enum drm_mode_status inno_hdmi_display_mode_valid(struct inno_hdmi *hdmi,
> @@ -544,11 +507,11 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder,
>   
>   	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
>   	if (WARN_ON(!crtc_state))
>   		return;
>   
> -	inno_hdmi_setup(hdmi, &crtc_state->adjusted_mode);
> +	inno_hdmi_setup(hdmi, crtc_state, conn_state);
>   }
>   
>   static void inno_hdmi_encoder_disable(struct drm_encoder *encoder,
>   				      struct drm_atomic_state *state)
>   {
> @@ -561,11 +524,10 @@ static int
>   inno_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>   			       struct drm_crtc_state *crtc_state,
>   			       struct drm_connector_state *conn_state)
>   {
>   	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> -	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>   	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>   	u8 vic = drm_match_cea_mode(mode);
>   	struct inno_hdmi_connector_state *inno_conn_state =
>   					to_inno_hdmi_conn_state(conn_state);
>   
> @@ -578,16 +540,11 @@ inno_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>   	    vic == 17 || vic == 18)
>   		inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_601;
>   	else
>   		inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
>   
> -	inno_conn_state->enc_out_format = HDMI_COLORSPACE_RGB;
> -	inno_conn_state->rgb_limited_range =
> -		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> -
> -	return  inno_hdmi_display_mode_valid(hdmi,
> -				&crtc_state->adjusted_mode) == MODE_OK ? 0 : -EINVAL;
> +	return 0;
>   }
>   
>   static struct drm_encoder_helper_funcs inno_hdmi_encoder_helper_funcs = {
>   	.atomic_check	= inno_hdmi_encoder_atomic_check,
>   	.atomic_enable	= inno_hdmi_encoder_enable,
> @@ -660,14 +617,13 @@ static void inno_hdmi_connector_reset(struct drm_connector *connector)
>   	inno_conn_state = kzalloc(sizeof(*inno_conn_state), GFP_KERNEL);
>   	if (!inno_conn_state)
>   		return;
>   
>   	__drm_atomic_helper_connector_reset(connector, &inno_conn_state->base);
> +	__drm_atomic_helper_connector_hdmi_reset(connector, connector->state);
>   
>   	inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
> -	inno_conn_state->enc_out_format = HDMI_COLORSPACE_RGB;
> -	inno_conn_state->rgb_limited_range = false;
>   }
>   
>   static struct drm_connector_state *
>   inno_hdmi_connector_duplicate_state(struct drm_connector *connector)
>   {
> @@ -696,10 +652,11 @@ static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
>   	.atomic_duplicate_state = inno_hdmi_connector_duplicate_state,
>   	.atomic_destroy_state = inno_hdmi_connector_destroy_state,
>   };
>   
>   static struct drm_connector_helper_funcs inno_hdmi_connector_helper_funcs = {
> +	.atomic_check = drm_atomic_helper_connector_hdmi_check,
>   	.get_modes = inno_hdmi_connector_get_modes,
>   	.mode_valid = inno_hdmi_connector_mode_valid,
>   };
>   
>   static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
> @@ -723,14 +680,18 @@ static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
>   
>   	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>   
>   	drm_connector_helper_add(&hdmi->connector,
>   				 &inno_hdmi_connector_helper_funcs);
> -	drm_connector_init_with_ddc(drm, &hdmi->connector,
> -				    &inno_hdmi_connector_funcs,
> -				    DRM_MODE_CONNECTOR_HDMIA,
> -				    hdmi->ddc);
> +	drmm_connector_hdmi_init(drm, &hdmi->connector,
> +				 "Rockchip", "Inno HDMI",
> +				 &inno_hdmi_connector_funcs,
> +				 &inno_hdmi_hdmi_connector_funcs,
> +				 DRM_MODE_CONNECTOR_HDMIA,
> +				 hdmi->ddc,
> +				 BIT(HDMI_COLORSPACE_RGB),
> +				 8);
>   
>   	drm_connector_attach_encoder(&hdmi->connector, encoder);
>   
>   	return 0;
>   }
>
Andy Yan April 27, 2024, 10:44 a.m. UTC | #2 
Hi Maxime,

On 4/23/24 18:45, Maxime Ripard wrote:
> The new HDMI connector infrastructure allows to remove some boilerplate,
> especially to generate infoframes. Let's switch to it.
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/rockchip/Kconfig     |   1 +
>   drivers/gpu/drm/rockchip/inno_hdmi.c | 143 +++++++++++++----------------------
>   2 files changed, 53 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
> index 4c7072e6e34e..e2ec20c2e2eb 100644
> --- a/drivers/gpu/drm/rockchip/Kconfig
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -72,10 +72,11 @@ config ROCKCHIP_DW_MIPI_DSI
>   	  enable MIPI DSI on RK3288 or RK3399 based SoC, you should
>   	  select this option.
>   
>   config ROCKCHIP_INNO_HDMI
>   	bool "Rockchip specific extensions for Innosilicon HDMI"
> +	depends on DRM_HDMI_STATE_HELPER
>   	help
>   	  This selects support for Rockchip SoC specific extensions
>   	  for the Innosilicon HDMI driver. If you want to enable
>   	  HDMI on RK3036 based SoC, you should select this option.
>   
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 1d2261643743..3756259bfb10 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -65,13 +65,11 @@ struct inno_hdmi {
>   	const struct inno_hdmi_variant *variant;
>   };
>   

I think we should include <drm/display/drm_hdmi_state_helper.h>, or the compile will complain about:
implicit decalaration of funciont drm_atomic_helper_connector_hdmi_update_infoframes

>   struct inno_hdmi_connector_state {
>   	struct drm_connector_state	base;
> -	unsigned int			enc_out_format;
>   	unsigned int			colorimetry;
> -	bool				rgb_limited_range;
>   };
>   
>   static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
>   {
>   	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> @@ -255,90 +253,53 @@ static void inno_hdmi_reset(struct inno_hdmi *hdmi)
>   	hdmi_modb(hdmi, HDMI_SYS_CTRL, msk, val);
>   
>   	inno_hdmi_standby(hdmi);
>   }
>   
> -static void inno_hdmi_disable_frame(struct inno_hdmi *hdmi,
> -				    enum hdmi_infoframe_type type)
> +static int inno_hdmi_disable_frame(struct drm_connector *connector,
> +				   enum hdmi_infoframe_type type)
>   {
> -	struct drm_connector *connector = &hdmi->connector;
> -
> -	if (type != HDMI_INFOFRAME_TYPE_AVI) {
> -		drm_err(connector->dev,
> -			"Unsupported infoframe type: %u\n", type);
> -		return;
> -	}
> -
> -	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
> -}
> -
> -static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi,
> -				  union hdmi_infoframe *frame, enum hdmi_infoframe_type type)
> -{
> -	struct drm_connector *connector = &hdmi->connector;
> -	u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
> -	ssize_t rc, i;
> +	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
>   
>   	if (type != HDMI_INFOFRAME_TYPE_AVI) {
>   		drm_err(connector->dev,
>   			"Unsupported infoframe type: %u\n", type);
>   		return 0;
>   	}
>   
> -	inno_hdmi_disable_frame(hdmi, type);
> +	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
>   
> -	rc = hdmi_infoframe_pack(frame, packed_frame,
> -				 sizeof(packed_frame));
> -	if (rc < 0)
> -		return rc;
> +	return 0;
> +}
>   
> -	for (i = 0; i < rc; i++)
> +static int inno_hdmi_upload_frame(struct drm_connector *connector,
> +				  enum hdmi_infoframe_type type,
> +				  const u8 *buffer, size_t len)
> +{
> +	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
> +	u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
> +	ssize_t i;
> +
> +	if (type != HDMI_INFOFRAME_TYPE_AVI) {
> +		drm_err(connector->dev,
> +			"Unsupported infoframe type: %u\n", type);
> +		return 0;
> +	}
> +
> +	inno_hdmi_disable_frame(connector, type);
> +
> +	for (i = 0; i < len; i++)
>   		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i,
>   			    packed_frame[i]);
>   
>   	return 0;
>   }
>   
> -static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> -				      struct drm_display_mode *mode)
> -{
> -	struct drm_connector *connector = &hdmi->connector;
> -	struct drm_connector_state *conn_state = connector->state;
> -	struct inno_hdmi_connector_state *inno_conn_state =
> -					to_inno_hdmi_conn_state(conn_state);
> -	union hdmi_infoframe frame;
> -	int rc;
> -
> -	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> -						      &hdmi->connector,
> -						      mode);
> -	if (rc) {
> -		inno_hdmi_disable_frame(hdmi, HDMI_INFOFRAME_TYPE_AVI);
> -		return rc;
> -	}
> -
> -	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444)
> -		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> -	else if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV422)
> -		frame.avi.colorspace = HDMI_COLORSPACE_YUV422;
> -	else
> -		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> -
> -	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
> -		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> -						   connector, mode,
> -						   inno_conn_state->rgb_limited_range ?
> -						   HDMI_QUANTIZATION_RANGE_LIMITED :
> -						   HDMI_QUANTIZATION_RANGE_FULL);
> -	} else {
> -		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> -		frame.avi.ycc_quantization_range =
> -			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> -	}
> -
> -	return inno_hdmi_upload_frame(hdmi, &frame, HDMI_INFOFRAME_TYPE_AVI);
> -}
> +static const struct drm_connector_hdmi_funcs inno_hdmi_hdmi_connector_funcs = {
> +	.clear_infoframe	= inno_hdmi_disable_frame,
> +	.write_infoframe	= inno_hdmi_upload_frame,
> +};
>   
>   static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>   {
>   	struct drm_connector *connector = &hdmi->connector;
>   	struct drm_connector_state *conn_state = connector->state;
> @@ -359,12 +320,12 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>   	value = v_VIDEO_INPUT_BITS(VIDEO_INPUT_8BITS) |
>   		v_VIDEO_OUTPUT_COLOR(0) |
>   		v_VIDEO_INPUT_CSP(0);
>   	hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL2, value);
>   
> -	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
> -		if (inno_conn_state->rgb_limited_range) {
> +	if (conn_state->hdmi.output_format == HDMI_COLORSPACE_RGB) {
> +		if (conn_state->hdmi.is_limited_range) {
>   			csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>   			auto_csc = AUTO_CSC_DISABLE;
>   			c0_c2_change = C0_C2_CHANGE_DISABLE;
>   			csc_enable = v_CSC_ENABLE;
>   
> @@ -378,18 +339,18 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>   				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>   			return 0;
>   		}
>   	} else {
>   		if (inno_conn_state->colorimetry == HDMI_COLORIMETRY_ITU_601) {
> -			if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444) {
> +			if (conn_state->hdmi.output_format == HDMI_COLORSPACE_YUV444) {
>   				csc_mode = CSC_RGB_0_255_TO_ITU601_16_235_8BIT;
>   				auto_csc = AUTO_CSC_DISABLE;
>   				c0_c2_change = C0_C2_CHANGE_DISABLE;
>   				csc_enable = v_CSC_ENABLE;
>   			}
>   		} else {
> -			if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444) {
> +			if (conn_state->hdmi.output_format == HDMI_COLORSPACE_YUV444) {
>   				csc_mode = CSC_RGB_0_255_TO_ITU709_16_235_8BIT;
>   				auto_csc = AUTO_CSC_DISABLE;
>   				c0_c2_change = C0_C2_CHANGE_DISABLE;
>   				csc_enable = v_CSC_ENABLE;
>   			}
> @@ -460,14 +421,16 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
>   
>   	return 0;
>   }
>   
>   static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> -			   struct drm_display_mode *mode)
> +			   struct drm_crtc_state *new_crtc_state,
> +			   struct drm_connector_state *new_conn_state)
>   {
> -	struct drm_display_info *display = &hdmi->connector.display_info;
> -	unsigned long mpixelclock = mode->clock * 1000;
> +	struct drm_connector *connector = &hdmi->connector;
> +	struct drm_display_info *display = &connector->display_info;
> +	struct drm_display_mode *mode = &new_crtc_state->adjusted_mode;
>   
>   	/* Mute video and audio output */
>   	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
>   		  v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
>   
> @@ -477,26 +440,26 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>   
>   	inno_hdmi_config_video_timing(hdmi, mode);
>   
>   	inno_hdmi_config_video_csc(hdmi);
>   
> -	if (display->is_hdmi)
> -		inno_hdmi_config_video_avi(hdmi, mode);
> +	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
> +							   new_conn_state->state);
>   
>   	/*
>   	 * When IP controller have configured to an accurate video
>   	 * timing, then the TMDS clock source would be switched to
>   	 * DCLK_LCDC, so we need to init the TMDS rate to mode pixel
>   	 * clock rate, and reconfigure the DDC clock.
>   	 */
> -	inno_hdmi_i2c_init(hdmi, mpixelclock);
> +	inno_hdmi_i2c_init(hdmi, new_conn_state->hdmi.tmds_char_rate);
>   
>   	/* Unmute video and audio output */
>   	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
>   		  v_AUDIO_MUTE(0) | v_VIDEO_MUTE(0));
>   
> -	inno_hdmi_power_up(hdmi, mpixelclock);
> +	inno_hdmi_power_up(hdmi, new_conn_state->hdmi.tmds_char_rate);
>   
>   	return 0;
>   }
>   
>   static enum drm_mode_status inno_hdmi_display_mode_valid(struct inno_hdmi *hdmi,
> @@ -544,11 +507,11 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder,
>   
>   	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
>   	if (WARN_ON(!crtc_state))
>   		return;
>   
> -	inno_hdmi_setup(hdmi, &crtc_state->adjusted_mode);
> +	inno_hdmi_setup(hdmi, crtc_state, conn_state);
>   }
>   
>   static void inno_hdmi_encoder_disable(struct drm_encoder *encoder,
>   				      struct drm_atomic_state *state)
>   {
> @@ -561,11 +524,10 @@ static int
>   inno_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>   			       struct drm_crtc_state *crtc_state,
>   			       struct drm_connector_state *conn_state)
>   {
>   	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> -	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>   	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>   	u8 vic = drm_match_cea_mode(mode);
>   	struct inno_hdmi_connector_state *inno_conn_state =
>   					to_inno_hdmi_conn_state(conn_state);
>   
> @@ -578,16 +540,11 @@ inno_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>   	    vic == 17 || vic == 18)
>   		inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_601;
>   	else
>   		inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
>   
> -	inno_conn_state->enc_out_format = HDMI_COLORSPACE_RGB;
> -	inno_conn_state->rgb_limited_range =
> -		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> -
> -	return  inno_hdmi_display_mode_valid(hdmi,
> -				&crtc_state->adjusted_mode) == MODE_OK ? 0 : -EINVAL;
> +	return 0;
>   }
>   
>   static struct drm_encoder_helper_funcs inno_hdmi_encoder_helper_funcs = {
>   	.atomic_check	= inno_hdmi_encoder_atomic_check,
>   	.atomic_enable	= inno_hdmi_encoder_enable,
> @@ -660,14 +617,13 @@ static void inno_hdmi_connector_reset(struct drm_connector *connector)
>   	inno_conn_state = kzalloc(sizeof(*inno_conn_state), GFP_KERNEL);
>   	if (!inno_conn_state)
>   		return;
>   
>   	__drm_atomic_helper_connector_reset(connector, &inno_conn_state->base);
> +	__drm_atomic_helper_connector_hdmi_reset(connector, connector->state);
>   
>   	inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
> -	inno_conn_state->enc_out_format = HDMI_COLORSPACE_RGB;
> -	inno_conn_state->rgb_limited_range = false;
>   }
>   
>   static struct drm_connector_state *
>   inno_hdmi_connector_duplicate_state(struct drm_connector *connector)
>   {
> @@ -696,10 +652,11 @@ static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
>   	.atomic_duplicate_state = inno_hdmi_connector_duplicate_state,
>   	.atomic_destroy_state = inno_hdmi_connector_destroy_state,
>   };
>   
>   static struct drm_connector_helper_funcs inno_hdmi_connector_helper_funcs = {
> +	.atomic_check = drm_atomic_helper_connector_hdmi_check,
>   	.get_modes = inno_hdmi_connector_get_modes,
>   	.mode_valid = inno_hdmi_connector_mode_valid,
>   };
>   
>   static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
> @@ -723,14 +680,18 @@ static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
>   
>   	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>   
>   	drm_connector_helper_add(&hdmi->connector,
>   				 &inno_hdmi_connector_helper_funcs);
> -	drm_connector_init_with_ddc(drm, &hdmi->connector,
> -				    &inno_hdmi_connector_funcs,
> -				    DRM_MODE_CONNECTOR_HDMIA,
> -				    hdmi->ddc);
> +	drmm_connector_hdmi_init(drm, &hdmi->connector,
> +				 "Rockchip", "Inno HDMI",
> +				 &inno_hdmi_connector_funcs,
> +				 &inno_hdmi_hdmi_connector_funcs,
> +				 DRM_MODE_CONNECTOR_HDMIA,
> +				 hdmi->ddc,
> +				 BIT(HDMI_COLORSPACE_RGB),
> +				 8);
     This function call will failed as inno_hdmi_connector_funcs has a destory callback.
     drmm_connector_init have the check:
     if (drm_WARN_ON(dev, funcs && funcs->destory))
         return -EINVAL.

I think the patch for sun4i has the same issue.

>   
>   	drm_connector_attach_encoder(&hdmi->connector, encoder);
>   
>   	return 0;
>   }
> 

And after this whole series applied on linux 6.9-rc4, the display on rk3036 kylin is lost, I get
the following error:
[  178.999421] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] checking 87b7fbde
[  178.999471] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] mode changed
[  178.999498] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] enable changed
[  178.999521] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] active changed
[  178.999547] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:37:HDMI-A-1]
[  178.999575] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] using [ENCODER:36:TMDS-36] on [CRTC:35:crtc-0]
[  178.999604] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying with a 8 bpc output
[  178.999636] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying RGB output format
[  178.999730] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB Format, checking the constraints.
[  178.999757] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB output format not supported with 8 bpc
[  178.999783] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Failed. No Format Supported for that bpc count.
[  178.999810] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] driver check failed
[  178.999836] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] atomic driver check for 87b7fbde failed: -22
[  178.999868] rockchip-drm display-subsystem: [drm:drm_atomic_state_default_clear] Clearing atomic state 87b7fbde
[  178.999898] [drm:drm_mode_object_put] OBJ ID: 37 (4)
[  178.999922] [drm:drm_mode_object_put] OBJ ID: 37 (3)
[  178.999944] [drm:drm_mode_object_put] OBJ ID: 40 (1)
[  178.999971] [drm:drm_mode_object_put] OBJ ID: 39 (2)
Maxime Ripard May 6, 2024, 7:49 a.m. UTC | #3 
Hi,

On Sat, Apr 27, 2024 at 06:12:26PM GMT, Andy Yan wrote:
> On 4/23/24 18:45, Maxime Ripard wrote:
> > The new HDMI connector infrastructure allows to remove some boilerplate,
> > especially to generate infoframes. Let's switch to it.
> > 
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Acked-by: Heiko Stuebner <heiko@sntech.de>
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >   drivers/gpu/drm/rockchip/Kconfig     |   1 +
> >   drivers/gpu/drm/rockchip/inno_hdmi.c | 143 +++++++++++++----------------------
> >   2 files changed, 53 insertions(+), 91 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
> > index 4c7072e6e34e..e2ec20c2e2eb 100644
> > --- a/drivers/gpu/drm/rockchip/Kconfig
> > +++ b/drivers/gpu/drm/rockchip/Kconfig
> > @@ -72,10 +72,11 @@ config ROCKCHIP_DW_MIPI_DSI
> >   	  enable MIPI DSI on RK3288 or RK3399 based SoC, you should
> >   	  select this option.
> >   config ROCKCHIP_INNO_HDMI
> >   	bool "Rockchip specific extensions for Innosilicon HDMI"
> > +	depends on
> 
> Is this supposed to be  DRM_DISPLAY_HDMI_STATE_HELPER whith you introduced in PATCH 04/28?
> I couldn't find any place where  DRM_HDMI_STATE_HELPER is defined.

Yes, it was a typo indeed. I've fixed it in the next version.

Thanks!
Maxime
Maxime Ripard May 6, 2024, 7:59 a.m. UTC | #4 
Hi Andy,

Thanks a lot for giving it a try

All the issues you raised in your review are fixed.

On Sat, Apr 27, 2024 at 06:44:54PM GMT, Andy Yan wrote:
> And after this whole series applied on linux 6.9-rc4, the display on rk3036 kylin is lost, I get
> the following error:
> [  178.999421] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] checking 87b7fbde
> [  178.999471] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] mode changed
> [  178.999498] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] enable changed
> [  178.999521] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] active changed
> [  178.999547] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:37:HDMI-A-1]
> [  178.999575] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] using [ENCODER:36:TMDS-36] on [CRTC:35:crtc-0]
> [  178.999604] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying with a 8 bpc output
> [  178.999636] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying RGB output format
> [  178.999730] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB Format, checking the constraints.
> [  178.999757] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB output format not supported with 8 bpc
> [  178.999783] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Failed. No Format Supported for that bpc count.
> [  178.999810] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] driver check failed
> [  178.999836] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] atomic driver check for 87b7fbde failed: -22
> [  178.999868] rockchip-drm display-subsystem: [drm:drm_atomic_state_default_clear] Clearing atomic state 87b7fbde
> [  178.999898] [drm:drm_mode_object_put] OBJ ID: 37 (4)
> [  178.999922] [drm:drm_mode_object_put] OBJ ID: 37 (3)
> [  178.999944] [drm:drm_mode_object_put] OBJ ID: 40 (1)
> [  178.999971] [drm:drm_mode_object_put] OBJ ID: 39 (2)

Which kind of display are you testing with? The only reason it could do
so is if the display claim it doesn't support RGB in its EDID which is
contradicting the HDMI spec. Are you using an Analog display by any
chance? or the built-in EDIDs through the drm.edid_firmware mechanism?

Maxime
Andy Yan May 6, 2024, 8:22 a.m. UTC | #5 
Hi Maxime,

On 5/6/24 15:59, Maxime Ripard wrote:
> Hi Andy,
> 
> Thanks a lot for giving it a try
> 
> All the issues you raised in your review are fixed.
> 
> On Sat, Apr 27, 2024 at 06:44:54PM GMT, Andy Yan wrote:
>> And after this whole series applied on linux 6.9-rc4, the display on rk3036 kylin is lost, I get
>> the following error:
>> [  178.999421] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] checking 87b7fbde
>> [  178.999471] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] mode changed
>> [  178.999498] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] enable changed
>> [  178.999521] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] active changed
>> [  178.999547] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:37:HDMI-A-1]
>> [  178.999575] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] using [ENCODER:36:TMDS-36] on [CRTC:35:crtc-0]
>> [  178.999604] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying with a 8 bpc output
>> [  178.999636] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying RGB output format
>> [  178.999730] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB Format, checking the constraints.
>> [  178.999757] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB output format not supported with 8 bpc
>> [  178.999783] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Failed. No Format Supported for that bpc count.
>> [  178.999810] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] driver check failed
>> [  178.999836] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] atomic driver check for 87b7fbde failed: -22
>> [  178.999868] rockchip-drm display-subsystem: [drm:drm_atomic_state_default_clear] Clearing atomic state 87b7fbde
>> [  178.999898] [drm:drm_mode_object_put] OBJ ID: 37 (4)
>> [  178.999922] [drm:drm_mode_object_put] OBJ ID: 37 (3)
>> [  178.999944] [drm:drm_mode_object_put] OBJ ID: 40 (1)
>> [  178.999971] [drm:drm_mode_object_put] OBJ ID: 39 (2)
> 
> Which kind of display are you testing with? The only reason it could do
> so is if the display claim it doesn't support RGB in its EDID which is
> contradicting the HDMI spec. Are you using an Analog display by any
> chance? or the built-in EDIDs through the drm.edid_firmware mechanism?

It is because there is something wrong(I still don't know why) with the DDC on my board.
It always failed to read edid when it first bootup, that means inno_hdmi_connector_get_modes
will return 0.

and in function

drm_helper_probe_single_connector_modes:

         count = drm_helper_probe_get_modes(connector);

         if (count == 0 && (connector->status == connector_status_connected ||
                            connector->status == connector_status_unknown)) {
                 count = drm_add_modes_noedid(connector, 1024, 768);

                 /*
                  * Section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a
                  * Link CTS specifies that 640x480 (the official "failsafe"
                  * mode) needs to be the default if there's no EDID.
                  */
                 if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
                         drm_set_preferred_mode(connector, 640, 480);
         }
drm_add_modes_noedid will not initialize display_info.

> 
> Maxime
Andy Yan May 7, 2024, 1:32 a.m. UTC | #6 
Hi Maxime,

On 5/6/24 15:59, Maxime Ripard wrote:
> Hi Andy,
> 
> Thanks a lot for giving it a try
> 
> All the issues you raised in your review are fixed.
> 
> On Sat, Apr 27, 2024 at 06:44:54PM GMT, Andy Yan wrote:
>> And after this whole series applied on linux 6.9-rc4, the display on rk3036 kylin is lost, I get
>> the following error:
>> [  178.999421] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] checking 87b7fbde
>> [  178.999471] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] mode changed
>> [  178.999498] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] enable changed
>> [  178.999521] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] active changed
>> [  178.999547] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:37:HDMI-A-1]
>> [  178.999575] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] using [ENCODER:36:TMDS-36] on [CRTC:35:crtc-0]
>> [  178.999604] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying with a 8 bpc output
>> [  178.999636] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying RGB output format
>> [  178.999730] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB Format, checking the constraints.
>> [  178.999757] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB output format not supported with 8 bpc
>> [  178.999783] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Failed. No Format Supported for that bpc count.
>> [  178.999810] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] driver check failed
>> [  178.999836] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] atomic driver check for 87b7fbde failed: -22
>> [  178.999868] rockchip-drm display-subsystem: [drm:drm_atomic_state_default_clear] Clearing atomic state 87b7fbde
>> [  178.999898] [drm:drm_mode_object_put] OBJ ID: 37 (4)
>> [  178.999922] [drm:drm_mode_object_put] OBJ ID: 37 (3)
>> [  178.999944] [drm:drm_mode_object_put] OBJ ID: 40 (1)
>> [  178.999971] [drm:drm_mode_object_put] OBJ ID: 39 (2)
> 
> Which kind of display are you testing with? The only reason it could do
> so is if the display claim it doesn't support RGB in its EDID which is
> contradicting the HDMI spec. Are you using an Analog display by any
> chance? or the built-in EDIDs through the drm.edid_firmware mechanism?
> 

It is because there is something wrong(I still don't know why) with the DDC on my board.
It always failed to read edid when it first bootup, that means inno_hdmi_connector_get_modes
will return 0.

and in function

drm_helper_probe_single_connector_modes:

         count = drm_helper_probe_get_modes(connector);

         if (count == 0 && (connector->status == connector_status_connected ||
                            connector->status == connector_status_unknown)) {
                 count = drm_add_modes_noedid(connector, 1024, 768);

                 /*
                  * Section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a
                  * Link CTS specifies that 640x480 (the official "failsafe"
                  * mode) needs to be the default if there's no EDID.
                  */
                 if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
                         drm_set_preferred_mode(connector, 640, 480);
         }
drm_add_modes_noedid will not initialize display_info.

> Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index 4c7072e6e34e..e2ec20c2e2eb 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -72,10 +72,11 @@  config ROCKCHIP_DW_MIPI_DSI
 	  enable MIPI DSI on RK3288 or RK3399 based SoC, you should
 	  select this option.
 
 config ROCKCHIP_INNO_HDMI
 	bool "Rockchip specific extensions for Innosilicon HDMI"
+	depends on DRM_HDMI_STATE_HELPER
 	help
 	  This selects support for Rockchip SoC specific extensions
 	  for the Innosilicon HDMI driver. If you want to enable
 	  HDMI on RK3036 based SoC, you should select this option.
 
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 1d2261643743..3756259bfb10 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -65,13 +65,11 @@  struct inno_hdmi {
 	const struct inno_hdmi_variant *variant;
 };
 
 struct inno_hdmi_connector_state {
 	struct drm_connector_state	base;
-	unsigned int			enc_out_format;
 	unsigned int			colorimetry;
-	bool				rgb_limited_range;
 };
 
 static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
 {
 	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
@@ -255,90 +253,53 @@  static void inno_hdmi_reset(struct inno_hdmi *hdmi)
 	hdmi_modb(hdmi, HDMI_SYS_CTRL, msk, val);
 
 	inno_hdmi_standby(hdmi);
 }
 
-static void inno_hdmi_disable_frame(struct inno_hdmi *hdmi,
-				    enum hdmi_infoframe_type type)
+static int inno_hdmi_disable_frame(struct drm_connector *connector,
+				   enum hdmi_infoframe_type type)
 {
-	struct drm_connector *connector = &hdmi->connector;
-
-	if (type != HDMI_INFOFRAME_TYPE_AVI) {
-		drm_err(connector->dev,
-			"Unsupported infoframe type: %u\n", type);
-		return;
-	}
-
-	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
-}
-
-static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi,
-				  union hdmi_infoframe *frame, enum hdmi_infoframe_type type)
-{
-	struct drm_connector *connector = &hdmi->connector;
-	u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
-	ssize_t rc, i;
+	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
 
 	if (type != HDMI_INFOFRAME_TYPE_AVI) {
 		drm_err(connector->dev,
 			"Unsupported infoframe type: %u\n", type);
 		return 0;
 	}
 
-	inno_hdmi_disable_frame(hdmi, type);
+	hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI);
 
-	rc = hdmi_infoframe_pack(frame, packed_frame,
-				 sizeof(packed_frame));
-	if (rc < 0)
-		return rc;
+	return 0;
+}
 
-	for (i = 0; i < rc; i++)
+static int inno_hdmi_upload_frame(struct drm_connector *connector,
+				  enum hdmi_infoframe_type type,
+				  const u8 *buffer, size_t len)
+{
+	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
+	u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
+	ssize_t i;
+
+	if (type != HDMI_INFOFRAME_TYPE_AVI) {
+		drm_err(connector->dev,
+			"Unsupported infoframe type: %u\n", type);
+		return 0;
+	}
+
+	inno_hdmi_disable_frame(connector, type);
+
+	for (i = 0; i < len; i++)
 		hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i,
 			    packed_frame[i]);
 
 	return 0;
 }
 
-static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
-				      struct drm_display_mode *mode)
-{
-	struct drm_connector *connector = &hdmi->connector;
-	struct drm_connector_state *conn_state = connector->state;
-	struct inno_hdmi_connector_state *inno_conn_state =
-					to_inno_hdmi_conn_state(conn_state);
-	union hdmi_infoframe frame;
-	int rc;
-
-	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						      &hdmi->connector,
-						      mode);
-	if (rc) {
-		inno_hdmi_disable_frame(hdmi, HDMI_INFOFRAME_TYPE_AVI);
-		return rc;
-	}
-
-	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444)
-		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
-	else if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV422)
-		frame.avi.colorspace = HDMI_COLORSPACE_YUV422;
-	else
-		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
-
-	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
-		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
-						   connector, mode,
-						   inno_conn_state->rgb_limited_range ?
-						   HDMI_QUANTIZATION_RANGE_LIMITED :
-						   HDMI_QUANTIZATION_RANGE_FULL);
-	} else {
-		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
-		frame.avi.ycc_quantization_range =
-			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
-	}
-
-	return inno_hdmi_upload_frame(hdmi, &frame, HDMI_INFOFRAME_TYPE_AVI);
-}
+static const struct drm_connector_hdmi_funcs inno_hdmi_hdmi_connector_funcs = {
+	.clear_infoframe	= inno_hdmi_disable_frame,
+	.write_infoframe	= inno_hdmi_upload_frame,
+};
 
 static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
 {
 	struct drm_connector *connector = &hdmi->connector;
 	struct drm_connector_state *conn_state = connector->state;
@@ -359,12 +320,12 @@  static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
 	value = v_VIDEO_INPUT_BITS(VIDEO_INPUT_8BITS) |
 		v_VIDEO_OUTPUT_COLOR(0) |
 		v_VIDEO_INPUT_CSP(0);
 	hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL2, value);
 
-	if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
-		if (inno_conn_state->rgb_limited_range) {
+	if (conn_state->hdmi.output_format == HDMI_COLORSPACE_RGB) {
+		if (conn_state->hdmi.is_limited_range) {
 			csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
 			auto_csc = AUTO_CSC_DISABLE;
 			c0_c2_change = C0_C2_CHANGE_DISABLE;
 			csc_enable = v_CSC_ENABLE;
 
@@ -378,18 +339,18 @@  static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
 				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
 			return 0;
 		}
 	} else {
 		if (inno_conn_state->colorimetry == HDMI_COLORIMETRY_ITU_601) {
-			if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444) {
+			if (conn_state->hdmi.output_format == HDMI_COLORSPACE_YUV444) {
 				csc_mode = CSC_RGB_0_255_TO_ITU601_16_235_8BIT;
 				auto_csc = AUTO_CSC_DISABLE;
 				c0_c2_change = C0_C2_CHANGE_DISABLE;
 				csc_enable = v_CSC_ENABLE;
 			}
 		} else {
-			if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_YUV444) {
+			if (conn_state->hdmi.output_format == HDMI_COLORSPACE_YUV444) {
 				csc_mode = CSC_RGB_0_255_TO_ITU709_16_235_8BIT;
 				auto_csc = AUTO_CSC_DISABLE;
 				c0_c2_change = C0_C2_CHANGE_DISABLE;
 				csc_enable = v_CSC_ENABLE;
 			}
@@ -460,14 +421,16 @@  static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
 
 	return 0;
 }
 
 static int inno_hdmi_setup(struct inno_hdmi *hdmi,
-			   struct drm_display_mode *mode)
+			   struct drm_crtc_state *new_crtc_state,
+			   struct drm_connector_state *new_conn_state)
 {
-	struct drm_display_info *display = &hdmi->connector.display_info;
-	unsigned long mpixelclock = mode->clock * 1000;
+	struct drm_connector *connector = &hdmi->connector;
+	struct drm_display_info *display = &connector->display_info;
+	struct drm_display_mode *mode = &new_crtc_state->adjusted_mode;
 
 	/* Mute video and audio output */
 	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
 		  v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
 
@@ -477,26 +440,26 @@  static int inno_hdmi_setup(struct inno_hdmi *hdmi,
 
 	inno_hdmi_config_video_timing(hdmi, mode);
 
 	inno_hdmi_config_video_csc(hdmi);
 
-	if (display->is_hdmi)
-		inno_hdmi_config_video_avi(hdmi, mode);
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
+							   new_conn_state->state);
 
 	/*
 	 * When IP controller have configured to an accurate video
 	 * timing, then the TMDS clock source would be switched to
 	 * DCLK_LCDC, so we need to init the TMDS rate to mode pixel
 	 * clock rate, and reconfigure the DDC clock.
 	 */
-	inno_hdmi_i2c_init(hdmi, mpixelclock);
+	inno_hdmi_i2c_init(hdmi, new_conn_state->hdmi.tmds_char_rate);
 
 	/* Unmute video and audio output */
 	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
 		  v_AUDIO_MUTE(0) | v_VIDEO_MUTE(0));
 
-	inno_hdmi_power_up(hdmi, mpixelclock);
+	inno_hdmi_power_up(hdmi, new_conn_state->hdmi.tmds_char_rate);
 
 	return 0;
 }
 
 static enum drm_mode_status inno_hdmi_display_mode_valid(struct inno_hdmi *hdmi,
@@ -544,11 +507,11 @@  static void inno_hdmi_encoder_enable(struct drm_encoder *encoder,
 
 	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
 	if (WARN_ON(!crtc_state))
 		return;
 
-	inno_hdmi_setup(hdmi, &crtc_state->adjusted_mode);
+	inno_hdmi_setup(hdmi, crtc_state, conn_state);
 }
 
 static void inno_hdmi_encoder_disable(struct drm_encoder *encoder,
 				      struct drm_atomic_state *state)
 {
@@ -561,11 +524,10 @@  static int
 inno_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 			       struct drm_crtc_state *crtc_state,
 			       struct drm_connector_state *conn_state)
 {
 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
-	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	u8 vic = drm_match_cea_mode(mode);
 	struct inno_hdmi_connector_state *inno_conn_state =
 					to_inno_hdmi_conn_state(conn_state);
 
@@ -578,16 +540,11 @@  inno_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 	    vic == 17 || vic == 18)
 		inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_601;
 	else
 		inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
 
-	inno_conn_state->enc_out_format = HDMI_COLORSPACE_RGB;
-	inno_conn_state->rgb_limited_range =
-		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
-
-	return  inno_hdmi_display_mode_valid(hdmi,
-				&crtc_state->adjusted_mode) == MODE_OK ? 0 : -EINVAL;
+	return 0;
 }
 
 static struct drm_encoder_helper_funcs inno_hdmi_encoder_helper_funcs = {
 	.atomic_check	= inno_hdmi_encoder_atomic_check,
 	.atomic_enable	= inno_hdmi_encoder_enable,
@@ -660,14 +617,13 @@  static void inno_hdmi_connector_reset(struct drm_connector *connector)
 	inno_conn_state = kzalloc(sizeof(*inno_conn_state), GFP_KERNEL);
 	if (!inno_conn_state)
 		return;
 
 	__drm_atomic_helper_connector_reset(connector, &inno_conn_state->base);
+	__drm_atomic_helper_connector_hdmi_reset(connector, connector->state);
 
 	inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
-	inno_conn_state->enc_out_format = HDMI_COLORSPACE_RGB;
-	inno_conn_state->rgb_limited_range = false;
 }
 
 static struct drm_connector_state *
 inno_hdmi_connector_duplicate_state(struct drm_connector *connector)
 {
@@ -696,10 +652,11 @@  static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
 	.atomic_duplicate_state = inno_hdmi_connector_duplicate_state,
 	.atomic_destroy_state = inno_hdmi_connector_destroy_state,
 };
 
 static struct drm_connector_helper_funcs inno_hdmi_connector_helper_funcs = {
+	.atomic_check = drm_atomic_helper_connector_hdmi_check,
 	.get_modes = inno_hdmi_connector_get_modes,
 	.mode_valid = inno_hdmi_connector_mode_valid,
 };
 
 static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
@@ -723,14 +680,18 @@  static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
 
 	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
 
 	drm_connector_helper_add(&hdmi->connector,
 				 &inno_hdmi_connector_helper_funcs);
-	drm_connector_init_with_ddc(drm, &hdmi->connector,
-				    &inno_hdmi_connector_funcs,
-				    DRM_MODE_CONNECTOR_HDMIA,
-				    hdmi->ddc);
+	drmm_connector_hdmi_init(drm, &hdmi->connector,
+				 "Rockchip", "Inno HDMI",
+				 &inno_hdmi_connector_funcs,
+				 &inno_hdmi_hdmi_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA,
+				 hdmi->ddc,
+				 BIT(HDMI_COLORSPACE_RGB),
+				 8);
 
 	drm_connector_attach_encoder(&hdmi->connector, encoder);
 
 	return 0;
 }