On Tue, Nov 08, 2016 at 12:43:55PM +0100, Andrzej Hajda wrote:
> On 03.11.2016 13:53, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 at intel.com>
> > Cc: Andrzej Hajda <a.hajda at samsung.com>
> > Cc: Adam Jackson <ajax at redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at 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 at 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
