Created attachment 8498 [details]
DisplaySize patch

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

Confirm!  After update to Xorg-7.2 all fonts screwed up (looks like condensed and overlapped letters) at resolution 1152x864. Driver is VIA (unichrome).  Specifying dpi in command line solves problem.

Redhat has carried a version of this patch for some time now, how about applying it on master?

Created attachment 14642 [details] [review]
Updated patch (for xorg-server 1.4.1~git20080131)

I confirm the bug. DisplaySize is broken, with a side effect: the log never reports that DPI settings have be probed ("**" instead of "--").
So I think it's better not to modify widthmm & heightmm at all in xf86DDCMonitorSet.

I've updated the patch against a recent version of xorg-server (1.4.1~git20080131 is the latest version packaged by Debian).

Please consider apply it.

See also Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=421772

I'd like to add that the above patch fixes the chronic text clipping I was getting with KHTML in KDE 4 on my ancient CRT that is very much fond of being able to manually set DisplaySize. (My HW is a CRT + x700 AGP video card with the radeon driver.)

i assume this is still broken, making the overrides work would almost certainly be useful.

Fixed by ajax in commit 3b73d62791d925c465ec855f96981d151dd3c179.

I haven’t read much of the code, only some bits in Monitor.c, Configint.h and scan.c. But …

    if (Monitor->widthmm <= 0 && Monitor->heightmm <= 0) {
	Monitor->widthmm = 10 * DDC->features.hsize;
	Monitor->heightmm = 10 * DDC->features.vsize;
    }

only overwrites the values if *both*, widthmm and heightmm, are <= 0. If I read the code correctly, this happens
a) if atof returned an error, or
b) if the user explicitly set a negative or zero value in the config file, or
c) if the value is not set in the config file at all.

Shouldn’t this be

    if (Monitor->widthmm <= 0 || Monitor->heightmm <= 0) {
	Monitor->widthmm = 10 * DDC->features.hsize;
	Monitor->heightmm = 10 * DDC->features.vsize;
    }

instead? Or – even better – check/set the values separately, one by one?

Nico: thanks for that, fixed as 83023ffd09a84ff48e6b99f57ebad101a00478db

comment #9 is wrong.
Did you read last attachment (patch) and the code of xf86SetDpi (hw/xfree86/common/xf86Helper.c) ?

I don't know which is the better between commit 3b73d62 and the suggested patch. It depends if xf86SetDpi will be always called or not after. For me, commit 3b73d62 does useless work.

But I can't agree with commit 83023ff because it doesn't allow to set only 1 dimension (either height or width). As you can see in xf86SetDpi, if only Monitor->widthmm is > 0 (and Monitor->heightmm == 0)(or vice versa), the code automatically set the same DPI for height and width.

There are still bugs here with the current git version.

in hw/xfree86/modes/xf86Crtc.c:

handle_detailed_physical_size()

        p->output->mm_width = det_mon->section.d_timings.h_size;
        p->output->mm_height = det_mon->section.d_timings.v_size;

This unconditionally sets mm width and height from the detailed timings.

hw/xfree86/ddc/interpret_edid.c:

encode_aspect_ratio()

This function contains a quirk which determines whether the mm width and height in the detailed timings presented by the monitor are actually the aspect ratio.
If so:

        m->features.hsize = m->features.vsize = 0;

This results in a message like this in the log:
  "Quirked EDID physical size to 0x0 cm"


Back to hw/xfree86/modes/xf86Crtc.c:

xf86OutputSetEDID()

In this function, if the mm width and height haven't been determined by the DDC detailed timings probe (with no check whether they are valid), they are then set to values in the max size field (features.hsize and features.vsize) as long as it wasn't quirked to 0x0 due to the detailed timings being invalid... ...too late now!

Of course, if no information is available, the xrandr output dimensions needs to be set to something, whether that's 0x0 or 160x90 mm doesn't matter so much, either way it's not reflecting reality.  However, all this convolution completely overrides the DisplaySize value the user may have set.  Since it's sourced in hw/xfree86/modes/xf86RandR12.c, but then overridden during DDC mode probes it makes no difference.

Created attachment 87022 [details] [review]
patch against current git master to respect DisplaySize

By setting the output "mm sizes" to the user values from DisplaySize after the DDC probe but before checking whether they are set and falling back to the max size field gives priority to the user config.

(copied from hw/xfree86/modes/xf86RandR12.c)

Hoped that would be enough, but it seems there's also need to address the issue of unconditionally overriding the value in the handle_detailed_physical_size().

I've just removed the code here (which works with my test case), but that could well break something else... :/

Just to add for clarity, it shouldn't be surprising that I had to remove:

hw/xfree86/modes/xf86Crtc.c:

handle_detailed_physical_size() ...
      p->output->mm_width = det_mon->section.d_timings.h_size;
      p->output->mm_height = det_mon->section.d_timings.v_size;

since it seems to be called on each DDC probe and not just when the connected output device has changed.  This should at least be conditional on DisplaySize not being set.

I don't think this has really been noticed since the core DPI is no longer tied to the output size/resolution, but any app wanting to make use of the actual display characteristics via randr will potentially get some very wrong values!

Created attachment 88557 [details] [review]
patch against current git master to respect DisplaySize (20131103)

This should be sufficient to make it work.

Always uses user supplied DisplaySize in preference to detected size if specified.

Also reports if monitor features and detailed timings report differing sizes, this is/was just to help debug and can be dropped.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/xserver/issues/246.