[PATCH 02/10] drm/edid: Allow HDMI infoframe without VIC or S3D
Sharma, Shashank
shashank.sharma at intel.com
Fri Nov 17 03:10:02 UTC 2017
Regards
Shashank
On 11/16/2017 9:51 PM, Ville Syrjälä wrote:
> On Thu, Nov 16, 2017 at 08:10:55PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
>>> 3D to 2D mode in a timely fashion if the source simply stops sending the
>>> HDMI infoframe. The suggested workaround is to keep sending the
>>> infoframe even when strictly not necessary (ie. no VIC and no S3D).
>>> HDMI 1.4 does allow for this behaviour, stating that sending the
>>> infoframe is optional in this case.
>>>
>>> The infoframe was first specified in HDMI 1.4, so in theory sinks
>>> predating that may not appreciate us sending an uknown infoframe
>>> their way. To avoid regressions let's try to determine if the sink
>>> supports the infoframe or not. Unfortunately there's no direct way
>>> to do that, so instead we'll just check if we managed to parse any
>>> HDMI 1.4 4k or stereo modes from the EDID, and if so we assume the
>>> sink will accept the infoframe. Also if the EDID contains the HDMI
>>> 2.0 HDMI Forum VSDB we can assume the sink is prepared to receive
>>> the infoframe.
>> I am trying to get some sense from HDMI 2.0 spec section 10.2.1, which
>> talks about
>> switch from 3D to 2D. To me it looks like:
>> If (sending_to_hdmi2_sinks) {
>> - for 3d modes send HF-VSIF
>> - for 2d modes && defined in H14b HFVSIF, send H14B-VSIF
>> When you switch from 3d->2d {
>> - send_HF-VSIF with 3D_valid bit = 0/1
>> }
>> } else { /* HDMI 1.4b sinks from Appendix F */
>> - send H14b-VSIF with HDMI_video_format[2:0 = 0 OR 1]
>> }
>>
>> Should we add a is_hdmi2 check and separate these cases ?
> We don't support the HDMI forum infoframe. Maybe someone forgot to
> implement that when adding the rest of HDMI 2.0 support? ;)
How to make an 'embarrassed smile ' smiley :) ?
Will start looking into it. Meanwhile
Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>>> v2: Fix the getting has_hdmi_infoframe from display_info
>>> Always fail constructing the infoframe if the display
>>> possibly can't handle it
>>>
>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>> Cc: Andrzej Hajda <a.hajda at samsung.com>
>>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/bridge/sil-sii8620.c | 3 ++-
>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +++-
>>> drivers/gpu/drm/drm_edid.c | 34 +++++++++++++++++++++++++------
>>> drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +-
>>> drivers/gpu/drm/i915/intel_hdmi.c | 14 +++++++------
>>> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++-
>>> drivers/gpu/drm/nouveau/nv50_display.c | 3 ++-
>>> drivers/gpu/drm/rockchip/inno_hdmi.c | 1 +
>>> drivers/gpu/drm/sti/sti_hdmi.c | 4 +++-
>>> drivers/gpu/drm/zte/zx_hdmi.c | 1 +
>>> include/drm/drm_connector.h | 5 +++++
>>> include/drm/drm_edid.h | 1 +
>>> 12 files changed, 57 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> index b7eb704d0a8a..4417276ba02e 100644
>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> @@ -2220,8 +2220,9 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>> union hdmi_infoframe frm;
>>> u8 mhl_vic[] = { 0, 95, 94, 93, 98 };
>>>
>>> + /* FIXME: We need the connector here */
>>> drm_hdmi_vendor_infoframe_from_display_mode(
>>> - &frm.vendor.hdmi, adjusted_mode);
>>> + &frm.vendor.hdmi, NULL, adjusted_mode);
>>> vic = frm.vendor.hdmi.vic;
>>> if (vic >= ARRAY_SIZE(mhl_vic))
>>> vic = 0;
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index a64ce7112288..b172139502d6 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -1437,7 +1437,9 @@ static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>>> u8 buffer[10];
>>> ssize_t err;
>>>
>>> - err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
>>> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame,
>>> + &hdmi->connector,
>>> + mode);
>>> if (err < 0)
>>> /*
>>> * Going into that statement does not means vendor infoframe
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 749d07a01772..9ada0ccf50df 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -3393,6 +3393,7 @@ static int
>>> do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>>> const u8 *video_db, u8 video_len)
>>> {
>>> + struct drm_display_info *info = &connector->display_info;
>>> int modes = 0, offset = 0, i, multi_present = 0, multi_len;
>>> u8 vic_len, hdmi_3d_len = 0;
>>> u16 mask;
>>> @@ -3520,6 +3521,8 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>>> }
>>>
>>> out:
>>> + if (modes > 0)
>>> + info->has_hdmi_infoframe = true;
>>> return modes;
>>> }
>>>
>>> @@ -4243,6 +4246,8 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>>> struct drm_display_info *display = &connector->display_info;
>>> struct drm_hdmi_info *hdmi = &display->hdmi;
>>>
>>> + display->has_hdmi_infoframe = true;
>>> +
>>> if (hf_vsdb[6] & 0x80) {
>>> hdmi->scdc.supported = true;
>>> if (hf_vsdb[6] & 0x40)
>>> @@ -4416,6 +4421,7 @@ static void drm_add_display_info(struct drm_connector *connector,
>>> info->cea_rev = 0;
>>> info->max_tmds_clock = 0;
>>> info->dvi_dual = false;
>>> + info->has_hdmi_infoframe = false;
>>>
>>> if (edid->revision < 3)
>>> return;
>>> @@ -4903,6 +4909,7 @@ s3d_structure_from_display_mode(const struct drm_display_mode *mode)
>>> * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI infoframe with
>>> * data from a DRM display mode
>>> * @frame: HDMI vendor infoframe
>>> + * @connector: the connector
>> I remember our old discussion where we realized that we will need this
>> connector for all types of infoframes,
>> eventually, do you think we should start thinking about creating a
>> wrapper like drm_any_infoframe_from_display_mode(),
>> and pass connector and desired type to it ?Or I am getting too ambitious
>> targeting this series :) ?
>>> * @mode: DRM display mode
>>> *
>>> * Note that there's is a need to send HDMI vendor infoframes only when using a
>>> @@ -4913,8 +4920,15 @@ s3d_structure_from_display_mode(const struct drm_display_mode *mode)
>>> */
>>> int
>>> drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>> + struct drm_connector *connector,
>>> const struct drm_display_mode *mode)
>>> {
>>> + /*
>>> + * FIXME: sil-sii8620 doesn't have a connector around when
>>> + * we need one, so we have to be prepared for a NULL connector.
>>> + */
>>> + bool has_hdmi_infoframe = connector ?
>>> + connector->display_info.has_hdmi_infoframe : false;
>>> int err;
>>> u32 s3d_flags;
>>> u8 vic;
>>> @@ -4922,11 +4936,21 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>> if (!frame || !mode)
>>> return -EINVAL;
>>>
>>> + if (!has_hdmi_infoframe)
>>> + return -EINVAL;
>>> +
>>> vic = drm_match_hdmi_mode(mode);
>>> s3d_flags = mode->flags & DRM_MODE_FLAG_3D_MASK;
>>>
>>> - if (!vic && !s3d_flags)
>>> - return -EINVAL;
>>> + /*
>>> + * Even if it's not absolutely necessary to send the infoframe
>>> + * (ie.vic==0 and s3d_struct==0) we will still send it if we
>>> + * know that the sink can handle it. This is based on a
>>> + * suggestion in HDMI 2.0 Appendix F. Apparently some sinks
>>> + * have trouble realizing that they shuld switch from 3D to 2D
>>> + * mode if the source simply stops sending the infoframe when
>>> + * it wants to switch from 3D to 2D.
>>> + */
>>>
>>> if (vic && s3d_flags)
>>> return -EINVAL;
>>> @@ -4935,10 +4959,8 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>> if (err < 0)
>>> return err;
>>>
>>> - if (vic)
>>> - frame->vic = vic;
>>> - else
>>> - frame->s3d_struct = s3d_structure_from_display_mode(mode);
>>> + frame->vic = vic;
>>> + frame->s3d_struct = s3d_structure_from_display_mode(mode);
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> index 0109ff40b1db..812b2773ed69 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> @@ -795,7 +795,7 @@ static void hdmi_reg_infoframes(struct hdmi_context *hdata)
>>> }
>>>
>>> ret = drm_hdmi_vendor_infoframe_from_display_mode(&frm.vendor.hdmi,
>>> - &hdata->current_mode);
>>> + &hdata->connector, &hdata->current_mode);
>>> if (!ret)
>>> ret = hdmi_vendor_infoframe_pack(&frm.vendor.hdmi, buf,
>>> sizeof(buf));
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index 2d95db64cdf2..2ccba4ccf7ad 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -512,12 +512,14 @@ static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder,
>>>
>>> static void
>>> intel_hdmi_set_hdmi_infoframe(struct drm_encoder *encoder,
>>> - const struct intel_crtc_state *crtc_state)
>>> + const struct intel_crtc_state *crtc_state,
>>> + const struct drm_connector_state *conn_state)
>>> {
>>> union hdmi_infoframe frame;
>>> int ret;
>>>
>>> ret = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
>>> + conn_state->connector,
>>> &crtc_state->base.adjusted_mode);
>>> if (ret < 0)
>>> return;
>>> @@ -584,7 +586,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
>>>
>>> intel_hdmi_set_avi_infoframe(encoder, crtc_state);
>>> intel_hdmi_set_spd_infoframe(encoder, crtc_state);
>>> - intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
>>> + intel_hdmi_set_hdmi_infoframe(encoder, crtc_state, conn_state);
>>> }
>>>
>>> static bool hdmi_sink_is_deep_color(const struct drm_connector_state *conn_state)
>>> @@ -725,7 +727,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
>>>
>>> intel_hdmi_set_avi_infoframe(encoder, crtc_state);
>>> intel_hdmi_set_spd_infoframe(encoder, crtc_state);
>>> - intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
>>> + intel_hdmi_set_hdmi_infoframe(encoder, crtc_state, conn_state);
>>> }
>>>
>>> static void cpt_set_infoframes(struct drm_encoder *encoder,
>>> @@ -768,7 +770,7 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
>>>
>>> intel_hdmi_set_avi_infoframe(encoder, crtc_state);
>>> intel_hdmi_set_spd_infoframe(encoder, crtc_state);
>>> - intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
>>> + intel_hdmi_set_hdmi_infoframe(encoder, crtc_state, conn_state);
>>> }
>>>
>>> static void vlv_set_infoframes(struct drm_encoder *encoder,
>>> @@ -821,7 +823,7 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>>>
>>> intel_hdmi_set_avi_infoframe(encoder, crtc_state);
>>> intel_hdmi_set_spd_infoframe(encoder, crtc_state);
>>> - intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
>>> + intel_hdmi_set_hdmi_infoframe(encoder, crtc_state, conn_state);
>>> }
>>>
>>> static void hsw_set_infoframes(struct drm_encoder *encoder,
>>> @@ -854,7 +856,7 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
>>>
>>> intel_hdmi_set_avi_infoframe(encoder, crtc_state);
>>> intel_hdmi_set_spd_infoframe(encoder, crtc_state);
>>> - intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
>>> + intel_hdmi_set_hdmi_infoframe(encoder, crtc_state, conn_state);
>>> }
>>>
>>> void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable)
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
>>> index b78791061983..59a11026dceb 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
>>> @@ -1054,7 +1054,8 @@ static int mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
>>> u8 buffer[10];
>>> ssize_t err;
>>>
>>> - err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
>>> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame,
>>> + &hdmi->conn, mode);
>>> if (err) {
>>> dev_err(hdmi->dev,
>>> "Failed to get vendor infoframe from mode: %zd\n", err);
>>> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
>>> index b26a506d20ca..9d7b2afd7cfc 100644
>>> --- a/drivers/gpu/drm/nouveau/nv50_display.c
>>> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
>>> @@ -2754,7 +2754,8 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
>>> = hdmi_infoframe_pack(&avi_frame, args.infoframes, 17);
>>> }
>>>
>>> - ret = drm_hdmi_vendor_infoframe_from_display_mode(&vendor_frame.vendor.hdmi, mode);
>>> + ret = drm_hdmi_vendor_infoframe_from_display_mode(&vendor_frame.vendor.hdmi,
>>> + &nv_connector->base, mode);
>>> if (!ret) {
>>> /* We have a Vendor InfoFrame, populate it to the display */
>>> args.pwr.vendor_infoframe_length
>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>> index ee584d87111f..fab30927a889 100644
>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>> @@ -282,6 +282,7 @@ static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi,
>>> int rc;
>>>
>>> rc = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
>>> + &hdmi->connector,
>>> mode);
>>>
>>> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_VSI,
>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>>> index d1902750a85d..c3b292ab17a5 100644
>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>>> @@ -515,7 +515,9 @@ static int hdmi_vendor_infoframe_config(struct sti_hdmi *hdmi)
>>>
>>> DRM_DEBUG_DRIVER("\n");
>>>
>>> - ret = drm_hdmi_vendor_infoframe_from_display_mode(&infoframe, mode);
>>> + ret = drm_hdmi_vendor_infoframe_from_display_mode(&infoframe,
>>> + hdmi->drm_connector,
>>> + mode);
>>> if (ret < 0) {
>>> /*
>>> * Going into that statement does not means vendor infoframe
>>> diff --git a/drivers/gpu/drm/zte/zx_hdmi.c b/drivers/gpu/drm/zte/zx_hdmi.c
>>> index b8abb1b496ff..13ea90f7a185 100644
>>> --- a/drivers/gpu/drm/zte/zx_hdmi.c
>>> +++ b/drivers/gpu/drm/zte/zx_hdmi.c
>>> @@ -108,6 +108,7 @@ static int zx_hdmi_config_video_vsi(struct zx_hdmi *hdmi,
>>> int ret;
>>>
>>> ret = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
>>> + &hdmi->connector,
>>> mode);
>>> if (ret) {
>>> DRM_DEV_ERROR(hdmi->dev, "failed to get vendor infoframe: %d\n",
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 2b97d1e28f60..1543212b0449 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -270,6 +270,11 @@ struct drm_display_info {
>>> bool dvi_dual;
>>>
>>> /**
>>> + * @has_hdmi_infoframe: Does the sink support the HDMI infoframe?
>>> + */
>>> + bool has_hdmi_infoframe;
>>> +
>>> + /**
>>> * @edid_hdmi_dc_modes: Mask of supported hdmi deep color modes. Even
>>> * more stuff redundant with @bus_formats.
>>> */
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index 9e4e23524840..3c8740ad1db6 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -356,6 +356,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>> bool is_hdmi2_sink);
>>> int
>>> drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>> + struct drm_connector *connector,
>>> const struct drm_display_mode *mode);
>>> void
>>> drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>> Otherwise looks good
>> - Shashank
More information about the dri-devel
mailing list