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

[Arkadiusz Hiler <arkadiusz.hiler@intel.com>]

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?

-- 
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx