Bug 9758 - xorg-server (xf86DDCMonitorSet) does not honor xorg.conf's DisplaySize
Summary: xorg-server (xf86DDCMonitorSet) does not honor xorg.conf's DisplaySize
Status: RESOLVED MOVED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: 7.2 (2007.02)
Hardware: All All
: low minor
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: xserver-1.7.x
  Show dependency treegraph
 
Reported: 2007-01-25 01:06 UTC by Paul Bender
Modified: 2018-12-13 18:34 UTC (History)
10 users (show)

See Also:
i915 platform:
i915 features:


Attachments
DisplaySize patch (841 bytes, text/plain)
2007-01-25 01:07 UTC, Paul Bender
no flags Details
Updated patch (for xorg-server 1.4.1~git20080131) (866 bytes, patch)
2008-02-28 04:33 UTC, Julien Muchembled
no flags Details | Splinter Review
patch against current git master to respect DisplaySize (682 bytes, patch)
2013-10-03 10:32 UTC, Steven Newbury
no flags Details | Splinter Review
patch against current git master to respect DisplaySize (20131103) (2.32 KB, patch)
2013-11-03 12:09 UTC, Steven Newbury
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Bender 2007-01-25 01:06:07 UTC
According to the xorg.conf manual page, if DisplaySize is set, then Xorg will use it to calculate the DPI values. Indeed, in xorg-server 1.0.1, this is how it behaved. However, in xorg-server 1.2.0, it does not behave this way. Instead, if DDC returns a display size, then the DDC returned display size overrides the DisplaySize value in xorg.conf.

The problem appears to be in the xf86DDCMonitorSet function in 'hw/xfree86/ddc/ddcProperty.c'. Unlike vertical refresh and horizontal sync which are only set to their DDC values when they are not already set, height and width are set to their DDC values unconditionally.

I have attached a patch. This patch overrides the values individually. However, since DisplaySize sets both parameters, one may wish to make the override and all or nothing decision.
Comment 1 Paul Bender 2007-01-25 01:07:02 UTC
Created attachment 8498 [details]
DisplaySize patch
Comment 2 Daniel Stone 2007-02-27 01:35:55 UTC
Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.
Comment 3 Max Arnold 2007-05-22 21:07:24 UTC
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.
Comment 4 Timo Aaltonen 2008-02-05 08:25:46 UTC
Redhat has carried a version of this patch for some time now, how about applying it on master?
Comment 5 Julien Muchembled 2008-02-28 04:33:15 UTC
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
Comment 6 James Spencer 2008-03-31 14:39:52 UTC
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.)
Comment 7 Daniel Stone 2009-08-31 18:18:16 UTC
i assume this is still broken, making the overrides work would almost certainly be useful.
Comment 8 Peter Hutterer 2009-09-21 21:30:29 UTC
Fixed by ajax in commit 3b73d62791d925c465ec855f96981d151dd3c179.
Comment 9 Nico R. 2009-09-22 03:21:40 UTC
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?
Comment 10 Peter Hutterer 2009-09-27 21:55:05 UTC
Nico: thanks for that, fixed as 83023ffd09a84ff48e6b99f57ebad101a00478db
Comment 11 Julien Muchembled 2009-10-09 04:34:51 UTC
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.
Comment 12 Steven Newbury 2013-10-03 10:26:04 UTC
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.
Comment 13 Steven Newbury 2013-10-03 10:32:03 UTC
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)
Comment 14 Steven Newbury 2013-10-03 10:50:48 UTC
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... :/
Comment 15 Steven Newbury 2013-10-08 13:47:08 UTC
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!
Comment 16 Steven Newbury 2013-11-03 12:09:30 UTC
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.
Comment 17 GitLab Migration User 2018-12-13 18:34:47 UTC
-- 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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.