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@xxxxxxxxxxxxxxx> > > > > > --- > > > > > 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@xxxxxxxxxxxxxxx> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
