diff mbox

[v4,2/2] drm: Add HDMI 2.0 VIC support for AVI info-frames

Message ID 1490282059-16331-2-git-send-email-shashank.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sharma, Shashank March 23, 2017, 3:14 p.m. UTC
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 the connector->display_info
to check if the connected display is HDMI 2.0.

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

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         |  5 ++++-
 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, 38 insertions(+), 21 deletions(-)

Comments

Ilia Mirkin March 23, 2017, 3:17 p.m. UTC | #1
On Thu, Mar 23, 2017 at 11:14 AM, Shashank Sharma
<shashank.sharma@intel.com> 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.

Should this patch come before the patch which recognizes VICs 65+?
Otherwise if only the first patch is applied but not this one, you
could end up with the bad scenario. (As can happen in bisections, for
example.)

>
> 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 the connector->display_info
> to check if the connected display is HDMI 2.0.
>
> 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
>
> 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         |  5 ++++-
>  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, 38 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 d4452d8..ab7a399 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -1877,7 +1877,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 5b24e89..3c5fd83 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -1861,7 +1861,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 d2590d7..c564dca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1760,7 +1760,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 a2a8236..f9b77b8 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 9126d03..dcf15d7 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 af93f7a..5ff2886 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>                 frame.colorspace = HDMI_COLORSPACE_YUV444;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3b494c2..f9a4b08 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4664,12 +4664,14 @@ EXPORT_SYMBOL(drm_set_preferred_mode);
>   *                                              data from a DRM display mode
>   * @frame: HDMI AVI infoframe
>   * @mode: DRM display mode
> + * @is_hdmi2: 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)
>  {
>         int err;
>
> @@ -4685,6 +4687,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 && 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 5243840..5cb44d4 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -781,7 +781,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 3eec74c..96644f3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -458,11 +458,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 = 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);
>         if (ret < 0) {
>                 DRM_ERROR("couldn't fill AVI infoframe\n");
>                 return;
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 816a6f5..694f626 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1011,7 +1011,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 c262512..80c36af 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 b214663..49b42bf 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 006260d..b105f1c 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 ce2dcba..2128b8c 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 e9cbe26..5d81301 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -394,7 +394,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 c47b9cb..3e267dd 100644
> --- a/drivers/gpu/drm/zte/zx_hdmi.c
> +++ b/drivers/gpu/drm/zte/zx_hdmi.c
> @@ -125,7 +125,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 a21d494..a4d174e7 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -348,7 +348,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);
>  int
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>                                             const struct drm_display_mode *mode);
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sharma, Shashank March 23, 2017, 3:22 p.m. UTC | #2
Regards

Shashank


On 3/23/2017 5:17 PM, Ilia Mirkin wrote:
> On Thu, Mar 23, 2017 at 11:14 AM, Shashank Sharma
> <shashank.sharma@intel.com> 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.
> Should this patch come before the patch which recognizes VICs 65+?
> Otherwise if only the first patch is applied but not this one, you
> could end up with the bad scenario. (As can happen in bisections, for
> example.)
I kindof agree, this could be the case, but also, for the correct 
functionality, you should have the whole series
together.
>> 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 the connector->display_info
>> to check if the connected display is HDMI 2.0.
>>
>> 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
>>
>> 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         |  5 ++++-
>>   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, 38 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 d4452d8..ab7a399 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> @@ -1877,7 +1877,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 5b24e89..3c5fd83 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> @@ -1861,7 +1861,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 d2590d7..c564dca 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> @@ -1760,7 +1760,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 a2a8236..f9b77b8 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 9126d03..dcf15d7 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 af93f7a..5ff2886 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>                  frame.colorspace = HDMI_COLORSPACE_YUV444;
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 3b494c2..f9a4b08 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4664,12 +4664,14 @@ EXPORT_SYMBOL(drm_set_preferred_mode);
>>    *                                              data from a DRM display mode
>>    * @frame: HDMI AVI infoframe
>>    * @mode: DRM display mode
>> + * @is_hdmi2: 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)
>>   {
>>          int err;
>>
>> @@ -4685,6 +4687,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 && 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 5243840..5cb44d4 100644
>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> @@ -781,7 +781,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 3eec74c..96644f3 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -458,11 +458,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 = 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);
>>          if (ret < 0) {
>>                  DRM_ERROR("couldn't fill AVI infoframe\n");
>>                  return;
>> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
>> index 816a6f5..694f626 100644
>> --- a/drivers/gpu/drm/i915/intel_sdvo.c
>> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
>> @@ -1011,7 +1011,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 c262512..80c36af 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 b214663..49b42bf 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 006260d..b105f1c 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 ce2dcba..2128b8c 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 e9cbe26..5d81301 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -394,7 +394,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 c47b9cb..3e267dd 100644
>> --- a/drivers/gpu/drm/zte/zx_hdmi.c
>> +++ b/drivers/gpu/drm/zte/zx_hdmi.c
>> @@ -125,7 +125,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 a21d494..a4d174e7 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -348,7 +348,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);
>>   int
>>   drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>                                              const struct drm_display_mode *mode);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jose Abreu March 23, 2017, 3:28 p.m. UTC | #3
Hi Shashank,


On 23-03-2017 15:14, 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 the connector->display_info
> to check if the connected display is HDMI 2.0.
>
> 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
>
> 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         |  5 ++++-
>  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, 38 insertions(+), 21 deletions(-)
>

[snip]

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index af93f7a..5ff2886 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>

dw-hdmi controller has full support for HDMI 2.0 features. It all
depends on the platform it is integrated.

I think adding a parameter to
drm_hdmi_avi_infoframe_from_display_mode is not the best idea
because of this case: A bridge can have support for HDMI 2.0
features but the platform may limit this support. I guess it can
happen in other drivers too.

Best regards,
Jose Miguel Abreu
Ilia Mirkin March 23, 2017, 3:28 p.m. UTC | #4
On Thu, Mar 23, 2017 at 11:22 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> On 3/23/2017 5:17 PM, Ilia Mirkin wrote:
>>
>> On Thu, Mar 23, 2017 at 11:14 AM, Shashank Sharma
>> <shashank.sharma@intel.com> 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.
>>
>> Should this patch come before the patch which recognizes VICs 65+?
>> Otherwise if only the first patch is applied but not this one, you
>> could end up with the bad scenario. (As can happen in bisections, for
>> example.)
>
> I kindof agree, this could be the case, but also, for the correct
> functionality, you should have the whole series
> together.

OK, so ... I have a bug in my filesystem, and I'm bisecting it and
rebooting my box at each step. I happen to hit this commit in my
bisect and my monitor doesn't turn on? Seems unpleasant, no?
Ville Syrjälä March 23, 2017, 3:36 p.m. UTC | #5
On Thu, Mar 23, 2017 at 05:14:19PM +0200, 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.

The spec is unfortunately vague when it comes to the CEA-861-F VIC
transmission when there is a corresponding HDMI VIC for the same mode.
I'm not sure if it's telling us to set both or just one depending on
whether we're transmitting 3D video or not. Or at least I can't parse
that information from the spec. Anyone have a better crystal ball
in their possession?

> 
> 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 the connector->display_info
> to check if the connected display is HDMI 2.0.
> 
> 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
> 
> 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         |  5 ++++-
>  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, 38 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 d4452d8..ab7a399 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -1877,7 +1877,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 5b24e89..3c5fd83 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -1861,7 +1861,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 d2590d7..c564dca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1760,7 +1760,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 a2a8236..f9b77b8 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 9126d03..dcf15d7 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 af93f7a..5ff2886 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3b494c2..f9a4b08 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4664,12 +4664,14 @@ EXPORT_SYMBOL(drm_set_preferred_mode);
>   *                                              data from a DRM display mode
>   * @frame: HDMI AVI infoframe
>   * @mode: DRM display mode
> + * @is_hdmi2: 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)
>  {
>  	int err;
>  
> @@ -4685,6 +4687,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 && 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 5243840..5cb44d4 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -781,7 +781,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 3eec74c..96644f3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -458,11 +458,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 = 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);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 816a6f5..694f626 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1011,7 +1011,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 c262512..80c36af 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 b214663..49b42bf 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 006260d..b105f1c 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 ce2dcba..2128b8c 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 e9cbe26..5d81301 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -394,7 +394,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 c47b9cb..3e267dd 100644
> --- a/drivers/gpu/drm/zte/zx_hdmi.c
> +++ b/drivers/gpu/drm/zte/zx_hdmi.c
> @@ -125,7 +125,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 a21d494..a4d174e7 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -348,7 +348,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);
>  int
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  					    const struct drm_display_mode *mode);
> -- 
> 2.7.4
Ville Syrjälä March 23, 2017, 3:45 p.m. UTC | #6
On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
> Hi Shashank,
> 
> 
> On 23-03-2017 15:14, 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 the connector->display_info
> > to check if the connected display is HDMI 2.0.
> >
> > 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
> >
> > 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         |  5 ++++-
> >  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, 38 insertions(+), 21 deletions(-)
> >
> 
> [snip]
> 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index af93f7a..5ff2886 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
> >  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> >
> 
> dw-hdmi controller has full support for HDMI 2.0 features. It all
> depends on the platform it is integrated.
> 
> I think adding a parameter to
> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
> because of this case: A bridge can have support for HDMI 2.0
> features but the platform may limit this support. I guess it can
> happen in other drivers too.

Your driver is in full control of what gets passed here. So I don't see
why that would be a problem.

Also this doesn't really have anything to do with the capabilities of
the source. All we want to make sure is that we don't send a VIC the
sink will not understand.
Jose Abreu March 23, 2017, 3:52 p.m. UTC | #7
Hi Ville,


On 23-03-2017 15:36, Ville Syrjälä wrote:
> On Thu, Mar 23, 2017 at 05:14:19PM +0200, 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.
> The spec is unfortunately vague when it comes to the CEA-861-F VIC
> transmission when there is a corresponding HDMI VIC for the same mode.
> I'm not sure if it's telling us to set both or just one depending on
> whether we're transmitting 3D video or not. Or at least I can't parse
> that information from the spec. Anyone have a better crystal ball
> in their possession?
>

I've been working in HDMI receivers and this is what I've got in
a comment:

1282        
/*                                                                                                       

1283          * Update current VIC: When transmitting any
extended video format                                       
1284          * indicated through use of the HDMI_VIC field in
the HDMI Vendor                                        
1285          * Specific InfoFrame or any other format which is
not described in                                      
1286          * the above cases, an HDMI Source shall set the AVI
InfoFrame VIC                                       
1287          * field to
zero.                                                                                        

1288          */

This was directly taken from the spec, can't remember exactly
were though.

So, the VIC in AVIIF must be set to 0 and the HDMI_VIC field in
VSIF shall be set to the HDMI 4k VIC.

Best regards,
Jose Miguel Abreu
Jose Abreu March 23, 2017, 3:57 p.m. UTC | #8
Hi Ville,


On 23-03-2017 15:45, Ville Syrjälä wrote:
> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> On 23-03-2017 15:14, 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 the connector->display_info
>>> to check if the connected display is HDMI 2.0.
>>>
>>> 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
>>>
>>> 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         |  5 ++++-
>>>  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, 38 insertions(+), 21 deletions(-)
>>>
>> [snip]
>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index af93f7a..5ff2886 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>
>> dw-hdmi controller has full support for HDMI 2.0 features. It all
>> depends on the platform it is integrated.
>>
>> I think adding a parameter to
>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>> because of this case: A bridge can have support for HDMI 2.0
>> features but the platform may limit this support. I guess it can
>> happen in other drivers too.
> Your driver is in full control of what gets passed here. So I don't see
> why that would be a problem.
>
> Also this doesn't really have anything to do with the capabilities of
> the source. All we want to make sure is that we don't send a VIC the
> sink will not understand.
>

But the driver is not aware of the platform limitations, its
generic to the controller only. We could add a field in pdata
which tells if platform is HDMI 2.0+ but what about other bridge
drivers or HDMI drivers? They will have to replicate the same
thing also.

Best regards,
Jose Miguel Abreu
Sharma, Shashank March 23, 2017, 4:08 p.m. UTC | #9
Regards

Shashank


On 3/23/2017 5:57 PM, Jose Abreu wrote:
> Hi Ville,
>
>
> On 23-03-2017 15:45, Ville Syrjälä wrote:
>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>>> Hi Shashank,
>>>
>>>
>>> On 23-03-2017 15:14, 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 the connector->display_info
>>>> to check if the connected display is HDMI 2.0.
>>>>
>>>> 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
>>>>
>>>> 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         |  5 ++++-
>>>>   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, 38 insertions(+), 21 deletions(-)
>>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>> index af93f7a..5ff2886 100644
>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>>   		frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>
>>> dw-hdmi controller has full support for HDMI 2.0 features. It all
>>> depends on the platform it is integrated.
>>>
>>> I think adding a parameter to
>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>>> because of this case: A bridge can have support for HDMI 2.0
>>> features but the platform may limit this support. I guess it can
>>> happen in other drivers too.
>> Your driver is in full control of what gets passed here. So I don't see
>> why that would be a problem.
>>
>> Also this doesn't really have anything to do with the capabilities of
>> the source. All we want to make sure is that we don't send a VIC the
>> sink will not understand.
>>
> But the driver is not aware of the platform limitations, its
> generic to the controller only. We could add a field in pdata
> which tells if platform is HDMI 2.0+ but what about other bridge
> drivers or HDMI drivers? They will have to replicate the same
> thing also.
>
> Best regards,
> Jose Miguel Abreu
I think the driver would be aware of the platform's capabilities, isn't 
it ?
Else how would it even decide which mode to allow, and which to reject ?

Regards
Shashank
Sharma, Shashank March 23, 2017, 4:31 p.m. UTC | #10
Regards

Shashank


On 3/23/2017 5:52 PM, Jose Abreu wrote:
> Hi Ville,
>
>
> On 23-03-2017 15:36, Ville Syrjälä wrote:
>> On Thu, Mar 23, 2017 at 05:14:19PM +0200, 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.
>> The spec is unfortunately vague when it comes to the CEA-861-F VIC
>> transmission when there is a corresponding HDMI VIC for the same mode.
>> I'm not sure if it's telling us to set both or just one depending on
>> whether we're transmitting 3D video or not. Or at least I can't parse
>> that information from the spec. Anyone have a better crystal ball
>> in their possession?
>>
> I've been working in HDMI receivers and this is what I've got in
> a comment:
>
> 1282
> /*
>
> 1283          * Update current VIC: When transmitting any
> extended video format
> 1284          * indicated through use of the HDMI_VIC field in
> the HDMI Vendor
> 1285          * Specific InfoFrame or any other format which is
> not described in
> 1286          * the above cases, an HDMI Source shall set the AVI
> InfoFrame VIC
> 1287          * field to
> zero.
>
> 1288          */
>
> This was directly taken from the spec, can't remember exactly
> were though.
>
> So, the VIC in AVIIF must be set to 0 and the HDMI_VIC field in
> VSIF shall be set to the HDMI 4k VIC.
>
> Best regards,
> Jose Miguel Abreu
Even my understanding of the two specs seems similar
If its a HDMI 1.4b monitor, 2D mode
     - VIC field in AVI_IF should be set to appropriate VIC [if VIC is 
not listed in CEA-861-D (VIC 1-64), make VIC=0]
If its a HDMI 1.4b monitor, 3D mode
     - VIC field in AVI_IF should be set to appropriate VIC, in 
conjunction with 3D_structure field in HDMI VSIF
If its a HDMI 2.0 monitor, 2D mode
     - VIC filed in the AVI_IF should be set to appropriate VIC (1-107)
If its a HDMI 2.0 monitor, 3D mode
     - VIC field in AVI_IF should be set to appropriate VIC, in 
conjunction with 3D_structure field in HDMI VSIF

Regards
Shashank
Sharma, Shashank March 23, 2017, 4:33 p.m. UTC | #11
Regards

Shashank


On 3/23/2017 5:28 PM, Ilia Mirkin wrote:
> On Thu, Mar 23, 2017 at 11:22 AM, Sharma, Shashank
> <shashank.sharma@intel.com> wrote:
>> On 3/23/2017 5:17 PM, Ilia Mirkin wrote:
>>> On Thu, Mar 23, 2017 at 11:14 AM, Shashank Sharma
>>> <shashank.sharma@intel.com> 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.
>>> Should this patch come before the patch which recognizes VICs 65+?
>>> Otherwise if only the first patch is applied but not this one, you
>>> could end up with the bad scenario. (As can happen in bisections, for
>>> example.)
>> I kindof agree, this could be the case, but also, for the correct
>> functionality, you should have the whole series
>> together.
> OK, so ... I have a bug in my filesystem, and I'm bisecting it and
> rebooting my box at each step. I happen to hit this commit in my
> bisect and my monitor doesn't turn on? Seems unpleasant, no?
Cant say no :-), will happily change the order if this makes you feel 
better.

Regards
Shashank
Jose Abreu March 23, 2017, 4:33 p.m. UTC | #12
Hi Shashank,


On 23-03-2017 16:08, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 3/23/2017 5:57 PM, Jose Abreu wrote:
>> Hi Ville,
>>
>>
>> On 23-03-2017 15:45, Ville Syrjälä wrote:
>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>>>> Hi Shashank,
>>>>
>>>>
>>>> On 23-03-2017 15:14, 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 the
>>>>> connector->display_info
>>>>> to check if the connected display is HDMI 2.0.
>>>>>
>>>>> 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
>>>>>
>>>>> 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         |  5 ++++-
>>>>>   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, 38 insertions(+), 21 deletions(-)
>>>>>
>>>> [snip]
>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> index af93f7a..5ff2886 100644
>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>>>           frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>
>>>> dw-hdmi controller has full support for HDMI 2.0 features.
>>>> It all
>>>> depends on the platform it is integrated.
>>>>
>>>> I think adding a parameter to
>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>>>> because of this case: A bridge can have support for HDMI 2.0
>>>> features but the platform may limit this support. I guess it
>>>> can
>>>> happen in other drivers too.
>>> Your driver is in full control of what gets passed here. So I
>>> don't see
>>> why that would be a problem.
>>>
>>> Also this doesn't really have anything to do with the
>>> capabilities of
>>> the source. All we want to make sure is that we don't send a
>>> VIC the
>>> sink will not understand.
>>>
>> But the driver is not aware of the platform limitations, its
>> generic to the controller only. We could add a field in pdata
>> which tells if platform is HDMI 2.0+ but what about other bridge
>> drivers or HDMI drivers? They will have to replicate the same
>> thing also.
>>
>> Best regards,
>> Jose Miguel Abreu
> I think the driver would be aware of the platform's
> capabilities, isn't it ?
> Else how would it even decide which mode to allow, and which to
> reject ?

The DRM core propagates the mode to the chain of configuration
before reaching the bridge driver also, there is a callback
supplied by pdata (mode_valid) which can check if the mode is
valid. (see
https://cgit.freedesktop.org/~airlied/linux/tree/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c?h=drm-next#n1740)

Best regards,
Jose Miguel Abreu


>
> Regards
> Shashank
Sharma, Shashank March 23, 2017, 4:43 p.m. UTC | #13
Regards

Shashank


On 3/23/2017 6:33 PM, Jose Abreu wrote:
> Hi Shashank,
>
>
> On 23-03-2017 16:08, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 3/23/2017 5:57 PM, Jose Abreu wrote:
>>> Hi Ville,
>>>
>>>
>>> On 23-03-2017 15:45, Ville Syrjälä wrote:
>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>>>>> Hi Shashank,
>>>>>
>>>>>
>>>>> On 23-03-2017 15:14, 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 the
>>>>>> connector->display_info
>>>>>> to check if the connected display is HDMI 2.0.
>>>>>>
>>>>>> 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
>>>>>>
>>>>>> 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         |  5 ++++-
>>>>>>    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, 38 insertions(+), 21 deletions(-)
>>>>>>
>>>>> [snip]
>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>> index af93f7a..5ff2886 100644
>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>>>>            frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>>
>>>>> dw-hdmi controller has full support for HDMI 2.0 features.
>>>>> It all
>>>>> depends on the platform it is integrated.
>>>>>
>>>>> I think adding a parameter to
>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>>>>> because of this case: A bridge can have support for HDMI 2.0
>>>>> features but the platform may limit this support. I guess it
>>>>> can
>>>>> happen in other drivers too.
>>>> Your driver is in full control of what gets passed here. So I
>>>> don't see
>>>> why that would be a problem.
>>>>
>>>> Also this doesn't really have anything to do with the
>>>> capabilities of
>>>> the source. All we want to make sure is that we don't send a
>>>> VIC the
>>>> sink will not understand.
>>>>
>>> But the driver is not aware of the platform limitations, its
>>> generic to the controller only. We could add a field in pdata
>>> which tells if platform is HDMI 2.0+ but what about other bridge
>>> drivers or HDMI drivers? They will have to replicate the same
>>> thing also.
>>>
>>> Best regards,
>>> Jose Miguel Abreu
>> I think the driver would be aware of the platform's
>> capabilities, isn't it ?
>> Else how would it even decide which mode to allow, and which to
>> reject ?
> The DRM core propagates the mode to the chain of configuration
> before reaching the bridge driver also, there is a callback
> supplied by pdata (mode_valid) which can check if the mode is
> valid. (see
> https://cgit.freedesktop.org/~airlied/linux/tree/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c?h=drm-next#n1740)
>
> Best regards,
> Jose Miguel Abreu
>
>
>> Regards
>> Shashank
Please correct me if my understanding is not right, but drivers call 
mode_valid() to prune/reject modes which they cant support.
and they call drm_set_infoframe_from_videomode() function, when they are 
going ahead to the modeset with a mode.
Why would a driver choose to do a modeset, which it could not support 
(shouldn't mode_valid have dropped it in atomic_check() or may be even 
edid_parsing time ?)

Regards
Shashank
Ville Syrjälä March 23, 2017, 5:01 p.m. UTC | #14
On Thu, Mar 23, 2017 at 06:31:33PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/23/2017 5:52 PM, Jose Abreu wrote:
> > Hi Ville,
> >
> >
> > On 23-03-2017 15:36, Ville Syrjälä wrote:
> >> On Thu, Mar 23, 2017 at 05:14:19PM +0200, 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.
> >> The spec is unfortunately vague when it comes to the CEA-861-F VIC
> >> transmission when there is a corresponding HDMI VIC for the same mode.
> >> I'm not sure if it's telling us to set both or just one depending on
> >> whether we're transmitting 3D video or not. Or at least I can't parse
> >> that information from the spec. Anyone have a better crystal ball
> >> in their possession?
> >>
> > I've been working in HDMI receivers and this is what I've got in
> > a comment:
> >
> > 1282
> > /*
> >
> > 1283          * Update current VIC: When transmitting any
> > extended video format
> > 1284          * indicated through use of the HDMI_VIC field in
> > the HDMI Vendor
> > 1285          * Specific InfoFrame or any other format which is
> > not described in
> > 1286          * the above cases, an HDMI Source shall set the AVI
> > InfoFrame VIC
> > 1287          * field to
> > zero.
> >
> > 1288          */
> >
> > This was directly taken from the spec, can't remember exactly
> > were though.
> >
> > So, the VIC in AVIIF must be set to 0 and the HDMI_VIC field in
> > VSIF shall be set to the HDMI 4k VIC.
> >
> > Best regards,
> > Jose Miguel Abreu
> Even my understanding of the two specs seems similar
> If its a HDMI 1.4b monitor, 2D mode
>      - VIC field in AVI_IF should be set to appropriate VIC [if VIC is 
> not listed in CEA-861-D (VIC 1-64), make VIC=0]
> If its a HDMI 1.4b monitor, 3D mode
>      - VIC field in AVI_IF should be set to appropriate VIC, in 
> conjunction with 3D_structure field in HDMI VSIF
> If its a HDMI 2.0 monitor, 2D mode
>      - VIC filed in the AVI_IF should be set to appropriate VIC (1-107)
> If its a HDMI 2.0 monitor, 3D mode
>      - VIC field in AVI_IF should be set to appropriate VIC, in 
> conjunction with 3D_structure field in HDMI VSIF

You're overlooking the HDMI VIC entirely in that list.

OK, so our code even refuses to send both 3D stuff and HDMI VIC
at the same time. I presume this was an illegal combination in HDMI 1.4.
And since there was no overlap between the CEA VICs and HDMI VICs this
was all that we needed.

But now that there is overlap, I think we'll need to set CEA VIC=0
whenever HDMI VIC!=0. Or at least that's my strictest interpretation of
the spec. I wish they would have stated this stuff more explicitly.

So either we need to pass the AVI IF to
drm_hdmi_vendor_infoframe_from_display_mode() and have it clear the AVI
IF VIC if HDMI VIC is specified, or we need to pass the HDMI IF to
drm_hdmi_avi_infoframe_from_display_mode() and have it not set the AVI
IF VIC if HDMI VIC is specified. Or we need to make the caller handle
this, which would probably lead to more bugs.
Jose Abreu March 23, 2017, 5:11 p.m. UTC | #15
Hi Shashank,


On 23-03-2017 16:43, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 3/23/2017 6:33 PM, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> On 23-03-2017 16:08, Sharma, Shashank wrote:
>>> Regards
>>>
>>> Shashank
>>>
>>>
>>> On 3/23/2017 5:57 PM, Jose Abreu wrote:
>>>> Hi Ville,
>>>>
>>>>
>>>> On 23-03-2017 15:45, Ville Syrjälä wrote:
>>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>>>>>> Hi Shashank,
>>>>>>
>>>>>>
>>>>>> On 23-03-2017 15:14, 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 the
>>>>>>> connector->display_info
>>>>>>> to check if the connected display is HDMI 2.0.
>>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>> 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         |  5 ++++-
>>>>>>>    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, 38 insertions(+), 21 deletions(-)
>>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> index af93f7a..5ff2886 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>>>>>            frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>>>
>>>>>> dw-hdmi controller has full support for HDMI 2.0 features.
>>>>>> It all
>>>>>> depends on the platform it is integrated.
>>>>>>
>>>>>> I think adding a parameter to
>>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>>>>>> because of this case: A bridge can have support for HDMI 2.0
>>>>>> features but the platform may limit this support. I guess it
>>>>>> can
>>>>>> happen in other drivers too.
>>>>> Your driver is in full control of what gets passed here. So I
>>>>> don't see
>>>>> why that would be a problem.
>>>>>
>>>>> Also this doesn't really have anything to do with the
>>>>> capabilities of
>>>>> the source. All we want to make sure is that we don't send a
>>>>> VIC the
>>>>> sink will not understand.
>>>>>
>>>> But the driver is not aware of the platform limitations, its
>>>> generic to the controller only. We could add a field in pdata
>>>> which tells if platform is HDMI 2.0+ but what about other
>>>> bridge
>>>> drivers or HDMI drivers? They will have to replicate the same
>>>> thing also.
>>>>
>>>> Best regards,
>>>> Jose Miguel Abreu
>>> I think the driver would be aware of the platform's
>>> capabilities, isn't it ?
>>> Else how would it even decide which mode to allow, and which to
>>> reject ?
>> The DRM core propagates the mode to the chain of configuration
>> before reaching the bridge driver also, there is a callback
>> supplied by pdata (mode_valid) which can check if the mode is
>> valid. (see
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__cgit.freedesktop.org_-7Eairlied_linux_tree_drivers_gpu_drm_bridge_synopsys_dw-2Dhdmi.c-3Fh-3Ddrm-2Dnext-23n1740&d=DwID-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zC1cWRy6RDWimQG2o866yYylV7c5h_U4aGWjVtnIHH0&s=DPZA3OgLWOO299hjy9dBDBK66kmUGfZArxKbeHNMKOc&e=
>> )
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>
>>> Regards
>>> Shashank
> Please correct me if my understanding is not right, but drivers
> call mode_valid() to prune/reject modes which they cant support.
> and they call drm_set_infoframe_from_videomode() function, when
> they are going ahead to the modeset with a mode.
> Why would a driver choose to do a modeset, which it could not
> support (shouldn't mode_valid have dropped it in atomic_check()
> or may be even edid_parsing time ?)

Yes, thats all correct :) I got a little out of topic here, sorry.

The thing I don't agree with is the adding of the parameter for
the drm_hdmi_avi_infoframe_from_display_mode(), because I think
its not enough. I will state why:

Picture this scenario:
    1) EDID is read from HDMI 2.0 sink
    2) The 2.0 modes are probed and as there is no check for
these modes (they are in the CEA table) they will be added to
mode list
    3) The modes are passed to userspace. This is a problem
because userspace does not yet understand the new aspect ratios,
but lets imagine it understands
    4) User selects an HDMI 2.0 mode
    5) The drm core checks if mode is valid with driver, the
driver can say yes or no. Lets think it says yes because the
platform supports HDMI 2.0 modes.
    6) Mode is committed but VIC will be set to zero according to
the patch

Now, according to 2.0 spec VIC shall be set when we are in 2.0
modes. I didn't do much inter-op with HDMI 2.0 receivers but I
can picture that some of them may just not display anything
because VIC and HDMI_VIC are not set. Though, from my experience,
generally  the receiver is much more robust than the transmitter.

Even so, I think we should make sure that transmitter drivers are
fully compliant so that we can guarantee the support for less
robust receivers.

Best regards,
Jose Miguel Abreu


>
> Regards
> Shashank
Ville Syrjälä March 23, 2017, 5:42 p.m. UTC | #16
On Thu, Mar 23, 2017 at 05:11:44PM +0000, Jose Abreu wrote:
> Hi Shashank,
> 
> 
> On 23-03-2017 16:43, Sharma, Shashank wrote:
> > Regards
> >
> > Shashank
> >
> >
> > On 3/23/2017 6:33 PM, Jose Abreu wrote:
> >> Hi Shashank,
> >>
> >>
> >> On 23-03-2017 16:08, Sharma, Shashank wrote:
> >>> Regards
> >>>
> >>> Shashank
> >>>
> >>>
> >>> On 3/23/2017 5:57 PM, Jose Abreu wrote:
> >>>> Hi Ville,
> >>>>
> >>>>
> >>>> On 23-03-2017 15:45, Ville Syrjälä wrote:
> >>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
> >>>>>> Hi Shashank,
> >>>>>>
> >>>>>>
> >>>>>> On 23-03-2017 15:14, 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 the
> >>>>>>> connector->display_info
> >>>>>>> to check if the connected display is HDMI 2.0.
> >>>>>>>
> >>>>>>> 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
> >>>>>>>
> >>>>>>> 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         |  5 ++++-
> >>>>>>>    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, 38 insertions(+), 21 deletions(-)
> >>>>>>>
> >>>>>> [snip]
> >>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> index af93f7a..5ff2886 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
> >>>>>>>            frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>>>>
> >>>>>> dw-hdmi controller has full support for HDMI 2.0 features.
> >>>>>> It all
> >>>>>> depends on the platform it is integrated.
> >>>>>>
> >>>>>> I think adding a parameter to
> >>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
> >>>>>> because of this case: A bridge can have support for HDMI 2.0
> >>>>>> features but the platform may limit this support. I guess it
> >>>>>> can
> >>>>>> happen in other drivers too.
> >>>>> Your driver is in full control of what gets passed here. So I
> >>>>> don't see
> >>>>> why that would be a problem.
> >>>>>
> >>>>> Also this doesn't really have anything to do with the
> >>>>> capabilities of
> >>>>> the source. All we want to make sure is that we don't send a
> >>>>> VIC the
> >>>>> sink will not understand.
> >>>>>
> >>>> But the driver is not aware of the platform limitations, its
> >>>> generic to the controller only. We could add a field in pdata
> >>>> which tells if platform is HDMI 2.0+ but what about other
> >>>> bridge
> >>>> drivers or HDMI drivers? They will have to replicate the same
> >>>> thing also.
> >>>>
> >>>> Best regards,
> >>>> Jose Miguel Abreu
> >>> I think the driver would be aware of the platform's
> >>> capabilities, isn't it ?
> >>> Else how would it even decide which mode to allow, and which to
> >>> reject ?
> >> The DRM core propagates the mode to the chain of configuration
> >> before reaching the bridge driver also, there is a callback
> >> supplied by pdata (mode_valid) which can check if the mode is
> >> valid. (see
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__cgit.freedesktop.org_-7Eairlied_linux_tree_drivers_gpu_drm_bridge_synopsys_dw-2Dhdmi.c-3Fh-3Ddrm-2Dnext-23n1740&d=DwID-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zC1cWRy6RDWimQG2o866yYylV7c5h_U4aGWjVtnIHH0&s=DPZA3OgLWOO299hjy9dBDBK66kmUGfZArxKbeHNMKOc&e=
> >> )
> >>
> >> Best regards,
> >> Jose Miguel Abreu
> >>
> >>
> >>> Regards
> >>> Shashank
> > Please correct me if my understanding is not right, but drivers
> > call mode_valid() to prune/reject modes which they cant support.
> > and they call drm_set_infoframe_from_videomode() function, when
> > they are going ahead to the modeset with a mode.
> > Why would a driver choose to do a modeset, which it could not
> > support (shouldn't mode_valid have dropped it in atomic_check()
> > or may be even edid_parsing time ?)
> 
> Yes, thats all correct :) I got a little out of topic here, sorry.
> 
> The thing I don't agree with is the adding of the parameter for
> the drm_hdmi_avi_infoframe_from_display_mode(), because I think
> its not enough. I will state why:
> 
> Picture this scenario:
>     1) EDID is read from HDMI 2.0 sink
>     2) The 2.0 modes are probed and as there is no check for
> these modes (they are in the CEA table) they will be added to
> mode list
>     3) The modes are passed to userspace. This is a problem
> because userspace does not yet understand the new aspect ratios,
> but lets imagine it understands
>     4) User selects an HDMI 2.0 mode
>     5) The drm core checks if mode is valid with driver, the
> driver can say yes or no. Lets think it says yes because the
> platform supports HDMI 2.0 modes.
>     6) Mode is committed but VIC will be set to zero according to
> the patch

That's only because you haven't updated your driver to pass 'true'
as needed.

The other option would be to pass the connector instead of a boolean,
but according to Shashank that would required a lot of plumbing changes
because many drivers don't have the connector immediately available
where they call these functions. And plumbing the connector through
probably requires intimate knowledge of each driver, and thus is not
something we would be confortable doing. Hence I think the bool is the
best short term solution, letting each driver author figure out how to
get at the right connector and pass on the required information.

I don't really understand what you're even suggesting as an alternative.
We need to know what the sink supports, and if your driver is architected
in some crazy way where you don't even have that information where it's
needed, I don't think there's anything anyone can do.
Jose Abreu March 23, 2017, 5:53 p.m. UTC | #17
Hi Ville,


On 23-03-2017 17:42, Ville Syrjälä wrote:
> On Thu, Mar 23, 2017 at 05:11:44PM +0000, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> On 23-03-2017 16:43, Sharma, Shashank wrote:
>>> Regards
>>>
>>> Shashank
>>>
>>>
>>> On 3/23/2017 6:33 PM, Jose Abreu wrote:
>>>> Hi Shashank,
>>>>
>>>>
>>>> On 23-03-2017 16:08, Sharma, Shashank wrote:
>>>>> Regards
>>>>>
>>>>> Shashank
>>>>>
>>>>>
>>>>> On 3/23/2017 5:57 PM, Jose Abreu wrote:
>>>>>> Hi Ville,
>>>>>>
>>>>>>
>>>>>> On 23-03-2017 15:45, Ville Syrjälä wrote:
>>>>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>>>>>>>> Hi Shashank,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23-03-2017 15:14, 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 the
>>>>>>>>> connector->display_info
>>>>>>>>> to check if the connected display is HDMI 2.0.
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>> 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         |  5 ++++-
>>>>>>>>>    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, 38 insertions(+), 21 deletions(-)
>>>>>>>>>
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>> index af93f7a..5ff2886 100644
>>>>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>>>>>>>            frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>>>>>
>>>>>>>> dw-hdmi controller has full support for HDMI 2.0 features.
>>>>>>>> It all
>>>>>>>> depends on the platform it is integrated.
>>>>>>>>
>>>>>>>> I think adding a parameter to
>>>>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>>>>>>>> because of this case: A bridge can have support for HDMI 2.0
>>>>>>>> features but the platform may limit this support. I guess it
>>>>>>>> can
>>>>>>>> happen in other drivers too.
>>>>>>> Your driver is in full control of what gets passed here. So I
>>>>>>> don't see
>>>>>>> why that would be a problem.
>>>>>>>
>>>>>>> Also this doesn't really have anything to do with the
>>>>>>> capabilities of
>>>>>>> the source. All we want to make sure is that we don't send a
>>>>>>> VIC the
>>>>>>> sink will not understand.
>>>>>>>
>>>>>> But the driver is not aware of the platform limitations, its
>>>>>> generic to the controller only. We could add a field in pdata
>>>>>> which tells if platform is HDMI 2.0+ but what about other
>>>>>> bridge
>>>>>> drivers or HDMI drivers? They will have to replicate the same
>>>>>> thing also.
>>>>>>
>>>>>> Best regards,
>>>>>> Jose Miguel Abreu
>>>>> I think the driver would be aware of the platform's
>>>>> capabilities, isn't it ?
>>>>> Else how would it even decide which mode to allow, and which to
>>>>> reject ?
>>>> The DRM core propagates the mode to the chain of configuration
>>>> before reaching the bridge driver also, there is a callback
>>>> supplied by pdata (mode_valid) which can check if the mode is
>>>> valid. (see
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__cgit.freedesktop.org_-7Eairlied_linux_tree_drivers_gpu_drm_bridge_synopsys_dw-2Dhdmi.c-3Fh-3Ddrm-2Dnext-23n1740&d=DwID-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zC1cWRy6RDWimQG2o866yYylV7c5h_U4aGWjVtnIHH0&s=DPZA3OgLWOO299hjy9dBDBK66kmUGfZArxKbeHNMKOc&e=
>>>> )
>>>>
>>>> Best regards,
>>>> Jose Miguel Abreu
>>>>
>>>>
>>>>> Regards
>>>>> Shashank
>>> Please correct me if my understanding is not right, but drivers
>>> call mode_valid() to prune/reject modes which they cant support.
>>> and they call drm_set_infoframe_from_videomode() function, when
>>> they are going ahead to the modeset with a mode.
>>> Why would a driver choose to do a modeset, which it could not
>>> support (shouldn't mode_valid have dropped it in atomic_check()
>>> or may be even edid_parsing time ?)
>> Yes, thats all correct :) I got a little out of topic here, sorry.
>>
>> The thing I don't agree with is the adding of the parameter for
>> the drm_hdmi_avi_infoframe_from_display_mode(), because I think
>> its not enough. I will state why:
>>
>> Picture this scenario:
>>     1) EDID is read from HDMI 2.0 sink
>>     2) The 2.0 modes are probed and as there is no check for
>> these modes (they are in the CEA table) they will be added to
>> mode list
>>     3) The modes are passed to userspace. This is a problem
>> because userspace does not yet understand the new aspect ratios,
>> but lets imagine it understands
>>     4) User selects an HDMI 2.0 mode
>>     5) The drm core checks if mode is valid with driver, the
>> driver can say yes or no. Lets think it says yes because the
>> platform supports HDMI 2.0 modes.
>>     6) Mode is committed but VIC will be set to zero according to
>> the patch
> That's only because you haven't updated your driver to pass 'true'
> as needed.
>
> The other option would be to pass the connector instead of a boolean,
> but according to Shashank that would required a lot of plumbing changes
> because many drivers don't have the connector immediately available
> where they call these functions. And plumbing the connector through
> probably requires intimate knowledge of each driver, and thus is not
> something we would be confortable doing. Hence I think the bool is the
> best short term solution, letting each driver author figure out how to
> get at the right connector and pass on the required information.
>
> I don't really understand what you're even suggesting as an alternative.
> We need to know what the sink supports, and if your driver is architected
> in some crazy way where you don't even have that information where it's
> needed, I don't think there's anything anyone can do.
>

I submitted a RFC yesterday where there was an alternative
[1][2][3][4][5][6]: Don't expose 2.0 modes to drivers and to
userspace unless asked to. The infoframe part is a different
topic though. We could add a helper in the DRM EDID which checks
if sink is 2.0 and then, in conjunction with the flag I
introduced in the RFC patch 4/5 we would know that driver
supports 2.0 and sink is 2.0, then we would eliminate the bool.
What do you think?

Best regards,
Jose Miguel Abreu

[1]
https://lists.freedesktop.org/archives/dri-devel/2017-March/136596.html
[2] https://patchwork.kernel.org/patch/9640215/
[3] https://patchwork.kernel.org/patch/9640219/
[4] https://patchwork.kernel.org/patch/9640207/
[5] https://patchwork.kernel.org/patch/9640217/
[6] https://patchwork.kernel.org/patch/9640205/
Ville Syrjälä March 23, 2017, 6:14 p.m. UTC | #18
On Thu, Mar 23, 2017 at 05:53:34PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 23-03-2017 17:42, Ville Syrjälä wrote:
> > On Thu, Mar 23, 2017 at 05:11:44PM +0000, Jose Abreu wrote:
> >> Hi Shashank,
> >>
> >>
> >> On 23-03-2017 16:43, Sharma, Shashank wrote:
> >>> Regards
> >>>
> >>> Shashank
> >>>
> >>>
> >>> On 3/23/2017 6:33 PM, Jose Abreu wrote:
> >>>> Hi Shashank,
> >>>>
> >>>>
> >>>> On 23-03-2017 16:08, Sharma, Shashank wrote:
> >>>>> Regards
> >>>>>
> >>>>> Shashank
> >>>>>
> >>>>>
> >>>>> On 3/23/2017 5:57 PM, Jose Abreu wrote:
> >>>>>> Hi Ville,
> >>>>>>
> >>>>>>
> >>>>>> On 23-03-2017 15:45, Ville Syrjälä wrote:
> >>>>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
> >>>>>>>> Hi Shashank,
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 23-03-2017 15:14, 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 the
> >>>>>>>>> connector->display_info
> >>>>>>>>> to check if the connected display is HDMI 2.0.
> >>>>>>>>>
> >>>>>>>>> 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
> >>>>>>>>>
> >>>>>>>>> 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         |  5 ++++-
> >>>>>>>>>    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, 38 insertions(+), 21 deletions(-)
> >>>>>>>>>
> >>>>>>>> [snip]
> >>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>>>> index af93f7a..5ff2886 100644
> >>>>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
> >>>>>>>>>            frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>>>>>>
> >>>>>>>> dw-hdmi controller has full support for HDMI 2.0 features.
> >>>>>>>> It all
> >>>>>>>> depends on the platform it is integrated.
> >>>>>>>>
> >>>>>>>> I think adding a parameter to
> >>>>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
> >>>>>>>> because of this case: A bridge can have support for HDMI 2.0
> >>>>>>>> features but the platform may limit this support. I guess it
> >>>>>>>> can
> >>>>>>>> happen in other drivers too.
> >>>>>>> Your driver is in full control of what gets passed here. So I
> >>>>>>> don't see
> >>>>>>> why that would be a problem.
> >>>>>>>
> >>>>>>> Also this doesn't really have anything to do with the
> >>>>>>> capabilities of
> >>>>>>> the source. All we want to make sure is that we don't send a
> >>>>>>> VIC the
> >>>>>>> sink will not understand.
> >>>>>>>
> >>>>>> But the driver is not aware of the platform limitations, its
> >>>>>> generic to the controller only. We could add a field in pdata
> >>>>>> which tells if platform is HDMI 2.0+ but what about other
> >>>>>> bridge
> >>>>>> drivers or HDMI drivers? They will have to replicate the same
> >>>>>> thing also.
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Jose Miguel Abreu
> >>>>> I think the driver would be aware of the platform's
> >>>>> capabilities, isn't it ?
> >>>>> Else how would it even decide which mode to allow, and which to
> >>>>> reject ?
> >>>> The DRM core propagates the mode to the chain of configuration
> >>>> before reaching the bridge driver also, there is a callback
> >>>> supplied by pdata (mode_valid) which can check if the mode is
> >>>> valid. (see
> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__cgit.freedesktop.org_-7Eairlied_linux_tree_drivers_gpu_drm_bridge_synopsys_dw-2Dhdmi.c-3Fh-3Ddrm-2Dnext-23n1740&d=DwID-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zC1cWRy6RDWimQG2o866yYylV7c5h_U4aGWjVtnIHH0&s=DPZA3OgLWOO299hjy9dBDBK66kmUGfZArxKbeHNMKOc&e=
> >>>> )
> >>>>
> >>>> Best regards,
> >>>> Jose Miguel Abreu
> >>>>
> >>>>
> >>>>> Regards
> >>>>> Shashank
> >>> Please correct me if my understanding is not right, but drivers
> >>> call mode_valid() to prune/reject modes which they cant support.
> >>> and they call drm_set_infoframe_from_videomode() function, when
> >>> they are going ahead to the modeset with a mode.
> >>> Why would a driver choose to do a modeset, which it could not
> >>> support (shouldn't mode_valid have dropped it in atomic_check()
> >>> or may be even edid_parsing time ?)
> >> Yes, thats all correct :) I got a little out of topic here, sorry.
> >>
> >> The thing I don't agree with is the adding of the parameter for
> >> the drm_hdmi_avi_infoframe_from_display_mode(), because I think
> >> its not enough. I will state why:
> >>
> >> Picture this scenario:
> >>     1) EDID is read from HDMI 2.0 sink
> >>     2) The 2.0 modes are probed and as there is no check for
> >> these modes (they are in the CEA table) they will be added to
> >> mode list
> >>     3) The modes are passed to userspace. This is a problem
> >> because userspace does not yet understand the new aspect ratios,
> >> but lets imagine it understands
> >>     4) User selects an HDMI 2.0 mode
> >>     5) The drm core checks if mode is valid with driver, the
> >> driver can say yes or no. Lets think it says yes because the
> >> platform supports HDMI 2.0 modes.
> >>     6) Mode is committed but VIC will be set to zero according to
> >> the patch
> > That's only because you haven't updated your driver to pass 'true'
> > as needed.
> >
> > The other option would be to pass the connector instead of a boolean,
> > but according to Shashank that would required a lot of plumbing changes
> > because many drivers don't have the connector immediately available
> > where they call these functions. And plumbing the connector through
> > probably requires intimate knowledge of each driver, and thus is not
> > something we would be confortable doing. Hence I think the bool is the
> > best short term solution, letting each driver author figure out how to
> > get at the right connector and pass on the required information.
> >
> > I don't really understand what you're even suggesting as an alternative.
> > We need to know what the sink supports, and if your driver is architected
> > in some crazy way where you don't even have that information where it's
> > needed, I don't think there's anything anyone can do.
> >
> 
> I submitted a RFC yesterday where there was an alternative
> [1][2][3][4][5][6]: Don't expose 2.0 modes to drivers and to
> userspace unless asked to.

I don't think that's a good plan. We already hit a massive snag with
the aspect ratio uapi changes. So we should aim for something that
requires no userspace changes. For this stuff I see no reason to
bother userspace with these details. A mode is just a mode, whether
it was specified in HDMI 2.0 or DMT or CVT is quite irrelevant.

> The infoframe part is a different
> topic though. We could add a helper in the DRM EDID which checks
> if sink is 2.0 and then,

But that needs the connector, which some drivers don't have ready
at hand. And we already have that information pre-parsed in the
display_info so we pretty much already have what you suggest.
The only thing is that we require the driver to get that information
since the infoframe code doesn't know how to do that for every
driver.

> in conjunction with the flag I
> introduced in the RFC patch 4/5 we would know that driver
> supports 2.0 and sink is 2.0, then we would eliminate the bool.
> What do you think?
> 
> Best regards,
> Jose Miguel Abreu
> 
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2017-March/136596.html
> [2] https://patchwork.kernel.org/patch/9640215/
> [3] https://patchwork.kernel.org/patch/9640219/
> [4] https://patchwork.kernel.org/patch/9640207/
> [5] https://patchwork.kernel.org/patch/9640217/
> [6] https://patchwork.kernel.org/patch/9640205/
Jose Abreu March 23, 2017, 6:35 p.m. UTC | #19
Hi Ville,


On 23-03-2017 18:14, Ville Syrjälä wrote:
> On Thu, Mar 23, 2017 at 05:53:34PM +0000, Jose Abreu wrote:
>> Hi Ville,
>>
>>
>> On 23-03-2017 17:42, Ville Syrjälä wrote:
>>> On Thu, Mar 23, 2017 at 05:11:44PM +0000, Jose Abreu wrote:
>>>> Hi Shashank,
>>>>
>>>>
>>>> On 23-03-2017 16:43, Sharma, Shashank wrote:
>>>>> Regards
>>>>>
>>>>> Shashank
>>>>>
>>>>>
>>>>> On 3/23/2017 6:33 PM, Jose Abreu wrote:
>>>>>> Hi Shashank,
>>>>>>
>>>>>>
>>>>>> On 23-03-2017 16:08, Sharma, Shashank wrote:
>>>>>>> Regards
>>>>>>>
>>>>>>> Shashank
>>>>>>>
>>>>>>>
>>>>>>> On 3/23/2017 5:57 PM, Jose Abreu wrote:
>>>>>>>> Hi Ville,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23-03-2017 15:45, Ville Syrjälä wrote:
>>>>>>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote:
>>>>>>>>>> Hi Shashank,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 23-03-2017 15:14, 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 the
>>>>>>>>>>> connector->display_info
>>>>>>>>>>> to check if the connected display is HDMI 2.0.
>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>>> 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         |  5 ++++-
>>>>>>>>>>>    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, 38 insertions(+), 21 deletions(-)
>>>>>>>>>>>
>>>>>>>>>> [snip]
>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>>>> index af93f7a..5ff2886 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>>>>>>>> @@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
>>>>>>>>>>>            frame.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>>>>>>>
>>>>>>>>>> dw-hdmi controller has full support for HDMI 2.0 features.
>>>>>>>>>> It all
>>>>>>>>>> depends on the platform it is integrated.
>>>>>>>>>>
>>>>>>>>>> I think adding a parameter to
>>>>>>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea
>>>>>>>>>> because of this case: A bridge can have support for HDMI 2.0
>>>>>>>>>> features but the platform may limit this support. I guess it
>>>>>>>>>> can
>>>>>>>>>> happen in other drivers too.
>>>>>>>>> Your driver is in full control of what gets passed here. So I
>>>>>>>>> don't see
>>>>>>>>> why that would be a problem.
>>>>>>>>>
>>>>>>>>> Also this doesn't really have anything to do with the
>>>>>>>>> capabilities of
>>>>>>>>> the source. All we want to make sure is that we don't send a
>>>>>>>>> VIC the
>>>>>>>>> sink will not understand.
>>>>>>>>>
>>>>>>>> But the driver is not aware of the platform limitations, its
>>>>>>>> generic to the controller only. We could add a field in pdata
>>>>>>>> which tells if platform is HDMI 2.0+ but what about other
>>>>>>>> bridge
>>>>>>>> drivers or HDMI drivers? They will have to replicate the same
>>>>>>>> thing also.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Jose Miguel Abreu
>>>>>>> I think the driver would be aware of the platform's
>>>>>>> capabilities, isn't it ?
>>>>>>> Else how would it even decide which mode to allow, and which to
>>>>>>> reject ?
>>>>>> The DRM core propagates the mode to the chain of configuration
>>>>>> before reaching the bridge driver also, there is a callback
>>>>>> supplied by pdata (mode_valid) which can check if the mode is
>>>>>> valid. (see
>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__cgit.freedesktop.org_-7Eairlied_linux_tree_drivers_gpu_drm_bridge_synopsys_dw-2Dhdmi.c-3Fh-3Ddrm-2Dnext-23n1740&d=DwID-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zC1cWRy6RDWimQG2o866yYylV7c5h_U4aGWjVtnIHH0&s=DPZA3OgLWOO299hjy9dBDBK66kmUGfZArxKbeHNMKOc&e=
>>>>>> )
>>>>>>
>>>>>> Best regards,
>>>>>> Jose Miguel Abreu
>>>>>>
>>>>>>
>>>>>>> Regards
>>>>>>> Shashank
>>>>> Please correct me if my understanding is not right, but drivers
>>>>> call mode_valid() to prune/reject modes which they cant support.
>>>>> and they call drm_set_infoframe_from_videomode() function, when
>>>>> they are going ahead to the modeset with a mode.
>>>>> Why would a driver choose to do a modeset, which it could not
>>>>> support (shouldn't mode_valid have dropped it in atomic_check()
>>>>> or may be even edid_parsing time ?)
>>>> Yes, thats all correct :) I got a little out of topic here, sorry.
>>>>
>>>> The thing I don't agree with is the adding of the parameter for
>>>> the drm_hdmi_avi_infoframe_from_display_mode(), because I think
>>>> its not enough. I will state why:
>>>>
>>>> Picture this scenario:
>>>>     1) EDID is read from HDMI 2.0 sink
>>>>     2) The 2.0 modes are probed and as there is no check for
>>>> these modes (they are in the CEA table) they will be added to
>>>> mode list
>>>>     3) The modes are passed to userspace. This is a problem
>>>> because userspace does not yet understand the new aspect ratios,
>>>> but lets imagine it understands
>>>>     4) User selects an HDMI 2.0 mode
>>>>     5) The drm core checks if mode is valid with driver, the
>>>> driver can say yes or no. Lets think it says yes because the
>>>> platform supports HDMI 2.0 modes.
>>>>     6) Mode is committed but VIC will be set to zero according to
>>>> the patch
>>> That's only because you haven't updated your driver to pass 'true'
>>> as needed.
>>>
>>> The other option would be to pass the connector instead of a boolean,
>>> but according to Shashank that would required a lot of plumbing changes
>>> because many drivers don't have the connector immediately available
>>> where they call these functions. And plumbing the connector through
>>> probably requires intimate knowledge of each driver, and thus is not
>>> something we would be confortable doing. Hence I think the bool is the
>>> best short term solution, letting each driver author figure out how to
>>> get at the right connector and pass on the required information.
>>>
>>> I don't really understand what you're even suggesting as an alternative.
>>> We need to know what the sink supports, and if your driver is architected
>>> in some crazy way where you don't even have that information where it's
>>> needed, I don't think there's anything anyone can do.
>>>
>> I submitted a RFC yesterday where there was an alternative
>> [1][2][3][4][5][6]: Don't expose 2.0 modes to drivers and to
>> userspace unless asked to.
> I don't think that's a good plan. We already hit a massive snag with
> the aspect ratio uapi changes. So we should aim for something that
> requires no userspace changes. For this stuff I see no reason to
> bother userspace with these details. A mode is just a mode, whether
> it was specified in HDMI 2.0 or DMT or CVT is quite irrelevant.

I agree that we should aim for no userspace changes but sometimes
they are necessary. Anyway, I was forgetting that the new aspect
ratios will not be exported, so I guess we are safe for now.

>
>> The infoframe part is a different
>> topic though. We could add a helper in the DRM EDID which checks
>> if sink is 2.0 and then,
> But that needs the connector, which some drivers don't have ready
> at hand. And we already have that information pre-parsed in the
> display_info so we pretty much already have what you suggest.
> The only thing is that we require the driver to get that information
> since the infoframe code doesn't know how to do that for every
> driver.

Right, so, no connector :/ I can check if dw-hdmi is in this
condition and send a patch to adjust the flag.

Best regards,
Jose Miguel Abreu

>
>> in conjunction with the flag I
>> introduced in the RFC patch 4/5 we would know that driver
>> supports 2.0 and sink is 2.0, then we would eliminate the bool.
>> What do you think?
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>> [1]
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_dri-2Ddevel_2017-2DMarch_136596.html&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=7cX3HPDl6xffu7Zfoz30XBwhOyvIZ1RzTTb1z35ziV0&e= 
>> [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640215_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=ZHn9oCaMsLmPoaHbthOZXA8x8ixy2jik_3ZZRrjNtOs&e= 
>> [3] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640219_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=Xk-h91zq4XWDElWy9unrTER7BgryuUyKBb6dOusgAVw&e= 
>> [4] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640207_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=ByJ1jqHxbvkcZp64T1FFO9C_MljhDpJzVQgPIywiQ74&e= 
>> [5] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640217_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=giBMu5vbko6VWhq-Ob90DDBhEcpRqFLeSgZmeNnOEX8&e= 
>> [6] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640205_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=cAN-hG4zuHeWTJSYg1ID2hCprnYuKeZDDtxIKsy81j8&e=
diff mbox

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index d4452d8..ab7a399 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1877,7 +1877,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 5b24e89..3c5fd83 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1861,7 +1861,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 d2590d7..c564dca 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1760,7 +1760,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 a2a8236..f9b77b8 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 9126d03..dcf15d7 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 af93f7a..5ff2886 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1146,7 +1146,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->hdmi_data.enc_out_format == YCBCR444)
 		frame.colorspace = HDMI_COLORSPACE_YUV444;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3b494c2..f9a4b08 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4664,12 +4664,14 @@  EXPORT_SYMBOL(drm_set_preferred_mode);
  *                                              data from a DRM display mode
  * @frame: HDMI AVI infoframe
  * @mode: DRM display mode
+ * @is_hdmi2: 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)
 {
 	int err;
 
@@ -4685,6 +4687,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 && 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 5243840..5cb44d4 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -781,7 +781,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 3eec74c..96644f3 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -458,11 +458,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 = 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);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
 		return;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 816a6f5..694f626 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1011,7 +1011,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 c262512..80c36af 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 b214663..49b42bf 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 006260d..b105f1c 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 ce2dcba..2128b8c 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 e9cbe26..5d81301 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -394,7 +394,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 c47b9cb..3e267dd 100644
--- a/drivers/gpu/drm/zte/zx_hdmi.c
+++ b/drivers/gpu/drm/zte/zx_hdmi.c
@@ -125,7 +125,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 a21d494..a4d174e7 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -348,7 +348,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);
 int
 drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
 					    const struct drm_display_mode *mode);