Nouveau - Jan 2017 - [PATCH 5/6] drm: Delete "mandatory" stereographic modes

On Tue, Jan 17, 2017 at 5:42 PM, Alastair Bridgewater
<alastair.bridgewater at gmail.com> wrote:
> HDMI specification 1.4a, table 8-15 is very explicitly a "must
> support at least one of" table, not a "must support all of"
table.
> It is not hard to find hardware that does not support some of the
> so-called "mandatory" modes.
>
> More seriously, this code generates invalid display modes for both
> of the 3D-capable panels that I have (a 42-inch LG TV and a Sony
> PlayStation 3D Display).
>
> If we want to be persnickety, one option would be to check the
> final list of modes against the table and give some message if
> none of them are valid, but it's a whole lot easier just to delete
> the code in question.
Damien added this in commit c858cfcae6d some 3 years ago.

Damien, do you remember why you added these "required" modes? Did you
have a monitor that only advertised 3D support without the actual
modes?
>
> Signed-off-by: Alastair Bridgewater <alastair.bridgewater at
gmail.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 66
----------------------------------------------
>  1 file changed, 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 336be31..723116a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2926,70 +2926,6 @@ do_cea_modes(struct drm_connector *connector, const
u8 *db, u8 len)
>         return modes;
>  }
>
> -struct stereo_mandatory_mode {
> -       int width, height, vrefresh;
> -       unsigned int flags;
> -};
> -
> -static const struct stereo_mandatory_mode stereo_mandatory_modes[] = {
> -       { 1920, 1080, 24, DRM_MODE_FLAG_3D_TOP_AND_BOTTOM },
> -       { 1920, 1080, 24, DRM_MODE_FLAG_3D_FRAME_PACKING },
> -       { 1920, 1080, 50,
> -         DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF },
> -       { 1920, 1080, 60,
> -         DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF },
> -       { 1280, 720,  50, DRM_MODE_FLAG_3D_TOP_AND_BOTTOM },
> -       { 1280, 720,  50, DRM_MODE_FLAG_3D_FRAME_PACKING },
> -       { 1280, 720,  60, DRM_MODE_FLAG_3D_TOP_AND_BOTTOM },
> -       { 1280, 720,  60, DRM_MODE_FLAG_3D_FRAME_PACKING }
> -};
> -
> -static bool
> -stereo_match_mandatory(const struct drm_display_mode *mode,
> -                      const struct stereo_mandatory_mode *stereo_mode)
> -{
> -       unsigned int interlaced = mode->flags &
DRM_MODE_FLAG_INTERLACE;
> -
> -       return mode->hdisplay == stereo_mode->width &&
> -              mode->vdisplay == stereo_mode->height &&
> -              interlaced == (stereo_mode->flags &
DRM_MODE_FLAG_INTERLACE) &&
> -              drm_mode_vrefresh(mode) == stereo_mode->vrefresh;
> -}
> -
> -static int add_hdmi_mandatory_stereo_modes(struct drm_connector
*connector)
> -{
> -       struct drm_device *dev = connector->dev;
> -       const struct drm_display_mode *mode;
> -       struct list_head stereo_modes;
> -       int modes = 0, i;
> -
> -       INIT_LIST_HEAD(&stereo_modes);
> -
> -       list_for_each_entry(mode, &connector->probed_modes, head) {
> -               for (i = 0; i < ARRAY_SIZE(stereo_mandatory_modes); i++)
{
> -                       const struct stereo_mandatory_mode *mandatory;
> -                       struct drm_display_mode *new_mode;
> -
> -                       if (!stereo_match_mandatory(mode,
> -                                                  
&stereo_mandatory_modes[i]))
> -                               continue;
> -
> -                       mandatory = &stereo_mandatory_modes[i];
> -                       new_mode = drm_mode_duplicate(dev, mode);
> -                       if (!new_mode)
> -                               continue;
> -
> -                       new_mode->flags |= mandatory->flags;
> -                       list_add_tail(&new_mode->head,
&stereo_modes);
> -                       modes++;
> -               }
> -       }
> -
> -       list_splice_tail(&stereo_modes,
&connector->probed_modes);
> -
> -       return modes;
> -}
> -
>  static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
>  {
>         struct drm_device *dev = connector->dev;
> @@ -3090,8 +3026,6 @@ do_hdmi_vsdb_modes(struct drm_connector *connector,
const u8 *db, u8 len,
>         /* 3D_Present */
>         offset++;
>         if (db[8 + offset] & (1 << 7)) {
> -               modes += add_hdmi_mandatory_stereo_modes(connector);
> -
>                 /* 3D_Multi_present */
>                 multi_present = (db[8 + offset] & 0x60) >> 5;
>         }
> --
> 2.10.2
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

On Wed, Jan 18, 2017 at 12:10:56AM -0500, Ilia Mirkin
wrote:
> On Tue, Jan 17, 2017 at 5:42 PM, Alastair Bridgewater
> <alastair.bridgewater at gmail.com> wrote:
> > HDMI specification 1.4a, table 8-15 is very explicitly a "must
> > support at least one of" table, not a "must support all
of" table.
> > It is not hard to find hardware that does not support some of the
> > so-called "mandatory" modes.
> >
> > More seriously, this code generates invalid display modes for both
> > of the 3D-capable panels that I have (a 42-inch LG TV and a Sony
> > PlayStation 3D Display).
> >
> > If we want to be persnickety, one option would be to check the
> > final list of modes against the table and give some message if
> > none of them are valid, but it's a whole lot easier just to delete
> > the code in question.
> 
> Damien added this in commit c858cfcae6d some 3 years ago.
> 
> Damien, do you remember why you added these "required" modes? Did
you
> have a monitor that only advertised 3D support without the actual
> modes?
Another quick glance at the specs leads me to believe c858cfcae6d is
correct.

8-15 does say that a sink supporting 3D must at lesst support one of the
modes listed in that table. But that's just the very start of the story
and we are really talking about the 3D_present bit in the HMDI VSDB and
the associated "mandatory" modes (the term comes from the spec).

HDMI 1.4a Page 155:

"3D_present [1bit] This bit indicates 3D support by the HDMI Sink,
including the mandatory formats. If set (=1), an HDMI Sink supports the
3D video formats that are mandatory formats," 

Continuing page 157:

"If 3D_present is set (=1), an HDMI Sink shall support 3D video formats
per the following requirements.
・An HDMI Sink which supports at least one 59.94 / 60Hz 2D video format
shall support *all* of <list of mandatory formats>"
・An HDMI Sink which supports at least one 50Hz 2D video format shall
support *all* of <list of mandatory formats>" 

The 3D pdf extraction from the spec is available would one want to
triple check the above:

   http://www.hdmi.org/manufacturer/specification.aspx

I have even dug up a direct link that someone made available:

  https://etmriwi.home.xs4all.nl/forum/hdmi_spec1.4a_3dextraction.pdf

Look for 3D_present, p. 15 and 17.

If the above is indeed correct, there may still be an issue in the way
we derive the mandatory modes - that part isn't really clear. Or, it
could be that people don't follow standards!

HTH,

-- 
Damien

On Wed, Jan 18, 2017 at 6:45 AM, Damien Lespiau
<damien.lespiau at intel.com> wrote:
> On Wed, Jan 18, 2017 at 12:10:56AM -0500, Ilia Mirkin wrote:
>> On Tue, Jan 17, 2017 at 5:42 PM, Alastair Bridgewater
>> <alastair.bridgewater at gmail.com> wrote:
>> > HDMI specification 1.4a, table 8-15 is very explicitly a
"must
>> > support at least one of" table, not a "must support all
of" table.
>> > It is not hard to find hardware that does not support some of the
>> > so-called "mandatory" modes.
>> >
>> > More seriously, this code generates invalid display modes for both
>> > of the 3D-capable panels that I have (a 42-inch LG TV and a Sony
>> > PlayStation 3D Display).
>> >
>> > If we want to be persnickety, one option would be to check the
>> > final list of modes against the table and give some message if
>> > none of them are valid, but it's a whole lot easier just to
delete
>> > the code in question.
>>
>> Damien added this in commit c858cfcae6d some 3 years ago.
>>
>> Damien, do you remember why you added these "required" modes?
Did you
>> have a monitor that only advertised 3D support without the actual
>> modes?
>
> Another quick glance at the specs leads me to believe c858cfcae6d is
> correct.
>
> 8-15 does say that a sink supporting 3D must at lesst support one of the
> modes listed in that table. But that's just the very start of the story
> and we are really talking about the 3D_present bit in the HMDI VSDB and
> the associated "mandatory" modes (the term comes from the spec).
>
> HDMI 1.4a Page 155:
>
> "3D_present [1bit] This bit indicates 3D support by the HDMI Sink,
> including the mandatory formats. If set (=1), an HDMI Sink supports the
> 3D video formats that are mandatory formats,"
>
> Continuing page 157:
>
> "If 3D_present is set (=1), an HDMI Sink shall support 3D video
formats
> per the following requirements.
> ・An HDMI Sink which supports at least one 59.94 / 60Hz 2D video format
> shall support *all* of <list of mandatory formats>"
> ・An HDMI Sink which supports at least one 50Hz 2D video format shall
> support *all* of <list of mandatory formats>"
>
> The 3D pdf extraction from the spec is available would one want to
> triple check the above:
>
>    http://www.hdmi.org/manufacturer/specification.aspx
>
> I have even dug up a direct link that someone made available:
>
>   https://etmriwi.home.xs4all.nl/forum/hdmi_spec1.4a_3dextraction.pdf
>
> Look for 3D_present, p. 15 and 17.
>
> If the above is indeed correct, there may still be an issue in the way
> we derive the mandatory modes - that part isn't really clear. Or, it
> could be that people don't follow standards!
OK, so the text definitely makes it seem like you're right.

Damien - did you ever test these mandatory modes on an actual
commercial 3D TV or similar device?

Alastair - I think this warrants some more attempts at getting those
"extra" modes to work. I'd be especially curious whether the blob
shows support for these modes, or what Windows shows with any
hardware.