diff mbox

change initial modesetting if outputs are aligned in 1 dimension

Message ID 1306922798-29344-1-git-send-email-florian@mickler.org
State New, archived
Headers show

Commit Message

Florian Mickler June 1, 2011, 10:06 a.m. UTC
Recently the kernel started reporting my outputs in a different ordering due to

    commit cb0953d734
    (drm/i915: Initialize LVDS and eDP outputs before anything else)

Which made X choose a "wrong" resolution for my VGA display. Since they are
aligned horizontally, I wish them to be aligned in vertical
Resolution only.

Before this patch, the sum of squared distances would force my VGA display
(1680x1050 native resolution) to 1280x1024 (non-native) due to my internal
display beeing considered first and 1400x1050 as native resolution.

This was not an issue the other way around (VGA beeing first) because 1400x1050
is nearest to 1680x1050 anyway.

This patch changes the heuristic to only align resolution vertically if the
displays are horizontally aligned, or vice versa.

Signed-off-by: Florian Mickler <florian@mickler.org>
---

Ok, Adam... seems I lost the staring contest... :)
What about something like this?

 hw/xfree86/modes/xf86Crtc.c |  115 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 96 insertions(+), 19 deletions(-)

Comments

Adam Jackson June 1, 2011, 7:30 p.m. UTC | #1
On 6/1/11 6:06 AM, Florian Mickler wrote:
> Recently the kernel started reporting my outputs in a different ordering due to
>
>      commit cb0953d734
>      (drm/i915: Initialize LVDS and eDP outputs before anything else)
>
> Which made X choose a "wrong" resolution for my VGA display. Since they are
> aligned horizontally, I wish them to be aligned in vertical
> Resolution only.
>
> Before this patch, the sum of squared distances would force my VGA display
> (1680x1050 native resolution) to 1280x1024 (non-native) due to my internal
> display beeing considered first and 1400x1050 as native resolution.
>
> This was not an issue the other way around (VGA beeing first) because 1400x1050
> is nearest to 1680x1050 anyway.
>
> This patch changes the heuristic to only align resolution vertically if the
> displays are horizontally aligned, or vice versa.
>
> Signed-off-by: Florian Mickler<florian@mickler.org>
> ---
>
> Ok, Adam... seems I lost the staring contest... :)
> What about something like this?

At this point, given the near-unity overlap of "RANDRful drivers" and 
"usable KMS support", I think I'd prefer something more like:

http://pkgs.fedoraproject.org/gitweb/?p=xorg-x11-server.git;a=blob_plain;f=xserver-1.6.99-right-of.patch;h=a0c9e7f64b98a091f64faaf5a8a432e2122e25f9;hb=HEAD

particularly once per-crtc pixmaps land.

- ajax
Florian Mickler June 1, 2011, 8:44 p.m. UTC | #2
On Wed, 01 Jun 2011 15:30:03 -0400
Adam Jackson <ajax@redhat.com> wrote:

> On 6/1/11 6:06 AM, Florian Mickler wrote:
> > Recently the kernel started reporting my outputs in a different ordering due to
> >
> >      commit cb0953d734
> >      (drm/i915: Initialize LVDS and eDP outputs before anything else)
> >
> > Which made X choose a "wrong" resolution for my VGA display. Since they are
> > aligned horizontally, I wish them to be aligned in vertical
> > Resolution only.
> >
> > Before this patch, the sum of squared distances would force my VGA display
> > (1680x1050 native resolution) to 1280x1024 (non-native) due to my internal
> > display beeing considered first and 1400x1050 as native resolution.
> >
> > This was not an issue the other way around (VGA beeing first) because 1400x1050
> > is nearest to 1680x1050 anyway.
> >
> > This patch changes the heuristic to only align resolution vertically if the
> > displays are horizontally aligned, or vice versa.
> >
> > Signed-off-by: Florian Mickler<florian@mickler.org>
> > ---
> >
> > Ok, Adam... seems I lost the staring contest... :)
> > What about something like this?
> 
> At this point, given the near-unity overlap of "RANDRful drivers" and 
> "usable KMS support", I think I'd prefer something more like:
> 
> http://pkgs.fedoraproject.org/gitweb/?p=xorg-x11-server.git;a=blob_plain;f=xserver-1.6.99-right-of.patch;h=a0c9e7f64b98a091f64faaf5a8a432e2122e25f9;hb=HEAD
> 
> particularly once per-crtc pixmaps land.
> 
> - ajax

I think using horizontal spanning as a default is a good idea. 

Also bringing all outputs up in their preferred mode could be the right
move.

[That commit in question wouldn't help my case, since I have a "Right Of" in the
xorg.conf and thus it would use a different code-path (right now).]


When there is no preferred mode, xf86ClosestMode (probably enhanced to
respect horizontal or vertical relation in some form) would still be
needed... choosing of mode and positioning of outputs
relative to each other should probably be decoupled...

Regards,
Flo
Adam Jackson June 1, 2011, 8:48 p.m. UTC | #3
On 6/1/11 4:44 PM, Florian Mickler wrote:
> I think using horizontal spanning as a default is a good idea.
>
> Also bringing all outputs up in their preferred mode could be the right
> move.
>
> [That commit in question wouldn't help my case, since I have a "Right Of" in the
> xorg.conf and thus it would use a different code-path (right now).]

Oh, sure, I see what you're saying.

> When there is no preferred mode, xf86ClosestMode (probably enhanced to
> respect horizontal or vertical relation in some form) would still be
> needed... choosing of mode and positioning of outputs
> relative to each other should probably be decoupled...

Probably.  But in your case, why would you not just pick the preferred 
mode for all outputs, rather than contorting to try to pick modes with 
the same height?

- ajax
Florian Mickler June 1, 2011, 9:06 p.m. UTC | #4
On Wed, 01 Jun 2011 16:48:30 -0400
Adam Jackson <ajax@redhat.com> wrote:

> On 6/1/11 4:44 PM, Florian Mickler wrote:
> > I think using horizontal spanning as a default is a good idea.
> >
> > Also bringing all outputs up in their preferred mode could be the right
> > move.
> >
> > [That commit in question wouldn't help my case, since I have a "Right Of" in the
> > xorg.conf and thus it would use a different code-path (right now).]
> 
> Oh, sure, I see what you're saying.
> 
> > When there is no preferred mode, xf86ClosestMode (probably enhanced to
> > respect horizontal or vertical relation in some form) would still be
> > needed... choosing of mode and positioning of outputs
> > relative to each other should probably be decoupled...
> 
> Probably.  But in your case, why would you not just pick the preferred 
> mode for all outputs, rather than contorting to try to pick modes with 
> the same height?
> 
> - ajax

Well... because the endeffect is the same in my case. :) 

But yes, if there is a preferred mode, that should be used. 

Is there a preferred mode in all cases? 

Regards,
Flo
Adam Jackson June 1, 2011, 9:30 p.m. UTC | #5
On 6/1/11 5:06 PM, Florian Mickler wrote:

> Well... because the endeffect is the same in my case. :)
>
> But yes, if there is a preferred mode, that should be used.
>
> Is there a preferred mode in all cases?

For any display with a discrete number of pixels (LCDs, etc), almost 
always.  CRTs, most of the time, although it's not always the mode the 
user wants.

- ajax
diff mbox

Patch

diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index 4e3f6bf..96c6c62 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -872,15 +872,92 @@  xf86DefaultMode (xf86OutputPtr output, int width, int height)
     return target_mode;
 }
 
+static Bool
+xf86GetOptRelation(xf86OutputPtr output, OutputOpts *relation, char **relative_name)
+{
+	int r;
+	static const OutputOpts	relations[] = {
+	    OPTION_BELOW, OPTION_RIGHT_OF, OPTION_ABOVE, OPTION_LEFT_OF
+	};
+
+	*relation = 0;
+	*relative_name = NULL;
+
+	for (r = 0; r < 4; r++)
+	{
+	    *relation = relations[r];
+	    *relative_name = xf86GetOptValString (output->options,
+						     *relation);
+	    if (*relative_name)
+		break;
+	}
+	return (*relative_name != NULL);
+}
+
+static Bool
+xf86OutputsRelatedHorizontal(xf86OutputPtr out1, xf86OutputPtr out2)
+{
+	OutputOpts relation;
+	char *other_name;
+	Bool ret = FALSE;
+
+	if (xf86GetOptRelation(out1, &relation, &other_name) &&
+			strcmp(other_name, out2->name) == 0) {
+		/*
+		 * out1 related to out2
+		 */
+		ret = (relation == OPTION_RIGHT_OF ||
+			relation == OPTION_LEFT_OF);
+	}
+	if (!ret && xf86GetOptRelation(out2, &relation, &other_name) &&
+			strcmp(other_name, out1->name) == 0) {
+		/*
+		 * out2 related to out1
+		 */
+		ret = (relation == OPTION_RIGHT_OF ||
+			relation == OPTION_LEFT_OF);
+	}
+
+	return ret;
+
+}
+
+static Bool
+xf86OutputsRelatedVertical(xf86OutputPtr out1, xf86OutputPtr out2)
+{
+	OutputOpts relation;
+	char *other_name;
+	Bool ret = FALSE;
+
+	if (xf86GetOptRelation(out1, &relation, &other_name) &&
+			strcmp(other_name, out2->name) == 0) {
+		/*
+		 * out1 related to out2
+		 */
+		ret = (relation == OPTION_ABOVE ||
+			relation == OPTION_BELOW);
+	}
+	if (!ret && xf86GetOptRelation(out2, &relation, &other_name) &&
+			strcmp(other_name, out1->name) == 0) {
+		/*
+		 * out2 related to out1
+		 */
+		ret = (relation == OPTION_ABOVE ||
+			relation == OPTION_BELOW);
+	}
+
+	return ret;
+}
+
 static DisplayModePtr
-xf86ClosestMode (xf86OutputPtr output, 
+xf86ClosestMode (xf86OutputPtr output, xf86OutputPtr match_output,
 		 DisplayModePtr match, Rotation match_rotation,
 		 int width, int height)
 {
     DisplayModePtr  target_mode = NULL;
     DisplayModePtr  mode;
     int		    target_diff = 0;
-    
+
     /*
      * Pick a mode closest to the specified mode
      */
@@ -892,15 +969,26 @@  xf86ClosestMode (xf86OutputPtr output,
 	if (xf86ModeWidth (mode, output->initial_rotation) > width ||
 	    xf86ModeHeight (mode, output->initial_rotation) > height)
 	    continue;
-	
+
 	/* exact matches are preferred */
 	if (output->initial_rotation == match_rotation &&
 	    xf86ModesEqual (mode, match))
 	    return mode;
-	
+
 	dx = xf86ModeWidth (match, match_rotation) - xf86ModeWidth (mode, output->initial_rotation);
 	dy = xf86ModeHeight (match, match_rotation) - xf86ModeHeight (mode, output->initial_rotation);
-	diff = dx * dx + dy * dy;
+
+	/*
+	 * If we are aligning screens horizontally or vertically use
+	 * only one dimension for matching
+	 */
+	if (xf86OutputsRelatedHorizontal(output, match_output))
+			diff = dy * dy;
+	else if (xf86OutputsRelatedVertical(output, match_output))
+			diff = dx * dx;
+	else
+		diff = dx * dx + dy * dy;
+
 	xf86DrvMsg (0, X_INFO,
 		"XF86ClosestMode: Output %s, Mode %s [ dx = %i, dy = %i, diff = %i ]\n",
 		output->name, mode->name, dx, dy, diff);
@@ -1122,10 +1210,6 @@  xf86UserConfiguredOutputs(ScrnInfoPtr scrn, DisplayModePtr *modes)
 	char	    *position;
 	char	    *relative_name;
 	OutputOpts	    relation;
-	int r;
-	static const OutputOpts	relations[] = {
-	    OPTION_BELOW, OPTION_RIGHT_OF, OPTION_ABOVE, OPTION_LEFT_OF
-	};
 
 	position = xf86GetOptValString (output->options,
 					OPTION_POSITION);
@@ -1137,15 +1221,7 @@  xf86UserConfiguredOutputs(ScrnInfoPtr scrn, DisplayModePtr *modes)
 	}
 	relation = 0;
 	relative_name = NULL;
-	for (r = 0; r < 4; r++)
-	{
-	    relation = relations[r];
-	    relative_name = xf86GetOptValString (output->options,
-						     relation);
-	    if (relative_name)
-		break;
-	}
-	if (relative_name) {
+	if (xf86GetOptRelation(output, &relation, &relative_name)) {
 	    user_conf = TRUE;
 	    xf86DrvMsg (scrn->scrnIndex, X_INFO,
 			"Output %s has user_conf due to relation: %s\n",
@@ -2245,7 +2321,8 @@  xf86TargetFallback(ScrnInfoPtr scrn, xf86CrtcConfigPtr config,
     /* Fill in other output modes */
     for (o = -1; nextEnabledOutput(config, enabled, &o); ) {
 	if (!modes[o])
-	    modes[o] = xf86ClosestMode(config->output[o], target_mode,
+	    modes[o] = xf86ClosestMode(config->output[o],
+			    config->output[config->compat_output], target_mode,
 				       target_rotation, width, height);
     }