weston: weston randr protocol for testing and configuration

[Pekka Paalanen]

Hi

On Fri, 14 Mar 2014 11:18:48 +0800
Quanxian Wang <quanxian.wang at intel.com> wrote:

> Objective:
> With discussion in mail list, currently we have an agreement. Randr interfaces will not be exposed public.
> The objective will be only for testing and configuration. Thanks Pq, Jason, Jasper, Hardening, and other's comment.
> 
> Before sending the email, the function list below is implementedi(tested) except new mode creating(not testing).
> 
> Thanks for your support.
> 
> Updates:
> 1) Change wl_randr to weston_randr for weston-specific.
> 2) Unique operation issue
> Given that one client has two more threads to do mode setting on the same output at the same time, how to identify what response (done event) is belong to one or another request when they want to get response? 
> 

Talking about threads here is misleading. Such threaded operations
do not make sense to begin with. The question is more about
ambiguity between operations and acknowledgements, and preferring
explicit correspondence than relying on protocol message ordering.
So the goal is good, but the rationale is slightly wrong.

> This is a design flaw in the first version of randr protocol.
> 
> The solution is to use the wayland/weston mechanism, every request will generate a resource which be used to send done event to the owner who start it. Owner could set the listener on that and keep tuning on the response event.
> 
> For example
> In client:
> randr_resource = wl_randr_set_transform(randr.randr,wayland_output,argument.transform);
> wl_randr_add_listener(randr_resource, &randr_transform_listener, &randr);
> 
> In compositor:
> randr_resource = wl_resource_create(client,&wl_randr_interface,1, action);
> ...
> wl_randr_send_action_done(randr_resource, 1<<WL_RANDR_ACTION_TRANSFORM, ret, action);
> wl_resource_destroy(randr_resource);

I do not want a reply for every single part of output state, I want
a reply for the final commit request that applies all the state
updates gathered so far. That will also solve the atomicity problem.

See how wl_surface works, and add a reply object only to the commit
request.

> 3) Mess up issue
> Will take randr protocol implementation as a module. This will keep compositor clear and avoid messing up in compositor.c. 
> 
> You can enable it from command line
> 'weston --tty=1 --enable-wrandr' or put wrandr.so in to module list in weston.ini.

This kind of "messing up" was never a problem. Making it a module
is a solution to the security concerns. Again, the right goal, but
not the right reason.

> 4) Add group randr operations
> After talking with Pq, in order to avoid glitches, group operation is needed. However, if operating on two outputs more at one time, it will bring more issues which could not control. In this randr design, will not provide group operation on multiple outputs. We provide atomic operation on one output, multi outputs operation logic are left to designer/developers. Group operation on one output will be designed. For example, you can set mode, scale, and transform at one time with one randr request.
> 

This seems to contradict your example above about the reply objects.

> 5) Mode setting parameters control
> Output name will be under the control, for mode setting, from suggestion of Hardening and Jasper St. Pierre, it'd better to let user new mode. Defaultly I agree that. If someone has other suggestion and reason, please let me know.
> 

Sorry, what?

If this means you will support custom modes, that's good. It's not
in your proposal below, though.

> 6) Disconnected outputs
> Currently I have disable that. But basically, when user query output information, showing connected and disconnected output as a whole will be fine for user and QA people. Sometime, QA people or user like that information. It will be helpful for them to identify how many outputs are provided by their machine. Which is connected and which is not connected.
> So when user has requirement, that is fine. I could enable it.
> 
> 7) wl_output property name event
> I have sent patches to enable it. Not only for randr, for any client of wayland, it will be helpful to get a sensible name instead of only wl_output object.
> 
> 8) adding set_scale request
> Mode, scale, transform are the basic properties of output, I will add them as a whole.
> 
> Original information:
> 
> The idea is from xrandr provided by xserver. *Dynamic* mode
> setting is the main objective of this protocol. Remember,
> it is one shot operation. For example, if setting the mode,
> just call one request wl_randr_set_mode. If you want to make
> sure if it is successful, you need keep tuning done event.
> after all, it is a protocol communication.
> 
> With this protocol, weston-wrandr application is developped to help 
> implement randr protocol in command line just like xrandr command 
> in xserver.
> 
> Weston protocol wrandr will provide interface to 
> 1) set output mode, scale, transform
> 2) move output to relative position
> 
> Here are some use cases.
> 
> 1. weston-randr -q # query all output mode info and disconnected output
> 
> HDMI3
> 1)1440x900 at 60 (current)
> 2)1920x1200 at 60
> 3)1680x1050 at 60
> ...
> 
> VGA1
> 1)1280x1024 at 60 (current)
> 2)1152x864 at 60
> 3)1024x768 at 60
> ...
> 
> 2. weston-randr --output HDMI3 # query HDMI3 output mode info
> 
> HDMI3
> 1)1440x900 at 60 (current)
> 2)1920x1200 at 60
> 3)1680x1050 at 60
> 
> 3. weston-randr --output HDMI3 --mode 2 # which will set mode as 1920x1200
> or weston-randr --output HDMI3 --mode 1920x1200 at 60
> or weston-randr --output HDMI3 --mode 1920x1200
> 
> 4. weston-randr --output HDMI3 --transform 1 # rotate HDMI3 output 90 degree
> 
> 5. weston-randr --output HDMI3 --leftof VGA1 # put HDMI3 output leftof VGA1
> 
> Group operations example:
> 5. weston-randr --output HDMI3 --transform 3 --scale 2 --mode 2 -leftof VGA1

