| Message ID | 20230413085635.2515647-1-zhouzongmin@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | Input: vmmouse - add macros to enable vmmouse relative mode | expand |
On Thu, 2023-04-13 at 16:56 +0800, Zongmin Zhou wrote: > Add macros to enable request relative mode. > > Change the REL_Y value passed by input_report_rel function, > to match the direction of mouse movement. Thanks for the patch, but in its current form it's a nack. First of all we don't want any defines in the driver code that affect compilation, it's never going to be tested or compiled in. Either a kconfig or a module parameter would be acceptable but that's only if you can actually explain what it is that you're fixing. The current single line description just mentions what the effect it has (not completely correctly either because for merged packets absolute will still be x=x, y=y, but relative will be x += dx, y -= dy) but not why it's done, what it's fixing and how to reproduce. z
On Fri, 2023-04-14 at 01:22 +0000, Zack Rusin wrote: > On Thu, 2023-04-13 at 16:56 +0800, Zongmin Zhou wrote: > > Add macros to enable request relative mode. > > > > Change the REL_Y value passed by input_report_rel function, > > to match the direction of mouse movement. > > Thanks for the patch, but in its current form it's a nack. First of > all we don't > want any defines in the driver code that affect compilation, it's > never going to be > tested or compiled in. Either a kconfig or a module parameter would > be acceptable > but that's only if you can actually explain what it is that you're > fixing. The > current single line description just mentions what the effect it has > (not completely > correctly either because for merged packets absolute will still be > x=x, y=y, but > relative will be x += dx, y -= dy) but not why it's done, what it's > fixing and how > to reproduce. > > z > Dear zack: Firstly,thanks for your reply. The reason I want to add macros to request different vmmouse modes(relative or absolute) is that the vmmouse drivers currently only supports request absolute mode.But in some case we want request relative mode so that Pointer acceleration feature can be used.(as I know,libinput module only support Pointer acceleration feature in relative mode.) So I think we can provide two vmmouse modes to facilitate the use of different needs. If need,I can change it to a kconfig or a module parameter. The reasons of fix for REL_Y value，are as follows: When I request relative vmmouse mode,and let mouse move up,the mouse pointer moved down instead. Similarly, when I move the mouse down, the mouse pointer moved up. it obviously with a wrong motion direction in y. Actually，I understand that the value of y here is the end calculation result of relative coordinate movement，the real calculation is in motion_event() of spice-gtk and legacy_mouse_event() of qemu. Test scenario: 1) start virtual machine with qemu command "vmport=on"，also with spice protocal. 2) modify guest vmmouse drivers to request relative mode. 3) move the mouse,will observe the pointer freezed,it's because driver not match the condition 'status & VMMOUSE_RELATIVE_PACKET',can't find correct input device.need merge this patch in qemu: https://lore.kernel.org/all/20230413081526.2229916-1-zhouzongmin@kylinos.cn/ 4) after merge the patch in qemu,we can observe the issue of wrong motion direction in y. Looking forward to your reply.
On Fri, 2023-04-14 at 16:58 +0800, zongmin zhou wrote: > On Fri, 2023-04-14 at 01:22 +0000, Zack Rusin wrote: > > On Thu, 2023-04-13 at 16:56 +0800, Zongmin Zhou wrote: > > > Add macros to enable request relative mode. > > > > > > Change the REL_Y value passed by input_report_rel function, > > > to match the direction of mouse movement. > > > > Thanks for the patch, but in its current form it's a nack. First of > > all we don't > > want any defines in the driver code that affect compilation, it's > > never going to be > > tested or compiled in. Either a kconfig or a module parameter would > > be acceptable > > but that's only if you can actually explain what it is that you're > > fixing. The > > current single line description just mentions what the effect it has > > (not completely > > correctly either because for merged packets absolute will still be > > x=x, y=y, but > > relative will be x += dx, y -= dy) but not why it's done, what it's > > fixing and how > > to reproduce. > > > > z > > > Dear zack: > > Firstly,thanks for your reply. > > The reason I want to add macros to request different vmmouse > modes(relative or absolute) is that the vmmouse drivers currently only > supports request absolute mode.But in some case we want request > relative mode so that Pointer acceleration feature can be used.(as I > know,libinput module only support Pointer acceleration feature in > relative mode.) > So I think we can provide two vmmouse modes to facilitate the use of > different needs. > If need,I can change it to a kconfig or a module parameter. > > The reasons of fix for REL_Y value，are as follows: > When I request relative vmmouse mode,and let mouse move up,the mouse > pointer moved down instead. > Similarly, when I move the mouse down, the mouse pointer moved up. > it obviously with a wrong motion direction in y. > > Actually，I understand that the value of y here is the end calculation > result of relative coordinate movement，the real calculation is in > motion_event() of spice-gtk and legacy_mouse_event() of qemu. > > Test scenario: > 1) start virtual machine with qemu command "vmport=on"，also with spice > protocal. > 2) modify guest vmmouse drivers to request relative mode. > 3) move the mouse,will observe the pointer freezed,it's because driver > not match the condition 'status & VMMOUSE_RELATIVE_PACKET',can't find > correct input device.need merge this patch in qemu: > https://lore.kernel.org/all/20230413081526.2229916-1-zhouzongmin@kylinos.cn/ > 4) after merge the patch in qemu,we can observe the issue of wrong > motion direction in y. Sounds like you have a bug in the monitor code to me. The mouse might request relative mode, but that doesn't mean that it's going to be switched, it's a hint. On enable we definitely want to request the default absolute mode. Ultimately it's the delivery mechanism (i.e. whether the delivered event is VMMOUSE_RELATIVE_PACKET or not) that defines what an event is. This sounds to me like the monitor delivers VMMOUSE_RELATIVE_PACKET events but doesn't mark them as such. You can confirm by putting some debugging output in the vmmouse_report_events. z
diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c index ea9eff7c8099..f39ce21f7af9 100644 --- a/drivers/input/mouse/vmmouse.c +++ b/drivers/input/mouse/vmmouse.c @@ -21,6 +21,12 @@ #include "psmouse.h" #include "vmmouse.h" +/* + * Enable this to request relative mode. + * Defaut to absolute mode. + */ +//#define VMMOUSE_RELATIVE_MODE + #define VMMOUSE_PROTO_MAGIC 0x564D5868U /* @@ -184,7 +190,7 @@ static psmouse_ret_t vmmouse_report_events(struct psmouse *psmouse) if (status & VMMOUSE_RELATIVE_PACKET) { pref_dev = rel_dev; input_report_rel(rel_dev, REL_X, (s32)x); - input_report_rel(rel_dev, REL_Y, -(s32)y); + input_report_rel(rel_dev, REL_Y, (s32)y); } else { pref_dev = abs_dev; input_report_abs(abs_dev, ABS_X, x); @@ -304,8 +310,13 @@ static int vmmouse_enable(struct psmouse *psmouse) VMMOUSE_CMD(ABSPOINTER_RESTRICT, VMMOUSE_RESTRICT_CPL0, dummy1, dummy2, dummy3, dummy4); +#ifdef VMMOUSE_RELATIVE_MODE + VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_REQUEST_RELATIVE, + dummy1, dummy2, dummy3, dummy4); +#else VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_REQUEST_ABSOLUTE, dummy1, dummy2, dummy3, dummy4); +#endif return 0; }
Add macros to enable request relative mode. Change the REL_Y value passed by input_report_rel function, to match the direction of mouse movement. Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn> --- drivers/input/mouse/vmmouse.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)