* [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment @ 2016-11-03 12:53 ` ville.syrjala 2016-11-03 12:53 ` [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC ville.syrjala 2016-11-04 7:29 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment Andrzej Hajda 0 siblings, 2 replies; 7+ messages in thread From: ville.syrjala @ 2016-11-03 12:53 UTC (permalink / raw) To: dri-devel From: Ville Syrjälä <ville.syrjala@linux.intel.com> All the VICs apart from 58 and 59 have the word "Hz" included in the comment. Include it for 59 and 59 as well. Cc: Shashank Sharma <shashank.sharma@intel.com> Cc: Andrzej Hajda <a.hajda@samsung.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_edid.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 9506933b41cd..7eec18925b70 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -957,13 +957,13 @@ static const struct drm_display_mode edid_cea_modes[] = { 798, 858, 0, 480, 489, 495, 525, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, - /* 58 - 720(1440)x480i@240 */ + /* 58 - 720(1440)x480i@240Hz */ { 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 = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, }, - /* 59 - 720(1440)x480i@240 */ + /* 59 - 720(1440)x480i@240Hz */ { 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 | -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC 2016-11-03 12:53 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment ville.syrjala @ 2016-11-03 12:53 ` ville.syrjala 2016-11-08 11:43 ` Andrzej Hajda 2016-11-04 7:29 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment Andrzej Hajda 1 sibling, 1 reply; 7+ messages in thread From: ville.syrjala @ 2016-11-03 12:53 UTC (permalink / raw) To: dri-devel From: Ville Syrjälä <ville.syrjala@linux.intel.com> CEA-861 specifies that the vertical front porch may vary by one or two lines for specific VICs. Up to now we've only considered a mode to match the VIC if it matched the shortest possible vertical front porch length (as that is the variant we store in cea_modes[]). Let's allow our VIC matching to work with the other timings variants as well so that that we'll send out the correct VIC if the variant actually used isn't the one with the shortest vertical front porch. Cc: Shashank Sharma <shashank.sharma@intel.com> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Adam Jackson <ajax@redhat.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_edid.c | 66 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7eec18925b70..728990fee4ef 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2613,6 +2613,41 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode) return clock; } +static bool +cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode) +{ + /* + * For certain VICs the spec allows the vertical + * front porch to vary by one or two lines. + * + * cea_modes[] stores the variant with the shortest + * vertical front porch. We can adjust the mode to + * get the other variants by simply increasing the + * vertical front porch length. + */ + BUILD_BUG_ON(edid_cea_modes[8].vtotal != 262 || + edid_cea_modes[9].vtotal != 262 || + edid_cea_modes[12].vtotal != 262 || + edid_cea_modes[13].vtotal != 262 || + edid_cea_modes[23].vtotal != 312 || + edid_cea_modes[24].vtotal != 312 || + edid_cea_modes[27].vtotal != 312 || + edid_cea_modes[28].vtotal != 312); + + if (((vic == 8 || vic == 9 || + vic == 12 || vic == 13) && mode->vtotal < 263) || + ((vic == 23 || vic == 24 || + vic == 27 || vic == 28) && mode->vtotal < 314)) { + mode->vsync_start++; + mode->vsync_end++; + mode->vtotal++; + + return true; + } + + return false; +} + static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match, unsigned int clock_tolerance) { @@ -2622,19 +2657,21 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m return 0; for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; + struct drm_display_mode cea_mode = edid_cea_modes[vic]; unsigned int clock1, clock2; /* Check both 60Hz and 59.94Hz */ - clock1 = cea_mode->clock; - clock2 = cea_mode_alternate_clock(cea_mode); + clock1 = cea_mode.clock; + clock2 = cea_mode_alternate_clock(&cea_mode); if (abs(to_match->clock - clock1) > clock_tolerance && abs(to_match->clock - clock2) > clock_tolerance) continue; - if (drm_mode_equal_no_clocks(to_match, cea_mode)) - return vic; + do { + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) + return vic; + } while (cea_mode_alternate_timings(vic, &cea_mode)); } return 0; @@ -2655,18 +2692,23 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) return 0; for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; + struct drm_display_mode cea_mode = edid_cea_modes[vic]; unsigned int clock1, clock2; /* Check both 60Hz and 59.94Hz */ - clock1 = cea_mode->clock; - clock2 = cea_mode_alternate_clock(cea_mode); + clock1 = cea_mode.clock; + clock2 = cea_mode_alternate_clock(&cea_mode); - if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || - KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && - drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode)) - return vic; + if (KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock1) && + KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock2)) + continue; + + do { + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) + return vic; + } while (cea_mode_alternate_timings(vic, &cea_mode)); } + return 0; } EXPORT_SYMBOL(drm_match_cea_mode); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC 2016-11-03 12:53 ` [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC ville.syrjala @ 2016-11-08 11:43 ` Andrzej Hajda 2016-11-08 13:09 ` Ville Syrjälä 2016-11-28 16:33 ` Ville Syrjälä 0 siblings, 2 replies; 7+ messages in thread From: Andrzej Hajda @ 2016-11-08 11:43 UTC (permalink / raw) To: ville.syrjala, dri-devel On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > CEA-861 specifies that the vertical front porch may vary by one or two > lines for specific VICs. Up to now we've only considered a mode to match > the VIC if it matched the shortest possible vertical front porch length > (as that is the variant we store in cea_modes[]). Let's allow our VIC > matching to work with the other timings variants as well so that that > we'll send out the correct VIC if the variant actually used isn't the > one with the shortest vertical front porch. > > Cc: Shashank Sharma <shashank.sharma@intel.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Adam Jackson <ajax@redhat.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I have checked against specification and it looks OK. I have few nitpicks below regarding implementation, but this could be matter of taste, so: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/gpu/drm/drm_edid.c | 66 +++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 54 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 7eec18925b70..728990fee4ef 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2613,6 +2613,41 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode) > return clock; > } > > +static bool > +cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode) > +{ > + /* > + * For certain VICs the spec allows the vertical > + * front porch to vary by one or two lines. > + * > + * cea_modes[] stores the variant with the shortest > + * vertical front porch. We can adjust the mode to > + * get the other variants by simply increasing the > + * vertical front porch length. > + */ > + BUILD_BUG_ON(edid_cea_modes[8].vtotal != 262 || > + edid_cea_modes[9].vtotal != 262 || > + edid_cea_modes[12].vtotal != 262 || > + edid_cea_modes[13].vtotal != 262 || > + edid_cea_modes[23].vtotal != 312 || > + edid_cea_modes[24].vtotal != 312 || > + edid_cea_modes[27].vtotal != 312 || > + edid_cea_modes[28].vtotal != 312); I am not sure if this compile check is really necessary, I would rather put comment before edid_cea_modes array which mode should be put into array in multi-mode case. > + > + if (((vic == 8 || vic == 9 || > + vic == 12 || vic == 13) && mode->vtotal < 263) || > + ((vic == 23 || vic == 24 || > + vic == 27 || vic == 28) && mode->vtotal < 314)) { I wonder if it wouldn't be better to mark somehow these modes in the array as alternating ones. This way all info about cea modes will be in one place. For example by (ab)using private_flags: /* 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 = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, .private_flags = CEA_MODE_FLAG_VFP2}, This is of course just an idea, I am not sure if not overkill :) > + mode->vsync_start++; > + mode->vsync_end++; > + mode->vtotal++; > + > + return true; > + } > + > + return false; > +} > + > static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match, > unsigned int clock_tolerance) > { > @@ -2622,19 +2657,21 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > return 0; > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; > + struct drm_display_mode cea_mode = edid_cea_modes[vic]; > unsigned int clock1, clock2; > > /* Check both 60Hz and 59.94Hz */ > - clock1 = cea_mode->clock; > - clock2 = cea_mode_alternate_clock(cea_mode); > + clock1 = cea_mode.clock; > + clock2 = cea_mode_alternate_clock(&cea_mode); > > if (abs(to_match->clock - clock1) > clock_tolerance && > abs(to_match->clock - clock2) > clock_tolerance) > continue; > > - if (drm_mode_equal_no_clocks(to_match, cea_mode)) > - return vic; > + do { > + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > + return vic; > + } while (cea_mode_alternate_timings(vic, &cea_mode)); > } > > return 0; > @@ -2655,18 +2692,23 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > return 0; > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; > + struct drm_display_mode cea_mode = edid_cea_modes[vic]; > unsigned int clock1, clock2; > > /* Check both 60Hz and 59.94Hz */ > - clock1 = cea_mode->clock; > - clock2 = cea_mode_alternate_clock(cea_mode); > + clock1 = cea_mode.clock; > + clock2 = cea_mode_alternate_clock(&cea_mode); > > - if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > - KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > - drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode)) > - return vic; > + if (KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock1) && > + KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock2)) > + continue; > + > + do { > + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > + return vic; > + } while (cea_mode_alternate_timings(vic, &cea_mode)); > } > + > return 0; > } > EXPORT_SYMBOL(drm_match_cea_mode); And finally there is no modification of add_alternate_cea_modes, but I guess it can be added later. Regards Andrzej _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC 2016-11-08 11:43 ` Andrzej Hajda @ 2016-11-08 13:09 ` Ville Syrjälä 2016-11-28 16:33 ` Ville Syrjälä 1 sibling, 0 replies; 7+ messages in thread From: Ville Syrjälä @ 2016-11-08 13:09 UTC (permalink / raw) To: Andrzej Hajda; +Cc: dri-devel On Tue, Nov 08, 2016 at 12:43:55PM +0100, Andrzej Hajda wrote: > On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > CEA-861 specifies that the vertical front porch may vary by one or two > > lines for specific VICs. Up to now we've only considered a mode to match > > the VIC if it matched the shortest possible vertical front porch length > > (as that is the variant we store in cea_modes[]). Let's allow our VIC > > matching to work with the other timings variants as well so that that > > we'll send out the correct VIC if the variant actually used isn't the > > one with the shortest vertical front porch. > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Adam Jackson <ajax@redhat.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > I have checked against specification and it looks OK. > I have few nitpicks below regarding implementation, but this could be > matter of taste, so: > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > > > --- > > drivers/gpu/drm/drm_edid.c | 66 +++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 54 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 7eec18925b70..728990fee4ef 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -2613,6 +2613,41 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode) > > return clock; > > } > > > > +static bool > > +cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode) > > +{ > > + /* > > + * For certain VICs the spec allows the vertical > > + * front porch to vary by one or two lines. > > + * > > + * cea_modes[] stores the variant with the shortest > > + * vertical front porch. We can adjust the mode to > > + * get the other variants by simply increasing the > > + * vertical front porch length. > > + */ > > + BUILD_BUG_ON(edid_cea_modes[8].vtotal != 262 || > > + edid_cea_modes[9].vtotal != 262 || > > + edid_cea_modes[12].vtotal != 262 || > > + edid_cea_modes[13].vtotal != 262 || > > + edid_cea_modes[23].vtotal != 312 || > > + edid_cea_modes[24].vtotal != 312 || > > + edid_cea_modes[27].vtotal != 312 || > > + edid_cea_modes[28].vtotal != 312); > > I am not sure if this compile check is really necessary, > I would rather put comment before edid_cea_modes array > which mode should be put into array in multi-mode case. Comments can't guarantee this wouldn't break. Doesn't mean we can't have a comment in addition though. > > > + > > + if (((vic == 8 || vic == 9 || > > + vic == 12 || vic == 13) && mode->vtotal < 263) || > > + ((vic == 23 || vic == 24 || > > + vic == 27 || vic == 28) && mode->vtotal < 314)) { > > I wonder if it wouldn't be better to mark somehow these modes > in the array as alternating ones. This way all info about cea modes > will be in one place. For example by (ab)using private_flags: > /* 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 = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, > .private_flags = CEA_MODE_FLAG_VFP2}, > > This is of course just an idea, I am not sure if not overkill :) Sounds potentially sensible. Should do the same for the alternate clock thing as well then I suppose. I was pondering whether I should just convert the cea mode array into some kind of other data structure that could store multiple variants for each mode. But then I figured it's maybe a bit too much work for just these few excepttions. Another thought that occurred to me recently is that the cea mode list is getting rather long, so I was wondering if could speed up the lookups somehow. Currently we just do a linear search through the whole thing. A simple bsearch() might not work out since the modes aren't necessarily sorted in any useful order, so we might need some kind of a hash or something, But then I figured that we shouldn't do these lookups too often so it might not be worth the effort to optimize it. > > > > + mode->vsync_start++; > > + mode->vsync_end++; > > + mode->vtotal++; > > + > > + return true; > > + } > > + > > + return false; > > +} > > + > > static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match, > > unsigned int clock_tolerance) > > { > > @@ -2622,19 +2657,21 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > > return 0; > > > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > > - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; > > + struct drm_display_mode cea_mode = edid_cea_modes[vic]; > > unsigned int clock1, clock2; > > > > /* Check both 60Hz and 59.94Hz */ > > - clock1 = cea_mode->clock; > > - clock2 = cea_mode_alternate_clock(cea_mode); > > + clock1 = cea_mode.clock; > > + clock2 = cea_mode_alternate_clock(&cea_mode); > > > > if (abs(to_match->clock - clock1) > clock_tolerance && > > abs(to_match->clock - clock2) > clock_tolerance) > > continue; > > > > - if (drm_mode_equal_no_clocks(to_match, cea_mode)) > > - return vic; > > + do { > > + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > > + return vic; > > + } while (cea_mode_alternate_timings(vic, &cea_mode)); > > } > > > > return 0; > > @@ -2655,18 +2692,23 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > > return 0; > > > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > > - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; > > + struct drm_display_mode cea_mode = edid_cea_modes[vic]; > > unsigned int clock1, clock2; > > > > /* Check both 60Hz and 59.94Hz */ > > - clock1 = cea_mode->clock; > > - clock2 = cea_mode_alternate_clock(cea_mode); > > + clock1 = cea_mode.clock; > > + clock2 = cea_mode_alternate_clock(&cea_mode); > > > > - if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > > - KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > > - drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode)) > > - return vic; > > + if (KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock1) && > > + KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock2)) > > + continue; > > + > > + do { > > + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > > + return vic; > > + } while (cea_mode_alternate_timings(vic, &cea_mode)); > > } > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_match_cea_mode); > > And finally there is no modification of add_alternate_cea_modes, but I > guess it can be added later. I doubt we want to add all the variants of these modes to the list. Having just one should be enough I think. -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC 2016-11-08 11:43 ` Andrzej Hajda 2016-11-08 13:09 ` Ville Syrjälä @ 2016-11-28 16:33 ` Ville Syrjälä 1 sibling, 0 replies; 7+ messages in thread From: Ville Syrjälä @ 2016-11-28 16:33 UTC (permalink / raw) To: Andrzej Hajda; +Cc: dri-devel On Tue, Nov 08, 2016 at 12:43:55PM +0100, Andrzej Hajda wrote: > On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > CEA-861 specifies that the vertical front porch may vary by one or two > > lines for specific VICs. Up to now we've only considered a mode to match > > the VIC if it matched the shortest possible vertical front porch length > > (as that is the variant we store in cea_modes[]). Let's allow our VIC > > matching to work with the other timings variants as well so that that > > we'll send out the correct VIC if the variant actually used isn't the > > one with the shortest vertical front porch. > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Adam Jackson <ajax@redhat.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > I have checked against specification and it looks OK. > I have few nitpicks below regarding implementation, but this could be > matter of taste, so: > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> I decieded to leave the ideas for further improvements to cook longer. Patch pushed to drm-misc-next. Thanks for the review. > > > > --- > > drivers/gpu/drm/drm_edid.c | 66 +++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 54 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 7eec18925b70..728990fee4ef 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -2613,6 +2613,41 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode) > > return clock; > > } > > > > +static bool > > +cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode) > > +{ > > + /* > > + * For certain VICs the spec allows the vertical > > + * front porch to vary by one or two lines. > > + * > > + * cea_modes[] stores the variant with the shortest > > + * vertical front porch. We can adjust the mode to > > + * get the other variants by simply increasing the > > + * vertical front porch length. > > + */ > > + BUILD_BUG_ON(edid_cea_modes[8].vtotal != 262 || > > + edid_cea_modes[9].vtotal != 262 || > > + edid_cea_modes[12].vtotal != 262 || > > + edid_cea_modes[13].vtotal != 262 || > > + edid_cea_modes[23].vtotal != 312 || > > + edid_cea_modes[24].vtotal != 312 || > > + edid_cea_modes[27].vtotal != 312 || > > + edid_cea_modes[28].vtotal != 312); > > I am not sure if this compile check is really necessary, > I would rather put comment before edid_cea_modes array > which mode should be put into array in multi-mode case. > > > + > > + if (((vic == 8 || vic == 9 || > > + vic == 12 || vic == 13) && mode->vtotal < 263) || > > + ((vic == 23 || vic == 24 || > > + vic == 27 || vic == 28) && mode->vtotal < 314)) { > > I wonder if it wouldn't be better to mark somehow these modes > in the array as alternating ones. This way all info about cea modes > will be in one place. For example by (ab)using private_flags: > /* 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 = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, > .private_flags = CEA_MODE_FLAG_VFP2}, > > This is of course just an idea, I am not sure if not overkill :) > > > > + mode->vsync_start++; > > + mode->vsync_end++; > > + mode->vtotal++; > > + > > + return true; > > + } > > + > > + return false; > > +} > > + > > static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match, > > unsigned int clock_tolerance) > > { > > @@ -2622,19 +2657,21 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > > return 0; > > > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > > - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; > > + struct drm_display_mode cea_mode = edid_cea_modes[vic]; > > unsigned int clock1, clock2; > > > > /* Check both 60Hz and 59.94Hz */ > > - clock1 = cea_mode->clock; > > - clock2 = cea_mode_alternate_clock(cea_mode); > > + clock1 = cea_mode.clock; > > + clock2 = cea_mode_alternate_clock(&cea_mode); > > > > if (abs(to_match->clock - clock1) > clock_tolerance && > > abs(to_match->clock - clock2) > clock_tolerance) > > continue; > > > > - if (drm_mode_equal_no_clocks(to_match, cea_mode)) > > - return vic; > > + do { > > + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > > + return vic; > > + } while (cea_mode_alternate_timings(vic, &cea_mode)); > > } > > > > return 0; > > @@ -2655,18 +2692,23 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > > return 0; > > > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > > - const struct drm_display_mode *cea_mode = &edid_cea_modes[vic]; > > + struct drm_display_mode cea_mode = edid_cea_modes[vic]; > > unsigned int clock1, clock2; > > > > /* Check both 60Hz and 59.94Hz */ > > - clock1 = cea_mode->clock; > > - clock2 = cea_mode_alternate_clock(cea_mode); > > + clock1 = cea_mode.clock; > > + clock2 = cea_mode_alternate_clock(&cea_mode); > > > > - if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > > - KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > > - drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode)) > > - return vic; > > + if (KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock1) && > > + KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock2)) > > + continue; > > + > > + do { > > + if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > > + return vic; > > + } while (cea_mode_alternate_timings(vic, &cea_mode)); > > } > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_match_cea_mode); > > And finally there is no modification of add_alternate_cea_modes, but I > guess it can be added later. > > > Regards > > Andrzej > > > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment 2016-11-03 12:53 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment ville.syrjala 2016-11-03 12:53 ` [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC ville.syrjala @ 2016-11-04 7:29 ` Andrzej Hajda 2016-11-08 10:10 ` Daniel Vetter 1 sibling, 1 reply; 7+ messages in thread From: Andrzej Hajda @ 2016-11-04 7:29 UTC (permalink / raw) To: ville.syrjala, dri-devel On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > All the VICs apart from 58 and 59 have the word "Hz" included in the > comment. Include it for 59 and 59 as well. > > Cc: Shashank Sharma <shashank.sharma@intel.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > --- > drivers/gpu/drm/drm_edid.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 9506933b41cd..7eec18925b70 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -957,13 +957,13 @@ static const struct drm_display_mode edid_cea_modes[] = { > 798, 858, 0, 480, 489, 495, 525, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), > .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > - /* 58 - 720(1440)x480i@240 */ > + /* 58 - 720(1440)x480i@240Hz */ > { 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 = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, }, > - /* 59 - 720(1440)x480i@240 */ > + /* 59 - 720(1440)x480i@240Hz */ > { 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 | _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment 2016-11-04 7:29 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment Andrzej Hajda @ 2016-11-08 10:10 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2016-11-08 10:10 UTC (permalink / raw) To: Andrzej Hajda; +Cc: dri-devel On Fri, Nov 04, 2016 at 08:29:11AM +0100, Andrzej Hajda wrote: > On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > All the VICs apart from 58 and 59 have the word "Hz" included in the > > comment. Include it for 59 and 59 as well. > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Applied to drm-misc. Can you pls also review patch 2/2? Thanks, Daniel > -- > Regards > Andrzej > > > --- > > drivers/gpu/drm/drm_edid.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 9506933b41cd..7eec18925b70 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -957,13 +957,13 @@ static const struct drm_display_mode edid_cea_modes[] = { > > 798, 858, 0, 480, 489, 495, 525, 0, > > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC), > > .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, }, > > - /* 58 - 720(1440)x480i@240 */ > > + /* 58 - 720(1440)x480i@240Hz */ > > { 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 = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, }, > > - /* 59 - 720(1440)x480i@240 */ > > + /* 59 - 720(1440)x480i@240Hz */ > > { 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 | > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-28 16:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20161103125342epcas1p17d427735d108a953a53995a9d1c7f79f@epcas1p1.samsung.com>
2016-11-03 12:53 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment ville.syrjala
2016-11-03 12:53 ` [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC ville.syrjala
2016-11-08 11:43 ` Andrzej Hajda
2016-11-08 13:09 ` Ville Syrjälä
2016-11-28 16:33 ` Ville Syrjälä
2016-11-04 7:29 ` [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment Andrzej Hajda
2016-11-08 10:10 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).