diff mbox

[i-g-t,4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

Message ID 20170626135906.32708-4-paul.kocialkowski@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Kocialkowki June 26, 2017, 1:59 p.m. UTC
This adds support for reading a SuspendResumeDelay property (under
[DUT]) in the IGT configuration (igtrc) and exposing it through a
chamelium_get_suspend_resume_delay function.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
 lib/igt_chamelium.h |  1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

Comments

Martin Peres June 26, 2017, 2:25 p.m. UTC | #1
On 26/06/17 16:59, Paul Kocialkowski wrote:
> This adds support for reading a SuspendResumeDelay property (under
> [DUT]) in the IGT configuration (igtrc) and exposing it through a
> chamelium_get_suspend_resume_delay function.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>   lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
>   lib/igt_chamelium.h |  1 +
>   2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 225f98c3..a1aaf405 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -58,7 +58,7 @@
>    *	[Chamelium]
>    *	URL=http://chameleon:9992 # The URL used for connecting to the Chamelium's RPC server
>    *
> - *	# The rest of the sections are used for defining connector mappings.
> + *	# The following sections are used for defining connector mappings.
>    *	# This is required so any tests using the Chamelium know which connector
>    *	# on the test machine should be connected to each Chamelium port.
>    *	#
> @@ -70,12 +70,19 @@
>    *
>    *	[Chamelium:HDMI-A-1]
>    *	ChameliumPortID=3
> + *
> + *	# The following section is used for configuring the Device Under Test.
> + *	# It is not mandatory and allows overriding default values.
> + *	[DUT]
> + *	SuspendResumeDelay=10
>    * ]|
>    *
>    * By default, this file is expected to exist in ~/.igtrc . The directory for
>    * this can be overriden by setting the environment variable %IGT_CONFIG_PATH.
>    */
>   
> +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> +
>   struct chamelium_edid {
>   	int id;
>   	struct igt_list link;
> @@ -100,6 +107,7 @@ struct chamelium {
>   	xmlrpc_env env;
>   	xmlrpc_client *client;
>   	char *url;
> +	int suspend_resume_delay;
>   
>   	/* Indicates the last port to have been used for capturing video */
>   	struct chamelium_port *capturing_port;
> @@ -114,6 +122,20 @@ struct chamelium {
>   static struct chamelium *cleanup_instance;

Why do you make this part of the chamelium configuration? This should be 
common to all the suspend tests.
Paul Kocialkowki June 26, 2017, 2:33 p.m. UTC | #2
On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> On 26/06/17 16:59, Paul Kocialkowski wrote:
> > This adds support for reading a SuspendResumeDelay property (under
> > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > chamelium_get_suspend_resume_delay function.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > ---
> >   lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> >   lib/igt_chamelium.h |  1 +
> >   2 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index 225f98c3..a1aaf405 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -58,7 +58,7 @@
> >    *	[Chamelium]
> >    *	URL=http://chameleon:9992 # The URL used for connecting to the
> > Chamelium's RPC server
> >    *
> > - *	# The rest of the sections are used for defining connector
> > mappings.
> > + *	# The following sections are used for defining connector
> > mappings.
> >    *	# This is required so any tests using the Chamelium know which
> > connector
> >    *	# on the test machine should be connected to each Chamelium
> > port.
> >    *	#
> > @@ -70,12 +70,19 @@
> >    *
> >    *	[Chamelium:HDMI-A-1]
> >    *	ChameliumPortID=3
> > + *
> > + *	# The following section is used for configuring the Device Under
> > Test.
> > + *	# It is not mandatory and allows overriding default values.
> > + *	[DUT]
> > + *	SuspendResumeDelay=10
> >    * ]|
> >    *
> >    * By default, this file is expected to exist in ~/.igtrc . The directory
> > for
> >    * this can be overriden by setting the environment variable
> > %IGT_CONFIG_PATH.
> >    */
> >   
> > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > +
> >   struct chamelium_edid {
> >   	int id;
> >   	struct igt_list link;
> > @@ -100,6 +107,7 @@ struct chamelium {
> >   	xmlrpc_env env;
> >   	xmlrpc_client *client;
> >   	char *url;
> > +	int suspend_resume_delay;
> >   
> >   	/* Indicates the last port to have been used for capturing video
> > */
> >   	struct chamelium_port *capturing_port;
> > @@ -114,6 +122,20 @@ struct chamelium {
> >   static struct chamelium *cleanup_instance;
> 
> Why do you make this part of the chamelium configuration? This should be 
> common to all the suspend tests.

Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc and all
the parsing is implemented there.

So one course of action here would be to move igtrc parsing outside of
igt_chamelium (maybe to igt_core?) and use some global variable for storing this
value. What do you think?
Martin Peres June 26, 2017, 3:15 p.m. UTC | #3
On 26/06/17 17:33, Paul Kocialkowski wrote:
> On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
>> On 26/06/17 16:59, Paul Kocialkowski wrote:
>>> This adds support for reading a SuspendResumeDelay property (under
>>> [DUT]) in the IGT configuration (igtrc) and exposing it through a
>>> chamelium_get_suspend_resume_delay function.
>>>
>>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
>>> ---
>>>    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
>>>    lib/igt_chamelium.h |  1 +
>>>    2 files changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
>>> index 225f98c3..a1aaf405 100644
>>> --- a/lib/igt_chamelium.c
>>> +++ b/lib/igt_chamelium.c
>>> @@ -58,7 +58,7 @@
>>>     *	[Chamelium]
>>>     *	URL=http://chameleon:9992 # The URL used for connecting to the
>>> Chamelium's RPC server
>>>     *
>>> - *	# The rest of the sections are used for defining connector
>>> mappings.
>>> + *	# The following sections are used for defining connector
>>> mappings.
>>>     *	# This is required so any tests using the Chamelium know which
>>> connector
>>>     *	# on the test machine should be connected to each Chamelium
>>> port.
>>>     *	#
>>> @@ -70,12 +70,19 @@
>>>     *
>>>     *	[Chamelium:HDMI-A-1]
>>>     *	ChameliumPortID=3
>>> + *
>>> + *	# The following section is used for configuring the Device Under
>>> Test.
>>> + *	# It is not mandatory and allows overriding default values.
>>> + *	[DUT]
>>> + *	SuspendResumeDelay=10
>>>     * ]|
>>>     *
>>>     * By default, this file is expected to exist in ~/.igtrc . The directory
>>> for
>>>     * this can be overriden by setting the environment variable
>>> %IGT_CONFIG_PATH.
>>>     */
>>>    
>>> +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
>>> +
>>>    struct chamelium_edid {
>>>    	int id;
>>>    	struct igt_list link;
>>> @@ -100,6 +107,7 @@ struct chamelium {
>>>    	xmlrpc_env env;
>>>    	xmlrpc_client *client;
>>>    	char *url;
>>> +	int suspend_resume_delay;
>>>    
>>>    	/* Indicates the last port to have been used for capturing video
>>> */
>>>    	struct chamelium_port *capturing_port;
>>> @@ -114,6 +122,20 @@ struct chamelium {
>>>    static struct chamelium *cleanup_instance;
>>
>> Why do you make this part of the chamelium configuration? This should be
>> common to all the suspend tests.
> 
> Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc and all
> the parsing is implemented there.
> 
> So one course of action here would be to move igtrc parsing outside of
> igt_chamelium (maybe to igt_core?) and use some global variable for storing this
> value. What do you think?
> 

That seems like a plan, except you can just call a function that will do 
the parsing for you and return a structure with all the parameters. If 
it is called a second time, just return the cached value.

What do you think, Lyude?
Lyude Paul June 26, 2017, 9:14 p.m. UTC | #4
> > 
> > Fair enough, but at this point, only lib/igt_chamelium.c is using
> > igtrc and all
> > the parsing is implemented there.
> > 
> > So one course of action here would be to move igtrc parsing outside
> > of
> > igt_chamelium (maybe to igt_core?) and use some global variable for
> > storing this
> > value. What do you think?
> > 
> 
> That seems like a plan, except you can just call a function that will
> do 
> the parsing for you and return a structure with all the parameters.
> If 
> it is called a second time, just return the cached value.
> 
> What do you think, Lyude?

Yep, the reason it's called .igtrc is mainly because we were eventually
expecting someone else to start using it, and suspend/resume delay is
definitely something that will be useful for the rest of the tests
(I've got quite a few machines myself that i had to increase the delay
on just because the firmware was too slow when suspending/resuming). So
I'm all for it.
Paul Kocialkowki June 27, 2017, 7:33 a.m. UTC | #5
On Mon, 2017-06-26 at 18:15 +0300, Martin Peres wrote:
> On 26/06/17 17:33, Paul Kocialkowski wrote:
> > On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> > > On 26/06/17 16:59, Paul Kocialkowski wrote:
> > > > This adds support for reading a SuspendResumeDelay property (under
> > > > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > > > chamelium_get_suspend_resume_delay function.
> > > > 
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > > > ---
> > > >    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> > > >    lib/igt_chamelium.h |  1 +
> > > >    2 files changed, 31 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > index 225f98c3..a1aaf405 100644
> > > > --- a/lib/igt_chamelium.c
> > > > +++ b/lib/igt_chamelium.c
> > > > @@ -58,7 +58,7 @@
> > > >     *	[Chamelium]
> > > >     *	URL=http://chameleon:9992 # The URL used for connecting to
> > > > the
> > > > Chamelium's RPC server
> > > >     *
> > > > - *	# The rest of the sections are used for defining connector
> > > > mappings.
> > > > + *	# The following sections are used for defining connector
> > > > mappings.
> > > >     *	# This is required so any tests using the Chamelium know
> > > > which
> > > > connector
> > > >     *	# on the test machine should be connected to each Chamelium
> > > > port.
> > > >     *	#
> > > > @@ -70,12 +70,19 @@
> > > >     *
> > > >     *	[Chamelium:HDMI-A-1]
> > > >     *	ChameliumPortID=3
> > > > + *
> > > > + *	# The following section is used for configuring the Device
> > > > Under
> > > > Test.
> > > > + *	# It is not mandatory and allows overriding default values.
> > > > + *	[DUT]
> > > > + *	SuspendResumeDelay=10
> > > >     * ]|
> > > >     *
> > > >     * By default, this file is expected to exist in ~/.igtrc . The
> > > > directory
> > > > for
> > > >     * this can be overriden by setting the environment variable
> > > > %IGT_CONFIG_PATH.
> > > >     */
> > > >    
> > > > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > > > +
> > > >    struct chamelium_edid {
> > > >    	int id;
> > > >    	struct igt_list link;
> > > > @@ -100,6 +107,7 @@ struct chamelium {
> > > >    	xmlrpc_env env;
> > > >    	xmlrpc_client *client;
> > > >    	char *url;
> > > > +	int suspend_resume_delay;
> > > >    
> > > >    	/* Indicates the last port to have been used for capturing
> > > > video
> > > > */
> > > >    	struct chamelium_port *capturing_port;
> > > > @@ -114,6 +122,20 @@ struct chamelium {
> > > >    static struct chamelium *cleanup_instance;
> > > 
> > > Why do you make this part of the chamelium configuration? This should be
> > > common to all the suspend tests.
> > 
> > Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc and
> > all
> > the parsing is implemented there.
> > 
> > So one course of action here would be to move igtrc parsing outside of
> > igt_chamelium (maybe to igt_core?) and use some global variable for storing
> > this
> > value. What do you think?
> > 
> 
> That seems like a plan, except you can just call a function that will do 
> the parsing for you and return a structure with all the parameters. If 
> it is called a second time, just return the cached value.

Thinking about it twice, I think it would make a lot more sense to delegate (at
least some of) the parsing to specific parts of the code. For instance for
chamelium, the data structures are really only defined in igt_chamelium and I
don't think it would make a lot of sense to make those common just for the sake
of returning a common configuration structure. We could also have some
intermediate representation of that data for that structure, which isn't very
straightforward either.

So I would suggest making GKeyFile *igt_key_file; a global (instead of the
parsed parameters) so that specific parts of IGT can use it to get the keys they
want.

Then, having a common parsing function for common things (suspend/resume delay
would fit well there). Also, we already have igt_set_autoresume_delay so no
extra global variable would be needed for that (or rather, it already exists).

Does that seem agreeable?
Paul Kocialkowki June 27, 2017, 7:45 a.m. UTC | #6
On Tue, 2017-06-27 at 10:33 +0300, Paul Kocialkowski wrote:
> On Mon, 2017-06-26 at 18:15 +0300, Martin Peres wrote:
> > On 26/06/17 17:33, Paul Kocialkowski wrote:
> > > On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> > > > On 26/06/17 16:59, Paul Kocialkowski wrote:
> > > > > This adds support for reading a SuspendResumeDelay property (under
> > > > > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > > > > chamelium_get_suspend_resume_delay function.
> > > > > 
> > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > > > > ---
> > > > >    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> > > > >    lib/igt_chamelium.h |  1 +
> > > > >    2 files changed, 31 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > > index 225f98c3..a1aaf405 100644
> > > > > --- a/lib/igt_chamelium.c
> > > > > +++ b/lib/igt_chamelium.c
> > > > > @@ -58,7 +58,7 @@
> > > > >     *	[Chamelium]
> > > > >     *	URL=http://chameleon:9992 # The URL used for connecting
> > > > > to
> > > > > the
> > > > > Chamelium's RPC server
> > > > >     *
> > > > > - *	# The rest of the sections are used for defining connector
> > > > > mappings.
> > > > > + *	# The following sections are used for defining connector
> > > > > mappings.
> > > > >     *	# This is required so any tests using the Chamelium know
> > > > > which
> > > > > connector
> > > > >     *	# on the test machine should be connected to each
> > > > > Chamelium
> > > > > port.
> > > > >     *	#
> > > > > @@ -70,12 +70,19 @@
> > > > >     *
> > > > >     *	[Chamelium:HDMI-A-1]
> > > > >     *	ChameliumPortID=3
> > > > > + *
> > > > > + *	# The following section is used for configuring the Device
> > > > > Under
> > > > > Test.
> > > > > + *	# It is not mandatory and allows overriding default values.
> > > > > + *	[DUT]
> > > > > + *	SuspendResumeDelay=10
> > > > >     * ]|
> > > > >     *
> > > > >     * By default, this file is expected to exist in ~/.igtrc . The
> > > > > directory
> > > > > for
> > > > >     * this can be overriden by setting the environment variable
> > > > > %IGT_CONFIG_PATH.
> > > > >     */
> > > > >    
> > > > > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > > > > +
> > > > >    struct chamelium_edid {
> > > > >    	int id;
> > > > >    	struct igt_list link;
> > > > > @@ -100,6 +107,7 @@ struct chamelium {
> > > > >    	xmlrpc_env env;
> > > > >    	xmlrpc_client *client;
> > > > >    	char *url;
> > > > > +	int suspend_resume_delay;
> > > > >    
> > > > >    	/* Indicates the last port to have been used for capturing
> > > > > video
> > > > > */
> > > > >    	struct chamelium_port *capturing_port;
> > > > > @@ -114,6 +122,20 @@ struct chamelium {
> > > > >    static struct chamelium *cleanup_instance;
> > > > 
> > > > Why do you make this part of the chamelium configuration? This should be
> > > > common to all the suspend tests.
> > > 
> > > Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc
> > > and
> > > all
> > > the parsing is implemented there.
> > > 
> > > So one course of action here would be to move igtrc parsing outside of
> > > igt_chamelium (maybe to igt_core?) and use some global variable for
> > > storing
> > > this
> > > value. What do you think?
> > > 
> > 
> > That seems like a plan, except you can just call a function that will do 
> > the parsing for you and return a structure with all the parameters. If 
> > it is called a second time, just return the cached value.
> 
> Thinking about it twice, I think it would make a lot more sense to delegate
> (at
> least some of) the parsing to specific parts of the code. For instance for
> chamelium, the data structures are really only defined in igt_chamelium and I
> don't think it would make a lot of sense to make those common just for the
> sake
> of returning a common configuration structure. We could also have some
> intermediate representation of that data for that structure, which isn't very
> straightforward either.
> 
> So I would suggest making GKeyFile *igt_key_file; a global (instead of the
> parsed parameters) so that specific parts of IGT can use it to get the keys
> they
> want.
> 
> Then, having a common parsing function for common things (suspend/resume delay
> would fit well there). Also, we already have igt_set_autoresume_delay so no
> extra global variable would be needed for that (or rather, it already exists).
> 
> Does that seem agreeable?

By the way, I suggest sending this as a follow-up patch, since moving the config
out of IGT is a separate topic in itself. Would that make sense?
Paul Kocialkowki June 27, 2017, 10:16 a.m. UTC | #7
On Tue, 2017-06-27 at 10:45 +0300, Paul Kocialkowski wrote:
> On Tue, 2017-06-27 at 10:33 +0300, Paul Kocialkowski wrote:
> > On Mon, 2017-06-26 at 18:15 +0300, Martin Peres wrote:
> > > On 26/06/17 17:33, Paul Kocialkowski wrote:
> > > > On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> > > > > On 26/06/17 16:59, Paul Kocialkowski wrote:
> > > > > > This adds support for reading a SuspendResumeDelay property (under
> > > > > > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > > > > > chamelium_get_suspend_resume_delay function.
> > > > > > 
> > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > > > > > ---
> > > > > >    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> > > > > >    lib/igt_chamelium.h |  1 +
> > > > > >    2 files changed, 31 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > > > index 225f98c3..a1aaf405 100644
> > > > > > --- a/lib/igt_chamelium.c
> > > > > > +++ b/lib/igt_chamelium.c
> > > > > > @@ -58,7 +58,7 @@
> > > > > >     *	[Chamelium]
> > > > > >     *	URL=http://chameleon:9992 # The URL used for connecting
> > > > > > to
> > > > > > the
> > > > > > Chamelium's RPC server
> > > > > >     *
> > > > > > - *	# The rest of the sections are used for defining
> > > > > > connector
> > > > > > mappings.
> > > > > > + *	# The following sections are used for defining connector
> > > > > > mappings.
> > > > > >     *	# This is required so any tests using the Chamelium
> > > > > > know
> > > > > > which
> > > > > > connector
> > > > > >     *	# on the test machine should be connected to each
> > > > > > Chamelium
> > > > > > port.
> > > > > >     *	#
> > > > > > @@ -70,12 +70,19 @@
> > > > > >     *
> > > > > >     *	[Chamelium:HDMI-A-1]
> > > > > >     *	ChameliumPortID=3
> > > > > > + *
> > > > > > + *	# The following section is used for configuring the
> > > > > > Device
> > > > > > Under
> > > > > > Test.
> > > > > > + *	# It is not mandatory and allows overriding default
> > > > > > values.
> > > > > > + *	[DUT]
> > > > > > + *	SuspendResumeDelay=10
> > > > > >     * ]|
> > > > > >     *
> > > > > >     * By default, this file is expected to exist in ~/.igtrc . The
> > > > > > directory
> > > > > > for
> > > > > >     * this can be overriden by setting the environment variable
> > > > > > %IGT_CONFIG_PATH.
> > > > > >     */
> > > > > >    
> > > > > > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > > > > > +
> > > > > >    struct chamelium_edid {
> > > > > >    	int id;
> > > > > >    	struct igt_list link;
> > > > > > @@ -100,6 +107,7 @@ struct chamelium {
> > > > > >    	xmlrpc_env env;
> > > > > >    	xmlrpc_client *client;
> > > > > >    	char *url;
> > > > > > +	int suspend_resume_delay;
> > > > > >    
> > > > > >    	/* Indicates the last port to have been used for
> > > > > > capturing
> > > > > > video
> > > > > > */
> > > > > >    	struct chamelium_port *capturing_port;
> > > > > > @@ -114,6 +122,20 @@ struct chamelium {
> > > > > >    static struct chamelium *cleanup_instance;
> > > > > 
> > > > > Why do you make this part of the chamelium configuration? This should
> > > > > be
> > > > > common to all the suspend tests.
> > > > 
> > > > Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc
> > > > and
> > > > all
> > > > the parsing is implemented there.
> > > > 
> > > > So one course of action here would be to move igtrc parsing outside of
> > > > igt_chamelium (maybe to igt_core?) and use some global variable for
> > > > storing
> > > > this
> > > > value. What do you think?
> > > > 
> > > 
> > > That seems like a plan, except you can just call a function that will do 
> > > the parsing for you and return a structure with all the parameters. If 
> > > it is called a second time, just return the cached value.
> > 
> > Thinking about it twice, I think it would make a lot more sense to delegate
> > (at
> > least some of) the parsing to specific parts of the code. For instance for
> > chamelium, the data structures are really only defined in igt_chamelium and
> > I
> > don't think it would make a lot of sense to make those common just for the
> > sake
> > of returning a common configuration structure. We could also have some
> > intermediate representation of that data for that structure, which isn't
> > very
> > straightforward either.
> > 
> > So I would suggest making GKeyFile *igt_key_file; a global (instead of the
> > parsed parameters) so that specific parts of IGT can use it to get the keys
> > they
> > want.
> > 
> > Then, having a common parsing function for common things (suspend/resume
> > delay
> > would fit well there). Also, we already have igt_set_autoresume_delay so no
> > extra global variable would be needed for that (or rather, it already
> > exists).
> > 
> > Does that seem agreeable?
> 
> By the way, I suggest sending this as a follow-up patch, since moving the
> config out of IGT is a separate topic in itself. Would that make sense?

Or maybe not, it's way too unreadable to do this in many patches, where things
get added and removed later. I'm crafting v2 as a single patch with the proposal
I have in mind. Let's follow-up the discussing there.
diff mbox

Patch

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 225f98c3..a1aaf405 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -58,7 +58,7 @@ 
  *	[Chamelium]
  *	URL=http://chameleon:9992 # The URL used for connecting to the Chamelium's RPC server
  *
- *	# The rest of the sections are used for defining connector mappings.
+ *	# The following sections are used for defining connector mappings.
  *	# This is required so any tests using the Chamelium know which connector
  *	# on the test machine should be connected to each Chamelium port.
  *	#
@@ -70,12 +70,19 @@ 
  *
  *	[Chamelium:HDMI-A-1]
  *	ChameliumPortID=3
+ *
+ *	# The following section is used for configuring the Device Under Test.
+ *	# It is not mandatory and allows overriding default values.
+ *	[DUT]
+ *	SuspendResumeDelay=10
  * ]|
  *
  * By default, this file is expected to exist in ~/.igtrc . The directory for
  * this can be overriden by setting the environment variable %IGT_CONFIG_PATH.
  */
 
+#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
+
 struct chamelium_edid {
 	int id;
 	struct igt_list link;
@@ -100,6 +107,7 @@  struct chamelium {
 	xmlrpc_env env;
 	xmlrpc_client *client;
 	char *url;
+	int suspend_resume_delay;
 
 	/* Indicates the last port to have been used for capturing video */
 	struct chamelium_port *capturing_port;
@@ -114,6 +122,20 @@  struct chamelium {
 static struct chamelium *cleanup_instance;
 
 /**
+ * chamelium_get_suspend_resume_delay:
+ * @chamelium: The Chamelium instance to use
+ *
+ * Retrieves the suspend/resume delay as specified in the configuration or
+ * its default value.
+ *
+ * Returns: the suspend/resume delay in seconds
+ */
+int chamelium_get_suspend_resume_delay(struct chamelium *chamelium)
+{
+	return chamelium->suspend_resume_delay;
+}
+
+/**
  * chamelium_get_ports:
  * @chamelium: The Chamelium instance to use
  * @count: Where to store the number of ports
@@ -1157,6 +1179,13 @@  static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 		goto out;
 	}
 
+	rc = g_key_file_get_integer(key_file, "DUT", "SuspendResumeDelay",
+				    &error);
+	if (rc == 0)
+		chamelium->suspend_resume_delay = SUSPEND_RESUME_DELAY_DEFAULT;
+	else
+		chamelium->suspend_resume_delay = rc;
+
 	ret = chamelium_read_port_mappings(chamelium, drm_fd, key_file);
 
 out:
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index 81322ad2..380f9b36 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -41,6 +41,7 @@  struct chamelium *chamelium_init(int drm_fd);
 void chamelium_deinit(struct chamelium *chamelium);
 void chamelium_reset(struct chamelium *chamelium);
 
+int chamelium_get_suspend_resume_delay(struct chamelium *chamelium);
 struct chamelium_port **chamelium_get_ports(struct chamelium *chamelium,
 					    int *count);
 unsigned int chamelium_port_get_type(const struct chamelium_port *port);