[PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Juergen Gross]

Add a parameter for setting the resolution of xen-kbdfront in order to
be able to cope with a (virtual) frame buffer of arbitrary resolution.

While at it remove the pointless second reading of parameters from
Xenstore in the device connection phase: all parameters are available
during device probing already and that is where they should be read.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3: - merged the two patches
    - read Xenstore parameters during probing of the device only
---
 drivers/input/misc/xen-kbdfront.c | 39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index 3900875dec10..2fc7895373ab 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -41,6 +41,12 @@ struct xenkbd_info {
 	char phys[32];
 };
 
+enum { KPARAM_X, KPARAM_Y, KPARAM_CNT };
+static int ptr_size[KPARAM_CNT] = { XENFB_WIDTH, XENFB_HEIGHT };
+module_param_array(ptr_size, int, NULL, 0444);
+MODULE_PARM_DESC(ptr_size,
+	"Pointing device width, height in pixels (default 800,600)");
+
 static int xenkbd_remove(struct xenbus_device *);
 static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info *);
 static void xenkbd_disconnect_backend(struct xenkbd_info *);
@@ -128,7 +134,12 @@ static int xenkbd_probe(struct xenbus_device *dev,
 	if (!info->page)
 		goto error_nomem;
 
+	/* Set input abs params to match backend screen res */
 	abs = xenbus_read_unsigned(dev->otherend, "feature-abs-pointer", 0);
+	ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend, "width",
+						  ptr_size[KPARAM_X]);
+	ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend, "height",
+						  ptr_size[KPARAM_Y]);
 	if (abs) {
 		ret = xenbus_write(XBT_NIL, dev->nodename,
 				   "request-abs-pointer", "1");
@@ -174,8 +185,8 @@ static int xenkbd_probe(struct xenbus_device *dev,
 
 	if (abs) {
 		__set_bit(EV_ABS, ptr->evbit);
-		input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
-		input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
+		input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
+		input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
 	} else {
 		input_set_capability(ptr, EV_REL, REL_X);
 		input_set_capability(ptr, EV_REL, REL_Y);
@@ -309,9 +320,6 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *info)
 static void xenkbd_backend_changed(struct xenbus_device *dev,
 				   enum xenbus_state backend_state)
 {
-	struct xenkbd_info *info = dev_get_drvdata(&dev->dev);
-	int ret, val;
-
 	switch (backend_state) {
 	case XenbusStateInitialising:
 	case XenbusStateInitialised:
@@ -321,15 +329,6 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateInitWait:
-InitWait:
-		if (xenbus_read_unsigned(info->xbdev->otherend,
-					 "feature-abs-pointer", 0)) {
-			ret = xenbus_write(XBT_NIL, info->xbdev->nodename,
-					   "request-abs-pointer", "1");
-			if (ret)
-				pr_warning("xenkbd: can't request abs-pointer");
-		}
-
 		xenbus_switch_state(dev, XenbusStateConnected);
 		break;
 
@@ -340,17 +339,7 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
 		 * get Connected twice here.
 		 */
 		if (dev->state != XenbusStateConnected)
-			goto InitWait; /* no InitWait seen yet, fudge it */
-
-		/* Set input abs params to match backend screen res */
-		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-				 "width", "%d", &val) > 0)
-			input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0);
-
-		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-				 "height", "%d", &val) > 0)
-			input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
-
+			xenbus_switch_state(dev, XenbusStateConnected);
 		break;
 
 	case XenbusStateClosed:
-- 
2.12.0

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Oleksandr Andrushchenko]

Hi, Juergen!


On 04/11/2017 03:30 PM, Juergen Gross wrote:
> Add a parameter for setting the resolution of xen-kbdfront in order to
> be able to cope with a (virtual) frame buffer of arbitrary resolution.
>
> While at it remove the pointless second reading of parameters from
> Xenstore in the device connection phase: all parameters are available
> during device probing already and that is where they should be read.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3: - merged the two patches
>      - read Xenstore parameters during probing of the device only
> ---
>   drivers/input/misc/xen-kbdfront.c | 39 ++++++++++++++-------------------------
>   1 file changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> index 3900875dec10..2fc7895373ab 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -41,6 +41,12 @@ struct xenkbd_info {
>   	char phys[32];
>   };
>   
> +enum { KPARAM_X, KPARAM_Y, KPARAM_CNT };
> +static int ptr_size[KPARAM_CNT] = { XENFB_WIDTH, XENFB_HEIGHT };
> +module_param_array(ptr_size, int, NULL, 0444);
> +MODULE_PARM_DESC(ptr_size,
> +	"Pointing device width, height in pixels (default 800,600)");
> +
>   static int xenkbd_remove(struct xenbus_device *);
>   static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info *);
>   static void xenkbd_disconnect_backend(struct xenkbd_info *);
> @@ -128,7 +134,12 @@ static int xenkbd_probe(struct xenbus_device *dev,
>   	if (!info->page)
>   		goto error_nomem;
>   
> +	/* Set input abs params to match backend screen res */
>   	abs = xenbus_read_unsigned(dev->otherend, "feature-abs-pointer", 0);
> +	ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend, "width",
> +						  ptr_size[KPARAM_X]);
> +	ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend, "height",
> +						  ptr_size[KPARAM_Y]);
>   	if (abs) {
>   		ret = xenbus_write(XBT_NIL, dev->nodename,
>   				   "request-abs-pointer", "1");
> @@ -174,8 +185,8 @@ static int xenkbd_probe(struct xenbus_device *dev,
>   
>   	if (abs) {
>   		__set_bit(EV_ABS, ptr->evbit);
> -		input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
> -		input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
> +		input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
> +		input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
>   	} else {
>   		input_set_capability(ptr, EV_REL, REL_X);
>   		input_set_capability(ptr, EV_REL, REL_Y);
> @@ -309,9 +320,6 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *info)
>   static void xenkbd_backend_changed(struct xenbus_device *dev,
>   				   enum xenbus_state backend_state)
>   {
> -	struct xenkbd_info *info = dev_get_drvdata(&dev->dev);
> -	int ret, val;
> -
>   	switch (backend_state) {
>   	case XenbusStateInitialising:
>   	case XenbusStateInitialised:
> @@ -321,15 +329,6 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
>   		break;
>   
>   	case XenbusStateInitWait:
> -InitWait:
> -		if (xenbus_read_unsigned(info->xbdev->otherend,
> -					 "feature-abs-pointer", 0)) {
> -			ret = xenbus_write(XBT_NIL, info->xbdev->nodename,
> -					   "request-abs-pointer", "1");
> -			if (ret)
> -				pr_warning("xenkbd: can't request abs-pointer");
> -		}
> -
>   		xenbus_switch_state(dev, XenbusStateConnected);
>   		break;
>   
> @@ -340,17 +339,7 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
>   		 * get Connected twice here.
>   		 */
>   		if (dev->state != XenbusStateConnected)
> -			goto InitWait; /* no InitWait seen yet, fudge it */
> -
> -		/* Set input abs params to match backend screen res */
> -		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
> -				 "width", "%d", &val) > 0)
> -			input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0);
> -
> -		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
> -				 "height", "%d", &val) > 0)
> -			input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
> -
> +			xenbus_switch_state(dev, XenbusStateConnected);
>   		break;
>   
>   	case XenbusStateClosed:
I have tested this patch and here we go
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

I also tested this with my patches for multi-touch support and almost
ready to push those as well. With this respect, I have a question on
how this can better be done: my patches depend on your patch + they depend
on new kbdif.h queued for Linux 4.12.

What do you guys think would be the best approach for upstreaming
multi-touch then?

Thank you,
Oleksandr

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Juergen Gross]

On 12/04/17 16:13, Oleksandr Andrushchenko wrote:
> Hi, Juergen!
> 
> 
> On 04/11/2017 03:30 PM, Juergen Gross wrote:
>> Add a parameter for setting the resolution of xen-kbdfront in order to
>> be able to cope with a (virtual) frame buffer of arbitrary resolution.
>>
>> While at it remove the pointless second reading of parameters from
>> Xenstore in the device connection phase: all parameters are available
>> during device probing already and that is where they should be read.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3: - merged the two patches
>>      - read Xenstore parameters during probing of the device only
>> ---
>>   drivers/input/misc/xen-kbdfront.c | 39
>> ++++++++++++++-------------------------
>>   1 file changed, 14 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/input/misc/xen-kbdfront.c
>> b/drivers/input/misc/xen-kbdfront.c
>> index 3900875dec10..2fc7895373ab 100644
>> --- a/drivers/input/misc/xen-kbdfront.c
>> +++ b/drivers/input/misc/xen-kbdfront.c
>> @@ -41,6 +41,12 @@ struct xenkbd_info {
>>       char phys[32];
>>   };
>>   +enum { KPARAM_X, KPARAM_Y, KPARAM_CNT };
>> +static int ptr_size[KPARAM_CNT] = { XENFB_WIDTH, XENFB_HEIGHT };
>> +module_param_array(ptr_size, int, NULL, 0444);
>> +MODULE_PARM_DESC(ptr_size,
>> +    "Pointing device width, height in pixels (default 800,600)");
>> +
>>   static int xenkbd_remove(struct xenbus_device *);
>>   static int xenkbd_connect_backend(struct xenbus_device *, struct
>> xenkbd_info *);
>>   static void xenkbd_disconnect_backend(struct xenkbd_info *);
>> @@ -128,7 +134,12 @@ static int xenkbd_probe(struct xenbus_device *dev,
>>       if (!info->page)
>>           goto error_nomem;
>>   +    /* Set input abs params to match backend screen res */
>>       abs = xenbus_read_unsigned(dev->otherend, "feature-abs-pointer",
>> 0);
>> +    ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend, "width",
>> +                          ptr_size[KPARAM_X]);
>> +    ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend, "height",
>> +                          ptr_size[KPARAM_Y]);
>>       if (abs) {
>>           ret = xenbus_write(XBT_NIL, dev->nodename,
>>                      "request-abs-pointer", "1");
>> @@ -174,8 +185,8 @@ static int xenkbd_probe(struct xenbus_device *dev,
>>         if (abs) {
>>           __set_bit(EV_ABS, ptr->evbit);
>> -        input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
>> -        input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
>> +        input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
>> +        input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
>>       } else {
>>           input_set_capability(ptr, EV_REL, REL_X);
>>           input_set_capability(ptr, EV_REL, REL_Y);
>> @@ -309,9 +320,6 @@ static void xenkbd_disconnect_backend(struct
>> xenkbd_info *info)
>>   static void xenkbd_backend_changed(struct xenbus_device *dev,
>>                      enum xenbus_state backend_state)
>>   {
>> -    struct xenkbd_info *info = dev_get_drvdata(&dev->dev);
>> -    int ret, val;
>> -
>>       switch (backend_state) {
>>       case XenbusStateInitialising:
>>       case XenbusStateInitialised:
>> @@ -321,15 +329,6 @@ static void xenkbd_backend_changed(struct
>> xenbus_device *dev,
>>           break;
>>         case XenbusStateInitWait:
>> -InitWait:
>> -        if (xenbus_read_unsigned(info->xbdev->otherend,
>> -                     "feature-abs-pointer", 0)) {
>> -            ret = xenbus_write(XBT_NIL, info->xbdev->nodename,
>> -                       "request-abs-pointer", "1");
>> -            if (ret)
>> -                pr_warning("xenkbd: can't request abs-pointer");
>> -        }
>> -
>>           xenbus_switch_state(dev, XenbusStateConnected);
>>           break;
>>   @@ -340,17 +339,7 @@ static void xenkbd_backend_changed(struct
>> xenbus_device *dev,
>>            * get Connected twice here.
>>            */
>>           if (dev->state != XenbusStateConnected)
>> -            goto InitWait; /* no InitWait seen yet, fudge it */
>> -
>> -        /* Set input abs params to match backend screen res */
>> -        if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
>> -                 "width", "%d", &val) > 0)
>> -            input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0);
>> -
>> -        if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
>> -                 "height", "%d", &val) > 0)
>> -            input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
>> -
>> +            xenbus_switch_state(dev, XenbusStateConnected);
>>           break;
>>         case XenbusStateClosed:
> I have tested this patch and here we go
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Thanks.

> 
> I also tested this with my patches for multi-touch support and almost
> ready to push those as well. With this respect, I have a question on
> how this can better be done: my patches depend on your patch + they depend
> on new kbdif.h queued for Linux 4.12.
> 
> What do you guys think would be the best approach for upstreaming
> multi-touch then?

Mention the dependency in the commit message below a marker line
containing only '---' (without the "'"). Probably there will be at
least a few review rounds so the patches you are depending on have
a chance to make it into the kernel until your patches are applied.
In case your code is perfect from the beginning we can coordinate
the pull requests to be in correct sequence. :-)


Juergen

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Dmitry Torokhov]

Hi Juergen,

On Tue, Apr 11, 2017 at 02:30:37PM +0200, Juergen Gross wrote:
> Add a parameter for setting the resolution of xen-kbdfront in order to
> be able to cope with a (virtual) frame buffer of arbitrary resolution.
> 
> While at it remove the pointless second reading of parameters from
> Xenstore in the device connection phase: all parameters are available
> during device probing already and that is where they should be read.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3: - merged the two patches
>     - read Xenstore parameters during probing of the device only

I guess 2 module parameters are not big deal, but could you tell me why
you can't always have them specified in Xenstore?

Also, I still think you are going in the wrong direction here. Can your
framebuffer size change after booting the guest? If it can, you have to
reconcile the new size and the coordinates reported by the pointing
device, and I think guest should be doing it. If you look, for example,
at vmmouse driver, they do not try to match coordinates it reports to
the screen.

Thanks.

-- 
Dmitry

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Juergen Gross]

On 12/04/17 17:16, Dmitry Torokhov wrote:
> Hi Juergen,
> 
> On Tue, Apr 11, 2017 at 02:30:37PM +0200, Juergen Gross wrote:
>> Add a parameter for setting the resolution of xen-kbdfront in order to
>> be able to cope with a (virtual) frame buffer of arbitrary resolution.
>>
>> While at it remove the pointless second reading of parameters from
>> Xenstore in the device connection phase: all parameters are available
>> during device probing already and that is where they should be read.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3: - merged the two patches
>>     - read Xenstore parameters during probing of the device only
> 
> I guess 2 module parameters are not big deal, but could you tell me why
> you can't always have them specified in Xenstore?

This will depend on Xen version then. I'd prefer to have a way to
specify the resolution in case a new kernel is running as a guest
of an old Xen version.

> Also, I still think you are going in the wrong direction here. Can your
> framebuffer size change after booting the guest? If it can, you have to
> reconcile the new size and the coordinates reported by the pointing
> device, and I think guest should be doing it. If you look, for example,
> at vmmouse driver, they do not try to match coordinates it reports to
> the screen.

I'm no expert in input driver interface. Can you tell me how the mouse
pointer is positioned in case of the vmmouse driver? Are the real limits
used just the minimum of pointing device and screen size limits? So
specifying a size of 0xffff * 0xffff like the vmmouse driver does will
work with any screen size being smaller than that?


Juergen

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Dmitry Torokhov]

On Wed, Apr 12, 2017 at 06:04:30PM +0200, Juergen Gross wrote:
> On 12/04/17 17:16, Dmitry Torokhov wrote:
> > Hi Juergen,
> > 
> > On Tue, Apr 11, 2017 at 02:30:37PM +0200, Juergen Gross wrote:
> >> Add a parameter for setting the resolution of xen-kbdfront in order to
> >> be able to cope with a (virtual) frame buffer of arbitrary resolution.
> >>
> >> While at it remove the pointless second reading of parameters from
> >> Xenstore in the device connection phase: all parameters are available
> >> during device probing already and that is where they should be read.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> ---
> >> V3: - merged the two patches
> >>     - read Xenstore parameters during probing of the device only
> > 
> > I guess 2 module parameters are not big deal, but could you tell me why
> > you can't always have them specified in Xenstore?
> 
> This will depend on Xen version then. I'd prefer to have a way to
> specify the resolution in case a new kernel is running as a guest
> of an old Xen version.

Who would be seeting up the kernel parameters in this case? User
manually in guest bootloader config?

> 
> > Also, I still think you are going in the wrong direction here. Can your
> > framebuffer size change after booting the guest? If it can, you have to
> > reconcile the new size and the coordinates reported by the pointing
> > device, and I think guest should be doing it. If you look, for example,
> > at vmmouse driver, they do not try to match coordinates it reports to
> > the screen.
> 
> I'm no expert in input driver interface. Can you tell me how the mouse
> pointer is positioned in case of the vmmouse driver? Are the real limits
> used just the minimum of pointing device and screen size limits? So
> specifying a size of 0xffff * 0xffff like the vmmouse driver does will
> work with any screen size being smaller than that?

I think 0xffff is just the limits for coordinates reported through this
interface; the expectation is that guest window size is not larger than
that. I do not recall if the backend reports real screen pointer
position, of offset from the (0,0) of guest's window, Thomas might tel
you. IIRC code in vmware tools package (that runs in guest) gets
notifications about screen changes and reacts accordingly.

Thanks.

-- 
Dmitry

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Juergen Gross]

On 12/04/17 18:24, Dmitry Torokhov wrote:
> On Wed, Apr 12, 2017 at 06:04:30PM +0200, Juergen Gross wrote:
>> On 12/04/17 17:16, Dmitry Torokhov wrote:
>>> Hi Juergen,
>>>
>>> On Tue, Apr 11, 2017 at 02:30:37PM +0200, Juergen Gross wrote:
>>>> Add a parameter for setting the resolution of xen-kbdfront in order to
>>>> be able to cope with a (virtual) frame buffer of arbitrary resolution.
>>>>
>>>> While at it remove the pointless second reading of parameters from
>>>> Xenstore in the device connection phase: all parameters are available
>>>> during device probing already and that is where they should be read.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V3: - merged the two patches
>>>>     - read Xenstore parameters during probing of the device only
>>>
>>> I guess 2 module parameters are not big deal, but could you tell me why
>>> you can't always have them specified in Xenstore?
>>
>> This will depend on Xen version then. I'd prefer to have a way to
>> specify the resolution in case a new kernel is running as a guest
>> of an old Xen version.
> 
> Who would be seeting up the kernel parameters in this case? User
> manually in guest bootloader config?

Either the guest administrator through guest boot loader or the Xen
administrator in the guest config file.

In the case where the problem has been reported (automatic tests of
graphical tools) this would be in the guest config file.

>>> Also, I still think you are going in the wrong direction here. Can your
>>> framebuffer size change after booting the guest? If it can, you have to
>>> reconcile the new size and the coordinates reported by the pointing
>>> device, and I think guest should be doing it. If you look, for example,
>>> at vmmouse driver, they do not try to match coordinates it reports to
>>> the screen.
>>
>> I'm no expert in input driver interface. Can you tell me how the mouse
>> pointer is positioned in case of the vmmouse driver? Are the real limits
>> used just the minimum of pointing device and screen size limits? So
>> specifying a size of 0xffff * 0xffff like the vmmouse driver does will
>> work with any screen size being smaller than that?
> 
> I think 0xffff is just the limits for coordinates reported through this
> interface; the expectation is that guest window size is not larger than
> that. I do not recall if the backend reports real screen pointer
> position, of offset from the (0,0) of guest's window, Thomas might tel
> you. IIRC code in vmware tools package (that runs in guest) gets
> notifications about screen changes and reacts accordingly.

So a small test revealed that only with a resolution matching that of
the virtual console the virtual mouse pointer will be at the same
position as the real mouse pointer. Working with different resolutions
will require some sort of scaling available only if _both_ resolutions
are known. And as you don't like the input driver knowing about the
console I'm limited to fixed values via module parameters (taking into
account the embedded scenario without udev).


Juergen

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Juergen Gross]

On 12/04/17 20:26, Juergen Gross wrote:
> On 12/04/17 18:24, Dmitry Torokhov wrote:
>> On Wed, Apr 12, 2017 at 06:04:30PM +0200, Juergen Gross wrote:
>>> On 12/04/17 17:16, Dmitry Torokhov wrote:
>>>> Hi Juergen,
>>>>
>>>> On Tue, Apr 11, 2017 at 02:30:37PM +0200, Juergen Gross wrote:
>>>>> Add a parameter for setting the resolution of xen-kbdfront in order to
>>>>> be able to cope with a (virtual) frame buffer of arbitrary resolution.
>>>>>
>>>>> While at it remove the pointless second reading of parameters from
>>>>> Xenstore in the device connection phase: all parameters are available
>>>>> during device probing already and that is where they should be read.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>> V3: - merged the two patches
>>>>>     - read Xenstore parameters during probing of the device only
>>>>
>>>> I guess 2 module parameters are not big deal, but could you tell me why
>>>> you can't always have them specified in Xenstore?
>>>
>>> This will depend on Xen version then. I'd prefer to have a way to
>>> specify the resolution in case a new kernel is running as a guest
>>> of an old Xen version.
>>
>> Who would be seeting up the kernel parameters in this case? User
>> manually in guest bootloader config?
> 
> Either the guest administrator through guest boot loader or the Xen
> administrator in the guest config file.
> 
> In the case where the problem has been reported (automatic tests of
> graphical tools) this would be in the guest config file.
> 
>>>> Also, I still think you are going in the wrong direction here. Can your
>>>> framebuffer size change after booting the guest? If it can, you have to
>>>> reconcile the new size and the coordinates reported by the pointing
>>>> device, and I think guest should be doing it. If you look, for example,
>>>> at vmmouse driver, they do not try to match coordinates it reports to
>>>> the screen.
>>>
>>> I'm no expert in input driver interface. Can you tell me how the mouse
>>> pointer is positioned in case of the vmmouse driver? Are the real limits
>>> used just the minimum of pointing device and screen size limits? So
>>> specifying a size of 0xffff * 0xffff like the vmmouse driver does will
>>> work with any screen size being smaller than that?
>>
>> I think 0xffff is just the limits for coordinates reported through this
>> interface; the expectation is that guest window size is not larger than
>> that. I do not recall if the backend reports real screen pointer
>> position, of offset from the (0,0) of guest's window, Thomas might tel
>> you. IIRC code in vmware tools package (that runs in guest) gets
>> notifications about screen changes and reacts accordingly.
> 
> So a small test revealed that only with a resolution matching that of
> the virtual console the virtual mouse pointer will be at the same
> position as the real mouse pointer. Working with different resolutions
> will require some sort of scaling available only if _both_ resolutions
> are known. And as you don't like the input driver knowing about the
> console I'm limited to fixed values via module parameters (taking into
> account the embedded scenario without udev).

Dmitry, are you okay with this now?

I'd _really_ like to get this in - I'm trying for 3 months now.


Juergen

Re: [PATCH v3] xen,input: add xen-kbdfront module parameter for setting resolution

[Dmitry Torokhov]

On Tue, Apr 11, 2017 at 02:30:37PM +0200, Juergen Gross wrote:
> Add a parameter for setting the resolution of xen-kbdfront in order to
> be able to cope with a (virtual) frame buffer of arbitrary resolution.
> 
> While at it remove the pointless second reading of parameters from
> Xenstore in the device connection phase: all parameters are available
> during device probing already and that is where they should be read.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Applied, thank you.

> ---
> V3: - merged the two patches
>     - read Xenstore parameters during probing of the device only
> ---
>  drivers/input/misc/xen-kbdfront.c | 39 ++++++++++++++-------------------------
>  1 file changed, 14 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> index 3900875dec10..2fc7895373ab 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -41,6 +41,12 @@ struct xenkbd_info {
>  	char phys[32];
>  };
>  
> +enum { KPARAM_X, KPARAM_Y, KPARAM_CNT };
> +static int ptr_size[KPARAM_CNT] = { XENFB_WIDTH, XENFB_HEIGHT };
> +module_param_array(ptr_size, int, NULL, 0444);
> +MODULE_PARM_DESC(ptr_size,
> +	"Pointing device width, height in pixels (default 800,600)");
> +
>  static int xenkbd_remove(struct xenbus_device *);
>  static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info *);
>  static void xenkbd_disconnect_backend(struct xenkbd_info *);
> @@ -128,7 +134,12 @@ static int xenkbd_probe(struct xenbus_device *dev,
>  	if (!info->page)
>  		goto error_nomem;
>  
> +	/* Set input abs params to match backend screen res */
>  	abs = xenbus_read_unsigned(dev->otherend, "feature-abs-pointer", 0);
> +	ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend, "width",
> +						  ptr_size[KPARAM_X]);
> +	ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend, "height",
> +						  ptr_size[KPARAM_Y]);
>  	if (abs) {
>  		ret = xenbus_write(XBT_NIL, dev->nodename,
>  				   "request-abs-pointer", "1");
> @@ -174,8 +185,8 @@ static int xenkbd_probe(struct xenbus_device *dev,
>  
>  	if (abs) {
>  		__set_bit(EV_ABS, ptr->evbit);
> -		input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
> -		input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
> +		input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
> +		input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
>  	} else {
>  		input_set_capability(ptr, EV_REL, REL_X);
>  		input_set_capability(ptr, EV_REL, REL_Y);
> @@ -309,9 +320,6 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *info)
>  static void xenkbd_backend_changed(struct xenbus_device *dev,
>  				   enum xenbus_state backend_state)
>  {
> -	struct xenkbd_info *info = dev_get_drvdata(&dev->dev);
> -	int ret, val;
> -
>  	switch (backend_state) {
>  	case XenbusStateInitialising:
>  	case XenbusStateInitialised:
> @@ -321,15 +329,6 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
>  		break;
>  
>  	case XenbusStateInitWait:
> -InitWait:
> -		if (xenbus_read_unsigned(info->xbdev->otherend,
> -					 "feature-abs-pointer", 0)) {
> -			ret = xenbus_write(XBT_NIL, info->xbdev->nodename,
> -					   "request-abs-pointer", "1");
> -			if (ret)
> -				pr_warning("xenkbd: can't request abs-pointer");
> -		}
> -
>  		xenbus_switch_state(dev, XenbusStateConnected);
>  		break;
>  
> @@ -340,17 +339,7 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
>  		 * get Connected twice here.
>  		 */
>  		if (dev->state != XenbusStateConnected)
> -			goto InitWait; /* no InitWait seen yet, fudge it */
> -
> -		/* Set input abs params to match backend screen res */
> -		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
> -				 "width", "%d", &val) > 0)
> -			input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0);
> -
> -		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
> -				 "height", "%d", &val) > 0)
> -			input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
> -
> +			xenbus_switch_state(dev, XenbusStateConnected);
>  		break;
>  
>  	case XenbusStateClosed:
> -- 
> 2.12.0
> 

-- 
Dmitry