Re: [PATCH] drm/drm_edid: correct VIC and HDMI_VIC under HDMI 2.0

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

 



On Thu, Oct 03, 2019 at 06:49:05AM +0000, Lin, Wayne wrote:
> 
> 
> ________________________________
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: Wednesday, October 2, 2019 19:58
> To: Lin, Wayne <Wayne.Lin@xxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx <dri-devel@xxxxxxxxxxxxxxxxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>
> Subject: Re: [PATCH] drm/drm_edid: correct VIC and HDMI_VIC under HDMI 2.0
> 
> On Tue, Sep 24, 2019 at 01:26:21PM +0800, Wayne Lin wrote:
> > In HDMI 1.4 defines 4k modes without specific aspect ratio.
> > However, in HDMI 2.0, adds aspect ratio attribute to distinguish different
> > 4k modes.
> >
> > According to Appendix E of HDMI 2.0 spec, source should use VSIF to
> > indicate VIC mode only when the mode is one defined in HDMI 1.4b 4K modes.
> > Otherwise, use AVI infoframes to convey VIC.
> >
> > eg: VIC_103 should use AVI infoframes and VIC_93 use VSIF
> >
> > When the sink is HDMI 2.0, current code in
> > drm_hdmi_avi_infoframe_from_display_mode will also force mode VIC_103 to
> > have VIC value 0. This violates the spec and needs to be corrected.
> 
> > Where is that being done? We only set the AVI VIC to zero if we're going
> > to use the HDMI VIC instead.
> 
> Appreciate for your time and apologize for not explaining it clearly.
> Current code in drm_hdmi_avi_infoframe_from_display_mode() will call
> drm_match_hdmi_mode() to set up vendor_if_vic. By checking
> drm_valid_hdmi_vic(vendor_if_vic) to see if the vic info should be conveyed by avi
> or not.
> 
> But in drm_match_hdmi_mode(), code doesn't enable match_flags with
> DRM_MODE_MATCH_ASPECT_RATIO. I think it's due to HDMI1.4b doesn't specify
> 4K mode conveyed by HDMI VIC with particular aspect ratio. But in Appendix E of
> HDMI 2.0 spec, it specify only 4k modes with particular aspect ratio should use VSIF to convey.
> Hence, when the sink support HDMI 2.0 and set the mode to be VIC_103, calling
> drm_match_hdmi_mode(mode) will return vendor_if_vic = 3 (VIC_93 and VIC_103 are having
> the same timing but different aspect ratio). Thereafter will set the  frame->video_code to 0.
> However, VIC_103 should use AVI VIC according to HDMI 2.0 spec (only VIC: 93, 94, 95 &
> 98 should use VSIF).
> 
> This patch try to revise that, when the sink support HDMI 2.0, drm_match_hdmi_mode()
> should also take aspect ratio into consideration.
> But for easy reading, I add another function "drm_match_hdmi_1_4_mode" to do so.

Seems rather convoluted. I think we should just add the aspect ratios
to edid_4k_modes[]. Or is there some problem with that approach?

> 
> > The same situation occurs in drm_hdmi_vendor_infoframe_from_display_mode
> > and should set HDMI_VIC when the mode is one defined in HDMI 1.4b 4K
> > modes.
> 
> > Yes, and we do that. "vic = drm_match_hdmi_mode(mode);"
> 
> > Apart from adding the aspect ratios I don't really understand what
> > you're trying to achieve here.
> 
> For HDMI 2.0 sink, drm_match_hdmi_mode() should also take aspect ratio into consideration.
> Once again, very appreciate for your time.
> 
> > ---
> >  drivers/gpu/drm/drm_edid.c | 95 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 92 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 649cfd8b4200..0fea9bf4ec67 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1306,6 +1306,37 @@ static const struct drm_display_mode edid_4k_modes[] = {
> >          .vrefresh = 24, },
> >  };
> >
> > +/*
> > + * 4k modes of HDMI 1.4 defined in HDMI 2.0. Index using the VIC.
> > + */
> > +static const struct drm_display_mode hdmi_1_4_edid_4k_modes[] = {
> > +     /* 0 - dummy, VICs start at 1 */
> > +     { },
> > +     /* 1 - 3840x2160@30Hz */
> > +     { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> > +                3840, 4016, 4104, 4400, 0,
> > +                2160, 2168, 2178, 2250, 0,
> > +                DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> > +       .vrefresh = 30, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
> > +     /* 2 - 3840x2160@25Hz */
> > +     { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> > +                3840, 4896, 4984, 5280, 0,
> > +                2160, 2168, 2178, 2250, 0,
> > +                DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> > +       .vrefresh = 25, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
> > +     /* 3 - 3840x2160@24Hz */
> > +     { DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> > +                3840, 5116, 5204, 5500, 0,
> > +                2160, 2168, 2178, 2250, 0,
> > +                DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> > +       .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
> > +     /* 4 - 4096x2160@24Hz (SMPTE) */
> > +     { DRM_MODE("4096x2160", DRM_MODE_TYPE_DRIVER, 297000,
> > +                4096, 5116, 5204, 5500, 0,
> > +                2160, 2168, 2178, 2250, 0,
> > +                DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> > +       .vrefresh = 24, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135, },
> > +};
> >  /*** DDC fetch and block validation ***/
> >
> >  static const u8 edid_header[] = {
> > @@ -3061,6 +3092,19 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode)
> >        return cea_mode_alternate_clock(hdmi_mode);
> >  }
> >
> > +/*
> > + * Calculate the alternate clock for HDMI modes (those from the HDMI vendor
> > + * specific block).
> > + *
> > + * It's almost like hdmi_mode_alternate_clock(), but no exception for VIC 4 mode.
> > + * There is an alternate clock (23.98Hz) of VIC 4 mode (4096x2160@24Hz) in HDMI 2.0
> > + */
> > +static unsigned int
> > +hdmi_1_4_mode_alternate_clock(const struct drm_display_mode *hdmi_mode)
> > +{
> > +     return cea_mode_alternate_clock(hdmi_mode);
> > +}
> > +
> >  static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_match,
> >                                              unsigned int clock_tolerance)
> >  {
> > @@ -3121,11 +3165,53 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
> >        return 0;
> >  }
> >
> > +/*
> > + * drm_match_hdmi_1_4_mode - look for a HDMI 1.4 mode matching given mode
> > + * @to_match: display mode
> > + *
> > + * An HDMI mode is one defined in the HDMI vendor specific block.
> > + * In HDMI 2.0, only few 4k resolutions with specific aspect ratio should
> > + * utilize H14b VSIF.
> > + *
> > + * Returns the HDMI Video ID (VIC) of the mode or 0 if it isn't one.
> > + */
> > +static u8 drm_match_hdmi_1_4_mode(const struct drm_display_mode *to_match)
> > +{
> > +     unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
> > +     u8 vic;
> > +
> > +     if (!to_match->clock)
> > +             return 0;
> > +
> > +     if (to_match->picture_aspect_ratio)
> > +             match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > +
> > +     for (vic = 1; vic < ARRAY_SIZE(hdmi_1_4_edid_4k_modes); vic++) {
> > +             const struct drm_display_mode *hdmi_1_4_mode = &hdmi_1_4_edid_4k_modes[vic];
> > +             unsigned int clock1, clock2;
> > +
> > +             /* Make sure to also match alternate clocks */
> > +             clock1 = hdmi_1_4_mode->clock;
> > +             clock2 = hdmi_1_4_mode_alternate_clock(hdmi_1_4_mode);
> > +
> > +             if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
> > +                  KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
> > +                 drm_mode_match(to_match, hdmi_1_4_mode, match_flags))
> > +                     return vic;
> > +     }
> > +     return 0;
> > +}
> > +
> >  static bool drm_valid_hdmi_vic(u8 vic)
> >  {
> >        return vic > 0 && vic < ARRAY_SIZE(edid_4k_modes);
> >  }
> >
> > +static bool drm_valid_hdmi_1_4_vic(u8 vic)
> > +{
> > +     return vic > 0 && vic < ARRAY_SIZE(hdmi_1_4_edid_4k_modes);
> > +}
> > +
> >  static int
> >  add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
> >  {
> > @@ -4894,10 +4980,12 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> >         * HDMI 1.4b 4K modes
> >         */
> >        if (frame->video_code) {
> > -             u8 vendor_if_vic = drm_match_hdmi_mode(mode);
> > +             u8 vendor_if_vic = is_hdmi2_sink(connector) ?
> > +                     drm_match_hdmi_1_4_mode(mode) : drm_match_hdmi_mode(mode);
> >                bool is_s3d = mode->flags & DRM_MODE_FLAG_3D_MASK;
> >
> > -             if (drm_valid_hdmi_vic(vendor_if_vic) && !is_s3d)
> > +             if (!is_s3d && is_hdmi2_sink(connector) ?
> > +                     drm_valid_hdmi_1_4_vic(vendor_if_vic) : drm_valid_hdmi_vic(vendor_if_vic))
> >                        frame->video_code = 0;
> >        }
> >
> > @@ -5125,7 +5213,8 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
> >        if (!has_hdmi_infoframe)
> >                return -EINVAL;
> >
> > -     vic = drm_match_hdmi_mode(mode);
> > +     vic = is_hdmi2_sink(connector) ?
> > +                     drm_match_hdmi_1_4_mode(mode) : drm_match_hdmi_mode(mode);
> >        s3d_flags = mode->flags & DRM_MODE_FLAG_3D_MASK;
> >
> >        /*
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
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