[1/4] drm/edid: Pass connector to AVI inforframe functions

Commit Message

Ville Syrjälä Nov. 20, 2018, 4:13 p.m. UTC

From: Ville Syrjälä <[e-mail]>

Make life easier for drivers by simply passing the connector
to drm_hdmi_avi_infoframe_from_display_mode() and
drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
need to worry about is_hdmi2_sink mess.

Cc: Alex Deucher <[e-mail]>
Cc: "Christian König" <[e-mail]>
Cc: "David (ChunMing) Zhou" <[e-mail]>
Cc: Archit Taneja <[e-mail]>
Cc: Andrzej Hajda <[e-mail]>
Cc: Laurent Pinchart <[e-mail]>
Cc: Inki Dae <[e-mail]>
Cc: Joonyoung Shim <[e-mail]>
Cc: Seung-Woo Kim <[e-mail]>
Cc: Kyungmin Park <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: CK Hu <[e-mail]>
Cc: Philipp Zabel <[e-mail]>
Cc: Rob Clark <[e-mail]>
Cc: Ben Skeggs <[e-mail]>
Cc: Tomi Valkeinen <[e-mail]>
Cc: Sandy Huang <[e-mail]>
Cc: "Heiko Stübner" <[e-mail]>
Cc: Benjamin Gaignard <[e-mail]>
Cc: Vincent Abriou <[e-mail]>
Cc: Thierry Reding <[e-mail]>
Cc: Eric Anholt <[e-mail]>
Cc: Shawn Guo <[e-mail]>
Cc: Ilia Mirkin <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
 drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
 drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
 drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
 drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
 drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
 drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
 drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
 drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
 drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
 drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
 drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
 drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
 drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
 drivers/gpu/drm/tegra/sor.c               |  3 ++-
 drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
 drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
 include/drm/drm_edid.h                    |  8 +++---
 27 files changed, 94 insertions(+), 66 deletions(-)

Comments

Thierry Reding Nov. 20, 2018, 4:27 p.m. UTC | #1

On Tue, Nov 20, 2018 at 06:13:42PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> Make life easier for drivers by simply passing the connector
> to drm_hdmi_avi_infoframe_from_display_mode() and
> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> need to worry about is_hdmi2_sink mess.
> 
> Cc: Alex Deucher <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: "David (ChunMing) Zhou" <[e-mail]>
> Cc: Archit Taneja <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Inki Dae <[e-mail]>
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Seung-Woo Kim <[e-mail]>
> Cc: Kyungmin Park <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: CK Hu <[e-mail]>
> Cc: Philipp Zabel <[e-mail]>
> Cc: Rob Clark <[e-mail]>
> Cc: Ben Skeggs <[e-mail]>
> Cc: Tomi Valkeinen <[e-mail]>
> Cc: Sandy Huang <[e-mail]>
> Cc: "Heiko Stübner" <[e-mail]>
> Cc: Benjamin Gaignard <[e-mail]>
> Cc: Vincent Abriou <[e-mail]>
> Cc: Thierry Reding <[e-mail]>
> Cc: Eric Anholt <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Ilia Mirkin <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>  include/drm/drm_edid.h                    |  8 +++---
>  27 files changed, 94 insertions(+), 66 deletions(-)

That's actually a lot nicer:

Acked-by: Thierry Reding <[e-mail]>

Jani Nikula Nov. 21, 2018, 11:40 a.m. UTC | #2

On Tue, 20 Nov 2018, Ville Syrjala <[e-mail]> wrote:
> From: Ville Syrjälä <[e-mail]>
>
> Make life easier for drivers by simply passing the connector
> to drm_hdmi_avi_infoframe_from_display_mode() and
> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> need to worry about is_hdmi2_sink mess.

Overall looks about right and nice,

Reviewed-by: Jani Nikula <[e-mail]>

But please do take that with a grain of salt for everything outside of
i915 and drm core.

Please also find a few comments inline below.

> Cc: Alex Deucher <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: "David (ChunMing) Zhou" <[e-mail]>
> Cc: Archit Taneja <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Inki Dae <[e-mail]>
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Seung-Woo Kim <[e-mail]>
> Cc: Kyungmin Park <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: CK Hu <[e-mail]>
> Cc: Philipp Zabel <[e-mail]>
> Cc: Rob Clark <[e-mail]>
> Cc: Ben Skeggs <[e-mail]>
> Cc: Tomi Valkeinen <[e-mail]>
> Cc: Sandy Huang <[e-mail]>
> Cc: "Heiko Stübner" <[e-mail]>
> Cc: Benjamin Gaignard <[e-mail]>
> Cc: Vincent Abriou <[e-mail]>
> Cc: Thierry Reding <[e-mail]>
> Cc: Eric Anholt <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Ilia Mirkin <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>  include/drm/drm_edid.h                    |  8 +++---
>  27 files changed, 94 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 4cfecdce29a3..1f0426d2fc2a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder *encoder,
>  	dce_v10_0_audio_write_sad_regs(encoder);
>  	dce_v10_0_audio_write_latency_fields(encoder, mode);
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
>  	if (err < 0) {
>  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
>  		return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 7c868916d90f..2280b971d758 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder *encoder,
>  	dce_v11_0_audio_write_sad_regs(encoder);
>  	dce_v11_0_audio_write_latency_fields(encoder, mode);
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
>  	if (err < 0) {
>  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
>  		return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 17eaaba36017..db443ec53d3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -1423,6 +1423,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder,
>  	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
>  	struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
> +	struct drm_connector *connector = amdgpu_get_connector_for_encoder(encoder);
>  	struct hdmi_avi_infoframe frame;
>  	u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
>  	uint8_t *payload = buffer + 3;
> @@ -1430,7 +1431,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder,
>  	ssize_t err;
>  	u32 tmp;
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
>  	if (err < 0) {
>  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
>  		return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 8c0576978d36..13da915991dd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1616,7 +1616,7 @@ static void dce_v8_0_afmt_setmode(struct drm_encoder *encoder,
>  	dce_v8_0_audio_write_sad_regs(encoder);
>  	dce_v8_0_audio_write_latency_fields(encoder, mode);
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
>  	if (err < 0) {
>  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
>  		return;
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> index f8433c93f463..e11309e9bc4f 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> @@ -1094,8 +1094,9 @@ static void anx78xx_bridge_mode_set(struct drm_bridge *bridge,
>  
>  	mutex_lock(&anx78xx->lock);
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode,
> -						       false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						       &anx78xx->connector,
> +						       adjusted_mode);
>  	if (err) {
>  		DRM_ERROR("Failed to setup AVI infoframe: %d\n", err);
>  		goto unlock;
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index bfa902013aa4..a9b4f45ae87c 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -258,7 +258,8 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>  	if (ret)
>  		return;
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						       &sii902x->connector, adj);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>  		return;
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index a6e8f4591e63..0cc293a6ac24 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
>  	int ret;
>  
>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> -						       mode,
> -						       true);
> +						       NULL, mode);
>  	if (ctx->use_packed_pixel)
>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
>  
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 64c3cf027518..88b720b63126 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	u8 val;
>  
>  	/* Initialise info frame from DRM mode */
> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						 &hdmi->connector, mode);
>  
>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index b506e3622b08..501ac05ba7da 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_set_preferred_mode);
>  
> +static bool is_hdmi2_sink(struct drm_connector *connector)

You're usually known for adding const all around, why not const pointer
here and in all the other drm_* functions that call this?

> +{
> +	/*
> +	 * FIXME: sil-sii8620 doesn't have a connector around when
> +	 * we need one, so we have to be prepared for a NULL connector.
> +	 */
> +	if (!connector)
> +		return false;

This actually changes the is_hdmi2_sink value for sil-sii8620.

> +
> +	return connector->display_info.hdmi.scdc.supported ||
> +		connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB420;
> +}
> +
>  /**
>   * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
>   *                                              data from a DRM display mode
>   * @frame: HDMI AVI infoframe
> + * @connector: the connector
>   * @mode: DRM display mode
> - * @is_hdmi2_sink: Sink is HDMI 2.0 compliant
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> -					 const struct drm_display_mode *mode,
> -					 bool is_hdmi2_sink)
> +					 struct drm_connector *connector,
> +					 const struct drm_display_mode *mode)
>  {
>  	enum hdmi_picture_aspect picture_aspect;
>  	int err;
> @@ -4864,7 +4877,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  	 * HDMI 2.0 VIC range: 1 <= VIC <= 107 (CEA-861-F). So we
>  	 * have to make sure we dont break HDMI 1.4 sinks.
>  	 */
> -	if (!is_hdmi2_sink && frame->video_code > 64)
> +	if (!is_hdmi2_sink(connector) && frame->video_code > 64)
>  		frame->video_code = 0;
>  
>  	/*
> @@ -4923,21 +4936,17 @@ EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
>   *                                        quantization range information
>   * @frame: HDMI AVI infoframe
> + * @connector: the connector
>   * @mode: DRM display mode
>   * @rgb_quant_range: RGB quantization range (Q)
>   * @rgb_quant_range_selectable: Sink support selectable RGB quantization range (QS)
> - * @is_hdmi2_sink: HDMI 2.0 sink, which has different default recommendations
> - *
> - * Note that @is_hdmi2_sink can be derived by looking at the
> - * &drm_scdc.supported flag stored in &drm_hdmi_info.scdc,
> - * &drm_display_info.hdmi, which can be found in &drm_connector.display_info.
>   */
>  void
>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
> +				   struct drm_connector *connector,
>  				   const struct drm_display_mode *mode,
>  				   enum hdmi_quantization_range rgb_quant_range,
> -				   bool rgb_quant_range_selectable,
> -				   bool is_hdmi2_sink)
> +				   bool rgb_quant_range_selectable)
>  {
>  	/*
>  	 * CEA-861:
> @@ -4968,7 +4977,7 @@ drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>  	 * we limit non-zero YQ to HDMI 2.0 sinks only as HDMI 2.0 is based
>  	 * on on CEA-861-F.
>  	 */
> -	if (!is_hdmi2_sink ||
> +	if (!is_hdmi2_sink(connector) ||
>  	    rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED)
>  		frame->ycc_quantization_range =
>  			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 2092a650df7d..b857df67aff0 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -819,7 +819,8 @@ static void hdmi_reg_infoframes(struct hdmi_context *hdata)
>  		return;
>  	}
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi, m, false);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> +						       &hdata->connector, m);
>  	if (!ret)
>  		ret = hdmi_avi_infoframe_pack(&frm.avi, buf, sizeof(buf));
>  	if (ret > 0) {
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..38c66fbc8276 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -849,7 +849,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
>  {
>  	union hdmi_infoframe frame;
>  
> -	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
> +	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						 &priv->connector, mode);
>  	frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
>  
>  	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index e2c6a2b3e8f2..8a260bd6954c 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -465,15 +465,12 @@ static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->base.adjusted_mode;
> -	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
> -	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported ||
> -	   connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB420;
>  	union hdmi_infoframe frame;
>  	int ret;
>  
>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> -						       adjusted_mode,
> -						       is_hdmi2_sink);
> +						       conn_state->connector,
> +						       adjusted_mode);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>  		return;
> @@ -486,12 +483,13 @@ static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>  
> -	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
> +	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> +					   conn_state->connector,
> +					   adjusted_mode,
>  					   crtc_state->limited_color_range ?
>  					   HDMI_QUANTIZATION_RANGE_LIMITED :
>  					   HDMI_QUANTIZATION_RANGE_FULL,
> -					   intel_hdmi->rgb_quant_range_selectable,
> -					   is_hdmi2_sink);
> +					   intel_hdmi->rgb_quant_range_selectable);
>  
>  	drm_hdmi_avi_infoframe_content_type(&frame.avi,
>  					    conn_state);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 96a8d9524b0c..c18f14d41f1d 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -462,10 +462,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
>  	uint8_t buf[VIDEO_DIP_DATA_SIZE];
>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>  	struct intel_lspcon *lspcon = &dig_port->lspcon;
> -	struct intel_dp *intel_dp = &dig_port->dp;
> -	struct drm_connector *connector = &intel_dp->attached_connector->base;
> -	const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode;
> -	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
> +	const struct drm_display_mode *adjusted_mode =
> +		&crtc_state->base.adjusted_mode;
>  
>  	if (!lspcon->active) {
>  		DRM_ERROR("Writing infoframes while LSPCON disabled ?\n");
> @@ -473,7 +471,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
>  	}
>  
>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> -						       mode, is_hdmi2_sink);
> +						       conn_state->connector,
> +						       adjusted_mode);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>  		return;
> @@ -488,11 +487,13 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>  	}
>  
> -	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> +	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> +					   conn_state->connector,
> +					   adjusted_mode,
>  					   crtc_state->limited_color_range ?
>  					   HDMI_QUANTIZATION_RANGE_LIMITED :
>  					   HDMI_QUANTIZATION_RANGE_FULL,
> -					   false, is_hdmi2_sink);
> +					   false);
>  
>  	ret = hdmi_infoframe_pack(&frame, buf, sizeof(buf));
>  	if (ret < 0) {
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 5805ec1aba12..1277d31adb54 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -981,7 +981,8 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
>  }
>  
>  static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
> -					 const struct intel_crtc_state *pipe_config)
> +					 const struct intel_crtc_state *pipe_config,
> +					 const struct drm_connector_state *conn_state)
>  {
>  	uint8_t sdvo_data[HDMI_INFOFRAME_SIZE(AVI)];
>  	union hdmi_infoframe frame;
> @@ -989,8 +990,8 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
>  	ssize_t len;
>  
>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> -						       &pipe_config->base.adjusted_mode,
> -						       false);
> +						       conn_state->connector,
> +						       &pipe_config->base.adjusted_mode);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>  		return false;
> @@ -1316,7 +1317,8 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
>  		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_HDMI);
>  		intel_sdvo_set_colorimetry(intel_sdvo,
>  					   SDVO_COLORIMETRY_RGB256);
> -		intel_sdvo_set_avi_infoframe(intel_sdvo, crtc_state);
> +		intel_sdvo_set_avi_infoframe(intel_sdvo,
> +					     crtc_state, conn_state);
>  	} else
>  		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI);
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 862f3ec22131..f3ef7bf80563 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -981,7 +981,8 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi,
>  	u8 buffer[17];
>  	ssize_t err;
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						       &hdmi->conn, mode);
>  	if (err < 0) {
>  		dev_err(hdmi->dev,
>  			"Failed to get AVI infoframe from mode: %zd\n", err);
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 7e357077ed26..5ed4cab2819f 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -101,7 +101,8 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
>  	u32 val;
>  	int len;
>  
> -	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
> +	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						 hdmi->connector, mode);
>  
>  	len = hdmi_infoframe_pack(&frame, buffer, sizeof(buffer));
>  	if (len < 0) {
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 6cbbae3f438b..d8e512c83211 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -554,7 +554,7 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
>  	u32 max_ac_packet;
>  	union hdmi_infoframe avi_frame;
>  	union hdmi_infoframe vendor_frame;
> -	bool scdc_supported, high_tmds_clock_ratio = false, scrambling = false;
> +	bool high_tmds_clock_ratio = false, scrambling = false;
>  	u8 config;
>  	int ret;
>  	int size;
> @@ -564,10 +564,9 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
>  		return;
>  
>  	hdmi = &nv_connector->base.display_info.hdmi;
> -	scdc_supported = hdmi->scdc.supported;
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame.avi, mode,
> -						       scdc_supported);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame.avi,
> +						       &nv_connector->base, mode);
>  	if (!ret) {
>  		/* We have an AVI InfoFrame, populate it to the display */
>  		args.pwr.avi_infoframe_length
> diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
> index 452e625f6ce3..281b2164969c 100644
> --- a/drivers/gpu/drm/omapdrm/omap_encoder.c
> +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
> @@ -128,8 +128,9 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder,
>  	if (hdmi_mode && dssdev->ops->hdmi.set_infoframe) {
>  		struct hdmi_avi_infoframe avi;
>  
> -		r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode,
> -							     false);
> +		r = drm_hdmi_avi_infoframe_from_display_mode(&avi,
> +							     connector,
> +							     adjusted_mode);
>  		if (r == 0)
>  			dssdev->ops->hdmi.set_infoframe(dssdev, &avi);
>  	}
> diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c
> index 770e31f5fd1b..5a7d48339b32 100644
> --- a/drivers/gpu/drm/radeon/radeon_audio.c
> +++ b/drivers/gpu/drm/radeon/radeon_audio.c
> @@ -516,7 +516,7 @@ static int radeon_audio_set_avi_packet(struct drm_encoder *encoder,
>  	if (!connector)
>  		return -EINVAL;
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
>  	if (err < 0) {
>  		DRM_ERROR("failed to setup AVI infoframe: %d\n", err);
>  		return err;
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 1c02b3e61299..27c945e030a0 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -295,7 +295,9 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>  	union hdmi_infoframe frame;
>  	int rc;
>  
> -	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
> +	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						      &hdmi->connector,
> +						      mode);
>  
>  	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444)
>  		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index ccf718404a1c..4b86878f8ddf 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -434,7 +434,8 @@ static int hdmi_avi_infoframe_config(struct sti_hdmi *hdmi)
>  
>  	DRM_DEBUG_DRIVER("\n");
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode, false);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe,
> +						       hdmi->drm_connector, mode);
>  	if (ret < 0) {
>  		DRM_ERROR("failed to setup AVI infoframe: %d\n", ret);
>  		return ret;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index 061d2e0d9011..554a6f4561f3 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -52,7 +52,8 @@ static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi,
>  	u8 buffer[17];
>  	int i, ret;
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						       &hdmi->connector, mode);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to get infoframes from mode\n");
>  		return ret;
> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index 0082468f703c..a7566c67bfb0 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -741,7 +741,8 @@ static void tegra_hdmi_setup_avi_infoframe(struct tegra_hdmi *hdmi,
>  	u8 buffer[17];
>  	ssize_t err;
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						       &hdmi->output.connector, mode);
>  	if (err < 0) {
>  		dev_err(hdmi->dev, "failed to setup AVI infoframe: %zd\n", err);
>  		return;
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index b129da2e5afd..d90bf5f6a67a 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -2116,7 +2116,8 @@ tegra_sor_hdmi_setup_avi_infoframe(struct tegra_sor *sor,
>  	value &= ~INFOFRAME_CTRL_ENABLE;
>  	tegra_sor_writel(sor, value, SOR_HDMI_AVI_INFOFRAME_CTRL);
>  
> -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> +						       &sor->output.connector, mode);
>  	if (err < 0) {
>  		dev_err(sor->dev, "failed to setup AVI infoframe: %d\n", err);
>  		return err;
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index fd5522fd179e..cb3dca7426c8 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -409,22 +409,25 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
>  {
>  	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
>  	struct drm_crtc *crtc = encoder->crtc;
> +	struct vc4_dev *vc4 = to_vc4_dev(encoder->dev);
> +	struct vc4_hdmi *hdmi = vc4->hdmi;
>  	const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>  	union hdmi_infoframe frame;
>  	int ret;
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						       hdmi->connector, mode);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>  		return;
>  	}
>  
> -	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> +	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> +					   hdmi->connector, mode,
>  					   vc4_encoder->limited_rgb_range ?
>  					   HDMI_QUANTIZATION_RANGE_LIMITED :
>  					   HDMI_QUANTIZATION_RANGE_FULL,
> -					   vc4_encoder->rgb_range_selectable,
> -					   false);
> +					   vc4_encoder->rgb_range_selectable);
>  
>  	vc4_hdmi_write_infoframe(encoder, &frame);
>  }
> diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c
> index 78655269d843..9fc98bb4f3d9 100644
> --- a/drivers/gpu/drm/zte/zx_hdmi.c
> +++ b/drivers/gpu/drm/zte/zx_hdmi.c
> @@ -125,7 +125,9 @@ static int zx_hdmi_config_video_avi(struct zx_hdmi *hdmi,
>  	union hdmi_infoframe frame;
>  	int ret;
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						       &hdmi->connector,
> +						       mode);
>  	if (ret) {
>  		DRM_DEV_ERROR(hdmi->dev, "failed to get avi infoframe: %d\n",
>  			      ret);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index e3c404833115..9db6f130df65 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -352,18 +352,18 @@ drm_load_edid_firmware(struct drm_connector *connector)
>  
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> -					 const struct drm_display_mode *mode,
> -					 bool is_hdmi2_sink);
> +					 struct drm_connector *connector,
> +					 const struct drm_display_mode *mode);
>  int
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  					    struct drm_connector *connector,
>  					    const struct drm_display_mode *mode);
>  void
>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
> +				   struct drm_connector *connector,
>  				   const struct drm_display_mode *mode,
>  				   enum hdmi_quantization_range rgb_quant_range,
> -				   bool rgb_quant_range_selectable,
> -				   bool is_hdmi2_sink);
> +				   bool rgb_quant_range_selectable);
>  
>  /**
>   * drm_eld_mnl - Get ELD monitor name length in bytes.

Ville Syrjälä Nov. 21, 2018, 11:51 a.m. UTC | #3

On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> On Tue, 20 Nov 2018, Ville Syrjala <[e-mail]> wrote:
> > From: Ville Syrjälä <[e-mail]>
> >
> > Make life easier for drivers by simply passing the connector
> > to drm_hdmi_avi_infoframe_from_display_mode() and
> > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> > need to worry about is_hdmi2_sink mess.
> 
> Overall looks about right and nice,
> 
> Reviewed-by: Jani Nikula <[e-mail]>
> 
> But please do take that with a grain of salt for everything outside of
> i915 and drm core.
> 
> Please also find a few comments inline below.
> 
> > Cc: Alex Deucher <[e-mail]>
> > Cc: "Christian König" <[e-mail]>
> > Cc: "David (ChunMing) Zhou" <[e-mail]>
> > Cc: Archit Taneja <[e-mail]>
> > Cc: Andrzej Hajda <[e-mail]>
> > Cc: Laurent Pinchart <[e-mail]>
> > Cc: Inki Dae <[e-mail]>
> > Cc: Joonyoung Shim <[e-mail]>
> > Cc: Seung-Woo Kim <[e-mail]>
> > Cc: Kyungmin Park <[e-mail]>
> > Cc: Russell King <[e-mail]>
> > Cc: CK Hu <[e-mail]>
> > Cc: Philipp Zabel <[e-mail]>
> > Cc: Rob Clark <[e-mail]>
> > Cc: Ben Skeggs <[e-mail]>
> > Cc: Tomi Valkeinen <[e-mail]>
> > Cc: Sandy Huang <[e-mail]>
> > Cc: "Heiko Stübner" <[e-mail]>
> > Cc: Benjamin Gaignard <[e-mail]>
> > Cc: Vincent Abriou <[e-mail]>
> > Cc: Thierry Reding <[e-mail]>
> > Cc: Eric Anholt <[e-mail]>
> > Cc: Shawn Guo <[e-mail]>
> > Cc: Ilia Mirkin <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
> >  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
> >  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
> >  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
> >  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
> >  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
> >  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
> >  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
> >  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
> >  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
> >  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
> >  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
> >  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
> >  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
> >  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
> >  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
> >  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
> >  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
> >  drivers/gpu/drm/tegra/sor.c               |  3 ++-
> >  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
> >  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
> >  include/drm/drm_edid.h                    |  8 +++---
> >  27 files changed, 94 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > index 4cfecdce29a3..1f0426d2fc2a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder *encoder,
> >  	dce_v10_0_audio_write_sad_regs(encoder);
> >  	dce_v10_0_audio_write_latency_fields(encoder, mode);
> >  
> > -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> > +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
> >  	if (err < 0) {
> >  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> >  		return;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > index 7c868916d90f..2280b971d758 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder *encoder,
> >  	dce_v11_0_audio_write_sad_regs(encoder);
> >  	dce_v11_0_audio_write_latency_fields(encoder, mode);
> >  
> > -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> > +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
> >  	if (err < 0) {
> >  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> >  		return;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > index 17eaaba36017..db443ec53d3a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > @@ -1423,6 +1423,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder,
> >  	struct amdgpu_device *adev = dev->dev_private;
> >  	struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
> >  	struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
> > +	struct drm_connector *connector = amdgpu_get_connector_for_encoder(encoder);
> >  	struct hdmi_avi_infoframe frame;
> >  	u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
> >  	uint8_t *payload = buffer + 3;
> > @@ -1430,7 +1431,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder,
> >  	ssize_t err;
> >  	u32 tmp;
> >  
> > -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> > +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
> >  	if (err < 0) {
> >  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> >  		return;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > index 8c0576978d36..13da915991dd 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > @@ -1616,7 +1616,7 @@ static void dce_v8_0_afmt_setmode(struct drm_encoder *encoder,
> >  	dce_v8_0_audio_write_sad_regs(encoder);
> >  	dce_v8_0_audio_write_latency_fields(encoder, mode);
> >  
> > -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> > +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
> >  	if (err < 0) {
> >  		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> >  		return;
> > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> > index f8433c93f463..e11309e9bc4f 100644
> > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> > @@ -1094,8 +1094,9 @@ static void anx78xx_bridge_mode_set(struct drm_bridge *bridge,
> >  
> >  	mutex_lock(&anx78xx->lock);
> >  
> > -	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode,
> > -						       false);
> > +	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> > +						       &anx78xx->connector,
> > +						       adjusted_mode);
> >  	if (err) {
> >  		DRM_ERROR("Failed to setup AVI infoframe: %d\n", err);
> >  		goto unlock;
> > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> > index bfa902013aa4..a9b4f45ae87c 100644
> > --- a/drivers/gpu/drm/bridge/sii902x.c
> > +++ b/drivers/gpu/drm/bridge/sii902x.c
> > @@ -258,7 +258,8 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> >  	if (ret)
> >  		return;
> >  
> > -	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
> > +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> > +						       &sii902x->connector, adj);
> >  	if (ret < 0) {
> >  		DRM_ERROR("couldn't fill AVI infoframe\n");
> >  		return;
> > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> > index a6e8f4591e63..0cc293a6ac24 100644
> > --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> > @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
> >  	int ret;
> >  
> >  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> > -						       mode,
> > -						       true);
> > +						       NULL, mode);
> >  	if (ctx->use_packed_pixel)
> >  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >  
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index 64c3cf027518..88b720b63126 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> >  	u8 val;
> >  
> >  	/* Initialise info frame from DRM mode */
> > -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> > +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> > +						 &hdmi->connector, mode);
> >  
> >  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index b506e3622b08..501ac05ba7da 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_set_preferred_mode);
> >  
> > +static bool is_hdmi2_sink(struct drm_connector *connector)
> 
> You're usually known for adding const all around, why not const pointer
> here and in all the other drm_* functions that call this?

My current approach is to constify states/fbs/etc. but not so much
crtcs/connectors/etc. Too much const can sometimes get in the way
of things requiring that you remove the const later. But I guess
in this case the const shouldn't really get in the way of anything
because these are pretty much supposed to be pure functions.

> 
> > +{
> > +	/*
> > +	 * FIXME: sil-sii8620 doesn't have a connector around when
> > +	 * we need one, so we have to be prepared for a NULL connector.
> > +	 */
> > +	if (!connector)
> > +		return false;
> 
> This actually changes the is_hdmi2_sink value for sil-sii8620.

Hmm. No idea why they would have set that to true when everyone else is
passing false. I guess I can change this to true to not change it. IIRC
that was the only driver that didn't have a connector around.

That said, I was actually thinking of removing this hdmi2 vs. not
stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
"just in case", but we already have a similar issue with earlier
cea-861 revisions and haven't seen any bugs where an older monitor
would get confused by a VIC not yet defined when the monitor was
produced.

Andrzej Hajda Nov. 29, 2018, 8:46 a.m. UTC | #4

Quite late, hopefully not too late.


On 21.11.2018 12:51, Ville Syrjälä wrote:
> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
>>
>>>  		return;
>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> index a6e8f4591e63..0cc293a6ac24 100644
>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
>>>  	int ret;
>>>  
>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
>>> -						       mode,
>>> -						       true);
>>> +						       NULL, mode);
>>>  	if (ctx->use_packed_pixel)
>>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
>>>  
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index 64c3cf027518..88b720b63126 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>>>  	u8 val;
>>>  
>>>  	/* Initialise info frame from DRM mode */
>>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
>>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
>>> +						 &hdmi->connector, mode);
>>>  
>>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index b506e3622b08..501ac05ba7da 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
>>>  }
>>>  EXPORT_SYMBOL(drm_set_preferred_mode);
>>>  
>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
>> You're usually known for adding const all around, why not const pointer
>> here and in all the other drm_* functions that call this?
> My current approach is to constify states/fbs/etc. but not so much
> crtcs/connectors/etc. Too much const can sometimes get in the way
> of things requiring that you remove the const later. But I guess
> in this case the const shouldn't really get in the way of anything
> because these are pretty much supposed to be pure functions.
>
>>> +{
>>> +	/*
>>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
>>> +	 * we need one, so we have to be prepared for a NULL connector.
>>> +	 */
>>> +	if (!connector)
>>> +		return false;
>> This actually changes the is_hdmi2_sink value for sil-sii8620.
> Hmm. No idea why they would have set that to true when everyone else is
> passing false. 


Because false does not work :) More precisely MHLv3 (used in Sii8620)
uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.

Unfortunately I have no access to MHL specs, but my experiments and
vendor drivers strongly suggests it is done this way.

This is important in case of 4K modes which are handled differently by
HDMI 1.4 and HDMI2.0.

The pipeline looks like (in parenthesis HDMI version on the stream):

exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV


> I guess I can change this to true to not change it. IIRC
> that was the only driver that didn't have a connector around.
>
> That said, I was actually thinking of removing this hdmi2 vs. not
> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> "just in case", but we already have a similar issue with earlier
> cea-861 revisions and haven't seen any bugs where an older monitor
> would get confused by a VIC not yet defined when the monitor was
> produced.
>
Are you sure this is a good idea? As I said earlier 4K modes are present
in HDMI 1.4 and 2.0 but they are handled differently, so this is not
only matter of unknown VIC in AVIF.


Regards

Andrzej

Andrzej Hajda Nov. 29, 2018, 9:08 a.m. UTC | #5

On 21.11.2018 19:19, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
>> From: Ville Syrjälä <[e-mail]>
>>
>> Make life easier for drivers by simply passing the connector
>> to drm_hdmi_avi_infoframe_from_display_mode() and
>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
>> need to worry about is_hdmi2_sink mess.
> While this is good for display controller drivers, the change isn't great for 
> bridge drivers. Down the road we're looking at moving connector support out of 
> the bridge drivers. Adding an additional dependency to connectors in the 
> bridges will make that more difficult. Ideally bridges should retrieve the 
> information from their sink, regardless of whether it is a connector or 
> another bridge.


I agree with it, and case of sii8620 shows that there are cases where
bridge has no direct access to the connector.

On the other side,  since you are passing connector to
drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
parameter and rename the function to
drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
mode set on the connector differs?


Regards

Andrzej


>
> Please see below for an additional comment.
>
>> Cc: Alex Deucher <[e-mail]>
>> Cc: "Christian König" <[e-mail]>
>> Cc: "David (ChunMing) Zhou" <[e-mail]>
>> Cc: Archit Taneja <[e-mail]>
>> Cc: Andrzej Hajda <[e-mail]>
>> Cc: Laurent Pinchart <[e-mail]>
>> Cc: Inki Dae <[e-mail]>
>> Cc: Joonyoung Shim <[e-mail]>
>> Cc: Seung-Woo Kim <[e-mail]>
>> Cc: Kyungmin Park <[e-mail]>
>> Cc: Russell King <[e-mail]>
>> Cc: CK Hu <[e-mail]>
>> Cc: Philipp Zabel <[e-mail]>
>> Cc: Rob Clark <[e-mail]>
>> Cc: Ben Skeggs <[e-mail]>
>> Cc: Tomi Valkeinen <[e-mail]>
>> Cc: Sandy Huang <[e-mail]>
>> Cc: "Heiko Stübner" <[e-mail]>
>> Cc: Benjamin Gaignard <[e-mail]>
>> Cc: Vincent Abriou <[e-mail]>
>> Cc: Thierry Reding <[e-mail]>
>> Cc: Eric Anholt <[e-mail]>
>> Cc: Shawn Guo <[e-mail]>
>> Cc: Ilia Mirkin <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Signed-off-by: Ville Syrjälä <[e-mail]>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>>  include/drm/drm_edid.h                    |  8 +++---
>>  27 files changed, 94 insertions(+), 66 deletions(-)
> For dw-hdmi and omapdrm,
>
> Reviewed-by: Laurent Pinchart <[e-mail]>
>

Ville Syrjälä Dec. 3, 2018, 9:38 p.m. UTC | #6

On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote:
> On 21.11.2018 19:19, Laurent Pinchart wrote:
> > Hi Ville,
> >
> > Thank you for the patch.
> >
> > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
> >> From: Ville Syrjälä <[e-mail]>
> >>
> >> Make life easier for drivers by simply passing the connector
> >> to drm_hdmi_avi_infoframe_from_display_mode() and
> >> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> >> need to worry about is_hdmi2_sink mess.
> > While this is good for display controller drivers, the change isn't great for 
> > bridge drivers. Down the road we're looking at moving connector support out of 
> > the bridge drivers. Adding an additional dependency to connectors in the 
> > bridges will make that more difficult. Ideally bridges should retrieve the 
> > information from their sink, regardless of whether it is a connector or 
> > another bridge.
> 
> 
> I agree with it, and case of sii8620 shows that there are cases where
> bridge has no direct access to the connector.

It's just a matter of plumbing it through.

> 
> On the other side,  since you are passing connector to
> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
> parameter and rename the function to
> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
> mode set on the connector differs?

Connectors don't have a mode.

> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> > Please see below for an additional comment.
> >
> >> Cc: Alex Deucher <[e-mail]>
> >> Cc: "Christian König" <[e-mail]>
> >> Cc: "David (ChunMing) Zhou" <[e-mail]>
> >> Cc: Archit Taneja <[e-mail]>
> >> Cc: Andrzej Hajda <[e-mail]>
> >> Cc: Laurent Pinchart <[e-mail]>
> >> Cc: Inki Dae <[e-mail]>
> >> Cc: Joonyoung Shim <[e-mail]>
> >> Cc: Seung-Woo Kim <[e-mail]>
> >> Cc: Kyungmin Park <[e-mail]>
> >> Cc: Russell King <[e-mail]>
> >> Cc: CK Hu <[e-mail]>
> >> Cc: Philipp Zabel <[e-mail]>
> >> Cc: Rob Clark <[e-mail]>
> >> Cc: Ben Skeggs <[e-mail]>
> >> Cc: Tomi Valkeinen <[e-mail]>
> >> Cc: Sandy Huang <[e-mail]>
> >> Cc: "Heiko Stübner" <[e-mail]>
> >> Cc: Benjamin Gaignard <[e-mail]>
> >> Cc: Vincent Abriou <[e-mail]>
> >> Cc: Thierry Reding <[e-mail]>
> >> Cc: Eric Anholt <[e-mail]>
> >> Cc: Shawn Guo <[e-mail]>
> >> Cc: Ilia Mirkin <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Signed-off-by: Ville Syrjälä <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
> >>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
> >>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
> >>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
> >>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
> >>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
> >>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
> >>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
> >>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
> >>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
> >>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
> >>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
> >>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
> >>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
> >>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
> >>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
> >>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
> >>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
> >>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
> >>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
> >>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
> >>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
> >>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
> >>  include/drm/drm_edid.h                    |  8 +++---
> >>  27 files changed, 94 insertions(+), 66 deletions(-)
> > For dw-hdmi and omapdrm,
> >
> > Reviewed-by: Laurent Pinchart <[e-mail]>
> >

Ville Syrjälä Dec. 3, 2018, 9:48 p.m. UTC | #7

On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> Quite late, hopefully not too late.
> 
> 
> On 21.11.2018 12:51, Ville Syrjälä wrote:
> > On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>
> >>>  		return;
> >>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> index a6e8f4591e63..0cc293a6ac24 100644
> >>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
> >>>  	int ret;
> >>>  
> >>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> >>> -						       mode,
> >>> -						       true);
> >>> +						       NULL, mode);
> >>>  	if (ctx->use_packed_pixel)
> >>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>  
> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> index 64c3cf027518..88b720b63126 100644
> >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> >>>  	u8 val;
> >>>  
> >>>  	/* Initialise info frame from DRM mode */
> >>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> >>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> >>> +						 &hdmi->connector, mode);
> >>>  
> >>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>> index b506e3622b08..501ac05ba7da 100644
> >>> --- a/drivers/gpu/drm/drm_edid.c
> >>> +++ b/drivers/gpu/drm/drm_edid.c
> >>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
> >>>  }
> >>>  EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>  
> >>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >> You're usually known for adding const all around, why not const pointer
> >> here and in all the other drm_* functions that call this?
> > My current approach is to constify states/fbs/etc. but not so much
> > crtcs/connectors/etc. Too much const can sometimes get in the way
> > of things requiring that you remove the const later. But I guess
> > in this case the const shouldn't really get in the way of anything
> > because these are pretty much supposed to be pure functions.
> >
> >>> +{
> >>> +	/*
> >>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
> >>> +	 * we need one, so we have to be prepared for a NULL connector.
> >>> +	 */
> >>> +	if (!connector)
> >>> +		return false;
> >> This actually changes the is_hdmi2_sink value for sil-sii8620.
> > Hmm. No idea why they would have set that to true when everyone else is
> > passing false. 
> 
> 
> Because false does not work :) More precisely MHLv3 (used in Sii8620)
> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
> 
> Unfortunately I have no access to MHL specs, but my experiments and
> vendor drivers strongly suggests it is done this way.
> 
> This is important in case of 4K modes which are handled differently by
> HDMI 1.4 and HDMI2.0.

HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
switch over to the HDMI 2.0 specific signalling.

> 
> The pipeline looks like (in parenthesis HDMI version on the stream):
> 
> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
> 
> 
> > I guess I can change this to true to not change it. IIRC
> > that was the only driver that didn't have a connector around.
> >
> > That said, I was actually thinking of removing this hdmi2 vs. not
> > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> > "just in case", but we already have a similar issue with earlier
> > cea-861 revisions and haven't seen any bugs where an older monitor
> > would get confused by a VIC not yet defined when the monitor was
> > produced.
> >
> Are you sure this is a good idea? As I said earlier 4K modes are present
> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
> only matter of unknown VIC in AVIF.
> 
> 
> Regards
> 
> Andrzej

Andrzej Hajda Dec. 4, 2018, 7:03 a.m. UTC | #8

On 03.12.2018 22:48, Ville Syrjälä wrote:
> On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
>> Quite late, hopefully not too late.
>>
>>
>> On 21.11.2018 12:51, Ville Syrjälä wrote:
>>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
>>>>>  		return;
>>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>>>>> index a6e8f4591e63..0cc293a6ac24 100644
>>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
>>>>>  	int ret;
>>>>>  
>>>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
>>>>> -						       mode,
>>>>> -						       true);
>>>>> +						       NULL, mode);
>>>>>  	if (ctx->use_packed_pixel)
>>>>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
>>>>>  
>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> index 64c3cf027518..88b720b63126 100644
>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>>>>>  	u8 val;
>>>>>  
>>>>>  	/* Initialise info frame from DRM mode */
>>>>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
>>>>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
>>>>> +						 &hdmi->connector, mode);
>>>>>  
>>>>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>>>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>>> index b506e3622b08..501ac05ba7da 100644
>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
>>>>>  }
>>>>>  EXPORT_SYMBOL(drm_set_preferred_mode);
>>>>>  
>>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
>>>> You're usually known for adding const all around, why not const pointer
>>>> here and in all the other drm_* functions that call this?
>>> My current approach is to constify states/fbs/etc. but not so much
>>> crtcs/connectors/etc. Too much const can sometimes get in the way
>>> of things requiring that you remove the const later. But I guess
>>> in this case the const shouldn't really get in the way of anything
>>> because these are pretty much supposed to be pure functions.
>>>
>>>>> +{
>>>>> +	/*
>>>>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
>>>>> +	 * we need one, so we have to be prepared for a NULL connector.
>>>>> +	 */
>>>>> +	if (!connector)
>>>>> +		return false;
>>>> This actually changes the is_hdmi2_sink value for sil-sii8620.
>>> Hmm. No idea why they would have set that to true when everyone else is
>>> passing false. 
>>
>> Because false does not work :) More precisely MHLv3 (used in Sii8620)
>> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
>>
>> Unfortunately I have no access to MHL specs, but my experiments and
>> vendor drivers strongly suggests it is done this way.
>>
>> This is important in case of 4K modes which are handled differently by
>> HDMI 1.4 and HDMI2.0.
> HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
> the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
> switch over to the HDMI 2.0 specific signalling.


The difference is in infoframes:

HDMI 1.4 sets AVI infoframe VIC to 0, and sends HDMI_VIC in VSI.

HDMI 2.0 sets AVI infoframe to non zero VICs introduced by
HDMI2.0/CEA-861-F, VSI can be omitted if I remember correctly, unless 3d
is in use.


So setting VICs to non-zero in case of HDMI1.4 sinks and 4k modes seems
risky.


Regards

Andrzej


>
>> The pipeline looks like (in parenthesis HDMI version on the stream):
>>
>> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
>>
>>
>>> I guess I can change this to true to not change it. IIRC
>>> that was the only driver that didn't have a connector around.
>>>
>>> That said, I was actually thinking of removing this hdmi2 vs. not
>>> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
>>> "just in case", but we already have a similar issue with earlier
>>> cea-861 revisions and haven't seen any bugs where an older monitor
>>> would get confused by a VIC not yet defined when the monitor was
>>> produced.
>>>
>> Are you sure this is a good idea? As I said earlier 4K modes are present
>> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
>> only matter of unknown VIC in AVIF.
>>
>>
>> Regards
>>
>> Andrzej

Andrzej Hajda Dec. 4, 2018, 7:46 a.m. UTC | #9

On 03.12.2018 22:38, Ville Syrjälä wrote:
> On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote:
>> On 21.11.2018 19:19, Laurent Pinchart wrote:
>>> Hi Ville,
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
>>>> From: Ville Syrjälä <[e-mail]>
>>>>
>>>> Make life easier for drivers by simply passing the connector
>>>> to drm_hdmi_avi_infoframe_from_display_mode() and
>>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
>>>> need to worry about is_hdmi2_sink mess.
>>> While this is good for display controller drivers, the change isn't great for 
>>> bridge drivers. Down the road we're looking at moving connector support out of 
>>> the bridge drivers. Adding an additional dependency to connectors in the 
>>> bridges will make that more difficult. Ideally bridges should retrieve the 
>>> information from their sink, regardless of whether it is a connector or 
>>> another bridge.
>>
>> I agree with it, and case of sii8620 shows that there are cases where
>> bridge has no direct access to the connector.
> It's just a matter of plumbing it through.


What do you mean exactly?


>
>> On the other side,  since you are passing connector to
>> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
>> parameter and rename the function to
>> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
>> mode set on the connector differs?
> Connectors don't have a mode.


As they are passing video stream they should have it, even if not
directly, for example:

connector->state->crtc->mode

In moment of creating infoframe it should be set properly.


Regards

Andrzej


>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> Please see below for an additional comment.
>>>
>>>> Cc: Alex Deucher <[e-mail]>
>>>> Cc: "Christian König" <[e-mail]>
>>>> Cc: "David (ChunMing) Zhou" <[e-mail]>
>>>> Cc: Archit Taneja <[e-mail]>
>>>> Cc: Andrzej Hajda <[e-mail]>
>>>> Cc: Laurent Pinchart <[e-mail]>
>>>> Cc: Inki Dae <[e-mail]>
>>>> Cc: Joonyoung Shim <[e-mail]>
>>>> Cc: Seung-Woo Kim <[e-mail]>
>>>> Cc: Kyungmin Park <[e-mail]>
>>>> Cc: Russell King <[e-mail]>
>>>> Cc: CK Hu <[e-mail]>
>>>> Cc: Philipp Zabel <[e-mail]>
>>>> Cc: Rob Clark <[e-mail]>
>>>> Cc: Ben Skeggs <[e-mail]>
>>>> Cc: Tomi Valkeinen <[e-mail]>
>>>> Cc: Sandy Huang <[e-mail]>
>>>> Cc: "Heiko Stübner" <[e-mail]>
>>>> Cc: Benjamin Gaignard <[e-mail]>
>>>> Cc: Vincent Abriou <[e-mail]>
>>>> Cc: Thierry Reding <[e-mail]>
>>>> Cc: Eric Anholt <[e-mail]>
>>>> Cc: Shawn Guo <[e-mail]>
>>>> Cc: Ilia Mirkin <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Signed-off-by: Ville Syrjälä <[e-mail]>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>>>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>>>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>>>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>>>>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>>>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>>>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>>>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>>>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>>>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>>>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>>>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>>>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>>>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>>>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>>>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>>>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>>>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>>>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>>>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>>>>  include/drm/drm_edid.h                    |  8 +++---
>>>>  27 files changed, 94 insertions(+), 66 deletions(-)
>>> For dw-hdmi and omapdrm,
>>>
>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>>>

Ville Syrjälä Dec. 4, 2018, 7:02 p.m. UTC | #10

On Tue, Dec 04, 2018 at 08:03:53AM +0100, Andrzej Hajda wrote:
> On 03.12.2018 22:48, Ville Syrjälä wrote:
> > On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> >> Quite late, hopefully not too late.
> >>
> >>
> >> On 21.11.2018 12:51, Ville Syrjälä wrote:
> >>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>>>>  		return;
> >>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> index a6e8f4591e63..0cc293a6ac24 100644
> >>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
> >>>>>  	int ret;
> >>>>>  
> >>>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> >>>>> -						       mode,
> >>>>> -						       true);
> >>>>> +						       NULL, mode);
> >>>>>  	if (ctx->use_packed_pixel)
> >>>>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>>>  
> >>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> index 64c3cf027518..88b720b63126 100644
> >>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> >>>>>  	u8 val;
> >>>>>  
> >>>>>  	/* Initialise info frame from DRM mode */
> >>>>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> >>>>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> >>>>> +						 &hdmi->connector, mode);
> >>>>>  
> >>>>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>>> index b506e3622b08..501ac05ba7da 100644
> >>>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
> >>>>>  }
> >>>>>  EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>>>  
> >>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >>>> You're usually known for adding const all around, why not const pointer
> >>>> here and in all the other drm_* functions that call this?
> >>> My current approach is to constify states/fbs/etc. but not so much
> >>> crtcs/connectors/etc. Too much const can sometimes get in the way
> >>> of things requiring that you remove the const later. But I guess
> >>> in this case the const shouldn't really get in the way of anything
> >>> because these are pretty much supposed to be pure functions.
> >>>
> >>>>> +{
> >>>>> +	/*
> >>>>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
> >>>>> +	 * we need one, so we have to be prepared for a NULL connector.
> >>>>> +	 */
> >>>>> +	if (!connector)
> >>>>> +		return false;
> >>>> This actually changes the is_hdmi2_sink value for sil-sii8620.
> >>> Hmm. No idea why they would have set that to true when everyone else is
> >>> passing false. 
> >>
> >> Because false does not work :) More precisely MHLv3 (used in Sii8620)
> >> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
> >>
> >> Unfortunately I have no access to MHL specs, but my experiments and
> >> vendor drivers strongly suggests it is done this way.
> >>
> >> This is important in case of 4K modes which are handled differently by
> >> HDMI 1.4 and HDMI2.0.
> > HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
> > the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
> > switch over to the HDMI 2.0 specific signalling.
> 
> 
> The difference is in infoframes:
> 
> HDMI 1.4 sets AVI infoframe VIC to 0, and sends HDMI_VIC in VSI.
> 
> HDMI 2.0 sets AVI infoframe to non zero VICs introduced by
> HDMI2.0/CEA-861-F, VSI can be omitted if I remember correctly, unless 3d
> is in use.

Like I said, The HDMI 1.4 method is used even with HDMI 2.0 sinks unless
some feature gets used which can't be signalled via the HDMI 1.4 vendor
specific infoframe.

> 
> 
> So setting VICs to non-zero in case of HDMI1.4 sinks and 4k modes seems
> risky.

That is not what I was proposing.

> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> >> The pipeline looks like (in parenthesis HDMI version on the stream):
> >>
> >> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
> >>
> >>
> >>> I guess I can change this to true to not change it. IIRC
> >>> that was the only driver that didn't have a connector around.
> >>>
> >>> That said, I was actually thinking of removing this hdmi2 vs. not
> >>> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> >>> "just in case", but we already have a similar issue with earlier
> >>> cea-861 revisions and haven't seen any bugs where an older monitor
> >>> would get confused by a VIC not yet defined when the monitor was
> >>> produced.
> >>>
> >> Are you sure this is a good idea? As I said earlier 4K modes are present
> >> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
> >> only matter of unknown VIC in AVIF.
> >>
> >>
> >> Regards
> >>
> >> Andrzej
>

Ville Syrjälä Dec. 4, 2018, 7:13 p.m. UTC | #11

On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote:
> On 03.12.2018 22:38, Ville Syrjälä wrote:
> > On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote:
> >> On 21.11.2018 19:19, Laurent Pinchart wrote:
> >>> Hi Ville,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
> >>>> From: Ville Syrjälä <[e-mail]>
> >>>>
> >>>> Make life easier for drivers by simply passing the connector
> >>>> to drm_hdmi_avi_infoframe_from_display_mode() and
> >>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> >>>> need to worry about is_hdmi2_sink mess.
> >>> While this is good for display controller drivers, the change isn't great for 
> >>> bridge drivers. Down the road we're looking at moving connector support out of 
> >>> the bridge drivers. Adding an additional dependency to connectors in the 
> >>> bridges will make that more difficult. Ideally bridges should retrieve the 
> >>> information from their sink, regardless of whether it is a connector or 
> >>> another bridge.
> >>
> >> I agree with it, and case of sii8620 shows that there are cases where
> >> bridge has no direct access to the connector.
> > It's just a matter of plumbing it through.
> 
> 
> What do you mean exactly?

void bridge_foo(...
+               ,struct drm_connector *connector);

> 
> 
> >
> >> On the other side,  since you are passing connector to
> >> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
> >> parameter and rename the function to
> >> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
> >> mode set on the connector differs?
> > Connectors don't have a mode.
> 
> 
> As they are passing video stream they should have it, even if not
> directly, for example:
> 
> connector->state->crtc->mode

That's not really how atomic works. One shouldn't go digging
through the obj->state pointers when we're not holding the
relevant locks anymore. The atomic way would be to pass either
both crtc state and connector state, or drm_atomic_state +
crtc/connector.

> 
> In moment of creating infoframe it should be set properly.
> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> >>
> >> Regards
> >>
> >> Andrzej
> >>
> >>
> >>> Please see below for an additional comment.
> >>>
> >>>> Cc: Alex Deucher <[e-mail]>
> >>>> Cc: "Christian König" <[e-mail]>
> >>>> Cc: "David (ChunMing) Zhou" <[e-mail]>
> >>>> Cc: Archit Taneja <[e-mail]>
> >>>> Cc: Andrzej Hajda <[e-mail]>
> >>>> Cc: Laurent Pinchart <[e-mail]>
> >>>> Cc: Inki Dae <[e-mail]>
> >>>> Cc: Joonyoung Shim <[e-mail]>
> >>>> Cc: Seung-Woo Kim <[e-mail]>
> >>>> Cc: Kyungmin Park <[e-mail]>
> >>>> Cc: Russell King <[e-mail]>
> >>>> Cc: CK Hu <[e-mail]>
> >>>> Cc: Philipp Zabel <[e-mail]>
> >>>> Cc: Rob Clark <[e-mail]>
> >>>> Cc: Ben Skeggs <[e-mail]>
> >>>> Cc: Tomi Valkeinen <[e-mail]>
> >>>> Cc: Sandy Huang <[e-mail]>
> >>>> Cc: "Heiko Stübner" <[e-mail]>
> >>>> Cc: Benjamin Gaignard <[e-mail]>
> >>>> Cc: Vincent Abriou <[e-mail]>
> >>>> Cc: Thierry Reding <[e-mail]>
> >>>> Cc: Eric Anholt <[e-mail]>
> >>>> Cc: Shawn Guo <[e-mail]>
> >>>> Cc: Ilia Mirkin <[e-mail]>
> >>>> Cc: [e-mail]
> >>>> Cc: [e-mail]
> >>>> Cc: [e-mail]
> >>>> Cc: [e-mail]
> >>>> Cc: [e-mail]
> >>>> Signed-off-by: Ville Syrjälä <[e-mail]>
> >>>> ---
> >>>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
> >>>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
> >>>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
> >>>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
> >>>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
> >>>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
> >>>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
> >>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
> >>>>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++++---------
> >>>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
> >>>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
> >>>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
> >>>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
> >>>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
> >>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
> >>>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
> >>>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
> >>>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
> >>>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
> >>>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
> >>>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
> >>>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
> >>>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
> >>>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
> >>>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
> >>>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
> >>>>  include/drm/drm_edid.h                    |  8 +++---
> >>>>  27 files changed, 94 insertions(+), 66 deletions(-)
> >>> For dw-hdmi and omapdrm,
> >>>
> >>> Reviewed-by: Laurent Pinchart <[e-mail]>
> >>>

Andrzej Hajda Dec. 5, 2018, 7:40 a.m. UTC | #12

On 04.12.2018 20:02, Ville Syrjälä wrote:
> On Tue, Dec 04, 2018 at 08:03:53AM +0100, Andrzej Hajda wrote:
>> On 03.12.2018 22:48, Ville Syrjälä wrote:
>>> On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
>>>> Quite late, hopefully not too late.
>>>>
>>>>
>>>> On 21.11.2018 12:51, Ville Syrjälä wrote:
>>>>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
>>>>>>>  		return;
>>>>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>>>>>>> index a6e8f4591e63..0cc293a6ac24 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>>>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
>>>>>>>  	int ret;
>>>>>>>  
>>>>>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
>>>>>>> -						       mode,
>>>>>>> -						       true);
>>>>>>> +						       NULL, mode);
>>>>>>>  	if (ctx->use_packed_pixel)
>>>>>>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
>>>>>>>  
>>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> index 64c3cf027518..88b720b63126 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>>>>>>>  	u8 val;
>>>>>>>  
>>>>>>>  	/* Initialise info frame from DRM mode */
>>>>>>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
>>>>>>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
>>>>>>> +						 &hdmi->connector, mode);
>>>>>>>  
>>>>>>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>>>>>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>>>>> index b506e3622b08..501ac05ba7da 100644
>>>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
>>>>>>>  }
>>>>>>>  EXPORT_SYMBOL(drm_set_preferred_mode);
>>>>>>>  
>>>>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
>>>>>> You're usually known for adding const all around, why not const pointer
>>>>>> here and in all the other drm_* functions that call this?
>>>>> My current approach is to constify states/fbs/etc. but not so much
>>>>> crtcs/connectors/etc. Too much const can sometimes get in the way
>>>>> of things requiring that you remove the const later. But I guess
>>>>> in this case the const shouldn't really get in the way of anything
>>>>> because these are pretty much supposed to be pure functions.
>>>>>
>>>>>>> +{
>>>>>>> +	/*
>>>>>>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
>>>>>>> +	 * we need one, so we have to be prepared for a NULL connector.
>>>>>>> +	 */
>>>>>>> +	if (!connector)
>>>>>>> +		return false;
>>>>>> This actually changes the is_hdmi2_sink value for sil-sii8620.
>>>>> Hmm. No idea why they would have set that to true when everyone else is
>>>>> passing false. 
>>>> Because false does not work :) More precisely MHLv3 (used in Sii8620)
>>>> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
>>>>
>>>> Unfortunately I have no access to MHL specs, but my experiments and
>>>> vendor drivers strongly suggests it is done this way.
>>>>
>>>> This is important in case of 4K modes which are handled differently by
>>>> HDMI 1.4 and HDMI2.0.
>>> HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
>>> the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
>>> switch over to the HDMI 2.0 specific signalling.
>>
>> The difference is in infoframes:
>>
>> HDMI 1.4 sets AVI infoframe VIC to 0, and sends HDMI_VIC in VSI.
>>
>> HDMI 2.0 sets AVI infoframe to non zero VICs introduced by
>> HDMI2.0/CEA-861-F, VSI can be omitted if I remember correctly, unless 3d
>> is in use.
> Like I said, The HDMI 1.4 method is used even with HDMI 2.0 sinks unless
> some feature gets used which can't be signalled via the HDMI 1.4 vendor
> specific infoframe.


Do you mean that 4K VICs 95, 94, 93, 98 defined in CEA-861-F are not
used at all for non-3d video in HDMI 2.0?

Chapter 10.1 of HDMI2.0 spec says clearly:

> When transmitting any additional Video Format for which a VIC value
> has been defined in
> CEA-861-F tables 1, 2, and 3, an HDMI Source shall set the VIC field
> to the Video Code for
> that format.


It contradicts your statement, or am I missing something?


>
>>
>> So setting VICs to non-zero in case of HDMI1.4 sinks and 4k modes seems
>> risky.
> That is not what I was proposing.


Maybe I have misread your statement:

On 21.11.2018 12:51, Ville Syrjälä wrote:

> That said, I was actually thinking of removing this hdmi2 vs. not
> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> "just in case", but we already have a similar issue with earlier
> cea-861 revisions and haven't seen any bugs where an older monitor
> would get confused by a VIC not yet defined when the monitor was
> produced.


I do not know what it could mean other that allowing sending VICs
unknown to sink?

Maybe better would be just to wait for the patch to avoid misunderstandings.


Regards

Andrzej


>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>>> The pipeline looks like (in parenthesis HDMI version on the stream):
>>>>
>>>> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
>>>>
>>>>
>>>>> I guess I can change this to true to not change it. IIRC
>>>>> that was the only driver that didn't have a connector around.
>>>>>
>>>>> That said, I was actually thinking of removing this hdmi2 vs. not
>>>>> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
>>>>> "just in case", but we already have a similar issue with earlier
>>>>> cea-861 revisions and haven't seen any bugs where an older monitor
>>>>> would get confused by a VIC not yet defined when the monitor was
>>>>> produced.
>>>>>
>>>> Are you sure this is a good idea? As I said earlier 4K modes are present
>>>> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
>>>> only matter of unknown VIC in AVIF.
>>>>
>>>>
>>>> Regards
>>>>
>>>> Andrzej

Andrzej Hajda Dec. 5, 2018, 8:46 a.m. UTC | #13

On 05.12.2018 07:32, Laurent Pinchart wrote:
> Hi Ville,
>
> On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote:
>> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote:
>>> On 03.12.2018 22:38, Ville Syrjälä wrote:
>>>> On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote:
>>>>> On 21.11.2018 19:19, Laurent Pinchart wrote:
>>>>>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
>>>>>>> From: Ville Syrjälä <[e-mail]>
>>>>>>>
>>>>>>> Make life easier for drivers by simply passing the connector
>>>>>>> to drm_hdmi_avi_infoframe_from_display_mode() and
>>>>>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
>>>>>>> need to worry about is_hdmi2_sink mess.
>>>>>> While this is good for display controller drivers, the change isn't
>>>>>> great for bridge drivers. Down the road we're looking at moving
>>>>>> connector support out of the bridge drivers. Adding an additional
>>>>>> dependency to connectors in the bridges will make that more
>>>>>> difficult. Ideally bridges should retrieve the information from their
>>>>>> sink, regardless of whether it is a connector or another bridge.
>>>>> I agree with it, and case of sii8620 shows that there are cases where
>>>>> bridge has no direct access to the connector.
>>>> It's just a matter of plumbing it through.
>>> What do you mean exactly?
>> void bridge_foo(...
>> +               ,struct drm_connector *connector);
>>
>>>>> On the other side,  since you are passing connector to
>>>>> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
>>>>> parameter and rename the function to
>>>>> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
>>>>> mode set on the connector differs?
>>>> Connectors don't have a mode.
>>> As they are passing video stream they should have it, even if not
>>> directly, for example:
>>>
>>> connector->state->crtc->mode
>> That's not really how atomic works. One shouldn't go digging
>> through the obj->state pointers when we're not holding the
>> relevant locks anymore. The atomic way would be to pass either
>> both crtc state and connector state, or drm_atomic_state +
>> crtc/connector.


Usually infoframe contents is generated in modesetting/enable callbacks
so the locks should be in place.

And the locks should be hold for
drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if

generated infoframe is not relevant to actual video mode. I guess that
if connector->state->crtc->mode

differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it
is a sign of possible problem.

And if they do not differ passing mode as an extra argument is redundant.


> Or a bridge state ? With chained bridges the mode can vary along the pipeline, 
> the CRTC adjusted mode will only cover the link between the CRTC and the first 
> bridge. It's only a matter of time until we need to store other intermediate 
> modes in states. I'd rather prepare for that instead of passing the CRTC state 
> to bridges.


Yes, optional bridge states seems reasonable, volunteers needed :)


Regards

Andrzej


>
>>> In moment of creating infoframe it should be set properly.
>>>
>>>>>> Please see below for an additional comment.
>>>>>>
>>>>>>> Cc: Alex Deucher <[e-mail]>
>>>>>>> Cc: "Christian König" <[e-mail]>
>>>>>>> Cc: "David (ChunMing) Zhou" <[e-mail]>
>>>>>>> Cc: Archit Taneja <[e-mail]>
>>>>>>> Cc: Andrzej Hajda <[e-mail]>
>>>>>>> Cc: Laurent Pinchart <[e-mail]>
>>>>>>> Cc: Inki Dae <[e-mail]>
>>>>>>> Cc: Joonyoung Shim <[e-mail]>
>>>>>> Cc: Seung-Woo Kim <[e-mail]>
>>>>>>> Cc: Kyungmin Park <[e-mail]>
>>>>>>> Cc: Russell King <[e-mail]>
>>>>>>> Cc: CK Hu <[e-mail]>
>>>>>>> Cc: Philipp Zabel <[e-mail]>
>>>>>>> Cc: Rob Clark <[e-mail]>
>>>>>>> Cc: Ben Skeggs <[e-mail]>
>>>>>>> Cc: Tomi Valkeinen <[e-mail]>
>>>>>>> Cc: Sandy Huang <[e-mail]>
>>>>>>> Cc: "Heiko Stübner" <[e-mail]>
>>>>>>> Cc: Benjamin Gaignard <[e-mail]>
>>>>>>> Cc: Vincent Abriou <[e-mail]>
>>>>>>> Cc: Thierry Reding <[e-mail]>
>>>>>>> Cc: Eric Anholt <[e-mail]>
>>>>>>> Cc: Shawn Guo <[e-mail]>
>>>>>>> Cc: Ilia Mirkin <[e-mail]>
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Signed-off-by: Ville Syrjälä <[e-mail]>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
>>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
>>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
>>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
>>>>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
>>>>>>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
>>>>>>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
>>>>>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
>>>>>>>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++--------
>>>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
>>>>>>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
>>>>>>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
>>>>>>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
>>>>>>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
>>>>>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
>>>>>>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
>>>>>>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
>>>>>>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
>>>>>>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
>>>>>>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
>>>>>>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
>>>>>>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
>>>>>>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
>>>>>>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
>>>>>>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
>>>>>>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
>>>>>>>  include/drm/drm_edid.h                    |  8 +++---
>>>>>>>  27 files changed, 94 insertions(+), 66 deletions(-)
>>>>>> For dw-hdmi and omapdrm,
>>>>>>
>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>

Russell King (Oracle) Dec. 5, 2018, 10:19 a.m. UTC | #14

On Tue, Nov 20, 2018 at 06:13:42PM +0200, Ville Syrjala wrote:
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..38c66fbc8276 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -849,7 +849,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
>  {
>  	union hdmi_infoframe frame;
>  
> -	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
> +	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						 &priv->connector, mode);
>  	frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
>  
>  	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);

For this,

Acked-by: Russell King <[e-mail]>

Thanks.

Ville Syrjälä Dec. 5, 2018, 2:43 p.m. UTC | #15

On Wed, Dec 05, 2018 at 09:46:40AM +0100, Andrzej Hajda wrote:
> On 05.12.2018 07:32, Laurent Pinchart wrote:
> > Hi Ville,
> >
> > On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote:
> >> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote:
> >>> On 03.12.2018 22:38, Ville Syrjälä wrote:
> >>>> On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote:
> >>>>> On 21.11.2018 19:19, Laurent Pinchart wrote:
> >>>>>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote:
> >>>>>>> From: Ville Syrjälä <[e-mail]>
> >>>>>>>
> >>>>>>> Make life easier for drivers by simply passing the connector
> >>>>>>> to drm_hdmi_avi_infoframe_from_display_mode() and
> >>>>>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> >>>>>>> need to worry about is_hdmi2_sink mess.
> >>>>>> While this is good for display controller drivers, the change isn't
> >>>>>> great for bridge drivers. Down the road we're looking at moving
> >>>>>> connector support out of the bridge drivers. Adding an additional
> >>>>>> dependency to connectors in the bridges will make that more
> >>>>>> difficult. Ideally bridges should retrieve the information from their
> >>>>>> sink, regardless of whether it is a connector or another bridge.
> >>>>> I agree with it, and case of sii8620 shows that there are cases where
> >>>>> bridge has no direct access to the connector.
> >>>> It's just a matter of plumbing it through.
> >>> What do you mean exactly?
> >> void bridge_foo(...
> >> +               ,struct drm_connector *connector);
> >>
> >>>>> On the other side,  since you are passing connector to
> >>>>> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode
> >>>>> parameter and rename the function to
> >>>>> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and
> >>>>> mode set on the connector differs?
> >>>> Connectors don't have a mode.
> >>> As they are passing video stream they should have it, even if not
> >>> directly, for example:
> >>>
> >>> connector->state->crtc->mode
> >> That's not really how atomic works. One shouldn't go digging
> >> through the obj->state pointers when we're not holding the
> >> relevant locks anymore. The atomic way would be to pass either
> >> both crtc state and connector state, or drm_atomic_state +
> >> crtc/connector.
> 
> 
> Usually infoframe contents is generated in modesetting/enable callbacks
> so the locks should be in place.

Not when doing a nonblocking modeset.

> 
> And the locks should be hold for
> drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if
> 
> generated infoframe is not relevant to actual video mode. I guess that
> if connector->state->crtc->mode
> 
> differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it
> is a sign of possible problem.
> 
> And if they do not differ passing mode as an extra argument is redundant.
> 
> 
> > Or a bridge state ? With chained bridges the mode can vary along the pipeline, 
> > the CRTC adjusted mode will only cover the link between the CRTC and the first 
> > bridge. It's only a matter of time until we need to store other intermediate 
> > modes in states. I'd rather prepare for that instead of passing the CRTC state 
> > to bridges.
> 
> 
> Yes, optional bridge states seems reasonable, volunteers needed :)
> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> >>> In moment of creating infoframe it should be set properly.
> >>>
> >>>>>> Please see below for an additional comment.
> >>>>>>
> >>>>>>> Cc: Alex Deucher <[e-mail]>
> >>>>>>> Cc: "Christian König" <[e-mail]>
> >>>>>>> Cc: "David (ChunMing) Zhou" <[e-mail]>
> >>>>>>> Cc: Archit Taneja <[e-mail]>
> >>>>>>> Cc: Andrzej Hajda <[e-mail]>
> >>>>>>> Cc: Laurent Pinchart <[e-mail]>
> >>>>>>> Cc: Inki Dae <[e-mail]>
> >>>>>>> Cc: Joonyoung Shim <[e-mail]>
> >>>>>> Cc: Seung-Woo Kim <[e-mail]>
> >>>>>>> Cc: Kyungmin Park <[e-mail]>
> >>>>>>> Cc: Russell King <[e-mail]>
> >>>>>>> Cc: CK Hu <[e-mail]>
> >>>>>>> Cc: Philipp Zabel <[e-mail]>
> >>>>>>> Cc: Rob Clark <[e-mail]>
> >>>>>>> Cc: Ben Skeggs <[e-mail]>
> >>>>>>> Cc: Tomi Valkeinen <[e-mail]>
> >>>>>>> Cc: Sandy Huang <[e-mail]>
> >>>>>>> Cc: "Heiko Stübner" <[e-mail]>
> >>>>>>> Cc: Benjamin Gaignard <[e-mail]>
> >>>>>>> Cc: Vincent Abriou <[e-mail]>
> >>>>>>> Cc: Thierry Reding <[e-mail]>
> >>>>>>> Cc: Eric Anholt <[e-mail]>
> >>>>>>> Cc: Shawn Guo <[e-mail]>
> >>>>>>> Cc: Ilia Mirkin <[e-mail]>
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Signed-off-by: Ville Syrjälä <[e-mail]>
> >>>>>>> ---
> >>>>>>>
> >>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c    |  2 +-
> >>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c    |  2 +-
> >>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c     |  3 ++-
> >>>>>>>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c     |  2 +-
> >>>>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  5 ++--
> >>>>>>>  drivers/gpu/drm/bridge/sii902x.c          |  3 ++-
> >>>>>>>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 +--
> >>>>>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 ++-
> >>>>>>>  drivers/gpu/drm/drm_edid.c                | 33 ++++++++++++--------
> >>>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  3 ++-
> >>>>>>>  drivers/gpu/drm/i2c/tda998x_drv.c         |  3 ++-
> >>>>>>>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 +++++-----
> >>>>>>>  drivers/gpu/drm/i915/intel_lspcon.c       | 15 ++++++-----
> >>>>>>>  drivers/gpu/drm/i915/intel_sdvo.c         | 10 ++++---
> >>>>>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
> >>>>>>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c    |  3 ++-
> >>>>>>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  7 +++--
> >>>>>>>  drivers/gpu/drm/omapdrm/omap_encoder.c    |  5 ++--
> >>>>>>>  drivers/gpu/drm/radeon/radeon_audio.c     |  2 +-
> >>>>>>>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  4 ++-
> >>>>>>>  drivers/gpu/drm/sti/sti_hdmi.c            |  3 ++-
> >>>>>>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c    |  3 ++-
> >>>>>>>  drivers/gpu/drm/tegra/hdmi.c              |  3 ++-
> >>>>>>>  drivers/gpu/drm/tegra/sor.c               |  3 ++-
> >>>>>>>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 11 +++++---
> >>>>>>>  drivers/gpu/drm/zte/zx_hdmi.c             |  4 ++-
> >>>>>>>  include/drm/drm_edid.h                    |  8 +++---
> >>>>>>>  27 files changed, 94 insertions(+), 66 deletions(-)
> >>>>>> For dw-hdmi and omapdrm,
> >>>>>>
> >>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>

Ville Syrjälä Dec. 5, 2018, 3:06 p.m. UTC | #16

On Wed, Dec 05, 2018 at 08:40:34AM +0100, Andrzej Hajda wrote:
> On 04.12.2018 20:02, Ville Syrjälä wrote:
> > On Tue, Dec 04, 2018 at 08:03:53AM +0100, Andrzej Hajda wrote:
> >> On 03.12.2018 22:48, Ville Syrjälä wrote:
> >>> On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> >>>> Quite late, hopefully not too late.
> >>>>
> >>>>
> >>>> On 21.11.2018 12:51, Ville Syrjälä wrote:
> >>>>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>>>>>>  		return;
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> index a6e8f4591e63..0cc293a6ac24 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
> >>>>>>>  	int ret;
> >>>>>>>  
> >>>>>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> >>>>>>> -						       mode,
> >>>>>>> -						       true);
> >>>>>>> +						       NULL, mode);
> >>>>>>>  	if (ctx->use_packed_pixel)
> >>>>>>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>>>>>  
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> index 64c3cf027518..88b720b63126 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> >>>>>>>  	u8 val;
> >>>>>>>  
> >>>>>>>  	/* Initialise info frame from DRM mode */
> >>>>>>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> >>>>>>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> >>>>>>> +						 &hdmi->connector, mode);
> >>>>>>>  
> >>>>>>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>>>>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>>>>> index b506e3622b08..501ac05ba7da 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
> >>>>>>>  }
> >>>>>>>  EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>>>>>  
> >>>>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >>>>>> You're usually known for adding const all around, why not const pointer
> >>>>>> here and in all the other drm_* functions that call this?
> >>>>> My current approach is to constify states/fbs/etc. but not so much
> >>>>> crtcs/connectors/etc. Too much const can sometimes get in the way
> >>>>> of things requiring that you remove the const later. But I guess
> >>>>> in this case the const shouldn't really get in the way of anything
> >>>>> because these are pretty much supposed to be pure functions.
> >>>>>
> >>>>>>> +{
> >>>>>>> +	/*
> >>>>>>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
> >>>>>>> +	 * we need one, so we have to be prepared for a NULL connector.
> >>>>>>> +	 */
> >>>>>>> +	if (!connector)
> >>>>>>> +		return false;
> >>>>>> This actually changes the is_hdmi2_sink value for sil-sii8620.
> >>>>> Hmm. No idea why they would have set that to true when everyone else is
> >>>>> passing false. 
> >>>> Because false does not work :) More precisely MHLv3 (used in Sii8620)
> >>>> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
> >>>>
> >>>> Unfortunately I have no access to MHL specs, but my experiments and
> >>>> vendor drivers strongly suggests it is done this way.
> >>>>
> >>>> This is important in case of 4K modes which are handled differently by
> >>>> HDMI 1.4 and HDMI2.0.
> >>> HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
> >>> the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
> >>> switch over to the HDMI 2.0 specific signalling.
> >>
> >> The difference is in infoframes:
> >>
> >> HDMI 1.4 sets AVI infoframe VIC to 0, and sends HDMI_VIC in VSI.
> >>
> >> HDMI 2.0 sets AVI infoframe to non zero VICs introduced by
> >> HDMI2.0/CEA-861-F, VSI can be omitted if I remember correctly, unless 3d
> >> is in use.
> > Like I said, The HDMI 1.4 method is used even with HDMI 2.0 sinks unless
> > some feature gets used which can't be signalled via the HDMI 1.4 vendor
> > specific infoframe.
> 
> 
> Do you mean that 4K VICs 95, 94, 93, 98 defined in CEA-861-F are not
> used at all for non-3d video in HDMI 2.0?
> 
> Chapter 10.1 of HDMI2.0 spec says clearly:
> 
> > When transmitting any additional Video Format for which a VIC value
> > has been defined in
> > CEA-861-F tables 1, 2, and 3, an HDMI Source shall set the VIC field
> > to the Video Code for
> > that format.
> 
> 
> It contradicts your statement, or am I missing something?

IIRC the spec said you can only use either one or the other.

Section 10.2 doesn't explicity say that the AVI VIC must be zero when
HDMI_VIC is non-zero, but it does more or less imply it. And looks
like appendix E outright forbids having both as non-zero.

> 
> 
> >
> >>
> >> So setting VICs to non-zero in case of HDMI1.4 sinks and 4k modes seems
> >> risky.
> > That is not what I was proposing.
> 
> 
> Maybe I have misread your statement:
> 
> On 21.11.2018 12:51, Ville Syrjälä wrote:
> 
> > That said, I was actually thinking of removing this hdmi2 vs. not
> > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> > "just in case", but we already have a similar issue with earlier
> > cea-861 revisions and haven't seen any bugs where an older monitor
> > would get confused by a VIC not yet defined when the monitor was
> > produced.
> 
> 
> I do not know what it could mean other that allowing sending VICs
> unknown to sink?

Yes it might send unknown VICs to sinks. But that happens anyway becasue
we can't tell which VICs the sink supports. We can only try to deduce
the spec version indirectly which is not very robust. And we only do that
for CTA-861-F, earlier revisions of the spec we don't try to deduce at all.
And alternative might be to send VICs only for those modes that the sink
actually declared in the EDID, but that opens up the question of whether
we should only do that for modes that have an SVD, or whether we should
do it for modes we get by other means (eg. detailed timing descriptors).

What I wasn't suggesting was to send AVI VIC != 0 + HDMI_VIC != 0
because that is not legal.

> 
> Maybe better would be just to wait for the patch to avoid misunderstandings.
> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> >>
> >> Regards
> >>
> >> Andrzej
> >>
> >>
> >>>> The pipeline looks like (in parenthesis HDMI version on the stream):
> >>>>
> >>>> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
> >>>>
> >>>>
> >>>>> I guess I can change this to true to not change it. IIRC
> >>>>> that was the only driver that didn't have a connector around.
> >>>>>
> >>>>> That said, I was actually thinking of removing this hdmi2 vs. not
> >>>>> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> >>>>> "just in case", but we already have a similar issue with earlier
> >>>>> cea-861 revisions and haven't seen any bugs where an older monitor
> >>>>> would get confused by a VIC not yet defined when the monitor was
> >>>>> produced.
> >>>>>
> >>>> Are you sure this is a good idea? As I said earlier 4K modes are present
> >>>> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
> >>>> only matter of unknown VIC in AVIF.
> >>>>
> >>>>
> >>>> Regards
> >>>>
> >>>> Andrzej
>

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 4cfecdce29a3..1f0426d2fc2a 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1682,7 +1682,7 @@  static void dce_v10_0_afmt_setmode(struct drm_encoder *encoder,
 	dce_v10_0_audio_write_sad_regs(encoder);
 	dce_v10_0_audio_write_latency_fields(encoder, mode);
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
 	if (err < 0) {
 		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
 		return;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 7c868916d90f..2280b971d758 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1724,7 +1724,7 @@  static void dce_v11_0_afmt_setmode(struct drm_encoder *encoder,
 	dce_v11_0_audio_write_sad_regs(encoder);
 	dce_v11_0_audio_write_latency_fields(encoder, mode);
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
 	if (err < 0) {
 		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
 		return;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 17eaaba36017..db443ec53d3a 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1423,6 +1423,7 @@  static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder,
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
 	struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
+	struct drm_connector *connector = amdgpu_get_connector_for_encoder(encoder);
 	struct hdmi_avi_infoframe frame;
 	u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
 	uint8_t *payload = buffer + 3;
@@ -1430,7 +1431,7 @@  static void dce_v6_0_audio_set_avi_infoframe(struct drm_encoder *encoder,
 	ssize_t err;
 	u32 tmp;
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
 	if (err < 0) {
 		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
 		return;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 8c0576978d36..13da915991dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1616,7 +1616,7 @@  static void dce_v8_0_afmt_setmode(struct drm_encoder *encoder,
 	dce_v8_0_audio_write_sad_regs(encoder);
 	dce_v8_0_audio_write_latency_fields(encoder, mode);
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
 	if (err < 0) {
 		DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
 		return;
diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index f8433c93f463..e11309e9bc4f 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1094,8 +1094,9 @@  static void anx78xx_bridge_mode_set(struct drm_bridge *bridge,
 
 	mutex_lock(&anx78xx->lock);
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode,
-						       false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						       &anx78xx->connector,
+						       adjusted_mode);
 	if (err) {
 		DRM_ERROR("Failed to setup AVI infoframe: %d\n", err);
 		goto unlock;
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index bfa902013aa4..a9b4f45ae87c 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -258,7 +258,8 @@  static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
 	if (ret)
 		return;
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						       &sii902x->connector, adj);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
 		return;
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index a6e8f4591e63..0cc293a6ac24 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -1104,8 +1104,7 @@  static void sii8620_set_infoframes(struct sii8620 *ctx,
 	int ret;
 
 	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
-						       mode,
-						       true);
+						       NULL, mode);
 	if (ctx->use_packed_pixel)
 		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
 
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 64c3cf027518..88b720b63126 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1344,7 +1344,8 @@  static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	u8 val;
 
 	/* Initialise info frame from DRM mode */
-	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						 &hdmi->connector, mode);
 
 	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
 		frame.colorspace = HDMI_COLORSPACE_YUV444;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b506e3622b08..501ac05ba7da 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4830,19 +4830,32 @@  void drm_set_preferred_mode(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_set_preferred_mode);
 
+static bool is_hdmi2_sink(struct drm_connector *connector)
+{
+	/*
+	 * FIXME: sil-sii8620 doesn't have a connector around when
+	 * we need one, so we have to be prepared for a NULL connector.
+	 */
+	if (!connector)
+		return false;
+
+	return connector->display_info.hdmi.scdc.supported ||
+		connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB420;
+}
+
 /**
  * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
  *                                              data from a DRM display mode
  * @frame: HDMI AVI infoframe
+ * @connector: the connector
  * @mode: DRM display mode
- * @is_hdmi2_sink: Sink is HDMI 2.0 compliant
  *
  * Return: 0 on success or a negative error code on failure.
  */
 int
 drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
-					 const struct drm_display_mode *mode,
-					 bool is_hdmi2_sink)
+					 struct drm_connector *connector,
+					 const struct drm_display_mode *mode)
 {
 	enum hdmi_picture_aspect picture_aspect;
 	int err;
@@ -4864,7 +4877,7 @@  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 	 * HDMI 2.0 VIC range: 1 <= VIC <= 107 (CEA-861-F). So we
 	 * have to make sure we dont break HDMI 1.4 sinks.
 	 */
-	if (!is_hdmi2_sink && frame->video_code > 64)
+	if (!is_hdmi2_sink(connector) && frame->video_code > 64)
 		frame->video_code = 0;
 
 	/*
@@ -4923,21 +4936,17 @@  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
  * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
  *                                        quantization range information
  * @frame: HDMI AVI infoframe
+ * @connector: the connector
  * @mode: DRM display mode
  * @rgb_quant_range: RGB quantization range (Q)
  * @rgb_quant_range_selectable: Sink support selectable RGB quantization range (QS)
- * @is_hdmi2_sink: HDMI 2.0 sink, which has different default recommendations
- *
- * Note that @is_hdmi2_sink can be derived by looking at the
- * &drm_scdc.supported flag stored in &drm_hdmi_info.scdc,
- * &drm_display_info.hdmi, which can be found in &drm_connector.display_info.
  */
 void
 drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
+				   struct drm_connector *connector,
 				   const struct drm_display_mode *mode,
 				   enum hdmi_quantization_range rgb_quant_range,
-				   bool rgb_quant_range_selectable,
-				   bool is_hdmi2_sink)
+				   bool rgb_quant_range_selectable)
 {
 	/*
 	 * CEA-861:
@@ -4968,7 +4977,7 @@  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
 	 * we limit non-zero YQ to HDMI 2.0 sinks only as HDMI 2.0 is based
 	 * on on CEA-861-F.
 	 */
-	if (!is_hdmi2_sink ||
+	if (!is_hdmi2_sink(connector) ||
 	    rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED)
 		frame->ycc_quantization_range =
 			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 2092a650df7d..b857df67aff0 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -819,7 +819,8 @@  static void hdmi_reg_infoframes(struct hdmi_context *hdata)
 		return;
 	}
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi, m, false);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
+						       &hdata->connector, m);
 	if (!ret)
 		ret = hdmi_avi_infoframe_pack(&frm.avi, buf, sizeof(buf));
 	if (ret > 0) {
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..38c66fbc8276 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -849,7 +849,8 @@  tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 {
 	union hdmi_infoframe frame;
 
-	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
+	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						 &priv->connector, mode);
 	frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
 
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index e2c6a2b3e8f2..8a260bd6954c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -465,15 +465,12 @@  static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->base.adjusted_mode;
-	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
-	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported ||
-	   connector->display_info.color_formats & DRM_COLOR_FORMAT_YCRCB420;
 	union hdmi_infoframe frame;
 	int ret;
 
 	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						       adjusted_mode,
-						       is_hdmi2_sink);
+						       conn_state->connector,
+						       adjusted_mode);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
 		return;
@@ -486,12 +483,13 @@  static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
 	else
 		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
 
-	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
+					   conn_state->connector,
+					   adjusted_mode,
 					   crtc_state->limited_color_range ?
 					   HDMI_QUANTIZATION_RANGE_LIMITED :
 					   HDMI_QUANTIZATION_RANGE_FULL,
-					   intel_hdmi->rgb_quant_range_selectable,
-					   is_hdmi2_sink);
+					   intel_hdmi->rgb_quant_range_selectable);
 
 	drm_hdmi_avi_infoframe_content_type(&frame.avi,
 					    conn_state);
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 96a8d9524b0c..c18f14d41f1d 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -462,10 +462,8 @@  void lspcon_set_infoframes(struct intel_encoder *encoder,
 	uint8_t buf[VIDEO_DIP_DATA_SIZE];
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 	struct intel_lspcon *lspcon = &dig_port->lspcon;
-	struct intel_dp *intel_dp = &dig_port->dp;
-	struct drm_connector *connector = &intel_dp->attached_connector->base;
-	const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode;
-	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
+	const struct drm_display_mode *adjusted_mode =
+		&crtc_state->base.adjusted_mode;
 
 	if (!lspcon->active) {
 		DRM_ERROR("Writing infoframes while LSPCON disabled ?\n");
@@ -473,7 +471,8 @@  void lspcon_set_infoframes(struct intel_encoder *encoder,
 	}
 
 	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						       mode, is_hdmi2_sink);
+						       conn_state->connector,
+						       adjusted_mode);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
 		return;
@@ -488,11 +487,13 @@  void lspcon_set_infoframes(struct intel_encoder *encoder,
 		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
 	}
 
-	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
+					   conn_state->connector,
+					   adjusted_mode,
 					   crtc_state->limited_color_range ?
 					   HDMI_QUANTIZATION_RANGE_LIMITED :
 					   HDMI_QUANTIZATION_RANGE_FULL,
-					   false, is_hdmi2_sink);
+					   false);
 
 	ret = hdmi_infoframe_pack(&frame, buf, sizeof(buf));
 	if (ret < 0) {
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5805ec1aba12..1277d31adb54 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -981,7 +981,8 @@  static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
 }
 
 static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
-					 const struct intel_crtc_state *pipe_config)
+					 const struct intel_crtc_state *pipe_config,
+					 const struct drm_connector_state *conn_state)
 {
 	uint8_t sdvo_data[HDMI_INFOFRAME_SIZE(AVI)];
 	union hdmi_infoframe frame;
@@ -989,8 +990,8 @@  static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
 	ssize_t len;
 
 	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						       &pipe_config->base.adjusted_mode,
-						       false);
+						       conn_state->connector,
+						       &pipe_config->base.adjusted_mode);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
 		return false;
@@ -1316,7 +1317,8 @@  static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
 		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_HDMI);
 		intel_sdvo_set_colorimetry(intel_sdvo,
 					   SDVO_COLORIMETRY_RGB256);
-		intel_sdvo_set_avi_infoframe(intel_sdvo, crtc_state);
+		intel_sdvo_set_avi_infoframe(intel_sdvo,
+					     crtc_state, conn_state);
 	} else
 		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI);
 
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 862f3ec22131..f3ef7bf80563 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -981,7 +981,8 @@  static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi,
 	u8 buffer[17];
 	ssize_t err;
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						       &hdmi->conn, mode);
 	if (err < 0) {
 		dev_err(hdmi->dev,
 			"Failed to get AVI infoframe from mode: %zd\n", err);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 7e357077ed26..5ed4cab2819f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -101,7 +101,8 @@  static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
 	u32 val;
 	int len;
 
-	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
+	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						 hdmi->connector, mode);
 
 	len = hdmi_infoframe_pack(&frame, buffer, sizeof(buffer));
 	if (len < 0) {
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 6cbbae3f438b..d8e512c83211 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -554,7 +554,7 @@  nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
 	u32 max_ac_packet;
 	union hdmi_infoframe avi_frame;
 	union hdmi_infoframe vendor_frame;
-	bool scdc_supported, high_tmds_clock_ratio = false, scrambling = false;
+	bool high_tmds_clock_ratio = false, scrambling = false;
 	u8 config;
 	int ret;
 	int size;
@@ -564,10 +564,9 @@  nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
 		return;
 
 	hdmi = &nv_connector->base.display_info.hdmi;
-	scdc_supported = hdmi->scdc.supported;
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame.avi, mode,
-						       scdc_supported);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame.avi,
+						       &nv_connector->base, mode);
 	if (!ret) {
 		/* We have an AVI InfoFrame, populate it to the display */
 		args.pwr.avi_infoframe_length
diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index 452e625f6ce3..281b2164969c 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -128,8 +128,9 @@  static void omap_encoder_mode_set(struct drm_encoder *encoder,
 	if (hdmi_mode && dssdev->ops->hdmi.set_infoframe) {
 		struct hdmi_avi_infoframe avi;
 
-		r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode,
-							     false);
+		r = drm_hdmi_avi_infoframe_from_display_mode(&avi,
+							     connector,
+							     adjusted_mode);
 		if (r == 0)
 			dssdev->ops->hdmi.set_infoframe(dssdev, &avi);
 	}
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c
index 770e31f5fd1b..5a7d48339b32 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -516,7 +516,7 @@  static int radeon_audio_set_avi_packet(struct drm_encoder *encoder,
 	if (!connector)
 		return -EINVAL;
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, connector, mode);
 	if (err < 0) {
 		DRM_ERROR("failed to setup AVI infoframe: %d\n", err);
 		return err;
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 1c02b3e61299..27c945e030a0 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -295,7 +295,9 @@  static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
 	union hdmi_infoframe frame;
 	int rc;
 
-	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
+	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						      &hdmi->connector,
+						      mode);
 
 	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444)
 		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index ccf718404a1c..4b86878f8ddf 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -434,7 +434,8 @@  static int hdmi_avi_infoframe_config(struct sti_hdmi *hdmi)
 
 	DRM_DEBUG_DRIVER("\n");
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode, false);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe,
+						       hdmi->drm_connector, mode);
 	if (ret < 0) {
 		DRM_ERROR("failed to setup AVI infoframe: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index 061d2e0d9011..554a6f4561f3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -52,7 +52,8 @@  static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi,
 	u8 buffer[17];
 	int i, ret;
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						       &hdmi->connector, mode);
 	if (ret < 0) {
 		DRM_ERROR("Failed to get infoframes from mode\n");
 		return ret;
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index 0082468f703c..a7566c67bfb0 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -741,7 +741,8 @@  static void tegra_hdmi_setup_avi_infoframe(struct tegra_hdmi *hdmi,
 	u8 buffer[17];
 	ssize_t err;
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						       &hdmi->output.connector, mode);
 	if (err < 0) {
 		dev_err(hdmi->dev, "failed to setup AVI infoframe: %zd\n", err);
 		return;
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index b129da2e5afd..d90bf5f6a67a 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -2116,7 +2116,8 @@  tegra_sor_hdmi_setup_avi_infoframe(struct tegra_sor *sor,
 	value &= ~INFOFRAME_CTRL_ENABLE;
 	tegra_sor_writel(sor, value, SOR_HDMI_AVI_INFOFRAME_CTRL);
 
-	err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
+	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
+						       &sor->output.connector, mode);
 	if (err < 0) {
 		dev_err(sor->dev, "failed to setup AVI infoframe: %d\n", err);
 		return err;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index fd5522fd179e..cb3dca7426c8 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -409,22 +409,25 @@  static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	struct drm_crtc *crtc = encoder->crtc;
+	struct vc4_dev *vc4 = to_vc4_dev(encoder->dev);
+	struct vc4_hdmi *hdmi = vc4->hdmi;
 	const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
 	union hdmi_infoframe frame;
 	int ret;
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       hdmi->connector, mode);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
 		return;
 	}
 
-	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
+					   hdmi->connector, mode,
 					   vc4_encoder->limited_rgb_range ?
 					   HDMI_QUANTIZATION_RANGE_LIMITED :
 					   HDMI_QUANTIZATION_RANGE_FULL,
-					   vc4_encoder->rgb_range_selectable,
-					   false);
+					   vc4_encoder->rgb_range_selectable);
 
 	vc4_hdmi_write_infoframe(encoder, &frame);
 }
diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c
index 78655269d843..9fc98bb4f3d9 100644
--- a/drivers/gpu/drm/zte/zx_hdmi.c
+++ b/drivers/gpu/drm/zte/zx_hdmi.c
@@ -125,7 +125,9 @@  static int zx_hdmi_config_video_avi(struct zx_hdmi *hdmi,
 	union hdmi_infoframe frame;
 	int ret;
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       &hdmi->connector,
+						       mode);
 	if (ret) {
 		DRM_DEV_ERROR(hdmi->dev, "failed to get avi infoframe: %d\n",
 			      ret);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index e3c404833115..9db6f130df65 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -352,18 +352,18 @@  drm_load_edid_firmware(struct drm_connector *connector)
 
 int
 drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
-					 const struct drm_display_mode *mode,
-					 bool is_hdmi2_sink);
+					 struct drm_connector *connector,
+					 const struct drm_display_mode *mode);
 int
 drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
 					    struct drm_connector *connector,
 					    const struct drm_display_mode *mode);
 void
 drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
+				   struct drm_connector *connector,
 				   const struct drm_display_mode *mode,
 				   enum hdmi_quantization_range rgb_quant_range,
-				   bool rgb_quant_range_selectable,
-				   bool is_hdmi2_sink);
+				   bool rgb_quant_range_selectable);
 
 /**
  * drm_eld_mnl - Get ELD monitor name length in bytes.