[PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D
Andrzej Hajda
a.hajda at samsung.com
Tue Jul 4 13:58:02 UTC 2017
On 04.07.2017 14:44, Ville Syrjälä wrote:
> On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote:
>> On 03.07.2017 21:19, ville.syrjala at linux.intel.com 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.
>> My impression from the specs is that it should be done only after
>> switching from 3d to 2d mode.
>> In such case we just need to remember previous mode, if it was 3d, empty
>> VSIF infoframe should be still generated for 2seconds.
>> No need to do guesses from EDID.
>> Am I right, or just missing something?
> This code has no idea about any 3D->2D transitions, trying to make it
> do that would just result in a lot of complexity. Much easier to just
> always send the infoframe.
With such approach I see following 'issues':
0. It does not follow advices from specs.
1. It changes behavior of old drivers, probably in harmless way but
still there can be some sinks which will stop working.
2. What if EDID does not advertises 3d/4k modes but the sink supports
it, in such case userspace can set 3d mode, but after switch 3d->2d
empty infoframe will not be sent.
3. With this patch connector is required to generate infoframe, but
there are pipelines where infoframe can be generated in non-connector
driver, for example:
crtc -> hdmi_encoder -> mhl_encoder -> connector
In such case encoder has no access to the connector, of course it can
violate abstraction layers and localize one, but it shows that something
here is probably wrong.
Maybe another helper drm_hdmi_vendor_infoframe_from_connector will be
enough to solve it.
4. Video mode provided by user has nothing to do with EDID, why
infoframe generated for that mode should depend on EDID.
All above 'issues' are not serious ones but suggests that the solution
is not the ideal one.
On the other side I do not see much complication in 3D->2D transition,
it requires just recording if 3d mode was set before and again
drm_hdmi_vendor_infoframe_from_connector can perform this check.
Regards
Andrzej
>
>>> 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.
>>> Cc: Archit Taneja <architt at codeaurora.org>
>>> Cc: Andrzej Hajda <a.hajda at samsung.com>
>>> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
>>> Cc: Inki Dae <inki.dae at samsung.com>
>>> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
>>> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
>>> Cc: Kyungmin Park <kyungmin.park at samsung.com>
>>> Cc: CK Hu <ck.hu at mediatek.com>
>>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>>> Cc: Ben Skeggs <bskeggs at redhat.com>
>>> Cc: Mark Yao <mark.yao at rock-chips.com>
>>> Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>>> Cc: Vincent Abriou <vincent.abriou at st.com>
>>> Cc: Shawn Guo <shawnguo at kernel.org>
>>> Cc: Shashank Sharma <shashank.sharma at intel.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 | 32 +++++++++++++++++++++++++------
>>> 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, 55 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> index 2d51a2269fc6..758b5a4546f1 100644
>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> @@ -2136,8 +2136,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;
>> Usage of drm_hdmi_vendor_infoframe_from_display_mode is just dirty
>> workaround to get hdmi vic from mode (drm_match_hdmi_mode is static,
>> opposite to drm_match_cea_mode).
> Ah. I guess I should have read more than the single line of code :)
>
>> I guess better solution is to export drm_match_hdmi_mode, or wait for
>> extending cea table with HDMI2.0 modes, I plan to fix it after merging
>> HDMI2.0 patches.
> OK. Cool. That means we can nuke the NULL connector handling from
> drm_hdmi_vendor_infoframe_from_display_mode() at that point.
>
>> So beside the comment, the change looks OK.
>>
>>> 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 ead11242c4b9..c43389774691 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -1426,7 +1426,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 2e55599816aa..c061dd5d25c0 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -3081,6 +3081,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;
>>> @@ -3208,6 +3209,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;
>>> }
>>>
>>> @@ -3829,6 +3832,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)
>>> @@ -3998,6 +4003,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;
>>> @@ -4452,6 +4458,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
>>> * @mode: DRM display mode
>>> *
>>> * Note that there's is a need to send HDMI vendor infoframes only when using a
>>> @@ -4462,8 +4469,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 : NULL;
>> I wonder if requiring connector is a good idea, I can imagine that this
>> function can be necessary in pure drm_encoder or non-terminal drm_bridge.
> No decent way around it, unless we want to risk sending the infoframe to
> pre HDMI 1.4 sinks. Sounds like you have some kind of layering problem
> if you can't get at the connector when you call this.
>
>> Regards
>> Andrzej
>>
>>> int err;
>>> u32 s3d_flags;
>>> u8 vic;
>>> @@ -4474,20 +4488,26 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>> vic = drm_match_hdmi_mode(mode);
>>> s3d_flags = mode->flags & DRM_MODE_FLAG_3D_MASK;
>>>
>>> - if (!vic && !s3d_flags)
>>> + if (vic && s3d_flags)
>>> return -EINVAL;
>>>
>>> - if (vic && s3d_flags)
>>> + /*
>>> + * Even if it's not absolutely necessary to send the infoframe
>>> + * 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 && !has_hdmi_infoframe)
>>> return -EINVAL;
>>>
>>> err = hdmi_vendor_infoframe_init(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 06bfbe400cf1..28754744a7cf 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 ec0779a52d53..a0f001418248 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -497,12 +497,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;
>>> @@ -569,7 +571,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)
>>> @@ -710,7 +712,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,
>>> @@ -753,7 +755,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,
>>> @@ -806,7 +808,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,
>>> @@ -839,7 +841,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 0a4ffd724146..07e618981a5c 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 42a85c14aea0..1b5fc837e35e 100644
>>> --- a/drivers/gpu/drm/nouveau/nv50_display.c
>>> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
>>> @@ -2769,7 +2769,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 7d9b75eb6c44..0126c5bdceab 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 a59c95a8081b..fa6e9b37b136 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 0df7366e594b..84487332e5c6 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 ae5b7dc316c8..b18939280616 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -247,6 +247,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 7b9f48b62e07..b0f37708b5ab 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -346,6 +346,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>> const struct drm_display_mode *mode);
>>> int
>>> drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>> + struct drm_connector *connector,
>>> const struct drm_display_mode *mode);
>>> void
>>> drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
More information about the dri-devel
mailing list