Re: [PATCH 2/2] drm/hdmi: Allow HDMI infoframe without VIC or S3D

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ville,

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

[snip]

> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 2e55599816aa..c061dd5d25c0 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c

[snip]

> >> @@ -4452,6 +4458,7 @@ s3d_structure_from_display_mode(const struct
> >> drm_display_mode *mode)> > 
> >>   * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI
> >>   infoframe with
> >>   * data from a DRM display mode
> >>   * @frame: HDMI vendor infoframe
> >> + * @connector: the connector
> >>   * @mode: DRM display mode
> >>   *
> >>   * Note that there's is a need to send HDMI vendor infoframes only when
> >>   using a
> >> @@ -4462,8 +4469,15 @@ s3d_structure_from_display_mode(const struct
> >> drm_display_mode *mode)
> >>   */
> >>  
> >>  int
> >>  drm_hdmi_vendor_infoframe_from_display_mode(struct
> >>  hdmi_vendor_infoframe *frame,
> >> +					    struct drm_connector *connector,
> >>  					    const struct drm_display_mode
> >>  					    *mode)
> >>  {
> >> +	/*
> >> +	 * FIXME: sil-sii8620 doesn't have a connector around when
> >> +	 * we need one, so we have to be prepared for a NULL connector.
> >> +	 */
> >> +	bool has_hdmi_infoframe = connector ?
> >> +		&connector->display_info.has_hdmi_infoframe : NULL;
> > 
> > I wonder if requiring connector is a good idea, I can imagine that this
> > function can be necessary in pure drm_encoder or non-terminal drm_bridge.
> 
> No decent way around it, unless we want to risk sending the infoframe to
> pre HDMI 1.4 sinks. Sounds like you have some kind of layering problem
> if you can't get at the connector when you call this.

I think you have a layering violation if you assume that there's a connector 
after the HDMI encoder :-) What if the encoder's output is connected to, let's 
say, an HDMI to DSI bridge ? There's no connector in that case.

> > >  	int err;
> > >  	u32 s3d_flags;
> > >  	u8 vic;

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel