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