From mboxrd@z Thu Jan 1 00:00:00 1970 From: Clint Taylor Subject: Re: [PATCH] drm/edid: Reduce horizontal timings for pixel replicated modes Date: Mon, 18 Aug 2014 10:29:15 -0700 Message-ID: <53F237EB.1080700@intel.com> References: <1408039765-30255-1-git-send-email-clinton.a.taylor@intel.com> <20140814184811.GK4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 502D9892C6 for ; Mon, 18 Aug 2014 10:30:40 -0700 (PDT) In-Reply-To: <20140814184811.GK4193@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: Intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 08/14/2014 11:48 AM, Ville Syrj=E4l=E4 wrote: > On Thu, Aug 14, 2014 at 11:09:25AM -0700, clinton.a.taylor@intel.com wrot= e: >> From: Clint Taylor >> >> Pixel replicated modes should be 720 horizontal pixel and pixel >> replicated by the HW across the HDMI cable at 2X pixel clock. Current >> horizontal resolution of 1440 does not allow pixel duplication to >> occur and scaling artifacts occur on the TV. HDMI certification >> 7-26 currently fails for all pixel replicated modes. This change fizes >> the HDMI certification issues with 480i/576i. >> >> Signed-off-by: Clint Taylor >> --- >> drivers/gpu/drm/drm_edid.c | 100 ++++++++++++++++++-----------= -------- >> drivers/gpu/drm/i915/intel_hdmi.c | 14 +++++- >> 2 files changed, 63 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index f905c63..c7a26a7 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -632,27 +632,27 @@ static const struct drm_display_mode edid_cea_mode= s[] =3D { >> DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC | >> DRM_MODE_FLAG_INTERLACE), >> .vrefresh =3D 60, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_= 9, }, >> - /* 6 - 1440x480i@60Hz */ >> - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, >> - 1602, 1716, 0, 480, 488, 494, 525, 0, >> + /* 6 - 720(1440)x480i@60Hz */ >> + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, >> + 801, 858, 0, 480, 488, 494, 525, 0, > > As stated before I think I would have preferred explicit /2 here, but > if you prefer to have it this way I can live with it. Sorry, I missed the other patches comments. The CEA-861-E specification = actually has the pixel doubled values in (Table 3) the detailed sync = information data, so it does make sense to have the doubled values in = the table with /2. I prefer the actual values myself, but I am willing = to use either method. > >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 60, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_3= , }, >> - /* 7 - 1440x480i@60Hz */ >> - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, >> - 1602, 1716, 0, 480, 488, 494, 525, 0, >> + /* 7 - 720(1440)x480i@60Hz */ >> + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, >> + 801, 858, 0, 480, 488, 494, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 60, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_= 9, }, >> - /* 8 - 1440x240@60Hz */ >> - { DRM_MODE("1440x240", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, >> - 1602, 1716, 0, 240, 244, 247, 262, 0, >> + /* 8 - 720(1440)x240@60Hz */ >> + { DRM_MODE("720x240", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, >> + 801, 858, 0, 240, 244, 247, 262, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 60, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_3= , }, >> - /* 9 - 1440x240@60Hz */ >> - { DRM_MODE("1440x240", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478, >> - 1602, 1716, 0, 240, 244, 247, 262, 0, >> + /* 9 - 720(1440)x240@60Hz */ >> + { DRM_MODE("720x240", DRM_MODE_TYPE_DRIVER, 13500, 720, 739, >> + 801, 858, 0, 240, 244, 247, 262, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 60, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_= 9, }, >> @@ -714,27 +714,27 @@ static const struct drm_display_mode edid_cea_mode= s[] =3D { >> DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC | >> DRM_MODE_FLAG_INTERLACE), >> .vrefresh =3D 50, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_= 9, }, >> - /* 21 - 1440x576i@50Hz */ >> - { DRM_MODE("1440x576i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1464, >> - 1590, 1728, 0, 576, 580, 586, 625, 0, >> + /* 21 - 720(1440)x576i@50Hz */ >> + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 13500, 720, 732, >> + 795, 864, 0, 576, 580, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 50, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_3= , }, >> - /* 22 - 1440x576i@50Hz */ >> - { DRM_MODE("1440x576i", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1464, >> - 1590, 1728, 0, 576, 580, 586, 625, 0, >> + /* 22 - 720(1440)x576i@50Hz */ >> + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 13500, 720, 732, >> + 795, 864, 0, 576, 580, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 50, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_= 9, }, >> - /* 23 - 1440x288@50Hz */ >> - { DRM_MODE("1440x288", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1464, >> - 1590, 1728, 0, 288, 290, 293, 312, 0, >> + /* 23 - 720(1440)x288@50Hz */ >> + { DRM_MODE("720x288", DRM_MODE_TYPE_DRIVER, 13500, 720, 732, >> + 795, 864, 0, 288, 290, 293, 312, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 50, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_3= , }, >> - /* 24 - 1440x288@50Hz */ >> - { DRM_MODE("1440x288", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1464, >> - 1590, 1728, 0, 288, 290, 293, 312, 0, >> + /* 24 - 720(1440)x288@50Hz */ >> + { DRM_MODE("720x288", DRM_MODE_TYPE_DRIVER, 13500, 720, 732, >> + 795, 864, 0, 288, 290, 293, 312, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 50, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_= 9, }, >> @@ -837,17 +837,17 @@ static const struct drm_display_mode edid_cea_mode= s[] =3D { >> 796, 864, 0, 576, 581, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), >> .vrefresh =3D 100, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> - /* 44 - 1440x576i@100Hz */ >> - { DRM_MODE("1440x576", DRM_MODE_TYPE_DRIVER, 54000, 1440, 1464, >> - 1590, 1728, 0, 576, 580, 586, 625, 0, >> + /* 44 - 720(1440)x576i@100Hz */ >> + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 27000, 720, 732, >> + 795, 864, 0, 576, 580, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> - DRM_MODE_FLAG_DBLCLK), >> + DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), > > /me actually reads through the patch this time... > > So seems we were missing the interlace flag sometimes. Fixing these > should really be a separate patch. Agreed > >> .vrefresh =3D 100, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_= 3, }, >> - /* 45 - 1440x576i@100Hz */ >> - { DRM_MODE("1440x576", DRM_MODE_TYPE_DRIVER, 54000, 1440, 1464, >> - 1590, 1728, 0, 576, 580, 586, 625, 0, >> + /* 45 - 720(1440)x576i@100Hz */ >> + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 27000, 720, 732, >> + 795, 864, 0, 576, 580, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> - DRM_MODE_FLAG_DBLCLK), >> + DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 100, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> /* 46 - 1920x1080i@120Hz */ >> { DRM_MODE("1920x1080i", DRM_MODE_TYPE_DRIVER, 148500, 1920, 2008, >> @@ -870,15 +870,15 @@ static const struct drm_display_mode edid_cea_mode= s[] =3D { >> 798, 858, 0, 480, 489, 495, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), >> .vrefresh =3D 120, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> - /* 50 - 1440x480i@120Hz */ >> - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 54000, 1440, 1478, >> - 1602, 1716, 0, 480, 488, 494, 525, 0, >> + /* 50 - 720(1440)x480i@120Hz */ >> + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 27000, 720, 739, >> + 801, 858, 0, 480, 488, 494, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 120, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_= 3, }, >> - /* 51 - 1440x480i@120Hz */ >> - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 54000, 1440, 1478, >> - 1602, 1716, 0, 480, 488, 494, 525, 0, >> + /* 51 - 720(1440)x480i@120Hz */ >> + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 27000, 720, 739, >> + 801, 858, 0, 480, 488, 494, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 120, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> @@ -892,15 +892,15 @@ static const struct drm_display_mode edid_cea_mode= s[] =3D { >> 796, 864, 0, 576, 581, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), >> .vrefresh =3D 200, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> - /* 54 - 1440x576i@200Hz */ >> - { DRM_MODE("1440x576i", DRM_MODE_TYPE_DRIVER, 108000, 1440, 1464, >> - 1590, 1728, 0, 576, 580, 586, 625, 0, >> + /* 54 - 720(1440)x576i@200Hz */ >> + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 54000, 720, 732, >> + 795, 864, 0, 576, 580, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 200, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_= 3, }, >> - /* 55 - 1440x576i@200Hz */ >> - { DRM_MODE("1440x576i", DRM_MODE_TYPE_DRIVER, 108000, 1440, 1464, >> - 1590, 1728, 0, 576, 580, 586, 625, 0, >> + /* 55 - 720(1440)x576i@200Hz */ >> + { DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 54000, 720, 732, >> + 795, 865, 0, 576, 580, 586, 625, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 200, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> @@ -914,15 +914,15 @@ static const struct drm_display_mode edid_cea_mode= s[] =3D { >> 798, 858, 0, 480, 489, 495, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), >> .vrefresh =3D 240, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> - /* 58 - 1440x480i@240 */ >> - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 108000, 1440, 1478, >> - 1602, 1716, 0, 480, 488, 494, 525, 0, >> + /* 58 - 720(1440)x480i@240 */ >> + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739, >> + 801, 858, 0, 480, 488, 494, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 240, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_4_= 3, }, >> - /* 59 - 1440x480i@240 */ >> - { DRM_MODE("1440x480i", DRM_MODE_TYPE_DRIVER, 108000, 1440, 1478, >> - 1602, 1716, 0, 480, 488, 494, 525, 0, >> + /* 59 - 720(1440)x480i@240 */ >> + { DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739, >> + 801, 858, 0, 480, 488, 494, 525, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC | >> DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK), >> .vrefresh =3D 240, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16= _9, }, >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/in= tel_hdmi.c >> index 9169786..c6307cb 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -867,7 +867,11 @@ intel_hdmi_mode_valid(struct drm_connector *connect= or, >> if (mode->clock > hdmi_portclock_limit(intel_attached_hdmi(connector), >> true)) >> return MODE_CLOCK_HIGH; >> - if (mode->clock < 20000) >> + if (mode->flags & DRM_MODE_FLAG_DBLCLK) { >> + if ((mode->clock * 2) < 20000) > ^ ^ > > Pointless parens. Extra parens from prototype code in multiple spots will be removed. > >> + return MODE_CLOCK_LOW; > > Indentation is messed up here. > >> + } >> + else if (mode->clock < 20000) >> return MODE_CLOCK_LOW; >> >> if (mode->flags & DRM_MODE_FLAG_DBLSCAN) >> @@ -921,6 +925,14 @@ bool intel_hdmi_compute_config(struct intel_encoder= *encoder, >> intel_hdmi->color_range =3D 0; >> } >> >> + /* Adjust pipe timings and pixel multiplier for pixel doubled modes */ > > I don't think this comment is really useful. Just drop it IMO. > >> + if ((adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)) { > ^ ^ > > Pointless parens. > >> + drm_mode_set_crtcinfo(adjusted_mode, 0); > > This shouldn't be needed now that the original timings are halved. agreed > >> + >> + /* Set 2x pixel double on pipe */ > > Another comment that doesn't really do anything. Can be dropped too. > >> + pipe_config->pixel_multiplier =3D 2; >> + } >> + >> if (intel_hdmi->color_range) >> pipe_config->limited_color_range =3D true; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >