RandR (etc) DriverFunc

[Andy Ritger]

Sorry I didn't get a chance to respond to this yesterday, Thomas
and Egbert.

This is a good point, Thomas.

In the SetConfig case, it's nice for the driver to be notified
when the size is changed, even if the rotation is not changing (eg,
to reallocate vidmem for the root window's new resolution), though
I'd say it's not crucial (atleast for the NVIDIA driver).

There does seem to be a fundamental problem with interpretting the
return value of DriverFunc:

    TRUE  - DriverFunc() recognized xorgDriverFuncOp and successfully
            handled the request

    FALSE - DriverFunc() recognized xorgDriverFuncOp but failed
            while processing it, in which case the caller should
            presumably abort its operation

                - or -

            DriverFunc() did not recognize xorgDriverFuncOp; it is
            caller-specific how to react to this.  In some cases,
            the caller should probably continue, but in other
            cases, we might want to bail out because DriverFunc()
            doesn't provide the support we need.

There are 3 possible states that the common X server code needs to
infer from the X driver's boolean return value :(

At this point, there are drivers shipping (eg: the NVIDIA driver)
that plug into DriverFunc to provide rotation support, so I'd ask
that we not change the semantics of DriverFunc() or otherwise break
binary compatibility.

This proc was originally named RRFunc and was intended to be specific
to RandR.  Egbert generalized it so that it could be used for other
things, which seems like a good idea, except for this issue of
overloading the return value.

I expect DriverFunc won't scale well in the future due to this
return value problem.

I'll offer a solution; it may not be the best solution, but it does
maintain binary compatibility and solves the return value dilemma
going forward:

    - rename DriverFunc back to RRFunc and only use it for
      RR_GET_INFO and RR_SET_CONFIG.  We alter the semantics of
      this proc such that if a driver provides this function then
      it must recognize both those values.

    - add a new proc to ScrnInfoRec named DriverFunc.  This new
      DriverFunc would return a tri-state enum; eg:

        typedef enum {
            DRIVER_FUNC_SUCCESS = 0,
            DRIVER_FUNC_FAILURE = 1,
            DRIVER_FUNC_UNRECOGIZED = 2
        } DriverFuncReturnCodes;

I'm not sure how this solution would impact the
GET_REQUIRED_HW_INTERFACES request used with the current DriverFunc.
This solution would also break compile-time compatibility with
drivers built outside the Xorg tree.  We might want to name the new
proc "DriverFunc2" or something different so as not to be confused
with the current DriverFunc.

Thoughts?

Thanks,
- Andy


On Wed, 16 Mar 2005, Thomas Winischhofer wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Egbert Eich wrote:
> | Thomas Winischhofer writes:
> |  > xf86str.h states that the driver should return FALSE if a driver
> |  > function is not supported.
> |  >
> |  > This does not comply with the current use of DriverFunc, namely in
> |  > xf86RandR.c. If the driver returns FALSE there, the function
> calling the
> |  > driver function will fail.
> |  >
> |  > So, either we should change the default behavior to return TRUE in the
> |  > driver func if the function opcode is not understood by the driver, or
> |  > change the existing code not to bail out on FALSE.
> |  >
> |
> | You definitely have a point there.
> | In the case of xf86RandRGetInfo() we definitely should return TRUE.
> | In the case of xf86RandRSetConfig() I'm not clear about the semantics
> | of the driver func.
> | Should this function do rotation and mode switching (we are passing
> | size information)?
> 
> 
> As far as I understand the code, it's only for rotation. Modeswitching
> is done separately.
> 
> | I would think currently:
> |
> |  if (randrp->rotation != rotation) {
> |
> |     /* Have the driver do its thing. */
> |         if (scrp->DriverFunc) {
> |             xorgRRRotation RRRotation;
> |             RRRotation.RRConfig.rotation = rotation;
> |             RRRotation.RRConfig.rate = rate;
> |             RRRotation.RRConfig.width = pSize->width;
> |             RRRotation.RRConfig.height = pSize->height;
> |
> |             /*
> |              * Currently we need to rely on HW support for rotation.
> |              */
> |             if (!(*scrp->DriverFunc)(scrp, RR_SET_CONFIG, &RRRotation))
> |                 return FALSE;
> |         }
> |         randrp->rotation = rotation;
> |     }
> |
> | would do as we don't support SW rotation any change in rotation would
> | have to be handled by the driver.
> 
> 
> I think so, too.
> 
> 
> | In any other case we'd need to have a SW fallback - however then we
> | may consider calling the DriverFunc() from inside xf86RandRSetMode ().
> 
> Well, currently the driver in RR_GET_CONFIG defines what rotations it
> supports. If the driver doesn't support rotation, the SetConfig function
> bails out with BadMatch anyway on any attempt to alter the rotation.
> Until there is a way to do software rotation, I think this is reasonable.
> 
> Thomas
> 
> - --
> Thomas Winischhofer
> Vienna/Austria
> thomas AT winischhofer DOT net	       *** http://www.winischhofer.net
> twini AT xfree86 DOT org
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.0 (GNU/Linux)
> 
> iD8DBQFCOBc1zydIRAktyUcRAmZYAKCF3ZXZ+sr5DA3DXr/Wo9kH3uyEywCg1KCN
> QUJenCdxJ0R5Wre0pK9JHS4=
> =ECNs
> -----END PGP SIGNATURE-----
> _______________________________________________
> xorg mailing list
> xorg at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xorg
>