On 06/21/2017 12:33 PM, Shashank Sharma wrote:

HDMI 1.4b support the CEA video modes as per range of CEA-861-D (VIC 1-64). For any other mode, the VIC filed in AVI infoframes should be 0. HDMI 2.0 sinks, support video modes range as per CEA-861-F spec, which is extended to (VIC 1-107).

This patch adds a bool input variable, which indicates if the connected sink is a HDMI 2.0 sink or not. This will make sure that we don't pass a HDMI 2.0 VIC to a HDMI 1.4 sink.

This patch touches all drm drivers, who are callers of this function drm_hdmi_avi_infoframe_from_display_mode but to make sure there is no change in current behavior, is_hdmi2 is kept as false.

In case of I915 driver, this patch:

• checks if the connected display is HDMI 2.0.
• HDMI infoframes carry one of this two type of information:
  • VIC for 4K modes for HDMI 1.4 sinks
  • S3D information for S3D modes
  As CEA-861-F has already defined VICs for 4K videomodes, this patch doesn't allow sending HDMI infoframes for HDMI 2.0 sinks, until the mode is 3D.

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu jose.abreu@synopsys.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Daniel Vetter daniel.vetter@intel.com

PS: This patch touches a few lines in few files, which were already above 80 char, so checkpatch gives 80 char warning again.

• gpu/drm/omapdrm/omap_encoder.c
• gpu/drm/i915/intel_sdvo.c

V2: Rebase, Added r-b from Andrzej V3: Addressed review comment from Ville:

• Do not send VICs in both AVI-IF and HDMI-IF send only one of it.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

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_v8_0.c | 2 +- drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 ++- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/drm_edid.c | 12 +++++++++++- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/i915/intel_hdmi.c | 17 ++++++++++++++++- drivers/gpu/drm/i915/intel_sdvo.c | 3 ++- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 ++- drivers/gpu/drm/radeon/radeon_audio.c | 2 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/tegra/hdmi.c | 2 +- drivers/gpu/drm/tegra/sor.c | 2 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- drivers/gpu/drm/zte/zx_hdmi.c | 2 +- include/drm/drm_edid.h | 3 ++- 21 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c index 3c62c45..4923ddc 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c @@ -1864,7 +1864,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 c8ed0fa..4101684 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c @@ -1848,7 +1848,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 3e90c19..a7f6b32 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c @@ -1747,7 +1747,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 9006578..a083211 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -1097,7 +1097,8 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode,

• 				       false);
  

  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 9b87067..3dc40f6 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -269,7 +269,7 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, if (ret) return;

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index ead1124..0b0c9de 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1317,7 +1317,7 @@ 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);

• drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);

  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 2e55599..d312fe1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4334,12 +4334,14 @@ EXPORT_SYMBOL(drm_set_preferred_mode);

•                                          data from a DRM display mode
  

• @frame: HDMI AVI infoframe
• @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)
  

• 			 const struct drm_display_mode *mode,
  

• 			 bool is_hdmi2_sink)
  

{ int err;

@@ -4355,6 +4357,14 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,

frame->video_code = drm_match_cea_mode(mode);

• /*

• * HDMI 1.4 VIC range: 1 <= VIC <= 64 (CEA-861-D) but
  

• * 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)

• frame->video_code = 0;
  

• frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;

  /*

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 06bfbe4..c953927 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -784,7 +784,7 @@ static void hdmi_reg_infoframes(struct hdmi_context *hdata) }

ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,

• 	&hdata->current_mode);
  

• 	&hdata->current_mode, false);
  

  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 86f47e1..d1e7ac5 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -712,7 +712,7 @@ 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);

• drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); 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 ec0779a..170abc4 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -459,11 +459,14 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); 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; union hdmi_infoframe frame; int ret;

  ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,

• 				       adjusted_mode);
  

• 				       adjusted_mode,
  

• 				       is_hdmi2_sink);
  

  if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

@@ -816,6 +819,8 @@ static void hsw_set_infoframes(struct drm_encoder *encoder, { struct drm_i915_private *dev_priv = to_i915(encoder->dev); struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);

• struct drm_hdmi_info *hdmi = &conn_state->connector->display_info.hdmi;
• const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode; i915_reg_t reg = HSW_TVIDEO_DIP_CTL(crtc_state->cpu_transcoder); u32 val = I915_READ(reg);

@@ -839,6 +844,16 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,

intel_hdmi_set_avi_infoframe(encoder, crtc_state); intel_hdmi_set_spd_infoframe(encoder, crtc_state);

• /*

• * CEA-861-F added new VICs for 4k video modes, due to which
  

• * HDMI 2.0 sinks need HDMI IF only for S3D information (Not for
  

• * 4k VICs). So for a HDMI 2.0 sink, if the video mode is not 3D,
  

• * don't bother sending HDMI IF.
  

• */
  

• if (hdmi->scdc.supported && !(mode->flags & DRM_MODE_FLAG_3D_MASK))

• return;
  

• intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);

}

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index f4329d2..737076e 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -996,7 +996,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);
  

• 				       &pipe_config->base.adjusted_mode,
  

• 				       false);
  

  if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return false;

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 0a4ffd7..5c0d024 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -975,7 +975,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); if (err < 0) { dev_err(hdmi->dev, "Failed to get AVI infoframe from mode: %zd\n", err);

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 86c977b..624f5b5 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -85,7 +85,8 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder, if (hdmi_mode && dssdev->driver->set_hdmi_infoframe) { struct hdmi_avi_infoframe avi;

• r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode);
  

• r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode,
  

• 					     false);
  

  if (r == 0) dssdev->driver->set_hdmi_infoframe(dssdev, &avi); }

diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c index aaacac1..770e31f 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 7d9b75e..7149968 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -294,7 +294,7 @@ 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);

• rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);

  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 a59c95a..dbc6a19 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -434,7 +434,7 @@ static int hdmi_avi_infoframe_config(struct sti_hdmi *hdmi)

DRM_DEBUG_DRIVER("\n");

• ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode, false); if (ret < 0) { DRM_ERROR("failed to setup AVI infoframe: %d\n", ret); return ret;

diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index cda0491..718d8db 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -734,7 +734,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 a8f5289..fb2709c 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -1904,7 +1904,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 ed63d4e..406d6d8 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -395,7 +395,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) union hdmi_infoframe frame; int ret;

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c index 0df7366..7e834e3 100644 --- a/drivers/gpu/drm/zte/zx_hdmi.c +++ b/drivers/gpu/drm/zte/zx_hdmi.c @@ -124,7 +124,7 @@ 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);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); 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 7b9f48b..89c0062 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -343,7 +343,8 @@ 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);
  

• 			 const struct drm_display_mode *mode,
  

• 			 bool is_hdmi2_sink);
  

int drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame, const struct drm_display_mode *mode);

Reviewed-by: Neil Armstrong narmstrong@baylibre.com

Regards

Shashank

On 6/30/2017 5:24 PM, Ville Syrjälä wrote:

On Wed, Jun 21, 2017 at 04:03:59PM +0530, Shashank Sharma wrote:

...

HDMI 1.4b support the CEA video modes as per range of CEA-861-D (VIC 1-64). For any other mode, the VIC filed in AVI infoframes should be 0. HDMI 2.0 sinks, support video modes range as per CEA-861-F spec, which is extended to (VIC 1-107).

This patch adds a bool input variable, which indicates if the connected sink is a HDMI 2.0 sink or not. This will make sure that we don't pass a HDMI 2.0 VIC to a HDMI 1.4 sink.

This patch touches all drm drivers, who are callers of this function drm_hdmi_avi_infoframe_from_display_mode but to make sure there is no change in current behavior, is_hdmi2 is kept as false.

In case of I915 driver, this patch:

• checks if the connected display is HDMI 2.0.
• HDMI infoframes carry one of this two type of information:
  • VIC for 4K modes for HDMI 1.4 sinks
  • S3D information for S3D modes
  As CEA-861-F has already defined VICs for 4K videomodes, this patch doesn't allow sending HDMI infoframes for HDMI 2.0 sinks, until the mode is 3D.

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu jose.abreu@synopsys.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Daniel Vetter daniel.vetter@intel.com

PS: This patch touches a few lines in few files, which were already above 80 char, so checkpatch gives 80 char warning again.

• gpu/drm/omapdrm/omap_encoder.c
• gpu/drm/i915/intel_sdvo.c

V2: Rebase, Added r-b from Andrzej V3: Addressed review comment from Ville:

• Do not send VICs in both AVI-IF and HDMI-IF send only one of it.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

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_v8_0.c | 2 +- drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 ++- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/drm_edid.c | 12 +++++++++++- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/i915/intel_hdmi.c | 17 ++++++++++++++++- drivers/gpu/drm/i915/intel_sdvo.c | 3 ++- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 ++- drivers/gpu/drm/radeon/radeon_audio.c | 2 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/tegra/hdmi.c | 2 +- drivers/gpu/drm/tegra/sor.c | 2 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- drivers/gpu/drm/zte/zx_hdmi.c | 2 +- include/drm/drm_edid.h | 3 ++- 21 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c index 3c62c45..4923ddc 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c @@ -1864,7 +1864,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 c8ed0fa..4101684 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c @@ -1848,7 +1848,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 3e90c19..a7f6b32 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c @@ -1747,7 +1747,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 9006578..a083211 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -1097,7 +1097,8 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode,

• 				       false);
  

  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 9b87067..3dc40f6 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -269,7 +269,7 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, if (ret) return;

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index ead1124..0b0c9de 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1317,7 +1317,7 @@ 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);

• drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);

  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 2e55599..d312fe1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4334,12 +4334,14 @@ EXPORT_SYMBOL(drm_set_preferred_mode);

•                                          data from a DRM display mode
  

• @frame: HDMI AVI infoframe
• @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)
  

• 			 const struct drm_display_mode *mode,
  

• 			 bool is_hdmi2_sink)
  

  { int err;

@@ -4355,6 +4357,14 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,

frame->video_code = drm_match_cea_mode(mode);

• /*

• * HDMI 1.4 VIC range: 1 <= VIC <= 64 (CEA-861-D) but
  

• * 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)

• frame->video_code = 0;
  

• frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;

  /*

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 06bfbe4..c953927 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -784,7 +784,7 @@ static void hdmi_reg_infoframes(struct hdmi_context *hdata) }

ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,

• 	&hdata->current_mode);
  

• 	&hdata->current_mode, false);
  

  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 86f47e1..d1e7ac5 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -712,7 +712,7 @@ 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);

• drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); 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 ec0779a..170abc4 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -459,11 +459,14 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); 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; union hdmi_infoframe frame; int ret;

  ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,

• 				       adjusted_mode);
  

• 				       adjusted_mode,
  

• 				       is_hdmi2_sink);
  

  if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

@@ -816,6 +819,8 @@ static void hsw_set_infoframes(struct drm_encoder *encoder, { struct drm_i915_private *dev_priv = to_i915(encoder->dev); struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);

• struct drm_hdmi_info *hdmi = &conn_state->connector->display_info.hdmi;
• const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode; i915_reg_t reg = HSW_TVIDEO_DIP_CTL(crtc_state->cpu_transcoder); u32 val = I915_READ(reg);

@@ -839,6 +844,16 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,

intel_hdmi_set_avi_infoframe(encoder, crtc_state); intel_hdmi_set_spd_infoframe(encoder, crtc_state);

• /*

• * CEA-861-F added new VICs for 4k video modes, due to which
  

• * HDMI 2.0 sinks need HDMI IF only for S3D information (Not for
  

• * 4k VICs). So for a HDMI 2.0 sink, if the video mode is not 3D,
  

• * don't bother sending HDMI IF.
  

• */
  

• if (hdmi->scdc.supported && !(mode->flags & DRM_MODE_FLAG_3D_MASK))

• return;
  

That doesn't look like the rigth place for this check.

Also I think we still want to send the vendor specific infoframe always, if for no other reason that to get extra testing for the code. We just need to select whether we should be sending the old or the new one.

I think in the previous review comments, this is what you asked to do: send HDMI 2.0 VIC only in one place, either in AVI IF or HDMI IF, not both.

Now, HDMI IF contains only two piece of information (from the code): frame->vic = vic (for HDMI 1.4 4k HDMI modes) or frame->s3d_strucrt = s3d_structure_from_display_mode()

So if its a HDMI 2.0 monitor && we don't want to send s3d_info, then there is nothing in the frame right ?

- Shashank

...

• intel_hdmi_set_hdmi_infoframe(encoder, crtc_state); }

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index f4329d2..737076e 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -996,7 +996,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);
  

• 				       &pipe_config->base.adjusted_mode,
  

• 				       false);
  

  if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return false;

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 0a4ffd7..5c0d024 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -975,7 +975,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); if (err < 0) { dev_err(hdmi->dev, "Failed to get AVI infoframe from mode: %zd\n", err);

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 86c977b..624f5b5 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -85,7 +85,8 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder, if (hdmi_mode && dssdev->driver->set_hdmi_infoframe) { struct hdmi_avi_infoframe avi;

• r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode);
  

• r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode,
  

• 					     false);
  

  if (r == 0) dssdev->driver->set_hdmi_infoframe(dssdev, &avi); }

diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c index aaacac1..770e31f 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 7d9b75e..7149968 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -294,7 +294,7 @@ 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);

• rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);

  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 a59c95a..dbc6a19 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -434,7 +434,7 @@ static int hdmi_avi_infoframe_config(struct sti_hdmi *hdmi)

DRM_DEBUG_DRIVER("\n");

• ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode, false); if (ret < 0) { DRM_ERROR("failed to setup AVI infoframe: %d\n", ret); return ret;

diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index cda0491..718d8db 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -734,7 +734,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 a8f5289..fb2709c 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -1904,7 +1904,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 ed63d4e..406d6d8 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -395,7 +395,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) union hdmi_infoframe frame; int ret;

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c index 0df7366..7e834e3 100644 --- a/drivers/gpu/drm/zte/zx_hdmi.c +++ b/drivers/gpu/drm/zte/zx_hdmi.c @@ -124,7 +124,7 @@ 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);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); 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 7b9f48b..89c0062 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -343,7 +343,8 @@ 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);
  

• 			 const struct drm_display_mode *mode,
  

• 			 bool is_hdmi2_sink);
  

  int drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame, const struct drm_display_mode *mode);

-- 2.7.4

Regards

Shashank

On 7/3/2017 3:27 PM, Ville Syrjälä wrote:

On Mon, Jul 03, 2017 at 02:36:58PM +0530, Sharma, Shashank wrote:

...

Regards

Shashank

On 6/30/2017 5:24 PM, Ville Syrjälä wrote:

...

On Wed, Jun 21, 2017 at 04:03:59PM +0530, Shashank Sharma wrote:

...

HDMI 1.4b support the CEA video modes as per range of CEA-861-D (VIC 1-64). For any other mode, the VIC filed in AVI infoframes should be 0. HDMI 2.0 sinks, support video modes range as per CEA-861-F spec, which is extended to (VIC 1-107).

This patch adds a bool input variable, which indicates if the connected sink is a HDMI 2.0 sink or not. This will make sure that we don't pass a HDMI 2.0 VIC to a HDMI 1.4 sink.

This patch touches all drm drivers, who are callers of this function drm_hdmi_avi_infoframe_from_display_mode but to make sure there is no change in current behavior, is_hdmi2 is kept as false.

In case of I915 driver, this patch:

• checks if the connected display is HDMI 2.0.
• HDMI infoframes carry one of this two type of information:
  • VIC for 4K modes for HDMI 1.4 sinks
  • S3D information for S3D modes
  As CEA-861-F has already defined VICs for 4K videomodes, this patch doesn't allow sending HDMI infoframes for HDMI 2.0 sinks, until the mode is 3D.

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu jose.abreu@synopsys.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Daniel Vetter daniel.vetter@intel.com

PS: This patch touches a few lines in few files, which were already above 80 char, so checkpatch gives 80 char warning again.

• gpu/drm/omapdrm/omap_encoder.c
• gpu/drm/i915/intel_sdvo.c

V2: Rebase, Added r-b from Andrzej V3: Addressed review comment from Ville:

• Do not send VICs in both AVI-IF and HDMI-IF send only one of it.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

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_v8_0.c | 2 +- drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 ++- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/drm_edid.c | 12 +++++++++++- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/i915/intel_hdmi.c | 17 ++++++++++++++++- drivers/gpu/drm/i915/intel_sdvo.c | 3 ++- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 ++- drivers/gpu/drm/radeon/radeon_audio.c | 2 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/tegra/hdmi.c | 2 +- drivers/gpu/drm/tegra/sor.c | 2 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- drivers/gpu/drm/zte/zx_hdmi.c | 2 +- include/drm/drm_edid.h | 3 ++- 21 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c index 3c62c45..4923ddc 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c @@ -1864,7 +1864,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 c8ed0fa..4101684 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c @@ -1848,7 +1848,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 3e90c19..a7f6b32 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c @@ -1747,7 +1747,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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 9006578..a083211 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -1097,7 +1097,8 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode,

• 				       false);
  

  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 9b87067..3dc40f6 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -269,7 +269,7 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, if (ret) return;

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index ead1124..0b0c9de 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1317,7 +1317,7 @@ 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);

• drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);

  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 2e55599..d312fe1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4334,12 +4334,14 @@ EXPORT_SYMBOL(drm_set_preferred_mode); * data from a DRM display mode * @frame: HDMI AVI infoframe * @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)
  

• 			 const struct drm_display_mode *mode,
  

• 			 bool is_hdmi2_sink)
  

  { int err;

@@ -4355,6 +4357,14 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,

frame->video_code = drm_match_cea_mode(mode);

• /*

• * HDMI 1.4 VIC range: 1 <= VIC <= 64 (CEA-861-D) but
  

• * 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)

• frame->video_code = 0;
  

• frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;

  /*

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 06bfbe4..c953927 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -784,7 +784,7 @@ static void hdmi_reg_infoframes(struct hdmi_context *hdata) }

ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,

• 	&hdata->current_mode);
  

• 	&hdata->current_mode, false);
  

  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 86f47e1..d1e7ac5 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -712,7 +712,7 @@ 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);

• drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); 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 ec0779a..170abc4 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -459,11 +459,14 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); 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; union hdmi_infoframe frame; int ret;

  ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,

• 				       adjusted_mode);
  

• 				       adjusted_mode,
  

• 				       is_hdmi2_sink);
  

  if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

@@ -816,6 +819,8 @@ static void hsw_set_infoframes(struct drm_encoder *encoder, { struct drm_i915_private *dev_priv = to_i915(encoder->dev); struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);

• struct drm_hdmi_info *hdmi = &conn_state->connector->display_info.hdmi;
• const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode; i915_reg_t reg = HSW_TVIDEO_DIP_CTL(crtc_state->cpu_transcoder); u32 val = I915_READ(reg);

@@ -839,6 +844,16 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,

intel_hdmi_set_avi_infoframe(encoder, crtc_state);
intel_hdmi_set_spd_infoframe(encoder, crtc_state);

• /*

• * CEA-861-F added new VICs for 4k video modes, due to which
  

• * HDMI 2.0 sinks need HDMI IF only for S3D information (Not for
  

• * 4k VICs). So for a HDMI 2.0 sink, if the video mode is not 3D,
  

• * don't bother sending HDMI IF.
  

• */
  

• if (hdmi->scdc.supported && !(mode->flags & DRM_MODE_FLAG_3D_MASK))

• return;
  

That doesn't look like the rigth place for this check.

Also I think we still want to send the vendor specific infoframe always, if for no other reason that to get extra testing for the code. We just need to select whether we should be sending the old or the new one.

I think in the previous review comments, this is what you asked to do: send HDMI 2.0 VIC only in one place, either in AVI IF or HDMI IF, not both.

Yes, the VIC. I didn't say anything about not sending any infoframe.

...

Now, HDMI IF contains only two piece of information (from the code): frame->vic = vic (for HDMI 1.4 4k HDMI modes) or frame->s3d_strucrt = s3d_structure_from_display_mode()

So if its a HDMI 2.0 monitor && we don't want to send s3d_info, then there is nothing in the frame right ?

Not sending any infoframe means a change in current behaviour which we may not want. It also means we don't treat HDMI 2.0 and 1.x sinks equally since for 1.x sinks we always send the infoframe.

And appendix F of the spec seems to be telling us that there are sinks out there that don't properly switch from 3D to 2D when the source stops sending the old infoframe. Hence it seems like a good idea to always send it. They speak about HDMI 1.4 sinks specifically, but I wouldn't be surprised if similar problems crop up in 2.0 sinks as well.

In fact I think it would have been nice if they had just totally replaced the old infoframe with the new one, and allowed us to always just send the new infoframe. But the spec specifically says that we have to use the old infoframe if possible :(

So should I remove this check, and keep things going as it was ? - Shashank

...

• Shashank

...

...

• intel_hdmi_set_hdmi_infoframe(encoder, crtc_state); }

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index f4329d2..737076e 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -996,7 +996,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);
  

• 				       &pipe_config->base.adjusted_mode,
  

• 				       false);
  

  if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return false;

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 0a4ffd7..5c0d024 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -975,7 +975,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); if (err < 0) { dev_err(hdmi->dev, "Failed to get AVI infoframe from mode: %zd\n", err);

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 86c977b..624f5b5 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -85,7 +85,8 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder, if (hdmi_mode && dssdev->driver->set_hdmi_infoframe) { struct hdmi_avi_infoframe avi;

• r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode);
  

• r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode,
  

• 					     false);
  if (r == 0)
  	dssdev->driver->set_hdmi_infoframe(dssdev, &avi);
  

  }

diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c index aaacac1..770e31f 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 7d9b75e..7149968 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -294,7 +294,7 @@ 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);

• rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false);

  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 a59c95a..dbc6a19 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -434,7 +434,7 @@ static int hdmi_avi_infoframe_config(struct sti_hdmi *hdmi)

DRM_DEBUG_DRIVER("\n");

• ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe, mode, false); if (ret < 0) { DRM_ERROR("failed to setup AVI infoframe: %d\n", ret); return ret;

diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index cda0491..718d8db 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -734,7 +734,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 a8f5289..fb2709c 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -1904,7 +1904,7 @@ 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);

• err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); 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 ed63d4e..406d6d8 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -395,7 +395,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) union hdmi_infoframe frame; int ret;

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return;

diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c index 0df7366..7e834e3 100644 --- a/drivers/gpu/drm/zte/zx_hdmi.c +++ b/drivers/gpu/drm/zte/zx_hdmi.c @@ -124,7 +124,7 @@ 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);

• ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, mode, false); 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 7b9f48b..89c0062 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -343,7 +343,8 @@ 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);
  

• 			 const struct drm_display_mode *mode,
  

• 			 bool is_hdmi2_sink);
  

  int drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame, const struct drm_display_mode *mode);

-- 2.7.4

On 06/21/2017 12:34 PM, Shashank Sharma wrote:

This patch adds a bool variable (ycbcr_420_allowed) in the drm connector structure. While handling the EDID from HDMI 2.0 sinks, its important to know if the source is capable of handling YCBCR 420 outputs or not, so that a lot of work can be done/bypassed based on this information. One such example is adding YCBCR420 only modes.

If the driver knows that this source is not HDMI 2.0 capable, it will not add YCBCR420-only modes while adding EDID modes, and will prevent any runtime modeset failures.

This variable will be initialized from I915 driver in the next patch and will be used in the EDID handling for HDMI 2.0 specific features, in this same series.

V3: introduced the new variable V4: changed variable name from is_hdmi2_src to ycbcr_420_allowed (Ville)

Signed-off-by: Shashank Sharma shashank.sharma@intel.com

include/drm/drm_connector.h | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index d8bb25f..7493fd3 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -728,6 +728,15 @@ struct drm_connector { bool interlace_allowed; bool doublescan_allowed; bool stereo_allowed;

• /**

• * @ycbcr_420_allowed : This bool indicates if this connector is
  

• * capable of handling YCBCR 420 output. While parsing the EDID
  

• * blocks, its very helpful to know, if the source is capable of
  

• * handling YCBCR 420 outputs.
  

• */
  

• bool ycbcr_420_allowed;
• /**
  • @registered: Is this connector exposed (registered) with userspace?
  • Protected by @mutex.

Reviewed-by: Neil Armstrong narmstrong@baylibre.com

On 06/21/2017 12:34 PM, Shashank Sharma wrote:

CEA-861-F specs defines new video modes to be used with HDMI 2.0 EDIDs. The VIC range has been extended from 1-64 to 1-107.

Our existing CEA modedb contains only 64 modes (VIC=1 to VIC=64). Now to be able to parse new CEA modes using the existing methods, we have to complete the modedb (VIC=65 onwards).

This patch adds:

• Timings for existing CEA video modes (from VIC=65 till VIC=92)
• Newly added 4k modes (from VIC=93 to VIC=107).

The patch was originaly discussed and reviewed here: https://patchwork.freedesktop.org/patch/135810/

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu Jose.Abreu@synopsys.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Harry Wentland harry.wentland@amd.com

V2: Rebase V3: Rebase V4: Added native bit handling as per CEA-861-F spec (Ville)

Reviewed-by: Jose Abreu Jose.Abreu@synopsys.com Reviewed-by: Alex Deucher alexander.deucher@amd.com Acked-by: Harry Wentland harry.wentland@amd.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

drivers/gpu/drm/drm_edid.c | 227 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 226 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d312fe1..e2d1b30 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1006,6 +1006,221 @@ static const struct drm_display_mode edid_cea_modes[] = { 2492, 2640, 0, 1080, 1084, 1089, 1125, 0, DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },

• /* 65 - 1280x720@24Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 59400, 1280, 3040,

•    3080, 3300, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 66 - 1280x720@25Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 74250, 1280, 3700,

•    3740, 3960, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 67 - 1280x720@30Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 74250, 1280, 3040,

•    3080, 3300, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 68 - 1280x720@50Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 74250, 1280, 1720,

•    1760, 1980, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 69 - 1280x720@60Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 74250, 1280, 1390,

•    1430, 1650, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 70 - 1280x720@100Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 148500, 1280, 1720,

•    1760, 1980, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 71 - 1280x720@120Hz */
• { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 148500, 1280, 1390,

•    1430, 1650, 0, 720, 725, 730, 750, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 120, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 72 - 1920x1080@24Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2558,

•    2602, 2750, 0, 1080, 1084, 1089, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 73 - 1920x1080@25Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2448,

•    2492, 2640, 0, 1080, 1084, 1089, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 74 - 1920x1080@30Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 74250, 1920, 2008,

•    2052, 2200, 0, 1080, 1084, 1089, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 75 - 1920x1080@50Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 148500, 1920, 2448,

•    2492, 2640, 0, 1080, 1084, 1089, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 76 - 1920x1080@60Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 148500, 1920, 2008,

•    2052, 2200, 0, 1080, 1084, 1089, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

•  .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 77 - 1920x1080@100Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 297000, 1920, 2448,

•    2492, 2640, 0, 1080, 1084, 1094, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 78 - 1920x1080@120Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 297000, 1920, 2008,

•    2052, 2200, 0, 1080, 1084, 1089, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 120, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

• /* 79 - 1680x720@24Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 59400, 1680, 3040,

• 3080, 3300, 0, 720, 725, 730, 750, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 80 - 1680x720@25Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 59400, 1680, 2908,

• 2948, 3168, 0, 720, 725, 730, 750, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 81 - 1680x720@30Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 59400, 1680, 2380,

• 2420, 2640, 0, 720, 725, 730, 750, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 82 - 1680x720@50Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 82500, 1680, 1940,

• 1980, 2200, 0, 720, 725, 730, 750, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 83 - 1680x720@60Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 99000, 1680, 1940,

• 1980, 2200, 0, 720, 725, 730, 750, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 84 - 1680x720@100Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 165000, 1680, 1740,

• 1780, 2000, 0, 720, 725, 730, 825, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 85 - 1680x720@120Hz */
• { DRM_MODE("1680x720", DRM_MODE_TYPE_DRIVER, 198000, 1680, 1740,

• 1780, 2000, 0, 720, 725, 730, 825, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 120, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 86 - 2560x1080@24Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 99000, 2560, 3558,

• 3602, 3750, 0, 1080, 1084, 1089, 1100, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 87 - 2560x1080@25Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 90000, 2560, 3008,

• 3052, 3200, 0, 1080, 1084, 1089, 1125, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 88 - 2560x1080@30Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 118800, 2560, 3328,

• 3372, 3520, 0, 1080, 1084, 1089, 1125, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 89 - 2560x1080@50Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 185625, 2560, 3108,

• 3152, 3300, 0, 1080, 1084, 1089, 1125, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 90 - 2560x1080@60Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 198000, 2560, 2808,

• 2852, 3000, 0, 1080, 1084, 1089, 1100, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 91 - 2560x1080@100Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 371250, 2560, 2778,

• 2822, 2970, 0, 1080, 1084, 1089, 1250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 92 - 2560x1080@120Hz */
• { DRM_MODE("2560x1080", DRM_MODE_TYPE_DRIVER, 495000, 2560, 3108,

• 3152, 3300, 0, 1080, 1084, 1089, 1250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 120, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
• /* 93 - 3840x2160p@24Hz 16:9 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 5116,

• 5204, 5500, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9,},
• /* 94 - 3840x2160p@25Hz 16:9 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 4896,

• 4984, 5280, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9},
• /* 95 - 3840x2160p@30Hz 16:9 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 4016,

• 4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9},
• /* 96 - 3840x2160p@50Hz 16:9 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 594000, 3840, 4896,

• 4984, 5280, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9},
• /* 97 - 3840x2160p@60Hz 16:9 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 594000, 3840, 4016,

• 4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9},
• /* 98 - 4096x2160p@24Hz 256:135 */
• { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 297000, 4096, 5116,

• 5204, 5500, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135},
• /* 99 - 4096x2160p@25Hz 256:135 */
• { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 297000, 4096, 5064,

• 5152, 5280, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135},
• /* 100 - 4096x2160p@30Hz 256:135 */
• { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 297000, 4096, 4184,

• 4272, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135},
• /* 101 - 4096x2160p@50Hz 256:135 */
• { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 594000, 4096, 5064,

• 5152, 5280, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135},
• /* 102 - 4096x2160p@60Hz 256:135 */
• { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 594000, 4096, 4184,

• 4272, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135},
• /* 103 - 3840x2160p@24Hz 64:27 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 5116,

• 5204, 5500, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27},
• /* 104 - 3840x2160p@25Hz 64:27 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 4016,

• 4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27},
• /* 105 - 3840x2160p@30Hz 64:27 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 4016,

• 4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27},
• /* 106 - 3840x2160p@50Hz 64:27 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 594000, 3840, 4896,

• 4984, 5280, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 50, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27},
• /* 107 - 3840x2160p@60Hz 64:27 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 594000, 3840, 4016,

• 4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27},

};

/* @@ -2902,6 +3117,16 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) return modes; }

+static u8 svd_to_vic(u8 svd) +{

• /* 0-6 bit vic, 7th bit native mode indicator */
• if (((svd >= 1) && (svd <= 64)) || ((svd >= 129) && (svd <= 192)))

• return svd & 127;
  

• return svd;

+}

static struct drm_display_mode * drm_display_mode_from_vic_index(struct drm_connector *connector, const u8 *video_db, u8 video_len, @@ -2915,7 +3140,7 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, return NULL;

/* CEA modes are numbered 1..127 */

• vic = (video_db[video_index] & 127);

• vic = svd_to_vic(video_db[video_index]); if (!drm_valid_cea_vic(vic)) return NULL;

Reviewed-by: Neil Armstrong narmstrong@baylibre.com

Regards

Shashank

On 6/27/2017 5:02 PM, Ville Syrjälä wrote:

On Wed, Jun 21, 2017 at 04:04:01PM +0530, Shashank Sharma wrote:

...

CEA-861-F specs defines new video modes to be used with HDMI 2.0 EDIDs. The VIC range has been extended from 1-64 to 1-107.

Our existing CEA modedb contains only 64 modes (VIC=1 to VIC=64). Now to be able to parse new CEA modes using the existing methods, we have to complete the modedb (VIC=65 onwards).

This patch adds:

• Timings for existing CEA video modes (from VIC=65 till VIC=92)
• Newly added 4k modes (from VIC=93 to VIC=107).

The patch was originaly discussed and reviewed here: https://patchwork.freedesktop.org/patch/135810/

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu Jose.Abreu@synopsys.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Harry Wentland harry.wentland@amd.com

V2: Rebase V3: Rebase V4: Added native bit handling as per CEA-861-F spec (Ville)

Reviewed-by: Jose Abreu Jose.Abreu@synopsys.com Reviewed-by: Alex Deucher alexander.deucher@amd.com Acked-by: Harry Wentland harry.wentland@amd.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

drivers/gpu/drm/drm_edid.c | 227 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 226 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d312fe1..e2d1b30 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1006,6 +1006,221 @@ static const struct drm_display_mode edid_cea_modes[] = { 2492, 2640, 0, 1080, 1084, 1089, 1125, 0, DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },

...

...

• /* 77 - 1920x1080@100Hz */
• { DRM_MODE("1920x1080", DRM_MODE_TYPE_DRIVER, 297000, 1920, 2448,

•    2492, 2640, 0, 1080, 1084, 1094, 1125, 0,
  

•    DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 100, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27, },
  

...

...

• /* 104 - 3840x2160p@25Hz 64:27 */
• { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000, 3840, 4016,

• 4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
  

• DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
  

• .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27},

https://lists.freedesktop.org/archives/dri-devel/2016-November/122669.html

Ok, looks like I missed this one while reformatting the patches, thanks for reminder. Will cross check as per your script output.

...

@@ -2902,6 +3117,16 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) return modes; }

+static u8 svd_to_vic(u8 svd) +{

• /* 0-6 bit vic, 7th bit native mode indicator */
• if (((svd >= 1) && (svd <= 64)) || ((svd >= 129) && (svd <= 192)))

Too many parentheses. lgtm otherwise.

Got it.

- Shashank

...

• return svd & 127;
  

• return svd;

+}

• static struct drm_display_mode * drm_display_mode_from_vic_index(struct drm_connector *connector, const u8 *video_db, u8 video_len,

@@ -2915,7 +3140,7 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, return NULL;

/* CEA modes are numbered 1..127 */

• vic = (video_db[video_index] & 127);

• vic = svd_to_vic(video_db[video_index]); if (!drm_valid_cea_vic(vic)) return NULL;

-- 2.7.4

On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:

HDMI 2.0 spec adds support for YCBCR420 sub-sampled output. CEA-861-F adds two new blocks in EDID's CEA extension blocks, to provide information about sink's YCBCR420 output capabilities.

These blocks are:

• YCBCR420vdb(YCBCR 420 video data block):

This block contains VICs of video modes, which can be sopported only in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal SVD block, valid for YCBCR420 modes only.

• YCBCR420cmdb(YCBCR 420 capability map data block):

This block gives information about video modes which can support YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This block contains a bitmap index of normal svd videomodes, which can support YCBCR420 output too. So if bit 0 from first vcb byte is set, first video mode in the svd list can support YCBCR420 output too. Bit 1 means second video mode from svd list can support YCBCR420 output too, and so on.

This patch adds two bitmaps in display's hdmi_info structure, one each for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch adds:

• VDB modes (YCBCR 420 only modes) in connector's mode list, also makes an entry in the vdb_bitmap per vic.
• VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu joabreu@synopsys.com Cc: Emil Velikov emil.l.velikov@gmail.com

V2: Addressed Review comments from Emil: - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit. - Use the suggested method for updating dbmap. - Add documentation for YCBCR420_vcb_map to fix kbuild warning.

Review comments from Ville:
- Do not expose the YCBCR420 flags in uabi layer, keep it internal.
- Save a map of YCBCR420 modes for future reference.
- Check db length before trying to parse extended tag.
- Add a warning if there are > 64 modes in capability map block.
- Use y420cmdb in function names and macros while dealing with vcb
  to be aligned with spec.
- Move the display information parsing block ahead of mode parsing
  blocks.

V3: Addressed design/review comments from Ville - Do not add flags in video modes, else we have to expose them to user - There should not be a UABI change, and kernel should detect the choice of the output based on type of mode, and the bitmaps. - Use standard bitops from kernel bitmap header, instead of calculating bit positions manually.

V4: Addressed review comments from Ville: - s/ycbcr_420_vdb/y420vdb - s/ycbcr_420_vcb/y420cmdb - Be less verbose on description of do_y420vdb_modes - Move newmode variable in the loop scope. - Use svd_to_vic() to get a VIC, instead of 0x7f - Remove bitmap description for CMDB modes & VDB modes - Dont add connector->ycbcr_420_allowed check for cmdb modes - Remove 'len' variable, in is_y420cmdb function, which is used only once - Add length check in is_y420vdb function - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap - Do not add print about YCBCR 420 modes - Fix indentation in few places - Move ycbcr420_dc_modes in next patch, where its used - Add a separate patch for movement of drm_add_display_info()

Signed-off-by: Shashank Sharma shashank.sharma@intel.com

drivers/gpu/drm/drm_edid.c | 157 +++++++++++++++++++++++++++++++++++++++++++- include/drm/drm_connector.h | 20 ++++++ 2 files changed, 174 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index e2d1b30..8c7e73b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define VIDEO_BLOCK 0x02 #define VENDOR_BLOCK 0x03 #define SPEAKER_BLOCK 0x04 -#define VIDEO_CAPABILITY_BLOCK 0x07 +#define VIDEO_CAPABILITY_BLOCK 0x07 +#define VIDEO_DATA_BLOCK_420 0x0E +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F #define EDID_BASIC_AUDIO (1 << 6) #define EDID_CEA_YCRCB444 (1 << 5) #define EDID_CEA_YCRCB422 (1 << 4) @@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, return newmode; }

+/*

• • do_y420vdb_modes - Parse YCBCR 420 only modes
• • @connector: connector corresponding to the HDMI sink
• • @svds: start of the data block of CEA YCBCR 420 VDB
• • @len: length of the CEA YCBCR 420 VDB
• • Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
• • which contains modes which can be supported in YCBCR 420
• • output format only.
• */

+static int do_y420vdb_modes(struct drm_connector *connector,

• 		   const u8 *svds, u8 svds_len)
  

+{

• int modes = 0, i;
• struct drm_device *dev = connector->dev;
• struct drm_display_info *info = &connector->display_info;
• struct drm_hdmi_info *hdmi = &info->hdmi;
• for (i = 0; i < svds_len; i++) {

• u8 vic = svd_to_vic(svds[i]);
  

• struct drm_display_mode *newmode;
  

• newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
  

• if (!newmode)
  

• 	break;
  

• bitmap_set(hdmi->y420_vdb_modes, vic, 1);
  

• drm_mode_probed_add(connector, newmode);
  

• modes++;
  

• }
• if (modes > 0)

• info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
  

• return modes;

+}

+/*

• • drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
• • @connector: connector corresponding to the HDMI sink
• • @vic: CEA vic for the video mode to be added in the map
• • Makes an entry for a videomode in the YCBCR 420 bitmap
• */

+static void +drm_add_cmdb_modes(struct drm_connector *connector, u8 svd) +{

• u8 vic = svd_to_vic(svd);
• struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
• bitmap_set(hdmi->y420_cmdb_modes, vic, 1);

+}

static int do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len) { int i, modes = 0;

• struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;

  for (i = 0; i < len; i++) { struct drm_display_mode *mode; mode = drm_display_mode_from_vic_index(connector, db, len, i); if (mode) {

• 	/*
  

• 	 * YCBCR420 capability block contains a bitmap which
  

• 	 * gives the index of CEA modes from CEA VDB, which
  

• 	 * can support YCBCR 420 sampling output also (apart
  

• 	 * from RGB/YCBCR444 etc).
  

• 	 * For example, if the bit 0 in bitmap is set,
  

• 	 * first mode in VDB can support YCBCR420 output too.
  

• 	 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
  

• 	 */
  

• 	if (test_bit(i, &hdmi->y420_cmdb_map))
  

• 		drm_add_cmdb_modes(connector, db[i]);
  

• drm_mode_probed_add(connector, mode);
  modes++;
  

  }

@@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len, }

static int +cea_db_extended_tag(const u8 *db) +{

• return db[1];

+}

+static int cea_db_payload_len(const u8 *db) { return db[0] & 0x1f; @@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) return oui == HDMI_FORUM_IEEE_OUI; }

+static bool cea_db_is_y420cmdb(const u8 *db) +{

• if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)

Still would like to see that patch to clean up the current extended tag usage...

• return false;
  

• if (!cea_db_payload_len(db))

• return false;
  

• if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)

• return false;
  

• return true;

+}

+static bool cea_db_is_y420vdb(const u8 *db) +{

• if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)

• return false;
  

• if (!cea_db_payload_len(db))

• return false;
  

• if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)

• return false;
  

• return true;

+}

#define for_each_cea_db(cea, i, start, end) \ for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)

+static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,

• 		     const u8 *db)
  

+{

• struct drm_display_info *info = &connector->display_info;
• struct drm_hdmi_info *hdmi = &info->hdmi;
• u8 map_len = cea_db_payload_len(db) - 1;
• u8 count;
• u64 map = 0;
• if (map_len == 0) {

• /* All CEA modes support ycbcr420 sampling also.*/
  

• hdmi->y420_cmdb_map = U64_MAX;
  

• info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
  

• return;
  

• }
• /*

• * This map indicates which of the existing CEA block modes
  

• * from VDB can support YCBCR420 output too. So if bit=0 is
  

• * set, first mode from VDB can support YCBCR420 output too.
  

• * We will parse and keep this map, before parsing VDB itself
  

• * to avoid going through the same block again and again.
  

• *
  

• * Spec is not clear about max possible size of this block.
  

• * Clamping max bitmap block size at 8 bytes. Every byte can
  

• * address 8 CEA modes, in this way this map can address
  

• * 8*8 = first 64 SVDs.
  

• */
  

• if (WARN_ON_ONCE(map_len > 8))

• map_len = 8;
  

• for (count = 0; count < map_len; count++)

• map |= (u64)db[2 + count] << (8 * count);
  

• if (map)

• info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
  

• hdmi->y420_cmdb_map = map;

+}

static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { @@ -3522,10 +3663,17 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) video = db + 1; video_len = dbl; modes += do_cea_modes(connector, video, dbl);

• 	}
  

• 	else if (cea_db_is_hdmi_vsdb(db)) {
  

• 	} else if (cea_db_is_hdmi_vsdb(db)) {
  	hdmi = db;
  	hdmi_len = dbl;
  

• 	} else if (cea_db_is_y420vdb(db) &&
  

• 		   connector->ycbcr_420_allowed) {
  

• 		const u8 *vdb420 = &db[2];
  

• 		/* Add 4:2:0(only) modes present in EDID */
  

• 		modes += do_y420vdb_modes(connector,
  

• 					  vdb420,
  

• 					  dbl - 1);
  }
  

  } }

@@ -4206,6 +4354,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, drm_parse_hdmi_vsdb_video(connector, db); if (cea_db_is_hdmi_forum_vsdb(db)) drm_parse_hdmi_forum_vsdb(connector, db);

• if (cea_db_is_y420cmdb(db))
  

• 	drm_parse_y420cmdb_bitmap(connector, db);
  

  }

}

@@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) num_modes += add_cea_modes(connector, edid); num_modes += add_alternate_cea_modes(connector, edid); num_modes += add_displayid_detailed_modes(connector, edid);

• if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF) num_modes += add_inferred_modes(connector, edid);

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 7493fd3..ce2212d 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -137,6 +137,25 @@ struct drm_scdc { struct drm_hdmi_info { /** @scdc: sink's scdc support and capabilities */ struct drm_scdc scdc;

• /**

• * @y420_vdb_modes: bitmap of modes which can support ycbcr420
  

• * output only (not normal RGB/YCBCR444/422 outputs). There are total
  

• * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
  

• * upto 128 VICs;
  

• */
  

• unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
• /**

• * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
  

• * output also, along with normal HDMI outputs. There are total 107
  

• * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
  

• * 128 VICs;
  

• */
  

• unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
• /** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
• unsigned long y420_cmdb_map;

u64, otherwise it's busted on 32bit machines.

Mostl this looks pretty good. I think the main problem is the lack of mode validation that would reject the 4:2:0 only modes if not supported by the driver/device. I guess that's in a later patch? If so we need to reorder the series a bit to avoid regeressions.

};

/** @@ -200,6 +219,7 @@ struct drm_display_info { #define DRM_COLOR_FORMAT_RGB444 (1<<0) #define DRM_COLOR_FORMAT_YCRCB444 (1<<1) #define DRM_COLOR_FORMAT_YCRCB422 (1<<2) +#define DRM_COLOR_FORMAT_YCRCB420 (1<<3)

/** * @color_formats: HDMI Color formats, selects between RGB and YCrCb -- 2.7.4

On Fri, Jun 30, 2017 at 10:47:48AM +0530, Sharma, Shashank wrote:

Regards

Shashank

On 6/27/2017 5:22 PM, Ville Syrjälä wrote:

...

On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:

...

HDMI 2.0 spec adds support for YCBCR420 sub-sampled output. CEA-861-F adds two new blocks in EDID's CEA extension blocks, to provide information about sink's YCBCR420 output capabilities.

These blocks are:

• YCBCR420vdb(YCBCR 420 video data block):

This block contains VICs of video modes, which can be sopported only in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal SVD block, valid for YCBCR420 modes only.

• YCBCR420cmdb(YCBCR 420 capability map data block):

This block gives information about video modes which can support YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This block contains a bitmap index of normal svd videomodes, which can support YCBCR420 output too. So if bit 0 from first vcb byte is set, first video mode in the svd list can support YCBCR420 output too. Bit 1 means second video mode from svd list can support YCBCR420 output too, and so on.

This patch adds two bitmaps in display's hdmi_info structure, one each for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch adds:

• VDB modes (YCBCR 420 only modes) in connector's mode list, also makes an entry in the vdb_bitmap per vic.
• VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu joabreu@synopsys.com Cc: Emil Velikov emil.l.velikov@gmail.com

V2: Addressed Review comments from Emil: - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit. - Use the suggested method for updating dbmap. - Add documentation for YCBCR420_vcb_map to fix kbuild warning.

 Review comments from Ville:
 - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
 - Save a map of YCBCR420 modes for future reference.
 - Check db length before trying to parse extended tag.
 - Add a warning if there are > 64 modes in capability map block.
 - Use y420cmdb in function names and macros while dealing with vcb
   to be aligned with spec.
 - Move the display information parsing block ahead of mode parsing
   blocks.

V3: Addressed design/review comments from Ville - Do not add flags in video modes, else we have to expose them to user - There should not be a UABI change, and kernel should detect the choice of the output based on type of mode, and the bitmaps. - Use standard bitops from kernel bitmap header, instead of calculating bit positions manually.

V4: Addressed review comments from Ville: - s/ycbcr_420_vdb/y420vdb - s/ycbcr_420_vcb/y420cmdb - Be less verbose on description of do_y420vdb_modes - Move newmode variable in the loop scope. - Use svd_to_vic() to get a VIC, instead of 0x7f - Remove bitmap description for CMDB modes & VDB modes - Dont add connector->ycbcr_420_allowed check for cmdb modes - Remove 'len' variable, in is_y420cmdb function, which is used only once - Add length check in is_y420vdb function - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap - Do not add print about YCBCR 420 modes - Fix indentation in few places - Move ycbcr420_dc_modes in next patch, where its used - Add a separate patch for movement of drm_add_display_info()

Signed-off-by: Shashank Sharma shashank.sharma@intel.com

drivers/gpu/drm/drm_edid.c | 157 +++++++++++++++++++++++++++++++++++++++++++- include/drm/drm_connector.h | 20 ++++++ 2 files changed, 174 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index e2d1b30..8c7e73b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define VIDEO_BLOCK 0x02 #define VENDOR_BLOCK 0x03 #define SPEAKER_BLOCK 0x04 -#define VIDEO_CAPABILITY_BLOCK 0x07 +#define VIDEO_CAPABILITY_BLOCK 0x07 +#define VIDEO_DATA_BLOCK_420 0x0E +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F #define EDID_BASIC_AUDIO (1 << 6) #define EDID_CEA_YCRCB444 (1 << 5) #define EDID_CEA_YCRCB422 (1 << 4) @@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, return newmode; }

+/*

• • do_y420vdb_modes - Parse YCBCR 420 only modes
• • @connector: connector corresponding to the HDMI sink
• • @svds: start of the data block of CEA YCBCR 420 VDB
• • @len: length of the CEA YCBCR 420 VDB
• • Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
• • which contains modes which can be supported in YCBCR 420
• • output format only.
• */

+static int do_y420vdb_modes(struct drm_connector *connector,

• 		   const u8 *svds, u8 svds_len)
  

+{

• int modes = 0, i;
• struct drm_device *dev = connector->dev;
• struct drm_display_info *info = &connector->display_info;
• struct drm_hdmi_info *hdmi = &info->hdmi;
• for (i = 0; i < svds_len; i++) {

• u8 vic = svd_to_vic(svds[i]);
  

• struct drm_display_mode *newmode;
  

• newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
  

• if (!newmode)
  

• 	break;
  

• bitmap_set(hdmi->y420_vdb_modes, vic, 1);
  

• drm_mode_probed_add(connector, newmode);
  

• modes++;
  

• }
• if (modes > 0)

• info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
  

• return modes;

+}

+/*

• • drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
• • @connector: connector corresponding to the HDMI sink
• • @vic: CEA vic for the video mode to be added in the map
• • Makes an entry for a videomode in the YCBCR 420 bitmap
• */

+static void +drm_add_cmdb_modes(struct drm_connector *connector, u8 svd) +{

• u8 vic = svd_to_vic(svd);
• struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
• bitmap_set(hdmi->y420_cmdb_modes, vic, 1);

+}

• static int do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len) { int i, modes = 0;

• struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;

  for (i = 0; i < len; i++) { struct drm_display_mode *mode; mode = drm_display_mode_from_vic_index(connector, db, len, i); if (mode) {

• 	/*
  

• 	 * YCBCR420 capability block contains a bitmap which
  

• 	 * gives the index of CEA modes from CEA VDB, which
  

• 	 * can support YCBCR 420 sampling output also (apart
  

• 	 * from RGB/YCBCR444 etc).
  

• 	 * For example, if the bit 0 in bitmap is set,
  

• 	 * first mode in VDB can support YCBCR420 output too.
  

• 	 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
  

• 	 */
  

• 	if (test_bit(i, &hdmi->y420_cmdb_map))
  

• 		drm_add_cmdb_modes(connector, db[i]);
  

• drm_mode_probed_add(connector, mode);
  modes++;
  

  }

@@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len, }

static int +cea_db_extended_tag(const u8 *db) +{

• return db[1];

+}

+static int cea_db_payload_len(const u8 *db) { return db[0] & 0x1f; @@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) return oui == HDMI_FORUM_IEEE_OUI; }

+static bool cea_db_is_y420cmdb(const u8 *db) +{

• if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)

Still would like to see that patch to clean up the current extended tag usage...

Yep, its on the way, running a bit loaded.

...

...

• return false;
  

• if (!cea_db_payload_len(db))

• return false;
  

• if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)

• return false;
  

• return true;

+}

+static bool cea_db_is_y420vdb(const u8 *db) +{

• if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)

• return false;
  

• if (!cea_db_payload_len(db))

• return false;
  

• if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)

• return false;
  

• return true;

+}

• #define for_each_cea_db(cea, i, start, end) \ for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)

+static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,

• 		     const u8 *db)
  

+{

• struct drm_display_info *info = &connector->display_info;
• struct drm_hdmi_info *hdmi = &info->hdmi;
• u8 map_len = cea_db_payload_len(db) - 1;
• u8 count;
• u64 map = 0;
• if (map_len == 0) {

• /* All CEA modes support ycbcr420 sampling also.*/
  

• hdmi->y420_cmdb_map = U64_MAX;
  

• info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
  

• return;
  

• }
• /*

• * This map indicates which of the existing CEA block modes
  

• * from VDB can support YCBCR420 output too. So if bit=0 is
  

• * set, first mode from VDB can support YCBCR420 output too.
  

• * We will parse and keep this map, before parsing VDB itself
  

• * to avoid going through the same block again and again.
  

• *
  

• * Spec is not clear about max possible size of this block.
  

• * Clamping max bitmap block size at 8 bytes. Every byte can
  

• * address 8 CEA modes, in this way this map can address
  

• * 8*8 = first 64 SVDs.
  

• */
  

• if (WARN_ON_ONCE(map_len > 8))

• map_len = 8;
  

• for (count = 0; count < map_len; count++)

• map |= (u64)db[2 + count] << (8 * count);
  

• if (map)

• info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
  

• hdmi->y420_cmdb_map = map;

+}

• static int add_cea_modes(struct drm_connector *connector, struct edid *edid) {

@@ -3522,10 +3663,17 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) video = db + 1; video_len = dbl; modes += do_cea_modes(connector, video, dbl);

• 	}
  

• 	else if (cea_db_is_hdmi_vsdb(db)) {
  

• 	} else if (cea_db_is_hdmi_vsdb(db)) {
  	hdmi = db;
  	hdmi_len = dbl;
  

• 	} else if (cea_db_is_y420vdb(db) &&
  

• 		   connector->ycbcr_420_allowed) {
  

• 		const u8 *vdb420 = &db[2];
  

• 		/* Add 4:2:0(only) modes present in EDID */
  

• 		modes += do_y420vdb_modes(connector,
  

• 					  vdb420,
  

• 					  dbl - 1);
  }
  

  } }

@@ -4206,6 +4354,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, drm_parse_hdmi_vsdb_video(connector, db); if (cea_db_is_hdmi_forum_vsdb(db)) drm_parse_hdmi_forum_vsdb(connector, db);

• if (cea_db_is_y420cmdb(db))
  

• 	drm_parse_y420cmdb_bitmap(connector, db);
  

  } }

@@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) num_modes += add_cea_modes(connector, edid); num_modes += add_alternate_cea_modes(connector, edid); num_modes += add_displayid_detailed_modes(connector, edid);

• if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF) num_modes += add_inferred_modes(connector, edid);

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 7493fd3..ce2212d 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -137,6 +137,25 @@ struct drm_scdc { struct drm_hdmi_info { /** @scdc: sink's scdc support and capabilities */ struct drm_scdc scdc;

• /**

• * @y420_vdb_modes: bitmap of modes which can support ycbcr420
  

• * output only (not normal RGB/YCBCR444/422 outputs). There are total
  

• * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
  

• * upto 128 VICs;
  

• */
  

• unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
• /**

• * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
  

• * output also, along with normal HDMI outputs. There are total 107
  

• * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
  

• * 128 VICs;
  

• */
  

• unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
• /** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
• unsigned long y420_cmdb_map;

u64, otherwise it's busted on 32bit machines.

Got it.

...

Mostl this looks pretty good. I think the main problem is the lack of mode validation that would reject the 4:2:0 only modes if not supported by the driver/device. I guess that's in a later patch?

Yep, its the next patch in the series.

The only thing the next patch does is set up the y420_dc_modes, but it doesn't use that for anything, and neither does it filter anything. I can't find any mode filtering patch actually.

...

If so we need to reorder the series a bit to avoid regeressions.

That patch uses parameters added in this patch, to detect the 4:2:0 only modes. So cant make it before this patch. Let me see what best can I do on the reordering.

• Shashank

...

...

};

/** @@ -200,6 +219,7 @@ struct drm_display_info { #define DRM_COLOR_FORMAT_RGB444 (1<<0) #define DRM_COLOR_FORMAT_YCRCB444 (1<<1) #define DRM_COLOR_FORMAT_YCRCB422 (1<<2) +#define DRM_COLOR_FORMAT_YCRCB420 (1<<3)

/** * @color_formats: HDMI Color formats, selects between RGB and YCrCb -- 2.7.4

Regards

Shashank

On 6/27/2017 5:23 PM, Ville Syrjälä wrote:

On Wed, Jun 21, 2017 at 04:04:03PM +0530, Shashank Sharma wrote:

...

CEA-861-F spec adds ycbcr420 deep color support information in hf-vsdb block. This patch extends the existing hf-vsdb parsing function by adding parsing of ycbcr420 deep color support from the EDID and adding it into display information stored.

V2: Rebase V3: Rebase V4: Moved definition of y420_dc_modes into this patch, where its used (Ville)

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Jose Abreu joabreu@synopsys.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++ include/drm/drm_connector.h | 3 +++ include/drm/drm_edid.h | 5 +++++ 3 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 8c7e73b..b4583f6 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4196,6 +4196,19 @@ drm_default_rgb_quant_range(const struct drm_display_mode *mode) } EXPORT_SYMBOL(drm_default_rgb_quant_range);

+static void drm_parse_ycbcr420_deep_color_info(struct drm_connector *connector,

• 			     const u8 *db)
  

+{

• struct drm_hdmi_info *info = &connector->display_info.hdmi;
• if (db[7] & DRM_EDID_YCBCR420_DC_48)

• info->y420_dc_modes |= DRM_EDID_YCBCR420_DC_48;
  

• if (db[7] & DRM_EDID_YCBCR420_DC_36)

• info->y420_dc_modes |= DRM_EDID_YCBCR420_DC_36;
  

• if (db[7] & DRM_EDID_YCBCR420_DC_30)

• info->y420_dc_modes |= DRM_EDID_YCBCR420_DC_30;
  

y420_dc_modes = db[7] & (...);

will do if you plan to just copy the bits over.

Yep, that's a good idea. Shashank

...

+}

• static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector, const u8 *hf_vsdb) {

@@ -4236,6 +4249,8 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector, scdc->scrambling.low_rates = true; } }

• drm_parse_ycbcr420_deep_color_info(connector, hf_vsdb); }

  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ce2212d..1305fe9 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -156,6 +156,9 @@ struct drm_hdmi_info {

/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */ unsigned long y420_cmdb_map;

• /** @y420_dc_modes: bitmap of deep color support index */

• u8 y420_dc_modes; };

  /**

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 89c0062..d4ff17c 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -213,6 +213,11 @@ struct detailed_timing { #define DRM_EDID_HDMI_DC_30 (1 << 4) #define DRM_EDID_HDMI_DC_Y444 (1 << 3)

+/* YCBCR 420 deep color modes */ +#define DRM_EDID_YCBCR420_DC_48 (1 << 6) +#define DRM_EDID_YCBCR420_DC_36 (1 << 5) +#define DRM_EDID_YCBCR420_DC_30 (1 << 4)

• /* ELD Header Block */ #define DRM_ELD_HEADER_BLOCK_SIZE 4

-- 2.7.4

On Wed, Jun 21, 2017 at 04:04:04PM +0530, Shashank Sharma wrote:

CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks. This block contains a map of indexes of CEA modes, which can support YCBCR 420 output also. To avoid multiple parsing of same CEA block, let's parse the sink information and get this map, before parsing CEA modes.

This patch moves the call to drm_add_display_info function, before the mode parsing block.

Signed-off-by: Shashank Sharma shashank.sharma@intel.com

drivers/gpu/drm/drm_edid.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index b4583f6..42934b2 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4605,6 +4605,13 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) quirks = edid_get_quirks(edid);

/*

• * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
  

• * To avoid multiple parsing of same block, lets parse that map
  

• * from sink info, before parsing CEA modes.
  

• */
  

• drm_add_display_info(connector, edid);

This patch should come before the 4:2:0 mode parsing, no?

Otherwise I think this should be fine so Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

• /*
  • EDID spec says modes should be preferred in this order:
  • • preferred detailed mode
  • • other detailed modes from base block

@@ -4632,8 +4639,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75)) edid_fixup_preferred(connector, quirks);

• drm_add_display_info(connector, edid);
• if (quirks & EDID_QUIRK_FORCE_6BPC) connector->display_info.bpc = 6;

-- 2.7.4

On Fri, Jun 30, 2017 at 10:52:54AM +0530, Sharma, Shashank wrote:

Regards

Shashank

On 6/27/2017 5:25 PM, Ville Syrjälä wrote:

...

On Wed, Jun 21, 2017 at 04:04:04PM +0530, Shashank Sharma wrote:

...

CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks. This block contains a map of indexes of CEA modes, which can support YCBCR 420 output also. To avoid multiple parsing of same CEA block, let's parse the sink information and get this map, before parsing CEA modes.

This patch moves the call to drm_add_display_info function, before the mode parsing block.

Signed-off-by: Shashank Sharma shashank.sharma@intel.com

drivers/gpu/drm/drm_edid.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index b4583f6..42934b2 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4605,6 +4605,13 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) quirks = edid_get_quirks(edid);

/*

• * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
  

• * To avoid multiple parsing of same block, lets parse that map
  

• * from sink info, before parsing CEA modes.
  

• */
  

• drm_add_display_info(connector, edid);

This patch should come before the 4:2:0 mode parsing, no?

Dint you ask me to move it later (in the previous series comments), for git-bisect regression type of changes ?

I wanted it split out to help with bisecting. It should be early in the series because otherwise the rest makes no sense. And I suppose we should be able to push this in on its own right now. Just need a CI run for it, so maybe resesnd just this patch on its own.

...

Otherwise I think this should be fine so Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

Thanks.

...

...

• /*
  • EDID spec says modes should be preferred in this order:
  • • preferred detailed mode
  • • other detailed modes from base block

@@ -4632,8 +4639,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75)) edid_fixup_preferred(connector, quirks);

• drm_add_display_info(connector, edid);
• if (quirks & EDID_QUIRK_FORCE_6BPC) connector->display_info.bpc = 6;

-- 2.7.4

On Wed, Jun 21, 2017 at 04:04:06PM +0530, Shashank Sharma wrote:

This patch adds set of helper functions for YCBCR HDMI output handling. These functions provide functionality like:

• check if a given video mode is YCBCR 420 only mode.
• check if a given video mode is YCBCR 420 mode.
• get the highest subsamled ycbcr output.
• get the lowest subsamled ycbcr output.
• check if a given source and sink combination can support any YCBCR HDMI output.
• check if a given source and sink combination can support a particular YCBCR HDMI output.

Currently all these helpers are kept static, and only one wrapper function is exposed to callers, which is "drm_find_hdmi_output_type". This function takes inputs as current mode, user preference and src + sink capabilities, and provides the most suitable HDMI output format to match all of these inputs.

This wrapper function will be used in next few patches in this series from I915 driver.

V2: Added YCBCR functions as helpers in DRM layer, instead of keeping it in I915 layer. V3: Added handling for YCBCR-420 only modes too. V4: EXPORT_SYMBOL(drm_find_hdmi_output_type)

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jose Abreu Jose.Abreu@synopsys.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

Not sure why you mark the series as RESEND-CI when not everything is reviewed yet. Anyway, stumbled over this, some comments below.

---

drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_modes.c | 307 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_edid.h | 1 + include/drm/drm_modes.h | 5 + 4 files changed, 314 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index edba190..2f8810a 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2949,7 +2949,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) } EXPORT_SYMBOL(drm_match_cea_mode);

-static bool drm_valid_cea_vic(u8 vic) +bool drm_valid_cea_vic(u8 vic) { return vic > 0 && vic < ARRAY_SIZE(edid_cea_modes); } diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f2493b9..09e6b10 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1576,3 +1576,310 @@ int drm_mode_convert_umode(struct drm_display_mode *out, out: return ret; }

+/**

• • drm_mode_is_420_only - if a given videomode can be only supported in YCBCR420
• • output format
• • @connector: drm connector under action.
• • @mode: video mode to be tested.
• • Returns:
• • true if the mode can support YCBCR420 output
• • false if not.
• */

For static functions we generally don't ever document them using kernel-doc. And within the drm core generally only the functions exported to driver (i.e. relevant for driver authors) are fully documented. Especially for such small helpers, if you need docs the function name isn't descriptive enough. I'd remove them all (except the one kerneldoc for the exported function).

On function naming itself, the usual pattern is

object_prefix_verb_stuff

In your case here all the helpers check against limits in drm_display_info, so I'd go with

drm_display_info_is_420_only|also_mode drm_display_supports_ycbcr_mode drm_display_supports_ycbcr (can_support is a bit tedious, supports is shorter)

Those are all bikesheds, but for the main exported function I think it matters. I'll discuss that below.

+static bool drm_mode_is_420_only(struct drm_display_info *display,

• 	struct drm_display_mode *mode)
  

+{

• u8 vic = drm_match_cea_mode(mode);
• /*

• * Requirements of a 420_only mode:
  

• * must be a valid cea mode
  

• * entry in 420_only bitmap
  

• */
  

• if (!drm_valid_cea_vic(vic))

• return false;
  

• return test_bit(vic, display->hdmi.y420_vdb_modes);

+}

+/**

• • drm_mode_is_420_also - if a given videomode can be supported in YCBCR420
• • output format also (along with RGB/YCBCR444/422)
• • @display: display under action.
• • @mode: video mode to be tested.
• • Returns:
• • true if the mode can support YCBCR420 output
• • false if not.
• */

+static bool drm_mode_is_420_also(struct drm_display_info *display,

• 	struct drm_display_mode *mode)
  

+{

• u8 vic = drm_match_cea_mode(mode);
• /*

• * Requirements of a 420_also mode:
  

• * must be a valid cea mode
  

• * entry in 420_also bitmap
  

• */
  

• if (!drm_valid_cea_vic(vic))

• return false;
  

• return test_bit(vic, display->hdmi.y420_cmdb_modes);

+}

+static bool drm_mode_is_420(struct drm_display_info *display,

• 	struct drm_display_mode *mode)
  

+{

• return drm_mode_is_420_only(display, mode) ||

• 	drm_mode_is_420_also(display, mode);
  

+}

+/**

• • drm_can_support_this_ycbcr_output - can a given source and sink combination
• • support a particular YCBCR output type.
• • @display: sink information.
• • @mode: video mode from modeset
• • @type: enum indicating YCBCR output type
• • @source_outputs: bitmap of source supported HDMI output formats.
• • Returns:
• • true on success.
• • false on failure.
• */

+static bool drm_can_support_this_ycbcr_output(struct drm_display_info *display,

• 			struct drm_display_mode *mode,
  

• 			enum drm_hdmi_output_type type,
  

• 			u32 source_outputs)
  

+{

• /* YCBCR420 output support can be per mode basis */
• if (type == DRM_HDMI_OUTPUT_YCBCR420 &&

• !drm_mode_is_420(display, mode))
  

• return false;
  

• return display->color_formats & source_outputs & type;

+}

+/**

• • drm_can_support_ycbcr_output - can a given source and sink combination
• • support any YCBCR outputs ?
• • @display: sink information.
• • @source_outputs: bitmap of source supported HDMI output formats.
• • Returns:
• • true on success.
• • false on failure.
• */

+static bool drm_can_support_ycbcr_output(struct drm_display_info *display,

• 			u32 source_outputs)
  

+{

• u32 supported_formats;
• if (!source_outputs || !display->color_formats) {

• DRM_DEBUG_KMS("Source/Sink doesn't support any output ?\n");
  

• return DRM_HDMI_OUTPUT_INVALID;
  

• }
• /* Get the common supported fromats between source and sink */
• supported_formats = display->color_formats & source_outputs;
• if (!supported_formats || (supported_formats ==

• DRM_COLOR_FORMAT_RGB444)) {
  

• DRM_ERROR("No common YCBCR formats between source and sink\n");
  

• return false;
  

• }
• DRM_DEBUG_KMS("Src and Sink combination can support YCBCR output\n");
• return true;

+}

+/**

• • drm_get_highest_quality_ycbcr_supported - get the ycbcr output mode
• • with highest subsampling rate
• • @display: struct drm_display_info from current connector
• • @mode: video mode from modeset
• • @source_output_map: bitmap of supported HDMI output modes from source
• • Finds the best possible ycbcr output mode (based on subsampling), for the
• • given source and sink combination.
• • Returns:
• • enum corresponding to best output mode on success.
• • DRM_HDMI_OUTPUT_INVALID on failure.
• */

+static enum drm_hdmi_output_type +drm_get_highest_quality_ycbcr_supported(struct drm_display_info *display,

• 			struct drm_display_mode *mode,
  

• 			u32 source_output_map)
  

+{

• enum drm_hdmi_output_type output = DRM_HDMI_OUTPUT_INVALID;
• u32 supported_formats = source_output_map & display->color_formats;
• /*

• * Get the ycbcr output with the highest possible subsampling rate.
  

• * Preference should go as:
  

• * ycbcr 444
  

• * ycbcr 422
  

• * ycbcr 420
  

• */
  

• if (supported_formats & DRM_COLOR_FORMAT_YCRCB444)

• output = DRM_COLOR_FORMAT_YCRCB444;
  

• else if (supported_formats & DRM_COLOR_FORMAT_YCRCB422)

• output = DRM_COLOR_FORMAT_YCRCB422;
  

• else if (supported_formats & DRM_COLOR_FORMAT_YCRCB420 &&

• 	drm_mode_is_420(display, mode))
  

• output = DRM_COLOR_FORMAT_YCRCB420;
  

• DRM_DEBUG_KMS("Highest subsampled YCBCR mode supported is %s\n",

• 	drm_get_hdmi_output_name(supported_formats));
  

• return output;

+}

+/**

• • drm_get_lowest_quality_ycbcr_supported - get the ycbcr output mode
• • with lowest subsampling rate
• • @display: struct drm_display_info from current connector
• • @mode: video mode from modeset
• • @source_output_map: bitmap of supported HDMI output modes from source
• • Finds the lowest possible ycbcr output mode (based on subsampling), for the
• • given source and sink combination.
• • Returns:
• • enum corresponding to best output mode on success.
• • DRM_HDMI_OUTPUT_INVALID on failure.
• */

+static enum drm_hdmi_output_type +drm_get_lowest_quality_ycbcr_supported(struct drm_display_info *display,

• 			struct drm_display_mode *mode,
  

• 			u32 source_output_map)
  

+{

• enum drm_hdmi_output_type output = DRM_HDMI_OUTPUT_INVALID;
• u32 supported_formats = source_output_map & display->color_formats;
• /*

• * Get the ycbcr output with the lowest possible subsampling rate.
  

• * Preference should go as:
  

• * ycbcr 420
  

• * ycbcr 422
  

• * ycbcr 444
  

• */
  

• if (supported_formats & DRM_COLOR_FORMAT_YCRCB420 &&

• drm_mode_is_420(display, mode))
  

• output = DRM_HDMI_OUTPUT_YCBCR420;
  

• else if (display->color_formats & DRM_COLOR_FORMAT_YCRCB422)

• output = DRM_HDMI_OUTPUT_YCBCR422;
  

• else if (display->color_formats & DRM_COLOR_FORMAT_YCRCB444)

• output = DRM_HDMI_OUTPUT_YCBCR420;
  

• DRM_DEBUG_KMS("Lowest subsampled YCBCR mode supported is %s\n",

• 	drm_get_hdmi_output_name(supported_formats));
  

• return output;

+}

+/**

• • drm_find_hdmi_output_type - get the most suitable output
• • Find the best suitable HDMI output considering source capability,
• • sink capability and user's choice (expressed in form of drm property)
• • @connector: drm connector in action
• • @mode: video mode under modeset
• • @type: user's choice for preferred mode, set via drm property
• • "hdmi_output_format"
• • @src_output_cap: bitmap of source's supported outputs formats
• • src_output_cap = (1 << DRM_COLOR_FORMAT_RGB444) means source
• • supports RGB444
• • src_output_cap = (1 << DRM_COLOR_FORMAT_YCRCB444) means source
• • supports YUV444, and so on.

You should explain in 1-2 sentences what exactly this function does, and when a driver should use it. Just documenting the input/output stuff doesn't make the kerneldoc all that useful.

• • Returns:
• • enum corresponding to best suitable output type on success.
• • DRM_HDMI_OUTPUT_INVALID on failure.
• */

+enum drm_hdmi_output_type +drm_find_hdmi_output_type(struct drm_connector *connector,

• 		struct drm_display_mode *mode,
  

• 		enum drm_hdmi_output_type type,
  

• 		u32 src_output_cap)
  

+{

• bool ret;
• struct drm_display_info *info = &connector->display_info;

Imo you should pass the display_info, not connector, to this function. For consistency. I'm also not sure we should place this in here, since drm_display_info is in drm_connector.h (but maybe that's just misplaced and perhaps we should move it to drm_modes.h). I wouldn't bother with moving it, but we definitely need a consistent prefix for exported functiosn. Since drm_display_info_set_bus_formats() already exists, I think the interface here should be:

enum drm_hdmi_output_type drm_display_info_hdmi_output_type(struct drm_display_info *display_info, struct drm_display_mode *mode, enum drm_hdmi_output_type type, u32 src_output_cap)

You can also drop the _find_ (sometimes we also call it compute e.g. in i915 modeset code), since computing stuff is implied.

Cheers, Daniel

• bool mode_is_420_only = drm_mode_is_420_only(info, mode);
• /*

• * If the preferred output is not set to YCBCR by user, and the mode
  

• * doesn't force us to drive YCBCR420 output, respect the user
  

• * preference for the output type. But if the mode is 420_only, we will
  

• * be force to drive YCBCR420 output.
  

• */
  

• if (!mode_is_420_only) {

• if (type == DRM_HDMI_OUTPUT_DEFAULT_RGB)
  

• 	return DRM_HDMI_OUTPUT_DEFAULT_RGB;
  

• } else {

• type = DRM_HDMI_OUTPUT_YCBCR420;
  

• DRM_DEBUG_KMS("Got a 420 only mode(%s)\n", mode->name);
  

• }
• /* If this src + sink combination can support any YCBCR output */
• ret = drm_can_support_ycbcr_output(info, src_output_cap);
• if (!ret) {

• DRM_ERROR("No supported YCBCR output\n");
  

• return DRM_HDMI_OUTPUT_INVALID;
  

• }
• switch (type) {
• case DRM_HDMI_OUTPUT_YCBCR_HQ:

• type = drm_get_highest_quality_ycbcr_supported(info, mode,
  

• 						src_output_cap);
  

• break;
  

• case DRM_HDMI_OUTPUT_YCBCR_LQ:

• type = drm_get_lowest_quality_ycbcr_supported(info, mode,
  

• 						src_output_cap);
  

• break;
  

• case DRM_HDMI_OUTPUT_YCBCR420:

• ret = drm_mode_is_420(info, mode);
  

• if (!ret) {
  

• 	DRM_ERROR("Mode %s doesn't support 420 output\n",
  

• 		   mode->name);
  

• 	type = DRM_HDMI_OUTPUT_INVALID;
  

• }
  

• break;
  

• /* Below cases are just to satisfy checkpatch's AI */
• case DRM_HDMI_OUTPUT_DEFAULT_RGB:
• case DRM_HDMI_OUTPUT_YCBCR444:
• case DRM_HDMI_OUTPUT_YCBCR422:

• break;
  

• default:

• type = DRM_HDMI_OUTPUT_INVALID;
  

• }
• if (type == DRM_HDMI_OUTPUT_INVALID) {

• DRM_ERROR("Can't support mode %s in YCBCR format\n",
  

• 		mode->name);
  

• return DRM_HDMI_OUTPUT_INVALID;
  

• }
• /* Test if this src/sink/mode combination can support selected output */
• ret = drm_can_support_this_ycbcr_output(info, mode, type,

• 				src_output_cap);
  

• if (!ret) {

• DRM_ERROR("Output %s can't be supported\n",
  

• 			drm_get_hdmi_output_name(type));
  

• return DRM_HDMI_OUTPUT_INVALID;
  

• }
• DRM_DEBUG_KMS("Best supported output is: %s\n",

• 	drm_get_hdmi_output_name(type));
  

• return type;

+} +EXPORT_SYMBOL(drm_find_hdmi_output_type); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 3ea833f..483f668 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -468,6 +468,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);

u8 drm_match_cea_mode(const struct drm_display_mode *to_match); enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code); +bool drm_valid_cea_vic(u8 vic); bool drm_detect_hdmi_monitor(struct edid *edid); bool drm_detect_monitor_audio(struct edid *edid); bool drm_rgb_quant_range_selectable(struct edid *edid); diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 94ac771..3c237e1 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -451,6 +451,11 @@ int drm_mode_convert_umode(struct drm_display_mode *out, void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode); void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);

+enum drm_hdmi_output_type +drm_find_hdmi_output_type(struct drm_connector *connector,

• 	  struct drm_display_mode *mode,
  

• 	  enum drm_hdmi_output_type type,
  

• 	  u32 src_output_cap);
  

struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh, bool reduced, bool interlaced, -- 2.7.4

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote:

To get a YCBCR420 output from intel platforms, we need one scaler to scale down YCBCR444 samples to YCBCR420 samples.

This patch:

• Does scaler allocation for HDMI ycbcr420 outputs.
• Programs PIPE_MISC register for ycbcr420 output.
• Adds a new scaler user "HDMI output" to plug-into existing scaler framework. This output type is identified using bit 30 of the scaler users bitmap.

V2: rebase V3: rebase V4: rebase

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Ander Conselvan De Oliveira ander.conselvan.de.oliveira@intel.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 3 ++- 5 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 36d4e63..a8c9ac5 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,

	/* panel fitter case: assign as a crtc scaler */
	scaler_id = &scaler_state->scaler_id;

• } else if (i == SKL_HDMI_OUTPUT_INDEX) {
  

• 	name = "HDMI-OUTPUT";
  

• 	idx = intel_crtc->base.base.id;
  

• 	/* hdmi output case: needs a pipe scaler */
  

• 	scaler_id = &scaler_state->scaler_id;
  

  } else { name = "PLANE";

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index da29536..983f581 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, */ need_scaling = src_w != dst_w || src_h != dst_h;

• if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420

• && scaler_user == SKL_HDMI_OUTPUT_INDEX)
  

Is it really necessary to check for both? If it is, what's the point of creating SKL_HDMI_OUTPUT_INDEX?

• /* YCBCR 444 -> 420 conversion needs a scaler */
  

• need_scaling = true;
  

• /*
  • if plane is being disabled or scaler is no more required or force detach
  • • free scaler binded to this plane/crtc

@@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, }

/**

• • skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI.
• • HDMI YCBCR 420 output needs a scaler, for downsampling.
• • @state: crtc's scaler state
• • Return

• • 0 - scaler_usage updated successfully
    

• • error - requested scaling cannot be supported or other error condition
• */

+int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) +{

• const struct drm_display_mode *mode = &state->base.adjusted_mode;
• return skl_update_scaler(state, !state->base.active,

• SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id,
  

• state->pipe_src_w, state->pipe_src_h,
  

• mode->crtc_hdisplay, mode->crtc_vdisplay);
  

+}

+/**

• skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
• @state: crtc's scaler state

@@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output;

  if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { u32 val = 0;

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 38fe56c..2206aa8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { * * If a bit is set, a user is using a scaler. * Here user can be a plane or crtc as defined below:

• *       bits 0-30 - plane (bit position is index from drm_plane_index)
  

• *       bits 0-29 - plane (bit position is index from drm_plane_index)
  

• *       bit 30    - hdmi output
  

  •   bit 31    - crtc
    

  • Instead of creating a new index to cover planes and crtc, using

@@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { * avilability. */ #define SKL_CRTC_INDEX 31

• /*

• * HDMI YCBCR 420 output consume a scaler. So adding a user
  

• * for HDMI output 420 requirement.
  

• */
  

+#define SKL_HDMI_OUTPUT_INDEX 30

I'm not convinced about this. The scaler is still used as a pipe scaler and the patch even adds a call intel_pch_panel_fitting().

unsigned scaler_users;

/* scaler used by crtc for panel fitting purpose */ @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_state *pipe_config);

int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);

static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 22da5cd..8d5aa1e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, }

if (type == DRM_HDMI_OUTPUT_YCBCR420) {

• struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc);
  
  

  /* YCBCR420 TMDS rate requirement is half the pixel clock */ config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;

@@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, *clock_12bpc /= 2; *clock_8bpc /= 2;

• /* YCBCR 420 output conversion needs a scaler */
  

• if (skl_update_scaler_crtc_hdmi_output(config)) {
  

• 	DRM_ERROR("Scaler allocation for output failed\n");
  

• 	return DRM_HDMI_OUTPUT_INVALID;
  

• }
  

• /* Bind this scaler to pipe */
  

• intel_pch_panel_fitting(intel_crtc, config,
  

• 			DRM_MODE_SCALE_FULLSCREEN);
  

  }

  /* Encoder is capable of this output, lets commit to CRTC */

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 96c2cbd..b6a32c4 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,

/* Native modes don't need fitting */ if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&

•    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h)
  

•    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
  

•    pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420)
  

I don't like that the knowledge that YCBCR420 needs the scaler is spread everywhere, including this function that should only deal with panel fitting. I would rather add a force parameter to intel_pch_panel_fitting() and skl_update_scaler() that would mean setup the scaler even if looks like it's not necessary. That way the hdmi code would just call them with force enabled, without sprinkling "hdmi_output == YCBCR420" everywhere.

Ander

goto done;

switch (fitting_mode) {

On Fri, 2017-06-30 at 11:20 +0530, Sharma, Shashank wrote:

Regards

Shashank

On 6/27/2017 5:46 PM, Ander Conselvan De Oliveira wrote:

...

On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote:

...

To get a YCBCR420 output from intel platforms, we need one scaler to scale down YCBCR444 samples to YCBCR420 samples.

This patch:

• Does scaler allocation for HDMI ycbcr420 outputs.
• Programs PIPE_MISC register for ycbcr420 output.
• Adds a new scaler user "HDMI output" to plug-into existing scaler framework. This output type is identified using bit 30 of the scaler users bitmap.

V2: rebase V3: rebase V4: rebase

Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Ander Conselvan De Oliveira ander.conselvan.de.oliveira@intel.com Signed-off-by: Shashank Sharma shashank.sharma@intel.com

---

drivers/gpu/drm/i915/intel_atomic.c | 6 ++++++ drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++- drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 3 ++- 5 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 36d4e63..a8c9ac5 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,

	/* panel fitter case: assign as a crtc scaler */
	scaler_id = &scaler_state->scaler_id;

• } else if (i == SKL_HDMI_OUTPUT_INDEX) {
  

• 	name = "HDMI-OUTPUT";
  

• 	idx = intel_crtc->base.base.id;
  

• 	/* hdmi output case: needs a pipe scaler */
  

• 	scaler_id = &scaler_state->scaler_id;
  

  } else { name = "PLANE";

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index da29536..983f581 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, */ need_scaling = src_w != dst_w || src_h != dst_h;

• if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420

• && scaler_user == SKL_HDMI_OUTPUT_INDEX)
  

Is it really necessary to check for both? If it is, what's the point of creating SKL_HDMI_OUTPUT_INDEX?

Yes, else this will affect scaler update for planes, as this function gets called to update the plane scalers too, at that time the output will be still valid (as its at CRTC level), but the scaler user would be different.

Right, so there is a need to check for scaler_user == HDMI_OUTPUT. But then wouldn't it be a bug if hdmi_output != YCBCR420 ? Point is, if the caller asked for a HDMI_OUTPUT scaler hopefully it knows what its doing, no need to second guess it.

...

...

• /* YCBCR 444 -> 420 conversion needs a scaler */
  

• need_scaling = true;
  

• /*
  • if plane is being disabled or scaler is no more required or force detach
  • • free scaler binded to this plane/crtc

@@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, }

/**

• • skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI.
• • HDMI YCBCR 420 output needs a scaler, for downsampling.
• • @state: crtc's scaler state
• • Return

• • 0 - scaler_usage updated successfully
    

• • error - requested scaling cannot be supported or other error condition
• */

+int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state) +{

• const struct drm_display_mode *mode = &state->base.adjusted_mode;
• return skl_update_scaler(state, !state->base.active,

• SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id,
  

• state->pipe_src_w, state->pipe_src_h,
  

• mode->crtc_hdisplay, mode->crtc_vdisplay);
  

+}

+/**

• skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
• @state: crtc's scaler state

@@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output;

  if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { u32 val = 0;

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 38fe56c..2206aa8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { * * If a bit is set, a user is using a scaler. * Here user can be a plane or crtc as defined below:

• *       bits 0-30 - plane (bit position is index from drm_plane_index)
  

• *       bits 0-29 - plane (bit position is index from drm_plane_index)
  

• *       bit 30    - hdmi output
  

  •   bit 31    - crtc
    

  • Instead of creating a new index to cover planes and crtc, using

@@ -484,6 +485,12 @@ struct intel_crtc_scaler_state { * avilability. */ #define SKL_CRTC_INDEX 31

• /*

• * HDMI YCBCR 420 output consume a scaler. So adding a user
  

• * for HDMI output 420 requirement.
  

• */
  

+#define SKL_HDMI_OUTPUT_INDEX 30

I'm not convinced about this. The scaler is still used as a pipe scaler and the patch even adds a call intel_pch_panel_fitting().

This is a special mode usage of scalar defined in the bspec, when we want to run the scalar into 5x3 auto mode for YCBCR444->YCBCR420 scale down. It can be used only when the PIPE_MISC is also programmed to act accordingly. So this is different from using it as a panel fitter, and that's why added a new scalar user all together.

...

...

unsigned scaler_users;

/* scaler used by crtc for panel fitting purpose */ @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_state *pipe_config);

int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state); int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);

static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 22da5cd..8d5aa1e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, }

if (type == DRM_HDMI_OUTPUT_YCBCR420) {

• struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc);
  
  

  /* YCBCR420 TMDS rate requirement is half the pixel clock */ config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;

@@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state, *clock_12bpc /= 2; *clock_8bpc /= 2;

• /* YCBCR 420 output conversion needs a scaler */
  

• if (skl_update_scaler_crtc_hdmi_output(config)) {
  

• 	DRM_ERROR("Scaler allocation for output failed\n");
  

• 	return DRM_HDMI_OUTPUT_INVALID;
  

• }
  

• /* Bind this scaler to pipe */
  

• intel_pch_panel_fitting(intel_crtc, config,
  

• 			DRM_MODE_SCALE_FULLSCREEN);
  

  }

  /* Encoder is capable of this output, lets commit to CRTC */

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 96c2cbd..b6a32c4 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,

/* Native modes don't need fitting */ if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&

•    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h)
  

•    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
  

•    pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420)
  

I don't like that the knowledge that YCBCR420 needs the scaler is spread everywhere, including this function that should only deal with panel fitting. I would rather add a force parameter to intel_pch_panel_fitting() and skl_update_scaler() that would mean setup the scaler even if looks like it's not necessary. That way the hdmi code would just call them with force enabled, without sprinkling "hdmi_output == YCBCR420" everywhere.

As explained in the comment above, its special case usage of pipe scalar, and that's why being checked specifically.