Re: [PATCH] remove dead code in dummy driver

[Bob Terek]

On 09/27/2016 12:22 AM, Antoine Martin wrote:


It think it probably makes sense to apply the cleanup patches first, to
remove code before adding some more?


I had planned to send them all at once, but you're right, it is a good 
idea to get the cleanups in separately.


My problem has been that my day job has been getting in the way so far, 
and now I'm leaving for a 3 week vacation in a few hours. I'd be happy 
to address the cleanups around the 24th, if that is ok, and my proposed 
change shortly thereafter. I'm sure Aaron has better things to do than 
deal with dummy.


Cheers,

Bob Terek
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Antoine Martin]

On 24/09/16 00:20, Aaron Plattner wrote:
> On 09/22/2016 04:30 PM, Bob Terek wrote:
>> On 09/21/2016 10:22 AM, Aaron Plattner wrote:
>>> On 09/20/2016 02:07 AM, Eric Engestrom wrote:
 On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
> Signed-off-by: Antoine Martin 

 Reviewed-by: Eric Engestrom 
>>>
>>> Looks good to me too (although I'm cheating since this chunk is
>>> identical to part of
>>> https://patchwork.freedesktop.org/patch/41058/)
>>
>> Shouldn't the first 5 of Aaron's patches be applied, since they are all
>> cleanup items?
>>
>> https://lists.x.org/archives/xorg-devel/2015-January/045395.html
> 
> I never pushed them because they were never reviewed. Would it help if I
> resent them?
Yes, please re-send and I'll make sure to test and review this week.

>> Patch 6 supposedly caused a server crash, but the first 5 should be ok?
> 
> Patch 6 was kind of controversial so I don't know if we want it anyway.
IIRC, I was the one who reported a crash when I tested it - I didn't
investigate it further.

Sounds like Bob Terek's approach is much more complete anyway.

Cheers
Antoine



> 
>> -- 
>> Bob Terek
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Antoine Martin]

On 25/09/16 18:52, Bob Terek wrote:
> 
> On 09/23/2016 07:20 AM, Aaron Plattner wrote:
> 
>> On 09/22/2016 04:30 PM, Bob Terek wrote:
> 
>>> Shouldn't the first 5 of Aaron's patches be applied, since they are all
>>> cleanup items?
>>>
>>> https://lists.x.org/archives/xorg-devel/2015-January/045395.html
>>
>> I never pushed them because they were never reviewed. Would it help if I
>> resent them?
> 
> Why don't you hold off for a bit. . .
> 
> 
>>> Patch 6 supposedly caused a server crash, but the first 5 should be ok?
>>
>> Patch 6 was kind of controversial so I don't know if we want it anyway.
> 
> Welp, on that topic, your patch 6 was an alternative approach to support
> arbitrary pixel dimensions for the framebuffer. The more conventional
> approach had been posted by Boichat,
> https://lists.x.org/archives/xorg-devel/2014-November/044580.html . I'd
> like to suggest that Boichat's approach be used, but extended even further:
> 
> In 2014 I had also modified the dummy driver while working with Teradici
> Corporation to support not only arbitrary pixel dimensions, but also
> multiple monitors. The latter feature might not make sense to some
> folks, but if you have a server-side Xserver mapped to a hardware thin
> client sitting across a network, which has multiple physical monitors
> attached, you want the Xserver to be configured in the exact same manner
> as the tin client. Even though there is just a virtual framebuffer in
> main memory, X applications need to know the number/size/location of
> monitors so that toolbars are placed properly, windows are fullscreen'ed
> properly, etc. And when the user moves from one hardware thin client to
> another, which may have a different monitor configuration, the Xserver
> session needs to change to that configuration.
> 
> At Teradici we made dummy be RandR 1.2 compliant in a manner similar to
> Boichat, but added xorg.conf parameters to specify how many CRTC/Outputs
> should be created at startup. Thus the configuration could be
> manipulated via the standard RandR API (either programatically or
> through the xrandr command). This might happen when the hardware thin
> client connects to an existing X session, and server-side session
> management software will issue what is needed to reconfigure the
> Xserver. The end result is that the hardware thin client could be
> supported by multiple CRTC/Output's, with typical VESA modes, but a
> software thin client could use arbitrary pixel dimensions on a single
> CRTC/Output to map exactly the size of the window it was running in.
> 
> Teradici and I would like to submit these patches to dummy to support
> the functionality that many want. The dummy driver is so simple that I
> don't see this as overly complex. Yes, it is a bit more code to add, but
> it is very straightforward, nothing at all surprising. And it is
> backwards compatible, the multiple CRTC/Output support does not have to
> be utilized, the default is 1. Our modified driver behaves just like the
> current one, except that arbitrary modes may be added and switched to
> via the RandR protocol.
> 
> I can post these patches, and include Aaron's cleanups, if folks will
> support this approach. It has been tested via Teradici for over a year
> and seems to be stable. I am in the process of updating the code to the
> current version and so far so good.
> 
> Thoughts?
That sounds great, I'd be happy to test and review those changes.
It think it probably makes sense to apply the cleanup patches first, to
remove code before adding some more?
These RandR bits probably warrant bumping the version number too, 0.4.0?

