* [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes @ 2017-07-03 19:19 ` ville.syrjala 2017-07-03 19:19 ` [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D ville.syrjala ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: ville.syrjala @ 2017-07-03 19:19 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> HDMI 2.0 Appendix F suggest that we should keep sending the infoframe when switching from 3D to 2D mode, even if the infoframe isn't strictly necessary (ie. not needed to transmit the VIC or stereo information). This is a workaround against some sinks that fail to realize that they should switch from 3D to 2D mode when the source stop transmitting the infoframe. Cc: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/video/hdmi.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 1cf907ecded4..af0c8dad29eb 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -341,10 +341,6 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, u8 *ptr = buffer; size_t length; - /* empty info frame */ - if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) - return -EINVAL; - /* only one of those can be supplied */ if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) return -EINVAL; @@ -352,8 +348,10 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, /* for side by side (half) we also need to provide 3D_Ext_Data */ if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) frame->length = 6; - else + else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) frame->length = 5; + else + frame->length = 4; length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; @@ -372,14 +370,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, ptr[5] = 0x0c; ptr[6] = 0x00; - if (frame->vic) { - ptr[7] = 0x1 << 5; /* video format */ - ptr[8] = frame->vic; - } else { + if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) { ptr[7] = 0x2 << 5; /* video format */ ptr[8] = (frame->s3d_struct & 0xf) << 4; if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) ptr[9] = (frame->s3d_ext_data & 0xf) << 4; + } else if (frame->vic) { + ptr[7] = 0x1 << 5; /* video format */ + ptr[8] = frame->vic; + } else { + ptr[7] = 0x0 << 5; /* video format */ } hdmi_infoframe_set_checksum(buffer, length); -- 2.13.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-03 19:19 ` [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes ville.syrjala @ 2017-07-03 19:19 ` ville.syrjala 2017-07-04 10:16 ` Ville Syrjälä 2017-07-04 11:56 ` Andrzej Hajda 2017-07-03 19:39 ` ✓ Fi.CI.BAT: success for series starting with [1/2] video/hdmi: Allow "empty" HDMI infoframes Patchwork 2017-07-04 9:41 ` [PATCH 1/2] " Andrzej Hajda 2 siblings, 2 replies; 14+ messages in thread From: ville.syrjala @ 2017-07-03 19:19 UTC (permalink / raw) To: dri-devel Cc: Archit Taneja, Joonyoung Shim, Benjamin Gaignard, Shawn Guo, intel-gfx, Seung-Woo Kim, Inki Dae, Andrzej Hajda, Kyungmin Park, Laurent Pinchart, Philipp Zabel, CK Hu, Mark Yao, Vincent Abriou, Ben Skeggs From: Ville Syrjälä <ville.syrjala@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. Cc: Archit Taneja <architt@codeaurora.org> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Inki Dae <inki.dae@samsung.com> Cc: Joonyoung Shim <jy0922.shim@samsung.com> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> Cc: Kyungmin Park <kyungmin.park@samsung.com> Cc: CK Hu <ck.hu@mediatek.com> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: Mark Yao <mark.yao@rock-chips.com> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> Cc: Vincent Abriou <vincent.abriou@st.com> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@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; 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; 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, -- 2.13.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-03 19:19 ` [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D ville.syrjala @ 2017-07-04 10:16 ` Ville Syrjälä 2017-07-04 11:56 ` Andrzej Hajda 1 sibling, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-07-04 10:16 UTC (permalink / raw) To: dri-devel Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, Kyungmin Park, Laurent Pinchart, Vincent Abriou, Ben Skeggs On Mon, Jul 03, 2017 at 10:19:38PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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. > > Cc: Archit Taneja <architt@codeaurora.org> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: CK Hu <ck.hu@mediatek.com> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Mark Yao <mark.yao@rock-chips.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Vincent Abriou <vincent.abriou@st.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@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; > 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; Oh, and I forgot to mention that this depends on Shashank's patch to reorder things such that drm_add_display_info() gets called prior to parsing the modes. With the current order we'd end up clearing this almost immediately afterwards. > 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; -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-03 19:19 ` [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D ville.syrjala 2017-07-04 10:16 ` Ville Syrjälä @ 2017-07-04 11:56 ` Andrzej Hajda 2017-07-04 12:44 ` Ville Syrjälä 1 sibling, 1 reply; 14+ messages in thread From: Andrzej Hajda @ 2017-07-04 11:56 UTC (permalink / raw) To: ville.syrjala, dri-devel Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, Kyungmin Park, Laurent Pinchart, Vincent Abriou, Ben Skeggs On 03.07.2017 21:19, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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? > > 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@codeaurora.org> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: CK Hu <ck.hu@mediatek.com> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Mark Yao <mark.yao@rock-chips.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Vincent Abriou <vincent.abriou@st.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@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). 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. 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. 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, _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-04 11:56 ` Andrzej Hajda @ 2017-07-04 12:44 ` Ville Syrjälä 2017-07-04 13:58 ` Andrzej Hajda 2017-07-05 8:46 ` Laurent Pinchart 0 siblings, 2 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-07-04 12:44 UTC (permalink / raw) To: Andrzej Hajda Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Kyungmin Park, Laurent Pinchart, Vincent Abriou, Ben Skeggs On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote: > On 03.07.2017 21:19, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@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. > > > > > 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@codeaurora.org> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > Cc: Inki Dae <inki.dae@samsung.com> > > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > Cc: CK Hu <ck.hu@mediatek.com> > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Cc: Mark Yao <mark.yao@rock-chips.com> > > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > Cc: Vincent Abriou <vincent.abriou@st.com> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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, > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-04 12:44 ` Ville Syrjälä @ 2017-07-04 13:58 ` Andrzej Hajda 2017-07-04 14:25 ` Ville Syrjälä 2017-07-05 8:46 ` Laurent Pinchart 1 sibling, 1 reply; 14+ messages in thread From: Andrzej Hajda @ 2017-07-04 13:58 UTC (permalink / raw) To: Ville Syrjälä Cc: Archit Taneja, Joonyoung Shim, Benjamin Gaignard, Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Inki Dae, Kyungmin Park, Laurent Pinchart, Philipp Zabel, CK Hu, Mark Yao, Vincent Abriou, Ben Skeggs 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@linux.intel.com wrote: >>> From: Ville Syrjälä <ville.syrjala@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@codeaurora.org> >>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> >>> Cc: Inki Dae <inki.dae@samsung.com> >>> Cc: Joonyoung Shim <jy0922.shim@samsung.com> >>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> >>> Cc: Kyungmin Park <kyungmin.park@samsung.com> >>> Cc: CK Hu <ck.hu@mediatek.com> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de> >>> Cc: Ben Skeggs <bskeggs@redhat.com> >>> Cc: Mark Yao <mark.yao@rock-chips.com> >>> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> >>> Cc: Vincent Abriou <vincent.abriou@st.com> >>> Cc: Shawn Guo <shawnguo@kernel.org> >>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@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, _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-04 13:58 ` Andrzej Hajda @ 2017-07-04 14:25 ` Ville Syrjälä 2017-07-04 15:09 ` Andrzej Hajda 0 siblings, 1 reply; 14+ messages in thread From: Ville Syrjälä @ 2017-07-04 14:25 UTC (permalink / raw) To: Andrzej Hajda Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Kyungmin Park, Laurent Pinchart, Vincent Abriou, Ben Skeggs On Tue, Jul 04, 2017 at 03:58:02PM +0200, Andrzej Hajda wrote: > 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@linux.intel.com wrote: > >>> From: Ville Syrjälä <ville.syrjala@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. Sure it does. The spec says you should keep sending it for at least two seconds, but it doesn't say that you can't keep sending it for longer, or even when there are no 3D->2D transitions. > 1. It changes behavior of old drivers, probably in harmless way but > still there can be some sinks which will stop working. I think this is a justified risk. If we start to worry too much about every little change we stop making progress altogether. I did minimize the danger by making sure we don't send it for pre-1.4 sinks, and I was almost leaning towards not checking for that. But then I saw some language in the spec which might be interpreted to mean that the source isn't allowed to send unknown infoframe types to the sink, and I figured that maybe it's better to play it safe. > 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. IMO no point in worrying about broken EDIDs until one is proven to exist. > 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. Just pass the connector down if needed. We'll need the connector to decide whether to send the new CEA-864-F VICs or not as well. And you could need it (well really the connector state) to figure out the value if some connector properties and whanot. Also I didn't actually run into any cases where the connector is unavailable in the tree. > 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. Well, then we'd have to pass in the old and new crtc/connector states etc. We don't want to start maintaining some custom state in the infoframe helpers that bypasses the normal atomic state handling altogether. And what causes us to stop sending it after the 2 seconds? Is two second the correct amount of time or should we do it longer? etc. So this line of thinking leads to a lot of new problems we can avoid by keeping it simple. -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-04 14:25 ` Ville Syrjälä @ 2017-07-04 15:09 ` Andrzej Hajda 2017-07-04 15:20 ` Ville Syrjälä 0 siblings, 1 reply; 14+ messages in thread From: Andrzej Hajda @ 2017-07-04 15:09 UTC (permalink / raw) To: Ville Syrjälä Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Kyungmin Park, Laurent Pinchart, Vincent Abriou, Ben Skeggs On 04.07.2017 16:25, Ville Syrjälä wrote: > On Tue, Jul 04, 2017 at 03:58:02PM +0200, Andrzej Hajda wrote: >> 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@linux.intel.com wrote: >>>>> From: Ville Syrjälä <ville.syrjala@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. > Sure it does. The spec says you should keep sending it for at least two > seconds, but it doesn't say that you can't keep sending it for longer, > or even when there are no 3D->2D transitions. > >> 1. It changes behavior of old drivers, probably in harmless way but >> still there can be some sinks which will stop working. > I think this is a justified risk. If we start to worry too much about > every little change we stop making progress altogether. I did minimize > the danger by making sure we don't send it for pre-1.4 sinks, and > I was almost leaning towards not checking for that. But then I saw some > language in the spec which might be interpreted to mean that the source > isn't allowed to send unknown infoframe types to the sink, and I figured > that maybe it's better to play it safe. > >> 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. > IMO no point in worrying about broken EDIDs until one is proven to > exist. I though the whole 'fake edid' interface, possibility to provide custom mode, and avoiding connector:mode_valid in such case is enough proof of existence of such EDIDs. Internet is also full of advices how to force certain mode (also 3d mode), even if EDID does not advertise it. > >> 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. > Just pass the connector down if needed. We'll need the connector to > decide whether to send the new CEA-864-F VICs or not as well. And you > could need it (well really the connector state) to figure out the value > if some connector properties and whanot. > > Also I didn't actually run into any cases where the connector is > unavailable in the tree. Not unavailable, just requires digging across layers. > >> 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. > Well, then we'd have to pass in the old and new crtc/connector states > etc. We don't want to start maintaining some custom state in the > infoframe helpers that bypasses the normal atomic state handling > altogether. I understand, on the other side the issue the patch tries to solve has such nature. > > And what causes us to stop sending it after the 2 seconds? Is two > second the correct amount of time or should we do it longer? etc. I though here about simpler approach: to send frame always after 3d mode was ever used. Approach with 2s timeout would require more work on drivers side. > > So this line of thinking leads to a lot of new problems we can avoid > by keeping it simple. > I have pointed out some weakness IMO, but as I said earlier they are non-blockers, so if you still think this approach is better it is OK. Regards Andrzej _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-04 15:09 ` Andrzej Hajda @ 2017-07-04 15:20 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-07-04 15:20 UTC (permalink / raw) To: Andrzej Hajda Cc: Archit Taneja, Joonyoung Shim, Benjamin Gaignard, Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Inki Dae, Kyungmin Park, Laurent Pinchart, Philipp Zabel, CK Hu, Mark Yao, Vincent Abriou, Ben Skeggs On Tue, Jul 04, 2017 at 05:09:36PM +0200, Andrzej Hajda wrote: > On 04.07.2017 16:25, Ville Syrjälä wrote: > > On Tue, Jul 04, 2017 at 03:58:02PM +0200, Andrzej Hajda wrote: > >> 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@linux.intel.com wrote: > >>>>> From: Ville Syrjälä <ville.syrjala@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. > > Sure it does. The spec says you should keep sending it for at least two > > seconds, but it doesn't say that you can't keep sending it for longer, > > or even when there are no 3D->2D transitions. > > > >> 1. It changes behavior of old drivers, probably in harmless way but > >> still there can be some sinks which will stop working. > > I think this is a justified risk. If we start to worry too much about > > every little change we stop making progress altogether. I did minimize > > the danger by making sure we don't send it for pre-1.4 sinks, and > > I was almost leaning towards not checking for that. But then I saw some > > language in the spec which might be interpreted to mean that the source > > isn't allowed to send unknown infoframe types to the sink, and I figured > > that maybe it's better to play it safe. > > > >> 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. > > IMO no point in worrying about broken EDIDs until one is proven to > > exist. > > I though the whole 'fake edid' interface, possibility to provide custom > mode, and avoiding connector:mode_valid in such case is enough proof of > existence of such EDIDs. It's proof that there are buggy EDIDs in general. It isn't proof of displays that can do 3D + don't advertize 3D/4k + require the 2D infoframe to switch out of 3D mode. > Internet is also full of advices how to force certain mode (also 3d > mode), even if EDID does not advertise it. If people insist on shooting themselves in the foot they can. > > > > >> 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. > > Just pass the connector down if needed. We'll need the connector to > > decide whether to send the new CEA-864-F VICs or not as well. And you > > could need it (well really the connector state) to figure out the value > > if some connector properties and whanot. > > > > Also I didn't actually run into any cases where the connector is > > unavailable in the tree. > > Not unavailable, just requires digging across layers. > > > > >> 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. > > Well, then we'd have to pass in the old and new crtc/connector states > > etc. We don't want to start maintaining some custom state in the > > infoframe helpers that bypasses the normal atomic state handling > > altogether. > > I understand, on the other side the issue the patch tries to solve has > such nature. > > > > > And what causes us to stop sending it after the 2 seconds? Is two > > second the correct amount of time or should we do it longer? etc. > > I though here about simpler approach: to send frame always after 3d mode > was ever used. That would just result in inconsistent behaviour depending on whether you start with the 2D mode from the get go, or if you start with 3D and later switch to 2D. That kind of inconsistent behaviour just leads to harder to analyze bugs in my experience. > Approach with 2s timeout would require more work on drivers side. > > > > > So this line of thinking leads to a lot of new problems we can avoid > > by keeping it simple. > > > I have pointed out some weakness IMO, but as I said earlier they are > non-blockers, so if you still think this approach is better it is OK. > > > Regards > > Andrzej > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-04 12:44 ` Ville Syrjälä 2017-07-04 13:58 ` Andrzej Hajda @ 2017-07-05 8:46 ` Laurent Pinchart 2017-07-05 11:48 ` Ville Syrjälä 1 sibling, 1 reply; 14+ messages in thread From: Laurent Pinchart @ 2017-07-05 8:46 UTC (permalink / raw) To: Ville Syrjälä Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Kyungmin Park, Ben Skeggs, Vincent Abriou Hi Ville, On Tuesday 04 Jul 2017 15:44:02 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@linux.intel.com wrote: > >> From: Ville Syrjälä <ville.syrjala@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. > > >> 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@codeaurora.org> > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > >> Cc: Inki Dae <inki.dae@samsung.com> > >> Cc: Joonyoung Shim <jy0922.shim@samsung.com> > >> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > >> Cc: Kyungmin Park <kyungmin.park@samsung.com> > >> Cc: CK Hu <ck.hu@mediatek.com> > >> Cc: Philipp Zabel <p.zabel@pengutronix.de> > >> Cc: Ben Skeggs <bskeggs@redhat.com> > >> Cc: Mark Yao <mark.yao@rock-chips.com> > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > >> Cc: Vincent Abriou <vincent.abriou@st.com> > >> Cc: Shawn Guo <shawnguo@kernel.org> > >> Cc: Shashank Sharma <shashank.sharma@intel.com> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@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(-) [snip] > > > 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 [snip] > >> @@ -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. I think you have a layering violation if you assume that there's a connector after the HDMI encoder :-) What if the encoder's output is connected to, let's say, an HDMI to DSI bridge ? There's no connector in that case. > > > int err; > > > u32 s3d_flags; > > > u8 vic; [snip] -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D 2017-07-05 8:46 ` Laurent Pinchart @ 2017-07-05 11:48 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-07-05 11:48 UTC (permalink / raw) To: Laurent Pinchart Cc: Shawn Guo, intel-gfx, Seung-Woo Kim, dri-devel, Kyungmin Park, Ben Skeggs, Vincent Abriou On Wed, Jul 05, 2017 at 11:46:14AM +0300, Laurent Pinchart wrote: > Hi Ville, > > On Tuesday 04 Jul 2017 15:44:02 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@linux.intel.com wrote: > > >> From: Ville Syrjälä <ville.syrjala@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. > > > > >> 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@codeaurora.org> > > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > >> Cc: Inki Dae <inki.dae@samsung.com> > > >> Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > >> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > >> Cc: Kyungmin Park <kyungmin.park@samsung.com> > > >> Cc: CK Hu <ck.hu@mediatek.com> > > >> Cc: Philipp Zabel <p.zabel@pengutronix.de> > > >> Cc: Ben Skeggs <bskeggs@redhat.com> > > >> Cc: Mark Yao <mark.yao@rock-chips.com> > > >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > >> Cc: Vincent Abriou <vincent.abriou@st.com> > > >> Cc: Shawn Guo <shawnguo@kernel.org> > > >> Cc: Shashank Sharma <shashank.sharma@intel.com> > > >> Signed-off-by: Ville Syrjälä <ville.syrjala@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(-) > > [snip] > > > > > 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 > > [snip] > > > >> @@ -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. > > I think you have a layering violation if you assume that there's a connector > after the HDMI encoder :-) It doesn't have to be immediately after it. > What if the encoder's output is connected to, let's > say, an HDMI to DSI bridge ? There's no connector in that case. The connector must be somewhere. Otherwise what's the point. > > > > > int err; > > > > u32 s3d_flags; > > > > u8 vic; > > [snip] > > -- > Regards, > > Laurent Pinchart -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] video/hdmi: Allow "empty" HDMI infoframes 2017-07-03 19:19 ` [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes ville.syrjala 2017-07-03 19:19 ` [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D ville.syrjala @ 2017-07-03 19:39 ` Patchwork 2017-07-04 9:41 ` [PATCH 1/2] " Andrzej Hajda 2 siblings, 0 replies; 14+ messages in thread From: Patchwork @ 2017-07-03 19:39 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx == Series Details == Series: series starting with [1/2] video/hdmi: Allow "empty" HDMI infoframes URL : https://patchwork.freedesktop.org/series/26773/ State : success == Summary == Series 26773v1 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/26773/revisions/1/mbox/ Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-legacy: pass -> FAIL (fi-snb-2600) fdo#100215 Test kms_pipe_crc_basic: Subgroup hang-read-crc-pipe-b: pass -> DMESG-WARN (fi-pnv-d510) fdo#101597 fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215 fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597 fi-bdw-5557u total:279 pass:264 dwarn:0 dfail:0 fail:3 skip:11 time:439s fi-bdw-gvtdvm total:279 pass:257 dwarn:8 dfail:0 fail:0 skip:14 time:425s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:354s fi-bsw-n3050 total:279 pass:239 dwarn:0 dfail:0 fail:3 skip:36 time:509s fi-bxt-j4205 total:279 pass:256 dwarn:0 dfail:0 fail:3 skip:19 time:502s fi-byt-j1900 total:279 pass:250 dwarn:1 dfail:0 fail:3 skip:24 time:480s fi-byt-n2820 total:279 pass:246 dwarn:1 dfail:0 fail:3 skip:28 time:471s fi-glk-2a total:279 pass:256 dwarn:0 dfail:0 fail:3 skip:19 time:588s fi-hsw-4770 total:279 pass:259 dwarn:0 dfail:0 fail:3 skip:16 time:426s fi-hsw-4770r total:279 pass:259 dwarn:0 dfail:0 fail:3 skip:16 time:411s fi-ilk-650 total:279 pass:225 dwarn:0 dfail:0 fail:3 skip:50 time:422s fi-ivb-3520m total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:495s fi-ivb-3770 total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:472s fi-kbl-7500u total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:456s fi-kbl-7560u total:279 pass:264 dwarn:1 dfail:0 fail:3 skip:10 time:559s fi-kbl-r total:279 pass:256 dwarn:1 dfail:0 fail:3 skip:18 time:573s fi-pnv-d510 total:279 pass:222 dwarn:2 dfail:0 fail:0 skip:55 time:561s fi-skl-6260u total:279 pass:265 dwarn:0 dfail:0 fail:3 skip:10 time:451s fi-skl-6700hq total:279 pass:219 dwarn:1 dfail:0 fail:33 skip:24 time:303s fi-skl-6700k total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:461s fi-skl-6770hq total:279 pass:265 dwarn:0 dfail:0 fail:3 skip:10 time:470s fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:434s fi-snb-2520m total:279 pass:247 dwarn:0 dfail:0 fail:3 skip:28 time:534s fi-snb-2600 total:279 pass:245 dwarn:0 dfail:0 fail:4 skip:29 time:398s df0182c2c95385492772c6e4ace76b463298b8ca drm-tip: 2017y-07m-03d-13h-20m-24s UTC integration manifest b3f15c0 drm/hdmi: Allow HDMI infoframe without VIC or S3D 84a78cc video/hdmi: Allow "empty" HDMI infoframes == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5100/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes 2017-07-03 19:19 ` [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes ville.syrjala 2017-07-03 19:19 ` [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D ville.syrjala 2017-07-03 19:39 ` ✓ Fi.CI.BAT: success for series starting with [1/2] video/hdmi: Allow "empty" HDMI infoframes Patchwork @ 2017-07-04 9:41 ` Andrzej Hajda 2017-07-04 10:12 ` Ville Syrjälä 2 siblings, 1 reply; 14+ messages in thread From: Andrzej Hajda @ 2017-07-04 9:41 UTC (permalink / raw) To: ville.syrjala, dri-devel; +Cc: intel-gfx On 03.07.2017 21:19, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > HDMI 2.0 Appendix F suggest that we should keep sending the infoframe > when switching from 3D to 2D mode, even if the infoframe isn't strictly > necessary (ie. not needed to transmit the VIC or stereo information). > This is a workaround against some sinks that fail to realize that they > should switch from 3D to 2D mode when the source stop transmitting > the infoframe. > > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/video/hdmi.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 1cf907ecded4..af0c8dad29eb 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -341,10 +341,6 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > u8 *ptr = buffer; > size_t length; > > - /* empty info frame */ > - if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) > - return -EINVAL; > - > /* only one of those can be supplied */ > if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > return -EINVAL; > @@ -352,8 +348,10 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > /* for side by side (half) we also need to provide 3D_Ext_Data */ > if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) > frame->length = 6; > - else > + else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > frame->length = 5; > + else > + frame->length = 4; > > length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; > > @@ -372,14 +370,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > ptr[5] = 0x0c; > ptr[6] = 0x00; > > - if (frame->vic) { > - ptr[7] = 0x1 << 5; /* video format */ > - ptr[8] = frame->vic; > - } else { > + if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) { > ptr[7] = 0x2 << 5; /* video format */ > ptr[8] = (frame->s3d_struct & 0xf) << 4; > if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) > ptr[9] = (frame->s3d_ext_data & 0xf) << 4; > + } else if (frame->vic) { > + ptr[7] = 0x1 << 5; /* video format */ > + ptr[8] = frame->vic; > + } else { > + ptr[7] = 0x0 << 5; /* video format */ Changed order of conditionals (vic,s3d versus s3d,vic) could result in different output, but only in case of incorrect VSIFs, so probably harmless. Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > } > > hdmi_infoframe_set_checksum(buffer, length); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes 2017-07-04 9:41 ` [PATCH 1/2] " Andrzej Hajda @ 2017-07-04 10:12 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-07-04 10:12 UTC (permalink / raw) To: Andrzej Hajda; +Cc: intel-gfx, dri-devel On Tue, Jul 04, 2017 at 11:41:27AM +0200, Andrzej Hajda wrote: > On 03.07.2017 21:19, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > HDMI 2.0 Appendix F suggest that we should keep sending the infoframe > > when switching from 3D to 2D mode, even if the infoframe isn't strictly > > necessary (ie. not needed to transmit the VIC or stereo information). > > This is a workaround against some sinks that fail to realize that they > > should switch from 3D to 2D mode when the source stop transmitting > > the infoframe. > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/video/hdmi.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > > index 1cf907ecded4..af0c8dad29eb 100644 > > --- a/drivers/video/hdmi.c > > +++ b/drivers/video/hdmi.c > > @@ -341,10 +341,6 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > > u8 *ptr = buffer; > > size_t length; > > > > - /* empty info frame */ > > - if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID) > > - return -EINVAL; > > - > > /* only one of those can be supplied */ > > if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > > return -EINVAL; > > @@ -352,8 +348,10 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > > /* for side by side (half) we also need to provide 3D_Ext_Data */ > > if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) > > frame->length = 6; > > - else > > + else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) > > frame->length = 5; > > + else > > + frame->length = 4; > > > > length = HDMI_INFOFRAME_HEADER_SIZE + frame->length; > > > > @@ -372,14 +370,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame, > > ptr[5] = 0x0c; > > ptr[6] = 0x00; > > > > - if (frame->vic) { > > - ptr[7] = 0x1 << 5; /* video format */ > > - ptr[8] = frame->vic; > > - } else { > > + if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) { > > ptr[7] = 0x2 << 5; /* video format */ > > ptr[8] = (frame->s3d_struct & 0xf) << 4; > > if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) > > ptr[9] = (frame->s3d_ext_data & 0xf) << 4; > > + } else if (frame->vic) { > > + ptr[7] = 0x1 << 5; /* video format */ > > + ptr[8] = frame->vic; > > + } else { > > + ptr[7] = 0x0 << 5; /* video format */ > > Changed order of conditionals (vic,s3d versus s3d,vic) could result in > different output, but only in case of incorrect VSIFs, so probably harmless. Such an infoframe would have been rejected earlier, so can't happen. I did the reordering because I wanted the order to be the same here as it's where the length is determined. easier to cross check that way. > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Thanks. > > -- > Regards > Andrzej > > > } > > > > hdmi_infoframe_set_checksum(buffer, length); > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-07-05 11:48 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20170703192016epcas2p14dd4f6f04bff3325a4ba8bec715876e2@epcas2p1.samsung.com>
2017-07-03 19:19 ` [PATCH 1/2] video/hdmi: Allow "empty" HDMI infoframes ville.syrjala
2017-07-03 19:19 ` [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D ville.syrjala
2017-07-04 10:16 ` Ville Syrjälä
2017-07-04 11:56 ` Andrzej Hajda
2017-07-04 12:44 ` Ville Syrjälä
2017-07-04 13:58 ` Andrzej Hajda
2017-07-04 14:25 ` Ville Syrjälä
2017-07-04 15:09 ` Andrzej Hajda
2017-07-04 15:20 ` Ville Syrjälä
2017-07-05 8:46 ` Laurent Pinchart
2017-07-05 11:48 ` Ville Syrjälä
2017-07-03 19:39 ` ✓ Fi.CI.BAT: success for series starting with [1/2] video/hdmi: Allow "empty" HDMI infoframes Patchwork
2017-07-04 9:41 ` [PATCH 1/2] " Andrzej Hajda
2017-07-04 10:12 ` Ville Syrjälä
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.