| Message ID | 20170605132840.9125-3-liviu.dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 05, 2017 at 02:28:38PM +0100, Liviu Dudau wrote: > From: Brian Starkey <brian.starkey@arm.com> > > igt_display_commit isn't refreshing all outputs anymore, which means > that an override mode may never get picked up. > > Instead of forcing a reprobe to handle copying the override_mode into > default_mode, just change igt_output_get_mode() to return the > override_mode if it's been set, and remove the old code which would > directly overwrite default_mode. > > This should be more robust, as igt_output_get_mode() will always return > the correct mode, without needing the output to be reprobed. > > This change means that output->config.default_mode always contains the > "non-overridden" default mode. > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > lib/igt_kms.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 5e2ef97b..6d60a14f 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -2821,7 +2821,10 @@ const char *igt_output_name(igt_output_t *output) > > drmModeModeInfo *igt_output_get_mode(igt_output_t *output) > { > - return &output->config.default_mode; > + if (output->use_override_mode) > + return &output->override_mode; > + else > + return &output->config.default_mode; > } > > /** > @@ -2839,10 +2842,6 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode) > > if (mode) > output->override_mode = *mode; > - else /* restore default_mode, may have been overwritten in igt_output_refresh */ The warning caught my eye. From what I understand, the idea for this change is that: * we should not ever go for output->cofnig.default_mode if the default mode is not what we really want. * we should use the igt_output_get_mode() to get the current mode (i.e. either the default or the overriden) And yet igt_output_refresh() has this: if (output->use_override_mode) output->config.default_mode = output->override_mode; Am I missing something or is this an oversight?
On Tue, Jun 06, 2017 at 01:30:28PM +0300, Arkadiusz Hiler wrote: > On Mon, Jun 05, 2017 at 02:28:38PM +0100, Liviu Dudau wrote: > > From: Brian Starkey <brian.starkey@arm.com> > > > > igt_display_commit isn't refreshing all outputs anymore, which means > > that an override mode may never get picked up. > > > > Instead of forcing a reprobe to handle copying the override_mode into > > default_mode, just change igt_output_get_mode() to return the > > override_mode if it's been set, and remove the old code which would > > directly overwrite default_mode. > > > > This should be more robust, as igt_output_get_mode() will always return > > the correct mode, without needing the output to be reprobed. > > > > This change means that output->config.default_mode always contains the > > "non-overridden" default mode. > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > --- > > lib/igt_kms.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > > index 5e2ef97b..6d60a14f 100644 > > --- a/lib/igt_kms.c > > +++ b/lib/igt_kms.c > > @@ -2821,7 +2821,10 @@ const char *igt_output_name(igt_output_t *output) > > > > drmModeModeInfo *igt_output_get_mode(igt_output_t *output) > > { > > - return &output->config.default_mode; > > + if (output->use_override_mode) > > + return &output->override_mode; > > + else > > + return &output->config.default_mode; > > } > > > > /** > > @@ -2839,10 +2842,6 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode) > > > > if (mode) > > output->override_mode = *mode; > > - else /* restore default_mode, may have been overwritten in igt_output_refresh */ > > The warning caught my eye. > > From what I understand, the idea for this change is that: > * we should not ever go for output->cofnig.default_mode if the default > mode is not what we really want. > * we should use the igt_output_get_mode() to get the current mode (i.e. > either the default or the overriden) > > And yet igt_output_refresh() has this: > > if (output->use_override_mode) > output->config.default_mode = output->override_mode; > > > Am I missing something or is this an oversight? Hi Arek, An oversight, will fix in the next version. Thanks for reviewing this! Liviu > > -- > Cheers, > Arek > > > - kmstest_get_connector_default_mode(output->display->drm_fd, > > - output->config.connector, > > - &output->config.default_mode); > > > > output->use_override_mode = !!mode; > > > > -- > > 2.13.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 5e2ef97b..6d60a14f 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -2821,7 +2821,10 @@ const char *igt_output_name(igt_output_t *output) drmModeModeInfo *igt_output_get_mode(igt_output_t *output) { - return &output->config.default_mode; + if (output->use_override_mode) + return &output->override_mode; + else + return &output->config.default_mode; } /** @@ -2839,10 +2842,6 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode) if (mode) output->override_mode = *mode; - else /* restore default_mode, may have been overwritten in igt_output_refresh */ - kmstest_get_connector_default_mode(output->display->drm_fd, - output->config.connector, - &output->config.default_mode); output->use_override_mode = !!mode;