I was going to add comments to Aaron's cleanup patches, but patchwork
won't let me add comments.

Cheers
Antoine


> 
> Thanks,
> 
> Bob Terek
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Bob Terek]

On 09/23/2016 07:20 AM, Aaron Plattner wrote:


On 09/22/2016 04:30 PM, Bob Terek wrote:



Shouldn't the first 5 of Aaron's patches be applied, since they are all
cleanup items?

https://lists.x.org/archives/xorg-devel/2015-January/045395.html


I never pushed them because they were never reviewed. Would it help if I
resent them?


Why don't you hold off for a bit. . .



Patch 6 supposedly caused a server crash, but the first 5 should be ok?


Patch 6 was kind of controversial so I don't know if we want it anyway.


Welp, on that topic, your patch 6 was an alternative approach to support 
arbitrary pixel dimensions for the framebuffer. The more conventional 
approach had been posted by Boichat, 
https://lists.x.org/archives/xorg-devel/2014-November/044580.html . I'd 
like to suggest that Boichat's approach be used, but extended even further:


In 2014 I had also modified the dummy driver while working with Teradici 
Corporation to support not only arbitrary pixel dimensions, but also 
multiple monitors. The latter feature might not make sense to some 
folks, but if you have a server-side Xserver mapped to a hardware thin 
client sitting across a network, which has multiple physical monitors 
attached, you want the Xserver to be configured in the exact same manner 
as the tin client. Even though there is just a virtual framebuffer in 
main memory, X applications need to know the number/size/location of 
monitors so that toolbars are placed properly, windows are fullscreen'ed 
properly, etc. And when the user moves from one hardware thin client to 
another, which may have a different monitor configuration, the Xserver 
session needs to change to that configuration.


At Teradici we made dummy be RandR 1.2 compliant in a manner similar to 
Boichat, but added xorg.conf parameters to specify how many CRTC/Outputs 
should be created at startup. Thus the configuration could be 
manipulated via the standard RandR API (either programatically or 
through the xrandr command). This might happen when the hardware thin 
client connects to an existing X session, and server-side session 
management software will issue what is needed to reconfigure the 
Xserver. The end result is that the hardware thin client could be 
supported by multiple CRTC/Output's, with typical VESA modes, but a 
software thin client could use arbitrary pixel dimensions on a single 
CRTC/Output to map exactly the size of the window it was running in.


Teradici and I would like to submit these patches to dummy to support 
the functionality that many want. The dummy driver is so simple that I 
don't see this as overly complex. Yes, it is a bit more code to add, but 
it is very straightforward, nothing at all surprising. And it is 
backwards compatible, the multiple CRTC/Output support does not have to 
be utilized, the default is 1. Our modified driver behaves just like the 
current one, except that arbitrary modes may be added and switched to 
via the RandR protocol.


I can post these patches, and include Aaron's cleanups, if folks will 
support this approach. It has been tested via Teradici for over a year 
and seems to be stable. I am in the process of updating the code to the 
current version and so far so good.


Thoughts?

Thanks,

Bob Terek
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Aaron Plattner]

On 09/22/2016 04:30 PM, Bob Terek wrote:
> On 09/21/2016 10:22 AM, Aaron Plattner wrote:
>> On 09/20/2016 02:07 AM, Eric Engestrom wrote:
>>> On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
 Signed-off-by: Antoine Martin 
>>>
>>> Reviewed-by: Eric Engestrom 
>>
>> Looks good to me too (although I'm cheating since this chunk is
>> identical to part of
>> https://patchwork.freedesktop.org/patch/41058/)
> 
> Shouldn't the first 5 of Aaron's patches be applied, since they are all
> cleanup items?
> 
> https://lists.x.org/archives/xorg-devel/2015-January/045395.html

I never pushed them because they were never reviewed. Would it help if I
resent them?

> Patch 6 supposedly caused a server crash, but the first 5 should be ok?

Patch 6 was kind of controversial so I don't know if we want it anyway.

> -- 
> Bob Terek
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Bob Terek]

On 09/21/2016 10:22 AM, Aaron Plattner wrote:

On 09/20/2016 02:07 AM, Eric Engestrom wrote:

On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:

Signed-off-by: Antoine Martin 


Reviewed-by: Eric Engestrom 


Looks good to me too (although I'm cheating since this chunk is identical to 
part of
https://patchwork.freedesktop.org/patch/41058/)


Shouldn't the first 5 of Aaron's patches be applied, since they are all 
cleanup items?


https://lists.x.org/archives/xorg-devel/2015-January/045395.html

Patch 6 supposedly caused a server crash, but the first 5 should be ok?

--
Bob Terek

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Antoine Martin]

On 22/09/16 03:22, Aaron Plattner wrote:
> On 09/20/2016 02:07 AM, Eric Engestrom wrote:
>> On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
>>> Signed-off-by: Antoine Martin 
>>
>> Reviewed-by: Eric Engestrom 
> 
> Looks good to me too (although I'm cheating since this chunk is identical to 
> part of https://patchwork.freedesktop.org/patch/41058/)
That one is even better.

You can add to that one:
Reviewed-by: Antoine Martin 

Cheers
Antoine



> 
> Pushed:
> remote: Updating patchwork state for 
> https://patchwork.freedesktop.org/project/Xorg/list/
> remote: I: patch #111435 updated using rev 
> 367c778240b4266958f33cec3653d5389e283557.
> remote: I: 1 patch(es) updated to state Accepted.
> To git+ssh://git.freedesktop.org/git/xorg/driver/xf86-video-dummy
>8706f60ab457..367c778240b4  HEAD -> master
> 
>>> ---
>>>  src/dummy_driver.c | 19 ---
>>>  1 file changed, 19 deletions(-)
>>>
>>> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
>>> index c84000f..ec1acf3 100644
>>> --- a/src/dummy_driver.c
>>> +++ b/src/dummy_driver.c
>>> @@ -700,25 +700,6 @@ DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
>>>  void
>>>  DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
>>>  {
>>> -SCRN_INFO_PTR(arg);
>>> -int Base; 
>>> -
>>> -Base = (y * pScrn->displayWidth + x) >> 2;
>>> -
>>> -/* Scale Base by the number of bytes per pixel. */
>>> -switch (pScrn->depth) {
>>> -case  8 :
>>> -   break;
>>> -case 15 :
>>> -case 16 :
>>> -   Base *= 2;
>>> -   break;
>>> -case 24 :
>>> -   Base *= 3;
>>> -   break;
>>> -default :
>>> -   break;
>>> -}
>>>  }
>>>  
>>>  /* Mandatory */
>>> -- 
>>> 2.7.4
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Aaron Plattner]

On 09/20/2016 02:07 AM, Eric Engestrom wrote:
> On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
>> Signed-off-by: Antoine Martin 
> 
> Reviewed-by: Eric Engestrom 

Looks good to me too (although I'm cheating since this chunk is identical to 
part of https://patchwork.freedesktop.org/patch/41058/)

Pushed:
remote: Updating patchwork state for 
https://patchwork.freedesktop.org/project/Xorg/list/
remote: I: patch #111435 updated using rev 
367c778240b4266958f33cec3653d5389e283557.
remote: I: 1 patch(es) updated to state Accepted.
To git+ssh://git.freedesktop.org/git/xorg/driver/xf86-video-dummy
   8706f60ab457..367c778240b4  HEAD -> master

>> ---
>>  src/dummy_driver.c | 19 ---
>>  1 file changed, 19 deletions(-)
>>
>> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
>> index c84000f..ec1acf3 100644
>> --- a/src/dummy_driver.c
>> +++ b/src/dummy_driver.c
>> @@ -700,25 +700,6 @@ DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
>>  void
>>  DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
>>  {
>> -SCRN_INFO_PTR(arg);
>> -int Base; 
>> -
>> -Base = (y * pScrn->displayWidth + x) >> 2;
>> -
>> -/* Scale Base by the number of bytes per pixel. */
>> -switch (pScrn->depth) {
>> -case  8 :
>> -break;
>> -case 15 :
>> -case 16 :
>> -Base *= 2;
>> -break;
>> -case 24 :
>> -Base *= 3;
>> -break;
>> -default :
>> -break;
>> -}
>>  }
>>  
>>  /* Mandatory */
>> -- 
>> 2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Eric Engestrom]

On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
> Signed-off-by: Antoine Martin 

Reviewed-by: Eric Engestrom 

> ---
>  src/dummy_driver.c | 19 ---
>  1 file changed, 19 deletions(-)
> 
> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
> index c84000f..ec1acf3 100644
> --- a/src/dummy_driver.c
> +++ b/src/dummy_driver.c
> @@ -700,25 +700,6 @@ DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
>  void
>  DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
>  {
> -SCRN_INFO_PTR(arg);
> -int Base; 
> -
> -Base = (y * pScrn->displayWidth + x) >> 2;
> -
> -/* Scale Base by the number of bytes per pixel. */
> -switch (pScrn->depth) {
> -case  8 :
> - break;
> -case 15 :
> -case 16 :
> - Base *= 2;
> - break;
> -case 24 :
> - Base *= 3;
> - break;
> -default :
> - break;
> -}
>  }
>  
>  /* Mandatory */
> -- 
> 2.7.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] remove dead code in dummy driver

[Antoine Martin]

Signed-off-by: Antoine Martin 
---
 src/dummy_driver.c | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index c84000f..ec1acf3 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -700,25 +700,6 @@ DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
 void
 DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
 {
-SCRN_INFO_PTR(arg);
-int Base; 
-
-Base = (y * pScrn->displayWidth + x) >> 2;
-
-/* Scale Base by the number of bytes per pixel. */
-switch (pScrn->depth) {
-case  8 :
-   break;
-case 15 :
-case 16 :
-   Base *= 2;
-   break;
-case 24 :
-   Base *= 3;
-   break;
-default :
-   break;
-}
 }
 
 /* Mandatory */
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel