On Thu, Oct 03, 2019 at 06:49:05AM +0000, Lin, Wayne wrote:
>
>
> ________________________________
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, October 2, 2019 19:58
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
> 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?
Thanks for your time.
>
> > 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@lists.freedesktop.org
> >
https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
> --
> Ville Syrjälä
> Intel