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

[Paul Kocialkowski <paul.kocialkowski@linux.intel.com>]

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.

-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx