Re: [PATCH 2/4] lib/igt_kms: Fix override_mode handling

[Liviu Dudau <liviu.dudau@arm.com>]

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx