Re: [PATCH 02/10] drm/edid: Allow HDMI infoframe without VIC or S3D

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>

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@xxxxxxxxx>
> >>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> >>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>> ---
> >>>    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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux