Re: Re: PVFB wheel events (z-axis)

[Pat Campbell <plc@novell.com>]

Markus Armbruster wrote:
> Pat Campbell <plc@novell.com> writes:
>
>   
>> Markus Armbruster wrote:
>>     
>>> I got questions on this changeset:
>>>
>>>     changeset:   354:c3ff0b26f664
>>>     date:        Mon Dec 10 13:52:47 2007 +0000
>>>
>>>     Decode mouse event packet dz value and passes it as a wheel event into
>>>     the input stream.
>>>
>>>     Signed-off-by: Pat Campbell <plc@novell.com>
>>>
>>>     diff -r 7232a025140f -r c3ff0b26f664 drivers/xen/fbfront/xenkbd.c
>>>     --- a/drivers/xen/fbfront/xenkbd.c	Mon Dec 10 13:51:57 2007 +0000
>>>     +++ b/drivers/xen/fbfront/xenkbd.c	Mon Dec 10 13:52:47 2007 +0000
>>>     @@ -64,8 +64,13 @@ static irqreturn_t input_handler(int rq,
>>>                     dev = info->ptr;
>>>                     switch (event->type) {
>>>                     case XENKBD_TYPE_MOTION:
>>>     -			input_report_rel(dev, REL_X, event->motion.rel_x);
>>>     -			input_report_rel(dev, REL_Y, event->motion.rel_y);
>>>     +			if ( event->motion.rel_z == 1 || event->motion.rel_z == -1 ) {
>>>     +				input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z);
>>>     +			}           
>>>     +			else {
>>>     +				input_report_rel(dev, REL_X, event->motion.rel_x);
>>>     +				input_report_rel(dev, REL_Y, event->motion.rel_y);
>>>     +			}
>>>
>>> I don't understand the conditional.  Why is rel_z to be used *only*
>>> when it's 1 or -1, and why are rel_x and rel_y to be used *only* when
>>> rel_z isn't?  That sure is a weird protocol, and it isn't documented
>>> anywhere...
>>>   
>>>       
>> In my testing I never saw a case where the rel_x and rel_y were not zero
>> or the abs_x and abs_y changed when a  z event came thru.   A small
>> attempt to not flood the input stream with redundant data. 
>>     
>
> Assuming conditions you observed in your tests will hold elsehwere in
> space or time is calling for trouble :)
>
>   
>> Probably for clarity should have been:  (same for the abs case)
>>    if (event->motion.rel_z != 0)
>>        input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z);
>>    input_report_rel(dev, REL_X, event->motion.rel_x);
>>    input_report_rel(dev, REL_Y, event->motion.rel_y);
>>     
>
> Why use REL_WHEEL and not REL_Z?
>   
Same answer as below, Xorg 6.9 does not decode REL_Z.  Xorg.7.3 handles
REL_Z and REL_WHEEL as the same.
> Why suppress zero Z-axis motion, but not X/Y-axis?
>
>   
Hmm.   Delving back into X
   Xorg 6.9
          case REL_WHEEL:
                if (value > 0)
                    PostButtonClicks(pInfo, wheel_up_button, value);
                if (value < 0)
                    PostButtonClicks(pInfo, wheel_down_button, -value);
                break;
    Xorg 7.3
              Just sends the value up, gets ignored later

I guess I saw that sending a '0' for REL_WHEEL was a waste of time and
decided to filtered it out.   Also not really necessary if it makes the
code easier to understand.  

>>>                             break;
>>>                     case XENKBD_TYPE_KEY:
>>>                             dev = NULL;
>>>     @@ -81,8 +86,13 @@ static irqreturn_t input_handler(int rq,
>>>                                            event->key.keycode);
>>>                             break;
>>>                     case XENKBD_TYPE_POS:
>>>     -			input_report_abs(dev, ABS_X, event->pos.abs_x);
>>>     -			input_report_abs(dev, ABS_Y, event->pos.abs_y);
>>>     +			if ( event->pos.abs_z == 1 || event->pos.abs_z == -1 ) {
>>>     +				input_report_rel(dev, REL_WHEEL, 0 - event->pos.abs_z);
>>>     +			}
>>>     +			else {
>>>     +				input_report_abs(dev, ABS_X, event->pos.abs_x);
>>>     +				input_report_abs(dev, ABS_Y, event->pos.abs_y);
>>>     +			}
>>>
>>> And why isn't this using REL_ABS?
>>>   
>>>       
>> I assume you meant to ask why not ABS_WHEEL.  Xorg 6.9 evdev driver does
>>     
>
> Yes.
>
>   
>> not decode ABS_WHEEL.  Xorg 7.3 decodes  both REL and ABS WHEEL but
>> ABS_WHEEL requires extra xorg.conf input options.   We get greater
>> coverage by using REL_WHEEL and reduce the need to edit  xorg.conf.
>>     
>
> Okay, that's fair.
>   
> [...]
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>