On Wed, Dec 02, 2015 at 02:57:39PM +0200, Jani Nikula wrote:
> On Wed, 02 Dec 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Wed, Dec 02, 2015 at 01:46:40PM +0200, Jani Nikula wrote:
> >> On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> >> > On Tue, Dec 01, 2015 at 05:16:54PM +0200, Jani Nikula wrote:
> >> >> Add a dummy entry to CEA/HDMI mode tables so they can be indexed
> >> >> directly using the VIC, avoiding a +1/-1 dance here and there. This adds
> >> >> clarity to the error checking for various functions that return the VIC
> >> >> on success and zero on failure; we now explicitly check for 0 instead of
> >> >> just subtracting one from an unsigned type.
> >> >>
> >> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> >>
> >> >> ---
> >> >>
> >> >> This is on top of
> >> >>
> >> >> commit 4c6bcf44549907cb50b67f98eb13717a4adc6b33
> >> >> Author: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> >> Date: Mon Nov 16 21:05:12 2015 +0200
> >> >>
> >> >> drm/edid: Make the detailed timing CEA/HDMI mode fixup accept up to 5kHz clock difference
> >> >>
> >> >> in drm-intel/topic/drm-misc
> >> >> ---
> >> >> drivers/gpu/drm/drm_edid.c | 84 ++++++++++++++++++++++++----------------------
> >> >> 1 file changed, 43 insertions(+), 41 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> >> index c214f1246cb4..aea09eae407e 100644
> >> >> --- a/drivers/gpu/drm/drm_edid.c
> >> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> > <snip>
> >> >> @@ -2713,16 +2716,16 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
> >> >> list_for_each_entry(mode, &connector->probed_modes, head) {
> >> >> const struct drm_display_mode *cea_mode = NULL;
> >> >> struct drm_display_mode *newmode;
> >> >> - u8 mode_idx = drm_match_cea_mode(mode) - 1;
> >> >> + u8 vic = drm_match_cea_mode(mode);
> >> >> unsigned int clock1, clock2;
> >> >>
> >> >> - if (mode_idx < ARRAY_SIZE(edid_cea_modes)) {
> >> >> - cea_mode = &edid_cea_modes[mode_idx];
> >> >> + if (vic > 0 && vic < ARRAY_SIZE(edid_cea_modes)) {
> >> >
> >> > Maybe add some sort of vic_is_valid() helpers to avoid duplicating
> >> > these in so many places?
> >>
> >> Any thoughts on how that should handle CEA vs. HDMI VICs, comparing
> >> against ARRAY_SIZE(edid_cea_modes) and ARRAY_SIZE(edid_4k_modes)?
> >
> > I was thinking one function for each type, and yeah just
> >
> > return vic != 0 && vic < ARRAY_SIZE(whatever_modes);
> >
> > or something along those lines.
> >
> >>
> >> Add versions of drm_match_*_mode*() that return struct drm_display_mode
> >> * or NULL on errors?
> >
> > Some time ago I was readin the DP spec, and ended up sketching out
> > some functions that might help deal with EDIDless DP branch devices.
> > For that I cooked up a function to return a pointer to the appropriate
> > CEA mode based on the vic (or NULL for bad vics). So I guess if you did
> > something like that + take the already existing match function, you
> > could wrap them in a neat package that returns the matching cea mode
> > for whatever mode is passed in.
>
> Mmmh, so how about just taking this patch as-is for now...?
We can do that. But I just figured it wouldn't be too much extra work
to yank yank the "is this a valid vic" logic into two small functions.
>
> BR,
> Jani.
>
>
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> > Otherwise I like it.
> >> >
> >> >> + cea_mode = &edid_cea_modes[vic];
> >> >> clock2 = cea_mode_alternate_clock(cea_mode);
> >> >> } else {
> >> >> - mode_idx = drm_match_hdmi_mode(mode) - 1;
> >> >> - if (mode_idx < ARRAY_SIZE(edid_4k_modes)) {
> >> >> - cea_mode = &edid_4k_modes[mode_idx];
> >> >> + vic = drm_match_hdmi_mode(mode);
> >> >> + if (vic > 0 && vic < ARRAY_SIZE(edid_4k_modes)) {
> >> >> + cea_mode = &edid_4k_modes[vic];
> >> >> clock2 = hdmi_mode_alternate_clock(cea_mode);
> >> >> }
> >> >> }
> >> >> @@ -2773,17 +2776,17 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
> >> >> {
> >> >> struct drm_device *dev = connector->dev;
> >> >> struct drm_display_mode *newmode;
> >> >> - u8 cea_mode;
> >> >> + u8 vic;
> >> >>
> >> >> if (video_db == NULL || video_index >= video_len)
> >> >> return NULL;
> >> >>
> >> >> /* CEA modes are numbered 1..127 */
> >> >> - cea_mode = (video_db[video_index] & 127) - 1;
> >> >> - if (cea_mode >= ARRAY_SIZE(edid_cea_modes))
> >> >> + vic = (video_db[video_index] & 127);
> >> >> + if (vic == 0 || vic >= ARRAY_SIZE(edid_cea_modes))
> >> >> return NULL;
> >> >>
> >> >> - newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
> >> >> + newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> >> >> if (!newmode)
> >> >> return NULL;
> >> >>
> >> >> @@ -2878,8 +2881,7 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
> >> >> struct drm_device *dev = connector->dev;
> >> >> struct drm_display_mode *newmode;
> >> >>
> >> >> - vic--; /* VICs start at 1 */
> >> >> - if (vic >= ARRAY_SIZE(edid_4k_modes)) {
> >> >> + if (vic == 0 || vic >= ARRAY_SIZE(edid_4k_modes)) {
> >> >> DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
> >> >> return 0;
> >> >> }
> >> >> @@ -3170,24 +3172,24 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
> >> >> {
> >> >> const struct drm_display_mode *cea_mode;
> >> >> int clock1, clock2, clock;
> >> >> - u8 mode_idx;
> >> >> + u8 vic;
> >> >> const char *type;
> >> >>
> >> >> /*
> >> >> * allow 5kHz clock difference either way to account for
> >> >> * the 10kHz clock resolution limit of detailed timings.
> >> >> */
> >> >> - mode_idx = drm_match_cea_mode_clock_tolerance(mode, 5) - 1;
> >> >> - if (mode_idx < ARRAY_SIZE(edid_cea_modes)) {
> >> >> + vic = drm_match_cea_mode_clock_tolerance(mode, 5);
> >> >> + if (vic > 0 && vic < ARRAY_SIZE(edid_cea_modes)) {
> >> >> type = "CEA";
> >> >> - cea_mode = &edid_cea_modes[mode_idx];
> >> >> + cea_mode = &edid_cea_modes[vic];
> >> >> clock1 = cea_mode->clock;
> >> >> clock2 = cea_mode_alternate_clock(cea_mode);
> >> >> } else {
> >> >> - mode_idx = drm_match_hdmi_mode_clock_tolerance(mode, 5) - 1;
> >> >> - if (mode_idx < ARRAY_SIZE(edid_4k_modes)) {
> >> >> + vic = drm_match_hdmi_mode_clock_tolerance(mode, 5);
> >> >> + if (vic > 0 && vic < ARRAY_SIZE(edid_4k_modes)) {
> >> >> type = "HDMI";
> >> >> - cea_mode = &edid_4k_modes[mode_idx];
> >> >> + cea_mode = &edid_4k_modes[vic];
> >> >> clock1 = cea_mode->clock;
> >> >> clock2 = hdmi_mode_alternate_clock(cea_mode);
> >> >> } else {
> >> >> @@ -3205,7 +3207,7 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
> >> >> return;
> >> >>
> >> >> DRM_DEBUG("detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
> >> >> - type, mode_idx + 1, mode->clock, clock);
> >> >> + type, vic, mode->clock, clock);
> >> >> mode->clock = clock;
> >> >> }
> >> >>
> >> >> --
> >> >> 2.1.4
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
