set_mode_major vs RandR transforms

[Keith Packard]

Ok, so Benjamin Defnet proposed a change in the X server in where
set_mode_major was called. The essential change moves it from completely
replacing xf86CrtcSetModeTransform to replacing only the actual mode set
component, and not the assignment of new values to the crtc.

This seems like a fairly radical API change; set_mode_major will now see
only the new configuration and not be able to examine the old setup
before hand. So, let's see why this change is needed and figure out if
it's what we want.

First, the fundamental problem -- set_mode_major cannot ever see a
proposed transform, and so said transform will never be used. In fact,
in the drmmode_set_mode_major implementation, there's a simple static
assignment to transformPresent:


	crtc->transformPresent = FALSE;

That's fairly harsh, but expresses the reality that the API cannot
support transforms, so it doesn't even try.

The 'obvious' fix to this would be to pass the transform in to
set_mode_major and have it deal with that along with rotation (which
does work). That has the unfortunate effect of changing the driver ABI
along with moving a pile of duplicate code from xf86CrtcSetModeTransform
into every set_mode_major implementation.

Given that Matthias Hopf recently found a fairly significant bug in the
code would would need to be duplicated, it seems like a good idea to
avoid that if possible.

Ok, so what does the proposed patch do then? It moves the call to
set_mode_major below all of the code which changes the contents of the
crtc to match the requested state, if set_mode_major fails, it resets
the crtc back to the original state. By doing this,
xf86CrtcSetModeTransform becomes responsible for setting up the
crtc->transform and crtc->transformPresent values; set_mode_major can
then either use those directly, or have xf86CrtcRotate deal with them if
it can't handle the transform itself.

This leaves a few issues outstanding though:

 1) set_mode_major no longer sees the current state
    of the hardware. That was already partially true -- the
    output->crtc fields are lost by the time xf86CrtSetModeTransform
    is invoked, so set_mode_major can't tell how outputs are wired,
    even if it can see the current mode and position.

 2) existing set_mode_major implementations set transformPresent
    to FALSE as the old API required. Doing that now will break
    calls to xf86CrtcRotate as it depends on that value being
    set correctly in order to detect the presence of a transform.
    To make transforms work, all set_mode_major implementations
    should now leave transformPresent alone; it will be
    correctly set before set_mode_major is invoked. Of course,
    this means that the same set_mode_major will not set
    transformPresent to FALSE should a client request a
    transform. Given that clients will get surprising results
    with transforms in an old X server, I'm not sure I care
    precisely which wrong answer we get here.

 3) Without changing the ABI, drivers will have no way
    of knowing whether to assign the mode/x/y/rotation fields
    in the crtc. Doing them again is harmless though, so I
    don't see an issue here (aside from wasted code).

In short, I'm planning on pushing this change to master without bumping
the driver ABI version number, after which, I'll be pulling the patch
into the 1.6 branch for release there shortly.

Here's the commit against master today, for reference:

commit feea08b3bfe776c5b74ef454419b58307c7873d5
Author: Benjamin Defnet <benjamin.r.defnet at intel.com>
Date:   Mon Jun 8 21:45:42 2009 -0700

    hw/xf86/modes: Set crtc mode/rotation/transform before calling set_mode_major

    This moves code out of each implementation of set_mode_major and back into
    the X server. The real feature here is that the transform is now available
    in the crtc for use by either xf86CrtcRotate or whatever the driver wants to
    do. Without this change, the transform was lost for drivers providing the
    set_mode_major interface.

    Note that users of this API will want to stop smashing the transformPresent
    field, and can also stop setting mode/x/y/rotation for new enough X servers

    Signed-off-by: Keith Packard <keithp at keithp.com>

diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index b40e096..585f84d 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -266,9 +266,6 @@ xf86CrtcSetModeTransform (xf86CrtcPtr crtc, DisplayModePtr mode, Rotation rotati
     RRTransformRec     saved_transform;
     Bool               saved_transform_present;

-    if (crtc->funcs->set_mode_major)
-       return crtc->funcs->set_mode_major(crtc, mode, rotation, x, y);
-
     crtc->enabled = xf86CrtcInUse (crtc);
 
     /* We only hit this if someone explicitly sends a "disabled" modeset. */
@@ -306,6 +303,11 @@ xf86CrtcSetModeTransform (xf86CrtcPtr crtc, DisplayModePtr mode, Rotation rotati
     } else
        crtc->transformPresent = FALSE;

+    if (crtc->funcs->set_mode_major) {
+       ret = crtc->funcs->set_mode_major(crtc, mode, rotation, x, y);
+       goto done;
+    }
+
     /* Pass our mode to the outputs and the CRTC to give them a chance to
      * adjust it according to limitations or output properties, and also
      * a chance to reject the mode entirely.


-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.x.org/archives/xorg-devel/attachments/20090608/a77c32e0/attachment.pgp