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:
> closure->modes += drm_gtf_modes_for_range(closure->connector,
> closure->edid,
> timing);
> + 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 at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120412/81cc067d/attachment.pgp>
