[PATCH resent] Option to use DPI calculated from EDID

[Guillem Jover]

Hi!

On Mon, 2011-10-31 at 13:16:21 +0100, Michal Suchanek wrote:
> It adds back functionality lost a few years ago which makes X
> needlessly hard to use on devices that report correct DPI and aren't
> 96 DPI which is the value unconditionally set since commit fff00d

This affected me recently too when setting up a new laptop, quite
annoying. Here's a quick review.

> From 33a200fa5b670a9e479e8d9be533ec1112867213 Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at centrum.cz>
> Date: Sat, 24 Sep 2011 18:46:29 +0200
> Subject: [PATCH] Add option to set default DPI
> 
>  The default DPI can be either specified as fixed value or 'auto'
>  meaning DDC is used when available.
> 
>  Based on patch by Nick Bowler

> Index: xserver/hw/xfree86/common/xf86Config.c
> ===================================================================
> --- xserver.orig/hw/xfree86/common/xf86Config.c	2011-10-11 18:58:07.000000000 +0200
> +++ xserver/hw/xfree86/common/xf86Config.c	2011-10-11 18:59:40.000000000 +0200
> @@ -880,6 +883,15 @@
>  	}
>      }
>  
> +
> +    xf86Info.defaultDPI = DEFAULT_DPI;
> +    if ((s = xf86GetOptValString(FlagOptions, FLAG_DEFAULT_DPI))) {
> +	if (!xf86NameCmp(s, "auto")) {
> +	    xf86Info.defaultDPI = 0;
> +	} else {
> +	    xf86Info.defaultDPI = strtoul(s, NULL, 0);

This does not check for parsing errors.

> +	}
> +    }
>  #ifdef RANDR
>      xf86Info.disableRandR = FALSE;
>      xf86Info.randRFrom = X_DEFAULT;
> Index: xserver/hw/xfree86/man/xorg.conf.man
> ===================================================================
> --- xserver.orig/hw/xfree86/man/xorg.conf.man	2011-10-10 17:52:26.000000000 +0200
> +++ xserver/hw/xfree86/man/xorg.conf.man	2011-10-11 18:59:40.000000000 +0200
> @@ -544,6 +544,12 @@
>  and are passed to clients.
>  Default: off.
>  .TP 7
> +.BI "Option \*qDefaultDPI\*q  \*q" string \*q
> +This option allows to set a fixed DPI which is reported for all screens or the

"allows to" is not correct, I guess either "makes it possible to" or
some sentence rewording to avoid the usage.

> +special value \*qauto\*q which infers DPI from screen size read from DDC. The
> +fallback in case DDC reading fails is 96.
> +Default: 96.
> +.TP 7
>  .BI "Option \*qDisableVidModeExtension\*q  \*q" boolean \*q
>  This disables the parts of the VidMode extension used by the xvidtune client
>  that can be used to change the video modes.
> Index: xserver/hw/xfree86/modes/xf86RandR12.c
> ===================================================================
> --- xserver.orig/hw/xfree86/modes/xf86RandR12.c	2011-10-11 18:58:07.000000000 +0200
> +++ xserver/hw/xfree86/modes/xf86RandR12.c	2011-10-11 18:59:40.000000000 +0200
> @@ -807,6 +807,7 @@
>  	else
>  	{
>  	    xf86OutputPtr   output = xf86CompatOutput(pScrn);
> +	    xf86CrtcPtr     crtc   = output->crtc;
>  
>  	    if (output &&
>  		output->conf_monitor &&
> @@ -819,11 +820,27 @@
>  		mmWidth = output->conf_monitor->mon_width;
>  		mmHeight = output->conf_monitor->mon_height;
>  	    }
> -	    else
> +	    else if (!xf86Info.defaultDPI && crtc && crtc->mode.HDisplay &&
> +		     output->mm_width && output->mm_height)

You could move the «else if (xf86Info.defaultDPI)» before this one and
avoid having to check for !xf86Info.defaultDPI.

> +	    {
> +		/*
> +		 * If the output has a mode and a declared size, use that
> +		 * to scale the screen size
> +		 */
> +		DisplayModePtr	mode = &crtc->mode;
> +		mmWidth = output->mm_width * width / mode->HDisplay;
> +		mmHeight = output->mm_height * height / mode->VDisplay;
> +	    }
> +	    else if (xf86Info.defaultDPI)
>  	    {
>  		/*
>  		 * Otherwise, just set the screen to DEFAULT_DPI
>  		 */

This comment seems misplaced.

> +		mmWidth = width * 25.4 / xf86Info.defaultDPI;
> +		mmHeight = height * 25.4 / xf86Info.defaultDPI;
> +	    }
> +	    else
> +	    {
>  		mmWidth = width * 25.4 / DEFAULT_DPI;
>  		mmHeight = height * 25.4 / DEFAULT_DPI;
>  	    }

Thanks,
Guillem