On Tue, Nov 08, 2016 at 12:43:55PM +0100, Andrzej Hajda wrote:
> On 03.11.2016 13:53, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Cc: Adam Jackson <ajax@xxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>
>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
