On Wed, 2012-04-11 at 21:59 -0300, Rodrigo Vivi wrote:
There are many bugs open on fd.o regarding missing modes that are supported on Windows and other closed source drivers. From EDID spec we can (might?) infer modes using GTF and CVT when monitor allows it trough range limited flag... obviously limiting by the range. From our code:
- EDID spec says modes should be preferred in this order:
- preferred detailed mode
- other detailed modes from base block
- detailed modes from extension blocks
- CVT 3-byte code modes
- standard timing codes
- established timing codes
- modes inferred from GTF or CVT range information
- We get this pretty much right.
Not actually so right... We were inferring just using GTF... not CVT or even GTF2. This patch not just add some common cvt modes but also allows some modes been inferred when using gtf2 as well.
The intent here is great, but I don't like the way this is phrased, or the implementation.
CVT monitors _must_ accept GTF as well, EDID says so. So functionally there's nothing wrong with the existing code. The thing you're trying to sneak in here is a 1600x900 timing that doesn't correspond to anything in DMT (at least, not in the copy of DMT that I have handy). It's fine to want to add more modes - although I'm still unclear exactly which machine you're trying to compensate for here - but not if it comes by sacrificing the DMT list, which is there for a reason.
So...
+static int +drm_cvt_modes_for_range(struct drm_connector *connector, struct edid *edid,
struct detailed_timing *timing)+{
- int i, modes = 0;
- struct drm_display_mode *newmode;
- struct drm_device *dev = connector->dev;
- for (i = 0; i < drm_num_cvt_inferred_modes; i++) {
if (mode_in_range(drm_cvt_inferred_modes + i, edid, timing)) {newmode = drm_mode_duplicate(dev, &drm_cvt_inferred_modes[i]);if (newmode) {drm_mode_probed_add(connector, newmode);modes++;}}- }
- return modes;
+}
The mode list you're iterating over here should just be a w/h/r/rb tuple like est3_modes. This list should _not_ duplicate any size or rate already present in the DMT list. There should be a function like this for each of CVT and GTF (and I guess dual-curve GTF too, although honestly I have no monitors left that support it with which to test), all iterating over the same list, and they should generate the timing from drm_{cvt,gtf}_mode(). The CVT version should generate RB modes if the monitor is RB-capable.
static void do_inferred_modes(struct detailed_timing *timing, void *c) { struct detailed_mode_closure *closure = c; struct detailed_non_pixel *data = &timing->data.other_data;
- int gtf = (closure->edid->features & DRM_EDID_FEATURE_DEFAULT_GTF);
- int timing_level = standard_timing_level(closure->edid);
- if (gtf && data->type == EDID_DETAIL_MONITOR_RANGE)
- if (data->type == EDID_DETAIL_MONITOR_RANGE)
switch (timing_level) {case LEVEL_DMT:break;case LEVEL_GTF:case LEVEL_GTF2:break;case LEVEL_CVT:closure->modes += drm_cvt_modes_for_range(closure->connector,closure->edid,timing);break;}}
drm_gtf_modes_for_range should be renamed drm_dmt_modes_for_range, and run unconditionally for the range descriptor. drm_*_modes_for_range will then handle generating the timings by formula.
- /* 900x600@60Hz */
- { DRM_MODE("900x600", DRM_MODE_TYPE_DRIVER, 45250, 960, 992,
1088, 1216, 0, 600, 603, 609, 624, 0,DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },- /* 1024x576@60Hz */
- { DRM_MODE("1024x576", DRM_MODE_TYPE_DRIVER, 46500, 1024, 1064,
1160, 1296, 0, 576, 579, 584, 599, 0,DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
Citation needed. Can you point to real hardware with these panel sizes, or are you just copying from Windows?
- /* 2560x1600@60Hz */
- { DRM_MODE("2560x1600", DRM_MODE_TYPE_DRIVER, 348500, 2560, 2760,
3032, 3504, 0, 1600, 1603, 1609, 1658, 0,DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
lol no. Nobody does a size this large without also doing reduced blanking.
I have a patch somewhere to fix the DMT list to re-include the reduced blanking modes (as should have been done in the first place). I'll send that along.
- ajax