[Intel-gfx] [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D

Wed Jul 5 11:48:53 UTC 2017

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 at linux.intel.com wrote:
> > >> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >> 
> > >> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
> > >> 3D to 2D mode in a timely fashion if the source simply stops sending the
> > >> HDMI infoframe. The suggested workaround is to keep sending the
> > >> infoframe even when strictly not necessary (ie. no VIC and no S3D).
> > >> HDMI 1.4 does allow for this behaviour, stating that sending the
> > >> infoframe is optional in this case.
> > > 
> > > My impression from the specs is that it should be done only after
> > > switching from 3d to 2d mode.
> > > In such case we just need to remember previous mode, if it was 3d, empty
> > > VSIF infoframe should be still generated for 2seconds.
> > > No need to do guesses from EDID.
> > > Am I right, or just missing something?
> > 
> > This code has no idea about any 3D->2D transitions, trying to make it
> > do that would just result in a lot of complexity. Much easier to just
> > always send the infoframe.
> > 
> > >> The infoframe was first specified in HDMI 1.4, so in theory sinks
> > >> predating that may not appreciate us sending an uknown infoframe
> > >> their way. To avoid regressions let's try to determine if the sink
> > >> supports the infoframe or not. Unfortunately there's no direct way
> > >> to do that, so instead we'll just check if we managed to parse any
> > >> HDMI 1.4 4k or stereo modes from the EDID, and if so we assume the
> > >> sink will accept the infoframe. Also if the EDID contains the HDMI
> > >> 2.0 HDMI Forum VSDB we can assume the sink is prepared to receive
> > >> the infoframe.
> > >> Cc: Archit Taneja <architt at codeaurora.org>
> > >> Cc: Andrzej Hajda <a.hajda at samsung.com>
> > >> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> > >> Cc: Inki Dae <inki.dae at samsung.com>
> > >> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
> > >> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
> > >> Cc: Kyungmin Park <kyungmin.park at samsung.com>
> > >> Cc: CK Hu <ck.hu at mediatek.com>
> > >> Cc: Philipp Zabel <p.zabel at pengutronix.de>
> > >> Cc: Ben Skeggs <bskeggs at redhat.com>
> > >> Cc: Mark Yao <mark.yao at rock-chips.com>
> > >> Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
> > >> Cc: Vincent Abriou <vincent.abriou at st.com>
> > >> Cc: Shawn Guo <shawnguo at kernel.org>
> > >> Cc: Shashank Sharma <shashank.sharma at intel.com>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >> ---
> > >> 
> > >>  drivers/gpu/drm/bridge/sil-sii8620.c      |  3 ++-
> > >>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  4 +++-
> > >>  drivers/gpu/drm/drm_edid.c                | 32 ++++++++++++++++++------
> > >>  drivers/gpu/drm/exynos/exynos_hdmi.c      |  2 +-
> > >>  drivers/gpu/drm/i915/intel_hdmi.c         | 14 ++++++++------
> > >>  drivers/gpu/drm/mediatek/mtk_hdmi.c       |  3 ++-
> > >>  drivers/gpu/drm/nouveau/nv50_display.c    |  3 ++-
> > >>  drivers/gpu/drm/rockchip/inno_hdmi.c      |  1 +
> > >>  drivers/gpu/drm/sti/sti_hdmi.c            |  4 +++-
> > >>  drivers/gpu/drm/zte/zx_hdmi.c             |  1 +
> > >>  include/drm/drm_connector.h               |  5 +++++
> > >>  include/drm/drm_edid.h                    |  1 +
> > >>  12 files changed, 55 insertions(+), 18 deletions(-)
> 
> [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