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

[Paul Kocialkowski]

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 at 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 Kocialkowski <paul.kocialkowski at linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo