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

[Ville Syrjälä]

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.syrj...@linux.intel.com wrote:
> > >> From: Ville Syrjälä 
> > >> 
> > >> 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 
> > >> Cc: Andrzej Hajda 
> > >> Cc: Laurent Pinchart 
> > >> Cc: Inki Dae 
> > >> Cc: Joonyoung Shim 
> > >> Cc: Seung-Woo Kim 
> > >> Cc: Kyungmin Park 
> > >> Cc: CK Hu 
> > >> Cc: Philipp Zabel 
> > >> Cc: Ben Skeggs 
> > >> Cc: Mark Yao 
> > >> Cc: Benjamin Gaignard 
> > >> Cc: Vincent Abriou 
> > >> Cc: Shawn Guo 
> > >> Cc: Shashank Sharma 
> > >> Signed-off-by: Ville Syrjälä 
> > >> ---
> > >> 
> > >>  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 ?
> > >> +

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

[Laurent Pinchart]

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.syrj...@linux.intel.com wrote:
> >> From: Ville Syrjälä 
> >> 
> >> 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 
> >> Cc: Andrzej Hajda 
> >> Cc: Laurent Pinchart 
> >> Cc: Inki Dae 
> >> Cc: Joonyoung Shim 
> >> Cc: Seung-Woo Kim 
> >> Cc: Kyungmin Park 
> >> Cc: CK Hu 
> >> Cc: Philipp Zabel 
> >> Cc: Ben Skeggs 
> >> Cc: Mark Yao 
> >> Cc: Benjamin Gaignard 
> >> Cc: Vincent Abriou 
> >> Cc: Shawn Guo 
> >> Cc: Shashank Sharma 
> >> Signed-off-by: Ville Syrjälä 
> >> ---
> >> 
> >>  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 ?
> >> +  >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 

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

[Ville Syrjälä]

On Tue, Jul 04, 2017 at 05:09:36PM +0200, Andrzej Hajda wrote:
> On 04.07.2017 16:25, Ville Syrjälä wrote:
> > On Tue, Jul 04, 2017 at 03:58:02PM +0200, Andrzej Hajda wrote:
> >> On 04.07.2017 14:44, Ville Syrjälä wrote:
> >>> On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote:
>  On 03.07.2017 21:19, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
> > 3D to 2D mode in a timely fashion if the source simply stops sending the
> > HDMI infoframe. The suggested workaround is to keep sending the
> > infoframe even when strictly not necessary (ie. no VIC and no S3D).
> > HDMI 1.4 does allow for this behaviour, stating that sending the
> > infoframe is optional in this case.
>  My impression from the specs is that it should be done only after
>  switching from 3d to 2d mode.
>  In such case we just need to remember previous mode, if it was 3d, empty
>  VSIF infoframe should be still generated for 2seconds.
>  No need to do guesses from EDID.
>  Am I right, or just missing something?
> >>> This code has no idea about any 3D->2D transitions, trying to make it
> >>> do that would just result in a lot of complexity. Much easier to just
> >>> always send the infoframe.
> >> With such approach I see following 'issues':
> >> 0. It does not follow advices from specs.
> > Sure it does. The spec says you should keep sending it for at least two
> > seconds, but it doesn't say that you can't keep sending it for longer,
> > or even when there are no 3D->2D transitions.
> >
> >> 1. It changes behavior of old drivers, probably in harmless way but
> >> still there can be some sinks which will stop working.
> > I think this is a justified risk. If we start to worry too much about
> > every little change we stop making progress altogether. I did minimize
> > the danger by making sure we don't send it for pre-1.4 sinks, and
> > I was almost leaning towards not checking for that. But then I saw some
> > language in the spec which might be interpreted to mean that the source
> > isn't allowed to send unknown infoframe types to the sink, and I figured
> > that maybe it's better to play it safe.
> >
> >> 2. What if EDID does not advertises 3d/4k modes but the sink supports
> >> it, in such case userspace can set 3d mode, but after switch 3d->2d
> >> empty infoframe will not be sent.
> > IMO no point in worrying about broken EDIDs until one is proven to
> > exist.
> 
> I though the whole 'fake edid' interface, possibility to provide custom
> mode, and avoiding connector:mode_valid in such case is enough proof of
> existence of such EDIDs.

It's proof that there are buggy EDIDs in general. It isn't proof of
displays that can do 3D + don't advertize 3D/4k + require the 2D infoframe
to switch out of 3D mode.

> Internet is also full of advices how to force certain mode (also 3d
> mode), even if EDID does not advertise it.

If people insist on shooting themselves in the foot they can.

> 
> >
> >> 3. With this patch connector is required to generate infoframe, but
> >> there are pipelines where infoframe can be generated in non-connector
> >> driver, for example:
> >> crtc -> hdmi_encoder -> mhl_encoder -> connector
> >> In such case encoder has no access to the connector, of course it can
> >> violate abstraction layers and localize one, but it shows that something
> >> here is probably wrong.
> > Just pass the connector down if needed. We'll need the connector to
> > decide whether to send the new CEA-864-F VICs or not as well. And you
> > could need it (well really the connector state) to figure out the value
> > if some connector properties and whanot.
> >
> > Also I didn't actually run into any cases where the connector is
> > unavailable in the tree.
> 
> Not unavailable, just requires digging across layers.
> 
> >
> >> Maybe another helper drm_hdmi_vendor_infoframe_from_connector will be
> >> enough to solve it.
> >> 4. Video mode provided by user has nothing to do with EDID, why
> >> infoframe generated for that mode should depend on EDID.
> >>
> >> All above 'issues' are not serious ones but suggests that the solution
> >> is not the ideal one.
> >>
> >> On the other side I do not see much complication in 3D->2D transition,
> >> it requires just recording if 3d mode was set before and again
> >> drm_hdmi_vendor_infoframe_from_connector can perform this check.
> > Well, then we'd have to pass in the old and new crtc/connector states
> > etc. We don't want to start maintaining some custom state in the
> > infoframe helpers that bypasses the normal atomic state handling
> > altogether.
> 
> I understand, on the other side the issue the patch tries to solve has
> such nature.
> 
> >
> > And what causes us to stop sending it after the 2 seconds? Is two
> > second the correct amount of time or should we do it longer? etc.
> 
> I though here 

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

[Andrzej Hajda]

On 04.07.2017 16:25, Ville Syrjälä wrote:
> On Tue, Jul 04, 2017 at 03:58:02PM +0200, Andrzej Hajda wrote:
>> On 04.07.2017 14:44, Ville Syrjälä wrote:
>>> On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote:
 On 03.07.2017 21:19, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
>
> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
> 3D to 2D mode in a timely fashion if the source simply stops sending the
> HDMI infoframe. The suggested workaround is to keep sending the
> infoframe even when strictly not necessary (ie. no VIC and no S3D).
> HDMI 1.4 does allow for this behaviour, stating that sending the
> infoframe is optional in this case.
 My impression from the specs is that it should be done only after
 switching from 3d to 2d mode.
 In such case we just need to remember previous mode, if it was 3d, empty
 VSIF infoframe should be still generated for 2seconds.
 No need to do guesses from EDID.
 Am I right, or just missing something?
>>> This code has no idea about any 3D->2D transitions, trying to make it
>>> do that would just result in a lot of complexity. Much easier to just
>>> always send the infoframe.
>> With such approach I see following 'issues':
>> 0. It does not follow advices from specs.
> Sure it does. The spec says you should keep sending it for at least two
> seconds, but it doesn't say that you can't keep sending it for longer,
> or even when there are no 3D->2D transitions.
>
>> 1. It changes behavior of old drivers, probably in harmless way but
>> still there can be some sinks which will stop working.
> I think this is a justified risk. If we start to worry too much about
> every little change we stop making progress altogether. I did minimize
> the danger by making sure we don't send it for pre-1.4 sinks, and
> I was almost leaning towards not checking for that. But then I saw some
> language in the spec which might be interpreted to mean that the source
> isn't allowed to send unknown infoframe types to the sink, and I figured
> that maybe it's better to play it safe.
>
>> 2. What if EDID does not advertises 3d/4k modes but the sink supports
>> it, in such case userspace can set 3d mode, but after switch 3d->2d
>> empty infoframe will not be sent.
> IMO no point in worrying about broken EDIDs until one is proven to
> exist.

I though the whole 'fake edid' interface, possibility to provide custom
mode, and avoiding connector:mode_valid in such case is enough proof of
existence of such EDIDs.
Internet is also full of advices how to force certain mode (also 3d
mode), even if EDID does not advertise it.

>
>> 3. With this patch connector is required to generate infoframe, but
>> there are pipelines where infoframe can be generated in non-connector
>> driver, for example:
>> crtc -> hdmi_encoder -> mhl_encoder -> connector
>> In such case encoder has no access to the connector, of course it can
>> violate abstraction layers and localize one, but it shows that something
>> here is probably wrong.
> Just pass the connector down if needed. We'll need the connector to
> decide whether to send the new CEA-864-F VICs or not as well. And you
> could need it (well really the connector state) to figure out the value
> if some connector properties and whanot.
>
> Also I didn't actually run into any cases where the connector is
> unavailable in the tree.

Not unavailable, just requires digging across layers.

>
>> Maybe another helper drm_hdmi_vendor_infoframe_from_connector will be
>> enough to solve it.
>> 4. Video mode provided by user has nothing to do with EDID, why
>> infoframe generated for that mode should depend on EDID.
>>
>> All above 'issues' are not serious ones but suggests that the solution
>> is not the ideal one.
>>
>> On the other side I do not see much complication in 3D->2D transition,
>> it requires just recording if 3d mode was set before and again
>> drm_hdmi_vendor_infoframe_from_connector can perform this check.
> Well, then we'd have to pass in the old and new crtc/connector states
> etc. We don't want to start maintaining some custom state in the
> infoframe helpers that bypasses the normal atomic state handling
> altogether.

I understand, on the other side the issue the patch tries to solve has
such nature.

>
> And what causes us to stop sending it after the 2 seconds? Is two
> second the correct amount of time or should we do it longer? etc.

I though here about simpler approach: to send frame always after 3d mode
was ever used.
Approach with 2s timeout would require more work on drivers side.

>
> So this line of thinking leads to a lot of new problems we can avoid
> by keeping it simple.
>
I have pointed out some weakness IMO, but as I said earlier they are
non-blockers, so if you still think this approach is better it is OK.


Regards

Andrzej


___
dri-devel mailing list

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

[Ville Syrjälä]

On Tue, Jul 04, 2017 at 03:58:02PM +0200, Andrzej Hajda wrote:
> On 04.07.2017 14:44, Ville Syrjälä wrote:
> > On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote:
> >> On 03.07.2017 21:19, ville.syrj...@linux.intel.com wrote:
> >>> From: Ville Syrjälä 
> >>>
> >>> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
> >>> 3D to 2D mode in a timely fashion if the source simply stops sending the
> >>> HDMI infoframe. The suggested workaround is to keep sending the
> >>> infoframe even when strictly not necessary (ie. no VIC and no S3D).
> >>> HDMI 1.4 does allow for this behaviour, stating that sending the
> >>> infoframe is optional in this case.
> >> My impression from the specs is that it should be done only after
> >> switching from 3d to 2d mode.
> >> In such case we just need to remember previous mode, if it was 3d, empty
> >> VSIF infoframe should be still generated for 2seconds.
> >> No need to do guesses from EDID.
> >> Am I right, or just missing something?
> > This code has no idea about any 3D->2D transitions, trying to make it
> > do that would just result in a lot of complexity. Much easier to just
> > always send the infoframe.
> 
> With such approach I see following 'issues':
> 0. It does not follow advices from specs.

Sure it does. The spec says you should keep sending it for at least two
seconds, but it doesn't say that you can't keep sending it for longer,
or even when there are no 3D->2D transitions.

> 1. It changes behavior of old drivers, probably in harmless way but
> still there can be some sinks which will stop working.

I think this is a justified risk. If we start to worry too much about
every little change we stop making progress altogether. I did minimize
the danger by making sure we don't send it for pre-1.4 sinks, and
I was almost leaning towards not checking for that. But then I saw some
language in the spec which might be interpreted to mean that the source
isn't allowed to send unknown infoframe types to the sink, and I figured
that maybe it's better to play it safe.

> 2. What if EDID does not advertises 3d/4k modes but the sink supports
> it, in such case userspace can set 3d mode, but after switch 3d->2d
> empty infoframe will not be sent.

IMO no point in worrying about broken EDIDs until one is proven to
exist.

> 3. With this patch connector is required to generate infoframe, but
> there are pipelines where infoframe can be generated in non-connector
> driver, for example:
> crtc -> hdmi_encoder -> mhl_encoder -> connector
> In such case encoder has no access to the connector, of course it can
> violate abstraction layers and localize one, but it shows that something
> here is probably wrong.

Just pass the connector down if needed. We'll need the connector to
decide whether to send the new CEA-864-F VICs or not as well. And you
could need it (well really the connector state) to figure out the value
if some connector properties and whanot.

Also I didn't actually run into any cases where the connector is
unavailable in the tree.

> Maybe another helper drm_hdmi_vendor_infoframe_from_connector will be
> enough to solve it.
> 4. Video mode provided by user has nothing to do with EDID, why
> infoframe generated for that mode should depend on EDID.
> 
> All above 'issues' are not serious ones but suggests that the solution
> is not the ideal one.
> 
> On the other side I do not see much complication in 3D->2D transition,
> it requires just recording if 3d mode was set before and again
> drm_hdmi_vendor_infoframe_from_connector can perform this check.

Well, then we'd have to pass in the old and new crtc/connector states
etc. We don't want to start maintaining some custom state in the
infoframe helpers that bypasses the normal atomic state handling
altogether.

And what causes us to stop sending it after the 2 seconds? Is two
second the correct amount of time or should we do it longer? etc.

So this line of thinking leads to a lot of new problems we can avoid
by keeping it simple.

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

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

[Andrzej Hajda]

On 04.07.2017 14:44, Ville Syrjälä wrote:
> On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote:
>> On 03.07.2017 21:19, ville.syrj...@linux.intel.com wrote:
>>> From: Ville Syrjälä 
>>>
>>> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
>>> 3D to 2D mode in a timely fashion if the source simply stops sending the
>>> HDMI infoframe. The suggested workaround is to keep sending the
>>> infoframe even when strictly not necessary (ie. no VIC and no S3D).
>>> HDMI 1.4 does allow for this behaviour, stating that sending the
>>> infoframe is optional in this case.
>> My impression from the specs is that it should be done only after
>> switching from 3d to 2d mode.
>> In such case we just need to remember previous mode, if it was 3d, empty
>> VSIF infoframe should be still generated for 2seconds.
>> No need to do guesses from EDID.
>> Am I right, or just missing something?
> This code has no idea about any 3D->2D transitions, trying to make it
> do that would just result in a lot of complexity. Much easier to just
> always send the infoframe.

With such approach I see following 'issues':
0. It does not follow advices from specs.
1. It changes behavior of old drivers, probably in harmless way but
still there can be some sinks which will stop working.
2. What if EDID does not advertises 3d/4k modes but the sink supports
it, in such case userspace can set 3d mode, but after switch 3d->2d
empty infoframe will not be sent.
3. With this patch connector is required to generate infoframe, but
there are pipelines where infoframe can be generated in non-connector
driver, for example:
crtc -> hdmi_encoder -> mhl_encoder -> connector
In such case encoder has no access to the connector, of course it can
violate abstraction layers and localize one, but it shows that something
here is probably wrong.
Maybe another helper drm_hdmi_vendor_infoframe_from_connector will be
enough to solve it.
4. Video mode provided by user has nothing to do with EDID, why
infoframe generated for that mode should depend on EDID.

All above 'issues' are not serious ones but suggests that the solution
is not the ideal one.

On the other side I do not see much complication in 3D->2D transition,
it requires just recording if 3d mode was set before and again
drm_hdmi_vendor_infoframe_from_connector can perform this check.

Regards
Andrzej

>
>>> The infoframe was first specified in HDMI 1.4, so in theory sinks
>>> predating that may not appreciate us sending an uknown infoframe
>>> their way. To avoid regressions let's try to determine if the sink
>>> supports the infoframe or not. Unfortunately there's no direct way
>>> to do that, so instead we'll just check if we managed to parse any
>>> HDMI 1.4 4k or stereo modes from the EDID, and if so we assume the
>>> sink will accept the infoframe. Also if the EDID contains the HDMI
>>> 2.0 HDMI Forum VSDB we can assume the sink is prepared to receive
>>> the infoframe.
>>> Cc: Archit Taneja 
>>> Cc: Andrzej Hajda 
>>> Cc: Laurent Pinchart 
>>> Cc: Inki Dae 
>>> Cc: Joonyoung Shim 
>>> Cc: Seung-Woo Kim 
>>> Cc: Kyungmin Park 
>>> Cc: CK Hu 
>>> Cc: Philipp Zabel 
>>> Cc: Ben Skeggs 
>>> Cc: Mark Yao 
>>> Cc: Benjamin Gaignard 
>>> Cc: Vincent Abriou 
>>> Cc: Shawn Guo 
>>> Cc: Shashank Sharma 
>>> Signed-off-by: Ville Syrjälä 
>>> ---
>>>  drivers/gpu/drm/bridge/sil-sii8620.c  |  3 ++-
>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  4 +++-
>>>  drivers/gpu/drm/drm_edid.c| 32 
>>> +--
>>>  drivers/gpu/drm/exynos/exynos_hdmi.c  |  2 +-
>>>  drivers/gpu/drm/i915/intel_hdmi.c | 14 --
>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c   |  3 ++-
>>>  drivers/gpu/drm/nouveau/nv50_display.c|  3 ++-
>>>  drivers/gpu/drm/rockchip/inno_hdmi.c  |  1 +
>>>  drivers/gpu/drm/sti/sti_hdmi.c|  4 +++-
>>>  drivers/gpu/drm/zte/zx_hdmi.c |  1 +
>>>  include/drm/drm_connector.h   |  5 +
>>>  include/drm/drm_edid.h|  1 +
>>>  12 files changed, 55 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c 
>>> b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> index 2d51a2269fc6..758b5a4546f1 100644
>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> @@ -2136,8 +2136,9 @@ static bool sii8620_mode_fixup(struct drm_bridge 
>>> *bridge,
>>> union hdmi_infoframe frm;
>>> u8 mhl_vic[] = { 0, 95, 94, 93, 98 };
>>>  
>>> +   

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

[Ville Syrjälä]

On Tue, Jul 04, 2017 at 01:56:07PM +0200, Andrzej Hajda wrote:
> On 03.07.2017 21:19, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > 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 
> > Cc: Andrzej Hajda 
> > Cc: Laurent Pinchart 
> > Cc: Inki Dae 
> > Cc: Joonyoung Shim 
> > Cc: Seung-Woo Kim 
> > Cc: Kyungmin Park 
> > Cc: CK Hu 
> > Cc: Philipp Zabel 
> > Cc: Ben Skeggs 
> > Cc: Mark Yao 
> > Cc: Benjamin Gaignard 
> > Cc: Vincent Abriou 
> > Cc: Shawn Guo 
> > Cc: Shashank Sharma 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/bridge/sil-sii8620.c  |  3 ++-
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  4 +++-
> >  drivers/gpu/drm/drm_edid.c| 32 
> > +--
> >  drivers/gpu/drm/exynos/exynos_hdmi.c  |  2 +-
> >  drivers/gpu/drm/i915/intel_hdmi.c | 14 --
> >  drivers/gpu/drm/mediatek/mtk_hdmi.c   |  3 ++-
> >  drivers/gpu/drm/nouveau/nv50_display.c|  3 ++-
> >  drivers/gpu/drm/rockchip/inno_hdmi.c  |  1 +
> >  drivers/gpu/drm/sti/sti_hdmi.c|  4 +++-
> >  drivers/gpu/drm/zte/zx_hdmi.c |  1 +
> >  include/drm/drm_connector.h   |  5 +
> >  include/drm/drm_edid.h|  1 +
> >  12 files changed, 55 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c 
> > b/drivers/gpu/drm/bridge/sil-sii8620.c
> > index 2d51a2269fc6..758b5a4546f1 100644
> > --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> > @@ -2136,8 +2136,9 @@ static bool sii8620_mode_fixup(struct drm_bridge 
> > *bridge,
> > union hdmi_infoframe frm;
> > u8 mhl_vic[] = { 0, 95, 94, 93, 98 };
> >  
> > +   /* FIXME: We need the connector here */
> > drm_hdmi_vendor_infoframe_from_display_mode(
> > -   , adjusted_mode);
> > +   , NULL, adjusted_mode);
> > vic = frm.vendor.hdmi.vic;
> 
> Usage of drm_hdmi_vendor_infoframe_from_display_mode is just dirty
> workaround to get hdmi vic from mode (drm_match_hdmi_mode is static,
> opposite to drm_match_cea_mode).

Ah. I guess I should have read more than the single line of code :)

> I guess better solution is to export drm_match_hdmi_mode, or wait for
> extending cea table with HDMI2.0 modes, I plan to fix it after merging
> HDMI2.0 patches.

OK. Cool. That means we can nuke the NULL connector handling from
drm_hdmi_vendor_infoframe_from_display_mode() at that point.

> So beside the comment, the change looks OK.
> 
> > if (vic >= ARRAY_SIZE(mhl_vic))
> > vic = 0;
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index ead11242c4b9..c43389774691 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -1426,7 +1426,9 @@ static void 
> > hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> > u8 

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

[Andrzej Hajda]

On 03.07.2017 21:19, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
>
> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
> 3D to 2D mode in a timely fashion if the source simply stops sending the
> HDMI infoframe. The suggested workaround is to keep sending the
> infoframe even when strictly not necessary (ie. no VIC and no S3D).
> HDMI 1.4 does allow for this behaviour, stating that sending the
> infoframe is optional in this case.

My impression from the specs is that it should be done only after
switching from 3d to 2d mode.
In such case we just need to remember previous mode, if it was 3d, empty
VSIF infoframe should be still generated for 2seconds.
No need to do guesses from EDID.
Am I right, or just missing something?

>
> The infoframe was first specified in HDMI 1.4, so in theory sinks
> predating that may not appreciate us sending an uknown infoframe
> their way. To avoid regressions let's try to determine if the sink
> supports the infoframe or not. Unfortunately there's no direct way
> to do that, so instead we'll just check if we managed to parse any
> HDMI 1.4 4k or stereo modes from the EDID, and if so we assume the
> sink will accept the infoframe. Also if the EDID contains the HDMI
> 2.0 HDMI Forum VSDB we can assume the sink is prepared to receive
> the infoframe.
> Cc: Archit Taneja 
> Cc: Andrzej Hajda 
> Cc: Laurent Pinchart 
> Cc: Inki Dae 
> Cc: Joonyoung Shim 
> Cc: Seung-Woo Kim 
> Cc: Kyungmin Park 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> Cc: Ben Skeggs 
> Cc: Mark Yao 
> Cc: Benjamin Gaignard 
> Cc: Vincent Abriou 
> Cc: Shawn Guo 
> Cc: Shashank Sharma 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/bridge/sil-sii8620.c  |  3 ++-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  4 +++-
>  drivers/gpu/drm/drm_edid.c| 32 
> +--
>  drivers/gpu/drm/exynos/exynos_hdmi.c  |  2 +-
>  drivers/gpu/drm/i915/intel_hdmi.c | 14 --
>  drivers/gpu/drm/mediatek/mtk_hdmi.c   |  3 ++-
>  drivers/gpu/drm/nouveau/nv50_display.c|  3 ++-
>  drivers/gpu/drm/rockchip/inno_hdmi.c  |  1 +
>  drivers/gpu/drm/sti/sti_hdmi.c|  4 +++-
>  drivers/gpu/drm/zte/zx_hdmi.c |  1 +
>  include/drm/drm_connector.h   |  5 +
>  include/drm/drm_edid.h|  1 +
>  12 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c 
> b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 2d51a2269fc6..758b5a4546f1 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -2136,8 +2136,9 @@ static bool sii8620_mode_fixup(struct drm_bridge 
> *bridge,
>   union hdmi_infoframe frm;
>   u8 mhl_vic[] = { 0, 95, 94, 93, 98 };
>  
> + /* FIXME: We need the connector here */
>   drm_hdmi_vendor_infoframe_from_display_mode(
> - , adjusted_mode);
> + , NULL, adjusted_mode);
>   vic = frm.vendor.hdmi.vic;

Usage of drm_hdmi_vendor_infoframe_from_display_mode is just dirty
workaround to get hdmi vic from mode (drm_match_hdmi_mode is static,
opposite to drm_match_cea_mode).
I guess better solution is to export drm_match_hdmi_mode, or wait for
extending cea table with HDMI2.0 modes, I plan to fix it after merging
HDMI2.0 patches.
So beside the comment, the change looks OK.

>   if (vic >= ARRAY_SIZE(mhl_vic))
>   vic = 0;
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index ead11242c4b9..c43389774691 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1426,7 +1426,9 @@ static void 
> hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>   u8 buffer[10];
>   ssize_t err;
>  
> - err = drm_hdmi_vendor_infoframe_from_display_mode(, mode);
> + err = drm_hdmi_vendor_infoframe_from_display_mode(,
> +   >connector,
> +   mode);
>   if (err < 0)
>   /*
>* Going into that statement does not means vendor infoframe
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2e55599816aa..c061dd5d25c0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ 

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

[Ville Syrjälä]

On Mon, Jul 03, 2017 at 10:19:38PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Appedix F of HDMI 2.0 says that some HDMI sink may fail to switch from
> 3D to 2D mode in a timely fashion if the source simply stops sending the
> HDMI infoframe. The suggested workaround is to keep sending the
> infoframe even when strictly not necessary (ie. no VIC and no S3D).
> HDMI 1.4 does allow for this behaviour, stating that sending the
> infoframe is optional in this case.
> 
> The infoframe was first specified in HDMI 1.4, so in theory sinks
> predating that may not appreciate us sending an uknown infoframe
> their way. To avoid regressions let's try to determine if the sink
> supports the infoframe or not. Unfortunately there's no direct way
> to do that, so instead we'll just check if we managed to parse any
> HDMI 1.4 4k or stereo modes from the EDID, and if so we assume the
> sink will accept the infoframe. Also if the EDID contains the HDMI
> 2.0 HDMI Forum VSDB we can assume the sink is prepared to receive
> the infoframe.
> 
> Cc: Archit Taneja 
> Cc: Andrzej Hajda 
> Cc: Laurent Pinchart 
> Cc: Inki Dae 
> Cc: Joonyoung Shim 
> Cc: Seung-Woo Kim 
> Cc: Kyungmin Park 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> Cc: Ben Skeggs 
> Cc: Mark Yao 
> Cc: Benjamin Gaignard 
> Cc: Vincent Abriou 
> Cc: Shawn Guo 
> Cc: Shashank Sharma 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/bridge/sil-sii8620.c  |  3 ++-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  4 +++-
>  drivers/gpu/drm/drm_edid.c| 32 
> +--
>  drivers/gpu/drm/exynos/exynos_hdmi.c  |  2 +-
>  drivers/gpu/drm/i915/intel_hdmi.c | 14 --
>  drivers/gpu/drm/mediatek/mtk_hdmi.c   |  3 ++-
>  drivers/gpu/drm/nouveau/nv50_display.c|  3 ++-
>  drivers/gpu/drm/rockchip/inno_hdmi.c  |  1 +
>  drivers/gpu/drm/sti/sti_hdmi.c|  4 +++-
>  drivers/gpu/drm/zte/zx_hdmi.c |  1 +
>  include/drm/drm_connector.h   |  5 +
>  include/drm/drm_edid.h|  1 +
>  12 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c 
> b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 2d51a2269fc6..758b5a4546f1 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -2136,8 +2136,9 @@ static bool sii8620_mode_fixup(struct drm_bridge 
> *bridge,
>   union hdmi_infoframe frm;
>   u8 mhl_vic[] = { 0, 95, 94, 93, 98 };
>  
> + /* FIXME: We need the connector here */
>   drm_hdmi_vendor_infoframe_from_display_mode(
> - , adjusted_mode);
> + , NULL, adjusted_mode);
>   vic = frm.vendor.hdmi.vic;
>   if (vic >= ARRAY_SIZE(mhl_vic))
>   vic = 0;
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index ead11242c4b9..c43389774691 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1426,7 +1426,9 @@ static void 
> hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>   u8 buffer[10];
>   ssize_t err;
>  
> - err = drm_hdmi_vendor_infoframe_from_display_mode(, mode);
> + err = drm_hdmi_vendor_infoframe_from_display_mode(,
> +   >connector,
> +   mode);
>   if (err < 0)
>   /*
>* Going into that statement does not means vendor infoframe
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2e55599816aa..c061dd5d25c0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3081,6 +3081,7 @@ static int
>  do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  const u8 *video_db, u8 video_len)
>  {
> + struct drm_display_info *info = >display_info;
>   int modes = 0, offset = 0, i, multi_present = 0, multi_len;
>   u8 vic_len, hdmi_3d_len = 0;
>   u16 mask;
> @@ -3208,6 +3209,8 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, 
> const u8 *db, u8 len,
>   }
>  
>  out:
> + if (modes > 0)
> + info->has_hdmi_infoframe = true;

Oh, and I forgot to mention that this depends on Shashank's patch to
reorder things such that