Nice.

> 
> Signed-off-by: Quanxian Wang <quanxian.wang at intel.com>
> ---
>  protocol/randr.xml   | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 166 insertions(+)
>  create mode 100644 protocol/randr.xml
> 
> diff --git a/protocol/randr.xml b/protocol/randr.xml
> new file mode 100644
> index 0000000..5c1d2d2
> --- /dev/null
> +++ b/protocol/randr.xml
> @@ -0,0 +1,166 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="randr">
> +
> +  <copyright>
> +    Copyright © 2014 Quanxian Wang
> +    Copyright © 2014 Intel Corporation
> +
> +    Permission to use, copy, modify, distribute, and sell this
> +    software and its documentation for any purpose is hereby granted
> +    without fee, provided that the above copyright notice appear in
> +    all copies and that both that copyright notice and this permission
> +    notice appear in supporting documentation, and that the name of
> +    the copyright holders not be used in advertising or publicity
> +    pertaining to distribution of the software without specific,
> +    written prior permission.  The copyright holders make no
> +    representations about the suitability of this software for any
> +    purpose.  It is provided "as is" without express or implied
> +    warranty.
> +
> +    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> +    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> +    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> +    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> +    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> +    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
> +    THIS SOFTWARE.
> +  </copyright>
> +
> +  <interface name="weston_randr" version="1">
> +    <description summary="randr">
> +      The global interface exposing randr capabilities.
> +      As a weston_randr, that provides the interfaces for apps to more operations
> +      on output.
> +
> +      The aim of weston_randr is to get modes list, choose preferred mode,
> +      layout the output including position, rotate, and en/disable.
> +      The idea is from xrandr protocoal of xserver. It is very convenient for
> +      weston/wayland user to operates on mode setting of output.
> +    </description>
> +
> +    <enum name="error">
> +      <entry name="bad_randr" value="0"
> +             summary="the to-be weston_randr is invalid"/>

When does this error happen?

While I cannot understand what "bad_randr" could mean, I see no
definition on what happens if a client sends illegal values, like
scale -400.

> +    </enum>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="unbind from the weston_randr interface">
> +	Informs the server that the client will not be using this
> +	protocol object anymore. This does not affect any other
> +	objects, weston_randr objects included.
> +      </description>
> +    </request>
> +
> +    <request name="set_mode">
> +      <description summary="set the mode of output">
> +	Set the mode of output.
> +      </description>
> +      <arg name="output" type="object" interface="wl_output"
> +           summary="the output object"/>
> +      <arg name="width" type="int" summary="width of the mode in hardware units"/>
> +      <arg name="height" type="int" summary="height of the mode in hardware units"/>
> +      <arg name="refresh" type="int" summary="vertical refresh rate in mHz"/>

So this is the simple mode set request.

Do you think width/height/refresh is really enough to identify a
mode? I don't think so. I think in the early days of X11 RandR,
NVidia hit the same problem, and started to expose fake refresh
values, only to be able to distinguish modes that were identical in
width/height/refresh but still different in timings. Or actually I
think it was much more complicated than that, but this is the point
in simple terms.

So we need something else here to identify a mode.

Check what kind of protocol GNOME uses, and how current RandR
protocol works.

> +      <arg name="randr_mode" type="new_id" interface="weston_randr"
> +           summary="weston_randr resource handle"/>

See earlier on what I said about the reply objects and committing.

weston_randr is your global interface, it makes no sense for its
requests to create more weston_randr objects. You should not use
weston_randr here as the object type to be created, but another
interface written only for the reply.

> +    </request>
> +
> +    <request name="set_transform">
> +      <description summary="set the transform of output">
> +	Set the transform of output, the valuable value should
> +	be 0-7. This means 0, 90, 180, 270, FLIP-0, FLIP-90,
> +	FLIP-180, FLIP-270.

Just refer to the wl_output specification where these come from,
right? These values are not invented here, they specified
elsewhere, so say what specifies them.

> +      </description>
> +      <arg name="output" type="object" interface="wl_output"
> +           summary="the output object"/>
> +      <arg name="transform" type="uint"
> +           summary="the value should be between 0-7"/>
> +      <arg name="randr_transform" type="new_id" interface="weston_randr"
> +           summary="weston_randr resource handle"/>
> +    </request>
> +
> +    <request name="set_scale">
> +      <description summary="set the scale of output">
> +	Set the scale of output.
> +      </description>
> +      <arg name="output" type="object" interface="wl_output"
> +           summary="the output object"/>
> +      <arg name="scale" type="int"
> +           summary="Scale the output"/>
> +      <arg name="randr_scale" type="new_id" interface="weston_randr"
> +           summary="weston_randr resource handle"/>
> +    </request>
> +
> +    <enum name="move">
> +      <description summary="move target output to the position of source output">
> +	The purpose is mainly to allow clients move target output to
> +        the position of source output.
> +      </description>
> +      <entry name="rightof" value="0"/>
> +      <entry name="leftof" value="1"/>

What about above/below?

> +    </enum>
> +
> +    <request name="move">
> +      <description summary="move target output">
> +	Move the target output to be leftof/rightof source output.
> +      </description>
> +      <arg name="target_output" type="object" interface="wl_output"
> +           summary="the target output object"/>
> +      <arg name="source_output" type="object" interface="wl_output"
> +           summary="the source output object"/>
> +      <arg name="move" type="int"/>
> +      <arg name="randr_move" type="new_id" interface="weston_randr"
> +           summary="weston_randr resource handle"/>
> +    </request>
> +
> +    <request name="operations">
> +      <description summary="multiple operations on one output">
> +	Do multiple operations on target output. This allows uer
> +	to do mode, scale , transform, move at the same time.
> +	It will stop glitches for multiple actions
> +	one by one when mode setting.
> +      </description>
> +      <arg name="target_output" type="object" interface="wl_output"
> +           summary="the target output object"/>
> +      <arg name="left" type="object" interface="wl_output" allow-null="true"/>
> +      <arg name="right" type="object" interface="wl_output" allow-null="true"/>
> +      <arg name="width" type="int"/>
> +      <arg name="height" type="int"/>
> +      <arg name="refresh" type="int"/>
> +      <arg name="transform" type="int"
> +           summary="The value should be between 0-7"/>
> +      <arg name="scale" type="int"
> +           summary="Scale the output"/>
> +      <arg name="randr_operations" type="new_id" interface="weston_randr"
> +           summary="weston_randr resource handle"/>

No, no, not like this. Look at how wl_surface collects several
pieces of state, and then applies them all in one go.

Fixing this, all the reply object stuff, and supporting atomic
setting over multiple outputs will actually make your protocol much
simpler, and at the same time more powerful and extensible.

Extendability is important, we may get more output properties
later, just like scale was added after wl_output interface was
already stable.

> +    </request>
> +
> +    <enum name="status">
> +      <description summary="flags">
> +	These flags show the status of every action excuted by randr.
> +      </description>
> +      <entry name="mode" value="0"/>
> +      <entry name="move" value="4"/>
> +      <entry name="transform" value="8"/>
> +      <entry name="scale" value="12"/>
> +    </enum>

It is unclear how this would work or what it means.

> +
> +    <enum name="result">
> +      <description summary="action result">
> +	Action result
> +      </description>
> +      <entry name="SUCCESS" value="1"/>
> +      <entry name="FAIL" value="2"/>
> +      <entry name="NOACT" value="3"/>
> +    </enum>
> +
> +    <event name="action_done">
> +      <description summary="action done event">
> +	Notify the randr client that mode setting has been done.
> +      </description>
> +      <arg name="flags" type="uint"
> +           summary="flag action and result"/>
> +      <arg name="action_id" type="uint"/>

This is the old kind of stuff you had, which I thought you replaced
with a reply object interface. But you didn't define any reply
object interface anywhere yet.

The reply interface could be something like this:

<interface name="weston_randr_update" version="1">
  <event name="done">
    <arg name="status" type="uint"/>
  </event>
</interface>

Of course you need to add all the details about object destruction,
etc.

That is very similar to wl_callback, except this one is specific to
weston_randr and you can extend it if needed, and define what the
status codes are: success/fail; maybe add some reason-why-failed if
it makes sense...

Or instead of one "done" event there could be events "success" and
"failure", where only failure would return the extra info.

> +    </event>
> +  </interface>
> +</protocol>
> -- 
> 1.8.1.2
> 


Thanks,
pq