Re: [PATCH] drm/edid: Adding common CVT inferred modes when monitor allows range limited ones trough EDID.

[Adam Jackson <ajax@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4678 bytes --]

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@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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx